new_module proxmox_firewall #183

JanaHoch · 2025-09-07T17:06:31Z

SUMMARY

Create/update/delete firewall rules at cluster/group/vnet/vm level
create/delete firewall security group at cluster level
create alias at cluster/vm level
list all/ at pos firewall rules at cluster/group/vnet/vm level
list all firewall security groups
list all alias at cluster/vm level

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

proxmox_firewall
proxmox_firewall_info

ADDITIONAL INFORMATION

This is part of #30

- Added method to get FW rules at cluster, node, vm, vnet levels

- Fix Sanity issues

IamLunchbox · 2025-09-08T06:48:00Z

I am neither Collaborator nor Maintainer, but I think you will need to provide Unit Tests as well. Additionally, there is an integration test for several functions, which probably would be well suited for this module. :)

codecov · 2025-09-08T09:44:06Z

Codecov Report

❌ Patch coverage is 67.50630% with 129 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.86%. Comparing base (80dc876) to head (508e616).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
plugins/modules/proxmox_firewall.py	50.00%	69 Missing and 19 partials ⚠️
plugins/module_utils/proxmox.py	40.90%	10 Missing and 3 partials ⚠️
plugins/module_utils/proxmox_sdn.py	47.36%	8 Missing and 2 partials ⚠️
plugins/modules/proxmox_firewall_info.py	80.00%	7 Missing and 3 partials ⚠️
...ests/unit/plugins/modules/test_proxmox_firewall.py	94.11%	3 Missing and 1 partial ⚠️
...unit/plugins/modules/test_proxmox_firewall_info.py	93.54%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #183      +/-   ##
==========================================
+ Coverage   63.57%   64.86%   +1.28%     
==========================================
  Files          60       69       +9     
  Lines        6315     6941     +626     
  Branches     1235     1326      +91     
==========================================
+ Hits         4015     4502     +487     
- Misses       2128     2223      +95     
- Partials      172      216      +44

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

JanaHoch · 2025-09-08T15:05:08Z

I am neither Collaborator nor Maintainer, but I think you will need to provide Unit Tests as well. Additionally, there is an integration test for several functions, which probably would be well suited for this module. :)

aah I hate writing unit tests.... But let me try......

- 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

JanaHoch · 2025-09-09T16:37:12Z

Hi I've added unit test for this module. can you trigger the checks again. I'm working on others but it'll take some time....

JanaHoch · 2025-09-13T05:30:07Z

Hi @IamLunchbox , Hold off on reviewing this. I'm adding way to create aliases and update firewall level options.

- 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

- 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

- Also add get methods in module_utils.

…ty issues

JanaHoch · 2025-09-21T06:39:44Z

Hi @Thulium-Drake / @IamLunchbox please review this also.
I've added get methods in module_utils/proxmox_sdn.py but not sure if this is correct because we will be getting details for both sdn resources and non sdn resources. If it needs to be moved let me know.
And as discussed in previous PRs I've merged state present and update here as well.

IamLunchbox · 2025-09-21T07:05:31Z

I will check it out.

JanaHoch · 2025-09-21T10:17:47Z

Sorry I just added 1 more small change with 0b01684 which i needed for another PR.

Thulium-Drake · 2025-09-22T16:10:10Z

Please ping me when you are happy with the results @IamLunchbox :-) I will merge it and re-start the CI on #182

Thanks for the work!

Also @JanaHoch, I think it's a good moment to do a new release after #181 is merged. Is that PR also ready for prime time?

IamLunchbox

Thank you for your extensive work! @JanaHoch A nice PR, I mostly annotated grammar errors and such.

The only think I would like to discuss is, if the level option is actually needed, when there are implicit arguments, which define the desired level already. E.g. vmid or node.

@Thulium-Drake, what do you think?

plugins/modules/proxmox_firewall_info.py

plugins/modules/proxmox_firewall.py

IamLunchbox · 2025-09-22T21:27:17Z

plugins/modules/proxmox_firewall.py

+            params_to_ignore=['digest', 'ipversion']
+        )
+
+        if len(rules_to_create) == 0 and len(rules_to_update) == 0:


Just a note :)

len(emtpy_list) == 0 is the same as not emtpy_list. Evaluating an empty list with bool() returns False, otherwise True.

JanaHoch · 2025-09-22T22:16:07Z

Please ping me when you are happy with the results @IamLunchbox :-) I will merge it and re-start the CI on #182

Thanks for the work!

Also @JanaHoch, I think it's a good moment to do a new release after #181 is merged. Is that PR also ready for prime time?

#181 will need some time. I need to write unit tests and a way to compare subnets. But I think I can finish it over the weekend. Thanks.

JanaHoch · 2025-09-22T22:30:31Z

Thank you for your extensive work! @JanaHoch A nice PR, I mostly annotated grammar errors and such.

The only think I would like to discuss is, if the level option is actually needed, when there are implicit arguments, which define the desired level already. E.g. vmid or node.

@Thulium-Drake, what do you think?

Yeah, I'm also not sure. I used it so that it'll be clear where this is getting applied. but i guess same can be done with clear docs. also @IamLunchbox Thanks for review. I'm just little packed for the week. I'll make changes over the weekend. Thanks.

Thulium-Drake · 2025-09-23T15:36:14Z

Thank you for your extensive work! @JanaHoch A nice PR, I mostly annotated grammar errors and such.

The only think I would like to discuss is, if the level option is actually needed, when there are implicit arguments, which define the desired level already. E.g. vmid or node.

@Thulium-Drake, what do you think?

I think it's good to make them explicit, so people using this know what they are doing. And who knows what the implications of Proxmox Datacenter Manager's "let's stretch your SDN over all your clusters" feature will bring to the table. (https://pve.proxmox.com/wiki/Proxmox_Datacenter_Manager_Roadmap)

That said, if there is a reason to assume that the option is either redundant or not actually needed, it could be left out.

@IamLunchbox

Added suggestions from @IamLunchbox Co-authored-by: IamLunchbox <[email protected]>

JanaHoch · 2025-09-27T02:03:51Z

Thank you for your extensive work! @JanaHoch A nice PR, I mostly annotated grammar errors and such.
The only think I would like to discuss is, if the level option is actually needed, when there are implicit arguments, which define the desired level already. E.g. vmid or node.
@Thulium-Drake, what do you think?

I think it's good to make them explicit, so people using this know what they are doing. And who knows what the implications of Proxmox Datacenter Manager's "let's stretch your SDN over all your clusters" feature will bring to the table. (https://pve.proxmox.com/wiki/Proxmox_Datacenter_Manager_Roadmap)

That said, if there is a reason to assume that the option is either redundant or not actually needed, it could be left out.

I also vote for keeping it explicit so that there is no confusion. And yes the level is part of the API endpoint we are calling but also that it can be easily inferred based on other parameters.

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

Thulium-Drake · 2025-09-27T14:34:46Z

@JanaHoch thanks for updating the PR with the fixes proposed! 🚀

@IamLunchbox Are you happy with the end-result? ;-)

IamLunchbox · 2025-09-28T07:18:48Z

Lgtm :)

JanaHoch added 9 commits September 6, 2025 23:21

proxmox_firewall: new_module for firewall config

d00c335

- Added method to get FW rules at cluster, node, vm, vnet levels

proxmox_firewall: Add condition for security group

5185516

proxmox_firewall: Add method to create rule at cluster level

bcf4446

proxmox_firewall: Refactor to remove similar functions

7e97435

proxmox_firewall: Add method to update fw rules

32fc55f

proxmox_firewall: Added method to delete rule

f5f231c

proxmox_firewall: create/delete group

7d0b401

proxmox_firewall: Added param validations

36aa623

proxmox_firewall: Added Doc

47d52ca

- Fix Sanity issues

JanaHoch mentioned this pull request Sep 7, 2025

Add proxmox_firewall module #141

Closed

JanaHoch marked this pull request as draft September 7, 2025 23:57

JanaHoch marked this pull request as ready for review September 8, 2025 00:59

JanaHoch force-pushed the feature/firewall branch from 79eefe7 to b480c59 Compare September 9, 2025 16:09

Thulium-Drake mentioned this pull request Sep 11, 2025

proxmox_zone - new module #176

Merged

JanaHoch marked this pull request as draft September 13, 2025 05:30

JanaHoch added 5 commits September 14, 2025 01:32

proxmox_firewall: Add unit tests

6327901

proxmox_firewall: simplify conditions

4b14774

proxmox_firewall: Improve checks

a7cb2df

- 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

proxmox_firewall & proxmox module_utils

1adb2ca

- 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

proxmox_firewall: Add methods to create/update/delete aliases

11a3a19

JanaHoch force-pushed the feature/firewall branch from b480c59 to 11a3a19 Compare September 13, 2025 20:04

Thulium-Drake and others added 3 commits September 17, 2025 15:43

Merge branch 'main' into feature/firewall

ac5d44b

proxmox_firewall: remove unnecsary getattr

a5419bd

proxmox_firewall: Split into seprate proxmox_firewall_info

0741d0c

- Also add get methods in module_utils.

JanaHoch added 2 commits September 21, 2025 10:57

proxmox_firewall: Merge state present and update

bfd5e5c

proxmox_firewall & proxmox_firewall_info - Added tests and fixed sani…

74b3d44

…ty issues

JanaHoch marked this pull request as ready for review September 21, 2025 06:36

module_utils/proxmox: updated compare_list_of_dict()

0b01684

JanaHoch mentioned this pull request Sep 21, 2025

new module proxmox_vnet #182

Open

IamLunchbox reviewed Sep 22, 2025

View reviewed changes

Apply suggestions from code review

6f37333

Added suggestions from @IamLunchbox Co-authored-by: IamLunchbox <[email protected]>

JanaHoch force-pushed the feature/firewall branch from 54d7085 to fd0d2ab Compare September 27, 2025 03:45

proxmox_firewall: Fix minor bugs and sanity issues

508e616

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

JanaHoch force-pushed the feature/firewall branch from fd0d2ab to 508e616 Compare September 27, 2025 03:48

JanaHoch mentioned this pull request Sep 28, 2025

new_module proxmox_subnet #181

Open

Thulium-Drake merged commit b190cb3 into ansible-collections:main Oct 3, 2025
15 checks passed

new_module proxmox_firewall #183

new_module proxmox_firewall #183

Uh oh!

Conversation

JanaHoch commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Uh oh!

IamLunchbox commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JanaHoch commented Sep 8, 2025

Uh oh!

JanaHoch commented Sep 9, 2025

Uh oh!

JanaHoch commented Sep 13, 2025

Uh oh!

JanaHoch commented Sep 21, 2025

Uh oh!

IamLunchbox commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JanaHoch commented Sep 21, 2025

Uh oh!

Thulium-Drake commented Sep 22, 2025

Uh oh!

IamLunchbox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IamLunchbox Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

JanaHoch commented Sep 22, 2025

Uh oh!

JanaHoch commented Sep 22, 2025

Uh oh!

Thulium-Drake commented Sep 23, 2025

Uh oh!

JanaHoch commented Sep 27, 2025

Uh oh!

Thulium-Drake commented Sep 27, 2025

Uh oh!

IamLunchbox commented Sep 28, 2025

Uh oh!

Uh oh!

Uh oh!

JanaHoch commented Sep 7, 2025 •

edited

Loading

IamLunchbox commented Sep 8, 2025 •

edited

Loading

codecov bot commented Sep 8, 2025 •

edited

Loading

IamLunchbox commented Sep 21, 2025 •

edited

Loading