Add dual-path EdgeZero entry point with feature flag (PR 14)#628
Conversation
Replace the app.rs stub with the full EdgeZero application wiring:
- AppState struct holding Settings, AuctionOrchestrator,
IntegrationRegistry, and PlatformKvStore
- build_per_request_services() builds RuntimeServices per request using
FastlyRequestContext for client IP extraction
- http_error() mirrors legacy http_error_response() from main.rs
- All 12 routes from legacy route_request() registered on RouterService
- Catch-all GET/POST handlers using matchit {*rest} wildcard dispatch
to integration proxy or publisher origin fallback
- FinalizeResponseMiddleware (outermost) and AuthMiddleware registered
…x handler pattern
- Remove Arc::new() wrapper around build_state() which already returns Arc<AppState>
- Remove dedicated GET /static/{*rest} route and its tsjs_handler closure
- Move tsjs handling into GET /{*rest} catch-all: check path.starts_with("/static/tsjs=") first
- Extract path/method from ctx.request() before ctx.into_request() to keep &req valid
- Replace .map_err(|e| EdgeError::internal(...)) with .unwrap_or_else(|e| http_error(&e)) in all named-route handlers
- Remove configure() method from TrustedServerApp (not part of spec)
- Remove unused App import
…, unused turbofish, and overly-broad field visibility - Drop `.into_bytes()` in `http_error`; `Body` implements `From<String>` directly - Remove `Box::pin` wrapper from `get_fallback` closure; plain `async move` matches all other handlers - Remove `Ok::<Response, EdgeError>` turbofish in `post_fallback`; type is now inferred - Drop now-unused `EdgeError` import that was only needed for the turbofish - Narrow `AppState` field visibility from `pub` to `pub(crate)`; struct is internal to this crate
Switches all four edgezero workspace dependencies from rev=170b74b to branch=main so the adapter can use dispatch_with_config, the non-deprecated public dispatch path. The main branch requires toml ^1.1, so the workspace pin is bumped from "1.0" to "1.1" to resolve the version conflict.
Replaces the deprecated dispatch() call with dispatch_with_config(), which injects the named config store into request extensions without initialising the logger a second time (a second set_logger call would panic because the custom fern logger is already initialised above). Adds log::info lines for both the EdgeZero and legacy routing paths.
matchit's /{*rest} catch-all does not match the bare root path /. Add
explicit .get("/", ...) and .post("/", ...) routes that clone the fallback
closures so requests to / reach the publisher origin fallback rather than
returning a 404.
Registers the trusted_server_config config store in fastly.toml with edgezero_enabled = "true" so that fastly compute serve routes requests through the EdgeZero path without needing a deployed service.
- Normalise get_fallback to extract path/method from req after consuming the context, consistent with post_fallback and avoiding a double borrow on ctx - Add comment to http_error documenting the intentional duplication with http_error_response in main.rs (different HTTP type systems; removable in PR 15) - Add comment above route handlers explaining why the explicit per-handler pattern is kept over a macro abstraction
aram356
left a comment
There was a problem hiding this comment.
Summary
Reviewed the 13 files that PR 628 actually changes vs its base (feature/edgezero-pr13-...). The dual-path entry point is a sensible migration shape, and the explicit GET/POST "/" routes + the header-precedence middleware test are the kind of load-bearing details that prevent future outages. However, the EdgeZero path currently diverges from the legacy path in ways that are security- and reliability-relevant, and the branch = "main" pin on upstream deps makes builds non-deterministic. Blocking on those.
Blocking
🔧 wrench
- Forwarded-header sanitization missing on EdgeZero path — legacy strips
Forwarded/X-Forwarded-*/Fastly-SSLbefore routing; EdgeZero hands the rawreqtodispatch_with_config. Withedgezero_enabled = "true"as the local-dev default, this is the default path. (main.rs:95) build_state()panics on misconfig —expect("should …")on settings / orchestrator / registry. Legacy returns a structured error response; EdgeZero now 5xx's with no detail. (app.rs:75)- Docstring "built once at startup" is misleading — every request spins up a fresh Wasm instance, so
build_state()runs per-request. Invites future false caching. (app.rs:61) - Stale
#[allow(dead_code)]on now-live middleware — five suppressions with "until Task 4 wires app.rs" comments. Task 4 is this PR. (middleware.rs:50,57,96,103,146) AuthMiddlewareflattensReport<TrustedServerError>intoEdgeError::internal(io::Error::other(...))— loses per-variant status code and user message; generic 500 instead of the specific error. (middleware.rs:122)edgezero-*deps pinned tobranch = "main"— non-deterministic builds; supply-chain path into prod via a moving upstream branch. Pin to a specificrevor fork tag. (Cargo.toml:59-62)
Non-blocking
🤔 thinking
- TLS metadata dropped on EdgeZero path —
tls_protocol/tls_cipherhardcoded toNone; legacy populates both. Low impact today (debug logging only), but a silent regression if any future signing/audit path reads them. (app.rs:123-124)
♻️ refactor
- 11 near-identical handler closures in
routes()— a pair of file-localmake_sync_handler/make_async_handlerhelpers would cut ~120 lines without harming auditability. (app.rs:175-301) FinalizeResponseMiddlewarehardcodesFastlyPlatformGeo— takeArc<dyn PlatformGeo>instead soMiddleware::handlecan be unit-tested end-to-end. (middleware.rs:68)build_per_request_servicesduplicatesplatform::build_runtime_services— extract a shared helper that takesClientInfo. (app.rs:111-127)
🌱 seedling
fastly.tomlflips local dev default to EdgeZero — combined with the blockers above, everyfastly compute servenow exposes them. Consider defaulting to"false"until the blockers land. (fastly.toml:52)
⛏ nitpick
AppStatefields can be private (notpub(crate)). (app.rs:62-66)- Root-route pairs clone closures four times — upstream
RouterService::get_manywould help. (app.rs:374-377)
📝 note
- The
dispatch_with_configcomment explaining theset_loggerpanic is excellent "why, not what" documentation. (main.rs:91-94)
👍 praise
operator_response_headers_override_earlier_headerscodifies a brittle precedence contract. (middleware.rs:217-233)- Explicit
GET "/"/POST "/"routes with the in-code explanation of matchit's wildcard gap prevent a future 404 outage. (app.rs:374-377)
CI Status
- browser integration tests: PASS
- integration tests: PASS
- prepare integration artifacts: PASS
- fmt / clippy / unit tests: not surfaced by
gh pr checks— please confirm these ran.
| // internally — a second `set_logger` call panics because our custom | ||
| // fern logger is already initialised above. `dispatch_with_config` | ||
| // skips logger initialisation and injects the config store directly. | ||
| edgezero_adapter_fastly::dispatch_with_config(&app, req, "trusted_server_config") |
There was a problem hiding this comment.
🔧 wrench — EdgeZero path does not strip spoofable forwarded headers
The legacy path calls compat::sanitize_fastly_forwarded_headers(&mut req) at main.rs:159 to strip Forwarded, X-Forwarded-Host, X-Forwarded-Proto, Fastly-SSL, etc. before any request-derived context is built. The EdgeZero dispatch path hands req directly to dispatch_with_config with no equivalent sanitization — any downstream code that trusts those headers (geo/host/proto) is now reachable with client-spoofed values when the flag is on.
With fastly.toml defaulting local dev to edgezero_enabled = "true", this is the default code path going forward.
Fix: strip before dispatch, and add a regression test asserting the headers are absent from ctx.request() at handler entry.
if is_edgezero_enabled().unwrap_or_else(|e| { /* … */ }) {
log::info!("routing request through EdgeZero path");
let app = TrustedServerApp::build_app();
compat::sanitize_fastly_forwarded_headers(&mut req);
edgezero_adapter_fastly::dispatch_with_config(&app, req, "trusted_server_config")
}(req will need to be mut in main.)
| /// errors or unrecoverable misconfiguration that cannot be handled at request | ||
| /// time. | ||
| fn build_state() -> Arc<AppState> { | ||
| let settings = get_settings().expect("should load trusted-server settings at startup"); |
There was a problem hiding this comment.
🔧 wrench — Settings / orchestrator / registry failures now panic instead of returning an error response
build_state() uses expect("should …") on get_settings, build_orchestrator, and IntegrationRegistry::new. On Fastly Compute each instance is per-request, so routes() → build_state() runs every request — an expect becomes a panic → generic 5xx with no structured error.
The legacy path (main.rs:116-140) handles each of these as Result and returns to_error_response(&e) with the correct status code and user message. This is a behavioral regression for any misconfigured deploy.
Fix: propagate the errors instead of panicking. Options:
- Memoize the state outside
routes()and install a trivial "config error" handler on failure. - Have each handler closure check a pre-built
Result<Arc<AppState>, Report<TrustedServerError>>and returnhttp_error(&report)onErr.
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// Application state built once at startup and shared across all requests. | ||
| pub struct AppState { |
There was a problem hiding this comment.
🔧 wrench — Docstring claims "built once at startup" but routes() rebuilds per request
On Fastly Compute every request spins up a fresh Wasm instance, so TrustedServerApp::build_app() → routes() → build_state() runs every request. The current docstring invites future contributors to cache expensive values in AppState under a false assumption.
Fix: rewrite the comment to say "once per request instance" — or, preferably, hoist settings/orchestrator/registry construction out of routes() so adding real caching is cheap later.
| /// 3. `X-TS-ENV: staging` when `FASTLY_IS_STAGING == "1"` | ||
| /// 4. Operator-configured `settings.response_headers` (can override any managed header) | ||
| // Used in Task 4 when app.rs registers the middleware chain. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
🔧 wrench — Stale #[allow(dead_code)] on now-live middleware
FinalizeResponseMiddleware, AuthMiddleware, their new()s, and apply_finalize_headers all carry #[allow(dead_code)] with "until Task 4 wires app.rs" comments. Task 4 is done in this PR — app.rs:360-361 registers both. Leaving these in place masks future dead code; clippy -D warnings will not flag unused-behind-allow.
Fix: delete the five #[allow(dead_code)] lines (50, 57, 96, 103, 146) and the stale "Used in Task 4" comments.
| // as a direct dependency (which the project convention forbids). | ||
| return Err(EdgeError::internal(std::io::Error::other(format!( | ||
| "auth check failed: {report}" | ||
| )))); |
There was a problem hiding this comment.
🔧 wrench — AuthMiddleware downgrades rich errors to a generic 500
On internal failure, enforce_basic_auth's Report<TrustedServerError> is flattened into EdgeError::internal(std::io::Error::other(...)). That loses the TrustedServerError discriminant and per-variant status_code() / user_message() — callers see a generic 500 instead of the specific error the legacy path would have surfaced via http_error_response. The std::io::Error::other workaround-for-anyhow is also a code smell.
Fix: convert the report to a Response via http_error(&report) and return Ok(response), matching how app.rs handlers surface TrustedServerError.
Err(report) => {
log::error!("auth check failed: {:?}", report);
return Ok(crate::app::http_error(&report));
}(Requires exporting http_error from app.rs or moving it to a shared module.)
|
|
||
| [local_server.config_stores] | ||
| [local_server.config_stores.trusted_server_config] | ||
| format = "inline-toml" |
There was a problem hiding this comment.
🌱 seedling — this flips local dev default to EdgeZero
Landing with edgezero_enabled = "true" means every fastly compute serve now exercises the new path. Good for dogfooding, but combined with the header-sanitization gap and the panic-on-misconfig findings above, every local dev run is currently exposed to those issues.
Consider defaulting local dev to "false" and flipping it in a follow-up once the blockers land — or call this out in the PR description so reviewers know what they're opting into.
|
|
||
| /// Application state built once at startup and shared across all requests. | ||
| pub struct AppState { | ||
| pub(crate) settings: Arc<Settings>, |
There was a problem hiding this comment.
⛏ nitpick — AppState fields can be private
pub(crate) but only touched inside app.rs. Drop to private.
| // matchit's `/{*rest}` does not match the bare root `/` — register | ||
| // explicit root routes so `/` reaches the publisher fallback too. | ||
| .get("/", get_fallback.clone()) | ||
| .post("/", post_fallback.clone()) |
There was a problem hiding this comment.
⛏ nitpick — Root-route double-registration clones closures
.get("/", get_fallback.clone()) + .get("/{*rest}", get_fallback). Tiny, but a RouterService::get_many(["/", "/{*rest}"], h) helper upstream would be cleaner. Worth a seedling issue against edgezero_core::router.
| let app = TrustedServerApp::build_app(); | ||
| // `run_app_with_config` and `run_app_with_logging` call `init_logger` | ||
| // internally — a second `set_logger` call panics because our custom | ||
| // fern logger is already initialised above. `dispatch_with_config` |
There was a problem hiding this comment.
📝 note — great "why, not what" comment
Calling out the exact reason (set_logger panic) future readers will need when they touch the dispatch path. Keep doing this.
| Some("operator-override"), | ||
| "should override the managed geo header with the operator-configured value" | ||
| ); | ||
| } |
There was a problem hiding this comment.
👍 praise — header-precedence test codifies a brittle contract
operator_response_headers_override_earlier_headers nails down a precedence rule that's easy to break accidentally. Exactly the kind of regression test this layer needs.
There was a problem hiding this comment.
Summary
Supplementary review — see aram356's review for the primary findings. This review covers additional items not raised there.
Non-blocking
🔧 wrench (cross-cutting, from earlier PRs in this stack)
set_headerdrops multi-valued headers:edge_request_to_fastlyinplatform.rs:187usesset_headerinstead ofappend_header, silently dropping duplicate headers. Pre-existing pattern (also incompat::to_fastly_request), but the EdgeZero path creates a new copy of the same bug.
🌱 seedling
parse_edgezero_flagis case-sensitive:"TRUE"and"True"silently fall through to legacy path. Considereq_ignore_ascii_caseor logging unrecognized values.
📝 note (cross-cutting, from earlier PRs)
- Stale doc comment in
platform/mod.rs:31: Referencesfastly::Bodyin publisher.rs, but PR 11 already migrated toEdgeBody.
♻️ refactor (cross-cutting, from earlier PRs)
- Duplicated
body_as_readerhelper: Identical function inproxy.rs:24andpublisher.rs:23. Extract to shared utility.
⛏ nitpick (cross-cutting)
- Management API client re-created per write: Each
put/deleteinplatform.rsconstructs a newFastlyManagementApiClient. Fine for current usage, noted for future batch writes.
📌 out of scope
compat.rsin core depends onfastlytypes: Already tracked as PR 15 removal target.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- prepare integration artifacts: PASS
| /// | ||
| /// Accepted values (after whitespace trimming): `"true"` and `"1"`. | ||
| /// All other values, including the empty string, are treated as disabled. | ||
| fn parse_edgezero_flag(value: &str) -> bool { |
There was a problem hiding this comment.
🌱 seedling — parse_edgezero_flag is case-sensitive
"TRUE" and "True" silently fall through to the legacy path. Operators who write edgezero_enabled = "True" in a config store will get unexpected behavior with no indication why. Consider eq_ignore_ascii_case or at least logging unrecognized values:
fn parse_edgezero_flag(value: &str) -> bool {
let v = value.trim();
v.eq_ignore_ascii_case("true") || v == "1"
}
Summary
TrustedServerAppor the preservedlegacy_mainbased onedgezero_enabledin thetrusted_server_configFastly ConfigStore, withfalseas the safe default on any read failureTrustedServerAppviaedgezero_core::app::Hookswith all routes, plusFinalizeResponseMiddlewareandAuthMiddleware— matching legacy routing semantics without the compat conversion layerbranch=mainand usesdispatch_with_config(non-deprecated) to avoid the doubleset_loggerpanic thatrun_app/run_app_with_loggingwould causeChanges
Cargo.tomlbranch=main; bumptomlto"1.1"Cargo.lockcrates/trusted-server-adapter-fastly/src/main.rsis_edgezero_enabled(), feature-flag dispatch block, extractlegacy_main(), usedispatch_with_configcrates/trusted-server-adapter-fastly/src/app.rsTrustedServerAppimplementingHookswith all 14 routes; explicitGET /andPOST /to cover matchit root path gapcrates/trusted-server-adapter-fastly/src/middleware.rsFinalizeResponseMiddlewareandAuthMiddlewarewith golden header-precedence testfastly.tomltrusted_server_configconfig store for local dev withedgezero_enabled = "true"crates/trusted-server-core/src/auction/orchestrator.rscrates/trusted-server-core/src/integrations/google_tag_manager.rscrates/trusted-server-core/src/integrations/lockr.rscrates/trusted-server-core/src/integrations/permutive.rscrates/trusted-server-core/src/integrations/prebid.rscrates/trusted-server-core/src/integrations/testlight.rs.claude/settings.jsonCloses
Closes #495
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatfastly compute serve— EdgeZero path routes all named routes andGET /correctly; legacy path reachable by settingedgezero_enabled = "false"Checklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)