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.

Author: danwinship

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)...)
 			}

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,

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

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)
-				}
-			}
 		})
 	}
 }