Repository navigation
Routed networking binds a published port on the host address its mapping names - #1030
Merged
Merged
Conversation
This was referenced Oct 1, 2026
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (15)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughPort mappings now accept explicit IPv4 and bracketed IPv6 host addresses. Routed TCP proxies bind to the specified address, or to the VM’s loopback IP when omitted. Routed setup allocates and reports that loopback IP only when needed. Relay shutdown, mode-specific validation, CLI help, documentation, and tests are updated. ChangesHost-addressed port forwarding
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Client as Host client
participant Relay as TCP proxy relay
participant Guest as Guest service
Client->>Relay: Connect to mapping host address and port
Relay->>Guest: Open connection to guest address and port
Guest-->>Relay: Return guest response
Relay-->>Client: Forward response
Merge Risk: ⚪ Minimal · up to Host-addressed forwarding, conditional loopback allocation, and snapshot mapping preservation have no established merge-blocking issue. Relay shutdown does not introduce the suspected cleanup stall; the change is mergeable subject to normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Explicit host addresses intentionally make published guest ports reachable beyond loopback, including over both IP families. Defaults remain loopback-bound, and setup and shutdown handling improve. However, externally reachable listeners expose the privileged VM-management process to connection accumulation without an application-level admission limit. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 78.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 60 functions across 12 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4472878f6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ing names
--publish takes [HOSTIP:]HOSTPORT:GUESTPORT[/PROTO]. Rootless networking listens
on HOSTIP. Routed networking ignored it and bound every mapping on the VM's
loopback address, so --publish 127.0.0.1:8080:80 and --publish 8080:80 did the
same thing, and an IPv6 HOSTIP could not be written at all because the parser
split on ':'. A routed VM that replaces the host's own web server has to answer
on the host's own addresses and ports.
Grammar (src/network/types.rs). An IPv6 HOSTIP goes in brackets, [::]:80:80 or
[::1]:8080:80/tcp, and host_ip holds it in canonical text form without them
([0:0:0:0:0:0:0:1] is stored as ::1). An unbracketed HOSTIP is an IPv4
address: localhost:8080:80 used to parse, and then failed when the listener
was bound or was ignored by bridged networking. Every rejection names the spec
it rejected.
Routed (src/network/tcp_proxy.rs, routed.rs). A mapping with a HOSTIP listens
there. One without listens on the VM's loopback address, as before. The choice
is port_forward_bind_addr. An IPv6 listener clears IPV6_V6ONLY, so [::] accepts
IPv4 and IPv6 clients whatever net.ipv6.bindv6only says: tokio's bind sets
SO_REUSEADDR and nothing else, which leaves the option to that sysctl. Every
listener is bound before any relay task starts, so a bind failure returns an
error naming the address and port with the listeners bound before it already
closed. Before, a relay task was spawned per mapping as it went, and a failure
aborted them, which closes their listeners only when the runtime next runs the
aborted tasks.
State. The loopback address is allocated, and reported as
config.network.loopback_ip, only when a mapping listens on it
(RoutedNetwork::needs_loopback_ip). A routed VM whose mappings all name an
address has none, and fcvm ls shows "-" as its HOST_ADDR: the fallback there
was network.host_ip, which for a routed VM is the gateway inside its
namespace. config.port_mappings[].host_ip already says where a host-addressed
mapping listens, so nothing is added to the state.
Bridged. Its port forwarding is IPv4 iptables DNAT, and setup rescopes every
mapping to the veth address, so an IPv4 HOSTIP has always been ignored there.
An IPv6 HOSTIP is now refused by name in BridgedNetwork::setup, before
anything is created.
Rootless. pasta takes the address without brackets (-t ::1/8080:80), checked
against /usr/bin/pasta and both builds under /mnt/fcvm-btrfs/pasta: all bind
the address, and all make IPv6 listeners v6only, so [::] is IPv6-only in
rootless mode. On a host with no global IPv6 address fcvm starts pasta with
--ipv4-only, the guest has no IPv6 address, and pasta exits on an IPv6 forward
("IPv6 forward, but IPv6 not enabled"). Such a mapping is now refused by name
before pasta starts. The "adding port forward" log line prints host_ip and
host_port as separate fields.
Snapshots. A disk-only clone and a clone's reboot plan build their RunArgs from
the snapshot's metadata (run_args_from_snapshot_metadata), which wrote each
mapping back as HOSTPORT:GUESTPORT/PROTO. With routed networking listening
where a mapping says, a clone made that way would have listened on its own
loopback address and not on the snapshot's host address. Each mapping is now
written as the --publish spec that parses back to it, host address included
(PortMapping's Display).
Measured on this host with Python sockets: a dual-stack [::]:P listener and a
127.x.y.z:P listener exclude each other (EADDRINUSE either way), so a routed VM
on [::]:80 and VMs publishing port 80 on their loopback addresses cannot run at
the same time. The README says so.
Red. Each test failed with the behaviour it pins put back to what origin/main
does and the test kept, and passes on this commit:
types::tests::parse_accepts_every_form_of_the_grammar
[::]:80:80: invalid port mapping format: [::]:80:80
types::tests::parse_rejects_malformed_specs_and_names_them
[]:8080:80 must be rejected, parsed as PortMapping { host_ip: Some("[]"), .. }
localhost:8080:80 must be rejected, parsed as PortMapping { host_ip: Some("localhost"), .. }
tcp_proxy::tests::a_mapping_binds_its_own_host_address_or_the_vm_loopback
left: Ok("127.0.0.5:8080") right: Ok("127.0.0.1:8080")
tcp_proxy::tests::a_mapping_listens_on_its_host_address_and_not_on_the_vm_loopback
left: 127.129.164.130 right: 127.130.164.130
tcp_proxy::tests::a_listener_on_the_ipv6_wildcard_accepts_ipv4_and_ipv6_clients
left: 127.0.0.1 right: ::
tcp_proxy::tests::a_bind_failure_names_the_address_and_leaves_no_listener_behind
binding port forward on 127.131.164.184:37179: Address already in use (os error 98)
routed::tests::a_loopback_address_is_needed_only_by_a_mapping_without_a_host_address
assertion failed: !needs(&[Some("::")])
portmap::tests::an_ipv6_host_address_is_refused_by_name
called `Result::unwrap_err()` on an `Ok` value: ()
bridged::tests::an_ipv6_host_address_is_refused_before_any_setup
reserving per-VM network names
pasta::tests::an_ipv6_host_address_is_refused_when_pasta_runs_ipv4_only
called `Result::unwrap_err()` on an `Ok` value: ()
commands::ls::tests::host_addr_is_an_address_on_the_host
left: "10.0.2.2" right: "-"
commands::snapshot::tests::run_args_from_metadata_carries_boot_plan_fields
left: ["80:80/tcp", "5300:53/udp"] right: ["[::]:80:80/tcp", "127.0.0.1:5300:53/udp"]
network::types::tests::a_mapping_is_written_as_the_spec_that_parses_back_to_it
covers the writer, which is new here.
Two more reds, each with one line of this tree changed. With the parser storing
the bracket contents as written, parse_accepts_every_form_of_the_grammar:
[0:0:0:0:0:0:0:1]:8080:80 left: host_ip: Some("0:0:0:0:0:0:0:1") right: host_ip: Some("::1")
With the IPV6_V6ONLY line removed, the wildcard test run from the built test
binary under unshare -Urn with net.ipv6.bindv6only=1:
IPV6_V6ONLY must be cleared whatever net.ipv6.bindv6only says
and with the line in place it passes there and in the host namespace.
Red at the VM level, with every routed mapping bound on the VM's loopback
address again (one line of port_forward_bind_addr; as root, set up as in the
block below):
make _test-root FILTER="-E 'test(/test_port_forward_routed/)'"
Summary [ 90.312s] 1 test run: 0 passed, 1 failed, 1716 skipped
test_port_forward_routed
Routed port forwarding on the host address a mapping names should work
Green on this commit:
make test-unit
Summary [ 71.264s] 1359 tests run: 1359 passed (1 slow), 0 skipped
make lint rc 0
As root on the build host, at the top of this stack with the commit that reads
--ipv6-prefix from FCVM_IPV6_PREFIX picked on top (the host's own addresses are
not a routable /64, so the routed tests take its delegated subnet from that
variable):
make test-root FILTER="-E 'test(/test_port_forward_routed|test_clone_port_forward_routed|test_forward_localhost_routed/)'"
Summary [ 58.010s] 3 tests run: 3 passed, 1719 skipped
PASS [ 9.0s] fcvm::test_forward_localhost test_forward_localhost_routed
PASS [ 11.5s] fcvm::test_port_forward test_port_forward_routed
PASS [ 37.5s] fcvm::test_snapshot_clone test_clone_port_forward_routed
test_port_forward_routed publishes a second mapping on a 127/8 address of its
own and checks that it answers there and is refused on the VM's loopback
address.
… gone when cleanup returns
RoutedNetwork::cleanup aborted its TCP relay tasks and returned. A relay task
owns its listener, and abort only asks the runtime to drop the task, so the
listener could still be open after cleanup. With every listener on a per-VM
loopback address that did not matter. With a mapping on a host address
(--publish [::]:80:80) it does: on a snapshot miss an NV2 guest is torn down
and restored from its snapshot in one process, and the restore binds the same
address and port.
A relay also owns the connections it accepted. Each one ran as a detached task
that nothing stopped, so after a teardown in a process that keeps running, a
client stayed attached to a VM that was gone and the relay's upstream socket
kept the deleted network namespace alive.
A relay is now a Relay: its task, and a CancellationToken that stops it. The
accept loop keeps its connections in a JoinSet. On the signal it ends, closes
the listener and calls JoinSet::shutdown, which aborts the connections and
returns when their tasks have finished. tcp_proxy::stop_relays cancels every
token and then awaits every task, and cleanup calls it, so when cleanup
returns the listeners are closed and no connection task is left. It logs a
warning if a relay task ended in a panic. spawn_relay_loop,
start_port_forwards, start_localhost_forwards and start_proxy_relay return
Relay where they returned JoinHandle<()>, and RoutedNetwork holds Vec<Relay>.
Dropping a Relay without stop_relays aborts its task: the relay ends, and
nothing waits for it. Before this a dropped handle left its relay listening
for the life of the process.
Not covered: a dial of the guest that is still in progress when its relay is
stopped. connect_in_namespace runs it on the blocking pool, where it cannot be
cancelled. It ends by itself, and its socket closes then.
Red, with stop_relays cancelling the relays and not waiting for their tasks:
make _test-unit FILTER="-p fcvm --lib -E 'test(/^network::tcp_proxy::tests::/)'"
Summary [ 6.020s] 15 tests run: 13 passed, 2 failed, 709 skipped
tcp_proxy::tests::stopped_relays_have_closed_their_listeners
the address must be free again once the relays are stopped: Os { code: 98, kind: AddrInUse, message: "Address already in use" }
Red, with each connection a detached task again:
Summary [ 15.022s] 15 tests run: 13 passed, 2 failed, 709 skipped
tcp_proxy::tests::stopped_relays_have_closed_their_connections
client is still connected 5 s after the relay stopped
Red, with stop_relays as origin/main's cleanup had it, an abort of each task
and nothing more, and only the last two tests added (on the stack's top before
this change was folded in):
Summary [ 15.034s] 19 tests run: 17 passed, 2 failed, 715 skipped
tcp_proxy::tests::stopped_relays_have_finished_their_connection_tasks
stop_relays returned while a connection task was still running
tcp_proxy::tests::a_dropped_relay_releases_its_listener
a dropped relay still listens on 127.138.81.151:35249 after 5 s
Green on this commit:
the same command
Summary [ 0.511s] 15 tests run: 15 passed, 709 skipped
make test-unit
Summary [ 71.297s] 1363 tests run: 1363 passed (1 slow, 1 flaky), 0 skipped
flaky, passed on its retry: test_cargo_target_link target_pruner_invalidates_cargo_before_reclaiming_hardlinked_payloads (#985)
make lint rc 0
As root on the build host, at the top of this stack with the commit that reads
--ipv6-prefix from FCVM_IPV6_PREFIX picked on top (the host's own addresses are
not a routable /64, so the routed tests take its delegated subnet from that
variable):
make test-root FILTER="-E 'test(/test_port_forward_routed|test_clone_port_forward_routed|test_forward_localhost_routed/)'"
Summary [ 58.010s] 3 tests run: 3 passed, 1719 skipped
PASS [ 9.0s] fcvm::test_forward_localhost test_forward_localhost_routed
PASS [ 11.5s] fcvm::test_port_forward test_port_forward_routed
PASS [ 37.5s] fcvm::test_snapshot_clone test_clone_port_forward_routed
ejc3
force-pushed
the
routed-publish-host-address
branch
from
October 1, 2026 13:31
4472878 to
482c48c
Compare
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ejc3
commented
Oct 1, 2026
ejc3
left a comment
ejc3
left a comment
Owner
Author
There was a problem hiding this comment.
RED-VERIFIED: commands::snapshot::tests::run_args_from_metadata_carries_boot_plan_fields and network::tcp_proxy::tests::stopped_relays_have_finished_their_connection_tasks
These answer the two findings of the Codex review of 4472878: the host address dropped from rebuilt snapshot arguments, and stop_relays returning before its connection tasks had ended. Each has its own reply on its thread with the output of the test without the fix, 2 tests run: 1 passed, 1 failed for the first and 19 tests run: 17 passed, 2 failed for the second. Codex's review of the head, 482c48c, found no major issue.
ejc3
commented
Oct 1, 2026
ejc3
left a comment
ejc3
left a comment
Owner
Author
There was a problem hiding this comment.
DISAGREE: the retained concern in CodeRabbit's summary does not block this PR. It says a mapping on a reachable host address lets clients accumulate relay tasks and guest dials without a bound. The relay had no bound on connections before this PR either, as the summary says, and a port is published beyond the VM's own loopback address only when the operator names a host address. A limit on live connections and pending dials, with a defined behaviour at the limit, is a change of its own and is tracked in #1040.
NOT-A-DEFECT: the docstring coverage warning (78.33 percent against 80) is CodeRabbit's own pre-merge check, and it is not one of this repository's required checks.
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.
Stacked on: main. First of four: this, then
snapshot-run-publish,forward-localhost-any-host-loopback, andpublish-parse-before-cache.--publish [HOSTIP:]HOSTPORT:GUESTPORTcould name a host address, but routed networking ignored it and bound every mapping on the VM's loopback address, and an IPv6 address could not be written at all. A routed VM that stands in for the host's own web server has to answer on the host's own addresses and ports.What changes
[::]:80:80) and is stored in canonical form. An unbracketed HOSTIP must be an IPv4 address:localhost:8080:80used to parse and fail later, and is now rejected by the parser by name.[::]accepts IPv4 and IPv6 clients whatevernet.ipv6.bindv6onlysays. Every listener is bound before a relay starts, so a failed bind leaves none behind.fcvm lsshows-as HOST_ADDR for a routed VM that has no loopback address. It used to show the gateway inside the VM's namespace.Contract and impact
Production networking code: routed setup and teardown, the
--publishparser every mode uses, and the mappings a disk-only clone is given. A mapping without a HOSTIP behaves as before in every mode. What changes for a user: routed honours HOSTIP, a HOSTIP that is not an IP address is a parse error, and a clone of a routed VM published on a host address inherits that address, so it cannot start while the address is in use (the next PR addssnapshot run --publishto give it another).Minimum evidence
A unit test with a red for each behaviour, the unit suite and lint at both commits, and
test_port_forward_routedon a VM with a red.Evidence
Routed networking binds a published port on the host address its mapping names
Stop relays and wait for them, so their listeners and connections are gone when cleanup returns
Summary by CodeRabbit
New Features
Bug Fixes
Documentation