From 840e808c943503d490a221229519f86749281c6e Mon Sep 17 00:00:00 2001 From: Nont Date: Fri, 17 Oct 2025 20:01:22 -0500 Subject: [PATCH 1/4] Tighten the VAP rule for all managed resources Signed-off-by: Nont --- .../templates/serviceaccount.yaml | 2 + .../managedresource/createordelete_test.go | 4 + .../validatingadmissionpolicy.go | 52 +- .../validatingadmissionpolicy_test.go | 84 ++- test/e2e/framework/cluster.go | 6 +- test/e2e/managed_resource_vap_test.go | 549 +++++++++++------- 6 files changed, 406 insertions(+), 291 deletions(-) diff --git a/charts/member-agent-arc/templates/serviceaccount.yaml b/charts/member-agent-arc/templates/serviceaccount.yaml index bbc3309a7..73f38d686 100644 --- a/charts/member-agent-arc/templates/serviceaccount.yaml +++ b/charts/member-agent-arc/templates/serviceaccount.yaml @@ -1,6 +1,8 @@ apiVersion: v1 kind: ServiceAccount metadata: + # the name is also used in the managed resource validating admission webhook. + # please make the change accordingly when you change the name. name: fleet-member-agent-sa namespace: fleet-system labels: diff --git a/pkg/webhook/managedresource/createordelete_test.go b/pkg/webhook/managedresource/createordelete_test.go index 5609e613f..dac688649 100644 --- a/pkg/webhook/managedresource/createordelete_test.go +++ b/pkg/webhook/managedresource/createordelete_test.go @@ -371,9 +371,11 @@ func TestGetVAPWithMutator(t *testing.T) { // Verify initial state if vap == nil { t.Fatal("getVAPWithMutator() returned nil VAP") + return } if mutateFunc == nil { t.Fatal("getVAPWithMutator() returned nil mutate function") + return } // Verify mutate function works @@ -411,9 +413,11 @@ func TestGetVAPBindingWithMutator(t *testing.T) { // Verify initial state if vapb == nil { t.Fatal("getVAPBindingWithMutator() returned nil VAP binding") + return } if mutateFunc == nil { t.Fatal("getVAPBindingWithMutator() returned nil mutate function") + return } // Verify mutate function works diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy.go b/pkg/webhook/managedresource/validatingadmissionpolicy.go index 011ff96e0..cf8f042c5 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy.go @@ -42,58 +42,30 @@ func mutateValidatingAdmissionPolicy(vap *admv1.ValidatingAdmissionPolicy, isHub { 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}, - }, - ResourceNames: []string{"default"}, - }, - { - RuleWithOperations: admv1.RuleWithOperations{ - Rule: admv1.Rule{ - APIGroups: []string{"networking.k8s.io"}, - Resources: []string{"networkpolicies"}, + APIGroups: []string{"*"}, + Resources: []string{"*"}, 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 || "system:serviceaccounts:fleet-system" in request.userInfo.groups || "system:serviceaccounts:openshift-kube-controller-manager" in request.userInfo.groups`, - Message: "Create, Update, or Delete operations on ARM managed resources is forbidden", - Reason: &forbidden, + Expression: `( + request.userInfo.username == "aksService" || + request.userInfo.username == "fleet-member-agent-sa") + && ( + "system:masters" in request.userInfo.groups || + "system:serviceaccounts:kube-system" in request.userInfo.groups || + "system:serviceaccounts:fleet-system" in request.userInfo.groups || + "system:serviceaccounts:openshift-kube-controller-manager" in request.userInfo.groups)`, + Message: "Create, Update, or Delete operations on ARM managed resources is forbidden", + Reason: &forbidden, }, }, } - - 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{"*"}, - APIVersions: []string{"*"}, - }, - Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, - }, - }) - } } func getValidatingAdmissionPolicyBinding() *admv1.ValidatingAdmissionPolicyBinding { diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy_test.go b/pkg/webhook/managedresource/validatingadmissionpolicy_test.go index 935085076..f1a2d9e70 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy_test.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy_test.go @@ -16,60 +16,52 @@ import ( func TestGetValidatingAdmissionPolicy(t *testing.T) { t.Parallel() - t.Run("member", func(t *testing.T) { - t.Parallel() + tests := []struct { + name string + isHub bool + }{ + { + name: "member cluster", + isHub: false, + }, + { + name: "hub cluster", + isHub: true, + }, + } - vap := getValidatingAdmissionPolicy(false) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() - unwantedRule := admv1.NamedRuleWithOperations{ - RuleWithOperations: admv1.RuleWithOperations{ - Rule: admv1.Rule{ - APIGroups: []string{"placement.kubernetes-fleet.io"}, - Resources: []string{"clusterresourceplacements"}, - APIVersions: []string{"*"}, + vap := getValidatingAdmissionPolicy(tt.isHub) + + // Both hub and member clusters should have the same single rule that covers all resources + expectedRule := admv1.NamedRuleWithOperations{ + RuleWithOperations: admv1.RuleWithOperations{ + Rule: admv1.Rule{ + APIGroups: []string{"*"}, + Resources: []string{"*"}, + APIVersions: []string{"*"}, + }, + Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete}, }, - 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() + if vap.Spec.MatchConstraints == nil { + t.Fatal("MatchConstraints should not be nil") + } - vap := getValidatingAdmissionPolicy(true) + if len(vap.Spec.MatchConstraints.ResourceRules) != 1 { + t.Errorf("Expected exactly 1 resource rule, got %d", len(vap.Spec.MatchConstraints.ResourceRules)) + } - wantedRule := admv1.NamedRuleWithOperations{ - RuleWithOperations: admv1.RuleWithOperations{ - Rule: admv1.Rule{ - APIGroups: []string{"placement.kubernetes-fleet.io"}, - Resources: []string{"*"}, - APIVersions: []string{"*"}, - }, - 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 - } + actualRule := vap.Spec.MatchConstraints.ResourceRules[0] + if diff := cmp.Diff(expectedRule, actualRule); diff != "" { + t.Errorf("Resource rule mismatch (-want +got):\n%s", diff) } - } - if !found { - t.Errorf("getValidatingAdmissionPolicy(true) missing expected rule %+v", wantedRule) - } - }) + }) + } } func TestMutateValidatingAdmissionPolicy(t *testing.T) { diff --git a/test/e2e/framework/cluster.go b/test/e2e/framework/cluster.go index 659dbc0ec..9c15a33a1 100644 --- a/test/e2e/framework/cluster.go +++ b/test/e2e/framework/cluster.go @@ -62,7 +62,7 @@ func NewCluster(name, svcAccountName string, scheme *runtime.Scheme, pp trackers func GetClusterClient(cluster *Cluster) { clusterConfig := GetClientConfig(cluster) impersonateClusterConfig := GetImpersonateClientConfig(cluster) - systemMastersConfig := GetSystemMastersClientConfig(cluster) + systemMastersConfig := GetAKSServiceSystemMastersClientConfig(cluster) restConfig, err := clusterConfig.ClientConfig() if err != nil { @@ -106,13 +106,13 @@ func GetClientConfig(cluster *Cluster) clientcmd.ClientConfig { }) } -func GetSystemMastersClientConfig(cluster *Cluster) clientcmd.ClientConfig { +func GetAKSServiceSystemMastersClientConfig(cluster *Cluster) clientcmd.ClientConfig { return clientcmd.NewNonInteractiveDeferredLoadingClientConfig( &clientcmd.ClientConfigLoadingRules{ExplicitPath: kubeconfigPath}, &clientcmd.ConfigOverrides{ CurrentContext: cluster.ClusterName, AuthInfo: api.AuthInfo{ - Impersonate: "system", + Impersonate: "aksService", ImpersonateGroups: []string{"system:masters"}, }, }) diff --git a/test/e2e/managed_resource_vap_test.go b/test/e2e/managed_resource_vap_test.go index ca2c963cd..f853ab769 100644 --- a/test/e2e/managed_resource_vap_test.go +++ b/test/e2e/managed_resource_vap_test.go @@ -18,7 +18,6 @@ package e2e import ( "errors" "fmt" - "net/http" "reflect" . "github.com/onsi/ginkgo/v2" @@ -26,13 +25,19 @@ import ( admissionregistrationv1 "k8s.io/api/admissionregistration/v1" corev1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" + rbacv1 "k8s.io/api/rbac/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/version" "k8s.io/client-go/discovery" + "sigs.k8s.io/controller-runtime/pkg/client" + placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" "go.goms.io/fleet/test/e2e/framework" ) @@ -48,19 +53,132 @@ var managedByLabelMap = map[string]string{ managedByLabel: managedByLabelValue, } +// Need this because Entry() evaluates parameters at definition time, not at runtime. +// Without this, the client value sent to Entry() would always be nil. +func getUserClient() client.Client { + return hubCluster.ImpersonateKubeClient +} + +func getAksServiceClient() client.Client { + return hubCluster.SystemMastersClient +} + +var _ = Describe("ValidatingAdmissionPolicy for Managed Resources", Label("managedresource"), Ordered, 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("Baseline: Unmanaged resources", func() { + DescribeTable("should allow all operations on unmanaged resources", + func(resourceType string, clientFunc func() client.Client, suffix string) { + client := clientFunc() // Call the function to get the client at runtime + + namespace := "default" + switch resourceType { + case "namespace", "clusterrole", "crp": + namespace = "" + } + resource := createResource(resourceType, "test-unmanaged-"+suffix, namespace, false) + testResourceOperations(client, resource, true, resourceType) + }, + Entry("allowed: namespace by non-aksService user", "namespace", getUserClient, "ns-user"), + Entry("allowed: namespace by aksService user", "namespace", getAksServiceClient, "ns-aks"), + Entry("allowed: configmap by non-aksService user", "configmap", getUserClient, "cm-user"), + Entry("allowed: configmap by aksService user", "configmap", getAksServiceClient, "cm-aks"), + Entry("allowed: resourcequota by non-aksService user", "resourcequota", getUserClient, "rq-user"), + Entry("allowed: resourcequota by aksService user", "resourcequota", getAksServiceClient, "rq-aks"), + Entry("allowed: networkpolicy by non-aksService user", "networkpolicy", getUserClient, "np-user"), + Entry("allowed: networkpolicy by aksService user", "networkpolicy", getAksServiceClient, "np-aks"), + Entry("allowed: clusterrole by non-aksService user", "clusterrole", getUserClient, "cr-user"), + Entry("allowed: clusterrole by aksService user", "clusterrole", getAksServiceClient, "cr-aks"), + Entry("allowed: crp by non-aksService user", "crp", getUserClient, "crp-user"), + Entry("allowed: crp by aksService user", "crp", getAksServiceClient, "crp-aks"), + ) + }) + + Context("Core resource validation", func() { + describeResourceContexts([]struct{ resourceType, testPrefix, namespace, contextName string }{ + {"namespace", "test-namespace", "", "Cluster-scoped resources"}, + {"configmap", "test-configmap", "default", "Namespaced resources"}, + {"clusterrole", "test-clusterrole", "", "RBAC resources"}, + }) + }) + + Context("Resources for Fleet managed namespaces", func() { + describeResourceContexts([]struct{ resourceType, testPrefix, namespace, contextName string }{ + {"crp", "test-crp", "", "ClusterResourcePlacement"}, + {"namespace", "test-fleet-ns", "", "Namespace"}, + {"resourcequota", "test-quota", "default", "ResourceQuota"}, + {"networkpolicy", "test-netpol", "default", "NetworkPolicy"}, + }) + }) + + Context("Dynamic resource types", func() { + Context("Custom Resource Definitions", Ordered, func() { + var testCRD *apiextensionsv1.CustomResourceDefinition + + BeforeAll(func() { + testCRD = createTestCRD() + authorizedClient := hubCluster.SystemMastersClient + Expect(authorizedClient.Create(ctx, testCRD)).To(Succeed()) + // Wait for CRD to be established + Eventually(func() bool { + var crd apiextensionsv1.CustomResourceDefinition + err := authorizedClient.Get(ctx, types.NamespacedName{Name: testCRD.Name}, &crd) + if err != nil { + return false + } + for _, condition := range crd.Status.Conditions { + if condition.Type == apiextensionsv1.Established && condition.Status == apiextensionsv1.ConditionTrue { + return true + } + } + return false + }, eventuallyDuration, eventuallyInterval).Should(BeTrue()) + }) + + DescribeTable("customresource operations", + func(userType string, clientFunc func() client.Client, shouldSucceed bool, suffix string) { + client := clientFunc() // Call the function to get the client at runtime + resourceName := fmt.Sprintf("test-cr-%s", suffix) + resource := createResource("customresource", resourceName, "default", true) + testResourceOperations(client, resource, shouldSucceed, "customresource") + }, + Entry("non-aksService user", "unauthorized", getUserClient, false, "deny"), + Entry("aksService user", "authorized", getAksServiceClient, true, "allow"), + ) + + AfterAll(func() { + if testCRD != nil { + authorizedClient := hubCluster.SystemMastersClient + Expect(authorizedClient.Delete(ctx, testCRD)).To(Succeed()) + } + }) + }) + + }) + + Context("Policy infrastructure validation", func() { + It("should have proper VAP and binding configuration", func() { + checkVAPAndBindingExistence(hubCluster) + }) + }) +}) + // Helper functions for creating managed resources -func createUnmanagedNamespace(name string) *corev1.Namespace { +func createManagedNamespace(name string) *corev1.Namespace { return &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: name, + Name: name, + Labels: managedByLabelMap, }, } } -func createManagedNamespace(name string) *corev1.Namespace { - ns := createUnmanagedNamespace(name) - ns.Labels = managedByLabelMap - return ns -} func createManagedResourceQuota(ns, name string) *corev1.ResourceQuota { return &corev1.ResourceQuota{ @@ -69,6 +187,11 @@ func createManagedResourceQuota(ns, name string) *corev1.ResourceQuota { Namespace: ns, Labels: managedByLabelMap, }, + Spec: corev1.ResourceQuotaSpec{ + Hard: corev1.ResourceList{ + "pods": resource.MustParse("10"), + }, + }, } } @@ -79,6 +202,10 @@ func createManagedNetworkPolicy(ns, name string) *networkingv1.NetworkPolicy { Namespace: ns, Labels: managedByLabelMap, }, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + }, } } @@ -100,35 +227,231 @@ func createManagedCRP(name string) *placementv1beta1.ClusterResourcePlacement { } } -func createManagedResourcePlacement(name string) *placementv1beta1.ResourcePlacement { - return &placementv1beta1.ResourcePlacement{ +func createManagedConfigMap(ns, name string) *corev1.ConfigMap { + return &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: name, - Namespace: "default", + Namespace: ns, Labels: managedByLabelMap, }, - Spec: placementv1beta1.PlacementSpec{ - ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{ + Data: map[string]string{"key": "value"}, + } +} + +func createManagedClusterRole(name string) *rbacv1.ClusterRole { + return &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: managedByLabelMap, + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"pods"}, + Verbs: []string{"get", "list"}, + }, + }, + } +} + +func createTestCRD() *apiextensionsv1.CustomResourceDefinition { + return &apiextensionsv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testresources.example.com", + }, + Spec: apiextensionsv1.CustomResourceDefinitionSpec{ + Group: "example.com", + Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ { - Group: "", - Version: "v1", - Kind: "Pod", + Name: "v1", + Served: true, + Storage: true, + Schema: &apiextensionsv1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "spec": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "field": {Type: "string"}, + }, + }, + }, + }, + }, }, }, + Scope: apiextensionsv1.NamespaceScoped, + Names: apiextensionsv1.CustomResourceDefinitionNames{ + Plural: "testresources", + Singular: "testresource", + Kind: "TestResource", + }, }, } } +func createManagedCustomResource(ns, name string) *unstructured.Unstructured { + cr := &unstructured.Unstructured{} + cr.SetAPIVersion("example.com/v1") + cr.SetKind("TestResource") + cr.SetName(name) + cr.SetNamespace(ns) + cr.SetLabels(managedByLabelMap) + cr.Object["spec"] = map[string]interface{}{ + "field": "test-value", + } + return cr +} + func expectDeniedByVAP(err error) { + Expect(k8sErrors.IsForbidden(err)).To(BeTrue(), fmt.Sprintf("Expected Forbidden error, got error %s of type %s", err, reflect.TypeOf(err))) + 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.StatusForbidden)), "Expected HTTP 403 Forbidden") - // 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") + if errors.As(err, &statusErr) { + Expect(statusErr.ErrStatus.Message).To(SatisfyAny( + ContainSubstring("ValidatingAdmissionPolicy"), + ContainSubstring("denied the request"), + ContainSubstring("violates policy"), + ), "Error should indicate policy violation") + } +} + +// Generic test helper for creating resources with optional managed labels +func createResource(resourceType string, name, namespace string, managed bool) client.Object { + var resource client.Object + + // Always create the managed version first + switch resourceType { + case "namespace": + resource = createManagedNamespace(name) + case "configmap": + resource = createManagedConfigMap(namespace, name) + case "clusterrole": + resource = createManagedClusterRole(name) + case "resourcequota": + resource = createManagedResourceQuota(namespace, name) + case "networkpolicy": + resource = createManagedNetworkPolicy(namespace, name) + case "crp": + resource = createManagedCRP(name) + case "customresource": + resource = createManagedCustomResource(namespace, name) + default: + panic(fmt.Sprintf("Unsupported resource type: %s", resourceType)) + } + + if !managed { + resource.SetLabels(nil) + } + + return resource +} + +func describeResourceContexts(resources []struct{ resourceType, testPrefix, namespace, contextName string }) { + for _, r := range resources { + Context(r.contextName, func() { + DescribeTable(fmt.Sprintf("%s operations", r.resourceType), + func(userType string, clientFunc func() client.Client, shouldSucceed bool, suffix string) { + client := clientFunc() // Call the function to get the client at runtime + resourceName := fmt.Sprintf("%s-%s", r.testPrefix, suffix) + resource := createResource(r.resourceType, resourceName, r.namespace, true) + testResourceOperations(client, resource, shouldSucceed, r.resourceType) + }, + Entry("non-aksService user", "unauthorized", getUserClient, false, "deny"), + Entry("aksService user", "authorized", getAksServiceClient, true, "allow"), + ) + }) + } +} + +func testResourceOperations(client client.Client, resource client.Object, shouldSucceed bool, resourceType string) { + resourceKind := resource.GetObjectKind().GroupVersionKind().Kind + if resourceKind == "" { + // Fallback for resources that don't have GVK set + resourceKind = fmt.Sprintf("%T", resource) + } + + if shouldSucceed { + By(fmt.Sprintf("Creating %s", resourceKind)) + Expect(client.Create(ctx, resource)).To(Succeed()) + + By(fmt.Sprintf("Updating %s", resourceKind)) + Eventually(func() error { + // Get latest version to avoid conflicts + key := types.NamespacedName{ + Name: resource.GetName(), + Namespace: resource.GetNamespace(), + } + if err := client.Get(ctx, key, resource); err != nil { + return err + } + + annotations := resource.GetAnnotations() + if annotations == nil { + annotations = make(map[string]string) + } + annotations["test"] = "annotation" + resource.SetAnnotations(annotations) + return client.Update(ctx, resource) + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + + By(fmt.Sprintf("Deleting %s", resourceKind)) + Expect(client.Delete(ctx, resource)).To(Succeed()) + } else { + By(fmt.Sprintf("Expecting denial of CREATE operation on %s", resourceKind)) + err := client.Create(ctx, resource) + expectDeniedByVAP(err) + + // For UPDATE and DELETE tests, we need an existing resource first + By(fmt.Sprintf("Creating %s with authorized client for UPDATE/DELETE tests", resourceKind)) + + // Use a more unique name to avoid conflicts in parallel test execution + resourceName := fmt.Sprintf("%s-for-update-delete-%d", resource.GetName(), GinkgoRandomSeed()) + existingResource := createResource(resourceType, resourceName, resource.GetNamespace(), true) + authorizedClient := hubCluster.SystemMastersClient + Expect(authorizedClient.Create(ctx, existingResource)).To(Succeed()) + + // Test UPDATE denial + By(fmt.Sprintf("Expecting denial of UPDATE operation on %s", resourceKind)) + Eventually(func() error { + // Create a fresh object and get the latest version to have proper resourceVersion + updateResource := createResource(resourceType, existingResource.GetName(), existingResource.GetNamespace(), true) + key := types.NamespacedName{ + Name: existingResource.GetName(), + Namespace: existingResource.GetNamespace(), + } + if err := client.Get(ctx, key, updateResource); err != nil { + return err + } + + annotations := updateResource.GetAnnotations() + if annotations == nil { + annotations = make(map[string]string) + } + annotations["unauthorized-update"] = "should-fail" + updateResource.SetAnnotations(annotations) + + err := client.Update(ctx, updateResource) + + // If it's a conflict error (409), retry + if k8sErrors.IsConflict(err) { + return err + } + + // Otherwise, we expect it to be denied by VAP (403) + expectDeniedByVAP(err) + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + + By(fmt.Sprintf("Expecting denial of DELETE operation on %s", resourceKind)) + err = client.Delete(ctx, existingResource) + expectDeniedByVAP(err) + + // Clean up the test resource using authorized client + By(fmt.Sprintf("Cleaning up test %s with authorized client", resourceKind)) + Expect(authorizedClient.Delete(ctx, existingResource)).To(Succeed()) + } } func checkVAPAndBindingExistence(c *framework.Cluster) { @@ -188,184 +511,6 @@ func checkVAPAndBindingAbsence(c *framework.Cluster) { fmt.Sprintf("ValidatingAdmissionPolicyBinding %s should not exist", vapName)) } -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()) - - 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("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("When the namespace doesn't exist", func() { - It("should deny CREATE operation on managed namespace for non-system:masters user", func() { - managedNS := createManagedNamespace("test-managed-ns-create") - By("expecting denial of CREATE operation on managed namespace") - err := notMasterUser.Create(ctx, managedNS) - expectDeniedByVAP(err) - }) - - It("should allow CREATE operation on managed namespace for system:masters user", func() { - managedNS := createManagedNamespace("test-managed-ns-masters") - By("expecting successful CREATE operation with system:masters user") - Expect(sysMastersClient.Create(ctx, managedNS)).To(Succeed()) - - Expect(sysMastersClient.Delete(ctx, managedNS)).To(Succeed()) - }) - }) - - Context("When the namespace exists", Ordered, func() { - managedNS := createManagedNamespace("test-managed-ns-update") - BeforeAll(func() { - err := sysMastersClient.Delete(ctx, managedNS) - if err != nil { - Expect(k8sErrors.IsNotFound(err)).To(BeTrue()) - } - Expect(sysMastersClient.Create(ctx, managedNS)).To(Succeed()) - var ns corev1.Namespace - err = sysMastersClient.Get(ctx, types.NamespacedName{Name: managedNS.Name}, &ns) - Expect(err).To(BeNil()) - Expect(ns.Labels).To(HaveKeyWithValue(managedByLabel, managedByLabelValue)) - }) - - It("should deny DELETE operation on managed namespace for non-system:masters user", func() { - By("expecting denial of DELETE operation on managed namespace") - err := notMasterUser.Delete(ctx, managedNS) - expectDeniedByVAP(err) - }) - - It("should deny UPDATE operation on managed namespace for non-system:masters user", func() { - var updateErr error - 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 = notMasterUser.Update(ctx, &ns) - if k8sErrors.IsConflict(updateErr) { - return updateErr - } - return nil - }, eventuallyDuration, eventuallyInterval).Should(Succeed()) - - expectDeniedByVAP(updateErr) - }) - - 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()) - }) - - 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()) - } - }) - }) - }) -}) - // 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() From d31bf8e25e152378127132fc52c5a933fdd6a55c Mon Sep 17 00:00:00 2001 From: Nont Date: Tue, 21 Oct 2025 13:38:18 -0500 Subject: [PATCH 2/4] Remove isHub parameter Signed-off-by: Nont --- cmd/hubagent/main.go | 2 +- .../v1beta1/member_controller.go | 4 +- pkg/webhook/managedresource/createordelete.go | 14 +-- .../managedresource/createordelete_test.go | 98 ++++++------------- .../validatingadmissionpolicy.go | 6 +- .../validatingadmissionpolicy_test.go | 28 ++---- 6 files changed, 48 insertions(+), 104 deletions(-) diff --git a/cmd/hubagent/main.go b/cmd/hubagent/main.go index 1e1d2071d..080aca57c 100644 --- a/cmd/hubagent/main.go +++ b/cmd/hubagent/main.go @@ -205,7 +205,7 @@ func main() { exitWithErrorFunc() } - if err := managedresource.EnsureVAP(ctx, mgr.GetClient(), true); err != nil { + if err := managedresource.EnsureVAP(ctx, mgr.GetClient()); err != nil { klog.Errorf("unable to create managed resource validating admission policy: %s", err) exitWithErrorFunc() } diff --git a/pkg/controllers/internalmembercluster/v1beta1/member_controller.go b/pkg/controllers/internalmembercluster/v1beta1/member_controller.go index 4419477ea..884ba3d7d 100644 --- a/pkg/controllers/internalmembercluster/v1beta1/member_controller.go +++ b/pkg/controllers/internalmembercluster/v1beta1/member_controller.go @@ -214,7 +214,7 @@ 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 { + if err := managedresource.EnsureVAP(ctx, r.memberClient); err != nil { return ctrl.Result{}, err } klog.V(2).InfoS("Successfully installed managed resource validating admission policy") @@ -247,7 +247,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu 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 { + if err := managedresource.EnsureNoVAP(ctx, r.memberClient); err != nil { return ctrl.Result{}, err } klog.V(2).InfoS("Successfully uninstalled managed resource validating admission policy") diff --git a/pkg/webhook/managedresource/createordelete.go b/pkg/webhook/managedresource/createordelete.go index 206a01b13..0e661b45e 100644 --- a/pkg/webhook/managedresource/createordelete.go +++ b/pkg/webhook/managedresource/createordelete.go @@ -16,8 +16,8 @@ import ( "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()} +func EnsureNoVAP(ctx context.Context, c client.Client) error { + objs := []client.Object{getValidatingAdmissionPolicy(), getValidatingAdmissionPolicyBinding()} for _, obj := range objs { err := c.Delete(ctx, obj) switch { @@ -34,13 +34,13 @@ func EnsureNoVAP(ctx context.Context, c client.Client, isHub bool) error { return nil } -func EnsureVAP(ctx context.Context, c client.Client, isHub bool) error { +func EnsureVAP(ctx context.Context, c client.Client) 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) + vap, mutateVAP := getVAPWithMutator() vapb, mutateVAPB := getVAPBindingWithMutator() objsAndMutators := []vapObjectAndMutator{ { @@ -69,10 +69,10 @@ func EnsureVAP(ctx context.Context, c client.Client, isHub bool) error { return nil } -func getVAPWithMutator(isHub bool) (*v1.ValidatingAdmissionPolicy, func() error) { - vap := getValidatingAdmissionPolicy(isHub) +func getVAPWithMutator() (*v1.ValidatingAdmissionPolicy, func() error) { + vap := getValidatingAdmissionPolicy() return vap, func() error { - mutateValidatingAdmissionPolicy(vap, isHub) + mutateValidatingAdmissionPolicy(vap) return nil } } diff --git a/pkg/webhook/managedresource/createordelete_test.go b/pkg/webhook/managedresource/createordelete_test.go index dac688649..548b3fd79 100644 --- a/pkg/webhook/managedresource/createordelete_test.go +++ b/pkg/webhook/managedresource/createordelete_test.go @@ -32,13 +32,11 @@ func TestEnsureVAP(t *testing.T) { t.Fatalf("Failed to add admissionregistration scheme: %v", err) } - vapHub := getValidatingAdmissionPolicy(true) - vapMember := getValidatingAdmissionPolicy(false) + vap := getValidatingAdmissionPolicy() binding := getValidatingAdmissionPolicyBinding() tests := []struct { name string - isHub bool existingObjs []client.Object createOrUpdateErrors map[client.ObjectKey]error wantErr bool @@ -46,30 +44,18 @@ func TestEnsureVAP(t *testing.T) { wantObjects []client.Object }{ { - name: "hub cluster - create new objects", - isHub: true, + name: "create new objects", existingObjs: []client.Object{}, wantErr: false, wantObjects: []client.Object{ - vapHub.DeepCopy(), + vap.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, + name: "update existing objects", existingObjs: func() []client.Object { - existingVAP := vapHub.DeepCopy() + existingVAP := vap.DeepCopy() existingBinding := binding.DeepCopy() existingVAP.Spec.Validations = nil @@ -79,32 +65,29 @@ func TestEnsureVAP(t *testing.T) { }(), wantErr: false, wantObjects: []client.Object{ - vapHub.DeepCopy(), + vap.DeepCopy(), binding.DeepCopy(), }, }, { - name: "hub cluster - skip no match error", - isHub: true, + name: "skip no match error (cluster version < v1.30)", existingObjs: []client.Object{}, createOrUpdateErrors: map[client.ObjectKey]error{ - client.ObjectKeyFromObject(vapHub): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}}, + client.ObjectKeyFromObject(vap): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}}, }, wantErr: false, }, { - name: "hub cluster - create error propagated", - isHub: true, + name: "vap create error", existingObjs: []client.Object{}, createOrUpdateErrors: map[client.ObjectKey]error{ - client.ObjectKeyFromObject(vapHub): apierrors.NewInternalError(errors.New("internal server error")), + client.ObjectKeyFromObject(vap): apierrors.NewInternalError(errors.New("internal server error")), }, wantErr: true, wantErrMessage: "internal server error", }, { - name: "member cluster - binding creation error", - isHub: false, + name: "binding create error", 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")), @@ -173,7 +156,7 @@ func TestEnsureVAP(t *testing.T) { WithInterceptorFuncs(interceptorFuncs). Build() - err := EnsureVAP(context.Background(), fakeClient, tt.isHub) + err := EnsureVAP(context.Background(), fakeClient) if tt.wantErr { if err == nil { @@ -219,74 +202,57 @@ func TestEnsureNoVAP(t *testing.T) { t.Fatalf("Failed to add admissionregistration scheme: %v", err) } - vapHub := getValidatingAdmissionPolicy(true) - vapMember := getValidatingAdmissionPolicy(false) + vap := getValidatingAdmissionPolicy() 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, + name: "no existing objects", existingObjs: []client.Object{}, wantErr: false, }, { - name: "hub cluster - existing objects deleted successfully", - isHub: true, + name: "existing objects deleted successfully", existingObjs: []client.Object{ - vapHub.DeepCopy(), + vap.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, + name: "not found errors ignored", 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(vap): apierrors.NewNotFound(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicies"}, vap.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, + name: "no match error handled gracefully", existingObjs: []client.Object{}, deleteErrors: map[client.ObjectKey]error{ - client.ObjectKeyFromObject(vapHub): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}}, + client.ObjectKeyFromObject(vap): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}}, }, wantErr: false, }, { - name: "hub cluster - delete error", - isHub: true, + name: "delete error on vap", existingObjs: []client.Object{}, deleteErrors: map[client.ObjectKey]error{ - client.ObjectKeyFromObject(vapHub): apierrors.NewInternalError(errors.New("internal server error")), + client.ObjectKeyFromObject(vap): apierrors.NewInternalError(errors.New("internal server error")), }, wantErr: true, wantErrMessage: "internal server error", }, { - name: "member cluster - delete error on binding propagated", - isHub: false, + name: "delete error on vap binding", 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")), @@ -316,7 +282,7 @@ func TestEnsureNoVAP(t *testing.T) { WithInterceptorFuncs(interceptorFuncs). Build() - err := EnsureNoVAP(context.Background(), fakeClient, tt.isHub) + err := EnsureNoVAP(context.Background(), fakeClient) if tt.wantErr { if err == nil { @@ -334,7 +300,7 @@ func TestEnsureNoVAP(t *testing.T) { } // Verify objects are deleted (or don't exist) - expectedObjs := []client.Object{getValidatingAdmissionPolicy(tt.isHub), getValidatingAdmissionPolicyBinding()} + expectedObjs := []client.Object{getValidatingAdmissionPolicy(), getValidatingAdmissionPolicyBinding()} for _, obj := range expectedObjs { err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(obj), obj) if !apierrors.IsNotFound(err) { @@ -349,16 +315,10 @@ func TestGetVAPWithMutator(t *testing.T) { t.Parallel() tests := []struct { - name string - isHub bool + name string }{ { - name: "hub cluster VAP", - isHub: true, - }, - { - name: "member cluster VAP", - isHub: false, + name: "VAP with mutator", }, } @@ -366,7 +326,7 @@ func TestGetVAPWithMutator(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - vap, mutateFunc := getVAPWithMutator(tt.isHub) + vap, mutateFunc := getVAPWithMutator() // Verify initial state if vap == nil { @@ -380,7 +340,7 @@ func TestGetVAPWithMutator(t *testing.T) { // Verify mutate function works gotVAP := vap.DeepCopy() - wantVAP := getValidatingAdmissionPolicy(tt.isHub) + wantVAP := getValidatingAdmissionPolicy() ignoreOpts := cmp.Options{ cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation"), } diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy.go b/pkg/webhook/managedresource/validatingadmissionpolicy.go index cf8f042c5..d351e1fd9 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy.go @@ -16,13 +16,13 @@ const resourceName = "aks-fleet-managed-by-arm" var forbidden = metav1.StatusReasonForbidden -func getValidatingAdmissionPolicy(isHub bool) *admv1.ValidatingAdmissionPolicy { +func getValidatingAdmissionPolicy() *admv1.ValidatingAdmissionPolicy { vap := &admv1.ValidatingAdmissionPolicy{} - mutateValidatingAdmissionPolicy(vap, isHub) + mutateValidatingAdmissionPolicy(vap) return vap } -func mutateValidatingAdmissionPolicy(vap *admv1.ValidatingAdmissionPolicy, isHub bool) { +func mutateValidatingAdmissionPolicy(vap *admv1.ValidatingAdmissionPolicy) { ometa := metav1.ObjectMeta{ Name: resourceName, Labels: map[string]string{ diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy_test.go b/pkg/webhook/managedresource/validatingadmissionpolicy_test.go index f1a2d9e70..0a74a06ba 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy_test.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy_test.go @@ -17,16 +17,10 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { t.Parallel() tests := []struct { - name string - isHub bool + name string }{ { - name: "member cluster", - isHub: false, - }, - { - name: "hub cluster", - isHub: true, + name: "validating admission policy", }, } @@ -34,9 +28,9 @@ func TestGetValidatingAdmissionPolicy(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - vap := getValidatingAdmissionPolicy(tt.isHub) + vap := getValidatingAdmissionPolicy() - // Both hub and member clusters should have the same single rule that covers all resources + // Should have a single rule that covers all resources expectedRule := admv1.NamedRuleWithOperations{ RuleWithOperations: admv1.RuleWithOperations{ Rule: admv1.Rule{ @@ -69,31 +63,21 @@ func TestMutateValidatingAdmissionPolicy(t *testing.T) { tests := []struct { name string - isHub bool resourceVersion string initialLabels map[string]string }{ { - name: "preserves ResourceVersion and updates labels for member cluster", - isHub: false, + name: "preserves ResourceVersion and updates labels", 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"}, }, @@ -111,7 +95,7 @@ func TestMutateValidatingAdmissionPolicy(t *testing.T) { }, } - mutateValidatingAdmissionPolicy(vap, tt.isHub) + mutateValidatingAdmissionPolicy(vap) if vap.ResourceVersion != tt.resourceVersion { t.Errorf("mutateValidatingAdmissionPolicy() ResourceVersion = %v, want %v", vap.ResourceVersion, tt.resourceVersion) From 9066d2df16e6713d111deb3252cc77df772ea953 Mon Sep 17 00:00:00 2001 From: Nont Date: Tue, 21 Oct 2025 14:41:01 -0500 Subject: [PATCH 3/4] Adjust the allow list for managed resource vap Signed-off-by: Nont --- .../validatingadmissionpolicy.go | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy.go b/pkg/webhook/managedresource/validatingadmissionpolicy.go index d351e1fd9..cf0a273be 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy.go @@ -53,14 +53,24 @@ func mutateValidatingAdmissionPolicy(vap *admv1.ValidatingAdmissionPolicy) { }, Validations: []admv1.Validation{ { - Expression: `( - request.userInfo.username == "aksService" || - request.userInfo.username == "fleet-member-agent-sa") - && ( - "system:masters" in request.userInfo.groups || - "system:serviceaccounts:kube-system" in request.userInfo.groups || - "system:serviceaccounts:fleet-system" in request.userInfo.groups || - "system:serviceaccounts:openshift-kube-controller-manager" in request.userInfo.groups)`, + Expression: ` + ( + ( + request.userInfo.username == "aksService" || + request.userInfo.username == "fleet-member-agent-sa" + ) + && + ( + "system:masters" in request.userInfo.groups || + "system:serviceaccounts:kube-system" in request.userInfo.groups || + "system:serviceaccounts:fleet-system" in request.userInfo.groups || + "system:serviceaccounts:openshift-kube-controller-manager" in request.userInfo.groups + ) + ) + || + ( + "system:serviceaccounts:openshift-infra" in request.userInfo.groups + )`, Message: "Create, Update, or Delete operations on ARM managed resources is forbidden", Reason: &forbidden, }, From a828d882da318359263ce19538fbb609a9f836ca Mon Sep 17 00:00:00 2001 From: Nont Date: Tue, 21 Oct 2025 22:04:38 -0500 Subject: [PATCH 4/4] Remove system:serviceaccounts:openshift-kube-controller-manager Signed-off-by: Nont --- .../managedresource/createordelete_test.go | 2 -- .../validatingadmissionpolicy.go | 3 +-- test/e2e/managed_resource_vap_test.go | 21 +++++++++---------- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/pkg/webhook/managedresource/createordelete_test.go b/pkg/webhook/managedresource/createordelete_test.go index 548b3fd79..b46dc2482 100644 --- a/pkg/webhook/managedresource/createordelete_test.go +++ b/pkg/webhook/managedresource/createordelete_test.go @@ -373,11 +373,9 @@ func TestGetVAPBindingWithMutator(t *testing.T) { // Verify initial state if vapb == nil { t.Fatal("getVAPBindingWithMutator() returned nil VAP binding") - return } if mutateFunc == nil { t.Fatal("getVAPBindingWithMutator() returned nil mutate function") - return } // Verify mutate function works diff --git a/pkg/webhook/managedresource/validatingadmissionpolicy.go b/pkg/webhook/managedresource/validatingadmissionpolicy.go index cf0a273be..80ba75979 100644 --- a/pkg/webhook/managedresource/validatingadmissionpolicy.go +++ b/pkg/webhook/managedresource/validatingadmissionpolicy.go @@ -63,8 +63,7 @@ func mutateValidatingAdmissionPolicy(vap *admv1.ValidatingAdmissionPolicy) { ( "system:masters" in request.userInfo.groups || "system:serviceaccounts:kube-system" in request.userInfo.groups || - "system:serviceaccounts:fleet-system" in request.userInfo.groups || - "system:serviceaccounts:openshift-kube-controller-manager" in request.userInfo.groups + "system:serviceaccounts:fleet-system" in request.userInfo.groups ) ) || diff --git a/test/e2e/managed_resource_vap_test.go b/test/e2e/managed_resource_vap_test.go index f853ab769..fed4631e0 100644 --- a/test/e2e/managed_resource_vap_test.go +++ b/test/e2e/managed_resource_vap_test.go @@ -35,7 +35,6 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/version" "k8s.io/client-go/discovery" - "sigs.k8s.io/controller-runtime/pkg/client" placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" @@ -53,16 +52,6 @@ var managedByLabelMap = map[string]string{ managedByLabel: managedByLabelValue, } -// Need this because Entry() evaluates parameters at definition time, not at runtime. -// Without this, the client value sent to Entry() would always be nil. -func getUserClient() client.Client { - return hubCluster.ImpersonateKubeClient -} - -func getAksServiceClient() client.Client { - return hubCluster.SystemMastersClient -} - var _ = Describe("ValidatingAdmissionPolicy for Managed Resources", Label("managedresource"), Ordered, func() { BeforeEach(func() { discoveryClient := framework.GetDiscoveryClient(hubCluster) @@ -520,3 +509,13 @@ func isAPIServerVersionAtLeast(disco discovery.DiscoveryInterface, targetVersion server, target := version.MustParseSemantic(serverVersion.GitVersion), version.MustParseSemantic(targetVersion) return server.AtLeast(target), nil } + +// Need this because Entry() evaluates parameters at definition time, not at runtime. +// Without this, the client value sent to Entry() would always be nil. +func getUserClient() client.Client { + return hubCluster.ImpersonateKubeClient +} + +func getAksServiceClient() client.Client { + return hubCluster.SystemMastersClient +}