Re: [pdm-devel] [PATCH proxmox v2 02/14] network-types: move `Fqdn` type from proxmox-installer-common

["Lukas Wagner" <l.wagner@proxmox.com>]

Looks good in general, I assume this was reviewed before this it was
moved from somewhere else.

One note inline.

Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>

On Fri Dec 5, 2025 at 12:25 PM CET, Christoph Heiss wrote:
> This introduces an `Fqdn` type for safely representing (valid) FQDNs on
> Debian, following all relevant RFCs as well as restrictions given by
> Debian.
>
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
> Changes v1 -> v2:
>   * no changes
>
>  proxmox-network-types/Cargo.toml     |   3 +-
>  proxmox-network-types/debian/control |   2 +
>  proxmox-network-types/src/fqdn.rs    | 248 +++++++++++++++++++++++++++
>  proxmox-network-types/src/lib.rs     |   1 +
>  4 files changed, 253 insertions(+), 1 deletion(-)
>  create mode 100644 proxmox-network-types/src/fqdn.rs
>
> diff --git a/proxmox-network-types/Cargo.toml b/proxmox-network-types/Cargo.toml
> index 6333a37f..25c4bcf2 100644
> --- a/proxmox-network-types/Cargo.toml
> +++ b/proxmox-network-types/Cargo.toml
> @@ -10,9 +10,10 @@ exclude.workspace = true
>  rust-version.workspace = true
>  
>  [dependencies]
> -regex = { workspace = true, optional = true}
> +regex = { workspace = true, optional = true }
>  serde = { workspace = true, features = [ "derive", "std" ] }
>  serde_with = "3.8.1"
> +serde_plain.workspace = true
>  thiserror.workspace = true
>  
>  proxmox-schema = { workspace = true, features = [ "api-macro", "api-types" ], optional = true}
> diff --git a/proxmox-network-types/debian/control b/proxmox-network-types/debian/control
> index 8b68deb1..08df0f9f 100644
> --- a/proxmox-network-types/debian/control
> +++ b/proxmox-network-types/debian/control
> @@ -9,6 +9,7 @@ Build-Depends-Arch: cargo:native <!nocheck>,
>   librust-serde-1+default-dev <!nocheck>,
>   librust-serde-1+derive-dev <!nocheck>,
>   librust-serde-1+std-dev <!nocheck>,
> + librust-serde-plain-1+default-dev <!nocheck>,
>   librust-serde-with-3+default-dev (>= 3.8.1-~~) <!nocheck>,
>   librust-thiserror-2+default-dev <!nocheck>
>  Maintainer: Proxmox Support Team <support@proxmox.com>
> @@ -26,6 +27,7 @@ Depends:
>   librust-serde-1+default-dev,
>   librust-serde-1+derive-dev,
>   librust-serde-1+std-dev,
> + librust-serde-plain-1+default-dev,
>   librust-serde-with-3+default-dev (>= 3.8.1-~~),
>   librust-thiserror-2+default-dev
>  Suggests:
> diff --git a/proxmox-network-types/src/fqdn.rs b/proxmox-network-types/src/fqdn.rs
> new file mode 100644
> index 00000000..9582639d
> --- /dev/null
> +++ b/proxmox-network-types/src/fqdn.rs
> @@ -0,0 +1,248 @@
> +//! A type for safely representing fully-qualified domain names (FQDNs).
> +
> +use std::{fmt, str::FromStr};
> +
> +use serde::Deserialize;
> +
> +/// Possible errors that might occur when parsing FQDNs.
> +#[derive(Debug, Eq, PartialEq)]
> +pub enum FqdnParseError {
> +    MissingHostname,
> +    NumericHostname,
> +    InvalidPart(String),
> +    TooLong(usize),
> +}
> +
> +impl fmt::Display for FqdnParseError {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        use FqdnParseError::*;
> +        match self {
> +            MissingHostname => write!(f, "missing hostname part"),
> +            NumericHostname => write!(f, "hostname cannot be purely numeric"),
> +            InvalidPart(part) => write!(
> +                f,
> +                "FQDN must only consist of alphanumeric characters and dashes. Invalid part: '{part}'",
> +            ),
> +            TooLong(len) => write!(f, "FQDN too long: {len} > {}", Fqdn::MAX_LENGTH),
> +        }
> +    }
> +}
> +
> +/// A type for safely representing fully-qualified domain names (FQDNs).
> +///
> +/// It considers following RFCs:
> +/// - [RFC952] (sec. "ASSUMPTIONS", 1.)
> +/// - [RFC1035] (sec. 2.3. "Conventions")
> +/// - [RFC1123] (sec. 2.1. "Host Names and Numbers")
> +/// - [RFC3492]
> +/// - [RFC4343]
> +///
> +/// .. and applies some restriction given by Debian, e.g. 253 instead of 255
> +/// maximum total length and maximum 63 characters per label, per the
> +/// [hostname(7)].
> +///
> +/// Additionally:
> +/// - It enforces the restriction as per Bugzilla #1054, in that
> +///   purely numeric hostnames are not allowed - against RFC1123 sec. 2.1.
> +///
> +/// Some terminology:
> +/// - "label" - a single part of a FQDN, e.g. {label}.{label}.{tld}
> +///
> +/// [RFC952]: <https://www.ietf.org/rfc/rfc952.txt>
> +/// [RFC1035]: <https://www.ietf.org/rfc/rfc1035.txt>
> +/// [RFC1123]: <https://www.ietf.org/rfc/rfc1123.txt>
> +/// [RFC3492]: <https://www.ietf.org/rfc/rfc3492.txt>
> +/// [RFC4343]: <https://www.ietf.org/rfc/rfc4343.txt>
> +/// [hostname(7)]: <https://manpages.debian.org/stable/manpages/hostname.7.en.html>
> +#[derive(Clone, Debug, Eq)]
> +pub struct Fqdn {
> +    parts: Vec<String>,
> +}
> +
> +impl Fqdn {
> +    /// Maximum length of a single label of the FQDN
> +    const MAX_LABEL_LENGTH: usize = 63;
> +    /// Maximum total length of the FQDN
> +    const MAX_LENGTH: usize = 253;
> +
> +    pub fn from(fqdn: &str) -> Result<Self, FqdnParseError> {
> +        if fqdn.len() > Self::MAX_LENGTH {
> +            return Err(FqdnParseError::TooLong(fqdn.len()));
> +        }
> +
> +        let parts = fqdn
> +            .split('.')
> +            .map(ToOwned::to_owned)
> +            .collect::<Vec<String>>();
> +
> +        for part in &parts {
> +            if !Self::validate_single(part) {
> +                return Err(FqdnParseError::InvalidPart(part.clone()));
> +            }
> +        }
> +
> +        if parts.len() < 2 {
> +            Err(FqdnParseError::MissingHostname)

(a)

Since `Fqdn::from` seems to be the only way to instantiate the Fqdn
type, and since the parts.len() < 2 establishes the invariant that the
number of parts can only ever be >= 2,

> +        } else if parts[0].chars().all(|c| c.is_ascii_digit()) {
> +            // Do not allow a purely numeric hostname, see:
> +            // https://bugzilla.proxmox.com/show_bug.cgi?id=1054
> +            Err(FqdnParseError::NumericHostname)
> +        } else {
> +            Ok(Self { parts })
> +        }
> +    }
> +
> +    pub fn host(&self) -> Option<&str> {
> +        self.has_host().then_some(&self.parts[0])
> +    }

(c)

... therefore you could assume that it is safe to just access
self.parts[0] and avoid the `Option<..>` in the return type?

I'm always on board with being careful, but since this type is
relatively small, I think it would be reasonable to rely on the
invariant, which makes the call sites a bit nicer since it does not have
to handle the Option.

What do you think? 

> +
> +    pub fn domain(&self) -> String {
> +        let parts = if self.has_host() {
> +            &self.parts[1..]
> +        } else {
> +            &self.parts
> +        };
> +
> +        parts.join(".")
> +    }
> +
> +    /// Checks whether the FQDN has a hostname associated with it, i.e. is has more than 1 part.
> +    fn has_host(&self) -> bool {
> +        self.parts.len() > 1
> +    }

(b)

... which means that this should always return true, ...

> +
> +    fn validate_single(s: &str) -> bool {
> +        !s.is_empty()
> +            && s.len() <= Self::MAX_LABEL_LENGTH
> +            // First character must be alphanumeric
> +            && s.chars()
> +                .next()
> +                .map(|c| c.is_ascii_alphanumeric())
> +                .unwrap_or_default()
> +            // .. last character as well,
> +            && s.chars()
> +                .last()
> +                .map(|c| c.is_ascii_alphanumeric())
> +                .unwrap_or_default()
> +            // and anything between must be alphanumeric or -
> +            && s.chars()
> +                .skip(1)
> +                .take(s.len().saturating_sub(2))
> +                .all(|c| c.is_ascii_alphanumeric() || c == '-')
> +    }
> +}
> +
> +impl FromStr for Fqdn {
> +    type Err = FqdnParseError;
> +
> +    fn from_str(value: &str) -> Result<Self, Self::Err> {
> +        Self::from(value)
> +    }
> +}
> +
> +impl fmt::Display for Fqdn {
> +    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> +        write!(f, "{}", self.parts.join("."))
> +    }
> +}
> +
> +serde_plain::derive_serialize_from_display!(Fqdn);
> +
> +impl<'de> Deserialize<'de> for Fqdn {
> +    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
> +    where
> +        D: serde::Deserializer<'de>,
> +    {
> +        let s: String = Deserialize::deserialize(deserializer)?;
> +        s.parse()
> +            .map_err(|_| serde::de::Error::custom("invalid FQDN"))
> +    }
> +}
> +
> +impl PartialEq for Fqdn {
> +    // Case-insensitive comparison, as per RFC 952 "ASSUMPTIONS", RFC 1035 sec. 2.3.3. "Character
> +    // Case" and RFC 4343 as a whole
> +    fn eq(&self, other: &Self) -> bool {
> +        if self.parts.len() != other.parts.len() {
> +            return false;
> +        }
> +
> +        self.parts
> +            .iter()
> +            .zip(other.parts.iter())
> +            .all(|(a, b)| a.to_lowercase() == b.to_lowercase())
> +    }
> +}
> +
> +#[cfg(test)]
> +mod tests {
> +    use super::*;
> +
> +    #[test]
> +    fn fqdn_construct() {
> +        use FqdnParseError::*;
> +        assert!(Fqdn::from("foo.example.com").is_ok());
> +        assert!(Fqdn::from("foo-bar.com").is_ok());
> +        assert!(Fqdn::from("a-b.com").is_ok());
> +
> +        assert_eq!(Fqdn::from("foo"), Err(MissingHostname));
> +
> +        assert_eq!(Fqdn::from("-foo.com"), Err(InvalidPart("-foo".to_owned())));
> +        assert_eq!(Fqdn::from("foo-.com"), Err(InvalidPart("foo-".to_owned())));
> +        assert_eq!(Fqdn::from("foo.com-"), Err(InvalidPart("com-".to_owned())));
> +        assert_eq!(Fqdn::from("-o-.com"), Err(InvalidPart("-o-".to_owned())));
> +
> +        // https://bugzilla.proxmox.com/show_bug.cgi?id=1054
> +        assert_eq!(Fqdn::from("123.com"), Err(NumericHostname));
> +        assert!(Fqdn::from("foo123.com").is_ok());
> +        assert!(Fqdn::from("123foo.com").is_ok());
> +
> +        assert!(Fqdn::from(&format!("{}.com", "a".repeat(63))).is_ok());
> +        assert_eq!(
> +            Fqdn::from(&format!("{}.com", "a".repeat(250))),
> +            Err(TooLong(254)),
> +        );
> +        assert_eq!(
> +            Fqdn::from(&format!("{}.com", "a".repeat(64))),
> +            Err(InvalidPart("a".repeat(64))),
> +        );
> +
> +        // https://bugzilla.proxmox.com/show_bug.cgi?id=5230
> +        assert_eq!(
> +            Fqdn::from("123@foo.com"),
> +            Err(InvalidPart("123@foo".to_owned()))
> +        );
> +    }
> +
> +    #[test]
> +    fn fqdn_parts() {
> +        let fqdn = Fqdn::from("pve.example.com").unwrap();
> +        assert_eq!(fqdn.host().unwrap(), "pve");
> +        assert_eq!(fqdn.domain(), "example.com");
> +        assert_eq!(
> +            fqdn.parts,
> +            &["pve".to_owned(), "example".to_owned(), "com".to_owned()]
> +        );
> +    }
> +
> +    #[test]
> +    fn fqdn_display() {
> +        assert_eq!(
> +            Fqdn::from("foo.example.com").unwrap().to_string(),
> +            "foo.example.com"
> +        );
> +    }
> +
> +    #[test]
> +    fn fqdn_compare() {
> +        assert_eq!(Fqdn::from("example.com"), Fqdn::from("example.com"));
> +        assert_eq!(Fqdn::from("example.com"), Fqdn::from("ExAmPle.Com"));
> +        assert_eq!(Fqdn::from("ExAmPle.Com"), Fqdn::from("example.com"));
> +        assert_ne!(
> +            Fqdn::from("subdomain.ExAmPle.Com"),
> +            Fqdn::from("example.com")
> +        );
> +        assert_ne!(Fqdn::from("foo.com"), Fqdn::from("bar.com"));
> +        assert_ne!(Fqdn::from("example.com"), Fqdn::from("example.net"));
> +    }
> +}
> diff --git a/proxmox-network-types/src/lib.rs b/proxmox-network-types/src/lib.rs
> index ee26b1c1..e5d31285 100644
> --- a/proxmox-network-types/src/lib.rs
> +++ b/proxmox-network-types/src/lib.rs
> @@ -1,5 +1,6 @@
>  #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))]
>  #![deny(unsafe_op_in_unsafe_fn)]
>  
> +pub mod fqdn;
>  pub mod ip_address;
>  pub mod mac_address;



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