From 32ef6e7376c8c3cdbacea12a16ae34484f02670e Mon Sep 17 00:00:00 2001 From: Alka Kumari Date: Fri, 4 Sep 2026 12:20:56 +0530 Subject: [PATCH] fix: omit nonResourceURLs from namespaced Roles Signed-off-by: Alka Kumari --- controllers/argocd/openshift/openshift.go | 30 +++++++- .../argocd/openshift/openshift_test.go | 48 +++++++++++++ .../1-113_validate_controller_role_test.go | 68 +++++++++++++++++++ 3 files changed, 145 insertions(+), 1 deletion(-) diff --git a/controllers/argocd/openshift/openshift.go b/controllers/argocd/openshift/openshift.go index 0e5133af0a8..6286acf8a5a 100644 --- a/controllers/argocd/openshift/openshift.go +++ b/controllers/argocd/openshift/openshift.go @@ -106,7 +106,11 @@ func ReconcilerHook(cr *argoapp.ArgoCD, v any, hint string) error { return err } policyRules := getPolicyRuleForApplicationController() - policyRules = append(policyRules, clusterRole.Rules...) + namespacedAdminRules := policyRulesForNamespacedRole(clusterRole.Rules) + if omitted := len(clusterRole.Rules) - len(namespacedAdminRules); omitted > 0 { + logv.Info("omitted nonResourceURLs rules from namespaced Role; they are only valid on ClusterRoles", "omitted", omitted) + } + policyRules = append(policyRules, namespacedAdminRules...) o.Rules = policyRules } } @@ -224,6 +228,30 @@ func BuilderHook(_ *argoapp.ArgoCD, v any, _ string) error { return nil } +// policyRulesForNamespacedRole copies ClusterRole rules that are valid on a +// namespaced Role. nonResourceURLs is only permitted on ClusterRoles; +// Rules that are empty after stripping are dropped. +func policyRulesForNamespacedRole(rules []rbacv1.PolicyRule) []rbacv1.PolicyRule { + filtered := make([]rbacv1.PolicyRule, 0, len(rules)) + for _, rule := range rules { + if len(rule.NonResourceURLs) > 0 { + rule.NonResourceURLs = nil + } + if isEmptyPolicyRule(rule) { + continue + } + filtered = append(filtered, rule) + } + return filtered +} + +func isEmptyPolicyRule(rule rbacv1.PolicyRule) bool { + return len(rule.APIGroups) == 0 && + len(rule.Resources) == 0 && + len(rule.ResourceNames) == 0 && + len(rule.NonResourceURLs) == 0 +} + func getPolicyRuleForApplicationController() []rbacv1.PolicyRule { return []rbacv1.PolicyRule{ { diff --git a/controllers/argocd/openshift/openshift_test.go b/controllers/argocd/openshift/openshift_test.go index 353ad036d30..eb0f43ed471 100644 --- a/controllers/argocd/openshift/openshift_test.go +++ b/controllers/argocd/openshift/openshift_test.go @@ -94,6 +94,54 @@ func TestReconcileArgoCD_notInClusterConfigNamespaces(t *testing.T) { assert.Equal(t, want, testClusterRole.Rules) } +// Test that nonResourceURLs are stripped from rules in namespaced roles +func TestPolicyRulesForNamespacedRole(t *testing.T) { + resourceRule := rbacv1.PolicyRule{ + APIGroups: []string{"apps"}, + Resources: []string{"deployments"}, + Verbs: []string{"get", "list"}, + } + nonResourceRule := rbacv1.PolicyRule{ + NonResourceURLs: []string{"*"}, + Verbs: []string{"get"}, + } + metricsNonResourceRule := rbacv1.PolicyRule{ + NonResourceURLs: []string{"/metrics"}, + Verbs: []string{"get"}, + } + mixedRule := rbacv1.PolicyRule{ + APIGroups: []string{"test.com"}, + Resources: []string{"tests"}, + NonResourceURLs: []string{"/healthz"}, + Verbs: []string{"get"}, + } + + t.Run("drops rules that only grant nonResourceURLs", func(t *testing.T) { + got := policyRulesForNamespacedRole([]rbacv1.PolicyRule{resourceRule, nonResourceRule, metricsNonResourceRule}) + assert.Equal(t, []rbacv1.PolicyRule{resourceRule}, got) + }) + + t.Run("strips nonResourceURLs from mixed rules and keeps resource fields", func(t *testing.T) { + got := policyRulesForNamespacedRole([]rbacv1.PolicyRule{mixedRule}) + assert.Equal(t, []rbacv1.PolicyRule{{ + APIGroups: []string{"test.com"}, + Resources: []string{"tests"}, + Verbs: []string{"get"}, + }}, got) + }) + + t.Run("returns empty slice when every rule is nonResourceURLs only", func(t *testing.T) { + got := policyRulesForNamespacedRole([]rbacv1.PolicyRule{nonResourceRule}) + assert.Empty(t, got) + assert.NotNil(t, got) + }) + + t.Run("keeps resource rules unchanged", func(t *testing.T) { + got := policyRulesForNamespacedRole([]rbacv1.PolicyRule{resourceRule}) + assert.Equal(t, []rbacv1.PolicyRule{resourceRule}, got) + }) +} + func TestAllowedNamespaces(t *testing.T) { argocdNamespace := testNamespace diff --git a/test/openshift/e2e/ginkgo/sequential/1-113_validate_controller_role_test.go b/test/openshift/e2e/ginkgo/sequential/1-113_validate_controller_role_test.go index 369bc453367..d406e9c2d4e 100644 --- a/test/openshift/e2e/ginkgo/sequential/1-113_validate_controller_role_test.go +++ b/test/openshift/e2e/ginkgo/sequential/1-113_validate_controller_role_test.go @@ -56,6 +56,21 @@ func roleContainsPolicyRule(k8sClient client.Client, role *rbacv1.Role, expected return false } +func roleContainsNonResourceURLRules(k8sClient client.Client, role *rbacv1.Role) bool { + if err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(role), role); err != nil { + GinkgoWriter.Println(err) + return true + } + + for _, rule := range role.Rules { + if len(rule.NonResourceURLs) > 0 { + GinkgoWriter.Println("roleContainsNonResourceURLRules - found nonResourceURLs rule:", rule) + return true + } + } + return false +} + var _ = Describe("GitOps Operator Sequential E2E Tests", func() { Context("1-113_validate_controller_role", func() { @@ -131,5 +146,58 @@ var _ = Describe("GitOps Operator Sequential E2E Tests", func() { return roleContainsPolicyRule(k8sClient, appControllerRole, aggregatedControllerRoleRule) }, "30s", "5s").Should(BeFalse()) }) + + It("omits aggregated admin nonResourceURLs from the namespaced application-controller Role", Label("openshift"), func() { + By("creating a namespace managed by openshift-gitops") + testNS = fixture.CreateManagedNamespace("test-1-113-nonresource-ns", "openshift-gitops") + defer func() { + Expect(k8sClient.Delete(ctx, testNS)).To(Succeed()) + }() + + openshiftGitopsArgoCD, err := argocdFixture.GetOpenShiftGitOpsNSArgoCD() + Expect(err).ToNot(HaveOccurred()) + Eventually(openshiftGitopsArgoCD, "5m", "5s").Should(argocdFixture.BeAvailable()) + + appControllerRole := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "openshift-gitops-argocd-application-controller", + Namespace: testNS.Name, + }, + } + + By("verifying openshift-gitops application-controller Role is created in the managed namespace") + Eventually(appControllerRole).Should(k8sFixture.ExistByName()) + + nonResourceRule := rbacv1.PolicyRule{ + NonResourceURLs: []string{"*"}, + Verbs: []string{"get"}, + } + aggregateClusterRole := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-1-113-nonresource", + Labels: map[string]string{ + "rbac.authorization.k8s.io/aggregate-to-admin": "true", + }, + }, + Rules: []rbacv1.PolicyRule{aggregatedControllerRoleRule, nonResourceRule}, + } + + By("creating a ClusterRole that aggregates a nonResourceURLs rule into admin") + Expect(k8sClient.Create(ctx, aggregateClusterRole)).To(Succeed()) + defer func() { + Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, aggregateClusterRole))).To(Succeed()) + }() + + By("verifying resource rules are aggregated and nonResourceURLs are omitted from the namespaced Role") + Eventually(func() bool { + return roleContainsPolicyRule(k8sClient, appControllerRole, aggregatedControllerRoleRule) + }, "3m", "5s").Should(BeTrue()) + Consistently(func() bool { + return !roleContainsNonResourceURLRules(k8sClient, appControllerRole) + }, "30s", "5s").Should(BeTrue()) + + By("verifying Argo CD remains available after aggregating the nonResourceURLs ClusterRole") + Eventually(openshiftGitopsArgoCD, "2m", "5s").Should(argocdFixture.BeAvailable()) + }) }) })