firewall: T8761: Reintroduce VRF-interface names in generated config#5167
firewall: T8761: Reintroduce VRF-interface names in generated config#5167davi2367 wants to merge 1 commit into
Conversation
📝 WalkthroughSummary by CodeRabbit
Walkthroughget_config() exposes per‑VRF member lists; nftables-zone.j2 emits per‑VRF oifname/jump/return rules in forward and OUT chains; many interface modules detect vrf node changes and call set_dependents('firewall', conf); apply() paths run call_dependents(); smoketests adjusted for per-interface oifname expectations. ChangesVRF Member Iteration Refactoring
VRF Dependency Wiring Across Interfaces
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
All contributors have signed the CLA ✍️ ✅ |
|
❌ Typos detected in PR (3 found)
|
|
👍 |
|
Thanks for looking into this so quickly! |
c-po
changed the title
c-po
changed the title
|
@Mergifyio rebase |
|
Deprecation notice: This pull request comes from a fork and was rebased using |
✅ Branch has been successfully rebased |
|
I found an issue when a VRF has more than one member interface. multiple VRF members produce invalid oifname Your PR description shows the expected output as: However, the actual generated output with two VRF members: 1)Reproduce two VRF members 2)additional interface types not picked up: Adding vti or vlan subinterfaces to VRF after commit they are not reflected in nftables at all: 3)five VRF members When the combined interface name string exceeds 16 characters, commit fails: 4)stale nftables rules after removing interface from VRF: After removing eth1 from VRF and committing, nftables rules are not updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@data/templates/firewall/nftables-zone.j2`:
- Around line 92-97: The default_local VRF branch is missing the VRF-device
oifname jump/return pair, so modify the block that iterates over
zone[from_zone].member.vrf to also emit the VRF-name oifname match like the
from_local branch: for each vrf_name in zone[from_zone].member.vrf add an
oifname { <vrf device names> } counter jump NAME{{ suffix }}_{{
from_conf[fw_name] }} and a matching oifname { <vrf device names> } counter
return using zone[from_zone]['vrf_interfaces'][vrf_name] (the same
quoted_join(",") expression) so default_local will catch local-originated VRF
traffic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 504e3fe0-d609-48bd-a4e2-7fcedb99c4c2
📒 Files selected for processing (3)
data/templates/firewall/nftables-zone.j2smoketest/scripts/cli/test_firewall.pysrc/conf_mode/firewall.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_iso
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python code must use Python 3.11 or higher
Files:
src/conf_mode/firewall.pysmoketest/scripts/cli/test_firewall.py
src/conf_mode/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in src/conf_mode/
Files:
src/conf_mode/firewall.py
{**/*.py,data/templates/**/*}
📄 CodeRabbit inference engine (AGENTS.md)
Linting must use ruff 0.6.4, darker, pylint W0611, and Jinja2 lint as enforced by vyos/.github reusables
Files:
src/conf_mode/firewall.pydata/templates/firewall/nftables-zone.j2smoketest/scripts/cli/test_firewall.py
data/templates/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Jinja2 templates should be stored as discrete files under data/templates/ rather than as inline Python strings
Files:
data/templates/firewall/nftables-zone.j2
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Smoketest scripts must be nose2 compatible and located in smoketest/
Files:
smoketest/scripts/cli/test_firewall.py
🧠 Learnings (2)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/cli/test_firewall.py
📚 Learning: 2026-05-26T06:04:29.163Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:118-120
Timestamp: 2026-05-26T06:04:29.163Z
Learning: In VyOS smoketest scripts under `smoketest/scripts/cli/`, it is intentional to call `self.cli_delete(['vrf'])` in both `setUpClass` and `tearDown` to wipe the entire VRF subtree and ensure a clean slate. During code review, do not recommend narrowing the delete to specific VRF identifiers or name subsets (e.g., `['vrf', 'name', 'mgmt']`)—the broad teardown behavior is the established project-wide pattern for these tests.
Applied to files:
smoketest/scripts/cli/test_firewall.py
🔇 Additional comments (3)
src/conf_mode/firewall.py (1)
141-141: LGTM!data/templates/firewall/nftables-zone.j2 (1)
15-17: LGTM!Also applies to: 77-80
smoketest/scripts/cli/test_firewall.py (1)
1146-1147: LGTM!Also applies to: 1159-1163, 1196-1200, 1211-1211
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@data/templates/firewall/nftables-zone.j2`:
- Around line 92-100: The two oifname lines that use zone[from_zone].member.vrf
are currently outside the if 'vrf' in zone[from_zone].member block and use the
wrong accessor; move those two lines so they are generated only inside the
existing if 'vrf' in zone[from_zone].member conditional (same block that
iterates vrf_name) and change the accessor from from_conf.firewall[fw_name] to
from_conf[fw_name] so they match the other uses (referencing
zone[from_zone].member.vrf, zone[from_zone]['vrf_interfaces'], and
from_conf[fw_name]).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 93a8b841-506a-487e-a4b7-bbc8fc8e556e
📒 Files selected for processing (3)
data/templates/firewall/nftables-zone.j2smoketest/scripts/cli/test_firewall.pysrc/conf_mode/firewall.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_iso
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python code must use Python 3.11 or higher
Files:
src/conf_mode/firewall.pysmoketest/scripts/cli/test_firewall.py
src/conf_mode/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in src/conf_mode/
Files:
src/conf_mode/firewall.py
{**/*.py,data/templates/**/*}
📄 CodeRabbit inference engine (AGENTS.md)
Linting must use ruff 0.6.4, darker, pylint W0611, and Jinja2 lint as enforced by vyos/.github reusables
Files:
src/conf_mode/firewall.pysmoketest/scripts/cli/test_firewall.pydata/templates/firewall/nftables-zone.j2
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Smoketest scripts must be nose2 compatible and located in smoketest/
Files:
smoketest/scripts/cli/test_firewall.py
data/templates/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Jinja2 templates should be stored as discrete files under data/templates/ rather than as inline Python strings
Files:
data/templates/firewall/nftables-zone.j2
🧠 Learnings (2)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/cli/test_firewall.py
📚 Learning: 2026-05-26T06:04:29.163Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:118-120
Timestamp: 2026-05-26T06:04:29.163Z
Learning: In VyOS smoketest scripts under `smoketest/scripts/cli/`, it is intentional to call `self.cli_delete(['vrf'])` in both `setUpClass` and `tearDown` to wipe the entire VRF subtree and ensure a clean slate. During code review, do not recommend narrowing the delete to specific VRF identifiers or name subsets (e.g., `['vrf', 'name', 'mgmt']`)—the broad teardown behavior is the established project-wide pattern for these tests.
Applied to files:
smoketest/scripts/cli/test_firewall.py
🔍 Remote MCP
Summary of Relevant Findings for PR Review
nftables Interface Name Limitations
The ifname datatype in nftables is limited to 16 characters maximum as a string. This is critical context for the reported issues in the PR—when concatenating multiple interface names as comma-separated strings, the combined length can easily exceed this limit.
nftables Syntax for Multiple Interfaces
In nftables, oif matches on the integer index of the interface in the kernel, with userspace converting the interface name to the index when the rule is evaluated. For multiple interfaces, nftables uses set syntax like oifname { "eth0", "eth1" } rather than comma-separated strings like oifname "eth1,eth2".
Key Issue from Reported Comments: The PR appears to be generating comma-separated strings (e.g., oifname "eth1,eth2") instead of proper nftables sets. This explains the reviewer's reported problem where multiple VRF members render as a single concatenated string instead of a set, and why string length is exceeded when multiple interfaces are combined.
VyOS Firewall VRF Configuration Context
For VyOS firewall rules with VRFs, inbound-interface uses the VRF name (like MGMT or LAN), while outbound-interface uses the actual interface name (like eth0, vtun0, eth2*). This distinction is important for understanding how the template should structure interface matching rules.
Historical VRF Firewall Work
Previous VyOS firewall VRF work (PR #1330) required passing the VRF name as an argument to nft chain instead of the original interface name—for example, using iifname "mgmt" instead of iifname "eth0" for eth0 in VRF "mgmt".
Data Type Constraints
Set names in nftables must be 16 characters or less, further constraining how interface names can be grouped when concatenated.
These findings establish that the PR's implementation likely has a fundamental issue: the template is generating comma-separated interface strings instead of proper nftables set syntax, which explains all four reported issues (multiple interfaces in single string, string length limits, missing interface types, and stale entries). Reviewers should verify whether the template change properly uses nftables set syntax { "iface1", "iface2" } rather than string concatenation "iface1,iface2".
🔇 Additional comments (8)
src/conf_mode/firewall.py (1)
138-142: LGTM!data/templates/firewall/nftables-zone.j2 (3)
15-17: LGTM!
77-80: LGTM!
94-95: LGTM!smoketest/scripts/cli/test_firewall.py (4)
1146-1147: LGTM!
1159-1163: LGTM!
1196-1199: LGTM!
1211-1211: LGTM!
|
I will have a look :) 1 and 3 are related, and seem to be caused by the introduction of the quotes. I've pushed a version to solve 1 and 3, waiting for the smoke-tests to run in order to verify :) |
davi2367
force-pushed
the
zbf-vrf
branch
2 times, most recently
from
e37dfcd to
43082e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
data/templates/firewall/nftables-zone.j2 (1)
98-99:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix misplaced VRF rules and wrong accessor in
default_localblock.Lines 98-99 are outside the VRF guard and use
from_conf.firewall[fw_name]even though this loop is keyed byfrom_conf[fw_name](Line 87). This can generate invalid output for non-VRF source zones and breaks jump target resolution.Proposed fix
{% if 'vrf' in zone[from_zone].member %} {% for vrf_name in zone[from_zone].member.vrf %} oifname { {{ zone[from_zone]['vrf_interfaces'][vrf_name] | quoted_join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf[fw_name] }} oifname { {{ zone[from_zone]['vrf_interfaces'][vrf_name] | quoted_join(",") }} } counter return {% endfor %} + oifname { {{ zone[from_zone].member.vrf | quoted_join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf[fw_name] }} + oifname { {{ zone[from_zone].member.vrf | quoted_join(",") }} } counter return {% endif %} - oifname { {{ zone[from_zone].member.vrf | quoted_join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }} - oifname { {{ zone[from_zone].member.vrf | quoted_join(",") }} } counter return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data/templates/firewall/nftables-zone.j2` around lines 98 - 99, The two oifname lines generating VRF-specific rules are placed outside the VRF guard and reference the wrong accessor (using from_conf.firewall[fw_name] while the loop is keyed by from_conf[fw_name]); move those lines into the VRF guard within the default_local block and replace from_conf.firewall[fw_name] with from_conf[fw_name] (so the jump target NAME{{ suffix }}_{{ from_conf[fw_name] }} and the return line use the correct key), ensuring non‑VRF source zones don’t get VRF rules emitted.
🧹 Nitpick comments (1)
smoketest/scripts/cli/test_firewall.py (1)
1232-1243: ⚡ Quick winHarden VRF swap validation with inverse checks for stale rules.
This block only asserts new lines exist. It does not fail if old
ZONE1VRF-member lines remain, which is the stale-rule regression this PR is addressing.Proposed test hardening
nftables_search = [ ['oifname "eth0"', 'counter packets', 'jump VZONE_ZONE1'], ['oifname { "vti1", "vti2" }', 'counter packets', 'jump VZONE_ZONE2'], ] nftables_search_v6 = [ ['oifname "eth0"', 'counter packets', 'jump VZONE_ZONE1'], ['oifname { "vti1", "vti2" }', 'counter packets', 'jump VZONE_ZONE2'], ] self.verify_nftables(nftables_search, 'ip vyos_filter') self.verify_nftables(nftables_search_v6, 'ip6 vyos_filter') + + stale_v4 = [ + ['oifname { "eth0", "vti1" }', 'counter packets', 'jump VZONE_ZONE1'], + ] + stale_v6 = [ + ['oifname { "eth0", "vti1" }', 'counter packets', 'jump VZONE_ZONE1'], + ] + self.verify_nftables(stale_v4, 'ip vyos_filter', inverse=True) + self.verify_nftables(stale_v6, 'ip6 vyos_filter', inverse=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@smoketest/scripts/cli/test_firewall.py` around lines 1232 - 1243, The test only asserts presence of expected nftables lines (nftables_search / nftables_search_v6) but doesn't assert absence of stale VRF-member rules, so add inverse checks to fail when old ZONE1 entries remain: update the test around nftables_search / nftables_search_v6 to also verify that the old-rule patterns (e.g., entries that bind vti1/vti2 to VZONE_ZONE1 or eth0 to VZONE_ZONE2) are absent by calling the negative verification path (either extend verify_nftables to accept an "absent" flag or add a new verify_nftables_absent invocation) for both 'ip vyos_filter' and 'ip6 vyos_filter' so stale rules are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/conf_mode/interfaces_bonding.py`:
- Around line 305-307: The condition guarding call_dependents() in
interfaces_bonding.py misses VRF removals; update the if expression that
currently checks dict_search('member.interface_remove', bond) or 'static_arp' in
bond or 'vrf' in bond to also detect VRF removal (e.g. include
dict_search('member.vrf_remove', bond)). This ensures call_dependents() is
invoked when set_dependents('firewall', conf) registered the firewall dependency
for VRF changes; modify the condition around call_dependents() to include
dict_search('member.vrf_remove', bond) so firewall rules are updated on VRF
removal.
In `@src/conf_mode/interfaces_bridge.py`:
- Line 241: The condition that gates running dependents uses
"interfaces_need_update or 'static_arp' in bridge or 'vrf' in bridge", which
misses the case where VRF was removed and the 'vrf' key is absent even though
is_node_changed(..., 'vrf') is true; update the check to also consult
is_node_changed(bridge, 'vrf') (or the equivalent node-change flag used earlier)
so that when VRF changes/removal is detected the code will still enter the block
and call_dependents() (referencing interfaces_need_update, bridge,
is_node_changed, and call_dependents in your patch).
In `@src/conf_mode/interfaces_pseudo-ethernet.py`:
- Around line 69-71: The VRF detach case can skip firing queued firewall updates
because you only set dependents when is_node_changed(conf, base + [ifname,
'vrf']) but later only call_dependents() when 'vrf' in peth; modify the logic to
use an explicit boolean from get_config() (e.g. vrf_changed) instead of checking
for the 'vrf' key: have get_config() return vrf_changed,
set_dependents('firewall', conf) when vrf_changed is true, and change the gate
that invokes call_dependents() to check vrf_changed (or a combined changed flag)
so removals (no 'vrf' key) still trigger call_dependents() for the 'firewall'
dependents; update references around is_node_changed, set_dependents,
call_dependents, and get_config to use the new vrf_changed flag consistently.
In `@src/conf_mode/interfaces_sstpc.py`:
- Around line 63-64: Compute a dedicated VRF-change marker (e.g., vrf_changed =
is_node_changed(conf, base + [ifname, 'vrf'])) early in the routine and call
set_dependents('firewall', conf) whenever vrf_changed before any early returns;
do not rely on "'vrf' in sstpc" to detect deletes/unsets—use the marker so
deletions/unsets also trigger dependents. Ensure the vrf_changed check is
evaluated prior to the branches that return early (the blocks currently gated by
"'vrf' in sstpc" and the earlier return paths) so firewall dependents are always
invoked on VRF add/change/delete.
In `@src/conf_mode/interfaces_tunnel.py`:
- Around line 84-85: The VRF delete path can skip running dependents; change the
logic around is_node_changed(conf, base + [ifname, 'vrf']) and
set_dependents('firewall', conf) to set a vrf_changed boolean marker when the
VRF node was changed (added/modified/removed), and then use that vrf_changed
marker to drive call_dependents('firewall', conf) before any early returns on
delete paths; also replace checks that test "'vrf' in tunnel" with checks on
vrf_changed (or OR with it) so dependents run even when the vrf key has been
deleted. Ensure you update locations referencing is_node_changed,
set_dependents, call_dependents and the tunnel vrf existence checks so the
firewall dependents always run if vrf_changed is true.
In `@src/conf_mode/interfaces_virtual-ethernet.py`:
- Around line 59-60: The code detects VRF node changes with
is_node_changed(conf, base + [ifname, 'vrf']) but later only calls
set_dependents('firewall', conf) when 'vrf' exists in the final veth dict, so
VRF removals are ignored; change the logic to capture the boolean VRF change
(e.g., vrf_changed = is_node_changed(...)) and use that flag to call
set_dependents('firewall', conf) whenever vrf_changed is true regardless of
whether 'vrf' is present in veth, and apply the same pattern to the other
occurrence around the existing "'vrf' in veth" check (lines referencing veth and
set_dependents) so both VRF additions and removals trigger dependents.
In `@src/conf_mode/interfaces_vti.py`:
- Around line 45-46: The VRF change handling can miss deletions because
dependents are only queued when is_node_changed(conf, base + [ifname, 'vrf']) is
true but early returns on delete (lines ~61-63) and later checks only look for
'vrf' in vti; fix by introducing a vrf_changed boolean set when
is_node_changed(...) is true, call set_dependents('firewall', conf) at that
moment, and ensure any early-return-on-delete path invokes dependents if
vrf_changed is true (i.e., run dependents before returning); also replace later
checks that test only "'vrf' in vti" with a combined condition that considers
vrf_changed so VRF removals still trigger set_dependents.
In `@src/conf_mode/interfaces_vxlan.py`:
- Around line 93-94: The VRF-change detection currently only calls
set_dependents('firewall', conf) but apply() checks "'vrf' in vxlan" (current
state) so VRF removals can miss signaling; change the detection path (where
is_node_changed(conf, base + [ifname, 'vrf']) is true) to set a persistent flag
in the config like conf[...]['vrf_changed']=True (and still call
set_dependents('firewall', conf)), then update apply() to test the persisted
vrf_changed flag on the vxlan config object (e.g., vxlan.get('vrf_changed') or
conf lookup) instead of "'vrf' in vxlan", and ensure apply() clears the
vrf_changed flag after handling so the signal is not re-used.
In `@src/conf_mode/interfaces_wireless.py`:
- Around line 96-97: The VRF removal path can skip firewall dependents because
code can return before call_dependents() runs; add a vrf_changed boolean marker
where the VRF-dependent firewall is registered (the same place that currently
calls call_dependents() on VRF node change) and set it true when the VRF state
differs, then ensure every early return path (including the branch that
currently returns at the block around Lines 324-327 and the check using "'vrf'
in wifi" around Lines 402-403) calls call_dependents() if vrf_changed is true
before returning; specifically, set vrf_changed when you detect VRF
registration/change and at every exit point for the function ensure
call_dependents() is invoked if vrf_changed to guarantee firewall refresh on VRF
unset/delete.
In `@src/conf_mode/interfaces_wwan.py`:
- Line 193: Typo fix: update the comment text "run the dependends" to "run the
dependents" (it's a plain comment string) so the wording is correct; locate the
comment containing "run the dependends" in interfaces_wwan.py and replace it
with "run the dependents".
- Around line 194-195: VRF removals don't trigger call_dependents() because the
current check in apply() only looks for keys existing in wwan; instead, extend
the condition to also detect removed keys by using is_node_changed(). Update the
if guarding call_dependents() so it calls call_dependents() when 'static_arp' in
wwan or 'vrf' in wwan OR when is_node_changed(conf, 'vrf') (and similarly for
'static_arp' if relevant), ensuring set_dependents('firewall', conf)
registrations due to is_node_changed() actually result in dependents being
invoked even when the key was removed.
---
Duplicate comments:
In `@data/templates/firewall/nftables-zone.j2`:
- Around line 98-99: The two oifname lines generating VRF-specific rules are
placed outside the VRF guard and reference the wrong accessor (using
from_conf.firewall[fw_name] while the loop is keyed by from_conf[fw_name]); move
those lines into the VRF guard within the default_local block and replace
from_conf.firewall[fw_name] with from_conf[fw_name] (so the jump target NAME{{
suffix }}_{{ from_conf[fw_name] }} and the return line use the correct key),
ensuring non‑VRF source zones don’t get VRF rules emitted.
---
Nitpick comments:
In `@smoketest/scripts/cli/test_firewall.py`:
- Around line 1232-1243: The test only asserts presence of expected nftables
lines (nftables_search / nftables_search_v6) but doesn't assert absence of stale
VRF-member rules, so add inverse checks to fail when old ZONE1 entries remain:
update the test around nftables_search / nftables_search_v6 to also verify that
the old-rule patterns (e.g., entries that bind vti1/vti2 to VZONE_ZONE1 or eth0
to VZONE_ZONE2) are absent by calling the negative verification path (either
extend verify_nftables to accept an "absent" flag or add a new
verify_nftables_absent invocation) for both 'ip vyos_filter' and 'ip6
vyos_filter' so stale rules are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f2e8bb9c-3e1f-49dc-8096-5ecdefbd816f
📒 Files selected for processing (21)
data/templates/firewall/nftables-zone.j2smoketest/scripts/cli/test_firewall.pysrc/conf_mode/firewall.pysrc/conf_mode/interfaces_bonding.pysrc/conf_mode/interfaces_bridge.pysrc/conf_mode/interfaces_dummy.pysrc/conf_mode/interfaces_ethernet.pysrc/conf_mode/interfaces_geneve.pysrc/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_macsec.pysrc/conf_mode/interfaces_openvpn.pysrc/conf_mode/interfaces_pppoe.pysrc/conf_mode/interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_sstpc.pysrc/conf_mode/interfaces_tunnel.pysrc/conf_mode/interfaces_virtual-ethernet.pysrc/conf_mode/interfaces_vti.pysrc/conf_mode/interfaces_vxlan.pysrc/conf_mode/interfaces_wireguard.pysrc/conf_mode/interfaces_wireless.pysrc/conf_mode/interfaces_wwan.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_iso
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python code must use Python 3.11 or higher
Files:
src/conf_mode/interfaces_bridge.pysrc/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_vxlan.pysrc/conf_mode/interfaces_vti.pysrc/conf_mode/interfaces_virtual-ethernet.pysrc/conf_mode/interfaces_tunnel.pysrc/conf_mode/interfaces_geneve.pysrc/conf_mode/interfaces_wireguard.pysrc/conf_mode/interfaces_macsec.pysrc/conf_mode/interfaces_pppoe.pysrc/conf_mode/interfaces_sstpc.pysrc/conf_mode/interfaces_bonding.pysrc/conf_mode/interfaces_openvpn.pysrc/conf_mode/interfaces_dummy.pysrc/conf_mode/interfaces_wwan.pysrc/conf_mode/interfaces_ethernet.pysrc/conf_mode/interfaces_wireless.pysrc/conf_mode/firewall.pysrc/conf_mode/interfaces_pseudo-ethernet.pysmoketest/scripts/cli/test_firewall.py
src/conf_mode/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in src/conf_mode/
Files:
src/conf_mode/interfaces_bridge.pysrc/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_vxlan.pysrc/conf_mode/interfaces_vti.pysrc/conf_mode/interfaces_virtual-ethernet.pysrc/conf_mode/interfaces_tunnel.pysrc/conf_mode/interfaces_geneve.pysrc/conf_mode/interfaces_wireguard.pysrc/conf_mode/interfaces_macsec.pysrc/conf_mode/interfaces_pppoe.pysrc/conf_mode/interfaces_sstpc.pysrc/conf_mode/interfaces_bonding.pysrc/conf_mode/interfaces_openvpn.pysrc/conf_mode/interfaces_dummy.pysrc/conf_mode/interfaces_wwan.pysrc/conf_mode/interfaces_ethernet.pysrc/conf_mode/interfaces_wireless.pysrc/conf_mode/firewall.pysrc/conf_mode/interfaces_pseudo-ethernet.py
{**/*.py,data/templates/**/*}
📄 CodeRabbit inference engine (AGENTS.md)
Linting must use ruff 0.6.4, darker, pylint W0611, and Jinja2 lint as enforced by vyos/.github reusables
Files:
src/conf_mode/interfaces_bridge.pysrc/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_vxlan.pysrc/conf_mode/interfaces_vti.pysrc/conf_mode/interfaces_virtual-ethernet.pysrc/conf_mode/interfaces_tunnel.pysrc/conf_mode/interfaces_geneve.pysrc/conf_mode/interfaces_wireguard.pysrc/conf_mode/interfaces_macsec.pysrc/conf_mode/interfaces_pppoe.pysrc/conf_mode/interfaces_sstpc.pysrc/conf_mode/interfaces_bonding.pysrc/conf_mode/interfaces_openvpn.pysrc/conf_mode/interfaces_dummy.pysrc/conf_mode/interfaces_wwan.pysrc/conf_mode/interfaces_ethernet.pysrc/conf_mode/interfaces_wireless.pysrc/conf_mode/firewall.pysrc/conf_mode/interfaces_pseudo-ethernet.pydata/templates/firewall/nftables-zone.j2smoketest/scripts/cli/test_firewall.py
data/templates/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Jinja2 templates should be stored as discrete files under data/templates/ rather than as inline Python strings
Files:
data/templates/firewall/nftables-zone.j2
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Smoketest scripts must be nose2 compatible and located in smoketest/
Files:
smoketest/scripts/cli/test_firewall.py
🧠 Learnings (2)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/cli/test_firewall.py
📚 Learning: 2026-05-26T06:04:29.163Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:118-120
Timestamp: 2026-05-26T06:04:29.163Z
Learning: In VyOS smoketest scripts under `smoketest/scripts/cli/`, it is intentional to call `self.cli_delete(['vrf'])` in both `setUpClass` and `tearDown` to wipe the entire VRF subtree and ensure a clean slate. During code review, do not recommend narrowing the delete to specific VRF identifiers or name subsets (e.g., `['vrf', 'name', 'mgmt']`)—the broad teardown behavior is the established project-wide pattern for these tests.
Applied to files:
smoketest/scripts/cli/test_firewall.py
🔇 Additional comments (11)
src/conf_mode/firewall.py (1)
141-141: LGTM!src/conf_mode/interfaces_dummy.py (1)
75-76: Duplicate of VRF-detach dependent trigger gap already raised.Same pattern as
src/conf_mode/interfaces_bridge.py(gatingcall_dependents()on'vrf' in ...) can miss VRF removal events.src/conf_mode/interfaces_ethernet.py (1)
450-451: Duplicate of VRF-detach dependent trigger gap already raised.Same
'vrf' in ethernetgate can skipcall_dependents()when VRF is removed but changed.src/conf_mode/interfaces_geneve.py (1)
104-105: Duplicate of VRF-detach dependent trigger gap already raised.
'vrf' in geneveas execution gate has the same miss-on-removal behavior.src/conf_mode/interfaces_l2tpv3.py (1)
115-116: Duplicate of VRF-detach dependent trigger gap already raised.
'vrf' in l2tpv3can miss dependent execution on VRF removal changes.src/conf_mode/interfaces_macsec.py (1)
207-208: Duplicate of VRF-detach dependent trigger gap already raised.
'vrf' in macsecgate has the same removal-path miss.src/conf_mode/interfaces_openvpn.py (1)
837-838: Duplicate of VRF-detach dependent trigger gap already raised.
'vrf' in openvpnmay be false on detach even when VRF node changed, so dependents may not run.src/conf_mode/interfaces_pppoe.py (1)
150-151: Duplicate of VRF-detach dependent trigger gap already raised.
'vrf' in pppoegating can skipcall_dependents()on VRF removal.src/conf_mode/interfaces_wireguard.py (1)
90-91: LGTM!src/conf_mode/interfaces_wwan.py (1)
95-98: LGTM!src/conf_mode/interfaces_bonding.py (1)
173-176: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
src/conf_mode/interfaces_vxlan.py (1)
93-94:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftVRF removal does not trigger firewall update.
Line 93 detects VRF changes and marks firewall dependents, but line 265 checks current VRF presence (
'vrf' in vxlan) instead of change state. When VRF is removed,set_dependents('firewall', conf)runs butcall_dependents()does not, leaving stale nftables rules. This matches issue#4reported by AnDroed09 in PR comments.Persist a
vrf_changedflag in the config dict at line 93-94, then check that flag (not current VRF presence) at line 265.🔧 Suggested fix pattern
In
get_config()after line 94:# Check vrf membership, to ensure firewall is updated if is_node_changed(conf, base + [ifname, 'vrf']): + vxlan['vrf_changed'] = True set_dependents('firewall', conf)In
apply()at line 265:# run the dependents -if 'static_arp' in vxlan or 'vrf' in vxlan: +if 'static_arp' in vxlan or vxlan.get('vrf_changed'): call_dependents()Note: This pattern appears in 17+ interface modules in this PR; all require the same fix.
Also applies to: 264-265
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/conf_mode/interfaces_vxlan.py` around lines 93 - 94, When detecting a VRF change via is_node_changed(conf, base + [ifname, 'vrf']) in get_config(), set a boolean flag in the interface config (e.g., conf[ifname]['vrf_changed'] = True) instead of relying on the current presence of 'vrf'; then in apply(), replace the conditional that checks "'vrf' in vxlan" with a check of that flag (e.g., if vxlan.get('vrf_changed'): call_dependents('firewall', conf) ) and clear the flag after calling dependents. Ensure you use the same key name (vrf_changed) so other modules can be updated the same way.src/conf_mode/interfaces_bonding.py (1)
305-307:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winVRF removal won't trigger call_dependents().
When a VRF is removed from the bond, line 175 calls
set_dependents('firewall', conf), but the condition'vrf' in bondevaluates toFalsebecause the VRF key no longer exists in the config dict. As a result,call_dependents()is never invoked, leaving firewall rules stale.🔧 Proposed fix: track dependent registration
def get_config(config=None): ... # Check vrf membership, to ensure firewall is updated if is_node_changed(conf, base + [ifname, 'vrf']): set_dependents('firewall', conf) + bond['firewall_dependent_set'] = True bond['vpp_ifaces'] = cli_ifaces_list(conf) return bond- if dict_search('member.interface_remove', bond) or 'static_arp' in bond or 'vrf' in bond: + if dict_search('member.interface_remove', bond) or 'static_arp' in bond or 'vrf' in bond or 'firewall_dependent_set' in bond: try: call_dependents()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/conf_mode/interfaces_bonding.py` around lines 305 - 307, The condition checking "'vrf' in bond" misses VRF removals because the key is already gone; change the logic that decides to call call_dependents() to detect VRF removal by comparing the previous bond/config to the new one (e.g. check if 'vrf' was present in the prior bond/old_conf and is now absent) or use the dependent-registration flag set by set_dependents('firewall', conf) to decide to call call_dependents(); update the conditional around dict_search('member.interface_remove', bond) || 'vrf' in bond to also trigger when VRF was removed (previous_bond contains 'vrf' and bond does not) so call_dependents() runs for VRF removals.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/conf_mode/interfaces_dummy.py`:
- Around line 45-47: The firewall dependents are only being queued when the
'vrf' key exists in dummy, so VRF detach (key removal) doesn't trigger updates;
change the check at the later block that currently uses "'vrf' in dummy" to
instead use the same change-detection used earlier (is_node_changed(conf, base +
[ifname, 'vrf'])) and call set_dependents('firewall', conf) when that function
returns true; ensure both places (the earlier call and the later conditional)
use the same is_node_changed(...) predicate so VRF additions, updates, and
removals all trigger firewall dependents.
In `@src/conf_mode/interfaces_ethernet.py`:
- Around line 199-200: The firewall update is skipped on VRF removal because
later logic checks for the current presence of 'vrf' in the ethernet dict,
missing detach events; change the gating so it triggers when the VRF node
actually changed (including removals). Replace the presence check that looks for
"'vrf' in ethernet" (around the block gated by is_node_changed(conf, base +
[ifname, 'vrf']) and executed via set_dependents('firewall', conf)) with a
condition that uses is_node_changed(conf, base + [ifname, 'vrf']) OR checks the
previous config for a vrf entry (i.e., treat removal as a change by inspecting
the old config at base + [ifname, 'vrf'] or using the same is_node_changed
helper), ensuring set_dependents('firewall', conf) runs for both attach and
detach events.
In `@src/conf_mode/interfaces_geneve.py`:
- Around line 61-62: The firewall dependents are being triggered based on the
presence of the 'vrf' key instead of whether the VRF actually changed; update
the logic so you call set_dependents('firewall', conf) when the VRF node changed
(use the same change-detection used earlier) rather than when 'vrf' is present.
Concretely, replace the "'vrf' in geneve" presence check with an
is_node_changed(conf, base + [ifname, 'vrf'])-based condition (or otherwise
compare old vs new geneve.vrf) so set_dependents('firewall', conf) runs on VRF
attach/detach changes for the geneve interface.
In `@src/conf_mode/interfaces_l2tpv3.py`:
- Around line 67-68: The VRF removal isn't triggering dependent execution
because the later presence check only inspects the current config keys; update
the logic that decides whether to call set_dependents/call_dependents for
'firewall' so it detects deletions too (e.g., use is_node_changed(conf, base +
[ifname, 'vrf']) or check the previous config/state rather than only checking
for the key in conf). Specifically, modify the code that currently checks key
presence (the block that decides whether to call_dependents / call_dependents
for 'firewall' around the existing presence check) to also treat a removed VRF
(node deletion) as a change and invoke set_dependents('firewall', conf) /
call_dependents accordingly.
In `@src/conf_mode/interfaces_macsec.py`:
- Around line 88-89: The VRF-change handling currently only calls
set_dependents('firewall', conf) when is_node_changed(base + [ifname, 'vrf'])
but you also skip this on detach/delete paths and later only check "'vrf' in
macsec", which can leave stale firewall state; update the logic in
interfaces_macsec.py so that any VRF removal or change triggers set_dependents
regardless of the normal update path: ensure you check both the node-change
(is_node_changed(base + [ifname, 'vrf'])) and the presence/absence of 'vrf' in
the macsec dict (i.e., handle when 'vrf' is removed on detach/delete), and move
or add the set_dependents('firewall', conf) call into the detach/delete handling
(before the early return) so firewall dependents are always updated when VRF is
removed or changed.
In `@src/conf_mode/interfaces_openvpn.py`:
- Around line 159-160: The VRF-change detection for OpenVPN is done too late and
misses interface deletions/disables and key removals; move the is_node_changed
check for base + [ifname, 'vrf'] so it runs before any early-return paths in the
interface handling blocks and call set_dependents('firewall', conf) when that
check is true; additionally replace the fragile "'vrf' in openvpn" presence test
with a proper node-change/compare (e.g., use is_node_changed or compare old vs
new VRF value) so a removed VRF key also triggers the firewall detach/update
logic.
In `@src/conf_mode/interfaces_pppoe.py`:
- Around line 78-79: The VRF change check uses is_node_changed(conf, base +
[ifname, 'vrf']) but dependent execution (set_dependents('firewall', conf)) can
be skipped on VRF detach/delete or when flows return early during interface
disable/delete; update the code so that whenever is_node_changed(...) for the
VRF path is true you always call set_dependents('firewall', conf) before any
early return paths (e.g. the interface delete/disable return branches), or
alternatively move the set_dependents('firewall', conf) invocation into the
shared teardown/exit path executed for delete/disable; ensure both the
VRF-change branch and the delete/disable branches reference the same condition
(is_node_changed on base + [ifname, 'vrf']) so detach is not missed.
---
Duplicate comments:
In `@src/conf_mode/interfaces_bonding.py`:
- Around line 305-307: The condition checking "'vrf' in bond" misses VRF
removals because the key is already gone; change the logic that decides to call
call_dependents() to detect VRF removal by comparing the previous bond/config to
the new one (e.g. check if 'vrf' was present in the prior bond/old_conf and is
now absent) or use the dependent-registration flag set by
set_dependents('firewall', conf) to decide to call call_dependents(); update the
conditional around dict_search('member.interface_remove', bond) || 'vrf' in bond
to also trigger when VRF was removed (previous_bond contains 'vrf' and bond does
not) so call_dependents() runs for VRF removals.
In `@src/conf_mode/interfaces_vxlan.py`:
- Around line 93-94: When detecting a VRF change via is_node_changed(conf, base
+ [ifname, 'vrf']) in get_config(), set a boolean flag in the interface config
(e.g., conf[ifname]['vrf_changed'] = True) instead of relying on the current
presence of 'vrf'; then in apply(), replace the conditional that checks "'vrf'
in vxlan" with a check of that flag (e.g., if vxlan.get('vrf_changed'):
call_dependents('firewall', conf) ) and clear the flag after calling dependents.
Ensure you use the same key name (vrf_changed) so other modules can be updated
the same way.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5108f500-73b4-46b4-9b29-35b8fb594c92
📒 Files selected for processing (21)
data/templates/firewall/nftables-zone.j2smoketest/scripts/cli/test_firewall.pysrc/conf_mode/firewall.pysrc/conf_mode/interfaces_bonding.pysrc/conf_mode/interfaces_bridge.pysrc/conf_mode/interfaces_dummy.pysrc/conf_mode/interfaces_ethernet.pysrc/conf_mode/interfaces_geneve.pysrc/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_macsec.pysrc/conf_mode/interfaces_openvpn.pysrc/conf_mode/interfaces_pppoe.pysrc/conf_mode/interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_sstpc.pysrc/conf_mode/interfaces_tunnel.pysrc/conf_mode/interfaces_virtual-ethernet.pysrc/conf_mode/interfaces_vti.pysrc/conf_mode/interfaces_vxlan.pysrc/conf_mode/interfaces_wireguard.pysrc/conf_mode/interfaces_wireless.pysrc/conf_mode/interfaces_wwan.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_iso
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python code must use Python 3.11 or higher
Files:
src/conf_mode/interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_virtual-ethernet.pysrc/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_ethernet.pysrc/conf_mode/interfaces_geneve.pysrc/conf_mode/firewall.pysrc/conf_mode/interfaces_vxlan.pysrc/conf_mode/interfaces_dummy.pysrc/conf_mode/interfaces_wwan.pysrc/conf_mode/interfaces_openvpn.pysrc/conf_mode/interfaces_sstpc.pysrc/conf_mode/interfaces_wireguard.pysrc/conf_mode/interfaces_macsec.pysrc/conf_mode/interfaces_pppoe.pysrc/conf_mode/interfaces_tunnel.pysrc/conf_mode/interfaces_vti.pysrc/conf_mode/interfaces_bridge.pysmoketest/scripts/cli/test_firewall.pysrc/conf_mode/interfaces_wireless.pysrc/conf_mode/interfaces_bonding.py
src/conf_mode/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in src/conf_mode/
Files:
src/conf_mode/interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_virtual-ethernet.pysrc/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_ethernet.pysrc/conf_mode/interfaces_geneve.pysrc/conf_mode/firewall.pysrc/conf_mode/interfaces_vxlan.pysrc/conf_mode/interfaces_dummy.pysrc/conf_mode/interfaces_wwan.pysrc/conf_mode/interfaces_openvpn.pysrc/conf_mode/interfaces_sstpc.pysrc/conf_mode/interfaces_wireguard.pysrc/conf_mode/interfaces_macsec.pysrc/conf_mode/interfaces_pppoe.pysrc/conf_mode/interfaces_tunnel.pysrc/conf_mode/interfaces_vti.pysrc/conf_mode/interfaces_bridge.pysrc/conf_mode/interfaces_wireless.pysrc/conf_mode/interfaces_bonding.py
{**/*.py,data/templates/**/*}
📄 CodeRabbit inference engine (AGENTS.md)
Linting must use ruff 0.6.4, darker, pylint W0611, and Jinja2 lint as enforced by vyos/.github reusables
Files:
src/conf_mode/interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_virtual-ethernet.pysrc/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_ethernet.pysrc/conf_mode/interfaces_geneve.pysrc/conf_mode/firewall.pysrc/conf_mode/interfaces_vxlan.pydata/templates/firewall/nftables-zone.j2src/conf_mode/interfaces_dummy.pysrc/conf_mode/interfaces_wwan.pysrc/conf_mode/interfaces_openvpn.pysrc/conf_mode/interfaces_sstpc.pysrc/conf_mode/interfaces_wireguard.pysrc/conf_mode/interfaces_macsec.pysrc/conf_mode/interfaces_pppoe.pysrc/conf_mode/interfaces_tunnel.pysrc/conf_mode/interfaces_vti.pysrc/conf_mode/interfaces_bridge.pysmoketest/scripts/cli/test_firewall.pysrc/conf_mode/interfaces_wireless.pysrc/conf_mode/interfaces_bonding.py
data/templates/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Jinja2 templates should be stored as discrete files under data/templates/ rather than as inline Python strings
Files:
data/templates/firewall/nftables-zone.j2
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Smoketest scripts must be nose2 compatible and located in smoketest/
Files:
smoketest/scripts/cli/test_firewall.py
🧠 Learnings (2)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/cli/test_firewall.py
📚 Learning: 2026-05-26T06:04:29.163Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:118-120
Timestamp: 2026-05-26T06:04:29.163Z
Learning: In VyOS smoketest scripts under `smoketest/scripts/cli/`, it is intentional to call `self.cli_delete(['vrf'])` in both `setUpClass` and `tearDown` to wipe the entire VRF subtree and ensure a clean slate. During code review, do not recommend narrowing the delete to specific VRF identifiers or name subsets (e.g., `['vrf', 'name', 'mgmt']`)—the broad teardown behavior is the established project-wide pattern for these tests.
Applied to files:
smoketest/scripts/cli/test_firewall.py
🔍 Remote MCP
Additional Context for PR Review
nftables oifname Syntax and Multiple Interface Handling
nftables supports anonymous sets where members are enclosed in curly braces with commas separating elements. This is the correct way to match multiple interface names. The expected syntax for multiple interfaces should be:
oifname { "eth0", "eth1", "vti1" }
However, the PR currently uses a quoted comma-separated string approach (e.g., oifname "eth0,eth1") which treats the entire string as a single interface name, not as a set. This is the root cause of issue #1 reported by AnDroed09.
nftables String Length Limitation
Interface names that exceed 16 characters in quoted strings cause the error "String exceeds maximum length of 16" in nftables. This directly corresponds to issue #3 in the PR comments, where combining multiple interface names with commas in a quoted string exceeds the 16-character limit for a single quoted string. When the PR uses quoted_join(",") to combine interface names, the result becomes a single quoted string that can easily exceed this limit.
VyOS VRF and Firewall Architecture
VyOS implements VRF-aware connection tracking using nftables, allowing stateful firewall rules to work correctly across VRF boundaries. VyOS firewall zones can contain interfaces or VRFs, and policies define how traffic is handled between zones.
Interface Name Handling in nftables
nftables can only use real interface names in iifname and oifname parameters, and alternative names (altname) are not supported. This means the generated rules must use actual interface names as they appear on the system.
Key Issues Summary
-
Set vs. String Syntax: The PR needs to emit nftables set syntax
{ "iface1", "iface2" }instead of quoted strings"iface1,iface2"for multiple interfaces. -
16-Character Limit: Combining interface names in a single quoted string bypasses the proper set-based syntax and triggers the 16-character limit when names are too long.
-
Dependency Tracking: The extensive changes across multiple interface modules (bonding, bridge, dummy, ethernet, etc.) to add firewall dependency triggers for VRF changes are crucial for ensuring that firewall rules are regenerated when interfaces are added to, removed from, or reassigned within VRFs.
-
VRF Member Resolution: The PR changes
src/conf_mode/firewall.pyto store the direct return value fromget_vrf_members()instead of converting it to a comma-delimited string, suggesting the template should handle list/array data rather than pre-joined strings.
[::web_search::]
🔇 Additional comments (14)
data/templates/firewall/nftables-zone.j2 (2)
98-99: Fix unresolveddefault_localVRF rule placement/accessor bug.Line 98 and Line 99 are still outside the VRF guard and still reference
from_conf.firewall[fw_name]in thedefault_localbranch, which should usefrom_conf[fw_name].
15-17: LGTM!Also applies to: 77-80, 94-95
src/conf_mode/firewall.py (1)
141-141: LGTM!smoketest/scripts/cli/test_firewall.py (1)
1146-1147: LGTM!Also applies to: 1149-1150, 1160-1163, 1197-1200, 1212-1213, 1227-1243
src/conf_mode/interfaces_bridge.py (1)
241-241: Unresolved from earlier review: VRF detach can still skip dependent execution.Line 241 still checks
'vrf' in bridge; when VRF is removed, that key is absent even though Line 134 can queuefirewallviais_node_changed(...).src/conf_mode/interfaces_pseudo-ethernet.py (1)
69-71: VRF detach path still skips queued firewall dependent execution.Line 70 correctly queues firewall dependent updates on VRF node changes, but Line 108 only executes dependents when
'vrf' in peth. On VRF removal/delete, the key is absent, so queued firewall updates can be missed.Also applies to: 107-109
src/conf_mode/interfaces_sstpc.py (1)
63-64: VRF unset/delete still has a dependent-execution hole.Line 63 registers firewall dependents for VRF changes, but delete/disable returns at Lines 111-116 before dependents run, and Line 138 also requires
'vrf' in sstpc. VRF removals can leave firewall updates unapplied.Also applies to: 111-116, 137-139
src/conf_mode/interfaces_tunnel.py (1)
84-85: Dependent trigger still misses VRF removal/delete transitions.Line 84 queues firewall dependents on VRF changes, but Line 227 can return before execution and Line 233 requires
'vrf' in tunnel. VRF detach/delete paths can still skip firewall dependent application.Also applies to: 226-227, 233-234
src/conf_mode/interfaces_virtual-ethernet.py (1)
59-60: State-basedvrfpresence check still drops VRF-unset updates.Line 59 detects VRF node changes, but Line 117 executes dependents based on
'vrf' in veth. When VRF is removed, the key is absent and queued firewall updates can be skipped.Also applies to: 117-118
src/conf_mode/interfaces_vti.py (1)
45-46: Delete/unset VRF path still bypasses firewall dependent execution.Line 45 queues firewall dependents on VRF changes, but Lines 61-63 return before dependent execution and Line 69 requires
'vrf' in vti. VRF removals can still miss firewall refresh.Also applies to: 61-63, 69-70
src/conf_mode/interfaces_wireless.py (1)
150-151: VRF removal/deletion path still has a firewall dependent gap.Line 150 registers the firewall dependent correctly, but Lines 324-327 can return before dependents run, and Line 402 still depends on
'vrf' in wifi. VRF-unset/delete updates can be missed.Also applies to: 324-327, 402-403
src/conf_mode/interfaces_wwan.py (1)
97-98: VRF-detach dependent execution is still incomplete.Line 97 queues firewall dependents for VRF changes, but delete/disable exits at Line 171 and Line 194 requires
'vrf' in wwan. VRF removal paths can still leave firewall updates unapplied.Also applies to: 159-171, 194-195
src/conf_mode/interfaces_wireguard.py (1)
89-91: LGTM!src/conf_mode/interfaces_bonding.py (1)
173-176: LGTM!
davi2367
force-pushed
the
zbf-vrf
branch
3 times, most recently
from
f7bea90 to
f478ad1
Compare
|
The main fix works: when a zone has a VRF member, nft now generates correct there is a small issue with VIF subinterface handling. VIF subinterface VRF membership changes do not update nftables When a VLAN subinterface is added to or removed from a VRF without touching Initial setup rules are correct: Add a new VIF member after initial commit eth2.100 does not appear in nft: Same for Remove affected member keep stay in nft (eth2.200) Changing a top-level interface VRF membership trigger nft regeneration, |
|
Actionable comments posted: 0 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/conf_mode/interfaces_bonding.py`:
- Around line 174-188: In get_config() where VRF membership changes call
set_dependents('firewall', conf) (the blocks using is_node_changed and
conf.list_nodes for base + [ifname, 'vrf'], 'vif', 'vif-s', etc.), also set a
vrf_changed flag on the enclosing resource dict (e.g., mark bond['vrf_changed']
= True for bonding and bridge['vrf_changed'] = True for bridging) so that
apply()'s conditional that checks for 'vrf_changed' in bond/bridge will run
call_dependents(); ensure you set the marker in every place that currently
queues set_dependents('firewall', conf) (including inner vif and vif-c loops) so
VRF-only updates trigger call_dependents().
In `@src/conf_mode/interfaces_bridge.py`:
- Around line 133-140: The bridge VRF-change branch queues firewall dependents
but never sets the bridge-level flag that apply() checks; update the VRF-change
detection in interfaces_bridge.py (the is_node_changed checks for base +
[ifname,'vrf'] and base + [ifname,'vif',vif,'vrf']) to also set
bridge['vrf_changed'] = True (or call the same setter used elsewhere) when a VRF
change is detected so call_dependents() will run; make the analogous change in
interfaces_bonding.py for bond['vrf_changed'] when its VRF is changed to ensure
firewall re-application is triggered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 60cfdc02-8b5a-4759-90d8-0cc40009c57d
📒 Files selected for processing (22)
data/config-mode-dependencies/vyos-1x.jsondata/templates/firewall/nftables-zone.j2smoketest/scripts/cli/test_firewall.pysrc/conf_mode/firewall.pysrc/conf_mode/interfaces_bonding.pysrc/conf_mode/interfaces_bridge.pysrc/conf_mode/interfaces_dummy.pysrc/conf_mode/interfaces_ethernet.pysrc/conf_mode/interfaces_geneve.pysrc/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_macsec.pysrc/conf_mode/interfaces_openvpn.pysrc/conf_mode/interfaces_pppoe.pysrc/conf_mode/interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_sstpc.pysrc/conf_mode/interfaces_tunnel.pysrc/conf_mode/interfaces_virtual-ethernet.pysrc/conf_mode/interfaces_vti.pysrc/conf_mode/interfaces_vxlan.pysrc/conf_mode/interfaces_wireguard.pysrc/conf_mode/interfaces_wireless.pysrc/conf_mode/interfaces_wwan.py
🚧 Files skipped from review as they are similar to previous changes (18)
- src/conf_mode/interfaces_wwan.py
- src/conf_mode/interfaces_dummy.py
- src/conf_mode/interfaces_wireguard.py
- src/conf_mode/interfaces_geneve.py
- src/conf_mode/interfaces_virtual-ethernet.py
- src/conf_mode/interfaces_vti.py
- src/conf_mode/interfaces_tunnel.py
- src/conf_mode/interfaces_l2tpv3.py
- src/conf_mode/firewall.py
- data/config-mode-dependencies/vyos-1x.json
- src/conf_mode/interfaces_wireless.py
- src/conf_mode/interfaces_ethernet.py
- src/conf_mode/interfaces_pppoe.py
- src/conf_mode/interfaces_openvpn.py
- src/conf_mode/interfaces_macsec.py
- src/conf_mode/interfaces_sstpc.py
- data/templates/firewall/nftables-zone.j2
- smoketest/scripts/cli/test_firewall.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python code must use Python 3.11 or higher
Files:
src/conf_mode/interfaces_bonding.pysrc/conf_mode/interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_bridge.pysrc/conf_mode/interfaces_vxlan.py
src/conf_mode/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and placed in
src/conf_mode/
Files:
src/conf_mode/interfaces_bonding.pysrc/conf_mode/interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_bridge.pysrc/conf_mode/interfaces_vxlan.py
🔍 Remote MCP
Based on the information gathered, I now have enough context to provide a comprehensive summary for the PR review. Let me compile the relevant findings.
Pull Request Review Context: Firewall VRF Interface Names
Key Technical Context
nftables oifname syntax and VRF expectations:
In nftables, multiple interfaces can be matched using set notation: oifname { "eth1", "eth2" }. This PR addresses the proper generation of this syntax for VRF member interfaces. nftables only supports real interface names in oifname parameters, which is critical for understanding why individual interface names must be emitted rather than combined strings.
VRF and firewall integration in VyOS:
VyOS implements VRF-aware connection tracking using nftables, allowing stateful firewall rules to work correctly across VRF boundaries. A VRF device is created with an associated route table, and network interfaces are then enslaved to a VRF device. The firewall rules must properly reference these member interfaces in the outbound rules.
PR Issues and Root Causes
Reported problems addressed by this PR:
-
Malformed oifname syntax: Earlier versions were generating
oifname "eth1,eth2"(single quoted string) instead of proper set notationoifname { "eth1", "eth2" } -
String length errors: When combined interface names exceeded 16 characters, nftables would fail validation because the entire comma-separated string was being treated as a single interface name
-
Stale entries and missing updates: When interfaces were added or removed from a VRF, firewall rules were not being regenerated because interface config modules weren't registering firewall as a dependent
-
VIF subinterface handling: Changes to VLAN subinterfaces (vif) under a VRF-assigned interface don't trigger firewall regeneration without a top-level interface VRF change
Solution Architecture
The PR implements three coordinated changes:
-
Template changes (
nftables-zone.j2): Converts from combined string emission to per-interface oifname iteration usingquoted_jointo properly format multiple interfaces as an nftables set -
Config module updates: Adds VRF change detection to 15+ interface config modules (
interfaces_ethernet.py,interfaces_bonding.py, etc.) viais_node_changed(['vrf'])to trigger firewall updates when VRF membership changes -
Dependency registration: Registers
firewallas a dependent viaset_dependents('firewall', conf)when VRF changes are detected, and callscall_dependents()unconditionally in theapply()phase
[::web_search::]
🔇 Additional comments (2)
src/conf_mode/interfaces_pseudo-ethernet.py (1)
69-83: LGTM!Also applies to: 119-120
src/conf_mode/interfaces_vxlan.py (1)
92-94: LGTM!Also applies to: 264-265
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/conf_mode/interfaces_pppoe.py (1)
123-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelete/disable path returns before
call_dependents()— stale firewall on VRF detach.
get_config()(Line 78) registers thefirewalldependent when thevrfnode changes, including detach/delete. But this branch returns at Line 128 before thecall_dependents()at Line 150, so removing/disabling a PPPoE interface that was a VRF member never regenerates nftables → staleoifnameentries. The equivalent macsec path was fixed (call in the deletion branch); pppoe was missed.🐛 Proposed fix
if 'deleted' in pppoe or 'disable' in pppoe: if os.path.isdir(f'/sys/class/net/{ifname}'): p = PPPoEIf(ifname) p.remove() call(f'systemctl stop ppp@{ifname}.service') + # run the dependents to refresh firewall on VRF detach/removal + call_dependents() return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/conf_mode/interfaces_pppoe.py` around lines 123 - 128, The delete/disable branch for PPPoE (the if 'deleted' in pppoe or 'disable' in pppoe block that creates PPPoEIf(ifname), calls p.remove() and systemctl stop) returns before call_dependents() runs, so VRF detach doesn't trigger the registered firewall dependent from get_config(); move or add a call_dependents() invocation (the same pattern used in the macsec deletion fix) into this PPPoE deletion path before the function returns so nftables regeneration will run and stale oifname entries are removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/conf_mode/interfaces_pppoe.py`:
- Around line 123-128: The delete/disable branch for PPPoE (the if 'deleted' in
pppoe or 'disable' in pppoe block that creates PPPoEIf(ifname), calls p.remove()
and systemctl stop) returns before call_dependents() runs, so VRF detach doesn't
trigger the registered firewall dependent from get_config(); move or add a
call_dependents() invocation (the same pattern used in the macsec deletion fix)
into this PPPoE deletion path before the function returns so nftables
regeneration will run and stale oifname entries are removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4901cce5-5ef4-4133-8e11-55001d2fb9af
📒 Files selected for processing (22)
data/config-mode-dependencies/vyos-1x.jsondata/templates/firewall/nftables-zone.j2smoketest/scripts/cli/test_firewall.pysrc/conf_mode/firewall.pysrc/conf_mode/interfaces_bonding.pysrc/conf_mode/interfaces_bridge.pysrc/conf_mode/interfaces_dummy.pysrc/conf_mode/interfaces_ethernet.pysrc/conf_mode/interfaces_geneve.pysrc/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_macsec.pysrc/conf_mode/interfaces_openvpn.pysrc/conf_mode/interfaces_pppoe.pysrc/conf_mode/interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_sstpc.pysrc/conf_mode/interfaces_tunnel.pysrc/conf_mode/interfaces_virtual-ethernet.pysrc/conf_mode/interfaces_vti.pysrc/conf_mode/interfaces_vxlan.pysrc/conf_mode/interfaces_wireguard.pysrc/conf_mode/interfaces_wireless.pysrc/conf_mode/interfaces_wwan.py
🚧 Files skipped from review as they are similar to previous changes (20)
- src/conf_mode/interfaces_dummy.py
- src/conf_mode/interfaces_virtual-ethernet.py
- src/conf_mode/interfaces_geneve.py
- src/conf_mode/interfaces_pseudo-ethernet.py
- data/config-mode-dependencies/vyos-1x.json
- src/conf_mode/interfaces_vti.py
- src/conf_mode/interfaces_ethernet.py
- src/conf_mode/interfaces_l2tpv3.py
- src/conf_mode/interfaces_wireguard.py
- src/conf_mode/firewall.py
- src/conf_mode/interfaces_vxlan.py
- src/conf_mode/interfaces_openvpn.py
- src/conf_mode/interfaces_wwan.py
- src/conf_mode/interfaces_bridge.py
- src/conf_mode/interfaces_wireless.py
- src/conf_mode/interfaces_sstpc.py
- src/conf_mode/interfaces_bonding.py
- data/templates/firewall/nftables-zone.j2
- smoketest/scripts/cli/test_firewall.py
- src/conf_mode/interfaces_tunnel.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_iso
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python code must use Python 3.11 or higher
Files:
src/conf_mode/interfaces_macsec.pysrc/conf_mode/interfaces_pppoe.py
src/conf_mode/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and placed in
src/conf_mode/
Files:
src/conf_mode/interfaces_macsec.pysrc/conf_mode/interfaces_pppoe.py
🔇 Additional comments (1)
src/conf_mode/interfaces_macsec.py (1)
87-89: LGTM!Also applies to: 194-197, 209-210
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/conf_mode/interfaces_wireguard.py (1)
89-91: 💤 Low valueFirewall dependent registration is correct and
call_dependents()at Line 213 already fires unconditionally, so VRF changes will trigger a firewall rebuild.Minor: the
vrfnode is now evaluated twice — once in the fwmark/VRF block (Lines 79-81) and again here. The two serve different purposes (ip-rule teardown vs. firewall dependent), so this is fine as-is; you could fold theset_dependents('firewall', conf)into a single VRF-change check if you want to avoid the duplicateis_node_changedlookup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/conf_mode/interfaces_wireguard.py` around lines 89 - 91, The vrf node is being checked twice with is_node_changed (once in the fwmark/VRF block around the ip-rule teardown and again before set_dependents('firewall', conf)); consolidate these into a single vrf-change check to avoid duplicate lookups by computing the vrf_changed boolean once (reuse the same result in both places) and call set_dependents('firewall', conf) only when that vrf_changed is true; keep the separate behaviors (ip-rule teardown logic and calling set_dependents) but use the shared vrf_changed variable instead of calling is_node_changed twice, referencing the existing is_node_changed and set_dependents symbols and ensuring call_dependents() still runs unconditionally as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/conf_mode/interfaces_wireguard.py`:
- Around line 89-91: The vrf node is being checked twice with is_node_changed
(once in the fwmark/VRF block around the ip-rule teardown and again before
set_dependents('firewall', conf)); consolidate these into a single vrf-change
check to avoid duplicate lookups by computing the vrf_changed boolean once
(reuse the same result in both places) and call set_dependents('firewall', conf)
only when that vrf_changed is true; keep the separate behaviors (ip-rule
teardown logic and calling set_dependents) but use the shared vrf_changed
variable instead of calling is_node_changed twice, referencing the existing
is_node_changed and set_dependents symbols and ensuring call_dependents() still
runs unconditionally as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e5b825c8-1fbf-4703-8022-f110f41bdc29
📒 Files selected for processing (22)
data/config-mode-dependencies/vyos-1x.jsondata/templates/firewall/nftables-zone.j2smoketest/scripts/cli/test_firewall.pysrc/conf_mode/firewall.pysrc/conf_mode/interfaces_bonding.pysrc/conf_mode/interfaces_bridge.pysrc/conf_mode/interfaces_dummy.pysrc/conf_mode/interfaces_ethernet.pysrc/conf_mode/interfaces_geneve.pysrc/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_macsec.pysrc/conf_mode/interfaces_openvpn.pysrc/conf_mode/interfaces_pppoe.pysrc/conf_mode/interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_sstpc.pysrc/conf_mode/interfaces_tunnel.pysrc/conf_mode/interfaces_virtual-ethernet.pysrc/conf_mode/interfaces_vti.pysrc/conf_mode/interfaces_vxlan.pysrc/conf_mode/interfaces_wireguard.pysrc/conf_mode/interfaces_wireless.pysrc/conf_mode/interfaces_wwan.py
🚧 Files skipped from review as they are similar to previous changes (19)
- src/conf_mode/interfaces_pseudo-ethernet.py
- src/conf_mode/interfaces_geneve.py
- src/conf_mode/interfaces_vti.py
- src/conf_mode/interfaces_l2tpv3.py
- src/conf_mode/interfaces_macsec.py
- src/conf_mode/interfaces_wwan.py
- src/conf_mode/interfaces_tunnel.py
- data/templates/firewall/nftables-zone.j2
- src/conf_mode/interfaces_virtual-ethernet.py
- src/conf_mode/interfaces_vxlan.py
- data/config-mode-dependencies/vyos-1x.json
- src/conf_mode/interfaces_bridge.py
- src/conf_mode/interfaces_sstpc.py
- src/conf_mode/interfaces_bonding.py
- src/conf_mode/interfaces_wireless.py
- src/conf_mode/interfaces_dummy.py
- src/conf_mode/interfaces_ethernet.py
- src/conf_mode/interfaces_openvpn.py
- smoketest/scripts/cli/test_firewall.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_iso
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python code must use Python 3.11 or higher
Files:
src/conf_mode/interfaces_pppoe.pysrc/conf_mode/interfaces_wireguard.pysrc/conf_mode/firewall.py
src/conf_mode/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and placed in
src/conf_mode/
Files:
src/conf_mode/interfaces_pppoe.pysrc/conf_mode/interfaces_wireguard.pysrc/conf_mode/firewall.py
🔍 Remote MCP
I'll search for relevant technical documentation to supplement this pull request review context.## Summary of Relevant Context for PR #5167 Review
Based on my research, here's the additional context that would help in reviewing this pull request:
nftables oifname Set Syntax Validation
In nftables, anonymous sets (sets with no specific name) have their members enclosed in curly braces with commas separating elements when creating the rule, and once the rule is removed, the set is removed. Anonymous sets cannot be updated once declared—they can only be changed by removing or altering the rule that uses them.
The PR's expected output format (oifname { "eth1", "eth2" } as a set) aligns with proper nftables syntax, which is critical since the original issue reported involved incorrect rendering as a single comma-separated string (oifname "eth1,eth2").
VyOS Zone-Based Firewall with VRF Context
Zone-based firewall is an alternative to applying policy to an interface directly, allowing multiple interfaces to belong to the same security zone by applying rule-sets to source-destination zone pairs rather than to individual interfaces. This PR addresses a critical gap in how VRF-member interfaces are rendered in the nftables zone rules.
Previously, all traffic originating from an interface attached to a VRF would show the source interface as the VRF master interface, making it difficult to segment traffic between interfaces attached to the same VRF. This PR attempts to fix that by explicitly including both the per-interface oifname entries and the VRF name itself.
Configuration Dependency Management
The PR makes extensive changes to interface configuration modules (bonding, bridge, dummy, ethernet, etc.) to trigger firewall updates via the dependency system when VRF membership changes. The logic for Zone Based firewall had remained unchanged despite VRF introduction, necessitating improvements for better config parsing when using VRFs. This dependency-based approach should help address the reported issues where interface changes (adding/removing VLANs or changing VRF memberships) don't immediately trigger nftables regeneration.
Key Review Considerations
-
nftables Syntax Correctness: The PR uses
quoted_join(",")to render multiple interface names as proper nftables sets, which is the correct approach and should fix the 16-character length error reported. -
Dependency Triggering: The addition of
set_dependents('firewall', conf)across multiple interface modules whenvrfchanges is the primary mechanism to address the issue where VLAN additions/removals don't update nftables rules. -
Data Type Change: In
src/conf_mode/firewall.py,vrf_interfaces[vrf_name]is now set to the direct return value ofget_vrf_members()(a list) rather than a comma-delimited string, which enables proper set rendering in the template.
🔇 Additional comments (3)
src/conf_mode/interfaces_pppoe.py (2)
78-79: VRF-change detection wired correctly.is_node_changedcovers add/change/delete, so detach registers the firewall dependent too.This is the same area flagged previously; the delete/disable early-exit now also runs
call_dependents()(Lines 128-131), so the detach gap is closed.
121-155:call_dependents()is now invoked in both the delete/disable branch (Lines 128-131) and the main path (Lines 152-153). It's a no-op when no dependent was registered, so the unconditional call is safe.LGTM!
src/conf_mode/firewall.py (1)
138-144: Storing the rawget_vrf_members()list is consistent downstream:verify()line 605if not vrf_interfacesstill catches empty membership (empty list is falsy), and the per-interface set rendering relies on the list shape. This is the correct fix for the prioroifname "eth1,eth2"/ 16-char overflow.LGTM!
|
@davi2367 could you rebase and force push ? |
…l config, aswell as dependents
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/conf_mode/interfaces_openvpn.py (2)
864-865:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFirewall dependents never execute for deleted or disabled OpenVPN interfaces.
Line 842 returns early for deleted/disabled paths before reaching
call_dependents()at line 865. Combined with the get_config issue (lines 186-188), deleted VRF-member OpenVPN interfaces leave stale nftables oifname entries.🔧 Call dependents before early return
if 'deleted' in openvpn or 'disable' in openvpn: call(f'systemctl stop openvpn@{interface}.service') for cleanup_file in glob(f'/run/openvpn/{interface}.*'): if os.path.isfile(cleanup_file): os.unlink(cleanup_file) if interface_exists(interface): VTunIf(interface).remove() + + # run the dependents + call_dependents() return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/conf_mode/interfaces_openvpn.py` around lines 864 - 865, The early return that exits when an OpenVPN interface is deleted/disabled prevents dependents from running, leaving stale nftables oifname entries; move the call to call_dependents() so it executes before that early return (i.e., ensure call_dependents() is invoked for deleted/disabled paths inside the same function where the early return occurs) and adjust any logic that relies on get_config (see get_config usage around lines ~186-188) so dependents receive the correct pre-deletion config state; in short: invoke call_dependents() prior to returning for deleted/disabled interfaces and ensure get_config supplies the necessary data for those dependent calls.
186-188:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftVRF change detection bypassed when OpenVPN interface is deleted.
Line 119 returns early for deleted interfaces before reaching the VRF detection at line 186. When an OpenVPN interface with VRF membership is deleted,
set_dependents('firewall', conf)is never called, so the firewall config won't regenerate to remove the interface from VRF zone rules.Per learning,
is_node_changed(conf, base + [ifname, 'vrf'])correctly detects deletion, but only if called.🔧 Move VRF detection before early return
ifname, openvpn = get_interface_dict(conf, base, with_pki=True) openvpn['auth_user_pass_file'] = '/run/openvpn/{ifname}.pw'.format(**openvpn) + # Check vrf membership, to ensure firewall is updated + if is_node_changed(conf, base + [ifname, 'vrf']): + set_dependents('firewall', conf) + if 'deleted' in openvpn: return openvpnThen remove the duplicate check at lines 186-188.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/conf_mode/interfaces_openvpn.py` around lines 186 - 188, The VRF-change detection must run before the early return that skips deleted OpenVPN interfaces so deletions trigger firewall regeneration: move the is_node_changed(conf, base + [ifname, 'vrf']) check (and its set_dependents('firewall', conf) call) to execute before the early-return block around line 119 (the block that returns for deleted interfaces), and then remove the duplicate is_node_changed(...)/set_dependents('firewall', conf) at lines 186-188; use the same symbols is_node_changed and set_dependents to locate and update the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/conf_mode/interfaces_openvpn.py`:
- Around line 864-865: The early return that exits when an OpenVPN interface is
deleted/disabled prevents dependents from running, leaving stale nftables
oifname entries; move the call to call_dependents() so it executes before that
early return (i.e., ensure call_dependents() is invoked for deleted/disabled
paths inside the same function where the early return occurs) and adjust any
logic that relies on get_config (see get_config usage around lines ~186-188) so
dependents receive the correct pre-deletion config state; in short: invoke
call_dependents() prior to returning for deleted/disabled interfaces and ensure
get_config supplies the necessary data for those dependent calls.
- Around line 186-188: The VRF-change detection must run before the early return
that skips deleted OpenVPN interfaces so deletions trigger firewall
regeneration: move the is_node_changed(conf, base + [ifname, 'vrf']) check (and
its set_dependents('firewall', conf) call) to execute before the early-return
block around line 119 (the block that returns for deleted interfaces), and then
remove the duplicate is_node_changed(...)/set_dependents('firewall', conf) at
lines 186-188; use the same symbols is_node_changed and set_dependents to locate
and update the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 650fe3fa-2db2-4947-bab7-3ec87d472e0a
📒 Files selected for processing (22)
data/config-mode-dependencies/vyos-1x.jsondata/templates/firewall/nftables-zone.j2smoketest/scripts/cli/test_firewall.pysrc/conf_mode/firewall.pysrc/conf_mode/interfaces_bonding.pysrc/conf_mode/interfaces_bridge.pysrc/conf_mode/interfaces_dummy.pysrc/conf_mode/interfaces_ethernet.pysrc/conf_mode/interfaces_geneve.pysrc/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_macsec.pysrc/conf_mode/interfaces_openvpn.pysrc/conf_mode/interfaces_pppoe.pysrc/conf_mode/interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_sstpc.pysrc/conf_mode/interfaces_tunnel.pysrc/conf_mode/interfaces_virtual-ethernet.pysrc/conf_mode/interfaces_vti.pysrc/conf_mode/interfaces_vxlan.pysrc/conf_mode/interfaces_wireguard.pysrc/conf_mode/interfaces_wireless.pysrc/conf_mode/interfaces_wwan.py
🚧 Files skipped from review as they are similar to previous changes (20)
- src/conf_mode/firewall.py
- src/conf_mode/interfaces_pseudo-ethernet.py
- src/conf_mode/interfaces_ethernet.py
- src/conf_mode/interfaces_wireless.py
- data/config-mode-dependencies/vyos-1x.json
- src/conf_mode/interfaces_dummy.py
- src/conf_mode/interfaces_tunnel.py
- src/conf_mode/interfaces_virtual-ethernet.py
- src/conf_mode/interfaces_geneve.py
- src/conf_mode/interfaces_pppoe.py
- src/conf_mode/interfaces_bonding.py
- src/conf_mode/interfaces_bridge.py
- src/conf_mode/interfaces_vti.py
- src/conf_mode/interfaces_vxlan.py
- data/templates/firewall/nftables-zone.j2
- src/conf_mode/interfaces_wwan.py
- src/conf_mode/interfaces_sstpc.py
- src/conf_mode/interfaces_wireguard.py
- src/conf_mode/interfaces_macsec.py
- smoketest/scripts/cli/test_firewall.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_iso
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must target Python 3.11 or higher
Prefer storing Jinja2 templates as discrete files underdata/templates/rather than inline Python strings
Use ruff (version 0.6.4), darker, pylint W0611, and Jinja2 lint for linting; configuration viaruff.tomlandnose2.cfgat repository root
Files:
src/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_openvpn.py
src/conf_mode/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and placed in
src/conf_mode/
Files:
src/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_openvpn.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: vyos/vyos-1x PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-01T00:03:28.710Z
Learning: Commit and PR titles must follow the format: `component: T1234: description` with Phorge IDs from https://vyos.dev
Learnt from: natali-rs1985
Repo: vyos/vyos-1x PR: 5175
File: src/conf_mode/interfaces_wireguard.py:79-87
Timestamp: 2026-05-11T09:59:48.690Z
Learning: In VyOS `interfaces_wireguard.py` (`src/conf_mode/interfaces_wireguard.py`), when a WireGuard interface is deleted, `is_node_changed(conf, base + [ifname, 'fwmark'])` and `is_node_changed(conf, base + [ifname, 'vrf'])` still return `True` for any child nodes that existed in the running config, because VyOS's `DiffTree` uses a recursive diff algorithm that enumerates every previously configured leaf individually in the `sub` subtree when a parent node is deleted. Therefore, `prev_fwmark` and `prev_vrf` are correctly captured on interface deletion without needing a separate `'deleted' in wireguard` branch.
Learnt from: natali-rs1985
Repo: vyos/vyos-1x PR: 5232
File: src/conf_mode/vrf.py:268-273
Timestamp: 2026-05-28T12:37:18.313Z
Learning: In iproute2 and the Linux kernel (net/core/fib_rules.c, net/ipv4/fib_rules.c), `ip rule del` uses partial/filter matching for RTM_DELRULE: only attributes explicitly included in the deletion request are checked. Omitting `fwmark` from `ip rule del pref 1998 table {table_id}` does NOT prevent matching rules that carry a fwmark selector — the kernel's compare callback skips the fwmark check entirely when FRA_FWMARK is absent from the netlink message. So `ip {afi} rule del pref 1998 table {table_id}` correctly removes WireGuard fwmark rules in vyos-1x src/conf_mode/vrf.py.
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:118-120
Timestamp: 2026-05-26T06:04:35.545Z
Learning: In VyOS smoketests (`smoketest/scripts/cli/`), calling `self.cli_delete(['vrf'])` in both `setUpClass` and `tearDown` to wipe the entire VRF subtree is intentional and the established project-wide pattern. Do not suggest scoping the delete to individual VRF names (e.g., `['vrf', 'name', 'mgmt']`); the broad teardown is by design to ensure a clean slate.
📚 Learning: 2026-05-11T09:59:48.690Z
Learnt from: natali-rs1985
Repo: vyos/vyos-1x PR: 5175
File: src/conf_mode/interfaces_wireguard.py:79-87
Timestamp: 2026-05-11T09:59:48.690Z
Learning: In VyOS `interfaces_wireguard.py` (`src/conf_mode/interfaces_wireguard.py`), when a WireGuard interface is deleted, `is_node_changed(conf, base + [ifname, 'fwmark'])` and `is_node_changed(conf, base + [ifname, 'vrf'])` still return `True` for any child nodes that existed in the running config, because VyOS's `DiffTree` uses a recursive diff algorithm that enumerates every previously configured leaf individually in the `sub` subtree when a parent node is deleted. Therefore, `prev_fwmark` and `prev_vrf` are correctly captured on interface deletion without needing a separate `'deleted' in wireguard` branch.
Applied to files:
src/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_openvpn.py
📚 Learning: 2026-05-28T12:37:18.313Z
Learnt from: natali-rs1985
Repo: vyos/vyos-1x PR: 5232
File: src/conf_mode/vrf.py:268-273
Timestamp: 2026-05-28T12:37:18.313Z
Learning: In iproute2 and the Linux kernel (net/core/fib_rules.c, net/ipv4/fib_rules.c), `ip rule del` uses partial/filter matching for RTM_DELRULE: only attributes explicitly included in the deletion request are checked. Omitting `fwmark` from `ip rule del pref 1998 table {table_id}` does NOT prevent matching rules that carry a fwmark selector — the kernel's compare callback skips the fwmark check entirely when FRA_FWMARK is absent from the netlink message. So `ip {afi} rule del pref 1998 table {table_id}` correctly removes WireGuard fwmark rules in vyos-1x src/conf_mode/vrf.py.
Applied to files:
src/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_openvpn.py
📚 Learning: 2026-05-26T06:04:35.545Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:118-120
Timestamp: 2026-05-26T06:04:35.545Z
Learning: In VyOS smoketests (`smoketest/scripts/cli/`), calling `self.cli_delete(['vrf'])` in both `setUpClass` and `tearDown` to wipe the entire VRF subtree is intentional and the established project-wide pattern. Do not suggest scoping the delete to individual VRF names (e.g., `['vrf', 'name', 'mgmt']`); the broad teardown is by design to ensure a clean slate.
Applied to files:
src/conf_mode/interfaces_l2tpv3.pysrc/conf_mode/interfaces_openvpn.py
📚 Learning: 2026-05-14T12:04:13.809Z
Learnt from: alexandr-san4ez
Repo: vyos/vyos-1x PR: 5197
File: src/conf_mode/vpn_ipsec.py:718-723
Timestamp: 2026-05-14T12:04:13.809Z
Learning: In `src/conf_mode/vpn_ipsec.py` (VyOS vyos-1x, IPsec site-to-site), the `ikev2_reauth` validation intentionally requires the peer to explicitly set `ikev2-reauth inherit` to inherit the IKE group setting. Group-level `ikev2_reauth` is NOT automatically applied when the peer override is absent — that is expected behavior by design. Do not flag this as a missing case.
Applied to files:
src/conf_mode/interfaces_l2tpv3.py
🔇 Additional comments (4)
src/conf_mode/interfaces_l2tpv3.py (3)
23-23: LGTM!
66-68: LGTM!
114-115: LGTM!src/conf_mode/interfaces_openvpn.py (1)
36-37: LGTM!
|
CI integration 👍 passed! Details
|
|
Works correctly on integration build 2026.06.09-1307: zones with VRF members |
sarthurdev
left a comment
sarthurdev
left a comment
There was a problem hiding this comment.
Thanks for implementing this, just one change request to reduce the footprint and deduplicate code in the PR.
| # Check vrf membership, to ensure firewall is updated | ||
| # Parent interface | ||
| if is_node_changed(conf, base + [ifname, 'vrf']): | ||
| bond.update({'vrf_changed': {}}) | ||
| set_dependents('firewall', conf) | ||
| # vif interface | ||
| for vif in conf.list_nodes(base + [ifname, 'vif']): | ||
| if is_node_changed(conf, base + [ifname, 'vif', vif, 'vrf']): | ||
| bond.update({'vrf_changed': {}}) | ||
| set_dependents('firewall', conf) | ||
| # q-in-q interface | ||
| for vif_s in conf.list_nodes(base + [ifname, 'vif-s']): | ||
| if is_node_changed(conf, base + [ifname, 'vif-s', vif_s, 'vrf']): | ||
| bond.update({'vrf_changed': {}}) | ||
| set_dependents('firewall', conf) | ||
| for vif_c in conf.list_nodes(base + [ifname, 'vif-s', vif_s, 'vif-c']): | ||
| if is_node_changed(conf, base + [ifname, 'vif-s', vif_s, 'vif-c', vif_c, 'vrf']): | ||
| bond.update({'vrf_changed': {}}) | ||
| set_dependents('firewall', conf) |
There was a problem hiding this comment.
I realise this is likely a bit of pain, but Ideally for code like this duplicated across majority of interface conf scripts, a single function in vyos.utils or vyos.configdict to check for changes across all VRF nodes called from each script would reduce the footprint quite a lot here.
7 participants
Change summary
This change reimplements the intended behaviour from #4180 aswell as from #4506, it ensures that both the vrf-member interface aswell as the vrf itself is added as an oifname -> meaning that traffic traversing and originating from withing VyOS is matches outbound.
Types of changes
Related Task(s)
https://vyos.dev/T8761
Related PR(s)
How to test / Smoketest result
The tests are carried out by generating some firewall rules and checking the config from nftables - it will not generate both an oifname with the vrf member interfaces, aswell as a line for the vrf itself.
Checklist: