Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (9)
|
| Layer / File(s) | Summary |
|---|---|
Render and verify local preference interface-definitions/include/bgp/protocol-common-config.xml.i, python/vyos/frrender.py, data/templates/frr/bgpd.frr.j2, smoketest/scripts/cli/test_protocols_bgp.py |
The XML defines 100 as the default. FRR rendering data passes the XML default for global BGP and VRFs. The template emits the directive only when the configured value differs from the default. The smoke test checks unset, default, non-default, and reset values in global and VRF configurations. |
Priority: ➖ Normal
Merge Risk: ⚪ Minimal · up to df227
No confirmed issue prevents merging. The added test covers the return to the default value, subject to normal test execution.
Security Architecture Review
Security architecture risk: 🔵 Low · up to df227
The change keeps the existing configuration interface and separates global and VRF settings. No introduced security issue was established. Remaining uncertainty concerns whether resetting or removing a non-default value reliably converges after an interrupted or failed reload.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
- inferred — The relevant policy scope is the configured global or named-VRF BGP instance on the router. A reconciliation error could affect routing preference within that instance; the inspected changes do not establish broader service or credential exposure.
Security Findings and Attack Paths
- observed — The supplied public-entrypoint range is a unittest method under smoketest. It calls existing test helpers and reads configuration output; it is not a newly exposed production endpoint.
Trust Boundaries and Controls
- observed — The existing numeric validation remains in the schema, and application continues through the existing reload test and reload commands. The new template condition does not introduce a new command executor or credential source.
Resilience and Maintainability Implications
- inferred — The inspected commit daemon processes requests synchronously with one long-lived renderer, providing serialization within that process. Cross-process coordination and convergence after interruption remain unverified.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (2 skipped: 2 … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Title check | ✅ Passed | The title clearly identifies the BGP default local-preference change and the condition for omitting the command. |
| Description check | ✅ Passed | The description explains the change, its purpose, implementation, and test results. It is directly related to the changeset. |
Full details: Docstring Coverage
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (2 skipped: 2 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
- Commit to this branch
- Create a new PR
✨ Simplify code
- Create a new PR
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
There was a problem hiding this comment.
Local-pref 100 is the default for FRR/BGP
Do not render/configure the default local-pref option if we have the configured local-pref 100
I wonder if we should catch and remove it from the Python config dictionary instead. Or remove the default value from XML
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
c-po
left a comment
c-po
left a comment
There was a problem hiding this comment.
The PR itself catches a nice corner case - but the implementation can be improved.
The FRR default value should be added as proper XML default https://github.com/vyos/vyos-1x/blob/rolling/interface-definitions/include/bgp/protocol-common-config.xml.i#L1430
And then the proper XML default should be read-back in the Jina template for comparison, this will avoid multiple hardocded places of default 100
…ue is 100 FRR does not print "bgp default local-preference 100" in show running-config because 100 is its default, so frr-reload never finds the line in the running config and sends it again on every reload. The command handler calls bgp_clear_star_soft_in() whether or not the value changed, so every reload ran "clear bgp * soft in" (once in pass 0 and twice in pass 1), a full inbound re-evaluation of every peer. Add the default (100) to the XML definition of parameters default local-pref. Templates cannot query the XML definition while rendering, so frrender takes the value from the config defaults of "parameters default" (conf.get_config_defaults) and adds it to the BGP dict as the key xml_default_local_pref, for the global instance and for every VRF instance. bgpd.frr.j2 renders the line only when the configured value differs from xml_default_local_pref, so the value 100 is not hardcoded in the template. A real value change (100 to 200, or 200 to 100) is still sent: frr-reload sees the difference between the rendered file and the running config.
50b4d40 to
df22720
Compare
|
CI integration ❌ failed! Details
|
|
@c-po Updated:
|
3 participants
Change summary
bgp default local-preferenceis now rendered only when the configured value differs from the default, 100:interface-definitions/include/bgp/protocol-common-config.xml.i:<defaultValue>100</defaultValue>onparameters default local-pref(shared by the global and VRF instances).python/vyos/frrender.py: the default is added to the BGP dict asxml_default_local_pref(fromconf.get_config_defaults()), for the global instance and for every VRF instance.data/templates/frr/bgpd.frr.j2: renders the line only whenparameters.default.local_prefdiffers fromxml_default_local_pref.FRR does not print
bgp default local-preference 100in its running configuration, because 100 is the default. frr-reload therefore treated the rendered line as missing and sent it again on every reload. FRR re-processes the routes of every peer whenever that command is applied, even with an unchanged value, so every commit made the router run all received routes through the import policy again (with soft-reconfiguration inbound) or ask every peer to send its whole table again. Not rendering the default value removes the repeated command; FRR's effective value stays 100.Changing the value still reaches FRR: from 100 to another value the line is rendered and applied; from another value back to 100, frr-reload sends
no bgp default local-preference <old>, which restores FRR's default.Measured on a VyOS 1.5.1 VM with about 1.76M BGP paths (2 route servers, 2 transits, 80 IXP peers, soft-reconfiguration inbound),
default local-pref 100configured:maximum-prefixchange on the IXP peer-groupOn rolling (FRR 10.6.1) with one eBGP session, counted as route-refresh messages sent by the router per commit:
FRR's local preference was correct after each step.
Types of changes
Related Task(s)
https://vyos.dev/T9404
Related PR(s)
How to test / Smoketest result
test_bgp_107_default_local_pref_default_valueinsmoketest/scripts/cli/test_protocols_bgp.pyreads the rendered/run/frr/config/vyos.frr.conf(FRR never prints 100, so the running configuration cannot show it): nobgp default local-preferenceline when local-pref is not configured or is 100; the line with 200, in the file and in FRR's running configuration; gone again after going back to 100; and the same for a VRF instance, where 200 appears only in the VRF block.Checklist: