From 2f8bc0c8606c005925b6d7d7b4dca96f8d3f4251 Mon Sep 17 00:00:00 2001 From: Nont Date: Wed, 20 Aug 2025 23:02:42 -0500 Subject: [PATCH 1/8] Add ValidatingAdmissionPolicy for managedresource Signed-off-by: Nont --- cmd/crdinstaller/main.go | 6 + cmd/crdinstaller/utils/util.go | 54 +++- cmd/crdinstaller/utils/util_test.go | 231 ++++++++++++++++++ .../validatingadmissionpolicy.go | 104 ++++++++ .../validatingadmissionpolicy_test.go | 53 ++++ 5 files changed, 443 insertions(+), 5 deletions(-) create mode 100644 pkg/webhook/managedresource/validatingadmissionpolicy.go create mode 100644 pkg/webhook/managedresource/validatingadmissionpolicy_test.go diff --git a/cmd/crdinstaller/main.go b/cmd/crdinstaller/main.go index 0ac527d3c..7211b6fec 100644 --- a/cmd/crdinstaller/main.go +++ b/cmd/crdinstaller/main.go @@ -70,6 +70,12 @@ func main() { } klog.Infof("Successfully installed %s CRDs", *mode) + + if err := utils.InstallManagedResourceVAP(ctx, client, *mode); err != nil { + klog.Fatalf("Failed to install managed resource ValidatingAdmissionPolicy: %v", err) + } + + 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 000195437..8c4ba1ad9 100644 --- a/cmd/crdinstaller/utils/util.go +++ b/cmd/crdinstaller/utils/util.go @@ -15,12 +15,16 @@ 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/schema" "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 ( @@ -51,7 +55,7 @@ func InstallCRD(ctx context.Context, client client.Client, crd *apiextensionsv1. }, } - createOrUpdateRes, err := controllerutil.CreateOrUpdate(ctx, client, &existingCRD, func() error { + mutFn := func() error { // Copy spec from our decoded CRD to the object we're creating/updating. existingCRD.Spec = crd.Spec @@ -65,14 +69,20 @@ func InstallCRD(ctx context.Context, client client.Client, crd *apiextensionsv1. // needed for clean up of CRD by kube-addon-manager. existingCRD.Labels[AzureManagedLabelKey] = FleetLabelValue return nil - }) + } + err := install(ctx, client, &existingCRD, mutFn) + if err == nil { + klog.Infof("Successfully created/updated CRD %s", crd.Name) + } + return err +} +func install(ctx context.Context, client client.Client, obj client.Object, mut func() error) error { + result, err := controllerutil.CreateOrUpdate(ctx, client, obj, mut) if err != nil { - klog.ErrorS(err, "Failed to create or update CRD", "name", crd.Name, "operation", createOrUpdateRes) + klog.ErrorS(err, "Failed to create or update", "kind", obj.GetObjectKind().GroupVersionKind().Kind, obj.GetName(), "operation", result) return err } - - klog.Infof("Successfully created/updated CRD %s", crd.Name) return nil } @@ -161,3 +171,37 @@ 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() + + ok, err := checkResourceSupport(c, vap.GroupVersionKind()) + if err != nil { + return err + } + if !ok { // the cluster doesn't support VAP + return nil // not installing + } + + for _, ob := range []client.Object{vap, vapBinding} { + if err = install(ctx, c, ob, nil); err != nil { + return err + } + } + klog.Infof("Successfully installed managed resource ValidatingAdmissionPolicy") + return nil +} + +func checkResourceSupport(client client.Client, gvk schema.GroupVersionKind) (bool, error) { + restMapper := client.RESTMapper() + + _, err := restMapper.RESTMapping(gvk.GroupKind(), gvk.Version) + if err != nil { + if meta.IsNoMatchError(err) { + return false, nil + } + return false, err + } + return true, nil +} diff --git a/cmd/crdinstaller/utils/util_test.go b/cmd/crdinstaller/utils/util_test.go index 718d04248..db4156c21 100644 --- a/cmd/crdinstaller/utils/util_test.go +++ b/cmd/crdinstaller/utils/util_test.go @@ -7,13 +7,22 @@ Licensed under the MIT license. package utils import ( + "context" "os" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + 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" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ) var ( @@ -114,3 +123,225 @@ func runTest(t *testing.T, crdPath string) { }) } } + +func TestInstallCRD(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, apiextensionsv1.AddToScheme(scheme)) + + testCRD := &apiextensionsv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test.example.com", + }, + Spec: apiextensionsv1.CustomResourceDefinitionSpec{ + Group: "example.com", + Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ + { + Name: "v1", + Served: true, + Storage: true, + Schema: &apiextensionsv1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + }, + }, + }, + }, + Scope: apiextensionsv1.NamespaceScoped, + Names: apiextensionsv1.CustomResourceDefinitionNames{ + Plural: "tests", + Singular: "test", + Kind: "Test", + }, + }, + } + + tests := []struct { + name string + crd *apiextensionsv1.CustomResourceDefinition + wantError bool + }{ + { + name: "successful CRD installation", + crd: testCRD, + wantError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() + err := InstallCRD(context.Background(), fakeClient, tt.crd) + + if tt.wantError { + assert.Error(t, err) + return + } + + assert.NoError(t, err) + + var installedCRD apiextensionsv1.CustomResourceDefinition + err = fakeClient.Get(context.Background(), types.NamespacedName{Name: tt.crd.Name}, &installedCRD) + assert.NoError(t, err) + + assert.Equal(t, "true", installedCRD.Labels[CRDInstallerLabelKey]) + assert.Equal(t, FleetLabelValue, installedCRD.Labels[AzureManagedLabelKey]) + assert.Equal(t, tt.crd.Spec, installedCRD.Spec) + }) + } +} + +func TestInstall(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, apiextensionsv1.AddToScheme(scheme)) + + testCRD := &apiextensionsv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test.example.com", + }, + Spec: apiextensionsv1.CustomResourceDefinitionSpec{ + Group: "example.com", + Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ + { + Name: "v1", + Served: true, + Storage: true, + Schema: &apiextensionsv1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + }, + }, + }, + }, + Scope: apiextensionsv1.NamespaceScoped, + Names: apiextensionsv1.CustomResourceDefinitionNames{ + Plural: "tests", + Singular: "test", + Kind: "Test", + }, + }, + } + + tests := []struct { + name string + obj client.Object + mutFunc func() error + wantError bool + }{ + { + name: "successful install with mutation", + obj: testCRD, + mutFunc: func() error { + if testCRD.Labels == nil { + testCRD.Labels = make(map[string]string) + } + testCRD.Labels["test"] = "value" + return nil + }, + wantError: false, + }, + { + name: "successful install without mutation", + obj: &apiextensionsv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test2.example.com", + }, + Spec: testCRD.Spec, + }, + mutFunc: func() error { + return nil + }, + wantError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() + err := install(context.Background(), fakeClient, tt.obj, tt.mutFunc) + + if tt.wantError { + assert.Error(t, err) + return + } + + assert.NoError(t, err) + + var installed apiextensionsv1.CustomResourceDefinition + err = fakeClient.Get(context.Background(), types.NamespacedName{Name: tt.obj.GetName()}, &installed) + assert.NoError(t, err) + + if tt.mutFunc != nil && tt.obj.GetName() == "test.example.com" { + assert.Equal(t, "value", installed.Labels["test"]) + } + }) + } +} + +func TestCheckResourceSupport(t *testing.T) { + tests := []struct { + name string + gvk schema.GroupVersionKind + }{ + { + name: "VAP resource check", + gvk: schema.GroupVersionKind{ + Group: "admissionregistration.k8s.io", + Version: "v1", + Kind: "ValidatingAdmissionPolicy", + }, + }, + { + name: "nonexistent resource check", + gvk: schema.GroupVersionKind{ + Group: "nonexistent.io", + Version: "v1", + Kind: "NonExistentResource", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, admv1.AddToScheme(scheme)) + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() + supported, err := checkResourceSupport(fakeClient, tt.gvk) + + // The function should not panic and should return a boolean result + assert.NoError(t, err) + assert.IsType(t, false, supported) + }) + } +} + +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() + require.NoError(t, admv1.AddToScheme(scheme)) + + 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 + assert.NoError(t, err) + }) + } +} diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy.go b/pkg/webhook/managedresource/validatingadmissionpolicy.go new file mode 100644 index 000000000..76089a355 --- /dev/null +++ b/pkg/webhook/managedresource/validatingadmissionpolicy.go @@ -0,0 +1,104 @@ +/* +Copyright (c) Microsoft Corporation. +Licensed under the MIT license. +*/ + +package managedresource + +import ( + admv1 "k8s.io/api/admissionregistration/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func GetValidatingAdmissionPolicy(isHub bool) *admv1.ValidatingAdmissionPolicy { + vap := &admv1.ValidatingAdmissionPolicy{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "admissionregistration.k8s.io/v1", + Kind: "ValidatingAdmissionPolicy", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "aks-fleet-managed-by-arm", + }, + 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{"*"}, + }, + Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, + }, + }, + { + RuleWithOperations: admv1.RuleWithOperations{ + Rule: admv1.Rule{ + APIGroups: []string{""}, + Resources: []string{"resourcequotas"}, + 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{"*"}, + }, + 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`, + }, + }, + }, + } + + if isHub { + vap.Spec.MatchConstraints.ResourceRules = append(vap.Spec.MatchConstraints.ResourceRules, admv1.NamedRuleWithOperations{ + RuleWithOperations: admv1.RuleWithOperations{ + Rule: admv1.Rule{ + APIGroups: []string{"placement.kubernetes-fleet.io"}, + Resources: []string{"clusterresourceplacements"}, + APIVersions: []string{"*"}, + }, + Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, + }, + }) + } + + return vap +} + +func GetValidatingAdmissionPolicyBinding() *admv1.ValidatingAdmissionPolicyBinding { + return &admv1.ValidatingAdmissionPolicyBinding{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "admissionregistration.k8s.io/v1", + Kind: "ValidatingAdmissionPolicyBinding", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "aks-fleet-managed-by-arm", + }, + Spec: admv1.ValidatingAdmissionPolicyBindingSpec{ + PolicyName: "aks-fleet-managed-by-arm", + ValidationActions: []admv1.ValidationAction{ + admv1.Deny, + }, + }, + } +} diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy_test.go b/pkg/webhook/managedresource/validatingadmissionpolicy_test.go new file mode 100644 index 000000000..41a617004 --- /dev/null +++ b/pkg/webhook/managedresource/validatingadmissionpolicy_test.go @@ -0,0 +1,53 @@ +package managedresource + +import ( + "testing" + + "github.com/stretchr/testify/assert" + admv1 "k8s.io/api/admissionregistration/v1" +) + +func TestGetValidatingAdmissionPolicy(t *testing.T) { + t.Parallel() + + 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{ + RuleWithOperations: admv1.RuleWithOperations{ + Rule: admv1.Rule{ + APIGroups: []string{"placement.kubernetes-fleet.io"}, + Resources: []string{"clusterresourceplacements"}, + APIVersions: []string{"*"}, + }, + Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, + }, + }) + }) + + 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{ + RuleWithOperations: admv1.RuleWithOperations{ + Rule: admv1.Rule{ + APIGroups: []string{"placement.kubernetes-fleet.io"}, + Resources: []string{"clusterresourceplacements"}, + APIVersions: []string{"*"}, + }, + Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, + }, + }) + }) +} + +func TestGetValidatingAdmissionPolicyBinding(t *testing.T) { + t.Parallel() + + vap := GetValidatingAdmissionPolicyBinding() + assert.NotNil(t, vap) +} From 6d2eee1fa9436289bad7b58eeddd10d7b780aff5 Mon Sep 17 00:00:00 2001 From: Nont Date: Thu, 21 Aug 2025 11:12:16 -0500 Subject: [PATCH 2/8] Fix comments and docker build Signed-off-by: Nont --- cmd/crdinstaller/main.go | 7 ++----- cmd/crdinstaller/utils/util.go | 3 ++- docker/crd-installer.Dockerfile | 1 + 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/cmd/crdinstaller/main.go b/cmd/crdinstaller/main.go index 7211b6fec..a66990b53 100644 --- a/cmd/crdinstaller/main.go +++ b/cmd/crdinstaller/main.go @@ -21,9 +21,7 @@ import ( "go.goms.io/fleet/cmd/crdinstaller/utils" ) -var ( - mode = flag.String("mode", "", "Mode to run in: 'hub' or 'member' (required)") -) +var mode = flag.String("mode", "", "Mode to run in: 'hub' or 'member' (required)") func main() { klog.InitFlags(nil) @@ -58,7 +56,6 @@ func main() { client, err := client.New(config, client.Options{ Scheme: scheme, }) - if err != nil { klog.Fatalf("Failed to create Kubernetes client: %v", err) } @@ -72,7 +69,7 @@ func main() { klog.Infof("Successfully installed %s CRDs", *mode) if err := utils.InstallManagedResourceVAP(ctx, client, *mode); err != nil { - klog.Fatalf("Failed to install managed resource ValidatingAdmissionPolicy: %v", err) + klog.Warningf("Failed to install managed resource ValidatingAdmissionPolicy: %v", err) } klog.Infof("Successfully installed %s managed resource ValidatingAdmissionPolicy", *mode) diff --git a/cmd/crdinstaller/utils/util.go b/cmd/crdinstaller/utils/util.go index 8c4ba1ad9..399d75683 100644 --- a/cmd/crdinstaller/utils/util.go +++ b/cmd/crdinstaller/utils/util.go @@ -80,7 +80,7 @@ func InstallCRD(ctx context.Context, client client.Client, crd *apiextensionsv1. func install(ctx context.Context, client client.Client, obj client.Object, mut func() error) error { result, err := controllerutil.CreateOrUpdate(ctx, client, obj, mut) if err != nil { - klog.ErrorS(err, "Failed to create or update", "kind", obj.GetObjectKind().GroupVersionKind().Kind, obj.GetName(), "operation", result) + klog.ErrorS(err, "Failed to create or update", "kind", obj.GetObjectKind().GroupVersionKind().Kind, "name", obj.GetName(), "operation", result) return err } return nil @@ -181,6 +181,7 @@ func InstallManagedResourceVAP(ctx context.Context, c client.Client, mode string return err } if !ok { // the cluster doesn't support VAP + klog.Infof("Cluster does not support ValidatingAdmissionPolicy, skipping installation") return nil // not installing } diff --git a/docker/crd-installer.Dockerfile b/docker/crd-installer.Dockerfile index 8485663e3..95c99b884 100644 --- a/docker/crd-installer.Dockerfile +++ b/docker/crd-installer.Dockerfile @@ -11,6 +11,7 @@ RUN go mod download # Copy the go source COPY cmd/crdinstaller/ cmd/crdinstaller/ +COPY pkg pkg ARG TARGETARCH From b501191a11004fc26a2f5bfbfae0d65d4aa9b6a4 Mon Sep 17 00:00:00 2001 From: Nont Date: Thu, 21 Aug 2025 11:21:35 -0500 Subject: [PATCH 3/8] Remove checkResourceSupport Signed-off-by: Nont --- cmd/crdinstaller/utils/util.go | 33 +++++--------------- cmd/crdinstaller/utils/util_test.go | 47 ++--------------------------- 2 files changed, 11 insertions(+), 69 deletions(-) diff --git a/cmd/crdinstaller/utils/util.go b/cmd/crdinstaller/utils/util.go index 399d75683..7146d4e39 100644 --- a/cmd/crdinstaller/utils/util.go +++ b/cmd/crdinstaller/utils/util.go @@ -18,7 +18,6 @@ import ( "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" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" @@ -78,6 +77,10 @@ func InstallCRD(ctx context.Context, client client.Client, crd *apiextensionsv1. } func install(ctx context.Context, client client.Client, obj client.Object, mut func() error) error { + if mut == nil { + mut = func() error { return nil } + } + result, err := controllerutil.CreateOrUpdate(ctx, client, obj, mut) if err != nil { klog.ErrorS(err, "Failed to create or update", "kind", obj.GetObjectKind().GroupVersionKind().Kind, "name", obj.GetName(), "operation", result) @@ -175,34 +178,14 @@ func GetCRDFromPath(crdPath string, scheme *runtime.Scheme) (*apiextensionsv1.Cu func InstallManagedResourceVAP(ctx context.Context, c client.Client, mode string) error { vap := managedresource.GetValidatingAdmissionPolicy(mode == "hub") vapBinding := managedresource.GetValidatingAdmissionPolicyBinding() - - ok, err := checkResourceSupport(c, vap.GroupVersionKind()) - if err != nil { - return err - } - if !ok { // the cluster doesn't support VAP - klog.Infof("Cluster does not support ValidatingAdmissionPolicy, skipping installation") - return nil // not installing - } - for _, ob := range []client.Object{vap, vapBinding} { - if err = install(ctx, c, ob, nil); err != nil { + 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 err } } klog.Infof("Successfully installed managed resource ValidatingAdmissionPolicy") return nil } - -func checkResourceSupport(client client.Client, gvk schema.GroupVersionKind) (bool, error) { - restMapper := client.RESTMapper() - - _, err := restMapper.RESTMapping(gvk.GroupKind(), gvk.Version) - if err != nil { - if meta.IsNoMatchError(err) { - return false, nil - } - return false, err - } - return true, nil -} diff --git a/cmd/crdinstaller/utils/util_test.go b/cmd/crdinstaller/utils/util_test.go index db4156c21..06b20de9a 100644 --- a/cmd/crdinstaller/utils/util_test.go +++ b/cmd/crdinstaller/utils/util_test.go @@ -19,17 +19,14 @@ import ( apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) -var ( - lessFunc = func(s1, s2 string) bool { - return s1 < s2 - } -) +var lessFunc = func(s1, s2 string) bool { + return s1 < s2 +} // Test using the actual config/crd/bases directory. func TestCollectCRDFileNamesWithActualPath(t *testing.T) { @@ -277,44 +274,6 @@ func TestInstall(t *testing.T) { } } -func TestCheckResourceSupport(t *testing.T) { - tests := []struct { - name string - gvk schema.GroupVersionKind - }{ - { - name: "VAP resource check", - gvk: schema.GroupVersionKind{ - Group: "admissionregistration.k8s.io", - Version: "v1", - Kind: "ValidatingAdmissionPolicy", - }, - }, - { - name: "nonexistent resource check", - gvk: schema.GroupVersionKind{ - Group: "nonexistent.io", - Version: "v1", - Kind: "NonExistentResource", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - scheme := runtime.NewScheme() - require.NoError(t, admv1.AddToScheme(scheme)) - - fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() - supported, err := checkResourceSupport(fakeClient, tt.gvk) - - // The function should not panic and should return a boolean result - assert.NoError(t, err) - assert.IsType(t, false, supported) - }) - } -} - func TestInstallManagedResourceVAP(t *testing.T) { tests := []struct { name string From 9a1b8f31902d04405ca0a442ac0b29504e048704 Mon Sep 17 00:00:00 2001 From: Nont Date: Thu, 21 Aug 2025 12:49:34 -0500 Subject: [PATCH 4/8] Fix comment Signed-off-by: Nont --- cmd/crdinstaller/main.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/crdinstaller/main.go b/cmd/crdinstaller/main.go index a66990b53..a1913a4a3 100644 --- a/cmd/crdinstaller/main.go +++ b/cmd/crdinstaller/main.go @@ -70,6 +70,7 @@ func main() { 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) From 8d7ca30de0d5a0825e20fce0f2f389be56819bf5 Mon Sep 17 00:00:00 2001 From: Nont Date: Thu, 21 Aug 2025 14:42:10 -0500 Subject: [PATCH 5/8] Address comments Signed-off-by: Nont --- cmd/crdinstaller/utils/util.go | 1 + cmd/crdinstaller/utils/util_test.go | 64 +++++++++++++++++++++-------- 2 files changed, 49 insertions(+), 16 deletions(-) diff --git a/cmd/crdinstaller/utils/util.go b/cmd/crdinstaller/utils/util.go index 7146d4e39..18399f5d5 100644 --- a/cmd/crdinstaller/utils/util.go +++ b/cmd/crdinstaller/utils/util.go @@ -182,6 +182,7 @@ func InstallManagedResourceVAP(ctx context.Context, c client.Client, mode string 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 } diff --git a/cmd/crdinstaller/utils/util_test.go b/cmd/crdinstaller/utils/util_test.go index 06b20de9a..f4f8b3cdc 100644 --- a/cmd/crdinstaller/utils/util_test.go +++ b/cmd/crdinstaller/utils/util_test.go @@ -13,8 +13,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" admv1 "k8s.io/api/admissionregistration/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -123,7 +121,9 @@ func runTest(t *testing.T, crdPath string) { func TestInstallCRD(t *testing.T) { scheme := runtime.NewScheme() - require.NoError(t, apiextensionsv1.AddToScheme(scheme)) + if err := apiextensionsv1.AddToScheme(scheme); err != nil { + t.Fatalf("Failed to add apiextensions scheme: %v", err) + } testCRD := &apiextensionsv1.CustomResourceDefinition{ ObjectMeta: metav1.ObjectMeta{ @@ -170,26 +170,44 @@ func TestInstallCRD(t *testing.T) { err := InstallCRD(context.Background(), fakeClient, tt.crd) if tt.wantError { - assert.Error(t, err) + if err == nil { + t.Errorf("InstallCRD() expected error but got none") + } return } - assert.NoError(t, err) + if err != nil { + t.Errorf("InstallCRD() unexpected error: %v", err) + return + } var installedCRD apiextensionsv1.CustomResourceDefinition err = fakeClient.Get(context.Background(), types.NamespacedName{Name: tt.crd.Name}, &installedCRD) - assert.NoError(t, err) + if err != nil { + t.Errorf("Failed to get installed CRD: %v", err) + return + } + + if installedCRD.Labels[CRDInstallerLabelKey] != "true" { + t.Errorf("Expected CRD label %s to be 'true', got %q", CRDInstallerLabelKey, installedCRD.Labels[CRDInstallerLabelKey]) + } + + if installedCRD.Labels[AzureManagedLabelKey] != FleetLabelValue { + t.Errorf("Expected CRD label %s to be %q, got %q", AzureManagedLabelKey, FleetLabelValue, installedCRD.Labels[AzureManagedLabelKey]) + } - assert.Equal(t, "true", installedCRD.Labels[CRDInstallerLabelKey]) - assert.Equal(t, FleetLabelValue, installedCRD.Labels[AzureManagedLabelKey]) - assert.Equal(t, tt.crd.Spec, installedCRD.Spec) + if diff := cmp.Diff(tt.crd.Spec, installedCRD.Spec); diff != "" { + t.Errorf("CRD spec mismatch (-want +got):\n%s", diff) + } }) } } func TestInstall(t *testing.T) { scheme := runtime.NewScheme() - require.NoError(t, apiextensionsv1.AddToScheme(scheme)) + if err := apiextensionsv1.AddToScheme(scheme); err != nil { + t.Fatalf("Failed to add apiextensions scheme: %v", err) + } testCRD := &apiextensionsv1.CustomResourceDefinition{ ObjectMeta: metav1.ObjectMeta{ @@ -257,18 +275,28 @@ func TestInstall(t *testing.T) { err := install(context.Background(), fakeClient, tt.obj, tt.mutFunc) if tt.wantError { - assert.Error(t, err) + if err == nil { + t.Errorf("install() expected error but got none") + } return } - assert.NoError(t, err) + if err != nil { + t.Errorf("install() unexpected error: %v", err) + return + } var installed apiextensionsv1.CustomResourceDefinition err = fakeClient.Get(context.Background(), types.NamespacedName{Name: tt.obj.GetName()}, &installed) - assert.NoError(t, err) + if err != nil { + t.Errorf("Failed to get installed object: %v", err) + return + } if tt.mutFunc != nil && tt.obj.GetName() == "test.example.com" { - assert.Equal(t, "value", installed.Labels["test"]) + if installed.Labels["test"] != "value" { + t.Errorf("Expected label 'test' to be 'value', got %q", installed.Labels["test"]) + } } }) } @@ -292,7 +320,9 @@ func TestInstallManagedResourceVAP(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { scheme := runtime.NewScheme() - require.NoError(t, admv1.AddToScheme(scheme)) + 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) @@ -300,7 +330,9 @@ func TestInstallManagedResourceVAP(t *testing.T) { // 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 - assert.NoError(t, err) + if err != nil { + t.Errorf("InstallManagedResourceVAP() unexpected error: %v", err) + } }) } } From 264971a658ca077f75f79b7d616b8032db895941 Mon Sep 17 00:00:00 2001 From: Nont Date: Thu, 21 Aug 2025 16:54:50 -0500 Subject: [PATCH 6/8] Add e2e and register admissionregistration to scheme Signed-off-by: Nont --- cmd/crdinstaller/main.go | 4 + test/e2e/framework/cluster.go | 8 +- test/e2e/managed_resource_vap_test.go | 217 ++++++++++++++++++++++ test/e2e/resources/managed-namespace.yaml | 7 + 4 files changed, 233 insertions(+), 3 deletions(-) create mode 100644 test/e2e/managed_resource_vap_test.go create mode 100644 test/e2e/resources/managed-namespace.yaml diff --git a/cmd/crdinstaller/main.go b/cmd/crdinstaller/main.go index a1913a4a3..e968c46b2 100644 --- a/cmd/crdinstaller/main.go +++ b/cmd/crdinstaller/main.go @@ -11,6 +11,7 @@ import ( "flag" "fmt" + admv1 "k8s.io/api/admissionregistration/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/klog/v2" @@ -53,6 +54,9 @@ func main() { if err := apiextensionsv1.AddToScheme(scheme); err != nil { klog.Fatalf("Failed to add apiextensions scheme: %v", err) } + if err := admv1.AddToScheme(scheme); err != nil { + klog.Fatalf("Failed to add admissionregistration scheme: %v", err) + } client, err := client.New(config, client.Options{ Scheme: scheme, }) diff --git a/test/e2e/framework/cluster.go b/test/e2e/framework/cluster.go index 229385064..0357eb852 100644 --- a/test/e2e/framework/cluster.go +++ b/test/e2e/framework/cluster.go @@ -32,9 +32,7 @@ import ( "go.goms.io/fleet/pkg/propertyprovider/azure/trackers" ) -var ( - kubeconfigPath = os.Getenv("KUBECONFIG") -) +var kubeconfigPath = os.Getenv("KUBECONFIG") // Cluster object defines the required clients based on the kubeconfig of the test cluster. type Cluster struct { @@ -94,6 +92,10 @@ func GetClientConfig(cluster *Cluster) clientcmd.ClientConfig { &clientcmd.ClientConfigLoadingRules{ExplicitPath: kubeconfigPath}, &clientcmd.ConfigOverrides{ CurrentContext: cluster.ClusterName, + AuthInfo: api.AuthInfo{ + Impersonate: "system", + ImpersonateGroups: []string{"system:masters"}, + }, }) } diff --git a/test/e2e/managed_resource_vap_test.go b/test/e2e/managed_resource_vap_test.go new file mode 100644 index 000000000..ef69c75c1 --- /dev/null +++ b/test/e2e/managed_resource_vap_test.go @@ -0,0 +1,217 @@ +/* +Copyright 2025 The KubeFleet Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package e2e + +import ( + "errors" + "fmt" + "net/http" + "reflect" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + corev1 "k8s.io/api/core/v1" + networkingv1 "k8s.io/api/networking/v1" + k8sErrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" + + testutils "go.goms.io/fleet/test/e2e/v1alpha1/utils" +) + +const ( + managedByLabel = "fleet.azure.com/managed-by" + managedByLabelValue = "arm" + vapName = "aks-fleet-managed-by-arm" + vapBindingName = "aks-fleet-managed-by-arm" +) + +// Helper functions for creating managed resources +func createManagedNamespace(name string) *corev1.Namespace { + return &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: map[string]string{ + managedByLabel: managedByLabelValue, + }, + }, + } +} + +func createUnmanagedNamespace(name string) *corev1.Namespace { + return &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + } +} + +func createManagedResourceQuota(name, namespace string) *corev1.ResourceQuota { + return &corev1.ResourceQuota{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: map[string]string{ + managedByLabel: managedByLabelValue, + }, + }, + Spec: corev1.ResourceQuotaSpec{ + Hard: corev1.ResourceList{ + corev1.ResourcePods: resource.MustParse("10"), + }, + }, + } +} + +func createManagedNetworkPolicy(name, namespace string) *networkingv1.NetworkPolicy { + return &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: map[string]string{ + managedByLabel: managedByLabelValue, + }, + }, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{ + networkingv1.PolicyTypeIngress, + }, + }, + } +} + +func createManagedCRP(name string) *placementv1beta1.ClusterResourcePlacement { + return &placementv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: map[string]string{ + managedByLabel: managedByLabelValue, + }, + }, + Spec: placementv1beta1.PlacementSpec{ + ResourceSelectors: []placementv1beta1.ClusterResourceSelector{ + { + Group: "", + Version: "v1", + Kind: "Namespace", + Name: "test-ns", + }, + }, + }, + } +} + +func expectDeniedByVAP(err error) { + var statusErr *k8sErrors.StatusError + Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Expected StatusError, got error %s of type %s", err, reflect.TypeOf(err))) + Expect(statusErr.ErrStatus.Code).To(Equal(int32(http.StatusUnprocessableEntity)), "Expected 422 UnprocessableEntity") + // ValidatingAdmissionPolicy denials typically contain these patterns + Expect(statusErr.ErrStatus.Message).To(SatisfyAny( + ContainSubstring("ValidatingAdmissionPolicy"), + ContainSubstring("denied the request"), + ContainSubstring("violates policy"), + ), "Error should indicate policy violation") +} + +var _ = Describe("ValidatingAdmissionPolicy for Managed Resources", Label("managedresource"), Ordered, func() { + BeforeEach(func() { + Eventually(func() error { + var vap admissionregistrationv1.ValidatingAdmissionPolicy + return hubClient.Get(ctx, types.NamespacedName{Name: vapName}, &vap) + }, testutils.PollTimeout, testutils.PollInterval).Should(Succeed(), "ValidatingAdmissionPolicy should be installed") + + Eventually(func() error { + var vapBinding admissionregistrationv1.ValidatingAdmissionPolicyBinding + return hubClient.Get(ctx, types.NamespacedName{Name: vapBindingName}, &vapBinding) + }, testutils.PollTimeout, testutils.PollInterval).Should(Succeed(), "ValidatingAdmissionPolicyBinding should be installed") + }) + + Context("Namespace operations on managed-by label", 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 := impersonateHubClient.Create(ctx, managedNS) + expectDeniedByVAP(err) + }) + + It("should deny UPDATE operation on managed namespace for non-system:masters user", func() { + managedNS := createManagedNamespace("test-managed-ns-update") + By("creating managed namespace with system:masters user") + Expect(hubClient.Create(ctx, managedNS)).To(Succeed()) + + Eventually(func() error { + var ns corev1.Namespace + if err := hubClient.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") + err := impersonateHubClient.Update(ctx, &ns) + if k8sErrors.IsConflict(err) { + return err + } + expectDeniedByVAP(err) + return nil + }, testutils.PollTimeout, testutils.PollInterval).Should(Succeed()) + + Expect(hubClient.Delete(ctx, managedNS)).To(Succeed()) + }) + + It("should deny DELETE operation on managed namespace for non-system:masters user", func() { + managedNS := createManagedNamespace("test-managed-ns-delete") + By("creating managed namespace with system:masters user") + Expect(hubClient.Create(ctx, managedNS)).To(Succeed()) + + By("expecting denial of DELETE operation on managed namespace") + err := impersonateHubClient.Delete(ctx, managedNS) + expectDeniedByVAP(err) + + Expect(hubClient.Delete(ctx, managedNS)).To(Succeed()) + }) + + 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(hubClient.Create(ctx, managedNS)).To(Succeed()) + + Expect(hubClient.Delete(ctx, managedNS)).To(Succeed()) + }) + + 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(impersonateHubClient.Create(ctx, unmanagedNS)).To(Succeed()) + + By("expecting successful UPDATE operation on unmanaged namespace") + Eventually(func() error { + var ns corev1.Namespace + if err := impersonateHubClient.Get(ctx, types.NamespacedName{Name: unmanagedNS.Name}, &ns); err != nil { + return err + } + ns.Annotations = map[string]string{"test": "annotation"} + return impersonateHubClient.Update(ctx, &ns) + }, testutils.PollTimeout, testutils.PollInterval).Should(Succeed()) + + By("expecting successful DELETE operation on unmanaged namespace") + Expect(impersonateHubClient.Delete(ctx, unmanagedNS)).To(Succeed()) + }) + }) +}) diff --git a/test/e2e/resources/managed-namespace.yaml b/test/e2e/resources/managed-namespace.yaml new file mode 100644 index 000000000..66a976437 --- /dev/null +++ b/test/e2e/resources/managed-namespace.yaml @@ -0,0 +1,7 @@ +apiVersion: v1 +kind: Namespace +metadata: + name: managed-test-namespace + labels: + fleet.azure.com/managed-by: arm +spec: {} \ No newline at end of file From e769fbfdfb5b6f614596cb84c3719dee1be5d439 Mon Sep 17 00:00:00 2001 From: Nont Date: Thu, 21 Aug 2025 18:19:42 -0500 Subject: [PATCH 7/8] Fix review comments Signed-off-by: Nont --- .../validatingadmissionpolicy.go | 6 +- .../validatingadmissionpolicy_test.go | 5 ++ test/e2e/managed_resource_vap_test.go | 80 +++---------------- test/e2e/resources/managed-namespace.yaml | 7 -- 4 files changed, 18 insertions(+), 80 deletions(-) delete mode 100644 test/e2e/resources/managed-namespace.yaml diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy.go b/pkg/webhook/managedresource/validatingadmissionpolicy.go index 76089a355..a557849b1 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" ) +const resourceName = "aks-fleet-managed-by-arm" + func GetValidatingAdmissionPolicy(isHub bool) *admv1.ValidatingAdmissionPolicy { vap := &admv1.ValidatingAdmissionPolicy{ TypeMeta: metav1.TypeMeta{ @@ -17,7 +19,7 @@ func GetValidatingAdmissionPolicy(isHub bool) *admv1.ValidatingAdmissionPolicy { Kind: "ValidatingAdmissionPolicy", }, ObjectMeta: metav1.ObjectMeta{ - Name: "aks-fleet-managed-by-arm", + Name: resourceName, }, Spec: admv1.ValidatingAdmissionPolicySpec{ MatchConstraints: &admv1.MatchResources{ @@ -92,7 +94,7 @@ func GetValidatingAdmissionPolicyBinding() *admv1.ValidatingAdmissionPolicyBindi Kind: "ValidatingAdmissionPolicyBinding", }, ObjectMeta: metav1.ObjectMeta{ - Name: "aks-fleet-managed-by-arm", + Name: resourceName, }, Spec: admv1.ValidatingAdmissionPolicyBindingSpec{ PolicyName: "aks-fleet-managed-by-arm", diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy_test.go b/pkg/webhook/managedresource/validatingadmissionpolicy_test.go index 41a617004..403734ca2 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy_test.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy_test.go @@ -1,3 +1,8 @@ +/* +Copyright (c) Microsoft Corporation. +Licensed under the MIT license. +*/ + package managedresource import ( diff --git a/test/e2e/managed_resource_vap_test.go b/test/e2e/managed_resource_vap_test.go index ef69c75c1..ef5da74cf 100644 --- a/test/e2e/managed_resource_vap_test.go +++ b/test/e2e/managed_resource_vap_test.go @@ -25,14 +25,10 @@ import ( . "github.com/onsi/gomega" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" corev1 "k8s.io/api/core/v1" - networkingv1 "k8s.io/api/networking/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" - testutils "go.goms.io/fleet/test/e2e/v1alpha1/utils" ) @@ -63,62 +59,6 @@ func createUnmanagedNamespace(name string) *corev1.Namespace { } } -func createManagedResourceQuota(name, namespace string) *corev1.ResourceQuota { - return &corev1.ResourceQuota{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, - Labels: map[string]string{ - managedByLabel: managedByLabelValue, - }, - }, - Spec: corev1.ResourceQuotaSpec{ - Hard: corev1.ResourceList{ - corev1.ResourcePods: resource.MustParse("10"), - }, - }, - } -} - -func createManagedNetworkPolicy(name, namespace string) *networkingv1.NetworkPolicy { - return &networkingv1.NetworkPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, - Labels: map[string]string{ - managedByLabel: managedByLabelValue, - }, - }, - Spec: networkingv1.NetworkPolicySpec{ - PodSelector: metav1.LabelSelector{}, - PolicyTypes: []networkingv1.PolicyType{ - networkingv1.PolicyTypeIngress, - }, - }, - } -} - -func createManagedCRP(name string) *placementv1beta1.ClusterResourcePlacement { - return &placementv1beta1.ClusterResourcePlacement{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Labels: map[string]string{ - managedByLabel: managedByLabelValue, - }, - }, - Spec: placementv1beta1.PlacementSpec{ - ResourceSelectors: []placementv1beta1.ClusterResourceSelector{ - { - Group: "", - Version: "v1", - Kind: "Namespace", - Name: "test-ns", - }, - }, - }, - } -} - func expectDeniedByVAP(err error) { var statusErr *k8sErrors.StatusError Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Expected StatusError, got error %s of type %s", err, reflect.TypeOf(err))) @@ -132,16 +72,12 @@ func expectDeniedByVAP(err error) { } var _ = Describe("ValidatingAdmissionPolicy for Managed Resources", Label("managedresource"), Ordered, func() { - BeforeEach(func() { - Eventually(func() error { - var vap admissionregistrationv1.ValidatingAdmissionPolicy - return hubClient.Get(ctx, types.NamespacedName{Name: vapName}, &vap) - }, testutils.PollTimeout, testutils.PollInterval).Should(Succeed(), "ValidatingAdmissionPolicy should be installed") - - Eventually(func() error { - var vapBinding admissionregistrationv1.ValidatingAdmissionPolicyBinding - return hubClient.Get(ctx, types.NamespacedName{Name: vapBindingName}, &vapBinding) - }, testutils.PollTimeout, testutils.PollInterval).Should(Succeed(), "ValidatingAdmissionPolicyBinding should be installed") + BeforeAll(func() { + var vap admissionregistrationv1.ValidatingAdmissionPolicy + Expect(hubClient.Get(ctx, types.NamespacedName{Name: vapName}, &vap)).Should(Succeed(), "ValidatingAdmissionPolicy should be installed") + + var vapBinding admissionregistrationv1.ValidatingAdmissionPolicyBinding + Expect(hubClient.Get(ctx, types.NamespacedName{Name: vapBindingName}, &vapBinding)).Should(Succeed(), "ValidatingAdmissionPolicyBinding should be installed") }) Context("Namespace operations on managed-by label", func() { @@ -157,6 +93,7 @@ var _ = Describe("ValidatingAdmissionPolicy for Managed Resources", Label("manag By("creating managed namespace with system:masters user") Expect(hubClient.Create(ctx, managedNS)).To(Succeed()) + var updateErr error Eventually(func() error { var ns corev1.Namespace if err := hubClient.Get(ctx, types.NamespacedName{Name: managedNS.Name}, &ns); err != nil { @@ -168,10 +105,11 @@ var _ = Describe("ValidatingAdmissionPolicy for Managed Resources", Label("manag if k8sErrors.IsConflict(err) { return err } - expectDeniedByVAP(err) + updateErr = err return nil }, testutils.PollTimeout, testutils.PollInterval).Should(Succeed()) + expectDeniedByVAP(updateErr) Expect(hubClient.Delete(ctx, managedNS)).To(Succeed()) }) diff --git a/test/e2e/resources/managed-namespace.yaml b/test/e2e/resources/managed-namespace.yaml deleted file mode 100644 index 66a976437..000000000 --- a/test/e2e/resources/managed-namespace.yaml +++ /dev/null @@ -1,7 +0,0 @@ -apiVersion: v1 -kind: Namespace -metadata: - name: managed-test-namespace - labels: - fleet.azure.com/managed-by: arm -spec: {} \ No newline at end of file From 74eac8c13ef1531c7591dc8be973e704dda29366 Mon Sep 17 00:00:00 2001 From: Nont Date: Fri, 22 Aug 2025 19:10:12 -0500 Subject: [PATCH 8/8] Use a different client for system:masters Signed-off-by: Nont --- .../validatingadmissionpolicy.go | 4 + test/e2e/fleet_guard_rail_test.go | 78 ++++++------ test/e2e/framework/cluster.go | 18 +++ test/e2e/managed_resource_vap_test.go | 118 +++++++++++------- test/e2e/setup_test.go | 13 +- 5 files changed, 141 insertions(+), 90 deletions(-) diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy.go b/pkg/webhook/managedresource/validatingadmissionpolicy.go index a557849b1..c1b7516b7 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy.go @@ -12,6 +12,8 @@ import ( const resourceName = "aks-fleet-managed-by-arm" +var forbidden = metav1.StatusReasonForbidden + func GetValidatingAdmissionPolicy(isHub bool) *admv1.ValidatingAdmissionPolicy { vap := &admv1.ValidatingAdmissionPolicy{ TypeMeta: metav1.TypeMeta{ @@ -66,6 +68,8 @@ func GetValidatingAdmissionPolicy(isHub bool) *admv1.ValidatingAdmissionPolicy { Validations: []admv1.Validation{ { Expression: `"system:masters" in request.userInfo.groups || "system:serviceaccounts:kube-system" in request.userInfo.groups`, + Message: "Create, Update, or Delete operations on ARM managed resources is forbidden", + Reason: &forbidden, }, }, }, diff --git a/test/e2e/fleet_guard_rail_test.go b/test/e2e/fleet_guard_rail_test.go index eb4e6f070..123602761 100644 --- a/test/e2e/fleet_guard_rail_test.go +++ b/test/e2e/fleet_guard_rail_test.go @@ -78,7 +78,7 @@ var _ = Describe("fleet guard rail tests for deny fleet MC CREATE operations", f } By(fmt.Sprintf("expecting denial of operation CREATE of fleet member cluster %s", mc.Name)) - Expect(checkIfStatusErrorWithMessage(impersonateHubClient.Create(ctx, mc), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Create, &mcGVK, "", types.NamespacedName{Name: mc.Name}))).Should(Succeed()) + Expect(checkIfStatusErrorWithMessage(notMasterUser.Create(ctx, mc), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Create, &mcGVK, "", types.NamespacedName{Name: mc.Name}))).Should(Succeed()) }) }) @@ -105,7 +105,7 @@ var _ = Describe("fleet guard rail tests for allow/deny fleet MC UPDATE, DELETE return errors.New("annotations are empty") } mc.Annotations[fleetClusterResourceIDAnnotationKey] = clusterID2 - err = impersonateHubClient.Update(ctx, &mc) + err = notMasterUser.Update(ctx, &mc) if k8sErrors.IsConflict(err) { return err } @@ -120,7 +120,7 @@ var _ = Describe("fleet guard rail tests for allow/deny fleet MC UPDATE, DELETE } By(fmt.Sprintf("remove fleet member cluster, cluster id annotation %s", mc.Name)) mc.SetAnnotations(nil) - err = impersonateHubClient.Update(ctx, &mc) + err = notMasterUser.Update(ctx, &mc) if k8sErrors.IsConflict(err) { return err } @@ -155,7 +155,7 @@ var _ = Describe("fleet guard rail tests for allow/deny fleet MC UPDATE, DELETE By(fmt.Sprintf("update fleet member cluster spec for MC %s", mc.Name)) mc.Spec.HeartbeatPeriodSeconds = 30 By(fmt.Sprintf("expecting denial of operation UPDATE of fleet member cluster %s", mc.Name)) - err = impersonateHubClient.Update(ctx, &mc) + err = notMasterUser.Update(ctx, &mc) if k8sErrors.IsConflict(err) { return err } @@ -175,7 +175,7 @@ var _ = Describe("fleet guard rail tests for allow/deny fleet MC UPDATE, DELETE return errors.New("status conditions are empty") } mc.Status.Conditions[0].Reason = testReason - err = impersonateHubClient.Status().Update(ctx, &mc) + err = notMasterUser.Status().Update(ctx, &mc) if k8sErrors.IsConflict(err) { return err } @@ -190,7 +190,7 @@ var _ = Describe("fleet guard rail tests for allow/deny fleet MC UPDATE, DELETE }, } By(fmt.Sprintf("expecting denial of operation DELETE of fleet member cluster %s", mc.Name)) - Expect(checkIfStatusErrorWithMessage(impersonateHubClient.Delete(ctx, &mc), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Delete, &mcGVK, "", types.NamespacedName{Name: mc.Name}))).Should(Succeed()) + Expect(checkIfStatusErrorWithMessage(notMasterUser.Delete(ctx, &mc), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Delete, &mcGVK, "", types.NamespacedName{Name: mc.Name}))).Should(Succeed()) }) It("should allow update operation on fleet member cluster CR labels for non system user", func() { @@ -202,7 +202,7 @@ var _ = Describe("fleet guard rail tests for allow/deny fleet MC UPDATE, DELETE return err } mc.SetLabels(map[string]string{testKey: testValue}) - return impersonateHubClient.Update(ctx, &mc) + return notMasterUser.Update(ctx, &mc) }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) @@ -220,7 +220,7 @@ var _ = Describe("fleet guard rail tests for allow/deny fleet MC UPDATE, DELETE } annotations[testKey] = testValue mc.SetAnnotations(annotations) - return impersonateHubClient.Update(ctx, &mc) + return notMasterUser.Update(ctx, &mc) }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) @@ -288,7 +288,7 @@ var _ = Describe("fleet guard rail tests for allow/deny fleet MC UPDATE, DELETE Effect: corev1.TaintEffectNoSchedule, } mc.Spec.Taints = append(mc.Spec.Taints, taint) - return impersonateHubClient.Update(ctx, &mc) + return notMasterUser.Update(ctx, &mc) }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) @@ -339,9 +339,9 @@ var _ = Describe("fleet guard rail tests for allow upstream MC CREATE, DELETE op HeartbeatPeriodSeconds: 60, }, } - Expect(impersonateHubClient.Create(ctx, mc)).Should(Succeed()) - Expect(impersonateHubClient.Get(ctx, types.NamespacedName{Name: mc.Name}, mc)).Should(Succeed()) - Expect(impersonateHubClient.Delete(ctx, mc)).Should(Succeed()) + Expect(notMasterUser.Create(ctx, mc)).Should(Succeed()) + Expect(notMasterUser.Get(ctx, types.NamespacedName{Name: mc.Name}, mc)).Should(Succeed()) + Expect(notMasterUser.Delete(ctx, mc)).Should(Succeed()) ensureMemberClusterAndRelatedResourcesDeletion(mcName) }) }) @@ -366,7 +366,7 @@ var _ = Describe("fleet guard rail tests for allow/deny upstream MC UPDATE opera } By(fmt.Sprintf("add fleet member cluster, cluster id annotation %s", mc.Name)) mc.SetAnnotations(map[string]string{fleetClusterResourceIDAnnotationKey: clusterID1}) - err = impersonateHubClient.Update(ctx, &mc) + err = notMasterUser.Update(ctx, &mc) if k8sErrors.IsConflict(err) { return err } @@ -403,7 +403,7 @@ var _ = Describe("fleet guard rail tests for allow/deny upstream MC UPDATE opera return errors.New("status conditions are empty") } mc.Status.Conditions[0].Reason = testReason - err = impersonateHubClient.Status().Update(ctx, &mc) + err = notMasterUser.Status().Update(ctx, &mc) if k8sErrors.IsConflict(err) { return err } @@ -420,7 +420,7 @@ var _ = Describe("fleet guard rail tests for allow/deny upstream MC UPDATE opera return err } mc.SetLabels(map[string]string{testKey: testValue}) - return impersonateHubClient.Update(ctx, &mc) + return notMasterUser.Update(ctx, &mc) }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) @@ -433,7 +433,7 @@ var _ = Describe("fleet guard rail tests for allow/deny upstream MC UPDATE opera return err } mc.SetAnnotations(map[string]string{testKey: testValue}) - return impersonateHubClient.Update(ctx, &mc) + return notMasterUser.Update(ctx, &mc) }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) @@ -451,7 +451,7 @@ var _ = Describe("fleet guard rail tests for allow/deny upstream MC UPDATE opera Effect: corev1.TaintEffectNoSchedule, } mc.Spec.Taints = append(mc.Spec.Taints, taint) - return impersonateHubClient.Update(ctx, &mc) + return notMasterUser.Update(ctx, &mc) }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) @@ -478,7 +478,7 @@ var _ = Describe("fleet guard rail tests for allow/deny upstream MC UPDATE opera By(fmt.Sprintf("update upstream member cluster spec for MC %s", mc.Name)) mc.Spec.HeartbeatPeriodSeconds = 32 By(fmt.Sprintf("expecting denial of operation UPDATE of upstream member cluster %s", mc.Name)) - return impersonateHubClient.Update(ctx, &mc) + return notMasterUser.Update(ctx, &mc) }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) @@ -524,7 +524,7 @@ var _ = Describe("fleet guard rail tests for IMC UPDATE operation, in fleet-memb }, } By(fmt.Sprintf("expecting denial of operation CREATE of Internal Member Cluster %s/%s", mcName, imcNamespace)) - Expect(checkIfStatusErrorWithMessage(impersonateHubClient.Create(ctx, &imc), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Create, &imcGVK, "", types.NamespacedName{Name: imc.Name, Namespace: imc.Namespace}))).Should(Succeed()) + Expect(checkIfStatusErrorWithMessage(notMasterUser.Create(ctx, &imc), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Create, &imcGVK, "", types.NamespacedName{Name: imc.Name, Namespace: imc.Namespace}))).Should(Succeed()) }) It("should deny UPDATE operation on internal member cluster CR for user not in MC identity in fleet member namespace", func() { @@ -536,7 +536,7 @@ var _ = Describe("fleet guard rail tests for IMC UPDATE operation, in fleet-memb } imc.Spec.HeartbeatPeriodSeconds = 25 By("expecting denial of operation UPDATE of Internal Member Cluster") - err = impersonateHubClient.Update(ctx, &imc) + err = notMasterUser.Update(ctx, &imc) if k8sErrors.IsConflict(err) { return err } @@ -548,7 +548,7 @@ var _ = Describe("fleet guard rail tests for IMC UPDATE operation, in fleet-memb var imc clusterv1beta1.InternalMemberCluster Expect(hubClient.Get(ctx, types.NamespacedName{Name: mcName, Namespace: imcNamespace}, &imc)).Should(Succeed()) By("expecting denial of operation DELETE of Internal Member Cluster") - Expect(checkIfStatusErrorWithMessage(impersonateHubClient.Delete(ctx, &imc), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Delete, &imcGVK, "", types.NamespacedName{Name: imc.Name, Namespace: imc.Namespace}))).Should(Succeed()) + Expect(checkIfStatusErrorWithMessage(notMasterUser.Delete(ctx, &imc), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Delete, &imcGVK, "", types.NamespacedName{Name: imc.Name, Namespace: imc.Namespace}))).Should(Succeed()) }) It("should deny UPDATE operation on internal member cluster CR status for user not in MC identity in fleet member namespace", func() { @@ -569,7 +569,7 @@ var _ = Describe("fleet guard rail tests for IMC UPDATE operation, in fleet-memb AgentStatus: nil, } By("expecting denial of operation UPDATE of internal member cluster CR status") - err = impersonateHubClient.Status().Update(ctx, &imc) + err = notMasterUser.Status().Update(ctx, &imc) if k8sErrors.IsConflict(err) { return err } @@ -609,7 +609,7 @@ var _ = Describe("fleet guard rail tests for IMC UPDATE operation, in fleet-memb AgentStatus: nil, } By("expecting successful UPDATE of Internal Member Cluster Status") - return impersonateHubClient.Status().Update(ctx, &imc) + return notMasterUser.Status().Update(ctx, &imc) }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) @@ -622,7 +622,7 @@ var _ = Describe("fleet guard rail tests for IMC UPDATE operation, in fleet-memb } imc.Labels = map[string]string{testKey: testValue} By("expecting successful UPDATE of Internal Member Cluster Status") - return impersonateHubClient.Update(ctx, &imc) + return notMasterUser.Update(ctx, &imc) }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) @@ -693,7 +693,7 @@ var _ = Describe("fleet guard rail for UPDATE work operations, in fleet prefixed }, } By(fmt.Sprintf("expecting denial of operation CREATE of Work %s/%s", workName, imcNamespace)) - Expect(checkIfStatusErrorWithMessage(impersonateHubClient.Create(ctx, &w), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Create, &workGVK, "", types.NamespacedName{Name: w.Name, Namespace: w.Namespace}))).Should(Succeed()) + Expect(checkIfStatusErrorWithMessage(notMasterUser.Create(ctx, &w), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Create, &workGVK, "", types.NamespacedName{Name: w.Name, Namespace: w.Namespace}))).Should(Succeed()) }) It("should deny UPDATE operation on work CR status for user not in MC identity", func() { @@ -715,7 +715,7 @@ var _ = Describe("fleet guard rail for UPDATE work operations, in fleet prefixed }, } By("expecting denial of operation UPDATE of work CR status") - err = impersonateHubClient.Status().Update(ctx, &w) + err = notMasterUser.Status().Update(ctx, &w) if k8sErrors.IsConflict(err) { return err } @@ -727,7 +727,7 @@ var _ = Describe("fleet guard rail for UPDATE work operations, in fleet prefixed var w placementv1beta1.Work Expect(hubClient.Get(ctx, types.NamespacedName{Name: workName, Namespace: imcNamespace}, &w)).Should(Succeed()) By("expecting denial of operation DELETE of work") - Expect(checkIfStatusErrorWithMessage(impersonateHubClient.Delete(ctx, &w), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Delete, &workGVK, "", types.NamespacedName{Name: w.Name, Namespace: w.Namespace}))).Should(Succeed()) + Expect(checkIfStatusErrorWithMessage(notMasterUser.Delete(ctx, &w), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Delete, &workGVK, "", types.NamespacedName{Name: w.Name, Namespace: w.Namespace}))).Should(Succeed()) }) }) @@ -755,7 +755,7 @@ var _ = Describe("fleet guard rail for UPDATE work operations, in fleet prefixed } w.SetLabels(map[string]string{testKey: testValue}) By("expecting successful UPDATE of work") - return impersonateHubClient.Update(ctx, &w) + return notMasterUser.Update(ctx, &w) }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) @@ -778,7 +778,7 @@ var _ = Describe("fleet guard rail for UPDATE work operations, in fleet prefixed }, } By("expecting successful UPDATE of work Status") - return impersonateHubClient.Status().Update(ctx, &w) + return notMasterUser.Status().Update(ctx, &w) }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) @@ -827,7 +827,7 @@ var _ = Describe("fleet guard rail networking E2Es", Serial, Ordered, func() { } ise.SetLabels(map[string]string{testKey: testValue}) By("expecting denial of operation UPDATE of Internal Service Export") - err = impersonateHubClient.Update(ctx, &ise) + err = notMasterUser.Update(ctx, &ise) if k8sErrors.IsConflict(err) { return err } @@ -854,29 +854,29 @@ var _ = Describe("fleet guard rail networking E2Es", Serial, Ordered, func() { It("should allow CREATE operation on Internal service export resource in fleet-member namespace for user in member cluster identity", func() { ise := internalServiceExport(iseName, imcNamespace) By("expecting successful CREATE of Internal Service Export") - Expect(impersonateHubClient.Create(ctx, &ise)).Should(Succeed()) + Expect(notMasterUser.Create(ctx, &ise)).Should(Succeed()) By("deleting Internal Service Export") // Cleanup the internalServiceExport so that it won't block the member deletion. - Expect(impersonateHubClient.Delete(ctx, &ise)).Should(Succeed(), "failed to delete Internal Service Export") + Expect(notMasterUser.Delete(ctx, &ise)).Should(Succeed(), "failed to delete Internal Service Export") }) It("should allow CREATE operation on Internal service import resource in fleet-member namespace for user in member cluster identity", func() { isi := internalServiceImport(isiName, imcNamespace) By("expecting successful CREATE of Internal Service Import") - Expect(impersonateHubClient.Create(ctx, &isi)).Should(Succeed()) + Expect(notMasterUser.Create(ctx, &isi)).Should(Succeed()) By("deleting Internal Service Import") - Expect(impersonateHubClient.Delete(ctx, &isi)).Should(Succeed(), "failed to delete Internal Service Import") + Expect(notMasterUser.Delete(ctx, &isi)).Should(Succeed(), "failed to delete Internal Service Import") }) It("should allow CREATE operation on Endpoint slice export resource in fleet-member namespace for user in member cluster identity", func() { esx := endpointSliceExport(epName, imcNamespace) By("expecting successful CREATE of Endpoint slice export") - Expect(impersonateHubClient.Create(ctx, &esx)).Should(Succeed()) + Expect(notMasterUser.Create(ctx, &esx)).Should(Succeed()) By("deleting Endpoint Slice Export") - Expect(impersonateHubClient.Delete(ctx, &esx)).Should(Succeed(), "failed to delete EndpointSlice Export") + Expect(notMasterUser.Delete(ctx, &esx)).Should(Succeed(), "failed to delete EndpointSlice Export") }) }) @@ -910,7 +910,7 @@ var _ = Describe("fleet guard rail networking E2Es", Serial, Ordered, func() { } ise.SetLabels(map[string]string{testKey: testValue}) By("expecting denial of operation UPDATE of Internal Service Export") - return impersonateHubClient.Update(ctx, &ise) + return notMasterUser.Update(ctx, &ise) }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) }) @@ -929,12 +929,12 @@ var _ = Describe("fleet guard rail restrict internal fleet resources from being HeartbeatPeriodSeconds: 30, }, } - Expect(checkIfStatusErrorWithMessage(impersonateHubClient.Create(ctx, &imc), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Create, &imcGVK, "", types.NamespacedName{Name: imc.Name, Namespace: imc.Namespace}))).Should(Succeed()) + Expect(checkIfStatusErrorWithMessage(notMasterUser.Create(ctx, &imc), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Create, &imcGVK, "", types.NamespacedName{Name: imc.Name, Namespace: imc.Namespace}))).Should(Succeed()) }) }) It("should deny CREATE operation on internal service export resource in kube-system namespace for invalid user", func() { ise := internalServiceExport("test-ise", "kube-system") - Expect(checkIfStatusErrorWithMessage(impersonateHubClient.Create(ctx, &ise), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Create, &iseGVK, "", types.NamespacedName{Name: ise.Name, Namespace: ise.Namespace}))).Should(Succeed()) + Expect(checkIfStatusErrorWithMessage(notMasterUser.Create(ctx, &ise), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Create, &iseGVK, "", types.NamespacedName{Name: ise.Name, Namespace: ise.Namespace}))).Should(Succeed()) }) }) diff --git a/test/e2e/framework/cluster.go b/test/e2e/framework/cluster.go index 0357eb852..b9f3d7da0 100644 --- a/test/e2e/framework/cluster.go +++ b/test/e2e/framework/cluster.go @@ -45,6 +45,7 @@ type Cluster struct { HubURL string RestMapper meta.RESTMapper PricingProvider trackers.PricingProvider + SystemMastersClient client.Client } func NewCluster(name, svcAccountName string, scheme *runtime.Scheme, pp trackers.PricingProvider) *Cluster { @@ -60,6 +61,7 @@ func NewCluster(name, svcAccountName string, scheme *runtime.Scheme, pp trackers func GetClusterClient(cluster *Cluster) { clusterConfig := GetClientConfig(cluster) impersonateClusterConfig := GetImpersonateClientConfig(cluster) + systemMastersConfig := GetSystemMastersClientConfig(cluster) restConfig, err := clusterConfig.ClientConfig() if err != nil { @@ -71,6 +73,11 @@ func GetClusterClient(cluster *Cluster) { gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up impersonate rest config") } + systemMastersRestConfig, err := systemMastersConfig.ClientConfig() + if err != nil { + gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up impersonate rest config") + } + cluster.KubeClient, err = client.New(restConfig, client.Options{Scheme: cluster.Scheme}) gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Kube Client") @@ -85,9 +92,20 @@ func GetClusterClient(cluster *Cluster) { cluster.ImpersonateKubeClient, err = client.New(impersonateRestConfig, client.Options{Scheme: cluster.Scheme}) gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Impersonate Kube Client") + + cluster.SystemMastersClient, err = client.New(systemMastersRestConfig, client.Options{Scheme: cluster.Scheme}) + gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Impersonate Kube Client") } func GetClientConfig(cluster *Cluster) clientcmd.ClientConfig { + return clientcmd.NewNonInteractiveDeferredLoadingClientConfig( + &clientcmd.ClientConfigLoadingRules{ExplicitPath: kubeconfigPath}, + &clientcmd.ConfigOverrides{ + CurrentContext: cluster.ClusterName, + }) +} + +func GetSystemMastersClientConfig(cluster *Cluster) clientcmd.ClientConfig { return clientcmd.NewNonInteractiveDeferredLoadingClientConfig( &clientcmd.ClientConfigLoadingRules{ExplicitPath: kubeconfigPath}, &clientcmd.ConfigOverrides{ diff --git a/test/e2e/managed_resource_vap_test.go b/test/e2e/managed_resource_vap_test.go index ef5da74cf..d538d2576 100644 --- a/test/e2e/managed_resource_vap_test.go +++ b/test/e2e/managed_resource_vap_test.go @@ -62,7 +62,7 @@ func createUnmanagedNamespace(name string) *corev1.Namespace { func expectDeniedByVAP(err error) { var statusErr *k8sErrors.StatusError Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Expected StatusError, got error %s of type %s", err, reflect.TypeOf(err))) - Expect(statusErr.ErrStatus.Code).To(Equal(int32(http.StatusUnprocessableEntity)), "Expected 422 UnprocessableEntity") + Expect(statusErr.ErrStatus.Code).To(Equal(int32(http.StatusForbidden)), "Expected HTTP 403 Forbidden") // ValidatingAdmissionPolicy denials typically contain these patterns Expect(statusErr.ErrStatus.Message).To(SatisfyAny( ContainSubstring("ValidatingAdmissionPolicy"), @@ -74,82 +74,110 @@ func expectDeniedByVAP(err error) { var _ = Describe("ValidatingAdmissionPolicy for Managed Resources", Label("managedresource"), Ordered, func() { BeforeAll(func() { var vap admissionregistrationv1.ValidatingAdmissionPolicy - Expect(hubClient.Get(ctx, types.NamespacedName{Name: vapName}, &vap)).Should(Succeed(), "ValidatingAdmissionPolicy should be installed") + Expect(sysMastersClient.Get(ctx, types.NamespacedName{Name: vapName}, &vap)).Should(Succeed(), "ValidatingAdmissionPolicy should be installed") var vapBinding admissionregistrationv1.ValidatingAdmissionPolicyBinding - Expect(hubClient.Get(ctx, types.NamespacedName{Name: vapBindingName}, &vapBinding)).Should(Succeed(), "ValidatingAdmissionPolicyBinding should be installed") + Expect(sysMastersClient.Get(ctx, types.NamespacedName{Name: vapBindingName}, &vapBinding)).Should(Succeed(), "ValidatingAdmissionPolicyBinding should be installed") }) - Context("Namespace operations on managed-by label", 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) + }, testutils.PollTimeout, testutils.PollInterval).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 := impersonateHubClient.Create(ctx, managedNS) + err := notMasterUser.Create(ctx, managedNS) expectDeniedByVAP(err) }) - It("should deny UPDATE operation on managed namespace for non-system:masters user", func() { - managedNS := createManagedNamespace("test-managed-ns-update") - By("creating managed namespace with system:masters user") - Expect(hubClient.Create(ctx, managedNS)).To(Succeed()) + 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 Eventually(func() error { var ns corev1.Namespace - if err := hubClient.Get(ctx, types.NamespacedName{Name: managedNS.Name}, &ns); err != nil { + 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") - err := impersonateHubClient.Update(ctx, &ns) - if k8sErrors.IsConflict(err) { - return err + updateErr = notMasterUser.Update(ctx, &ns) + if k8sErrors.IsConflict(updateErr) { + return updateErr } - updateErr = err return nil }, testutils.PollTimeout, testutils.PollInterval).Should(Succeed()) expectDeniedByVAP(updateErr) - Expect(hubClient.Delete(ctx, managedNS)).To(Succeed()) }) - It("should deny DELETE operation on managed namespace for non-system:masters user", func() { - managedNS := createManagedNamespace("test-managed-ns-delete") - By("creating managed namespace with system:masters user") - Expect(hubClient.Create(ctx, managedNS)).To(Succeed()) - - By("expecting denial of DELETE operation on managed namespace") - err := impersonateHubClient.Delete(ctx, managedNS) - expectDeniedByVAP(err) - - Expect(hubClient.Delete(ctx, managedNS)).To(Succeed()) - }) - - 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(hubClient.Create(ctx, managedNS)).To(Succeed()) - - Expect(hubClient.Delete(ctx, managedNS)).To(Succeed()) - }) - - 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(impersonateHubClient.Create(ctx, unmanagedNS)).To(Succeed()) - - By("expecting successful UPDATE operation on unmanaged namespace") + 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 := impersonateHubClient.Get(ctx, types.NamespacedName{Name: unmanagedNS.Name}, &ns); err != nil { + if err := sysMastersClient.Get(ctx, types.NamespacedName{Name: managedNS.Name}, &ns); err != nil { return err } ns.Annotations = map[string]string{"test": "annotation"} - return impersonateHubClient.Update(ctx, &ns) + By("expecting denial of UPDATE operation on managed namespace") + updateErr = notMasterUser.Update(ctx, &ns) + if k8sErrors.IsConflict(updateErr) { + return updateErr + } + return nil }, testutils.PollTimeout, testutils.PollInterval).Should(Succeed()) + }) - By("expecting successful DELETE operation on unmanaged namespace") - Expect(impersonateHubClient.Delete(ctx, unmanagedNS)).To(Succeed()) + AfterAll(func() { + err := sysMastersClient.Delete(ctx, managedNS) + if err != nil { + Expect(k8sErrors.IsNotFound(err)).To(BeTrue()) + } }) }) }) diff --git a/test/e2e/setup_test.go b/test/e2e/setup_test.go index 13622005c..f0127b17d 100644 --- a/test/e2e/setup_test.go +++ b/test/e2e/setup_test.go @@ -107,7 +107,8 @@ var ( memberCluster3WestProd *framework.Cluster hubClient client.Client - impersonateHubClient client.Client + notMasterUser client.Client + sysMastersClient client.Client memberCluster1EastProdClient client.Client memberCluster2EastCanaryClient client.Client memberCluster3WestProdClient client.Client @@ -172,9 +173,7 @@ var ( } ) -var ( - fleetBinaryPath = filepath.Join("../../", "hack", "tools", "bin", "kubectl-fleet") -) +var fleetBinaryPath = filepath.Join("../../", "hack", "tools", "bin", "kubectl-fleet") var ( isAzurePropertyProviderEnabled = (os.Getenv(propertyProviderEnvVarName) == azurePropertyProviderEnvVarValue) @@ -334,8 +333,10 @@ func beforeSuiteForAllProcesses() { framework.GetClusterClient(hubCluster) hubClient = hubCluster.KubeClient Expect(hubClient).NotTo(BeNil(), "Failed to initialize client for accessing Kubernetes cluster") - impersonateHubClient = hubCluster.ImpersonateKubeClient - Expect(impersonateHubClient).NotTo(BeNil(), "Failed to initialize impersonate client for accessing Kubernetes cluster") + notMasterUser = hubCluster.ImpersonateKubeClient + Expect(notMasterUser).NotTo(BeNil(), "Failed to initialize impersonate client for accessing Kubernetes cluster") + sysMastersClient = hubCluster.SystemMastersClient + Expect(notMasterUser).NotTo(BeNil(), "Failed to initialize impersonate client for accessing Kubernetes cluster") var pricingProvider1 trackers.PricingProvider if isAzurePropertyProviderEnabled {