Repository navigation
Separate the native AWS runtime wrapper - #1538
ChristianPavilonis wants to merge 18 commits into
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Adds a separate native AWS runtime wrapper: two digest-pinned inputs (provider binary image + Debian 13 runtime), an entrypoint that publishes the bindings/Geo TOML literally as 0600 files via temp-file + rename (symlinks replaced, never followed), clears the input env and execs the app so SIGTERM reaches it directly. The shell logic is sound and fails closed; the Geo-off path is correct (native_geo.rs only reads the profile when TS_NATIVE_GEO_PROFILE_FILE is set, so a stale volume file is inert). No blocking findings.
3 of the inline comments below carry a one-click
suggestion(comment/docs only).
Non-blocking
🤔 thinking
unsetcomment describes inherited defaults that are not inherited — see inline atdeploy/runtime/native-aws/entrypoint.sh:47
⛏ nitpick
- README test command fails on macOS (
mv -Tis GNU-only) — see inline atdeploy/runtime/native-aws/README.md:37 - Missing spaces in README — see inline at
deploy/runtime/native-aws/README.md:11
Cross-cutting / body-level findings
- 🤔 Wrapper tests, ShellCheck and image build are not run by any CI job. No workflow references
deploy/runtimeortest-entrypoint.mjs, and the CI ShellCheck step covers onlyscripts/docs-proposal/*.sh. A later edit toentrypoint.shthat broke literal publication or symlink replacement would merge green.native-container.ymlalready has the path-filtered pattern; a similar job onubuntu-24.04runningshellcheck deploy/runtime/native-aws/entrypoint.shandnode --test deploy/runtime/native-aws/test-entrypoint.mjs(paths:deploy/runtime/native-aws/**) would keep these protections enforced. Fine as a follow-up given this unit's baseline-extraction scope.
Verification
shellcheckonentrypoint.sh: pass.node --test deploy/runtime/native-aws/test-entrypoint.mjson macOS with Node 24.12.0: 7 pass / 7 fail, allmv: illegal option -- T(expected on BSDmv; the Debian target is unaffected). Linux run not reproduced here; the 14/14 result is the author's.- Wrapper image build/run, ECS volume ownership and PID-1 SIGTERM handling were not exercised in this review.
CI Status
All 17 reported checks PASS (vitest, integration tests, integration tests (Fastly EC lifecycle), prepare integration artifacts, browser integration tests, format-typescript, format-docs, docs-proposal scripts, cargo fmt, cargo test, cargo test (axum native), cargo test (cross-adapter parity), cargo test (ts CLI, native) ubuntu-latest + macos-latest, spin native + wasm32-wasip2, cloudflare native + wasm32-unknown-unknown, CLAUDE.md symlink guard). None of them exercises the files in this PR.
| unset TS_NATIVE_GEO_PROFILE_FILE | ||
| fi | ||
|
|
||
| # The base image's local roots need not exist when both stores are remote. |
There was a problem hiding this comment.
🤔 thinking — The runtime stage starts FROM ${TS_NATIVE_RUNTIME_IMAGE}, so it does not inherit the provider image's EDGEZERO__ADAPTER__CONFIG_DIR=/config / DATA_DIR=/data ENV. In practice this unset only discards roots an operator sets on the ECS task. If bindings then select a local store, startup fails with CONFIG_DIR required (EdgeZero 83d7a62, dev_server.rs:1283/:1332) — fail-closed, but with no hint the wrapper removed it. Behaviour is fine; the comment should say what actually happens.
| # The base image's local roots need not exist when both stores are remote. | |
| # The runtime image does not inherit the provider image's local roots. Drop any | |
| # task-supplied roots too: this wrapper supports remote config/secret stores only. |
| D --> E[Exec native TS and forward signals] | ||
| ``` | ||
|
|
||
| Run the retained local publication and signal tests with the repository Node: |
There was a problem hiding this comment.
⛏ nitpick — On macOS (Node 24.12.0, matching .tool-versions) this command gives 7 pass / 7 fail, all mv: illegal option -- T. mv -T is right for the Debian image; the README just shouldn't imply the tests run on any host.
| Run the retained local publication and signal tests with the repository Node: | |
| Run the retained local publication and signal tests on Linux with the | |
| repository Node. The entrypoint relies on GNU `mv -T`, which macOS lacks: |
| the image whose inputs and runtime checks were actually reviewed; syntax alone | ||
| is not qualification. | ||
|
|
||
| The image runs as UID/GID10001. `/run/trusted-server` must be an owned, writable |
There was a problem hiding this comment.
⛏ nitpick — Missing space in UID/GID10001 (same on line 17, literal0600).
| The image runs as UID/GID10001. `/run/trusted-server` must be an owned, writable | |
| The image runs as UID/GID 10001. `/run/trusted-server` must be an owned, writable |
| provide required writable temporary storage when selecting that runtime mode. | ||
|
|
||
| `TS_NATIVE_BINDINGS_TOML` contains provider identifiers/references, never secret | ||
| values. The entrypoint publishes it as a literal0600 file under the private |
There was a problem hiding this comment.
⛏ nitpick — Missing space in literal0600.
| values. The entrypoint publishes it as a literal0600 file under the private | |
| values. The entrypoint publishes it as a literal 0600 file under the private |
Preserve the existing explicit image-input boundary, non-root private file publication, environment cleanup and exec signal ownership. No provider schema, resource identity, production configuration or deployment change is included.
There was a problem hiding this comment.
Superseded by a follow-up review with inline comments: #1538 (review)
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
This PR splits the native AWS runtime into its own wrapper image: a digest-pinned binary image plus a Debian runtime image, an entrypoint that publishes the provider files literally, and 14 node --test cases. The shell logic is careful: it never evaluates TOML, uses atomic mv -fT, sets 0600 modes, and execs the app directly so signals reach it. Two gaps remain. The new tests and shell script aren't run by any CI job, and the image build context and the apt layer don't match the reproducibility and isolation the comments and labels claim.
2 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch) to apply them. The other comments describe the fix in prose because it spans files or is a design choice.
Blocking
🔧 wrench
Dockerfile.dockerignorere-includes all ofdeploy/: see inline atdeploy/runtime/native-aws/Dockerfile.dockerignore:4-6apt-get upgrademakes the image non-reproducible: see inline atdeploy/runtime/native-aws/Dockerfile:15-16- The new tests are not in CI: see the cross-cutting section below
Non-blocking
🤔 thinking
- Fixed publish path vs
EDGEZERO__STORE_BINDINGS_FILE: see inline atdeploy/runtime/native-aws/entrypoint.sh:23
⛏ nitpick
- Missing spaces in the README: see inline at
deploy/runtime/native-aws/README.md:11and:17
Cross-cutting / body-level findings
- 🔧
test-entrypoint.mjsandentrypoint.share not exercised by CI. No workflow under.github/workflows/referencesdeploy/runtime/native-aws/, so the 14 publication, permission and signal cases (which pass locally with Node 24.20.0) and the ShellCheck run mentioned in the test plan are local-only, and a regression would merge unnoticed. Add a job (or a step in an existing scripts job) that runsnode --test deploy/runtime/native-aws/test-entrypoint.mjswith the.tool-versionsNode andshellcheck deploy/runtime/native-aws/entrypoint.sh, path-filtered todeploy/runtime/native-aws/**likepbs-example.ymldoes. - ⛏ PR description spacing. Several numbers are glued to the preceding word (for example "All14", "UID/GID10001", "Playwright1.58.2", "Ubuntu26.04"). Worth cleaning up before this becomes ready for review.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- browser integration tests: PASS
- trace runtime and browser acceptance: PASS
- vitest: PASS
- cargo test: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- format-typescript: PASS
- release scripts: PASS
- docs-proposal scripts: PASS
- Fastly response diff script: PASS
- ts dev lint domains (changed lines): PASS
- cargo test (cross-adapter parity): PASS
- format-docs: PASS
- CLAUDE.md symlink guard: PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CI Node pin guard: PASS
- HTML EOF runtime (Fastly): PASS
- cargo fmt: PASS
- worker-deadlines (cloudflare): PASS
- cargo test (axum native): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- worker-deadlines (spin): PASS
- prepare integration artifacts: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
| !deploy/ | ||
| !deploy/runtime/ | ||
| !deploy/runtime/native-aws/ |
There was a problem hiding this comment.
🔧 wrench — These re-includes widen the context to all of deploy/, not just entrypoint.sh
Docker cleans !deploy/ to deploy and treats a match on any parent directory as a match for the file. So !deploy/ re-includes everything under deploy/ (for example a local deploy/pbs-example/terraform.tfvars), and !deploy/runtime/native-aws/ re-includes every file next to the entrypoint. That defeats the header comment ("Keep operator input/state/config files out of the deployment-image context"). An explicit file re-include is enough, because the builder walks excluded directories when a later ! rule could match inside them:
**
!deploy/runtime/native-aws/entrypoint.shApply manually. This can't be a one-click suggestion because test-entrypoint.mjs (the "Dockerfile-specific context permits only the bootstrap script" test) pins the exact rule list and has to change too. Also note that only BuildKit reads Dockerfile.dockerignore. The legacy builder uses the root .dockerignore, which excludes deploy/, so the COPY on Dockerfile line 26 fails there. It's worth saying in the README that BuildKit is required.
| apt-get update; \ | ||
| apt-get upgrade -y --no-install-recommends; \ |
There was a problem hiding this comment.
🔧 wrench — apt-get update && apt-get upgrade makes the image non-reproducible
Both inputs are digest-pinned, and the labels on lines 21-22 record those digests as though they describe the image. But update/upgrade/install pull whatever the Debian mirror serves at build time, so two builds from the same labelled inputs can produce different runtime layers, and the labels no longer describe what was reviewed. Pick one:
- Drop
apt-get upgradeand pin package versions (or rely on a refreshed runtime digest for security updates). - Pin the apt sources to a
snapshot.debian.orgtimestamp and record that timestamp as a label. - At minimum, document in the README that the labels don't fully describe the runtime layer.
Apply manually. The right option is a design choice, not a mechanical edit.
| # printf treats the entire input as data. Never source or evaluate these bytes. | ||
| printf '%s' "$TS_NATIVE_BINDINGS_TOML" > "$temporary" | ||
| chmod 0600 "$temporary" | ||
| mv -fT -- "$temporary" "$runtime_dir/native-bindings.toml" |
There was a problem hiding this comment.
🤔 thinking — The publish path is fixed, but the app reads EDGEZERO__STORE_BINDINGS_FILE
The entrypoint always writes $runtime_dir/native-bindings.toml, while the application loads whatever EDGEZERO__STORE_BINDINGS_FILE points to. The Dockerfile sets the two to the same value, but a task definition that overrides the env var would make the app read a different (possibly stale or missing) file, and nothing would report it. Consider failing fast on a mismatch, for example:
if [ "${EDGEZERO__STORE_BINDINGS_FILE:-}" != "$runtime_dir/native-bindings.toml" ]; then
printf '%s\n' 'EDGEZERO__STORE_BINDINGS_FILE must point at the published bindings file' >&2
exit 1
fiThe test fixture sets EDGEZERO__STORE_BINDINGS_FILE to the relocated path, so the check would hold under the existing tests once the fixture's runtime_dir replacement is taken into account.
| the image whose inputs and runtime checks were actually reviewed; syntax alone | ||
| is not qualification. | ||
|
|
||
| The image runs as UID/GID10001. `/run/trusted-server` must be an owned, writable |
There was a problem hiding this comment.
⛏ nitpick — Missing space before the UID/GID value.
| The image runs as UID/GID10001. `/run/trusted-server` must be an owned, writable | |
| The image runs as UID/GID 10001. `/run/trusted-server` must be an owned, writable |
| provide required writable temporary storage when selecting that runtime mode. | ||
|
|
||
| `TS_NATIVE_BINDINGS_TOML` contains provider identifiers/references, never secret | ||
| values. The entrypoint publishes it as a literal0600 file under the private |
There was a problem hiding this comment.
⛏ nitpick — Missing space before the file mode.
| values. The entrypoint publishes it as a literal0600 file under the private | |
| values. The entrypoint publishes it as a literal 0600 file under the private |
Summary
Draft baseline review unit. Not a current-main merge candidate, deployment approval or completed current-head CI claim.
Baseline and sequence
Head:
46f621112d5b15ffa48ae8e99c3c0000718032b2. Base:baseline/native-image-20261110.Historical main anchor:
2dd968aeebeb2cc0d5110115f64e0ad58e2e1d13. Original demo: TS2423a2ed plus separately preserved dirty inputs. Matching EdgeZero: draft431, exact revision83d7a6296d285e135c4ea70021b19e6c8dca7e9f.The user selected compatible-baseline cleanup. Current-main integration with EdgeZero462 and the authenticated-forwarder incompatibility are deferred. Preserve current-main PR1525/1526 ready and1527/1528 draft, with their existing bases. No excluded1499 or independent1506 implementation, relay/privacy experiment, new transport fallback, group/codec/phase design or operational deployment is included.
Changes
Closes
Refs #1530. This unit completes one part of the baseline extraction and does not close the overall tracker on its own.
Spec
Not a new feature. This is extraction of existing demo behavior and bounded build/caller corrections. Original native and EC storage design/review records remain in the baseline where applicable; they are not presented as approval of later combined source. No new forwarding, consent, timer or provider policy is designed here.
Test plan
All14 publication/permission/signal cases and ShellCheck pass. The exact binary/runtime wrapper image combination and ECS volume ownership remain unqualified. No credential values or operator config are added.
The b67ffc6 baseline checkpoint has passing documented local gates: Fastly/default/reuse/native/Cloudflare/Spin suites, all adapter strict lint lanes, 17 cross-adapter parity cases, 1252 JS cases, CLI suites, formatting, clock/digest/proposal checks and changed-lines domain lint. Passing checks at121ebcc36 are reused only for unchanged relevant inputs; the final follow-up covers the CLI test-import and documentation-only corrections. These results are not a substitute for this PR's current-head hosted checks.
Checklist
Operational Terraform remains in trusted-server-deployer. Local checks and prototype tests do not qualify cloud access, fleet durability or a live publisher.
Hosted docs-build correction
The first hosted format-docs job caught an additional site-build gate that the original local formatter checkpoint had not run: a relative link from the native EC guide to a Terraform example outside the site. Native EC correction8b11dbad3 replaces it with the plain repository path; no dead-link check is disabled. That docs-only correction is carried through the existing upper PR identities. Native EC and final integration8985b111 both pass docs ESLint and VitePress build; the final docs formatter also passes. Runtime, dependency, image-build-context and test inputs are unchanged from b67ffc6, so their prior exact-input proofs are reused. Hosted current-head checks still control readiness.
Startup negative-probe correction
A hosted native EC case failed the no-listener probe while its child logged the expected startup validation failure. The old log has no socket endpoints, so the failure's exact subtype is not claimed confirmed. A separate controlled Linux reproduction demonstrates a successful TCP self-connection with no listening server. Native correction8490c691d distinguishes equal local/peer endpoints from a real listener; actual socket controls prove the self-connection is excluded and a real listener is still detected. Nonzero exit, expected diagnostic, secret-redaction and the original startup deadline remain required. This changes only the acceptance fixture, not production admission, transport or trust.
The earliest native unit and final9356c557 default/all-feature acceptance checks and strict lint pass, including both controls and the original failure case. Ignores remain explicit; earlier separately executed celld proof is not relabeled as a new ignored-case run. Correction is merged upward through existing PR identities without force-pushing or retargeting. Current-head hosted checks must pass; the old failure is retained rather than rerun-only waived. Runtime/API/dependency inputs remain unchanged. A fresh exact9356 producer build and real runtime smoke now pass: image
sha256:65a735d38e9dbc0573d0f6068a8b261bd202c43aea05085c2b2d6bccc91ebdbd, linux/amd64 UID/GID10001, including startup rejection and in-flight SIGTERM drain. Prior b67 image receipts remain separate; no image was pushed. All local parent runners are complete.Browser CI host compatibility
Preserved older browser failures show Playwright1.58.2 rejecting ubuntu-latest's new Ubuntu26.04 image before browser installation; later missing-executable failures are consequences, not passing browser tests. Nativeb1216763a pins only the browser job to ubuntu-24.04, leaving dependency versions, tests and assertions unchanged. Direct Actionlint1.7.12 passes at the owning native and final integration points; the first shim-only attempt could not run because no global version was configured, and is not counted as lint proof. No global tooling was changed. The only9356 ->ee7 delta is this workflow host/comment; runtime, Cargo, docs, tests and Docker-context inputs are unchanged. Actual browser execution on the supported hosted runner remains pending and must pass before readiness. No waiver or browser skip.