Skip to content

Honor EdgeZero app config store default - #879

Open
ChristianPavilonis wants to merge 5 commits into
mainfrom
fix/honor-ez-app-config
Open

Honor EdgeZero app config store default#879
ChristianPavilonis wants to merge 5 commits into
mainfrom
fix/honor-ez-app-config

Conversation

@ChristianPavilonis

Copy link
Copy Markdown
Collaborator

Summary

  • Derive the Trusted Server runtime app-config store ID and blob key from [stores.config].default in edgezero.toml at build time.
  • Preserve trusted_server_config as the current default while retaining EdgeZero store name/key environment overrides.
  • Align Fastly, Axum, Cloudflare, integration fixtures, and operator documentation with the manifest-defined default.

Changes

File Change
crates/trusted-server-core/build.rs Validate edgezero.toml and expose its default config-store ID as a compile-time value.
crates/trusted-server-core/src/config_payload.rs, settings_data.rs Use the manifest-derived ID for the default runtime store and blob key, with regression coverage for manifest alignment and environment overrides.
crates/trusted-server-adapter-fastly/src/main.rs Open the resolved manifest-default config store instead of a separate hardcoded name.
crates/trusted-server-adapter-cloudflare/ Read the manifest-default blob key while retaining compatibility with the legacy app_config key.
Integration fixtures and docs/guide/configuration.md Generate and document app-config stores and keys from the shared manifest-derived constant.

Closes

Closes #866

Test plan

  • cargo test-fastly && cargo test-axum
  • cargo clippy-fastly && cargo clippy-axum
  • cargo fmt --all -- --check
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run
  • JS format: cd crates/trusted-server-js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve
  • Other: cargo test-cloudflare; cargo clippy-cloudflare && cargo clippy-cloudflare-wasm; integration config generator and non-ignored integration tests; Spin, CLI, and parity tests

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses tracing macros (not println!) — no runtime logging added; build-script println! calls emit required Cargo directives
  • New code has tests
  • No secrets or credentials committed

@ChristianPavilonis
ChristianPavilonis requested review from aram356 and prk-Jr and removed request for aram356 July 9, 2026 21:12

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Replacing the Fastly hardcode with a manifest-derived constant is the right direction — Spin already resolved its config store through ctx.config_store_default() (adapter-spin/src/platform.rs:726), so Fastly's literal was the last one holding out, and the Cloudflare key alignment keeps a legacy fallback rather than breaking existing Workers. Read all 13 changed files.

Two blocking items. One is the red cargo fmt job, a one-line import reorder. The other is the documentation paragraph introducing the EDGEZERO__STORES__CONFIG__<ID>__KEY override: ts config push does not honor that variable, so following the instruction as written puts the blob at one key and the runtime read at another, and the service fails closed at startup. I confirmed both halves with dry-run probes against this branch's own CLI (details inline). The rest is non-blocking — mostly a question about whether the new build script earns its coupling, given the test added alongside it already catches the same drift.

Blocking

🔧 wrench

  • cargo fmt fails, CI red: import ordering (crates/trusted-server-core/src/settings_data.rs:6). Only formatting diff in the workspace.
  • The newly documented …__KEY override fails the deploy closed: the runtime reads it, ts config push ignores it, so the push and the read disagree on the blob key (docs/guide/configuration.md:1473-1476). Probe output and the edgezero-cli call site are in the inline comment.

Non-blocking

🤔 thinking

  • Does the build script earn its coupling? config_defaults_match_edgezero_manifest already catches manifest drift, while the build script adds a host build of edgezero-core and its transitive deps, a hard repo-layout dependency on the library crate, and full-manifest validation as a build gate — so an error in an unrelated [adapters.spin] block now breaks the build of every adapter (crates/trusted-server-core/build.rs:14).
  • Cloudflare gets a legacy fallback; Fastly and Axum do not: undocumented asymmetry, and LEGACY_CONFIG_BLOB_KEY has no deprecation note or removal condition (crates/trusted-server-adapter-cloudflare/src/app.rs:106).
  • A Cloudflare startup error can name a key the operator never set: manifest key absent plus a non-string at app_config reports "missing string value at trusted_server_config" (crates/trusted-server-adapter-cloudflare/src/app.rs:97).

♻️ refactor

  • CONFIG_BLOB_KEY now fills five roles and is named for one: it is derived from a logical store id, then used as env-override lookup id, platform store name, blob key, Cloudflare JSON object key, and Viceroy store name and key. Suggest a DEFAULT_CONFIG_STORE_ID const with CONFIG_BLOB_KEY defined from it (crates/trusted-server-core/src/config_payload.rs:15).
  • Build-script panics omit the path they looked at (crates/trusted-server-core/build.rs:15).

⛏ nitpick

  • Comment line overruns the block's wrap width (edgezero.toml:19).

📝 note

  • Base is two commits behind origin/main (d744b764 vs f6a2fb85) — worth a refresh before merge so CI runs against current main.

👍 praise

  • config_defaults_match_edgezero_manifest asserts both directions, so an accidental edgezero.toml edit fails a test instead of silently repointing every adapter (crates/trusted-server-core/src/settings_data.rs:248).
  • cloudflare_config_does_not_mask_malformed_manifest_value pins the one behavior an or_else chain would have gotten wrong (crates/trusted-server-adapter-cloudflare/src/app.rs:646).
  • The docs correct a claim that was wrong: the config store is not optional-and-possibly-empty; an absent entry fails startup closed. Both new CLI invocations check out against edgezero-cli v0.0.4's argument definitions (docs/guide/configuration.md:1479).

CI Status

  • fmt: FAIL (crates/trusted-server-core/src/settings_data.rs:6)
  • clippy (fastly / axum / cloudflare native + wasm / spin native + wasm): PASS
  • rust tests (fastly, axum, cloudflare, spin, cross-adapter parity, ts CLI): PASS
  • js tests (vitest): PASS
  • browser + integration + Fastly EC lifecycle: PASS
  • CodeQL: PASS

Comment thread crates/trusted-server-core/src/settings_data.rs Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-core/build.rs Outdated
Comment thread crates/trusted-server-core/build.rs Outdated
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs Outdated
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs Outdated
Comment thread crates/trusted-server-core/src/settings_data.rs
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs
Comment thread edgezero.toml Outdated

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Re-review of 2760e429 against 044cd7e9. All eight findings from the previous round are answered: the cargo fmt regression is gone (verified locally, exit 0, matching the now-green CI job), the build script's load failure carries the attempted path, DEFAULT_CONFIG_STORE_ID is split from CONFIG_BLOB_KEY and threaded through the store-identity call sites, the Cloudflare fallback gained a typed error plus three new tests, and the manifest comment is rewrapped.

One documentation change introduced in the fix commit reproduces the failure mode the previous round flagged, this time as a copy-pasteable command, and one sentence next to it conflicts with the Fastly deployment mechanism documented elsewhere in this repo. Everything else is non-blocking.

Blocking

🔧 wrench

  • --key example sets the runtime override on the push process: ts config push never reads store_key (verified in the pinned CLI at edgezero-cli/src/config.rs:435), so the env prefix is inert and the runtime never sees it — the operator lands in the fail-closed startup described three lines below (docs/guide/configuration.md:1480-1483)

❓ question

  • Does the Fastly runtime honor EDGEZERO__STORES__CONFIG__<ID>__NAME?: .claude/skills/deploying-trusted-server-to-fastly/SKILL.md:28-34 documents fastly resource-link as the runtime mapping instead, which would make NAME push-side only on this adapter (docs/guide/configuration.md:1473-1476)

Non-blocking

♻️ refactor

  • Missing [stores.config] still panics without the path: the load-failure path was fixed, the section-missing path one line later was not (crates/trusted-server-core/build.rs:32)
  • normalize_env_segment hand-rolled in the integration harness: duplicates adapter-axum/src/platform.rs:24, doubled by this commit, and already diverges on to_ascii_uppercase() vs to_uppercase() (crates/trusted-server-integration-tests/tests/environments/axum.rs:37-44)
  • Dead match guard: when the two keys are equal, the guarded arm and the final arm produce the same Missing (crates/trusted-server-adapter-cloudflare/src/app.rs:149)

🤔 thinking

  • KEY override is adapter-dependent: Fastly and Axum resolve through default_config_key(); Cloudflare reads the constant directly, so the override does not reach it (crates/trusted-server-core/src/config_payload.rs:20)

⛏ nitpick

  • Comment between #[cfg] and item: belongs above the attribute, and reads better as /// (crates/trusted-server-adapter-cloudflare/src/app.rs:78-82)

👍 praise

  • Typed Cloudflare envelope error: names the key that is actually malformed rather than the one the operator was expected to set, and the malformed-legacy test asserts the rendered message, not just the variant (crates/trusted-server-adapter-cloudflare/src/app.rs:84-110)
  • Formatting regression fixed: cargo fmt --all -- --check is clean locally

CI Status

  • fmt: PASS (also verified locally)
  • clippy (cloudflare native + wasm32-unknown-unknown, spin native + wasm32-wasip1): PASS
  • rust tests: PASS (cargo test, axum native, cloudflare, spin, cross-adapter parity, ts CLI native)
  • integration tests: PASS (Fastly EC lifecycle, browser, general)
  • js tests: PASS (vitest)
  • format-typescript / format-docs: PASS
  • Analyze (javascript-typescript): FAIL — GitHub infrastructure, not code. The CodeQL init action died with No server is currently available to service your request before producing any artifacts. Analyze (rust) and Analyze (actions) passed on the same commit.

Comment thread docs/guide/configuration.md Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-core/build.rs Outdated
Comment thread crates/trusted-server-integration-tests/tests/environments/axum.rs Outdated
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs Outdated
Comment thread crates/trusted-server-core/src/config_payload.rs
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs Outdated
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Third pass on this branch. All eight findings from the previous round check out as addressed in 391fbb52: the runtime KEY example is gone from the Fastly guide, the resource-link runtime model is documented, a missing [stores.config] now emits cargo::error with the resolved manifest path (reproduced locally by corrupting the manifest), the axum harness calls config_env_var, the dead equality guard is gone, and the legacy-key rationale is a /// doc above the #[cfg]. The Rust side of this PR is clean; what is left is one operator-facing gap in the new Fastly documentation plus a scope note on where the EDGEZERO__* overrides can actually be read.

2 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch for both) to apply them as commits on this branch. Both were verified in a scratch worktree at this head: cargo fmt --all -- --check, cargo check --tests and cargo clippy --all-targets -- -D warnings on the integration-tests manifest, and the pinned docs/ Prettier, with pre/post-verification patch snapshots byte-identical. The remaining comments describe the change in prose.

Blocking

🔧 wrench

  • fastly resource-link create sequence cannot run on a deployed service, and never takes effect — see inline at docs/guide/configuration.md:1481-1484

Non-blocking

🤔 thinking

  • EDGEZERO__…__NAME / __KEY are inert on Fastly, and the new test reads as a guarantee that they are not — see inline at crates/trusted-server-core/src/settings_data.rs:32
  • This section now forks the deploy skill's multi-service guidance, minus its guardrails — see inline at docs/guide/configuration.md:1487-1490

♻️ refactor

  • Hand-built Map where the same PR uses json! with a const key — see inline at crates/trusted-server-integration-tests/tests/common/config.rs:39-43

👍 praise

  • main.rs and app.rs now resolve the same store name — see inline at crates/trusted-server-adapter-fastly/src/main.rs:52
  • Override test injects EnvConfig instead of mutating global env — see inline at crates/trusted-server-core/src/settings_data.rs:258

Cross-cutting / body-level findings

  • 📌 Last shipped app_config reference points the JWKS store at the app-config storetrusted-server.example.toml:38 and crates/trusted-server-integration-tests/fixtures/configs/trusted-server.integration.toml:31 both set [request_signing].config_store_id = "app_config". That field is the Fastly Config Store ID used for JWKS (docs/guide/configuration.md:516 documents it as such, and docs/guide/request-signing.md:219 uses jwks_store), so the value is wrong on two counts: wrong store, and a store name where an ID belongs. Inert today because [request_signing].enabled = false in both files, and genuinely outside this PR's scope — but after this sweep it is the only app_config left in shipped configuration, so it is now the one place an operator could still read the old name as current. Worth a follow-up issue rather than a change here.

CI Status

  • browser integration tests: PASS
  • integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • prepare integration artifacts: PASS
  • CodeQL: PASS
  • Analyze (rust): PASS
  • Analyze (actions): PASS
  • Analyze (javascript-typescript): PASS
  • cargo fmt: PASS (required)
  • cargo test: PASS (required)
  • cargo test (axum native): PASS
  • cargo test (ts CLI, native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • vitest: PASS
  • format-docs: PASS (required)
  • format-typescript: PASS (required)

Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-core/src/settings_data.rs
Comment thread crates/trusted-server-integration-tests/tests/common/config.rs Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-adapter-fastly/src/main.rs
Comment thread crates/trusted-server-core/src/settings_data.rs

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Fourth pass. All four findings from the previous round check out as addressed in 953f0df2: fastly resource-link create now carries --autoclone and is followed by service-version activate, the hand-built Map in the integration harness became json! with the const key, and both default_config_store_name / default_config_key carry a /// note that the process-env path is native-adapter-only. The Rust side of this PR reads clean and its test coverage pins the manifest-to-constant relationship in the right place. What is left is one operator-facing correctness bug in the new Fastly deployment sequence, plus a merge conflict against current main.

1 of the inline comments below carries a one-click GitHub suggestion — use Commit suggestion to apply it as a commit on this branch. It was verified in a scratch worktree at this head against the pinned docs/ Prettier, with pre/post-verification patch snapshots byte-identical. The remaining comments describe the change in prose.

Blocking

🔧 wrench

  • Shared-account first-deployment path links the store under the physical name, not the logical one — see inline at docs/guide/configuration.md:1503-1519
  • Merge conflict against main — see the cross-cutting section below

Non-blocking

🤔 thinking

  • __KEY override is honoured on read but never on write — see inline at crates/trusted-server-core/src/settings_data.rs:46

♻️ refactor

  • Hand-built Value::Object helper where the same file uses json! with a const key — see inline at crates/trusted-server-adapter-cloudflare/src/app.rs:685
  • Error type does not follow the derive_more::Display + impl Error convention — see inline at crates/trusted-server-adapter-cloudflare/src/app.rs:92

👍 praise

  • Build script fails closed with the resolved manifest path — see inline at crates/trusted-server-core/build.rs:20
  • Manifest-to-constant alignment is pinned by a test — see inline at crates/trusted-server-core/src/settings_data.rs:237

Cross-cutting / body-level findings

  • 🔧 Merge conflict against mainmergeStateStatus is DIRTY. One file conflicts: crates/trusted-server-adapter-cloudflare/src/app.rs, at the import block (lines 13-18 of the merged result), against 42a34ae3 (#860, configurable cache header policies) which landed on main after this branch's merge commit. The two sides add adjacent use lines and the resolution is keep-both:

    #[cfg(any(test, target_arch = "wasm32"))]
    use trusted_server_core::config_payload::CONFIG_BLOB_KEY;
    use trusted_server_core::cache_policy::EdgeCacheHeader;

    Flagged at the body level because the conflict is not inside this PR's diff, so it has no inline anchor. Note that CI is green at 29cd794e, but that run predates 42a34ae3; the checks have not seen the two changes combined.

CI Status

  • integration tests: PASS
  • browser integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • prepare integration artifacts: PASS
  • CodeQL: PASS
  • Analyze (rust): PASS
  • Analyze (actions): PASS
  • Analyze (javascript-typescript): PASS
  • cargo fmt: PASS (required)
  • cargo test: PASS (required)
  • cargo test (axum native): PASS
  • cargo test (ts CLI, native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • vitest: PASS
  • format-docs: PASS (required)
  • format-typescript: PASS (required)

Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-core/src/settings_data.rs Outdated
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs Outdated
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs
Comment thread crates/trusted-server-core/build.rs
Comment thread crates/trusted-server-core/src/settings_data.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Honor edgezero.toml config store id for Trusted Server app config

2 participants