More routing sanities - #1675
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
📝 WalkthroughWalkthroughChangesThe routing path validates FIB entries before group insertion and supports egress creation from resolved addresses. RMAC updates report changes and trigger VNI-specific FIB refreshes only when needed. Packet egress metadata and packet dump logging use updated handling. Routing and egress resolution
RMAC refresh handling
Packet dump logging
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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: 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 `@routing/src/rib/nexthop.rs`:
- Around line 919-962: Update test_nhop_instruction_build_and_fibroup so both
fibgroup entries are validated: collect the egress (ifindex, address) pair from
e1 and e2, then compare the resulting set or list against the two expected
interface/address pairs, preserving order independence and ensuring neither
entry can be incorrect.
🪄 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: ce183423-b6a4-44da-83c6-f79e5b6f13ca
📒 Files selected for processing (5)
dataplane/src/packet_processor/ipforward.rsrouting/src/fib/fibgroupstore.rsrouting/src/fib/fibobjects.rsrouting/src/rib/nexthop.rsrouting/src/rib/rib2fib.rs
There was a problem hiding this comment.
Pull request overview
This PR tightens routing/FIB “sanity” behavior by ensuring unresolved or invalid next-hop resolution results don’t accidentally get committed as usable forwarding entries, and by making egress-resolution intent clearer through both code and tests.
Changes:
- Emit
Egressinstructions for next-hops that have an address even when they don’t yet have anifindex, enabling correct recursive resolution behavior. - Add
FibEntry::is_valid()and use it during next-hop → fibgroup construction to ignore invalid leaf entries and only inject a DROP when the resulting group is empty. - Encapsulate
FibGroupinternals (privateentries) and add safe accessors; update call sites and add new unit tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| routing/src/rib/rib2fib.rs | Improves next-hop instruction generation and validates/sanitizes leaf fib entries before committing to groups. |
| routing/src/rib/nexthop.rs | Adds new sanity/unit tests around instruction building and DROP insertion behavior. |
| routing/src/fib/fibobjects.rs | Makes FibGroup.entries private, adds entries_mut(), and introduces FibEntry::is_valid(). |
| routing/src/fib/fibgroupstore.rs | Updates indexing and test helpers to use the new FibGroup accessors. |
| dataplane/src/packet_processor/ipforward.rs | Simplifies egress metadata assignment and logs when an egress object lacks an outgoing interface. |
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
I'd appreciate a comment to explain why a FIB entry is valid or not, but that's not blocking. Looks good otherwise, thanks!
f4b99e1 to
ba8a2e7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
routing/src/fib/fibobjects.rs:132
confidence: 8
tags: [style]
`FibGroup::entries_mut()` is only used from `#[cfg(test)]` code right now, but it’s exposed as a public API. This widens the public surface and lets callers arbitrarily mutate the internal `Vec`, undermining the intent of making `entries` private.
Consider restricting it to `pub(crate)` (or gating it under `#[cfg(test)]`) unless there’s a concrete external consumer that needs mutable access.
pub fn entries_mut(&mut self) -> &mut Vec<FibEntry> {
</details>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
routing/src/fib/fibobjects.rs:205
confidence: 10
tags: [logic]
`FibEntry::is_valid()` takes an extra reference to `self.instructions.last()`, making `inst` a `&&PktInstruction`. This is easy to miss and can lead to a compile error / incorrect matching; also `unwrap_or_else(|| unreachable!())` is redundant because this branch is only hit when `len() >= 2`.
Remove the extra `&` and simplify the branch to avoid the double-reference.
_ => {
let inst = &self.instructions.last().unwrap_or_else(|| unreachable!());
match inst {
PktInstruction::Egress(e) => e.ifindex().is_some(),
_ => false,
**routing/src/fib/fibobjects.rs:132**
* ```yaml
confidence: 8
tags: [style]
FibGroup::entries_mut() is declared pub, which expands the public API to allow any downstream crate to mutate the internal Vec<FibEntry>. Since the only current use is within this crate’s tests, this should be pub(crate) to keep the encapsulation benefits of making entries private.
pub fn entries_mut(&mut self) -> &mut Vec<FibEntry> {
routing/src/evpn/rmac.rs:90
confidence: 8
tags: [logic]
`RmacStore::add_rmac_entry()` documents “newly inserted or updated”, and it also decrements the stale counter when replacing a stale entry. However it currently returns `false` when the MAC is unchanged even if the previous entry was stale (i.e. the store state did change). Since callers (e.g. `router/cpi.rs`) use this boolean to decide whether to refresh VRF FIBs, stale→fresh replacements should return `true` as well.
if let Some(stale_t) = &old.stale_t {
debug!("The rmac was stale for {}s", stale_t.elapsed().as_secs());
self.stale = self.stale.saturating_sub(1);
}
was_updated
</details>
Add method to validate a fib entries and replace by a drop if the entry is not correct. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Let add_rmac_entry() report if the rmac added created a new entry or updated one. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Force a vrf/fib refresh when we learn a new rmac or it changes. FRR sends first rmacs and then the routes that depend on it, but protecting dataplane against reordering is good. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
9e61422 to
1e6642a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
routing/src/fib/fibobjects.rs:126
confidence: 10
tags: [docs]
Doc comment uses the incorrect pluralization "[`FibEntry`]ies"; it should be "[`FibEntry`]s".
pub fn entries(&self) -> &Vec<FibEntry> {
**routing/src/evpn/rmac.rs:70**
* ```yaml
confidence: 9
tags: [docs]
add_rmac_entry()’s doc says it returns true when an entry is “newly inserted or updated”, but the implementation returns false when the entry already exists and the MAC is unchanged (even though other fields like stale_t may differ). This makes the API contract ambiguous for callers (e.g., deciding whether to refresh dependent FIBs).
Consider tightening the doc to match the actual behavior (inserted or MAC-changed), or adjust the boolean computation if broader “updated” semantics are intended.
/// Add a `RmacEntry` to the rmac store. This method never fails.
/// Returns true if a `RmacEntry` was newly inserted or updated.
//////////////////////////////////////////////////////////////////
#[must_use]
pub fn add_rmac_entry(&mut self, entry: RmacEntry) -> bool {
routing/src/rib/nexthop.rs:993
confidence: 8
tags: [docs]
This test comment attributes the DROP fib entry to the resulting `FibEntry` being “not valid”, but in this setup the resolver chain ends in an unresolved next-hop (`must_be_resolved()`), so the recursive builder returns early and `build_nhop_fibgroup()` injects a DROP because the fibgroup is empty. Updating the comment would make the test intent clearer.
// check: the fibgroup for nh1 contains 1 fib entry drop, in spite of the egress instruction, since
// the resulting fibentry would not be valid.
</details>
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 1 comment.
Suppressed comments (4)
routing/src/rib/nexthop.rs:963
confidence: 9
tags: [logic]
Same compilation issue here: `IpAddr::from_str(...)` requires the `FromStr` trait to be in scope in this module. Switching to `parse::<std::net::IpAddr>()` avoids the missing-import problem.
assert!(matches!(&e1.instructions[0], PktInstruction::Egress(e)
if e.ifindex().unwrap().to_u32() == 1 && e.address().unwrap() == IpAddr::from_str("10.0.0.1").unwrap()
|| e.ifindex().unwrap().to_u32() == 2 && e.address().unwrap() == IpAddr::from_str("10.0.1.1").unwrap()));
**routing/src/rib/nexthop.rs:966**
* ```yaml
confidence: 9
tags: [logic]
Same compilation issue here as well (IpAddr::from_str without importing FromStr into the tests module).
assert!(matches!(&e2.instructions[0], PktInstruction::Egress(e)
if e.ifindex().unwrap().to_u32() == 1 && e.address().unwrap() == IpAddr::from_str("10.0.0.1").unwrap()
|| e.ifindex().unwrap().to_u32() == 2 && e.address().unwrap() == IpAddr::from_str("10.0.1.1").unwrap()));
routing/src/rib/nexthop.rs:990
confidence: 9
tags: [logic]
`IpAddr::from_str(...)` here has the same missing-import issue inside the `tests` module; using `parse::<std::net::IpAddr>()` keeps it compiling without adding new `use` statements elsewhere.
assert!(
matches!(inst, PktInstruction::Egress(e) if e.address().unwrap() == IpAddr::from_str("7.0.0.1").unwrap())
);
**routing/src/fib/fibobjects.rs:128**
* ```yaml
confidence: 10
tags: [docs]
Same pluralization typo in this doc comment ("[FibEntry]ies").
/// Provide a reference to the vector of [`FibEntry`]ies in a [`FibGroup`]
#[must_use]
pub fn entries(&self) -> &Vec<FibEntry> {
&self.entries
}
No description provided.