diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 68166359257..116cbf85455 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3790,7 +3790,7 @@ func validatePodDNSConfig(dnsConfig *core.PodDNSConfig, dnsPolicy *core.DNSPolic allErrs = append(allErrs, field.Invalid(fldPath.Child("nameservers"), dnsConfig.Nameservers, fmt.Sprintf("must not have more than %v nameservers", MaxDNSNameservers))) } for i, ns := range dnsConfig.Nameservers { - allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Child("nameservers").Index(i), ns)...) + allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Child("nameservers").Index(i), ns, nil)...) } // Validate searches. if len(dnsConfig.Searches) > MaxDNSSearchPaths { @@ -3966,7 +3966,7 @@ func validateOnlyDeletedSchedulingGates(newGates, oldGates []core.PodSchedulingG func ValidateHostAliases(hostAliases []core.HostAlias, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for i, hostAlias := range hostAliases { - allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Index(i).Child("ip"), hostAlias.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Index(i).Child("ip"), hostAlias.IP, nil)...) for j, hostname := range hostAlias.Hostnames { allErrs = append(allErrs, ValidateDNS1123Subdomain(hostname, fldPath.Index(i).Child("hostnames").Index(j))...) } @@ -4108,14 +4108,21 @@ func validatePodMetadataAndSpec(pod *core.Pod, opts PodValidationOptions) field. } // validatePodIPs validates IPs in pod status -func validatePodIPs(pod *core.Pod) field.ErrorList { +func validatePodIPs(pod, oldPod *core.Pod) field.ErrorList { allErrs := field.ErrorList{} podIPsField := field.NewPath("status", "podIPs") - // all PodIPs must be valid IPs + // all new PodIPs must be valid IPs, but existing invalid ones can be kept. + var existingPodIPs []string + if oldPod != nil { + existingPodIPs = make([]string, len(oldPod.Status.PodIPs)) + for i, podIP := range oldPod.Status.PodIPs { + existingPodIPs[i] = podIP.IP + } + } for i, podIP := range pod.Status.PodIPs { - allErrs = append(allErrs, IsValidIPForLegacyField(podIPsField.Index(i), podIP.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(podIPsField.Index(i), podIP.IP, existingPodIPs)...) } // if we have more than one Pod.PodIP then we must have a dual-stack pair @@ -4140,7 +4147,7 @@ func validatePodIPs(pod *core.Pod) field.ErrorList { } // validateHostIPs validates IPs in pod status -func validateHostIPs(pod *core.Pod) field.ErrorList { +func validateHostIPs(pod, oldPod *core.Pod) field.ErrorList { allErrs := field.ErrorList{} if len(pod.Status.HostIPs) == 0 { @@ -4154,9 +4161,16 @@ func validateHostIPs(pod *core.Pod) field.ErrorList { allErrs = append(allErrs, field.Invalid(hostIPsField.Index(0).Child("ip"), pod.Status.HostIPs[0].IP, "must be equal to `hostIP`")) } - // all HostIPs must be valid IPs + // all new HostIPs must be valid IPs, but existing invalid ones can be kept. + var existingHostIPs []string + if oldPod != nil { + existingHostIPs = make([]string, len(oldPod.Status.HostIPs)) + for i, hostIP := range oldPod.Status.HostIPs { + existingHostIPs[i] = hostIP.IP + } + } for i, hostIP := range pod.Status.HostIPs { - allErrs = append(allErrs, IsValidIPForLegacyField(hostIPsField.Index(i), hostIP.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(hostIPsField.Index(i), hostIP.IP, existingHostIPs)...) } // if we have more than one Pod.HostIP then we must have a dual-stack pair @@ -5462,11 +5476,11 @@ func ValidatePodStatusUpdate(newPod, oldPod *core.Pod, opts PodValidationOptions allErrs = append(allErrs, ValidateContainerStateTransition(newPod.Status.EphemeralContainerStatuses, oldPod.Status.EphemeralContainerStatuses, fldPath.Child("ephemeralContainerStatuses"), core.RestartPolicyNever)...) allErrs = append(allErrs, validatePodResourceClaimStatuses(newPod.Status.ResourceClaimStatuses, newPod.Spec.ResourceClaims, fldPath.Child("resourceClaimStatuses"))...) - if newIPErrs := validatePodIPs(newPod); len(newIPErrs) > 0 { + if newIPErrs := validatePodIPs(newPod, oldPod); len(newIPErrs) > 0 { allErrs = append(allErrs, newIPErrs...) } - if newIPErrs := validateHostIPs(newPod); len(newIPErrs) > 0 { + if newIPErrs := validateHostIPs(newPod, oldPod); len(newIPErrs) > 0 { allErrs = append(allErrs, newIPErrs...) } @@ -5860,7 +5874,7 @@ var supportedServiceIPFamilyPolicy = sets.New( core.IPFamilyPolicyRequireDualStack) // ValidateService tests if required fields/annotations of a Service are valid. -func ValidateService(service *core.Service) field.ErrorList { +func ValidateService(service, oldService *core.Service) field.ErrorList { metaPath := field.NewPath("metadata") allErrs := ValidateObjectMeta(&service.ObjectMeta, true, ValidateServiceName, metaPath) @@ -5935,12 +5949,19 @@ func ValidateService(service *core.Service) field.ErrorList { } // dualstack <-> ClusterIPs <-> ipfamilies - allErrs = append(allErrs, ValidateServiceClusterIPsRelatedFields(service)...) + allErrs = append(allErrs, ValidateServiceClusterIPsRelatedFields(service, oldService)...) + // All new ExternalIPs must be valid and "non-special". (Existing ExternalIPs may + // have been validated against older rules, but if we allowed them before we can't + // reject them now.) ipPath := specPath.Child("externalIPs") + var existingExternalIPs []string + if oldService != nil { + existingExternalIPs = oldService.Spec.ExternalIPs // +k8s:verify-mutation:reason=clone + } for i, ip := range service.Spec.ExternalIPs { idxPath := ipPath.Index(i) - if errs := IsValidIPForLegacyField(idxPath, ip); len(errs) != 0 { + if errs := IsValidIPForLegacyField(idxPath, ip, existingExternalIPs); len(errs) != 0 { allErrs = append(allErrs, errs...) } else { // For historical reasons, this uses ValidateEndpointIP even @@ -5997,18 +6018,27 @@ func ValidateService(service *core.Service) field.ErrorList { ports[key] = true } - // Validate SourceRanges field or annotation. + // Validate SourceRanges field or annotation. Existing invalid CIDR values do not + // need to be fixed. Note that even with the tighter CIDR validation we still + // allow excess whitespace, because that is effectively part of the API. if len(service.Spec.LoadBalancerSourceRanges) > 0 { fieldPath := specPath.Child("LoadBalancerSourceRanges") if service.Spec.Type != core.ServiceTypeLoadBalancer { allErrs = append(allErrs, field.Forbidden(fieldPath, "may only be used when `type` is 'LoadBalancer'")) } + var existingSourceRanges []string + if oldService != nil { + existingSourceRanges = make([]string, len(oldService.Spec.LoadBalancerSourceRanges)) + for i, value := range oldService.Spec.LoadBalancerSourceRanges { + existingSourceRanges[i] = strings.TrimSpace(value) + } + } for idx, value := range service.Spec.LoadBalancerSourceRanges { // Note: due to a historical accident around transition from the // annotation value, these values are allowed to be space-padded. value = strings.TrimSpace(value) - allErrs = append(allErrs, IsValidCIDRForLegacyField(fieldPath.Index(idx), value)...) + allErrs = append(allErrs, IsValidCIDRForLegacyField(fieldPath.Index(idx), value, existingSourceRanges)...) } } else if val, annotationSet := service.Annotations[core.AnnotationLoadBalancerSourceRangesKey]; annotationSet { fieldPath := field.NewPath("metadata", "annotations").Key(core.AnnotationLoadBalancerSourceRangesKey) @@ -6016,12 +6046,14 @@ func ValidateService(service *core.Service) field.ErrorList { allErrs = append(allErrs, field.Forbidden(fieldPath, "may only be used when `type` is 'LoadBalancer'")) } - val = strings.TrimSpace(val) - if val != "" { - cidrs := strings.Split(val, ",") - for _, value := range cidrs { - value = strings.TrimSpace(value) - allErrs = append(allErrs, IsValidCIDRForLegacyField(fieldPath, value)...) + if oldService == nil || oldService.Annotations[core.AnnotationLoadBalancerSourceRangesKey] != val { + val = strings.TrimSpace(val) + if val != "" { + cidrs := strings.Split(val, ",") + for _, value := range cidrs { + value = strings.TrimSpace(value) + allErrs = append(allErrs, IsValidCIDRForLegacyField(fieldPath, value, nil)...) + } } } } @@ -6181,7 +6213,7 @@ func validateServiceTrafficDistribution(service *core.Service) field.ErrorList { // ValidateServiceCreate validates Services as they are created. func ValidateServiceCreate(service *core.Service) field.ErrorList { - return ValidateService(service) + return ValidateService(service, nil) } // ValidateServiceUpdate tests if required fields in the service are set during an update @@ -6204,13 +6236,13 @@ func ValidateServiceUpdate(service, oldService *core.Service) field.ErrorList { allErrs = append(allErrs, validateServiceExternalTrafficFieldsUpdate(oldService, service)...) - return append(allErrs, ValidateService(service)...) + return append(allErrs, ValidateService(service, oldService)...) } // ValidateServiceStatusUpdate tests if required fields in the Service are set when updating status. func ValidateServiceStatusUpdate(service, oldService *core.Service) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&service.ObjectMeta, &oldService.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateLoadBalancerStatus(&service.Status.LoadBalancer, field.NewPath("status", "loadBalancer"), &service.Spec)...) + allErrs = append(allErrs, ValidateLoadBalancerStatus(&service.Status.LoadBalancer, &oldService.Status.LoadBalancer, field.NewPath("status", "loadBalancer"), &service.Spec)...) return allErrs } @@ -6405,7 +6437,7 @@ func ValidateNode(node *core.Node) field.ErrorList { // all PodCIDRs should be valid ones for idx, value := range node.Spec.PodCIDRs { - allErrs = append(allErrs, IsValidCIDRForLegacyField(podCIDRsField.Index(idx), value)...) + allErrs = append(allErrs, IsValidCIDRForLegacyField(podCIDRsField.Index(idx), value, nil)...) } // if more than PodCIDR then it must be a dual-stack pair @@ -7432,16 +7464,28 @@ func ValidateNamespaceFinalizeUpdate(newNamespace, oldNamespace *core.Namespace) } // ValidateEndpoints validates Endpoints on create and update. -func ValidateEndpoints(endpoints *core.Endpoints) field.ErrorList { +func ValidateEndpoints(endpoints, oldEndpoints *core.Endpoints) field.ErrorList { allErrs := ValidateObjectMeta(&endpoints.ObjectMeta, true, ValidateEndpointsName, field.NewPath("metadata")) allErrs = append(allErrs, ValidateEndpointsSpecificAnnotations(endpoints.Annotations, field.NewPath("annotations"))...) - allErrs = append(allErrs, validateEndpointSubsets(endpoints.Subsets, field.NewPath("subsets"))...) + + subsetErrs := validateEndpointSubsets(endpoints.Subsets, field.NewPath("subsets")) + if len(subsetErrs) != 0 { + // If this is an update, and Subsets was unchanged, then ignore the + // validation errors, since apparently older versions of Kubernetes + // considered the data valid. (We only check this after getting a + // validation error since Endpoints may be large and DeepEqual is slow.) + if oldEndpoints != nil && apiequality.Semantic.DeepEqual(oldEndpoints.Subsets, endpoints.Subsets) { + subsetErrs = nil + } + } + allErrs = append(allErrs, subsetErrs...) + return allErrs } // ValidateEndpointsCreate validates Endpoints on create. func ValidateEndpointsCreate(endpoints *core.Endpoints) field.ErrorList { - return ValidateEndpoints(endpoints) + return ValidateEndpoints(endpoints, nil) } // ValidateEndpointsUpdate validates Endpoints on update. NodeName changes are @@ -7450,7 +7494,7 @@ func ValidateEndpointsCreate(endpoints *core.Endpoints) field.ErrorList { // happens. func ValidateEndpointsUpdate(newEndpoints, oldEndpoints *core.Endpoints) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&newEndpoints.ObjectMeta, &oldEndpoints.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateEndpoints(newEndpoints)...) + allErrs = append(allErrs, ValidateEndpoints(newEndpoints, oldEndpoints)...) return allErrs } @@ -7481,7 +7525,7 @@ func validateEndpointSubsets(subsets []core.EndpointSubset, fldPath *field.Path) func validateEndpointAddress(address *core.EndpointAddress, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Child("ip"), address.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(fldPath.Child("ip"), address.IP, nil)...) if len(address.Hostname) > 0 { allErrs = append(allErrs, ValidateDNS1123Label(address.Hostname, fldPath.Child("hostname"))...) } @@ -7844,16 +7888,25 @@ var ( ) // ValidateLoadBalancerStatus validates required fields on a LoadBalancerStatus -func ValidateLoadBalancerStatus(status *core.LoadBalancerStatus, fldPath *field.Path, spec *core.ServiceSpec) field.ErrorList { +func ValidateLoadBalancerStatus(status, oldStatus *core.LoadBalancerStatus, fldPath *field.Path, spec *core.ServiceSpec) field.ErrorList { allErrs := field.ErrorList{} ingrPath := fldPath.Child("ingress") if !utilfeature.DefaultFeatureGate.Enabled(features.AllowServiceLBStatusOnNonLB) && spec.Type != core.ServiceTypeLoadBalancer && len(status.Ingress) != 0 { allErrs = append(allErrs, field.Forbidden(ingrPath, "may only be used when `spec.type` is 'LoadBalancer'")) } else { + var existingIngressIPs []string + if oldStatus != nil { + existingIngressIPs = make([]string, 0, len(oldStatus.Ingress)) + for _, ingress := range oldStatus.Ingress { + if len(ingress.IP) > 0 { + existingIngressIPs = append(existingIngressIPs, ingress.IP) + } + } + } for i, ingress := range status.Ingress { idxPath := ingrPath.Index(i) if len(ingress.IP) > 0 { - allErrs = append(allErrs, IsValidIPForLegacyField(idxPath.Child("ip"), ingress.IP)...) + allErrs = append(allErrs, IsValidIPForLegacyField(idxPath.Child("ip"), ingress.IP, existingIngressIPs)...) } if utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) && ingress.IPMode == nil { @@ -8120,7 +8173,7 @@ func validateLabelKeys(fldPath *field.Path, labelKeys []string, labelSelector *m // ValidateServiceClusterIPsRelatedFields validates .spec.ClusterIPs,, // .spec.IPFamilies, .spec.ipFamilyPolicy. This is exported because it is used // during IP init and allocation. -func ValidateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorList { +func ValidateServiceClusterIPsRelatedFields(service, oldService *core.Service) field.ErrorList { // ClusterIP, ClusterIPs, IPFamilyPolicy and IPFamilies are validated prior (all must be unset) for ExternalName service if service.Spec.Type == core.ServiceTypeExternalName { return field.ErrorList{} @@ -8174,6 +8227,11 @@ func ValidateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorLi } } + var existingClusterIPs []string + if oldService != nil { + existingClusterIPs = oldService.Spec.ClusterIPs // +k8s:verify-mutation:reason=clone + } + // clusterIPs stand alone validation // valid ips with None and empty string handling // duplication check is done as part of DualStackvalidation below @@ -8187,8 +8245,8 @@ func ValidateServiceClusterIPsRelatedFields(service *core.Service) field.ErrorLi continue } - // is it valid ip? - errorMessages := IsValidIPForLegacyField(clusterIPsField.Index(i), clusterIP) + // is it a valid ip? (or was it at least previously considered valid?) + errorMessages := IsValidIPForLegacyField(clusterIPsField.Index(i), clusterIP, existingClusterIPs) hasInvalidIPs = (len(errorMessages) != 0) || hasInvalidIPs allErrs = append(allErrs, errorMessages...) } @@ -8707,13 +8765,13 @@ func isRestartableInitContainer(initContainer *core.Container) bool { // IsValidIPForLegacyField is a wrapper around validation.IsValidIPForLegacyField that // handles setting strictValidation correctly. This is only for fields that use legacy IP // address validation; use validation.IsValidIP for new fields. -func IsValidIPForLegacyField(fldPath *field.Path, value string) field.ErrorList { - return validation.IsValidIPForLegacyField(fldPath, value, utilfeature.DefaultFeatureGate.Enabled(features.StrictIPCIDRValidation)) +func IsValidIPForLegacyField(fldPath *field.Path, value string, validOldIPs []string) field.ErrorList { + return validation.IsValidIPForLegacyField(fldPath, value, utilfeature.DefaultFeatureGate.Enabled(features.StrictIPCIDRValidation), validOldIPs) } // IsValidCIDRForLegacyField is a wrapper around validation.IsValidCIDRForLegacyField that // handles setting strictValidation correctly. This is only for fields that use legacy CIDR // value validation; use validation.IsValidCIDR for new fields. -func IsValidCIDRForLegacyField(fldPath *field.Path, value string) field.ErrorList { - return validation.IsValidCIDRForLegacyField(fldPath, value, utilfeature.DefaultFeatureGate.Enabled(features.StrictIPCIDRValidation)) +func IsValidCIDRForLegacyField(fldPath *field.Path, value string, validOldCIDRs []string) field.ErrorList { + return validation.IsValidCIDRForLegacyField(fldPath, value, utilfeature.DefaultFeatureGate.Enabled(features.StrictIPCIDRValidation), validOldCIDRs) } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 13226465734..d3f199bb9a5 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -18892,10 +18892,212 @@ func TestValidateServiceUpdate(t *testing.T) { }, numErrs: 1, }, + + // IP validation ratcheting tests. Note: this is tested for + // LoadBalancerStatus in TestValidateLoadBalancerStatus. + { + name: "pre-existing invalid clusterIP ignored when updating other fields", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ClusterIP = "1.2.3.04" + oldSvc.Spec.ClusterIPs = []string{"1.2.3.04"} + newSvc.Spec.ClusterIP = "1.2.3.04" + newSvc.Spec.ClusterIPs = []string{"1.2.3.04"} + newSvc.Labels["foo"] = "bar" + }, + numErrs: 0, + }, { + name: "pre-existing invalid clusterIP ignored when adding clusterIPs", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ClusterIP = "1.2.3.04" + oldSvc.Spec.ClusterIPs = []string{"1.2.3.04"} + oldSvc.Spec.IPFamilyPolicy = &singleStack + oldSvc.Spec.IPFamilies = []core.IPFamily{core.IPv4Protocol} + + newSvc.Spec.ClusterIP = "1.2.3.04" + newSvc.Spec.ClusterIPs = []string{"1.2.3.04", "2001:db8::4"} + newSvc.Spec.IPFamilyPolicy = &requireDualStack + newSvc.Spec.IPFamilies = []core.IPFamily{core.IPv4Protocol, core.IPv6Protocol} + }, + numErrs: 0, + }, { + name: "pre-existing invalid clusterIP ignored when removing clusterIPs", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ClusterIP = "1.2.3.04" + oldSvc.Spec.ClusterIPs = []string{"1.2.3.04", "2001:db::4"} + oldSvc.Spec.IPFamilyPolicy = &requireDualStack + oldSvc.Spec.IPFamilies = []core.IPFamily{core.IPv4Protocol, core.IPv6Protocol} + + newSvc.Spec.ClusterIP = "1.2.3.04" + newSvc.Spec.ClusterIPs = []string{"1.2.3.04"} + newSvc.Spec.IPFamilyPolicy = &singleStack + newSvc.Spec.IPFamilies = []core.IPFamily{core.IPv4Protocol} + }, + numErrs: 0, + }, { + name: "pre-existing invalid externalIP ignored", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + oldSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + newSvc.Labels["foo"] = "bar" + }, + numErrs: 0, + }, { + name: "pre-existing invalid externalIP ignored when adding externalIPs", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + oldSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"5.6.7.8", "1.2.3.04"} + }, + numErrs: 0, + }, { + name: "pre-existing invalid externalIP ignored when removing externalIPs", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + oldSvc.Spec.ExternalIPs = []string{"5.6.7.8", "1.2.3.04"} + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + }, + numErrs: 0, + }, { + name: "can fix invalid externalIP", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + oldSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"1.2.3.4"} + }, + numErrs: 0, + }, { + name: "can't add invalid externalIP", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.ExternalIPs = []string{"1.2.3.04"} + }, + numErrs: 1, + }, { + name: "pre-existing invalid loadBalancerSourceRanges ignored when updating other fields", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + newSvc.Labels["foo"] = "bar" + }, + numErrs: 0, + }, { + name: "pre-existing invalid loadBalancerSourceRanges ignored when adding source ranges", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.LoadBalancerSourceRanges = []string{"1.2.3.0/24", "010.0.0.0/8"} + }, + numErrs: 0, + }, { + name: "pre-existing invalid loadBalancerSourceRanges ignored when removing source ranges", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"1.2.3.0/24", "010.0.0.0/8"} + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + }, + numErrs: 0, + }, { + name: "can fix invalid loadBalancerSourceRanges", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.LoadBalancerSourceRanges = []string{"10.0.0.0/8"} + }, + numErrs: 0, + }, { + name: "can't add invalid loadBalancerSourceRanges", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.LoadBalancerSourceRanges = []string{"010.0.0.0/8"} + }, + numErrs: 1, + }, { + name: "pre-existing invalid source ranges ignored when updating other fields", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8" + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8" + newSvc.Labels["foo"] = "bar" + }, + numErrs: 0, + }, { + name: "can fix invalid source ranges annotation", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8" + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "10.0.0.0/8" + }, + numErrs: 0, + }, { + name: "can't modify pre-existing invalid source ranges annotation without fixing it", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + oldSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8" + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8, 1.2.3.0/24" + }, + numErrs: 1, + }, { + name: "can't add invalid source ranges annotation", + tweakSvc: func(oldSvc, newSvc *core.Service) { + oldSvc.Spec.Type = core.ServiceTypeLoadBalancer + oldSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Spec.Type = core.ServiceTypeLoadBalancer + newSvc.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + newSvc.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) + newSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8, 1.2.3.0/24" + }, + numErrs: 1, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) + oldSvc := makeValidService() newSvc := makeValidService() tc.tweakSvc(&oldSvc, &newSvc) @@ -22089,7 +22291,7 @@ func TestEndpointAddressNodeNameUpdateRestrictions(t *testing.T) { updatedEndpoint := newNodeNameEndpoint("kubernetes-changed-nodename") // Check that NodeName can be changed during update, this is to accommodate the case where nodeIP or PodCIDR is reused. // The same ip will now have a different nodeName. - errList := ValidateEndpoints(updatedEndpoint) + errList := ValidateEndpoints(updatedEndpoint, oldEndpoint) errList = append(errList, ValidateEndpointsUpdate(updatedEndpoint, oldEndpoint)...) if len(errList) != 0 { t.Error("Endpoint should allow changing of Subset.Addresses.NodeName on update") @@ -22099,7 +22301,7 @@ func TestEndpointAddressNodeNameUpdateRestrictions(t *testing.T) { func TestEndpointAddressNodeNameInvalidDNSSubdomain(t *testing.T) { // Check NodeName DNS validation endpoint := newNodeNameEndpoint("illegal*.nodename") - errList := ValidateEndpoints(endpoint) + errList := ValidateEndpoints(endpoint, nil) if len(errList) == 0 { t.Error("Endpoint should reject invalid NodeName") } @@ -22107,7 +22309,7 @@ func TestEndpointAddressNodeNameInvalidDNSSubdomain(t *testing.T) { func TestEndpointAddressNodeNameCanBeAnIPAddress(t *testing.T) { endpoint := newNodeNameEndpoint("10.10.1.1") - errList := ValidateEndpoints(endpoint) + errList := ValidateEndpoints(endpoint, nil) if len(errList) != 0 { t.Error("Endpoint should accept a NodeName that is an IP address") } @@ -23033,11 +23235,13 @@ func makePod(podName string, podNamespace string, podIPs []core.PodIP) core.Pod func TestPodIPsValidation(t *testing.T) { // We test updating every pod in testCases to every other pod in testCases. // expectError is true if we expect an error when updating *to* that pod. + // allowNoOpUpdate is true if we expect a no-op update to succeed. testCases := []struct { - pod core.Pod - legacyIPs bool - expectError bool + pod core.Pod + legacyIPs bool + expectError bool + allowNoOpUpdate bool }{ { expectError: false, @@ -23064,11 +23268,13 @@ func TestPodIPsValidation(t *testing.T) { }, /* failure cases start here */ { - expectError: true, - pod: makePod("invalid-pod-ip", "ns", []core.PodIP{{IP: "this-is-not-an-ip"}}), + expectError: true, + allowNoOpUpdate: true, + pod: makePod("invalid-pod-ip", "ns", []core.PodIP{{IP: "1.1.1.01"}}), }, { - expectError: true, - pod: makePod("legacy-pod-ip-with-strict-validation", "ns", []core.PodIP{{IP: "001.002.003.004"}}), + expectError: true, + allowNoOpUpdate: true, + pod: makePod("legacy-pod-ip-with-strict-validation", "ns", []core.PodIP{{IP: "001.002.003.004"}}), }, { expectError: true, pod: makePod("dualstack-same-ip-family-6", "ns", []core.PodIP{{IP: "::1"}, {IP: "::2"}}), @@ -23092,10 +23298,14 @@ func TestPodIPsValidation(t *testing.T) { }, } - for _, testCase := range testCases { + for i, testCase := range testCases { t.Run(testCase.pod.Name, func(t *testing.T) { featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !testCase.legacyIPs) - for _, oldTestCase := range testCases { + for j, oldTestCase := range testCases { + if oldTestCase.legacyIPs && !testCase.legacyIPs { + continue + } + newPod := testCase.pod.DeepCopy() newPod.ResourceVersion = "1" @@ -23105,7 +23315,11 @@ func TestPodIPsValidation(t *testing.T) { errs := ValidatePodStatusUpdate(newPod, oldPod, PodValidationOptions{}) - if len(errs) == 0 && testCase.expectError { + expectError := testCase.expectError + if testCase.allowNoOpUpdate && i == j { + expectError = false + } + if len(errs) == 0 && expectError { t.Fatalf("expected failure updating from %s, but there were none", oldTestCase.pod.Name) } if len(errs) != 0 && !testCase.expectError { @@ -23142,11 +23356,13 @@ func makePodWithHostIPs(podName string, podNamespace string, hostIPs []core.Host func TestHostIPsValidation(t *testing.T) { // We test updating every pod in testCases to every other pod in testCases. // expectError is true if we expect an error when updating *to* that pod. + // allowNoOpUpdate is true if we expect a no-op update to succeed. testCases := []struct { - pod core.Pod - legacyIPs bool - expectError bool + pod core.Pod + legacyIPs bool + expectError bool + allowNoOpUpdate bool }{ { expectError: false, @@ -23179,12 +23395,14 @@ func TestHostIPsValidation(t *testing.T) { }, /* failure cases start here */ { - expectError: true, - pod: makePodWithHostIPs("invalid-host-ip", "ns", []core.HostIP{{IP: "this-is-not-an-ip"}}), + expectError: true, + allowNoOpUpdate: true, + pod: makePodWithHostIPs("invalid-host-ip", "ns", []core.HostIP{{IP: "this-is-not-an-ip"}}), }, { - expectError: true, - pod: makePodWithHostIPs("invalid-legacy-host-ip-with-strict-validation", "ns", []core.HostIP{{IP: "001.002.003.004"}}), + expectError: true, + allowNoOpUpdate: true, + pod: makePodWithHostIPs("invalid-legacy-host-ip-with-strict-validation", "ns", []core.HostIP{{IP: "001.002.003.004"}}), }, { expectError: true, @@ -23213,10 +23431,14 @@ func TestHostIPsValidation(t *testing.T) { }, } - for _, testCase := range testCases { + for i, testCase := range testCases { t.Run(testCase.pod.Name, func(t *testing.T) { featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !testCase.legacyIPs) - for _, oldTestCase := range testCases { + for j, oldTestCase := range testCases { + if oldTestCase.legacyIPs && !testCase.legacyIPs { + continue + } + newPod := testCase.pod.DeepCopy() newPod.ResourceVersion = "1" @@ -23226,7 +23448,11 @@ func TestHostIPsValidation(t *testing.T) { errs := ValidatePodStatusUpdate(newPod, oldPod, PodValidationOptions{}) - if len(errs) == 0 && testCase.expectError { + expectError := testCase.expectError + if testCase.allowNoOpUpdate && i == j { + expectError = false + } + if len(errs) == 0 && expectError { t.Fatalf("expected failure updating from %s, but there were none", oldTestCase.pod.Name) } if len(errs) != 0 && !testCase.expectError { @@ -24782,13 +25008,14 @@ func TestValidateLoadBalancerStatus(t *testing.T) { ipModeDummy := core.LoadBalancerIPMode("dummy") testCases := []struct { - name string - ipModeEnabled bool - legacyIPs bool - nonLBAllowed bool - tweakLBStatus func(s *core.LoadBalancerStatus) - tweakSvcSpec func(s *core.ServiceSpec) - numErrs int + name string + ipModeEnabled bool + legacyIPs bool + nonLBAllowed bool + tweakOldLBStatus func(s *core.LoadBalancerStatus) + tweakLBStatus func(s *core.LoadBalancerStatus) + tweakSvcSpec func(s *core.ServiceSpec) + numErrs int }{ { name: "type is not LB", @@ -24892,6 +25119,54 @@ func TestValidateLoadBalancerStatus(t *testing.T) { }} }, numErrs: 1, + }, { + name: "invalid ingress IP ignored on update", + tweakOldLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }} + }, + tweakLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }} + }, + numErrs: 0, + }, { + name: "invalid ingress IP ignored when adding IP", + tweakOldLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }} + }, + tweakLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }, { + IP: "5.6.7.8", + IPMode: &ipModeVIP, + }} + }, + numErrs: 0, + }, { + name: "invalid ingress IP can be fixed", + tweakOldLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.04", + IPMode: &ipModeVIP, + }} + }, + tweakLBStatus: func(s *core.LoadBalancerStatus) { + s.Ingress = []core.LoadBalancerIngress{{ + IP: "1.2.3.4", + IPMode: &ipModeVIP, + }} + }, + numErrs: 0, }, } for _, tc := range testCases { @@ -24905,13 +25180,17 @@ func TestValidateLoadBalancerStatus(t *testing.T) { } featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LoadBalancerIPMode, tc.ipModeEnabled) featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AllowServiceLBStatusOnNonLB, tc.nonLBAllowed) + oldStatus := core.LoadBalancerStatus{} + if tc.tweakOldLBStatus != nil { + tc.tweakOldLBStatus(&oldStatus) + } status := core.LoadBalancerStatus{} tc.tweakLBStatus(&status) spec := core.ServiceSpec{Type: core.ServiceTypeLoadBalancer} if tc.tweakSvcSpec != nil { tc.tweakSvcSpec(&spec) } - errs := ValidateLoadBalancerStatus(&status, field.NewPath("status"), &spec) + errs := ValidateLoadBalancerStatus(&status, &oldStatus, field.NewPath("status"), &spec) if len(errs) != tc.numErrs { t.Errorf("Unexpected error list for case %q(expected:%v got %v) - Errors:\n %v", tc.name, tc.numErrs, len(errs), errs.ToAggregate()) } diff --git a/pkg/apis/discovery/validation/validation.go b/pkg/apis/discovery/validation/validation.go index a50cec5b43c..3a09837a8b5 100644 --- a/pkg/apis/discovery/validation/validation.go +++ b/pkg/apis/discovery/validation/validation.go @@ -20,6 +20,7 @@ import ( "fmt" corev1 "k8s.io/api/core/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation" metavalidation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/util/sets" @@ -55,23 +56,34 @@ var ( var ValidateEndpointSliceName = apimachineryvalidation.NameIsDNSSubdomain // ValidateEndpointSlice validates an EndpointSlice. -func ValidateEndpointSlice(endpointSlice *discovery.EndpointSlice) field.ErrorList { +func ValidateEndpointSlice(endpointSlice, oldEndpointSlice *discovery.EndpointSlice) field.ErrorList { allErrs := apivalidation.ValidateObjectMeta(&endpointSlice.ObjectMeta, true, ValidateEndpointSliceName, field.NewPath("metadata")) allErrs = append(allErrs, validateAddressType(endpointSlice.AddressType)...) - allErrs = append(allErrs, validateEndpoints(endpointSlice.Endpoints, endpointSlice.AddressType, field.NewPath("endpoints"))...) allErrs = append(allErrs, validatePorts(endpointSlice.Ports, field.NewPath("ports"))...) + endpointsErrs := validateEndpoints(endpointSlice.Endpoints, endpointSlice.AddressType, field.NewPath("endpoints")) + if len(endpointsErrs) != 0 { + // If this is an update, and Endpoints was unchanged, then ignore the + // validation errors, since apparently older versions of Kubernetes + // considered the data valid. (We only check this after getting a + // validation error since Endpoints may be large and DeepEqual is slow.) + if oldEndpointSlice != nil && apiequality.Semantic.DeepEqual(oldEndpointSlice.Endpoints, endpointSlice.Endpoints) { + endpointsErrs = nil + } + } + allErrs = append(allErrs, endpointsErrs...) + return allErrs } // ValidateEndpointSliceCreate validates an EndpointSlice when it is created. func ValidateEndpointSliceCreate(endpointSlice *discovery.EndpointSlice) field.ErrorList { - return ValidateEndpointSlice(endpointSlice) + return ValidateEndpointSlice(endpointSlice, nil) } // ValidateEndpointSliceUpdate validates an EndpointSlice when it is updated. func ValidateEndpointSliceUpdate(newEndpointSlice, oldEndpointSlice *discovery.EndpointSlice) field.ErrorList { - allErrs := ValidateEndpointSlice(newEndpointSlice) + allErrs := ValidateEndpointSlice(newEndpointSlice, oldEndpointSlice) allErrs = append(allErrs, apivalidation.ValidateImmutableField(newEndpointSlice.AddressType, oldEndpointSlice.AddressType, field.NewPath("addressType"))...) return allErrs @@ -100,7 +112,7 @@ func validateEndpoints(endpoints []discovery.Endpoint, addrType discovery.Addres // and do not get validated. switch addrType { case discovery.AddressTypeIPv4: - ipErrs := apivalidation.IsValidIPForLegacyField(addressPath.Index(i), address) + ipErrs := apivalidation.IsValidIPForLegacyField(addressPath.Index(i), address, nil) if len(ipErrs) > 0 { allErrs = append(allErrs, ipErrs...) } else { diff --git a/pkg/apis/discovery/validation/validation_test.go b/pkg/apis/discovery/validation/validation_test.go index 96830d51a9e..50a8bf2b69b 100644 --- a/pkg/apis/discovery/validation/validation_test.go +++ b/pkg/apis/discovery/validation/validation_test.go @@ -637,7 +637,7 @@ func TestValidateEndpointSlice(t *testing.T) { for name, testCase := range testCases { t.Run(name, func(t *testing.T) { featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !testCase.legacyIPs) - errs := ValidateEndpointSlice(testCase.endpointSlice) + errs := ValidateEndpointSlice(testCase.endpointSlice, nil) if len(errs) != testCase.expectedErrors { t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs) } @@ -737,6 +737,7 @@ func TestValidateEndpointSliceCreate(t *testing.T) { for name, testCase := range testCases { t.Run(name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) errs := ValidateEndpointSliceCreate(testCase.endpointSlice) if len(errs) != testCase.expectedErrors { t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs) @@ -765,6 +766,23 @@ func TestValidateEndpointSliceUpdate(t *testing.T) { }, expectedErrors: 0, }, + "unchanged IPs are not revalidated": { + oldEndpointSlice: &discovery.EndpointSlice{ + ObjectMeta: standardMeta, + AddressType: discovery.AddressTypeIPv4, + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"10.1.2.04"}, + }}, + }, + newEndpointSlice: &discovery.EndpointSlice{ + ObjectMeta: standardMeta, + AddressType: discovery.AddressTypeIPv4, + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"10.1.2.04"}, + }}, + }, + expectedErrors: 0, + }, // expected errors "invalid node name set": { @@ -823,10 +841,28 @@ func TestValidateEndpointSliceUpdate(t *testing.T) { }, expectedErrors: 1, }, + "changed IPs are revalidated": { + oldEndpointSlice: &discovery.EndpointSlice{ + ObjectMeta: standardMeta, + AddressType: discovery.AddressTypeIPv4, + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"10.1.2.3"}, + }}, + }, + newEndpointSlice: &discovery.EndpointSlice{ + ObjectMeta: standardMeta, + AddressType: discovery.AddressTypeIPv4, + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"10.1.2.03"}, + }}, + }, + expectedErrors: 1, + }, } for name, testCase := range testCases { t.Run(name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) errs := ValidateEndpointSliceUpdate(testCase.newEndpointSlice, testCase.oldEndpointSlice) if len(errs) != testCase.expectedErrors { t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs) diff --git a/pkg/apis/networking/validation/validation.go b/pkg/apis/networking/validation/validation.go index 8f00d0d8ffc..8bbad55ddc0 100644 --- a/pkg/apis/networking/validation/validation.go +++ b/pkg/apis/networking/validation/validation.go @@ -56,6 +56,7 @@ var ( type NetworkPolicyValidationOptions struct { AllowInvalidLabelValueInSelector bool + AllowCIDRsEvenIfInvalid []string } // ValidateNetworkPolicyName can be used to check whether the given networkpolicy @@ -118,7 +119,7 @@ func ValidateNetworkPolicyPeer(peer *networking.NetworkPolicyPeer, opts NetworkP } if peer.IPBlock != nil { numPeers++ - allErrs = append(allErrs, ValidateIPBlock(peer.IPBlock, peerPath.Child("ipBlock"))...) + allErrs = append(allErrs, ValidateIPBlock(peer.IPBlock, peerPath.Child("ipBlock"), opts)...) } if numPeers == 0 { @@ -206,20 +207,47 @@ func ValidationOptionsForNetworking(new, old *networking.NetworkPolicy) NetworkP // ValidateNetworkPolicyUpdate tests if an update to a NetworkPolicy is valid. func ValidateNetworkPolicyUpdate(update, old *networking.NetworkPolicy, opts NetworkPolicyValidationOptions) field.ErrorList { + // Record all existing CIDRs in the policy; see ValidateIPBlock. + var existingCIDRs []string + for _, ingress := range old.Spec.Ingress { + for _, peer := range ingress.From { + if peer.IPBlock != nil { + existingCIDRs = append(existingCIDRs, peer.IPBlock.CIDR) + existingCIDRs = append(existingCIDRs, peer.IPBlock.Except...) + } + } + } + for _, egress := range old.Spec.Egress { + for _, peer := range egress.To { + if peer.IPBlock != nil { + existingCIDRs = append(existingCIDRs, peer.IPBlock.CIDR) + existingCIDRs = append(existingCIDRs, peer.IPBlock.Except...) + } + } + } + opts.AllowCIDRsEvenIfInvalid = existingCIDRs + allErrs := field.ErrorList{} allErrs = append(allErrs, apivalidation.ValidateObjectMetaUpdate(&update.ObjectMeta, &old.ObjectMeta, field.NewPath("metadata"))...) allErrs = append(allErrs, ValidateNetworkPolicySpec(&update.Spec, opts, field.NewPath("spec"))...) return allErrs } -// ValidateIPBlock validates a cidr and the except fields of an IpBlock NetworkPolicyPeer -func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path) field.ErrorList { +// ValidateIPBlock validates a cidr and the except fields of an IPBlock NetworkPolicyPeer. +// +// If a pre-existing CIDR is invalid/insecure, but it is not being changed by this update, +// then we have to continue allowing it. But since the user may have changed the policy in +// arbitrary ways (adding/removing rules, adding/removing peers, adding/removing +// ipBlock.except values, etc), we can't reliably determine whether a CIDR value we see +// here is a new value or a pre-existing one. So we just allow any CIDR value that +// appeared in the old NetworkPolicy to be used here without revalidation. +func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path, opts NetworkPolicyValidationOptions) field.ErrorList { allErrs := field.ErrorList{} if ipb.CIDR == "" { allErrs = append(allErrs, field.Required(fldPath.Child("cidr"), "")) return allErrs } - allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(fldPath.Child("cidr"), ipb.CIDR)...) + allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(fldPath.Child("cidr"), ipb.CIDR, opts.AllowCIDRsEvenIfInvalid)...) _, cidrIPNet, err := netutils.ParseCIDRSloppy(ipb.CIDR) if err != nil { // Implies validation would have failed so we already added errors for it. @@ -228,7 +256,7 @@ func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path) field.ErrorLi for i, exceptCIDRStr := range ipb.Except { exceptPath := fldPath.Child("except").Index(i) - allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(exceptPath, exceptCIDRStr)...) + allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(exceptPath, exceptCIDRStr, opts.AllowCIDRsEvenIfInvalid)...) _, exceptCIDR, err := netutils.ParseCIDRSloppy(exceptCIDRStr) if err != nil { // Implies validation would have failed so we already added errors for it. @@ -346,17 +374,28 @@ func ValidateIngressSpec(spec *networking.IngressSpec, fldPath *field.Path, opts // ValidateIngressStatusUpdate tests if required fields in the Ingress are set when updating status. func ValidateIngressStatusUpdate(ingress, oldIngress *networking.Ingress) field.ErrorList { allErrs := apivalidation.ValidateObjectMetaUpdate(&ingress.ObjectMeta, &oldIngress.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateIngressLoadBalancerStatus(&ingress.Status.LoadBalancer, field.NewPath("status", "loadBalancer"))...) + allErrs = append(allErrs, ValidateIngressLoadBalancerStatus(&ingress.Status.LoadBalancer, &oldIngress.Status.LoadBalancer, field.NewPath("status", "loadBalancer"))...) return allErrs } // ValidateIngressLoadBalancerStatus validates required fields on an IngressLoadBalancerStatus -func ValidateIngressLoadBalancerStatus(status *networking.IngressLoadBalancerStatus, fldPath *field.Path) field.ErrorList { +func ValidateIngressLoadBalancerStatus(status, oldStatus *networking.IngressLoadBalancerStatus, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} + + var existingIngressIPs []string + if oldStatus != nil { + existingIngressIPs = make([]string, 0, len(oldStatus.Ingress)) + for _, ingress := range oldStatus.Ingress { + if len(ingress.IP) > 0 { + existingIngressIPs = append(existingIngressIPs, ingress.IP) + } + } + } + for i, ingress := range status.Ingress { idxPath := fldPath.Child("ingress").Index(i) if len(ingress.IP) > 0 { - allErrs = append(allErrs, apivalidation.IsValidIPForLegacyField(idxPath.Child("ip"), ingress.IP)...) + allErrs = append(allErrs, apivalidation.IsValidIPForLegacyField(idxPath.Child("ip"), ingress.IP, existingIngressIPs)...) } if len(ingress.Hostname) > 0 { for _, msg := range validation.IsDNS1123Subdomain(ingress.Hostname) { diff --git a/pkg/apis/networking/validation/validation_test.go b/pkg/apis/networking/validation/validation_test.go index 438d7f5bea7..7217964ed42 100644 --- a/pkg/apis/networking/validation/validation_test.go +++ b/pkg/apis/networking/validation/validation_test.go @@ -442,13 +442,95 @@ func TestValidateNetworkPolicyUpdate(t *testing.T) { }, }, }, + "fix pre-existing invalid CIDR": { + old: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.2.0/24", + "192.168.3.1/24", + }, + }, + }}, + }}, + }, + }, + update: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.2.0/24", + "192.168.3.0/24", + }, + }, + }}, + }}, + }, + }, + }, + "fail to fix pre-existing invalid CIDR": { + old: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.2.0/24", + "192.168.3.1/24", + }, + }, + }}, + }}, + }, + }, + update: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.1.0/24", + "192.168.2.0/24", + "192.168.3.1/24", + }, + }, + }}, + }}, + }, + }, + }, } for testName, successCase := range successCases { t.Run(testName, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) successCase.old.ObjectMeta.ResourceVersion = "1" successCase.update.ObjectMeta.ResourceVersion = "1" - if errs := ValidateNetworkPolicyUpdate(&successCase.update, &successCase.old, NetworkPolicyValidationOptions{false}); len(errs) != 0 { + if errs := ValidateNetworkPolicyUpdate(&successCase.update, &successCase.old, NetworkPolicyValidationOptions{}); len(errs) != 0 { t.Errorf("expected success, but got %v", errs) } }) @@ -471,13 +553,55 @@ func TestValidateNetworkPolicyUpdate(t *testing.T) { }, }, }, + "add new invalid CIDR": { + old: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.2.0/24", + "192.168.3.0/24", + }, + }, + }}, + }}, + }, + }, + update: networking.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"}, + Spec: networking.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"a": "b"}, + }, + Ingress: []networking.NetworkPolicyIngressRule{{ + From: []networking.NetworkPolicyPeer{{ + IPBlock: &networking.IPBlock{ + CIDR: "192.168.0.0/16", + Except: []string{ + "192.168.1.1/24", + "192.168.2.0/24", + "192.168.3.0/24", + }, + }, + }}, + }}, + }, + }, + }, } for testName, errorCase := range errorCases { t.Run(testName, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true) errorCase.old.ObjectMeta.ResourceVersion = "1" errorCase.update.ObjectMeta.ResourceVersion = "1" - if errs := ValidateNetworkPolicyUpdate(&errorCase.update, &errorCase.old, NetworkPolicyValidationOptions{false}); len(errs) == 0 { + if errs := ValidateNetworkPolicyUpdate(&errorCase.update, &errorCase.old, NetworkPolicyValidationOptions{}); len(errs) == 0 { t.Errorf("expected failure") } }) @@ -1865,6 +1989,10 @@ func TestValidateIngressStatusUpdate(t *testing.T) { newValue: legacyIP, legacyIPs: true, }, + "legacy IPs unchanged in update": { + oldValue: legacyIP, + newValue: legacyIP, + }, } for k, tc := range successCases { t.Run(k, func(t *testing.T) { diff --git a/pkg/registry/core/service/storage/alloc.go b/pkg/registry/core/service/storage/alloc.go index a69261fb91f..8285f08804e 100644 --- a/pkg/registry/core/service/storage/alloc.go +++ b/pkg/registry/core/service/storage/alloc.go @@ -151,9 +151,7 @@ func (al *Allocators) initIPFamilyFields(after After, before Before) error { // Do some loose pre-validation of the input. This makes it easier in the // rest of allocation code to not have to consider corner cases. - // TODO(thockin): when we tighten validation (e.g. to require IPs) we will - // need a "strict" and a "loose" form of this. - if el := validation.ValidateServiceClusterIPsRelatedFields(service); len(el) != 0 { + if el := validation.ValidateServiceClusterIPsRelatedFields(service, oldService); len(el) != 0 { return errors.NewInvalid(api.Kind("Service"), service.Name, el) } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go index 9c836656ed7..6e947c74c9e 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go @@ -20,6 +20,7 @@ import ( "fmt" "net" "net/netip" + "slices" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/klog/v2" @@ -70,10 +71,17 @@ func parseIP(fldPath *field.Path, value string, strictValidation bool) (net.IP, // netip.ParseAddr both allow these, but there are no use cases for representing IPv4 // addresses as IPv4-mapped IPv6 addresses in Kubernetes.) // +// Alternatively, when validating an update to an existing field, you can pass a list of +// IP values from the old object that should be accepted if they appear in the new object +// even if they are not valid. +// // This function should only be used to validate the existing fields that were // historically validated in this way, and strictValidation should be true unless the // StrictIPCIDRValidation feature gate is disabled. Use IsValidIP for parsing new fields. -func IsValidIPForLegacyField(fldPath *field.Path, value string, strictValidation bool) field.ErrorList { +func IsValidIPForLegacyField(fldPath *field.Path, value string, strictValidation bool, validOldIPs []string) field.ErrorList { + if slices.Contains(validOldIPs, value) { + return nil + } _, allErrors := parseIP(fldPath, value, strictValidation) return allErrors.WithOrigin("format=ip-sloppy") } @@ -165,11 +173,19 @@ func parseCIDR(fldPath *field.Path, value string, strictValidation bool) (*net.I // // 3. The prefix length is allowed to have leading 0s. // +// Alternatively, when validating an update to an existing field, you can pass a list of +// CIDR values from the old object that should be accepted if they appear in the new +// object even if they are not valid. +// // This function should only be used to validate the existing fields that were // historically validated in this way, and strictValidation should be true unless the // StrictIPCIDRValidation feature gate is disabled. Use IsValidCIDR or // IsValidInterfaceAddress for parsing new fields. -func IsValidCIDRForLegacyField(fldPath *field.Path, value string, strictValidation bool) field.ErrorList { +func IsValidCIDRForLegacyField(fldPath *field.Path, value string, strictValidation bool, validOldCIDRs []string) field.ErrorList { + if slices.Contains(validOldCIDRs, value) { + return nil + } + _, allErrors := parseCIDR(fldPath, value, strictValidation) return allErrors } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip_test.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip_test.go index fed7e73d3a9..caf5cf1fe51 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip_test.go @@ -25,7 +25,7 @@ import ( ) func TestIsValidIP(t *testing.T) { - for _, tc := range []struct { + testCases := []struct { name string in string @@ -202,7 +202,16 @@ func TestIsValidIP(t *testing.T) { legacyErr: "must be a valid IP address", legacyStrictErr: "must be a valid IP address", }, - } { + } + + var badIPs []string + for _, tc := range testCases { + if tc.legacyStrictErr != "" { + badIPs = append(badIPs, tc.in) + } + } + + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { errs := IsValidIP(field.NewPath(""), tc.in) if tc.err == "" { @@ -217,7 +226,7 @@ func TestIsValidIP(t *testing.T) { } } - errs = IsValidIPForLegacyField(field.NewPath(""), tc.in, false) + errs = IsValidIPForLegacyField(field.NewPath(""), tc.in, false, nil) if tc.legacyErr == "" { if len(errs) != 0 { t.Errorf("expected %q to be valid according to IsValidIPForLegacyField but got: %v", tc.in, errs) @@ -230,7 +239,7 @@ func TestIsValidIP(t *testing.T) { } } - errs = IsValidIPForLegacyField(field.NewPath(""), tc.in, true) + errs = IsValidIPForLegacyField(field.NewPath(""), tc.in, true, nil) if tc.legacyStrictErr == "" { if len(errs) != 0 { t.Errorf("expected %q to be valid according to IsValidIPForLegacyField with strict validation, but got: %v", tc.in, errs) @@ -242,6 +251,11 @@ func TestIsValidIP(t *testing.T) { t.Errorf("expected error from IsValidIPForLegacyField with strict validation for %q to contain %q but got: %q", tc.in, tc.legacyStrictErr, errs[0].Detail) } } + + errs = IsValidIPForLegacyField(field.NewPath(""), tc.in, true, badIPs) + if len(errs) != 0 { + t.Errorf("expected %q to be accepted when using validOldIPs, but got: %v", tc.in, errs) + } }) } } @@ -300,7 +314,7 @@ func TestGetWarningsForIP(t *testing.T) { } func TestIsValidCIDR(t *testing.T) { - for _, tc := range []struct { + testCases := []struct { name string in string @@ -455,7 +469,16 @@ func TestIsValidCIDR(t *testing.T) { legacyErr: "must be a valid CIDR value", legacyStrictErr: "must be a valid CIDR value", }, - } { + } + + var badCIDRs []string + for _, tc := range testCases { + if tc.legacyStrictErr != "" { + badCIDRs = append(badCIDRs, tc.in) + } + } + + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { errs := IsValidCIDR(field.NewPath(""), tc.in) if tc.err == "" { @@ -470,7 +493,7 @@ func TestIsValidCIDR(t *testing.T) { } } - errs = IsValidCIDRForLegacyField(field.NewPath(""), tc.in, false) + errs = IsValidCIDRForLegacyField(field.NewPath(""), tc.in, false, nil) if tc.legacyErr == "" { if len(errs) != 0 { t.Errorf("expected %q to be valid according to IsValidCIDRForLegacyField but got: %v", tc.in, errs) @@ -483,7 +506,7 @@ func TestIsValidCIDR(t *testing.T) { } } - errs = IsValidCIDRForLegacyField(field.NewPath(""), tc.in, true) + errs = IsValidCIDRForLegacyField(field.NewPath(""), tc.in, true, nil) if tc.legacyStrictErr == "" { if len(errs) != 0 { t.Errorf("expected %q to be valid according to IsValidCIDRForLegacyField with strict validation but got: %v", tc.in, errs) @@ -495,6 +518,11 @@ func TestIsValidCIDR(t *testing.T) { t.Errorf("expected error for %q from IsValidCIDRForLegacyField with strict validation to contain %q but got: %q", tc.in, tc.legacyStrictErr, errs[0].Detail) } } + + errs = IsValidCIDRForLegacyField(field.NewPath(""), tc.in, true, badCIDRs) + if len(errs) != 0 { + t.Errorf("expected %q to be accepted when using validOldCIDRs, but got: %v", tc.in, errs) + } }) } }