Repository navigation
Put --expose port forwards on the vmnet network configuration - #223
Conversation
| futures-util = { version = "0.3", default-features = false, features = ["std", "sink"] } | ||
| dhcproto = { git = "https://github.com/bluecatengineering/dhcproto.git", branch = "master" } | ||
| vmnet = "0.7.0" | ||
| vmnet = "0.9.0" |
There was a problem hiding this comment.
vmnet 0.7 → 0.9. No other softnet changes were needed for the API changes in 0.8 and 0.9 (serial event queue, Send callbacks, finalize behaviour, Send packet-size iterator).
| debug = true | ||
|
|
||
| [patch.crates-io] | ||
| vmnet = { git = "https://github.com/simonisjp/vmnet", branch = "network-configuration-port-forwarding" } |
There was a problem hiding this comment.
Temporary, so this branch builds: it points at the branch of the vmnet PR that adds NetworkConfiguration::add_port_forwarding_rule.
Before merge it should be replaced by the vmnet release that contains it, with a regenerated Cargo.lock.
There was a problem hiding this comment.
Version 0.10.0 with your NetworkConfiguration::add_port_forwarding_rule addition should be available now.
There was a problem hiding this comment.
Thanks! Switched to vmnet 0.10.0 from crates.io and dropped the [patch.crates-io] entry. I also rebased onto #221 and moved the forwards into Network::new.
| vm_net_type: NetType, | ||
| vm_mac: EthernetAddress, | ||
| enable_isolation: bool, | ||
| exposed_ports: &[ExposedPort], |
There was a problem hiding this comment.
Host::new now receives the exposed ports. They have to go on the configuration before the network exists, and this is where the configuration is built.
| .add_dhcp_reservation(vm_mac.0, vm_ip) | ||
| .context("failed to add DHCP address reservation")?; | ||
|
|
||
| // Forward the exposed ports to the VM's reserved address |
There was a problem hiding this comment.
The forwards are added to the NetworkConfiguration after the DHCP reservation, which fixes vm_ip, and before Network::new. On these networks that's the only point where vmnet accepts them.
The details strings are built once, so the error and the log keep the same text as before.
| ) | ||
| .context("failed to initialize vmnet interface")?; | ||
|
|
||
| for details in &port_forwardings { |
There was a problem hiding this comment.
Logged once the interface is up, so the line only appears when the forward is actually in place.
| } | ||
|
|
||
| impl Host { | ||
| pub fn port_forwarding_add_rule( |
There was a problem hiding this comment.
Removed. This was the interface-level call (vmnet_interface_add_ip_port_forwarding_rule) that vmnet rejects on an interface attached to a network, and nothing else used it.
| vm_net_type, | ||
| vm_mac_address, | ||
| !allow.contains(&Rule::Stateless(Target::Prefix(Ipv4Net::default()))), | ||
| &exposed_ports, |
There was a problem hiding this comment.
Proxy::new now hands the ports to Host::new instead of adding them after the host is created (the loop removed below).
| host.gateway_ip, | ||
| ); | ||
|
|
||
| // Install port forwardings for the reserved VM's IP address |
There was a problem hiding this comment.
The failing path: this loop ran after the interface was already attached to its network, so every --expose failed here.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f97352b to
296545f
Compare
| futures-util = { version = "0.3", default-features = false, features = ["std"] } | ||
| dhcproto = { git = "https://github.com/bluecatengineering/dhcproto.git", branch = "master" } | ||
| vmnet = "0.9.0" | ||
| vmnet = "0.10.0" |
There was a problem hiding this comment.
vmnet 0.9 → 0.10, which adds NetworkConfiguration::add_port_forwarding_rule. No other changes were needed for 0.10.
| net_type: NetType, | ||
| subnet: Ipv4Net, | ||
| reservations: &[(MacAddress, Ipv4Addr)], | ||
| port_forwardings: &[(Ipv4Addr, ExposedPort)], |
There was a problem hiding this comment.
One entry per forward, each paired with its VM's reserved address and flattened across the batch, so Network::new doesn't need to know about VMs.
| })?; | ||
| } | ||
|
|
||
| // Forward exposed ports to the reserved VM addresses, which vmnet only |
There was a problem hiding this comment.
This has to happen before vmnet::Network::new. Once an interface is attached to the network, the interface-level vmnet_interface_add_ip_port_forwarding_rule returns Failure, which is the bug in #222.
| let handle = vmnet::Network::new(&configuration) | ||
| .with_context(|| format!("failed to create vmnet network {subnet}"))?; | ||
|
|
||
| for details in port_forwardings { |
There was a problem hiding this comment.
Logged only after the network exists, so the line only appears when the forward is actually in place.
| .map(|(_, config, vm_ip, _)| (config.vm_mac_address, *vm_ip)) | ||
| .collect(); | ||
| let network = Network::new(vm_net_type, subnet, &reservations)?; | ||
| let port_forwardings: Vec<_> = configured |
There was a problem hiding this comment.
Built from the same configured list as reservations just above, so each forward points at the address its VM is reserved.
|
|
||
| #[tokio::test] | ||
| #[serial_test::serial] | ||
| async fn native_batch_exposes_ports_for_every_vm() { |
There was a problem hiding this comment.
Regression test for #222. On main it fails with failed to add port forwarding rule external_port=52222, …: vmnet returned an error status: Failure; with this PR it passes.
| host.gateway_ip, | ||
| ); | ||
|
|
||
| // Install port forwardings for the VM's reserved IP address |
There was a problem hiding this comment.
The failing path: this loop ran after Host::new had attached the interface to the network, where vmnet rejects interface-level forwarding.
| } | ||
|
|
||
| impl Host { | ||
| pub fn port_forwarding_add_rule( |
There was a problem hiding this comment.
Removed. This was the interface-level call (vmnet_interface_add_ip_port_forwarding_rule), and nothing else uses it.
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: 296545fe63
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .iter() | ||
| .flat_map(|(_, config, vm_ip, _)| { | ||
| config.exposed_ports.iter().map(|port| (*vm_ip, *port)) | ||
| }) |
There was a problem hiding this comment.
When two entries in one softnet.initialize batch request the same external TCP port, this flat_map passes both mappings to vmnet. vmnet accepts duplicate rules in one network configuration, so initialization reports success even though a single external port cannot select two target addresses; at least one VM's exposure is ineffective and traffic may reach the other VM instead. Validate uniqueness of external_port across the complete batch before creating native resources.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
There was no explicit duplicate-port check before.
Let's just keep relying on vmnet framework's behavior here where a first matching rule wins.
TL;DR
Fixes #222: since 0.24.0, any
--exposemakes softnet exit at startup withvmnet returned an error status: Failure, which breakstart run --net-softnet --net-softnet-expose. softnet now adds every VM's forwards to the shared network's configuration inNetwork::new, before the network is created. It usesNetworkConfiguration::add_port_forwarding_rulefrom vmnet 0.10.0 (edigaryev/vmnet#12).Details
What broke. #209 moved VMs onto networks created with
Network::new+Interface::with_network, and #221 kept that with one shared network for up to five VMs.Proxy::newstill added each--exposeforward after the interface was up, withHost::port_forwarding_add_rule. That calls the interface-levelvmnet_interface_add_ip_port_forwarding_rule, which vmnet rejects on an interface attached to a network, so initialization failed.The change.
Network::newtakesport_forwardings: &[(Ipv4Addr, ExposedPort)]and adds them to the configuration after the DHCP reservations, beforevmnet::Network::new. That's the only point where vmnet accepts them for these networks.Manager::initializecollects every VM's exposed ports with its reserved address, next to the reservations, and passes them toNetwork::new.Proxy::newno longer takesexposed_ports, andHost::port_forwarding_add_ruleis removed, since nothing else uses it.Cargo.tomlbumps vmnet from 0.9.0 to 0.10.0. No other changes were needed.manager::tests::native_batch_exposes_ports_for_every_vminitializes a batch of two VMs with three forwards. Onmainit fails with the error from--exposefails on 0.24: vmnet rejects port forwarding on per-VM networks #222; with this PR it passes.Behaviour notes.
failed to add port forwarding rule external_port=…, internal_addr=…, internal_port=…, with the vmnet error as its cause, and initialization still fails as a whole. The outerfailed to configure port forwardingcontext is gone.added port forwarding rule …is logged once the network is created.tartpath), stopping the VM removes its forwards. I haven't checked when they go away for one VM of a multi-VM batch.Verification
Tested on an Apple silicon Mac mini running macOS 27.0.1, with tart 2.41.1 and the public image
ghcr.io/cirruslabs/macos-sonoma-vanilla:latest. Builds were made frommain(f834910) with and without this PR. I probed the forwarded ports from a different network.mainvmnet returned an error status: Failure--expose 2222:22and2223:22SSH-2.0-OpenSSH_9.9); guests on 10.x and 192.168.x subnetsBefore #221 landed, the same approach also passed five start/stop cycles with guests on 10.x, 172.16.x and 192.168.x subnets, plus a check that guests can still reach the internet.
cargo fmt --checkandcargo clippy --all-targets --all-features -- -D warningsare clean.cargo testpasses 85 tests. Twocontrol_transporttests (excessive_attachments_close_every_descriptor_without_kernel_truncation,validates_owned_descriptor_and_signals_eof_on_drop) fail on this host on unmodifiedmaintoo.--control-fdend to end (only the new test covers it), UDP and IPv6 forwards (which--exposedoesn't offer), and hosts with the stock pf configuration. The test host uses a custom pf ruleset.Questions are welcome here.
🤖 Generated with Claude Code