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 [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id 043661FF16F
for <inbox@lore.proxmox.com>; Thu, 13 Mar 2025 14:23:50 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 36E6219D11;
Thu, 13 Mar 2025 14:23:32 +0100 (CET)
From: Stefan Hanreich <s.hanreich@proxmox.com>
To: pve-devel@lists.proxmox.com
Date: Thu, 13 Mar 2025 14:22:27 +0100
Message-Id: <20250313132231.166477-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.220 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
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.
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
Subject: [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-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
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