feat(minimald,sandbox2): wire net switch into live OwnIp launch path - #547
Conversation
`sandbox2` treated `OwnIp` like `NoNet` and `SandboxLauncher` never constructed a switch, so an `OwnIp` PTask got an empty namespace and nothing downstream (#499/#500) had a running gvproxy to attach policy to. This lands the live R1.5 wiring on the minimald sandbox2/launcher side. - server.rs: construct the per-host `GvproxySwitch` + `IpAllocator` once at daemon scope (R1.4 one-gvproxy-per-host, R1.6 process-lifetime allocator) and thread the shared `Arc<Mutex<GvproxySwitch>>` through the sessions manager and session actor into every `SandboxLauncher`. The gvproxy binary path comes from a new `Config::gvproxy_bin` field (fixed install-path default when unset), not the test's `GVPROXY_BIN`. - session_host.rs: on an `OwnIp` launch, drive the attach on the minimald side — allocate a lease, `open_tap`, move the tap into the PTask netns (targeted by the `hakoniwa::Child`'s PID) and configure its MAC + `100.64.0.0/16` address + route, `attach_to_switch`, and hold the `SwitchRelay` (plus a switch-detach guard) for the session lifetime. `HostNet`/`NoNet` are unchanged. - net/switch.rs: add `tap_netns_commands` (single-sourced move/configure argv) + `move_tap_into_netns`; net/mod.rs exposes `SwitchSubnet::prefix` and `GvproxySwitch::subnet`. - sandbox2: no `minimald::net` call (that would be a dependency cycle); only the empty namespace is unshared and the PID surfaced. The stale `OwnIp` comment is corrected accordingly. - tests/netns.rs: the UC6 proof drives the production `tap_netns_commands` against a PID-identified netns (the same `CLONE_NEWNET` sandbox2 unshares), not a hand-rolled `ip netns` sequence. Refs #499, #525. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a daemon-scoped ChangesOwnIp Gvproxy Switch Wiring
Sequence Diagram(s)sequenceDiagram
participant ServerState
participant Manager
participant Session
participant SandboxLauncher
participant GvproxySwitch
participant SandboxProcess
ServerState->>GvproxySwitch: new(gvproxy_bin_path, state_dir)
activate GvproxySwitch
deactivate GvproxySwitch
ServerState->>Manager: init(net_switch: Arc<Mutex<GvproxySwitch>>)
Manager->>Session: run(..., net_switch: Arc::clone)
Session->>SandboxLauncher: construct(net_switch: Arc::clone)
SandboxLauncher->>SandboxProcess: spawn (OwnIp mode)
activate SandboxProcess
SandboxProcess-->>SandboxLauncher: Child (netns_pid)
deactivate SandboxProcess
SandboxLauncher->>GvproxySwitch: attach() lease
activate GvproxySwitch
GvproxySwitch-->>SandboxLauncher: PtaskLease, SwitchSubnet
deactivate GvproxySwitch
SandboxLauncher->>SandboxLauncher: open_tap + move_tap_into_netns(netns_pid)
SandboxLauncher->>GvproxySwitch: attach_to_switch(relay_fd)
activate GvproxySwitch
GvproxySwitch-->>SandboxLauncher: SwitchRelay
deactivate GvproxySwitch
SandboxLauncher-->>Session: OwnIpAttachment (guards relay)
Note over Session: Session lifecycle holds guard<br/>On drop: async detach()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minimald/src/session_host.rs`:
- Around line 742-749: The spawned sandbox process is not being cleaned up when
the attach_own_ip call fails at the error boundary where it returns with the
question mark operator. To fix this, either set kill_on_drop(true) on the
hakoniwa::Command before calling spawn (similar to the pattern used in
exec.rs:344 and net/mod.rs:502), or explicitly call kill on the process variable
in the error path before returning if attach_own_ip fails. The key is ensuring
that when attach_own_ip returns an error for the OwnIp NetworkMode case, the
process variable does not simply drop and orphan the child process.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 383c312d-171d-4843-b601-31ba9f0c5c60
📒 Files selected for processing (10)
crates/minimald/src/main.rscrates/minimald/src/net/mod.rscrates/minimald/src/net/switch.rscrates/minimald/src/server.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/src/sessions.rscrates/minimald/src/test_harness.rscrates/minimald/tests/netns.rscrates/sandbox2/src/lib.rs
|
Revise claim for head 103f944. |
|
Auto-revise 1 of 3. |
This comment has been minimized.
This comment has been minimized.
When `attach_own_ip` failed during an `OwnIp` launch, the spawned `hakoniwa::Child` was dropped without being killed. A `hakoniwa::Child` does not terminate on drop (it orphans the child, the same hazard the `kill_on_drop(true)` calls in `exec.rs`/`net/mod.rs` guard against), so a failed switch attach left the sandbox process running. Kill and reap the process explicitly in the attach error path before propagating the error. Also document two correctness constraints surfaced in review: sessions must be drained before the tokio runtime is stopped so each `OwnIpAttachment`'s scheduled `detach` runs, and the PTask network namespace is empty until `attach_own_ip` returns. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Commit pushed:
|
|
Addressed the review feedback in a follow-up commit on this branch. 1. (Major, CodeRabbit) Orphaned sandbox process on 2. (Low) Shutdown ordering — documented on the 3. (Low) Empty-namespace startup window — documented at the launch site: the PTask netns is empty until Verification (touched crates):
The full-workspace build is still blocked here only by
|
This comment has been minimized.
This comment has been minimized.
The OwnIp tap setup loop in move_tap_into_netns used the prefix "configuring PTask tap failed" for every command, but command 0 moves the tap into the PTask namespace (ip link set <tap> netns <pid>) rather than configuring it. Name the failing phase by command index so a move failure no longer reports as a configuration failure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Commit pushed:
|
|
Addressed the review note on
Verified green on the touched crate:
|
This comment has been minimized.
This comment has been minimized.
The OwnIp attach error path used an `else if`, so `process.wait()` only ran when `process.kill()` succeeded. When `kill` fails with `ESRCH` because the sandbox process already exited during the attach window, `wait` never ran and the child was left as a zombie for the daemon lifetime. Split into two independent `if` arms so the child is always reaped — the standard SIGKILL-then-waitpid idiom. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Commit pushed:
|
|
Addressed the The error arm used Verified green for the touched crates:
|
This comment has been minimized.
This comment has been minimized.
move_tap_into_netns dispatched ip/nsenter by PATH-relative name while holding CAP_NET_ADMIN in the host namespace; a binary shadowing them early in an inherited PATH would run at that capability. Resolve the program against a fixed trusted directory list and pin the same PATH on the child so nsenter's inner ip resolves safely too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
sdd-validate findings — Implementation boundaryPR: #547 · Head SHA: Gate 1 — Proof artifacts re-executed and passingBoth proof artifacts declared in task #542 are subject to the same infrastructure limit as prior validation passes: the agent sandbox has no Rust toolchain and no outbound registry egress (
No proof artifact ran and reported a failure. Each is blocked only by the network constraint before exercising the change, and each is covered by a named consumer required check. No Gate 2 — Changed files within task scopeTask #542 The PR changes 7 additional files outside the explicit list:
No protected paths touched (no Result: Gate 3 — No real credentials in the diffPass. No API keys, tokens, secrets, or credentials in the diff. Note on commit
|
| Gate | Result |
|---|---|
| G1 — Proof artifacts | i️ ×2 Info — deferred to consumer CI (ci-netns.yml, ci.yml/ci-success) |
| G2 — Files in scope | |
| G3 — No credentials | ✅ Pass |
No Blocker findings. No needs-human applied.
Lifecycle: tracking issue #478 already carries sdd:review — no label move needed (idempotent).
Generated by sdd-validate for PR #547
Generated by sdd-validate for issue #547 · ◷
| Ok(relay) => relay, | ||
| Err(e) => { | ||
| let _ = switch.lock().await.detach().await; | ||
| return Err(e); |
There was a problem hiding this comment.
LOW · correctness — When the inner async block fails and the rollback switch.detach() also fails, its error is silently discarded:
let _ = switch.lock().await.detach().await;If detach returns Err here, the gvproxy refcount stays elevated and gvproxy will not stop when the last PTask eventually leaves ("a leaked count would keep it running", per the comment above). The OwnIpAttachment::drop path uses tracing::warn! on detach failures; matching that approach here would surface this edge case for debugging:
Err(e) => {
if let Err(de) = switch.lock().await.detach().await {
tracing::warn!(error = %de, "rolling back switch attach after OwnIp setup failure");
}
return Err(e);
}| return candidate.to_string_lossy().into_owned(); | ||
| } | ||
| } | ||
| program.to_string() |
There was a problem hiding this comment.
LOW · security — When trusted_program returns a bare name (the fallback for unusual layouts), Command::new(bare_name) resolves the executable using the parent process's PATH environment variable, not TRUSTED_EXEC_PATH. The .env("PATH", TRUSTED_EXEC_PATH) call on the Command is the child's environment — it applies to programs the child invokes (e.g. the ip that nsenter -n re-execs inside the PTask namespace), not to the initial exec lookup. A tampered parent PATH in this fallback branch could shadow ip or nsenter at CAP_NET_ADMIN.
The code comment correctly notes "just without the hardening" and in practice ip / nsenter will be under /sbin or /usr/sbin on every standard Linux distribution, so the fallback is rare. No change is strictly required, but extending the comment to clarify that .env("PATH", ...) does not protect the initial exec lookup (only the child's inner calls) would prevent a future maintainer from assuming full coverage when the fallback fires.
Lands the live R1.5 wiring for the minimald sandbox2/
SandboxLauncherside: the
minimald::netbuilding blocks from Unit 1 (#496, merged via #525)are now connected to the real session-launch path, so an
OwnIpPTask gets atap bridged onto a running per-host gvproxy switch instead of an empty
namespace. This unblocks #499 (egress/ingress policy) and #500, which need a
running switch to attach policy to. The minvmd-side analog is #535/#540.
The three architectural decisions confirmed on the issue are applied as agreed:
GvproxySwitch+IpAllocatoris adaemon-scoped singleton constructed once in
server.rs(R1.4 one gvproxyper host, R1.6 process-lifetime allocator). A shared
Arc<tokio::sync::Mutex<GvproxySwitch>>is threaded through the sessionsmanager and session actor into each per-launch
SandboxLauncher, whichdrives attach/detach against it rather than owning its lifecycle.
sandbox2↔minimald::netboundary —sandbox2makes nominimald::netcall (that would be a dependency cycle). It only unsharesthe empty network namespace; the launched
hakoniwa::Child's PID(
id(), whose/proc/<pid>/ns/netis the PTask netns — the unsharehappens before hakoniwa's internal PID-namespace fork) is what the
minimaldside targets. The staleOwnIpcomment insandbox2iscorrected. All tap/
ipcalls live insession_host.rs/net::switch.Config::gvproxy_binfield with a fixedinstall-path default when unset;
GVPROXY_BINstays scoped to the#[ignore]proof.On an
OwnIplaunch the launcher allocates a lease, opens a host tap, movesit into the PTask netns and configures its MAC +
100.64.0.0/16address +route, attaches the relay, and holds the
SwitchRelay(plus a switch-detachguard that decrements the refcount and stops gvproxy after the last PTask
leaves) for the session lifetime.
HostNet/NoNetare untouched, soOwnIpnow diverges from
NoNetin the live path.Proof artifacts
1. Test (CI, deferred) — UC1/UC6 netns proofs drive the production wiring.
crates/minimald/tests/netns.rsUC6 now moves+configures each PTask tap viathe production
minimald::net::switch::tap_netns_commandsagainst aPID-identified netns created by the same
CLONE_NEWNETunsharethatsandbox2::new_containerissues — not a hand-rolledip netnssequence. Theseare
#[ignore]and gated onMINIMALD_NETNS_TEST; they need netns + gvproxy +root and run in
.github/workflows/ci-netns.yml. They compile and are listed(ignored) in the local run:
2. Test (in-sandbox) — the production move/configure command construction.
A new unit test asserts
tap_netns_commandsmoves the tap by PID then entersthe namespace via
nsenter -t <pid> -nfor every config command, renders thelease as
<ip>/<prefix>CIDR, and routes via the switch gateway. It fails onbase (the function does not exist):
3. CLI — the wiring compiles and the unit suites pass;
OwnIpdiverges fromNoNet.cargo test -p minimald -p sandbox2 --lockedandcargo clippy -p minimald -p sandbox2 --all-targets --locked -- -D warnings:cargo build --locked -p minimald -p sandbox2andcargo fmt --all -- --checkare also green.
Verification note
The whole-workspace
cargo build --lockedcould not complete in the buildsandbox because the unrelated
remote-protocrate's build script requiresprotoc, which is not installed here (and cannot be installed withoutprivileges).
remote-protois not inminimald/sandbox2's dependencytree, so this is an environment gap orthogonal to this change — the crates this
PR touches build, test, lint, and format cleanly. The privileged netns e2e is
deferred to
ci-netns.ymlby design.Merging this pull request closes the task sub-issue #542. Once every task
sub-issue of its tracking issue is closed, the pipeline advances that tracking
issue to
sdd:donefor a final human review.Closes #542
Summary by CodeRabbit
Release Notes
OwnIpsession networking by provisioning a tap in the target network namespace, configuring it with the allocated MAC/IP (CIDR) and a default route via the subnet gateway.