From: Stefan Hanreich <s.hanreich@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [pve-devel] [PATCH proxmox-ve-rs v2 1/1] partially fix #6176: config: guest: change default for firewall key
Date: Thu, 13 Mar 2025 14:22:27 +0100 [thread overview]
Message-ID: <20250313132231.166477-1-s.hanreich@proxmox.com> (raw)
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> {
+ 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,
+ #[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(
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
next reply other threads:[~2025-03-13 13:23 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-13 13:22 Stefan Hanreich [this message]
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 ` [pve-devel] [PATCH proxmox-ve-rs v2 1/1] partially fix #6176: config: guest: change default for " Hannes Laimer
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=20250313132231.166477-1-s.hanreich@proxmox.com \
--to=s.hanreich@proxmox.com \
--cc=pve-devel@lists.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