From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 462571FF16F for ; Tue, 8 Jul 2025 13:00:58 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4CDF11473F; Tue, 8 Jul 2025 13:01:41 +0200 (CEST) Date: Tue, 8 Jul 2025 13:01:29 +0200 From: Wolfgang Bumiller To: Gabriel Goller Message-ID: <2ngeb6fvpt5nbffuh4gmwpml47s3pdjluh37uhxtp4h3vsgb5p@uqtctzp2idyz> References: <20250702145101.894299-1-g.goller@proxmox.com> <20250702145101.894299-25-g.goller@proxmox.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20250702145101.894299-25-g.goller@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.079 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 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 v4 18/22] common: sdn: fabrics: implement validation X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Cc: 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" On Wed, Jul 02, 2025 at 04:50:09PM +0200, Gabriel Goller wrote: > From: Stefan Hanreich > > The SDN fabrics configuration needs to validate properties of structs > that are dependent on their context. For instance the IP of a node > needs to be contained in the referenced fabric. Simple schema > validation is not sufficient for proper validation of the complete > fabrics configuration. > > In order to better model the validation state via the Rust type > system, we provide a type Valid and a accompanying trait Validatable. > The Valid type can only be constructed from types, that implement the > Validatable trait. Anything wrapped in Valid has to unwrapped before > it can be mutated again, ensuring that only valid values can be > contained in a Valid. This makes it possible for methods to require > callers to validate everything beforehand. This is later utilized by > the FabricConfig to ensure that it is only possible to write validated > configurations to the config file. > > We implement Validatable for almost any type representing the fabrics > configuration and call them from the top-level fabric configuration. > > Co-authored-by: Gabriel Goller > Signed-off-by: Stefan Hanreich > --- > proxmox-ve-config/src/common/mod.rs | 2 + > proxmox-ve-config/src/common/valid.rs | 53 +++++ > proxmox-ve-config/src/sdn/fabric/mod.rs | 197 +++++++++++++++++- > .../src/sdn/fabric/section_config/fabric.rs | 24 ++- > .../src/sdn/fabric/section_config/node.rs | 13 ++ > .../section_config/protocol/openfabric.rs | 38 +++- > .../fabric/section_config/protocol/ospf.rs | 47 ++++- > 7 files changed, 367 insertions(+), 7 deletions(-) > create mode 100644 proxmox-ve-config/src/common/valid.rs > > diff --git a/proxmox-ve-config/src/common/mod.rs b/proxmox-ve-config/src/common/mod.rs > index ef09791dd754..9fde536bd02b 100644 > --- a/proxmox-ve-config/src/common/mod.rs > +++ b/proxmox-ve-config/src/common/mod.rs > @@ -2,6 +2,8 @@ use core::hash::Hash; > use std::cmp::Eq; > use std::collections::HashSet; > > +pub mod valid; > + > #[derive(Clone, Debug, Default)] > pub struct Allowlist(HashSet); > > diff --git a/proxmox-ve-config/src/common/valid.rs b/proxmox-ve-config/src/common/valid.rs > new file mode 100644 > index 000000000000..1f92ef9bb409 > --- /dev/null > +++ b/proxmox-ve-config/src/common/valid.rs > @@ -0,0 +1,53 @@ > +use std::ops::Deref; > + > +/// A wrapper type for validatable structs. > +/// > +/// It can only be constructed by implementing the [`Validatable`] type for a struct. Its contents > +/// can be read, but not modified, guaranteeing the content of this struct to always be valid, as > +/// defined by the [`Validatable::validate`] function. > +/// > +/// If you want to edit the content, this struct has to be unwrapped via [`Valid::into_inner`]. > +#[repr(transparent)] > +#[derive(Clone, Default, Debug)] > +pub struct Valid(T); > + > +impl Valid { > + /// returns the wrapped value owned, consumes the Valid struct > + pub fn into_inner(self) -> T { > + self.0 > + } > +} > + > +impl Deref for Valid { > + type Target = T; > + > + fn deref(&self) -> &T { > + &self.0 > + } > +} > + > +impl AsRef for Valid { > + fn as_ref(&self) -> &T { > + &self.0 > + } > +} > + > +/// Defines a struct that can be validated > +/// > +/// This can be useful if a struct can not be validated solely by its structure, for instance if > +/// the validity of a value of a field depends on another field. This trait can help with > +/// abstracting that requirement away and implementing it provides the only way of constructing a > +/// [`Valid`]. > +pub trait Validatable: Sized { > + type Error; > + > + /// Checks whether the values in the struct are valid or not. > + fn validate(&self) -> Result<(), Self::Error>; > + > + /// Calls [`Validatable::validate`] to validate the struct and returns a [`Valid`] if > + /// validation succeeds. > + fn into_valid(self) -> Result, Self::Error> { > + self.validate()?; > + Ok(Valid(self)) > + } > +} > diff --git a/proxmox-ve-config/src/sdn/fabric/mod.rs b/proxmox-ve-config/src/sdn/fabric/mod.rs > index 3342a7053d3f..a2132e8aff3f 100644 > --- a/proxmox-ve-config/src/sdn/fabric/mod.rs > +++ b/proxmox-ve-config/src/sdn/fabric/mod.rs > @@ -1,11 +1,13 @@ > pub mod section_config; > > -use std::collections::BTreeMap; > +use std::collections::{BTreeMap, HashSet}; > use std::marker::PhantomData; > use std::ops::Deref; > > use serde::{Deserialize, Serialize}; > > +use crate::common::valid::Validatable; > + > use crate::sdn::fabric::section_config::{ > fabric::{ > Fabric, FabricDeletableProperties, FabricId, FabricSection, FabricSectionUpdater, > @@ -34,15 +36,38 @@ pub enum FabricConfigError { > FabricDoesNotExist(String), > #[error("node '{0}' does not exist in fabric '{1}'")] > NodeDoesNotExist(String, String), > + #[error("node IP {0} is outside the IP prefix {1} of the fabric")] > + NodeIpOutsideFabricRange(String, String), > #[error("node has a different protocol than the referenced fabric")] > ProtocolMismatch, > #[error("fabric '{0}' already exists in config")] > DuplicateFabric(String), > #[error("node '{0}' already exists in config for fabric {1}")] > DuplicateNode(String, String), > + #[error("fabric {0} contains nodes with duplicated IPs")] > + DuplicateNodeIp(String), > + #[error("fabric '{0}' does not have an IP prefix configured for the node IP {1}")] > + FabricNoIpPrefixForNode(String, String), > + #[error("node '{0}' does not have an IP configured for this fabric prefix {1}")] > + NodeNoIpForFabricPrefix(String, String), > + #[error("fabric '{0}' does not have an IP prefix configured")] > + FabricNoIpPrefix(String), > + #[error("node '{0}' does not have an IP configured")] > + NodeNoIp(String), > + #[error("interface is already in use by another fabric")] > + DuplicateInterface, > + #[error("IPv6 is currently not supported for protocol {0}")] > + NoIpv6(String), ^ "NoIpv6" is a weird variant name for this. Maybe use Ipv6Unsupported. I also wonder if we really need all those messages as separate variants. As a followup to the previous thoughts on error types: Generally I think only errors which are worth distinguishing need their own variants. We definitely don't need one for every single message. The ones here could probably all just be a single `Invalid(String)`. (Or if we don't want to expose `String` here, `Validation(ValidationError)`, with the inner one being a newtype wrapper around the `String`. Internally there can be a helper `fn FabricConfigError::validation(impl Into)`. > // should usually not occur, but we still check for it nonetheless > #[error("mismatched fabric_id")] > FabricIdMismatch, > + // this is technically possible, but we don't allow it > + #[error("duplicate OSPF area")] > + DuplicateOspfArea, > + #[error("IP prefix {0} in fabric '{1}' overlaps with IPv4 prefix {2} in fabric '{3}'")] > + OverlappingIp4Prefix(String, String, String, String), > + #[error("IPv6 prefix {0} in fabric '{1}' overlaps with IPv6 prefix {2} in fabric '{3}'")] > + OverlappingIp6Prefix(String, String, String, String), > } > > /// An entry in a [`FabricConfig`]. > @@ -367,6 +392,93 @@ impl From for FabricEntry { > } > } > > +impl Validatable for FabricEntry { > + type Error = FabricConfigError; > + > + /// Validates the [FabricEntry] configuration. > + /// > + /// Ensures that: > + /// - Node IP addresses are within their respective fabric IP prefix ranges > + /// - IP addresses are unique across all nodes in the fabric > + /// - Each node passes its own validation checks > + fn validate(&self) -> Result<(), FabricConfigError> { > + let fabric = self.fabric(); > + > + let mut ips = HashSet::new(); > + let mut ip6s = HashSet::new(); > + > + for (_id, node) in self.nodes() { > + // Check IPv4 prefix and ip > + match (fabric.ip_prefix(), node.ip()) { > + (None, Some(ip)) => { > + // Fabric needs to have a prefix if a node has an IP configured > + return Err(FabricConfigError::FabricNoIpPrefixForNode( > + fabric.id().to_string(), > + ip.to_string(), > + )); > + } > + (Some(prefix), None) => { > + return Err(FabricConfigError::NodeNoIpForFabricPrefix( > + node.id().to_string(), > + prefix.to_string(), > + )); > + } > + (Some(prefix), Some(ip)) => { > + // Fabric prefix needs to contain the node IP > + if !prefix.contains_address(&ip) { > + return Err(FabricConfigError::NodeIpOutsideFabricRange( > + ip.to_string(), > + prefix.to_string(), > + )); > + } > + } > + _ => {} > + } > + > + // Check IPv6 prefix and ip > + match (fabric.ip6_prefix(), node.ip6()) { > + (None, Some(ip)) => { > + // Fabric needs to have a prefix if a node has an IP configured > + return Err(FabricConfigError::FabricNoIpPrefixForNode( > + fabric.id().to_string(), > + ip.to_string(), > + )); > + } > + (Some(prefix), None) => { > + return Err(FabricConfigError::NodeNoIpForFabricPrefix( > + node.id().to_string(), > + prefix.to_string(), > + )) > + } > + (Some(prefix), Some(ip)) => { > + // Fabric prefix needs to contain the node IP > + if !prefix.contains_address(&ip) { > + return Err(FabricConfigError::NodeIpOutsideFabricRange( > + ip.to_string(), > + prefix.to_string(), > + )); > + } > + } > + _ => {} > + } > + > + // Node IPs need to be unique inside a fabric > + if !node.ip().map(|ip| ips.insert(ip)).unwrap_or(true) { > + return Err(FabricConfigError::DuplicateNodeIp(fabric.id().to_string())); > + } > + > + // Node IPs need to be unique inside a fabric > + if !node.ip6().map(|ip| ip6s.insert(ip)).unwrap_or(true) { > + return Err(FabricConfigError::DuplicateNodeIp(fabric.id().to_string())); > + } > + > + node.validate()?; > + } > + > + fabric.validate() > + } > +} > + > /// A complete SDN fabric configuration. > /// > /// This struct contains the whole fabric configuration in a tree-like structure (fabrics -> nodes > @@ -384,6 +496,89 @@ impl Deref for FabricConfig { > } > } > > +impl Validatable for FabricConfig { > + type Error = FabricConfigError; > + > + /// Validate the [FabricConfig]. > + /// > + /// Ensures that: > + /// - (node, interface) combinations exist only once across all fabrics > + /// - every entry (fabric) validates > + /// - all the ospf fabrics have different areas > + /// - IP prefixes of fabrics do not overlap > + fn validate(&self) -> Result<(), FabricConfigError> { > + let mut node_interfaces = HashSet::new(); > + let mut ospf_area = HashSet::new(); > + > + // Check for overlapping IP prefixes across fabrics > + let fabrics: Vec<_> = self.fabrics.values().map(|f| f.fabric()).collect(); > + let cartesian_product = fabrics > + .iter() > + .enumerate() > + .flat_map(|(i, f1)| fabrics.iter().skip(i + 1).map(move |f2| (f1, f2))); > + > + for (fabric1, fabric2) in cartesian_product { > + if let (Some(prefix1), Some(prefix2)) = (fabric1.ip_prefix(), fabric2.ip_prefix()) { > + if prefix1.overlaps(&prefix2) { > + return Err(FabricConfigError::OverlappingIp4Prefix( > + prefix2.to_string(), > + fabric2.id().to_string(), > + prefix1.to_string(), > + fabric1.id().to_string(), > + )); > + } > + } > + if let (Some(prefix1), Some(prefix2)) = (fabric1.ip6_prefix(), fabric2.ip6_prefix()) { > + if prefix1.overlaps(&prefix2) { > + return Err(FabricConfigError::OverlappingIp6Prefix( > + prefix2.to_string(), > + fabric2.id().to_string(), > + prefix1.to_string(), > + fabric1.id().to_string(), > + )); > + } > + } > + } > + > + // validate that each (node, interface) combination exists only once across all fabrics > + for entry in self.fabrics.values() { > + if let FabricEntry::Ospf(entry) = entry { > + if !ospf_area.insert( > + entry > + .fabric_section() > + .properties() > + .area() > + .get_ipv4_representation(), > + ) { > + return Err(FabricConfigError::DuplicateOspfArea); > + } > + } > + for (node_id, node) in entry.nodes() { > + match node { > + Node::Ospf(node_section) => { > + if !node_section.properties().interfaces().all(|interface| { > + node_interfaces.insert((node_id, interface.name.as_str())) > + }) { > + return Err(FabricConfigError::DuplicateInterface); > + } > + } > + Node::Openfabric(node_section) => { > + if !node_section.properties().interfaces().all(|interface| { > + node_interfaces.insert((node_id, interface.name.as_str())) > + }) { > + return Err(FabricConfigError::DuplicateInterface); > + } > + } > + } > + } > + > + entry.validate()?; > + } > + > + Ok(()) > + } > +} > + > impl FabricConfig { > /// Add a fabric to the [FabricConfig]. > /// > diff --git a/proxmox-ve-config/src/sdn/fabric/section_config/fabric.rs b/proxmox-ve-config/src/sdn/fabric/section_config/fabric.rs > index 75a309398ca2..b8d7649faed3 100644 > --- a/proxmox-ve-config/src/sdn/fabric/section_config/fabric.rs > +++ b/proxmox-ve-config/src/sdn/fabric/section_config/fabric.rs > @@ -7,11 +7,15 @@ use proxmox_schema::{ > Updater, UpdaterType, > }; > > -use crate::sdn::fabric::section_config::protocol::{ > - openfabric::{ > - OpenfabricDeletableProperties, OpenfabricProperties, OpenfabricPropertiesUpdater, > +use crate::common::valid::Validatable; > +use crate::sdn::fabric::{ > + section_config::protocol::{ > + openfabric::{ > + OpenfabricDeletableProperties, OpenfabricProperties, OpenfabricPropertiesUpdater, > + }, > + ospf::{OspfDeletableProperties, OspfProperties, OspfPropertiesUpdater}, > }, > - ospf::{OspfDeletableProperties, OspfProperties, OspfPropertiesUpdater}, > + FabricConfigError, > }; > > pub const FABRIC_ID_REGEX_STR: &str = r"(?:[a-zA-Z0-9])(?:[a-zA-Z0-9\-]){0,6}(?:[a-zA-Z0-9])?"; > @@ -195,6 +199,18 @@ impl Fabric { > } > } > > +impl Validatable for Fabric { > + type Error = FabricConfigError; > + > + /// Validate the [Fabric] by calling the validation function for the respective protocol. > + fn validate(&self) -> Result<(), Self::Error> { > + match self { > + Fabric::Openfabric(fabric_section) => fabric_section.validate(), > + Fabric::Ospf(fabric_section) => fabric_section.validate(), > + } > + } > +} > + > impl From> for Fabric { > fn from(section: FabricSection) -> Self { > Fabric::Openfabric(section) > diff --git a/proxmox-ve-config/src/sdn/fabric/section_config/node.rs b/proxmox-ve-config/src/sdn/fabric/section_config/node.rs > index 6bccbb7468ed..b04b295db64e 100644 > --- a/proxmox-ve-config/src/sdn/fabric/section_config/node.rs > +++ b/proxmox-ve-config/src/sdn/fabric/section_config/node.rs > @@ -10,10 +10,12 @@ use proxmox_schema::{ > StringSchema, UpdaterType, > }; > > +use crate::common::valid::Validatable; > use crate::sdn::fabric::section_config::{ > fabric::{FabricId, FABRIC_ID_REGEX_STR}, > protocol::{openfabric::OpenfabricNodeProperties, ospf::OspfNodeProperties}, > }; > +use crate::sdn::fabric::FabricConfigError; > > pub const NODE_ID_REGEX_STR: &str = r"(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]){0,61}(?:[a-zA-Z0-9]){0,1})"; > > @@ -212,6 +214,17 @@ impl Node { > } > } > > +impl Validatable for Node { > + type Error = FabricConfigError; > + > + fn validate(&self) -> Result<(), Self::Error> { > + match self { > + Node::Openfabric(node_section) => node_section.validate(), > + Node::Ospf(node_section) => node_section.validate(), > + } > + } > +} > + > impl From> for Node { > fn from(value: NodeSection) -> Self { > Self::Openfabric(value) > diff --git a/proxmox-ve-config/src/sdn/fabric/section_config/protocol/openfabric.rs b/proxmox-ve-config/src/sdn/fabric/section_config/protocol/openfabric.rs > index 156ff2bae3d6..ccbde63e1db4 100644 > --- a/proxmox-ve-config/src/sdn/fabric/section_config/protocol/openfabric.rs > +++ b/proxmox-ve-config/src/sdn/fabric/section_config/protocol/openfabric.rs > @@ -6,7 +6,13 @@ use serde::{Deserialize, Serialize}; > use proxmox_schema::{api, property_string::PropertyString, ApiStringFormat, Updater}; > use proxmox_sdn_types::openfabric::{CsnpInterval, HelloInterval, HelloMultiplier}; > > -use crate::sdn::fabric::section_config::interface::InterfaceName; > +use crate::{ > + common::valid::Validatable, > + sdn::fabric::{ > + section_config::{fabric::FabricSection, interface::InterfaceName, node::NodeSection}, > + FabricConfigError, > + }, > +}; > > /// Protocol-specific options for an OpenFabric Fabric. > #[api] > @@ -24,6 +30,21 @@ pub struct OpenfabricProperties { > pub(crate) csnp_interval: Option, > } > > +impl Validatable for FabricSection { > + type Error = FabricConfigError; > + > + /// Validates the [FabricSection]. > + /// > + /// Checks if we have either IPv4-prefix or IPv6-prefix. If both are not set, return an error. > + fn validate(&self) -> Result<(), Self::Error> { > + if self.ip_prefix().is_none() && self.ip6_prefix().is_none() { > + return Err(FabricConfigError::FabricNoIpPrefix(self.id().to_string())); > + } > + > + Ok(()) > + } > +} > + > #[derive(Debug, Clone, Serialize, Deserialize, Hash)] > #[serde(rename_all = "snake_case")] > pub enum OpenfabricDeletableProperties { > @@ -61,6 +82,21 @@ impl OpenfabricNodeProperties { > } > } > > +impl Validatable for NodeSection { > + type Error = FabricConfigError; > + > + /// Validates the [FabricSection]. > + /// > + /// Checks if we have either an IPv4 or an IPv6 address. If neither is set, return an error. > + fn validate(&self) -> Result<(), Self::Error> { > + if self.ip().is_none() && self.ip6().is_none() { > + return Err(FabricConfigError::NodeNoIp(self.id().to_string())); > + } > + > + Ok(()) > + } > +} > + > #[derive(Debug, Clone, Serialize, Deserialize)] > #[serde(rename_all = "snake_case")] > pub enum OpenfabricNodeDeletableProperties { > diff --git a/proxmox-ve-config/src/sdn/fabric/section_config/protocol/ospf.rs b/proxmox-ve-config/src/sdn/fabric/section_config/protocol/ospf.rs > index 8c94c9e10432..b783279e9089 100644 > --- a/proxmox-ve-config/src/sdn/fabric/section_config/protocol/ospf.rs > +++ b/proxmox-ve-config/src/sdn/fabric/section_config/protocol/ospf.rs > @@ -6,7 +6,13 @@ use serde::{Deserialize, Serialize}; > > use proxmox_schema::{api, property_string::PropertyString, ApiStringFormat, Updater}; > > -use crate::sdn::fabric::section_config::interface::InterfaceName; > +use crate::{ > + common::valid::Validatable, > + sdn::fabric::{ > + section_config::{fabric::FabricSection, interface::InterfaceName, node::NodeSection}, > + FabricConfigError, > + }, > +}; > > #[api] > #[derive(Debug, Clone, Serialize, Deserialize, Updater, Hash)] > @@ -25,6 +31,26 @@ impl OspfProperties { > } > } > > +impl Validatable for FabricSection { > + type Error = FabricConfigError; > + > + /// Validate the [FabricSection]. > + /// > + /// Checks if the ip-prefix (IPv4) is set. If not, then return an error. > + /// If the ip6-prefix (IPv6) is set, also return an error, as OSPF doesn't support IPv6. > + fn validate(&self) -> Result<(), Self::Error> { > + if self.ip_prefix().is_none() { > + return Err(FabricConfigError::FabricNoIpPrefix(self.id().to_string())); > + } > + > + if self.ip6_prefix().is_some() { > + return Err(FabricConfigError::NoIpv6("ospf".to_string())); > + } > + > + Ok(()) > + } > +} > + > #[derive(Debug, Clone, Serialize, Deserialize)] > #[serde(rename_all = "snake_case", untagged)] > pub enum OspfDeletableProperties {} > @@ -58,6 +84,25 @@ impl OspfNodeProperties { > } > } > > +impl Validatable for NodeSection { > + type Error = FabricConfigError; > + > + /// Validate the [NodeSection]. > + /// > + /// Error if the IPv4 address is not set. Error if the IPv6 address is set (OSPF does not > + /// support IPv6). > + fn validate(&self) -> Result<(), Self::Error> { > + if self.ip().is_none() { > + return Err(FabricConfigError::NodeNoIp(self.id().to_string())); > + } > + if self.ip6().is_some() { > + return Err(FabricConfigError::NoIpv6("ospf".to_string())); > + } > + > + Ok(()) > + } > +} > + > #[derive(Debug, Clone, Serialize, Deserialize)] > #[serde(rename_all = "snake_case", untagged)] > pub enum OspfNodeDeletableProperties { > -- > 2.39.5 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel