Conversation
|
@claude-fleet review |
|
Review task queued. Task: 7bd82c6d-86ba-4b2c-833a-251fe77313d2 |
There was a problem hiding this comment.
Verified the finding by reading the code paths. Writing the review.
Review
Verdict: comment. The core fix is correct and well-tested — --net shared on hvi without a gateway now refuses instead of booting a guest that silently reaches nothing, which is what #48 asked for. One should-fix: the remediation the new error prints does not actually work as written.
What I verified holds
- Both call sites are correct and consistent. The pre-pull check (
run.go:217) readsnetModeandgatewaySock, both assigned earlier inrunInstance(run.go:178,199), so they are in scope. The post-resolve check (run.go:487) runs aftervmmTypeis set. Both route throughnormalizeVmmName, so they cannot disagree aboutqemu-hvf. normalizeVmmNameextraction is behaviour-preserving — identical arms to the inline switch it replaced.- Backend identity is right.
HviVmm == "hvi",VzVmm == "vz"in the vendoredhypervisorspackage;normalizeVmmName("hvi")is"hvi", so--hypervisor hvitriggers the refusal andvz/qemudo not. - The test is not a tautology in structure. It asserts the refusal message names
--gateway-sock,network-gateway, and--net none; the actual error string contains all three. Theshredcase confirms an unvalidated--nettypo is still refused. I could not execute it (see gates), but the assertions bite by construction: revertingcheckHviNetworkingtoreturn nilfails everywantErr: truecase. - No conflict with the existing hvi net handling (
containerBootResolveratrun.go:1730,vzNetArgs). - Docs updated consistently across README,
backends.md,cli.md,networking.md,troubleshooting.md; the table cells and prose now say "refused" in agreement.
Gates
This is a linux host; cmd/hull is darwin-only (//go:build darwin on run.go, supervisor.go, etc.), so the package does not build or test natively here — go vet ./cmd/hull/ fails on undefined: supervisedService, which is the build-constraint mismatch, not the branch.
GOOS=darwin go build ./cmd/hull/— clean.GOOS=darwin go vet ./cmd/hull/— clean.GOOS=darwin go test -c ./cmd/hull/— the test binary compiles (cannot run a darwin binary on linux).
I did not run make test or the new test end-to-end. The PR's claim that it passes is plausible but unverified from this host.
Finding: the error's "exact two commands" fail for hvi
The refusal prints, as the way out:
hull network-gateway --socket /tmp/gw.sock &
hull run --hypervisor hvi --net shared --gateway-sock /tmp/gw.sock --gateway-cidr 10.87.0.10/24 ...
The second command does not reach the gateway the first starts. On hvi (and qemu), the run path dials the QEMU stream socket at qemuGatewaySock(gatewaySock) = gatewaySock + ".qemu" (run.go:582-592, gateway.go:394) and pre-flights it with a net.DialTimeout. The gateway only opens that socket when started with --qemu-socket (gateway.go:238 — if qemuSockPath != ""; the flag has no default). Started with --socket /tmp/gw.sock alone, it never listens on /tmp/gw.sock.qemu, so the run fails with failed to reach network gateway at /tmp/gw.sock.qemu. This is exactly why compose passes --qemu-socket qemuGatewaySock(sockPath) explicitly (compose.go:1641).
So a user who copy-pastes the printed commands trades "boots and reaches nothing" for "refused, then the suggested fix also errors." The gateway line needs --qemu-socket /tmp/gw.sock.qemu. The validation table's --gateway-sock row shows only that the run proceeds past the new check ("No VM was booted"), so this second failure was not exercised.
The same omission is in the hand-start examples in networking.md:114, troubleshooting.md:160, and pre-existing network-egress.md:227 — repo-wide, not introduced here, but worth a sweep since this PR rewrites those sections. (cli.md:196 claims --qemu-socket defaults to "--socket plus .qemu", which the gateway code does not do — that stale doc is the likely origin of the pattern.)
Not blocking: the safety goal of #48 is met. But the message's entire value is naming commands that work, so it should be corrected before merge.
Not checked:
- make test / the new test end-to-end (linux host; cmd/hull is darwin-only, so tests cannot run natively — only GOOS=darwin build, vet, and test-binary compile were done)
- External hvi claims restated in code/docs: TCP path ending at
None // no egress yet, the(deny default)Seatbelt profile, and that socket(2) succeeds while connect(2)/sendto(2) return EPERM — hvi source is not in this repo and was not read - Real end-to-end hvi networking (host has SIP/entitlement constraints per the PR; no VM booted)
Task 7bd82c6d-86ba-4b2c-833a-251fe77313d2 on ci-16gb-fsn1-1, model executor default, effort high.
| return fmt.Errorf("--net %s cannot work on the hvi hypervisor without the network gateway: "+ | ||
| "hvi's built-in stack forwards no traffic, so the guest would take an address and "+ | ||
| "reach nothing. Start a gateway and join it:\n"+ | ||
| " hull network-gateway --socket /tmp/gw.sock &\n"+ |
There was a problem hiding this comment.
should_fix: The remediation this error prints does not connect. On hvi the run path dials qemuGatewaySock(gatewaySock) = gatewaySock+".qemu" (run.go:582-592, gateway.go:394) and pre-flights it with DialTimeout. The gateway only opens that socket when given --qemu-socket (gateway.go:238; the flag has no default), which is why compose passes it explicitly (compose.go:1641). Starting hull network-gateway --socket /tmp/gw.sock alone never listens on /tmp/gw.sock.qemu, so the printed hull run ... --gateway-sock /tmp/gw.sock fails with failed to reach network gateway at /tmp/gw.sock.qemu. The gateway line needs --qemu-socket /tmp/gw.sock.qemu. The --gateway-sock validation-table row only confirms the run passes the new check (no VM booted), so this second failure was not exercised. Same omission in networking.md:114, troubleshooting.md:160, network-egress.md:227.
`hull run --hypervisor hvi --net shared IMAGE` booted a guest that took a DHCP address and could not reach anything, and reported success. The old README used exactly that command to run nginx. Two independent reasons, and neither is a feature waiting to be finished. hvi's built-in stack is a responder inside the VMM. It answers ARP, ICMP, DHCP and DNS for a fixed address set and forwards nothing: the TCP arm ends at `None // no egress yet`, and any UDP that is not DHCP or a query to its own resolver is dropped the same way. hvi's own README lists "no egress from the built-in network stack" among its known limits. More fundamentally, hvi installs a `(deny default)` Seatbelt profile as the last thing before it starts the vCPUs. Under that profile socket(2) still succeeds but connect(2) and sendto(2) both return EPERM, measured against the profile hvi ships. So the confined VMM cannot originate traffic even if the forwarding code existed. Its own resolver is caught by the same rule: getaddrinfo has to reach mDNSResponder and nothing earlier in the boot warms that connection, so the query comes back with no records. That is also why the gateway works and why this refusal exempts it. hvi connects the gateway socket before confinement; the sandbox comment names it among the host authority acquired above that line. So refuse, and name the way out. hvi takes the same line itself, refusing --net-tap on macOS and naming --net-gateway. Nothing in the repository used the rejected combination: test/hvi-boot-test.py passes --net none with a comment saying networking on hvi needs the gateway. The check runs twice on purpose. The flag names the backend before the pull, so a caller gets the error immediately rather than after a download; the backend can also come from an image annotation, which is only known afterwards. Both call sites share one function, and normalizeVmmName is extracted so they cannot disagree about "qemu-hvf". Refs: #48 Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
Follows the behavior change in the previous commit. These pages had just been written to document the old behavior accurately, which is now the wrong thing to document. Two substantive corrections beyond the change in behavior, both from measuring the sandbox rather than reading it: The pages said the profile blocks opening an outbound socket. It does not. socket(2) succeeds; connect(2) and sendto(2) are what return EPERM. A reader chasing the first claim would look in the wrong place. The pages treated the missing TCP forwarding and the failing resolver as two separate problems. They have one cause. The VMM confines itself before the guest runs, so it cannot originate traffic at all, and the resolver needs mDNSResponder that nothing warmed. That also explains why the gateway is exempt, which is the part worth understanding: its socket is connected before confinement. On hvi the gateway is the design, not a workaround, and the pages now say so. Refs: #48 Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
8c56e7d to
58b9d89
Compare
|
@claude-fleet review |
|
Review task queued. Task: 98056cee-ae6c-4c09-b077-8f05eb7ac8c1 |
There was a problem hiding this comment.
Reviewed at 58b9d89 (bfbff42 code + 58b9d89 docs), diffed against the merge base 609e0dd. Full pass: I read the hvi source at the pinned submodule commit c5305f9 rather than the comments about it, swept the repo for other entry points into the rejected combination, and mutation-tested the new test.
Verdict: request changes. The check itself is correct and well-placed, and every claim the description makes about hvi holds. But the remediation the new error prints does not work, which is the one thing a refusal has to get right — that is the prior thread on run.go:1797, still unfixed at this commit, and I am still asking for it. Separately, a fifth docs page kept the old framing.
What I verified holds
The hvi claims, read against hvi-vmm@c5305f9, not the comments:
virtio_net.rs:544-556— the TCP arm captures, logs and returnsNone // no egress yetat line 555. Confirmed.virtio_net.rs:530-541— UDP is forwarded tohandle_dhcpon port 67 andhandle_dnson port 53 toDNS_IP; everything else logs, captures and returnsNone. The description's "any UDP that is not DHCP or a query to its own resolver is dropped the same way" is exact.sandbox.rs:123is(deny default), andmachine_macos.rs:465-481enters it as the last statement before the vCPU threads are spawned at 484-487. The comment there names "the gateway connection" among the host authority acquired above the line, as does the module doc atsandbox.rs:37. The description's reason for exempting the gateway is the code's own stated reason.virtio_net.rs:658-660—handle_dnsresolves viato_socket_addrs, i.e.getaddrinfo. Confirmed.machine_macos.rs:239-241refuses--net-tapon macOS with an error naming--net-gateway. The precedent argument is real.hvi-vmm/README.md:372lists "No egress from the built-in network stack" under known limits. Address set10.0.2.15/10.0.2.2/10.0.2.3matchesGUEST_IP,GW_IP,DNS_IPatvirtio_net.rs:83-85.
The two corrections the PR claims to have found by measuring are both real corrections. The deleted sentence said hvi's selftest "asserts that opening an outbound socket fails once the sandbox is entered". sandbox.rs:407-414 shows the probe labelled "open an outbound TCP socket" actually calls TcpListener::bind("127.0.0.1:0"), with a comment explaining it binds rather than connects on purpose. That probe proves nothing about socket(2), so the old page was reporting a probe label as a result. Replacing it with your own connect/sendto measurement is the right fix and worth keeping in the docs.
Placement. The pre-pull call site at run.go:216-224 sits above globalStore(cmd) at 251 and s.CreateInstance at 266, so "refused, before the store is opened" in the validation table is accurate. The post-resolve call at 487 sits inside the instanceStarted defer at 271-277, so the refusal after a pull cleans up its instance directory rather than squatting the name.
Coverage of the whole class, not the instance. runInstance is the only builder of this configuration: serviceRunArgs at compose.go:241-244 always passes --net shared with both gateway flags, so compose is exempt by construction; checkpoint.go:85 requires the Vz backend, so restore cannot resurrect an hvi guest around the check; grep -rn '"--hypervisor"' over cmd/ and internal/ returns only compose.go:242. Both call sites share one function and one normalizer, so they cannot disagree. --net defaults to "none" (run.go:64-66), so a bare hull run --hypervisor hvi IMAGE is untouched.
The test bites. I extracted checkHviNetworking and TestCheckHviNetworking verbatim into a standalone module and ran five mutations. Every one fails, each on the case it should:
| mutation | fails on |
|---|---|
return nil unconditionally |
hvi_shared_without_a_gateway_is_refused, hvi_unrecognised_net_mode_is_refused |
drop the netMode == "none" exemption |
hvi_none_needs_no_gateway |
drop the gatewaySock != "" exemption |
hvi_shared_with_a_gateway_is_allowed |
drop --net none from the message |
both refusal cases, on the message assertion |
drop network-gateway from the message |
both refusal cases, on the message assertion |
No tautology. The message assertions are load-bearing, which is unusual and good.
Gates
| gate | result |
|---|---|
GOOS=darwin GOARCH=arm64 go build ./... |
pass |
GOOS=darwin GOARCH=arm64 go vet ./... |
pass |
GOOS=darwin GOARCH=arm64 go test -c ./cmd/hull/ |
pass (test binary compiles) |
go test -count=1 ./internal/... ./pkg/... (linux) |
pass, no failures |
bash test/harness-selftest.sh |
6 passed, 0 failed, 5 skipped (the hvi cases skip on Linux/x86_64, as the Makefile comment says) |
I could not run make test or go test ./cmd/hull/ for real: this is a Linux/x86_64 host and cmd/hull is darwin-only. Native go vet ./... fails with vet: cmd/hull/supervisor_test.go:578:15: undefined: supervisedService, because supervisor.go:15 carries //go:build darwin and supervisor_test.go carries no tag. That is pre-existing — I reproduced it on the merge base 609e0dd in a scratch worktree — and it is not this branch's fault, nor a contradiction of your "go vet clean" claim, which is true on the target platform. Worth a separate issue for the missing tag, since it means go vet on a Linux CI runner would never be clean.
Findings
-
The printed remediation still does not work — the prior thread on
run.go:1797, unresolved at this commit. I re-derived it independently and it is correct:run.go:588-593pre-flightsqemuGatewaySock(gatewaySock)=gatewaySock + ".qemu"(gateway.go:425-427) with a hardDialTimeoutfailure, andrunGatewayonly claims that socket when--qemu-socketis non-empty (gateway.go:240-241); the flag has noValue(gateway.go:99), which is why compose passes it explicitly atcompose.go:1641. So the exact pair this PR's new error prints —hull network-gateway --socket /tmp/gw.sock &thenhull run ... --gateway-sock /tmp/gw.sock— fails withfailed to reach network gateway at /tmp/gw.sock.qemu. The user is walked from one refusal into a second. This is the only part of the change a user actually reads, so it should not merge as-is. Not re-anchored inline below, to keep the existing thread the single place it is discussed. -
docs/running.md:80-82is the fifth page and still carries the old framing. Detail inline. -
containerBootResolver's hvi branch is now dead. Detail inline. -
The explanation is now written out six times. Detail inline.
Smaller notes
- The refusal text interpolates
netModebut hardcodes--net sharedin the suggested command. That is the right call — a user who typedshredgets the correct spelling back — worth keeping if anyone questions it. troubleshooting.md:147uses the error string itself as the heading, which makes the page findable by pasting the error. Good.docs/networking.mddropped "ARP is answered only for the gateway and DNS addresses", which was true (virtio_net.rs:493). The replacement, "for a fixed address set", is not wrong, just less specific. No action needed.
Not checked:
- End-to-end behavior on a real HVF host: this is Linux/x86_64, so no VM was booted and the validation table's six rows were confirmed by reading the code paths, not by running hull.
make testandgo test ./cmd/hull/as such: cmd/hull is //go:build darwin and cannot run here. I compiled the darwin test binary and ran the extracted function under mutation instead.- The sandbox measurement in the description (socket()=3, connect()=EPERM, sendto()=EPERM) was not re-measured; I verified the profile text, the entry point and its ordering, which is what the conclusion rests on.
- The annotation-driven post-resolve path was not exercised by booting an image carrying com.urunc.unikernel.hypervisor=hvi; I confirmed the call site, its position relative to the instance-dir cleanup defer, and that both call sites share normalizeVmmName.
- Whether
--hypervisor HVIor other casings reach a clear error: normalizeVmmName does not fold case, so neither check fires, but I did not trace what the VMM factory reports for an unknown backend name. Pre-existing either way. - docs/network-egress.md, which the prior thread also names for the --qemu-socket omission, is not in this diff and I did not review it.
- Whether the pre-existing
go vetfailure on Linux (supervisor_test.go missing a darwin build tag) is already tracked in an issue.
Findings outside the diff:
- docs/running.md:80 (should_fix): This page kept the pre-PR framing. "
--net sharedgives the guest NAT onvz. It does not give egress onhvi." is literally true but implies the run proceeds and produces a dead network, which is exactly the behavior this PR removes. It then sends the reader to networking.md, which now says the combination is refused, so the two pages disagree.
The description says "the four pages that documented the old behavior now describe the refusal"; running.md is a fifth. It also matters more than the others, because it sits in the walkthrough a new user reads before they ever open networking.md: they will expect a booted guest with no egress and get a hard error instead.
Suggest matching the others: --net shared gives the guest NAT on vz. On hvi it is refused without --gateway-sock.
Task 98056cee-ae6c-4c09-b077-8f05eb7ac8c1 on ravel-clickbench-ubuntu, model executor default, effort high.
| // This deliberately does not fire when a gateway socket is given: that is the | ||
| // supported way to network an hvi guest, and `hull compose` uses it. | ||
| func checkHviNetworking(vmm hypervisors.VmmType, netMode, gatewaySock string) error { | ||
| if vmm != hypervisors.HviVmm || netMode == "none" || gatewaySock != "" { |
There was a problem hiding this comment.
nit: This check makes the hvi branch of containerBootResolver (run.go:1730-1732) unreachable, and the docs paragraph that described it was deleted in the same commit.
The reasoning: containerBootResolver returns the gateway address first when gatewayCIDR != "" (run.go:1726-1729), and only falls through to return "10.0.2.3" for HviVmm && netMode != "none". After this change, hvi with netMode != "none" implies gatewaySock != "", and run.go:199-201 requires --gateway-sock and --gateway-cidr together, so gatewayCIDR is always non-empty on that path. The first branch always wins.
That branch was the code behind the networking.md sentence this PR removed ("hull still writes nameserver 10.0.2.3 into the boot initrd for an hvi generic container boot with any net mode other than none"). Deleting the sentence was right; leaving the code means the function's doc comment ("HVI's built-in stack has a fixed DNS endpoint") now explains a path that cannot be taken, and a future edit to checkHviNetworking would silently re-arm a resolver address with no responder behind it. Either drop the branch or leave a line saying it is now reachable only if the refusal is relaxed.
| return name | ||
| } | ||
|
|
||
| // checkHviNetworking refuses a net mode the hvi backend cannot honor. |
There was a problem hiding this comment.
nit: This 21-line comment guards a 5-line function, and the same explanation now exists in six places: here, run_network_test.go:98-105, docs/networking.md:69-100, docs/backends.md:168-186, docs/troubleshooting.md:147-153, and README.md:124-130 (docs/cli.md:94-96 carries a one-clause version, which is the right size for a table entry). Four of those spell out both reasons in full.
The docs copies mostly pre-date this PR, so this is not a pattern you introduced -- but you added two more, and every copy is a sync obligation the next person to touch hvi networking has to find. The paragraphs about getaddrinfo/mDNSResponder and about the gateway socket being connected before confinement are background for the decision, not information a reader of this function needs.
Suggest keeping the first two sentences (what is refused and why egress does not exist) plus a pointer to docs/networking.md as the normative home, and moving the sandbox measurement there. What genuinely belongs here and nowhere else is the last paragraph, which explains why the gateway is exempt -- that one is about this code.
| } | ||
| } | ||
|
|
||
| // A guest on hvi with `--net shared` and no gateway takes an address and |
There was a problem hiding this comment.
nit: This eight-line preamble restates the doc comment on checkHviNetworking in the same package. The part that earns its place is the sentence inside the table at line 128-130, explaining why the "shred" case exists -- that one records a real design decision (--net is unvalidated, per #49) which is not obvious from the case name and would be the first thing deleted by someone tidying up. The header above the function is background the reader can get from run.go:1769.
Worth cutting to one line: "hvi cannot honour a net mode other than none without the gateway; see checkHviNetworking. The gateway case must keep working, because hull compose drives it."
Summary
hull run --hypervisor hvi --net shared IMAGEbooted a guest that took a DHCPaddress, could not reach anything, and reported success. Fixes #48 by refusing
the combination and naming the gateway.
The investigation is written up in
#48; the
short version is that there are two independent causes and neither is a feature
waiting to be finished.
hvi's built-in stack is a responder inside the VMM. It answers ARP, ICMP, DHCP
and DNS for a fixed address set and forwards nothing: the TCP arm ends at
None // no egress yet, and any UDP that is not DHCP or a query to its ownresolver is dropped the same way.
More fundamentally, hvi installs a
(deny default)Seatbelt profile as the laststatement before it starts the vCPUs. Measured against the profile hvi ships:
So the confined VMM cannot originate traffic even if the forwarding code
existed, and its own resolver fails for the same reason:
getaddrinfohas toreach
mDNSResponderand nothing earlier in the boot warms that connection.That is also why the gateway is exempt from this refusal. hvi connects the
gateway socket before confinement, and the sandbox comment names it among the
host authority acquired above that line. On hvi the gateway is the design, not
a workaround.
Related issues
Fixes #48. Refs #20, #21 for the gateway direction.
Built on the pages #62 added, and rebased onto
mainnow that it has merged.Changes
checkHviNetworkingrefuses a net mode other thannoneon the hvi backendwhen no
--gateway-sockis given, with an error that names the gateway, theexact two commands to use instead, and
--net none.--hypervisornames the backendbefore the pull, so the common case fails immediately instead of after a
download; the backend can also come from an image annotation, which is only
known once the image is resolved. Both call sites share one function.
normalizeVmmNameextracted from the inline switch, so the pre-pull andpost-resolve checks cannot disagree about what
qemu-hvfmeans.TestCheckHviNetworkingcovers the refusal, the gateway exemption, both--net nonecases, the two untouched backends, and an unrecognised net mode.That last case matters:
--netis not validated against a set (run: --net and --rootfs-type accept unvalidated values #49), sowithout it a typo would slip past the check.
the reason for it, including two corrections found by measuring rather than
reading: the profile does not block
socket(2), and the missing forwardingand the failing resolver have one cause rather than two.
Why refuse rather than warn
Every neighbouring check in this file refuses:
--guiand--rosettaoff vz,--net nonewith--gateway-sock, a read-only share on QEMU. hvi itselfrefuses
--net-tapon macOS and names--net-gatewayin the error. A warningwould leave the failure mode that #48 reports intact, just with a line above it.
Nothing in the repository used the rejected combination.
test/hvi-boot-test.py:86-88already passes--net none, with the comment"Networking on hvi needs the gateway and is a separate concern."
If a local-responder-only mode is ever genuinely wanted, it deserves its own
--netvalue rather thanshared, which promises egress.Validation
make testpasses,go vetclean, and the new test passes on its own.Verified against a build from this branch, with a short store path:
--hypervisor hvi --net shared--hypervisor hvi --net shred--hypervisor hvi --net shared --gateway-sock ... --gateway-cidr ...--hypervisor hvi --net none--hypervisor vz --net shared--hypervisor qemu --net sharedNo VM was booted. This host has SIP enabled with only ad-hoc local
binaries, so hvi would be refused its entitlement before reaching any
networking. The sandbox measurements above are real, taken against hvi's own
profile text compiled standalone; everything else is source at
c5305f9, thecommit
mainrecords. The post-resolve call site is covered by the unit testand by reading, not by an annotation-driven boot.
A reviewer with a working HVF host and a signed build can confirm the
end-to-end behavior with the table above.
Checklist
make testpasses— Go only, no Swift or Rust source touched.make macosbuilds hull, vz-runner and hvi, if Go, Swift or Rust codechanged
go buildandmake testpass.I have run the e2e harnesses (— this host cannot run them, as above. Thetest/*.py) for changes touching boot,console, or the run path
affected harness (
hvi-boot-test.py) uses--net noneand is unaffected bythe refusal.
docs/)