sysctl will now return an error if the value is invalid (ansible#55695)

* sysctl will now return an error if the value is invalid

sysctl can fail to set a value even if it returns an exit status 0. More
details: https://bugzilla.redhat.com/show_bug.cgi?id=1264080. Because of
this in case of an invalid value or a read-only file system, sysctl
module would return OK, even though it didn't set anything. To be sure
that sysctl correctly applied the changes we also need to check the
output of stderr.

* Run sysctl with LANG=C

Because we are parsing sysctl stderr we need to make sure that errors
are persistent across different system language settings.

* Add changelog fragment for sysctl

changelogs/fragments/sysctl-invalid-value.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - "sysctl: the module now also checks the output of STDERR to report if values are correctly set (https://github.com/ansible/ansible/pull/55695)"

lib/ansible/modules/system/sysctl.py

@@ -99,6 +99,7 @@
 # ==============================================================
 
 import os
+import re
 import tempfile
 
 from ansible.module_utils.basic import get_platform, AnsibleModule
@@ -109,6 +110,10 @@
 
 class SysctlModule(object):
 
+    # We have to use LANG=C because we are capturing STDERR of sysctl to detect
+    # success or failure.
+    LANG_ENV = {'LANG': 'C', 'LC_ALL': 'C', 'LC_MESSAGES': 'C'}
+
     def __init__(self, module):
         self.module = module
         self.args = self.module.params
@@ -215,6 +220,14 @@ def _parse_value(self, value):
         else:
             return value
 
+    def _stderr_failed(self, err):
+        # sysctl can fail to set a value even if it returns an exit status 0
+        # (https://bugzilla.redhat.com/show_bug.cgi?id=1264080). That's why we
+        # also have to check stderr for errors. For now we will only fail on
+        # specific errors defined by the regex below.
+        errors_regex = r'^sysctl: setting key "[^"]+": (Invalid argument|Read-only file system)$'
+        return re.search(errors_regex, err, re.MULTILINE) is not None
+
     # ==============================================================
     #   SYSCTL COMMAND MANAGEMENT
     # ==============================================================
@@ -226,7 +239,7 @@ def get_token_curr_value(self, token):
             thiscmd = "%s -n %s" % (self.sysctl_cmd, token)
         else:
             thiscmd = "%s -e -n %s" % (self.sysctl_cmd, token)
-        rc, out, err = self.module.run_command(thiscmd)
+        rc, out, err = self.module.run_command(thiscmd, environ_update=self.LANG_ENV)
         if rc != 0:
             return None
         else:
@@ -250,8 +263,8 @@ def set_token_value(self, token, value):
             if self.args['ignoreerrors']:
                 ignore_missing = '-e'
             thiscmd = "%s %s -w %s=%s" % (self.sysctl_cmd, ignore_missing, token, value)
-        rc, out, err = self.module.run_command(thiscmd)
-        if rc != 0:
+        rc, out, err = self.module.run_command(thiscmd, environ_update=self.LANG_ENV)
+        if rc != 0 or self._stderr_failed(err):
             self.module.fail_json(msg='setting %s failed: %s' % (token, out + err))
         else:
             return rc
@@ -261,7 +274,7 @@ def reload_sysctl(self):
         # do it
         if self.platform == 'freebsd':
             # freebsd doesn't support -p, so reload the sysctl service
-            rc, out, err = self.module.run_command('/etc/rc.d/sysctl reload')
+            rc, out, err = self.module.run_command('/etc/rc.d/sysctl reload', environ_update=self.LANG_ENV)
         elif self.platform == 'openbsd':
             # openbsd doesn't support -p and doesn't have a sysctl service,
             # so we have to set every value with its own sysctl call
@@ -279,9 +292,9 @@ def reload_sysctl(self):
             if self.args['ignoreerrors']:
                 sysctl_args.insert(1, '-e')
 
-            rc, out, err = self.module.run_command(sysctl_args)
+            rc, out, err = self.module.run_command(sysctl_args, environ_update=self.LANG_ENV)
 
-        if rc != 0:
+        if rc != 0 or self._stderr_failed(err):
             self.module.fail_json(msg="Failed to reload sysctl: %s" % to_native(out) + to_native(err))
 
     # ==============================================================

test/integration/targets/sysctl/tasks/main.yml

@@ -16,6 +16,10 @@
 # You should have received a copy of the GNU General Public License
 # along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
 
+# NOTE: Testing sysctl inside an unprivileged container means that we cannot
+# apply sysctl, or it will always fail, because of that in most cases (except
+# those when it should fail) we have to use `reload=no`.
+
 - set_fact:
     output_dir_test: "{{ output_dir }}/test_sysctl"
 
@@ -115,6 +119,22 @@
     that:
       - sysctl_test2_change_test is not changed
 
+- name: Try sysctl with an invalid value
+  sysctl:
+    name: net.ipv4.ip_forward
+    value: foo
+  register: sysctl_test3
+  ignore_errors: yes
+
+- debug:
+    var: sysctl_test3
+    verbosity: 1
+
+- name: validate results for test 3
+  assert:
+    that:
+      - sysctl_test3 is failed
+
 ##
 ## sysctl - sysctl_set
 ##
@@ -171,3 +191,20 @@
     that:
       - sysctl_no_value is failed
       - "sysctl_no_value.msg == 'value cannot be None'"
+
+- name: Try sysctl with an invalid value
+  sysctl:
+    name: net.ipv4.ip_forward
+    value: foo
+    sysctl_set: yes
+  register: sysctl_test4
+  ignore_errors: yes
+
+- debug:
+    var: sysctl_test4
+    verbosity: 1
+
+- name: validate results for test 4
+  assert:
+    that:
+      - sysctl_test4 is failed