cc_ntp: add support for BSDs

[igalic]

Proposed Commit Message

cc_ntp: add support for BSDs

*BSDs have ntpd installed in base the base system
This PR extends cc_ntp to add support for ntpd,
openntpd, and chrony on the FreeBSD, and OpenBSD.

To make tests pass, we ensure that we are mocking functions,
not entire classes.

Co-authored-by: Ryan Harper <rharper@woxford.com>
Co-authored-by: James Falcon <james.falcon@canonical.com>

Sponsored by: FreeBSD Foundation

LP: #1990041

Additional Context

This PR depends on #1758

Test Steps

• start a cloud-init vm with *BSD
• configure ntp
• observe results

Checklist:

• My code follows the process laid out in the documentation
• I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

[igalic]

so far, I've successfully tested ntpd from FreeBSD base, and openntpd from ports (with base ntpd still running)
tests with chrony are still outstanding.

n.b.: cloud-init won't be able to replace openntpd, with chrony, or vice-versa — and that's not the intention of this PR.

[igalic]

tested Chrony too: ✅

now to add actual tests.

[TheRealFalcon]

I don't understand why we're changing modules we're importing from.

[TheRealFalcon]

Nit: if the config is empty, I believe it can be omitted (but we should test first before removing it).

[igalic]

maybe let's postpone this until i have infrastructure for better testing on OpenBSD, including https://bugs.launchpad.net/cloud-init/+bug/1992853

[TheRealFalcon]

This patch work for you?

diff --git a/cloudinit/distros/bsd.py b/cloudinit/distros/bsd.py
index a38f0edd5..c4e1e15c6 100644
--- a/cloudinit/distros/bsd.py
+++ b/cloudinit/distros/bsd.py
@@ -3,6 +3,7 @@ from typing import List, Optional
 
 from cloudinit import distros, helpers
 from cloudinit import log as logging
+from cloudinit import net, subp, util
 from cloudinit.distros import bsd_utils
 from cloudinit.distros.networking import BSDNetworking
 
@@ -50,20 +51,20 @@ class BSD(distros.Distro):
         bsd_utils.set_rc_config_value("hostname", hostname, fn="/etc/rc.conf")
 
     def create_group(self, name, members=None):
-        if distros.util.is_group(name):
+        if util.is_group(name):
             LOG.warning("Skipping creation of existing group '%s'", name)
         else:
             group_add_cmd = self.group_add_cmd_prefix + [name]
             try:
-                distros.subp.subp(group_add_cmd)
+                subp.subp(group_add_cmd)
                 LOG.info("Created new group %s", name)
             except Exception:
-                distros.util.logexc(LOG, "Failed to create group %s", name)
+                util.logexc(LOG, "Failed to create group %s", name)
 
         if not members:
             members = []
         for member in members:
-            if not distros.util.is_user(member):
+            if not util.is_user(member):
                 LOG.warning(
                     "Unable to add group member '%s' to group '%s'"
                     "; user does not exist.",
@@ -72,18 +73,16 @@ class BSD(distros.Distro):
                 )
                 continue
             try:
-                distros.subp.subp(
-                    self._get_add_member_to_group_cmd(member, name)
-                )
+                subp.subp(self._get_add_member_to_group_cmd(member, name))
                 LOG.info("Added user '%s' to group '%s'", member, name)
             except Exception:
-                distros.util.logexc(
+                util.logexc(
                     LOG, "Failed to add user '%s' to group '%s'", member, name
                 )
 
     def generate_fallback_config(self):
         nconf = {"config": [], "version": 1}
-        for mac, name in distros.net.get_interfaces_by_mac().items():
+        for mac, name in net.get_interfaces_by_mac().items():
             nconf["config"].append(
                 {
                     "type": "physical",
@@ -124,11 +123,11 @@ class BSD(distros.Distro):
         elif args and isinstance(args, list):
             cmd.extend(args)
 
-        pkglist = distros.util.expand_package_list("%s-%s", pkgs)
+        pkglist = util.expand_package_list("%s-%s", pkgs)
         cmd.extend(pkglist)
 
         # Allow the output of this to flow outwards (ie not be captured)
-        distros.subp.subp(cmd, env=self._get_pkg_cmd_environ(), capture=False)
+        subp.subp(cmd, env=self._get_pkg_cmd_environ(), capture=False)
 
     def set_timezone(self, tz):
         distros.set_etc_timezone(tz=tz, tz_file=self._find_tz_file(tz))
diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py
index bb50232be..0982b2ead 100644
--- a/cloudinit/distros/freebsd.py
+++ b/cloudinit/distros/freebsd.py
@@ -8,8 +8,8 @@ import os
 import re
 from io import StringIO
 
-from cloudinit import distros
 from cloudinit import log as logging
+from cloudinit import subp, util
 from cloudinit.distros import bsd
 from cloudinit.distros.networking import FreeBSDNetworking
 from cloudinit.settings import PER_INSTANCE
@@ -57,13 +57,13 @@ class Distro(bsd.BSD):
             "status": [service, "status"],
         }
         cmd = list(init_cmd) + list(cmds[action])
-        return distros.subp.subp(cmd, capture=True)
+        return subp.subp(cmd, capture=True)
 
     def _get_add_member_to_group_cmd(self, member_name, group_name):
         return ["pw", "usermod", "-n", member_name, "-G", group_name]
 
     def add_user(self, name, **kwargs):
-        if distros.util.is_user(name):
+        if util.is_user(name):
             LOG.info("User %s already exists, skipping.", name)
             return False
 
@@ -109,9 +109,9 @@ class Distro(bsd.BSD):
         # Run the command
         LOG.info("Adding user %s", name)
         try:
-            distros.subp.subp(pw_useradd_cmd, logstring=log_pw_useradd_cmd)
+            subp.subp(pw_useradd_cmd, logstring=log_pw_useradd_cmd)
         except Exception:
-            distros.util.logexc(LOG, "Failed to create user %s", name)
+            util.logexc(LOG, "Failed to create user %s", name)
             raise
         # Set the password if it is provided
         # For security consideration, only hashed passwd is assumed
@@ -121,11 +121,9 @@ class Distro(bsd.BSD):
 
     def expire_passwd(self, user):
         try:
-            distros.subp.subp(["pw", "usermod", user, "-p", "01-Jan-1970"])
+            subp.subp(["pw", "usermod", user, "-p", "01-Jan-1970"])
         except Exception:
-            distros.util.logexc(
-                LOG, "Failed to set pw expiration for %s", user
-            )
+            util.logexc(LOG, "Failed to set pw expiration for %s", user)
             raise
 
     def set_passwd(self, user, passwd, hashed=False):
@@ -135,47 +133,47 @@ class Distro(bsd.BSD):
             hash_opt = "-h"
 
         try:
-            distros.subp.subp(
+            subp.subp(
                 ["pw", "usermod", user, hash_opt, "0"],
                 data=passwd,
                 logstring="chpasswd for %s" % user,
             )
         except Exception:
-            distros.util.logexc(LOG, "Failed to set password for %s", user)
+            util.logexc(LOG, "Failed to set password for %s", user)
             raise
 
     def lock_passwd(self, name):
         try:
-            distros.subp.subp(["pw", "usermod", name, "-h", "-"])
+            subp.subp(["pw", "usermod", name, "-h", "-"])
         except Exception:
-            distros.util.logexc(LOG, "Failed to lock user %s", name)
+            util.logexc(LOG, "Failed to lock user %s", name)
             raise
 
     def apply_locale(self, locale, out_fn=None):
         # Adjust the locales value to the new value
         newconf = StringIO()
-        for line in distros.util.load_file(self.login_conf_fn).splitlines():
+        for line in util.load_file(self.login_conf_fn).splitlines():
             newconf.write(
                 re.sub(r"^default:", r"default:lang=%s:" % locale, line)
             )
             newconf.write("\n")
 
         # Make a backup of login.conf.
-        distros.util.copy(self.login_conf_fn, self.login_conf_fn_bak)
+        util.copy(self.login_conf_fn, self.login_conf_fn_bak)
 
         # And write the new login.conf.
-        distros.util.write_file(self.login_conf_fn, newconf.getvalue())
+        util.write_file(self.login_conf_fn, newconf.getvalue())
 
         try:
             LOG.debug("Running cap_mkdb for %s", locale)
-            distros.subp.subp(["cap_mkdb", self.login_conf_fn])
-        except distros.subp.ProcessExecutionError:
+            subp.subp(["cap_mkdb", self.login_conf_fn])
+        except subp.ProcessExecutionError:
             # cap_mkdb failed, so restore the backup.
-            distros.util.logexc(LOG, "Failed to apply locale %s", locale)
+            util.logexc(LOG, "Failed to apply locale %s", locale)
             try:
-                distros.util.copy(self.login_conf_fn_bak, self.login_conf_fn)
+                util.copy(self.login_conf_fn_bak, self.login_conf_fn)
             except IOError:
-                distros.util.logexc(
+                util.logexc(
                     LOG, "Failed to restore %s backup", self.login_conf_fn
                 )
 
diff --git a/cloudinit/distros/netbsd.py b/cloudinit/distros/netbsd.py
index 9f7540824..3b05de2b7 100644
--- a/cloudinit/distros/netbsd.py
+++ b/cloudinit/distros/netbsd.py
@@ -8,6 +8,7 @@ import platform
 
 from cloudinit import distros
 from cloudinit import log as logging
+from cloudinit import subp, util
 from cloudinit.distros import bsd
 
 LOG = logging.getLogger(__name__)
@@ -38,7 +39,7 @@ class NetBSD(bsd.BSD):
         return ["usermod", "-G", group_name, member_name]
 
     def add_user(self, name, **kwargs):
-        if distros.util.is_user(name):
+        if util.is_user(name):
             LOG.info("User %s already exists, skipping.", name)
             return False
 
@@ -76,9 +77,9 @@ class NetBSD(bsd.BSD):
         # Run the command
         LOG.info("Adding user %s", name)
         try:
-            distros.subp.subp(adduser_cmd, logstring=log_adduser_cmd)
+            subp.subp(adduser_cmd, logstring=log_adduser_cmd)
         except Exception:
-            distros.util.logexc(LOG, "Failed to create user %s", name)
+            util.logexc(LOG, "Failed to create user %s", name)
             raise
         # Set the password if it is provided
         # For security consideration, only hashed passwd is assumed
@@ -94,33 +95,33 @@ class NetBSD(bsd.BSD):
             hashed_pw = crypt.crypt(passwd, crypt.mksalt(method))
 
         try:
-            distros.subp.subp(["usermod", "-p", hashed_pw, user])
+            subp.subp(["usermod", "-p", hashed_pw, user])
         except Exception:
-            distros.util.logexc(LOG, "Failed to set password for %s", user)
+            util.logexc(LOG, "Failed to set password for %s", user)
             raise
         self.unlock_passwd(user)
 
     def force_passwd_change(self, user):
         try:
-            distros.subp.subp(["usermod", "-F", user])
+            subp.subp(["usermod", "-F", user])
         except Exception:
-            distros.util.logexc(
+            util.logexc(
                 LOG, "Failed to set pw expiration for %s", user
             )
             raise
 
     def lock_passwd(self, name):
         try:
-            distros.subp.subp(["usermod", "-C", "yes", name])
+            subp.subp(["usermod", "-C", "yes", name])
         except Exception:
-            distros.util.logexc(LOG, "Failed to lock user %s", name)
+            util.logexc(LOG, "Failed to lock user %s", name)
             raise
 
     def unlock_passwd(self, name):
         try:
-            distros.subp.subp(["usermod", "-C", "no", name])
+            subp.subp(["usermod", "-C", "no", name])
         except Exception:
-            distros.util.logexc(LOG, "Failed to unlock user %s", name)
+            util.logexc(LOG, "Failed to unlock user %s", name)
             raise
 
     def apply_locale(self, locale, out_fn=None):
diff --git a/cloudinit/distros/openbsd.py b/cloudinit/distros/openbsd.py
index 10569600d..869b25b4b 100644
--- a/cloudinit/distros/openbsd.py
+++ b/cloudinit/distros/openbsd.py
@@ -4,8 +4,8 @@
 
 import os
 
-from cloudinit import distros
 from cloudinit import log as logging
+from cloudinit import subp, util
 from cloudinit.distros import netbsd
 
 LOG = logging.getLogger(__name__)
@@ -16,11 +16,11 @@ class Distro(netbsd.NetBSD):
     init_cmd = ["rcctl"]
 
     def _read_hostname(self, filename, default=None):
-        return distros.util.load_file(self.hostname_conf_fn)
+        return util.load_file(self.hostname_conf_fn)
 
     def _write_hostname(self, hostname, filename):
         content = hostname + "\n"
-        distros.util.write_file(self.hostname_conf_fn, content)
+        util.write_file(self.hostname_conf_fn, content)
 
     def _get_add_member_to_group_cmd(self, member_name, group_name):
         return ["usermod", "-G", group_name, member_name]
@@ -43,13 +43,13 @@ class Distro(netbsd.NetBSD):
             "status": ["check", service],
         }
         cmd = list(init_cmd) + list(cmds[action])
-        return distros.subp.subp(cmd, capture=True)
+        return subp.subp(cmd, capture=True)
 
     def lock_passwd(self, name):
         try:
-            distros.subp.subp(["usermod", "-p", "*", name])
+            subp.subp(["usermod", "-p", "*", name])
         except Exception:
-            distros.util.logexc(LOG, "Failed to lock user %s", name)
+            util.logexc(LOG, "Failed to lock user %s", name)
             raise
 
     def unlock_passwd(self, name):
diff --git a/tests/unittests/config/test_cc_ntp.py b/tests/unittests/config/test_cc_ntp.py
index 0035b7ca6..492158f9b 100644
--- a/tests/unittests/config/test_cc_ntp.py
+++ b/tests/unittests/config/test_cc_ntp.py
@@ -426,15 +426,15 @@ class TestNtp(FilesystemMockingTestCase):
             cc_ntp.handle("notimportant", cfg, mycloud, None, None)
             self.assertEqual(0, m_select.call_count)
 
-    @mock.patch("cloudinit.distros.subp")
-    @mock.patch("cloudinit.config.cc_ntp.subp")
+    @mock.patch("cloudinit.subp.subp")
+    @mock.patch("cloudinit.subp.which", return_value=True)
     @mock.patch("cloudinit.config.cc_ntp.select_ntp_client")
     @mock.patch("cloudinit.distros.Distro.uses_systemd")
-    def test_ntp_the_whole_package(self, m_sysd, m_select, m_subp, m_dsubp):
+    def test_ntp_the_whole_package(self, m_sysd, m_select, m_which, m_subp):
         """Test enabled config renders template, and restarts service"""
         cfg = {"ntp": {"enabled": True}}
         for distro in cc_ntp.distros:
-            m_dsubp.reset_mock()
+            m_subp.reset_mock()
             mycloud = self._get_cloud(distro)
             ntpconfig = self._mock_ntp_client_config(distro=distro)
             confpath = ntpconfig["confpath"]
@@ -482,7 +482,6 @@ class TestNtp(FilesystemMockingTestCase):
                 # allow use of util.mergemanydict
                 m_util.mergemanydict.side_effect = util.mergemanydict
                 # default client is present
-                m_subp.which.return_value = True
                 # use the config 'enabled' value
                 m_util.is_false.return_value = util.is_false(
                     cfg["ntp"]["enabled"]
@@ -491,9 +490,7 @@ class TestNtp(FilesystemMockingTestCase):
                 m_util.is_FreeBSD.return_value = is_FreeBSD
                 m_util.is_OpenBSD.return_value = is_OpenBSD
                 cc_ntp.handle("notimportant", cfg, mycloud, None, None)
-                m_dsubp.subp.assert_called_with(
-                    expected_service_call, capture=True
-                )
+                m_subp.assert_called_with(expected_service_call, capture=True)
 
             self.assertEqual(expected_content, util.load_file(confpath))
 

[TheRealFalcon]

This should fix the last failing unit test:

diff --git a/tests/unittests/test_cli.py b/tests/unittests/test_cli.py
index 04f5f4573..c585efd56 100644
--- a/tests/unittests/test_cli.py
+++ b/tests/unittests/test_cli.py
@@ -247,9 +247,9 @@ class TestCLI:
                 [
                     "**Supported distros:** all",
                     "**Supported distros:** almalinux, alpine, centos, "
-                    "cloudlinux, debian, eurolinux, fedora, miraclelinux, "
-                    "openEuler, openmandriva, opensuse, photon, rhel, rocky, "
-                    "sles, ubuntu, virtuozzo",
+                    "cloudlinux, debian, eurolinux, fedora, freebsd, "
+                    "miraclelinux, openbsd, openEuler, openmandriva, "
+                    "opensuse, photon, rhel, rocky, sles, ubuntu, virtuozzo",
                     "**Config schema**:\n    **resize_rootfs:** "
                     "(``true``/``false``/``noblock``)",
                     "**Examples**::\n\n    runcmd:\n        - [ ls, -l, / ]\n",

[TheRealFalcon]

LGTM, thanks!