[PATCH installer v5 07/23] install: network: set up management bond if requested

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

Adds the necessary configuration to /etc/network/interfaces when bonding
is enabled in the installation config.

A bond named 'bond0' will be created, with the default bridge (as
before) 'vmbr0' on top of that. Without bridged networking, the address
is configured on the bond directly.

The bond options are written out in the same order and with the same
defaults the Proxmox VE network API uses, so that a later edit through
the web interface does not cause unrelated changes.

For the same reason, the members are sorted by name and marked auto.
On Proxmox VE, their order also matters on its own: ifupdown2 enslaves
them in the configured order and, as Proxmox VE disables the persistent
MAC address policy of systemd, the kernel assigns the MAC address of
the first one to the bond. If the first save through the web interface
reordered them, the bond would come up with a different MAC address on
the next boot, which networks filtering on MAC addresses may block.

Originally-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v4 -> v5:
  * no changes

Changes v3 -> v4:
  * no changes

Changes v2 -> v3:
  * use suggested patch from @Thomas, adapted tests

Changes v1 -> v2:
  * no changes

 Proxmox/Install.pm                | 102 +++++++++++++--
 test/generate-ifupdown2-config.pl | 207 +++++++++++++++++++++++++++---
 2 files changed, 280 insertions(+), 29 deletions(-)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index 6c7af3d..9e167d8 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -819,35 +819,107 @@ sub generate_ifupdown2_config {
 
     my $ifaces = "auto lo\niface lo inet loopback\n\n";
 
+    my $get_link_name = sub {
+        my ($mac) = @_;
+        $mac = lc($mac);
+
+        return $netif_pin_map->{$mac}
+            if defined($netif_pin_map) && defined($netif_pin_map->{$mac});
+
+        my ($if) = grep { $_->{mac} eq $mac } values $interfaces->%*;
+        die "no network interface with MAC address '$mac' found for the bond\n"
+            if !defined($if);
+        return $if->{name};
+    };
+
+    # the physical links carrying the management network
+    my @mngmt_links = ($ethdev);
+
+    my $bond_enabled = Proxmox::Install::Config::get_mngmt_bond_enabled();
+    my $bond_config = '';
+    if ($bond_enabled) {
+        my $bond_opts = Proxmox::Install::Config::get_mngmt_bond_opt();
+        my $mode = $bond_opts->{mode};
+
+        # When setting up bridged networking, the bond is attached to the main bridge as port.
+        # Otherwise, the bond represents the main management interface, i.e. with the address
+        # configured directly on it.
+
+        $ethdev = 'bond0';
+        my $addr_method = $iso_env->{cfg}->{bridged_network} ? 'manual' : 'static';
+
+        # translate all bond members MACs to their final interface name, either pinned or not
+        my @members;
+        for my $mac ($bond_opts->{members}->@*) {
+            $mac = lc($mac); # normalize into lowercase
+
+            if (defined($netif_pin_map) && defined($netif_pin_map->{$mac})) {
+                push @members, $netif_pin_map->{$mac};
+            } else {
+                my ($if) = grep { $_->{mac} eq $mac } values %$interfaces;
+                push @members, $if->{name};
+            }
+        }
+
+        # sorted as Proxmox VE saves them, where the first one enslaved sets the bond's MAC address
+        @mngmt_links = sort map { $get_link_name->($_) } $bond_opts->{members}->@*;
+
+        # same order and defaults as the Proxmox VE network API writes them
+        $bond_config = "\tbond-slaves " . join(' ', @mngmt_links) . "\n";
+        $bond_config .= "\tbond-miimon 100\n";
+        $bond_config .= "\tbond-mode $mode\n";
+        $bond_config .= "\tbond-xmit-hash-policy $bond_opts->{hash_policy}\n"
+            if $mode eq '802.3ad' || $mode eq 'balance-xor';
+        $bond_config .= "\tbond-primary " . $get_link_name->($bond_opts->{primary_interface}) . "\n"
+            if $mode eq 'active-backup' && defined($bond_opts->{primary_interface});
+    }
+
     # iproute2 omits the gateway and protocol keys for some routes; route devices are original
-    # kernel interface names, while $ethdev can be the pinned one
+    # kernel interface names, while the management links can be pinned ones
+    my %is_mngmt_link = map { $_ => 1 } @mngmt_links;
+
     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
+        $is_mngmt_link{$dev}
             && ($_->{gateway} // '') eq $gateway
             && ($_->{protocol} // '') eq 'ra';
     } $gateways6->@*;
 
     if ($iso_env->{cfg}->{bridged_network}) {
-        $ifaces .= "iface $ethdev $ntype manual\n";
+        if ($bond_enabled) {
+            $ifaces .= "auto $ethdev\n" #
+                . "iface $ethdev $ntype manual\n" #
+                . $bond_config;
+        } else {
+            $ifaces .= "iface $ethdev $ntype manual\n";
+        }
 
-        $ifaces .=
-            "\nauto vmbr0\niface vmbr0 $ntype static\n"
-            . "\tbridge-ports $ethdev\n"
-            . "\tbridge-stp off\n"
+        $ifaces .= "\nauto vmbr0\niface vmbr0 $ntype static\n";
+
+        $ifaces .= "\taddress $cidr\n";
+        if (!$is_gateway6_from_ra) {
+            $ifaces .= "\tgateway $gateway\n";
+        }
+
+        $ifaces .= "\tbridge-ports $ethdev\n" #
+            . "\tbridge-stp off\n" #
             . "\tbridge-fd 0\n";
     } else {
-        $ifaces .= "auto $ethdev\n" . #
-            "iface $ethdev $ntype static\n";
-    }
+        # if bonding is enabled, this becomes the bond interface stanza
+        # otherwise, it's the physical interface stanza
+        $ifaces .= "auto $ethdev\n" #
+            . "iface $ethdev $ntype static\n";
 
-    $ifaces .= "\taddress $cidr\n";
-    if (!$is_gateway6_from_ra) {
-        $ifaces .= "\tgateway $gateway\n";
+        $ifaces .= "\taddress $cidr\n";
+        if (!$is_gateway6_from_ra) {
+            $ifaces .= "\tgateway $gateway\n";
+        }
+
+        $ifaces .= $bond_config;
     }
 
     foreach my $iface (sort keys $interfaces->%*) {
@@ -860,7 +932,9 @@ sub generate_ifupdown2_config {
 
         next if $name eq $ethdev;
 
-        $ifaces .= "\niface $name $ntype manual\n";
+        # as Proxmox VE writes them, ifupdown2 then also keeps them up if removed from the bond
+        my $auto = $bond_enabled && $is_mngmt_link{$name} ? "auto $name\n" : '';
+        $ifaces .= "\n${auto}iface $name $ntype manual\n";
     }
 
     $ifaces .= "\n\nsource /etc/network/interfaces.d/*\n";
diff --git a/test/generate-ifupdown2-config.pl b/test/generate-ifupdown2-config.pl
index 7f92e4f..f40689c 100755
--- a/test/generate-ifupdown2-config.pl
+++ b/test/generate-ifupdown2-config.pl
@@ -31,6 +31,15 @@ my sub set_config {
             };
         },
     );
+
+    Proxmox::Install::Config::merge({
+        mngmt_nic => $cfg{bond} ? 'bond0' : 'eno1',
+        mngmt_bond => {
+            members => $cfg{bond} ? ['12:34:56:ab:cd:ef', 'ab:cd:ef:12:34:56'] : [],
+            mode => 'active-backup',
+            hash_policy => 'layer2',
+        },
+    });
 }
 
 my sub is_generated {
@@ -45,7 +54,7 @@ $expected
 
 source /etc/network/interfaces.d/*
 EOT
-        "$msg (bridge:$current_cfg->{bridge})",
+        "$msg (bridge:$current_cfg->{bridge} bond:$current_cfg->{bond})",
     );
 }
 
@@ -75,9 +84,8 @@ $proxmox_install_runenv->redefine(
         };
     },
 );
-set_config(bridge => 0);
+set_config(bridge => 0, bond => 0);
 Proxmox::Install::Config::merge({
-    mngmt_nic => 'eno1',
     cidr => '192.168.0.10/24',
     gateway => '192.168.0.1',
 });
@@ -85,16 +93,18 @@ Proxmox::Install::Config::merge({
 ###
 # There's essentially a matrix to test w.r.t the overall networking setup:
 #
-# +-------+--------+
-# | test# | bridge |
-# +=======+========+
-# |   1   |    0   |
-# |   2   |    1   |
-# +-------+--------+
+# +-------+--------+------+
+# | test# | bridge | bond |
+# +=======+========+======+
+# |   1   |    0   |  0   |
+# |   2   |    0   |  1   |
+# |   3   |    1   |  0   |
+# |   4   |    1   |  1   |
+# +-------+--------+------+
 ###
 
 # 1.
-set_config(bridge => 0);
+set_config(bridge => 0, bond => 0);
 is_generated(
     <<EOT
 auto eno1
@@ -108,19 +118,67 @@ EOT
 );
 
 # 2.
-set_config(bridge => 1);
+set_config(bridge => 0, bond => 1);
+is_generated(
+    <<EOT
+auto bond0
+iface bond0 inet static
+	address 192.168.0.10/24
+	gateway 192.168.0.1
+	bond-slaves eno1 eno2
+	bond-miimon 100
+	bond-mode active-backup
+
+auto eno1
+iface eno1 inet manual
+
+auto eno2
+iface eno2 inet manual
+EOT
+    , 'host network setup',
+);
+
+# 3.
+set_config(bridge => 1, bond => 0);
 is_generated(
     <<EOT
 iface eno1 inet manual
 
 auto vmbr0
 iface vmbr0 inet static
+	address 192.168.0.10/24
+	gateway 192.168.0.1
 	bridge-ports eno1
 	bridge-stp off
 	bridge-fd 0
+
+iface eno2 inet manual
+EOT
+    , 'host network setup',
+);
+
+# 4.
+set_config(bridge => 1, bond => 1);
+is_generated(
+    <<EOT
+auto bond0
+iface bond0 inet manual
+	bond-slaves eno1 eno2
+	bond-miimon 100
+	bond-mode active-backup
+
+auto vmbr0
+iface vmbr0 inet static
 	address 192.168.0.10/24
 	gateway 192.168.0.1
+	bridge-ports bond0
+	bridge-stp off
+	bridge-fd 0
 
+auto eno1
+iface eno1 inet manual
+
+auto eno2
 iface eno2 inet manual
 EOT
     , 'host network setup',
@@ -131,17 +189,18 @@ Proxmox::Install::Config::merge({
     mngmt_nic => 'eno1',
     network_interface_pin_map => $pin_map,
 });
+set_config(bridge => 1, bond => 0);
 is_generated(
     <<EOT
 iface nic0 inet manual
 
 auto vmbr0
 iface vmbr0 inet static
+	address 192.168.0.10/24
+	gateway 192.168.0.1
 	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
@@ -180,6 +239,7 @@ $proxmox_install_runenv->redefine(
         };
     },
 );
+set_config(bridge => 1, bond => 0);
 
 is_generated(
     <<EOT
@@ -187,23 +247,49 @@ iface eno1 inet6 manual
 
 auto vmbr0
 iface vmbr0 inet6 static
+	address 2001:db8::1/64
 	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',
 );
 
+# A gateway learned via IPv6 router advertisements is not written out for a bond
+Proxmox::Install::Config::merge({
+    mngmt_nic => 'bond0',
+    cidr => '2001:db8::1/64',
+    gateway => 'fe80::1',
+});
+set_config(bridge => 0, bond => 1);
+
+is_generated(
+    <<EOT
+auto bond0
+iface bond0 inet6 static
+	address 2001:db8::1/64
+	bond-slaves eno1 eno2
+	bond-miimon 100
+	bond-mode active-backup
+
+auto eno1
+iface eno1 inet6 manual
+
+auto eno2
+iface eno2 inet6 manual
+EOT
+    , 'gateway learned from IPv6 router advertisements is not configured statically',
+);
+
 # Check that unpinnable interfaces (e.g. virtual ones) keep their kernel-assigned name
 Proxmox::Install::Config::merge({
     network_interface_pin_map => $pin_map,
     cidr => '192.168.0.10/24',
     gateway => '192.168.0.1',
 });
-set_config(bridge => 0);
+set_config(bridge => 0, bond => 0);
 $proxmox_install_runenv->redefine(
     get => sub {
         return {
@@ -244,4 +330,95 @@ EOT
     , 'unpinnable names keep their kernel name',
 );
 
+set_config(bridge => 1, bond => 1);
+Proxmox::Install::Config::merge({
+    mngmt_nic => 'bond0',
+    mngmt_bond => {
+        members => ['12:34:56:ab:cd:ef', 'ab:cd:ef:12:34:56'],
+        mode => 'active-backup',
+        hash_policy => 'layer2',
+        primary_interface => 'ab:cd:ef:12:34:56',
+    },
+});
+is_generated(
+    <<EOT
+auto bond0
+iface bond0 inet manual
+	bond-slaves nic0 nic1
+	bond-miimon 100
+	bond-mode active-backup
+	bond-primary nic1
+
+auto vmbr0
+iface vmbr0 inet static
+	address 192.168.0.10/24
+	gateway 192.168.0.1
+	bridge-ports bond0
+	bridge-stp off
+	bridge-fd 0
+
+iface dummy inet manual
+
+auto nic0
+iface nic0 inet manual
+
+auto nic1
+iface nic1 inet manual
+EOT
+    , 'active-backup bond with interface pinning enabled and primary interface set',
+);
+
+Proxmox::Install::Config::merge({
+    network_interface_pin_map => undef,
+    mngmt_bond => {
+        members => ['12:34:56:ab:cd:ef', 'ab:cd:ef:12:34:56'],
+        mode => '802.3ad',
+        hash_policy => 'layer3+4',
+        # will be ignored on 802.3ad
+        primary_interface => 'ab:cd:ef:12:34:56',
+    },
+});
+is_generated(
+    <<EOT
+auto bond0
+iface bond0 inet manual
+	bond-slaves eno1 eno2
+	bond-miimon 100
+	bond-mode 802.3ad
+	bond-xmit-hash-policy layer3+4
+
+auto vmbr0
+iface vmbr0 inet static
+	address 192.168.0.10/24
+	gateway 192.168.0.1
+	bridge-ports bond0
+	bridge-stp off
+	bridge-fd 0
+
+iface dummy inet manual
+
+auto eno1
+iface eno1 inet manual
+
+auto eno2
+iface eno2 inet manual
+EOT
+    , 'LACP bond enabled, with hash policy and no primary interface',
+);
+
+Proxmox::Install::Config::merge({
+    mngmt_bond => {
+        members => ['12:34:56:ab:cd:ff', 'ab:cd:ef:12:34:56'],
+        mode => 'active-backup',
+        hash_policy => 'layer2',
+        primary_interface => undef,
+    },
+});
+eval { is_generated(); };
+is(
+    $@,
+    "no network interface with MAC address '12:34:56:ab:cd:ff' found for the bond\n",
+    'unknown bond member is an error',
+);
+
 done_testing();
-- 
2.56.0