Complete removal of the legacy consent KV path - #903
ChristianPavilonis wants to merge 2 commits into
Conversation
8a53ee1 to
4bd5bcd
Compare
|
@ChristianPavilonis please assign issue for this PR |
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Removes the legacy consent KV persistence path — config field, dead storage module, ConsentSource::KvStore, the Fastly consent-route store opener, and the local fixture — and replaces it with strict fail-fast config rejection plus operator migration guidance. I verified the central claim independently: at the base commit the sole production ConsentPipelineInput call site (crates/trusted-server-core/src/ec/mod.rs:298) already passed ec_id: None, kv_store: None, every Some(...) was a test, and delete_consent_from_kv had no callers at all. The path really was dead, so this is a pure-subtraction change with no live behavior removed.
A whole-tree residual sweep for consent_store, consent_kv, ConsentSource::KvStore, storage::kv_store, open_kv_store, and runtime_services_for_consent_route comes back clean — matches are limited to the strict-rejection tests, the migration docs, the new plan, and one spec already marked obsolete. Dropping the ConsentSource::KvStore variant carries no stored-record hazard (ConsentSource has no serde derives), no intra-doc link references a removed item, and the absence of Cargo.toml/Cargo.lock churn is correct since hex and sha2 remain widely used. The migration_guards.rs entry removal is required, not optional — include_str! on the deleted paths would fail to compile. The deleted edgezero_missing_consent_store_breaks_only_consent_routes test also asserted admin-401; that coverage survives at app.rs:1597 and app.rs:2286, so nothing of value was lost.
No blocking findings. Four non-blocking, three of them one-click suggestions.
3 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. All three were applied in an isolated worktree at this head and verified there:cargo fmt --all -- --check,cargo clippy-spin-native, andcargo clippy-spin-wasmclean for the Rust one; the repo-pinned Prettier 3.8.1--checkclean for the two Markdown ones; and a byte-exact before/after patch comparison confirmed verification did not mutate the tree beyond the reviewed bytes.
Non-blocking
♻️ refactor
- Spin TTL doc names the wrong
KvErrorvariant — see inline atcrates/trusted-server-adapter-spin/src/platform.rs:212 - Migration doc understates the blast radius and omits the pre-flight command — see inline at
docs/guide/configuration.md:505 - Retained legacy store is not covered by EC withdrawal deletion — see inline at
docs/guide/fastly.md:307
Cross-cutting / body-level findings
-
📌 Generic
RuntimeServicesKV slot now has zero production consumers — after this PR,RuntimeServices::kv_store(),kv_handle(), andwith_kv_store()have no production callers; only tests reach them. The plan's contract item 7 states that retaining this infrastructure is deliberate and out of scope, so this is a follow-up rather than a change request. Two consequences seem worth tracking. Cloudflare (crates/trusted-server-adapter-cloudflare/src/platform.rs:616) and Spin (crates/trusted-server-adapter-spin/src/platform.rs:726) still resolve a real KV binding on every request to populate a slot nothing reads. And Fastly permanently populates it withUnavailableKvStore(crates/trusted-server-adapter-fastly/src/app.rs:189), so the first future consumer of the generic slot will silently degrade on the production adapter rather than fail loudly. A tracked issue would keep that from being rediscovered the hard way. -
👍 Test-first ordering on a deliberately breaking config change — writing
settings_rejects_removed_consent_store_toml/_jsonand confirming they failed while the field still existed is exactly the right sequencing for a strict-schema removal; it proves the guard tests the removal rather than merely passing alongside it. Covering bothSettings::from_tomlandSettings::from_json_valuematters too, since the runtime blob path goes through the latter viaconfig_payload.rs:39. The residual audit also holds up under an independent sweep, and droppingruntime_services_for_consent_routeremoved an entire nesting level fromdispatch_fallbackfor free.
CI Status
- cargo fmt: PASS (this job also runs all 8 target-matched clippy invocations plus the CLI and OpenRTB-codegen clippy steps)
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- browser integration tests: PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- Analyze (rust) / CodeQL: not run (absent from
gh pr checksoutput for this PR)
9ea1f1c to
aee5bf6
Compare
aee5bf6 to
2c5ba81
Compare
33e8de8 to
240f0f8
Compare
| use crate::platform::{ | ||
| FastlyPlatformBackend, FastlyPlatformConfigStore, FastlyPlatformGeo, FastlyPlatformHttpClient, | ||
| FastlyPlatformSecretStore, UnavailableKvStore, open_kv_store, | ||
| FastlyPlatformSecretStore, UnavailableKvStore, |
There was a problem hiding this comment.
🤔 thinking — Re-verified at 240f0f8e2: with open_kv_store and runtime_services_for_consent_route gone, the generic RuntimeServices KV slot still has no production consumer anywhere in the workspace. The only remaining .kv_store() call is a test assertion in adapter-spin/src/platform.rs:1065, and with_kv_store has no non-test caller — every Fastly request now carries UnavailableKvStore in that slot.
The plan doc's contract item 7 scopes this out as platform infrastructure, which is a fair deferral. But I searched open and closed issues and found no follow-up filed, so nothing currently tracks it. A quick issue to either wire the slot to a real consumer or delete with_kv_store / kv_store() would keep it from ossifying as permanent dead plumbing. Not a merge blocker.
Summary
ec.ec_storeas the sole KV-backed EC identity/withdrawal store.consent_store, with explicit operator migration guidance.This PR is stacked on #902.
Changes
ConsentPipelineInput, delete the dead storage module, and removeConsentSource::KvStoreConsentConfig::consent_storeand add strict TOML/runtime-JSON rejection testsec.ec_storeMigration note
Deployments that still configure
[consent].consent_storemust remove the field before upgrading. Strict config loading intentionally rejects it. The old store may be retained unchanged for a rollback window, then unlinked and deleted; its records must not be copied intoec.ec_store.Before this change, Fastly returned 503 on auction, page-bids, and publisher routes when a configured legacy store could not be opened, even though consent code never read that store. After this removal, a stale
consent_storefield instead prevents normal startup, and Fastly's startup-error router returns 500 on registered user routes until the configuration is corrected.Closes
Closes #883
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serve— not runcargo test-cloudflare,cargo test-spin, parity,./scripts/test-cli.sh, all Cloudflare/Spin clippy targets, and all-features core rustdocChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)