Enhance documentation and validation for VM lifecycle and network management; add tests for legacy cleanup and address validation.

tests/test_firewall.py

@@ -442,6 +442,80 @@ class FirewallManagerTests(unittest.TestCase):
         with self.assertRaises(CommandError):
             self.manager.cleanup_connections(self.vm)
 
+    def test_legacy_cleanup_outside_current_network_uses_only_recorded_ip(self):
+        record = dict(id="vm-legacy", cpu=1, ram_mib=128, guest_ip="203.0.113.7",
+                      gateway="203.0.113.1", tap="tap-legacy", mac=self.vm.mac,
+                      socket="", config="", log="", status="stopped")
+        for stored_network in (None, "203.0.113.0/24"):
+            with self.subTest(network=stored_network):
+                vm = VmRecord.from_dict({**record, **({"network": stored_network} if stored_network else {})})
+                self.assertIsNone(vm.ports)
+                self.runner.calls.clear()
+                self.manager.cleanup_connections(vm)
+                expected = [("ip", "-j", "-d", "link", "show")]
+                expected.extend(("conntrack", command, "--family", "ipv4", field, "203.0.113.7")
+                                for field in ("--orig-src", "--orig-dst", "--reply-src", "--reply-dst")
+                                for command in ("--delete", "--list"))
+                # No nft policy, publishing, or unrelated address/conntrack flush.
+                self.assertEqual([args for args, _ in self.runner.calls], expected)
+
+    def test_cleanup_host_boundaries_use_stored_network_not_current_settings(self):
+        cases = (
+            ("10.77.8.2", "10.77.0.0/16"),
+            ("100.64.3.4", "100.64.0.0/16"),
+            ("10.42.0.1", None),  # Current gateway is not a cleanup constraint.
+            ("10.42.0.0", None),  # Do not guess the old prefix from current settings.
+            ("10.42.0.255", None),
+            ("203.0.113.0", "203.0.0.0/16"),
+            ("203.0.113.255", "203.0.0.0/16"),
+            ("203.0.113.8", "203.0.113.8/31"),
+            ("203.0.113.9", "203.0.113.8/31"),
+            ("203.0.113.8", "203.0.113.8/32"),
+        )
+        for address, network in cases:
+            with self.subTest(address=address, network=network):
+                self.runner.calls.clear()
+                self.manager.cleanup_connections(vm_record(guest_ip=address, network=network, ports=None))
+                deletes = [args for args, _ in self.runner.calls if args[:2] == ("conntrack", "--delete")]
+                self.assertEqual(len(deletes), 4)
+                self.assertTrue(all(args[-1] == address for args in deletes))
+
+    def test_cleanup_rejects_invalid_or_non_host_persisted_addresses(self):
+        cases = [(address, None) for address in (
+            "not-an-ip", "203.0.113.999", "203.0.113.7/24", "2001:db8::7",
+            "::ffff:203.0.113.7", None, True, 3405803783,
+            "0.0.0.0", "127.0.0.1", "127.12.2.3", "169.254.1.2",
+            "224.0.0.1", "239.255.255.255", "255.255.255.255",
+        )]
+        cases.extend((
+            ("203.0.113.0", "203.0.113.0/24"),
+            ("203.0.113.255", "203.0.113.0/24"),
+            ("203.0.113.7", "203.0.114.0/24"),
+            ("203.0.113.7", "not-a-network"),
+            ("203.0.113.7", ""),
+            ("203.0.113.7", "2001:db8::/64"),
+        ))
+        for address, network in cases:
+            with self.subTest(address=address, network=network):
+                self.runner.calls.clear()
+                with self.assertRaises(ValidationError):
+                    self.manager.cleanup_connections(vm_record(guest_ip=address, network=network, ports=None))
+                self.assertEqual([args for args, _ in self.runner.calls], [("ip", "-j", "-d", "link", "show")])
+
+    def test_legacy_public_cleanup_still_requires_tap_absence(self):
+        self.runner.links = [{"ifname": "tap-legacy"}]
+        vm = vm_record(tap="tap-legacy", guest_ip="203.0.113.7", ports=None)
+        with self.assertRaisesRegex(UvmError, "still exists"):
+            self.manager.cleanup_connections(vm)
+        self.assertFalse(any(args[0] == "conntrack" for args, _ in self.runner.calls))
+
+    def test_cleanup_compatibility_does_not_allow_a_public_managed_policy(self):
+        vm = vm_record(guest_ip="203.0.113.7", network="203.0.113.0/24", ports=())
+        with self.assertRaisesRegex(ValidationError, "private guest IP"):
+            self.manager.apply(vm)
+        self.assertEqual(self.runner.calls, [])
+        self.manager.cleanup_connections(vm)
+
     def test_availability_checks_kernel_and_full_policy_without_committing(self):
         self.manager.check_available()
         self.assertEqual(self.runner.tables, {})

tests/test_lifecycle.py

@@ -1,11 +1,12 @@
 from __future__ import annotations
 
+import json
 import tempfile
 import unittest
 from collections.abc import Callable, Iterable, Iterator
 from contextlib import contextmanager
 from dataclasses import replace
-from ipaddress import IPv4Address
+from ipaddress import IPv4Address, IPv4Network
 from pathlib import Path
 from unittest.mock import patch
 
@@ -485,6 +486,81 @@ class LifecycleTests(LifecycleTestCase):
         self.provision.assert_not_called()
         self.copy_disk.assert_not_called()
 
+    def test_create_rejects_retained_topology_changes_before_allocation_or_bridge_work(self) -> None:
+        vm = self.create_vm()
+        self.copy_disk.reset_mock()
+        self.provision.reset_mock()
+        changes = (
+            {"gateway": IPv4Address(vm.guest_ip)},
+            {"gateway": IPv4Address("10.42.0.10")},
+            {"network": IPv4Network("10.42.0.0/25")},
+            {"bridge": "uvm-other"},
+            {"gateway": IPv4Address(vm.guest_ip), "network": IPv4Network("10.42.0.0/25")},
+        )
+        for status in ("running", "stopped"):
+            if status == "stopped":
+                self.service.stop(vm.id)
+            before = self.settings.state_path.read_bytes()
+            before_directories = set(self.settings.vms_dir.iterdir())
+            before_taps = self.network.taps.copy()
+            before_active = self.network.active_taps.copy()
+            before_tables = self.network.firewall.tables.copy()
+            for changed in changes:
+                with self.subTest(status=status, changed=changed):
+                    settings = replace(self.settings, **changed)
+                    service = LifecycleService(
+                        settings, self.store, self.images, self.network, self.process,
+                        client_factory=self.make_client,
+                    )
+                    self.host.events.clear()
+                    with (
+                        patch.object(self.network, "_settings", settings),
+                        patch.object(self.network, "allocate_ip", wraps=self.network.allocate_ip) as allocate,
+                        self.assertRaisesRegex(ConflictError, "topology conflicts with retained VMs"),
+                    ):
+                        service.create(VmSpec(cpu=1, ram_mib=512))
+                    allocate.assert_not_called()
+                    self.assertEqual(self.settings.state_path.read_bytes(), before)
+                    self.assertEqual(set(self.settings.vms_dir.iterdir()), before_directories)
+                    self.assertEqual(self.network.taps, before_taps)
+                    self.assertEqual(self.network.active_taps, before_active)
+                    self.assertEqual(self.network.firewall.tables, before_tables)
+                    self.assertEqual(self.host.events, [("firewall.check_available", None)])
+        self.copy_disk.assert_not_called()
+        self.provision.assert_not_called()
+        self.assertEqual(len(self.process.started), 1)
+        self.assertEqual(Path(vm.disk).read_bytes(), b"rootfs")
+
+    def test_non_rfc1918_network_fails_before_kvm_assets_or_registry_reservation(self) -> None:
+        for network, gateway in (
+            ("203.0.113.0/24", "203.0.113.1"),
+            ("100.64.0.0/24", "100.64.0.1"),
+            ("8.8.8.0/24", "8.8.8.1"),
+            ("10.0.0.0/7", "10.0.0.1"),
+        ):
+            with self.subTest(network=network):
+                settings = replace(self.settings, network=IPv4Network(network), gateway=IPv4Address(gateway))
+                service = LifecycleService(
+                    settings, self.store, self.images, self.network, self.process,
+                    client_factory=self.make_client,
+                )
+                with (
+                    patch.object(self.network, "_settings", settings),
+                    patch.object(self.images, "installed_assets", wraps=self.images.installed_assets) as assets,
+                    patch.object(self.network, "allocate_ip", wraps=self.network.allocate_ip) as allocate,
+                    self.assertRaisesRegex(ValidationError, "RFC 1918"),
+                ):
+                    service.create(VmSpec(cpu=1, ram_mib=512))
+                assets.assert_not_called()
+                allocate.assert_not_called()
+                self.check_kvm.assert_not_called()
+                self.assertFalse(self.settings.state_path.exists())
+                self.assertEqual(self.store.load().vms, {})
+                self.assertEqual(list(self.settings.vms_dir.iterdir()), [])
+                self.assertEqual(self.host.events, [])
+                self.provision.assert_not_called()
+                self.copy_disk.assert_not_called()
+
     def test_create_cleans_up_if_policy_readback_is_not_enforced_after_boot(self) -> None:
         with patch.object(self.network.firewall, "status", return_value="drift"):
             with self.assertRaisesRegex(UvmError, "enforcement could not be confirmed after boot"):
@@ -605,10 +681,54 @@ class LifecycleTests(LifecycleTestCase):
         self.copy_disk.assert_not_called()
         self.assertEqual(self.network.created_taps, [])
         self.assertEqual(self.process.started, [])
-        retained = self.store.load().vms.get("vm-existing")
-        if retained is not None:
-            self.assertFalse(retained.runtime_owned)
-            self.assertFalse(retained.disk_ready)
+        retained = self.store.get("vm-existing")
+        self.assertFalse(retained.runtime_owned)
+        self.assertFalse(retained.disk_ready)
+        with self.assertRaisesRegex(UvmError, "refusing to remove unowned VM directory"):
+            self.service.destroy(retained.id)
+        self.assertFalse(self.store.get(retained.id).runtime_owned)
+        self.assertEqual({path.name: (path.stat().st_ino, path.read_bytes()) for path in directory.iterdir()}, before)
+
+    def test_delete_legacy_runtime_without_private_disk_preserves_shared_rootfs(self) -> None:
+        shared_rootfs = self.settings.rootfs_image
+        before = (shared_rootfs.stat().st_ino, shared_rootfs.read_bytes())
+        for disk_field in ({}, {"disk": ""}):
+            with self.subTest(disk_field=disk_field):
+                directory = self.settings.vm_dir("vm-legacy")
+                directory.mkdir(parents=True, mode=0o700)
+                raw = {
+                    "id": "vm-legacy", "cpu": 1, "ram_mib": 512,
+                    "guest_ip": "10.42.0.2", "gateway": "10.42.0.1",
+                    "tap": self.network.tap_name("vm-legacy"), "mac": self.network.mac_for(1),
+                    "socket": str(directory / "firecracker.sock"),
+                    "config": str(directory / "config.json"),
+                    "log": str(directory / "firecracker.log"),
+                    "status": "running", "pid": 12345, "process_start_time": "42",
+                    **disk_field,
+                }
+                Path(raw["socket"]).touch()
+                Path(raw["log"]).write_bytes(b"legacy boot log")
+                Path(raw["config"]).write_text(json.dumps({
+                    "drives": [{"path_on_host": str(shared_rootfs), "is_read_only": False}],
+                }), encoding="utf-8")
+                self.settings.state_path.write_text(json.dumps({
+                    "schema_version": 1, "vms": {raw["id"]: raw},
+                }), encoding="utf-8")
+                self.settings.state_path.chmod(0o600)
+                vm = self.store.get(raw["id"])
+                self.assertTrue(vm.runtime_owned)
+                self.assertEqual(vm.disk, "")
+                self.assertFalse(vm.disk_ready)
+                self.assertEqual(self.process.observe(vm), "gone")
+
+                self.service.destroy(vm.id)
+
+                self.assertEqual(self.store.load().vms, {})
+                self.assertFalse(directory.exists())
+                self.assertEqual((shared_rootfs.stat().st_ino, shared_rootfs.read_bytes()), before)
+        self.copy_disk.assert_not_called()
+        self.provision.assert_not_called()
+        self.check_kvm.assert_not_called()
 
     def test_stale_stopping_and_deleting_records_can_finish_cleanup(self) -> None:
         for action, status, operation in (("stop", "stopping", "stop"), ("destroy", "terminating", "delete")):

tests/test_lifecycle_commands.py

@@ -43,7 +43,7 @@ class LifecycleLookupTests(LifecycleTestCase):
         before = self.settings.state_path.read_bytes()
         for kwargs, message in (
             ({"name": vm.name}, "VM name already exists"),
-            ({"name": vm.id}, "VM name already exists"),
+            ({"name": vm.id}, "must not impersonate generated VM IDs"),
             ({"guest_ip": IPv4Address(vm.guest_ip)}, "IP already allocated"),
         ):
             with self.subTest(kwargs=kwargs), self.assertRaisesRegex(UvmError, message):
@@ -53,6 +53,16 @@ class LifecycleLookupTests(LifecycleTestCase):
         self.assertEqual(self.copy_disk.call_count, 1)
         self.assertEqual(len(self.process.started), 1)
 
+    def test_generated_id_names_are_rejected_before_state_or_host_work(self) -> None:
+        with self.assertRaisesRegex(ValidationError, "must not impersonate generated VM IDs"):
+            self.create_vm(name="vm-0123456789abcdef0123456789abcdef")
+        self.assertFalse(self.settings.state_path.exists())
+        self.assertEqual(self.store.operation_locks, 0)
+        self.assertEqual(self.host.events, [])
+        self.check_kvm.assert_not_called()
+        self.provision.assert_not_called()
+        self.copy_disk.assert_not_called()
+
     def test_generated_id_retries_id_name_and_tap_collisions(self) -> None:
         with patch("uvm.lifecycle.new_vm_id", return_value="vm-first-same1234567"):
             first = self.create_vm(name="reserved-name")
@@ -68,8 +78,9 @@ class LifecycleLookupTests(LifecycleTestCase):
         self.assertNotEqual(second.guest_ip, first.guest_ip)
         self.assertEqual(self.network.taps, {first.tap: first.id, second.tap: second.id})
 
-    def test_ambiguous_persisted_name_ip_or_id_never_selects_an_arbitrary_vm(self) -> None:
-        first = self.create_vm(name="first")
+    def test_ambiguous_persisted_name_ip_or_legacy_id_never_selects_an_arbitrary_vm(self) -> None:
+        with patch("uvm.lifecycle.new_vm_id", return_value="vm-legacy"):
+            first = self.create_vm(name="first")
         second = self.create_vm(name="second")
         for identifier in (first.guest_ip, first.id, first.name):
             with self.subTest(identifier=identifier):
@@ -85,6 +96,34 @@ class LifecycleLookupTests(LifecycleTestCase):
                 self.assertEqual(self.settings.state_path.read_bytes(), before)
                 self.assertEqual(self.host.events, [])
 
+    def test_exact_generated_id_wins_over_a_malformed_legacy_name(self) -> None:
+        first = self.create_vm(name="first")
+        second = self.create_vm(name="second")
+        self.persist(second, name=first.id)
+        before = self.settings.state_path.read_bytes()
+        second_record = self.store.get(second.id).to_dict()
+        self.host.events.clear()
+        self.assertEqual(self.service.inspect(first.id).vm.id, first.id)
+        self.assertEqual(self.service.find_for_ssh(first.id).id, first.id)
+        self.assertEqual(self.service.log_path(first.id), Path(first.log))
+        self.assertEqual([vm.id for vm in self.service.resolve_targets([first.id])], [first.id])
+        self.assertEqual(self.settings.state_path.read_bytes(), before)
+        self.assertEqual(self.service.stop(first.id).id, first.id)
+        self.service.destroy(first.id)
+        self.assertEqual(self.store.get(second.id).to_dict(), second_record)
+        self.assertEqual(self.host.actions(second.id), [])
+
+        before = self.settings.state_path.read_bytes()
+        self.host.events.clear()
+        for method in (self.service.inspect, self.service.find_for_ssh,
+                       self.service.log_path, self.service.stop):
+            with self.assertRaises(NotFoundError):
+                method(first.id)
+        with self.assertRaises(NotFoundError):
+            self.service.resolve_targets([first.id])
+        self.assertEqual(self.settings.state_path.read_bytes(), before)
+        self.assertEqual(self.host.events, [])
+
     def test_target_resolution_rejects_invalid_or_missing_targets_without_mutation(self) -> None:
         vm = self.create_vm(name="web")
         before = self.settings.state_path.read_bytes()
@@ -102,6 +141,36 @@ class LifecycleLookupTests(LifecycleTestCase):
 
 
 class LifecyclePortTests(LifecycleTestCase):
+    def test_set_ports_requires_an_exact_registry_id_not_a_name_or_ip(self) -> None:
+        vm = self.create_vm(name="web")
+        before = self.settings.state_path.read_bytes()
+        self.host.events.clear()
+        for identifier in (vm.name, vm.guest_ip):
+            with self.subTest(identifier=identifier), self.assertRaises(NotFoundError):
+                self.service.set_ports(identifier, ())
+        self.assertEqual(self.settings.state_path.read_bytes(), before)
+        self.assertEqual(self.host.events, [])
+        self.assertEqual(self.service.set_ports(vm.id, ()).ports, ())
+
+    def test_frozen_legacy_id_cannot_retarget_policy_after_name_reuse(self) -> None:
+        with patch("uvm.lifecycle.new_vm_id", return_value="vm-legacy"):
+            original = self.create_vm(name="original")
+        selected = self.service.inspect(original.name).vm.id
+        self.service.destroy(original.id)
+        replacement = self.create_vm(name=original.id, ports=(PortRule(22), PortRule(80)))
+        self.assertEqual(self.service.inspect(selected).vm.id, replacement.id)
+        before = self.settings.state_path.read_bytes()
+        before_tables = self.network.firewall.tables.copy()
+        self.host.events.clear()
+        for mode, ports in (("open", (PortRule(443),)), ("close", (PortRule(80),)), ("set", ())):
+            with self.subTest(mode=mode), self.assertRaises(NotFoundError):
+                self.service.set_ports(selected, ports, mode=mode)
+            self.assertEqual(self.settings.state_path.read_bytes(), before)
+            self.assertEqual(self.network.firewall.tables, before_tables)
+            self.assertEqual(self.host.events, [])
+        self.assertIn(replacement.tap, self.network.active_taps)
+        self.assertEqual(self.process.observe(replacement), "alive")
+
     def test_stopped_port_edits_and_sync_are_metadata_only(self) -> None:
         vm = self.create_vm()
         stopped = self.service.stop(vm.id)
@@ -195,8 +264,61 @@ class LifecyclePortTests(LifecycleTestCase):
         pending = self.store.get(vm.id)
         self.assertEqual(pending.ports, (PortRule(22),))
         self.assertEqual(pending.operation, "policy")
+        self.assertIn("link-down failed", pending.last_error)
+        self.assertTrue(self.network.has_tap(pending))
+        self.assertIn(vm.tap, self.network.active_taps)
         self.assertEqual(self.network.firewall.status(pending), "quarantined")
-        self.assertIn("firewall.quarantine", self.host.actions(vm.id))
+        self.assert_call_order(vm, "firewall.apply", "network.deactivate_tap", "firewall.quarantine")
+
+    def test_policy_failure_never_quarantines_a_missing_or_foreign_tap(self) -> None:
+        for owner in (None, "operator-owned"):
+            with self.subTest(owner=owner):
+                vm = self.create_vm(ports=(PortRule(22), PortRule(80)))
+                if owner is None:
+                    del self.network.taps[vm.tap]
+                    self.network.active_taps.discard(vm.tap)
+                else:
+                    self.network.taps[vm.tap] = owner
+                    with self.assertRaisesRegex(ConflictError, "unowned TAP"):
+                        self.network.has_tap(vm)
+                before_taps = self.network.taps.copy()
+                before_active = self.network.active_taps.copy()
+                before_tables = self.network.firewall.tables.copy()
+                self.host.events.clear()
+                with self.assertRaisesRegex(UvmError, "policy pending"):
+                    self.service.set_ports(vm.id, (PortRule(80),), mode="close")
+                pending = self.store.get(vm.id)
+                self.assertEqual((pending.ports, pending.operation), ((PortRule(22),), "policy"))
+                self.assertEqual(pending.pid, vm.pid)
+                self.assertEqual(self.network.taps, before_taps)
+                self.assertEqual(self.network.active_taps, before_active)
+                self.assertEqual(self.network.firewall.tables, before_tables)
+                self.assertFalse(set(self.host.actions(vm.id)) & {
+                    "firewall.apply", "firewall.quarantine", "firewall.remove",
+                    "firewall.cleanup_connections", "network.delete_vm_tap", "process.terminate",
+                })
+
+    def test_policy_fallback_rechecks_ownership_after_a_tap_is_replaced(self) -> None:
+        vm = self.create_vm(ports=(PortRule(22), PortRule(80)))
+        before_tables = self.network.firewall.tables.copy()
+        apply = self.network.firewall.apply
+
+        def replace_tap_and_fail(record, *, quarantine=False):
+            if not quarantine:
+                self.network.taps[record.tap] = "operator-owned"
+                self.host.failures["firewall.apply", record.id] = UvmError("policy rejected")
+            return apply(record, quarantine=quarantine)
+
+        self.host.events.clear()
+        with patch.object(self.network.firewall, "apply", side_effect=replace_tap_and_fail) as applied:
+            with self.assertRaisesRegex(UvmError, "policy pending.*unowned TAP"):
+                self.service.set_ports(vm.id, (PortRule(80),), mode="close")
+        self.assertEqual(applied.call_count, 1)
+        self.assertEqual(self.network.taps[vm.tap], "operator-owned")
+        self.assertIn(vm.tap, self.network.active_taps)
+        self.assertEqual(self.network.firewall.tables, before_tables)
+        self.assertNotIn("firewall.quarantine", self.host.actions(vm.id))
+        self.assertEqual(self.store.get(vm.id).ports, (PortRule(22),))
 
     def test_failed_close_keeps_tap_down_if_quarantine_also_fails(self) -> None:
         vm = self.create_vm(ports=(PortRule(22), PortRule(80)))
@@ -348,10 +470,12 @@ class LifecycleBulkTests(LifecycleTestCase):
         self.assertEqual(self.store.get(retained.id).status, "stopped")
 
     def test_bulk_exact_ids_do_not_retarget_a_new_vm_named_after_a_deleted_id(self) -> None:
-        original = self.create_vm(name="original")
+        with patch("uvm.lifecycle.new_vm_id", return_value="vm-legacy"):
+            original = self.create_vm(name="original")
         selected = tuple(vm.id for vm in self.service.resolve_targets([original.name]))
         self.service.destroy(original.id)
         replacement = self.create_vm(name=original.id)
+        self.assertEqual(self.service.inspect(original.id).vm.id, replacement.id)
         before = self.store.get(replacement.id).to_dict()
         self.host.events.clear()
         results = self.service.run_many("delete", selected)