Allow multiple MachineNetworks in dual stack

Multiple MachineNetworks in the same address family and IPv6-primary dual-stack clusters are a thing, so relax the dual-stack validation requirements for machine networks to allow them.
openshift · Sep 3, 2024 · c559bcb · c559bcb
1 parent 5a072e0
commit c559bcb
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 49 deletions.
diff --git a/internal/bminventory/inventory_test.go b/internal/bminventory/inventory_test.go
@@ -4339,33 +4339,6 @@ var _ = Describe("cluster", func() {
 					actual := reply.(*installer.V2UpdateClusterCreated)
 					Expect(len(actual.Payload.MachineNetworks)).To(Equal(2))
 				})
-				It("Wrong order of machine network CIDRs in non dhcp for dual-stack", func() {
-					mockClusterUpdatability(1)
-					mockSuccess(1)
-
-					apiVip := "10.11.12.15"
-					ingressVip := "10.11.12.16"
-					reply := bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
-						ClusterID: clusterID,
-						ClusterUpdateParams: &models.V2ClusterUpdateParams{
-							APIVips:         []*models.APIVip{{IP: models.IP(apiVip)}},
-							IngressVips:     []*models.IngressVip{{IP: models.IP(ingressVip)}},
-							ClusterNetworks: []*models.ClusterNetwork{{Cidr: "10.128.0.0/14", HostPrefix: 23}, {Cidr: "fd01::/48", HostPrefix: 64}},
-							MachineNetworks: []*models.MachineNetwork{{Cidr: "10.11.0.0/16"}, {Cidr: "fd2e:6f44:5dd8:c956::/120"}},
-						},
-					})
-					Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))
-
-					reply = bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
-						ClusterID: clusterID,
-						ClusterUpdateParams: &models.V2ClusterUpdateParams{
-							APIVips:         []*models.APIVip{{IP: models.IP(apiVip)}},
-							IngressVips:     []*models.IngressVip{{IP: models.IP(ingressVip)}},
-							MachineNetworks: []*models.MachineNetwork{{Cidr: "fd2e:6f44:5dd8:c956::/120"}, {Cidr: "10.12.0.0/16"}},
-						},
-					})
-					verifyApiErrorString(reply, http.StatusBadRequest, "First machine network has to be IPv4 subnet")
-				})
 				It("API VIP in wrong subnet for dual-stack", func() {
 					apiVip := "10.11.12.15"
 					ingressVip := "10.11.12.16"
@@ -17290,9 +17263,36 @@ var _ = Describe("Dual-stack cluster", func() {
 				reply := bm.V2RegisterCluster(ctx, params)
 				verifyApiErrorString(reply, http.StatusBadRequest, errStr)
 			})
-			It("v6-first in machine networks rejected", func() {
-				errStr := "First machine network has to be IPv4 subnet"
-				params.NewClusterParams.MachineNetworks = TestDualStackNetworkingWrongOrder.MachineNetworks
+		})
+
+		Context("Cluster with two networks of same stack", func() {
+			It("only v4 in cluster networks rejected", func() {
+				errStr := "Second cluster network has to be IPv6 subnet"
+				params.NewClusterParams.ClusterNetworks = []*models.ClusterNetwork{
+					{Cidr: "1.3.0.0/16", HostPrefix: 16},
+					{Cidr: "1.4.0.0/16", HostPrefix: 16},
+				}
+				params.NewClusterParams.ServiceNetworks = common.TestDualStackNetworking.ServiceNetworks
+				reply := bm.V2RegisterCluster(ctx, params)
+				verifyApiErrorString(reply, http.StatusBadRequest, errStr)
+			})
+			It("only v4 in service networks rejected", func() {
+				errStr := "Second service network has to be IPv6 subnet"
+				params.NewClusterParams.ClusterNetworks = common.TestDualStackNetworking.ClusterNetworks
+				params.NewClusterParams.ServiceNetworks = []*models.ServiceNetwork{
+					{Cidr: "1.2.5.0/24"},
+					{Cidr: "1.2.6.0/24"},
+				}
+				reply := bm.V2RegisterCluster(ctx, params)
+				verifyApiErrorString(reply, http.StatusBadRequest, errStr)
+			})
+			It("only v4 in machine networks rejected", func() {
+				errStr := "One machine network has to be IPv6 subnet"
+				params.NewClusterParams.ClusterNetworks = common.TestDualStackNetworking.ClusterNetworks
+				params.NewClusterParams.MachineNetworks = []*models.MachineNetwork{
+					{Cidr: "1.2.3.0/24"},
+					{Cidr: "1.2.4.0/24"},
+				}
 				reply := bm.V2RegisterCluster(ctx, params)
 				verifyApiErrorString(reply, http.StatusBadRequest, errStr)
 			})
@@ -17356,11 +17356,6 @@ var _ = Describe("Dual-stack cluster", func() {
 				reply := bm.V2UpdateCluster(ctx, params)
 				verifyApiErrorString(reply, http.StatusBadRequest, "First service network has to be IPv4 subnet")
 			})
-			It("v6-first in machine networks rejected", func() {
-				params.ClusterUpdateParams.MachineNetworks = TestDualStackNetworkingWrongOrder.MachineNetworks
-				reply := bm.V2UpdateCluster(ctx, params)
-				verifyApiErrorString(reply, http.StatusBadRequest, "First machine network has to be IPv4 subnet")
-			})
 		})
 
 		Context("Cluster with single network when two required", func() {

diff --git a/internal/cluster/validations/validation_test.go b/internal/cluster/validations/validation_test.go
@@ -657,19 +657,16 @@ var _ = Describe("Machine Network amount and order", func() {
 			valid:   true,
 		},
 		{
-			// Invalid because violates the "IPv4 subnet as the first one" constraint
 			element: []*models.MachineNetwork{{Cidr: "1002:db8::/119"}, {Cidr: "1.2.5.0/24"}},
-			valid:   false,
+			valid:   true,
 		},
 		{
-			// Invalid because violates the "exactly 2 networks" constraint
 			element: []*models.MachineNetwork{{Cidr: "1.2.5.0/24"}, {Cidr: "1002:db8::/119"}, {Cidr: "1.2.6.0/24"}, {Cidr: "1.2.7.0/24"}},
-			valid:   false,
+			valid:   true,
 		},
 		{
-			// Invalid because violates the "exactly 2 networks" constraint
 			element: []*models.MachineNetwork{{Cidr: "1002:db8::/119"}, {Cidr: "1.2.5.0/24"}, {Cidr: "1.2.6.0/24"}, {Cidr: "1.2.7.0/24"}},
-			valid:   false,
+			valid:   true,
 		},
 	}
 	for _, test := range tests {

diff --git a/internal/network/dual_stack_validations.go b/internal/network/dual_stack_validations.go
@@ -7,21 +7,29 @@ import (
 )
 
 // Verify if the constrains for dual-stack machine networks are met:
-//   - there are exactly two machine networks
-//   - the first one is IPv4 subnet
-//   - the second one is IPv6 subnet
+//   - there are at least two machine networks
+//   - at least one is IPv4 subnet
+//   - at least one is IPv6 subnet
 func VerifyMachineNetworksDualStack(networks []*models.MachineNetwork, isDualStack bool) error {
 	if !isDualStack {
 		return nil
 	}
-	if len(networks) != 2 {
+	if len(networks) < 2 {
 		return errors.Errorf("Expected 2 machine networks, found %d", len(networks))
 	}
-	if !IsIPV4CIDR(string(networks[0].Cidr)) {
-		return errors.Errorf("First machine network has to be IPv4 subnet")
+	var haveIPv4, haveIPv6 bool
+	for _, net := range networks {
+		if IsIPV4CIDR(string(net.Cidr)) {
+			haveIPv4 = true
+		} else if IsIPv6CIDR(string(net.Cidr)) {
+			haveIPv6 = true
+		}
 	}
-	if !IsIPv6CIDR(string(networks[1].Cidr)) {
-		return errors.Errorf("Second machine network has to be IPv6 subnet")
+	if !haveIPv4 {
+		return errors.Errorf("One machine network has to be IPv4 subnet")
+	}
+	if !haveIPv6 {
+		return errors.Errorf("One machine network has to be IPv6 subnet")
 	}
 
 	return nil