Skip to content

fix(smoke): assert the CSP from inside the cluster, not from the runner - #876

Merged
joryirving merged 1 commit into
mainfrom
fix/smoke-assert-in-cluster
Aug 27, 2026
Merged

fix(smoke): assert the CSP from inside the cluster, not from the runner#876
joryirving merged 1 commit into
mainfrom
fix/smoke-assert-in-cluster

Conversation

@joryirving

Copy link
Copy Markdown
Contributor

Summary

  • The CSP assertion now fetches from a throwaway in-cluster pod instead of from the runner.
  • smoke-csp.mjs gains a --raw-response mode that parses curl -i output.

Why

The assertion has been failing with a connect timeout while the app was demonstrably serving. The release PR's run makes the asymmetry explicit:

14:32:07  app pod restarts (IP changes)
14:32:31  health gate: healthy (1/3)   <- fresh in-cluster pod
14:32:39  health gate: healthy (2/3)   <- fresh in-cluster pod
14:32:47  health gate: healthy (3/3)   <- fresh in-cluster pod
14:32:48  runner fetch -> ConnectTimeoutError after 10s
14:33:02  dispatch-pr871-...  1/1 Running  RESTARTS 1 (55s ago)

Three in-cluster clients reached the Service in the twenty seconds before the runner could not, and the pod never restarted again. The runner is a long-lived pod in another namespace; fresh pods get clean connection state, it does not. The app was never the problem, and neither was the assertion — only where the request came from.

Fetching from a throwaway pod removes the runner's connection state from the equation. curl -i keeps headers and body in one request, so the CSP header and the scripts it governs are guaranteed to describe the same response — two separate fetches could straddle a restart and compare a header against a different page.

Why the earlier probe told us nothing

#866 added a differential probe for exactly this question. On the one run where it fired, it produced no signal, because the runner's Role grants pods/log but not pods/attach:

warning: couldn't attach to pod/diag-pr871-18396, falling back to streaming logs:
  pods "diag-pr871-18396" is forbidden: ... cannot create resource "pods/attach"
Error from server (Forbidden): endpointslices.discovery.k8s.io is forbidden

kubectl run -i degrades silently rather than failing, so curl's output was lost. This step uses --attach=false plus kubectl logs instead, which the Role does permit.

Verification

The --raw-response parser was checked against three synthetic responses:

fixture expected result
nonced inline script, script-src 'self' 'nonce-abc123' pass exit 0, "all 1 inline script(s) carry a nonce"
bare inline script, script-src 'self' fail exit 1, names the unmatched script
no CSP header at all fail exit 1, "no Content-Security-Policy header"

npm run lint clean.

Notes

  • No retries. A failure here still means something is wrong. Retrying would have made this look like a flake forever, which is what I nearly did two days ago.
  • The health gate is unchanged — it already ran from in-cluster pods, which is why it kept passing while this step failed. It is now the same shape as the assertion rather than accidentally more reliable.
  • The --raw-response mode tolerates CRLF and a proxy-prepended block, and splits on the first blank line only.

The CSP assertion has failed repeatedly with a connect timeout while the app
was demonstrably serving. On the release PR: the health gate passed three
times from throwaway in-cluster pods at 14:32:31, :39 and :47, then the
runner's fetch timed out at :48 against a pod that was still 1/1 Running with
no further restart. The pod had restarted at ~14:32:07, changing its IP.

Fresh in-cluster pods reach the Service; the long-lived runner in another
namespace does not, immediately after the backend IP changes. The app was
never the problem, and neither was the assertion.

Fetch from a throwaway pod instead, which removes the runner's connection
state from the equation. curl -i keeps headers and body in one request so the
CSP header and the scripts it governs describe the same response, and
smoke-csp.mjs grows a --raw-response mode to parse that.

--attach=false is deliberate: the runner's Role grants pods/log but not
pods/attach, so `kubectl run -i` degrades to streaming logs and loses the
output. That is why the differential probe added in #866 produced no signal
on the one run where it finally fired.

No retries. A failure here still means something is wrong.

Claude-Session: https://claude.ai/code/session_01YSuDvZq9ncvyX85Uzx3cQh

@its-saffron its-saffron Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Automated Review

Full PR review.

Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — primary route

Recommendation: Approve

This PR correctly fixes the CSP smoke assertion by running it from inside the cluster rather than from the long-lived runner pod. The problem was well-documented: the runner's stale connection state timed out after app pod restarts while fresh in-cluster pods reached the same Service address immediately.

Change-by-Change Findings

.github/workflows/pr-smoke.yaml (+37/-21)

  • Replaces direct node scripts/smoke-csp.mjs call with a throwaway pod that fetches from inside the cluster
  • Uses kubectl run with --restart=Never --attach=false to avoid the pods/attach permission gap noted in the PR body
  • Polls pod phase for up to 60 seconds (30 × 2s) before fetching logs
  • Cleans up the pod with --ignore-not-found --wait=false immediately after logging
  • Validates non-empty response before passing to the script
  • Removed the differential probe added in PR 866, as the fix eliminates the runner vs. in-cluster asymmetry entirely

scripts/smoke-csp.mjs (+48/-10)

  • Added --raw-response <file> mode that parses curl -i output (status line, headers, blank line, body)
  • Handles CRLF (\r?\n) per HTTP spec
  • Splits on the first blank line to isolate headers from body, tolerating proxy-prepended 100-continue blocks
  • Falls back gracefully when CSP header is absent, continuing to parse body for inline scripts
  • Original direct-fetch mode (node smoke-csp.mjs <url>) retained for local dev use
  • Exits 2 for usage errors, 1 for test failures

Standards Compliance

The PR conforms to repository conventions:

  • Uses the same curlimages/curl:latest image as the health gate elsewhere in the workflow
  • No schema or migration changes
  • No new secrets or environment variables
  • Follows the existing workflow patterns (kubectl run, phase polling, log extraction)

Tool Harness Findings

No tool harness output in the corpus.

CI Check Results

All checks passed: Database integration, Docker Build, Tests, Typecheck, smoke, Coverage, Database migrations, Build, Docker Build (MCP), Lint, npm audit.

Unknowns / Needs Verification

None identified. The fix is self-contained, CI-passed, and the reasoning is well-supported by the observed timing data in the PR body (three in-cluster health checks succeeded while the runner timed out at the same Service address 47 seconds later).

@joryirving
joryirving merged commit 242c616 into main Aug 27, 2026
12 checks passed
@joryirving
joryirving deleted the fix/smoke-assert-in-cluster branch August 27, 2026 14:47
@its-miso its-miso Bot mentioned this pull request Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant