-
Notifications
You must be signed in to change notification settings - Fork 288
ROB-2714 more crds default access #1976
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Roi Glinik <groi.tech@gmail.com>
Signed-off-by: Roi Glinik <groi.tech@gmail.com>
Signed-off-by: Roi Glinik <groi.tech@gmail.com>
WalkthroughAdds configurable per-tool CRD read-only permissions to the runner via Helm values (including a new externalSecrets toggle), updates the ClusterRole template to conditionally include those RBAC rules, introduces a top-level Changes
Sequence Diagram(s)(omitted — changes are configuration/docs and conditional RBAC additions without multi-component control-flow that warrants a sequence diagram) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (3)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
helm/robusta/values.yaml (1)
115-117: Consider using lowercase YAML boolean convention.The value
Falseuses Python-style capitalization. YAML convention typically uses lowercasefalsefor boolean values, though both are valid.🔎 Proposed fix
-scaleAlertsProcessing: False +scaleAlertsProcessing: falsedocs/setup-robusta/crds.rst (1)
86-103: LGTM! Clear documentation of the new feature.The new "Default CRD Permissions" section clearly explains the opt-out model and provides a helpful code example with inline comments describing each tool.
Consider adding a brief note about the relationship between
runner.crdPermissions.*and the existingrunner.customCRD/argoRolloutsconfigurations, since they provide overlapping functionality for some resources. This would help users understand when to use each approach.Example addition
.. note:: The ``crdPermissions`` flags provide read-only access to common operators. For write access or custom CRDs not covered by these defaults, use ``customClusterRoleRules`` as shown in the Basic Configuration section above.helm/robusta/templates/runner-service-account.yaml (1)
402-428: Document the configuration strategy for overlapping StrimziPodSet permissions.Lines 298-308 grant write-level permissions to
strimzipodsets(undercore.strimzi.io) when"StrimziPodSet"is in.Values.runner.customCRD(withpatchandupdateverbs). Lines 402-428 grant read-only access to the same resource via.Values.runner.crdPermissions.kafka. Both configuration methods target the same resource but with different permission levels, and both can be independently enabled. This creates potential confusion about which configuration method to use and what the expected permission scope should be. Consider documenting the intended relationship and migration path between these two configuration approaches, particularly for overlapping Strimzi resources.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/setup-robusta/crds.rsthelm/robusta/templates/runner-service-account.yamlhelm/robusta/values.yaml
⏰ 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: run_tests
🔇 Additional comments (8)
helm/robusta/values.yaml (1)
742-751: LGTM! Well-structured CRD permissions configuration.The new
crdPermissionssection provides a clean, granular way to control read-only access to common operators. Defaulting totrueoffers good out-of-the-box visibility while allowing users to disable specific tools as needed.helm/robusta/templates/runner-service-account.yaml (7)
351-401: LGTM! Comprehensive Flux CD permissions.The Flux CD permissions cover all major Flux toolkit API groups with appropriate read-only access. This provides full visibility into GitOps resources without granting modification rights.
429-442: LGTM! KEDA permissions are appropriate.The KEDA autoscaler permissions cover all essential resources with read-only access, providing visibility into scaling configurations without modification rights.
443-465: LGTM! Crossplane permissions are well-structured.The Crossplane permissions appropriately cover both package management and composition resources with read-only access, enabling visibility into infrastructure provisioning without granting modification rights.
466-493: LGTM! Comprehensive Istio service mesh permissions.The Istio permissions cover both networking and security resources with appropriate read-only access, providing full visibility into service mesh configuration.
494-511: LGTM! Gateway API permissions are complete.The Gateway API permissions cover all standard gateway and route resources with read-only access, providing visibility into ingress configuration using the Gateway API standard.
512-531: LGTM! Velero backup permissions are comprehensive.The Velero permissions cover all essential backup, restore, and schedule resources with read-only access, enabling visibility into disaster recovery operations without granting modification rights.
331-350: Review the overlap between.Values.argoRolloutsand.Values.runner.crdPermissions.argo.The
argoRolloutsflag (line 287) grants write permissions (patch,update) torolloutsand also sets an environment variable to enable Argo Rollouts-specific logic in the runner. The newcrdPermissions.argoblock (line 331) grants read-only access to a broader set of Argo resources, includingrollouts.If both flags are enabled, the service account gains both read and write permissions for rollouts, which is redundant. More importantly, users may be confused about which flag controls what:
argoRolloutsappears to be a legacy flag with no documentation invalues.yaml, whilecrdPermissions.argois part of a modern, organized structure. Consider clarifying the intended relationship between these two flags or documenting when each should be used.
Signed-off-by: Roi Glinik <groi.tech@gmail.com>
No description provided.