From 7a56b6e3f7ae2896a05c869be1e24a0885ccffce Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Wed, 27 Dec 2023 09:44:45 -0500 Subject: [PATCH] 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