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/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 572653796..1fa081ee2 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 ) @@ -210,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 23ed70898..c05af5086 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,10 @@ 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 + } + klog.V(2).InfoS("Successfully installed managed resource validating admission policy") if err := r.startAgents(ctx, &imc); err != nil { return ctrl.Result{}, err } @@ -242,6 +247,10 @@ 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 + } + 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/pkg/webhook/managedresource/createordelete.go b/pkg/webhook/managedresource/createordelete.go new file mode 100644 index 000000000..206a01b13 --- /dev/null +++ b/pkg/webhook/managedresource/createordelete.go @@ -0,0 +1,86 @@ +/* +Copyright (c) Microsoft Corporation. +Licensed under the MIT license. +*/ + +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" + "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) + 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 + } + } + return nil +} + +func EnsureVAP(ctx context.Context, c client.Client, isHub bool) error { + type vapObjectAndMutator struct { + 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{ + { + 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: + // 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 new file mode 100644 index 000000000..5609e613f --- /dev/null +++ b/pkg/webhook/managedresource/createordelete_test.go @@ -0,0 +1,447 @@ +/* +Copyright (c) Microsoft Corporation. +Licensed under the MIT license. +*/ + +package managedresource + +import ( + "context" + "errors" + "strings" + "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/client/interceptor" +) + +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: 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(), + binding.DeepCopy(), + }, + }, + { + 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: false, + }, + { + 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", + 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 { + 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{ + // 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) { + 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 + } + 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 + } + // Update our store with the new object state + objectStore[key] = obj.DeepCopyObject().(client.Object) + return nil + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(tt.existingObjs...). + WithInterceptorFuncs(interceptorFuncs). + Build() + + err := EnsureVAP(context.Background(), fakeClient, tt.isHub) + + if tt.wantErr { + if err == nil { + t.Error("EnsureVAP() = nil, want error") + return + } + 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("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 := 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) + } + } + }) + } +} + +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", + 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 { + t.Run(tt.name, func(t *testing.T) { + tt := tt + 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). + Build() + + err := EnsureNoVAP(context.Background(), fakeClient, tt.isHub) + + if tt.wantErr { + if err == nil { + t.Error("EnsureNoVAP() = nil, want error") + return + } + 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("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 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 + gotVAP := vap.DeepCopy() + wantVAP := getValidatingAdmissionPolicy(tt.isHub) + ignoreOpts := cmp.Options{ + cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation"), + } + 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(gotVAP, 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(wantVAP, 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..e0835507a 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy.go @@ -10,67 +10,74 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// TODO use a different name for binding to simplify the code +// and add a migration path const resourceName = "aks-fleet-managed-by-arm" var forbidden = metav1.StatusReasonForbidden -func GetValidatingAdmissionPolicy(isHub bool) *admv1.ValidatingAdmissionPolicy { - vap := &admv1.ValidatingAdmissionPolicy{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "admissionregistration.k8s.io/v1", - Kind: "ValidatingAdmissionPolicy", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: resourceName, +func getValidatingAdmissionPolicy(isHub bool) *admv1.ValidatingAdmissionPolicy { + 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", }, - Spec: admv1.ValidatingAdmissionPolicySpec{ - MatchConstraints: &admv1.MatchResources{ - ObjectSelector: &metav1.LabelSelector{ - MatchLabels: 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{ + 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 +94,27 @@ func GetValidatingAdmissionPolicy(isHub bool) *admv1.ValidatingAdmissionPolicy { }, }) } +} - return vap +func getValidatingAdmissionPolicyBinding() *admv1.ValidatingAdmissionPolicyBinding { + vapb := &admv1.ValidatingAdmissionPolicyBinding{} + mutateValidatingAdmissionPolicyBinding(vapb) + return vapb } -func GetValidatingAdmissionPolicyBinding() *admv1.ValidatingAdmissionPolicyBinding { - return &admv1.ValidatingAdmissionPolicyBinding{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "admissionregistration.k8s.io/v1", - Kind: "ValidatingAdmissionPolicyBinding", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: resourceName, +func mutateValidatingAdmissionPolicyBinding(vapb *admv1.ValidatingAdmissionPolicyBinding) { + ometa := metav1.ObjectMeta{ + Name: resourceName, + Labels: map[string]string{ + "fleet.azure.com/managed-by": "arm", }, - Spec: admv1.ValidatingAdmissionPolicyBindingSpec{ - PolicyName: resourceName, - ValidationActions: []admv1.ValidationAction{ - admv1.Deny, - }, + ResourceVersion: vapb.ResourceVersion, + } + vapb.ObjectMeta = ometa + 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..935085076 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) { @@ -18,9 +19,9 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { t.Run("member", func(t *testing.T) { t.Parallel() - vap := GetValidatingAdmissionPolicy(false) - assert.NotNil(t, vap) - assert.NotContains(t, vap.Spec.MatchConstraints.ResourceRules, admv1.NamedRuleWithOperations{ + vap := getValidatingAdmissionPolicy(false) + + unwantedRule := admv1.NamedRuleWithOperations{ RuleWithOperations: admv1.RuleWithOperations{ Rule: admv1.Rule{ APIGroups: []string{"placement.kubernetes-fleet.io"}, @@ -29,15 +30,23 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { }, Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, }, - }) + } + + 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) + } + } + } }) 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{ + vap := getValidatingAdmissionPolicy(true) + + wantedRule := admv1.NamedRuleWithOperations{ RuleWithOperations: admv1.RuleWithOperations{ Rule: admv1.Rule{ APIGroups: []string{"placement.kubernetes-fleet.io"}, @@ -46,13 +55,156 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { }, Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, }, - }) + } + + found := false + if vap.Spec.MatchConstraints != nil { + 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) + vap := getValidatingAdmissionPolicyBinding() + 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) + } + }) + } } 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 5bbd641c3..ca2c963cd 100644 --- a/test/e2e/managed_resource_vap_test.go +++ b/test/e2e/managed_resource_vap_test.go @@ -27,16 +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" + "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{ @@ -126,176 +131,247 @@ 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") - - var vapBinding admissionregistrationv1.ValidatingAdmissionPolicyBinding - Expect(sysMastersClient.Get(ctx, types.NamespacedName{Name: vapName}, &vapBinding)).Should(Succeed(), "ValidatingAdmissionPolicyBinding should be installed") - }) - - 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()) - }) - }) +func checkVAPAndBindingExistence(c *framework.Cluster) { + client := c.KubeClient + discoveryClient := framework.GetDiscoveryClient(c) + + serverVersionWithVAP, err := isAPIServerVersionAtLeast(discoveryClient, k8sVersionWithVAP) + Expect(err).To(BeNil(), "Error checking API server version") + + 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()) + } +} - 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)) - }) +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) + Expect(k8sErrors.IsNotFound(err)).To(BeTrue(), + fmt.Sprintf("ValidatingAdmissionPolicy %s should not exist", 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(), + fmt.Sprintf("ValidatingAdmissionPolicyBinding %s should not exist", vapName)) +} - 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) - }) +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") + By("expecting successful CREATE operation on unmanaged namespace") + Expect(notMasterUser.Create(ctx, unmanagedNS)).To(Succeed()) - 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 4b2c8a302..05bffea25 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(hubCluster) } func maxDuration(a, b time.Duration) time.Duration { @@ -394,6 +397,11 @@ func beforeSuiteForProcess1() { setAllMemberClustersToJoin() checkIfAllMemberClustersHaveJoined() + // TODO: add checks for VAP and its binding existence in Join Leave scenarios + for _, c := range allMemberClusters { + checkVAPAndBindingExistence(c) + } + checkIfAzurePropertyProviderIsWorking() // Simulate that member cluster 4 become unhealthy, and member cluster 5 has left the fleet. @@ -411,5 +419,9 @@ 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) + } cleanupInvalidClusters() })