Re: [pve-devel] [PATCH proxmox-ve-rs v2 05/25] firewall: add ip range types

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

On Thu, Oct 10, 2024 at 05:56:17PM GMT, Stefan Hanreich wrote:
> Currently we are using tuples to represent IP ranges which is
> suboptimal. Validation logic and invariant checking needs to happen at
> every site using the IP range rather than having a unified struct for
> enforcing those invariants.
> 
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  .../src/firewall/types/address.rs             | 230 +++++++++++++++++-
>  1 file changed, 228 insertions(+), 2 deletions(-)
> 
> diff --git a/proxmox-ve-config/src/firewall/types/address.rs b/proxmox-ve-config/src/firewall/types/address.rs
> index e48ac1b..42ec1a1 100644
> --- a/proxmox-ve-config/src/firewall/types/address.rs
> +++ b/proxmox-ve-config/src/firewall/types/address.rs
> @@ -1,9 +1,9 @@
> -use std::fmt;
> +use std::fmt::{self, Display};
>  use std::net::{IpAddr, Ipv4Addr, Ipv6Addr};
>  use std::ops::Deref;
>  
>  use anyhow::{bail, format_err, Error};
> -use serde_with::DeserializeFromStr;
> +use serde_with::{DeserializeFromStr, SerializeDisplay};
>  
>  #[derive(Clone, Copy, Debug, Eq, PartialEq)]
>  pub enum Family {
> @@ -239,6 +239,202 @@ impl<T: Into<Ipv6Addr>> From<T> for Ipv6Cidr {
>      }
>  }
>  
> +#[derive(Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)]
> +pub enum IpRangeError {
> +    MismatchedFamilies,
> +    StartGreaterThanEnd,
> +    InvalidFormat,
> +}
> +
> +impl std::error::Error for IpRangeError {}
> +
> +impl Display for IpRangeError {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        f.write_str(match self {
> +            IpRangeError::MismatchedFamilies => "mismatched ip address families",
> +            IpRangeError::StartGreaterThanEnd => "start is greater than end",
> +            IpRangeError::InvalidFormat => "invalid ip range format",
> +        })
> +    }
> +}
> +
> +/// Represents a range of IPv4 or IPv6 addresses
> +///
> +/// For more information see [`AddressRange`]
> +#[derive(Clone, Copy, Debug, PartialEq, Eq, SerializeDisplay, DeserializeFromStr)]
> +pub enum IpRange {
> +    V4(AddressRange<Ipv4Addr>),
> +    V6(AddressRange<Ipv6Addr>),
> +}
> +
> +impl IpRange {
> +    /// returns the family of the IpRange
> +    pub fn family(&self) -> Family {
> +        match self {
> +            IpRange::V4(_) => Family::V4,
> +            IpRange::V6(_) => Family::V6,
> +        }
> +    }
> +
> +    /// creates a new [`IpRange`] from two [`IpAddr`]
> +    ///
> +    /// # Errors
> +    ///
> +    /// This function will return an error if start and end IP address are not from the same family.
> +    pub fn new(start: impl Into<IpAddr>, end: impl Into<IpAddr>) -> Result<Self, IpRangeError> {
> +        match (start.into(), end.into()) {
> +            (IpAddr::V4(start), IpAddr::V4(end)) => Self::new_v4(start, end),
> +            (IpAddr::V6(start), IpAddr::V6(end)) => Self::new_v6(start, end),
> +            _ => Err(IpRangeError::MismatchedFamilies),
> +        }
> +    }
> +
> +    /// construct a new Ipv4 Range
> +    pub fn new_v4(
> +        start: impl Into<Ipv4Addr>,
> +        end: impl Into<Ipv4Addr>,
> +    ) -> Result<Self, IpRangeError> {
> +        Ok(IpRange::V4(AddressRange::new_v4(start, end)?))
> +    }
> +
> +    pub fn new_v6(
> +        start: impl Into<Ipv6Addr>,
> +        end: impl Into<Ipv6Addr>,
> +    ) -> Result<Self, IpRangeError> {
> +        Ok(IpRange::V6(AddressRange::new_v6(start, end)?))
> +    }
> +}
> +
> +impl std::str::FromStr for IpRange {
> +    type Err = IpRangeError;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        if let Ok(range) = s.parse() {
> +            return Ok(IpRange::V4(range));
> +        }
> +
> +        if let Ok(range) = s.parse() {
> +            return Ok(IpRange::V6(range));
> +        }
> +
> +        Err(IpRangeError::InvalidFormat)
> +    }
> +}
> +
> +impl fmt::Display for IpRange {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        match self {
> +            IpRange::V4(range) => range.fmt(f),
> +            IpRange::V6(range) => range.fmt(f),
> +        }
> +    }
> +}
> +
> +/// Represents a range of IP addresses from start to end
> +///
> +/// This type is for encapsulation purposes for the [`IpRange`] enum and should be instantiated via
> +/// that enum.
> +///
> +/// # Invariants
> +///
> +/// * start and end have the same IP address family
> +/// * start is lesser than or equal to end

lesser -> less

Also:

This range *includes* the `end`. In rust `std` we have `std::ops::Range`
while *this* works like `std::ops::RangeInclusive`.

This might be fine, given it's a vastly different context, however...

> +///
> +/// # Textual representation
> +///
> +/// Two IP addresses separated by a hyphen, e.g.: `127.0.0.1-127.0.0.255`
> +#[derive(Clone, Copy, Debug, PartialEq, Eq)]
> +pub struct AddressRange<T> {
> +    start: T,
> +    end: T,

...I think we should name this `last`, so it's less confusing and...

> +}
> +
> +impl AddressRange<Ipv4Addr> {
> +    pub(crate) fn new_v4(
> +        start: impl Into<Ipv4Addr>,
> +        end: impl Into<Ipv4Addr>,
> +    ) -> Result<AddressRange<Ipv4Addr>, IpRangeError> {
> +        let (start, end) = (start.into(), end.into());
> +
> +        if start > end {
> +            return Err(IpRangeError::StartGreaterThanEnd);
> +        }
> +
> +        Ok(Self { start, end })
> +    }
> +}
> +
> +impl AddressRange<Ipv6Addr> {
> +    pub(crate) fn new_v6(
> +        start: impl Into<Ipv6Addr>,
> +        end: impl Into<Ipv6Addr>,
> +    ) -> Result<AddressRange<Ipv6Addr>, IpRangeError> {
> +        let (start, end) = (start.into(), end.into());
> +
> +        if start > end {
> +            return Err(IpRangeError::StartGreaterThanEnd);
> +        }
> +
> +        Ok(Self { start, end })
> +    }
> +}
> +
> +impl<T> AddressRange<T> {
> +    pub fn start(&self) -> &T {
> +        &self.start
> +    }
> +
> +    pub fn end(&self) -> &T {

... similarly these getters should be named `last`. Mainly because with
the ranges being inclusive, this represents the "*last* usable address",
while "end" is also used in `std::ops::Range` to mean "fist *unusable*
number".

> +        &self.end
> +    }
> +}
> +
> +impl std::str::FromStr for AddressRange<Ipv4Addr> {
> +    type Err = IpRangeError;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        if let Some((start, end)) = s.split_once('-') {
> +            let start_address = start
> +                .parse::<Ipv4Addr>()
> +                .map_err(|_| IpRangeError::InvalidFormat)?;
> +
> +            let end_address = end
> +                .parse::<Ipv4Addr>()
> +                .map_err(|_| IpRangeError::InvalidFormat)?;
> +
> +            return Self::new_v4(start_address, end_address);
> +        }
> +
> +        Err(IpRangeError::InvalidFormat)
> +    }
> +}
> +
> +impl std::str::FromStr for AddressRange<Ipv6Addr> {
> +    type Err = IpRangeError;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        if let Some((start, end)) = s.split_once('-') {
> +            let start_address = start
> +                .parse::<Ipv6Addr>()
> +                .map_err(|_| IpRangeError::InvalidFormat)?;
> +
> +            let end_address = end
> +                .parse::<Ipv6Addr>()
> +                .map_err(|_| IpRangeError::InvalidFormat)?;
> +
> +            return Self::new_v6(start_address, end_address);
> +        }
> +
> +        Err(IpRangeError::InvalidFormat)
> +    }
> +}
> +
> +impl<T: fmt::Display> fmt::Display for AddressRange<T> {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        write!(f, "{}-{}", self.start, self.end)
> +    }
> +}
> +
>  #[derive(Clone, Debug)]
>  #[cfg_attr(test, derive(Eq, PartialEq))]
>  pub enum IpEntry {
> @@ -612,4 +808,34 @@ mod tests {
>          ])
>          .expect_err("cannot mix ip families in ip list");
>      }
> +
> +    #[test]
> +    fn test_ip_range() {
> +        IpRange::new([10, 0, 0, 2], [10, 0, 0, 1]).unwrap_err();
> +
> +        IpRange::new(
> +            [0x2001, 0x0db8, 0, 0, 0, 0, 0, 0x1000],
> +            [0x2001, 0x0db8, 0, 0, 0, 0, 0, 0],
> +        )
> +        .unwrap_err();
> +
> +        let v4_range = IpRange::new([10, 0, 0, 0], [10, 0, 0, 100]).unwrap();
> +        assert_eq!(v4_range.family(), Family::V4);
> +
> +        let v6_range = IpRange::new(
> +            [0x2001, 0x0db8, 0, 0, 0, 0, 0, 0],
> +            [0x2001, 0x0db8, 0, 0, 0, 0, 0, 0x1000],
> +        )
> +        .unwrap();
> +        assert_eq!(v6_range.family(), Family::V6);
> +
> +        "10.0.0.1-10.0.0.100".parse::<IpRange>().unwrap();
> +        "2001:db8::1-2001:db8::f".parse::<IpRange>().unwrap();
> +
> +        "10.0.0.1-2001:db8::1000".parse::<IpRange>().unwrap_err();
> +        "2001:db8::1-192.168.0.2".parse::<IpRange>().unwrap_err();
> +
> +        "10.0.0.1-10.0.0.0".parse::<IpRange>().unwrap_err();
> +        "2001:db8::1-2001:db8::0".parse::<IpRange>().unwrap_err();
> +    }
>  }
> -- 
> 2.39.5


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