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
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
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
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.
| 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.
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