Re: [pve-devel] [PATCH proxmox-ve-rs 10/21] sdn: add ipam module

["Max Carrara" <m.carrara@proxmox.com>]

On Wed Jun 26, 2024 at 2:15 PM CEST, Stefan Hanreich wrote:
> This module includes structs for representing the JSON schema from the
> PVE ipam. Those can be used to parse the current IPAM state.
>
> We also include a general Ipam struct, and provide a method for
> converting the PVE IPAM to the general struct. The idea behind this
> is that we have multiple IPAM plugins in PVE and will likely add
> support for importing them in the future. With the split, we can have
> our dedicated structs for representing the different data formats from
> the different IPAM plugins and then convert them into a common
> representation that can then be used internally, decoupling the
> concrete plugin from the code using the IPAM configuration.

Big fan of this - as I had already mentioned, I find it always nice to
have different types for such things.

IMO it would be neat if those types were logically grouped though, e.g.
the types for PVE could live in a separate `mod` inside the file. Or, if
you want, you can also convert `ipam.rs` to a `ipam/mod.rs` and add
further files depending on the types of different representations there.

Either solution would make it harder for these types to become
"intermingled" in the future, IMO. So this kind of grouping would simply
serve as a "decent barrier" between the separate representations.
Perhaps the module(s) could then also use a little bit of developer
documentation or something that describes how and why the types are
organized that way.

It's probably best to just add `mod`s for now, as those can be split up
into files later anyway. (Just don't `use super::*;` :P )

>
> Enforcing the invariants the way we currently do adds a bit of runtime
> complexity when building the object, but we get the upside of never
> being able to construct an invalid struct. For the amount of entries
> the ipam usually has, this should be fine. Should it turn out to be
> not performant enough we could always add a HashSet for looking up
> values and speeding up the validation. For now, I wanted to avoid the
> additional complexity.
>
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  .../src/firewall/types/address.rs             |   8 +
>  proxmox-ve-config/src/guest/vm.rs             |   4 +
>  proxmox-ve-config/src/sdn/ipam.rs             | 330 ++++++++++++++++++
>  proxmox-ve-config/src/sdn/mod.rs              |   2 +
>  4 files changed, 344 insertions(+)
>  create mode 100644 proxmox-ve-config/src/sdn/ipam.rs
>
> diff --git a/proxmox-ve-config/src/firewall/types/address.rs b/proxmox-ve-config/src/firewall/types/address.rs
> index a0b82c5..3ad1a7a 100644
> --- a/proxmox-ve-config/src/firewall/types/address.rs
> +++ b/proxmox-ve-config/src/firewall/types/address.rs
> @@ -61,6 +61,14 @@ impl Cidr {
>      pub fn is_ipv6(&self) -> bool {
>          matches!(self, Cidr::Ipv6(_))
>      }
> +
> +    pub fn contains_address(&self, ip: &IpAddr) -> bool {
> +        match (self, ip) {
> +            (Cidr::Ipv4(cidr), IpAddr::V4(ip)) => cidr.contains_address(ip),
> +            (Cidr::Ipv6(cidr), IpAddr::V6(ip)) => cidr.contains_address(ip),
> +            _ => false,
> +        }
> +    }
>  }
>  
>  impl fmt::Display for Cidr {
> diff --git a/proxmox-ve-config/src/guest/vm.rs b/proxmox-ve-config/src/guest/vm.rs
> index a7ea9bb..6a706c7 100644
> --- a/proxmox-ve-config/src/guest/vm.rs
> +++ b/proxmox-ve-config/src/guest/vm.rs
> @@ -17,6 +17,10 @@ static LOCAL_PART: [u8; 8] = [0xFE, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00];
>  static EUI64_MIDDLE_PART: [u8; 2] = [0xFF, 0xFE];
>  
>  impl MacAddress {
> +    pub fn new(address: [u8; 6]) -> Self {
> +        Self(address)
> +    }
> +
>      /// generates a link local IPv6-address according to RFC 4291 (Appendix A)
>      pub fn eui64_link_local_address(&self) -> Ipv6Addr {
>          let head = &self.0[..3];
> diff --git a/proxmox-ve-config/src/sdn/ipam.rs b/proxmox-ve-config/src/sdn/ipam.rs
> new file mode 100644
> index 0000000..682bbe7
> --- /dev/null
> +++ b/proxmox-ve-config/src/sdn/ipam.rs
> @@ -0,0 +1,330 @@
> +use std::{
> +    collections::{BTreeMap, HashMap},
> +    error::Error,
> +    fmt::Display,
> +    net::IpAddr,
> +};
> +
> +use serde::Deserialize;
> +
> +use crate::{
> +    firewall::types::Cidr,
> +    guest::{types::Vmid, vm::MacAddress},
> +    sdn::{SdnNameError, SubnetName, ZoneName},
> +};
> +
> +/// struct for deserializing a gateway entry in PVE IPAM
> +///
> +/// They are automatically generated by the PVE SDN module when creating a new subnet.
> +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Hash, PartialOrd, Ord)]
> +pub struct IpamJsonDataGateway {
> +    #[serde(rename = "gateway")]
> +    _gateway: u8,
> +}
> +
> +/// struct for deserializing a guest entry in PVE IPAM
> +///
> +/// They are automatically created when adding a guest to a VNet that has a Subnet with DHCP
> +/// configured.
> +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Hash, PartialOrd, Ord)]
> +pub struct IpamJsonDataVm {
> +    vmid: Vmid,
> +    hostname: Option<String>,
> +    mac: MacAddress,
> +}
> +
> +/// struct for deserializing a custom entry in PVE IPAM
> +///
> +/// Custom entries are created manually by the user via the Web UI / API.
> +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Hash, PartialOrd, Ord)]
> +pub struct IpamJsonDataCustom {
> +    mac: MacAddress,
> +}
> +
> +/// Enum representing the different kinds of entries that can be located in PVE IPAM
> +///
> +/// For more information about the members see the documentation of the respective structs in the
> +/// enum.
> +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Hash, PartialOrd, Ord)]
> +#[serde(untagged)]
> +pub enum IpamJsonData {
> +    Vm(IpamJsonDataVm),
> +    Gateway(IpamJsonDataGateway),
> +    Custom(IpamJsonDataCustom),
> +}
> +
> +/// struct for deserializing IPs from the PVE IPAM
> +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Hash, PartialOrd, Ord, Default)]
> +pub struct IpJson {
> +    ips: BTreeMap<IpAddr, IpamJsonData>,
> +}
> +
> +/// struct for deserializing subnets from the PVE IPAM
> +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Hash, PartialOrd, Ord, Default)]
> +pub struct SubnetJson {
> +    subnets: BTreeMap<Cidr, IpJson>,
> +}
> +
> +/// struct for deserializing the PVE IPAM
> +///
> +/// It is usually located in `/etc/pve/priv/ipam.db`
> +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Hash, PartialOrd, Ord, Default)]
> +pub struct IpamJson {
> +    zones: BTreeMap<ZoneName, SubnetJson>,
> +}
> +
> +/// holds the data for the IPAM entry of a VM
> +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Hash, PartialOrd, Ord)]
> +pub struct IpamDataVm {
> +    ip: IpAddr,
> +    vmid: Vmid,
> +    mac: MacAddress,
> +    hostname: Option<String>,
> +}
> +
> +impl IpamDataVm {
> +    pub fn new(
> +        ip: impl Into<IpAddr>,
> +        vmid: impl Into<Vmid>,
> +        mac: MacAddress,
> +        hostname: impl Into<Option<String>>,
> +    ) -> Self {
> +        Self {
> +            ip: ip.into(),
> +            vmid: vmid.into(),
> +            mac,
> +            hostname: hostname.into(),
> +        }
> +    }
> +
> +    pub fn from_json_data(ip: IpAddr, data: IpamJsonDataVm) -> Self {
> +        Self::new(ip, data.vmid, data.mac, data.hostname)
> +    }
> +
> +    pub fn ip(&self) -> &IpAddr {
> +        &self.ip
> +    }
> +
> +    pub fn vmid(&self) -> Vmid {
> +        self.vmid
> +    }
> +
> +    pub fn mac(&self) -> &MacAddress {
> +        &self.mac
> +    }
> +
> +    pub fn hostname(&self) -> Option<&str> {
> +        self.hostname.as_deref()
> +    }
> +}
> +
> +/// holds the data for the IPAM entry of a Gateway
> +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Hash, PartialOrd, Ord)]
> +pub struct IpamDataGateway {
> +    ip: IpAddr,
> +}
> +
> +impl IpamDataGateway {
> +    pub fn new(ip: IpAddr) -> Self {
> +        Self { ip }
> +    }
> +
> +    fn from_json_data(ip: IpAddr, _json_data: IpamJsonDataGateway) -> Self {
> +        Self::new(ip)
> +    }
> +
> +    pub fn ip(&self) -> &IpAddr {
> +        &self.ip
> +    }
> +}
> +
> +/// holds the data for a custom IPAM entry
> +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Hash, PartialOrd, Ord)]
> +pub struct IpamDataCustom {
> +    ip: IpAddr,
> +    mac: MacAddress,
> +}
> +
> +impl IpamDataCustom {
> +    pub fn new(ip: IpAddr, mac: MacAddress) -> Self {
> +        Self { ip, mac }
> +    }
> +
> +    fn from_json_data(ip: IpAddr, json_data: IpamJsonDataCustom) -> Self {
> +        Self::new(ip, json_data.mac)
> +    }
> +
> +    pub fn ip(&self) -> &IpAddr {
> +        &self.ip
> +    }
> +
> +    pub fn mac(&self) -> &MacAddress {
> +        &self.mac
> +    }
> +}
> +
> +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Hash, PartialOrd, Ord)]
> +pub enum IpamData {
> +    Vm(IpamDataVm),
> +    Gateway(IpamDataGateway),
> +    Custom(IpamDataCustom),
> +}
> +
> +impl IpamData {
> +    pub fn from_json_data(ip: IpAddr, json_data: IpamJsonData) -> Self {
> +        match json_data {
> +            IpamJsonData::Vm(json_data) => IpamDataVm::from_json_data(ip, json_data).into(),
> +            IpamJsonData::Gateway(json_data) => {
> +                IpamDataGateway::from_json_data(ip, json_data).into()
> +            }
> +            IpamJsonData::Custom(json_data) => IpamDataCustom::from_json_data(ip, json_data).into(),
> +        }
> +    }
> +
> +    pub fn ip_address(&self) -> &IpAddr {
> +        match &self {
> +            IpamData::Vm(data) => data.ip(),
> +            IpamData::Gateway(data) => data.ip(),
> +            IpamData::Custom(data) => data.ip(),
> +        }
> +    }
> +}
> +
> +impl From<IpamDataVm> for IpamData {
> +    fn from(value: IpamDataVm) -> Self {
> +        IpamData::Vm(value)
> +    }
> +}
> +
> +impl From<IpamDataGateway> for IpamData {
> +    fn from(value: IpamDataGateway) -> Self {
> +        IpamData::Gateway(value)
> +    }
> +}
> +
> +impl From<IpamDataCustom> for IpamData {
> +    fn from(value: IpamDataCustom) -> Self {
> +        IpamData::Custom(value)
> +    }
> +}
> +
> +#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
> +pub enum IpamError {
> +    NameError(SdnNameError),
> +    InvalidIpAddress,
> +    DuplicateIpAddress,
> +    IpAddressOutOfBounds,
> +}
> +
> +impl Error for IpamError {}
> +
> +impl Display for IpamError {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        f.write_str("")
> +    }
> +}
> +
> +/// represents an entry in the PVE IPAM database
> +#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
> +pub struct IpamEntry {
> +    subnet: SubnetName,
> +    data: IpamData,
> +}
> +
> +impl IpamEntry {
> +    /// creates a new PVE IPAM entry
> +    ///
> +    /// # Errors
> +    ///
> +    /// This function will return an error if the IP address of the entry does not match the CIDR
> +    /// of the subnet.
> +    pub fn new(subnet: SubnetName, data: IpamData) -> Result<Self, IpamError> {
> +        if !subnet.cidr().contains_address(data.ip_address()) {
> +            return Err(IpamError::IpAddressOutOfBounds);
> +        }
> +
> +        Ok(IpamEntry { subnet, data })
> +    }
> +
> +    pub fn subnet(&self) -> &SubnetName {
> +        &self.subnet
> +    }
> +
> +    pub fn data(&self) -> &IpamData {
> +        &self.data
> +    }
> +
> +    pub fn ip_address(&self) -> &IpAddr {
> +        self.data.ip_address()
> +    }
> +}
> +
> +/// Common representation of IPAM data used in SDN
> +///
> +/// this should be instantiated by reading from one of the concrete IPAM implementations and then
> +/// converting into this common struct.
> +///
> +/// # Invariants
> +/// * No IP address in a Subnet is allocated twice
> +#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Default)]
> +pub struct Ipam {
> +    entries: BTreeMap<SubnetName, Vec<IpamEntry>>,
> +}
> +
> +impl Ipam {
> +    pub fn new() -> Self {
> +        Self::default()
> +    }
> +
> +    pub fn from_entries(entries: impl IntoIterator<Item = IpamEntry>) -> Result<Self, IpamError> {
> +        let mut ipam = Self::new();
> +
> +        for entry in entries {
> +            ipam.add_entry(entry)?;
> +        }
> +
> +        Ok(ipam)
> +    }
> +
> +    /// adds a new [`IpamEntry`] to the database
> +    ///
> +    /// # Errors
> +    ///
> +    /// This function will return an error if the IP is already allocated by another guest.
> +    pub fn add_entry(&mut self, entry: IpamEntry) -> Result<(), IpamError> {
> +        if let Some(entries) = self.entries.get_mut(entry.subnet()) {
> +            for ipam_entry in &*entries {
> +                if ipam_entry.ip_address() == entry.ip_address() {
> +                    return Err(IpamError::DuplicateIpAddress);
> +                }
> +            }
> +
> +            entries.push(entry);
> +        } else {
> +            self.entries
> +                .insert(entry.subnet().clone(), [entry].to_vec());
> +        }
> +
> +        Ok(())
> +    }
> +}
> +
> +impl TryFrom<IpamJson> for Ipam {
> +    type Error = IpamError;
> +
> +    fn try_from(value: IpamJson) -> Result<Self, Self::Error> {
> +        let mut ipam = Ipam::default();
> +
> +        for (zone_name, subnet_json) in value.zones {
> +            for (cidr, ip_json) in subnet_json.subnets {
> +                for (ip, json_data) in ip_json.ips {
> +                    let data = IpamData::from_json_data(ip, json_data);
> +                    let subnet = SubnetName::new(zone_name.clone(), cidr);
> +                    ipam.add_entry(IpamEntry::new(subnet, data)?)?;
> +                }
> +            }
> +        }
> +
> +        Ok(ipam)
> +    }
> +}
> diff --git a/proxmox-ve-config/src/sdn/mod.rs b/proxmox-ve-config/src/sdn/mod.rs
> index 4e7c525..67af24e 100644
> --- a/proxmox-ve-config/src/sdn/mod.rs
> +++ b/proxmox-ve-config/src/sdn/mod.rs
> @@ -1,3 +1,5 @@
> +pub mod ipam;
> +
>  use std::{error::Error, fmt::Display, str::FromStr};
>  
>  use serde_with::DeserializeFromStr;



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