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
next prev parent 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