Skip to content

Commit d4f8522

Browse files
aaron-prindleaaron-prindle
committed
address jpbetz review comments - round 1
1 parent 5e75cbb commit d4f8522

File tree

2 files changed

+106
-56
lines changed

2 files changed

+106
-56
lines changed

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

Lines changed: 36 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,9 @@ import (
2929
// course of an update operation. It does nothing if the old value is not
3030
// provided. If the caller needs to compare types that are not trivially
3131
// comparable, they should use FrozenByReflect instead.
32-
//
32+
// Semantics:
33+
// - Forbids ALL transitions after creation
34+
// - This includes: set->unset, unset->set, and modify operations
3335
// Caution: structs with pointer fields satisfy comparable, but this function
3436
// will only compare pointer values. It does not compare the pointed-to
3537
// values.
@@ -52,6 +54,9 @@ func FrozenByCompare[T comparable](_ context.Context, op operation.Operation, fl
5254
// the course of an update operation. It does nothing if the old value is not
5355
// provided. Unlike ImmutableByCompare, this function can be used with types that are
5456
// not directly comparable, at the cost of performance.
57+
// Semantics:
58+
// - Forbids ALL transitions after creation
59+
// - This includes: set->unset (set), unset->set (clear), and modify
5560
func FrozenByReflect[T any](_ context.Context, op operation.Operation, fldPath *field.Path, value, oldValue T) field.ErrorList {
5661
if op.Type != operation.Update {
5762
return nil
@@ -73,13 +78,7 @@ func FrozenByReflect[T any](_ context.Context, op operation.Operation, fldPath *
7378
// This function is optimized for comparable types.
7479
// For non-comparable types use ImmutableByReflect instead.
7580
func ImmutableValueByCompare[T comparable](ctx context.Context, op operation.Operation, fldPath *field.Path, value, oldValue *T) field.ErrorList {
76-
return immutableCheck(op, fldPath, value, oldValue, func(v *T) bool {
77-
if v == nil {
78-
return true
79-
}
80-
var zero T
81-
return *v == zero
82-
})
81+
return immutableByCompareCheck(op, fldPath, value, oldValue, isUnsetComparable[T])
8382
}
8483

8584
// ImmutablePointerByCompare allows a field to be set
@@ -92,7 +91,7 @@ func ImmutableValueByCompare[T comparable](ctx context.Context, op operation.Ope
9291
// This function is optimized for comparable types.
9392
// For non-comparable types, use ImmutableByReflect instead.
9493
func ImmutablePointerByCompare[T comparable](ctx context.Context, op operation.Operation, fldPath *field.Path, value, oldValue *T) field.ErrorList {
95-
return immutableCheck(op, fldPath, value, oldValue, func(v *T) bool {
94+
return immutableByCompareCheck(op, fldPath, value, oldValue, func(v *T) bool {
9695
return v == nil
9796
})
9897
}
@@ -110,38 +109,22 @@ func ImmutableByReflect[T any](_ context.Context, op operation.Operation, fldPat
110109
if op.Type != operation.Update {
111110
return nil
112111
}
113-
114-
valueIsUnset := isUnsetForImmutable(value)
115-
oldValueIsUnset := isUnsetForImmutable(oldValue)
116-
117-
if oldValueIsUnset && valueIsUnset {
112+
if equality.Semantic.DeepEqual(value, oldValue) {
118113
return nil
119114
}
120-
if !oldValueIsUnset && !valueIsUnset && equality.Semantic.DeepEqual(value, oldValue) {
115+
oldValueIsUnset := isUnsetForReflect(oldValue)
116+
valueIsUnset := isUnsetForReflect(value)
117+
if oldValueIsUnset && !valueIsUnset {
121118
return nil
122119
}
123-
124-
switch {
125-
case oldValueIsUnset && !valueIsUnset:
126-
return nil
127-
case !oldValueIsUnset && valueIsUnset:
128-
return field.ErrorList{
129-
field.Forbidden(fldPath, "field is immutable"),
130-
}
131-
case !oldValueIsUnset && !valueIsUnset:
132-
return field.ErrorList{
133-
field.Forbidden(fldPath, "field is immutable"),
134-
}
135-
default:
136-
// Both unset, shouldn't happen since we checked equality
137-
return nil
120+
return field.ErrorList{
121+
field.Forbidden(fldPath, "field is immutable"),
138122
}
139123
}
140124

141-
func immutableCheck[T comparable](op operation.Operation, fldPath *field.Path,
142-
value, oldValue *T,
143-
isUnset func(*T) bool,
144-
) field.ErrorList {
125+
func immutableByCompareCheck[T comparable](op operation.Operation,
126+
fldPath *field.Path, value, oldValue *T,
127+
isUnset func(*T) bool) field.ErrorList {
145128
if op.Type != operation.Update {
146129
return nil
147130
}
@@ -158,39 +141,37 @@ func immutableCheck[T comparable](op operation.Operation, fldPath *field.Path,
158141
}
159142
}
160143

161-
if *value == *oldValue {
162-
return nil
163-
}
164-
165144
oldIsUnset := isUnset(oldValue)
166145
newIsUnset := isUnset(value)
167-
168-
switch {
169-
case oldIsUnset && !newIsUnset:
146+
if oldIsUnset == newIsUnset && *value == *oldValue {
170147
return nil
171-
case !oldIsUnset && newIsUnset:
172-
return field.ErrorList{
173-
field.Forbidden(fldPath, "field is immutable"),
174-
}
175-
case !oldIsUnset && !newIsUnset:
176-
return field.ErrorList{
177-
field.Forbidden(fldPath, "field is immutable"),
178-
}
179-
default:
180-
// Both unset, shouldn't happen since we checked equality
148+
}
149+
if oldIsUnset && !newIsUnset {
181150
return nil
182151
}
152+
return field.ErrorList{
153+
field.Forbidden(fldPath, "field is immutable"),
154+
}
183155
}
184156

185-
// isUnsetForImmutable determines if a value should
186-
// be considered "unset" for immutability.
187-
func isUnsetForImmutable(value interface{}) bool {
157+
// isUnsetComparable determines if a comparable value should be considered
158+
// "unset" for immutability validation by comparing to its zero value.
159+
func isUnsetComparable[T comparable](v *T) bool {
160+
if v == nil {
161+
return true
162+
}
163+
var zero T
164+
return *v == zero
165+
}
166+
167+
// isUnsetReflect determines if value should be considered "unset"
168+
// for immutability validation by comparing to its zero value.
169+
func isUnsetForReflect(value interface{}) bool {
188170
if value == nil {
189171
return true
190172
}
191173

192174
v := reflect.ValueOf(value)
193-
194175
if v.Kind() == reflect.Ptr {
195176
if v.IsNil() {
196177
return true
@@ -202,7 +183,6 @@ func isUnsetForImmutable(value interface{}) bool {
202183
zero := reflect.Zero(elem.Type())
203184
return reflect.DeepEqual(elem.Interface(), zero.Interface())
204185
}
205-
206186
// For pointers to other types, being non-nil means it's set.
207187
// Aligns with +k8s:required behavior for pointer fields.
208188
return false

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

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,36 @@ func TestFrozenByReflect(t *testing.T) {
249249
}
250250
}
251251

252+
func TestFrozenVariantsConsistency(t *testing.T) {
253+
for _, tc := range []struct {
254+
name string
255+
oldValue *string
256+
newValue *string
257+
}{
258+
{"string both nil", nil, nil},
259+
{"string nil to empty", nil, ptr.To("")},
260+
{"string nil to value", nil, ptr.To("hello")},
261+
{"string empty to value", ptr.To(""), ptr.To("hello")},
262+
{"string value to empty", ptr.To("hello"), ptr.To("")},
263+
{"string same value", ptr.To("hello"), ptr.To("hello")},
264+
{"string different values", ptr.To("hello"), ptr.To("world")},
265+
} {
266+
t.Run(tc.name, func(t *testing.T) {
267+
ctx := context.Background()
268+
op := operation.Operation{Type: operation.Update}
269+
path := field.NewPath("test")
270+
271+
errs1 := FrozenByCompare(ctx, op, path, tc.newValue, tc.oldValue)
272+
errs2 := FrozenByReflect(ctx, op, path, tc.newValue, tc.oldValue)
273+
274+
if len(errs1) != len(errs2) {
275+
t.Errorf("FrozenByCompare and FrozenByReflect differ: %v, %v",
276+
errs1, errs2)
277+
}
278+
})
279+
}
280+
}
281+
252282
func TestImmutableValueByCompare(t *testing.T) {
253283
for _, tc := range []struct {
254284
name string
@@ -558,3 +588,43 @@ func TestImmutableByReflect(t *testing.T) {
558588
})
559589
}
560590
}
591+
592+
func TestImmutableVariantsConsistency(t *testing.T) {
593+
for _, tc := range []struct {
594+
name string
595+
oldValue *string
596+
newValue *string
597+
}{
598+
{"string both nil", nil, nil},
599+
{"string nil to empty", nil, ptr.To("")},
600+
{"string nil to value", nil, ptr.To("hello")},
601+
{"string empty to value", ptr.To(""), ptr.To("hello")},
602+
{"string value to empty", ptr.To("hello"), ptr.To("")},
603+
{"string same value", ptr.To("hello"), ptr.To("hello")},
604+
{"string different values", ptr.To("hello"), ptr.To("world")},
605+
} {
606+
t.Run(tc.name, func(t *testing.T) {
607+
ctx := context.Background()
608+
op := operation.Operation{Type: operation.Update}
609+
path := field.NewPath("test")
610+
611+
errs1 := ImmutablePointerByCompare(ctx, op, path, tc.newValue, tc.oldValue)
612+
errs2 := ImmutableByReflect(ctx, op, path, tc.newValue, tc.oldValue)
613+
614+
if len(errs1) != len(errs2) {
615+
t.Errorf("ImmutablePointerByCompare and ImmutableByReflect differ: %v, %v",
616+
errs1, errs2)
617+
}
618+
619+
if tc.oldValue != nil && tc.newValue != nil {
620+
errs3 := ImmutableValueByCompare(ctx, op, path, tc.newValue, tc.oldValue)
621+
errs4 := ImmutableByReflect(ctx, op, path, *tc.newValue, *tc.oldValue)
622+
623+
if len(errs3) != len(errs4) {
624+
t.Errorf("ImmutableValueByCompare and ImmutableByReflect differ: %v, %v",
625+
errs3, errs4)
626+
}
627+
}
628+
})
629+
}
630+
}

0 commit comments

Comments
 (0)