Repository navigation
Extract native EC lifecycle and DynamoDB persistence - #1536
ChristianPavilonis wants to merge 20 commits into
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The native EC lifecycle extraction and the DynamoDB backend look correct to me. I checked the store against the EcKvStore contract: reads are strongly consistent, conditional writes report precondition failures as outcomes rather than errors, logically expired rows are hidden from lookup/list but still count for key_exists so withdrawal can overwrite them, and listing fails instead of returning a short result. The Axum behaviour changes (fail closed on an unreadable Cookie header, running integration request filters, /identify erroring without a selected store) now match Fastly. Approving; the four comments below are non-blocking.
Non-blocking
- 🤔 The Axum integration-route no-generation rule has no test (inline on
app.rs). - 🤔 CI never runs the
aws-dynamodb-ectests (inline onec_kv.rs). - 📝 The guide cites a Dockerfile and a Terraform example that only arrive in #1539 / #1540 (inline on the guide).
- ⛏
cookie_valueduplicates the core cookie helper and selects headers differently (inline onapp.rs).
Verification
- Local, Rust 1.98.1:
cargo test -p trusted-server-adapter-axum --features aws-dynamodb-ec --no-fail-fastpasses, exceptnative_acceptance::managed_ingress_ignores_direct_spoof_and_isolates_two_gateway_clients. That test binds a non-127.0.0.1 loopback alias, which macOS doesn't provide; the file is unchanged by this PR and passes in hosted Linux CI. - Mutation: changing Axum's
allow_generation: falsetotrueleaves the Axum lib,native_ec,routes,native_httpandstartuptests passing. - Not run: the ignored DynamoDB Local contract test,
scripts/test-native-ec-image.mjs, and anything against AWS.
CI Status
All 17 hosted checks pass on c568207d.
| kv: None, | ||
| ec_context: &mut ec_context, | ||
| kv: graph, | ||
| allow_generation: false, |
There was a problem hiding this comment.
🤔 thinking: Nothing protects this false. The guide says only a GET publisher document navigation may issue an identity, but if I change this to true, the Axum lib, native_ec, routes, native_http and startup tests all still pass. No core test passes allow_generation: false either. A later refactor could flip it, and consented browser navigations to /integrations/<id>/… would then start creating identities without any test failing.
Suggested fix: add a native_ec.rs case that sends a consented browser GET navigation (Sec-Fetch-Dest: document) to an integration proxy route. Assert that no ts-ec Set-Cookie is emitted and the store records no Add.
| } | ||
| } | ||
|
|
||
| #[cfg(all(test, feature = "aws-dynamodb-ec"))] |
There was a problem hiding this comment.
🤔 thinking: These tests never run in CI. The cargo test (axum native) job runs cargo test-axum, which doesn't enable aws-dynamodb-ec. clippy-axum uses --all-features, so the tests compile but don't execute. That covers the six SDK-expression tests in ec_kv_tests.rs and the feature-only cases in tests/ec_kv_configuration.rs (endpoint validation, missing binding). I ran them locally and they pass. Apart from the one marked ignored, they use an in-process local fixture and need no DynamoDB Local or AWS.
Suggested fix: add cargo test -p trusted-server-adapter-axum --features aws-dynamodb-ec to the Axum CI job. The DynamoDB Local test stays #[ignore] and opt-in.
| From the repository root, add the EC feature to the features your native image already needs: | ||
|
|
||
| ```sh | ||
| docker build --platform linux/amd64 \ |
There was a problem hiding this comment.
📝 note: At this revision the repository has no root Dockerfile (so this NATIVE_FEATURES build fails) and no deploy/examples/native-ec-dynamodb/ (cited at line 59). Both arrive later in the stack: the Dockerfile in #1539 and the Terraform example in #1540. Someone reading the guide from this unit hits a missing file. Consider naming those PRs here, or moving the build and Terraform sections to the units that add them.
| (signals, browser) | ||
| } | ||
|
|
||
| fn cookie_value(req: &Request, name: &str) -> Option<String> { |
There was a problem hiding this comment.
⛏ nitpick: This duplicates trusted_server_core::cookies::extract_cookie_value, which Fastly's equivalent helper mirrors, but selects headers differently. This version reads every Cookie header field. Core's EC jar (handle_request_cookies) and the core helper read only the first. With split Cookie fields, ts-eids/sharedid could come from a different header than the EC id. Using the core helper keeps Axum on the same rule as core and Fastly.
Rebuild the existing native EC feature above the tag-bound async graph. Keep live conditional rows separate from immutable config snapshots, build the shared router from prepared state, and retain the publisher response owner through buffering and exactly-once EC finalization, including handled errors. Preserve the current direct client/test seams, frame limits, trace authorization and protected Geo. Verify the real binary against pinned local DynamoDB, including stale CAS, expiry/withdrawal, lost acknowledgements and draining while storage waits. No image, live AWS or deployment qualification is implied.
There was a problem hiding this comment.
Superseded by a follow-up review with inline comments: #1536 (review)
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
This extracts the native request-local EC lifecycle and an optional DynamoDB EcKvStore for Axum. The core change is small (allow_generation on ProxyDispatchInput). The design and docs are careful, but the DynamoDB tests never run in CI, and two behaviour paths need a test or a fix.
1 of the inline comments below carries a one-click GitHub
suggestion. The rest describe the fix in prose because it touches lines outside the diff or other files.
Blocking
🔧 wrench
- Identify without a provider returns 503 (reads as an outage): see inline at
crates/trusted-server-adapter-axum/src/app.rs:693 allow_generation: falseis untested: see inline atcrates/trusted-server-adapter-axum/src/app.rs:388
Non-blocking
🤔 thinking
- Provider-less Axum now fails closed on malformed EC cookies and expires cookies: see inline at
crates/trusted-server-adapter-axum/src/app.rs:211
⛏ nitpick
- Unchecked
+inside thechecked_addchain: see inline atcrates/trusted-server-adapter-axum/src/ec_kv.rs:386
Cross-cutting / body-level findings
- 🔧 The DynamoDB provider tests never run in CI:
ec_kv.rs's test module is#[cfg(all(test, feature = "aws-dynamodb-ec"))], andtests/ec_kv_configuration.rsgates its endpoint-validation cases on the same feature. Thetest-axumjob runscargo test-axum(test -p trusted-server-adapter-axum) with no features.clippy-axumuses--all-features, so these tests are linted but never executed. They pass locally withcargo test -p trusted-server-adapter-axum --features aws-dynamodb-ec. Add that as a step in thetest-axumjob in.github/workflows/test.yml.
CI Status
- vitest: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo fmt: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- HTML EOF runtime (Fastly): PASS
- format-typescript: PASS
- format-docs: PASS
- ts dev lint domains (changed lines): PASS
- other checks (worker-deadlines spin/cloudflare, trace runtime and browser acceptance, release scripts, prepare integration artifacts, Fastly response diff script, docs-proposal scripts, CLAUDE.md symlink guard, CI Node pin guard): PASS
| cors_preflight_identify(&state.settings, &req) | ||
| } else { | ||
| let context = build_ec_context(&state, &services, &req)?; | ||
| let graph = state.identity_graph().ok_or_else(|| { |
There was a problem hiding this comment.
🔧 wrench — /_ts/api/v1/identify now returns 503 on a server without a selected provider. That is the default Axum dev server, and a 503 looks like a storage outage. TrustedServerError::KvStore maps to SERVICE_UNAVAILABLE, so a partner probing the default dev server sees an outage instead of "not supported here". The base branch did not register this route on Axum at all.
Proposed fix (apply manually; it needs a response helper that lives outside this hunk): when state.identity_graph() is None, return a not-supported response the way AdminEcNotSupported does (admin_ec_lookup_not_supported(), 501), and add a native_ec.rs/routes.rs case that asserts the status without a provider.
| kv: None, | ||
| ec_context: &mut ec_context, | ||
| kv: graph, | ||
| allow_generation: false, |
There was a problem hiding this comment.
🔧 wrench — allow_generation: false is the only thing that keeps native issuance off integration-proxy navigations, and no test covers it. native_ec.rs checks /static/tsjs=…, but that path returns before handle_proxy runs. Please add a case to missing_consent_unknown_geo_storage_failure_and_non_navigation_do_not_mint that registers a fixture integration route, sends a consented browser GET navigation to it, and asserts that no ts-ec cookie is set and no row is written. Apply manually: it needs a fixture integration in tests/native_ec.rs, which is outside this hunk.
| @@ -178,10 +209,89 @@ fn build_ec_context(state: &AppState, services: &RuntimeServices, req: &Request) | |||
| None | |||
| }); | |||
| EcContext::read_from_request_with_geo(&state.settings, req, services, geo_info.as_ref()) | |||
There was a problem hiding this comment.
🤔 thinking — On the default Axum server with no provider, this changes behaviour. Before, a malformed ts-ec cookie logged a warning and fell back to EcContext::default(). Now build_ec_context returns the error, NativeEcRequest::read propagates it with ?, and every publisher, auction or page-bids request with a bad cookie fails before dispatch. Also, ec_finalize_response now runs on every EC route even when graph is None, so cookie expiry on unusable or withdrawn consent applies to the plain dev server too. Failing closed may be intended. If so, please state it in docs/guide/native-ec-persistence.md (or the PR description) as a behaviour change for the provider-less Axum server, and consider a test that pins the malformed-cookie status.
| .body | ||
| .len() | ||
| .checked_add(write.metadata.len()) | ||
| .and_then(|n| n.checked_add(self.partition.len() + key.len() + 256)) |
There was a problem hiding this comment.
⛏ nitpick — The checked_add chain still does an unchecked self.partition.len() + key.len() + 256 inside it. In practice it cannot overflow, because both lengths are validated, but the chain should be checked all the way through.
| .and_then(|n| n.checked_add(self.partition.len() + key.len() + 256)) | |
| .and_then(|n| n.checked_add(self.partition.len())) | |
| .and_then(|n| n.checked_add(key.len())) | |
| .and_then(|n| n.checked_add(256)) |
Per the user, the RC line is forward-ported to main (DEC-6). Rechecking the proposed stack against DEC-3 and DEC-16: #1536 and #1537 are mostly Axum-only stores, store selection and wiring that EZI-2b, T6 and T21 replace, so A2 becomes a new pull request that takes their tests, and both close with #1535 once their code moves. The image and Terraform units restack on #1543, which publishes the image and shares .dockerignore with #1539. #1531 goes on #1506 for A1 and #1532 on #1531 for #1529.
|
CREATOR: From the server-side identity plan (#1559, Part of #1574 |
Summary
Draft baseline review unit. Not a current-main merge candidate, deployment approval or completed current-head CI claim.
Baseline and sequence
Head:
c568207dba73f022b723f9bae7ac4a130603b744. Base:baseline/shared-async-ec-20261110.Historical main anchor:
2dd968aeebeb2cc0d5110115f64e0ad58e2e1d13. Original demo: TS2423a2ed plus separately preserved dirty inputs. Matching EdgeZero: draft431, exact revision83d7a6296d285e135c4ea70021b19e6c8dca7e9f.The user selected compatible-baseline cleanup. Current-main integration with EdgeZero462 and the authenticated-forwarder incompatibility are deferred. Preserve current-main PR1525/1526 ready and1527/1528 draft, with their existing bases. No excluded1499 or independent1506 implementation, relay/privacy experiment, new transport fallback, group/codec/phase design or operational deployment is included.
Changes
Closes
Refs #1530. This unit completes one part of the baseline extraction and does not close the overall tracker on its own.
Spec
Not a new feature. This is extraction of existing demo behavior and bounded build/caller corrections. Original native and EC storage design/review records remain in the baseline where applicable; they are not presented as approval of later combined source. No new forwarding, consent, timer or provider policy is designed here.
Test plan
Core3006 and native all-feature lifecycle/configuration suites pass; strict Core/Axum lint pass. Native static/integration routes retain their original generation restrictions. No AWS-account or fleet qualification is claimed.
The b67ffc6 baseline checkpoint has passing documented local gates: Fastly/default/reuse/native/Cloudflare/Spin suites, all adapter strict lint lanes, 17 cross-adapter parity cases, 1252 JS cases, CLI suites, formatting, clock/digest/proposal checks and changed-lines domain lint. Passing checks at121ebcc36 are reused only for unchanged relevant inputs; the final follow-up covers the CLI test-import and documentation-only corrections. These results are not a substitute for this PR's current-head hosted checks.
Checklist
Operational Terraform remains in trusted-server-deployer. Local checks and prototype tests do not qualify cloud access, fleet durability or a live publisher.
Hosted docs-build correction
The first hosted format-docs job caught an additional site-build gate that the original local formatter checkpoint had not run: a relative link from the native EC guide to a Terraform example outside the site. Native EC correction8b11dbad3 replaces it with the plain repository path; no dead-link check is disabled. That docs-only correction is carried through the existing upper PR identities. Native EC and final integration8985b111 both pass docs ESLint and VitePress build; the final docs formatter also passes. Runtime, dependency, image-build-context and test inputs are unchanged from b67ffc6, so their prior exact-input proofs are reused. Hosted current-head checks still control readiness.
Startup negative-probe correction
A hosted native EC case failed the no-listener probe while its child logged the expected startup validation failure. The old log has no socket endpoints, so the failure's exact subtype is not claimed confirmed. A separate controlled Linux reproduction demonstrates a successful TCP self-connection with no listening server. Native correction8490c691d distinguishes equal local/peer endpoints from a real listener; actual socket controls prove the self-connection is excluded and a real listener is still detected. Nonzero exit, expected diagnostic, secret-redaction and the original startup deadline remain required. This changes only the acceptance fixture, not production admission, transport or trust.
The earliest native unit and final9356c557 default/all-feature acceptance checks and strict lint pass, including both controls and the original failure case. Ignores remain explicit; earlier separately executed celld proof is not relabeled as a new ignored-case run. Correction is merged upward through existing PR identities without force-pushing or retargeting. Current-head hosted checks must pass; the old failure is retained rather than rerun-only waived. Runtime/API/dependency inputs remain unchanged. A fresh exact9356 producer build and real runtime smoke now pass: image
sha256:65a735d38e9dbc0573d0f6068a8b261bd202c43aea05085c2b2d6bccc91ebdbd, linux/amd64 UID/GID10001, including startup rejection and in-flight SIGTERM drain. Prior b67 image receipts remain separate; no image was pushed. All local parent runners are complete.Browser CI host compatibility
Preserved older browser failures show Playwright1.58.2 rejecting ubuntu-latest's new Ubuntu26.04 image before browser installation; later missing-executable failures are consequences, not passing browser tests. Nativeb1216763a pins only the browser job to ubuntu-24.04, leaving dependency versions, tests and assertions unchanged. Direct Actionlint1.7.12 passes at the owning native and final integration points; the first shim-only attempt could not run because no global version was configured, and is not counted as lint proof. No global tooling was changed. The only9356 ->ee7 delta is this workflow host/comment; runtime, Cargo, docs, tests and Docker-context inputs are unchanged. Actual browser execution on the supported hosted runner remains pending and must pass before readiness. No waiver or browser skip.