[pve-devel] [PATCH installer v3 15/15] gui: add support for pinning network interface names

[Christoph Heiss <c.heiss@proxmox.com>]

Adds an additional checkbox and option button in the network panel, the
latter triggering a dialog for setting custom names per network
interface present on the system.

Pinning is enabled by default.

Each pinned network interface name defaults to `nicN`, where N is the
pinned ID from the low-level installer.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v2 -> v3:
  * moved pinning map validation into own subroutine in
    Proxmox::Sys::Net
  * added unit tests for the above, taken from the rust implementation

Changes v1 -> v2:
  * added better validation for interface names, now according to our
    `pve-iface` json schema
  * implemented gtk-specific suggestions made by Maximilano
  * run `make tidy`
  * fixed error message dialog getting wrongly z-ordered

 Proxmox/Sys/Net.pm            |  62 ++++++++++-
 proxinstall                   | 189 ++++++++++++++++++++++++++++++----
 test/Makefile                 |   7 +-
 test/validate-link-pin-map.pl |  42 ++++++++
 4 files changed, 278 insertions(+), 22 deletions(-)
 create mode 100755 test/validate-link-pin-map.pl

diff --git a/Proxmox/Sys/Net.pm b/Proxmox/Sys/Net.pm
index 2183d27..991f723 100644
--- a/Proxmox/Sys/Net.pm
+++ b/Proxmox/Sys/Net.pm
@@ -8,7 +8,21 @@ use Proxmox::Sys::Udev;
 use JSON qw();
 
 use base qw(Exporter);
-our @EXPORT_OK = qw(parse_ip_address parse_ip_mask parse_fqdn);
+our @EXPORT_OK = qw(
+    parse_ip_address
+    parse_ip_mask
+    parse_fqdn
+    validate_link_pin_map
+    MAX_IFNAME_LEN
+    DEFAULT_PIN_PREFIX
+);
+
+# Maximum length of the (primary) name of a network interface
+# IFNAMSIZ - 1 to account for NUL byte
+use constant {
+    MAX_IFNAME_LEN => 15,
+    DEFAULT_PIN_PREFIX => 'nic',
+};
 
 our $HOSTNAME_RE = "(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]{,61}?[a-zA-Z0-9])?)";
 our $FQDN_RE = "(?:${HOSTNAME_RE}\\.)*${HOSTNAME_RE}";
@@ -310,4 +324,50 @@ sub parse_fqdn : prototype($) {
     die "Hostname does not look like a fully qualified domain name\n";
 }
 
+# Does some basic checks on a link name mapping.
+# Takes a hashref mapping mac => name.
+#
+# Includes checks for:
+# - empty interface names
+# - overlong interface names
+# - duplicate interface names
+# - only contains ASCII alphanumeric characters and underscore, as enforced by
+#   our `pve-iface` json schema.
+sub validate_link_pin_map : prototype($) {
+    my ($mapping) = @_;
+    my $reverse_mapping = {};
+
+    while (my ($mac, $name) = each %$mapping) {
+        if (!defined($name) || $name eq '') {
+            die "interface name for '$mac' cannot be empty\n";
+        }
+
+        if (length($name) > MAX_IFNAME_LEN) {
+            die "interface name '$name' for '$mac' cannot be longer than "
+                . MAX_IFNAME_LEN
+                . " characters\n";
+        }
+
+        if ($name =~ m/^[0-9]+$/) {
+            die "interface name '$name' for '$mac' is invalid: "
+                . "name must not be fully numeric\n";
+        }
+
+        if ($name =~ m/^[0-9]/) {
+            die "interface name '$name' for '$mac' is invalid: name must not start with a number\n";
+        }
+
+        if ($name !~ m/^[a-zA-Z_][a-zA-Z0-9_]*$/) {
+            die "interface name '$name' for '$mac' is invalid: "
+                . "name must only consist of alphanumeric characters and underscores\n";
+        }
+
+        if ($reverse_mapping->{$name}) {
+            die "duplicate interface name mapping '$name' for: $mac, $reverse_mapping->{$name}\n";
+        }
+
+        $reverse_mapping->{$name} = $mac;
+    }
+}
+
 1;
diff --git a/proxinstall b/proxinstall
index 5ba65fa..49dd796 100755
--- a/proxinstall
+++ b/proxinstall
@@ -37,7 +37,8 @@ use Proxmox::Sys;
 use Proxmox::Sys::Block qw(get_cached_disks);
 use Proxmox::Sys::Command qw(syscmd);
 use Proxmox::Sys::File qw(file_read_all file_write_all);
-use Proxmox::Sys::Net qw(parse_ip_address parse_ip_mask);
+use Proxmox::Sys::Net
+    qw(parse_ip_address parse_ip_mask validate_link_pin_map MAX_IFNAME_LEN DEFAULT_PIN_PREFIX);
 use Proxmox::UI;
 
 my $step_number = 0; # Init number for global function list
@@ -340,11 +341,92 @@ my $create_basic_grid = sub {
     return $grid;
 };
 
+my sub create_network_interface_pin_view {
+    my ($done_cb) = @_;
+
+    my $dialog = Gtk3::Dialog->new();
+    $dialog->set_title('Interface Name Pinning Options');
+    $dialog->add_button('_OK', 'ok');
+
+    my $content = $dialog->get_content_area();
+
+    my $hbox = Gtk3::Box->new('horizontal', 0);
+    $content->pack_start($hbox, 1, 1, 5);
+
+    my $grid = Gtk3::Grid->new();
+    $grid->set_column_spacing(10);
+    $grid->set_row_spacing(10);
+
+    # make the list scrollable, in case there are lots of interfaces
+    my $scrolled_window = Gtk3::ScrolledWindow->new();
+    $scrolled_window->set_hexpand(1);
+    $scrolled_window->set_propagate_natural_height(1);
+
+    $scrolled_window->add($grid);
+    $scrolled_window->set_policy('never', 'automatic');
+    $scrolled_window->set_min_content_height(200);
+    $scrolled_window->set_margin_start(10);
+    $scrolled_window->set_margin_end(10);
+
+    $hbox->pack_start($scrolled_window, 1, 0, 5);
+
+    my $interfaces = Proxmox::Install::RunEnv::get()->{network}->{interfaces};
+    my $mapping = Proxmox::Install::Config::get_network_interface_pin_map();
+
+    my $inputs = {};
+    my $row = 0;
+    for my $ifname (sort keys $interfaces->%*) {
+        my $iface = $interfaces->{$ifname};
+
+        my $name = $mapping->{ $iface->{mac} };
+        my $label_text = "$iface->{mac} ($ifname, $iface->{driver}, $iface->{state})";
+
+        # if the interface has addresses assigned through DHCP, show them for
+        # reference
+        if (defined($iface->{addresses})) {
+            $label_text .=
+                "\n  " . join(', ', map { "$_->{address}/$_->{prefix}" } @{ $iface->{addresses} });
+        }
+
+        my ($label, $input) = create_text_input($name, $label_text);
+        $label->set_xalign(0.);
+
+        $grid->attach($label, 0, $row, 1, 1);
+        $grid->attach($input, 1, $row, 1, 1);
+        $row++;
+
+        $inputs->{ $iface->{mac} } = $input;
+    }
+
+    $hbox->show_all();
+
+    $dialog->signal_connect(
+        response => sub {
+            my %new_mapping = map { $_ => $inputs->{$_}->get_text() } keys %$inputs;
+
+            eval { validate_link_pin_map(\%new_mapping); };
+            if ($@) {
+                Proxmox::UI::message($@, $dialog);
+                return;
+            }
+
+            Proxmox::Install::Config::set_network_interface_pin_map(\%new_mapping);
+            $dialog->destroy();
+            $done_cb->();
+        },
+    );
+
+    $dialog->present();
+}
+
 sub create_ipconf_view {
 
     cleanup_view();
     Proxmox::UI::display_html('ipconf.htm');
 
+    my $run_env = Proxmox::Install::RunEnv::get();
+    my $ipconf = $run_env->{ipconf};
+
     my $grid = &$create_basic_grid();
     $grid->set_row_spacing(10);
     $grid->set_column_spacing(10);
@@ -355,7 +437,7 @@ sub create_ipconf_view {
 
     my ($cidr_label, $cidr_box, $ipconf_entry_addr, $ipconf_entry_mask) = create_cidr_inputs($cidr);
 
-    my $device_model = Gtk3::ListStore->new('Glib::String', 'Glib::String');
+    my $device_model = Gtk3::ListStore->new('Glib::String', 'Glib::String', 'Glib::String');
     my $device_cb = Gtk3::ComboBox->new_with_model($device_model);
     $device_cb->set_active(0);
     $device_cb->set_visible(1);
@@ -369,19 +451,62 @@ sub create_ipconf_view {
     $device_cb->pack_start($cell, 0);
     $device_cb->add_attribute($cell, 'text', 1);
 
-    my $get_device_desc = sub {
-        my $iface = shift;
-        return "$iface->{name} - $iface->{mac} ($iface->{driver})";
+    my $refresh_device_cb = sub {
+        # clear all entries and re-add them with their new names
+        my $active = $device_cb->get_active();
+        $device_model->clear();
+
+        my $mapping = Proxmox::Install::Config::get_network_interface_pin_map();
+        my $i = 0;
+        for my $index (sort keys $ipconf->{ifaces}->%*) {
+            my $iface = $ipconf->{ifaces}->{$index};
+            my $iter = $device_model->append();
+
+            my $symbol = "$iface->{state}" eq "UP" ? "\x{25CF}" : ' ';
+            my $name =
+                $gtk_state->{network_pinning_enabled}
+                ? $mapping->{ $iface->{mac} }
+                : $iface->{name};
+
+            $device_model->set(
+                $iter,
+                0 => $symbol,
+                1 => "$name - $iface->{mac} ($iface->{driver})",
+            );
+            $i++;
+        }
+
+        # re-set the currently active entry to keep the users selection
+        $device_cb->set_active($active);
     };
 
-    my $run_env = Proxmox::Install::RunEnv::get();
-    my $ipconf = $run_env->{ipconf};
+    my $name_pin_opts_button = Gtk3::Button->new('Options');
+    $name_pin_opts_button->set_sensitive($gtk_state->{network_pinning_enabled});
+    $name_pin_opts_button->signal_connect(
+        clicked => sub {
+            create_network_interface_pin_view($refresh_device_cb);
+        },
+    );
+
+    my $name_pin_checkbox = Gtk3::CheckButton->new('Pin network interface names');
+    $name_pin_checkbox->set_active($gtk_state->{network_pinning_enabled});
+    $name_pin_checkbox->signal_connect(
+        toggled => sub {
+            $name_pin_opts_button->set_sensitive(!!$name_pin_checkbox->get_active());
+            $gtk_state->{network_pinning_enabled} = !!$name_pin_checkbox->get_active();
+            $refresh_device_cb->();
+        },
+    );
 
     my ($device_active_map, $device_active_reverse_map) = ({}, {});
 
     my $device_change_handler = sub {
         my $current = shift;
 
+        # happens during the clear + re-insertion of all interfaces after
+        # the pinning changed, can be safely ignored
+        return if $current->get_active() == -1;
+
         my $new = $device_active_map->{ $current->get_active() };
         my $iface = $ipconf->{ifaces}->{$new};
 
@@ -389,6 +514,7 @@ sub create_ipconf_view {
         return if defined($selected) && $iface->{name} eq $selected;
 
         Proxmox::Install::Config::set_mngmt_nic($iface->{name});
+
         $ipconf_entry_addr->set_text($iface->{inet}->{addr} || $iface->{inet6}->{addr})
             if $iface->{inet}->{addr} || $iface->{inet6}->{addr};
         $ipconf_entry_mask->set_text($iface->{inet}->{prefix} || $iface->{inet6}->{prefix})
@@ -400,13 +526,6 @@ sub create_ipconf_view {
     my $i = 0;
     for my $index (sort keys $ipconf->{ifaces}->%*) {
         my $iface = $ipconf->{ifaces}->{$index};
-        my $iter = $device_model->append();
-        my $symbol = "$iface->{state}" eq "UP" ? "\x{25CF}" : ' ';
-        $device_model->set(
-            $iter,
-            0 => $symbol,
-            1 => $get_device_desc->($iface),
-        );
         $device_active_map->{$i} = $index;
         $device_active_reverse_map->{ $iface->{name} } = $i;
 
@@ -418,6 +537,9 @@ sub create_ipconf_view {
         $i++;
     }
 
+    # fill the combobox with entries
+    $refresh_device_cb->();
+
     if (my $nic = Proxmox::Install::Config::get_mngmt_nic()) {
         $initial_active_device_pos = $device_active_reverse_map->{$nic};
     } else {
@@ -443,7 +565,7 @@ sub create_ipconf_view {
     $label->set_xalign(1.0);
 
     $grid->attach($label, 0, 0, 1, 1);
-    $grid->attach($device_cb, 1, 0, 1, 1);
+    $grid->attach($device_cb, 1, 0, 2, 1);
 
     my $fqdn = Proxmox::Install::Config::get_fqdn();
     my $hostname = $run_env->{network}->{hostname} || $iso_env->{product};
@@ -452,17 +574,17 @@ sub create_ipconf_view {
 
     my ($host_label, $hostentry) = create_text_input($fqdn, 'Hostname (FQDN)');
     $grid->attach($host_label, 0, 1, 1, 1);
-    $grid->attach($hostentry, 1, 1, 1, 1);
+    $grid->attach($hostentry, 1, 1, 2, 1);
 
     $grid->attach($cidr_label, 0, 2, 1, 1);
-    $grid->attach($cidr_box, 1, 2, 1, 1);
+    $grid->attach($cidr_box, 1, 2, 2, 1);
 
     my $cfg_gateway = Proxmox::Install::Config::get_gateway();
     my $gateway = $cfg_gateway // $ipconf->{gateway} || '192.168.100.1';
 
     my ($gw_label, $ipconf_entry_gw) = create_text_input($gateway, 'Gateway');
     $grid->attach($gw_label, 0, 3, 1, 1);
-    $grid->attach($ipconf_entry_gw, 1, 3, 1, 1);
+    $grid->attach($ipconf_entry_gw, 1, 3, 2, 1);
 
     my $cfg_dns = Proxmox::Install::Config::get_dns();
     my $dnsserver = $cfg_dns // $ipconf->{dnsserver} || $gateway;
@@ -470,7 +592,10 @@ sub create_ipconf_view {
     my ($dns_label, $ipconf_entry_dns) = create_text_input($dnsserver, 'DNS Server');
 
     $grid->attach($dns_label, 0, 4, 1, 1);
-    $grid->attach($ipconf_entry_dns, 1, 4, 1, 1);
+    $grid->attach($ipconf_entry_dns, 1, 4, 2, 1);
+
+    $grid->attach($name_pin_checkbox, 1, 5, 1, 1);
+    $grid->attach($name_pin_opts_button, 2, 5, 1, 1);
 
     $gtk_state->{inbox}->show_all;
     set_next(
@@ -538,6 +663,8 @@ sub create_ipconf_view {
             }
             Proxmox::Install::Config::set_dns($dns_ip);
 
+            $gtk_state->{network_pinning_enabled} = !!$name_pin_checkbox->get_active();
+
             #print STDERR "TEST $ipaddress/$netmask $gateway_ip $dns_ip\n";
 
             $step_number++;
@@ -573,6 +700,13 @@ sub create_ack_view {
 
     my $country = Proxmox::Install::Config::get_country();
 
+    my $mngmt_nic = Proxmox::Install::Config::get_mngmt_nic();
+    my $iface = Proxmox::Install::RunEnv::get('network')->{interfaces}->{$mngmt_nic};
+
+    my $nic_mapping = Proxmox::Install::Config::get_network_interface_pin_map();
+    my $interface =
+        $gtk_state->{network_pinning_enabled} ? $nic_mapping->{ $iface->{mac} } : $iface->{name};
+
     my %config_values = (
         __target_hd__ => join(' | ', $target_hds->@*),
         __target_fs__ => Proxmox::Install::Config::get_filesys(),
@@ -580,7 +714,7 @@ sub create_ack_view {
         __timezone__ => Proxmox::Install::Config::get_timezone(),
         __keymap__ => Proxmox::Install::Config::get_keymap(),
         __mailto__ => Proxmox::Install::Config::get_mailto(),
-        __interface__ => Proxmox::Install::Config::get_mngmt_nic(),
+        __interface__ => $interface,
         __hostname__ => Proxmox::Install::Config::get_hostname(),
         __cidr__ => Proxmox::Install::Config::get_cidr(),
         __gateway__ => Proxmox::Install::Config::get_gateway(),
@@ -609,6 +743,12 @@ sub create_ack_view {
     set_next(
         undef,
         sub {
+            # before starting the install, unset the name pinning map if it
+            # is disabled
+            if (!$gtk_state->{network_pinning_enabled}) {
+                Proxmox::Install::Config::set_network_interface_pin_map(undef);
+            }
+
             $step_number++;
             create_extract_view();
         },
@@ -1775,6 +1915,15 @@ if (!$initial_error && (scalar keys $run_env->{ipconf}->{ifaces}->%* == 0)) {
     $initial_error = 1;
     Proxmox::UI::display_html("nonics.htm");
     set_next("Reboot", sub { app_quit(0); });
+} else {
+    # we enable it by default for new installation
+    $gtk_state->{network_pinning_enabled} = 1;
+
+    # pre-fill the name mapping before starting
+    my %mapping = map {
+        $_->{mac} => DEFAULT_PIN_PREFIX . $_->{pinned_id}
+    } values $run_env->{network}->{interfaces}->%*;
+    Proxmox::Install::Config::set_network_interface_pin_map(\%mapping);
 }
 
 create_intro_view() if !$initial_error;
diff --git a/test/Makefile b/test/Makefile
index 70a05be..bfe7674 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -4,7 +4,8 @@ export PERLLIB=..
 
 .PHONY: check
 check: test-zfs-arc-max test-run-command test-parse-fqdn test-ui2-stdio \
-       test-zfs-get-pool-list test-parse-kernel-cmdline
+       test-zfs-get-pool-list test-parse-kernel-cmdline \
+       test-validate-link-pin-map
 
 .PHONY: test-zfs-arc-max
 test-zfs-arc-max:
@@ -29,3 +30,7 @@ test-zfs-get-pool-list:
 .PHONY: test-parse-kernel-cmdline
 test-parse-kernel-cmdline:
 	./parse-kernel-cmdline.pl
+
+.PHONY: test-validate-link-pin-map
+test-validate-link-pin-map:
+	./validate-link-pin-map.pl
diff --git a/test/validate-link-pin-map.pl b/test/validate-link-pin-map.pl
new file mode 100755
index 0000000..6386700
--- /dev/null
+++ b/test/validate-link-pin-map.pl
@@ -0,0 +1,42 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use Test::More;
+
+use Proxmox::Sys::Net qw(validate_link_pin_map);
+
+eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => '' }) };
+is($@, "interface name for 'ab:cd:ef:12:34:56' cannot be empty\n");
+
+eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => 'waytoolonginterfacename' }) };
+is(
+    $@,
+    "interface name 'waytoolonginterfacename' for 'ab:cd:ef:12:34:56' cannot be longer than 15 characters\n",
+);
+
+eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => 'nic0', '12:34:56:ab:cd:ef' => 'nic0' }) };
+like($@, qr/^duplicate interface name mapping 'nic0' for: /);
+like($@, qr/ab:cd:ef:12:34:56/);
+like($@, qr/12:34:56:ab:cd:ef/);
+
+eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => 'nic-' }) };
+is(
+    $@,
+    "interface name 'nic-' for 'ab:cd:ef:12:34:56' is invalid: name must only consist of alphanumeric characters and underscores\n",
+);
+
+eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => '0nic' }) };
+is(
+    $@,
+    "interface name '0nic' for 'ab:cd:ef:12:34:56' is invalid: name must not start with a number\n",
+);
+
+eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => '12345' }) };
+is(
+    $@,
+    "interface name '12345' for 'ab:cd:ef:12:34:56' is invalid: name must not be fully numeric\n",
+);
+
+done_testing();
-- 
2.51.0



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