Properly handle secure and verify on clickhouse #756
Conversation
WalkthroughAdds DSN parameter support for ClickHouse URLs, introduces a reusable wget no-verify flag helper (duplicated definition), propagates that flag into multiple init-container wget readiness checks, adds new DSN param fields in values, and updates BusyBox/init image tags from 1.35 → 1.37. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Values as Helm values
participant TPL as _clickhouse.tpl
participant URL as ClickHouse URL/DSN
Values->>TPL: provide secure, skip_verify, dsnParams
TPL->>TPL: clickHouseDsnParams formats "secure=...&skip_verify=...&..."
TPL->>URL: build host:port?{DSN_PARAMS}
note right of URL: Used by schemamigrator, exporters, and clients
sequenceDiagram
autonumber
participant ENV as Pod env (CLICKHOUSE_VERIFY)
participant HELP as _helpers.tpl
participant INIT as Init container
participant CH as ClickHouse endpoint
ENV-->>HELP: CLICKHOUSE_VERIFY value
HELP-->>INIT: include wget.clickhouse.noVerifyFlag
INIT->>CH: wget [--no-check-certificate?] --user ... --spider -q <url>
alt reachable
INIT->>INIT: proceed
else unreachable
INIT->>INIT: retry/sleep loop
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Files/areas to check closely:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (6)
🔇 Additional comments (13)
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 |
|
fix: #747 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
charts/signoz/templates/_clickhouse.tpl (2)
96-123: ExposeCLICKHOUSE_SKIP_VERIFYin the credentials snippet as well.Workloads that rely on
snippet.clickhouse-credentialswon’t see the newCLICKHOUSE_SKIP_VERIFYflag, so they’ll keep default behavior instead of honoring the chart setting. Please propagate the same env var here to stay consistent withsnippet.clickhouse-env.- name: CLICKHOUSE_VERIFY value: {{ .Values.clickhouse.verify | quote }} +- name: CLICKHOUSE_SKIP_VERIFY + value: {{ not .Values.clickhouse.verify | quote }} {{- else -}} … - name: CLICKHOUSE_VERIFY value: {{ .Values.externalClickhouse.verify | quote }} +- name: CLICKHOUSE_SKIP_VERIFY + value: {{ not .Values.externalClickhouse.verify | quote }}
242-247: Avoid leaving a trailing&in the generated DSN.When
dsnParamsis empty this helper produces...?skip_verify=true&, leaving an empty query segment. Let’s trim the trailing ampersand so we never emit malformed parameter pairs.-{{- printf "secure=%v&skip_verify=%v&%v" ( default false .Values.clickhouse.secure ) ( not ( default false .Values.clickhouse.verify ) ) ( default "" .Values.clickhouse.dsnParams ) -}} +{{- $raw := printf "secure=%v&skip_verify=%v&%v" ( default false .Values.clickhouse.secure ) ( not ( default false .Values.clickhouse.verify ) ) ( default "" .Values.clickhouse.dsnParams ) -}} +{{- trimSuffix "&" $raw -}} {{- else -}} -{{- printf "secure=%v&skip_verify=%v&%v" ( default false .Values.externalClickhouse.secure ) ( not ( default false .Values.externalClickhouse.verify ) ) ( default "" .Values.externalClickhouse.dsnParams ) -}} +{{- $raw := printf "secure=%v&skip_verify=%v&%v" ( default false .Values.externalClickhouse.secure ) ( not ( default false .Values.externalClickhouse.verify ) ) ( default "" .Values.externalClickhouse.dsnParams ) -}} +{{- trimSuffix "&" $raw -}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
charts/clickhouse/values.yaml(1 hunks)charts/signoz/templates/_clickhouse.tpl(6 hunks)charts/signoz/templates/_helpers.tpl(1 hunks)charts/signoz/templates/otel-collector/deployment.yaml(1 hunks)charts/signoz/templates/schema-migrator/migrations-async.yaml(1 hunks)charts/signoz/templates/schema-migrator/migrations-sync.yaml(1 hunks)charts/signoz/templates/signoz/statefulset.yaml(1 hunks)charts/signoz/values.yaml(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-01T05:29:30.302Z
Learnt from: prashant-shahi
PR: SigNoz/charts#544
File: charts/signoz/templates/schema-migrator/migrations-sync.yaml:16-23
Timestamp: 2024-11-01T05:29:30.302Z
Learning: In `charts/signoz/templates/schema-migrator/migrations-sync.yaml`, the `Sync` ArgoCD hook and `pre-upgrade` Helm hook are intentionally used to maintain consistent behavior and prevent issues with Kubernetes Job cleanup.
Applied to files:
charts/signoz/templates/schema-migrator/migrations-async.yamlcharts/signoz/templates/schema-migrator/migrations-sync.yaml
🔇 Additional comments (1)
charts/clickhouse/values.yaml (1)
375-376: BusyBox tag 1.37 is available:busybox:1.37is published on Docker Hub and can be pulled successfully.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/signoz/templates/_clickhouse.tpl (1)
51-58: External env block missing CLICKHOUSE_METER_DATABASEParity with internal block is needed for components that depend on meter DB envs.
Apply this diff:
- name: CLICKHOUSE_LOG_DATABASE value: {{ default "signoz_logs" .Values.externalClickhouse.logDatabase | quote }} +- name: CLICKHOUSE_METER_DATABASE + value: {{ default "signoz_meter" .Values.externalClickhouse.meterDatabase | quote }} - name: CLICKHOUSE_USER value: {{ .Values.externalClickhouse.user | quote }}
🧹 Nitpick comments (3)
charts/signoz/templates/_clickhouse.tpl (3)
242-248: Avoid trailing ampersand in DSN paramsWhen
dsnParamsis empty, this prints a trailing&. Some parsers tolerate it, but it’s easy to avoid.Apply this diff:
{{- define "clickhouse.clickHouseDsnParams" -}} {{- if .Values.clickhouse.enabled -}} -{{- printf "secure=%v&skip_verify=%v&%v" ( default false .Values.clickhouse.secure ) ( not ( default false .Values.clickhouse.verify ) ) ( default "" .Values.clickhouse.dsnParams ) -}} +{{- $secure := default false .Values.clickhouse.secure -}} +{{- $skipVerify := not ( default false .Values.clickhouse.verify ) -}} +{{- $extra := trimAll "& " (default "" .Values.clickhouse.dsnParams) -}} +{{- if $extra -}} +{{- printf "secure=%v&skip_verify=%v&%s" $secure $skipVerify $extra -}} +{{- else -}} +{{- printf "secure=%v&skip_verify=%v" $secure $skipVerify -}} +{{- end -}} {{- else -}} -{{- printf "secure=%v&skip_verify=%v&%v" ( default false .Values.externalClickhouse.secure ) ( not ( default false .Values.externalClickhouse.verify ) ) ( default "" .Values.externalClickhouse.dsnParams ) -}} +{{- $secure := default false .Values.externalClickhouse.secure -}} +{{- $skipVerify := not ( default false .Values.externalClickhouse.verify ) -}} +{{- $extra := trimAll "& " (default "" .Values.externalClickhouse.dsnParams) -}} +{{- if $extra -}} +{{- printf "secure=%v&skip_verify=%v&%s" $secure $skipVerify $extra -}} +{{- else -}} +{{- printf "secure=%v&skip_verify=%v" $secure $skipVerify -}} +{{- end -}} {{- end -}} {{- end -}}
195-210: Always include scheme in httpUrl (use http:// when not secure)wget behaves more predictably with explicit schemes. Recommend defaulting to http:// when not secure.
Apply this diff:
{{- define "clickhouse.httpUrl" -}} {{- $httpUrl := "" -}} -{{- $httpPrefix := "" -}} +{{- $httpPrefix := "http://" -}} {{- if .Values.clickhouse.enabled }} {{- $httpUrl = printf "%s:%s" (include "clickhouse.servicename" .) (include "clickhouse.httpPort" .) }} {{- if .Values.clickhouse.secure }} {{- $httpPrefix = "https://" }} {{- end }} {{- else }} {{- $httpUrl = printf "%s:%s" (required "externalClickhouse.host is required if using external clickhouse" .Values.externalClickhouse.host) ( include "clickhouse.httpPort" .) }} {{- if .Values.externalClickhouse.secure }} {{- $httpPrefix = "https://" }} {{- end }} {{- end }} {{- printf "%s%s" $httpPrefix $httpUrl }} {{- end -}}
96-98: Optional: add CLICKHOUSE_SKIP_VERIFY to credentials snippet for parityNot strictly required here, but keeping both env snippets aligned reduces surprises.
Apply this diff:
- name: CLICKHOUSE_VERIFY value: {{ .Values.clickhouse.verify | quote }} +# Optional: parity with full env block +- name: CLICKHOUSE_SKIP_VERIFY + value: {{ not .Values.clickhouse.verify | quote }}And similarly in the external branch.
Also applies to: 121-123
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
charts/signoz/templates/_clickhouse.tpl(6 hunks)charts/signoz/templates/_helpers.tpl(1 hunks)charts/signoz/templates/otel-collector/deployment.yaml(1 hunks)charts/signoz/templates/schema-migrator/migrations-async.yaml(1 hunks)charts/signoz/templates/schema-migrator/migrations-sync.yaml(1 hunks)charts/signoz/templates/signoz/statefulset.yaml(1 hunks)charts/signoz/values.yaml(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- charts/signoz/templates/_helpers.tpl
- charts/signoz/templates/signoz/statefulset.yaml
- charts/signoz/templates/otel-collector/deployment.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-01T05:29:30.302Z
Learnt from: prashant-shahi
PR: SigNoz/charts#544
File: charts/signoz/templates/schema-migrator/migrations-sync.yaml:16-23
Timestamp: 2024-11-01T05:29:30.302Z
Learning: In `charts/signoz/templates/schema-migrator/migrations-sync.yaml`, the `Sync` ArgoCD hook and `pre-upgrade` Helm hook are intentionally used to maintain consistent behavior and prevent issues with Kubernetes Job cleanup.
Applied to files:
charts/signoz/templates/schema-migrator/migrations-sync.yamlcharts/signoz/templates/schema-migrator/migrations-async.yaml
🔇 Additional comments (9)
charts/signoz/templates/schema-migrator/migrations-sync.yaml (2)
45-45: Root scope passed correctly to wget helper — LGTMUsing
$ensures the helper can access.Values. All good here.
45-45: Confirmed single helper definition:wget.clickhouse.noVerifyFlagappears only once in charts/signoz/templates/_helpers.tpl.charts/signoz/templates/schema-migrator/migrations-async.yaml (2)
46-46: Root scope passed correctly to wget helper — LGTMSame as sync job; correct use of
$here.
46-46: Consistent usage confirmed – allwget.clickhouse.noVerifyFlagincludes pass$to the helper across templates.charts/signoz/values.yaml (3)
253-256: Expose DSN params — good additionAdds flexibility for custom ClickHouse DSN tuning.
748-751: External DSN params — good additionMatches internal DSN params; consistent API surface.
1911-1930: Fix env interpolation in ClickHouse DSNs — LGTM
${env:...}restored for secure/skip_verify across exporters. This unblocks TLS/verification behavior.charts/signoz/templates/_clickhouse.tpl (2)
5-10: Append DSN params to schema migrator URL — LGTMThe DSN assembly for schema migrator is correct for both internal and external paths.
39-41: Env var parity for verify/skip_verify — LGTMIntroducing CLICKHOUSE_SKIP_VERIFY (negation of verify) aligns with driver expectations.
Also applies to: 72-74
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary by CodeRabbit