[pve-devel] [PATCH installer 0/5] use hostname from DHCP lease if available

[pve-devel] [PATCH installer 0/5] use hostname from DHCP lease if available

[Christoph Heiss]

DHCP servers can set option 12 ("host-name") for client leases [0],
telling them about their hostname. It's very much non-invasive and falls
back to the default values as done currently.

This came up while talking to Aaron, which he noticed (esp. during
trainings) that this would be a very useful feature too have.

I have tested this with the "host-name" entry set and unset, as well as
any combinations of that with the domain name being set or unset.

[0] https://datatracker.ietf.org/doc/html/rfc2132#section-3.14

Christoph Heiss (5):
  net: move hostname/fqdn regexes into common code
  run env: retrieve and store hostname from DHCP lease if available
  proxinstall: use hostname from run env if available
  tui: use hostname from run env if available
  tui: add some tests for `NetworkInfo` -> `NetworkOptions` conversion

 Proxmox/Install/RunEnv.pm            |   4 +
 Proxmox/Sys/Net.pm                   |  28 ++++++
 proxinstall                          |  15 +--
 proxmox-tui-installer/src/options.rs | 132 +++++++++++++++++++++++++--
 proxmox-tui-installer/src/setup.rs   |   3 +
 proxmox-tui-installer/src/utils.rs   |   2 +-
 6 files changed, 169 insertions(+), 15 deletions(-)

--
2.42.0

[pve-devel] [PATCH installer 1/5] net: move hostname/fqdn regexes into common code

[Christoph Heiss]

Such that they can be re-used by other parts.
No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Sys/Net.pm | 3 +++
 proxinstall        | 9 +++++----
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Proxmox/Sys/Net.pm b/Proxmox/Sys/Net.pm
index ba368c1..f5a9885 100644
--- a/Proxmox/Sys/Net.pm
+++ b/Proxmox/Sys/Net.pm
@@ -6,6 +6,9 @@ use warnings;
 use base qw(Exporter);
 our @EXPORT_OK = qw(parse_ip_address parse_ip_mask);
 
+our $HOSTNAME_RE = "(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?)";
+our $FQDN_RE = "(?:${HOSTNAME_RE}\.)*${HOSTNAME_RE}";
+
 my $IPV4OCTET = "(?:25[0-5]|(?:2[0-4]|1[0-9]|[1-9])?[0-9])";
 my $IPV4RE = "(?:(?:$IPV4OCTET\\.){3}$IPV4OCTET)";
 my $IPV6H16 = "(?:[0-9a-fA-F]{1,4})";
diff --git a/proxinstall b/proxinstall
index d5b2565..88f194f 100755
--- a/proxinstall
+++ b/proxinstall
@@ -429,8 +429,6 @@ sub create_ipconf_view {
 	$text =~ s/^\s+//;
 	$text =~ s/\s+$//;
 
-	my $namere = "([a-zA-Z0-9]([a-zA-Z0-9\-]*[a-zA-Z0-9])?)";
-
 	# Debian does not support purely numeric hostnames
 	if ($text && $text =~ /^[0-9]+(?:\.|$)/) {
 	    Proxmox::UI::message("Purely numeric hostnames are not allowed.");
@@ -438,8 +436,11 @@ sub create_ipconf_view {
 	    return;
 	}
 
-	if ($text && $text =~ m/^(${namere}\.)*${namere}$/ && $text !~ m/.example.invalid$/ &&
-	    $text =~ m/^([^\.]+)\.(\S+)$/) {
+	if ($text
+	    && $text =~ m/^${Proxmox::Sys::Net::FQDN_RE}$/
+	    && $text !~ m/.example.invalid$/
+	    && $text =~ m/^([^\.]+)\.(\S+)$/
+	) {
 	    Proxmox::Install::Config::set_hostname($1);
 	    Proxmox::Install::Config::set_domain($2);
 	} else {
-- 
2.42.0

[pve-devel] [PATCH installer 2/5] run env: retrieve and store hostname from DHCP lease if available

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Install/RunEnv.pm |  4 ++++
 Proxmox/Sys/Net.pm        | 25 +++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index 19b5387..7589679 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -263,6 +263,10 @@ sub query_installation_environment : prototype() {
 	routes => $routes,
 	dns => query_dns(),
     };
+
+    # Cannot be put directly in the above hash as it might return undef ..
+    $output->{network}->{hostname} = Proxmox::Sys::Net::get_dhcp_hostname();
+
     # FIXME: move whatever makes sense over to Proxmox::Sys::Net:: and keep that as single source,
     # it can then use some different structure just fine (after adapting the GTK GUI to that) but
     # **never** to (slightly different!) things for the same stuff...
diff --git a/Proxmox/Sys/Net.pm b/Proxmox/Sys/Net.pm
index f5a9885..35d2abd 100644
--- a/Proxmox/Sys/Net.pm
+++ b/Proxmox/Sys/Net.pm
@@ -189,4 +189,29 @@ sub get_ip_config {
     }
 }
 
+# Tries to detect the hostname for this system via DHCP, if available.
+# DHCP server can set option 12 to inform the client about it's hostname [0].
+# dhclient dumps all options set by the DHCP server it in lease file, so just
+# read it from there.
+# [0] RFC 2132, section 3.14
+sub get_dhcp_hostname : prototype() {
+    my $leasefile = '/var/lib/dhcp/dhclient.leases';
+    return if ! -f $leasefile;
+
+    open (my $fh, '<', $leasefile) or return;
+
+    my $name = undef;
+    while (my $line = <$fh>) {
+	# "The name may or may not be qualified with the local domain name"
+	# Thus, only match the first part.
+	if ($line =~ m/^\s+option host-name \"(${FQDN_RE})\";$/) {
+	    $name = $1;
+	    last;
+	}
+    }
+
+    close($fh);
+    return $1 if defined($name) && $name =~ m/^([^\.]+)(?:\.(?:\S+))?$/;
+}
+
 1;
-- 
2.42.0

[pve-devel] [PATCH installer 3/5] proxinstall: use hostname from run env if available

[Christoph Heiss]

This now tries to use the hostname from the DHCP lease if it was set,
falling back to the product name as before.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxinstall | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/proxinstall b/proxinstall
index 88f194f..4d61fa7 100755
--- a/proxinstall
+++ b/proxinstall
@@ -402,9 +402,11 @@ sub create_ipconf_view {
     $vbox->pack_start($devicebox, 0, 0, 2);
 
     my $fqdn = Proxmox::Install::Config::get_fqdn();
-    my $hn = $fqdn // "$iso_env->{product}." . ($ipconf->{domain} // "example.invalid");
+    my $hostname = $run_env->{network}->{hostname} || $iso_env->{product};
+    my $domain = $ipconf->{domain} || "example.invalid";
+    $fqdn //= "$hostname.$domain";
 
-    my ($hostbox, $hostentry) = create_text_input($hn, 'Hostname (FQDN):');
+    my ($hostbox, $hostentry) = create_text_input($fqdn, 'Hostname (FQDN):');
     $vbox->pack_start($hostbox, 0, 0, 2);
 
     $vbox->pack_start($cidr_box, 0, 0, 2);
-- 
2.42.0

[pve-devel] [PATCH installer 4/5] tui: use hostname from run env if available

[Christoph Heiss]

This now tries to use the hostname from the DHCP lease if it was set,
falling back to the product name as before.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-tui-installer/src/options.rs | 22 +++++++++++++++-------
 proxmox-tui-installer/src/setup.rs   |  3 +++
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/proxmox-tui-installer/src/options.rs b/proxmox-tui-installer/src/options.rs
index a0a81c9..9e54da7 100644
--- a/proxmox-tui-installer/src/options.rs
+++ b/proxmox-tui-installer/src/options.rs
@@ -337,11 +337,16 @@ pub struct NetworkOptions {
     pub dns_server: IpAddr,
 }
 
+impl NetworkOptions {
+    const DEFAULT_DOMAIN: &str = "example.invalid";
+}
+
 impl Default for NetworkOptions {
     fn default() -> Self {
         let fqdn = format!(
-            "{}.example.invalid",
-            crate::current_product().default_hostname()
+            "{}.{}",
+            crate::current_product().default_hostname(),
+            Self::DEFAULT_DOMAIN
         );
         // TODO: Retrieve automatically
         Self {
@@ -363,11 +368,14 @@ impl From<&NetworkInfo> for NetworkOptions {
             this.dns_server = *ip;
         }
 
-        if let Some(domain) = &info.dns.domain {
-            let hostname = crate::current_product().default_hostname();
-            if let Ok(fqdn) = Fqdn::from(&format!("{hostname}.{domain}")) {
-                this.fqdn = fqdn;
-            }
+        let hostname = info
+            .hostname
+            .as_deref()
+            .unwrap_or_else(|| crate::current_product().default_hostname());
+        let domain = info.dns.domain.as_deref().unwrap_or(Self::DEFAULT_DOMAIN);
+
+        if let Ok(fqdn) = Fqdn::from(&format!("{hostname}.{domain}")) {
+            this.fqdn = fqdn;
         }
 
         if let Some(routes) = &info.routes {
diff --git a/proxmox-tui-installer/src/setup.rs b/proxmox-tui-installer/src/setup.rs
index 942e319..c7b32e5 100644
--- a/proxmox-tui-installer/src/setup.rs
+++ b/proxmox-tui-installer/src/setup.rs
@@ -408,6 +408,9 @@ pub struct NetworkInfo {
     /// (Contains no entries for devices with only link-local addresses.)
     #[serde(default)]
     pub interfaces: HashMap<String, Interface>,
+
+    /// The hostname of this machine, if set by the DHCP server.
+    pub hostname: Option<String>,
 }
 
 #[derive(Clone, Deserialize)]
-- 
2.42.0

[pve-devel] [PATCH installer 5/5] tui: add some tests for `NetworkInfo` -> `NetworkOptions` conversion

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-tui-installer/src/options.rs | 110 ++++++++++++++++++++++++++-
 proxmox-tui-installer/src/utils.rs   |   2 +-
 2 files changed, 110 insertions(+), 2 deletions(-)

diff --git a/proxmox-tui-installer/src/options.rs b/proxmox-tui-installer/src/options.rs
index 9e54da7..d4614aa 100644
--- a/proxmox-tui-installer/src/options.rs
+++ b/proxmox-tui-installer/src/options.rs
@@ -328,7 +328,7 @@ impl Default for PasswordOptions {
     }
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, PartialEq)]
 pub struct NetworkOptions {
     pub ifname: String,
     pub fqdn: Fqdn,
@@ -450,3 +450,111 @@ impl InstallerOptions {
         ]
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::setup::{
+        Dns, Gateway, Interface, IsoInfo, IsoLocations, NetworkInfo, ProductConfig, ProxmoxProduct,
+        Routes, SetupInfo,
+    };
+    use std::{collections::HashMap, path::PathBuf};
+
+    fn fill_setup_info() {
+        crate::init_setup_info(SetupInfo {
+            config: ProductConfig {
+                fullname: "Proxmox VE".to_owned(),
+                product: ProxmoxProduct::PVE,
+                enable_btrfs: true,
+            },
+            iso_info: IsoInfo {
+                release: String::new(),
+                isorelease: String::new(),
+            },
+            locations: IsoLocations {
+                iso: PathBuf::new(),
+            },
+        });
+    }
+
+    #[test]
+    fn network_options_from_setup_network_info() {
+        fill_setup_info();
+
+        let mut interfaces = HashMap::new();
+        interfaces.insert(
+            "eth0".to_owned(),
+            Interface {
+                name: "eth0".to_owned(),
+                index: 0,
+                mac: "01:23:45:67:89:ab".to_owned(),
+                addresses: Some(vec![
+                    CidrAddress::new(Ipv4Addr::new(192, 168, 0, 2), 24).unwrap()
+                ]),
+            },
+        );
+
+        let mut info = NetworkInfo {
+            dns: Dns {
+                domain: Some("bar.com".to_owned()),
+                dns: Vec::new(),
+            },
+            routes: Some(Routes {
+                gateway4: Some(Gateway {
+                    dev: "eth0".to_owned(),
+                    gateway: IpAddr::V4(Ipv4Addr::new(192, 168, 0, 1)),
+                }),
+                gateway6: None,
+            }),
+            interfaces,
+            hostname: Some("foo".to_owned()),
+        };
+
+        assert_eq!(
+            NetworkOptions::from(&info),
+            NetworkOptions {
+                ifname: "eth0".to_owned(),
+                fqdn: Fqdn::from("foo.bar.com").unwrap(),
+                address: CidrAddress::new(Ipv4Addr::new(192, 168, 0, 2), 24).unwrap(),
+                gateway: IpAddr::V4(Ipv4Addr::new(192, 168, 0, 1)),
+                dns_server: Ipv4Addr::UNSPECIFIED.into(),
+            }
+        );
+
+        info.hostname = None;
+        assert_eq!(
+            NetworkOptions::from(&info),
+            NetworkOptions {
+                ifname: "eth0".to_owned(),
+                fqdn: Fqdn::from("pve.bar.com").unwrap(),
+                address: CidrAddress::new(Ipv4Addr::new(192, 168, 0, 2), 24).unwrap(),
+                gateway: IpAddr::V4(Ipv4Addr::new(192, 168, 0, 1)),
+                dns_server: Ipv4Addr::UNSPECIFIED.into(),
+            }
+        );
+
+        info.dns.domain = None;
+        assert_eq!(
+            NetworkOptions::from(&info),
+            NetworkOptions {
+                ifname: "eth0".to_owned(),
+                fqdn: Fqdn::from("pve.example.invalid").unwrap(),
+                address: CidrAddress::new(Ipv4Addr::new(192, 168, 0, 2), 24).unwrap(),
+                gateway: IpAddr::V4(Ipv4Addr::new(192, 168, 0, 1)),
+                dns_server: Ipv4Addr::UNSPECIFIED.into(),
+            }
+        );
+
+        info.hostname = Some("foo".to_owned());
+        assert_eq!(
+            NetworkOptions::from(&info),
+            NetworkOptions {
+                ifname: "eth0".to_owned(),
+                fqdn: Fqdn::from("foo.example.invalid").unwrap(),
+                address: CidrAddress::new(Ipv4Addr::new(192, 168, 0, 2), 24).unwrap(),
+                gateway: IpAddr::V4(Ipv4Addr::new(192, 168, 0, 1)),
+                dns_server: Ipv4Addr::UNSPECIFIED.into(),
+            }
+        );
+    }
+}
diff --git a/proxmox-tui-installer/src/utils.rs b/proxmox-tui-installer/src/utils.rs
index 516f9c6..89349ed 100644
--- a/proxmox-tui-installer/src/utils.rs
+++ b/proxmox-tui-installer/src/utils.rs
@@ -33,7 +33,7 @@ pub enum CidrAddressParseError {
 /// assert_eq!(ipv4.to_string(), "192.168.0.1/24");
 /// assert_eq!(ipv6.to_string(), "2001:db8::c0a8:1/32");
 /// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, PartialEq)]
 pub struct CidrAddress {
     addr: IpAddr,
     mask: usize,
-- 
2.42.0

[pve-devel] applied-series: [PATCH installer 0/5] use hostname from DHCP lease if available

[Thomas Lamprecht]

Am 20/10/2023 um 11:46 schrieb Christoph Heiss:
> DHCP servers can set option 12 ("host-name") for client leases [0],
> telling them about their hostname. It's very much non-invasive and falls
> back to the default values as done currently.
> 
> This came up while talking to Aaron, which he noticed (esp. during
> trainings) that this would be a very useful feature too have.
> 
> I have tested this with the "host-name" entry set and unset, as well as
> any combinations of that with the domain name being set or unset.
> 
> [0] https://datatracker.ietf.org/doc/html/rfc2132#section-3.14
> 
> Christoph Heiss (5):
>   net: move hostname/fqdn regexes into common code
>   run env: retrieve and store hostname from DHCP lease if available
>   proxinstall: use hostname from run env if available
>   tui: use hostname from run env if available
>   tui: add some tests for `NetworkInfo` -> `NetworkOptions` conversion
> 
>  Proxmox/Install/RunEnv.pm            |   4 +
>  Proxmox/Sys/Net.pm                   |  28 ++++++
>  proxinstall                          |  15 +--
>  proxmox-tui-installer/src/options.rs | 132 +++++++++++++++++++++++++--
>  proxmox-tui-installer/src/setup.rs   |   3 +
>  proxmox-tui-installer/src/utils.rs   |   2 +-
>  6 files changed, 169 insertions(+), 15 deletions(-)
> 

applied series, squashed in a trivial fix to fix the tests though, you
probably based of an older git state where the Interface struct doesn't
have the "state" member yet, thanks!

Re: [pve-devel] applied-series: [PATCH installer 0/5] use hostname from DHCP lease if available

[Christoph Heiss]

On Fri, Oct 20, 2023 at 05:21:41PM +0200, Thomas Lamprecht wrote:
>
> Am 20/10/2023 um 11:46 schrieb Christoph Heiss:
> > DHCP servers can set option 12 ("host-name") for client leases [0],
> > telling them about their hostname. It's very much non-invasive and falls
> > back to the default values as done currently.
> >
> > This came up while talking to Aaron, which he noticed (esp. during
> > trainings) that this would be a very useful feature too have.
> >
> > I have tested this with the "host-name" entry set and unset, as well as
> > any combinations of that with the domain name being set or unset.
> >
> > [0] https://datatracker.ietf.org/doc/html/rfc2132#section-3.14
> > [..]
>
> applied series, squashed in a trivial fix to fix the tests though, you
> probably based of an older git state where the Interface struct doesn't
> have the "state" member yet, thanks!
Yeah, exactly. Sorry about that - thanks for fixing it up & applying!