From: Hannes Laimer <h.laimer@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Stefan Hanreich <s.hanreich@proxmox.com>
Subject: Re: [pve-devel] [PATCH proxmox-ve-rs v2 1/1] partially fix #6176: config: guest: change default for firewall key
Date: Thu, 13 Mar 2025 17:22:12 +0100 [thread overview]
Message-ID: <642dea4f-cfdc-4e44-81a8-08a0b0fbb381@proxmox.com> (raw)
In-Reply-To: <20250313132231.166477-1-s.hanreich@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
prev parent reply other threads:[~2025-03-13 16:22 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-13 13:22 Stefan Hanreich
2025-03-13 13:22 ` [pve-devel] [PATCH proxmox-firewall v2 1/4] ipsets: remove dereference Stefan Hanreich
2025-03-13 13:22 ` [pve-devel] [PATCH proxmox-firewall v2 2/4] partially fix #6176: ipfilter: honor firewall setting from guest cfg Stefan Hanreich
2025-03-13 13:22 ` [pve-devel] [PATCH proxmox-firewall v2 3/4] partially fix #6176: do not generate mac filter if firewall disabled Stefan Hanreich
2025-03-13 13:22 ` [pve-devel] [PATCH proxmox-firewall v2 4/4] tests: add network device without firewall key Stefan Hanreich
2025-03-13 16:22 ` Hannes Laimer [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=642dea4f-cfdc-4e44-81a8-08a0b0fbb381@proxmox.com \
--to=h.laimer@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=s.hanreich@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal