Merge pull request #14997 from johngmyers/validate-addlcidrs

Validate nonMasqueradeCIDR doesn't overlap additionalNetworkCIDRs

pkg/apis/kops/validation/legacy.go

@@ -73,82 +73,15 @@ func ValidateCluster(c *kops.Cluster, strict bool) field.ErrorList {
 		allErrs = append(allErrs, field.Required(fieldSpec.Child("docker"), "docker not configured"))
 	}
 
-	var networkCIDR *net.IPNet
+	var nonMasqueradeCIDR *net.IPNet
+	var serviceClusterIPRange *net.IPNet
 	var err error
 
-	if c.Spec.Networking.NetworkCIDR != "" {
+	if c.Spec.Networking.NonMasqueradeCIDR != "" {
-		_, networkCIDR, _ = net.ParseCIDR(c.Spec.Networking.NetworkCIDR)
+		_, nonMasqueradeCIDR, _ = net.ParseCIDR(c.Spec.Networking.NonMasqueradeCIDR)
 	}
-
+	if c.Spec.Networking.ServiceClusterIPRange != "" {
-	// nonMasqueradeCIDR is essentially deprecated, and we're moving to cluster-cidr instead (which is better named pod-cidr)
+		_, serviceClusterIPRange, _ = net.ParseCIDR(c.Spec.Networking.ServiceClusterIPRange)
-	nonMasqueradeCIDRRequired := true
-	serviceClusterMustBeSubnetOfNonMasqueradeCIDR := true
-	if c.Spec.Networking.GCE != nil {
-		nonMasqueradeCIDRRequired = false
-		serviceClusterMustBeSubnetOfNonMasqueradeCIDR = false
-	}
-
-	// Check NonMasqueradeCIDR
-	var nonMasqueradeCIDR *net.IPNet
-	{
-		nonMasqueradeCIDRString := c.Spec.Networking.NonMasqueradeCIDR
-		if nonMasqueradeCIDRString == "" {
-			if nonMasqueradeCIDRRequired {
-				allErrs = append(allErrs, field.Required(fieldSpec.Child("networking", "nonMasqueradeCIDR"), "Cluster did not have nonMasqueradeCIDR set"))
-			}
-		} else {
-			_, nonMasqueradeCIDR, err = net.ParseCIDR(nonMasqueradeCIDRString)
-			if err != nil {
-				allErrs = append(allErrs, field.Invalid(fieldSpec.Child("networking", "nonMasqueradeCIDR"), nonMasqueradeCIDRString, "Cluster had an invalid nonMasqueradeCIDR"))
-			} else {
-				if strings.Contains(nonMasqueradeCIDRString, ":") && nonMasqueradeCIDRString != "::/0" {
-					allErrs = append(allErrs, field.Forbidden(fieldSpec.Child("networking", "nonMasqueradeCIDR"), "IPv6 clusters must have a nonMasqueradeCIDR of \"::/0\""))
-				}
-
-				if networkCIDR != nil && subnet.Overlap(nonMasqueradeCIDR, networkCIDR) && c.Spec.Networking.AmazonVPC == nil && (c.Spec.Networking.Cilium == nil || c.Spec.Networking.Cilium.IPAM != kops.CiliumIpamEni) {
-					allErrs = append(allErrs, field.Forbidden(fieldSpec.Child("networking", "nonMasqueradeCIDR"), fmt.Sprintf("nonMasqueradeCIDR %q cannot overlap with networkCIDR %q", nonMasqueradeCIDRString, c.Spec.Networking.NetworkCIDR)))
-				}
-
-				if c.Spec.ContainerRuntime == "docker" && c.Spec.Kubelet != nil && fi.ValueOf(c.Spec.Kubelet.NetworkPluginName) == "kubenet" {
-					if fi.ValueOf(c.Spec.Kubelet.NonMasqueradeCIDR) != nonMasqueradeCIDRString {
-						if strict || c.Spec.Kubelet.NonMasqueradeCIDR != nil {
-							allErrs = append(allErrs, field.Forbidden(fieldSpec.Child("kubelet", "networking", "nonMasqueradeCIDR"), "kubelet nonMasqueradeCIDR did not match cluster nonMasqueradeCIDR"))
-						}
-					}
-					if fi.ValueOf(c.Spec.ControlPlaneKubelet.NonMasqueradeCIDR) != nonMasqueradeCIDRString {
-						if strict || c.Spec.ControlPlaneKubelet.NonMasqueradeCIDR != nil {
-							allErrs = append(allErrs, field.Forbidden(fieldSpec.Child("controlPlaneKubelet", "nonMasqueradeCIDR"), "controlPlaneKubelet nonMasqueradeCIDR did not match cluster nonMasqueradeCIDR"))
-						}
-					}
-				}
-			}
-		}
-	}
-
-	// Check ServiceClusterIPRange
-	var serviceClusterIPRange *net.IPNet
-	{
-		serviceClusterIPRangeString := c.Spec.Networking.ServiceClusterIPRange
-		if serviceClusterIPRangeString == "" {
-			if strict {
-				allErrs = append(allErrs, field.Required(fieldSpec.Child("networking", "serviceClusterIPRange"), "Cluster did not have serviceClusterIPRange set"))
-			}
-		} else {
-			_, serviceClusterIPRange, err = net.ParseCIDR(serviceClusterIPRangeString)
-			if err != nil {
-				allErrs = append(allErrs, field.Invalid(fieldSpec.Child("networking", "serviceClusterIPRange"), serviceClusterIPRangeString, "Cluster had an invalid serviceClusterIPRange"))
-			} else {
-				if nonMasqueradeCIDR != nil && serviceClusterMustBeSubnetOfNonMasqueradeCIDR && !subnet.BelongsTo(nonMasqueradeCIDR, serviceClusterIPRange) {
-					allErrs = append(allErrs, field.Forbidden(fieldSpec.Child("networking", "serviceClusterIPRange"), fmt.Sprintf("serviceClusterIPRange %q must be a subnet of nonMasqueradeCIDR %q", serviceClusterIPRangeString, c.Spec.Networking.NonMasqueradeCIDR)))
-				}
-
-				if c.Spec.KubeAPIServer != nil && c.Spec.KubeAPIServer.ServiceClusterIPRange != serviceClusterIPRangeString {
-					if strict || c.Spec.KubeAPIServer.ServiceClusterIPRange != "" {
-						allErrs = append(allErrs, field.Forbidden(fieldSpec.Child("kubeAPIServer", "serviceClusterIPRange"), "kubeAPIServer serviceClusterIPRange did not match cluster serviceClusterIPRange"))
-					}
-				}
-			}
-		}
 	}
 
 	// Check ClusterCIDR

pkg/apis/kops/validation/validation.go

@@ -35,6 +35,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 	utilvalidation "k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/kops/pkg/util/subnet"
 
 	"k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/pkg/featureflag"
@@ -127,7 +128,7 @@ func validateClusterSpec(spec *kops.ClusterSpec, c *kops.Cluster, fieldPath *fie
 	}
 
 	if spec.KubeAPIServer != nil {
-		allErrs = append(allErrs, validateKubeAPIServer(spec.KubeAPIServer, c, fieldPath.Child("kubeAPIServer"))...)
+		allErrs = append(allErrs, validateKubeAPIServer(spec.KubeAPIServer, c, fieldPath.Child("kubeAPIServer"), strict)...)
 	}
 
 	if spec.ExternalCloudControllerManager == nil && spec.IsIPv6Only() {
@@ -290,17 +291,21 @@ func validateClusterSpec(spec *kops.ClusterSpec, c *kops.Cluster, fieldPath *fie
 }
 
 type cloudProviderConstraints struct {
-	requiresSubnets      bool
+	requiresSubnets                                 bool
-	requiresNetworkCIDR  bool
+	requiresNetworkCIDR                             bool
-	prohibitsNetworkCIDR bool
+	prohibitsNetworkCIDR                            bool
-	requiresSubnetCIDR   bool
+	requiresNonMasqueradeCIDR                       bool
+	requiresServiceClusterSubnetOfNonMasqueradeCIDR bool
+	requiresSubnetCIDR                              bool
 }
 
 func validateCloudProvider(c *kops.Cluster, provider *kops.CloudProviderSpec, fieldSpec *field.Path) (allErrs field.ErrorList, constraints *cloudProviderConstraints) {
 	constraints = &cloudProviderConstraints{
-		requiresSubnets:     true,
+		requiresSubnets:                                 true,
-		requiresNetworkCIDR: true,
+		requiresNetworkCIDR:                             true,
-		requiresSubnetCIDR:  true,
+		requiresNonMasqueradeCIDR:                       true,
+		requiresServiceClusterSubnetOfNonMasqueradeCIDR: true,
+		requiresSubnetCIDR:                              true,
 	}
 
 	optionTaken := false
@@ -335,6 +340,8 @@ func validateCloudProvider(c *kops.Cluster, provider *kops.CloudProviderSpec, fi
 		if c.Spec.Networking.NetworkCIDR != "" {
 			allErrs = append(allErrs, field.Forbidden(fieldSpec.Child("networking", "networkCIDR"), "networkCIDR should not be set on GCE"))
 		}
+		constraints.requiresNonMasqueradeCIDR = false
+		constraints.requiresServiceClusterSubnetOfNonMasqueradeCIDR = false
 	}
 	if c.Spec.CloudProvider.Hetzner != nil {
 		if optionTaken {
@@ -679,7 +686,7 @@ func validateExecContainerAction(v *kops.ExecContainerAction, fldPath *field.Pat
 	return allErrs
 }
 
-func validateKubeAPIServer(v *kops.KubeAPIServerConfig, c *kops.Cluster, fldPath *field.Path) field.ErrorList {
+func validateKubeAPIServer(v *kops.KubeAPIServerConfig, c *kops.Cluster, fldPath *field.Path, strict bool) field.ErrorList {
 	allErrs := field.ErrorList{}
 
 	if len(v.AdmissionControl) > 0 {
@@ -774,6 +781,12 @@ func validateKubeAPIServer(v *kops.KubeAPIServerConfig, c *kops.Cluster, fldPath
 		}
 	}
 
+	if v.ServiceClusterIPRange != c.Spec.Networking.ServiceClusterIPRange {
+		if strict || v.ServiceClusterIPRange != "" {
+			allErrs = append(allErrs, field.Forbidden(fldPath.Child("serviceClusterIPRange"), "kubeAPIServer serviceClusterIPRange did not match cluster serviceClusterIPRange"))
+		}
+	}
+
 	return allErrs
 }
 
@@ -865,6 +878,10 @@ func validateKubelet(k *kops.KubeletConfigSpec, c *kops.Cluster, kubeletPath *fi
 			if k.NonMasqueradeCIDR != nil {
 				allErrs = append(allErrs, field.Forbidden(kubeletPath.Child("nonMasqueradeCIDR"), "nonMasqueradeCIDR has been removed on Kubernetes >=1.24"))
 			}
+		} else {
+			if c.Spec.ContainerRuntime == "docker" && fi.ValueOf(c.Spec.Kubelet.NetworkPluginName) == "kubenet" && fi.ValueOf(k.NonMasqueradeCIDR) != c.Spec.Networking.NonMasqueradeCIDR {
+				allErrs = append(allErrs, field.Forbidden(kubeletPath.Child("nonMasqueradeCIDR"), "kubelet nonMasqueradeCIDR does not match cluster nonMasqueradeCIDR"))
+			}
 		}
 
 		if k.ShutdownGracePeriodCriticalPods != nil {
@@ -907,6 +924,47 @@ func validateNetworking(cluster *kops.Cluster, v *kops.NetworkingSpec, fldPath *
 		allErrs = append(allErrs, validateCIDR(cidr, fldPath.Child("additionalNetworkCIDRs").Index(i), &networkCIDRs)...)
 	}
 
+	var nonMasqueradeCIDRs []*net.IPNet
+	{
+		if v.NonMasqueradeCIDR == "" {
+			if providerConstraints.requiresNonMasqueradeCIDR {
+				allErrs = append(allErrs, field.Required(fldPath.Child("nonMasqueradeCIDR"), "Cluster does not have nonMasqueradeCIDR set"))
+			}
+		} else {
+			allErrs = append(allErrs, validateCIDR(v.NonMasqueradeCIDR, fldPath.Child("nonMasqueradeCIDR"), &nonMasqueradeCIDRs)...)
+
+			if strings.Contains(v.NonMasqueradeCIDR, ":") && v.NonMasqueradeCIDR != "::/0" {
+				allErrs = append(allErrs, field.Forbidden(fldPath.Child("nonMasqueradeCIDR"), "IPv6 clusters must have a nonMasqueradeCIDR of \"::/0\""))
+			}
+
+			if len(networkCIDRs) > 0 && v.AmazonVPC == nil && (v.Cilium == nil || v.Cilium.IPAM != kops.CiliumIpamEni) {
+				if subnet.Overlap(nonMasqueradeCIDRs[0], networkCIDRs[0]) {
+					allErrs = append(allErrs, field.Forbidden(fldPath.Child("nonMasqueradeCIDR"), fmt.Sprintf("nonMasqueradeCIDR %q cannot overlap with networkCIDR %q", v.NonMasqueradeCIDR, v.NetworkCIDR)))
+				}
+				for i, cidr := range networkCIDRs[1:] {
+					if subnet.Overlap(nonMasqueradeCIDRs[0], cidr) {
+						allErrs = append(allErrs, field.Forbidden(fldPath.Child("nonMasqueradeCIDR"), fmt.Sprintf("nonMasqueradeCIDR %q cannot overlap with additionalNetworkCIDRs[%d] %q", v.NonMasqueradeCIDR, i, cidr)))
+					}
+				}
+			}
+		}
+	}
+
+	var serviceClusterIPRanges []*net.IPNet
+	{
+		if v.ServiceClusterIPRange == "" {
+			if strict {
+				allErrs = append(allErrs, field.Required(fldPath.Child("serviceClusterIPRange"), "Cluster did not have serviceClusterIPRange set"))
+			}
+		} else {
+			allErrs = append(allErrs, validateCIDR(v.ServiceClusterIPRange, fldPath.Child("serviceClusterIPRange"), &serviceClusterIPRanges)...)
+
+			if len(nonMasqueradeCIDRs) > 0 && providerConstraints.requiresServiceClusterSubnetOfNonMasqueradeCIDR && !subnet.BelongsTo(nonMasqueradeCIDRs[0], serviceClusterIPRanges[0]) {
+				allErrs = append(allErrs, field.Forbidden(fldPath.Child("serviceClusterIPRange"), fmt.Sprintf("serviceClusterIPRange %q must be a subnet of nonMasqueradeCIDR %q", v.ServiceClusterIPRange, v.NonMasqueradeCIDR)))
+			}
+		}
+	}
+
 	allErrs = append(allErrs, validateSubnets(&cluster.Spec, v.Subnets, fldPath.Child("subnets"), strict, providerConstraints, networkCIDRs)...)
 
 	if v.Topology != nil {

pkg/apis/kops/validation/validation_test.go

@@ -317,7 +317,7 @@ func TestValidateKubeAPIServer(t *testing.T) {
 				},
 			}
 		}
-		errs := validateKubeAPIServer(&g.Input, g.Cluster, field.NewPath("KubeAPIServer"))
+		errs := validateKubeAPIServer(&g.Input, g.Cluster, field.NewPath("KubeAPIServer"), true)
 
 		testErrors(t, g.Input, errs, g.ExpectedErrors)
 
@@ -392,7 +392,9 @@ func Test_Validate_Networking_Flannel(t *testing.T) {
 		cluster := &kops.Cluster{
 			Spec: kops.ClusterSpec{
 				Networking: kops.NetworkingSpec{
-					NetworkCIDR: "10.0.0.0/8",
+					NetworkCIDR:           "10.0.0.0/8",
+					NonMasqueradeCIDR:     "100.64.0.0/10",
+					ServiceClusterIPRange: "100.64.0.0/13",
 					Subnets: []kops.ClusterSubnetSpec{
 						{
 							Name: "sg-test",
@@ -456,7 +458,9 @@ func Test_Validate_AdditionalPolicies(t *testing.T) {
 				AWS: &kops.AWSSpec{},
 			},
 			Networking: kops.NetworkingSpec{
-				NetworkCIDR: "10.10.0.0/16",
+				NetworkCIDR:           "10.10.0.0/16",
+				NonMasqueradeCIDR:     "100.64.0.0/10",
+				ServiceClusterIPRange: "100.64.0.0/13",
 				Subnets: []kops.ClusterSubnetSpec{
 					{
 						Name: "subnet1",