From 8fc691be940e73dac324e92c3207ed03df74b1ca Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 23 Jan 2024 08:25:52 -0500 Subject: [PATCH 1/4] Drop validation.IsValidSocketAddr It's not used anywhere, and if someone was going to validate an IP:port somewhere, they should think about exactly what they want rather than just using this function. (E.g., validation should be slightly different for an IP:port to bind to vs an IP:port to connect to.) --- .../pkg/util/validation/validation.go | 17 ----------- .../pkg/util/validation/validation_test.go | 28 ------------------- 2 files changed, 45 deletions(-) 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 0b8a6cb354a..6f8a2e26bdb 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go @@ -19,9 +19,7 @@ package validation import ( "fmt" "math" - "net" "regexp" - "strconv" "strings" "k8s.io/apimachinery/pkg/util/validation/field" @@ -493,18 +491,3 @@ func hasChDirPrefix(value string) []string { } return errs } - -// IsValidSocketAddr checks that string represents a valid socket address -// as defined in RFC 789. (e.g 0.0.0.0:10254 or [::]:10254)) -func IsValidSocketAddr(value string) []string { - var errs []string - ip, port, err := net.SplitHostPort(value) - if err != nil { - errs = append(errs, "must be a valid socket address format, (e.g. 0.0.0.0:10254 or [::]:10254)") - return errs - } - portInt, _ := strconv.Atoi(port) - errs = append(errs, IsValidPortNum(portInt)...) - errs = append(errs, IsValidIP(ip)...) - return errs -} 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 499b13c737a..c439e0398c2 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 @@ -710,31 +710,3 @@ func TestIsDomainPrefixedPath(t *testing.T) { } } } - -func TestIsValidSocketAddr(t *testing.T) { - goodValues := []string{ - "0.0.0.0:10254", - "127.0.0.1:8888", - "[2001:db8:1f70::999:de8:7648:6e8]:10254", - "[::]:10254", - } - for _, val := range goodValues { - if errs := IsValidSocketAddr(val); len(errs) != 0 { - t.Errorf("expected no errors for %q: %v", val, errs) - } - } - - badValues := []string{ - "0.0.0.0.0:2020", - "0.0.0.0", - "6.6.6.6:909090", - "2001:db8:1f70::999:de8:7648:6e8:87567:102545", - "", - "*", - } - for _, val := range badValues { - if errs := IsValidSocketAddr(val); len(errs) == 0 { - t.Errorf("expected errors for %q", val) - } - } -} From f999b24fad907fa6f566e088bf417e9d7b217403 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Wed, 27 Dec 2023 09:44:45 -0500 Subject: [PATCH 2/4] Expand IsValidIP unit tests Add more test cases, and merge the IsValidIP, IsValidIPv4Address and IsValidIPv6Address tests together. (Any string that passes IsValidIP should pass either IsValidIPv4Address or IsValidIPv6Address but not both, and any string that fails IsValidIP should fail both IsValidIPv4Address and IsValidIPv6Address.) --- .../pkg/util/validation/validation_test.go | 258 +++++++++++------- 1 file changed, 165 insertions(+), 93 deletions(-) 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 c439e0398c2..aeda7bf5662 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 @@ -323,104 +323,176 @@ func TestIsValidLabelValue(t *testing.T) { } func TestIsValidIP(t *testing.T) { - goodValues := []string{ - "::1", - "2a00:79e0:2:0:f1c3:e797:93c1:df80", - "::", - "2001:4860:4860::8888", - "::fff:1.1.1.1", - "1.1.1.1", - "1.1.1.01", - "255.0.0.1", - "1.0.0.0", - "0.0.0.0", - } - for _, val := range goodValues { - if msgs := IsValidIP(val); len(msgs) != 0 { - t.Errorf("expected true for %q: %v", val, msgs) - } - } + for _, tc := range []struct { + name string + in string + family int + err string + }{ + // GOOD VALUES + { + name: "ipv4", + in: "1.2.3.4", + family: 4, + }, + { + name: "ipv4, all zeros", + in: "0.0.0.0", + family: 4, + }, + { + name: "ipv4, max", + in: "255.255.255.255", + family: 4, + }, + { + name: "ipv6", + in: "1234::abcd", + family: 6, + }, + { + name: "ipv6, all zeros, collapsed", + in: "::", + family: 6, + }, + { + name: "ipv6, max", + in: "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", + family: 6, + }, - badValues := []string{ - "[2001:db8:0:1]:80", - "myhost.mydomain", - "-1.0.0.0", - "[2001:db8:0:1]", - "a", - } - for _, val := range badValues { - if msgs := IsValidIP(val); len(msgs) == 0 { - t.Errorf("expected false for %q", val) - } - } -} + // GOOD, THOUGH NON-CANONICAL, VALUES + { + name: "ipv6, all zeros, expanded (non-canonical)", + in: "0:0:0:0:0:0:0:0", + family: 6, + }, + { + name: "ipv6, leading 0s (non-canonical)", + in: "0001:002:03:4::", + family: 6, + }, + { + name: "ipv6, capital letters (non-canonical)", + in: "1234::ABCD", + family: 6, + }, -func TestIsValidIPv4Address(t *testing.T) { - goodValues := []string{ - "1.1.1.1", - "1.1.1.01", - "255.0.0.1", - "1.0.0.0", - "0.0.0.0", - } - for _, val := range goodValues { - if msgs := IsValidIPv4Address(field.NewPath(""), val); len(msgs) != 0 { - t.Errorf("expected %q to be valid IPv4 address: %v", val, msgs) - } - } + // BAD VALUES WE CURRENTLY CONSIDER GOOD + { + name: "ipv4 with leading 0s", + in: "1.1.1.01", + family: 4, + }, + { + name: "ipv4-in-ipv6 value", + in: "::ffff:1.1.1.1", + family: 4, + }, - badValues := []string{ - "[2001:db8:0:1]:80", - "myhost.mydomain", - "-1.0.0.0", - "[2001:db8:0:1]", - "a", - "2001:4860:4860::8888", - "::fff:1.1.1.1", - "::1", - "2a00:79e0:2:0:f1c3:e797:93c1:df80", - "::", - } - for _, val := range badValues { - if msgs := IsValidIPv4Address(field.NewPath(""), val); len(msgs) == 0 { - t.Errorf("expected %q to be invalid IPv4 address", val) - } - } -} + // BAD VALUES + { + name: "empty string", + in: "", + err: "must be a valid IP address", + }, + { + name: "junk", + in: "aaaaaaa", + err: "must be a valid IP address", + }, + { + name: "domain name", + in: "myhost.mydomain", + err: "must be a valid IP address", + }, + { + name: "cidr", + in: "1.2.3.0/24", + err: "must be a valid IP address", + }, + { + name: "ipv4 with out-of-range octets", + in: "1.2.3.400", + err: "must be a valid IP address", + }, + { + name: "ipv4 with negative octets", + in: "-1.0.0.0", + err: "must be a valid IP address", + }, + { + name: "ipv6 with out-of-range segment", + in: "2001:db8::10005", + err: "must be a valid IP address", + }, + { + name: "ipv4:port", + in: "1.2.3.4:80", + err: "must be a valid IP address", + }, + { + name: "ipv6 with brackets", + in: "[2001:db8::1]", + err: "must be a valid IP address", + }, + { + name: "[ipv6]:port", + in: "[2001:db8::1]:80", + err: "must be a valid IP address", + }, + { + name: "host:port", + in: "example.com:80", + err: "must be a valid IP address", + }, + { + name: "ipv6 with zone", + in: "1234::abcd%eth0", + err: "must be a valid IP address", + }, + { + name: "ipv4 with zone", + in: "169.254.0.0%eth0", + err: "must be a valid IP address", + }, + } { + t.Run(tc.name, func(t *testing.T) { + msgs := IsValidIP(tc.in) + if tc.err == "" { + if len(msgs) != 0 { + t.Errorf("expected %q to be valid but got: %v", tc.in, msgs) + } + } else { + if len(msgs) != 1 { + t.Errorf("expected %q to have 1 error but got: %v", tc.in, msgs) + } else if !strings.Contains(msgs[0], tc.err) { + t.Errorf("expected error for %q to contain %q but got: %q", tc.in, tc.err, msgs[0]) + } + } -func TestIsValidIPv6Address(t *testing.T) { - goodValues := []string{ - "2001:4860:4860::8888", - "2a00:79e0:2:0:f1c3:e797:93c1:df80", - "2001:0db8:85a3:0000:0000:8a2e:0370:7334", - "::fff:1.1.1.1", - "::1", - "::", - } + errs := IsValidIPv4Address(field.NewPath(""), tc.in) + if tc.family == 4 { + if len(errs) != 0 { + t.Errorf("expected %q to pass IsValidIPv4Address but got: %v", tc.in, errs) + } + } else { + if len(errs) == 0 { + t.Errorf("expected %q to fail IsValidIPv4Address", tc.in) + } + } - for _, val := range goodValues { - if msgs := IsValidIPv6Address(field.NewPath(""), val); len(msgs) != 0 { - t.Errorf("expected %q to be valid IPv6 address: %v", val, msgs) - } - } - - badValues := []string{ - "1.1.1.1", - "1.1.1.01", - "255.0.0.1", - "1.0.0.0", - "0.0.0.0", - "[2001:db8:0:1]:80", - "myhost.mydomain", - "2001:0db8:85a3:0000:0000:8a2e:0370:7334:2001:0db8:85a3:0000:0000:8a2e:0370:7334", - "-1.0.0.0", - "[2001:db8:0:1]", - "a", - } - for _, val := range badValues { - if msgs := IsValidIPv6Address(field.NewPath(""), val); len(msgs) == 0 { - t.Errorf("expected %q to be invalid IPv6 address", val) - } + errs = IsValidIPv6Address(field.NewPath(""), tc.in) + if tc.family == 6 { + if len(errs) != 0 { + t.Errorf("expected %q to pass IsValidIPv6Address but got: %v", tc.in, errs) + } + } else { + if len(errs) == 0 { + t.Errorf("expected %q to fail IsValidIPv6Address", tc.in) + } + } + }) } } From 519dd6887dc275f77d5be32a1f711ff71117c87d Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 26 Dec 2023 21:11:15 -0500 Subject: [PATCH 3/4] Make validation.IsValidIP return a field.ErrorList for consistency --- pkg/apis/core/validation/validation.go | 24 ++++++------------- .../pkg/util/validation/validation.go | 7 +++--- .../pkg/util/validation/validation_test.go | 16 ++++++------- 3 files changed, 19 insertions(+), 28 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index a6f7fef3012..fde26bf97ee 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -4027,9 +4027,7 @@ func validatePodIPs(pod *core.Pod) field.ErrorList { // all PodIPs must be valid IPs for i, podIP := range pod.Status.PodIPs { - for _, msg := range validation.IsValidIP(podIP.IP) { - allErrs = append(allErrs, field.Invalid(podIPsField.Index(i), podIP.IP, msg)) - } + allErrs = append(allErrs, validation.IsValidIP(podIPsField.Index(i), podIP.IP)...) } // if we have more than one Pod.PodIP then @@ -4081,9 +4079,7 @@ func validateHostIPs(pod *core.Pod) field.ErrorList { // all HostPs must be valid IPs for i, hostIP := range pod.Status.HostIPs { - for _, msg := range validation.IsValidIP(hostIP.IP) { - allErrs = append(allErrs, field.Invalid(hostIPsField.Index(i), hostIP.IP, msg)) - } + allErrs = append(allErrs, validation.IsValidIP(hostIPsField.Index(i), hostIP.IP)...) } // if we have more than one Pod.HostIP then @@ -5399,10 +5395,8 @@ func ValidateService(service *core.Service) field.ErrorList { ipPath := specPath.Child("externalIPs") for i, ip := range service.Spec.ExternalIPs { idxPath := ipPath.Index(i) - if msgs := validation.IsValidIP(ip); len(msgs) != 0 { - for i := range msgs { - allErrs = append(allErrs, field.Invalid(idxPath, ip, msgs[i])) - } + if errs := validation.IsValidIP(idxPath, ip); len(errs) != 0 { + allErrs = append(allErrs, errs...) } else { allErrs = append(allErrs, ValidateNonSpecialIP(ip, idxPath)...) } @@ -6914,9 +6908,7 @@ func validateEndpointSubsets(subsets []core.EndpointSubset, fldPath *field.Path) func validateEndpointAddress(address *core.EndpointAddress, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - for _, msg := range validation.IsValidIP(address.IP) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("ip"), address.IP, msg)) - } + allErrs = append(allErrs, validation.IsValidIP(fldPath.Child("ip"), address.IP)...) if len(address.Hostname) > 0 { allErrs = append(allErrs, ValidateDNS1123Label(address.Hostname, fldPath.Child("hostname"))...) } @@ -7618,11 +7610,9 @@ func ValidateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorLi } // is it valid ip? - errorMessages := validation.IsValidIP(clusterIP) + errorMessages := validation.IsValidIP(clusterIPsField.Index(i), clusterIP) hasInvalidIPs = (len(errorMessages) != 0) || hasInvalidIPs - for _, msg := range errorMessages { - allErrs = append(allErrs, field.Invalid(clusterIPsField.Index(i), clusterIP, msg)) - } + allErrs = append(allErrs, errorMessages...) } // max two 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 6f8a2e26bdb..0eb33e68aaf 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go @@ -350,11 +350,12 @@ func IsValidPortName(port string) []string { } // IsValidIP tests that the argument is a valid IP address. -func IsValidIP(value string) []string { +func IsValidIP(fldPath *field.Path, value string) field.ErrorList { + var allErrors field.ErrorList if netutils.ParseIPSloppy(value) == nil { - return []string{"must be a valid IP address, (e.g. 10.9.8.7 or 2001:db8::ffff)"} + allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid IP address, (e.g. 10.9.8.7 or 2001:db8::ffff)")) } - return nil + return allErrors } // IsValidIPv4Address tests that the argument is a valid IPv4 address. 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 aeda7bf5662..2ce11d560f7 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 @@ -458,20 +458,20 @@ func TestIsValidIP(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - msgs := IsValidIP(tc.in) + errs := IsValidIP(field.NewPath(""), tc.in) if tc.err == "" { - if len(msgs) != 0 { - t.Errorf("expected %q to be valid but got: %v", tc.in, msgs) + if len(errs) != 0 { + t.Errorf("expected %q to be valid but got: %v", tc.in, errs) } } else { - if len(msgs) != 1 { - t.Errorf("expected %q to have 1 error but got: %v", tc.in, msgs) - } else if !strings.Contains(msgs[0], tc.err) { - t.Errorf("expected error for %q to contain %q but got: %q", tc.in, tc.err, msgs[0]) + 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) } } - errs := IsValidIPv4Address(field.NewPath(""), tc.in) + errs = IsValidIPv4Address(field.NewPath(""), tc.in) if tc.family == 4 { if len(errs) != 0 { t.Errorf("expected %q to pass IsValidIPv4Address but got: %v", tc.in, errs) From 1d59d6b6c657f71e740a2ba5d936f4e8985e2781 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 26 Dec 2023 19:26:24 -0500 Subject: [PATCH 4/4] Use validation.IsValidIP in a few more places Rather than using netutils.ParseIPSloppy directly. Also fix the field paths in the errors for pod.spec.hostAliases to include the array index. --- pkg/apis/core/validation/validation.go | 18 ++++++------------ pkg/apis/networking/validation/validation.go | 4 +--- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index fde26bf97ee..ceef3291b06 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3718,9 +3718,7 @@ func validatePodDNSConfig(dnsConfig *core.PodDNSConfig, dnsPolicy *core.DNSPolic allErrs = append(allErrs, field.Invalid(fldPath.Child("nameservers"), dnsConfig.Nameservers, fmt.Sprintf("must not have more than %v nameservers", MaxDNSNameservers))) } for i, ns := range dnsConfig.Nameservers { - if ip := netutils.ParseIPSloppy(ns); ip == nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("nameservers").Index(i), ns, "must be valid IP address")) - } + allErrs = append(allErrs, validation.IsValidIP(fldPath.Child("nameservers").Index(i), ns)...) } // Validate searches. if len(dnsConfig.Searches) > MaxDNSSearchPaths { @@ -3887,12 +3885,10 @@ func validateOnlyDeletedSchedulingGates(newGates, oldGates []core.PodSchedulingG func ValidateHostAliases(hostAliases []core.HostAlias, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - for _, hostAlias := range hostAliases { - if ip := netutils.ParseIPSloppy(hostAlias.IP); ip == nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("ip"), hostAlias.IP, "must be valid IP address")) - } - for _, hostname := range hostAlias.Hostnames { - allErrs = append(allErrs, ValidateDNS1123Subdomain(hostname, fldPath.Child("hostnames"))...) + for i, hostAlias := range hostAliases { + allErrs = append(allErrs, validation.IsValidIP(fldPath.Index(i).Child("ip"), hostAlias.IP)...) + for j, hostname := range hostAlias.Hostnames { + allErrs = append(allErrs, ValidateDNS1123Subdomain(hostname, fldPath.Index(i).Child("hostnames").Index(j))...) } } return allErrs @@ -7254,9 +7250,7 @@ func ValidateLoadBalancerStatus(status *core.LoadBalancerStatus, fldPath *field. for i, ingress := range status.Ingress { idxPath := ingrPath.Index(i) if len(ingress.IP) > 0 { - if isIP := (netutils.ParseIPSloppy(ingress.IP) != nil); !isIP { - allErrs = append(allErrs, field.Invalid(idxPath.Child("ip"), ingress.IP, "must be a valid IP address")) - } + allErrs = append(allErrs, validation.IsValidIP(idxPath.Child("ip"), ingress.IP)...) } if utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) && ingress.IPMode == nil { diff --git a/pkg/apis/networking/validation/validation.go b/pkg/apis/networking/validation/validation.go index 1f7df28cee7..03470f98cc4 100644 --- a/pkg/apis/networking/validation/validation.go +++ b/pkg/apis/networking/validation/validation.go @@ -354,9 +354,7 @@ func ValidateIngressLoadBalancerStatus(status *networking.IngressLoadBalancerSta for i, ingress := range status.Ingress { idxPath := fldPath.Child("ingress").Index(i) if len(ingress.IP) > 0 { - if isIP := (netutils.ParseIPSloppy(ingress.IP) != nil); !isIP { - allErrs = append(allErrs, field.Invalid(idxPath.Child("ip"), ingress.IP, "must be a valid IP address")) - } + allErrs = append(allErrs, validation.IsValidIP(idxPath.Child("ip"), ingress.IP)...) } if len(ingress.Hostname) > 0 { for _, msg := range validation.IsDNS1123Subdomain(ingress.Hostname) {