Update RBAC permissions#200
Conversation
WalkthroughThis PR expands and reorganizes Kubernetes RBAC configurations for the ZXporter Operator, introducing comprehensive permissions across multiple API groups, new metrics authentication roles, reorganized leader-election scope, and corresponding documentation. Changes span configuration files, Helm charts, controller annotations, and new RBAC documentation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docs/rbac.md (2)
9-15: Consider adding blank lines around tables for markdownlint complianceThe three permission tables start immediately after headings. Adding a blank line before and after each table (Main Operator ClusterRole, Conditional Third-Party Permissions, Leader Election Role) will satisfy MD058 and improve readability.
Also applies to: 57-61, 79-83
274-288: Clarify “runtime” write permissions vs overall RBAC capabilitiesThe “two-phase permission model” section says the runtime phase “operates with minimal write permissions (only ConfigMap updates)”. That’s accurate for what the Go controller intends to do at runtime, but the bound ServiceAccount also retains bootstrap-capable rights (e.g., RBAC and metrics-server resources) from the ClusterRole. Consider rephrasing to make it explicit that:
- Bootstrap permissions remain granted at the RBAC level, but
- Only ConfigMap updates are exercised by the controller during normal runtime.
This keeps the security description precise without changing behavior.
helm-chart/zxporter/templates/zxporter-rbac.yaml (1)
1-7: RBAC manifest structure looks solid; consider tweaking the header commentThe split between:
devzero-zxporter-leader-election-role(leases/events/configmaps in the operator namespace),devzero-zxporter-manager-role(bootstrap + runtime + optional third‑party monitoring), and- the new metrics auth/reader ClusterRoles
is clear and aligns with the controller’s RBAC annotations and the new docs.
One small nit: the header comment (“This role contains permissions needed for… 2. Metrics server bootstrap installation”) reads as if the Role handles metrics-server bootstrap, but those permissions actually live in the
devzero-zxporter-manager-roleClusterRole below. Consider rewording the comment to refer to “this file” or explicitly to the manager ClusterRole to avoid confusion.Also applies to: 113-177, 425-501
config/rbac/role.yaml (1)
7-18: Manager ClusterRole RBAC updates align with controller annotations; optional tightening for ConfigMapsThe added/updated rules for:
configmaps(get,list,update,watch),serviceaccounts/services(create + read),apiregistration.k8s.io/apiservices(now includingwatch), andapps/deployments(create + read)bring
manager-rolein line with the controller’s kubebuilder RBAC annotations and the Helmdevzero-zxporter-manager-role, which is great for reducing drift between paths.If you ever want stricter least‑privilege for non‑Helm installs, you could consider scoping the
configmapsupdatepermission to just the operator’s config ConfigMap (e.g., by splitting into a namespaced Role and/or usingresourceNames). Not required for correctness, but it would better reflect the intent that only that ConfigMap is modified.Also applies to: 41-52, 62-71, 82-92
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
dist/backend-install.yamlis excluded by!**/dist/**dist/install.yamlis excluded by!**/dist/**dist/installer_updater.yamlis excluded by!**/dist/**dist/zxporter.yamlis excluded by!**/dist/**
📒 Files selected for processing (4)
config/rbac/role.yaml(3 hunks)docs/rbac.md(1 hunks)helm-chart/zxporter/templates/zxporter-rbac.yaml(9 hunks)internal/controller/collectionpolicy_controller.go(3 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/rbac.md
[grammar] ~276-~276: Ensure spelling is correct
Context: ... ### Principle of Least Privilege The zxporter operator follows a two-phase permission...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/rbac.md
10-10: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
58-58: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
79-79: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Docker Image
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
internal/controller/collectionpolicy_controller.go (1)
139-245: RBAC annotation reorganization looks consistent with manifests and usageThe new kubebuilder RBAC sections (CRD management, bootstrap, runtime, optional third‑party) line up with what the controller actually touches and with the expanded ClusterRole rules in
config/rbac/role.yamland the Helm RBAC template. The split between bootstrap (write-heavy) and runtime (mostly read-only, except ConfigMap updates) is clear and matches the docs.
* Update RBAC permissions * Bring back arch
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.