review comments

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{

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)
-			}
-		})
-	}
-}

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

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)
+			}
+		})
+	}
+}

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{

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

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{

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")

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)

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 {