[PR #10417/44ca3661 backport][stable-11] sysrc: refactor (#10504)

sysrc: refactor (#10417)

* sysrc: refactor

* sysrc: refactor changelog fragment

* sysrc: forgot the os import

* sysrc: update test to edit the correct file

* sysrc: Added copyright info to the test conf file

* sysrc: Added full copyright info to the test conf file

* sysrc: Detect permission denied when using sysrc

* sysrc: Fixed the permission check and 2.7 compatibility

* sysrc: Fix typo of import

* sysrc: Fix err.find check

* sysrc: Add bugfixes changelog fragment

* sysrc: Use `StateModuleHelper`

* sysrc: updated imports

* sysrc: remove re import and set errno.EACCES on the OSError

* sysrc: format code properly

* sysrc: fix Python 2.7 compatibility and set changed manually

* sysrc: add missing name format check

Also use `self.module.fail_json` through out

* sysrc: Removed os import by accident

* sysrc: updated per review, and the way the existing value is retrieved

(cherry picked from commit 44ca366173)

Co-authored-by: David Lundgren <dlundgren@syberisle.net>

changelogs/fragments/10417-sysrc-refactor.yml

@@ -0,0 +1,4 @@
+minor_changes:
+  - sysrc - adjustments to the code (https://github.com/ansible-collections/community.general/pull/10417).
+bugfixes:
+  - sysrc - fixes parsing with multi-line variables (https://github.com/ansible-collections/community.general/issues/10394, https://github.com/ansible-collections/community.general/pull/10417).

plugins/modules/sysrc.py

@@ -7,6 +7,7 @@
 # SPDX-License-Identifier: GPL-3.0-or-later
 
 from __future__ import absolute_import, division, print_function
+
 __metaclass__ = type
 
 DOCUMENTATION = r"""
@@ -102,157 +103,121 @@ changed:
   sample: true
 """
 
-from ansible.module_utils.basic import AnsibleModule
+from ansible_collections.community.general.plugins.module_utils.module_helper import StateModuleHelper
+
+import os
 import re
 
 
-class Sysrc(object):
+class Sysrc(StateModuleHelper):
-    def __init__(self, module, name, value, path, delim, jail):
+    module = dict(
-        self.module = module
-        self.name = name
-        self.changed = False
-        self.value = value
-        self.path = path
-        self.delim = delim
-        self.jail = jail
-        self.sysrc = module.get_bin_path('sysrc', True)
-
-    def has_unknown_variable(self, out, err):
-        # newer versions of sysrc use stderr instead of stdout
-        return err.find("unknown variable") > 0 or out.find("unknown variable") > 0
-
-    def exists(self):
-        """
-        Tests whether the name is in the file.  If parameter value is defined,
-        then tests whether name=value is in the file.  These tests are necessary
-        because sysrc doesn't use exit codes.  Instead, let sysrc read the
-        file's content and create a dictionary comprising the configuration.
-        Use this dictionary to preform the tests.
-        """
-        (rc, out, err) = self.run_sysrc('-e', '-a')
-        conf = dict([i.split('=', 1) for i in out.splitlines()])
-        if self.value is None:
-            return self.name in conf
-        else:
-            return self.name in conf and conf[self.name] == '"%s"' % self.value
-
-    def contains(self):
-        (rc, out, err) = self.run_sysrc('-n', self.name)
-        if self.has_unknown_variable(out, err):
-            return False
-
-        return self.value in out.strip().split(self.delim)
-
-    def present(self):
-        if self.exists():
-            return
-
-        if not self.module.check_mode:
-            (rc, out, err) = self.run_sysrc("%s=%s" % (self.name, self.value))
-
-        self.changed = True
-
-    def absent(self):
-        if not self.exists():
-            return
-
-        # inversed since we still need to mark as changed
-        if not self.module.check_mode:
-            (rc, out, err) = self.run_sysrc('-x', self.name)
-            if self.has_unknown_variable(out, err):
-                return
-
-        self.changed = True
-
-    def value_present(self):
-        if self.contains():
-            return
-
-        if self.module.check_mode:
-            self.changed = True
-            return
-
-        setstring = '%s+=%s%s' % (self.name, self.delim, self.value)
-        (rc, out, err) = self.run_sysrc(setstring)
-        if out.find("%s:" % self.name) == 0:
-            values = out.split(' -> ')[1].strip().split(self.delim)
-            if self.value in values:
-                self.changed = True
-
-    def value_absent(self):
-        if not self.contains():
-            return
-
-        if self.module.check_mode:
-            self.changed = True
-            return
-
-        setstring = '%s-=%s%s' % (self.name, self.delim, self.value)
-        (rc, out, err) = self.run_sysrc(setstring)
-        if out.find("%s:" % self.name) == 0:
-            values = out.split(' -> ')[1].strip().split(self.delim)
-            if self.value not in values:
-                self.changed = True
-
-    def run_sysrc(self, *args):
-        cmd = [self.sysrc, '-f', self.path]
-        if self.jail:
-            cmd += ['-j', self.jail]
-        cmd.extend(args)
-
-        (rc, out, err) = self.module.run_command(cmd)
-
-        return (rc, out, err)
-
-
-def main():
-    module = AnsibleModule(
         argument_spec=dict(
             name=dict(type='str', required=True),
             value=dict(type='str', default=None),
             state=dict(type='str', default='present', choices=['absent', 'present', 'value_present', 'value_absent']),
             path=dict(type='str', default='/etc/rc.conf'),
             delim=dict(type='str', default=' '),
-            jail=dict(type='str', default=None),
+            jail=dict(type='str', default=None)
         ),
-        supports_check_mode=True,
+        supports_check_mode=True
     )
+    output_params = ('value',)
+    use_old_vardict = False
 
-    name = module.params.pop('name')
+    def __init_module__(self):
-    # OID style names are not supported
+        # OID style names are not supported
-    if not re.match('^[a-zA-Z0-9_]+$', name):
+        if not re.match(r'^\w+$', self.vars.name, re.ASCII):
-        module.fail_json(
+            self.module.fail_json(msg="Name may only contain alpha-numeric and underscore characters")
-            msg="Name may only contain alphanumeric and underscore characters"
-        )
 
-    value = module.params.pop('value')
+        self.sysrc = self.module.get_bin_path('sysrc', True)
-    state = module.params.pop('state')
-    path = module.params.pop('path')
-    delim = module.params.pop('delim')
-    jail = module.params.pop('jail')
-    result = dict(
-        name=name,
-        state=state,
-        value=value,
-        path=path,
-        delim=delim,
-        jail=jail
-    )
 
-    rc_value = Sysrc(module, name, value, path, delim, jail)
+    def _contains(self):
+        value = self._get()
+        if value is None:
+            return False, None
 
-    if state == 'present':
+        value = value.split(self.vars.delim)
-        rc_value.present()
-    elif state == 'absent':
-        rc_value.absent()
-    elif state == 'value_present':
-        rc_value.value_present()
-    elif state == 'value_absent':
-        rc_value.value_absent()
 
-    result['changed'] = rc_value.changed
+        return self.vars.value in value, value
 
-    module.exit_json(**result)
+    def _get(self):
+        if not os.path.exists(self.vars.path):
+            return None
+
+        (rc, out, err) = self._sysrc('-v', '-n', self.vars.name)
+        if "unknown variable" in err or "unknown variable" in out:
+            # Prior to FreeBSD 11.1 sysrc would write "unknown variable" to stdout and not stderr
+            # https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=229806
+            return None
+
+        if out.startswith(self.vars.path):
+            return out.split(':', 1)[1].strip()
+
+        return None
+
+    def _modify(self, op, changed):
+        (rc, out, err) = self._sysrc("%s%s=%s%s" % (self.vars.name, op, self.vars.delim, self.vars.value))
+        if out.startswith("%s:" % self.vars.name):
+            return changed(out.split(' -> ')[1].strip().split(self.vars.delim))
+
+        return False
+
+    def _sysrc(self, *args):
+        cmd = [self.sysrc, '-f', self.vars.path]
+        if self.vars.jail:
+            cmd += ['-j', self.vars.jail]
+        cmd.extend(args)
+
+        (rc, out, err) = self.module.run_command(cmd)
+        if "Permission denied" in err:
+            self.module.fail_json(msg="Permission denied for %s" % self.vars.path)
+
+        return rc, out, err
+
+    def state_absent(self):
+        if self._get() is None:
+            return
+
+        if not self.check_mode:
+            self._sysrc('-x', self.vars.name)
+
+        self.changed = True
+
+    def state_present(self):
+        value = self._get()
+        if value == self.vars.value:
+            return
+
+        if self.vars.value is None:
+            self.vars.set('value', value)
+            return
+
+        if not self.check_mode:
+            self._sysrc("%s=%s" % (self.vars.name, self.vars.value))
+
+        self.changed = True
+
+    def state_value_absent(self):
+        (contains, _unused) = self._contains()
+        if not contains:
+            return
+
+        self.changed = self.check_mode or self._modify('-', lambda values: self.vars.value not in values)
+
+    def state_value_present(self):
+        (contains, value) = self._contains()
+        if contains:
+            return
+
+        if self.vars.value is None:
+            self.vars.set('value', value)
+            return
+
+        self.changed = self.check_mode or self._modify('+', lambda values: self.vars.value in values)
+
+
+def main():
+    Sysrc.execute()
 
 
 if __name__ == '__main__':

tests/integration/targets/sysrc/files/10394.conf

@@ -0,0 +1,7 @@
+# Copyright (c) Ansible Project
+# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt)
+# SPDX-License-Identifier: GPL-3.0-or-later
+k1="v1"
+jail_list="
+  foo
+  bar"

tests/integration/targets/sysrc/tasks/main.yml

@@ -369,14 +369,79 @@
               - "value_2 == sysrc_equals_sign_2.value"
               - "value_2 == conf.spamd_flags"
 
+    ##
+    ## sysrc - #10004 state=absent when using default settings will report `changed=true`
+    ##
+    - name: Test that a key from /etc/defaults/rc.conf is not used to mark changed
+      sysrc:
+        name: dumpdev
+        state: absent
+        path: /tmp/10004.conf
+      register: sysrc_10004_absent
+      failed_when: sysrc_10004_absent.changed
+
+    - name: Test that a delimited key from /etc/defaults/rc.conf is not used to mark changed
+      sysrc:
+        name: rc_conf_files
+        state: value_absent
+        path: /tmp/10004.conf
+      register: sysrc_10004_value_absent
+      failed_when: sysrc_10004_value_absent.changed
+
+    - name: Test that a key from /etc/defaults/rc.conf is not used to mark changed without a path
+      sysrc:
+        name: static_routes
+        state: absent
+      register: sysrc_absent_default
+      failed_when: sysrc_absent_default.changed
+
+    ##
+    ## sysrc - #10394 Ensure that files with multi-line values work
+    ##
+    - name: Copy 10394.conf
+      copy:
+        src: 10394.conf
+        dest: /tmp/10394.conf
+
+    - name: Change value for k1
+      sysrc:
+        name: k1
+        value: v2
+        path: /tmp/10394.conf
+      register: sysrc_10394_changed
+
+    - name: Get file content
+      shell: "cat /tmp/10394.conf"
+      register: sysrc_10394_content
+
+    - name: Ensure sysrc changed k1 from v1 to v2
+      assert:
+        that:
+          - sysrc_10394_changed.changed
+          - >
+            'k1="v2"' in sysrc_10394_content.stdout_lines
+
+    ##
+    ## sysrc - additional tests
+    ##
+    - name: Ensure failure on OID style name since sysrc does not support them
+      sysrc:
+        name: not.valid.var
+        value: test
+      register: sysrc_name_check
+      failed_when:
+        - sysrc_name_check is not failed
+        - >
+          'Name may only contain alpha-numeric and underscore characters' != sysrc_name_check.msg
+
   always:
 
     - name: Restore /etc/rc.conf
       copy:
-        content: "{{ cached_etc_rcconf_content }}"
+        content: "{{ cached_etc_rcconf_content.stdout }}"
         dest: /etc/rc.conf
 
     - name: Restore /boot/loader.conf
       copy:
-        content: "{{ cached_boot_loaderconf_content }}"
+        content: "{{ cached_boot_loaderconf_content.stdout }}"
         dest: /boot/loader.conf