[pve-devel] [PATCH installer 3/5] proxinstall: avoid open-coding FQDN sanity check

[Christoph Heiss]

.. by moving it into its own subroutine. Makes the whole thing quite a
bit neater and easier to maintain.

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss at proxmox.com>
---
FWIW, might be a nice case for refactoring using perlmod and reusing the
(more fleshed-out) Rust implementation from the proxmox-installer-common
crate. Having to implementations of the same thing to keep on par is
kind of a PITA.

 Proxmox/Sys/Net.pm | 22 +++++++++++++++++++-
 proxinstall        | 22 +++++++-------------
 test/Makefile      |  6 +++++-
 test/parse-fqdn.pl | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 83 insertions(+), 17 deletions(-)
 create mode 100755 test/parse-fqdn.pl

diff --git a/Proxmox/Sys/Net.pm b/Proxmox/Sys/Net.pm
index ed83fb0..3e01d37 100644
--- a/Proxmox/Sys/Net.pm
+++ b/Proxmox/Sys/Net.pm
@@ -4,7 +4,7 @@ use strict;
 use warnings;

 use base qw(Exporter);
-our @EXPORT_OK = qw(parse_ip_address parse_ip_mask);
+our @EXPORT_OK = qw(parse_ip_address parse_ip_mask parse_fqdn);

 our $HOSTNAME_RE = "(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?)";
 our $FQDN_RE = "(?:${HOSTNAME_RE}\.)*${HOSTNAME_RE}";
@@ -214,4 +214,24 @@ sub get_dhcp_fqdn : prototype() {
     return $name if defined($name) && $name =~ m/^([^\.]+)(?:\.(?:\S+))?$/;
 }

+# Follows the rules as laid out by proxmox_installer_common::utils::Fqdn
+sub parse_fqdn : prototype($) {
+    my ($text) = @_;
+
+    die "FQDN cannot be empty\n"
+	if !$text || length($text) == 0;
+
+    die "Purely numeric hostnames are not allowed\n"
+	if $text =~ /^[0-9]+(?:\.|$)/;
+
+    die "FQDN must only consist of alphanumeric characters and dashes\n"
+	if $text !~ m/^${Proxmox::Sys::Net::FQDN_RE}$/;
+
+    if ($text =~ m/^([^\.]+)\.(\S+)$/) {
+	return ($1, $2);
+    }
+
+    die "Hostname does not look like a fully qualified domain name\n";
+}
+
 1;
diff --git a/proxinstall b/proxinstall
index 81dd368..4265ba7 100755
--- a/proxinstall
+++ b/proxinstall
@@ -446,24 +446,16 @@ sub create_ipconf_view {
 	$text =~ s/^\s+//;
 	$text =~ s/\s+$//;

-	# Debian does not support purely numeric hostnames
-	if ($text && $text =~ /^[0-9]+(?:\.|$)/) {
-	    Proxmox::UI::message("Purely numeric hostnames are not allowed.");
-	    $hostentry->grab_focus();
-	    return;
-	}
+	my ($hostname, $domainname) = eval { Proxmox::Sys::Net::parse_fqdn($text) };
+	my $err = $@;

-	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 {
-	    Proxmox::UI::message("Hostname does not look like a fully qualified domain name.");
+	if ($err || $text =~ m/.example.invalid$/) {
+	    Proxmox::UI::message($err // 'Hostname does not look like a valid fully qualified domain name');
 	    $hostentry->grab_focus();
 	    return;
+	} else {
+	    Proxmox::Install::Config::set_hostname($hostname);
+	    Proxmox::Install::Config::set_domain($domainname);
 	}

 	# verify ip address
diff --git a/test/Makefile b/test/Makefile
index fb80fc4..004bc1e 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -3,8 +3,12 @@ all:
 export PERLLIB=..

 .PHONY: check
-check: test-zfs-arc-max
+check: test-zfs-arc-max test-parse-fqdn

 .PHONY: test-zfs-arc-max
 test-zfs-arc-max:
 	./zfs-arc-max.pl
+
+.PHONY: test-parse-fqdn
+test-parse-fqdn:
+	./parse-fqdn.pl
diff --git a/test/parse-fqdn.pl b/test/parse-fqdn.pl
new file mode 100755
index 0000000..1ffb300
--- /dev/null
+++ b/test/parse-fqdn.pl
@@ -0,0 +1,50 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use Test::More;
+
+use Proxmox::Sys::Net qw(parse_fqdn);
+
+# some constants just to avoid repeating ourselves
+use constant ERR_INVALID => "Hostname does not look like a fully qualified domain name\n";
+use constant ERR_ALPHANUM => "FQDN must only consist of alphanumeric characters and dashes\n";
+use constant ERR_NUMERIC => "Purely numeric hostnames are not allowed\n";
+use constant ERR_TOOLONG => "FQDN too long\n";
+use constant ERR_EMPTY => "FQDN cannot be empty\n";
+
+sub is_parsed {
+    my ($fqdn, $expected) = @_;
+
+    my @parsed = parse_fqdn($fqdn);
+    is_deeply(\@parsed, $expected, "FQDN is valid and compared successfully: $fqdn");
+}
+
+sub is_invalid {
+    my ($fqdn, $expected_err) = @_;
+
+    my $parsed = eval { parse_fqdn($fqdn) };
+    is($parsed, undef, "invalid FQDN did fail parsing: $fqdn");
+    is($@, $expected_err, "invalid FQDN threw correct error: $fqdn");
+}
+
+is_invalid(undef, ERR_EMPTY);
+is_invalid('', ERR_EMPTY);
+
+is_parsed('foo.example.com', ['foo', 'example.com']);
+is_parsed('foo-bar.com', ['foo-bar', 'com']);
+is_parsed('a-b.com', ['a-b', 'com']);
+
+is_invalid('foo', ERR_INVALID);
+is_invalid('-foo.com', ERR_ALPHANUM);
+is_invalid('foo-.com', ERR_ALPHANUM);
+is_invalid('foo.com-', ERR_ALPHANUM);
+is_invalid('-o-.com', ERR_ALPHANUM);
+
+# https://bugzilla.proxmox.com/show_bug.cgi?id=1054
+is_invalid('123.com', ERR_NUMERIC);
+is_parsed('foo123.com', ['foo123', 'com']);
+is_parsed('123foo.com', ['123foo', 'com']);
+
+done_testing();
--
2.43.0