fix(helm): expand CLICKHOUSE_PASSWORD in webapp CLICKHOUSE_URL via kubelet#3449
Draft
ThullyoCunha wants to merge 1 commit intotriggerdotdev:mainfrom
Draft
Conversation
…belet
When clickhouse.external.existingSecret is set, the chart rendered the
CLICKHOUSE_URL env var with a literal shell-style ${CLICKHOUSE_PASSWORD}
placeholder, expecting bash to expand it at container start. But
docker/scripts/entrypoint.sh hands the value straight to goose with a
single-pass sh expansion (export GOOSE_DBSTRING="$CLICKHOUSE_URL"), so
the inner ${...} reaches goose as literal text and breaks the
ClickHouse migration:
goose run: parse "http://default:${CLICKHOUSE_PASSWORD}@host:8123?secure=false":
net/url: invalid userinfo
Switch to Kubernetes' $(VAR) syntax in both clickhouse URL helpers.
Kubelet substitutes $(CLICKHOUSE_PASSWORD) at container-creation time
from the CLICKHOUSE_PASSWORD env var the chart already sets just before
CLICKHOUSE_URL, so the URL arrives at the entrypoint with the real
password already inlined — no entrypoint change needed, works for any
container image / shell.
The plain-password branch (no existingSecret) is unchanged.
Operator caveat: CLICKHOUSE_PASSWORD must be URL-userinfo-safe because
kubelet substitutes verbatim without percent-encoding. Hex-encoded
passwords (e.g. openssl rand -hex 32) are safe by construction.
|
Contributor
WalkthroughThis change updates the Helm template helpers file to clarify and correct ClickHouse credential handling when using existing Kubernetes Secrets. The documentation is expanded to specify that passwords should use Kubernetes env-var placeholder syntax Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hosting/k8s/helm/templates/_helpers.tpl (1)
403-415: Consider surfacing the password URL-safety caveat invalues.yamlcomments too.The new helper comments are accurate, but operators are more likely to see
values.yamlguidance than_helpers.tpl. Mirroring this caveat there would reduce misconfiguration risk.Based on learnings: In the Trigger.dev v4 Helm chart, the user prefers to rely on documentation and examples in values files rather than implementing defensive coding in templates, particularly for features like extraManifests where proper usage is documented.
Also applies to: 436-438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hosting/k8s/helm/templates/_helpers.tpl` around lines 403 - 415, Add the same URL-safety caveat currently in _helpers.tpl into values.yaml comments: mention that CLICKHOUSE_PASSWORD is expanded using Kubernetes $(VAR) syntax (not shell ${VAR}), that CLICKHOUSE_URL is built from CLICKHOUSE_PASSWORD and passed into GOOSE_DBSTRING via docker/scripts/entrypoint.sh, and that the password must be URL-userinfo-safe (avoid @ : / ? # [ ] % or use hex/percent-encoding) so the generated ClickHouse/Goose DB URL parses correctly; place this comment near the CLICKHOUSE/connection-related keys and any external+existingSecret examples so operators see it in values.yaml.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hosting/k8s/helm/templates/_helpers.tpl`:
- Around line 403-415: Add the same URL-safety caveat currently in _helpers.tpl
into values.yaml comments: mention that CLICKHOUSE_PASSWORD is expanded using
Kubernetes $(VAR) syntax (not shell ${VAR}), that CLICKHOUSE_URL is built from
CLICKHOUSE_PASSWORD and passed into GOOSE_DBSTRING via
docker/scripts/entrypoint.sh, and that the password must be URL-userinfo-safe
(avoid @ : / ? # [ ] % or use hex/percent-encoding) so the generated
ClickHouse/Goose DB URL parses correctly; place this comment near the
CLICKHOUSE/connection-related keys and any external+existingSecret examples so
operators see it in values.yaml.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 30e2cee5-aa87-48d9-ae54-a398d89dd880
📒 Files selected for processing (1)
hosting/k8s/helm/templates/_helpers.tpl
📜 Review details
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2195
File: hosting/k8s/helm/values-production-example.yaml:95-102
Timestamp: 2025-06-25T14:14:11.965Z
Learning: In the Trigger.dev Helm chart production examples, the maintainer prefers to include initial/bootstrap credentials with clear warnings that they should be changed after first login, rather than requiring external secret references that could complicate initial setup. This follows their pattern of providing working examples with explicit security guidance.
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2155
File: hosting/docker/.env.example:4-7
Timestamp: 2025-06-06T23:55:01.933Z
Learning: In the trigger.dev project, .env.example files should contain actual example secret values rather than placeholders, as these help users understand the expected format. The files include clear warnings about not using these defaults in production and instructions for generating proper secrets.
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2155
File: hosting/docker/registry/auth.htpasswd:1-1
Timestamp: 2025-06-06T18:49:18.144Z
Learning: Example credentials in self-hosting documentation files are acceptable when they serve as templates or examples for users to understand the setup process, particularly in hosting/docker configuration files.
📚 Learning: 2025-06-25T14:14:11.965Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2195
File: hosting/k8s/helm/values-production-example.yaml:95-102
Timestamp: 2025-06-25T14:14:11.965Z
Learning: In the Trigger.dev Helm chart production examples, the maintainer prefers to include initial/bootstrap credentials with clear warnings that they should be changed after first login, rather than requiring external secret references that could complicate initial setup. This follows their pattern of providing working examples with explicit security guidance.
Applied to files:
hosting/k8s/helm/templates/_helpers.tpl
📚 Learning: 2025-06-25T13:18:04.827Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2195
File: hosting/k8s/helm/templates/extra-manifests.yaml:1-4
Timestamp: 2025-06-25T13:18:04.827Z
Learning: In the Trigger.dev v4 Helm chart, the user prefers to rely on documentation and examples in values files rather than implementing defensive coding in templates, particularly for features like extraManifests where proper usage is documented.
Applied to files:
hosting/k8s/helm/templates/_helpers.tpl
📚 Learning: 2025-06-06T18:49:18.144Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2155
File: hosting/docker/registry/auth.htpasswd:1-1
Timestamp: 2025-06-06T18:49:18.144Z
Learning: Example credentials in self-hosting documentation files are acceptable when they serve as templates or examples for users to understand the setup process, particularly in hosting/docker configuration files.
Applied to files:
hosting/k8s/helm/templates/_helpers.tpl
📚 Learning: 2025-06-25T13:20:17.174Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2195
File: hosting/k8s/helm/values.yaml:22-51
Timestamp: 2025-06-25T13:20:17.174Z
Learning: In the Trigger.dev Helm chart values.yaml, the maintainer prefers to use explicit comprehensive warnings for security-sensitive default values rather than implementing secure-by-default behavior that would fail installation. The project uses deterministic default secrets with clear "TESTING ONLY" warnings and instructions for production deployment.
Applied to files:
hosting/k8s/helm/templates/_helpers.tpl
📚 Learning: 2026-04-23T08:44:10.511Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3429
File: hosting/k8s/helm/values.yaml:211-218
Timestamp: 2026-04-23T08:44:10.511Z
Learning: In the Trigger.dev Helm chart helper templates, for both `trigger-v4.webappServiceAccountName` and `trigger-v4.supervisorServiceAccountName`, if `serviceAccount.create` is `false` and `serviceAccount.name` is empty, use Helm `fail` to stop rendering (raise a template error) rather than falling back to the namespace default service account or depending on documentation warnings. This prevents silently producing an invalid/undesired ServiceAccount configuration.
Applied to files:
hosting/k8s/helm/templates/_helpers.tpl
🔇 Additional comments (1)
hosting/k8s/helm/templates/_helpers.tpl (1)
426-426: Good fix: kubelet-style substitution is correctly applied in both ClickHouse URL helpers.Switching to
$(CLICKHOUSE_PASSWORD)in both external+existingSecret branches addresses the crash-loop root cause while keeping the plain-password branch behavior intact.Also applies to: 446-446
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
1 participant
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
When the official Helm chart is deployed with an external ClickHouse and
clickhouse.external.existingSecretset — the documented path for not committing secrets tovalues.yaml— the webapp pod crash-loops on startup:Context in vouch request #3443. Re-opening in draft status per bot policy (previous attempt was #3445, closed by automation because it wasn't draft; no changes to the patch).
Root cause
Two pieces interact:
hosting/k8s/helm/templates/_helpers.tplrendersCLICKHOUSE_URL(andRUN_REPLICATION_CLICKHOUSE_URL) with a shell-style literal${CLICKHOUSE_PASSWORD}expecting bash expansion at container start.docker/scripts/entrypoint.shdoesexport GOOSE_DBSTRING="$CLICKHOUSE_URL"— single-pass POSIX sh substitution, so the inner${...}survives as literal text and goose rejects it.Reproduces against the latest published chart (
oci://ghcr.io/triggerdotdev/charts/trigger:4.0.5) andmain.Fix
Switch the two helpers (external +
existingSecretbranch) from shell-style${CLICKHOUSE_PASSWORD}to Kubernetes'$(CLICKHOUSE_PASSWORD). Kubelet substitutes$(VAR)at pod-creation time from earlier env entries, and the chart already declaresCLICKHOUSE_PASSWORDfrom the Secret immediately beforeCLICKHOUSE_URL, so the URL reaches the entrypoint with the real password already inlined. No entrypoint change, no image change. The plain-password branch (noexistingSecret) is unchanged.Operator caveat added as template comments:
CLICKHOUSE_PASSWORDmust be URL-userinfo-safe since kubelet substitutes verbatim without percent-encoding. Hex-encoded passwords (e.g.openssl rand -hex 32) are safe by construction.Verification
helm templateagainstexternal.existingSecretnow rendersvalue: "http://default:$(CLICKHOUSE_PASSWORD)@<host>:8123?secure=false"(was${CLICKHOUSE_PASSWORD}).helm templateagainst the plain-password branch is byte-identical to before.goose: successfully migrated database to version: 6, Node.js ClickHouse client connects at runtime.Alternatives considered
entrypoint.shtoeval/envsubstthe URL — larger surface, touches every deployment mode (Docker Compose + k8s) and every container image.valueFrom.secretKeyRef, as intrigger-v4.postgres.useSecretUrl) — cleaner long-term but requires a newvalues.yamlfield and a migration path for existing users. Happy to follow up with that as a separate PR if the minimal fix here isn't the preferred direction.Changeset
None added — the Helm chart isn't versioned through
@changesets/cli(docs/chart-only PRs historically merge without a changeset, e.g. #2671). Happy to add one if the policy changed.Closes #3443.