Add validation for number of CIDR ranges specified in Clusters

Signed-off-by: killianmuldoon <[email protected]>
kubernetes-sigs · Oct 18, 2022 · 03f6846 · 03f6846
1 parent 61b22ba
commit 03f6846
Show file tree

Hide file tree

Showing 6 changed files with 129 additions and 3 deletions.
diff --git a/api/v1beta1/cluster_types.go b/api/v1beta1/cluster_types.go
@@ -468,8 +468,14 @@ func ipFamilyForCIDRStrings(cidrs []string) (ClusterIPFamily, error) {
 			return InvalidIPFamily, fmt.Errorf("could not parse CIDR: %s", err)
 		}
 		if ip.To4() != nil {
+			if foundIPv4 {
+				return InvalidIPFamily, fmt.Errorf("multiple CIDRs in the IPv4 family can not be specified")
+			}
 			foundIPv4 = true
 		} else {
+			if foundIPv6 {
+				return InvalidIPFamily, fmt.Errorf("multiple CIDRs in the IPv6 family can not be specified")
+			}
 			foundIPv6 = true
 		}
 	}

diff --git a/api/v1beta1/cluster_types_test.go b/api/v1beta1/cluster_types_test.go
@@ -183,6 +183,26 @@ func TestClusterIPFamily(t *testing.T) {
 			expectErr: "services: too many CIDRs specified",
 			c:         clusterWithNetwork(nil, []string{"192.168.0.0/16", "fd00:100:96::/48", "10.128.0.0/12"}),
 		},
+		{
+			name:      "services: multiple IPv4 CIDRs",
+			expectErr: "services: multiple CIDRs in the IPv4 family can not be specified",
+			c:         clusterWithNetwork(nil, []string{"192.168.0.0/16", "10.128.0.0/12"}),
+		},
+		{
+			name:      "pods: multiple IPv4 CIDRs",
+			expectErr: "pods: multiple CIDRs in the IPv4 family can not be specified",
+			c:         clusterWithNetwork([]string{"192.168.0.0/16", "10.128.0.0/12"}, nil),
+		},
+		{
+			name:      "services: multiple IPv4 CIDRs",
+			expectErr: "services: multiple CIDRs in the IPv6 family can not be specified",
+			c:         clusterWithNetwork(nil, []string{"fd00:100:96::/48", "fd00:100:97::/48"}),
+		},
+		{
+			name:      "pods: multiple IPv4 CIDRs",
+			expectErr: "pods: multiple CIDRs in the IPv6 family can not be specified",
+			c:         clusterWithNetwork([]string{"fd00:100:96::/48", "fd00:100:97::/48"}, nil),
+		},
 	}
 
 	for _, tt := range invalid {

diff --git a/internal/test/builder/builders.go b/internal/test/builder/builders.go
@@ -39,6 +39,7 @@ type ClusterBuilder struct {
 	topology              *clusterv1.Topology
 	infrastructureCluster *unstructured.Unstructured
 	controlPlane          *unstructured.Unstructured
+	network               *clusterv1.ClusterNetwork
 }
 
 // Cluster returns a ClusterBuilder with the given name and namespace.
@@ -49,6 +50,12 @@ func Cluster(namespace, name string) *ClusterBuilder {
 	}
 }
 
+// WithClusterNetwork sets the ClusterNetwork for the ClusterBuilder.
+func (c *ClusterBuilder) WithClusterNetwork(clusterNetwork *clusterv1.ClusterNetwork) *ClusterBuilder {
+	c.network = clusterNetwork
+	return c
+}
+
 // WithLabels sets the labels for the ClusterBuilder.
 func (c *ClusterBuilder) WithLabels(labels map[string]string) *ClusterBuilder {
 	c.labels = labels
@@ -93,7 +100,8 @@ func (c *ClusterBuilder) Build() *clusterv1.Cluster {
 			Annotations: c.annotations,
 		},
 		Spec: clusterv1.ClusterSpec{
-			Topology: c.topology,
+			Topology:       c.topology,
+			ClusterNetwork: c.network,
 		},
 	}
 	if c.infrastructureCluster != nil {

diff --git a/internal/test/builder/zz_generated.deepcopy.go b/internal/test/builder/zz_generated.deepcopy.go
diff --git a/internal/webhooks/cluster.go b/internal/webhooks/cluster.go
@@ -22,6 +22,7 @@ import (
 	"strings"
 
 	"github.com/blang/semver"
+	"github.com/davecgh/go-spew/spew"
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -170,6 +171,18 @@ func (webhook *Cluster) validate(ctx context.Context, oldCluster, newCluster *cl
 			),
 		)
 	}
+	if newCluster.Spec.ClusterNetwork != nil {
+		if _, err := newCluster.GetIPFamily(); err != nil {
+			allErrs = append(
+				allErrs,
+				field.Invalid(
+					specPath.Child("clusterNetwork"),
+					spew.Sprint(newCluster.Spec.ClusterNetwork),
+					err.Error(),
+				))
+		}
+	}
+
 	topologyPath := specPath.Child("topology")
 
 	// Validate the managed topology, if defined.
@@ -433,7 +446,7 @@ func validateMachineHealthChecks(cluster *clusterv1.Cluster, clusterClass *clust
 }
 
 // machineDeploymentClassOfName find a MachineDeploymentClass of the given name in the provided ClusterClass.
-// Returns nill if can not find one.
+// Returns nil if can not find one.
 // TODO: Check if there is already a helper function that can do this.
 func machineDeploymentClassOfName(clusterClass *clusterv1.ClusterClass, name string) *clusterv1.MachineDeploymentClass {
 	for _, mdClass := range clusterClass.Spec.Workers.MachineDeployments {

diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go
@@ -360,7 +360,6 @@ func TestClusterDefaultTopologyVersion(t *testing.T) {
 
 func TestClusterValidation(t *testing.T) {
 	// NOTE: ClusterTopology feature flag is disabled by default, thus preventing to set Cluster.Topologies.
-
 	var (
 		tests = []struct {
 			name      string
@@ -409,6 +408,81 @@ func TestClusterValidation(t *testing.T) {
 					WithTopology(&clusterv1.Topology{}).
 					Build(),
 			},
+			{
+				name:      "pass with valid CIDR ranges",
+				expectErr: false,
+				in: builder.Cluster("fooNamespace", "cluster1").
+					WithClusterNetwork(&clusterv1.ClusterNetwork{
+						Services: &clusterv1.NetworkRanges{
+							CIDRBlocks: []string{"10.10.10.10/24"}},
+						Pods: &clusterv1.NetworkRanges{
+							CIDRBlocks: []string{"10.10.10.10/24"}},
+					}).
+					Build(),
+			},
+			{
+				name:      "fails if multiple CIDR ranges of IPv4 are passed",
+				expectErr: true,
+				in: builder.Cluster("fooNamespace", "cluster1").
+					WithClusterNetwork(&clusterv1.ClusterNetwork{
+						Services: &clusterv1.NetworkRanges{
+							CIDRBlocks: []string{"10.10.10.10/24", "11.11.11.11/24"}},
+					}).
+					Build(),
+			},
+			{
+				name:      "fails if multiple CIDR ranges of IPv6 are passed",
+				expectErr: true,
+				in: builder.Cluster("fooNamespace", "cluster1").
+					WithClusterNetwork(&clusterv1.ClusterNetwork{
+						Services: &clusterv1.NetworkRanges{
+							CIDRBlocks: []string{"2002::1234:abcd:ffff:c0a8:101/64", "2004::1234:abcd:ffff:c0a8:101/64"}},
+					}).
+					Build(),
+			},
+
+			{
+				name:      "fails if too many cidr ranges are specified in the clusterNetwork pods field",
+				expectErr: true,
+				in: builder.Cluster("fooNamespace", "cluster1").
+					WithClusterNetwork(&clusterv1.ClusterNetwork{
+						Pods: &clusterv1.NetworkRanges{
+							CIDRBlocks: []string{"10.10.10.10/24", "11.11.11.11/24", "12.12.12.12/24"}}}).
+					Build(),
+			},
+			{
+				name:      "fails if too many cidr ranges are specified in the clusterNetwork pods field",
+				expectErr: true,
+				in: builder.Cluster("fooNamespace", "cluster1").
+					WithClusterNetwork(&clusterv1.ClusterNetwork{
+						Services: &clusterv1.NetworkRanges{
+							CIDRBlocks: []string{"10.10.10.10/24", "11.11.11.11/24", "12.12.12.12/24"}}}).
+					Build(),
+			},
+			{
+				name:      "fails if cidr ranges are not valid",
+				expectErr: true,
+				in: builder.Cluster("fooNamespace", "cluster1").
+					WithClusterNetwork(&clusterv1.ClusterNetwork{
+						Services: &clusterv1.NetworkRanges{
+							// Invalid ranges: missing network prefix
+							CIDRBlocks: []string{"10.10.10.10", "11.11.11.11"}}}).
+					Build(),
+			},
+			{
+				name:      "fails if pod and service cidr families do not match",
+				expectErr: true,
+				in: builder.Cluster("fooNamespace", "cluster1").
+					WithClusterNetwork(&clusterv1.ClusterNetwork{
+						// Services are IPv4
+						Services: &clusterv1.NetworkRanges{
+							CIDRBlocks: []string{"10.10.10.10/24", "11.11.11.11/24"}},
+						// Pods are IPv6
+						Pods: &clusterv1.NetworkRanges{
+							CIDRBlocks: []string{"2002::1234:abcd:ffff:c0a8:101/64"}},
+					}).
+					Build(),
+			},
 		}
 	)
 	for _, tt := range tests {