3956c3
From 4ada8f2ec17d11835b44ab3d7786e5f3a732df41 Mon Sep 17 00:00:00 2001
3956c3
From: Scott Moser <smoser@brickies.net>
3956c3
Date: Fri, 31 Mar 2017 10:56:04 -0400
3956c3
Subject: [PATCH] Fix bug that resulted in an attempt to rename bonds or vlans.
3956c3
3956c3
When cloud-init ran in the init stage (after networking had come up).
3956c3
A bug could occur where cloud-init would attempt and fail to rename
3956c3
network devices that had "inherited" mac addresses.
3956c3
3956c3
The intent of apply_network_config_names was always to rename only
3956c3
the devices that were "physical" per the network config.  (This would
3956c3
include veth devices in a container).  The bug was in creating
3956c3
the dictionary of interfaces by mac address.  If there were multiple
3956c3
interfaces with the same mac address then renames could fail.
3956c3
This situation was guaranteed to occur with bonds or vlans or other
3956c3
devices that inherit their mac.
3956c3
3956c3
The solution is to change get_interfaces_by_mac to skip interfaces
3956c3
that have an inherited mac.
3956c3
3956c3
Also drop the 'devs' argument to get_interfaces_by_mac.  It was
3956c3
non-obvious what the result should be if a device in the input
3956c3
list was filtered out. ie should the following have an entry for
3956c3
bond0 or not.  get_interfaces_by_mac(devs=['bond0'])
3956c3
3956c3
LP: #1669860
3956c3
(cherry picked from commit bf7723e8092bb1f8a442aa2399dd870e130a27d9)
3956c3
3956c3
Resolves: rhbz#1512247
3956c3
Signed-off-by: Ryan McCabe <rmccabe@redhat.com>
3956c3
(cherry picked from commit f9e8f13f916fe740e46c9a0e9dd2dbb3cdb39975)
3956c3
---
3956c3
 cloudinit/net/__init__.py   | 78 ++++++++++++++++++++++++++++++++++-----------
3956c3
 tests/unittests/test_net.py | 76 +++++++++++++++++++++++++++++++++++++++++++
3956c3
 2 files changed, 135 insertions(+), 19 deletions(-)
3956c3
 mode change 100755 => 100644 cloudinit/net/__init__.py
3956c3
3956c3
diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
3956c3
old mode 100755
3956c3
new mode 100644
3956c3
index ea649cc2..ab7e8996
3956c3
--- a/cloudinit/net/__init__.py
3956c3
+++ b/cloudinit/net/__init__.py
3956c3
@@ -82,6 +82,10 @@ def is_wireless(devname):
3956c3
     return os.path.exists(sys_dev_path(devname, "wireless"))
3956c3
 
3956c3
 
3956c3
+def is_bridge(devname):
3956c3
+    return os.path.exists(sys_dev_path(devname, "bridge"))
3956c3
+
3956c3
+
3956c3
 def is_connected(devname):
3956c3
     # is_connected isn't really as simple as that.  2 is
3956c3
     # 'physically connected'. 3 is 'not connected'. but a wlan interface will
3956c3
@@ -132,7 +136,7 @@ def generate_fallback_config():
3956c3
     for interface in potential_interfaces:
3956c3
         if interface.startswith("veth"):
3956c3
             continue
3956c3
-        if os.path.exists(sys_dev_path(interface, "bridge")):
3956c3
+        if is_bridge(interface):
3956c3
             # skip any bridges
3956c3
             continue
3956c3
         carrier = read_sys_net_int(interface, 'carrier')
3956c3
@@ -187,7 +191,11 @@ def apply_network_config_names(netcfg, strict_present=True, strict_busy=True):
3956c3
     """read the network config and rename devices accordingly.
3956c3
     if strict_present is false, then do not raise exception if no devices
3956c3
     match.  if strict_busy is false, then do not raise exception if the
3956c3
-    device cannot be renamed because it is currently configured."""
3956c3
+    device cannot be renamed because it is currently configured.
3956c3
+
3956c3
+    renames are only attempted for interfaces of type 'physical'.  It is
3956c3
+    expected that the network system will create other devices with the
3956c3
+    correct name in place."""
3956c3
     renames = []
3956c3
     for ent in netcfg.get('config', {}):
3956c3
         if ent.get('type') != 'physical':
3956c3
@@ -201,13 +209,35 @@ def apply_network_config_names(netcfg, strict_present=True, strict_busy=True):
3956c3
     return _rename_interfaces(renames)
3956c3
 
3956c3
 
3956c3
+def interface_has_own_mac(ifname, strict=False):
3956c3
+    """return True if the provided interface has its own address.
3956c3
+
3956c3
+    Based on addr_assign_type in /sys.  Return true for any interface
3956c3
+    that does not have a 'stolen' address. Examples of such devices
3956c3
+    are bonds or vlans that inherit their mac from another device.
3956c3
+    Possible values are:
3956c3
+      0: permanent address    2: stolen from another device
3956c3
+      1: randomly generated   3: set using dev_set_mac_address"""
3956c3
+
3956c3
+    assign_type = read_sys_net_int(ifname, "addr_assign_type")
3956c3
+    if strict and assign_type is None:
3956c3
+        raise ValueError("%s had no addr_assign_type.")
3956c3
+    return assign_type in (0, 1, 3)
3956c3
+
3956c3
+
3956c3
 def _get_current_rename_info(check_downable=True):
3956c3
-    """Collect information necessary for rename_interfaces."""
3956c3
-    names = get_devicelist()
3956c3
+    """Collect information necessary for rename_interfaces.
3956c3
+
3956c3
+    returns a dictionary by mac address like:
3956c3
+       {mac:
3956c3
+         {'name': name
3956c3
+          'up': boolean: is_up(name),
3956c3
+          'downable': None or boolean indicating that the
3956c3
+                      device has only automatically assigned ip addrs.}}
3956c3
+    """
3956c3
     bymac = {}
3956c3
-    for n in names:
3956c3
-        bymac[get_interface_mac(n)] = {
3956c3
-            'name': n, 'up': is_up(n), 'downable': None}
3956c3
+    for mac, name in get_interfaces_by_mac().items():
3956c3
+        bymac[mac] = {'name': name, 'up': is_up(name), 'downable': None}
3956c3
 
3956c3
     if check_downable:
3956c3
         nmatch = re.compile(r"[0-9]+:\s+(\w+)[@:]")
3956c3
@@ -346,22 +376,32 @@ def get_interface_mac(ifname):
3956c3
     return read_sys_net_safe(ifname, path)
3956c3
 
3956c3
 
3956c3
-def get_interfaces_by_mac(devs=None):
3956c3
-    """Build a dictionary of tuples {mac: name}"""
3956c3
-    if devs is None:
3956c3
-        try:
3956c3
-            devs = get_devicelist()
3956c3
-        except OSError as e:
3956c3
-            if e.errno == errno.ENOENT:
3956c3
-                devs = []
3956c3
-            else:
3956c3
-                raise
3956c3
+def get_interfaces_by_mac():
3956c3
+    """Build a dictionary of tuples {mac: name}.
3956c3
+
3956c3
+    Bridges and any devices that have a 'stolen' mac are excluded."""
3956c3
+    try:
3956c3
+        devs = get_devicelist()
3956c3
+    except OSError as e:
3956c3
+        if e.errno == errno.ENOENT:
3956c3
+            devs = []
3956c3
+        else:
3956c3
+            raise
3956c3
     ret = {}
3956c3
     for name in devs:
3956c3
+        if not interface_has_own_mac(name):
3956c3
+            continue
3956c3
+        if is_bridge(name):
3956c3
+            continue
3956c3
         mac = get_interface_mac(name)
3956c3
         # some devices may not have a mac (tun0)
3956c3
-        if mac:
3956c3
-            ret[mac] = name
3956c3
+        if not mac:
3956c3
+            continue
3956c3
+        if mac in ret:
3956c3
+            raise RuntimeError(
3956c3
+                "duplicate mac found! both '%s' and '%s' have mac '%s'" %
3956c3
+                (name, ret[mac], mac))
3956c3
+        ret[mac] = name
3956c3
     return ret
3956c3
 
3956c3
 # vi: ts=4 expandtab
3956c3
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
3956c3
index 551370d4..cadaf596 100644
3956c3
--- a/tests/unittests/test_net.py
3956c3
+++ b/tests/unittests/test_net.py
3956c3
@@ -1173,6 +1173,82 @@ class TestEniRoundTrip(TestCase):
3956c3
             expected, [line for line in found if line])
3956c3
 
3956c3
 
3956c3
+class TestGetInterfacesByMac(TestCase):
3956c3
+    _data = {'devices': ['enp0s1', 'enp0s2', 'bond1', 'bridge1',
3956c3
+                         'bridge1-nic', 'tun0'],
3956c3
+             'bonds': ['bond1'],
3956c3
+             'bridges': ['bridge1'],
3956c3
+             'own_macs': ['enp0s1', 'enp0s2', 'bridge1-nic', 'bridge1'],
3956c3
+             'macs': {'enp0s1': 'aa:aa:aa:aa:aa:01',
3956c3
+                      'enp0s2': 'aa:aa:aa:aa:aa:02',
3956c3
+                      'bond1': 'aa:aa:aa:aa:aa:01',
3956c3
+                      'bridge1': 'aa:aa:aa:aa:aa:03',
3956c3
+                      'bridge1-nic': 'aa:aa:aa:aa:aa:03',
3956c3
+                      'tun0': None}}
3956c3
+    data = {}
3956c3
+
3956c3
+    def _se_get_devicelist(self):
3956c3
+        return self.data['devices']
3956c3
+
3956c3
+    def _se_get_interface_mac(self, name):
3956c3
+        return self.data['macs'][name]
3956c3
+
3956c3
+    def _se_is_bridge(self, name):
3956c3
+        return name in self.data['bridges']
3956c3
+
3956c3
+    def _se_interface_has_own_mac(self, name):
3956c3
+        return name in self.data['own_macs']
3956c3
+
3956c3
+    def _mock_setup(self):
3956c3
+        self.data = copy.deepcopy(self._data)
3956c3
+        mocks = ('get_devicelist', 'get_interface_mac', 'is_bridge',
3956c3
+                 'interface_has_own_mac')
3956c3
+        self.mocks = {}
3956c3
+        for n in mocks:
3956c3
+            m = mock.patch('cloudinit.net.' + n,
3956c3
+                           side_effect=getattr(self, '_se_' + n))
3956c3
+            self.addCleanup(m.stop)
3956c3
+            self.mocks[n] = m.start()
3956c3
+
3956c3
+    def test_raise_exception_on_duplicate_macs(self):
3956c3
+        self._mock_setup()
3956c3
+        self.data['macs']['bridge1-nic'] = self.data['macs']['enp0s1']
3956c3
+        self.assertRaises(RuntimeError, net.get_interfaces_by_mac)
3956c3
+
3956c3
+    def test_excludes_any_without_mac_address(self):
3956c3
+        self._mock_setup()
3956c3
+        ret = net.get_interfaces_by_mac()
3956c3
+        self.assertIn('tun0', self._se_get_devicelist())
3956c3
+        self.assertNotIn('tun0', ret.values())
3956c3
+
3956c3
+    def test_excludes_stolen_macs(self):
3956c3
+        self._mock_setup()
3956c3
+        ret = net.get_interfaces_by_mac()
3956c3
+        self.mocks['interface_has_own_mac'].assert_has_calls(
3956c3
+            [mock.call('enp0s1'), mock.call('bond1')], any_order=True)
3956c3
+        self.assertEqual(
3956c3
+            {'aa:aa:aa:aa:aa:01': 'enp0s1', 'aa:aa:aa:aa:aa:02': 'enp0s2',
3956c3
+             'aa:aa:aa:aa:aa:03': 'bridge1-nic'},
3956c3
+            ret)
3956c3
+
3956c3
+    def test_excludes_bridges(self):
3956c3
+        self._mock_setup()
3956c3
+        # add a device 'b1', make all return they have their "own mac",
3956c3
+        # set everything other than 'b1' to be a bridge.
3956c3
+        # then expect b1 is the only thing left.
3956c3
+        self.data['macs']['b1'] = 'aa:aa:aa:aa:aa:b1'
3956c3
+        self.data['devices'].append('b1')
3956c3
+        self.data['bonds'] = []
3956c3
+        self.data['own_macs'] = self.data['devices']
3956c3
+        self.data['bridges'] = [f for f in self.data['devices'] if f != "b1"]
3956c3
+        ret = net.get_interfaces_by_mac()
3956c3
+        self.assertEqual({'aa:aa:aa:aa:aa:b1': 'b1'}, ret)
3956c3
+        self.mocks['is_bridge'].assert_has_calls(
3956c3
+            [mock.call('bridge1'), mock.call('enp0s1'), mock.call('bond1'),
3956c3
+             mock.call('b1')],
3956c3
+            any_order=True)
3956c3
+
3956c3
+
3956c3
 def _gzip_data(data):
3956c3
     with io.BytesIO() as iobuf:
3956c3
         gzfp = gzip.GzipFile(mode="wb", fileobj=iobuf)
3956c3
-- 
3956c3
2.14.3
3956c3