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