Re: [pve-devel] [PATCH proxmox-ve-rs 02/11] add proxmox-frr crate with frr types

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

On 03.03.2025 17:29, Stefan Hanreich wrote:
>This uses stuff from a later patch, doesn't it? Shouldn't the order of
>patches 2 and 3 be flipped?

oops, yeah mb.

>> [snip]
>> diff --git a/proxmox-frr/src/lib.rs b/proxmox-frr/src/lib.rs
>> new file mode 100644
>> index 000000000000..ceef82999619
>> --- /dev/null
>> +++ b/proxmox-frr/src/lib.rs
>> @@ -0,0 +1,223 @@
>> +pub mod common;
>> +pub mod openfabric;
>> +pub mod ospf;
>> +
>> +use std::{collections::{hash_map::Entry, HashMap}, fmt::Display, str::FromStr};
>> +
>> +use common::{FrrWord, FrrWordError};
>> +use proxmox_ve_config::sdn::fabric::common::Hostname;
>
>Maybe move it to network-types, if it is always needed? Seems like a
>better fit. Especially since the dependency is optional.

Agree.

>> [snip]
>> +#[derive(Clone, Debug, PartialEq, Eq, Hash, DeserializeFromStr, SerializeDisplay)]
>> +pub enum RouterName {
>> +    OpenFabric(openfabric::OpenFabricRouterName),
>> +    Ospf(ospf::OspfRouterName),
>> +}
>> +
>> +impl From<openfabric::OpenFabricRouterName> for RouterName {
>> +    fn from(value: openfabric::OpenFabricRouterName) -> Self {
>> +        Self::OpenFabric(value)
>> +    }
>> +}
>> +
>> +impl Display for RouterName {
>> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
>> +        match self {
>> +            Self::OpenFabric(r) => r.fmt(f),
>> +            Self::Ospf(r) => r.fmt(f),
>> +        }
>> +    }
>> +}
>> +
>> +impl FromStr for RouterName {
>> +    type Err = RouterNameError;
>> +
>> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
>> +        if let Ok(router) = s.parse() {
>> +            return Ok(Self::OpenFabric(router));
>> +        }
>
>does this make sense here? can we actually make a clear distinction on
>whether this is a OpenFabric / OSPF router name (and for all other
>future RouterNames) from the string alone? I think it might be better to
>explicitly construct the specific type and then make a RouterName out of
>it and do not implement FromStr at all. Or get rid of RouterName
>altogether (see below).
>
>It's also constructing an OpenFabric RouterName but never an OSPF router
>name.

nope, this doesn't make any sense at all, no idea how it got in here.
Removed it and the Deserialize derive for all the parent types as it
isn't needed anyway.

>> +        Err(RouterNameError::InvalidName)
>> +    }
>> +}
>> +
>> +/// The interface name is the same on ospf and openfabric, but it is an enum so we can have two
>> +/// different entries in the hashmap. This allows us to have an interface in an ospf and openfabric
>> +/// fabric.
>> +#[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize, Hash)]
>> +pub enum InterfaceName {
>> +    OpenFabric(FrrWord),
>> +    Ospf(FrrWord),
>> +}
>
>maybe this should be a struct representing a linux interface name
>(nul-terminated 16byte string) instead of an FrrWord?

Makes sense, added a different InterfaceName struct that wraps a String and
checks the length upon creation.

>> [snip]
>> +#[derive(Clone, Debug, PartialEq, Eq, Default, Deserialize, Serialize)]
>> +pub struct FrrConfig {
>> +    router: HashMap<RouterName, Router>,
>> +    interfaces: HashMap<InterfaceName, Interface>,
>
>are we ever querying the frr router/interface by name? judging from the
>public API we don't and only iterate over it. we could move the name
>into the Router/Interface then, this would ensure that the name always
>fits the concrete router. We could probably also make converting from
>the section config easier then and possibly save us the whole RouterName
>struct.
>
>Are duplicates possible with how the ID in the SectionConfig works? If
>we want to avoid that, we could probably use other ways.

The problem here are duplicate interfaces over different fabrics
(areas), which are not allowed.

These are not checked by SectionConfig because the key is:
"<area>_<interface-name>".

We could simply implement checks when inserting, sure, but why not just
use HashMaps and seal the deal? :)

>> +}
>> +
>> +impl FrrConfig {
>> +    pub fn new() -> Self {
>> +        Self::default()
>> +    }
>> +
>> +    #[cfg(feature = "config-ext")]
>> +    pub fn builder() -> FrrConfigBuilder {
>> +        FrrConfigBuilder::default()
>> +    }
>
>see above for if we really need a builder here or if implementing
>conversion traits suffices.

My idea is that we can add the bgp stuff here as well (soonTM).
I wanted to avoid a TryFrom<(FabricConfig, BgpConfig, ...)>
implementation to be honest.

>> +    pub fn router(&self) -> impl Iterator<Item = (&RouterName, &Router)> + '_ {
>> +        self.router.iter()
>> +    }
>> +
>> +    pub fn interfaces(&self) -> impl Iterator<Item = (&InterfaceName, &Interface)> + '_ {
>> +        self.interfaces.iter()
>> +    }
>> +}
>> +
>> +#[derive(Default)]
>> +#[cfg(feature = "config-ext")]
>> +pub struct FrrConfigBuilder {
>> +    fabrics: FabricConfig,
>> +    //bgp: Option<internal::BgpConfig>
>> +}
>> +
>> +#[cfg(feature = "config-ext")]
>> +impl FrrConfigBuilder {
>> +    pub fn add_fabrics(mut self, fabric: FabricConfig) -> FrrConfigBuilder {
>> +        self.fabrics = fabric;
>> +        self
>> +    }
>
>From<FabricConfig> might be better if it replaces self.fabrics /
>consumes FabricConfig anyway? Maybe even TryFrom<FabricConfig> for
>FrrConfig itself?

see above.

>> +
>> +    pub fn build(self, current_node: &str) -> Result<FrrConfig, anyhow::Error> {
>> +        let mut router: HashMap<RouterName, Router> = HashMap::new();
>> +        let mut interfaces: HashMap<InterfaceName, Interface> = HashMap::new();
>> +
>> +        if let Some(openfabric) = self.fabrics.openfabric() {
>> +            // openfabric
>> +            openfabric
>> +                .fabrics()
>> +                .iter()
>> +                .try_for_each(|(fabric_id, fabric_config)| {
>> +                    let node_config = fabric_config.nodes().get(&Hostname::new(current_node));
>> +                    if let Some(node_config) = node_config {
>> +                        let ofr = openfabric::OpenFabricRouter::from((fabric_config, node_config));
>> +                        let router_item = Router::OpenFabric(ofr);
>> +                        let router_name = RouterName::OpenFabric(
>> +                            openfabric::OpenFabricRouterName::try_from(fabric_id)?,
>> +                        );
>> +                        router.insert(router_name.clone(), router_item);
>> +                        node_config.interfaces().try_for_each(|interface| {
>> +                            let mut openfabric_interface: openfabric::OpenFabricInterface =
>> +                                (fabric_id, interface).try_into()?;
>
>The only fallible thing here is constructing the RouterName, so we could
>just clone it from above and make a
>OpenFabricInterface::from_section_config() method that accepts the name
>+ sectionconfig structs?

fair – or just have the 
TryFrom<(&internal::FabricId,&internal::Interface)> for OpenFabricInterface
be a 
TryFrom<(&OpenFabricRouterName, &internal::Interface)> for OpenFabricInterface

imo that's even better.

>> +                            // If no specific hello_interval is set, get default one from fabric
>> +                            // config
>> +                            if openfabric_interface.hello_interval().is_none() {
>> +                                openfabric_interface
>> +                                    .set_hello_interval(fabric_config.hello_interval().clone());
>> +                            }
>> +                            let interface_name = InterfaceName::OpenFabric(FrrWord::from_str(interface.name())?);
>> +                            // Openfabric doesn't allow an interface to be in multiple openfabric
>> +                            // fabrics. Frr will just ignore it and take the first one.
>> +                            if let Entry::Vacant(e) = interfaces.entry(interface_name) {
>> +                                e.insert(openfabric_interface.into());
>> +                            } else {
>> +                                tracing::warn!("An interface cannot be in multiple openfabric fabrics");
>> +                            }
>
>if let Err(_) = interfaces.try_insert(..) maybe (if we keep the HashMap)?

try_insert is unstable :(

>> +                            Ok::<(), anyhow::Error>(())
>> +                        })?;
>> +                    } else {
>> +                        tracing::warn!("no node configuration for fabric \"{fabric_id}\" – this fabric is not configured for this node.");
>
>Maybe it would make sense to split this into two functions, where we
>could just return early if there is no configuration for this node?

do you mean one function for ospf and one for openfabric?
I'd agree to that.

>Then here an early return would suffice, since otherwise the log gets
>spammed on nodes that are simply not part of a fabric (which is
>perfectly valid)?

True, did not consider that. I think it's ok if we just make this a
debug print. No need to add some fancy checks imo.

>> [snip]
>> +#[derive(Clone, Debug, PartialEq, Eq, Hash, Deserialize, Serialize)]
>> +pub struct OpenFabricInterface {
>> +    // Note: an interface can only be a part of a single fabric (so no vec needed here)
>> +    fabric_id: OpenFabricRouterName,
>> +    passive: Option<bool>,
>> +    hello_interval: Option<openfabric::HelloInterval>,
>> +    csnp_interval: Option<openfabric::CsnpInterval>,
>> +    hello_multiplier: Option<openfabric::HelloMultiplier>,
>> +}
>> +
>> +impl OpenFabricInterface {
>> +    pub fn fabric_id(&self) -> &OpenFabricRouterName {
>> +        &self.fabric_id
>> +    }
>> +    pub fn passive(&self) -> &Option<bool> {
>> +        &self.passive
>> +    }
>> +    pub fn hello_interval(&self) -> &Option<openfabric::HelloInterval> {
>> +        &self.hello_interval
>> +    }
>> +    pub fn csnp_interval(&self) -> &Option<openfabric::CsnpInterval> {
>> +        &self.csnp_interval
>> +    }
>> +    pub fn hello_multiplier(&self) -> &Option<openfabric::HelloMultiplier> {
>> +        &self.hello_multiplier
>> +    }
>
>If we implement Copy for those types it's usually just easier to return
>them owned.

true

>> +    pub fn set_hello_interval(&mut self, interval: Option<openfabric::HelloInterval>) {
>
>nit: I usually like impl Into<Option<..>> because it makes the API nicer
>(don't have to write Some(..) all the time ...)

yep.

>> [snip]
>> +#[cfg(feature = "config-ext")]
>> +impl TryFrom<(&internal::FabricId, &internal::Interface)> for OpenFabricInterface {
>> +    type Error = OpenFabricInterfaceError;
>> +
>> +    fn try_from(value: (&internal::FabricId, &internal::Interface)) -> Result<Self, Self::Error> {
>> +        Ok(Self {
>> +            fabric_id: OpenFabricRouterName::try_from(value.0)?,
>> +            passive: value.1.passive(),
>> +            hello_interval: value.1.hello_interval().clone(),
>> +            csnp_interval: value.1.csnp_interval().clone(),
>> +            hello_multiplier: value.1.hello_multiplier().clone(),
>
>We can easily implement Copy for those values, since they're u16.

nice

>> +        })
>> +    }
>> +}
>> +
>> +#[cfg(feature = "config-ext")]
>> +impl TryFrom<&internal::FabricId> for OpenFabricRouterName {
>> +    type Error = RouterNameError;
>> +
>> +    fn try_from(value: &internal::FabricId) -> Result<Self, Self::Error> {
>> +        Ok(OpenFabricRouterName::new(FrrWord::new(value.to_string())?))
>> +    }
>> +}
>> +
>> +#[cfg(feature = "config-ext")]
>> +impl From<(&internal::FabricConfig, &internal::NodeConfig)> for OpenFabricRouter {
>> +    fn from(value: (&internal::FabricConfig, &internal::NodeConfig)) -> Self {
>> +        Self {
>> +            net: value.1.net().to_owned(),
>> +        }
>> +    }
>> +}
>
>We never use value.0 here, but we might if we have some global options,
>right?

Exactly. Although I've been thinking about that. Maybe we should just
all chuck them in to the Intermediate Representation (and expand the
properties to every node) and don't bother with them in the FrrConfig?

>> [snip]
>> +/// The name of the ospf frr router. There is only one ospf fabric possible in frr (ignoring
>> +/// multiple invocations of the ospfd daemon) and the separation is done with areas. Still,
>> +/// different areas have the same frr router, so the name of the router is just "ospf" in "router
>> +/// ospf". This type still contains the Area so that we can insert it in the Hashmap.
>> +#[derive(Clone, Debug, PartialEq, Eq, Hash, Deserialize, Serialize)]
>> +pub struct OspfRouterName(Area);
>> +
>> +impl From<Area> for OspfRouterName {
>> +    fn from(value: Area) -> Self {
>> +        Self(value)
>> +    }
>> +}
>> +
>> +impl Display for OspfRouterName {
>> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
>> +        write!(f, "ospf")
>
>this is missing the area, or?

Nope, this is fine, you can't have more than one ospf router except if
your having multiple ospfd daemons. Note that the router-id also
per-daemon, not per-area.

>> [snip]
>> +/// The OSPF Area. Most commonly, this is just a number, e.g. 5, but sometimes also a
>> +/// pseudo-ipaddress, e.g. 0.0.0.0
>> +#[derive(Clone, Debug, PartialEq, Eq, Hash, Deserialize, Serialize)]
>> +pub struct Area(FrrWord);
>> +
>> +impl TryFrom<FrrWord> for Area {
>> +    type Error = AreaParsingError;
>> +
>> +    fn try_from(value: FrrWord) -> Result<Self, Self::Error> {
>> +        Area::new(value)
>> +    }
>> +}
>> +
>> +impl Area {
>> +    pub fn new(name: FrrWord) -> Result<Self, AreaParsingError> {
>> +        if name.as_ref().parse::<i32>().is_ok() || name.as_ref().parse::<Ipv4Addr>().is_ok() {
>
>use u32 here? otherwise area ID can be negative, which isn't allowed afaict?

true.

Thanks for the review!


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