Skip to content

Add Fastly deploy action with config push#303

Open
ChristianPavilonis wants to merge 1 commit into
mainfrom
feature/edgezero-actions
Open

Add Fastly deploy action with config push#303
ChristianPavilonis wants to merge 1 commit into
mainfrom
feature/edgezero-actions

Conversation

@ChristianPavilonis

Copy link
Copy Markdown
Contributor

Summary

  • Add a Fastly-only EdgeZero deploy composite action at .github/actions/deploy.
  • Add edgezero-cli config push --adapter fastly --file <path> -- --store-id <id> and Fastly Config Store upsert support.
  • Document the GitHub Actions deploy contract, config-push behavior, and Trusted Server deployer migration plan.

Validation

  • .github/actions/deploy/tests/run.sh
  • bash -n .github/actions/deploy/scripts/*.sh .github/actions/deploy/tests/run.sh
  • go run github.com/rhysd/actionlint/cmd/actionlint@v1.7.7
  • zizmor --offline .github/actions/deploy/action.yml .github/workflows/deploy-action.yml
  • cd docs && npm run format && npm run lint && npm run build
  • cargo fmt --all -- --check
  • cargo test --workspace --all-targets
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo check --workspace --all-targets --features "fastly cloudflare"
  • git diff --check

Notes

Use the commit SHA from this PR for pre-release action pinning while testing. If this PR changes before merge, update downstream pins to the latest desired commit SHA; for production, prefer the final merged commit SHA.

@ChristianPavilonis ChristianPavilonis marked this pull request as ready for review July 3, 2026 14:31
@ChristianPavilonis ChristianPavilonis force-pushed the feature/edgezero-actions branch from 094a3b5 to 9debe90 Compare July 3, 2026 14:41

@aram356 aram356 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Summary

The Fastly deploy composite action is well-built and its credential-handling discipline is genuinely strong (every non-deploy step clears 15 FASTLY_* env aliases, the token is scoped to the Deploy step only, argv is NUL-delimited, the Fastly CLI is pinned by version + SHA-256, and paths are confined to github.workspace). CI passes locally (fmt, clippy, edgezero-core tests = 419, the action's own tests/run.sh, and shellcheck). The changes requested below are about scope, robustness, and hygiene — none are correctness bugs, but they affect reviewability and the durability of the action's security controls.

Findings

Requested changes

  • 🤔 Undocumented scope creep bundled incrates/edgezero-core/src/key_value_store.rs (~+200 lines) adds a KV operation-timing / observability feature that is unrelated to a "Fastly deploy action," is not mentioned anywhere in the PR body, and rides inside the single commit titled "Add Fastly deploy GitHub action." Please split it into its own PR/commit (or at least describe it in the body). The code itself is fine — web-time/log are already unconditional core deps, and the debug-timing is zero-cost when debug logging is off — but bundling an unannounced core-crate feature into an infra PR hurts reviewability and bisectability.

  • 🤔 deploy-args denylist is fragilevalidate-inputs.sh rejects a fixed set of long flags but not short flags (-s, -t, -v) or future Fastly flags. Since it's presented as a security control, an allowlist (fails closed) is sturdier than a denylist (fails open). See inline. (non-blocking; trusted caller input today)

  • python3 used before require_cmdvalidate-inputs.sh line 47. See inline. Also flags an opportunity to drop Python from the SHA-256 and path-confinement helpers in favor of coreutils (sha256sum, realpath).

  • Personal absolute path committeddocs/specs/trusted-server-deployer-migration-plan.md:6. See inline.

Informational

  • 📝 Stale PR description — the body advertises edgezero-cli config push --adapter fastly and "Fastly Config Store upsert support," but both already exist on main; this diff touches neither the CLI nor the fastly adapter. Please trim the body so reviewers aren't hunting for code that isn't in the diff.

  • 📝 Incidental Cargo.lock churn — bumps brotli, chrono, handlebars, http, and pulls indexmap in under serde_json (preserve_order became transitively enabled). Unrelated to the action; fine to keep, just noting it's bundled.

CI Status

  • fmt: PASS
  • clippy (edgezero-core, --all-features): PASS
  • tests (edgezero-core --all-targets): PASS (419)
  • deploy action contract tests (tests/run.sh): PASS
  • shellcheck: PASS (SC1091 info only)

Note: full-workspace clippy/test not re-run locally; only the crate touched by this diff (edgezero-core) plus the action's own gates were exercised.

}
dangerous_prefix = tuple(flag + '=' for flag in dangerous_exact if flag.startswith('--'))
for arg in args:
if arg in dangerous_exact or arg.startswith(dangerous_prefix):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 deploy-args guard is a denylist, and denylists leak. This blocks a fixed set of long flags plus their --flag= forms, but not short flags (-s, -t, -v, …) or any flag a future Fastly CLI version adds. Action-owned flags are prepended and Fastly's parser is generally last-wins, so a caller arg could still shadow --service-id or the token. deploy-args is trusted workflow config today, so this is defense-in-depth rather than an exploit — but it is presented as a security control.

Suggested fix: switch to an allowlist of known-safe flags (e.g. --comment), or if that's too restrictive, at minimum add the short-flag aliases and document in the guide that the denylist is best-effort. An allowlist fails closed; a denylist fails open.

esac
[[ "$FASTLY_API_TOKEN_PRESENT" == "true" ]] || fail "missing required input 'fastly-api-token'"
[[ -n "$FASTLY_SERVICE_ID" ]] || fail "missing required input 'fastly-service-id'"
python3 - "$FASTLY_SERVICE_ID" <<'PY'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

python3 invoked before it's guaranteed. This is the first python3 call in the script, with no require_cmd python3 guard — unlike install-fastly.sh, which guards. On ubuntu-24.04 python3 is always present so this is cosmetic, but on a minimal self-hosted runner the failure is a raw command not found instead of a clean action error.

Fix: add require_cmd python3 near the top, alongside the existing helpers.

(Related: several of these Python uses — the SHA-256 hashing and the realpath-based path helpers — could move to coreutils sha256sum/realpath and drop the interpreter entirely for those steps.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On this note .. is there a way not to use python?

**Status:** Draft migration plan

**Repository reviewed:** `/Users/christian/Projects/stackpop/trusted-server-deployer`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personal absolute path committed to the repo. Repository reviewed: /Users/christian/Projects/stackpop/trusted-server-deployer leaks a local username and is meaningless to anyone else reading the doc.

Fix: drop the line or genericize it (e.g. the trusted-server-deployer repository).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants