Repository navigation
Separate the source-built native image - #1539
ChristianPavilonis wants to merge 19 commits into
Conversation
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
No blocking issues. The build-context allowlist, the digest-pinned multi-stage build and the smoke test are careful work. The test covers startup rejection of an invalid config envelope and of a missing secret, non-root execution, a read-only root filesystem with explicit writable mounts, and SIGTERM draining with a request in flight. All checks pass at b9e1ff2c, including local-provider-image.
Non-blocking
- ♻️ Builds download unused wasm targets because of
rust-toolchain.toml(inline suggestion onDockerfile). - 📝 The new docs page isn't linked from the sidebar.
- 🤔 The
TRUSTED_SERVER__EC__PASSPHRASEpoison value isn't asserted. - 🤔
apt-get upgrademakes the runtime layer unrepeatable despite the digest pins.
Verification notes
I relied on the head-matched CI run and did not build the image locally. Non-default NATIVE_FEATURES sets are compiled only by the clippy-axum --all-features CI step, not built or smoke-tested as images.
| # Optional existing build metadata input; .git never enters the build context. | ||
| ARG TRUSTED_SERVER__GIT_VERSION="" | ||
| RUN <<'SH' | ||
| set -eu |
There was a problem hiding this comment.
♻️ refactor — Each image build downloads three unused wasm standard libraries.
rust-toolchain.toml is in the build context and lists wasm32-wasip1, wasm32-wasip2 and wasm32-unknown-unknown. The first rustc --version below makes rustup install all three before this native-only build starts. The local-provider-image run at this head shows downloading 3 components and about 98 spurious network error retry warnings. It passed only because the retries succeeded. That adds roughly 20s and a network step that can fail the image build for no product reason.
Setting RUSTUP_TOOLCHAIN before the first rustc call selects the installed 1.98.1 toolchain without applying the file's target list. The rust-toolchain.toml channel assertion below still keeps the two in sync.
| set -eu | |
| set -eu | |
| # Native-only build: skip the toolchain file's wasm targets. | |
| export RUSTUP_TOOLCHAIN=1.98.1 |
| @@ -0,0 +1,46 @@ | |||
| # Native container | |||
There was a problem hiding this comment.
📝 note — Nothing links to this page. It isn't in the docs/.vitepress/config.mts sidebar, and no other doc links to it, so readers can only reach it by URL or search. Consider adding a sidebar entry next to the other guide pages (outside this diff, so no suggestion).
| '--env', | ||
| 'EDGEZERO__ADAPTER__SHUTDOWN_GRACE_SECONDS=3', | ||
| '--env', | ||
| 'TRUSTED_SERVER__EC__PASSPHRASE=poison', |
There was a problem hiding this comment.
🤔 thinking — This poison value is never checked. The origin URL poison on the next line is covered indirectly, because the page request would fail if it were used. Nothing distinguishes whether the EC passphrase came from poison or from the envelope's secret reference, so a change that started honoring this variable would still pass. Either add an assertion that tells the two passphrases apart, or drop the line so it doesn't read as coverage.
|
|
||
| FROM debian:bookworm-slim@sha256:7c7b2c966bc9ee8cedfeef67e0e279108992c77681fa595db4a9d65c06ccc587 AS runtime | ||
| RUN apt-get update \ | ||
| && apt-get upgrade -y --no-install-recommends \ |
There was a problem hiding this comment.
🤔 thinking — The base images are digest-pinned and the image records a hash of its build inputs. But apt-get upgrade pulls whatever Debian packages are current, so two builds of the same commit can produce different runtime images. Getting security patches this way is a reasonable trade-off. A short comment saying it's deliberate would keep readers from assuming the runtime layer is reproducible.
Keep the existing PR's isolated source build, non-root runtime, minimal build context and local-provider lifecycle smoke. Resolve and pin Node 24.20.0 to its registry index digest, check it against .tool-versions and retain Rust 1.98.1 with two build jobs. The local Linux image passes actual startup rejection, listener, page/TSJS, runtime permissions and in-flight SIGTERM checks. No image publication, cloud access or production-feature image qualification is implied.
There was a problem hiding this comment.
Superseded by a follow-up review with inline comments: #1539 (review)
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Adds a hardened multi-stage Dockerfile for the Axum adapter, a tight .dockerignore allowlist, a workflow, and an account-free smoke test that checks startup rejection, HTML rewriting, embedded TSJS, non-root/read-only runtime and SIGTERM draining. The CI failures from the previous pass are now green, and the Dockerfile now asserts the Node and Rust pins against .tool-versions. One non-blocking item remains.
1 of the inline comments below carries a one-click GitHub
suggestion. Use Commit suggestion to apply it.
Non-blocking
🤔 thinking
- Image version label uses the merge commit on PRs: see inline at
.github/workflows/native-container.yml:43
Cross-cutting / body-level findings
- 👍 Thorough smoke test:
scripts/test-native-container.mjsregisters owned containers and the network before creating them, so cleanup survives a timed-out Docker client. It also proves an in-flight request finishes after draining starts.
CI Status
- All reported checks: PASS (including
local-provider-image,cargo test (axum native)andprepare integration artifacts)
| with: | ||
| node-version: ${{ steps.node.outputs.version }} | ||
| - name: Build local-provider image | ||
| run: docker build --build-arg TRUSTED_SERVER__GIT_VERSION="${{ github.sha }}" -t ts-rc-local:test . |
There was a problem hiding this comment.
🤔 thinking: On pull_request events github.sha is the synthetic merge commit, not the PR head, so the image's baked x-ts-version names a commit that exists on neither branch. The guide builds with $(git rev-parse HEAD), so CI should label the image with the head commit too. workflow_dispatch still falls back to github.sha.
| run: docker build --build-arg TRUSTED_SERVER__GIT_VERSION="${{ github.sha }}" -t ts-rc-local:test . | |
| run: docker build --build-arg TRUSTED_SERVER__GIT_VERSION="${{ github.event.pull_request.head.sha || github.sha }}" -t ts-rc-local:test . |
Per the user, the RC line is forward-ported to main (DEC-6). Rechecking the proposed stack against DEC-3 and DEC-16: #1536 and #1537 are mostly Axum-only stores, store selection and wiring that EZI-2b, T6 and T21 replace, so A2 becomes a new pull request that takes their tests, and both close with #1535 once their code moves. The image and Terraform units restack on #1543, which publishes the image and shares .dockerignore with #1539. #1531 goes on #1506 for A1 and #1532 on #1531 for #1529.
Summary
Draft baseline review unit. Not a current-main merge candidate, deployment approval or completed current-head CI claim.
Baseline and sequence
Head:
b9e1ff2cf4ecbc752f9843379d034268d7965689. Base:baseline/ec-storage-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
A fresh exact-final b67ffc6 linux/amd64 producer build and real runtime smoke pass. Image50e07713a542a03689b76833b2ac78daeea789e494156e6c4db72d3624a7069e runs as UID/GID10001. Producer files match this unit; final integration also carries the separately reviewed wrapper/examples. Earlier f45 image proof remains retained separately. Default/local providers only; no image publication, AWS access or separate wrapper acceptance is claimed.
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.