Skip to content

Commit 79b3632

Browse files
k8s-publishing-botk8s-publishing-bot
committed
Merge pull request #137864 from yongruilin/dv-dra-mismatch
fix(declarative validation): union validation ratcheting when oldObj is nil Kubernetes-commit: 6e845f67faa4e8da1355bfb546495a6f68fed399
2 parents 729062d + a8822f7 commit 79b3632

5 files changed

Lines changed: 196 additions & 44 deletions

File tree

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ require (
2121
gopkg.in/inf.v0 v0.9.1
2222
k8s.io/klog/v2 v2.140.0
2323
k8s.io/kube-openapi v0.0.0-20260317180543-43fb72c5454a
24-
k8s.io/streaming v0.0.0-20260317070603-951b6bf67777
24+
k8s.io/streaming v0.0.0-20260409181516-ff6889be5347
2525
k8s.io/utils v0.0.0-20260210185600-b8788abfbbc2
2626
sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730
2727
sigs.k8s.io/randfill v1.0.0

‎go.sum‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,8 @@ k8s.io/klog/v2 v2.140.0 h1:Tf+J3AH7xnUzZyVVXhTgGhEKnFqye14aadWv7bzXdzc=
9191
k8s.io/klog/v2 v2.140.0/go.mod h1:o+/RWfJ6PwpnFn7OyAG3QnO47BFsymfEfrz6XyYSSp0=
9292
k8s.io/kube-openapi v0.0.0-20260317180543-43fb72c5454a h1:xCeOEAOoGYl2jnJoHkC3hkbPJgdATINPMAxaynU2Ovg=
9393
k8s.io/kube-openapi v0.0.0-20260317180543-43fb72c5454a/go.mod h1:uGBT7iTA6c6MvqUvSXIaYZo9ukscABYi2btjhvgKGZ0=
94-
k8s.io/streaming v0.0.0-20260317070603-951b6bf67777 h1:EwFwvVp6vSJ41TUsv+DJx0UhkLnB8NizEcBJLgK/olU=
95-
k8s.io/streaming v0.0.0-20260317070603-951b6bf67777/go.mod h1:5Zm1U2Duu3uu1nG/ijKuwNWDkQk24aXVpScAhzRkzkk=
94+
k8s.io/streaming v0.0.0-20260409181516-ff6889be5347 h1:kRebECfasGEW70Zc8BT65RZGJoVqsAXf69v4+mcCazo=
95+
k8s.io/streaming v0.0.0-20260409181516-ff6889be5347/go.mod h1:5Zm1U2Duu3uu1nG/ijKuwNWDkQk24aXVpScAhzRkzkk=
9696
k8s.io/utils v0.0.0-20260210185600-b8788abfbbc2 h1:AZYQSJemyQB5eRxqcPky+/7EdBj0xi3g0ZcxxJ7vbWU=
9797
k8s.io/utils v0.0.0-20260210185600-b8788abfbbc2/go.mod h1:xDxuJ0whA3d0I4mf/C4ppKHxXynQ+fxnkmQH0vTHnuk=
9898
sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 h1:IpInykpT6ceI+QxKBbEflcR5EXP7sU1kvOlxwZh5txg=

‎pkg/api/validate/union.go‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package validate
1919
import (
2020
"context"
2121
"fmt"
22+
"reflect"
2223
"strings"
2324

2425
"k8s.io/apimachinery/pkg/api/operation"
@@ -60,6 +61,10 @@ type UnionValidationOptions struct {
6061
// )...)
6162
// return errs
6263
// }
64+
//
65+
// Note that T is "any", rather than "comparable", because union-members can be
66+
// slices, meaning T might be a struct with a slice, meaning it is not
67+
// comparable.
6368
func Union[T any](_ context.Context, op operation.Operation, fldPath *field.Path, obj, oldObj T, union *UnionMembership, isSetFns ...ExtractorFn[T, bool]) field.ErrorList {
6469
options := UnionValidationOptions{
6570
ErrorForEmpty: func(fldPath *field.Path, allFields []string) *field.Error {
@@ -98,6 +103,10 @@ func Union[T any](_ context.Context, op operation.Operation, fldPath *field.Path
98103
//
99104
// It is not an error for the discriminatorValue to be unknown. That must be
100105
// validated on its own.
106+
//
107+
// Note that T is "any", rather than "comparable", because union-members can be
108+
// slices, meaning T might be a struct with a slice, meaning it is not
109+
// comparable.
101110
func DiscriminatedUnion[T any, D ~string](_ context.Context, op operation.Operation, fldPath *field.Path, obj, oldObj T, union *UnionMembership, discriminatorExtractor ExtractorFn[T, D], isSetFns ...ExtractorFn[T, bool]) (errs field.ErrorList) {
102111
if len(union.members) != len(isSetFns) {
103112
return field.ErrorList{
@@ -106,6 +115,7 @@ func DiscriminatedUnion[T any, D ~string](_ context.Context, op operation.Operat
106115
len(isSetFns), len(union.members))),
107116
}
108117
}
118+
hasOldValue := !reflect.ValueOf(oldObj).IsZero() // because T is any, rather than comparable
109119
var changed bool
110120
discriminatorValue := discriminatorExtractor(obj)
111121
if op.Type == operation.Update {
@@ -131,7 +141,7 @@ func DiscriminatedUnion[T any, D ~string](_ context.Context, op operation.Operat
131141
}
132142
// If the union discriminator and membership is unchanged, we don't need to
133143
// re-validate.
134-
if op.Type == operation.Update && !changed {
144+
if op.Type == operation.Update && hasOldValue && !changed {
135145
return nil
136146
}
137147
return errs.WithOrigin("union")
@@ -195,6 +205,7 @@ func unionValidate[T any](op operation.Operation, fldPath *field.Path,
195205
}
196206
}
197207

208+
hasOldValue := !reflect.ValueOf(oldObj).IsZero() // because T is any, rather than comparable
198209
var specifiedFields []string
199210
var changed bool
200211
for i, fieldIsSet := range isSetFns {
@@ -209,7 +220,7 @@ func unionValidate[T any](op operation.Operation, fldPath *field.Path,
209220
}
210221

211222
// If the union membership is unchanged, we don't need to re-validate.
212-
if op.Type == operation.Update && !changed {
223+
if op.Type == operation.Update && hasOldValue && !changed {
213224
return nil
214225
}
215226

‎pkg/api/validate/union_test.go‎

Lines changed: 178 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -67,18 +67,34 @@ func TestUnion(t *testing.T) {
6767
members = append(members, NewUnionMember(f))
6868
}
6969

70-
// Create mock extractors that return predefined values instead of
71-
// actually extracting from the object.
72-
extractors := make([]ExtractorFn[*testMember, bool], len(tc.fieldValues))
73-
for i, val := range tc.fieldValues {
74-
extractors[i] = func(_ *testMember) bool { return val }
75-
}
70+
t.Run("pointer", func(t *testing.T) {
71+
// Create mock extractors that return predefined values instead of
72+
// actually extracting from the object.
73+
extractors := make([]ExtractorFn[*testMember, bool], len(tc.fieldValues))
74+
for i, val := range tc.fieldValues {
75+
extractors[i] = func(_ *testMember) bool { return val }
76+
}
7677

77-
got := Union(context.Background(), operation.Operation{}, nil, &testMember{}, nil,
78-
NewUnionMembership(members...), extractors...)
79-
if !reflect.DeepEqual(got, tc.expected) {
80-
t.Errorf("got %v want %v", got, tc.expected)
81-
}
78+
got := Union(context.Background(), operation.Operation{}, nil, &testMember{}, nil,
79+
NewUnionMembership(members...), extractors...)
80+
if !reflect.DeepEqual(got, tc.expected) {
81+
t.Errorf("got %v want %v", got, tc.expected)
82+
}
83+
})
84+
t.Run("value", func(t *testing.T) {
85+
// Create mock extractors that return predefined values instead of
86+
// actually extracting from the object.
87+
extractors := make([]ExtractorFn[testMember, bool], len(tc.fieldValues))
88+
for i, val := range tc.fieldValues {
89+
extractors[i] = func(_ testMember) bool { return val }
90+
}
91+
92+
got := Union(context.Background(), operation.Operation{}, nil, testMember{}, testMember{},
93+
NewUnionMembership(members...), extractors...)
94+
if !reflect.DeepEqual(got, tc.expected) {
95+
t.Errorf("got %v want %v", got, tc.expected)
96+
}
97+
})
8298
})
8399
}
84100
}
@@ -131,33 +147,53 @@ func TestDiscriminatedUnion(t *testing.T) {
131147
}
132148

133149
for _, tc := range testCases {
150+
members := []UnionMember{}
151+
for _, f := range tc.fields {
152+
members = append(members, NewDiscriminatedUnionMember(f[0], f[1]))
153+
}
154+
134155
t.Run(tc.name, func(t *testing.T) {
135-
members := []UnionMember{}
136-
for _, f := range tc.fields {
137-
members = append(members, NewDiscriminatedUnionMember(f[0], f[1]))
138-
}
156+
t.Run("pointer", func(t *testing.T) {
157+
discriminatorExtractor := func(_ *testMember) string { return tc.discriminatorValue }
139158

140-
discriminatorExtractor := func(_ *testMember) string { return tc.discriminatorValue }
159+
// Create mock extractors that return predefined values instead of
160+
// actually extracting from the object.
161+
extractors := make([]ExtractorFn[*testMember, bool], len(tc.fieldValues))
162+
for i, val := range tc.fieldValues {
163+
extractors[i] = func(_ *testMember) bool { return val }
164+
}
141165

142-
// Create mock extractors that return predefined values instead of
143-
// actually extracting from the object.
144-
extractors := make([]ExtractorFn[*testMember, bool], len(tc.fieldValues))
145-
for i, val := range tc.fieldValues {
146-
extractors[i] = func(_ *testMember) bool { return val }
147-
}
166+
got := DiscriminatedUnion(context.Background(), operation.Operation{}, nil, &testMember{}, nil,
167+
NewDiscriminatedUnionMembership(tc.discriminatorField, members...), discriminatorExtractor, extractors...)
168+
if !reflect.DeepEqual(got, tc.expected) {
169+
t.Errorf("got %v want %v", got.ToAggregate(), tc.expected.ToAggregate())
170+
}
171+
})
172+
t.Run("value", func(t *testing.T) {
173+
discriminatorExtractor := func(_ testMember) string { return tc.discriminatorValue }
148174

149-
got := DiscriminatedUnion(context.Background(), operation.Operation{}, nil, &testMember{}, nil,
150-
NewDiscriminatedUnionMembership(tc.discriminatorField, members...), discriminatorExtractor, extractors...)
151-
if !reflect.DeepEqual(got, tc.expected) {
152-
t.Errorf("got %v want %v", got.ToAggregate(), tc.expected.ToAggregate())
153-
}
175+
// Create mock extractors that return predefined values instead of
176+
// actually extracting from the object.
177+
extractors := make([]ExtractorFn[testMember, bool], len(tc.fieldValues))
178+
for i, val := range tc.fieldValues {
179+
extractors[i] = func(_ testMember) bool { return val }
180+
}
181+
182+
got := DiscriminatedUnion(context.Background(), operation.Operation{}, nil, testMember{}, testMember{},
183+
NewDiscriminatedUnionMembership(tc.discriminatorField, members...), discriminatorExtractor, extractors...)
184+
if !reflect.DeepEqual(got, tc.expected) {
185+
t.Errorf("got %v want %v", got.ToAggregate(), tc.expected.ToAggregate())
186+
}
187+
})
154188
})
155189
}
156190
}
157191

158192
type testStruct struct {
159-
M1 *m1 `json:"m1"`
160-
M2 *m2 `json:"m2"`
193+
M1 *m1 `json:"m1"`
194+
M2 *m2 `json:"m2"`
195+
M3 []string `json:"m3"`
196+
M4 map[string]string `json:"m4"`
161197
}
162198

163199
type m1 struct{}
@@ -176,6 +212,18 @@ var extractors = []ExtractorFn[*testStruct, bool]{
176212
}
177213
return s.M2 != nil
178214
},
215+
func(s *testStruct) bool {
216+
if s == nil {
217+
return false
218+
}
219+
return len(s.M3) != 0
220+
},
221+
func(s *testStruct) bool {
222+
if s == nil {
223+
return false
224+
}
225+
return len(s.M4) != 0
226+
},
179227
}
180228

181229
func TestUnionRatcheting(t *testing.T) {
@@ -186,9 +234,12 @@ func TestUnionRatcheting(t *testing.T) {
186234
expected field.ErrorList
187235
}{
188236
{
189-
name: "both nil",
237+
name: "old nil - no ratcheting",
190238
oldStruct: nil,
191-
newStruct: nil,
239+
newStruct: &testStruct{},
240+
expected: field.ErrorList{
241+
field.Invalid(nil, "", "must specify one of: `m1`, `m2`, `m3`, `m4`"),
242+
}.WithOrigin("union"),
192243
},
193244
{
194245
name: "both empty struct",
@@ -216,14 +267,40 @@ func TestUnionRatcheting(t *testing.T) {
216267
M2: &m2{},
217268
},
218269
expected: field.ErrorList{
219-
field.Invalid(nil, "{m1, m2}", "must specify exactly one of: `m1`, `m2`"),
270+
field.Invalid(nil, "{m1, m2}", "must specify exactly one of: `m1`, `m2`, `m3`, `m4`"),
271+
}.WithOrigin("union"),
272+
},
273+
{
274+
name: "slice member ratcheting: unchanged membership",
275+
oldStruct: &testStruct{M3: []string{"a"}},
276+
newStruct: &testStruct{M3: []string{"b"}},
277+
},
278+
{
279+
name: "map member ratcheting: unchanged membership",
280+
oldStruct: &testStruct{M4: map[string]string{"k": "v1"}},
281+
newStruct: &testStruct{M4: map[string]string{"k": "v2"}},
282+
},
283+
{
284+
name: "empty slice is not set",
285+
oldStruct: nil,
286+
newStruct: &testStruct{M3: []string{}},
287+
expected: field.ErrorList{
288+
field.Invalid(nil, "", "must specify one of: `m1`, `m2`, `m3`, `m4`"),
289+
}.WithOrigin("union"),
290+
},
291+
{
292+
name: "empty map is not set",
293+
oldStruct: nil,
294+
newStruct: &testStruct{M4: map[string]string{}},
295+
expected: field.ErrorList{
296+
field.Invalid(nil, "", "must specify one of: `m1`, `m2`, `m3`, `m4`"),
220297
}.WithOrigin("union"),
221298
},
222299
}
223300

224301
for _, tc := range testCases {
225302
t.Run(tc.name, func(t *testing.T) {
226-
members := []UnionMember{NewUnionMember("m1"), NewUnionMember("m2")}
303+
members := []UnionMember{NewUnionMember("m1"), NewUnionMember("m2"), NewUnionMember("m3"), NewUnionMember("m4")}
227304
got := Union(context.Background(), operation.Operation{Type: operation.Update}, nil, tc.newStruct, tc.oldStruct,
228305
NewUnionMembership(members...), extractors...)
229306
if !reflect.DeepEqual(got, tc.expected) {
@@ -234,9 +311,11 @@ func TestUnionRatcheting(t *testing.T) {
234311
}
235312

236313
type testDiscriminatedStruct struct {
237-
D string `json:"d"`
238-
M1 *m1 `json:"m1"`
239-
M2 *m2 `json:"m2"`
314+
D string `json:"d"`
315+
M1 *m1 `json:"m1"`
316+
M2 *m2 `json:"m2"`
317+
M3 []string `json:"m3"`
318+
M4 map[string]string `json:"m4"`
240319
}
241320

242321
var testDiscriminatorExtractor = func(s *testDiscriminatedStruct) string {
@@ -258,6 +337,18 @@ var testDiscriminatedExtractors = []ExtractorFn[*testDiscriminatedStruct, bool]{
258337
}
259338
return s.M2 != nil
260339
},
340+
func(s *testDiscriminatedStruct) bool {
341+
if s == nil {
342+
return false
343+
}
344+
return len(s.M3) != 0
345+
},
346+
func(s *testDiscriminatedStruct) bool {
347+
if s == nil {
348+
return false
349+
}
350+
return len(s.M4) != 0
351+
},
261352
}
262353

263354
func TestDiscriminatedUnionRatcheting(t *testing.T) {
@@ -329,11 +420,61 @@ func TestDiscriminatedUnionRatcheting(t *testing.T) {
329420
field.Invalid(field.NewPath("m2"), "", "must be specified when `d` is \"m2\""),
330421
}.WithOrigin("union"),
331422
},
423+
{
424+
name: "slice member ratcheting: unchanged membership",
425+
oldStruct: &testDiscriminatedStruct{
426+
D: "m3",
427+
M3: []string{"a"},
428+
},
429+
newStruct: &testDiscriminatedStruct{
430+
D: "m3",
431+
M3: []string{"b"},
432+
},
433+
},
434+
{
435+
name: "map member ratcheting: unchanged membership",
436+
oldStruct: &testDiscriminatedStruct{
437+
D: "m4",
438+
M4: map[string]string{"k": "v1"},
439+
},
440+
newStruct: &testDiscriminatedStruct{
441+
D: "m4",
442+
M4: map[string]string{"k": "v2"},
443+
},
444+
},
445+
{
446+
name: "empty slice is not set",
447+
oldStruct: &testDiscriminatedStruct{
448+
D: "m3",
449+
M3: []string{"a"},
450+
},
451+
newStruct: &testDiscriminatedStruct{
452+
D: "m3",
453+
M3: []string{},
454+
},
455+
expected: field.ErrorList{
456+
field.Invalid(field.NewPath("m3"), "", "must be specified when `d` is \"m3\""),
457+
}.WithOrigin("union"),
458+
},
459+
{
460+
name: "empty map is not set",
461+
oldStruct: &testDiscriminatedStruct{
462+
D: "m4",
463+
M4: map[string]string{"k": "v"},
464+
},
465+
newStruct: &testDiscriminatedStruct{
466+
D: "m4",
467+
M4: map[string]string{},
468+
},
469+
expected: field.ErrorList{
470+
field.Invalid(field.NewPath("m4"), "", "must be specified when `d` is \"m4\""),
471+
}.WithOrigin("union"),
472+
},
332473
}
333474

334475
for _, tc := range testCases {
335476
t.Run(tc.name, func(t *testing.T) {
336-
members := []UnionMember{NewDiscriminatedUnionMember("m1", "m1"), NewDiscriminatedUnionMember("m2", "m2")}
477+
members := []UnionMember{NewDiscriminatedUnionMember("m1", "m1"), NewDiscriminatedUnionMember("m2", "m2"), NewDiscriminatedUnionMember("m3", "m3"), NewDiscriminatedUnionMember("m4", "m4")}
337478
got := DiscriminatedUnion(context.Background(), operation.Operation{Type: operation.Update}, nil, tc.newStruct, tc.oldStruct,
338479
NewDiscriminatedUnionMembership("d", members...), testDiscriminatorExtractor, testDiscriminatedExtractors...)
339480
if !reflect.DeepEqual(got, tc.expected) {

‎pkg/api/validate/zeroorone_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ func TestZeroOrOneOfUnionRatcheting(t *testing.T) {
119119
M2: &m2{},
120120
},
121121
expected: field.ErrorList{
122-
field.Invalid(nil, "{m1, m2}", "must specify at most one of: `m1`, `m2`").WithOrigin("zeroOrOneOf"),
122+
field.Invalid(nil, "{m1, m2}", "must specify at most one of: `m1`, `m2`, `m3`, `m4`").WithOrigin("zeroOrOneOf"),
123123
},
124124
},
125125
{
@@ -142,7 +142,7 @@ func TestZeroOrOneOfUnionRatcheting(t *testing.T) {
142142

143143
for _, tc := range testCases {
144144
t.Run(tc.name, func(t *testing.T) {
145-
members := []UnionMember{NewUnionMember("m1"), NewUnionMember("m2")}
145+
members := []UnionMember{NewUnionMember("m1"), NewUnionMember("m2"), NewUnionMember("m3"), NewUnionMember("m4")}
146146
got := ZeroOrOneOfUnion(context.Background(), operation.Operation{Type: operation.Update}, nil, tc.newStruct, tc.oldStruct,
147147
NewUnionMembership(members...), extractors...)
148148
if !reflect.DeepEqual(got, tc.expected) {

0 commit comments

Comments
 (0)
⚡