bsd: refactor common netbsd and freebsd method up in BSD distro class

There is a lot of duplication of code that could be shared between
the FreeBSD and NetBSD classes. Let's avoid as much boilerplate
cut-n-paste as possible.

Move the following methods up into the parent BSD class:
 * create_group which now uses group_add_cmd_prefix and
   _get_add_member_to_group_cmd to specialize subclass commands
 * install_packages can be generalized since NetBSD.update_package_sources
   is nothing
 * package_command is generalized with minor command diffs sourced from
   class attributes pkg_cmd_install_prefix and pkg_cmd_remove_prefix and
   custom environment given by _get_pkg_cmd_environ

Also in this commit:
 * A bug fix in existing Freebsd which was switching group name and
   and username parameters
 * I left a TODO message for your review as I don't think either BSD class
 handles a distro.package_command('upgrade') call and it might be needed
 in a followup PR

cloudinit/distros/bsd.py

@@ -1,15 +1,24 @@
 import platform
 
 from cloudinit import distros
+from cloudinit.distros import bsd_utils
 from cloudinit import helpers
+from cloudinit import log as logging
 from cloudinit import net
-from cloudinit.distros import bsd_utils
+from cloudinit import util
+
+LOG = logging.getLogger(__name__)
 
 
 class BSD(distros.Distro):
     hostname_conf_fn = '/etc/rc.conf'
     rc_conf_fn = "/etc/rc.conf"
 
+    # Set in BSD distro subclasses
+    group_add_cmd_prefix = []
+    pkg_cmd_install_prefix = []
+    pkg_cmd_remove_prefix = []
+
     def __init__(self, name, cfg, paths):
         super().__init__(name, cfg, paths)
         # This will be used to restrict certain
@@ -26,10 +35,36 @@ def _read_system_hostname(self):
     def _read_hostname(self, filename, default=None):
         return bsd_utils.get_rc_config_value('hostname')
 
+    def _get_add_member_to_group_cmd(self, member_name, group_name):
+        raise NotImplementedError('Return list cmd to add member to group')
+
     def _write_hostname(self, hostname, filename):
-        bsd_utils.set_rc_config_value(
-                'hostname', hostname,
-                fn='/etc/rc.conf')
+        bsd_utils.set_rc_config_value('hostname', hostname, fn='/etc/rc.conf')
+
+    def create_group(self, name, members=None):
+        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:
+                util.subp(group_add_cmd)
+                LOG.info("Created new group %s", name)
+            except Exception:
+                util.logexc(LOG, "Failed to create group %s", name)
+
+        if not members:
+            members = []
+        for member in members:
+            if not util.is_user(member):
+                LOG.warning("Unable to add group member '%s' to group '%s'"
+                            "; user does not exist.", member, name)
+                continue
+            try:
+                util.subp(self._get_add_member_to_group_cmd(member, name))
+                LOG.info("Added user '%s' to group '%s'", member, name)
+            except Exception:
+                util.logexc(LOG, "Failed to add user '%s' to group '%s'",
+                            member, name)
 
     def generate_fallback_config(self):
         nconf = {'config': [], 'version': 1}
@@ -39,6 +74,36 @@ def generate_fallback_config(self):
                  'mac_address': mac, 'subnets': [{'type': 'dhcp'}]})
         return nconf
 
+    def install_packages(self, pkglist):
+        self.update_package_sources()
+        self.package_command('install', pkgs=pkglist)
+
+    def _get_pkg_cmd_environ(self):
+        """Return environment vars used in *BSD package_command operations"""
+        raise NotImplementedError('BSD subclasses return a dict of env vars')
+
+    def package_command(self, command, args=None, pkgs=None):
+        if pkgs is None:
+            pkgs = []
+
+        # TODO neither freebsd nor netbsd handles a command 'upgrade'
+        # provided by cloudinit/config/cc_package_update_upgrade_install.py
+        if command == 'install':
+            cmd = self.pkg_cmd_install_prefix
+        elif command == 'remove':
+            cmd = self.pkg_cmd_remove_prefix
+
+        if args and isinstance(args, str):
+            cmd.append(args)
+        elif args and isinstance(args, list):
+            cmd.extend(args)
+
+        pkglist = util.expand_package_list('%s-%s', pkgs)
+        cmd.extend(pkglist)
+
+        # Allow the output of this to flow outwards (ie not be captured)
+        util.subp(cmd, env=self._get_pkg_cmd_environ(), capture=False)
+
     def _write_network_config(self, netconfig):
         return self._supported_write_network_config(netconfig)
 

cloudinit/distros/freebsd.py

@@ -21,38 +21,18 @@ class Distro(cloudinit.distros.bsd.BSD):
     login_conf_fn = '/etc/login.conf'
     login_conf_fn_bak = '/etc/login.conf.orig'
     ci_sudoers_fn = '/usr/local/etc/sudoers.d/90-cloud-init-users'
+    group_add_cmd_prefix = ['pw', 'group', 'add']
+    pkg_cmd_install_prefix = ["pkg", "install"]
+    pkg_cmd_remove_prefix = ["pkg", "remove"]
 
     def _select_hostname(self, hostname, fqdn):
         # Should be FQDN if available. See rc.conf(5) in FreeBSD
         if fqdn:
             return fqdn
         return hostname
 
-    def create_group(self, name, members=None):
-        group_add_cmd = ['pw', 'group', 'add', name]
-        if util.is_group(name):
-            LOG.warning("Skipping creation of existing group '%s'", name)
-        else:
-            try:
-                util.subp(group_add_cmd)
-                LOG.info("Created new group %s", name)
-            except Exception:
-                util.logexc(LOG, "Failed to create group %s", name)
-                raise
-        if not members:
-            members = []
-
-        for member in members:
-            if not util.is_user(member):
-                LOG.warning("Unable to add group member '%s' to group '%s'"
-                            "; user does not exist.", member, name)
-                continue
-            try:
-                util.subp(['pw', 'usermod', '-n', name, '-G', member])
-                LOG.info("Added user '%s' to group '%s'", member, name)
-            except Exception:
-                util.logexc(LOG, "Failed to add user '%s' to group '%s'",
-                            member, name)
+    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 util.is_user(name):
@@ -134,7 +114,7 @@ def lock_passwd(self, name):
             raise
 
     def apply_locale(self, locale, out_fn=None):
-        # Adjust the locals value to the new value
+        # Adjust the locales value to the new value
         newconf = StringIO()
         for line in util.load_file(self.login_conf_fn).splitlines():
             newconf.write(re.sub(r'^default:',
@@ -164,36 +144,17 @@ def apply_network_config_names(self, netconfig):
         # /etc/rc.conf a line with the following format:
         #    ifconfig_OLDNAME_name=NEWNAME
         # FreeBSD network script will rename the interface automatically.
-        return
-
-    def install_packages(self, pkglist):
-        self.update_package_sources()
-        self.package_command('install', pkgs=pkglist)
-
-    def package_command(self, command, args=None, pkgs=None):
-        if pkgs is None:
-            pkgs = []
+        pass
 
+    def _get_pkg_cmd_environ(self):
+        """Return environment vars used in *BSD package_command operations"""
         e = os.environ.copy()
         e['ASSUME_ALWAYS_YES'] = 'YES'
-
-        cmd = ['pkg']
-        if args and isinstance(args, str):
-            cmd.append(args)
-        elif args and isinstance(args, list):
-            cmd.extend(args)
-
-        if command:
-            cmd.append(command)
-
-        pkglist = util.expand_package_list('%s-%s', pkgs)
-        cmd.extend(pkglist)
-
-        # Allow the output of this to flow outwards (ie not be captured)
-        util.subp(cmd, env=e, capture=False)
+        return e
 
     def update_package_sources(self):
-        self._runner.run("update-sources", self.package_command,
-                         ["update"], freq=PER_INSTANCE)
+        self._runner.run(
+            "update-sources", self.package_command,
+            ["update"], freq=PER_INSTANCE)
 
 # vi: ts=4 expandtab

cloudinit/distros/netbsd.py

@@ -17,30 +17,12 @@
 class Distro(cloudinit.distros.bsd.BSD):
     ci_sudoers_fn = '/usr/pkg/etc/sudoers.d/90-cloud-init-users'
 
-    def create_group(self, name, members=None):
-        group_add_cmd = ['groupadd', name]
-        if util.is_group(name):
-            LOG.warning("Skipping creation of existing group '%s'", name)
-        else:
-            try:
-                util.subp(group_add_cmd)
-                LOG.info("Created new group %s", name)
-            except Exception:
-                util.logexc(LOG, "Failed to create group %s", name)
-
-        if not members:
-            members = []
-        for member in members:
-            if not util.is_user(member):
-                LOG.warning("Unable to add group member '%s' to group '%s'"
-                            "; user does not exist.", member, name)
-                continue
-            try:
-                util.subp(['usermod', '-G', name, member])
-                LOG.info("Added user '%s' to group '%s'", member, name)
-            except Exception:
-                util.logexc(LOG, "Failed to add user '%s' to group '%s'",
-                            member, name)
+    group_add_cmd_prefix = ["groupadd"]
+    pkg_cmd_install_prefix = ["pkg_add", "-U"]
+    pkg_cmd_remove_prefix = ['pkg_delete']
+
+    def _get_add_member_to_group_cmd(self, member_name, group_name):
+        return ['usermod', '-G', group_name, member_name]
 
     def add_user(self, name, **kwargs):
         if util.is_user(name):
@@ -92,9 +74,9 @@ def add_user(self, name, **kwargs):
         if passwd_val is not None:
             self.set_passwd(name, passwd_val, hashed=True)
 
-    def set_passwd(self, user, password, hashed=False):
+    def set_passwd(self, user, passwd, hashed=False):
         if hashed:
-            hashed_pw = password
+            hashed_pw = passwd
         elif not hasattr(crypt, 'METHOD_BLOWFISH'):
             # crypt.METHOD_BLOWFISH comes with Python 3.7 which is available
             # on NetBSD 7 and 8.
@@ -105,7 +87,7 @@ def set_passwd(self, user, password, hashed=False):
         else:
             method = crypt.METHOD_BLOWFISH  # pylint: disable=E1101
             hashed_pw = crypt.crypt(
-                    password,
+                    passwd,
                     crypt.mksalt(method))
 
         try:
@@ -133,36 +115,16 @@ def apply_locale(self, locale, out_fn=None):
 
     def apply_network_config_names(self, netconfig):
         LOG.debug('NetBSD cannot rename network interface.')
-        return
-
-    def install_packages(self, pkglist):
-        self.package_command('install', pkgs=pkglist)
-
-    def package_command(self, command, args=None, pkgs=None):
-        if pkgs is None:
-            pkgs = []
 
+    def _get_pkg_cmd_environ(self):
+        """Return environment vars used in *BSD package_command operations"""
         os_release = platform.release()
         os_arch = platform.machine()
         e = os.environ.copy()
         e['PKG_PATH'] = (
                 'http://cdn.netbsd.org/pub/pkgsrc/'
                 'packages/NetBSD/%s/%s/All') % (os_arch, os_release)
-
-        if command == 'install':
-            cmd = ['pkg_add', '-U']
-        elif command == 'remove':
-            cmd = ['pkg_delete']
-        if args and isinstance(args, str):
-            cmd.append(args)
-        elif args and isinstance(args, list):
-            cmd.extend(args)
-
-        pkglist = util.expand_package_list('%s-%s', pkgs)
-        cmd.extend(pkglist)
-
-        # Allow the output of this to flow outwards (ie not be captured)
-        util.subp(cmd, env=e, capture=False)
+        return e
 
     def update_package_sources(self):
         pass