[pve-devel] [PATCH proxmox-ve-rs 1/1] partially fix #6176: config: guest: change default for firewall key

* [pve-devel] [PATCH proxmox-ve-rs 1/1] partially fix #6176: config: guest: change default for firewall key
@ 2025-02-19 10:09 Stefan Hanreich
  2025-02-19 10:09 ` [pve-devel] [PATCH proxmox-firewall 1/4] ipsets: remove dereference Stefan Hanreich
                   ` (5 more replies)
  0 siblings, 6 replies; 9+ messages in thread
From: Stefan Hanreich @ 2025-02-19 10:09 UTC (permalink / raw)
  To: 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 <s.hanreich@proxmox.com>
---
 .../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<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..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<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();
+}
+
+#[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> {
+        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<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()?),
+        })
+    }
+}
+
+#[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<Ipv4Cidr>,
-    ip6: Option<Ipv6Cidr>,
+    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();
+}
+
+#[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<Ipv4Cidr> {
+        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<Ipv6Cidr> {
+        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<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 +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


^ permalink raw reply	[flat|nested] 9+ messages in thread