proxmox_firewall: Fix minor bugs and sanity issues

- When state is absent and pos is 0 if condition with pos was failing.
  to fix it explicitly check if pos is not None.

Author: JanaHoch

plugins/module_utils/proxmox.py

@@ -102,20 +102,20 @@ def compare_list_of_dicts(existing_list, new_list, uid, params_to_ignore=None):
 
     common_uids = set(existing_list.keys()).intersection(set(new_list.keys()))
     missing_uids = set(new_list.keys()) - set(existing_list.keys())
-    items_to_create = [new_list[id] for id in missing_uids]
+    items_to_create = [new_list[uid] for uid in missing_uids]
 
-    for id in common_uids:
+    for uid in common_uids:
         # If new rule has a parameter that is not present in existing rule we need to update
-        if set(new_list[id].keys()) - set(existing_list[id].keys()) != set():
-            items_to_update.append(new_list[id])
+        if set(new_list[uid].keys()) - set(existing_list[uid].keys()) != set():
+            items_to_update.append(new_list[uid])
             continue
 
         # If existing rule param value doesn't match new rule param OR
         # If existing rule has a param that is not present in new rule except for params in params_to_ignore
-        for existing_rule_param, existing_parm_value in existing_list[id].items():
+        for existing_rule_param, existing_parm_value in existing_list[uid].items():
             if (existing_rule_param not in params_to_ignore and
-                    new_list[id].get(existing_rule_param) != existing_parm_value):
-                items_to_update.append(new_list[id])
+                    new_list[uid].get(existing_rule_param) != existing_parm_value):
+                items_to_update.append(new_list[uid])
 
     return items_to_create, items_to_update
 

plugins/module_utils/proxmox_sdn.py

@@ -113,7 +113,7 @@ def get_aliases(self, firewall_obj):
             return firewall_obj().aliases().get()
         except Exception as e:
             self.module.fail_json(
-                msg='Failed to retrieve aliases'
+                msg=f'Failed to retrieve aliases - {e}'
             )
 
     def get_fw_rules(self, rules_obj, pos=None):
@@ -123,13 +123,14 @@ def get_fw_rules(self, rules_obj, pos=None):
         :param pos: Rule position if it is None it'll return all rules
         :return: Firewall rules as a list of dict
         """
-        if pos:
-            try:
-                return rules_obj(str(pos)).get()
-            except Exception as e:
-                self.module.fail_json(
-                    msg=f'Failed to retrieve firewall rules: {e}'
-                )
+        if pos is not None:
+            pos = str(pos)
+        try:
+            return rules_obj(pos).get()
+        except Exception as e:
+            self.module.fail_json(
+                msg=f'Failed to retrieve firewall rules: {e}'
+            )
 
     def get_groups(self):
         """Get firewall security groups

plugins/modules/proxmox_firewall.py

@@ -14,8 +14,9 @@
 short_description: Manage firewall rules in Proxmox
 version_added: "1.4.0"
 description:
-    - Create/update/delete firewall rules at cluster/group/vnet/node/vm level. 
-    - Create/delete firewall security groups.
+    - create/update/delete FW rules at cluster/group/vnet/node/vm level
+    - Create/delete firewall security groups
+    - Create/delete aliases
 author: 'Jana Hoch <[email protected]> (!UNKNOWN)'
 attributes:
   check_mode:
@@ -25,15 +26,15 @@
 options:
   state:
     description:
-      - Create/update/delete firewall rules or security group. 
+      - Create/update/delete firewall rules or security group.
     type: str
     choices:
       - present
       - absent
     default: present
   update:
     description:
-      - If O(state=present) and if one or more rule/alias already exists it will update them. 
+      - If O(state=present) and if one or more rule/alias already exists it will update them.
     type: bool
     default: true
   level:
@@ -427,7 +428,8 @@ def validate_params(self):
                     msg="When state is present either group_conf should be true or rules/aliases must be present but not both"
                 )
         elif self.params.get('state') == 'absent':
-            if self.params.get('group_conf') != bool(self.params.get('pos') or self.params.get('aliases')):
+            if self.params.get('group_conf') != bool(
+                    (self.params.get('pos') is not None) or self.params.get('aliases')):
                 return True
             else:
                 self.module.fail_json(
@@ -482,7 +484,7 @@ def run(self):
             if aliases:
                 self.aliases_present(firewall_obj=firewall_obj, level=level, aliases=aliases, update=update)
         elif state == "absent":
-            if self.params.get('pos'):
+            if self.params.get('pos') is not None:
                 self.fw_rule_absent(rules_obj=rules_obj, pos=self.params.get('pos'))
             if group_conf:
                 self.group_absent(group_name=group)

plugins/modules/proxmox_firewall_info.py

@@ -15,6 +15,8 @@
 version_added: "1.4.0"
 description:
     - Get firewall rules at cluster/group/vnet/node/vm level.
+    - Get firewall security groups at cluster level.
+    - Get aliases at cluster/VM level.
 author: 'Jana Hoch <[email protected]> (!UNKNOWN)'
 options:
   level:

tests/unit/plugins/modules/test_proxmox_firewall.py

@@ -123,7 +123,7 @@ def setUp(self):
         self.connect_mock = patch(
             "ansible_collections.community.proxmox.plugins.module_utils.proxmox.ProxmoxAnsible._connect",
         ).start()
-        self.connect_mock.return_value.cluster.return_value.firewall.return_value.rules.get.return_value = RAW_FIREWALL_RULES
+        self.connect_mock.return_value.cluster.return_value.firewall.return_value.rules.return_value.get.return_value = RAW_FIREWALL_RULES
         self.connect_mock.return_value.cluster.return_value.firewall.return_value.groups.return_value.get.return_value = RAW_GROUPS
 
     def tearDown(self):
@@ -159,7 +159,7 @@ def test_create_fw_rules(self):
 
     def test_delete_fw_rule(self):
         with pytest.raises(SystemExit) as exc_info:
-            with set_module_args(get_module_args_fw_delete(state='absent', pos=1)):
+            with set_module_args(get_module_args_fw_delete(state='absent', pos=0)):
                 self.module.main()
         result = exc_info.value.args[0]
         assert result['changed'] is True

tests/unit/plugins/modules/test_proxmox_firewall_info.py

@@ -147,11 +147,11 @@ def setUp(self):
         mock_cluster_fw = self.connect_mock.return_value.cluster.return_value.firewall.return_value
         mock_vm100_fw = self.connect_mock.return_value.nodes.return_value.return_value.return_value.firewall.return_value
 
-        mock_cluster_fw.rules.get.return_value = RAW_FIREWALL_RULES
+        mock_cluster_fw.rules.return_value.get.return_value = RAW_FIREWALL_RULES
         mock_cluster_fw.groups.return_value.get.return_value = RAW_GROUPS
         mock_cluster_fw.aliases.return_value.get.return_value = RAW_ALIASES
 
-        mock_vm100_fw.rules.get.return_value = RAW_FIREWALL_RULES
+        mock_vm100_fw.rules.return_value.get.return_value = RAW_FIREWALL_RULES
         mock_vm100_fw.aliases.return_value.get.return_value = RAW_ALIASES
 
     def tearDown(self):