Merge pull request #123174 from danwinship/cidr-validation-cleanup

Make CIDR validation consistent

pkg/apis/core/validation/validation.go

@@ -5450,24 +5450,32 @@ func ValidateService(service *core.Service) field.ErrorList {
 		ports[key] = true
 	}
 
-	// Validate SourceRange field and annotation
+	// Validate SourceRanges field or annotation.
-	_, ok := service.Annotations[core.AnnotationLoadBalancerSourceRangesKey]
+	if len(service.Spec.LoadBalancerSourceRanges) > 0 {
-	if len(service.Spec.LoadBalancerSourceRanges) > 0 || ok {
+		fieldPath := specPath.Child("LoadBalancerSourceRanges")
-		var fieldPath *field.Path
+
-		var val string
-		if len(service.Spec.LoadBalancerSourceRanges) > 0 {
-			fieldPath = specPath.Child("LoadBalancerSourceRanges")
-			val = fmt.Sprintf("%v", service.Spec.LoadBalancerSourceRanges)
-		} else {
-			fieldPath = field.NewPath("metadata", "annotations").Key(core.AnnotationLoadBalancerSourceRangesKey)
-			val = service.Annotations[core.AnnotationLoadBalancerSourceRangesKey]
-		}
 		if service.Spec.Type != core.ServiceTypeLoadBalancer {
 			allErrs = append(allErrs, field.Forbidden(fieldPath, "may only be used when `type` is 'LoadBalancer'"))
 		}
-		_, err := apiservice.GetLoadBalancerSourceRanges(service)
+		for idx, value := range service.Spec.LoadBalancerSourceRanges {
-		if err != nil {
+			// Note: due to a historical accident around transition from the
-			allErrs = append(allErrs, field.Invalid(fieldPath, val, "must be a list of IP ranges. For example, 10.240.0.0/24,10.250.0.0/24 "))
+			// annotation value, these values are allowed to be space-padded.
+			value = strings.TrimSpace(value)
+			allErrs = append(allErrs, validation.IsValidCIDR(fieldPath.Index(idx), value)...)
+		}
+	} else if val, annotationSet := service.Annotations[core.AnnotationLoadBalancerSourceRangesKey]; annotationSet {
+		fieldPath := field.NewPath("metadata", "annotations").Key(core.AnnotationLoadBalancerSourceRangesKey)
+		if service.Spec.Type != core.ServiceTypeLoadBalancer {
+			allErrs = append(allErrs, field.Forbidden(fieldPath, "may only be used when `type` is 'LoadBalancer'"))
+		}
+
+		val = strings.TrimSpace(val)
+		if val != "" {
+			cidrs := strings.Split(val, ",")
+			for _, value := range cidrs {
+				value = strings.TrimSpace(value)
+				allErrs = append(allErrs, validation.IsValidCIDR(fieldPath, value)...)
+			}
 		}
 	}
 
@@ -5870,9 +5878,7 @@ func ValidateNode(node *core.Node) field.ErrorList {
 
 		// all PodCIDRs should be valid ones
 		for idx, value := range node.Spec.PodCIDRs {
-			if _, err := ValidateCIDR(value); err != nil {
+			allErrs = append(allErrs, validation.IsValidCIDR(podCIDRsField.Index(idx), value)...)
-				allErrs = append(allErrs, field.Invalid(podCIDRsField.Index(idx), node.Spec.PodCIDRs, "must be valid CIDR"))
-			}
 		}
 
 		// if more than PodCIDR then
@@ -7300,15 +7306,6 @@ func validateVolumeNodeAffinity(nodeAffinity *core.VolumeNodeAffinity, fldPath *
 	return true, allErrs
 }
 
-// ValidateCIDR validates whether a CIDR matches the conventions expected by net.ParseCIDR
-func ValidateCIDR(cidr string) (*net.IPNet, error) {
-	_, net, err := netutils.ParseCIDRSloppy(cidr)
-	if err != nil {
-		return nil, err
-	}
-	return net, nil
-}
-
 func IsDecremented(update, old *int32) bool {
 	if update == nil && old != nil {
 		return true

pkg/apis/core/validation/validation_test.go

@@ -16154,11 +16154,11 @@ func TestValidateServiceCreate(t *testing.T) {
 			s.Spec.Type = core.ServiceTypeLoadBalancer
 			s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster
 			s.Spec.AllocateLoadBalancerNodePorts = utilpointer.Bool(true)
-			s.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "1.2.3.4/8,  5.6.7.8/16"
+			s.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "1.2.3.0/24,  5.6.0.0/16"
 		},
 		numErrs: 0,
 	}, {
-		name: "empty LoadBalancer source range annotation",
+		name: "valid empty LoadBalancer source range annotation",
 		tweakSvc: func(s *core.Service) {
 			s.Spec.Type = core.ServiceTypeLoadBalancer
 			s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster
@@ -16166,12 +16166,24 @@ func TestValidateServiceCreate(t *testing.T) {
 			s.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = ""
 		},
 		numErrs: 0,
+	}, {
+		name: "valid whitespace-only LoadBalancer source range annotation",
+		tweakSvc: func(s *core.Service) {
+			s.Spec.Type = core.ServiceTypeLoadBalancer
+			s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster
+			s.Spec.AllocateLoadBalancerNodePorts = utilpointer.Bool(true)
+			s.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "  "
+		},
+		numErrs: 0,
 	}, {
 		name: "invalid LoadBalancer source range annotation (hostname)",
 		tweakSvc: func(s *core.Service) {
+			s.Spec.Type = core.ServiceTypeLoadBalancer
+			s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster
+			s.Spec.AllocateLoadBalancerNodePorts = utilpointer.Bool(true)
 			s.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "foo.bar"
 		},
-		numErrs: 2,
+		numErrs: 1,
 	}, {
 		name: "invalid LoadBalancer source range annotation (invalid CIDR)",
 		tweakSvc: func(s *core.Service) {
@@ -16182,9 +16194,15 @@ func TestValidateServiceCreate(t *testing.T) {
 		},
 		numErrs: 1,
 	}, {
-		name: "invalid source range for non LoadBalancer type service",
+		name: "invalid LoadBalancer source range annotation for non LoadBalancer type service",
 		tweakSvc: func(s *core.Service) {
-			s.Spec.LoadBalancerSourceRanges = []string{"1.2.3.4/8", "5.6.7.8/16"}
+			s.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "1.2.3.0/24"
+		},
+		numErrs: 1,
+	}, {
+		name: "invalid empty-but-set LoadBalancer source range annotation for non LoadBalancer type service",
+		tweakSvc: func(s *core.Service) {
+			s.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = ""
 		},
 		numErrs: 1,
 	}, {
@@ -16193,11 +16211,20 @@ func TestValidateServiceCreate(t *testing.T) {
 			s.Spec.Type = core.ServiceTypeLoadBalancer
 			s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster
 			s.Spec.AllocateLoadBalancerNodePorts = utilpointer.Bool(true)
-			s.Spec.LoadBalancerSourceRanges = []string{"1.2.3.4/8", "5.6.7.8/16"}
+			s.Spec.LoadBalancerSourceRanges = []string{"1.2.3.0/24", "5.6.0.0/16"}
 		},
 		numErrs: 0,
 	}, {
-		name: "empty LoadBalancer source range",
+		name: "valid LoadBalancer source range with whitespace",
+		tweakSvc: func(s *core.Service) {
+			s.Spec.Type = core.ServiceTypeLoadBalancer
+			s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster
+			s.Spec.AllocateLoadBalancerNodePorts = utilpointer.Bool(true)
+			s.Spec.LoadBalancerSourceRanges = []string{"1.2.3.0/24  ", " 5.6.0.0/16"}
+		},
+		numErrs: 0,
+	}, {
+		name: "invalid empty LoadBalancer source range",
 		tweakSvc: func(s *core.Service) {
 			s.Spec.Type = core.ServiceTypeLoadBalancer
 			s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster
@@ -16206,7 +16233,7 @@ func TestValidateServiceCreate(t *testing.T) {
 		},
 		numErrs: 1,
 	}, {
-		name: "invalid LoadBalancer source range",
+		name: "invalid LoadBalancer source range (hostname)",
 		tweakSvc: func(s *core.Service) {
 			s.Spec.Type = core.ServiceTypeLoadBalancer
 			s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster
@@ -16214,6 +16241,31 @@ func TestValidateServiceCreate(t *testing.T) {
 			s.Spec.LoadBalancerSourceRanges = []string{"foo.bar"}
 		},
 		numErrs: 1,
+	}, {
+		name: "invalid LoadBalancer source range (invalid CIDR)",
+		tweakSvc: func(s *core.Service) {
+			s.Spec.Type = core.ServiceTypeLoadBalancer
+			s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster
+			s.Spec.AllocateLoadBalancerNodePorts = utilpointer.Bool(true)
+			s.Spec.LoadBalancerSourceRanges = []string{"1.2.3.4/33"}
+		},
+		numErrs: 1,
+	}, {
+		name: "invalid source range for non LoadBalancer type service",
+		tweakSvc: func(s *core.Service) {
+			s.Spec.LoadBalancerSourceRanges = []string{"1.2.3.0/24", "5.6.0.0/16"}
+		},
+		numErrs: 1,
+	}, {
+		name: "invalid source range annotation ignored with valid source range field",
+		tweakSvc: func(s *core.Service) {
+			s.Spec.Type = core.ServiceTypeLoadBalancer
+			s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster
+			s.Spec.AllocateLoadBalancerNodePorts = utilpointer.Bool(true)
+			s.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "foo.bar"
+			s.Spec.LoadBalancerSourceRanges = []string{"1.2.3.0/24", "5.6.0.0/16"}
+		},
+		numErrs: 0,
 	}, {
 		name: "valid ExternalName",
 		tweakSvc: func(s *core.Service) {

pkg/apis/networking/validation/validation.go

@@ -216,27 +216,30 @@ func ValidateNetworkPolicyUpdate(update, old *networking.NetworkPolicy, opts Net
 // ValidateIPBlock validates a cidr and the except fields of an IpBlock NetworkPolicyPeer
 func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
-	if len(ipb.CIDR) == 0 || ipb.CIDR == "" {
+	if ipb.CIDR == "" {
 		allErrs = append(allErrs, field.Required(fldPath.Child("cidr"), ""))
 		return allErrs
 	}
-	cidrIPNet, err := apivalidation.ValidateCIDR(ipb.CIDR)
+	allErrs = append(allErrs, validation.IsValidCIDR(fldPath.Child("cidr"), ipb.CIDR)...)
+	_, cidrIPNet, err := netutils.ParseCIDRSloppy(ipb.CIDR)
 	if err != nil {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("cidr"), ipb.CIDR, "not a valid CIDR"))
+		// Implies validation would have failed so we already added errors for it.
 		return allErrs
 	}
-	exceptCIDR := ipb.Except
+
-	for i, exceptIP := range exceptCIDR {
+	for i, exceptCIDRStr := range ipb.Except {
 		exceptPath := fldPath.Child("except").Index(i)
-		exceptCIDR, err := apivalidation.ValidateCIDR(exceptIP)
+		allErrs = append(allErrs, validation.IsValidCIDR(exceptPath, exceptCIDRStr)...)
+		_, exceptCIDR, err := netutils.ParseCIDRSloppy(exceptCIDRStr)
 		if err != nil {
-			allErrs = append(allErrs, field.Invalid(exceptPath, exceptIP, "not a valid CIDR"))
+			// Implies validation would have failed so we already added errors for it.
-			return allErrs
+			continue
 		}
+
 		cidrMaskLen, _ := cidrIPNet.Mask.Size()
 		exceptMaskLen, _ := exceptCIDR.Mask.Size()
 		if !cidrIPNet.Contains(exceptCIDR.IP) || cidrMaskLen >= exceptMaskLen {
-			allErrs = append(allErrs, field.Invalid(exceptPath, exceptIP, "must be a strict subset of `cidr`"))
+			allErrs = append(allErrs, field.Invalid(exceptPath, exceptCIDRStr, "must be a strict subset of `cidr`"))
 		}
 	}
 	return allErrs

staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go

@@ -378,6 +378,16 @@ func IsValidIPv6Address(fldPath *field.Path, value string) field.ErrorList {
 	return allErrors
 }
 
+// IsValidCIDR tests that the argument is a valid CIDR value.
+func IsValidCIDR(fldPath *field.Path, value string) field.ErrorList {
+	var allErrors field.ErrorList
+	_, _, err := netutils.ParseCIDRSloppy(value)
+	if err != nil {
+		allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid CIDR value, (e.g. 10.9.8.0/24 or 2001:db8::/64)"))
+	}
+	return allErrors
+}
+
 const percentFmt string = "[0-9]+%"
 const percentErrMsg string = "a valid percent string must be a numeric string followed by an ending '%'"
 

staging/src/k8s.io/apimachinery/pkg/util/validation/validation_test.go

@@ -496,6 +496,128 @@ func TestIsValidIP(t *testing.T) {
 	}
 }
 
+func TestIsValidCIDR(t *testing.T) {
+	for _, tc := range []struct {
+		name string
+		in   string
+		err  string
+	}{
+		// GOOD VALUES
+		{
+			name: "ipv4",
+			in:   "1.0.0.0/8",
+		},
+		{
+			name: "ipv4, all IPs",
+			in:   "0.0.0.0/0",
+		},
+		{
+			name: "ipv4, single IP",
+			in:   "1.1.1.1/32",
+		},
+		{
+			name: "ipv6",
+			in:   "2001:4860:4860::/48",
+		},
+		{
+			name: "ipv6, all IPs",
+			in:   "::/0",
+		},
+		{
+			name: "ipv6, single IP",
+			in:   "::1/128",
+		},
+
+		// GOOD, THOUGH NON-CANONICAL, VALUES
+		{
+			name: "ipv6, extra 0s (non-canonical)",
+			in:   "2a00:79e0:2:0::/64",
+		},
+		{
+			name: "ipv6, capital letters (non-canonical)",
+			in:   "2001:DB8::/64",
+		},
+
+		// BAD VALUES WE CURRENTLY CONSIDER GOOD
+		{
+			name: "ipv4 with leading 0s",
+			in:   "1.1.01.0/24",
+		},
+		{
+			name: "ipv4-in-ipv6 with ipv4-sized prefix",
+			in:   "::ffff:1.1.1.0/24",
+		},
+		{
+			name: "ipv4-in-ipv6 with ipv6-sized prefix",
+			in:   "::ffff:1.1.1.0/120",
+		},
+		{
+			name: "ipv4 with bits past prefix",
+			in:   "1.2.3.4/24",
+		},
+		{
+			name: "ipv6 with bits past prefix",
+			in:   "2001:db8::1/64",
+		},
+		{
+			name: "prefix length with leading 0s",
+			in:   "192.168.0.0/016",
+		},
+
+		// BAD VALUES
+		{
+			name: "empty string",
+			in:   "",
+			err:  "must be a valid CIDR value",
+		},
+		{
+			name: "junk",
+			in:   "aaaaaaa",
+			err:  "must be a valid CIDR value",
+		},
+		{
+			name: "IP address",
+			in:   "1.2.3.4",
+			err:  "must be a valid CIDR value",
+		},
+		{
+			name: "partial URL",
+			in:   "192.168.0.1/healthz",
+			err:  "must be a valid CIDR value",
+		},
+		{
+			name: "partial URL 2",
+			in:   "192.168.0.1/0/99",
+			err:  "must be a valid CIDR value",
+		},
+		{
+			name: "negative prefix length",
+			in:   "192.168.0.0/-16",
+			err:  "must be a valid CIDR value",
+		},
+		{
+			name: "prefix length with sign",
+			in:   "192.168.0.0/+16",
+			err:  "must be a valid CIDR value",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			errs := IsValidCIDR(field.NewPath(""), tc.in)
+			if tc.err == "" {
+				if len(errs) != 0 {
+					t.Errorf("expected %q to be valid but got: %v", tc.in, errs)
+				}
+			} else {
+				if len(errs) != 1 {
+					t.Errorf("expected %q to have 1 error but got: %v", tc.in, errs)
+				} else if !strings.Contains(errs[0].Detail, tc.err) {
+					t.Errorf("expected error for %q to contain %q but got: %q", tc.in, tc.err, errs[0].Detail)
+				}
+			}
+		})
+	}
+}
+
 func TestIsHTTPHeaderName(t *testing.T) {
 	goodValues := []string{
 		// Common ones