firewall: T7781: Firewall chains are created when unneeded

[l0crian1]

Change summary

This fixes incorrect behavior where the input/output/forward/prerouting/etc... chains were created regardless if a user configured them or not. The checks to prevent this were already present in the nftables.j2 Jinja2 template, but it was being skipped due to the <defaultValue> field for default-action making the checks always pass.

This behavior would cause filtering to occur on traffic when there was no intention to filter, effectively slowing down processing.

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/T7781

Related PR(s)

How to test / Smoketest result

Configure a single chain:

vyos@vyos# set firewall ipv4 output filter rule 1 action accept 
vyos@vyos# commit

Check the chains that are created:

Before fix:

vyos@vyos# sudo nft list table vyos_filter
table ip vyos_filter {
        chain VYOS_FORWARD_filter {
                type filter hook forward priority filter; policy accept;
                counter packets 0 bytes 0 accept comment "FWD-filter default-action accept"
        }

        chain VYOS_INPUT_filter {
                type filter hook input priority filter; policy accept;
                counter packets 0 bytes 0 accept comment "INP-filter default-action accept"
        }

        chain VYOS_OUTPUT_filter {
                type filter hook output priority filter; policy accept;
                counter packets 0 bytes 0 accept comment "ipv4-OUT-filter-1"
                counter packets 0 bytes 0 accept comment "OUT-filter default-action accept"
        }

        chain VYOS_OUTPUT_raw {
                type filter hook output priority raw; policy accept;
                counter packets 0 bytes 0 accept comment "OUT-raw default-action accept"
        }

        chain VYOS_PREROUTING_raw {
                type filter hook prerouting priority raw; policy accept;
                counter packets 0 bytes 0 accept comment "PRE-raw default-action accept"
        }

        chain VYOS_FRAG_MARK {
                type filter hook prerouting priority -450; policy accept;
                ip frag-off & 0x3fff != 0x0 meta mark set 0x000ffff1 return
        }
}

After fix:

vyos@vyos# sudo nft list table vyos_filter
table ip vyos_filter {
        chain VYOS_OUTPUT_filter {
                type filter hook output priority filter; policy accept;
                counter packets 370 bytes 38689 accept comment "ipv4-OUT-filter-1"
                counter packets 0 bytes 0 accept comment "OUT-filter default-action accept"
        }

        chain VYOS_FRAG_MARK {
                type filter hook prerouting priority -450; policy accept;
                ip frag-off & 0x3fff != 0x0 meta mark set 0x000ffff1 return
        }
}

Smoketest results:

test_bridge_firewall (__main__.TestFirewall.test_bridge_firewall) ... ok
test_cyclic_jump_validation (__main__.TestFirewall.test_cyclic_jump_validation) ... ok
test_disable_conntrack_per_chain (__main__.TestFirewall.test_disable_conntrack_per_chain) ... ok
test_flow_offload (__main__.TestFirewall.test_flow_offload) ... ok
test_geoip (__main__.TestFirewall.test_geoip) ... ok
test_gre_match (__main__.TestFirewall.test_gre_match) ... ok
test_groups (__main__.TestFirewall.test_groups) ... ok
test_ipsec_metadata_match (__main__.TestFirewall.test_ipsec_metadata_match) ... ok
test_ipv4_advanced (__main__.TestFirewall.test_ipv4_advanced) ... ok
test_ipv4_basic_rules (__main__.TestFirewall.test_ipv4_basic_rules) ... ok
test_ipv4_dynamic_groups (__main__.TestFirewall.test_ipv4_dynamic_groups) ... ok
test_ipv4_global_state (__main__.TestFirewall.test_ipv4_global_state) ... ok
test_ipv4_mask (__main__.TestFirewall.test_ipv4_mask) ... ok
test_ipv4_remote_group (__main__.TestFirewall.test_ipv4_remote_group) ... ok
test_ipv4_state_and_status_rules (__main__.TestFirewall.test_ipv4_state_and_status_rules) ... ok
test_ipv4_synproxy (__main__.TestFirewall.test_ipv4_synproxy) ... ok
test_ipv6_advanced (__main__.TestFirewall.test_ipv6_advanced) ... ok
test_ipv6_basic_rules (__main__.TestFirewall.test_ipv6_basic_rules) ... ok
test_ipv6_dynamic_groups (__main__.TestFirewall.test_ipv6_dynamic_groups) ... ok
test_ipv6_mask (__main__.TestFirewall.test_ipv6_mask) ... ok
test_ipv6_remote_group (__main__.TestFirewall.test_ipv6_remote_group) ... ok
test_nested_groups (__main__.TestFirewall.test_nested_groups) ... ok
test_remote_group (__main__.TestFirewall.test_remote_group) ... ok
test_source_validation (__main__.TestFirewall.test_source_validation) ... ok
test_sysfs (__main__.TestFirewall.test_sysfs) ... ok
test_timeout_sysctl (__main__.TestFirewall.test_timeout_sysctl) ... ok
test_zone_basic (__main__.TestFirewall.test_zone_basic) ... ok
test_zone_flow_offload (__main__.TestFirewall.test_zone_flow_offload) ... ok
test_zone_with_vrf (__main__.TestFirewall.test_zone_with_vrf) ... ok

----------------------------------------------------------------------
Ran 29 tests in 562.998s

OK

Checklist:

• I have read the CONTRIBUTING document
• I have linked this PR to one or more Phabricator Task(s)
• I have run the components SMOKETESTS if applicable
• My commit headlines contain a valid Task id
• My change requires a change to the documentation
• I have updated the documentation accordingly

[github-actions]

❌
PR title does not match the required format

[github-actions]

CI integration ❌ failed!

Details

CI logs

• CLI Smoketests (no interfaces) ❌ failed
• CLI Smoketests VPP 👍 passed
• CLI Smoketests (interfaces only) 👍 passed
• Config tests 👍 passed
• Config tests VPP 👍 passed
• RAID1 tests 👍 passed
• TPM tests 👍 passed

[c-po]

The defaultValue not only serves internally as the default for the code, but also for our documentation. defaultValues will be automatically attached to the <help> message during CLI completion.

The is only one "better/ugly/you name it" solution to the problem you identified:

Mangling the dict and removing defaults if required.

vyos-1x/python/vyos/frrender.py

Lines 78 to 83 in 5845c4b

	def dict_helper_ospf_defaults(ospf, path):
	# We have gathered the dict representation of the CLI, but there are default
	# options which we need to update into the dictionary retrived.
	default_values = conf.get_config_defaults(path, key_mangling=('-', '_'),
	get_first_key=True, recursive=True)

[l0crian1]

Generating unnecessary config, just to turn around and delete it so it doesn't cause unintended behavior seems a little backwards to me. It's treating a symptom rather than solving the problem.

Is the only additional concern the help string? I can just manually update it to:

Default-action for rule-set (default: accept)

The defaultValue has a lot of value within the code, (like for the firewall global settings), but to enable a default value of accept in python, all that is required was this change (along with deleting the defaultValue):

    default_action = fw_conf.get('default_action', 'accept')

All of the other changes (smoketests, verify(), Jinja2, etc...) would be required regardless of the solution. Does keeping the default value in the XML really buy anything at that point?

[c-po]

Is the only additional concern the help string? I can just manually update it to:

Currently this is true - but it will (unfortunately until we find a generic solution to this issue) decouple the CLI help string from the internal implementation. I am a fan of a generic solution but yet have no proper idea how to handle this in the firewall context. Thus I tend to agree we go with your implementation.

Please add a <!-- XML COMMENT --> to the XML help string indicating that the default you define here in the help string is implemented in a different file, so we have a future reference by comment in case anyone starts wondering.

[jestabro]

Following conversations with @l0crian1 , an alternative to dropping the defaultValue element is a simple pruning step (diff below). His additional suggestion to codify this approach by adding hints within the XML, for not including paths solely of default values bears consideration; that is an ongoing discussion ... In the interim, we may consider the suggested pruning step as a solution to inform any later revision, as the key idea is to remove (or not include) certain paths not explicitly set --- N.B. neither this nor the original PR should be adopted without careful review: it is unclear if there are cases where we would want to retain the default paths. Note that with the changes to the smoketest and nftables.j2 in this PR, firewall smoketests pass with the alternative patch below. Tested version here: current...jestabro:prune-default-chains

From cefef39c2846813ff5aa43c0ef2f28fd4f5d473c Mon Sep 17 00:00:00 2001
From: John Estabrook <[email protected]>
Date: Mon, 20 Oct 2025 15:38:01 -0500
Subject: [PATCH 1/3] firewall: T7781: prune config_dict chains added solely by
 defaultValue

---
 src/conf_mode/firewall.py | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py
index 6630b811d..ad899ef20 100755
--- a/src/conf_mode/firewall.py
+++ b/src/conf_mode/firewall.py
@@ -22,6 +22,7 @@
 from sys import exit
 from vyos.base import Warning
 from vyos.config import Config
+from vyos.config import ConfigDict
 from vyos.configdict import is_node_changed
 from vyos.configdiff import get_config_diff, Diff
 from vyos.configdep import set_dependents, call_dependents
@@ -117,6 +118,20 @@ def geoip_updated(conf, firewall):

     return False

+
+def prune_chains_added_by_default(d: ConfigDict):
+    for p in ('ipv4', 'ipv6', 'bridge'):
+        for q in ('forward', 'input', 'output', 'prerouting'):
+            for r in ('filter', 'raw'):
+                path = [p] + [q] + [r]
+                if d.from_defaults(path):
+                    del d[p][q][r]
+                    if not d[p][q]:
+                        del d[p][q]
+        if p in d and not d[p]:
+            del d[p]
+
+
 def get_config(config=None):
     if config:
         conf = config
@@ -129,6 +144,7 @@ def get_config(config=None):
                                     get_first_key=True,
                                     with_recursive_defaults=True)

+    prune_chains_added_by_default(firewall)

     firewall['group_resync'] = bool('group' in firewall or is_node_changed(conf, base + ['group']))
     if firewall['group_resync']:
-- 
2.39.5

[jestabro]

See summary of discussion above: this is not a request for changes, but more of a request for further discussion.