Skip to content

ROB-2714 default crd - #1243

Merged
RoiGlinik merged 3 commits into
masterfrom
ROB-2714-default-crd
Dec 25, 2025
Merged

RoiGlinik merged 3 commits into
masterfrom
ROB-2714-default-crd

Conversation

@RoiGlinik

@RoiGlinik RoiGlinik commented Dec 25, 2025 •

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • Bug Fixes / Security Improvements

    • Replaced overly permissive wildcard permissions with specific, enumerated resource access controls across multiple integrations (Argo, Flux, Kafka, Keda, Crossplane, Istio, Gateway API, Velero).
  • New Features

    • Added support for external-secrets integration and configuration.

✏️ Tip: You can customize this high-level summary in your review settings.

Signed-off-by: Roi Glinik <groi.tech@gmail.com>
Signed-off-by: Roi Glinik <groi.tech@gmail.com>
@RoiGlinik
RoiGlinik requested a review from arikalon1 December 25, 2025 11:28
@coderabbitai

coderabbitai Bot commented Dec 25, 2025 •

Copy link
Copy Markdown
Contributor

Walkthrough

This pull request tightens RBAC security by replacing wildcard resource permissions with explicit, enumerated resource lists across multiple Kubernetes API groups (monitoring, argoproj, flux, kafka, keda, crossplane, istio, gateway, velero, external-secrets) in the Holmes Helm chart. Documentation and configuration values are updated to reflect external-secrets support.

Changes

Cohort / File(s) Summary
Documentation & Configuration
docs/data-sources/permissions.md, helm/holmes/values.yaml
Added externalSecrets: true to CRD permissions configuration for Holmes and Robusta Helm Charts; minor cleanup of trailing whitespace in values file.
RBAC Service Account Template
helm/holmes/templates/holmesgpt-service-account.yaml
Replaced wildcard resource permissions (*) with explicit resource lists for 9+ API groups (monitoring, argoproj, flux, kafka, keda, crossplane, istio, gateway, velero, external-secrets). Added conditional permission blocks controlled by .Values.crdPermissions and enumerated specific verbs (get, list, watch) per resource type.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • moshemorad
  • arikalon1
  • Avi-Robusta

Pre-merge checks

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'ROB-2714 default crd' is vague and doesn't clearly convey the actual changes made. The PR adds explicit RBAC permissions for multiple CRDs (external-secrets, flux, kafka, keda, crossplane, istio, gateway API, velero) replacing wildcards with specific resources, but the title only says 'default crd' without describing what was done or which CRDs are affected. Revise the title to be more descriptive, such as 'Add explicit RBAC permissions for external-secrets and other CRDs' or 'Replace wildcard RBAC rules with explicit resource permissions for multiple CRDs'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 95ed687 and fa468d6.

📒 Files selected for processing (3)
  • docs/data-sources/permissions.md
  • helm/holmes/templates/holmesgpt-service-account.yaml
  • helm/holmes/values.yaml
🧰 Additional context used
📓 Path-based instructions (1)
docs/**/*.md

📄 CodeRabbit inference engine (CLAUDE.md)

When writing MkDocs documentation, always add a blank line between headers/bold text and lists to ensure proper rendering

Files:

  • docs/data-sources/permissions.md
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-25T11:30:26.515Z
Learning: Applies to holmes/plugins/toolsets/**/kubernetes* : RBAC permissions must be respected for Kubernetes access
📚 Learning: 2025-12-25T11:30:26.515Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-25T11:30:26.515Z
Learning: Applies to holmes/plugins/toolsets/**/kubernetes* : RBAC permissions must be respected for Kubernetes access

Applied to files:

  • docs/data-sources/permissions.md
  • helm/holmes/templates/holmesgpt-service-account.yaml
📚 Learning: 2025-12-25T11:30:26.515Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-25T11:30:26.515Z
Learning: Applies to tests/llm/**/*.{yaml,yml} : Use ALWAYS use Secrets for scripts in Kubernetes manifests, not inline manifests or ConfigMaps, to prevent code visibility with kubectl describe

Applied to files:

  • docs/data-sources/permissions.md
  • helm/holmes/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). (5)
  • GitHub Check: build (3.11)
  • GitHub Check: build (3.12)
  • GitHub Check: build (3.10)
  • GitHub Check: llm_evals
  • GitHub Check: build
🔇 Additional comments (4)
docs/data-sources/permissions.md (1)

24-24: Documentation updated correctly for externalSecrets.

The addition of externalSecrets: true to both the Holmes and Robusta Helm Chart examples is consistent with the changes in values.yaml and the RBAC template. The MkDocs formatting follows the coding guidelines with proper spacing between headers and lists.

Also applies to: 41-41

helm/holmes/templates/holmesgpt-service-account.yaml (2)

188-209: Excellent security improvement: Prometheus CRDs now use explicit resource lists.

Replacing wildcard permissions with explicit resource lists follows least-privilege principles. The comprehensive list covers alertmanagers, prometheuses, prometheusagents, thanosrulers, and related monitoring resources with appropriate read-only verbs.


210-320: LGTM! Comprehensive CRD permissions for Argo, Flux, Kafka, KEDA, and Crossplane.

The explicit resource lists for each operator are well-structured:

  • Argo: Covers CD, Workflows, Rollouts, and Events resources
  • Flux: Properly separates source, kustomize, helm, and notification toolkit API groups
  • Kafka (Strimzi): Includes all major Kafka CRDs
  • KEDA: Covers scaled objects/jobs and trigger authentications
  • Crossplane: Separates pkg and apiextensions groups appropriately

All use read-only verbs consistent with HolmesGPT's analysis requirements.

helm/holmes/values.yaml (1)

42-43: LGTM. New CRD permission flags added consistently.

The addition of velero and externalSecrets permission flags aligns with the corresponding RBAC rules in the service account template. These permissions are read-only (get, list, watch verbs only) and remain individually toggleable via the crdPermissions section. This design is documented in the permissions guide as the intended default behavior.


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
helm/holmes/templates/holmesgpt-service-account.yaml (1)

322-396: Add pushsecrets to the External Secrets resources for completeness.

The external-secrets.io section is missing pushsecrets, which is an official CRD in the external-secrets operator. Following the comprehensive approach of other sections (Istio, Gateway API, Velero), add it to the resource list:

{{- if .Values.crdPermissions.externalSecrets }}
  - apiGroups:
      - external-secrets.io
    resources:
      - externalsecrets
      - secretstores
      - clustersecretstores
      - clusterexternalsecrets
      - pushsecrets
    verbs:
      - get
      - list
      - watch
{{- end }}
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 95ed687 and fa468d6.

📒 Files selected for processing (3)
  • docs/data-sources/permissions.md
  • helm/holmes/templates/holmesgpt-service-account.yaml
  • helm/holmes/values.yaml
🧰 Additional context used
📓 Path-based instructions (1)
docs/**/*.md

📄 CodeRabbit inference engine (CLAUDE.md)

When writing MkDocs documentation, always add a blank line between headers/bold text and lists to ensure proper rendering

Files:

  • docs/data-sources/permissions.md
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-25T11:30:26.515Z
Learning: Applies to holmes/plugins/toolsets/**/kubernetes* : RBAC permissions must be respected for Kubernetes access
📚 Learning: 2025-12-25T11:30:26.515Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-25T11:30:26.515Z
Learning: Applies to holmes/plugins/toolsets/**/kubernetes* : RBAC permissions must be respected for Kubernetes access

Applied to files:

  • docs/data-sources/permissions.md
  • helm/holmes/templates/holmesgpt-service-account.yaml
📚 Learning: 2025-12-25T11:30:26.515Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-25T11:30:26.515Z
Learning: Applies to tests/llm/**/*.{yaml,yml} : Use ALWAYS use Secrets for scripts in Kubernetes manifests, not inline manifests or ConfigMaps, to prevent code visibility with kubectl describe

Applied to files:

  • docs/data-sources/permissions.md
  • helm/holmes/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). (5)
  • GitHub Check: build (3.11)
  • GitHub Check: build (3.12)
  • GitHub Check: build (3.10)
  • GitHub Check: llm_evals
  • GitHub Check: build
🔇 Additional comments (4)
docs/data-sources/permissions.md (1)

24-24: Documentation updated correctly for externalSecrets.

The addition of externalSecrets: true to both the Holmes and Robusta Helm Chart examples is consistent with the changes in values.yaml and the RBAC template. The MkDocs formatting follows the coding guidelines with proper spacing between headers and lists.

Also applies to: 41-41

helm/holmes/templates/holmesgpt-service-account.yaml (2)

188-209: Excellent security improvement: Prometheus CRDs now use explicit resource lists.

Replacing wildcard permissions with explicit resource lists follows least-privilege principles. The comprehensive list covers alertmanagers, prometheuses, prometheusagents, thanosrulers, and related monitoring resources with appropriate read-only verbs.


210-320: LGTM! Comprehensive CRD permissions for Argo, Flux, Kafka, KEDA, and Crossplane.

The explicit resource lists for each operator are well-structured:

  • Argo: Covers CD, Workflows, Rollouts, and Events resources
  • Flux: Properly separates source, kustomize, helm, and notification toolkit API groups
  • Kafka (Strimzi): Includes all major Kafka CRDs
  • KEDA: Covers scaled objects/jobs and trigger authentications
  • Crossplane: Separates pkg and apiextensions groups appropriately

All use read-only verbs consistent with HolmesGPT's analysis requirements.

helm/holmes/values.yaml (1)

42-43: LGTM. New CRD permission flags added consistently.

The addition of velero and externalSecrets permission flags aligns with the corresponding RBAC rules in the service account template. These permissions are read-only (get, list, watch verbs only) and remain individually toggleable via the crdPermissions section. This design is documented in the permissions guide as the intended default behavior.

@github-actions

github-actions Bot commented Dec 25, 2025 •

Copy link
Copy Markdown
Contributor

Dev Docker images are ready for this commit:

Use this tag to pull the image for testing.

⚠️ Temporary images are deleted after 30 days. Copy to a permanent registry before using them:

gcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:2556bd1
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:2556bd1 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:2556bd1
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:2556bd1

Patch Helm values in one line (choose the chart you use):

  • HolmesGPT chart:
helm upgrade --install holmesgpt ./helm/holmes \
  --set registry=me-west1-docker.pkg.dev/robusta-development/development \
  --set image=holmes-dev:2556bd1
  • Robusta wrapper chart:
helm upgrade --install robusta robusta/robusta \
  --reuse-values \
  --set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
  --set holmes.image=holmes-dev:2556bd1

@github-actions

Copy link
Copy Markdown
Contributor

Results of HolmesGPT evals

  • ask_holmes: 7/7 test cases were successful, 0 regressions
Test suite Test case Status
ask 09_crashpod ✅
ask 101_loki_historical_logs_pod_deleted ✅
ask 12_job_crashing ✅
ask 162_get_runbooks ✅
ask 176_network_policy_blocking_traffic_no_runbooks ✅
ask 43_current_datetime_from_prompt ✅
ask 61_exact_match_counting ✅

Legend

  • ✅ the test was successful
  • :minus: the test was skipped
  • ⚠️ the test failed but is known to be flaky or known to fail
  • 🚧 the test had a setup failure (not a code regression)
  • 🔧 the test failed due to mock data issues (not a code regression)
  • 🚫 the test was throttled by API rate limits/overload
  • ❌ the test failed and should be fixed before merging the PR

@RoiGlinik
RoiGlinik merged commit ea96356 into master Dec 25, 2025
10 of 11 checks passed
@RoiGlinik
RoiGlinik deleted the ROB-2714-default-crd branch December 25, 2025 16:53
FilipGrebowski pushed a commit to FilipGrebowski/holmesgpt that referenced this pull request Dec 27, 2025
Signed-off-by: Roi Glinik <groi.tech@gmail.com>
Co-authored-by: arik <alon.arik@gmail.com>
Signed-off-by: Filip Grebowski <grebowskifilip@gmail.com>
@coderabbitai coderabbitai Bot mentioned this pull request Dec 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants