ROB-2714 more default crd permissions - #1204
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>
Signed-off-by: Roi Glinik <groi.tech@gmail.com>
WalkthroughReworks in-cluster permissions docs and adds configurable per-CRD read permissions to the Holmes Helm chart via a new Changes
Sequence Diagram(s)(omitted — changes are documentation and Helm templating/configuration only) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (5)
🔇 Additional comments (3)
Comment |
![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
🧹 Nitpick comments (2)
helm/holmes/templates/holmesgpt-service-account.yaml (1)
184-209: Consider clarifying the comment to reflect both Prometheus and optional CRD permissions.The comment
# Prometheus CRDson line 184 now precedes not just Prometheus CRDs but also conditionally-included CRDs for Argo, Flux, Kafka, KEDA, Crossplane, Istio, Gateway API, and Velero. Consider updating the comment to reflect the broader scope of CRD permissions in this section.Suggested comment update
- # Prometheus CRDs + # Prometheus CRDs and optional CRD permissions for common operatorsdocs/data-sources/permissions.md (1)
8-41: Consider adding more context about what each CRD permission enables.The documentation shows the available CRD permission flags but doesn't explain what each one controls or when you might want to disable them. Adding brief descriptions would help users make informed decisions about which permissions to enable.
Suggested enhancement
Consider adding a table or list explaining each flag:
## Default CRD Permissions HolmesGPT includes read-only permissions for common Kubernetes operators and tools by default. These can be individually enabled or disabled: - **argo**: ArgoCD applications, Argo Workflows, and Argo Rollouts - **flux**: Flux GitOps toolkit resources (sources, kustomizations, helm releases) - **kafka**: Strimzi Kafka operator resources - **keda**: KEDA autoscaling resources - **crossplane**: Crossplane providers and compositions - **istio**: Istio service mesh resources - **gatewayApi**: Kubernetes Gateway API resources - **velero**: Velero backup and restore resources
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/data-sources/permissions.md(2 hunks)helm/holmes/templates/holmesgpt-service-account.yaml(2 hunks)helm/holmes/values.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). (5)
- GitHub Check: llm_evals
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build
🔇 Additional comments (2)
docs/data-sources/permissions.md (1)
51-90: LGTM! Cert-Manager is a good example choice.The switch from ArgoCD to Cert-Manager for the custom permissions example is appropriate, since Cert-Manager is common but not included in the default CRD permissions. The examples correctly show the syntax for both Holmes and Robusta Helm charts.
helm/holmes/templates/holmesgpt-service-account.yaml (1)
210-383: Well-structured RBAC configuration across multiple operators.The conditional permission blocks are consistently implemented and appropriate for read-only access. The listed CRD resources align with documented APIs for ArgoCD, Flux CD, Strimzi Kafka, KEDA, and Crossplane. However, verification against the latest Istio, Kubernetes Gateway API, and Velero operator documentation would be beneficial to ensure complete accuracy, particularly for operators that may have evolved their API structures in recent releases.
|
Dev Docker images are ready for this commit: Use this tag to pull the image for testing. gcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:dbb42b1
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:dbb42b1 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:dbb42b1
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:dbb42b1Patch Helm values in one line (choose the chart you use):
helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:dbb42b1
helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:dbb42b1 |
arikalon1
left a comment
arikalon1
left a comment
There was a problem hiding this comment.
looks good., left small comment
Signed-off-by: Roi Glinik <groi.tech@gmail.com>
de9596f
Signed-off-by: Roi Glinik <groi.tech@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
helm/holmes/templates/holmesgpt-service-account.yaml (1)
193-312: Well-structured conditional CRD permission blocks with proper defaults.The implementation is clean and consistent across all CRD groups. The
crdPermissionsdefaults are properly defined in values.yaml with all groups enabled. A few observations:
Istio coverage: Consider adding
security.istio.iofor resources likeAuthorizationPolicyandPeerAuthentication, which are commonly needed when troubleshooting Istio-related issues.Crossplane coverage: The
crossplane.iocore API group (forCompositeResourceDefinitions,Compositions) may also be useful, though the current coverage ofpkgandapiextensionshandles the most common troubleshooting scenarios.🔎 Optional: Add Istio security API group
{{- if .Values.crdPermissions.istio }} - apiGroups: - networking.istio.io resources: - "*" verbs: - get - list - watch - apiGroups: - telemetry.istio.io resources: - "*" verbs: - get - list - watch + - apiGroups: + - security.istio.io + resources: + - "*" + verbs: + - get + - list + - watch {{- end }}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
helm/holmes/templates/holmesgpt-service-account.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). (5)
- GitHub Check: llm_evals
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build
🔇 Additional comments (1)
helm/holmes/templates/holmesgpt-service-account.yaml (1)
184-192: LGTM! Good implementation of the wildcard approach.This addresses the previous review feedback by using
"*"for resources undermonitoring.coreos.com, ensuring any future Prometheus Operator CRDs are automatically covered without requiring chart updates.
|
Dev Docker images are ready for this commit:
Use either tag to pull the image for testing. |
|
Dev Docker images are ready for this commit:
Use either tag to pull the image for testing. |
add default read permission to some kubernetes tools --------- Signed-off-by: Roi Glinik <groi.tech@gmail.com> Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
add default read permission to some kubernetes tools --------- Signed-off-by: Roi Glinik <groi.tech@gmail.com> Signed-off-by: Filip Grebowski <grebowskifilip@gmail.com>
3 participants
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.