T7260 Remove last firewall group member.

[RubenNL]

Change Summary

Added the ability to delete the last address in a group.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Code style update (formatting, renaming)
• Refactoring (no functional changes)
• Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
• Other (please describe):

Related Task(s)

• https://vyos.dev/T7260

Related PR(s)

Component(s) name

firewall(_global)

Proposed changes

VyOS groups can have 0 members, but this currently isn't possible with the current implementation.

How to test

To reproduce, on a testing VyOS:

delete firewall
set firewall group address-group test address 1.2.3.4
commit

I would expect the following ansible task to result in one of those 2 being run:

• delete firewall group address-group test address 1.2.3.4
• delete firewall group address-group test address

(both of those result in the same line remaining in the config: set firewall group address-group test)

- name: clear address group test
   vyos.vyos.vyos_firewall_global:
     config:
       group:
         address_group:
         - name: test
           members: []

Currently, nothing happens.

Test results

Tests have ran on a PR in my repo: RubenNL#4

• Sanity tests passed
• Unit tests passed

Tested against VyOS versions:

Integration tests of module firewall_global have run successfully on versions:

• 1.3.4 (out of support, but best I can do without a license)
• 1.5-rolling-202503030030

When I receive a license(already requested, currently being processed), I'll also test it on 1.3, 1.4 and 1.5 stable.

Checklist:

• I have read the CONTRIBUTING document
• I have linked this PR to one or more Phabricator Task(s)
• I have run the ansible sanity and unit tests
• My commit headlines contain a valid Task id
• My change requires a change to the documentation
• I have updated the documentation accordingly
• I have added unit tests to cover my changes
• I have added a file to changelogs/fragments to describe the changes

[omnom62]

@RubenNL are you able to run integration tests against 1.4.x and 1.5.x (e.g. rolling), please?

[RubenNL]

@omnom62 I'm preparing to run it against the newest 1.5: 1.5-rolling-202503030030. (sourced from https://vyos.net/get/nightly-builds/). As I'm not a paying customer, I don't think I can find a 1.4 download link?

[RubenNL]

Just ran the tests against 1.5.

With my modification, I get this error:

TASK [vyos_firewall_global : Assert that correct set of commands were generated] ***
[WARNING]: conditional statements should not include jinja2 templating
delimiters such as {{ }} or {% %}. Found: {{ rendered['commands'] |
symmetric_difference(result['rendered']) |length == 0 }}
fatal: [test]: FAILED! => {
    "assertion": "{{ rendered['commands'] | symmetric_difference(result['rendered']) |length == 0 }}",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed"
}

So i'll have some debugging to do :( I'll set this PR to draft.

[omnom62]

I will try to review and rerun the tests

[RubenNL]

Found the difference, when deleting a firewall group, it now also adds a useless line delete firewall group address-group <group_name> address. Which isn't required when the next line is delete firewall group address-group <group_name>.

Working on fixing this :) shouldn't be too hard.

[RubenNL]

The integration tests are now succeeding on 1.5-rolling-202503030030.

[gaige]

Please add a unit test that fails with the previous code and succeeds with the new code.

[RubenNL]

Please add a unit test that fails with the previous code and succeeds with the new code.

Done. Results can be seen here:

Failing tests, without changes from this PR: https://github.com/RubenNL/vyos.vyos/actions/runs/13950524465/job/39048553338
With my changes: https://github.com/RubenNL/vyos.vyos/actions/runs/13950641004/job/39048943693

[omnom62]

just FYI - there seems to be a bug in 1.5 that fail the integration tests:
[email protected]# set firewall global-options state-policy invalid action reject [edit] [email protected]# commit [ firewall ] Failed to apply firewall: /run/nftables.conf:172:9-39: Error: Could not process rule: Operation not supported ct state invalid counter reject ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ /run/nftables.conf:173:9-14: Error: Could not process rule: Operation not supported return ^^^^^^ [[firewall]] failed Commit failed [edit]
The module fails fatal: [vyos1.5]: FAILED! => { "changed": false, "module_stderr": "commit failed: commit\r\n[ firewall ]\r\nFailed to apply firewall: /run/nftables.conf:169:9-39: Error: Could not\r\nprocess rule: Operation not supported ct state invalid counter\r\nreject ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\r\n/run/nftables.conf:170:9-14: Error: Could not process rule: Operation\r\nnot supported return ^^^^^^\r\n[[firewall]] failed\r\nCommit failed\r\n[edit]\r\r\[email protected]# ", "module_stdout": "", "msg": "MODULE FAILURE: No start of json char found\nSee stdout/stderr for the exact error" }

[RubenNL]

just FYI - there seems to be a bug in 1.5 that fail the integration tests: [email protected]# set firewall global-options state-policy invalid action reject [edit] [email protected]# commit [ firewall ] Failed to apply firewall: /run/nftables.conf:172:9-39: Error: Could not process rule: Operation not supported ct state invalid counter reject ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ /run/nftables.conf:173:9-14: Error: Could not process rule: Operation not supported return ^^^^^^ [[firewall]] failed Commit failed [edit] The module fails fatal: [vyos1.5]: FAILED! => { "changed": false, "module_stderr": "commit failed: commit\r\n[ firewall ]\r\nFailed to apply firewall: /run/nftables.conf:169:9-39: Error: Could not\r\nprocess rule: Operation not supported ct state invalid counter\r\nreject ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\r\n/run/nftables.conf:170:9-14: Error: Could not process rule: Operation\r\nnot supported return ^^^^^^\r\n[[firewall]] failed\r\nCommit failed\r\n[edit]\r\r\[email protected]# ", "module_stdout": "", "msg": "MODULE FAILURE: No start of json char found\nSee stdout/stderr for the exact error" }

I don't have this issue, using 1.5-rolling-202503030030. I'm only running the vyos_firewall_global integration tests, which succeed.

[omnom62]

I don't have this issue, using 1.5-rolling-202503030030. I'm only running the vyos_firewall_global integration tests, which succeed.

All is good, mate, I will retest with fresh image as suggested in Slack

[RubenNL]

it'll probably also work with a manual delete firewall before running the tests.

[gaige]

I don't have this issue, using 1.5-rolling-202503030030. I'm only running the vyos_firewall_global integration tests, which succeed.

Insofar as possible, we should be running all integration tests, especially as part of the intention of this revamp is that we get to the point that the integration tests can be made part of the automation for PR checks.

At the very least, we need to insure that no integration tests either require more than a new VM/physical device with the config lines documented in the readme, nor do they leave anything in their wake. Any pre-conditions need to live defensively in the tests themselves.

Thanks

[gaige]

I think the changes look good. Please update the PR text to indicate which integration tests are succeeding with the current code. Since 6.x is still supporting 1.3, 1.4 (and current), please verify and indicate the specific version of each these were tested against. At that point, I’ll approve.

[omnom62]

LGTM

[RubenNL]

I have just updated the description with the versions I tested this MR with. I've also updated the branch to be up-to-date with the main.

[gaige]

LGTM