From c559bcba4c5e4e04f2ad9afafe53598d9750f955 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Wed, 28 Feb 2024 17:59:04 +1300 Subject: [PATCH] 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. --- internal/bminventory/inventory_test.go | 65 +++++++++---------- .../cluster/validations/validation_test.go | 9 +-- internal/network/dual_stack_validations.go | 24 ++++--- 3 files changed, 49 insertions(+), 49 deletions(-) diff --git a/internal/bminventory/inventory_test.go b/internal/bminventory/inventory_test.go index faa94c90260..d66ad8f9ff4 100644 --- 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 index 57246802545..f67211d0c9c 100644 --- 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 index 114476dbe07..ba1bd8bb1be 100644 --- 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