diff --git a/changelogs/fragments/sysctl-invalid-value.yml b/changelogs/fragments/sysctl-invalid-value.yml new file mode 100644 index 00000000000000..75ae5e790d84ca --- /dev/null +++ b/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)" diff --git a/lib/ansible/modules/system/sysctl.py b/lib/ansible/modules/system/sysctl.py index 8664dda1999ea2..143c7ce27d2c3d 100644 --- a/lib/ansible/modules/system/sysctl.py +++ b/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)) # ============================================================== diff --git a/test/integration/targets/sysctl/tasks/main.yml b/test/integration/targets/sysctl/tasks/main.yml index cdea4aa0389770..e23b2ccee665ad 100644 --- a/test/integration/targets/sysctl/tasks/main.yml +++ b/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 . +# 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