Skip to content

Commit 9d519c7

Browse files
thockinthockin
committed
Fix immutable validation for structs with pointers
Comparing pointers is not the same as deep-equal. This was an embarassing oversight.
1 parent 0b190b8 commit 9d519c7

File tree

10 files changed

+198
-55
lines changed

10 files changed

+198
-55
lines changed

‎staging/src/k8s.io/apimachinery/pkg/api/validate/immutable.go‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,14 @@ import (
2424
"k8s.io/apimachinery/pkg/util/validation/field"
2525
)
2626

27-
// Immutable verifies that the specified value has not changed in the course of
28-
// an update operation. It does nothing if the old value is not provided. If
29-
// the caller needs to compare types that are not trivially comparable, they
30-
// should use ImmutableNonComparable instead.
31-
func Immutable[T comparable](_ context.Context, op operation.Operation, fldPath *field.Path, value, oldValue *T) field.ErrorList {
27+
// ImmutableByCompare verifies that the specified value has not changed in the
28+
// course of an update operation. It does nothing if the old value is not
29+
// provided. If the caller needs to compare types that are not trivially
30+
// comparable, they should use ImmutableByReflect instead.
31+
//
32+
// Caution: structs with pointer fields satisfy comparable, but this function
33+
// will onlyu compare pointer values. It does not do a "deep" comparison.
34+
func ImmutableByCompare[T comparable](_ context.Context, op operation.Operation, fldPath *field.Path, value, oldValue *T) field.ErrorList {
3235
if op.Type != operation.Update {
3336
return nil
3437
}
@@ -43,11 +46,11 @@ func Immutable[T comparable](_ context.Context, op operation.Operation, fldPath
4346
return nil
4447
}
4548

46-
// ImmutableNonComparable verifies that the specified value has not changed in
49+
// ImmutableByReflect verifies that the specified value has not changed in
4750
// the course of an update operation. It does nothing if the old value is not
48-
// provided. Unlike Immutable, this function can be used with types that are
51+
// provided. Unlike ImmutableByCompare, this function can be used with types that are
4952
// not directly comparable, at the cost of performance.
50-
func ImmutableNonComparable[T any](_ context.Context, op operation.Operation, fldPath *field.Path, value, oldValue T) field.ErrorList {
53+
func ImmutableByReflect[T any](_ context.Context, op operation.Operation, fldPath *field.Path, value, oldValue T) field.ErrorList {
5154
if op.Type != operation.Update {
5255
return nil
5356
}

‎staging/src/k8s.io/apimachinery/pkg/api/validate/immutable_test.go‎

Lines changed: 149 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -25,15 +25,16 @@ import (
2525
"k8s.io/utils/ptr"
2626
)
2727

28-
type Struct struct {
28+
type StructComparable struct {
2929
S string
3030
I int
3131
B bool
3232
}
3333

34-
func TestImmutable(t *testing.T) {
35-
structA := Struct{"abc", 123, true}
36-
structB := Struct{"xyz", 456, false}
34+
func TestImmutableByCompare(t *testing.T) {
35+
structA := StructComparable{"abc", 123, true}
36+
structA2 := structA
37+
structB := StructComparable{"xyz", 456, false}
3738

3839
for _, tc := range []struct {
3940
name string
@@ -42,62 +43,194 @@ func TestImmutable(t *testing.T) {
4243
}{{
4344
name: "nil both values",
4445
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
45-
return Immutable[int](context.Background(), op, fld, nil, nil)
46+
return ImmutableByCompare[int](context.Background(), op, fld, nil, nil)
4647
},
4748
}, {
4849
name: "nil value",
4950
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
50-
return Immutable(context.Background(), op, fld, nil, ptr.To(123))
51+
return ImmutableByCompare(context.Background(), op, fld, nil, ptr.To(123))
5152
},
5253
fail: true,
5354
}, {
5455
name: "nil oldValue",
5556
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
56-
return Immutable(context.Background(), op, fld, ptr.To(123), nil)
57+
return ImmutableByCompare(context.Background(), op, fld, ptr.To(123), nil)
5758
},
5859
fail: true,
5960
}, {
6061
name: "int",
6162
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
62-
return Immutable(context.Background(), op, fld, ptr.To(123), ptr.To(123))
63+
return ImmutableByCompare(context.Background(), op, fld, ptr.To(123), ptr.To(123))
6364
},
6465
}, {
6566
name: "int fail",
6667
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
67-
return Immutable(context.Background(), op, fld, ptr.To(123), ptr.To(456))
68+
return ImmutableByCompare(context.Background(), op, fld, ptr.To(123), ptr.To(456))
6869
},
6970
fail: true,
7071
}, {
7172
name: "string",
7273
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
73-
return Immutable(context.Background(), op, fld, ptr.To("abc"), ptr.To("abc"))
74+
return ImmutableByCompare(context.Background(), op, fld, ptr.To("abc"), ptr.To("abc"))
7475
},
7576
}, {
7677
name: "string fail",
7778
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
78-
return Immutable(context.Background(), op, fld, ptr.To("abc"), ptr.To("xyz"))
79+
return ImmutableByCompare(context.Background(), op, fld, ptr.To("abc"), ptr.To("xyz"))
7980
},
8081
fail: true,
8182
}, {
8283
name: "bool",
8384
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
84-
return Immutable(context.Background(), op, fld, ptr.To(true), ptr.To(true))
85+
return ImmutableByCompare(context.Background(), op, fld, ptr.To(true), ptr.To(true))
8586
},
8687
}, {
8788
name: "bool fail",
8889
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
89-
return Immutable(context.Background(), op, fld, ptr.To(true), ptr.To(false))
90+
return ImmutableByCompare(context.Background(), op, fld, ptr.To(true), ptr.To(false))
9091
},
9192
fail: true,
9293
}, {
93-
name: "struct",
94+
name: "same struct",
9495
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
95-
return Immutable(context.Background(), op, fld, ptr.To(structA), ptr.To(structA))
96+
return ImmutableByCompare(context.Background(), op, fld, ptr.To(structA), ptr.To(structA))
97+
},
98+
}, {
99+
name: "equal struct",
100+
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
101+
return ImmutableByCompare(context.Background(), op, fld, ptr.To(structA), ptr.To(structA2))
102+
},
103+
}, {
104+
name: "struct fail",
105+
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
106+
return ImmutableByCompare(context.Background(), op, fld, ptr.To(structA), ptr.To(structB))
107+
},
108+
fail: true,
109+
}} {
110+
t.Run(tc.name, func(t *testing.T) {
111+
errs := tc.fn(operation.Operation{Type: operation.Create}, field.NewPath(""))
112+
if len(errs) != 0 { // Create should always succeed
113+
t.Errorf("case %q (create): expected success: %v", tc.name, errs)
114+
}
115+
errs = tc.fn(operation.Operation{Type: operation.Update}, field.NewPath(""))
116+
if tc.fail && len(errs) == 0 {
117+
t.Errorf("case %q (update): expected failure", tc.name)
118+
} else if !tc.fail && len(errs) != 0 {
119+
t.Errorf("case %q (update): expected success: %v", tc.name, errs)
120+
}
121+
})
122+
}
123+
}
124+
125+
type StructNonComparable struct {
126+
S string
127+
SP *string
128+
I int
129+
IP *int
130+
B bool
131+
BP *bool
132+
SS []string
133+
MSS map[string]string
134+
}
135+
136+
func TestImmutableByReflect(t *testing.T) {
137+
structA := StructNonComparable{
138+
S: "abc",
139+
SP: ptr.To("abc"),
140+
I: 123,
141+
IP: ptr.To(123),
142+
B: true,
143+
BP: ptr.To(true),
144+
SS: []string{"a", "b", "c"},
145+
MSS: map[string]string{"a": "b", "c": "d"},
146+
}
147+
148+
structA2 := structA
149+
structA2.SP = ptr.To("abc")
150+
structA2.IP = ptr.To(123)
151+
structA2.BP = ptr.To(true)
152+
structA2.SS = []string{"a", "b", "c"}
153+
structA2.MSS = map[string]string{"a": "b", "c": "d"}
154+
155+
structB := StructNonComparable{
156+
S: "xyz",
157+
SP: ptr.To("xyz"),
158+
I: 456,
159+
IP: ptr.To(456),
160+
B: false,
161+
BP: ptr.To(false),
162+
SS: []string{"x", "y", "z"},
163+
MSS: map[string]string{"x": "X", "y": "Y"},
164+
}
165+
166+
for _, tc := range []struct {
167+
name string
168+
fn func(operation.Operation, *field.Path) field.ErrorList
169+
fail bool
170+
}{{
171+
name: "nil both values",
172+
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
173+
return ImmutableByReflect[*int](context.Background(), op, fld, nil, nil)
174+
},
175+
}, {
176+
name: "nil value",
177+
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
178+
return ImmutableByReflect(context.Background(), op, fld, nil, ptr.To(123))
179+
},
180+
fail: true,
181+
}, {
182+
name: "nil oldValue",
183+
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
184+
return ImmutableByReflect(context.Background(), op, fld, ptr.To(123), nil)
185+
},
186+
fail: true,
187+
}, {
188+
name: "int",
189+
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
190+
return ImmutableByReflect(context.Background(), op, fld, ptr.To(123), ptr.To(123))
191+
},
192+
}, {
193+
name: "int fail",
194+
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
195+
return ImmutableByReflect(context.Background(), op, fld, ptr.To(123), ptr.To(456))
196+
},
197+
fail: true,
198+
}, {
199+
name: "string",
200+
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
201+
return ImmutableByReflect(context.Background(), op, fld, ptr.To("abc"), ptr.To("abc"))
202+
},
203+
}, {
204+
name: "string fail",
205+
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
206+
return ImmutableByReflect(context.Background(), op, fld, ptr.To("abc"), ptr.To("xyz"))
207+
},
208+
fail: true,
209+
}, {
210+
name: "bool",
211+
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
212+
return ImmutableByReflect(context.Background(), op, fld, ptr.To(true), ptr.To(true))
213+
},
214+
}, {
215+
name: "bool fail",
216+
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
217+
return ImmutableByReflect(context.Background(), op, fld, ptr.To(true), ptr.To(false))
218+
},
219+
fail: true,
220+
}, {
221+
name: "same struct",
222+
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
223+
return ImmutableByReflect(context.Background(), op, fld, ptr.To(structA), ptr.To(structA))
224+
},
225+
}, {
226+
name: "equal struct",
227+
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
228+
return ImmutableByReflect(context.Background(), op, fld, ptr.To(structA), ptr.To(structA2))
96229
},
97230
}, {
98231
name: "struct fail",
99232
fn: func(op operation.Operation, fld *field.Path) field.ErrorList {
100-
return Immutable(context.Background(), op, fld, ptr.To(structA), ptr.To(structB))
233+
return ImmutableByReflect(context.Background(), op, fld, ptr.To(structA), ptr.To(structB))
101234
},
102235
fail: true,
103236
}} {

‎staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/immutable/doc.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,8 @@ type Struct struct {
5757
}
5858

5959
type ComparableStruct struct {
60-
StringField string `json:"stringField"`
60+
StringField string `json:"stringField"`
61+
StringPtrField *string `json:"stringPtrField"`
6162
}
6263

6364
type NonComparableStruct struct {

‎staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/immutable/doc_test.go‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@ func Test(t *testing.T) {
2929
structA := Struct{
3030
StringField: "aaa",
3131
StringPtrField: ptr.To("aaa"),
32-
StructField: ComparableStruct{"bbb"},
33-
StructPtrField: ptr.To(ComparableStruct{"bbb"}),
32+
StructField: ComparableStruct{"bbb", ptr.To("BBB")},
33+
StructPtrField: ptr.To(ComparableStruct{"bbb", ptr.To("BBB")}),
3434
NonComparableStructField: NonComparableStruct{[]string{"ccc"}},
3535
NonComparableStructPtrField: ptr.To(NonComparableStruct{[]string{"ccc"}}),
3636
SliceField: []string{"ddd"},
@@ -41,15 +41,17 @@ func Test(t *testing.T) {
4141

4242
structA2 := structA // dup of A but with different pointer values
4343
structA2.StringPtrField = ptr.To(*structA2.StringPtrField)
44+
structA2.StructField.StringPtrField = ptr.To("BBB")
4445
structA2.StructPtrField = ptr.To(*structA2.StructPtrField)
46+
structA2.StructPtrField.StringPtrField = ptr.To("BBB")
4547
structA2.NonComparableStructPtrField = ptr.To(*structA2.NonComparableStructPtrField)
4648
structA2.ImmutablePtrField = ptr.To(*structA2.ImmutablePtrField)
4749

4850
structB := Struct{
4951
StringField: "uuu",
5052
StringPtrField: ptr.To("uuu"),
51-
StructField: ComparableStruct{"vvv"},
52-
StructPtrField: ptr.To(ComparableStruct{"vvv"}),
53+
StructField: ComparableStruct{"vvv", ptr.To("VVV")},
54+
StructPtrField: ptr.To(ComparableStruct{"vvv", ptr.To("VVV")}),
5355
NonComparableStructField: NonComparableStruct{[]string{"www"}},
5456
NonComparableStructPtrField: ptr.To(NonComparableStruct{[]string{"www"}}),
5557
SliceField: []string{"xxx"},

‎staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/immutable/zz_generated.validations.go‎

Lines changed: 9 additions & 9 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/listmap/multiple_keys/zz_generated.validations.go‎

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)