#6181 fixing host references when using gatway api#6255
Conversation
eamonnmoloney
left a comment
There was a problem hiding this comment.
crev review
This PR correctly adds gateway-API awareness to camundaPlatform.orchestrationExternalURL in chart 8.9, but has two issues: (1) the new gateway branch silently produces a malformed URL (https:///path) when global.host is not set (its default is ""), with no required guard or fallback — all downstream service URLs (operate, tasklist, identity) are affected; (2) the identical bug exists in chart 8.10 where the same helper still returns http://localhost:8080 for gateway users, violating the repo's policy against single-version fixes when the same fix is clearly needed across versions.
Specialists run: correctness, devils-advocate, test-adequacy, api-stability. Devil's-advocate hypotheses: 8 raised, 2 promoted.
Hypotheses by stance: adversarial-input=2 author-blind-spot=2 missing-case=3 scope-discipline=1 · by disposition: dropped_low_severity=2 dropped_ungroundable=4 promoted=2
Escalation: Human review required (score: 0.62, threshold: 0.50). Hard escalation rule triggered: the fix is applied only to chart version 8.9 while the same defect demonstrably exists in 8.10 (confirmed by direct inspection of the 8.10 helper). The escalation policy NEVER rule states changes must not apply to one version when the same fix is clearly needed across multiple versions. Additionally, the fix introduces a silent failure mode (malformed URL) for the default global.host="" configuration with no required guard.
Findings on lines outside this PR's diff:
-
P1
charts/camunda-platform-8.10/templates/common/_helpers.tpl:676— Same gateway URL bug exists in 8.10 — fix only applied to 8.9
ThecamundaPlatform.orchestrationExternalURLhelper incharts/camunda-platform-8.10/templates/common/_helpers.tpl(lines 676–683) has noelse if .Values.global.gateway.enabledbranch. Users on 8.10 who enableglobal.gateway.enabled=true(withglobal.ingress.enabled=false) will receivehttp://localhost:8080for all external service URLs — the same bug this PR fixes for 8.9.Chart 8.10 has full Gateway API infrastructure: it renders HTTPRoutes, ReferenceGrants, and its tests (
test/unit/identity/httproute_test.go,test/unit/connectors/httproute_test.go,test/unit/common/gateway_test.go) all exerciseglobal.gateway.enabled=true. The URL helper is the missing piece.The escalation policy explicitly states: "NEVER approve changes that only apply to one chart version when the same fix is clearly needed across multiple versions (8.8, 8.9, 8.10)."
Note: 8.8 uses a different pattern (
tpl .Values.global.ingress.host $withoutglobal.host) so the fix shape differs there and is lower priority.Recommendation: Apply an equivalent gateway branch to
charts/camunda-platform-8.10/templates/common/_helpers.tplbefore or alongside this PR, or open a tracked follow-up issue and link it here.
| {{- printf "%s://%s%s" $proto (tpl .Values.global.host $ | default (tpl .Values.global.ingress.host $)) (include "camundaPlatform.joinpath" (list .Values.orchestration.contextPath)) -}} | ||
| {{- else if .Values.global.gateway.enabled -}} | ||
| {{ $proto := ternary "https" "http" .Values.global.gateway.tls.enabled -}} | ||
| {{- printf "%s://%s%s" $proto (tpl .Values.global.host $) (include "camundaPlatform.joinpath" (list .Values.orchestration.contextPath)) -}} |
There was a problem hiding this comment.
P1 · Gateway branch produces malformed URL when global.host is empty (default) (via correctness)
The new else if .Values.global.gateway.enabled branch at line 635 calls tpl .Values.global.host $ with no fallback. global.host defaults to "" in values.yaml (line 120). When a user enables global.gateway.enabled=true without setting global.host, Helm renders:
https:///orchestration
This is a malformed URL that silently propagates to every derived service URL — orchestrationOperateExternalURL (/operate), orchestrationTasklistExternalURL (/tasklist), orchestrationIdentityExternalURL (/identity) — all of which call trimSuffix "/" on this base URL and then append their path. The result is broken OAuth redirect URIs, broken inter-service URLs, and broken Identity application registrations, all without any error during helm install.
By contrast, the ingress branch (line 632) has | default (tpl .Values.global.ingress.host $) as a fallback. The gateway branch provides no equivalent safety net.
Recommendation: Add a required guard:
{{- printf "%s://%s%s" $proto (tpl .Values.global.host $ | required "global.host must be set when global.gateway.enabled is true") (include "camundaPlatform.joinpath" (list .Values.orchestration.contextPath)) -}}
This ensures a meaningful error at render time rather than a silently broken deployment.
{{- printf "%s://%s%s" $proto (tpl .Values.global.host $) (include "camundaPlatform.joinpath" (list .Values.orchestration.contextPath)) -}}
eamonnmoloney
left a comment
There was a problem hiding this comment.
please see comments
|
@eamonnmoloney applied both the |
Which problem does the PR fix?
#6181
What's in this PR?
i've added functionality to handle the gatway api config into
camundaPlatform.orchestrationExternalURL.this functionality only uses
global.hostas host provider. The ingress implementation usesglobal.ingress.hostas fallback.i did not introduce a
global.gateway.hostvalues because theglobal.ingress.hostvalue is already deprecated so introducing a gateway equivalent seemed wrong.Checklist
Before opening the PR:
make go.update-golden-only.After opening the PR: