Repository navigation
Extract native EC lifecycle and DynamoDB persistence - #1536
ChristianPavilonis wants to merge 8 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.
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.