📝 Support cluster-wide permissions grants in AddonTemplate - #176
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bhperry The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds an enhancement design and metadata for addon-template-clusterrolebinding, proposing a new Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
enhancements/sig-architecture/233-addon-template-clusterrolebinding/README.md (2)
77-78: Add examples or remove empty section.The examples section is currently empty. Consider either:
- Adding an example showing how to configure an AddonTemplate with the new
AllNamespacesbinding type, or- Removing this section if examples will be added later
Examples would help users understand how to use the new feature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@enhancements/sig-architecture/233-addon-template-clusterrolebinding/README.md` around lines 77 - 78, The README currently has an empty "#### examples" section; either populate it with a concrete example showing how to configure an AddonTemplate that uses the new AllNamespaces binding type (include the AddonTemplate YAML or a minimal snippet illustrating metadata, spec.binding.type: AllNamespaces, and any required fields), or remove the "#### examples" heading entirely if you don't want examples in this PR; update the section to reference the AddonTemplate and AllNamespaces symbols so readers can find the usage easily.
84-85: Consider defining graduation criteria before implementation.While "N/A" may be acceptable for a provisional enhancement, graduation criteria should be defined before this enhancement moves to implementable status. Consider adding criteria for when this feature should be considered ready for different maturity levels (dev preview, tech preview, GA).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@enhancements/sig-architecture/233-addon-template-clusterrolebinding/README.md` around lines 84 - 85, Replace the placeholder "N/A" under the "Graduation Criteria" section with concrete, testable criteria for each maturity level; update the "Graduation Criteria" section to list specific requirements for dev preview, tech preview, and GA (e.g., stability/performance targets, API/behavior contracts, documentation, upgrade/migration paths, and security/compliance checks) so reviewers can verify when the enhancement is ready to advance.enhancements/sig-architecture/233-addon-template-clusterrolebinding/metadata.yaml (1)
14-14: Consider adding references to related enhancements.Since this enhancement extends the AddonTemplate feature, consider adding references to related enhancements in the
see-alsofield for better documentation and traceability.🔗 Proposed enhancement to add related references
-see-also: [] +see-also: + - "/enhancements/sig-architecture/82-addon-template"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@enhancements/sig-architecture/233-addon-template-clusterrolebinding/metadata.yaml` at line 14, Update the metadata.yaml "see-also" field to list related enhancements that this RFC extends or depends on (e.g., the AddonTemplate enhancement and any other relevant SIG-Architecture proposals); edit the see-also array to include those enhancement IDs/titles or links so reviewers can trace related work and context. Ensure you modify the see-also field in metadata.yaml (the "see-also" key) to contain a YAML array of references (IDs/titles/URLs) and keep entries concise and consistent with other enhancement metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@enhancements/sig-architecture/233-addon-template-clusterrolebinding/metadata.yaml`:
- Around line 11-12: Update the metadata fields in metadata.yaml so the
creation-date and last-updated reflect the actual creation time of the
enhancement (change the current 2025-05-22 values to 2026-03-25 or the correct
creation date); locate and update the creation-date and last-updated keys in the
file to the accurate ISO date string, commit the change, and ensure the
metadata.yaml entry for those two fields matches the PR creation date.
In
`@enhancements/sig-architecture/233-addon-template-clusterrolebinding/README.md`:
- Around line 60-62: Fix the typo in the comment above the AllNamespaces field:
change "wwhen" to "when" in the comment that describes
AllNamespaces/AllNamespacesBindingConfig so the comment reads "It is required
when the type is AllNamespaces"; update the comment next to the AllNamespaces
*AllNamespacesBindingConfig `json:"allNamespaces,omitempty"` declaration.
- Around line 5-9: Update the Markdown checklist items that currently use "- []"
to the correct GitHub-flavored Markdown checkbox syntax "- [ ]" so each
unchecked item renders properly; specifically edit the lines starting with "- []
Enhancement is `provisional`", "- [] Design details are appropriately documented
from clear requirements", "- [] Test plan is defined", "- [] Graduation criteria
for dev preview, tech preview, GA", and "- [] User-facing documentation is
created in
[website](https://github.com/open-cluster-management-io/open-cluster-management-io.github.io/)"
to use "- [ ]" for each checkbox.
- Around line 65-71: The comment incorrectly describes the struct name instead
of the field: update the doc comment above the ClusterRoleName field in the
AllNamespacesBindingConfig struct so it documents the field (e.g.,
"ClusterRoleName is the name of the ClusterRole that the addon agent will be
bound to; a ClusterRoleBinding will be created referencing this ClusterRole with
subjects for each cluster namespace. The user must ensure the ClusterRole exists
on the hub cluster."). Keep the +kubebuilder:validation:Required tag and the
json tag `clusterRoleName` unchanged and ensure the comment is placed
immediately above the ClusterRoleName field.
---
Nitpick comments:
In
`@enhancements/sig-architecture/233-addon-template-clusterrolebinding/metadata.yaml`:
- Line 14: Update the metadata.yaml "see-also" field to list related
enhancements that this RFC extends or depends on (e.g., the AddonTemplate
enhancement and any other relevant SIG-Architecture proposals); edit the
see-also array to include those enhancement IDs/titles or links so reviewers can
trace related work and context. Ensure you modify the see-also field in
metadata.yaml (the "see-also" key) to contain a YAML array of references
(IDs/titles/URLs) and keep entries concise and consistent with other enhancement
metadata.
In
`@enhancements/sig-architecture/233-addon-template-clusterrolebinding/README.md`:
- Around line 77-78: The README currently has an empty "#### examples" section;
either populate it with a concrete example showing how to configure an
AddonTemplate that uses the new AllNamespaces binding type (include the
AddonTemplate YAML or a minimal snippet illustrating metadata,
spec.binding.type: AllNamespaces, and any required fields), or remove the "####
examples" heading entirely if you don't want examples in this PR; update the
section to reference the AddonTemplate and AllNamespaces symbols so readers can
find the usage easily.
- Around line 84-85: Replace the placeholder "N/A" under the "Graduation
Criteria" section with concrete, testable criteria for each maturity level;
update the "Graduation Criteria" section to list specific requirements for dev
preview, tech preview, and GA (e.g., stability/performance targets, API/behavior
contracts, documentation, upgrade/migration paths, and security/compliance
checks) so reviewers can verify when the enhancement is ready to advance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b8c51b6-a825-467a-aacf-a29febd58ad6
📒 Files selected for processing (2)
enhancements/sig-architecture/233-addon-template-clusterrolebinding/README.mdenhancements/sig-architecture/233-addon-template-clusterrolebinding/metadata.yaml
ad7a077 to
c3fedab
Compare
Signed-off-by: Ben Perry <bhperry94@gmail.com>
c3fedab to
aa0e789
Compare
|
cc @zhujian7 |
|
@bhperry thanks for the proposal. I am thinking of several things:
|
| ## Proposal | ||
| - Add "AllNamespaces" hub permission type, which references a single ClusterRole in the hub | ||
| - Addon Manager will manage ClusterRoleBindings for spoke agent addons with AllNamespaces permissions | ||
| - Each ManagedClusterAddon has its own ClusterRoleBinding named `open-cluster-management:<addon-name>:clusterrole:<cluster-name>:agent` |
There was a problem hiding this comment.
Can multiple clusterroles be configured? How to prevent the name collision?
We may need to validate that AllNamespaces typed registration can only have 1?
There was a problem hiding this comment.
Actually I recently noticed there is the same problem in general with hubPermissions. There is a single rolebinding name used. If you try to set multiple entries it causes an error.
For one, privileges can only be granted up to the level granted to the addon-manager itself, it needs to have any permissions that it is trying to grant. Attempting to grant cluster-admin would fail unless it is also already bound to the addon-manager since it has permissions that are outside the scope of what is normally granted. Definitely agree there should be clear warnings in the documentation about using AllNamespaces. I was also considering if it should be an opt-in feature flag, or have a list of allowed clusterRoles specified in the clustermanager config. Being allowed to create AddonTemplates at all already assumes a great deal of priviledge though, since with existing functionality you could grant permission to get secrets from any arbitrary namespace.
That would need to be added. SingleNamespace has the same problem since it can't be garbage collected via ownerReference to ManagedClusterAddon like CurrentCluster does. There is a TODO comment about this here. Both could be addressed together if this is accepted. |
3 participants
Summary by CodeRabbit