ACM-37273 - adding clusterrole for hcp proxy - #397
Conversation
Signed-off-by: kurwang <kurwang@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kurwang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe chart adds view and admin ACM ClusterRoles for HyperShift hosted control plane resources. It also adds a policy that labels existing HyperShift ClusterRoles and includes that policy in the virtual RBAC placement binding. ChangesHyperShift HCP RBAC
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/hold |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
charts/fine-grained-rbac/templates/policy-hcp-clusterroles-policy.yaml (1)
4-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an operational description and template comments.
Line 7 sets the policy description to an empty value. Lines 24-47 do not explain why the policy exposes these ClusterRoles for discovery. Add a meaningful description and comments for the label-enforcement rules.
As per coding guidelines, “Add meaningful labels and annotations to Kubernetes resources” and “Add comments explaining permission purposes in RBAC configurations.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/fine-grained-rbac/templates/policy-hcp-clusterroles-policy.yaml` around lines 4 - 47, Update the policy annotation description from empty to a meaningful operational explanation of the HCP ClusterRole discovery policy, and add template comments near the hypershift-admin and hypershift-reader label-enforcement rules explaining their discovery purpose. Keep the existing lookup and enforcement behavior unchanged.Source: Coding guidelines
charts/fine-grained-rbac/templates/acm-roles-addontemplate.yaml (1)
575-685: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new ClusterRoles and permission rules.
Lines 575-685 add labels but no meaningful annotations. The rules also have no comments that explain their permission purposes. Add role-purpose annotations and concise comments for each RBAC rule.
As per coding guidelines, “Add meaningful labels and annotations to Kubernetes resources” and “Add comments explaining permission purposes in RBAC configurations.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/fine-grained-rbac/templates/acm-roles-addontemplate.yaml` around lines 575 - 685, Add meaningful purpose annotations to the ClusterRoles acm-hcp-hosting:view and acm-hcp-hosting:admin, and add concise comments before each rules entry explaining the permissions granted for its API group and resources. Keep the existing labels, resources, verbs, and role behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/fine-grained-rbac/templates/acm-roles-addontemplate.yaml`:
- Around line 646-659: Update the acm-hcp-hosting:admin RBAC definition so
Secret, ConfigMap, and Service permissions are granted through namespace-scoped
RoleBinding objects in the intended HCP namespaces. Split these
namespace-resource rules out of the ClusterRole and configure the related
MulticlusterRoleAssignment with explicit targetNamespaces, preventing creation
of a cluster-wide ClusterRoleBinding.
---
Nitpick comments:
In `@charts/fine-grained-rbac/templates/acm-roles-addontemplate.yaml`:
- Around line 575-685: Add meaningful purpose annotations to the ClusterRoles
acm-hcp-hosting:view and acm-hcp-hosting:admin, and add concise comments before
each rules entry explaining the permissions granted for its API group and
resources. Keep the existing labels, resources, verbs, and role behavior
unchanged.
In `@charts/fine-grained-rbac/templates/policy-hcp-clusterroles-policy.yaml`:
- Around line 4-47: Update the policy annotation description from empty to a
meaningful operational explanation of the HCP ClusterRole discovery policy, and
add template comments near the hypershift-admin and hypershift-reader
label-enforcement rules explaining their discovery purpose. Keep the existing
lookup and enforcement behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d129cc68-3c26-4bb2-9773-0d57ac40f2b7
📒 Files selected for processing (3)
charts/fine-grained-rbac/templates/acm-roles-addontemplate.yamlcharts/fine-grained-rbac/templates/binding-policy-virt-rbac-placementbinding.yamlcharts/fine-grained-rbac/templates/policy-hcp-clusterroles-policy.yaml
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - secrets | ||
| - configmaps | ||
| - services | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - update | ||
| - patch | ||
| - delete |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the assignment implementation and manifests for binding scope.
rg -n -C 8 \
-g '*.go' -g '*.{yaml,yml}' \
'MulticlusterRoleAssignment|acm-hcp-hosting:admin|kind:\s*(ClusterRoleBinding|RoleBinding)|namespace' .Repository: stolostron/multicluster-role-assignment
Length of output: 50396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Candidate files:"
fd -a 'acm-roles-addontemplate.yaml|multiclusterroleassignment_controller.go|.*_types.*\.go|clusterpermission.*' . | sed 's#^\./##' | head -200
echo
echo "Relevant chart section (acm-roles-addontemplate.yaml):"
sed -n '620,670p' charts/fine-grained-rbac/templates/acm-roles-addontemplate.yaml | cat -n
echo
echo "Definitions/usages for acm-hcp-hosting:admin:"
rg -n -C 5 'acm-hcp-hosting:admin|secrets|configmaps|services' charts/fine-grained-rbac templates internal manifests 2>/dev/null | head -300
echo
echo "Binding-related controller code locations:"
rg -n -C 4 'generateBindingName|createCPFor|apply.*ClusterPermission|ClusterRoleBinding|RoleBinding|TargetNamespace' internal/controller -g '*.go' | head -500Repository: stolostron/multicluster-role-assignment
Length of output: 12518
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Controller outline around binding generation/configuration:"
ast-grep outline internal/controller/multiclusterroleassignment_controller.go --view compact | sed -n '1,220p'
echo
echo "Binding generation and applied binding methods:"
rg -n -C 6 'func .*generateBindingName|func .*apply.*Binding|func .*createCP|func .*ensureCluster|func .*apply.*Cluster|ClusterRoleBinding|RoleBinding|TargetNamespaces|RoleBindingSpec|kind:' internal/controller/multiclusterroleassignment_controller.go | sed -n '1,260p'
echo
echo "API type for TargetNamespaces and RoleAssignments:"
sed -n '1,220p' api/v1beta1/multiclusterroleassignment_types.go | cat -n
echo
echo "Template context for target namespaces / placement binding in chart:"
rg -n -C 6 'RoleAssignments|TargetNames|ClusterSelection|Placements|multiclusterRole|{{' charts/fine-grained-rbac/templates/acm-roles-addontemplate.yaml | sed -n '520,760p'Repository: stolostron/multicluster-role-assignment
Length of output: 388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Controller outline around binding generation/configuration:"
ast-grep outline internal/controller/multiclusterroleassignment_controller.go --view signatures | sed -n '1,220p'
echo
echo "Binding generation and applied binding methods:"
rg -n -C 6 'func .*generateBindingName|func .*apply.*Binding|func .*createCP|func .*ensureCluster|func .*apply.*Cluster|ClusterRoleBinding|RoleBoundary|RoleBinding|TargetNamespaces|RoleBindingSpec|kind:' internal/controller/multiclusterroleassignment_controller.go | sed -n '1,340p'
echo
echo "API type for TargetNamespaces and RoleAssignments:"
sed -n '1,220p' api/v1beta1/multiclusterroleassignment_types.go | cat -n
echo
echo "Template context for target namespaces / placement binding in chart:"
rg -n -C 6 'RoleAssignments|TargetNames|ClusterSelection|Placements|multiclusterRole|{{' charts/fine-grained-rbac/templates/acm-roles-addontemplate.yaml | sed -n '520,760p'Repository: stolostron/multicluster-role-assignment
Length of output: 26277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Template context for target namespaces / placement binding in chart:"
rg -n -C 6 'RoleAssignments|TargetNamesages|ClusterSelection|Placements|multiclusterRole|\\{\\{' charts/fine-grained-rbac/templates/acm-roles-addontemplate.yaml | sed -n '500,770p'
echo
echo "ClusterPermission type files:"
fd -a 'clusterpermission.*|.*_types.*\.go' internal api test charts | sed 's#^\./##' | sort
echo
echo "ClusterPermission type references:"
rg -n -C 4 'type ClusterPermission|ClusterPermissionSpec|ClusterRoleBindings|RoleBindings|roleBindings|clusterRoleBindings' api test internal -g '*.go' -g '*.yaml' | sed -n '1,260p'Repository: stolostron/multicluster-role-assignment
Length of output: 503
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Template context for target namespaces / placement binding in chart:"
rg -n -C 6 'RoleAssignments|TargetNamesages|ClusterSelection|Placements|multiclusterRole' charts/fine-grained-rbac/templates/acm-roles-addontemplate.yaml | sed -n '500,770p'
echo "--- chart file around 50 lines ---"
cat -n charts/fine-grained-rbac/templates/acm-roles-addontemplate.yaml | sed -n '480,535p'
echo
echo "ClusterPermission type references:"
rg -n -C 5 'type ClusterPermission|type .*RoleBinding|ClusterRoleBindings|RoleBindings|roleBindings|clusterRoleBindings' api test internal -g '*.go' -g '*.yaml' | sed -n '1,260p'Repository: stolostron/multicluster-role-assignment
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Chart file length and relevant region:"
wc -l charts/fine-grained-rbac/templates/acm-roles-addontemplate.yaml
cat -n charts/fine-grained-rbac/templates/acm-roles-addontemplate.yaml | sed -n '480,680p'
echo
echo "ClusterPermission references:"
rg -n -C 5 'type ClusterPermission|type .*RoleBinding|ClusterRoleBindings|RoleBindings|roleBindings|clusterRoleBindings' api test internal charts -g '*.go' -g '*.yaml' | sed -n '1,260p'Repository: stolostron/multicluster-role-assignment
Length of output: 33184
Require namespace-scoped bindings for the HCP admin role.
MulticlusterRoleAssignment creates a ClusterRoleBinding whenever targetNamespaces is omitted, so this acm-hcp-hosting:admin ClusterRole with namespace resources allows the assigned user to read/create/unlimited Secret, ConfigMap, and Service objects cluster-wide. Use HCP namespace RoleBinding objects, split this ClusterRole into namespace-scoped rules, and enforce the intended namespace boundary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/fine-grained-rbac/templates/acm-roles-addontemplate.yaml` around lines
646 - 659, Update the acm-hcp-hosting:admin RBAC definition so Secret,
ConfigMap, and Service permissions are granted through namespace-scoped
RoleBinding objects in the intended HCP namespaces. Split these
namespace-resource rules out of the ClusterRole and configure the related
MulticlusterRoleAssignment with explicit targetNamespaces, preventing creation
of a cluster-wide ClusterRoleBinding.
Source: Coding guidelines
|
1 participant
📝 Summary
Ticket Summary (Title):
As a hub admin, I can grant a user access to specific hosting clusters for hcp from-hub using MulticlusterRoleAssignment
Ticket Link:
https://issues.redhat.com/browse/ACM-37273
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
Adds HCP hosting cluster RBAC support to the Helm chart:
acm-hcp-hosting:view— read-only access to HyperShift resources (hostedclusters,nodepools, status subresources)acm-hcp-hosting:admin— full CRUD on HyperShift resources including secrets/configmaps forhcp from-hub createpolicy-hcp-clusterrolesensures pre-existing HyperShift ClusterRoles (hypershift-admin,hypershift-reader) get discovery labels, following the same pattern aspolicy-virt-clusterrolesOnce these ClusterRoles are deployed to hosting clusters, hub admins can create a
MulticlusterRoleAssignmentreferencingacm-hcp-hosting:adminto grant users access to specific hosting clusters forhcp from-huboperations.Summary by CodeRabbit