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 the distro classes are using
distro util functions, this means that our mock objects will b
grabbing onto the correct thing.

Co-authored-by: Ryan Harper <[email protected]>

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:

• [x] My code follows the process laid out in the documentation
• [ ] I have updated or added any unit tests accordingly
• [x] 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]

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",