Skip to content

Commit 797eae5

Browse files
nwntnwnt
authored
fix: tighten the VAP rule for all managed resources (#1212)
2 parents 42ebac7 + a828d88 commit 797eae5

File tree

9 files changed

+450
-385
lines changed

9 files changed

+450
-385
lines changed

‎charts/member-agent-arc/templates/serviceaccount.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
apiVersion: v1
22
kind: ServiceAccount
33
metadata:
4+
# the name is also used in the managed resource validating admission webhook.
5+
# please make the change accordingly when you change the name.
46
name: fleet-member-agent-sa
57
namespace: fleet-system
68
labels:

‎cmd/hubagent/main.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ func main() {
205205
exitWithErrorFunc()
206206
}
207207

208-
if err := managedresource.EnsureVAP(ctx, mgr.GetClient(), true); err != nil {
208+
if err := managedresource.EnsureVAP(ctx, mgr.GetClient()); err != nil {
209209
klog.Errorf("unable to create managed resource validating admission policy: %s", err)
210210
exitWithErrorFunc()
211211
}

‎pkg/controllers/internalmembercluster/v1beta1/member_controller.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
214214

215215
switch imc.Spec.State {
216216
case clusterv1beta1.ClusterStateJoin:
217-
if err := managedresource.EnsureVAP(ctx, r.memberClient, false); err != nil {
217+
if err := managedresource.EnsureVAP(ctx, r.memberClient); err != nil {
218218
return ctrl.Result{}, err
219219
}
220220
klog.V(2).InfoS("Successfully installed managed resource validating admission policy")
@@ -247,7 +247,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
247247
return ctrl.Result{RequeueAfter: time.Millisecond *
248248
(time.Duration(hbinterval) + time.Duration(utilrand.Int63nRange(0, jitterRange)-jitterRange/2))}, nil
249249
case clusterv1beta1.ClusterStateLeave:
250-
if err := managedresource.EnsureNoVAP(ctx, r.memberClient, false); err != nil {
250+
if err := managedresource.EnsureNoVAP(ctx, r.memberClient); err != nil {
251251
return ctrl.Result{}, err
252252
}
253253
klog.V(2).InfoS("Successfully uninstalled managed resource validating admission policy")

‎pkg/webhook/managedresource/createordelete.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ import (
1616
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
1717
)
1818

19-
func EnsureNoVAP(ctx context.Context, c client.Client, isHub bool) error {
20-
objs := []client.Object{getValidatingAdmissionPolicy(isHub), getValidatingAdmissionPolicyBinding()}
19+
func EnsureNoVAP(ctx context.Context, c client.Client) error {
20+
objs := []client.Object{getValidatingAdmissionPolicy(), getValidatingAdmissionPolicyBinding()}
2121
for _, obj := range objs {
2222
err := c.Delete(ctx, obj)
2323
switch {
@@ -34,13 +34,13 @@ func EnsureNoVAP(ctx context.Context, c client.Client, isHub bool) error {
3434
return nil
3535
}
3636

37-
func EnsureVAP(ctx context.Context, c client.Client, isHub bool) error {
37+
func EnsureVAP(ctx context.Context, c client.Client) error {
3838
type vapObjectAndMutator struct {
3939
obj client.Object
4040
mutate func() error
4141
}
4242
// TODO: this can be simplified by dealing with the specific type rather than using client.Object
43-
vap, mutateVAP := getVAPWithMutator(isHub)
43+
vap, mutateVAP := getVAPWithMutator()
4444
vapb, mutateVAPB := getVAPBindingWithMutator()
4545
objsAndMutators := []vapObjectAndMutator{
4646
{
@@ -69,10 +69,10 @@ func EnsureVAP(ctx context.Context, c client.Client, isHub bool) error {
6969
return nil
7070
}
7171

72-
func getVAPWithMutator(isHub bool) (*v1.ValidatingAdmissionPolicy, func() error) {
73-
vap := getValidatingAdmissionPolicy(isHub)
72+
func getVAPWithMutator() (*v1.ValidatingAdmissionPolicy, func() error) {
73+
vap := getValidatingAdmissionPolicy()
7474
return vap, func() error {
75-
mutateValidatingAdmissionPolicy(vap, isHub)
75+
mutateValidatingAdmissionPolicy(vap)
7676
return nil
7777
}
7878
}

‎pkg/webhook/managedresource/createordelete_test.go‎

Lines changed: 31 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -32,44 +32,30 @@ 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)
35+
vap := getValidatingAdmissionPolicy()
3736
binding := getValidatingAdmissionPolicyBinding()
3837

3938
tests := []struct {
4039
name string
41-
isHub bool
4240
existingObjs []client.Object
4341
createOrUpdateErrors map[client.ObjectKey]error
4442
wantErr bool
4543
wantErrMessage string
4644
wantObjects []client.Object
4745
}{
4846
{
49-
name: "hub cluster - create new objects",
50-
isHub: true,
47+
name: "create new objects",
5148
existingObjs: []client.Object{},
5249
wantErr: false,
5350
wantObjects: []client.Object{
54-
vapHub.DeepCopy(),
51+
vap.DeepCopy(),
5552
binding.DeepCopy(),
5653
},
5754
},
5855
{
59-
name: "member cluster - create new objects",
60-
isHub: false,
61-
existingObjs: []client.Object{},
62-
wantErr: false,
63-
wantObjects: []client.Object{
64-
vapMember.DeepCopy(),
65-
binding.DeepCopy(),
66-
},
67-
},
68-
{
69-
name: "hub cluster - update existing objects",
70-
isHub: true,
56+
name: "update existing objects",
7157
existingObjs: func() []client.Object {
72-
existingVAP := vapHub.DeepCopy()
58+
existingVAP := vap.DeepCopy()
7359
existingBinding := binding.DeepCopy()
7460

7561
existingVAP.Spec.Validations = nil
@@ -79,32 +65,29 @@ func TestEnsureVAP(t *testing.T) {
7965
}(),
8066
wantErr: false,
8167
wantObjects: []client.Object{
82-
vapHub.DeepCopy(),
68+
vap.DeepCopy(),
8369
binding.DeepCopy(),
8470
},
8571
},
8672
{
87-
name: "hub cluster - skip no match error",
88-
isHub: true,
73+
name: "skip no match error (cluster version < v1.30)",
8974
existingObjs: []client.Object{},
9075
createOrUpdateErrors: map[client.ObjectKey]error{
91-
client.ObjectKeyFromObject(vapHub): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}},
76+
client.ObjectKeyFromObject(vap): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}},
9277
},
9378
wantErr: false,
9479
},
9580
{
96-
name: "hub cluster - create error propagated",
97-
isHub: true,
81+
name: "vap create error",
9882
existingObjs: []client.Object{},
9983
createOrUpdateErrors: map[client.ObjectKey]error{
100-
client.ObjectKeyFromObject(vapHub): apierrors.NewInternalError(errors.New("internal server error")),
84+
client.ObjectKeyFromObject(vap): apierrors.NewInternalError(errors.New("internal server error")),
10185
},
10286
wantErr: true,
10387
wantErrMessage: "internal server error",
10488
},
10589
{
106-
name: "member cluster - binding creation error",
107-
isHub: false,
90+
name: "binding create error",
10891
existingObjs: []client.Object{},
10992
createOrUpdateErrors: map[client.ObjectKey]error{
11093
client.ObjectKeyFromObject(binding): apierrors.NewForbidden(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicybindings"}, binding.Name, errors.New("forbidden")),
@@ -173,7 +156,7 @@ func TestEnsureVAP(t *testing.T) {
173156
WithInterceptorFuncs(interceptorFuncs).
174157
Build()
175158

176-
err := EnsureVAP(context.Background(), fakeClient, tt.isHub)
159+
err := EnsureVAP(context.Background(), fakeClient)
177160

178161
if tt.wantErr {
179162
if err == nil {
@@ -219,74 +202,57 @@ func TestEnsureNoVAP(t *testing.T) {
219202
t.Fatalf("Failed to add admissionregistration scheme: %v", err)
220203
}
221204

222-
vapHub := getValidatingAdmissionPolicy(true)
223-
vapMember := getValidatingAdmissionPolicy(false)
205+
vap := getValidatingAdmissionPolicy()
224206
binding := getValidatingAdmissionPolicyBinding()
225207

226208
tests := []struct {
227209
name string
228-
isHub bool
229210
existingObjs []client.Object
230211
deleteErrors map[client.ObjectKey]error
231212
wantErr bool
232213
wantErrMessage string
233214
}{
234215
{
235-
name: "hub cluster - no existing objects",
236-
isHub: true,
216+
name: "no existing objects",
237217
existingObjs: []client.Object{},
238218
wantErr: false,
239219
},
240220
{
241-
name: "hub cluster - existing objects deleted successfully",
242-
isHub: true,
221+
name: "existing objects deleted successfully",
243222
existingObjs: []client.Object{
244-
vapHub.DeepCopy(),
223+
vap.DeepCopy(),
245224
binding.DeepCopy(),
246225
},
247226
wantErr: false,
248227
},
249228
{
250-
name: "member cluster - existing objects deleted successfully",
251-
isHub: false,
252-
existingObjs: []client.Object{
253-
vapMember.DeepCopy(),
254-
binding.DeepCopy(),
255-
},
256-
wantErr: false,
257-
},
258-
{
259-
name: "hub cluster - not found errors ignored",
260-
isHub: true,
229+
name: "not found errors ignored",
261230
existingObjs: []client.Object{},
262231
deleteErrors: map[client.ObjectKey]error{
263-
client.ObjectKeyFromObject(vapHub): apierrors.NewNotFound(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicies"}, vapHub.Name),
232+
client.ObjectKeyFromObject(vap): apierrors.NewNotFound(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicies"}, vap.Name),
264233
client.ObjectKeyFromObject(binding): apierrors.NewNotFound(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicybindings"}, binding.Name),
265234
},
266235
wantErr: false,
267236
},
268237
{
269-
name: "hub cluster - no match error handled gracefully",
270-
isHub: true,
238+
name: "no match error handled gracefully",
271239
existingObjs: []client.Object{},
272240
deleteErrors: map[client.ObjectKey]error{
273-
client.ObjectKeyFromObject(vapHub): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}},
241+
client.ObjectKeyFromObject(vap): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}},
274242
},
275243
wantErr: false,
276244
},
277245
{
278-
name: "hub cluster - delete error",
279-
isHub: true,
246+
name: "delete error on vap",
280247
existingObjs: []client.Object{},
281248
deleteErrors: map[client.ObjectKey]error{
282-
client.ObjectKeyFromObject(vapHub): apierrors.NewInternalError(errors.New("internal server error")),
249+
client.ObjectKeyFromObject(vap): apierrors.NewInternalError(errors.New("internal server error")),
283250
},
284251
wantErr: true,
285252
wantErrMessage: "internal server error",
286253
},
287254
{
288-
name: "member cluster - delete error on binding propagated",
289-
isHub: false,
255+
name: "delete error on vap binding",
290256
existingObjs: []client.Object{},
291257
deleteErrors: map[client.ObjectKey]error{
292258
client.ObjectKeyFromObject(binding): apierrors.NewForbidden(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicybindings"}, binding.Name, errors.New("forbidden")),
@@ -316,7 +282,7 @@ func TestEnsureNoVAP(t *testing.T) {
316282
WithInterceptorFuncs(interceptorFuncs).
317283
Build()
318284

319-
err := EnsureNoVAP(context.Background(), fakeClient, tt.isHub)
285+
err := EnsureNoVAP(context.Background(), fakeClient)
320286

321287
if tt.wantErr {
322288
if err == nil {
@@ -334,7 +300,7 @@ func TestEnsureNoVAP(t *testing.T) {
334300
}
335301

336302
// Verify objects are deleted (or don't exist)
337-
expectedObjs := []client.Object{getValidatingAdmissionPolicy(tt.isHub), getValidatingAdmissionPolicyBinding()}
303+
expectedObjs := []client.Object{getValidatingAdmissionPolicy(), getValidatingAdmissionPolicyBinding()}
338304
for _, obj := range expectedObjs {
339305
err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)
340306
if !apierrors.IsNotFound(err) {
@@ -349,36 +315,32 @@ func TestGetVAPWithMutator(t *testing.T) {
349315
t.Parallel()
350316

351317
tests := []struct {
352-
name string
353-
isHub bool
318+
name string
354319
}{
355320
{
356-
name: "hub cluster VAP",
357-
isHub: true,
358-
},
359-
{
360-
name: "member cluster VAP",
361-
isHub: false,
321+
name: "VAP with mutator",
362322
},
363323
}
364324

365325
for _, tt := range tests {
366326
t.Run(tt.name, func(t *testing.T) {
367327
t.Parallel()
368328

369-
vap, mutateFunc := getVAPWithMutator(tt.isHub)
329+
vap, mutateFunc := getVAPWithMutator()
370330

371331
// Verify initial state
372332
if vap == nil {
373333
t.Fatal("getVAPWithMutator() returned nil VAP")
334+
return
374335
}
375336
if mutateFunc == nil {
376337
t.Fatal("getVAPWithMutator() returned nil mutate function")
338+
return
377339
}
378340

379341
// Verify mutate function works
380342
gotVAP := vap.DeepCopy()
381-
wantVAP := getValidatingAdmissionPolicy(tt.isHub)
343+
wantVAP := getValidatingAdmissionPolicy()
382344
ignoreOpts := cmp.Options{
383345
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation"),
384346
}

0 commit comments

Comments
 (0)