Standardize kernel commandline user interface (#2093)

- deprecate ci.ds= and ci.datasource= in favor of ds=
- enable semi-colon-delimited datasource everywhere
- add support for case-insensitive datasource match
- add integration tests

9 files changed, 165 insertions, 61 deletions

diff --git a/cloudinit/importer.py b/cloudinit/importer.py
index ce25fe9a..2ddd7a47 100644
--- a/cloudinit/importer.py
+++ b/cloudinit/importer.py
@@ -12,6 +12,8 @@ import importlib
 from types import ModuleType
 from typing import Optional, Sequence
 
+from cloudinit import util
+
 
 def import_module(module_name: str) -> ModuleType:
     return importlib.import_module(module_name)
@@ -30,6 +32,26 @@ def _count_attrs(
     return found_attrs
 
 
+def match_case_insensitive_module_name(mod_name: str) -> Optional[str]:
+    """Check the importable datasource modules for a case-insensitive match."""
+
+    # nocloud-net is the only datasource that requires matching on a name that
+    # does not match its python module - canonicalize it here
+    if "nocloud-net" == mod_name.lower():
+        mod_name = mod_name[:-4]
+    if not mod_name.startswith("DataSource"):
+        ds_name = f"DataSource{mod_name}"
+    modules = {}
+    spec = importlib.util.find_spec("cloudinit.sources")
+    if spec and spec.submodule_search_locations:
+        for dir in spec.submodule_search_locations:
+            modules.update(util.get_modules_from_dir(dir))
+        for module in modules.values():
+            if module.lower() == ds_name.lower():
+                return module
+    return ds_name
+
+
 def find_module(
     base_name: str,
     search_paths: Sequence[str],
@@ -38,23 +60,16 @@ def find_module(
     """Finds specified modules"""
     if not required_attrs:
         required_attrs = []
-    # NOTE(harlowja): translate the search paths to include the base name.
     lookup_paths = []
+    found_paths = []
+
     for path in search_paths:
-        real_path = []
-        if path:
-            real_path.extend(path.split("."))
-        real_path.append(base_name)
-        full_path = ".".join(real_path)
+        # Add base name to search paths. Filter out empty paths.
+        full_path = ".".join(filter(None, [path, base_name]))
         lookup_paths.append(full_path)
-    found_paths = []
-    for full_path in lookup_paths:
         if not importlib.util.find_spec(full_path):
             continue
         # Check that required_attrs are all present within the module.
         if _count_attrs(full_path, required_attrs) == len(required_attrs):
             found_paths.append(full_path)
     return (found_paths, lookup_paths)
-
-
-# vi: ts=4 expandtab
diff --git a/cloudinit/sources/DataSourceNoCloud.py b/cloudinit/sources/DataSourceNoCloud.py
index a32bd4d0..596a96a7 100644
--- a/cloudinit/sources/DataSourceNoCloud.py
+++ b/cloudinit/sources/DataSourceNoCloud.py
@@ -357,6 +357,14 @@ class DataSourceNoCloudNet(DataSourceNoCloud):
         DataSourceNoCloud.__init__(self, sys_cfg, distro, paths)
         self.supported_seed_starts = ("http://", "https://")
 
+    def ds_detect(self):
+        """NoCloud requires "nocloud-net" as the way to specify
+        seeding from an http(s) address. This diverges from all other
+        datasources in that it does a kernel commandline match on something
+        other than the datasource dsname for only DEP_NETWORK.
+        """
+        return "nocloud-net" == sources.parse_cmdline()
+
 
 # Used to match classes to dependencies
 datasources = [
@@ -368,6 +376,3 @@ datasources = [
 # Return a list of data sources that match this set of dependencies
 def get_datasource_list(depends):
     return sources.list_from_depends(depends, datasources)
-
-
-# vi: ts=4 expandtab
diff --git a/cloudinit/sources/__init__.py b/cloudinit/sources/__init__.py
index 2779cac4..90521ba2 100644
--- a/cloudinit/sources/__init__.py
+++ b/cloudinit/sources/__init__.py
@@ -321,7 +321,7 @@ class DataSource(CloudInitPickleMixin, metaclass=abc.ABCMeta):
         does not run, _something_ needs to detect the kernel command line
         definition.
         """
-        if self.dsname == parse_cmdline():
+        if self.dsname.lower() == parse_cmdline().lower():
             LOG.debug(
                 "Machine is configured by the kernel commandline to run on "
                 "single datasource %s.",
@@ -998,11 +998,12 @@ def find_source(
     raise DataSourceNotFoundException(msg)
 
 
-# Return a list of classes that have the same depends as 'depends'
-# iterate through cfg_list, loading "DataSource*" modules
-# and calling their "get_datasource_list".
-# Return an ordered list of classes that match (if any)
 def list_sources(cfg_list, depends, pkg_list):
+    """Return a list of classes that have the same depends as 'depends'
+    iterate through cfg_list, loading "DataSource*" modules
+    and calling their "get_datasource_list".
+    Return an ordered list of classes that match (if any)
+    """
     src_list = []
     LOG.debug(
         "Looking for data source in: %s,"
@@ -1011,9 +1012,9 @@ def list_sources(cfg_list, depends, pkg_list):
         pkg_list,
         depends,
     )
-    for ds_name in cfg_list:
-        if not ds_name.startswith(DS_PREFIX):
-            ds_name = "%s%s" % (DS_PREFIX, ds_name)
+
+    for ds in cfg_list:
+        ds_name = importer.match_case_insensitive_module_name(ds)
         m_locs, _looked_locs = importer.find_module(
             ds_name, pkg_list, ["get_datasource_list"]
         )
@@ -1149,13 +1150,27 @@ def pkl_load(fname: str) -> Optional[DataSource]:
         return None
 
 
-def parse_cmdline():
+def parse_cmdline() -> str:
     """Check if command line argument for this datasource was passed
     Passing by command line overrides runtime datasource detection
     """
     cmdline = util.get_cmdline()
-    ds_parse_1 = re.search(r"ci\.ds=([a-zA-Z]+)(\s|$)", cmdline)
-    ds_parse_2 = re.search(r"ci\.datasource=([a-zA-Z]+)(\s|$)", cmdline)
-    ds = ds_parse_1 or ds_parse_2
-    if ds:
+    ds_parse_0 = re.search(r"ds=([a-zA-Z]+)(\s|$|;)", cmdline)
+    ds_parse_1 = re.search(r"ci\.ds=([a-zA-Z]+)(\s|$|;)", cmdline)
+    ds_parse_2 = re.search(r"ci\.datasource=([a-zA-Z]+)(\s|$|;)", cmdline)
+    ds = ds_parse_0 or ds_parse_1 or ds_parse_2
+    deprecated = ds_parse_1 or ds_parse_2
+    if deprecated:
+        dsname = deprecated.group(1).strip()
+        util.deprecate(
+            deprecated=(
+                f"Defining the datasource on the commandline using "
+                f"ci.ds={dsname} or "
+                f"ci.datasource={dsname}"
+            ),
+            deprecated_version="23.2",
+            extra_message=f"Use ds={dsname} instead",
+        )
+    if ds and ds.group(1):
         return ds.group(1)
+    return ""
diff --git a/tests/integration_tests/datasources/test_detect_openstack.py b/tests/integration_tests/test_kernel_commandline_match.py
index 73246f8f..af1e305f 100644
--- a/tests/integration_tests/datasources/test_detect_openstack.py
+++ b/tests/integration_tests/test_kernel_commandline_match.py
@@ -4,28 +4,19 @@ from tests.integration_tests.instances import IntegrationInstance
 from tests.integration_tests.integration_settings import PLATFORM
 
 
-@pytest.mark.lxd_use_exec
-@pytest.mark.skipif(PLATFORM != "lxd_vm", reason="Modifies grub config")
-def test_lxd_datasource_kernel_override(client: IntegrationInstance):
-    """This test is twofold: it tests kernel commandline override, which also
-    validates OpenStack Ironic requirements. OpenStack Ironic does not
-    advertise itself to cloud-init via any of the conventional methods: DMI,
-    etc.
-
-    On systemd, ds-identify is able to grok kernel commandline, however to
-    support cloud-init kernel command line parsing on non-systemd, parsing
-    kernel commandline in Python code is required.
-
-    This test runs on LXD, but forces cloud-init to attempt to run OpenStack.
-    This will inevitably fail on LXD, but we only care that it tried - on
-    Ironic it will succeed.
-
+def override_kernel_cmdline(ds_str: str, c: IntegrationInstance) -> str:
+    """
     Configure grub's kernel command line to tell cloud-init to use OpenStack
     - even though LXD should naturally be detected.
+
+    This runs on LXD, but forces cloud-init to attempt to run OpenStack.
+    This will inevitably fail on LXD, but we only care that it tried - on
+    Ironic, for example, it will succeed.
     """
+    client = c
     client.execute(
         "sed --in-place "
-        '\'s/^.*GRUB_CMDLINE_LINUX=.*$/GRUB_CMDLINE_LINUX="ci.ds=OpenStack"/g'
+        "'s/^.*GRUB_CMDLINE_LINUX=.*$/GRUB_CMDLINE_LINUX=\"" + ds_str + '"/g'
         "' /etc/default/grub"
     )
 
@@ -37,8 +28,49 @@ def test_lxd_datasource_kernel_override(client: IntegrationInstance):
     client.instance.execute_via_ssh = False
     client.instance.start()
     client.execute("cloud-init status --wait")
-    log = client.execute("cat /var/log/cloud-init.log")
+    return client.execute("cat /var/log/cloud-init.log")
+
+
+@pytest.mark.skipif(PLATFORM != "lxd_vm", reason="Modifies grub config")
+@pytest.mark.lxd_use_exec
+@pytest.mark.parametrize(
+    "ds_str, configured",
+    (
+        ("ds=nocloud", "DataSourceNoCloud"),
+        ("ci.ds=openstack", "DataSourceOpenStack"),
+    ),
+)
+def test_lxd_datasource_kernel_override(
+    ds_str, configured, client: IntegrationInstance
+):
+    """This test is twofold: it tests kernel commandline override, which also
+    validates OpenStack Ironic requirements. OpenStack Ironic does not
+    advertise itself to cloud-init via any of the conventional methods: DMI,
+    etc.
+
+    On systemd, ds-identify is able to grok kernel commandline, however to
+    support cloud-init kernel command line parsing on non-systemd, parsing
+    kernel commandline in Python code is required.
+    """
+
     assert (
         "Machine is configured by the kernel commandline to run on single "
-        "datasource DataSourceOpenStackLocal"
-    ) in log
+        f"datasource {configured}"
+    ) in override_kernel_cmdline(ds_str, client)
+
+
+@pytest.mark.skipif(PLATFORM != "lxd_vm", reason="Modifies grub config")
+@pytest.mark.lxd_use_exec
+@pytest.mark.parametrize("ds_str", ("ci.ds=nocloud-net",))
+def test_lxd_datasource_kernel_override_nocloud_net(
+    ds_str, client: IntegrationInstance
+):
+    """NoCloud requirements vary slightly from other datasources with parsing
+    nocloud-net due to historical reasons. Do to this variation, this is
+    implemented in NoCloud's ds_detect and therefore has a different log
+    message.
+    """
+
+    assert (
+        "Machine is running on DataSourceNoCloud [seed=None][dsmode=net]."
+    ) in override_kernel_cmdline(ds_str, client)
diff --git a/tests/unittests/sources/test___init__.py b/tests/unittests/sources/test___init__.py
index b84976da..d40f9ad1 100644
--- a/tests/unittests/sources/test___init__.py
+++ b/tests/unittests/sources/test___init__.py
@@ -13,11 +13,15 @@ from tests.unittests.helpers import mock
         "ci.ds=OpenStack",
         "aosiejfoij ci.ds=OpenStack blah",
         "aosiejfoij ci.ds=OpenStack faljskebflk",
+        "ci.ds=OpenStack;",
+        "ci.ds=openstack;",
         # test ci.datasource=
         "aosiejfoij ci.datasource=OpenStack ",
         "ci.datasource=OpenStack",
         "aosiejfoij ci.datasource=OpenStack blah",
         "aosiejfoij ci.datasource=OpenStack faljskebflk",
+        "ci.datasource=OpenStack;",
+        "ci.datasource=openstack;",
         # weird whitespace
         "ci.datasource=OpenStack\n",
         "ci.datasource=OpenStack\t",
@@ -36,5 +40,6 @@ def test_ds_detect_kernel_commandline(m_cmdline):
         return_value=m_cmdline,
     ):
         assert (
-            ds.DataSourceOpenStack.dsname == sources.parse_cmdline()
+            ds.DataSourceOpenStack.dsname.lower()
+            == sources.parse_cmdline().lower()
         ), f"could not parse [{m_cmdline}]"
diff --git a/tests/unittests/sources/test_cloudsigma.py b/tests/unittests/sources/test_cloudsigma.py
index 3b3279c8..0410c3c7 100644
--- a/tests/unittests/sources/test_cloudsigma.py
+++ b/tests/unittests/sources/test_cloudsigma.py
@@ -1,8 +1,10 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
+
 import copy
+from unittest import mock
 
-from cloudinit import distros, helpers, sources
+from cloudinit import distros, helpers, importer, sources
 from cloudinit.sources import DataSourceCloudSigma
 from cloudinit.sources.helpers.cloudsigma import Cepko
 from tests.unittests import helpers as test_helpers
@@ -137,8 +139,15 @@ class DsLoads(test_helpers.TestCase):
         ds_list = DataSourceCloudSigma.get_datasource_list(deps)
         self.assertEqual(ds_list, [DataSourceCloudSigma.DataSourceCloudSigma])
 
+    @mock.patch.object(
+        importer,
+        "match_case_insensitive_module_name",
+        lambda name: f"DataSource{name}",
+    )
     def test_list_sources_finds_ds(self):
         found = sources.list_sources(
-            ["CloudSigma"], (sources.DEP_FILESYSTEM,), ["cloudinit.sources"]
+            ["CloudSigma"],
+            (sources.DEP_FILESYSTEM,),
+            ["cloudinit.sources"],
         )
         self.assertEqual([DataSourceCloudSigma.DataSourceCloudSigma], found)
diff --git a/tests/unittests/sources/test_common.py b/tests/unittests/sources/test_common.py
index daeefa8a..d38d2fce 100644
--- a/tests/unittests/sources/test_common.py
+++ b/tests/unittests/sources/test_common.py
@@ -1,6 +1,8 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
-from cloudinit import settings, sources, type_utils
+from unittest.mock import patch
+
+from cloudinit import importer, settings, sources, type_utils
 from cloudinit.sources import DataSource
 from cloudinit.sources import DataSourceAliYun as AliYun
 from cloudinit.sources import DataSourceAltCloud as AltCloud
@@ -80,21 +82,42 @@ class ExpectedDataSources(test_helpers.TestCase):
     deps_network = [sources.DEP_FILESYSTEM, sources.DEP_NETWORK]
     pkg_list = [type_utils.obj_name(sources)]
 
+    @patch.object(
+        importer,
+        "match_case_insensitive_module_name",
+        lambda name: f"DataSource{name}",
+    )
     def test_expected_default_local_sources_found(self):
         found = sources.list_sources(
-            self.builtin_list, self.deps_local, self.pkg_list
+            self.builtin_list,
+            self.deps_local,
+            self.pkg_list,
         )
         self.assertEqual(set(DEFAULT_LOCAL), set(found))
 
+    @patch.object(
+        importer,
+        "match_case_insensitive_module_name",
+        lambda name: f"DataSource{name}",
+    )
     def test_expected_default_network_sources_found(self):
         found = sources.list_sources(
-            self.builtin_list, self.deps_network, self.pkg_list
+            self.builtin_list,
+            self.deps_network,
+            self.pkg_list,
         )
         self.assertEqual(set(DEFAULT_NETWORK), set(found))
 
+    @patch.object(
+        importer,
+        "match_case_insensitive_module_name",
+        lambda name: f"DataSource{name}",
+    )
     def test_expected_nondefault_network_sources_found(self):
         found = sources.list_sources(
-            ["AliYun"], self.deps_network, self.pkg_list
+            ["AliYun"],
+            self.deps_network,
+            self.pkg_list,
         )
         self.assertEqual(set([AliYun.DataSourceAliYun]), set(found))
 
@@ -120,6 +143,3 @@ class TestDataSourceInvariants(test_helpers.TestCase):
             )
             self.assertNotEqual(ds.dsname, DataSource.dsname, fail_msg)
             self.assertIsNotNone(ds.dsname)
-
-
-# vi: ts=4 expandtab
diff --git a/tests/unittests/sources/test_upcloud.py b/tests/unittests/sources/test_upcloud.py
index 694f4084..0bab508a 100644
--- a/tests/unittests/sources/test_upcloud.py
+++ b/tests/unittests/sources/test_upcloud.py
@@ -4,7 +4,7 @@
 
 import json
 
-from cloudinit import helpers, settings, sources
+from cloudinit import helpers, importer, settings, sources
 from cloudinit.sources.DataSourceUpCloud import (
     DataSourceUpCloud,
     DataSourceUpCloudLocal,
@@ -321,6 +321,11 @@ class TestUpCloudDatasourceLoading(CiTestCase):
         ds_list = sources.DataSourceUpCloud.get_datasource_list(deps)
         self.assertEqual(ds_list, [DataSourceUpCloud])
 
+    @mock.patch.object(
+        importer,
+        "match_case_insensitive_module_name",
+        lambda name: f"DataSource{name}",
+    )
     def test_list_sources_finds_ds(self):
         found = sources.list_sources(
             ["UpCloud"],
diff --git a/tools/ds-identify b/tools/ds-identify
index cd07565d..1a94c9ab 100755
--- a/tools/ds-identify
+++ b/tools/ds-identify
@@ -834,9 +834,6 @@ dscheck_LXD() {
 
 dscheck_NoCloud() {
     local fslabel="cidata CIDATA" d=""
-    case " ${DI_KERNEL_CMDLINE} " in
-        *\ ds=nocloud*) return ${DS_FOUND};;
-    esac
     case " ${DI_DMI_PRODUCT_SERIAL} " in
         *\ ds=nocloud*) return ${DS_FOUND};;
     esac
@@ -1743,6 +1740,7 @@ read_config() {
         key=${tok%%=*}
         val=${tok#*=}
         case "$key" in
+            ds) _rc_dsname="$val";;
             ci.ds) _rc_dsname="$val";;
             ci.datasource) _rc_dsname="$val";;
             ci.di.policy) _rc_policy="$val";;