Complete removal of the legacy consent KV path - #903
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)
| /// returns `KvError::Validation` rather than silently writing a non-expiring | ||
| /// record. Callers of the generic platform KV interface must handle that | ||
| /// capability difference explicitly. |
There was a problem hiding this comment.
♻️ refactor — This sentence was rewritten in this PR but kept the wrong variant. The pinned edgezero v0.0.4 (9e661ae) Spin KV store returns KvError::Unsupported { operation: "put_bytes_with_ttl" } for TTL writes (edgezero-adapter-spin/src/key_value_store.rs:138), and KvError::Unsupported's own doc comment names "TTL writes on Spin" as its motivating example.
KvError::Validation is a different condition entirely: KvHandle::put_bytes_with_ttl runs validate_key / validate_ttl / validate_value before delegating, and those are what produce Validation — for a malformed key, TTL, or value, on any backend. KvHandleAdapter itself plain-delegates, so it never originates either error.
The distinction matters to callers beyond documentation accuracy: edgezero maps KvError::Unsupported to HTTP 501, and Validation does not, so a caller branching on the error kind to detect the capability gap would be looking for the wrong variant.
| /// returns `KvError::Validation` rather than silently writing a non-expiring | |
| /// record. Callers of the generic platform KV interface must handle that | |
| /// capability difference explicitly. | |
| /// returns `KvError::Unsupported` (operation `put_bytes_with_ttl`) rather than | |
| /// silently writing a non-expiring record. `KvError::Validation` is a different | |
| /// condition: it comes from `KvHandle`'s key/TTL/value validators, not from the | |
| /// Spin capability gap. Callers of the generic platform KV interface must handle | |
| /// that difference explicitly. |
Verified in an isolated worktree at this head: cargo fmt --all -- --check, cargo clippy-spin-native, and cargo clippy-spin-wasm all clean. Worth checking rather than assuming, since doc_markdown is set to warn in [workspace.lints.clippy] and clippy runs with -D warnings.
|
|
||
| ### Migrating from `consent_store` | ||
|
|
||
| The legacy `[consent].consent_store` setting has been removed. Trusted Server uses a strict configuration schema, so TOML and JSON/app-config that still contain `consent_store` fail during configuration loading. Remove the field before upgrading; it is not accepted as an ignored or deprecated option. |
There was a problem hiding this comment.
♻️ refactor — "fail during configuration loading" reads like a scoped, consent-feature degradation, but the actual failure is total. Tracing it: Settings::from_json_value returns TrustedServerError::Configuration, whose status_code() is INTERNAL_SERVER_ERROR (crates/trusted-server-core/src/error.rs:120); that propagates out of build_state, which hands off to startup_error_router; and that router registers both / and /{*rest} across all seven publisher_fallback_methods() (crates/trusted-server-adapter-fastly/src/app.rs:251). So a leftover consent_store field means every request on every path returns 500 until the field is removed — not a degraded consent path.
That is worth stating plainly, because it changes how an operator sequences the upgrade. It also makes the pre-flight worth naming: ts config validate deserializes through the same strict Settings path (TrustedServerAppConfig::deserialize delegates to Settings::deserialize), so it catches a stale field before ts config push ever publishes the blob.
| The legacy `[consent].consent_store` setting has been removed. Trusted Server uses a strict configuration schema, so TOML and JSON/app-config that still contain `consent_store` fail during configuration loading. Remove the field before upgrading; it is not accepted as an ignored or deprecated option. | |
| The legacy `[consent].consent_store` setting has been removed. Trusted Server uses a strict configuration schema, so TOML and JSON/app-config that still contain `consent_store` fail during configuration loading. Remove the field before upgrading; it is not accepted as an ignored or deprecated option. Configuration loading happens at startup, so a stale field is not a partial degradation: the service answers every request with an HTTP 500 startup error until the field is removed. Run `ts config validate` before `ts config push` to catch it ahead of deployment. |
Verified with the repo-pinned Prettier 3.8.1 --check; the single-line shape is required by proseWrap: "preserve" in docs/.prettierrc.
| If EC sync returns `kv_unavailable` or identify responses are degraded, first check that the identity store is present and linked to the active version. Legacy partner/consent KV bindings can be removed once no deployment-specific tooling depends on them. | ||
| If EC sync returns `kv_unavailable` or identify responses are degraded, first check that the identity store is present and linked to the active version. | ||
|
|
||
| Before upgrading a deployment that used the legacy consent store, remove `[consent].consent_store` from TOML or JSON/app-config; strict configuration loading rejects the removed field. Remove its local Viceroy fixture and active Fastly resource link as well. The old records are not read or migrated into `ec.ec_store` and must not be copied there. You may retain the old store unchanged for a defined rollback window, then delete the store and its records. |
There was a problem hiding this comment.
♻️ refactor — This paragraph recommends retaining the old store for a rollback window, which is reasonable. Worth making explicit that nothing cleans it up in the meantime: during that window the store still holds consent records keyed by EC ID, and no code path deletes them when a user withdraws consent. delete_consent_from_kv existed for exactly that purpose but was never wired to a caller (I checked the base commit as well as this head), and this PR removes it along with the rest of the module. So withdrawal writes its tombstone to ec.ec_store and the retained legacy records are simply untouched.
Nothing in this release reads those records, so this is not a live exposure, and the property predates the PR. But since this is the paragraph telling operators to keep the store around, one sentence bounding the window and warning against re-pointing a service at it seems worth the space in a privacy-focused product.
| Before upgrading a deployment that used the legacy consent store, remove `[consent].consent_store` from TOML or JSON/app-config; strict configuration loading rejects the removed field. Remove its local Viceroy fixture and active Fastly resource link as well. The old records are not read or migrated into `ec.ec_store` and must not be copied there. You may retain the old store unchanged for a defined rollback window, then delete the store and its records. | |
| Before upgrading a deployment that used the legacy consent store, remove `[consent].consent_store` from TOML or JSON/app-config; strict configuration loading rejects the removed field. Remove its local Viceroy fixture and active Fastly resource link as well. The old records are not read or migrated into `ec.ec_store` and must not be copied there. You may retain the old store unchanged for a defined rollback window, then delete the store and its records. Edge Cookie withdrawal does not delete anything from the retained legacy store, so keep the window short and do not re-point a service at it. |
Verified with the repo-pinned Prettier 3.8.1 --check.
4bd5bcd to
9ea1f1c
Compare
9ea1f1c to
aee5bf6
Compare
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.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!)