Fix regression in package installation

This commit addresses 3 issues introduced by commit 226ba25.
1. We regressed the ability to provide specific
   versions in generic package installs with the `packages:`
   directive in #cloud-config YAML.
2. Package installation failure changed to logging error rather than
   raising Exception.
3. Package lists being passed to package installation became constrained
   to `packages` schema when it was originally more open.

To fix 1, added _validate_entry function to ensure packages specified
at both the top level and under a package manager are parsed the same.

To fix 2, raise Exception rather than log error if package installation
fails. Also, change any intermediary package install failures to INFO
rather than ERROR.

To fix 3, apply typing to the pkglist accepted by `install_packages` to
(mostly) adhere to the `packages` schema defition so that we're using a
consistent interface. Call sites have been updated accordingly.

Fixes GH-4461

cloudinit/config/cc_ansible.py

@@ -136,7 +136,7 @@ def install(self, pkg_name: str):
             try:
                 import pip  # noqa: F401
             except ImportError:
-                self.distro.install_packages(self.distro.pip_package_name)
+                self.distro.install_packages([self.distro.pip_package_name])
             cmd = [
                 sys.executable,
                 "-m",
@@ -162,7 +162,7 @@ def is_installed(self) -> bool:
 class AnsiblePullDistro(AnsiblePull):
     def install(self, pkg_name: str):
         if not self.is_installed():
-            self.distro.install_packages(pkg_name)
+            self.distro.install_packages([pkg_name])
 
     def is_installed(self) -> bool:
         return bool(which("ansible"))

cloudinit/config/cc_apt_configure.py

@@ -16,7 +16,7 @@
 import shutil
 import signal
 from textwrap import dedent, indent
-from typing import Dict
+from typing import Dict, Iterable, List, Mapping
 
 from cloudinit import features, gpg, subp, templater, util
 from cloudinit.cloud import Cloud
@@ -37,7 +37,7 @@
 frequency = PER_INSTANCE
 distros = ["ubuntu", "debian"]
 
-PACKAGE_DEPENDENCY_BY_COMMAND = {
+PACKAGE_DEPENDENCY_BY_COMMAND: Mapping[str, str] = {
     "add-apt-repository": "software-properties-common",
     "gpg": "gnupg",
 }
@@ -709,8 +709,8 @@ def _ensure_dependencies(cfg, aa_repo_match, cloud):
     distro.install_packages due to a preliminary 'apt update' called before
     package installation.
     """
-    missing_packages = []
-    required_cmds = set()
+    missing_packages: List[str] = []
+    required_cmds: Iterable[str] = set()
     if util.is_false(cfg.get("preserve_sources_list", False)):
         for mirror_key in ("primary", "security"):
             if cfg.get(mirror_key):

cloudinit/config/cc_chef.py

@@ -13,6 +13,7 @@
 import logging
 import os
 from textwrap import dedent
+from typing import List
 
 from cloudinit import subp, temp_utils, templater, url_helper, util
 from cloudinit.cloud import Cloud
@@ -161,7 +162,7 @@ def get_template_params(iid, chef_cfg):
     # Allow users to overwrite any of the keys they want (if they so choose),
     # when a value is None, then the value will be set to None and no boolean
     # or string version will be populated...
-    for (k, v) in chef_cfg.items():
+    for k, v in chef_cfg.items():
         if k not in CHEF_RB_TPL_KEYS:
             LOG.debug("Skipping unknown chef template key '%s'", k)
             continue
@@ -233,7 +234,7 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
         # are associated with paths have their parent directory created
         # before they are used by the chef-client itself.
         param_paths = set()
-        for (k, v) in params.items():
+        for k, v in params.items():
             if k in CHEF_RB_TPL_PATH_KEYS and v:
                 param_paths.add(os.path.dirname(v))
         util.ensure_dirs(param_paths)
@@ -363,7 +364,7 @@ def install_chef(cloud: Cloud, chef_cfg):
         run = util.get_cfg_option_bool(chef_cfg, "exec", default=True)
     elif install_type == "packages":
         # This will install and run the chef-client from packages
-        cloud.distro.install_packages(("chef",))
+        cloud.distro.install_packages(["chef"])
     elif install_type == "omnibus":
         omnibus_version = util.get_cfg_option_str(chef_cfg, "omnibus_version")
         install_chef_from_omnibus(
@@ -378,9 +379,9 @@ def install_chef(cloud: Cloud, chef_cfg):
     return run
 
 
-def get_ruby_packages(version):
+def get_ruby_packages(version) -> List[str]:
     # return a list of packages needed to install ruby at version
-    pkgs = ["ruby%s" % version, "ruby%s-dev" % version]
+    pkgs: List[str] = ["ruby%s" % version, "ruby%s-dev" % version]
     if version == "1.8":
         pkgs.extend(("libopenssl-ruby1.8", "rubygems1.8"))
     return pkgs

cloudinit/config/cc_landscape.py

@@ -132,7 +132,7 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
         )
     if not ls_cloudcfg:
         return
-    cloud.distro.install_packages(("landscape-client",))
+    cloud.distro.install_packages(["landscape-client"])
 
     # Later order config values override earlier values
     merge_data = [

cloudinit/config/cc_mcollective.py

@@ -109,7 +109,7 @@ def configure(
                 server_cfg,
             )
             mcollective_config = ConfigObj()
-    for (cfg_name, cfg) in config.items():
+    for cfg_name, cfg in config.items():
         if cfg_name == "public-cert":
             util.write_file(pubcert_file, cfg, mode=0o644)
             mcollective_config["plugin.ssl_server_public"] = pubcert_file
@@ -127,7 +127,7 @@ def configure(
                 # it is needed and then add/or create items as needed
                 if cfg_name not in mcollective_config.sections:
                     mcollective_config[cfg_name] = {}
-                for (o, v) in cfg.items():
+                for o, v in cfg.items():
                     mcollective_config[cfg_name][o] = v
             else:
                 # Otherwise just try to convert it to a string
@@ -151,7 +151,6 @@ def configure(
 
 
 def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
-
     # If there isn't a mcollective key in the configuration don't do anything
     if "mcollective" not in cfg:
         LOG.debug(
@@ -163,7 +162,7 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
     mcollective_cfg = cfg["mcollective"]
 
     # Start by installing the mcollective package ...
-    cloud.distro.install_packages(("mcollective",))
+    cloud.distro.install_packages(["mcollective"])
 
     # ... and then update the mcollective configuration
     if "conf" in mcollective_cfg:

cloudinit/config/cc_puppet.py

@@ -11,16 +11,18 @@
 import logging
 import os
 import socket
+from contextlib import suppress
 from io import StringIO
 from textwrap import dedent
+from typing import List, Union
 
 import yaml
 
 from cloudinit import helpers, subp, temp_utils, url_helper, util
 from cloudinit.cloud import Cloud
 from cloudinit.config import Config
 from cloudinit.config.schema import MetaSchema, get_meta_doc
-from cloudinit.distros import ALL_DISTROS, Distro
+from cloudinit.distros import ALL_DISTROS, Distro, InstallerError
 from cloudinit.settings import PER_INSTANCE
 
 AIO_INSTALL_URL = "https://raw.githubusercontent.com/puppetlabs/install-puppet/main/install.sh"  # noqa: E501
@@ -237,21 +239,28 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
         )
 
         if install_type == "packages":
+            to_install: List[Union[str, List[str]]]
             if package_name is None:  # conf has no package_nam
                 for puppet_name in PUPPET_PACKAGE_NAMES:
-                    try:
-                        cloud.distro.install_packages((puppet_name, version))
+                    with suppress(InstallerError):
+                        to_install = (
+                            [[puppet_name, version]]
+                            if version
+                            else [puppet_name]
+                        )
+                        cloud.distro.install_packages(to_install)
                         package_name = puppet_name
                         break
-                    except subp.ProcessExecutionError:
-                        pass
                 if not package_name:
                     LOG.warning(
                         "No installable puppet package in any of: %s",
                         ", ".join(PUPPET_PACKAGE_NAMES),
                     )
             else:
-                cloud.distro.install_packages((package_name, version))
+                to_install = (
+                    [[package_name, version]] if version else [package_name]
+                )
+                cloud.distro.install_packages(to_install)
 
         elif install_type == "aio":
             install_puppet_aio(
@@ -289,7 +298,7 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
         puppet_config.read_file(
             StringIO(cleaned_contents), source=p_constants.conf_path
         )
-        for (cfg_name, cfg) in puppet_cfg["conf"].items():
+        for cfg_name, cfg in puppet_cfg["conf"].items():
             # Cert configuration is a special case
             # Dump the puppetserver ca certificate in the correct place
             if cfg_name == "ca_cert":
@@ -307,7 +316,7 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
             else:
                 # Iterate through the config items, we'll use ConfigParser.set
                 # to overwrite or create new items as needed
-                for (o, v) in cfg.items():
+                for o, v in cfg.items():
                     if o == "certname":
                         # Expand %f as the fqdn
                         # TODO(harlowja) should this use the cloud fqdn??

cloudinit/config/cc_salt_minion.py

@@ -116,7 +116,7 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
     const = SaltConstants(cfg=s_cfg)
 
     # Start by installing the salt package ...
-    cloud.distro.install_packages(const.pkg_name)
+    cloud.distro.install_packages([const.pkg_name])
 
     # Ensure we can configure files at the right dir
     util.ensure_dir(const.conf_dir)

cloudinit/distros/__init__.py

@@ -29,6 +29,7 @@
     Set,
     Tuple,
     Type,
+    Union,
 )
 
 import cloudinit.net.netops.iproute2 as iproute2
@@ -104,6 +105,24 @@
 # Letters/Digits/Hyphen characters, for use in domain name validation
 LDH_ASCII_CHARS = string.ascii_letters + string.digits + "-"
 
+# Before you try to go rewriting this better using Unions, read
+# https://github.com/microsoft/pyright/blob/main/docs/type-concepts.md#generic-types  # noqa: E501
+# The Immutable types mentioned there won't work for us because
+# we need to distinguish between a str and a Sequence[str]
+# This also isn't exhaustive. If you have a unique case that adheres to
+# the `packages` schema, you can add it here.
+PackageList = Union[
+    List[str],
+    List[Mapping],
+    List[List[str]],
+    List[Union[str, List[str]]],
+    List[Union[str, List[str], Mapping]],
+]
+
+
+class InstallerError(Exception):
+    pass
+
 
 class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta):
     pip_package_name = "python3-pip"
@@ -156,6 +175,17 @@ def _unpickle(self, ci_pkl_version: int) -> None:
             # missing expected instance state otherwise.
             self.networking = self.networking_cls()
 
+    def _validate_entry(self, entry):
+        if isinstance(entry, str):
+            return entry
+        elif isinstance(entry, (list, tuple)):
+            if len(entry) == 2:
+                return tuple(entry)
+        raise ValueError(
+            "Invalid 'packages' yaml specification. "
+            "Check schema definition."
+        )
+
     def _extract_package_by_manager(
         self, pkglist: Iterable
     ) -> Tuple[Dict[Type[PackageManager], Set], Set]:
@@ -169,8 +199,7 @@ def _extract_package_by_manager(
             if isinstance(entry, dict):
                 for package_manager, package_list in entry.items():
                     for definition in package_list:
-                        if isinstance(definition, list):
-                            definition = tuple(definition)
+                        definition = self._validate_entry(definition)
                         try:
                             packages_by_manager[
                                 known_package_managers[package_manager]
@@ -181,16 +210,11 @@ def _extract_package_by_manager(
                                 "not a supported package manager!",
                                 package_manager,
                             )
-            elif isinstance(entry, str):
-                generic_packages.add(entry)
             else:
-                raise ValueError(
-                    "Invalid 'packages' yaml specification. "
-                    "Check schema definition."
-                )
+                generic_packages.add(self._validate_entry(entry))
         return dict(packages_by_manager), generic_packages
 
-    def install_packages(self, pkglist):
+    def install_packages(self, pkglist: PackageList):
         error_message = (
             "Failed to install the following packages: %s. "
             "See associated package manager logs for more details."
@@ -217,23 +241,23 @@ def install_packages(self, pkglist):
                 pkg for pkg in uninstalled if pkg not in generic_packages
             }
             if failed:
-                LOG.error(error_message, failed)
+                LOG.info(error_message, failed)
             generic_packages = set(uninstalled)
 
         # Now attempt any specified package managers not explicitly supported
         # by distro
-        for manager, packages in packages_by_manager.items():
-            if manager.name in [p.name for p in self.package_managers]:
+        for manager_type, packages in packages_by_manager.items():
+            if manager_type.name in [p.name for p in self.package_managers]:
                 # We already installed/attempted these; don't try again
                 continue
             uninstalled.extend(
-                manager.from_config(self._runner, self._cfg).install_packages(
-                    pkglist=packages
-                )
+                manager_type.from_config(
+                    self._runner, self._cfg
+                ).install_packages(pkglist=packages)
             )
 
         if uninstalled:
-            LOG.error(error_message, uninstalled)
+            raise InstallerError(error_message, uninstalled)
 
     def _write_network(self, settings):
         """Deprecated. Remove if/when arch and gentoo support renderers."""

cloudinit/distros/alpine.py

@@ -69,7 +69,7 @@ def apply_locale(self, locale, out_fn=None):
         ]
         util.write_file(out_fn, "\n".join(lines), 0o644)
 
-    def install_packages(self, pkglist):
+    def install_packages(self, pkglist: distros.PackageList):
         self.update_package_sources()
         self.package_command("add", pkgs=pkglist)
 

cloudinit/distros/arch.py

@@ -8,7 +8,7 @@
 import os
 
 from cloudinit import distros, helpers, subp, util
-from cloudinit.distros import net_util
+from cloudinit.distros import PackageList, net_util
 from cloudinit.distros.parsers.hostname import HostnameConf
 from cloudinit.net.renderer import Renderer
 from cloudinit.net.renderers import RendererNotFoundError
@@ -57,7 +57,7 @@ def apply_locale(self, locale, out_fn=None):
         # https://github.com/systemd/systemd/pull/9864
         subp.subp(["localectl", "set-locale", locale], capture=False)
 
-    def install_packages(self, pkglist):
+    def install_packages(self, pkglist: PackageList):
         self.update_package_sources()
         self.package_command("", pkgs=pkglist)
 

cloudinit/distros/bsd.py

@@ -4,7 +4,7 @@
 
 import cloudinit.net.netops.bsd_netops as bsd_netops
 from cloudinit import distros, helpers, net, subp, util
-from cloudinit.distros import bsd_utils
+from cloudinit.distros import PackageList, bsd_utils
 from cloudinit.distros.networking import BSDNetworking
 
 LOG = logging.getLogger(__name__)
@@ -96,7 +96,7 @@ def generate_fallback_config(self):
             )
         return nconf
 
-    def install_packages(self, pkglist):
+    def install_packages(self, pkglist: PackageList):
         self.update_package_sources()
         self.package_command("install", pkgs=pkglist)
 

cloudinit/distros/gentoo.py

@@ -9,7 +9,7 @@
 import logging
 
 from cloudinit import distros, helpers, subp, util
-from cloudinit.distros import net_util
+from cloudinit.distros import PackageList, net_util
 from cloudinit.distros.parsers.hostname import HostnameConf
 from cloudinit.settings import PER_INSTANCE
 
@@ -56,7 +56,7 @@ def apply_locale(self, _, out_fn=None):
             ["eselect", "locale", "set", self.default_locale], capture=False
         )
 
-    def install_packages(self, pkglist):
+    def install_packages(self, pkglist: PackageList):
         self.update_package_sources()
         self.package_command("", pkgs=pkglist)