fix(common): harden legacy configmap.* passthrough for productized env helpers (SD + streaming + MT) - #1791
Conversation
…ngress The service-discovery helper only emitted SD_EXTERNAL_ADDRESS/SD_EXTERNAL_PORT when an Ingress host could be derived, silently dropping an operator-supplied external endpoint on on-prem installs with ingress.enabled=false. This broke the documented configmap.SD_* -> global -> default precedence for the external view. Emit the external endpoint when it can be derived from an Ingress host OR when the legacy configmap.SD_EXTERNAL_ADDRESS / SD_EXTERNAL_PORT keys are present, independent of ingressHost. Internal-only / consumer-only instances (no host, no legacy keys) still omit the block, so no-Ingress internal renders are unchanged.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe shared Helm helpers now use presence-based legacy configuration overrides. Service discovery supports Ingress-derived or explicitly configured external endpoints and tuning values. Multi-tenant, streaming, and datastore helpers retain explicit values and support component-specific inputs. ChangesShared configuration precedence
Possibly related PRs
✨ Finishing Touches✨ Simplify code
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
🤖 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 `@charts/lerian-common/templates/_service_discovery.tpl`:
- Around line 105-109: Update the SD_EXTERNAL_PORT documentation comment in
values.yaml to state that the external endpoint may also be emitted when legacy
configmap.SD_EXTERNAL_ADDRESS or configmap.SD_EXTERNAL_PORT is configured, even
without an Ingress host. Keep the documented precedence and
internal-only/consumer-only exclusions consistent with the helper condition in
the service-discovery template.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 49804baa-d546-407c-ad11-b962a4b84b83
📒 Files selected for processing (2)
charts/lerian-common/README.mdcharts/lerian-common/templates/_service_discovery.tpl
guimoreirar
changed the title
The productized serviceDiscovery.env helper emitted only its known SD_* keys, silently dropping six advanced tuning knobs that main exposed as flat configmap.SD_* passthrough: SD_DIAL_TIMEOUT, SD_TLS_HANDSHAKE_TIMEOUT, SD_RESPONSE_HEADER_TIMEOUT, SD_SEED_TIMEOUT, SD_WATCH_WAIT_TIME, SD_ALLOW_STALE. An operator who tuned these via configmap would lose them on upgrade. Emit each from configmap when present (hasKey-gated), so they survive backward -compat while the block stays clean when unset.
guimoreirar
changed the title
|
Broadened scope after a full render-parity audit of #1741 (main vs PR, all 213 configmap passthrough keys under identical inputs). Second gap found — same class as the external-endpoint one: the grouped helper also dropped six advanced SD tuning knobs that main exposed as flat Fixed in Audit result: of 213 passthrough keys (163 ledger + 50 crm), the PR dropped only these 8 SD keys (2 external + 6 tuning) — all now covered here. Every other key + all shared values render identically. |
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 `@charts/lerian-common/README.md`:
- Around line 19-23: The upgrade guide’s legacy external endpoint section still
documents the old Ingress-only behavior. Update the relevant section of
UPGRADE-1.0.md to describe the configmap.SD_EXTERNAL_* fallback, including its
precedence relative to global values and defaults, and remove the outdated
SD_EXTERNAL_PORT-only wording.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 8fb2adcc-6d61-4dad-8a11-3893f53539de
📒 Files selected for processing (2)
charts/lerian-common/README.mdcharts/lerian-common/templates/_service_discovery.tpl
Apply the same presence-based (hasKey) resolution that #1779 gave serviceDiscovery to streaming.env and multiTenant.env: - streaming.env activation now also triggers from legacy configmap.STREAMING_BROKERS (not only global.streaming.brokers) — mirrors the SD gate fix. - Per-key resolution uses hasKey instead of sprig default for every global/param -derived key, so an explicit configmap value survives even as a YAML false / 0 / empty string. Previously e.g. configmap STREAMING_TLS_ENABLED: false was silently overridden to the global true; MULTI_TENANT_REDIS_PORT: 0 became 6379. - Literal-default keys are left untouched (byte-identical to main). Backward-compat verified: with no configmap the derived/global values render unchanged.
guimoreirar
changed the title
|
Extended once more to close the last gap from the full parity audit — folding the streaming/multiTenant hardening here so a single lib bump → single midaz re-pin covers all backward-compat passthrough fixes (was the out-of-scope follow-up noted on #1779). Gap: truthiness footgun in
Also fixed a Blocker-1-class activation gate in Scope discipline: only global/param-derived keys switched to This PR now covers, per the audit of #1741 (main vs PR, all 213 passthrough keys):
Only 8 keys diverged in the whole audit; all closed here. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@charts/lerian-common/templates/_multi_tenant.tpl`:
- Around line 58-65: Update the MULTI_TENANT_URL resolution in the template, and
apply the same change to MULTI_TENANT_REDIS_HOST: apply the empty-string
fallback to the global value before the hasKey-based component override, then
quote the resolved value directly without another default. Preserve explicit
component values such as false, 0, or "" through the override path, while
keeping the requiredUrl validation unchanged.
In `@charts/lerian-common/templates/_streaming.tpl`:
- Around line 59-60: Align the SASL mechanism input used by the streaming.secret
helper with the precedence already implemented for $saslMechanism in
_streaming.tpl: use configmap.STREAMING_SASL_MECHANISM when present, otherwise
fall back to the global mechanism value. Update the helper inputs consistently
before adding any executable consumer.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: aab8bbfe-03d9-4c69-887b-359b0b3cdca2
📒 Files selected for processing (2)
charts/lerian-common/templates/_multi_tenant.tplcharts/lerian-common/templates/_streaming.tpl
…ithic charts The dedicated tier read only .context.Values.datastores, which resolves to the per-product block ONLY in a subchart. In a monolithic parent chart (midaz), the per-component masks live at <component>.datastores (ledger.datastores / crm.datastores) and were never consulted — the documented DEDICATED API was inert. Accept an optional dedicated map param; when passed it is the dedicated source, else fall back to .context.Values.datastores (subchart mode, unchanged). Precedence stays configmap > dedicated > global > default.
- multiTenant.env: MULTI_TENANT_URL / MULTI_TENANT_REDIS_HOST applied the global fallback via a trailing 'default ""' AFTER the hasKey override, which re-collapsed an explicit component false/0 back to empty — defeating the presence-based passthrough. Move the fallback onto the global value before the override and quote the resolved value directly. - Update the values.yaml + UPGRADE-1.0 notes: SD_EXTERNAL_* is emitted with an Ingress host OR when legacy configmap.SD_EXTERNAL_* is set (on-prem).
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 `@charts/lerian-common/docs/UPGRADE-1.0.md`:
- Line 66: Do not edit the generated UPGRADE-1.0.md file directly; update the
shared migration template that produces the SD_EXTERNAL_PORT documentation, then
run the release generation process to regenerate this file and keep the
generated upgrade guide synchronized.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a4be0112-b959-4b28-9dc5-ebc5d4332ca9
📒 Files selected for processing (3)
charts/lerian-common/docs/UPGRADE-1.0.mdcharts/lerian-common/templates/_multi_tenant.tplcharts/lerian-common/values.yaml
1 participant
Follow-up to #1779, addressing the RED blocker raised on #1741.
Problem
The
serviceDiscovery.envhelper emittedSD_EXTERNAL_ADDRESS/SD_EXTERNAL_PORTonly when an Ingress host could be derived ({{- if .ingressHost }}). On an on-prem install withingress.enabled=false, an operator who supplied the external endpoint explicitly via the legacyconfigmap.SD_EXTERNAL_ADDRESS/SD_EXTERNAL_PORTkeys had those values silently dropped — breaking the documentedconfigmap.SD_*→global.*→ default precedence for the external view.Repro (SD enabled, explicit address + external endpoint in configmap, no Ingress):
Fix
configmap.SD_EXTERNAL_ADDRESS/SD_EXTERNAL_PORTkeys are present, independent ofingressHost.$externalAddrno longer defaults tohttps://(empty ingressHost) — it stays empty unless an Ingress host or an explicit configmap value provides it.Validation
Rendered the helper across three scenarios (probe chart including the patched lib):
SD_EXTERNAL_*configmap.SD_EXTERNAL_*sethttps://ledger.onprem.local:8443)https://<host>:443) — unchangedhelm lintpasses.The consuming render test (on-prem-no-Ingress case in the midaz render-values) lands with the re-pin in #1741 — the helper is a library and is exercised through its consumers.