ROB-3465 k8s mcp addon - #1992
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>
Signed-off-by: Roi Glinik <groi.tech@gmail.com>
Signed-off-by: Roi Glinik <groi.tech@gmail.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
WalkthroughAdds a configurable Kubernetes MCP server Helm addon and RBAC/service-account controls: new ChangesKubernetes MCP Server Addon & RBAC
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as rgba(66,133,244,0.5) User/Helm
participant Helm as rgba(219,68,55,0.5) Helm chart
participant K8sAPI as rgba(15,157,88,0.5) Kubernetes API
participant MCP as rgba(171,71,188,0.5) MCP Deployment
participant SecretStore as rgba(244,180,0,0.5) Secret/ConfigMap
User->>Helm: render with mcpAddons.kubernetes enabled
Helm->>Helm: validate config sources (fail if both serverConfig & configSecret)
Helm->>SecretStore: optionally create Secret (stringData.config.toml)
Helm->>K8sAPI: apply ConfigMap, Secret, Deployment, Service, ServiceAccount, NetworkPolicy, ClusterRoleBinding (conditional)
K8sAPI->>MCP: schedule pod with mounts (kubeconfig / config)
MCP->>K8sAPI: probe readiness/liveness on :8000
User->>MCP: interact via Service on port 8000
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 failed. Why did it fail? →
|
📂 Previous Runs📜 #3 · Run @ __6e907fd__ (#25430775664) — May 6, 10:55 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 6e907fd on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: 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:
📜 #2 · Run @ __244f46a__ (#25425657473) — May 6, 08:59 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 244f46a on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: 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:
📜 #1 · Run @ __060fd1d__ (#25306655305) — May 4, 07:40 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 060fd1d on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: 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:
✅ Results of HolmesGPT evalsAutomatically triggered by commit cfd2ed3 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: 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:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" Option 3: Add PR labels to include extra evals (applies to both automatic runs and
Examples: 🏷️ Valid tags
🤖 Valid models
Commands: CLI: |
|
✅ 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:bf6b1ccb
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:bf6b1ccb me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:bf6b1ccb
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:bf6b1ccb
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:bf6b1ccb
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:bf6b1ccb me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:bf6b1ccb
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:bf6b1ccbPatch 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:bf6b1ccb \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:bf6b1ccbRobusta 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:bf6b1ccb \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:bf6b1ccb |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-rbac-service-account.yaml`:
- Around line 1-18: The template holmesgpt-rbac-service-account.yaml creates
only a ServiceAccount when .Values.k8sRBAC is true, but the corresponding
ClusterRole/ClusterRoleBinding from holmesgpt-service-account.yaml are skipped
when k8sRBAC=true, leaving the holmes service account without permissions; fix
by adding ClusterRole and ClusterRoleBinding resources that are rendered when
.Values.k8sRBAC is true (or by changing holmesgpt-service-account.yaml
conditionals so its ClusterRole/ClusterRoleBinding are also emitted for
k8sRBAC=true), ensure the ClusterRole rules match the existing operator rules
you intend to grant, and set the ClusterRoleBinding subject to the
ServiceAccount using include "holmes.serviceAccountName" and .Release.Namespace
so the holmes GPT SA gets the intended cluster permissions.
In `@helm/holmes/templates/mcp-servers/kubernetes/_helpers.tpl`:
- Around line 21-35: Add a new explicit guideline to the checklist in
helm/holmes/templates/mcp-servers/kubernetes/_helpers.tpl (the block ending with
"{{- end -}}") that mandates when any tool/command/API call fails the error must
include: the exact executed command or query, the namespace and
time-range/filters/parameters used, and the full underlying API error response;
update the checklist near items "Check events"/"Inspect pods"/"Examine
resources" so all diagnostic steps reference this requirement and ensure the
guidance text is clear and unambiguous.
In `@helm/holmes/templates/mcp-servers/kubernetes/deployment.yaml`:
- Around line 122-126: The secret volume mounts for the kubeconfig and
configSecret are not mapping secret keys to the expected filenames, so if a user
sets non-default secretKey values the mounted filenames will be wrong; update
the secret volume definitions referenced by
.Values.mcpAddons.kubernetes.config.kubeconfig.secretName and
.Values.mcpAddons.kubernetes.config.configSecret.secretName to include an items:
mapping that maps each secretKey (e.g.
.Values.mcpAddons.kubernetes.config.kubeconfig.secretKey and
.Values.mcpAddons.kubernetes.config.configSecret.secretKey) to the expected
filenames (kubeconfig and config.toml respectively) so the container sees
/etc/kubernetes/kubeconfig and the expected config file name; make the same
change for the second secret block mentioned around lines 154-165.
In `@helm/holmes/templates/mcp-servers/kubernetes/networkpolicy.yaml`:
- Around line 23-27: The ingress podSelector is too broad and will fail if you
only change the NetworkPolicy; update the Holmes pod template in
helm/holmes/templates/holmes.yaml to add the release-scoped label key
"app.kubernetes.io/instance": {{ .Release.Name }} (or equivalent Helm tpl) to
the pod metadata/labels, and then update
helm/holmes/templates/mcp-servers/kubernetes/networkpolicy.yaml to replace the
current from.podSelector.matchLabels: { app: holmes } with a selector that
matches the same release-scoped label (app.kubernetes.io/instance: {{
.Release.Name }}), ensuring both the Deployment/Pod template and the
NetworkPolicy use the identical label key/value so connectivity is preserved.
🪄 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: 500b916b-8f21-4627-86bf-e7a5b52c324d
📒 Files selected for processing (7)
helm/holmes/templates/holmesgpt-rbac-service-account.yamlhelm/holmes/templates/holmesgpt-service-account.yamlhelm/holmes/templates/mcp-servers/kubernetes/_helpers.tplhelm/holmes/templates/mcp-servers/kubernetes/deployment.yamlhelm/holmes/templates/mcp-servers/kubernetes/networkpolicy.yamlhelm/holmes/templates/toolset-config.yamlhelm/holmes/values.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
helm/holmes/values.yaml (2)
689-690: 💤 Low valueConsider defaulting
networkPolicy.enabledtotruefor the Kubernetes MCP addon.This addon is granted
view(and optionallycluster-admin) on the cluster API and exposes a service that can read/mutate cluster state. Other privileged-ish addons (aws,mariadb,github) default totruewith a "recommended" comment. Defaulting tofalsehere weakens the security posture out of the box. If there's a rendering reason it has to be off (e.g. egress to in-cluster API server is awkward to express), a comment explaining the rationale would help.🤖 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 `@helm/holmes/values.yaml` around lines 689 - 690, Default the Kubernetes MCP addon's networkPolicy to enabled by setting networkPolicy.enabled: true (or, if enabling breaks chart rendering, add an explanatory comment next to the networkPolicy block describing the rendering/egress limitation and why it must remain false). Update the values entry for networkPolicy (symbol: networkPolicy.enabled) so it defaults to true like other privileged addons (aws, mariadb, github), or include the rationale comment explaining why it remains false.
779-788: 💤 Low valueTrailing blank lines and trailing whitespace at end of file.
Lines 779-788 add 9 blank lines plus trailing whitespace on line 788. Likely an editor artifact; trim to a single trailing newline.
🤖 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 `@helm/holmes/values.yaml` around lines 779 - 788, Remove the extra blank lines and trailing whitespace at the end of values.yaml (the EOF region shown in the diff); trim all trailing spaces on the final line, collapse the multiple blank lines to a single newline, and ensure the file ends with exactly one trailing newline character.
🤖 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/values.yaml`:
- Around line 638-640: The values field kubeconfig.secretKey is declared but not
used in the deployment template; update the Kubernetes deployment template
(templates/mcp-servers/kubernetes/deployment.yaml) to read the key from
.Values.mcpAddons.kubernetes.config.kubeconfig.secretKey instead of the
hardcoded "kubeconfig" so the projected/secret volume uses the user-provided
secret key (ensure you preserve the existing default behavior when the value is
empty or unset), and if you prefer not to support a configurable key remove
kubeconfig.secretKey from values.yaml and any docs to avoid a misleading option.
- Around line 666-668: The values.yaml contains an unused field
configSecret.secretKey while the deployment template
(helm/holmes/templates/mcp-servers/kubernetes/deployment.yaml) mounts the whole
secret without an items selector; either remove configSecret.secretKey from
values.yaml and update docs to require the secret key be named config.toml, or
modify the deployment template to add an items block on the secret volume that
references .Values.mcpAddons.kubernetes.config.configSecret.secretKey so the
secret is mounted per-key; update any gating logic that checks $useConfigFile to
continue relying on configSecret.secretName only.
---
Nitpick comments:
In `@helm/holmes/values.yaml`:
- Around line 689-690: Default the Kubernetes MCP addon's networkPolicy to
enabled by setting networkPolicy.enabled: true (or, if enabling breaks chart
rendering, add an explanatory comment next to the networkPolicy block describing
the rendering/egress limitation and why it must remain false). Update the values
entry for networkPolicy (symbol: networkPolicy.enabled) so it defaults to true
like other privileged addons (aws, mariadb, github), or include the rationale
comment explaining why it remains false.
- Around line 779-788: Remove the extra blank lines and trailing whitespace at
the end of values.yaml (the EOF region shown in the diff); trim all trailing
spaces on the final line, collapse the multiple blank lines to a single newline,
and ensure the file ends with exactly one trailing newline character.
🪄 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: 05970eb5-7f52-4ea1-b11b-f5af0abefb13
📒 Files selected for processing (1)
helm/holmes/values.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (3)
helm/holmes/templates/mcp-servers/kubernetes/deployment.yaml (3)
78-78: 💤 Low valueConfirm pod behavior when
serviceAccount.create=falseand a different SA is used.The pod always sets
serviceAccountNamefrommcpAddons.kubernetes.serviceAccount.nameregardless ofserviceAccount.create. That correctly supports "bring your own SA", but make sure the docs invalues.yaml(around lines 612-621) explicitly state that disablingcreaterequires the user to provision an SA whose name matchesserviceAccount.name, otherwise the pod will land on a non-existent SA and silently fail RBAC checks at runtime.🤖 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 `@helm/holmes/templates/mcp-servers/kubernetes/deployment.yaml` at line 78, Deployment always sets serviceAccountName from mcpAddons.kubernetes.serviceAccount.name regardless of serviceAccount.create, so update the values.yaml documentation near the serviceAccount block to explicitly state that when serviceAccount.create=false the operator/user must provision a ServiceAccount with the exact name specified in mcpAddons.kubernetes.serviceAccount.name; mention that otherwise pods will reference a non-existent SA and silently fail RBAC checks at runtime, and optionally suggest adding a pre-install validation or helm note to remind users to create the SA beforehand.
17-29: 💤 Low valueEmpty
ConfigMapis unused — clarify intent or remove.The
{{ .Release.Name }}-k8s-mcp-configConfigMap is created withdata: {}and never referenced anywhere in this template (no volumeMount, no envFrom). It just produces a stray resource on every install. Either drop it, or add a comment documenting the future intent (e.g., "reserved for user-supplied non-secret config") so reviewers and operators understand why it exists.♻️ Proposed removal
-apiVersion: v1 -kind: ConfigMap -metadata: - name: {{ .Release.Name }}-k8s-mcp-config - namespace: {{ .Release.Namespace }} - labels: - app: {{ .Release.Name }}-k8s-mcp - app.kubernetes.io/name: k8s-mcp-server - app.kubernetes.io/instance: {{ .Release.Name }} - app.kubernetes.io/component: mcp-server - app.kubernetes.io/part-of: holmes -data: {} -{{- if $serverConfig }} ---- +{{- if $serverConfig }} +--- apiVersion: v1 kind: Secret🤖 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 `@helm/holmes/templates/mcp-servers/kubernetes/deployment.yaml` around lines 17 - 29, The ConfigMap resource named {{ .Release.Name }}-k8s-mcp-config is created with empty data and never referenced; either remove the entire ConfigMap block or, if intended as a placeholder, add a clear comment above the resource (e.g., "reserved for user-supplied non-secret config") and include at least one explicit use (volume/envFrom) or a README note; update the template by deleting the ConfigMap stanza or adding the explanatory comment and a TODO reference so reviewers/operators understand its purpose.
130-139: 💤 Low valueUse
httpGetprobe pointing to the/healthendpoint instead of TCP.The
kubernetes-mcp-serverexposes an HTTP health endpoint atGET /health, making it ideal for readiness and liveness probes. This will catch protocol-level failures and server hangs earlier than TCP probes, which only verify the socket is open. Replace thetcpSocketblocks withhttpGetpointing tohttp://localhost:8000/health.🤖 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 `@helm/holmes/templates/mcp-servers/kubernetes/deployment.yaml` around lines 130 - 139, Replace the tcpSocket probes with HTTP GET probes: update the readinessProbe and livenessProbe blocks for kubernetes-mcp-server to use httpGet on port 8000 and path /health (e.g., readinessProbe.httpGet.path = "/health", readinessProbe.httpGet.port = 8000 and same for livenessProbe) and keep or adjust initialDelaySeconds and periodSeconds as appropriate; ensure the probes target localhost (default host) so the server's HTTP /health endpoint is used instead of plain TCP.
🤖 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 `@helm/holmes/templates/mcp-servers/kubernetes/deployment.yaml`:
- Line 78: Deployment always sets serviceAccountName from
mcpAddons.kubernetes.serviceAccount.name regardless of serviceAccount.create, so
update the values.yaml documentation near the serviceAccount block to explicitly
state that when serviceAccount.create=false the operator/user must provision a
ServiceAccount with the exact name specified in
mcpAddons.kubernetes.serviceAccount.name; mention that otherwise pods will
reference a non-existent SA and silently fail RBAC checks at runtime, and
optionally suggest adding a pre-install validation or helm note to remind users
to create the SA beforehand.
- Around line 17-29: The ConfigMap resource named {{ .Release.Name
}}-k8s-mcp-config is created with empty data and never referenced; either remove
the entire ConfigMap block or, if intended as a placeholder, add a clear comment
above the resource (e.g., "reserved for user-supplied non-secret config") and
include at least one explicit use (volume/envFrom) or a README note; update the
template by deleting the ConfigMap stanza or adding the explanatory comment and
a TODO reference so reviewers/operators understand its purpose.
- Around line 130-139: Replace the tcpSocket probes with HTTP GET probes: update
the readinessProbe and livenessProbe blocks for kubernetes-mcp-server to use
httpGet on port 8000 and path /health (e.g., readinessProbe.httpGet.path =
"/health", readinessProbe.httpGet.port = 8000 and same for livenessProbe) and
keep or adjust initialDelaySeconds and periodSeconds as appropriate; ensure the
probes target localhost (default host) so the server's HTTP /health endpoint is
used instead of plain TCP.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab09378e-188a-40bc-bcea-5b31f94cc1e0
📒 Files selected for processing (2)
helm/holmes/templates/mcp-servers/kubernetes/deployment.yamlhelm/holmes/values.yaml
2 participants
Summary by CodeRabbit