Conversation
Approval is now purely tool-name based via approval_required_tools. The legacy restricted_tools / skill-gating coupling (skills unlocking restricted tools) is removed entirely — it is no longer needed. Core: - tools.py: drop Tool.restricted, Tool._is_restricted(), and the restricted_tools fields on Toolset and ToolsetYamlFromConfig. The approval_required_tools / _check_approval_config / requires_approval path is kept as-is (this is what gates run_kubectl_command). - tool_calling_llm.py: drop _skill_in_use and _should_include_restricted_tools; stop passing include_restricted. Skill fetching itself is untouched. - tool_executor.py / frontend_tools.py: drop the include_restricted param/filter and the _is_restricted override. Helm (mcpAddons.kubernetesRemediation): - values: drop restrictedTools; approvalRequiredTools -> ["run_kubectl_command"]; clusterRole "" (chart creates a scoped role); networkPolicy on by default; image 1.1.0; new config keys (preapproved commands, diagnostic images, file-read paths, allowArbitraryKubectlCommands). - new rbac.yaml: scoped least-privilege ClusterRole (no cluster-admin, no secret access), gated on serviceAccount.create and empty clusterRole. - deployment: wire the new env vars; binding no longer defaults to cluster-admin. - networkpolicy: ingress-only, scoped to Holmes pods in the release namespace. - _helpers.tpl: rewrite llm_instructions around the auto-approved vs approval-gated tool split. - toolset-config: emit approval_required_tools instead of restricted_tools. Docs: rewrite kubernetes-remediation-mcp.md (5-tool table with approval column, scoped RBAC sample, plug-and-play defaults, CLI config without restricted_tools). Tests: tool-name approval gating, and a helm values/template regression check. Signed-off-by: Claude <noreply@anthropic.com>
Backwards compatibility: old configs carrying the removed restricted_tools key now load with a deprecation warning instead of silently dropping it, guiding users to approval_required_tools. Signed-off-by: Claude <noreply@anthropic.com>
Addresses CodeRabbit nitpick / repo guideline to keep imports at file top. Signed-off-by: Claude <noreply@anthropic.com>
- specs/kubernetes-remediation-mcp.md: full design + security review across both repos (tool taxonomy, RBAC/NetworkPolicy, the agent-core removal, the actual security boundaries, residual risks, test/eval coverage) to ease team review. - docs: path-policy row now reflects in-container symlink resolution and the hard /proc,/sys,/dev denial (server PR robusta-dev/holmes-mcp-integrations#24). Signed-off-by: Claude <noreply@anthropic.com>
Master's multi_instance.py (merged in) propagated the now-removed Toolset.restricted_tools to child toolsets in _forward_overrides, which broke all multi-instance toolset tests (azure_sql/datadog/elasticsearch/...) once this branch's removal met it. Drop the restricted_tools propagation (keep approval_required_tools) and fix the _RoutingTool docstring accordingly. Signed-off-by: Claude <noreply@anthropic.com>
…ecklist Addresses CodeRabbit review on specs/kubernetes-remediation-mcp.md: - §6.6 Audit logging: what the server/HolmesGPT/k8s capture today and the forwarding/retention/alerting gaps. - §6.4: resource-exhaustion risk from concurrent diagnostic pods (+ mitigations), and the NetworkPolicy-inert-without-CNI / direct-call monitoring implications of the cross-system approval boundary. - §8: eval acceptance checklist separating authorization tests from LLM-behavior evals, prioritizing the auto-approved tools and noting the harness hook the gated tool needs. Signed-off-by: Claude <noreply@anthropic.com>
… note Address CodeRabbit nitpicks on the k8s-remediation spec (§6.4): - Enumerate which CNIs enforce NetworkPolicy (Calico/Cilium/Antrea/Weave Net vs plain Flannel) plus managed-cluster caveats and a quick detection method, so operators can validate the approval-boundary backstop. - Add a tracking-status note to the resource-exhaustion mitigations matching the §8 style (ResourceQuota/LimitRange available today; server-side concurrency cap is a follow-up, not blocking this PR). Signed-off-by: Claude <noreply@anthropic.com>
…uth/steer-away framing The page was underselling the remediation MCP and steering readers toward the default toolset: - Dropped the shared kubernetes_toolset_picker snippet here (its OAuth/OIDC comparison is relevant on the kubernetes-mcp page, not this one) and replaced it with capability-first positioning. - Lead with what this MCP adds over read-only access (act on the cluster + deeper in-container diagnostics), with a comparison table. - Removed the 'Don't use this server for reads' / 'Prefer the no-approval tools' lines that read as discouragement; reframed the built-in toolset as complementary and the approval split as a positive design property. Signed-off-by: Claude <noreply@anthropic.com>
…ostic_image Track the server-side tool rename (#24) so the docs, spec, Helm LLM instructions, and unit tests reference the tool by its new name, consistent with run_preapproved_kubectl_command. 11 unit tests pass. Signed-off-by: Claude <noreply@anthropic.com>
Follow the holmes-mcp-integrations redesign: the auto-approved exec tool now takes pod/namespace/container/command as separate parameters and uses a plain binary allowlist (no wildcards). Update Helm wiring (preapprovedCommands -> preapprovedExecBinaries, KUBECTL_PREAPPROVED_COMMANDS -> KUBECTL_PREAPPROVED_EXEC_BINARIES), the LLM instructions in _helpers.tpl, the docs tool/config tables and Security Controls, the spec, and the helm/approval unit tests. Signed-off-by: Claude <noreply@anthropic.com>
Contributor
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:d15714980
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:d15714980 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:d15714980
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:d15714980
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:d15714980
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:d15714980 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:d15714980
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:d15714980Patch 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:d15714980 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:d15714980Robusta 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:d15714980 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:d15714980 |
Contributor
WalkthroughThis PR renames the preapproved kubectl exec tool, changes its allowlist from command patterns to exact binary names, updates the Helm config and env wiring, and aligns docs and tests with the new contract. ChangesKubernetes Remediation MCP exec preapproval
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ 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. Comment |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helm/holmes/templates/mcp-servers/kubernetes-remediation/deployment.yaml`:
- Line 22: The Kubernetes remediation deployment template now reads only the new
config.preapprovedExecBinaries key, which drops existing overrides from the
legacy config.preapprovedCommands value during upgrades. Update the deployment
template and related Helm wiring to preserve backward compatibility by
translating or falling back from config.preapprovedCommands to the server’s
KUBECTL_PREAPPROVED_COMMANDS input, and add a regression test covering an
upgrade path where only the deprecated Helm key is set.
🪄 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: af77e48b-c753-4e44-b9a5-11e9aa6465db
📒 Files selected for processing (7)
docs/data-sources/builtin-toolsets/kubernetes-remediation-mcp.mdhelm/holmes/templates/mcp-servers/kubernetes-remediation/_helpers.tplhelm/holmes/templates/mcp-servers/kubernetes-remediation/deployment.yamlhelm/holmes/values.yamlspecs/kubernetes-remediation-mcp.mdtests/test_approval_required_tools.pytests/test_kubernetes_remediation_helm.py
Match the server clean break — no deprecated legacy env var fallback. Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
specs/kubernetes-remediation-mcp.md (1)
109-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the deprecated env var in the compatibility docs.
The PR summary says
KUBECTL_PREAPPROVED_COMMANDSis still accepted with a warning, but this table now drops it entirely. A brief deprecated/compatibility note here would prevent existing installs from reading this as a hard break.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/kubernetes-remediation-mcp.md` at line 109, The compatibility docs currently list only KUBECTL_PREAPPROVED_EXEC_BINARIES, which makes the deprecated KUBECTL_PREAPPROVED_COMMANDS alias look removed; update the environment-variable table in the Kubernetes remediation spec to keep the deprecated name visible as a compatibility entry and note that it is still accepted with a warning. Use the existing docs section around the allowlist/env var table to add a brief deprecated or legacy note rather than deleting the old symbol entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@specs/kubernetes-remediation-mcp.md`:
- Line 109: The compatibility docs currently list only
KUBECTL_PREAPPROVED_EXEC_BINARIES, which makes the deprecated
KUBECTL_PREAPPROVED_COMMANDS alias look removed; update the environment-variable
table in the Kubernetes remediation spec to keep the deprecated name visible as
a compatibility entry and note that it is still accepted with a warning. Use the
existing docs section around the allowlist/env var table to add a brief
deprecated or legacy note rather than deleting the old symbol entirely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99b40ed2-12e6-416f-a3a6-8392f788f122
📒 Files selected for processing (1)
specs/kubernetes-remediation-mcp.md
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
HolmesGPT-side companion to the Kubernetes Remediation MCP exec redesign (robusta-dev/holmes-mcp-integrations).
The auto-approved exec tool was renamed
run_preapproved_kubectl_command→run_preapproved_kubectl_exec_commandand now takespod/namespace/container/commandas separate parameters with a plain, wildcard-free binary allowlist. This PR threads that rename through the chart, docs, and spec.Changes
config.preapprovedCommands→config.preapprovedExecBinaries(ps,top,df,ls,netstat,ss); ConfigMap/envKUBECTL_PREAPPROVED_COMMANDS→KUBECTL_PREAPPROVED_EXEC_BINARIES(values.yaml,deployment.yaml)._helpers.tpl): updated tool name, usage guidance (passpod/namespace/command, nokubectl/exec/--), and example.kubernetes-remediation-mcp.md): Available Tools row, Security Controls row, Configuration Reference.specs/kubernetes-remediation-mcp.md): tool contract, env table, residual-risk + acceptance references.test_kubernetes_remediation_helm.py,test_approval_required_tools.py. 11/11 pass.Compatibility
This is a clean break, not a deprecation. The old
preapprovedCommands/KUBECTL_PREAPPROVED_COMMANDSname is removed entirely (no chart shim, no server fallback). That's safe because the old key never shipped in a released chart — it was introduced in #2148 (92c27b1e) and renamed here, both after the latest release0.33.0(whosevalues.yamlhas nopreapprovedCommands), so there is no installed base to migrate.🤖 Generated with Claude Code