From 27d2359e1b416d5d94a8e19b1048c3b55b2352e9 Mon Sep 17 00:00:00 2001 From: Etienne Audet-Cobello Date: Fri, 27 Sep 2024 17:34:59 -0400 Subject: [PATCH] review comments --- src/k8s/cmd/k8s/k8s_bootstrap.go | 1 - src/k8s/cmd/k8s/k8s_bootstrap_test.go | 36 ------------ src/k8s/pkg/k8sd/app/hooks_bootstrap.go | 54 ++++++++++------- src/k8s/pkg/k8sd/app/hooks_bootstrap_test.go | 58 +++++++++++++++++++ src/k8s/pkg/k8sd/features/calico/network.go | 4 +- .../pkg/k8sd/features/calico/network_test.go | 4 +- src/k8s/pkg/k8sd/features/cilium/network.go | 2 +- .../pkg/k8sd/features/cilium/network_test.go | 2 +- src/k8s/pkg/utils/cidr.go | 4 +- src/k8s/pkg/utils/cidr_test.go | 2 +- 10 files changed, 99 insertions(+), 68 deletions(-) create mode 100644 src/k8s/pkg/k8sd/app/hooks_bootstrap_test.go diff --git a/src/k8s/cmd/k8s/k8s_bootstrap.go b/src/k8s/cmd/k8s/k8s_bootstrap.go index 926c161f6f..5510ed34ea 100644 --- a/src/k8s/cmd/k8s/k8s_bootstrap.go +++ b/src/k8s/cmd/k8s/k8s_bootstrap.go @@ -127,7 +127,6 @@ func newBootstrapCmd(env cmdutil.ExecutionEnvironment) *cobra.Command { } } - cmd.PrintErrln("Bootstrapping the cluster. This may take a few seconds, please wait.") response, err := client.BootstrapCluster(cmd.Context(), apiv1.BootstrapClusterRequest{ diff --git a/src/k8s/cmd/k8s/k8s_bootstrap_test.go b/src/k8s/cmd/k8s/k8s_bootstrap_test.go index b401c8d0c1..b486beb185 100644 --- a/src/k8s/cmd/k8s/k8s_bootstrap_test.go +++ b/src/k8s/cmd/k8s/k8s_bootstrap_test.go @@ -155,39 +155,3 @@ func TestGetConfigFromYaml_Stdin(t *testing.T) { expectedConfig := apiv1.BootstrapConfig{SecurePort: utils.Pointer(5000)} g.Expect(config).To(Equal(expectedConfig)) } - -func TestValidateCIDROverlapAndSize(t *testing.T) { - tests := []struct { - name string - podCIDR string - serviceCIDR string - expectErr bool - }{ - {"Empty", "", "", true}, - {"EmptyServiceCIDR", "192.168.1.0/24", "", true}, - {"EmptyPodCIDR", "", "192.168.1.0/24", true}, - //IPv4 - {"SameIPv4CIDRs", "192.168.100.0/24", "192.168.100.0/24", true}, - {"OverlappingIPv4CIDRs", "10.2.0.13/24", "10.2.0.0/24", true}, - {"IPv4CIDRMinimumSize", "192.168.1.1/32", "10.0.0.0/24", false}, - {"InvalidIPv4CIDRFormat", "192.168.1.1/33", "10.0.0.0/24", true}, - {"MaxSizeIPv4CIDRs", "0.0.0.0/0", "0.0.0.0/0", true}, - //IPv6 - {"SameIPv6CIDRs", "fe80::1/32", "fe80::1/32", true}, - {"OverlappingIPv6CIDRs", "fe80::/48", "fe80::dead/48", true}, - {"IPv6CIDRMinimumSize", "2001:db8::1/128", "fc00::/32", false}, - {"IPv6CIDRPrefixAtLimit", "192.168.1.0/24", "192.168.2.0/24,fe80::/64", true}, - {"IPv6CIDRPrefixGreaterThanLimit", "192.168.1.0/24", "fe80::/68", true}, - {"InvalidIPv6CIDRFormat", "2001:db8::1/129", "fc00::/64", true}, - {"MaxSizeIPv6CIDRs", "::/0", "::/0", true}, - //Mixed - {"IPv4AndIPv6MixedCIDRs", "192.168.0.0/16", "2001:db8::/32", false}, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - if err := validateCIDROverlapAndSize(tc.podCIDR, tc.serviceCIDR); (err != nil) != tc.expectErr { - t.Errorf("validateCIDROverlapAndSize() error = %v, expectErr %v", err, tc.expectErr) - } - }) - } -} diff --git a/src/k8s/pkg/k8sd/app/hooks_bootstrap.go b/src/k8s/pkg/k8sd/app/hooks_bootstrap.go index a87d19f5da..79b9063d23 100644 --- a/src/k8s/pkg/k8sd/app/hooks_bootstrap.go +++ b/src/k8s/pkg/k8sd/app/hooks_bootstrap.go @@ -303,7 +303,13 @@ func (a *App) onBootstrapControlPlane(ctx context.Context, s state.State, bootst } if cfg.Network.GetPodCIDR() != "" && cfg.Network.GetServiceCIDR() != "" { - if err = validateCIDROverlapAndSize(cfg.Network.GetPodCIDR(), cfg.Network.GetServiceCIDR()); err != nil { + if err = validateCIDROverlap(cfg.Network.GetPodCIDR(), cfg.Network.GetServiceCIDR()); err != nil { + return fmt.Errorf("failed to validate the CIDR configuration: %w", err) + } + } + // Can't be an else-if, because default values are already set. + if cfg.Network.GetServiceCIDR() != "" { + if err = validateIPv6CIDRSize(cfg.Network.GetServiceCIDR()); err != nil { return fmt.Errorf("failed to validate the CIDR configuration: %w", err) } } @@ -502,24 +508,23 @@ func (a *App) onBootstrapControlPlane(ctx context.Context, s state.State, bootst } // validateCIDROverlapAndSize checks for overlap and size constraints between pod and service CIDRs. -// It parses the provided podCIDR and serviceCIDR strings, checks for IPv4 and IPv6 overlaps, and ensures -// that the service IPv6 CIDR does not have a prefix length of 64 or more. -func validateCIDROverlapAndSize(podCIDR string, serviceCIDR string) error { +// It parses the provided podCIDR and serviceCIDR strings, checks for IPv4 and IPv6 overlaps. +func validateCIDROverlap(podCIDR string, serviceCIDR string) error { // Parse the CIDRs - podIPv4CIDR, podIPv6CIDR, err := utils.ParseCIDRs(podCIDR) + podIPv4CIDR, podIPv6CIDR, err := utils.SplitCIDRStrings(podCIDR) if err != nil { - return err + return fmt.Errorf("failed to parse pod CIDR: %w", err) } - svcIPv4CIDR, svcIPv6CIDR, err := utils.ParseCIDRs(serviceCIDR) + svcIPv4CIDR, svcIPv6CIDR, err := utils.SplitCIDRStrings(serviceCIDR) if err != nil { - return err + return fmt.Errorf("failed to parse service CIDR: %w", err) } // Check for IPv4 overlap if podIPv4CIDR != "" && svcIPv4CIDR != "" { if overlap, err := utils.CIDRsOverlap(podIPv4CIDR, svcIPv4CIDR); err != nil { - return err + return fmt.Errorf("failed to check for IPv4 overlap: %w", err) } else if overlap { return fmt.Errorf("pod CIDR %q and service CIDR %q overlap", podCIDR, serviceCIDR) } @@ -528,26 +533,31 @@ func validateCIDROverlapAndSize(podCIDR string, serviceCIDR string) error { // Check for IPv6 overlap if podIPv6CIDR != "" && svcIPv6CIDR != "" { if overlap, err := utils.CIDRsOverlap(podIPv6CIDR, svcIPv6CIDR); err != nil { - return err + return fmt.Errorf("failed to check for IPv6 overlap: %w", err) } else if overlap { return fmt.Errorf("pod CIDR %q and service CIDR %q overlap", podCIDR, serviceCIDR) } } - // Check CIDR size - // Ref: https://documentation.ubuntu.com/canonical-kubernetes/latest/snap/howto/networking/dualstack/#cidr-size-limitations - if svcIPv6CIDR != "" { - _, ipv6Net, err := net.ParseCIDR(svcIPv6CIDR) - if err != nil { - // Should not happen, as we already parsed the CIDR - return fmt.Errorf("invalid service CIDR %q: %w", svcIPv6CIDR, err) - } + return nil +} - prefixLength, _ := ipv6Net.Mask.Size() +// Check CIDR size ensures that the service IPv6 CIDR is not larger than /108. +// Ref: https://documentation.ubuntu.com/canonical-kubernetes/latest/snap/howto/networking/dualstack/#cidr-size-limitations +func validateIPv6CIDRSize(serviceCIDR string) error { + _, svcIPv6CIDR, err := utils.SplitCIDRStrings(serviceCIDR) + if err != nil { + return fmt.Errorf("invalid CIDR: %w", err) + } - if prefixLength >= 64 { - return fmt.Errorf("service CIDR %q cannot have a prefix length of 64 or more", svcIPv6CIDR) - } + _, ipv6Net, err := net.ParseCIDR(svcIPv6CIDR) + if err != nil { + return fmt.Errorf("invalid CIDR: %w", err) + } + + prefixLength, _ := ipv6Net.Mask.Size() + if prefixLength < 108 { + return fmt.Errorf("service CIDR %q cannot be larger than /108", serviceCIDR) } return nil diff --git a/src/k8s/pkg/k8sd/app/hooks_bootstrap_test.go b/src/k8s/pkg/k8sd/app/hooks_bootstrap_test.go new file mode 100644 index 0000000000..ef398e9f19 --- /dev/null +++ b/src/k8s/pkg/k8sd/app/hooks_bootstrap_test.go @@ -0,0 +1,58 @@ +package app + +import "testing" + +func TestValidateCIDROverlap(t *testing.T) { + tests := []struct { + name string + podCIDR string + serviceCIDR string + expectErr bool + }{ + {"Empty", "", "", true}, + {"EmptyServiceCIDR", "192.168.1.0/24", "", true}, + {"EmptyPodCIDR", "", "192.168.1.0/24", true}, + //IPv4 + {"SameIPv4CIDRs", "192.168.100.0/24", "192.168.100.0/24", true}, + {"OverlappingIPv4CIDRs", "10.2.0.13/24", "10.2.0.0/24", true}, + {"IPv4CIDRMinimumSize", "192.168.1.1/32", "10.0.0.0/24", false}, + {"InvalidIPv4CIDRFormat", "192.168.1.1/33", "10.0.0.0/24", true}, + {"MaxSizeIPv4CIDRs", "0.0.0.0/0", "0.0.0.0/0", true}, + //IPv6 + {"SameIPv6CIDRs", "fe80::1/32", "fe80::1/32", true}, + {"OverlappingIPv6CIDRs", "fe80::/48", "fe80::dead/48", true}, + {"IPv6CIDRMinimumSize", "2001:db8::1/128", "fc00::/32", false}, + {"InvalidIPv6CIDRFormat", "2001:db8::1/129", "fc00::/64", true}, + {"MaxSizeIPv6CIDRs", "::/0", "::/0", true}, + //Mixed + {"IPv4AndIPv6MixedCIDRs", "192.168.0.0/16", "2001:db8::/32", false}, + {"OnlyInvalidIPv6CIDR", "", "2001:db8::/65", true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if err := validateCIDROverlap(tc.podCIDR, tc.serviceCIDR); (err != nil) != tc.expectErr { + t.Errorf("validateCIDROverlap() error = %v, expectErr %v", err, tc.expectErr) + } + }) + } +} + +func TestValidateCIDRSize(t *testing.T) { + tests := []struct { + name string + cidr string + expectErr bool + }{ + {"Empty", "", true}, + {"DualstackCIDRValid", "192.168.2.0/24,fe80::/128", false}, + {"DualstackCIDRPrefixAtLimit", "192.168.2.0/24,fe80::/108", true}, + {"IPv6CIDRPrefixBiggerThanLimit", "fe80::/64", true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if err := validateIPv6CIDRSize(tc.cidr); (err != nil) != tc.expectErr { + t.Errorf("validateCIDRSize() error = %v, expectErr %v", err, tc.expectErr) + } + }) + } +} diff --git a/src/k8s/pkg/k8sd/features/calico/network.go b/src/k8s/pkg/k8sd/features/calico/network.go index 82e98c7887..436b193193 100644 --- a/src/k8s/pkg/k8sd/features/calico/network.go +++ b/src/k8s/pkg/k8sd/features/calico/network.go @@ -54,7 +54,7 @@ func ApplyNetwork(ctx context.Context, snap snap.Snap, cfg types.Network, annota } podIpPools := []map[string]any{} - ipv4PodCIDR, ipv6PodCIDR, err := utils.ParseCIDRs(cfg.GetPodCIDR()) + ipv4PodCIDR, ipv6PodCIDR, err := utils.SplitCIDRStrings(cfg.GetPodCIDR()) if err != nil { err = fmt.Errorf("invalid pod cidr: %w", err) return types.FeatureStatus{ @@ -79,7 +79,7 @@ func ApplyNetwork(ctx context.Context, snap snap.Snap, cfg types.Network, annota } serviceCIDRs := []string{} - ipv4ServiceCIDR, ipv6ServiceCIDR, err := utils.ParseCIDRs(cfg.GetServiceCIDR()) + ipv4ServiceCIDR, ipv6ServiceCIDR, err := utils.SplitCIDRStrings(cfg.GetServiceCIDR()) if err != nil { err = fmt.Errorf("invalid service cidr: %v", err) return types.FeatureStatus{ diff --git a/src/k8s/pkg/k8sd/features/calico/network_test.go b/src/k8s/pkg/k8sd/features/calico/network_test.go index c0a3240282..0024096bca 100644 --- a/src/k8s/pkg/k8sd/features/calico/network_test.go +++ b/src/k8s/pkg/k8sd/features/calico/network_test.go @@ -192,10 +192,10 @@ func TestEnabled(t *testing.T) { func validateValues(t *testing.T, values map[string]any, cfg types.Network) { g := NewWithT(t) - podIPv4CIDR, podIPv6CIDR, err := utils.ParseCIDRs(cfg.GetPodCIDR()) + podIPv4CIDR, podIPv6CIDR, err := utils.SplitCIDRStrings(cfg.GetPodCIDR()) g.Expect(err).ToNot(HaveOccurred()) - svcIPv4CIDR, svcIPv6CIDR, err := utils.ParseCIDRs(cfg.GetServiceCIDR()) + svcIPv4CIDR, svcIPv6CIDR, err := utils.SplitCIDRStrings(cfg.GetServiceCIDR()) g.Expect(err).ToNot(HaveOccurred()) // calico network diff --git a/src/k8s/pkg/k8sd/features/cilium/network.go b/src/k8s/pkg/k8sd/features/cilium/network.go index 4418e8b1d1..c3fd573ec4 100644 --- a/src/k8s/pkg/k8sd/features/cilium/network.go +++ b/src/k8s/pkg/k8sd/features/cilium/network.go @@ -44,7 +44,7 @@ func ApplyNetwork(ctx context.Context, snap snap.Snap, cfg types.Network, _ type }, nil } - ipv4CIDR, ipv6CIDR, err := utils.ParseCIDRs(cfg.GetPodCIDR()) + ipv4CIDR, ipv6CIDR, err := utils.SplitCIDRStrings(cfg.GetPodCIDR()) if err != nil { err = fmt.Errorf("invalid kube-proxy --cluster-cidr value: %v", err) return types.FeatureStatus{ diff --git a/src/k8s/pkg/k8sd/features/cilium/network_test.go b/src/k8s/pkg/k8sd/features/cilium/network_test.go index 175cd95a0d..3ceb408a83 100644 --- a/src/k8s/pkg/k8sd/features/cilium/network_test.go +++ b/src/k8s/pkg/k8sd/features/cilium/network_test.go @@ -163,7 +163,7 @@ func validateNetworkValues(t *testing.T, values map[string]any, cfg types.Networ t.Helper() g := NewWithT(t) - ipv4CIDR, ipv6CIDR, err := utils.ParseCIDRs(cfg.GetPodCIDR()) + ipv4CIDR, ipv6CIDR, err := utils.SplitCIDRStrings(cfg.GetPodCIDR()) g.Expect(err).ToNot(HaveOccurred()) bpfMount, err := utils.GetMountPath("bpf") diff --git a/src/k8s/pkg/utils/cidr.go b/src/k8s/pkg/utils/cidr.go index 4d969d7cf9..ffe7bdfa31 100644 --- a/src/k8s/pkg/utils/cidr.go +++ b/src/k8s/pkg/utils/cidr.go @@ -101,8 +101,8 @@ func ParseAddressString(address string, port int64) (string, error) { return util.CanonicalNetworkAddress(address, port), nil } -// ParseCIDRs parses the given CIDR string and returns the respective IPv4 and IPv6 CIDRs. -func ParseCIDRs(CIDRstring string) (string, string, error) { +// SplitCIDRStrings parses the given CIDR string and returns the respective IPv4 and IPv6 CIDRs. +func SplitCIDRStrings(CIDRstring string) (string, string, error) { clusterCIDRs := strings.Split(CIDRstring, ",") if v := len(clusterCIDRs); v != 1 && v != 2 { return "", "", fmt.Errorf("invalid CIDR list: %v", clusterCIDRs) diff --git a/src/k8s/pkg/utils/cidr_test.go b/src/k8s/pkg/utils/cidr_test.go index 1d0e9da96d..5bb1aef237 100644 --- a/src/k8s/pkg/utils/cidr_test.go +++ b/src/k8s/pkg/utils/cidr_test.go @@ -153,7 +153,7 @@ func TestParseCIDRs(t *testing.T) { for _, tc := range testCases { t.Run(tc.input, func(t *testing.T) { - ipv4CIDR, ipv6CIDR, err := utils.ParseCIDRs(tc.input) + ipv4CIDR, ipv6CIDR, err := utils.SplitCIDRStrings(tc.input) if tc.expectedErr { Expect(err).To(HaveOccurred()) } else {