Repository navigation
Extract explicit EC storage selection and celld - #1537
ChristianPavilonis wants to merge 21 commits into
Conversation
# Conflicts: # crates/trusted-server-adapter-axum/src/ec_kv_tests.rs
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The typed [ec.storage] model, the Fastly legacy compatibility and the Cloudflare/Spin rejection paths look sound. Two things should be addressed before this unit is ready: the shared example config now fails at startup on every adapter except Fastly, and the new cargo test (axum native) job fails at this head.
Blocking
🔧 wrench
-
The example config fails at startup off Fastly. See the inline comment on
trusted-server.example.toml. -
CI:
cargo test (axum native)fails atf14eefa(not a required check on this base).active_legacy_ec_environment_cannot_disable_or_override_published_storagepanics atcrates/trusted-server-adapter-axum/tests/startup.rs:165with "should not listen while required startup inputs are invalid". The same test passed on the earliercdad6df31run, so this looks like a flake rather than a production defect. The likely cause is the Linux self-connection already fixed innative_acceptance.rs(connected_to_listener, merged in83f58c5e3).ServerFixture::assert_failed_before_listeninstartup.rsuses the same reserve-release-reconnect probe without that guard, and this PR adds three new callers. The CI log has no socket endpoints, so the cause is inferred rather than confirmed. Suggested change (outside the diff, so not a one-click suggestion; not compiled):if let Ok(stream) = TcpStream::connect_timeout(&self.addr, Duration::from_millis(20)) { // Linux can self-connect when an ephemeral source port equals // the target; only distinct endpoints prove a real listener. assert_eq!( stream.local_addr().expect("should read probe source"), stream.peer_addr().expect("should read probe peer"), "should not listen while required startup inputs are invalid" ); }
Non-blocking
🤔 thinking
ts config push --adapter <x>already knows its target runtime, but publishing only runsvalidate_storage. Resolving against the target adapter at push time would surface the example-config failure before the server starts. This is separate from the "do not resolve according to the CLI host" rule.- Celld preparation errors: see the inline comment on
celld_ec.rs.
📝 note
- The
[ec]field table indocs/guide/configuration.md(around line 645) still lists onlyec_storeand does not documentstorage.
CI Status
- 16 checks pass.
- FAIL:
cargo test (axum native), which is not in the base branch's required set. - Locally, the core
ec_storage_configtests pass (5/5). I also ran a throwaway probe that resolves the shipped example config for each runtime. The full adapter suites, the real-celld ignored tests and the image script were not re-run for this review.
| [ec.storage] | ||
| store = "ec_identity_store" |
There was a problem hiding this comment.
🔧 wrench: This makes the shared starter config fail at startup on Axum, Cloudflare and Spin.
ts config init writes this file, and the Axum quickstart (docs/guide/getting-started.md, Option B) copies it unchanged. Previously, ec_store = "ec_identity_store" was inactive off Fastly, so one starter worked on every adapter. With the provider omitted, resolve_storage returns ec.storage.provider is required outside Fastly for Native, Cloudflare and Spin. Axum fails in build_state_with_services and initialize_native, while ts config push --adapter axum still succeeds because publication only checks shape.
I reproduced this by parsing this exact file (with resolved test secrets) and resolving it for each runtime: Fastly returns Ok(Some), and the other three return that error. The integration fixtures in this PR already remove ec.storage for Axum and Cloudflare to work around it (tests/common/config.rs). This conflicts with the spec's goal to preserve the behavior of deployments without configured persistence.
Options:
- (a), preferred: keep
ec_store = "ec_identity_store"in the example. It is still supported with no removal deadline, so one starter keeps working everywhere. The cost is a one-line spec change ("new examples use[ec.storage]"). - (b): keep this section and add a "remove
[ec.storage]" step to the Axum, Cloudflare and Spin setup docs.ts config initwould then still produce a file that only works on Fastly.
| records: CelldRecordStore, | ||
| } | ||
|
|
||
| fn preparation_error() -> EdgeError { |
There was a problem hiding this comment.
🤔 thinking: Every preparation failure (bad or rejected token, unsupported protocol or record version, unreachable service) collapses into the same native EC celld preparation failed, and store_error does the same for request-time errors. Hiding input values is intended, but an operator can't tell a credential problem from an outage. Could you attach a value-free error category from the celld error kind (for example credential, protocol or transport)?
Resolve provider shape in Core and construct the selected native live handle during bounded prepared startup. Reject obsolete launch selection and unsupported or uncompiled providers before readiness. Keep exact retained table and namespace identities. Use EdgeZero .2 conditional records for celld, without a generic-KV replacement or cached identity snapshots. Preserve current adapter normalization and direct transport ownership. Qualify real disposable celld persistence, restart, CAS, withdrawal and outage, plus typed-config DynamoDB binary fault controls.
The shared CLI template deliberately selects Fastly storage. Leave that graph intact in Fastly fixtures and remove only its known placeholder for Axum, Cloudflare and Spin smokes before typed validation and export. Give the EOF fixture its serving adapter and keep non-Fastly graphs unconfigured. Preserve production provider validation and all negative startup/body/timeout assertions. Native CLI export now reaches prepared startup without an unsupported Fastly selection.
Pass the serving adapter through the existing helper caller, keep both listener-port cases, and assert that Axum/Cloudflare/Spin omit Fastly storage while Fastly retains it.
There was a problem hiding this comment.
Superseded by a follow-up review with inline comments: #1537 (review)
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Moves EC persistence selection into explicit [ec.storage] config with per-runtime resolution, adds celld, and cuts native DynamoDB over from env vars. The design is clean and well tested; nothing blocking.
2 of the inline comments below carry a one-click GitHub
suggestion. The rest describe the fix in prose.
Non-blocking
🤔 thinking
local_endpointnow lives in published config — see inline atcrates/trusted-server-core/src/ec/storage.rs:43- Example config now fails startup off Fastly — see inline at
trusted-server.example.toml:102
♻️ refactor
- DynamoDB binding rules duplicated — see inline at
crates/trusted-server-core/src/ec/storage.rs:125
⛏ nitpick
unreachable!on provider mismatch — see inline atcrates/trusted-server-adapter-fastly/src/main.rs:698andcrates/trusted-server-adapter-axum/src/startup.rs:319- Unformatted long line — see inline at
crates/trusted-server-core/src/ec/storage.rs:144 - Guide understates Cloudflare/Spin rejection — see inline at
docs/guide/native-ec-persistence.md:56
CI Status
- All 25 reported checks: PASS (including
prepare integration artifacts,cargo fmt, adapter tests, parity, integration and browser tests)
| pub namespace: String, | ||
| /// Explicit loopback emulator endpoint, never a production override. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub local_endpoint: Option<String>, |
There was a problem hiding this comment.
🤔 thinking — Before this PR the emulator endpoint was a launch-time env var (TS_EC_DYNAMODB_LOCAL_ENDPOINT). It is now a field of the signed, published app config, so the same artifact that goes to production can carry it. Validation limits it to http://127.0.0.1:<port>/ and it fails closed (writes go nowhere rather than somewhere wrong), but it also bypasses the credential chain. Consider keeping it out of the published schema (env or test-only preparation API), or rejecting it unless a local-only build feature or flag is set.
| "ec.storage.region is required and must not contain whitespace", | ||
| )); | ||
| } | ||
| if !(3..=255).contains(&self.table.len()) |
There was a problem hiding this comment.
♻️ refactor — These table/namespace rules repeat validate_binding in crates/trusted-server-adapter-axum/src/ec_kv.rs (around line 178). Two copies can drift. Since resolve_storage already calls validate() before handing the config to the adapter, the adapter copy could be dropped, or both could call one shared helper.
Apply manually: the change spans two files.
| Ok(Some(KvIdentityGraph::new(FastlyEcKvStore::new(store_name)))) | ||
| } | ||
| None => Ok(None), | ||
| _ => unreachable!("Fastly resolution should select only Fastly storage"), |
There was a problem hiding this comment.
⛏ nitpick — resolve_storage(EcRuntime::Fastly) cannot return a native provider today, so this is unreachable. It is still a production panic path if the resolver changes later. Returning a configuration error keeps it fail-closed without a panic:
Some(_) => Err(Report::new(TrustedServerError::Configuration {
message: "ec.storage.provider is unsupported by the selected serving runtime".to_owned(),
})),The same applies to the matching unreachable! in crates/trusted-server-adapter-axum/src/startup.rs:319.
| } | ||
| } | ||
| Some(EcStorageSelection::Fastly(_)) => { | ||
| unreachable!("native resolution should not select Fastly") |
There was a problem hiding this comment.
⛏ nitpick — Same as crates/trusted-server-adapter-fastly/src/main.rs:698: return configuration_error(...) here instead of panicking.
| )); | ||
| } | ||
| if let Some(endpoint) = &self.local_endpoint { | ||
| let url=url::Url::parse(endpoint).map_err(|_|invalid("ec.storage.local_endpoint must be a loopback HTTP root URL with an explicit port"))?; |
There was a problem hiding this comment.
⛏ nitpick — rustfmt skips this statement because the string literal is too long, so it stays unformatted. Wrapping the closure lets rustfmt accept it.
| let url=url::Url::parse(endpoint).map_err(|_|invalid("ec.storage.local_endpoint must be a loopback HTTP root URL with an explicit port"))?; | |
| let url = url::Url::parse(endpoint).map_err(|_| { | |
| invalid("ec.storage.local_endpoint must be a loopback HTTP root URL with an explicit port") | |
| })?; |
| # logical KV declaration does not select EC storage. Legacy ec.ec_store remains | ||
| # supported on Fastly, but cannot be combined with this section. | ||
| [ec.storage] | ||
| store = "ec_identity_store" |
There was a problem hiding this comment.
🤔 thinking — Before this PR the example used legacy ec_store, which is inactive on Axum, Cloudflare and Spin. The store-only [ec.storage] shorthand is a hard startup error on all three ("provider is required outside Fastly"). Anyone who copies the shared example to run a non-Fastly adapter now fails before readiness. Consider commenting this section out with a note, or calling the change out in the migration notes.
|
|
||
| This is a hard cutover from native environment-only selection. `TS_EC_DYNAMODB_ENABLED=true` now fails before readiness with migration guidance, even if new config is also present. Malformed enablement also fails. Absent or `false` does not select a backend; orphaned old binding variables are ignored, not another source. Prepare and publish the new config, then remove the old enablement/region/table/namespace/endpoint launch inputs. There is no fallback reader. | ||
|
|
||
| Legacy `[ec] ec_store = "exact-binding"` remains supported on Fastly and inactive on other adapters. New Fastly config may use `[ec.storage] store = "exact-binding"`, with an omitted or explicit `fastly` provider. Both forms together are rejected. Cloudflare and Spin reject any explicit storage selection before routed application work. Shared parsing and publication do not resolve providers according to the CLI host. Missing storage stays absent when serialized, with no injected provider defaults. |
There was a problem hiding this comment.
⛏ nitpick — The code also rejects the store-only shorthand on Cloudflare and Spin, not just explicit providers. Suggested wording:
| Legacy `[ec] ec_store = "exact-binding"` remains supported on Fastly and inactive on other adapters. New Fastly config may use `[ec.storage] store = "exact-binding"`, with an omitted or explicit `fastly` provider. Both forms together are rejected. Cloudflare and Spin reject any explicit storage selection before routed application work. Shared parsing and publication do not resolve providers according to the CLI host. Missing storage stays absent when serialized, with no injected provider defaults. | |
| Legacy `[ec] ec_store = "exact-binding"` remains supported on Fastly and inactive on other adapters. New Fastly config may use `[ec.storage] store = "exact-binding"`, with an omitted or explicit `fastly` provider. Both forms together are rejected. Cloudflare and Spin reject any `[ec.storage]` selection, explicit or store-only, before routed application work. Shared parsing and publication do not resolve providers according to the CLI host. Missing storage stays absent when serialized, with no injected provider defaults. |
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:
f14eefa672e83517dc57ff0c0d87383447e07807. Base:baseline/sourcepoint-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
Core3016 plus5 storage configuration cases and native all-feature suites pass. Three explicitly selected real local celld cases pass for lifecycle, prepared startup and native HTTP issuance/withdrawal/restart/outage. Local fixture proof is not replicated/fleet durability.
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.