Repository navigation
[rbac] Use hierarchical naming scheme - #2019
Conversation
dosubot
Bot
added
size/M
Summary of ChangesHello Timofei Larkin (@lllamnyp), I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request standardizes the naming scheme for Cozystack's RBAC resources, aligning them with Kubernetes' hierarchical naming conventions. By replacing hyphens with colons in resource names like Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughRenamed Kubernetes RBAC resource names from hyphenated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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)
No actionable comments were generated in the recent review. 🎉 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the naming of RBAC resources to use a hierarchical, colon-separated scheme, aligning with Kubernetes conventions. The changes are applied consistently across all relevant ClusterRole, Role, and RoleBinding definitions. All references between role bindings and their corresponding roles have been updated correctly. The aggregation logic for cluster roles remains intact and correct. This is a good improvement for clarity and maintainability of RBAC resources.
![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
🤖 Fix all issues with AI agents
In `@packages/apps/tenant/templates/tenant.yaml`:
- Around line 32-35: The RoleBinding roleRef values in tenant.yaml are being
changed (e.g., roleRef.name values 'cozy:tenant', 'cozy:tenant:view',
'cozy:tenant:use', 'cozy:tenant:admin', 'cozy:tenant:super-admin', and the Role
'cozy:tenant:dashboard') but roleRef is immutable on existing RoleBindings;
implement a migration so Helm upgrade doesn't fail: add a pre-upgrade hook (or a
separate migration Job) that deletes the existing RoleBindings with those
metadata names before Helm recreates them, or run `kubectl auth reconcile -f
<tenant.yaml>` as part of the upgrade; target the RoleBinding resources that
contain roleRef in tenant.yaml (and the Role resource 'cozy:tenant:dashboard')
so the new roleRef values can be applied without attempting an in-place patch.
🧹 Nitpick comments (1)
packages/system/cozystack-basics/templates/clusterroles.yaml (1)
52-60: Aggregation labels still use hyphens — intentional but worth a note.The ClusterRole names now use colons (
cozy:tenant:view) but the aggregation labels remain hyphenated (rbac.cozystack.io/aggregate-to-tenant-view). This is functionally fine since labels and names are independent, but the inconsistency may cause confusion for future maintainers. Consider adding a brief comment or documenting this convention choice.
| roleRef: | ||
| kind: ClusterRole | ||
| name: cozy-tenant | ||
| name: cozy:tenant | ||
| apiGroup: rbac.authorization.k8s.io |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full RoleBinding definitions in tenant.yaml
fd -t f tenant.yaml packages/apps/tenant/templates/ -x cat -n {}Repository: cozystack/cozystack
Length of output: 4172
🏁 Script executed:
# Check for any migration hooks or pre-upgrade strategies
find packages/apps/tenant -type f -name "*.yaml" -o -name "*.yml" | xargs grep -l "hook\|pre-upgrade\|migration" 2>/dev/null || echo "No migration hooks found"Repository: cozystack/cozystack
Length of output: 110
🌐 Web query:
Kubernetes RoleBinding roleRef immutability constraints and upgrade behavior
💡 Result:
Immutability constraint (what you can’t change)
- In both RoleBinding and ClusterRoleBinding, the field
.roleRefis immutable. If you try to update it (changekind,name, orapiGroup), the API server rejects the update with a validation error like “cannot change roleRef”. (kubernetes.io) - Kubernetes documents two reasons: (1) allow delegating “update binding subjects” without allowing changing the granted role; (2) switching to a different role is considered a fundamentally different binding, so it must be delete+recreate. (kubernetes.io)
Upgrade / reconciliation behavior (kubectl / Helm / controllers)
kubectl apply/ patch / SSA: can update mutable parts (e.g.,subjects, metadata), but will fail if the manifest changesroleRef. (kubernetes.io)- To “change roleRef” you must replace the object: delete the existing binding and create a new one. (kubernetes.io)
kubectl auth reconcile -f …is purpose-built for RBAC objects and will delete+recreate bindings when needed to account for an immutableroleRefchange; it can also semantically reconcile subjects (optionally removing extras with--remove-extra-subjects). (kubernetes.io)- Helm upgrades: if a chart revision changes
roleRef, the upgrade typically fails because Helm attempts to patch the existing object; you generally need recreate semantics (for example, deleting that binding first, or using a forced replace approach) because Kubernetes won’t allow an in-place change. (kubernetes.io)
Practical rule of thumb
- Role/ClusterRole changes (rules inside the Role/ClusterRole): safe to upgrade in-place.
- Binding subject changes (who gets the role): safe to upgrade in-place.
- Binding roleRef changes (what role is granted): must be delete+recreate (or use
kubectl auth reconcile, which handles this). (kubernetes.io)
Citations:
- 1: https://kubernetes.io/docs/reference/access-authn-authz/rbac/?utm_source=openai
- 2: https://kubernetes.io/docs/reference/access-authn-authz/rbac/?utm_source=openai
- 3: https://kubernetes.io/docs/reference/access-authn-authz/rbac/?utm_source=openai
- 4: https://kubernetes.io/docs/reference/access-authn-authz/rbac/?utm_source=openai
- 5: https://kubernetes.io/docs/reference/access-authn-authz/rbac/?utm_source=openai
- 6: https://kubernetes.io/docs/reference/access-authn-authz/rbac/?utm_source=openai
- 7: https://kubernetes.io/docs/reference/access-authn-authz/rbac/?utm_source=openai
🏁 Script executed:
cat -n packages/apps/tenant/templates/cleanup-job.yamlRepository: cozystack/cozystack
Length of output: 3268
Critical: roleRef is immutable on existing RoleBindings — Helm upgrade will fail.
Kubernetes does not allow modifying roleRef on an existing RoleBinding. Changing roleRef.name from cozy-tenant to cozy:tenant (and similar changes for all six RoleBindings) while keeping the same RoleBinding metadata.name will cause Helm to attempt a patch/update, which the API server will reject with an error like:
roleRef cannot be changed
This applies to all six RoleBindings in tenant.yaml:
- Line 34:
cozy:tenant - Line 47:
cozy:tenant:view - Line 60:
cozy:tenant:use - Line 73:
cozy:tenant:admin - Line 86:
cozy:tenant:super-admin - Line 113:
cozy:tenant:dashboard(Role, not ClusterRole)
You need a strategy to handle this migration, such as:
- A pre-upgrade hook that deletes the existing RoleBindings before Helm attempts to recreate them.
- Using
kubectl auth reconcile -f …which is purpose-built to handle immutableroleRefchanges. - A migration job that deletes the old bindings to allow recreation with new roleRef values.
🤖 Prompt for AI Agents
In `@packages/apps/tenant/templates/tenant.yaml` around lines 32 - 35, The
RoleBinding roleRef values in tenant.yaml are being changed (e.g., roleRef.name
values 'cozy:tenant', 'cozy:tenant:view', 'cozy:tenant:use',
'cozy:tenant:admin', 'cozy:tenant:super-admin', and the Role
'cozy:tenant:dashboard') but roleRef is immutable on existing RoleBindings;
implement a migration so Helm upgrade doesn't fail: add a pre-upgrade hook (or a
separate migration Job) that deletes the existing RoleBindings with those
metadata names before Helm recreates them, or run `kubectl auth reconcile -f
<tenant.yaml>` as part of the upgrade; target the RoleBinding resources that
contain roleRef in tenant.yaml (and the Role resource 'cozy:tenant:dashboard')
so the new roleRef values can be applied without attempting an in-place patch.
b607691 to
2e0d899
Compare
## What this PR does This patch improves the naming conventions used in Cozystack's RBAC resources. It follows the k8s system convention of using colons as separators in RBAC resource names (e.g. system:nodes:<nodename>) and renames some default tenant clusterroles to a scheme like cozy:tenant:admin. ### Release note ```release-note [rbac] Use a more structured naming convention for Cozystack's RBAC resources. ``` Signed-off-by: Timofei Larkin <[email protected]>
2e0d899 to
36b2a19
Compare
2 participants
What this PR does
This patch improves the naming conventions used in Cozystack's RBAC resources. It follows the k8s system convention of using colons as separators in RBAC resource names (e.g. system:nodes:) and renames some default tenant clusterroles to a scheme like cozy:tenant:admin.
Release note
Summary by CodeRabbit