Merge pull request #9398 from tthvo/OCPBUGS-48089

OCPBUGS-48089: validate hostPrefix to be the same when multiple clusternetwork CIDRs are present
openshift · Jan 31, 2025 · 1c141f0 · 1c141f0
2 parents 1e5a97e + 4b12df9
commit 1c141f0
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 7 deletions.
diff --git a/data/data/install.openshift.io_installconfigs.yaml b/data/data/install.openshift.io_installconfigs.yaml
@@ -2442,6 +2442,8 @@ spec:
                         HostPrefix is the prefix size to allocate to each node from the CIDR.
                         For example, 24 would allocate 2^8=256 adresses to each node. If this
                         field is not used by the plugin, it can be left unset.
+                        When multiple CIDRs of the same family (i.e. IPv4/IPv6) are present,
+                        their HostPrefix value must be the same.
                       format: int32
                       type: integer
                     hostSubnetLength:
@@ -2477,6 +2479,8 @@ spec:
                         HostPrefix is the prefix size to allocate to each node from the CIDR.
                         For example, 24 would allocate 2^8=256 adresses to each node. If this
                         field is not used by the plugin, it can be left unset.
+                        When multiple CIDRs of the same family (i.e. IPv4/IPv6) are present,
+                        their HostPrefix value must be the same.
                       format: int32
                       type: integer
                     hostSubnetLength:

diff --git a/docs/user/customization.md b/docs/user/customization.md
@@ -45,7 +45,7 @@ feature set.
         The default is 10.128.0.0/14 with a host prefix of /23.
         * `cidr` (required [IP network](#ip-networks)): The IP block address pool.
         * `hostPrefix` (required integer): The prefix size to allocate to each node from the CIDR.
-        For example, 24 would allocate 2^8=256 addresses to each node. If this field is not used by the plugin, it can be left unset.
+        For example, 24 would allocate 2^8=256 addresses to each node. If this field is not used by the plugin, it can be left unset. When multiple CIDRs of the same family (i.e. IPv4/IPv6) are present, their `hostPrefix` value must be the same.
     * `machineNetwork` (optional array of objects): The IP address pools for machines.
         * `cidr` (required [IP network](#ip-networks)): The IP block address pool.
             The default is 10.0.0.0/16 for all platforms other than libvirt.

diff --git a/pkg/types/installconfig.go b/pkg/types/installconfig.go
@@ -441,6 +441,8 @@ type ClusterNetworkEntry struct {
 	// HostPrefix is the prefix size to allocate to each node from the CIDR.
 	// For example, 24 would allocate 2^8=256 adresses to each node. If this
 	// field is not used by the plugin, it can be left unset.
+	// When multiple CIDRs of the same family (i.e. IPv4/IPv6) are present,
+	// their HostPrefix value must be the same.
 	// +optional
 	HostPrefix int32 `json:"hostPrefix,omitempty"`
 

diff --git a/pkg/types/validation/installconfig.go b/pkg/types/validation/installconfig.go
@@ -502,13 +502,22 @@ func validateClusterNetwork(n *types.Networking, cn *types.ClusterNetworkEntry,
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("cidr"), cn.CIDR.String(), fmt.Sprintf("cluster network must not overlap with cluster network %d", i)))
 		}
 	}
-	if cn.HostPrefix < 0 {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("hostPrefix"), cn.HostPrefix, "hostPrefix must be positive"))
-	}
+
 	// ignore hostPrefix if the plugin does not use it and has it unset
 	if pluginsUsingHostPrefix.Has(n.NetworkType) || (cn.HostPrefix != 0) {
-		if ones, bits := cn.CIDR.Mask.Size(); cn.HostPrefix < int32(ones) {
+		if cn.HostPrefix < 0 {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("hostPrefix"), cn.HostPrefix, "hostPrefix must be positive"))
+		} else if ones, bits := cn.CIDR.Mask.Size(); cn.HostPrefix < int32(ones) {
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("hostPrefix"), cn.HostPrefix, "cluster network host subnetwork prefix must not be larger size than CIDR "+cn.CIDR.String()))
+		} else if bits == 32 {
+			// setting different value for clusternetwork CIDR host prefix is not allowed
+			// we only need to check IPv4 as IPv6 prefix must be 64
+			for _, acn := range n.ClusterNetwork[0:idx] {
+				if acn.CIDR.IP.To4() != nil && cn.HostPrefix != acn.HostPrefix {
+					allErrs = append(allErrs, field.Invalid(fldPath.Child("hostPrefix"), cn.HostPrefix, "cluster network host subnetwork prefix must be the same value for IPv4 networks"))
+					break
+				}
+			}
 		} else if bits == 128 && cn.HostPrefix != 64 {
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("hostPrefix"), cn.HostPrefix, "cluster network host subnetwork prefix must be 64 for IPv6 networks"))
 		}

diff --git a/pkg/types/validation/installconfig_test.go b/pkg/types/validation/installconfig_test.go
@@ -516,8 +516,8 @@ func TestValidateInstallConfig(t *testing.T) {
 			installConfig: func() *types.InstallConfig {
 				c := validInstallConfig()
 				c.Networking.ClusterNetwork = []types.ClusterNetworkEntry{
-					{CIDR: *ipnet.MustParseCIDR("12.0.0.0/16"), HostPrefix: 23},
-					{CIDR: *ipnet.MustParseCIDR("12.0.3.0/24"), HostPrefix: 25},
+					{CIDR: *ipnet.MustParseCIDR("12.0.0.0/16"), HostPrefix: 28},
+					{CIDR: *ipnet.MustParseCIDR("12.0.3.0/24"), HostPrefix: 28},
 				}
 				return c
 			}(),
@@ -554,6 +554,33 @@ func TestValidateInstallConfig(t *testing.T) {
 			}(),
 			expectedError: ``,
 		},
+		{
+			name: "cluster network host prefix negative",
+			installConfig: func() *types.InstallConfig {
+				c := validInstallConfig()
+				c.Networking.ClusterNetwork[0].HostPrefix = -23
+				return c
+			}(),
+			expectedError: `^networking\.clusterNetwork\[0]\.hostPrefix: Invalid value: -23: hostPrefix must be positive$`,
+		},
+		{
+			name: "multiple cluster network host prefix different",
+			installConfig: func() *types.InstallConfig {
+				c := validInstallConfig()
+				c.Networking.ClusterNetwork = append(c.Networking.ClusterNetwork,
+					types.ClusterNetworkEntry{
+						CIDR:       *ipnet.MustParseCIDR("192.168.2.0/24"),
+						HostPrefix: 30,
+					},
+					types.ClusterNetworkEntry{
+						CIDR:       *ipnet.MustParseCIDR("ffd2::/48"),
+						HostPrefix: 64,
+					},
+				)
+				return c
+			}(),
+			expectedError: `^networking\.clusterNetwork\[1]\.hostPrefix: Invalid value: 30: cluster network host subnetwork prefix must be the same value for IPv4 networks$`,
+		},
 		{
 			name: "networking clusterNetworkMTU - valid high limit ovn",
 			installConfig: func() *types.InstallConfig {