Fix check mode in iptables_state for incomplete iptables-save files along with integration tests (#8029)

* Implement integration test to reproduce #7463

* Make new iptables_state checks async

* Add missing commit to iptable_state integration test

* Remove async when using checkmode in iptables_state integration tests

* Do per table comparison in check mode for iptables_state

* Calculate changes of iptables state per table based on result

* Output target iptables state in checkmode

* Refactor calculation of invidual table states in iptables_state

* Add missing return for table calculation

* Add missing arg to regex check

* Remove leftover debug output for target iptable state

* Parse per table state from raw state string

* Join restored state for extration of table specific rules

* Switch arguments for joining restored iptable state

* Output final ip table state

* Compare content of tables

* Complete iptables partial tables test cases

* Correct order of test iptables data

* Update docu for iptables tables_after

* Add changelog fragment

* Appease the linting gods for iptables_state

* Adjust spelling and remove tables_after from return values

changelogs/fragments/8029-iptables-state-restore-check-mode.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - iptables_state - fix idempotency issues when restoring incomplete iptables dumps (https://github.com/ansible-collections/community.general/issues/8029).

plugins/modules/iptables_state.py

@@ -207,7 +207,9 @@ saved:
       "# Completed"
     ]
 tables:
-  description: The iptables we have interest for when module starts.
+  description:
+    - The iptables on the system before the module has run, separated by table.
+    - If the option O(table) is used, only this table is included.
   type: dict
   contains:
     table:
@@ -346,20 +348,27 @@ def filter_and_format_state(string):
     return lines
 
 
-def per_table_state(command, state):
+def parse_per_table_state(all_states_dump):
     '''
     Convert raw iptables-save output into usable datastructure, for reliable
     comparisons between initial and final states.
     '''
+    lines = filter_and_format_state(all_states_dump)
     tables = dict()
-    for t in TABLES:
-        COMMAND = list(command)
-        if '*%s' % t in state.splitlines():
-            COMMAND.extend(['--table', t])
-            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)
-            tables[t] = [tt for tt in out.splitlines() if tt != '']
+    current_table = ''
+    current_list = list()
+    for line in lines:
+        if re.match(r'^[*](filter|mangle|nat|raw|security)$', line):
+            current_table = line[1:]
+            continue
+        if line == 'COMMIT':
+            tables[current_table] = current_list
+            current_table = ''
+            current_list = list()
+            continue
+        if line.startswith('# '):
+            continue
+        current_list.append(line)
     return tables
 
 
@@ -486,7 +495,7 @@ def main():
     # Depending on the value of 'table', initref_state may differ from
     # initial_state.
     (rc, stdout, stderr) = module.run_command(SAVECOMMAND, check_rc=True)
-    tables_before = per_table_state(SAVECOMMAND, stdout)
+    tables_before = parse_per_table_state(stdout)
     initref_state = filter_and_format_state(stdout)
 
     if state == 'saved':
@@ -583,14 +592,17 @@ def main():
 
         (rc, stdout, stderr) = module.run_command(SAVECOMMAND, check_rc=True)
         restored_state = filter_and_format_state(stdout)
-
+    tables_after = parse_per_table_state('\n'.join(restored_state))
     if restored_state not in (initref_state, initial_state):
-        if module.check_mode:
+        for table_name, table_content in tables_after.items():
+            if table_name not in tables_before:
+                # Would initialize a table, which doesn't exist yet
                 changed = True
-        else:
-            tables_after = per_table_state(SAVECOMMAND, stdout)
-            if tables_after != tables_before:
+                break
+            if tables_before[table_name] != table_content:
+                # Content of some table changes
                 changed = True
+                break
 
     if _back is None or module.check_mode:
         module.exit_json(
@@ -633,7 +645,7 @@ def main():
     os.remove(b_back)
 
     (rc, stdout, stderr) = module.run_command(SAVECOMMAND, check_rc=True)
-    tables_rollback = per_table_state(SAVECOMMAND, stdout)
+    tables_rollback = parse_per_table_state(stdout)
 
     msg = (
         "Failed to confirm state restored from %s after %ss. "

tests/integration/targets/iptables_state/tasks/main.yml

@@ -29,6 +29,12 @@
       when:
         - xtables_lock is undefined
 
+    - name: include tasks to test partial restore files
+      include_tasks: tests/02-partial-restore.yml
+      when:
+        - xtables_lock is undefined
+
+
     - name: include tasks to test rollbacks
       include_tasks: tests/10-rollback.yml
       when:

tests/integration/targets/iptables_state/tasks/tests/02-partial-restore.yml

@@ -0,0 +1,66 @@
+---
+# Copyright (c) Ansible Project
+# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt)
+# SPDX-License-Identifier: GPL-3.0-or-later
+
+- name: "Create initial rule set to use"
+  copy:
+    dest: "{{ iptables_tests }}"
+    content: |
+      *filter
+      :INPUT ACCEPT [0:0]
+      :FORWARD ACCEPT [0:0]
+      :OUTPUT ACCEPT [0:0]
+      -A INPUT -m state --state NEW,ESTABLISHED -j ACCEPT
+      COMMIT
+      *nat
+      :PREROUTING ACCEPT [151:17304]
+      :INPUT ACCEPT [151:17304]
+      :OUTPUT ACCEPT [151:17304]
+      :POSTROUTING ACCEPT [151:17304]
+      -A POSTROUTING -o eth0 -j MASQUERADE
+      COMMIT
+
+- name: "Restore initial state"
+  iptables_state:
+    path: "{{ iptables_tests }}"
+    state: restored
+  async: "{{ ansible_timeout }}"
+  poll: 0
+
+- name: "Create partial ruleset only specifying input"
+  copy:
+    dest: "{{ iptables_tests }}"
+    content: |
+      *filter
+      :INPUT ACCEPT [0:0]
+      :FORWARD ACCEPT [0:0]
+      :OUTPUT ACCEPT [0:0]
+      -A INPUT -m state --state NEW,ESTABLISHED -j ACCEPT
+      COMMIT
+
+- name: "Check restoring partial state"
+  iptables_state:
+    path: "{{ iptables_tests }}"
+    state: restored
+  check_mode: true
+  register: iptables_state
+
+
+- name: "assert that no changes are detected in check mode"
+  assert:
+    that:
+      - iptables_state is not changed
+
+- name: "Restore partial state"
+  iptables_state:
+    path: "{{ iptables_tests }}"
+    state: restored
+  register: iptables_state
+  async: "{{ ansible_timeout }}"
+  poll: 0
+
+- name: "assert that no changes are made"
+  assert:
+    that:
+      - iptables_state is not changed