Repository navigation
Support implicit credential lookup plans and shared Fastly secret operations - #444
ChristianPavilonis wants to merge 10 commits into
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Adds application-compiled implicit secret lookup plans, with native-name rendering and collision checks across typed config validate / push / diff, plus shared host-side Fastly secret operations. I found no blocking issues. Two behaviours are worth confirming (inline), plus a few nits.
😃 Praise
- The credential handling is careful. The token and secret value reach
curlonly through the stdin config. Provider bodies and stderr are kept out of errors. Writes use explicit POST (create) or PATCH (replace) and are never retried. A per-key 404 isn't taken as proof of absence, and repeated pagination cursors are caught. The fake-curlsubprocess test shows the token stays off argv and leaked stderr is stripped from errors.
Findings
Non-blocking
- 🤔 Shared store/key across descriptors is always rejected, with misleading messages (
crates/edgezero-cli/src/config.rs:1991,:2006). Inline. - 🤔 Opting in skips all legacy
#[secret]leaf checks (crates/edgezero-cli/src/config.rs:1846). Inline. - 🌱 No derive-level opt-in.
#[derive(AppConfig)]writes the wholeAppConfigMetaimpl, so opting in means hand-writing the root impl and losing the derive's compile-time rejections (e.g.rename_all). A derive attribute that forwards to app-supplied functions could come later. - ⛏ Stray
\'in an untouched doc comment (crates/edgezero-core/src/app_config.rs:141). Inline. - ⛏ Read-only Fastly calls report "mutation outcome may be unknown" (
crates/edgezero-adapter-fastly/src/secret_operations.rs:246). Inline. - ⛏
#[path = "secret_operations.rs"] pub mod secretsname mismatch (crates/edgezero-adapter-fastly/src/cli.rs:7). Inline. - 📝 The test-plan section reports gates at
5ad74df2/4f645b1e. The head now has two more commits (c24cd994,47784581). Hosted CI and the local run below cover the current head, so this only needs a description refresh.
CI Status (reviewed head 47784581, local Rust 1.98.1 per .tool-versions)
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS (1,565 tests) - check (
--features "fastly cloudflare spin"): PASS - Hosted CI: all 28 checks pass, including the WASM jobs.
Not covered here: whether Fastly really accepts these API calls, and the companion application integration (both noted as unverified in the PR).
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Adds application-compiled implicit secret lookup plans, applied by typed config validate / push / diff, plus shared host-side Fastly secret operations. Requesting changes for two items. The new Fastly transport test races the rest of the crate's env-mutating tests and can run the real fastly CLI. Five of the new guards can be deleted with every test still passing.
Findings
Blocking
- 🔧 New fake-curl test uses a different env mutex than the rest of the Fastly test binary: reproduced 2 of 18 failing runs (0 of 12 with the test skipped), including the real
fastly compute deployrunning from a unit test (crates/edgezero-adapter-fastly/src/secret_operations.rs:626) - 🔧 Five new guards are untested, and the push arm of the all-consumers test can't fail: verified by deleting each guard (
crates/edgezero-cli/src/config.rs:5708)
Non-blocking
- ♻️ Fastly special case in the adapter-agnostic CLI: move it to an adapter hook (
crates/edgezero-cli/src/config.rs:1983) - 🤔 Cloudflare native names are never validated (
crates/edgezero-cli/src/config.rs:2016) - ♻️ Resource resolution re-implements deploy and skips
from_inventories(crates/edgezero-adapter-fastly/src/secret_operations.rs:155) - 🤔 Secret operations require all declared Config/KV stores to exist (
crates/edgezero-adapter-fastly/src/secret_operations.rs:168) - 🤔 Errors drop store identifiers that aren't secret (
crates/edgezero-adapter-fastly/src/secret_operations.rs:193) - 🤔 A definite 4xx rejection is reported as an unknown outcome (
crates/edgezero-adapter-fastly/src/secret_operations.rs:371) - ♻️
ImplicitSecretLookupAPI shape:#[non_exhaustive], three-state return (crates/edgezero-core/src/app_config.rs:94) - ♻️
context_from_manifestduplicatesrun_deploy's context builder (crates/edgezero-cli/src/fastly_secrets.rs:26) - 🌱
secret_existslists the whole store per key (crates/edgezero-adapter-fastly/src/secret_operations.rs:266) - ⛏ Glob re-export widens the public API (
crates/edgezero-cli/src/fastly_secrets.rs:4) - ⛏ Axum native names may start with a digit (
crates/edgezero-adapter-axum/src/cli.rs:366) - ⛏
base64dependency ordering (Cargo.toml:46) - 🌱 Docs:
docs/guide/configuration.mddocuments#[secret]and hand-writtenAppConfigMeta, but not the opt-in hooks (implicit_secret_lookups/render_implicit_secret_name) or theedgezero_cli::fastly_secretshelpers. A short section would help the companion's reviewers, and later adopters, find the contract.
The two existing 🤔 threads from prk-Jr (descriptors sharing a credential; opting in skips the legacy #[secret] checks) are still open, and I agree with both.
📌 Out of Scope
push_typed_honours_key_override(crates/edgezero-cli/src/config.rs:4721) runs a push withoutmanifest_guard(), while guarded tests such asconfig.rs:2601setEDGEZERO__STORES__CONFIG__APP_CONFIG__NAME="". This already exists onmainand isn't touched by this PR, but it failed 3 times during these runs. Worth a separate issue.
CI Status (reviewed head 47784581, local Rust 1.98.1)
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS (1,565). Theedgezero-adapter-fastlylib suite fails intermittently (2 of 18 runs) because of the first blocking finding. - check (
--features "fastly cloudflare spin"): PASS - WASM checks (spin
wasm32-wasip2, fastlywasm32-wasip1, core and cloudflarewasm32-unknown-unknown): PASS - Hosted CI: 28/28 checks pass
|
Addressed the approved review recommendations in 04d58ee and refreshed the PR description with checks for that head. Each inline request has its own detailed reply. Summary-only points:
Re-requesting review from both reviewers after resolving the handled threads. No merge or release. |
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Follow-up review of 217983c0. The changes in 04d58ee4 address both blocking findings from the previous round and the non-blocking threads, either with a fix or with a documented, reasoned decision. The merge from main (217983c0) only touches CI/scripts. I found no blocking issues, so approving. One nitpick and one seed are inline.
😃 Praise
- The credential handling survived the refactor intact. The token and secret value reach
curlonly through the stdin config. Path inputs are validated or percent-encoded, and redirects andcurlrcare disabled. Provider bodies and stderr never reach errors, and writes are never retried. Explicit POST/PATCH keeps create vs replace safe against races.
Confirmed fixes
- Shared env lock:
path_mutation_guard()now returnsenv_lock(), with a pointer-identity test. The Fastly CLI lib suite passed 5 consecutive parallel runs (337 tests each). - Guard coverage: the push arm now sets
yes = trueand every consumer asserts the specific diagnostic. As a spot check, I disabled the per-store namespace branch incheck_implicit_adapter, andimplicit_fastly_detects_physical_store_alias_collisionsandimplicit_namespace_policy_is_adapter_ownedboth failed. - Adapter-owned
secret_names_scoped_per_store(). Resolution now reuses deploy'sfrom_inventories/desired_resource_links. The deploy-context builder is shared.ImplicitSecretLookupis#[non_exhaustive]with a constructor. 4xx rejection vs uncertain outcome is now distinguished. Re-exports are explicit. Axum leading-digit check, docs section and nits all done. - The deferred or declined items (combined trait, secret-only resolver, batch listing, Cloudflare name rule, shared physical credential) have reasoned explanations and are documented in the guide. That's fine for this prerequisite.
Findings
Non-blocking
- ⛏ Spin legacy-path messages now mention native names (
crates/edgezero-adapter-spin/src/cli.rs:546,:552). Inline. - 🌱 No length cap on Fastly native names (
crates/edgezero-adapter-fastly/src/cli/secrets.rs:229). Inline.
CI Status (reviewed head 217983c0, local Rust 1.98.1 per .tool-versions)
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS (1,582) - check (
--features "fastly cloudflare spin"): PASS - Spin
wasm32-wasip2check: PASS - Hosted CI: all checks pass
Not covered here: whether Fastly really accepts these API calls (including the exact 64KB boundary), and the companion application's runtime integration. The PR also lists both as unverified.
| let reason = spin_key_rule_violation(&spin_var); | ||
| return Err(format!( | ||
| "`#[secret]` field `{field}` does not reference a valid Spin variable name. {reason}. Pick a secret reference that conforms; its value is redacted.", | ||
| "secret lookup at `{field}` does not reference a valid Spin variable name. {reason}. Use a conforming native name; the name is redacted.", |
There was a problem hiding this comment.
⛏ validate_typed_secrets also serves legacy #[secret] apps (where native_name is None), but both messages now only describe the opt-in path. A legacy app whose secret reference isn't a valid Spin variable is told to "Use a conforming native name", which it can't set. The collision message at line 552 also lost the "after lowercasing" hint, which was the only clue to why two distinct references like Token and token collide. Only the wording is affected; the checks themselves are unchanged.
Suggested wording that covers both paths:
- Line 546:
... Use a conforming secret reference or native name; the name is redacted. - Line 552:
... reference the same Spin variable (secret references are lowercased); Spin's flat variable namespace cannot disambiguate them. Use distinct secret references or native names; the names are redacted.
There was a problem hiding this comment.
⛏ Agree. Both Spin messages changed in 04d58ee4, so the legacy-path regression is introduced by this PR and worth fixing here; the suggested wording covers both paths.
| Ok(()) | ||
| } | ||
|
|
||
| pub(super) fn validate_key(key: &str) -> Result<(), String> { |
There was a problem hiding this comment.
🌱 validate_key now also backs config validate's native-name check for Fastly, but it only checks the alphabet, not the length. If Fastly caps secret-name length, an over-long rendered name would pass preflight and only fail at write time. I haven't confirmed the provider's limit, so this is just something to consider later.
There was a problem hiding this comment.
🌱 Agree. Fastly's secret-store reference documents the allowed characters but no maximum name length, so a cap needs a provider rehearsal before it is added.
| `Replace` uses PATCH. The provider enforces the selected action without a | ||
| delete/recreate gap. Injectable `_with_transport` variants support fictional tests. |
There was a problem hiding this comment.
🤔 Fastly doesn't document a gap-free replace
Fastly describes PATCH only as "Recreate a secret based on the secret's name. Returns an error if there is no existing secret with the same name." It says nothing about whether recreation is atomic for concurrent readers. EdgeZero's own guarantee is narrower: it never deletes first and never falls back from Replace to Create. As written, operators could read this as a zero-downtime rotation promise.
| `Replace` uses PATCH. The provider enforces the selected action without a | |
| delete/recreate gap. Injectable `_with_transport` variants support fictional tests. | |
| `Replace` uses PATCH. POST fails if the name exists and PATCH fails if it does | |
| not; EdgeZero never deletes first or falls back between them. Fastly does not | |
| document whether recreation is atomic for concurrent readers, so that remains | |
| unverified. Injectable `_with_transport` variants support fictional tests. |
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Thanks for this. The Fastly secret operations are carefully built; one blocking question and a few design points below.
CI locally (cargo +1.98.1): fmt pass, clippy (--all-features -D warnings) pass, cargo test --workspace --all-targets 1582 passed, 0 failed.
Blocking
- ❓
--no-envis ignored by the implicit secret preflight (inline onconfig.rs:1855/:1864).
Non-blocking
- 🤔 Implicit lookups disable all legacy
#[secret]checks (inline onapp_config.rs:146). - 🤔 The new hooks can't be used with
#[derive(AppConfig)]; the docs admit the derive can't forward them, so adopters must hand-write all ofAppConfigMetaand lose the derive's compile-time checks. That's a lot of new public API acrossedgezero-core,edgezero-adapterandedgezero-clifor one consumer (trusted-server#1037). Please add a derive attribute such as#[app_config(implicit_secrets = path)], or open a follow-up issue. - 🤔 64KB secret cap boundary unverified (inline on
secrets.rs:20). - ⛏ Spin error wording now misleads apps not using the feature (inline on
spin/src/cli.rs:546/:552). - ⛏ The Fastly read paths (
resolve_resources,secret_exists,secrets.rs:178-184,:293) return a bare "Fastly secret API returned HTTP 429" with no retry hint; the "never retried" docs cover writes only. A doc line on read retry expectations would help. - 🌱
secret_exists(secrets.rs:279) pages the whole store per key, O(n) per lookup. Fine given batch listing is deferred; just noting for later.
😃 Nice work
- Leak-conscious helpers: no
Debugon responses, provider bodies/stderr dropped from errors, token and body passed to curl via stdin. - POST/PATCH for create/replace avoids a delete-then-recreate gap; uncertain outcomes are clearly distinguished from explicit HTTP rejections.
- Duplicate lookups matched by descriptor, store and key, and the Fastly check catches two logical stores mapping to the same physical store, both well tested.
deploy_context_from_loaderextraction and moving Fastly tests onto the shared env lock are good cleanups.
No WASM/Tokio concerns: base64 is only pulled in by the Fastly cli feature, new code is properly feature-gated, and tests need no network or credentials.
| let environment = effective_manifest_environment( | ||
| ctx.manifest(), | ||
| adapter.name(), | ||
| env::vars(), |
There was a problem hiding this comment.
❓ typed_secret_checks always builds the renderer environment from manifest defaults plus env::vars(), but config push --no-env pushes against an empty environment (config.rs:393-397). So push --no-env can validate against one physical store name and push to another, and parent EDGEZERO__STORES__SECRETS__* vars leak into the collision check despite --no-env.
Diff (:650) already used env::vars() before this PR, and implicit_consumers_pass_effective_manifest_environment_to_native_renderer runs diff with no_env: true expecting manifest defaults, so this may be deliberate. Could you either thread the caller's no_env through and use the same empty environment push uses, or explain why preflight should ignore the flag?
| } | ||
| AdapterValidationScope::Selected(adapter) => { | ||
| let environment = | ||
| effective_manifest_environment(ctx.manifest(), adapter.name(), env::vars()); |
There was a problem hiding this comment.
❓ Same --no-env concern as above: this env::vars() call should honour the caller's no_env so preflight and push resolve the same physical store names.
| /// # Errors | ||
| /// Returns a credential-free application compilation diagnostic. | ||
| #[inline] | ||
| fn implicit_secret_lookups(&self) -> Result<Option<Vec<ImplicitSecretLookup>>, String> { |
There was a problem hiding this comment.
🤔 Returning Some(..) here, even an empty list, skips the non-empty, declared-store and store-reference checks for every #[secret] field. It is documented, but an app that opts in silently loses that safety net. Suggest keeping the old checks for any secret field the plan doesn't cover, or warning when secret_fields() is non-empty under an implicit plan.
| /// Local maximum raw bytes, before Base64 encoding. Fastly documents "64KB" | ||
| /// without specifying whether it means 64,000 or 65,536 bytes; provider boundary | ||
| /// acceptance remains unverified. | ||
| pub const FASTLY_SECRET_MAX_BYTES: usize = 0x0001_0000; |
There was a problem hiding this comment.
🤔 The local cap is 65,536 bytes, but Fastly documents the limit only as "64KB". If Fastly means 64,000, values between 64,001 and 65,536 bytes pass local validation and then fail remotely. Suggest using the stricter 64,000 until the real boundary is confirmed.
| let reason = spin_key_rule_violation(&spin_var); | ||
| return Err(format!( | ||
| "`#[secret]` field `{field}` does not reference a valid Spin variable name. {reason}. Pick a secret reference that conforms; its value is redacted.", | ||
| "secret lookup at `{field}` does not reference a valid Spin variable name. {reason}. Use a conforming native name; the name is redacted.", |
There was a problem hiding this comment.
⛏ This wording ("secret lookup at … Use a conforming native name") now also applies to apps that don't use implicit lookups, which have no native name to change. Suggest choosing the message based on whether entry.native_name is set.
| if let Some(prev_field) = seen.insert(spin_var.clone(), entry.field_name.as_str()) { | ||
| return Err(format!( | ||
| "`#[secret]` fields `{prev_field}` and `{this_field}` reference the same Spin variable after lowercasing; Spin's flat variable namespace cannot disambiguate them. Pick distinct secret references; their values are redacted.", | ||
| "secret lookups at `{prev_field}` and `{this_field}` reference the same Spin variable; Spin's flat variable namespace cannot disambiguate them. Use distinct native names; the names are redacted.", |
There was a problem hiding this comment.
⛏ Same as above: the native-name advice only fits implicit lookups; keep the #[secret] field wording when entry.native_name is unset.
|
CREATOR: From the server-side identity plan (IABTechLab/trusted-server#1559, |
Summary
Companion prerequisite for IABTechLab/trusted-server#1037. This does not implement or prove that project's runtime secret-scoping guarantee.
Changes
No deploy descriptors, registry, raw-key fallback, release tags, merging, or production provider calls.
API
AppConfigMeta::implicit_secret_lookups(&self) -> Result<Option<Vec<ImplicitSecretLookup>>, String>Each lookup has
descriptor_id,field_path,store_id, andkey.Noneretains the old leaf walk;Some(vec![])means no active credentials. The application compiler owns activation and complete descriptor coverage.AppConfigMeta::render_implicit_secret_name(&self, adapter, lookup, environment: &EnvConfig) -> Result<String, String>The application supplies its runtime bridge's native name, not EdgeZero's generic mapping. Plans are public metadata, not resolved credentials or authorization.
Host helpers are under
edgezero_cli::fastly_secrets:context_from_manifest,resolve_resources,secret_exists,write_secret, injectable_with_transportvariants, comparable resource metadata, andFASTLY_SECRET_MAX_BYTES. Explicit create/replace methods preserve the reviewed action on races. Errors do not expose provider response bodies or stderr, writes are not retried, and unconfirmed writes report uncertainty.Review follow-up
Addressed review feedback in
04d58ee4: unified test environment locking; made negative tests diagnostic-specific; corrected full-purpose lookup deduplication; added the lookup constructor and adapter namespace hook; reused deploy resource/context resolution; improved safe diagnostics and explicit HTTP rejection handling; documented opt-in and host-helper contracts.Implicit plans continue replacing legacy CLI leaf checks. Full Fastly resource resolution still requires all declared stores. The combined plan trait redesign, derive forwarding and batch secret listing are deferred. No guessed Cloudflare identifier rule was added after primary-source inspection and a local workerd binding probe.
Test plan
Rust 1.98.1 and Node.js 24.20.0 on Linux x86_64. Two Cargo jobs, dev/test debug info disabled. All results below cover
04d58ee4. Temporary mutation probes were restored to exact verified SHA-256 file fingerprints before committing.cargo test --workspace --all-targets -- --test-threads=1: 1,582 tests passed. Serial workspace execution avoids the separately reported pre-existing CLI environment race.cargo test -p edgezero-adapter-fastly --features cli --lib: 10 consecutive parallel runs passed, 337 tests each.cargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkandgit diff --checkcargo check --workspace --all-targets --features "fastly cloudflare spin"wasm32-wasip2, Fastlywasm32-wasip1and Cloudflarewasm32-unknown-unknownadapter checks with their corresponding runtime features.npm ci, format, lint and VitePress build.Unverified and blocked
The provider API documentation specifies POST create, PATCH replacement and a raw value limit written as "64KB". The exact 64,000 versus 65,536-byte provider boundary remains unverified. Host tests verify request construction and subprocess argv/stdin handling with fictional inputs, not real provider acceptance, replacement propagation, read-through-runtime byte preservation, native declaration limits, or config-writer/secret-writer IAM separation. No real credentials or provider mutations were used.
The consuming application's actual renderer/CLI integration and four-adapter runtime proof belong to the companion change. Successful storage never implies a deploy, resource link, activation, retained-state refresh or immediate fleet-wide rotation.
Checklist