From 692785d25b688f83db14c20f26d0b99e2e99adb8 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 28 Feb 2025 17:41:10 -0500 Subject: [PATCH] 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