Skip to content

Commit 7316d83

Browse files
committed
Add warnings to all IP/CIDR-valued fields
1 parent d4c55d0 commit 7316d83

File tree

16 files changed

+561
-25
lines changed

16 files changed

+561
-25
lines changed

pkg/api/pod/warnings.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424

2525
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2626
"k8s.io/apimachinery/pkg/util/sets"
27+
"k8s.io/apimachinery/pkg/util/validation"
2728
"k8s.io/apimachinery/pkg/util/validation/field"
2829
nodeapi "k8s.io/kubernetes/pkg/api/node"
2930
pvcutil "k8s.io/kubernetes/pkg/api/persistentvolumeclaim"
@@ -351,6 +352,16 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta
351352
}
352353
}
353354

355+
// Deprecated IP address formats
356+
if podSpec.DNSConfig != nil {
357+
for i, ns := range podSpec.DNSConfig.Nameservers {
358+
warnings = append(warnings, validation.GetWarningsForIP(fieldPath.Child("spec", "dnsConfig", "nameservers").Index(i), ns)...)
359+
}
360+
}
361+
for i, hostAlias := range podSpec.HostAliases {
362+
warnings = append(warnings, validation.GetWarningsForIP(fieldPath.Child("spec", "hostAliases").Index(i).Child("ip"), hostAlias.IP)...)
363+
}
364+
354365
return warnings
355366
}
356367

pkg/api/pod/warnings_test.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1767,6 +1767,21 @@ func TestWarnings(t *testing.T) {
17671767
`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])?')`,
17681768
},
17691769
},
1770+
{
1771+
name: "dubious IP address formats",
1772+
template: &api.PodTemplateSpec{Spec: api.PodSpec{
1773+
DNSConfig: &api.PodDNSConfig{
1774+
Nameservers: []string{"1.2.3.4", "05.06.07.08"},
1775+
},
1776+
HostAliases: []api.HostAlias{
1777+
{IP: "::ffff:1.2.3.4"},
1778+
},
1779+
}},
1780+
expected: []string{
1781+
`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"`,
1782+
`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"`,
1783+
},
1784+
},
17701785
}
17711786

17721787
for _, tc := range testcases {

pkg/registry/core/endpoint/strategy.go

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,13 @@ import (
2020
"context"
2121

2222
"k8s.io/apimachinery/pkg/runtime"
23+
utilvalidation "k8s.io/apimachinery/pkg/util/validation"
2324
"k8s.io/apimachinery/pkg/util/validation/field"
2425
"k8s.io/apiserver/pkg/storage/names"
2526
"k8s.io/kubernetes/pkg/api/legacyscheme"
2627
api "k8s.io/kubernetes/pkg/apis/core"
2728
"k8s.io/kubernetes/pkg/apis/core/validation"
29+
endpointscontroller "k8s.io/kubernetes/pkg/controller/endpoint"
2830
)
2931

3032
// endpointsStrategy implements behavior for Endpoints
@@ -57,7 +59,7 @@ func (endpointsStrategy) Validate(ctx context.Context, obj runtime.Object) field
5759

5860
// WarningsOnCreate returns warnings for the creation of the given object.
5961
func (endpointsStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string {
60-
return nil
62+
return endpointsWarnings(obj.(*api.Endpoints))
6163
}
6264

6365
// Canonicalize normalizes the object after validation.
@@ -76,9 +78,33 @@ func (endpointsStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Ob
7678

7779
// WarningsOnUpdate returns warnings for the given update.
7880
func (endpointsStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string {
79-
return nil
81+
return endpointsWarnings(obj.(*api.Endpoints))
8082
}
8183

8284
func (endpointsStrategy) AllowUnconditionalUpdate() bool {
8385
return true
8486
}
87+
88+
func endpointsWarnings(endpoints *api.Endpoints) []string {
89+
// Save time by not checking for bad IPs if the request is coming from the
90+
// Endpoints controller, since we know it fixes up any invalid IPs from its input
91+
// data when outputting the Endpoints. (The "managed-by" label is new, so this
92+
// heuristic may fail in skewed clusters, but that just means we won't get the
93+
// optimization during the skew.)
94+
if endpoints.Labels[endpointscontroller.LabelManagedBy] == endpointscontroller.ControllerName {
95+
return nil
96+
}
97+
98+
var warnings []string
99+
for i := range endpoints.Subsets {
100+
for j := range endpoints.Subsets[i].Addresses {
101+
fldPath := field.NewPath("subsets").Index(i).Child("addresses").Index(j).Child("ip")
102+
warnings = append(warnings, utilvalidation.GetWarningsForIP(fldPath, endpoints.Subsets[i].Addresses[j].IP)...)
103+
}
104+
for j := range endpoints.Subsets[i].NotReadyAddresses {
105+
fldPath := field.NewPath("subsets").Index(i).Child("notReadyAddresses").Index(j).Child("ip")
106+
warnings = append(warnings, utilvalidation.GetWarningsForIP(fldPath, endpoints.Subsets[i].NotReadyAddresses[j].IP)...)
107+
}
108+
}
109+
return warnings
110+
}
Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
/*
2+
Copyright 2024 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package endpoint
18+
19+
import (
20+
"strings"
21+
"testing"
22+
23+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
24+
api "k8s.io/kubernetes/pkg/apis/core"
25+
)
26+
27+
func Test_endpointsWarning(t *testing.T) {
28+
tests := []struct {
29+
name string
30+
endpoints *api.Endpoints
31+
warnings []string
32+
}{
33+
{
34+
name: "empty Endpoints",
35+
endpoints: &api.Endpoints{},
36+
warnings: nil,
37+
},
38+
{
39+
name: "valid Endpoints",
40+
endpoints: &api.Endpoints{
41+
Subsets: []api.EndpointSubset{
42+
{
43+
Addresses: []api.EndpointAddress{
44+
{IP: "1.2.3.4"},
45+
{IP: "fd00::1234"},
46+
},
47+
},
48+
},
49+
},
50+
warnings: nil,
51+
},
52+
{
53+
name: "bad Endpoints",
54+
endpoints: &api.Endpoints{
55+
Subsets: []api.EndpointSubset{
56+
{
57+
Addresses: []api.EndpointAddress{
58+
{IP: "fd00::1234"},
59+
{IP: "01.02.03.04"},
60+
},
61+
},
62+
{
63+
Addresses: []api.EndpointAddress{
64+
{IP: "::ffff:1.2.3.4"},
65+
},
66+
},
67+
{
68+
Addresses: []api.EndpointAddress{
69+
{IP: "1.2.3.4"},
70+
},
71+
NotReadyAddresses: []api.EndpointAddress{
72+
{IP: "::ffff:1.2.3.4"},
73+
},
74+
},
75+
},
76+
},
77+
warnings: []string{
78+
"subsets[0].addresses[1].ip",
79+
"subsets[1].addresses[0].ip",
80+
"subsets[2].notReadyAddresses[0].ip",
81+
},
82+
},
83+
{
84+
// We don't actually want to let bad IPs through in this case; the
85+
// point here is that we trust the Endpoints controller to not do
86+
// that, and we're testing that the checks correctly get skipped.
87+
name: "bad Endpoints ignored because of label",
88+
endpoints: &api.Endpoints{
89+
ObjectMeta: metav1.ObjectMeta{
90+
Labels: map[string]string{
91+
"endpoints.kubernetes.io/managed-by": "endpoint-controller",
92+
},
93+
},
94+
Subsets: []api.EndpointSubset{
95+
{
96+
Addresses: []api.EndpointAddress{
97+
{IP: "fd00::1234"},
98+
{IP: "01.02.03.04"},
99+
},
100+
},
101+
},
102+
},
103+
warnings: nil,
104+
},
105+
}
106+
107+
for _, test := range tests {
108+
t.Run(test.name, func(t *testing.T) {
109+
warnings := endpointsWarnings(test.endpoints)
110+
ok := len(warnings) == len(test.warnings)
111+
if ok {
112+
for i := range warnings {
113+
if !strings.HasPrefix(warnings[i], test.warnings[i]) {
114+
ok = false
115+
break
116+
}
117+
}
118+
}
119+
if !ok {
120+
t.Errorf("Expected warnings for %v, got %v", test.warnings, warnings)
121+
}
122+
})
123+
}
124+
}

pkg/registry/core/node/strategy.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import (
3030
"k8s.io/apimachinery/pkg/runtime"
3131
"k8s.io/apimachinery/pkg/types"
3232
utilnet "k8s.io/apimachinery/pkg/util/net"
33+
utilvalidation "k8s.io/apimachinery/pkg/util/validation"
3334
"k8s.io/apimachinery/pkg/util/validation/field"
3435
"k8s.io/apiserver/pkg/registry/generic"
3536
pkgstorage "k8s.io/apiserver/pkg/storage"
@@ -131,7 +132,7 @@ func (nodeStrategy) Validate(ctx context.Context, obj runtime.Object) field.Erro
131132

132133
// WarningsOnCreate returns warnings for the creation of the given object.
133134
func (nodeStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string {
134-
return fieldIsDeprecatedWarnings(obj)
135+
return nodeWarnings(obj)
135136
}
136137

137138
// Canonicalize normalizes the object after validation.
@@ -146,7 +147,7 @@ func (nodeStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object)
146147

147148
// WarningsOnUpdate returns warnings for the given update.
148149
func (nodeStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string {
149-
return fieldIsDeprecatedWarnings(obj)
150+
return nodeWarnings(obj)
150151
}
151152

152153
func (nodeStrategy) AllowUnconditionalUpdate() bool {
@@ -287,7 +288,7 @@ func isProxyableHostname(ctx context.Context, hostname string) error {
287288
return nil
288289
}
289290

290-
func fieldIsDeprecatedWarnings(obj runtime.Object) []string {
291+
func nodeWarnings(obj runtime.Object) []string {
291292
newNode := obj.(*api.Node)
292293
var warnings []string
293294
if newNode.Spec.ConfigSource != nil {
@@ -297,6 +298,14 @@ func fieldIsDeprecatedWarnings(obj runtime.Object) []string {
297298
if len(newNode.Spec.DoNotUseExternalID) > 0 {
298299
warnings = append(warnings, "spec.externalID: this field is deprecated, and is unused by Kubernetes")
299300
}
301+
302+
if len(newNode.Spec.PodCIDRs) > 0 {
303+
podCIDRsField := field.NewPath("spec", "podCIDRs")
304+
for i, value := range newNode.Spec.PodCIDRs {
305+
warnings = append(warnings, utilvalidation.GetWarningsForCIDR(podCIDRsField.Index(i), value)...)
306+
}
307+
}
308+
300309
return warnings
301310
}
302311

pkg/registry/core/node/strategy_test.go

Lines changed: 54 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -287,25 +287,65 @@ func TestWarningOnUpdateAndCreate(t *testing.T) {
287287
node api.Node
288288
warningText string
289289
}{
290-
{api.Node{},
290+
{
291+
api.Node{},
292+
api.Node{},
293+
"",
294+
},
295+
{
291296
api.Node{},
292-
""},
293-
{api.Node{},
297+
//nolint:staticcheck // ignore deprecation warning
294298
api.Node{Spec: api.NodeSpec{ConfigSource: &api.NodeConfigSource{}}},
295-
"spec.configSource"},
296-
{api.Node{Spec: api.NodeSpec{ConfigSource: &api.NodeConfigSource{}}},
299+
"spec.configSource",
300+
},
301+
{
302+
//nolint:staticcheck // ignore deprecation warning
297303
api.Node{Spec: api.NodeSpec{ConfigSource: &api.NodeConfigSource{}}},
298-
"spec.configSource"},
299-
{api.Node{Spec: api.NodeSpec{ConfigSource: &api.NodeConfigSource{}}},
300-
api.Node{}, ""},
301-
{api.Node{},
304+
//nolint:staticcheck // ignore deprecation warning
305+
api.Node{Spec: api.NodeSpec{ConfigSource: &api.NodeConfigSource{}}},
306+
"spec.configSource",
307+
},
308+
{
309+
//nolint:staticcheck // ignore deprecation warning
310+
api.Node{Spec: api.NodeSpec{ConfigSource: &api.NodeConfigSource{}}},
311+
api.Node{},
312+
"",
313+
},
314+
{
315+
api.Node{},
316+
api.Node{Spec: api.NodeSpec{DoNotUseExternalID: "externalID"}},
317+
"spec.externalID",
318+
},
319+
{
302320
api.Node{Spec: api.NodeSpec{DoNotUseExternalID: "externalID"}},
303-
"spec.externalID"},
304-
{api.Node{Spec: api.NodeSpec{DoNotUseExternalID: "externalID"}},
305321
api.Node{Spec: api.NodeSpec{DoNotUseExternalID: "externalID"}},
306-
"spec.externalID"},
307-
{api.Node{Spec: api.NodeSpec{DoNotUseExternalID: "externalID"}},
308-
api.Node{}, ""},
322+
"spec.externalID",
323+
},
324+
{
325+
api.Node{Spec: api.NodeSpec{DoNotUseExternalID: "externalID"}},
326+
api.Node{},
327+
"",
328+
},
329+
{
330+
api.Node{},
331+
api.Node{Spec: api.NodeSpec{PodCIDRs: []string{"10.0.1.0/24", "fd00::/64"}}},
332+
"",
333+
},
334+
{
335+
api.Node{},
336+
api.Node{Spec: api.NodeSpec{PodCIDRs: []string{"010.000.001.000/24", "fd00::/64"}}},
337+
"spec.podCIDRs[0]",
338+
},
339+
{
340+
api.Node{},
341+
api.Node{Spec: api.NodeSpec{PodCIDRs: []string{"10.0.1.0/24", "fd00::1234/64"}}},
342+
"spec.podCIDRs[1]",
343+
},
344+
{
345+
api.Node{Spec: api.NodeSpec{PodCIDRs: []string{"010.000.001.000/24", "fd00::1234/64"}}},
346+
api.Node{Spec: api.NodeSpec{PodCIDRs: []string{"10.0.1.0/24", "fd00::/64"}}},
347+
"",
348+
},
309349
}
310350
for i, test := range tests {
311351
warnings := (nodeStrategy{}).WarningsOnUpdate(context.Background(), &test.node, &test.oldNode)

pkg/registry/core/pod/strategy.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,17 @@ func (podStatusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Ob
241241

242242
// WarningsOnUpdate returns warnings for the given update.
243243
func (podStatusStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string {
244-
return nil
244+
pod := obj.(*api.Pod)
245+
var warnings []string
246+
247+
for i, podIP := range pod.Status.PodIPs {
248+
warnings = append(warnings, utilvalidation.GetWarningsForIP(field.NewPath("status", "podIPs").Index(i).Child("ip"), podIP.IP)...)
249+
}
250+
for i, hostIP := range pod.Status.HostIPs {
251+
warnings = append(warnings, utilvalidation.GetWarningsForIP(field.NewPath("status", "hostIPs").Index(i).Child("ip"), hostIP.IP)...)
252+
}
253+
254+
return warnings
245255
}
246256

247257
type podEphemeralContainersStrategy struct {

0 commit comments

Comments
 (0)