From ba189de78ff13f6882e753138a5df07d6ab49433 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 28 Feb 2025 18:14:44 -0500 Subject: [PATCH] Slightly improve EndpointSlice address validation Because it used both IsValidIPv4Address and ValidateEndpointIP, EndpointSlice validation produced duplicate error messages when given an invalid IP. Fix this by calling IsValidIP first, and only doing the other checks if that one fails. Also, since no one else was using the IsValidIPv4Address and IsValidIPv6Address methods anyway, just inline them into the EndpointSlice validation, so we don't have to worry about "should they do legacy or strict validation" later. --- pkg/apis/discovery/validation/validation.go | 23 ++++- .../discovery/validation/validation_test.go | 6 +- .../apimachinery/pkg/util/validation/ip.go | 20 ----- .../pkg/util/validation/ip_test.go | 84 ++++++------------- 4 files changed, 47 insertions(+), 86 deletions(-) diff --git a/pkg/apis/discovery/validation/validation.go b/pkg/apis/discovery/validation/validation.go index 5df21d41b35..706c1194dcd 100644 --- a/pkg/apis/discovery/validation/validation.go +++ b/pkg/apis/discovery/validation/validation.go @@ -28,6 +28,7 @@ import ( api "k8s.io/kubernetes/pkg/apis/core" apivalidation "k8s.io/kubernetes/pkg/apis/core/validation" "k8s.io/kubernetes/pkg/apis/discovery" + netutils "k8s.io/utils/net" ) var ( @@ -99,11 +100,25 @@ func validateEndpoints(endpoints []discovery.Endpoint, addrType discovery.Addres // and do not get validated. switch addrType { case discovery.AddressTypeIPv4: - allErrs = append(allErrs, validation.IsValidIPv4Address(addressPath.Index(i), address)...) - allErrs = append(allErrs, apivalidation.ValidateEndpointIP(address, addressPath.Index(i))...) + ipErrs := validation.IsValidIP(addressPath.Index(i), address) + if len(ipErrs) > 0 { + allErrs = append(allErrs, ipErrs...) + } else { + if !netutils.IsIPv4String(address) { + allErrs = append(allErrs, field.Invalid(addressPath, address, "must be an IPv4 address")) + } + allErrs = append(allErrs, apivalidation.ValidateEndpointIP(address, addressPath.Index(i))...) + } case discovery.AddressTypeIPv6: - allErrs = append(allErrs, validation.IsValidIPv6Address(addressPath.Index(i), address)...) - allErrs = append(allErrs, apivalidation.ValidateEndpointIP(address, addressPath.Index(i))...) + ipErrs := validation.IsValidIP(addressPath.Index(i), address) + if len(ipErrs) > 0 { + allErrs = append(allErrs, ipErrs...) + } else { + if !netutils.IsIPv6String(address) { + allErrs = append(allErrs, field.Invalid(addressPath, address, "must be an IPv6 address")) + } + allErrs = append(allErrs, apivalidation.ValidateEndpointIP(address, addressPath.Index(i))...) + } case discovery.AddressTypeFQDN: allErrs = append(allErrs, validation.IsFullyQualifiedDomainName(addressPath.Index(i), address)...) } diff --git a/pkg/apis/discovery/validation/validation_test.go b/pkg/apis/discovery/validation/validation_test.go index 3e6c4cd1d38..16366de6e57 100644 --- a/pkg/apis/discovery/validation/validation_test.go +++ b/pkg/apis/discovery/validation/validation_test.go @@ -408,7 +408,7 @@ func TestValidateEndpointSlice(t *testing.T) { }, }, "bad-ip": { - expectedErrors: 2, + expectedErrors: 1, endpointSlice: &discovery.EndpointSlice{ ObjectMeta: standardMeta, AddressType: discovery.AddressTypeIPv4, @@ -423,7 +423,7 @@ func TestValidateEndpointSlice(t *testing.T) { }, }, "bad-ipv4": { - expectedErrors: 3, + expectedErrors: 2, endpointSlice: &discovery.EndpointSlice{ ObjectMeta: standardMeta, AddressType: discovery.AddressTypeIPv4, @@ -438,7 +438,7 @@ func TestValidateEndpointSlice(t *testing.T) { }, }, "bad-ipv6": { - expectedErrors: 4, + expectedErrors: 2, endpointSlice: &discovery.EndpointSlice{ ObjectMeta: standardMeta, AddressType: discovery.AddressTypeIPv6, diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go index ae9497ff87d..bed741b074a 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go @@ -65,26 +65,6 @@ func GetWarningsForIP(fldPath *field.Path, value string) []string { return nil } -// IsValidIPv4Address tests that the argument is a valid IPv4 address. -func IsValidIPv4Address(fldPath *field.Path, value string) field.ErrorList { - var allErrors field.ErrorList - ip := netutils.ParseIPSloppy(value) - if ip == nil || ip.To4() == nil { - allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid IPv4 address")) - } - return allErrors -} - -// IsValidIPv6Address tests that the argument is a valid IPv6 address. -func IsValidIPv6Address(fldPath *field.Path, value string) field.ErrorList { - var allErrors field.ErrorList - ip := netutils.ParseIPSloppy(value) - if ip == nil || ip.To4() != nil { - allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid IPv6 address")) - } - 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 diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip_test.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip_test.go index 9ff4f9ca0f9..8c33a29134d 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip_test.go @@ -26,70 +26,58 @@ import ( func TestIsValidIP(t *testing.T) { for _, tc := range []struct { - name string - in string - family int - err string + name string + in string + err string }{ // GOOD VALUES { - name: "ipv4", - in: "1.2.3.4", - family: 4, + name: "ipv4", + in: "1.2.3.4", }, { - name: "ipv4, all zeros", - in: "0.0.0.0", - family: 4, + name: "ipv4, all zeros", + in: "0.0.0.0", }, { - name: "ipv4, max", - in: "255.255.255.255", - family: 4, + name: "ipv4, max", + in: "255.255.255.255", }, { - name: "ipv6", - in: "1234::abcd", - family: 6, + name: "ipv6", + in: "1234::abcd", }, { - name: "ipv6, all zeros, collapsed", - in: "::", - family: 6, + name: "ipv6, all zeros, collapsed", + in: "::", }, { - name: "ipv6, max", - in: "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", - family: 6, + name: "ipv6, max", + in: "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", }, // 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, all zeros, expanded (non-canonical)", + in: "0:0:0:0:0:0:0:0", }, { - name: "ipv6, leading 0s (non-canonical)", - in: "0001:002:03:4::", - family: 6, + name: "ipv6, leading 0s (non-canonical)", + in: "0001:002:03:4::", }, { - name: "ipv6, capital letters (non-canonical)", - in: "1234::ABCD", - family: 6, + name: "ipv6, capital letters (non-canonical)", + in: "1234::ABCD", }, // BAD VALUES WE CURRENTLY CONSIDER GOOD { - name: "ipv4 with leading 0s", - in: "1.1.1.01", - family: 4, + name: "ipv4 with leading 0s", + in: "1.1.1.01", }, { - name: "ipv4-in-ipv6 value", - in: "::ffff:1.1.1.1", - family: 4, + name: "ipv4-in-ipv6 value", + in: "::ffff:1.1.1.1", }, // BAD VALUES @@ -172,28 +160,6 @@ func TestIsValidIP(t *testing.T) { 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) - 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) - } - } - - 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) - } - } }) } }