controller add roles #1342
controller add roles #1342
Conversation
Signed-off-by: kklinch0 <kklinch0@gmail.com>
WalkthroughAdds read permissions on apps/deployments to the cozystack-controller ClusterRole and introduces a namespace-scoped Role and RoleBinding in cozy-system granting patch/update on the cozystack-api Deployment to the cozystack-controller ServiceAccount. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller SA as cozystack-controller SA
participant K8s API as Kubernetes API (RBAC)
participant Deploy as apps/Deployments
Controller SA->>K8s API: get/list/watch deployments (ClusterRole)
K8s API-->>Controller SA: Allowed
Controller SA->>Deploy: Watch deployments
sequenceDiagram
participant Controller SA as cozystack-controller SA (cozy-system)
participant K8s API as Kubernetes API (RBAC)
participant Deploy as Deployment/cozystack-api (cozy-system)
Controller SA->>K8s API: patch/update cozystack-api (RoleBinding)
K8s API-->>Controller SA: Allowed (namespace-scoped)
Controller SA->>Deploy: Patch/Update spec/status
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @klinch0, 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 enhances the cozystack-controller's capabilities by granting it additional Kubernetes Role-Based Access Control (RBAC) permissions. The changes allow the controller to view existing deployments and specifically enable it to modify the cozystack-api deployment. This is crucial for the controller to effectively manage or interact with deployment resources within the cluster, ensuring it has the necessary privileges to perform its intended operations.
Highlights
rbac.yamlupdate: Thecozystack-controller's ClusterRole is updated to includeget,list, andwatchverbs fordeploymentsresources in theappsAPI group.- New
role.yamlfor deployment management: A newRoleandRoleBindingare introduced, granting thecozystack-controllerServiceAccountpatchandupdatepermissions specifically on thecozystack-apideployment.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds RBAC permissions for the cozystack-controller. It introduces a ClusterRole update granting cluster-wide read access to Deployments, and a new Role and RoleBinding for namespace-scoped write access to the cozystack-api Deployment. While the specific write permissions are well-scoped, the cluster-wide read permissions are quite broad. My feedback focuses on ensuring the principle of least privilege is followed by questioning the necessity of these broad permissions.
| - apiGroups: ["apps"] | ||
| resources: ["deployments"] | ||
| verbs: ["get", "list", "watch"] |
There was a problem hiding this comment.
Granting cluster-wide get, list, and watch permissions on all deployments is a significant privilege. If the controller's functionality does not require visibility into deployments across all namespaces, it would be more secure to use a Role and RoleBinding scoped to the specific namespace(s) it operates in. This would better adhere to the principle of least privilege. Could you clarify if cluster-wide access is strictly necessary?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/system/cozystack-controller/templates/rbac.yaml (1)
18-20: Consider whether namespace-scoped read or additional subresources are needed
- If the controller only needs to observe Deployments in cozy-system (or a bounded set of namespaces), consider using a namespaced Role + RoleBinding for read to avoid cluster-wide visibility. If it genuinely needs to watch cluster-wide, current approach is fine.
- If the controller will adjust replica counts via the Scale subresource, add read on deployments/scale here or in the namespaced Role.
Apply if you decide to include the Scale subresource read at cluster scope:
- apiGroups: ["apps"] - resources: ["deployments"] + resources: ["deployments", "deployments/scale"] verbs: ["get", "list", "watch"]packages/system/cozystack-controller/templates/role.yaml (2)
1-11: Tightly scoped write permissions on a single Deployment are good; consider adding get and scale if neededNice use of resourceNames to keep patch/update confined to cozystack-api only. Two optional tweaks:
- If your reconcile flow does a read before patch/update, add "get" to avoid coupling to ClusterRole read permissions.
- If you use the Scale subresource to change replicas, include deployments/scale with patch/update (and optionally get).
Proposed diff:
rules: - apiGroups: ["apps"] - resources: ["deployments"] + resources: ["deployments", "deployments/scale"] resourceNames: ["cozystack-api"] - verbs: ["patch", "update"] + verbs: ["get", "patch", "update"]
1-11: Optional: Helm-parameterize hardcoded names for portabilityIf this chart might be installed under different namespaces or if the target Deployment name could vary across environments, consider templating:
- metadata.namespace -> {{ .Release.Namespace }}
- resourceNames -> make configurable via a value (e.g., .Values.apiDeploymentName) with a sane default.
Example (not a full patch, just indicative):
- metadata.namespace: {{ .Release.Namespace }}
- resourceNames: [{{ tpl .Values.apiDeploymentName . | quote }}]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/system/cozystack-controller/templates/rbac.yaml(1 hunks)packages/system/cozystack-controller/templates/role.yaml(1 hunks)
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (1)
packages/system/cozystack-controller/templates/rbac.yaml (1)
18-20: Read-only access to apps/deployments looks goodGranting get/list/watch on Deployments to the controller is a sensible, least-privilege addition and aligns with the Role/RoleBinding introduced for targeted write access.
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: RoleBinding | ||
| metadata: | ||
| name: cozystack-controller-deployment-patch-update | ||
| namespace: cozy-system | ||
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: Role | ||
| name: cozystack-controller-deployment-patch-update | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: cozystack-controller | ||
| namespace: cozy-system |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
RoleBinding wiring looks correct; verify ServiceAccount presence
Binding the Role to cozystack-controller in cozy-system is correct. Please confirm that the ServiceAccount is created by this chart (or elsewhere) and is in the cozy-system namespace to avoid install-time failures.
Run this script to verify the ServiceAccount exists and is namespaced correctly within the repo:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for ServiceAccount 'cozystack-controller' definitions..."
rg -n -C3 -g '!**/node_modules/**' -P '(?s)^\s*kind:\s*ServiceAccount\b.*?^\s*metadata:\s*\n(?:.*\n)*?\s*name:\s*cozystack-controller\b' || {
echo "No ServiceAccount named 'cozystack-controller' found."
exit 1
}
echo
echo "Checking that it's in namespace cozy-system (or templated appropriately)..."
rg -n -C3 -g '!**/node_modules/**' -P '(?s)^\s*kind:\s*ServiceAccount\b.*?^\s*metadata:\s*\n(?:.*\n)*?\s*name:\s*cozystack-controller\b(?:.*\n)*?\s*namespace:\s*cozy-system\b' || {
echo "Found the SA, but did not detect explicit namespace cozy-system near it. If templated, ensure it resolves to cozy-system at install time."
exit 0
}
echo "ServiceAccount appears to exist and be namespaced to cozy-system."Length of output: 439
Action required: ServiceAccount 'cozystack-controller' is missing — add it or align the RoleBinding
Repo search returned: "No ServiceAccount named 'cozystack-controller' found." The RoleBinding in packages/system/cozystack-controller/templates/role.yaml (lines 13–25) binds to that SA in namespace cozy-system and will fail at install time.
-
Files to check/fix:
- packages/system/cozystack-controller/templates/role.yaml (lines 13–25) — RoleBinding subjects reference ServiceAccount: cozystack-controller, namespace: cozy-system.
- Ensure a ServiceAccount exists (add to the chart or confirm it's provided elsewhere). Suggested location: packages/system/cozystack-controller/templates/serviceaccount.yaml
-
Suggested fixes (pick one):
- Add a ServiceAccount manifest in the chart, e.g.:
apiVersion: v1
kind: ServiceAccount
metadata:
name: cozystack-controller
namespace: cozy-system - OR change the RoleBinding to reference the chart's templated service account name/namespace (e.g., values/.Release.Namespace) and ensure that name is created or provided by a dependency.
- OR document/declare a dependency if the SA is created by another chart so installs don't fail.
- Add a ServiceAccount manifest in the chart, e.g.:
🤖 Prompt for AI Agents
In packages/system/cozystack-controller/templates/role.yaml around lines 13-25
the RoleBinding references a ServiceAccount named "cozystack-controller" in
namespace "cozy-system" but no such ServiceAccount exists in the repo; fix by
either adding a ServiceAccount manifest to the chart (suggested file:
packages/system/cozystack-controller/templates/serviceaccount.yaml) with
metadata.name set to the same name/namespace, or modify the RoleBinding to
reference the chart's templated SA name/namespace (e.g., using the release
namespace or a value from values.yaml) and ensure that templated ServiceAccount
is created or declare the external chart dependency that provides it so
installation won't fail.
What this PR does
Release note
Summary by CodeRabbit
Chores
Bug Fixes