From d00c335828a48383f86340268eb44e203ae25036 Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sat, 6 Sep 2025 23:21:22 +0530 Subject: [PATCH 01/22] proxmox_firewall: new_module for firewall config - Added method to get FW rules at cluster, node, vm, vnet levels --- plugins/modules/proxmox_firewall.py | 122 ++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+) create mode 100644 plugins/modules/proxmox_firewall.py diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py new file mode 100644 index 00000000..7df3fd41 --- /dev/null +++ b/plugins/modules/proxmox_firewall.py @@ -0,0 +1,122 @@ +#!/usr/bin/python +# -*- coding: utf-8 -*- +# +# Copyright (c) 2025, Jana Hoch +# 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 + +from __future__ import absolute_import, division, print_function + +__metaclass__ = type + +DOCUMENTATION = r"""""" + +EXAMPLES = r"""""" + +RETURN = r"""""" + +from ansible.module_utils.basic import AnsibleModule +from ansible_collections.community.proxmox.plugins.module_utils.proxmox import ( + proxmox_auth_argument_spec, + ansible_to_proxmox_bool, + ProxmoxAnsible +) + + +def get_proxmox_args(): + return dict( + state=dict(type="str", choices=["present", "absent", "update"], required=False), + force=dict(type="bool", default=False, required=False), + level=dict(type="str", choices=["cluster", "node", "vm", "vnet"], default="cluster", required=False), + node=dict(type="str", required=False), + vmid=dict(type="int", required=False), + vnet=dict(type="str", required=False) + ) + + +def get_ansible_module(): + module_args = proxmox_auth_argument_spec() + module_args.update(get_proxmox_args()) + + return AnsibleModule( + argument_spec=module_args, + required_if=[ + ] + ) + + +class ProxmoxFirewallAnsible(ProxmoxAnsible): + def __init__(self, module): + super(ProxmoxFirewallAnsible, self).__init__(module) + self.params = module.params + + def run(self): + state = self.params.get("state") + force = self.params.get("force") + level = self.params.get("level") + + if level == "vm": + rules = self.get_vmid_fw_rules(vmid=self.params['vmid']) + elif level == "node": + rules = self.get_node_fw_rules(node=self.params['node']) + elif level == "vnet": + rules = self.get_vnet_fw_rules(vnet=self.params['vnet']) + else: + rules = self.get_cluster_fw_rules() + self.module.exit_json( + changed=False, firewall_rules=rules, msg=f'successfully retrieved firewall rules' + ) + + def get_vnet_fw_rules(self, vnet, pos=None): + try: + vnet = getattr(self.proxmox_api.cluster().sdn().vnets(), vnet) + return vnet().firewall().rules().get() + except Exception as e: + self.module.fail_json( + msg=f'Failed to retrieve vnet level firewall rules: {e}' + ) + + def get_cluster_fw_rules(self, pos=None): + try: + return self.proxmox_api.cluster().firewall().rules().get(pos=pos) + except Exception as e: + self.module.fail_json( + msg=f'Failed to retrieve cluster level firewall rules: {e}' + ) + + def get_node_fw_rules(self, node, pos=None): + try: + node = getattr(self.proxmox_api.nodes(), node) + return node().firewall().rules().get(pos=pos) + except Exception as e: + self.module.fail_json( + msg=f'Failed to retrieve cluster level firewall rules: {e}' + ) + + def get_vmid_fw_rules(self, vmid, pos=None): + try: + vm = self.get_vm(vmid=vmid) + + node = getattr(self.proxmox_api.nodes(), vm['node']) + virt = getattr(node(), vm['type']) + vm = getattr(virt(), vmid) + + return vm().firewall().rules().get() + except Exception as e: + self.module.fail_json( + msg=f'Failed to retrieve firewall rules for vmid - {vmid}: {e}' + ) + + +def main(): + module = get_ansible_module() + proxmox = ProxmoxFirewallAnsible(module) + + try: + proxmox.run() + except Exception as e: + module.fail_json(msg=f'An error occurred: {e}') + + +if __name__ == "__main__": + main() From 5185516d4bdcab043453e5da96daaa67cfc7ec65 Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sun, 7 Sep 2025 05:48:01 +0530 Subject: [PATCH 02/22] proxmox_firewall: Add condition for security group --- plugins/modules/proxmox_firewall.py | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index 7df3fd41..e458ea9c 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -27,7 +27,7 @@ def get_proxmox_args(): return dict( state=dict(type="str", choices=["present", "absent", "update"], required=False), force=dict(type="bool", default=False, required=False), - level=dict(type="str", choices=["cluster", "node", "vm", "vnet"], default="cluster", required=False), + level=dict(type="str", choices=["cluster", "node", "vm", "vnet", "group"], default="cluster", required=False), node=dict(type="str", required=False), vmid=dict(type="int", required=False), vnet=dict(type="str", required=False) @@ -61,16 +61,27 @@ def run(self): rules = self.get_node_fw_rules(node=self.params['node']) elif level == "vnet": rules = self.get_vnet_fw_rules(vnet=self.params['vnet']) + elif level == "group": + rules = self.get_group_fw_rules(group=self.params['group']) else: rules = self.get_cluster_fw_rules() self.module.exit_json( changed=False, firewall_rules=rules, msg=f'successfully retrieved firewall rules' ) + def get_group_fw_rules(self, group, pos=None): + try: + group = getattr(self.proxmox_api.cluster().firewall().groups(), group) + return group().get(pos=pos) + except Exception as e: + self.module.fail_json( + msg=f'Failed to retrieve security group level firewall rules: {e}' + ) + def get_vnet_fw_rules(self, vnet, pos=None): try: vnet = getattr(self.proxmox_api.cluster().sdn().vnets(), vnet) - return vnet().firewall().rules().get() + return vnet().firewall().rules().get(pos=pos) except Exception as e: self.module.fail_json( msg=f'Failed to retrieve vnet level firewall rules: {e}' @@ -101,7 +112,7 @@ def get_vmid_fw_rules(self, vmid, pos=None): virt = getattr(node(), vm['type']) vm = getattr(virt(), vmid) - return vm().firewall().rules().get() + return vm().firewall().rules().get(pos=pos) except Exception as e: self.module.fail_json( msg=f'Failed to retrieve firewall rules for vmid - {vmid}: {e}' From bcf44463fe8c97f712a5a33d07d1a9f129809a73 Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sun, 7 Sep 2025 15:38:07 +0530 Subject: [PATCH 03/22] proxmox_firewall: Add method to create rule at cluster level --- plugins/modules/proxmox_firewall.py | 116 ++++++++++++++++++++++++---- 1 file changed, 102 insertions(+), 14 deletions(-) diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index e458ea9c..f4f959ed 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -30,7 +30,31 @@ def get_proxmox_args(): level=dict(type="str", choices=["cluster", "node", "vm", "vnet", "group"], default="cluster", required=False), node=dict(type="str", required=False), vmid=dict(type="int", required=False), - vnet=dict(type="str", required=False) + vnet=dict(type="str", required=False), + rules=dict( + type="list", + elements="dict", + required=False, + options=dict( + action=dict(type="str", required=True), + type=dict(type="str", choices=["in", "out", "forward", "group"], required=True), + comment=dict(type="str", required=False), + dest=dict(type="str", required=False), + digest=dict(type="str", required=False), + dport=dict(type="str", required=False), + enable=dict(type="bool", required=False), + icmp_type=dict(type="str", required=False), + iface=dict(type="str", required=False), + log=dict(type="str", + choices=["emerg", "alert", "crit", "err", "warning", "notice", "info", "debug", "nolog"], + required=False), + macro=dict(type="str", required=False), + pos=dict(type="int", required=False), + proto=dict(type="str", required=False), + source=dict(type="str", required=False), + sport=dict(type="str", required=False) + ) + ) ) @@ -54,20 +78,37 @@ def run(self): state = self.params.get("state") force = self.params.get("force") level = self.params.get("level") - - if level == "vm": - rules = self.get_vmid_fw_rules(vmid=self.params['vmid']) - elif level == "node": - rules = self.get_node_fw_rules(node=self.params['node']) - elif level == "vnet": - rules = self.get_vnet_fw_rules(vnet=self.params['vnet']) - elif level == "group": - rules = self.get_group_fw_rules(group=self.params['group']) + rules =self.params.get("rules") + + # if rules is not None: + # rules = [rules.get('icmp_type') + + if state == "present": + if level == "vm": + pass + elif level == "node": + pass + elif level == "vnet": + pass + elif level == "group": + pass + else: + if rules is not None: + self.create_cluster_fw_rules(rules=rules) else: - rules = self.get_cluster_fw_rules() - self.module.exit_json( - changed=False, firewall_rules=rules, msg=f'successfully retrieved firewall rules' - ) + if level == "vm": + rules = self.get_vmid_fw_rules(vmid=self.params['vmid']) + elif level == "node": + rules = self.get_node_fw_rules(node=self.params['node']) + elif level == "vnet": + rules = self.get_vnet_fw_rules(vnet=self.params['vnet']) + elif level == "group": + rules = self.get_group_fw_rules(group=self.params['group']) + else: + rules = self.get_cluster_fw_rules() + self.module.exit_json( + changed=False, firewall_rules=rules, msg=f'successfully retrieved firewall rules' + ) def get_group_fw_rules(self, group, pos=None): try: @@ -118,6 +159,53 @@ def get_vmid_fw_rules(self, vmid, pos=None): msg=f'Failed to retrieve firewall rules for vmid - {vmid}: {e}' ) + def create_cluster_fw_rules(self, rules): + for rule in rules: + rule['icmp-type'] = rule.get('icmp_type') + rule['enable'] = ansible_to_proxmox_bool(rule.get('enable')) + del rule['icmp_type'] + try: + firewall_obj = self.proxmox_api.cluster().firewall + firewall_obj().rules().post(**rule) + self.move_rule_to_correct_pos(firewall_obj, rule) + + except Exception as e: + self.module.fail_json( + msg=f'Failed to create firewall rule {rule}: {e}' + ) + else: + self.module.exit_json( + changed=True, msg=f'successfully created firewall rules' + ) + + def move_rule_to_correct_pos(self, firewall_obj, rule): + ################################################################################################## + # TODO: Once below mentioned issue is fixed. Remove this workaround. # + # Currently Proxmox API doesn't honor pos. All new rules are created at pos 0 # + # https://forum.proxmox.com/threads/issue-when-creating-a-firewall-rule.135878/ # + # Not able to find it in BUGZILLA. So maybe this is expected behaviour. # + # To workaround this issue we will check rule at pos 0 and if needed move it to correct position # + ################################################################################################## + + pos = rule.get('pos') + rule = {k: v for k, v in rule.items() if v is not None} + if pos is not None and pos != 0: + try: + fw_rule_at0 = getattr(firewall_obj().rules(), str(0)) + for param, value, in fw_rule_at0.get().items(): + if param in rule.keys() and param != 'pos' and value != rule.get(param): + self.module.warn( + msg=f'Skipping workaround for rule placement. ' + f'Verify rule is at correct pos ' + f'provided - {rule} rule_at0 - {fw_rule_at0.get()}') + break # No need to move this. Potentially the issue is resolved. + else: + fw_rule_at0.put(moveto=pos+1) # `moveto` moves rule to one position before the value + except Exception as e: + self.module.fail_json( + msg=f'Rule created but failed to move it to correct pos. {e}' + ) + def main(): module = get_ansible_module() From 7e97435f57d76827415c894b3e020f0430bb1e98 Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sun, 7 Sep 2025 19:34:34 +0530 Subject: [PATCH 04/22] proxmox_firewall: Refactor to remove similar functions --- plugins/modules/proxmox_firewall.py | 110 ++++++++++------------------ 1 file changed, 37 insertions(+), 73 deletions(-) diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index f4f959ed..88740ede 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -80,94 +80,58 @@ def run(self): level = self.params.get("level") rules =self.params.get("rules") - # if rules is not None: - # rules = [rules.get('icmp_type') + if level == "vm": + vm = self.get_vm(vmid=self.params.get('vmid')) + node = getattr(self.proxmox_api.nodes(), vm['node']) + virt = getattr(node(), vm['type']) + vm = getattr(virt(), vm['vmid']) + firewall_obj = vm().firewall + rules_obj = firewall_obj().rules - if state == "present": - if level == "vm": - pass - elif level == "node": - pass - elif level == "vnet": - pass - elif level == "group": - pass - else: - if rules is not None: - self.create_cluster_fw_rules(rules=rules) - else: - if level == "vm": - rules = self.get_vmid_fw_rules(vmid=self.params['vmid']) - elif level == "node": - rules = self.get_node_fw_rules(node=self.params['node']) - elif level == "vnet": - rules = self.get_vnet_fw_rules(vnet=self.params['vnet']) - elif level == "group": - rules = self.get_group_fw_rules(group=self.params['group']) - else: - rules = self.get_cluster_fw_rules() - self.module.exit_json( - changed=False, firewall_rules=rules, msg=f'successfully retrieved firewall rules' - ) + elif level == "node": + node = getattr(self.proxmox_api.nodes(), self.params.get('node')) + firewall_obj = node().firewall + rules_obj = firewall_obj().rules - def get_group_fw_rules(self, group, pos=None): - try: - group = getattr(self.proxmox_api.cluster().firewall().groups(), group) - return group().get(pos=pos) - except Exception as e: - self.module.fail_json( - msg=f'Failed to retrieve security group level firewall rules: {e}' - ) + elif level == "vnet": + vnet = getattr(self.proxmox_api.cluster().sdn().vnets(), self.params.get('vnet')) + firewall_obj = vnet().firewall + rules_obj = firewall_obj().rules - def get_vnet_fw_rules(self, vnet, pos=None): - try: - vnet = getattr(self.proxmox_api.cluster().sdn().vnets(), vnet) - return vnet().firewall().rules().get(pos=pos) - except Exception as e: - self.module.fail_json( - msg=f'Failed to retrieve vnet level firewall rules: {e}' - ) + elif level == "group": + rules_obj = getattr(self.proxmox_api.cluster().firewall().groups(), self.params.get('group')) - def get_cluster_fw_rules(self, pos=None): - try: - return self.proxmox_api.cluster().firewall().rules().get(pos=pos) - except Exception as e: - self.module.fail_json( - msg=f'Failed to retrieve cluster level firewall rules: {e}' - ) + else: + firewall_obj = self.proxmox_api.cluster().firewall + rules_obj = firewall_obj().rules - def get_node_fw_rules(self, node, pos=None): - try: - node = getattr(self.proxmox_api.nodes(), node) - return node().firewall().rules().get(pos=pos) - except Exception as e: - self.module.fail_json( - msg=f'Failed to retrieve cluster level firewall rules: {e}' + if state == "present": + if rules is not None: + self.create_fw_rules(rules_obj=rules_obj, rules=rules) + else: + rules = self.get_fw_rules(rules_obj) + self.module.exit_json( + changed=False, firewall_rules=rules, msg=f'successfully retrieved firewall rules' ) - def get_vmid_fw_rules(self, vmid, pos=None): + def get_fw_rules(self, rules_obj, pos=None): + if pos is not None: + rules_obj = getattr(rules_obj(), str(pos)) try: - vm = self.get_vm(vmid=vmid) - - node = getattr(self.proxmox_api.nodes(), vm['node']) - virt = getattr(node(), vm['type']) - vm = getattr(virt(), vmid) - - return vm().firewall().rules().get(pos=pos) + return rules_obj.get() except Exception as e: self.module.fail_json( - msg=f'Failed to retrieve firewall rules for vmid - {vmid}: {e}' + msg=f'Failed to retrieve firewall rules: {e}' ) - def create_cluster_fw_rules(self, rules): + def create_fw_rules(self, rules_obj, rules): for rule in rules: rule['icmp-type'] = rule.get('icmp_type') rule['enable'] = ansible_to_proxmox_bool(rule.get('enable')) del rule['icmp_type'] try: - firewall_obj = self.proxmox_api.cluster().firewall - firewall_obj().rules().post(**rule) - self.move_rule_to_correct_pos(firewall_obj, rule) + rules_obj().post(**rule) + self.move_rule_to_correct_pos(rules_obj, rule) except Exception as e: self.module.fail_json( @@ -178,7 +142,7 @@ def create_cluster_fw_rules(self, rules): changed=True, msg=f'successfully created firewall rules' ) - def move_rule_to_correct_pos(self, firewall_obj, rule): + def move_rule_to_correct_pos(self, rules_obj, rule): ################################################################################################## # TODO: Once below mentioned issue is fixed. Remove this workaround. # # Currently Proxmox API doesn't honor pos. All new rules are created at pos 0 # @@ -191,7 +155,7 @@ def move_rule_to_correct_pos(self, firewall_obj, rule): rule = {k: v for k, v in rule.items() if v is not None} if pos is not None and pos != 0: try: - fw_rule_at0 = getattr(firewall_obj().rules(), str(0)) + fw_rule_at0 = getattr(rules_obj(), str(0)) for param, value, in fw_rule_at0.get().items(): if param in rule.keys() and param != 'pos' and value != rule.get(param): self.module.warn( From 32fc55f7ef55bd71f7efdc1136b3faa39d750a0a Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sun, 7 Sep 2025 19:56:18 +0530 Subject: [PATCH 05/22] proxmox_firewall: Add method to update fw rules --- plugins/modules/proxmox_firewall.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index 88740ede..57dbf720 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -108,6 +108,9 @@ def run(self): if state == "present": if rules is not None: self.create_fw_rules(rules_obj=rules_obj, rules=rules) + elif state == "update": + if rules is not None: + self.update_fw_rules(rules_obj=rules_obj, rules=rules) else: rules = self.get_fw_rules(rules_obj) self.module.exit_json( @@ -124,6 +127,25 @@ def get_fw_rules(self, rules_obj, pos=None): msg=f'Failed to retrieve firewall rules: {e}' ) + def update_fw_rules(self, rules_obj, rules): + for rule in rules: + rule['icmp-type'] = rule.get('icmp_type') + rule['enable'] = ansible_to_proxmox_bool(rule.get('enable')) + del rule['icmp_type'] + try: + rule_obj = getattr(rules_obj(), str(rule['pos'])) + rule['digest'] = rule_obj.get().get('digest') # Avoids concurrent changes + rule_obj.put(**rule) + + except Exception as e: + self.module.fail_json( + msg=f'Failed to update firewall rule at pos {rule["pos"]}: {e}' + ) + else: + self.module.exit_json( + changed=True, msg=f'successfully created firewall rules' + ) + def create_fw_rules(self, rules_obj, rules): for rule in rules: rule['icmp-type'] = rule.get('icmp_type') From f5f231c7eef9609d20498ea668eeb770b427646d Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sun, 7 Sep 2025 20:08:38 +0530 Subject: [PATCH 06/22] proxmox_firewall: Added method to delete rule --- plugins/modules/proxmox_firewall.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index 57dbf720..10ffa42e 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -31,6 +31,7 @@ def get_proxmox_args(): node=dict(type="str", required=False), vmid=dict(type="int", required=False), vnet=dict(type="str", required=False), + pos=dict(type="int", required=False), rules=dict( type="list", elements="dict", @@ -111,6 +112,8 @@ def run(self): elif state == "update": if rules is not None: self.update_fw_rules(rules_obj=rules_obj, rules=rules) + elif state == "absent": + self.delete_fw_rule(rules_obj=rules_obj, pos=self.params.get('pos')) else: rules = self.get_fw_rules(rules_obj) self.module.exit_json( @@ -127,6 +130,20 @@ def get_fw_rules(self, rules_obj, pos=None): msg=f'Failed to retrieve firewall rules: {e}' ) + def delete_fw_rule(self, rules_obj, pos): + try: + rule_obj = getattr(rules_obj(), str(pos)) + digest = rule_obj.get().get('digest') + rule_obj.delete(pos=pos, digest=digest) + + self.module.exit_json( + changed=True, msg=f'successfully deleted firewall rules' + ) + except Exception as e: + self.module.fail_json( + msg=f'Failed to delete firewall rule at pos {pos}: {e}' + ) + def update_fw_rules(self, rules_obj, rules): for rule in rules: rule['icmp-type'] = rule.get('icmp_type') From 7d0b4012f0297ef7e7bb7b8b14e2633295d9b6e2 Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sun, 7 Sep 2025 20:38:08 +0530 Subject: [PATCH 07/22] proxmox_firewall: create/delete group --- plugins/modules/proxmox_firewall.py | 37 +++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index 10ffa42e..5025ff00 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -32,6 +32,9 @@ def get_proxmox_args(): vmid=dict(type="int", required=False), vnet=dict(type="str", required=False), pos=dict(type="int", required=False), + group_conf=dict(type="bool", default=False), + group=dict(type="str", required=False), + comment=dict(type="str", required=False), rules=dict( type="list", elements="dict", @@ -107,19 +110,49 @@ def run(self): rules_obj = firewall_obj().rules if state == "present": + if self.params.get('group_conf'): + self.create_group(group=self.params.get('group'), comment=self.params.get('comment')) if rules is not None: self.create_fw_rules(rules_obj=rules_obj, rules=rules) elif state == "update": + if self.params.get('group_conf'): + self.create_group(group=self.params.get('group'), comment=self.params.get('comment')) if rules is not None: self.update_fw_rules(rules_obj=rules_obj, rules=rules) elif state == "absent": - self.delete_fw_rule(rules_obj=rules_obj, pos=self.params.get('pos')) + if self.params.get('pos'): + self.delete_fw_rule(rules_obj=rules_obj, pos=self.params.get('pos')) + if self.params.get('group_conf'): + self.delete_group(group_name=self.params.get('group')) else: - rules = self.get_fw_rules(rules_obj) + rules = self.get_fw_rules(rules_obj, pos=self.params.get('pos')) self.module.exit_json( changed=False, firewall_rules=rules, msg=f'successfully retrieved firewall rules' ) + def create_group(self, group, comment=None): + try: + self.proxmox_api.cluster().firewall().groups.post(group=group, comment=comment) + self.module.exit_json( + changed=True, group=group, msg=f'successfully created security group {group}' + ) + except Exception as e: + self.module.fail_json( + msg=f'Failed to create security group: {e}' + ) + + def delete_group(self, group_name): + try: + group = getattr(self.proxmox_api.cluster().firewall().groups(), group_name) + group.delete() + self.module.exit_json( + changed=True, group=group_name, msg=f'successfully deleted security group {group_name}' + ) + except Exception as e: + self.module.fail_json( + msg=f'Failed to delete security group {group_name}: {e}' + ) + def get_fw_rules(self, rules_obj, pos=None): if pos is not None: rules_obj = getattr(rules_obj(), str(pos)) From 36aa62306c5e2ebbbd3d0979bcb269ffe8287b59 Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sun, 7 Sep 2025 21:16:24 +0530 Subject: [PATCH 08/22] proxmox_firewall: Added param validations --- plugins/modules/proxmox_firewall.py | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index 5025ff00..ae920aec 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -26,7 +26,6 @@ def get_proxmox_args(): return dict( state=dict(type="str", choices=["present", "absent", "update"], required=False), - force=dict(type="bool", default=False, required=False), level=dict(type="str", choices=["cluster", "node", "vm", "vnet", "group"], default="cluster", required=False), node=dict(type="str", required=False), vmid=dict(type="int", required=False), @@ -69,6 +68,11 @@ def get_ansible_module(): return AnsibleModule( argument_spec=module_args, required_if=[ + ('group_conf', True, ['group']), + ('level', 'vm', ['vmid']), + ('level', 'node', ['node']), + ('level', 'vnet', ['vnet']), + ('level', 'group', ['group']), ] ) @@ -78,11 +82,26 @@ def __init__(self, module): super(ProxmoxFirewallAnsible, self).__init__(module) self.params = module.params + def validate_params(self): + if self.params.get('state') in ['present', 'update']: + return self.params.get('group_conf') or self.params.get('rules') + elif self.params.get('state') == 'absent': + return self.params.get('group_conf') or self.params.get('pos') + else: + return True + + def run(self): + if not self.validate_params(): + self.module.fail_json( + msg=f'parameter validation failed. ' + f'If state is present/update we need either group_conf to be True or rules to be present. ' + f'If state is absent we need group_conf to be True or pos to be present. ' + ) + state = self.params.get("state") - force = self.params.get("force") level = self.params.get("level") - rules =self.params.get("rules") + rules = self.params.get("rules") if level == "vm": vm = self.get_vm(vmid=self.params.get('vmid')) From 47d52ca76ce46663892d768eb78cc7261bf08d11 Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sun, 7 Sep 2025 22:22:51 +0530 Subject: [PATCH 09/22] proxmox_firewall: Added Doc - Fix Sanity issues --- plugins/modules/proxmox_firewall.py | 404 ++++++++++++++++++++++++++-- 1 file changed, 383 insertions(+), 21 deletions(-) diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index ae920aec..fa14211f 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -9,11 +9,376 @@ __metaclass__ = type -DOCUMENTATION = r"""""" - -EXAMPLES = r"""""" - -RETURN = r"""""" +DOCUMENTATION = r""" +module: proxmox_firewall +short_description: Manage firewall rules in Proxmox +description: + - create/update/delete FW rules at cluster/group/vnet/node/vm level + - Create/delete firewall security groups + - get firewall rules at cluster/group/vnet/node/vm level +author: 'Jana Hoch (!UNKNOWN)' +attributes: + check_mode: + support: none + diff_mode: + support: none +options: + state: + description: + - create/update/delete firewall rules or security group + - if state is not provided then it will just list firewall rules at level + type: str + choices: + - present + - update + - absent + level: + description: + - Level at which the firewall rule applies. + type: str + choices: + - cluster + - group + - vnet + - node + - vm + default: cluster + node: + description: + - Name of the node. + - only needed when level is node. + type: str + vmid: + description: + - ID of the VM to which the rule applies. + - only needed when level is vm. + type: int + vnet: + description: + - Name of the virtual network for the rule. + - only needed when level is vnet. + type: str + pos: + description: + - Position of the rule in the list. + - only needed if deleting rule or trying to list it + type: int + group_conf: + description: + - Whether security group should be created or deleted. + type: bool + default: false + group: + description: + - Name of the group to which the rule belongs. + - only needed when level is group or group_conf is True. + type: str + comment: + description: + - Comment for security group. + - Only needed when creating group. + type: str + rules: + description: + - List of individual rules to be applied. + type: list + elements: dict + suboptions: + action: + description: + - Rule action ('ACCEPT', 'DROP', 'REJECT') or security group name. + type: str + required: true + type: + description: + - Rule type. + choices: + - in + - out + - forward + - group + type: str + required: true + comment: + description: + - Optional comment for the specific rule. + type: str + dest: + description: + - Restrict packet destination address. + - This can refer to a single IP address, an IP set ('+ipsetname') or an IP alias definition. + - You can also specify an address range like '20.34.101.207-201.3.9.99', or a list of IP addresses and networks (entries are separated by comma). + - Please do not mix IPv4 and IPv6 addresses inside such lists. + type: str + digest: + description: + - Prevent changes if current configuration file has a different digest. + - This can be used to prevent concurrent modifications. + - If not provided we will calculate at runtime. + type: str + dport: + description: + - Restrict TCP/UDP destination port. + - You can use service names or simple numbers (0-65535), as defined in '/etc/services'. + - Port ranges can be specified with '\d+:\d+', for example '80:85', and you can use comma separated list to match several ports or ranges. + type: str + enable: + description: + - Enable or disable the rule. + type: bool + icmp_type: + description: + - Specify icmp-type. Only valid if proto equals 'icmp' or 'icmpv6'/'ipv6-icmp'. + type: str + iface: + description: + - Network interface name. You have to use network configuration key names for VMs and containers ('net\d+'). + - Host related rules can use arbitrary strings. + type: str + log: + description: + - Logging level for the rule. + choices: + - emerg + - alert + - crit + - err + - warning + - notice + - info + - debug + - nolog + type: str + macro: + description: + - Use predefined standard macro. + type: str + pos: + description: + - Position of the rule in the list. + type: int + proto: + description: + - IP protocol. You can use protocol names ('tcp'/'udp') or simple numbers, as defined in '/etc/protocols'. + type: str + source: + description: + - Restrict packet source address. + - This can refer to a single IP address, an IP set ('+ipsetname') or an IP alias definition. + - You can also specify an address range like '20.34.101.207-201.3.9.99', or a list of IP addresses and networks (entries are separated by comma). + - Please do not mix IPv4 and IPv6 addresses inside such lists. + type: str + sport: + description: + - Restrict TCP/UDP source port. + - You can use service names or simple numbers (0-65535), as defined in '/etc/services'. + - Port ranges can be specified with '\d+:\d+', for example '80:85', and you can use comma separated list to match several ports or ranges. + type: str +extends_documentation_fragment: + - community.proxmox.proxmox.actiongroup_proxmox + - community.proxmox.proxmox.documentation + - community.proxmox.attributes +""" + +EXAMPLES = r""" +- name: Get Cluster level firewall rules + community.proxmox.proxmox_firewall: + api_user: "{{ pc.proxmox.api_user }}" + api_token_id: "{{ pc.proxmox.api_token_id }}" + api_token_secret: "{{ vault.proxmox.api_token_secret }}" + api_host: "{{ pc.proxmox.api_host }}" + validate_certs: no + level: cluster + +- name: Create firewall rules at cluster level + community.proxmox.proxmox_firewall: + api_user: "{{ pc.proxmox.api_user }}" + api_token_id: "{{ pc.proxmox.api_token_id }}" + api_token_secret: "{{ vault.proxmox.api_token_secret }}" + api_host: "{{ pc.proxmox.api_host }}" + validate_certs: no + level: cluster + state: present + rules: + - type: out + action: ACCEPT + source: 1.1.1.1 + log: nolog + pos: 9 + enable: True + - type: out + action: ACCEPT + source: 1.0.0.1 + pos: 10 + enable: True + +- name: Update Cluster level firewall rules + community.proxmox.proxmox_firewall: + api_user: "{{ pc.proxmox.api_user }}" + api_token_id: "{{ pc.proxmox.api_token_id }}" + api_token_secret: "{{ vault.proxmox.api_token_secret }}" + api_host: "{{ pc.proxmox.api_host }}" + validate_certs: no + level: cluster + state: update + rules: + - type: out + action: ACCEPT + source: 8.8.8.8 + log: nolog + pos: 9 + enable: False + - type: out + action: ACCEPT + source: 8.8.4.4 + pos: 10 + enable: False + +- name: Delete cluster level firewall rule at pos 10 + community.proxmox.proxmox_firewall: + api_user: "{{ pc.proxmox.api_user }}" + api_token_id: "{{ pc.proxmox.api_token_id }}" + api_token_secret: "{{ vault.proxmox.api_token_secret }}" + api_host: "{{ pc.proxmox.api_host }}" + validate_certs: no + level: cluster + state: absent + pos: 10 + +- name: Create security group + community.proxmox.proxmox_firewall: + api_user: "{{ pc.proxmox.api_user }}" + api_token_id: "{{ pc.proxmox.api_token_id }}" + api_token_secret: "{{ vault.proxmox.api_token_secret }}" + api_host: "{{ pc.proxmox.api_host }}" + validate_certs: no + group_conf: True + state: present + group: test + +- name: Delete security group + community.proxmox.proxmox_firewall: + api_user: "{{ pc.proxmox.api_user }}" + api_token_id: "{{ pc.proxmox.api_token_id }}" + api_token_secret: "{{ vault.proxmox.api_token_secret }}" + api_host: "{{ pc.proxmox.api_host }}" + validate_certs: no + group_conf: True + state: absent + group: test +""" + +RETURN = r""" +group: + description: group name which was created/deleted + returned: on success + type: str + sample: + test + +firewall_rules: + description: List of firewall rules. + returned: on success + type: list + elements: dict + sample: + [ + { + "action": "ACCEPT", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "dport": "53", + "enable": 1, + "ipversion": 4, + "log": "nolog", + "pos": 0, + "proto": "udp", + "source": "192.168.1.0/24", + "type": "in" + }, + { + "action": "ACCEPT", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "dport": "53", + "enable": 1, + "ipversion": 4, + "log": "nolog", + "pos": 1, + "proto": "tcp", + "source": "192.168.1.0/24", + "type": "in" + }, + { + "action": "ACCEPT", + "dest": "192.168.1.0/24", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "enable": 1, + "ipversion": 4, + "log": "nolog", + "pos": 2, + "type": "out" + }, + { + "action": "ACCEPT", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "enable": 1, + "ipversion": 4, + "log": "nolog", + "pos": 3, + "source": "192.168.1.0/24", + "type": "in" + }, + { + "action": "ACCEPT", + "dest": "+sdn/test2-gateway", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "enable": 1, + "iface": "test2", + "log": "nolog", + "macro": "DNS", + "pos": 4, + "type": "in" + }, + { + "action": "ACCEPT", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "enable": 1, + "iface": "test2", + "log": "nolog", + "macro": "DHCPfwd", + "pos": 5, + "type": "in" + }, + { + "action": "ACCEPT", + "dest": "+sdn/test2-all", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "dport": "68", + "enable": 1, + "log": "nolog", + "pos": 6, + "proto": "udp", + "source": "+sdn/test2-gateway", + "sport": "67", + "type": "out" + }, + { + "action": "DROP", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "enable": 1, + "log": "nolog", + "pos": 7, + "type": "in" + }, + { + "action": "DROP", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "enable": 1, + "log": "nolog", + "pos": 8, + "type": "out" + } + ] +""" from ansible.module_utils.basic import AnsibleModule from ansible_collections.community.proxmox.plugins.module_utils.proxmox import ( @@ -90,13 +455,12 @@ def validate_params(self): else: return True - def run(self): if not self.validate_params(): self.module.fail_json( - msg=f'parameter validation failed. ' - f'If state is present/update we need either group_conf to be True or rules to be present. ' - f'If state is absent we need group_conf to be True or pos to be present. ' + msg='parameter validation failed. ' + 'If state is present/update we need either group_conf to be True or rules to be present. ' + 'If state is absent we need group_conf to be True or pos to be present. ' ) state = self.params.get("state") @@ -146,7 +510,7 @@ def run(self): else: rules = self.get_fw_rules(rules_obj, pos=self.params.get('pos')) self.module.exit_json( - changed=False, firewall_rules=rules, msg=f'successfully retrieved firewall rules' + changed=False, firewall_rules=rules, msg='successfully retrieved firewall rules' ) def create_group(self, group, comment=None): @@ -189,7 +553,7 @@ def delete_fw_rule(self, rules_obj, pos): rule_obj.delete(pos=pos, digest=digest) self.module.exit_json( - changed=True, msg=f'successfully deleted firewall rules' + changed=True, msg='successfully deleted firewall rules' ) except Exception as e: self.module.fail_json( @@ -203,17 +567,16 @@ def update_fw_rules(self, rules_obj, rules): del rule['icmp_type'] try: rule_obj = getattr(rules_obj(), str(rule['pos'])) - rule['digest'] = rule_obj.get().get('digest') # Avoids concurrent changes + rule['digest'] = rule_obj.get().get('digest') # Avoids concurrent changes rule_obj.put(**rule) except Exception as e: self.module.fail_json( msg=f'Failed to update firewall rule at pos {rule["pos"]}: {e}' ) - else: - self.module.exit_json( - changed=True, msg=f'successfully created firewall rules' - ) + self.module.exit_json( + changed=True, msg='successfully created firewall rules' + ) def create_fw_rules(self, rules_obj, rules): for rule in rules: @@ -228,10 +591,9 @@ def create_fw_rules(self, rules_obj, rules): self.module.fail_json( msg=f'Failed to create firewall rule {rule}: {e}' ) - else: - self.module.exit_json( - changed=True, msg=f'successfully created firewall rules' - ) + self.module.exit_json( + changed=True, msg='successfully created firewall rules' + ) def move_rule_to_correct_pos(self, rules_obj, rule): ################################################################################################## @@ -255,7 +617,7 @@ def move_rule_to_correct_pos(self, rules_obj, rule): f'provided - {rule} rule_at0 - {fw_rule_at0.get()}') break # No need to move this. Potentially the issue is resolved. else: - fw_rule_at0.put(moveto=pos+1) # `moveto` moves rule to one position before the value + fw_rule_at0.put(moveto=(pos + 1)) # moveto moves rule to one position before the value except Exception as e: self.module.fail_json( msg=f'Rule created but failed to move it to correct pos. {e}' From 43d18f5597df4ec713f116a89dd7c43087fef382 Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Mon, 8 Sep 2025 06:15:37 +0530 Subject: [PATCH 10/22] proxmox_firewall: Add force condition - state=present: + check if fw rules already exists and if needed update them instead of creating + check if group exists and if so don't do anything - state=update: + check if fw rules don't existsand if needed create them instead of updating - make rules.pos as required this is to handle above conditions - add method to get security groups and list them with firewall rules when state is not provided - add proxmox_firewall in meta/runtime.yml --- meta/runtime.yml | 1 + plugins/modules/proxmox_firewall.py | 117 ++++++++++++++++++++++++---- 2 files changed, 101 insertions(+), 17 deletions(-) diff --git a/meta/runtime.yml b/meta/runtime.yml index 23f05cbd..518e9eaf 100644 --- a/meta/runtime.yml +++ b/meta/runtime.yml @@ -35,3 +35,4 @@ action_groups: - proxmox_user - proxmox_user_info - proxmox_vm_info + - proxmox_firewall diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index fa14211f..768f6921 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -32,6 +32,12 @@ - present - update - absent + force: + description: + - If state is present and if 1 or more rule already exists at given pos force will update them + - If state is update and if 1 or more rule doesn't exist force will create + type: bool + default: false level: description: - Level at which the firewall rule applies. @@ -157,6 +163,7 @@ description: - Position of the rule in the list. type: int + required: true proto: description: - IP protocol. You can use protocol names ('tcp'/'udp') or simple numbers, as defined in '/etc/protocols'. @@ -276,6 +283,14 @@ sample: test +groups: + description: list of firewall security groups + returned: on success + type: list + elements: str + sample: + [ "test" ] + firewall_rules: description: List of firewall rules. returned: on success @@ -391,6 +406,7 @@ def get_proxmox_args(): return dict( state=dict(type="str", choices=["present", "absent", "update"], required=False), + force=dict(type="bool", default=False), level=dict(type="str", choices=["cluster", "node", "vm", "vnet", "group"], default="cluster", required=False), node=dict(type="str", required=False), vmid=dict(type="int", required=False), @@ -417,7 +433,7 @@ def get_proxmox_args(): choices=["emerg", "alert", "crit", "err", "warning", "notice", "info", "debug", "nolog"], required=False), macro=dict(type="str", required=False), - pos=dict(type="int", required=False), + pos=dict(type="int", required=True), proto=dict(type="str", required=False), source=dict(type="str", required=False), sport=dict(type="str", required=False) @@ -449,21 +465,29 @@ def __init__(self, module): def validate_params(self): if self.params.get('state') in ['present', 'update']: - return self.params.get('group_conf') or self.params.get('rules') + if ((self.params.get('group_conf') and self.params.get('rules') is None) or + (not self.params.get('group_conf') and self.params.get('rules') is not None)): + return True + else: + self.module.fail_json( + msg="When state is present either group_conf should be true or rules must be present but not both" + ) elif self.params.get('state') == 'absent': - return self.params.get('group_conf') or self.params.get('pos') + if ((self.params.get('group_conf') and self.params.get('pos') is None) or + (not self.params.get('group_conf') and self.params.get('pos') is not None)): + return True + else: + self.module.fail_json( + msg="When State is absent either group_conf should be true or pos must be present but not both" + ) else: return True def run(self): - if not self.validate_params(): - self.module.fail_json( - msg='parameter validation failed. ' - 'If state is present/update we need either group_conf to be True or rules to be present. ' - 'If state is absent we need group_conf to be True or pos to be present. ' - ) + self.validate_params() state = self.params.get("state") + force = self.params.get("force") level = self.params.get("level") rules = self.params.get("rules") @@ -496,12 +520,12 @@ def run(self): if self.params.get('group_conf'): self.create_group(group=self.params.get('group'), comment=self.params.get('comment')) if rules is not None: - self.create_fw_rules(rules_obj=rules_obj, rules=rules) + self.create_fw_rules(rules_obj=rules_obj, rules=rules, force=force) elif state == "update": if self.params.get('group_conf'): self.create_group(group=self.params.get('group'), comment=self.params.get('comment')) if rules is not None: - self.update_fw_rules(rules_obj=rules_obj, rules=rules) + self.update_fw_rules(rules_obj=rules_obj, rules=rules, force=force) elif state == "absent": if self.params.get('pos'): self.delete_fw_rule(rules_obj=rules_obj, pos=self.params.get('pos')) @@ -509,11 +533,19 @@ def run(self): self.delete_group(group_name=self.params.get('group')) else: rules = self.get_fw_rules(rules_obj, pos=self.params.get('pos')) + groups = self.get_groups() self.module.exit_json( - changed=False, firewall_rules=rules, msg='successfully retrieved firewall rules' + changed=False, + firewall_rules=rules, + groups=groups, + msg='successfully retrieved firewall rules and groups' ) def create_group(self, group, comment=None): + if group in self.get_groups(): + self.module.exit_json( + changed=False, group=group, msg=f"security group {group} already exists" + ) try: self.proxmox_api.cluster().firewall().groups.post(group=group, comment=comment) self.module.exit_json( @@ -525,6 +557,10 @@ def create_group(self, group, comment=None): ) def delete_group(self, group_name): + if group_name not in self.get_groups(): + self.module.exit_json( + changed=False, group=group_name, msg=f"security group {group_name} already doesn't exists" + ) try: group = getattr(self.proxmox_api.cluster().firewall().groups(), group_name) group.delete() @@ -546,8 +582,23 @@ def get_fw_rules(self, rules_obj, pos=None): msg=f'Failed to retrieve firewall rules: {e}' ) + def get_groups(self): + try: + return [x['group'] for x in self.proxmox_api.cluster().firewall().groups().get()] + except Exception as e: + self.module.fail_json( + msg=f'Failed to retrieve firewall security groups: {e}' + ) + def delete_fw_rule(self, rules_obj, pos): try: + for item in self.get_fw_rules(rules_obj): + if item.get('pos') == pos: + break + else: + self.module.exit_json( + changed=False, msg="Firewall rule already doesn't exist" + ) rule_obj = getattr(rules_obj(), str(pos)) digest = rule_obj.get().get('digest') rule_obj.delete(pos=pos, digest=digest) @@ -560,8 +611,21 @@ def delete_fw_rule(self, rules_obj, pos): msg=f'Failed to delete firewall rule at pos {pos}: {e}' ) - def update_fw_rules(self, rules_obj, rules): - for rule in rules: + def update_fw_rules(self, rules_obj, rules, force): + existing_rules = self.get_fw_rules(rules_obj) + rules_to_create = [] + if len(existing_rules) > 0: + existing_pos = [x['pos'] for x in existing_rules] + for rule in rules: + if rule.get('pos') not in existing_pos: + if force: + rules_to_create.append(rule) + else: + self.module.fail_json( + msg=f"Rule doesn't exists at pos - {rule.get('pos')} and force is false." + ) + rules_to_update = [rule for rule in rules if rule not in rules_to_create] + for rule in rules_to_update: rule['icmp-type'] = rule.get('icmp_type') rule['enable'] = ansible_to_proxmox_bool(rule.get('enable')) del rule['icmp_type'] @@ -574,12 +638,29 @@ def update_fw_rules(self, rules_obj, rules): self.module.fail_json( msg=f'Failed to update firewall rule at pos {rule["pos"]}: {e}' ) + + if len(rules_to_create) > 0: + self.create_fw_rules(rules_obj=rules_obj, rules=rules_to_create, force=False) self.module.exit_json( - changed=True, msg='successfully created firewall rules' + changed=True, msg='successfully updated firewall rules' ) - def create_fw_rules(self, rules_obj, rules): - for rule in rules: + def create_fw_rules(self, rules_obj, rules, force): + existing_rules = self.get_fw_rules(rules_obj) + rules_to_update = [] + if len(existing_rules) > 0: + existing_pos = [x['pos'] for x in existing_rules] + for rule in rules: + if rule.get('pos') in existing_pos: + if force: + rules_to_update.append(rule) + else: + self.module.fail_json( + msg=f"Rule already exists at pos - {rule.get('pos')} and force is false." + ) + rules_to_create = [rule for rule in rules if rule not in rules_to_update] + + for rule in rules_to_create: rule['icmp-type'] = rule.get('icmp_type') rule['enable'] = ansible_to_proxmox_bool(rule.get('enable')) del rule['icmp_type'] @@ -591,6 +672,8 @@ def create_fw_rules(self, rules_obj, rules): self.module.fail_json( msg=f'Failed to create firewall rule {rule}: {e}' ) + if len(rules_to_update) > 0: + self.update_fw_rules(rules_obj=rules_obj, rules=rules_to_update, force=False) self.module.exit_json( changed=True, msg='successfully created firewall rules' ) From 6327901ac9c5db3eaadfbeabe9f95355c5f51abb Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Tue, 9 Sep 2025 21:27:42 +0530 Subject: [PATCH 11/22] proxmox_firewall: Add unit tests --- .../plugins/modules/test_proxmox_firewall.py | 197 ++++++++++++++++++ 1 file changed, 197 insertions(+) create mode 100644 tests/unit/plugins/modules/test_proxmox_firewall.py diff --git a/tests/unit/plugins/modules/test_proxmox_firewall.py b/tests/unit/plugins/modules/test_proxmox_firewall.py new file mode 100644 index 00000000..e5b50f3f --- /dev/null +++ b/tests/unit/plugins/modules/test_proxmox_firewall.py @@ -0,0 +1,197 @@ +# -*- coding: utf-8 -*- +# +# Copyright (c) 2025, Jana Hoch +# 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 + +from __future__ import absolute_import, division, print_function + +__metaclass__ = type + +from unittest.mock import patch + +import pytest + +proxmoxer = pytest.importorskip("proxmoxer") + +from ansible.module_utils import basic +from ansible_collections.community.proxmox.plugins.modules import proxmox_firewall +from ansible_collections.community.internal_test_tools.tests.unit.plugins.modules.utils import ( + ModuleTestCase, + set_module_args, +) +import ansible_collections.community.proxmox.plugins.module_utils.proxmox as proxmox_utils + +RAW_FIREWALL_RULES = [ + { + "ipversion": 4, + "digest": "245f9fb31d5f59543dedc5a84ba7cd6afa4dbcc0", + "log": "nolog", + "action": "ACCEPT", + "enable": 1, + "type": "out", + "source": "1.1.1.1", + "pos": 0 + }, + { + "enable": 1, + "pos": 1, + "source": "1.0.0.1", + "type": "out", + "action": "ACCEPT", + "digest": "245f9fb31d5f59543dedc5a84ba7cd6afa4dbcc0", + "ipversion": 4 + } +] + +RAW_GROUPS = [ + { + "digest": "fdb62dec01018d4f35c83ecc2ae3f110a8b3bd62", + "group": "test1" + }, + { + "group": "test2", + "digest": "fdb62dec01018d4f35c83ecc2ae3f110a8b3bd62" + } +] + + +def exit_json(*args, **kwargs): + """function to patch over exit_json; package return data into an exception""" + if 'changed' not in kwargs: + kwargs['changed'] = False + raise SystemExit(kwargs) + + +def fail_json(*args, **kwargs): + """function to patch over fail_json; package return data into an exception""" + kwargs['failed'] = True + raise SystemExit(kwargs) + + +def get_module_args_state_none(level="cluster", vmid=None, node=None, vnet=None, group=None): + return { + "api_host": "host", + "api_user": "user", + "api_password": "password", + "level": level, + "vmid": vmid, + "node": node, + "vnet": vnet, + "group": group + } + + +def get_module_args_group_conf(group, level="cluster", state="present"): + return { + "api_host": "host", + "api_user": "user", + "api_password": "password", + "level": level, + "group": group, + "group_conf": True, + "state": state + } + + +def get_module_args_rules(state, pos=1, level='cluster', source_ip='1.1.1.1'): + return { + "api_host": "host", + "api_user": "user", + "api_password": "password", + "level": level, + "state": state, + 'rules': [ + { + 'type': 'out', + 'action': 'ACCEPT', + 'source': source_ip, + 'pos': pos, + 'enable': True + } + ] + } + + +def get_module_args_fw_delete(pos, level='cluster', state='absent'): + return { + "api_host": "host", + "api_user": "user", + "api_password": "password", + "level": level, + "state": state, + "pos": pos + } + + +class TestProxmoxFirewallModule(ModuleTestCase): + def setUp(self): + super(TestProxmoxFirewallModule, self).setUp() + proxmox_utils.HAS_PROXMOXER = True + self.module = proxmox_firewall + self.mock_module_helper = patch.multiple(basic.AnsibleModule, + exit_json=exit_json, + fail_json=fail_json) + self.mock_module_helper.start() + 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.groups.return_value.get.return_value = RAW_GROUPS + + def tearDown(self): + self.connect_mock.stop() + self.mock_module_helper.stop() + super(TestProxmoxFirewallModule, self).tearDown() + + def test_get_fw_state_none(self): + with pytest.raises(SystemExit) as exc_info: + with set_module_args(get_module_args_state_none()): + self.module.main() + result = exc_info.value.args[0] + assert result["changed"] is False + assert result["msg"] == "successfully retrieved firewall rules and groups" + assert result["firewall_rules"] == RAW_FIREWALL_RULES + assert result["groups"] == ['test1', 'test2'] + + def test_create_group(self): + with pytest.raises(SystemExit) as exc_info: + with set_module_args(get_module_args_group_conf(group='test')): + self.module.main() + result = exc_info.value.args[0] + assert result['changed'] is True + assert result["msg"] == 'successfully created security group test' + assert result['group'] == 'test' + + def test_delete_group(self): + with pytest.raises(SystemExit) as exc_info: + with set_module_args(get_module_args_group_conf(group='test1', state="absent")): + self.module.main() + result = exc_info.value.args[0] + assert result['changed'] is True + assert result["msg"] == 'successfully deleted security group test1' + assert result['group'] == 'test1' + + def test_update_fw_rules(self): + with pytest.raises(SystemExit) as exc_info: + with set_module_args(get_module_args_rules(state='update')): + self.module.main() + result = exc_info.value.args[0] + assert result['changed'] is True + assert result["msg"] == 'successfully updated firewall rules' + + def test_create_fw_rules(self): + with pytest.raises(SystemExit) as exc_info: + with set_module_args(get_module_args_rules(state='present', pos=2)): + self.module.main() + result = exc_info.value.args[0] + assert result['changed'] is True + assert result["msg"] == 'successfully created firewall rules' + + 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)): + self.module.main() + result = exc_info.value.args[0] + assert result['changed'] is True + assert result["msg"] == 'successfully deleted firewall rules' From 4b14774fdc49636d752b4b0eefac03a5db660c8c Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sat, 13 Sep 2025 13:42:23 +0530 Subject: [PATCH 12/22] proxmox_firewall: simplify conditions --- plugins/modules/proxmox_firewall.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index 768f6921..23c21e29 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -465,16 +465,14 @@ def __init__(self, module): def validate_params(self): if self.params.get('state') in ['present', 'update']: - if ((self.params.get('group_conf') and self.params.get('rules') is None) or - (not self.params.get('group_conf') and self.params.get('rules') is not None)): + if self.params.get('group_conf') != bool(self.params.get('rules')): return True else: self.module.fail_json( msg="When state is present either group_conf should be true or rules must be present but not both" ) elif self.params.get('state') == 'absent': - if ((self.params.get('group_conf') and self.params.get('pos') is None) or - (not self.params.get('group_conf') and self.params.get('pos') is not None)): + if self.params.get('group_conf') != bool(self.params.get('pos')): return True else: self.module.fail_json( @@ -490,6 +488,8 @@ def run(self): force = self.params.get("force") level = self.params.get("level") rules = self.params.get("rules") + group = self.params.get("group") + group_conf = self.params.get("group_conf") if level == "vm": vm = self.get_vm(vmid=self.params.get('vmid')) @@ -510,27 +510,27 @@ def run(self): rules_obj = firewall_obj().rules elif level == "group": - rules_obj = getattr(self.proxmox_api.cluster().firewall().groups(), self.params.get('group')) + rules_obj = getattr(self.proxmox_api.cluster().firewall().groups(), group) else: firewall_obj = self.proxmox_api.cluster().firewall rules_obj = firewall_obj().rules if state == "present": - if self.params.get('group_conf'): - self.create_group(group=self.params.get('group'), comment=self.params.get('comment')) + if group_conf: + self.create_group(group=group, comment=self.params.get('comment')) if rules is not None: self.create_fw_rules(rules_obj=rules_obj, rules=rules, force=force) elif state == "update": - if self.params.get('group_conf'): - self.create_group(group=self.params.get('group'), comment=self.params.get('comment')) + if group_conf: + self.create_group(group=group, comment=self.params.get('comment')) if rules is not None: self.update_fw_rules(rules_obj=rules_obj, rules=rules, force=force) elif state == "absent": if self.params.get('pos'): self.delete_fw_rule(rules_obj=rules_obj, pos=self.params.get('pos')) - if self.params.get('group_conf'): - self.delete_group(group_name=self.params.get('group')) + if group_conf: + self.delete_group(group_name=group) else: rules = self.get_fw_rules(rules_obj, pos=self.params.get('pos')) groups = self.get_groups() From a7cb2dfe4099bd4043b661b3acebbbac0ab99298 Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sat, 13 Sep 2025 20:37:51 +0530 Subject: [PATCH 13/22] proxmox_firewall: Improve checks - Earlier it was only checking if rule at pos already exists - If it did it would update it given force was true. - But it means if we ran same pipeline twice without force it would fail - To fix it Checking the entier rule --- plugins/modules/proxmox_firewall.py | 97 +++++++++++++++++++---------- 1 file changed, 65 insertions(+), 32 deletions(-) diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index 23c21e29..7c843fe5 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -491,6 +491,11 @@ def run(self): group = self.params.get("group") group_conf = self.params.get("group_conf") + for rule in rules: + rule['icmp-type'] = rule.get('icmp_type') + rule['enable'] = ansible_to_proxmox_bool(rule.get('enable')) + del rule['icmp_type'] + if level == "vm": vm = self.get_vm(vmid=self.params.get('vmid')) node = getattr(self.proxmox_api.nodes(), vm['node']) @@ -611,24 +616,56 @@ def delete_fw_rule(self, rules_obj, pos): msg=f'Failed to delete firewall rule at pos {pos}: {e}' ) + def check_rules(self, existing_rules, new_rules): + rules_to_update = [] + new_rules = [{k: v for k, v in item.items() if v is not None} for item in new_rules] + + if existing_rules is None: + rules_to_create = new_rules + rules_to_update = list() + return rules_to_create, rules_to_update + + existing_rules = {x['pos']: x for x in existing_rules} + new_rules = {x['pos']: x for x in new_rules} + + common_pos = set(existing_rules.keys()).intersection(set(new_rules.keys())) + pos_to_create = set(new_rules.keys()) - set(existing_rules.keys()) + rules_to_create = [new_rules[pos] for pos in pos_to_create] + + params_to_ignore = ['digest', 'ipversion'] + + for pos in common_pos: + # If new rule has a parameter that is not present in existing rule we need to update + if set(new_rules[pos].keys()) - set(existing_rules[pos].keys()) != set(): + rules_to_update.append(new_rules[pos]) + 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_rules[pos].items(): + if (existing_rule_param not in params_to_ignore and + new_rules[pos].get(existing_rule_param) != existing_parm_value): + rules_to_update.append(new_rules[pos]) + + return rules_to_create, rules_to_update + def update_fw_rules(self, rules_obj, rules, force): existing_rules = self.get_fw_rules(rules_obj) - rules_to_create = [] - if len(existing_rules) > 0: - existing_pos = [x['pos'] for x in existing_rules] - for rule in rules: - if rule.get('pos') not in existing_pos: - if force: - rules_to_create.append(rule) - else: - self.module.fail_json( - msg=f"Rule doesn't exists at pos - {rule.get('pos')} and force is false." - ) - rules_to_update = [rule for rule in rules if rule not in rules_to_create] + rules_to_create, rules_to_update = self.check_rules(existing_rules=existing_rules, new_rules=rules) + + if len(rules_to_update) == 0: + if len(rules_to_create) == 0: + self.module.exit_json( + changed=False, + msg='No need to update any FW rules.' + + ) + elif len(rules_to_create) > 0 and not force: + self.module.fail_json( + msg=f"Need to create new rules for pos - {[x['pos'] for x in rules_to_create]} But force is false" + ) + for rule in rules_to_update: - rule['icmp-type'] = rule.get('icmp_type') - rule['enable'] = ansible_to_proxmox_bool(rule.get('enable')) - del rule['icmp_type'] try: rule_obj = getattr(rules_obj(), str(rule['pos'])) rule['digest'] = rule_obj.get().get('digest') # Avoids concurrent changes @@ -646,24 +683,20 @@ def update_fw_rules(self, rules_obj, rules, force): ) def create_fw_rules(self, rules_obj, rules, force): - existing_rules = self.get_fw_rules(rules_obj) - rules_to_update = [] - if len(existing_rules) > 0: - existing_pos = [x['pos'] for x in existing_rules] - for rule in rules: - if rule.get('pos') in existing_pos: - if force: - rules_to_update.append(rule) - else: - self.module.fail_json( - msg=f"Rule already exists at pos - {rule.get('pos')} and force is false." - ) - rules_to_create = [rule for rule in rules if rule not in rules_to_update] + existing_rules = self.get_fw_rules(rules_obj=rules_obj) + rules_to_create, rules_to_update = self.check_rules(existing_rules=existing_rules, new_rules=rules) + + if len(rules_to_create) == 0 and len(rules_to_update) == 0: + self.module.exit_json( + changed=False, + msg='No need to create/update any rule' + ) + elif len(rules_to_update) > 0 and not force: + self.module.fail_json( + msg=f"Need to update rules at pos - {[x['pos'] for x in rules_to_update]} but force is false" + ) for rule in rules_to_create: - rule['icmp-type'] = rule.get('icmp_type') - rule['enable'] = ansible_to_proxmox_bool(rule.get('enable')) - del rule['icmp_type'] try: rules_obj().post(**rule) self.move_rule_to_correct_pos(rules_obj, rule) @@ -672,7 +705,7 @@ def create_fw_rules(self, rules_obj, rules, force): self.module.fail_json( msg=f'Failed to create firewall rule {rule}: {e}' ) - if len(rules_to_update) > 0: + if len(rules_to_update) > 0 and force: self.update_fw_rules(rules_obj=rules_obj, rules=rules_to_update, force=False) self.module.exit_json( changed=True, msg='successfully created firewall rules' From 1adb2ca2864315e3b1f1e1bdb42555fc488e4e1a Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sat, 13 Sep 2025 21:52:10 +0530 Subject: [PATCH 14/22] proxmox_firewall & proxmox module_utils - Move check_rules() to proxmox module_utils and rename to compare_list_of_dicts() - Generalize the implemnetation as this is usefull in multiple places. - e.g. filtering out which fw rules, aliases, etc needs to be created/updated --- plugins/module_utils/proxmox.py | 47 ++++++++++++++++++ plugins/modules/proxmox_firewall.py | 76 ++++++++++------------------- 2 files changed, 74 insertions(+), 49 deletions(-) diff --git a/plugins/module_utils/proxmox.py b/plugins/module_utils/proxmox.py index 0455e793..d02a49f5 100644 --- a/plugins/module_utils/proxmox.py +++ b/plugins/module_utils/proxmox.py @@ -73,6 +73,53 @@ def ansible_to_proxmox_bool(value): return 1 if value else 0 +def compare_list_of_dicts(existing_list, new_list, uid, params_to_ignore=None): + """ Compare 2 list of dicts + Use case - for firewall rules we will be getting a list of rules from user. + We want to filter out which rules needs to be updated and which rules are completely new and needs to be created + + @param existing_list: Existing values example - list of existing rules + @param new_list: New values example - list of rules passed to module + @param uid: unique identifier in dict. It should always be present in both lists - in case of firewall rules it's pos + @param params_to_ignore: list of params we want to ignore which are present in existing_list's dict. + In case of firewall rules we want to ignore ['digest', 'ipversion'] + + @return: returns 2 list items 1st is the list of items which are completely new and needs to be created + 2nd is a list of items which needs to be updated + """ + if params_to_ignore is None: + params_to_ignore = list() + items_to_update = [] + new_list = [{k: v for k, v in item.items() if v is not None} for item in new_list] + + if existing_list is None: + items_to_create = new_list + items_to_update = list() + return items_to_create, items_to_update + + existing_list = {x[uid]: x for x in existing_list} + new_list = {x[uid]: x for x in new_list} + + 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] + + for id 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]) + 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(): + 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]) + + return items_to_create, items_to_update + + class ProxmoxAnsible(object): """Base class for Proxmox modules""" TASK_TIMED_OUT = 'timeout expired' diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index 7c843fe5..459e1145 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -399,6 +399,7 @@ from ansible_collections.community.proxmox.plugins.module_utils.proxmox import ( proxmox_auth_argument_spec, ansible_to_proxmox_bool, + compare_list_of_dicts, ProxmoxAnsible ) @@ -491,10 +492,11 @@ def run(self): group = self.params.get("group") group_conf = self.params.get("group_conf") - for rule in rules: - rule['icmp-type'] = rule.get('icmp_type') - rule['enable'] = ansible_to_proxmox_bool(rule.get('enable')) - del rule['icmp_type'] + if rules is not None: + for rule in rules: + rule['icmp-type'] = rule.get('icmp_type') + rule['enable'] = ansible_to_proxmox_bool(rule.get('enable')) + del rule['icmp_type'] if level == "vm": vm = self.get_vm(vmid=self.params.get('vmid')) @@ -616,54 +618,25 @@ def delete_fw_rule(self, rules_obj, pos): msg=f'Failed to delete firewall rule at pos {pos}: {e}' ) - def check_rules(self, existing_rules, new_rules): - rules_to_update = [] - new_rules = [{k: v for k, v in item.items() if v is not None} for item in new_rules] - - if existing_rules is None: - rules_to_create = new_rules - rules_to_update = list() - return rules_to_create, rules_to_update - - existing_rules = {x['pos']: x for x in existing_rules} - new_rules = {x['pos']: x for x in new_rules} - - common_pos = set(existing_rules.keys()).intersection(set(new_rules.keys())) - pos_to_create = set(new_rules.keys()) - set(existing_rules.keys()) - rules_to_create = [new_rules[pos] for pos in pos_to_create] - - params_to_ignore = ['digest', 'ipversion'] - - for pos in common_pos: - # If new rule has a parameter that is not present in existing rule we need to update - if set(new_rules[pos].keys()) - set(existing_rules[pos].keys()) != set(): - rules_to_update.append(new_rules[pos]) - 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_rules[pos].items(): - if (existing_rule_param not in params_to_ignore and - new_rules[pos].get(existing_rule_param) != existing_parm_value): - rules_to_update.append(new_rules[pos]) - - return rules_to_create, rules_to_update - def update_fw_rules(self, rules_obj, rules, force): existing_rules = self.get_fw_rules(rules_obj) - rules_to_create, rules_to_update = self.check_rules(existing_rules=existing_rules, new_rules=rules) + rules_to_create, rules_to_update = compare_list_of_dicts( + existing_list=existing_rules, + new_list=rules, + uid='pos', + params_to_ignore=['digest', 'ipversion'] + ) - if len(rules_to_update) == 0: - if len(rules_to_create) == 0: - self.module.exit_json( - changed=False, - msg='No need to update any FW rules.' + if len(rules_to_update) == 0 and len(rules_to_create) == 0: + self.module.exit_json( + changed=False, + msg='No need to update any FW rules.' - ) - elif len(rules_to_create) > 0 and not force: - self.module.fail_json( - msg=f"Need to create new rules for pos - {[x['pos'] for x in rules_to_create]} But force is false" - ) + ) + elif len(rules_to_create) > 0 and not force: + self.module.fail_json( + msg=f"Need to create new rules for pos - {[x['pos'] for x in rules_to_create]} But force is false" + ) for rule in rules_to_update: try: @@ -684,7 +657,12 @@ def update_fw_rules(self, rules_obj, rules, force): def create_fw_rules(self, rules_obj, rules, force): existing_rules = self.get_fw_rules(rules_obj=rules_obj) - rules_to_create, rules_to_update = self.check_rules(existing_rules=existing_rules, new_rules=rules) + rules_to_create, rules_to_update = compare_list_of_dicts( + existing_list=existing_rules, + new_list=rules, + uid='pos', + params_to_ignore=['digest', 'ipversion'] + ) if len(rules_to_create) == 0 and len(rules_to_update) == 0: self.module.exit_json( From 11a3a19fd682108c3bc15bea6fa8f2915c2fb86b Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sun, 14 Sep 2025 00:19:44 +0530 Subject: [PATCH 15/22] proxmox_firewall: Add methods to create/update/delete aliases --- plugins/modules/proxmox_firewall.py | 227 +++++++++++++++++++++++++++- 1 file changed, 223 insertions(+), 4 deletions(-) diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index 459e1145..50e65e67 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -84,6 +84,27 @@ - Comment for security group. - Only needed when creating group. type: str + aliases: + description: + - List of aliases + - Alias can only be created/updated/deleted at cluster or VM level + type: list + elements: dict + suboptions: + name: + description: Alias name + type: str + required: true + cidr: + description: + - CIDR for alias + - only needed when O(state=present) or O(state=update) + type: str + required: false + comment: + description: Comment for Alias + type: str + required: false rules: description: - List of individual rules to be applied. @@ -273,6 +294,46 @@ group_conf: True state: absent group: test + +- name: Create FW aliases + community.proxmox.proxmox_firewall: + api_user: "{{ pc.proxmox.api_user }}" + api_token_id: "{{ pc.proxmox.api_token_id }}" + api_token_secret: "{{ vault.proxmox.api_token_secret }}" + api_host: "{{ pc.proxmox.api_host }}" + validate_certs: no + state: present + aliases: + - name: test1 + cidr: '10.10.1.0/24' + - name: test2 + cidr: '10.10.2.0/24' + +- name: Update FW aliases + community.proxmox.proxmox_firewall: + api_user: "{{ pc.proxmox.api_user }}" + api_token_id: "{{ pc.proxmox.api_token_id }}" + api_token_secret: "{{ vault.proxmox.api_token_secret }}" + api_host: "{{ pc.proxmox.api_host }}" + validate_certs: no + state: update + aliases: + - name: test1 + cidr: '10.10.1.0/28' + - name: test2 + cidr: '10.10.2.0/28' + +- name: Delete FW aliases + community.proxmox.proxmox_firewall: + api_user: "{{ pc.proxmox.api_user }}" + api_token_id: "{{ pc.proxmox.api_token_id }}" + api_token_secret: "{{ vault.proxmox.api_token_secret }}" + api_host: "{{ pc.proxmox.api_host }}" + validate_certs: no + state: absent + aliases: + - name: test1 + - name: test2 """ RETURN = r""" @@ -291,6 +352,35 @@ sample: [ "test" ] +aliases: + description: + - list of alias present at given level + - aliases are only available for cluster and VM level so if any other level it'll be empty list + returned: on success + type: list + elements: dict + sample: + [ + { + "cidr": "10.10.1.0/24", + "digest": "978391f460484e8d4fb3ca785cfe5a9d16fe8b1f", + "ipversion": 4, + "name": "test1" + }, + { + "cidr": "10.10.2.0/24", + "digest": "978391f460484e8d4fb3ca785cfe5a9d16fe8b1f", + "ipversion": 4, + "name": "test2" + }, + { + "cidr": "10.10.3.0/24", + "digest": "978391f460484e8d4fb3ca785cfe5a9d16fe8b1f", + "ipversion": 4, + "name": "test3" + } + ] + firewall_rules: description: List of firewall rules. returned: on success @@ -416,6 +506,16 @@ def get_proxmox_args(): group_conf=dict(type="bool", default=False), group=dict(type="str", required=False), comment=dict(type="str", required=False), + aliases=dict( + type="list", + elements="dict", + required=False, + options=dict( + name=dict(type="str", required=True), + cidr=dict(type="str", required=False), + comment=dict(type="str", required=False) + ) + ), rules=dict( type="list", elements="dict", @@ -455,6 +555,9 @@ def get_ansible_module(): ('level', 'node', ['node']), ('level', 'vnet', ['vnet']), ('level', 'group', ['group']), + ], + mutually_exclusive=[ + ('aliases', 'rules'), ] ) @@ -466,18 +569,18 @@ def __init__(self, module): def validate_params(self): if self.params.get('state') in ['present', 'update']: - if self.params.get('group_conf') != bool(self.params.get('rules')): + if self.params.get('group_conf') != bool(self.params.get('rules') or self.params.get('aliases')): return True else: self.module.fail_json( - msg="When state is present either group_conf should be true or rules must be present but not both" + 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')): + if self.params.get('group_conf') != bool(self.params.get('pos') or self.params.get('aliases')): return True else: self.module.fail_json( - msg="When State is absent either group_conf should be true or pos must be present but not both" + msg="When State is absent either group_conf should be true or pos/aliases must be present but not both" ) else: return True @@ -488,6 +591,7 @@ def run(self): state = self.params.get("state") force = self.params.get("force") level = self.params.get("level") + aliases = self.params.get("aliases") rules = self.params.get("rules") group = self.params.get("group") group_conf = self.params.get("group_conf") @@ -517,6 +621,7 @@ def run(self): rules_obj = firewall_obj().rules elif level == "group": + firewall_obj = None rules_obj = getattr(self.proxmox_api.cluster().firewall().groups(), group) else: @@ -528,26 +633,140 @@ def run(self): self.create_group(group=group, comment=self.params.get('comment')) if rules is not None: self.create_fw_rules(rules_obj=rules_obj, rules=rules, force=force) + if aliases is not None: + self.create_aliases(firewall_obj=firewall_obj, level=level, aliases=aliases, force=force) elif state == "update": if group_conf: self.create_group(group=group, comment=self.params.get('comment')) if rules is not None: self.update_fw_rules(rules_obj=rules_obj, rules=rules, force=force) + if aliases is not None: + self.update_aliases(firewall_obj=firewall_obj, level=level, aliases=aliases, force=force) elif state == "absent": if self.params.get('pos'): self.delete_fw_rule(rules_obj=rules_obj, pos=self.params.get('pos')) if group_conf: self.delete_group(group_name=group) + if aliases is not None: + self.delete_aliases(firewall_obj=firewall_obj, level=level, aliases=aliases) else: rules = self.get_fw_rules(rules_obj, pos=self.params.get('pos')) groups = self.get_groups() + aliases = self.get_aliases(firewall_obj=firewall_obj, level=level) self.module.exit_json( changed=False, firewall_rules=rules, groups=groups, + aliases=aliases, msg='successfully retrieved firewall rules and groups' ) + def get_aliases(self, firewall_obj, level): + if firewall_obj is None or level not in ['cluster', 'vm']: + return list() + try: + return firewall_obj().aliases().get() + except Exception as e: + self.module.fail_json( + msg='Failed to retrieve aliases' + ) + + def create_aliases(self, firewall_obj, level, aliases, force=False): + if firewall_obj is None or level not in ['cluster', 'vm']: + self.module.fail_json( + msg='Aliases can only be created at cluster or VM level' + ) + + aliases_to_create, aliases_to_update = compare_list_of_dicts( + existing_list=self.get_aliases(firewall_obj=firewall_obj, level=level), + new_list=aliases, + uid='name', + params_to_ignore=['digest', 'ipversion'] + ) + + if len(aliases_to_create) == 0 and len(aliases_to_update) == 0: + self.module.exit_json( + changed=False, + msg='No need to create/update any aliases' + ) + elif len(aliases_to_update) > 0 and not force: + self.module.fail_json( + msg=f"Need to update aliases - {[x['name'] for x in aliases_to_update]} but force is false" + ) + + for alias in aliases_to_create: + try: + firewall_obj().aliases().post(**alias) + except Exception as e: + self.module.fail_json( + msg=f"Failed to create Alias {alias['name']} - {e}" + ) + if len(aliases_to_update) > 0 and force: + self.update_aliases(firewall_obj=firewall_obj, level=level, aliases=aliases_to_update, force=False) + else: + self.module.exit_json( + changed=True, + msg="Aliases created" + ) + + def update_aliases(self, firewall_obj, level, aliases, force=False): + aliases_to_create, aliases_to_update = compare_list_of_dicts( + existing_list=self.get_aliases(firewall_obj=firewall_obj, level=level), + new_list=aliases, + uid='name', + params_to_ignore=['digest', 'ipversion'] + ) + + if len(aliases_to_update) == 0 and len(aliases_to_create) == 0: + self.module.exit_json( + changed=False, + msg='No need to create/update any alias.' + + ) + elif len(aliases_to_create) > 0 and not force: + self.module.fail_json( + msg=f"Need to create new alias - {[x['name'] for x in aliases_to_create]} But force is false" + ) + + for alias in aliases_to_update: + try: + alias_obj = getattr(firewall_obj().aliases(), alias['name']) + alias_obj().put(**alias) + except Exception as e: + self.module.fail_json( + msg=f"Failed to update Alias {alias['name']} - {e}" + ) + if len(aliases_to_update) > 0 and force: + self.update_aliases(firewall_obj=firewall_obj, level=level, aliases=aliases_to_update, force=False) + else: + self.module.exit_json( + changed=True, + msg="Aliases updated" + ) + + def delete_aliases(self, firewall_obj, level, aliases): + existing_aliases = set([x.get('name') for x in self.get_aliases(firewall_obj=firewall_obj, level=level)]) + aliases = set([x.get('name') for x in aliases]) + aliases_to_delete = list(existing_aliases.intersection(aliases)) + + if len(aliases_to_delete) == 0: + self.module.exit_json( + changed=False, + msg="No need to delete any alias" + ) + for alias_name in aliases_to_delete: + try: + alias_obj = getattr(firewall_obj().aliases(), alias_name) + alias_obj().delete() + except Exception as e: + self.module.fail_json( + msg=f"Failed to delete alias {alias_name} - {e}" + ) + self.module.exit_json( + changed=True, + msg="Successfully deleted aliases" + ) + def create_group(self, group, comment=None): if group in self.get_groups(): self.module.exit_json( From a5419bd773e50d4931bfe95d5c28707b4f73ad68 Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sun, 21 Sep 2025 10:47:42 +0530 Subject: [PATCH 16/22] proxmox_firewall: remove unnecsary getattr --- plugins/modules/proxmox_firewall.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index 50e65e67..046a11d3 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -604,25 +604,22 @@ def run(self): if level == "vm": vm = self.get_vm(vmid=self.params.get('vmid')) - node = getattr(self.proxmox_api.nodes(), vm['node']) - virt = getattr(node(), vm['type']) - vm = getattr(virt(), vm['vmid']) - firewall_obj = vm().firewall + node = self.proxmox_api.nodes(vm['node']) + virt = node(vm['type']) + firewall_obj = virt(str(vm['vmid'])).firewall rules_obj = firewall_obj().rules elif level == "node": - node = getattr(self.proxmox_api.nodes(), self.params.get('node')) - firewall_obj = node().firewall + firewall_obj = self.proxmox_api.nodes(self.params.get('node')).firewall rules_obj = firewall_obj().rules elif level == "vnet": - vnet = getattr(self.proxmox_api.cluster().sdn().vnets(), self.params.get('vnet')) - firewall_obj = vnet().firewall + firewall_obj = self.proxmox_api.cluster().sdn().vnets(self.params.get('vnet')).firewall rules_obj = firewall_obj().rules elif level == "group": firewall_obj = None - rules_obj = getattr(self.proxmox_api.cluster().firewall().groups(), group) + rules_obj = self.proxmox_api.cluster().firewall().groups(group) else: firewall_obj = self.proxmox_api.cluster().firewall From 0741d0cfc9b782bfe855a9c6c502b6046995aa7e Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sun, 21 Sep 2025 10:56:04 +0530 Subject: [PATCH 17/22] proxmox_firewall: Split into seprate proxmox_firewall_info - Also add get methods in module_utils. --- plugins/module_utils/proxmox_sdn.py | 55 ++++ plugins/modules/proxmox_firewall.py | 193 +------------- plugins/modules/proxmox_firewall_info.py | 304 +++++++++++++++++++++++ 3 files changed, 362 insertions(+), 190 deletions(-) create mode 100644 plugins/modules/proxmox_firewall_info.py diff --git a/plugins/module_utils/proxmox_sdn.py b/plugins/module_utils/proxmox_sdn.py index 3ed65854..99c408e8 100644 --- a/plugins/module_utils/proxmox_sdn.py +++ b/plugins/module_utils/proxmox_sdn.py @@ -8,6 +8,17 @@ __metaclass__ = type +import traceback + +PROXMOXER_IMP_ERR = None +try: + from proxmoxer import ProxmoxResource + from proxmoxer import __version__ as proxmoxer_version + HAS_PROXMOXER = True +except ImportError: + HAS_PROXMOXER = False + PROXMOXER_IMP_ERR = traceback.format_exc() + from typing import List, Dict from ansible_collections.community.proxmox.plugins.module_utils.proxmox import ( @@ -99,3 +110,47 @@ def get_zones(self, zone_type: str = None) -> List[Dict]: self.module.fail_json( msg=f'Failed to retrieve zone information from cluster: {e}' ) + + def get_aliases(self, firewall_obj: ProxmoxResource | None) -> List[Dict]: + """Get aliases for IP/CIDR at given firewall endpoint level + + :param firewall_obj: Firewall endpoint as a ProxmoxResource e.g. self.proxmox_api.cluster().firewall + If it is None it'll return empty list + :return: List of aliases and corresponding IP/CIDR + """ + if firewall_obj is None: + return list() + try: + return firewall_obj().aliases().get() + except Exception as e: + self.module.fail_json( + msg='Failed to retrieve aliases' + ) + + def get_fw_rules(self, rules_obj: ProxmoxResource, pos: int = None) -> List[Dict]: + """Get firewall rules at given rules endpoint level + + :param rules_obj: Firewall Rules endpoint as a ProxmoxResource e.g. self.proxmox_api.cluster().firewall().rules + :param pos: Rule position if it is None it'll return all rules + :return: Firewall rules as a list of dict + """ + if pos is not None: + rules_obj = getattr(rules_obj(), str(pos)) + try: + return rules_obj.get() + except Exception as e: + self.module.fail_json( + msg=f'Failed to retrieve firewall rules: {e}' + ) + + def get_groups(self) -> List: + """Get firewall security groups + + :return: list of groups + """ + try: + return [x['group'] for x in self.proxmox_api.cluster().firewall().groups().get()] + except Exception as e: + self.module.fail_json( + msg=f'Failed to retrieve firewall security groups: {e}' + ) diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index 046a11d3..c4da1b30 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -209,15 +209,6 @@ """ EXAMPLES = r""" -- name: Get Cluster level firewall rules - community.proxmox.proxmox_firewall: - api_user: "{{ pc.proxmox.api_user }}" - api_token_id: "{{ pc.proxmox.api_token_id }}" - api_token_secret: "{{ vault.proxmox.api_token_secret }}" - api_host: "{{ pc.proxmox.api_host }}" - validate_certs: no - level: cluster - - name: Create firewall rules at cluster level community.proxmox.proxmox_firewall: api_user: "{{ pc.proxmox.api_user }}" @@ -343,154 +334,14 @@ type: str sample: test - -groups: - description: list of firewall security groups - returned: on success - type: list - elements: str - sample: - [ "test" ] - -aliases: - description: - - list of alias present at given level - - aliases are only available for cluster and VM level so if any other level it'll be empty list - returned: on success - type: list - elements: dict - sample: - [ - { - "cidr": "10.10.1.0/24", - "digest": "978391f460484e8d4fb3ca785cfe5a9d16fe8b1f", - "ipversion": 4, - "name": "test1" - }, - { - "cidr": "10.10.2.0/24", - "digest": "978391f460484e8d4fb3ca785cfe5a9d16fe8b1f", - "ipversion": 4, - "name": "test2" - }, - { - "cidr": "10.10.3.0/24", - "digest": "978391f460484e8d4fb3ca785cfe5a9d16fe8b1f", - "ipversion": 4, - "name": "test3" - } - ] - -firewall_rules: - description: List of firewall rules. - returned: on success - type: list - elements: dict - sample: - [ - { - "action": "ACCEPT", - "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", - "dport": "53", - "enable": 1, - "ipversion": 4, - "log": "nolog", - "pos": 0, - "proto": "udp", - "source": "192.168.1.0/24", - "type": "in" - }, - { - "action": "ACCEPT", - "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", - "dport": "53", - "enable": 1, - "ipversion": 4, - "log": "nolog", - "pos": 1, - "proto": "tcp", - "source": "192.168.1.0/24", - "type": "in" - }, - { - "action": "ACCEPT", - "dest": "192.168.1.0/24", - "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", - "enable": 1, - "ipversion": 4, - "log": "nolog", - "pos": 2, - "type": "out" - }, - { - "action": "ACCEPT", - "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", - "enable": 1, - "ipversion": 4, - "log": "nolog", - "pos": 3, - "source": "192.168.1.0/24", - "type": "in" - }, - { - "action": "ACCEPT", - "dest": "+sdn/test2-gateway", - "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", - "enable": 1, - "iface": "test2", - "log": "nolog", - "macro": "DNS", - "pos": 4, - "type": "in" - }, - { - "action": "ACCEPT", - "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", - "enable": 1, - "iface": "test2", - "log": "nolog", - "macro": "DHCPfwd", - "pos": 5, - "type": "in" - }, - { - "action": "ACCEPT", - "dest": "+sdn/test2-all", - "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", - "dport": "68", - "enable": 1, - "log": "nolog", - "pos": 6, - "proto": "udp", - "source": "+sdn/test2-gateway", - "sport": "67", - "type": "out" - }, - { - "action": "DROP", - "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", - "enable": 1, - "log": "nolog", - "pos": 7, - "type": "in" - }, - { - "action": "DROP", - "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", - "enable": 1, - "log": "nolog", - "pos": 8, - "type": "out" - } - ] """ from ansible.module_utils.basic import AnsibleModule +from ansible_collections.community.proxmox.plugins.module_utils.proxmox_sdn import ProxmoxSdnAnsible from ansible_collections.community.proxmox.plugins.module_utils.proxmox import ( proxmox_auth_argument_spec, ansible_to_proxmox_bool, - compare_list_of_dicts, - ProxmoxAnsible + compare_list_of_dicts ) @@ -562,7 +413,7 @@ def get_ansible_module(): ) -class ProxmoxFirewallAnsible(ProxmoxAnsible): +class ProxmoxFirewallAnsible(ProxmoxSdnAnsible): def __init__(self, module): super(ProxmoxFirewallAnsible, self).__init__(module) self.params = module.params @@ -646,27 +497,7 @@ def run(self): self.delete_group(group_name=group) if aliases is not None: self.delete_aliases(firewall_obj=firewall_obj, level=level, aliases=aliases) - else: - rules = self.get_fw_rules(rules_obj, pos=self.params.get('pos')) - groups = self.get_groups() - aliases = self.get_aliases(firewall_obj=firewall_obj, level=level) - self.module.exit_json( - changed=False, - firewall_rules=rules, - groups=groups, - aliases=aliases, - msg='successfully retrieved firewall rules and groups' - ) - def get_aliases(self, firewall_obj, level): - if firewall_obj is None or level not in ['cluster', 'vm']: - return list() - try: - return firewall_obj().aliases().get() - except Exception as e: - self.module.fail_json( - msg='Failed to retrieve aliases' - ) def create_aliases(self, firewall_obj, level, aliases, force=False): if firewall_obj is None or level not in ['cluster', 'vm']: @@ -795,24 +626,6 @@ def delete_group(self, group_name): msg=f'Failed to delete security group {group_name}: {e}' ) - def get_fw_rules(self, rules_obj, pos=None): - if pos is not None: - rules_obj = getattr(rules_obj(), str(pos)) - try: - return rules_obj.get() - except Exception as e: - self.module.fail_json( - msg=f'Failed to retrieve firewall rules: {e}' - ) - - def get_groups(self): - try: - return [x['group'] for x in self.proxmox_api.cluster().firewall().groups().get()] - except Exception as e: - self.module.fail_json( - msg=f'Failed to retrieve firewall security groups: {e}' - ) - def delete_fw_rule(self, rules_obj, pos): try: for item in self.get_fw_rules(rules_obj): diff --git a/plugins/modules/proxmox_firewall_info.py b/plugins/modules/proxmox_firewall_info.py new file mode 100644 index 00000000..383e3910 --- /dev/null +++ b/plugins/modules/proxmox_firewall_info.py @@ -0,0 +1,304 @@ +#!/usr/bin/python +# -*- coding: utf-8 -*- +# +# Copyright (c) 2025, Jana Hoch +# 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 + +from __future__ import absolute_import, division, print_function + +__metaclass__ = type + +DOCUMENTATION = r""" +module: proxmox_firewall +short_description: Manage firewall rules in Proxmox +description: + - create/update/delete FW rules at cluster/group/vnet/node/vm level + - Create/delete firewall security groups + - get firewall rules at cluster/group/vnet/node/vm level +author: 'Jana Hoch (!UNKNOWN)' +options: + level: + description: + - Level at which the firewall rule applies. + type: str + choices: + - cluster + - group + - vnet + - node + - vm + default: cluster + node: + description: + - Name of the node. + - only needed when level is node. + type: str + vmid: + description: + - ID of the VM to which the rule applies. + - only needed when level is vm. + type: int + vnet: + description: + - Name of the virtual network for the rule. + - only needed when level is vnet. + type: str + pos: + description: + - Position of the rule in the list. + - only needed if deleting rule or trying to list it + type: int + group: + description: + - Name of the group to which the rule belongs. + - only needed when level is group or group_conf is True. + type: str +extends_documentation_fragment: + - community.proxmox.proxmox.actiongroup_proxmox + - community.proxmox.proxmox.documentation + - community.proxmox.attributes +""" + +EXAMPLES = r""" +- name: Get Cluster level firewall rules, aliases, and security groups + community.proxmox.proxmox_firewall: + api_user: "{{ pc.proxmox.api_user }}" + api_token_id: "{{ pc.proxmox.api_token_id }}" + api_token_secret: "{{ vault.proxmox.api_token_secret }}" + api_host: "{{ pc.proxmox.api_host }}" + validate_certs: no + level: cluster +""" + +RETURN = r""" +groups: + description: + - List of firewall security groups. + - This will always be given for cluster level regardless of the level passed. + - Because only at cluster level we can have firewall security groups + returned: on success + type: list + elements: str + sample: + [ "test" ] + +aliases: + description: + - list of alias present at given level + - aliases are only available for cluster and VM level so if any other level it'll be empty list + returned: on success + type: list + elements: dict + sample: + [ + { + "cidr": "10.10.1.0/24", + "digest": "978391f460484e8d4fb3ca785cfe5a9d16fe8b1f", + "ipversion": 4, + "name": "test1" + }, + { + "cidr": "10.10.2.0/24", + "digest": "978391f460484e8d4fb3ca785cfe5a9d16fe8b1f", + "ipversion": 4, + "name": "test2" + }, + { + "cidr": "10.10.3.0/24", + "digest": "978391f460484e8d4fb3ca785cfe5a9d16fe8b1f", + "ipversion": 4, + "name": "test3" + } + ] + +firewall_rules: + description: List of firewall rules at given level. + returned: on success + type: list + elements: dict + sample: + [ + { + "action": "ACCEPT", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "dport": "53", + "enable": 1, + "ipversion": 4, + "log": "nolog", + "pos": 0, + "proto": "udp", + "source": "192.168.1.0/24", + "type": "in" + }, + { + "action": "ACCEPT", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "dport": "53", + "enable": 1, + "ipversion": 4, + "log": "nolog", + "pos": 1, + "proto": "tcp", + "source": "192.168.1.0/24", + "type": "in" + }, + { + "action": "ACCEPT", + "dest": "192.168.1.0/24", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "enable": 1, + "ipversion": 4, + "log": "nolog", + "pos": 2, + "type": "out" + }, + { + "action": "ACCEPT", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "enable": 1, + "ipversion": 4, + "log": "nolog", + "pos": 3, + "source": "192.168.1.0/24", + "type": "in" + }, + { + "action": "ACCEPT", + "dest": "+sdn/test2-gateway", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "enable": 1, + "iface": "test2", + "log": "nolog", + "macro": "DNS", + "pos": 4, + "type": "in" + }, + { + "action": "ACCEPT", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "enable": 1, + "iface": "test2", + "log": "nolog", + "macro": "DHCPfwd", + "pos": 5, + "type": "in" + }, + { + "action": "ACCEPT", + "dest": "+sdn/test2-all", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "dport": "68", + "enable": 1, + "log": "nolog", + "pos": 6, + "proto": "udp", + "source": "+sdn/test2-gateway", + "sport": "67", + "type": "out" + }, + { + "action": "DROP", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "enable": 1, + "log": "nolog", + "pos": 7, + "type": "in" + }, + { + "action": "DROP", + "digest": "b5ddaed23b415b9368706fc9edc83d037526aae9", + "enable": 1, + "log": "nolog", + "pos": 8, + "type": "out" + } + ] +""" + +from ansible.module_utils.basic import AnsibleModule +from ansible_collections.community.proxmox.plugins.module_utils.proxmox_sdn import ProxmoxSdnAnsible +from ansible_collections.community.proxmox.plugins.module_utils.proxmox import proxmox_auth_argument_spec + + +def get_proxmox_args(): + return dict( + level=dict(type="str", choices=["cluster", "node", "vm", "vnet", "group"], default="cluster", required=False), + node=dict(type="str", required=False), + vmid=dict(type="int", required=False), + vnet=dict(type="str", required=False), + group=dict(type="str", required=False), + pos=dict(type="int", required=False), + ) + + +def get_ansible_module(): + module_args = proxmox_auth_argument_spec() + module_args.update(get_proxmox_args()) + + return AnsibleModule( + argument_spec=module_args, + required_if=[ + ('level', 'vm', ['vmid']), + ('level', 'node', ['node']), + ('level', 'vnet', ['vnet']), + ('level', 'group', ['group']), + ] + ) + + +class ProxmoxFirewallInfoAnsible(ProxmoxSdnAnsible): + def __init__(self, module): + super(ProxmoxFirewallInfoAnsible, self).__init__(module) + self.params = module.params + + def run(self): + level = self.params.get("level") + + if level == "vm": + vm = self.get_vm(vmid=self.params.get('vmid')) + node = self.proxmox_api.nodes( vm['node']) + virt = node(vm['type']) + firewall_obj = virt(str(vm['vmid'])).firewall + rules_obj = firewall_obj().rules + + elif level == "node": + firewall_obj = self.proxmox_api.nodes(self.params.get('node')).firewall + rules_obj = firewall_obj().rules + + elif level == "vnet": + firewall_obj = self.proxmox_api.cluster().sdn().vnets(self.params.get('vnet')).firewall + rules_obj = firewall_obj().rules + + elif level == "group": + firewall_obj = None + rules_obj = self.proxmox_api.cluster().firewall().groups(self.params.get("group")) + + else: + firewall_obj = self.proxmox_api.cluster().firewall + rules_obj = firewall_obj().rules + + rules = self.get_fw_rules(rules_obj, pos=self.params.get('pos')) + groups = self.get_groups() + aliases = self.get_aliases(firewall_obj=firewall_obj) + self.module.exit_json( + changed=False, + firewall_rules=rules, + groups=groups, + aliases=aliases, + msg='successfully retrieved firewall rules and groups' + ) + + +def main(): + module = get_ansible_module() + proxmox = ProxmoxFirewallInfoAnsible(module) + + try: + proxmox.run() + except Exception as e: + module.fail_json(msg=f'An error occurred: {e}') + + +if __name__ == "__main__": + main() From bfd5e5c2ea545bf2bdf193daa9d0b05a662627a4 Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sun, 21 Sep 2025 10:57:49 +0530 Subject: [PATCH 18/22] proxmox_firewall: Merge state present and update --- plugins/modules/proxmox_firewall.py | 152 +++++++--------------------- 1 file changed, 38 insertions(+), 114 deletions(-) diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index c4da1b30..515224ac 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -30,14 +30,13 @@ type: str choices: - present - - update - absent - force: + default: present + update: description: - - If state is present and if 1 or more rule already exists at given pos force will update them - - If state is update and if 1 or more rule doesn't exist force will create + - If O(state=present) and if 1 or more rule/alias already exists it will update them type: bool - default: false + default: truw level: description: - Level at which the firewall rule applies. @@ -239,7 +238,8 @@ api_host: "{{ pc.proxmox.api_host }}" validate_certs: no level: cluster - state: update + state: present + update: True rules: - type: out action: ACCEPT @@ -307,7 +307,8 @@ api_token_secret: "{{ vault.proxmox.api_token_secret }}" api_host: "{{ pc.proxmox.api_host }}" validate_certs: no - state: update + state: present + update: True aliases: - name: test1 cidr: '10.10.1.0/28' @@ -347,8 +348,8 @@ def get_proxmox_args(): return dict( - state=dict(type="str", choices=["present", "absent", "update"], required=False), - force=dict(type="bool", default=False), + state=dict(type="str", choices=["present", "absent"], default="present"), + update=dict(type="bool", default=True), level=dict(type="str", choices=["cluster", "node", "vm", "vnet", "group"], default="cluster", required=False), node=dict(type="str", required=False), vmid=dict(type="int", required=False), @@ -419,7 +420,7 @@ def __init__(self, module): self.params = module.params def validate_params(self): - if self.params.get('state') in ['present', 'update']: + if self.params.get('state') == 'present': if self.params.get('group_conf') != bool(self.params.get('rules') or self.params.get('aliases')): return True else: @@ -433,14 +434,12 @@ def validate_params(self): self.module.fail_json( msg="When State is absent either group_conf should be true or pos/aliases must be present but not both" ) - else: - return True def run(self): self.validate_params() state = self.params.get("state") - force = self.params.get("force") + update = self.params.get("update") level = self.params.get("level") aliases = self.params.get("aliases") rules = self.params.get("rules") @@ -478,48 +477,38 @@ def run(self): if state == "present": if group_conf: - self.create_group(group=group, comment=self.params.get('comment')) - if rules is not None: - self.create_fw_rules(rules_obj=rules_obj, rules=rules, force=force) - if aliases is not None: - self.create_aliases(firewall_obj=firewall_obj, level=level, aliases=aliases, force=force) - elif state == "update": - if group_conf: - self.create_group(group=group, comment=self.params.get('comment')) + self.group_present(group=group, comment=self.params.get('comment')) if rules is not None: - self.update_fw_rules(rules_obj=rules_obj, rules=rules, force=force) + self.fw_rules_present(rules_obj=rules_obj, rules=rules, update=update) if aliases is not None: - self.update_aliases(firewall_obj=firewall_obj, level=level, aliases=aliases, force=force) + self.aliases_present(firewall_obj=firewall_obj, level=level, aliases=aliases, update=update) elif state == "absent": if self.params.get('pos'): - self.delete_fw_rule(rules_obj=rules_obj, pos=self.params.get('pos')) + self.fw_rule_absent(rules_obj=rules_obj, pos=self.params.get('pos')) if group_conf: - self.delete_group(group_name=group) + self.group_absent(group_name=group) if aliases is not None: - self.delete_aliases(firewall_obj=firewall_obj, level=level, aliases=aliases) + self.aliases_absent(firewall_obj=firewall_obj, aliases=aliases) - def create_aliases(self, firewall_obj, level, aliases, force=False): + def aliases_present(self, firewall_obj, level, aliases, update): if firewall_obj is None or level not in ['cluster', 'vm']: self.module.fail_json( msg='Aliases can only be created at cluster or VM level' ) aliases_to_create, aliases_to_update = compare_list_of_dicts( - existing_list=self.get_aliases(firewall_obj=firewall_obj, level=level), + existing_list=self.get_aliases(firewall_obj=firewall_obj), new_list=aliases, uid='name', params_to_ignore=['digest', 'ipversion'] ) if len(aliases_to_create) == 0 and len(aliases_to_update) == 0: - self.module.exit_json( - changed=False, - msg='No need to create/update any aliases' - ) - elif len(aliases_to_update) > 0 and not force: + self.module.exit_json(changed=False, msg='No need to create/update any aliases') + elif len(aliases_to_update) > 0 and not update: self.module.fail_json( - msg=f"Need to update aliases - {[x['name'] for x in aliases_to_update]} but force is false" + msg=f"Need to update aliases - {[x['name'] for x in aliases_to_update]} but update is false" ) for alias in aliases_to_create: @@ -529,51 +518,18 @@ def create_aliases(self, firewall_obj, level, aliases, force=False): self.module.fail_json( msg=f"Failed to create Alias {alias['name']} - {e}" ) - if len(aliases_to_update) > 0 and force: - self.update_aliases(firewall_obj=firewall_obj, level=level, aliases=aliases_to_update, force=False) - else: - self.module.exit_json( - changed=True, - msg="Aliases created" - ) - - def update_aliases(self, firewall_obj, level, aliases, force=False): - aliases_to_create, aliases_to_update = compare_list_of_dicts( - existing_list=self.get_aliases(firewall_obj=firewall_obj, level=level), - new_list=aliases, - uid='name', - params_to_ignore=['digest', 'ipversion'] - ) - - if len(aliases_to_update) == 0 and len(aliases_to_create) == 0: - self.module.exit_json( - changed=False, - msg='No need to create/update any alias.' - - ) - elif len(aliases_to_create) > 0 and not force: - self.module.fail_json( - msg=f"Need to create new alias - {[x['name'] for x in aliases_to_create]} But force is false" - ) - for alias in aliases_to_update: try: - alias_obj = getattr(firewall_obj().aliases(), alias['name']) - alias_obj().put(**alias) + firewall_obj().aliases(alias['name']).put(**alias) except Exception as e: self.module.fail_json( msg=f"Failed to update Alias {alias['name']} - {e}" ) - if len(aliases_to_update) > 0 and force: - self.update_aliases(firewall_obj=firewall_obj, level=level, aliases=aliases_to_update, force=False) - else: - self.module.exit_json( - changed=True, - msg="Aliases updated" - ) - def delete_aliases(self, firewall_obj, level, aliases): - existing_aliases = set([x.get('name') for x in self.get_aliases(firewall_obj=firewall_obj, level=level)]) + self.module.exit_json(changed=True, msg="Aliases created/updated") + + def aliases_absent(self, firewall_obj, aliases): + existing_aliases = set([x.get('name') for x in self.get_aliases(firewall_obj=firewall_obj)]) aliases = set([x.get('name') for x in aliases]) aliases_to_delete = list(existing_aliases.intersection(aliases)) @@ -595,7 +551,7 @@ def delete_aliases(self, firewall_obj, level, aliases): msg="Successfully deleted aliases" ) - def create_group(self, group, comment=None): + def group_present(self, group, comment=None): if group in self.get_groups(): self.module.exit_json( changed=False, group=group, msg=f"security group {group} already exists" @@ -610,7 +566,7 @@ def create_group(self, group, comment=None): msg=f'Failed to create security group: {e}' ) - def delete_group(self, group_name): + def group_absent(self, group_name): if group_name not in self.get_groups(): self.module.exit_json( changed=False, group=group_name, msg=f"security group {group_name} already doesn't exists" @@ -626,7 +582,7 @@ def delete_group(self, group_name): msg=f'Failed to delete security group {group_name}: {e}' ) - def delete_fw_rule(self, rules_obj, pos): + def fw_rule_absent(self, rules_obj, pos): try: for item in self.get_fw_rules(rules_obj): if item.get('pos') == pos: @@ -647,8 +603,8 @@ def delete_fw_rule(self, rules_obj, pos): msg=f'Failed to delete firewall rule at pos {pos}: {e}' ) - def update_fw_rules(self, rules_obj, rules, force): - existing_rules = self.get_fw_rules(rules_obj) + def fw_rules_present(self, rules_obj, rules, update): + existing_rules = self.get_fw_rules(rules_obj=rules_obj) rules_to_create, rules_to_update = compare_list_of_dicts( existing_list=existing_rules, new_list=rules, @@ -656,15 +612,11 @@ def update_fw_rules(self, rules_obj, rules, force): params_to_ignore=['digest', 'ipversion'] ) - if len(rules_to_update) == 0 and len(rules_to_create) == 0: - self.module.exit_json( - changed=False, - msg='No need to update any FW rules.' - - ) - elif len(rules_to_create) > 0 and not force: + if len(rules_to_create) == 0 and len(rules_to_update) == 0: + self.module.exit_json(changed=False, msg='No need to create/update any rule') + elif len(rules_to_update) > 0 and not update: self.module.fail_json( - msg=f"Need to create new rules for pos - {[x['pos'] for x in rules_to_create]} But force is false" + msg=f"Need to update rules at pos - {[x['pos'] for x in rules_to_update]} but update is false" ) for rule in rules_to_update: @@ -677,32 +629,6 @@ def update_fw_rules(self, rules_obj, rules, force): self.module.fail_json( msg=f'Failed to update firewall rule at pos {rule["pos"]}: {e}' ) - - if len(rules_to_create) > 0: - self.create_fw_rules(rules_obj=rules_obj, rules=rules_to_create, force=False) - self.module.exit_json( - changed=True, msg='successfully updated firewall rules' - ) - - def create_fw_rules(self, rules_obj, rules, force): - existing_rules = self.get_fw_rules(rules_obj=rules_obj) - rules_to_create, rules_to_update = compare_list_of_dicts( - existing_list=existing_rules, - new_list=rules, - uid='pos', - params_to_ignore=['digest', 'ipversion'] - ) - - if len(rules_to_create) == 0 and len(rules_to_update) == 0: - self.module.exit_json( - changed=False, - msg='No need to create/update any rule' - ) - elif len(rules_to_update) > 0 and not force: - self.module.fail_json( - msg=f"Need to update rules at pos - {[x['pos'] for x in rules_to_update]} but force is false" - ) - for rule in rules_to_create: try: rules_obj().post(**rule) @@ -712,10 +638,8 @@ def create_fw_rules(self, rules_obj, rules, force): self.module.fail_json( msg=f'Failed to create firewall rule {rule}: {e}' ) - if len(rules_to_update) > 0 and force: - self.update_fw_rules(rules_obj=rules_obj, rules=rules_to_update, force=False) self.module.exit_json( - changed=True, msg='successfully created firewall rules' + changed=True, msg='successfully created/updated firewall rules' ) def move_rule_to_correct_pos(self, rules_obj, rule): From 74b3d44801042cd38fe425286a07e0cf0699af89 Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sun, 21 Sep 2025 12:04:23 +0530 Subject: [PATCH 19/22] proxmox_firewall & proxmox_firewall_info - Added tests and fixed sanity issues --- meta/runtime.yml | 1 + plugins/module_utils/proxmox_sdn.py | 17 +- plugins/modules/proxmox_firewall.py | 6 +- plugins/modules/proxmox_firewall_info.py | 9 +- .../plugins/modules/test_proxmox_firewall.py | 33 +--- .../modules/test_proxmox_firewall_info.py | 182 ++++++++++++++++++ 6 files changed, 196 insertions(+), 52 deletions(-) create mode 100644 tests/unit/plugins/modules/test_proxmox_firewall_info.py diff --git a/meta/runtime.yml b/meta/runtime.yml index 72eef8b5..065cd7f1 100644 --- a/meta/runtime.yml +++ b/meta/runtime.yml @@ -20,6 +20,7 @@ action_groups: - proxmox_disk - proxmox_domain_info - proxmox_firewall + - proxmox_firewall_info - proxmox_group - proxmox_group_info - proxmox_kvm diff --git a/plugins/module_utils/proxmox_sdn.py b/plugins/module_utils/proxmox_sdn.py index 99c408e8..8636b88a 100644 --- a/plugins/module_utils/proxmox_sdn.py +++ b/plugins/module_utils/proxmox_sdn.py @@ -8,17 +8,6 @@ __metaclass__ = type -import traceback - -PROXMOXER_IMP_ERR = None -try: - from proxmoxer import ProxmoxResource - from proxmoxer import __version__ as proxmoxer_version - HAS_PROXMOXER = True -except ImportError: - HAS_PROXMOXER = False - PROXMOXER_IMP_ERR = traceback.format_exc() - from typing import List, Dict from ansible_collections.community.proxmox.plugins.module_utils.proxmox import ( @@ -111,7 +100,7 @@ def get_zones(self, zone_type: str = None) -> List[Dict]: msg=f'Failed to retrieve zone information from cluster: {e}' ) - def get_aliases(self, firewall_obj: ProxmoxResource | None) -> List[Dict]: + def get_aliases(self, firewall_obj): """Get aliases for IP/CIDR at given firewall endpoint level :param firewall_obj: Firewall endpoint as a ProxmoxResource e.g. self.proxmox_api.cluster().firewall @@ -127,7 +116,7 @@ def get_aliases(self, firewall_obj: ProxmoxResource | None) -> List[Dict]: msg='Failed to retrieve aliases' ) - def get_fw_rules(self, rules_obj: ProxmoxResource, pos: int = None) -> List[Dict]: + def get_fw_rules(self, rules_obj, pos=None): """Get firewall rules at given rules endpoint level :param rules_obj: Firewall Rules endpoint as a ProxmoxResource e.g. self.proxmox_api.cluster().firewall().rules @@ -143,7 +132,7 @@ def get_fw_rules(self, rules_obj: ProxmoxResource, pos: int = None) -> List[Dict msg=f'Failed to retrieve firewall rules: {e}' ) - def get_groups(self) -> List: + def get_groups(self): """Get firewall security groups :return: list of groups diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index 515224ac..7211c413 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -12,6 +12,7 @@ DOCUMENTATION = r""" module: proxmox_firewall short_description: Manage firewall rules in Proxmox +version_added: "1.4.0" description: - create/update/delete FW rules at cluster/group/vnet/node/vm level - Create/delete firewall security groups @@ -36,7 +37,7 @@ description: - If O(state=present) and if 1 or more rule/alias already exists it will update them type: bool - default: truw + default: true level: description: - Level at which the firewall rule applies. @@ -348,7 +349,7 @@ def get_proxmox_args(): return dict( - state=dict(type="str", choices=["present", "absent"], default="present"), + state=dict(type="str", choices=["present", "absent"], default="present"), update=dict(type="bool", default=True), level=dict(type="str", choices=["cluster", "node", "vm", "vnet", "group"], default="cluster", required=False), node=dict(type="str", required=False), @@ -490,7 +491,6 @@ def run(self): if aliases is not None: self.aliases_absent(firewall_obj=firewall_obj, aliases=aliases) - def aliases_present(self, firewall_obj, level, aliases, update): if firewall_obj is None or level not in ['cluster', 'vm']: self.module.fail_json( diff --git a/plugins/modules/proxmox_firewall_info.py b/plugins/modules/proxmox_firewall_info.py index 383e3910..8ccfdf7a 100644 --- a/plugins/modules/proxmox_firewall_info.py +++ b/plugins/modules/proxmox_firewall_info.py @@ -10,8 +10,9 @@ __metaclass__ = type DOCUMENTATION = r""" -module: proxmox_firewall +module: proxmox_firewall_info short_description: Manage firewall rules in Proxmox +version_added: "1.4.0" description: - create/update/delete FW rules at cluster/group/vnet/node/vm level - Create/delete firewall security groups @@ -58,6 +59,7 @@ - community.proxmox.proxmox.actiongroup_proxmox - community.proxmox.proxmox.documentation - community.proxmox.attributes + - community.proxmox.attributes.info_module """ EXAMPLES = r""" @@ -73,7 +75,7 @@ RETURN = r""" groups: - description: + description: - List of firewall security groups. - This will always be given for cluster level regardless of the level passed. - Because only at cluster level we can have firewall security groups @@ -238,6 +240,7 @@ def get_ansible_module(): return AnsibleModule( argument_spec=module_args, + supports_check_mode=True, required_if=[ ('level', 'vm', ['vmid']), ('level', 'node', ['node']), @@ -257,7 +260,7 @@ def run(self): if level == "vm": vm = self.get_vm(vmid=self.params.get('vmid')) - node = self.proxmox_api.nodes( vm['node']) + node = self.proxmox_api.nodes(vm['node']) virt = node(vm['type']) firewall_obj = virt(str(vm['vmid'])).firewall rules_obj = firewall_obj().rules diff --git a/tests/unit/plugins/modules/test_proxmox_firewall.py b/tests/unit/plugins/modules/test_proxmox_firewall.py index e5b50f3f..5c1dbeea 100644 --- a/tests/unit/plugins/modules/test_proxmox_firewall.py +++ b/tests/unit/plugins/modules/test_proxmox_firewall.py @@ -69,19 +69,6 @@ def fail_json(*args, **kwargs): raise SystemExit(kwargs) -def get_module_args_state_none(level="cluster", vmid=None, node=None, vnet=None, group=None): - return { - "api_host": "host", - "api_user": "user", - "api_password": "password", - "level": level, - "vmid": vmid, - "node": node, - "vnet": vnet, - "group": group - } - - def get_module_args_group_conf(group, level="cluster", state="present"): return { "api_host": "host", @@ -144,16 +131,6 @@ def tearDown(self): self.mock_module_helper.stop() super(TestProxmoxFirewallModule, self).tearDown() - def test_get_fw_state_none(self): - with pytest.raises(SystemExit) as exc_info: - with set_module_args(get_module_args_state_none()): - self.module.main() - result = exc_info.value.args[0] - assert result["changed"] is False - assert result["msg"] == "successfully retrieved firewall rules and groups" - assert result["firewall_rules"] == RAW_FIREWALL_RULES - assert result["groups"] == ['test1', 'test2'] - def test_create_group(self): with pytest.raises(SystemExit) as exc_info: with set_module_args(get_module_args_group_conf(group='test')): @@ -172,21 +149,13 @@ def test_delete_group(self): assert result["msg"] == 'successfully deleted security group test1' assert result['group'] == 'test1' - def test_update_fw_rules(self): - with pytest.raises(SystemExit) as exc_info: - with set_module_args(get_module_args_rules(state='update')): - self.module.main() - result = exc_info.value.args[0] - assert result['changed'] is True - assert result["msg"] == 'successfully updated firewall rules' - def test_create_fw_rules(self): with pytest.raises(SystemExit) as exc_info: with set_module_args(get_module_args_rules(state='present', pos=2)): self.module.main() result = exc_info.value.args[0] assert result['changed'] is True - assert result["msg"] == 'successfully created firewall rules' + assert result["msg"] == 'successfully created/updated firewall rules' def test_delete_fw_rule(self): with pytest.raises(SystemExit) as exc_info: diff --git a/tests/unit/plugins/modules/test_proxmox_firewall_info.py b/tests/unit/plugins/modules/test_proxmox_firewall_info.py new file mode 100644 index 00000000..2e580b75 --- /dev/null +++ b/tests/unit/plugins/modules/test_proxmox_firewall_info.py @@ -0,0 +1,182 @@ +# -*- coding: utf-8 -*- +# +# Copyright (c) 2025, Jana Hoch +# 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 + +from __future__ import absolute_import, division, print_function + +__metaclass__ = type + +from unittest.mock import patch + +import pytest + +proxmoxer = pytest.importorskip("proxmoxer") + +from ansible.module_utils import basic +from ansible_collections.community.proxmox.plugins.modules import proxmox_firewall_info +from ansible_collections.community.internal_test_tools.tests.unit.plugins.modules.utils import ( + ModuleTestCase, + set_module_args, +) +import ansible_collections.community.proxmox.plugins.module_utils.proxmox as proxmox_utils + +RAW_FIREWALL_RULES = [ + { + "ipversion": 4, + "digest": "245f9fb31d5f59543dedc5a84ba7cd6afa4dbcc0", + "log": "nolog", + "action": "ACCEPT", + "enable": 1, + "type": "out", + "source": "1.1.1.1", + "pos": 0 + }, + { + "enable": 1, + "pos": 1, + "source": "1.0.0.1", + "type": "out", + "action": "ACCEPT", + "digest": "245f9fb31d5f59543dedc5a84ba7cd6afa4dbcc0", + "ipversion": 4 + } +] + +RAW_GROUPS = [ + { + "digest": "fdb62dec01018d4f35c83ecc2ae3f110a8b3bd62", + "group": "test1" + }, + { + "group": "test2", + "digest": "fdb62dec01018d4f35c83ecc2ae3f110a8b3bd62" + } +] + +RAW_ALIASES = [ + { + "name": "test1", + "cidr": "10.10.1.0/24", + "digest": "978391f460484e8d4fb3ca785cfe5a9d16fe8b1f", + "ipversion": 4 + }, + { + "name": "test2", + "cidr": "10.10.2.0/24", + "digest": "978391f460484e8d4fb3ca785cfe5a9d16fe8b1f", + "ipversion": 4 + }, + { + "name": "test3", + "cidr": "10.10.3.0/24", + "digest": "978391f460484e8d4fb3ca785cfe5a9d16fe8b1f", + "ipversion": 4 + } +] + +RAW_CLUSTER_RESOURCES = [ + { + "vmid": 100, + "maxcpu": 8, + "memhost": 860138496, + "type": "qemu", + "id": "qemu/100", + "diskread": 127452302, + "netin": 42, + "netout": 0, + "cpu": 0.0046731498237984, + "uptime": 119787, + "template": 0, + "disk": 0, + "name": "nextcloud", + "maxdisk": 644245094400, + "mem": 445415424, + "status": "running", + "diskwrite": 1024, + "maxmem": 8589934592, + "node": "pve" + } +] + + +def exit_json(*args, **kwargs): + """function to patch over exit_json; package return data into an exception""" + if 'changed' not in kwargs: + kwargs['changed'] = False + raise SystemExit(kwargs) + + +def fail_json(*args, **kwargs): + """function to patch over fail_json; package return data into an exception""" + kwargs['failed'] = True + raise SystemExit(kwargs) + + +def get_module_args(level="cluster", vmid=None, node=None, vnet=None, group=None): + return { + "api_host": "host", + "api_user": "user", + "api_password": "password", + "level": level, + "vmid": vmid, + "node": node, + "vnet": vnet, + "group": group + } + + +class TestProxmoxFirewallModule(ModuleTestCase): + def setUp(self): + super(TestProxmoxFirewallModule, self).setUp() + proxmox_utils.HAS_PROXMOXER = True + self.module = proxmox_firewall_info + self.mock_module_helper = patch.multiple(basic.AnsibleModule, + exit_json=exit_json, + fail_json=fail_json) + self.mock_module_helper.start() + self.connect_mock = patch( + "ansible_collections.community.proxmox.plugins.module_utils.proxmox.ProxmoxAnsible._connect", + ).start() + + self.connect_mock.return_value.cluster.resources.get.return_value = ( + RAW_CLUSTER_RESOURCES + ) + + 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.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.aliases.return_value.get.return_value = RAW_ALIASES + + def tearDown(self): + self.connect_mock.stop() + self.mock_module_helper.stop() + super(TestProxmoxFirewallModule, self).tearDown() + + def test_cluster_level_info(self): + with pytest.raises(SystemExit) as exc_info: + with set_module_args(get_module_args()): + self.module.main() + result = exc_info.value.args[0] + assert result["changed"] is False + assert result["msg"] == "successfully retrieved firewall rules and groups" + assert result["firewall_rules"] == RAW_FIREWALL_RULES + assert result["groups"] == ['test1', 'test2'] + assert result["aliases"] == RAW_ALIASES + + def test_vm_level_info(self): + with pytest.raises(SystemExit) as exc_info: + with set_module_args(get_module_args(level='vm', vmid=100)): + self.module.main() + result = exc_info.value.args[0] + assert result["changed"] is False + assert result["msg"] == "successfully retrieved firewall rules and groups" + assert result["firewall_rules"] == RAW_FIREWALL_RULES + assert result["groups"] == ['test1', 'test2'] + assert result["aliases"] == RAW_ALIASES From 0b01684841805979904dcc495fb50fce47ee8caf Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sun, 21 Sep 2025 15:45:49 +0530 Subject: [PATCH 20/22] module_utils/proxmox: updated compare_list_of_dict() --- plugins/module_utils/proxmox.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/module_utils/proxmox.py b/plugins/module_utils/proxmox.py index d02a49f5..d665966e 100644 --- a/plugins/module_utils/proxmox.py +++ b/plugins/module_utils/proxmox.py @@ -90,7 +90,7 @@ def compare_list_of_dicts(existing_list, new_list, uid, params_to_ignore=None): if params_to_ignore is None: params_to_ignore = list() items_to_update = [] - new_list = [{k: v for k, v in item.items() if v is not None} for item in new_list] + new_list = [{k: v for k, v in item.items() if v is not None and k not in params_to_ignore} for item in new_list] if existing_list is None: items_to_create = new_list From 6f3733391d496e6ea138396199ef489d1c446a08 Mon Sep 17 00:00:00 2001 From: JanaHoch Date: Sat, 27 Sep 2025 07:20:07 +0530 Subject: [PATCH 21/22] Apply suggestions from code review Added suggestions from @IamLunchbox Co-authored-by: IamLunchbox <56757745+IamLunchbox@users.noreply.github.com> --- plugins/module_utils/proxmox.py | 10 ++-- plugins/module_utils/proxmox_sdn.py | 17 +++--- plugins/modules/proxmox_firewall.py | 76 ++++++++++++------------ plugins/modules/proxmox_firewall_info.py | 21 +++---- 4 files changed, 59 insertions(+), 65 deletions(-) diff --git a/plugins/module_utils/proxmox.py b/plugins/module_utils/proxmox.py index d665966e..76fddda6 100644 --- a/plugins/module_utils/proxmox.py +++ b/plugins/module_utils/proxmox.py @@ -78,13 +78,13 @@ def compare_list_of_dicts(existing_list, new_list, uid, params_to_ignore=None): Use case - for firewall rules we will be getting a list of rules from user. We want to filter out which rules needs to be updated and which rules are completely new and needs to be created - @param existing_list: Existing values example - list of existing rules - @param new_list: New values example - list of rules passed to module - @param uid: unique identifier in dict. It should always be present in both lists - in case of firewall rules it's pos - @param params_to_ignore: list of params we want to ignore which are present in existing_list's dict. + :param existing_list: Existing values example - list of existing rules + :param new_list: New values example - list of rules passed to module + :param uid: unique identifier in dict. It should always be present in both lists - in case of firewall rules it's pos + :param params_to_ignore: list of params we want to ignore which are present in existing_list's dict. In case of firewall rules we want to ignore ['digest', 'ipversion'] - @return: returns 2 list items 1st is the list of items which are completely new and needs to be created + :return: returns 2 list items 1st is the list of items which are completely new and needs to be created 2nd is a list of items which needs to be updated """ if params_to_ignore is None: diff --git a/plugins/module_utils/proxmox_sdn.py b/plugins/module_utils/proxmox_sdn.py index 8636b88a..e5656081 100644 --- a/plugins/module_utils/proxmox_sdn.py +++ b/plugins/module_utils/proxmox_sdn.py @@ -104,7 +104,7 @@ def get_aliases(self, firewall_obj): """Get aliases for IP/CIDR at given firewall endpoint level :param firewall_obj: Firewall endpoint as a ProxmoxResource e.g. self.proxmox_api.cluster().firewall - If it is None it'll return empty list + If it is None it'll return an empty list :return: List of aliases and corresponding IP/CIDR """ if firewall_obj is None: @@ -123,14 +123,13 @@ 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 is not None: - rules_obj = getattr(rules_obj(), str(pos)) - try: - return rules_obj.get() - except Exception as e: - self.module.fail_json( - msg=f'Failed to retrieve firewall rules: {e}' - ) + 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}' + ) def get_groups(self): """Get firewall security groups diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index 7211c413..aaea0479 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/plugins/modules/proxmox_firewall.py @@ -14,9 +14,8 @@ short_description: Manage firewall rules in Proxmox version_added: "1.4.0" description: - - create/update/delete FW rules at cluster/group/vnet/node/vm level - - Create/delete firewall security groups - - get firewall rules at cluster/group/vnet/node/vm level + - Create/update/delete firewall rules at cluster/group/vnet/node/vm level. + - Create/delete firewall security groups. author: 'Jana Hoch (!UNKNOWN)' attributes: check_mode: @@ -26,8 +25,7 @@ options: state: description: - - create/update/delete firewall rules or security group - - if state is not provided then it will just list firewall rules at level + - Create/update/delete firewall rules or security group. type: str choices: - present @@ -35,7 +33,7 @@ default: present update: description: - - If O(state=present) and if 1 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: @@ -52,22 +50,22 @@ node: description: - Name of the node. - - only needed when level is node. + - Only needed when O(level=node). type: str vmid: description: - ID of the VM to which the rule applies. - - only needed when level is vm. + - Only needed when O(level=vm). type: int vnet: description: - Name of the virtual network for the rule. - - only needed when level is vnet. + - Only needed when O(level=vnet). type: str pos: description: - Position of the rule in the list. - - only needed if deleting rule or trying to list it + - Only needed if O(state=absent). type: int group_conf: description: @@ -77,7 +75,7 @@ group: description: - Name of the group to which the rule belongs. - - only needed when level is group or group_conf is True. + - Only needed when O(level=group) or O(group_conf=true). type: str comment: description: @@ -86,23 +84,23 @@ type: str aliases: description: - - List of aliases - - Alias can only be created/updated/deleted at cluster or VM level + - List of aliases. + - Alias can only be created/updated/deleted at cluster or VM level. type: list elements: dict suboptions: name: - description: Alias name + description: Alias name. type: str required: true cidr: description: - - CIDR for alias - - only needed when O(state=present) or O(state=update) + - CIDR for alias. + - Only needed when O(state=present) or O(state=update). type: str required: false comment: - description: Comment for Alias + description: Comment for alias. type: str required: false rules: @@ -215,7 +213,7 @@ api_token_id: "{{ pc.proxmox.api_token_id }}" api_token_secret: "{{ vault.proxmox.api_token_secret }}" api_host: "{{ pc.proxmox.api_host }}" - validate_certs: no + validate_certs: false level: cluster state: present rules: @@ -224,12 +222,12 @@ source: 1.1.1.1 log: nolog pos: 9 - enable: True + enable: true - type: out action: ACCEPT source: 1.0.0.1 pos: 10 - enable: True + enable: true - name: Update Cluster level firewall rules community.proxmox.proxmox_firewall: @@ -237,22 +235,22 @@ api_token_id: "{{ pc.proxmox.api_token_id }}" api_token_secret: "{{ vault.proxmox.api_token_secret }}" api_host: "{{ pc.proxmox.api_host }}" - validate_certs: no + validate_certs: false level: cluster state: present - update: True + update: true rules: - type: out action: ACCEPT source: 8.8.8.8 log: nolog pos: 9 - enable: False + enable: false - type: out action: ACCEPT source: 8.8.4.4 pos: 10 - enable: False + enable: false - name: Delete cluster level firewall rule at pos 10 community.proxmox.proxmox_firewall: @@ -260,7 +258,7 @@ api_token_id: "{{ pc.proxmox.api_token_id }}" api_token_secret: "{{ vault.proxmox.api_token_secret }}" api_host: "{{ pc.proxmox.api_host }}" - validate_certs: no + validate_certs: false level: cluster state: absent pos: 10 @@ -271,8 +269,8 @@ api_token_id: "{{ pc.proxmox.api_token_id }}" api_token_secret: "{{ vault.proxmox.api_token_secret }}" api_host: "{{ pc.proxmox.api_host }}" - validate_certs: no - group_conf: True + validate_certs: false + group_conf: true state: present group: test @@ -282,8 +280,8 @@ api_token_id: "{{ pc.proxmox.api_token_id }}" api_token_secret: "{{ vault.proxmox.api_token_secret }}" api_host: "{{ pc.proxmox.api_host }}" - validate_certs: no - group_conf: True + validate_certs: false + group_conf: true state: absent group: test @@ -293,7 +291,7 @@ api_token_id: "{{ pc.proxmox.api_token_id }}" api_token_secret: "{{ vault.proxmox.api_token_secret }}" api_host: "{{ pc.proxmox.api_host }}" - validate_certs: no + validate_certs: false state: present aliases: - name: test1 @@ -307,9 +305,9 @@ api_token_id: "{{ pc.proxmox.api_token_id }}" api_token_secret: "{{ vault.proxmox.api_token_secret }}" api_host: "{{ pc.proxmox.api_host }}" - validate_certs: no + validate_certs: false state: present - update: True + update: true aliases: - name: test1 cidr: '10.10.1.0/28' @@ -322,7 +320,7 @@ api_token_id: "{{ pc.proxmox.api_token_id }}" api_token_secret: "{{ vault.proxmox.api_token_secret }}" api_host: "{{ pc.proxmox.api_host }}" - validate_certs: no + validate_certs: false state: absent aliases: - name: test1 @@ -433,7 +431,7 @@ def validate_params(self): return True else: self.module.fail_json( - msg="When State is absent either group_conf should be true or pos/aliases must be present but not both" + msg="When state is absent either group_conf should be true or pos/aliases must be present but not both" ) def run(self): @@ -447,7 +445,7 @@ def run(self): group = self.params.get("group") group_conf = self.params.get("group_conf") - if rules is not None: + if rules: for rule in rules: rule['icmp-type'] = rule.get('icmp_type') rule['enable'] = ansible_to_proxmox_bool(rule.get('enable')) @@ -479,20 +477,20 @@ def run(self): if state == "present": if group_conf: self.group_present(group=group, comment=self.params.get('comment')) - if rules is not None: + if rules: self.fw_rules_present(rules_obj=rules_obj, rules=rules, update=update) - if aliases is not None: + if aliases: self.aliases_present(firewall_obj=firewall_obj, level=level, aliases=aliases, update=update) elif state == "absent": if self.params.get('pos'): self.fw_rule_absent(rules_obj=rules_obj, pos=self.params.get('pos')) if group_conf: self.group_absent(group_name=group) - if aliases is not None: + if aliases: self.aliases_absent(firewall_obj=firewall_obj, aliases=aliases) def aliases_present(self, firewall_obj, level, aliases, update): - if firewall_obj is None or level not in ['cluster', 'vm']: + if not firewall_obj or level not in ['cluster', 'vm']: self.module.fail_json( msg='Aliases can only be created at cluster or VM level' ) diff --git a/plugins/modules/proxmox_firewall_info.py b/plugins/modules/proxmox_firewall_info.py index 8ccfdf7a..bf178495 100644 --- a/plugins/modules/proxmox_firewall_info.py +++ b/plugins/modules/proxmox_firewall_info.py @@ -14,9 +14,7 @@ short_description: Manage firewall rules in Proxmox version_added: "1.4.0" description: - - create/update/delete FW rules at cluster/group/vnet/node/vm level - - Create/delete firewall security groups - - get firewall rules at cluster/group/vnet/node/vm level + - Get firewall rules at cluster/group/vnet/node/vm level. author: 'Jana Hoch (!UNKNOWN)' options: level: @@ -33,27 +31,26 @@ node: description: - Name of the node. - - only needed when level is node. + - Only needed when O(level=node). type: str vmid: description: - ID of the VM to which the rule applies. - - only needed when level is vm. + - Only needed when O(level=vm). type: int vnet: description: - Name of the virtual network for the rule. - - only needed when level is vnet. + - Only needed when O(level=vnet). type: str pos: description: - Position of the rule in the list. - - only needed if deleting rule or trying to list it type: int group: description: - Name of the group to which the rule belongs. - - only needed when level is group or group_conf is True. + - Only needed when O(level=group). type: str extends_documentation_fragment: - community.proxmox.proxmox.actiongroup_proxmox @@ -69,7 +66,7 @@ api_token_id: "{{ pc.proxmox.api_token_id }}" api_token_secret: "{{ vault.proxmox.api_token_secret }}" api_host: "{{ pc.proxmox.api_host }}" - validate_certs: no + validate_certs: false level: cluster """ @@ -78,7 +75,7 @@ description: - List of firewall security groups. - This will always be given for cluster level regardless of the level passed. - - Because only at cluster level we can have firewall security groups + - Because only at cluster level we can have firewall security groups. returned: on success type: list elements: str @@ -87,8 +84,8 @@ aliases: description: - - list of alias present at given level - - aliases are only available for cluster and VM level so if any other level it'll be empty list + - List of alias present at given level. + - Aliases are only available for cluster and VM level so if any other level it'll be empty list. returned: on success type: list elements: dict From 508e6167d2f896fac5f4c8b29072b154128e6d2a Mon Sep 17 00:00:00 2001 From: Jana Hoch Date: Sat, 27 Sep 2025 08:18:32 +0530 Subject: [PATCH 22/22] 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. --- plugins/module_utils/proxmox.py | 14 +++++++------- plugins/module_utils/proxmox_sdn.py | 17 +++++++++-------- plugins/modules/proxmox_firewall.py | 14 ++++++++------ plugins/modules/proxmox_firewall_info.py | 2 ++ .../plugins/modules/test_proxmox_firewall.py | 4 ++-- .../modules/test_proxmox_firewall_info.py | 4 ++-- 6 files changed, 30 insertions(+), 25 deletions(-) diff --git a/plugins/module_utils/proxmox.py b/plugins/module_utils/proxmox.py index 76fddda6..fc38907e 100644 --- a/plugins/module_utils/proxmox.py +++ b/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 diff --git a/plugins/module_utils/proxmox_sdn.py b/plugins/module_utils/proxmox_sdn.py index e5656081..7e764a1f 100644 --- a/plugins/module_utils/proxmox_sdn.py +++ b/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 diff --git a/plugins/modules/proxmox_firewall.py b/plugins/modules/proxmox_firewall.py index aaea0479..c5023dcc 100644 --- a/plugins/modules/proxmox_firewall.py +++ b/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 (!UNKNOWN)' attributes: check_mode: @@ -25,7 +26,7 @@ options: state: description: - - Create/update/delete firewall rules or security group. + - Create/update/delete firewall rules or security group. type: str choices: - present @@ -33,7 +34,7 @@ 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) diff --git a/plugins/modules/proxmox_firewall_info.py b/plugins/modules/proxmox_firewall_info.py index bf178495..814f6572 100644 --- a/plugins/modules/proxmox_firewall_info.py +++ b/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 (!UNKNOWN)' options: level: diff --git a/tests/unit/plugins/modules/test_proxmox_firewall.py b/tests/unit/plugins/modules/test_proxmox_firewall.py index 5c1dbeea..d9975f11 100644 --- a/tests/unit/plugins/modules/test_proxmox_firewall.py +++ b/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 diff --git a/tests/unit/plugins/modules/test_proxmox_firewall_info.py b/tests/unit/plugins/modules/test_proxmox_firewall_info.py index 2e580b75..938af769 100644 --- a/tests/unit/plugins/modules/test_proxmox_firewall_info.py +++ b/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):