ConfigDrive: do not use 'id' on a link for the device name

'id' on a link in the openstack spec should be "Generic, generated ID".
current implementation was to use the host's name for the host
side nic.  Which provided names like 'tap-adfasdffd'.

We do not want to name devices like that as its quite unexpected
and non user friendly.  So here we use the system name for any
nic that is present, but then require that the nics found also
be present at the time of rendering.

The end result is that if the system boots with net.ifnames=0
then it will get 'eth0' like names.  and if it boots without net.ifnames
then it will get enp0s1 like names.

3 files changed, 74 insertions, 11 deletions

diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
index 05152ead..f47053b2 100644
--- a/cloudinit/net/__init__.py
+++ b/cloudinit/net/__init__.py
@@ -970,9 +970,16 @@ def get_interface_mac(ifname):
     return read_sys_net(ifname, "address", enoent=False)
 
 
-def get_ifname_mac_pairs():
-    """Build a list of tuples (ifname, mac)"""
-    return [(ifname, get_interface_mac(ifname)) for ifname in get_devicelist()]
-
+def get_interfaces_by_mac(devs=None):
+    """Build a dictionary of tuples {mac: name}"""
+    if devs is None:
+        devs = get_devicelist()
+    ret = {}
+    for name in devs:
+        mac = get_interface_mac(name)
+        # some devices may not have a mac (tun0)
+        if mac:
+            ret[mac] = name
+    return ret
 
 # vi: ts=4 expandtab syntax=python
diff --git a/cloudinit/sources/DataSourceConfigDrive.py b/cloudinit/sources/DataSourceConfigDrive.py
index 61d967d9..3cc9155d 100644
--- a/cloudinit/sources/DataSourceConfigDrive.py
+++ b/cloudinit/sources/DataSourceConfigDrive.py
@@ -255,7 +255,7 @@ def find_candidate_devs(probe_optical=True):
 
 
 # Convert OpenStack ConfigDrive NetworkData json to network_config yaml
-def convert_network_data(network_json=None):
+def convert_network_data(network_json=None, known_macs=None):
     """Return a dictionary of network_config by parsing provided
        OpenStack ConfigDrive NetworkData json format
 
@@ -319,9 +319,15 @@ def convert_network_data(network_json=None):
         subnets = []
         cfg = {k: v for k, v in link.items()
                if k in valid_keys['physical']}
-        cfg.update({'name': link['id']})
-        for network in [net for net in networks
-                        if net['link'] == link['id']]:
+        # 'name' is not in openstack spec yet, but we will support it if it is
+        # present.  The 'id' in the spec is currently implemented as the host
+        # nic's name, meaning something like 'tap-adfasdffd'.  We do not want
+        # to name guest devices with such ugly names.
+        if 'name' in link:
+            cfg['name'] = link['name']
+
+        for network in [n for n in networks
+                        if n['link'] == link['id']]:
             subnet = {k: v for k, v in network.items()
                       if k in valid_keys['subnet']}
             if 'dhcp' in network['type']:
@@ -365,6 +371,21 @@ def convert_network_data(network_json=None):
 
         config.append(cfg)
 
+    need_names = [d for d in config
+                  if d.get('type') == 'physical' and 'name' not in d]
+
+    if need_names:
+        if known_macs is None:
+            known_macs = net.get_interfaces_by_mac()
+
+        for d in need_names:
+            mac = d.get('mac_address')
+            if not mac:
+                raise ValueError("No mac_address or name entry for %s" % d)
+            if mac not in known_macs:
+                raise ValueError("Unable to find a system nic for %s" % d)
+            d['name'] = known_macs[mac]
+
     for service in services:
         cfg = service
         cfg.update({'type': 'nameserver'})
diff --git a/tests/unittests/test_datasource/test_configdrive.py b/tests/unittests/test_datasource/test_configdrive.py
index 195b8207..1364b39d 100644
--- a/tests/unittests/test_datasource/test_configdrive.py
+++ b/tests/unittests/test_datasource/test_configdrive.py
@@ -73,7 +73,7 @@ NETWORK_DATA = {
          'type': 'ovs', 'mtu': None, 'id': 'tap2f88d109-5b'},
         {'vif_id': '1a5382f8-04c5-4d75-ab98-d666c1ef52cc',
          'ethernet_mac_address': 'fa:16:3e:05:30:fe',
-         'type': 'ovs', 'mtu': None, 'id': 'tap1a5382f8-04'}
+         'type': 'ovs', 'mtu': None, 'id': 'tap1a5382f8-04', 'name': 'nic0'}
     ],
     'networks': [
         {'link': 'tap2ecc7709-b3', 'type': 'ipv4_dhcp',
@@ -88,6 +88,10 @@ NETWORK_DATA = {
     ]
 }
 
+KNOWN_MACS = {
+    'fa:16:3e:69:b0:58': 'enp0s1',
+    'fa:16:3e:d4:57:ad': 'enp0s2'}
+
 CFG_DRIVE_FILES_V2 = {
     'ec2/2009-04-04/meta-data.json': json.dumps(EC2_META),
     'ec2/2009-04-04/user-data': USER_DATA,
@@ -365,10 +369,40 @@ class TestConfigDriveDataSource(TestCase):
         """Verify that network_data is converted and present on ds object."""
         populate_dir(self.tmp, CFG_DRIVE_FILES_V2)
         myds = cfg_ds_from_dir(self.tmp)
-        network_config = ds.convert_network_data(NETWORK_DATA)
+        network_config = ds.convert_network_data(NETWORK_DATA,
+                                                 known_macs=KNOWN_MACS)
         self.assertEqual(myds.network_config, network_config)
 
 
+class TestConvertNetworkData(TestCase):
+    def _getnames_in_config(self, ncfg):
+        return set([n['name'] for n in ncfg['config']
+                    if n['type'] == 'physical'])
+
+    def test_conversion_fills_names(self):
+        ncfg = ds.convert_network_data(NETWORK_DATA, known_macs=KNOWN_MACS)
+        expected = set(['nic0', 'enp0s1', 'enp0s2'])
+        found = self._getnames_in_config(ncfg)
+        self.assertEqual(found, expected)
+
+    @mock.patch('cloudinit.net.get_interfaces_by_mac')
+    def test_convert_reads_system_prefers_name(self, get_interfaces_by_mac):
+        macs = KNOWN_MACS.copy()
+        macs.update({'fa:16:3e:05:30:fe': 'foonic1',
+                     'fa:16:3e:69:b0:58': 'ens1'})
+        get_interfaces_by_mac.return_value = macs
+
+        ncfg = ds.convert_network_data(NETWORK_DATA)
+        expected = set(['nic0', 'ens1', 'enp0s2'])
+        found = self._getnames_in_config(ncfg)
+        self.assertEqual(found, expected)
+
+    def test_convert_raises_value_error_on_missing_name(self):
+        macs = {'aa:aa:aa:aa:aa:00': 'ens1'}
+        self.assertRaises(ValueError, ds.convert_network_data,
+                          NETWORK_DATA, known_macs=macs)
+
+
 def cfg_ds_from_dir(seed_d):
     found = ds.read_config_drive(seed_d)
     cfg_ds = ds.DataSourceConfigDrive(settings.CFG_BUILTIN, None,
@@ -387,7 +421,8 @@ def populate_ds_from_read_config(cfg_ds, source, results):
     cfg_ds.userdata_raw = results.get('userdata')
     cfg_ds.version = results.get('version')
     cfg_ds.network_json = results.get('networkdata')
-    cfg_ds._network_config = ds.convert_network_data(cfg_ds.network_json)
+    cfg_ds._network_config = ds.convert_network_data(
+        cfg_ds.network_json, known_macs=KNOWN_MACS)
 
 
 def populate_dir(seed_dir, files):