feat: enhance network management and lifecycle handling with improved egress setup and validation

tests/integration/test_private_ports.py

@@ -2,16 +2,18 @@
 
 Run: UVM_FIREWALL_INTEGRATION=1 python3 -B -m unittest discover -s tests/integration -v
 
-Needs Linux, nft, ip, bridge, conntrack, sysctl and permission to create network
+Needs Linux, nft, iptables, ip, bridge, conntrack, sysctl and permission to create network
 namespaces (root, or unprivileged user namespaces). No sudo, installation,
 /run/netns mounts, host firewall changes or br_netfilter sysctls are used.
-Bridge-connected veths stand in for guest TAPs. A third, routed namespace tests
+Bridge-connected veths stand in for guest TAPs. A third namespace with two
-guest egress without NAT. Ordinary test runs skip without executing commands.
+uplinks tests NAT, offline startup, reconnects and host route changes without
+using the real internet. Ordinary test runs skip without executing commands.
 """
 
 from __future__ import annotations
 
 import contextlib
+import errno
 import json
 import os
 import re
@@ -62,6 +64,13 @@ class SocketEndpoint:
             except TimeoutError:
                 sock.close()
                 return False
+            except OSError as error:
+                sock.close()
+                if operation == "connect" and error.errno in {
+                    errno.ENETUNREACH, errno.EHOSTUNREACH, errno.ENETDOWN, errno.ECONNREFUSED,
+                }:
+                    return False
+                raise
             except BaseException:
                 sock.close()
                 raise
@@ -206,6 +215,7 @@ def _sandbox(parent_fd):
     from uvm.domain import PortRule, VmRecord
     from uvm.errors import UvmError
     from uvm.firewall import FirewallManager
+    from uvm.network import NetworkManager
     from uvm.system import CommandRunner
 
     runner = CommandRunner(emit=None)
@@ -220,11 +230,12 @@ def _sandbox(parent_fd):
     manager = FirewallManager(settings, runner)
     manager.check_available()
     run("ip", "link", "set", "lo", "up")
-    run("ip", "link", "add", settings.bridge, "type", "bridge", "stp_state", "0")
+    # All changes are inside the freshly verified namespace. A pre-existing
-    run("ip", "addr", "add", f"{settings.gateway}/24", "dev", settings.bridge)
+    # terminal drop catches regressions that append ineffective forward rules.
-    run("ip", "link", "set", settings.bridge, "up")
+    run("iptables", "-P", "FORWARD", "DROP")
-    # This IPv4 sysctl belongs only to the freshly verified network namespace.
+    run("iptables", "-A", "FORWARD", "-j", "DROP")
-    run("sysctl", "-w", "net.ipv4.ip_forward=1")
+    assert not run("ip", "route", "show", "default").strip()
+    NetworkManager(settings, runner).ensure_bridge()
 
     metadata = dict(cpu=1, ram_mib=128, gateway=str(settings.gateway),
                     socket="", config="", log="", network=str(settings.network), bridge=settings.bridge)
@@ -263,6 +274,16 @@ def _sandbox(parent_fd):
                 run("ip", "link", "set", tap, "type", "bridge_slave", "group_fwd_mask", "0x4000")
                 endpoint.request("run", command=["ip", "route", "add", "default", "via", str(settings.gateway)])
 
+        run("ip", "link", "add", "fw-up2", "type", "veth", "peer", "name", "fw-end2")
+        run("ip", "link", "set", "fw-end2", "netns", str(outside.process.pid))
+        run("ip", "addr", "add", "172.30.74.1/24", "dev", "fw-up2")
+        run("ip", "link", "set", "fw-up2", "up")
+        outside.request("run", command=["ip", "link", "set", "fw-end2", "name", "eth1"])
+        outside.request("run", command=["ip", "addr", "add", "172.30.74.2/24", "dev", "eth1"])
+        outside.request("run", command=["ip", "link", "set", "eth1", "up"])
+        internet_ip = "203.0.113.73"
+        outside.request("run", command=["ip", "addr", "add", f"{internet_ip}/32", "dev", "lo"])
+
         manager.apply(vm)
         manager.apply(peer_vm)
         for tap in (vm.tap, peer_vm.tap):
@@ -328,6 +349,7 @@ def _sandbox(parent_fd):
         guest.request("udp_send", name="dns", ip="172.30.73.2", port=53, data="dns-query")
         query = outside.request("udp_recv", name="dns")
         assert query[0] == "dns-query"
+        assert query[1][0] == "172.30.73.1", "guest egress must use the host uplink's address"
         outside.request("udp_send", name="dns", ip=query[1][0], port=query[1][1], data="dns-reply")
         assert guest.request("udp_recv", name="dns")[0] == "dns-reply"
         guest.request("udp_bind", name="unsolicited", ip=vm.guest_ip, port=40054)
@@ -338,6 +360,44 @@ def _sandbox(parent_fd):
             assert endpoint.request("ping", ip=target), (endpoint, target)
         print("PASS: independent peer policy, outbound UDP/DNS and ICMP; unsolicited UDP blocked", flush=True)
 
+        # No UVM reconfiguration after this point: the kernel must follow the
+        # host routes for both TCP and UDP/53, including after offline creation.
+        outside.request("listen", name="internet", ip=internet_ip, port=9443)
+        outside.request("udp_bind", name="internet-dns", ip=internet_ip, port=53)
+
+        def offline(label):
+            assert not host.request("connect", name=f"host-{label}", ip=internet_ip, port=9443)
+            assert not guest.request("connect", name=f"guest-{label}", ip=internet_ip, port=9443)
+            assert guest.request("ping", ip=str(settings.gateway)), "local networking must still work offline"
+
+        def online(label, source):
+            for endpoint, role in ((host, "host"), (guest, "guest")):
+                name = f"{role}-{label}"
+                assert endpoint.request("connect", name=name, ip=internet_ip, port=9443)
+                assert outside.request("accept", listener="internet", name=name)
+                transfer(endpoint, outside, name, "request")
+                transfer(outside, endpoint, name, "response")
+            guest.request("udp_bind", name=label, ip=vm.guest_ip, port=0)
+            guest.request("udp_send", name=label, ip=internet_ip, port=53, data=label)
+            query = outside.request("udp_recv", name="internet-dns")
+            assert query == [label, [source, query[1][1]]], query
+            outside.request("udp_send", name="internet-dns", ip=source, port=query[1][1], data="dns-reply")
+            assert guest.request("udp_recv", name=label)[0] == "dns-reply"
+
+        offline("initially-offline")
+        run("ip", "route", "add", "default", "via", "172.30.73.2", "dev", "fw-up")
+        online("first-uplink", "172.30.73.1")
+        run("ip", "route", "replace", "default", "via", "172.30.74.2", "dev", "fw-up2")
+        online("changed-uplink", "172.30.74.1")
+        run("ip", "route", "del", "default")
+        run("ip", "link", "set", "fw-up", "down")
+        run("ip", "link", "set", "fw-up2", "down")
+        offline("disconnected")
+        run("ip", "link", "set", "fw-up", "up")
+        run("ip", "route", "add", "default", "via", "172.30.73.2", "dev", "fw-up")
+        online("reconnected", "172.30.73.1")
+        print("PASS: host/guest NAT egress follows offline/online, uplink changes and reconnects", flush=True)
+
         # Spoofed source MACs must not reach FDB learning, including the local
         # link-local path which can skip bridge prerouting entirely.
         spoof = bytes.fromhex("02aabbccddee")
@@ -393,7 +453,7 @@ class PrivatePortNamespaceTests(unittest.TestCase):
         probe = subprocess.run((*prefix, "true"), text=True, capture_output=True, timeout=5)
         if probe.returncode:
             self.skipTest(f"isolated namespace permission unavailable: {probe.stderr.strip()}")
-        missing = [name for name in ("ip", "bridge", "nft", "conntrack", "sysctl") if shutil.which(name) is None]
+        missing = [name for name in ("ip", "bridge", "nft", "iptables", "conntrack", "sysctl") if shutil.which(name) is None]
         if missing:
             self.skipTest(f"integration tools unavailable: {', '.join(missing)}")
         with open("/proc/self/ns/net", "rb") as parent:

tests/test_lifecycle_commands.py

@@ -385,7 +385,7 @@ class LifecyclePortTests(LifecycleTestCase):
         self.assertEqual(self.settings.state_path.read_bytes(), before)
         self.assertEqual(self.host.events, [])
 
-    def test_start_of_running_vm_repairs_policy_without_reboot_or_reprovision(self) -> None:
+    def test_start_of_running_vm_repairs_policy_and_egress_without_reboot_or_reprovision(self) -> None:
         vm = self.create_vm()
         del self.network.firewall.tables[vm.id]
         self.host.events.clear()
@@ -395,9 +395,34 @@ class LifecyclePortTests(LifecycleTestCase):
         self.assertNotIn("process.start", self.host.actions(vm.id))
         self.assertNotIn("process.terminate", self.host.actions(vm.id))
         self.assertNotIn("client.configure", self.host.actions(vm.id))
+        self.assertIn(("network.ensure_bridge", None), self.host.events)
+        self.assertLess(self.host.events.index(("firewall.apply", vm.id)),
+                        self.host.events.index(("network.ensure_bridge", None)))
         self.assertEqual(self.provision.call_count, 1)
         self.assertEqual(self.copy_disk.call_count, 1)
 
+    def test_live_start_rejects_changed_network_settings_before_repairing_network(self) -> None:
+        vm = self.create_vm()
+        self.service._settings = replace(self.settings, bridge="another-bridge")
+        before = self.settings.state_path.read_bytes()
+        self.host.events.clear()
+        with self.assertRaisesRegex(ConflictError, "retained network"):
+            self.service.start(vm.id)
+        self.assertEqual(self.settings.state_path.read_bytes(), before)
+        self.assertNotIn(("network.ensure_bridge", None), self.host.events)
+        self.assertNotIn("firewall.apply", self.host.actions(vm.id))
+
+    def test_live_start_can_retry_an_egress_setup_failure_with_the_same_process(self) -> None:
+        vm = self.create_vm()
+        self.host.failures["network.ensure_bridge", None] = UvmError("egress setup failed")
+        with self.assertRaisesRegex(UvmError, "egress setup failed"):
+            self.service.start(vm.id)
+        self.assertEqual(self.process.observe(self.store.get(vm.id)), "alive")
+        self.assertEqual(self.network.firewall.status(vm), "enforced")
+        self.assertEqual(self.service.start(vm.id).pid, vm.pid)
+        self.assertEqual(len(self.process.started), 1)
+        self.assertEqual(self.provision.call_count, 1)
+
 
 class LifecycleBulkTests(LifecycleTestCase):
     def test_all_bulk_actions_take_one_outer_lock_and_use_only_selected_ids(self) -> None:

tests/test_network.py

@@ -5,6 +5,7 @@ import unittest
 from copy import deepcopy
 from dataclasses import replace
 from ipaddress import IPv4Address, IPv4Network
+from unittest.mock import patch
 
 from uvm.config import Settings
 from uvm.domain import PortRule, VmRecord
@@ -22,6 +23,10 @@ class FakeRunner:
         self.links: dict[str, dict] = {}
         self.inventory: str | None = None
         self.failures: dict[tuple[str, ...], BaseException] = {}
+        self.default_routes = "default via 192.0.2.1 dev eth0\n"
+        self.rules: dict[tuple[str, str], list[tuple[str, ...]]] = {
+            ("nat", "POSTROUTING"): [], ("filter", "FORWARD"): [],
+        }
 
     def run(self, command, *, check=True, capture=False, timeout=None) -> CommandResult:
         del capture, timeout
@@ -68,11 +73,21 @@ class FakeRunner:
         elif args[:3] == ("ip", "addr", "replace") or args[:2] == ("sysctl", "-w"):
             pass
         elif args[0] == "iptables":
-            return CommandResult(args=args, returncode=1 if "-C" in args else 0)
+            if args[1:4] != ("--wait", "5", "-t"):
+                raise AssertionError(f"unexpected iptables options: {args}")
+            table, operation, chain, *rule = args[4:]
+            rules = self.rules[table, chain]
+            if operation == "-C":
+                return CommandResult(args=args, returncode=0 if tuple(rule) in rules else 1)
+            if operation == "-I":
+                position, *rule = rule
+                rules.insert(int(position) - 1, tuple(rule))
+            else:
+                raise AssertionError(f"unexpected iptables mutation: {args}")
         elif args != ("ip", "route", "show", "default"):
             raise AssertionError(f"unexpected host command: {args}")
         if args == ("ip", "route", "show", "default"):
-            return CommandResult(args=args, returncode=0, stdout="default via 192.0.2.1 dev eth0\n")
+            return CommandResult(args=args, returncode=0, stdout=self.default_routes)
         return CommandResult(args=args, returncode=0)
 
 
@@ -127,44 +142,61 @@ class NetworkManagerTests(unittest.TestCase):
             ("ip", "addr", "replace", "10.42.0.1/24", "dev", "uvm0"),
             self.runner.calls,
         )
-        self.assertIn(
+        self.assertIn(("sysctl", "-w", "net.ipv4.ip_forward=1"), self.runner.calls)
-            (
+        nat, = self.runner.rules["nat", "POSTROUTING"]
-                "iptables",
+        self.assertEqual(nat[:6], ("-s", "10.42.0.0/24", "!", "-o", "uvm0", "-m"))
-                "-A",
+        self.assertEqual(nat[-2:], ("-j", "MASQUERADE"))
-                "FORWARD",
+        inbound, outbound = self.runner.rules["filter", "FORWARD"]
-                "-i",
+        self.assertEqual(outbound[:4], ("-i", "uvm0", "-s", "10.42.0.0/24"))
-                "uvm0",
+        self.assertEqual(inbound[:8], (
-                "-o",
+            "-o", "uvm0", "-d", "10.42.0.0/24", "-m", "conntrack",
-                "eth0",
+            "--ctstate", "ESTABLISHED,RELATED",
-                "-s",
+        ))
-                "10.42.0.0/24",
+        self.assertEqual(inbound[-2:], ("-j", "ACCEPT"))
-                "-j",
+        self.assertEqual(outbound[-2:], ("-j", "ACCEPT"))
-                "ACCEPT",
+
-            ),
+    def test_egress_is_installed_offline_and_survives_uplink_changes_without_extra_rules(self) -> None:
-            self.runner.calls,
+        self.runner.default_routes = ""
-        )
+        self.network.ensure_bridge()
-        checks = [
+        offline_rules = deepcopy(self.runner.rules)
-            call
+        self.assertEqual([len(rules) for rules in offline_rules.values()], [1, 2])
-            for call in self.runner.calls
+
-            if call[:5] == ("iptables", "-t", "nat", "-C", "POSTROUTING")
+        for route in (
+            "default via 192.0.2.1 dev eth0\n",
+            "default via 198.51.100.1 dev wlan0\n",
+            "default dev tun0\ndefault via 198.51.100.1 dev wlan0 metric 600\n",
+            "",
+        ):
+            with self.subTest(route=route):
+                self.runner.default_routes = route
+                self.runner.calls.clear()
+                self.network.ensure_bridge()
+                self.assertEqual(self.runner.rules, offline_rules)
+                self.assertFalse(any("-I" in call for call in self.runner.calls))
+        self.assertFalse(any("eth0" in rule or "wlan0" in rule or "tun0" in rule
+                             for rules in offline_rules.values() for rule in rules))
+
+    def test_egress_exceptions_precede_existing_drops_and_preserve_foreign_and_legacy_rules(self) -> None:
+        existing = [
+            ("-j", "HOST-FIREWALL"), ("-j", "DROP"),
+            ("-i", "uvm0", "-o", "eth0", "-s", "10.42.0.0/24", "-j", "ACCEPT"),
         ]
-        self.assertEqual(len(checks), 1)
+        self.runner.rules["filter", "FORWARD"] = list(existing)
-        self.assertIn(
+        self.network.ensure_bridge()
-            (
+        rules = list(self.runner.rules["filter", "FORWARD"])
-                "iptables",
+        self.assertEqual(rules[2:], existing)
-                "-t",
+        self.assertTrue(all("uvm-egress" in rule and rule[-1] == "ACCEPT" for rule in rules[:2]))
-                "nat",
+        self.network.ensure_bridge()
-                "-A",
+        self.assertEqual(self.runner.rules["filter", "FORWARD"], rules)
-                "POSTROUTING",
+
-                "-s",
+    def test_iptables_inspection_errors_are_not_treated_as_missing_rules(self) -> None:
-                "10.42.0.0/24",
+        check = ("iptables", "--wait", "5", "-t", "nat", "-C", "POSTROUTING",
-                "-o",
+                 "-s", "10.42.0.0/24", "!", "-o", "uvm0",
-                "eth0",
+                 "-m", "comment", "--comment", "uvm-egress", "-j", "MASQUERADE")
-                "-j",
+        with patch.object(self.runner, "run", return_value=CommandResult(check, 4, stderr="permission denied")) as run:
-                "MASQUERADE",
+            with self.assertRaisesRegex(CommandError, "could not inspect.*permission denied"):
-            ),
+                self.network._ensure_masquerade()
-            self.runner.calls,
+        self.assertEqual(run.call_count, 1)
-        )
 
     def test_bridge_and_guest_network_support_non_default_prefixes(self) -> None:
         settings = Settings(
@@ -178,6 +210,10 @@ class NetworkManagerTests(unittest.TestCase):
             ("ip", "addr", "replace", "10.50.0.1/16", "dev", "uvm0"),
             runner.calls,
         )
+        for rules in runner.rules.values():
+            for rule in rules:
+                self.assertIn("10.50.0.0/16", rule)
+                self.assertNotIn("10.42.0.0/24", rule)
 
     def test_refuses_to_adopt_an_existing_tap_device(self) -> None:
         for alias in (None, "foreign-owner", f"uvm:{self.vm.id}"):

uvm/lifecycle.py

@@ -177,7 +177,10 @@ class LifecycleService:
                 raise ConflictError(f"resize is pending for {vm.id}; retry resize or stop it before starting")
             if vm.status != "running" or vm.operation not in {None, "policy"}:
                 raise ConflictError(f"{vm.id} has an interrupted boot/stop; stop it before starting")
-            return self._sync_ports_locked(vm)
+            self._validate_retained_network(vm)
+            vm = self._sync_ports_locked(vm)
+            self._network.ensure_bridge()
+            return vm
         self._validate_resume(vm)
         if kernel is None:
             self._check_boot()
@@ -276,6 +279,9 @@ class LifecycleService:
         disk = Path(vm.disk)
         if not vm.disk or not disk.is_file() or disk.is_symlink() or disk.stat().st_nlink != 1:
             raise UvmError(f"private VM disk is missing or unsafe: {disk}")
+        self._validate_retained_network(vm)
+
+    def _validate_retained_network(self, vm: VmRecord) -> None:
         if (vm.gateway != str(self._settings.gateway)
                 or vm.network not in {None, str(self._settings.network)}
                 or vm.bridge not in {None, self._settings.bridge}):

uvm/network.py

@@ -9,7 +9,7 @@ from ipaddress import IPv4Address
 
 from .config import Settings
 from .domain import VmRecord
-from .errors import ConflictError, UvmError, ValidationError
+from .errors import CommandError, ConflictError, UvmError, ValidationError
 from .firewall import FirewallManager
 from .system import CommandRunner
 
@@ -178,111 +178,38 @@ class NetworkManager:
         raise UvmError(f"could not delete TAP device {name}{suffix}")
 
     def _ensure_masquerade(self) -> None:
-        route = self._runner.run(("ip", "route", "show", "default"), capture=True)
+        # Match the guest network, not a snapshot of the host's default uplink.
-        match = re.search(r"\bdev\s+(\S+)", route.stdout)
+        # Routing selects the current interface for each destination, even after
-        if match is None:
+        # an offline boot, a reconnect, or a switch between Wi-Fi/Ethernet/VPN.
-            return
+        bridge = self._settings.bridge
-        uplink = match.group(1)
+        network = str(self._settings.network)
-        rule = (
+        owner = ("-m", "comment", "--comment", "uvm-egress")
-            "iptables",
+        self._ensure_iptables_rule(
-            "-t",
+            "nat", "POSTROUTING",
-            "nat",
+            ("-s", network, "!", "-o", bridge, *owner, "-j", "MASQUERADE"),
-            "-C",
-            "POSTROUTING",
-            "-s",
-            str(self._settings.network),
-            "-o",
-            uplink,
-            "-j",
-            "MASQUERADE",
-        )
-        present = self._runner.run(rule, check=False)
-        if present.returncode != 0:
-            self._runner.run(
-                (
-                    "iptables",
-                    "-t",
-                    "nat",
-                    "-A",
-                    "POSTROUTING",
-                    "-s",
-                    str(self._settings.network),
-                    "-o",
-                    uplink,
-                    "-j",
-                    "MASQUERADE",
         )
+        # Native bridge policy still checks guest identity and private ports.
+        # This also permits peer traffic when br_netfilter is already enabled.
+        self._ensure_iptables_rule(
+            "filter", "FORWARD",
+            ("-i", bridge, "-s", network, *owner, "-j", "ACCEPT"),
         )
         self._ensure_iptables_rule(
-            (
+            "filter", "FORWARD",
-                "iptables",
+            ("-o", bridge, "-d", network, "-m", "conntrack",
-                "-C",
+             "--ctstate", "ESTABLISHED,RELATED", *owner, "-j", "ACCEPT"),
-                "FORWARD",
-                "-i",
-                self._settings.bridge,
-                "-o",
-                uplink,
-                "-s",
-                str(self._settings.network),
-                "-j",
-                "ACCEPT",
-            ),
-            (
-                "iptables",
-                "-A",
-                "FORWARD",
-                "-i",
-                self._settings.bridge,
-                "-o",
-                uplink,
-                "-s",
-                str(self._settings.network),
-                "-j",
-                "ACCEPT",
-            ),
-        )
-        self._ensure_iptables_rule(
-            (
-                "iptables",
-                "-C",
-                "FORWARD",
-                "-i",
-                uplink,
-                "-o",
-                self._settings.bridge,
-                "-d",
-                str(self._settings.network),
-                "-m",
-                "conntrack",
-                "--ctstate",
-                "ESTABLISHED,RELATED",
-                "-j",
-                "ACCEPT",
-            ),
-            (
-                "iptables",
-                "-A",
-                "FORWARD",
-                "-i",
-                uplink,
-                "-o",
-                self._settings.bridge,
-                "-d",
-                str(self._settings.network),
-                "-m",
-                "conntrack",
-                "--ctstate",
-                "ESTABLISHED,RELATED",
-                "-j",
-                "ACCEPT",
-            ),
         )
 
     def _ensure_iptables_rule(
-        self,
+        self, table: str, chain: str, rule: tuple[str, ...],
-        check_rule: tuple[str, ...],
-        add_rule: tuple[str, ...],
     ) -> None:
-        present = self._runner.run(check_rule, check=False)
+        command = ("iptables", "--wait", "5", "-t", table)
-        if present.returncode != 0:
+        present = self._runner.run((*command, "-C", chain, *rule), check=False, capture=True)
-            self._runner.run(add_rule)
+        if present.returncode == 0:
+            return
+        if present.returncode != 1:
+            detail = present.stderr.strip() or present.stdout.strip()
+            raise CommandError(f"could not inspect UVM {table}/{chain} rule ({present.returncode}): {detail}")
+        # Scoped exceptions must precede terminal drops in the host's chains.
+        # Keep unrelated rules and the global forwarding policy intact.
+        self._runner.run((*command, "-I", chain, "1", *rule), capture=True)