interfaces: T9152: Refactor source-validation to use vmaps - #5366
interfaces: T9152: Refactor source-validation to use vmaps#5366l0crian1 wants to merge 4 commits into
Conversation
- Refactor source-validation to use vmaps - Update broken source-validation tests - Fix source-validation failing to remove rules when interface is removed (T9153)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change replaces per-interface reverse-path filtering with shared nftables sets and maps for IPv4 and IPv6. Interface updates render, validate, and apply mode-specific configuration. Smoke tests verify the shared nftables state. ChangesSource validation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
python/vyos/ifconfig/interface.py (2)
898-904: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReport the nft error and check the apply return code.
The message drops the nft diagnostics, so a failure gives no reason. The final
nft -freturn code on Line 904 is discarded, so a partial or failed apply passes silently.♻️ Proposed change
def _apply_rpf_nft_config(self, rpf_dict): rpf_template = '/run/nftables-source-validation.conf' render(rpf_template, 'firewall/nftables-source-validation.j2', rpf_dict) - tmp = run(['nft', '-c', '-f', rpf_template]) - if tmp > 0: - raise ConfigError('Source validation configuration file errors encountered!') - run(['nft', '-f', rpf_template]) + rc, out = rc_cmd(['nft', '-c', '-f', rpf_template]) + if rc != 0: + raise ConfigError(f'Source validation configuration file errors encountered: {out}') + rc, out = rc_cmd(['nft', '-f', rpf_template]) + if rc != 0: + raise ConfigError(f'Failed to apply source validation configuration: {out}')
rc_cmdmust be imported fromvyos.utils.process.🤖 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 `@python/vyos/ifconfig/interface.py` around lines 898 - 904, Update _apply_rpf_nft_config to capture and report nft diagnostics from the validation command, using rc_cmd from vyos.utils.process as required. Also capture the return code from the final nft apply command and raise ConfigError when it fails instead of discarding it; preserve the existing successful flow.
918-938: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated source-validation flow in
python/vyos/ifconfig/interface.py. Both methods repeat the same set lookup, dict build, needs-rule computation, delete scheduling, and apply. Only the family literal differs. Extract one private helper and keep the public methods as thin wrappers.
python/vyos/ifconfig/interface.py#L918-L938: replace the body withreturn self._set_source_validation('ip', mode).python/vyos/ifconfig/interface.py#L952-L972: replace the body withreturn self._set_source_validation('ip6', mode).♻️ Proposed shared helper
def _set_source_validation(self, family, mode): # Don't allow for netns yet if 'netns' in self.config: return None strict_ifaces, loose_ifaces = self._get_rpf_interface_rules(family) rpf_dict = { 'family': family, 'iface': self.ifname, 'mode': mode, } def needs_rule(ifaces): return len(ifaces) == 0 or (len(ifaces) == 1 and self.ifname in ifaces) strict_needs_rule = needs_rule(strict_ifaces) loose_needs_rule = needs_rule(loose_ifaces) self._set_rpf_rule_deletes(family, rpf_dict, strict_needs_rule, loose_needs_rule) if mode == 'strict' and strict_needs_rule: rpf_dict['strict_add'] = True if mode == 'loose' and loose_needs_rule: rpf_dict['loose_add'] = True self._apply_rpf_nft_config(rpf_dict)🤖 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 `@python/vyos/ifconfig/interface.py` around lines 918 - 938, The source-validation methods duplicate identical rule-processing logic; extract it into a private _set_source_validation(family, mode) helper. In python/vyos/ifconfig/interface.py lines 918-938, make the public method return self._set_source_validation('ip', mode); in lines 952-972, return self._set_source_validation('ip6', mode). Move the shared lookup, configuration, rule computation, deletion, add flags, and apply behavior into the helper while preserving the netns guard and family-specific behavior.
🤖 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 `@python/vyos/ifconfig/interface.py`:
- Around line 403-408: Guard the source-validation cleanup in remove() by
checking that the interface still exists before calling
set_ipv4_source_validation('disable') and set_ipv6_source_validation('disable').
Match the missing-interface handling already used by flush_addrs(), while
preserving cleanup for live interfaces.
- Around line 868-871: Update _get_nft_set_elements to invoke _cmdl with
ConfigError handling and convert missing rpfilter set failures into ConfigError.
Validate that the parsed nftables response contains a set element before
indexing it, returning [] when the set is absent.
In `@smoketest/scripts/cli/base_interfaces_test.py`:
- Around line 1129-1133: Update both source-validation checks in
smoketest/scripts/cli/base_interfaces_test.py at lines 1129-1133 and 1193-1197
to assert the expected IPv4 and IPv6 rule text directly against the complete
chain output, ensuring the test fails when the rule is absent. Remove the unused
f-string prefixes, and also verify configured interfaces are present in
rpfilter_strict_ifaces and absent after deletion.
---
Nitpick comments:
In `@python/vyos/ifconfig/interface.py`:
- Around line 898-904: Update _apply_rpf_nft_config to capture and report nft
diagnostics from the validation command, using rc_cmd from vyos.utils.process as
required. Also capture the return code from the final nft apply command and
raise ConfigError when it fails instead of discarding it; preserve the existing
successful flow.
- Around line 918-938: The source-validation methods duplicate identical
rule-processing logic; extract it into a private _set_source_validation(family,
mode) helper. In python/vyos/ifconfig/interface.py lines 918-938, make the
public method return self._set_source_validation('ip', mode); in lines 952-972,
return self._set_source_validation('ip6', mode). Move the shared lookup,
configuration, rule computation, deletion, add flags, and apply behavior into
the helper while preserving the netns guard and family-specific behavior.
🪄 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: 0dc344dd-dbe4-452e-a9cd-7b1f4d72d6f4
📒 Files selected for processing (4)
data/templates/firewall/nftables-source-validation.j2data/vyos-firewall-init.confpython/vyos/ifconfig/interface.pysmoketest/scripts/cli/base_interfaces_test.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
smoketest/scripts/cli/base_interfaces_test.pypython/vyos/ifconfig/interface.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Runtime smoketests must be located under
smoketest/and use nose2 framework
Files:
smoketest/scripts/cli/base_interfaces_test.py
data/templates/**/*.j2
📄 CodeRabbit inference engine (AGENTS.md)
Prefer storing Jinja2 templates as discrete files in
data/templates/rather than inline Python strings
Files:
data/templates/firewall/nftables-source-validation.j2
**/*.j2
📄 CodeRabbit inference engine (AGENTS.md)
Jinja2 templates must pass linting validation
Files:
data/templates/firewall/nftables-source-validation.j2
python/vyos/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python version must be >=3.11 for all code in the
vyos.*library
Files:
python/vyos/ifconfig/interface.py
🧠 Learnings (3)
📚 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/base_interfaces_test.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/base_interfaces_test.py
📚 Learning: 2026-06-29T12:13:51.293Z
Learnt from: andamasov
Repo: vyos/vyos-1x PR: 5298
File: smoketest/scripts/cli/test_vpp.py:0-0
Timestamp: 2026-06-29T12:13:51.293Z
Learning: When reviewing vyos-1x code that parses or asserts VPP CLI output (e.g., smoketest CLI tests and VPP op-mode code), do not flag the token spelling "Forwrd" / "U-Forwrd" as a typo. It is intentionally preserved verbatim from the upstream VPP CLI text shown by commands like `vppctl show bridge-domain ... detail`. This misspelling is centrally allowlisted (vyos/.github#153) for that specific VPP-CLI context, so typo-review comments should exclude "Forwrd" when it originates from that VPP output.
Applied to files:
smoketest/scripts/cli/base_interfaces_test.py
🪛 ast-grep (0.45.0)
python/vyos/ifconfig/interface.py
[error] 899-899: Avoid HTML built in strings
Context: render(rpf_template, 'firewall/nftables-source-validation.j2', rpf_dict)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
🔍 Remote MCP vyos.dev
Relevant Phorge context
- T9152 — “firewall: Move source-validation rules into a vmap to improve traffic processing speed.” Status: Open, priority Low. Its target is reducing per-interface processing from two rules per interface to shared strict/loose vmaps and interface sets; it specifically calls for removing the global chain’s
return. - T9153 — “interface: source-validation is not removed when a logical interface is removed.” Status: Open, priority Requires assessment. The reproduction shows stale source-validation rules remaining after deleting a VIF before its
ip/ipv6sections; the task notes cleanup must occur from the interfaceremove()path. - Neither task has comments requiring additional behavior.
These requirements align with the described vmap refactor and interface-removal cleanup. During review, verify both IPv4/IPv6 cleanup paths and that strict/loose map entries and interface sets remain consistent after deletion.
🔇 Additional comments (3)
data/vyos-firewall-init.conf (1)
19-37: 🎯 Functional CorrectnessValidate the per-interface
acceptsemantics.
data/vyos-firewall-init.conflines 18-76 define the raw maps and join tovyos_rpfilter;data/templates/firewall/nftables.j2already rendersreturnfromvyos_global_rpfilter, so no template change is needed there. Check the nftaccepthandling for loose source-validationiifname@rpfilter_loose_ifacesfib saddr oif vmap@rpfilter_loose``, because a matching egress interface from routing can terminatepreroutingand skip later filter policy. The same check applies both to raw `ip` and raw `ip6`.python/vyos/ifconfig/interface.py (1)
1963-1971: 🎯 Functional CorrectnessNo action needed.
source-validationCLI values are constrained tostrict,loose, ordisable, and existing callers use text modes only.data/templates/firewall/nftables-source-validation.j2 (1)
1-11: 🩺 Stability & AvailabilityNo change needed.
The current VyOS nftables base includes
destroysupport for rules and map elements.
There was a problem hiding this comment.
Pull request overview
Refactors per-interface reverse-path source validation to use nftables vmaps/sets, reducing the number of rules needed as interface count grows and improving lookup efficiency. It also updates the related smoketests and adds nftables initialization structures needed for the new approach.
Changes:
- Switch interface source-validation programming from per-interface rules to vmap/set based rules driven by a rendered nftables snippet.
- Ensure rpfilter entries are removed during interface deletion before the interface disappears (to allow key resolution).
- Fix smoketest matching to tolerate leading whitespace and reflect the new vmap rule shape.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| smoketest/scripts/cli/base_interfaces_test.py | Updates source-validation assertions to match the new vmap-based rpfilter rules and avoid whitespace sensitivity. |
| python/vyos/ifconfig/interface.py | Implements vmap/set driven source-validation management and ensures cleanup occurs during interface removal. |
| data/vyos-firewall-init.conf | Initializes rpfilter maps/sets in nftables so per-interface updates can manage elements/rules. |
| data/templates/firewall/nftables-source-validation.j2 | Adds nftables snippet template to delete/add rpfilter elements and (optionally) rules for strict/loose modes. |
Suppressed comments (2)
smoketest/scripts/cli/base_interfaces_test.py:1197
- This smoketest can pass even if the expected rpfilter rule is missing: if no line matches
base_options, no assertion runs. Capturing matching lines and asserting at least one match makes the test actually validate source-validation behavior.
if cli_defined(self._base_path + ['ipv6'], 'source-validation'):
base_options = f'iifname @rpfilter_strict_ifaces'
out = cmdl(['nft', 'list', 'chain', 'ip6', 'raw', 'vyos_rpfilter'], sudo=True)
for line in out.splitlines():
if base_options in line:
self.assertIn('iifname @rpfilter_strict_ifaces fib saddr . iif oif vmap @rpfilter_strict', line)
data/vyos-firewall-init.conf:83
- vyos_global_rpfilter is defined as an empty chain here, but other firewall nft templates consistently include an explicit
returnstatement (e.g., data/templates/firewall/nftables.j2:22-31). Addingreturnkeeps initialization consistent and avoids relying on implicit end-of-chain behavior.
chain vyos_global_rpfilter {
}
| def _get_nft_set_elements(self, family, table, set_name): | ||
| """Return elements of an nftables set, or [] if none are present.""" | ||
| tmp = json.loads(self._cmdl(['nft', '-j', 'list', 'set', family, table, set_name])) | ||
| return dict_search('set.elem', tmp['nftables'][1], []) |
There was a problem hiding this comment.
This is because the output of the command always returns a list, even if there's just one interface. If there is no 'elem' key, then it returns an empty list. There is no need for your suggestion.
| rpf_template = '/run/nftables-source-validation.conf' | ||
| render(rpf_template, 'firewall/nftables-source-validation.j2', rpf_dict) | ||
| tmp = run(['nft', '-c', '-f', rpf_template]) | ||
| if tmp > 0: | ||
| raise ConfigError('Source validation configuration file errors encountered!') | ||
| run(['nft', '-f', rpf_template]) |
There was a problem hiding this comment.
Agreed, change made!
| if cli_defined(self._base_path + ['ip'], 'source-validation'): | ||
| base_options = f'iifname "{interface}"' | ||
| base_options = f'iifname @rpfilter_loose_ifaces' | ||
| out = cmdl(['nft', 'list', 'chain', 'ip', 'raw', 'vyos_rpfilter'], sudo=True) | ||
| for line in out.splitlines(): | ||
| if line.startswith(base_options): | ||
| self.assertIn('fib saddr oif 0', line) | ||
| self.assertIn('drop', line) | ||
| if base_options in line: | ||
| self.assertIn('iifname @rpfilter_loose_ifaces fib saddr oif vmap @rpfilter_loose', line) |
There was a problem hiding this comment.
This was mentioned by coderabbit as well. The previous check was always passing, so this makes it so this is at least evaluated.
There are multiple places in that file where tests follow that pattern, so it makes more sense to correct all of those in a single future PR than trying to fix them one at a time.
| chain vyos_global_rpfilter { | ||
| return | ||
| } |
There was a problem hiding this comment.
The explicit return buys nothing here and is unnecessary computational overhead. The goal of this PR is to improve inefficiencies in the existing implementation, so keeping the unnecessary return that must be processed as a rule is antithetical to the goal of this change.
Ultimately, all unnecessary returns should be removed from the full nftables implementation, rather than forcing that inefficiency on this change.
Merge Protections🔴 1 of 1 protections blocking · waiting on 🙋 you
🔴 invalid-task-id label must be absent to mergeWaiting for
This rule is failing.Block merge while the invalid-task-id label is present. Set by the per-repo product T-ID rule (product repos only); dormant where the label is never applied.
|
- Reduced rule processing of source-validation rules through hashable objects
|
CI integration ❌ failed! Details
|
| base_options = f'iifname "{interface}"' | ||
| out = cmdl(['nft', 'list', 'chain', 'ip', 'raw', 'vyos_rpfilter'], sudo=True) | ||
| base_options = f'iifname @rpfilter_loose_ifaces fib saddr oif' | ||
| out = cmdl(['nft', '-s', 'list', 'chain', 'ip', 'raw', 'vyos_rpfilter'], sudo=True) | ||
| for line in out.splitlines(): | ||
| if line.startswith(base_options): | ||
| self.assertIn('fib saddr oif 0', line) | ||
| self.assertIn('drop', line) | ||
| if base_options in line: | ||
| self.assertIn('iifname @rpfilter_loose_ifaces fib saddr oif != 0 counter accept', line) |
There was a problem hiding this comment.
This should also assert the presence of the interface in the strict/loose sets
| base_options = f'iifname "{interface}"' | ||
| base_options = f'iifname @rpfilter_strict_ifaces' | ||
| out = cmdl(['nft', 'list', 'chain', 'ip6', 'raw', 'vyos_rpfilter'], sudo=True) | ||
| for line in out.splitlines(): | ||
| if line.startswith(base_options): | ||
| self.assertIn('fib saddr . iif oif 0', line) | ||
| self.assertIn('drop', line) | ||
| if base_options in line: | ||
| self.assertIn('iifname @rpfilter_strict_ifaces fib saddr . iif oif vmap @rpfilter_strict', line) |
There was a problem hiding this comment.
This should also assert the presence of the interface in the strict/loose sets
Change summary
This changes the inefficient 2 rules per interface implementation of
source-validationto use vmaps instead. This will generally mean 1 rule lookup regardless of the number of interfaces configured for source-validation.Additionally, 2 more issues were corrected:
startswith, but nftables output has whitespace to the left that wasn't accounted for.iporipv6sections were, the source-validation failed to get removed, leaving it present for non-existent interfaces. This is tracked in T9153Types of changes
Related Task(s)
https://vyos.dev/T9152
https://vyos.dev/T9153
Related PR(s)
How to test / Smoketest result
Configure source validation for one or more interfaces:
Verify nftables output:
Delete interfaces:
Verify config was removed from nftables:
Checklist: