diff --git a/docs/bridge-networking.md b/docs/bridge-networking.md index 6bf3e6d5e..ec4e22949 100644 --- a/docs/bridge-networking.md +++ b/docs/bridge-networking.md @@ -199,6 +199,41 @@ The remaining bytes are derived from the VM ID hash. The prefix applies to all n - Docker's nftables chains (`DOCKER-FORWARD`) run before libvirt's but do not block virbr0 traffic - Use `setup-bridge.sh check --bridge ` to diagnose missing rules +### Host port mappings + +A VM's `port_map` has always been implemented as QEMU `hostfwd=` entries on a +user-mode netdev. A bridge NIC has no such netdev, so **the VMM does not forward +host ports for bridge VMs** — it is deliberately run without `CAP_NET_ADMIN`, +and a userspace proxy on the host would give back the per-packet cost that +moving off user mode was meant to escape. + +What the VMM does instead is carry the requirement to `netd`, which is +privileged and is the only component that sees every VMM instance on the host — +and therefore the only one that can arbitrate a host port between them. Whether +a given `netd` implements forwarding is its own business; the one in this +repository does not, and says so through `hello` rather than accepting ports it +will not forward. + +So on a node whose `netd` does not forward: + +- the VM starts normally and its bridge networking works +- its port mappings do not apply, and the VMM says so in its log at every launch +- `GetInfo` reports an empty `ingress` on the interface, against the non-empty + `ports` on the configuration — that difference is the signal + +Expose such a VM by reaching its address on the bridge directly, by putting an +L7 proxy in front of it, or by running a `netd` that forwards. `GetInfo` reports +the interface's `guest_ip` when `netd` is the authority for addresses on that +segment; a bridge NIC otherwise takes its address from a DHCP server the VMM +does not run and cannot ask. + +`macvtap` and `custom` NICs can never carry host port mappings — the first +bypasses the host bridge, and the second hands the whole netdev string to the +operator. The VMM warns rather than refusing to start, because a VM deployed +before any of this existed has been running with its ports dropped, and turning +that into a failed launch on upgrade would make an outage out of a +misconfiguration that was already there. + ### Mixing networking modes Bridge and user-mode VMs can coexist. Set the global default in `vmm.toml` and override per-VM as needed: diff --git a/dstack/vmm/rpc/proto/vmm_rpc.proto b/dstack/vmm/rpc/proto/vmm_rpc.proto index 31fef51f4..e6b7bbe6b 100644 --- a/dstack/vmm/rpc/proto/vmm_rpc.proto +++ b/dstack/vmm/rpc/proto/vmm_rpc.proto @@ -67,6 +67,26 @@ message NetworkInterfaceStatus { // configuration decides it, so it is reported here with the rest of the // resolved state rather than on the VM's own NetworkingConfig. optional string macvtap_mode = 8; + // Address this interface reaches its segment at, when the node's netd is the + // one that decides addresses. A bridge NIC gets its address from a DHCP + // server the VMM does not run, so absent means "not reported to us", never + // "has none". + optional string guest_ip = 9; + // Host ports actually forwarded to this interface. This is what the node + // did; VmConfiguration.ports is what was asked for. They differ whenever the + // node's netd allocates a port itself or declines one, and an empty list + // against a non-empty request means nothing was forwarded at all. + repeated IngressBinding ingress = 10; +} + +// One host port forwarded to a VM, as the node established it. +message IngressBinding { + // "tcp" or "udp". + string protocol = 1; + // Host address the port is accepted on. + string host_address = 2; + uint32 host_port = 3; + uint32 guest_port = 4; } // Structured log or lifecycle event emitted by the guest or runtime. diff --git a/dstack/vmm/src/app.rs b/dstack/vmm/src/app.rs index 151960229..29e4c55dc 100644 --- a/dstack/vmm/src/app.rs +++ b/dstack/vmm/src/app.rs @@ -558,6 +558,75 @@ impl App { Ok(()) } + /// The host ports this VM asks for, if the node can forward any. + /// + /// `port_map` has always been implemented as QEMU `hostfwd=` entries on a + /// user-mode netdev, so a VM that moves to a bridge silently loses every + /// one of them: nothing warns, and `GetInfo` keeps reporting the ports as + /// though they worked. Handing them to netd is what can make them real, + /// and saying so when it cannot is what makes the loss visible. + /// + /// Returns an empty list rather than failing when the node has no forwarder. + /// A VM deployed before this existed has been running with its ports + /// dropped, and refusing to start it now would turn a silent + /// misconfiguration into an outage on upgrade. + async fn ingress_for( + &self, + vm: &VmConfig, + networks: &[Networking], + ) -> Vec { + if vm.manifest.port_map.is_empty() { + return Vec::new(); + } + // QEMU's own `hostfwd=` entries ride on a user-mode netdev, so a VM + // that still has one needs nothing from netd. This mirrors how the + // launch picks the interface to put them on. + if networks + .iter() + .any(|network| network.nic.mode == NetworkingMode::User) + { + return Vec::new(); + } + let ports = vm.manifest.port_map.len(); + if !networks + .iter() + .any(|network| network.nic.mode == NetworkingMode::Bridge) + { + // macvtap bypasses the host bridge and custom mode owns its own + // netdev string, so neither has anywhere for the host to forward to. + warn!( + vm_id = %vm.manifest.id, + ports, + "this VM's networking mode cannot carry host port mappings, so \ + its {ports} port mapping(s) do not apply" + ); + return Vec::new(); + } + let forwards = netd::capabilities(&self.config.netd.socket) + .await + .map(|capabilities| capabilities.has("ingress")) + .unwrap_or(false); + if !forwards { + warn!( + vm_id = %vm.manifest.id, + ports, + "no netd on this node forwards host ports, so this VM's {ports} \ + port mapping(s) do not apply to its bridge interface" + ); + return Vec::new(); + } + vm.manifest + .port_map + .iter() + .map(|mapping| netd::IngressRequest { + protocol: mapping.protocol.as_str().to_string(), + host_address: mapping.address.to_string(), + host_port: mapping.from, + guest_port: mapping.to, + }) + .collect() + } + async fn prepare_filtered_networks( &self, vm: &VmConfig, @@ -577,6 +646,7 @@ impl App { .work_dir(&vm.manifest.id) .map(|dir| dir.path().display().to_string()) .unwrap_or_default(); + let ingress = self.ingress_for(vm, networks).await; let mut prepared = Vec::new(); for (nic_index, network) in networks.iter_mut().enumerate() { if !needs_netd_interface(network, &self.config.cvm) { @@ -607,6 +677,15 @@ impl App { filtered, queues, workdir: workdir.clone(), + // Only the first NIC carries them, matching where user-mode + // networking puts its `hostfwd=` entries. A second NIC that + // repeated the list would ask two interfaces to answer on + // one host port. + ingress: if nic_index == 0 { + ingress.clone() + } else { + Vec::new() + }, }), NetworkingMode::Macvtap => NetdRequest::PrepareMacvtap(PrepareMacvtapRequest { identity: identity.clone(), @@ -691,6 +770,18 @@ impl App { ) ); } + // Ports that were asked for and not answered for are the + // failure this whole path exists to end. A netd that forwards + // says what it built; silence is not consent. + let requested = if nic_index == 0 { ingress.len() } else { 0 }; + if requested > 0 { + let established = response + .ingress + .clone() + .context("netd accepted host ports without saying what it forwarded")?; + network.ingress = established; + } + network.guest_ip = response.guest_ip.clone().unwrap_or_default(); Ok(()) })(); if let Err(error) = accepted { diff --git a/dstack/vmm/src/app/vm_info.rs b/dstack/vmm/src/app/vm_info.rs index 19d72118a..15025c17c 100644 --- a/dstack/vmm/src/app/vm_info.rs +++ b/dstack/vmm/src/app/vm_info.rs @@ -87,6 +87,21 @@ fn interfaces_to_proto( // state. The VM's own record cannot carry one. macvtap_mode: (networking.nic.mode == NetworkingMode::Macvtap) .then(|| networking.macvtap_mode.clone()), + // Both are what the node reported at launch, not what the VM + // asked for. `port_map` is the request and stays on the + // configuration; an empty list here against a non-empty request + // is the node saying it forwarded nothing. + guest_ip: (!networking.guest_ip.is_empty()).then(|| networking.guest_ip.clone()), + ingress: networking + .ingress + .iter() + .map(|binding| pb::IngressBinding { + protocol: binding.protocol.clone(), + host_address: binding.host_address.clone(), + host_port: binding.host_port as u32, + guest_port: binding.guest_port as u32, + }) + .collect(), } }) .collect() diff --git a/dstack/vmm/src/config.rs b/dstack/vmm/src/config.rs index d7479abc4..86af43171 100644 --- a/dstack/vmm/src/config.rs +++ b/dstack/vmm/src/config.rs @@ -10,6 +10,7 @@ use path_absolutize::Absolutize; use rocket::figment::Figment; use serde::{Deserialize, Serialize}; +use crate::netd::IngressBinding; use dstack_types::TdxAttestationVariant; use lspci::{lspci_filtered, Device}; use tracing::{info, warn}; @@ -1072,6 +1073,27 @@ pub struct Networking { /// the VM runs, and what has to be removed is what was created. #[serde(default, skip_serializing_if = "NetdInterface::is_none")] pub netd_interface: NetdInterface, + /// The address netd says this NIC reaches its segment at. + /// + /// Runtime state, cleared when the VM stops. A bridge NIC's address comes + /// from a DHCP server the VMM does not run, so without netd reporting it the + /// VMM can only say which bridge a VM is on, not where it is. Empty + /// whenever netd does not assign addresses. + /// + /// Unlike `device` this is persisted, so a VMM restart does not lose the + /// address of a VM that is still running. A stale `/dev/tapN` can name a + /// different device after a restart, which is why that one is not; a stale + /// address is only ever reported, never acted on. + #[serde(default, skip_serializing_if = "String::is_empty")] + pub guest_ip: String, + /// The host ports netd forwards to this NIC, as netd established them. + /// + /// Runtime state, persisted for the same reason as `guest_ip`. What was + /// asked for lives in the manifest's `port_map`; this is what the node + /// actually did, which is not the same thing when netd allocates a port + /// from a range or declines one. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub ingress: Vec, } /// The host interface netd created for a NIC, if any. diff --git a/dstack/vmm/src/netd.rs b/dstack/vmm/src/netd.rs index fc65b5a97..3ee3f17e0 100644 --- a/dstack/vmm/src/netd.rs +++ b/dstack/vmm/src/netd.rs @@ -92,6 +92,15 @@ pub struct PrepareBridgeRequest { /// VM that asked for it without going through the VMM. #[serde(default)] pub workdir: String, + /// Host ports this VM wants reachable at its guest. + /// + /// Empty asks for nothing, which is what a caller that predates the field + /// sends. A netd that does not implement forwarding refuses a non-empty + /// list rather than building the interface without it: the alternative is + /// the failure this field exists to end, where ports are accepted, reported + /// back, and silently never forwarded. + #[serde(default)] + pub ingress: Vec, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -113,6 +122,47 @@ pub struct PrepareMacvtapRequest { pub workdir: String, } +/// One host port a VM wants reachable at its guest. +/// +/// The caller names every field. That is the same treatment `bridge`, `mac` and +/// `queues` get, and the opposite of `filtered`: an nwfilter name cannot be +/// checked for whether it filters anything, so naming one is excluded, while a +/// host port is a closed space netd can check a request against. Naming is not +/// deciding -- which ports may be handed out, and to whom, stays netd's own +/// configuration, exactly as `allowed_bridges` governs the bridge a caller +/// names. +/// +/// The VMM does not implement forwarding for these. It carries the requirement +/// to whoever configures the host, because the VMM runs without +/// `CAP_NET_ADMIN` by design, and because netd is the only component that sees +/// every VMM instance on the host and can therefore arbitrate a host port +/// between them. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct IngressRequest { + /// `"tcp"` or `"udp"`. + pub protocol: String, + /// Host address to accept on. Empty leaves the choice to netd. + /// + /// Not merely cosmetic: an admin or metrics port bound to loopback and one + /// published to the world differ only here, and a forwarder that dropped + /// the distinction would publish the first. + #[serde(default)] + pub host_address: String, + /// Host port. Zero asks netd to choose from whatever range it allocates. + pub host_port: u16, + pub guest_port: u16, +} + +/// One forwarding rule netd established, echoed so the caller can report what +/// the VM actually got rather than what it asked for. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct IngressBinding { + pub protocol: String, + pub host_address: String, + pub host_port: u16, + pub guest_port: u16, +} + /// A netd-managed interface as `List` found it on the host. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct ManagedInterface { @@ -196,6 +246,20 @@ struct Response { /// none; absent means it did not understand the question. #[serde(default, skip_serializing_if = "Option::is_none")] interfaces: Option>, + /// The address this NIC will reach the segment at, when netd is the one + /// that decides it. + /// + /// A DNAT rule needs an address at install time and a DHCP lease does not + /// exist until the guest has booted, so a netd that forwards ports is + /// necessarily also the authority on the address. Reporting it back is what + /// lets the VMM show a bridge VM's address without a lease callback. + #[serde(default, skip_serializing_if = "Option::is_none")] + guest_ip: Option, + /// Forwarding rules netd established. Absent from a netd that does not + /// implement forwarding, which is how the caller tells "nothing was asked + /// for" apart from "this was ignored". + #[serde(default, skip_serializing_if = "Option::is_none")] + ingress: Option>, #[serde(default, skip_serializing_if = "Option::is_none")] error: Option, } @@ -212,6 +276,8 @@ impl Response { version: None, features: None, interfaces: None, + guest_ip: None, + ingress: None, error: None, } } @@ -223,6 +289,8 @@ struct Prepared { tap: String, device: Option, queues: Option, + guest_ip: Option, + ingress: Option>, } impl Prepared { @@ -231,6 +299,8 @@ impl Prepared { tap, device: None, queues: None, + guest_ip: None, + ingress: None, } } } @@ -301,6 +371,10 @@ pub fn instance_id(configured: &str, run_path: &Path) -> String { pub struct PreparedInterface { pub device: Option, pub queues: Option, + /// The address netd says this NIC will use, when netd decides addresses. + pub guest_ip: Option, + /// The forwarding rules netd established, if it establishes any. + pub ingress: Option>, } /// Marker carried in the error chain when the VMM could not reach netd at all. @@ -330,6 +404,8 @@ pub async fn request(socket: &Path, request: &Request) -> Result Resul tap: Some(prepared.tap), device: prepared.device, queues: prepared.queues, + guest_ip: prepared.guest_ip, + ingress: prepared.ingress, ..Response::empty() }, Ok(Outcome::Capabilities) => Response { @@ -731,6 +809,8 @@ fn prepare_macvtap( tap, device: Some(device), queues: Some(queues), + guest_ip: None, + ingress: None, }) } Err(error) => { @@ -813,6 +893,10 @@ fn prepare_bridge( tap, device: None, queues: Some(queues), + // This netd assigns no addresses and forwards no ports, so it has + // nothing to report beyond the interface itself. + guest_ip: None, + ingress: None, }) } @@ -947,6 +1031,16 @@ fn validate_prepare_bridge( if filter.requires_binding() && !request.filtered { bail!("this netd requires an nwfilter binding on every bridge TAP"); } + // This netd creates interfaces; it is not the host's forwarder. Building + // the TAP anyway and leaving the ports unforwarded is the silent failure + // the field exists to end, so the request is refused whole. `hello` does + // not name `ingress`, so a caller learns this before it ever asks. + if !request.ingress.is_empty() { + bail!( + "this netd does not forward host ports, but {} was/were requested", + request.ingress.len() + ); + } if !Path::new("/sys/class/net") .join(&request.bridge) .join("bridge") @@ -1142,6 +1236,7 @@ mod tests { filtered: true, queues: 0, workdir: String::new(), + ingress: Vec::new(), }; let filter = NetworkFilterConfig { mode: crate::config::NetworkFilterMode::Libvirt, @@ -1189,6 +1284,7 @@ mod tests { filtered: true, queues: 0, workdir: String::new(), + ingress: Vec::new(), }); let value = serde_json::to_value(request).unwrap(); assert_eq!(value["operation"], "prepare_bridge"); @@ -1259,6 +1355,7 @@ mod tests { filtered: false, queues: 4, workdir: String::new(), + ingress: Vec::new(), }); let value = serde_json::to_value(request).unwrap(); assert_eq!(value["queues"], 4); @@ -1310,6 +1407,7 @@ mod tests { filtered: true, queues: 0, workdir: String::new(), + ingress: Vec::new(), }); let value = serde_json::to_value(request).unwrap(); assert_eq!(value["operation"], "prepare_bridge"); @@ -1387,6 +1485,7 @@ mod tests { filtered: false, queues: 4, workdir: String::new(), + ingress: Vec::new(), }; let filtering = NetworkFilterConfig { mode: crate::config::NetworkFilterMode::Libvirt, @@ -1460,6 +1559,7 @@ mod tests { filtered: true, queues: 1, workdir: String::new(), + ingress: Vec::new(), }; // Nothing on the wire can name a filter: the field does not exist. let wire = serde_json::to_value(Request::PrepareBridge(request.clone())).unwrap(); @@ -1571,6 +1671,7 @@ mod tests { filtered: true, queues: 1, workdir: "/opt/dstack/run/vm/vm".into(), + ingress: Vec::new(), }); let value = serde_json::to_value(request).unwrap(); assert_eq!(value["workdir"], "/opt/dstack/run/vm/vm"); @@ -1594,4 +1695,94 @@ mod tests { }; assert_eq!(decoded.workdir, ""); } + + #[test] + fn ports_travel_with_the_bridge_prepare_and_default_to_none() { + let request = Request::PrepareBridge(PrepareBridgeRequest { + identity: identity("instance", "vm", 0), + bridge: "br0".into(), + mac: "02:00:00:00:00:01".into(), + qemu_uid: 1000, + filtered: true, + queues: 1, + workdir: String::new(), + ingress: vec![IngressRequest { + protocol: "udp".into(), + host_address: "0.0.0.0".into(), + host_port: 7483, + guest_port: 51820, + }], + }); + let value = serde_json::to_value(request).unwrap(); + assert_eq!(value["ingress"][0]["protocol"], "udp"); + assert_eq!(value["ingress"][0]["host_port"], 7483); + assert_eq!(value["ingress"][0]["guest_port"], 51820); + // The bind address separates an admin port from a published one, so a + // forwarder that lost it would publish the admin port. + assert_eq!(value["ingress"][0]["host_address"], "0.0.0.0"); + + // A caller that predates the field asks for nothing. + let decoded = serde_json::from_value::(serde_json::json!({ + "operation": "prepare_bridge", + "instance_id": "instance", + "vm_id": "vm", + "nic_index": 0, + "bridge": "br0", + "mac": "02:00:00:00:00:01", + "qemu_uid": 1000, + "filtered": true, + "queues": 1, + })) + .unwrap(); + let Request::PrepareBridge(decoded) = decoded else { + panic!("expected a bridge prepare"); + }; + assert!(decoded.ingress.is_empty()); + } + + #[test] + fn a_netd_that_does_not_forward_refuses_the_ports_instead_of_dropping_them() { + let mut request = PrepareBridgeRequest { + identity: identity("instance", "vm", 0), + bridge: "dt-absent-br".into(), + mac: "02:00:00:00:00:01".into(), + qemu_uid: 1000, + filtered: true, + queues: 1, + workdir: String::new(), + ingress: vec![IngressRequest { + protocol: "tcp".into(), + host_address: "0.0.0.0".into(), + host_port: 8443, + guest_port: 443, + }], + }; + let filter = NetworkFilterConfig { + mode: crate::config::NetworkFilterMode::Libvirt, + filter: "clean-traffic".into(), + parameters: Default::default(), + }; + // Building the TAP and dropping the ports is the failure being ended, + // so the whole request is refused -- and refused for the request's own + // sake, before the host is inspected for a bridge that may not exist. + let error = validate_prepare_bridge(&request, &filter).unwrap_err(); + assert!( + error.to_string().contains("does not forward host ports"), + "{error}" + ); + + // Asking for nothing gets past this check and on to the host, which is + // where a bridge that does not exist is noticed. + request.ingress.clear(); + request.bridge = "dt-absent-br".into(); + let error = validate_prepare_bridge(&request, &filter).unwrap_err(); + assert!(error.to_string().contains("not a host bridge"), "{error}"); + } + + #[test] + fn a_forwarding_netd_is_not_this_one() { + // `hello` is what tells a caller this before it asks, so the refusal + // above is never the first thing it learns. + assert!(!FEATURES.contains(&"ingress")); + } }