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)
…on' into feature/edgezero-pr14-entry-point-dual-path
…dispatch_unregistered_method_returns_405_at_router_level
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed the remaining PR #628 findings after excluding the consent_store replacement item and the orchestrator issue per discussion. I found two high-impact EdgeZero/legacy parity concerns plus a few documentation/comment accuracy issues. GitHub checks currently shown for the PR are passing.
| // carry TS/geo headers, matching the legacy path's all-methods guarantee. | ||
| // For requests that ran through FinalizeResponseMiddleware, this is | ||
| // idempotent — header::insert overwrites with the same values. | ||
| if let Ok(settings) = get_settings() { |
There was a problem hiding this comment.
Entry-point finalization re-adds geo headers to auth-rejected 401 responses
FinalizeResponseMiddleware intentionally skips geo lookup for 401 Unauthorized responses, matching the legacy path. The EdgeZero entry point then finalizes every response again after dispatch_with_config() and unconditionally performs geo lookup. That can overwrite X-Geo-Info-Available: false with full geo headers on auth-rejected admin requests.
Suggestion: Only entry-point-finalize responses that did not already pass through middleware, or preserve the 401 no-geo rule here as well. For example:
if let Ok(settings) = get_settings() {
let geo_info = if response.status() == edgezero_core::http::StatusCode::UNAUTHORIZED {
None
} else {
FastlyPlatformGeo.lookup(client_ip).unwrap_or_else(|e| {
log::warn!("entry-point geo lookup failed: {e}");
None
})
};
apply_finalize_headers(&settings, geo_info.as_ref(), &mut response);
}Better: use a sentinel header such as X-Geo-Info-Available to skip re-finalizing responses that already ran through FinalizeResponseMiddleware.
| .geo(Arc::new(FastlyPlatformGeo)) | ||
| .client_info(ClientInfo { | ||
| client_ip, | ||
| tls_protocol: None, |
There was a problem hiding this comment.
EdgeZero path loses TLS metadata, making scheme detection fall back to http
build_per_request_services() sets tls_protocol and tls_cipher to None. After forwarded-header sanitization, RequestInfo::from_request() can no longer use X-Forwarded-Proto / Fastly-SSL, so it defaults to http.
This can change publisher URL rewriting and first-party URL generation from https://... to http://... on the EdgeZero path.
Suggestion: Carry TLS metadata through the EdgeZero Fastly request context, or temporarily assume HTTPS in the Fastly adapter until upstream exposes TLS fields.
| .build() | ||
| } | ||
|
|
||
| fn publisher_fallback_methods() -> [Method; 7] { |
There was a problem hiding this comment.
Fallback routing still does not preserve arbitrary-method legacy behavior
Legacy routing falls through to publisher origin for every method after auth enforcement. EdgeZero now registers common methods (GET, POST, HEAD, OPTIONS, PUT, PATCH, DELETE), but methods such as TRACE, CONNECT, and WebDAV verbs still return router-level 405 before middleware/auth/fallback routing.
Suggestion: Use an EdgeZero/router “any method” catch-all if available. If not, explicitly document the intentional method restriction and avoid claiming full all-method parity.
| let mut response = compat::from_fastly_response( | ||
| edgezero_adapter_fastly::dispatch_with_config(&app, req, "trusted_server_config")?, | ||
| ); | ||
| // Apply finalize headers at the entry point so that router-level 405/404 |
There was a problem hiding this comment.
Entry-point finalization comment incorrectly claims duplicate finalization is idempotent
The comment says repeated apply_finalize_headers() overwrites with the same values, but the 401 geo behavior shows that is not always true: middleware can write X-Geo-Info-Available: false, while the entry-point wrapper can later add real geo headers.
Suggestion: Either skip re-finalization for already-finalized responses or narrow this comment to only claim router-level header coverage.
| /// - `Ok(Some(response))` from [`enforce_basic_auth`] → auth failed; return the | ||
| /// challenge response (bubbles through [`FinalizeResponseMiddleware`] for header injection). | ||
| /// - `Ok(None)` → no auth required or credentials accepted; continue the chain. | ||
| /// - `Err(report)` → internal error; log and convert to a 500 HTTP response. |
There was a problem hiding this comment.
AuthMiddleware docs still describe generic 500 behavior
The implementation now calls crate::app::http_error(&report), preserving the TrustedServerError status code and user message. This comment still says auth errors are converted to a 500 HTTP response.
Suggestion: Update the docs to say the error report is converted via http_error() using the error's documented status code.
| /// `#[fastly::main]` so we can call `send_to_client()` explicitly when needed. | ||
| fn main() { | ||
| init_logger(); | ||
| /// Accepted values (after whitespace trimming): `"true"` and `"1"`. |
There was a problem hiding this comment.
EdgeZero flag docs omit case-insensitive true
parse_edgezero_flag() accepts TRUE / True via eq_ignore_ascii_case, but the comment only lists true and 1.
Suggestion: Document accepted values as "1" or "true" in any ASCII case.
| /// | ||
| /// **Behavior change from pre-PR-14**: `proxy_with_redirects` previously always | ||
| /// enforced `&settings.proxy.allowed_domains` regardless of the caller. After PR 14, | ||
| /// only [`handle_first_party_proxy`] and its siblings enforce the operator allowlist; |
There was a problem hiding this comment.
allowed_domains docs overstate allowlist enforcement
The doc says handle_first_party_proxy “and its siblings” enforce settings.proxy.allowed_domains, but the current code only passes the allowlist from handle_first_party_proxy; click/sign/rebuild do not all enforce it in the same way.
Suggestion: Make the comment precise: only callers passing a non-empty allowed_domains slice enforce the allowlist, and currently handle_first_party_proxy does that.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-review against main-merge intent. The dual-path entry point and middleware shape are right, and prior round-2 blockers (build_state Result, forwarded-header sanitization, parse_edgezero_flag casing) are resolved. Three new functional regressions remain that change request handling vs legacy on identical input, plus two correctness/process gaps.
PR base is feature/edgezero-pr13-…; the diff vs main is 58 files because PRs 9–13 are unmerged. Findings below scope to the 13 files in this PR's actual delta.
CI: PASS (browser/integration tests, prepare artifacts).
Blocking
🔧 wrench
- EdgeZero path silently degrades when
consent_storeis misconfigured (compliance regression) —AppState::kv_storeis hardcoded toUnavailableKvStoreand the EdgeZero path never callsruntime_services_for_consent_route. Legacy returns 503 fail-closed whenopen_kv_store(name)fails (seeroute_tests.rs::configured_missing_consent_store_only_breaks_consent_routes); EdgeZero fail-opens becausetry_kv_fallbackswallowsKvError::Unavailablevia?. Inline onapp.rs:92. - Entry-point geo lookup overrides middleware's 401 geo-skip in production — Middleware correctly skips geo on 401 and emits
X-Geo-Info-Available: false; the entry-pointapply_finalize_headersthen runs an unconditional geo lookup and overwrites it with full geo headers when a real client IP is present. The unit test passes only because it bypassesmain()andlookup(None) → None. Inline onmain.rs:115. - Non-GET/POST methods on registered named-route paths regress to 405 — Named routes register a single method; matchit prefers the exact path over
/{*rest}, soHEAD /first-party/proxy,OPTIONS /auction,PUT /admin/keys/rotate, etc. return 405 (legacy proxies them to publisher origin). The existing 405 test only covers TRACE on/, which doesn't exercise this case. Inline onapp.rs:376. - Silent settings drop on entry-point
if let Ok(settings) = get_settings()— OnErr, response goes out without TS headers; legacy fails fast atbuild_state(). Inline onmain.rs:110. - PR description doesn't match the diff — undisclosed
tokio-test = "0.4"workspace dep (unused, not inCargo.lock); description says edgezero deps pinned tobranch=mainbut actual is a 40-char SHA;proxy.rsrefactor + new "Behavior change from pre-PR-14" doc paragraph (factually wrong vsorigin/main) not mentioned. Inlines onCargo.toml:86andproxy.rs:89.
❓ question
- Where is consent KV opened on the EdgeZero path? — If finding 1 is intentional (wired in PR 15/16), please link the issue/spec. Inline on
app.rs:92.
Non-blocking
🤔 thinking
- TLS metadata still hardcoded to
None—tls_protocol/tls_ciphernot populated fromFastlyRequestContext; silent regression for any future signing/audit path. Inline onapp.rs:124. Previously flagged. - Two geo lookups per request on the happy path — middleware + entry-point. Guard the second behind a "response missing TS headers" check. Inline on
main.rs:111. - Per-request double open of
trusted_server_configconfig store —is_edgezero_enabled+dispatch_with_configboth open it; SDK caches the handle, but the duplication is avoidable. Inline onmain.rs:66. Previously flagged.
♻️ refactor
- Eleven near-identical handler closures —
app.rs:273-359. Helper orroute!macro collapses them. Inline onapp.rs:351. Previously flagged. build_per_request_servicesduplicatesplatform::build_runtime_services— bodies identical except forclient_infosource. Inline onapp.rs:127. Previously flagged.- Module docstring overstates middleware coverage — top summary contradicts the "Startup error handling" section. Inline on
app.rs:9.
🌱 seedling
fastly.tomldefaults local dev to EdgeZero — everyfastly compute servereproduces the regressions above. Recommend defaulting to"false"until parity is reached. Inline onfastly.toml:48.
📝 note
- No EdgeZero-side test for the consent-store regression — mirror
configured_missing_consent_store_only_breaks_consent_routesdrivingTrustedServerApp::routes()after fix. - No EdgeZero-side test for HEAD/OPTIONS on a named-route path — add
dispatch_head_on_named_get_route_proxies_to_publisher. legacy_maincleanup TODO references issue #495 — confirm the issue coverscompat.rs,route_request,legacy_main, theapp::http_errorduplication, and the entry-point finalize wrap.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- prepare integration artifacts: PASS
|
|
||
| let registry = IntegrationRegistry::new(&settings)?; | ||
|
|
||
| let kv_store = Arc::new(UnavailableKvStore) as Arc<dyn PlatformKvStore>; |
There was a problem hiding this comment.
🔧 wrench — EdgeZero path silently degrades when consent_store is misconfigured (compliance regression).
AppState::kv_store is hardcoded to UnavailableKvStore for the lifetime of the Wasm instance. Legacy route_request calls runtime_services_for_consent_route for /auction and the publisher fallback (main.rs:307-323); when settings.consent.consent_store is set but open_kv_store(name) fails, it returns Report<TrustedServerError::KvStore> and the route responds 503 fail-closed (test in route_tests.rs:222-260).
On this path, every consent KV read goes through UnavailableKvStore, which returns KvError::Unavailable from every method (platform/kv.rs:18-46). try_kv_fallback swallows the error via ? (consent/mod.rs:528-540), so the auction proceeds with no stored consent — fail-open.
Net effect: an operator typo on consent_store is fail-closed on legacy, fail-open on EdgeZero.
Fix: port runtime_services_for_consent_route into the EdgeZero path. Either resolve the consent KV store eagerly when building per-request services for consent-touching routes, or wrap auction_handler and fallback_handler with a helper that swaps services.kv_store() to the configured consent store and short-circuits with a 503 on open failure. Add a test mirroring configured_missing_consent_store_only_breaks_consent_routes driving TrustedServerApp::routes().
| log::warn!("entry-point geo lookup failed: {e}"); | ||
| None | ||
| }); | ||
| apply_finalize_headers(&settings, geo_info.as_ref(), &mut response); |
There was a problem hiding this comment.
🔧 wrench — Entry-point geo lookup overrides the middleware's 401 geo-skip in production.
FinalizeResponseMiddleware::handle correctly skips geo for UNAUTHORIZED and inserts X-Geo-Info-Available: false (middleware.rs:78-87); the dispatch_auth_rejected_401_carries_finalize_headers test verifies it in isolation.
But the entry-point apply_finalize_headers here runs an unconditional FastlyPlatformGeo.lookup(client_ip). With a real client IP in production, geo_info is Some(...), and apply_finalize_headers overwrites X-Geo-Info-Available: false with full geo headers (X-Geo-Country, X-Geo-City, …). legacy_main checks the status before geo (main.rs:162-172); the EdgeZero entry-point doesn't.
The unit test passes only because it bypasses main() and lookup(None) returns None.
Fix:
let geo_info = if response.status() == StatusCode::UNAUTHORIZED {
None
} else {
FastlyPlatformGeo.lookup(client_ip).unwrap_or_else(|e| {
log::warn!("entry-point geo lookup failed: {e}");
None
})
};| .get("/first-party/click", fp_click_handler) | ||
| .get("/first-party/sign", fp_sign_get_handler) | ||
| .post("/first-party/sign", fp_sign_post_handler) | ||
| .post("/first-party/proxy-rebuild", fp_rebuild_handler); |
There was a problem hiding this comment.
🔧 wrench — Non-GET/POST methods on registered named-route paths regress to 405.
Named routes register a single method (.get("/first-party/proxy", …), .post("/auction", …), etc.). The /{*rest} catch-all on the next block registers all 7 publisher_fallback methods, but matchit prefers exact path matches over wildcards — confirmed by your own dispatch_unregistered_method_returns_405_at_router_level test, which produces 405 even with the catch-all registered.
Therefore HEAD /first-party/proxy, OPTIONS /auction, PUT /admin/keys/rotate, etc. return 405 in EdgeZero. Legacy route_request falls through to _ => handle_publisher_request for every non-matching (method, path) pair (main.rs:256-275), proxying to the publisher origin.
This breaks cache-validation HEADs, CORS preflight OPTIONS, and any non-GET/POST traffic on a registered path. The existing 405 test only covers TRACE on /, which doesn't exercise this case (TRACE isn't in the catch-all method list either).
Fix options:
- (a) Register all 7 publisher_fallback methods on each named-route path, with non-matching methods routed to
fallback_handler. - (b) Drop the named routes and dispatch by method+path inside
fallback_handler(closest to legacy semantics). - (c) Use the catch-all only and prefix-check inside the handler (brittle).
Plus a regression test asserting HEAD /first-party/proxy returns a publisher-origin response rather than 405.
| // carry TS/geo headers, matching the legacy path's all-methods guarantee. | ||
| // For requests that ran through FinalizeResponseMiddleware, this is | ||
| // idempotent — header::insert overwrites with the same values. | ||
| if let Ok(settings) = get_settings() { |
There was a problem hiding this comment.
🔧 wrench — if let Ok(settings) = get_settings() silently drops TS headers on settings-load failure.
If get_settings() returns Err here, the response goes out without TS headers. legacy_main fails fast at build_state() (main.rs:138-143) and returns an error response. In practice settings load at Wasm startup so re-parsing is unlikely to fail mid-request, but the silent skip is fragile and inconsistent with legacy fail-fast semantics.
Fix: at minimum log a warning on the error branch. Better: cache the Arc<Settings> parsed during build_state() (the TrustedServerApp already has it) and reuse it here — threading it through dispatch_with_config request extensions avoids both the silent failure mode and the redundant parse.
| temp-env = "0.3.6" | ||
| tokio = { version = "1.49", features = ["sync", "macros", "io-util", "rt", "time"] } | ||
| toml = "1.0" | ||
| tokio-test = "0.4" |
There was a problem hiding this comment.
🔧 wrench — Undisclosed tokio-test = "0.4" workspace dep is unused.
Not referenced anywhere in the workspace and isn't even resolved into Cargo.lock. Either remove or justify in the PR description.
While here: the PR description claims edgezero deps are pinned to branch=main, but the diff actually pins to a 40-char SHA 38198f9839b70aef03ab971ae5876982773fc2a1 (good — deterministic builds). Update the description to match the diff.
| [local_server.config_stores.trusted_server_config.contents] | ||
| # "true" / "1" enable the EdgeZero path. Missing, unreadable, or | ||
| # any other value falls back to the legacy entry point. | ||
| edgezero_enabled = "true" |
There was a problem hiding this comment.
🌱 seedling — Local dev defaults to EdgeZero.
With the regressions in findings 1–3, every fastly compute serve reproduces them by default. Recommend edgezero_enabled = "false" in fastly.toml until the path reaches functional parity, then flip the default in a follow-up PR. Previously flagged.
| if is_edgezero_enabled().unwrap_or_else(|e| { | ||
| log::warn!("failed to read edgezero_enabled flag, falling back to legacy path: {e}"); | ||
| false | ||
| }) { |
There was a problem hiding this comment.
👍 praise — Defensive default for the edgezero_enabled flag read.
unwrap_or_else(|e| { log::warn!(...); false }) is exactly the right safe-fallback shape: store-unavailable or key-missing routes through the legacy path with observable logging, so the new path can never be silently engaged on a flag-read error.
| fn parse_edgezero_flag(value: &str) -> bool { | ||
| let v = value.trim(); | ||
| v.eq_ignore_ascii_case("true") || v == "1" | ||
| } |
There was a problem hiding this comment.
👍 praise — Case-insensitive + trim-tolerant flag parsing with full test coverage (tests mod at line 351-380). Small detail that prevents annoying operator errors.
| Some("operator-override"), | ||
| "should override the managed geo header with the operator-configured value" | ||
| ); | ||
| } |
There was a problem hiding this comment.
👍 praise — apply_finalize_headers precedence test.
operator_response_headers_override_earlier_headers documents the precedence contract in code (operator headers can intentionally override managed X-Geo-* / X-TS-* headers). Solid invariant captured as an assertion, not just a comment.
| Some("false"), | ||
| "FinalizeResponseMiddleware must run even for auth-rejected responses" | ||
| ); | ||
| } |
There was a problem hiding this comment.
👍 praise — End-to-end dispatch test that exercises the real router.
dispatch_auth_rejected_401_carries_finalize_headers is exactly the right shape: build the actual RouterService via TrustedServerApp::routes() and assert middleware behavior on a real request. Reusable pattern for the missing tests on the consent-store and HEAD-on-named-path regressions.
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!)