cloud-init-dev team mailing list archive

[Ben Howard <bh@xxxxx>]

Ben Howard has proposed merging ~utlemming/cloud-init:lp-1676908 into cloud-init:master.

Requested reviews:
  cloud init development team (cloud-init-dev)
  Server Team CI bot (server-team-bot): continuous-integration

For more details, see:
https://code.launchpad.net/~utlemming/cloud-init/+git/cloud-init-1/+merge/322321

This is a request to merge the improvements to the DigitalOcean datasource.

The changes:
- No longer bind the nameservers to a specific interface to bring it inline with the other DataSources like OpenStack and SmartOS.
- Fix mis-binding the IPV4all address to a secondary interface by considering 'eth0' or 'ens3' first
- Consider all network definitions, not just 'public' or 'private'.
-- 
Your team cloud init development team is requested to review the proposed merge of ~utlemming/cloud-init:lp-1676908 into cloud-init:master.

diff --git a/cloudinit/sources/helpers/digitalocean.py b/cloudinit/sources/helpers/digitalocean.py
index 72f7bde..4c9253e 100644
--- a/cloudinit/sources/helpers/digitalocean.py
+++ b/cloudinit/sources/helpers/digitalocean.py
@@ -10,6 +10,7 @@ from cloudinit import net as cloudnet
 from cloudinit import url_helper
 from cloudinit import util
 
+FIRST_NICS = ['eth0', 'ens3']
 NIC_MAP = {'public': 'eth0', 'private': 'eth1'}
 
 LOG = logging.getLogger(__name__)
@@ -22,8 +23,13 @@ def assign_ipv4_link_local(nic=None):
        address is random.
     """
 
+    # if there is an eth0, then it has already been mapped from ens3 to eth0
+    c_devs = FIRST_NICS
+    c_devs.extend([x for x in sorted(cloudnet.get_devicelist())
+                   if x not in FIRST_NICS])
+
     if not nic:
-        for cdev in sorted(cloudnet.get_devicelist()):
+        for cdev in c_devs:
             if cloudnet.is_physical(cdev):
                 nic = cdev
                 LOG.debug("assigned nic '%s' for link-local discovery", nic)
@@ -107,15 +113,12 @@ def convert_network_configuration(config, dns_servers):
         }
     """
 
-    def _get_subnet_part(pcfg, nameservers=None):
+    def _get_subnet_part(pcfg):
         subpart = {'type': 'static',
                    'control': 'auto',
                    'address': pcfg.get('ip_address'),
                    'gateway': pcfg.get('gateway')}
 
-        if nameservers:
-            subpart['dns_nameservers'] = nameservers
-
         if ":" in pcfg.get('ip_address'):
             subpart['address'] = "{0}/{1}".format(pcfg.get('ip_address'),
                                                   pcfg.get('cidr'))
@@ -124,27 +127,32 @@ def convert_network_configuration(config, dns_servers):
 
         return subpart
 
-    all_nics = []
-    for k in ('public', 'private'):
-        if k in config:
-            all_nics.extend(config[k])
-
-    macs_to_nics = cloudnet.get_interfaces_by_mac()
     nic_configs = []
+    macs_to_nics = cloudnet.get_interfaces_by_mac()
+    LOG.debug("nic mapping: %s", macs_to_nics)
 
-    for nic in all_nics:
+    for n in config:
+        nic = config[n][0]
+        LOG.debug("considering %s", nic)
 
         mac_address = nic.get('mac')
+        LOG.critical("%s / %s" % (mac_address, macs_to_nics))
+        if mac_address not in macs_to_nics:
+            raise RuntimeError("Did not find network interface on system "
+                               "with mac '%s'. Cannot apply configuration: %s"
+                                % (mac_address, nic))
+
         sysfs_name = macs_to_nics.get(mac_address)
         nic_type = nic.get('type', 'unknown')
-        # Note: the entry 'public' above contains a list, but
-        # the list will only ever have one nic inside it per digital ocean.
-        # If it ever had more than one nic, then this code would
-        # assign all 'public' the same name.
-        if_name = NIC_MAP.get(nic_type, sysfs_name)
 
-        LOG.debug("mapped %s interface to %s, assigning name of %s",
-                  mac_address, sysfs_name, if_name)
+        if_name = NIC_MAP.get(nic_type, sysfs_name)
+        if if_name != sysfs_name:
+            LOG.info("Found %s interface '%s' on '%s', assigned name of '%s'",
+                     nic_type, mac_address, sysfs_name, if_name)
+        else:
+            msg = ("Found interface '%s' on '%s', which is not a public "
+                   "or private interface. Using default system naming.")
+            LOG.info(msg, mac_address, sysfs_name)
 
         ncfg = {'type': 'physical',
                 'mac_address': mac_address,
@@ -157,13 +165,8 @@ def convert_network_configuration(config, dns_servers):
                 continue
 
             sub_part = _get_subnet_part(raw_subnet)
-            if nic_type == 'public' and 'anchor' not in netdef:
-                # add DNS resolvers to the public interfaces only
-                sub_part = _get_subnet_part(raw_subnet, dns_servers)
-            else:
-                # remove the gateway any non-public interfaces
-                if 'gateway' in sub_part:
-                    del sub_part['gateway']
+            if netdef in ('private', 'anchor_ipv4', 'anchor_ipv6'):
+                del sub_part['gateway']
 
             subnets.append(sub_part)
 
@@ -171,6 +174,10 @@ def convert_network_configuration(config, dns_servers):
         nic_configs.append(ncfg)
         LOG.debug("nic '%s' configuration: %s", if_name, ncfg)
 
+    if dns_servers:
+        LOG.debug("added dns servers")
+        nic_configs.append({'type': 'nameserver', 'address': dns_servers})
+
     return {'version': 1, 'config': nic_configs}
 
 
diff --git a/tests/unittests/test_datasource/test_digitalocean.py b/tests/unittests/test_datasource/test_digitalocean.py
index 61d6e00..59f5422 100644
--- a/tests/unittests/test_datasource/test_digitalocean.py
+++ b/tests/unittests/test_datasource/test_digitalocean.py
@@ -1,6 +1,7 @@
 # Copyright (C) 2014 Neal Shrader
 #
 # Author: Neal Shrader <neal@xxxxxxxxxxxxxxxx>
+# Author: Ben Howard <bh@xxxxxxxxxxxxxxxx>
 #
 # This file is part of cloud-init. See LICENSE file for license information.
 
@@ -209,17 +210,24 @@ class TestNetworkConvert(TestCase):
         netcfg = self._get_networking()
         self.assertIsNotNone(netcfg)
 
-        for nic_def in netcfg.get('config'):
-            print(json.dumps(nic_def, indent=3))
-            n_type = nic_def.get('type')
-            n_subnets = nic_def.get('type')
-            n_name = nic_def.get('name')
-            n_mac = nic_def.get('mac_address')
+        for part in netcfg.get('config'):
+            print(json.dumps(part, indent=3))
+            n_type = part.get('type')
+
+            if n_type == 'nameserver':
+                n_address = part.get('address')
+                self.assertIsNotNone(n_address)
+                self.assertEqual(len(n_address), 3)
+
+            else:
+                n_subnets = part.get('type')
+                n_name = part.get('name')
+                n_mac = part.get('mac_address')
 
-            self.assertIsNotNone(n_type)
-            self.assertIsNotNone(n_subnets)
-            self.assertIsNotNone(n_name)
-            self.assertIsNotNone(n_mac)
+                self.assertIsNotNone(n_type)
+                self.assertIsNotNone(n_subnets)
+                self.assertIsNotNone(n_name)
+                self.assertIsNotNone(n_mac)
 
     def _get_nic_definition(self, int_type, expected_name):
         """helper function to return if_type (i.e. public) and the expected
@@ -260,12 +268,6 @@ class TestNetworkConvert(TestCase):
         self.assertEqual(meta_def.get('mac'), nic_def.get('mac_address'))
         self.assertEqual('physical', nic_def.get('type'))
 
-    def _check_dns_nameservers(self, subn_def):
-        self.assertIn('dns_nameservers', subn_def)
-        expected_nameservers = DO_META['dns']['nameservers']
-        nic_nameservers = subn_def.get('dns_nameservers')
-        self.assertEqual(expected_nameservers, nic_nameservers)
-
     def test_public_interface_ipv6(self):
         """test public ipv6 addressing"""
         (nic_def, meta_def) = self._get_nic_definition('public', 'eth0')
@@ -280,7 +282,6 @@ class TestNetworkConvert(TestCase):
 
         self.assertEqual(cidr_notated_address, subn_def.get('address'))
         self.assertEqual(ipv6_def.get('gateway'), subn_def.get('gateway'))
-        self._check_dns_nameservers(subn_def)
 
     def test_public_interface_ipv4(self):
         """test public ipv4 addressing"""
@@ -293,7 +294,6 @@ class TestNetworkConvert(TestCase):
 
         self.assertEqual(ipv4_def.get('netmask'), subn_def.get('netmask'))
         self.assertEqual(ipv4_def.get('gateway'), subn_def.get('gateway'))
-        self._check_dns_nameservers(subn_def)
 
     def test_public_interface_anchor_ipv4(self):
         """test public ipv4 addressing"""
@@ -308,14 +308,11 @@ class TestNetworkConvert(TestCase):
         self.assertNotIn('gateway', subn_def)
 
     @mock.patch('cloudinit.net.get_interfaces_by_mac')
-    def test_convert_without_private(self, m_get_by_mac):
-        m_get_by_mac.return_value = {
-            'b8:ae:ed:75:5f:9a': 'enp0s25',
+    def test_convert_without_private(self, m_get_interfaces_by_mac):
+        m_get_interfaces_by_mac.return_value = {
             'ae:cc:08:7c:88:00': 'meta2p1'}
         netcfg = digitalocean.convert_network_configuration(
             DO_META_2['interfaces'], DO_META_2['dns']['nameservers'])
-
-        # print(netcfg)
         byname = {}
         for i in netcfg['config']:
             if 'name' in i:
@@ -330,4 +327,11 @@ class TestNetworkConvert(TestCase):
             sorted(['45.55.249.133', '10.17.0.5']),
             sorted([i['address'] for i in eth0['subnets']]))
 
+    @mock.patch('cloudinit.net.get_interfaces_by_mac')
+    def test_missing_nic_raises_runtime(self, m_get_interfaces_by_mac):
+        m_get_interfaces_by_mac.return_value = {'99:99:99:00:00:00': 'eth0'}
+        self.assertRaises(
+            RuntimeError, digitalocean.convert_network_configuration,
+            *[DO_META_2['interfaces'], DO_META_2['dns']['nameservers']])
+
 # vi: ts=4 expandtab