Skip to content

Conversation

@eduardodbr
Copy link
Member

@eduardodbr eduardodbr commented Dec 12, 2025

Fixes #15071

Motivation

If the service account (SA) running the controller is granted a role that provides access to ClusterWorkflowTemplates within the controller’s namespace, the controller incorrectly assumes the SA has access to ClusterWorkflowTemplates cluster-wide. While this scenario is uncommon, it is possible. This PR addresses the issue by verifying access at the cluster scope rather than relying on namespace-scoped permissions.

Modifications

Verification

Documentation

Summary by CodeRabbit

  • Bug Fixes
    • Simplified access control validation for cluster workflow templates to use global permission checks instead of namespace-scoped checks.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 12, 2025

📝 Walkthrough

Walkthrough

The changes remove the namespace parameter from the HasAccessToClusterWorkflowTemplates function across all call sites. This corrects an RBAC verification logic to properly handle ClusterWorkflowTemplate as a cluster-wide resource without namespace-scoped access checks.

Changes

Cohort / File(s) Summary
Function signature update
util/rbac/rbac.go
Removed namespace string parameter from HasAccessToClusterWorkflowTemplates(ctx, kubeclientset). Simplified internal auth checks to pass empty string for all namespace arguments in authutil.CanIArgo calls.
Call site updates
pkg/apiclient/argo-kube-client.go, server/apiserver/argoserver.go, workflow/controller/controller.go
Removed namespace argument from all HasAccessToClusterWorkflowTemplates invocations to match updated function signature.
Test updates
util/rbac/rbac_test.go
Updated all three test cases to call HasAccessToClusterWorkflowTemplates(ctx, kubeclientset) without the third parameter.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Areas requiring attention:
    • Verify all call sites of HasAccessToClusterWorkflowTemplates have been updated (search for any remaining namespace argument usages)
    • Confirm the function implementation correctly omits namespace in permission checks for cluster-wide resources
    • Validate test coverage reflects the simplified signature and cluster-wide permission model

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly describes the main change: removing namespace scope from ClusterWorkflowTemplate RBAC checks and fixing issue #15071.
Linked Issues check ✅ Passed The code changes successfully address all key requirements from issue #15071: removing namespace argument from HasAccessToClusterWorkflowTemplates calls across all call sites to enable cluster-wide RBAC checks instead of namespaced checks.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the RBAC check for ClusterWorkflowTemplate cluster-wide access; no unrelated modifications detected.
Description check ✅ Passed PR description includes issue reference and motivation but lacks explicit modifications, verification, and documentation sections.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@eduardodbr eduardodbr changed the title fix: check ClusterWorkflowTemplate RBAC cluster wide. Fixes #15071 fix: check ClusterWorkflowTemplate RBAC cluster wide instead of namespaces. Fixes #15071 Dec 12, 2025
@eduardodbr eduardodbr changed the title fix: check ClusterWorkflowTemplate RBAC cluster wide instead of namespaces. Fixes #15071 fix: check ClusterWorkflowTemplate RBAC cluster wide instead of namespaced. Fixes #15071 Dec 12, 2025
@eduardodbr
Copy link
Member Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Dec 13, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@eduardodbr
Copy link
Member Author

/retest

@eduardodbr eduardodbr marked this pull request as ready for review December 15, 2025 18:44
Copy link
Member

@Joibel Joibel left a comment

Choose a reason for hiding this comment

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

Looks good, thanks

@Joibel Joibel added the cherry-pick/3.7 Cherry-pick this to release-3.7 label Dec 16, 2025
@Joibel Joibel merged commit e621d1f into argoproj:main Dec 16, 2025
70 of 73 checks passed
@argo-cd-cherry-pick-bot
Copy link

❌ Cherry-pick failed for 3.7. Please check the workflow logs for details.

Joibel pushed a commit that referenced this pull request Dec 16, 2025
…paced. Fixes #15071 (#15162)

(cherry picked from commit e621d1f)

Signed-off-by: Eduardo Rodrigues <[email protected]>
Signed-off-by: Alan Clucas <[email protected]>
Joibel pushed a commit that referenced this pull request Dec 16, 2025
…paced. Fixes #15071 (#15162)

(cherry picked from commit e621d1f)

Signed-off-by: Eduardo Rodrigues <[email protected]>
Signed-off-by: Alan Clucas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-pick/3.7 Cherry-pick this to release-3.7

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Argo Server: Incomplete RBAC check for ClusterWorkflowTemplate informer creation

2 participants