Re: [pve-devel] [PATCH proxmox-ve-rs v4 17/22] config: sdn: fabrics: add fabric config

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Wed, Jul 02, 2025 at 04:50:08PM +0200, Gabriel Goller wrote:
> From: Stefan Hanreich <s.hanreich@proxmox.com>
> 
> The FabricConfig transforms the flat section configuration into its
> hierarchical representation, which makes querying and validating the
> fabrics configuration more ergonomic.
> 
> It provides the CRUD methods for safely manipulating the fabric
> configuration, while checking for possible errors. It is intended to
> be the interface for external users to use the fabric configuration.
> 
> By encapsulating the configuration into this struct, we can always
> assure that invariants are upheld and many of them can actually be
> checked at compile time (e.g. proper combination of FabricSection and
> NodeSection in the hierarchy via the Entry struct).
> 
> It uses the Fabric and Node enums foremost in its public API, so
> adding new protocols does not change the public API. This enables us
> to write generic API methods that do not need to be updated when
> adding new protocols.
> 
> If so desired, users can still access the protocol-specific properties
> by matching on the FabricEntry enum.
> 
> Co-authored-by: Gabriel Goller <g.goller@proxmox.com>
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  proxmox-ve-config/src/sdn/fabric/mod.rs | 516 ++++++++++++++++++++++++
>  1 file changed, 516 insertions(+)
> 
> diff --git a/proxmox-ve-config/src/sdn/fabric/mod.rs b/proxmox-ve-config/src/sdn/fabric/mod.rs
> index 007be6a3fd8e..3342a7053d3f 100644
> --- a/proxmox-ve-config/src/sdn/fabric/mod.rs
> +++ b/proxmox-ve-config/src/sdn/fabric/mod.rs
> @@ -1 +1,517 @@
>  pub mod section_config;
> +
> +use std::collections::BTreeMap;
> +use std::marker::PhantomData;
> +use std::ops::Deref;
> +
> +use serde::{Deserialize, Serialize};
> +
> +use crate::sdn::fabric::section_config::{
> +    fabric::{
> +        Fabric, FabricDeletableProperties, FabricId, FabricSection, FabricSectionUpdater,
> +        FabricUpdater,
> +    },
> +    node::{
> +        api::{NodeDataUpdater, NodeDeletableProperties, NodeUpdater},
> +        Node, NodeId, NodeSection,
> +    },
> +    protocol::{
> +        openfabric::{
> +            OpenfabricDeletableProperties, OpenfabricNodeDeletableProperties,
> +            OpenfabricNodeProperties, OpenfabricNodePropertiesUpdater, OpenfabricProperties,
> +            OpenfabricPropertiesUpdater,
> +        },
> +        ospf::{
> +            OspfDeletableProperties, OspfNodeDeletableProperties, OspfNodeProperties,
> +            OspfNodePropertiesUpdater, OspfProperties, OspfPropertiesUpdater,
> +        },
> +    },
> +};
> +
> +#[derive(thiserror::Error, Debug)]
> +pub enum FabricConfigError {
> +    #[error("fabric '{0}' does not exist in configuration")]
> +    FabricDoesNotExist(String),
> +    #[error("node '{0}' does not exist in fabric '{1}'")]
> +    NodeDoesNotExist(String, String),

^ So, in the "usual" cases, `.get()` methods return an `Option`. Where
this is not the case, we often have an empty error.
Eg.: `io::ErrorKind::NotFound` has no content, `std::num::ParseIntError`
does not include the number/digits.
The names are usually added via `.context()` or some such.
On the other hand, sometimes there are "functional" errors, like
`NulError` in `CString`, since it takes an `Into<Vec<u8>>` it includes
the allocated vector in case the user wants to reuse it in the error
case. Similarly, `Mutex::lock()`'s error is a
`PoisonError<MutexGuard<T>>` which can be turned into the guard to still
access the data...

The use of this below may as well just return an `Option`, but I get
that we want more context, pretty much all cases. I'm just worried we
might end up with more inconsistencies across our various code bases
(callers adding context because they did not expect it to already be
there), so we should probably declare some rules for how to design error
types in our coding style at some point... (This is long overdue!)

Anyway... until we actually define how we want to deal with error types,
we can leave it like this for now...

> +    #[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),
> +    // should usually not occur, but we still check for it nonetheless
> +    #[error("mismatched fabric_id")]
> +    FabricIdMismatch,
> +}
> +
> +/// An entry in a [`FabricConfig`].
> +///
> +/// It enforces compatible types for its containing [`FabricSection`] and [`NodeSection`] via the
> +/// generic parameters, so only Nodes and Fabrics with compatible types can be inserted into an
> +/// entry.
> +#[derive(Debug, Clone, Serialize, Deserialize, Hash)]
> +pub struct Entry<F, N> {
> +    // we want to store the enum structs Fabric & Node here, in order to have access to the
> +    // properties and methods defined on the enum itself.
> +    // In order to still be able to type-check that an Entry contains the right combination of
> +    // NodeSection and FabricSection, we type hint the actual types wrapped into Fabric & Node here
> +    // via PhantomData and only allow insertion of the proper types via the provided methods.
> +    #[serde(skip)]
> +    _phantom_fabric: PhantomData<FabricSection<F>>,
> +    #[serde(skip)]
> +    _phantom_node: PhantomData<NodeSection<N>>,

Technically, since this struct *contains* neither N nor F, this could
use `PhantomData<fn() -> FooSection<X>>`. Here this probably won't make
a difference. In general, though, the way you use the generic type of
`PhantomData` influences auto-traits (Send/Sync), variance and
dropchecking.

Since this struct's Send/Sync don't depend on neither N nor F, the
`fn() -> …` version would be suitable.

See https://doc.rust-lang.org/nomicon/phantom-data.html

> +
> +    fabric: Fabric,
> +    nodes: BTreeMap<NodeId, Node>,
> +}
> +
> +impl<F, N> Entry<F, N>
> +where
> +    Fabric: From<FabricSection<F>>,
> +    Node: From<NodeSection<N>>,
> +{
> +    /// Create a new [`Entry`] from the passed [`FabricSection<F>`] with no nodes.
> +    fn new(fabric: FabricSection<F>) -> Self {
> +        Self {
> +            fabric: fabric.into(),
> +            nodes: Default::default(),
> +            _phantom_fabric: Default::default(),
> +            _phantom_node: Default::default(),

^ `PhantomData` is shorter, const and more obvious than `Default::default()`.

> +        }
> +    }
> +
> +    /// Adds a node to this entry
> +    ///
> +    /// # Errors

Please include a newline here.

> +    /// Returns an error if the node's fabric_id doesn't match this entry's fabric_id
> +    /// or if a node with the same ID already exists in this entry.
> +    fn add_node(&mut self, node: NodeSection<N>) -> Result<(), FabricConfigError> {
> +        if self.nodes.contains_key(node.id().node_id()) {
> +            return Err(FabricConfigError::DuplicateNode(
> +                node.id().node_id().to_string(),
> +                self.fabric.id().to_string(),
> +            ));
> +        }
> +
> +        if node.id().fabric_id() != self.fabric.id() {
> +            return Err(FabricConfigError::FabricIdMismatch);
> +        }
> +
> +        self.nodes.insert(node.id().node_id().clone(), node.into());
> +
> +        Ok(())
> +    }
> +
> +    /// Get a reference to the node with the passed node_id. Return an error if the node doesn't exist.
> +    fn get_node(&self, id: &NodeId) -> Result<&Node, FabricConfigError> {
> +        self.nodes.get(id).ok_or_else(|| {
> +            FabricConfigError::NodeDoesNotExist(id.to_string(), self.fabric.id().to_string())
> +        })
> +    }
> +
> +    /// Get a mutable reference to the Node with the passed node_id.
> +    fn get_node_mut(&mut self, id: &NodeId) -> Result<&mut Node, FabricConfigError> {
> +        self.nodes.get_mut(id).ok_or_else(|| {
> +            FabricConfigError::NodeDoesNotExist(id.to_string(), self.fabric.id().to_string())
> +        })
> +    }
> +
> +    /// Removes and returns a node with the specified node_id from this entry.
> +    ///
> +    /// # Errors
> +    /// Returns `FabricConfigError::NodeDoesNotExist` if no node with the given node_id exists.
> +    fn delete_node(&mut self, id: &NodeId) -> Result<Node, FabricConfigError> {
> +        self.nodes.remove(id).ok_or_else(|| {
> +            FabricConfigError::NodeDoesNotExist(id.to_string(), self.fabric.id().to_string())
> +        })
> +    }
> +
> +    /// Get entry as a (Fabric, Vec<Node>) pair. This consumes the Entry.
> +    fn into_pair(self) -> (Fabric, Vec<Node>) {
> +        (self.fabric, self.nodes.into_values().collect())
> +    }
> +}
> +
> +impl Entry<OpenfabricProperties, OpenfabricNodeProperties> {
> +    /// Get the OpenFabric fabric config.
> +    ///
> +    /// This method is implemented for [Entry<OpenfabricProperties, OpenfabricNodeProperties>],
> +    /// so it is guaranteed that a [FabricSection<OpenfabricProperties>] is returned.
> +    pub fn fabric_section(&self) -> &FabricSection<OpenfabricProperties> {
> +        if let Fabric::Openfabric(section) = &self.fabric {
> +            return section;
> +        }
> +
> +        unreachable!();
> +    }
> +
> +    /// Get the OpenFabric node config for the given node_id.
> +    ///
> +    /// This method is implemented for [Entry<OpenfabricProperties, OpenfabricNodeProperties>],
> +    /// so it is guaranteed that a [NodeSection<OpenfabricNodeProperties>] is returned.
> +    /// An error is returned if the node is not found.
> +    pub fn node_section(
> +        &self,
> +        id: &NodeId,
> +    ) -> Result<&NodeSection<OpenfabricNodeProperties>, FabricConfigError> {
> +        if let Node::Openfabric(section) = self.get_node(id)? {
> +            return Ok(section);
> +        }
> +
> +        unreachable!();
> +    }
> +}
> +
> +impl Entry<OspfProperties, OspfNodeProperties> {
> +    /// Get the OSPF fabric config.
> +    ///
> +    /// This method is implemented for [Entry<OspfProperties, OspfNodeProperties>],
> +    /// so it is guaranteed that a [FabricSection<OspfProperties>] is returned.
> +    pub fn fabric_section(&self) -> &FabricSection<OspfProperties> {
> +        if let Fabric::Ospf(section) = &self.fabric {
> +            return section;
> +        }
> +
> +        unreachable!();
> +    }
> +
> +    /// Get the OSPF node config for the given node_id.
> +    ///
> +    /// This method is implemented for [Entry<OspfProperties, OspfNodeProperties>],
> +    /// so it is guaranteed that a [NodeSection<OspfNodeProperties>] is returned.
> +    /// An error is returned if the node is not found.
> +    pub fn node_section(
> +        &self,
> +        id: &NodeId,
> +    ) -> Result<&NodeSection<OspfNodeProperties>, FabricConfigError> {
> +        if let Node::Ospf(section) = self.get_node(id)? {
> +            return Ok(section);
> +        }
> +
> +        unreachable!();
> +    }
> +}
> +
> +/// All possible entries in a [`FabricConfig`].
> +///
> +/// It utilizes the [`Entry`] struct to validate proper combinations of [`FabricSection`] and
> +/// [`NodeSection`].
> +#[derive(Debug, Clone, Serialize, Deserialize, Hash)]
> +pub enum FabricEntry {
> +    Openfabric(Entry<OpenfabricProperties, OpenfabricNodeProperties>),
> +    Ospf(Entry<OspfProperties, OspfNodeProperties>),
> +}
> +
> +impl FabricEntry {
> +    /// Adds a node to the fabric entry.
> +    /// The node must match the protocol type of the fabric entry.
> +    pub fn add_node(&mut self, node: Node) -> Result<(), FabricConfigError> {
> +        match (self, node) {
> +            (FabricEntry::Openfabric(entry), Node::Openfabric(node_section)) => {
> +                entry.add_node(node_section)
> +            }
> +            (FabricEntry::Ospf(entry), Node::Ospf(node_section)) => entry.add_node(node_section),
> +            _ => Err(FabricConfigError::ProtocolMismatch),
> +        }
> +    }
> +
> +    /// Get a reference to a Node specified by the node_id. Returns an error if the node is not
> +    /// found.
> +    pub fn get_node(&self, id: &NodeId) -> Result<&Node, FabricConfigError> {
> +        match self {
> +            FabricEntry::Openfabric(entry) => entry.get_node(id),
> +            FabricEntry::Ospf(entry) => entry.get_node(id),
> +        }
> +    }
> +
> +    /// Get a mutable reference to a Node specified by the node_id. Returns an error if the node is not
> +    /// found.
> +    pub fn get_node_mut(&mut self, id: &NodeId) -> Result<&mut Node, FabricConfigError> {
> +        match self {
> +            FabricEntry::Openfabric(entry) => entry.get_node_mut(id),
> +            FabricEntry::Ospf(entry) => entry.get_node_mut(id),
> +        }
> +    }
> +
> +    /// Update the Node with the specified node_id using the passed [NodeUpdater].
> +    pub fn update_node(
> +        &mut self,
> +        id: &NodeId,
> +        updater: NodeUpdater,
> +    ) -> Result<(), FabricConfigError> {
> +        let node = self.get_node_mut(id)?;
> +
> +        match (node, updater) {
> +            (Node::Openfabric(node_section), NodeUpdater::Openfabric(updater)) => {
> +                let NodeDataUpdater::<
> +                    OpenfabricNodePropertiesUpdater,
> +                    OpenfabricNodeDeletableProperties,
> +                > {
> +                    ip,
> +                    ip6,
> +                    properties: OpenfabricNodePropertiesUpdater { interfaces },
> +                    delete,
> +                } = updater;
> +
> +                if let Some(ip) = ip {
> +                    node_section.ip = Some(ip);
> +                }
> +
> +                if let Some(ip) = ip6 {
> +                    node_section.ip6 = Some(ip);
> +                }
> +
> +                if let Some(interfaces) = interfaces {
> +                    node_section.properties.interfaces = interfaces;
> +                }
> +
> +                for property in delete {
> +                    match property {
> +                        NodeDeletableProperties::Ip => node_section.ip = None,
> +                        NodeDeletableProperties::Ip6 => node_section.ip6 = None,
> +                        NodeDeletableProperties::Protocol(
> +                            OpenfabricNodeDeletableProperties::Interfaces,
> +                        ) => node_section.properties.interfaces = Vec::new(),
> +                    }
> +                }
> +
> +                Ok(())
> +            }
> +            (Node::Ospf(node_section), NodeUpdater::Ospf(updater)) => {
> +                let NodeDataUpdater::<OspfNodePropertiesUpdater, OspfNodeDeletableProperties> {
> +                    ip,
> +                    ip6,
> +                    properties: OspfNodePropertiesUpdater { interfaces },
> +                    delete,
> +                } = updater;
> +
> +                if let Some(ip) = ip {
> +                    node_section.ip = Some(ip);
> +                }
> +
> +                if let Some(ip) = ip6 {
> +                    node_section.ip6 = Some(ip);
> +                }
> +
> +                if let Some(interfaces) = interfaces {
> +                    node_section.properties.interfaces = interfaces;
> +                }
> +
> +                for property in delete {
> +                    match property {
> +                        NodeDeletableProperties::Ip => node_section.ip = None,
> +                        NodeDeletableProperties::Ip6 => node_section.ip6 = None,
> +                        NodeDeletableProperties::Protocol(
> +                            OspfNodeDeletableProperties::Interfaces,
> +                        ) => node_section.properties.interfaces = Vec::new(),
> +                    }
> +                }
> +
> +                Ok(())
> +            }
> +            _ => Err(FabricConfigError::ProtocolMismatch),
> +        }
> +    }
> +
> +    /// Get an iterator over all the nodes in this fabric.
> +    pub fn nodes(&self) -> impl Iterator<Item = (&NodeId, &Node)> + '_ {
> +        match self {
> +            FabricEntry::Openfabric(entry) => entry.nodes.iter(),
> +            FabricEntry::Ospf(entry) => entry.nodes.iter(),
> +        }
> +    }
> +
> +    /// Delete the node specified with the node_id. Returns an error if it doesn't exist.
> +    pub fn delete_node(&mut self, id: &NodeId) -> Result<Node, FabricConfigError> {
> +        match self {
> +            FabricEntry::Openfabric(entry) => entry.delete_node(id),
> +            FabricEntry::Ospf(entry) => entry.delete_node(id),
> +        }
> +    }
> +
> +    /// Consume this entry and return a (Fabric, Vec<Node>) pair. This is used to write to the
> +    /// section-config file.
> +    pub fn into_section_config(self) -> (Fabric, Vec<Node>) {
> +        match self {
> +            FabricEntry::Openfabric(entry) => entry.into_pair(),
> +            FabricEntry::Ospf(entry) => entry.into_pair(),
> +        }
> +    }
> +
> +    /// Get a reference to the Fabric.
> +    pub fn fabric(&self) -> &Fabric {
> +        match self {
> +            FabricEntry::Openfabric(entry) => &entry.fabric,
> +            FabricEntry::Ospf(entry) => &entry.fabric,
> +        }
> +    }
> +
> +    /// Get a mutable reference to the Fabric.
> +    pub fn fabric_mut(&mut self) -> &mut Fabric {
> +        match self {
> +            FabricEntry::Openfabric(entry) => &mut entry.fabric,
> +            FabricEntry::Ospf(entry) => &mut entry.fabric,
> +        }
> +    }
> +}
> +
> +impl From<Fabric> for FabricEntry {
> +    fn from(fabric: Fabric) -> Self {
> +        match fabric {
> +            Fabric::Openfabric(fabric_section) => {
> +                FabricEntry::Openfabric(Entry::new(fabric_section))
> +            }
> +            Fabric::Ospf(fabric_section) => FabricEntry::Ospf(Entry::new(fabric_section)),
> +        }
> +    }
> +}
> +
> +/// A complete SDN fabric configuration.
> +///
> +/// This struct contains the whole fabric configuration in a tree-like structure (fabrics -> nodes
> +/// -> interfaces).
> +#[derive(Default, Debug, Serialize, Deserialize, Clone, Hash)]
> +pub struct FabricConfig {
> +    fabrics: BTreeMap<FabricId, FabricEntry>,
> +}
> +
> +impl Deref for FabricConfig {
> +    type Target = BTreeMap<FabricId, FabricEntry>;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.fabrics
> +    }
> +}
> +
> +impl FabricConfig {
> +    /// Add a fabric to the [FabricConfig].
> +    ///
> +    /// Returns an error if a fabric with the same name exists.
> +    pub fn add_fabric(&mut self, fabric: Fabric) -> Result<(), FabricConfigError> {
> +        if self.fabrics.contains_key(fabric.id()) {
> +            return Err(FabricConfigError::DuplicateFabric(fabric.id().to_string()));
> +        }
> +
> +        self.fabrics.insert(fabric.id().clone(), fabric.into());
> +
> +        Ok(())
> +    }
> +
> +    /// Get a reference to the fabric with the specified fabric_id.
> +    pub fn get_fabric(&self, id: &FabricId) -> Result<&FabricEntry, FabricConfigError> {
> +        self.fabrics
> +            .get(id)
> +            .ok_or_else(|| FabricConfigError::FabricDoesNotExist(id.to_string()))
> +    }
> +
> +    /// Get a mutable reference to the fabric with the specified fabric_id.
> +    pub fn get_fabric_mut(&mut self, id: &FabricId) -> Result<&mut FabricEntry, FabricConfigError> {
> +        self.fabrics
> +            .get_mut(id)
> +            .ok_or_else(|| FabricConfigError::FabricDoesNotExist(id.to_string()))
> +    }
> +
> +    /// Delete a fabric with the specified fabric_id from the [FabricConfig].
> +    pub fn delete_fabric(&mut self, id: &FabricId) -> Result<FabricEntry, FabricConfigError> {
> +        self.fabrics
> +            .remove(id)
> +            .ok_or_else(|| FabricConfigError::FabricDoesNotExist(id.to_string()))
> +    }
> +
> +    /// Update the fabric specified by the fabric_id using the [FabricUpdater].
> +    pub fn update_fabric(
> +        &mut self,
> +        id: &FabricId,
> +        updater: FabricUpdater,
> +    ) -> Result<(), FabricConfigError> {
> +        let fabric = self.get_fabric_mut(id)?.fabric_mut();
> +
> +        match (fabric, updater) {
> +            (Fabric::Openfabric(fabric_section), FabricUpdater::Openfabric(updater)) => {
> +                let FabricSectionUpdater::<
> +                    OpenfabricPropertiesUpdater,
> +                    OpenfabricDeletableProperties,
> +                > {
> +                    ip_prefix,
> +                    ip6_prefix,
> +                    properties:
> +                        OpenfabricPropertiesUpdater {
> +                            hello_interval,
> +                            csnp_interval,
> +                        },
> +                    delete,
> +                } = updater;
> +
> +                if let Some(prefix) = ip_prefix {
> +                    fabric_section.ip_prefix = Some(prefix);
> +                }
> +
> +                if let Some(prefix) = ip6_prefix {
> +                    fabric_section.ip6_prefix = Some(prefix);
> +                }
> +
> +                if let Some(hello_interval) = hello_interval {
> +                    fabric_section.properties.hello_interval = Some(hello_interval);
> +                }
> +
> +                if let Some(csnp_interval) = csnp_interval {
> +                    fabric_section.properties.csnp_interval = Some(csnp_interval);
> +                }
> +
> +                for property in delete {
> +                    match property {
> +                        FabricDeletableProperties::IpPrefix => {
> +                            fabric_section.ip_prefix = None;
> +                        }
> +                        FabricDeletableProperties::Ip6Prefix => {
> +                            fabric_section.ip6_prefix = None;
> +                        }
> +                        FabricDeletableProperties::Protocol(
> +                            OpenfabricDeletableProperties::CsnpInterval,
> +                        ) => fabric_section.properties.csnp_interval = None,
> +                        FabricDeletableProperties::Protocol(
> +                            OpenfabricDeletableProperties::HelloInterval,
> +                        ) => fabric_section.properties.hello_interval = None,
> +                    }
> +                }
> +
> +                Ok(())
> +            }
> +            (Fabric::Ospf(fabric_section), FabricUpdater::Ospf(updater)) => {
> +                let FabricSectionUpdater::<OspfPropertiesUpdater, OspfDeletableProperties> {
> +                    ip_prefix,
> +                    ip6_prefix,
> +                    properties: OspfPropertiesUpdater { area },
> +                    delete,
> +                } = updater;
> +
> +                if let Some(prefix) = ip_prefix {
> +                    fabric_section.ip_prefix = Some(prefix);
> +                }
> +
> +                if let Some(prefix) = ip6_prefix {
> +                    fabric_section.ip6_prefix = Some(prefix);
> +                }
> +
> +                if let Some(area) = area {
> +                    fabric_section.properties.area = area;
> +                }
> +
> +                for property in delete {
> +                    match property {
> +                        FabricDeletableProperties::IpPrefix => {
> +                            fabric_section.ip_prefix = None;
> +                        }
> +                        FabricDeletableProperties::Ip6Prefix => {
> +                            fabric_section.ip6_prefix = None;
> +                        }
> +                    }
> +                }
> +
> +                Ok(())
> +            }
> +            _ => Err(FabricConfigError::ProtocolMismatch),
> +        }
> +    }
> +}
> -- 
> 2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel