From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <pve-devel-bounces@lists.proxmox.com> Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 3F86B1FF16F for <inbox@lore.proxmox.com>; Thu, 13 Mar 2025 17:22:28 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 691851E31A; Thu, 13 Mar 2025 17:22:18 +0100 (CET) Message-ID: <642dea4f-cfdc-4e44-81a8-08a0b0fbb381@proxmox.com> Date: Thu, 13 Mar 2025 17:22:12 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>, Stefan Hanreich <s.hanreich@proxmox.com> References: <20250313132231.166477-1-s.hanreich@proxmox.com> Content-Language: en-US From: Hannes Laimer <h.laimer@proxmox.com> In-Reply-To: <20250313132231.166477-1-s.hanreich@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.025 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH proxmox-ve-rs v2 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 <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com> Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com> a comment and small nit inline, other than that LGMT! Consider this and the following three patches: Tested-by: Hannes Laimer <h.laimer@proxmox.com> Reviewed-by: Hannes Laimer <h.laimer@proxmox.com> On 13.03.25 14:22, Stefan Hanreich wrote: > 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 <s.hanreich@proxmox.com> > --- > > Notes: > Changes from v1: > * extracted the default value into a documented const > * added documentation to the newly created types > > .../src/firewall/types/address.rs | 4 +- > proxmox-ve-config/src/guest/vm.rs | 363 +++++++++++++----- > 2 files changed, 263 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<IpAddr> 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..d656a61 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,8 @@ impl Display for MacAddress { > } > } > > -#[derive(Debug, Clone, Copy)] > +/// All possible models of network devices for both QEMU and LXC guests. > +#[derive(Debug, Clone, Copy, DeserializeFromStr)] > #[cfg_attr(test, derive(Eq, PartialEq))] > pub enum NetworkDeviceModel { > VirtIO, > @@ -100,35 +104,217 @@ impl FromStr for NetworkDeviceModel { > } > } > > -#[derive(Debug)] > +/// Representation of the network device property string of a QEMU guest. > +/// > +/// It currently only cotains properties that are required for the firewall to function, there are > +/// still missing properties that can be contained in the schema. > +#[derive(Debug, Deserialize)] > #[cfg_attr(test, derive(Eq, PartialEq))] > -pub struct NetworkDevice { > +pub struct QemuNetworkDevice { > model: NetworkDeviceModel, > + #[serde(rename = "macaddr")] > mac_address: MacAddress, > - firewall: bool, > - ip: Option<Ipv4Cidr>, > - ip6: Option<Ipv6Cidr>, > + firewall: Option<bool>, > +} > + > +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(); > +} > + > +/// Representation of possible values for an LXC guest IPv4 field. > +#[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<Ipv4Cidr> { > + match self { > + LxcIpv4Addr::Ip(ipv4_cidr) => Some(*ipv4_cidr), > + _ => None, > + } > + } > +} > + > +impl FromStr for LxcIpv4Addr { > + type Err = Error; > + > + fn from_str(s: &str) -> Result<Self, Self::Err> { parse() seems to fail with `invalid IPv4 address syntax` and in the context of the firewall where this is used, it results in `proxmox_firewall: error updating firewall rules: invalid IPv4 address syntax` which is not really helpful in locating the cause. Maybe something like ``` if let Ok(ip) = s.parse::<Ipv4Cidr>() { return Ok(LxcIpv4Addr::Ip(ip)); } Ok(match s { "dhcp" => LxcIpv4Addr::Dhcp, "manual" => LxcIpv4Addr::Manual, _ => bail!("Invalid LXC IPv4 setting: {}", s), }) could make sense here? Similarly for IPv6 ``` > + Ok(match s { > + "dhcp" => LxcIpv4Addr::Dhcp, > + "manual" => LxcIpv4Addr::Manual, > + _ => LxcIpv4Addr::Ip(s.parse()?), > + }) > + } > +} > + > +/// Representation of possible values for an LXC guest IPv6 field. > +#[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<Ipv6Cidr> { > + match self { > + LxcIpv6Addr::Ip(ipv6_cidr) => Some(*ipv6_cidr), > + _ => None, > + } > + } > } > > +impl FromStr for LxcIpv6Addr { > + type Err = Error; > + > + fn from_str(s: &str) -> Result<Self, Self::Err> { > + Ok(match s { > + "dhcp" => LxcIpv6Addr::Dhcp, > + "manual" => LxcIpv6Addr::Manual, > + "auto" => LxcIpv6Addr::Auto, > + _ => LxcIpv6Addr::Ip(s.parse()?), > + }) > + } > +} > + > +/// Representation of the network device property string of a LXC guest. > +/// > +/// It currently only cotains properties that are required for the firewall to function, there are > +/// still missing properties that can be contained in the schema. > +#[derive(Debug, Deserialize)] > +#[cfg_attr(test, derive(Eq, PartialEq))] > +pub struct LxcNetworkDevice { > + #[serde(rename = "type")] > + ty: NetworkDeviceModel, nit: we call this `model` for VMs, we could use the same name here > + #[serde(rename = "hwaddr")] > + mac_address: MacAddress, > + firewall: Option<bool>, > + ip: Option<LxcIpv4Addr>, > + ip6: Option<LxcIpv6Addr>, > +} > + > +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(); > +} > + > +/// Container type that can hold both LXC and QEMU network devices. > +#[derive(Debug)] > +#[cfg_attr(test, derive(Eq, PartialEq))] > +pub enum NetworkDevice { > + Qemu(QemuNetworkDevice), > + Lxc(LxcNetworkDevice), > +} > + > +/// default return value for [`NetworkDevice::has_firewall()`] > +pub const NETWORK_DEVICE_FIREWALL_DEFAULT: bool = false; > + > 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 > + /// Returns the MAC address of the network device > + 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() > + /// Returns the IPv4 of the network device, if set in the configuration (LXC only). > + pub fn ip(&self) -> Option<Ipv4Cidr> { > + if let NetworkDevice::Lxc(device) = self { > + return device.ip?.cidr(); > + } > + > + None > } > > - pub fn ip6(&self) -> Option<&Ipv6Cidr> { > - self.ip6.as_ref() > + /// Returns the IPv6 of the network device, if set in the configuration (LXC only). > + pub fn ip6(&self) -> Option<Ipv6Cidr> { > + if let NetworkDevice::Lxc(device) = self { > + return device.ip6?.cidr(); > + } > + > + None > } > > + /// Whether the firewall is enabled for this network device, defaults to [`NETWORK_DEVICE_FIREWALL_DEFAULT`] > 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(NETWORK_DEVICE_FIREWALL_DEFAULT) > } > } > > @@ -136,57 +322,15 @@ impl FromStr for NetworkDevice { > type Err = Error; > > fn from_str(s: &str) -> Result<Self, Self::Err> { > - 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::<PropertyString<QemuNetworkDevice>>() { > + 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::<PropertyString<LxcNetworkDevice>>() { > + 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 +456,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 +500,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 +526,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 +590,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 +603,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 +618,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( _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel