diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index bcbf344963a..61b543cc587 100644 --- a/pkg/apis/core/validation/validation.go +++ b/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 - _, ok := service.Annotations[core.AnnotationLoadBalancerSourceRangesKey] - if len(service.Spec.LoadBalancerSourceRanges) > 0 || ok { - 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] - } + // Validate SourceRanges field or annotation. + if len(service.Spec.LoadBalancerSourceRanges) > 0 { + fieldPath := specPath.Child("LoadBalancerSourceRanges") + if service.Spec.Type != core.ServiceTypeLoadBalancer { allErrs = append(allErrs, field.Forbidden(fieldPath, "may only be used when `type` is 'LoadBalancer'")) } - _, err := apiservice.GetLoadBalancerSourceRanges(service) - if err != nil { - 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 ")) + for idx, value := range service.Spec.LoadBalancerSourceRanges { + // Note: due to a historical accident around transition from the + // 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, field.Invalid(podCIDRsField.Index(idx), node.Spec.PodCIDRs, "must be valid CIDR")) - } + allErrs = append(allErrs, validation.IsValidCIDR(podCIDRsField.Index(idx), value)...) } // 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 diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 9dc0a70b8fe..1ae129e8a4f 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/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) { diff --git a/pkg/apis/networking/validation/validation.go b/pkg/apis/networking/validation/validation.go index 581c72e2819..0750851dd85 100644 --- a/pkg/apis/networking/validation/validation.go +++ b/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")) - return allErrs + // Implies validation would have failed so we already added errors for it. + 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 diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go index 0eb33e68aaf..94c3fdb77f4 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go +++ b/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 '%'" diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation_test.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/validation_test.go index 2ce11d560f7..da62083db72 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation_test.go +++ b/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