Remove fastly dependency from trusted-server-core (PR 15)#635
Remove fastly dependency from trusted-server-core (PR 15)#635prk-Jr wants to merge 20 commits intofeature/edgezero-pr14-entry-point-dual-pathfrom
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)
| url, | ||
| integration, | ||
| true, | ||
| std::time::Duration::from_secs(15), |
There was a problem hiding this comment.
🏕 camp site — Duration::from_secs(15) is inlined here even though crate::backend::DEFAULT_FIRST_BYTE_TIMEOUT already exists for exactly this purpose. The adapter's backend.rs:37 duplicates the same 15s again.
Suggested fix:
use crate::backend::DEFAULT_FIRST_BYTE_TIMEOUT;
// ...
.ensure(&integration_backend_spec(
url,
integration,
true,
DEFAULT_FIRST_BYTE_TIMEOUT,
)?)While here, ensure_integration_backend is essentially ensure_integration_backend_with_timeout(services, url, integration, DEFAULT_FIRST_BYTE_TIMEOUT) — the two helpers could collapse to one wrapper.
| } | ||
|
|
||
| /// Default first-byte timeout for backends (15 seconds). | ||
| pub(crate) const DEFAULT_FIRST_BYTE_TIMEOUT: Duration = Duration::from_secs(15); |
There was a problem hiding this comment.
🏕 camp site — core/src/backend.rs is now a 3-line stub holding a single constant. The PR's stated goal is to make core platform-agnostic; this leftover scaffolding could either move into crate::platform (more honestly placed alongside PlatformBackendSpec), or be inlined at its 2 call sites in proxy.rs / platform/test_support.rs. Either is mild architectural cleanup that finishes the file move.
| trusted-server-core = { workspace = true } | ||
| url = { workspace = true } | ||
| urlencoding = { workspace = true } | ||
| trusted-server-js = { path = "../js" } |
There was a problem hiding this comment.
🌱 seedling — trusted-server-js = { path = "../js" } is in the adapter's [dependencies] but never imported by adapter source. The adapter calls handle_tsjs_dynamic from trusted_server_core, which already pulls trusted-server-js transitively. Likely a leftover from PR15's adapter-side surgery. Removing this line should be a no-op for build/test and trims the dep graph.
| }) | ||
| } | ||
|
|
||
| /// Compute the deterministic backend name for a URL without registering a backend. |
There was a problem hiding this comment.
⛏ nitpick — "Compute the deterministic backend name for a URL without registering a backend." In a platform-agnostic core, an unqualified "backend" could mislead readers from non-Fastly adapters. Consider "platform backend" or referencing [PlatformBackend::predict_name] directly.
3 participants
Summary
fastlycrate imports fromtrusted-server-core, making it fully platform-agnostic — no Fastly SDK types leak into the shared libraryConsentKvOpstrait in core so the consent pipeline stays synchronous while abstracting away the Fastly KV implementation;FastlyConsentKvStorein the adapter implements the traitcompat,backend,geo_from_fastly, config/secret stores) totrusted-server-adapter-fastlywhere it belongsChanges
crates/trusted-server-core/src/compat.rscrates/trusted-server-core/src/storage/FastlyConfigStore/FastlySecretStoreremoved (callers already on platform traits)crates/trusted-server-core/src/geo.rsgeo_from_fastly; moved to adapterplatform.rscrates/trusted-server-core/src/consent/kv.rsfastly::kv_store::KVStorewith new syncConsentKvOpstrait; fingerprint stored in bodyfpfield instead of Fastly metadatacrates/trusted-server-core/src/consent/mod.rskv_ops: Option<&dyn ConsentKvOps>throughConsentPipelineInputcrates/trusted-server-core/src/integrations/mod.rsensure_integration_backend_with_timeoutandpredict_backend_name_for_urlhelperscrates/trusted-server-core/src/integrations/{aps,adserver_mock,prebid}.rsBackendConfigto platform helperscrates/trusted-server-core/src/publisher.rskv_ops: Option<&dyn ConsentKvOps>instead of touching Fastly KV directlycrates/trusted-server-core/Cargo.tomlfastlydep; movedtokiotodev-dependencies(test-only)crates/trusted-server-adapter-fastly/src/compat.rscrates/trusted-server-adapter-fastly/src/backend.rsbackend_name_for_urlcrates/trusted-server-adapter-fastly/src/platform.rsgeo_from_fastly; addedFastlyConsentKvStoreimplementingConsentKvOpscrates/trusted-server-adapter-fastly/src/app.rsFastlyConsentKvStoreintoConsentPipelineInputcrates/trusted-server-adapter-fastly/src/main.rsFastlyConsentKvStorefor legacy path; removesuse trusted_server_core::compatcrates/trusted-server-adapter-fastly/Cargo.tomlurldep (needed by relocatedbackend.rs)Closes
Closes #496
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 formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)