Fix rename of matched interfaces at runtime (LP: #1904662) (#174)

Renaming of network interfaces was not working when running netplan apply and interfaces would only be renamed after a reboot via udev. There is some udev-rename logic inside cli/commands/apply.py, which doesn't seem to be working, though.

This seems to be related to systemd-udevd's NamePolicy=keep default, which is set explicitly as of systemd v240 [0][1] and prevents renaming of interfaces via udev, if the name was set once (i.e. during boot). Even before v240 this has be the (implicit) default, so I'm not sure if this rename logic has ever worked...

I reworked/refactored the renaming of interfaces in apply.py to make use of iproute2 ip link set eth_OLD set name eth_NEW command instead and also added an integration test.

In order to rename interfaces via iproute2, they need to be DOWN, therefore the new code is bringing all to-be-renamed interfaces - which are not defined to be critical - down during apply and renames them accordingly.

Also, there was another issue with the detection of physical interfaces, where the system's current network interface names (from netifaces.interfaces()) have been compared to config_manager.py's physical_interfaces list, which are not (always) actual interface names (but netplan IDs), especially for netplan definitions with a match condition. So I added another util to lookup/match the actual interface name for a given match: condition.

[0] https://www.freedesktop.org/software/systemd/man/systemd.net-naming-scheme.html
[1] https://github.com/systemd/systemd/blob/e62a7fea757f259eb330da5b6d3ab4ede46400a2/NEWS#L25

Commits:
* WIP: rename down interfaces on apply
* cli: improve rename of interfaces at runtime, during 'netplan apply'
* cli: fix linter
* WIP: refactor interface rename code
udevadm test-builtin net_setup_link seems to be only running once (at
boot time) and _not_ rename interfaces during 'netplan apply'
* cli: keep udev link changes like WakeOnLan=
* cli:utils: some cleanup
* cli: big cleanup + unit-tests
* tests: set-name on apply integration test
* cli: update comment about NamePolicy=keep rename

Author: slyon

doc/netplan.md

@@ -275,9 +275,9 @@ Virtual devices
 
 ``critical`` (bool)
 
-:   (networkd backend only) Designate the connection as "critical to the
-    system", meaning that special care will be taken by systemd-networkd to
-    not release the assigned IP when the daemon is restarted.
+:   Designate the connection as "critical to the system", meaning that special
+    care will be taken by to not release the assigned IP when the daemon is
+    restarted. (not recognized by NetworkManager)
 
 ``dhcp-identifier`` (scalar)
 

netplan/cli/commands/apply.py

@@ -187,6 +187,10 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True):  # p
         changes = NetplanApply.process_link_changes(devices, config_manager)
 
         # if the interface is up, we can still apply some .link file changes
+        # but we cannot apply the interface rename via udev, as it won't touch
+        # the interface name, if it was already renamed once (e.g. during boot),
+        # because of the NamePolicy=keep default:
+        # https://www.freedesktop.org/software/systemd/man/systemd.net-naming-scheme.html
         devices = netifaces.interfaces()
         for device in devices:
             logging.debug('netplan triggering .link rules for %s', device)
@@ -199,9 +203,15 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True):  # p
             except subprocess.CalledProcessError:
                 logging.debug('Ignoring device without syspath: %s', device)
 
-        # apply renames to "down" devices
+        # apply some more changes manually
         for iface, settings in changes.items():
+            # rename non-critical network interfaces
             if settings.get('name'):
+                # bring down the interface, using its current (matched) interface name
+                subprocess.check_call(['ip', 'link', 'set', 'dev', iface, 'down'],
+                                      stdout=subprocess.DEVNULL,
+                                      stderr=subprocess.DEVNULL)
+                # rename the interface to the name given via 'set-name'
                 subprocess.check_call(['ip', 'link', 'set',
                                        'dev', iface,
                                        'name', settings.get('name')],
@@ -234,7 +244,7 @@ def is_composite_member(composites, phy):  # pragma: nocover (covered in autopkg
         interface? (bond, bridge)
         """
         for composite in composites:
-            for id, settings in composite.items():
+            for _, settings in composite.items():
                 members = settings.get('interfaces', [])
                 for iface in members:
                     if iface == phy:
@@ -245,68 +255,49 @@ def is_composite_member(composites, phy):  # pragma: nocover (covered in autopkg
     @staticmethod
     def process_link_changes(interfaces, config_manager):  # pragma: nocover (covered in autopkgtest)
         """
-        Go through the pending changes and pick what needs special
-        handling. Only applies to "down" interfaces which can be safely
-        updated.
+        Go through the pending changes and pick what needs special handling.
+        Only applies to non-critical interfaces which can be safely updated.
         """
 
         changes = {}
         phys = dict(config_manager.physical_interfaces)
         composite_interfaces = [config_manager.bridges, config_manager.bonds]
 
-        # TODO (cyphermox): factor out some of this matching code (and make it
-        # pretty) in its own module.
-        matches = {'by-driver': {},
-                   'by-mac': {},
-                   }
+        # Find physical interfaces which need a rename
+        # But do not rename virtual interfaces
         for phy, settings in phys.items():
-            if not settings:
-                continue
-            if phy == 'renderer':
-                continue
+            if not settings or not isinstance(settings, dict):
+                continue  # Skip special values, like "renderer: ..."
             newname = settings.get('set-name')
             if not newname:
-                continue
+                continue  # Skip if no new name needs to be set
             match = settings.get('match')
             if not match:
-                continue
-            driver = match.get('driver')
-            mac = match.get('macaddress')
-            if driver:
-                matches['by-driver'][driver] = newname
-            if mac:
-                matches['by-mac'][mac] = newname
-
-        # /sys/class/net/ens3/device -> ../../../virtio0
-        # /sys/class/net/ens3/device/driver -> ../../../../bus/virtio/drivers/virtio_net
-        for interface in interfaces:
-            if interface not in phys:
-                # do not rename  virtual devices
-                logging.debug('Skipping non-physical interface: %s', interface)
-                continue
-            if NetplanApply.is_composite_member(composite_interfaces, interface):
-                logging.debug('Skipping composite member %s', interface)
+                continue  # Skip if no match for current name is given
+            if NetplanApply.is_composite_member(composite_interfaces, phy):
+                logging.debug('Skipping composite member {}'.format(phy))
                 # do not rename members of virtual devices. MAC addresses
                 # may be the same for all interface members.
                 continue
-
-            driver_name = utils.get_interface_driver_name(interface, only_down=True)
-            if not driver_name:
-                # don't allow up interfaces to match by mac
+            # Find current name of the interface, according to match conditions and globs (name, mac, driver)
+            current_iface_name = utils.find_matching_iface(interfaces, match)
+            if not current_iface_name:
+                logging.warning('Cannot find unique matching interface for {}: {}'.format(phy, match))
+                continue
+            if current_iface_name == newname:
+                # Skip interface if it already has the correct name
+                logging.debug('Skipping correctly named interface: {}'.format(newname))
                 continue
-            macaddress = utils.get_interface_macaddress(interface)
-            if driver_name in matches['by-driver']:
-                new_name = matches['by-driver'][driver_name]
-                logging.debug(new_name)
-                logging.debug(interface)
-                if new_name != interface:
-                    changes.update({interface: {'name': new_name}})
-            if macaddress in matches['by-mac']:
-                new_name = matches['by-mac'][macaddress]
-                if new_name != interface:
-                    changes.update({interface: {'name': new_name}})
-
-        logging.debug(changes)
+            if settings.get('critical', False):
+                # Skip interfaces defined as critical, as we should not take them down in order to rename
+                logging.warning('Cannot rename {} ({} -> {}) at runtime (needs reboot), due to being critical'
+                                .format(phy, current_iface_name, newname))
+                continue
+
+            # record the interface rename change
+            changes[current_iface_name] = {'name': newname}
+
+        logging.debug('Link changes: {}'.format(changes))
         return changes
 
     @staticmethod

netplan/cli/utils.py

@@ -3,6 +3,7 @@
 # Copyright (C) 2018-2020 Canonical, Ltd.
 # Author: Mathieu Trudel-Lapierre <[email protected]>
 # Author: Łukasz 'sil2100' Zemczak <[email protected]>
+# Author: Lukas 'slyon' Märdian <[email protected]>
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -166,24 +167,46 @@ def get_interface_driver_name(interface, only_down=False):  # pragma: nocover (c
 
 def get_interface_macaddress(interface):  # pragma: nocover (covered in autopkgtest)
     link = netifaces.ifaddresses(interface)[netifaces.AF_LINK][0]
-
     return link.get('addr')
 
 
-def is_interface_matching_name(interface, match_driver):
-    return fnmatch.fnmatchcase(interface, match_driver)
+def is_interface_matching_name(interface, match_name):
+    # globs are supported
+    return fnmatch.fnmatchcase(interface, match_name)
 
 
 def is_interface_matching_driver_name(interface, match_driver):
     driver_name = get_interface_driver_name(interface)
-
-    return match_driver == driver_name
+    # globs are supported
+    return fnmatch.fnmatchcase(driver_name, match_driver)
 
 
 def is_interface_matching_macaddress(interface, match_mac):
     macaddress = get_interface_macaddress(interface)
+    # exact, case insensitive match. globs are not supported
+    return match_mac.lower() == macaddress.lower()
+
+
+def find_matching_iface(interfaces, match):
+    assert isinstance(match, dict)
 
-    return match_mac == macaddress
+    # Filter for match.name glob, fallback to '*'
+    name_glob = match.get('name') if match.get('name', False) else '*'
+    matches = fnmatch.filter(interfaces, name_glob)
+
+    # Filter for match.macaddress (exact match)
+    if len(matches) > 1 and match.get('macaddress'):
+        matches = list(filter(lambda iface: is_interface_matching_macaddress(iface, match.get('macaddress')), matches))
+
+    # Filter for match.driver glob
+    if len(matches) > 1 and match.get('driver'):
+        matches = list(filter(lambda iface: is_interface_matching_driver_name(iface, match.get('driver')), matches))
+
+    # Return current name of unique matched interface, if available
+    if len(matches) != 1:
+        logging.info(matches)
+        return None
+    return matches[0]
 
 
 class NetplanCommand(argparse.Namespace):

tests/integration/ethernets.py

@@ -240,6 +240,29 @@ def test_link_local_all(self):
         # Verify IPv4 and IPv6 link local addresses are there
         self.assert_iface(self.dev_e_client, ['inet6 fe80:', 'inet 169.254.'])
 
+    def test_rename_interfaces(self):
+        self.setup_eth(None)
+        with open(self.config, 'w') as f:
+            f.write('''network:
+  renderer: %(r)s
+  ethernets:
+    idx:
+      match:
+        name: %(ec)s
+      set-name: iface1
+      addresses: [10.10.10.11/24]
+    idy:
+      match:
+        macaddress: %(e2c_mac)s
+      set-name: iface2
+      addresses: [10.10.10.22/24]
+''' % {'r': self.backend, 'ec': self.dev_e_client, 'e2c_mac': self.dev_e2_client_mac})
+        self.generate_and_settle()
+        self.assert_iface('iface1', ['inet 10.10.10.11'])
+        self.assert_iface_up('iface1', ['inet 10.10.10.11'])
+        self.assert_iface('iface2', ['inet 10.10.10.22'])
+        self.assert_iface_up('iface2', ['inet 10.10.10.22'])
+
 
 @unittest.skipIf("networkd" not in test_backends,
                      "skipping as networkd backend tests are disabled")

tests/test_utils.py

@@ -20,6 +20,8 @@
 import tempfile
 import glob
 
+from unittest.mock import patch
+
 import netplan.cli.utils as utils
 
 
@@ -71,3 +73,26 @@ def test_nm_interfaces_globbing2(self):
         self.assertTrue('ens3' in ifaces)
         self.assertTrue('ens4' in ifaces)
         self.assertTrue(len(ifaces) == 4)
+
+    def test_find_matching_iface_too_many(self):
+        # too many matches
+        iface = utils.find_matching_iface(DEVICES, {'name': 'e*'})
+        self.assertEqual(iface, None)
+
+    @patch('netplan.cli.utils.get_interface_macaddress')
+    def test_find_matching_iface(self, gim):
+        # we mock-out get_interface_macaddress to return useful values for the test
+        gim.side_effect = lambda x: '00:01:02:03:04:05' if x == 'eth1' else '00:00:00:00:00:00'
+
+        match = {'name': 'e*', 'macaddress': '00:01:02:03:04:05'}
+        iface = utils.find_matching_iface(DEVICES, match)
+        self.assertEqual(iface, 'eth1')
+
+    @patch('netplan.cli.utils.get_interface_driver_name')
+    def test_find_matching_iface_name_and_driver(self, gidn):
+        # we mock-out get_interface_driver_name to return useful values for the test
+        gidn.side_effect = lambda x: 'foo' if x == 'ens4' else 'bar'
+
+        match = {'name': 'ens?', 'driver': 'f*'}
+        iface = utils.find_matching_iface(DEVICES, match)
+        self.assertEqual(iface, 'ens4')