forward_ports not persistent

[rlenferink]

What happened:

I am using the firewall role with the following definition:

firewall:
  # Wipe existing config and replace with the config defined below.
  - previous: replaced

  # For some reason the cockpit and dhcpv6-client are enabled by default.
  # Explicitly disable them.
  - service:
      - cockpit
      - dhcpv6-client
    state: disabled

  # Only allow the ports below.
  - zone: public
    state: enabled
    permanent: true
    interface: ens18
    services:
      - ssh
    port:
        # Web ports
      - '80/tcp'    # HTTP
      - '443/tcp'   # HTTPS

    # NOTE: For some reason the forward_ports are not persistent. When making changes always make them persistent manually using:
    #   firewall-cmd --runtime-to-permanent
    masquerade: true
    forward_port:
      - port: 1194
        proto: udp
        toaddr: 192.168.1.1
        toport: 1195

      - port: 16001
        proto: tcp
        toaddr: 192.168.1.21  # nas.loc (Video Station HTTPS)
        toport: 16001

What you expected to happen:

I expected because of the permanent flag that the forward_ports would also be persistent. The rest of the firewall configuration is persistent (if no forward_ports are specified), however when forward_ports are specified, the configuration is lost after a reboot of the target node.

I verified this with firewall-cmd --list-all-zones. It seems that the forward_ports are gone, and if there is a forward_port specified that the 'normal' ports are also gone.

How to reproduce it (as minimally and precisely as possible):

Use e.g. the snipped above.

Anything else we need to know?:

Environment:

• Firewalld Version (if Fedora based dnf info firewalld or commit hash if developing from git git log -n1 --format=format:"%H"):
• Firewalld Backend (cat /etc/firewalld/firewalld.conf | grep FirewallBackend): 0.9.11
• Ansible Version (ansible --version): 2.16.5
• Firewall System Role Version (if installing from ansible galaxy ansible-galaxy list | grep linux-system-roles.firewall or commit hash if developing from git
  git log -n1 --format=format:"%H"): 4c4a134
• OS (e.g: cat /etc/os-release): Rocky Linux 8.10
• Others:
  • Ansible controller node is Fedora 40
  • This is my home environment

[richm]

@rlenferink I'm trying to reproduce this issue. I have added a test which I think does exactly what you have described: https://github.com/linux-system-roles/firewall/pull/237/files

But I cannot reproduce your results: https://dl.fedoraproject.org/pub/alt/linuxsystemroles/logs/tf_firewall-237_CentOS-Stream-8-2.16_20241015-223409/artifacts/tests_issue_234-ANSIBLE-2.16-general-SUCCESS.log

TASK [Debug1] ******************************************************************
task path: /tmp/collections-VEk/ansible_collections/fedora/linux_system_roles/tests/firewall/tests_issue_234.yml:49
...
public (active)
  target: default
  icmp-block-inversion: no
  interfaces: ens18 eth0
  sources: 
  services: ssh
  ports: 80/tcp 443/tcp
  protocols: 
  forward: no
  masquerade: yes
  forward-ports: 
	port=1194:proto=udp:toport=1195:toaddr=192.168.1.1
	port=16001:proto=tcp:toport=16001:toaddr=192.168.1.21
  source-ports: 
  icmp-blocks: 
  rich rules: 
...
TASK [Reboot] ******************************************************************
task path: /tmp/collections-VEk/ansible_collections/fedora/linux_system_roles/tests/firewall/tests_issue_234.yml:53
Tuesday 15 October 2024  18:44:51 -0400 (0:00:00.684)       0:00:10.317 ******* 
changed: [managed-node1] => {
    "changed": true,
    "elapsed": 21,
    "rebooted": true
}

TASK [Debug2] ******************************************************************
task path: /tmp/collections-VEk/ansible_collections/fedora/linux_system_roles/tests/firewall/tests_issue_234.yml:56
...
public (active)
  target: default
  icmp-block-inversion: no
  interfaces: ens18 eth0
  sources: 
  services: ssh
  ports: 80/tcp 443/tcp
  protocols: 
  forward: no
  masquerade: yes
  forward-ports: 
	port=1194:proto=udp:toport=1195:toaddr=192.168.1.1
	port=16001:proto=tcp:toport=16001:toaddr=192.168.1.21
  source-ports: 
  icmp-blocks: 
  rich rules: 

Am I missing something?

[rlenferink]

@richm thanks for thinking along and investigating this!

I tried to replicate this locally using your suggested playbook, and was eventually able to do so (see https://github.com/rlenferink/ansible-firewall-role-test)

The problem why you (and I) were initially unable to reproduce the problem, was that when a non-existing interface is defined, everything works fine. However, when an existing interface is defined (this is expected behavior), the changed value was overwritten to False, leading to the following part no longer being executed:

firewall/library/firewall_lib.py

Lines 1551 to 1559 in dd6bdeb

	# apply permanent changes
	if changed and (zone_operation or permanent):
	if fw_zone and fw_settings:
	if fw_offline:
	fw.config.set_zone_config(fw_zone, fw_settings.settings)
	else:
	fw_zone.update(fw_settings)
	if need_reload:
	fw.reload()

This resulted in changes only being applied runtime and not permanent.

I couldn't really think on a proper way to add a test for this, since I think what you were trying to do in #237 was to be able to reproduce/debug it and then look at the logging for problem analysis, or is the intention of that PR to merge and prevent regression later on?

[richm]

I couldn't really think on a proper way to add a test for this, since I think what you were trying to do in #237 was to be able to reproduce/debug it and then look at the logging for problem analysis,

Correct. Trying to reproduce the behavior you reported.

or is the intention of that PR to merge and prevent regression later on?

I would really like to add a regression test for this case, ideally as part of #241, but I can add a test after your PR too.

[richm]

I couldn't really think on a proper way to add a test for this, since I think what you were trying to do in #237 was to be able to reproduce/debug it and then look at the logging for problem analysis,

Correct. Trying to reproduce the behavior you reported.

or is the intention of that PR to merge and prevent regression later on?

I would really like to add a regression test for this case, ideally as part of #241, but I can add a test after your PR too.

here is the regression test - #241 (comment)