Backport/2.7/55695

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

@@ -81,7 +81,7 @@
     sysctl_file: /tmp/test_sysctl.conf
     reload: no
 
-# Set ip forwarding on in /proc and do not reload the sysctl file
+# Set ip forwarding on in /proc and verify token value with the sysctl command
 - sysctl:
     name: net.ipv4.ip_forward
     value: 1
@@ -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" % str(out) + str(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