Fix check mode in iptables_state for incomplete iptables-save files along with integration tests

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

@@ -0,0 +1,2 @@
+bugfixes:
+  - iptables_state - Fix idempotency issues when restoring incomplete iptables dumps and add target state as data structure to output (https://github.com/ansible-collections/community.general/issues/8029).

plugins/modules/iptables_state.py

@@ -207,7 +207,9 @@
       "# 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:
@@ -233,6 +235,36 @@
       ]
     }
   returned: always
+tables_after:
+  description:
+    - The iptables on the system after the module has run, separated by table.
+    - If the option table is used, only this table is included.
+    - If the module fails and no changes are made, tables_after will have the same content as tables
+  type: dict
+  contains:
+    table:
+      description: Policies and rules for all chains of the named table.
+      type: list
+      elements: str
+  sample: |-
+    {
+      "filter": [
+        ":INPUT ACCEPT",
+        ":FORWARD ACCEPT",
+        ":OUTPUT ACCEPT",
+        "-A INPUT -i lo -j ACCEPT",
+        "-A INPUT -p icmp -j ACCEPT",
+        "-A INPUT -p tcp -m tcp --dport 22 -j ACCEPT",
+        "-A INPUT -j REJECT --reject-with icmp-host-prohibited"
+      ],
+      "nat": [
+        ":PREROUTING ACCEPT",
+        ":INPUT ACCEPT",
+        ":OUTPUT ACCEPT",
+        ":POSTROUTING ACCEPT"
+      ]
+    }
+  returned: Only if O(state=restored)
 '''
 
 
@@ -346,20 +378,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 +525,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':
@@ -543,6 +582,7 @@ def main():
                 stdout=stdout,
                 stderr=stderr,
                 tables=tables_before,
+                tables_after=tables_before,
                 initial_state=initial_state,
                 restored=state_to_restore,
                 applied=False)
@@ -577,26 +617,31 @@ def main():
                 stdout=stdout,
                 stderr=stderr,
                 tables=tables_before,
+                tables_after=tables_before,
                 initial_state=initial_state,
                 restored=state_to_restore,
                 applied=False)
 
         (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:
-            changed = True
-        else:
-            tables_after = per_table_state(SAVECOMMAND, stdout)
-            if tables_after != tables_before:
+        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
+                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(
             changed=changed,
             cmd=cmd,
             tables=tables_before,
+            tables_after=tables_after,
             initial_state=initial_state,
             restored=restored_state,
             applied=True)
@@ -622,6 +667,7 @@ def main():
             changed=changed,
             cmd=cmd,
             tables=tables_before,
+            tables_after=tables_after,
             initial_state=initial_state,
             restored=restored_state,
             applied=True)
@@ -633,7 +679,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. "
@@ -645,6 +691,7 @@ def main():
         msg=msg,
         cmd=cmd,
         tables=tables_before,
+        tables_after=tables_after,
         initial_state=initial_state,
         restored=restored_state,
         applied=False)

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