fix(weather-demo): teach pick_log_container the proxy-sidecar container name#420
Conversation
…er name The `pick_log_container` helper in deploy_and_verify_advanced.sh only knew two AuthBridge sidecar container names: `envoy-proxy` (envoy-sidecar mode) and `authbridge` (the pre-split combined-mode name from before rossoctl#411). After rossoctl#411 + kagenti-operator#361 the cluster default became proxy-sidecar, whose container is named `authbridge-proxy` — the script `die()`s on every CI run because neither of its two known names matches. Reproduced on kagenti#1592 just now. The pod actually had two containers — `mcp` and `authbridge-proxy` — i.e. the webhook had injected fine and the demo flow was healthy. The script just couldn't tell which container to read logs from: ERROR: pod weather-tool-advanced-... has neither envoy-proxy nor authbridge container (mcp authbridge-proxy) (The error message itself was misleading — the pod's container list `mcp\nauthbridge-proxy` got split across two log lines, so casual readers saw "(mcp" and assumed no sidecar was injected.) Fix: * Replace the two-branch if-elif with a small fallthrough loop over `authbridge-proxy / envoy-proxy / authbridge` so the cluster default's container name is recognized first, the envoy-sidecar mode's name still works, and the legacy combined name is kept for older clusters / replays. * Wrap `$names` in `$(echo $names)` inside the die message so the pod's container list renders on a single line, making the diagnostic accurate ("expected one of …; got: mcp authbridge-proxy") rather than visually truncated. Verified: bash -n on the script; repo grep finds no other places with the same restricted-name pick. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
226cc9a to
543e801
Compare
pdettori
left a comment
There was a problem hiding this comment.
Clean, minimal fix that updates pick_log_container() to recognize the post-#361 proxy-sidecar container name (authbridge-proxy). The fallthrough loop is cleaner than the old if-elif, the inline comments citing operator constants are helpful, and the $(echo $names) fix for single-line error output is a nice touch. One nit: the commit message body is stale — it describes a three-name loop that was trimmed to two before push.
Areas reviewed: Shell, PR format, commit conventions, security
Commits: 1 commit, signed-off: yes
CI status: all 17 checks passing
| # removed in #361 — and this script already requires the post-#361 | ||
| # AgentRuntime CRD just above, so there's no pre-#361 path to | ||
| # support here. | ||
| for c in authbridge-proxy envoy-proxy; do |
There was a problem hiding this comment.
nit: The commit message body says the loop covers three names (authbridge-proxy / envoy-proxy / authbridge) and that "the legacy combined name is kept for older clusters / replays" — but the code only loops over two (authbridge-proxy envoy-proxy), matching the PR description's deliberate exclusion of bare authbridge. The commit message appears to be from an earlier iteration. Not blocking since the code + PR description are correct, but worth a force-push to fix if convenient.
Summary
pick_log_containerindeploy_and_verify_advanced.shonly knew two AuthBridge sidecar container names —envoy-proxy(envoy-sidecar mode) andauthbridge. After #411 + kagenti-operator#361 the cluster default became proxy-sidecar with container nameauthbridge-proxy, which the script doesn't recognize, so every CI run dies on the firstpick_log_containercall.Caught on kagenti#1592's
Deploy & Testlane:The pod actually had
mcp+authbridge-proxy— the webhook injected fine and the demo flow was healthy. The script just couldn't tell which container to read logs from. The error message itself was misleading (the multi-line container list got split across two log lines, so casual readers saw "(mcp" and assumed no sidecar was injected).Fix
The two recognized names in
pick_log_containerare now the only two the operator actually produces today:authbridge-proxy— proxy-sidecar mode, the post-feat: Add MCP parser plugin and body access infrastructure #361 cluster default. FromAuthBridgeProxyContainerNameinkagenti-operator/internal/webhook/injector/constants.go:34.envoy-proxy— envoy-sidecar mode. FromEnvoyProxyContainerNameinkagenti-operator/internal/webhook/injector/container_builder.go:34.The bare
authbridgename from the pre-#361combinedSidecarfeature gate is not kept as a fallback. That gate (and itsAuthBridgeContainerName = "authbridge"constant) was removed in #361, anddeploy_and_verify_advanced.shalready requires the post-#361agentruntimes.agent.kagenti.devCRD a few lines above, so there's no pre-#361 path to keep working.Also reformat the diagnostic message — wrap
$namesin$(echo $names)so the pod's container list renders on a single line, making the error readexpected one of …; got: mcp authbridge-proxyinstead of visually truncating.Test plan
bash -non the scriptgrep -rfor similar restricted name lists across other shell / Python helpers — none foundDeploy & Testonce this lands and the e2e clone of extensions main picks up the fixAssisted-By: Claude (Anthropic AI) noreply@anthropic.com