diff --git a/ADMIN_API_ENDPOINTS.md b/ADMIN_API_ENDPOINTS.md index b0cf2fbc..c33a6a17 100644 --- a/ADMIN_API_ENDPOINTS.md +++ b/ADMIN_API_ENDPOINTS.md @@ -1769,11 +1769,10 @@ Body: // optional - if not provided, cost plan will be auto-created "region_id": number, "ip4_count": number, - // optional - IPv4 addresses included in this offer, default 1. Capped at 1 - // until the guest network config can carry more than one address per family + // optional - IPv4 addresses included in this offer, default 1 "ip6_count": number, - // optional - IPv6 addresses included in this offer, default 1, capped at 1. - // Assignment is best-effort: a region with no IPv6 range still provisions + // optional - IPv6 addresses included in this offer, default 1. Assignment is + // best-effort: a region with no IPv6 range still provisions // Cost plan auto-creation fields (used when cost_plan_id not provided) "cost_plan_name": "string", @@ -2042,12 +2041,11 @@ Body: "min_ip4": number, // optional - Minimum IPv4 addresses selectable, default 1 "max_ip4": number, - // optional - Maximum IPv4 addresses selectable, default 1. Capped at 1 until - // the guest network config can carry more than one address per family + // optional - Maximum IPv4 addresses selectable, default 1 "min_ip6": number, // optional - Minimum IPv6 addresses selectable, default 1 "max_ip6": number, - // optional - Maximum IPv6 addresses selectable, default 1, capped at 1 + // optional - Maximum IPv6 addresses selectable, default 1 "disk_iops_read": number, // optional - Maximum disk read IOPS (omit or null = uncapped) "disk_iops_write": number, @@ -2120,12 +2118,11 @@ Body (all optional): "min_ip4": number, // optional - Minimum IPv4 addresses selectable, default 1 "max_ip4": number, - // optional - Maximum IPv4 addresses selectable, default 1. Capped at 1 until - // the guest network config can carry more than one address per family + // optional - Maximum IPv4 addresses selectable, default 1 "min_ip6": number, // optional - Minimum IPv6 addresses selectable, default 1 "max_ip6": number, - // optional - Maximum IPv6 addresses selectable, default 1, capped at 1 + // optional - Maximum IPv6 addresses selectable, default 1 "disk_iops_read": "number | null", // Maximum disk read IOPS - send null to clear "disk_iops_write": "number | null", @@ -4852,12 +4849,11 @@ The RBAC system uses the following permission format: `resource::action` "min_ip4": number, // optional - Minimum IPv4 addresses selectable, default 1 "max_ip4": number, - // optional - Maximum IPv4 addresses selectable, default 1. Capped at 1 until - // the guest network config can carry more than one address per family + // optional - Maximum IPv4 addresses selectable, default 1 "min_ip6": number, // optional - Minimum IPv6 addresses selectable, default 1 "max_ip6": number, - // optional - Maximum IPv6 addresses selectable, default 1, capped at 1 + // optional - Maximum IPv6 addresses selectable, default 1 "disk_iops_read": number, // Maximum disk read IOPS - omitted when uncapped "disk_iops_write": number, diff --git a/API_CHANGELOG.md b/API_CHANGELOG.md index cc6611a9..2b5ac2f5 100644 --- a/API_CHANGELOG.md +++ b/API_CHANGELOG.md @@ -28,6 +28,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - **Admin can create a custom-spec VM** (issue #335) — `POST /api/admin/v1/vms/custom` creates a VM from an arbitrary spec for any user, billed against a custom pricing plan, instead of only from the fixed template catalog. Body is the customer custom-order shape plus `user_id`/`reason`: `pricing_id` (also decides the region), `cpu`, `memory` and `disk` in bytes, `disk_type`, `disk_interface`, optional `cpu_mfg`/`cpu_arch`/`cpu_feature`, `image_id`, `ssh_key_id` (must belong to the target user), optional `ref_code`. Returns `{ "job_id": string }`; the VM is created unpaid like a customer order. Unknown enum spellings are `400` and are never defaulted. Additive: `POST /api/admin/v1/vms` (template path) is unchanged. +- **A VM can hold more than one address per family** (issue #343) — every assignment is now placed on the VM's single NIC through a per-VM cloud-init network config (`cicustom network=`), instead of the built-in `ipconfig`, which carries only one IPv4 and one IPv6 per interface. A VM with one address per family is unaffected and keeps its existing `ipconfig`, so nothing already deployed is reconfigured. The interface is matched by MAC so the config does not depend on guest device naming; one default route per family is emitted (the first assignment's gateway wins, as before); a SLAAC range contributes router advertisement rather than a pinned address. No response shape changed — the offered maximum is now simply whatever an operator sets on the plan or template. + - **IP counts are a sellable dimension of an offer** (issue #105) — a VM offer now states how many addresses it includes instead of implying exactly one IPv4 plus an optional IPv6. - `ApiVmTemplate` (in `GET /api/v1/vm/templates`, `GET /api/v1/vm/{id}` and every VM listing) gains `ip4_count` and `ip6_count`. Additive; nothing existing changed shape. - `GET /api/v1/vm/custom-template` params gain `min_ip4`/`max_ip4`/`min_ip6`/`max_ip6`. `max_ip4` is already capped by what the region can actually supply, so a client can use it directly as a selector bound, and a plan whose `min_ip4` cannot be met is not listed at all. @@ -35,7 +37,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Custom VM billing now takes the counts from the ordered spec rather than counting the VM's current assignments. Existing VMs are migrated to the counts they currently hold, so no live renewal amount moves. - Provisioning allocates the stated number of addresses across the region's ranges. IPv4 is all-or-nothing: an order asking for addresses the region cannot supply fails instead of quietly getting fewer. IPv6 remains best-effort, unchanged — a region without an IPv6 range still provisions, and a SLAAC range yields one address per VM. - Admin: `ip4_count`/`ip6_count` on VM template create/update/read, and `min_ip4`/`max_ip4`/`min_ip6`/`max_ip6` on custom pricing create/update/read/copy. All default to 1, so no plan starts offering a choice until an operator widens it. - - **The offered maximum is capped at 1 address per family for now** — a guest gets one cloud-init `ipconfig` entry, which carries a single IPv4 and a single IPv6, so a wider offer would allocate and bill for addresses that never reach the VM. `ip4_count`/`ip6_count` above 1 on a template, and `max_ip4`/`max_ip6` above 1 on a custom pricing plan, are refused with a `400`, and an order for more than 1 is refused even if a stored plan's range would allow it. The cap lifts once the guest-side network config can carry more than one address per family (issue #343). - **`host_ssh_keys` on VM status** (issue #154) — `GET /api/v1/vm/{id}` and `GET /api/v1/vm` now return the VM's own SSH host keys as `[{ key_type, public_key, fingerprint_sha256 }]`, so a client can verify the host on first connect instead of accepting whatever key answers. The list is empty until the keys are captured (scanned from the host once the VM is running, public keys only) and is re-captured after a reinstall, which regenerates them. Additive: no existing field changed. Not to be confused with `ssh_key`, which is the customer's authorized key. diff --git a/lnvps_api/src/host/proxmox.rs b/lnvps_api/src/host/proxmox.rs index 574e4347..b892e595 100644 --- a/lnvps_api/src/host/proxmox.rs +++ b/lnvps_api/src/host/proxmox.rs @@ -346,49 +346,42 @@ impl ProxmoxClient { /// /// This snippet disables SSH host key regeneration so that cloud-init /// reconfiguration (IP changes, SSH key updates, etc.) does not cause - /// host-key warnings for users connecting via SSH. + /// host-key warnings for users connecting via SSH, and pins the guest + /// resolvers, which minimal images otherwise drop. /// - /// The snippet is written once via SSH to the storage's snippet directory. /// Returns the Proxmox volume reference (e.g. `local:snippets/lnvps-vendor.yaml`) /// or `None` if SSH is not configured or no snippet storage is available. async fn ensure_vendor_snippet(&self) -> OpResult> { + self.write_snippet("lnvps-vendor.yaml", &build_vendor_snippet(GUEST_DNS_SERVERS)) + .await + } + + /// Resolve the on-disk path of a snippet volume, writing it only when the + /// content differs. Returns the volume reference to put in `cicustom`. + async fn write_snippet(&self, filename: &str, content: &str) -> OpResult> { let ssh_config = match &self.ssh { Some(s) => s, None => return Ok(None), }; - let storage_name = match self.get_snippet_storage(&self.node).await? { Some(s) => s, None => return Ok(None), }; - let snippet_filename = "lnvps-vendor.yaml"; - // Snippet storage path depends on the storage type; for the default - // `local` storage this is `/var/lib/vz/snippets/`. For other directory- - // based storages it varies. We use `pvesm path` to resolve it. + // `local` storage this is `/var/lib/vz/snippets/`. For other directory- + // based storages it varies, so `pvesm path` resolves it. let host = self.api.base().host().unwrap().to_string(); - let ssh_user = ssh_config.user.clone(); - let ssh_key = ssh_config.key.clone(); - let mut ssh = SshClient::new(); - ssh.connect((host.clone(), 22), &ssh_user, &ssh_key) + ssh.connect((host, 22), &ssh_config.user, &ssh_config.key) .await .map_err(OpError::Transient)?; - // Force a fixed set of public resolvers into every guest's - // /etc/resolv.conf via cloud-init. Without this, minimal images - // (Alpine, NixOS) silently drop the DNS servers Proxmox provides. - let snippet_content = build_vendor_snippet(GUEST_DNS_SERVERS); - - let vol_ref = format!("{storage_name}:snippets/{snippet_filename}"); - - // Resolve the on-disk path for the snippet volume reference + let vol_ref = format!("{storage_name}:snippets/{filename}"); let (exit_code, path_output) = ssh .execute(&format!("pvesm path '{vol_ref}'")) .await .map_err(OpError::Transient)?; - if exit_code != 0 { info!( "Cannot resolve snippet path for {}: {}", @@ -397,43 +390,99 @@ impl ProxmoxClient { ); return Ok(None); } - let snippet_path = path_output.trim(); + let snippet_path = path_output.trim().to_string(); - // Ensure parent directory exists and write only if missing or changed let (_, existing) = ssh .execute(&format!("cat '{snippet_path}' 2>/dev/null || true")) .await .map_err(OpError::Transient)?; - - if existing.trim() != snippet_content.trim() { + if existing.trim() != content.trim() { let parent = snippet_path .rsplit_once('/') .map(|(p, _)| p) - .unwrap_or("/tmp"); + .unwrap_or("/tmp") + .to_string(); ssh.execute(&format!("mkdir -p '{parent}'")) .await .map_err(OpError::Transient)?; + // Uploaded rather than echoed through a shell: the content is + // multi-line YAML and quoting it into a command is a foot-gun. + ssh.scp_upload( + content.as_bytes(), + std::path::Path::new(&snippet_path), + 0o644, + ) + .await + .map_err(OpError::Transient)?; + info!("Wrote cloud-init snippet to {}", snippet_path); + } + Ok(Some(vol_ref)) + } - let (code, output) = ssh - .execute(&format!( - "printf '%s' '{}' > '{}'", - snippet_content, snippet_path - )) - .await - .map_err(OpError::Transient)?; + /// Snippet filename holding a VM's network config, if it needs one. + fn network_snippet_filename(vm_id: u64) -> String { + format!("lnvps-net-{vm_id}.yaml") + } - if code != 0 { - info!( - "Failed to write vendor snippet to {}: {}", - snippet_path, - output.trim() - ); - return Ok(None); - } - info!("Wrote cloud-init vendor snippet to {}", snippet_path); + /// Write the VM's network snippet, if it needs one. + /// + /// Failing to write one the VM needs is fatal rather than a downgrade: every + /// address is already allocated, routed and billed, and carrying on would + /// hand the customer a VM holding one of them. + async fn ensure_network_snippet(&self, cfg: &FullVmInfo) -> OpResult> { + let content = match Self::make_network_config(cfg).map_err(OpError::Fatal)? { + Some(c) => c, + None => return Ok(None), + }; + match self + .write_snippet(&Self::network_snippet_filename(cfg.vm.id), &content) + .await? + { + Some(vol_ref) => Ok(Some(vol_ref)), + None => op_fatal!( + "VM {} needs a cloud-init network snippet but none could be written; \ + snippet storage and SSH access are required to configure more than \ + one address per family", + cfg.vm.id + ), } + } - Ok(Some(vol_ref)) + /// Remove a VM's network snippet, if one was ever written. + /// + /// A VM cannot start when a `cicustom` volume it references is missing, so + /// this only ever runs after the VM itself is gone. + async fn remove_network_snippet(&self, vm_id: u64) -> OpResult<()> { + let ssh_config = match &self.ssh { + Some(s) => s, + None => return Ok(()), + }; + let storage_name = match self.get_snippet_storage(&self.node).await? { + Some(s) => s, + None => return Ok(()), + }; + let host = self.api.base().host().unwrap().to_string(); + let mut ssh = SshClient::new(); + ssh.connect((host, 22), &ssh_config.user, &ssh_config.key) + .await + .map_err(OpError::Transient)?; + + let vol_ref = format!( + "{storage_name}:snippets/{}", + Self::network_snippet_filename(vm_id) + ); + let (exit_code, path_output) = ssh + .execute(&format!("pvesm path '{vol_ref}'")) + .await + .map_err(OpError::Transient)?; + if exit_code != 0 { + return Ok(()); + } + let snippet_path = path_output.trim(); + ssh.execute(&format!("rm -f '{snippet_path}'")) + .await + .map_err(OpError::Transient)?; + Ok(()) } pub async fn import_disk_image(&self, req: ImportDiskImageRequest) -> OpResult<()> { @@ -980,7 +1029,96 @@ impl ProxmoxClient { .collect() } - fn make_config(&self, value: &FullVmInfo, vendor_snippet: Option<&str>) -> Result { + /// Build a cloud-init network-config (v2) putting every assignment on the + /// VM's single NIC. + /// + /// Returns `None` when the VM holds at most one address per family: a single + /// `ipconfig0` expresses that exactly, and leaving those VMs on the built-in + /// path keeps their config untouched. Above that, `ipconfig[n]` cannot help — + /// it carries one `ip=` and one `ip6=` per interface — and a second NIC is not + /// an option either, because the addresses share one router-issued vMAC and + /// must therefore share the interface carrying it. + /// + /// The interface is matched by MAC rather than by name so the config does not + /// depend on how the guest enumerates devices. + fn make_network_config(value: &FullVmInfo) -> Result> { + let mut v4 = Vec::new(); + let mut v6 = Vec::new(); + let mut gw4 = None; + let mut gw6 = None; + let mut accept_ra = false; + + for ip in &value.ips { + let addr: IpAddr = match ip.ip.parse() { + Ok(a) => a, + Err(_) => continue, + }; + let ip_range = match value.ranges.iter().find(|r| r.id == ip.ip_range_id) { + Some(r) => r, + None => continue, + }; + if addr.is_ipv6() + && matches!(ip_range.allocation_mode, IpRangeAllocationMode::SlaacEui64) + { + // The host hands out the address; what the database holds is + // informational only. + accept_ra = true; + continue; + } + let range: IpNetwork = ip_range.cidr.parse()?; + let range_gw: IpNetwork = parse_gateway(&ip_range.gateway)?; + // Same widening as the ipconfig path: take the shorter prefix so an + // off-subnet gateway stays directly reachable without an on-link route. + let prefix = range.prefix().min(range_gw.prefix()); + let cidr = IpNetwork::new(addr, prefix)?.to_string(); + if addr.is_ipv4() { + // Only one default route per family is meaningful; addresses from + // a second range stay reachable on-link through the widened + // prefix. First assignment wins, as it does via `ipconfig`. + gw4.get_or_insert_with(|| range_gw.ip()); + v4.push(cidr); + } else { + gw6.get_or_insert_with(|| range_gw.ip()); + v6.push(cidr); + } + } + + if v4.len() <= 1 && v6.len() <= 1 { + return Ok(None); + } + + let mut cfg = + String::from("version: 2\nethernets:\n nic0:\n match:\n macaddress: "); + cfg.push_str(&value.vm.mac_address.to_lowercase()); + cfg.push_str("\n dhcp4: false\n dhcp6: false\n"); + if accept_ra { + cfg.push_str(" accept-ra: true\n"); + } + cfg.push_str(" addresses:\n"); + for a in v4.iter().chain(v6.iter()) { + cfg.push_str(&format!(" - {a}\n")); + } + let routes: Vec = gw4 + .into_iter() + .chain(gw6) + .map(|gw| format!(" - to: default\n via: {gw}")) + .collect(); + if !routes.is_empty() { + cfg.push_str(" routes:\n"); + for r in &routes { + cfg.push_str(r); + cfg.push('\n'); + } + } + Ok(Some(cfg)) + } + + fn make_config( + &self, + value: &FullVmInfo, + vendor_snippet: Option<&str>, + network_snippet: Option<&str>, + ) -> Result { let ip_config = value .ips .iter() @@ -1023,6 +1161,23 @@ impl ProxmoxClient { }) .collect::>(); + // `ipconfig[n]` is a property string holding at most one `ip=` and one + // `ip6=`; repeating a key produces something the guest cannot act on. Any + // address beyond the first of each family is carried by the network + // snippet instead, so keep only the first of each here. + let ip_config: Vec = { + let mut first_v4 = None; + let mut first_v6 = None; + for entry in ip_config { + if entry.starts_with("ip6=") { + first_v6.get_or_insert(entry); + } else { + first_v4.get_or_insert(entry); + } + } + first_v4.into_iter().chain(first_v6).collect() + }; + let mut net = vec![ format!("virtio={}", value.vm.mac_address), format!("bridge={}", self.config.bridge), @@ -1043,7 +1198,16 @@ impl ProxmoxClient { net.push(format!("rate={}", mbps as f32 / 8.0)); } - let cicustom = vendor_snippet.map(|vol_ref| format!("vendor={vol_ref}")); + let cicustom_parts: Vec = vendor_snippet + .map(|v| format!("vendor={v}")) + .into_iter() + .chain(network_snippet.map(|n| format!("network={n}"))) + .collect(); + let cicustom = if cicustom_parts.is_empty() { + None + } else { + Some(cicustom_parts.join(",")) + }; let vm_resources = value.resources()?; Ok(VmConfig { @@ -1612,7 +1776,9 @@ impl VmHostClient for ProxmoxClient { async fn create_vm(&self, req: &FullVmInfo) -> OpResult<()> { let vendor_snippet = self.ensure_vendor_snippet().await?; - let config = self.make_config(req, vendor_snippet.as_deref())?; + let network_snippet = self.ensure_network_snippet(req).await?; + let config = + self.make_config(req, vendor_snippet.as_deref(), network_snippet.as_deref())?; let vm_id: ProxmoxVmId = req.vm.id.into(); let ctx = CreateVmContext { @@ -1678,7 +1844,13 @@ impl VmHostClient for ProxmoxClient { } async fn delete_vm(&self, vm: &Vm) -> OpResult<()> { - self.destroy_vm(vm.id.into()).await + self.destroy_vm(vm.id.into()).await?; + // Best-effort: a leaked snippet wastes a few hundred bytes, while failing + // here would leave the caller believing a destroyed VM still exists. + if let Err(e) = self.remove_network_snippet(vm.id).await { + warn!("Failed to remove network snippet for VM {}: {}", vm.id, e); + } + Ok(()) } async fn unlink_primary_disk(&self, vm: &Vm) -> OpResult<()> { @@ -1734,7 +1906,11 @@ impl VmHostClient for ProxmoxClient { let current_config = self.get_vm_config(&self.node, cfg.vm.id.into()).await?; let vendor_snippet = self.ensure_vendor_snippet().await?; - let mut config = self.make_config(cfg, vendor_snippet.as_deref())?; + // Rewrites the file when the assignments changed, so the volume ref can + // compare equal while the config behind it is new. + let network_snippet = self.ensure_network_snippet(cfg).await?; + let mut config = + self.make_config(cfg, vendor_snippet.as_deref(), network_snippet.as_deref())?; // dont re-create the disks config.scsi_0 = None; @@ -1768,7 +1944,8 @@ impl VmHostClient for ProxmoxClient { // Check and fix cloud-init IP config if it doesn't match expected let current_config = self.get_vm_config(&self.node, vm_id).await?; - let expected_config = self.make_config(cfg, None)?; + let network_snippet = self.ensure_network_snippet(cfg).await?; + let expected_config = self.make_config(cfg, None, network_snippet.as_deref())?; if current_config.config.ip_config != expected_config.ip_config { info!( "IP config mismatch for VM {}: current={:?}, expected={:?}", @@ -2973,7 +3150,7 @@ mod tests { None, ); - let vm = p.make_config(&cfg, None)?; + let vm = p.make_config(&cfg, None, None)?; assert_eq!(vm.cpu, Some(q_cfg.cpu)); assert_eq!(vm.cores, Some(template.cpu as i32)); assert_eq!(vm.memory, Some((template.memory / MB).to_string())); @@ -2982,12 +3159,11 @@ mod tests { assert_eq!(vm.on_boot, Some(true)); assert!(vm.net.as_ref().unwrap().contains("tag=100")); assert!(vm.net.as_ref().unwrap().contains("firewall=1")); + // One address per family: the fixture holds two IPv4s, and repeating + // `ip=` in the property string is not something the guest can act on. assert_eq!( vm.ip_config, - Some( - "ip=192.168.1.2/16,gw=192.168.1.1,ip=192.168.2.2/24,gw=10.10.10.10,ip6=auto" - .to_string() - ) + Some("ip=192.168.1.2/16,gw=192.168.1.1,ip6=auto".to_string()) ); Ok(()) } @@ -3010,7 +3186,7 @@ mod tests { }; let p = ProxmoxClient::new("http://localhost:8006".parse()?, "", "", None, q_cfg, None); - let vm = p.make_config(&cfg, None)?; + let vm = p.make_config(&cfg, None, None)?; // Full memory is still the sold amount; balloon is the 90% floor. assert_eq!(vm.memory, Some(memory_mb.to_string())); assert_eq!(vm.balloon, Some((memory_mb * 90 / 100) as i32)); @@ -3047,7 +3223,7 @@ mod tests { }; let p = ProxmoxClient::new("http://localhost:8006".parse()?, "", "", None, q_cfg, None); - let vm = p.make_config(&cfg, None)?; + let vm = p.make_config(&cfg, None, None)?; let ip_config = vm.ip_config.unwrap(); // The IP should use /24 (gateway prefix), not /26 (range prefix), // so the gateway 185.18.221.1 is inside the VM's subnet. @@ -3116,7 +3292,7 @@ mod tests { let p = ProxmoxClient::new("http://localhost:8006".parse()?, "", "", None, q_cfg, None); - let vm = p.make_config(&cfg, None)?; + let vm = p.make_config(&cfg, None, None)?; let net = vm.net.unwrap(); // 800 Mbit/s ÷ 8 = 100 MB/s assert!( @@ -3145,7 +3321,7 @@ mod tests { let p = ProxmoxClient::new("http://localhost:8006".parse()?, "", "", None, q_cfg, None); - let vm = p.make_config(&cfg, None)?; + let vm = p.make_config(&cfg, None, None)?; assert_eq!(vm.cpu_limit, Some(0.5)); Ok(()) } @@ -3364,7 +3540,7 @@ mod tests { let p = ProxmoxClient::new("http://localhost:8006".parse()?, "", "", None, q_cfg, None); - let vm = p.make_config(&cfg, None)?; + let vm = p.make_config(&cfg, None, None)?; assert!( !vm.net.as_deref().unwrap_or("").contains("rate="), "rate= must not appear when network_mbps is None" @@ -3407,16 +3583,205 @@ mod tests { let p = ProxmoxClient::new("http://localhost:8006".parse()?, "", "", None, q_cfg, None); // With vendor snippet - let vm = p.make_config(&cfg, Some("local:snippets/lnvps-vendor.yaml"))?; + let vm = p.make_config(&cfg, Some("local:snippets/lnvps-vendor.yaml"), None)?; assert_eq!( vm.cicustom, Some("vendor=local:snippets/lnvps-vendor.yaml".to_string()) ); // Without vendor snippet - let vm = p.make_config(&cfg, None)?; + let vm = p.make_config(&cfg, None, None)?; assert_eq!(vm.cicustom, None); + // Both snippets + let vm = p.make_config( + &cfg, + Some("local:snippets/lnvps-vendor.yaml"), + Some("local:snippets/lnvps-net-1.yaml"), + )?; + assert_eq!( + vm.cicustom, + Some( + "vendor=local:snippets/lnvps-vendor.yaml,network=local:snippets/lnvps-net-1.yaml" + .to_string() + ) + ); + + Ok(()) + } + + /// A VM with one address per family stays on the built-in `ipconfig` path, so + /// nothing already deployed is reconfigured by the snippet feature. + /// One IPv4 plus one IPv6 on a single range each, which is what every VM + /// holds today. + fn single_address_vm() -> FullVmInfo { + let mut cfg = mock_full_vm(); + cfg.ranges = vec![IpRange { + id: 1, + cidr: "185.18.221.64/26".to_string(), + gateway: "185.18.221.1/24".to_string(), + enabled: true, + region_id: 1, + ..Default::default() + }]; + cfg.ips = vec![lnvps_db::VmIpAssignment { + id: 1, + vm_id: cfg.vm.id, + ip_range_id: 1, + ip: "185.18.221.65".to_string(), + ..Default::default() + }]; + cfg + } + + /// A VM with one address per family stays on the built-in `ipconfig` path, so + /// nothing already deployed is reconfigured by the snippet feature. + #[test] + fn test_no_network_snippet_for_single_address() -> Result<()> { + assert!(ProxmoxClient::make_network_config(&single_address_vm())?.is_none()); + Ok(()) + } + + #[test] + fn test_network_snippet_carries_every_ipv4() -> Result<()> { + let mut cfg = single_address_vm(); + cfg.ips.push(lnvps_db::VmIpAssignment { + id: 2, + ip: "185.18.221.66".to_string(), + ip_range_id: 1, + vm_id: cfg.vm.id, + ..Default::default() + }); + + let net = ProxmoxClient::make_network_config(&cfg)?.expect("two v4 needs a snippet"); + + // Both addresses, widened to the gateway prefix so the gateway is on-link. + assert!(net.contains("- 185.18.221.65/24"), "{net}"); + assert!(net.contains("- 185.18.221.66/24"), "{net}"); + // Matched by MAC, not by guest interface name. + assert!(net.contains("macaddress: ff:ff:ff:ff:ff:fe"), "{net}"); + // One default route, not one per address. + assert_eq!(1, net.matches("to: default").count(), "{net}"); + assert!(net.contains("via: 185.18.221.1"), "{net}"); + Ok(()) + } + + /// `ipconfig[n]` holds one address per family, so a VM carrying more must not + /// have the extras repeated into that property string — they live in the + /// snippet. A repeated key is either rejected by PVE or silently contradicts + /// the snippet, and nothing in CI reaches a real host to find out. + #[test] + fn test_ipconfig_holds_one_address_per_family() -> Result<()> { + let mut cfg = single_address_vm(); + cfg.ips.push(lnvps_db::VmIpAssignment { + id: 2, + ip: "185.18.221.66".to_string(), + ip_range_id: 1, + vm_id: cfg.vm.id, + ..Default::default() + }); + cfg.ranges.push(IpRange { + id: 3, + cidr: "fd00::/64".to_string(), + gateway: "fd00::1".to_string(), + enabled: true, + region_id: 1, + ..Default::default() + }); + for (id, ip) in [(3u64, "fd00::2"), (4, "fd00::3")] { + cfg.ips.push(lnvps_db::VmIpAssignment { + id, + ip: ip.to_string(), + ip_range_id: 3, + vm_id: cfg.vm.id, + ..Default::default() + }); + } + + let q_cfg = QemuConfig { + machine: "q35".to_string(), + os_type: "l26".to_string(), + bridge: "vmbr0".to_string(), + cpu: "host".to_string(), + kvm: true, + arch: "x86_64".to_string(), + balloon_min_pct: None, + firewall_config: None, + }; + let p = ProxmoxClient::new("http://localhost:8006".parse()?, "", "", None, q_cfg, None); + let vm = p.make_config(&cfg, None, Some("local:snippets/lnvps-net-1.yaml"))?; + let ip_config = vm.ip_config.unwrap(); + + assert_eq!(1, ip_config.matches("ip=").count(), "{ip_config}"); + assert_eq!(1, ip_config.matches("ip6=").count(), "{ip_config}"); + // The first assignment of each family, matching what the snippet routes. + assert!(ip_config.contains("ip=185.18.221.65/24"), "{ip_config}"); + assert!(ip_config.contains("ip6=fd00::2/64"), "{ip_config}"); + Ok(()) + } + + /// Addresses from a second range must not add a second default route. + #[test] + fn test_network_snippet_has_one_default_route_per_family() -> Result<()> { + let mut cfg = single_address_vm(); + cfg.ranges.push(IpRange { + id: 2, + cidr: "185.18.221.128/25".to_string(), + gateway: "185.18.221.2/24".to_string(), + enabled: true, + region_id: 1, + ..Default::default() + }); + cfg.ips.push(lnvps_db::VmIpAssignment { + id: 2, + ip: "185.18.221.130".to_string(), + ip_range_id: 2, + vm_id: cfg.vm.id, + ..Default::default() + }); + + let net = ProxmoxClient::make_network_config(&cfg)?.expect("two v4 needs a snippet"); + assert_eq!(1, net.matches("to: default").count(), "{net}"); + // The first assignment's gateway wins, as it does via `ipconfig`. + assert!(net.contains("via: 185.18.221.1"), "{net}"); + assert!(net.contains("- 185.18.221.130/24"), "{net}"); + Ok(()) + } + + /// A SLAAC range contributes autoconfiguration rather than a static address. + #[test] + fn test_network_snippet_uses_accept_ra_for_slaac() -> Result<()> { + let mut cfg = single_address_vm(); + cfg.ips.push(lnvps_db::VmIpAssignment { + id: 2, + ip: "185.18.221.66".to_string(), + ip_range_id: 1, + vm_id: cfg.vm.id, + ..Default::default() + }); + cfg.ranges.push(IpRange { + id: 3, + cidr: "fd00::/64".to_string(), + gateway: "fd00::1".to_string(), + enabled: true, + region_id: 1, + allocation_mode: IpRangeAllocationMode::SlaacEui64, + ..Default::default() + }); + cfg.ips.push(lnvps_db::VmIpAssignment { + id: 3, + ip: "fd00::ffff:ffff:ffff:fffe".to_string(), + ip_range_id: 3, + vm_id: cfg.vm.id, + ..Default::default() + }); + + let net = ProxmoxClient::make_network_config(&cfg)?.expect("two v4 needs a snippet"); + assert!(net.contains("accept-ra: true"), "{net}"); + assert!( + !net.contains("fd00::"), + "SLAAC address must not be pinned: {net}" + ); Ok(()) } diff --git a/lnvps_api_admin/src/admin/custom_pricing.rs b/lnvps_api_admin/src/admin/custom_pricing.rs index c8daaeed..59ebc594 100644 --- a/lnvps_api_admin/src/admin/custom_pricing.rs +++ b/lnvps_api_admin/src/admin/custom_pricing.rs @@ -1,9 +1,9 @@ +use crate::admin::RouterState; use crate::admin::auth::AdminAuth; use crate::admin::model::{ AdminCustomPricingDisk, AdminCustomPricingInfo, CopyCustomPricingRequest, CreateCustomPricingRequest, UpdateCustomPricingRequest, }; -use crate::admin::{RouterState, validate_offered_ip_count}; use axum::extract::{Path, Query, State}; use axum::routing::{get, post}; use axum::{Json, Router}; @@ -495,7 +495,5 @@ fn validate_ip_ranges(pricing: &VmCustomPricing) -> Result<(), ApiError> { if pricing.min_ip6 > pricing.max_ip6 { return Err(ApiError::bad_request("min_ip6 cannot exceed max_ip6")); } - validate_offered_ip_count("max_ip4", pricing.max_ip4)?; - validate_offered_ip_count("max_ip6", pricing.max_ip6)?; Ok(()) } diff --git a/lnvps_api_admin/src/admin/mod.rs b/lnvps_api_admin/src/admin/mod.rs index 569725ba..c75a856b 100644 --- a/lnvps_api_admin/src/admin/mod.rs +++ b/lnvps_api_admin/src/admin/mod.rs @@ -1,9 +1,6 @@ use axum::Router; use axum::extract::FromRef; -use lnvps_api_common::{ - ApiError, ExchangeRateService, MAX_CONFIGURABLE_IPS, RedisWorkFeedback, VmStateCache, - WorkCommander, -}; +use lnvps_api_common::{ExchangeRateService, RedisWorkFeedback, VmStateCache, WorkCommander}; use lnvps_db::LNVpsDb; use std::sync::Arc; @@ -37,18 +34,6 @@ mod vm_templates; mod vms; mod websocket; -/// Refuse an offer wider than what a guest can actually be configured with, so an -/// operator cannot create a plan whose orders would allocate and bill addresses -/// the VM never receives. The order path enforces the same bound. -pub(crate) fn validate_offered_ip_count(field: &str, count: u16) -> Result<(), ApiError> { - if count > MAX_CONFIGURABLE_IPS { - return Err(ApiError::bad_request(format!( - "{field} cannot exceed {MAX_CONFIGURABLE_IPS}: a VM can only be configured with that many addresses per family" - ))); - } - Ok(()) -} - #[derive(Clone, FromRef)] pub(crate) struct RouterState { pub db: Arc, diff --git a/lnvps_api_admin/src/admin/vm_templates.rs b/lnvps_api_admin/src/admin/vm_templates.rs index bf1b4fca..2b673dcb 100644 --- a/lnvps_api_admin/src/admin/vm_templates.rs +++ b/lnvps_api_admin/src/admin/vm_templates.rs @@ -1,8 +1,8 @@ +use crate::admin::RouterState; use crate::admin::auth::AdminAuth; use crate::admin::model::{ AdminCreateVmTemplateRequest, AdminUpdateVmTemplateRequest, AdminVmTemplateInfo, }; -use crate::admin::{RouterState, validate_offered_ip_count}; use axum::extract::{Path, Query, State}; use axum::routing::get; use axum::{Json, Router}; @@ -140,8 +140,6 @@ async fn admin_create_vm_template( let ip4_count = req.ip4_count.unwrap_or(1); let ip6_count = req.ip6_count.unwrap_or(1); - validate_offered_ip_count("ip4_count", ip4_count)?; - validate_offered_ip_count("ip6_count", ip6_count)?; // Handle cost plan creation or validation let cost_plan_id = if let Some(existing_cost_plan_id) = req.cost_plan_id { @@ -329,11 +327,9 @@ async fn admin_update_vm_template( template.region_id = region_id; } if let Some(v) = req.ip4_count { - validate_offered_ip_count("ip4_count", v)?; template.ip4_count = v; } if let Some(v) = req.ip6_count { - validate_offered_ip_count("ip6_count", v)?; template.ip6_count = v; } if let Some(v) = req.disk_iops_read { diff --git a/lnvps_api_common/src/model.rs b/lnvps_api_common/src/model.rs index 204c7016..0f173b32 100644 --- a/lnvps_api_common/src/model.rs +++ b/lnvps_api_common/src/model.rs @@ -803,13 +803,6 @@ fn default_ip_count() -> u16 { 1 } -/// Largest address count a VM can actually be given, per family. -/// -/// A guest gets one cloud-init `ipconfig` entry, which holds a single IPv4 and a -/// single IPv6, so anything above this would be allocated and billed without ever -/// reaching the VM. Raise once the guest-side config can carry more. -pub const MAX_CONFIGURABLE_IPS: u16 = 1; - impl CustomVmSpec { /// Build the template this spec describes. /// diff --git a/lnvps_api_common/src/pricing.rs b/lnvps_api_common/src/pricing.rs index 788ae2bf..d49e7bbd 100644 --- a/lnvps_api_common/src/pricing.rs +++ b/lnvps_api_common/src/pricing.rs @@ -1,6 +1,5 @@ use crate::{ - ConvertedCurrencyAmount, ExchangeRateService, MAX_CONFIGURABLE_IPS, Ticker, TickerRate, - UpgradeConfig, VatClient, + ConvertedCurrencyAmount, ExchangeRateService, Ticker, TickerRate, UpgradeConfig, VatClient, }; use anyhow::{Result, anyhow, bail, ensure}; use chrono::{DateTime, Days, Months, TimeDelta, Utc}; @@ -867,23 +866,6 @@ impl PricingEngine { pricing.max_ip6 ); } - // The plan's own bounds are not enough: a stored plan can exceed what a - // guest can be configured with, and an order against it would allocate - // and bill addresses the VM never receives. - if template.ip4_count > MAX_CONFIGURABLE_IPS { - bail!( - "IPv4 count {} exceeds the {} address(es) a VM can be configured with", - template.ip4_count, - MAX_CONFIGURABLE_IPS - ); - } - if template.ip6_count > MAX_CONFIGURABLE_IPS { - bail!( - "IPv6 count {} exceeds the {} address(es) a VM can be configured with", - template.ip6_count, - MAX_CONFIGURABLE_IPS - ); - } Ok(()) } @@ -2032,39 +2014,6 @@ mod tests { Ok(()) } - /// A stored plan can be wider than what a guest can be configured with — the - /// migration adopts counts from existing VMs — so the order path must bound - /// the count itself and not trust the plan's range. - #[tokio::test] - async fn validate_custom_vm_spec_refuses_unconfigurable_ip_counts() -> Result<()> { - let db = MockDb::default(); - add_custom_pricing(&db).await; - let db: Arc = Arc::new(db); - - let base = db.get_custom_vm_template(1).await?; - let pricing = db.get_custom_pricing(base.pricing_id).await?; - assert!( - pricing.max_ip4 > MAX_CONFIGURABLE_IPS && pricing.max_ip6 > MAX_CONFIGURABLE_IPS, - "plan must be wider than the cap for this to test anything" - ); - - let mut within_plan_v4 = base.clone(); - within_plan_v4.ip4_count = MAX_CONFIGURABLE_IPS + 1; - let err = PricingEngine::validate_custom_vm_spec(&db, &within_plan_v4) - .await - .expect_err("within the plan but above what a guest can hold"); - assert!(err.to_string().contains("can be configured with"), "{err}"); - - let mut within_plan_v6 = base.clone(); - within_plan_v6.ip6_count = MAX_CONFIGURABLE_IPS + 1; - let err = PricingEngine::validate_custom_vm_spec(&db, &within_plan_v6) - .await - .expect_err("within the plan but above what a guest can hold"); - assert!(err.to_string().contains("can be configured with"), "{err}"); - - Ok(()) - } - /// Regression: sub-GB memory/disk must be billed (rounded up), not truncated /// to 0. Previously `memory / GB` floored a request just under 2 GB to 1 GB /// (and anything under 1 GB to free). diff --git a/lnvps_e2e/src/admin_api.rs b/lnvps_e2e/src/admin_api.rs index 9d3e3cca..97aa6f88 100644 --- a/lnvps_e2e/src/admin_api.rs +++ b/lnvps_e2e/src/admin_api.rs @@ -3025,13 +3025,13 @@ mod tests { assert_eq!(resp.status(), StatusCode::BAD_REQUEST); } - /// The guest can only be configured with one address per family, so an offer - /// above that would allocate and bill for addresses the VM never receives. + /// A plan may offer more than one address per family; the guest network + /// config carries them all on one NIC. #[tokio::test] - async fn test_admin_create_custom_pricing_rejects_unconfigurable_ip_count() { + async fn test_admin_create_custom_pricing_accepts_multi_ip_range() { let client = setup().await; let body = serde_json::json!({ - "name": "e2e-too-many-ips", + "name": "e2e-multi-ip", "enabled": true, "region_id": 1, "currency": "EUR", @@ -3051,32 +3051,7 @@ mod tests { .post_auth("/api/admin/v1/custom_pricing", &body) .await .unwrap(); - assert_eq!(resp.status(), StatusCode::BAD_REQUEST); - } - - #[tokio::test] - async fn test_admin_create_vm_template_rejects_unconfigurable_ip_count() { - let client = setup().await; - let body = serde_json::json!({ - "name": "e2e-too-many-ips-template", - "enabled": true, - "region_id": 1, - "cpu": 1, - "memory": 1073741824_u64, - "disk_size": 10737418240_u64, - "disk_type": "ssd", - "disk_interface": "pcie", - "ip4_count": 2, - "cost_plan_amount": 500, - "cost_plan_currency": "EUR", - "cost_plan_interval_amount": 1, - "cost_plan_interval_type": "month" - }); - let resp = client - .post_auth("/api/admin/v1/vm_templates", &body) - .await - .unwrap(); - assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + assert_eq!(resp.status(), StatusCode::OK); } #[tokio::test] diff --git a/lnvps_e2e/src/lifecycle.rs b/lnvps_e2e/src/lifecycle.rs index dec9a36f..d1a7fc11 100644 --- a/lnvps_e2e/src/lifecycle.rs +++ b/lnvps_e2e/src/lifecycle.rs @@ -221,7 +221,7 @@ mod tests { "min_memory": 1073741824_u64, "max_memory": 17179869184_u64, "min_ip4": 1, - "max_ip4": 1, + "max_ip4": 2, "min_ip6": 0, "max_ip6": 1, "disk_pricing": [{ @@ -1743,7 +1743,7 @@ mod tests { "disk": 10737418240_u64, "disk_type": "ssd", "disk_interface": "pcie", - "ip4_count": 1, + "ip4_count": 2, "ip6_count": 0, "image_id": image_id, "ssh_key_id": ssh_key_id @@ -1758,15 +1758,21 @@ mod tests { let id = vm["data"]["id"].as_u64().unwrap(); multi_ip_vm_id = Some(id); assert_eq!( - 1, + 2, vm["data"]["template"]["ip4_count"].as_u64().unwrap(), "ordered IPv4 count must be reflected on the VM's template" ); assert_eq!(0, vm["data"]["template"]["ip6_count"].as_u64().unwrap()); - eprintln!("Ordered custom VM {id} with an explicit IPv4 count"); + // Two addresses at 200 cents each, so the IPv4 share alone exceeds + // the single-address price this plan used to imply. + let amount = vm["data"]["template"]["cost_plan"]["amount"] + .as_u64() + .unwrap(); + assert!(amount >= 400, "2x IPv4 must be billed, got {amount}"); + eprintln!("Ordered custom VM {id} with 2x IPv4 (amount {amount})"); } else { eprintln!( - "Explicit-IP custom VM order returned {} (expected if provisioner unavailable)", + "Multi-IP custom VM order returned {} (expected if provisioner unavailable)", resp.status() ); } @@ -1784,21 +1790,6 @@ mod tests { "an out-of-range IPv4 count must not be orderable" ); - // A plan cannot offer more addresses than the guest can be configured - // with, so widening it is refused. - let resp = admin - .patch_auth( - &format!("/api/admin/v1/custom_pricing/{custom_pricing_id}"), - &serde_json::json!({ "max_ip4": 2 }), - ) - .await - .unwrap(); - assert_eq!( - resp.status(), - StatusCode::BAD_REQUEST, - "max_ip4 above the configurable limit must be refused" - ); - // ---------------------------------------------------------------- // 20. Cleanup: hard-delete VMs and all infrastructure via DB // The worker cannot reach fake hosts, so API-level VM deletion