From 6bc671a46a35f611b29b68c034014fc7d9f8bed5 Mon Sep 17 00:00:00 2001 From: Strahinja Kustudic Date: Tue, 21 May 2019 03:10:31 +0200 Subject: [PATCH] Backport/2.7/55695 (#56253) * 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. (cherry picked from commit 0432b7f2522dbf82c4fabdb3fd17f7ac83f34e62) * 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. (cherry picked from commit a16128f778b1e7574c5986aed26e146ac0561533) * Add changelog fragment for sysctl (cherry picked from commit 3ad9d4d83c1d2bbfccefb8388904c596d98f8731) --- changelogs/fragments/sysctl-invalid-value.yml | 2 + lib/ansible/modules/system/sysctl.py | 27 ++++++++++---- .../integration/targets/sysctl/tasks/main.yml | 37 +++++++++++++++++++ 3 files changed, 59 insertions(+), 7 deletions(-) create mode 100644 changelogs/fragments/sysctl-invalid-value.yml diff --git a/changelogs/fragments/sysctl-invalid-value.yml b/changelogs/fragments/sysctl-invalid-value.yml new file mode 100644 index 00000000000..75ae5e790d8 --- /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 8664dda1999..143c7ce27d2 100644 --- a/lib/ansible/modules/system/sysctl.py +++ b/lib/ansible/modules/system/sysctl.py @@ -81,7 +81,7 @@ EXAMPLES = ''' 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 @@ EXAMPLES = ''' # ============================================================== import os +import re import tempfile from ansible.module_utils.basic import get_platform, AnsibleModule @@ -109,6 +110,10 @@ from ansible.module_utils._text import to_native 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 @@ class SysctlModule(object): 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 @@ class SysctlModule(object): 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 @@ class SysctlModule(object): 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 @@ class SysctlModule(object): # 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 @@ class SysctlModule(object): 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 cdea4aa0389..e23b2ccee66 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