Consistently strip top-level network key (SC-963)

cloudinit/net/network_state.py

@@ -1059,7 +1059,8 @@ def parse_net_config_data(net_config, skip_broken=True) -> NetworkState:
     if not state:
         raise RuntimeError(
             "No valid network_state object created from network config. "
-            "Did you specify the correct version?"
+            "Did you specify the correct version? Network config:\n"
+            f"{net_config}"
         )
 
     return state

cloudinit/sources/DataSourceLXD.py

@@ -109,34 +109,6 @@ def get_connection(self, url, proxies=None):
         return SocketConnectionPool(LXD_SOCKET_PATH)
 
 
-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."""
-    if "network" not in cfg:
-        return cfg
-    network_val = cfg["network"]
-    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 network_val.get("config") == "disabled":
-        LOG.debug(bmsg, "was config/disabled", cfg)
-    elif not all(("config" in network_val, "version" in network_val)):
-        LOG.debug(bmsg, "but missing 'config' or 'version'", cfg)
-        return cfg
-    LOG.debug(bmsg, "fixed by removing shifting network.", cfg)
-    return network_val
-
-
 def _raw_instance_data_to_dict(metadata_type: str, metadata_value) -> dict:
     """Convert raw instance data from str, bytes, YAML to dict
 
@@ -211,10 +183,8 @@ def _get_data(self) -> bool:
         if "user-data" in self._crawled_metadata:
             self.userdata_raw = self._crawled_metadata["user-data"]
         if "network-config" in self._crawled_metadata:
-            self._network_config = _maybe_remove_top_network(
-                _raw_instance_data_to_dict(
-                    "network-config", self._crawled_metadata["network-config"]
-                )
+            self._network_config = _raw_instance_data_to_dict(
+                "network-config", self._crawled_metadata["network-config"]
             )
         if "vendor-data" in self._crawled_metadata:
             self.vendordata_raw = self._crawled_metadata["vendor-data"]

cloudinit/sources/DataSourceNoCloud.py

@@ -331,35 +331,6 @@ 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 network_val.get("config") == "disabled":
-        LOG.debug(bmsg, "was config/disabled", cfg)
-    elif not all(("config" in network_val, "version" in network_val)):
-        LOG.debug(bmsg, "but missing 'config' or '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()
 
@@ -369,9 +340,7 @@ def _merge_new_seed(cur, seeded):
     ret["meta-data"] = util.mergemanydict([cur["meta-data"], newmd])
 
     if seeded.get("network-config"):
-        ret["network-config"] = _maybe_remove_top_network(
-            util.load_yaml(seeded.get("network-config"))
-        )
+        ret["network-config"] = util.load_yaml(seeded.get("network-config"))
 
     if "user-data" in seeded:
         ret["user-data"] = seeded["user-data"]

cloudinit/stages.py

@@ -799,6 +799,16 @@ def _consume_userdata(self, frequency=PER_INSTANCE):
         # Run the handlers
         self._do_handlers(user_data_msg, c_handlers_list, frequency)
 
+    def _remove_top_level_network_key(self, cfg):
+        """If network-config contains top level 'network' key, then remove it.
+
+        Some providers of network configuration skip the top-level network
+        key, so ensure both methods works.
+        """
+        if cfg and "network" in cfg:
+            return cfg["network"]
+        return cfg
+
     def _find_networking_config(self):
         disable_file = os.path.join(
             self.paths.get_cpath("data"), "upgraded-network"
@@ -836,7 +846,9 @@ def _find_networking_config(self):
                     cfg_source,
                 )
                 continue
-            ncfg = available_cfgs[cfg_source]
+            ncfg = self._remove_top_level_network_key(
+                available_cfgs[cfg_source]
+            )
             if net.is_disabled_cfg(ncfg):
                 LOG.debug("network config disabled by %s", cfg_source)
                 return (None, cfg_source)

tests/unittests/net/test_network_state.py

@@ -217,6 +217,3 @@ def test_mask_to_net_prefix_ipv6_object(self):
         expected = 48
         prefix_value = network_state.ipv6_mask_to_net_prefix(netmask_value)
         assert prefix_value == expected
-
-
-# vi: ts=4 expandtab

tests/unittests/sources/test_nocloud.py

@@ -7,10 +7,7 @@
 
 from cloudinit import dmi, helpers, util
 from cloudinit.sources.DataSourceNoCloud import DataSourceNoCloud as dsNoCloud
-from cloudinit.sources.DataSourceNoCloud import (
-    _maybe_remove_top_network,
-    parse_cmdline_data,
-)
+from cloudinit.sources.DataSourceNoCloud import parse_cmdline_data
 from tests.unittests.helpers import CiTestCase, ExitStack, mock, populate_dir
 
 
@@ -253,25 +250,6 @@ def test_metadata_network_config(self, m_is_lxd):
         self.assertTrue(ret)
         self.assertEqual(netconf, dsrc.network_config)
 
-    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)
-
     def test_metadata_network_config_over_interfaces(self, m_is_lxd):
         # network-config should override meta-data/network-interfaces
         gateway = "103.225.10.1"
@@ -406,48 +384,3 @@ def test_parse_cmdline_data_none(self):
             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_missing_config_or_version(self):
-        """should not shift unless network entry has config and version."""
-        mcfg = {"network": {"config": self.basecfg}}
-        self.assertEqual(mcfg, _maybe_remove_top_network(mcfg))
-
-        mcfg = {"network": {"version": 1}}
-        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