From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id D43721FF15C for ; Wed, 19 Feb 2025 11:10:30 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 025DD1CD6F; Wed, 19 Feb 2025 11:10:24 +0100 (CET) From: Stefan Hanreich To: pve-devel@lists.proxmox.com Date: Wed, 19 Feb 2025 11:09:45 +0100 Message-Id: <20250219100949.54413-1-s.hanreich@proxmox.com> X-Mailer: git-send-email 2.39.5 MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.241 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment KAM_LAZY_DOMAIN_SECURITY 1 Sending domain does not have any anti-forgery methods RDNS_NONE 0.793 Delivered to internal network by a host with no rDNS SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_NONE 0.001 SPF: sender does not publish an SPF Record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [address.rs, vm.rs] Subject: [pve-devel] [PATCH proxmox-ve-rs 1/1] partially fix #6176: config: guest: change default for firewall key X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" When the firewall key wasn't present in the network device configuration of a guest, the firewall defaulted to on instead of off. Since the UI omitted the firewall setting in the API calls when it is unchecked, there was no way for the firewall to be turned off for a specific network device of a guest with the firewall enabled. Since the whole section didn't even properly parse the property string via the existing proxmox-schema facilities, also refactor the whole property string parsing logic while we're at it. This is done by splitting the network configuration structs into different ones for LXC and QEMU and then using those to deserialize the PropertyString. There is also a slight breakage for callers utilizing NetworkDevice, since it now returns owned values instead of references, which makes life easier and is okay since the values all implement Copy. At this time pve-api-types isn't packaged yet, but as soon as it is we can use the API schemas generated from pve-api-types instead of manually creating our own subset schema. Signed-off-by: Stefan Hanreich --- .../src/firewall/types/address.rs | 4 +- proxmox-ve-config/src/guest/vm.rs | 344 ++++++++++++------ 2 files changed, 244 insertions(+), 104 deletions(-) diff --git a/proxmox-ve-config/src/firewall/types/address.rs b/proxmox-ve-config/src/firewall/types/address.rs index 9b73d3d..c218d37 100644 --- a/proxmox-ve-config/src/firewall/types/address.rs +++ b/proxmox-ve-config/src/firewall/types/address.rs @@ -119,7 +119,7 @@ impl From for Cidr { const IPV4_LENGTH: u8 = 32; -#[derive(Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash, DeserializeFromStr)] pub struct Ipv4Cidr { addr: Ipv4Addr, mask: u8, @@ -193,7 +193,7 @@ impl fmt::Display for Ipv4Cidr { const IPV6_LENGTH: u8 = 128; -#[derive(Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash, DeserializeFromStr)] pub struct Ipv6Cidr { addr: Ipv6Addr, mask: u8, diff --git a/proxmox-ve-config/src/guest/vm.rs b/proxmox-ve-config/src/guest/vm.rs index 3476b93..8e226bd 100644 --- a/proxmox-ve-config/src/guest/vm.rs +++ b/proxmox-ve-config/src/guest/vm.rs @@ -3,15 +3,18 @@ use std::io; use std::str::FromStr; use std::{collections::HashMap, net::Ipv6Addr}; -use proxmox_schema::property_string::PropertyIterator; +use proxmox_schema::property_string::PropertyString; +use proxmox_sortable_macro::sortable; use anyhow::{bail, Error}; +use proxmox_schema::{ApiType, BooleanSchema, KeyAliasInfo, ObjectSchema, StringSchema}; +use serde::Deserialize; use serde_with::DeserializeFromStr; -use crate::firewall::parse::{match_digits, parse_bool}; +use crate::firewall::parse::match_digits; use crate::firewall::types::address::{Ipv4Cidr, Ipv6Cidr}; -#[derive(Clone, Debug, DeserializeFromStr, PartialEq, Eq, Hash, PartialOrd, Ord)] +#[derive(Clone, Copy, Debug, DeserializeFromStr, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct MacAddress([u8; 6]); static LOCAL_PART: [u8; 8] = [0xFE, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]; @@ -75,7 +78,7 @@ impl Display for MacAddress { } } -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, DeserializeFromStr)] #[cfg_attr(test, derive(Eq, PartialEq))] pub enum NetworkDeviceModel { VirtIO, @@ -100,35 +103,199 @@ impl FromStr for NetworkDeviceModel { } } -#[derive(Debug)] +#[derive(Debug, Deserialize)] #[cfg_attr(test, derive(Eq, PartialEq))] -pub struct NetworkDevice { +pub struct QemuNetworkDevice { model: NetworkDeviceModel, + #[serde(rename = "macaddr")] + mac_address: MacAddress, + firewall: Option, +} + +impl ApiType for QemuNetworkDevice { + #[sortable] + const API_SCHEMA: proxmox_schema::Schema = ObjectSchema::new( + "QEMU Network Device", + &sorted!([ + ( + "firewall", + true, + &BooleanSchema::new("firewall enabled for this network device").schema(), + ), + ( + "macaddr", + false, + &StringSchema::new("mac address for this network device").schema(), + ), + ( + "model", + false, + &StringSchema::new("type of this network device").schema(), + ), + ]), + ) + .additional_properties(true) + .key_alias_info(KeyAliasInfo::new( + "model", + &sorted!(["e1000", "rtl8139", "virtio", "vmxnet3"]), + "macaddr", + )) + .schema(); +} + +#[derive(Debug, DeserializeFromStr, Copy, Clone)] +#[cfg_attr(test, derive(Eq, PartialEq))] +pub enum LxcIpv4Addr { + Ip(Ipv4Cidr), + Dhcp, + Manual, +} + +impl LxcIpv4Addr { + pub fn cidr(&self) -> Option { + match self { + LxcIpv4Addr::Ip(ipv4_cidr) => Some(*ipv4_cidr), + _ => None, + } + } +} + +impl FromStr for LxcIpv4Addr { + type Err = Error; + + fn from_str(s: &str) -> Result { + Ok(match s { + "dhcp" => LxcIpv4Addr::Dhcp, + "manual" => LxcIpv4Addr::Manual, + _ => LxcIpv4Addr::Ip(s.parse()?), + }) + } +} + +#[derive(Debug, DeserializeFromStr, Copy, Clone)] +#[cfg_attr(test, derive(Eq, PartialEq))] +pub enum LxcIpv6Addr { + Ip(Ipv6Cidr), + Dhcp, + Auto, + Manual, +} + +impl LxcIpv6Addr { + pub fn cidr(&self) -> Option { + match self { + LxcIpv6Addr::Ip(ipv6_cidr) => Some(*ipv6_cidr), + _ => None, + } + } +} + +impl FromStr for LxcIpv6Addr { + type Err = Error; + + fn from_str(s: &str) -> Result { + Ok(match s { + "dhcp" => LxcIpv6Addr::Dhcp, + "manual" => LxcIpv6Addr::Manual, + "auto" => LxcIpv6Addr::Auto, + _ => LxcIpv6Addr::Ip(s.parse()?), + }) + } +} + +#[derive(Debug, Deserialize)] +#[cfg_attr(test, derive(Eq, PartialEq))] +pub struct LxcNetworkDevice { + #[serde(rename = "type")] + ty: NetworkDeviceModel, + #[serde(rename = "hwaddr")] mac_address: MacAddress, - firewall: bool, - ip: Option, - ip6: Option, + firewall: Option, + ip: Option, + ip6: Option, +} + +impl ApiType for LxcNetworkDevice { + #[sortable] + const API_SCHEMA: proxmox_schema::Schema = ObjectSchema::new( + "LXC Network Device", + &sorted!([ + ( + "firewall", + true, + &BooleanSchema::new("firewall enabled for this network device").schema(), + ), + ( + "hwaddr", + false, + &StringSchema::new("mac address for this network device").schema(), + ), + ( + "ip", + true, + &StringSchema::new("IP settings for this network device").schema(), + ), + ( + "ip6", + true, + &StringSchema::new("IPv6 settings for this network device").schema(), + ), + ( + "type", + false, + &StringSchema::new("type of the network device").schema(), + ), + ]), + ) + .additional_properties(true) + .schema(); +} + +#[derive(Debug)] +#[cfg_attr(test, derive(Eq, PartialEq))] +pub enum NetworkDevice { + Qemu(QemuNetworkDevice), + Lxc(LxcNetworkDevice), } impl NetworkDevice { pub fn model(&self) -> NetworkDeviceModel { - self.model + match self { + NetworkDevice::Qemu(qemu_network_device) => qemu_network_device.model, + NetworkDevice::Lxc(lxc_network_device) => lxc_network_device.ty, + } } - pub fn mac_address(&self) -> &MacAddress { - &self.mac_address + pub fn mac_address(&self) -> MacAddress { + match self { + NetworkDevice::Qemu(qemu_network_device) => qemu_network_device.mac_address, + NetworkDevice::Lxc(lxc_network_device) => lxc_network_device.mac_address, + } } - pub fn ip(&self) -> Option<&Ipv4Cidr> { - self.ip.as_ref() + pub fn ip(&self) -> Option { + if let NetworkDevice::Lxc(device) = self { + return device.ip?.cidr(); + } + + None } - pub fn ip6(&self) -> Option<&Ipv6Cidr> { - self.ip6.as_ref() + pub fn ip6(&self) -> Option { + if let NetworkDevice::Lxc(device) = self { + return device.ip6?.cidr(); + } + + None } pub fn has_firewall(&self) -> bool { - self.firewall + let firewall_option = match self { + NetworkDevice::Qemu(qemu_network_device) => qemu_network_device.firewall, + NetworkDevice::Lxc(lxc_network_device) => lxc_network_device.firewall, + }; + + firewall_option.unwrap_or(false) } } @@ -136,57 +303,15 @@ impl FromStr for NetworkDevice { type Err = Error; fn from_str(s: &str) -> Result { - let (mut ty, mut hwaddr, mut firewall, mut ip, mut ip6) = (None, None, true, None, None); - - for entry in PropertyIterator::new(s) { - let (key, value) = entry.unwrap(); - - if let Some(key) = key { - match key { - "type" | "model" => { - ty = Some(NetworkDeviceModel::from_str(&value)?); - } - "hwaddr" | "macaddr" => { - hwaddr = Some(MacAddress::from_str(&value)?); - } - "firewall" => { - firewall = parse_bool(&value)?; - } - "ip" => { - if value == "dhcp" { - continue; - } - - ip = Some(Ipv4Cidr::from_str(&value)?); - } - "ip6" => { - if value == "dhcp" || value == "auto" { - continue; - } - - ip6 = Some(Ipv6Cidr::from_str(&value)?); - } - _ => { - if let Ok(model) = NetworkDeviceModel::from_str(key) { - ty = Some(model); - hwaddr = Some(MacAddress::from_str(&value)?); - } - } - } - } + if let Ok(qemu_device) = s.parse::>() { + return Ok(NetworkDevice::Qemu(qemu_device.into_inner())); } - if let (Some(ty), Some(hwaddr)) = (ty, hwaddr) { - return Ok(NetworkDevice { - model: ty, - mac_address: hwaddr, - firewall, - ip, - ip6, - }); + if let Ok(lxc_device) = s.parse::>() { + return Ok(NetworkDevice::Lxc(lxc_device.into_inner())); } - bail!("No valid network device detected in string {s}"); + bail!("not a valid network device property string: {s}") } } @@ -312,30 +437,43 @@ mod tests { assert_eq!( network_device, - NetworkDevice { + NetworkDevice::Qemu(QemuNetworkDevice { model: NetworkDeviceModel::VirtIO, mac_address: MacAddress([0xAA, 0xAA, 0xAA, 0x17, 0x19, 0x81]), - firewall: true, - ip: None, - ip6: None, - } + firewall: Some(true), + }) + ); + + network_device = "model=virtio,macaddr=AA:AA:AA:17:19:81,bridge=public" + .parse() + .expect("valid network configuration"); + + assert_eq!( + network_device, + NetworkDevice::Qemu(QemuNetworkDevice { + model: NetworkDeviceModel::VirtIO, + mac_address: MacAddress([0xAA, 0xAA, 0xAA, 0x17, 0x19, 0x81]), + firewall: None, + }) ); + assert!(!network_device.has_firewall()); + network_device = "model=virtio,macaddr=AA:AA:AA:17:19:81,bridge=public,firewall=1,queues=4" .parse() .expect("valid network configuration"); assert_eq!( network_device, - NetworkDevice { + NetworkDevice::Qemu(QemuNetworkDevice { model: NetworkDeviceModel::VirtIO, mac_address: MacAddress([0xAA, 0xAA, 0xAA, 0x17, 0x19, 0x81]), - firewall: true, - ip: None, - ip6: None, - } + firewall: Some(true), + }) ); + assert!(network_device.has_firewall()); + network_device = "name=eth0,bridge=public,firewall=0,hwaddr=AA:AA:AA:E2:3E:24,ip=dhcp,type=veth" .parse() @@ -343,13 +481,13 @@ mod tests { assert_eq!( network_device, - NetworkDevice { - model: NetworkDeviceModel::Veth, + NetworkDevice::Lxc(LxcNetworkDevice { + ty: NetworkDeviceModel::Veth, mac_address: MacAddress([0xAA, 0xAA, 0xAA, 0xE2, 0x3E, 0x24]), - firewall: false, - ip: None, + firewall: Some(false), + ip: Some(LxcIpv4Addr::Dhcp), ip6: None, - } + }) ); "model=virtio" @@ -369,7 +507,7 @@ mod tests { } #[test] - fn test_parse_network_confg() { + fn test_parse_network_config() { let mut guest_config = "\ boot: order=scsi0;net0 cores: 4 @@ -433,13 +571,11 @@ vmgenid: 706fbe99-d28b-4047-a9cd-3677c859ca8a" assert_eq!( network_config.network_devices()[&0], - NetworkDevice { + NetworkDevice::Qemu(QemuNetworkDevice { model: NetworkDeviceModel::VirtIO, mac_address: MacAddress([0xAA, 0xBB, 0xCC, 0xF2, 0xFE, 0x75]), - firewall: true, - ip: None, - ip6: None, - } + firewall: None, + }) ); guest_config = "\ @@ -448,7 +584,7 @@ cores: 1 features: nesting=1 hostname: dnsct memory: 512 -net0: name=eth0,bridge=data,firewall=1,hwaddr=BC:24:11:47:83:11,ip=dhcp,type=veth +net0: name=eth0,bridge=data,firewall=1,hwaddr=BC:24:11:47:83:11,ip=dhcp,ip6=auto,type=veth net2: name=eth0,bridge=data,firewall=0,hwaddr=BC:24:11:47:83:12,ip=123.123.123.123/24,type=veth net5: name=eth0,bridge=data,firewall=1,hwaddr=BC:24:11:47:83:13,ip6=fd80::1/64,type=veth ostype: alpine @@ -463,35 +599,39 @@ unprivileged: 1" assert_eq!( network_config.network_devices()[&0], - NetworkDevice { - model: NetworkDeviceModel::Veth, + NetworkDevice::Lxc(LxcNetworkDevice { + ty: NetworkDeviceModel::Veth, mac_address: MacAddress([0xBC, 0x24, 0x11, 0x47, 0x83, 0x11]), - firewall: true, - ip: None, - ip6: None, - } + firewall: Some(true), + ip: Some(LxcIpv4Addr::Dhcp), + ip6: Some(LxcIpv6Addr::Auto), + }) ); assert_eq!( network_config.network_devices()[&2], - NetworkDevice { - model: NetworkDeviceModel::Veth, + NetworkDevice::Lxc(LxcNetworkDevice { + ty: NetworkDeviceModel::Veth, mac_address: MacAddress([0xBC, 0x24, 0x11, 0x47, 0x83, 0x12]), - firewall: false, - ip: Some(Ipv4Cidr::from_str("123.123.123.123/24").expect("valid ipv4")), + firewall: Some(false), + ip: Some(LxcIpv4Addr::Ip( + Ipv4Cidr::from_str("123.123.123.123/24").expect("valid ipv4") + )), ip6: None, - } + }) ); assert_eq!( network_config.network_devices()[&5], - NetworkDevice { - model: NetworkDeviceModel::Veth, + NetworkDevice::Lxc(LxcNetworkDevice { + ty: NetworkDeviceModel::Veth, mac_address: MacAddress([0xBC, 0x24, 0x11, 0x47, 0x83, 0x13]), - firewall: true, + firewall: Some(true), ip: None, - ip6: Some(Ipv6Cidr::from_str("fd80::1/64").expect("valid ipv6")), - } + ip6: Some(LxcIpv6Addr::Ip( + Ipv6Cidr::from_str("fd80::1/64").expect("valid ipv6") + )), + }) ); NetworkConfig::parse( -- 2.39.5 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel