Re: [pve-devel] [PATCH proxmox-ve-rs 01/11] add crate with common network types

[Stefan Hanreich <s.hanreich@proxmox.com>]

Maybe we should think about internally representing the Network Entity
Title and its parts as bytes rather than strings? So [u8; n]? Since in
practice it's exactly that. I think this would simplify a lot of the
methods here, and obsolete some stuff like e.g. length validation. I've
noted some parts below where I think we could benefit from that.

We could also provide conversion methods for MAC addresses easily then,
although IP addresses might be a bit awkward since they map to network
entity titles as binary-coded decimal and there's no std support for
that so we'd have to brew our own conversions...

Also, theoretically, the Area ID is of variable length, but for our use
case the current additional constraints are fine imo and not worth the
hassle of implementing variable length Area IDs. Might be good to note
this though.

On 2/14/25 14:39, Gabriel Goller wrote:
> The new proxmox-network-types crate holds some common types that are
> used by proxmox-frr, proxmox-ve-config and proxmox-perl-rs. These types
> are here because we don't want proxmox-frr to be a dependency of
> proxmox-ve-config or vice-versa (or at least it should be feature-gated).
> They should be independent.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  Cargo.toml                       |   6 +
>  proxmox-network-types/Cargo.toml |  15 ++
>  proxmox-network-types/src/lib.rs |   1 +
>  proxmox-network-types/src/net.rs | 239 +++++++++++++++++++++++++++++++
>  4 files changed, 261 insertions(+)
>  create mode 100644 proxmox-network-types/Cargo.toml
>  create mode 100644 proxmox-network-types/src/lib.rs
>  create mode 100644 proxmox-network-types/src/net.rs
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index dc7f312fb8a9..e452c931e78c 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -1,6 +1,7 @@
>  [workspace]
>  members = [
>      "proxmox-ve-config",
> +    "proxmox-network-types",
>  ]
>  exclude = [
>      "build",
> @@ -15,3 +16,8 @@ homepage = "https://proxmox.com"
>  exclude = [ "debian" ]
>  rust-version = "1.82"
>  
> +[workspace.dependencies]
> +proxmox-section-config = "2.1.1"
> +serde = "1"
> +serde_with = "3.8.1"
> +thiserror = "1.0.59"
> diff --git a/proxmox-network-types/Cargo.toml b/proxmox-network-types/Cargo.toml
> new file mode 100644
> index 000000000000..93f4df87a59f
> --- /dev/null
> +++ b/proxmox-network-types/Cargo.toml
> @@ -0,0 +1,15 @@
> +[package]
> +name = "proxmox-network-types"
> +version = "0.1.0"
> +authors.workspace = true
> +edition.workspace = true
> +license.workspace = true
> +homepage.workspace = true
> +exclude.workspace = true
> +rust-version.workspace = true
> +
> +[dependencies]
> +thiserror = { workspace = true }
> +anyhow = "1"
> +serde = { workspace = true, features = [ "derive" ] }
> +serde_with = { workspace = true }
> diff --git a/proxmox-network-types/src/lib.rs b/proxmox-network-types/src/lib.rs
> new file mode 100644
> index 000000000000..f9faf2ff6542
> --- /dev/null
> +++ b/proxmox-network-types/src/lib.rs
> @@ -0,0 +1 @@
> +pub mod net;
> diff --git a/proxmox-network-types/src/net.rs b/proxmox-network-types/src/net.rs
> new file mode 100644
> index 000000000000..5fdbe3920800
> --- /dev/null
> +++ b/proxmox-network-types/src/net.rs
> @@ -0,0 +1,239 @@
> +use std::{fmt::Display, str::FromStr};
> +
> +use serde::Serialize;
> +use serde_with::{DeserializeFromStr, SerializeDisplay};
> +use thiserror::Error;
> +
> +#[derive(Error, Debug)]
> +pub enum NetError {
> +    #[error("Some octets are missing")]
> +    WrongLength,
> +    #[error("The NET selector must be two characters wide and be 00")]
> +    InvalidNetSelector,
> +    #[error("Invalid AFI (wrong size or position)")]
> +    InvalidAFI,
> +    #[error("Invalid Area (wrong size or position)")]
> +    InvalidArea,
> +    #[error("Invalid SystemId (wrong size or position)")]
> +    InvalidSystemId,
> +}
> +
> +/// Address Family authority Identifier - 49 The AFI value 49 is what IS-IS (and openfabric) uses
> +/// for private addressing.
> +#[derive(
> +    Debug, DeserializeFromStr, SerializeDisplay, Clone, Hash, PartialEq, Eq, PartialOrd, Ord,
> +)]
> +struct NetAFI(String);
> +
> +impl Default for NetAFI {
> +    fn default() -> Self {
> +        Self("49".to_owned())
> +    }
> +}
> +
> +impl Display for NetAFI {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        write!(f, "{}", self.0)
> +    }
> +}
> +
> +impl FromStr for NetAFI {
> +    type Err = NetError;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        if s.len() != 2 {
> +            Err(NetError::InvalidAFI)
> +        } else {
> +            Ok(Self(s.to_owned()))
> +        }
> +    }
> +}
> +
> +/// Area identifier: 0001 IS-IS area number (numerical area 1)
> +/// The second part (system) of the `net` identifier. Every node has to have a different system
> +/// number.
> +#[derive(Debug, DeserializeFromStr, Serialize, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
> +struct NetArea(String);
> +
> +impl Default for NetArea {
> +    fn default() -> Self {
> +        Self("0001".to_owned())
> +    }
> +}
> +
> +impl Display for NetArea {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        write!(f, "{}", self.0)
> +    }
> +}
> +
> +impl FromStr for NetArea {
> +    type Err = NetError;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        if s.len() != 4 {
> +            Err(NetError::InvalidArea)
> +        } else {
> +            Ok(Self(s.to_owned()))
> +        }
> +    }
> +}
> +
> +/// System identifier: 1921.6800.1002 - for system identifiers we recommend to use IP address or
> +/// MAC address of the router itself. The way to construct this is to keep all of the zeroes of the
> +/// router IP address, and then change the periods from being every three numbers to every four
> +/// numbers. The address that is listed here is 192.168.1.2, which if expanded will turn into
> +/// 192.168.001.002. Then all one has to do is move the dots to have four numbers instead of three.
> +/// This gives us 1921.6800.1002.
> +#[derive(Debug, DeserializeFromStr, Serialize, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
> +struct NetSystemId(String);
> +
> +impl Display for NetSystemId {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        write!(f, "{}", self.0)
> +    }
> +}
> +
> +impl FromStr for NetSystemId {
> +    type Err = NetError;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        if s.split(".").count() != 3 || s.split(".").any(|octet| octet.len() != 4) {

would simplify those checks a lot imo. and would also be a more natural
representation since the nomenclature here (octets, but len == 4) is a
bit misleading as well..

> +            Err(NetError::InvalidArea)
> +        } else {
> +            Ok(Self(s.to_owned()))
> +        }
> +    }
> +}
> +
> +/// NET selector: 00 Must always be 00. This setting indicates “this system” or “local system.”
> +#[derive(Debug, DeserializeFromStr, Serialize, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
> +struct NetSelector(String);
> +
> +impl Default for NetSelector {
> +    fn default() -> Self {
> +        Self("00".to_owned())
> +    }
> +}
> +
> +impl Display for NetSelector {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        write!(f, "{}", self.0)
> +    }
> +}
> +
> +impl FromStr for NetSelector {
> +    type Err = NetError;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        if s.len() != 2 {
> +            Err(NetError::InvalidNetSelector)
> +        } else {
> +            Ok(Self(s.to_owned()))
> +        }
> +    }
> +}
> +
> +/// The first part (area) of the `net` identifier. The entire OpenFabric fabric has to have the
> +/// same area.
> +/// f.e.: "49.0001"
> +#[derive(
> +    Debug, DeserializeFromStr, SerializeDisplay, Clone, Hash, PartialEq, Eq, PartialOrd, Ord,
> +)]
> +pub struct Net {
> +    afi: NetAFI,
> +    area: NetArea,
> +    system: NetSystemId,
> +    selector: NetSelector,
> +}
> +
> +impl FromStr for Net {
> +    type Err = NetError;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        if s.split(".").count() != 6 {
> +            return Err(NetError::WrongLength);
> +        }

parsing here could be greatly simplified imo if we convert everything to u8

> +        let mut iter = s.split(".");
> +        let afi = iter.next().ok_or(NetError::WrongLength)?;
> +        let area = iter.next().ok_or(NetError::WrongLength)?;
> +        let system = format!(
> +            "{}.{}.{}",
> +            iter.next().ok_or(NetError::WrongLength)?,
> +            iter.next().ok_or(NetError::WrongLength)?,
> +            iter.next().ok_or(NetError::WrongLength)?
> +        );
> +        let selector = iter.next().ok_or(NetError::WrongLength)?;
> +        Ok(Self {
> +            afi: afi.parse()?,
> +            area: area.parse()?,
> +            system: system.parse()?,
> +            selector: selector.parse()?,
> +        })
> +    }
> +}
> +
> +impl Display for Net {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        write!(
> +            f,
> +            "{}.{}.{}.{}",
> +            self.afi, self.area, self.system, self.selector
> +        )
> +    }
> +}
> +
> +#[cfg(test)]
> +mod tests {
> +    use super::*;
> +
> +    #[test]
> +    fn test_net_from_str() {
> +        let input = "49.0001.1921.6800.1002.00";
> +        let net = input.parse::<Net>().expect("this net should parse");
> +        assert_eq!(net.afi, NetAFI("49".to_owned()));
> +        assert_eq!(net.area, NetArea("0001".to_owned()));
> +        assert_eq!(net.system, NetSystemId("1921.6800.1002".to_owned()));
> +        assert_eq!(net.selector, NetSelector("00".to_owned()));
> +
> +        let input = "45.0200.0100.1001.0010.01";
> +        let net = input.parse::<Net>().expect("this net should parse");
> +        assert_eq!(net.afi, NetAFI("45".to_owned()));
> +        assert_eq!(net.area, NetArea("0200".to_owned()));
> +        assert_eq!(net.system, NetSystemId("0100.1001.0010".to_owned()));
> +        assert_eq!(net.selector, NetSelector("01".to_owned()));
> +    }
> +
> +    #[test]
> +    fn test_net_from_str_failed() {
> +        let input = "49.0001.1921.6800.1002.000";
> +        assert!(matches!(
> +            input.parse::<Net>(),
> +            Err(NetError::InvalidNetSelector)
> +        ));
> +
> +        let input = "49.0001.1921.6800.1002.00.00";
> +        assert!(matches!(input.parse::<Net>(), Err(NetError::WrongLength)));
> +
> +        let input = "49.0001.1921.6800.10002.00";
> +        assert!(matches!(input.parse::<Net>(), Err(NetError::InvalidArea)));
> +
> +        let input = "409.0001.1921.6800.1002.00";
> +        assert!(matches!(input.parse::<Net>(), Err(NetError::InvalidAFI)));
> +
> +        let input = "49.00001.1921.6800.1002.00";
> +        assert!(matches!(input.parse::<Net>(), Err(NetError::InvalidArea)));
> +    }
> +
> +    #[test]
> +    fn test_net_display() {
> +        let net = Net {
> +            afi: NetAFI("49".to_owned()),
> +            area: NetArea("0001".to_owned()),
> +            system: NetSystemId("1921.6800.1002".to_owned()),
> +            selector: NetSelector("00".to_owned()),
> +        };
> +        assert_eq!(format!("{net}"), "49.0001.1921.6800.1002.00");
> +    }
> +}
> +



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