From 5e067b6781cdc19972f53e0e7ccee9daca0cd863 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Wed, 27 Dec 2023 09:44:45 -0500 Subject: [PATCH] Minor IP/CIDR validation cleanups/fixups Remove unnecessary duplicate checks for pod.spec.podIPs / pod.spec.hostIPs / node.spec.podCIDRs. (A list that is known to contain exactly 2 values, where one is IPv4 and the other is IPv6, cannot possibly contain duplicates.) Fix a bad CIDR in the NetworkPolicy validation tests. Fix some comment typos. --- pkg/apis/core/validation/validation.go | 41 +++---------------- pkg/apis/networking/validation/validation.go | 2 +- .../networking/validation/validation_test.go | 2 +- 3 files changed, 7 insertions(+), 38 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index ce84c41b96d..20f30f0dd7c 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -4118,9 +4118,7 @@ func validatePodIPs(pod *core.Pod) field.ErrorList { allErrs = append(allErrs, validation.IsValidIP(podIPsField.Index(i), podIP.IP)...) } - // if we have more than one Pod.PodIP then - // - validate for dual stack - // - validate for duplication + // if we have more than one Pod.PodIP then we must have a dual-stack pair if len(pod.Status.PodIPs) > 1 { podIPs := make([]string, 0, len(pod.Status.PodIPs)) for _, podIP := range pod.Status.PodIPs { @@ -4136,15 +4134,6 @@ func validatePodIPs(pod *core.Pod) field.ErrorList { if !dualStack || len(podIPs) > 2 { allErrs = append(allErrs, field.Invalid(podIPsField, pod.Status.PodIPs, "may specify no more than one IP for each IP family")) } - - // There should be no duplicates in list of Pod.PodIPs - seen := sets.Set[string]{} // := make(map[string]int) - for i, podIP := range pod.Status.PodIPs { - if seen.Has(podIP.IP) { - allErrs = append(allErrs, field.Duplicate(podIPsField.Index(i), podIP)) - } - seen.Insert(podIP.IP) - } } return allErrs @@ -4165,25 +4154,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 HostPs must be valid IPs + // all HostIPs must be valid IPs for i, hostIP := range pod.Status.HostIPs { allErrs = append(allErrs, validation.IsValidIP(hostIPsField.Index(i), hostIP.IP)...) } - // if we have more than one Pod.HostIP then - // - validate for dual stack - // - validate for duplication + // if we have more than one Pod.HostIP then we must have a dual-stack pair if len(pod.Status.HostIPs) > 1 { - seen := sets.Set[string]{} hostIPs := make([]string, 0, len(pod.Status.HostIPs)) - - // There should be no duplicates in list of Pod.HostIPs - for i, hostIP := range pod.Status.HostIPs { + for _, hostIP := range pod.Status.HostIPs { hostIPs = append(hostIPs, hostIP.IP) - if seen.Has(hostIP.IP) { - allErrs = append(allErrs, field.Duplicate(hostIPsField.Index(i), hostIP)) - } - seen.Insert(hostIP.IP) } dualStack, err := netutils.IsDualStackIPStrings(hostIPs) @@ -6417,9 +6397,7 @@ func ValidateNode(node *core.Node) field.ErrorList { allErrs = append(allErrs, validation.IsValidCIDR(podCIDRsField.Index(idx), value)...) } - // if more than PodCIDR then - // - validate for dual stack - // - validate for duplication + // if more than PodCIDR then it must be a dual-stack pair if len(node.Spec.PodCIDRs) > 1 { dualStack, err := netutils.IsDualStackCIDRStrings(node.Spec.PodCIDRs) if err != nil { @@ -6428,15 +6406,6 @@ func ValidateNode(node *core.Node) field.ErrorList { if !dualStack || len(node.Spec.PodCIDRs) > 2 { allErrs = append(allErrs, field.Invalid(podCIDRsField, node.Spec.PodCIDRs, "may specify no more than one CIDR for each IP family")) } - - // PodCIDRs must not contain duplicates - seen := sets.Set[string]{} - for i, value := range node.Spec.PodCIDRs { - if seen.Has(value) { - allErrs = append(allErrs, field.Duplicate(podCIDRsField.Index(i), value)) - } - seen.Insert(value) - } } } diff --git a/pkg/apis/networking/validation/validation.go b/pkg/apis/networking/validation/validation.go index f41ba5f3826..1371ec7d8c2 100644 --- a/pkg/apis/networking/validation/validation.go +++ b/pkg/apis/networking/validation/validation.go @@ -351,7 +351,7 @@ func ValidateIngressStatusUpdate(ingress, oldIngress *networking.Ingress) field. return allErrs } -// ValidateLIngressoadBalancerStatus validates required fields on an IngressLoadBalancerStatus +// ValidateIngressLoadBalancerStatus validates required fields on an IngressLoadBalancerStatus func ValidateIngressLoadBalancerStatus(status *networking.IngressLoadBalancerStatus, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for i, ingress := range status.Ingress { diff --git a/pkg/apis/networking/validation/validation_test.go b/pkg/apis/networking/validation/validation_test.go index bb1f579a211..7b2ad6f0515 100644 --- a/pkg/apis/networking/validation/validation_test.go +++ b/pkg/apis/networking/validation/validation_test.go @@ -311,7 +311,7 @@ func TestValidateNetworkPolicy(t *testing.T) { "except IP is outside of CIDR range": makeNetworkPolicyCustom(setIngressFromEmptyFirstElement, func(networkPolicy *networking.NetworkPolicy) { networkPolicy.Spec.Ingress[0].From[0].IPBlock = &networking.IPBlock{ CIDR: "192.168.8.0/24", - Except: []string{"192.168.9.1/24"}, + Except: []string{"192.168.9.1/32"}, } }), "except IP is not strictly within CIDR range": makeNetworkPolicyCustom(setIngressFromEmptyFirstElement, func(networkPolicy *networking.NetworkPolicy) {