From 8bc22a3dc4708ab3ba3cc052f6c2d143cb458660 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 3 May 2026 16:55:27 +0000 Subject: [PATCH 01/10] config: scope host-global resources to a per-installation instance id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ember installations on the same host previously shared the dm-thin pool name, the `em-*` TAP namespace, and (when both ran VMs) the IP subnet. Worse, reconcile() listed every `em-*` interface and would delete the other install's TAPs on any command, and `deinit --purge` tore down the singleton `ember-pool` regardless of which install was asking. Integration tests against a temp `--state-dir` could destroy the user's live install on the same machine. Introduce `instance_id` (4 hex chars, derived from a hash of the canonicalized state-dir at `ember init`, overridable via `--instance-id`) and `ip_subnet` (defaulting to a `/16` slot in `10.0.0.0/8` derived from the same hash, overridable via `--ip-subnet`). Both are persisted on `GlobalConfig`. Helper accessors derive every host-global name from the id: `ember-{id}-pool`, `ember-{id}-{img,vm}-` device-mapper prefixes, `em{id}-` TAP prefix (14-char names fit Linux IFNAMSIZ), and `ember:{id}` iptables comment threaded through every rule add/remove so cleanup only ever matches this install's rules. reconcile() now scopes the orphan-TAP sweep by the install's prefix and skips it entirely if the config can't be read, which is preferable to deleting a foreign device. NetworkBackend::teardown takes &GlobalConfig to thread the comment through; macOS impl is a no-op update. Removes the singleton `pool::POOL_NAME`, `thin::IMAGE_PREFIX`, `thin::VM_PREFIX`, `network::ip::DEFAULT_SUBNET`, and `tap::list_ember_devices()` constants/helpers — names now flow from config. No back-compat shim for older configs: existing installs need `ember deinit --purge && ember init` to migrate. https://claude.ai/code/session_01XbXjsQa8SRWLzxnuKFzxt6 --- crates/ember-core/src/backend.rs | 11 +- crates/ember-core/src/config.rs | 168 ++++++++++++++++++++++ crates/ember-core/src/network/ip.rs | 9 +- crates/ember-core/src/state/vm.rs | 2 +- crates/ember-linux/src/dm_thin.rs | 11 +- crates/ember-linux/src/dm_thin/pool.rs | 7 +- crates/ember-linux/src/dm_thin/thin.rs | 43 +++--- crates/ember-linux/src/dm_thin_storage.rs | 99 +++++++------ crates/ember-linux/src/network.rs | 14 +- crates/ember-linux/src/network/nat.rs | 68 +++++++-- crates/ember-linux/src/network/tap.rs | 42 +++--- crates/ember-linux/src/network_backend.rs | 20 ++- crates/ember-linux/src/platform.rs | 4 +- crates/ember-linux/src/reconcile.rs | 36 ++++- crates/ember-macos/src/network.rs | 2 +- src/cli/init.rs | 48 ++++++- src/cli/vm.rs | 14 +- tests/dm_thin.rs | 13 +- tests/vm.rs | 4 +- 19 files changed, 475 insertions(+), 140 deletions(-) diff --git a/crates/ember-core/src/backend.rs b/crates/ember-core/src/backend.rs index 9bd48a2..a0816df 100644 --- a/crates/ember-core/src/backend.rs +++ b/crates/ember-core/src/backend.rs @@ -62,6 +62,10 @@ pub struct InitConfig { pub storage_backend: crate::config::StorageKind, /// Path to the state directory (e.g., `/var/lib/ember` or `~/Library/Application Support/ember`). pub state_dir: PathBuf, + /// Per-installation namespace embedded in dm-thin pool / device + /// names so `ember init` against a fresh state-dir doesn't trample + /// another install's pool. Mirrors `GlobalConfig::instance_id`. + pub instance_id: String, /// ZFS pool name. Used on Linux for `zfs create`; ignored on macOS. pub pool: String, /// Dataset name within the ZFS pool. Used on Linux; ignored on macOS. @@ -208,7 +212,7 @@ pub trait StorageBackend { /// Mountable device path for a VM's root disk. /// /// Linux/ZFS: `/dev/zvol/pool/dataset/vms/vm_name`. - /// Linux/dm-thin: `/dev/mapper/ember-vm-`. + /// Linux/dm-thin: `/dev/mapper/ember--vm-`. /// macOS/APFS: `/vms//rootfs.img`. /// /// Backends that lazily activate kernel state (notably dm-thin: pool @@ -314,9 +318,10 @@ pub trait NetworkBackend { /// Tear down networking for a VM. /// - /// Linux: removes iptables rules, deletes TAP device, releases IP. + /// Linux: removes iptables rules (matched by per-installation + /// comment), deletes TAP device, releases IP. /// macOS: no-op (vmnet cleans up automatically). - fn teardown(&self, vm: &VmMetadata) -> Result<()>; + fn teardown(&self, vm: &VmMetadata, config: &GlobalConfig) -> Result<()>; /// Discover the guest's IP address from its MAC address. /// diff --git a/crates/ember-core/src/config.rs b/crates/ember-core/src/config.rs index dd12bc6..99de949 100644 --- a/crates/ember-core/src/config.rs +++ b/crates/ember-core/src/config.rs @@ -68,6 +68,22 @@ pub struct GlobalConfig { /// Populated during `ember init`; defaults to empty path for backwards compat. #[serde(default)] pub state_dir: PathBuf, + /// Per-installation namespace, derived at `ember init` from a hash + /// of the canonicalized state directory (or supplied via + /// `--instance-id`). 4 hex chars; embedded in every host-global + /// resource name (dm-thin pool, TAP devices, iptables comments) so + /// two ember installations on the same host don't clash. Empty + /// values are treated as a malformed config — `ember init` always + /// pins a non-empty value. + #[serde(default)] + pub instance_id: String, + /// IPv4 base subnet handed out as /30 links to VMs. Defaults at + /// init time to `10.{slot}.0.0/16` where `slot` is derived from the + /// instance-id hash, so two installs get non-overlapping ranges + /// without the user having to think about it. Overridable via + /// `--ip-subnet`. + #[serde(default = "default_ip_subnet")] + pub ip_subnet: String, /// Backing path for non-ZFS backends. /// /// * btrfs: block device or sparse image file containing the btrfs filesystem. @@ -92,6 +108,55 @@ pub struct GlobalConfig { pub dm_thin_mode: Option, } +/// Fallback subnet used when a config predates `ip_subnet`. New +/// installs derive this at init from the instance-id hash; this value +/// only applies to deserialization of older `config.json` files. +pub fn default_ip_subnet() -> String { + "10.100.0.0/16".to_string() +} + +/// Derive a default 4-hex-char `instance_id` from the canonicalized +/// state directory path. Two installations with distinct state +/// directories almost always get distinct ids; the same state +/// directory is stable across invocations so reactivation finds the +/// resources it created at init time. +/// +/// 16-bit space is small (~256-instance birthday-collision threshold), +/// but two installs on one host is a personal-use scenario; users who +/// hit a collision can pass `--instance-id` explicitly. +pub fn derive_instance_id(state_dir: &std::path::Path) -> String { + // Canonicalize when possible so `/var/lib/ember` and + // `/var/lib/ember/` hash to the same id; fall back to the literal + // path bytes when the directory does not yet exist. + let canonical = state_dir + .canonicalize() + .unwrap_or_else(|_| state_dir.to_path_buf()); + let bytes = canonical.as_os_str().as_encoded_bytes(); + format!("{:04x}", fnv1a_32(bytes) as u16) +} + +/// Derive a default `/16` IPv4 subnet from an instance id, chosen so +/// two installations rarely overlap: `10.{slot}.0.0/16` where `slot` +/// is the high byte of the same FNV-1a hash that produced the id. The +/// /16 still gives ~16k VMs per install via /30 links. +pub fn derive_ip_subnet(instance_id: &str) -> String { + let hash = fnv1a_32(instance_id.as_bytes()); + let slot = ((hash >> 8) & 0xff) as u8; + format!("10.{slot}.0.0/16") +} + +/// FNV-1a 32-bit hash. Stable across Rust versions (unlike +/// `DefaultHasher`) and small enough to inline rather than pulling in +/// a crypto dep just for non-security-critical name derivation. +fn fnv1a_32(bytes: &[u8]) -> u32 { + let mut h: u32 = 0x811c_9dc5; + for &b in bytes { + h ^= b as u32; + h = h.wrapping_mul(0x0100_0193); + } + h +} + impl GlobalConfig { /// Full ZFS dataset path for images (e.g. `ember/ember/images`). pub fn images_dataset(&self) -> String { @@ -102,4 +167,107 @@ impl GlobalConfig { pub fn vms_dataset(&self) -> String { format!("{}/{}/vms", self.pool, self.dataset) } + + /// dm-thin pool name, e.g. `ember-a3f4-pool`. Embedded in every + /// `dmsetup` invocation against the pool. + pub fn dm_thin_pool_name(&self) -> String { + format!("ember-{}-pool", self.instance_id) + } + + /// Device-mapper name prefix for image base volumes, e.g. + /// `ember-a3f4-img-`. + pub fn dm_thin_image_prefix(&self) -> String { + format!("ember-{}-img-", self.instance_id) + } + + /// Device-mapper name prefix for VM disks, e.g. `ember-a3f4-vm-`. + pub fn dm_thin_vm_prefix(&self) -> String { + format!("ember-{}-vm-", self.instance_id) + } + + /// TAP device name prefix, e.g. `ema3f4-`. Bounded so `prefix + + /// 7-hex VM id` fits in Linux's 15-char `IFNAMSIZ - 1` budget. + pub fn tap_prefix(&self) -> String { + format!("em{}-", self.instance_id) + } + + /// Comment string embedded in iptables rules, used to filter this + /// install's rules from any other ember install on the host. + pub fn iptables_comment(&self) -> String { + format!("ember:{}", self.instance_id) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::PathBuf; + + fn config_with_id(id: &str) -> GlobalConfig { + GlobalConfig { + storage_backend: StorageKind::Zfs, + pool: "tank".to_string(), + dataset: "ember".to_string(), + kernel_path: None, + wan_iface: None, + state_dir: PathBuf::default(), + instance_id: id.to_string(), + ip_subnet: "10.100.0.0/16".to_string(), + storage_path: None, + dm_thin_block_size: None, + dm_thin_mode: None, + } + } + + #[test] + fn instance_id_derived_from_state_dir_is_4_hex_chars() { + let id = derive_instance_id(std::path::Path::new("/var/lib/ember")); + assert_eq!(id.len(), 4); + assert!(id.chars().all(|c| c.is_ascii_hexdigit())); + } + + #[test] + fn instance_id_is_stable_across_calls() { + let p = std::path::Path::new("/some/path/that/does/not/exist"); + assert_eq!(derive_instance_id(p), derive_instance_id(p)); + } + + #[test] + fn distinct_state_dirs_usually_get_distinct_ids() { + // 16-bit space, but two unrelated paths almost never collide. + let a = derive_instance_id(std::path::Path::new("/var/lib/ember")); + let b = derive_instance_id(std::path::Path::new("/tmp/ember-test")); + assert_ne!(a, b); + } + + #[test] + fn derived_subnet_lands_in_10_slash_8() { + let subnet = derive_ip_subnet("a3f4"); + assert!(subnet.starts_with("10.")); + assert!(subnet.ends_with(".0.0/16")); + } + + #[test] + fn dm_thin_pool_name_embeds_instance_id() { + let cfg = config_with_id("a3f4"); + assert_eq!(cfg.dm_thin_pool_name(), "ember-a3f4-pool"); + assert_eq!(cfg.dm_thin_image_prefix(), "ember-a3f4-img-"); + assert_eq!(cfg.dm_thin_vm_prefix(), "ember-a3f4-vm-"); + } + + #[test] + fn tap_prefix_fits_ifnamsiz_with_7_hex_vm_id() { + let cfg = config_with_id("ffff"); + let prefix = cfg.tap_prefix(); + assert_eq!(prefix, "emffff-"); + // Linux IFNAMSIZ is 16 with NUL; usable budget is 15. The full + // device name is `prefix + 7 hex chars` = 14 chars. + assert!(prefix.len() + 7 <= 15); + } + + #[test] + fn iptables_comment_tags_install() { + let cfg = config_with_id("a3f4"); + assert_eq!(cfg.iptables_comment(), "ember:a3f4"); + } } diff --git a/crates/ember-core/src/network/ip.rs b/crates/ember-core/src/network/ip.rs index e9da5bc..8581d31 100644 --- a/crates/ember-core/src/network/ip.rs +++ b/crates/ember-core/src/network/ip.rs @@ -4,8 +4,10 @@ //! Allocations are tracked in `allocations.json` via the state store //! with flock-based locking for concurrent safety. //! -//! With the default /16 subnet (10.100.0.0/16), this supports ~16,384 -//! concurrent VMs. +//! The base subnet is set per-installation in `GlobalConfig::ip_subnet` +//! (derived at `ember init` from the instance id, overridable via +//! `--ip-subnet`). With a /16, each install supports ~16,384 concurrent +//! VMs. use std::collections::HashMap; use std::net::Ipv4Addr; @@ -37,9 +39,6 @@ pub struct IpAllocation { pub netmask: String, } -/// Default base subnet when none is configured. -pub const DEFAULT_SUBNET: &str = "10.100.0.0/16"; - /// Netmask for a /30 subnet. const NETMASK_30: &str = "255.255.255.252"; diff --git a/crates/ember-core/src/state/vm.rs b/crates/ember-core/src/state/vm.rs index 706269a..2ea3f52 100644 --- a/crates/ember-core/src/state/vm.rs +++ b/crates/ember-core/src/state/vm.rs @@ -109,7 +109,7 @@ pub struct VmMetadata { #[serde(default)] pub boot_args: Option, /// Network subnet for IP allocation (e.g., "10.100.0.0/16"). - /// Defaults to [`network::ip::DEFAULT_SUBNET`] when not set. + /// Defaults to [`crate::config::GlobalConfig::ip_subnet`] when not set. #[serde(default)] pub subnet: Option, /// Network configuration, if networking is set up. diff --git a/crates/ember-linux/src/dm_thin.rs b/crates/ember-linux/src/dm_thin.rs index ee9e916..9a6040a 100644 --- a/crates/ember-linux/src/dm_thin.rs +++ b/crates/ember-linux/src/dm_thin.rs @@ -1,11 +1,12 @@ //! Linux device-mapper thin provisioning backend. //! //! Thin pools provide block-level copy-on-write storage. A single -//! [`pool::POOL_NAME`] pool aggregates two backing devices (metadata and -//! data) and exposes any number of independent thin volumes addressed by -//! 64-bit numeric IDs. Snapshots and clones are the same primitive -//! ([`thin::create_snap`]) — snapshotting a thin volume produces another -//! thin volume that shares blocks until divergence. +//! per-installation pool aggregates two backing devices (metadata and +//! data) and exposes any number of independent thin volumes addressed +//! by 64-bit numeric IDs. The pool name comes from +//! `GlobalConfig::dm_thin_pool_name()`. Snapshots and clones are the +//! same primitive ([`thin::create_snap`]) — snapshotting a thin volume +//! produces another thin volume that shares blocks until divergence. //! //! See `docs/DM-THIN-SPEC.md` for the full design. diff --git a/crates/ember-linux/src/dm_thin/pool.rs b/crates/ember-linux/src/dm_thin/pool.rs index 28cda33..7188560 100644 --- a/crates/ember-linux/src/dm_thin/pool.rs +++ b/crates/ember-linux/src/dm_thin/pool.rs @@ -2,16 +2,15 @@ //! //! A thin pool is the kernel-side container holding metadata + data //! devices and exposing thin volumes as snapshot-capable block devices. -//! Ember runs a single named pool ([`POOL_NAME`]) per installation. +//! Ember runs one named pool per installation; the pool name is +//! derived from `GlobalConfig::dm_thin_pool_name()` so two installs on +//! the same host don't share a pool. use std::path::{Path, PathBuf}; use std::process::Command; use ember_core::error::{Error, Result}; -/// Device-mapper name of the singleton thin pool used by ember. -pub const POOL_NAME: &str = "ember-pool"; - /// Default pool block size in 512-byte sectors (= 64 KiB). /// /// Permanent at pool creation. Smaller blocks improve sharing across diff --git a/crates/ember-linux/src/dm_thin/thin.rs b/crates/ember-linux/src/dm_thin/thin.rs index ff62ee3..e78540f 100644 --- a/crates/ember-linux/src/dm_thin/thin.rs +++ b/crates/ember-linux/src/dm_thin/thin.rs @@ -15,11 +15,6 @@ use ember_core::error::{Error, Result}; use super::{is_already_exists, pool}; -/// Device-mapper name prefix for image base volumes. -pub const IMAGE_PREFIX: &str = "ember-img-"; -/// Device-mapper name prefix for VM disks. -pub const VM_PREFIX: &str = "ember-vm-"; - /// Maximum thin device id accepted by the kernel. /// /// `drivers/md/dm-thin.c` enforces `dev_id <= (1 << 24) - 1`: @@ -187,26 +182,23 @@ pub fn sanitize_dm_name(name: &str) -> String { .collect() } -/// Device-mapper name for a VM volume. -pub fn vm_dm_name(vm_name: &str) -> String { - format!("{VM_PREFIX}{}", sanitize_dm_name(vm_name)) +/// Device-mapper name for a VM volume. `vm_prefix` comes from +/// [`GlobalConfig::dm_thin_vm_prefix`]. +pub fn vm_dm_name(vm_prefix: &str, vm_name: &str) -> String { + format!("{vm_prefix}{}", sanitize_dm_name(vm_name)) } -/// Device-mapper name for an image base volume. -pub fn image_dm_name(image_name: &str) -> String { - format!("{IMAGE_PREFIX}{}", sanitize_dm_name(image_name)) +/// Device-mapper name for an image base volume. `image_prefix` comes +/// from [`GlobalConfig::dm_thin_image_prefix`]. +pub fn image_dm_name(image_prefix: &str, image_name: &str) -> String { + format!("{image_prefix}{}", sanitize_dm_name(image_name)) } /// Device-mapper name for the temporary staging volume used while /// writing a fresh image into the pool. Held only between /// `create_thin` and the post-`dd` snapshot. -pub fn image_staging_dm_name(image_name: &str) -> String { - format!("{IMAGE_PREFIX}{}-staging", sanitize_dm_name(image_name)) -} - -/// Path that should be passed to Firecracker as `path_on_host`. -pub fn vm_device_path(vm_name: &str) -> PathBuf { - device_path(&vm_dm_name(vm_name)) +pub fn image_staging_dm_name(image_prefix: &str, image_name: &str) -> String { + format!("{image_prefix}{}-staging", sanitize_dm_name(image_name)) } #[cfg(test)] @@ -232,18 +224,21 @@ mod tests { #[test] fn thin_table_shape() { - let t = thin_table("ember-pool", 42, 16_777_216); - assert_eq!(t, "0 16777216 thin /dev/mapper/ember-pool 42"); + let t = thin_table("ember-a3f4-pool", 42, 16_777_216); + assert_eq!(t, "0 16777216 thin /dev/mapper/ember-a3f4-pool 42"); } #[test] fn dm_names() { - assert_eq!(vm_dm_name("myvm"), "ember-vm-myvm"); + assert_eq!(vm_dm_name("ember-a3f4-vm-", "myvm"), "ember-a3f4-vm-myvm"); + assert_eq!( + image_dm_name("ember-a3f4-img-", "library-alpine-latest"), + "ember-a3f4-img-library-alpine-latest" + ); assert_eq!( - image_dm_name("library-alpine-latest"), - "ember-img-library-alpine-latest" + image_staging_dm_name("ember-a3f4-img-", "foo"), + "ember-a3f4-img-foo-staging" ); - assert_eq!(image_staging_dm_name("foo"), "ember-img-foo-staging"); } #[test] diff --git a/crates/ember-linux/src/dm_thin_storage.rs b/crates/ember-linux/src/dm_thin_storage.rs index 51fa08e..6b23829 100644 --- a/crates/ember-linux/src/dm_thin_storage.rs +++ b/crates/ember-linux/src/dm_thin_storage.rs @@ -64,6 +64,16 @@ pub struct DmThinStorage { /// Pool block size in 512-byte sectors. Permanent at pool creation; /// the value here must match what the running pool was created with. block_size_sectors: u32, + /// Per-installation device-mapper pool name, e.g. + /// `ember-a3f4-pool`. Pinned from `GlobalConfig` at construction + /// rather than recomputed at every call site so the backend acts on + /// exactly the pool the persisted config refers to. + pool_name: String, + /// Per-installation prefix for image base volumes + /// (`ember-a3f4-img-`). + image_prefix: String, + /// Per-installation prefix for VM disks (`ember-a3f4-vm-`). + vm_prefix: String, } impl DmThinStorage { @@ -92,6 +102,9 @@ impl DmThinStorage { block_size_sectors: config .dm_thin_block_size .unwrap_or(pool::DEFAULT_BLOCK_SIZE_SECTORS), + pool_name: config.dm_thin_pool_name(), + image_prefix: config.dm_thin_image_prefix(), + vm_prefix: config.dm_thin_vm_prefix(), } } @@ -118,7 +131,7 @@ impl DmThinStorage { /// devices and re-runs `dmsetup create` if the kernel state is gone /// (e.g., after a reboot). fn ensure_pool_active(&self) -> Result<()> { - if dm_device_exists(pool::POOL_NAME)? { + if dm_device_exists(&self.pool_name)? { return Ok(()); } @@ -145,7 +158,7 @@ impl DmThinStorage { let data_sectors = device_sectors(&data_loop)?; pool::create( - pool::POOL_NAME, + &self.pool_name, &metadata_loop, &data_loop, data_sectors, @@ -165,7 +178,7 @@ impl DmThinStorage { if dm_device_exists(dm_name)? { return Ok(thin::device_path(dm_name)); } - thin::activate(dm_name, pool::POOL_NAME, thin_id, size_sectors) + thin::activate(dm_name, &self.pool_name, thin_id, size_sectors) } /// Read a VM's required size in sectors from its metadata. @@ -203,22 +216,22 @@ impl DmThinStorage { /// path for [`PoolMode::OutOfDataSpace`]; destroy paths are also /// not gated since freeing thin ids must work even on a sick pool. fn assert_pool_healthy(&self) -> Result<()> { - let status = pool::status(pool::POOL_NAME)?; + let status = pool::status(&self.pool_name)?; match status.mode { pool::PoolMode::ReadWrite => Ok(()), pool::PoolMode::ReadOnly => Err(Error::Pool(format!( "dm-thin pool '{}' is read-only — run `thin_check` and `thin_repair` to recover", - pool::POOL_NAME + &self.pool_name ))), pool::PoolMode::OutOfDataSpace => Err(Error::Pool(format!( "dm-thin pool '{}' is out of data space ({}/{} blocks used) — run `ember storage grow --size ` to extend it", - pool::POOL_NAME, + &self.pool_name, status.used_data_blocks, status.total_data_blocks, ))), pool::PoolMode::Failed => Err(Error::Pool(format!( "dm-thin pool '{}' has failed — inspect dmesg and `thin_check` the metadata device", - pool::POOL_NAME + &self.pool_name ))), } } @@ -232,6 +245,10 @@ impl StorageBackend for DmThinStorage { pool::ensure_target_loaded()?; + // Pool is named per-installation so two installs on one host + // don't share kernel state. + let pool_name = format!("ember-{}-pool", config.instance_id); + let block_size_sectors = config .dm_thin_block_size .unwrap_or(pool::DEFAULT_BLOCK_SIZE_SECTORS); @@ -311,7 +328,7 @@ impl StorageBackend for DmThinStorage { } }; if let Err(e) = pool::create( - pool::POOL_NAME, + &pool_name, &metadata_loop, &data_loop, data_sectors, @@ -326,8 +343,7 @@ impl StorageBackend for DmThinStorage { } println!( - "dm-thin pool '{}' active ({} data, {} block size).", - pool::POOL_NAME, + "dm-thin pool '{pool_name}' active ({} data, {} block size).", format_bytes(pool_size_bytes), format_bytes((block_size_sectors as u64) * SECTOR_SIZE), ); @@ -344,8 +360,8 @@ impl StorageBackend for DmThinStorage { self.ensure_pool_active()?; self.assert_pool_healthy()?; - let staging_dm = thin::image_staging_dm_name(name); - let final_dm = thin::image_dm_name(name); + let staging_dm = thin::image_staging_dm_name(&self.image_prefix, name); + let final_dm = thin::image_dm_name(&self.image_prefix, name); let size_sectors = (size_mib * 1024 * 1024) / SECTOR_SIZE; // A previous failed run may have left the staging device @@ -358,12 +374,12 @@ impl StorageBackend for DmThinStorage { } // 1. Allocate a fresh staging thin and write the ext4 image. - let staging_id = thin::allocate(pool::POOL_NAME)?; + let staging_id = thin::allocate(&self.pool_name)?; let staging_dev = - match thin::activate(&staging_dm, pool::POOL_NAME, staging_id, size_sectors) { + match thin::activate(&staging_dm, &self.pool_name, staging_id, size_sectors) { Ok(p) => p, Err(e) => { - let _ = thin::delete(pool::POOL_NAME, staging_id); + let _ = thin::delete(&self.pool_name, staging_id); return Err(e); } }; @@ -371,7 +387,7 @@ impl StorageBackend for DmThinStorage { // 2. dd the ext4 image onto the staging device. if let Err(e) = dd_image(image_path, &staging_dev) { let _ = thin::deactivate(&staging_dm); - let _ = thin::delete(pool::POOL_NAME, staging_id); + let _ = thin::delete(&self.pool_name, staging_id); return Err(e); } @@ -379,7 +395,7 @@ impl StorageBackend for DmThinStorage { // the staging device first so the snapshot sees a coherent // metadata commit; resume it on the way out either way. let base_id_result = thin::suspend(&staging_dm).and_then(|()| { - let id = thin::allocate_snap(pool::POOL_NAME, staging_id); + let id = thin::allocate_snap(&self.pool_name, staging_id); let _ = thin::resume(&staging_dm); id }); @@ -387,7 +403,7 @@ impl StorageBackend for DmThinStorage { Ok(id) => id, Err(e) => { let _ = thin::deactivate(&staging_dm); - let _ = thin::delete(pool::POOL_NAME, staging_id); + let _ = thin::delete(&self.pool_name, staging_id); return Err(e); } }; @@ -395,7 +411,7 @@ impl StorageBackend for DmThinStorage { // 4. Drop the staging device + thin id; the base id retains all // of its blocks. let _ = thin::deactivate(&staging_dm); - let _ = thin::delete(pool::POOL_NAME, staging_id); + let _ = thin::delete(&self.pool_name, staging_id); // The base thin is left inactive. Lazy activation creates the // device on first use. Record the would-be path so it can be @@ -411,19 +427,19 @@ impl StorageBackend for DmThinStorage { self.assert_pool_healthy()?; let base_id = Self::require_image_thin_id(image)?; - let dm_name = thin::vm_dm_name(vm_name); + let dm_name = thin::vm_dm_name(&self.vm_prefix, vm_name); // The VM's virtual size matches the image's size at clone time; // resize to a larger disk happens in a subsequent `resize` call. let size_sectors = (image.size_mib * 1024 * 1024) / SECTOR_SIZE; - let vm_id = thin::allocate_snap(pool::POOL_NAME, base_id)?; - match thin::activate(&dm_name, pool::POOL_NAME, vm_id, size_sectors) { + let vm_id = thin::allocate_snap(&self.pool_name, base_id)?; + match thin::activate(&dm_name, &self.pool_name, vm_id, size_sectors) { Ok(disk_path) => Ok(VolumeHandle { disk_path, thin_id: Some(vm_id), }), Err(e) => { - let _ = thin::delete(pool::POOL_NAME, vm_id); + let _ = thin::delete(&self.pool_name, vm_id); Err(e) } } @@ -433,14 +449,14 @@ impl StorageBackend for DmThinStorage { self.ensure_pool_active()?; self.assert_pool_healthy()?; let vm_id = Self::require_vm_thin_id(vm)?; - let dm_name = thin::vm_dm_name(&vm.name); + let dm_name = thin::vm_dm_name(&self.vm_prefix, &vm.name); let new_sectors = new_size.bytes() / SECTOR_SIZE; // Activate (lazy) so we have a device to reload. let current_sectors = Self::vm_size_sectors(vm); let dev_path = self.ensure_thin_active(&dm_name, vm_id, current_sectors)?; - thin::reload_size(&dm_name, pool::POOL_NAME, vm_id, new_sectors)?; + thin::reload_size(&dm_name, &self.pool_name, vm_id, new_sectors)?; zvol::wait_for_device(&dev_path)?; e2fsck(&dev_path)?; resize2fs(&dev_path)?; @@ -451,12 +467,12 @@ impl StorageBackend for DmThinStorage { // Best-effort: deactivate first, then free the thin id. Either // step may already be done by an earlier failure path. let _ = self.ensure_pool_active(); - let dm_name = thin::vm_dm_name(&vm.name); + let dm_name = thin::vm_dm_name(&self.vm_prefix, &vm.name); if let Ok(true) = dm_device_exists(&dm_name) { let _ = thin::deactivate(&dm_name); } if let Some(id) = vm.thin_id { - let _ = thin::delete(pool::POOL_NAME, id); + let _ = thin::delete(&self.pool_name, id); } Ok(()) } @@ -466,12 +482,12 @@ impl StorageBackend for DmThinStorage { // safe even when VMs still have clones — they keep their own // thin ids and stay readable. `force` doesn't change behavior. let _ = self.ensure_pool_active(); - let dm_name = thin::image_dm_name(&image.local_name); + let dm_name = thin::image_dm_name(&self.image_prefix, &image.local_name); if let Ok(true) = dm_device_exists(&dm_name) { let _ = thin::deactivate(&dm_name); } if let Some(id) = image.thin_id { - let _ = thin::delete(pool::POOL_NAME, id); + let _ = thin::delete(&self.pool_name, id); } Ok(()) } @@ -483,7 +499,7 @@ impl StorageBackend for DmThinStorage { // path that resolves to ENOENT. self.ensure_pool_active()?; let thin_id = Self::require_vm_thin_id(vm)?; - let dm_name = thin::vm_dm_name(&vm.name); + let dm_name = thin::vm_dm_name(&self.vm_prefix, &vm.name); let size_sectors = Self::vm_size_sectors(vm); self.ensure_thin_active(&dm_name, thin_id, size_sectors) } @@ -492,17 +508,17 @@ impl StorageBackend for DmThinStorage { self.ensure_pool_active()?; self.assert_pool_healthy()?; let source_id = Self::require_vm_thin_id(source)?; - let dm_name = thin::vm_dm_name(target_vm); + let dm_name = thin::vm_dm_name(&self.vm_prefix, target_vm); let size_sectors = Self::vm_size_sectors(source); - let fork_id = thin::allocate_snap(pool::POOL_NAME, source_id)?; - match thin::activate(&dm_name, pool::POOL_NAME, fork_id, size_sectors) { + let fork_id = thin::allocate_snap(&self.pool_name, source_id)?; + match thin::activate(&dm_name, &self.pool_name, fork_id, size_sectors) { Ok(disk_path) => Ok(VolumeHandle { disk_path, thin_id: Some(fork_id), }), Err(e) => { - let _ = thin::delete(pool::POOL_NAME, fork_id); + let _ = thin::delete(&self.pool_name, fork_id); Err(e) } } @@ -520,16 +536,17 @@ impl StorageBackend for DmThinStorage { } fn deinit(&self, purge: bool) -> Result<()> { - // 1. Deactivate every ember-managed thin volume so the pool - // can be removed cleanly. - for prefix in [thin::IMAGE_PREFIX, thin::VM_PREFIX] { + // 1. Deactivate every thin volume that belongs to *this* + // installation so the pool can be removed cleanly. Other + // ember installs use distinct prefixes and stay untouched. + for prefix in [&self.image_prefix, &self.vm_prefix] { for name in pool::list_with_prefix(prefix)? { let _ = thin::deactivate(&name); } } // 2. Drop the pool itself (if active). - if dm_device_exists(pool::POOL_NAME)? { - pool::remove(pool::POOL_NAME)?; + if dm_device_exists(&self.pool_name)? { + pool::remove(&self.pool_name)?; } // 3. Detach the loop devices, if any. let metadata_path = self.metadata_file(); @@ -553,7 +570,7 @@ impl StorageBackend for DmThinStorage { let _ = fs::remove_dir(&self.storage_path); } } - println!("dm-thin pool '{}' torn down.", pool::POOL_NAME); + println!("dm-thin pool '{}' torn down.", &self.pool_name); Ok(()) } @@ -597,7 +614,7 @@ impl StorageBackend for DmThinStorage { let data_sectors = device_sectors(&data_loop)?; pool::reload( - pool::POOL_NAME, + &self.pool_name, &metadata_loop, &data_loop, data_sectors, diff --git a/crates/ember-linux/src/network.rs b/crates/ember-linux/src/network.rs index 9d7f130..37f36ea 100644 --- a/crates/ember-linux/src/network.rs +++ b/crates/ember-linux/src/network.rs @@ -5,14 +5,24 @@ pub mod nat; pub mod tap; pub mod wan; +use ember_core::config::GlobalConfig; use ember_core::state::store::StateStore; use ember_core::state::vm::NetworkInfo; /// Best-effort cleanup of networking resources for a VM (Linux only). -pub fn cleanup(store: &StateStore, vm_name: &str, net_info: &NetworkInfo) { +/// +/// The iptables comment comes from `config.iptables_comment()` so the +/// `-D` calls only match this installation's rules even when another +/// ember install on the same host has rules for the same TAP/IP. +pub fn cleanup(store: &StateStore, config: &GlobalConfig, vm_name: &str, net_info: &NetworkInfo) { let wan_iface = net_info.wan_iface.clone().or_else(|| wan::detect().ok()); if let Some(wan_iface) = wan_iface { - let _ = nat::remove_rules(&net_info.tap_device, &net_info.guest_ip, &wan_iface); + let _ = nat::remove_rules( + &net_info.tap_device, + &net_info.guest_ip, + &wan_iface, + &config.iptables_comment(), + ); } let _ = tap::delete(&net_info.tap_device); let _ = ip::release(store, vm_name); diff --git a/crates/ember-linux/src/network/nat.rs b/crates/ember-linux/src/network/nat.rs index 1ea21b5..2d29744 100644 --- a/crates/ember-linux/src/network/nat.rs +++ b/crates/ember-linux/src/network/nat.rs @@ -19,11 +19,16 @@ use ember_core::error::{Error, Result}; /// through the host's WAN interface via masquerading (SNAT): /// /// ```text -/// -t nat -A POSTROUTING -s /32 -o -j MASQUERADE -/// -A FORWARD -i -o -j ACCEPT -/// -A FORWARD -i -o -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT +/// -t nat -A POSTROUTING -s /32 -o -m comment --comment -j MASQUERADE +/// -A FORWARD -i -o -m comment --comment -j ACCEPT +/// -A FORWARD -i -o -m conntrack --ctstate RELATED,ESTABLISHED -m comment --comment -j ACCEPT /// ``` -pub fn add_rules(tap_device: &str, guest_ip: &str, wan_iface: &str) -> Result<()> { +/// +/// `comment` is a per-installation tag (e.g. `ember:a3f4`) embedded in +/// every rule via the `comment` match. It lets cleanup scope deletions +/// to this installation's rules and lets users grep `iptables-save` for +/// "rules ember put here". +pub fn add_rules(tap_device: &str, guest_ip: &str, wan_iface: &str, comment: &str) -> Result<()> { let guest_cidr = format!("{guest_ip}/32"); // 1. NAT masquerade for outbound guest traffic. @@ -36,13 +41,28 @@ pub fn add_rules(tap_device: &str, guest_ip: &str, wan_iface: &str) -> Result<() &guest_cidr, "-o", wan_iface, + "-m", + "comment", + "--comment", + comment, "-j", "MASQUERADE", ])?; // 2. Allow forwarding from TAP to WAN. iptables(&[ - "-A", "FORWARD", "-i", tap_device, "-o", wan_iface, "-j", "ACCEPT", + "-A", + "FORWARD", + "-i", + tap_device, + "-o", + wan_iface, + "-m", + "comment", + "--comment", + comment, + "-j", + "ACCEPT", ])?; // 3. Allow established/related return traffic from WAN to TAP. @@ -57,6 +77,10 @@ pub fn add_rules(tap_device: &str, guest_ip: &str, wan_iface: &str) -> Result<() "conntrack", "--ctstate", "RELATED,ESTABLISHED", + "-m", + "comment", + "--comment", + comment, "-j", "ACCEPT", ])?; @@ -67,8 +91,17 @@ pub fn add_rules(tap_device: &str, guest_ip: &str, wan_iface: &str) -> Result<() /// Remove iptables NAT and forwarding rules for a VM. /// /// Mirrors [`add_rules`] but uses `-D` (delete) instead of `-A` (append). -/// Idempotent — silently ignores errors when rules don't exist. -pub fn remove_rules(tap_device: &str, guest_ip: &str, wan_iface: &str) -> Result<()> { +/// Idempotent — silently ignores errors when rules don't exist. The +/// `comment` argument must match the value passed to +/// [`add_rules`]; iptables compares the full rule including the comment +/// match, so a wrong tag turns the delete into a no-op rather than +/// removing another install's rule. +pub fn remove_rules( + tap_device: &str, + guest_ip: &str, + wan_iface: &str, + comment: &str, +) -> Result<()> { let guest_cidr = format!("{guest_ip}/32"); // Same rules as add_rules, but with -D to delete. @@ -82,12 +115,27 @@ pub fn remove_rules(tap_device: &str, guest_ip: &str, wan_iface: &str) -> Result &guest_cidr, "-o", wan_iface, + "-m", + "comment", + "--comment", + comment, "-j", "MASQUERADE", ]); let _ = iptables_delete(&[ - "-D", "FORWARD", "-i", tap_device, "-o", wan_iface, "-j", "ACCEPT", + "-D", + "FORWARD", + "-i", + tap_device, + "-o", + wan_iface, + "-m", + "comment", + "--comment", + comment, + "-j", + "ACCEPT", ]); let _ = iptables_delete(&[ @@ -101,6 +149,10 @@ pub fn remove_rules(tap_device: &str, guest_ip: &str, wan_iface: &str) -> Result "conntrack", "--ctstate", "RELATED,ESTABLISHED", + "-m", + "comment", + "--comment", + comment, "-j", "ACCEPT", ]); diff --git a/crates/ember-linux/src/network/tap.rs b/crates/ember-linux/src/network/tap.rs index f5a3bf2..c6e6c6c 100644 --- a/crates/ember-linux/src/network/tap.rs +++ b/crates/ember-linux/src/network/tap.rs @@ -1,8 +1,11 @@ //! TAP device creation and cleanup via ioctl. //! -//! Each Firecracker VM gets a dedicated TAP device (`em-`) for -//! its point-to-point network link to the host. This module handles -//! creating and deleting those devices using the Linux TUN/TAP driver. +//! Each Firecracker VM gets a dedicated TAP device named +//! `em-` for its point-to-point network link +//! to the host. The `` segment scopes devices to one ember +//! installation so two installs on the same host don't see (or delete) +//! each other's TAPs. This module handles creating and deleting those +//! devices using the Linux TUN/TAP driver. use std::ffi::CString; use std::fs::OpenOptions; @@ -149,11 +152,13 @@ pub fn delete(name: &str) -> Result<()> { Ok(()) } -/// List all ember TAP devices (names starting with `em-`) on the system. +/// List TAP devices on the system whose name starts with `prefix`. /// -/// Parses the output of `ip -o link show type tun` to find persistent -/// TAP devices created by ember. Returns just the device names. -pub fn list_ember_devices() -> Result> { +/// Parses the output of `ip -o link show type tun`. Pass the +/// per-installation TAP prefix from +/// [`GlobalConfig::tap_prefix`](ember_core::config::GlobalConfig::tap_prefix) +/// so reconciliation only sees devices belonging to *this* install. +pub fn list_devices_with_prefix(prefix: &str) -> Result> { let output = Command::new("ip") .args(["-o", "link", "show", "type", "tun"]) .output() @@ -170,12 +175,12 @@ pub fn list_ember_devices() -> Result> { let stdout = String::from_utf8_lossy(&output.stdout); let mut devices = Vec::new(); for line in stdout.lines() { - // Format: "3: em-abc1234: <...>" + // Format: "3: ema3f4-abc1234: <...>" // Split on ':' and take the second field (device name), trimmed. let parts: Vec<&str> = line.splitn(3, ':').collect(); if parts.len() >= 2 { let name = parts[1].trim(); - if name.starts_with("em-") { + if name.starts_with(prefix) { devices.push(name.to_string()); } } @@ -186,11 +191,13 @@ pub fn list_ember_devices() -> Result> { /// Generate the TAP device name for a VM from its UUID. /// -/// Format: `em-`. This fits within the -/// Linux `IFNAMSIZ` limit of 15 characters (3 prefix + 7 hex = 10). -pub fn device_name(vm_id: &uuid::Uuid) -> String { +/// Format: ``. With the default +/// 4-char `instance_id`, the prefix is `em-` (7 chars) and the +/// full name is 14 chars — within Linux's `IFNAMSIZ - 1 = 15` budget +/// with one char to spare. +pub fn device_name(tap_prefix: &str, vm_id: &uuid::Uuid) -> String { let hex = vm_id.as_simple().to_string(); - format!("em-{}", &hex[..7]) + format!("{tap_prefix}{}", &hex[..7]) } #[cfg(test)] @@ -201,16 +208,17 @@ mod tests { #[test] fn device_name_format() { let id = Uuid::parse_str("550e8400-e29b-41d4-a716-446655440000").unwrap(); - let name = device_name(&id); - assert_eq!(name, "em-550e840"); + let name = device_name("ema3f4-", &id); + assert_eq!(name, "ema3f4-550e840"); assert!(name.len() < libc::IFNAMSIZ); } #[test] fn device_name_fits_ifnamsiz() { - // Any UUID should produce a name < IFNAMSIZ (16). + // 4-char instance id + 7-hex VM id + dashes/`em` = 14 chars, + // one byte under IFNAMSIZ - 1. let id = Uuid::new_v4(); - let name = device_name(&id); + let name = device_name("emffff-", &id); assert!(name.len() < libc::IFNAMSIZ); } diff --git a/crates/ember-linux/src/network_backend.rs b/crates/ember-linux/src/network_backend.rs index 5d59577..f3897f0 100644 --- a/crates/ember-linux/src/network_backend.rs +++ b/crates/ember-linux/src/network_backend.rs @@ -38,12 +38,14 @@ impl NetworkBackend for LinuxNetwork { None => network::wan::detect()?, }; - // Allocate a /30 IP block for this VM. - let subnet = vm.subnet.as_deref().unwrap_or(network::ip::DEFAULT_SUBNET); + // Allocate a /30 IP block for this VM. The VM-level override + // (`vm.subnet`) wins; otherwise inherit the per-installation + // default that `ember init` derived from the instance id. + let subnet = vm.subnet.as_deref().unwrap_or(&config.ip_subnet); let allocation = network::ip::allocate(&self.store, subnet, &vm.name)?; // Create TAP device. - let tap_name = network::tap::device_name(&vm.id); + let tap_name = network::tap::device_name(&config.tap_prefix(), &vm.id); let host_ip_cidr = format!("{}/30", allocation.host_ip); if let Err(e) = network::tap::create(&tap_name, &host_ip_cidr) { // Clean up IP allocation on failure. @@ -58,8 +60,12 @@ impl NetworkBackend for LinuxNetwork { return Err(e); } - // Add iptables NAT/forwarding rules. - if let Err(e) = network::nat::add_rules(&tap_name, &allocation.guest_ip, &wan_iface) { + // Add iptables NAT/forwarding rules tagged with this install's + // comment so cleanup can scope to *this* installation. + let comment = config.iptables_comment(); + if let Err(e) = + network::nat::add_rules(&tap_name, &allocation.guest_ip, &wan_iface, &comment) + { let _ = network::tap::delete(&tap_name); let _ = network::ip::release(&self.store, &vm.name); return Err(e); @@ -79,9 +85,9 @@ impl NetworkBackend for LinuxNetwork { /// /// Best-effort cleanup — continues even if individual steps fail, since /// this is called during stop/delete where partial cleanup is acceptable. - fn teardown(&self, vm: &VmMetadata) -> Result<()> { + fn teardown(&self, vm: &VmMetadata, config: &GlobalConfig) -> Result<()> { if let Some(ref net_info) = vm.network { - network::cleanup(&self.store, &vm.name, net_info); + network::cleanup(&self.store, config, &vm.name, net_info); } Ok(()) } diff --git a/crates/ember-linux/src/platform.rs b/crates/ember-linux/src/platform.rs index f773495..1749fbe 100644 --- a/crates/ember-linux/src/platform.rs +++ b/crates/ember-linux/src/platform.rs @@ -6,8 +6,6 @@ use ember_core::error::Result; use ember_core::image::registry::ImageEntry; use ember_core::state::vm::VmMetadata; -use crate::dm_thin::pool; - pub struct LinuxPlatform; fn linux_install_hint(name: &str) -> String { @@ -82,7 +80,7 @@ impl Platform for LinuxPlatform { ("Dataset", format!("{}/{}", config.pool, config.dataset)), ], StorageKind::DmThin => { - let mut rows = vec![("dm-thin pool", pool::POOL_NAME.to_string())]; + let mut rows = vec![("dm-thin pool", config.dm_thin_pool_name())]; if let Some(ref path) = config.storage_path { rows.push(("Storage path", path.display().to_string())); } diff --git a/crates/ember-linux/src/reconcile.rs b/crates/ember-linux/src/reconcile.rs index 36a86b0..aad2e08 100644 --- a/crates/ember-linux/src/reconcile.rs +++ b/crates/ember-linux/src/reconcile.rs @@ -6,8 +6,10 @@ //! is still alive. If dead, mark the VM as Stopped and clean up its //! network resources (TAP device, iptables rules, IP allocation). //! -//! 2. Find orphaned `em-*` TAP devices (present on the system but not -//! associated with any running VM) and delete them. +//! 2. Find orphaned TAP devices belonging to *this* installation +//! (matched against `GlobalConfig::tap_prefix()`) and delete them. +//! Other ember installs use distinct prefixes, so reconciliation +//! here never touches their devices. //! //! All operations are best-effort: errors are logged but never propagated, //! since reconciliation should not block normal CLI operation. @@ -17,6 +19,7 @@ use std::path::Path; use crate::firecracker; use crate::network; +use ember_core::config::GlobalConfig; use ember_core::state::store::StateStore; use ember_core::state::vm::{self, VmStatus}; @@ -30,6 +33,12 @@ pub fn run(state_dir: &Path) { None => return, // State dir doesn't exist yet (pre-init), nothing to reconcile. }; + // Need the global config for the per-installation TAP prefix and + // iptables comment. If it's missing or unreadable, reconcile + // per-VM state but skip the prefix-based TAP sweep — without a + // prefix we'd risk deleting another install's devices. + let config: Option = store.read_optional(&store.config_path()).ok().flatten(); + let vms = match vm::list(&store) { Ok(vms) => vms, Err(e) => { @@ -76,14 +85,22 @@ pub fn run(state_dir: &Path) { metadata.name ); if let Some(ref net_info) = metadata.network { - cleanup_network(&store, &metadata.name, net_info); + if let Some(ref cfg) = config { + cleanup_network(&store, cfg, &metadata.name, net_info); + } } mark_stopped(&store, &mut metadata); } } - // Phase 2: Clean up orphaned TAP devices. - let system_devices = match network::tap::list_ember_devices() { + // Phase 2: Clean up orphaned TAP devices belonging to this install. + // Without a config we have no way to scope the listing safely, so + // skip — leaving an orphan is preferable to deleting a foreign one. + let Some(cfg) = config else { + return; + }; + let prefix = cfg.tap_prefix(); + let system_devices = match network::tap::list_devices_with_prefix(&prefix) { Ok(devs) => devs, Err(e) => { eprintln!("Warning: failed to list TAP devices: {e}"); @@ -113,6 +130,11 @@ fn mark_stopped(store: &StateStore, metadata: &mut vm::VmMetadata) { } /// Best-effort network cleanup for a dead VM. -fn cleanup_network(store: &StateStore, vm_name: &str, net_info: &vm::NetworkInfo) { - network::cleanup(store, vm_name, net_info); +fn cleanup_network( + store: &StateStore, + config: &GlobalConfig, + vm_name: &str, + net_info: &vm::NetworkInfo, +) { + network::cleanup(store, config, vm_name, net_info); } diff --git a/crates/ember-macos/src/network.rs b/crates/ember-macos/src/network.rs index 8bd7243..bd83621 100644 --- a/crates/ember-macos/src/network.rs +++ b/crates/ember-macos/src/network.rs @@ -57,7 +57,7 @@ impl NetworkBackend for MacosNetwork { }) } - fn teardown(&self, vm: &VmMetadata) -> Result<()> { + fn teardown(&self, vm: &VmMetadata, _config: &GlobalConfig) -> Result<()> { let _ = network::ip::release(&self.store, &vm.name); Ok(()) } diff --git a/src/cli/init.rs b/src/cli/init.rs index a3039d1..e0f72ba 100644 --- a/src/cli/init.rs +++ b/src/cli/init.rs @@ -4,7 +4,9 @@ use clap::Args; use crate::backend::{init_storage, CurrentPlatform, InitConfig, Platform}; use ember_core::config::size::ByteSize; -use ember_core::config::{DmThinMode, GlobalConfig, StorageKind}; +use ember_core::config::{ + derive_instance_id, derive_ip_subnet, DmThinMode, GlobalConfig, StorageKind, +}; use ember_core::state::store::StateStore; /// dm-thin pool block size (in 512-byte sectors) used when the user does @@ -89,6 +91,31 @@ pub struct InitArgs { /// WAN interface for NAT (auto-detected if not specified) #[arg(long)] pub wan_iface: Option, + + /// Per-installation namespace, embedded in dm-thin pool name, TAP + /// devices, and iptables rules so two ember installations on the + /// same host don't clash. 4 hex chars; auto-derived from a hash of + /// the state directory when omitted. + #[arg(long, value_parser = parse_instance_id)] + pub instance_id: Option, + + /// IPv4 base subnet handed out as /30 links to VMs (e.g. + /// `10.42.0.0/16`). Defaults to a `/16` slot inside `10.0.0.0/8` + /// derived from the instance id, so two installs get + /// non-overlapping ranges automatically. + #[arg(long)] + pub ip_subnet: Option, +} + +/// Validate `--instance-id`: 4 lowercase hex chars (uppercase is folded). +fn parse_instance_id(s: &str) -> Result { + let lower = s.to_ascii_lowercase(); + if lower.len() != 4 || !lower.chars().all(|c| c.is_ascii_hexdigit()) { + return Err(format!( + "--instance-id must be exactly 4 hex chars (got {s:?})" + )); + } + Ok(lower) } pub fn run(args: &InitArgs, state_dir: &Path) -> anyhow::Result<()> { @@ -143,9 +170,22 @@ pub fn run(args: &InitArgs, state_dir: &Path) -> anyhow::Result<()> { _ => None, }; + // Resolve instance_id and ip_subnet up-front so InitConfig and the + // persisted GlobalConfig agree. dm-thin in particular needs the + // instance id during init to name the kernel pool. + let instance_id = args + .instance_id + .clone() + .unwrap_or_else(|| derive_instance_id(state_dir)); + let ip_subnet = args + .ip_subnet + .clone() + .unwrap_or_else(|| derive_ip_subnet(&instance_id)); + let init_config = InitConfig { storage_backend: args.storage, state_dir: state_dir.to_path_buf(), + instance_id: instance_id.clone(), pool: args.pool.clone(), dataset: args.dataset.clone(), device: args.device.clone(), @@ -184,12 +224,16 @@ pub fn run(args: &InitArgs, state_dir: &Path) -> anyhow::Result<()> { kernel_path, wan_iface, state_dir: state_dir.to_path_buf(), + instance_id: instance_id.clone(), + ip_subnet: ip_subnet.clone(), storage_path, dm_thin_block_size: resolved_block_size, dm_thin_mode: resolved_dm_thin_mode, }; store.write(&store.config_path(), &config)?; println!("Configuration written to {}", store.config_path().display()); + println!("Instance id: {instance_id}"); + println!("VM IP subnet: {ip_subnet}"); println!("\nember initialized successfully."); Ok(()) @@ -209,6 +253,8 @@ mod tests { kernel_path: None, wan_iface: None, state_dir: PathBuf::default(), + instance_id: "abcd".to_string(), + ip_subnet: "10.100.0.0/16".to_string(), storage_path: None, dm_thin_block_size: None, dm_thin_mode: None, diff --git a/src/cli/vm.rs b/src/cli/vm.rs index 19f4320..8f421ad 100644 --- a/src/cli/vm.rs +++ b/src/cli/vm.rs @@ -761,13 +761,14 @@ fn start(args: &StartArgs, state_dir: &Path) -> anyhow::Result<()> { let net = Network::new(StateStore::new(state_dir.to_path_buf())); let meta_name = metadata.name.clone(); let net_info_clone = net_info.clone(); + let config_clone = config.clone(); rollback.push("network", move || { let teardown_meta = VmMetadata { name: meta_name, network: Some(net_info_clone), ..VmMetadata::default_for_teardown() }; - let _ = net.teardown(&teardown_meta); + let _ = net.teardown(&teardown_meta, &config_clone); }); } @@ -847,7 +848,8 @@ fn stop(args: &StopArgs, state_dir: &Path) -> anyhow::Result<()> { // Clean up networking via the backend. let net_backend = Network::new(store.clone()); - let _ = net_backend.teardown(&metadata); + let config: GlobalConfig = store.read(&store.config_path())?; + let _ = net_backend.teardown(&metadata, &config); // Update metadata. metadata.status = VmStatus::Stopped; @@ -1128,15 +1130,19 @@ pub fn force_delete_vm(store: &StateStore, metadata: &VmMetadata) -> anyhow::Res } } + // Read the persisted config once and reuse it for both network + // teardown (needs the per-installation iptables comment) and + // storage teardown. + let config: GlobalConfig = store.read(&store.config_path())?; + // Clean up networking via the backend. let net_backend = Network::new(store.clone()); - let _ = net_backend.teardown(metadata); + let _ = net_backend.teardown(metadata, &config); // Platform-specific post-delete cleanup (e.g. udevadm settle on Linux). CurrentPlatform::post_delete_cleanup(); // Destroy storage via the backend. - let config: GlobalConfig = store.read(&store.config_path())?; let storage = create_storage(&config); println!("Destroying storage for VM '{}'...", metadata.name); diff --git a/tests/dm_thin.rs b/tests/dm_thin.rs index a3c9476..aa6564a 100644 --- a/tests/dm_thin.rs +++ b/tests/dm_thin.rs @@ -39,7 +39,8 @@ fn dm_thin_init_and_deinit_round_trip() { state_dir: state_dir.clone(), }; - // Init. + // Init. Pin the instance id so the pool name we assert on + // matches what `ember init` actually creates. let output = common::ember(&[ "--state-dir", state_dir.to_str().unwrap(), @@ -50,6 +51,8 @@ fn dm_thin_init_and_deinit_round_trip() { storage_path.to_str().unwrap(), "--size", "200M", + "--instance-id", + "dead", ]); let stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); @@ -58,8 +61,8 @@ fn dm_thin_init_and_deinit_round_trip() { "init failed.\nstdout: {stdout}\nstderr: {stderr}" ); assert!( - Path::new("/dev/mapper/ember-pool").exists(), - "ember-pool should be active after init" + Path::new("/dev/mapper/ember-dead-pool").exists(), + "ember-dead-pool should be active after init" ); assert!(storage_path.join("metadata.img").exists()); assert!(storage_path.join("data.img").exists()); @@ -78,8 +81,8 @@ fn dm_thin_init_and_deinit_round_trip() { "deinit failed.\nstdout: {stdout}\nstderr: {stderr}" ); assert!( - !Path::new("/dev/mapper/ember-pool").exists(), - "ember-pool should be torn down after deinit" + !Path::new("/dev/mapper/ember-dead-pool").exists(), + "ember-dead-pool should be torn down after deinit" ); assert!(!storage_path.join("metadata.img").exists()); assert!(!storage_path.join("data.img").exists()); diff --git a/tests/vm.rs b/tests/vm.rs index 7b3e2ed..6576d6c 100644 --- a/tests/vm.rs +++ b/tests/vm.rs @@ -750,8 +750,8 @@ fn networking_ssh_and_internet() { .expect("expected host_ip string"); assert!( - tap_device.starts_with("em-"), - "TAP device should start with 'em-', got: {tap_device}" + tap_device.starts_with("em") && tap_device.contains('-'), + "TAP device should match em-, got: {tap_device}" ); assert!(!guest_ip.is_empty(), "guest_ip should not be empty"); assert!(!host_ip.is_empty(), "host_ip should not be empty"); From 1596e7fc0dc43bae288f93ed371c24690a85f00d Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 3 May 2026 17:37:54 +0000 Subject: [PATCH 02/10] config: keep legacy installs working without re-init MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previous commit's clean-slate naming forced existing installs to deinit + reinit because the config schema gained two required-by- convention fields (instance_id, ip_subnet). That's a worse migration than the problem it solves. Make the accessors back-compat aware instead: when instance_id is empty (the serde default for older configs), return the historical unprefixed names — `ember-pool`, `em-`, `ember-img-`, `ember-vm-`, and an empty iptables comment. ip_subnet's serde default already maps to `10.100.0.0/16`. The iptables comment match is the subtle one. Older binaries added rules without `-m comment` at all; if the upgraded binary then tried to delete the same rule WITH a comment match, iptables would treat it as a different rule and the cleanup would silently no-op. Splice the comment match in only when the comment is non-empty so the byte- exact rule stays matchable on `-D`. Add three integration tests guarding the isolation contract: * `dm_thin_two_installs_dont_interfere`: two installs with distinct --instance-id stand up `ember-aaaa-pool` and `ember-bbbb-pool` side-by-side; deinit on A leaves B's pool intact. * `legacy_config_without_instance_id_keeps_working`: writes a hand-crafted legacy config (no instance_id), runs `ember info` through the binary, asserts the rendered pool name is the unprefixed `ember-pool`. * `reconcile_does_not_touch_other_installs_taps`: creates a TAP matching install A's prefix manually, runs `ember vm list` against install B (which triggers reconcile), verifies A's TAP survives. https://claude.ai/code/session_01XbXjsQa8SRWLzxnuKFzxt6 --- crates/ember-core/src/config.rs | 72 ++++++-- crates/ember-linux/src/network/nat.rs | 237 +++++++++++++----------- tests/dm_thin.rs | 256 ++++++++++++++++++++++++++ 3 files changed, 444 insertions(+), 121 deletions(-) diff --git a/crates/ember-core/src/config.rs b/crates/ember-core/src/config.rs index 99de949..5060010 100644 --- a/crates/ember-core/src/config.rs +++ b/crates/ember-core/src/config.rs @@ -168,33 +168,59 @@ impl GlobalConfig { format!("{}/{}/vms", self.pool, self.dataset) } - /// dm-thin pool name, e.g. `ember-a3f4-pool`. Embedded in every - /// `dmsetup` invocation against the pool. + /// dm-thin pool name. New installs get `ember-{instance_id}-pool`; + /// configs that predate `instance_id` (empty value) keep the + /// legacy unprefixed `ember-pool` so existing kernel state on + /// upgraded hosts is still reachable. pub fn dm_thin_pool_name(&self) -> String { - format!("ember-{}-pool", self.instance_id) + if self.instance_id.is_empty() { + "ember-pool".to_string() + } else { + format!("ember-{}-pool", self.instance_id) + } } - /// Device-mapper name prefix for image base volumes, e.g. - /// `ember-a3f4-img-`. + /// Device-mapper name prefix for image base volumes + /// (`ember-{id}-img-`, or legacy `ember-img-`). pub fn dm_thin_image_prefix(&self) -> String { - format!("ember-{}-img-", self.instance_id) + if self.instance_id.is_empty() { + "ember-img-".to_string() + } else { + format!("ember-{}-img-", self.instance_id) + } } - /// Device-mapper name prefix for VM disks, e.g. `ember-a3f4-vm-`. + /// Device-mapper name prefix for VM disks (`ember-{id}-vm-`, or + /// legacy `ember-vm-`). pub fn dm_thin_vm_prefix(&self) -> String { - format!("ember-{}-vm-", self.instance_id) + if self.instance_id.is_empty() { + "ember-vm-".to_string() + } else { + format!("ember-{}-vm-", self.instance_id) + } } - /// TAP device name prefix, e.g. `ema3f4-`. Bounded so `prefix + - /// 7-hex VM id` fits in Linux's 15-char `IFNAMSIZ - 1` budget. + /// TAP device name prefix (`em{id}-`, or legacy `em-`). Bounded so + /// `prefix + 7-hex VM id` fits in Linux's 15-char `IFNAMSIZ - 1` + /// budget (14 chars with the 4-hex id, 10 chars in legacy mode). pub fn tap_prefix(&self) -> String { - format!("em{}-", self.instance_id) + if self.instance_id.is_empty() { + "em-".to_string() + } else { + format!("em{}-", self.instance_id) + } } - /// Comment string embedded in iptables rules, used to filter this - /// install's rules from any other ember install on the host. + /// Comment string embedded in iptables rules to scope cleanup to + /// this installation. Returns `""` for legacy configs — the old + /// binary added rules without a comment match, so the rules + /// already on the host don't carry one to match against. pub fn iptables_comment(&self) -> String { - format!("ember:{}", self.instance_id) + if self.instance_id.is_empty() { + String::new() + } else { + format!("ember:{}", self.instance_id) + } } } @@ -270,4 +296,22 @@ mod tests { let cfg = config_with_id("a3f4"); assert_eq!(cfg.iptables_comment(), "ember:a3f4"); } + + /// Configs written by older ember binaries don't have `instance_id`. + /// Serde fills the field with the empty-string default; the + /// accessors then have to return the legacy unprefixed names, or + /// we'd lose track of kernel state on every upgrade. + #[test] + fn legacy_config_without_instance_id_uses_unprefixed_names() { + let json = r#"{"pool":"tank","dataset":"ember","kernel_path":null}"#; + let cfg: GlobalConfig = serde_json::from_str(json).unwrap(); + assert_eq!(cfg.instance_id, ""); + assert_eq!(cfg.dm_thin_pool_name(), "ember-pool"); + assert_eq!(cfg.dm_thin_image_prefix(), "ember-img-"); + assert_eq!(cfg.dm_thin_vm_prefix(), "ember-vm-"); + assert_eq!(cfg.tap_prefix(), "em-"); + assert_eq!(cfg.iptables_comment(), ""); + // ip_subnet falls back to the historical default subnet. + assert_eq!(cfg.ip_subnet, "10.100.0.0/16"); + } } diff --git a/crates/ember-linux/src/network/nat.rs b/crates/ember-linux/src/network/nat.rs index 2d29744..c57b268 100644 --- a/crates/ember-linux/src/network/nat.rs +++ b/crates/ember-linux/src/network/nat.rs @@ -19,71 +19,57 @@ use ember_core::error::{Error, Result}; /// through the host's WAN interface via masquerading (SNAT): /// /// ```text -/// -t nat -A POSTROUTING -s /32 -o -m comment --comment -j MASQUERADE -/// -A FORWARD -i -o -m comment --comment -j ACCEPT -/// -A FORWARD -i -o -m conntrack --ctstate RELATED,ESTABLISHED -m comment --comment -j ACCEPT +/// -t nat -A POSTROUTING -s /32 -o [-m comment --comment ] -j MASQUERADE +/// -A FORWARD -i -o [-m comment --comment ] -j ACCEPT +/// -A FORWARD -i -o -m conntrack --ctstate RELATED,ESTABLISHED [-m comment --comment ] -j ACCEPT /// ``` /// /// `comment` is a per-installation tag (e.g. `ember:a3f4`) embedded in /// every rule via the `comment` match. It lets cleanup scope deletions /// to this installation's rules and lets users grep `iptables-save` for -/// "rules ember put here". +/// "rules ember put here". An empty `comment` skips the match entirely +/// so rules added by older ember binaries (which never tagged anything) +/// stay byte-for-byte identical and remain matchable by `remove_rules`. pub fn add_rules(tap_device: &str, guest_ip: &str, wan_iface: &str, comment: &str) -> Result<()> { let guest_cidr = format!("{guest_ip}/32"); - // 1. NAT masquerade for outbound guest traffic. - iptables(&[ - "-t", - "nat", - "-A", - "POSTROUTING", - "-s", - &guest_cidr, - "-o", - wan_iface, - "-m", - "comment", - "--comment", + iptables(&with_comment( + &[ + "-t", + "nat", + "-A", + "POSTROUTING", + "-s", + &guest_cidr, + "-o", + wan_iface, + ], comment, - "-j", - "MASQUERADE", - ])?; - - // 2. Allow forwarding from TAP to WAN. - iptables(&[ - "-A", - "FORWARD", - "-i", - tap_device, - "-o", - wan_iface, - "-m", - "comment", - "--comment", + &["-j", "MASQUERADE"], + ))?; + + iptables(&with_comment( + &["-A", "FORWARD", "-i", tap_device, "-o", wan_iface], comment, - "-j", - "ACCEPT", - ])?; - - // 3. Allow established/related return traffic from WAN to TAP. - iptables(&[ - "-A", - "FORWARD", - "-i", - wan_iface, - "-o", - tap_device, - "-m", - "conntrack", - "--ctstate", - "RELATED,ESTABLISHED", - "-m", - "comment", - "--comment", + &["-j", "ACCEPT"], + ))?; + + iptables(&with_comment( + &[ + "-A", + "FORWARD", + "-i", + wan_iface, + "-o", + tap_device, + "-m", + "conntrack", + "--ctstate", + "RELATED,ESTABLISHED", + ], comment, - "-j", - "ACCEPT", - ])?; + &["-j", "ACCEPT"], + ))?; Ok(()) } @@ -92,10 +78,10 @@ pub fn add_rules(tap_device: &str, guest_ip: &str, wan_iface: &str, comment: &st /// /// Mirrors [`add_rules`] but uses `-D` (delete) instead of `-A` (append). /// Idempotent — silently ignores errors when rules don't exist. The -/// `comment` argument must match the value passed to -/// [`add_rules`]; iptables compares the full rule including the comment -/// match, so a wrong tag turns the delete into a no-op rather than -/// removing another install's rule. +/// `comment` argument must match the value passed to [`add_rules`]; +/// iptables compares the full rule including the comment match, so a +/// wrong tag turns the delete into a no-op rather than removing +/// another install's rule. pub fn remove_rules( tap_device: &str, guest_ip: &str, @@ -104,62 +90,99 @@ pub fn remove_rules( ) -> Result<()> { let guest_cidr = format!("{guest_ip}/32"); - // Same rules as add_rules, but with -D to delete. - // Ignore "does not exist" errors for idempotency. - let _ = iptables_delete(&[ - "-t", - "nat", - "-D", - "POSTROUTING", - "-s", - &guest_cidr, - "-o", - wan_iface, - "-m", - "comment", - "--comment", + let _ = iptables_delete(&with_comment( + &[ + "-t", + "nat", + "-D", + "POSTROUTING", + "-s", + &guest_cidr, + "-o", + wan_iface, + ], comment, - "-j", - "MASQUERADE", - ]); - - let _ = iptables_delete(&[ - "-D", - "FORWARD", - "-i", - tap_device, - "-o", - wan_iface, - "-m", - "comment", - "--comment", + &["-j", "MASQUERADE"], + )); + + let _ = iptables_delete(&with_comment( + &["-D", "FORWARD", "-i", tap_device, "-o", wan_iface], comment, - "-j", - "ACCEPT", - ]); - - let _ = iptables_delete(&[ - "-D", - "FORWARD", - "-i", - wan_iface, - "-o", - tap_device, - "-m", - "conntrack", - "--ctstate", - "RELATED,ESTABLISHED", - "-m", - "comment", - "--comment", + &["-j", "ACCEPT"], + )); + + let _ = iptables_delete(&with_comment( + &[ + "-D", + "FORWARD", + "-i", + wan_iface, + "-o", + tap_device, + "-m", + "conntrack", + "--ctstate", + "RELATED,ESTABLISHED", + ], comment, - "-j", - "ACCEPT", - ]); + &["-j", "ACCEPT"], + )); Ok(()) } +/// Splice `-m comment --comment ` between rule head and tail +/// when `comment` is non-empty. Empty comment yields the unwrapped +/// rule, matching what older ember binaries emitted byte-for-byte — +/// crucial because iptables compares full rules during `-D`. +fn with_comment<'a>(head: &[&'a str], comment: &'a str, tail: &[&'a str]) -> Vec<&'a str> { + let mut out = Vec::with_capacity(head.len() + tail.len() + 4); + out.extend_from_slice(head); + if !comment.is_empty() { + out.extend_from_slice(&["-m", "comment", "--comment", comment]); + } + out.extend_from_slice(tail); + out +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn with_comment_skips_match_when_empty() { + // Legacy mode (empty comment) must produce byte-for-byte the + // same rule the old binary added, otherwise `iptables -D` + // won't match existing rules on upgraded hosts. + let args = with_comment(&["-A", "FORWARD", "-i", "tap0"], "", &["-j", "ACCEPT"]); + assert_eq!(args, vec!["-A", "FORWARD", "-i", "tap0", "-j", "ACCEPT"]); + } + + #[test] + fn with_comment_inserts_comment_match_when_non_empty() { + let args = with_comment( + &["-A", "FORWARD", "-i", "tap0"], + "ember:a3f4", + &["-j", "ACCEPT"], + ); + assert_eq!( + args, + vec![ + "-A", + "FORWARD", + "-i", + "tap0", + "-m", + "comment", + "--comment", + "ember:a3f4", + "-j", + "ACCEPT" + ] + ); + } +} + /// Enable IPv4 forwarding via sysctl. /// /// This is required once before any VM can route traffic through the host. diff --git a/tests/dm_thin.rs b/tests/dm_thin.rs index aa6564a..b96b2e8 100644 --- a/tests/dm_thin.rs +++ b/tests/dm_thin.rs @@ -137,6 +137,262 @@ fn dm_thin_init_refuses_backend_switch() { ); } +/// Two installs at different `--state-dir` with distinct +/// `--instance-id`s must not share dm-thin pools, and tearing one +/// down must leave the other intact. This is the core promise that +/// `instance_id` exists to deliver. +#[test] +#[ignore = "requires root + dm-thin kernel module"] +fn dm_thin_two_installs_dont_interfere() { + let tmp = tempfile::tempdir().unwrap(); + + let storage_a = tmp.path().join("dm-thin-a"); + let state_a = tmp.path().join("state-a"); + let storage_b = tmp.path().join("dm-thin-b"); + let state_b = tmp.path().join("state-b"); + + // Order matters: cleanup runs in reverse, so install A is torn + // down last. If install B's deinit accidentally killed pool A, + // the assertions below catch it before this fires. + let _cleanup_b = common::linux::DmThinCleanup { + state_dir: state_b.clone(), + }; + let _cleanup_a = common::linux::DmThinCleanup { + state_dir: state_a.clone(), + }; + + // Init install A. + let output = common::ember(&[ + "--state-dir", + state_a.to_str().unwrap(), + "init", + "--storage", + "dm-thin", + "--storage-path", + storage_a.to_str().unwrap(), + "--size", + "200M", + "--instance-id", + "aaaa", + ]); + assert!( + output.status.success(), + "init A failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + // Init install B. Different instance id — pool name must not + // collide. + let output = common::ember(&[ + "--state-dir", + state_b.to_str().unwrap(), + "init", + "--storage", + "dm-thin", + "--storage-path", + storage_b.to_str().unwrap(), + "--size", + "200M", + "--instance-id", + "bbbb", + ]); + assert!( + output.status.success(), + "init B failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + // Both pools live side-by-side in the kernel. + assert!( + Path::new("/dev/mapper/ember-aaaa-pool").exists(), + "ember-aaaa-pool should exist after install A's init" + ); + assert!( + Path::new("/dev/mapper/ember-bbbb-pool").exists(), + "ember-bbbb-pool should exist after install B's init" + ); + + // Tear down install A. Install B must survive untouched — this + // is the regression guard against the old singleton pool name. + let output = common::ember(&[ + "--state-dir", + state_a.to_str().unwrap(), + "deinit", + "--purge", + ]); + assert!( + output.status.success(), + "deinit A failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + assert!( + !Path::new("/dev/mapper/ember-aaaa-pool").exists(), + "ember-aaaa-pool should be gone after install A's deinit" + ); + assert!( + Path::new("/dev/mapper/ember-bbbb-pool").exists(), + "ember-bbbb-pool must NOT have been torn down by install A's deinit" + ); + + // Confirm install B is still functional: deinit it cleanly. + let output = common::ember(&[ + "--state-dir", + state_b.to_str().unwrap(), + "deinit", + "--purge", + ]); + assert!( + output.status.success(), + "deinit B failed (would indicate install A's deinit corrupted B): {}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(!Path::new("/dev/mapper/ember-bbbb-pool").exists()); +} + +/// Configs written by older ember binaries (no `instance_id` field) +/// must keep working without forcing a deinit/reinit. Verifies the +/// binary deserializes such a config and emits the legacy unprefixed +/// pool name through the public `ember info` surface. +#[test] +#[ignore = "requires root (writes to a temp state dir but doesn't touch the kernel)"] +fn legacy_config_without_instance_id_keeps_working() { + let tmp = tempfile::tempdir().unwrap(); + let state_dir = tmp.path().join("state"); + // Recreate the directory layout that `StateStore::init()` would + // have produced. `try_open` keys off the presence of `vms/`. + for sub in ["", "vms", "images", "network", "kernels"] { + std::fs::create_dir_all(state_dir.join(sub)).unwrap(); + } + + // Hand-craft a config exactly like an older binary would write + // it: storage_backend present, no instance_id, no ip_subnet. + let legacy_config = serde_json::json!({ + "storage_backend": "dm-thin", + "pool": "tank", + "dataset": "ember", + "kernel_path": null, + "wan_iface": null, + "state_dir": state_dir.to_str().unwrap(), + "storage_path": tmp.path().join("dm-thin").to_str().unwrap(), + "dm_thin_block_size": 128, + "dm_thin_mode": "file", + }); + std::fs::write( + state_dir.join("config.json"), + serde_json::to_vec_pretty(&legacy_config).unwrap(), + ) + .unwrap(); + + // `ember info` reads config and renders the dm-thin pool name + // via the accessor. Legacy mode must report the unprefixed name. + let output = common::ember(&["--state-dir", state_dir.to_str().unwrap(), "info"]); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + output.status.success(), + "info on a legacy config failed.\nstdout: {stdout}\nstderr: {stderr}" + ); + assert!( + stdout.contains("ember-pool"), + "expected legacy pool name 'ember-pool' in info output:\n{stdout}" + ); + // Make sure the new prefixed form did NOT sneak in. + assert!( + !stdout.contains("ember--pool"), + "legacy compat regression: empty instance_id produced 'ember--pool':\n{stdout}" + ); +} + +/// Reconcile (run at the start of every command) must only sweep +/// TAP devices belonging to *this* install's prefix. Create a TAP +/// device manually with install A's prefix, then run a reconcile- +/// triggering command in install B and verify A's TAP survives. +#[test] +#[ignore = "requires root + ip-link"] +fn reconcile_does_not_touch_other_installs_taps() { + let tmp = tempfile::tempdir().unwrap(); + let storage_a = tmp.path().join("dm-thin-a"); + let state_a = tmp.path().join("state-a"); + let storage_b = tmp.path().join("dm-thin-b"); + let state_b = tmp.path().join("state-b"); + + let _cleanup_b = common::linux::DmThinCleanup { + state_dir: state_b.clone(), + }; + let _cleanup_a = common::linux::DmThinCleanup { + state_dir: state_a.clone(), + }; + + // Init both installs with distinct instance ids → distinct TAP + // prefixes (`emaaaa-` and `embbbb-`). + for (state, storage, id) in [ + (&state_a, &storage_a, "aaaa"), + (&state_b, &storage_b, "bbbb"), + ] { + let output = common::ember(&[ + "--state-dir", + state.to_str().unwrap(), + "init", + "--storage", + "dm-thin", + "--storage-path", + storage.to_str().unwrap(), + "--size", + "200M", + "--instance-id", + id, + ]); + assert!( + output.status.success(), + "init {id} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + } + + // Manually create a TAP device that looks like one of install A's + // VMs. We don't bring it up or address it — `ip tuntap add` is + // enough for the kernel to list it as a tun device, which is what + // ember's reconcile sweep enumerates. + let tap_a = "emaaaa-deadbee"; + let status = std::process::Command::new("ip") + .args(["tuntap", "add", tap_a, "mode", "tap"]) + .status() + .expect("failed to run ip tuntap add"); + assert!(status.success(), "failed to create test TAP {tap_a}"); + // Always remove the TAP, even on assertion panic below. + struct TapCleanup(&'static str); + impl Drop for TapCleanup { + fn drop(&mut self) { + let _ = std::process::Command::new("ip") + .args(["tuntap", "del", self.0, "mode", "tap"]) + .status(); + } + } + let _tap_cleanup = TapCleanup(tap_a); + + // Run a reconcile-triggering command in install B. `vm list` is + // cheap and runs reconcile up-front; if reconcile is wrongly + // unscoped, it will sweep `emaaaa-deadbee`. + let output = common::ember(&["--state-dir", state_b.to_str().unwrap(), "vm", "list"]); + assert!( + output.status.success(), + "vm list in install B failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + // Install A's TAP must still exist. + let check = std::process::Command::new("ip") + .args(["link", "show", tap_a]) + .output() + .expect("failed to run ip link show"); + assert!( + check.status.success(), + "install B's reconcile deleted install A's TAP '{tap_a}': {}", + String::from_utf8_lossy(&check.stderr) + ); +} + /// `ember storage grow --size ` should grow the data device. #[test] #[ignore = "requires root + dm-thin kernel module"] From 8b3c6f36585434159d0d3e5b870ed7e8cd88b84f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 4 May 2026 04:49:56 +0000 Subject: [PATCH 03/10] tests: split isolation contract into its own integration file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cross-installation isolation tests (two-install dm-thin non-interference, legacy config compatibility, scoped TAP reconciliation) live in `tests/isolation.rs` rather than piggybacking on `tests/dm_thin.rs`. The dm-thin file is for backend mechanics; isolation is a different invariant that happens to use dm-thin as its substrate. Also fold a one-line WHY note into each legacy-mode branch on GlobalConfig — the literals there must match what the pre-instance-id binary wrote to disk and to the kernel, and the specific failure mode (orphaned thin ids, IFNAMSIZ overflow, silent iptables -D no-ops) varies per accessor. https://claude.ai/code/session_01XbXjsQa8SRWLzxnuKFzxt6 --- crates/ember-core/src/config.rs | 22 +++ tests/dm_thin.rs | 256 ----------------------------- tests/isolation.rs | 278 ++++++++++++++++++++++++++++++++ 3 files changed, 300 insertions(+), 256 deletions(-) create mode 100644 tests/isolation.rs diff --git a/crates/ember-core/src/config.rs b/crates/ember-core/src/config.rs index 5060010..2f0c355 100644 --- a/crates/ember-core/src/config.rs +++ b/crates/ember-core/src/config.rs @@ -174,6 +174,10 @@ impl GlobalConfig { /// upgraded hosts is still reachable. pub fn dm_thin_pool_name(&self) -> String { if self.instance_id.is_empty() { + // Older binaries created `dmsetup create ember-pool`. Any + // other string here would point at a non-existent pool + // (or, worse, a `ember--pool` typo that init would race + // to create), orphaning the data on disk. "ember-pool".to_string() } else { format!("ember-{}-pool", self.instance_id) @@ -184,6 +188,10 @@ impl GlobalConfig { /// (`ember-{id}-img-`, or legacy `ember-img-`). pub fn dm_thin_image_prefix(&self) -> String { if self.instance_id.is_empty() { + // Existing image volumes on the host are named + // `ember-img-`; teardown's prefix sweep and + // per-image lookups must match that exact prefix or the + // pool will accumulate orphaned thin ids. "ember-img-".to_string() } else { format!("ember-{}-img-", self.instance_id) @@ -194,6 +202,9 @@ impl GlobalConfig { /// legacy `ember-vm-`). pub fn dm_thin_vm_prefix(&self) -> String { if self.instance_id.is_empty() { + // Same reasoning as `dm_thin_image_prefix`: existing VM + // disks are `ember-vm-`, and `vm.json` records the + // device path with that prefix. "ember-vm-".to_string() } else { format!("ember-{}-vm-", self.instance_id) @@ -205,6 +216,10 @@ impl GlobalConfig { /// budget (14 chars with the 4-hex id, 10 chars in legacy mode). pub fn tap_prefix(&self) -> String { if self.instance_id.is_empty() { + // Older binaries persisted TAP names like `em-` on + // every running VM's `vm.json`; reconcile's orphan sweep + // and teardown's `ip link delete` both reference that + // exact name, so the prefix has to stay 3 chars. "em-".to_string() } else { format!("em{}-", self.instance_id) @@ -217,6 +232,13 @@ impl GlobalConfig { /// already on the host don't carry one to match against. pub fn iptables_comment(&self) -> String { if self.instance_id.is_empty() { + // `iptables -D` requires a byte-for-byte rule match. If + // we returned a non-empty comment here, the upgraded + // binary would emit `... -m comment --comment ember ...` + // on add and try to delete the same shape on remove — + // but the rule already on the host has no comment match, + // so the delete silently no-ops and rules accumulate + // forever. Empty preserves the original rule shape. String::new() } else { format!("ember:{}", self.instance_id) diff --git a/tests/dm_thin.rs b/tests/dm_thin.rs index b96b2e8..aa6564a 100644 --- a/tests/dm_thin.rs +++ b/tests/dm_thin.rs @@ -137,262 +137,6 @@ fn dm_thin_init_refuses_backend_switch() { ); } -/// Two installs at different `--state-dir` with distinct -/// `--instance-id`s must not share dm-thin pools, and tearing one -/// down must leave the other intact. This is the core promise that -/// `instance_id` exists to deliver. -#[test] -#[ignore = "requires root + dm-thin kernel module"] -fn dm_thin_two_installs_dont_interfere() { - let tmp = tempfile::tempdir().unwrap(); - - let storage_a = tmp.path().join("dm-thin-a"); - let state_a = tmp.path().join("state-a"); - let storage_b = tmp.path().join("dm-thin-b"); - let state_b = tmp.path().join("state-b"); - - // Order matters: cleanup runs in reverse, so install A is torn - // down last. If install B's deinit accidentally killed pool A, - // the assertions below catch it before this fires. - let _cleanup_b = common::linux::DmThinCleanup { - state_dir: state_b.clone(), - }; - let _cleanup_a = common::linux::DmThinCleanup { - state_dir: state_a.clone(), - }; - - // Init install A. - let output = common::ember(&[ - "--state-dir", - state_a.to_str().unwrap(), - "init", - "--storage", - "dm-thin", - "--storage-path", - storage_a.to_str().unwrap(), - "--size", - "200M", - "--instance-id", - "aaaa", - ]); - assert!( - output.status.success(), - "init A failed: {}", - String::from_utf8_lossy(&output.stderr) - ); - - // Init install B. Different instance id — pool name must not - // collide. - let output = common::ember(&[ - "--state-dir", - state_b.to_str().unwrap(), - "init", - "--storage", - "dm-thin", - "--storage-path", - storage_b.to_str().unwrap(), - "--size", - "200M", - "--instance-id", - "bbbb", - ]); - assert!( - output.status.success(), - "init B failed: {}", - String::from_utf8_lossy(&output.stderr) - ); - - // Both pools live side-by-side in the kernel. - assert!( - Path::new("/dev/mapper/ember-aaaa-pool").exists(), - "ember-aaaa-pool should exist after install A's init" - ); - assert!( - Path::new("/dev/mapper/ember-bbbb-pool").exists(), - "ember-bbbb-pool should exist after install B's init" - ); - - // Tear down install A. Install B must survive untouched — this - // is the regression guard against the old singleton pool name. - let output = common::ember(&[ - "--state-dir", - state_a.to_str().unwrap(), - "deinit", - "--purge", - ]); - assert!( - output.status.success(), - "deinit A failed: {}", - String::from_utf8_lossy(&output.stderr) - ); - - assert!( - !Path::new("/dev/mapper/ember-aaaa-pool").exists(), - "ember-aaaa-pool should be gone after install A's deinit" - ); - assert!( - Path::new("/dev/mapper/ember-bbbb-pool").exists(), - "ember-bbbb-pool must NOT have been torn down by install A's deinit" - ); - - // Confirm install B is still functional: deinit it cleanly. - let output = common::ember(&[ - "--state-dir", - state_b.to_str().unwrap(), - "deinit", - "--purge", - ]); - assert!( - output.status.success(), - "deinit B failed (would indicate install A's deinit corrupted B): {}", - String::from_utf8_lossy(&output.stderr) - ); - assert!(!Path::new("/dev/mapper/ember-bbbb-pool").exists()); -} - -/// Configs written by older ember binaries (no `instance_id` field) -/// must keep working without forcing a deinit/reinit. Verifies the -/// binary deserializes such a config and emits the legacy unprefixed -/// pool name through the public `ember info` surface. -#[test] -#[ignore = "requires root (writes to a temp state dir but doesn't touch the kernel)"] -fn legacy_config_without_instance_id_keeps_working() { - let tmp = tempfile::tempdir().unwrap(); - let state_dir = tmp.path().join("state"); - // Recreate the directory layout that `StateStore::init()` would - // have produced. `try_open` keys off the presence of `vms/`. - for sub in ["", "vms", "images", "network", "kernels"] { - std::fs::create_dir_all(state_dir.join(sub)).unwrap(); - } - - // Hand-craft a config exactly like an older binary would write - // it: storage_backend present, no instance_id, no ip_subnet. - let legacy_config = serde_json::json!({ - "storage_backend": "dm-thin", - "pool": "tank", - "dataset": "ember", - "kernel_path": null, - "wan_iface": null, - "state_dir": state_dir.to_str().unwrap(), - "storage_path": tmp.path().join("dm-thin").to_str().unwrap(), - "dm_thin_block_size": 128, - "dm_thin_mode": "file", - }); - std::fs::write( - state_dir.join("config.json"), - serde_json::to_vec_pretty(&legacy_config).unwrap(), - ) - .unwrap(); - - // `ember info` reads config and renders the dm-thin pool name - // via the accessor. Legacy mode must report the unprefixed name. - let output = common::ember(&["--state-dir", state_dir.to_str().unwrap(), "info"]); - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); - assert!( - output.status.success(), - "info on a legacy config failed.\nstdout: {stdout}\nstderr: {stderr}" - ); - assert!( - stdout.contains("ember-pool"), - "expected legacy pool name 'ember-pool' in info output:\n{stdout}" - ); - // Make sure the new prefixed form did NOT sneak in. - assert!( - !stdout.contains("ember--pool"), - "legacy compat regression: empty instance_id produced 'ember--pool':\n{stdout}" - ); -} - -/// Reconcile (run at the start of every command) must only sweep -/// TAP devices belonging to *this* install's prefix. Create a TAP -/// device manually with install A's prefix, then run a reconcile- -/// triggering command in install B and verify A's TAP survives. -#[test] -#[ignore = "requires root + ip-link"] -fn reconcile_does_not_touch_other_installs_taps() { - let tmp = tempfile::tempdir().unwrap(); - let storage_a = tmp.path().join("dm-thin-a"); - let state_a = tmp.path().join("state-a"); - let storage_b = tmp.path().join("dm-thin-b"); - let state_b = tmp.path().join("state-b"); - - let _cleanup_b = common::linux::DmThinCleanup { - state_dir: state_b.clone(), - }; - let _cleanup_a = common::linux::DmThinCleanup { - state_dir: state_a.clone(), - }; - - // Init both installs with distinct instance ids → distinct TAP - // prefixes (`emaaaa-` and `embbbb-`). - for (state, storage, id) in [ - (&state_a, &storage_a, "aaaa"), - (&state_b, &storage_b, "bbbb"), - ] { - let output = common::ember(&[ - "--state-dir", - state.to_str().unwrap(), - "init", - "--storage", - "dm-thin", - "--storage-path", - storage.to_str().unwrap(), - "--size", - "200M", - "--instance-id", - id, - ]); - assert!( - output.status.success(), - "init {id} failed: {}", - String::from_utf8_lossy(&output.stderr) - ); - } - - // Manually create a TAP device that looks like one of install A's - // VMs. We don't bring it up or address it — `ip tuntap add` is - // enough for the kernel to list it as a tun device, which is what - // ember's reconcile sweep enumerates. - let tap_a = "emaaaa-deadbee"; - let status = std::process::Command::new("ip") - .args(["tuntap", "add", tap_a, "mode", "tap"]) - .status() - .expect("failed to run ip tuntap add"); - assert!(status.success(), "failed to create test TAP {tap_a}"); - // Always remove the TAP, even on assertion panic below. - struct TapCleanup(&'static str); - impl Drop for TapCleanup { - fn drop(&mut self) { - let _ = std::process::Command::new("ip") - .args(["tuntap", "del", self.0, "mode", "tap"]) - .status(); - } - } - let _tap_cleanup = TapCleanup(tap_a); - - // Run a reconcile-triggering command in install B. `vm list` is - // cheap and runs reconcile up-front; if reconcile is wrongly - // unscoped, it will sweep `emaaaa-deadbee`. - let output = common::ember(&["--state-dir", state_b.to_str().unwrap(), "vm", "list"]); - assert!( - output.status.success(), - "vm list in install B failed: {}", - String::from_utf8_lossy(&output.stderr) - ); - - // Install A's TAP must still exist. - let check = std::process::Command::new("ip") - .args(["link", "show", tap_a]) - .output() - .expect("failed to run ip link show"); - assert!( - check.status.success(), - "install B's reconcile deleted install A's TAP '{tap_a}': {}", - String::from_utf8_lossy(&check.stderr) - ); -} - /// `ember storage grow --size ` should grow the data device. #[test] #[ignore = "requires root + dm-thin kernel module"] diff --git a/tests/isolation.rs b/tests/isolation.rs new file mode 100644 index 0000000..c76234c --- /dev/null +++ b/tests/isolation.rs @@ -0,0 +1,278 @@ +//! Cross-installation isolation contract. +//! +//! These tests guard the promise that two ember installations on the +//! same host don't see, share, or destroy each other's resources, and +//! that an installation predating `instance_id` keeps working after +//! the binary upgrade. Failures here mean an integration-test run can +//! corrupt the developer's live install — exactly what `instance_id` +//! exists to prevent. +//! +//! Gated `#[ignore]` and Linux-only because they touch real +//! device-mapper, TAP, and iptables state. Run explicitly with: +//! +//! ```text +//! sudo cargo test --test isolation -- --ignored --test-threads=1 +//! ``` + +#![cfg(target_os = "linux")] + +#[allow(dead_code)] +mod common; + +use std::path::Path; + +/// Two installs at different `--state-dir` with distinct +/// `--instance-id`s must not share dm-thin pools, and tearing one +/// down must leave the other intact. This is the core promise that +/// `instance_id` exists to deliver. +#[test] +#[ignore = "requires root + dm-thin kernel module"] +fn dm_thin_two_installs_dont_interfere() { + let tmp = tempfile::tempdir().unwrap(); + + let storage_a = tmp.path().join("dm-thin-a"); + let state_a = tmp.path().join("state-a"); + let storage_b = tmp.path().join("dm-thin-b"); + let state_b = tmp.path().join("state-b"); + + // Order matters: cleanup runs in reverse, so install A is torn + // down last. If install B's deinit accidentally killed pool A, + // the assertions below catch it before this fires. + let _cleanup_b = common::linux::DmThinCleanup { + state_dir: state_b.clone(), + }; + let _cleanup_a = common::linux::DmThinCleanup { + state_dir: state_a.clone(), + }; + + // Init install A. + let output = common::ember(&[ + "--state-dir", + state_a.to_str().unwrap(), + "init", + "--storage", + "dm-thin", + "--storage-path", + storage_a.to_str().unwrap(), + "--size", + "200M", + "--instance-id", + "aaaa", + ]); + assert!( + output.status.success(), + "init A failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + // Init install B. Different instance id — pool name must not + // collide. + let output = common::ember(&[ + "--state-dir", + state_b.to_str().unwrap(), + "init", + "--storage", + "dm-thin", + "--storage-path", + storage_b.to_str().unwrap(), + "--size", + "200M", + "--instance-id", + "bbbb", + ]); + assert!( + output.status.success(), + "init B failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + // Both pools live side-by-side in the kernel. + assert!( + Path::new("/dev/mapper/ember-aaaa-pool").exists(), + "ember-aaaa-pool should exist after install A's init" + ); + assert!( + Path::new("/dev/mapper/ember-bbbb-pool").exists(), + "ember-bbbb-pool should exist after install B's init" + ); + + // Tear down install A. Install B must survive untouched — this + // is the regression guard against the old singleton pool name. + let output = common::ember(&[ + "--state-dir", + state_a.to_str().unwrap(), + "deinit", + "--purge", + ]); + assert!( + output.status.success(), + "deinit A failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + assert!( + !Path::new("/dev/mapper/ember-aaaa-pool").exists(), + "ember-aaaa-pool should be gone after install A's deinit" + ); + assert!( + Path::new("/dev/mapper/ember-bbbb-pool").exists(), + "ember-bbbb-pool must NOT have been torn down by install A's deinit" + ); + + // Confirm install B is still functional: deinit it cleanly. + let output = common::ember(&[ + "--state-dir", + state_b.to_str().unwrap(), + "deinit", + "--purge", + ]); + assert!( + output.status.success(), + "deinit B failed (would indicate install A's deinit corrupted B): {}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(!Path::new("/dev/mapper/ember-bbbb-pool").exists()); +} + +/// Configs written by older ember binaries (no `instance_id` field) +/// must keep working without forcing a deinit/reinit. Verifies the +/// binary deserializes such a config and emits the legacy unprefixed +/// pool name through the public `ember info` surface. +#[test] +#[ignore = "requires root (writes to a temp state dir but doesn't touch the kernel)"] +fn legacy_config_without_instance_id_keeps_working() { + let tmp = tempfile::tempdir().unwrap(); + let state_dir = tmp.path().join("state"); + // Recreate the directory layout that `StateStore::init()` would + // have produced. `try_open` keys off the presence of `vms/`. + for sub in ["", "vms", "images", "network", "kernels"] { + std::fs::create_dir_all(state_dir.join(sub)).unwrap(); + } + + // Hand-craft a config exactly like an older binary would write + // it: storage_backend present, no instance_id, no ip_subnet. + let legacy_config = serde_json::json!({ + "storage_backend": "dm-thin", + "pool": "tank", + "dataset": "ember", + "kernel_path": null, + "wan_iface": null, + "state_dir": state_dir.to_str().unwrap(), + "storage_path": tmp.path().join("dm-thin").to_str().unwrap(), + "dm_thin_block_size": 128, + "dm_thin_mode": "file", + }); + std::fs::write( + state_dir.join("config.json"), + serde_json::to_vec_pretty(&legacy_config).unwrap(), + ) + .unwrap(); + + // `ember info` reads config and renders the dm-thin pool name + // via the accessor. Legacy mode must report the unprefixed name. + let output = common::ember(&["--state-dir", state_dir.to_str().unwrap(), "info"]); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + output.status.success(), + "info on a legacy config failed.\nstdout: {stdout}\nstderr: {stderr}" + ); + assert!( + stdout.contains("ember-pool"), + "expected legacy pool name 'ember-pool' in info output:\n{stdout}" + ); + // Make sure the new prefixed form did NOT sneak in. + assert!( + !stdout.contains("ember--pool"), + "legacy compat regression: empty instance_id produced 'ember--pool':\n{stdout}" + ); +} + +/// Reconcile (run at the start of every command) must only sweep +/// TAP devices belonging to *this* install's prefix. Create a TAP +/// device manually with install A's prefix, then run a reconcile- +/// triggering command in install B and verify A's TAP survives. +#[test] +#[ignore = "requires root + ip-link"] +fn reconcile_does_not_touch_other_installs_taps() { + let tmp = tempfile::tempdir().unwrap(); + let storage_a = tmp.path().join("dm-thin-a"); + let state_a = tmp.path().join("state-a"); + let storage_b = tmp.path().join("dm-thin-b"); + let state_b = tmp.path().join("state-b"); + + let _cleanup_b = common::linux::DmThinCleanup { + state_dir: state_b.clone(), + }; + let _cleanup_a = common::linux::DmThinCleanup { + state_dir: state_a.clone(), + }; + + // Init both installs with distinct instance ids → distinct TAP + // prefixes (`emaaaa-` and `embbbb-`). + for (state, storage, id) in [ + (&state_a, &storage_a, "aaaa"), + (&state_b, &storage_b, "bbbb"), + ] { + let output = common::ember(&[ + "--state-dir", + state.to_str().unwrap(), + "init", + "--storage", + "dm-thin", + "--storage-path", + storage.to_str().unwrap(), + "--size", + "200M", + "--instance-id", + id, + ]); + assert!( + output.status.success(), + "init {id} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + } + + // Manually create a TAP device that looks like one of install A's + // VMs. We don't bring it up or address it — `ip tuntap add` is + // enough for the kernel to list it as a tun device, which is what + // ember's reconcile sweep enumerates. + let tap_a = "emaaaa-deadbee"; + let status = std::process::Command::new("ip") + .args(["tuntap", "add", tap_a, "mode", "tap"]) + .status() + .expect("failed to run ip tuntap add"); + assert!(status.success(), "failed to create test TAP {tap_a}"); + // Always remove the TAP, even on assertion panic below. + struct TapCleanup(&'static str); + impl Drop for TapCleanup { + fn drop(&mut self) { + let _ = std::process::Command::new("ip") + .args(["tuntap", "del", self.0, "mode", "tap"]) + .status(); + } + } + let _tap_cleanup = TapCleanup(tap_a); + + // Run a reconcile-triggering command in install B. `vm list` is + // cheap and runs reconcile up-front; if reconcile is wrongly + // unscoped, it will sweep `emaaaa-deadbee`. + let output = common::ember(&["--state-dir", state_b.to_str().unwrap(), "vm", "list"]); + assert!( + output.status.success(), + "vm list in install B failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + // Install A's TAP must still exist. + let check = std::process::Command::new("ip") + .args(["link", "show", tap_a]) + .output() + .expect("failed to run ip link show"); + assert!( + check.status.success(), + "install B's reconcile deleted install A's TAP '{tap_a}': {}", + String::from_utf8_lossy(&check.stderr) + ); +} From 0cd9429332942a6293e45fd7c1ebb86535927935 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 4 May 2026 18:31:25 +0000 Subject: [PATCH 04/10] macos: sub-allocate vmnet's /24 into per-installation /27 slices vmnet's shared subnet (192.168.64.0/24) is owned by the framework and there's no public API to ask for a different one, so two macOS installs running VMs concurrently both allocate from the same /24 and hand out colliding guest IPs. State-dir isolation handled everything else on macOS, but this last collision survived. Carve the /24 into 8 /27 slots (32 addresses, 8 /30 VM blocks per install) and pick a slot from the low 3 bits of the instance id. Two installs collide with 1/8 probability per pair; `--ip-subnet` is the override when that bites. 8 concurrent VMs per install is generous for personal use on macOS. Plumb via a new `Platform::default_ip_subnet(instance_id)` trait method so init.rs picks the right derivation per platform: Linux keeps the existing /16 slot inside 10/8, macOS gets the /27 slice. `MacosNetwork::setup` reads `config.ip_subnet` for new configs and falls back to the full /24 when `instance_id` is empty so legacy installs continue to use the same allocations.json layout the old binary wrote. IP reuse on VM teardown was already correct: the allocator picks the lowest-numbered free `/30` block and `release()` removes the entry, so freed slots are reused before higher indices. Existing `allocate_reuses_released_block` test covers it. https://claude.ai/code/session_01XbXjsQa8SRWLzxnuKFzxt6 --- crates/ember-core/src/backend.rs | 13 ++++ crates/ember-linux/src/platform.rs | 6 +- crates/ember-macos/src/network.rs | 95 ++++++++++++++++++++++++++++-- crates/ember-macos/src/platform.rs | 4 ++ src/cli/init.rs | 8 +-- 5 files changed, 115 insertions(+), 11 deletions(-) diff --git a/crates/ember-core/src/backend.rs b/crates/ember-core/src/backend.rs index a0816df..c456d41 100644 --- a/crates/ember-core/src/backend.rs +++ b/crates/ember-core/src/backend.rs @@ -388,6 +388,19 @@ pub trait Platform { /// Linux: `/var/lib/ember`. macOS: `~/Library/Application Support/ember`. fn default_state_dir() -> PathBuf; + /// Default IP subnet handed to `GlobalConfig.ip_subnet` at + /// `ember init` when the user doesn't pass `--ip-subnet`. + /// + /// Linux carves a `/16` slot inside `10.0.0.0/8` because the + /// host has full control of routing — installations can scale to + /// 16k VMs each. macOS sub-allocates a `/27` inside vmnet's + /// host-wide `192.168.64.0/24` because Apple's vmnet shared mode + /// owns that /24 and there's no way to ask for a different one. + /// Each macOS install gets 8 VMs; a `/8` collision between two + /// installs is unlikely (1/8 per pair) and resolvable via the + /// `--ip-subnet` override. + fn default_ip_subnet(instance_id: &str) -> String; + /// Console device name for inittab injection. /// /// Linux/Firecracker: `"ttyS0"`. macOS/AVF: `"hvc0"`. diff --git a/crates/ember-linux/src/platform.rs b/crates/ember-linux/src/platform.rs index 1749fbe..742e0da 100644 --- a/crates/ember-linux/src/platform.rs +++ b/crates/ember-linux/src/platform.rs @@ -1,7 +1,7 @@ use std::path::{Path, PathBuf}; use ember_core::backend::{ImageToolConfig, Platform, ResolvConfMode}; -use ember_core::config::{GlobalConfig, StorageKind}; +use ember_core::config::{derive_ip_subnet, GlobalConfig, StorageKind}; use ember_core::error::Result; use ember_core::image::registry::ImageEntry; use ember_core::state::vm::VmMetadata; @@ -23,6 +23,10 @@ impl Platform for LinuxPlatform { PathBuf::from("/var/lib/ember") } + fn default_ip_subnet(instance_id: &str) -> String { + derive_ip_subnet(instance_id) + } + fn console_device() -> &'static str { "ttyS0" } diff --git a/crates/ember-macos/src/network.rs b/crates/ember-macos/src/network.rs index bd83621..76a2c1f 100644 --- a/crates/ember-macos/src/network.rs +++ b/crates/ember-macos/src/network.rs @@ -19,11 +19,35 @@ pub const VMNET_GATEWAY: &str = "192.168.64.1"; /// Default netmask for vmnet shared mode (/24). pub const VMNET_NETMASK: &str = "255.255.255.0"; -/// vmnet shared mode subnet for IP allocation. +/// Host-wide vmnet shared-mode subnet. Apple's `VZNATNetworkDevice` +/// owns this /24 and there's no public API to ask for a different +/// one, so per-installation isolation can only sub-allocate inside +/// it; see [`derive_vmnet_subnet`]. /// -/// Uses /30 blocks via the shared `network::ip` allocator, giving 64 -/// concurrent VMs. The gateway (.1) is always vmnet's built-in router. -const VMNET_SUBNET: &str = "192.168.64.0/24"; +/// Used directly only as the legacy fallback for configs that +/// predate `instance_id` — new installs read +/// [`GlobalConfig::ip_subnet`](ember_core::config::GlobalConfig::ip_subnet) +/// (a /27 slice) instead. +pub const VMNET_SUBNET: &str = "192.168.64.0/24"; + +/// Derive a per-installation /27 sub-range inside [`VMNET_SUBNET`]. +/// +/// vmnet's shared subnet is fixed by the framework, so isolation has +/// to come from carving up the /24 rather than picking a different +/// one. We split it into 8 /27 slots (32 addresses, 8 /30 VM blocks +/// each) and pick by the low 3 bits of the instance id parsed as +/// hex. The collision probability between two installs is 1/8 per +/// pair — acceptable for personal use, with `--ip-subnet` as the +/// escape hatch when it bites. +/// +/// 8 VMs per install caps the install at 8 concurrent guests on +/// macOS, well above any realistic personal workflow. +pub fn derive_vmnet_subnet(instance_id: &str) -> String { + let id_int = u16::from_str_radix(instance_id, 16).unwrap_or(0); + let slot = (id_int & 0b111) as u8; + let base = slot * 32; + format!("192.168.64.{base}/27") +} /// macOS network backend using vmnet (shared mode). /// @@ -44,8 +68,20 @@ impl NetworkBackend for MacosNetwork { /// /// The IP is passed to the kernel via boot args (`ip=::...`) /// so the guest has connectivity immediately at boot — no DHCP needed. - fn setup(&self, vm: &VmMetadata, _config: &GlobalConfig) -> Result { - let allocation = network::ip::allocate(&self.store, VMNET_SUBNET, &vm.name)?; + fn setup(&self, vm: &VmMetadata, config: &GlobalConfig) -> Result { + // Legacy configs (no instance_id) keep using vmnet's full /24 + // exactly the way pre-instance-id binaries did — same + // allocator state, same IP layout, no migration. New + // configs honor `config.ip_subnet`, which `ember init` + // populates with a per-installation /27 slice via + // `MacosPlatform::default_ip_subnet`. + let default_subnet = if config.instance_id.is_empty() { + VMNET_SUBNET + } else { + config.ip_subnet.as_str() + }; + let subnet = vm.subnet.as_deref().unwrap_or(default_subnet); + let allocation = network::ip::allocate(&self.store, subnet, &vm.name)?; Ok(NetworkInfo { tap_device: String::new(), @@ -150,4 +186,51 @@ destination: default fn parse_route_empty() { assert_eq!(parse_interface_from_route(""), None); } + + // ── vmnet sub-range derivation ─────────────────────────────── + + #[test] + fn vmnet_subnet_lands_in_192_168_64_slash_24() { + for id in ["0000", "a3f4", "ffff", "dead", "beef"] { + let subnet = derive_vmnet_subnet(id); + assert!( + subnet.starts_with("192.168.64."), + "instance id {id} produced subnet outside vmnet's /24: {subnet}" + ); + assert!( + subnet.ends_with("/27"), + "instance id {id} produced non-/27 subnet: {subnet}" + ); + } + } + + #[test] + fn vmnet_subnet_uses_low_three_bits_of_id() { + // Same low 3 bits → same slot; bits 3-15 don't move it. + // 0x0007 and 0xffff both end in 0b111 → slot 7 → base 224. + assert_eq!(derive_vmnet_subnet("0007"), "192.168.64.224/27"); + assert_eq!(derive_vmnet_subnet("ffff"), "192.168.64.224/27"); + // 0x0000 → slot 0 → base 0. + assert_eq!(derive_vmnet_subnet("0000"), "192.168.64.0/27"); + // 0x0008 → slot 0 → base 0 (only low 3 bits matter). + assert_eq!(derive_vmnet_subnet("0008"), "192.168.64.0/27"); + } + + #[test] + fn vmnet_subnet_is_stable() { + // Reactivation must land on the same /27 the install was + // initialized with, otherwise allocations.json's pinned + // `base_subnet` mismatches and `network::ip::allocate` + // refuses to hand out IPs. + assert_eq!(derive_vmnet_subnet("a3f4"), derive_vmnet_subnet("a3f4")); + } + + #[test] + fn vmnet_subnet_falls_back_to_slot_zero_on_garbage_id() { + // Defensive: parse failure shouldn't panic, just land on + // the legacy-equivalent base. The user can then pick + // `--ip-subnet` explicitly. + assert_eq!(derive_vmnet_subnet("zzzz"), "192.168.64.0/27"); + assert_eq!(derive_vmnet_subnet(""), "192.168.64.0/27"); + } } diff --git a/crates/ember-macos/src/platform.rs b/crates/ember-macos/src/platform.rs index 96ae868..95fa9bd 100644 --- a/crates/ember-macos/src/platform.rs +++ b/crates/ember-macos/src/platform.rs @@ -38,6 +38,10 @@ impl Platform for MacosPlatform { } } + fn default_ip_subnet(instance_id: &str) -> String { + crate::network::derive_vmnet_subnet(instance_id) + } + fn console_device() -> &'static str { "hvc0" } diff --git a/src/cli/init.rs b/src/cli/init.rs index e0f72ba..cca3b01 100644 --- a/src/cli/init.rs +++ b/src/cli/init.rs @@ -4,9 +4,7 @@ use clap::Args; use crate::backend::{init_storage, CurrentPlatform, InitConfig, Platform}; use ember_core::config::size::ByteSize; -use ember_core::config::{ - derive_instance_id, derive_ip_subnet, DmThinMode, GlobalConfig, StorageKind, -}; +use ember_core::config::{derive_instance_id, DmThinMode, GlobalConfig, StorageKind}; use ember_core::state::store::StateStore; /// dm-thin pool block size (in 512-byte sectors) used when the user does @@ -177,10 +175,12 @@ pub fn run(args: &InitArgs, state_dir: &Path) -> anyhow::Result<()> { .instance_id .clone() .unwrap_or_else(|| derive_instance_id(state_dir)); + // Default subnet is platform-derived: Linux carves up 10.0.0.0/8, + // macOS sub-allocates inside vmnet's host-wide 192.168.64.0/24. let ip_subnet = args .ip_subnet .clone() - .unwrap_or_else(|| derive_ip_subnet(&instance_id)); + .unwrap_or_else(|| CurrentPlatform::default_ip_subnet(&instance_id)); let init_config = InitConfig { storage_backend: args.storage, From 6a982ae212c9290059784b1c58594d1371d6ba60 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 4 May 2026 18:52:35 +0000 Subject: [PATCH 05/10] macos: single-IP allocation lifts the per-install cap from 8 to ~30 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previous /27 slicing capped each macOS install at 8 VMs because ember reused Linux's /30 P2P allocator on macOS, where it makes no sense — vmnet provides a shared L2 bridge so every guest sits on the same subnet behind one gateway, and the host (.1) / broadcast (.3) of the per-VM /30 are pure waste. 75% of the address space went unused. Add `network::ip::allocate_single`: hands out one /32 address from a shared subnet, with a `reserved` list so callers can pin host-global addresses (vmnet's network/gateway/broadcast in our case) without seeding allocations.json. Persists through the same flock-locked file as `allocate`, with `block_index` reinterpreted as an address offset. `MacosNetwork::setup` branches on `instance_id`: * Legacy (no instance id): keeps using the /30 allocator against the full /24 so existing allocations.json layouts stay valid byte-for-byte across the upgrade. * New install: uses `allocate_single` against the /27 slot. ~30 VMs/install (32 - reserved), same 1/8 collision profile. Move the /30-minimum constraint out of `parse_cidr` into `allocate` so the shared parser can also accept /27, /28, /32 for single-IP callers. https://claude.ai/code/session_01XbXjsQa8SRWLzxnuKFzxt6 --- crates/ember-core/src/backend.rs | 16 +- crates/ember-core/src/network/ip.rs | 321 ++++++++++++++++++++++++++-- crates/ember-macos/src/network.rs | 66 ++++-- 3 files changed, 358 insertions(+), 45 deletions(-) diff --git a/crates/ember-core/src/backend.rs b/crates/ember-core/src/backend.rs index c456d41..c65e70e 100644 --- a/crates/ember-core/src/backend.rs +++ b/crates/ember-core/src/backend.rs @@ -391,14 +391,14 @@ pub trait Platform { /// Default IP subnet handed to `GlobalConfig.ip_subnet` at /// `ember init` when the user doesn't pass `--ip-subnet`. /// - /// Linux carves a `/16` slot inside `10.0.0.0/8` because the - /// host has full control of routing — installations can scale to - /// 16k VMs each. macOS sub-allocates a `/27` inside vmnet's - /// host-wide `192.168.64.0/24` because Apple's vmnet shared mode - /// owns that /24 and there's no way to ask for a different one. - /// Each macOS install gets 8 VMs; a `/8` collision between two - /// installs is unlikely (1/8 per pair) and resolvable via the - /// `--ip-subnet` override. + /// Linux carves a `/16` slot inside `10.0.0.0/8` and uses /30 + /// blocks per VM (host has full control of routing), scaling to + /// ~16k VMs per install. macOS sub-allocates a `/27` inside + /// vmnet's host-wide `192.168.64.0/24` and uses single-IP + /// allocation (vmnet's shared L2 bridge means /30 P2P links are + /// pointless), giving ~30 VMs per install. A `/8` collision + /// between two installs is unlikely (1/8 per pair) and + /// resolvable via the `--ip-subnet` override. fn default_ip_subnet(instance_id: &str) -> String; /// Console device name for inittab injection. diff --git a/crates/ember-core/src/network/ip.rs b/crates/ember-core/src/network/ip.rs index 8581d31..f29d8d0 100644 --- a/crates/ember-core/src/network/ip.rs +++ b/crates/ember-core/src/network/ip.rs @@ -1,13 +1,21 @@ -//! IP allocation from a configurable /N subnet in /30 blocks. +//! IP allocation for VM networking. //! -//! Each VM gets a point-to-point /30 link: host gets .1, guest gets .2. -//! Allocations are tracked in `allocations.json` via the state store -//! with flock-based locking for concurrent safety. +//! Two allocation strategies, picked per backend: //! -//! The base subnet is set per-installation in `GlobalConfig::ip_subnet` -//! (derived at `ember init` from the instance id, overridable via -//! `--ip-subnet`). With a /16, each install supports ~16,384 concurrent -//! VMs. +//! * [`allocate`] — Linux: hand out `/30` blocks for point-to-point +//! TAP routing. Each VM gets its own host (.1) and guest (.2) IPs +//! on a dedicated /30. With a `/16` base, ~16,384 VMs fit. +//! * [`allocate_single`] — macOS: hand out single `/32` addresses on +//! a shared subnet (vmnet's `192.168.64.0/24`). All VMs sit on the +//! same L2 segment behind one shared gateway, so a /30 link per VM +//! is overkill and would waste 75% of the address space. +//! +//! Both share the same `allocations.json` persistence layer with +//! flock-based locking; the `block_index` field's unit (4 addresses +//! for `allocate`, 1 for `allocate_single`) is implicit in which +//! function reads the file. An installation must use exactly one +//! strategy across its lifetime — the persisted `base_subnet` is +//! verified on every read. use std::collections::HashMap; use std::net::Ipv4Addr; @@ -20,22 +28,27 @@ use crate::state::store::StateStore; /// Persisted IP allocation state, stored as `allocations.json`. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct IpAllocations { - /// Base subnet in CIDR notation (e.g., "10.100.0.0/16"). + /// Base subnet in CIDR notation (e.g., "10.100.0.0/16" or + /// "192.168.64.0/27"). pub base_subnet: String, - /// Map from /30 block index to VM name. + /// Map from block index to VM name. Block size depends on which + /// allocator wrote the file: 4 addresses for [`allocate`], 1 for + /// [`allocate_single`]. pub allocations: HashMap, } /// A single IP allocation for one VM. #[derive(Debug, Clone, PartialEq)] pub struct IpAllocation { - /// Index of the /30 block within the base subnet. + /// Index within the base subnet. Unit is allocator-dependent. pub block_index: u32, - /// Host-side IP — first usable address in the /30 (e.g., "10.100.0.1"). + /// Host-side IP. For [`allocate`] (Linux): first usable address + /// in the /30 (e.g., "10.100.0.1"). For [`allocate_single`] + /// (macOS): the shared gateway passed by the caller. pub host_ip: String, - /// Guest-side IP — second usable address in the /30 (e.g., "10.100.0.2"). + /// Guest-side IP. pub guest_ip: String, - /// Netmask for the /30 link ("255.255.255.252"). + /// Netmask for the link. pub netmask: String, } @@ -43,6 +56,8 @@ pub struct IpAllocation { const NETMASK_30: &str = "255.255.255.252"; /// Parse a CIDR subnet string into (base address, prefix length). +/// Accepts any prefix `/0`..`/32`; per-allocator constraints (e.g. +/// `/30` minimum for [`allocate`]) are enforced at the call site. fn parse_cidr(cidr: &str) -> Result<(Ipv4Addr, u8)> { let (ip_str, prefix_str) = cidr .split_once('/') @@ -56,9 +71,9 @@ fn parse_cidr(cidr: &str) -> Result<(Ipv4Addr, u8)> { .parse() .map_err(|e| Error::Network(format!("invalid prefix in CIDR '{cidr}': {e}")))?; - if prefix > 30 { + if prefix > 32 { return Err(Error::Network(format!( - "subnet /{prefix} is too small for /30 allocations" + "invalid CIDR prefix /{prefix}: must be 0..=32" ))); } @@ -108,6 +123,11 @@ fn block_ips(base: Ipv4Addr, block_index: u32) -> IpAllocation { pub fn allocate(store: &StateStore, subnet: &str, vm_name: &str) -> Result { let path = store.network_allocations_path(); let (base, prefix) = parse_cidr(subnet)?; + if prefix > 30 { + return Err(Error::Network(format!( + "subnet /{prefix} is too small for /30 allocations" + ))); + } let max = max_blocks(prefix); let mut allocs: IpAllocations = store @@ -141,6 +161,77 @@ pub fn allocate(store: &StateStore, subnet: &str, vm_name: &str) -> Result Result { + let path = store.network_allocations_path(); + let (base, prefix) = parse_cidr(subnet)?; + let max = 1u32 << (32 - prefix); + + let mut allocs: IpAllocations = store + .read_optional(&path)? + .unwrap_or_else(|| IpAllocations { + base_subnet: subnet.to_string(), + allocations: HashMap::new(), + }); + + if allocs.base_subnet != subnet { + return Err(Error::Network(format!( + "subnet mismatch: state has '{}', requested '{subnet}'", + allocs.base_subnet + ))); + } + + let base_u32 = u32::from(base); + + // Walk the subnet looking for an unallocated, non-reserved slot. + // Skipping reserved addresses keeps the gateway (and the wider + // /24's network/broadcast when carved into /27s) un-handout-able + // without the caller having to seed allocations.json. + let block_index = (0..max) + .find(|i| { + if allocs.allocations.contains_key(i) { + return false; + } + let addr = Ipv4Addr::from(base_u32 + i); + !reserved.contains(&addr) + }) + .ok_or_else(|| { + Error::Network(format!( + "no free addresses in {subnet} (all {max} candidates allocated or reserved)" + )) + })?; + + let guest_ip = Ipv4Addr::from(base_u32 + block_index); + allocs.allocations.insert(block_index, vm_name.to_string()); + store.write(&path, &allocs)?; + + Ok(IpAllocation { + block_index, + host_ip: host_ip.to_string(), + guest_ip: guest_ip.to_string(), + netmask: netmask.to_string(), + }) +} + /// Release a VM's IP allocation. /// /// Removes all allocation entries for the given VM name, making the /30 @@ -185,8 +276,18 @@ mod tests { } #[test] - fn parse_cidr_rejects_slash_31() { - assert!(parse_cidr("10.0.0.0/31").is_err()); + fn parse_cidr_accepts_up_to_slash_32() { + // The /30-minimum constraint moved into `allocate` so the + // shared parser can also serve `allocate_single`, which + // accepts narrow prefixes (a /27 or even /32). + assert!(parse_cidr("10.0.0.0/31").is_ok()); + assert!(parse_cidr("192.168.64.0/27").is_ok()); + assert!(parse_cidr("10.0.0.5/32").is_ok()); + } + + #[test] + fn parse_cidr_rejects_slash_above_32() { + assert!(parse_cidr("10.0.0.0/33").is_err()); } #[test] @@ -347,4 +448,188 @@ mod tests { assert_eq!(allocs.base_subnet, "10.100.0.0/16"); assert_eq!(allocs.allocations.len(), 2); } + + #[test] + fn allocate_rejects_too_narrow_subnet() { + // /31 is too small for /30 P2P allocation; the constraint + // lives on `allocate` (not `parse_cidr`) since the shared + // parser also serves `allocate_single`. + let (_dir, store) = test_store(); + let err = allocate(&store, "10.0.0.0/31", "vm1").unwrap_err(); + assert!(matches!(err, Error::Network(_))); + } + + // --- allocate_single (macOS shared-subnet path) --- + + /// Helper: vmnet's host-global reservations carved out of the /24. + fn vmnet_reserved() -> [Ipv4Addr; 3] { + [ + Ipv4Addr::new(192, 168, 64, 0), // /24 network + Ipv4Addr::new(192, 168, 64, 1), // vmnet gateway + Ipv4Addr::new(192, 168, 64, 255), // /24 broadcast + ] + } + + #[test] + fn allocate_single_skips_network_and_gateway_in_slot_zero() { + // Slot 0 (192.168.64.0/27) contains both the /24 network + // (.0) and the vmnet gateway (.1). The first guest allocated + // must land on .2, not .0 or .1. + let (_dir, store) = test_store(); + let reserved = vmnet_reserved(); + let alloc = allocate_single( + &store, + "192.168.64.0/27", + "vm1", + "192.168.64.1", + "255.255.255.0", + &reserved, + ) + .unwrap(); + assert_eq!(alloc.guest_ip, "192.168.64.2"); + assert_eq!(alloc.host_ip, "192.168.64.1"); + assert_eq!(alloc.netmask, "255.255.255.0"); + // block_index reflects the address offset, not a /30 index. + assert_eq!(alloc.block_index, 2); + } + + #[test] + fn allocate_single_packs_addresses_one_per_vm() { + // /27 with 2 reserved addresses (.0, .1) yields 30 usable + // single-IP slots — 4× what /30 allocation gets. + let (_dir, store) = test_store(); + let reserved = vmnet_reserved(); + let mut last_octet = None; + for i in 0..30 { + let alloc = allocate_single( + &store, + "192.168.64.0/27", + &format!("vm{i}"), + "192.168.64.1", + "255.255.255.0", + &reserved, + ) + .unwrap(); + let octet: u8 = alloc.guest_ip.split('.').nth(3).unwrap().parse().unwrap(); + // Strictly monotonic, never .0 or .1. + assert!(octet >= 2); + if let Some(prev) = last_octet { + assert!(octet > prev, "expected strictly monotonic guest IPs"); + } + last_octet = Some(octet); + } + } + + #[test] + fn allocate_single_skips_broadcast_in_top_slot() { + // Slot 7 (192.168.64.224/27) ends at .255, which is the + // surrounding /24's broadcast. The allocator must not hand + // it out, so the slot holds 31 (not 32) usable addresses. + let (_dir, store) = test_store(); + let reserved = vmnet_reserved(); + for i in 0..31 { + allocate_single( + &store, + "192.168.64.224/27", + &format!("vm{i}"), + "192.168.64.1", + "255.255.255.0", + &reserved, + ) + .unwrap(); + } + // 32nd allocation hits .255 → reserved → no free slot. + let err = allocate_single( + &store, + "192.168.64.224/27", + "overflow", + "192.168.64.1", + "255.255.255.0", + &reserved, + ) + .unwrap_err(); + assert!(matches!(err, Error::Network(_))); + } + + #[test] + fn allocate_single_reuses_released_addresses() { + // Drop vm2, allocate vm4 → vm4 should land on vm2's freed + // slot (lowest unused index). + let (_dir, store) = test_store(); + let reserved = vmnet_reserved(); + let a1 = allocate_single( + &store, + "192.168.64.32/27", + "vm1", + "192.168.64.1", + "255.255.255.0", + &reserved, + ) + .unwrap(); + let a2 = allocate_single( + &store, + "192.168.64.32/27", + "vm2", + "192.168.64.1", + "255.255.255.0", + &reserved, + ) + .unwrap(); + let a3 = allocate_single( + &store, + "192.168.64.32/27", + "vm3", + "192.168.64.1", + "255.255.255.0", + &reserved, + ) + .unwrap(); + assert_ne!(a1.guest_ip, a2.guest_ip); + assert_ne!(a2.guest_ip, a3.guest_ip); + + release(&store, "vm2").unwrap(); + + let a4 = allocate_single( + &store, + "192.168.64.32/27", + "vm4", + "192.168.64.1", + "255.255.255.0", + &reserved, + ) + .unwrap(); + assert_eq!( + a4.guest_ip, a2.guest_ip, + "freed slot should be reused before allocating beyond the high-water mark" + ); + } + + #[test] + fn allocate_single_rejects_subnet_mismatch_on_reread() { + // allocations.json pins the base subnet so an install can't + // accidentally re-interpret block indices in a different + // layout (e.g. switching between /30 and /32 strategies + // would re-stamp every existing entry). + let (_dir, store) = test_store(); + let reserved = vmnet_reserved(); + allocate_single( + &store, + "192.168.64.32/27", + "vm1", + "192.168.64.1", + "255.255.255.0", + &reserved, + ) + .unwrap(); + let err = allocate_single( + &store, + "192.168.64.64/27", + "vm2", + "192.168.64.1", + "255.255.255.0", + &reserved, + ) + .unwrap_err(); + assert!(matches!(err, Error::Network(msg) if msg.contains("subnet mismatch"))); + } } diff --git a/crates/ember-macos/src/network.rs b/crates/ember-macos/src/network.rs index 76a2c1f..2b5a533 100644 --- a/crates/ember-macos/src/network.rs +++ b/crates/ember-macos/src/network.rs @@ -34,14 +34,16 @@ pub const VMNET_SUBNET: &str = "192.168.64.0/24"; /// /// vmnet's shared subnet is fixed by the framework, so isolation has /// to come from carving up the /24 rather than picking a different -/// one. We split it into 8 /27 slots (32 addresses, 8 /30 VM blocks -/// each) and pick by the low 3 bits of the instance id parsed as -/// hex. The collision probability between two installs is 1/8 per -/// pair — acceptable for personal use, with `--ip-subnet` as the -/// escape hatch when it bites. +/// one. We split it into 8 /27 slots (32 addresses each) and pick by +/// the low 3 bits of the instance id parsed as hex. The collision +/// probability between two installs is 1/8 per pair — acceptable for +/// personal use, with `--ip-subnet` as the escape hatch when it +/// bites. /// -/// 8 VMs per install caps the install at 8 concurrent guests on -/// macOS, well above any realistic personal workflow. +/// Pairs with [`network::ip::allocate_single`](ember_core::network::ip::allocate_single): +/// each /27 slot holds 30–32 single-IP allocations after subtracting +/// vmnet's reserved network/gateway/broadcast addresses, well above +/// any realistic personal workflow. pub fn derive_vmnet_subnet(instance_id: &str) -> String { let id_int = u16::from_str_radix(instance_id, 16).unwrap_or(0); let slot = (id_int & 0b111) as u8; @@ -67,21 +69,47 @@ impl NetworkBackend for MacosNetwork { /// Allocate a static guest IP from the vmnet subnet. /// /// The IP is passed to the kernel via boot args (`ip=::...`) - /// so the guest has connectivity immediately at boot — no DHCP needed. + /// so the guest has connectivity immediately at boot — no DHCP + /// needed. + /// + /// Two allocation strategies, picked by whether the config has + /// an `instance_id`: + /// + /// * Legacy (no instance id): `network::ip::allocate` against the + /// full /24, matching what pre-instance-id binaries wrote so an + /// upgrade doesn't reinterpret existing `allocations.json` + /// block indices. Caps at ~64 VMs (one /30 per VM). + /// * New install: `network::ip::allocate_single` against the + /// per-installation /27 slice. Single-IP allocation gives ~30 + /// VMs per slot — vmnet's shared L2 bridge means the /30 P2P + /// link Linux needs is overkill here and would waste 75% of + /// the address space. fn setup(&self, vm: &VmMetadata, config: &GlobalConfig) -> Result { - // Legacy configs (no instance_id) keep using vmnet's full /24 - // exactly the way pre-instance-id binaries did — same - // allocator state, same IP layout, no migration. New - // configs honor `config.ip_subnet`, which `ember init` - // populates with a per-installation /27 slice via - // `MacosPlatform::default_ip_subnet`. - let default_subnet = if config.instance_id.is_empty() { - VMNET_SUBNET + let allocation = if config.instance_id.is_empty() { + // Per-VM `vm.subnet` overrides the install default; keeps + // parity with pre-instance-id behavior. + let subnet = vm.subnet.as_deref().unwrap_or(VMNET_SUBNET); + network::ip::allocate(&self.store, subnet, &vm.name)? } else { - config.ip_subnet.as_str() + let subnet = vm.subnet.as_deref().unwrap_or(config.ip_subnet.as_str()); + // Reserve vmnet's host-global addresses so the allocator + // never hands them out, regardless of which /27 slot the + // install landed in. .0/.255 are the surrounding /24's + // network/broadcast; .1 is vmnet's built-in router. + let reserved = [ + std::net::Ipv4Addr::new(192, 168, 64, 0), + std::net::Ipv4Addr::new(192, 168, 64, 1), + std::net::Ipv4Addr::new(192, 168, 64, 255), + ]; + network::ip::allocate_single( + &self.store, + subnet, + &vm.name, + VMNET_GATEWAY, + VMNET_NETMASK, + &reserved, + )? }; - let subnet = vm.subnet.as_deref().unwrap_or(default_subnet); - let allocation = network::ip::allocate(&self.store, subnet, &vm.name)?; Ok(NetworkInfo { tap_device: String::new(), From b01716f4f918a0265f9b5adaceee4daaf815185c Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 16 May 2026 08:50:39 +0000 Subject: [PATCH 06/10] config: move per-subsystem name derivation out of GlobalConfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GlobalConfig previously owned the format of the dm-thin pool name, the image/VM thin-volume prefixes, the TAP device prefix, and the iptables comment — five accessors that hard-coded what should be a subsystem-private concern. Each had its own legacy-fallback branch duplicating the same `if instance_id.is_empty()` shape. Replace the five accessors with one generic identity surface, `GlobalConfig::instance_namespace() -> Option<&str>`, that collapses the empty-string legacy sentinel into `None`. Each subsystem owns its own pure derivation: * `dm_thin::pool::name(Option<&str>)` * `dm_thin::thin::{image_prefix, vm_prefix}(Option<&str>)` * `network::tap::prefix(Option<&str>)` * `network::nat::comment(Option<&str>)` The legacy literals (`ember-pool`, `ember-img-`, `ember-vm-`, `em-`, empty iptables comment) live next to the code that has to match them byte-for-byte against existing kernel state, with the "why this exact string" comment alongside. Two related cleanups from the review: * `DmThinStorage::init` was rebuilding the pool name itself with `format!("ember-{}-pool", ...)`, duplicating the `dm_thin_pool_name()` accessor's format and creating a silent drift risk. It now calls `pool::name(Some(...))` — one source of truth. * The macOS vmnet-reserved address list was inlined in `MacosNetwork::setup`; hoist to a module-level `VMNET_RESERVED` const so the three host-global addresses have one definition. * `reconcile::cleanup_network` was a one-line passthrough to `network::cleanup` — inlined the call. 229 ember-core + ember-linux unit tests still pass; integration tests (`tests/isolation.rs`, `tests/dm_thin.rs`) are unchanged and still cover the cross-install isolation contract via the binary. https://claude.ai/code/session_01F51Z281wnRSUHqHPub4msK --- crates/ember-core/src/config.rs | 122 ++++------------------ crates/ember-linux/src/dm_thin.rs | 9 +- crates/ember-linux/src/dm_thin/pool.rs | 32 +++++- crates/ember-linux/src/dm_thin/thin.rs | 47 ++++++++- crates/ember-linux/src/dm_thin_storage.rs | 16 ++- crates/ember-linux/src/network.rs | 9 +- crates/ember-linux/src/network/nat.rs | 30 ++++++ crates/ember-linux/src/network/tap.rs | 37 ++++++- crates/ember-linux/src/network_backend.rs | 9 +- crates/ember-linux/src/platform.rs | 5 +- crates/ember-linux/src/reconcile.rs | 19 +--- crates/ember-macos/src/network.rs | 24 +++-- 12 files changed, 208 insertions(+), 151 deletions(-) diff --git a/crates/ember-core/src/config.rs b/crates/ember-core/src/config.rs index 2f0c355..4e8e86e 100644 --- a/crates/ember-core/src/config.rs +++ b/crates/ember-core/src/config.rs @@ -168,80 +168,22 @@ impl GlobalConfig { format!("{}/{}/vms", self.pool, self.dataset) } - /// dm-thin pool name. New installs get `ember-{instance_id}-pool`; - /// configs that predate `instance_id` (empty value) keep the - /// legacy unprefixed `ember-pool` so existing kernel state on - /// upgraded hosts is still reachable. - pub fn dm_thin_pool_name(&self) -> String { - if self.instance_id.is_empty() { - // Older binaries created `dmsetup create ember-pool`. Any - // other string here would point at a non-existent pool - // (or, worse, a `ember--pool` typo that init would race - // to create), orphaning the data on disk. - "ember-pool".to_string() - } else { - format!("ember-{}-pool", self.instance_id) - } - } - - /// Device-mapper name prefix for image base volumes - /// (`ember-{id}-img-`, or legacy `ember-img-`). - pub fn dm_thin_image_prefix(&self) -> String { - if self.instance_id.is_empty() { - // Existing image volumes on the host are named - // `ember-img-`; teardown's prefix sweep and - // per-image lookups must match that exact prefix or the - // pool will accumulate orphaned thin ids. - "ember-img-".to_string() - } else { - format!("ember-{}-img-", self.instance_id) - } - } - - /// Device-mapper name prefix for VM disks (`ember-{id}-vm-`, or - /// legacy `ember-vm-`). - pub fn dm_thin_vm_prefix(&self) -> String { - if self.instance_id.is_empty() { - // Same reasoning as `dm_thin_image_prefix`: existing VM - // disks are `ember-vm-`, and `vm.json` records the - // device path with that prefix. - "ember-vm-".to_string() - } else { - format!("ember-{}-vm-", self.instance_id) - } - } - - /// TAP device name prefix (`em{id}-`, or legacy `em-`). Bounded so - /// `prefix + 7-hex VM id` fits in Linux's 15-char `IFNAMSIZ - 1` - /// budget (14 chars with the 4-hex id, 10 chars in legacy mode). - pub fn tap_prefix(&self) -> String { - if self.instance_id.is_empty() { - // Older binaries persisted TAP names like `em-` on - // every running VM's `vm.json`; reconcile's orphan sweep - // and teardown's `ip link delete` both reference that - // exact name, so the prefix has to stay 3 chars. - "em-".to_string() - } else { - format!("em{}-", self.instance_id) - } - } - - /// Comment string embedded in iptables rules to scope cleanup to - /// this installation. Returns `""` for legacy configs — the old - /// binary added rules without a comment match, so the rules - /// already on the host don't carry one to match against. - pub fn iptables_comment(&self) -> String { + /// The per-installation namespace, or `None` for legacy configs + /// that predate isolation. + /// + /// This is the only generic identity surface `GlobalConfig` + /// exposes for host-global resource scoping: each subsystem + /// (dm-thin pool/volume names, TAP device prefix, iptables + /// comment) derives its own scoped strings from the namespace + /// rather than asking `GlobalConfig` to know dm-thin or + /// networking trivia. Subsystems handle the `None` case in their + /// own modules so legacy literals live next to the code that + /// reads them. + pub fn instance_namespace(&self) -> Option<&str> { if self.instance_id.is_empty() { - // `iptables -D` requires a byte-for-byte rule match. If - // we returned a non-empty comment here, the upgraded - // binary would emit `... -m comment --comment ember ...` - // on add and try to delete the same shape on remove — - // but the rule already on the host has no comment match, - // so the delete silently no-ops and rules accumulate - // forever. Empty preserves the original rule shape. - String::new() + None } else { - format!("ember:{}", self.instance_id) + Some(&self.instance_id) } } } @@ -296,43 +238,21 @@ mod tests { } #[test] - fn dm_thin_pool_name_embeds_instance_id() { - let cfg = config_with_id("a3f4"); - assert_eq!(cfg.dm_thin_pool_name(), "ember-a3f4-pool"); - assert_eq!(cfg.dm_thin_image_prefix(), "ember-a3f4-img-"); - assert_eq!(cfg.dm_thin_vm_prefix(), "ember-a3f4-vm-"); - } - - #[test] - fn tap_prefix_fits_ifnamsiz_with_7_hex_vm_id() { - let cfg = config_with_id("ffff"); - let prefix = cfg.tap_prefix(); - assert_eq!(prefix, "emffff-"); - // Linux IFNAMSIZ is 16 with NUL; usable budget is 15. The full - // device name is `prefix + 7 hex chars` = 14 chars. - assert!(prefix.len() + 7 <= 15); - } - - #[test] - fn iptables_comment_tags_install() { + fn instance_namespace_returns_id_for_new_install() { let cfg = config_with_id("a3f4"); - assert_eq!(cfg.iptables_comment(), "ember:a3f4"); + assert_eq!(cfg.instance_namespace(), Some("a3f4")); } /// Configs written by older ember binaries don't have `instance_id`. - /// Serde fills the field with the empty-string default; the - /// accessors then have to return the legacy unprefixed names, or - /// we'd lose track of kernel state on every upgrade. + /// Serde fills the field with the empty-string default and + /// `instance_namespace()` collapses that to `None` so subsystems + /// can pattern-match on legacy-vs-tagged in their own modules. #[test] - fn legacy_config_without_instance_id_uses_unprefixed_names() { + fn legacy_config_has_no_instance_namespace() { let json = r#"{"pool":"tank","dataset":"ember","kernel_path":null}"#; let cfg: GlobalConfig = serde_json::from_str(json).unwrap(); assert_eq!(cfg.instance_id, ""); - assert_eq!(cfg.dm_thin_pool_name(), "ember-pool"); - assert_eq!(cfg.dm_thin_image_prefix(), "ember-img-"); - assert_eq!(cfg.dm_thin_vm_prefix(), "ember-vm-"); - assert_eq!(cfg.tap_prefix(), "em-"); - assert_eq!(cfg.iptables_comment(), ""); + assert_eq!(cfg.instance_namespace(), None); // ip_subnet falls back to the historical default subnet. assert_eq!(cfg.ip_subnet, "10.100.0.0/16"); } diff --git a/crates/ember-linux/src/dm_thin.rs b/crates/ember-linux/src/dm_thin.rs index 9a6040a..cce5ba3 100644 --- a/crates/ember-linux/src/dm_thin.rs +++ b/crates/ember-linux/src/dm_thin.rs @@ -3,10 +3,11 @@ //! Thin pools provide block-level copy-on-write storage. A single //! per-installation pool aggregates two backing devices (metadata and //! data) and exposes any number of independent thin volumes addressed -//! by 64-bit numeric IDs. The pool name comes from -//! `GlobalConfig::dm_thin_pool_name()`. Snapshots and clones are the -//! same primitive ([`thin::create_snap`]) — snapshotting a thin volume -//! produces another thin volume that shares blocks until divergence. +//! by 64-bit numeric IDs. The pool name comes from [`pool::name`], +//! which derives from the install's namespace. Snapshots and clones +//! are the same primitive ([`thin::create_snap`]) — snapshotting a +//! thin volume produces another thin volume that shares blocks until +//! divergence. //! //! See `docs/DM-THIN-SPEC.md` for the full design. diff --git a/crates/ember-linux/src/dm_thin/pool.rs b/crates/ember-linux/src/dm_thin/pool.rs index 7188560..2f405a6 100644 --- a/crates/ember-linux/src/dm_thin/pool.rs +++ b/crates/ember-linux/src/dm_thin/pool.rs @@ -3,14 +3,30 @@ //! A thin pool is the kernel-side container holding metadata + data //! devices and exposing thin volumes as snapshot-capable block devices. //! Ember runs one named pool per installation; the pool name is -//! derived from `GlobalConfig::dm_thin_pool_name()` so two installs on -//! the same host don't share a pool. +//! derived from the install's namespace by [`name`] so two installs +//! on the same host don't share a pool. use std::path::{Path, PathBuf}; use std::process::Command; use ember_core::error::{Error, Result}; +/// dm-thin pool name for an installation. +/// +/// `instance_id` is `Some(ns)` for a per-installation pool +/// (`ember-{ns}-pool`) and `None` for legacy configs that predate +/// per-installation isolation. Older binaries created the singleton +/// `ember-pool`, and that exact name must remain reachable across +/// upgrades — any other string in legacy mode would point at a +/// non-existent pool (or, worse, a `ember--pool` typo that init +/// would race to create), orphaning the data on disk. +pub fn name(instance_id: Option<&str>) -> String { + match instance_id { + None => "ember-pool".to_string(), + Some(id) => format!("ember-{id}-pool"), + } +} + /// Default pool block size in 512-byte sectors (= 64 KiB). /// /// Permanent at pool creation. Smaller blocks improve sharing across @@ -392,4 +408,16 @@ mod tests { ); assert_eq!(t, "0 1048576 thin-pool /dev/loop0 /dev/loop1 128 32768"); } + + #[test] + fn name_for_new_install_embeds_namespace() { + assert_eq!(name(Some("a3f4")), "ember-a3f4-pool"); + } + + /// Locked: legacy hosts have a pool named `ember-pool` in the + /// kernel and any other string here would orphan their data. + #[test] + fn name_for_legacy_install_is_unprefixed() { + assert_eq!(name(None), "ember-pool"); + } } diff --git a/crates/ember-linux/src/dm_thin/thin.rs b/crates/ember-linux/src/dm_thin/thin.rs index e78540f..de63e65 100644 --- a/crates/ember-linux/src/dm_thin/thin.rs +++ b/crates/ember-linux/src/dm_thin/thin.rs @@ -182,14 +182,38 @@ pub fn sanitize_dm_name(name: &str) -> String { .collect() } -/// Device-mapper name for a VM volume. `vm_prefix` comes from -/// [`GlobalConfig::dm_thin_vm_prefix`]. +/// Device-mapper name prefix for image base volumes. +/// +/// `Some(ns)` → `ember-{ns}-img-`; `None` → legacy `ember-img-`. +/// Pre-instance-id binaries wrote image volumes as `ember-img-`, +/// and teardown's prefix sweep + per-image lookups must keep matching +/// that exact form or the pool will accumulate orphaned thin ids. +pub fn image_prefix(instance_id: Option<&str>) -> String { + match instance_id { + None => "ember-img-".to_string(), + Some(id) => format!("ember-{id}-img-"), + } +} + +/// Device-mapper name prefix for VM disks. +/// +/// `Some(ns)` → `ember-{ns}-vm-`; `None` → legacy `ember-vm-`. +/// Existing VM disks are recorded on `vm.json` as `ember-vm-`, +/// so legacy mode must keep that exact prefix or the persisted +/// device paths stop resolving. +pub fn vm_prefix(instance_id: Option<&str>) -> String { + match instance_id { + None => "ember-vm-".to_string(), + Some(id) => format!("ember-{id}-vm-"), + } +} + +/// Device-mapper name for a VM volume. pub fn vm_dm_name(vm_prefix: &str, vm_name: &str) -> String { format!("{vm_prefix}{}", sanitize_dm_name(vm_name)) } -/// Device-mapper name for an image base volume. `image_prefix` comes -/// from [`GlobalConfig::dm_thin_image_prefix`]. +/// Device-mapper name for an image base volume. pub fn image_dm_name(image_prefix: &str, image_name: &str) -> String { format!("{image_prefix}{}", sanitize_dm_name(image_name)) } @@ -241,6 +265,21 @@ mod tests { ); } + #[test] + fn prefixes_for_new_install_embed_namespace() { + assert_eq!(image_prefix(Some("a3f4")), "ember-a3f4-img-"); + assert_eq!(vm_prefix(Some("a3f4")), "ember-a3f4-vm-"); + } + + #[test] + fn prefixes_for_legacy_install_match_pre_instance_id_literals() { + // Locked: existing kernel state on upgraded hosts must remain + // reachable, so the legacy literals are part of the + // on-the-wire contract. + assert_eq!(image_prefix(None), "ember-img-"); + assert_eq!(vm_prefix(None), "ember-vm-"); + } + #[test] fn sanitize_keeps_safe_chars() { assert_eq!(sanitize_dm_name("alpine_3.18-edge"), "alpine_3_18-edge"); diff --git a/crates/ember-linux/src/dm_thin_storage.rs b/crates/ember-linux/src/dm_thin_storage.rs index 6b23829..9929d47 100644 --- a/crates/ember-linux/src/dm_thin_storage.rs +++ b/crates/ember-linux/src/dm_thin_storage.rs @@ -95,6 +95,9 @@ impl DmThinStorage { DmThinMode::RawDevice } }); + // dm-thin owns its own name derivation; we just feed it the + // install's namespace (or `None` for legacy configs). + let ns = config.instance_namespace(); Self { storage_path, state_dir: config.state_dir.clone(), @@ -102,9 +105,9 @@ impl DmThinStorage { block_size_sectors: config .dm_thin_block_size .unwrap_or(pool::DEFAULT_BLOCK_SIZE_SECTORS), - pool_name: config.dm_thin_pool_name(), - image_prefix: config.dm_thin_image_prefix(), - vm_prefix: config.dm_thin_vm_prefix(), + pool_name: pool::name(ns), + image_prefix: thin::image_prefix(ns), + vm_prefix: thin::vm_prefix(ns), } } @@ -246,8 +249,11 @@ impl StorageBackend for DmThinStorage { pool::ensure_target_loaded()?; // Pool is named per-installation so two installs on one host - // don't share kernel state. - let pool_name = format!("ember-{}-pool", config.instance_id); + // don't share kernel state. `init` is only ever run on a + // fresh install (the CLI always pins a real `instance_id`), + // so feeding `pool::name` a `Some` here matches what + // `DmThinStorage::new` derives from the persisted config. + let pool_name = pool::name(Some(&config.instance_id)); let block_size_sectors = config .dm_thin_block_size diff --git a/crates/ember-linux/src/network.rs b/crates/ember-linux/src/network.rs index 37f36ea..7f39580 100644 --- a/crates/ember-linux/src/network.rs +++ b/crates/ember-linux/src/network.rs @@ -11,9 +11,10 @@ use ember_core::state::vm::NetworkInfo; /// Best-effort cleanup of networking resources for a VM (Linux only). /// -/// The iptables comment comes from `config.iptables_comment()` so the -/// `-D` calls only match this installation's rules even when another -/// ember install on the same host has rules for the same TAP/IP. +/// The iptables comment is derived via [`nat::comment`] from the +/// install's namespace so the `-D` calls only match this +/// installation's rules even when another ember install on the same +/// host has rules for the same TAP/IP. pub fn cleanup(store: &StateStore, config: &GlobalConfig, vm_name: &str, net_info: &NetworkInfo) { let wan_iface = net_info.wan_iface.clone().or_else(|| wan::detect().ok()); if let Some(wan_iface) = wan_iface { @@ -21,7 +22,7 @@ pub fn cleanup(store: &StateStore, config: &GlobalConfig, vm_name: &str, net_inf &net_info.tap_device, &net_info.guest_ip, &wan_iface, - &config.iptables_comment(), + &nat::comment(config.instance_namespace()), ); } let _ = tap::delete(&net_info.tap_device); diff --git a/crates/ember-linux/src/network/nat.rs b/crates/ember-linux/src/network/nat.rs index c57b268..8a317ec 100644 --- a/crates/ember-linux/src/network/nat.rs +++ b/crates/ember-linux/src/network/nat.rs @@ -13,6 +13,22 @@ use std::process::Command; use ember_core::error::{Error, Result}; +/// iptables comment that scopes rule cleanup to one ember install. +/// +/// `Some(ns)` → `ember:{ns}`, embedded via `-m comment --comment` in +/// every rule so `-D` only matches *this* install's rules. `None` +/// returns the empty string, which [`with_comment`] uses as the +/// signal to omit the `-m comment` match entirely — older binaries +/// added rules without a comment match, so emitting one on legacy +/// installs would make `iptables -D` silently no-op and rules would +/// accumulate forever. Empty preserves the original rule shape. +pub fn comment(instance_id: Option<&str>) -> String { + match instance_id { + None => String::new(), + Some(id) => format!("ember:{id}"), + } +} + /// Add iptables NAT and forwarding rules for a VM. /// /// Creates three rules that together give the guest outbound internet access @@ -149,6 +165,20 @@ fn with_comment<'a>(head: &[&'a str], comment: &'a str, tail: &[&'a str]) -> Vec mod tests { use super::*; + #[test] + fn comment_for_new_install_tags_namespace() { + assert_eq!(comment(Some("a3f4")), "ember:a3f4"); + } + + /// Locked: legacy mode must return an empty string so the rule + /// shape stays byte-for-byte identical to what older binaries + /// emitted (no `-m comment` match), or `iptables -D` silently + /// no-ops on upgraded hosts. + #[test] + fn comment_for_legacy_install_is_empty() { + assert_eq!(comment(None), ""); + } + #[test] fn with_comment_skips_match_when_empty() { // Legacy mode (empty comment) must produce byte-for-byte the diff --git a/crates/ember-linux/src/network/tap.rs b/crates/ember-linux/src/network/tap.rs index c6e6c6c..3574596 100644 --- a/crates/ember-linux/src/network/tap.rs +++ b/crates/ember-linux/src/network/tap.rs @@ -152,12 +152,27 @@ pub fn delete(name: &str) -> Result<()> { Ok(()) } +/// TAP device name prefix for an installation. +/// +/// `Some(ns)` → `em{ns}-`; `None` → legacy `em-`. Bounded so +/// `prefix + 7-hex VM id` fits in Linux's 15-char `IFNAMSIZ - 1` +/// budget (14 chars with the default 4-char namespace, 10 in legacy +/// mode). Pre-instance-id binaries persisted TAP names like +/// `em-` on every running VM's `vm.json`, so the legacy +/// 3-char prefix must stay byte-for-byte stable or reconcile's +/// orphan sweep and teardown's `ip link delete` stop matching. +pub fn prefix(instance_id: Option<&str>) -> String { + match instance_id { + None => "em-".to_string(), + Some(id) => format!("em{id}-"), + } +} + /// List TAP devices on the system whose name starts with `prefix`. /// /// Parses the output of `ip -o link show type tun`. Pass the -/// per-installation TAP prefix from -/// [`GlobalConfig::tap_prefix`](ember_core::config::GlobalConfig::tap_prefix) -/// so reconciliation only sees devices belonging to *this* install. +/// per-installation TAP prefix from [`prefix`] so reconciliation +/// only sees devices belonging to *this* install. pub fn list_devices_with_prefix(prefix: &str) -> Result> { let output = Command::new("ip") .args(["-o", "link", "show", "type", "tun"]) @@ -230,4 +245,20 @@ mod tests { let msg = err.to_string(); assert!(msg.contains("too long"), "unexpected error: {msg}"); } + + #[test] + fn prefix_for_new_install_embeds_namespace() { + let p = prefix(Some("ffff")); + assert_eq!(p, "emffff-"); + // Locks the IFNAMSIZ budget: prefix (7) + 7-hex VM id ≤ 15. + assert!(p.len() + 7 <= 15); + } + + /// Locked at 3 chars: legacy hosts have `em-` TAP names + /// persisted in their `vm.json`, and the orphan sweep + delete + /// paths reference that exact form. + #[test] + fn prefix_for_legacy_install_is_three_chars() { + assert_eq!(prefix(None), "em-"); + } } diff --git a/crates/ember-linux/src/network_backend.rs b/crates/ember-linux/src/network_backend.rs index f3897f0..d637361 100644 --- a/crates/ember-linux/src/network_backend.rs +++ b/crates/ember-linux/src/network_backend.rs @@ -44,8 +44,11 @@ impl NetworkBackend for LinuxNetwork { let subnet = vm.subnet.as_deref().unwrap_or(&config.ip_subnet); let allocation = network::ip::allocate(&self.store, subnet, &vm.name)?; - // Create TAP device. - let tap_name = network::tap::device_name(&config.tap_prefix(), &vm.id); + // Each network subsystem owns its own name derivation; we + // hand them the install's namespace and let them produce the + // strings (legacy fallbacks included). + let ns = config.instance_namespace(); + let tap_name = network::tap::device_name(&network::tap::prefix(ns), &vm.id); let host_ip_cidr = format!("{}/30", allocation.host_ip); if let Err(e) = network::tap::create(&tap_name, &host_ip_cidr) { // Clean up IP allocation on failure. @@ -62,7 +65,7 @@ impl NetworkBackend for LinuxNetwork { // Add iptables NAT/forwarding rules tagged with this install's // comment so cleanup can scope to *this* installation. - let comment = config.iptables_comment(); + let comment = network::nat::comment(ns); if let Err(e) = network::nat::add_rules(&tap_name, &allocation.guest_ip, &wan_iface, &comment) { diff --git a/crates/ember-linux/src/platform.rs b/crates/ember-linux/src/platform.rs index 742e0da..a1395d5 100644 --- a/crates/ember-linux/src/platform.rs +++ b/crates/ember-linux/src/platform.rs @@ -84,7 +84,10 @@ impl Platform for LinuxPlatform { ("Dataset", format!("{}/{}", config.pool, config.dataset)), ], StorageKind::DmThin => { - let mut rows = vec![("dm-thin pool", config.dm_thin_pool_name())]; + let mut rows = vec![( + "dm-thin pool", + crate::dm_thin::pool::name(config.instance_namespace()), + )]; if let Some(ref path) = config.storage_path { rows.push(("Storage path", path.display().to_string())); } diff --git a/crates/ember-linux/src/reconcile.rs b/crates/ember-linux/src/reconcile.rs index aad2e08..432c376 100644 --- a/crates/ember-linux/src/reconcile.rs +++ b/crates/ember-linux/src/reconcile.rs @@ -7,9 +7,9 @@ //! network resources (TAP device, iptables rules, IP allocation). //! //! 2. Find orphaned TAP devices belonging to *this* installation -//! (matched against `GlobalConfig::tap_prefix()`) and delete them. -//! Other ember installs use distinct prefixes, so reconciliation -//! here never touches their devices. +//! (matched against [`network::tap::prefix`] for the install's +//! namespace) and delete them. Other ember installs use distinct +//! prefixes, so reconciliation here never touches their devices. //! //! All operations are best-effort: errors are logged but never propagated, //! since reconciliation should not block normal CLI operation. @@ -86,7 +86,7 @@ pub fn run(state_dir: &Path) { ); if let Some(ref net_info) = metadata.network { if let Some(ref cfg) = config { - cleanup_network(&store, cfg, &metadata.name, net_info); + network::cleanup(&store, cfg, &metadata.name, net_info); } } mark_stopped(&store, &mut metadata); @@ -99,7 +99,7 @@ pub fn run(state_dir: &Path) { let Some(cfg) = config else { return; }; - let prefix = cfg.tap_prefix(); + let prefix = network::tap::prefix(cfg.instance_namespace()); let system_devices = match network::tap::list_devices_with_prefix(&prefix) { Ok(devs) => devs, Err(e) => { @@ -129,12 +129,3 @@ fn mark_stopped(store: &StateStore, metadata: &mut vm::VmMetadata) { } } -/// Best-effort network cleanup for a dead VM. -fn cleanup_network( - store: &StateStore, - config: &GlobalConfig, - vm_name: &str, - net_info: &vm::NetworkInfo, -) { - network::cleanup(store, config, vm_name, net_info); -} diff --git a/crates/ember-macos/src/network.rs b/crates/ember-macos/src/network.rs index 2b5a533..325e633 100644 --- a/crates/ember-macos/src/network.rs +++ b/crates/ember-macos/src/network.rs @@ -30,6 +30,19 @@ pub const VMNET_NETMASK: &str = "255.255.255.0"; /// (a /27 slice) instead. pub const VMNET_SUBNET: &str = "192.168.64.0/24"; +/// vmnet-owned addresses that no guest may receive, regardless of +/// which /27 slot an install lands in: the surrounding /24's network +/// (.0) and broadcast (.255), plus vmnet's built-in router (.1). +/// Addresses outside the install's /27 slot are still listed; the +/// allocator just ignores reservations outside the subnet it's +/// walking, so this single list covers slots 0, 7, and the middle +/// six identically. +const VMNET_RESERVED: [std::net::Ipv4Addr; 3] = [ + std::net::Ipv4Addr::new(192, 168, 64, 0), + std::net::Ipv4Addr::new(192, 168, 64, 1), + std::net::Ipv4Addr::new(192, 168, 64, 255), +]; + /// Derive a per-installation /27 sub-range inside [`VMNET_SUBNET`]. /// /// vmnet's shared subnet is fixed by the framework, so isolation has @@ -92,22 +105,13 @@ impl NetworkBackend for MacosNetwork { network::ip::allocate(&self.store, subnet, &vm.name)? } else { let subnet = vm.subnet.as_deref().unwrap_or(config.ip_subnet.as_str()); - // Reserve vmnet's host-global addresses so the allocator - // never hands them out, regardless of which /27 slot the - // install landed in. .0/.255 are the surrounding /24's - // network/broadcast; .1 is vmnet's built-in router. - let reserved = [ - std::net::Ipv4Addr::new(192, 168, 64, 0), - std::net::Ipv4Addr::new(192, 168, 64, 1), - std::net::Ipv4Addr::new(192, 168, 64, 255), - ]; network::ip::allocate_single( &self.store, subnet, &vm.name, VMNET_GATEWAY, VMNET_NETMASK, - &reserved, + &VMNET_RESERVED, )? }; From 357f810fbe5c66e14a8ea01e89fb4a3ee1124e76 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 16 May 2026 08:54:52 +0000 Subject: [PATCH 07/10] docs: note that we value clear interfaces and avoid abstraction leaks Adds a one-bullet style note under "Coding Style & Conventions" capturing the principle that motivated the previous refactor: subsystems own their own names/formats, shared types like `GlobalConfig` expose generic identity, and reaching across a boundary to format or match another subsystem's strings is a signal to move the logic to the owning side. https://claude.ai/code/session_01F51Z281wnRSUHqHPub4msK --- CLAUDE.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CLAUDE.md b/CLAUDE.md index f452866..48f5830 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -69,6 +69,7 @@ sudo cargo test --test dm_thin -- --ignored --test-threads=1 - Prefer explicit error handling. Use `?` for propagation, not `.unwrap()`. - Shell out to platform CLI tools — no fragile C library bindings. Linux: `zfs`/`zpool`/`iptables`. macOS: `hdiutil`/`diskutil`/`cp -c`/`ember-vz`. +- Value clear interfaces, boundaries, and abstractions; avoid leaks between them. Subsystems own their own formats — dm-thin owns its pool/volume names, networking owns its TAP prefix and iptables comment, and so on. Shared types like `GlobalConfig` expose generic identity (e.g. `instance_namespace()`) and stay free of subsystem trivia. If you find yourself reaching across a boundary to format a name, match a string, or branch on another subsystem's mode, that's the cue to move the logic to the side that owns the concept. ## Architecture From c3920a48d144ac36106ee0ac2fa988e4d9d4a7a3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 16 May 2026 14:17:59 +0000 Subject: [PATCH 08/10] macos: route the legacy/tagged branch through instance_namespace() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MacosNetwork::setup` was the lone holdout still reading `config.instance_id` raw to decide between the legacy /30 allocator and the new /27 single-IP path — every other subsystem goes through `GlobalConfig::instance_namespace()`. Switch to the accessor and match on `Option<&str>` so the legacy-vs-tagged intent reads at the type level, consistent with `dm_thin::pool`, `network::tap`, `network::nat`, etc. No behavior change — `instance_namespace()` collapses the empty instance_id to `None`, which selects the same legacy branch the `is_empty()` check did. https://claude.ai/code/session_01DdHPXvcXCZagRRDobyPbvi --- crates/ember-macos/src/network.rs | 33 +++++++++++++++++-------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/crates/ember-macos/src/network.rs b/crates/ember-macos/src/network.rs index 325e633..db0dc5d 100644 --- a/crates/ember-macos/src/network.rs +++ b/crates/ember-macos/src/network.rs @@ -98,21 +98,24 @@ impl NetworkBackend for MacosNetwork { /// link Linux needs is overkill here and would waste 75% of /// the address space. fn setup(&self, vm: &VmMetadata, config: &GlobalConfig) -> Result { - let allocation = if config.instance_id.is_empty() { - // Per-VM `vm.subnet` overrides the install default; keeps - // parity with pre-instance-id behavior. - let subnet = vm.subnet.as_deref().unwrap_or(VMNET_SUBNET); - network::ip::allocate(&self.store, subnet, &vm.name)? - } else { - let subnet = vm.subnet.as_deref().unwrap_or(config.ip_subnet.as_str()); - network::ip::allocate_single( - &self.store, - subnet, - &vm.name, - VMNET_GATEWAY, - VMNET_NETMASK, - &VMNET_RESERVED, - )? + let allocation = match config.instance_namespace() { + None => { + // Per-VM `vm.subnet` overrides the install default; keeps + // parity with pre-instance-id behavior. + let subnet = vm.subnet.as_deref().unwrap_or(VMNET_SUBNET); + network::ip::allocate(&self.store, subnet, &vm.name)? + } + Some(_) => { + let subnet = vm.subnet.as_deref().unwrap_or(config.ip_subnet.as_str()); + network::ip::allocate_single( + &self.store, + subnet, + &vm.name, + VMNET_GATEWAY, + VMNET_NETMASK, + &VMNET_RESERVED, + )? + } }; Ok(NetworkInfo { From 12ab2ecdaad830609248efae18b3e1cc00c3b9d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 16 May 2026 14:18:26 +0000 Subject: [PATCH 09/10] config: move /16-in-10/8 subnet derivation to the linux network crate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `derive_ip_subnet` lived in `ember_core::config` but was only ever called from `LinuxPlatform::default_ip_subnet`. macOS has its own derivation (`derive_vmnet_subnet` in `ember-macos`) that carves a /27 out of vmnet's fixed /24, so keeping the Linux-specific function in core was the opposite of the boundary discipline this branch is trying to land: shared types stay generic; platforms own their own scoped derivations. Move it to `ember_linux::network::ip::derive_default_subnet`, alongside the `network::ip::allocate`/`release` it pairs with. Expose `ember_core::config::fnv1a_32` as `pub` so the platform crate can hash the instance id with the same primitive `derive_instance_id` (still in core, still platform-agnostic) uses — duplicating a 7-line hash would be sillier than publishing a stable utility. `network.rs`'s `pub use ember_core::network::ip` re-export becomes a local `pub mod ip` that re-exports the same surface (`allocate`, `allocate_single`, `release`, `IpAllocation`, …) and adds the Linux-only `derive_default_subnet`. Every existing `network::ip::X` call site keeps resolving — the new module pulls core's public items through `pub use ember_core::network::ip::*`. https://claude.ai/code/session_01DdHPXvcXCZagRRDobyPbvi --- crates/ember-core/src/config.rs | 23 ++++----------- crates/ember-linux/src/network.rs | 3 +- crates/ember-linux/src/network/ip.rs | 44 ++++++++++++++++++++++++++++ crates/ember-linux/src/platform.rs | 4 +-- 4 files changed, 52 insertions(+), 22 deletions(-) create mode 100644 crates/ember-linux/src/network/ip.rs diff --git a/crates/ember-core/src/config.rs b/crates/ember-core/src/config.rs index 4e8e86e..79bd57f 100644 --- a/crates/ember-core/src/config.rs +++ b/crates/ember-core/src/config.rs @@ -135,20 +135,14 @@ pub fn derive_instance_id(state_dir: &std::path::Path) -> String { format!("{:04x}", fnv1a_32(bytes) as u16) } -/// Derive a default `/16` IPv4 subnet from an instance id, chosen so -/// two installations rarely overlap: `10.{slot}.0.0/16` where `slot` -/// is the high byte of the same FNV-1a hash that produced the id. The -/// /16 still gives ~16k VMs per install via /30 links. -pub fn derive_ip_subnet(instance_id: &str) -> String { - let hash = fnv1a_32(instance_id.as_bytes()); - let slot = ((hash >> 8) & 0xff) as u8; - format!("10.{slot}.0.0/16") -} - /// FNV-1a 32-bit hash. Stable across Rust versions (unlike /// `DefaultHasher`) and small enough to inline rather than pulling in /// a crypto dep just for non-security-critical name derivation. -fn fnv1a_32(bytes: &[u8]) -> u32 { +/// +/// Exposed for platform crates that derive their own scoped names +/// from the instance id (e.g. Linux's `/16`-in-`10.0.0.0/8` subnet +/// slot). +pub fn fnv1a_32(bytes: &[u8]) -> u32 { let mut h: u32 = 0x811c_9dc5; for &b in bytes { h ^= b as u32; @@ -230,13 +224,6 @@ mod tests { assert_ne!(a, b); } - #[test] - fn derived_subnet_lands_in_10_slash_8() { - let subnet = derive_ip_subnet("a3f4"); - assert!(subnet.starts_with("10.")); - assert!(subnet.ends_with(".0.0/16")); - } - #[test] fn instance_namespace_returns_id_for_new_install() { let cfg = config_with_id("a3f4"); diff --git a/crates/ember-linux/src/network.rs b/crates/ember-linux/src/network.rs index 7f39580..dfa27f5 100644 --- a/crates/ember-linux/src/network.rs +++ b/crates/ember-linux/src/network.rs @@ -1,6 +1,5 @@ -pub use ember_core::network::ip; - pub mod dns; +pub mod ip; pub mod nat; pub mod tap; pub mod wan; diff --git a/crates/ember-linux/src/network/ip.rs b/crates/ember-linux/src/network/ip.rs new file mode 100644 index 0000000..be97776 --- /dev/null +++ b/crates/ember-linux/src/network/ip.rs @@ -0,0 +1,44 @@ +//! Linux-side IP allocation surface. +//! +//! Re-exports the shared `allocate`/`release` allocator from +//! `ember_core::network::ip` and adds the Linux-specific default-subnet +//! derivation that picks a `/16` slot inside `10.0.0.0/8` from the +//! installation's instance id. macOS has its own derivation that +//! sub-allocates inside vmnet's fixed `192.168.64.0/24` and lives in +//! the `ember-macos` crate. + +pub use ember_core::network::ip::*; + +use ember_core::config::fnv1a_32; + +/// Derive a default `/16` IPv4 subnet from an instance id, chosen so +/// two installations on the same host rarely overlap: +/// `10.{slot}.0.0/16` where `slot` is the high byte of an FNV-1a hash +/// of the id. The /16 still gives ~16k VMs per install via /30 P2P +/// links — well above any realistic personal-use workload. +pub fn derive_default_subnet(instance_id: &str) -> String { + let hash = fnv1a_32(instance_id.as_bytes()); + let slot = ((hash >> 8) & 0xff) as u8; + format!("10.{slot}.0.0/16") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn derived_subnet_lands_in_10_slash_8() { + let subnet = derive_default_subnet("a3f4"); + assert!(subnet.starts_with("10.")); + assert!(subnet.ends_with(".0.0/16")); + } + + #[test] + fn derivation_is_stable() { + // Reactivation must land on the same /16 the install was + // initialized with, otherwise allocations.json's pinned + // `base_subnet` mismatches and `allocate` refuses to hand out + // IPs. + assert_eq!(derive_default_subnet("a3f4"), derive_default_subnet("a3f4")); + } +} diff --git a/crates/ember-linux/src/platform.rs b/crates/ember-linux/src/platform.rs index a1395d5..85c4b41 100644 --- a/crates/ember-linux/src/platform.rs +++ b/crates/ember-linux/src/platform.rs @@ -1,7 +1,7 @@ use std::path::{Path, PathBuf}; use ember_core::backend::{ImageToolConfig, Platform, ResolvConfMode}; -use ember_core::config::{derive_ip_subnet, GlobalConfig, StorageKind}; +use ember_core::config::{GlobalConfig, StorageKind}; use ember_core::error::Result; use ember_core::image::registry::ImageEntry; use ember_core::state::vm::VmMetadata; @@ -24,7 +24,7 @@ impl Platform for LinuxPlatform { } fn default_ip_subnet(instance_id: &str) -> String { - derive_ip_subnet(instance_id) + crate::network::ip::derive_default_subnet(instance_id) } fn console_device() -> &'static str { From 62250d9e63fc2b40bda50c075114ab047433d9d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 16 May 2026 14:18:46 +0000 Subject: [PATCH 10/10] macos: panic instead of silent slot-0 fallback on a non-hex instance_id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `derive_vmnet_subnet`'s `unwrap_or(0)` was defensive against a hand-edited config, but the function is only called from `MacosPlatform::default_ip_subnet` at `ember init` — never at runtime — and the `instance_id` it gets there is either CLI-validated (`parse_instance_id` enforces 4 lowercase hex) or auto-derived (`derive_instance_id` always emits 4 hex chars). The fallback was unreachable, but in the hypothetical world where validation regresses it would silently collide every affected install on slot 0 — exactly the cross-install clash this branch exists to prevent. Switch to a `panic!` with a message that names the contract ("CLI validation should have rejected this"), and replace the `falls_back_to_slot_zero_on_garbage_id` test with two `#[should_panic]` cases. The test that was locking in the silent fallback is now locking in the loud one. Also drops a trailing blank line in reconcile.rs that `cargo fmt` flagged in passing. https://claude.ai/code/session_01DdHPXvcXCZagRRDobyPbvi --- crates/ember-linux/src/reconcile.rs | 1 - crates/ember-macos/src/network.rs | 32 ++++++++++++++++++++++------- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/crates/ember-linux/src/reconcile.rs b/crates/ember-linux/src/reconcile.rs index 432c376..5abcac1 100644 --- a/crates/ember-linux/src/reconcile.rs +++ b/crates/ember-linux/src/reconcile.rs @@ -128,4 +128,3 @@ fn mark_stopped(store: &StateStore, metadata: &mut vm::VmMetadata) { ); } } - diff --git a/crates/ember-macos/src/network.rs b/crates/ember-macos/src/network.rs index db0dc5d..598e9ef 100644 --- a/crates/ember-macos/src/network.rs +++ b/crates/ember-macos/src/network.rs @@ -58,7 +58,18 @@ const VMNET_RESERVED: [std::net::Ipv4Addr; 3] = [ /// vmnet's reserved network/gateway/broadcast addresses, well above /// any realistic personal workflow. pub fn derive_vmnet_subnet(instance_id: &str) -> String { - let id_int = u16::from_str_radix(instance_id, 16).unwrap_or(0); + // Only called from `MacosPlatform::default_ip_subnet` at + // `ember init`, where `instance_id` is either CLI-validated + // (`parse_instance_id` enforces 4 lowercase hex) or auto-derived + // (`derive_instance_id` always emits 4 hex chars). A non-hex id + // means upstream validation broke — panic so the bug is loud + // rather than silently colliding two installs on slot 0. + let id_int = u16::from_str_radix(instance_id, 16).unwrap_or_else(|_| { + panic!( + "derive_vmnet_subnet got non-hex instance_id {instance_id:?} — \ + CLI validation should have rejected this" + ) + }); let slot = (id_int & 0b111) as u8; let base = slot * 32; format!("192.168.64.{base}/27") @@ -261,11 +272,18 @@ destination: default } #[test] - fn vmnet_subnet_falls_back_to_slot_zero_on_garbage_id() { - // Defensive: parse failure shouldn't panic, just land on - // the legacy-equivalent base. The user can then pick - // `--ip-subnet` explicitly. - assert_eq!(derive_vmnet_subnet("zzzz"), "192.168.64.0/27"); - assert_eq!(derive_vmnet_subnet(""), "192.168.64.0/27"); + #[should_panic(expected = "non-hex instance_id")] + fn vmnet_subnet_panics_on_non_hex_id() { + // CLI validation guarantees a 4-hex `instance_id`; a non-hex + // value reaching this function means validation regressed. + // We'd rather panic loudly than silently collide every such + // install on slot 0. + let _ = derive_vmnet_subnet("zzzz"); + } + + #[test] + #[should_panic(expected = "non-hex instance_id")] + fn vmnet_subnet_panics_on_empty_id() { + let _ = derive_vmnet_subnet(""); } }