Routing sanities - #1672
Conversation
Fredi-raspall
commented
Jul 29, 2026
Fredi-raspall
commented
- Revisits routing logic
- Tidies up the code and comments future work
- Adds sanities around next-hop resolution loop detection and the implications in fib
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesNext-hop validation, unresolved-state handling, FIB construction, VRF refresh coordination, CLI display, and CPI VRF selection were updated across routing and RPC paths. Next-hop routing updates
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Comment |
Fredi-raspall
added
ci:+release
There was a problem hiding this comment.
Pull request overview
This PR revisits the routing pipeline to harden next-hop handling: it tightens RPC route/next-hop adaptation, improves next-hop resolution consistency (including loop/unresolved outcomes), and ensures the FIB fails closed (DROP) rather than risking misrouting.
Changes:
- Adds stricter RPC next-hop validation and installs a DROP next-hop when a route arrives with no usable next-hops to preserve consistency.
- Refactors next-hop refresh logic to rebuild instructions, flush/recompute resolvers, rebuild fibgroups, and publish targeted FIB updates using weak references.
- Ensures unresolved/loop-detected next-hops do not produce usable forwarding entries (DROP fallback) and surfaces unresolved state in CLI output; adds targeted tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| routing/src/router/rpc_adapt.rs | Validates/normalizes RPC next-hops, improves logging, and injects a DROP next-hop when none can be processed. |
| routing/src/router/cpi.rs | Adjusts VRF routing add/del behavior, relocates EVPN heuristic helpers, and documents current VRF resolution assumptions. |
| routing/src/rib/vrftable.rs | Updates tests to use renamed next-hop instruction rebuild API. |
| routing/src/rib/vrf.rs | Refactors FIB refresh/update flow, rebuilds next-hop state consistently, and restricts some APIs’ visibility. |
| routing/src/rib/rib2fib.rs | Updates fibgroup construction so unresolved next-hops fail closed (DROP) and clarifies behavior in comments. |
| routing/src/rib/nexthop.rs | Adds explicit unresolved/must-resolve helpers, improves lazy resolution behavior, renames instruction rebuild API, and adds tests. |
| routing/src/errors.rs | Introduces a dedicated InvalidNexthop error variant for RPC adaptation failures. |
| routing/src/cli/display.rs | Displays unresolved next-hop state in CLI formatting (including resolvers). |
![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
🧹 Nitpick comments (1)
routing/src/rib/vrf.rs (1)
464-514: 🚀 Performance & Scalability | 🔵 TrivialRedundant instruction-building/resolving/fibgroup work duplicates what
refresh_fibalready does at line 513.The loop at lines 479-485 and the
set_fibgroup/register_fibgroupcalls inside the fib-update block at lines 489-499 are explicitly flagged by your own comments as unnecessary ("none of this is needed since we call refresh_fib at the end", "not needed, set_fibgroup() calls it", "this is not needed if we always call refresh fib"). Sincerefresh_fib(rstore, vrf0)at line 513 unconditionally re-runsrebuild_nhop_instructions,lazy_resolve_all, andrebuild_fibgroupsfor the whole VRF, every route add viaadd_route_completecurrently does this work twice, plus issues extraregister_fibgroupwriter calls that get overwritten before publish. Per coding guidelines, this logic is complex enough to warrant simplification now rather than deferring, since the fix is bounded to this function: keep only thenhkeyscollection (needed foradd_fibroute) and drop the pre-resolution loop and the inlineset_fibgroup/register_fibgroupcalls, lettingrefresh_fibdo that work once.
[medium_effort_and_high_reward]As per coding guidelines, "If logic is overly complex, suggest simplifications."
♻️ Suggested simplification
- let rvrf = vrf0.unwrap_or(self); - - // resolve the next-hops of the received route: none of this is needed since we - // call refresh_fib at the end. Leaving it for future optimizations. - for shim in &route.s_nhops { - let refc = self.nhstore.nhop_strong_count(&shim.rc.key); - shim.rc.build_nhop_instructions(rstore); // not needed, set_fibgroup() calls it - if refc == 2 { - shim.rc.lazy_resolve(rvrf); - } - } - - // update fib: this is not needed if we always call refresh fib. - // Leaving it for future optimizations. + // Register the route -> next-hop mapping; fibgroup contents themselves + // are (re)built by refresh_fib() below. if let Some(fibw) = &mut self.fibw { let mut nhkeys = Vec::with_capacity(route.s_nhops.len()); for shim in &route.s_nhops { - if shim.rc.as_ref().set_fibgroup(rstore) { - let fibgroup = &*shim.rc.as_ref().fibgroup.borrow(); - fibw.register_fibgroup(&shim.rc.key, fibgroup, false); - } nhkeys.push(shim.rc.key.clone()); } fibw.add_fibroute(*prefix, nhkeys, 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 `@routing/src/rib/vrf.rs` around lines 464 - 514, Remove the pre-resolution loop in add_route_complete, including build_nhop_instructions and lazy_resolve calls. In the fibw block, retain only nhkeys collection and fibw.add_fibroute; remove set_fibgroup and register_fibgroup handling. Leave refresh_fib(rstore, vrf0) as the sole operation responsible for rebuilding instructions, resolving next-hops, and rebuilding fibgroups.
🤖 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 `@routing/src/rib/rib2fib.rs`:
- Around line 80-81: Correct the typo in the doc comment describing the next-hop
packet instructions by removing the stray “m”, so it states that the
instructions are attached to the next-hop which owns them.
---
Nitpick comments:
In `@routing/src/rib/vrf.rs`:
- Around line 464-514: Remove the pre-resolution loop in add_route_complete,
including build_nhop_instructions and lazy_resolve calls. In the fibw block,
retain only nhkeys collection and fibw.add_fibroute; remove set_fibgroup and
register_fibgroup handling. Leave refresh_fib(rstore, vrf0) as the sole
operation responsible for rebuilding instructions, resolving next-hops, and
rebuilding fibgroups.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35707e71-4216-4ed8-b51a-1fec70a10936
📒 Files selected for processing (8)
routing/src/cli/display.rsrouting/src/errors.rsrouting/src/rib/nexthop.rsrouting/src/rib/rib2fib.rsrouting/src/rib/vrf.rsrouting/src/rib/vrftable.rsrouting/src/router/cpi.rsrouting/src/router/rpc_adapt.rs
1a1222e to
891d138
Compare
Be more strict in what we accept and add sanities and logs. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Cleans up the code and makes sure to flush all next-hops' resolvers before attempting to re-resolve them lazily to ensure correctness. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
1) Adds methods to explicitly indicate if a next hop requires resolution and whether that succeeded, and tests. 2) Log next-hop resolution failures 3) When building a fibgroup for a next hop, make sure it has fib entries. Otherwise, add a drop entry to drop the traffic. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Let rebuild_fibgroups() return a vector of Weak<Nhop> instead of &Rc<Nhop>. This is like returning Rc<Nhop>, which gets around borrow checker complaints, while not incrementing the refcount of a next-hop, which could mislead the deletion logic. Re-arrange the housekeeping logic to better separate vrf/rib logic from the reflection of changes in the fib. The current implementation does several passes over the data, some unnecessary. Will address this in future work. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
891d138 to
48924bd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
routing/src/router/rpc_adapt.rs:230
confidence: 9
tags: [style]
The warning message has awkward punctuation (`!.)`), which makes logs harder to scan/grep. Consider a single sentence without mixed punctuation.
if nhops.is_empty() {
warn!("Route to {prefix} from RPC would have no next-hop. Will inject DROP next-hop");
nhops.push(RouteNhop::default());
**routing/src/router/rpc_adapt.rs:113**
* ```yaml
confidence: 9
tags: [style]
The InvalidNexthop message hard-codes 0 and includes an exclamation mark. Since InterfaceIndex::try_new() fails specifically because the value is zero (net/src/interface/mod.rs:66-101), the message can be more accurate and consistent by using the canonical wording and avoiding punctuation noise.
let mut ifindex = nh
.ifindex
.map(|i| match InterfaceIndex::try_new(i) {
Ok(idx) => Ok(idx),
Err(_) => Err(RouterError::InvalidNexthop("ifindex 0 is invalid!")),
})
routing/src/rib/vrf.rs:483
confidence: 8
tags: [other]
`build_nhop_instructions()` is redundant here: the loop below already calls `set_fibgroup()`, which rebuilds instructions, and `refresh_fib()` at the end rebuilds instructions for the whole store again. Dropping this call avoids duplicated work and keeps the inline comment from contradicting the actual behavior.
for shim in &route.s_nhops {
let refc = self.nhstore.nhop_strong_count(&shim.rc.key);
shim.rc.build_nhop_instructions(rstore); // not needed, set_fibgroup() calls it
if refc == 2 {
shim.rc.lazy_resolve(rvrf);
</details>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
routing/src/rib/vrf.rs (1)
475-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant resolve/build/fib-registration block, and comment scope is ambiguous for future cleanup.
The per-shim resolve/instruction-build loop (lines 479-485) and the per-shim
register_fibgroupcalls inside the fib-update block (lines 491-495) are explicitly called out by the developer comments as unnecessary becauserefresh_fibis invoked at the end of this function anyway. This is duplicate work today (harmless but wasteful) and a candidate for removal.However, the comment "update fib: this is not needed if we always call refresh fib" spans the whole
if let Some(fibw) = &mut self.fibw { ... }block (lines 489-499), which also containsfibw.add_fibroute(*prefix, nhkeys, true)(line 498). That call maps the route's prefix to its next-hop keys —refresh_fib/update_fibonly re-registers per-nexthop fibgroups, it never callsadd_fibroute. Soadd_fibrouteis not redundant; if a future cleanup removes this block wholesale based on the comment, routes would stop being linked to their next-hops in the FIB.Consider narrowing the comment to the two genuinely redundant pieces (the resolve/build loop and the inner
register_fibgroupcalls) so a future "optimization" doesn't accidentally dropadd_fibroute.🤖 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 `@routing/src/rib/vrf.rs` around lines 475 - 499, Clarify the comments in the route-update logic around the per-shim resolve/build loop and the inner fibgroup registration in the `fibw` block to identify only those operations as redundant because `refresh_fib` runs later. Explicitly preserve and distinguish `fibw.add_fibroute(*prefix, nhkeys, true)` as required for linking the prefix to its next-hop keys, and avoid implying that the entire `if let Some(fibw)` block can be 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.
Inline comments:
In `@routing/src/rib/vrf.rs`:
- Around line 590-599: Gate get_route_mut with #[cfg(test)] to match the
test-only get_route_v4_mut and get_route_v6_mut accessors, ensuring non-test
builds do not reference unavailable methods.
---
Nitpick comments:
In `@routing/src/rib/vrf.rs`:
- Around line 475-499: Clarify the comments in the route-update logic around the
per-shim resolve/build loop and the inner fibgroup registration in the `fibw`
block to identify only those operations as redundant because `refresh_fib` runs
later. Explicitly preserve and distinguish `fibw.add_fibroute(*prefix, nhkeys,
true)` as required for linking the prefix to its next-hop keys, and avoid
implying that the entire `if let Some(fibw)` block can be removed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4717539-be14-411f-aaa8-cc587ff4922e
📒 Files selected for processing (8)
routing/src/cli/display.rsrouting/src/errors.rsrouting/src/rib/nexthop.rsrouting/src/rib/rib2fib.rsrouting/src/rib/vrf.rsrouting/src/rib/vrftable.rsrouting/src/router/cpi.rsrouting/src/router/rpc_adapt.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- routing/src/cli/display.rs
- routing/src/rib/rib2fib.rs
- routing/src/router/cpi.rs
- routing/src/router/rpc_adapt.rs
- routing/src/rib/nexthop.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
routing/src/router/cpi.rs:199
confidence: 9
tags: [style]
Same issue as above: `#[allow(unused)]` won’t prevent `dead_code` warnings for this unused helper. If you want to keep `is_evpn_route()` around for future work, prefer `#[allow(dead_code)]` (or gate it behind `#[cfg(test)]`).
/// Util to tell if a route is EVPN - heuristic
#[must_use]
#[allow(unused)]
fn is_evpn_route(iproute: &IpRoute) -> bool {
**routing/src/router/cpi.rs:175**
* ```yaml
confidence: 9
tags: [style]
#[allow(unused)] does not suppress the dead_code lint for an unused private function. Since nonlocal_nhop() is currently unused, this can still emit warnings (and potentially fail CI if warnings are denied). Use #[allow(dead_code)] (or remove the helper) instead.
This issue also appears on line 196 of the same file.
#[must_use]
#[allow(unused)]
fn nonlocal_nhop(iproute: &IpRoute) -> bool {
routing/src/router/rpc_adapt.rs:230
confidence: 8
tags: [logic]
Injecting `RouteNhop::default()` here sets `vrfid: 0`, which makes the shim next-hop look like it comes “from VRF 0” (and sets `ext_vrf=Some(0)` for non-default VRFs). For an injected DROP next-hop, it’s clearer/safer to keep the route’s VRF id and only change the action to DROP.
nhops.push(RouteNhop::default());
**routing/src/rib/rib2fib.rs:109**
* ```yaml
confidence: 7
tags: [other]
Unresolved next-hops will already trigger a WARN when an empty fibgroup is converted into a DROP-only fibgroup (build_nhop_fibgroup). This additional WARN means unresolved next-hops log twice per rebuild; consider lowering this to debug! to avoid noisy logs under churn.
warn!("Next-hop {self} is unresolved: will not use it");
There was a problem hiding this comment.
🧹 Nitpick comments (1)
routing/src/rib/vrf.rs (1)
475-488: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment overstates that the fib-update block is unneeded —
add_fibrouteis not duplicated byrefresh_fib.The comments here say this next-hop resolution/fib-update work "is not needed since we call refresh_fib at the end" / "not needed if we always call refresh fib." However, the gated block below (lines 489-499, unchanged) also calls
fibw.add_fibroute(*prefix, nhkeys, true), which establishes the prefix→next-hop-keys route binding.refresh_fib/update_fib(lines 428-462) only callfibw.register_fibgroupper changed next-hop — they never calladd_fibroute. If a future cleanup trusts this comment and removes the whole block, routes would stop being published into the FIB. Consider clarifying that only the per-nhop instruction/resolve/register-fibgroup portion is redundant, whileadd_fibrouteremains required.🤖 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 `@routing/src/rib/vrf.rs` around lines 475 - 488, Correct the comments and any related guidance around the next-hop resolution and FIB update block in the route-processing method containing rvrf and route.s_nhops. Clarify that per-next-hop instruction/resolution and fib-group refresh work may be redundant because refresh_fib handles those operations, but explicitly state that the subsequent add_fibroute call remains required to establish the prefix-to-next-hop binding and must not be 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.
Nitpick comments:
In `@routing/src/rib/vrf.rs`:
- Around line 475-488: Correct the comments and any related guidance around the
next-hop resolution and FIB update block in the route-processing method
containing rvrf and route.s_nhops. Clarify that per-next-hop
instruction/resolution and fib-group refresh work may be redundant because
refresh_fib handles those operations, but explicitly state that the subsequent
add_fibroute call remains required to establish the prefix-to-next-hop binding
and must not be removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f48dbf6e-ceb0-476c-b304-1ac1c11b4947
📒 Files selected for processing (8)
routing/src/cli/display.rsrouting/src/errors.rsrouting/src/rib/nexthop.rsrouting/src/rib/rib2fib.rsrouting/src/rib/vrf.rsrouting/src/rib/vrftable.rsrouting/src/router/cpi.rsrouting/src/router/rpc_adapt.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- routing/src/cli/display.rs
- routing/src/errors.rs
- routing/src/rib/vrftable.rs
- routing/src/rib/rib2fib.rs
- routing/src/rib/nexthop.rs
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
Some minor comments, none blocking. Thanks!
Instead of getting a &mut Route, register_shared_nhops() gets a list of nexthops and returns a vector of references to shared nexthops. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
routing/src/router/cpi.rs:175
confidence: 7
tags: [style]
`nonlocal_nhop()` / `is_evpn_route()` are currently unused and are kept alive via `#[allow(unused)]`. Carrying dead code + lint suppression in production paths makes it easier to miss genuinely-unused code over time.
Consider removing these helpers until they’re needed, or moving them behind a test/debug-only cfg rather than suppressing lints here.
#[must_use]
#[allow(unused)]
fn nonlocal_nhop(iproute: &IpRoute) -> bool {
**routing/src/rib/rib2fib.rs:109**
* ```yaml
confidence: 8
tags: [docs]
The warning text "will not use it" is ambiguous given the later behavior of injecting a DROP entry when the fibgroup ends up empty. Clarifying that we’re omitting the forwarding entry (and may fall back to DROP) makes troubleshooting easier.
warn!("Next-hop {self} is unresolved: will not use it");
