scottk/kubernetes
1
0
Fork 0
mirror of https://github.com/optim-enterprises-bv/kubernetes.git synced 2025-10-30 01:42:48 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix IP/CIDR validation to allow updates to existing invalid objects

Browse Source 
Ignore pre-existing bad IP/CIDR values in:
  - pod.spec.podIP(s)
  - pod.spec.hostIP(s)
  - service.spec.externalIPs
  - service.spec.clusterIP(s)
  - service.spec.loadBalancerSourceRanges (and corresponding annotation)
  - service.status.loadBalancer.ingress[].ip
  - endpoints.subsets
  - endpointslice.endpoints
  - networkpolicy.spec.{ingress[].from[],egress[].to[]}.ipBlock
  - ingress.status.loadBalancer.ingress[].ip

In the Endpoints and EndpointSlice case, if *any* endpoint IP is
changed, then the entire object must be valid; invalid IPs are only
allowed to remain in place for updates that don't change any IPs.
(e.g., changing the labels or annotations).

In most of the other cases, when the invalid IP is part of an array,
it can be moved around within the array without triggering
revalidation.
This commit is contained in:
Dan Winship
2023-12-28 11:30:45 -05:00
parent 692785d25b
commit ad22c0d495
9 changed files with 693 additions and 99 deletions
Download Patch File Download Diff File Expand all files Collapse all files

136
pkg/apis/core/validation/validation.go
View File

@@ -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)
}

341
pkg/apis/core/validation/validation_test.go
View File

@@ -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())
}

22
pkg/apis/discovery/validation/validation.go
View File

@@ -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 {

38
pkg/apis/discovery/validation/validation_test.go
View File

@@ -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)

55
pkg/apis/networking/validation/validation.go
View File

@@ -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) {

132
pkg/apis/networking/validation/validation_test.go
View File

@@ -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) {

4
pkg/registry/core/service/storage/alloc.go
View File

@@ -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)
}

20
staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go
View File

@@ -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
}

44
staging/src/k8s.io/apimachinery/pkg/util/validation/ip_test.go
View File

@@ -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)
}
})
}
}