On Wed, Jul 02, 2025 at 04:50:09PM +0200, Gabriel Goller wrote:
> From: Stefan Hanreich <s.hanre...@proxmox.com>
>
> 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<T>. 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 <g.gol...@proxmox.com>
> Signed-off-by: Stefan Hanreich <s.hanre...@proxmox.com>
> ---
> 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<T>(HashSet<T>);
>
> 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<T>::into_inner`].
> +#[repr(transparent)]
> +#[derive(Clone, Default, Debug)]
> +pub struct Valid<T>(T);
> +
> +impl<T> Valid<T> {
> + /// returns the wrapped value owned, consumes the Valid struct
> + pub fn into_inner(self) -> T {
> + self.0
> + }
> +}
> +
> +impl<T> Deref for Valid<T> {
> + type Target = T;
> +
> + fn deref(&self) -> &T {
> + &self.0
> + }
> +}
> +
> +impl<T> AsRef<T> for Valid<T> {
> + 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<T>`].
> +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<T>`] if
> + /// validation succeeds.
> + fn into_valid(self) -> Result<Valid<Self>, 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<String>)`.
> // 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<Fabric> 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<FabricSection<OpenfabricProperties>> for Fabric {
> fn from(section: FabricSection<OpenfabricProperties>) -> 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<NodeSection<OpenfabricNodeProperties>> for Node {
> fn from(value: NodeSection<OpenfabricNodeProperties>) -> 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<CsnpInterval>,
> }
>
> +impl Validatable for FabricSection<OpenfabricProperties> {
> + type Error = FabricConfigError;
> +
> + /// Validates the [FabricSection<OpenfabricProperties>].
> + ///
> + /// 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<OpenfabricNodeProperties> {
> + type Error = FabricConfigError;
> +
> + /// Validates the [FabricSection<OpenfabricProperties>].
> + ///
> + /// 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<OspfProperties> {
> + type Error = FabricConfigError;
> +
> + /// Validate the [FabricSection<OspfProperties>].
> + ///
> + /// 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<OspfNodeProperties> {
> + type Error = FabricConfigError;
> +
> + /// Validate the [NodeSection<OspfNodeProperties>].
> + ///
> + /// 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
