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

[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       | 126 +++++++++++++++++-
 .../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, 296 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 ef09791..9fde536 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 0000000..1f92ef9
--- /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 3342a70..b7d1f2d 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,12 +36,26 @@ 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("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),
     // should usually not occur, but we still check for it nonetheless
     #[error("mismatched fabric_id")]
     FabricIdMismatch,
@@ -367,6 +383,75 @@ 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() {
+            if let Some(ip) = node.ip() {
+                // Fabric needs to have an IPv4 prefix if a node has an IPv4 configured
+                let prefix = fabric.ip_prefix().ok_or_else(|| {
+                    FabricConfigError::FabricNoIpPrefixForNode(
+                        fabric.id().to_string(),
+                        ip.to_string(),
+                    )
+                })?;
+
+                // Fabric prefix needs to contain the node IP
+                if !prefix.contains_address(&ip) {
+                    return Err(FabricConfigError::NodeIpOutsideFabricRange(
+                        ip.to_string(),
+                        prefix.to_string(),
+                    ));
+                }
+            }
+
+            if let Some(ip) = node.ip6() {
+                // Fabric needs to have an IPv6 prefix if a node has an IPv6 configured
+                let prefix = fabric.ip6_prefix().ok_or_else(|| {
+                    FabricConfigError::FabricNoIpPrefixForNode(
+                        fabric.id().to_string(),
+                        ip.to_string(),
+                    )
+                })?;
+
+                // 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 +469,45 @@ 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
+    fn validate(&self) -> Result<(), FabricConfigError> {
+        let mut node_interfaces = HashSet::new();
+
+        // validate that each (node, interface) combination exists only once across all fabrics
+        for entry in self.fabrics.values() {
+            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 75a3093..b8d7649 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 b1b2034..4dca1c9 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 156ff2b..ccbde63 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 4c368fa..f8010a3 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)]
@@ -22,6 +28,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 {}
@@ -55,6 +81,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