diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index 3b723d8d86d..3556f9d7603 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -24,6 +24,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" nodeapi "k8s.io/kubernetes/pkg/api/node" pvcutil "k8s.io/kubernetes/pkg/api/persistentvolumeclaim" @@ -351,6 +352,16 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta } } + // Deprecated IP address formats + if podSpec.DNSConfig != nil { + for i, ns := range podSpec.DNSConfig.Nameservers { + warnings = append(warnings, validation.GetWarningsForIP(fieldPath.Child("spec", "dnsConfig", "nameservers").Index(i), ns)...) + } + } + for i, hostAlias := range podSpec.HostAliases { + warnings = append(warnings, validation.GetWarningsForIP(fieldPath.Child("spec", "hostAliases").Index(i).Child("ip"), hostAlias.IP)...) + } + return warnings } diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index bea1b1df62a..0524c5188e6 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -1767,6 +1767,21 @@ func TestWarnings(t *testing.T) { `spec.affinity.nodeAffinity.preferredDuringSchedulingIgnoredDuringExecution[0].preference.matchExpressions[0].values[0]: -1 is invalid, a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')`, }, }, + { + name: "dubious IP address formats", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + DNSConfig: &api.PodDNSConfig{ + Nameservers: []string{"1.2.3.4", "05.06.07.08"}, + }, + HostAliases: []api.HostAlias{ + {IP: "::ffff:1.2.3.4"}, + }, + }}, + expected: []string{ + `spec.dnsConfig.nameservers[1]: non-standard IP address "05.06.07.08" will be considered invalid in a future Kubernetes release: use "5.6.7.8"`, + `spec.hostAliases[0].ip: non-standard IP address "::ffff:1.2.3.4" will be considered invalid in a future Kubernetes release: use "1.2.3.4"`, + }, + }, } for _, tc := range testcases { diff --git a/pkg/api/service/warnings.go b/pkg/api/service/warnings.go index 29b1097dffc..75fb43b7eaf 100644 --- a/pkg/api/service/warnings.go +++ b/pkg/api/service/warnings.go @@ -18,8 +18,8 @@ package service import ( "fmt" - "net/netip" + utilvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/core/helper" @@ -37,20 +37,20 @@ func GetWarningsForService(service, oldService *api.Service) []string { if helper.IsServiceIPSet(service) { for i, clusterIP := range service.Spec.ClusterIPs { - warnings = append(warnings, getWarningsForIP(field.NewPath("spec").Child("clusterIPs").Index(i), clusterIP)...) + warnings = append(warnings, utilvalidation.GetWarningsForIP(field.NewPath("spec").Child("clusterIPs").Index(i), clusterIP)...) } } for i, externalIP := range service.Spec.ExternalIPs { - warnings = append(warnings, getWarningsForIP(field.NewPath("spec").Child("externalIPs").Index(i), externalIP)...) + warnings = append(warnings, utilvalidation.GetWarningsForIP(field.NewPath("spec").Child("externalIPs").Index(i), externalIP)...) } if len(service.Spec.LoadBalancerIP) > 0 { - warnings = append(warnings, getWarningsForIP(field.NewPath("spec").Child("loadBalancerIP"), service.Spec.LoadBalancerIP)...) + warnings = append(warnings, utilvalidation.GetWarningsForIP(field.NewPath("spec").Child("loadBalancerIP"), service.Spec.LoadBalancerIP)...) } for i, cidr := range service.Spec.LoadBalancerSourceRanges { - warnings = append(warnings, getWarningsForCIDR(field.NewPath("spec").Child("loadBalancerSourceRanges").Index(i), cidr)...) + warnings = append(warnings, utilvalidation.GetWarningsForCIDR(field.NewPath("spec").Child("loadBalancerSourceRanges").Index(i), cidr)...) } if service.Spec.Type == api.ServiceTypeExternalName && len(service.Spec.ExternalIPs) > 0 { @@ -62,45 +62,3 @@ func GetWarningsForService(service, oldService *api.Service) []string { return warnings } - -func getWarningsForIP(fieldPath *field.Path, address string) []string { - // IPv4 addresses with leading zeros CVE-2021-29923 are not valid in golang since 1.17 - // This will also warn about possible future changes on the golang std library - // xref: https://issues.k8s.io/108074 - ip, err := netip.ParseAddr(address) - if err != nil { - return []string{fmt.Sprintf("%s: IP address was accepted, but will be invalid in a future Kubernetes release: %v", fieldPath, err)} - } - // A Recommendation for IPv6 Address Text Representation - // - // "All of the above examples represent the same IPv6 address. This - // flexibility has caused many problems for operators, systems - // engineers, and customers. - // ..." - // https://datatracker.ietf.org/doc/rfc5952/ - if ip.Is6() && ip.String() != address { - return []string{fmt.Sprintf("%s: IPv6 address %q is not in RFC 5952 canonical format (%q), which may cause controller apply-loops", fieldPath, address, ip.String())} - } - return []string{} -} - -func getWarningsForCIDR(fieldPath *field.Path, cidr string) []string { - // IPv4 addresses with leading zeros CVE-2021-29923 are not valid in golang since 1.17 - // This will also warn about possible future changes on the golang std library - // xref: https://issues.k8s.io/108074 - prefix, err := netip.ParsePrefix(cidr) - if err != nil { - return []string{fmt.Sprintf("%s: IP prefix was accepted, but will be invalid in a future Kubernetes release: %v", fieldPath, err)} - } - // A Recommendation for IPv6 Address Text Representation - // - // "All of the above examples represent the same IPv6 address. This - // flexibility has caused many problems for operators, systems - // engineers, and customers. - // ..." - // https://datatracker.ietf.org/doc/rfc5952/ - if prefix.Addr().Is6() && prefix.String() != cidr { - return []string{fmt.Sprintf("%s: IPv6 prefix %q is not in RFC 5952 canonical format (%q), which may cause controller apply-loops", fieldPath, cidr, prefix.String())} - } - return []string{} -} diff --git a/pkg/api/service/warnings_test.go b/pkg/api/service/warnings_test.go index a45af0bf62b..900d123fa19 100644 --- a/pkg/api/service/warnings_test.go +++ b/pkg/api/service/warnings_test.go @@ -22,7 +22,6 @@ import ( "github.com/google/go-cmp/cmp" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/validation/field" api "k8s.io/kubernetes/pkg/apis/core" ) @@ -108,58 +107,58 @@ func TestGetWarningsForServiceClusterIPs(t *testing.T) { name: "IPv4 with leading zeros", service: service([]string{"192.012.2.2"}), want: []string{ - `spec.clusterIPs[0]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`, + `spec.clusterIPs[0]: non-standard IP address "192.012.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`, }, }, { name: "Dual Stack IPv4-IPv6 and IPv4 with leading zeros", service: service([]string{"192.012.2.2", "2001:db8::2"}), want: []string{ - `spec.clusterIPs[0]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`, + `spec.clusterIPs[0]: non-standard IP address "192.012.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`, }, }, { name: "Dual Stack IPv6-IPv4 and IPv4 with leading zeros", service: service([]string{"2001:db8::2", "192.012.2.2"}), want: []string{ - `spec.clusterIPs[1]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`, + `spec.clusterIPs[1]: non-standard IP address "192.012.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`, }, }, { name: "IPv6 non canonical format", service: service([]string{"2001:db8:0:0::2"}), want: []string{ - `spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`, + `spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" should be in RFC 5952 canonical format ("2001:db8::2")`, }, }, { name: "Dual Stack IPv4-IPv6 and IPv6 non-canonical format", service: service([]string{"192.12.2.2", "2001:db8:0:0::2"}), want: []string{ - `spec.clusterIPs[1]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`, + `spec.clusterIPs[1]: IPv6 address "2001:db8:0:0::2" should be in RFC 5952 canonical format ("2001:db8::2")`, }, }, { name: "Dual Stack IPv6-IPv4 and IPv6 non-canonical formats", service: service([]string{"2001:db8:0:0::2", "192.12.2.2"}), want: []string{ - `spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`, + `spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" should be in RFC 5952 canonical format ("2001:db8::2")`, }, }, { name: "Dual Stack IPv4-IPv6 and IPv4 with leading zeros and IPv6 non-canonical format", service: service([]string{"192.012.2.2", "2001:db8:0:0::2"}), want: []string{ - `spec.clusterIPs[0]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`, - `spec.clusterIPs[1]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`, + `spec.clusterIPs[0]: non-standard IP address "192.012.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`, + `spec.clusterIPs[1]: IPv6 address "2001:db8:0:0::2" should be in RFC 5952 canonical format ("2001:db8::2")`, }, }, { name: "Dual Stack IPv6-IPv4 and IPv4 with leading zeros and IPv6 non-canonical format", service: service([]string{"2001:db8:0:0::2", "192.012.2.2"}), want: []string{ - `spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`, - `spec.clusterIPs[1]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`, + `spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" should be in RFC 5952 canonical format ("2001:db8::2")`, + `spec.clusterIPs[1]: non-standard IP address "192.012.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`, }, }, { @@ -179,13 +178,13 @@ func TestGetWarningsForServiceClusterIPs(t *testing.T) { }, }, want: []string{ - `spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`, - `spec.clusterIPs[1]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`, - `spec.externalIPs[0]: IPv6 address "2001:db8:1:0::2" is not in RFC 5952 canonical format ("2001:db8:1::2"), which may cause controller apply-loops`, - `spec.externalIPs[1]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("10.012.2.2"): IPv4 field has octet with leading zero`, - `spec.loadBalancerIP: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("10.001.1.1"): IPv4 field has octet with leading zero`, - `spec.loadBalancerSourceRanges[0]: IPv6 prefix "2001:db8:1:0::/64" is not in RFC 5952 canonical format ("2001:db8:1::/64"), which may cause controller apply-loops`, - `spec.loadBalancerSourceRanges[1]: IP prefix was accepted, but will be invalid in a future Kubernetes release: netip.ParsePrefix("10.012.2.0/24"): ParseAddr("10.012.2.0"): IPv4 field has octet with leading zero`, + `spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" should be in RFC 5952 canonical format ("2001:db8::2")`, + `spec.clusterIPs[1]: non-standard IP address "192.012.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`, + `spec.externalIPs[0]: IPv6 address "2001:db8:1:0::2" should be in RFC 5952 canonical format ("2001:db8:1::2")`, + `spec.externalIPs[1]: non-standard IP address "10.012.2.2" will be considered invalid in a future Kubernetes release: use "10.12.2.2"`, + `spec.loadBalancerIP: non-standard IP address "10.001.1.1" will be considered invalid in a future Kubernetes release: use "10.1.1.1"`, + `spec.loadBalancerSourceRanges[0]: IPv6 CIDR value "2001:db8:1:0::/64" should be in RFC 5952 canonical format ("2001:db8:1::/64")`, + `spec.loadBalancerSourceRanges[1]: non-standard CIDR value "10.012.2.0/24" will be considered invalid in a future Kubernetes release: use "10.12.2.0/24"`, }, }, } @@ -197,93 +196,3 @@ func TestGetWarningsForServiceClusterIPs(t *testing.T) { }) } } - -func Test_getWarningsForIP(t *testing.T) { - tests := []struct { - name string - fieldPath *field.Path - address string - want []string - }{ - { - name: "IPv4 No failures", - address: "192.12.2.2", - fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0), - want: []string{}, - }, - { - name: "IPv6 No failures", - address: "2001:db8::2", - fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0), - want: []string{}, - }, - { - name: "IPv4 with leading zeros", - address: "192.012.2.2", - fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0), - want: []string{ - `spec.clusterIPs[0]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`, - }, - }, - { - name: "IPv6 non-canonical format", - address: "2001:db8:0:0::2", - fieldPath: field.NewPath("spec").Child("loadBalancerIP"), - want: []string{ - `spec.loadBalancerIP: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := getWarningsForIP(tt.fieldPath, tt.address); !reflect.DeepEqual(got, tt.want) { - t.Errorf("getWarningsForIP() = %v, want %v", got, tt.want) - } - }) - } -} - -func Test_getWarningsForCIDR(t *testing.T) { - tests := []struct { - name string - fieldPath *field.Path - cidr string - want []string - }{ - { - name: "IPv4 No failures", - cidr: "192.12.2.0/24", - fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), - want: []string{}, - }, - { - name: "IPv6 No failures", - cidr: "2001:db8::/64", - fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), - want: []string{}, - }, - { - name: "IPv4 with leading zeros", - cidr: "192.012.2.0/24", - fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), - want: []string{ - `spec.loadBalancerSourceRanges[0]: IP prefix was accepted, but will be invalid in a future Kubernetes release: netip.ParsePrefix("192.012.2.0/24"): ParseAddr("192.012.2.0"): IPv4 field has octet with leading zero`, - }, - }, - { - name: "IPv6 non-canonical format", - cidr: "2001:db8:0:0::/64", - fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), - want: []string{ - `spec.loadBalancerSourceRanges[0]: IPv6 prefix "2001:db8:0:0::/64" is not in RFC 5952 canonical format ("2001:db8::/64"), which may cause controller apply-loops`, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := getWarningsForCIDR(tt.fieldPath, tt.cidr); !reflect.DeepEqual(got, tt.want) { - t.Errorf("getWarningsForCIDR() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 1687c6b51be..2d62ea13b55 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) @@ -6426,9 +6406,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 { @@ -6437,15 +6415,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) { diff --git a/pkg/controller/endpoint/endpoints_controller.go b/pkg/controller/endpoint/endpoints_controller.go index bfc052504b4..e7739c60799 100644 --- a/pkg/controller/endpoint/endpoints_controller.go +++ b/pkg/controller/endpoint/endpoints_controller.go @@ -67,18 +67,18 @@ const ( // maxCapacity truncated = "truncated" - // labelManagedBy is a label for recognizing Endpoints managed by this controller. - labelManagedBy = "endpoints.kubernetes.io/managed-by" + // LabelManagedBy is a label for recognizing Endpoints managed by this controller. + LabelManagedBy = "endpoints.kubernetes.io/managed-by" - // controllerName is the name of this controller - controllerName = "endpoint-controller" + // ControllerName is the name of this controller + ControllerName = "endpoint-controller" ) // NewEndpointController returns a new *Controller. func NewEndpointController(ctx context.Context, podInformer coreinformers.PodInformer, serviceInformer coreinformers.ServiceInformer, endpointsInformer coreinformers.EndpointsInformer, client clientset.Interface, endpointUpdatesBatchPeriod time.Duration) *Controller { broadcaster := record.NewBroadcaster(record.WithContext(ctx)) - recorder := broadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: controllerName}) + recorder := broadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: ControllerName}) e := &Controller{ client: client, @@ -503,7 +503,7 @@ func (e *Controller) syncService(ctx context.Context, key string) error { } else { newEndpoints.Labels = utillabels.CloneAndRemoveLabel(newEndpoints.Labels, v1.IsHeadlessService) } - newEndpoints.Labels[labelManagedBy] = controllerName + newEndpoints.Labels[LabelManagedBy] = ControllerName logger.V(4).Info("Update endpoints", "service", klog.KObj(service), "readyEndpoints", totalReadyEps, "notreadyEndpoints", totalNotReadyEps) var updatedEndpoints *v1.Endpoints @@ -720,16 +720,16 @@ func endpointSubsetsEqualIgnoreResourceVersion(subsets1, subsets2 []v1.EndpointS // labelsCorrectForEndpoints tests that epLabels is correctly derived from svcLabels // (ignoring the v1.IsHeadlessService label). func labelsCorrectForEndpoints(epLabels, svcLabels map[string]string) bool { - if epLabels[labelManagedBy] != controllerName { + if epLabels[LabelManagedBy] != ControllerName { return false } - // Every label in epLabels except v1.IsHeadlessService and labelManagedBy should + // Every label in epLabels except v1.IsHeadlessService and LabelManagedBy should // correspond to a label in svcLabels, and svcLabels should not have any other // labels that aren't in epLabels. skipped := 0 for k, v := range epLabels { - if k == v1.IsHeadlessService || k == labelManagedBy { + if k == v1.IsHeadlessService || k == LabelManagedBy { skipped++ } else if sv, exists := svcLabels[k]; !exists || sv != v { return false diff --git a/pkg/controller/endpoint/endpoints_controller_test.go b/pkg/controller/endpoint/endpoints_controller_test.go index 2ccc848864b..1b68978019f 100644 --- a/pkg/controller/endpoint/endpoints_controller_test.go +++ b/pkg/controller/endpoint/endpoints_controller_test.go @@ -318,7 +318,7 @@ func TestSyncEndpointsExistingNilSubsets(t *testing.T) { Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, }, }, Subsets: nil, @@ -350,7 +350,7 @@ func TestSyncEndpointsExistingEmptySubsets(t *testing.T) { Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, }, }, Subsets: []v1.EndpointSubset{}, @@ -383,7 +383,7 @@ func TestSyncEndpointsWithPodResourceVersionUpdateOnly(t *testing.T) { Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, }, }, Subsets: []v1.EndpointSubset{{ @@ -510,7 +510,7 @@ func TestSyncEndpointsProtocolTCP(t *testing.T) { Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -534,7 +534,7 @@ func TestSyncEndpointsHeadlessServiceLabel(t *testing.T) { Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -663,7 +663,7 @@ func TestSyncEndpointsProtocolUDP(t *testing.T) { Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -713,7 +713,7 @@ func TestSyncEndpointsProtocolSCTP(t *testing.T) { Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -759,7 +759,7 @@ func TestSyncEndpointsItemsEmptySelectorSelectsAll(t *testing.T) { Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -806,7 +806,7 @@ func TestSyncEndpointsItemsEmptySelectorSelectsAllNotReady(t *testing.T) { Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -853,7 +853,7 @@ func TestSyncEndpointsItemsEmptySelectorSelectsAllMixed(t *testing.T) { Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -878,7 +878,7 @@ func TestSyncEndpointsItemsPreexisting(t *testing.T) { Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, }, }, Subsets: []v1.EndpointSubset{{ @@ -906,7 +906,7 @@ func TestSyncEndpointsItemsPreexisting(t *testing.T) { Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -930,7 +930,7 @@ func TestSyncEndpointsItemsPreexistingIdentical(t *testing.T) { Name: "foo", Namespace: ns, Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, }, }, Subsets: []v1.EndpointSubset{{ @@ -995,7 +995,7 @@ func TestSyncEndpointsItems(t *testing.T) { ResourceVersion: "", Name: "foo", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -1046,7 +1046,7 @@ func TestSyncEndpointsItemsWithLabels(t *testing.T) { }} serviceLabels[v1.IsHeadlessService] = "" - serviceLabels[labelManagedBy] = controllerName + serviceLabels[LabelManagedBy] = ControllerName data := runtime.EncodeOrDie(clientscheme.Codecs.LegacyCodec(v1.SchemeGroupVersion), &v1.Endpoints{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", @@ -1099,7 +1099,7 @@ func TestSyncEndpointsItemsPreexistingLabelsChange(t *testing.T) { } serviceLabels[v1.IsHeadlessService] = "" - serviceLabels[labelManagedBy] = controllerName + serviceLabels[LabelManagedBy] = ControllerName data := runtime.EncodeOrDie(clientscheme.Codecs.LegacyCodec(v1.SchemeGroupVersion), &v1.Endpoints{ ObjectMeta: metav1.ObjectMeta{ Name: "foo", @@ -1209,7 +1209,7 @@ func TestSyncEndpointsHeadlessService(t *testing.T) { Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, "a": "b", v1.IsHeadlessService: "", }, @@ -1239,7 +1239,7 @@ func TestSyncEndpointsItemsExcludeNotReadyPodsWithRestartPolicyNeverAndPhaseFail Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, "foo": "bar", }, }, @@ -1264,7 +1264,7 @@ func TestSyncEndpointsItemsExcludeNotReadyPodsWithRestartPolicyNeverAndPhaseFail Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -1310,7 +1310,7 @@ func TestSyncEndpointsItemsExcludeNotReadyPodsWithRestartPolicyNeverAndPhaseSucc Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -1357,7 +1357,7 @@ func TestSyncEndpointsItemsExcludeNotReadyPodsWithRestartPolicyOnFailureAndPhase Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -1392,7 +1392,7 @@ func TestSyncEndpointsHeadlessWithoutPort(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -1612,7 +1612,7 @@ func TestLastTriggerChangeTimeAnnotation(t *testing.T) { v1.EndpointsLastChangeTriggerTime: triggerTimeString, }, Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -1669,7 +1669,7 @@ func TestLastTriggerChangeTimeAnnotation_AnnotationOverridden(t *testing.T) { v1.EndpointsLastChangeTriggerTime: triggerTimeString, }, Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, }, @@ -1724,7 +1724,7 @@ func TestLastTriggerChangeTimeAnnotation_AnnotationCleared(t *testing.T) { Namespace: ns, ResourceVersion: "1", Labels: map[string]string{ - labelManagedBy: controllerName, + LabelManagedBy: ControllerName, v1.IsHeadlessService: "", }, // Annotation not set anymore. }, diff --git a/pkg/controller/endpointslice/endpointslice_controller.go b/pkg/controller/endpointslice/endpointslice_controller.go index b0b67521b0e..1617367ebdc 100644 --- a/pkg/controller/endpointslice/endpointslice_controller.go +++ b/pkg/controller/endpointslice/endpointslice_controller.go @@ -72,9 +72,9 @@ const ( // maxSyncBackOff is the max backoff period for syncService calls. maxSyncBackOff = 1000 * time.Second - // controllerName is a unique value used with LabelManagedBy to indicated + // ControllerName is a unique value used with LabelManagedBy to indicated // the component managing an EndpointSlice. - controllerName = "endpointslice-controller.k8s.io" + ControllerName = "endpointslice-controller.k8s.io" // topologyQueueItemKey is the key for all items in the topologyQueue. topologyQueueItemKey = "topologyQueueItemKey" @@ -185,7 +185,7 @@ func NewController(ctx context.Context, podInformer coreinformers.PodInformer, c.endpointSliceTracker, c.topologyCache, c.eventRecorder, - controllerName, + ControllerName, endpointslicerec.WithTrafficDistributionEnabled(utilfeature.DefaultFeatureGate.Enabled(features.ServiceTrafficDistribution)), ) diff --git a/pkg/controller/endpointslice/endpointslice_controller_test.go b/pkg/controller/endpointslice/endpointslice_controller_test.go index 03476f112d0..96ca3a38c17 100644 --- a/pkg/controller/endpointslice/endpointslice_controller_test.go +++ b/pkg/controller/endpointslice/endpointslice_controller_test.go @@ -397,7 +397,7 @@ func TestSyncServiceEndpointSlicePendingDeletion(t *testing.T) { OwnerReferences: []metav1.OwnerReference{*ownerRef}, Labels: map[string]string{ discovery.LabelServiceName: serviceName, - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, DeletionTimestamp: &deletedTs, }, @@ -442,7 +442,7 @@ func TestSyncServiceEndpointSliceLabelSelection(t *testing.T) { OwnerReferences: []metav1.OwnerReference{*ownerRef}, Labels: map[string]string{ discovery.LabelServiceName: serviceName, - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, }, AddressType: discovery.AddressTypeIPv4, @@ -453,7 +453,7 @@ func TestSyncServiceEndpointSliceLabelSelection(t *testing.T) { OwnerReferences: []metav1.OwnerReference{*ownerRef}, Labels: map[string]string{ discovery.LabelServiceName: serviceName, - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, }, AddressType: discovery.AddressTypeIPv4, @@ -472,7 +472,7 @@ func TestSyncServiceEndpointSliceLabelSelection(t *testing.T) { Namespace: ns, Labels: map[string]string{ discovery.LabelServiceName: "something-else", - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, }, AddressType: discovery.AddressTypeIPv4, @@ -529,7 +529,7 @@ func TestOnEndpointSliceUpdate(t *testing.T) { Namespace: ns, Labels: map[string]string{ discovery.LabelServiceName: serviceName, - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, }, AddressType: discovery.AddressTypeIPv4, @@ -1750,7 +1750,7 @@ func TestSyncServiceStaleInformer(t *testing.T) { Generation: testcase.informerGenerationNumber, Labels: map[string]string{ discovery.LabelServiceName: serviceName, - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, }, AddressType: discovery.AddressTypeIPv4, @@ -1977,7 +1977,7 @@ func TestUpdateNode(t *testing.T) { Namespace: "ns", Labels: map[string]string{ discovery.LabelServiceName: "svc", - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, }, Endpoints: []discovery.Endpoint{ diff --git a/pkg/controller/endpointslicemirroring/endpointslicemirroring_controller.go b/pkg/controller/endpointslicemirroring/endpointslicemirroring_controller.go index a70def0f86b..7262dc1015e 100644 --- a/pkg/controller/endpointslicemirroring/endpointslicemirroring_controller.go +++ b/pkg/controller/endpointslicemirroring/endpointslicemirroring_controller.go @@ -62,9 +62,9 @@ const ( // maxSyncBackOff is the max backoff period for syncEndpoints calls. maxSyncBackOff = 100 * time.Second - // controllerName is a unique value used with LabelManagedBy to indicated + // ControllerName is a unique value used with LabelManagedBy to indicated // the component managing an EndpointSlice. - controllerName = "endpointslicemirroring-controller.k8s.io" + ControllerName = "endpointslicemirroring-controller.k8s.io" ) // NewController creates and initializes a new Controller @@ -537,7 +537,7 @@ func (c *Controller) deleteMirroredSlices(namespace, name string) error { func endpointSlicesMirroredForService(endpointSliceLister discoverylisters.EndpointSliceLister, namespace, name string) ([]*discovery.EndpointSlice, error) { esLabelSelector := labels.Set(map[string]string{ discovery.LabelServiceName: name, - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }).AsSelectorPreValidated() return endpointSliceLister.EndpointSlices(namespace).List(esLabelSelector) } diff --git a/pkg/controller/endpointslicemirroring/endpointslicemirroring_controller_test.go b/pkg/controller/endpointslicemirroring/endpointslicemirroring_controller_test.go index 75dfa155752..4846bcb76ce 100644 --- a/pkg/controller/endpointslicemirroring/endpointslicemirroring_controller_test.go +++ b/pkg/controller/endpointslicemirroring/endpointslicemirroring_controller_test.go @@ -172,7 +172,7 @@ func TestSyncEndpoints(t *testing.T) { Name: endpointsName + "-1", Labels: map[string]string{ discovery.LabelServiceName: endpointsName, - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, }, }}, @@ -358,7 +358,7 @@ func TestEndpointSlicesMirroredForService(t *testing.T) { Namespace: "ns1", Labels: map[string]string{ discovery.LabelServiceName: "svc1", - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, }, }, @@ -373,7 +373,7 @@ func TestEndpointSlicesMirroredForService(t *testing.T) { Namespace: "ns2", Labels: map[string]string{ discovery.LabelServiceName: "svc1", - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, }, }, @@ -388,7 +388,7 @@ func TestEndpointSlicesMirroredForService(t *testing.T) { Namespace: "ns1", Labels: map[string]string{ discovery.LabelServiceName: "svc2", - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, }, }, @@ -403,7 +403,7 @@ func TestEndpointSlicesMirroredForService(t *testing.T) { Namespace: "ns1", Labels: map[string]string{ discovery.LabelServiceName: "svc1", - discovery.LabelManagedBy: controllerName + "foo", + discovery.LabelManagedBy: ControllerName + "foo", }, }, }, @@ -431,7 +431,7 @@ func TestEndpointSlicesMirroredForService(t *testing.T) { Name: "example-1", Namespace: "ns1", Labels: map[string]string{ - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, }, }, diff --git a/pkg/controller/endpointslicemirroring/reconciler_test.go b/pkg/controller/endpointslicemirroring/reconciler_test.go index 5c9837d41f0..e205ef3ee1b 100644 --- a/pkg/controller/endpointslicemirroring/reconciler_test.go +++ b/pkg/controller/endpointslicemirroring/reconciler_test.go @@ -1078,7 +1078,7 @@ func TestReconcile(t *testing.T) { for _, epSlice := range tc.existingEndpointSlices { epSlice.Labels = map[string]string{ discovery.LabelServiceName: endpoints.Name, - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, } _, err := client.DiscoveryV1().EndpointSlices(namespace).Create(context.TODO(), epSlice, metav1.CreateOptions{}) if err != nil { @@ -1305,7 +1305,7 @@ func expectMatchingAddresses(t *testing.T, epSubset corev1.EndpointSubset, esEnd func fetchEndpointSlices(t *testing.T, client *fake.Clientset, namespace string) []discovery.EndpointSlice { t.Helper() fetchedSlices, err := client.DiscoveryV1().EndpointSlices(namespace).List(context.TODO(), metav1.ListOptions{ - LabelSelector: discovery.LabelManagedBy + "=" + controllerName, + LabelSelector: discovery.LabelManagedBy + "=" + ControllerName, }) if err != nil { t.Fatalf("Expected no error fetching Endpoint Slices, got: %v", err) diff --git a/pkg/controller/endpointslicemirroring/utils.go b/pkg/controller/endpointslicemirroring/utils.go index ebcd160cc7f..7a7e3b00275 100644 --- a/pkg/controller/endpointslicemirroring/utils.go +++ b/pkg/controller/endpointslicemirroring/utils.go @@ -73,7 +73,7 @@ func newEndpointSlice(endpoints *corev1.Endpoints, ports []discovery.EndpointPor // overwrite specific labels epSlice.Labels[discovery.LabelServiceName] = endpoints.Name - epSlice.Labels[discovery.LabelManagedBy] = controllerName + epSlice.Labels[discovery.LabelManagedBy] = ControllerName // clone all annotations but EndpointsLastChangeTriggerTime and LastAppliedConfigAnnotation for annotation, val := range endpoints.Annotations { @@ -267,5 +267,5 @@ func managedByChanged(endpointSlice1, endpointSlice2 *discovery.EndpointSlice) b // EndpointSlices is the EndpointSlice controller. func managedByController(endpointSlice *discovery.EndpointSlice) bool { managedBy, _ := endpointSlice.Labels[discovery.LabelManagedBy] - return managedBy == controllerName + return managedBy == ControllerName } diff --git a/pkg/controller/endpointslicemirroring/utils_test.go b/pkg/controller/endpointslicemirroring/utils_test.go index 4daa8632e49..91c62de5f84 100644 --- a/pkg/controller/endpointslicemirroring/utils_test.go +++ b/pkg/controller/endpointslicemirroring/utils_test.go @@ -64,7 +64,7 @@ func TestNewEndpointSlice(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ discovery.LabelServiceName: endpoints.Name, - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, Annotations: map[string]string{}, GenerateName: fmt.Sprintf("%s-", endpoints.Name), @@ -86,7 +86,7 @@ func TestNewEndpointSlice(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ discovery.LabelServiceName: endpoints.Name, - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, Annotations: map[string]string{"foo": "bar"}, GenerateName: fmt.Sprintf("%s-", endpoints.Name), @@ -109,7 +109,7 @@ func TestNewEndpointSlice(t *testing.T) { Labels: map[string]string{ "foo": "bar", discovery.LabelServiceName: endpoints.Name, - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, Annotations: map[string]string{}, GenerateName: fmt.Sprintf("%s-", endpoints.Name), @@ -134,7 +134,7 @@ func TestNewEndpointSlice(t *testing.T) { Labels: map[string]string{ "foo": "bar", discovery.LabelServiceName: endpoints.Name, - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, Annotations: map[string]string{"foo2": "bar2"}, GenerateName: fmt.Sprintf("%s-", endpoints.Name), @@ -162,7 +162,7 @@ func TestNewEndpointSlice(t *testing.T) { Labels: map[string]string{ "foo": "bar", discovery.LabelServiceName: endpoints.Name, - discovery.LabelManagedBy: controllerName, + discovery.LabelManagedBy: ControllerName, }, Annotations: map[string]string{"foo2": "bar2"}, GenerateName: fmt.Sprintf("%s-", endpoints.Name), diff --git a/pkg/registry/core/endpoint/strategy.go b/pkg/registry/core/endpoint/strategy.go index 3e0f5d30218..f67a0c34ba2 100644 --- a/pkg/registry/core/endpoint/strategy.go +++ b/pkg/registry/core/endpoint/strategy.go @@ -20,11 +20,13 @@ import ( "context" "k8s.io/apimachinery/pkg/runtime" + utilvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/storage/names" "k8s.io/kubernetes/pkg/api/legacyscheme" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/core/validation" + endpointscontroller "k8s.io/kubernetes/pkg/controller/endpoint" ) // endpointsStrategy implements behavior for Endpoints @@ -57,7 +59,7 @@ func (endpointsStrategy) Validate(ctx context.Context, obj runtime.Object) field // WarningsOnCreate returns warnings for the creation of the given object. func (endpointsStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { - return nil + return endpointsWarnings(obj.(*api.Endpoints)) } // Canonicalize normalizes the object after validation. @@ -76,9 +78,33 @@ func (endpointsStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Ob // WarningsOnUpdate returns warnings for the given update. func (endpointsStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { - return nil + return endpointsWarnings(obj.(*api.Endpoints)) } func (endpointsStrategy) AllowUnconditionalUpdate() bool { return true } + +func endpointsWarnings(endpoints *api.Endpoints) []string { + // Save time by not checking for bad IPs if the request is coming from the + // Endpoints controller, since we know it fixes up any invalid IPs from its input + // data when outputting the Endpoints. (The "managed-by" label is new, so this + // heuristic may fail in skewed clusters, but that just means we won't get the + // optimization during the skew.) + if endpoints.Labels[endpointscontroller.LabelManagedBy] == endpointscontroller.ControllerName { + return nil + } + + var warnings []string + for i := range endpoints.Subsets { + for j := range endpoints.Subsets[i].Addresses { + fldPath := field.NewPath("subsets").Index(i).Child("addresses").Index(j).Child("ip") + warnings = append(warnings, utilvalidation.GetWarningsForIP(fldPath, endpoints.Subsets[i].Addresses[j].IP)...) + } + for j := range endpoints.Subsets[i].NotReadyAddresses { + fldPath := field.NewPath("subsets").Index(i).Child("notReadyAddresses").Index(j).Child("ip") + warnings = append(warnings, utilvalidation.GetWarningsForIP(fldPath, endpoints.Subsets[i].NotReadyAddresses[j].IP)...) + } + } + return warnings +} diff --git a/pkg/registry/core/endpoint/strategy_test.go b/pkg/registry/core/endpoint/strategy_test.go new file mode 100644 index 00000000000..978cdbfaa00 --- /dev/null +++ b/pkg/registry/core/endpoint/strategy_test.go @@ -0,0 +1,124 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package endpoint + +import ( + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + api "k8s.io/kubernetes/pkg/apis/core" +) + +func Test_endpointsWarning(t *testing.T) { + tests := []struct { + name string + endpoints *api.Endpoints + warnings []string + }{ + { + name: "empty Endpoints", + endpoints: &api.Endpoints{}, + warnings: nil, + }, + { + name: "valid Endpoints", + endpoints: &api.Endpoints{ + Subsets: []api.EndpointSubset{ + { + Addresses: []api.EndpointAddress{ + {IP: "1.2.3.4"}, + {IP: "fd00::1234"}, + }, + }, + }, + }, + warnings: nil, + }, + { + name: "bad Endpoints", + endpoints: &api.Endpoints{ + Subsets: []api.EndpointSubset{ + { + Addresses: []api.EndpointAddress{ + {IP: "fd00::1234"}, + {IP: "01.02.03.04"}, + }, + }, + { + Addresses: []api.EndpointAddress{ + {IP: "::ffff:1.2.3.4"}, + }, + }, + { + Addresses: []api.EndpointAddress{ + {IP: "1.2.3.4"}, + }, + NotReadyAddresses: []api.EndpointAddress{ + {IP: "::ffff:1.2.3.4"}, + }, + }, + }, + }, + warnings: []string{ + "subsets[0].addresses[1].ip", + "subsets[1].addresses[0].ip", + "subsets[2].notReadyAddresses[0].ip", + }, + }, + { + // We don't actually want to let bad IPs through in this case; the + // point here is that we trust the Endpoints controller to not do + // that, and we're testing that the checks correctly get skipped. + name: "bad Endpoints ignored because of label", + endpoints: &api.Endpoints{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "endpoints.kubernetes.io/managed-by": "endpoint-controller", + }, + }, + Subsets: []api.EndpointSubset{ + { + Addresses: []api.EndpointAddress{ + {IP: "fd00::1234"}, + {IP: "01.02.03.04"}, + }, + }, + }, + }, + warnings: nil, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + warnings := endpointsWarnings(test.endpoints) + ok := len(warnings) == len(test.warnings) + if ok { + for i := range warnings { + if !strings.HasPrefix(warnings[i], test.warnings[i]) { + ok = false + break + } + } + } + if !ok { + t.Errorf("Expected warnings for %v, got %v", test.warnings, warnings) + } + }) + } +} diff --git a/pkg/registry/core/node/strategy.go b/pkg/registry/core/node/strategy.go index a699f084511..1569085a63c 100644 --- a/pkg/registry/core/node/strategy.go +++ b/pkg/registry/core/node/strategy.go @@ -30,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" utilnet "k8s.io/apimachinery/pkg/util/net" + utilvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/registry/generic" pkgstorage "k8s.io/apiserver/pkg/storage" @@ -131,7 +132,7 @@ func (nodeStrategy) Validate(ctx context.Context, obj runtime.Object) field.Erro // WarningsOnCreate returns warnings for the creation of the given object. func (nodeStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { - return fieldIsDeprecatedWarnings(obj) + return nodeWarnings(obj) } // Canonicalize normalizes the object after validation. @@ -146,7 +147,7 @@ func (nodeStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) // WarningsOnUpdate returns warnings for the given update. func (nodeStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { - return fieldIsDeprecatedWarnings(obj) + return nodeWarnings(obj) } func (nodeStrategy) AllowUnconditionalUpdate() bool { @@ -287,7 +288,7 @@ func isProxyableHostname(ctx context.Context, hostname string) error { return nil } -func fieldIsDeprecatedWarnings(obj runtime.Object) []string { +func nodeWarnings(obj runtime.Object) []string { newNode := obj.(*api.Node) var warnings []string if newNode.Spec.ConfigSource != nil { @@ -297,6 +298,14 @@ func fieldIsDeprecatedWarnings(obj runtime.Object) []string { if len(newNode.Spec.DoNotUseExternalID) > 0 { warnings = append(warnings, "spec.externalID: this field is deprecated, and is unused by Kubernetes") } + + if len(newNode.Spec.PodCIDRs) > 0 { + podCIDRsField := field.NewPath("spec", "podCIDRs") + for i, value := range newNode.Spec.PodCIDRs { + warnings = append(warnings, utilvalidation.GetWarningsForCIDR(podCIDRsField.Index(i), value)...) + } + } + return warnings } diff --git a/pkg/registry/core/node/strategy_test.go b/pkg/registry/core/node/strategy_test.go index bf5aa74bf1b..5addfc9e7b0 100644 --- a/pkg/registry/core/node/strategy_test.go +++ b/pkg/registry/core/node/strategy_test.go @@ -287,25 +287,65 @@ func TestWarningOnUpdateAndCreate(t *testing.T) { node api.Node warningText string }{ - {api.Node{}, + { api.Node{}, - ""}, - {api.Node{}, + api.Node{}, + "", + }, + { + api.Node{}, + //nolint:staticcheck // ignore deprecation warning api.Node{Spec: api.NodeSpec{ConfigSource: &api.NodeConfigSource{}}}, - "spec.configSource"}, - {api.Node{Spec: api.NodeSpec{ConfigSource: &api.NodeConfigSource{}}}, + "spec.configSource", + }, + { + //nolint:staticcheck // ignore deprecation warning api.Node{Spec: api.NodeSpec{ConfigSource: &api.NodeConfigSource{}}}, - "spec.configSource"}, - {api.Node{Spec: api.NodeSpec{ConfigSource: &api.NodeConfigSource{}}}, - api.Node{}, ""}, - {api.Node{}, + //nolint:staticcheck // ignore deprecation warning + api.Node{Spec: api.NodeSpec{ConfigSource: &api.NodeConfigSource{}}}, + "spec.configSource", + }, + { + //nolint:staticcheck // ignore deprecation warning + api.Node{Spec: api.NodeSpec{ConfigSource: &api.NodeConfigSource{}}}, + api.Node{}, + "", + }, + { + api.Node{}, api.Node{Spec: api.NodeSpec{DoNotUseExternalID: "externalID"}}, - "spec.externalID"}, - {api.Node{Spec: api.NodeSpec{DoNotUseExternalID: "externalID"}}, + "spec.externalID", + }, + { api.Node{Spec: api.NodeSpec{DoNotUseExternalID: "externalID"}}, - "spec.externalID"}, - {api.Node{Spec: api.NodeSpec{DoNotUseExternalID: "externalID"}}, - api.Node{}, ""}, + api.Node{Spec: api.NodeSpec{DoNotUseExternalID: "externalID"}}, + "spec.externalID", + }, + { + api.Node{Spec: api.NodeSpec{DoNotUseExternalID: "externalID"}}, + api.Node{}, + "", + }, + { + api.Node{}, + api.Node{Spec: api.NodeSpec{PodCIDRs: []string{"10.0.1.0/24", "fd00::/64"}}}, + "", + }, + { + api.Node{}, + api.Node{Spec: api.NodeSpec{PodCIDRs: []string{"010.000.001.000/24", "fd00::/64"}}}, + "spec.podCIDRs[0]", + }, + { + api.Node{}, + api.Node{Spec: api.NodeSpec{PodCIDRs: []string{"10.0.1.0/24", "fd00::1234/64"}}}, + "spec.podCIDRs[1]", + }, + { + api.Node{Spec: api.NodeSpec{PodCIDRs: []string{"010.000.001.000/24", "fd00::1234/64"}}}, + api.Node{Spec: api.NodeSpec{PodCIDRs: []string{"10.0.1.0/24", "fd00::/64"}}}, + "", + }, } for i, test := range tests { warnings := (nodeStrategy{}).WarningsOnUpdate(context.Background(), &test.node, &test.oldNode) diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index 505e2c50bdd..b95adcd746f 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/pkg/registry/core/pod/strategy.go @@ -270,7 +270,17 @@ func (podStatusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Ob // WarningsOnUpdate returns warnings for the given update. func (podStatusStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { - return nil + pod := obj.(*api.Pod) + var warnings []string + + for i, podIP := range pod.Status.PodIPs { + warnings = append(warnings, utilvalidation.GetWarningsForIP(field.NewPath("status", "podIPs").Index(i).Child("ip"), podIP.IP)...) + } + for i, hostIP := range pod.Status.HostIPs { + warnings = append(warnings, utilvalidation.GetWarningsForIP(field.NewPath("status", "hostIPs").Index(i).Child("ip"), hostIP.IP)...) + } + + return warnings } type podEphemeralContainersStrategy struct { diff --git a/pkg/registry/core/pod/strategy_test.go b/pkg/registry/core/pod/strategy_test.go index d0aefefb51d..d560240e0b3 100644 --- a/pkg/registry/core/pod/strategy_test.go +++ b/pkg/registry/core/pod/strategy_test.go @@ -3562,3 +3562,68 @@ func TestStatusPrepareForUpdate(t *testing.T) { }) } } + +func TestWarningsOnUpdate(t *testing.T) { + tests := []struct { + name string + pod *api.Pod + warnings []string + }{ + { + name: "no podIPs/hostIPs", + pod: &api.Pod{Status: api.PodStatus{}}, + warnings: nil, + }, + { + name: "valid podIPs/hostIPs", + pod: &api.Pod{ + Status: api.PodStatus{ + PodIPs: []api.PodIP{ + {IP: "1.2.3.4"}, + }, + HostIPs: []api.HostIP{ + {IP: "5.6.7.8"}, + {IP: "fd00::5678"}, + }, + }, + }, + warnings: nil, + }, + { + name: "bad podIPs/hostIPs", + pod: &api.Pod{ + Status: api.PodStatus{ + PodIPs: []api.PodIP{ + {IP: "01.02.03.04"}, + }, + HostIPs: []api.HostIP{ + {IP: "5.6.7.8"}, + {IP: "::ffff:9.10.11.12"}, + }, + }, + }, + warnings: []string{ + "status.podIPs[0].ip", + "status.hostIPs[1].ip", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + warnings := StatusStrategy.WarningsOnUpdate(context.Background(), test.pod, test.pod) + ok := len(warnings) == len(test.warnings) + if ok { + for i := range warnings { + if !strings.HasPrefix(warnings[i], test.warnings[i]) { + ok = false + break + } + } + } + if !ok { + t.Errorf("Expected warnings for %v, got %v", test.warnings, warnings) + } + }) + } +} diff --git a/pkg/registry/core/service/strategy.go b/pkg/registry/core/service/strategy.go index a5916b796bb..2a10734af5e 100644 --- a/pkg/registry/core/service/strategy.go +++ b/pkg/registry/core/service/strategy.go @@ -25,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" + utilvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/registry/generic" pkgstorage "k8s.io/apiserver/pkg/storage" @@ -168,7 +169,19 @@ func (serviceStatusStrategy) ValidateUpdate(ctx context.Context, obj, old runtim // WarningsOnUpdate returns warnings for the given update. func (serviceStatusStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { - return nil + svc := obj.(*api.Service) + var warnings []string + + if len(svc.Status.LoadBalancer.Ingress) > 0 { + fieldPath := field.NewPath("status", "loadBalancer", "ingress") + for i, ingress := range svc.Status.LoadBalancer.Ingress { + if len(ingress.IP) > 0 { + warnings = append(warnings, utilvalidation.GetWarningsForIP(fieldPath.Index(i), ingress.IP)...) + } + } + } + + return warnings } // GetAttrs returns labels and fields of a given object for filtering purposes. diff --git a/pkg/registry/core/service/strategy_test.go b/pkg/registry/core/service/strategy_test.go index 549e827f8c8..5ee914ff054 100644 --- a/pkg/registry/core/service/strategy_test.go +++ b/pkg/registry/core/service/strategy_test.go @@ -138,6 +138,18 @@ func TestServiceStatusStrategy(t *testing.T) { if len(errs) != 0 { t.Errorf("Unexpected error %v", errs) } + + warnings := StatusStrategy.WarningsOnUpdate(ctx, newService, oldService) + if len(warnings) != 0 { + t.Errorf("Unexpected warnings %v", errs) + } + + // Bad IP warning (leading zeros) + newService.Status.LoadBalancer.Ingress[0].IP = "127.000.000.002" + warnings = StatusStrategy.WarningsOnUpdate(ctx, newService, oldService) + if len(warnings) != 1 { + t.Errorf("Did not get warning for bad IP") + } } func makeServiceWithConditions(conditions []metav1.Condition) *api.Service { diff --git a/pkg/registry/discovery/endpointslice/strategy.go b/pkg/registry/discovery/endpointslice/strategy.go index 62571f294be..d7887ef0c46 100644 --- a/pkg/registry/discovery/endpointslice/strategy.go +++ b/pkg/registry/discovery/endpointslice/strategy.go @@ -21,12 +21,14 @@ import ( "fmt" corev1 "k8s.io/api/core/v1" + discoveryv1 "k8s.io/api/discovery/v1" discoveryv1beta1 "k8s.io/api/discovery/v1beta1" apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" + utilvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/storage/names" @@ -35,6 +37,8 @@ import ( apivalidation "k8s.io/kubernetes/pkg/apis/core/validation" "k8s.io/kubernetes/pkg/apis/discovery" "k8s.io/kubernetes/pkg/apis/discovery/validation" + endpointslicecontroller "k8s.io/kubernetes/pkg/controller/endpointslice" + endpointslicemirroringcontroller "k8s.io/kubernetes/pkg/controller/endpointslicemirroring" "k8s.io/kubernetes/pkg/features" ) @@ -99,6 +103,7 @@ func (endpointSliceStrategy) WarningsOnCreate(ctx context.Context, obj runtime.O } var warnings []string warnings = append(warnings, warnOnDeprecatedAddressType(eps.AddressType)...) + warnings = append(warnings, warnOnBadIPs(eps)...) return warnings } @@ -120,7 +125,13 @@ func (endpointSliceStrategy) ValidateUpdate(ctx context.Context, new, old runtim // WarningsOnUpdate returns warnings for the given update. func (endpointSliceStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { - return nil + eps := obj.(*discovery.EndpointSlice) + if eps == nil { + return nil + } + var warnings []string + warnings = append(warnings, warnOnBadIPs(eps)...) + return warnings } // AllowUnconditionalUpdate is the default update policy for EndpointSlice objects. @@ -222,3 +233,23 @@ func warnOnDeprecatedAddressType(addressType discovery.AddressType) []string { } return nil } + +// warnOnBadIPs returns warnings for bad IP address formats +func warnOnBadIPs(eps *discovery.EndpointSlice) []string { + // Save time by not checking for bad IPs if the request is coming from one of our + // controllers, since we know they fix up any invalid IPs from their input data + // when outputting the EndpointSlices. + if eps.Labels[discoveryv1.LabelManagedBy] == endpointslicecontroller.ControllerName || + eps.Labels[discoveryv1.LabelManagedBy] == endpointslicemirroringcontroller.ControllerName { + return nil + } + + var warnings []string + for i := range eps.Endpoints { + for j, addr := range eps.Endpoints[i].Addresses { + fldPath := field.NewPath("endpoints").Index(i).Child("addresses").Index(j) + warnings = append(warnings, utilvalidation.GetWarningsForIP(fldPath, addr)...) + } + } + return warnings +} diff --git a/pkg/registry/discovery/endpointslice/strategy_test.go b/pkg/registry/discovery/endpointslice/strategy_test.go index 0c12bf5804e..0d23e039297 100644 --- a/pkg/registry/discovery/endpointslice/strategy_test.go +++ b/pkg/registry/discovery/endpointslice/strategy_test.go @@ -18,6 +18,7 @@ package endpointslice import ( "context" + "strings" "testing" corev1 "k8s.io/api/core/v1" @@ -1014,3 +1015,86 @@ func TestWarningsOnEndpointSliceAddressType(t *testing.T) { }) } } + +func Test_warnOnBadIPs(t *testing.T) { + tests := []struct { + name string + slice *discovery.EndpointSlice + warnings []string + }{ + { + name: "empty EndpointSlice", + slice: &discovery.EndpointSlice{}, + warnings: nil, + }, + { + name: "valid EndpointSlice", + slice: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Addresses: []string{ + "1.2.3.4", + "fd00::1234", + }, + }, + }, + }, + warnings: nil, + }, + { + name: "bad EndpointSlice", + slice: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Addresses: []string{ + "fd00::1234", + "01.02.03.04", + }, + }, + { + Addresses: []string{ + "::ffff:1.2.3.4", + }, + }, + }, + }, + warnings: []string{ + "endpoints[0].addresses[1]", + "endpoints[1].addresses[0]", + }, + }, + { + name: "bad EndpointSlice ignored because of label", + slice: &discovery.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "endpointslice.kubernetes.io/managed-by": "endpointslice-controller.k8s.io", + }, + }, + Endpoints: []discovery.Endpoint{ + { + Addresses: []string{ + "fd00::1234", + "01.02.03.04", + }, + }, + }, + }, + warnings: nil, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + warnings := warnOnBadIPs(test.slice) + if len(warnings) != len(test.warnings) { + t.Fatalf("Expected warnings %v, got %v", test.warnings, warnings) + } + for i := range warnings { + if !strings.HasPrefix(warnings[i], test.warnings[i]) { + t.Fatalf("Expected warnings %v, got %v", test.warnings, warnings) + } + } + }) + } +} diff --git a/pkg/registry/networking/ingress/strategy.go b/pkg/registry/networking/ingress/strategy.go index a23763b9f3d..d7c737efbea 100644 --- a/pkg/registry/networking/ingress/strategy.go +++ b/pkg/registry/networking/ingress/strategy.go @@ -22,6 +22,7 @@ import ( apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/runtime" + utilvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/storage/names" "k8s.io/kubernetes/pkg/api/legacyscheme" @@ -172,5 +173,17 @@ func (ingressStatusStrategy) ValidateUpdate(ctx context.Context, obj, old runtim // WarningsOnUpdate returns warnings for the given update. func (ingressStatusStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { - return nil + newIngress := obj.(*networking.Ingress) + var warnings []string + + if len(newIngress.Status.LoadBalancer.Ingress) > 0 { + fieldPath := field.NewPath("status", "loadBalancer", "ingress") + for i, ingress := range newIngress.Status.LoadBalancer.Ingress { + if len(ingress.IP) > 0 { + warnings = append(warnings, utilvalidation.GetWarningsForIP(fieldPath.Index(i), ingress.IP)...) + } + } + } + + return warnings } diff --git a/pkg/registry/networking/ingress/strategy_test.go b/pkg/registry/networking/ingress/strategy_test.go index 2fd552ae0fe..6aabde83679 100644 --- a/pkg/registry/networking/ingress/strategy_test.go +++ b/pkg/registry/networking/ingress/strategy_test.go @@ -137,4 +137,15 @@ func TestIngressStatusStrategy(t *testing.T) { if len(errs) != 0 { t.Errorf("Unexpected error %v", errs) } + + warnings := StatusStrategy.WarningsOnUpdate(ctx, &newIngress, &oldIngress) + if len(warnings) != 0 { + t.Errorf("Unexpected warnings %v", errs) + } + + newIngress.Status.LoadBalancer.Ingress[0].IP = "127.000.000.002" + warnings = StatusStrategy.WarningsOnUpdate(ctx, &newIngress, &oldIngress) + if len(warnings) != 1 { + t.Errorf("Did not get warning for bad IP") + } } diff --git a/pkg/registry/networking/networkpolicy/strategy.go b/pkg/registry/networking/networkpolicy/strategy.go index 4e93da62a77..22b8deb144d 100644 --- a/pkg/registry/networking/networkpolicy/strategy.go +++ b/pkg/registry/networking/networkpolicy/strategy.go @@ -21,6 +21,7 @@ import ( "reflect" "k8s.io/apimachinery/pkg/runtime" + utilvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/storage/names" "k8s.io/kubernetes/pkg/api/legacyscheme" @@ -70,7 +71,7 @@ func (networkPolicyStrategy) Validate(ctx context.Context, obj runtime.Object) f // WarningsOnCreate returns warnings for the creation of the given object. func (networkPolicyStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { - return nil + return networkPolicyWarnings(obj.(*networking.NetworkPolicy)) } // Canonicalize normalizes the object after validation. @@ -91,10 +92,41 @@ func (networkPolicyStrategy) ValidateUpdate(ctx context.Context, obj, old runtim // WarningsOnUpdate returns warnings for the given update. func (networkPolicyStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { - return nil + return networkPolicyWarnings(obj.(*networking.NetworkPolicy)) } // AllowUnconditionalUpdate is the default update policy for NetworkPolicy objects. func (networkPolicyStrategy) AllowUnconditionalUpdate() bool { return true } + +func networkPolicyWarnings(networkPolicy *networking.NetworkPolicy) []string { + var warnings []string + for i := range networkPolicy.Spec.Ingress { + for j := range networkPolicy.Spec.Ingress[i].From { + ipBlock := networkPolicy.Spec.Ingress[i].From[j].IPBlock + if ipBlock == nil { + continue + } + fldPath := field.NewPath("spec").Child("ingress").Index(i).Child("from").Index(j).Child("ipBlock") + warnings = append(warnings, utilvalidation.GetWarningsForCIDR(fldPath.Child("cidr"), ipBlock.CIDR)...) + for k, except := range ipBlock.Except { + warnings = append(warnings, utilvalidation.GetWarningsForCIDR(fldPath.Child("except").Index(k), except)...) + } + } + } + for i := range networkPolicy.Spec.Egress { + for j := range networkPolicy.Spec.Egress[i].To { + ipBlock := networkPolicy.Spec.Egress[i].To[j].IPBlock + if ipBlock == nil { + continue + } + fldPath := field.NewPath("spec").Child("egress").Index(i).Child("to").Index(j).Child("ipBlock") + warnings = append(warnings, utilvalidation.GetWarningsForCIDR(fldPath.Child("cidr"), ipBlock.CIDR)...) + for k, except := range ipBlock.Except { + warnings = append(warnings, utilvalidation.GetWarningsForCIDR(fldPath.Child("except").Index(k), except)...) + } + } + } + return warnings +} diff --git a/pkg/registry/networking/networkpolicy/strategy_test.go b/pkg/registry/networking/networkpolicy/strategy_test.go index b5eca01299c..000a71f9aff 100644 --- a/pkg/registry/networking/networkpolicy/strategy_test.go +++ b/pkg/registry/networking/networkpolicy/strategy_test.go @@ -18,6 +18,7 @@ package networkpolicy import ( "context" + "strings" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -110,4 +111,43 @@ func TestNetworkPolicyStrategy(t *testing.T) { t.Errorf("Unexpected error from validation for updated Network Policy: %v", errs) } + warnings := Strategy.WarningsOnUpdate(context.Background(), updatedNetPol, netPol) + if len(warnings) != 0 { + t.Errorf("Unexpected warnings for Network Policy with no IPBlock: %v", warnings) + } + + updatedNetPol.Spec.Egress = append(updatedNetPol.Spec.Egress, + networking.NetworkPolicyEgressRule{ + To: []networking.NetworkPolicyPeer{ + { + IPBlock: &networking.IPBlock{ + CIDR: "10.0.0.0/8", + }, + }, + }, + }, + ) + warnings = Strategy.WarningsOnUpdate(context.Background(), updatedNetPol, netPol) + if len(warnings) != 0 { + t.Errorf("Unexpected warnings for Network Policy with valid IPBlock: %v", warnings) + } + + updatedNetPol.Spec.Egress = append(updatedNetPol.Spec.Egress, + networking.NetworkPolicyEgressRule{ + To: []networking.NetworkPolicyPeer{ + { + IPBlock: &networking.IPBlock{ + CIDR: "::ffff:10.0.0.0/104", + Except: []string{ + "10.0.0.1/16", + }, + }, + }, + }, + }, + ) + warnings = Strategy.WarningsOnUpdate(context.Background(), updatedNetPol, netPol) + if len(warnings) != 2 || !strings.HasPrefix(warnings[0], "spec.egress[2].to[0].ipBlock.cidr:") || !strings.HasPrefix(warnings[1], "spec.egress[2].to[0].ipBlock.except[0]:") { + t.Errorf("Incorrect warnings for Network Policy with invalid IPBlock: %v", warnings) + } } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go new file mode 100644 index 00000000000..f073c3668ca --- /dev/null +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip.go @@ -0,0 +1,139 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "fmt" + "net/netip" + + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/klog/v2" + netutils "k8s.io/utils/net" +) + +// IsValidIP tests that the argument is a valid IP address. +func IsValidIP(fldPath *field.Path, value string) field.ErrorList { + var allErrors field.ErrorList + if netutils.ParseIPSloppy(value) == nil { + allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid IP address, (e.g. 10.9.8.7 or 2001:db8::ffff)").WithOrigin("format=ip-sloppy")) + } + return allErrors +} + +// GetWarningsForIP returns warnings for IP address values in non-standard forms. This +// should only be used with fields that are validated with IsValidIP(). +func GetWarningsForIP(fldPath *field.Path, value string) []string { + ip := netutils.ParseIPSloppy(value) + if ip == nil { + klog.ErrorS(nil, "GetWarningsForIP called on value that was not validated with IsValidIP", "field", fldPath, "value", value) + return nil + } + + addr, _ := netip.ParseAddr(value) + if !addr.IsValid() || addr.Is4In6() { + // This catches 2 cases: leading 0s (if ParseIPSloppy() accepted it but + // ParseAddr() doesn't) or IPv4-mapped IPv6 (.Is4In6()). Either way, + // re-stringifying the net.IP value will give the preferred form. + return []string{ + fmt.Sprintf("%s: non-standard IP address %q will be considered invalid in a future Kubernetes release: use %q", fldPath, value, ip.String()), + } + } + + // If ParseIPSloppy() and ParseAddr() both accept it then it's fully valid, though + // it may be non-canonical. + if addr.Is6() && addr.String() != value { + return []string{ + fmt.Sprintf("%s: IPv6 address %q should be in RFC 5952 canonical format (%q)", fldPath, value, addr.String()), + } + } + + return nil +} + +// IsValidIPv4Address tests that the argument is a valid IPv4 address. +func IsValidIPv4Address(fldPath *field.Path, value string) field.ErrorList { + var allErrors field.ErrorList + ip := netutils.ParseIPSloppy(value) + if ip == nil || ip.To4() == nil { + allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid IPv4 address")) + } + return allErrors +} + +// IsValidIPv6Address tests that the argument is a valid IPv6 address. +func IsValidIPv6Address(fldPath *field.Path, value string) field.ErrorList { + var allErrors field.ErrorList + ip := netutils.ParseIPSloppy(value) + if ip == nil || ip.To4() != nil { + allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid IPv6 address")) + } + return allErrors +} + +// IsValidCIDR tests that the argument is a valid CIDR value. +func IsValidCIDR(fldPath *field.Path, value string) field.ErrorList { + var allErrors field.ErrorList + _, _, err := netutils.ParseCIDRSloppy(value) + if err != nil { + allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid CIDR value, (e.g. 10.9.8.0/24 or 2001:db8::/64)")) + } + return allErrors +} + +// GetWarningsForCIDR returns warnings for CIDR values in non-standard forms. This should +// only be used with fields that are validated with IsValidCIDR(). +func GetWarningsForCIDR(fldPath *field.Path, value string) []string { + ip, ipnet, err := netutils.ParseCIDRSloppy(value) + if err != nil { + klog.ErrorS(err, "GetWarningsForCIDR called on value that was not validated with IsValidCIDR", "field", fldPath, "value", value) + return nil + } + + var warnings []string + + // Check for bits set after prefix length + if !ip.Equal(ipnet.IP) { + _, addrlen := ipnet.Mask.Size() + singleIPCIDR := fmt.Sprintf("%s/%d", ip.String(), addrlen) + warnings = append(warnings, + fmt.Sprintf("%s: CIDR value %q is ambiguous in this context (should be %q or %q?)", fldPath, value, ipnet.String(), singleIPCIDR), + ) + } + + prefix, _ := netip.ParsePrefix(value) + addr := prefix.Addr() + if !prefix.IsValid() || addr.Is4In6() { + // This catches 2 cases: leading 0s (if ParseCIDRSloppy() accepted it but + // ParsePrefix() doesn't) or IPv4-mapped IPv6 (.Is4In6()). Either way, + // re-stringifying the net.IPNet value will give the preferred form. + warnings = append(warnings, + fmt.Sprintf("%s: non-standard CIDR value %q will be considered invalid in a future Kubernetes release: use %q", fldPath, value, ipnet.String()), + ) + } + + // If ParseCIDRSloppy() and ParsePrefix() both accept it then it's fully valid, + // though it may be non-canonical. But only check this if there are no other + // warnings, since either of the other warnings would also cause a round-trip + // failure. + if len(warnings) == 0 && addr.Is6() && prefix.String() != value { + warnings = append(warnings, + fmt.Sprintf("%s: IPv6 CIDR value %q should be in RFC 5952 canonical format (%q)", fldPath, value, prefix.String()), + ) + } + + return warnings +} 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 new file mode 100644 index 00000000000..6fb5d4f15cc --- /dev/null +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/ip_test.go @@ -0,0 +1,452 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "reflect" + "strings" + "testing" + + "k8s.io/apimachinery/pkg/util/validation/field" +) + +func TestIsValidIP(t *testing.T) { + for _, tc := range []struct { + name string + in string + family int + err string + }{ + // GOOD VALUES + { + name: "ipv4", + in: "1.2.3.4", + family: 4, + }, + { + name: "ipv4, all zeros", + in: "0.0.0.0", + family: 4, + }, + { + name: "ipv4, max", + in: "255.255.255.255", + family: 4, + }, + { + name: "ipv6", + in: "1234::abcd", + family: 6, + }, + { + name: "ipv6, all zeros, collapsed", + in: "::", + family: 6, + }, + { + name: "ipv6, max", + in: "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", + family: 6, + }, + + // GOOD, THOUGH NON-CANONICAL, VALUES + { + name: "ipv6, all zeros, expanded (non-canonical)", + in: "0:0:0:0:0:0:0:0", + family: 6, + }, + { + name: "ipv6, leading 0s (non-canonical)", + in: "0001:002:03:4::", + family: 6, + }, + { + name: "ipv6, capital letters (non-canonical)", + in: "1234::ABCD", + family: 6, + }, + + // BAD VALUES WE CURRENTLY CONSIDER GOOD + { + name: "ipv4 with leading 0s", + in: "1.1.1.01", + family: 4, + }, + { + name: "ipv4-in-ipv6 value", + in: "::ffff:1.1.1.1", + family: 4, + }, + + // BAD VALUES + { + name: "empty string", + in: "", + err: "must be a valid IP address", + }, + { + name: "junk", + in: "aaaaaaa", + err: "must be a valid IP address", + }, + { + name: "domain name", + in: "myhost.mydomain", + err: "must be a valid IP address", + }, + { + name: "cidr", + in: "1.2.3.0/24", + err: "must be a valid IP address", + }, + { + name: "ipv4 with out-of-range octets", + in: "1.2.3.400", + err: "must be a valid IP address", + }, + { + name: "ipv4 with negative octets", + in: "-1.0.0.0", + err: "must be a valid IP address", + }, + { + name: "ipv6 with out-of-range segment", + in: "2001:db8::10005", + err: "must be a valid IP address", + }, + { + name: "ipv4:port", + in: "1.2.3.4:80", + err: "must be a valid IP address", + }, + { + name: "ipv6 with brackets", + in: "[2001:db8::1]", + err: "must be a valid IP address", + }, + { + name: "[ipv6]:port", + in: "[2001:db8::1]:80", + err: "must be a valid IP address", + }, + { + name: "host:port", + in: "example.com:80", + err: "must be a valid IP address", + }, + { + name: "ipv6 with zone", + in: "1234::abcd%eth0", + err: "must be a valid IP address", + }, + { + name: "ipv4 with zone", + in: "169.254.0.0%eth0", + err: "must be a valid IP address", + }, + } { + t.Run(tc.name, func(t *testing.T) { + errs := IsValidIP(field.NewPath(""), tc.in) + if tc.err == "" { + if len(errs) != 0 { + t.Errorf("expected %q to be valid but got: %v", tc.in, errs) + } + } else { + if len(errs) != 1 { + t.Errorf("expected %q to have 1 error but got: %v", tc.in, errs) + } else if !strings.Contains(errs[0].Detail, tc.err) { + t.Errorf("expected error for %q to contain %q but got: %q", tc.in, tc.err, errs[0].Detail) + } + } + + errs = IsValidIPv4Address(field.NewPath(""), tc.in) + if tc.family == 4 { + if len(errs) != 0 { + t.Errorf("expected %q to pass IsValidIPv4Address but got: %v", tc.in, errs) + } + } else { + if len(errs) == 0 { + t.Errorf("expected %q to fail IsValidIPv4Address", tc.in) + } + } + + errs = IsValidIPv6Address(field.NewPath(""), tc.in) + if tc.family == 6 { + if len(errs) != 0 { + t.Errorf("expected %q to pass IsValidIPv6Address but got: %v", tc.in, errs) + } + } else { + if len(errs) == 0 { + t.Errorf("expected %q to fail IsValidIPv6Address", tc.in) + } + } + }) + } +} + +func TestGetWarningsForIP(t *testing.T) { + tests := []struct { + name string + fieldPath *field.Path + address string + want []string + }{ + { + name: "IPv4 No failures", + address: "192.12.2.2", + fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0), + want: nil, + }, + { + name: "IPv6 No failures", + address: "2001:db8::2", + fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0), + want: nil, + }, + { + name: "IPv4 with leading zeros", + address: "192.012.2.2", + fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0), + want: []string{ + `spec.clusterIPs[0]: non-standard IP address "192.012.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`, + }, + }, + { + name: "IPv4-mapped IPv6", + address: "::ffff:192.12.2.2", + fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0), + want: []string{ + `spec.clusterIPs[0]: non-standard IP address "::ffff:192.12.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`, + }, + }, + { + name: "IPv6 non-canonical format", + address: "2001:db8:0:0::2", + fieldPath: field.NewPath("spec").Child("loadBalancerIP"), + want: []string{ + `spec.loadBalancerIP: IPv6 address "2001:db8:0:0::2" should be in RFC 5952 canonical format ("2001:db8::2")`, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := GetWarningsForIP(tt.fieldPath, tt.address); !reflect.DeepEqual(got, tt.want) { + t.Errorf("getWarningsForIP() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestIsValidCIDR(t *testing.T) { + for _, tc := range []struct { + name string + in string + err string + }{ + // GOOD VALUES + { + name: "ipv4", + in: "1.0.0.0/8", + }, + { + name: "ipv4, all IPs", + in: "0.0.0.0/0", + }, + { + name: "ipv4, single IP", + in: "1.1.1.1/32", + }, + { + name: "ipv6", + in: "2001:4860:4860::/48", + }, + { + name: "ipv6, all IPs", + in: "::/0", + }, + { + name: "ipv6, single IP", + in: "::1/128", + }, + + // GOOD, THOUGH NON-CANONICAL, VALUES + { + name: "ipv6, extra 0s (non-canonical)", + in: "2a00:79e0:2:0::/64", + }, + { + name: "ipv6, capital letters (non-canonical)", + in: "2001:DB8::/64", + }, + + // BAD VALUES WE CURRENTLY CONSIDER GOOD + { + name: "ipv4 with leading 0s", + in: "1.1.01.0/24", + }, + { + name: "ipv4-in-ipv6 with ipv4-sized prefix", + in: "::ffff:1.1.1.0/24", + }, + { + name: "ipv4-in-ipv6 with ipv6-sized prefix", + in: "::ffff:1.1.1.0/120", + }, + { + name: "ipv4 with bits past prefix", + in: "1.2.3.4/24", + }, + { + name: "ipv6 with bits past prefix", + in: "2001:db8::1/64", + }, + { + name: "prefix length with leading 0s", + in: "192.168.0.0/016", + }, + + // BAD VALUES + { + name: "empty string", + in: "", + err: "must be a valid CIDR value", + }, + { + name: "junk", + in: "aaaaaaa", + err: "must be a valid CIDR value", + }, + { + name: "IP address", + in: "1.2.3.4", + err: "must be a valid CIDR value", + }, + { + name: "partial URL", + in: "192.168.0.1/healthz", + err: "must be a valid CIDR value", + }, + { + name: "partial URL 2", + in: "192.168.0.1/0/99", + err: "must be a valid CIDR value", + }, + { + name: "negative prefix length", + in: "192.168.0.0/-16", + err: "must be a valid CIDR value", + }, + { + name: "prefix length with sign", + in: "192.168.0.0/+16", + err: "must be a valid CIDR value", + }, + } { + t.Run(tc.name, func(t *testing.T) { + errs := IsValidCIDR(field.NewPath(""), tc.in) + if tc.err == "" { + if len(errs) != 0 { + t.Errorf("expected %q to be valid but got: %v", tc.in, errs) + } + } else { + if len(errs) != 1 { + t.Errorf("expected %q to have 1 error but got: %v", tc.in, errs) + } else if !strings.Contains(errs[0].Detail, tc.err) { + t.Errorf("expected error for %q to contain %q but got: %q", tc.in, tc.err, errs[0].Detail) + } + } + }) + } +} + +func TestGetWarningsForCIDR(t *testing.T) { + tests := []struct { + name string + fieldPath *field.Path + cidr string + want []string + }{ + { + name: "IPv4 No failures", + cidr: "192.12.2.0/24", + fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), + want: nil, + }, + { + name: "IPv6 No failures", + cidr: "2001:db8::/64", + fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), + want: nil, + }, + { + name: "IPv4 with leading zeros", + cidr: "192.012.2.0/24", + fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), + want: []string{ + `spec.loadBalancerSourceRanges[0]: non-standard CIDR value "192.012.2.0/24" will be considered invalid in a future Kubernetes release: use "192.12.2.0/24"`, + }, + }, + { + name: "leading zeros in prefix length", + cidr: "192.12.2.0/024", + fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), + want: []string{ + `spec.loadBalancerSourceRanges[0]: non-standard CIDR value "192.12.2.0/024" will be considered invalid in a future Kubernetes release: use "192.12.2.0/24"`, + }, + }, + { + name: "IPv4-mapped IPv6", + cidr: "::ffff:192.12.2.0/120", + fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), + want: []string{ + `spec.loadBalancerSourceRanges[0]: non-standard CIDR value "::ffff:192.12.2.0/120" will be considered invalid in a future Kubernetes release: use "192.12.2.0/24"`, + }, + }, + { + name: "bits after prefix length", + cidr: "192.12.2.8/24", + fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), + want: []string{ + `spec.loadBalancerSourceRanges[0]: CIDR value "192.12.2.8/24" is ambiguous in this context (should be "192.12.2.0/24" or "192.12.2.8/32"?)`, + }, + }, + { + name: "multiple problems", + cidr: "192.012.2.8/24", + fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), + want: []string{ + `spec.loadBalancerSourceRanges[0]: CIDR value "192.012.2.8/24" is ambiguous in this context (should be "192.12.2.0/24" or "192.12.2.8/32"?)`, + `spec.loadBalancerSourceRanges[0]: non-standard CIDR value "192.012.2.8/24" will be considered invalid in a future Kubernetes release: use "192.12.2.0/24"`, + }, + }, + { + name: "IPv6 non-canonical format", + cidr: "2001:db8:0:0::/64", + fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), + want: []string{ + `spec.loadBalancerSourceRanges[0]: IPv6 CIDR value "2001:db8:0:0::/64" should be in RFC 5952 canonical format ("2001:db8::/64")`, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := GetWarningsForCIDR(tt.fieldPath, tt.cidr); !reflect.DeepEqual(got, tt.want) { + t.Errorf("getWarningsForCIDR() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go index feb3d259b8a..b6be7af1b16 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go @@ -24,7 +24,6 @@ import ( "unicode" "k8s.io/apimachinery/pkg/util/validation/field" - netutils "k8s.io/utils/net" ) const qnameCharFmt string = "[A-Za-z0-9]" @@ -369,45 +368,6 @@ func IsValidPortName(port string) []string { return errs } -// IsValidIP tests that the argument is a valid IP address. -func IsValidIP(fldPath *field.Path, value string) field.ErrorList { - var allErrors field.ErrorList - if netutils.ParseIPSloppy(value) == nil { - allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid IP address, (e.g. 10.9.8.7 or 2001:db8::ffff)").WithOrigin("format=ip-sloppy")) - } - return allErrors -} - -// IsValidIPv4Address tests that the argument is a valid IPv4 address. -func IsValidIPv4Address(fldPath *field.Path, value string) field.ErrorList { - var allErrors field.ErrorList - ip := netutils.ParseIPSloppy(value) - if ip == nil || ip.To4() == nil { - allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid IPv4 address")) - } - return allErrors -} - -// IsValidIPv6Address tests that the argument is a valid IPv6 address. -func IsValidIPv6Address(fldPath *field.Path, value string) field.ErrorList { - var allErrors field.ErrorList - ip := netutils.ParseIPSloppy(value) - if ip == nil || ip.To4() != nil { - allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid IPv6 address")) - } - return allErrors -} - -// IsValidCIDR tests that the argument is a valid CIDR value. -func IsValidCIDR(fldPath *field.Path, value string) field.ErrorList { - var allErrors field.ErrorList - _, _, err := netutils.ParseCIDRSloppy(value) - if err != nil { - allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid CIDR value, (e.g. 10.9.8.0/24 or 2001:db8::/64)")) - } - return allErrors -} - const percentFmt string = "[0-9]+%" const percentErrMsg string = "a valid percent string must be a numeric string followed by an ending '%'" diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation_test.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/validation_test.go index 8bcf36b6b3d..848fb60edb9 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/validation_test.go @@ -322,302 +322,6 @@ func TestIsValidLabelValue(t *testing.T) { } } -func TestIsValidIP(t *testing.T) { - for _, tc := range []struct { - name string - in string - family int - err string - }{ - // GOOD VALUES - { - name: "ipv4", - in: "1.2.3.4", - family: 4, - }, - { - name: "ipv4, all zeros", - in: "0.0.0.0", - family: 4, - }, - { - name: "ipv4, max", - in: "255.255.255.255", - family: 4, - }, - { - name: "ipv6", - in: "1234::abcd", - family: 6, - }, - { - name: "ipv6, all zeros, collapsed", - in: "::", - family: 6, - }, - { - name: "ipv6, max", - in: "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", - family: 6, - }, - - // GOOD, THOUGH NON-CANONICAL, VALUES - { - name: "ipv6, all zeros, expanded (non-canonical)", - in: "0:0:0:0:0:0:0:0", - family: 6, - }, - { - name: "ipv6, leading 0s (non-canonical)", - in: "0001:002:03:4::", - family: 6, - }, - { - name: "ipv6, capital letters (non-canonical)", - in: "1234::ABCD", - family: 6, - }, - - // BAD VALUES WE CURRENTLY CONSIDER GOOD - { - name: "ipv4 with leading 0s", - in: "1.1.1.01", - family: 4, - }, - { - name: "ipv4-in-ipv6 value", - in: "::ffff:1.1.1.1", - family: 4, - }, - - // BAD VALUES - { - name: "empty string", - in: "", - err: "must be a valid IP address", - }, - { - name: "junk", - in: "aaaaaaa", - err: "must be a valid IP address", - }, - { - name: "domain name", - in: "myhost.mydomain", - err: "must be a valid IP address", - }, - { - name: "cidr", - in: "1.2.3.0/24", - err: "must be a valid IP address", - }, - { - name: "ipv4 with out-of-range octets", - in: "1.2.3.400", - err: "must be a valid IP address", - }, - { - name: "ipv4 with negative octets", - in: "-1.0.0.0", - err: "must be a valid IP address", - }, - { - name: "ipv6 with out-of-range segment", - in: "2001:db8::10005", - err: "must be a valid IP address", - }, - { - name: "ipv4:port", - in: "1.2.3.4:80", - err: "must be a valid IP address", - }, - { - name: "ipv6 with brackets", - in: "[2001:db8::1]", - err: "must be a valid IP address", - }, - { - name: "[ipv6]:port", - in: "[2001:db8::1]:80", - err: "must be a valid IP address", - }, - { - name: "host:port", - in: "example.com:80", - err: "must be a valid IP address", - }, - { - name: "ipv6 with zone", - in: "1234::abcd%eth0", - err: "must be a valid IP address", - }, - { - name: "ipv4 with zone", - in: "169.254.0.0%eth0", - err: "must be a valid IP address", - }, - } { - t.Run(tc.name, func(t *testing.T) { - errs := IsValidIP(field.NewPath(""), tc.in) - if tc.err == "" { - if len(errs) != 0 { - t.Errorf("expected %q to be valid but got: %v", tc.in, errs) - } - } else { - if len(errs) != 1 { - t.Errorf("expected %q to have 1 error but got: %v", tc.in, errs) - } else if !strings.Contains(errs[0].Detail, tc.err) { - t.Errorf("expected error for %q to contain %q but got: %q", tc.in, tc.err, errs[0].Detail) - } - } - - errs = IsValidIPv4Address(field.NewPath(""), tc.in) - if tc.family == 4 { - if len(errs) != 0 { - t.Errorf("expected %q to pass IsValidIPv4Address but got: %v", tc.in, errs) - } - } else { - if len(errs) == 0 { - t.Errorf("expected %q to fail IsValidIPv4Address", tc.in) - } - } - - errs = IsValidIPv6Address(field.NewPath(""), tc.in) - if tc.family == 6 { - if len(errs) != 0 { - t.Errorf("expected %q to pass IsValidIPv6Address but got: %v", tc.in, errs) - } - } else { - if len(errs) == 0 { - t.Errorf("expected %q to fail IsValidIPv6Address", tc.in) - } - } - }) - } -} - -func TestIsValidCIDR(t *testing.T) { - for _, tc := range []struct { - name string - in string - err string - }{ - // GOOD VALUES - { - name: "ipv4", - in: "1.0.0.0/8", - }, - { - name: "ipv4, all IPs", - in: "0.0.0.0/0", - }, - { - name: "ipv4, single IP", - in: "1.1.1.1/32", - }, - { - name: "ipv6", - in: "2001:4860:4860::/48", - }, - { - name: "ipv6, all IPs", - in: "::/0", - }, - { - name: "ipv6, single IP", - in: "::1/128", - }, - - // GOOD, THOUGH NON-CANONICAL, VALUES - { - name: "ipv6, extra 0s (non-canonical)", - in: "2a00:79e0:2:0::/64", - }, - { - name: "ipv6, capital letters (non-canonical)", - in: "2001:DB8::/64", - }, - - // BAD VALUES WE CURRENTLY CONSIDER GOOD - { - name: "ipv4 with leading 0s", - in: "1.1.01.0/24", - }, - { - name: "ipv4-in-ipv6 with ipv4-sized prefix", - in: "::ffff:1.1.1.0/24", - }, - { - name: "ipv4-in-ipv6 with ipv6-sized prefix", - in: "::ffff:1.1.1.0/120", - }, - { - name: "ipv4 with bits past prefix", - in: "1.2.3.4/24", - }, - { - name: "ipv6 with bits past prefix", - in: "2001:db8::1/64", - }, - { - name: "prefix length with leading 0s", - in: "192.168.0.0/016", - }, - - // BAD VALUES - { - name: "empty string", - in: "", - err: "must be a valid CIDR value", - }, - { - name: "junk", - in: "aaaaaaa", - err: "must be a valid CIDR value", - }, - { - name: "IP address", - in: "1.2.3.4", - err: "must be a valid CIDR value", - }, - { - name: "partial URL", - in: "192.168.0.1/healthz", - err: "must be a valid CIDR value", - }, - { - name: "partial URL 2", - in: "192.168.0.1/0/99", - err: "must be a valid CIDR value", - }, - { - name: "negative prefix length", - in: "192.168.0.0/-16", - err: "must be a valid CIDR value", - }, - { - name: "prefix length with sign", - in: "192.168.0.0/+16", - err: "must be a valid CIDR value", - }, - } { - t.Run(tc.name, func(t *testing.T) { - errs := IsValidCIDR(field.NewPath(""), tc.in) - if tc.err == "" { - if len(errs) != 0 { - t.Errorf("expected %q to be valid but got: %v", tc.in, errs) - } - } else { - if len(errs) != 1 { - t.Errorf("expected %q to have 1 error but got: %v", tc.in, errs) - } else if !strings.Contains(errs[0].Detail, tc.err) { - t.Errorf("expected error for %q to contain %q but got: %q", tc.in, tc.err, errs[0].Detail) - } - } - }) - } -} - func TestIsHTTPHeaderName(t *testing.T) { goodValues := []string{ // Common ones