From 2c1ab2d384cc44136e76a9177a7e87e4c7d1f96a Mon Sep 17 00:00:00 2001 From: quidame Date: Tue, 18 May 2021 11:51:37 +0200 Subject: [PATCH] iptables_state: fix per-table initialization command (#2525) * refactor initialize_from_null_state() * Use a more neutral command (iptables -L) to load per-table needed modules. * fix 'FutureWarning: Possible nested set at position ...' (re.sub) * fix pylints (module + action plugin) * unsubscriptable-object * superfluous-parens * consider-using-in * unused-variable * unused-import * no-else-break * cleanup other internal module_args if they exist * add changelog fragment * Apply suggestions from code review (changelog fragment) Co-authored-by: Felix Fontein * Remove useless plugin type in changelog fragment Co-authored-by: Amin Vakil Co-authored-by: Felix Fontein Co-authored-by: Amin Vakil --- ...ables_state-fix-initialization-command.yml | 6 +++ plugins/action/system/iptables_state.py | 19 ++++--- plugins/modules/system/iptables_state.py | 49 +++++++++---------- 3 files changed, 37 insertions(+), 37 deletions(-) create mode 100644 changelogs/fragments/2525-iptables_state-fix-initialization-command.yml diff --git a/changelogs/fragments/2525-iptables_state-fix-initialization-command.yml b/changelogs/fragments/2525-iptables_state-fix-initialization-command.yml new file mode 100644 index 0000000000..552c0b26ab --- /dev/null +++ b/changelogs/fragments/2525-iptables_state-fix-initialization-command.yml @@ -0,0 +1,6 @@ +--- +bugfixes: + - "iptables_state - fix initialization of iptables from null state when adressing + more than one table (https://github.com/ansible-collections/community.general/issues/2523)." + - "iptables_state - fix a 'FutureWarning' in a regex and do some basic code clean up + (https://github.com/ansible-collections/community.general/pull/2525)." diff --git a/plugins/action/system/iptables_state.py b/plugins/action/system/iptables_state.py index cc174b3bd7..96b6dc689c 100644 --- a/plugins/action/system/iptables_state.py +++ b/plugins/action/system/iptables_state.py @@ -7,7 +7,7 @@ __metaclass__ = type import time from ansible.plugins.action import ActionBase -from ansible.errors import AnsibleError, AnsibleActionFail, AnsibleConnectionFailure +from ansible.errors import AnsibleActionFail, AnsibleConnectionFailure from ansible.utils.vars import merge_hash from ansible.utils.display import Display @@ -46,7 +46,7 @@ class ActionModule(ActionBase): the async wrapper results (those with the ansible_job_id key). ''' # At least one iteration is required, even if timeout is 0. - for i in range(max(1, timeout)): + for dummy in range(max(1, timeout)): async_result = self._execute_module( module_name='ansible.builtin.async_status', module_args=module_args, @@ -76,7 +76,6 @@ class ActionModule(ActionBase): task_async = self._task.async_val check_mode = self._play_context.check_mode max_timeout = self._connection._play_context.timeout - module_name = self._task.action module_args = self._task.args if module_args.get('state', None) == 'restored': @@ -133,7 +132,7 @@ class ActionModule(ActionBase): # The module is aware to not process the main iptables-restore # command before finding (and deleting) the 'starter' cookie on # the host, so the previous query will not reach ssh timeout. - garbage = self._low_level_execute_command(starter_cmd, sudoable=self.DEFAULT_SUDOABLE) + dummy = self._low_level_execute_command(starter_cmd, sudoable=self.DEFAULT_SUDOABLE) # As the main command is not yet executed on the target, here # 'finished' means 'failed before main command be executed'. @@ -143,7 +142,7 @@ class ActionModule(ActionBase): except AttributeError: pass - for x in range(max_timeout): + for dummy in range(max_timeout): time.sleep(1) remaining_time -= 1 # - AnsibleConnectionFailure covers rejected requests (i.e. @@ -151,7 +150,7 @@ class ActionModule(ActionBase): # - ansible_timeout is able to cover dropped requests (due # to a rule or policy DROP) if not lower than async_val. try: - garbage = self._low_level_execute_command(confirm_cmd, sudoable=self.DEFAULT_SUDOABLE) + dummy = self._low_level_execute_command(confirm_cmd, sudoable=self.DEFAULT_SUDOABLE) break except AnsibleConnectionFailure: continue @@ -164,12 +163,12 @@ class ActionModule(ActionBase): del result[key] if result.get('invocation', {}).get('module_args'): - if '_timeout' in result['invocation']['module_args']: - del result['invocation']['module_args']['_back'] - del result['invocation']['module_args']['_timeout'] + for key in ('_back', '_timeout', '_async_dir', 'jid'): + if result['invocation']['module_args'].get(key): + del result['invocation']['module_args'][key] async_status_args['mode'] = 'cleanup' - garbage = self._execute_module( + dummy = self._execute_module( module_name='ansible.builtin.async_status', module_args=async_status_args, task_vars=task_vars, diff --git a/plugins/modules/system/iptables_state.py b/plugins/modules/system/iptables_state.py index 5647526819..326db862bc 100644 --- a/plugins/modules/system/iptables_state.py +++ b/plugins/modules/system/iptables_state.py @@ -232,7 +232,7 @@ import filecmp import shutil from ansible.module_utils.basic import AnsibleModule -from ansible.module_utils._text import to_bytes, to_native, to_text +from ansible.module_utils._text import to_bytes, to_native IPTABLES = dict( @@ -262,7 +262,7 @@ def read_state(b_path): lines = text.splitlines() while '' in lines: lines.remove('') - return (lines) + return lines def write_state(b_path, lines, changed): @@ -282,9 +282,9 @@ def write_state(b_path, lines, changed): if b_destdir and not os.path.exists(b_destdir) and not module.check_mode: try: os.makedirs(b_destdir) - except Exception as e: + except Exception as err: module.fail_json( - msg='Error creating %s. Error code: %s. Error description: %s' % (destdir, e[0], e[1]), + msg='Error creating %s: %s' % (destdir, to_native(err)), initial_state=lines) changed = True @@ -295,10 +295,10 @@ def write_state(b_path, lines, changed): if changed and not module.check_mode: try: shutil.copyfile(tmpfile, b_path) - except Exception as e: + except Exception as err: path = to_native(b_path, errors='surrogate_or_strict') module.fail_json( - msg='Error saving state into %s. Error code: %s. Error description: %s' % (path, e[0], e[1]), + msg='Error saving state into %s: %s' % (path, to_native(err)), initial_state=lines) return changed @@ -313,14 +313,11 @@ def initialize_from_null_state(initializer, initcommand, table): if table is None: table = 'filter' - tmpfd, tmpfile = tempfile.mkstemp() - with os.fdopen(tmpfd, 'w') as f: - f.write('*%s\nCOMMIT\n' % table) - - initializer.append(tmpfile) - (rc, out, err) = module.run_command(initializer, check_rc=True) + commandline = list(initializer) + commandline += ['-t', table] + (rc, out, err) = module.run_command(commandline, check_rc=True) (rc, out, err) = module.run_command(initcommand, check_rc=True) - return (rc, out, err) + return rc, out, err def filter_and_format_state(string): @@ -328,13 +325,13 @@ def filter_and_format_state(string): Remove timestamps to ensure idempotence between runs. Also remove counters by default. And return the result as a list. ''' - string = re.sub('((^|\n)# (Generated|Completed)[^\n]*) on [^\n]*', '\\1', string) + string = re.sub(r'((^|\n)# (Generated|Completed)[^\n]*) on [^\n]*', r'\1', string) if not module.params['counters']: - string = re.sub('[[][0-9]+:[0-9]+[]]', '[0:0]', string) + string = re.sub(r'\[[0-9]+:[0-9]+\]', r'[0:0]', string) lines = string.splitlines() while '' in lines: lines.remove('') - return (lines) + return lines def per_table_state(command, state): @@ -347,14 +344,14 @@ def per_table_state(command, state): COMMAND = list(command) if '*%s' % t in state.splitlines(): COMMAND.extend(['--table', t]) - (rc, out, err) = module.run_command(COMMAND, check_rc=True) - out = re.sub('(^|\n)(# Generated|# Completed|[*]%s|COMMIT)[^\n]*' % t, '', out) - out = re.sub(' *[[][0-9]+:[0-9]+[]] *', '', out) + dummy, out, dummy = module.run_command(COMMAND, check_rc=True) + out = re.sub(r'(^|\n)(# Generated|# Completed|[*]%s|COMMIT)[^\n]*' % t, r'', out) + out = re.sub(r' *\[[0-9]+:[0-9]+\] *', r'', out) table = out.splitlines() while '' in table: table.remove('') tables[t] = table - return (tables) + return tables def main(): @@ -402,7 +399,7 @@ def main(): changed = False COMMANDARGS = [] INITCOMMAND = [bin_iptables_save] - INITIALIZER = [bin_iptables_restore] + INITIALIZER = [bin_iptables, '-L', '-n'] TESTCOMMAND = [bin_iptables_restore, '--test'] if counters: @@ -502,7 +499,7 @@ def main(): if _back is not None: b_back = to_bytes(_back, errors='surrogate_or_strict') - garbage = write_state(b_back, initref_state, changed) + dummy = write_state(b_back, initref_state, changed) BACKCOMMAND = list(MAINCOMMAND) BACKCOMMAND.append(_back) @@ -559,9 +556,7 @@ def main(): if os.path.exists(b_starter): os.remove(b_starter) break - else: - time.sleep(0.01) - continue + time.sleep(0.01) (rc, stdout, stderr) = module.run_command(MAINCOMMAND) if 'Another app is currently holding the xtables lock' in stderr: @@ -579,7 +574,7 @@ def main(): (rc, stdout, stderr) = module.run_command(SAVECOMMAND, check_rc=True) restored_state = filter_and_format_state(stdout) - if restored_state != initref_state and restored_state != initial_state: + if restored_state not in (initref_state, initial_state): if module.check_mode: changed = True else: @@ -609,7 +604,7 @@ def main(): # timeout # * task attribute 'poll' equals 0 # - for x in range(_timeout): + for dummy in range(_timeout): if os.path.exists(b_back): time.sleep(1) continue