Let CI reach the admin vhost once Access is in front of it - #371
Merged
Conversation
The production smoke test polled GET /api/admin/storage on api.retina.fm for two minutes and failed the job if it never answered 200 or 202. A failed production smoke test SSHes to retina-prod and runs deploy/rollback.sh, so this probe could revert a release on its own. It has to go before require_admin starts enforcing identity, because it would then answer 401 to CI's unauthenticated curl and revert every deploy. A service token cannot rescue it either: api.retina.fm is the fleet's ingest hostname and must never carry an Access application, so there is nothing for a token to authenticate against, and the 401 comes from this codebase rather than the edge. Deleting rather than relocating it, because what it measures does not belong in that gate. It reports whether a background du scan has finished, which is a capacity signal on a timer, not evidence that the build now running is healthy; everything else in the block probes public routes and asserts on the request path a user actually takes. If the storage figure is worth watching it belongs in alerting, where noticing it late costs nothing. Nothing pinned this probe. test_towers_vhost_coverage.py constrains the staging smoke list, and test_smoke_tally_shared.py constrains only the shared counters, which the block still sources and still uses for the elevation and aircraft checks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tower contract is asserted against admin.retina.fm and staging-admin, and once those hostnames carry an Access application a browserless request meets a 302 to the login page. curl runs without -L, so assert_tower_contract reports `got HTTP 302` and the suite reads a missing credential as a routing fault. Both suites already share this file, so the headers go here rather than at each call site: the production block in ci.yml and staging-smoke-test.sh pick them up by sourcing it, and the two cannot drift on what they present. Inert until both variables are set, which is what makes it safe to land before the Access applications exist: an ungated hostname sees exactly the request it sees today, and so does a developer running the script by hand. Half a credential contributes nothing either, so a secret configured on its own cannot produce a request that is refused for a reason nobody can see. The array is expanded with the `+` guard at every call site because expanding an empty one is an unbound-variable error under `set -u` before bash 4.4, and staging-smoke-test.sh runs with `set -euo pipefail`. The token buys passage through the edge and nothing more. A service-token assertion carries common_name rather than email, so the origin's verifier finds no identity in it and it cannot reach an admin route; /api/towers, which is what these probes actually assert, needs none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The admin vhost will sit behind an Access application, and Playwright follows redirects, so a browser test against it would fetch Cloudflare's login page and then fail somewhere unhelpful: parsing that HTML as the app, rather than saying it was never admitted. The headers go on `use` so page navigation carries them, and are exported as well because request.newContext() does not inherit `use` and a spec building its own context against a gated host has to pass them itself. Empty unless both variables are set, so an ungated hostname and anyone running the suite locally see exactly what they see today. Both or neither: half a credential is refused at the edge just like none, and sending one would only make the refusal harder to read. Note this buys passage and not identity. A service-token assertion carries common_name rather than email, so the SPA's own /api/auth/me still comes back unauthenticated behind it, which is why this alone does not keep the admin surface-selection test alive once the bypass is closed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @Babissimo's task in 4m 39s —— View job Review: Let CI reach the admin vhost once Access is in front of itTraced every place this PR touches — Verified correct:
Worth noting, not blocking:
Verification claims in the PR description (tower-contract.sh exercised in all three secret states, workflow YAML parsing, No changes requested. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prepares CI for the Cloudflare Access applications that ClickUp
123zgec1gtf puts in front of the admin
hostnames. Nothing here depends on those applications existing, and nothing
changes behaviour until they do: this is the half that has to land first, so
they can be created without the pipeline going red in the window before the
backend change follows.
Independent of the backend PR #372 and touches no file it does. Either order.
Why now rather than with the rest
An Access application gates its hostname the moment it is saved, and there is no
disabled state.
staging-admin.retina.fmis probed bydeploy/staging-smoke-test.shand driven by
frontend/playwright.config.ts, ande2e-stagingis aneeds:ofdeploy-production, so creating that application before CI can present acredential stops every production deploy. With this merged, the applications can
be created at leisure.
Three changes
The
/api/admin/storageprobe is deleted. It polled that route onapi.retina.fmfor two minutes and failed the job if it never answered 200 or202, and a failed production smoke test runs
deploy/rollback.shonretina-prod. Once
require_adminenforces identity it answers 401 to CI'sunauthenticated curl and would revert every deploy. A service token cannot
rescue it:
api.retina.fmis the fleet's ingest hostname and must never carry anAccess application, so there is nothing to authenticate against and the 401 comes
from our own backend.
Deleted rather than moved, because what it measures does not belong in that gate:
it reports whether a background
duscan has finished, which is a capacity signalon a timer rather than evidence the build now running is healthy. Everything else
in that block probes public routes on the request path a user actually takes.
The smoke suites can present a service token. The headers live in
deploy/tower-contract.sh, which both suites already source, so the two cannotdrift on what they send. Empty unless both variables are set, so an ungated
hostname and a developer running the script by hand see exactly today's request.
Half a credential contributes nothing either, so a secret configured on its own
cannot produce a request refused for a reason nobody can see.
Playwright can too, via
use.extraHTTPHeaders, plus an exportedaccessHeadersfor specs that build their own request contexts (those do notinherit
use).Verification
deploy/tower-contract.shexercised in all three states: neither variable set(no headers, and survives
set -euo pipefail), both set (four correctly quotedargv words), one set (nothing).
e2e-stagingreceive the secrets.typecheck:e2epasses;npm run typecheckalso fails insrc/on thismachine for unrelated reasons (local Node 26 against CI's pinned 20).
pre-commit run --all-filesclean.What this does not do
The token buys passage through the edge and nothing more. A service-token
assertion carries
common_namerather thanemail, so the origin finds noidentity in it and it cannot reach an admin route.
/api/towers, which theseprobes actually assert, needs none.
🤖 Generated with Claude Code