public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Stefan Hanreich <s.hanreich@proxmox.com>
Cc: pve-devel@lists.proxmox.com
Subject: Re: [pve-devel] [PATCH proxmox-ve-rs v2 05/25] firewall: add ip range types
Date: Wed, 6 Nov 2024 14:13:03 +0100	[thread overview]
Message-ID: <dtlnqct7bsvavwdgacabqobbzgzg4n47fcdk7y2lgux3jwnr3q@kiy5dmwntmym> (raw)
In-Reply-To: <20241010155637.255451-6-s.hanreich@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


  reply	other threads:[~2024-11-06 13:13 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-10 15:56 [pve-devel] [PATCH docs/firewall/manager/proxmox{-ve-rs, -firewall, -perl-rs} v2 00/25] autogenerate ipsets for sdn objects Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 01/25] debian: add files for packaging Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 02/25] bump serde_with to 3 Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 03/25] bump dependencies Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 04/25] firewall: add sdn scope for ipsets Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 05/25] firewall: add ip range types Stefan Hanreich
2024-11-06 13:13   ` Wolfgang Bumiller [this message]
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 06/25] firewall: address: use new iprange type for ip entries Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 07/25] ipset: add range variant to addresses Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 08/25] iprange: add methods for converting an ip range to cidrs Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 09/25] ipset: address: add helper methods Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 10/25] firewall: guest: derive traits according to rust api guidelines Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 11/25] common: add allowlist Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 12/25] sdn: add name types Stefan Hanreich
2024-11-06 14:18   ` Wolfgang Bumiller
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 13/25] sdn: add ipam module Stefan Hanreich
2024-11-06 14:52   ` Wolfgang Bumiller
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 14/25] sdn: ipam: add method for generating ipsets Stefan Hanreich
2024-11-06 15:12   ` Wolfgang Bumiller
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 15/25] sdn: add config module Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 16/25] sdn: config: add method for generating ipsets Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 17/25] tests: add sdn config tests Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-ve-rs v2 18/25] tests: add ipam tests Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-firewall v2 19/25] config: tests: add support for loading sdn and ipam config Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-firewall v2 20/25] ipsets: autogenerate ipsets for vnets and ipam Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH pve-firewall v2 21/25] add support for loading sdn firewall configuration Stefan Hanreich
2024-11-07 10:44   ` Wolfgang Bumiller
2024-10-10 15:56 ` [pve-devel] [PATCH pve-firewall v2 22/25] api: load sdn ipsets Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH proxmox-perl-rs v2 23/25] add PVE::RS::Firewall::SDN module Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH pve-manager v2 24/25] firewall: add sdn scope to IPRefSelector Stefan Hanreich
2024-10-10 15:56 ` [pve-devel] [PATCH pve-docs v2 25/25] sdn: add documentation for firewall integration Stefan Hanreich
2024-11-12 12:34 ` [pve-devel] [PATCH docs/firewall/manager/proxmox{-ve-rs, -firewall, -perl-rs} v2 00/25] autogenerate ipsets for sdn objects Stefan Hanreich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=dtlnqct7bsvavwdgacabqobbzgzg4n47fcdk7y2lgux3jwnr3q@kiy5dmwntmym \
    --to=w.bumiller@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    --cc=s.hanreich@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal