From 66f06fb037cbcfe1eb792ea9b47f90c01cd0f2b9 Mon Sep 17 00:00:00 2001 From: Nont Date: Wed, 17 Sep 2025 08:32:25 -0500 Subject: [PATCH 01/11] Manage lifecycle of the VAP properly Signed-off-by: Nont --- cmd/hubagent/main.go | 6 + .../v1beta1/member_controller.go | 7 + pkg/webhook/managedresource/createordelete.go | 46 ++ .../managedresource/createordelete_test.go | 441 ++++++++++++++++++ 4 files changed, 500 insertions(+) create mode 100644 pkg/webhook/managedresource/createordelete.go create mode 100644 pkg/webhook/managedresource/createordelete_test.go diff --git a/cmd/hubagent/main.go b/cmd/hubagent/main.go index 572653796..26bebedb5 100644 --- a/cmd/hubagent/main.go +++ b/cmd/hubagent/main.go @@ -52,6 +52,7 @@ import ( mcv1beta1 "go.goms.io/fleet/pkg/controllers/membercluster/v1beta1" fleetmetrics "go.goms.io/fleet/pkg/metrics" "go.goms.io/fleet/pkg/webhook" + "go.goms.io/fleet/pkg/webhook/managedresource" // +kubebuilder:scaffold:imports ) @@ -195,6 +196,11 @@ func main() { exitWithErrorFunc() } + if managedresource.EnsureVAP(ctx, mgr.GetClient(), true); err != nil { + klog.Errorf("unable to create managedResource validating admission policy: %s", err) + exitWithErrorFunc() + } + // +kubebuilder:scaffold:builder wg.Add(1) diff --git a/pkg/controllers/internalmembercluster/v1beta1/member_controller.go b/pkg/controllers/internalmembercluster/v1beta1/member_controller.go index 23ed70898..895e82b39 100644 --- a/pkg/controllers/internalmembercluster/v1beta1/member_controller.go +++ b/pkg/controllers/internalmembercluster/v1beta1/member_controller.go @@ -44,6 +44,7 @@ import ( "go.goms.io/fleet/pkg/propertyprovider" "go.goms.io/fleet/pkg/utils/condition" "go.goms.io/fleet/pkg/utils/controller" + "go.goms.io/fleet/pkg/webhook/managedresource" ) // propertyProviderConfig is a group of settings for configuring the the property provider. @@ -213,6 +214,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu switch imc.Spec.State { case clusterv1beta1.ClusterStateJoin: + if err := managedresource.EnsureVAP(ctx, r.memberClient, false); err != nil { + return ctrl.Result{}, err + } if err := r.startAgents(ctx, &imc); err != nil { return ctrl.Result{}, err } @@ -242,6 +246,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{RequeueAfter: time.Millisecond * (time.Duration(hbinterval) + time.Duration(utilrand.Int63nRange(0, jitterRange)-jitterRange/2))}, nil case clusterv1beta1.ClusterStateLeave: + if err := managedresource.EnsureNoVAP(ctx, r.memberClient, false); err != nil { + return ctrl.Result{}, err + } if err := r.stopAgents(ctx, &imc); err != nil { return ctrl.Result{}, err } diff --git a/pkg/webhook/managedresource/createordelete.go b/pkg/webhook/managedresource/createordelete.go new file mode 100644 index 000000000..acde44e1b --- /dev/null +++ b/pkg/webhook/managedresource/createordelete.go @@ -0,0 +1,46 @@ +/* +Copyright (c) Microsoft Corporation. +Licensed under the MIT license. +*/ + +package managedresource + +import ( + "context" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/klog/v2" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +func EnsureNoVAP(ctx context.Context, c client.Client, isHub bool) error { + objs := []client.Object{GetValidatingAdmissionPolicy(isHub), GetValidatingAdmissionPolicyBinding()} + for _, obj := range objs { + err := c.Delete(ctx, obj) + if err != nil && !apierrors.IsNotFound(err) { + if meta.IsNoMatchError(err) { + klog.Infof("ValidatingAdmissionPolicy is not supported in this cluster, skipping deletion") + return nil + } + return err + } + } + return nil +} + +func EnsureVAP(ctx context.Context, c client.Client, isHub bool) error { + objs := []client.Object{GetValidatingAdmissionPolicy(isHub), GetValidatingAdmissionPolicyBinding()} + for _, obj := range objs { + opResult, err := controllerutil.CreateOrUpdate(ctx, c, obj, nil) + if err != nil { + if meta.IsNoMatchError(err) { + klog.Infof("ValidatingAdmissionPolicy is not supported in this cluster, skipping creation") + } + klog.Errorf("ValidatingAdmissionPolicy CreateOrUpdate (operation: %) failed: %s", opResult, err) + return err + } + } + return nil +} diff --git a/pkg/webhook/managedresource/createordelete_test.go b/pkg/webhook/managedresource/createordelete_test.go new file mode 100644 index 000000000..92a7e203f --- /dev/null +++ b/pkg/webhook/managedresource/createordelete_test.go @@ -0,0 +1,441 @@ +/* +Copyright (c) Microsoft Corporation. +Licensed under the MIT license. +*/ + +package managedresource + +import ( + "context" + "errors" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + admv1 "k8s.io/api/admissionregistration/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +func TestEnsureNoVAP(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := admv1.AddToScheme(scheme); err != nil { + t.Fatalf("Failed to add admissionregistration scheme: %v", err) + } + + vapHub := GetValidatingAdmissionPolicy(true) + vapMember := GetValidatingAdmissionPolicy(false) + binding := GetValidatingAdmissionPolicyBinding() + + tests := []struct { + name string + isHub bool + existingObjs []client.Object + deleteErrors map[client.ObjectKey]error + wantErr bool + wantErrMessage string + }{ + { + name: "hub cluster - no existing objects", + isHub: true, + existingObjs: []client.Object{}, + wantErr: false, + }, + { + name: "hub cluster - existing objects deleted successfully", + isHub: true, + existingObjs: []client.Object{ + vapHub.DeepCopy(), + binding.DeepCopy(), + }, + wantErr: false, + }, + { + name: "member cluster - existing objects deleted successfully", + isHub: false, + existingObjs: []client.Object{ + vapMember.DeepCopy(), + binding.DeepCopy(), + }, + wantErr: false, + }, + { + name: "hub cluster - not found errors ignored", + isHub: true, + existingObjs: []client.Object{}, + deleteErrors: map[client.ObjectKey]error{ + client.ObjectKeyFromObject(vapHub): apierrors.NewNotFound(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicies"}, vapHub.Name), + client.ObjectKeyFromObject(binding): apierrors.NewNotFound(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicybindings"}, binding.Name), + }, + wantErr: false, + }, + { + name: "hub cluster - no match error handled gracefully", + isHub: true, + existingObjs: []client.Object{}, + deleteErrors: map[client.ObjectKey]error{ + client.ObjectKeyFromObject(vapHub): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}}, + }, + wantErr: false, + }, + { + name: "hub cluster - delete error propagated", + isHub: true, + existingObjs: []client.Object{}, + deleteErrors: map[client.ObjectKey]error{ + client.ObjectKeyFromObject(vapHub): apierrors.NewInternalError(errors.New("internal server error")), + }, + wantErr: true, + wantErrMessage: "internal server error", + }, + { + name: "member cluster - delete error on binding propagated", + isHub: false, + existingObjs: []client.Object{}, + deleteErrors: map[client.ObjectKey]error{ + client.ObjectKeyFromObject(binding): apierrors.NewForbidden(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicybindings"}, binding.Name, errors.New("forbidden")), + }, + wantErr: true, + wantErrMessage: "forbidden", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(tt.existingObjs...). + WithInterceptorFuncs(interceptorFuncs{ + Delete: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + key := client.ObjectKeyFromObject(obj) + if err, exists := tt.deleteErrors[key]; exists { + return err + } + return client.Delete(ctx, obj, opts...) + }, + }). + Build() + + err := EnsureNoVAP(context.Background(), fakeClient, tt.isHub) + + if tt.wantErr { + if err == nil { + t.Error("EnsureNoVAP() = nil, want error") + return + } + if tt.wantErrMessage != "" && !errorContains(err, tt.wantErrMessage) { + t.Errorf("EnsureNoVAP() error = %v, want error containing %q", err, tt.wantErrMessage) + } + return + } + + if err != nil { + t.Errorf("EnsureNoVAP() = %v, want nil", err) + } + + // Verify objects are deleted (or don't exist) + expectedObjs := []client.Object{GetValidatingAdmissionPolicy(tt.isHub), GetValidatingAdmissionPolicyBinding()} + for _, obj := range expectedObjs { + err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(obj), obj) + if !apierrors.IsNotFound(err) { + t.Errorf("Expected object %s to be deleted, but Get() = %v", obj.GetName(), err) + } + } + }) + } +} + +func TestEnsureVAP(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := admv1.AddToScheme(scheme); err != nil { + t.Fatalf("Failed to add admissionregistration scheme: %v", err) + } + + vapHub := GetValidatingAdmissionPolicy(true) + vapMember := GetValidatingAdmissionPolicy(false) + binding := GetValidatingAdmissionPolicyBinding() + + tests := []struct { + name string + isHub bool + existingObjs []client.Object + createOrUpdateErrors map[client.ObjectKey]error + wantErr bool + wantErrMessage string + wantObjects []client.Object + }{ + { + name: "hub cluster - create new objects", + isHub: true, + existingObjs: []client.Object{}, + wantErr: false, + wantObjects: []client.Object{ + vapHub.DeepCopy(), + binding.DeepCopy(), + }, + }, + { + name: "member cluster - create new objects", + isHub: false, + existingObjs: []client.Object{}, + wantErr: false, + wantObjects: []client.Object{ + vapMember.DeepCopy(), + binding.DeepCopy(), + }, + }, + { + name: "hub cluster - update existing objects", + isHub: true, + existingObjs: []client.Object{ + vapHub.DeepCopy(), + binding.DeepCopy(), + }, + wantErr: false, + wantObjects: []client.Object{ + vapHub.DeepCopy(), + binding.DeepCopy(), + }, + }, + { + name: "hub cluster - no match error handled gracefully", + isHub: true, + existingObjs: []client.Object{}, + createOrUpdateErrors: map[client.ObjectKey]error{ + client.ObjectKeyFromObject(vapHub): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}}, + }, + wantErr: true, // Note: function returns error even for no match + }, + { + name: "hub cluster - create error propagated", + isHub: true, + existingObjs: []client.Object{}, + createOrUpdateErrors: map[client.ObjectKey]error{ + client.ObjectKeyFromObject(vapHub): apierrors.NewInternalError(errors.New("internal server error")), + }, + wantErr: true, + wantErrMessage: "internal server error", + }, + { + name: "member cluster - binding creation error propagated", + isHub: false, + existingObjs: []client.Object{}, + createOrUpdateErrors: map[client.ObjectKey]error{ + client.ObjectKeyFromObject(binding): apierrors.NewForbidden(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicybindings"}, binding.Name, errors.New("forbidden")), + }, + wantErr: true, + wantErrMessage: "forbidden", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(tt.existingObjs...). + WithInterceptorFuncs(interceptorFuncs{ + SubResourcePatch: func(ctx context.Context, client client.WithWatch, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { + key := client.ObjectKeyFromObject(obj) + if err, exists := tt.createOrUpdateErrors[key]; exists { + return err + } + return client.SubResourcePatch(ctx, obj, patch, opts...) + }, + Create: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + key := client.ObjectKeyFromObject(obj) + if err, exists := tt.createOrUpdateErrors[key]; exists { + return err + } + return client.Create(ctx, obj, opts...) + }, + Update: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + key := client.ObjectKeyFromObject(obj) + if err, exists := tt.createOrUpdateErrors[key]; exists { + return err + } + return client.Update(ctx, obj, opts...) + }, + }). + Build() + + err := EnsureVAP(context.Background(), fakeClient, tt.isHub) + + if tt.wantErr { + if err == nil { + t.Error("EnsureVAP() = nil, want error") + return + } + if tt.wantErrMessage != "" && !errorContains(err, tt.wantErrMessage) { + t.Errorf("EnsureVAP() error = %v, want error containing %q", err, tt.wantErrMessage) + } + return + } + + if err != nil { + t.Errorf("EnsureVAP() = %v, want nil", err) + } + + // Verify objects were created/updated correctly + for _, wantObj := range tt.wantObjects { + gotObj := wantObj.DeepCopyObject().(client.Object) + err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(wantObj), gotObj) + if err != nil { + t.Errorf("Failed to get object %s: %v", wantObj.GetName(), err) + continue + } + + // Compare relevant fields (ignore managed fields, resource version, etc.) + ignoreOpts := cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation") + if diff := cmp.Diff(wantObj, gotObj, ignoreOpts); diff != "" { + t.Errorf("Object %s mismatch (-want +got):\n%s", wantObj.GetName(), diff) + } + } + }) + } +} + +// interceptorFuncs provides a way to intercept client calls for testing +type interceptorFuncs struct { + Get func(ctx context.Context, client client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error + List func(ctx context.Context, client client.WithWatch, list client.ObjectList, opts ...client.ListOption) error + Create func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.CreateOption) error + Delete func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.DeleteOption) error + Update func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.UpdateOption) error + Patch func(ctx context.Context, client client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error + DeleteAllOf func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.DeleteAllOfOption) error + SubResourcePatch func(ctx context.Context, client client.WithWatch, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error +} + +func (f interceptorFuncs) Intercept(ctx context.Context, client client.WithWatch, verb string, obj client.Object, opts ...any) error { + switch verb { + case "get": + if f.Get != nil { + return f.Get(ctx, client, client.ObjectKeyFromObject(obj), obj, opts[0].([]client.GetOption)...) + } + case "list": + if f.List != nil { + return f.List(ctx, client, obj.(client.ObjectList), opts[0].([]client.ListOption)...) + } + case "create": + if f.Create != nil { + return f.Create(ctx, client, obj, opts[0].([]client.CreateOption)...) + } + case "delete": + if f.Delete != nil { + return f.Delete(ctx, client, obj, opts[0].([]client.DeleteOption)...) + } + case "update": + if f.Update != nil { + return f.Update(ctx, client, obj, opts[0].([]client.UpdateOption)...) + } + case "patch": + if f.Patch != nil { + return f.Patch(ctx, client, obj, opts[0].(client.Patch), opts[1].([]client.PatchOption)...) + } + case "deleteAllOf": + if f.DeleteAllOf != nil { + return f.DeleteAllOf(ctx, client, obj, opts[0].([]client.DeleteAllOfOption)...) + } + } + return nil +} + +// errorContains checks if the error message contains the expected substring +func errorContains(err error, expectedSubstr string) bool { + if err == nil { + return expectedSubstr == "" + } + return errorMessageContains(err.Error(), expectedSubstr) +} + +// errorMessageContains is a helper to check if error message contains substring +func errorMessageContains(errMsg, expectedSubstr string) bool { + return len(errMsg) >= len(expectedSubstr) && + (errMsg == expectedSubstr || + len(errMsg) > len(expectedSubstr) && + (errMsg[:len(expectedSubstr)] == expectedSubstr || + errMsg[len(errMsg)-len(expectedSubstr):] == expectedSubstr || + containsSubstring(errMsg, expectedSubstr))) +} + +// containsSubstring checks if a string contains a substring +func containsSubstring(s, substr string) bool { + if len(substr) > len(s) { + return false + } + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +} + +// TestCreateOrUpdateBehavior tests the specific behavior of controllerutil.CreateOrUpdate +func TestCreateOrUpdateBehavior(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := admv1.AddToScheme(scheme); err != nil { + t.Fatalf("Failed to add admissionregistration scheme: %v", err) + } + + t.Run("CreateOrUpdate creates new object", func(t *testing.T) { + t.Parallel() + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() + vap := GetValidatingAdmissionPolicy(true) + + result, err := controllerutil.CreateOrUpdate(context.Background(), fakeClient, vap, nil) + if err != nil { + t.Errorf("CreateOrUpdate() = %v, want nil", err) + } + + if result != controllerutil.OperationResultCreated { + t.Errorf("CreateOrUpdate() result = %v, want %v", result, controllerutil.OperationResultCreated) + } + + // Verify object was created + gotVap := &admv1.ValidatingAdmissionPolicy{} + err = fakeClient.Get(context.Background(), client.ObjectKeyFromObject(vap), gotVap) + if err != nil { + t.Errorf("Failed to get created object: %v", err) + } + }) + + t.Run("CreateOrUpdate updates existing object", func(t *testing.T) { + t.Parallel() + + existingVap := GetValidatingAdmissionPolicy(true) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(existingVap). + Build() + + vap := GetValidatingAdmissionPolicy(true) + result, err := controllerutil.CreateOrUpdate(context.Background(), fakeClient, vap, nil) + if err != nil { + t.Errorf("CreateOrUpdate() = %v, want nil", err) + } + + if result != controllerutil.OperationResultNone { + t.Errorf("CreateOrUpdate() result = %v, want %v", result, controllerutil.OperationResultNone) + } + }) +} \ No newline at end of file From 140603dcf9435e14fc9ba894ab5af2c0a282a929 Mon Sep 17 00:00:00 2001 From: Nont Date: Thu, 18 Sep 2025 17:29:40 -0500 Subject: [PATCH 02/11] Adjust the code Signed-off-by: Nont --- cmd/crdinstaller/main.go | 7 - cmd/crdinstaller/utils/util.go | 19 - cmd/crdinstaller/utils/util_test.go | 36 -- cmd/hubagent/main.go | 4 +- pkg/webhook/managedresource/createordelete.go | 6 +- .../managedresource/createordelete_test.go | 369 ++++++------------ 6 files changed, 123 insertions(+), 318 deletions(-) diff --git a/cmd/crdinstaller/main.go b/cmd/crdinstaller/main.go index e968c46b2..ba945f981 100644 --- a/cmd/crdinstaller/main.go +++ b/cmd/crdinstaller/main.go @@ -71,13 +71,6 @@ func main() { } klog.Infof("Successfully installed %s CRDs", *mode) - - if err := utils.InstallManagedResourceVAP(ctx, client, *mode); err != nil { - klog.Warningf("Failed to install managed resource ValidatingAdmissionPolicy: %v", err) - return - } - - klog.Infof("Successfully installed %s managed resource ValidatingAdmissionPolicy", *mode) } // installCRDs installs the CRDs from the specified directory based on the mode. diff --git a/cmd/crdinstaller/utils/util.go b/cmd/crdinstaller/utils/util.go index 18399f5d5..2ff94dc22 100644 --- a/cmd/crdinstaller/utils/util.go +++ b/cmd/crdinstaller/utils/util.go @@ -15,15 +15,12 @@ import ( "strings" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - - "go.goms.io/fleet/pkg/webhook/managedresource" ) const ( @@ -174,19 +171,3 @@ func GetCRDFromPath(crdPath string, scheme *runtime.Scheme) (*apiextensionsv1.Cu return crd, nil } - -func InstallManagedResourceVAP(ctx context.Context, c client.Client, mode string) error { - vap := managedresource.GetValidatingAdmissionPolicy(mode == "hub") - vapBinding := managedresource.GetValidatingAdmissionPolicyBinding() - for _, ob := range []client.Object{vap, vapBinding} { - if err := install(ctx, c, ob, nil); err != nil { - if meta.IsNoMatchError(err) { - klog.Infof("Cluster does not support %s resource, skipping installation", ob.GetObjectKind().GroupVersionKind().Kind) - return nil - } - return err - } - } - klog.Infof("Successfully installed managed resource ValidatingAdmissionPolicy") - return nil -} diff --git a/cmd/crdinstaller/utils/util_test.go b/cmd/crdinstaller/utils/util_test.go index f4f8b3cdc..fbcaa8e24 100644 --- a/cmd/crdinstaller/utils/util_test.go +++ b/cmd/crdinstaller/utils/util_test.go @@ -13,7 +13,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" - admv1 "k8s.io/api/admissionregistration/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -301,38 +300,3 @@ func TestInstall(t *testing.T) { }) } } - -func TestInstallManagedResourceVAP(t *testing.T) { - tests := []struct { - name string - mode string - }{ - { - name: "hub mode", - mode: "hub", - }, - { - name: "member mode", - mode: "member", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - scheme := runtime.NewScheme() - if err := admv1.AddToScheme(scheme); err != nil { - t.Fatalf("Failed to add admissionregistration scheme: %v", err) - } - - fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() - err := InstallManagedResourceVAP(context.Background(), fakeClient, tt.mode) - - // The function should complete without errors - // The actual installation behavior depends on the RESTMapper implementation - // which is difficult to test reliably with the fake client - if err != nil { - t.Errorf("InstallManagedResourceVAP() unexpected error: %v", err) - } - }) - } -} diff --git a/cmd/hubagent/main.go b/cmd/hubagent/main.go index 26bebedb5..d2cf7c25a 100644 --- a/cmd/hubagent/main.go +++ b/cmd/hubagent/main.go @@ -196,8 +196,8 @@ func main() { exitWithErrorFunc() } - if managedresource.EnsureVAP(ctx, mgr.GetClient(), true); err != nil { - klog.Errorf("unable to create managedResource validating admission policy: %s", err) + if err := managedresource.EnsureVAP(ctx, mgr.GetClient(), true); err != nil { + klog.Errorf("unable to create managed resource validating admission policy: %s", err) exitWithErrorFunc() } diff --git a/pkg/webhook/managedresource/createordelete.go b/pkg/webhook/managedresource/createordelete.go index acde44e1b..58ad4707b 100644 --- a/pkg/webhook/managedresource/createordelete.go +++ b/pkg/webhook/managedresource/createordelete.go @@ -33,12 +33,14 @@ func EnsureNoVAP(ctx context.Context, c client.Client, isHub bool) error { func EnsureVAP(ctx context.Context, c client.Client, isHub bool) error { objs := []client.Object{GetValidatingAdmissionPolicy(isHub), GetValidatingAdmissionPolicyBinding()} for _, obj := range objs { - opResult, err := controllerutil.CreateOrUpdate(ctx, c, obj, nil) + opResult, err := controllerutil.CreateOrUpdate(ctx, c, obj, func() error { + return nil + }) if err != nil { if meta.IsNoMatchError(err) { klog.Infof("ValidatingAdmissionPolicy is not supported in this cluster, skipping creation") } - klog.Errorf("ValidatingAdmissionPolicy CreateOrUpdate (operation: %) failed: %s", opResult, err) + klog.Errorf("ValidatingAdmissionPolicy CreateOrUpdate (operation: %s) failed: %s", opResult, err) return err } } diff --git a/pkg/webhook/managedresource/createordelete_test.go b/pkg/webhook/managedresource/createordelete_test.go index 92a7e203f..1b7082ad3 100644 --- a/pkg/webhook/managedresource/createordelete_test.go +++ b/pkg/webhook/managedresource/createordelete_test.go @@ -8,6 +8,7 @@ package managedresource import ( "context" "errors" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -20,10 +21,10 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ) -func TestEnsureNoVAP(t *testing.T) { +func TestEnsureVAP(t *testing.T) { t.Parallel() scheme := runtime.NewScheme() @@ -36,71 +37,71 @@ func TestEnsureNoVAP(t *testing.T) { binding := GetValidatingAdmissionPolicyBinding() tests := []struct { - name string - isHub bool - existingObjs []client.Object - deleteErrors map[client.ObjectKey]error - wantErr bool - wantErrMessage string + name string + isHub bool + existingObjs []client.Object + createOrUpdateErrors map[client.ObjectKey]error + wantErr bool + wantErrMessage string + wantObjects []client.Object }{ { - name: "hub cluster - no existing objects", + name: "hub cluster - create new objects", isHub: true, existingObjs: []client.Object{}, wantErr: false, - }, - { - name: "hub cluster - existing objects deleted successfully", - isHub: true, - existingObjs: []client.Object{ + wantObjects: []client.Object{ vapHub.DeepCopy(), binding.DeepCopy(), }, - wantErr: false, }, { - name: "member cluster - existing objects deleted successfully", - isHub: false, - existingObjs: []client.Object{ + name: "member cluster - create new objects", + isHub: false, + existingObjs: []client.Object{}, + wantErr: false, + wantObjects: []client.Object{ vapMember.DeepCopy(), binding.DeepCopy(), }, - wantErr: false, }, { - name: "hub cluster - not found errors ignored", - isHub: true, - existingObjs: []client.Object{}, - deleteErrors: map[client.ObjectKey]error{ - client.ObjectKeyFromObject(vapHub): apierrors.NewNotFound(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicies"}, vapHub.Name), - client.ObjectKeyFromObject(binding): apierrors.NewNotFound(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicybindings"}, binding.Name), + name: "hub cluster - update existing objects", + isHub: true, + existingObjs: []client.Object{ + vapHub.DeepCopy(), + binding.DeepCopy(), }, wantErr: false, + wantObjects: []client.Object{ + vapHub.DeepCopy(), + binding.DeepCopy(), + }, }, { name: "hub cluster - no match error handled gracefully", isHub: true, existingObjs: []client.Object{}, - deleteErrors: map[client.ObjectKey]error{ + createOrUpdateErrors: map[client.ObjectKey]error{ client.ObjectKeyFromObject(vapHub): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}}, }, - wantErr: false, + wantErr: true, // Note: function returns error even for no match }, { - name: "hub cluster - delete error propagated", + name: "hub cluster - create error propagated", isHub: true, existingObjs: []client.Object{}, - deleteErrors: map[client.ObjectKey]error{ + createOrUpdateErrors: map[client.ObjectKey]error{ client.ObjectKeyFromObject(vapHub): apierrors.NewInternalError(errors.New("internal server error")), }, wantErr: true, wantErrMessage: "internal server error", }, { - name: "member cluster - delete error on binding propagated", + name: "member cluster - binding creation error", isHub: false, existingObjs: []client.Object{}, - deleteErrors: map[client.ObjectKey]error{ + createOrUpdateErrors: map[client.ObjectKey]error{ client.ObjectKeyFromObject(binding): apierrors.NewForbidden(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicybindings"}, binding.Name, errors.New("forbidden")), }, wantErr: true, @@ -109,54 +110,68 @@ func TestEnsureNoVAP(t *testing.T) { } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() + interceptorFuncs := interceptor.Funcs{ + Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + key := client.ObjectKeyFromObject(obj) + if err, exists := tt.createOrUpdateErrors[key]; exists { + return err + } + return c.Create(ctx, obj, opts...) + }, + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + key := client.ObjectKeyFromObject(obj) + if err, exists := tt.createOrUpdateErrors[key]; exists { + return err + } + return c.Update(ctx, obj, opts...) + }, + } fakeClient := fake.NewClientBuilder(). WithScheme(scheme). WithObjects(tt.existingObjs...). - WithInterceptorFuncs(interceptorFuncs{ - Delete: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { - key := client.ObjectKeyFromObject(obj) - if err, exists := tt.deleteErrors[key]; exists { - return err - } - return client.Delete(ctx, obj, opts...) - }, - }). + WithInterceptorFuncs(interceptorFuncs). Build() - err := EnsureNoVAP(context.Background(), fakeClient, tt.isHub) + err := EnsureVAP(context.Background(), fakeClient, tt.isHub) if tt.wantErr { if err == nil { - t.Error("EnsureNoVAP() = nil, want error") + t.Error("EnsureVAP() = nil, want error") return } - if tt.wantErrMessage != "" && !errorContains(err, tt.wantErrMessage) { - t.Errorf("EnsureNoVAP() error = %v, want error containing %q", err, tt.wantErrMessage) + if tt.wantErrMessage != "" && !strings.Contains(err.Error(), tt.wantErrMessage) { + t.Errorf("EnsureVAP() error = %v, want error containing %q", err, tt.wantErrMessage) } return } if err != nil { - t.Errorf("EnsureNoVAP() = %v, want nil", err) + t.Errorf("EnsureVAP() = %v, want nil", err) } - // Verify objects are deleted (or don't exist) - expectedObjs := []client.Object{GetValidatingAdmissionPolicy(tt.isHub), GetValidatingAdmissionPolicyBinding()} - for _, obj := range expectedObjs { - err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(obj), obj) - if !apierrors.IsNotFound(err) { - t.Errorf("Expected object %s to be deleted, but Get() = %v", obj.GetName(), err) + // Verify objects were created/updated correctly + for _, wantObj := range tt.wantObjects { + gotObj := wantObj.DeepCopyObject().(client.Object) + err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(wantObj), gotObj) + if err != nil { + t.Errorf("Failed to get object %s: %v", wantObj.GetName(), err) + continue + } + + // Compare relevant fields (ignore managed fields, resource version, etc.) + ignoreOpts := cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation") + if diff := cmp.Diff(wantObj, gotObj, ignoreOpts); diff != "" { + t.Errorf("Object %s mismatch (-want +got):\n%s", wantObj.GetName(), diff) } } }) } } -func TestEnsureVAP(t *testing.T) { +func TestEnsureNoVAP(t *testing.T) { t.Parallel() scheme := runtime.NewScheme() @@ -169,71 +184,71 @@ func TestEnsureVAP(t *testing.T) { binding := GetValidatingAdmissionPolicyBinding() tests := []struct { - name string - isHub bool - existingObjs []client.Object - createOrUpdateErrors map[client.ObjectKey]error - wantErr bool - wantErrMessage string - wantObjects []client.Object + name string + isHub bool + existingObjs []client.Object + deleteErrors map[client.ObjectKey]error + wantErr bool + wantErrMessage string }{ { - name: "hub cluster - create new objects", + name: "hub cluster - no existing objects", isHub: true, existingObjs: []client.Object{}, wantErr: false, - wantObjects: []client.Object{ + }, + { + name: "hub cluster - existing objects deleted successfully", + isHub: true, + existingObjs: []client.Object{ vapHub.DeepCopy(), binding.DeepCopy(), }, + wantErr: false, }, { - name: "member cluster - create new objects", + name: "member cluster - existing objects deleted successfully", isHub: false, - existingObjs: []client.Object{}, - wantErr: false, - wantObjects: []client.Object{ + existingObjs: []client.Object{ vapMember.DeepCopy(), binding.DeepCopy(), }, + wantErr: false, }, { - name: "hub cluster - update existing objects", - isHub: true, - existingObjs: []client.Object{ - vapHub.DeepCopy(), - binding.DeepCopy(), + name: "hub cluster - not found errors ignored", + isHub: true, + existingObjs: []client.Object{}, + deleteErrors: map[client.ObjectKey]error{ + client.ObjectKeyFromObject(vapHub): apierrors.NewNotFound(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicies"}, vapHub.Name), + client.ObjectKeyFromObject(binding): apierrors.NewNotFound(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicybindings"}, binding.Name), }, wantErr: false, - wantObjects: []client.Object{ - vapHub.DeepCopy(), - binding.DeepCopy(), - }, }, { name: "hub cluster - no match error handled gracefully", isHub: true, existingObjs: []client.Object{}, - createOrUpdateErrors: map[client.ObjectKey]error{ + deleteErrors: map[client.ObjectKey]error{ client.ObjectKeyFromObject(vapHub): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}}, }, - wantErr: true, // Note: function returns error even for no match + wantErr: false, }, { - name: "hub cluster - create error propagated", + name: "hub cluster - delete error", isHub: true, existingObjs: []client.Object{}, - createOrUpdateErrors: map[client.ObjectKey]error{ + deleteErrors: map[client.ObjectKey]error{ client.ObjectKeyFromObject(vapHub): apierrors.NewInternalError(errors.New("internal server error")), }, wantErr: true, wantErrMessage: "internal server error", }, { - name: "member cluster - binding creation error propagated", + name: "member cluster - delete error on binding propagated", isHub: false, existingObjs: []client.Object{}, - createOrUpdateErrors: map[client.ObjectKey]error{ + deleteErrors: map[client.ObjectKey]error{ client.ObjectKeyFromObject(binding): apierrors.NewForbidden(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicybindings"}, binding.Name, errors.New("forbidden")), }, wantErr: true, @@ -246,196 +261,46 @@ func TestEnsureVAP(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() + interceptorFuncs := interceptor.Funcs{ + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + key := client.ObjectKeyFromObject(obj) + if err, exists := tt.deleteErrors[key]; exists { + return err + } + return c.Delete(ctx, obj, opts...) + }, + } fakeClient := fake.NewClientBuilder(). WithScheme(scheme). WithObjects(tt.existingObjs...). - WithInterceptorFuncs(interceptorFuncs{ - SubResourcePatch: func(ctx context.Context, client client.WithWatch, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { - key := client.ObjectKeyFromObject(obj) - if err, exists := tt.createOrUpdateErrors[key]; exists { - return err - } - return client.SubResourcePatch(ctx, obj, patch, opts...) - }, - Create: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.CreateOption) error { - key := client.ObjectKeyFromObject(obj) - if err, exists := tt.createOrUpdateErrors[key]; exists { - return err - } - return client.Create(ctx, obj, opts...) - }, - Update: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { - key := client.ObjectKeyFromObject(obj) - if err, exists := tt.createOrUpdateErrors[key]; exists { - return err - } - return client.Update(ctx, obj, opts...) - }, - }). + WithInterceptorFuncs(interceptorFuncs). Build() - err := EnsureVAP(context.Background(), fakeClient, tt.isHub) + err := EnsureNoVAP(context.Background(), fakeClient, tt.isHub) if tt.wantErr { if err == nil { - t.Error("EnsureVAP() = nil, want error") + t.Error("EnsureNoVAP() = nil, want error") return } - if tt.wantErrMessage != "" && !errorContains(err, tt.wantErrMessage) { - t.Errorf("EnsureVAP() error = %v, want error containing %q", err, tt.wantErrMessage) + if tt.wantErrMessage != "" && !strings.Contains(err.Error(), tt.wantErrMessage) { + t.Errorf("EnsureNoVAP() error = %v, want error containing %q", err, tt.wantErrMessage) } return } if err != nil { - t.Errorf("EnsureVAP() = %v, want nil", err) + t.Errorf("EnsureNoVAP() = %v, want nil", err) } - // Verify objects were created/updated correctly - for _, wantObj := range tt.wantObjects { - gotObj := wantObj.DeepCopyObject().(client.Object) - err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(wantObj), gotObj) - if err != nil { - t.Errorf("Failed to get object %s: %v", wantObj.GetName(), err) - continue - } - - // Compare relevant fields (ignore managed fields, resource version, etc.) - ignoreOpts := cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation") - if diff := cmp.Diff(wantObj, gotObj, ignoreOpts); diff != "" { - t.Errorf("Object %s mismatch (-want +got):\n%s", wantObj.GetName(), diff) + // Verify objects are deleted (or don't exist) + expectedObjs := []client.Object{GetValidatingAdmissionPolicy(tt.isHub), GetValidatingAdmissionPolicyBinding()} + for _, obj := range expectedObjs { + err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(obj), obj) + if !apierrors.IsNotFound(err) { + t.Errorf("Expected object %s to be deleted, but Get() = %v", obj.GetName(), err) } } }) } } - -// interceptorFuncs provides a way to intercept client calls for testing -type interceptorFuncs struct { - Get func(ctx context.Context, client client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error - List func(ctx context.Context, client client.WithWatch, list client.ObjectList, opts ...client.ListOption) error - Create func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.CreateOption) error - Delete func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.DeleteOption) error - Update func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.UpdateOption) error - Patch func(ctx context.Context, client client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error - DeleteAllOf func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.DeleteAllOfOption) error - SubResourcePatch func(ctx context.Context, client client.WithWatch, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error -} - -func (f interceptorFuncs) Intercept(ctx context.Context, client client.WithWatch, verb string, obj client.Object, opts ...any) error { - switch verb { - case "get": - if f.Get != nil { - return f.Get(ctx, client, client.ObjectKeyFromObject(obj), obj, opts[0].([]client.GetOption)...) - } - case "list": - if f.List != nil { - return f.List(ctx, client, obj.(client.ObjectList), opts[0].([]client.ListOption)...) - } - case "create": - if f.Create != nil { - return f.Create(ctx, client, obj, opts[0].([]client.CreateOption)...) - } - case "delete": - if f.Delete != nil { - return f.Delete(ctx, client, obj, opts[0].([]client.DeleteOption)...) - } - case "update": - if f.Update != nil { - return f.Update(ctx, client, obj, opts[0].([]client.UpdateOption)...) - } - case "patch": - if f.Patch != nil { - return f.Patch(ctx, client, obj, opts[0].(client.Patch), opts[1].([]client.PatchOption)...) - } - case "deleteAllOf": - if f.DeleteAllOf != nil { - return f.DeleteAllOf(ctx, client, obj, opts[0].([]client.DeleteAllOfOption)...) - } - } - return nil -} - -// errorContains checks if the error message contains the expected substring -func errorContains(err error, expectedSubstr string) bool { - if err == nil { - return expectedSubstr == "" - } - return errorMessageContains(err.Error(), expectedSubstr) -} - -// errorMessageContains is a helper to check if error message contains substring -func errorMessageContains(errMsg, expectedSubstr string) bool { - return len(errMsg) >= len(expectedSubstr) && - (errMsg == expectedSubstr || - len(errMsg) > len(expectedSubstr) && - (errMsg[:len(expectedSubstr)] == expectedSubstr || - errMsg[len(errMsg)-len(expectedSubstr):] == expectedSubstr || - containsSubstring(errMsg, expectedSubstr))) -} - -// containsSubstring checks if a string contains a substring -func containsSubstring(s, substr string) bool { - if len(substr) > len(s) { - return false - } - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -} - -// TestCreateOrUpdateBehavior tests the specific behavior of controllerutil.CreateOrUpdate -func TestCreateOrUpdateBehavior(t *testing.T) { - t.Parallel() - - scheme := runtime.NewScheme() - if err := admv1.AddToScheme(scheme); err != nil { - t.Fatalf("Failed to add admissionregistration scheme: %v", err) - } - - t.Run("CreateOrUpdate creates new object", func(t *testing.T) { - t.Parallel() - - fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() - vap := GetValidatingAdmissionPolicy(true) - - result, err := controllerutil.CreateOrUpdate(context.Background(), fakeClient, vap, nil) - if err != nil { - t.Errorf("CreateOrUpdate() = %v, want nil", err) - } - - if result != controllerutil.OperationResultCreated { - t.Errorf("CreateOrUpdate() result = %v, want %v", result, controllerutil.OperationResultCreated) - } - - // Verify object was created - gotVap := &admv1.ValidatingAdmissionPolicy{} - err = fakeClient.Get(context.Background(), client.ObjectKeyFromObject(vap), gotVap) - if err != nil { - t.Errorf("Failed to get created object: %v", err) - } - }) - - t.Run("CreateOrUpdate updates existing object", func(t *testing.T) { - t.Parallel() - - existingVap := GetValidatingAdmissionPolicy(true) - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects(existingVap). - Build() - - vap := GetValidatingAdmissionPolicy(true) - result, err := controllerutil.CreateOrUpdate(context.Background(), fakeClient, vap, nil) - if err != nil { - t.Errorf("CreateOrUpdate() = %v, want nil", err) - } - - if result != controllerutil.OperationResultNone { - t.Errorf("CreateOrUpdate() result = %v, want %v", result, controllerutil.OperationResultNone) - } - }) -} \ No newline at end of file From 5bbffb88623e67d79f2894bc2ea1c96bc0919305 Mon Sep 17 00:00:00 2001 From: Nont Date: Thu, 18 Sep 2025 19:19:38 -0500 Subject: [PATCH 03/11] Add logs and e2e test cases Signed-off-by: Nont --- cmd/hubagent/main.go | 17 ++++--- .../v1beta1/member_controller.go | 2 + test/e2e/managed_resource_vap_test.go | 44 ++++++++++++++++--- test/e2e/setup_test.go | 10 +++++ 4 files changed, 61 insertions(+), 12 deletions(-) diff --git a/cmd/hubagent/main.go b/cmd/hubagent/main.go index d2cf7c25a..1fa081ee2 100644 --- a/cmd/hubagent/main.go +++ b/cmd/hubagent/main.go @@ -196,11 +196,6 @@ func main() { exitWithErrorFunc() } - if err := managedresource.EnsureVAP(ctx, mgr.GetClient(), true); err != nil { - klog.Errorf("unable to create managed resource validating admission policy: %s", err) - exitWithErrorFunc() - } - // +kubebuilder:scaffold:builder wg.Add(1) @@ -216,6 +211,18 @@ func main() { klog.InfoS("The controller manager has exited") }() + // Wait for the cache to sync before creating managed resources + if !mgr.GetCache().WaitForCacheSync(ctx) { + klog.Error("failed to wait for cache sync") + exitWithErrorFunc() + } + + if err := managedresource.EnsureVAP(ctx, mgr.GetClient(), true); err != nil { + klog.Errorf("unable to create managed resource validating admission policy: %s", err) + exitWithErrorFunc() + } + klog.Info("managed resource validating admission policy is successfully setup") + // Wait for the controller manager and the scheduler to exit. wg.Wait() } diff --git a/pkg/controllers/internalmembercluster/v1beta1/member_controller.go b/pkg/controllers/internalmembercluster/v1beta1/member_controller.go index 895e82b39..c05af5086 100644 --- a/pkg/controllers/internalmembercluster/v1beta1/member_controller.go +++ b/pkg/controllers/internalmembercluster/v1beta1/member_controller.go @@ -217,6 +217,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu if err := managedresource.EnsureVAP(ctx, r.memberClient, false); err != nil { return ctrl.Result{}, err } + klog.V(2).InfoS("Successfully installed managed resource validating admission policy") if err := r.startAgents(ctx, &imc); err != nil { return ctrl.Result{}, err } @@ -249,6 +250,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu if err := managedresource.EnsureNoVAP(ctx, r.memberClient, false); err != nil { return ctrl.Result{}, err } + klog.V(2).InfoS("Successfully uninstalled managed resource validating admission policy") if err := r.stopAgents(ctx, &imc); err != nil { return ctrl.Result{}, err } diff --git a/test/e2e/managed_resource_vap_test.go b/test/e2e/managed_resource_vap_test.go index 5bbd641c3..d03baf5d6 100644 --- a/test/e2e/managed_resource_vap_test.go +++ b/test/e2e/managed_resource_vap_test.go @@ -29,6 +29,7 @@ import ( k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" ) @@ -126,14 +127,43 @@ func expectDeniedByVAP(err error) { ), "Error should indicate policy violation") } -var _ = Describe("ValidatingAdmissionPolicy for Managed Resources", Label("managedresource"), Ordered, func() { - It("The VAP and its binding should exist", func() { - var vap admissionregistrationv1.ValidatingAdmissionPolicy - Expect(sysMastersClient.Get(ctx, types.NamespacedName{Name: vapName}, &vap)).Should(Succeed(), "ValidatingAdmissionPolicy should be installed") +func checkVAPAndBindingExistence(client client.Client) { + By(fmt.Sprintf("Checking existence of ValidatingAdmissionPolicy: %s", vapName)) + var vap admissionregistrationv1.ValidatingAdmissionPolicy + Expect(client.Get(ctx, types.NamespacedName{Name: vapName}, &vap)).To(Succeed(), + fmt.Sprintf("ValidatingAdmissionPolicy %s should exist", vapName)) - var vapBinding admissionregistrationv1.ValidatingAdmissionPolicyBinding - Expect(sysMastersClient.Get(ctx, types.NamespacedName{Name: vapName}, &vapBinding)).Should(Succeed(), "ValidatingAdmissionPolicyBinding should be installed") - }) + By(fmt.Sprintf("Checking existence of ValidatingAdmissionPolicyBinding: %s", vapName)) + var vapBinding admissionregistrationv1.ValidatingAdmissionPolicyBinding + Expect(client.Get(ctx, types.NamespacedName{Name: vapName}, &vapBinding)).To(Succeed(), + fmt.Sprintf("ValidatingAdmissionPolicyBinding %s should exist", vapName)) + + By("Verifying VAP has validations configured") + Expect(vap.Spec.Validations).ToNot(BeEmpty(), "VAP should have validation rules") + + By("Verifying VAP binding references the correct policy") + Expect(vapBinding.Spec.PolicyName).To(Equal(vapName), + fmt.Sprintf("VAP binding should reference policy %s", vapName)) + + By("Verifying VAP binding has validation actions configured") + Expect(vapBinding.Spec.ValidationActions).ToNot(BeEmpty(), "VAP binding should have validation actions") +} + +func checkVAPAndBindingAbsence(client client.Client) { + By(fmt.Sprintf("Checking absence of ValidatingAdmissionPolicy: %s", vapName)) + var vap admissionregistrationv1.ValidatingAdmissionPolicy + err := client.Get(ctx, types.NamespacedName{Name: vapName}, &vap) + Expect(k8sErrors.IsNotFound(err)).To(BeTrue(), + fmt.Sprintf("ValidatingAdmissionPolicy %s should not exist", vapName)) + + By(fmt.Sprintf("Checking absence of ValidatingAdmissionPolicyBinding: %s", vapName)) + var vapBinding admissionregistrationv1.ValidatingAdmissionPolicyBinding + err = client.Get(ctx, types.NamespacedName{Name: vapName}, &vapBinding) + Expect(k8sErrors.IsNotFound(err)).To(BeTrue(), + fmt.Sprintf("ValidatingAdmissionPolicyBinding %s should not exist", vapName)) +} + +var _ = Describe("ValidatingAdmissionPolicy for Managed Resources", Label("managedresource"), Ordered, func() { It("should allow operations on unmanaged namespace for non-system:masters user", func() { unmanagedNS := createUnmanagedNamespace("test-unmanaged-ns") diff --git a/test/e2e/setup_test.go b/test/e2e/setup_test.go index 4b2c8a302..d89fe02f1 100644 --- a/test/e2e/setup_test.go +++ b/test/e2e/setup_test.go @@ -380,6 +380,9 @@ func beforeSuiteForAllProcesses() { _, err := os.Stat(fleetBinaryPath) Expect(os.IsNotExist(err)).To(BeFalse(), fmt.Sprintf("kubectl-fleet binary not found at %s", fleetBinaryPath)) }) + + // Expect that the managedResource VAP and its binding to exist when hub starts + checkVAPAndBindingExistence(hubClient) } func maxDuration(a, b time.Duration) time.Duration { @@ -394,6 +397,10 @@ func beforeSuiteForProcess1() { setAllMemberClustersToJoin() checkIfAllMemberClustersHaveJoined() + for _, c := range allMemberClusters { + checkVAPAndBindingExistence(c.KubeClient) + } + checkIfAzurePropertyProviderIsWorking() // Simulate that member cluster 4 become unhealthy, and member cluster 5 has left the fleet. @@ -411,5 +418,8 @@ var _ = SynchronizedAfterSuite(func() {}, func() { deleteTestResourceCRD() setAllMemberClustersToLeave() checkIfAllMemberClustersHaveLeft() + for _, c := range allMemberClusters { + checkVAPAndBindingAbsence(c.KubeClient) + } cleanupInvalidClusters() }) From 323249f7efcf97bd36ce153bf4a8ea05078e82e8 Mon Sep 17 00:00:00 2001 From: Nont Date: Mon, 29 Sep 2025 15:02:48 -0500 Subject: [PATCH 04/11] Fix bugs and add more tests Signed-off-by: Nont --- .codespellignore | 1 + pkg/webhook/managedresource/createordelete.go | 69 +++- .../managedresource/createordelete_test.go | 173 ++++++++- .../validatingadmissionpolicy.go | 134 ++++--- .../validatingadmissionpolicy_test.go | 6 +- test/e2e/framework/cluster.go | 10 + test/e2e/managed_resource_vap_test.go | 345 ++++++++++-------- test/e2e/setup_test.go | 6 +- 8 files changed, 499 insertions(+), 245 deletions(-) diff --git a/.codespellignore b/.codespellignore index ac6601862..4b9a4b8c3 100644 --- a/.codespellignore +++ b/.codespellignore @@ -4,3 +4,4 @@ CROs NotIn fo allReady +AtLeast diff --git a/pkg/webhook/managedresource/createordelete.go b/pkg/webhook/managedresource/createordelete.go index 58ad4707b..b2103b0ea 100644 --- a/pkg/webhook/managedresource/createordelete.go +++ b/pkg/webhook/managedresource/createordelete.go @@ -8,6 +8,7 @@ package managedresource import ( "context" + v1 "k8s.io/api/admissionregistration/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/klog/v2" @@ -16,14 +17,17 @@ import ( ) func EnsureNoVAP(ctx context.Context, c client.Client, isHub bool) error { - objs := []client.Object{GetValidatingAdmissionPolicy(isHub), GetValidatingAdmissionPolicyBinding()} + objs := []client.Object{getValidatingAdmissionPolicy(isHub), getValidatingAdmissionPolicyBinding()} for _, obj := range objs { err := c.Delete(ctx, obj) - if err != nil && !apierrors.IsNotFound(err) { - if meta.IsNoMatchError(err) { - klog.Infof("ValidatingAdmissionPolicy is not supported in this cluster, skipping deletion") - return nil - } + switch { + case err == nil, apierrors.IsNotFound(err): + // continue + case meta.IsNoMatchError(err): + klog.Infof("object type %T is not supported in this cluster, continuing", obj) + // continue + default: + klog.Errorf("Delete object type %T failed: %s", obj, err) return err } } @@ -31,18 +35,51 @@ func EnsureNoVAP(ctx context.Context, c client.Client, isHub bool) error { } func EnsureVAP(ctx context.Context, c client.Client, isHub bool) error { - objs := []client.Object{GetValidatingAdmissionPolicy(isHub), GetValidatingAdmissionPolicyBinding()} - for _, obj := range objs { - opResult, err := controllerutil.CreateOrUpdate(ctx, c, obj, func() error { - return nil - }) - if err != nil { - if meta.IsNoMatchError(err) { - klog.Infof("ValidatingAdmissionPolicy is not supported in this cluster, skipping creation") - } - klog.Errorf("ValidatingAdmissionPolicy CreateOrUpdate (operation: %s) failed: %s", opResult, err) + type vapObjectAndMutator struct { + obj client.Object + mutate func() error + } + vap, mutateVAP := getVAPWithMutator(isHub) + vapb, mutateVAPB := getVAPBindingWithMutator() + objsAndMutators := []vapObjectAndMutator{ + { + obj: vap, + mutate: mutateVAP, + }, + { + obj: vapb, + mutate: mutateVAPB, + }, + } + + for _, objectMutator := range objsAndMutators { + opResult, err := controllerutil.CreateOrUpdate(ctx, c, objectMutator.obj, objectMutator.mutate) + switch { + case err == nil, apierrors.IsNotFound(err): + // continue + case meta.IsNoMatchError(err): + klog.Infof("object type %T is not supported in this cluster, continuing", objectMutator.obj) + // continue + default: + klog.Errorf("CreateOrUpdate (operation: %s) for object type %T failed: %s", opResult, objectMutator.obj, err) return err } } return nil } + +func getVAPWithMutator(isHub bool) (*v1.ValidatingAdmissionPolicy, func() error) { + vap := getValidatingAdmissionPolicy(isHub) + return vap, func() error { + mutateValidatingAdmissionPolicy(vap, isHub) + return nil + } +} + +func getVAPBindingWithMutator() (*v1.ValidatingAdmissionPolicyBinding, func() error) { + vapb := getValidatingAdmissionPolicyBinding() + return vapb, func() error { + mutateValidatingAdmissionPolicyBinding(vapb) + return nil + } +} diff --git a/pkg/webhook/managedresource/createordelete_test.go b/pkg/webhook/managedresource/createordelete_test.go index 1b7082ad3..07f0caf6d 100644 --- a/pkg/webhook/managedresource/createordelete_test.go +++ b/pkg/webhook/managedresource/createordelete_test.go @@ -32,9 +32,9 @@ func TestEnsureVAP(t *testing.T) { t.Fatalf("Failed to add admissionregistration scheme: %v", err) } - vapHub := GetValidatingAdmissionPolicy(true) - vapMember := GetValidatingAdmissionPolicy(false) - binding := GetValidatingAdmissionPolicyBinding() + vapHub := getValidatingAdmissionPolicy(true) + vapMember := getValidatingAdmissionPolicy(false) + binding := getValidatingAdmissionPolicyBinding() tests := []struct { name string @@ -68,10 +68,15 @@ func TestEnsureVAP(t *testing.T) { { name: "hub cluster - update existing objects", isHub: true, - existingObjs: []client.Object{ - vapHub.DeepCopy(), - binding.DeepCopy(), - }, + existingObjs: func() []client.Object { + existingVAP := vapHub.DeepCopy() + existingBinding := binding.DeepCopy() + + existingVAP.Spec.Validations = nil + existingBinding.Spec.ValidationActions = nil + + return []client.Object{existingVAP, existingBinding} + }(), wantErr: false, wantObjects: []client.Object{ vapHub.DeepCopy(), @@ -79,13 +84,13 @@ func TestEnsureVAP(t *testing.T) { }, }, { - name: "hub cluster - no match error handled gracefully", + name: "hub cluster - skip no match error", isHub: true, existingObjs: []client.Object{}, createOrUpdateErrors: map[client.ObjectKey]error{ client.ObjectKeyFromObject(vapHub): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}}, }, - wantErr: true, // Note: function returns error even for no match + wantErr: false, }, { name: "hub cluster - create error propagated", @@ -111,24 +116,55 @@ func TestEnsureVAP(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + tt := tt t.Parallel() + // Object store for tracking updates (since different names eliminate collisions) + objectStore := make(map[client.ObjectKey]client.Object) + for _, obj := range tt.existingObjs { + key := client.ObjectKeyFromObject(obj) + objectStore[key] = obj.DeepCopyObject().(client.Object) + } + interceptorFuncs := interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if storedObj, exists := objectStore[key]; exists { + switch v := obj.(type) { + case *admv1.ValidatingAdmissionPolicy: + if stored, ok := storedObj.(*admv1.ValidatingAdmissionPolicy); ok { + *v = *stored + } + case *admv1.ValidatingAdmissionPolicyBinding: + if stored, ok := storedObj.(*admv1.ValidatingAdmissionPolicyBinding); ok { + *v = *stored + } + } + return nil + } + return c.Get(ctx, key, obj, opts...) + }, Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { key := client.ObjectKeyFromObject(obj) if err, exists := tt.createOrUpdateErrors[key]; exists { return err } - return c.Create(ctx, obj, opts...) + err := c.Create(ctx, obj, opts...) + if err == nil { + objectStore[key] = obj.DeepCopyObject().(client.Object) + } + return err }, Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { key := client.ObjectKeyFromObject(obj) if err, exists := tt.createOrUpdateErrors[key]; exists { return err } - return c.Update(ctx, obj, opts...) + // Update our store with the new object state + objectStore[key] = obj.DeepCopyObject().(client.Object) + return nil }, } + fakeClient := fake.NewClientBuilder(). WithScheme(scheme). WithObjects(tt.existingObjs...). @@ -162,7 +198,9 @@ func TestEnsureVAP(t *testing.T) { } // Compare relevant fields (ignore managed fields, resource version, etc.) - ignoreOpts := cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation") + ignoreOpts := cmp.Options{ + cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation"), + } if diff := cmp.Diff(wantObj, gotObj, ignoreOpts); diff != "" { t.Errorf("Object %s mismatch (-want +got):\n%s", wantObj.GetName(), diff) } @@ -179,9 +217,9 @@ func TestEnsureNoVAP(t *testing.T) { t.Fatalf("Failed to add admissionregistration scheme: %v", err) } - vapHub := GetValidatingAdmissionPolicy(true) - vapMember := GetValidatingAdmissionPolicy(false) - binding := GetValidatingAdmissionPolicyBinding() + vapHub := getValidatingAdmissionPolicy(true) + vapMember := getValidatingAdmissionPolicy(false) + binding := getValidatingAdmissionPolicyBinding() tests := []struct { name string @@ -257,8 +295,8 @@ func TestEnsureNoVAP(t *testing.T) { } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { + tt := tt t.Parallel() interceptorFuncs := interceptor.Funcs{ @@ -294,7 +332,7 @@ func TestEnsureNoVAP(t *testing.T) { } // Verify objects are deleted (or don't exist) - expectedObjs := []client.Object{GetValidatingAdmissionPolicy(tt.isHub), GetValidatingAdmissionPolicyBinding()} + expectedObjs := []client.Object{getValidatingAdmissionPolicy(tt.isHub), getValidatingAdmissionPolicyBinding()} for _, obj := range expectedObjs { err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(obj), obj) if !apierrors.IsNotFound(err) { @@ -304,3 +342,104 @@ func TestEnsureNoVAP(t *testing.T) { }) } } + +func TestGetVAPWithMutator(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + isHub bool + }{ + { + name: "hub cluster VAP", + isHub: true, + }, + { + name: "member cluster VAP", + isHub: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + vap, mutateFunc := getVAPWithMutator(tt.isHub) + + // Verify initial state + if vap == nil { + t.Fatal("getVAPWithMutator() returned nil VAP") + } + if mutateFunc == nil { + t.Fatal("getVAPWithMutator() returned nil mutate function") + } + + // Verify mutate function works + originalVAP := vap.DeepCopy() + expectedVAP := getValidatingAdmissionPolicy(tt.isHub) + ignoreOpts := cmp.Options{ + cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation"), + } + if diff := cmp.Diff(expectedVAP, vap, ignoreOpts); diff != "" { + t.Errorf("VAP after mutation mismatch (-want +got):\n%s", diff) + } + + vap.Spec = admv1.ValidatingAdmissionPolicySpec{} // Reset spec to empty to test idempotency + if diff := cmp.Diff(originalVAP, vap); diff == "" { + t.Error("VAP should be different after mutation") + } + + // The mutation should restore the spec to the expected state + err := mutateFunc() + if err != nil { + t.Errorf("second mutateFunc() = %v, want nil", err) + } + if diff := cmp.Diff(expectedVAP, vap, ignoreOpts); diff != "" { + t.Errorf("VAP after second mutation mismatch (-want +got):\n%s", diff) + } + }) + } +} + +func TestGetVAPBindingWithMutator(t *testing.T) { + t.Parallel() + + vapb, mutateFunc := getVAPBindingWithMutator() + + // Verify initial state + if vapb == nil { + t.Fatal("getVAPBindingWithMutator() returned nil VAP binding") + } + if mutateFunc == nil { + t.Fatal("getVAPBindingWithMutator() returned nil mutate function") + } + + // Verify mutate function works + originalVAPB := vapb.DeepCopy() + err := mutateFunc() + if err != nil { + t.Errorf("mutateFunc() = %v, want nil", err) + } + + expectedVAPB := getValidatingAdmissionPolicyBinding() + ignoreOpts := cmp.Options{ + cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation"), + } + if diff := cmp.Diff(expectedVAPB, vapb, ignoreOpts); diff != "" { + t.Errorf("VAP binding mismatch (-want +got):\n%s", diff) + } + + vapb.Spec = admv1.ValidatingAdmissionPolicyBindingSpec{} // Reset spec to empty to test mutation + if diff := cmp.Diff(originalVAPB, vapb); diff == "" { + t.Error("VAP binding should be different after mutation") + } + + // mutation should restore the spec to the expected state + err = mutateFunc() + if err != nil { + t.Errorf("second mutateFunc() = %v, want nil", err) + } + if diff := cmp.Diff(expectedVAPB, vapb, ignoreOpts); diff != "" { + t.Errorf("VAP binding after second mutation mismatch (-want +got):\n%s", diff) + } +} diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy.go b/pkg/webhook/managedresource/validatingadmissionpolicy.go index fdf7ad631..bb3dcb4a4 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy.go @@ -14,63 +14,74 @@ const resourceName = "aks-fleet-managed-by-arm" var forbidden = metav1.StatusReasonForbidden -func GetValidatingAdmissionPolicy(isHub bool) *admv1.ValidatingAdmissionPolicy { +func getValidatingAdmissionPolicy(isHub bool) *admv1.ValidatingAdmissionPolicy { vap := &admv1.ValidatingAdmissionPolicy{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "admissionregistration.k8s.io/v1", - Kind: "ValidatingAdmissionPolicy", - }, ObjectMeta: metav1.ObjectMeta{ Name: resourceName, }, - Spec: admv1.ValidatingAdmissionPolicySpec{ - MatchConstraints: &admv1.MatchResources{ - ObjectSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "fleet.azure.com/managed-by": "arm", - }, + } + mutateValidatingAdmissionPolicy(vap, isHub) + return vap +} + +func mutateValidatingAdmissionPolicy(vap *admv1.ValidatingAdmissionPolicy, isHub bool) { + vap.TypeMeta = metav1.TypeMeta{ + APIVersion: "admissionregistration.k8s.io/v1", + Kind: "ValidatingAdmissionPolicy", + } + vap.ObjectMeta = metav1.ObjectMeta{ + Name: vap.ObjectMeta.Name, + Labels: map[string]string{ + "fleet.azure.com/managed-by": "arm", + }, + } + vap.Spec = admv1.ValidatingAdmissionPolicySpec{ + MatchConstraints: &admv1.MatchResources{ + ObjectSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "fleet.azure.com/managed-by": "arm", }, - ResourceRules: []admv1.NamedRuleWithOperations{ - { - RuleWithOperations: admv1.RuleWithOperations{ - Rule: admv1.Rule{ - APIGroups: []string{""}, - Resources: []string{"namespaces"}, - APIVersions: []string{"v1"}, - }, - Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, + }, + ResourceRules: []admv1.NamedRuleWithOperations{ + { + RuleWithOperations: admv1.RuleWithOperations{ + Rule: admv1.Rule{ + APIGroups: []string{""}, + Resources: []string{"namespaces"}, + APIVersions: []string{"v1"}, }, + Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, }, - { - RuleWithOperations: admv1.RuleWithOperations{ - Rule: admv1.Rule{ - APIGroups: []string{""}, - Resources: []string{"resourcequotas"}, - APIVersions: []string{"v1"}, - }, - Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, + }, + { + RuleWithOperations: admv1.RuleWithOperations{ + Rule: admv1.Rule{ + APIGroups: []string{""}, + Resources: []string{"resourcequotas"}, + APIVersions: []string{"v1"}, }, - ResourceNames: []string{"default"}, + Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, }, - { - RuleWithOperations: admv1.RuleWithOperations{ - Rule: admv1.Rule{ - APIGroups: []string{"networking.k8s.io"}, - Resources: []string{"networkpolicies"}, - APIVersions: []string{"*"}, - }, - Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, + ResourceNames: []string{"default"}, + }, + { + RuleWithOperations: admv1.RuleWithOperations{ + Rule: admv1.Rule{ + APIGroups: []string{"networking.k8s.io"}, + Resources: []string{"networkpolicies"}, + APIVersions: []string{"*"}, }, - ResourceNames: []string{"default"}, + Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, }, + ResourceNames: []string{"default"}, }, }, - Validations: []admv1.Validation{ - { - Expression: `"system:masters" in request.userInfo.groups || "system:serviceaccounts:kube-system" in request.userInfo.groups || "system:serviceaccounts:fleet-system" in request.userInfo.groups`, - Message: "Create, Update, or Delete operations on ARM managed resources is forbidden", - Reason: &forbidden, - }, + }, + Validations: []admv1.Validation{ + { + Expression: `"system:masters" in request.userInfo.groups || "system:serviceaccounts:kube-system" in request.userInfo.groups || "system:serviceaccounts:fleet-system" in request.userInfo.groups`, + Message: "Create, Update, or Delete operations on ARM managed resources is forbidden", + Reason: &forbidden, }, }, } @@ -87,24 +98,33 @@ func GetValidatingAdmissionPolicy(isHub bool) *admv1.ValidatingAdmissionPolicy { }, }) } - - return vap } -func GetValidatingAdmissionPolicyBinding() *admv1.ValidatingAdmissionPolicyBinding { - return &admv1.ValidatingAdmissionPolicyBinding{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "admissionregistration.k8s.io/v1", - Kind: "ValidatingAdmissionPolicyBinding", - }, +func getValidatingAdmissionPolicyBinding() *admv1.ValidatingAdmissionPolicyBinding { + vapb := &admv1.ValidatingAdmissionPolicyBinding{ ObjectMeta: metav1.ObjectMeta{ Name: resourceName, }, - Spec: admv1.ValidatingAdmissionPolicyBindingSpec{ - PolicyName: resourceName, - ValidationActions: []admv1.ValidationAction{ - admv1.Deny, - }, + } + mutateValidatingAdmissionPolicyBinding(vapb) + return vapb +} + +func mutateValidatingAdmissionPolicyBinding(vapb *admv1.ValidatingAdmissionPolicyBinding) { + vapb.TypeMeta = metav1.TypeMeta{ + APIVersion: "admissionregistration.k8s.io/v1", + Kind: "ValidatingAdmissionPolicyBinding", + } + vapb.ObjectMeta = metav1.ObjectMeta{ + Name: vapb.ObjectMeta.Name, + Labels: map[string]string{ + "fleet.azure.com/managed-by": "arm", + }, + } + vapb.Spec = admv1.ValidatingAdmissionPolicyBindingSpec{ + PolicyName: resourceName, + ValidationActions: []admv1.ValidationAction{ + admv1.Deny, }, } } diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy_test.go b/pkg/webhook/managedresource/validatingadmissionpolicy_test.go index d894cea3f..ca51767ac 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy_test.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy_test.go @@ -18,7 +18,7 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { t.Run("member", func(t *testing.T) { t.Parallel() - vap := GetValidatingAdmissionPolicy(false) + vap := getValidatingAdmissionPolicy(false) assert.NotNil(t, vap) assert.NotContains(t, vap.Spec.MatchConstraints.ResourceRules, admv1.NamedRuleWithOperations{ RuleWithOperations: admv1.RuleWithOperations{ @@ -35,7 +35,7 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { t.Run("hub", func(t *testing.T) { t.Parallel() - vap := GetValidatingAdmissionPolicy(true) + vap := getValidatingAdmissionPolicy(true) assert.NotNil(t, vap) assert.Contains(t, vap.Spec.MatchConstraints.ResourceRules, admv1.NamedRuleWithOperations{ RuleWithOperations: admv1.RuleWithOperations{ @@ -53,6 +53,6 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { func TestGetValidatingAdmissionPolicyBinding(t *testing.T) { t.Parallel() - vap := GetValidatingAdmissionPolicyBinding() + vap := getValidatingAdmissionPolicyBinding() assert.NotNil(t, vap) } diff --git a/test/e2e/framework/cluster.go b/test/e2e/framework/cluster.go index b9f3d7da0..659dbc0ec 100644 --- a/test/e2e/framework/cluster.go +++ b/test/e2e/framework/cluster.go @@ -22,6 +22,7 @@ import ( "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/discovery" "k8s.io/client-go/dynamic" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" @@ -128,3 +129,12 @@ func GetImpersonateClientConfig(cluster *Cluster) clientcmd.ClientConfig { }, }) } + +func GetDiscoveryClient(cluster *Cluster) *discovery.DiscoveryClient { + clusterConfig := GetClientConfig(cluster) + restConfig, err := clusterConfig.ClientConfig() + if err != nil { + gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up rest config") + } + return discovery.NewDiscoveryClientForConfigOrDie(restConfig) +} diff --git a/test/e2e/managed_resource_vap_test.go b/test/e2e/managed_resource_vap_test.go index d03baf5d6..3bed3506e 100644 --- a/test/e2e/managed_resource_vap_test.go +++ b/test/e2e/managed_resource_vap_test.go @@ -27,17 +27,21 @@ import ( corev1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" + "k8s.io/apimachinery/pkg/util/version" + "k8s.io/client-go/discovery" placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" + "go.goms.io/fleet/test/e2e/framework" ) const ( managedByLabel = "fleet.azure.com/managed-by" managedByLabelValue = "arm" vapName = "aks-fleet-managed-by-arm" + k8sVersionWithVAP = "v1.30.0" ) var managedByLabelMap = map[string]string{ @@ -127,36 +131,57 @@ func expectDeniedByVAP(err error) { ), "Error should indicate policy violation") } -func checkVAPAndBindingExistence(client client.Client) { - By(fmt.Sprintf("Checking existence of ValidatingAdmissionPolicy: %s", vapName)) - var vap admissionregistrationv1.ValidatingAdmissionPolicy - Expect(client.Get(ctx, types.NamespacedName{Name: vapName}, &vap)).To(Succeed(), - fmt.Sprintf("ValidatingAdmissionPolicy %s should exist", vapName)) - - By(fmt.Sprintf("Checking existence of ValidatingAdmissionPolicyBinding: %s", vapName)) - var vapBinding admissionregistrationv1.ValidatingAdmissionPolicyBinding - Expect(client.Get(ctx, types.NamespacedName{Name: vapName}, &vapBinding)).To(Succeed(), - fmt.Sprintf("ValidatingAdmissionPolicyBinding %s should exist", vapName)) - - By("Verifying VAP has validations configured") - Expect(vap.Spec.Validations).ToNot(BeEmpty(), "VAP should have validation rules") +func checkVAPAndBindingExistence(c *framework.Cluster) { + client := c.KubeClient + discoveryClient := framework.GetDiscoveryClient(c) - By("Verifying VAP binding references the correct policy") - Expect(vapBinding.Spec.PolicyName).To(Equal(vapName), - fmt.Sprintf("VAP binding should reference policy %s", vapName)) + serverVersionWithVAP, err := isAPIServerVersionAtLeast(discoveryClient, k8sVersionWithVAP) + Expect(err).To(BeNil(), "Error checking API server version") - By("Verifying VAP binding has validation actions configured") - Expect(vapBinding.Spec.ValidationActions).ToNot(BeEmpty(), "VAP binding should have validation actions") + var vap admissionregistrationv1.ValidatingAdmissionPolicy + var vapBinding admissionregistrationv1.ValidatingAdmissionPolicyBinding + if serverVersionWithVAP { + By(fmt.Sprintf("Checking for the existence of ValidatingAdmissionPolicy: %s", vapName)) + Expect(client.Get(ctx, types.NamespacedName{Name: vapName}, &vap)).To(Succeed(), + fmt.Sprintf("ValidatingAdmissionPolicy %s should exist", vapName)) + By(fmt.Sprintf("Checking for the existence of ValidatingAdmissionPolicyBinding: %s", vapName)) + Expect(client.Get(ctx, types.NamespacedName{Name: vapName}, &vapBinding)).To(Succeed(), + fmt.Sprintf("ValidatingAdmissionPolicyBinding %s should exist", vapName)) + + By("Verifying that the VAP has validations configured") + Expect(vap.Spec.Validations).ToNot(BeEmpty(), "VAP should have validation rules") + + By("Verifying that the VAP binding references the correct policy") + Expect(vapBinding.Spec.PolicyName).To(Equal(vapName), + fmt.Sprintf("VAP binding should reference policy %s", vapName)) + + By("Verifying that the VAP binding has validation actions configured") + Expect(vapBinding.Spec.ValidationActions).ToNot(BeEmpty(), "VAP binding should have validation actions") + } else { + By("Verifying that the VAP resource type is not found") + err := client.Get(ctx, types.NamespacedName{Name: vapName}, &vap) + Expect(meta.IsNoMatchError(err)).To(BeTrue()) + By("Verifying that the VAP Binding resource type is not found") + err = client.Get(ctx, types.NamespacedName{Name: vapName}, &vapBinding) + Expect(meta.IsNoMatchError(err)).To(BeTrue()) + } } -func checkVAPAndBindingAbsence(client client.Client) { - By(fmt.Sprintf("Checking absence of ValidatingAdmissionPolicy: %s", vapName)) +func checkVAPAndBindingAbsence(c *framework.Cluster) { + client := c.KubeClient + discoveryClient := framework.GetDiscoveryClient(c) + clusterVersionWithVAP, err := isAPIServerVersionAtLeast(discoveryClient, k8sVersionWithVAP) + Expect(err).To(BeNil(), "Error checking API server version") + if !clusterVersionWithVAP { + return // no check + } + By(fmt.Sprintf("Checking for the absence of ValidatingAdmissionPolicy: %s", vapName)) var vap admissionregistrationv1.ValidatingAdmissionPolicy - err := client.Get(ctx, types.NamespacedName{Name: vapName}, &vap) + err = client.Get(ctx, types.NamespacedName{Name: vapName}, &vap) Expect(k8sErrors.IsNotFound(err)).To(BeTrue(), fmt.Sprintf("ValidatingAdmissionPolicy %s should not exist", vapName)) - By(fmt.Sprintf("Checking absence of ValidatingAdmissionPolicyBinding: %s", vapName)) + By(fmt.Sprintf("Checking for the absence of ValidatingAdmissionPolicyBinding: %s", vapName)) var vapBinding admissionregistrationv1.ValidatingAdmissionPolicyBinding err = client.Get(ctx, types.NamespacedName{Name: vapName}, &vapBinding) Expect(k8sErrors.IsNotFound(err)).To(BeTrue(), @@ -165,167 +190,189 @@ func checkVAPAndBindingAbsence(client client.Client) { var _ = Describe("ValidatingAdmissionPolicy for Managed Resources", Label("managedresource"), Ordered, func() { - It("should allow operations on unmanaged namespace for non-system:masters user", func() { - unmanagedNS := createUnmanagedNamespace("test-unmanaged-ns") - By("expecting successful CREATE operation on unmanaged namespace") - Expect(notMasterUser.Create(ctx, unmanagedNS)).To(Succeed()) + Context("Version-agnostic tests", func() { + It("should allow operations on unmanaged namespace for non-system:masters user", func() { + unmanagedNS := createUnmanagedNamespace("test-unmanaged-ns") + By("expecting successful CREATE operation on unmanaged namespace") + Expect(notMasterUser.Create(ctx, unmanagedNS)).To(Succeed()) - By("expecting successful UPDATE operation on unmanaged namespace") - Eventually(func() error { - var ns corev1.Namespace - if err := notMasterUser.Get(ctx, types.NamespacedName{Name: unmanagedNS.Name}, &ns); err != nil { - return err - } - ns.Annotations = map[string]string{"test": "annotation"} - return notMasterUser.Update(ctx, &ns) - }, eventuallyDuration, eventuallyInterval).Should(Succeed()) - - By("expecting successful DELETE operation on unmanaged namespace") - Expect(notMasterUser.Delete(ctx, unmanagedNS)).To(Succeed()) - }) - - Context("When the namespace doesn't exist", func() { - It("should deny CREATE operation on managed namespace for non-system:masters user", func() { - managedNS := createManagedNamespace("test-managed-ns-create") - By("expecting denial of CREATE operation on managed namespace") - err := notMasterUser.Create(ctx, managedNS) - expectDeniedByVAP(err) - }) - - It("should allow CREATE operation on managed namespace for system:masters user", func() { - managedNS := createManagedNamespace("test-managed-ns-masters") - By("expecting successful CREATE operation with system:masters user") - Expect(sysMastersClient.Create(ctx, managedNS)).To(Succeed()) - - Expect(sysMastersClient.Delete(ctx, managedNS)).To(Succeed()) - }) - }) - - Context("When the namespace exists", Ordered, func() { - managedNS := createManagedNamespace("test-managed-ns-update") - BeforeAll(func() { - err := sysMastersClient.Delete(ctx, managedNS) - if err != nil { - Expect(k8sErrors.IsNotFound(err)).To(BeTrue()) - } - Expect(sysMastersClient.Create(ctx, managedNS)).To(Succeed()) - var ns corev1.Namespace - err = sysMastersClient.Get(ctx, types.NamespacedName{Name: managedNS.Name}, &ns) - Expect(err).To(BeNil()) - Expect(ns.Labels).To(HaveKeyWithValue(managedByLabel, managedByLabelValue)) - }) - - It("should deny DELETE operation on managed namespace for non-system:masters user", func() { - By("expecting denial of DELETE operation on managed namespace") - err := notMasterUser.Delete(ctx, managedNS) - expectDeniedByVAP(err) - }) - - It("should deny UPDATE operation on managed namespace for non-system:masters user", func() { - var updateErr error + By("expecting successful UPDATE operation on unmanaged namespace") Eventually(func() error { var ns corev1.Namespace - if err := sysMastersClient.Get(ctx, types.NamespacedName{Name: managedNS.Name}, &ns); err != nil { + if err := notMasterUser.Get(ctx, types.NamespacedName{Name: unmanagedNS.Name}, &ns); err != nil { return err } ns.Annotations = map[string]string{"test": "annotation"} - By("expecting denial of UPDATE operation on managed namespace") - updateErr = notMasterUser.Update(ctx, &ns) - if k8sErrors.IsConflict(updateErr) { - return updateErr - } - return nil + return notMasterUser.Update(ctx, &ns) }, eventuallyDuration, eventuallyInterval).Should(Succeed()) - expectDeniedByVAP(updateErr) + By("expecting successful DELETE operation on unmanaged namespace") + Expect(notMasterUser.Delete(ctx, unmanagedNS)).To(Succeed()) }) + }) - It("should allow UPDATE operation on managed namespace for system:masters user", func() { - var updateErr error - Eventually(func() error { - var ns corev1.Namespace - if err := sysMastersClient.Get(ctx, types.NamespacedName{Name: managedNS.Name}, &ns); err != nil { - return err - } - ns.Annotations = map[string]string{"test": "annotation"} - By("expecting denial of UPDATE operation on managed namespace") - updateErr = sysMastersClient.Update(ctx, &ns) - if k8sErrors.IsConflict(updateErr) { - return updateErr - } - return nil - }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + Context("VAP validations (requires k8s >= v1.30)", func() { + BeforeEach(func() { + discoveryClient := framework.GetDiscoveryClient(hubCluster) + clusterVersionWithVAP, err := isAPIServerVersionAtLeast(discoveryClient, k8sVersionWithVAP) + Expect(err).To(BeNil(), "Error checking API server version") + if !clusterVersionWithVAP { + Skip(fmt.Sprintf("Skipping VAP tests as the cluster is running Kubernetes version < %s", k8sVersionWithVAP)) + } }) - Context("For other resources in scope", func() { - It("should deny creating managed resource quotas", func() { - rq := createManagedResourceQuota("default", "default") - err := notMasterUser.Create(ctx, rq) + Context("When the namespace doesn't exist", func() { + It("should deny CREATE operation on managed namespace for non-system:masters user", func() { + managedNS := createManagedNamespace("test-managed-ns-create") + By("expecting denial of CREATE operation on managed namespace") + err := notMasterUser.Create(ctx, managedNS) expectDeniedByVAP(err) }) - It("should deny creating managed network policy", func() { - np := createManagedNetworkPolicy("default", "default") - err := notMasterUser.Create(ctx, np) - expectDeniedByVAP(err) + It("should allow CREATE operation on managed namespace for system:masters user", func() { + managedNS := createManagedNamespace("test-managed-ns-masters") + By("expecting successful CREATE operation with system:masters user") + Expect(sysMastersClient.Create(ctx, managedNS)).To(Succeed()) + + Expect(sysMastersClient.Delete(ctx, managedNS)).To(Succeed()) }) + }) - It("should deny creating managed CRP", func() { - crp := createManagedCRP("test-crp") - err := notMasterUser.Create(ctx, crp) - expectDeniedByVAP(err) + Context("When the namespace exists", Ordered, func() { + managedNS := createManagedNamespace("test-managed-ns-update") + BeforeAll(func() { + err := sysMastersClient.Delete(ctx, managedNS) + if err != nil { + Expect(k8sErrors.IsNotFound(err)).To(BeTrue()) + } + Expect(sysMastersClient.Create(ctx, managedNS)).To(Succeed()) + var ns corev1.Namespace + err = sysMastersClient.Get(ctx, types.NamespacedName{Name: managedNS.Name}, &ns) + Expect(err).To(BeNil()) + Expect(ns.Labels).To(HaveKeyWithValue(managedByLabel, managedByLabelValue)) }) - It("general expected behavior of other resources", func() { - rq := createManagedResourceQuota("default", "default") - np := createManagedNetworkPolicy("default", "default") - crp := createManagedCRP("test-crp") - err := sysMastersClient.Create(ctx, rq) - Expect(err).To(BeNil(), "system:masters user should create managed ResourceQuota") - err = sysMastersClient.Create(ctx, np) - Expect(err).To(BeNil(), "system:masters user should create managed NetworkPolicy") - err = sysMastersClient.Create(ctx, crp) - Expect(err).To(BeNil(), "system:masters user should create managed CRP") - - work := createManagedResourcePlacement("test-work") - err = notMasterUser.Create(ctx, work) + It("should deny DELETE operation on managed namespace for non-system:masters user", func() { + By("expecting denial of DELETE operation on managed namespace") + err := notMasterUser.Delete(ctx, managedNS) expectDeniedByVAP(err) + }) + It("should deny UPDATE operation on managed namespace for non-system:masters user", func() { var updateErr error Eventually(func() error { - var urq corev1.ResourceQuota - if err := sysMastersClient.Get(ctx, types.NamespacedName{Name: "default", Namespace: "default"}, &urq); err != nil { + var ns corev1.Namespace + if err := sysMastersClient.Get(ctx, types.NamespacedName{Name: managedNS.Name}, &ns); err != nil { return err } - urq.Annotations = map[string]string{"test": "annotation"} - By("expecting denial of UPDATE operation on managed resource quota") - updateErr = notMasterUser.Update(ctx, &urq) + ns.Annotations = map[string]string{"test": "annotation"} + By("expecting denial of UPDATE operation on managed namespace") + updateErr = notMasterUser.Update(ctx, &ns) if k8sErrors.IsConflict(updateErr) { return updateErr } return nil }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + expectDeniedByVAP(updateErr) + }) - err = notMasterUser.Delete(ctx, np) - expectDeniedByVAP(err) - err = notMasterUser.Delete(ctx, crp) - expectDeniedByVAP(err) + It("should allow UPDATE operation on managed namespace for system:masters user", func() { + var updateErr error + Eventually(func() error { + var ns corev1.Namespace + if err := sysMastersClient.Get(ctx, types.NamespacedName{Name: managedNS.Name}, &ns); err != nil { + return err + } + ns.Annotations = map[string]string{"test": "annotation"} + updateErr = sysMastersClient.Update(ctx, &ns) + if k8sErrors.IsConflict(updateErr) { + return updateErr + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + }) - err = sysMastersClient.Delete(ctx, rq) - Expect(err).To(BeNil(), "system:masters user should delete managed ResourceQuota") - err = sysMastersClient.Delete(ctx, np) - Expect(err).To(BeNil(), "system:masters user should delete managed NetworkPolicy") - err = sysMastersClient.Delete(ctx, crp) - Expect(err).To(BeNil(), "system:masters user should delete managed CRP") + Context("For other resources in scope", func() { + It("should deny creating managed resource quotas", func() { + rq := createManagedResourceQuota("default", "default") + err := notMasterUser.Create(ctx, rq) + expectDeniedByVAP(err) + }) + + It("should deny creating managed network policy", func() { + np := createManagedNetworkPolicy("default", "default") + err := notMasterUser.Create(ctx, np) + expectDeniedByVAP(err) + }) + + It("should deny creating managed CRP", func() { + crp := createManagedCRP("test-crp") + err := notMasterUser.Create(ctx, crp) + expectDeniedByVAP(err) + }) + + It("general expected behavior of other resources", func() { + rq := createManagedResourceQuota("default", "default") + np := createManagedNetworkPolicy("default", "default") + crp := createManagedCRP("test-crp") + err := sysMastersClient.Create(ctx, rq) + Expect(err).To(BeNil(), "system:masters user should create managed ResourceQuota") + err = sysMastersClient.Create(ctx, np) + Expect(err).To(BeNil(), "system:masters user should create managed NetworkPolicy") + err = sysMastersClient.Create(ctx, crp) + Expect(err).To(BeNil(), "system:masters user should create managed CRP") + + work := createManagedResourcePlacement("test-work") + err = notMasterUser.Create(ctx, work) + expectDeniedByVAP(err) + + var updateErr error + Eventually(func() error { + var urq corev1.ResourceQuota + if err := sysMastersClient.Get(ctx, types.NamespacedName{Name: "default", Namespace: "default"}, &urq); err != nil { + return err + } + urq.Annotations = map[string]string{"test": "annotation"} + By("expecting denial of UPDATE operation on managed resource quota") + updateErr = notMasterUser.Update(ctx, &urq) + if k8sErrors.IsConflict(updateErr) { + return updateErr + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + expectDeniedByVAP(updateErr) + + err = notMasterUser.Delete(ctx, np) + expectDeniedByVAP(err) + err = notMasterUser.Delete(ctx, crp) + expectDeniedByVAP(err) + + err = sysMastersClient.Delete(ctx, rq) + Expect(err).To(BeNil(), "system:masters user should delete managed ResourceQuota") + err = sysMastersClient.Delete(ctx, np) + Expect(err).To(BeNil(), "system:masters user should delete managed NetworkPolicy") + err = sysMastersClient.Delete(ctx, crp) + Expect(err).To(BeNil(), "system:masters user should delete managed CRP") + }) }) - }) - AfterAll(func() { - err := sysMastersClient.Delete(ctx, managedNS) - if err != nil { - Expect(k8sErrors.IsNotFound(err)).To(BeTrue()) - } + AfterAll(func() { + err := sysMastersClient.Delete(ctx, managedNS) + if err != nil { + Expect(k8sErrors.IsNotFound(err)).To(BeTrue()) + } + }) }) }) }) + +// isAPIServerVersionAtLeast checks if the API server version is >= targetVersion (e.g., "v1.30.0") +func isAPIServerVersionAtLeast(disco discovery.DiscoveryInterface, targetVersion string) (bool, error) { + serverVersion, err := disco.ServerVersion() + if err != nil { + return false, fmt.Errorf("failed to get server version: %w", err) + } + server, target := version.MustParseSemantic(serverVersion.GitVersion), version.MustParseSemantic(targetVersion) + return server.AtLeast(target), nil +} diff --git a/test/e2e/setup_test.go b/test/e2e/setup_test.go index d89fe02f1..4af6cd719 100644 --- a/test/e2e/setup_test.go +++ b/test/e2e/setup_test.go @@ -382,7 +382,7 @@ func beforeSuiteForAllProcesses() { }) // Expect that the managedResource VAP and its binding to exist when hub starts - checkVAPAndBindingExistence(hubClient) + checkVAPAndBindingExistence(hubCluster) } func maxDuration(a, b time.Duration) time.Duration { @@ -398,7 +398,7 @@ func beforeSuiteForProcess1() { setAllMemberClustersToJoin() checkIfAllMemberClustersHaveJoined() for _, c := range allMemberClusters { - checkVAPAndBindingExistence(c.KubeClient) + checkVAPAndBindingExistence(c) } checkIfAzurePropertyProviderIsWorking() @@ -419,7 +419,7 @@ var _ = SynchronizedAfterSuite(func() {}, func() { setAllMemberClustersToLeave() checkIfAllMemberClustersHaveLeft() for _, c := range allMemberClusters { - checkVAPAndBindingAbsence(c.KubeClient) + checkVAPAndBindingAbsence(c) } cleanupInvalidClusters() }) From 3a204710864073108ef773b0102a4fd08a2461d5 Mon Sep 17 00:00:00 2001 From: Nont Date: Mon, 29 Sep 2025 19:18:50 -0500 Subject: [PATCH 05/11] Avoid updating ResourceVersion Signed-off-by: Nont --- .../managedresource/validatingadmissionpolicy.go | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy.go b/pkg/webhook/managedresource/validatingadmissionpolicy.go index bb3dcb4a4..79905c83e 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy.go @@ -29,11 +29,8 @@ func mutateValidatingAdmissionPolicy(vap *admv1.ValidatingAdmissionPolicy, isHub APIVersion: "admissionregistration.k8s.io/v1", Kind: "ValidatingAdmissionPolicy", } - vap.ObjectMeta = metav1.ObjectMeta{ - Name: vap.ObjectMeta.Name, - Labels: map[string]string{ - "fleet.azure.com/managed-by": "arm", - }, + vap.ObjectMeta.Labels = map[string]string{ + "fleet.azure.com/managed-by": "arm", } vap.Spec = admv1.ValidatingAdmissionPolicySpec{ MatchConstraints: &admv1.MatchResources{ @@ -115,11 +112,8 @@ func mutateValidatingAdmissionPolicyBinding(vapb *admv1.ValidatingAdmissionPolic APIVersion: "admissionregistration.k8s.io/v1", Kind: "ValidatingAdmissionPolicyBinding", } - vapb.ObjectMeta = metav1.ObjectMeta{ - Name: vapb.ObjectMeta.Name, - Labels: map[string]string{ - "fleet.azure.com/managed-by": "arm", - }, + vapb.ObjectMeta.Labels = map[string]string{ + "fleet.azure.com/managed-by": "arm", } vapb.Spec = admv1.ValidatingAdmissionPolicyBindingSpec{ PolicyName: resourceName, From 61661752af093aff9dff759542c5d4827ee6e290 Mon Sep 17 00:00:00 2001 From: Nont Date: Mon, 29 Sep 2025 19:30:39 -0500 Subject: [PATCH 06/11] Preserve ObjectMeta.ResourceVersion Signed-off-by: Nont --- .../validatingadmissionpolicy.go | 24 +-- .../validatingadmissionpolicy_test.go | 170 +++++++++++++++++- 2 files changed, 174 insertions(+), 20 deletions(-) diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy.go b/pkg/webhook/managedresource/validatingadmissionpolicy.go index 79905c83e..99c576c01 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy.go @@ -25,13 +25,13 @@ func getValidatingAdmissionPolicy(isHub bool) *admv1.ValidatingAdmissionPolicy { } func mutateValidatingAdmissionPolicy(vap *admv1.ValidatingAdmissionPolicy, isHub bool) { - vap.TypeMeta = metav1.TypeMeta{ - APIVersion: "admissionregistration.k8s.io/v1", - Kind: "ValidatingAdmissionPolicy", - } - vap.ObjectMeta.Labels = map[string]string{ - "fleet.azure.com/managed-by": "arm", + ometa := metav1.ObjectMeta{ + Labels: map[string]string{ + "fleet.azure.com/managed-by": "arm", + }, + ResourceVersion: vap.ResourceVersion, } + vap.ObjectMeta = ometa vap.Spec = admv1.ValidatingAdmissionPolicySpec{ MatchConstraints: &admv1.MatchResources{ ObjectSelector: &metav1.LabelSelector{ @@ -108,13 +108,13 @@ func getValidatingAdmissionPolicyBinding() *admv1.ValidatingAdmissionPolicyBindi } func mutateValidatingAdmissionPolicyBinding(vapb *admv1.ValidatingAdmissionPolicyBinding) { - vapb.TypeMeta = metav1.TypeMeta{ - APIVersion: "admissionregistration.k8s.io/v1", - Kind: "ValidatingAdmissionPolicyBinding", - } - vapb.ObjectMeta.Labels = map[string]string{ - "fleet.azure.com/managed-by": "arm", + ometa := metav1.ObjectMeta{ + Labels: map[string]string{ + "fleet.azure.com/managed-by": "arm", + }, + ResourceVersion: vapb.ResourceVersion, } + vapb.ObjectMeta = ometa vapb.Spec = admv1.ValidatingAdmissionPolicyBindingSpec{ PolicyName: resourceName, ValidationActions: []admv1.ValidationAction{ diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy_test.go b/pkg/webhook/managedresource/validatingadmissionpolicy_test.go index ca51767ac..f96d98f71 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy_test.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy_test.go @@ -8,8 +8,9 @@ package managedresource import ( "testing" - "github.com/stretchr/testify/assert" + "github.com/google/go-cmp/cmp" admv1 "k8s.io/api/admissionregistration/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestGetValidatingAdmissionPolicy(t *testing.T) { @@ -19,8 +20,11 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { t.Parallel() vap := getValidatingAdmissionPolicy(false) - assert.NotNil(t, vap) - assert.NotContains(t, vap.Spec.MatchConstraints.ResourceRules, admv1.NamedRuleWithOperations{ + if vap == nil { + t.Errorf("getValidatingAdmissionPolicy(false) = nil, want non-nil") + } + + unwantedRule := admv1.NamedRuleWithOperations{ RuleWithOperations: admv1.RuleWithOperations{ Rule: admv1.Rule{ APIGroups: []string{"placement.kubernetes-fleet.io"}, @@ -29,15 +33,24 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { }, Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, }, - }) + } + + for _, rule := range vap.Spec.MatchConstraints.ResourceRules { + if diff := cmp.Diff(unwantedRule, rule); diff == "" { + t.Errorf("getValidatingAdmissionPolicy(false) contains unwanted rule %+v", unwantedRule) + } + } }) t.Run("hub", func(t *testing.T) { t.Parallel() vap := getValidatingAdmissionPolicy(true) - assert.NotNil(t, vap) - assert.Contains(t, vap.Spec.MatchConstraints.ResourceRules, admv1.NamedRuleWithOperations{ + if vap == nil { + t.Errorf("getValidatingAdmissionPolicy(true) = nil, want non-nil") + } + + wantedRule := admv1.NamedRuleWithOperations{ RuleWithOperations: admv1.RuleWithOperations{ Rule: admv1.Rule{ APIGroups: []string{"placement.kubernetes-fleet.io"}, @@ -46,13 +59,154 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { }, Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, }, - }) + } + + found := false + for _, rule := range vap.Spec.MatchConstraints.ResourceRules { + if diff := cmp.Diff(wantedRule, rule); diff == "" { + found = true + break + } + } + if !found { + t.Errorf("getValidatingAdmissionPolicy(true) missing expected rule %+v", wantedRule) + } }) } +func TestMutateValidatingAdmissionPolicy(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + isHub bool + resourceVersion string + initialLabels map[string]string + }{ + { + name: "preserves ResourceVersion and updates labels for member cluster", + isHub: false, + resourceVersion: "12345", + initialLabels: map[string]string{"existing": "label"}, + }, + { + name: "preserves ResourceVersion and updates labels for hub cluster", + isHub: true, + resourceVersion: "67890", + initialLabels: map[string]string{"old": "value"}, + }, + { + name: "preserves empty ResourceVersion and sets labels", + isHub: false, + resourceVersion: "", + initialLabels: nil, + }, + { + name: "overwrites existing managed label while preserving ResourceVersion", + isHub: false, + resourceVersion: "54321", + initialLabels: map[string]string{"fleet.azure.com/managed-by": "old-value", "other": "label"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + vap := &admv1.ValidatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-policy", + ResourceVersion: tt.resourceVersion, + Labels: tt.initialLabels, + }, + } + + mutateValidatingAdmissionPolicy(vap, tt.isHub) + + if vap.ResourceVersion != tt.resourceVersion { + t.Errorf("mutateValidatingAdmissionPolicy() ResourceVersion = %v, want %v", vap.ResourceVersion, tt.resourceVersion) + } + + wantManagedByLabel := "arm" + if got := vap.Labels["fleet.azure.com/managed-by"]; got != wantManagedByLabel { + t.Errorf("mutateValidatingAdmissionPolicy() managed-by label = %v, want %v", got, wantManagedByLabel) + } + + // Verify that only the managed-by label exists (other labels are not preserved) + wantLabels := map[string]string{ + "fleet.azure.com/managed-by": "arm", + } + if diff := cmp.Diff(wantLabels, vap.Labels); diff != "" { + t.Errorf("mutateValidatingAdmissionPolicy() labels mismatch (-want +got):\n%s", diff) + } + }) + } +} + func TestGetValidatingAdmissionPolicyBinding(t *testing.T) { t.Parallel() vap := getValidatingAdmissionPolicyBinding() - assert.NotNil(t, vap) + if vap == nil { + t.Errorf("getValidatingAdmissionPolicyBinding() = nil, want non-nil") + } +} + +func TestMutateValidatingAdmissionPolicyBinding(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + resourceVersion string + initialLabels map[string]string + }{ + { + name: "preserves ResourceVersion and updates labels", + resourceVersion: "12345", + initialLabels: map[string]string{"existing": "label"}, + }, + { + name: "preserves empty ResourceVersion and sets labels", + resourceVersion: "", + initialLabels: nil, + }, + { + name: "overwrites existing managed label while preserving ResourceVersion", + resourceVersion: "67890", + initialLabels: map[string]string{"fleet.azure.com/managed-by": "old-value", "other": "label"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + vapb := &admv1.ValidatingAdmissionPolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-binding", + ResourceVersion: tt.resourceVersion, + Labels: tt.initialLabels, + }, + } + + mutateValidatingAdmissionPolicyBinding(vapb) + + if vapb.ResourceVersion != tt.resourceVersion { + t.Errorf("mutateValidatingAdmissionPolicyBinding() ResourceVersion = %v, want %v", vapb.ResourceVersion, tt.resourceVersion) + } + + wantManagedByLabel := "arm" + if got := vapb.Labels["fleet.azure.com/managed-by"]; got != wantManagedByLabel { + t.Errorf("mutateValidatingAdmissionPolicyBinding() managed-by label = %v, want %v", got, wantManagedByLabel) + } + + // Verify that only the managed-by label exists (other labels are not preserved) + wantLabels := map[string]string{ + "fleet.azure.com/managed-by": "arm", + } + if diff := cmp.Diff(wantLabels, vapb.Labels); diff != "" { + t.Errorf("mutateValidatingAdmissionPolicyBinding() labels mismatch (-want +got):\n%s", diff) + } + }) + } } From 8541c2a59c125ee23b1297be3f22332c73f3115a Mon Sep 17 00:00:00 2001 From: Nont Date: Mon, 29 Sep 2025 19:44:37 -0500 Subject: [PATCH 07/11] Fix lint Signed-off-by: Nont --- .../validatingadmissionpolicy_test.go | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy_test.go b/pkg/webhook/managedresource/validatingadmissionpolicy_test.go index f96d98f71..935085076 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy_test.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy_test.go @@ -20,9 +20,6 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { t.Parallel() vap := getValidatingAdmissionPolicy(false) - if vap == nil { - t.Errorf("getValidatingAdmissionPolicy(false) = nil, want non-nil") - } unwantedRule := admv1.NamedRuleWithOperations{ RuleWithOperations: admv1.RuleWithOperations{ @@ -34,10 +31,12 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, }, } - - for _, rule := range vap.Spec.MatchConstraints.ResourceRules { - if diff := cmp.Diff(unwantedRule, rule); diff == "" { - t.Errorf("getValidatingAdmissionPolicy(false) contains unwanted rule %+v", unwantedRule) + + if vap.Spec.MatchConstraints != nil { + for _, rule := range vap.Spec.MatchConstraints.ResourceRules { + if diff := cmp.Diff(unwantedRule, rule); diff == "" { + t.Errorf("getValidatingAdmissionPolicy(false) contains unwanted rule %+v", unwantedRule) + } } } }) @@ -46,9 +45,6 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { t.Parallel() vap := getValidatingAdmissionPolicy(true) - if vap == nil { - t.Errorf("getValidatingAdmissionPolicy(true) = nil, want non-nil") - } wantedRule := admv1.NamedRuleWithOperations{ RuleWithOperations: admv1.RuleWithOperations{ @@ -60,12 +56,14 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, }, } - + found := false - for _, rule := range vap.Spec.MatchConstraints.ResourceRules { - if diff := cmp.Diff(wantedRule, rule); diff == "" { - found = true - break + if vap.Spec.MatchConstraints != nil { + for _, rule := range vap.Spec.MatchConstraints.ResourceRules { + if diff := cmp.Diff(wantedRule, rule); diff == "" { + found = true + break + } } } if !found { @@ -78,10 +76,10 @@ func TestMutateValidatingAdmissionPolicy(t *testing.T) { t.Parallel() tests := []struct { - name string - isHub bool - resourceVersion string - initialLabels map[string]string + name string + isHub bool + resourceVersion string + initialLabels map[string]string }{ { name: "preserves ResourceVersion and updates labels for member cluster", @@ -131,7 +129,7 @@ func TestMutateValidatingAdmissionPolicy(t *testing.T) { if got := vap.Labels["fleet.azure.com/managed-by"]; got != wantManagedByLabel { t.Errorf("mutateValidatingAdmissionPolicy() managed-by label = %v, want %v", got, wantManagedByLabel) } - + // Verify that only the managed-by label exists (other labels are not preserved) wantLabels := map[string]string{ "fleet.azure.com/managed-by": "arm", @@ -156,9 +154,9 @@ func TestMutateValidatingAdmissionPolicyBinding(t *testing.T) { t.Parallel() tests := []struct { - name string - resourceVersion string - initialLabels map[string]string + name string + resourceVersion string + initialLabels map[string]string }{ { name: "preserves ResourceVersion and updates labels", @@ -199,7 +197,7 @@ func TestMutateValidatingAdmissionPolicyBinding(t *testing.T) { if got := vapb.Labels["fleet.azure.com/managed-by"]; got != wantManagedByLabel { t.Errorf("mutateValidatingAdmissionPolicyBinding() managed-by label = %v, want %v", got, wantManagedByLabel) } - + // Verify that only the managed-by label exists (other labels are not preserved) wantLabels := map[string]string{ "fleet.azure.com/managed-by": "arm", From 46bb1799a69e42a0c29861593267b4ff584aeca4 Mon Sep 17 00:00:00 2001 From: Nont Date: Wed, 1 Oct 2025 12:13:50 -0500 Subject: [PATCH 08/11] Fix name gets overridden Signed-off-by: Nont --- .../managedresource/validatingadmissionpolicy.go | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy.go b/pkg/webhook/managedresource/validatingadmissionpolicy.go index 99c576c01..f71fbfae5 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy.go @@ -15,17 +15,14 @@ const resourceName = "aks-fleet-managed-by-arm" var forbidden = metav1.StatusReasonForbidden func getValidatingAdmissionPolicy(isHub bool) *admv1.ValidatingAdmissionPolicy { - vap := &admv1.ValidatingAdmissionPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: resourceName, - }, - } + vap := &admv1.ValidatingAdmissionPolicy{} mutateValidatingAdmissionPolicy(vap, isHub) return vap } func mutateValidatingAdmissionPolicy(vap *admv1.ValidatingAdmissionPolicy, isHub bool) { ometa := metav1.ObjectMeta{ + Name: resourceName, Labels: map[string]string{ "fleet.azure.com/managed-by": "arm", }, @@ -98,17 +95,14 @@ func mutateValidatingAdmissionPolicy(vap *admv1.ValidatingAdmissionPolicy, isHub } func getValidatingAdmissionPolicyBinding() *admv1.ValidatingAdmissionPolicyBinding { - vapb := &admv1.ValidatingAdmissionPolicyBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: resourceName, - }, - } + vapb := &admv1.ValidatingAdmissionPolicyBinding{} mutateValidatingAdmissionPolicyBinding(vapb) return vapb } func mutateValidatingAdmissionPolicyBinding(vapb *admv1.ValidatingAdmissionPolicyBinding) { ometa := metav1.ObjectMeta{ + Name: resourceName, Labels: map[string]string{ "fleet.azure.com/managed-by": "arm", }, From d38f3a7517a17903b785a824b88e6579a52eadef Mon Sep 17 00:00:00 2001 From: Nont Date: Thu, 2 Oct 2025 09:56:17 -0500 Subject: [PATCH 09/11] Remove IsNotFound error check Signed-off-by: Nont --- pkg/webhook/managedresource/createordelete.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/webhook/managedresource/createordelete.go b/pkg/webhook/managedresource/createordelete.go index b2103b0ea..95e0b0bad 100644 --- a/pkg/webhook/managedresource/createordelete.go +++ b/pkg/webhook/managedresource/createordelete.go @@ -55,7 +55,7 @@ func EnsureVAP(ctx context.Context, c client.Client, isHub bool) error { for _, objectMutator := range objsAndMutators { opResult, err := controllerutil.CreateOrUpdate(ctx, c, objectMutator.obj, objectMutator.mutate) switch { - case err == nil, apierrors.IsNotFound(err): + case err == nil: // continue case meta.IsNoMatchError(err): klog.Infof("object type %T is not supported in this cluster, continuing", objectMutator.obj) From 203407e22bec5ceb78bf81f68f69e8bebb06b3e9 Mon Sep 17 00:00:00 2001 From: Nont Date: Thu, 2 Oct 2025 15:18:02 -0500 Subject: [PATCH 10/11] Add explanation for complex test logics and TODO for refactoring Signed-off-by: Nont --- pkg/webhook/managedresource/createordelete.go | 1 + pkg/webhook/managedresource/createordelete_test.go | 2 ++ pkg/webhook/managedresource/validatingadmissionpolicy.go | 2 ++ test/e2e/setup_test.go | 2 ++ 4 files changed, 7 insertions(+) diff --git a/pkg/webhook/managedresource/createordelete.go b/pkg/webhook/managedresource/createordelete.go index 95e0b0bad..206a01b13 100644 --- a/pkg/webhook/managedresource/createordelete.go +++ b/pkg/webhook/managedresource/createordelete.go @@ -39,6 +39,7 @@ func EnsureVAP(ctx context.Context, c client.Client, isHub bool) error { obj client.Object mutate func() error } + // TODO: this can be simplified by dealing with the specific type rather than using client.Object vap, mutateVAP := getVAPWithMutator(isHub) vapb, mutateVAPB := getVAPBindingWithMutator() objsAndMutators := []vapObjectAndMutator{ diff --git a/pkg/webhook/managedresource/createordelete_test.go b/pkg/webhook/managedresource/createordelete_test.go index 07f0caf6d..6339555d6 100644 --- a/pkg/webhook/managedresource/createordelete_test.go +++ b/pkg/webhook/managedresource/createordelete_test.go @@ -127,6 +127,8 @@ func TestEnsureVAP(t *testing.T) { } interceptorFuncs := interceptor.Funcs{ + // This is needed for a test scenario that GET would retrieve the same object as what was created/updated. + // TODO: refactor to simplify this. The store should not be needed as UPDATE should update the same object provided in fakeClient's WithObjects Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { if storedObj, exists := objectStore[key]; exists { switch v := obj.(type) { diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy.go b/pkg/webhook/managedresource/validatingadmissionpolicy.go index f71fbfae5..e8f5887b1 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy.go @@ -10,6 +10,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// TODO use a differnt name for binding to simplify the code +// and add a migration path const resourceName = "aks-fleet-managed-by-arm" var forbidden = metav1.StatusReasonForbidden diff --git a/test/e2e/setup_test.go b/test/e2e/setup_test.go index 4af6cd719..05bffea25 100644 --- a/test/e2e/setup_test.go +++ b/test/e2e/setup_test.go @@ -397,6 +397,7 @@ func beforeSuiteForProcess1() { setAllMemberClustersToJoin() checkIfAllMemberClustersHaveJoined() + // TODO: add checks for VAP and its binding existence in Join Leave scenarios for _, c := range allMemberClusters { checkVAPAndBindingExistence(c) } @@ -418,6 +419,7 @@ var _ = SynchronizedAfterSuite(func() {}, func() { deleteTestResourceCRD() setAllMemberClustersToLeave() checkIfAllMemberClustersHaveLeft() + // TODO: add checks for VAP and its binding absence in Join Leave scenarios for _, c := range allMemberClusters { checkVAPAndBindingAbsence(c) } From f0ed0f402c87c7052c497631ac668f8e6f987de3 Mon Sep 17 00:00:00 2001 From: Nont Date: Thu, 2 Oct 2025 16:18:19 -0500 Subject: [PATCH 11/11] Fix comments Signed-off-by: Nont --- pkg/webhook/managedresource/createordelete_test.go | 10 +++++----- .../managedresource/validatingadmissionpolicy.go | 2 +- test/e2e/managed_resource_vap_test.go | 1 - 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/pkg/webhook/managedresource/createordelete_test.go b/pkg/webhook/managedresource/createordelete_test.go index 6339555d6..5609e613f 100644 --- a/pkg/webhook/managedresource/createordelete_test.go +++ b/pkg/webhook/managedresource/createordelete_test.go @@ -377,17 +377,17 @@ func TestGetVAPWithMutator(t *testing.T) { } // Verify mutate function works - originalVAP := vap.DeepCopy() - expectedVAP := getValidatingAdmissionPolicy(tt.isHub) + gotVAP := vap.DeepCopy() + wantVAP := getValidatingAdmissionPolicy(tt.isHub) ignoreOpts := cmp.Options{ cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation"), } - if diff := cmp.Diff(expectedVAP, vap, ignoreOpts); diff != "" { + if diff := cmp.Diff(wantVAP, vap, ignoreOpts); diff != "" { t.Errorf("VAP after mutation mismatch (-want +got):\n%s", diff) } vap.Spec = admv1.ValidatingAdmissionPolicySpec{} // Reset spec to empty to test idempotency - if diff := cmp.Diff(originalVAP, vap); diff == "" { + if diff := cmp.Diff(gotVAP, vap); diff == "" { t.Error("VAP should be different after mutation") } @@ -396,7 +396,7 @@ func TestGetVAPWithMutator(t *testing.T) { if err != nil { t.Errorf("second mutateFunc() = %v, want nil", err) } - if diff := cmp.Diff(expectedVAP, vap, ignoreOpts); diff != "" { + if diff := cmp.Diff(wantVAP, vap, ignoreOpts); diff != "" { t.Errorf("VAP after second mutation mismatch (-want +got):\n%s", diff) } }) diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy.go b/pkg/webhook/managedresource/validatingadmissionpolicy.go index e8f5887b1..e0835507a 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy.go @@ -10,7 +10,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -// TODO use a differnt name for binding to simplify the code +// TODO use a different name for binding to simplify the code // and add a migration path const resourceName = "aks-fleet-managed-by-arm" diff --git a/test/e2e/managed_resource_vap_test.go b/test/e2e/managed_resource_vap_test.go index 3bed3506e..ca2c963cd 100644 --- a/test/e2e/managed_resource_vap_test.go +++ b/test/e2e/managed_resource_vap_test.go @@ -189,7 +189,6 @@ func checkVAPAndBindingAbsence(c *framework.Cluster) { } var _ = Describe("ValidatingAdmissionPolicy for Managed Resources", Label("managedresource"), Ordered, func() { - Context("Version-agnostic tests", func() { It("should allow operations on unmanaged namespace for non-system:masters user", func() { unmanagedNS := createUnmanagedNamespace("test-unmanaged-ns")