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.
prk-Jr
changed the title
- 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
prk-Jr
requested review from
ChristianPavilonis and
aram356
and removed request for
aram356
aram356
left a comment
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.
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
…n-provider-type-migration' into feature/edgezero-pr14-entry-point-dual-path
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
I found 5 issues to address before this dual-path entry-point rollout is ready. Three are functional regressions that can change request handling or response metadata, and two are documentation/configuration gaps that should be cleaned up before operators rely on the new path.
Findings folded into the review body
🤔 API/docs comments lag new proxy-rebuild and settings behavior: The latest reviewer findings also called out stale API/docs comments in crates/trusted-server-core/src/proxy.rs:989-997 and crates/trusted-server-core/src/settings.rs:476-480. Those files are not part of this PR's current diff, so I could not place them as inline comments on this review, but they should still be reconciled before the new proxy-rebuild and runtime-settings behavior is considered fully documented.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
The previously blocking findings from the earlier review round are resolved: forwarded-header sanitization is now applied on the EdgeZero path, build_state() returns a Result with a startup_error_router fallback instead of panicking, AuthMiddleware preserves per-variant status codes via http_error, the "built once at startup" docstring is clarified, and edgezero deps are pinned to the full 40-char commit SHA 38198f9839b70aef03ab971ae5876982773fc2a1. parse_edgezero_flag is now case-insensitive with full test coverage.
Deep analysis surfaces two new blockers that share a single root cause: RouterService middleware in edgezero-core only runs on matched routes, and only GET/POST handlers are registered on this app. The combination produces two independent regressions on the EdgeZero path — non-GET/POST requests return 405 instead of being proxied to origin (legacy proxies every method), and router-level errors bypass FinalizeResponseMiddleware so responses miss X-Geo-*, X-TS-Version, X-TS-ENV, and operator-configured response_headers. Both can plausibly be fixed in one pass.
Inline comments cover the two blockers and the remaining non-blocking findings. CI locally: cargo fmt --check, cargo clippy --workspace --all-targets --all-features -- -D warnings, and cargo test --workspace all pass (821 tests).
Blocking
🔧 wrench
- Non-GET/POST methods regress to 405 — legacy
route_requestfalls through tohandle_publisher_requestfor every method; EdgeZero registers only GET and POST.RouterInner::dispatchreturnsErr(EdgeError::method_not_allowed)for HEAD / OPTIONS / PUT / DELETE / PATCH, whichoneshotturns into a bare 405. See inline oncrates/trusted-server-adapter-fastly/src/app.rs:422. - TS response headers dropped on router-level errors and handler
EdgeErrorpaths —RouterService::oneshothandlesErr(EdgeError)viaerr.into_response()outside the middleware chain, so 405/404/handler-error responses never reachFinalizeResponseMiddleware. Separately,next.run(ctx).await?in the middleware short-circuits on any innerErr. Both need changing so finalize always runs. See inline oncrates/trusted-server-adapter-fastly/src/middleware.rs:69.
Non-blocking
🤔 thinking
- Geo lookup runs on auth-rejected responses — legacy skips geo on 401 and sets
X-Geo-Info-Available: false; EdgeZero unconditionally attaches full geo headers to the 401. Inline onmiddleware.rs:71. - Per-request double open of
trusted_server_config—is_edgezero_enabled()anddispatch_with_configeach open the store. Usedispatch_with_config_handleand reuse a single handle. Inline onmain.rs:98. - TLS metadata hardcoded to
Noneon EdgeZero path —tls_protocol/tls_cipherare populated by legacy; the EdgeZeroFastlyRequestContextonly carriesclient_ip. Silent regression for any future signing/audit path. Previously flagged; still present. Inline onapp.rs:126.
♻️ refactor
- Eleven near-identical handler closures — two small helpers collapse all of them. Previously flagged. Inline on
app.rs:214. build_per_request_servicesduplicatesplatform::build_runtime_services— extract a shared helper parameterized byClientInfo. Previously flagged. Inline onapp.rs:129.- Module docstring claims middleware is always attached — but
startup_error_routerskips it. Inline onapp.rs:5.
🌱 seedling
fastly.tomldefaults local dev to EdgeZero — combined with the blockers above, everyfastly compute servereproduces them. Inline onfastly.toml:54.
📝 note
- No tests for
AuthMiddleware::handle,FinalizeResponseMiddleware::handle, orstartup_error_router—apply_finalize_headersis tested standalone, but the middlewarehandlemethods and the degraded startup-error path have no coverage. Inline onmiddleware.rs:238.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- prepare integration artifacts: PASS
- fmt / clippy / unit tests (local): PASS — 821 tests passing
… collect_body_bounded
…thods FinalizeResponseMiddleware now absorbs errors from inner middleware/handlers by converting EdgeError to a Response via IntoResponse, so apply_finalize_headers always runs regardless of handler outcome. Geo lookup is moved after next.run and skipped for UNAUTHORIZED responses to avoid unnecessary backend calls. Register HEAD and OPTIONS catch-all routes so cache-validation and CORS preflight requests reach the publisher origin instead of returning 405. Update module docstring to note startup_error_router skips middleware.
|
Superseding note: I re-posted the line-specific findings as inline comments after switching to the single-comment review-comments API with the PR head commit SHA. One additional high-priority finding is still not inline because the relevant code is outside this PR diff: P1 —
Suggested fix: thread |
|
@ChristianPavilonis Fixed the non-inline |
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Round-3 review. The previously blocking findings from round 2 are all resolved: forwarded-header sanitization runs on the EdgeZero path, build_state returns a Result with a startup_error_router fallback, AuthMiddleware preserves per-variant status codes via http_error, FinalizeResponseMiddleware absorbs handler errors via IntoResponse, the AppState docstring is corrected, workspace edgezero deps are pinned to the full 40-char commit SHA 38198f9839b70aef03ab971ae5876982773fc2a1, parse_edgezero_flag is case-insensitive with full test coverage, HEAD/OPTIONS/PUT/PATCH/DELETE catch-all routes are registered, and FinalizeResponseMiddleware now takes Arc<dyn PlatformGeo> for testability.
This round surfaces two new blockers and four non-blocking findings. Both blockers are scope/behavior issues, not implementation bugs:
- Silent behavior change in
proxy.rs— integration-proxy redirect chains are no longer bounded bysettings.proxy.allowed_domains, and nothing in the PR body documents it. - Non-standard HTTP methods bypass the middleware chain — legacy
route_requestfalls through to publisher origin for every method; this path 405/404s them without runningFinalizeResponseMiddleware, dropping all TS/geo/operator headers. Contradicts theFinalizeResponseMiddlewaredoc contract.
Both are addressable in this PR without a large rework. Inline comments describe concrete fix options.
Local CI is green: cargo fmt --check, cargo clippy --workspace --all-targets --all-features -D warnings, and cargo test --workspace (867 tests).
Blocking
🔧 wrench
- Scope creep: silent integration-proxy allowlist semantics change —
proxy.rsnow lets integration proxies opt out ofsettings.proxy.allowed_domainsfor initial target and redirect hops. Not in PR description. See inline oncrates/trusted-server-core/src/proxy.rs:446. - Non-standard HTTP methods bypass middleware — TRACE/CONNECT/WebDAV verbs skip
FinalizeResponseMiddleware; TS/geo/operator headers dropped. Contradicts the doc contract. See inline oncrates/trusted-server-adapter-fastly/src/app.rs:149.
Non-blocking
🤔 thinking
- Per-request INFO-level routing log — noisy at production traffic; consider
log::debug!or once-per-cold-start. Inline onmain.rs:96.
📝 note
- No tests for
FinalizeResponseMiddleware::handle/AuthMiddleware::handle— the code changed this round has no direct unit coverage. Inline onmiddleware.rs:130. - No end-to-end test for the EdgeZero dispatch path — would catch the method-bypass regression directly. Inline on
app.rs:392.
CI Status
- browser integration tests: PASS (GitHub)
- integration tests: PASS (GitHub)
- prepare integration artifacts: PASS (GitHub)
- fmt: PASS (local)
- clippy: PASS (local)
- rust tests: PASS (local, 867 tests)
| copy_request_headers, | ||
| stream_passthrough, | ||
| allowed_domains: _, | ||
| allowed_domains, |
There was a problem hiding this comment.
🔧 wrench — Scope creep: silent behavior change to integration-proxy allowlist enforcement, not in PR description.
Before this PR, proxy_with_redirects accepted allowed_domains: _ (unused) and always enforced &settings.proxy.allowed_domains on the initial target and every redirect hop. After this PR, integration callers (gpt, google_tag_manager, permutive, etc.) pass allowed_domains: &[] → open mode (proxy.rs:519–524: empty slice permits all) → integration proxy redirect chains are no longer bounded by operator-configured allowed_domains. Only handle_first_party_proxy at proxy.rs:790 still enforces the settings allowlist.
This is security-relevant. Integration redirect hops can now follow Location to arbitrary hosts regardless of settings.proxy.allowed_domains. Likely intentional (integrations are operator-configured; first-party proxy handles untrusted creative-referenced URLs), but nothing in the PR body or the ## Changes table mentions it, and a reviewer reading git blame on the "dual-path entry point" PR will not expect this semantic shift.
Fix options (pick one, not all):
- Split into a separate PR with its own description, tests, and reviewer sign-off on the security trade-off.
- Update the PR body with an explicit "Proxy allowlist semantics" section explaining which callers opt into open mode and why.
- Add a doc comment on
ProxyRequestConfig::allowed_domainsexplaining the open-mode contract and the integration/first-party split, and update the PR body.
Blocking until the scope is explicit — the change may be fine to land, but not silently.
| Method::PATCH, | ||
| Method::DELETE, | ||
| ] | ||
| } |
There was a problem hiding this comment.
🔧 wrench — Non-standard HTTP methods bypass the middleware chain; TS/geo headers dropped.
publisher_fallback_methods() covers 7 methods (GET/POST/HEAD/OPTIONS/PUT/PATCH/DELETE). For any method outside that set (TRACE, CONNECT, WebDAV PROPFIND/PROPPATCH, LINK, UNLINK, custom verbs), RouterInner::dispatch (edgezero-core router.rs:260-271) returns Err(EdgeError::method_not_allowed) or Err(EdgeError::not_found). RouterService::oneshot (router.rs:234-240) converts that Err to a response via err.into_response() outside the middleware chain.
Consequence: 405/404 responses on those methods skip FinalizeResponseMiddleware — no X-Geo-*, X-TS-Version, X-TS-ENV, and no operator-configured response_headers. Legacy route_request falls through to handle_publisher_request for every method, so this is a silent legacy→EdgeZero semantic regression on the "unusual method" envelope, and it contradicts the FinalizeResponseMiddleware doc contract ("every outgoing response — including auth-rejected ones — carries a consistent set of headers").
Fix options:
- Wrap the dispatch at the entry point so
apply_finalize_headersruns on router-level errors too — call it on theFastlyResponseproduced bydispatch_with_configbefore returning fromfn main. - Add a top-level "always-apply-finalize" outer service that wraps the
RouterService. - Expand the method list to cover everything the edge can deliver.
Option 1 or 2 is the robust fix — Option 3 is fragile (new methods added upstream would silently regress).
| // skips logger initialisation and injects the config store directly. | ||
| edgezero_adapter_fastly::dispatch_with_config(&app, req, "trusted_server_config") | ||
| } else { | ||
| log::info!("routing request through legacy path"); |
There was a problem hiding this comment.
🤔 thinking — Per-request INFO-level routing log is noisy.
log::info!("routing request through EdgeZero path") at main.rs:85 and log::info!("routing request through legacy path") at main.rs:96 fire on every request. At production traffic rates this produces one INFO line per request with no actionable information — operators already know the flag value from config. Consider log::debug! for per-request, or use a OnceLock to log once per cold start so the signal is retained without the volume.
|
|
||
| next.run(ctx).await | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 note — Missing test coverage for FinalizeResponseMiddleware::handle and AuthMiddleware::handle.
apply_finalize_headers is tested directly (good), and startup_error_router / uses_dynamic_tsjs_fallback have focused tests. But the middleware handle methods themselves — the code that was changed this round — are not exercised:
- "Auth rejection bubbles through
FinalizeResponseMiddlewareand gets TS headers" contract. match next.run(ctx).await { Err(e) => e.into_response() }absorption path added this round.if response.status() != UNAUTHORIZED { geo.lookup(...) }branch.FastlyRequestContext::get(ctx.request()).and_then(|c| c.client_ip)extraction.
A couple of handle unit tests with a stub Next and a mock PlatformGeo would pin down the round-2 fixes and prevent regression.
| } | ||
|
|
||
| router.build() | ||
| } |
There was a problem hiding this comment.
📝 note — No end-to-end test exercises the full EdgeZero dispatch path.
None of the new tests drive TrustedServerApp::routes() → oneshot(request) end-to-end through the router + middleware + fallback chain. An E2E test would:
- Catch the method-bypass regression directly (assert
X-TS-Versionpresent on a TRACE/CONNECT response). - Pin down the middleware ordering contract (FinalizeResponseMiddleware outermost).
- Verify auth-rejected 401s still receive TS headers but skip geo lookup.
Given the size of this PR, an E2E test is high leverage.
3 participants
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!)