Support optional typed secret paths and Fastly store mappings - #344
Support optional typed secret paths and Fastly store mappings#344ChristianPavilonis wants to merge 4 commits into
Conversation
2b24957 to
0d6ebf9
Compare
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Three separable changes: an OptionalField secret-path segment threaded through core + CLI, env_config_from_runtime_dictionary made public, and Fastly provision/staged-deploy persistence of non-default logical→physical store-name mappings. The secret-path half is clean and symmetric — CLI and runtime walkers grew matching arms with matching tests. The Fastly store-mapping half introduces a new write into a store the ACTIVE version reads, sourced from ambient process env, with no reconciliation of stale entries; that is where the blocking findings are.
All four CI gates pass locally on 0d6ebf9b.
😃 Praise
collect_secret_leafextraction (crates/edgezero-cli/src/config.rs:1636) avoids a fourth copy of the leaf-resolution block, and threadingoptional_segmentseparately fromfield.optionalkeeps "this segment is optional" distinct from "this leaf isOption<String>" — which makes the required-Fieldarm provably unchanged.- CLI/runtime symmetry maintained exactly:
collect_secret_leavesandresolve_secret_fieldgrew the same two arms in the same order, each with a test on both sides. That lockstep is the hard part of this subsystem. staging_entries_from_productionkept pure, withoverlay_runtime_store_name_entriesas a separate testable function.
Findings
Blocking
- 🔧
provisionrewrites live production store resolution from ambient shell env —crates/edgezero-adapter-fastly/src/cli.rs:598.EnvConfig::from_env()(crates/edgezero-cli/src/provision.rs:96) +resolve_kind(provision.rs:145) mean anyEDGEZERO__STORES__*__NAMEin the operator's shell is written intoedgezero_runtime_env, read by the ACTIVE version. New behavior: before this PRprovisioncreated the store but wrote no entries into it. - 🔧 Stale
__NAMEmappings never removed —cli.rs:3640. Dropping an override leaves the old entry live; production has no equivalent of the staging mirror's upsert-then-delete reconciliation. - 🔧 Override validation weaker than the runtime's, fails open —
cli.rs:3487. Control characters pass the CLI check but are silently rejected byis_blank_or_control(crates/edgezero-core/src/env_config.rs:144), so the runtime falls back to the logical id — wrong store, no diagnostic. - ❓
OptionalFieldhas no derive path —crates/edgezero-core/src/app_config.rs:41. The macro only emitsField/ArrayEach, andnested_child_type(crates/edgezero-macros/src/app_config.rs:453) doesn't unwrapOption<T>, so#[app_config(nested)] x: Option<Inner>fails theAppConfigRootbound. Reachable only from a hand-writtenimpl AppConfigMeta— is that the intended surface? - 🔧 Add
#[non_exhaustive]toSecretPathSegment—app_config.rs:35-42. Public enum, exhaustively matchable; this PR is itself the breaking change. - 🔧 Docs now contradict the code —
docs/guide/configuration.md:393-401still states thatOption<Inner>is unsupported and that "only the leaf's ownOption<String>is skippable. A missing ornullintermediate object/array … is aConfigOutOfDateerror, not a silent skip — bothconfig validateand the runtime reject it." Both halves are now false forOptionalField. (No inline comment — the file is untouched by this PR.)
Non-blocking
- 🤔
env::vars()panics on non-UTF-8 environment —cli.rs:3586. - ♻️ Hoist that env read into the caller so
mirror_production_to_stagingstays pure and end-to-end testable — currently the only untested line in the new staging path. - 🤔
is_runtime_store_name_key(cli.rs:3465) accepts undeclared store ids, so a typo'd override silently no-ops. - ⛏
#[inline]onenv_config_from_runtime_dictionary(crates/edgezero-adapter-fastly/src/lib.rs:184) buys nothing. - ⛏ The
```ignoredoc example (lib.rs:177-182) references a non-existentMyApp::stores()and can never be caught rotting. - 🌱 Test gaps: no coverage of the non-dry-run provision write path, nor of
is_runtime_store_name_keyrejecting near-miss keys (…__A__B__NAME,…__NAME__EXTRA, lowercase kind). The dry-run test atcli.rs:6753covers the happy shape only.
CI Status
Run locally against 0d6ebf9b:
cargo fmt --all -- --check: PASScargo clippy --workspace --all-targets --all-features -- -D warnings: PASScargo test --workspace --all-targets: PASScargo check --workspace --all-targets --features "fastly cloudflare spin": PASS
The three unchecked boxes in the PR description's test plan (cargo test --workspace --all-targets, the feature check, and examples/app-demo) — the first two pass; examples/app-demo was not run.
d4dc05d to
79628ec
Compare
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Tightly scoped change adding optional secret-path intermediates and Fastly store-name mapping persistence. The CLI-validate ↔ runtime secret-walk symmetry is preserved with mirrored tests on both sides, and the reconciliation deliberately preserves undeclared-id and unrelated runtime entries for shared-store accounts. Two blocking findings around the new provision reconciliation and the deploy-time env overlay; the rest are non-blocking.
Findings
Blocking
- 🔧 Provision hard-fails in a previously-safe state: reconciliation errors when
edgezero_runtime_envis absent remotely even when there is nothing to write (crates/edgezero-adapter-fastly/src/cli.rs:3747) — see inline comment. - ❓ Scope and documentation of the deploy-time env overlay: the staging mirror applies all
EDGEZERO__STORES__*__NAMEprocess env vars, including undeclared ids, and hard-errors on malformed ones (crates/edgezero-adapter-fastly/src/cli.rs:3619) — see inline comment.
Non-blocking
- 🤔 Divergent invalid-value semantics between
EnvConfig::store_name(silent fallback) andruntime_store_name_entries_from_vars(fail closed) — see inline comment at cli.rs:3528. - 🤔 Case-normalization gap in
is_runtime_store_name_key— lowercase id segments pass validation but are dead at runtime — see inline comment at cli.rs:3498. - ♻️ Wrapper triplication in the
foo/foo_in/foo_with_cwdhelpers — see inline comment at cli.rs:3945. - ⛏ Dry-run noise: one "would remove … if a stale mapping is present" line per declared default-named store — see inline comment at cli.rs:3739.
- ⛏ Newly-public API keeps its internal doc voice:
env_config_from_runtime_dictionary— see inline comment at lib.rs:179. - 🌱 Derive support for optional intermediates:
SecretPathSegment::OptionalFieldis hand-written-meta-only; theAppConfigderive still emitsFieldfor every intermediate, so derived configs withOption<Struct>nesting cannot express this. The doc comment acknowledges it — worth a tracked follow-up to emitOptionalFieldforOption-typed nested fields. - 📝
#[non_exhaustive]semver note: it correctly forces the fail-closed wildcard arm inedgezero-cli, but adding#[non_exhaustive]to the existing publicSecretPathSegmentenum is a breaking change for any external code matching it exhaustively.
📌 Out of Scope
- Cloudflare parity: Fastly now persists non-default logical→physical mappings so its runtime resolves them; the Cloudflare adapter resolves
__NAMEat provision (binding writeback) but nothing persists the mapping for its runtimeEnvConfig. If that gap is real, it deserves its own issue.
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS (
cargo test --workspace --all-targets, including the workspace run the PR checklist left unchecked)
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Reviewed at 79628ecc. Three separable changes: the OptionalField secret-path segment threaded through core + CLI, env_config_from_runtime_dictionary made public, and Fastly provision / staged-deploy persistence of non-default logical-to-physical store-name mappings. The secret-path half stays CLI/runtime symmetric for the intermediate case it was written for. The blocking findings are on the mapping half: the reconciliation added since the last round now deletes production mappings while its desired state still comes from ambient process env, and there is a production/staging asymmetry in where the deploy-time overlay applies.
Findings below are new relative to both existing reviews (#5048264125 and #5027983720); items still open from the earlier round are listed separately at the end rather than re-raised as findings.
All five CI gates pass locally on 79628ecc.
😃 Praise
fake_fastly_runtime_mappingrecords$PWDper shell-out, so the new tests assert service-context resolution and not just call shape - see the inline comment at cli.rs:7583.runtime_store_name_reconciliation'sdeclared-scoped delete filter, with the shared-account rationale spelled out in the doc comment and proved by a negative assertion on unrelated runtime entries, is the right shape for a store multiple services can link.- Fail-closed wildcard arm in
edgezero-clifor the now-#[non_exhaustive]enum:Some((_unsupported, _)) => Err(...)rather than a silent skip.
Findings
Blocking
- 🔧
provisiondeletes live production store-name mappings when the override isn't exported - desired state comes fromEnvConfig::from_env(), so an unrelatedprovisionrun from a shell without the var exported silently repoints the ACTIVE version at the logical default store (cli.rs:3700, inline). - ❓ Deploy-time
__NAMEoverlay reaches staging only - the productiondeploypath is a barefastly compute deploypassthrough, so the same CI env yields different store resolution for the staged and production versions (cli.rs:3619, inline). - 🔧
OptionalFieldin terminal position silently overridesSecretField::optional, and both new leaf arms are untested - probed:optional: falseplus a terminalOptionalFieldreturnsOk(())where the all-required equivalent returnsConfigOutOfDate(extractor.rs:1006 and config.rs:1667, inline).
Non-blocking
- 🤔 CLI/runtime divergence on a scalar where an object is expected, immediately before an
OptionalField- runtimeOk(()), CLIErr("expected a table at \integrations`")`. Safe direction, but the comment asserting exact lockstep was deleted without note (extractor.rs:1030, inline). - ⛏
resolve_leaf's doc comment is now false -crates/edgezero-core/src/extractor.rs:1077-1080still says "The leaf's parent is a required intermediate, so a non-object parent is always an error". WithOptionalFieldthe leaf's parent may be optional. Every other comment in that function was updated in this PR; this one is outside the diff, hence no inline comment. - 🤔
push_entries_with_committer's failure text isconfig push-specific - a partial-write failure duringprovisiontells the operator to re-runconfig pushand explains content-addressed chunk keys (cli.rs:3754, inline). - 🤔 Uppercasing the logical id collides case-distinct declared ids -
ids = ["Sessions", "sessions"]passes manifest validation but shares one__NAMEkey, andEnvConfigcase-folds on lookup, so both resolve to one physical store (cli.rs:3635, inline). - ⛏ The
create_fastly_store_incwd change is broader than the feature - all three declared-kind creates move intomanifest_dir; only the runtime-env store's cwd is asserted (cli.rs:1480, inline). - 📝 PR description says "persist store mappings in local Fastly manifests" - the code persists them into the remote
edgezero_runtime_envconfig store, the one the ACTIVE version reads. Nothing writes[local_server.config_stores.edgezero_runtime_env]. A reviewer skimming the description would badly under-read the blast radius; worth correcting so the remote-write scope is visible up front.
📌 Out of Scope
- Local / Viceroy parity: nothing populates
[local_server.config_stores.edgezero_runtime_env], sofastly compute serveresolves logical ids while production now resolves__NAMEoverrides. Pre-existing (no__NAMEentries existed at all before this PR), but this change widens the practical gap between local and production store resolution. Deserves its own issue.
Still open from the earlier review (#5027983720) - listed, not re-raised
docs/guide/configuration.md:395-401still states thatOption<Inner>is unsupported and that a missing ornullintermediate is aConfigOutOfDateerror "bothconfig validateand the runtime reject". Both halves are now false forOptionalField.env::vars()(cli.rs:3619) still panics on a non-UTF-8 environment -vars_os()is the non-panicking form - and reading it inline still keepsmirror_production_to_stagingout of end-to-end test reach.#[inline]on the newly-pubenv_config_from_runtime_dictionary(lib.rs:177) buys nothing on a wasm target;pub+#[must_use]are the parts that serve the stated goal.
CI Status
Run locally against 79628ecc:
cargo fmt --all -- --check: PASScargo clippy --workspace --all-targets --all-features -- -D warnings: PASScargo test --workspace --all-targets: PASScargo check --workspace --all-targets --features "fastly cloudflare spin": PASScargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin: PASS
All five gates green, including the boxes the PR checklist leaves unchecked (cargo test --workspace --all-targets and the feature check). examples/app-demo was not run.
Verification note
The behavioral claims above - terminal-OptionalField overriding optional, and the scalar-parent divergence between the CLI and runtime walkers - were confirmed with temporary probe tests against 79628ecc, not inferred from reading. Actual output is quoted in the inline comments; the probes were reverted.
79628ec to
8773b05
Compare
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Two independent changes ride together here: an OptionalField segment for secret paths in core, and service-scoping of the Fastly edgezero_runtime_env Config Store (plus provision-time persistence of non-default store-name mappings). The Fastly half is the substantial one and is well-defended — fail-fast service-id resolution before any remote mutation, deletes confined to the current service namespace and to declared ids, and a fake-fastly oplog that asserts the exact keys and the cwd of every shell-out. All five CI gates pass locally.
Blocking on three correctness items and one question, all in the new code paths.
😃 Praise
- The reconciliation is conservative in exactly the right places.
runtime_store_name_reconciliationdeletes only keys that are both in the current service namespace and in the declared set, andprovision_reconciles_runtime_store_name_mappingsasserts negatively that another service's mappings, the legacy unscoped mapping, andEDGEZERO__LOGGING__LEVELall survive. That negative assertion is the one people skip, and it's the one that would catch a regression here. - Service-id resolution fails before any remote mutation.
provision_runtime_env_service_id_for_storesruns at the top ofprovision, andprovision_non_default_mapping_requires_service_id_before_fastly_mutationpins that ordering rather than just the error text. The manifest/FASTLY_SERVICE_IDmismatch check is a nice touch — refusing is better than picking a winner. - Splitting
commit_entries_with_committerso each caller owns its recovery text.provision_mapping_failure_recommends_provision_recoveryasserts the provision failure contains noconfig pushadvice and no chunk/root-pointer language. Factoring shared mechanics while keeping the operator-facing prose caller-specific is the right cut, and testing the absence of the wrong guidance is what makes it stick. deploy_staged_ignores_ambient_store_name_overrides— proving staging mirrors persisted production mappings rather than ambient process env is a subtle property and easy to regress.
Findings
Blocking
- 🔧 Legacy unscoped runtime-env entries are silently ignored on upgrade (
crates/edgezero-adapter-fastly/src/lib.rs:224) — an already-deployed service carrying a hand-written unscoped__KEY/__NAMEentry silently falls back to defaults on the next deploy and starts serving different config. Docs cover the manual migration; the runtime gives no signal. Suggested one-timelog::warn!inline. - 🔧
OptionalFieldleaf arm skips a malformed parent instead of rejecting it (crates/edgezero-core/src/extractor.rs:1006) —Value::getreturnsNonefor any non-object receiver, so a scalar parent passes the walk silently. Asymmetric with theFieldleaf arm, with the newOptionalFieldintermediate arm 23 lines below, and withcollect_secret_leafin the CLI — soconfig validatenow rejects what the runtime accepts. - 🔧 Service-prefix match is not on a segment boundary (
crates/edgezero-adapter-fastly/src/cli.rs:3595) —validate_service_idbans__but still accepts a trailing_, soEDGEZERO__SERVICES__SVC1__prefix-matches serviceSVC1_'s keys and pulls its overrides intoSVC1's staging twin. - ❓ Viceroy / local dev: panic risk and an undocumented local service id. On
wasm32-wasip1— EdgeZero's Fastly target —fastly::compute_runtime::service_id()isstd::env::var("FASTLY_SERVICE_ID").unwrap()(fastly 0.12.1,compute_runtime.rs:186). Locally theConfigStore::try_openguard succeeds via[local_server.config_stores.edgezero_runtime_env], soservice_id()is reached on every request. Two things I could not resolve from the diff:- Does the guest panic under Viceroy if
FASTLY_SERVICE_IDis unset? The new path has no guard, and the function's whole documented posture is fail-soft ("logs a warning and returns an emptyEnvConfig"). - What service id should an operator write into local keys?
docs/guide/blob-app-config-migration.md:265still describes the local block with no mention that its keys now need theEDGEZERO__SERVICES__<id>__prefix, and there's no documented way to learn Viceroy's id.
- Does the guest panic under Viceroy if
Non-blocking
- 🤔 A failed delete gets none of the recovery guidance the upserts get (
crates/edgezero-adapter-fastly/src/cli.rs:3843) — bare?after the carefully-worded upsert failure path, leaving a stale mapping live with no "re-run provision" hint. - 🤔
OptionalFieldis unreachable from the derive.crates/edgezero-macros/src/app_config.rs:227-252emits onlyFieldandArrayEach, and the variant's own doc says so. So "support optional typed secret paths" currently means "a hand-writtenAppConfigMetacan express one" — the derive-facing half of the feature isn't here. Is that a follow-up PR, or is hand-written meta the intended surface? Worth stating in the PR description either way. - 🤔
#[non_exhaustive]onSecretPathSegment(crates/edgezero-core/src/app_config.rs:35) is semver-breaking for downstream exhaustive matches and forces the dead_unsupportedarm atcrates/edgezero-cli/src/config.rs:1736. - ♻️
env_config_from_runtime_dictionarymadepub(crates/edgezero-adapter-fastly/src/lib.rs:204) with no in-repo caller beyondrun_app_with_request_extensions, documenting a custom-entry-point pattern nothing exercises. - ♻️ Stale unscoped-key docs outside this PR's file set. Two places still present the unscoped form as the Fastly override with no scoping note:
crates/edgezero-cli/src/templates/root/README.md.hbs:54-59— this is generated into every new project, and it names Fastly'sedgezero_runtime_envstore directly while showingEDGEZERO__STORES__CONFIG__APP_CONFIG__KEY=<name>.docs/guide/manifest-store-migration.md:89-91— the canonical env-var reference table.
- ⛏
RUNTIME_ENV_PREFIXused for the strip, then re-hardcoded in theformat!(crates/edgezero-adapter-fastly/src/lib.rs:70). - ⛏ Dry-run emits a possible-removal line per declared store (
crates/edgezero-adapter-fastly/src/cli.rs:3816) — 4 rows to 7 for a 3-store app, and the extra rows are hypotheticals.
📌 Out of Scope
- A store that is dropped from the app's declared ids while holding a non-default mapping keeps its scoped entry forever —
runtime_store_name_reconciliationonly considersdeclaredkeys for deletion. Documented as intentional ("undeclared ids ... are preserved"), and the alternative risks deleting a namespace the app no longer knows about, so leaving it. Might deserve aprovision --pruneescape hatch, or a status line naming the orphans, tracked separately. - Provision writes
__NAMEmappings from the process env at provision time while the runtime reads them from the Config Store. ChangingEDGEZERO__STORES__<KIND>__<ID>__NAMEand runningconfig pushwithout re-running provision leaves push and the runtime pointed at different stores. Pre-existing, and this PR narrows the gap rather than widening it.
CI Status
Run locally against origin/main merge-base 10263543:
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS — 0 failed - feature check (
--features "fastly cloudflare spin"): PASS - spin
wasm32-wasip2: PASS - additionally verified:
-p edgezero-adapter-fastly --target wasm32-wasip1 --features fastlyPASS, and--features cliwithoutfastlyPASS (the new#[cfg(any(feature = "cli", feature = "fastly", test))]gates hold in isolation).
|
Review-body-only follow-ups landed in c2bf557:
Validation completed:
|
c2bf557 to
055f7e9
Compare
Summary
This PR is stacked on #316 because Trusted Server currently pins that branch.
Changes
edgezero-coreSecretPathSegment::OptionalFieldand teach runtime extraction to skip absent or null optional containers atomically.edgezero-cliedgezero_runtime_env.edgezero-adapter-fastlyenv_config_from_runtime_dictionary, persist non-default logical-to-physical mappings underEDGEZERO__SERVICES__<SERVICE_ID>__*keys in the remoteedgezero_runtime_envConfig Store, and mirror the selected service’s persisted mappings into its staged runtime-env twin.Related
Test plan
cargo test --workspace --all-targetscargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcargo check --workspace --all-targets --features "fastly cloudflare spin"wasm32-wasip1(Fastly)examples/app-demoworkspace:cd examples/app-demo && cargo test --workspace --all-targetscd docs && npm run lint && npm run format && npm run buildedgezero serve --adapter axumcargo test -p edgezero-core,cargo test -p edgezero-cli,cargo test -p edgezero-macros, native Fastly adapter tests, Fastly WASM checks, live staged runtime mapping verification, and package-specific Clippy checks with-D warningsChecklist
{id}syntax (not:id) — no route changesedgezero_core(nothttpcrate)