fix: state no longer required for masquerade and ICMP block inversion

- The above, and added a new error message for attempting to not specify state when using options that require state like source, port, port_forward

-- Unit test case added for this error message

- Removed state option from integration tests for masquerading and ICMP block, retaining the same fail conditions

Fixes: #76

Author: BrennanPaciorek

library/firewall_lib.py

@@ -154,7 +154,7 @@
     description:
       Ensure presence or absence of entries.  Use C(present) and C(absent) only
       for zone-only operations, or for target operations.
-    required: true
+    required: false
     type: str
     choices: ["enabled", "disabled", "present", "absent"]
   __report_changed:
@@ -315,7 +315,9 @@ def main():
                 ],
             ),
             state=dict(
-                choices=["enabled", "disabled", "present", "absent"], required=True
+                choices=["enabled", "disabled", "present", "absent"],
+                required=False,
+                default=None,
             ),
             __report_changed=dict(required=False, type="bool", default=True),
         ),
@@ -359,6 +361,19 @@ def main():
     runtime = module.params["runtime"]
     state = module.params["state"]
 
+    # All options that require state to be set
+    state_required = any(
+        (
+            interface,
+            source,
+            service,
+            source_port,
+            port,
+            forward_port,
+            icmp_block,
+            rich_rule,
+        )
+    )
     if permanent is None:
         runtime = True
     elif not any((permanent, runtime)):
@@ -460,6 +475,9 @@ def main():
     if len(source) > 0 and permanent is None:
         module.fail_json(msg="source cannot be set without permanent")
 
+    if state is None and state_required:
+        module.fail_json(msg="Options invalid without state option set")
+
     if not HAS_FIREWALLD:
         module.fail_json(msg="No firewalld")
 

tasks/main.yml

@@ -55,7 +55,7 @@
     target: "{{ item.target | default(omit) }}"
     permanent: "{{ item.permanent | default(True) }}"
     runtime: "{{ item.runtime | default(True) }}"
-    state: "{{ item.state }}"
+    state: "{{ item.state | default(omit) }}"
     __report_changed: "{{ not __firewall_previous_replaced }}"
   loop: "{{ firewall is mapping | ternary([firewall], firewall) |
     map('dict2items') | map('difference', __previous) | select |

tests/tests_ansible.yml

@@ -200,7 +200,6 @@
       - name: Allow masquerading in permament dmz zone
         firewall_lib:
           masquerade: yes
-          state: enabled
           permanent: yes
           zone: dmz
         register: result
@@ -209,7 +208,6 @@
       - name: Allow masquerading in permament dmz zone, again
         firewall_lib:
           masquerade: yes
-          state: enabled
           permanent: yes
           zone: dmz
         register: result
@@ -226,7 +224,6 @@
       - name: Ensure ICMP block inversion in permanent drop zone
         firewall_lib:
           zone: drop
-          state: enabled
           permanent: yes
           icmp_block_inversion: yes
         register: result
@@ -235,7 +232,6 @@
       - name: Ensure ICMP block inversion in permanent drop zone, again
         firewall_lib:
           zone: drop
-          state: enabled
           permanent: yes
           icmp_block_inversion: yes
         register: result

tests/tests_purge_config.yml

@@ -18,7 +18,6 @@
                        '448/tcp;;1.2.3.5']
         state: enabled
       - masquerade: yes
-        state: enabled
       - service: http
         state: enabled
   tasks:
@@ -113,7 +112,6 @@
           vars:
             firewall:
               - set_default_zone: dmz
-                state: enabled
 
         - name: Verify role reports changed
           fail:
@@ -126,7 +124,6 @@
           vars:
             firewall:
               - set_default_zone: dmz
-                state: enabled
 
         - name: Verify role reports not changed
           fail:
@@ -140,7 +137,6 @@
             firewall:
               - previous: replaced
               - set_default_zone: dmz
-                state: enabled
 
         - name: Verify role reports not changed
           fail:

tests/tests_target.yml

@@ -8,7 +8,6 @@
           vars:
             firewall:
               - set_default_zone: public
-                state: enabled
                 permanent: yes
               - target: DROP
                 state: enabled

tests/unit/test_firewall_lib.py

@@ -383,6 +383,14 @@ def test_main_error_no_params(self, am_class):
             "icmp_block_inversion, target, zone or set_default_zone needs to be set"
         )
 
+    @patch("firewall_lib.HAS_FIREWALLD", True)
+    def test_main_error_state_required_for_options(self, am_class):
+        am = am_class.return_value
+        am.params = {"permanent": True, "source": ["192.0.2.0/24"]}
+        with self.assertRaises(MockException):
+            firewall_lib.main()
+        am.fail_json.assert_called_with(msg="Options invalid without state option set")
+
     @patch("firewall_lib.HAS_FIREWALLD", True)
     def test_main_error_timeout_icmp_block_inversion(self, am_class):
         am = am_class.return_value