feat: Allow helm chart to separate Kubewatch and Robusta Runner#2062
Open
hfoxy wants to merge 2 commits intorobusta-dev:masterfrom
Open
feat: Allow helm chart to separate Kubewatch and Robusta Runner#2062hfoxy wants to merge 2 commits intorobusta-dev:masterfrom
hfoxy wants to merge 2 commits intorobusta-dev:masterfrom
Conversation
|
|
WalkthroughThis pull request adds conditional rendering to Helm chart templates using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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/robusta/templates/forwarder-service-account.yaml (1)
1-1: ⚡ Quick winGuard against missing forwarder ServiceAccount when creation is disabled.
Line 1 can suppress SA creation while
helm/robusta/templates/forwarder.yamlLine 32 still defaults to{{ include "robusta.fullname" . }}-forwarder-service-account. Withkubewatch.enabled=true,kubewatch.createServiceAccount=false, and emptykubewatch.customServiceAccount, the Deployment references a non-existent SA.Proposed guard
+{{- if and .Values.kubewatch.enabled (not .Values.kubewatch.createServiceAccount) (empty .Values.kubewatch.customServiceAccount) }} +{{- fail "kubewatch.customServiceAccount must be set when kubewatch.createServiceAccount=false" }} +{{- end }} {{- if and .Values.kubewatch.enabled .Values.kubewatch.createServiceAccount }} kind: ClusterRole apiVersion: rbac.authorization.k8s.io/v1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/robusta/templates/forwarder-service-account.yaml` at line 1, The Deployment may reference a non-existent ServiceAccount because forwarder-service-account.yaml only creates the SA when .Values.kubewatch.createServiceAccount is true while forwarder.yaml unconditionally sets serviceAccountName to {{ include "robusta.fullname" . }}-forwarder-service-account; update forwarder.yaml’s serviceAccountName logic to guard: set serviceAccountName to .Values.kubewatch.customServiceAccount if provided, otherwise only set the generated "{{ include "robusta.fullname" . }}-forwarder-service-account" when .Values.kubewatch.createServiceAccount is true (or omit the field entirely when neither is true), using the same keys (.Values.kubewatch.enabled, .Values.kubewatch.createServiceAccount, .Values.kubewatch.customServiceAccount) to locate and fix the issue.helm/robusta/templates/runner-service-account.yaml (1)
1-1: ⚡ Quick winAdd a fail-fast guard for service-account configuration mismatch.
Line 1 can skip SA creation while
helm/robusta/templates/runner.yamlLine 35 still defaults the Deployment to{{ include "robusta.fullname" . }}-runner-service-account. Ifrunner.enabled=true,runner.createServiceAccount=false, andrunner.customServiceAccountis empty, the rendered Deployment points to a missing SA.Proposed guard
+{{- if and .Values.runner.enabled (not .Values.runner.createServiceAccount) (empty .Values.runner.customServiceAccount) }} +{{- fail "runner.customServiceAccount must be set when runner.createServiceAccount=false" }} +{{- end }} {{- if and .Values.runner.enabled .Values.runner.createServiceAccount }} kind: ClusterRole apiVersion: rbac.authorization.k8s.io/v1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/robusta/templates/runner-service-account.yaml` at line 1, Add a fail-fast guard that errors during chart rendering when runner.enabled is true, runner.createServiceAccount is false, and runner.customServiceAccount is empty so the Deployment in templates/runner.yaml won't reference a missing SA; implement this by adding a conditional check using .Values.runner.enabled, .Values.runner.createServiceAccount and .Values.runner.customServiceAccount (e.g. in templates/runner-service-account.yaml or at the top of templates/runner.yaml) and call Helm's fail with a clear message when the condition is met.
🤖 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/robusta/templates/kubewatch-configmap.yaml`:
- Line 11: The current default in kubewatch-configmap.yaml unconditionally falls
back to "http://{{ include "robusta.fullname" . }}-runner:80/api/handle", which
silently routes to a non-existent service when .Values.runner.enabled is false;
change the url logic to fail-fast or require an explicit override: update the
url template (the line using include "robusta.fullname" and
.Values.kubewatch.overrideUrl) to check .Values.runner.enabled and either (a)
use the runner URL only when .Values.runner.enabled is true, or (b) call
required("kubewatch.overrideUrl must be set when runner.enabled=false",
.Values.kubewatch.overrideUrl) when runner.enabled is false so Helm template
rendering fails unless an explicit override is provided; reference the include
"robusta.fullname", .Values.kubewatch.overrideUrl and .Values.runner.enabled
symbols when making the change.
---
Nitpick comments:
In `@helm/robusta/templates/forwarder-service-account.yaml`:
- Line 1: The Deployment may reference a non-existent ServiceAccount because
forwarder-service-account.yaml only creates the SA when
.Values.kubewatch.createServiceAccount is true while forwarder.yaml
unconditionally sets serviceAccountName to {{ include "robusta.fullname" .
}}-forwarder-service-account; update forwarder.yaml’s serviceAccountName logic
to guard: set serviceAccountName to .Values.kubewatch.customServiceAccount if
provided, otherwise only set the generated "{{ include "robusta.fullname" .
}}-forwarder-service-account" when .Values.kubewatch.createServiceAccount is
true (or omit the field entirely when neither is true), using the same keys
(.Values.kubewatch.enabled, .Values.kubewatch.createServiceAccount,
.Values.kubewatch.customServiceAccount) to locate and fix the issue.
In `@helm/robusta/templates/runner-service-account.yaml`:
- Line 1: Add a fail-fast guard that errors during chart rendering when
runner.enabled is true, runner.createServiceAccount is false, and
runner.customServiceAccount is empty so the Deployment in templates/runner.yaml
won't reference a missing SA; implement this by adding a conditional check using
.Values.runner.enabled, .Values.runner.createServiceAccount and
.Values.runner.customServiceAccount (e.g. in
templates/runner-service-account.yaml or at the top of templates/runner.yaml)
and call Helm's fail with a clear message when the condition is met.
🪄 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: 245fa5bc-3828-4599-be34-66d6ed549617
📒 Files selected for processing (7)
helm/robusta/templates/forwarder-service-account.yamlhelm/robusta/templates/forwarder.yamlhelm/robusta/templates/kubewatch-configmap.yamlhelm/robusta/templates/playbooks-config.yamlhelm/robusta/templates/runner-service-account.yamlhelm/robusta/templates/runner.yamlhelm/robusta/values.yaml
| handler: | ||
| cloudevent: | ||
| url: "http://{{ include "robusta.fullname" . }}-runner:80/api/handle" | ||
| url: {{ default (printf "http://%s-runner:80/api/handle" (include "robusta.fullname" .)) .Values.kubewatch.overrideUrl | quote }} |
There was a problem hiding this comment.
Prevent silent misrouting when runner is disabled.
Line 11 falls back to http://<release>-runner:80/api/handle even when runner.enabled=false (see helm/robusta/templates/runner.yaml Line 1). In multi-cluster mode this can silently route to a non-existent local service unless kubewatch.overrideUrl is set.
Proposed fail-fast validation
{{- if .Values.kubewatch.enabled }}
+{{- if and (not .Values.runner.enabled) (empty .Values.kubewatch.overrideUrl) }}
+{{- fail "kubewatch.overrideUrl must be set when runner.enabled=false" }}
+{{- end }}
apiVersion: v1
kind: ConfigMap
metadata:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helm/robusta/templates/kubewatch-configmap.yaml` at line 11, The current
default in kubewatch-configmap.yaml unconditionally falls back to "http://{{
include "robusta.fullname" . }}-runner:80/api/handle", which silently routes to
a non-existent service when .Values.runner.enabled is false; change the url
logic to fail-fast or require an explicit override: update the url template (the
line using include "robusta.fullname" and .Values.kubewatch.overrideUrl) to
check .Values.runner.enabled and either (a) use the runner URL only when
.Values.runner.enabled is true, or (b) call required("kubewatch.overrideUrl must
be set when runner.enabled=false", .Values.kubewatch.overrideUrl) when
runner.enabled is false so Helm template rendering fails unless an explicit
override is provided; reference the include "robusta.fullname",
.Values.kubewatch.overrideUrl and .Values.runner.enabled symbols when making the
change.
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
Allow the Helm chart installation to be separated, providing support for a source and destination cluster. Not all features are supported in this way, for example some graphs and logs are not included in alerts.
Testing
We have been running this version of the Helm chart for a few weeks now without issue other than the above mentioned.