From a18f27b927b6ea6490e277014c51d0a95803358f Mon Sep 17 00:00:00 2001 From: Etienne Audet-Cobello Date: Tue, 24 Sep 2024 16:05:23 -0400 Subject: [PATCH 1/9] Implement CIDR validation --- src/k8s/cmd/k8s/k8s_bootstrap.go | 44 ++++++++++++++++++++++++++++++++ src/k8s/pkg/utils/cidr.go | 15 +++++++++++ src/k8s/pkg/utils/cidr_test.go | 20 +++++++++++++++ 3 files changed, 79 insertions(+) diff --git a/src/k8s/cmd/k8s/k8s_bootstrap.go b/src/k8s/cmd/k8s/k8s_bootstrap.go index 5510ed34e..3c93f0195 100644 --- a/src/k8s/cmd/k8s/k8s_bootstrap.go +++ b/src/k8s/cmd/k8s/k8s_bootstrap.go @@ -127,6 +127,50 @@ func newBootstrapCmd(env cmdutil.ExecutionEnvironment) *cobra.Command { } } + var podIPv4CIDR, podIPv6CIDR string + if bootstrapConfig.PodCIDR != nil { + podIPv4CIDR, podIPv6CIDR, err = utils.ParseCIDRs(*bootstrapConfig.PodCIDR) + if err != nil { + cmd.PrintErrf("Error: Failed to parse the Pod CIDR %q.\n\nThe error was: %v\n", *bootstrapConfig.PodCIDR, err) + env.Exit(1) + return + } + } + + var svcIPv4CIDR, svcIPv6CIDR string + if bootstrapConfig.ServiceCIDR != nil { + svcIPv4CIDR, svcIPv6CIDR, err = utils.ParseCIDRs(*bootstrapConfig.ServiceCIDR) + if err != nil { + cmd.PrintErrf("Error: Failed to parse the Pod CIDR %q.\n\nThe error was: %v\n", *bootstrapConfig.PodCIDR, err) + env.Exit(1) + return + } + } + + if podIPv4CIDR != "" && svcIPv4CIDR != "" { + if overlap, err := utils.CIDRsOverlap(podIPv4CIDR, svcIPv4CIDR); err != nil { + cmd.PrintErrf("Error: Failed to check if the Pod CIDR %q and Service CIDR %q overlap.\n\nThe error was: %v\n", podIPv4CIDR, svcIPv4CIDR, err) + env.Exit(1) + return + } else if overlap { + cmd.PrintErrf("Error: The Pod CIDR %q and Service CIDR %q overlap.\n", podIPv4CIDR, svcIPv4CIDR) + env.Exit(1) + return + } + } + + if podIPv6CIDR != "" && svcIPv6CIDR != "" { + if overlap, err := utils.CIDRsOverlap(podIPv6CIDR, svcIPv6CIDR); err != nil { + cmd.PrintErrf("Error: Failed to check if the Pod CIDR %q and Service CIDR %q overlap.\n\nThe error was: %v\n", podIPv6CIDR, svcIPv6CIDR, err) + env.Exit(1) + return + } else if overlap { + cmd.PrintErrf("Error: The Pod CIDR %q and Service CIDR %q overlap.\n", podIPv6CIDR, svcIPv6CIDR) + env.Exit(1) + return + } + } + 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/pkg/utils/cidr.go b/src/k8s/pkg/utils/cidr.go index e5fe01bba..6bb852e3f 100644 --- a/src/k8s/pkg/utils/cidr.go +++ b/src/k8s/pkg/utils/cidr.go @@ -142,3 +142,18 @@ func ToIPString(ip net.IP) string { } return "[" + ip.String() + "]" } + +func CIDRsOverlap(cidr1, cidr2 string) (bool, error) { + _, ipNet1, err1 := net.ParseCIDR(cidr1) + _, ipNet2, err2 := net.ParseCIDR(cidr2) + + if err1 != nil || err2 != nil { + return false, fmt.Errorf("invalid CIDR blocks") + } + + if ipNet1.Contains(ipNet2.IP) || ipNet2.Contains(ipNet1.IP) { + return true, nil + } + + return false, nil +} diff --git a/src/k8s/pkg/utils/cidr_test.go b/src/k8s/pkg/utils/cidr_test.go index 1d0e9da96..ebaef7372 100644 --- a/src/k8s/pkg/utils/cidr_test.go +++ b/src/k8s/pkg/utils/cidr_test.go @@ -205,3 +205,23 @@ func TestToIPString(t *testing.T) { }) } } + +func TestCIDRsOverlap(t *testing.T) { + g := NewWithT(t) + + tests := []struct { + cidr1 string + cidr2 string + expected bool + }{ + {"192.168.100.0/24", "192.168.100.0/24", true}, + {"192.168.1.0/32", "192.168.5.0/32", false}, + {"fe80::1/64", "fe80::1/64", true}, + {"fe80::/64", "2001:db8::/32", false}, + {"fe80::/64", "fe80::dead/64", true}, + } + + for _, tc := range tests { + g.Expect(utils.CIDRsOverlap(tc.cidr1, tc.cidr2)).To(Equal(tc.expected)) + } +} From 1582d1f71cb97f7da20871c21133e8e4d0d4d9a0 Mon Sep 17 00:00:00 2001 From: Etienne Audet-Cobello Date: Tue, 24 Sep 2024 17:02:07 -0400 Subject: [PATCH 2/9] move validation logic to a function --- src/k8s/cmd/k8s/k8s_bootstrap.go | 103 ++++++++++++++++++------------- 1 file changed, 61 insertions(+), 42 deletions(-) diff --git a/src/k8s/cmd/k8s/k8s_bootstrap.go b/src/k8s/cmd/k8s/k8s_bootstrap.go index 3c93f0195..1cfbda487 100644 --- a/src/k8s/cmd/k8s/k8s_bootstrap.go +++ b/src/k8s/cmd/k8s/k8s_bootstrap.go @@ -5,6 +5,7 @@ import ( "bytes" "fmt" "io" + "net" "os" "slices" "strings" @@ -127,48 +128,10 @@ func newBootstrapCmd(env cmdutil.ExecutionEnvironment) *cobra.Command { } } - var podIPv4CIDR, podIPv6CIDR string - if bootstrapConfig.PodCIDR != nil { - podIPv4CIDR, podIPv6CIDR, err = utils.ParseCIDRs(*bootstrapConfig.PodCIDR) - if err != nil { - cmd.PrintErrf("Error: Failed to parse the Pod CIDR %q.\n\nThe error was: %v\n", *bootstrapConfig.PodCIDR, err) - env.Exit(1) - return - } - } - - var svcIPv4CIDR, svcIPv6CIDR string - if bootstrapConfig.ServiceCIDR != nil { - svcIPv4CIDR, svcIPv6CIDR, err = utils.ParseCIDRs(*bootstrapConfig.ServiceCIDR) - if err != nil { - cmd.PrintErrf("Error: Failed to parse the Pod CIDR %q.\n\nThe error was: %v\n", *bootstrapConfig.PodCIDR, err) - env.Exit(1) - return - } - } - - if podIPv4CIDR != "" && svcIPv4CIDR != "" { - if overlap, err := utils.CIDRsOverlap(podIPv4CIDR, svcIPv4CIDR); err != nil { - cmd.PrintErrf("Error: Failed to check if the Pod CIDR %q and Service CIDR %q overlap.\n\nThe error was: %v\n", podIPv4CIDR, svcIPv4CIDR, err) - env.Exit(1) - return - } else if overlap { - cmd.PrintErrf("Error: The Pod CIDR %q and Service CIDR %q overlap.\n", podIPv4CIDR, svcIPv4CIDR) - env.Exit(1) - return - } - } - - if podIPv6CIDR != "" && svcIPv6CIDR != "" { - if overlap, err := utils.CIDRsOverlap(podIPv6CIDR, svcIPv6CIDR); err != nil { - cmd.PrintErrf("Error: Failed to check if the Pod CIDR %q and Service CIDR %q overlap.\n\nThe error was: %v\n", podIPv6CIDR, svcIPv6CIDR, err) - env.Exit(1) - return - } else if overlap { - cmd.PrintErrf("Error: The Pod CIDR %q and Service CIDR %q overlap.\n", podIPv6CIDR, svcIPv6CIDR) - env.Exit(1) - return - } + if err = validateCIDROverlapAndSize(*bootstrapConfig.PodCIDR, *bootstrapConfig.ServiceCIDR); err != nil { + cmd.PrintErrf("Error: Failed to validate the CIDR configuration.\n\nThe error was: %v\n", err) + env.Exit(1) + return } cmd.PrintErrln("Bootstrapping the cluster. This may take a few seconds, please wait.") @@ -313,3 +276,59 @@ func askQuestion(stdin io.Reader, stdout io.Writer, stderr io.Writer, question s return s } } + +func validateCIDROverlapAndSize(podCIDR string, serviceCIDR string) error { + // Parse the CIDRs + var err error + + var podIPv4CIDR, podIPv6CIDR string + if podCIDR != "" { + podIPv4CIDR, podIPv6CIDR, err = utils.ParseCIDRs(podCIDR) + if err != nil { + return err + } + } + + var svcIPv4CIDR, svcIPv6CIDR string + if serviceCIDR != "" { + svcIPv4CIDR, svcIPv6CIDR, err = utils.ParseCIDRs(serviceCIDR) + if err != nil { + return err + } + } + + // Check for overlap + if podIPv4CIDR != "" && svcIPv4CIDR != "" { + if overlap, err := utils.CIDRsOverlap(podIPv4CIDR, svcIPv4CIDR); err != nil { + return err + } else if overlap { + return fmt.Errorf("pod CIDR %q and service CIDR %q overlap", podCIDR, serviceCIDR) + } + } + + if podIPv6CIDR != "" && svcIPv6CIDR != "" { + if overlap, err := utils.CIDRsOverlap(podIPv6CIDR, svcIPv6CIDR); err != nil { + return 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) + } + + prefixLength, _ := ipv6Net.Mask.Size() + + if prefixLength >= 64 { + return fmt.Errorf("service CIDR %q cannot have a prefix length of 64 or more", svcIPv6CIDR) + } + } + + return nil +} From 39b5f52d9aab6d07852af8c46acc0ce062369510 Mon Sep 17 00:00:00 2001 From: Etienne Audet-Cobello Date: Wed, 25 Sep 2024 14:25:42 -0400 Subject: [PATCH 3/9] refactor --- src/k8s/cmd/k8s/k8s_bootstrap.go | 36 +++++++++++++-------------- src/k8s/cmd/k8s/k8s_bootstrap_test.go | 36 +++++++++++++++++++++++++++ src/k8s/pkg/utils/cidr.go | 5 +++- src/k8s/pkg/utils/cidr_test.go | 20 --------------- 4 files changed, 57 insertions(+), 40 deletions(-) diff --git a/src/k8s/cmd/k8s/k8s_bootstrap.go b/src/k8s/cmd/k8s/k8s_bootstrap.go index 1cfbda487..55bfe6a18 100644 --- a/src/k8s/cmd/k8s/k8s_bootstrap.go +++ b/src/k8s/cmd/k8s/k8s_bootstrap.go @@ -128,10 +128,12 @@ func newBootstrapCmd(env cmdutil.ExecutionEnvironment) *cobra.Command { } } - if err = validateCIDROverlapAndSize(*bootstrapConfig.PodCIDR, *bootstrapConfig.ServiceCIDR); err != nil { - cmd.PrintErrf("Error: Failed to validate the CIDR configuration.\n\nThe error was: %v\n", err) - env.Exit(1) - return + if bootstrapConfig.PodCIDR != nil && bootstrapConfig.ServiceCIDR != nil { + if err = validateCIDROverlapAndSize(*bootstrapConfig.PodCIDR, *bootstrapConfig.ServiceCIDR); err != nil { + cmd.PrintErrf("Error: Failed to validate the CIDR configuration.\n\nThe error was: %v\n", err) + env.Exit(1) + return + } } cmd.PrintErrln("Bootstrapping the cluster. This may take a few seconds, please wait.") @@ -277,27 +279,22 @@ func askQuestion(stdin io.Reader, stdout io.Writer, stderr io.Writer, question s } } +// 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 { // Parse the CIDRs - var err error - - var podIPv4CIDR, podIPv6CIDR string - if podCIDR != "" { - podIPv4CIDR, podIPv6CIDR, err = utils.ParseCIDRs(podCIDR) - if err != nil { - return err - } + podIPv4CIDR, podIPv6CIDR, err := utils.ParseCIDRs(podCIDR) + if err != nil { + return err } - var svcIPv4CIDR, svcIPv6CIDR string - if serviceCIDR != "" { - svcIPv4CIDR, svcIPv6CIDR, err = utils.ParseCIDRs(serviceCIDR) - if err != nil { - return err - } + svcIPv4CIDR, svcIPv6CIDR, err := utils.ParseCIDRs(serviceCIDR) + if err != nil { + return err } - // Check for overlap + // Check for IPv4 overlap if podIPv4CIDR != "" && svcIPv4CIDR != "" { if overlap, err := utils.CIDRsOverlap(podIPv4CIDR, svcIPv4CIDR); err != nil { return err @@ -306,6 +303,7 @@ 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 diff --git a/src/k8s/cmd/k8s/k8s_bootstrap_test.go b/src/k8s/cmd/k8s/k8s_bootstrap_test.go index b486beb18..b401c8d0c 100644 --- a/src/k8s/cmd/k8s/k8s_bootstrap_test.go +++ b/src/k8s/cmd/k8s/k8s_bootstrap_test.go @@ -155,3 +155,39 @@ 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/utils/cidr.go b/src/k8s/pkg/utils/cidr.go index 6bb852e3f..4d969d7cf 100644 --- a/src/k8s/pkg/utils/cidr.go +++ b/src/k8s/pkg/utils/cidr.go @@ -143,12 +143,15 @@ func ToIPString(ip net.IP) string { return "[" + ip.String() + "]" } +// CIDRsOverlap checks if two given CIDR blocks overlap. +// It takes two strings representing the CIDR blocks as input and returns a boolean indicating +// whether they overlap and an error if any of the CIDR blocks are invalid. func CIDRsOverlap(cidr1, cidr2 string) (bool, error) { _, ipNet1, err1 := net.ParseCIDR(cidr1) _, ipNet2, err2 := net.ParseCIDR(cidr2) if err1 != nil || err2 != nil { - return false, fmt.Errorf("invalid CIDR blocks") + return false, fmt.Errorf("invalid CIDR blocks, %v, %v", err1, err2) } if ipNet1.Contains(ipNet2.IP) || ipNet2.Contains(ipNet1.IP) { diff --git a/src/k8s/pkg/utils/cidr_test.go b/src/k8s/pkg/utils/cidr_test.go index ebaef7372..1d0e9da96 100644 --- a/src/k8s/pkg/utils/cidr_test.go +++ b/src/k8s/pkg/utils/cidr_test.go @@ -205,23 +205,3 @@ func TestToIPString(t *testing.T) { }) } } - -func TestCIDRsOverlap(t *testing.T) { - g := NewWithT(t) - - tests := []struct { - cidr1 string - cidr2 string - expected bool - }{ - {"192.168.100.0/24", "192.168.100.0/24", true}, - {"192.168.1.0/32", "192.168.5.0/32", false}, - {"fe80::1/64", "fe80::1/64", true}, - {"fe80::/64", "2001:db8::/32", false}, - {"fe80::/64", "fe80::dead/64", true}, - } - - for _, tc := range tests { - g.Expect(utils.CIDRsOverlap(tc.cidr1, tc.cidr2)).To(Equal(tc.expected)) - } -} From 430d46dcea3c92b054f8befacda27e0dd32869cc Mon Sep 17 00:00:00 2001 From: Etienne Audet-Cobello Date: Fri, 27 Sep 2024 14:28:36 -0400 Subject: [PATCH 4/9] move impl. to k8sd api --- src/k8s/cmd/k8s/k8s_bootstrap.go | 60 ------------------------- src/k8s/pkg/k8sd/app/hooks_bootstrap.go | 58 ++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 60 deletions(-) diff --git a/src/k8s/cmd/k8s/k8s_bootstrap.go b/src/k8s/cmd/k8s/k8s_bootstrap.go index 55bfe6a18..926c161f6 100644 --- a/src/k8s/cmd/k8s/k8s_bootstrap.go +++ b/src/k8s/cmd/k8s/k8s_bootstrap.go @@ -5,7 +5,6 @@ import ( "bytes" "fmt" "io" - "net" "os" "slices" "strings" @@ -128,13 +127,6 @@ func newBootstrapCmd(env cmdutil.ExecutionEnvironment) *cobra.Command { } } - if bootstrapConfig.PodCIDR != nil && bootstrapConfig.ServiceCIDR != nil { - if err = validateCIDROverlapAndSize(*bootstrapConfig.PodCIDR, *bootstrapConfig.ServiceCIDR); err != nil { - cmd.PrintErrf("Error: Failed to validate the CIDR configuration.\n\nThe error was: %v\n", err) - env.Exit(1) - return - } - } cmd.PrintErrln("Bootstrapping the cluster. This may take a few seconds, please wait.") @@ -278,55 +270,3 @@ func askQuestion(stdin io.Reader, stdout io.Writer, stderr io.Writer, question s return s } } - -// 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 { - // Parse the CIDRs - podIPv4CIDR, podIPv6CIDR, err := utils.ParseCIDRs(podCIDR) - if err != nil { - return err - } - - svcIPv4CIDR, svcIPv6CIDR, err := utils.ParseCIDRs(serviceCIDR) - if err != nil { - return err - } - - // Check for IPv4 overlap - if podIPv4CIDR != "" && svcIPv4CIDR != "" { - if overlap, err := utils.CIDRsOverlap(podIPv4CIDR, svcIPv4CIDR); err != nil { - return err - } else if overlap { - return fmt.Errorf("pod CIDR %q and service CIDR %q overlap", podCIDR, serviceCIDR) - } - } - - // Check for IPv6 overlap - if podIPv6CIDR != "" && svcIPv6CIDR != "" { - if overlap, err := utils.CIDRsOverlap(podIPv6CIDR, svcIPv6CIDR); err != nil { - return 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) - } - - prefixLength, _ := ipv6Net.Mask.Size() - - if prefixLength >= 64 { - return fmt.Errorf("service CIDR %q cannot have a prefix length of 64 or more", svcIPv6CIDR) - } - } - - return nil -} diff --git a/src/k8s/pkg/k8sd/app/hooks_bootstrap.go b/src/k8s/pkg/k8sd/app/hooks_bootstrap.go index 2e35b9be6..738086670 100644 --- a/src/k8s/pkg/k8sd/app/hooks_bootstrap.go +++ b/src/k8s/pkg/k8sd/app/hooks_bootstrap.go @@ -320,6 +320,12 @@ func (a *App) onBootstrapControlPlane(ctx context.Context, s state.State, bootst return fmt.Errorf("failed to get IP address(es) from ServiceCIDR %q: %w", cfg.Network.GetServiceCIDR(), err) } + if cfg.Network.GetPodCIDR() != "" && cfg.Network.GetServiceCIDR() != "" { + if err = validateCIDROverlapAndSize(cfg.Network.GetPodCIDR(), cfg.Network.GetServiceCIDR()); err != nil { + return fmt.Errorf("failed to validate the CIDR configuration: %w", err) + } + } + // NOTE: Set the notBefore certificate time to the current time. notBefore := time.Now() @@ -512,3 +518,55 @@ func (a *App) onBootstrapControlPlane(ctx context.Context, s state.State, bootst a.NotifyUpdateNodeConfigController() return nil } + +// 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 { + // Parse the CIDRs + podIPv4CIDR, podIPv6CIDR, err := utils.ParseCIDRs(podCIDR) + if err != nil { + return err + } + + svcIPv4CIDR, svcIPv6CIDR, err := utils.ParseCIDRs(serviceCIDR) + if err != nil { + return err + } + + // Check for IPv4 overlap + if podIPv4CIDR != "" && svcIPv4CIDR != "" { + if overlap, err := utils.CIDRsOverlap(podIPv4CIDR, svcIPv4CIDR); err != nil { + return err + } else if overlap { + return fmt.Errorf("pod CIDR %q and service CIDR %q overlap", podCIDR, serviceCIDR) + } + } + + // Check for IPv6 overlap + if podIPv6CIDR != "" && svcIPv6CIDR != "" { + if overlap, err := utils.CIDRsOverlap(podIPv6CIDR, svcIPv6CIDR); err != nil { + return 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) + } + + prefixLength, _ := ipv6Net.Mask.Size() + + if prefixLength >= 64 { + return fmt.Errorf("service CIDR %q cannot have a prefix length of 64 or more", svcIPv6CIDR) + } + } + + return nil +} From d571ab5eef225e709ddcd415a3c2cacd99ab863e Mon Sep 17 00:00:00 2001 From: Etienne Audet-Cobello Date: Fri, 27 Sep 2024 17:34:59 -0400 Subject: [PATCH 5/9] 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 926c161f6..5510ed34e 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 b401c8d0c..b486beb18 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 738086670..07b47a7d9 100644 --- a/src/k8s/pkg/k8sd/app/hooks_bootstrap.go +++ b/src/k8s/pkg/k8sd/app/hooks_bootstrap.go @@ -321,7 +321,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) } } @@ -520,24 +526,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) } @@ -546,26 +551,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 000000000..ef398e9f1 --- /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 f89651df2..414ea2938 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, apiserver types.APIServer } podIpPools := []map[string]any{} - ipv4PodCIDR, ipv6PodCIDR, err := utils.ParseCIDRs(network.GetPodCIDR()) + ipv4PodCIDR, ipv6PodCIDR, err := utils.SplitCIDRStrings(network.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, apiserver types.APIServer } serviceCIDRs := []string{} - ipv4ServiceCIDR, ipv6ServiceCIDR, err := utils.ParseCIDRs(network.GetServiceCIDR()) + ipv4ServiceCIDR, ipv6ServiceCIDR, err := utils.SplitCIDRStrings(network.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 75d31e0af..96d602b37 100644 --- a/src/k8s/pkg/k8sd/features/calico/network_test.go +++ b/src/k8s/pkg/k8sd/features/calico/network_test.go @@ -210,10 +210,10 @@ func TestEnabled(t *testing.T) { func validateValues(t *testing.T, values map[string]any, network types.Network) { g := NewWithT(t) - podIPv4CIDR, podIPv6CIDR, err := utils.ParseCIDRs(network.GetPodCIDR()) + podIPv4CIDR, podIPv6CIDR, err := utils.SplitCIDRStrings(network.GetPodCIDR()) g.Expect(err).ToNot(HaveOccurred()) - svcIPv4CIDR, svcIPv6CIDR, err := utils.ParseCIDRs(network.GetServiceCIDR()) + svcIPv4CIDR, svcIPv6CIDR, err := utils.SplitCIDRStrings(network.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 e0573d72b..ba9720dad 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, apiserver types.APIServer }, nil } - ipv4CIDR, ipv6CIDR, err := utils.ParseCIDRs(network.GetPodCIDR()) + ipv4CIDR, ipv6CIDR, err := utils.SplitCIDRStrings(network.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 978bebc0e..7b9c2e36b 100644 --- a/src/k8s/pkg/k8sd/features/cilium/network_test.go +++ b/src/k8s/pkg/k8sd/features/cilium/network_test.go @@ -180,7 +180,7 @@ func validateNetworkValues(t *testing.T, values map[string]any, network types.Ne t.Helper() g := NewWithT(t) - ipv4CIDR, ipv6CIDR, err := utils.ParseCIDRs(network.GetPodCIDR()) + ipv4CIDR, ipv6CIDR, err := utils.SplitCIDRStrings(network.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 4d969d7cf..ffe7bdfa3 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 1d0e9da96..5bb1aef23 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 { From acaf6378611dfa8f05b4a5c5f99e6bfe1215f9b5 Mon Sep 17 00:00:00 2001 From: Etienne Audet-Cobello Date: Fri, 27 Sep 2024 18:50:07 -0400 Subject: [PATCH 6/9] move impl. to cluster_config_validate --- src/k8s/pkg/k8sd/app/hooks_bootstrap.go | 68 ------------------- src/k8s/pkg/k8sd/app/hooks_bootstrap_test.go | 57 ---------------- .../pkg/k8sd/types/cluster_config_validate.go | 66 ++++++++++++++++++ 3 files changed, 66 insertions(+), 125 deletions(-) diff --git a/src/k8s/pkg/k8sd/app/hooks_bootstrap.go b/src/k8s/pkg/k8sd/app/hooks_bootstrap.go index 07b47a7d9..2e35b9be6 100644 --- a/src/k8s/pkg/k8sd/app/hooks_bootstrap.go +++ b/src/k8s/pkg/k8sd/app/hooks_bootstrap.go @@ -320,18 +320,6 @@ func (a *App) onBootstrapControlPlane(ctx context.Context, s state.State, bootst return fmt.Errorf("failed to get IP address(es) from ServiceCIDR %q: %w", cfg.Network.GetServiceCIDR(), err) } - if cfg.Network.GetPodCIDR() != "" && cfg.Network.GetServiceCIDR() != "" { - 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) - } - } - // NOTE: Set the notBefore certificate time to the current time. notBefore := time.Now() @@ -524,59 +512,3 @@ func (a *App) onBootstrapControlPlane(ctx context.Context, s state.State, bootst a.NotifyUpdateNodeConfigController() return nil } - -// 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. -func validateCIDROverlap(podCIDR string, serviceCIDR string) error { - // Parse the CIDRs - podIPv4CIDR, podIPv6CIDR, err := utils.SplitCIDRStrings(podCIDR) - if err != nil { - return fmt.Errorf("failed to parse pod CIDR: %w", err) - } - - svcIPv4CIDR, svcIPv6CIDR, err := utils.SplitCIDRStrings(serviceCIDR) - if err != nil { - 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 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) - } - } - - // Check for IPv6 overlap - if podIPv6CIDR != "" && svcIPv6CIDR != "" { - if overlap, err := utils.CIDRsOverlap(podIPv6CIDR, svcIPv6CIDR); err != nil { - 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) - } - } - - return nil -} - -// 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) - } - - _, 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 index ef398e9f1..4879f7a48 100644 --- a/src/k8s/pkg/k8sd/app/hooks_bootstrap_test.go +++ b/src/k8s/pkg/k8sd/app/hooks_bootstrap_test.go @@ -1,58 +1 @@ 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/types/cluster_config_validate.go b/src/k8s/pkg/k8sd/types/cluster_config_validate.go index c673e3173..5a0997d86 100644 --- a/src/k8s/pkg/k8sd/types/cluster_config_validate.go +++ b/src/k8s/pkg/k8sd/types/cluster_config_validate.go @@ -6,6 +6,8 @@ import ( "net/netip" "net/url" "strings" + + "github.com/canonical/k8s/pkg/utils" ) func validateCIDRs(cidrString string) error { @@ -21,6 +23,62 @@ func validateCIDRs(cidrString string) error { return nil } +// 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. +func validateCIDROverlap(podCIDR string, serviceCIDR string) error { + // Parse the CIDRs + podIPv4CIDR, podIPv6CIDR, err := utils.SplitCIDRStrings(podCIDR) + if err != nil { + return fmt.Errorf("failed to parse pod CIDR: %w", err) + } + + svcIPv4CIDR, svcIPv6CIDR, err := utils.SplitCIDRStrings(serviceCIDR) + if err != nil { + 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 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) + } + } + + // Check for IPv6 overlap + if podIPv6CIDR != "" && svcIPv6CIDR != "" { + if overlap, err := utils.CIDRsOverlap(podIPv6CIDR, svcIPv6CIDR); err != nil { + 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) + } + } + + return nil +} + +// 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) + } + + _, 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 +} + // Validate that a ClusterConfig does not have conflicting or incompatible options. func (c *ClusterConfig) Validate() error { // check: validate that PodCIDR and ServiceCIDR are configured @@ -31,6 +89,14 @@ func (c *ClusterConfig) Validate() error { return fmt.Errorf("invalid service CIDR: %w", err) } + if err := validateCIDROverlap(c.Network.GetPodCIDR(), c.Network.GetServiceCIDR()); err != nil { + return fmt.Errorf("invalid cidr configuration: %w", err) + } + // Can't be an else-if, because default values could already be set. + if err := validateIPv6CIDRSize(c.Network.GetServiceCIDR()); err != nil { + return fmt.Errorf("invalid service CIDR: %w", err) + } + // check: ensure network is enabled if any of ingress, gateway, load-balancer are enabled if !c.Network.GetEnabled() { if c.Gateway.GetEnabled() { From 0f6d8307d55043cdca83b15d1662d59e22070566 Mon Sep 17 00:00:00 2001 From: Etienne Audet-Cobello Date: Fri, 27 Sep 2024 18:50:19 -0400 Subject: [PATCH 7/9] wip: move test to cluster_config_validate --- .../types/cluster_config_validate_test.go | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/src/k8s/pkg/k8sd/types/cluster_config_validate_test.go b/src/k8s/pkg/k8sd/types/cluster_config_validate_test.go index db58934ed..655a33612 100644 --- a/src/k8s/pkg/k8sd/types/cluster_config_validate_test.go +++ b/src/k8s/pkg/k8sd/types/cluster_config_validate_test.go @@ -13,12 +13,13 @@ func TestValidateCIDR(t *testing.T) { cidr string expectErr bool }{ - {cidr: "10.1.0.0/16"}, + {cidr: "192.168.0.0/16"}, {cidr: "2001:0db8::/32"}, {cidr: "10.1.0.0/16,2001:0db8::/32"}, {cidr: "", expectErr: true}, {cidr: "bananas", expectErr: true}, {cidr: "fd01::/64,fd02::/64,fd03::/64", expectErr: true}, + {cidr: "10.1.0.0/32", expectErr: true}, } { t.Run(tc.cidr, func(t *testing.T) { t.Run("Pod", func(t *testing.T) { @@ -128,3 +129,46 @@ func TestValidateExternalServers(t *testing.T) { }) } } + +func TestValidateCIDROverlap(t *testing.T) { + for _, tc := range []struct { + name string + podCIDR string + serviceCIDR string + expectErr bool + }{ + {"RandomName", "nogood", "nogood", true}, + {"BothValid", "10.1.0.0/16", "2001:0db8::/108", false}, + //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.0.0/32", "10.0.0.0/24", true}, + {"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", true}, + {"InvalidIPv6CIDRFormat", "2001:db8::1/129", "fc00::/64", true}, + {"MaxSizeIPv6CIDRs", "::/0", "::/0", true}, + //Mixed + {"IPv4AndIPv6MixedCIDRs", "192.168.0.0/16", "2001:db8::/32", true}, + {"OnlyInvalidIPv6CIDR", "", "2001:db8::/65", true}, + } { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + config := types.ClusterConfig{ + Network: types.Network{ + PodCIDR: utils.Pointer(tc.podCIDR), + ServiceCIDR: utils.Pointer(tc.serviceCIDR), + }, + } + err := config.Validate() + if tc.expectErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).To(BeNil()) + } + }) + } +} From b690edbb48193816fb071ef95bba69e6469f1d30 Mon Sep 17 00:00:00 2001 From: Etienne Audet-Cobello Date: Mon, 30 Sep 2024 12:38:17 -0400 Subject: [PATCH 8/9] simplify testing --- .../pkg/k8sd/types/cluster_config_validate.go | 4 ++ .../types/cluster_config_validate_test.go | 65 ++++--------------- 2 files changed, 16 insertions(+), 53 deletions(-) diff --git a/src/k8s/pkg/k8sd/types/cluster_config_validate.go b/src/k8s/pkg/k8sd/types/cluster_config_validate.go index 5a0997d86..692f8031d 100644 --- a/src/k8s/pkg/k8sd/types/cluster_config_validate.go +++ b/src/k8s/pkg/k8sd/types/cluster_config_validate.go @@ -66,6 +66,10 @@ func validateIPv6CIDRSize(serviceCIDR string) error { return fmt.Errorf("invalid CIDR: %w", err) } + if svcIPv6CIDR == "" { + return nil + } + _, ipv6Net, err := net.ParseCIDR(svcIPv6CIDR) if err != nil { return fmt.Errorf("invalid CIDR: %w", err) diff --git a/src/k8s/pkg/k8sd/types/cluster_config_validate_test.go b/src/k8s/pkg/k8sd/types/cluster_config_validate_test.go index 655a33612..b2ef53af5 100644 --- a/src/k8s/pkg/k8sd/types/cluster_config_validate_test.go +++ b/src/k8s/pkg/k8sd/types/cluster_config_validate_test.go @@ -10,16 +10,18 @@ import ( func TestValidateCIDR(t *testing.T) { for _, tc := range []struct { - cidr string - expectErr bool + cidr string + expectPodErr bool + expectSvcErr bool }{ {cidr: "192.168.0.0/16"}, - {cidr: "2001:0db8::/32"}, - {cidr: "10.1.0.0/16,2001:0db8::/32"}, - {cidr: "", expectErr: true}, - {cidr: "bananas", expectErr: true}, - {cidr: "fd01::/64,fd02::/64,fd03::/64", expectErr: true}, - {cidr: "10.1.0.0/32", expectErr: true}, + {cidr: "2001:0db8::/108"}, + {cidr: "10.2.0.0/16,2001:0db8::/108"}, + {cidr: "", expectPodErr: true, expectSvcErr: true}, + {cidr: "bananas", expectPodErr: true, expectSvcErr: true}, + {cidr: "fd01::/108,fd02::/108,fd03::/108", expectPodErr: true, expectSvcErr: true}, + {cidr: "10.1.0.0/32", expectPodErr: true, expectSvcErr: true}, + {cidr: "2001:0db8::/32", expectSvcErr: true}, } { t.Run(tc.cidr, func(t *testing.T) { t.Run("Pod", func(t *testing.T) { @@ -31,7 +33,7 @@ func TestValidateCIDR(t *testing.T) { }, } err := config.Validate() - if tc.expectErr { + if tc.expectPodErr { g.Expect(err).To(HaveOccurred()) } else { g.Expect(err).To(BeNil()) @@ -46,7 +48,7 @@ func TestValidateCIDR(t *testing.T) { }, } err := config.Validate() - if tc.expectErr { + if tc.expectSvcErr { g.Expect(err).To(HaveOccurred()) } else { g.Expect(err).To(BeNil()) @@ -129,46 +131,3 @@ func TestValidateExternalServers(t *testing.T) { }) } } - -func TestValidateCIDROverlap(t *testing.T) { - for _, tc := range []struct { - name string - podCIDR string - serviceCIDR string - expectErr bool - }{ - {"RandomName", "nogood", "nogood", true}, - {"BothValid", "10.1.0.0/16", "2001:0db8::/108", false}, - //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.0.0/32", "10.0.0.0/24", true}, - {"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", true}, - {"InvalidIPv6CIDRFormat", "2001:db8::1/129", "fc00::/64", true}, - {"MaxSizeIPv6CIDRs", "::/0", "::/0", true}, - //Mixed - {"IPv4AndIPv6MixedCIDRs", "192.168.0.0/16", "2001:db8::/32", true}, - {"OnlyInvalidIPv6CIDR", "", "2001:db8::/65", true}, - } { - t.Run(tc.name, func(t *testing.T) { - g := NewWithT(t) - config := types.ClusterConfig{ - Network: types.Network{ - PodCIDR: utils.Pointer(tc.podCIDR), - ServiceCIDR: utils.Pointer(tc.serviceCIDR), - }, - } - err := config.Validate() - if tc.expectErr { - g.Expect(err).To(HaveOccurred()) - } else { - g.Expect(err).To(BeNil()) - } - }) - } -} From b5b6ad2aac1c913caeba03240676fc388aa7cd26 Mon Sep 17 00:00:00 2001 From: Etienne Audet-Cobello Date: Mon, 30 Sep 2024 12:55:43 -0400 Subject: [PATCH 9/9] review comments --- src/k8s/pkg/k8sd/app/hooks_bootstrap_test.go | 1 - src/k8s/pkg/k8sd/types/cluster_config_validate.go | 7 +++---- src/k8s/pkg/utils/cidr.go | 8 ++++++-- 3 files changed, 9 insertions(+), 7 deletions(-) delete mode 100644 src/k8s/pkg/k8sd/app/hooks_bootstrap_test.go diff --git a/src/k8s/pkg/k8sd/app/hooks_bootstrap_test.go b/src/k8s/pkg/k8sd/app/hooks_bootstrap_test.go deleted file mode 100644 index 4879f7a48..000000000 --- a/src/k8s/pkg/k8sd/app/hooks_bootstrap_test.go +++ /dev/null @@ -1 +0,0 @@ -package app diff --git a/src/k8s/pkg/k8sd/types/cluster_config_validate.go b/src/k8s/pkg/k8sd/types/cluster_config_validate.go index 692f8031d..8ec1b8d50 100644 --- a/src/k8s/pkg/k8sd/types/cluster_config_validate.go +++ b/src/k8s/pkg/k8sd/types/cluster_config_validate.go @@ -23,7 +23,7 @@ func validateCIDRs(cidrString string) error { return nil } -// validateCIDROverlapAndSize checks for overlap and size constraints between pod and service CIDRs. +// validateCIDROverlap 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. func validateCIDROverlap(podCIDR string, serviceCIDR string) error { // Parse the CIDRs @@ -58,7 +58,7 @@ func validateCIDROverlap(podCIDR string, serviceCIDR string) error { return nil } -// Check CIDR size ensures that the service IPv6 CIDR is not larger than /108. +// validateIPv6CIDRSize 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) @@ -75,8 +75,7 @@ func validateIPv6CIDRSize(serviceCIDR string) error { return fmt.Errorf("invalid CIDR: %w", err) } - prefixLength, _ := ipv6Net.Mask.Size() - if prefixLength < 108 { + if prefixLength, _ := ipv6Net.Mask.Size(); prefixLength < 108 { return fmt.Errorf("service CIDR %q cannot be larger than /108", serviceCIDR) } diff --git a/src/k8s/pkg/utils/cidr.go b/src/k8s/pkg/utils/cidr.go index ffe7bdfa3..9b129b68f 100644 --- a/src/k8s/pkg/utils/cidr.go +++ b/src/k8s/pkg/utils/cidr.go @@ -150,8 +150,12 @@ func CIDRsOverlap(cidr1, cidr2 string) (bool, error) { _, ipNet1, err1 := net.ParseCIDR(cidr1) _, ipNet2, err2 := net.ParseCIDR(cidr2) - if err1 != nil || err2 != nil { - return false, fmt.Errorf("invalid CIDR blocks, %v, %v", err1, err2) + if err1 != nil { + return false, fmt.Errorf("couldn't parse CIDR block %q: %w", cidr1, err1) + } + + if err2 != nil { + return false, fmt.Errorf("couldn't parse CIDR block %q: %w", cidr2, err2) } if ipNet1.Contains(ipNet2.IP) || ipNet2.Contains(ipNet1.IP) {