cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

Scott Moser has proposed merging ~smoser/cloud-init:fix/1798117-allow-toplevel-network-in-network-config into cloud-init:master.

Commit message:
NoCloud: Allow top level 'network' key in network-config.

NoCloud's 'network-config' file was originally expected to contain
network configuration without the top level 'network' key.  This was
because the file was named 'network-config' so specifying 'network'
seemed redundant.

However, JuJu is currently providing a top level 'network' config when
it tries to disable networking ({"network": {"config": "disabled"}).
Other users have also been surprised/confused by the fact that
a network config in /etc/cloud/cloud.cfg.d/network.cfg differed from
what was expected in 'network-config'.

LP: #1798117

Requested reviews:
  cloud-init commiters (cloud-init-dev)
Related bugs:
  Bug #1798117 in cloud-init: "juju sends "network" top level key to user.network-config in lxd containers"
  https://bugs.launchpad.net/cloud-init/+bug/1798117

For more details, see:
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/356850

see commit message
-- 
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:fix/1798117-allow-toplevel-network-in-network-config into cloud-init:master.

diff --git a/cloudinit/sources/DataSourceNoCloud.py b/cloudinit/sources/DataSourceNoCloud.py
index 9010f06..8b4895b 100644
--- a/cloudinit/sources/DataSourceNoCloud.py
+++ b/cloudinit/sources/DataSourceNoCloud.py
@@ -311,6 +311,33 @@ def parse_cmdline_data(ds_id, fill, cmdline=None):
     return True
 
 
+def _maybe_remove_top_network(cfg):
+    """If network-config contains top level 'network' key, then remove it.
+
+    Some providers of network configuration may provide a top level
+    'network' key (LP: #1798117) even though it is not necessary.
+
+    Be friendly and remove it if it really seems so.
+
+    Return the original value if no change or the updated value if changed."""
+    nullval = object()
+    network_val = cfg.get('network', nullval)
+    if network_val is nullval:
+        return cfg
+    bmsg = 'Top level network key in network-config %s: %s'
+    if not isinstance(network_val, dict):
+        LOG.debug(bmsg, "was not a dict", cfg)
+        return cfg
+    if len(list(cfg.keys())) != 1:
+        LOG.debug(bmsg, "had multiple top level keys", cfg)
+        return cfg
+    if not ('config' in network_val or 'version' in network_val):
+        LOG.debug(bmsg, "but missing 'config' and 'version'", cfg)
+        return cfg
+    LOG.debug(bmsg, "fixed by removing shifting network.", cfg)
+    return network_val
+
+
 def _merge_new_seed(cur, seeded):
     ret = cur.copy()
 
@@ -320,7 +347,8 @@ def _merge_new_seed(cur, seeded):
     ret['meta-data'] = util.mergemanydict([cur['meta-data'], newmd])
 
     if seeded.get('network-config'):
-        ret['network-config'] = util.load_yaml(seeded['network-config'])
+        ret['network-config'] = _maybe_remove_top_network(
+            util.load_yaml(seeded.get('network-config')))
 
     if 'user-data' in seeded:
         ret['user-data'] = seeded['user-data']
diff --git a/tests/unittests/test_datasource/test_nocloud.py b/tests/unittests/test_datasource/test_nocloud.py
index b6468b6..848876b 100644
--- a/tests/unittests/test_datasource/test_nocloud.py
+++ b/tests/unittests/test_datasource/test_nocloud.py
@@ -1,7 +1,10 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
 from cloudinit import helpers
-from cloudinit.sources import DataSourceNoCloud
+from cloudinit.sources.DataSourceNoCloud import (
+    DataSourceNoCloud as dsNoCloud,
+    _maybe_remove_top_network,
+    parse_cmdline_data)
 from cloudinit import util
 from cloudinit.tests.helpers import CiTestCase, populate_dir, mock, ExitStack
 
@@ -40,9 +43,7 @@ class TestNoCloudDataSource(CiTestCase):
             'datasource': {'NoCloud': {'fs_label': None}}
         }
 
-        ds = DataSourceNoCloud.DataSourceNoCloud
-
-        dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
+        dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
         ret = dsrc.get_data()
         self.assertEqual(dsrc.userdata_raw, ud)
         self.assertEqual(dsrc.metadata, md)
@@ -63,9 +64,7 @@ class TestNoCloudDataSource(CiTestCase):
             'datasource': {'NoCloud': {'fs_label': None}}
         }
 
-        ds = DataSourceNoCloud.DataSourceNoCloud
-
-        dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
+        dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
         self.assertTrue(dsrc.get_data())
         self.assertEqual(dsrc.platform_type, 'nocloud')
         self.assertEqual(
@@ -73,8 +72,6 @@ class TestNoCloudDataSource(CiTestCase):
 
     def test_fs_label(self, m_is_lxd):
         # find_devs_with should not be called ff fs_label is None
-        ds = DataSourceNoCloud.DataSourceNoCloud
-
         class PsuedoException(Exception):
             pass
 
@@ -84,12 +81,12 @@ class TestNoCloudDataSource(CiTestCase):
 
         # by default, NoCloud should search for filesystems by label
         sys_cfg = {'datasource': {'NoCloud': {}}}
-        dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
+        dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
         self.assertRaises(PsuedoException, dsrc.get_data)
 
         # but disabling searching should just end up with None found
         sys_cfg = {'datasource': {'NoCloud': {'fs_label': None}}}
-        dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
+        dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
         ret = dsrc.get_data()
         self.assertFalse(ret)
 
@@ -97,13 +94,10 @@ class TestNoCloudDataSource(CiTestCase):
         # no source should be found if no cmdline, config, and fs_label=None
         sys_cfg = {'datasource': {'NoCloud': {'fs_label': None}}}
 
-        ds = DataSourceNoCloud.DataSourceNoCloud
-        dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
+        dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
         self.assertFalse(dsrc.get_data())
 
     def test_seed_in_config(self, m_is_lxd):
-        ds = DataSourceNoCloud.DataSourceNoCloud
-
         data = {
             'fs_label': None,
             'meta-data': yaml.safe_dump({'instance-id': 'IID'}),
@@ -111,7 +105,7 @@ class TestNoCloudDataSource(CiTestCase):
         }
 
         sys_cfg = {'datasource': {'NoCloud': data}}
-        dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
+        dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
         ret = dsrc.get_data()
         self.assertEqual(dsrc.userdata_raw, b"USER_DATA_RAW")
         self.assertEqual(dsrc.metadata.get('instance-id'), 'IID')
@@ -130,9 +124,7 @@ class TestNoCloudDataSource(CiTestCase):
             'datasource': {'NoCloud': {'fs_label': None}}
         }
 
-        ds = DataSourceNoCloud.DataSourceNoCloud
-
-        dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
+        dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
         ret = dsrc.get_data()
         self.assertEqual(dsrc.userdata_raw, ud)
         self.assertEqual(dsrc.metadata, md)
@@ -145,9 +137,7 @@ class TestNoCloudDataSource(CiTestCase):
 
         sys_cfg = {'datasource': {'NoCloud': {'fs_label': None}}}
 
-        ds = DataSourceNoCloud.DataSourceNoCloud
-
-        dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
+        dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
         ret = dsrc.get_data()
         self.assertEqual(dsrc.userdata_raw, b"ud")
         self.assertFalse(dsrc.vendordata)
@@ -174,9 +164,7 @@ class TestNoCloudDataSource(CiTestCase):
 
         sys_cfg = {'datasource': {'NoCloud': {'fs_label': None}}}
 
-        ds = DataSourceNoCloud.DataSourceNoCloud
-
-        dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
+        dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
         ret = dsrc.get_data()
         self.assertTrue(ret)
         # very simple check just for the strings above
@@ -195,9 +183,23 @@ class TestNoCloudDataSource(CiTestCase):
 
         sys_cfg = {'datasource': {'NoCloud': {'fs_label': None}}}
 
-        ds = DataSourceNoCloud.DataSourceNoCloud
+        dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
+        ret = dsrc.get_data()
+        self.assertTrue(ret)
+        self.assertEqual(netconf, dsrc.network_config)
 
-        dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
+    def test_metadata_network_config_with_toplevel_network(self, m_is_lxd):
+        """network-config may have 'network' top level key."""
+        netconf = {'config': 'disabled'}
+        populate_dir(
+            os.path.join(self.paths.seed_dir, "nocloud"),
+            {'user-data': b"ud",
+             'meta-data': "instance-id: IID\n",
+             'network-config': yaml.dump({'network': netconf}) + "\n"})
+
+        sys_cfg = {'datasource': {'NoCloud': {'fs_label': None}}}
+
+        dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
         ret = dsrc.get_data()
         self.assertTrue(ret)
         self.assertEqual(netconf, dsrc.network_config)
@@ -228,9 +230,7 @@ class TestNoCloudDataSource(CiTestCase):
 
         sys_cfg = {'datasource': {'NoCloud': {'fs_label': None}}}
 
-        ds = DataSourceNoCloud.DataSourceNoCloud
-
-        dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
+        dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
         ret = dsrc.get_data()
         self.assertTrue(ret)
         self.assertEqual(netconf, dsrc.network_config)
@@ -258,8 +258,7 @@ class TestParseCommandLineData(CiTestCase):
         for (fmt, expected) in pairs:
             fill = {}
             cmdline = fmt % {'ds_id': ds_id}
-            ret = DataSourceNoCloud.parse_cmdline_data(ds_id=ds_id, fill=fill,
-                                                       cmdline=cmdline)
+            ret = parse_cmdline_data(ds_id=ds_id, fill=fill, cmdline=cmdline)
             self.assertEqual(expected, fill)
             self.assertTrue(ret)
 
@@ -276,10 +275,40 @@ class TestParseCommandLineData(CiTestCase):
 
         for cmdline in cmdlines:
             fill = {}
-            ret = DataSourceNoCloud.parse_cmdline_data(ds_id=ds_id, fill=fill,
-                                                       cmdline=cmdline)
+            ret = parse_cmdline_data(ds_id=ds_id, fill=fill, cmdline=cmdline)
             self.assertEqual(fill, {})
             self.assertFalse(ret)
 
 
+class TestMaybeRemoveToplevelNetwork(CiTestCase):
+    """test _maybe_remove_top_network function."""
+    basecfg = [{'type': 'physical', 'name': 'interface0',
+                'subnets': [{'type': 'dhcp'}]}]
+
+    def test_should_remove_safely(self):
+        mcfg = {'config': self.basecfg, 'version': 1}
+        self.assertEqual(mcfg, _maybe_remove_top_network({'network': mcfg}))
+
+    def test_no_remove_if_other_keys(self):
+        """should not shift if other keys at top level."""
+        mcfg = {'network': {'config': self.basecfg, 'version': 1},
+                'unknown_keyname': 'keyval'}
+        self.assertEqual(mcfg, _maybe_remove_top_network(mcfg))
+
+    def test_no_remove_if_non_dict(self):
+        """should not shift if not a dict."""
+        mcfg = {'network': '"content here'}
+        self.assertEqual(mcfg, _maybe_remove_top_network(mcfg))
+
+    def test_no_remove_if_no_config_or_version(self):
+        """should not shift unless network entry has config and version."""
+        mcfg = {'network': '"content here'}
+        self.assertEqual(mcfg, _maybe_remove_top_network(mcfg))
+
+    def test_remove_with_config_disabled(self):
+        """network/config=disabled should be shifted."""
+        mcfg = {'config': 'disabled'}
+        self.assertEqual(mcfg, _maybe_remove_top_network({'network': mcfg}))
+
+
 # vi: ts=4 expandtab