Skip to content

Use built-in view ClusterRole instead of manually defining all permissions - #1822

Open
arikalon1 wants to merge 1 commit into
masterfrom
claude/service-account-view-role-NhPqi
Open

arikalon1 wants to merge 1 commit into
masterfrom
claude/service-account-view-role-NhPqi

Conversation

@arikalon1

@arikalon1 arikalon1 commented Mar 21, 2026 •

Copy link
Copy Markdown
Collaborator

Bind the service account to the Kubernetes built-in "view" ClusterRole for
standard namespace-scoped read-only access (pods, deployments, services, etc.)
and keep only a supplementary ClusterRole for permissions not in "view":
cluster-scoped resources (nodes, namespaces, PVs, storageclasses), metrics,
RBAC, CRDs, webhooks, events, and operator CRDs.

This also removes dead entries (daemonsets/deployments listed under the core
API group where they don't exist) and eliminates duplicate RBAC/autoscaling
rule blocks.

https://claude.ai/code/session_01TFeEVLmsp8TSf8kYX4iuH1
Signed-off-by: Claude noreply@anthropic.com

Summary by CodeRabbit

  • Documentation

    • Added RBAC structure documentation describing the two-layer permission model using built-in Kubernetes view role and custom cluster-scoped permissions.
  • Refactor

    • Reorganized RBAC configuration to separate standard namespace-scoped read-only access from additional cluster-scoped resource permissions.

…sions

Bind the service account to the Kubernetes built-in "view" ClusterRole for
standard namespace-scoped read-only access (pods, deployments, services, etc.)
and keep only a supplementary ClusterRole for permissions not in "view":
cluster-scoped resources (nodes, namespaces, PVs, storageclasses), metrics,
RBAC, CRDs, webhooks, events, and operator CRDs.

This also removes dead entries (daemonsets/deployments listed under the core
API group where they don't exist) and eliminates duplicate RBAC/autoscaling
rule blocks.

https://claude.ai/code/session_01TFeEVLmsp8TSf8kYX4iuH1
Signed-off-by: Claude <noreply@anthropic.com>

@claude claude 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.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review.

@github-actions

github-actions Bot commented Mar 21, 2026 •

Copy link
Copy Markdown
Contributor

✅ Results of HolmesGPT evals

Automatically triggered by commit 9043ea9 on branch claude/service-account-view-role-NhPqi

View workflow logs

Results of HolmesGPT evals

  • ask_holmes: 10/10 test cases were successful, 0 regressions
Status Test case Time Turns Tools Cost Total tokens Input Max input Output Max output Cached Non-cached Reasoning Compactions
✅ 09_crashpod 26.4s 4 9 $0.3209 82,294 80,358 23,008 1,936 961 39,838 40,520 — —
✅ 101_loki_historical_logs_pod_deleted 53.6s 8 14 $0.3252 178,246 174,986 26,275 3,260 633 147,565 27,421 — —
✅ 111_pod_names_contain_service 32.5s 6 9 $0.2382 119,363 117,423 22,172 1,940 671 94,474 22,949 — —
✅ 112_find_pvcs_by_uuid 20.0s 4 3 $0.1879 78,651 77,705 21,794 946 365 55,899 21,806 — —
✅ 12_job_crashing 35.6s 6 16 $0.2921 139,771 137,259 26,224 2,512 675 108,834 28,425 — —
✅ 176_network_policy_blocking_traffic_no_runbooks 43.8s 6 14 $0.2886 129,758 127,139 25,505 2,619 551 98,773 28,366 — —
✅ 227_count_configmaps_per_namespace[0] 25.9s 6 10 $0.2138 113,622 112,097 20,641 1,525 515 91,236 20,861 — —
✅ 24_misconfigured_pvc 36.1s 7 13 $0.2699 142,874 140,586 23,472 2,288 594 115,449 25,137 — —
✅ 43_current_datetime_from_prompt 4.4s 1 — $0.0115 17,063 16,945 16,945 118 118 16,935 10 — —
✅ 61_exact_match_counting 10.6s 3 3 $0.1386 52,885 52,515 17,927 370 223 34,577 17,938 — —
Total 28.9s avg 5.1 avg 10.1 avg $2.2867 1,054,527 1,037,013 26,275 17,514 961 803,580 233,433 — —
Benchmark Comparison Details

Baseline: latest ci-benchmark experiment on master

Status: Success - 73 test/model combinations loaded

Benchmark experiment:

No benchmark data available for comparison.

Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run.

Comparison indicators:

  • ±0% — diff under 10% (within noise threshold)
  • ↑N%/↓N% — diff 10-25%
  • ↑N%/↓N% — diff over 25% (significant)
📖 Legend
Icon Meaning
✅ The test was successful
➖ 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
🔄 Re-run evals manually

⚠️ Warning: /eval comments always run using the workflow from master, not from this PR branch. If you modified the GitHub Action (e.g., added secrets or env vars), those changes won't take effect.

To test workflow changes, use the GitHub CLI or Actions UI instead:

gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/service-account-view-role-NhPqi -f markers=regression -f filter=

Option 1: Comment on this PR with /eval:

/eval
tags: regression

Or with more options (one per line):

/eval
model: gpt-4o
tags: regression
id: 09_crashpod
iterations: 5

Run evals on a different branch (e.g., master) for comparison:

/eval
branch: master
tags: regression
Option Description
model Model(s) to test (default: same as automatic runs)
tags Pytest tags / markers (no default - runs all tests!)
id Eval ID / pytest -k filter (use /list to see valid eval names)
iterations Number of runs, max 10
branch Run evals on a different branch (for cross-branch comparison)

Quick re-run: Use /rerun to re-run the most recent /eval on this PR with the same parameters.

Option 2: Trigger via GitHub Actions UI → "Run workflow"

Option 3: Add PR labels to include extra evals (applies to both automatic runs and /eval comments):

Label Effect
evals-tag-<name> Run tests with tag <name> alongside regression
evals-id-<name> Run a specific eval by test ID
evals-model-<name> Override the model (use model list name, e.g. sonnet-4.5)

Examples: evals-tag-easy, evals-id-09_crashpod, evals-model-sonnet-4.5

🏷️ Valid tags

benchmark, chain-of-causation, compaction, confluence, context_window, coralogix, counting, database, datadog, datetime, db-connectors, easy, elasticsearch, embeds, fast, frontend, grafana, hard, integration, kafka, kubernetes, leaked-information, logs, loki, mcp, medium, metrics, network, newrelic, no-cicd, numerical, one-test, port-forward, prometheus, question-answer, regression, runbooks, slackbot, storage, toolset-limitation, traces, transparency

🤖 Valid models

deepseek-chat, deepseek-r1-reasoner, deepseek-reasoner, deepseek-v3.2-chat, gemini-3-flash-preview, gemini-3-pro-preview, gemini-3.1-pro-preview, gpt-4.1, gpt-5.2-high-reasoning, gpt-5.3-codex, gpt-5.4, haiku-4.5, kimi-2.5, kimi-2.5-openrouter, opus-4.5, opus-4.6, qwen-next-80B-instruct, qwen-next-80B-thinking, sonnet-4.5, sonnet-4.6


Commands: /eval · /rerun · /list

CLI: gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/service-account-view-role-NhPqi -f markers=regression -f filter=

@github-actions

github-actions Bot commented Mar 21, 2026 •

Copy link
Copy Markdown
Contributor

✅ Docker images ready for 57596ec6 (built in 4m 52s)

⚠️ Warning: does not support ARM (ARM images are built on release only - not on every PR)

Use these tags to pull the images for testing.

📋 Copy commands

⚠️ 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:57596ec6
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:57596ec6 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:57596ec6
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:57596ec6
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:57596ec6
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:57596ec6 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:57596ec6
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:57596ec6

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:57596ec6 \
  --set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
  --set operator.image=holmes-operator-dev:57596ec6

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:57596ec6 \
  --set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
  --set holmes.operator.image=holmes-operator-dev:57596ec6

@coderabbitai

coderabbitai Bot commented Mar 21, 2026 •

Copy link
Copy Markdown
Contributor

Walkthrough

This change refactors the Kubernetes RBAC configuration to use a two-layer model: binding the built-in view ClusterRole for standard namespace-scoped read-only access, and a custom cluster role for additional cluster-scoped and supplementary permissions. Documentation was added to explain the model.

Changes

Cohort / File(s) Summary
Documentation
docs/reference/kubernetes-permissions.md
Added RBAC Structure section explaining the two-layer RBAC model: built-in view ClusterRole for namespace-scoped access and supplementary custom cluster role for cluster-scoped resources and additional permissions.
Helm RBAC Template
helm/holmes/templates/holmesgpt-service-account.yaml
Refactored ClusterRole rules to leverage built-in view ClusterRole; split single ClusterRoleBinding into two bindings (one for view, one for custom <release>-holmes-cluster-role) to separate namespace-scoped from cluster-scoped permissions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • RoiGlinik
  • moshemorad
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: leveraging Kubernetes' built-in view ClusterRole for namespace-scoped permissions instead of manually defining them all.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

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

Tip

You can disable sequence diagrams in the walkthrough.

Disable the reviews.sequence_diagrams setting to disable sequence diagrams in the walkthrough.

@netlify

netlify Bot commented Mar 21, 2026

Copy link
Copy Markdown

✅ Deploy Preview for holmes-docs ready!

Name Link
🔨 Latest commit 9043ea9
🔍 Latest deploy log https://app.netlify.com/projects/holmes-docs/deploys/69be8d626b56000008220c92
😎 Deploy Preview https://deploy-preview-1822--holmes-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@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: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@helm/holmes/templates/holmesgpt-service-account.yaml`:
- Around line 336-349: The ClusterRoleBinding that grants the built-in "view"
role to the Holmes ServiceAccount could be misread as giving CRD access via
aggregation; clarify this by adding a short comment next to the
ClusterRoleBinding in the template (the block that creates the
ClusterRoleBinding for {{ include "holmes.serviceAccountName" . }}) stating that
CRD permissions are NOT granted via the "view" binding but are controlled
explicitly by the chart's custom ClusterRole defined by crdPermissions.* (refer
to the custom ClusterRole resource name) and thus CRD allowlisting is enforced
separately.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1e8e61e8-c788-441e-af9d-14a5a8e6798a

📥 Commits

Reviewing files that changed from the base of the PR and between 2b470f9 and 9043ea9.

📒 Files selected for processing (2)
  • docs/reference/kubernetes-permissions.md
  • helm/holmes/templates/holmesgpt-service-account.yaml

Comment on lines +336 to +349
# Bind the built-in "view" ClusterRole for standard read-only access to
# namespace-scoped resources (pods, deployments, services, configmaps, etc.)
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: {{ .Release.Name }}-holmes-view-role-binding
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: view
subjects:
- kind: ServiceAccount
name: {{ include "holmes.serviceAccountName" . }}
namespace: {{ .Release.Namespace }}

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.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Search for crdPermissions definition and usage in the chart
rg "crdPermissions" helm/holmes/ -A 3 -B 1 --context 5

Repository: HolmesGPT/holmesgpt

Length of output: 7613


🏁 Script executed:

# Look at the complete RBAC configuration for context
cat -n helm/holmes/templates/holmesgpt-service-account.yaml | head -100

Repository: HolmesGPT/holmesgpt

Length of output: 2555


🏁 Script executed:

# Search for any other ClusterRole or ClusterRoleBinding definitions in the chart
rg "kind: ClusterRole|kind: ClusterRoleBinding" helm/holmes/templates/ -B 2 -A 5

Repository: HolmesGPT/holmesgpt

Length of output: 7254


🏁 Script executed:

# Check if there's any documentation about RBAC strategy in the chart
find helm/holmes -name "README*" -o -name "NOTES*" | xargs cat 2>/dev/null

Repository: HolmesGPT/holmesgpt

Length of output: 45


🌐 Web query:

Kubernetes built-in view ClusterRole RBAC aggregation aggregate-to-view

💡 Result:

Kubernetes has a built-in "view" ClusterRole that uses RBAC aggregation to allow extensions without direct modification. The "view" ClusterRole includes an aggregationRule that selects other ClusterRoles labeled with rbac.authorization.k8s.io/aggregate-to-view: "true". Creating a new ClusterRole with this label automatically adds its rules (typically read-only permissions like get, list, watch) to the aggregated "view" ClusterRole. This enables custom resources or additional permissions to be viewed by anyone bound to the "view" role, such as third-party operators or monitoring tools. Example to extend the view role: apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: name: custom-view-extension labels: rbac.authorization.k8s.io/aggregate-to-view: "true" rules: - apiGroups: ["example.com"] resources: ["widgets"] verbs: ["get", "list", "watch"] The built-in "view" role is designed for read-only access to most namespaced resources and uses this aggregation mechanism, as documented in official Kubernetes RBAC docs. Similar labels exist for "edit" (aggregate-to-edit) and "admin" (aggregate-to-admin).

Citations:


🌐 Web query:

Kubernetes RBAC view role what permissions included namespace-scoped

💡 Result:

In Kubernetes RBAC, there is no built-in "view" Role object (namespace-scoped). Instead, there is a built-in "view" ClusterRole that provides read-only access to most objects in a namespace when bound via a namespace-scoped RoleBinding. The "view" ClusterRole grants get, list, and watch permissions on most namespace-scoped resources across various API groups (core "", apps, batch, etc.), including pods, services, deployments, configmaps, etc. It excludes access to roles/rolebindings (to prevent viewing RBAC config) and secrets (to avoid privilege escalation via service account tokens). To view the exact permissions: kubectl get clusterrole view -o yaml When used in a RoleBinding (namespace-scoped), it limits access to that namespace only. Official description from Kubernetes docs: "Allows read-only access to see most objects in a namespace. It does not allow viewing roles or role bindings. This role does not allow viewing Secrets, since reading the contents of Secrets enables access to ServiceAccount credentials in the namespace, which would allow API access as any ServiceAccount in the namespace (a form of privilege escalation)."

Citations:


🏁 Script executed:

# Check if crdPermissions rules reference namespaced or cluster-scoped resources
cat -n helm/holmes/templates/holmesgpt-service-account.yaml | sed -n '200,350p' | grep -A 10 "crdPermissions"

Repository: HolmesGPT/holmesgpt

Length of output: 1968


🏁 Script executed:

# Look at the scope of built-in view role - verify it's namespaced
# Check what verbs are in the view binding rules section
cat -n helm/holmes/templates/holmesgpt-service-account.yaml | sed -n '330,360p'

Repository: HolmesGPT/holmesgpt

Length of output: 1312


The view ClusterRole aggregation doesn't apply to CRD permissions here—they remain explicitly gated.

The concern about aggregation is technically valid: view is an aggregated role that admins can extend via rbac.authorization.k8s.io/aggregate-to-view: "true" labels, so Holmes would gain access to any CRDs admins add to it cluster-wide. However, CRD permissions in this chart (crdPermissions.*) are defined in the separate custom ClusterRole, not the view binding. The chart maintains an explicit allowlist for CRDs through per-flag controls, which is independent of what cluster admins do with view aggregation.

The design trade-off is acceptable: using the standard view role for namespace-scoped resources (pods, services, etc.) is conventional and maintainable, while cluster-scoped resources and optional CRDs remain under chart control via the custom role. If deterministic CRD visibility is a requirement regardless of cluster-wide view extensions, this is already satisfied by keeping CRD rules in the explicit custom ClusterRole.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@helm/holmes/templates/holmesgpt-service-account.yaml` around lines 336 - 349,
The ClusterRoleBinding that grants the built-in "view" role to the Holmes
ServiceAccount could be misread as giving CRD access via aggregation; clarify
this by adding a short comment next to the ClusterRoleBinding in the template
(the block that creates the ClusterRoleBinding for {{ include
"holmes.serviceAccountName" . }}) stating that CRD permissions are NOT granted
via the "view" binding but are controlled explicitly by the chart's custom
ClusterRole defined by crdPermissions.* (refer to the custom ClusterRole
resource name) and thus CRD allowlisting is enforced separately.

This branch has not been deployed

No deployments
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