cc_puppet: Update puppet service name (#1970)

cc_puppet: Update puppet service name to puppet-agent

From Lunar, we see that the default puppet version is 7.20
which replaces `puppet.service` with `puppet-agent.service`.
Thus, we need to have a way of calling the appropriate
service depending on the distribution of puppet installed.

Attempt to install, start or enable puppet-agent first and fallback
to puppet.
Log warnings if neither preferred package names exist or if the
package_name in user-data is not able to be configured.

LP: #2002969

3 files changed, 157 insertions, 103 deletions

diff --git a/cloudinit/config/cc_puppet.py b/cloudinit/config/cc_puppet.py
index b8a9fe17..38c2cc99 100644
--- a/cloudinit/config/cc_puppet.py
+++ b/cloudinit/config/cc_puppet.py
@@ -25,6 +25,7 @@ from cloudinit.settings import PER_INSTANCE
 
 AIO_INSTALL_URL = "https://raw.githubusercontent.com/puppetlabs/install-puppet/main/install.sh"  # noqa: E501
 PUPPET_AGENT_DEFAULT_ARGS = ["--test"]
+PUPPET_PACKAGE_NAMES = ("puppet-agent", "puppet")
 
 MODULE_DESCRIPTION = """\
 This module handles puppet installation and configuration. If the ``puppet``
@@ -118,26 +119,21 @@ class PuppetConstants:
         self.csr_attributes_path = csr_attributes_path
 
 
-def _autostart_puppet(log):
-    # Set puppet to automatically start
-    if os.path.exists("/etc/default/puppet"):
-        subp.subp(
-            [
-                "sed",
-                "-i",
-                "-e",
-                "s/^START=.*/START=yes/",
-                "/etc/default/puppet",
-            ],
-            capture=False,
-        )
-    elif subp.which("systemctl"):
-        subp.subp(["systemctl", "enable", "puppet.service"], capture=False)
-    elif os.path.exists("/sbin/chkconfig"):
-        subp.subp(["/sbin/chkconfig", "puppet", "on"], capture=False)
-    else:
+def _manage_puppet_services(log, cloud: Cloud, action: str):
+    """Attempts to perform action on one of the puppet services"""
+    service_managed: str = ""
+    for puppet_name in PUPPET_PACKAGE_NAMES:
+        try:
+            cloud.distro.manage_service(action, f"{puppet_name}.service")
+            service_managed = puppet_name
+            break
+        except subp.ProcessExecutionError:
+            pass
+    if not service_managed:
         log.warning(
-            "Sorry we do not know how to enable puppet services on this system"
+            "Could not '%s' any of the following services: %s",
+            action,
+            ", ".join(PUPPET_PACKAGE_NAMES),
         )
 
 
@@ -221,7 +217,7 @@ def handle(
     else:  # default to 'packages'
         puppet_user = "puppet"
         puppet_bin = "puppet"
-        puppet_package = "puppet"
+        puppet_package = None  # changes with distro
 
     package_name = util.get_cfg_option_str(
         puppet_cfg, "package_name", puppet_package
@@ -238,7 +234,22 @@ def handle(
         )
 
         if install_type == "packages":
-            cloud.distro.install_packages((package_name, version))
+            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))
+                        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))
+
         elif install_type == "aio":
             install_puppet_aio(
                 cloud.distro, aio_install_url, version, collection, cleanup
@@ -316,9 +327,9 @@ def handle(
             yaml.dump(puppet_cfg["csr_attributes"], default_flow_style=False),
         )
 
-    # Set it up so it autostarts
     if start_puppetd:
-        _autostart_puppet(log)
+        # Enables the services
+        _manage_puppet_services(log, cloud, "enable")
 
     # Run the agent if needed
     if run:
@@ -344,7 +355,7 @@ def handle(
 
     if start_puppetd:
         # Start puppetd
-        subp.subp(["service", "puppet", "start"], capture=False)
+        _manage_puppet_services(log, cloud, "start")
 
 
 # vi: ts=4 expandtab
diff --git a/tests/integration_tests/modules/test_puppet.py b/tests/integration_tests/modules/test_puppet.py
index 1bd9cee4..b8613866 100644
--- a/tests/integration_tests/modules/test_puppet.py
+++ b/tests/integration_tests/modules/test_puppet.py
@@ -17,7 +17,12 @@ def test_puppet_service(client: IntegrationInstance):
     """Basic test that puppet gets installed and runs."""
     log = client.read_from_file("/var/log/cloud-init.log")
     verify_clean_log(log)
-    assert client.execute("systemctl is-active puppet").ok
+    puppet_ok = client.execute("systemctl is-active puppet.service").ok
+    puppet_agent_ok = client.execute(
+        "systemctl is-active puppet-agent.service"
+    ).ok
+    assert True in [puppet_ok, puppet_agent_ok]
+    assert False in [puppet_ok, puppet_agent_ok]
     assert "Running command ['puppet', 'agent'" not in log
 
 
diff --git a/tests/unittests/config/test_cc_puppet.py b/tests/unittests/config/test_cc_puppet.py
index 27a49722..23461c2b 100644
--- a/tests/unittests/config/test_cc_puppet.py
+++ b/tests/unittests/config/test_cc_puppet.py
@@ -12,6 +12,7 @@ from cloudinit.config.schema import (
     get_schema,
     validate_cloudconfig_schema,
 )
+from cloudinit.subp import ProcessExecutionError
 from tests.unittests.helpers import CiTestCase, mock, skipUnlessJsonSchema
 from tests.unittests.util import get_cloud
 
@@ -25,67 +26,55 @@ def fake_tempdir(mocker, tmpdir):
     ).return_value.__enter__.return_value = str(tmpdir)
 
 
-@mock.patch("cloudinit.config.cc_puppet.subp.which")
 @mock.patch("cloudinit.config.cc_puppet.subp.subp")
-@mock.patch("cloudinit.config.cc_puppet.os")
-class TestAutostartPuppet(CiTestCase):
-    def test_wb_autostart_puppet_updates_puppet_default(
-        self, m_os, m_subp, m_subpw
-    ):
-        """Update /etc/default/puppet to autostart if it exists."""
-
-        def _fake_exists(path):
-            return path == "/etc/default/puppet"
-
-        m_os.path.exists.side_effect = _fake_exists
-        cc_puppet._autostart_puppet(LOG)
-        self.assertEqual(
-            [
-                mock.call(
-                    [
-                        "sed",
-                        "-i",
-                        "-e",
-                        "s/^START=.*/START=yes/",
-                        "/etc/default/puppet",
-                    ],
-                    capture=False,
-                )
-            ],
-            m_subp.call_args_list,
-        )
+class TestManagePuppetServices(CiTestCase):
+    def setUp(self):
+        super(TestManagePuppetServices, self).setUp()
+        self.cloud = get_cloud()
 
-    def test_wb_autostart_pupppet_enables_puppet_systemctl(
-        self, m_os, m_subp, m_subpw
+    def test_wb_manage_puppet_services_enables_puppet_systemctl(
+        self,
+        m_subp,
     ):
-        """If systemctl is present, enable puppet via systemctl."""
-
-        m_os.path.exists.return_value = False
-        m_subpw.return_value = "/usr/bin/systemctl"
-        cc_puppet._autostart_puppet(LOG)
+        cc_puppet._manage_puppet_services(LOG, self.cloud, "enable")
         expected_calls = [
-            mock.call(["systemctl", "enable", "puppet.service"], capture=False)
+            mock.call(
+                ["systemctl", "enable", "puppet-agent.service"],
+                capture=True,
+            )
         ]
-        self.assertEqual(expected_calls, m_subp.call_args_list)
+        self.assertIn(expected_calls, m_subp.call_args_list)
 
-    def test_wb_autostart_pupppet_enables_puppet_chkconfig(
-        self, m_os, m_subp, m_subpw
+    def test_wb_manage_puppet_services_starts_puppet_systemctl(
+        self,
+        m_subp,
     ):
-        """If chkconfig is present, enable puppet via checkcfg."""
-
-        def _fake_exists(path):
-            return path == "/sbin/chkconfig"
+        cc_puppet._manage_puppet_services(LOG, self.cloud, "start")
+        expected_calls = [
+            mock.call(
+                ["systemctl", "start", "puppet-agent.service"],
+                capture=True,
+            )
+        ]
+        self.assertIn(expected_calls, m_subp.call_args_list)
 
-        m_subpw.return_value = None
-        m_os.path.exists.side_effect = _fake_exists
-        cc_puppet._autostart_puppet(LOG)
+    def test_enable_fallback_on_failure(self, m_subp):
+        m_subp.side_effect = (ProcessExecutionError, 0)
+        cc_puppet._manage_puppet_services(LOG, self.cloud, "enable")
         expected_calls = [
-            mock.call(["/sbin/chkconfig", "puppet", "on"], capture=False)
+            mock.call(
+                ["systemctl", "enable", "puppet-agent.service"],
+                capture=True,
+            ),
+            mock.call(
+                ["systemctl", "enable", "puppet.service"],
+                capture=True,
+            ),
         ]
         self.assertEqual(expected_calls, m_subp.call_args_list)
 
 
-@mock.patch("cloudinit.config.cc_puppet._autostart_puppet")
+@mock.patch("cloudinit.config.cc_puppet._manage_puppet_services")
 class TestPuppetHandle(CiTestCase):
 
     with_logs = True
@@ -97,35 +86,36 @@ class TestPuppetHandle(CiTestCase):
         self.csr_attributes_path = self.tmp_path("csr_attributes.yaml")
         self.cloud = get_cloud()
 
-    def test_skips_missing_puppet_key_in_cloudconfig(self, m_auto):
+    def test_skips_missing_puppet_key_in_cloudconfig(self, m_man_puppet):
         """Cloud-config containing no 'puppet' key is skipped."""
 
         cfg = {}
         cc_puppet.handle("notimportant", cfg, self.cloud, LOG, None)
         self.assertIn("no 'puppet' configuration found", self.logs.getvalue())
-        self.assertEqual(0, m_auto.call_count)
+        self.assertEqual(0, m_man_puppet.call_count)
 
     @mock.patch("cloudinit.config.cc_puppet.subp.subp", return_value=("", ""))
-    def test_puppet_config_starts_puppet_service(self, m_subp, m_auto):
+    def test_puppet_config_starts_puppet_service(self, m_subp, m_man_puppet):
         """Cloud-config 'puppet' configuration starts puppet."""
 
         cfg = {"puppet": {"install": False}}
         cc_puppet.handle("notimportant", cfg, self.cloud, LOG, None)
-        self.assertEqual(1, m_auto.call_count)
-        self.assertIn(
-            [mock.call(["service", "puppet", "start"], capture=False)],
-            m_subp.call_args_list,
-        )
+        self.assertEqual(2, m_man_puppet.call_count)
+        expected_calls = [
+            mock.call(LOG, self.cloud, "enable"),
+            mock.call(LOG, self.cloud, "start"),
+        ]
+        self.assertEqual(expected_calls, m_man_puppet.call_args_list)
 
     @mock.patch("cloudinit.config.cc_puppet.subp.subp", return_value=("", ""))
-    def test_empty_puppet_config_installs_puppet(self, m_subp, m_auto):
+    def test_empty_puppet_config_installs_puppet(self, m_subp, m_man_puppet):
         """Cloud-config empty 'puppet' configuration installs latest puppet."""
 
         self.cloud.distro = mock.MagicMock()
         cfg = {"puppet": {}}
         cc_puppet.handle("notimportant", cfg, self.cloud, LOG, None)
         self.assertEqual(
-            [mock.call(("puppet", None))],
+            [mock.call(("puppet-agent", None))],
             self.cloud.distro.install_packages.call_args_list,
         )
 
@@ -136,8 +126,8 @@ class TestPuppetHandle(CiTestCase):
         self.cloud.distro = mock.MagicMock()
         cfg = {"puppet": {"install": True}}
         cc_puppet.handle("notimportant", cfg, self.cloud, LOG, None)
-        self.assertEqual(
-            [mock.call(("puppet", None))],
+        self.assertIn(
+            [mock.call(("puppet-agent", None))],
             self.cloud.distro.install_packages.call_args_list,
         )
 
@@ -246,14 +236,14 @@ class TestPuppetHandle(CiTestCase):
         cfg = {"puppet": {"version": "3.8"}}
         cc_puppet.handle("notimportant", cfg, self.cloud, LOG, None)
         self.assertEqual(
-            [mock.call(("puppet", "3.8"))],
+            [mock.call(("puppet-agent", "3.8"))],
             self.cloud.distro.install_packages.call_args_list,
         )
 
     @mock.patch("cloudinit.config.cc_puppet.get_config_value")
     @mock.patch("cloudinit.config.cc_puppet.subp.subp", return_value=("", ""))
     def test_puppet_config_updates_puppet_conf(
-        self, m_subp, m_default, m_auto
+        self, m_subp, m_default, m_man_puppet
     ):
         """When 'conf' is provided update values in PUPPET_CONF_PATH."""
 
@@ -277,7 +267,7 @@ class TestPuppetHandle(CiTestCase):
     @mock.patch("cloudinit.config.cc_puppet.get_config_value")
     @mock.patch("cloudinit.config.cc_puppet.subp.subp")
     def test_puppet_writes_csr_attributes_file(
-        self, m_subp, m_default, m_auto
+        self, m_subp, m_default, m_man_puppet
     ):
         """When csr_attributes is provided
         creates file in PUPPET_CSR_ATTRIBUTES_PATH."""
@@ -321,44 +311,50 @@ class TestPuppetHandle(CiTestCase):
         self.assertEqual(expected, content)
 
     @mock.patch("cloudinit.config.cc_puppet.subp.subp", return_value=("", ""))
-    def test_puppet_runs_puppet_if_requested(self, m_subp, m_auto):
+    def test_puppet_runs_puppet_if_requested(self, m_subp, m_man_puppet):
         """Run puppet with default args if 'exec' is set to True."""
 
         cfg = {"puppet": {"exec": True}}
         cc_puppet.handle("notimportant", cfg, self.cloud, LOG, None)
-        self.assertEqual(1, m_auto.call_count)
+        self.assertEqual(2, m_man_puppet.call_count)
+        expected_calls = [
+            mock.call(LOG, self.cloud, "enable"),
+            mock.call(LOG, self.cloud, "start"),
+        ]
+        self.assertEqual(expected_calls, m_man_puppet.call_args_list)
         self.assertIn(
             [mock.call(["puppet", "agent", "--test"], capture=False)],
             m_subp.call_args_list,
         )
 
     @mock.patch("cloudinit.config.cc_puppet.subp.subp", return_value=("", ""))
-    def test_puppet_starts_puppetd(self, m_subp, m_auto):
+    def test_puppet_starts_puppetd(self, m_subp, m_man_puppet):
         """Run puppet with default args if 'exec' is set to True."""
 
         cfg = {"puppet": {}}
         cc_puppet.handle("notimportant", cfg, self.cloud, LOG, None)
-        self.assertEqual(1, m_auto.call_count)
-        self.assertIn(
-            [mock.call(["service", "puppet", "start"], capture=False)],
-            m_subp.call_args_list,
-        )
+        self.assertEqual(2, m_man_puppet.call_count)
+        expected_calls = [
+            mock.call(LOG, self.cloud, "enable"),
+            mock.call(LOG, self.cloud, "start"),
+        ]
+        self.assertEqual(expected_calls, m_man_puppet.call_args_list)
 
     @mock.patch("cloudinit.config.cc_puppet.subp.subp", return_value=("", ""))
-    def test_puppet_skips_puppetd(self, m_subp, m_auto):
+    def test_puppet_skips_puppetd(self, m_subp, m_man_puppet):
         """Run puppet with default args if 'exec' is set to True."""
 
         cfg = {"puppet": {"start_service": False}}
         cc_puppet.handle("notimportant", cfg, self.cloud, LOG, None)
-        self.assertEqual(0, m_auto.call_count)
+        self.assertEqual(0, m_man_puppet.call_count)
         self.assertNotIn(
-            [mock.call(["service", "puppet", "start"], capture=False)],
+            [mock.call(["systemctl", "start", "puppet-agent"], capture=False)],
             m_subp.call_args_list,
         )
 
     @mock.patch("cloudinit.config.cc_puppet.subp.subp", return_value=("", ""))
     def test_puppet_runs_puppet_with_args_list_if_requested(
-        self, m_subp, m_auto
+        self, m_subp, m_man_puppet
     ):
         """Run puppet with 'exec_args' list if 'exec' is set to True."""
 
@@ -369,7 +365,7 @@ class TestPuppetHandle(CiTestCase):
             }
         }
         cc_puppet.handle("notimportant", cfg, self.cloud, LOG, None)
-        self.assertEqual(1, m_auto.call_count)
+        self.assertEqual(2, m_man_puppet.call_count)
         self.assertIn(
             [
                 mock.call(
@@ -382,7 +378,7 @@ class TestPuppetHandle(CiTestCase):
 
     @mock.patch("cloudinit.config.cc_puppet.subp.subp", return_value=("", ""))
     def test_puppet_runs_puppet_with_args_string_if_requested(
-        self, m_subp, m_auto
+        self, m_subp, m_man_puppet
     ):
         """Run puppet with 'exec_args' string if 'exec' is set to True."""
 
@@ -393,7 +389,7 @@ class TestPuppetHandle(CiTestCase):
             }
         }
         cc_puppet.handle("notimportant", cfg, self.cloud, LOG, None)
-        self.assertEqual(1, m_auto.call_count)
+        self.assertEqual(2, m_man_puppet.call_count)
         self.assertIn(
             [
                 mock.call(
@@ -404,6 +400,48 @@ class TestPuppetHandle(CiTestCase):
             m_subp.call_args_list,
         )
 
+    @mock.patch("cloudinit.config.cc_puppet.subp.subp", return_value=("", ""))
+    def test_puppet_falls_back_to_older_name(self, m_subp, m_man_puppet):
+        cfg = {"puppet": {}}
+        with mock.patch(
+            "tests.unittests.util.MockDistro.install_packages"
+        ) as install_pkg:
+            # puppet-agent not installed, but puppet is
+            install_pkg.side_effect = (ProcessExecutionError, 0)
+
+            cc_puppet.handle("notimportant", cfg, self.cloud, LOG, None)
+            expected_calls = [
+                mock.call(LOG, self.cloud, "enable"),
+                mock.call(LOG, self.cloud, "start"),
+            ]
+            self.assertEqual(expected_calls, m_man_puppet.call_args_list)
+
+    @mock.patch("cloudinit.config.cc_puppet.subp.subp", return_value=("", ""))
+    def test_puppet_with_conf_package_name_fails(self, m_subp, m_man_puppet):
+        cfg = {"puppet": {"package_name": "puppet"}}
+        with mock.patch(
+            "tests.unittests.util.MockDistro.install_packages"
+        ) as install_pkg:
+            # puppet-agent not installed, but puppet is
+            install_pkg.side_effect = (ProcessExecutionError, 0)
+            with pytest.raises(ProcessExecutionError):
+                cc_puppet.handle("notimportant", cfg, self.cloud, LOG, None)
+            self.assertEqual(0, m_man_puppet.call_count)
+            self.assertNotIn(
+                [
+                    mock.call(
+                        ["systemctl", "start", "puppet-agent"], capture=True
+                    )
+                ],
+                m_subp.call_args_list,
+            )
+
+    @mock.patch("cloudinit.config.cc_puppet.subp.subp", return_value=("", ""))
+    def test_puppet_with_conf_package_name_success(self, m_subp, m_man_puppet):
+        cfg = {"puppet": {"package_name": "puppet"}}
+        cc_puppet.handle("notimportant", cfg, self.cloud, LOG, None)
+        self.assertEqual(2, m_man_puppet.call_count)
+
 
 URL_MOCK = mock.Mock()
 URL_MOCK.contents = b'#!/bin/bash\necho "Hi Mom"'