Remove fastly dependency from trusted-server-core (PR 15)#635
Conversation
BackendConfig uses fastly::backend::Backend directly, making it incompatible with the platform-portability goal. This commit: - Copies backend.rs verbatim into trusted-server-adapter-fastly, updating the one crate-internal import path - Adds url dependency to the adapter Cargo.toml - Rewires platform.rs and management_api.rs to use crate::backend - Removes pub mod backend from trusted-server-core/lib.rs - Migrates aps.rs, adserver_mock.rs, and prebid.rs off the deleted BackendConfig, using context.services.backend().ensure() for registration and a new predict_backend_name_for_url free function in integrations/mod.rs for stateless name prediction cargo check --workspace passes with zero errors.
All callers were migrated to predict_backend_name_for_url in core's integrations module. Remove the now-unused method and update the parse_origin doc comment that referenced it.
Remove crates/trusted-server-core/src/storage/ entirely (config_store.rs, secret_store.rs, mod.rs). All call sites migrated to PlatformConfigStore / PlatformSecretStore traits. Also drop the now-broken intra-doc links in trusted-server-adapter-fastly/src/platform.rs that referenced the deleted types.
Replace direct fastly::kv_store::KVStore usage in consent/kv.rs with a platform-neutral ConsentKvOps trait. The trait has four sync methods (load_entry, save_entry_with_ttl, fingerprint_unchanged, delete_entry) keeping the consent pipeline synchronous. - consent/kv.rs: add ConsentKvOps trait; replace load_consent_from_kv / save_consent_to_kv / delete_consent_from_kv with load_consent / save_consent (trait-based); remove open_store / fingerprint_unchanged private fns; drop ConsentKvMetadata / metadata_from_context (metadata API was Fastly-specific; fingerprint now lives in the body fp field); add StubKvOps + integration tests - consent/mod.rs: add kv_ops field to ConsentPipelineInput; update try_kv_fallback and try_kv_write to consume kv_ops instead of store_name; update all struct literal sites - publisher.rs: add kv_ops: Option<&dyn ConsentKvOps> parameter to handle_publisher_request; wire it into ConsentPipelineInput and use it for the SSC-revocation delete - adapter/platform.rs: add FastlyConsentKvStore wrapping fastly::kv_store::KVStore implementing ConsentKvOps via the sync API - adapter/app.rs + main.rs: open FastlyConsentKvStore from settings.consent.consent_store and pass as kv_ops to handle_publisher_request
Remove `fingerprint_unchanged` from `ConsentKvOps` trait so the common case (consent unchanged) costs one KV read instead of two. `save_consent` now loads the entry once and compares the fingerprint inline. Extract `open_consent_kv` helper in `app.rs` to deduplicate the two identical KV-open blocks. Replace bare `unwrap()` with descriptive `expect` messages in consent KV tests. Run `cargo fmt --all` to fix all whitespace issues.
Resolved five conflicts: - Deleted compat.rs (PR15 goal: remove Fastly from core) - integrations/mod.rs: kept both PR15's ensure_integration_backend_with_timeout / predict_backend_name_for_url and PR14's INTEGRATION_MAX_BODY_BYTES constant - integrations/adserver_mock.rs, aps.rs, prebid.rs: merged collect_body import (PR14) with ensure_integration_backend_with_timeout / predict_backend_name_for_url imports (PR15) into single use statements
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This PR delivers its stated goal — trusted-server-core is now fully fastly-free and the consent pipeline is abstracted behind a minimal ConsentKvOps trait. Local verification passes (fmt, clippy -D warnings, cargo test --workspace, wasm32-wasip1 release build).
Three concerns merit changes before merge: a diagnostic regression in predict_backend_name_for_url, duplication of the security-adjacent SPOOFABLE_FORWARDED_HEADERS list, and a test-coverage regression in the relocated adapter compat.rs. The remaining items are non-blocking observations and follow-up candidates.
Note: this PR targets
feature/edgezero-pr14-entry-point-dual-path, notmain; review scope is the 31-file PR-15-specific delta (+2230/-2165).
Blocking
🔧 wrench
- Diagnostic regression in
predict_backend_name_for_url— inline comment atcrates/trusted-server-core/src/integrations/mod.rs:135 SPOOFABLE_FORWARDED_HEADERSduplicated across core and adapter — inline comment atcrates/trusted-server-adapter-fastly/src/compat.rs:18- Adapter
compat.rshas zero tests after relocation (14 security-relevant tests dropped) — inline comment atcrates/trusted-server-adapter-fastly/src/compat.rs:82
Non-blocking
🤔 thinking
- KV errors logged at
debuglevel as "lookup miss" — inline comment atcrates/trusted-server-adapter-fastly/src/platform.rs:420 - Backend-name compute logic duplicated between
predict_backend_name_for_url(core) andBackendConfig::compute_name(adapter) — inline comment atcrates/trusted-server-core/src/integrations/mod.rs:130
🌱 seedling
- Redundant KV read on fallback+save path — inline comment at
crates/trusted-server-core/src/consent/kv.rs:267 - Hardcoded
certificate_check: trueinensure_integration_backend(integrations/mod.rs:69) andensure_integration_backend_with_timeout(integrations/mod.rs:114). No regression vs. pre-PR behaviour, but the siblingpredict_backend_name_for_urldoes takecertificate_check: bool— asymmetric. If any integration ever needs a self-signed upstream (test/on-prem), these helpers would need to grow that parameter.
📌 out of scope
migration_guards.rsnot extended — migration_guards.rs:27-46 only covers 12 files and a narrow regex (Request|Response|http::Method|StatusCode|mime::APPLICATION_JSON). With core'sfastlydep now removed, this guard should either broaden to\bfastly::across all core files, be replaced by a Cargo-metadata dependency-presence test (more robust, no false negatives), or be deleted as structurally redundant. Follow-up issue.FastlyConsentKvStore::opensilent failure — inline comment atcrates/trusted-server-adapter-fastly/src/platform.rs:409
📝 note
- Stale "PR 15" forward references:
- adapter-fastly/src/app.rs:150: "will be removed when
legacy_mainis deleted in PR 15". This is PR 15 andlegacy_mainwas not deleted. - adapter-fastly/src/main.rs:113: "
compat.rswill be deleted in PR 15 once this legacy path is retired". This is PR 15;compat.rswas moved (not deleted) and the legacy path was not retired.
Update comments to reference the actual future PR number or drop the promise.
- adapter-fastly/src/app.rs:150: "will be removed when
⛏ nitpick
- Unnecessary
u16type suffix — inline comment atcrates/trusted-server-core/src/integrations/mod.rs:140
CI Status
- fmt: PASS
- clippy (
-D warnings): PASS - rust tests (
cargo test --workspace): PASS (800+ tests) - wasm32-wasip1 release build: PASS
- PR-reported checks (browser integration, integration tests, prepare integration artifacts): PASS
…mpat tests - predict_backend_name_for_url now returns Result<String, Report<TrustedServerError>> instead of Option<String>; call sites use inspect_err(...).ok() to preserve the Option<String> trait interface while logging the actual failure reason - Promote SPOOFABLE_FORWARDED_HEADERS to pub in http_util; replace inline copy in compat.rs with a use import — single source of truth for the strip list - Add sanitize_fastly_forwarded_headers_strips_spoofable test (all 4 entries + Host preserved) and to_fastly_response_with_streaming_body_produces_empty_body test (pins silent-drop behaviour) to compat.rs - Change Consent KV non-ItemNotFound error log from debug "lookup miss" to warn "lookup failed" for operational visibility - Drop stale "will be removed in PR 15" forward references from app.rs and main.rs - Drop unnecessary u16 type suffixes on port literal defaults
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
- Submitted reviewer-approved findings for consent/KV handling, backend naming consistency, and Fastly wiring.
- Verdict: REQUEST_CHANGES (includes a P1 finding).
Findings moved to review body
The following approved findings were included in the review body because GitHub could not resolve their requested inline locations in the current diff:
-
🤔 Consent bootstrap logic is now duplicated across auction and publisher paths (
crates/trusted-server-core/src/auction/endpoints.rs:61)
Both handlers now repeat the same sequence: parse cookies, generate/read synthetic ID, geo lookup with identical error handling, and build ConsentPipelineInput with kv_ops. This duplication is likely to drift as the consent pipeline evolves; this PR already had to touch both call sites just to thread the new abstraction through.
Suggestion: Extract a small shared helper that prepares request-scoped consent state once and returns the ConsentContext plus any reused intermediates such as synthetic_id, geo, and cookie_jar. -
⛏ Public docs/comments still lag the new kv_ops contract (
crates/trusted-server-core/src/auction/endpoints.rs:21)
The new kv_ops parameter materially changes how consent persistence is enabled, but the public docs still describe these handlers mostly in their pre-PR form, and one consent comment still says fallback requires consent_store rather than kv_ops.
Suggestion: Update the affected doc comments to explain that consent KV fallback/write-through occurs only when a concrete kv_ops implementation is supplied, and adjust the stale internal comment in consent/mod.rs.
|
Fixed the two findings GitHub moved into the review body in 3a53f33.
Fresh verification on this commit:
|
…ro-pr15-remove-fastly-core
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Approving this PR. The main refactor looks solid: trusted-server-core is decoupled from Fastly and the adapter now owns the Fastly-specific backend, KV, and compatibility wiring.
I left 2 non-blocking follow-up comments inline for logging/test coverage.
Incorporates the round-3 review fixes from PR14 (E2E tests, finalize-header coverage for unregistered methods, allowed_domains documentation) and the broader PR14 refactoring (EC ID rename from synthetic_id, kv_ops removal from handle_auction, runtime_services_for_consent_route consent KV approach, async tokio tests in proxy/orchestrator, PublisherResponse streaming). Conflict resolutions: - Take PR14's refactored core files (endpoints, orchestrator, consent, publisher, proxy, platform/mod, testlight) - Keep PR15's deletion of core/compat.rs and storage/mod.rs (Fastly code stays in adapter, not core) - Add from_fastly_response to adapter compat.rs (needed by EdgeZero path for entry-point finalize-header wrap introduced in PR14) - Remove obsolete FastlyConsentKvStore / open_consent_kv / ConsentKvOps references from platform.rs and app.rs (trait removed in PR14) - Fix orchestrator backend_name call to pass services (PR15 trait signature) - Add minimal backend.rs and storage/mod.rs stubs to core for DEFAULT_FIRST_BYTE_TIMEOUT and storage::kv_store module path - Add tokio as dev-dependency to trusted-server-core for async tests
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-review of PR 15 after the most recent commits (Apr 27). The core refactor goal is achieved — trusted-server-core no longer imports fastly, and the migration guard test (migration_guards.rs) durably enforces the invariant. Build, clippy, fmt, and the full Rust test suite (878 tests) pass locally; GitHub CI is green.
However, the merge with PR14 removed the ConsentKvOps / FastlyConsentKvStore abstraction without replacing the wiring on the EdgeZero entry path, leaving a privacy-sensitive gap: when edgezero_enabled = true and consent_store is configured, consent revocation deletes silently fail. The PR description still describes the pre-merge design and is now misleading.
Requesting changes to address (or explicitly accept and document) the EdgeZero consent-KV gap and to update the PR description before merge.
Blocking
❓ question
Consent KV is not opened on the EdgeZero entry path — crates/trusted-server-adapter-fastly/src/app.rs:92
build_state() constructs kv_store = Arc::new(UnavailableKvStore) unconditionally, and build_per_request_services (lines 111–127) hands that same store to every handler. The runtime_services_for_consent_route helper in crates/trusted-server-adapter-fastly/src/main.rs:307-323 (which actually opens the configured consent.consent_store) is only called from the legacy route_request.
Consequence: when edgezero_enabled = true and [consent] consent_store = "..." is configured, every services.kv_store() call in core returns KvError::Unavailable. In particular, the revocation delete in crates/trusted-server-core/src/publisher.rs:765 silently fails (Failed to delete consent KV entry … Unavailable) — the EC cookie expires, but the persisted consent data is not removed. KV-backed consent fallback also never engages on the EdgeZero path. This is a privacy/compliance concern (right-to-erasure).
This appears to be a regression from the merge resolution that removed FastlyConsentKvStore / open_consent_kv after merging PR14. Either (a) the EdgeZero path needs an equivalent of runtime_services_for_consent_route (open the consent store inside build_per_request_services or wrap the auction/publisher handlers), or (b) the EdgeZero path should fail loudly at startup when consent.consent_store is configured but cannot be opened.
Was this intentional, or did it slip through the merge?
🔧 wrench
PR description is stale and contradicts the merged code — The body still claims:
- "Introduces a sync
ConsentKvOpstrait in core" - "
FastlyConsentKvStorein the adapter implements the trait" - "Threads
kv_ops: Option<&dyn ConsentKvOps>throughConsentPipelineInput"
None of these exist after the PR14 merge — the actual code uses PlatformKvStore directly via RuntimeServices, and the consent KV is opened only on the legacy path. The "Changes" table for consent/kv.rs, consent/mod.rs, app.rs, main.rs, and platform.rs describes work that was reverted. Reviewers and git log consumers will be misled. Please update the PR description to reflect the post-PR14-merge reality before merging.
Non-blocking
🤔 thinking
UnavailableKvStore swallows configuration errors silently on the EdgeZero path — Combined with the question above, configuring consent_store = "missing-store" on the EdgeZero path produces no startup error and only per-request warn! lines. The legacy path correctly fails the auction/publisher routes with 503 (verified by route_tests.rs:184). Worth validating consent-store availability at startup so misconfiguration cannot silently disable revocation.
⛏ nitpick
Pre-existing: core is on edition = "2021" — CLAUDE.md mandates 2024 edition. Not introduced by this PR; flagged only for awareness/follow-up.
CI Status
- fmt: PASS (local + GitHub)
- clippy: PASS (local,
-D warnings) - rust tests: PASS (878 tests, local)
- GitHub CI: PASS (browser tests, integration tests, prepare integration artifacts)
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-review of PR 15 against HEAD 725ccdc2. Local verification passes (fmt, clippy -D warnings, 878 + 45 unit tests across the changed crates) and GitHub CI is green.
The core refactor goal is achieved — trusted-server-core no longer imports fastly and the migration guard test enforces the invariant. However, the previously flagged EdgeZero consent-KV regression (Apr 30 review) is still present at the same line, and the PR description still describes the pre-PR14-merge design that was reverted. Requesting changes to address (or explicitly accept and document) those before merge.
Blocking
🔧 wrench
-
EdgeZero path silently disables consent KV (privacy/compliance regression) —
crates/trusted-server-adapter-fastly/src/app.rs:92(line outside this PR's diff hunk, so flagging at body level).build_state()hardcodesArc::new(UnavailableKvStore), andbuild_per_request_services(lines 111–127) hands that exact store to every route. There is no analogue ofruntime_services_for_consent_route(main.rs:307-323) on this path.Concrete effect when
edgezero_enabled = trueAND[consent] consent_store = "..."is set:crates/trusted-server-core/src/publisher.rs:765callsdelete_consent_from_kv(services.kv_store(), cookie_ec_id), which hitsUnavailableKvStore.delete()→KvError::Unavailable.storage/kv_store.rs:323-332swallows the error (warn-log only). The EC cookie expires but the persisted consent is not removed — right-to-erasure broken.crates/trusted-server-core/src/auction/endpoints.rs:83-87andpublisher.rs:508-512passSome(services.kv_store())to the consent pipeline, but it's the unavailable store, so consent persistence and KV-backed consent fallback never engage.
Compare with the legacy path (correct):
route_tests.rs:184-260verifies that a missingconsent.consent_storeproduces a fail-closed 503 on/auctionand the publisher fallback viaroute_request. EdgeZero has no equivalent and silently degrades. Same blocking finding as the Apr 30 review, at the same line, still present at HEAD725ccdc2.Fix options:
- Open
consent.consent_storeinsidebuild_state()(or inbuild_per_request_services) usingplatform::open_kv_store, and either (a) fail startup if configured-but-unavailable or (b) wrap the consent-touching handlers (/auction, fallback) so they return 503 like the legacy path. - Mirror
runtime_services_for_consent_routeat the EdgeZero handler boundaries.
Either way, please add a counterpart of the existing
configured_missing_consent_store_only_breaks_consent_routestest that drivesTrustedServerApp::routes()so the regression cannot slip through another merge. -
PR description is stale and contradicts the merged code — the body still claims the PR introduces a sync
ConsentKvOpstrait, aFastlyConsentKvStoreadapter implementation, and threadskv_ops: Option<&dyn ConsentKvOps>throughConsentPipelineInput. None of those exist after the PR14 merge — the actual code usesPlatformKvStoredirectly viaRuntimeServices. The "Changes" table forconsent/kv.rs,consent/mod.rs,app.rs,main.rs, andplatform.rsdescribes work that was reverted. Please update the description to reflect the post-PR14-merge reality before merging so reviewers andgit logconsumers aren't misled.
Non-blocking
🤔 thinking
- No EdgeZero-side test for consent-store wiring —
route_tests.rs:184 configured_missing_consent_store_only_breaks_consent_routesonly exercises the legacyroute_request. There is no analogous test that constructsTrustedServerApp::routes()withconsent.consent_storeconfigured. Combined with the blocking finding above, the regression slipped through the PR14 merge with no failing test to catch it. - Both crates still on
edition = "2021"— CLAUDE.md mandates 2024 edition. Pre-existing across the repo, not introduced by this PR; flagged for awareness/follow-up.
📌 out of scope
UnavailableKvStoremakes consent-store misconfiguration invisible at startup — even after the blocking finding is addressed, configured-but-unopenable stores will only surface as per-requestwarn!lines unlessbuild_state()validates store availability at startup. Worth a follow-up to fail loudly whenconsent.consent_storeis set but cannot be opened.
CI Status
- fmt: PASS (local)
- clippy: PASS (local,
-D warnings) - rust tests: PASS (878 + 45 in changed crates)
- GitHub CI: PASS (browser tests, integration tests, prepare integration artifacts)
|
Addressed the body-level blocking findings from the Apr 30 and May 6 reviews in 50dc61f.
Fresh verification before commit:
|
3 participants
Summary
fastlycrate imports fromtrusted-server-core, keeping the shared crate platform-agnostic and enforcing that invariant withmigration_guards.rstrusted-server-adapter-fastlyRuntimeServices,PlatformBackendSpec,PlatformKvStore) for core request handling instead of Fastly SDK types/auctionand publisher fallback routes, matching the legacy path's fail-closed503behavior when the store is unavailableChanges
crates/trusted-server-core/src/compat.rscrates/trusted-server-core/src/storage/crates/trusted-server-core/src/geo.rscrates/trusted-server-core/src/storage/kv_store.rsPlatformKvStorerather than Fastly KV typescrates/trusted-server-core/src/consent/mod.rsConsentPipelineInputaccepts an optionalPlatformKvStorereference for KV fallback/write-throughcrates/trusted-server-core/src/platform/crates/trusted-server-core/src/integrations/mod.rscrates/trusted-server-core/src/integrations/{aps,adserver_mock,prebid}.rscrates/trusted-server-core/src/publisher.rsRuntimeServices/PlatformKvStorefor consent KV fallback and revocation deletioncrates/trusted-server-core/Cargo.tomlfastlydependency from corecrates/trusted-server-adapter-fastly/src/compat.rscrates/trusted-server-adapter-fastly/src/backend.rscrates/trusted-server-adapter-fastly/src/platform.rsopen_kv_storeforPlatformKvStorecrates/trusted-server-adapter-fastly/src/app.rscrates/trusted-server-adapter-fastly/src/main.rscrates/trusted-server-adapter-fastly/Cargo.tomlCloses
Closes #496
Test plan
cargo test -p trusted-server-adapter-fastly edgezero_missing_consent_store_breaks_only_consent_routescargo test -p trusted-server-adapter-fastly configured_missing_consent_store_only_breaks_consent_routescargo test -p trusted-server-core --libcargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspacecd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code; usesexpect("should ...")where neededlogmacros rather thanprintln!