Conversation
supportedPlatform() previously accepted linux/amd64 only, leaving the already-implemented darwin preflight (macOS 15+, Hypervisor.framework) dead. Widen it, and reuse the same predicate in the microvm_dev development-release gate instead of duplicating the linux-amd64 check. Also fix the dev-release prep script's host-platform case (was Linux-x86_64 only) and two portability bugs it exposed: GNU-only `find -printf`, and a hardcoded "linux-amd64" platform literal in the release descriptor writer. Local-only enablement for testing on Apple Silicon; stacked on #580. Co-Authored-By: mecatl <noreply@stacklok.com>
…erreach Two real bugs surfaced by actually running the darwin/arm64-enabled preflight path on real macOS hardware, both pre-existing (reproducible independent of the platform-gate commit): - lockManager called syscall.Flock directly; unlike os/net file descriptors it isn't wrapped with automatic EINTR retry, so a blocking LOCK_EX could spuriously fail if the thread received a signal (notably Go's SIGURG async-preemption) while parked in the kernel wait. Manifested as an EnsureReady hang/failure under concurrent load on darwin/arm64. Added flockRetryEINTR. - refuseSymlinkAncestors walked every ancestor up to the filesystem root refusing any symlink, including OS-level layout the manager never creates paths within. On macOS /var is itself a symlink to /private/var, and TMPDIR-derived paths (t.TempDir()) land under /var/folders/..., so the walk tripped on ordinary system layout. Now stops at the first pre-existing ancestor, which is exactly the boundary MkdirAll actually creates past — preserving the real symlink-swap protection for paths this manager creates while no longer flagging pre-existing host layout. Verified: go test -race, -tags=microvm_dev, and 400 concurrent stress iterations of the previously-hanging test, all clean. Stacked on #580. Co-Authored-By: mecatl <noreply@stacklok.com>
Three more bugs surfaced by actually running task microvm:dev:prepare
end to end on macOS/arm64:
- package-microvm-release.sh's write_artifact used GNU-tar-only
--sort/--mtime/--owner/--group/--numeric-owner. bsdtar (macOS)
rejects all five and exits nonzero, but the tar|gzip pipe has no
pipefail, so the archive silently came out empty while the pipeline
reported success. Added a portable reproducible_tar helper (sorted
-T file list + portable touch -t mtime stamping); byte parity with
GNU tar's own output isn't required since the signed provenance
binds artifact_tree_digest, computed independently over the raw
tree, not the archive bytes. Applied the same fix to the identical
copy-pasted pattern in microvm-ci-release_test.sh.
- Taskfile.yml's microvm:dev:prepare verification step hardcoded a
linux-amd64 descriptor path regardless of host platform; the
underlying script (already darwin-aware since a7ab5df77) writes to
a platform-specific dir. Mirrors the script's own uname-based
platform resolution inline.
- Fixing the above let TestPreparedDevelopmentReleaseBundleIsImportable
actually run on darwin-arm64 for the first time, surfacing two more
linux-amd64-hardcoded assumptions inside the test itself (expected
manifest filename, DefaultOperations{GOOS,GOARCH} fixture) — made
both platform-derived from the descriptor instead.
- macOS bsdtar also embeds AppleDouble "._*" sidecar entries for any
file carrying an xattr (macOS auto-stamps com.apple.provenance on
most files) — invisible to tar's own listing but visible to Go's
archive/tar, breaking the bundle-membership check. Fixed with
COPYFILE_DISABLE=1 (no-op on Linux).
Verified with a full green `task microvm:dev:prepare` run on real
macOS/Apple Silicon hardware. Stacked on #580.
Co-Authored-By: mecatl <noreply@stacklok.com>
The prior fix ("stop at the first ancestor that already exists") wasn't
a real boundary, just a heuristic that happened to dodge the /var case.
It broke again live: creating a session against a real running daemon
failed with "refusing symlink path /tmp", because the manager's
RuntimeDir fallback is created directly inside /tmp (also a symlink to
/private/tmp on macOS), making /tmp itself "the first existing
ancestor" and tripping the same false refusal.
The check had no notion of whose subtree it was protecting. Threaded
an explicit root boundary through Paths (StateRoot/RuntimeRoot/
DataRoot/ConfigRoot: the ambient XDG bases, or the /tmp fallback) and
every call site of secureMkdirAll/atomicWrite/atomicWriteMode/
Operations.Download/Operations.Install. refuseSymlinkAncestors now
climbs from the target up to, but never including, that root — root
and anything above it is ambient host OS layout the manager never
created and has no authority over, however the OS lays it out (macOS
/var and /tmp being symlinks included) — while a symlink planted
strictly between root and target is still refused, preserving the
real TOCTOU protection this function exists for.
Verified end to end on real macOS/Apple Silicon hardware: the
readiness pipeline now runs cleanly through prepare/preflight/
download/verify/install/daemon/socket/reconcile (all previously
blocked by this bug) and reaches a genuinely different, expected next
gap at the health stage (dev-build code-signing/entitlements, not a
symlink refusal).
Independently reviewed for architectural soundness (root-exclusive/
path-inclusive climb, complete threading across ~12 call sites, all
Operations implementers and Paths constructors updated, fail-closed
empty-root/out-of-root handling) before landing.
Co-Authored-By: mecatl <noreply@stacklok.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enables the microVM execution-environment feature (#580) to run its host-side
readiness/preflight path on macOS/Apple Silicon, in addition to the currently
supported Linux amd64/arm64. Stacked on #580's own branch.
A striking amount of macOS support already existed in the code (darwin
preflight logic, process-identity handling, darwin release artifacts) but was
gated behind a single hardcoded
linux/amd64-only check, leaving it dead andnever exercised. Flipping that gate and actually running the feature end to
end on real Apple Silicon hardware surfaced four real, previously-undetected
bugs — none of which were findable by code review alone, since the darwin
code paths had never executed. Each commit fixes one bug found this way:
feat(microvm): allow darwin/arm64 and linux/arm64 hosts— the platformgate itself (
supportedPlatform()and a separatemicrovm_devdev-releasegate), plus a macOS-portable fix to the dev-release packaging script's
tar/find invocation.
fix(microvm): darwin manager-lock EINTR retry and symlink-ancestor overreach— a rawsyscall.Flockwith no EINTR retry, causing anintermittent
EnsureReadylock-convergence hang under concurrent load(Go's own SIGURG async-preemption signal can interrupt a blocking
LOCK_EX); plus a first attempt at fixing an overly-broad symlink-ancestorsafety check.
fix(microvm): macOS-portable dev-release packaging— GNU-tar-only flags(
--sort,--mtime,--owner,--group,--numeric-owner) silentlyproduced empty archives on macOS's bsdtar (no
pipefail, so the failurewas swallowed); plus a macOS AppleDouble/xattr tar quirk and two more
platform-hardcoded test assumptions this unblocked for the first time.
fix(microvm): bound refuseSymlinkAncestors to the manager's own root—the real fix for the symlink-ancestor check: macOS's
/varand/tmparethemselves symlinks to
/private/var//private/tmp(normal OS layout,not an attack), and the check had no notion of its own security boundary.
Threaded an explicit root (the manager's own state/data/runtime/config
directory boundary) through the check and every call site, so ambient host
layout above that root is never inspected, while a symlink planted inside
the manager's own subtree is still refused. Independently reviewed for
architectural soundness before landing.
Verified on real macOS/Apple Silicon hardware, not just unit tests: after
these fixes,
mecated microvm doctorreportshost preflight: passed(thedarwin preflight code that had been dead until now), and the full readiness
pipeline (
prepare → preflight → download → verify → install → daemon → socket → reconcile → health → ready) completes successfully end to end.POST /v1/sessionsagainst a realmecated serve --default-placement microvm-localreturns a real session with"placement":{"kind":"microvm",...}.Development stage
Contract linkage
already-designed, already-partially-implemented darwin support (the darwin
preflight/process-identity code predates this PR and was already reviewed
as part of feat: add microVM execution environments #580); no new architecture, no public/guarded API change, no
persistence or trust-boundary change. Discovered and fixed via hands-on
execution of the feature on real hardware, directed interactively by a repo
maintainer (this account) rather than a pre-planned acceptance plan.
anticipated by
docs/adr/0342-microvm-execution-environments.md(currentlystates macOS is "not yet available"; follow-up needed once this merges).
definition, not a waiver of a required spine.
Interface conformance
No public/guarded engine API changed. The only interface touched
(
microvmmanager.Operations.Download/Install, gaining an explicitrootparameter) is an internal adapter interface in
internal/adapter/microvmmanager,not the
engine/apiguarded surface —task api:checkis unaffected.Issue relationship
Relates to #580
Type of change
Test plan
Baseline checks
task lint) — 0 issues in every file this PR touches. 4pre-existing issues remain in
environment/microvm(gosec G115 inprocess_identity_darwin.go, 3 staticcheck SA4023 incmd/mecatl-guest-agent/main.go) — confirmed viagit diff origin/acc/microvm-execution-environmentsto predate this PR entirely(last touched by feat: add microVM execution environments #580's own original commit); out of scope here.
task test) — 2 pre-existing failures unrelated tothis PR, both in
internal/app(last touched by unrelated commitsd14494785/147aa0d5f, confirmed outside this PR's diff):TestCanonicalConfiguredDir_AllowsMissingDefaultParents(a/varvs
/private/varpath-canonicalization mismatch — the same bug classthis PR fixes in
microvmmanager, but in a different, untouchedpackage) and
TestCommandRunnerEnvironment_Scenario2_DefaultScrub.Every test in
internal/adapter/microvmmanager(this PR's actualscope) passes, including with
-raceand both with/without-tags=microvm_dev.go run ./cmd/mecademo) — unaffected by this change;not re-verified here since this PR only touches microvm-specific code.
docs/adr/0342anduser-docs/building/deployment/microvm-environments.mdstill saymacOS is unsupported; intentionally left as a follow-up until this is
merged and the release pipeline is revisited (out of scope for this
local-dev-focused PR).
/panel-reviewnot run in this session;the symlink-boundary fix (the highest-risk change) was independently
reviewed by a separate architecture-focused pass before landing.
Changes
internal/adapter/microvmmanager/default_operations.gointernal/adapter/microvmmanager/manager.gorefuseSymlinkAncestorsinternal/adapter/microvmmanager/development_release_microvm_dev.gointernal/adapter/microvmmanager/*_test.goTaskfile.ymlmicrovm:dev:prepareverification path.github/scripts/prepare-microvm-development-release.shfind/tarUser-facing change
Developers on macOS/Apple Silicon can now use
microvm-localexecutionenvironments (previously hard-refused with "microvm-local supports Linux
amd64 with KVM only"). This PR covers the local development workflow
(
task microvm:dev:*); the release/CI pipeline for shipping signeddarwin-arm64 binaries to end users is a separate, larger follow-up.
Special notes for reviewers
refuseSymlinkAncestors) is the highest-riskchange here — it's a security-relevant path-safety check that's now been
fixed twice before landing correctly the third time (each prior attempt
was a heuristic, not a real boundary). It was independently reviewed for
architectural soundness (root-exclusive/path-inclusive climb, complete
threading across every call site, fail-closed empty-root/out-of-root
handling, test coverage of both sides of the boundary condition) before
this PR was opened.
for a maintainer decision): (1)
POST /v1/sessionsblocks synchronously forthe full first-boot readiness duration (~3+ minutes cold-cache on this
hardware) with no progress visible over the plain HTTP API, so any client
with a shorter default timeout will see an apparent hang/failure even
though the server-side operation would succeed; (2) a canceled/interrupted
first attempt (e.g. from exactly that client timeout) can leave wedged
daemon/socket state that
doctor/statusdon't self-heal, requiringmanual cleanup — a live reproduction of a risk feat: add microVM execution environments #580's own prior review
already flagged in the abstract.
builds/publishes darwin-arm64 artifacts (per feat: add microVM execution environments #580), but the code-signing/
entitlements story for a released binary was not re-verified here beyond
confirming that upstream
go-microvmalready ships a pre-signeddarwin-arm64 runtime — worth a maintainer sanity check before relying on it
for a real release.