Skip to content

Commit 323249f

Browse files
author
Nont
committed
Fix bugs and add more tests
Signed-off-by: Nont <[email protected]>
1 parent 5bbffb8 commit 323249f

File tree

8 files changed

+499
-245
lines changed

8 files changed

+499
-245
lines changed

‎.codespellignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,4 @@ CROs
44
NotIn
55
fo
66
allReady
7+
AtLeast

‎pkg/webhook/managedresource/createordelete.go‎

Lines changed: 53 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ package managedresource
88
import (
99
"context"
1010

11+
v1 "k8s.io/api/admissionregistration/v1"
1112
apierrors "k8s.io/apimachinery/pkg/api/errors"
1213
"k8s.io/apimachinery/pkg/api/meta"
1314
"k8s.io/klog/v2"
@@ -16,33 +17,69 @@ import (
1617
)
1718

1819
func EnsureNoVAP(ctx context.Context, c client.Client, isHub bool) error {
19-
objs := []client.Object{GetValidatingAdmissionPolicy(isHub), GetValidatingAdmissionPolicyBinding()}
20+
objs := []client.Object{getValidatingAdmissionPolicy(isHub), getValidatingAdmissionPolicyBinding()}
2021
for _, obj := range objs {
2122
err := c.Delete(ctx, obj)
22-
if err != nil && !apierrors.IsNotFound(err) {
23-
if meta.IsNoMatchError(err) {
24-
klog.Infof("ValidatingAdmissionPolicy is not supported in this cluster, skipping deletion")
25-
return nil
26-
}
23+
switch {
24+
case err == nil, apierrors.IsNotFound(err):
25+
// continue
26+
case meta.IsNoMatchError(err):
27+
klog.Infof("object type %T is not supported in this cluster, continuing", obj)
28+
// continue
29+
default:
30+
klog.Errorf("Delete object type %T failed: %s", obj, err)
2731
return err
2832
}
2933
}
3034
return nil
3135
}
3236

3337
func EnsureVAP(ctx context.Context, c client.Client, isHub bool) error {
34-
objs := []client.Object{GetValidatingAdmissionPolicy(isHub), GetValidatingAdmissionPolicyBinding()}
35-
for _, obj := range objs {
36-
opResult, err := controllerutil.CreateOrUpdate(ctx, c, obj, func() error {
37-
return nil
38-
})
39-
if err != nil {
40-
if meta.IsNoMatchError(err) {
41-
klog.Infof("ValidatingAdmissionPolicy is not supported in this cluster, skipping creation")
42-
}
43-
klog.Errorf("ValidatingAdmissionPolicy CreateOrUpdate (operation: %s) failed: %s", opResult, err)
38+
type vapObjectAndMutator struct {
39+
obj client.Object
40+
mutate func() error
41+
}
42+
vap, mutateVAP := getVAPWithMutator(isHub)
43+
vapb, mutateVAPB := getVAPBindingWithMutator()
44+
objsAndMutators := []vapObjectAndMutator{
45+
{
46+
obj: vap,
47+
mutate: mutateVAP,
48+
},
49+
{
50+
obj: vapb,
51+
mutate: mutateVAPB,
52+
},
53+
}
54+
55+
for _, objectMutator := range objsAndMutators {
56+
opResult, err := controllerutil.CreateOrUpdate(ctx, c, objectMutator.obj, objectMutator.mutate)
57+
switch {
58+
case err == nil, apierrors.IsNotFound(err):
59+
// continue
60+
case meta.IsNoMatchError(err):
61+
klog.Infof("object type %T is not supported in this cluster, continuing", objectMutator.obj)
62+
// continue
63+
default:
64+
klog.Errorf("CreateOrUpdate (operation: %s) for object type %T failed: %s", opResult, objectMutator.obj, err)
4465
return err
4566
}
4667
}
4768
return nil
4869
}
70+
71+
func getVAPWithMutator(isHub bool) (*v1.ValidatingAdmissionPolicy, func() error) {
72+
vap := getValidatingAdmissionPolicy(isHub)
73+
return vap, func() error {
74+
mutateValidatingAdmissionPolicy(vap, isHub)
75+
return nil
76+
}
77+
}
78+
79+
func getVAPBindingWithMutator() (*v1.ValidatingAdmissionPolicyBinding, func() error) {
80+
vapb := getValidatingAdmissionPolicyBinding()
81+
return vapb, func() error {
82+
mutateValidatingAdmissionPolicyBinding(vapb)
83+
return nil
84+
}
85+
}

‎pkg/webhook/managedresource/createordelete_test.go‎

Lines changed: 156 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,9 @@ func TestEnsureVAP(t *testing.T) {
3232
t.Fatalf("Failed to add admissionregistration scheme: %v", err)
3333
}
3434

35-
vapHub := GetValidatingAdmissionPolicy(true)
36-
vapMember := GetValidatingAdmissionPolicy(false)
37-
binding := GetValidatingAdmissionPolicyBinding()
35+
vapHub := getValidatingAdmissionPolicy(true)
36+
vapMember := getValidatingAdmissionPolicy(false)
37+
binding := getValidatingAdmissionPolicyBinding()
3838

3939
tests := []struct {
4040
name string
@@ -68,24 +68,29 @@ func TestEnsureVAP(t *testing.T) {
6868
{
6969
name: "hub cluster - update existing objects",
7070
isHub: true,
71-
existingObjs: []client.Object{
72-
vapHub.DeepCopy(),
73-
binding.DeepCopy(),
74-
},
71+
existingObjs: func() []client.Object {
72+
existingVAP := vapHub.DeepCopy()
73+
existingBinding := binding.DeepCopy()
74+
75+
existingVAP.Spec.Validations = nil
76+
existingBinding.Spec.ValidationActions = nil
77+
78+
return []client.Object{existingVAP, existingBinding}
79+
}(),
7580
wantErr: false,
7681
wantObjects: []client.Object{
7782
vapHub.DeepCopy(),
7883
binding.DeepCopy(),
7984
},
8085
},
8186
{
82-
name: "hub cluster - no match error handled gracefully",
87+
name: "hub cluster - skip no match error",
8388
isHub: true,
8489
existingObjs: []client.Object{},
8590
createOrUpdateErrors: map[client.ObjectKey]error{
8691
client.ObjectKeyFromObject(vapHub): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}},
8792
},
88-
wantErr: true, // Note: function returns error even for no match
93+
wantErr: false,
8994
},
9095
{
9196
name: "hub cluster - create error propagated",
@@ -111,24 +116,55 @@ func TestEnsureVAP(t *testing.T) {
111116

112117
for _, tt := range tests {
113118
t.Run(tt.name, func(t *testing.T) {
119+
tt := tt
114120
t.Parallel()
115121

122+
// Object store for tracking updates (since different names eliminate collisions)
123+
objectStore := make(map[client.ObjectKey]client.Object)
124+
for _, obj := range tt.existingObjs {
125+
key := client.ObjectKeyFromObject(obj)
126+
objectStore[key] = obj.DeepCopyObject().(client.Object)
127+
}
128+
116129
interceptorFuncs := interceptor.Funcs{
130+
Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
131+
if storedObj, exists := objectStore[key]; exists {
132+
switch v := obj.(type) {
133+
case *admv1.ValidatingAdmissionPolicy:
134+
if stored, ok := storedObj.(*admv1.ValidatingAdmissionPolicy); ok {
135+
*v = *stored
136+
}
137+
case *admv1.ValidatingAdmissionPolicyBinding:
138+
if stored, ok := storedObj.(*admv1.ValidatingAdmissionPolicyBinding); ok {
139+
*v = *stored
140+
}
141+
}
142+
return nil
143+
}
144+
return c.Get(ctx, key, obj, opts...)
145+
},
117146
Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error {
118147
key := client.ObjectKeyFromObject(obj)
119148
if err, exists := tt.createOrUpdateErrors[key]; exists {
120149
return err
121150
}
122-
return c.Create(ctx, obj, opts...)
151+
err := c.Create(ctx, obj, opts...)
152+
if err == nil {
153+
objectStore[key] = obj.DeepCopyObject().(client.Object)
154+
}
155+
return err
123156
},
124157
Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
125158
key := client.ObjectKeyFromObject(obj)
126159
if err, exists := tt.createOrUpdateErrors[key]; exists {
127160
return err
128161
}
129-
return c.Update(ctx, obj, opts...)
162+
// Update our store with the new object state
163+
objectStore[key] = obj.DeepCopyObject().(client.Object)
164+
return nil
130165
},
131166
}
167+
132168
fakeClient := fake.NewClientBuilder().
133169
WithScheme(scheme).
134170
WithObjects(tt.existingObjs...).
@@ -162,7 +198,9 @@ func TestEnsureVAP(t *testing.T) {
162198
}
163199

164200
// Compare relevant fields (ignore managed fields, resource version, etc.)
165-
ignoreOpts := cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation")
201+
ignoreOpts := cmp.Options{
202+
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation"),
203+
}
166204
if diff := cmp.Diff(wantObj, gotObj, ignoreOpts); diff != "" {
167205
t.Errorf("Object %s mismatch (-want +got):\n%s", wantObj.GetName(), diff)
168206
}
@@ -179,9 +217,9 @@ func TestEnsureNoVAP(t *testing.T) {
179217
t.Fatalf("Failed to add admissionregistration scheme: %v", err)
180218
}
181219

182-
vapHub := GetValidatingAdmissionPolicy(true)
183-
vapMember := GetValidatingAdmissionPolicy(false)
184-
binding := GetValidatingAdmissionPolicyBinding()
220+
vapHub := getValidatingAdmissionPolicy(true)
221+
vapMember := getValidatingAdmissionPolicy(false)
222+
binding := getValidatingAdmissionPolicyBinding()
185223

186224
tests := []struct {
187225
name string
@@ -257,8 +295,8 @@ func TestEnsureNoVAP(t *testing.T) {
257295
}
258296

259297
for _, tt := range tests {
260-
tt := tt
261298
t.Run(tt.name, func(t *testing.T) {
299+
tt := tt
262300
t.Parallel()
263301

264302
interceptorFuncs := interceptor.Funcs{
@@ -294,7 +332,7 @@ func TestEnsureNoVAP(t *testing.T) {
294332
}
295333

296334
// Verify objects are deleted (or don't exist)
297-
expectedObjs := []client.Object{GetValidatingAdmissionPolicy(tt.isHub), GetValidatingAdmissionPolicyBinding()}
335+
expectedObjs := []client.Object{getValidatingAdmissionPolicy(tt.isHub), getValidatingAdmissionPolicyBinding()}
298336
for _, obj := range expectedObjs {
299337
err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)
300338
if !apierrors.IsNotFound(err) {
@@ -304,3 +342,104 @@ func TestEnsureNoVAP(t *testing.T) {
304342
})
305343
}
306344
}
345+
346+
func TestGetVAPWithMutator(t *testing.T) {
347+
t.Parallel()
348+
349+
tests := []struct {
350+
name string
351+
isHub bool
352+
}{
353+
{
354+
name: "hub cluster VAP",
355+
isHub: true,
356+
},
357+
{
358+
name: "member cluster VAP",
359+
isHub: false,
360+
},
361+
}
362+
363+
for _, tt := range tests {
364+
t.Run(tt.name, func(t *testing.T) {
365+
t.Parallel()
366+
367+
vap, mutateFunc := getVAPWithMutator(tt.isHub)
368+
369+
// Verify initial state
370+
if vap == nil {
371+
t.Fatal("getVAPWithMutator() returned nil VAP")
372+
}
373+
if mutateFunc == nil {
374+
t.Fatal("getVAPWithMutator() returned nil mutate function")
375+
}
376+
377+
// Verify mutate function works
378+
originalVAP := vap.DeepCopy()
379+
expectedVAP := getValidatingAdmissionPolicy(tt.isHub)
380+
ignoreOpts := cmp.Options{
381+
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation"),
382+
}
383+
if diff := cmp.Diff(expectedVAP, vap, ignoreOpts); diff != "" {
384+
t.Errorf("VAP after mutation mismatch (-want +got):\n%s", diff)
385+
}
386+
387+
vap.Spec = admv1.ValidatingAdmissionPolicySpec{} // Reset spec to empty to test idempotency
388+
if diff := cmp.Diff(originalVAP, vap); diff == "" {
389+
t.Error("VAP should be different after mutation")
390+
}
391+
392+
// The mutation should restore the spec to the expected state
393+
err := mutateFunc()
394+
if err != nil {
395+
t.Errorf("second mutateFunc() = %v, want nil", err)
396+
}
397+
if diff := cmp.Diff(expectedVAP, vap, ignoreOpts); diff != "" {
398+
t.Errorf("VAP after second mutation mismatch (-want +got):\n%s", diff)
399+
}
400+
})
401+
}
402+
}
403+
404+
func TestGetVAPBindingWithMutator(t *testing.T) {
405+
t.Parallel()
406+
407+
vapb, mutateFunc := getVAPBindingWithMutator()
408+
409+
// Verify initial state
410+
if vapb == nil {
411+
t.Fatal("getVAPBindingWithMutator() returned nil VAP binding")
412+
}
413+
if mutateFunc == nil {
414+
t.Fatal("getVAPBindingWithMutator() returned nil mutate function")
415+
}
416+
417+
// Verify mutate function works
418+
originalVAPB := vapb.DeepCopy()
419+
err := mutateFunc()
420+
if err != nil {
421+
t.Errorf("mutateFunc() = %v, want nil", err)
422+
}
423+
424+
expectedVAPB := getValidatingAdmissionPolicyBinding()
425+
ignoreOpts := cmp.Options{
426+
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation"),
427+
}
428+
if diff := cmp.Diff(expectedVAPB, vapb, ignoreOpts); diff != "" {
429+
t.Errorf("VAP binding mismatch (-want +got):\n%s", diff)
430+
}
431+
432+
vapb.Spec = admv1.ValidatingAdmissionPolicyBindingSpec{} // Reset spec to empty to test mutation
433+
if diff := cmp.Diff(originalVAPB, vapb); diff == "" {
434+
t.Error("VAP binding should be different after mutation")
435+
}
436+
437+
// mutation should restore the spec to the expected state
438+
err = mutateFunc()
439+
if err != nil {
440+
t.Errorf("second mutateFunc() = %v, want nil", err)
441+
}
442+
if diff := cmp.Diff(expectedVAPB, vapb, ignoreOpts); diff != "" {
443+
t.Errorf("VAP binding after second mutation mismatch (-want +got):\n%s", diff)
444+
}
445+
}

0 commit comments

Comments
 (0)