From 76f1684117ad57fd61646af1eef8b9a52f2b5833 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 16 Feb 2024 10:07:39 -0500 Subject: [PATCH] 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. --- pkg/apis/core/validation/validation.go | 26 ++++++++++++--------- pkg/apis/core/validation/validation_test.go | 22 ++++++++++------- pkg/apis/discovery/validation/validation.go | 4 ++-- 3 files changed, 31 insertions(+), 21 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 2d62ea13b55..3069d74467a 100644 --- a/pkg/apis/core/validation/validation.go +++ b/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 { diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index aa3c6d2061f..10b1795267c 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/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) } }) } diff --git a/pkg/apis/discovery/validation/validation.go b/pkg/apis/discovery/validation/validation.go index a514ef77d8a..5df21d41b35 100644 --- a/pkg/apis/discovery/validation/validation.go +++ b/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)...) }