diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 2d62ea13b55..116cbf85455 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3790,7 +3790,7 @@ func validatePodDNSConfig(dnsConfig *core.PodDNSConfig, dnsPolicy *core.DNSPolic allErrs = append(allErrs, field.Invalid(fldPath.Child("nameservers"), dnsConfig.Nameservers, fmt.Sprintf("must not have more than %v nameservers", MaxDNSNameservers))) } for i, ns := range dnsConfig.Nameservers { - allErrs = append(allErrs, validation.IsValidIP(fldPath.Child("nameservers").Index(i), ns)...) + allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Child("nameservers").Index(i), ns, nil)...) } // Validate searches. if len(dnsConfig.Searches) > MaxDNSSearchPaths { @@ -3966,7 +3966,7 @@ func validateOnlyDeletedSchedulingGates(newGates, oldGates []core.PodSchedulingG func ValidateHostAliases(hostAliases []core.HostAlias, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for i, hostAlias := range hostAliases { - allErrs = append(allErrs, validation.IsValidIP(fldPath.Index(i).Child("ip"), hostAlias.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Index(i).Child("ip"), hostAlias.IP, nil)...) for j, hostname := range hostAlias.Hostnames { allErrs = append(allErrs, ValidateDNS1123Subdomain(hostname, fldPath.Index(i).Child("hostnames").Index(j))...) } @@ -4108,14 +4108,21 @@ func validatePodMetadataAndSpec(pod *core.Pod, opts PodValidationOptions) field. } // validatePodIPs validates IPs in pod status -func validatePodIPs(pod *core.Pod) field.ErrorList { +func validatePodIPs(pod, oldPod *core.Pod) field.ErrorList { allErrs := field.ErrorList{} podIPsField := field.NewPath("status", "podIPs") - // all PodIPs must be valid IPs + // all new PodIPs must be valid IPs, but existing invalid ones can be kept. + var existingPodIPs []string + if oldPod != nil { + existingPodIPs = make([]string, len(oldPod.Status.PodIPs)) + for i, podIP := range oldPod.Status.PodIPs { + existingPodIPs[i] = podIP.IP + } + } for i, podIP := range pod.Status.PodIPs { - allErrs = append(allErrs, validation.IsValidIP(podIPsField.Index(i), podIP.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(podIPsField.Index(i), podIP.IP, existingPodIPs)...) } // if we have more than one Pod.PodIP then we must have a dual-stack pair @@ -4140,7 +4147,7 @@ func validatePodIPs(pod *core.Pod) field.ErrorList { } // validateHostIPs validates IPs in pod status -func validateHostIPs(pod *core.Pod) field.ErrorList { +func validateHostIPs(pod, oldPod *core.Pod) field.ErrorList { allErrs := field.ErrorList{} if len(pod.Status.HostIPs) == 0 { @@ -4154,9 +4161,16 @@ func validateHostIPs(pod *core.Pod) field.ErrorList { allErrs = append(allErrs, field.Invalid(hostIPsField.Index(0).Child("ip"), pod.Status.HostIPs[0].IP, "must be equal to `hostIP`")) } - // all HostIPs must be valid IPs + // all new HostIPs must be valid IPs, but existing invalid ones can be kept. + var existingHostIPs []string + if oldPod != nil { + existingHostIPs = make([]string, len(oldPod.Status.HostIPs)) + for i, hostIP := range oldPod.Status.HostIPs { + existingHostIPs[i] = hostIP.IP + } + } for i, hostIP := range pod.Status.HostIPs { - allErrs = append(allErrs, validation.IsValidIP(hostIPsField.Index(i), hostIP.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(hostIPsField.Index(i), hostIP.IP, existingHostIPs)...) } // if we have more than one Pod.HostIP then we must have a dual-stack pair @@ -5462,11 +5476,11 @@ func ValidatePodStatusUpdate(newPod, oldPod *core.Pod, opts PodValidationOptions allErrs = append(allErrs, ValidateContainerStateTransition(newPod.Status.EphemeralContainerStatuses, oldPod.Status.EphemeralContainerStatuses, fldPath.Child("ephemeralContainerStatuses"), core.RestartPolicyNever)...) allErrs = append(allErrs, validatePodResourceClaimStatuses(newPod.Status.ResourceClaimStatuses, newPod.Spec.ResourceClaims, fldPath.Child("resourceClaimStatuses"))...) - if newIPErrs := validatePodIPs(newPod); len(newIPErrs) > 0 { + if newIPErrs := validatePodIPs(newPod, oldPod); len(newIPErrs) > 0 { allErrs = append(allErrs, newIPErrs...) } - if newIPErrs := validateHostIPs(newPod); len(newIPErrs) > 0 { + if newIPErrs := validateHostIPs(newPod, oldPod); len(newIPErrs) > 0 { allErrs = append(allErrs, newIPErrs...) } @@ -5860,7 +5874,7 @@ var supportedServiceIPFamilyPolicy = sets.New( core.IPFamilyPolicyRequireDualStack) // ValidateService tests if required fields/annotations of a Service are valid. -func ValidateService(service *core.Service) field.ErrorList { +func ValidateService(service, oldService *core.Service) field.ErrorList { metaPath := field.NewPath("metadata") allErrs := ValidateObjectMeta(&service.ObjectMeta, true, ValidateServiceName, metaPath) @@ -5935,15 +5949,24 @@ func ValidateService(service *core.Service) field.ErrorList { } // dualstack <-> ClusterIPs <-> ipfamilies - allErrs = append(allErrs, ValidateServiceClusterIPsRelatedFields(service)...) + allErrs = append(allErrs, ValidateServiceClusterIPsRelatedFields(service, oldService)...) + // All new ExternalIPs must be valid and "non-special". (Existing ExternalIPs may + // have been validated against older rules, but if we allowed them before we can't + // reject them now.) ipPath := specPath.Child("externalIPs") + var existingExternalIPs []string + if oldService != nil { + existingExternalIPs = oldService.Spec.ExternalIPs // +k8s:verify-mutation:reason=clone + } for i, ip := range service.Spec.ExternalIPs { idxPath := ipPath.Index(i) - if errs := validation.IsValidIP(idxPath, ip); len(errs) != 0 { + if errs := IsValidIPForLegacyField(idxPath, ip, existingExternalIPs); len(errs) != 0 { allErrs = append(allErrs, errs...) } else { - allErrs = append(allErrs, ValidateNonSpecialIP(ip, idxPath)...) + // For historical reasons, this uses ValidateEndpointIP even + // though that is not exactly the appropriate set of checks. + allErrs = append(allErrs, ValidateEndpointIP(ip, idxPath)...) } } @@ -5995,18 +6018,27 @@ func ValidateService(service *core.Service) field.ErrorList { ports[key] = true } - // Validate SourceRanges field or annotation. + // Validate SourceRanges field or annotation. Existing invalid CIDR values do not + // need to be fixed. Note that even with the tighter CIDR validation we still + // allow excess whitespace, because that is effectively part of the API. if len(service.Spec.LoadBalancerSourceRanges) > 0 { fieldPath := specPath.Child("LoadBalancerSourceRanges") if service.Spec.Type != core.ServiceTypeLoadBalancer { allErrs = append(allErrs, field.Forbidden(fieldPath, "may only be used when `type` is 'LoadBalancer'")) } + var existingSourceRanges []string + if oldService != nil { + existingSourceRanges = make([]string, len(oldService.Spec.LoadBalancerSourceRanges)) + for i, value := range oldService.Spec.LoadBalancerSourceRanges { + existingSourceRanges[i] = strings.TrimSpace(value) + } + } for idx, value := range service.Spec.LoadBalancerSourceRanges { // Note: due to a historical accident around transition from the // annotation value, these values are allowed to be space-padded. value = strings.TrimSpace(value) - allErrs = append(allErrs, validation.IsValidCIDR(fieldPath.Index(idx), value)...) + allErrs = append(allErrs, IsValidCIDRForLegacyField(fieldPath.Index(idx), value, existingSourceRanges)...) } } else if val, annotationSet := service.Annotations[core.AnnotationLoadBalancerSourceRangesKey]; annotationSet { fieldPath := field.NewPath("metadata", "annotations").Key(core.AnnotationLoadBalancerSourceRangesKey) @@ -6014,12 +6046,14 @@ func ValidateService(service *core.Service) field.ErrorList { allErrs = append(allErrs, field.Forbidden(fieldPath, "may only be used when `type` is 'LoadBalancer'")) } - val = strings.TrimSpace(val) - if val != "" { - cidrs := strings.Split(val, ",") - for _, value := range cidrs { - value = strings.TrimSpace(value) - allErrs = append(allErrs, validation.IsValidCIDR(fieldPath, value)...) + if oldService == nil || oldService.Annotations[core.AnnotationLoadBalancerSourceRangesKey] != val { + val = strings.TrimSpace(val) + if val != "" { + cidrs := strings.Split(val, ",") + for _, value := range cidrs { + value = strings.TrimSpace(value) + allErrs = append(allErrs, IsValidCIDRForLegacyField(fieldPath, value, nil)...) + } } } } @@ -6179,7 +6213,7 @@ func validateServiceTrafficDistribution(service *core.Service) field.ErrorList { // ValidateServiceCreate validates Services as they are created. func ValidateServiceCreate(service *core.Service) field.ErrorList { - return ValidateService(service) + return ValidateService(service, nil) } // ValidateServiceUpdate tests if required fields in the service are set during an update @@ -6202,13 +6236,13 @@ func ValidateServiceUpdate(service, oldService *core.Service) field.ErrorList { allErrs = append(allErrs, validateServiceExternalTrafficFieldsUpdate(oldService, service)...) - return append(allErrs, ValidateService(service)...) + return append(allErrs, ValidateService(service, oldService)...) } // ValidateServiceStatusUpdate tests if required fields in the Service are set when updating status. func ValidateServiceStatusUpdate(service, oldService *core.Service) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&service.ObjectMeta, &oldService.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateLoadBalancerStatus(&service.Status.LoadBalancer, field.NewPath("status", "loadBalancer"), &service.Spec)...) + allErrs = append(allErrs, ValidateLoadBalancerStatus(&service.Status.LoadBalancer, &oldService.Status.LoadBalancer, field.NewPath("status", "loadBalancer"), &service.Spec)...) return allErrs } @@ -6403,7 +6437,7 @@ func ValidateNode(node *core.Node) field.ErrorList { // all PodCIDRs should be valid ones for idx, value := range node.Spec.PodCIDRs { - allErrs = append(allErrs, validation.IsValidCIDR(podCIDRsField.Index(idx), value)...) + allErrs = append(allErrs, IsValidCIDRForLegacyField(podCIDRsField.Index(idx), value, nil)...) } // if more than PodCIDR then it must be a dual-stack pair @@ -7430,16 +7464,28 @@ func ValidateNamespaceFinalizeUpdate(newNamespace, oldNamespace *core.Namespace) } // ValidateEndpoints validates Endpoints on create and update. -func ValidateEndpoints(endpoints *core.Endpoints) field.ErrorList { +func ValidateEndpoints(endpoints, oldEndpoints *core.Endpoints) field.ErrorList { allErrs := ValidateObjectMeta(&endpoints.ObjectMeta, true, ValidateEndpointsName, field.NewPath("metadata")) allErrs = append(allErrs, ValidateEndpointsSpecificAnnotations(endpoints.Annotations, field.NewPath("annotations"))...) - allErrs = append(allErrs, validateEndpointSubsets(endpoints.Subsets, field.NewPath("subsets"))...) + + subsetErrs := validateEndpointSubsets(endpoints.Subsets, field.NewPath("subsets")) + if len(subsetErrs) != 0 { + // If this is an update, and Subsets was unchanged, then ignore the + // validation errors, since apparently older versions of Kubernetes + // considered the data valid. (We only check this after getting a + // validation error since Endpoints may be large and DeepEqual is slow.) + if oldEndpoints != nil && apiequality.Semantic.DeepEqual(oldEndpoints.Subsets, endpoints.Subsets) { + subsetErrs = nil + } + } + allErrs = append(allErrs, subsetErrs...) + return allErrs } // ValidateEndpointsCreate validates Endpoints on create. func ValidateEndpointsCreate(endpoints *core.Endpoints) field.ErrorList { - return ValidateEndpoints(endpoints) + return ValidateEndpoints(endpoints, nil) } // ValidateEndpointsUpdate validates Endpoints on update. NodeName changes are @@ -7448,7 +7494,7 @@ func ValidateEndpointsCreate(endpoints *core.Endpoints) field.ErrorList { // happens. func ValidateEndpointsUpdate(newEndpoints, oldEndpoints *core.Endpoints) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&newEndpoints.ObjectMeta, &oldEndpoints.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateEndpoints(newEndpoints)...) + allErrs = append(allErrs, ValidateEndpoints(newEndpoints, oldEndpoints)...) return allErrs } @@ -7479,7 +7525,7 @@ func validateEndpointSubsets(subsets []core.EndpointSubset, fldPath *field.Path) func validateEndpointAddress(address *core.EndpointAddress, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, validation.IsValidIP(fldPath.Child("ip"), address.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Child("ip"), address.IP, nil)...) if len(address.Hostname) > 0 { allErrs = append(allErrs, ValidateDNS1123Label(address.Hostname, fldPath.Child("hostname"))...) } @@ -7489,19 +7535,21 @@ func validateEndpointAddress(address *core.EndpointAddress, fldPath *field.Path) allErrs = append(allErrs, field.Invalid(fldPath.Child("nodeName"), *address.NodeName, msg).WithOrigin("format=dns-label")) } } - allErrs = append(allErrs, ValidateNonSpecialIP(address.IP, fldPath.Child("ip"))...) + allErrs = append(allErrs, ValidateEndpointIP(address.IP, fldPath.Child("ip"))...) return allErrs } -// ValidateNonSpecialIP is used to validate Endpoints, EndpointSlices, and -// external IPs. Specifically, this disallows unspecified and loopback addresses -// are nonsensical and link-local addresses tend to be used for node-centric -// purposes (e.g. metadata service). +// ValidateEndpointIP is used to validate Endpoints and EndpointSlice addresses, and also +// (for historical reasons) external IPs. It disallows certain address types that don't +// make sense in those contexts. Note that this function is _almost_, but not exactly, +// equivalent to net.IP.IsGlobalUnicast(). (Unlike IsGlobalUnicast, it allows global +// multicast IPs, which is probably a bug.) // -// IPv6 references -// - https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml -// - https://www.iana.org/assignments/ipv6-multicast-addresses/ipv6-multicast-addresses.xhtml -func ValidateNonSpecialIP(ipAddress string, fldPath *field.Path) field.ErrorList { +// This function should not be used for new validations; the exact set of IPs that do and +// don't make sense in a particular field is context-dependent (e.g., localhost makes +// sense in some places; unspecified IPs make sense in fields that are used as bind +// addresses rather than destination addresses). +func ValidateEndpointIP(ipAddress string, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} ip := netutils.ParseIPSloppy(ipAddress) if ip == nil { @@ -7520,7 +7568,7 @@ func ValidateNonSpecialIP(ipAddress string, fldPath *field.Path) field.ErrorList if ip.IsLinkLocalMulticast() { allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, "may not be in the link-local multicast range (224.0.0.0/24, ff02::/10)")) } - return allErrs.WithOrigin("format=non-special-ip") + return allErrs.WithOrigin("format=endpoint-ip") } func validateEndpointPort(port *core.EndpointPort, requireName bool, fldPath *field.Path) field.ErrorList { @@ -7840,16 +7888,25 @@ var ( ) // ValidateLoadBalancerStatus validates required fields on a LoadBalancerStatus -func ValidateLoadBalancerStatus(status *core.LoadBalancerStatus, fldPath *field.Path, spec *core.ServiceSpec) field.ErrorList { +func ValidateLoadBalancerStatus(status, oldStatus *core.LoadBalancerStatus, fldPath *field.Path, spec *core.ServiceSpec) field.ErrorList { allErrs := field.ErrorList{} ingrPath := fldPath.Child("ingress") if !utilfeature.DefaultFeatureGate.Enabled(features.AllowServiceLBStatusOnNonLB) && spec.Type != core.ServiceTypeLoadBalancer && len(status.Ingress) != 0 { allErrs = append(allErrs, field.Forbidden(ingrPath, "may only be used when `spec.type` is 'LoadBalancer'")) } else { + var existingIngressIPs []string + if oldStatus != nil { + existingIngressIPs = make([]string, 0, len(oldStatus.Ingress)) + for _, ingress := range oldStatus.Ingress { + if len(ingress.IP) > 0 { + existingIngressIPs = append(existingIngressIPs, ingress.IP) + } + } + } for i, ingress := range status.Ingress { idxPath := ingrPath.Index(i) if len(ingress.IP) > 0 { - allErrs = append(allErrs, validation.IsValidIP(idxPath.Child("ip"), ingress.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(idxPath.Child("ip"), ingress.IP, existingIngressIPs)...) } if utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) && ingress.IPMode == nil { @@ -8116,7 +8173,7 @@ func validateLabelKeys(fldPath *field.Path, labelKeys []string, labelSelector *m // ValidateServiceClusterIPsRelatedFields validates .spec.ClusterIPs,, // .spec.IPFamilies, .spec.ipFamilyPolicy. This is exported because it is used // during IP init and allocation. -func ValidateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorList { +func ValidateServiceClusterIPsRelatedFields(service, oldService *core.Service) field.ErrorList { // ClusterIP, ClusterIPs, IPFamilyPolicy and IPFamilies are validated prior (all must be unset) for ExternalName service if service.Spec.Type == core.ServiceTypeExternalName { return field.ErrorList{} @@ -8170,6 +8227,11 @@ func ValidateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorLi } } + var existingClusterIPs []string + if oldService != nil { + existingClusterIPs = oldService.Spec.ClusterIPs // +k8s:verify-mutation:reason=clone + } + // clusterIPs stand alone validation // valid ips with None and empty string handling // duplication check is done as part of DualStackvalidation below @@ -8183,8 +8245,8 @@ func ValidateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorLi continue } - // is it valid ip? - errorMessages := validation.IsValidIP(clusterIPsField.Index(i), clusterIP) + // is it a valid ip? (or was it at least previously considered valid?) + errorMessages := IsValidIPForLegacyField(clusterIPsField.Index(i), clusterIP, existingClusterIPs) hasInvalidIPs = (len(errorMessages) != 0) || hasInvalidIPs allErrs = append(allErrs, errorMessages...) } @@ -8699,3 +8761,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, validOldIPs []string) field.ErrorList { + return validation.IsValidIPForLegacyField(fldPath, value, utilfeature.DefaultFeatureGate.Enabled(features.StrictIPCIDRValidation), validOldIPs) +} + +// IsValidCIDRForLegacyField is a wrapper around validation.IsValidCIDRForLegacyField that +// handles setting strictValidation correctly. This is only for fields that use legacy CIDR +// value validation; use validation.IsValidCIDR for new fields. +func IsValidCIDRForLegacyField(fldPath *field.Path, value string, validOldCIDRs []string) field.ErrorList { + return validation.IsValidCIDRForLegacyField(fldPath, value, utilfeature.DefaultFeatureGate.Enabled(features.StrictIPCIDRValidation), validOldCIDRs) +} diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index bdb8784931b..f7141b1fade 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -9737,6 +9737,7 @@ func TestValidatePodDNSConfig(t *testing.T) { dnsConfig *core.PodDNSConfig dnsPolicy *core.DNSPolicy opts PodValidationOptions + legacyIPs bool expectedError bool }{{ desc: "valid: empty DNSConfig", @@ -9931,6 +9932,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{ @@ -9950,16 +9964,20 @@ func TestValidatePodDNSConfig(t *testing.T) { } for _, tc := range testCases { - if tc.dnsPolicy == nil { - tc.dnsPolicy = &testDNSClusterFirst - } + t.Run("", func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !tc.legacyIPs) - errs := validatePodDNSConfig(tc.dnsConfig, tc.dnsPolicy, field.NewPath("dnsConfig"), tc.opts) - if len(errs) != 0 && !tc.expectedError { - t.Errorf("%v: validatePodDNSConfig(%v) = %v, want nil", tc.desc, tc.dnsConfig, errs) - } else if len(errs) == 0 && tc.expectedError { - t.Errorf("%v: validatePodDNSConfig(%v) = nil, want error", tc.desc, tc.dnsConfig) - } + if tc.dnsPolicy == nil { + tc.dnsPolicy = &testDNSClusterFirst + } + + errs := validatePodDNSConfig(tc.dnsConfig, tc.dnsPolicy, field.NewPath("dnsConfig"), tc.opts) + if len(errs) != 0 && !tc.expectedError { + t.Errorf("%v: validatePodDNSConfig(%v) = %v, want nil", tc.desc, tc.dnsConfig, errs) + } else if len(errs) == 0 && tc.expectedError { + t.Errorf("%v: validatePodDNSConfig(%v) = nil, want error", tc.desc, tc.dnsConfig) + } + }) } } @@ -10189,6 +10207,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, } @@ -10234,6 +10270,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, @@ -10303,12 +10345,15 @@ func TestValidatePodSpec(t *testing.T) { ), } for k, v := range failureCases { - opts := PodValidationOptions{ - ResourceIsPod: true, - } - if errs := ValidatePodSpec(&v.Spec, nil, field.NewPath("field"), opts); len(errs) == 0 { - t.Errorf("expected failure for %q", k) - } + t.Run(k, func(t *testing.T) { + 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 failure") + } + }) } } @@ -15113,7 +15158,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 = "" @@ -15249,6 +15304,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) { @@ -15301,35 +15371,50 @@ func TestValidateServiceCreate(t *testing.T) { // numErrs: 1, numErrs: 0, }, { - name: "invalid publicIPs localhost", + name: "invalid externalIPs localhost", tweakSvc: func(s *core.Service) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"127.0.0.1"} }, numErrs: 1, }, { - name: "invalid publicIPs unspecified", + name: "invalid externalIPs unspecified", tweakSvc: func(s *core.Service) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"0.0.0.0"} }, numErrs: 1, }, { - name: "invalid publicIPs loopback", + name: "invalid externalIPs loopback", tweakSvc: func(s *core.Service) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"127.0.0.1"} }, numErrs: 1, }, { - name: "invalid publicIPs host", + name: "invalid externalIPs host", tweakSvc: func(s *core.Service) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"myhost.mydomain"} }, numErrs: 1, }, { - name: "valid publicIPs", + name: "valid 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) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"1.2.3.4"} @@ -15639,6 +15724,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) { @@ -16313,6 +16426,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) @@ -16988,9 +17102,40 @@ func TestValidateNode(t *testing.T) { }, } for _, successCase := range successCases { - if errs := ValidateNode(&successCase); len(errs) != 0 { - t.Errorf("expected success: %v", errs) - } + t.Run("", func(t *testing.T) { + 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{ @@ -17175,6 +17320,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", @@ -17194,30 +17356,34 @@ func TestValidateNode(t *testing.T) { }, } for k, v := range errorCases { - errs := ValidateNode(&v) - if len(errs) == 0 { - t.Errorf("expected failure for %s", k) - } - for i := range errs { - field := errs[i].Field - expectedFields := map[string]bool{ - "metadata.name": true, - "metadata.labels": true, - "metadata.annotations": true, - "metadata.namespace": true, - "spec.externalID": true, - "spec.taints[0].key": true, - "spec.taints[0].value": true, - "spec.taints[0].effect": true, - "metadata.annotations.scheduler.alpha.kubernetes.io/preferAvoidPods[0].PodSignature": true, - "metadata.annotations.scheduler.alpha.kubernetes.io/preferAvoidPods[0].PodSignature.PodController.Controller": true, + t.Run(k, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) + + errs := ValidateNode(&v) + if len(errs) == 0 { + t.Errorf("expected failure") } - if val, ok := expectedFields[field]; ok { - if !val { - t.Errorf("%s: missing prefix for: %v", k, errs[i]) + for i := range errs { + field := errs[i].Field + expectedFields := map[string]bool{ + "metadata.name": true, + "metadata.labels": true, + "metadata.annotations": true, + "metadata.namespace": true, + "spec.externalID": true, + "spec.taints[0].key": true, + "spec.taints[0].value": true, + "spec.taints[0].effect": true, + "metadata.annotations.scheduler.alpha.kubernetes.io/preferAvoidPods[0].PodSignature": true, + "metadata.annotations.scheduler.alpha.kubernetes.io/preferAvoidPods[0].PodSignature.PodController.Controller": true, + } + if val, ok := expectedFields[field]; ok { + if !val { + t.Errorf("missing prefix for: %v", errs[i]) + } } } - } + }) } } @@ -18725,10 +18891,212 @@ func TestValidateServiceUpdate(t *testing.T) { }, numErrs: 1, }, + + // IP validation ratcheting tests. Note: this is tested for + // LoadBalancerStatus in TestValidateLoadBalancerStatus. + { + name: "pre-existing invalid clusterIP ignored when updating other fields", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ClusterIP = "1.2.3.04" + oldSvc.Spec.ClusterIPs = []string{"1.2.3.04"} + newSvc.Spec.ClusterIP = "1.2.3.04" + newSvc.Spec.ClusterIPs = []string{"1.2.3.04"} + newSvc.Labels["foo"] = "bar" + }, + numErrs: 0, + }, { + name: "pre-existing invalid clusterIP ignored when adding clusterIPs", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ClusterIP = "1.2.3.04" + oldSvc.Spec.ClusterIPs = []string{"1.2.3.04"} + oldSvc.Spec.IPFamilyPolicy = &singleStack + oldSvc.Spec.IPFamilies = []core.IPFamily{core.IPv4Protocol} + + newSvc.Spec.ClusterIP = "1.2.3.04" + newSvc.Spec.ClusterIPs = []string{"1.2.3.04", "2001:db8::4"} + newSvc.Spec.IPFamilyPolicy = &requireDualStack + newSvc.Spec.IPFamilies = []core.IPFamily{core.IPv4Protocol, core.IPv6Protocol} + }, + numErrs: 0, + }, { + name: "pre-existing invalid clusterIP ignored when removing clusterIPs", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ClusterIP = "1.2.3.04" + oldSvc.Spec.ClusterIPs = []string{"1.2.3.04", "2001:db::4"} + oldSvc.Spec.IPFamilyPolicy = &requireDualStack + oldSvc.Spec.IPFamilies = []core.IPFamily{core.IPv4Protocol, core.IPv6Protocol} + + newSvc.Spec.ClusterIP = "1.2.3.04" + newSvc.Spec.ClusterIPs = []string{"1.2.3.04"} + newSvc.Spec.IPFamilyPolicy = &singleStack + newSvc.Spec.IPFamilies = []core.IPFamily{core.IPv4Protocol} + }, + numErrs: 0, + }, { + name: "pre-existing invalid externalIP ignored", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + oldSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + newSvc.Labels["foo"] = "bar" + }, + numErrs: 0, + }, { + name: "pre-existing invalid externalIP ignored when adding externalIPs", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + oldSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"5.6.7.8", "1.2.3.04"} + }, + numErrs: 0, + }, { + name: "pre-existing invalid externalIP ignored when removing externalIPs", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + oldSvc.Spec.ExternalIPs = []string{"5.6.7.8", "1.2.3.04"} + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + }, + numErrs: 0, + }, { + name: "can fix invalid externalIP", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + oldSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"1.2.3.4"} + }, + numErrs: 0, + }, { + name: "can't add invalid externalIP", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + }, + numErrs: 1, + }, { + name: "pre-existing invalid loadBalancerSourceRanges ignored when updating other fields", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + newSvc.Labels["foo"] = "bar" + }, + numErrs: 0, + }, { + name: "pre-existing invalid loadBalancerSourceRanges ignored when adding source ranges", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.LoadBalancerSourceRanges = []string{"1.2.3.0/24", "010.0.0.0/8"} + }, + numErrs: 0, + }, { + name: "pre-existing invalid loadBalancerSourceRanges ignored when removing source ranges", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"1.2.3.0/24", "010.0.0.0/8"} + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + }, + numErrs: 0, + }, { + name: "can fix invalid loadBalancerSourceRanges", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.LoadBalancerSourceRanges = []string{"10.0.0.0/8"} + }, + numErrs: 0, + }, { + name: "can't add invalid loadBalancerSourceRanges", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + }, + numErrs: 1, + }, { + name: "pre-existing invalid source ranges ignored when updating other fields", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8" + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8" + newSvc.Labels["foo"] = "bar" + }, + numErrs: 0, + }, { + name: "can fix invalid source ranges annotation", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8" + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "10.0.0.0/8" + }, + numErrs: 0, + }, { + name: "can't modify pre-existing invalid source ranges annotation without fixing it", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8" + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8, 1.2.3.0/24" + }, + numErrs: 1, + }, { + name: "can't add invalid source ranges annotation", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8, 1.2.3.0/24" + }, + numErrs: 1, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) + oldSvc := makeValidService() newSvc := makeValidService() tc.tweakSvc(&oldSvc, &newSvc) @@ -20581,9 +20949,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) @@ -20643,6 +21038,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"}, @@ -20724,7 +21131,7 @@ func TestValidateEndpointsCreate(t *testing.T) { }}, }, expectedErrs: field.ErrorList{ - field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=non-special-ip"), + field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=endpoint-ip"), }, }, "Address is link-local": { @@ -20736,7 +21143,7 @@ func TestValidateEndpointsCreate(t *testing.T) { }}, }, expectedErrs: field.ErrorList{ - field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=non-special-ip"), + field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=endpoint-ip"), }, }, "Address is link-local multicast": { @@ -20748,7 +21155,7 @@ func TestValidateEndpointsCreate(t *testing.T) { }}, }, expectedErrs: field.ErrorList{ - field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=non-special-ip"), + field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=endpoint-ip"), }, }, "Invalid AppProtocol": { @@ -20767,6 +21174,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 := field.ErrorMatcher{}.ByType().ByField().ByOrigin() @@ -21882,7 +22290,7 @@ func TestEndpointAddressNodeNameUpdateRestrictions(t *testing.T) { updatedEndpoint := newNodeNameEndpoint("kubernetes-changed-nodename") // Check that NodeName can be changed during update, this is to accommodate the case where nodeIP or PodCIDR is reused. // The same ip will now have a different nodeName. - errList := ValidateEndpoints(updatedEndpoint) + errList := ValidateEndpoints(updatedEndpoint, oldEndpoint) errList = append(errList, ValidateEndpointsUpdate(updatedEndpoint, oldEndpoint)...) if len(errList) != 0 { t.Error("Endpoint should allow changing of Subset.Addresses.NodeName on update") @@ -21892,7 +22300,7 @@ func TestEndpointAddressNodeNameUpdateRestrictions(t *testing.T) { func TestEndpointAddressNodeNameInvalidDNSSubdomain(t *testing.T) { // Check NodeName DNS validation endpoint := newNodeNameEndpoint("illegal*.nodename") - errList := ValidateEndpoints(endpoint) + errList := ValidateEndpoints(endpoint, nil) if len(errList) == 0 { t.Error("Endpoint should reject invalid NodeName") } @@ -21900,7 +22308,7 @@ func TestEndpointAddressNodeNameInvalidDNSSubdomain(t *testing.T) { func TestEndpointAddressNodeNameCanBeAnIPAddress(t *testing.T) { endpoint := newNodeNameEndpoint("10.10.1.1") - errList := ValidateEndpoints(endpoint) + errList := ValidateEndpoints(endpoint, nil) if len(errList) != 0 { t.Error("Endpoint should accept a NodeName that is an IP address") } @@ -22824,32 +23232,48 @@ func makePod(podName string, podNamespace string, podIPs []core.PodIP) core.Pod } } func TestPodIPsValidation(t *testing.T) { + // We test updating every pod in testCases to every other pod in testCases. + // expectError is true if we expect an error when updating *to* that pod. + // allowNoOpUpdate is true if we expect a no-op update to succeed. + testCases := []struct { - pod core.Pod - expectError bool - }{{ - expectError: false, - pod: makePod("nil-ips", "ns", nil), - }, { - expectError: false, - pod: makePod("empty-podips-list", "ns", []core.PodIP{}), - }, { - expectError: false, - pod: makePod("single-ip-family-6", "ns", []core.PodIP{{IP: "::1"}}), - }, { - expectError: false, - pod: makePod("single-ip-family-4", "ns", []core.PodIP{{IP: "1.1.1.1"}}), - }, { - expectError: false, - pod: makePod("dual-stack-4-6", "ns", []core.PodIP{{IP: "1.1.1.1"}, {IP: "::1"}}), - }, { - expectError: false, - pod: makePod("dual-stack-6-4", "ns", []core.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), - }, + pod core.Pod + legacyIPs bool + expectError bool + allowNoOpUpdate bool + }{ + { + expectError: false, + pod: makePod("nil-ips", "ns", nil), + }, { + expectError: false, + pod: makePod("empty-podips-list", "ns", []core.PodIP{}), + }, { + expectError: false, + pod: makePod("single-ip-family-6", "ns", []core.PodIP{{IP: "::1"}}), + }, { + expectError: false, + pod: makePod("single-ip-family-4", "ns", []core.PodIP{{IP: "1.1.1.1"}}), + }, { + expectError: false, + pod: makePod("dual-stack-4-6", "ns", []core.PodIP{{IP: "1.1.1.1"}, {IP: "::1"}}), + }, { + expectError: false, + pod: makePod("dual-stack-6-4", "ns", []core.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), + }, { + expectError: false, + 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, + allowNoOpUpdate: true, + pod: makePod("invalid-pod-ip", "ns", []core.PodIP{{IP: "1.1.1.01"}}), + }, { + expectError: true, + allowNoOpUpdate: true, + pod: makePod("legacy-pod-ip-with-strict-validation", "ns", []core.PodIP{{IP: "001.002.003.004"}}), }, { expectError: true, pod: makePod("dualstack-same-ip-family-6", "ns", []core.PodIP{{IP: "::1"}, {IP: "::2"}}), @@ -22873,9 +23297,14 @@ func TestPodIPsValidation(t *testing.T) { }, } - for _, testCase := range testCases { + for i, testCase := range testCases { t.Run(testCase.pod.Name, func(t *testing.T) { - for _, oldTestCase := range testCases { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !testCase.legacyIPs) + for j, oldTestCase := range testCases { + if oldTestCase.legacyIPs && !testCase.legacyIPs { + continue + } + newPod := testCase.pod.DeepCopy() newPod.ResourceVersion = "1" @@ -22885,11 +23314,15 @@ func TestPodIPsValidation(t *testing.T) { errs := ValidatePodStatusUpdate(newPod, oldPod, PodValidationOptions{}) - if len(errs) == 0 && testCase.expectError { - t.Fatalf("expected failure for %s, but there were none", testCase.pod.Name) + expectError := testCase.expectError + if testCase.allowNoOpUpdate && i == j { + expectError = false + } + if len(errs) == 0 && expectError { + t.Fatalf("expected failure updating from %s, but there were none", oldTestCase.pod.Name) } if len(errs) != 0 && !testCase.expectError { - t.Fatalf("expected success for %s, but there were errors: %v", testCase.pod.Name, errs) + t.Fatalf("expected success updating from %s, but there were errors: %v", oldTestCase.pod.Name, errs) } } }) @@ -22920,9 +23353,15 @@ func makePodWithHostIPs(podName string, podNamespace string, hostIPs []core.Host } func TestHostIPsValidation(t *testing.T) { + // We test updating every pod in testCases to every other pod in testCases. + // expectError is true if we expect an error when updating *to* that pod. + // allowNoOpUpdate is true if we expect a no-op update to succeed. + testCases := []struct { - pod core.Pod - expectError bool + pod core.Pod + legacyIPs bool + expectError bool + allowNoOpUpdate bool }{ { expectError: false, @@ -22948,10 +23387,21 @@ 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-pod-ip", "ns", []core.HostIP{{IP: "this-is-not-an-ip"}}), + expectError: true, + allowNoOpUpdate: true, + pod: makePodWithHostIPs("invalid-host-ip", "ns", []core.HostIP{{IP: "this-is-not-an-ip"}}), + }, + { + expectError: true, + allowNoOpUpdate: true, + pod: makePodWithHostIPs("invalid-legacy-host-ip-with-strict-validation", "ns", []core.HostIP{{IP: "001.002.003.004"}}), }, { expectError: true, @@ -22980,9 +23430,14 @@ func TestHostIPsValidation(t *testing.T) { }, } - for _, testCase := range testCases { + for i, testCase := range testCases { t.Run(testCase.pod.Name, func(t *testing.T) { - for _, oldTestCase := range testCases { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !testCase.legacyIPs) + for j, oldTestCase := range testCases { + if oldTestCase.legacyIPs && !testCase.legacyIPs { + continue + } + newPod := testCase.pod.DeepCopy() newPod.ResourceVersion = "1" @@ -22992,11 +23447,15 @@ func TestHostIPsValidation(t *testing.T) { errs := ValidatePodStatusUpdate(newPod, oldPod, PodValidationOptions{}) - if len(errs) == 0 && testCase.expectError { - t.Fatalf("expected failure for %s, but there were none", testCase.pod.Name) + expectError := testCase.expectError + if testCase.allowNoOpUpdate && i == j { + expectError = false + } + if len(errs) == 0 && expectError { + t.Fatalf("expected failure updating from %s, but there were none", oldTestCase.pod.Name) } if len(errs) != 0 && !testCase.expectError { - t.Fatalf("expected success for %s, but there were errors: %v", testCase.pod.Name, errs) + t.Fatalf("expected success updating from %s, but there were errors: %v", oldTestCase.pod.Name, errs) } } }) @@ -23640,7 +24099,7 @@ func TestValidateResourceRequirements(t *testing.T) { } } -func TestValidateNonSpecialIP(t *testing.T) { +func TestValidateEndpointIP(t *testing.T) { fp := field.NewPath("ip") // Valid values. @@ -23651,11 +24110,15 @@ func TestValidateNonSpecialIP(t *testing.T) { {"ipv4", "10.1.2.3"}, {"ipv4 class E", "244.1.2.3"}, {"ipv6", "2000::1"}, + + // These probably should not have been allowed, but they are + {"ipv4 multicast", "239.255.255.253"}, + {"ipv6 multicast", "ff05::1:3"}, } { t.Run(tc.desc, func(t *testing.T) { - errs := ValidateNonSpecialIP(tc.ip, fp) + errs := ValidateEndpointIP(tc.ip, fp) if len(errs) != 0 { - t.Errorf("ValidateNonSpecialIP(%q, ...) = %v; want nil", tc.ip, errs) + t.Errorf("ValidateEndpointIP(%q, ...) = %v; want nil", tc.ip, errs) } }) } @@ -23669,13 +24132,15 @@ func TestValidateNonSpecialIP(t *testing.T) { {"ipv4 localhost", "127.0.0.0"}, {"ipv4 localhost", "127.255.255.255"}, {"ipv6 localhost", "::1"}, + {"ipv4 link local", "169.254.169.254"}, {"ipv6 link local", "fe80::"}, + {"ipv4 local multicast", "224.0.0.251"}, {"ipv6 local multicast", "ff02::"}, } { t.Run(tc.desc, func(t *testing.T) { - errs := ValidateNonSpecialIP(tc.ip, fp) + errs := ValidateEndpointIP(tc.ip, fp) if len(errs) == 0 { - t.Errorf("ValidateNonSpecialIP(%q, ...) = nil; want non-nil (errors)", tc.ip) + t.Errorf("ValidateEndpointIP(%q, ...) = nil; want non-nil (errors)", tc.ip) } }) } @@ -24542,12 +25007,14 @@ func TestValidateLoadBalancerStatus(t *testing.T) { ipModeDummy := core.LoadBalancerIPMode("dummy") testCases := []struct { - name string - ipModeEnabled bool - nonLBAllowed bool - tweakLBStatus func(s *core.LoadBalancerStatus) - tweakSvcSpec func(s *core.ServiceSpec) - numErrs int + name string + ipModeEnabled bool + legacyIPs bool + nonLBAllowed bool + tweakOldLBStatus func(s *core.LoadBalancerStatus) + tweakLBStatus func(s *core.LoadBalancerStatus) + tweakSvcSpec func(s *core.ServiceSpec) + numErrs int }{ { name: "type is not LB", @@ -24630,22 +25097,99 @@ 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, + }, { + name: "invalid ingress IP ignored on update", + tweakOldLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }} + }, + tweakLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }} + }, + numErrs: 0, + }, { + name: "invalid ingress IP ignored when adding IP", + tweakOldLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }} + }, + tweakLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }, { + IP: "5.6.7.8", + IPMode: &ipModeVIP, + }} + }, + numErrs: 0, + }, { + name: "invalid ingress IP can be fixed", + tweakOldLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }} + }, + tweakLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.4", + IPMode: &ipModeVIP, + }} + }, + numErrs: 0, }, } for _, tc := range testCases { 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) + oldStatus := core.LoadBalancerStatus{} + if tc.tweakOldLBStatus != nil { + tc.tweakOldLBStatus(&oldStatus) + } status := core.LoadBalancerStatus{} tc.tweakLBStatus(&status) spec := core.ServiceSpec{Type: core.ServiceTypeLoadBalancer} if tc.tweakSvcSpec != nil { tc.tweakSvcSpec(&spec) } - errs := ValidateLoadBalancerStatus(&status, field.NewPath("status"), &spec) + errs := ValidateLoadBalancerStatus(&status, &oldStatus, field.NewPath("status"), &spec) if len(errs) != tc.numErrs { t.Errorf("Unexpected error list for case %q(expected:%v got %v) - Errors:\n %v", tc.name, tc.numErrs, len(errs), errs.ToAggregate()) } diff --git a/pkg/apis/discovery/validation/validation.go b/pkg/apis/discovery/validation/validation.go index a514ef77d8a..3a09837a8b5 100644 --- a/pkg/apis/discovery/validation/validation.go +++ b/pkg/apis/discovery/validation/validation.go @@ -20,6 +20,7 @@ import ( "fmt" corev1 "k8s.io/api/core/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation" metavalidation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/util/sets" @@ -28,6 +29,7 @@ import ( api "k8s.io/kubernetes/pkg/apis/core" apivalidation "k8s.io/kubernetes/pkg/apis/core/validation" "k8s.io/kubernetes/pkg/apis/discovery" + netutils "k8s.io/utils/net" ) var ( @@ -54,23 +56,34 @@ var ( var ValidateEndpointSliceName = apimachineryvalidation.NameIsDNSSubdomain // ValidateEndpointSlice validates an EndpointSlice. -func ValidateEndpointSlice(endpointSlice *discovery.EndpointSlice) field.ErrorList { +func ValidateEndpointSlice(endpointSlice, oldEndpointSlice *discovery.EndpointSlice) field.ErrorList { allErrs := apivalidation.ValidateObjectMeta(&endpointSlice.ObjectMeta, true, ValidateEndpointSliceName, field.NewPath("metadata")) allErrs = append(allErrs, validateAddressType(endpointSlice.AddressType)...) - allErrs = append(allErrs, validateEndpoints(endpointSlice.Endpoints, endpointSlice.AddressType, field.NewPath("endpoints"))...) allErrs = append(allErrs, validatePorts(endpointSlice.Ports, field.NewPath("ports"))...) + endpointsErrs := validateEndpoints(endpointSlice.Endpoints, endpointSlice.AddressType, field.NewPath("endpoints")) + if len(endpointsErrs) != 0 { + // If this is an update, and Endpoints was unchanged, then ignore the + // validation errors, since apparently older versions of Kubernetes + // considered the data valid. (We only check this after getting a + // validation error since Endpoints may be large and DeepEqual is slow.) + if oldEndpointSlice != nil && apiequality.Semantic.DeepEqual(oldEndpointSlice.Endpoints, endpointSlice.Endpoints) { + endpointsErrs = nil + } + } + allErrs = append(allErrs, endpointsErrs...) + return allErrs } // ValidateEndpointSliceCreate validates an EndpointSlice when it is created. func ValidateEndpointSliceCreate(endpointSlice *discovery.EndpointSlice) field.ErrorList { - return ValidateEndpointSlice(endpointSlice) + return ValidateEndpointSlice(endpointSlice, nil) } // ValidateEndpointSliceUpdate validates an EndpointSlice when it is updated. func ValidateEndpointSliceUpdate(newEndpointSlice, oldEndpointSlice *discovery.EndpointSlice) field.ErrorList { - allErrs := ValidateEndpointSlice(newEndpointSlice) + allErrs := ValidateEndpointSlice(newEndpointSlice, oldEndpointSlice) allErrs = append(allErrs, apivalidation.ValidateImmutableField(newEndpointSlice.AddressType, oldEndpointSlice.AddressType, field.NewPath("addressType"))...) return allErrs @@ -99,11 +112,25 @@ func validateEndpoints(endpoints []discovery.Endpoint, addrType discovery.Addres // and do not get validated. switch addrType { case discovery.AddressTypeIPv4: - allErrs = append(allErrs, validation.IsValidIPv4Address(addressPath.Index(i), address)...) - allErrs = append(allErrs, apivalidation.ValidateNonSpecialIP(address, addressPath.Index(i))...) + ipErrs := apivalidation.IsValidIPForLegacyField(addressPath.Index(i), address, nil) + if len(ipErrs) > 0 { + allErrs = append(allErrs, ipErrs...) + } else { + if !netutils.IsIPv4String(address) { + allErrs = append(allErrs, field.Invalid(addressPath, address, "must be an IPv4 address")) + } + allErrs = append(allErrs, apivalidation.ValidateEndpointIP(address, addressPath.Index(i))...) + } case discovery.AddressTypeIPv6: - allErrs = append(allErrs, validation.IsValidIPv6Address(addressPath.Index(i), address)...) - allErrs = append(allErrs, apivalidation.ValidateNonSpecialIP(address, addressPath.Index(i))...) + ipErrs := validation.IsValidIP(addressPath.Index(i), address) + if len(ipErrs) > 0 { + allErrs = append(allErrs, ipErrs...) + } else { + if !netutils.IsIPv6String(address) { + allErrs = append(allErrs, field.Invalid(addressPath, address, "must be an IPv6 address")) + } + allErrs = append(allErrs, apivalidation.ValidateEndpointIP(address, addressPath.Index(i))...) + } case discovery.AddressTypeFQDN: allErrs = append(allErrs, validation.IsFullyQualifiedDomainName(addressPath.Index(i), address)...) } diff --git a/pkg/apis/discovery/validation/validation_test.go b/pkg/apis/discovery/validation/validation_test.go index 3e6c4cd1d38..50a8bf2b69b 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": { @@ -408,7 +428,7 @@ func TestValidateEndpointSlice(t *testing.T) { }, }, "bad-ip": { - expectedErrors: 2, + expectedErrors: 1, endpointSlice: &discovery.EndpointSlice{ ObjectMeta: standardMeta, AddressType: discovery.AddressTypeIPv4, @@ -422,8 +442,23 @@ 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: 3, + expectedErrors: 2, endpointSlice: &discovery.EndpointSlice{ ObjectMeta: standardMeta, AddressType: discovery.AddressTypeIPv4, @@ -438,7 +473,7 @@ func TestValidateEndpointSlice(t *testing.T) { }, }, "bad-ipv6": { - expectedErrors: 4, + expectedErrors: 2, endpointSlice: &discovery.EndpointSlice{ ObjectMeta: standardMeta, AddressType: discovery.AddressTypeIPv6, @@ -601,7 +636,8 @@ func TestValidateEndpointSlice(t *testing.T) { for name, testCase := range testCases { t.Run(name, func(t *testing.T) { - errs := ValidateEndpointSlice(testCase.endpointSlice) + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !testCase.legacyIPs) + errs := ValidateEndpointSlice(testCase.endpointSlice, nil) if len(errs) != testCase.expectedErrors { t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs) } @@ -701,6 +737,7 @@ func TestValidateEndpointSliceCreate(t *testing.T) { for name, testCase := range testCases { t.Run(name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) errs := ValidateEndpointSliceCreate(testCase.endpointSlice) if len(errs) != testCase.expectedErrors { t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs) @@ -729,6 +766,23 @@ func TestValidateEndpointSliceUpdate(t *testing.T) { }, expectedErrors: 0, }, + "unchanged IPs are not revalidated": { + oldEndpointSlice: &discovery.EndpointSlice{ + ObjectMeta: standardMeta, + AddressType: discovery.AddressTypeIPv4, + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"10.1.2.04"}, + }}, + }, + newEndpointSlice: &discovery.EndpointSlice{ + ObjectMeta: standardMeta, + AddressType: discovery.AddressTypeIPv4, + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"10.1.2.04"}, + }}, + }, + expectedErrors: 0, + }, // expected errors "invalid node name set": { @@ -787,10 +841,28 @@ func TestValidateEndpointSliceUpdate(t *testing.T) { }, expectedErrors: 1, }, + "changed IPs are revalidated": { + oldEndpointSlice: &discovery.EndpointSlice{ + ObjectMeta: standardMeta, + AddressType: discovery.AddressTypeIPv4, + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"10.1.2.3"}, + }}, + }, + newEndpointSlice: &discovery.EndpointSlice{ + ObjectMeta: standardMeta, + AddressType: discovery.AddressTypeIPv4, + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"10.1.2.03"}, + }}, + }, + expectedErrors: 1, + }, } for name, testCase := range testCases { t.Run(name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) errs := ValidateEndpointSliceUpdate(testCase.newEndpointSlice, testCase.oldEndpointSlice) if len(errs) != testCase.expectedErrors { t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs) diff --git a/pkg/apis/networking/validation/validation.go b/pkg/apis/networking/validation/validation.go index 1371ec7d8c2..8bbad55ddc0 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" @@ -57,6 +56,7 @@ var ( type NetworkPolicyValidationOptions struct { AllowInvalidLabelValueInSelector bool + AllowCIDRsEvenIfInvalid []string } // ValidateNetworkPolicyName can be used to check whether the given networkpolicy @@ -119,7 +119,7 @@ func ValidateNetworkPolicyPeer(peer *networking.NetworkPolicyPeer, opts NetworkP } if peer.IPBlock != nil { numPeers++ - allErrs = append(allErrs, ValidateIPBlock(peer.IPBlock, peerPath.Child("ipBlock"))...) + allErrs = append(allErrs, ValidateIPBlock(peer.IPBlock, peerPath.Child("ipBlock"), opts)...) } if numPeers == 0 { @@ -207,20 +207,47 @@ func ValidationOptionsForNetworking(new, old *networking.NetworkPolicy) NetworkP // ValidateNetworkPolicyUpdate tests if an update to a NetworkPolicy is valid. func ValidateNetworkPolicyUpdate(update, old *networking.NetworkPolicy, opts NetworkPolicyValidationOptions) field.ErrorList { + // Record all existing CIDRs in the policy; see ValidateIPBlock. + var existingCIDRs []string + for _, ingress := range old.Spec.Ingress { + for _, peer := range ingress.From { + if peer.IPBlock != nil { + existingCIDRs = append(existingCIDRs, peer.IPBlock.CIDR) + existingCIDRs = append(existingCIDRs, peer.IPBlock.Except...) + } + } + } + for _, egress := range old.Spec.Egress { + for _, peer := range egress.To { + if peer.IPBlock != nil { + existingCIDRs = append(existingCIDRs, peer.IPBlock.CIDR) + existingCIDRs = append(existingCIDRs, peer.IPBlock.Except...) + } + } + } + opts.AllowCIDRsEvenIfInvalid = existingCIDRs + allErrs := field.ErrorList{} allErrs = append(allErrs, apivalidation.ValidateObjectMetaUpdate(&update.ObjectMeta, &old.ObjectMeta, field.NewPath("metadata"))...) allErrs = append(allErrs, ValidateNetworkPolicySpec(&update.Spec, opts, field.NewPath("spec"))...) return allErrs } -// ValidateIPBlock validates a cidr and the except fields of an IpBlock NetworkPolicyPeer -func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path) field.ErrorList { +// ValidateIPBlock validates a cidr and the except fields of an IPBlock NetworkPolicyPeer. +// +// If a pre-existing CIDR is invalid/insecure, but it is not being changed by this update, +// then we have to continue allowing it. But since the user may have changed the policy in +// arbitrary ways (adding/removing rules, adding/removing peers, adding/removing +// ipBlock.except values, etc), we can't reliably determine whether a CIDR value we see +// here is a new value or a pre-existing one. So we just allow any CIDR value that +// appeared in the old NetworkPolicy to be used here without revalidation. +func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path, opts NetworkPolicyValidationOptions) field.ErrorList { allErrs := field.ErrorList{} if ipb.CIDR == "" { allErrs = append(allErrs, field.Required(fldPath.Child("cidr"), "")) return allErrs } - allErrs = append(allErrs, validation.IsValidCIDR(fldPath.Child("cidr"), ipb.CIDR)...) + allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(fldPath.Child("cidr"), ipb.CIDR, opts.AllowCIDRsEvenIfInvalid)...) _, cidrIPNet, err := netutils.ParseCIDRSloppy(ipb.CIDR) if err != nil { // Implies validation would have failed so we already added errors for it. @@ -229,7 +256,7 @@ func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path) field.ErrorLi for i, exceptCIDRStr := range ipb.Except { exceptPath := fldPath.Child("except").Index(i) - allErrs = append(allErrs, validation.IsValidCIDR(exceptPath, exceptCIDRStr)...) + allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(exceptPath, exceptCIDRStr, opts.AllowCIDRsEvenIfInvalid)...) _, exceptCIDR, err := netutils.ParseCIDRSloppy(exceptCIDRStr) if err != nil { // Implies validation would have failed so we already added errors for it. @@ -347,17 +374,28 @@ func ValidateIngressSpec(spec *networking.IngressSpec, fldPath *field.Path, opts // ValidateIngressStatusUpdate tests if required fields in the Ingress are set when updating status. func ValidateIngressStatusUpdate(ingress, oldIngress *networking.Ingress) field.ErrorList { allErrs := apivalidation.ValidateObjectMetaUpdate(&ingress.ObjectMeta, &oldIngress.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateIngressLoadBalancerStatus(&ingress.Status.LoadBalancer, field.NewPath("status", "loadBalancer"))...) + allErrs = append(allErrs, ValidateIngressLoadBalancerStatus(&ingress.Status.LoadBalancer, &oldIngress.Status.LoadBalancer, field.NewPath("status", "loadBalancer"))...) return allErrs } // ValidateIngressLoadBalancerStatus validates required fields on an IngressLoadBalancerStatus -func ValidateIngressLoadBalancerStatus(status *networking.IngressLoadBalancerStatus, fldPath *field.Path) field.ErrorList { +func ValidateIngressLoadBalancerStatus(status, oldStatus *networking.IngressLoadBalancerStatus, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} + + var existingIngressIPs []string + if oldStatus != nil { + existingIngressIPs = make([]string, 0, len(oldStatus.Ingress)) + for _, ingress := range oldStatus.Ingress { + if len(ingress.IP) > 0 { + existingIngressIPs = append(existingIngressIPs, ingress.IP) + } + } + } + for i, ingress := range status.Ingress { idxPath := fldPath.Child("ingress").Index(i) if len(ingress.IP) > 0 { - allErrs = append(allErrs, validation.IsValidIP(idxPath.Child("ip"), ingress.IP)...) + allErrs = append(allErrs, apivalidation.IsValidIPForLegacyField(idxPath.Child("ip"), ingress.IP, existingIngressIPs)...) } if len(ingress.Hostname) > 0 { for _, msg := range validation.IsDNS1123Subdomain(ingress.Hostname) { @@ -653,12 +691,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 +786,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 7b2ad6f0515..7217964ed42 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,11 +251,27 @@ 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) + } + }) + } - for k, v := range successCases { - if errs := ValidateNetworkPolicy(v, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) != 0 { - t.Errorf("Expected success for the success validation test number %d, got %v", k, errs) - } + 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) + } + }) } invalidSelector := map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "b"} @@ -299,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{""} }), @@ -367,9 +389,12 @@ func TestValidateNetworkPolicy(t *testing.T) { // Error cases are not expected to pass validation. for testName, networkPolicy := range errorCases { - if errs := ValidateNetworkPolicy(networkPolicy, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) == 0 { - t.Errorf("Expected failure for test: %s", testName) - } + t.Run(testName, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) + if errs := ValidateNetworkPolicy(networkPolicy, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) == 0 { + t.Errorf("Expected failure") + } + }) } } @@ -417,14 +442,98 @@ func TestValidateNetworkPolicyUpdate(t *testing.T) { }, }, }, + "fix pre-existing invalid CIDR": { + old: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.2.0/24", + "192.168.3.1/24", + }, + }, + }}, + }}, + }, + }, + update: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.2.0/24", + "192.168.3.0/24", + }, + }, + }}, + }}, + }, + }, + }, + "fail to fix pre-existing invalid CIDR": { + old: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.2.0/24", + "192.168.3.1/24", + }, + }, + }}, + }}, + }, + }, + update: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.1.0/24", + "192.168.2.0/24", + "192.168.3.1/24", + }, + }, + }}, + }}, + }, + }, + }, } for testName, successCase := range successCases { - successCase.old.ObjectMeta.ResourceVersion = "1" - successCase.update.ObjectMeta.ResourceVersion = "1" - if errs := ValidateNetworkPolicyUpdate(&successCase.update, &successCase.old, NetworkPolicyValidationOptions{false}); len(errs) != 0 { - t.Errorf("expected success (%s): %v", testName, errs) - } + t.Run(testName, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) + successCase.old.ObjectMeta.ResourceVersion = "1" + successCase.update.ObjectMeta.ResourceVersion = "1" + if errs := ValidateNetworkPolicyUpdate(&successCase.update, &successCase.old, NetworkPolicyValidationOptions{}); len(errs) != 0 { + t.Errorf("expected success, but got %v", errs) + } + }) } errorCases := map[string]npUpdateTest{ @@ -444,14 +553,58 @@ func TestValidateNetworkPolicyUpdate(t *testing.T) { }, }, }, + "add new invalid CIDR": { + old: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.2.0/24", + "192.168.3.0/24", + }, + }, + }}, + }}, + }, + }, + update: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.1.1/24", + "192.168.2.0/24", + "192.168.3.0/24", + }, + }, + }}, + }}, + }, + }, + }, } for testName, errorCase := range errorCases { - errorCase.old.ObjectMeta.ResourceVersion = "1" - errorCase.update.ObjectMeta.ResourceVersion = "1" - if errs := ValidateNetworkPolicyUpdate(&errorCase.update, &errorCase.old, NetworkPolicyValidationOptions{false}); len(errs) == 0 { - t.Errorf("expected failure: %s", testName) - } + t.Run(testName, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) + errorCase.old.ObjectMeta.ResourceVersion = "1" + errorCase.update.ObjectMeta.ResourceVersion = "1" + if errs := ValidateNetworkPolicyUpdate(&errorCase.update, &errorCase.old, NetworkPolicyValidationOptions{}); len(errs) == 0 { + t.Errorf("expected failure") + } + }) } } @@ -1805,6 +1958,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{ @@ -1814,26 +1975,56 @@ 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, + }, + "legacy IPs unchanged in update": { + oldValue: legacyIP, + newValue: legacyIP, + }, + } + 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 { - errs := ValidateIngressStatusUpdate(&v, &oldValue) - if len(errs) == 0 { - t.Errorf("expected failure for %s", k) - } else { - s := strings.Split(k, ":") - err := errs[0] - if err.Field != s[0] || !strings.Contains(err.Error(), s[1]) { - t.Errorf("unexpected error: %q, expected: %q", err, k) + t.Run(k, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) + + errs := ValidateIngressStatusUpdate(&v, &oldValue) + if len(errs) == 0 { + t.Errorf("expected failure") + } else { + 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) + } } - } + }) } } @@ -2150,13 +2341,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/apis/resource/validation/validation.go b/pkg/apis/resource/validation/validation.go index 5853e79acef..0a1b1ce429f 100644 --- a/pkg/apis/resource/validation/validation.go +++ b/pkg/apis/resource/validation/validation.go @@ -37,7 +37,6 @@ import ( "k8s.io/dynamic-resource-allocation/structured" corevalidation "k8s.io/kubernetes/pkg/apis/core/validation" "k8s.io/kubernetes/pkg/apis/resource" - netutils "k8s.io/utils/net" ) var ( @@ -890,25 +889,7 @@ func validateNetworkDeviceData(networkDeviceData *resource.NetworkDeviceData, fl allErrs = append(allErrs, validateSet(networkDeviceData.IPs, resource.NetworkDeviceDataMaxIPs, func(address string, fldPath *field.Path) field.ErrorList { - // reformat CIDR to handle different ways IPs can be written - // (e.g. 2001:db8::1/64 == 2001:0db8::1/64) - ip, ipNet, err := netutils.ParseCIDRSloppy(address) - if err != nil { - // must fail - return validation.IsValidCIDR(fldPath, address) - } - maskSize, _ := ipNet.Mask.Size() - canonical := fmt.Sprintf("%s/%d", ip.String(), maskSize) - if address != canonical { - return field.ErrorList{ - field.Invalid(fldPath, address, fmt.Sprintf("must be in canonical form (%s)", canonical)), - } - } - return nil - }, - func(address string) (string, string) { - return address, "" - }, - fldPath.Child("ips"))...) + return validation.IsValidInterfaceAddress(fldPath, address) + }, stringKey, fldPath.Child("ips"))...) return allErrs } diff --git a/pkg/apis/resource/validation/validation_resourceclaim_test.go b/pkg/apis/resource/validation/validation_resourceclaim_test.go index 1a243920e4f..7224fc82f1f 100644 --- a/pkg/apis/resource/validation/validation_resourceclaim_test.go +++ b/pkg/apis/resource/validation/validation_resourceclaim_test.go @@ -1257,9 +1257,9 @@ func TestValidateClaimStatusUpdate(t *testing.T) { wantFailures: field.ErrorList{ field.TooLong(field.NewPath("status", "devices").Index(0).Child("networkData", "interfaceName"), "", resource.NetworkDeviceDataInterfaceNameMaxLength), field.TooLong(field.NewPath("status", "devices").Index(0).Child("networkData", "hardwareAddress"), "", resource.NetworkDeviceDataHardwareAddressMaxLength), - field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(0), "300.9.8.0/24", "must be a valid CIDR value, (e.g. 10.9.8.0/24 or 2001:db8::/64)"), - field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(1), "010.009.008.000/24", "must be in canonical form (10.9.8.0/24)"), - field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(2), "2001:0db8::1/64", "must be in canonical form (2001:db8::1/64)"), + field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(0), "300.9.8.0/24", "must be a valid address in CIDR form, (e.g. 10.9.8.7/24 or 2001:db8::1/64)"), + field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(1), "010.009.008.000/24", "must be in canonical form (\"10.9.8.0/24\")"), + field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(2), "2001:0db8::1/64", "must be in canonical form (\"2001:db8::1/64\")"), }, oldClaim: func() *resource.ResourceClaim { return validAllocatedClaim }(), update: func(claim *resource.ResourceClaim) *resource.ResourceClaim { @@ -1390,7 +1390,7 @@ func TestValidateClaimStatusUpdate(t *testing.T) { wantFailures: field.ErrorList{ field.TooLong(field.NewPath("status", "devices").Index(0).Child("networkData", "interfaceName"), "", resource.NetworkDeviceDataInterfaceNameMaxLength), field.TooLong(field.NewPath("status", "devices").Index(0).Child("networkData", "hardwareAddress"), "", resource.NetworkDeviceDataHardwareAddressMaxLength), - field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(0), "300.9.8.0/24", "must be a valid CIDR value, (e.g. 10.9.8.0/24 or 2001:db8::/64)"), + field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(0), "300.9.8.0/24", "must be a valid address in CIDR form, (e.g. 10.9.8.7/24 or 2001:db8::1/64)"), }, oldClaim: func() *resource.ResourceClaim { return validAllocatedClaim }(), update: func(claim *resource.ResourceClaim) *resource.ResourceClaim { diff --git a/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 29579b2ccb5..36ce02c8480 100644 --- a/pkg/features/versioned_kube_features.go +++ b/pkg/features/versioned_kube_features.go @@ -766,6 +766,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/pkg/registry/core/service/storage/alloc.go b/pkg/registry/core/service/storage/alloc.go index a69261fb91f..8285f08804e 100644 --- a/pkg/registry/core/service/storage/alloc.go +++ b/pkg/registry/core/service/storage/alloc.go @@ -151,9 +151,7 @@ func (al *Allocators) initIPFamilyFields(after After, before Before) error { // Do some loose pre-validation of the input. This makes it easier in the // rest of allocation code to not have to consider corner cases. - // TODO(thockin): when we tighten validation (e.g. to require IPs) we will - // need a "strict" and a "loose" form of this. - if el := validation.ValidateServiceClusterIPsRelatedFields(service); len(el) != 0 { + if el := validation.ValidateServiceClusterIPsRelatedFields(service, oldService); len(el) != 0 { return errors.NewInvalid(api.Kind("Service"), service.Name, el) } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go index f073c3668ca..6e947c74c9e 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go @@ -18,28 +18,94 @@ package validation import ( "fmt" + "net" "net/netip" + "slices" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/klog/v2" 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.) +// +// Alternatively, when validating an update to an existing field, you can pass a list of +// IP values from the old object that should be accepted if they appear in the new object +// even if they are not valid. +// +// This function should only be used to validate the existing fields that were +// historically validated in this way, and strictValidation should be true unless the +// StrictIPCIDRValidation feature gate is disabled. Use IsValidIP for parsing new fields. +func IsValidIPForLegacyField(fldPath *field.Path, value string, strictValidation bool, validOldIPs []string) field.ErrorList { + if slices.Contains(validOldIPs, value) { + return nil + } + _, 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 } @@ -64,42 +130,88 @@ func GetWarningsForIP(fldPath *field.Path, value string) []string { return nil } -// IsValidIPv4Address tests that the argument is a valid IPv4 address. -func IsValidIPv4Address(fldPath *field.Path, value string) field.ErrorList { +func parseCIDR(fldPath *field.Path, value string, strictValidation bool) (*net.IPNet, field.ErrorList) { var allErrors field.ErrorList - ip := netutils.ParseIPSloppy(value) - if ip == nil || ip.To4() == nil { - allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid IPv4 address")) - } - return allErrors -} -// IsValidIPv6Address tests that the argument is a valid IPv6 address. -func IsValidIPv6Address(fldPath *field.Path, value string) field.ErrorList { - var allErrors field.ErrorList - ip := netutils.ParseIPSloppy(value) - if ip == nil || ip.To4() != nil { - allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid IPv6 address")) - } - return allErrors -} - -// IsValidCIDR tests that the argument is a valid CIDR value. -func IsValidCIDR(fldPath *field.Path, value string) field.ErrorList { - var allErrors field.ErrorList - _, _, 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. +// +// Alternatively, when validating an update to an existing field, you can pass a list of +// CIDR values from the old object that should be accepted if they appear in the new +// object even if they are not valid. +// +// This function should only be used to validate the existing fields that were +// historically validated in this way, and strictValidation should be true unless the +// StrictIPCIDRValidation feature gate is disabled. Use IsValidCIDR or +// IsValidInterfaceAddress for parsing new fields. +func IsValidCIDRForLegacyField(fldPath *field.Path, value string, strictValidation bool, validOldCIDRs []string) field.ErrorList { + if slices.Contains(validOldCIDRs, value) { + return nil + } + + _, 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 } @@ -137,3 +249,30 @@ func GetWarningsForCIDR(fldPath *field.Path, value string) []string { return warnings } + +// IsValidInterfaceAddress tests that the argument is a valid "ifaddr"-style CIDR value in +// canonical form (e.g., "192.168.1.5/24", with a complete IP address and associated +// subnet length). Use IsValidCIDR for "subnet"/"mask"-style CIDR values (e.g., +// "192.168.1.0/24"). +func IsValidInterfaceAddress(fldPath *field.Path, value string) field.ErrorList { + var allErrors field.ErrorList + ip, ipnet, err := netutils.ParseCIDRSloppy(value) + if err != nil { + allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid address in CIDR form, (e.g. 10.9.8.7/24 or 2001:db8::1/64)")) + return allErrors + } + + // The canonical form of `value` is not `ipnet.String()`, because `ipnet` doesn't + // include the bits after the prefix. We need to construct the canonical form + // ourselves from `ip` and `ipnet.Mask`. + maskSize, _ := ipnet.Mask.Size() + if netutils.IsIPv4(ip) && maskSize > net.IPv4len*8 { + // "::ffff:192.168.0.1/120" -> "192.168.0.1/24" + maskSize -= (net.IPv6len - net.IPv4len) * 8 + } + canonical := fmt.Sprintf("%s/%d", ip.String(), maskSize) + if value != canonical { + allErrors = append(allErrors, field.Invalid(fldPath, value, fmt.Sprintf("must be in canonical form (%q)", canonical))) + } + return allErrors +} diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip_test.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip_test.go index 6fb5d4f15cc..caf5cf1fe51 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip_test.go @@ -25,140 +25,193 @@ import ( ) func TestIsValidIP(t *testing.T) { - for _, tc := range []struct { - name string - in string - family int - err string + testCases := []struct { + name string + in string + + err string + legacyErr string + legacyStrictErr string }{ // GOOD VALUES { - name: "ipv4", - in: "1.2.3.4", - family: 4, + name: "ipv4", + in: "1.2.3.4", }, { - name: "ipv4, all zeros", - in: "0.0.0.0", - family: 4, + name: "ipv4, all zeros", + in: "0.0.0.0", }, { - name: "ipv4, max", - in: "255.255.255.255", - family: 4, + name: "ipv4, max", + in: "255.255.255.255", }, { - name: "ipv6", - in: "1234::abcd", - family: 6, + name: "ipv6", + in: "1234::abcd", }, { - name: "ipv6, all zeros, collapsed", - in: "::", - family: 6, + name: "ipv6, all zeros, collapsed", + in: "::", }, { - name: "ipv6, max", - in: "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", - family: 6, + name: "ipv6, max", + in: "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", }, - // GOOD, THOUGH NON-CANONICAL, VALUES + // NON-CANONICAL VALUES { - name: "ipv6, all zeros, expanded (non-canonical)", - in: "0:0:0:0:0:0:0:0", - family: 6, + name: "ipv6, all zeros, expanded (non-canonical)", + in: "0:0:0:0:0:0:0:0", + + err: `must be in canonical form ("::")`, }, { - name: "ipv6, leading 0s (non-canonical)", - in: "0001:002:03:4::", - family: 6, + 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", - family: 6, + 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", - family: 4, + 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", - family: 4, + 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", }, - } { + } + + var badIPs []string + for _, tc := range testCases { + if tc.legacyStrictErr != "" { + badIPs = append(badIPs, tc.in) + } + } + + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { errs := IsValidIP(field.NewPath(""), tc.in) if tc.err == "" { @@ -173,27 +226,36 @@ func TestIsValidIP(t *testing.T) { } } - errs = IsValidIPv4Address(field.NewPath(""), tc.in) - if tc.family == 4 { + errs = IsValidIPForLegacyField(field.NewPath(""), tc.in, false, nil) + if tc.legacyErr == "" { if len(errs) != 0 { - t.Errorf("expected %q to pass IsValidIPv4Address but got: %v", tc.in, errs) + t.Errorf("expected %q to be valid according to IsValidIPForLegacyField but got: %v", tc.in, errs) } } else { - if len(errs) == 0 { - t.Errorf("expected %q to fail IsValidIPv4Address", tc.in) + 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 = IsValidIPv6Address(field.NewPath(""), tc.in) - if tc.family == 6 { + errs = IsValidIPForLegacyField(field.NewPath(""), tc.in, true, nil) + if tc.legacyStrictErr == "" { if len(errs) != 0 { - t.Errorf("expected %q to pass IsValidIPv6Address but got: %v", tc.in, errs) + t.Errorf("expected %q to be valid according to IsValidIPForLegacyField with strict validation, but got: %v", tc.in, errs) } } else { - if len(errs) == 0 { - t.Errorf("expected %q to fail IsValidIPv6Address", tc.in) + 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) } } + + errs = IsValidIPForLegacyField(field.NewPath(""), tc.in, true, badIPs) + if len(errs) != 0 { + t.Errorf("expected %q to be accepted when using validOldIPs, but got: %v", tc.in, errs) + } }) } } @@ -252,10 +314,13 @@ func TestGetWarningsForIP(t *testing.T) { } func TestIsValidCIDR(t *testing.T) { - for _, tc := range []struct { + testCases := []struct { name string in string - err string + + err string + legacyErr string + legacyStrictErr string }{ // GOOD VALUES { @@ -283,79 +348,137 @@ 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", }, - } { + } + + var badCIDRs []string + for _, tc := range testCases { + if tc.legacyStrictErr != "" { + badCIDRs = append(badCIDRs, tc.in) + } + } + + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { errs := IsValidCIDR(field.NewPath(""), tc.in) if tc.err == "" { @@ -369,6 +492,37 @@ 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, nil) + 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, nil) + if tc.legacyStrictErr == "" { + if len(errs) != 0 { + t.Errorf("expected %q to be valid according to IsValidCIDRForLegacyField with strict validation but got: %v", tc.in, errs) + } + } 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) + } + } + + errs = IsValidCIDRForLegacyField(field.NewPath(""), tc.in, true, badCIDRs) + if len(errs) != 0 { + t.Errorf("expected %q to be accepted when using validOldCIDRs, but got: %v", tc.in, errs) + } }) } } @@ -450,3 +604,106 @@ func TestGetWarningsForCIDR(t *testing.T) { }) } } + +func TestIsValidInterfaceAddress(t *testing.T) { + for _, tc := range []struct { + name string + in string + err string + }{ + // GOOD VALUES + { + name: "ipv4", + in: "1.2.3.4/24", + }, + { + name: "ipv4, single IP", + in: "1.1.1.1/32", + }, + { + name: "ipv6", + in: "2001:4860:4860::1/48", + }, + { + name: "ipv6, single IP", + in: "::1/128", + }, + + // BAD VALUES + { + name: "empty string", + in: "", + err: "must be a valid address in CIDR form", + }, + { + name: "junk", + in: "aaaaaaa", + err: "must be a valid address in CIDR form", + }, + { + name: "IP address", + in: "1.2.3.4", + err: "must be a valid address in CIDR form", + }, + { + name: "partial URL", + in: "192.168.0.1/healthz", + err: "must be a valid address in CIDR form", + }, + { + name: "partial URL 2", + in: "192.168.0.1/0/99", + err: "must be a valid address in CIDR form", + }, + { + name: "negative prefix length", + in: "192.168.0.0/-16", + err: "must be a valid address in CIDR form", + }, + { + name: "prefix length with sign", + in: "192.168.0.0/+16", + err: "must be a valid address in CIDR form", + }, + { + name: "ipv6 non-canonical", + in: "2001:0:0:0::0BCD/64", + err: `must be in canonical form ("2001::bcd/64")`, + }, + { + name: "ipv4 with leading 0s", + in: "1.1.01.002/24", + err: `must be in canonical form ("1.1.1.2/24")`, + }, + { + name: "ipv4-in-ipv6 with ipv4-sized prefix", + in: "::ffff:1.1.1.1/24", + err: `must be in canonical form ("1.1.1.1/24")`, + }, + { + name: "ipv4-in-ipv6 with ipv6-sized prefix", + in: "::ffff:1.1.1.1/120", + err: `must be in canonical form ("1.1.1.1/24")`, + }, + { + name: "prefix length with leading 0s", + in: "192.168.0.5/016", + err: `must be in canonical form ("192.168.0.5/16")`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + errs := IsValidInterfaceAddress(field.NewPath(""), tc.in) + if tc.err == "" { + if len(errs) != 0 { + t.Errorf("expected %q to be valid but got: %v", tc.in, errs) + } + } else { + if len(errs) != 1 { + t.Errorf("expected %q to have 1 error but got: %v", tc.in, errs) + } else if !strings.Contains(errs[0].Detail, tc.err) { + t.Errorf("expected error for %q to contain %q but got: %q", tc.in, tc.err, errs[0].Detail) + } + } + }) + } +} diff --git a/test/featuregates_linter/test_data/versioned_feature_list.yaml b/test/featuregates_linter/test_data/versioned_feature_list.yaml index 56de7c0ce60..fa9db33fc57 100644 --- a/test/featuregates_linter/test_data/versioned_feature_list.yaml +++ b/test/featuregates_linter/test_data/versioned_feature_list.yaml @@ -1388,6 +1388,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