[pve-devel] [PATCH proxmox-ve-rs v5 18/22] common: sdn: fabrics: implement validation

[Gabriel Goller <g.goller@proxmox.com>]

From: Stefan Hanreich <s.hanreich@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.goller@proxmox.com>
Signed-off-by: Stefan Hanreich <s.hanreich@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   |  14 ++
 .../src/sdn/fabric/section_config/node.rs     |  13 ++
 .../section_config/protocol/openfabric.rs     |  34 +++
 .../fabric/section_config/protocol/ospf.rs    |  43 ++++
 7 files changed, 355 insertions(+), 1 deletion(-)
 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 86d47b7ba6b3..825f2346d9f3 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, FabricUpdater,
 };
@@ -28,15 +30,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}")]
+    Ipv6Unsupported(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`].
@@ -365,6 +390,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
@@ -382,6 +494,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 2500f1231b31..d41c6d82fd88 100644
--- a/proxmox-ve-config/src/sdn/fabric/section_config/fabric.rs
+++ b/proxmox-ve-config/src/sdn/fabric/section_config/fabric.rs
@@ -7,12 +7,14 @@ use proxmox_schema::{
     Updater, UpdaterType,
 };
 
+use crate::common::valid::Validatable;
 use crate::sdn::fabric::section_config::protocol::openfabric::{
     OpenfabricDeletableProperties, OpenfabricProperties, OpenfabricPropertiesUpdater,
 };
 use crate::sdn::fabric::section_config::protocol::ospf::{
     OspfDeletableProperties, OspfProperties, OspfPropertiesUpdater,
 };
+use crate::sdn::fabric::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 +197,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 02b36b55ce5b..b8a14d2268a0 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 090f39798fc8..9a9f862d5190 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,11 @@ use serde::{Deserialize, Serialize};
 use proxmox_schema::{api, property_string::PropertyString, ApiStringFormat, Updater};
 use proxmox_sdn_types::openfabric::{CsnpInterval, HelloInterval, HelloMultiplier};
 
+use crate::common::valid::Validatable;
+use crate::sdn::fabric::section_config::fabric::FabricSection;
 use crate::sdn::fabric::section_config::interface::InterfaceName;
+use crate::sdn::fabric::section_config::node::NodeSection;
+use crate::sdn::fabric::FabricConfigError;
 
 /// Protocol-specific options for an OpenFabric Fabric.
 #[api]
@@ -24,6 +28,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 +80,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..1c8ce1da865e 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,11 @@ use serde::{Deserialize, Serialize};
 
 use proxmox_schema::{api, property_string::PropertyString, ApiStringFormat, Updater};
 
+use crate::common::valid::Validatable;
+use crate::sdn::fabric::section_config::fabric::FabricSection;
 use crate::sdn::fabric::section_config::interface::InterfaceName;
+use crate::sdn::fabric::section_config::node::NodeSection;
+use crate::sdn::fabric::FabricConfigError;
 
 #[api]
 #[derive(Debug, Clone, Serialize, Deserialize, Updater, Hash)]
@@ -25,6 +29,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::Ipv6Unsupported("ospf".to_string()));
+        }
+
+        Ok(())
+    }
+}
+
 #[derive(Debug, Clone, Serialize, Deserialize)]
 #[serde(rename_all = "snake_case", untagged)]
 pub enum OspfDeletableProperties {}
@@ -58,6 +82,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::Ipv6Unsupported("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