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/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 f9905f1a4cb..b9041bc8b2c 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/pkg/registry/core/pod/strategy.go @@ -241,7 +241,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 2b3fd197040..f2e0fe972f1 100644 --- a/pkg/registry/core/pod/strategy_test.go +++ b/pkg/registry/core/pod/strategy_test.go @@ -3407,3 +3407,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) + } }