[pdm-devel] [PATCH proxmox v2 02/14] network-types: move `Fqdn` type from proxmox-installer-common
Lukas Wagner
l.wagner at proxmox.com
Tue Dec 9 10:13:10 CET 2025
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 at 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 at 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 at 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 at foo.com"),
> + Err(InvalidPart("123 at 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;
More information about the pdm-devel
mailing list