feat(sandbox): network-identity-invariant snapshots - #562
Conversation
|
This run croaked 😵 The workflow encountered an error before any progress could be reported. Please check the link below for details. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryThe PR makes sandbox snapshots independent of external pool addresses by giving every guest a fixed link-local identity and translating each sandbox’s pool identity on the host side.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the scope of this follow-up review. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| virt/arcbox-vm/src/network/invariant.rs | Implements the per-TAP NAT, marking, policy-routing, sysctl, and teardown machinery for fixed guest identities. |
| guest/arcbox-agent/src/agent/linux/port_forward.rs | Extends exposed-port mappings with paired MARK and DNAT rules for invariant-addressed sandboxes. |
| virt/arcbox-vm/src/snapshot.rs | Adds backward-compatible snapshot metadata identifying network-invariant guests. |
| virt/arcbox-vm/src/sandbox/checkpoint.rs | Records the guest’s effective network identity when producing checkpoints. |
| virt/arcbox-vm/src/sandbox/lifecycle.rs | Skips legacy guest-side network reconfiguration when restoring invariant snapshots. |
| guest/arcbox-agent/src/init.rs | Updates guest boot forwarding and isolation rules for the fixed link-local identity. |
| tests/e2e/src/sandbox.rs | Updates sandbox network assertions to require the fixed guest address and exclude pool addresses. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Client[Client or host relay] --> PoolIP[Sandbox pool IP]
PoolIP --> Mark[Mark packet for sandbox TAP]
Mark --> DNAT[DNAT to fixed guest IP]
DNAT --> Route[Per-sandbox fwmark route]
Route --> TAP[Sandbox TAP]
TAP --> Guest[Guest 169.254.100.2]
Guest --> SNAT[Mark-scoped SNAT]
SNAT --> Client
Reviews (2): Last reviewed commit: "test(e2e): assert the invariant guest id..." | Re-trigger Greptile
No repo CI failure to fix on this PRDiagnosis: the failing check suite is not a repository CI job, so nothing in the branch needed fixing.
Verified the branch is green anywayRan the exact commands CI will execute once this stack reaches
Working tree is clean; nothing was committed or pushed. Unrelated pre-existing issues found (left alone)
One note on Greptile's flag:
|
a765a8c to
78e2447
Compare
Snapshots need a compatibility marker for the network-identity-invariant scheme (CORE-81): a checkpoint taken from a guest running the fixed link-local identity needs zero guest-side network work on restore, while legacy snapshots must keep the reconfig-RPC path. Record net_invariant in SnapshotMeta (serde default: false, so legacy metas load unchanged), carry it on the instance so chained checkpoints record the guest's actual addressing, and skip the restore-time reconfig when the flag is set. Nothing sets the instance flag to true yet; behavior is unchanged until the invariant boot path lands.
Every sandbox guest now boots ip=169.254.100.2 with gateway 169.254.100.1 (CORE-81), making snapshots network-agnostic: an invariant-snapshot restore needs no reconfig RPC and no resolv.conf rewrite, and a pooled slot (CORE-78) can attach any TAP without waking the guest. The pool IP becomes a host-side property of the TAP, applied as a per-TAP 1:1 translation inside the System VM with what the guest image actually ships (static iptables-legacy + policy routing; the kernel has no NETMAP, no tc act_nat, and no conntrack marks): to-sandbox traffic is marked from its pre-NAT destination and DNAT'd to the fixed guest IP, a per-sandbox fwmark fib rule + table routes the rewritten destination out the right TAP, and sandbox-originated traffic is SNAT'd back to the pool IP in INPUT (by input interface) and POSTROUTING (by fixed-guest source + mark). The fib rule and table route are programmed over rtnetlink directly, so nothing depends on busybox ip's fwmark/table support. Legacy snapshots keep the pre-invariant TAP shape and the reconfig RPC: restore selects the TAP mode from the snapshot's net_invariant marker. Translation teardown is tolerant of absence and runs in both the release and quarantine paths, so legacy TAPs and crash replays are no-ops.
…tity Expose DNAT must target what the guest actually owns: invariant sandboxes sit on the fixed link-local address, so their PREROUTING DNAT now targets it directly, paired with a mangle MARK rule on the same match so the rewritten destination routes out the sandbox's TAP via its fwmark table. Legacy sandboxes keep the pool-IP target. sandbox_network_identity grows into SandboxNetworkIdentity carrying the addressing mode alongside the external pool IP and cleanup token; DNS registration and the API keep reporting the pool IP unchanged. The orphan sweep now claims tokenized rules by their unique generation comment alone (their target differs by mode and MARK rules have none) and sweeps the mangle companions too. System VM boot adds FORWARD accepts for the fixed guest address (the filter sees post-DNAT/pre-SNAT addresses) and makes the deliberate sandbox-to-sandbox isolation explicit: pool-bound traffic from any sandbox TAP is dropped before marking, except toward the pool gateway legacy guests use for DNS.
The smoke's fresh-network clone check asserted the guest re-addressed eth0 to its pool IP — the pre-CORE-81 contract. Under invariant addressing the guest keeps the fixed link-local identity and pool IPs exist only host-side; assert exactly that, and keep the route-derived gateway ping as the datapath proof.
172e357 to
0360e72
Compare
There was a problem hiding this comment.
Important
The new invariant::remove teardown step is ?-propagated ahead of TAP destruction, so a transient iptables/netlink failure now strands the TAP device and permanently leaks the pool address. Details inline.
Reviewed changes — no prior Pullfrog review existed on this PR, so this covers all 4 commits. The 172e357 → 0360e72 range-diff is rebase-only (11 added lines, all context shifts around chroot_owner/pool_slot_id now that #561 has landed), so scope is the full diff.
- Verified the 7-rule per-TAP NAT contract in
virt/arcbox-vm/src/network/invariant.rsagainst the netfilter hook order it depends on.mangleruns beforenatin the same hook, so marking to-sandbox packets from their pre-NAT destination is sound, and themangle OUTPUT+nat OUTPUTpair correctly re-routes locally-originated replies after the destination rewrite forces a route re-lookup. Gating the POSTROUTING SNAT on-s 169.254.100.2/32rather than the mark alone is the right call — the mark is shared with to-sandbox packets that carry a real client source — and both properties are pinned by exact-match tests. - Traced the DNS chain end to end, since the design leans on "the nameserver is the fixed gateway everywhere". It holds: the sandbox's
vm-agentwritesnameserver {ip_param.gateway}from theip=cmdline,boot.rsnow bakesGUEST_GATEWAYthere, and the System VM's resolver binds0.0.0.0:53. Legacy restores still resolve because the pool gateway is the one address exempted from the newmangleisolation DROP. - Confirmed crash recovery needs no persisted
net_invariant.invariant::removederives every rule spec from(tap_name, pool_ip)alone, sosweep_orphansreplays teardown correctly for an invariant TAP it holds no record of. That state-free property is load-bearing and worth preserving. - Checked the
fwmark == table id == u32::from(pool_ip)choice. Unique per active sandbox, and the documented kube-proxy caveat is accurate: the 0x4000/0x8000 masks land in the third octet, so the first ~16k addresses of a /16 pool are conflict-free. - Reviewed the hand-rolled rtnetlink encoder in
network/rtnetlink.rs. Attribute padding, theNLM_F_EXCL-plus-tolerated-EEXISTidempotency pattern for rule adds,NLM_F_REPLACEfor the table route, andNLMSG_ERRORack decoding all look correct and are unit-tested at the byte level. - Confirmed the compatibility flag flows through the whole stack —
#[serde(default)]onSnapshotMeta.net_invariantmeans pre-PR snapshots deserialize tofalseand take theLegacySnapshotTAP shape with the guest reconfig RPC intact.
ℹ️ TapMode::Invariant is selected on a different condition than net_invariant is recorded
sandbox/lifecycle.rs:126 activates the TAP as TapMode::Invariant unconditionally whenever a network allocation exists, but line 195 records net_invariant = network.is_some() && !spec.boot_args.contains("ip="). When a caller supplies their own ip=, the TAP gets the invariant NAT (DNAT'ing to 169.254.100.2) while the guest boots a different address, and any snapshot taken from it would restore as LegacySnapshot — changing the TAP's local address out from under it.
This is unreachable today because boot_args is never populated from a request, which is why it's informational rather than a bug. The question for you is whether the contains("ip=") guard is meant to be a supported escape hatch (in which case activate needs the same condition) or is vestigial (in which case dropping it makes the two sites agree by construction).
Technical details
# `net_invariant` and `TapMode` disagree on the caller-supplied `ip=` case
## Affected sites
- `virt/arcbox-vm/src/sandbox/lifecycle.rs:126` — `activate(net, TapMode::Invariant)` with no `boot_args` condition
- `virt/arcbox-vm/src/sandbox/lifecycle.rs:195` — `net_invariant` gated on `!spec.boot_args.contains("ip=")`
- `virt/arcbox-vm/src/sandbox/boot.rs` — appends the fixed `ip=` param only when `spec.boot_args` lacks one
## Required outcome
- The condition that decides the TAP's addressing mode and the condition that records it in instance/snapshot metadata must be the same expression, so no reachable input can produce an invariant TAP paired with `net_invariant: false`.
## Open questions for the human
- Is a caller-supplied `ip=` a supported configuration, or is the guard vestigial now that the sandbox API always passes an empty `boot_args`? If vestigial, deleting it (and the `boot.rs` branch) removes the divergence outright.ℹ️ Nitpicks
virt/arcbox-vm/src/network/rtnetlink.rs:175—libc::recvisn't retried onEINTR. A signal arriving duringinstall()would fail the whole TAP activation (and, viaactivate's rollback, the sandbox creation) for a spurious reason. Alooparound therecvthat continues onEINTRwould close it.guest/arcbox-agent/Cargo.toml:65—arcbox-vmis added to the unconditional[dev-dependencies], while the real dependency sits under[target.'cfg(target_os = "linux")'.dependencies](line 54). Since the tests that consume it are Linux-gated anyway,[target.'cfg(target_os = "linux")'.dev-dependencies]would match the existing shape and keep the Firecracker VMM crate out of macOS dev-host test builds.
Claude Opus | 𝕏

Stacked on #561 (← #560 ← #559 ← #558 ← master); merge in order. Part of CORE-81, project Sandbox Cold Start.
What (4 commits)
Every sandbox guest now boots the identical link-local identity (
ip=169.254.100.2::169.254.100.1:255.255.255.252::eth0:off); the pool IP the rest of the system sees is applied host-side, per TAP. Snapshots become network-agnostic: anetwork_overriderestore of an invariant snapshot needs zero guest-side work — the eth0-reconfig vsock RPC and its resolv rewrite are skipped entirely (the reconfig wiring stays for legacy snapshots, gated by anet_invariantflag in snapshot metadata,serde(default)so old metas read false; chained checkpoints record the guest's actual addressing).Mechanism (chosen after surveying what the System VM actually ships — iptables-legacy 1.8.13 + busybox, no nftables, no NETMAP/CONNMARK in the kernel): per-TAP mark-from-pre-NAT-destination in mangle + DNAT to the fixed guest IP; SNAT back to the pool IP in nat INPUT (so DNS/relays keep seeing pool IPs) and POSTROUTING (mark-scoped, so real client IPs are never rewritten); post-DNAT routing via per-sandbox fwmark fib rule + table (mark = table = pool-ip u32) programmed over hand-rolled rtnetlink (busybox
ipfwmark support is build-dependent). Expose DNAT targets the guest IP with a mangle MARK companion; boot rules gain an explicit vmtap→pool isolation drop pair, making the "sandboxes are deliberately isolated" claim enforced rather than incidental. vm-agent is untouched — it derives DNS fromip=, so resolv.conf is invariant automatically.Also updates the sandbox smoke's fresh-net assertions to the new contract (guest carries the invariant IP; pool IPs never appear in-guest).
Hardware-validated (full 5-layer stack, probe + smoke)
Pool-hit restores with the net RPC deleted: best-case total 83–88 ms (
spawn 17–22 + stage 0 + load 4 + guest_cfg ≈58— guest_cfg is now the clock RPC alone); probe p50 128 ms with back-to-back-restore variance attributable to pool refill I/O racing the next resume (worst-case pattern; noted for CORE-78 follow-up). Full sandbox smoke green: both restore modes, expose, file I/O, docker template.Review notes / risks for reviewers
src_valid_mark; mangle LOCAL_OUT reroute-on-mark and LOCAL_IN SNAT are standard kernel semantics but called out for scrutiny.Validation
151 arcbox-vm tests green (new: exact NAT rule argv pinning, rtnetlink encodings, snapshot serde compat, expose both-halves removal, sweep matching); clippy clean host + musl (zero new warnings on arcbox-agent's changed lines); fmt clean.