Skip to content

Add dual-path EdgeZero entry point with feature flag (PR 14)#628

Open
prk-Jr wants to merge 15 commits intofeature/edgezero-pr13-integration-provider-type-migrationfrom
feature/edgezero-pr14-entry-point-dual-path
Open

Add dual-path EdgeZero entry point with feature flag (PR 14)#628
prk-Jr wants to merge 15 commits intofeature/edgezero-pr13-integration-provider-type-migrationfrom
feature/edgezero-pr14-entry-point-dual-path

Conversation

@prk-Jr
Copy link
Copy Markdown
Collaborator

@prk-Jr prk-Jr commented Apr 13, 2026

Summary

  • Adds a feature-flagged dual-path entry point: requests are dispatched through the new EdgeZero TrustedServerApp or the preserved legacy_main based on edgezero_enabled in the trusted_server_config Fastly ConfigStore, with false as the safe default on any read failure
  • Implements TrustedServerApp via edgezero_core::app::Hooks with all routes, plus FinalizeResponseMiddleware and AuthMiddleware — matching legacy routing semantics without the compat conversion layer
  • Upgrades edgezero workspace dependencies from a pinned revision to branch=main and uses dispatch_with_config (non-deprecated) to avoid the double set_logger panic that run_app/run_app_with_logging would cause

Changes

File Change
Cargo.toml Pin all four edgezero deps to branch=main; bump toml to "1.1"
Cargo.lock Updated for edgezero and toml version changes
crates/trusted-server-adapter-fastly/src/main.rs Add is_edgezero_enabled(), feature-flag dispatch block, extract legacy_main(), use dispatch_with_config
crates/trusted-server-adapter-fastly/src/app.rs TrustedServerApp implementing Hooks with all 14 routes; explicit GET / and POST / to cover matchit root path gap
crates/trusted-server-adapter-fastly/src/middleware.rs New file: FinalizeResponseMiddleware and AuthMiddleware with golden header-precedence test
fastly.toml Add trusted_server_config config store for local dev with edgezero_enabled = "true"
crates/trusted-server-core/src/auction/orchestrator.rs rustfmt: remove blank line
crates/trusted-server-core/src/integrations/google_tag_manager.rs rustfmt: reorder imports, reformat long use lists
crates/trusted-server-core/src/integrations/lockr.rs rustfmt: inline trailing-brace onto condition
crates/trusted-server-core/src/integrations/permutive.rs rustfmt: inline trailing-brace onto condition
crates/trusted-server-core/src/integrations/prebid.rs rustfmt: reorder imports
crates/trusted-server-core/src/integrations/testlight.rs rustfmt: reformat long use list
.claude/settings.json Remove trailing comma in allowed-tools array

Closes

Closes #495

Test plan

  • cargo test --workspace
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • JS tests: cd crates/js/lib && npx vitest run
  • JS format: cd crates/js/lib && npm run format
  • Docs format: cd docs && npm run format
  • Manual testing via fastly compute serve — EdgeZero path routes all named routes and GET / correctly; legacy path reachable by setting edgezero_enabled = "false"

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses log macros (not println!)
  • New code has tests
  • No secrets or credentials committed

prk-Jr added 14 commits April 13, 2026 11:15
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 prk-Jr marked this pull request as draft April 13, 2026 14:20
@prk-Jr prk-Jr self-assigned this Apr 13, 2026
@prk-Jr prk-Jr changed the title Add feature-flagged dual-path entry point for EdgeZero migration (PR 14) Add dual-path EdgeZero entry point with feature flag (PR 14) Apr 13, 2026
@prk-Jr prk-Jr linked an issue Apr 13, 2026 that may be closed by this pull request
@aram356 aram356 linked an issue Apr 13, 2026 that may be closed by this pull request
- 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 prk-Jr marked this pull request as ready for review April 13, 2026 16:43
@prk-Jr prk-Jr requested review from ChristianPavilonis and aram356 and removed request for aram356 April 13, 2026 16:43
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

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

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-SSL before routing; EdgeZero hands the raw req to dispatch_with_config. With edgezero_enabled = "true" as the local-dev default, this is the default path. (main.rs:95)
  • build_state() panics on misconfigexpect("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)
  • AuthMiddleware flattens Report<TrustedServerError> into EdgeError::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 to branch = "main" — non-deterministic builds; supply-chain path into prod via a moving upstream branch. Pin to a specific rev or fork tag. (Cargo.toml:59-62)

Non-blocking

🤔 thinking

  • TLS metadata dropped on EdgeZero pathtls_protocol / tls_cipher hardcoded to None; 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-local make_sync_handler / make_async_handler helpers would cut ~120 lines without harming auditability. (app.rs:175-301)
  • FinalizeResponseMiddleware hardcodes FastlyPlatformGeo — take Arc<dyn PlatformGeo> instead so Middleware::handle can be unit-tested end-to-end. (middleware.rs:68)
  • build_per_request_services duplicates platform::build_runtime_services — extract a shared helper that takes ClientInfo. (app.rs:111-127)

🌱 seedling

  • fastly.toml flips local dev default to EdgeZero — combined with the blockers above, every fastly compute serve now exposes them. Consider defaulting to "false" until the blockers land. (fastly.toml:52)

⛏ nitpick

  • AppState fields can be private (not pub(crate)). (app.rs:62-66)
  • Root-route pairs clone closures four times — upstream RouterService::get_many would help. (app.rs:374-377)

📝 note

  • The dispatch_with_config comment explaining the set_logger panic is excellent "why, not what" documentation. (main.rs:91-94)

👍 praise

  • operator_response_headers_override_earlier_headers codifies 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")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔧 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");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔧 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:

  1. Memoize the state outside routes() and install a trivial "config error" handler on failure.
  2. Have each handler closure check a pre-built Result<Arc<AppState>, Report<TrustedServerError>> and return http_error(&report) on Err.

// ---------------------------------------------------------------------------

/// Application state built once at startup and shared across all requests.
pub struct AppState {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔧 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)]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔧 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}"
))));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔧 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.)

Comment thread fastly.toml

[local_server.config_stores]
[local_server.config_stores.trusted_server_config]
format = "inline-toml"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🌱 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>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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())
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

📝 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"
);
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 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.

Copy link
Copy Markdown
Collaborator

@ChristianPavilonis ChristianPavilonis left a comment

Choose a reason for hiding this comment

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

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_header drops multi-valued headers: edge_request_to_fastly in platform.rs:187 uses set_header instead of append_header, silently dropping duplicate headers. Pre-existing pattern (also in compat::to_fastly_request), but the EdgeZero path creates a new copy of the same bug.

🌱 seedling

  • parse_edgezero_flag is case-sensitive: "TRUE" and "True" silently fall through to legacy path. Consider eq_ignore_ascii_case or logging unrecognized values.

📝 note (cross-cutting, from earlier PRs)

  • Stale doc comment in platform/mod.rs:31: References fastly::Body in publisher.rs, but PR 11 already migrated to EdgeBody.

♻️ refactor (cross-cutting, from earlier PRs)

  • Duplicated body_as_reader helper: Identical function in proxy.rs:24 and publisher.rs:23. Extract to shared utility.

⛏ nitpick (cross-cutting)

  • Management API client re-created per write: Each put/delete in platform.rs constructs a new FastlyManagementApiClient. Fine for current usage, noted for future batch writes.

📌 out of scope

  • compat.rs in core depends on fastly types: Already tracked as PR 15 removal target.

CI Status

  • browser integration tests: PASS
  • integration tests: PASS
  • prepare integration artifacts: PASS

Comment thread crates/trusted-server-adapter-fastly/src/app.rs
Comment thread crates/trusted-server-adapter-fastly/src/app.rs
Comment thread crates/trusted-server-adapter-fastly/src/middleware.rs
///
/// 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 {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🌱 seedlingparse_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"
}

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.

Fastly entry point switch (dual-path with flag)

3 participants