Skip to content

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

Open
prk-Jr wants to merge 17 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 17 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.

Comment thread crates/trusted-server-adapter-fastly/src/main.rs
Comment thread crates/trusted-server-adapter-fastly/src/app.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/app.rs
Comment thread crates/trusted-server-adapter-fastly/src/middleware.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/middleware.rs Outdated
Comment thread fastly.toml
Comment thread crates/trusted-server-adapter-fastly/src/app.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/app.rs
Comment thread crates/trusted-server-adapter-fastly/src/main.rs
Comment thread crates/trusted-server-adapter-fastly/src/middleware.rs
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 Outdated
Comment thread crates/trusted-server-adapter-fastly/src/main.rs
prk-Jr added 2 commits April 16, 2026 13:31
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