Conversation
Signed-off-by: Andrey Kolkov <[email protected]>
androndo
requested review from
IvanHunters,
kvaps,
lexfrei,
lllamnyp and
sircthulhu
as code owners
dosubot
bot
added
size:M
📝 WalkthroughWalkthroughTwo RBAC files are modified to adjust permissions for backup-related controllers. The backup-controller narrows its scope by removing cross-addon permissions and consolidating to backups.cozystack.io resources. The backupstrategy-controller gains expanded permissions across backup strategies, classes, jobs, applications, Velero resources, and leader election. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
Summary of ChangesHello, 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 refines the Role-Based Access Control (RBAC) configurations for the 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-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
|
![gemini-code-assist[bot]](proxy.php?url=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 RBAC permissions for the backup controllers, which is a great improvement for security and separation of concerns. I've identified a couple of opportunities to further tighten the permissions to more strictly follow the principle of least privilege. My comments below provide specific suggestions.
| - apiGroups: ["backups.cozystack.io"] | ||
| resources: ["backups"] | ||
| resources: ["backupjobs"] | ||
| verbs: ["create", "get", "list", "watch"] |
There was a problem hiding this comment.
The plan-controller only creates BackupJob resources. The get, list, and watch permissions are not necessary according to the controller's implementation in internal/backupcontroller/plan_controller.go. To adhere to the principle of least privilege, it's best to only grant the create verb.
verbs: ["create"]| # Velero Backup/Restore in cozy-velero namespace | ||
| - apiGroups: ["velero.io"] | ||
| resources: ["backups", "restores"] | ||
| verbs: ["create", "get", "list", "watch", "update", "patch"] |
There was a problem hiding this comment.
The backupstrategy-controller creates and lists Velero Backup and Restore resources, but it does not appear to watch, update, or patch them based on the logic in internal/backupcontroller/velerostrategy_controller.go. The controller polls for status by re-listing after a requeue, so watch permission is not used. To follow the principle of least privilege, the unused watch, update, and patch verbs should be removed.
verbs: ["create", "get", "list"]![coderabbitai[bot]](proxy.php?url=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 (1)
packages/system/backupstrategy-controller/templates/rbac.yaml (1)
34-36: Scope lease permissions to the fixed leader-election lease.The controller uses the hardcoded LeaderElectionID
strategy.backups.cozystack.io(seecmd/backupstrategy-controller/main.go:138). AddresourceNamesto restrict access to that specific lease instead of all leases.Suggested RBAC tightening
- apiGroups: ["coordination.k8s.io"] resources: ["leases"] + resourceNames: ["strategy.backups.cozystack.io"] verbs: ["get", "list", "watch", "create", "update", "patch"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/backupstrategy-controller/templates/rbac.yaml` around lines 34 - 36, The RBAC rule currently grants verbs on all coordination.k8s.io leases; restrict it to the controller's hardcoded leader lease by adding resourceNames: ["strategy.backups.cozystack.io"] under the leases rule in templates/rbac.yaml so the permissions only apply to the LeaderElectionID used by the controller (LeaderElectionID "strategy.backups.cozystack.io").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/system/backupstrategy-controller/templates/rbac.yaml`:
- Around line 7-9: Update the RBAC templates to replace wildcard resources with
explicit lists: in the Role where apiGroups is "strategy.backups.cozystack.io"
replace resources: ["*"] with resources: ["veleroes", "jobs"] to limit read
access, and in the Role for apiGroups "apps.cozystack.io" remove the "*" and
enumerate only the application kinds the controller truly needs (for example
VMInstance, VirtualMachine, etc.) so verbs: ["get","list","watch"] apply to a
concrete resources array rather than a wildcard; adjust any Role/RoleBinding
entries that reference the same wildcard to use the new explicit resource names.
---
Nitpick comments:
In `@packages/system/backupstrategy-controller/templates/rbac.yaml`:
- Around line 34-36: The RBAC rule currently grants verbs on all
coordination.k8s.io leases; restrict it to the controller's hardcoded leader
lease by adding resourceNames: ["strategy.backups.cozystack.io"] under the
leases rule in templates/rbac.yaml so the permissions only apply to the
LeaderElectionID used by the controller (LeaderElectionID
"strategy.backups.cozystack.io").
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/system/backup-controller/templates/rbac.yamlpackages/system/backupstrategy-controller/templates/rbac.yaml
| - apiGroups: ["strategy.backups.cozystack.io"] | ||
| resources: ["*"] | ||
| verbs: ["get", "list", "watch"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and show the RBAC rules
cat -n packages/system/backupstrategy-controller/templates/rbac.yaml | head -40Repository: cozystack/cozystack
Length of output: 1676
🏁 Script executed:
# Search for CRD definitions in the codebase for the mentioned API groups
rg -n "group:\s*strategy\.backups\.cozystack\.io|group:\s*apps\.cozystack\.io" -A 10 --type yamlRepository: cozystack/cozystack
Length of output: 16714
🏁 Script executed:
# Find plural resource names in CRD definitions
rg -n "plural:" --type yaml -B 3 -A 1 | grep -E "(strategy\.backups|apps\.cozystack|plural)"Repository: cozystack/cozystack
Length of output: 42260
🏁 Script executed:
# Check for backupstrategy-controller configuration or deployment to understand its scope
find packages/system/backupstrategy-controller -name "*.yaml" -o -name "*.go" | head -20Repository: cozystack/cozystack
Length of output: 680
Replace wildcard RBAC resources with explicit resource lists.
Line 8 grants read access to all resources in the strategy.backups.cozystack.io API group, but only two custom resources are defined: veleroes and jobs. This can be safely narrowed to resources: ["veleroes", "jobs"].
Line 27 grants read access to all resources in apps.cozystack.io for application reference handling. While this is broader than line 8, consider whether enumerating specific application resource kinds (e.g., VMInstance, VirtualMachine, etc.) would satisfy the controller's actual requirements without the wildcard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/backupstrategy-controller/templates/rbac.yaml` around lines 7
- 9, Update the RBAC templates to replace wildcard resources with explicit
lists: in the Role where apiGroups is "strategy.backups.cozystack.io" replace
resources: ["*"] with resources: ["veleroes", "jobs"] to limit read access, and
in the Role for apiGroups "apps.cozystack.io" remove the "*" and enumerate only
the application kinds the controller truly needs (for example VMInstance,
VirtualMachine, etc.) so verbs: ["get","list","watch"] apply to a concrete
resources array rather than a wildcard; adjust any Role/RoleBinding entries that
reference the same wildcard to use the new explicit resource names.
2 participants
What this PR does
Release note
Summary by CodeRabbit