Merge ~smoser/cloud-init:bug/1692028-empty-macs-for-bymac into cloud-init:master

[Scott Moser]

Fix get_interfaces_by_mac for empty macs

Some interfaces (greptap0 in the bug) have a mac address of
'00:00:00:00:00:00'. That was causing a duplicate mac detection
as the 'lo' device also has that mac.

The change here is to just ignore macs other than 'lo' that have that.

LP: #1692028

[Scott Moser (smoser)]

I'm not sure where this 'gretap0' device comes from or if it would ever appear in the network data. I'd suggest that it would be bad for it to...

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:2cb58ff6eb4d267b6f63ec6a983cc27d7a6bc187
https://jenkins.ubuntu.com/server/job/cloud-init-ci/360/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/360/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/360/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/360/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/360/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/360/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/360/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:686020a02e0835fc808a500320664fe9199923a6
https://jenkins.ubuntu.com/server/job/cloud-init-ci/361/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/361
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/361
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/361
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/361
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/361

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/361/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

GRE tap is an overlay network, I believe it's supported in openstack;
Google also found some AWS setup instructions mentioning gretap's

https://aws.amazon.com/articles/6234671078671125

Patch looks good to me.

review: Approve

[Scott Moser (smoser)]

Yeah, my question really was if we'd ever be expected to configure this device, or if its already configured...

Ie, i want to see network_json for this. It seems like there will be issues further down the road if we simply ignore the device in get_interfaces_by_mac but we're supposed to write a network configuration for it.

James, could you provide the config drive contents on the system you saw this on ?

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:2c0655feb9a194b5fbdfe90a5f847c16f1e15409
https://jenkins.ubuntu.com/server/job/cloud-init-ci/371/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/371
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/371
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/371
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/371
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/371

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/371/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

As ryan has approved, and we've opened nova-lxd/lxd bugs at
 https://bugs.launchpad.net/cloud-archive/+bug/1692545
and
 https://github.com/lxc/lxd/issues/3338

I'm going to pull the change.

review: Approve

diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
index a072a8d..8c6cd05 100644
--- a/cloudinit/net/__init__.py
+++ b/cloudinit/net/__init__.py
@@ -393,6 +393,7 @@ def get_interfaces_by_mac():
         else:
             raise
     ret = {}
+    empty_mac = '00:00:00:00:00:00'
     for name in devs:
         if not interface_has_own_mac(name):
             continue
@@ -404,6 +405,8 @@ def get_interfaces_by_mac():
         # some devices may not have a mac (tun0)
         if not mac:
             continue
+        if mac == empty_mac and name != 'lo':
+            continue
         if mac in ret:
             raise RuntimeError(
                 "duplicate mac found! both '%s' and '%s' have mac '%s'" %
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index 68a0157..8bd3f43 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -1542,24 +1542,24 @@ class TestNetRenderers(CiTestCase):
 
 
 class TestGetInterfacesByMac(CiTestCase):
-    _data = {'devices': ['enp0s1', 'enp0s2', 'bond1', 'bridge1',
-                         'bridge1-nic', 'tun0', 'bond1.101'],
-             'bonds': ['bond1'],
+    _data = {'bonds': ['bond1'],
              'bridges': ['bridge1'],
              'vlans': ['bond1.101'],
              'own_macs': ['enp0s1', 'enp0s2', 'bridge1-nic', 'bridge1',
-                          'bond1.101'],
+                          'bond1.101', 'lo'],
              'macs': {'enp0s1': 'aa:aa:aa:aa:aa:01',
                       'enp0s2': 'aa:aa:aa:aa:aa:02',
                       'bond1': 'aa:aa:aa:aa:aa:01',
                       'bond1.101': 'aa:aa:aa:aa:aa:01',
                       'bridge1': 'aa:aa:aa:aa:aa:03',
                       'bridge1-nic': 'aa:aa:aa:aa:aa:03',
+                      'lo': '00:00:00:00:00:00',
+                      'greptap0': '00:00:00:00:00:00',
                       'tun0': None}}
     data = {}
 
     def _se_get_devicelist(self):
-        return self.data['devices']
+        return list(self.data['devices'])
 
     def _se_get_interface_mac(self, name):
         return self.data['macs'][name]
@@ -1575,6 +1575,7 @@ class TestGetInterfacesByMac(CiTestCase):
 
     def _mock_setup(self):
         self.data = copy.deepcopy(self._data)
+        self.data['devices'] = set(list(self.data['macs'].keys()))
         mocks = ('get_devicelist', 'get_interface_mac', 'is_bridge',
                  'interface_has_own_mac', 'is_vlan')
         self.mocks = {}
@@ -1602,7 +1603,7 @@ class TestGetInterfacesByMac(CiTestCase):
             [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'},
+             'aa:aa:aa:aa:aa:03': 'bridge1-nic', '00:00:00:00:00:00': 'lo'},
             ret)
 
     def test_excludes_bridges(self):
@@ -1611,7 +1612,7 @@ class TestGetInterfacesByMac(CiTestCase):
         # 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['devices'].add('b1')
         self.data['bonds'] = []
         self.data['own_macs'] = self.data['devices']
         self.data['bridges'] = [f for f in self.data['devices'] if f != "b1"]
@@ -1628,7 +1629,7 @@ class TestGetInterfacesByMac(CiTestCase):
         # set everything other than 'b1' to be a vlan.
         # 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['devices'].add('b1')
         self.data['bonds'] = []
         self.data['bridges'] = []
         self.data['own_macs'] = self.data['devices']
@@ -1640,6 +1641,16 @@ class TestGetInterfacesByMac(CiTestCase):
              mock.call('b1')],
             any_order=True)
 
+    def test_duplicates_of_empty_mac_are_ok(self):
+        """Duplicate macs of 00:00:00:00:00:00 should be skipped."""
+        self._mock_setup()
+        empty_mac = "00:00:00:00:00:00"
+        addnics = ('greptap1', 'lo', 'greptap2')
+        self.data['macs'].update(dict((k, empty_mac) for k in addnics))
+        self.data['devices'].update(set(addnics))
+        ret = net.get_interfaces_by_mac()
+        self.assertEqual('lo', ret[empty_mac])
+
 
 def _gzip_data(data):
     with io.BytesIO() as iobuf: