[PATCH installer v4 11/30] install: factor out ifupdown2 config generation into own subroutine

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

Makes it more readable and to add more logic in the future, as well as
being now unit-testable.

While at it, unify the `address` and `gateway` and keep it of the
branched paths for bridged/unbridged. Makes things simpler and the
final configuration more predictable in layout.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v3 -> v4:
  * no changes

Changes v2 -> v3:
  * only generate config in subroutine, keep the writing of
    /etc/network/interfaces in the main logic
  * fix missing translation of $ethdev to pinned name, by passing it
    directly as a parameter
  * add initial testsuite for ifupdown2 config generation (including
    additional tests from @Thomas)
  * simplify config generation a bit by moving address and gateway
    setup into common path

Changes v1 -> v2:
  * no changes

 Proxmox/Install.pm                | 134 ++++++++++----------
 test/Makefile                     |   6 +-
 test/generate-ifupdown2-config.pl | 197 ++++++++++++++++++++++++++++++
 3 files changed, 270 insertions(+), 67 deletions(-)
 create mode 100755 test/generate-ifupdown2-config.pl

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index 7bb8e29..a98571f 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -798,6 +798,71 @@ my sub setup_proxmox_first_boot_service {
     }
 }
 
+sub generate_ifupdown2_config {
+    my $run_env = Proxmox::Install::RunEnv::get();
+    my $iso_env = Proxmox::Install::ISOEnv::get();
+
+    my $ip_version = Proxmox::Install::Config::get_ip_version();
+    my $ntype = $ip_version == 4 ? 'inet' : 'inet6';
+
+    my $interfaces = $run_env->{network}->{interfaces};
+    my $ethdev = Proxmox::Install::Config::get_mngmt_nic();
+    my $cidr = Proxmox::Install::Config::get_cidr();
+    my $gateway = Proxmox::Install::Config::get_gateway();
+
+    my $netif_pin_map = Proxmox::Install::Config::get_network_interface_pin_map();
+
+    # frontends may pass the original kernel name of an interface that gets pinned
+    if (defined($netif_pin_map) && defined($interfaces->{$ethdev})) {
+        $ethdev = $netif_pin_map->{ $interfaces->{$ethdev}->{mac} } // $ethdev;
+    }
+
+    my $ifaces = "auto lo\niface lo inet loopback\n\n";
+
+    # iproute2 omits the gateway and protocol keys for some routes; route devices are original
+    # kernel interface names, while $ethdev can be the pinned one
+    my $gateways6 = $run_env->{network}->{routes}->{gateways6} // [];
+    my $is_gateway6_from_ra = grep {
+        my $dev = $_->{dev} // '';
+        if (defined($netif_pin_map) && defined($interfaces->{$dev})) {
+            $dev = $netif_pin_map->{ $interfaces->{$dev}->{mac} } // $dev;
+        }
+        $dev eq $ethdev
+            && ($_->{gateway} // '') eq $gateway
+            && ($_->{protocol} // '') eq 'ra';
+    } $gateways6->@*;
+
+    if ($iso_env->{cfg}->{bridged_network}) {
+        $ifaces .= "iface $ethdev $ntype manual\n";
+
+        $ifaces .=
+            "\nauto vmbr0\niface vmbr0 $ntype static\n"
+            . "\tbridge-ports $ethdev\n"
+            . "\tbridge-stp off\n"
+            . "\tbridge-fd 0\n";
+    } else {
+        $ifaces .= "auto $ethdev\n" . #
+            "iface $ethdev $ntype static\n";
+    }
+
+    $ifaces .= "\taddress $cidr\n";
+    if (!$is_gateway6_from_ra) {
+        $ifaces .= "\tgateway $gateway\n";
+    }
+
+    foreach my $iface (sort keys $interfaces->%*) {
+        my $if = $interfaces->{$iface};
+        my $name = defined($netif_pin_map) ? $netif_pin_map->{ $if->{mac} } : $if->{name};
+
+        next if $name eq $ethdev;
+
+        $ifaces .= "\niface $name $ntype manual\n";
+    }
+
+    $ifaces .= "\n\nsource /etc/network/interfaces.d/*\n";
+    return $ifaces;
+}
+
 sub extract_data {
     my $iso_env = Proxmox::Install::ISOEnv::get();
     my $run_env = Proxmox::Install::RunEnv::get();
@@ -1138,21 +1203,11 @@ sub extract_data {
 
         syscmd("/bin/hostname $hostname") if !is_test_mode();
 
-        # configure interfaces
-
-        my $ifaces = "auto lo\niface lo inet loopback\n\n";
-
-        my $ip_version = Proxmox::Install::Config::get_ip_version();
-        my $ntype = $ip_version == 4 ? 'inet' : 'inet6';
-
-        my $ethdev = Proxmox::Install::Config::get_mngmt_nic();
-        my $cidr = Proxmox::Install::Config::get_cidr();
-        my $gateway = Proxmox::Install::Config::get_gateway();
-
         # configure pinned names for all network interfaces, if enabled
 
         my $netif_pin_map = Proxmox::Install::Config::get_network_interface_pin_map();
         if (defined($netif_pin_map)) {
+
             mkdir "$targetdir/usr/local/lib/systemd", 0755;
             mkdir "$targetdir/usr/local/lib/systemd/network", 0755;
 
@@ -1178,64 +1233,11 @@ sub extract_data {
                     "$targetdir/usr/local/lib/systemd/network/50-pmx-${pinned_name}.link",
                     $link_file_content,
                 );
-
-                # ensure that we use the correct interface name when writing out
-                # /etc/network/interfaces below - just in case the interface name
-                # is pinned and get_mngmt_nic() returned the original interface name.
-                # better be safe than sorry
-                if ($ethdev eq $ifname) {
-                    $ethdev = $pinned_name;
-                }
             }
         }
 
-        my $interfaces = $run_env->{network}->{interfaces};
-
-        # iproute2 omits the gateway and protocol keys for some routes; route devices are original
-        # kernel interface names, while $ethdev can be the pinned one
-        my $gateways6 = $run_env->{network}->{routes}->{gateways6} // [];
-        my $is_gateway6_from_ra = grep {
-            my $dev = $_->{dev} // '';
-            if (defined($netif_pin_map) && defined($interfaces->{$dev})) {
-                $dev = $netif_pin_map->{ $interfaces->{$dev}->{mac} } // $dev;
-            }
-            $dev eq $ethdev
-                && ($_->{gateway} // '') eq $gateway
-                && ($_->{protocol} // '') eq 'ra';
-        } $gateways6->@*;
-
-        if ($iso_env->{cfg}->{bridged_network}) {
-            $ifaces .= "iface $ethdev $ntype manual\n";
-
-            $ifaces .=
-                "\nauto vmbr0\niface vmbr0 $ntype static\n"
-                . "\taddress $cidr\n"
-                . "\tgateway $gateway\n"
-                . "\tbridge-ports $ethdev\n"
-                . "\tbridge-stp off\n"
-                . "\tbridge-fd 0\n";
-        } else {
-            $ifaces .= "auto $ethdev\n" . #
-                "iface $ethdev $ntype static\n" . #
-                "\taddress $cidr\n";
-
-            if (!$is_gateway6_from_ra) {
-                $ifaces .= "\tgateway $gateway\n";
-            }
-        }
-
-        foreach my $iface (sort keys $interfaces->%*) {
-            my $if = $interfaces->{$iface};
-            my $name = defined($netif_pin_map) ? $netif_pin_map->{ $if->{mac} } : $if->{name};
-
-            next if $name eq $ethdev;
-
-            $ifaces .= "\niface $name $ntype manual\n";
-        }
-
-        $ifaces .= "\n\nsource /etc/network/interfaces.d/*\n";
-
-        file_write_all("$targetdir/etc/network/interfaces", $ifaces);
+        # configure interfaces
+        file_write_all("$targetdir/etc/network/interfaces", generate_ifupdown2_config());
 
         # configure dns
 
diff --git a/test/Makefile b/test/Makefile
index bfe7674..9727244 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -5,7 +5,7 @@ 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-validate-link-pin-map
+       test-validate-link-pin-map test-generate-ifupdown2-config
 
 .PHONY: test-zfs-arc-max
 test-zfs-arc-max:
@@ -34,3 +34,7 @@ test-parse-kernel-cmdline:
 .PHONY: test-validate-link-pin-map
 test-validate-link-pin-map:
 	./validate-link-pin-map.pl
+
+.PHONY: test-generate-ifupdown2-config
+test-generate-ifupdown2-config:
+	./generate-ifupdown2-config.pl
diff --git a/test/generate-ifupdown2-config.pl b/test/generate-ifupdown2-config.pl
new file mode 100755
index 0000000..5c1794e
--- /dev/null
+++ b/test/generate-ifupdown2-config.pl
@@ -0,0 +1,197 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Test::More;
+use Test::MockModule qw(strict);
+
+use Proxmox::Install;
+use Proxmox::Install::Config;
+
+use Proxmox::Log;
+Proxmox::Log::init("/tmp/install.log");
+
+my $proxmox_install_runenv = Test::MockModule->new('Proxmox::Install::RunEnv');
+my $proxmox_install_isoenv = Test::MockModule->new('Proxmox::Install::ISOEnv');
+
+my $current_cfg;
+my sub set_config {
+    my (%cfg) = @_;
+
+    $current_cfg = \%cfg;
+    $proxmox_install_isoenv->redefine(
+        get => sub {
+            return {
+                product => $cfg{bridge} ? 'pve' : 'pbs',
+                cfg => {
+                    product => $cfg{bridge} ? 'pve' : 'pbs',
+                    bridged_network => $cfg{bridge},
+                },
+            };
+        },
+    );
+}
+
+my sub is_generated {
+    my ($expected, $msg) = @_;
+
+    is(
+        Proxmox::Install::generate_ifupdown2_config(), <<EOT,
+auto lo
+iface lo inet loopback
+
+$expected
+
+source /etc/network/interfaces.d/*
+EOT
+        "$msg (bridge:$current_cfg->{bridge})",
+    );
+}
+
+# global config
+$proxmox_install_runenv->redefine(
+    get => sub {
+        return {
+            network => {
+                interfaces => {
+                    eno1 => {
+                        mac => '12:34:56:ab:cd:ef',
+                        index => 1,
+                        name => 'eno1',
+                    },
+                    eno2 => {
+                        mac => 'ab:cd:ef:12:34:56',
+                        index => 2,
+                        name => 'eno2',
+                    },
+                },
+            },
+        };
+    },
+);
+set_config(bridge => 0);
+Proxmox::Install::Config::merge({
+    mngmt_nic => 'eno1',
+    cidr => '192.168.0.10/24',
+    gateway => '192.168.0.1',
+});
+
+###
+# There's essentially a matrix to test w.r.t the overall networking setup:
+#
+# +-------+--------+
+# | test# | bridge |
+# +=======+========+
+# |   1   |    0   |
+# |   2   |    1   |
+# +-------+--------+
+###
+
+# 1.
+set_config(bridge => 0);
+is_generated(
+    <<EOT
+auto eno1
+iface eno1 inet static
+	address 192.168.0.10/24
+	gateway 192.168.0.1
+
+iface eno2 inet manual
+EOT
+    , 'host network setup',
+);
+
+# 2.
+set_config(bridge => 1);
+is_generated(
+    <<EOT
+iface eno1 inet manual
+
+auto vmbr0
+iface vmbr0 inet static
+	bridge-ports eno1
+	bridge-stp off
+	bridge-fd 0
+	address 192.168.0.10/24
+	gateway 192.168.0.1
+
+iface eno2 inet manual
+EOT
+    , 'host network setup',
+);
+
+# interface name pinning test
+Proxmox::Install::Config::merge({
+    network_interface_pin_map => {
+        '12:34:56:ab:cd:ef' => 'nic0',
+        'ab:cd:ef:12:34:56' => 'nic1',
+    },
+});
+is_generated(
+    <<EOT
+iface nic0 inet manual
+
+auto vmbr0
+iface vmbr0 inet static
+	bridge-ports nic0
+	bridge-stp off
+	bridge-fd 0
+	address 192.168.0.10/24
+	gateway 192.168.0.1
+
+iface nic1 inet manual
+EOT
+    , 'management bridge uses pinned interface name',
+);
+Proxmox::Install::Config::merge({
+    network_interface_pin_map => undef,
+});
+
+# A gateway learned via IPv6 router advertisements is not written out
+Proxmox::Install::Config::merge({
+    mngmt_nic => 'eno1',
+    cidr => '2001:db8::1/64',
+    gateway => 'fe80::1',
+});
+$proxmox_install_runenv->redefine(
+    get => sub {
+        return {
+            network => {
+                interfaces => {
+                    eno1 => {
+                        mac => '12:34:56:ab:cd:ef',
+                        index => 1,
+                        name => 'eno1',
+                    },
+                    eno2 => {
+                        mac => 'ab:cd:ef:12:34:56',
+                        index => 2,
+                        name => 'eno2',
+                    },
+                },
+                routes => {
+                    gateways6 => [{ dev => 'eno1', gateway => 'fe80::1', protocol => 'ra' }],
+                },
+            },
+        };
+    },
+);
+
+is_generated(
+    <<EOT
+iface eno1 inet6 manual
+
+auto vmbr0
+iface vmbr0 inet6 static
+	bridge-ports eno1
+	bridge-stp off
+	bridge-fd 0
+	address 2001:db8::1/64
+
+iface eno2 inet6 manual
+EOT
+    , 'gateway learned from IPv6 router advertisements is not configured statically',
+);
+
+done_testing();
-- 
2.55.0