Rename ValidateNonSpecialIP to ValidateEndpointIP

There is not a single definition of "non-special IP" that makes sense
in all contexts. Rename ValidateNonSpecialIP to ValidateEndpointIP and
clarify that it shouldn't be used for other validations.

Also add a few more unit tests.

Author: danwinship

pkg/apis/core/validation/validation.go

@@ -5943,7 +5943,9 @@ func ValidateService(service *core.Service) field.ErrorList {
 		if errs := validation.IsValidIP(idxPath, ip); len(errs) != 0 {
 			allErrs = append(allErrs, errs...)
 		} else {
-			allErrs = append(allErrs, ValidateNonSpecialIP(ip, idxPath)...)
+			// For historical reasons, this uses ValidateEndpointIP even
+			// though that is not exactly the appropriate set of checks.
+			allErrs = append(allErrs, ValidateEndpointIP(ip, idxPath)...)
 		}
 	}
 
@@ -7489,19 +7491,21 @@ func validateEndpointAddress(address *core.EndpointAddress, fldPath *field.Path)
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("nodeName"), *address.NodeName, msg).WithOrigin("format=dns-label"))
 		}
 	}
-	allErrs = append(allErrs, ValidateNonSpecialIP(address.IP, fldPath.Child("ip"))...)
+	allErrs = append(allErrs, ValidateEndpointIP(address.IP, fldPath.Child("ip"))...)
 	return allErrs
 }
 
-// ValidateNonSpecialIP is used to validate Endpoints, EndpointSlices, and
-// external IPs. Specifically, this disallows unspecified and loopback addresses
-// are nonsensical and link-local addresses tend to be used for node-centric
-// purposes (e.g. metadata service).
+// ValidateEndpointIP is used to validate Endpoints and EndpointSlice addresses, and also
+// (for historical reasons) external IPs. It disallows certain address types that don't
+// make sense in those contexts. Note that this function is _almost_, but not exactly,
+// equivalent to net.IP.IsGlobalUnicast(). (Unlike IsGlobalUnicast, it allows global
+// multicast IPs, which is probably a bug.)
 //
-// IPv6 references
-// - https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml
-// - https://www.iana.org/assignments/ipv6-multicast-addresses/ipv6-multicast-addresses.xhtml
-func ValidateNonSpecialIP(ipAddress string, fldPath *field.Path) field.ErrorList {
+// This function should not be used for new validations; the exact set of IPs that do and
+// don't make sense in a particular field is context-dependent (e.g., localhost makes
+// sense in some places; unspecified IPs make sense in fields that are used as bind
+// addresses rather than destination addresses).
+func ValidateEndpointIP(ipAddress string, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 	ip := netutils.ParseIPSloppy(ipAddress)
 	if ip == nil {
@@ -7520,7 +7524,7 @@ func ValidateNonSpecialIP(ipAddress string, fldPath *field.Path) field.ErrorList
 	if ip.IsLinkLocalMulticast() {
 		allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, "may not be in the link-local multicast range (224.0.0.0/24, ff02::/10)"))
 	}
-	return allErrs.WithOrigin("format=non-special-ip")
+	return allErrs.WithOrigin("format=endpoint-ip")
 }
 
 func validateEndpointPort(port *core.EndpointPort, requireName bool, fldPath *field.Path) field.ErrorList {

pkg/apis/core/validation/validation_test.go

@@ -20725,7 +20725,7 @@ func TestValidateEndpointsCreate(t *testing.T) {
 				}},
 			},
 			expectedErrs: field.ErrorList{
-				field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=non-special-ip"),
+				field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=endpoint-ip"),
 			},
 		},
 		"Address is link-local": {
@@ -20737,7 +20737,7 @@ func TestValidateEndpointsCreate(t *testing.T) {
 				}},
 			},
 			expectedErrs: field.ErrorList{
-				field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=non-special-ip"),
+				field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=endpoint-ip"),
 			},
 		},
 		"Address is link-local multicast": {
@@ -20749,7 +20749,7 @@ func TestValidateEndpointsCreate(t *testing.T) {
 				}},
 			},
 			expectedErrs: field.ErrorList{
-				field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=non-special-ip"),
+				field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=endpoint-ip"),
 			},
 		},
 		"Invalid AppProtocol": {
@@ -23641,7 +23641,7 @@ func TestValidateResourceRequirements(t *testing.T) {
 	}
 }
 
-func TestValidateNonSpecialIP(t *testing.T) {
+func TestValidateEndpointIP(t *testing.T) {
 	fp := field.NewPath("ip")
 
 	// Valid values.
@@ -23652,11 +23652,15 @@ func TestValidateNonSpecialIP(t *testing.T) {
 		{"ipv4", "10.1.2.3"},
 		{"ipv4 class E", "244.1.2.3"},
 		{"ipv6", "2000::1"},
+
+		// These probably should not have been allowed, but they are
+		{"ipv4 multicast", "239.255.255.253"},
+		{"ipv6 multicast", "ff05::1:3"},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			errs := ValidateNonSpecialIP(tc.ip, fp)
+			errs := ValidateEndpointIP(tc.ip, fp)
 			if len(errs) != 0 {
-				t.Errorf("ValidateNonSpecialIP(%q, ...) = %v; want nil", tc.ip, errs)
+				t.Errorf("ValidateEndpointIP(%q, ...) = %v; want nil", tc.ip, errs)
 			}
 		})
 	}
@@ -23670,13 +23674,15 @@ func TestValidateNonSpecialIP(t *testing.T) {
 		{"ipv4 localhost", "127.0.0.0"},
 		{"ipv4 localhost", "127.255.255.255"},
 		{"ipv6 localhost", "::1"},
+		{"ipv4 link local", "169.254.169.254"},
 		{"ipv6 link local", "fe80::"},
+		{"ipv4 local multicast", "224.0.0.251"},
 		{"ipv6 local multicast", "ff02::"},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			errs := ValidateNonSpecialIP(tc.ip, fp)
+			errs := ValidateEndpointIP(tc.ip, fp)
 			if len(errs) == 0 {
-				t.Errorf("ValidateNonSpecialIP(%q, ...) = nil; want non-nil (errors)", tc.ip)
+				t.Errorf("ValidateEndpointIP(%q, ...) = nil; want non-nil (errors)", tc.ip)
 			}
 		})
 	}

pkg/apis/discovery/validation/validation.go

@@ -100,10 +100,10 @@ func validateEndpoints(endpoints []discovery.Endpoint, addrType discovery.Addres
 			switch addrType {
 			case discovery.AddressTypeIPv4:
 				allErrs = append(allErrs, validation.IsValidIPv4Address(addressPath.Index(i), address)...)
-				allErrs = append(allErrs, apivalidation.ValidateNonSpecialIP(address, addressPath.Index(i))...)
+				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.ValidateNonSpecialIP(address, addressPath.Index(i))...)
+				allErrs = append(allErrs, apivalidation.ValidateEndpointIP(address, addressPath.Index(i))...)
 			case discovery.AddressTypeFQDN:
 				allErrs = append(allErrs, validation.IsFullyQualifiedDomainName(addressPath.Index(i), address)...)
 			}