Fix bug that resulted in an attempt to rename bonds or vlans.

When cloud-init ran in the init stage (after networking had come up).
A bug could occur where cloud-init would attempt and fail to rename
network devices that had "inherited" mac addresses.

The intent of apply_network_config_names was always to rename only
the devices that were "physical" per the network config.  (This would
include veth devices in a container).  The bug was in creating
the dictionary of interfaces by mac address.  If there were multiple
interfaces with the same mac address then renames could fail.
This situation was guaranteed to occur with bonds or vlans or other
devices that inherit their mac.

The solution is to change get_interfaces_by_mac to skip interfaces
that have an inherited mac.

Also drop the 'devs' argument to get_interfaces_by_mac.  It was
non-obvious what the result should be if a device in the input
list was filtered out. ie should the following have an entry for
bond0 or not.  get_interfaces_by_mac(devs=['bond0'])

LP: #1669860

2 files changed, 135 insertions, 19 deletions

diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
index 1cf98ef5..346be5d3 100755..100644
--- a/cloudinit/net/__init__.py
+++ b/cloudinit/net/__init__.py
@@ -82,6 +82,10 @@ def is_wireless(devname):
     return os.path.exists(sys_dev_path(devname, "wireless"))
 
 
+def is_bridge(devname):
+    return os.path.exists(sys_dev_path(devname, "bridge"))
+
+
 def is_connected(devname):
     # is_connected isn't really as simple as that.  2 is
     # 'physically connected'. 3 is 'not connected'. but a wlan interface will
@@ -132,7 +136,7 @@ def generate_fallback_config():
     for interface in potential_interfaces:
         if interface.startswith("veth"):
             continue
-        if os.path.exists(sys_dev_path(interface, "bridge")):
+        if is_bridge(interface):
             # skip any bridges
             continue
         carrier = read_sys_net_int(interface, 'carrier')
@@ -187,7 +191,11 @@ def apply_network_config_names(netcfg, strict_present=True, strict_busy=True):
     """read the network config and rename devices accordingly.
     if strict_present is false, then do not raise exception if no devices
     match.  if strict_busy is false, then do not raise exception if the
-    device cannot be renamed because it is currently configured."""
+    device cannot be renamed because it is currently configured.
+
+    renames are only attempted for interfaces of type 'physical'.  It is
+    expected that the network system will create other devices with the
+    correct name in place."""
     renames = []
     for ent in netcfg.get('config', {}):
         if ent.get('type') != 'physical':
@@ -201,13 +209,35 @@ def apply_network_config_names(netcfg, strict_present=True, strict_busy=True):
     return _rename_interfaces(renames)
 
 
+def interface_has_own_mac(ifname, strict=False):
+    """return True if the provided interface has its own address.
+
+    Based on addr_assign_type in /sys.  Return true for any interface
+    that does not have a 'stolen' address. Examples of such devices
+    are bonds or vlans that inherit their mac from another device.
+    Possible values are:
+      0: permanent address    2: stolen from another device
+      1: randomly generated   3: set using dev_set_mac_address"""
+
+    assign_type = read_sys_net_int(ifname, "addr_assign_type")
+    if strict and assign_type is None:
+        raise ValueError("%s had no addr_assign_type.")
+    return assign_type in (0, 1, 3)
+
+
 def _get_current_rename_info(check_downable=True):
-    """Collect information necessary for rename_interfaces."""
-    names = get_devicelist()
+    """Collect information necessary for rename_interfaces.
+
+    returns a dictionary by mac address like:
+       {mac:
+         {'name': name
+          'up': boolean: is_up(name),
+          'downable': None or boolean indicating that the
+                      device has only automatically assigned ip addrs.}}
+    """
     bymac = {}
-    for n in names:
-        bymac[get_interface_mac(n)] = {
-            'name': n, 'up': is_up(n), 'downable': None}
+    for mac, name in get_interfaces_by_mac().items():
+        bymac[mac] = {'name': name, 'up': is_up(name), 'downable': None}
 
     if check_downable:
         nmatch = re.compile(r"[0-9]+:\s+(\w+)[@:]")
@@ -346,22 +376,32 @@ def get_interface_mac(ifname):
     return read_sys_net_safe(ifname, path)
 
 
-def get_interfaces_by_mac(devs=None):
-    """Build a dictionary of tuples {mac: name}"""
-    if devs is None:
-        try:
-            devs = get_devicelist()
-        except OSError as e:
-            if e.errno == errno.ENOENT:
-                devs = []
-            else:
-                raise
+def get_interfaces_by_mac():
+    """Build a dictionary of tuples {mac: name}.
+
+    Bridges and any devices that have a 'stolen' mac are excluded."""
+    try:
+        devs = get_devicelist()
+    except OSError as e:
+        if e.errno == errno.ENOENT:
+            devs = []
+        else:
+            raise
     ret = {}
     for name in devs:
+        if not interface_has_own_mac(name):
+            continue
+        if is_bridge(name):
+            continue
         mac = get_interface_mac(name)
         # some devices may not have a mac (tun0)
-        if mac:
-            ret[mac] = name
+        if not mac:
+            continue
+        if mac in ret:
+            raise RuntimeError(
+                "duplicate mac found! both '%s' and '%s' have mac '%s'" %
+                (name, ret[mac], mac))
+        ret[mac] = name
     return ret
 
 
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index bfd04ba0..9cc5e4ab 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -1461,6 +1461,82 @@ class TestNetRenderers(CiTestCase):
                           priority=['sysconfig', 'eni'])
 
 
+class TestGetInterfacesByMac(CiTestCase):
+    _data = {'devices': ['enp0s1', 'enp0s2', 'bond1', 'bridge1',
+                         'bridge1-nic', 'tun0'],
+             'bonds': ['bond1'],
+             'bridges': ['bridge1'],
+             'own_macs': ['enp0s1', 'enp0s2', 'bridge1-nic', 'bridge1'],
+             'macs': {'enp0s1': 'aa:aa:aa:aa:aa:01',
+                      'enp0s2': 'aa:aa:aa:aa:aa:02',
+                      'bond1': 'aa:aa:aa:aa:aa:01',
+                      'bridge1': 'aa:aa:aa:aa:aa:03',
+                      'bridge1-nic': 'aa:aa:aa:aa:aa:03',
+                      'tun0': None}}
+    data = {}
+
+    def _se_get_devicelist(self):
+        return self.data['devices']
+
+    def _se_get_interface_mac(self, name):
+        return self.data['macs'][name]
+
+    def _se_is_bridge(self, name):
+        return name in self.data['bridges']
+
+    def _se_interface_has_own_mac(self, name):
+        return name in self.data['own_macs']
+
+    def _mock_setup(self):
+        self.data = copy.deepcopy(self._data)
+        mocks = ('get_devicelist', 'get_interface_mac', 'is_bridge',
+                 'interface_has_own_mac')
+        self.mocks = {}
+        for n in mocks:
+            m = mock.patch('cloudinit.net.' + n,
+                           side_effect=getattr(self, '_se_' + n))
+            self.addCleanup(m.stop)
+            self.mocks[n] = m.start()
+
+    def test_raise_exception_on_duplicate_macs(self):
+        self._mock_setup()
+        self.data['macs']['bridge1-nic'] = self.data['macs']['enp0s1']
+        self.assertRaises(RuntimeError, net.get_interfaces_by_mac)
+
+    def test_excludes_any_without_mac_address(self):
+        self._mock_setup()
+        ret = net.get_interfaces_by_mac()
+        self.assertIn('tun0', self._se_get_devicelist())
+        self.assertNotIn('tun0', ret.values())
+
+    def test_excludes_stolen_macs(self):
+        self._mock_setup()
+        ret = net.get_interfaces_by_mac()
+        self.mocks['interface_has_own_mac'].assert_has_calls(
+            [mock.call('enp0s1'), mock.call('bond1')], any_order=True)
+        self.assertEqual(
+            {'aa:aa:aa:aa:aa:01': 'enp0s1', 'aa:aa:aa:aa:aa:02': 'enp0s2',
+             'aa:aa:aa:aa:aa:03': 'bridge1-nic'},
+            ret)
+
+    def test_excludes_bridges(self):
+        self._mock_setup()
+        # add a device 'b1', make all return they have their "own mac",
+        # set everything other than 'b1' to be a bridge.
+        # then expect b1 is the only thing left.
+        self.data['macs']['b1'] = 'aa:aa:aa:aa:aa:b1'
+        self.data['devices'].append('b1')
+        self.data['bonds'] = []
+        self.data['own_macs'] = self.data['devices']
+        self.data['bridges'] = [f for f in self.data['devices'] if f != "b1"]
+        ret = net.get_interfaces_by_mac()
+        self.assertEqual({'aa:aa:aa:aa:aa:b1': 'b1'}, ret)
+        self.mocks['is_bridge'].assert_has_calls(
+            [mock.call('bridge1'), mock.call('enp0s1'), mock.call('bond1'),
+             mock.call('b1')],
+            any_order=True)
+
+
 def _gzip_data(data):
     with io.BytesIO() as iobuf:
         gzfp = gzip.GzipFile(mode="wb", fileobj=iobuf)