Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion controllers/argocd/openshift/openshift.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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{
{
Expand Down
48 changes: 48 additions & 0 deletions controllers/argocd/openshift/openshift_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

@anandrkskd anandrkskd Sep 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we returning true in case of failure, this will make the condition pass even when the k8sClient fails to get the obj.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are expecting false from this function for the happy path as the name suggests roleContainsNonResourceURLRules. It will return true either if there is an error or nonResourceUrl is present(which is not expected)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. make sense. And we are logging the error as well so it will be clear in the logs that the failure happened due to api call hicups and not due to NonResourceURLs found.

}

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() {
Expand Down Expand Up @@ -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())
})
})
})
Loading