Add config store runtime config spec#647
Conversation
aram356
left a comment
There was a problem hiding this comment.
Summary
Spec for moving trusted-server.toml from build-time embedded config to runtime config-store loading. Scope and fail-closed posture are good, but several load-bearing details are unspecified or contradict the existing pipeline. Requesting changes on the items below before merge.
Blocking
🔧 wrench
- Hash algorithm not specified despite explicit "future attestation" goal (lines 328–332): The doc commits to a config hash over canonical TOML bytes but never names SHA-256 vs BLAKE3 vs SHA-3. This locks in (or breaks) the attestation contract in
2026-01-15-attestation-design.md. Pin the algorithm + encoding (suggest SHA-256, hex) or explicitly defer to the attestation spec with a forward reference. - Canonical TOML form is underspecified for hash stability (lines 310–318): "Stable ordering" leaves these undefined: key ordering rule (lexicographic vs schema-declared vs insertion), string flavor (basic
"..."vs literal'...'), numeric formatting (1vs1.0), table vs inline-table, array-of-tables vs inline-array, Unicode normalization. Twotoml-rsversions can disagree. Either enumerate the rules or pin a canonicalizer crate + version. - "Explicitly declared settings only" contradicts the existing Serialize round-trip (lines 313–316): Today
crates/trusted-server-core/build.rs:44-45doestoml::to_string_pretty(&settings), including defaults. The new contract requires a "provided vs defaulted" distinction that#[serde(default)]erases. Implementers will need customSerializeor a default-elision pass over a baselineSettings. Pick an approach or call this out as a known cost. - Bootstrap "store reference" is platform-undefined (lines 222–226): Fastly Compute references config stores by name at the WASM guest. The bootstrap section should at least state: on Fastly the store-name is a fixed constant compiled into the adapter, and
fastly.tomlmust declare ats-config[local_server.config_stores]for dev. As written the "bootstrap contract" is not actionable for the only currently supported platform. deny_unknown_fieldsis a behavioral break with no migration path (lines 278–286):Settingsdoes not currently use#[serde(deny_unknown_fields)]. The new contract fail-closes any operator TOML with stale or typo'd keys. Spec should explicitly state (a) this is a breaking change vs. existing operator configs and (b) theintegrationscarve-out applies only to per-integration payload internals — the dynamic top-level[integrations.<id>]keyspace itself is not denied.- Per-request load cost forecloses straightforward optimization (lines 198–213): "Correctness does not depend on cross-request in-memory persistence" plus Fastly's per-request guest model means a worst-case implementation does TOML parse + per-handler regex compile (
prepare_runtime()) every request. The spec allows in-memory caching as a perf optimization but forbids it for correctness. Worth flagging as a hard implementation constraint so readers don't underestimate cost.
❓ question
- Source-selection signal is unspecified (lines 249–258): Step 1 says "Select the source by environment" but never describes the runtime mechanism — cargo feature, env var, Fastly service-ID check, bootstrap flag? This is the central pipeline decision and is undefined. Either pin the mechanism or explicitly defer with a marker.
- Secrets migration is missing:
trusted-server.tomlcarriesedge_cookie.secret_keyandpublisher.proxy_secret(crates/trusted-server-core/src/settings_data.rs:43-57), overridden today viaTRUSTED_SERVER__EDGE_COOKIE__SECRET_KEYandTRUSTED_SERVER__PUBLISHER__PROXY_SECRETat build time. Removing build-time env merging implies either (a) plaintext secrets in the (unencrypted) Fastly Config Store, or (b) splitting secrets intoPlatformSecretStoreand referencing them by name fromSettings. The spec is silent — which is the intended model?
Non-blocking
🤔 thinking
- "Production vs development" runtime distinction is largely cosmetic (lines 187–190): Both paths read
ts-configfrom a (real or simulated) config store. The actual difference is who populates the store, not how the runtime reads it. Collapsing to one runtime path plus a tooling pre-flight step would simplify the doc. - Fastly Config Store value-size limit is not addressed: Fastly caps each value (~8 KB historically). Current canonical TOML is ~3.4 KB; source is ~6.8 KB. Future growth could exceed the limit. One line in "Implementation notes" surfaces this at design time.
- Multi-tenant constraint of fixed
ts-configkey (lines 235–243): Hard-coding the key globally precludes per-tenant config via key multiplexing in a shared binary. Likely intentional — call it out so future cross-platform abstractions don't assume key namespacing.
♻️ refactor
- Cross-link to existing attestation spec: Spec mentions "future attestation" four times.
docs/superpowers/specs/2026-01-15-attestation-design.mdalready exists. Add a "Related work" section linking it. - Name the existing platform abstraction:
crates/trusted-server-core/src/platform/traits.rs:12definesPlatformConfigStore. Spec says "existing generic config-store abstractions" but never names it. Naming makes implementation actionable. - Specify how the dev
ts-configpayload reaches Viceroy (lines 418–422): Viceroy reads config-store contents from[local_server.config_stores.<name>.contents]infastly.toml(fastly.toml:42-49). Either dev tooling regenerates that section perfastly compute serve, or an alternate loading mechanism. Worth one paragraph.
🌱 seedling
- Note
/healthcarve-out from fail-closed:crates/trusted-server-adapter-fastly/src/main.rs:55-61short-circuits/healthbefore settings load. Worth a one-liner so a future reader doesn't "fix" this as inconsistent with fail-closed semantics.
📌 out of scope
- Test fixtures + CI depend on tracked
trusted-server.toml: Removing it touchescrate_test_settings_str()consumers and 10+ tests incrates/trusted-server-core/src/settings.rs. Open a follow-up issue tracking the test/CI migration before implementation lands.
⛏ nitpick
- Add Author line: Peers use
> **Author:**. This spec omits it. - Inconsistent terminology: "platform config store" / "platform config-store" / "platform-store" used interchangeably. Pick one.
- Bullet phrasing at line 314:
do **not** expand…mid-sentence reads oddly inside the numbered list.
CI Status
- cargo fmt: PASS
- cargo test: PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- Analyze (rust / actions / js-ts): PASS
- browser integration tests: PASS
- integration tests: PASS
- CodeQL: PASS
| This section defines the semantic pipeline for both production and development. | ||
|
|
||
| ### Step 1: Select the source by environment | ||
|
|
There was a problem hiding this comment.
❓ question — Source-selection signal is unspecified. Step 1 says "Select the source by environment" but never describes the runtime mechanism. Cargo feature? Env var read at module init? Fastly service-ID check? Bootstrap config flag? This is the central pipeline decision and is undefined. Either pin the mechanism here or defer explicitly (e.g., "to be defined in the tooling spec, but must be a compile-time signal").
| The config hash is computed over the canonical TOML bytes. | ||
|
|
||
| This provides a stable hashable representation of application config suitable | ||
| for observability and future attestation work. |
There was a problem hiding this comment.
🔧 wrench — Hash algorithm not specified despite explicit "future attestation" goal. The doc commits to a config hash over canonical TOML bytes but never names SHA-256 vs BLAKE3 vs SHA-3, nor the encoding (hex / base64 / raw). Choosing here locks in the attestation contract in 2026-01-15-attestation-design.md. Pin the algorithm + encoding (suggest SHA-256, hex) or explicitly defer to the attestation spec with a forward reference.
| - do **not** expand the config into a full dump of all effective defaulted runtime values | ||
| - define stable ordering for map-like structures so identical semantic config | ||
| produces identical canonical bytes | ||
|
|
There was a problem hiding this comment.
🔧 wrench — Canonical TOML form is underspecified for hash stability. "Define stable ordering for map-like structures" leaves the following undefined and stability-critical:
- key ordering rule (lexicographic vs schema-declared vs insertion)
- string flavor (basic
"..."vs literal'...') - numeric formatting (
1vs1.0) - table vs inline-table normalization
- array-of-tables vs inline-array
- Unicode normalization of string values
Two correct implementations (or two toml-rs versions) can produce different bytes from the same Settings and break hash stability. Either enumerate the rules explicitly or pin a canonicalizer crate + version.
| - serialize it in a deterministic TOML form | ||
| - include explicitly declared settings only | ||
| - do **not** expand the config into a full dump of all effective defaulted runtime values | ||
| - define stable ordering for map-like structures so identical semantic config |
There was a problem hiding this comment.
🔧 wrench — "Explicitly declared settings only" contradicts the existing Serialize round-trip. Today crates/trusted-server-core/build.rs:44-45 does toml::to_string_pretty(&settings), which includes defaults. The new contract requires a "provided vs defaulted" distinction that #[serde(default)] erases. Implementers will need either custom Serialize impls per type or a default-elision pass that compares against a baseline Settings. Pick an approach or call this out as a known cost.
| Production bootstrap must provide: | ||
|
|
||
| - the platform config store reference needed to open/read the store | ||
|
|
There was a problem hiding this comment.
🔧 wrench — Bootstrap "store reference" is platform-undefined. Fastly Compute references config stores by name at the WASM guest level — the names live in fastly.toml, not in environment. The spec should at least state:
- On Fastly, the bootstrap form is a fixed store-name constant compiled into the adapter.
fastly.tomlmust declare ats-config[local_server.config_stores]entry for dev.
As written, the bootstrap contract is not actionable for the only currently supported platform.
|
|
||
| This spec does not define a special "healthy but unusable" mode for config | ||
| failure. | ||
|
|
There was a problem hiding this comment.
🌱 seedling — Note /health carve-out from fail-closed. crates/trusted-server-adapter-fastly/src/main.rs:55-61 short-circuits /health before settings load, which is a deliberate carve-out from the fail-closed model defined here. A one-liner acknowledging this would prevent a future reader from "fixing" the early-return as a bug.
| - tracked template file | ||
| - kept in sync with currently supported configuration features | ||
| - intended to help operators create a real `trusted-server.toml` | ||
|
|
There was a problem hiding this comment.
📌 out of scope — Test fixtures + CI depend on tracked trusted-server.toml. Removing it touches crate_test_settings_str() consumers and 10+ tests in crates/trusted-server-core/src/settings.rs, plus CI workflows that rely on the file existing at the repo root. Worth a follow-up issue tracking the test/CI migration before this becomes implementable.
|
|
||
| > **Status:** Proposal | ||
| > **Scope:** Config store architecture only | ||
| > **Date:** April 2026 |
There was a problem hiding this comment.
⛏ nitpick — Peer specs (e.g., 2026-01-15-attestation-design.md) include an Author: line. Adding one keeps consistency.
| - **Development:** local file at repo root by default | ||
|
|
||
| The authoritative production payload is stored in the platform config store under | ||
| key `ts-config`. |
There was a problem hiding this comment.
⛏ nitpick — Inconsistent terminology. "platform config store" / "platform config-store" / "platform-store" / "config-store payload" / "platform store payload" appear interchangeably. Pick one form for the doc.
| - parse valid config through the typed config model | ||
| - serialize it in a deterministic TOML form | ||
| - include explicitly declared settings only | ||
| - do **not** expand the config into a full dump of all effective defaulted runtime values |
There was a problem hiding this comment.
⛏ nitpick — do **not** expand the config into a full dump of all effective defaulted runtime values reads oddly mid-sentence inside this numbered list. Consider promoting it to its own bullet.
Summary
Closes #665
Testing