Repository navigation
Bind Fastly stores per deployment environment - #381
Conversation
prk-Jr
left a comment
There was a problem hiding this comment.
🔧 Request changes — two reproduced defects
The logical resource-link and immutable-release design has strong publication safeguards, but selector precedence and the documented file-based config-push workflow need correction. The selector issue and a nonblocking cleanup-test gap are commented inline.
🔧 P2 — Downloaded release blocks file-based config push
.github/actions/config-push-fastly/scripts/config-push.sh:123
The documented consumer downloads the release into $GITHUB_WORKSPACE/app-release/. In a publisher checkout where that directory is untracked, assert_committed_source rejects the entire checkout even when the selected config file is committed and unchanged. A nested working-directory: publisher-config does not avoid this because the check climbs to the Git root.
Reproduced with a temporary publisher repository containing committed runtime.toml and an untracked app-release/app-release.tar.gz: the real script exits before invoking the application CLI with committed source is required; the working tree for '.' is dirty.
Suggested fix: Validate that the resolved, confined config file is tracked and unchanged, rather than rejecting unrelated release-download artifacts. Alternatively, consistently download releases outside the publisher checkout and update the documented workflow. Add a regression covering release download followed by file-based config push.
😃 Strong publication and recovery safeguards
Managed deployment revalidates exact resource links, package identity, source/draft state, and protected provider configuration before publication. Publishing independently validated version and package-digest outputs before rejecting the other output contract also preserves useful recovery information.
📝 Validation
- Rust formatting and all-features Clippy: passed.
- Workspace tests: passed after rerunning outside the sandbox, which initially denied local HTTP-server port bindings.
- Workspace feature check (
fastly cloudflare spin) and Spinwasm32-wasip2check: passed. - Deploy-core action suite: 623 passed, 0 failed, 5 platform skips.
- Release-core, build-cache, and GitHub Environment tests: passed.
- Linux-only package-release test skipped on this macOS runner.
- Documentation formatting, lint, and build: passed.
- GitHub PR checks: green at the reviewed head.
dhruv8sh
left a comment
There was a problem hiding this comment.
PR Review
Summary
The logical resource-link model and the immutable-release pipeline are well designed and defensively tested. I'm requesting changes for three verified regressions (a Fastly scaffold breakage and two CI gating problems) and for docs examples that fail or silently target the wrong store when copied as written. This review leaves out everything already raised in earlier rounds. Where it follows up on an existing thread, it replies there.
😃 Praise
prepare-release.shchecks the outer digest before anytarread, uses an exact member allowlist, rejectsl/hentries, re-checks confinement after extraction, and only then does an atomicmv.- Link deletion keyed on
(kind, alias)and re-resolved against the draft's own link IDs is correct whether or not Fastly keeps link IDs across clones. verify_exact_memberscombines an exact set, a non-following walk, bounded hashing and adversarial tests.- Checked selector resolution is consistent across push, diff, gc, provision and deploy, and never echoes the selector value.
Findings
Blocking
- 🔧 Empty
service_id = ""rejected: breaksprovisionand deploy preflight for every scaffold and for app-demo. Reproduced locally (crates/edgezero-adapter-fastly/src/cli.rs:3195) - 🔧 The required check
cargo testnever reports: the PR isBLOCKEDby the ruleset (.github/workflows/test.yml:18) - 🔧 Generated-project wasm checks are silently skipped because the
rustup target addstep was removed (.github/workflows/test.yml:95) - 🔧 The
--application-releaseexamples fail: the manifest must be inside the release root (docs/guide/cli-reference.md:174,docs/guide/adapters/fastly.md:178) - 🔧 "Optional"
vars.*selectors expand to blank and are rejected (docs/guide/deploy-action-adoption.md:328) - 🔧 A
__NAMEselector withoutexportsilently targets the default store (docs/guide/blob-app-config-migration.md:247) - ❓ An orphan draft beside a retired version is adopted as
InitialDraft(cli.rs:5884) - ❓ Deploy source selection doesn't enforce staging-record uniqueness, although the PR body and rollback both do (
cli.rs:5869) - ❓ Non-managed adapters silently ignore
--application-release(crates/edgezero-cli/src/adapter.rs:175)
Non-blocking
- 🤔 Non-canonical
EDGEZERO__STORES__*names are dropped silently byrun-app-cli.sh:86 - 🤔 The pre-publication barrier has no failing-case Rust tests (
cli.rs:1514) - 🤔 The test PATH lock is separate from the manifest lock, so env mutation is still unsound (
test_support.rs:115) - 🤔
release.jsonis opened before the file-type check, so a FIFO hangs the verifier (release.rs:108) - 🤔 The 14-day artifact retention limits rollback of an "immutable" release (
deploy-action-adoption.md:15) - 🤔 Logging migration for custom entrypoints and the env-var table (
adapters/fastly.md:64) - 🤔 The deprecated
keyinput's guidance points to__KEY, which Fastly rejects (deploy-github-actions.md:270) - 🤔
package-application-release-fastlyis never exercised end to end (deploy-action.yml:197) - ♻️ Redundant configuration snapshots (
cli.rs:976, also 1021 and 1442). Each snapshot is 4 GETs plus 28 logging GETs, about 32 curl calls, and roughly 6 run perCloneActivedeploy, so about 190 sequential calls. The snapshot at 976 is fully covered by the barrier, which compares a fresh read against the sameplan.source_configuration. The re-reads of the locked active source can't change, becauserevalidate_active_sourcealready rejects an unlocked source. Dropping both saves about 96 calls with no loss of safety. Also,InitialDraftSnapshot.configurationis alwayssource_configuration.clone()(851), so that field andsnapshot_initial_draft(1718) add nothing. - ♻️ Dead code left by the removal (
cli.rs:5207,cli.rs:4954). Thecwd: Option<&Path>parameter ofclassify_remote_config_store_with_cwdandcreate_config_store_entry_with_cwdis now alwaysNone. The doc atcli.rs:5198still describes staged config isolation, which no longer exists. Collapse the wrappers and delete that sentence. - ⛏ Duplicate branch in
select_version_source(cli.rs:5908). Theif staging_versions.len() > 1 || drafts.len() > 1 { return Err(X) }is followed by an unconditionalErr(X)with the same message. Delete theif. - ⛏ Two new provision tests depend on the ambient environment (
cli.rs:9280,cli.rs:9319). Unlike the neighbouring tests, they don't clearFASTLY_SERVICE_IDwithEnvOverride::remove, so they behave differently on a machine where it is exported. - ⛏ Stale comments.
crates/edgezero-adapter-fastly/src/lib.rs:4sayschunked_configis only compiled for the CLI push/GC path, but thefastlyruntime uses it too (config_store.rs:7).request.rs:328says the bare-handle path "ignores those selectors", but no selectors are left. - ⛏ A contract test depends on locale (
.github/actions/deploy-core/tests/run.sh:2008).parse_action_surface … | sortgives a different order underen_US.UTF-8(retry-delaybeforeretry), so "healthcheck-fastly public surface" fails locally and passes on CI only because runners useC.UTF-8. UseLC_ALL=C sort. - ⛏ Two release-core rejection tests pass on any failure (
.github/actions/release-core/tests/run.sh:62-67). The wrong-adapter and wrong-protocol cases should capture stderr and assert the "unsupported format, lifecycle protocol, or adapter" reason. - ⛏ Broken anchor (
docs/guide/deploy-action-adoption.md:134).#migrating-a-custom-entry-pointshould be#migrating-a-custom-entrypoint, matching the heading atadapters/fastly.md:64. - ⛏ The migration mapping only covers
deploy-fastly(docs/guide/deploy-action-adoption.md:105).config-push-fastlyalso lostapp-cli-artifact,app-cli-binandmanifest.healthcheck-fastlyandrollback-fastlylostapp-cli-artifactandapp-cli-bin. All four now requireapp-release-archive,app-release-sha256andexpected-source-revision. - ⛏ The recovery helper runs a CLI it hasn't verified (
.github/actions/deploy-fastly/tests/recovery-active-version.sh:4-5, 19-31). The header says "already authenticated immutable release", but it extracts and runscli/app-cli.tarwithout checking the outer SHA-256. It's a test helper, but it models the operator runbook. Runprepare-release.shwith the expected digest first. - ⛏ Duplicate matrix work (
.github/workflows/test.yml:61, 85). Theworkspaceleg'scargo test --workspace --all-targetsalready runs the default-featureedgezero-cliand Axum tests that thecliandaxumlegs repeat. - 🌱 The paginated inventory has no page cap and no unit tests (
cli.rs:581-613).collect_paginated_store_inventoryonly stops on a repeated cursor. A provider or proxy returning ever-new cursors loops forever and growsrecordswithout bound. Add a cap that fails closed ("completeness cannot be proven"), plus closure-driven tests: two pages, a repeated cursor, and a cursor with+or/. - 🌱 Harden the GitHub Environment lookup's curl (
.github/actions/require-github-environment/scripts/require-environment.sh:33). Add-q(so a runner's~/.curlrccan't enable tracing of the bearer header),--proto =https,--max-time 30and--path-as-is, and reject./..segments inrepository. - 📝 Public API.
AdapterDeployContextandDeployStoreIdsare new public structs with public fields and no#[non_exhaustive], so every field added later breaks downstream struct literals. Worth deciding while the version is 0.1. - 📝 Breaking change for release notes. Fastly deploys of apps that declare stores now always take the managed path and require
--application-release, bypassing[adapters.fastly.commands].deploy. That breaks localedgezero deploy --adapter fastly.
Follow-ups on existing threads (posted as replies):
- Target-cache fix incomplete.
restore-keyscan no longer match the new key shape (setup-rust-build-cache/action.yml:48). - The selector-case precedence bug has wider scope. It also affects
config gc --yes, diff, validate, provision, every adapter, and__KEY(config.rs:1399). - Is
[setup]still used under managed deploy? (cli.rs:2167)
📌 Out of Scope
actions-rust-lang/setup-rust-toolchain@v1is still unpinned. This was already tracked in the first review round.
CI Status
Run locally on the pinned Rust 1.95.0 at 61e8444:
- fmt: PASS
- clippy (
--all-features -D warnings): PASS. It fails on rustc 1.98 because of the newinline_trait_boundslint, which is a toolchain issue, not a PR issue. - tests (
--workspace --all-targets): PASS - feature check (
fastly cloudflare spin): PASS - spin
wasm32-wasip2check: PASS - action suites:
release-core,setup-rust-build-cache,require-github-environmentandpackage-application-release-fastlyPASS.deploy-coregives 640/640 underC.UTF-8and 639/640 underen_US.UTF-8(the locale nit above). - actionlint (
-S warning), shellcheck and zizmor (repo config): clean - GitHub:
BLOCKED, because the requiredcargo testcontext is missing.
|
Addressed the remaining review findings in The non-inline config-push blocker is fixed as well: file-based push now validates and confines the selected config file, then requires that exact file to be tracked, committed, and unchanged. An unrelated untracked authenticated application-release download no longer makes the checkout fail the config-push preflight. The action suite includes that layout as an executable regression. I also merged current Local validation after the merge and fixes:
The Linux-only release-packaging script test remains platform-gated on this macOS checkout and is running in CI through the public composite-action smoke jobs. |
|
Follow-up: the public-packager smoke handoff is fixed in The complete PR check rollup is now green, including production, staging, recovery, store-free deployment, config push, the required |
prk-Jr
left a comment
There was a problem hiding this comment.
🔧 Reviewed PR #381 at 3c82f33. Requesting changes for one reproduced artifact correctness issue: build-app-cli can publish an older cached executable instead of the executable Cargo just built.
😃 The immutable-release verifier has strong adversarial coverage for member digest mismatches, symlinks, extra members, and loaded-manifest identity. The final publication barriers also verify package and resource-link identity before publication.
📝 Verification: formatting, all-features Clippy, workspace tests (1,492 passed; 1 ignored), Fastly/Cloudflare/Spin feature compilation, and Spin wasm32-wasip2 compilation passed locally. The workspace tests required loopback networking permission. The action harness passed 638 tests with 5 platform skips; build-cache tests, Actionlint, ShellCheck, and documentation format/lint/build passed. All GitHub checks were successful on the reviewed head. The stale-artifact reproduction used real Cargo on macOS, with only Linux runner detection and toolchain installation shimmed.
A configured Cargo target (CARGO_BUILD_TARGET or build.target) moves the fresh executable to <target-dir>/<triple>/release, but build-app-cli always packaged <target-dir>/release. With a restored cache, an older root-level executable still passed --help and was published as the current CLI. Pass the toolchain's host triple as --target, which overrides any configured target, and package the executable from that triple's directory.
prk-Jr
left a comment
There was a problem hiding this comment.
📝 Reviewed PR #381 at c2e862f. Two introduced migration-runbook errors prevent the documented Cloudflare and Axum secret setup from working. No additional blocking runtime or action defects were confirmed.
😃 The deployment wrapper preserves independently valid recovery outputs when another output contract fails, and the tests cover provider exit-status preservation. This helps operators reconcile a deployment that may already have mutated provider state.
📝 Validation: all five required Rust gates passed (workspace tests: 1,492 passed, 1 ignored); action contract suite: 639 passed, 0 failed, 5 macOS platform skips; release verification, cache/environment tests, actionlint, ShellCheck, and docs format/lint/build passed. Linux-only packaging and live provider behavior were not exercised locally. Reported GitHub checks passed.
Merging main (9da604f) resolved five guide files to this branch's side and dropped the corrections #373 made against the code. Restore each one that still matches the code here: the Fastly manifest version, provision behavior, build/serve commands, proxy import path, and streaming note; Cloudflare local push by binding; the serve fallbacks, healthcheck outputs, push outputs, and manifest/service-id rows in the CLI reference; and the bare-handle constructors and `wrangler kv key put` spelling in the store migration guide. Also correct the local secret commands that never worked: `wrangler secret put` takes no `--binding`, Fastly CLI 15.1.0 reads secret values from `--stdin` or `--file`, and Spin reads `SPIN_VARIABLE_<NAME>` rather than the bare variable name. Note the runner 2.327.1 requirement for the Node 24 producer actions and drop stale overlay references from the Fastly request rustdoc.
The example returned `text(...)`, which does not exist, so uncommenting it in a generated app failed with E0425. Return `Text<&'static str>` like the template's other handlers.
The Axum dev server looked a secret key up as an environment variable of the same name, so `api_token = "demo_api_token"` read `demo_api_token`. That breaks env-var conventions and lets any inherited variable whose name matches a key answer for it silently. Secret key `k` now resolves from `EDGEZERO__SECRETS__` plus `k` in ASCII uppercase, e.g. `EDGEZERO__SECRETS__DEMO_API_TOKEN`. One rule in `secret_env` serves both the runtime store and `config validate`/`push`: a key must be ASCII letters, digits, or `_`, and two different keys that differ only in case are rejected because they would share a variable. The bare variable is no longer read. Update the Axum guide, CLI reference, migration guide, the generated README and app config template, the core rustdoc, and the smoke scripts.
dhruv8sh
left a comment
There was a problem hiding this comment.
PR Review
Summary
The descriptor removal and logical-alias binding are well executed. CI is green, and locally fmt, clippy, workspace tests, and both feature/wasm checks pass on the pinned 1.95.0 toolchain. This review covers blocking items only.
😃 Praise
- The immutable-release verification in
release.rs/prepare-release.shis rigorous: it rejects path traversal and symlinks, denies unknown fields, verifies per-member digests on a private copy, and has adversarial tests. - Resource links are checked for exact map equality at the clone, reconciled-draft, and final stages. Every Fastly CLI call passes argv as an array, with no shell interpolation.
Findings
Blocking
- 🔧 Broken anchor: the adoption guide links a heading slug that does not exist (
docs/guide/deploy-action-adoption.md:139) - ❓ 14-day artifact retention vs. rollback window: the hard-coded retention can expire the archive that rollback needs (
.github/actions/package-application-release-fastly/action.yml:115,.github/actions/build-app-cli/action.yml:282) - ❓ Cross-kind resource-link name collisions: reconciliation keys links by
(kind, alias)(crates/edgezero-adapter-fastly/src/cli.rs:670)
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS
Problem and resulting behavior
PR #344 made Fastly store selection depend on a shared
edgezero_runtime_envConfig Store and service/version-scoped descriptor keys. That added mutable shared state, put Fastly service IDs into configuration names, and made application runtime code participate in deployment-target selection.This change removes that descriptor architecture. Applications keep stable logical store IDs in
edgezero.toml. The selected deployment environment chooses physical Config, KV, and optional Secret Stores through canonicalEDGEZERO__STORES__...__NAMEvariables. Before publication, managed Fastly deployment binds those physical resources to the logical aliases on the unpublished service version.Production and staging can select the same or different physical stores while deploying the same immutable application release. A logical Config Store always uses its logical ID as the entry key. For example, production and staging can select
config-prodandconfig-stage, while both versions expose the alias and keyapp_config.Changes
edgezero_runtime_env, runtime descriptors, service/version selector keys, staging selector stores,EDGEZERO_FASTLY_BUILD_SETTINGS, and all compatibility behavior for the Support optional typed secret paths and Fastly store mappings #344 contract.__NAMEvariables withparent environment > manifest default > logical IDprecedence for deploy, push, diff, GC, and provision. Normalize recognized selector casing before merging layers, reject noncanonical action inputs, and fail on present blank or invalid selectors before provider I/O.config gc --no-envstill forces the logical fallback.config,kv-store, andsecret-store.[setup]is written only when physical and logical names match. A different physical store requires a selected existing service; provision creates the physical resource and reports the logical resource-link operation without writing a misleading setup alias.service resource-link ...andservice version ...hierarchy, retaining the reviewed Fastly CLI 15.1.0 pin.service_idas unselected rather than as an invalid configured ID.pubsub,logentries, ands3.edgezero.toml, package, and selected adapter manifest byte-identical across publishers and targets. A Fastly release includes its adapter manifest at the relative path declared inedgezero.toml.config push/diff --adapter <name>validation to the selected adapter while retaining whole-projectconfig validate.release.jsonis checked as a confined regular file before opening, and lifecycle actions copy the archive once before authenticating, inspecting, and extracting that same copy.deploy-core/scripts/run-app-cli.sh; thin Fastly wrappers own Fastly arguments, credentials, and output policy.--packageargument. Managed immutable-release deployment rejects every--package/-pspelling because the release owns the package path, and adapters without managed-release support reject--application-releaseinstead of ignoring it.build-app-clitoapp-cli.tarwhile preserving the configurable artifact name, application binary/package names, and immutable membercli/app-cli.tar.setup-rust-build-cacheaction. Its requiredapp-namescopes Cargo sources, sccache, and optional target artifacts; the target key is stable for an application, runner, architecture, and lockfile, and a two-job smoke proves a real restore..githubwith Zizmor, restore the requiredcargo testaggregate check, install every generated-project WASM target in CI, minimize workflow credentials, and keep action tooling free of Python and pip.edgezero_runtime_envcutover, staging config re-push under the fixed logical key, optional selector exports, cross-repository release retrieval, mutable-config recovery, and the distinction between a real application domain and its GitHub Environment name.build.target.EDGEZERO__SECRETS__<KEY>, with the key in ASCII uppercase, instead of from a variable named exactly like the key.config validate/pushrejects keys outside ASCII letters, digits, and_, and distinct keys that collide once uppercased.Fastly does not expose a publication compare-and-swap token. Deployments and other version mutators for one service must share caller-side serialization. The source/clone and final snapshot checks reject observed interference; serialization closes the remaining provider time-of-check/time-of-use window.
Downstream consumers of the build action must consume
app-cli.tar.Local Axum runs must set secrets as
EDGEZERO__SECRETS__<KEY>; a variable named exactly like the key is no longer read.Closes #380
Validation
cargo test --workspace --all-targetscargo clippy --workspace --all-targets --all-features -- -D warningscargo check --workspace --all-targets --features "fastly cloudflare spin"cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spincargo fmt --all -- --checkandgit diff --check.github/actions/deploy-core/tests/run.sh— 639 passed, 0 failed, 5 expected platform skips.github/actions/setup-rust-build-cache/tests/run.sh.github/actions/package-application-release-fastly/tests/run.sh— exercised on Linux CI; non-Linux runners skip itzizmor --offline .githubrg 'edgezero-cli\\.tar'returns no product-contract references