From 7a56b6e3f7ae2896a05c869be1e24a0885ccffce Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Wed, 27 Dec 2023 09:44:45 -0500 Subject: [PATCH 1/3] Add validation.IsValidCIDR Move apivalidation.ValidateCIDR to apimachinery, and rename it and change its return value to match the other functions. Also, add unit tests. (Also, while updating NetworkPolicy validation for the API change, fix a variable name that implied that IPBlock.Except[] is IP-valued rather than CIDR-valued.) --- pkg/apis/core/validation/validation.go | 13 +- pkg/apis/networking/validation/validation.go | 21 +-- .../pkg/util/validation/validation.go | 10 ++ .../pkg/util/validation/validation_test.go | 122 ++++++++++++++++++ 4 files changed, 145 insertions(+), 21 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index bcbf344963a..686d184781f 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5870,9 +5870,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 +7298,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/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 From d93021579446df2d83d165314849b05fbec4b0ac Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Wed, 10 Jan 2024 12:23:25 -0500 Subject: [PATCH 2/3] Update service.Spec.LoadBalancerSourceRanges validation tests In preparation for rewriting LoadBalancerSourceRanges validation, add/update the existing unit tests to cover some of the more exciting edge cases of the existing validation code: - The values in service.Spec.LoadBalancerSourceRanges are allowed to have arbitrary whitespace around them. - The annotation must be unset for non-LoadBalancer services, but for LoadBalancer services, "set but empty" and "whitespace-only" are treated the same as "unset". - The annotation value is only validated if the field is not set. Also fix some of the existing tests to be more precise about what they are testing. Also fix the CIDR values to actually be valid. Sigh. --- pkg/apis/core/validation/validation_test.go | 68 ++++++++++++++++++--- 1 file changed, 60 insertions(+), 8 deletions(-) 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) { From 593b1c6c63ed3a3edd0a17922d84c0211f3f1f6e Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Wed, 10 Jan 2024 12:30:55 -0500 Subject: [PATCH 3/3] Do service.spec.LoadBalancerSourceRanges validation inline Inline the LoadBalancerSourceRanges parsing to make it more obvious what it's requiring (and more importantly, *not* requiring), and change it to use IsValidCIDR as well. --- pkg/apis/core/validation/validation.go | 38 ++++++++++++++++---------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 686d184781f..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)...) + } } }