From 76f1684117ad57fd61646af1eef8b9a52f2b5833 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 16 Feb 2024 10:07:39 -0500 Subject: [PATCH 1/6] 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)...) } From f79bccf4d99b16c855f70af44f1f2e7fdc817bc5 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 27 Feb 2025 09:32:52 -0500 Subject: [PATCH 2/6] validation unit test cleanups Fix some incorrect test case names. Use t.Run() in a few more places (to facilitate using SetFeatureGateDuringTest later). Clarify TestPodIPsValidation/TestHostIPsValidation (and fix weird indentation). --- pkg/apis/core/validation/validation_test.go | 151 ++++++++++-------- .../networking/validation/validation_test.go | 62 ++++--- 2 files changed, 119 insertions(+), 94 deletions(-) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 10b1795267c..eca9b3c5e08 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -9951,16 +9951,18 @@ func TestValidatePodDNSConfig(t *testing.T) { } for _, tc := range testCases { - if tc.dnsPolicy == nil { - tc.dnsPolicy = &testDNSClusterFirst - } + t.Run("", func(t *testing.T) { + if tc.dnsPolicy == nil { + tc.dnsPolicy = &testDNSClusterFirst + } - errs := validatePodDNSConfig(tc.dnsConfig, tc.dnsPolicy, field.NewPath("dnsConfig"), tc.opts) - if len(errs) != 0 && !tc.expectedError { - t.Errorf("%v: validatePodDNSConfig(%v) = %v, want nil", tc.desc, tc.dnsConfig, errs) - } else if len(errs) == 0 && tc.expectedError { - t.Errorf("%v: validatePodDNSConfig(%v) = nil, want error", tc.desc, tc.dnsConfig) - } + errs := validatePodDNSConfig(tc.dnsConfig, tc.dnsPolicy, field.NewPath("dnsConfig"), tc.opts) + if len(errs) != 0 && !tc.expectedError { + t.Errorf("%v: validatePodDNSConfig(%v) = %v, want nil", tc.desc, tc.dnsConfig, errs) + } else if len(errs) == 0 && tc.expectedError { + t.Errorf("%v: validatePodDNSConfig(%v) = nil, want error", tc.desc, tc.dnsConfig) + } + }) } } @@ -10304,12 +10306,14 @@ func TestValidatePodSpec(t *testing.T) { ), } for k, v := range failureCases { - opts := PodValidationOptions{ - ResourceIsPod: true, - } - if errs := ValidatePodSpec(&v.Spec, nil, field.NewPath("field"), opts); len(errs) == 0 { - t.Errorf("expected failure for %q", k) - } + t.Run(k, func(t *testing.T) { + opts := PodValidationOptions{ + ResourceIsPod: true, + } + if errs := ValidatePodSpec(&v.Spec, nil, field.NewPath("field"), opts); len(errs) == 0 { + t.Errorf("expected failure") + } + }) } } @@ -15302,35 +15306,35 @@ func TestValidateServiceCreate(t *testing.T) { // numErrs: 1, numErrs: 0, }, { - name: "invalid publicIPs localhost", + name: "invalid externalIPs localhost", tweakSvc: func(s *core.Service) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"127.0.0.1"} }, numErrs: 1, }, { - name: "invalid publicIPs unspecified", + name: "invalid externalIPs unspecified", tweakSvc: func(s *core.Service) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"0.0.0.0"} }, numErrs: 1, }, { - name: "invalid publicIPs loopback", + name: "invalid externalIPs loopback", tweakSvc: func(s *core.Service) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"127.0.0.1"} }, numErrs: 1, }, { - name: "invalid publicIPs host", + name: "invalid externalIPs host", tweakSvc: func(s *core.Service) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"myhost.mydomain"} }, numErrs: 1, }, { - name: "valid publicIPs", + name: "valid externalIPs", tweakSvc: func(s *core.Service) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"1.2.3.4"} @@ -16989,9 +16993,11 @@ func TestValidateNode(t *testing.T) { }, } for _, successCase := range successCases { - if errs := ValidateNode(&successCase); len(errs) != 0 { - t.Errorf("expected success: %v", errs) - } + t.Run("", func(t *testing.T) { + if errs := ValidateNode(&successCase); len(errs) != 0 { + t.Errorf("expected success: %v", errs) + } + }) } errorCases := map[string]core.Node{ @@ -17195,30 +17201,32 @@ func TestValidateNode(t *testing.T) { }, } for k, v := range errorCases { - errs := ValidateNode(&v) - if len(errs) == 0 { - t.Errorf("expected failure for %s", k) - } - for i := range errs { - field := errs[i].Field - expectedFields := map[string]bool{ - "metadata.name": true, - "metadata.labels": true, - "metadata.annotations": true, - "metadata.namespace": true, - "spec.externalID": true, - "spec.taints[0].key": true, - "spec.taints[0].value": true, - "spec.taints[0].effect": true, - "metadata.annotations.scheduler.alpha.kubernetes.io/preferAvoidPods[0].PodSignature": true, - "metadata.annotations.scheduler.alpha.kubernetes.io/preferAvoidPods[0].PodSignature.PodController.Controller": true, + t.Run(k, func(t *testing.T) { + errs := ValidateNode(&v) + if len(errs) == 0 { + t.Errorf("expected failure") } - if val, ok := expectedFields[field]; ok { - if !val { - t.Errorf("%s: missing prefix for: %v", k, errs[i]) + for i := range errs { + field := errs[i].Field + expectedFields := map[string]bool{ + "metadata.name": true, + "metadata.labels": true, + "metadata.annotations": true, + "metadata.namespace": true, + "spec.externalID": true, + "spec.taints[0].key": true, + "spec.taints[0].value": true, + "spec.taints[0].effect": true, + "metadata.annotations.scheduler.alpha.kubernetes.io/preferAvoidPods[0].PodSignature": true, + "metadata.annotations.scheduler.alpha.kubernetes.io/preferAvoidPods[0].PodSignature.PodController.Controller": true, + } + if val, ok := expectedFields[field]; ok { + if !val { + t.Errorf("missing prefix for: %v", errs[i]) + } } } - } + }) } } @@ -22825,28 +22833,32 @@ func makePod(podName string, podNamespace string, podIPs []core.PodIP) core.Pod } } func TestPodIPsValidation(t *testing.T) { + // We test updating every pod in testCases to every other pod in testCases. + // expectError is true if we expect an error when updating *to* that pod. + testCases := []struct { pod core.Pod expectError bool - }{{ - expectError: false, - pod: makePod("nil-ips", "ns", nil), - }, { - expectError: false, - pod: makePod("empty-podips-list", "ns", []core.PodIP{}), - }, { - expectError: false, - pod: makePod("single-ip-family-6", "ns", []core.PodIP{{IP: "::1"}}), - }, { - expectError: false, - pod: makePod("single-ip-family-4", "ns", []core.PodIP{{IP: "1.1.1.1"}}), - }, { - expectError: false, - pod: makePod("dual-stack-4-6", "ns", []core.PodIP{{IP: "1.1.1.1"}, {IP: "::1"}}), - }, { - expectError: false, - pod: makePod("dual-stack-6-4", "ns", []core.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), - }, + }{ + { + expectError: false, + pod: makePod("nil-ips", "ns", nil), + }, { + expectError: false, + pod: makePod("empty-podips-list", "ns", []core.PodIP{}), + }, { + expectError: false, + pod: makePod("single-ip-family-6", "ns", []core.PodIP{{IP: "::1"}}), + }, { + expectError: false, + pod: makePod("single-ip-family-4", "ns", []core.PodIP{{IP: "1.1.1.1"}}), + }, { + expectError: false, + pod: makePod("dual-stack-4-6", "ns", []core.PodIP{{IP: "1.1.1.1"}, {IP: "::1"}}), + }, { + expectError: false, + pod: makePod("dual-stack-6-4", "ns", []core.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), + }, /* failure cases start here */ { expectError: true, @@ -22887,10 +22899,10 @@ func TestPodIPsValidation(t *testing.T) { errs := ValidatePodStatusUpdate(newPod, oldPod, PodValidationOptions{}) if len(errs) == 0 && testCase.expectError { - t.Fatalf("expected failure for %s, but there were none", testCase.pod.Name) + t.Fatalf("expected failure updating from %s, but there were none", oldTestCase.pod.Name) } if len(errs) != 0 && !testCase.expectError { - t.Fatalf("expected success for %s, but there were errors: %v", testCase.pod.Name, errs) + t.Fatalf("expected success updating from %s, but there were errors: %v", oldTestCase.pod.Name, errs) } } }) @@ -22921,6 +22933,9 @@ func makePodWithHostIPs(podName string, podNamespace string, hostIPs []core.Host } func TestHostIPsValidation(t *testing.T) { + // We test updating every pod in testCases to every other pod in testCases. + // expectError is true if we expect an error when updating *to* that pod. + testCases := []struct { pod core.Pod expectError bool @@ -22952,7 +22967,7 @@ func TestHostIPsValidation(t *testing.T) { /* failure cases start here */ { expectError: true, - pod: makePodWithHostIPs("invalid-pod-ip", "ns", []core.HostIP{{IP: "this-is-not-an-ip"}}), + pod: makePodWithHostIPs("invalid-host-ip", "ns", []core.HostIP{{IP: "this-is-not-an-ip"}}), }, { expectError: true, @@ -22994,10 +23009,10 @@ func TestHostIPsValidation(t *testing.T) { errs := ValidatePodStatusUpdate(newPod, oldPod, PodValidationOptions{}) if len(errs) == 0 && testCase.expectError { - t.Fatalf("expected failure for %s, but there were none", testCase.pod.Name) + t.Fatalf("expected failure updating from %s, but there were none", oldTestCase.pod.Name) } if len(errs) != 0 && !testCase.expectError { - t.Fatalf("expected success for %s, but there were errors: %v", testCase.pod.Name, errs) + t.Fatalf("expected success updating from %s, but there were errors: %v", oldTestCase.pod.Name, errs) } } }) diff --git a/pkg/apis/networking/validation/validation_test.go b/pkg/apis/networking/validation/validation_test.go index 7b2ad6f0515..daf22967bf4 100644 --- a/pkg/apis/networking/validation/validation_test.go +++ b/pkg/apis/networking/validation/validation_test.go @@ -249,10 +249,12 @@ func TestValidateNetworkPolicy(t *testing.T) { // Success cases are expected to pass validation. - for k, v := range successCases { - if errs := ValidateNetworkPolicy(v, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) != 0 { - t.Errorf("Expected success for the success validation test number %d, got %v", k, errs) - } + for _, v := range successCases { + t.Run("", func(t *testing.T) { + if errs := ValidateNetworkPolicy(v, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) != 0 { + t.Errorf("Expected success, got %v", errs) + } + }) } invalidSelector := map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "b"} @@ -367,9 +369,11 @@ func TestValidateNetworkPolicy(t *testing.T) { // Error cases are not expected to pass validation. for testName, networkPolicy := range errorCases { - if errs := ValidateNetworkPolicy(networkPolicy, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) == 0 { - t.Errorf("Expected failure for test: %s", testName) - } + t.Run(testName, func(t *testing.T) { + if errs := ValidateNetworkPolicy(networkPolicy, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) == 0 { + t.Errorf("Expected failure") + } + }) } } @@ -420,11 +424,13 @@ func TestValidateNetworkPolicyUpdate(t *testing.T) { } for testName, successCase := range successCases { - successCase.old.ObjectMeta.ResourceVersion = "1" - successCase.update.ObjectMeta.ResourceVersion = "1" - if errs := ValidateNetworkPolicyUpdate(&successCase.update, &successCase.old, NetworkPolicyValidationOptions{false}); len(errs) != 0 { - t.Errorf("expected success (%s): %v", testName, errs) - } + t.Run(testName, func(t *testing.T) { + successCase.old.ObjectMeta.ResourceVersion = "1" + successCase.update.ObjectMeta.ResourceVersion = "1" + if errs := ValidateNetworkPolicyUpdate(&successCase.update, &successCase.old, NetworkPolicyValidationOptions{false}); len(errs) != 0 { + t.Errorf("expected success, but got %v", errs) + } + }) } errorCases := map[string]npUpdateTest{ @@ -447,11 +453,13 @@ func TestValidateNetworkPolicyUpdate(t *testing.T) { } for testName, errorCase := range errorCases { - errorCase.old.ObjectMeta.ResourceVersion = "1" - errorCase.update.ObjectMeta.ResourceVersion = "1" - if errs := ValidateNetworkPolicyUpdate(&errorCase.update, &errorCase.old, NetworkPolicyValidationOptions{false}); len(errs) == 0 { - t.Errorf("expected failure: %s", testName) - } + t.Run(testName, func(t *testing.T) { + errorCase.old.ObjectMeta.ResourceVersion = "1" + errorCase.update.ObjectMeta.ResourceVersion = "1" + if errs := ValidateNetworkPolicyUpdate(&errorCase.update, &errorCase.old, NetworkPolicyValidationOptions{false}); len(errs) == 0 { + t.Errorf("expected failure") + } + }) } } @@ -1824,16 +1832,18 @@ func TestValidateIngressStatusUpdate(t *testing.T) { "status.loadBalancer.ingress[0].hostname: Invalid value": invalidHostname, } for k, v := range errorCases { - errs := ValidateIngressStatusUpdate(&v, &oldValue) - if len(errs) == 0 { - t.Errorf("expected failure for %s", k) - } else { - s := strings.Split(k, ":") - err := errs[0] - if err.Field != s[0] || !strings.Contains(err.Error(), s[1]) { - t.Errorf("unexpected error: %q, expected: %q", err, k) + t.Run(k, func(t *testing.T) { + errs := ValidateIngressStatusUpdate(&v, &oldValue) + if len(errs) == 0 { + t.Errorf("expected failure") + } else { + s := strings.Split(k, ":") + err := errs[0] + if err.Field != s[0] || !strings.Contains(err.Error(), s[1]) { + t.Errorf("unexpected error: %q, expected: %q", err, k) + } } - } + }) } } From fc4bb4fdb906d3120f048ae7ee422a130bf05e94 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 28 Feb 2025 16:16:51 -0500 Subject: [PATCH 3/6] Add validation.IsValidInterfaceAddress Split "ifaddr"-style ("192.168.1.5/24") validation out of IsValidCIDR. Since there is currently only one field that uses this format, and it already requires canonical form, IsValidInterfaceAddress requires canonical form unconditionally. --- pkg/apis/resource/validation/validation.go | 23 +--- .../validation_resourceclaim_test.go | 8 +- .../apimachinery/pkg/util/validation/ip.go | 28 +++++ .../pkg/util/validation/ip_test.go | 103 ++++++++++++++++++ 4 files changed, 137 insertions(+), 25 deletions(-) diff --git a/pkg/apis/resource/validation/validation.go b/pkg/apis/resource/validation/validation.go index 5853e79acef..0a1b1ce429f 100644 --- a/pkg/apis/resource/validation/validation.go +++ b/pkg/apis/resource/validation/validation.go @@ -37,7 +37,6 @@ import ( "k8s.io/dynamic-resource-allocation/structured" corevalidation "k8s.io/kubernetes/pkg/apis/core/validation" "k8s.io/kubernetes/pkg/apis/resource" - netutils "k8s.io/utils/net" ) var ( @@ -890,25 +889,7 @@ func validateNetworkDeviceData(networkDeviceData *resource.NetworkDeviceData, fl allErrs = append(allErrs, validateSet(networkDeviceData.IPs, resource.NetworkDeviceDataMaxIPs, func(address string, fldPath *field.Path) field.ErrorList { - // reformat CIDR to handle different ways IPs can be written - // (e.g. 2001:db8::1/64 == 2001:0db8::1/64) - ip, ipNet, err := netutils.ParseCIDRSloppy(address) - if err != nil { - // must fail - return validation.IsValidCIDR(fldPath, address) - } - maskSize, _ := ipNet.Mask.Size() - canonical := fmt.Sprintf("%s/%d", ip.String(), maskSize) - if address != canonical { - return field.ErrorList{ - field.Invalid(fldPath, address, fmt.Sprintf("must be in canonical form (%s)", canonical)), - } - } - return nil - }, - func(address string) (string, string) { - return address, "" - }, - fldPath.Child("ips"))...) + return validation.IsValidInterfaceAddress(fldPath, address) + }, stringKey, fldPath.Child("ips"))...) return allErrs } diff --git a/pkg/apis/resource/validation/validation_resourceclaim_test.go b/pkg/apis/resource/validation/validation_resourceclaim_test.go index 1a243920e4f..7224fc82f1f 100644 --- a/pkg/apis/resource/validation/validation_resourceclaim_test.go +++ b/pkg/apis/resource/validation/validation_resourceclaim_test.go @@ -1257,9 +1257,9 @@ func TestValidateClaimStatusUpdate(t *testing.T) { wantFailures: field.ErrorList{ field.TooLong(field.NewPath("status", "devices").Index(0).Child("networkData", "interfaceName"), "", resource.NetworkDeviceDataInterfaceNameMaxLength), field.TooLong(field.NewPath("status", "devices").Index(0).Child("networkData", "hardwareAddress"), "", resource.NetworkDeviceDataHardwareAddressMaxLength), - field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(0), "300.9.8.0/24", "must be a valid CIDR value, (e.g. 10.9.8.0/24 or 2001:db8::/64)"), - field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(1), "010.009.008.000/24", "must be in canonical form (10.9.8.0/24)"), - field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(2), "2001:0db8::1/64", "must be in canonical form (2001:db8::1/64)"), + field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(0), "300.9.8.0/24", "must be a valid address in CIDR form, (e.g. 10.9.8.7/24 or 2001:db8::1/64)"), + field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(1), "010.009.008.000/24", "must be in canonical form (\"10.9.8.0/24\")"), + field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(2), "2001:0db8::1/64", "must be in canonical form (\"2001:db8::1/64\")"), }, oldClaim: func() *resource.ResourceClaim { return validAllocatedClaim }(), update: func(claim *resource.ResourceClaim) *resource.ResourceClaim { @@ -1390,7 +1390,7 @@ func TestValidateClaimStatusUpdate(t *testing.T) { wantFailures: field.ErrorList{ field.TooLong(field.NewPath("status", "devices").Index(0).Child("networkData", "interfaceName"), "", resource.NetworkDeviceDataInterfaceNameMaxLength), field.TooLong(field.NewPath("status", "devices").Index(0).Child("networkData", "hardwareAddress"), "", resource.NetworkDeviceDataHardwareAddressMaxLength), - field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(0), "300.9.8.0/24", "must be a valid CIDR value, (e.g. 10.9.8.0/24 or 2001:db8::/64)"), + field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(0), "300.9.8.0/24", "must be a valid address in CIDR form, (e.g. 10.9.8.7/24 or 2001:db8::1/64)"), }, oldClaim: func() *resource.ResourceClaim { return validAllocatedClaim }(), update: func(claim *resource.ResourceClaim) *resource.ResourceClaim { 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 f073c3668ca..ae9497ff87d 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go @@ -18,6 +18,7 @@ package validation import ( "fmt" + "net" "net/netip" "k8s.io/apimachinery/pkg/util/validation/field" @@ -137,3 +138,30 @@ func GetWarningsForCIDR(fldPath *field.Path, value string) []string { return warnings } + +// IsValidInterfaceAddress tests that the argument is a valid "ifaddr"-style CIDR value in +// canonical form (e.g., "192.168.1.5/24", with a complete IP address and associated +// subnet length). Use IsValidCIDR for "subnet"/"mask"-style CIDR values (e.g., +// "192.168.1.0/24"). +func IsValidInterfaceAddress(fldPath *field.Path, value string) field.ErrorList { + var allErrors field.ErrorList + ip, ipnet, err := netutils.ParseCIDRSloppy(value) + if err != nil { + allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid address in CIDR form, (e.g. 10.9.8.7/24 or 2001:db8::1/64)")) + return allErrors + } + + // The canonical form of `value` is not `ipnet.String()`, because `ipnet` doesn't + // include the bits after the prefix. We need to construct the canonical form + // ourselves from `ip` and `ipnet.Mask`. + maskSize, _ := ipnet.Mask.Size() + if netutils.IsIPv4(ip) && maskSize > net.IPv4len*8 { + // "::ffff:192.168.0.1/120" -> "192.168.0.1/24" + maskSize -= (net.IPv6len - net.IPv4len) * 8 + } + canonical := fmt.Sprintf("%s/%d", ip.String(), maskSize) + if value != canonical { + allErrors = append(allErrors, field.Invalid(fldPath, value, fmt.Sprintf("must be in canonical form (%q)", canonical))) + } + return allErrors +} 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 6fb5d4f15cc..9ff4f9ca0f9 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 @@ -450,3 +450,106 @@ func TestGetWarningsForCIDR(t *testing.T) { }) } } + +func TestIsValidInterfaceAddress(t *testing.T) { + for _, tc := range []struct { + name string + in string + err string + }{ + // GOOD VALUES + { + name: "ipv4", + in: "1.2.3.4/24", + }, + { + name: "ipv4, single IP", + in: "1.1.1.1/32", + }, + { + name: "ipv6", + in: "2001:4860:4860::1/48", + }, + { + name: "ipv6, single IP", + in: "::1/128", + }, + + // BAD VALUES + { + name: "empty string", + in: "", + err: "must be a valid address in CIDR form", + }, + { + name: "junk", + in: "aaaaaaa", + err: "must be a valid address in CIDR form", + }, + { + name: "IP address", + in: "1.2.3.4", + err: "must be a valid address in CIDR form", + }, + { + name: "partial URL", + in: "192.168.0.1/healthz", + err: "must be a valid address in CIDR form", + }, + { + name: "partial URL 2", + in: "192.168.0.1/0/99", + err: "must be a valid address in CIDR form", + }, + { + name: "negative prefix length", + in: "192.168.0.0/-16", + err: "must be a valid address in CIDR form", + }, + { + name: "prefix length with sign", + in: "192.168.0.0/+16", + err: "must be a valid address in CIDR form", + }, + { + name: "ipv6 non-canonical", + in: "2001:0:0:0::0BCD/64", + err: `must be in canonical form ("2001::bcd/64")`, + }, + { + name: "ipv4 with leading 0s", + in: "1.1.01.002/24", + err: `must be in canonical form ("1.1.1.2/24")`, + }, + { + name: "ipv4-in-ipv6 with ipv4-sized prefix", + in: "::ffff:1.1.1.1/24", + err: `must be in canonical form ("1.1.1.1/24")`, + }, + { + name: "ipv4-in-ipv6 with ipv6-sized prefix", + in: "::ffff:1.1.1.1/120", + err: `must be in canonical form ("1.1.1.1/24")`, + }, + { + name: "prefix length with leading 0s", + in: "192.168.0.5/016", + err: `must be in canonical form ("192.168.0.5/16")`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + errs := IsValidInterfaceAddress(field.NewPath(""), tc.in) + if tc.err == "" { + if len(errs) != 0 { + t.Errorf("expected %q to be valid but got: %v", tc.in, errs) + } + } else { + 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) + } + } + }) + } +} From ba189de78ff13f6882e753138a5df07d6ab49433 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 28 Feb 2025 18:14:44 -0500 Subject: [PATCH 4/6] 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) - } - } }) } } From 692785d25b688f83db14c20f26d0b99e2e99adb8 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 28 Feb 2025 17:41:10 -0500 Subject: [PATCH 5/6] Add legacy versions of IsValidIP/IsValidCIDR Add validation.IsValidIPForLegacyField and validation.IsValidCIDRForLegacyField, which validate "legacy" IP/CIDR fields correctly. Use them for all such fields (indirectly, via a wrapper in pkg/apis/core/validation that handles the StrictIPCIDRValidation feature gate correctly). Change IsValidIP and IsValidCIDR to require strict parsing and canonical form, and update the IPAddr, ServiceCIDR, and NetworkDeviceData validation to make use of them. --- pkg/apis/core/validation/validation.go | 36 ++- pkg/apis/core/validation/validation_test.go | 246 +++++++++++++++++- pkg/apis/discovery/validation/validation.go | 2 +- .../discovery/validation/validation_test.go | 36 +++ pkg/apis/networking/validation/validation.go | 35 +-- .../networking/validation/validation_test.go | 71 ++++- pkg/features/kube_features.go | 6 + pkg/features/versioned_kube_features.go | 4 + .../apimachinery/pkg/util/validation/ip.go | 139 +++++++++- .../pkg/util/validation/ip_test.go | 216 +++++++++++++-- .../test_data/versioned_feature_list.yaml | 6 + 11 files changed, 709 insertions(+), 88 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 3069d74467a..68166359257 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3790,7 +3790,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 { - allErrs = append(allErrs, validation.IsValidIP(fldPath.Child("nameservers").Index(i), ns)...) + allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Child("nameservers").Index(i), ns)...) } // Validate searches. if len(dnsConfig.Searches) > MaxDNSSearchPaths { @@ -3966,7 +3966,7 @@ func validateOnlyDeletedSchedulingGates(newGates, oldGates []core.PodSchedulingG func ValidateHostAliases(hostAliases []core.HostAlias, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for i, hostAlias := range hostAliases { - allErrs = append(allErrs, validation.IsValidIP(fldPath.Index(i).Child("ip"), hostAlias.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(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))...) } @@ -4115,7 +4115,7 @@ func validatePodIPs(pod *core.Pod) field.ErrorList { // all PodIPs must be valid IPs for i, podIP := range pod.Status.PodIPs { - allErrs = append(allErrs, validation.IsValidIP(podIPsField.Index(i), podIP.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(podIPsField.Index(i), podIP.IP)...) } // if we have more than one Pod.PodIP then we must have a dual-stack pair @@ -4156,7 +4156,7 @@ func validateHostIPs(pod *core.Pod) field.ErrorList { // all HostIPs must be valid IPs for i, hostIP := range pod.Status.HostIPs { - allErrs = append(allErrs, validation.IsValidIP(hostIPsField.Index(i), hostIP.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(hostIPsField.Index(i), hostIP.IP)...) } // if we have more than one Pod.HostIP then we must have a dual-stack pair @@ -5940,7 +5940,7 @@ func ValidateService(service *core.Service) field.ErrorList { ipPath := specPath.Child("externalIPs") for i, ip := range service.Spec.ExternalIPs { idxPath := ipPath.Index(i) - if errs := validation.IsValidIP(idxPath, ip); len(errs) != 0 { + if errs := IsValidIPForLegacyField(idxPath, ip); len(errs) != 0 { allErrs = append(allErrs, errs...) } else { // For historical reasons, this uses ValidateEndpointIP even @@ -6008,7 +6008,7 @@ func ValidateService(service *core.Service) field.ErrorList { // Note: due to a historical accident around transition from the // annotation value, these values are allowed to be space-padded. value = strings.TrimSpace(value) - allErrs = append(allErrs, validation.IsValidCIDR(fieldPath.Index(idx), value)...) + allErrs = append(allErrs, IsValidCIDRForLegacyField(fieldPath.Index(idx), value)...) } } else if val, annotationSet := service.Annotations[core.AnnotationLoadBalancerSourceRangesKey]; annotationSet { fieldPath := field.NewPath("metadata", "annotations").Key(core.AnnotationLoadBalancerSourceRangesKey) @@ -6021,7 +6021,7 @@ func ValidateService(service *core.Service) field.ErrorList { cidrs := strings.Split(val, ",") for _, value := range cidrs { value = strings.TrimSpace(value) - allErrs = append(allErrs, validation.IsValidCIDR(fieldPath, value)...) + allErrs = append(allErrs, IsValidCIDRForLegacyField(fieldPath, value)...) } } } @@ -6405,7 +6405,7 @@ func ValidateNode(node *core.Node) field.ErrorList { // all PodCIDRs should be valid ones for idx, value := range node.Spec.PodCIDRs { - allErrs = append(allErrs, validation.IsValidCIDR(podCIDRsField.Index(idx), value)...) + allErrs = append(allErrs, IsValidCIDRForLegacyField(podCIDRsField.Index(idx), value)...) } // if more than PodCIDR then it must be a dual-stack pair @@ -7481,7 +7481,7 @@ func validateEndpointSubsets(subsets []core.EndpointSubset, fldPath *field.Path) func validateEndpointAddress(address *core.EndpointAddress, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, validation.IsValidIP(fldPath.Child("ip"), address.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Child("ip"), address.IP)...) if len(address.Hostname) > 0 { allErrs = append(allErrs, ValidateDNS1123Label(address.Hostname, fldPath.Child("hostname"))...) } @@ -7853,7 +7853,7 @@ func ValidateLoadBalancerStatus(status *core.LoadBalancerStatus, fldPath *field. for i, ingress := range status.Ingress { idxPath := ingrPath.Index(i) if len(ingress.IP) > 0 { - allErrs = append(allErrs, validation.IsValidIP(idxPath.Child("ip"), ingress.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(idxPath.Child("ip"), ingress.IP)...) } if utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) && ingress.IPMode == nil { @@ -8188,7 +8188,7 @@ func ValidateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorLi } // is it valid ip? - errorMessages := validation.IsValidIP(clusterIPsField.Index(i), clusterIP) + errorMessages := IsValidIPForLegacyField(clusterIPsField.Index(i), clusterIP) hasInvalidIPs = (len(errorMessages) != 0) || hasInvalidIPs allErrs = append(allErrs, errorMessages...) } @@ -8703,3 +8703,17 @@ func isRestartableInitContainer(initContainer *core.Container) bool { } return *initContainer.RestartPolicy == core.ContainerRestartPolicyAlways } + +// IsValidIPForLegacyField is a wrapper around validation.IsValidIPForLegacyField that +// handles setting strictValidation correctly. This is only for fields that use legacy IP +// address validation; use validation.IsValidIP for new fields. +func IsValidIPForLegacyField(fldPath *field.Path, value string) field.ErrorList { + return validation.IsValidIPForLegacyField(fldPath, value, utilfeature.DefaultFeatureGate.Enabled(features.StrictIPCIDRValidation)) +} + +// IsValidCIDRForLegacyField is a wrapper around validation.IsValidCIDRForLegacyField that +// handles setting strictValidation correctly. This is only for fields that use legacy CIDR +// value validation; use validation.IsValidCIDR for new fields. +func IsValidCIDRForLegacyField(fldPath *field.Path, value string) field.ErrorList { + return validation.IsValidCIDRForLegacyField(fldPath, value, utilfeature.DefaultFeatureGate.Enabled(features.StrictIPCIDRValidation)) +} diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index eca9b3c5e08..13226465734 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -9738,6 +9738,7 @@ func TestValidatePodDNSConfig(t *testing.T) { dnsConfig *core.PodDNSConfig dnsPolicy *core.DNSPolicy opts PodValidationOptions + legacyIPs bool expectedError bool }{{ desc: "valid: empty DNSConfig", @@ -9932,6 +9933,19 @@ func TestValidatePodDNSConfig(t *testing.T) { Nameservers: []string{"invalid"}, }, expectedError: true, + }, { + desc: "valid legacy IP nameserver with legacy IP validation", + dnsConfig: &core.PodDNSConfig{ + Nameservers: []string{"001.002.003.004"}, + }, + legacyIPs: true, + expectedError: false, + }, { + desc: "invalid legacy IP nameserver with strict IP validation", + dnsConfig: &core.PodDNSConfig{ + Nameservers: []string{"001.002.003.004"}, + }, + expectedError: true, }, { desc: "invalid empty option name", dnsConfig: &core.PodDNSConfig{ @@ -9952,6 +9966,8 @@ func TestValidatePodDNSConfig(t *testing.T) { for _, tc := range testCases { t.Run("", func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !tc.legacyIPs) + if tc.dnsPolicy == nil { tc.dnsPolicy = &testDNSClusterFirst } @@ -10192,6 +10208,24 @@ func TestValidatePodSpec(t *testing.T) { } for k, v := range successCases { t.Run(k, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) + opts := PodValidationOptions{ + ResourceIsPod: true, + } + if errs := ValidatePodSpec(&v.Spec, nil, field.NewPath("field"), opts); len(errs) != 0 { + t.Errorf("expected success: %v", errs) + } + }) + } + + legacyValidationCases := map[string]*core.Pod{ + "populate HostAliases with legacy IP with legacy validation": podtest.MakePod("", + podtest.SetHostAliases(core.HostAlias{IP: "012.034.056.078", Hostnames: []string{"host1", "host2"}}), + ), + } + for k, v := range legacyValidationCases { + t.Run(k, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, false) opts := PodValidationOptions{ ResourceIsPod: true, } @@ -10237,6 +10271,12 @@ func TestValidatePodSpec(t *testing.T) { }), podtest.SetHostAliases(core.HostAlias{IP: "999.999.999.999", Hostnames: []string{"host1", "host2"}}), ), + "with hostAliases with invalid legacy IP with strict IP validation": *podtest.MakePod("", + podtest.SetSecurityContext(&core.PodSecurityContext{ + HostNetwork: false, + }), + podtest.SetHostAliases(core.HostAlias{IP: "001.002.003.004", Hostnames: []string{"host1", "host2"}}), + ), "with hostAliases with invalid hostname": *podtest.MakePod("", podtest.SetSecurityContext(&core.PodSecurityContext{ HostNetwork: false, @@ -10307,6 +10347,7 @@ func TestValidatePodSpec(t *testing.T) { } for k, v := range failureCases { t.Run(k, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) opts := PodValidationOptions{ ResourceIsPod: true, } @@ -15118,7 +15159,17 @@ func TestValidateServiceCreate(t *testing.T) { tweakSvc func(svc *core.Service) // given a basic valid service, each test case can customize it numErrs int featureGates []featuregate.Feature + legacyIPs bool }{{ + name: "default", + tweakSvc: func(s *core.Service) {}, + numErrs: 0, + }, { + name: "default, with legacy IP validation", + tweakSvc: func(s *core.Service) {}, + legacyIPs: true, + numErrs: 0, + }, { name: "missing namespace", tweakSvc: func(s *core.Service) { s.Namespace = "" @@ -15254,6 +15305,21 @@ func TestValidateServiceCreate(t *testing.T) { s.Spec.ClusterIPs = []string{"invalid"} }, numErrs: 1, + }, { + name: "valid legacy cluster ip with legacy validation", + tweakSvc: func(s *core.Service) { + s.Spec.ClusterIP = "001.002.003.004" + s.Spec.ClusterIPs = []string{"001.002.003.004"} + }, + legacyIPs: true, + numErrs: 0, + }, { + name: "invalid legacy cluster ip with strict validation", + tweakSvc: func(s *core.Service) { + s.Spec.ClusterIP = "001.002.003.004" + s.Spec.ClusterIPs = []string{"001.002.003.004"} + }, + numErrs: 1, }, { name: "missing port", tweakSvc: func(s *core.Service) { @@ -15333,6 +15399,21 @@ func TestValidateServiceCreate(t *testing.T) { s.Spec.ExternalIPs = []string{"myhost.mydomain"} }, numErrs: 1, + }, { + name: "valid legacy externalIPs with legacy validation", + tweakSvc: func(s *core.Service) { + s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + s.Spec.ExternalIPs = []string{"001.002.003.004"} + }, + legacyIPs: true, + numErrs: 0, + }, { + name: "invalid legacy externalIPs with strict validation", + tweakSvc: func(s *core.Service) { + s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + s.Spec.ExternalIPs = []string{"001.002.003.004"} + }, + numErrs: 1, }, { name: "valid externalIPs", tweakSvc: func(s *core.Service) { @@ -15644,6 +15725,34 @@ func TestValidateServiceCreate(t *testing.T) { s.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "" }, numErrs: 1, + }, { + name: "valid legacy LoadBalancer source range with legacy validation", + tweakSvc: func(s *core.Service) { + s.Spec.Type = core.ServiceTypeLoadBalancer + s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + s.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + s.Spec.LoadBalancerSourceRanges = []string{"001.002.003.000/24"} + }, + legacyIPs: true, + numErrs: 0, + }, { + name: "invalid legacy LoadBalancer source range with strict validation", + tweakSvc: func(s *core.Service) { + s.Spec.Type = core.ServiceTypeLoadBalancer + s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + s.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + s.Spec.LoadBalancerSourceRanges = []string{"001.002.003.000/24"} + }, + numErrs: 1, + }, { + name: "invalid legacy LoadBalancer source range annotation with strict validation", + tweakSvc: func(s *core.Service) { + s.Spec.Type = core.ServiceTypeLoadBalancer + s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + s.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + s.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "001.002.003.000/24" + }, + numErrs: 1, }, { name: "valid LoadBalancer source range", tweakSvc: func(s *core.Service) { @@ -16318,6 +16427,7 @@ func TestValidateServiceCreate(t *testing.T) { for i := range tc.featureGates { featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, tc.featureGates[i], true) } + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !tc.legacyIPs) svc := makeValidService() tc.tweakSvc(&svc) errs := ValidateServiceCreate(&svc) @@ -16994,12 +17104,41 @@ func TestValidateNode(t *testing.T) { } for _, successCase := range successCases { t.Run("", func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) if errs := ValidateNode(&successCase); len(errs) != 0 { t.Errorf("expected success: %v", errs) } }) } + legacyValidationCases := map[string]core.Node{ + "valid-legacy-pod-cidr-with-legacy-validation": { + ObjectMeta: metav1.ObjectMeta{ + Name: "abc", + }, + Status: core.NodeStatus{ + Addresses: []core.NodeAddress{ + {Type: core.NodeExternalIP, Address: "something"}, + }, + Capacity: core.ResourceList{ + core.ResourceName(core.ResourceCPU): resource.MustParse("10"), + core.ResourceName(core.ResourceMemory): resource.MustParse("0"), + }, + }, + Spec: core.NodeSpec{ + PodCIDRs: []string{"192.168.000.000/16"}, + }, + }, + } + for name, legacyCase := range legacyValidationCases { + t.Run(name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, false) + if errs := ValidateNode(&legacyCase); len(errs) != 0 { + t.Errorf("expected success: %v", errs) + } + }) + } + errorCases := map[string]core.Node{ "zero-length Name": { ObjectMeta: metav1.ObjectMeta{ @@ -17182,6 +17321,23 @@ func TestValidateNode(t *testing.T) { PodCIDRs: []string{"192.168.0.0"}, }, }, + "invalid-legacy-pod-cidr-with-strict-validation": { + ObjectMeta: metav1.ObjectMeta{ + Name: "abc", + }, + Status: core.NodeStatus{ + Addresses: []core.NodeAddress{ + {Type: core.NodeExternalIP, Address: "something"}, + }, + Capacity: core.ResourceList{ + core.ResourceName(core.ResourceCPU): resource.MustParse("10"), + core.ResourceName(core.ResourceMemory): resource.MustParse("0"), + }, + }, + Spec: core.NodeSpec{ + PodCIDRs: []string{"192.168.000.000/16"}, + }, + }, "duplicate-pod-cidr": { ObjectMeta: metav1.ObjectMeta{ Name: "abc", @@ -17202,6 +17358,8 @@ func TestValidateNode(t *testing.T) { } for k, v := range errorCases { t.Run(k, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) + errs := ValidateNode(&v) if len(errs) == 0 { t.Errorf("expected failure") @@ -20590,9 +20748,36 @@ func TestValidateEndpointsCreate(t *testing.T) { }, }, } - for name, tc := range successCases { t.Run(name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) + errs := ValidateEndpointsCreate(&tc.endpoints) + if len(errs) != 0 { + t.Errorf("Expected no validation errors, got %v", errs) + } + + }) + } + + legacyValidationCases := map[string]struct { + endpoints core.Endpoints + }{ + "legacy IPs with legacy validation": { + endpoints: core.Endpoints{ + ObjectMeta: metav1.ObjectMeta{Name: "mysvc", Namespace: "namespace"}, + Subsets: []core.EndpointSubset{{ + Addresses: []core.EndpointAddress{{IP: "010.010.001.001"}, {IP: "10.10.2.2"}}, + Ports: []core.EndpointPort{{Name: "a", Port: 8675, Protocol: "TCP"}, {Name: "b", Port: 309, Protocol: "TCP"}}, + }, { + Addresses: []core.EndpointAddress{{IP: "::ffff:10.10.3.3"}}, + Ports: []core.EndpointPort{{Name: "a", Port: 93, Protocol: "TCP"}, {Name: "b", Port: 76, Protocol: "TCP"}}, + }}, + }, + }, + } + for name, tc := range legacyValidationCases { + t.Run(name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, false) errs := ValidateEndpointsCreate(&tc.endpoints) if len(errs) != 0 { t.Errorf("Expected no validation errors, got %v", errs) @@ -20652,6 +20837,18 @@ func TestValidateEndpointsCreate(t *testing.T) { field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=ip-sloppy"), }, }, + "invalid legacy IP with strict validation": { + endpoints: core.Endpoints{ + ObjectMeta: metav1.ObjectMeta{Name: "mysvc", Namespace: "namespace"}, + Subsets: []core.EndpointSubset{{ + Addresses: []core.EndpointAddress{{IP: "001.002.003.004"}}, + Ports: []core.EndpointPort{{Name: "a", Port: 93, Protocol: "TCP"}}, + }}, + }, + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=ip-sloppy"), + }, + }, "Multiple ports, one without name": { endpoints: core.Endpoints{ ObjectMeta: metav1.ObjectMeta{Name: "mysvc", Namespace: "namespace"}, @@ -20776,6 +20973,7 @@ func TestValidateEndpointsCreate(t *testing.T) { for k, v := range errorCases { t.Run(k, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) errs := ValidateEndpointsCreate(&v.endpoints) // TODO: set .RequireOriginWhenInvalid() once metadata is done matcher := fldtest.ErrorMatcher{}.ByType().ByField().ByOrigin() @@ -22838,6 +23036,7 @@ func TestPodIPsValidation(t *testing.T) { testCases := []struct { pod core.Pod + legacyIPs bool expectError bool }{ { @@ -22858,11 +23057,18 @@ func TestPodIPsValidation(t *testing.T) { }, { expectError: false, pod: makePod("dual-stack-6-4", "ns", []core.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), + }, { + expectError: false, + legacyIPs: true, + pod: makePod("legacy-pod-ip-with-legacy-validation", "ns", []core.PodIP{{IP: "001.002.003.004"}}), }, /* failure cases start here */ { expectError: true, pod: makePod("invalid-pod-ip", "ns", []core.PodIP{{IP: "this-is-not-an-ip"}}), + }, { + expectError: true, + pod: makePod("legacy-pod-ip-with-strict-validation", "ns", []core.PodIP{{IP: "001.002.003.004"}}), }, { expectError: true, pod: makePod("dualstack-same-ip-family-6", "ns", []core.PodIP{{IP: "::1"}, {IP: "::2"}}), @@ -22888,6 +23094,7 @@ func TestPodIPsValidation(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.pod.Name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !testCase.legacyIPs) for _, oldTestCase := range testCases { newPod := testCase.pod.DeepCopy() newPod.ResourceVersion = "1" @@ -22938,6 +23145,7 @@ func TestHostIPsValidation(t *testing.T) { testCases := []struct { pod core.Pod + legacyIPs bool expectError bool }{ { @@ -22964,11 +23172,20 @@ func TestHostIPsValidation(t *testing.T) { expectError: false, pod: makePodWithHostIPs("dual-stack-6-4", "ns", []core.HostIP{{IP: "::1"}, {IP: "1.1.1.1"}}), }, + { + expectError: false, + legacyIPs: true, + pod: makePodWithHostIPs("legacy-host-ip-with-legacy-validation", "ns", []core.HostIP{{IP: "001.002.003.004"}}), + }, /* failure cases start here */ { expectError: true, pod: makePodWithHostIPs("invalid-host-ip", "ns", []core.HostIP{{IP: "this-is-not-an-ip"}}), }, + { + expectError: true, + pod: makePodWithHostIPs("invalid-legacy-host-ip-with-strict-validation", "ns", []core.HostIP{{IP: "001.002.003.004"}}), + }, { expectError: true, pod: makePodWithHostIPs("dualstack-same-ip-family-6", "ns", []core.HostIP{{IP: "::1"}, {IP: "::2"}}), @@ -22998,6 +23215,7 @@ func TestHostIPsValidation(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.pod.Name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !testCase.legacyIPs) for _, oldTestCase := range testCases { newPod := testCase.pod.DeepCopy() newPod.ResourceVersion = "1" @@ -24566,6 +24784,7 @@ func TestValidateLoadBalancerStatus(t *testing.T) { testCases := []struct { name string ipModeEnabled bool + legacyIPs bool nonLBAllowed bool tweakLBStatus func(s *core.LoadBalancerStatus) tweakSvcSpec func(s *core.ServiceSpec) @@ -24652,12 +24871,37 @@ func TestValidateLoadBalancerStatus(t *testing.T) { }} }, numErrs: 1, + }, { + name: "legacy IP with legacy validation", + ipModeEnabled: true, + legacyIPs: true, + tweakLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "001.002.003.004", + IPMode: &ipModeVIP, + }} + }, + numErrs: 0, + }, { + name: "legacy IP with strict validation", + ipModeEnabled: true, + tweakLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "001.002.003.004", + IPMode: &ipModeVIP, + }} + }, + numErrs: 1, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { if !tc.ipModeEnabled { featuregatetesting.SetFeatureGateEmulationVersionDuringTest(t, utilfeature.DefaultFeatureGate, version.MustParse("1.31")) + } else { + // (This feature gate doesn't exist in 1.31 so we can't set it + // when testing !ipModeEnabled.) + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !tc.legacyIPs) } featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LoadBalancerIPMode, tc.ipModeEnabled) featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AllowServiceLBStatusOnNonLB, tc.nonLBAllowed) diff --git a/pkg/apis/discovery/validation/validation.go b/pkg/apis/discovery/validation/validation.go index 706c1194dcd..a50cec5b43c 100644 --- a/pkg/apis/discovery/validation/validation.go +++ b/pkg/apis/discovery/validation/validation.go @@ -100,7 +100,7 @@ func validateEndpoints(endpoints []discovery.Endpoint, addrType discovery.Addres // and do not get validated. switch addrType { case discovery.AddressTypeIPv4: - ipErrs := validation.IsValidIP(addressPath.Index(i), address) + ipErrs := apivalidation.IsValidIPForLegacyField(addressPath.Index(i), address) if len(ipErrs) > 0 { allErrs = append(allErrs, ipErrs...) } else { diff --git a/pkg/apis/discovery/validation/validation_test.go b/pkg/apis/discovery/validation/validation_test.go index 16366de6e57..96830d51a9e 100644 --- a/pkg/apis/discovery/validation/validation_test.go +++ b/pkg/apis/discovery/validation/validation_test.go @@ -23,8 +23,11 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilfeature "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/discovery" + "k8s.io/kubernetes/pkg/features" "k8s.io/utils/ptr" ) @@ -36,6 +39,7 @@ func TestValidateEndpointSlice(t *testing.T) { testCases := map[string]struct { expectedErrors int + legacyIPs bool endpointSlice *discovery.EndpointSlice }{ "good-slice": { @@ -235,6 +239,22 @@ func TestValidateEndpointSlice(t *testing.T) { }}, }, }, + "legacy-ip-with-legacy-validation": { + expectedErrors: 0, + legacyIPs: true, + endpointSlice: &discovery.EndpointSlice{ + ObjectMeta: standardMeta, + AddressType: discovery.AddressTypeIPv4, + Ports: []discovery.EndpointPort{{ + Name: ptr.To("http"), + Protocol: ptr.To(api.ProtocolTCP), + }}, + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"012.034.056.078"}, + Hostname: ptr.To("valid-123"), + }}, + }, + }, // expected failures "duplicate-port-name": { @@ -422,6 +442,21 @@ func TestValidateEndpointSlice(t *testing.T) { }}, }, }, + "legacy-ip-with-strict-validation": { + expectedErrors: 1, + endpointSlice: &discovery.EndpointSlice{ + ObjectMeta: standardMeta, + AddressType: discovery.AddressTypeIPv4, + Ports: []discovery.EndpointPort{{ + Name: ptr.To("http"), + Protocol: ptr.To(api.ProtocolTCP), + }}, + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"012.034.056.078"}, + Hostname: ptr.To("valid-123"), + }}, + }, + }, "bad-ipv4": { expectedErrors: 2, endpointSlice: &discovery.EndpointSlice{ @@ -601,6 +636,7 @@ func TestValidateEndpointSlice(t *testing.T) { for name, testCase := range testCases { t.Run(name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !testCase.legacyIPs) errs := ValidateEndpointSlice(testCase.endpointSlice) if len(errs) != testCase.expectedErrors { t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs) diff --git a/pkg/apis/networking/validation/validation.go b/pkg/apis/networking/validation/validation.go index 1371ec7d8c2..8f00d0d8ffc 100644 --- a/pkg/apis/networking/validation/validation.go +++ b/pkg/apis/networking/validation/validation.go @@ -18,7 +18,6 @@ package validation import ( "fmt" - "net/netip" "strings" apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation" @@ -220,7 +219,7 @@ func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path) field.ErrorLi allErrs = append(allErrs, field.Required(fldPath.Child("cidr"), "")) return allErrs } - allErrs = append(allErrs, validation.IsValidCIDR(fldPath.Child("cidr"), ipb.CIDR)...) + allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(fldPath.Child("cidr"), ipb.CIDR)...) _, cidrIPNet, err := netutils.ParseCIDRSloppy(ipb.CIDR) if err != nil { // Implies validation would have failed so we already added errors for it. @@ -229,7 +228,7 @@ func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path) field.ErrorLi for i, exceptCIDRStr := range ipb.Except { exceptPath := fldPath.Child("except").Index(i) - allErrs = append(allErrs, validation.IsValidCIDR(exceptPath, exceptCIDRStr)...) + allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(exceptPath, exceptCIDRStr)...) _, exceptCIDR, err := netutils.ParseCIDRSloppy(exceptCIDRStr) if err != nil { // Implies validation would have failed so we already added errors for it. @@ -357,7 +356,7 @@ func ValidateIngressLoadBalancerStatus(status *networking.IngressLoadBalancerSta for i, ingress := range status.Ingress { idxPath := fldPath.Child("ingress").Index(i) if len(ingress.IP) > 0 { - allErrs = append(allErrs, validation.IsValidIP(idxPath.Child("ip"), ingress.IP)...) + allErrs = append(allErrs, apivalidation.IsValidIPForLegacyField(idxPath.Child("ip"), ingress.IP)...) } if len(ingress.Hostname) > 0 { for _, msg := range validation.IsDNS1123Subdomain(ingress.Hostname) { @@ -653,12 +652,12 @@ func allowInvalidWildcardHostRule(oldIngress *networking.Ingress) bool { // IPAddress does not support generating names, prefix is not considered. func ValidateIPAddressName(name string, prefix bool) []string { var errs []string - ip, err := netip.ParseAddr(name) - if err != nil { - errs = append(errs, err.Error()) - } else if ip.String() != name { - errs = append(errs, "must be a canonical format IP address") + allErrs := validation.IsValidIP(&field.Path{}, name) + // Need to unconvert the field.Error from IsValidIP back to a string so our caller + // can convert it back to a field.Error! + for _, err := range allErrs { + errs = append(errs, err.Detail) } return errs } @@ -748,28 +747,12 @@ func ValidateServiceCIDR(cidrConfig *networking.ServiceCIDR) field.ErrorList { } for i, cidr := range cidrConfig.Spec.CIDRs { - allErrs = append(allErrs, validateCIDR(cidr, fieldPath.Index(i))...) + allErrs = append(allErrs, validation.IsValidCIDR(fieldPath.Index(i), cidr)...) } return allErrs } -func validateCIDR(cidr string, fldPath *field.Path) field.ErrorList { - allErrs := field.ErrorList{} - prefix, err := netip.ParsePrefix(cidr) - if err != nil { - allErrs = append(allErrs, field.Invalid(fldPath, cidr, err.Error())) - } else { - if prefix.Addr() != prefix.Masked().Addr() { - allErrs = append(allErrs, field.Invalid(fldPath, cidr, "wrong CIDR format, IP doesn't match network IP address")) - } - if prefix.String() != cidr { - allErrs = append(allErrs, field.Invalid(fldPath, cidr, "CIDR not in RFC 5952 canonical format")) - } - } - return allErrs -} - // ValidateServiceCIDRUpdate tests if an update to a ServiceCIDR is valid. func ValidateServiceCIDRUpdate(update, old *networking.ServiceCIDR) field.ErrorList { var allErrs field.ErrorList diff --git a/pkg/apis/networking/validation/validation_test.go b/pkg/apis/networking/validation/validation_test.go index daf22967bf4..438d7f5bea7 100644 --- a/pkg/apis/networking/validation/validation_test.go +++ b/pkg/apis/networking/validation/validation_test.go @@ -25,8 +25,11 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/validation/field" + utilfeature "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/networking" + "k8s.io/kubernetes/pkg/features" utilpointer "k8s.io/utils/pointer" ) @@ -248,9 +251,23 @@ func TestValidateNetworkPolicy(t *testing.T) { } // Success cases are expected to pass validation. - for _, v := range successCases { t.Run("", func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) + if errs := ValidateNetworkPolicy(v, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) != 0 { + t.Errorf("Expected success, got %v", errs) + } + }) + } + + legacyValidationCases := []*networking.NetworkPolicy{ + makeNetworkPolicyCustom(setIngressFromIPBlockIPV4, func(networkPolicy *networking.NetworkPolicy) { + networkPolicy.Spec.Ingress[0].From[0].IPBlock.CIDR = "001.002.003.000/0" + }), + } + for _, v := range legacyValidationCases { + t.Run("", func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, false) if errs := ValidateNetworkPolicy(v, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) != 0 { t.Errorf("Expected success, got %v", errs) } @@ -301,6 +318,9 @@ func TestValidateNetworkPolicy(t *testing.T) { "invalid ipv6 cidr format": makeNetworkPolicyCustom(setIngressFromIPBlockIPV6, func(networkPolicy *networking.NetworkPolicy) { networkPolicy.Spec.Ingress[0].From[0].IPBlock.CIDR = "fd00:192:168::" }), + "legacy cidr format with strict validation": makeNetworkPolicyCustom(setIngressFromIPBlockIPV4, func(networkPolicy *networking.NetworkPolicy) { + networkPolicy.Spec.Ingress[0].From[0].IPBlock.CIDR = "001.002.003.000/0" + }), "except field is an empty string": makeNetworkPolicyCustom(setIngressFromIPBlockIPV4, func(networkPolicy *networking.NetworkPolicy) { networkPolicy.Spec.Ingress[0].From[0].IPBlock.Except = []string{""} }), @@ -370,6 +390,7 @@ func TestValidateNetworkPolicy(t *testing.T) { // Error cases are not expected to pass validation. for testName, networkPolicy := range errorCases { t.Run(testName, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) if errs := ValidateNetworkPolicy(networkPolicy, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) == 0 { t.Errorf("Expected failure") } @@ -1813,6 +1834,14 @@ func TestValidateIngressStatusUpdate(t *testing.T) { }, }, } + legacyIP := newValid() + legacyIP.Status = networking.IngressStatus{ + LoadBalancer: networking.IngressLoadBalancerStatus{ + Ingress: []networking.IngressLoadBalancerIngress{ + {IP: "001.002.003.004", Hostname: "foo.com"}, + }, + }, + } invalidHostname := newValid() invalidHostname.Status = networking.IngressStatus{ LoadBalancer: networking.IngressLoadBalancerStatus{ @@ -1822,22 +1851,46 @@ func TestValidateIngressStatusUpdate(t *testing.T) { }, } - errs := ValidateIngressStatusUpdate(&newValue, &oldValue) - if len(errs) != 0 { - t.Errorf("Unexpected error %v", errs) + successCases := map[string]struct { + oldValue networking.Ingress + newValue networking.Ingress + legacyIPs bool + }{ + "success": { + oldValue: oldValue, + newValue: newValue, + }, + "legacy IPs with legacy validation": { + oldValue: oldValue, + newValue: legacyIP, + legacyIPs: true, + }, + } + for k, tc := range successCases { + t.Run(k, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !tc.legacyIPs) + + errs := ValidateIngressStatusUpdate(&tc.newValue, &tc.oldValue) + if len(errs) != 0 { + t.Errorf("Unexpected error %v", errs) + } + }) } errorCases := map[string]networking.Ingress{ - "status.loadBalancer.ingress[0].ip: Invalid value": invalidIP, - "status.loadBalancer.ingress[0].hostname: Invalid value": invalidHostname, + "status.loadBalancer.ingress[0].ip: must be a valid IP address": invalidIP, + "status.loadBalancer.ingress[0].ip: must not have leading 0s": legacyIP, + "status.loadBalancer.ingress[0].hostname: must be a DNS name": invalidHostname, } for k, v := range errorCases { t.Run(k, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) + errs := ValidateIngressStatusUpdate(&v, &oldValue) if len(errs) == 0 { t.Errorf("expected failure") } else { - s := strings.Split(k, ":") + s := strings.SplitN(k, ":", 2) err := errs[0] if err.Field != s[0] || !strings.Contains(err.Error(), s[1]) { t.Errorf("unexpected error: %q, expected: %q", err, k) @@ -2160,13 +2213,13 @@ func TestValidateServiceCIDR(t *testing.T) { }, }, "badip-iprange-caps-ipv6": { - expectedErrors: 2, + expectedErrors: 1, ipRange: &networking.ServiceCIDR{ ObjectMeta: metav1.ObjectMeta{ Name: "test-name", }, Spec: networking.ServiceCIDRSpec{ - CIDRs: []string{"FD00:1234::2/64"}, + CIDRs: []string{"FD00:1234::0/64"}, }, }, }, diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 4f0c2ad9ed2..46c9a756017 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -715,6 +715,12 @@ const ( // Allow API server Protobuf encoder to encode collections item by item, instead of all at once. StreamingCollectionEncodingToProtobuf featuregate.Feature = "StreamingCollectionEncodingToProtobuf" + // owner: @danwinship + // kep: https://kep.k8s.io/4858 + // + // Requires stricter validation of IP addresses and CIDR values in API objects. + StrictIPCIDRValidation featuregate.Feature = "StrictIPCIDRValidation" + // owner: @robscott // kep: https://kep.k8s.io/2433 // diff --git a/pkg/features/versioned_kube_features.go b/pkg/features/versioned_kube_features.go index 79c4ecc0942..28948ef1b10 100644 --- a/pkg/features/versioned_kube_features.go +++ b/pkg/features/versioned_kube_features.go @@ -763,6 +763,10 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate {Version: version.MustParse("1.32"), Default: true, PreRelease: featuregate.Beta}, }, + StrictIPCIDRValidation: { + {Version: version.MustParse("1.33"), Default: false, PreRelease: featuregate.Alpha}, + }, + TopologyAwareHints: { {Version: version.MustParse("1.21"), Default: false, PreRelease: featuregate.Alpha}, {Version: version.MustParse("1.23"), Default: false, PreRelease: featuregate.Beta}, 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 bed741b074a..9c836656ed7 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go @@ -26,21 +26,78 @@ import ( netutils "k8s.io/utils/net" ) -// IsValidIP tests that the argument is a valid IP address. -func IsValidIP(fldPath *field.Path, value string) field.ErrorList { +func parseIP(fldPath *field.Path, value string, strictValidation bool) (net.IP, field.ErrorList) { var allErrors field.ErrorList - if netutils.ParseIPSloppy(value) == nil { - allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid IP address, (e.g. 10.9.8.7 or 2001:db8::ffff)").WithOrigin("format=ip-sloppy")) + + ip := netutils.ParseIPSloppy(value) + if ip == nil { + 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, allErrors } - return allErrors + + if strictValidation { + addr, err := netip.ParseAddr(value) + if err != nil { + // If netutils.ParseIPSloppy parsed it, but netip.ParseAddr + // doesn't, then it must have illegal leading 0s. + allErrors = append(allErrors, field.Invalid(fldPath, value, "must not have leading 0s")) + } + if addr.Is4In6() { + allErrors = append(allErrors, field.Invalid(fldPath, value, "must not be an IPv4-mapped IPv6 address")) + } + } + + return ip, allErrors +} + +// IsValidIPForLegacyField tests that the argument is a valid IP address for a "legacy" +// API field that predates strict IP validation. In particular, this allows IPs that are +// not in canonical form (e.g., "FE80:0:0:0:0:0:0:0abc" instead of "fe80::abc"). +// +// If strictValidation is false, this also allows IPs in certain invalid or ambiguous +// formats: +// +// 1. IPv4 IPs are allowed to have leading "0"s in octets (e.g. "010.002.003.004"). +// Historically, net.ParseIP (and later netutils.ParseIPSloppy) simply ignored leading +// "0"s in IPv4 addresses, but most libc-based software treats 0-prefixed IPv4 octets +// as octal, meaning different software might interpret the same string as two +// different IPs, potentially leading to security issues. (Current net.ParseIP and +// netip.ParseAddr simply reject inputs with leading "0"s.) +// +// 2. IPv4-mapped IPv6 IPs (e.g. "::ffff:1.2.3.4") are allowed. These can also lead to +// different software interpreting the value in different ways, because they may be +// treated as IPv4 by some software and IPv6 by other software. (net.ParseIP and +// netip.ParseAddr both allow these, but there are no use cases for representing IPv4 +// addresses as IPv4-mapped IPv6 addresses in Kubernetes.) +// +// This function should only be used to validate the existing fields that were +// historically validated in this way, and strictValidation should be true unless the +// StrictIPCIDRValidation feature gate is disabled. Use IsValidIP for parsing new fields. +func IsValidIPForLegacyField(fldPath *field.Path, value string, strictValidation bool) field.ErrorList { + _, allErrors := parseIP(fldPath, value, strictValidation) + return allErrors.WithOrigin("format=ip-sloppy") +} + +// IsValidIP tests that the argument is a valid IP address, according to current +// Kubernetes standards for IP address validation. +func IsValidIP(fldPath *field.Path, value string) field.ErrorList { + ip, allErrors := parseIP(fldPath, value, true) + if len(allErrors) != 0 { + return allErrors.WithOrigin("format=ip-strict") + } + + if value != ip.String() { + allErrors = append(allErrors, field.Invalid(fldPath, value, fmt.Sprintf("must be in canonical form (%q)", ip.String()))) + } + return allErrors.WithOrigin("format=ip-strict") } // GetWarningsForIP returns warnings for IP address values in non-standard forms. This -// should only be used with fields that are validated with IsValidIP(). +// should only be used with fields that are validated with IsValidIPForLegacyField(). func GetWarningsForIP(fldPath *field.Path, value string) []string { ip := netutils.ParseIPSloppy(value) if ip == nil { - klog.ErrorS(nil, "GetWarningsForIP called on value that was not validated with IsValidIP", "field", fldPath, "value", value) + klog.ErrorS(nil, "GetWarningsForIP called on value that was not validated with IsValidIPForLegacyField", "field", fldPath, "value", value) return nil } @@ -65,22 +122,80 @@ func GetWarningsForIP(fldPath *field.Path, value string) []string { return nil } -// IsValidCIDR tests that the argument is a valid CIDR value. -func IsValidCIDR(fldPath *field.Path, value string) field.ErrorList { +func parseCIDR(fldPath *field.Path, value string, strictValidation bool) (*net.IPNet, field.ErrorList) { var allErrors field.ErrorList - _, _, err := netutils.ParseCIDRSloppy(value) + + _, ipnet, err := netutils.ParseCIDRSloppy(value) if err != nil { allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid CIDR value, (e.g. 10.9.8.0/24 or 2001:db8::/64)")) + return nil, allErrors + } + + if strictValidation { + prefix, err := netip.ParsePrefix(value) + if err != nil { + // If netutils.ParseCIDRSloppy parsed it, but netip.ParsePrefix + // doesn't, then it must have illegal leading 0s (either in the + // IP part or the prefix). + allErrors = append(allErrors, field.Invalid(fldPath, value, "must not have leading 0s in IP or prefix length")) + } else if prefix.Addr().Is4In6() { + allErrors = append(allErrors, field.Invalid(fldPath, value, "must not have an IPv4-mapped IPv6 address")) + } else if prefix.Addr() != prefix.Masked().Addr() { + allErrors = append(allErrors, field.Invalid(fldPath, value, "must not have bits set beyond the prefix length")) + } + } + + return ipnet, allErrors +} + +// IsValidCIDRForLegacyField tests that the argument is a valid CIDR value for a "legacy" +// API field that predates strict IP validation. In particular, this allows IPs that are +// not in canonical form (e.g., "FE80:0abc:0:0:0:0:0:0/64" instead of "fe80:abc::/64"). +// +// If strictValidation is false, this also allows CIDR values in certain invalid or +// ambiguous formats: +// +// 1. The IP part of the CIDR value is parsed as with IsValidIPForLegacyField with +// strictValidation=false. +// +// 2. The CIDR value is allowed to be either a "subnet"/"mask" (with the lower bits after +// the prefix length all being 0), or an "interface address" as with `ip addr` (with a +// complete IP address and associated subnet length). With strict validation, the +// value is required to be in "subnet"/"mask" form. +// +// 3. The prefix length is allowed to have leading 0s. +// +// This function should only be used to validate the existing fields that were +// historically validated in this way, and strictValidation should be true unless the +// StrictIPCIDRValidation feature gate is disabled. Use IsValidCIDR or +// IsValidInterfaceAddress for parsing new fields. +func IsValidCIDRForLegacyField(fldPath *field.Path, value string, strictValidation bool) field.ErrorList { + _, allErrors := parseCIDR(fldPath, value, strictValidation) + return allErrors +} + +// IsValidCIDR tests that the argument is a valid CIDR value, according to current +// Kubernetes standards for CIDR validation. This function is only for +// "subnet"/"mask"-style CIDR values (e.g., "192.168.1.0/24", with no bits set beyond the +// prefix length). Use IsValidInterfaceAddress for "ifaddr"-style CIDR values. +func IsValidCIDR(fldPath *field.Path, value string) field.ErrorList { + ipnet, allErrors := parseCIDR(fldPath, value, true) + if len(allErrors) != 0 { + return allErrors + } + + if value != ipnet.String() { + allErrors = append(allErrors, field.Invalid(fldPath, value, fmt.Sprintf("must be in canonical form (%q)", ipnet.String()))) } return allErrors } // GetWarningsForCIDR returns warnings for CIDR values in non-standard forms. This should -// only be used with fields that are validated with IsValidCIDR(). +// only be used with fields that are validated with IsValidCIDRForLegacyField(). func GetWarningsForCIDR(fldPath *field.Path, value string) []string { ip, ipnet, err := netutils.ParseCIDRSloppy(value) if err != nil { - klog.ErrorS(err, "GetWarningsForCIDR called on value that was not validated with IsValidCIDR", "field", fldPath, "value", value) + klog.ErrorS(err, "GetWarningsForCIDR called on value that was not validated with IsValidCIDRForLegacyField", "field", fldPath, "value", value) return nil } 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 8c33a29134d..fed7e73d3a9 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 @@ -28,7 +28,10 @@ func TestIsValidIP(t *testing.T) { for _, tc := range []struct { name string in string - err string + + err string + legacyErr string + legacyStrictErr string }{ // GOOD VALUES { @@ -56,95 +59,148 @@ func TestIsValidIP(t *testing.T) { in: "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", }, - // GOOD, THOUGH NON-CANONICAL, VALUES + // NON-CANONICAL VALUES { name: "ipv6, all zeros, expanded (non-canonical)", in: "0:0:0:0:0:0:0:0", + + err: `must be in canonical form ("::")`, }, { name: "ipv6, leading 0s (non-canonical)", in: "0001:002:03:4::", + + err: `must be in canonical form ("1:2:3:4::")`, }, { name: "ipv6, capital letters (non-canonical)", in: "1234::ABCD", + + err: `must be in canonical form ("1234::abcd")`, }, - // BAD VALUES WE CURRENTLY CONSIDER GOOD + // GOOD WITH LEGACY VALIDATION, BAD WITH STRICT VALIDATION { name: "ipv4 with leading 0s", in: "1.1.1.01", + + err: "must not have leading 0s", + legacyErr: "", + legacyStrictErr: "must not have leading 0s", }, { name: "ipv4-in-ipv6 value", in: "::ffff:1.1.1.1", + + err: "must not be an IPv4-mapped IPv6 address", + legacyErr: "", + legacyStrictErr: "must not be an IPv4-mapped IPv6 address", }, // BAD VALUES { name: "empty string", in: "", - err: "must be a valid IP address", + + err: "must be a valid IP address", + legacyErr: "must be a valid IP address", + legacyStrictErr: "must be a valid IP address", }, { name: "junk", in: "aaaaaaa", - err: "must be a valid IP address", + + err: "must be a valid IP address", + legacyErr: "must be a valid IP address", + legacyStrictErr: "must be a valid IP address", }, { name: "domain name", in: "myhost.mydomain", - err: "must be a valid IP address", + + err: "must be a valid IP address", + legacyErr: "must be a valid IP address", + legacyStrictErr: "must be a valid IP address", }, { name: "cidr", in: "1.2.3.0/24", - err: "must be a valid IP address", + + err: "must be a valid IP address", + legacyErr: "must be a valid IP address", + legacyStrictErr: "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", + + err: "must be a valid IP address", + legacyErr: "must be a valid IP address", + legacyStrictErr: "must be a valid IP address", }, { name: "ipv4 with negative octets", in: "-1.0.0.0", - err: "must be a valid IP address", + + err: "must be a valid IP address", + legacyErr: "must be a valid IP address", + legacyStrictErr: "must be a valid IP address", }, { name: "ipv6 with out-of-range segment", in: "2001:db8::10005", - err: "must be a valid IP address", + + err: "must be a valid IP address", + legacyErr: "must be a valid IP address", + legacyStrictErr: "must be a valid IP address", }, { name: "ipv4:port", in: "1.2.3.4:80", - err: "must be a valid IP address", + + err: "must be a valid IP address", + legacyErr: "must be a valid IP address", + legacyStrictErr: "must be a valid IP address", }, { name: "ipv6 with brackets", in: "[2001:db8::1]", - err: "must be a valid IP address", + + err: "must be a valid IP address", + legacyErr: "must be a valid IP address", + legacyStrictErr: "must be a valid IP address", }, { name: "[ipv6]:port", in: "[2001:db8::1]:80", - err: "must be a valid IP address", + + err: "must be a valid IP address", + legacyErr: "must be a valid IP address", + legacyStrictErr: "must be a valid IP address", }, { name: "host:port", in: "example.com:80", - err: "must be a valid IP address", + + err: "must be a valid IP address", + legacyErr: "must be a valid IP address", + legacyStrictErr: "must be a valid IP address", }, { name: "ipv6 with zone", in: "1234::abcd%eth0", - err: "must be a valid IP address", + + err: "must be a valid IP address", + legacyErr: "must be a valid IP address", + legacyStrictErr: "must be a valid IP address", }, { name: "ipv4 with zone", in: "169.254.0.0%eth0", - err: "must be a valid IP address", + + err: "must be a valid IP address", + legacyErr: "must be a valid IP address", + legacyStrictErr: "must be a valid IP address", }, } { t.Run(tc.name, func(t *testing.T) { @@ -160,6 +216,32 @@ 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 = IsValidIPForLegacyField(field.NewPath(""), tc.in, false) + if tc.legacyErr == "" { + if len(errs) != 0 { + t.Errorf("expected %q to be valid according to IsValidIPForLegacyField but got: %v", tc.in, errs) + } + } else { + if len(errs) != 1 { + t.Errorf("expected %q to have 1 error from IsValidIPForLegacyField but got: %v", tc.in, errs) + } else if !strings.Contains(errs[0].Detail, tc.legacyErr) { + t.Errorf("expected error from IsValidIPForLegacyField for %q to contain %q but got: %q", tc.in, tc.legacyErr, errs[0].Detail) + } + } + + errs = IsValidIPForLegacyField(field.NewPath(""), tc.in, true) + if tc.legacyStrictErr == "" { + if len(errs) != 0 { + t.Errorf("expected %q to be valid according to IsValidIPForLegacyField with strict validation, but got: %v", tc.in, errs) + } + } else { + if len(errs) != 1 { + t.Errorf("expected %q to have 1 error from IsValidIPForLegacyField with strict validation, but got: %v", tc.in, errs) + } else if !strings.Contains(errs[0].Detail, tc.legacyStrictErr) { + t.Errorf("expected error from IsValidIPForLegacyField with strict validation for %q to contain %q but got: %q", tc.in, tc.legacyStrictErr, errs[0].Detail) + } + } }) } } @@ -221,7 +303,10 @@ func TestIsValidCIDR(t *testing.T) { for _, tc := range []struct { name string in string - err string + + err string + legacyErr string + legacyStrictErr string }{ // GOOD VALUES { @@ -249,77 +334,126 @@ func TestIsValidCIDR(t *testing.T) { in: "::1/128", }, - // GOOD, THOUGH NON-CANONICAL, VALUES + // NON-CANONICAL VALUES { name: "ipv6, extra 0s (non-canonical)", in: "2a00:79e0:2:0::/64", + + err: `must be in canonical form ("2a00:79e0:2::/64")`, }, { name: "ipv6, capital letters (non-canonical)", in: "2001:DB8::/64", + + err: `must be in canonical form ("2001:db8::/64")`, }, - // BAD VALUES WE CURRENTLY CONSIDER GOOD + // GOOD WITH LEGACY VALIDATION, BAD WITH STRICT VALIDATION { name: "ipv4 with leading 0s", in: "1.1.01.0/24", + + err: "must not have leading 0s in IP", + legacyErr: "", + legacyStrictErr: "must not have leading 0s in IP", }, { name: "ipv4-in-ipv6 with ipv4-sized prefix", in: "::ffff:1.1.1.0/24", + + err: "must not have an IPv4-mapped IPv6 address", + legacyErr: "", + legacyStrictErr: "must not have an IPv4-mapped IPv6 address", }, { name: "ipv4-in-ipv6 with ipv6-sized prefix", in: "::ffff:1.1.1.0/120", + + err: "must not have an IPv4-mapped IPv6 address", + legacyErr: "", + legacyStrictErr: "must not have an IPv4-mapped IPv6 address", }, { - name: "ipv4 with bits past prefix", + name: "ipv4 ifaddr", in: "1.2.3.4/24", + + err: "must not have bits set beyond the prefix length", + legacyErr: "", + legacyStrictErr: "must not have bits set beyond the prefix length", }, { - name: "ipv6 with bits past prefix", + name: "ipv6 ifaddr", in: "2001:db8::1/64", + + err: "must not have bits set beyond the prefix length", + legacyErr: "", + legacyStrictErr: "must not have bits set beyond the prefix length", }, { name: "prefix length with leading 0s", in: "192.168.0.0/016", + + err: "must not have leading 0s", + legacyErr: "", + legacyStrictErr: "must not have leading 0s", }, // BAD VALUES { name: "empty string", in: "", - err: "must be a valid CIDR value", + + err: "must be a valid CIDR value", + legacyErr: "must be a valid CIDR value", + legacyStrictErr: "must be a valid CIDR value", }, { name: "junk", in: "aaaaaaa", - err: "must be a valid CIDR value", + + err: "must be a valid CIDR value", + legacyErr: "must be a valid CIDR value", + legacyStrictErr: "must be a valid CIDR value", }, { name: "IP address", in: "1.2.3.4", - err: "must be a valid CIDR value", + + err: "must be a valid CIDR value", + legacyErr: "must be a valid CIDR value", + legacyStrictErr: "must be a valid CIDR value", }, { name: "partial URL", in: "192.168.0.1/healthz", - err: "must be a valid CIDR value", + + err: "must be a valid CIDR value", + legacyErr: "must be a valid CIDR value", + legacyStrictErr: "must be a valid CIDR value", }, { name: "partial URL 2", in: "192.168.0.1/0/99", - err: "must be a valid CIDR value", + + err: "must be a valid CIDR value", + legacyErr: "must be a valid CIDR value", + legacyStrictErr: "must be a valid CIDR value", }, { name: "negative prefix length", in: "192.168.0.0/-16", - err: "must be a valid CIDR value", + + err: "must be a valid CIDR value", + legacyErr: "must be a valid CIDR value", + legacyStrictErr: "must be a valid CIDR value", }, { name: "prefix length with sign", in: "192.168.0.0/+16", - err: "must be a valid CIDR value", + + err: "must be a valid CIDR value", + legacyErr: "must be a valid CIDR value", + legacyStrictErr: "must be a valid CIDR value", }, } { t.Run(tc.name, func(t *testing.T) { @@ -335,6 +469,32 @@ func TestIsValidCIDR(t *testing.T) { t.Errorf("expected error for %q to contain %q but got: %q", tc.in, tc.err, errs[0].Detail) } } + + errs = IsValidCIDRForLegacyField(field.NewPath(""), tc.in, false) + if tc.legacyErr == "" { + if len(errs) != 0 { + t.Errorf("expected %q to be valid according to IsValidCIDRForLegacyField but got: %v", tc.in, errs) + } + } else { + if len(errs) != 1 { + t.Errorf("expected %q to have 1 error from IsValidCIDRForLegacyField but got: %v", tc.in, errs) + } else if !strings.Contains(errs[0].Detail, tc.legacyErr) { + t.Errorf("expected error for %q from IsValidCIDRForLegacyField to contain %q but got: %q", tc.in, tc.legacyErr, errs[0].Detail) + } + } + + errs = IsValidCIDRForLegacyField(field.NewPath(""), tc.in, true) + if tc.legacyStrictErr == "" { + if len(errs) != 0 { + t.Errorf("expected %q to be valid according to IsValidCIDRForLegacyField with strict validation but got: %v", tc.in, errs) + } + } else { + if len(errs) != 1 { + t.Errorf("expected %q to have 1 error from IsValidCIDRForLegacyField with strict validation but got: %v", tc.in, errs) + } else if !strings.Contains(errs[0].Detail, tc.legacyStrictErr) { + t.Errorf("expected error for %q from IsValidCIDRForLegacyField with strict validation to contain %q but got: %q", tc.in, tc.legacyStrictErr, errs[0].Detail) + } + } }) } } diff --git a/test/featuregates_linter/test_data/versioned_feature_list.yaml b/test/featuregates_linter/test_data/versioned_feature_list.yaml index 4cb06108f66..44064f563f0 100644 --- a/test/featuregates_linter/test_data/versioned_feature_list.yaml +++ b/test/featuregates_linter/test_data/versioned_feature_list.yaml @@ -1376,6 +1376,12 @@ lockToDefault: true preRelease: GA version: "1.32" +- name: StrictIPCIDRValidation + versionedSpecs: + - default: false + lockToDefault: false + preRelease: Alpha + version: "1.33" - name: StructuredAuthenticationConfiguration versionedSpecs: - default: false From ad22c0d4954a760c0d0c1ffebb3de6c05c66ea4f Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 28 Dec 2023 11:30:45 -0500 Subject: [PATCH 6/6] Fix IP/CIDR validation to allow updates to existing invalid objects Ignore pre-existing bad IP/CIDR values in: - pod.spec.podIP(s) - pod.spec.hostIP(s) - service.spec.externalIPs - service.spec.clusterIP(s) - service.spec.loadBalancerSourceRanges (and corresponding annotation) - service.status.loadBalancer.ingress[].ip - endpoints.subsets - endpointslice.endpoints - networkpolicy.spec.{ingress[].from[],egress[].to[]}.ipBlock - ingress.status.loadBalancer.ingress[].ip In the Endpoints and EndpointSlice case, if *any* endpoint IP is changed, then the entire object must be valid; invalid IPs are only allowed to remain in place for updates that don't change any IPs. (e.g., changing the labels or annotations). In most of the other cases, when the invalid IP is part of an array, it can be moved around within the array without triggering revalidation. --- pkg/apis/core/validation/validation.go | 136 +++++-- pkg/apis/core/validation/validation_test.go | 341 ++++++++++++++++-- pkg/apis/discovery/validation/validation.go | 22 +- .../discovery/validation/validation_test.go | 38 +- pkg/apis/networking/validation/validation.go | 55 ++- .../networking/validation/validation_test.go | 132 ++++++- pkg/registry/core/service/storage/alloc.go | 4 +- .../apimachinery/pkg/util/validation/ip.go | 20 +- .../pkg/util/validation/ip_test.go | 44 ++- 9 files changed, 693 insertions(+), 99 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 68166359257..116cbf85455 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3790,7 +3790,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 { - allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Child("nameservers").Index(i), ns)...) + allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Child("nameservers").Index(i), ns, nil)...) } // Validate searches. if len(dnsConfig.Searches) > MaxDNSSearchPaths { @@ -3966,7 +3966,7 @@ func validateOnlyDeletedSchedulingGates(newGates, oldGates []core.PodSchedulingG func ValidateHostAliases(hostAliases []core.HostAlias, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for i, hostAlias := range hostAliases { - allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Index(i).Child("ip"), hostAlias.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Index(i).Child("ip"), hostAlias.IP, nil)...) for j, hostname := range hostAlias.Hostnames { allErrs = append(allErrs, ValidateDNS1123Subdomain(hostname, fldPath.Index(i).Child("hostnames").Index(j))...) } @@ -4108,14 +4108,21 @@ func validatePodMetadataAndSpec(pod *core.Pod, opts PodValidationOptions) field. } // validatePodIPs validates IPs in pod status -func validatePodIPs(pod *core.Pod) field.ErrorList { +func validatePodIPs(pod, oldPod *core.Pod) field.ErrorList { allErrs := field.ErrorList{} podIPsField := field.NewPath("status", "podIPs") - // all PodIPs must be valid IPs + // all new PodIPs must be valid IPs, but existing invalid ones can be kept. + var existingPodIPs []string + if oldPod != nil { + existingPodIPs = make([]string, len(oldPod.Status.PodIPs)) + for i, podIP := range oldPod.Status.PodIPs { + existingPodIPs[i] = podIP.IP + } + } for i, podIP := range pod.Status.PodIPs { - allErrs = append(allErrs, IsValidIPForLegacyField(podIPsField.Index(i), podIP.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(podIPsField.Index(i), podIP.IP, existingPodIPs)...) } // if we have more than one Pod.PodIP then we must have a dual-stack pair @@ -4140,7 +4147,7 @@ func validatePodIPs(pod *core.Pod) field.ErrorList { } // validateHostIPs validates IPs in pod status -func validateHostIPs(pod *core.Pod) field.ErrorList { +func validateHostIPs(pod, oldPod *core.Pod) field.ErrorList { allErrs := field.ErrorList{} if len(pod.Status.HostIPs) == 0 { @@ -4154,9 +4161,16 @@ func validateHostIPs(pod *core.Pod) field.ErrorList { allErrs = append(allErrs, field.Invalid(hostIPsField.Index(0).Child("ip"), pod.Status.HostIPs[0].IP, "must be equal to `hostIP`")) } - // all HostIPs must be valid IPs + // all new HostIPs must be valid IPs, but existing invalid ones can be kept. + var existingHostIPs []string + if oldPod != nil { + existingHostIPs = make([]string, len(oldPod.Status.HostIPs)) + for i, hostIP := range oldPod.Status.HostIPs { + existingHostIPs[i] = hostIP.IP + } + } for i, hostIP := range pod.Status.HostIPs { - allErrs = append(allErrs, IsValidIPForLegacyField(hostIPsField.Index(i), hostIP.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(hostIPsField.Index(i), hostIP.IP, existingHostIPs)...) } // if we have more than one Pod.HostIP then we must have a dual-stack pair @@ -5462,11 +5476,11 @@ func ValidatePodStatusUpdate(newPod, oldPod *core.Pod, opts PodValidationOptions allErrs = append(allErrs, ValidateContainerStateTransition(newPod.Status.EphemeralContainerStatuses, oldPod.Status.EphemeralContainerStatuses, fldPath.Child("ephemeralContainerStatuses"), core.RestartPolicyNever)...) allErrs = append(allErrs, validatePodResourceClaimStatuses(newPod.Status.ResourceClaimStatuses, newPod.Spec.ResourceClaims, fldPath.Child("resourceClaimStatuses"))...) - if newIPErrs := validatePodIPs(newPod); len(newIPErrs) > 0 { + if newIPErrs := validatePodIPs(newPod, oldPod); len(newIPErrs) > 0 { allErrs = append(allErrs, newIPErrs...) } - if newIPErrs := validateHostIPs(newPod); len(newIPErrs) > 0 { + if newIPErrs := validateHostIPs(newPod, oldPod); len(newIPErrs) > 0 { allErrs = append(allErrs, newIPErrs...) } @@ -5860,7 +5874,7 @@ var supportedServiceIPFamilyPolicy = sets.New( core.IPFamilyPolicyRequireDualStack) // ValidateService tests if required fields/annotations of a Service are valid. -func ValidateService(service *core.Service) field.ErrorList { +func ValidateService(service, oldService *core.Service) field.ErrorList { metaPath := field.NewPath("metadata") allErrs := ValidateObjectMeta(&service.ObjectMeta, true, ValidateServiceName, metaPath) @@ -5935,12 +5949,19 @@ func ValidateService(service *core.Service) field.ErrorList { } // dualstack <-> ClusterIPs <-> ipfamilies - allErrs = append(allErrs, ValidateServiceClusterIPsRelatedFields(service)...) + allErrs = append(allErrs, ValidateServiceClusterIPsRelatedFields(service, oldService)...) + // All new ExternalIPs must be valid and "non-special". (Existing ExternalIPs may + // have been validated against older rules, but if we allowed them before we can't + // reject them now.) ipPath := specPath.Child("externalIPs") + var existingExternalIPs []string + if oldService != nil { + existingExternalIPs = oldService.Spec.ExternalIPs // +k8s:verify-mutation:reason=clone + } for i, ip := range service.Spec.ExternalIPs { idxPath := ipPath.Index(i) - if errs := IsValidIPForLegacyField(idxPath, ip); len(errs) != 0 { + if errs := IsValidIPForLegacyField(idxPath, ip, existingExternalIPs); len(errs) != 0 { allErrs = append(allErrs, errs...) } else { // For historical reasons, this uses ValidateEndpointIP even @@ -5997,18 +6018,27 @@ func ValidateService(service *core.Service) field.ErrorList { ports[key] = true } - // Validate SourceRanges field or annotation. + // Validate SourceRanges field or annotation. Existing invalid CIDR values do not + // need to be fixed. Note that even with the tighter CIDR validation we still + // allow excess whitespace, because that is effectively part of the API. if len(service.Spec.LoadBalancerSourceRanges) > 0 { fieldPath := specPath.Child("LoadBalancerSourceRanges") if service.Spec.Type != core.ServiceTypeLoadBalancer { allErrs = append(allErrs, field.Forbidden(fieldPath, "may only be used when `type` is 'LoadBalancer'")) } + var existingSourceRanges []string + if oldService != nil { + existingSourceRanges = make([]string, len(oldService.Spec.LoadBalancerSourceRanges)) + for i, value := range oldService.Spec.LoadBalancerSourceRanges { + existingSourceRanges[i] = strings.TrimSpace(value) + } + } for idx, value := range service.Spec.LoadBalancerSourceRanges { // Note: due to a historical accident around transition from the // annotation value, these values are allowed to be space-padded. value = strings.TrimSpace(value) - allErrs = append(allErrs, IsValidCIDRForLegacyField(fieldPath.Index(idx), value)...) + allErrs = append(allErrs, IsValidCIDRForLegacyField(fieldPath.Index(idx), value, existingSourceRanges)...) } } else if val, annotationSet := service.Annotations[core.AnnotationLoadBalancerSourceRangesKey]; annotationSet { fieldPath := field.NewPath("metadata", "annotations").Key(core.AnnotationLoadBalancerSourceRangesKey) @@ -6016,12 +6046,14 @@ func ValidateService(service *core.Service) field.ErrorList { allErrs = append(allErrs, field.Forbidden(fieldPath, "may only be used when `type` is 'LoadBalancer'")) } - val = strings.TrimSpace(val) - if val != "" { - cidrs := strings.Split(val, ",") - for _, value := range cidrs { - value = strings.TrimSpace(value) - allErrs = append(allErrs, IsValidCIDRForLegacyField(fieldPath, value)...) + if oldService == nil || oldService.Annotations[core.AnnotationLoadBalancerSourceRangesKey] != val { + val = strings.TrimSpace(val) + if val != "" { + cidrs := strings.Split(val, ",") + for _, value := range cidrs { + value = strings.TrimSpace(value) + allErrs = append(allErrs, IsValidCIDRForLegacyField(fieldPath, value, nil)...) + } } } } @@ -6181,7 +6213,7 @@ func validateServiceTrafficDistribution(service *core.Service) field.ErrorList { // ValidateServiceCreate validates Services as they are created. func ValidateServiceCreate(service *core.Service) field.ErrorList { - return ValidateService(service) + return ValidateService(service, nil) } // ValidateServiceUpdate tests if required fields in the service are set during an update @@ -6204,13 +6236,13 @@ func ValidateServiceUpdate(service, oldService *core.Service) field.ErrorList { allErrs = append(allErrs, validateServiceExternalTrafficFieldsUpdate(oldService, service)...) - return append(allErrs, ValidateService(service)...) + return append(allErrs, ValidateService(service, oldService)...) } // ValidateServiceStatusUpdate tests if required fields in the Service are set when updating status. func ValidateServiceStatusUpdate(service, oldService *core.Service) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&service.ObjectMeta, &oldService.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateLoadBalancerStatus(&service.Status.LoadBalancer, field.NewPath("status", "loadBalancer"), &service.Spec)...) + allErrs = append(allErrs, ValidateLoadBalancerStatus(&service.Status.LoadBalancer, &oldService.Status.LoadBalancer, field.NewPath("status", "loadBalancer"), &service.Spec)...) return allErrs } @@ -6405,7 +6437,7 @@ func ValidateNode(node *core.Node) field.ErrorList { // all PodCIDRs should be valid ones for idx, value := range node.Spec.PodCIDRs { - allErrs = append(allErrs, IsValidCIDRForLegacyField(podCIDRsField.Index(idx), value)...) + allErrs = append(allErrs, IsValidCIDRForLegacyField(podCIDRsField.Index(idx), value, nil)...) } // if more than PodCIDR then it must be a dual-stack pair @@ -7432,16 +7464,28 @@ func ValidateNamespaceFinalizeUpdate(newNamespace, oldNamespace *core.Namespace) } // ValidateEndpoints validates Endpoints on create and update. -func ValidateEndpoints(endpoints *core.Endpoints) field.ErrorList { +func ValidateEndpoints(endpoints, oldEndpoints *core.Endpoints) field.ErrorList { allErrs := ValidateObjectMeta(&endpoints.ObjectMeta, true, ValidateEndpointsName, field.NewPath("metadata")) allErrs = append(allErrs, ValidateEndpointsSpecificAnnotations(endpoints.Annotations, field.NewPath("annotations"))...) - allErrs = append(allErrs, validateEndpointSubsets(endpoints.Subsets, field.NewPath("subsets"))...) + + subsetErrs := validateEndpointSubsets(endpoints.Subsets, field.NewPath("subsets")) + if len(subsetErrs) != 0 { + // If this is an update, and Subsets was unchanged, then ignore the + // validation errors, since apparently older versions of Kubernetes + // considered the data valid. (We only check this after getting a + // validation error since Endpoints may be large and DeepEqual is slow.) + if oldEndpoints != nil && apiequality.Semantic.DeepEqual(oldEndpoints.Subsets, endpoints.Subsets) { + subsetErrs = nil + } + } + allErrs = append(allErrs, subsetErrs...) + return allErrs } // ValidateEndpointsCreate validates Endpoints on create. func ValidateEndpointsCreate(endpoints *core.Endpoints) field.ErrorList { - return ValidateEndpoints(endpoints) + return ValidateEndpoints(endpoints, nil) } // ValidateEndpointsUpdate validates Endpoints on update. NodeName changes are @@ -7450,7 +7494,7 @@ func ValidateEndpointsCreate(endpoints *core.Endpoints) field.ErrorList { // happens. func ValidateEndpointsUpdate(newEndpoints, oldEndpoints *core.Endpoints) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&newEndpoints.ObjectMeta, &oldEndpoints.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateEndpoints(newEndpoints)...) + allErrs = append(allErrs, ValidateEndpoints(newEndpoints, oldEndpoints)...) return allErrs } @@ -7481,7 +7525,7 @@ func validateEndpointSubsets(subsets []core.EndpointSubset, fldPath *field.Path) func validateEndpointAddress(address *core.EndpointAddress, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Child("ip"), address.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Child("ip"), address.IP, nil)...) if len(address.Hostname) > 0 { allErrs = append(allErrs, ValidateDNS1123Label(address.Hostname, fldPath.Child("hostname"))...) } @@ -7844,16 +7888,25 @@ var ( ) // ValidateLoadBalancerStatus validates required fields on a LoadBalancerStatus -func ValidateLoadBalancerStatus(status *core.LoadBalancerStatus, fldPath *field.Path, spec *core.ServiceSpec) field.ErrorList { +func ValidateLoadBalancerStatus(status, oldStatus *core.LoadBalancerStatus, fldPath *field.Path, spec *core.ServiceSpec) field.ErrorList { allErrs := field.ErrorList{} ingrPath := fldPath.Child("ingress") if !utilfeature.DefaultFeatureGate.Enabled(features.AllowServiceLBStatusOnNonLB) && spec.Type != core.ServiceTypeLoadBalancer && len(status.Ingress) != 0 { allErrs = append(allErrs, field.Forbidden(ingrPath, "may only be used when `spec.type` is 'LoadBalancer'")) } else { + var existingIngressIPs []string + if oldStatus != nil { + existingIngressIPs = make([]string, 0, len(oldStatus.Ingress)) + for _, ingress := range oldStatus.Ingress { + if len(ingress.IP) > 0 { + existingIngressIPs = append(existingIngressIPs, ingress.IP) + } + } + } for i, ingress := range status.Ingress { idxPath := ingrPath.Index(i) if len(ingress.IP) > 0 { - allErrs = append(allErrs, IsValidIPForLegacyField(idxPath.Child("ip"), ingress.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(idxPath.Child("ip"), ingress.IP, existingIngressIPs)...) } if utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) && ingress.IPMode == nil { @@ -8120,7 +8173,7 @@ func validateLabelKeys(fldPath *field.Path, labelKeys []string, labelSelector *m // ValidateServiceClusterIPsRelatedFields validates .spec.ClusterIPs,, // .spec.IPFamilies, .spec.ipFamilyPolicy. This is exported because it is used // during IP init and allocation. -func ValidateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorList { +func ValidateServiceClusterIPsRelatedFields(service, oldService *core.Service) field.ErrorList { // ClusterIP, ClusterIPs, IPFamilyPolicy and IPFamilies are validated prior (all must be unset) for ExternalName service if service.Spec.Type == core.ServiceTypeExternalName { return field.ErrorList{} @@ -8174,6 +8227,11 @@ func ValidateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorLi } } + var existingClusterIPs []string + if oldService != nil { + existingClusterIPs = oldService.Spec.ClusterIPs // +k8s:verify-mutation:reason=clone + } + // clusterIPs stand alone validation // valid ips with None and empty string handling // duplication check is done as part of DualStackvalidation below @@ -8187,8 +8245,8 @@ func ValidateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorLi continue } - // is it valid ip? - errorMessages := IsValidIPForLegacyField(clusterIPsField.Index(i), clusterIP) + // is it a valid ip? (or was it at least previously considered valid?) + errorMessages := IsValidIPForLegacyField(clusterIPsField.Index(i), clusterIP, existingClusterIPs) hasInvalidIPs = (len(errorMessages) != 0) || hasInvalidIPs allErrs = append(allErrs, errorMessages...) } @@ -8707,13 +8765,13 @@ func isRestartableInitContainer(initContainer *core.Container) bool { // IsValidIPForLegacyField is a wrapper around validation.IsValidIPForLegacyField that // handles setting strictValidation correctly. This is only for fields that use legacy IP // address validation; use validation.IsValidIP for new fields. -func IsValidIPForLegacyField(fldPath *field.Path, value string) field.ErrorList { - return validation.IsValidIPForLegacyField(fldPath, value, utilfeature.DefaultFeatureGate.Enabled(features.StrictIPCIDRValidation)) +func IsValidIPForLegacyField(fldPath *field.Path, value string, validOldIPs []string) field.ErrorList { + return validation.IsValidIPForLegacyField(fldPath, value, utilfeature.DefaultFeatureGate.Enabled(features.StrictIPCIDRValidation), validOldIPs) } // IsValidCIDRForLegacyField is a wrapper around validation.IsValidCIDRForLegacyField that // handles setting strictValidation correctly. This is only for fields that use legacy CIDR // value validation; use validation.IsValidCIDR for new fields. -func IsValidCIDRForLegacyField(fldPath *field.Path, value string) field.ErrorList { - return validation.IsValidCIDRForLegacyField(fldPath, value, utilfeature.DefaultFeatureGate.Enabled(features.StrictIPCIDRValidation)) +func IsValidCIDRForLegacyField(fldPath *field.Path, value string, validOldCIDRs []string) field.ErrorList { + return validation.IsValidCIDRForLegacyField(fldPath, value, utilfeature.DefaultFeatureGate.Enabled(features.StrictIPCIDRValidation), validOldCIDRs) } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 13226465734..d3f199bb9a5 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -18892,10 +18892,212 @@ func TestValidateServiceUpdate(t *testing.T) { }, numErrs: 1, }, + + // IP validation ratcheting tests. Note: this is tested for + // LoadBalancerStatus in TestValidateLoadBalancerStatus. + { + name: "pre-existing invalid clusterIP ignored when updating other fields", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ClusterIP = "1.2.3.04" + oldSvc.Spec.ClusterIPs = []string{"1.2.3.04"} + newSvc.Spec.ClusterIP = "1.2.3.04" + newSvc.Spec.ClusterIPs = []string{"1.2.3.04"} + newSvc.Labels["foo"] = "bar" + }, + numErrs: 0, + }, { + name: "pre-existing invalid clusterIP ignored when adding clusterIPs", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ClusterIP = "1.2.3.04" + oldSvc.Spec.ClusterIPs = []string{"1.2.3.04"} + oldSvc.Spec.IPFamilyPolicy = &singleStack + oldSvc.Spec.IPFamilies = []core.IPFamily{core.IPv4Protocol} + + newSvc.Spec.ClusterIP = "1.2.3.04" + newSvc.Spec.ClusterIPs = []string{"1.2.3.04", "2001:db8::4"} + newSvc.Spec.IPFamilyPolicy = &requireDualStack + newSvc.Spec.IPFamilies = []core.IPFamily{core.IPv4Protocol, core.IPv6Protocol} + }, + numErrs: 0, + }, { + name: "pre-existing invalid clusterIP ignored when removing clusterIPs", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ClusterIP = "1.2.3.04" + oldSvc.Spec.ClusterIPs = []string{"1.2.3.04", "2001:db::4"} + oldSvc.Spec.IPFamilyPolicy = &requireDualStack + oldSvc.Spec.IPFamilies = []core.IPFamily{core.IPv4Protocol, core.IPv6Protocol} + + newSvc.Spec.ClusterIP = "1.2.3.04" + newSvc.Spec.ClusterIPs = []string{"1.2.3.04"} + newSvc.Spec.IPFamilyPolicy = &singleStack + newSvc.Spec.IPFamilies = []core.IPFamily{core.IPv4Protocol} + }, + numErrs: 0, + }, { + name: "pre-existing invalid externalIP ignored", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + oldSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + newSvc.Labels["foo"] = "bar" + }, + numErrs: 0, + }, { + name: "pre-existing invalid externalIP ignored when adding externalIPs", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + oldSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"5.6.7.8", "1.2.3.04"} + }, + numErrs: 0, + }, { + name: "pre-existing invalid externalIP ignored when removing externalIPs", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + oldSvc.Spec.ExternalIPs = []string{"5.6.7.8", "1.2.3.04"} + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + }, + numErrs: 0, + }, { + name: "can fix invalid externalIP", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + oldSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"1.2.3.4"} + }, + numErrs: 0, + }, { + name: "can't add invalid externalIP", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + }, + numErrs: 1, + }, { + name: "pre-existing invalid loadBalancerSourceRanges ignored when updating other fields", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + newSvc.Labels["foo"] = "bar" + }, + numErrs: 0, + }, { + name: "pre-existing invalid loadBalancerSourceRanges ignored when adding source ranges", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.LoadBalancerSourceRanges = []string{"1.2.3.0/24", "010.0.0.0/8"} + }, + numErrs: 0, + }, { + name: "pre-existing invalid loadBalancerSourceRanges ignored when removing source ranges", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"1.2.3.0/24", "010.0.0.0/8"} + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + }, + numErrs: 0, + }, { + name: "can fix invalid loadBalancerSourceRanges", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.LoadBalancerSourceRanges = []string{"10.0.0.0/8"} + }, + numErrs: 0, + }, { + name: "can't add invalid loadBalancerSourceRanges", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + }, + numErrs: 1, + }, { + name: "pre-existing invalid source ranges ignored when updating other fields", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8" + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8" + newSvc.Labels["foo"] = "bar" + }, + numErrs: 0, + }, { + name: "can fix invalid source ranges annotation", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8" + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "10.0.0.0/8" + }, + numErrs: 0, + }, { + name: "can't modify pre-existing invalid source ranges annotation without fixing it", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8" + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8, 1.2.3.0/24" + }, + numErrs: 1, + }, { + name: "can't add invalid source ranges annotation", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8, 1.2.3.0/24" + }, + numErrs: 1, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) + oldSvc := makeValidService() newSvc := makeValidService() tc.tweakSvc(&oldSvc, &newSvc) @@ -22089,7 +22291,7 @@ func TestEndpointAddressNodeNameUpdateRestrictions(t *testing.T) { updatedEndpoint := newNodeNameEndpoint("kubernetes-changed-nodename") // Check that NodeName can be changed during update, this is to accommodate the case where nodeIP or PodCIDR is reused. // The same ip will now have a different nodeName. - errList := ValidateEndpoints(updatedEndpoint) + errList := ValidateEndpoints(updatedEndpoint, oldEndpoint) errList = append(errList, ValidateEndpointsUpdate(updatedEndpoint, oldEndpoint)...) if len(errList) != 0 { t.Error("Endpoint should allow changing of Subset.Addresses.NodeName on update") @@ -22099,7 +22301,7 @@ func TestEndpointAddressNodeNameUpdateRestrictions(t *testing.T) { func TestEndpointAddressNodeNameInvalidDNSSubdomain(t *testing.T) { // Check NodeName DNS validation endpoint := newNodeNameEndpoint("illegal*.nodename") - errList := ValidateEndpoints(endpoint) + errList := ValidateEndpoints(endpoint, nil) if len(errList) == 0 { t.Error("Endpoint should reject invalid NodeName") } @@ -22107,7 +22309,7 @@ func TestEndpointAddressNodeNameInvalidDNSSubdomain(t *testing.T) { func TestEndpointAddressNodeNameCanBeAnIPAddress(t *testing.T) { endpoint := newNodeNameEndpoint("10.10.1.1") - errList := ValidateEndpoints(endpoint) + errList := ValidateEndpoints(endpoint, nil) if len(errList) != 0 { t.Error("Endpoint should accept a NodeName that is an IP address") } @@ -23033,11 +23235,13 @@ func makePod(podName string, podNamespace string, podIPs []core.PodIP) core.Pod func TestPodIPsValidation(t *testing.T) { // We test updating every pod in testCases to every other pod in testCases. // expectError is true if we expect an error when updating *to* that pod. + // allowNoOpUpdate is true if we expect a no-op update to succeed. testCases := []struct { - pod core.Pod - legacyIPs bool - expectError bool + pod core.Pod + legacyIPs bool + expectError bool + allowNoOpUpdate bool }{ { expectError: false, @@ -23064,11 +23268,13 @@ func TestPodIPsValidation(t *testing.T) { }, /* failure cases start here */ { - expectError: true, - pod: makePod("invalid-pod-ip", "ns", []core.PodIP{{IP: "this-is-not-an-ip"}}), + expectError: true, + allowNoOpUpdate: true, + pod: makePod("invalid-pod-ip", "ns", []core.PodIP{{IP: "1.1.1.01"}}), }, { - expectError: true, - pod: makePod("legacy-pod-ip-with-strict-validation", "ns", []core.PodIP{{IP: "001.002.003.004"}}), + expectError: true, + allowNoOpUpdate: true, + pod: makePod("legacy-pod-ip-with-strict-validation", "ns", []core.PodIP{{IP: "001.002.003.004"}}), }, { expectError: true, pod: makePod("dualstack-same-ip-family-6", "ns", []core.PodIP{{IP: "::1"}, {IP: "::2"}}), @@ -23092,10 +23298,14 @@ func TestPodIPsValidation(t *testing.T) { }, } - for _, testCase := range testCases { + for i, testCase := range testCases { t.Run(testCase.pod.Name, func(t *testing.T) { featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !testCase.legacyIPs) - for _, oldTestCase := range testCases { + for j, oldTestCase := range testCases { + if oldTestCase.legacyIPs && !testCase.legacyIPs { + continue + } + newPod := testCase.pod.DeepCopy() newPod.ResourceVersion = "1" @@ -23105,7 +23315,11 @@ func TestPodIPsValidation(t *testing.T) { errs := ValidatePodStatusUpdate(newPod, oldPod, PodValidationOptions{}) - if len(errs) == 0 && testCase.expectError { + expectError := testCase.expectError + if testCase.allowNoOpUpdate && i == j { + expectError = false + } + if len(errs) == 0 && expectError { t.Fatalf("expected failure updating from %s, but there were none", oldTestCase.pod.Name) } if len(errs) != 0 && !testCase.expectError { @@ -23142,11 +23356,13 @@ func makePodWithHostIPs(podName string, podNamespace string, hostIPs []core.Host func TestHostIPsValidation(t *testing.T) { // We test updating every pod in testCases to every other pod in testCases. // expectError is true if we expect an error when updating *to* that pod. + // allowNoOpUpdate is true if we expect a no-op update to succeed. testCases := []struct { - pod core.Pod - legacyIPs bool - expectError bool + pod core.Pod + legacyIPs bool + expectError bool + allowNoOpUpdate bool }{ { expectError: false, @@ -23179,12 +23395,14 @@ func TestHostIPsValidation(t *testing.T) { }, /* failure cases start here */ { - expectError: true, - pod: makePodWithHostIPs("invalid-host-ip", "ns", []core.HostIP{{IP: "this-is-not-an-ip"}}), + expectError: true, + allowNoOpUpdate: true, + pod: makePodWithHostIPs("invalid-host-ip", "ns", []core.HostIP{{IP: "this-is-not-an-ip"}}), }, { - expectError: true, - pod: makePodWithHostIPs("invalid-legacy-host-ip-with-strict-validation", "ns", []core.HostIP{{IP: "001.002.003.004"}}), + expectError: true, + allowNoOpUpdate: true, + pod: makePodWithHostIPs("invalid-legacy-host-ip-with-strict-validation", "ns", []core.HostIP{{IP: "001.002.003.004"}}), }, { expectError: true, @@ -23213,10 +23431,14 @@ func TestHostIPsValidation(t *testing.T) { }, } - for _, testCase := range testCases { + for i, testCase := range testCases { t.Run(testCase.pod.Name, func(t *testing.T) { featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !testCase.legacyIPs) - for _, oldTestCase := range testCases { + for j, oldTestCase := range testCases { + if oldTestCase.legacyIPs && !testCase.legacyIPs { + continue + } + newPod := testCase.pod.DeepCopy() newPod.ResourceVersion = "1" @@ -23226,7 +23448,11 @@ func TestHostIPsValidation(t *testing.T) { errs := ValidatePodStatusUpdate(newPod, oldPod, PodValidationOptions{}) - if len(errs) == 0 && testCase.expectError { + expectError := testCase.expectError + if testCase.allowNoOpUpdate && i == j { + expectError = false + } + if len(errs) == 0 && expectError { t.Fatalf("expected failure updating from %s, but there were none", oldTestCase.pod.Name) } if len(errs) != 0 && !testCase.expectError { @@ -24782,13 +25008,14 @@ func TestValidateLoadBalancerStatus(t *testing.T) { ipModeDummy := core.LoadBalancerIPMode("dummy") testCases := []struct { - name string - ipModeEnabled bool - legacyIPs bool - nonLBAllowed bool - tweakLBStatus func(s *core.LoadBalancerStatus) - tweakSvcSpec func(s *core.ServiceSpec) - numErrs int + name string + ipModeEnabled bool + legacyIPs bool + nonLBAllowed bool + tweakOldLBStatus func(s *core.LoadBalancerStatus) + tweakLBStatus func(s *core.LoadBalancerStatus) + tweakSvcSpec func(s *core.ServiceSpec) + numErrs int }{ { name: "type is not LB", @@ -24892,6 +25119,54 @@ func TestValidateLoadBalancerStatus(t *testing.T) { }} }, numErrs: 1, + }, { + name: "invalid ingress IP ignored on update", + tweakOldLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }} + }, + tweakLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }} + }, + numErrs: 0, + }, { + name: "invalid ingress IP ignored when adding IP", + tweakOldLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }} + }, + tweakLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }, { + IP: "5.6.7.8", + IPMode: &ipModeVIP, + }} + }, + numErrs: 0, + }, { + name: "invalid ingress IP can be fixed", + tweakOldLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }} + }, + tweakLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.4", + IPMode: &ipModeVIP, + }} + }, + numErrs: 0, }, } for _, tc := range testCases { @@ -24905,13 +25180,17 @@ func TestValidateLoadBalancerStatus(t *testing.T) { } featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LoadBalancerIPMode, tc.ipModeEnabled) featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AllowServiceLBStatusOnNonLB, tc.nonLBAllowed) + oldStatus := core.LoadBalancerStatus{} + if tc.tweakOldLBStatus != nil { + tc.tweakOldLBStatus(&oldStatus) + } status := core.LoadBalancerStatus{} tc.tweakLBStatus(&status) spec := core.ServiceSpec{Type: core.ServiceTypeLoadBalancer} if tc.tweakSvcSpec != nil { tc.tweakSvcSpec(&spec) } - errs := ValidateLoadBalancerStatus(&status, field.NewPath("status"), &spec) + errs := ValidateLoadBalancerStatus(&status, &oldStatus, field.NewPath("status"), &spec) if len(errs) != tc.numErrs { t.Errorf("Unexpected error list for case %q(expected:%v got %v) - Errors:\n %v", tc.name, tc.numErrs, len(errs), errs.ToAggregate()) } diff --git a/pkg/apis/discovery/validation/validation.go b/pkg/apis/discovery/validation/validation.go index a50cec5b43c..3a09837a8b5 100644 --- a/pkg/apis/discovery/validation/validation.go +++ b/pkg/apis/discovery/validation/validation.go @@ -20,6 +20,7 @@ import ( "fmt" corev1 "k8s.io/api/core/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation" metavalidation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/util/sets" @@ -55,23 +56,34 @@ var ( var ValidateEndpointSliceName = apimachineryvalidation.NameIsDNSSubdomain // ValidateEndpointSlice validates an EndpointSlice. -func ValidateEndpointSlice(endpointSlice *discovery.EndpointSlice) field.ErrorList { +func ValidateEndpointSlice(endpointSlice, oldEndpointSlice *discovery.EndpointSlice) field.ErrorList { allErrs := apivalidation.ValidateObjectMeta(&endpointSlice.ObjectMeta, true, ValidateEndpointSliceName, field.NewPath("metadata")) allErrs = append(allErrs, validateAddressType(endpointSlice.AddressType)...) - allErrs = append(allErrs, validateEndpoints(endpointSlice.Endpoints, endpointSlice.AddressType, field.NewPath("endpoints"))...) allErrs = append(allErrs, validatePorts(endpointSlice.Ports, field.NewPath("ports"))...) + endpointsErrs := validateEndpoints(endpointSlice.Endpoints, endpointSlice.AddressType, field.NewPath("endpoints")) + if len(endpointsErrs) != 0 { + // If this is an update, and Endpoints was unchanged, then ignore the + // validation errors, since apparently older versions of Kubernetes + // considered the data valid. (We only check this after getting a + // validation error since Endpoints may be large and DeepEqual is slow.) + if oldEndpointSlice != nil && apiequality.Semantic.DeepEqual(oldEndpointSlice.Endpoints, endpointSlice.Endpoints) { + endpointsErrs = nil + } + } + allErrs = append(allErrs, endpointsErrs...) + return allErrs } // ValidateEndpointSliceCreate validates an EndpointSlice when it is created. func ValidateEndpointSliceCreate(endpointSlice *discovery.EndpointSlice) field.ErrorList { - return ValidateEndpointSlice(endpointSlice) + return ValidateEndpointSlice(endpointSlice, nil) } // ValidateEndpointSliceUpdate validates an EndpointSlice when it is updated. func ValidateEndpointSliceUpdate(newEndpointSlice, oldEndpointSlice *discovery.EndpointSlice) field.ErrorList { - allErrs := ValidateEndpointSlice(newEndpointSlice) + allErrs := ValidateEndpointSlice(newEndpointSlice, oldEndpointSlice) allErrs = append(allErrs, apivalidation.ValidateImmutableField(newEndpointSlice.AddressType, oldEndpointSlice.AddressType, field.NewPath("addressType"))...) return allErrs @@ -100,7 +112,7 @@ func validateEndpoints(endpoints []discovery.Endpoint, addrType discovery.Addres // and do not get validated. switch addrType { case discovery.AddressTypeIPv4: - ipErrs := apivalidation.IsValidIPForLegacyField(addressPath.Index(i), address) + ipErrs := apivalidation.IsValidIPForLegacyField(addressPath.Index(i), address, nil) if len(ipErrs) > 0 { allErrs = append(allErrs, ipErrs...) } else { diff --git a/pkg/apis/discovery/validation/validation_test.go b/pkg/apis/discovery/validation/validation_test.go index 96830d51a9e..50a8bf2b69b 100644 --- a/pkg/apis/discovery/validation/validation_test.go +++ b/pkg/apis/discovery/validation/validation_test.go @@ -637,7 +637,7 @@ func TestValidateEndpointSlice(t *testing.T) { for name, testCase := range testCases { t.Run(name, func(t *testing.T) { featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !testCase.legacyIPs) - errs := ValidateEndpointSlice(testCase.endpointSlice) + errs := ValidateEndpointSlice(testCase.endpointSlice, nil) if len(errs) != testCase.expectedErrors { t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs) } @@ -737,6 +737,7 @@ func TestValidateEndpointSliceCreate(t *testing.T) { for name, testCase := range testCases { t.Run(name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) errs := ValidateEndpointSliceCreate(testCase.endpointSlice) if len(errs) != testCase.expectedErrors { t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs) @@ -765,6 +766,23 @@ func TestValidateEndpointSliceUpdate(t *testing.T) { }, expectedErrors: 0, }, + "unchanged IPs are not revalidated": { + oldEndpointSlice: &discovery.EndpointSlice{ + ObjectMeta: standardMeta, + AddressType: discovery.AddressTypeIPv4, + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"10.1.2.04"}, + }}, + }, + newEndpointSlice: &discovery.EndpointSlice{ + ObjectMeta: standardMeta, + AddressType: discovery.AddressTypeIPv4, + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"10.1.2.04"}, + }}, + }, + expectedErrors: 0, + }, // expected errors "invalid node name set": { @@ -823,10 +841,28 @@ func TestValidateEndpointSliceUpdate(t *testing.T) { }, expectedErrors: 1, }, + "changed IPs are revalidated": { + oldEndpointSlice: &discovery.EndpointSlice{ + ObjectMeta: standardMeta, + AddressType: discovery.AddressTypeIPv4, + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"10.1.2.3"}, + }}, + }, + newEndpointSlice: &discovery.EndpointSlice{ + ObjectMeta: standardMeta, + AddressType: discovery.AddressTypeIPv4, + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"10.1.2.03"}, + }}, + }, + expectedErrors: 1, + }, } for name, testCase := range testCases { t.Run(name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) errs := ValidateEndpointSliceUpdate(testCase.newEndpointSlice, testCase.oldEndpointSlice) if len(errs) != testCase.expectedErrors { t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs) diff --git a/pkg/apis/networking/validation/validation.go b/pkg/apis/networking/validation/validation.go index 8f00d0d8ffc..8bbad55ddc0 100644 --- a/pkg/apis/networking/validation/validation.go +++ b/pkg/apis/networking/validation/validation.go @@ -56,6 +56,7 @@ var ( type NetworkPolicyValidationOptions struct { AllowInvalidLabelValueInSelector bool + AllowCIDRsEvenIfInvalid []string } // ValidateNetworkPolicyName can be used to check whether the given networkpolicy @@ -118,7 +119,7 @@ func ValidateNetworkPolicyPeer(peer *networking.NetworkPolicyPeer, opts NetworkP } if peer.IPBlock != nil { numPeers++ - allErrs = append(allErrs, ValidateIPBlock(peer.IPBlock, peerPath.Child("ipBlock"))...) + allErrs = append(allErrs, ValidateIPBlock(peer.IPBlock, peerPath.Child("ipBlock"), opts)...) } if numPeers == 0 { @@ -206,20 +207,47 @@ func ValidationOptionsForNetworking(new, old *networking.NetworkPolicy) NetworkP // ValidateNetworkPolicyUpdate tests if an update to a NetworkPolicy is valid. func ValidateNetworkPolicyUpdate(update, old *networking.NetworkPolicy, opts NetworkPolicyValidationOptions) field.ErrorList { + // Record all existing CIDRs in the policy; see ValidateIPBlock. + var existingCIDRs []string + for _, ingress := range old.Spec.Ingress { + for _, peer := range ingress.From { + if peer.IPBlock != nil { + existingCIDRs = append(existingCIDRs, peer.IPBlock.CIDR) + existingCIDRs = append(existingCIDRs, peer.IPBlock.Except...) + } + } + } + for _, egress := range old.Spec.Egress { + for _, peer := range egress.To { + if peer.IPBlock != nil { + existingCIDRs = append(existingCIDRs, peer.IPBlock.CIDR) + existingCIDRs = append(existingCIDRs, peer.IPBlock.Except...) + } + } + } + opts.AllowCIDRsEvenIfInvalid = existingCIDRs + allErrs := field.ErrorList{} allErrs = append(allErrs, apivalidation.ValidateObjectMetaUpdate(&update.ObjectMeta, &old.ObjectMeta, field.NewPath("metadata"))...) allErrs = append(allErrs, ValidateNetworkPolicySpec(&update.Spec, opts, field.NewPath("spec"))...) return allErrs } -// ValidateIPBlock validates a cidr and the except fields of an IpBlock NetworkPolicyPeer -func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path) field.ErrorList { +// ValidateIPBlock validates a cidr and the except fields of an IPBlock NetworkPolicyPeer. +// +// If a pre-existing CIDR is invalid/insecure, but it is not being changed by this update, +// then we have to continue allowing it. But since the user may have changed the policy in +// arbitrary ways (adding/removing rules, adding/removing peers, adding/removing +// ipBlock.except values, etc), we can't reliably determine whether a CIDR value we see +// here is a new value or a pre-existing one. So we just allow any CIDR value that +// appeared in the old NetworkPolicy to be used here without revalidation. +func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path, opts NetworkPolicyValidationOptions) field.ErrorList { allErrs := field.ErrorList{} if ipb.CIDR == "" { allErrs = append(allErrs, field.Required(fldPath.Child("cidr"), "")) return allErrs } - allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(fldPath.Child("cidr"), ipb.CIDR)...) + allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(fldPath.Child("cidr"), ipb.CIDR, opts.AllowCIDRsEvenIfInvalid)...) _, cidrIPNet, err := netutils.ParseCIDRSloppy(ipb.CIDR) if err != nil { // Implies validation would have failed so we already added errors for it. @@ -228,7 +256,7 @@ func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path) field.ErrorLi for i, exceptCIDRStr := range ipb.Except { exceptPath := fldPath.Child("except").Index(i) - allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(exceptPath, exceptCIDRStr)...) + allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(exceptPath, exceptCIDRStr, opts.AllowCIDRsEvenIfInvalid)...) _, exceptCIDR, err := netutils.ParseCIDRSloppy(exceptCIDRStr) if err != nil { // Implies validation would have failed so we already added errors for it. @@ -346,17 +374,28 @@ func ValidateIngressSpec(spec *networking.IngressSpec, fldPath *field.Path, opts // ValidateIngressStatusUpdate tests if required fields in the Ingress are set when updating status. func ValidateIngressStatusUpdate(ingress, oldIngress *networking.Ingress) field.ErrorList { allErrs := apivalidation.ValidateObjectMetaUpdate(&ingress.ObjectMeta, &oldIngress.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateIngressLoadBalancerStatus(&ingress.Status.LoadBalancer, field.NewPath("status", "loadBalancer"))...) + allErrs = append(allErrs, ValidateIngressLoadBalancerStatus(&ingress.Status.LoadBalancer, &oldIngress.Status.LoadBalancer, field.NewPath("status", "loadBalancer"))...) return allErrs } // ValidateIngressLoadBalancerStatus validates required fields on an IngressLoadBalancerStatus -func ValidateIngressLoadBalancerStatus(status *networking.IngressLoadBalancerStatus, fldPath *field.Path) field.ErrorList { +func ValidateIngressLoadBalancerStatus(status, oldStatus *networking.IngressLoadBalancerStatus, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} + + var existingIngressIPs []string + if oldStatus != nil { + existingIngressIPs = make([]string, 0, len(oldStatus.Ingress)) + for _, ingress := range oldStatus.Ingress { + if len(ingress.IP) > 0 { + existingIngressIPs = append(existingIngressIPs, ingress.IP) + } + } + } + for i, ingress := range status.Ingress { idxPath := fldPath.Child("ingress").Index(i) if len(ingress.IP) > 0 { - allErrs = append(allErrs, apivalidation.IsValidIPForLegacyField(idxPath.Child("ip"), ingress.IP)...) + allErrs = append(allErrs, apivalidation.IsValidIPForLegacyField(idxPath.Child("ip"), ingress.IP, existingIngressIPs)...) } if len(ingress.Hostname) > 0 { for _, msg := range validation.IsDNS1123Subdomain(ingress.Hostname) { diff --git a/pkg/apis/networking/validation/validation_test.go b/pkg/apis/networking/validation/validation_test.go index 438d7f5bea7..7217964ed42 100644 --- a/pkg/apis/networking/validation/validation_test.go +++ b/pkg/apis/networking/validation/validation_test.go @@ -442,13 +442,95 @@ func TestValidateNetworkPolicyUpdate(t *testing.T) { }, }, }, + "fix pre-existing invalid CIDR": { + old: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.2.0/24", + "192.168.3.1/24", + }, + }, + }}, + }}, + }, + }, + update: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.2.0/24", + "192.168.3.0/24", + }, + }, + }}, + }}, + }, + }, + }, + "fail to fix pre-existing invalid CIDR": { + old: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.2.0/24", + "192.168.3.1/24", + }, + }, + }}, + }}, + }, + }, + update: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.1.0/24", + "192.168.2.0/24", + "192.168.3.1/24", + }, + }, + }}, + }}, + }, + }, + }, } for testName, successCase := range successCases { t.Run(testName, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) successCase.old.ObjectMeta.ResourceVersion = "1" successCase.update.ObjectMeta.ResourceVersion = "1" - if errs := ValidateNetworkPolicyUpdate(&successCase.update, &successCase.old, NetworkPolicyValidationOptions{false}); len(errs) != 0 { + if errs := ValidateNetworkPolicyUpdate(&successCase.update, &successCase.old, NetworkPolicyValidationOptions{}); len(errs) != 0 { t.Errorf("expected success, but got %v", errs) } }) @@ -471,13 +553,55 @@ func TestValidateNetworkPolicyUpdate(t *testing.T) { }, }, }, + "add new invalid CIDR": { + old: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.2.0/24", + "192.168.3.0/24", + }, + }, + }}, + }}, + }, + }, + update: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.1.1/24", + "192.168.2.0/24", + "192.168.3.0/24", + }, + }, + }}, + }}, + }, + }, + }, } for testName, errorCase := range errorCases { t.Run(testName, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) errorCase.old.ObjectMeta.ResourceVersion = "1" errorCase.update.ObjectMeta.ResourceVersion = "1" - if errs := ValidateNetworkPolicyUpdate(&errorCase.update, &errorCase.old, NetworkPolicyValidationOptions{false}); len(errs) == 0 { + if errs := ValidateNetworkPolicyUpdate(&errorCase.update, &errorCase.old, NetworkPolicyValidationOptions{}); len(errs) == 0 { t.Errorf("expected failure") } }) @@ -1865,6 +1989,10 @@ func TestValidateIngressStatusUpdate(t *testing.T) { newValue: legacyIP, legacyIPs: true, }, + "legacy IPs unchanged in update": { + oldValue: legacyIP, + newValue: legacyIP, + }, } for k, tc := range successCases { t.Run(k, func(t *testing.T) { diff --git a/pkg/registry/core/service/storage/alloc.go b/pkg/registry/core/service/storage/alloc.go index a69261fb91f..8285f08804e 100644 --- a/pkg/registry/core/service/storage/alloc.go +++ b/pkg/registry/core/service/storage/alloc.go @@ -151,9 +151,7 @@ func (al *Allocators) initIPFamilyFields(after After, before Before) error { // Do some loose pre-validation of the input. This makes it easier in the // rest of allocation code to not have to consider corner cases. - // TODO(thockin): when we tighten validation (e.g. to require IPs) we will - // need a "strict" and a "loose" form of this. - if el := validation.ValidateServiceClusterIPsRelatedFields(service); len(el) != 0 { + if el := validation.ValidateServiceClusterIPsRelatedFields(service, oldService); len(el) != 0 { return errors.NewInvalid(api.Kind("Service"), service.Name, el) } 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 9c836656ed7..6e947c74c9e 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go @@ -20,6 +20,7 @@ import ( "fmt" "net" "net/netip" + "slices" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/klog/v2" @@ -70,10 +71,17 @@ func parseIP(fldPath *field.Path, value string, strictValidation bool) (net.IP, // netip.ParseAddr both allow these, but there are no use cases for representing IPv4 // addresses as IPv4-mapped IPv6 addresses in Kubernetes.) // +// Alternatively, when validating an update to an existing field, you can pass a list of +// IP values from the old object that should be accepted if they appear in the new object +// even if they are not valid. +// // This function should only be used to validate the existing fields that were // historically validated in this way, and strictValidation should be true unless the // StrictIPCIDRValidation feature gate is disabled. Use IsValidIP for parsing new fields. -func IsValidIPForLegacyField(fldPath *field.Path, value string, strictValidation bool) field.ErrorList { +func IsValidIPForLegacyField(fldPath *field.Path, value string, strictValidation bool, validOldIPs []string) field.ErrorList { + if slices.Contains(validOldIPs, value) { + return nil + } _, allErrors := parseIP(fldPath, value, strictValidation) return allErrors.WithOrigin("format=ip-sloppy") } @@ -165,11 +173,19 @@ func parseCIDR(fldPath *field.Path, value string, strictValidation bool) (*net.I // // 3. The prefix length is allowed to have leading 0s. // +// Alternatively, when validating an update to an existing field, you can pass a list of +// CIDR values from the old object that should be accepted if they appear in the new +// object even if they are not valid. +// // This function should only be used to validate the existing fields that were // historically validated in this way, and strictValidation should be true unless the // StrictIPCIDRValidation feature gate is disabled. Use IsValidCIDR or // IsValidInterfaceAddress for parsing new fields. -func IsValidCIDRForLegacyField(fldPath *field.Path, value string, strictValidation bool) field.ErrorList { +func IsValidCIDRForLegacyField(fldPath *field.Path, value string, strictValidation bool, validOldCIDRs []string) field.ErrorList { + if slices.Contains(validOldCIDRs, value) { + return nil + } + _, allErrors := parseCIDR(fldPath, value, strictValidation) return allErrors } 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 fed7e73d3a9..caf5cf1fe51 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 @@ -25,7 +25,7 @@ import ( ) func TestIsValidIP(t *testing.T) { - for _, tc := range []struct { + testCases := []struct { name string in string @@ -202,7 +202,16 @@ func TestIsValidIP(t *testing.T) { legacyErr: "must be a valid IP address", legacyStrictErr: "must be a valid IP address", }, - } { + } + + var badIPs []string + for _, tc := range testCases { + if tc.legacyStrictErr != "" { + badIPs = append(badIPs, tc.in) + } + } + + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { errs := IsValidIP(field.NewPath(""), tc.in) if tc.err == "" { @@ -217,7 +226,7 @@ func TestIsValidIP(t *testing.T) { } } - errs = IsValidIPForLegacyField(field.NewPath(""), tc.in, false) + errs = IsValidIPForLegacyField(field.NewPath(""), tc.in, false, nil) if tc.legacyErr == "" { if len(errs) != 0 { t.Errorf("expected %q to be valid according to IsValidIPForLegacyField but got: %v", tc.in, errs) @@ -230,7 +239,7 @@ func TestIsValidIP(t *testing.T) { } } - errs = IsValidIPForLegacyField(field.NewPath(""), tc.in, true) + errs = IsValidIPForLegacyField(field.NewPath(""), tc.in, true, nil) if tc.legacyStrictErr == "" { if len(errs) != 0 { t.Errorf("expected %q to be valid according to IsValidIPForLegacyField with strict validation, but got: %v", tc.in, errs) @@ -242,6 +251,11 @@ func TestIsValidIP(t *testing.T) { t.Errorf("expected error from IsValidIPForLegacyField with strict validation for %q to contain %q but got: %q", tc.in, tc.legacyStrictErr, errs[0].Detail) } } + + errs = IsValidIPForLegacyField(field.NewPath(""), tc.in, true, badIPs) + if len(errs) != 0 { + t.Errorf("expected %q to be accepted when using validOldIPs, but got: %v", tc.in, errs) + } }) } } @@ -300,7 +314,7 @@ func TestGetWarningsForIP(t *testing.T) { } func TestIsValidCIDR(t *testing.T) { - for _, tc := range []struct { + testCases := []struct { name string in string @@ -455,7 +469,16 @@ func TestIsValidCIDR(t *testing.T) { legacyErr: "must be a valid CIDR value", legacyStrictErr: "must be a valid CIDR value", }, - } { + } + + var badCIDRs []string + for _, tc := range testCases { + if tc.legacyStrictErr != "" { + badCIDRs = append(badCIDRs, tc.in) + } + } + + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { errs := IsValidCIDR(field.NewPath(""), tc.in) if tc.err == "" { @@ -470,7 +493,7 @@ func TestIsValidCIDR(t *testing.T) { } } - errs = IsValidCIDRForLegacyField(field.NewPath(""), tc.in, false) + errs = IsValidCIDRForLegacyField(field.NewPath(""), tc.in, false, nil) if tc.legacyErr == "" { if len(errs) != 0 { t.Errorf("expected %q to be valid according to IsValidCIDRForLegacyField but got: %v", tc.in, errs) @@ -483,7 +506,7 @@ func TestIsValidCIDR(t *testing.T) { } } - errs = IsValidCIDRForLegacyField(field.NewPath(""), tc.in, true) + errs = IsValidCIDRForLegacyField(field.NewPath(""), tc.in, true, nil) if tc.legacyStrictErr == "" { if len(errs) != 0 { t.Errorf("expected %q to be valid according to IsValidCIDRForLegacyField with strict validation but got: %v", tc.in, errs) @@ -495,6 +518,11 @@ func TestIsValidCIDR(t *testing.T) { t.Errorf("expected error for %q from IsValidCIDRForLegacyField with strict validation to contain %q but got: %q", tc.in, tc.legacyStrictErr, errs[0].Detail) } } + + errs = IsValidCIDRForLegacyField(field.NewPath(""), tc.in, true, badCIDRs) + if len(errs) != 0 { + t.Errorf("expected %q to be accepted when using validOldCIDRs, but got: %v", tc.in, errs) + } }) } }