From 7da1f3ffeaa7d47c7fbdac7b4bda82d3ac13c4b5 Mon Sep 17 00:00:00 2001
From: "patchback[bot]" <45432694+patchback[bot]@users.noreply.github.com>
Date: Tue, 20 Oct 2020 22:46:12 +0200
Subject: [PATCH] iptables_state: fix race condition between module and its
 action plugin (#1140) (#1143)

* fix race condition between module and its action plugin

See https://github.com/ansible-collections/community.general/issues/1136.
Also remove irrelevant/unneeded display.v() and display.warning() around
connection reset.

* do not check for cookie if not in async mode

* add changelog fragment

(cherry picked from commit 3bc31f286e95a39a64a7a914588eee97807714d1)

Co-authored-by: quidame <quidame@poivron.org>
---
 ...1140-iptables_state-fix-race-condition.yml |  4 ++++
 plugins/action/system/iptables_state.py       | 19 ++++++++++++++-----
 plugins/modules/system/iptables_state.py      | 12 ++++++++++++
 3 files changed, 30 insertions(+), 5 deletions(-)
 create mode 100644 changelogs/fragments/1140-iptables_state-fix-race-condition.yml

diff --git a/changelogs/fragments/1140-iptables_state-fix-race-condition.yml b/changelogs/fragments/1140-iptables_state-fix-race-condition.yml
new file mode 100644
index 0000000000..00cd6d2d1b
--- /dev/null
+++ b/changelogs/fragments/1140-iptables_state-fix-race-condition.yml
@@ -0,0 +1,4 @@
+---
+bugfixes:
+  - iptables_state - fix race condition between module and its action plugin
+    (https://github.com/ansible-collections/community.general/issues/1136).
diff --git a/plugins/action/system/iptables_state.py b/plugins/action/system/iptables_state.py
index 1b689dfb0c..92fb079ae0 100644
--- a/plugins/action/system/iptables_state.py
+++ b/plugins/action/system/iptables_state.py
@@ -125,15 +125,18 @@ class ActionModule(ActionBase):
                     module_args['_back'] = '%s/iptables.state' % async_dir
                     async_status_args = dict(_async_dir=async_dir)
                     confirm_cmd = 'rm -f %s' % module_args['_back']
+                    starter_cmd = 'touch %s.starter' % module_args['_back']
                     remaining_time = max(task_async, max_timeout)
 
             # do work!
             result = merge_hash(result, self._execute_module(module_args=module_args, task_vars=task_vars, wrap_async=wrap_async))
 
             # Then the 3-steps "go ahead or rollback":
-            # - reset connection to ensure a persistent one will not be reused
-            # - confirm the restored state by removing the backup on the remote
-            # - retrieve the results of the asynchronous task to return them
+            # 1. Catch early errors of the module (in asynchronous task) if any.
+            #    Touch a file on the target to signal the module to process now.
+            # 2. Reset connection to ensure a persistent one will not be reused.
+            # 3. Confirm the restored state by removing the backup on the remote.
+            #    Retrieve the results of the asynchronous task to return them.
             if '_back' in module_args:
                 async_status_args['jid'] = result.get('ansible_job_id', None)
                 if async_status_args['jid'] is None:
@@ -143,12 +146,18 @@ class ActionModule(ActionBase):
                 # option type/value, missing required system command, etc.
                 result = merge_hash(result, self._async_result(async_status_args, task_vars, 0))
 
+                # 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)
+
+                # As the main command is not yet executed on the target, here
+                # 'finished' means 'failed before main command be executed'.
                 if not result['finished']:
                     try:
                         self._connection.reset()
-                        display.v("%s: reset connection" % (module_name))
                     except AttributeError:
-                        display.warning("Connection plugin does not allow to reset the connection.")
+                        pass
 
                     for x in range(max_timeout):
                         time.sleep(1)
diff --git a/plugins/modules/system/iptables_state.py b/plugins/modules/system/iptables_state.py
index a7dd073c8c..5647526819 100644
--- a/plugins/modules/system/iptables_state.py
+++ b/plugins/modules/system/iptables_state.py
@@ -551,6 +551,18 @@ def main():
             restored_state = state_to_restore
 
     else:
+        # Let time enough to the plugin to retrieve async status of the module
+        # in case of bad option type/value and the like.
+        if _back is not None:
+            b_starter = to_bytes('%s.starter' % _back, errors='surrogate_or_strict')
+            while True:
+                if os.path.exists(b_starter):
+                    os.remove(b_starter)
+                    break
+                else:
+                    time.sleep(0.01)
+                    continue
+
         (rc, stdout, stderr) = module.run_command(MAINCOMMAND)
         if 'Another app is currently holding the xtables lock' in stderr:
             module.fail_json(