From 8851c84fd4ccaf377c9dfa1ca914a862dddff5d6 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 12:31:14 -0600 Subject: [PATCH 01/14] test(routing): prop-test rib-to-fib conversion Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- routing/src/fib/fibobjects.rs | 164 ++++++++++++++++++++++++++++ routing/src/rib/nexthop.rs | 196 ++++++++++++++++++++++++++++++++++ 2 files changed, 360 insertions(+) diff --git a/routing/src/fib/fibobjects.rs b/routing/src/fib/fibobjects.rs index 70c7b25151..edd72d1fca 100644 --- a/routing/src/fib/fibobjects.rs +++ b/routing/src/fib/fibobjects.rs @@ -262,3 +262,167 @@ pub enum PktInstruction { Encap(Encapsulation), /* encapsulate the packet */ Egress(EgressObject), /* send the packet over interface to some ip */ } + +#[cfg(test)] +mod squash_properties { + use super::*; + use crate::rib::encapsulation::VxlanEncapsulation; + use bolero::{Driver, ValueGenerator}; + use std::net::Ipv4Addr; + use std::num::NonZero; + use std::ops::Bound::Included; + + const ADDRESSES: [IpAddr; 3] = [ + IpAddr::V4(Ipv4Addr::new(10, 0, 0, 1)), + IpAddr::V4(Ipv4Addr::new(10, 0, 0, 2)), + IpAddr::V4(Ipv4Addr::new(10, 0, 0, 3)), + ]; + const IFNAMES: [&str; 2] = ["eth0", "eth1"]; + + fn index(raw: u8) -> InterfaceIndex { + InterfaceIndex::new(NonZero::new(u32::from(raw)).unwrap_or_else(|| unreachable!())) + } + + fn choose(pick: u8, choices: &[T]) -> Option { + (pick > 0).then(|| choices[usize::from(pick - 1)].clone()) + } + + fn egress(driver: &mut D) -> Option { + let ifindex = driver.gen_u8(Included(&0), Included(&3))?; + let address = driver.gen_u8(Included(&0), Included(&3))?; + let ifname = driver.gen_u8(Included(&0), Included(&2))?; + Some(EgressObject::new( + choose(ifindex, &[index(1), index(2), index(3)]), + choose(address, &ADDRESSES), + choose(ifname, &IFNAMES).map(str::to_string), + )) + } + + fn instruction(driver: &mut D) -> Option { + Some(match driver.gen_u8(Included(&0), Included(&3))? { + 0 => PktInstruction::Local(index(driver.gen_u8(Included(&1), Included(&3))?)), + 1 => PktInstruction::Drop, + 2 => PktInstruction::Encap(Encapsulation::Vxlan(VxlanEncapsulation::new( + Vni::new_checked(u32::from(driver.gen_u8(Included(&1), Included(&3))?)) + .unwrap_or_else(|_| unreachable!()), + ADDRESSES[0], + ))), + _ => PktInstruction::Egress(egress(driver)?), + }) + } + + #[derive(Debug, Clone, Copy, Default)] + struct Entry; + + impl ValueGenerator for Entry { + type Output = FibEntry; + + fn generate(&self, driver: &mut D) -> Option { + let count = driver.gen_u8(Included(&0), Included(&5))?; + let mut entry = FibEntry::new(); + for _ in 0..count { + entry.add(instruction(driver)?); + } + Some(entry) + } + } + + fn egresses(entry: &FibEntry) -> Vec<&EgressObject> { + entry + .iter() + .filter_map(|inst| match inst { + PktInstruction::Egress(e) => Some(e), + _ => None, + }) + .collect() + } + + fn others(entry: &FibEntry) -> Vec<&PktInstruction> { + entry + .iter() + .filter(|inst| !matches!(inst, PktInstruction::Egress(_))) + .collect() + } + + #[test] + fn squash_preserves_the_other_instructions_in_order() { + bolero::check!() + .with_generator(Entry) + .cloned() + .for_each(|entry: FibEntry| { + let before: Vec = others(&entry).into_iter().cloned().collect(); + let mut squashed = entry.clone(); + squashed.squash(); + if entry.len() == 1 { + assert_eq!(squashed, entry); + return; + } + let after: Vec = others(&squashed).into_iter().cloned().collect(); + assert_eq!(after, before, "for {entry:?}"); + }); + } + + #[test] + fn squash_leaves_at_most_one_egress_and_puts_it_last() { + bolero::check!() + .with_generator(Entry) + .cloned() + .for_each(|entry: FibEntry| { + if entry.len() == 1 { + return; + } + let mut squashed = entry.clone(); + squashed.squash(); + + assert!(egresses(&squashed).len() <= 1, "for {entry:?}"); + if let Some(position) = squashed + .iter() + .position(|inst| matches!(inst, PktInstruction::Egress(_))) + { + assert_eq!(position, squashed.len() - 1, "for {entry:?}"); + } + }); + } + + #[test] + fn squash_merges_first_interface_last_address_first_name() { + bolero::check!() + .with_generator(Entry) + .cloned() + .for_each(|entry: FibEntry| { + if entry.len() == 1 { + return; + } + let inputs = egresses(&entry); + let ifindex = inputs.iter().find_map(|e| *e.ifindex()); + let address = inputs.iter().rev().find_map(|e| *e.address()); + let ifname = inputs.iter().find_map(|e| e.ifname().clone()); + + let mut squashed = entry.clone(); + squashed.squash(); + + match egresses(&squashed).first() { + Some(merged) => { + assert_eq!(*merged.ifindex(), ifindex, "interface, for {entry:?}"); + assert_eq!(*merged.address(), address, "address, for {entry:?}"); + assert_eq!(*merged.ifname(), ifname, "name, for {entry:?}"); + } + None => assert!(ifindex.is_none(), "for {entry:?}"), + } + }); + } + + #[test] + fn squash_is_idempotent() { + bolero::check!() + .with_generator(Entry) + .cloned() + .for_each(|entry: FibEntry| { + let mut once = entry.clone(); + once.squash(); + let mut twice = once.clone(); + twice.squash(); + assert_eq!(twice, once, "for {entry:?}"); + }); + } +} diff --git a/routing/src/rib/nexthop.rs b/routing/src/rib/nexthop.rs index f4a52e0582..b2757462ac 100644 --- a/routing/src/rib/nexthop.rs +++ b/routing/src/rib/nexthop.rs @@ -1045,3 +1045,199 @@ mod tests { assert!(a.resolves_with(checked.as_ref())); } } + +#[cfg(test)] +mod fibgroup_properties { + use super::*; + use crate::fib::fibobjects::FibEntry; + use bolero::{Driver, ValueGenerator}; + use std::ops::Bound::Included; + + const MAX_NODES: u8 = 6; + + #[derive(Debug, Clone)] + struct Dag { + shape: Vec>, + grounded: Vec, + } + + impl Dag { + fn edges(&self) -> Vec<(usize, usize)> { + let mut edges = Vec::new(); + for (from, offsets) in self.shape.iter().enumerate() { + for offset in offsets { + let to = from + usize::from(*offset); + if to < self.shape.len() { + edges.push((from, to)); + } + } + } + edges + } + + fn reachable_from(&self, start: usize) -> Vec { + let edges = self.edges(); + let mut seen = vec![false; self.shape.len()]; + let mut stack = vec![start]; + while let Some(node) = stack.pop() { + if std::mem::replace(&mut seen[node], true) { + continue; + } + for (from, to) in &edges { + if *from == node { + stack.push(*to); + } + } + } + seen + } + } + + #[derive(Debug, Clone, Copy, Default)] + struct Graphs; + + impl ValueGenerator for Graphs { + type Output = Dag; + + fn generate(&self, driver: &mut D) -> Option { + let nodes = usize::from(driver.gen_u8(Included(&1), Included(&MAX_NODES))?); + let mut shape = Vec::with_capacity(nodes); + let mut grounded = Vec::with_capacity(nodes); + for index in 0..nodes { + let behind = u8::try_from(nodes - index - 1).ok()?; + let count = driver.gen_u8(Included(&0), Included(&behind.min(2)))?; + let mut edges = Vec::new(); + for _ in 0..count { + edges.push(driver.gen_u8(Included(&1), Included(&behind.max(1)))?); + } + shape.push(edges); + grounded.push(driver.produce::()?); + } + Some(Dag { shape, grounded }) + } + } + + fn realize(dag: &Dag) -> (NhopStore, Vec>) { + let mut store = NhopStore::new(); + let nodes: Vec> = (0..dag.shape.len()) + .map(|index| { + let raw = u8::try_from(index).unwrap_or_else(|_| unreachable!()); + let mut key = NhopKey::from_address(&format!("10.0.0.{}", raw + 1)); + if dag.grounded[index] { + key.ifindex = Some( + InterfaceIndex::try_new(u32::from(raw) + 1) + .unwrap_or_else(|_| unreachable!()), + ); + } + store.add_nhop(&key) + }) + .collect(); + + for (from, to) in dag.edges() { + nodes[from].add_resolver(&nodes[to]); + } + (store, nodes) + } + + fn expected(node: &Rc, prefix: &FibEntry, out: &mut Vec) { + let mut entry = prefix.clone(); + entry.extend_from_slice(&node.instructions.borrow().clone()); + + let resolvers: Vec> = node + .resolvers + .borrow() + .iter() + .filter_map(Weak::upgrade) + .collect(); + + if resolvers.is_empty() { + if node.must_be_resolved() { + return; + } + entry.squash(); + if entry.is_valid() { + out.push(entry); + } + } else { + for resolver in resolvers { + expected(&resolver, &entry, out); + } + } + } + + #[test] + fn a_fibgroup_is_the_usable_paths_through_the_graph() { + let rstore = RmacStore::new(); + bolero::check!() + .with_generator(Graphs) + .cloned() + .for_each(|dag: Dag| { + let (_store, nodes) = realize(&dag); + for node in &nodes { + node.build_nhop_instructions(&rstore); + } + + let root = &nodes[0]; + let mut want = Vec::new(); + expected(root, &FibEntry::new(), &mut want); + if want.is_empty() { + want.push(FibEntry::drop_fibentry()); + } + + let got = root.build_nhop_fibgroup(); + assert_eq!(got.entries(), &want, "for {dag:?}"); + }); + } + + #[test] + fn every_entry_in_a_fibgroup_is_usable() { + let rstore = RmacStore::new(); + bolero::check!() + .with_generator(Graphs) + .cloned() + .for_each(|dag: Dag| { + let (_store, nodes) = realize(&dag); + for node in &nodes { + node.build_nhop_instructions(&rstore); + } + let group = nodes[0].build_nhop_fibgroup(); + assert!(!group.is_empty(), "for {dag:?}"); + for entry in group.iter() { + assert!(entry.is_valid(), "unusable entry {entry:?} for {dag:?}"); + } + }); + } + + #[test] + fn resolves_with_answers_reachability() { + bolero::check!() + .with_generator(Graphs) + .cloned() + .for_each(|dag: Dag| { + let (_store, nodes) = realize(&dag); + for (from, node) in nodes.iter().enumerate() { + let reachable = dag.reachable_from(from); + for (to, other) in nodes.iter().enumerate() { + assert_eq!( + node.resolves_with(other), + reachable[to], + "{from} -> {to}, for {dag:?}" + ); + } + } + }); + } + + #[test] + fn a_next_hop_resolves_via_itself() { + bolero::check!() + .with_generator(Graphs) + .cloned() + .for_each(|dag: Dag| { + let (_store, nodes) = realize(&dag); + for node in &nodes { + assert!(node.resolves_with(node)); + } + }); + } +} From f8914e386fbe8590f5517b3a8ad2819dca0f9d71 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 13:00:51 -0600 Subject: [PATCH 02/14] fix(routing): guard against resolution loops Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- routing/src/cli/display.rs | 57 +++++---- routing/src/rib/nexthop.rs | 239 ++++++++++++++++++++++++------------- routing/src/rib/rib2fib.rs | 28 ++++- 3 files changed, 215 insertions(+), 109 deletions(-) diff --git a/routing/src/cli/display.rs b/routing/src/cli/display.rs index 9c40338de0..a5c54cb9c8 100644 --- a/routing/src/cli/display.rs +++ b/routing/src/cli/display.rs @@ -21,7 +21,7 @@ use crate::router::cpi::{CpiStats, CpiStatus, StatsRow}; use crate::rib::VrfTable; use crate::rib::encapsulation::{Encapsulation, VxlanEncapsulation}; -use crate::rib::nexthop::{FwAction, Nhop, NhopKey, NhopStore}; +use crate::rib::nexthop::{FwAction, Nhop, NhopKey, NhopStore, Visited}; use crate::rib::vrf::{Route, RouteFlags, RouteOrigin, ShimNhop, Vrf, VrfStatus}; use crate::interfaces::iftable::IfTable; @@ -40,7 +40,7 @@ use net::vxlan::Vni; use std::fmt::Display; use std::fmt::Write; use std::os::unix::net::SocketAddr; -use std::rc::Rc; +use std::rc::{Rc, Weak}; use std::time::Duration; use std::time::Instant; @@ -126,27 +126,34 @@ impl Display for Nhop { if self.is_unresolved() { write!(f, " (unresolved)")?; } - fmt_nhop_resolvers(f, self, 2) + fmt_nhop_resolvers(f, self, 2, &mut vec![self.id()]) } } -fn fmt_nhop_resolvers(f: &mut std::fmt::Formatter<'_>, rc: &Nhop, depth: u8) -> std::fmt::Result { +fn fmt_nhop_resolvers( + f: &mut std::fmt::Formatter<'_>, + rc: &Nhop, + depth: u8, + path: &mut Visited, +) -> std::fmt::Result { let Ok(resolvers) = rc.resolvers.try_borrow() else { warn!("Try-borrow on nhop resolvers failed!"); return Ok(()); }; let tab = 5 * depth as usize; let indent = " ".repeat(tab); - if !resolvers.is_empty() { - for r in resolvers.iter() { - if let Some(r) = r.upgrade().as_ref() { - write!(f, "\n{indent} {}", r.key)?; - if r.is_unresolved() { - write!(f, " (UNRESOLVED)")?; - } - fmt_nhop_resolvers(f, r, depth + 1)?; - } + for r in resolvers.iter().filter_map(Weak::upgrade) { + write!(f, "\n{indent} {}", r.key)?; + if r.is_unresolved() { + write!(f, " (UNRESOLVED)")?; + } + if path.contains(&r.id()) { + write!(f, " (LOOP)")?; + continue; } + path.push(r.id()); + fmt_nhop_resolvers(f, &r, depth.saturating_add(1), path)?; + path.pop(); } Ok(()) } @@ -168,7 +175,12 @@ fn fmt_nhop_instruction(f: &mut std::fmt::Formatter<'_>, rc: &Nhop) -> std::fmt: // formats nhop using the display of the key, recoursing over resolvers // Does not use Nhop::fmt(). -fn fmt_nhop_rec(f: &mut std::fmt::Formatter<'_>, rc: &Rc, depth: u8) -> std::fmt::Result { +fn fmt_nhop_rec( + f: &mut std::fmt::Formatter<'_>, + rc: &Rc, + depth: u8, + path: &mut Visited, +) -> std::fmt::Result { let tab = 8 * depth as usize; let indent = " ".repeat(tab); @@ -184,6 +196,9 @@ fn fmt_nhop_rec(f: &mut std::fmt::Formatter<'_>, rc: &Rc, depth: u8) -> st if rc.is_unresolved() { write!(f, " (UNRESOLVED)")?; } + if path.contains(&rc.id()) { + return writeln!(f, " (LOOP)"); + } writeln!(f)?; // fmt_nhop_instruction(f, rc)?; @@ -191,11 +206,11 @@ fn fmt_nhop_rec(f: &mut std::fmt::Formatter<'_>, rc: &Rc, depth: u8) -> st error!("Try-borrow on next-hop resolvers failed!"); return Ok(()); }; - for r in resolvers.iter() { - if let Some(r) = r.upgrade().as_ref() { - fmt_nhop_rec(f, r, depth + 1)?; - } + path.push(rc.id()); + for r in resolvers.iter().filter_map(Weak::upgrade) { + fmt_nhop_rec(f, &r, depth.saturating_add(1), path)?; } + path.pop(); // if let Ok(fg) = rc.as_ref().fibgroup.read() { // writeln!(f, "FibG {}", fg)?; // } @@ -206,7 +221,7 @@ impl Display for NhopStore { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { Heading(format!("Next-hop Store ({})", self.len())).fmt(f)?; for nhop in self.iter() { - fmt_nhop_rec(f, nhop, 0)?; + fmt_nhop_rec(f, nhop, 0, &mut Visited::new())?; fmt_nhop_instruction(f, nhop)?; } line(f) @@ -407,7 +422,7 @@ impl Display for VrfV4Nexthops<'_> { .filter(|nh| nh.key.address.is_none_or(|a| a.is_ipv4())); for nhop in iter { - fmt_nhop_rec(f, nhop, 0)?; + fmt_nhop_rec(f, nhop, 0, &mut Visited::new())?; } line(f) } @@ -425,7 +440,7 @@ impl Display for VrfV6Nexthops<'_> { .filter(|nh| nh.key.address.is_none_or(|a| a.is_ipv6())); for nhop in iter { - fmt_nhop_rec(f, nhop, 0)?; + fmt_nhop_rec(f, nhop, 0, &mut Visited::new())?; } line(f) } diff --git a/routing/src/rib/nexthop.rs b/routing/src/rib/nexthop.rs index b2757462ac..562ad95a8a 100644 --- a/routing/src/rib/nexthop.rs +++ b/routing/src/rib/nexthop.rs @@ -161,6 +161,10 @@ impl Hash for Nhop { } } +pub(crate) type NhopId = *const Nhop; + +pub(crate) type Visited = Vec; + impl Nhop { /// Create a new Nhop object from a key object fn from_key(key: &NhopKey) -> Self { @@ -184,23 +188,30 @@ impl Nhop { self } - /// Recursive method to check if a next-hop resolves via another, `checked`. - /// We use this method to avoid resolution loops that would happen in case of routing loops. - /// Resolution loops would cause us to stack overflow. This method is recursive, but - /// short-circuits in case of loop. The method takes the advantage that there cannot be two - /// next-hops with the same key. + pub(crate) fn id(&self) -> NhopId { + std::ptr::from_ref(self) + } + fn resolves_with(&self, checked: &Nhop) -> bool { - // resolve to oneself is forbidden - if self.key == checked.key { + self.resolves_with_rec(checked, &mut Visited::new()) + } + + fn resolves_with_rec(&self, checked: &Nhop, visited: &mut Visited) -> bool { + if self.id() == checked.id() { error!("Loop detected for next-hop {}!", self.key); return true; } + if visited.contains(&self.id()) { + return false; + } + visited.push(self.id()); + // resolvers should not refer back to the checked next-hop let resolvers = self.resolvers.borrow(); resolvers .iter() .filter_map(Weak::upgrade) - .any(|res| res.resolves_with(checked)) + .any(|res| res.resolves_with_rec(checked, visited)) } /// Tell if a next-hop requires resolution @@ -263,9 +274,13 @@ impl Nhop { self.resolvers.replace(resolvers); } - /// Auxiliary recursive method used by `Nhop::quick_resolve()`. #[cfg(test)] - fn quick_resolve_rec(&self, result: &mut BTreeSet) { + fn quick_resolve_rec(&self, result: &mut BTreeSet, visited: &mut Visited) { + if visited.contains(&self.id()) { + return; + } + visited.push(self.id()); + let Ok(resolvers) = self.resolvers.try_borrow_mut() else { error!("Try-borrow-mut() failed on next-hop resolvers!"); return; @@ -297,7 +312,7 @@ impl Nhop { self.key.ifname.clone(), )); } else { - r.quick_resolve_rec(result); + r.quick_resolve_rec(result, visited); } } } @@ -311,7 +326,7 @@ impl Nhop { #[cfg(test)] pub fn quick_resolve(&self) -> BTreeSet { let mut out: BTreeSet = BTreeSet::new(); - self.quick_resolve_rec(&mut out); + self.quick_resolve_rec(&mut out, &mut Visited::new()); out } } @@ -1044,6 +1059,25 @@ mod tests { a.add_resolver(&checked); assert!(a.resolves_with(checked.as_ref())); } + + #[cfg_attr(not(emulated), traced_test)] + #[test] + fn test_display_of_a_resolution_loop_terminates() { + let mut store = NhopStore::new(); + let a = store.add_nhop(&NhopKey::from_address("7.0.0.1")); + let b = store.add_nhop(&NhopKey::from_address("8.0.0.2")); + a.add_resolver(&b); + b.add_resolver(&a); + + let nhop = format!("{a}"); + assert!(nhop.contains("(LOOP)"), "loop not reported in {nhop}"); + + let whole_store = format!("{store}"); + assert!( + whole_store.contains("(LOOP)"), + "loop not reported in {whole_store}" + ); + } } #[cfg(test)] @@ -1054,40 +1088,23 @@ mod fibgroup_properties { use std::ops::Bound::Included; const MAX_NODES: u8 = 6; + const MAX_RESOLVERS: u8 = 2; #[derive(Debug, Clone)] - struct Dag { - shape: Vec>, + struct Graph { + edges: Vec>, grounded: Vec, } - impl Dag { - fn edges(&self) -> Vec<(usize, usize)> { - let mut edges = Vec::new(); - for (from, offsets) in self.shape.iter().enumerate() { - for offset in offsets { - let to = from + usize::from(*offset); - if to < self.shape.len() { - edges.push((from, to)); - } - } - } - edges - } - + impl Graph { fn reachable_from(&self, start: usize) -> Vec { - let edges = self.edges(); - let mut seen = vec![false; self.shape.len()]; + let mut seen = vec![false; self.edges.len()]; let mut stack = vec![start]; while let Some(node) = stack.pop() { if std::mem::replace(&mut seen[node], true) { continue; } - for (from, to) in &edges { - if *from == node { - stack.push(*to); - } - } + stack.extend_from_slice(&self.edges[node]); } seen } @@ -1097,33 +1114,33 @@ mod fibgroup_properties { struct Graphs; impl ValueGenerator for Graphs { - type Output = Dag; + type Output = Graph; - fn generate(&self, driver: &mut D) -> Option { + fn generate(&self, driver: &mut D) -> Option { let nodes = usize::from(driver.gen_u8(Included(&1), Included(&MAX_NODES))?); - let mut shape = Vec::with_capacity(nodes); + let last = u8::try_from(nodes - 1).ok()?; + let mut edges = Vec::with_capacity(nodes); let mut grounded = Vec::with_capacity(nodes); - for index in 0..nodes { - let behind = u8::try_from(nodes - index - 1).ok()?; - let count = driver.gen_u8(Included(&0), Included(&behind.min(2)))?; - let mut edges = Vec::new(); + for _ in 0..nodes { + let count = driver.gen_u8(Included(&0), Included(&MAX_RESOLVERS))?; + let mut resolvers = Vec::with_capacity(usize::from(count)); for _ in 0..count { - edges.push(driver.gen_u8(Included(&1), Included(&behind.max(1)))?); + resolvers.push(usize::from(driver.gen_u8(Included(&0), Included(&last))?)); } - shape.push(edges); + edges.push(resolvers); grounded.push(driver.produce::()?); } - Some(Dag { shape, grounded }) + Some(Graph { edges, grounded }) } } - fn realize(dag: &Dag) -> (NhopStore, Vec>) { + fn realize(graph: &Graph) -> (NhopStore, Vec>) { let mut store = NhopStore::new(); - let nodes: Vec> = (0..dag.shape.len()) + let nodes: Vec> = (0..graph.edges.len()) .map(|index| { let raw = u8::try_from(index).unwrap_or_else(|_| unreachable!()); let mut key = NhopKey::from_address(&format!("10.0.0.{}", raw + 1)); - if dag.grounded[index] { + if graph.grounded[index] { key.ifindex = Some( InterfaceIndex::try_new(u32::from(raw) + 1) .unwrap_or_else(|_| unreachable!()), @@ -1133,36 +1150,44 @@ mod fibgroup_properties { }) .collect(); - for (from, to) in dag.edges() { - nodes[from].add_resolver(&nodes[to]); + for (from, resolvers) in graph.edges.iter().enumerate() { + for to in resolvers { + nodes[from].add_resolver(&nodes[*to]); + } } (store, nodes) } - fn expected(node: &Rc, prefix: &FibEntry, out: &mut Vec) { - let mut entry = prefix.clone(); - entry.extend_from_slice(&node.instructions.borrow().clone()); + fn expected( + graph: &Graph, + nodes: &[Rc], + from: usize, + path: &mut Vec, + prefix: &FibEntry, + out: &mut Vec, + ) { + if path.contains(&from) { + return; + } + path.push(from); - let resolvers: Vec> = node - .resolvers - .borrow() - .iter() - .filter_map(Weak::upgrade) - .collect(); + let mut entry = prefix.clone(); + entry.extend_from_slice(&nodes[from].instructions.borrow()); - if resolvers.is_empty() { - if node.must_be_resolved() { - return; - } - entry.squash(); - if entry.is_valid() { - out.push(entry); + if graph.edges[from].is_empty() { + if !nodes[from].must_be_resolved() { + entry.squash(); + if entry.is_valid() { + out.push(entry); + } } } else { - for resolver in resolvers { - expected(&resolver, &entry, out); + for to in &graph.edges[from] { + expected(graph, nodes, *to, path, &entry, out); } } + + path.pop(); } #[test] @@ -1171,21 +1196,27 @@ mod fibgroup_properties { bolero::check!() .with_generator(Graphs) .cloned() - .for_each(|dag: Dag| { - let (_store, nodes) = realize(&dag); + .for_each(|graph: Graph| { + let (_store, nodes) = realize(&graph); for node in &nodes { node.build_nhop_instructions(&rstore); } - let root = &nodes[0]; let mut want = Vec::new(); - expected(root, &FibEntry::new(), &mut want); + expected( + &graph, + &nodes, + 0, + &mut Vec::new(), + &FibEntry::new(), + &mut want, + ); if want.is_empty() { want.push(FibEntry::drop_fibentry()); } - let got = root.build_nhop_fibgroup(); - assert_eq!(got.entries(), &want, "for {dag:?}"); + let got = nodes[0].build_nhop_fibgroup(); + assert_eq!(got.entries(), &want, "for {graph:?}"); }); } @@ -1195,33 +1226,54 @@ mod fibgroup_properties { bolero::check!() .with_generator(Graphs) .cloned() - .for_each(|dag: Dag| { - let (_store, nodes) = realize(&dag); + .for_each(|graph: Graph| { + let (_store, nodes) = realize(&graph); for node in &nodes { node.build_nhop_instructions(&rstore); } let group = nodes[0].build_nhop_fibgroup(); - assert!(!group.is_empty(), "for {dag:?}"); + assert!(!group.is_empty(), "for {graph:?}"); for entry in group.iter() { - assert!(entry.is_valid(), "unusable entry {entry:?} for {dag:?}"); + assert!(entry.is_valid(), "unusable entry {entry:?} for {graph:?}"); } }); } + #[test] + fn a_next_hop_in_a_resolution_loop_drops() { + let rstore = RmacStore::new(); + let mut store = NhopStore::new(); + + let a = store.add_nhop(&NhopKey::from_address("7.0.0.1")); + let b = store.add_nhop(&NhopKey::from_address("8.0.0.2")); + let c = store.add_nhop(&NhopKey::from_address("9.0.0.3")); + a.add_resolver(&b); + b.add_resolver(&c); + c.add_resolver(&a); + store.rebuild_nhop_instructions(&rstore); + + let group = a.build_nhop_fibgroup(); + assert_eq!( + group.entries(), + &vec![FibEntry::drop_fibentry()], + "a packet caught in a routing loop must be dropped" + ); + } + #[test] fn resolves_with_answers_reachability() { bolero::check!() .with_generator(Graphs) .cloned() - .for_each(|dag: Dag| { - let (_store, nodes) = realize(&dag); + .for_each(|graph: Graph| { + let (_store, nodes) = realize(&graph); for (from, node) in nodes.iter().enumerate() { - let reachable = dag.reachable_from(from); + let reachable = graph.reachable_from(from); for (to, other) in nodes.iter().enumerate() { assert_eq!( node.resolves_with(other), reachable[to], - "{from} -> {to}, for {dag:?}" + "{from} -> {to}, for {graph:?}" ); } } @@ -1233,11 +1285,30 @@ mod fibgroup_properties { bolero::check!() .with_generator(Graphs) .cloned() - .for_each(|dag: Dag| { - let (_store, nodes) = realize(&dag); + .for_each(|graph: Graph| { + let (_store, nodes) = realize(&graph); for node in &nodes { assert!(node.resolves_with(node)); } }); } + + #[test] + fn no_next_hop_resolves_via_another_store() { + bolero::check!() + .with_generator(Graphs) + .cloned() + .for_each(|graph: Graph| { + let (_here, here) = realize(&graph); + let (_there, there) = realize(&graph); + for (from, node) in here.iter().enumerate() { + for (to, other) in there.iter().enumerate() { + assert!( + !node.resolves_with(other), + "{from} resolves via {to} in another store, for {graph:?}" + ); + } + } + }); + } } diff --git a/routing/src/rib/rib2fib.rs b/routing/src/rib/rib2fib.rs index 99ae73cd0f..dbf0d8f07b 100644 --- a/routing/src/rib/rib2fib.rs +++ b/routing/src/rib/rib2fib.rs @@ -9,7 +9,7 @@ use tracing::{debug, trace, warn}; use crate::evpn::RmacStore; use crate::fib::fibobjects::{EgressObject, FibEntry, FibGroup, PktInstruction}; use crate::rib::encapsulation::{Encapsulation, VxlanEncapsulation}; -use crate::rib::nexthop::{FwAction, Nhop}; +use crate::rib::nexthop::{FwAction, Nhop, Visited}; use crate::rib::vrf::RouteOrigin; use std::rc::Weak; @@ -104,7 +104,27 @@ impl Nhop { /// Recursive helper to build [`FibGroup`] for a next-hop. We accumulate /// a next-hop's packet instructions with those of its resolvers. ////////////////////////////////////////////////////////////////////// - fn build_nhop_fibgroup_rec(&self, fibgroup: &mut FibGroup, mut entry: FibEntry) { + fn build_nhop_fibgroup_rec( + &self, + fibgroup: &mut FibGroup, + entry: FibEntry, + path: &mut Visited, + ) { + if path.contains(&self.id()) { + warn!("Resolution loop at next-hop {self}: will not use this path"); + return; + } + path.push(self.id()); + self.build_nhop_fibgroup_visit(fibgroup, entry, path); + path.pop(); + } + + fn build_nhop_fibgroup_visit( + &self, + fibgroup: &mut FibGroup, + mut entry: FibEntry, + path: &mut Visited, + ) { // add the instructions for a next-hop to the entry let instructions = self.instructions.borrow().clone(); entry.extend_from_slice(&instructions); @@ -136,7 +156,7 @@ impl Nhop { } } else { for resolver in resolvers.iter().filter_map(Weak::upgrade) { - resolver.build_nhop_fibgroup_rec(fibgroup, entry.clone()); + resolver.build_nhop_fibgroup_rec(fibgroup, entry.clone(), path); } } } @@ -149,7 +169,7 @@ impl Nhop { ////////////////////////////////////////////////////////////////////// pub(crate) fn build_nhop_fibgroup(&self) -> FibGroup { let mut fibgroup = FibGroup::new(); - self.build_nhop_fibgroup_rec(&mut fibgroup, FibEntry::new()); + self.build_nhop_fibgroup_rec(&mut fibgroup, FibEntry::new(), &mut Visited::new()); if fibgroup.is_empty() { warn!("Next-hop {self} has empty fibgroup: will add DROP FibEntry"); fibgroup.add(FibEntry::drop_fibentry()); From dc156f2e2f2de0d895fa0c7cf634823d6b76b3e9 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 13:17:55 -0600 Subject: [PATCH 03/14] test(routing): model-check fib Use changelog to model check fib. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- routing/src/fib/fibtype.rs | 344 +++++++++++++++++++++++++++++++++++++ 1 file changed, 344 insertions(+) diff --git a/routing/src/fib/fibtype.rs b/routing/src/fib/fibtype.rs index ea368bb36b..4ed213948c 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -504,3 +504,347 @@ impl FibReaderFactory { FibReader(self.0.handle()) } } + +#[cfg(test)] +mod fib_properties { + use super::*; + use crate::fib::fibgroupstore::tests::{build_fib_entry_egress, build_fibgroup}; + use bolero::{Driver, ValueGenerator}; + use std::collections::{BTreeMap, BTreeSet}; + use std::ops::Bound::Included; + use std::str::FromStr; + + const NUM_NHOPS: u8 = 4; + const NUM_PREFIXES: u8 = 8; + const NUM_ENTRIES: u8 = 4; + const MAX_CHANGES: u8 = 12; + const MAX_KEYS_PER_ROUTE: u8 = 3; + const MAX_ENTRIES_PER_GROUP: u8 = 3; + + const DROP_KEY: usize = 0; + const ROOT_V4: usize = 0; + const ROOT_V6: usize = 5; + + fn nhop_keys() -> Vec { + vec![ + NhopKey::with_drop(), + NhopKey::with_addr_ifindex("10.0.0.1", 1), + NhopKey::with_addr_ifindex("10.0.0.2", 2), + NhopKey::with_ifindex(3), + ] + } + + fn prefixes() -> Vec { + [ + "0.0.0.0/0", + "10.0.0.0/8", + "10.1.0.0/16", + "10.1.2.0/24", + "10.1.2.3/32", + "::/0", + "2001:db8::/32", + "2001:db8:1::/48", + ] + .iter() + .map(|p| Prefix::from_str(p).unwrap_or_else(|_| unreachable!())) + .collect() + } + + fn probes() -> Vec { + [ + "9.9.9.9", + "10.9.9.9", + "10.1.9.9", + "10.1.2.9", + "10.1.2.3", + "2000::1", + "2001:db8::1", + "2001:db8:1::1", + ] + .iter() + .map(|a| IpAddr::from_str(a).unwrap_or_else(|_| unreachable!())) + .collect() + } + + fn entry_pool() -> Vec { + (1..=u32::from(NUM_ENTRIES)) + .map(|i| build_fib_entry_egress(i, &format!("10.0.9.{i}"), &format!("eth{i}"))) + .collect() + } + + #[derive(Debug, Clone)] + enum Change { + RegisterGroup { key: usize, entries: Vec }, + UnregisterGroup { key: usize }, + AddRoute { prefix: usize, keys: Vec }, + DelRoute { prefix: usize }, + } + + #[derive(Debug, Clone, Copy, Default)] + struct ChangeSequences; + + fn index(driver: &mut D, count: u8) -> Option { + driver + .gen_u8(Included(&0), Included(&(count - 1))) + .map(usize::from) + } + + fn indices(driver: &mut D, count: u8, most: u8) -> Option> { + let len = driver.gen_u8(Included(&0), Included(&most))?; + let mut out = Vec::with_capacity(usize::from(len)); + for _ in 0..len { + out.push(index(driver, count)?); + } + Some(out) + } + + impl ValueGenerator for ChangeSequences { + type Output = Vec; + + fn generate(&self, driver: &mut D) -> Option> { + let len = driver.gen_u8(Included(&0), Included(&MAX_CHANGES))?; + let mut out = Vec::with_capacity(usize::from(len)); + for _ in 0..len { + let change = match driver.gen_u8(Included(&0), Included(&3))? { + 0 => Change::RegisterGroup { + key: index(driver, NUM_NHOPS)?, + entries: indices(driver, NUM_ENTRIES, MAX_ENTRIES_PER_GROUP)?, + }, + 1 => Change::UnregisterGroup { + key: index(driver, NUM_NHOPS)?, + }, + 2 => Change::AddRoute { + prefix: index(driver, NUM_PREFIXES)?, + keys: indices(driver, NUM_NHOPS, MAX_KEYS_PER_ROUTE)?, + }, + _ => Change::DelRoute { + prefix: index(driver, NUM_PREFIXES)?, + }, + }; + out.push(change); + } + Some(out) + } + } + + #[derive(Debug, Clone)] + struct Model { + groups: BTreeMap>, + routes: BTreeMap>, + } + + impl Model { + fn new() -> Self { + Self { + groups: BTreeMap::from([(DROP_KEY, vec![FibEntry::drop_fibentry()])]), + routes: BTreeMap::from([(ROOT_V4, vec![DROP_KEY]), (ROOT_V6, vec![DROP_KEY])]), + } + } + + fn referenced(&self, key: usize) -> bool { + self.routes.values().any(|keys| keys.contains(&key)) + } + + fn purge(&mut self) { + let referenced: BTreeSet = self + .routes + .values() + .flatten() + .copied() + .collect::>(); + self.groups + .retain(|key, _| *key == DROP_KEY || referenced.contains(key)); + } + + fn apply(&mut self, change: &Change, pool: &[FibEntry]) { + match change { + Change::RegisterGroup { key, entries } => { + if entries.is_empty() { + return; + } + let entries = entries.iter().map(|i| pool[*i].clone()).collect(); + self.groups.insert(*key, entries); + } + Change::UnregisterGroup { key } => { + if *key == DROP_KEY || self.referenced(*key) { + return; + } + self.groups.remove(key); + } + Change::AddRoute { prefix, keys } => { + if keys.is_empty() || keys.iter().any(|k| !self.groups.contains_key(k)) { + return; + } + self.routes.insert(*prefix, keys.clone()); + } + Change::DelRoute { prefix } => { + let removed = if *prefix == ROOT_V4 || *prefix == ROOT_V6 { + self.routes.insert(*prefix, vec![DROP_KEY]) + } else { + self.routes.remove(prefix) + }; + if removed.is_some() { + self.purge(); + } + } + } + } + + fn lpm(&self, addr: &IpAddr, prefixes: &[Prefix]) -> Option { + self.routes + .keys() + .copied() + .filter(|i| prefixes[*i].covers_addr(addr)) + .max_by_key(|i| prefixes[*i].length()) + } + + fn entries_for(&self, prefix: usize) -> Vec { + self.routes[&prefix] + .iter() + .flat_map(|key| self.groups[key].iter().cloned()) + .collect() + } + } + + fn apply_to_fib(writer: &mut FibWriter, change: &Change, pool: &[FibEntry], keys: &[NhopKey]) { + let prefixes = prefixes(); + match change { + Change::RegisterGroup { key, entries } => { + let entries: Vec = entries.iter().map(|i| pool[*i].clone()).collect(); + writer.register_fibgroup(&keys[*key], &build_fibgroup(&entries), true); + } + Change::UnregisterGroup { key } => writer.unregister_fibgroup(&keys[*key], true), + Change::AddRoute { + prefix, + keys: route, + } => { + let route = route.iter().map(|k| keys[*k].clone()).collect(); + writer.add_fibroute(prefixes[*prefix], route, true); + } + Change::DelRoute { prefix } => writer.del_fibroute(prefixes[*prefix]), + } + } + + #[test] + fn the_pools_are_the_size_the_generator_thinks() { + assert_eq!(nhop_keys().len(), usize::from(NUM_NHOPS)); + assert_eq!(prefixes().len(), usize::from(NUM_PREFIXES)); + assert_eq!(entry_pool().len(), usize::from(NUM_ENTRIES)); + assert_eq!(nhop_keys()[DROP_KEY], NhopKey::with_drop()); + assert_eq!(prefixes()[ROOT_V4], Prefix::root_v4()); + assert_eq!(prefixes()[ROOT_V6], Prefix::root_v6()); + for probe in probes() { + assert!( + prefixes().iter().any(|p| p.covers_addr(&probe)), + "probe {probe} is covered by no prefix, not even a root" + ); + } + } + + #[test] + fn a_fib_answers_lookups_the_way_the_model_says() { + let keys = nhop_keys(); + let prefixes = prefixes(); + let probes = probes(); + let pool = entry_pool(); + + bolero::check!() + .with_generator(ChangeSequences) + .cloned() + .for_each(|changes: Vec| { + let (mut writer, _reader) = FibWriter::new(FibKey::from_vrfid(1)); + let mut model = Model::new(); + + for (step, change) in changes.iter().enumerate() { + apply_to_fib(&mut writer, change, &pool, &keys); + model.apply(change, &pool); + + let fib = writer.enter().unwrap_or_else(|| unreachable!()); + let at = || format!("at step {step} of {changes:?}"); + + assert_eq!(fib.len_groups(), model.groups.len(), "{}", at()); + + for probe in &probes { + let want = model + .lpm(probe, &prefixes) + .unwrap_or_else(|| panic!("model has no route for {probe} {}", at())); + + let (hit, route) = fib.lpm_with_prefix(probe); + assert_eq!(hit, prefixes[want], "for {probe} {}", at()); + + let got: Vec = route + .iter() + .flat_map(|group| group.entries().iter().cloned()) + .collect(); + assert_eq!(got, model.entries_for(want), "for {probe} {}", at()); + } + } + }); + } + + #[test] + fn every_route_a_lookup_reaches_has_an_entry_to_execute() { + let keys = nhop_keys(); + let probes = probes(); + let pool = entry_pool(); + + bolero::check!() + .with_generator(ChangeSequences) + .cloned() + .for_each(|changes: Vec| { + let (mut writer, _reader) = FibWriter::new(FibKey::from_vrfid(1)); + for change in &changes { + apply_to_fib(&mut writer, change, &pool, &keys); + } + + let fib = writer.enter().unwrap_or_else(|| unreachable!()); + for probe in &probes { + let (_, route) = fib.lpm_with_prefix(probe); + assert!(route.len() > 0, "no entry for {probe} after {changes:?}"); + for index in 0..route.len() { + let _ = route.get_fibentry(index); + } + } + }); + } + + #[test] + fn a_reader_and_a_writer_agree_after_publishing() { + let keys = nhop_keys(); + let probes = probes(); + let pool = entry_pool(); + + bolero::check!() + .with_generator(ChangeSequences) + .cloned() + .for_each(|changes: Vec| { + let (mut writer, reader) = FibWriter::new(FibKey::from_vrfid(1)); + for change in &changes { + apply_to_fib(&mut writer, change, &pool, &keys); + } + + for probe in &probes { + let (want_prefix, want_entries) = { + let fib = writer.enter().unwrap_or_else(|| unreachable!()); + let (prefix, route) = fib.lpm_with_prefix(probe); + let entries: Vec = route + .iter() + .flat_map(|group| group.entries().iter().cloned()) + .collect(); + (prefix, entries) + }; + + let (got_prefix, route) = reader + .lpm_route_with_prefix(*probe) + .unwrap_or_else(|| unreachable!()); + let got_entries: Vec = route + .iter() + .flat_map(|group| group.entries().iter().cloned()) + .collect(); + + assert_eq!(got_prefix, want_prefix, "for {probe} after {changes:?}"); + assert_eq!(got_entries, want_entries, "for {probe} after {changes:?}"); + } + }); + } +} From dd3a6bd0fe81e91ad1263011607333f48538f46f Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 13:18:08 -0600 Subject: [PATCH 04/14] fix(routing): remove fib vni aliases on delete A fib may be indexed by both id and vni. Make sure to delete every entry referring to the target fib so no alias outlives its writer. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- routing/src/fib/fibtable.rs | 190 ++++++++++++++++++++++++++++++++++-- routing/src/fib/test.rs | 4 +- routing/src/rib/vrftable.rs | 2 +- 3 files changed, 185 insertions(+), 11 deletions(-) diff --git a/routing/src/fib/fibtable.rs b/routing/src/fib/fibtable.rs index 5fbab2a74a..72a5d46141 100644 --- a/routing/src/fib/fibtable.rs +++ b/routing/src/fib/fibtable.rs @@ -38,10 +38,9 @@ impl FibTable { info!("Registering Fib with id {id} in the FibTable"); self.entries.insert(id, entry); } - /// Delete a `Fib`, by unregistering a `FibReaderFactory` for it fn del_fib(&mut self, id: FibKey) { info!("Unregistering Fib with id {id} from the FibTable"); - self.entries.remove(&id); + self.entries.retain(|_, entry| entry.id != id); } /// Register an existing `Fib` with a given [`Vni`]. /// This allows looking up a Fib (`FibReaderFactory`) from a [`Vni`] @@ -144,12 +143,9 @@ impl FibTableWriter { self.0.append(FibTableChange::UnRegisterVni(vni)); self.0.publish(); } - pub fn del_fib(&mut self, vrfid: VrfId, vni: Option) { - let fibid = FibKey::from_vrfid(vrfid); - self.0.append(FibTableChange::Del(fibid)); - if let Some(vni) = vni { - self.0.append(FibTableChange::UnRegisterVni(vni)); - } + pub fn del_fib(&mut self, vrfid: VrfId) { + self.0 + .append(FibTableChange::Del(FibKey::from_vrfid(vrfid))); self.0.publish(); } } @@ -234,3 +230,181 @@ impl FibTableReader { Ok(FibReader::rc_from_rc_rhandle(rhandle)) } } + +#[cfg(test)] +mod fibtable_properties { + use super::*; + use crate::fib::fibtype::FibWriter; + use bolero::{Driver, ValueGenerator}; + use std::ops::Bound::Included; + + const NUM_VRFS: u8 = 3; + const NUM_VNIS: u8 = 2; + const MAX_CHANGES: u8 = 10; + + fn vrf_ids() -> Vec { + (0..u32::from(NUM_VRFS)).collect() + } + + fn vnis() -> Vec { + (1..=u32::from(NUM_VNIS)) + .map(|i| Vni::new_checked(100 * i).unwrap_or_else(|_| unreachable!())) + .collect() + } + + fn keys() -> Vec { + vrf_ids() + .into_iter() + .map(FibKey::from_vrfid) + .chain(vnis().into_iter().map(FibKey::from_vni)) + .collect() + } + + #[derive(Debug, Clone)] + enum Change { + AddFib { vrf: usize, vni: Option }, + RegisterByVni { vrf: usize, vni: usize }, + UnregisterVni { vni: usize }, + DelFib { vrf: usize }, + } + + #[derive(Debug, Clone, Copy, Default)] + struct ChangeSequences; + + fn index(driver: &mut D, count: u8) -> Option { + driver + .gen_u8(Included(&0), Included(&(count - 1))) + .map(usize::from) + } + + impl ValueGenerator for ChangeSequences { + type Output = Vec; + + fn generate(&self, driver: &mut D) -> Option> { + let len = driver.gen_u8(Included(&0), Included(&MAX_CHANGES))?; + let mut out = Vec::with_capacity(usize::from(len)); + for _ in 0..len { + let change = match driver.gen_u8(Included(&0), Included(&3))? { + 0 => { + let vrf = index(driver, NUM_VRFS)?; + let drawn = index(driver, NUM_VNIS + 1)?; + Change::AddFib { + vrf, + vni: (drawn < usize::from(NUM_VNIS)).then_some(drawn), + } + } + 1 => Change::RegisterByVni { + vrf: index(driver, NUM_VRFS)?, + vni: index(driver, NUM_VNIS)?, + }, + 2 => Change::UnregisterVni { + vni: index(driver, NUM_VNIS)?, + }, + _ => Change::DelFib { + vrf: index(driver, NUM_VRFS)?, + }, + }; + out.push(change); + } + Some(out) + } + } + + type Model = BTreeMap; + + struct Fibs { + live: BTreeMap, + retired: Vec, + } + + fn apply(table: &mut FibTableWriter, fibs: &mut Fibs, model: &mut Model, change: &Change) { + let vrfs = vrf_ids(); + let all_vnis = vnis(); + match change { + Change::AddFib { vrf, vni } => { + let vrf = vrfs[*vrf]; + let vni = vni.map(|i| all_vnis[i]); + let writer = table.add_fib(vrf, vni); + if let Some(displaced) = fibs.live.insert(vrf, writer) { + fibs.retired.push(displaced); + } + model.insert(FibKey::from_vrfid(vrf), vrf); + if let Some(vni) = vni { + model.insert(FibKey::from_vni(vni), vrf); + } + } + Change::RegisterByVni { vrf, vni } => { + let vrf = vrfs[*vrf]; + let vni = all_vnis[*vni]; + table.register_fib_by_vni(vrf, vni); + if model.contains_key(&FibKey::from_vrfid(vrf)) { + model.insert(FibKey::from_vni(vni), vrf); + } + } + Change::UnregisterVni { vni } => { + let vni = all_vnis[*vni]; + table.unregister_vni(vni); + model.remove(&FibKey::from_vni(vni)); + } + Change::DelFib { vrf } => { + let vrf = vrfs[*vrf]; + table.del_fib(vrf); + model.retain(|_, named| *named != vrf); + if let Some(writer) = fibs.live.remove(&vrf) { + writer.destroy(); + } + } + } + } + + #[test] + fn the_pools_are_the_size_the_generator_thinks() { + assert_eq!(vrf_ids().len(), usize::from(NUM_VRFS)); + assert_eq!(vnis().len(), usize::from(NUM_VNIS)); + assert_eq!(keys().len(), usize::from(NUM_VRFS + NUM_VNIS)); + } + + #[test] + fn every_key_in_a_fib_table_reaches_the_fib_it_names() { + let keys = keys(); + + bolero::check!() + .with_generator(ChangeSequences) + .cloned() + .for_each(|changes: Vec| { + let (mut table, _reader) = FibTableWriter::new(); + let mut fibs = Fibs { + live: BTreeMap::new(), + retired: Vec::new(), + }; + let mut model = Model::new(); + + for (step, change) in changes.iter().enumerate() { + apply(&mut table, &mut fibs, &mut model, change); + + let at = || format!("at step {step} of {changes:?}"); + let held = table.enter().unwrap_or_else(|| unreachable!()); + + assert_eq!(held.len(), model.len(), "{}", at()); + + for key in &keys { + let Some(reader) = held.get_fib(*key) else { + assert!(!model.contains_key(key), "{key} missing {}", at()); + continue; + }; + let want = *model + .get(key) + .unwrap_or_else(|| panic!("{key} unexpected {}", at())); + + assert!(reader.is_valid(), "{key} reaches a dead fib {}", at()); + assert_eq!( + reader.get_id(), + Some(FibKey::from_vrfid(want)), + "{key} reaches the wrong fib {}", + at() + ); + } + } + }); + } +} diff --git a/routing/src/fib/test.rs b/routing/src/fib/test.rs index bac97ec8ca..e2e9ec8ca1 100644 --- a/routing/src/fib/test.rs +++ b/routing/src/fib/test.rs @@ -367,7 +367,7 @@ mod tests { } if updates.is_multiple_of(50) && fibw.is_some() { - fibtw.del_fib(vrfid, None); + fibtw.del_fib(vrfid); if let Some(fib) = fibw.take() { // fib is destroyed here fib.destroy(); @@ -519,7 +519,7 @@ mod concurrency_tests { loop { let fibw = fibtw.add_fib(vrfid, None); thread::sleep(Duration::from_millis(5)); - fibtw.del_fib(vrfid, None); + fibtw.del_fib(vrfid); fibw.destroy(); iterations += 1; if iterations == MAX_ITERATIONS { diff --git a/routing/src/rib/vrftable.rs b/routing/src/rib/vrftable.rs index d3035239ec..e12e6ddbc6 100644 --- a/routing/src/rib/vrftable.rs +++ b/routing/src/rib/vrftable.rs @@ -189,7 +189,7 @@ impl VrfTable { // delete the corresponding fib if let Some(fibw) = vrf.fibw.take() { debug!("Deleting Fib for vrf {vrfid} from the FibTable"); - self.fibtablew.del_fib(vrfid, vrf.vni); + self.fibtablew.del_fib(vrfid); fibw.destroy(); } From 6a841b2cac1ce3fda2858dd54f0441bec164b40a Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 13:28:17 -0600 Subject: [PATCH 05/14] fix(routing): refuse to remove the default vrf Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- routing/src/rib/vrf.rs | 2 +- routing/src/rib/vrftable.rs | 285 ++++++++++++++++++++++++++++++++++++ 2 files changed, 286 insertions(+), 1 deletion(-) diff --git a/routing/src/rib/vrf.rs b/routing/src/rib/vrf.rs index 6739a9d915..2ad0ca4899 100644 --- a/routing/src/rib/vrf.rs +++ b/routing/src/rib/vrf.rs @@ -113,7 +113,7 @@ impl ShimNhop { } } -#[derive(Copy, Clone, PartialEq)] +#[derive(Copy, Clone, Debug, PartialEq)] #[allow(unused)] pub enum VrfStatus { Active, diff --git a/routing/src/rib/vrftable.rs b/routing/src/rib/vrftable.rs index e12e6ddbc6..29f90c336d 100644 --- a/routing/src/rib/vrftable.rs +++ b/routing/src/rib/vrftable.rs @@ -176,6 +176,13 @@ impl VrfTable { vrfid: VrfId, iftablew: &mut IfTableWriter, ) -> Result<(), RouterError> { + if vrfid == Vrf::DEFAULT_VRFID { + error!("Refusing to remove the default vrf"); + return Err(RouterError::Internal( + "Bug: the default vrf cannot be removed", + )); + } + // remove the vrf from the vrf table debug!("Removing VRF {vrfid}..."); let Some(mut vrf) = self.by_id.remove(&vrfid) else { @@ -937,3 +944,281 @@ mod tests { test_vrf_fibgroup(build_test_vrf_nhops_partially_resolved()); } } + +#[cfg(test)] +mod vrftable_properties { + use super::*; + use crate::interfaces::iftablerw::IfTableWriter; + use crate::rib::vrf::VrfStatus; + use bolero::{Driver, ValueGenerator}; + use std::collections::BTreeMap; + use std::ops::Bound::Included; + + const NUM_VRFS: u8 = 3; + const NUM_VNIS: u8 = 2; + const NUM_STATUSES: u8 = 3; + const MAX_CHANGES: u8 = 12; + + fn vrf_ids() -> Vec { + (0..u32::from(NUM_VRFS)).collect() + } + + fn vnis() -> Vec { + (1..=u32::from(NUM_VNIS)) + .map(|i| Vni::new_checked(100 * i).unwrap_or_else(|_| unreachable!())) + .collect() + } + + fn statuses() -> Vec { + vec![VrfStatus::Active, VrfStatus::Deleting, VrfStatus::Deleted] + } + + #[derive(Debug, Clone)] + enum Change { + AddVrf { vrf: usize, vni: Option }, + SetVni { vrf: usize, vni: usize }, + UnsetVni { vrf: usize }, + RemoveVrf { vrf: usize }, + SetStatus { vrf: usize, status: usize }, + RemoveDeleted, + RemoveDeleting, + } + + #[derive(Debug, Clone, Copy, Default)] + struct ChangeSequences; + + fn index(driver: &mut D, count: u8) -> Option { + driver + .gen_u8(Included(&0), Included(&(count - 1))) + .map(usize::from) + } + + impl ValueGenerator for ChangeSequences { + type Output = Vec; + + fn generate(&self, driver: &mut D) -> Option> { + let len = driver.gen_u8(Included(&0), Included(&MAX_CHANGES))?; + let mut out = Vec::with_capacity(usize::from(len)); + for _ in 0..len { + let change = match driver.gen_u8(Included(&0), Included(&6))? { + 0 => { + let vrf = index(driver, NUM_VRFS)?; + let drawn = index(driver, NUM_VNIS + 1)?; + Change::AddVrf { + vrf, + vni: (drawn < usize::from(NUM_VNIS)).then_some(drawn), + } + } + 1 => Change::SetVni { + vrf: index(driver, NUM_VRFS)?, + vni: index(driver, NUM_VNIS)?, + }, + 2 => Change::UnsetVni { + vrf: index(driver, NUM_VRFS)?, + }, + 3 => Change::RemoveVrf { + vrf: index(driver, NUM_VRFS)?, + }, + 4 => Change::SetStatus { + vrf: index(driver, NUM_VRFS)?, + status: index(driver, NUM_STATUSES)?, + }, + 5 => Change::RemoveDeleted, + _ => Change::RemoveDeleting, + }; + out.push(change); + } + Some(out) + } + } + + type Model = BTreeMap, VrfStatus)>; + + fn owner_of(model: &Model, vni: Vni) -> Option { + model + .iter() + .find_map(|(id, (carried, _))| (*carried == Some(vni)).then_some(*id)) + } + + fn fresh_model() -> Model { + Model::from([(Vrf::DEFAULT_VRFID, (None, VrfStatus::Active))]) + } + + fn apply(table: &mut VrfTable, iftw: &mut IfTableWriter, model: &mut Model, change: &Change) { + let ids = vrf_ids(); + let all_vnis = vnis(); + match change { + Change::AddVrf { vrf, vni } => { + let id = ids[*vrf]; + let vni = vni.map(|i| all_vnis[i]); + let config = RouterVrfConfig::new(id, &format!("vrf{id}")).set_vni(vni); + let _ = table.add_vrf(&config); + if model.contains_key(&id) || vni.is_some_and(|v| owner_of(model, v).is_some()) { + return; + } + model.insert(id, (vni, VrfStatus::Active)); + } + Change::SetVni { vrf, vni } => { + let id = ids[*vrf]; + let vni = all_vnis[*vni]; + let _ = table.set_vni(id, vni); + match owner_of(model, vni) { + Some(_) => (), + None => { + if let Some(entry) = model.get_mut(&id) { + entry.0 = Some(vni); + } + } + } + } + Change::UnsetVni { vrf } => { + let id = ids[*vrf]; + let _ = table.unset_vni(id); + if let Some(entry) = model.get_mut(&id) { + entry.0 = None; + } + } + Change::RemoveVrf { vrf } => { + let id = ids[*vrf]; + let _ = table.remove_vrf(id, iftw); + if id != Vrf::DEFAULT_VRFID { + model.remove(&id); + } + } + Change::SetStatus { vrf, status } => { + let id = ids[*vrf]; + let status = statuses()[*status]; + if let Ok(vrf) = table.get_vrf_mut(id) { + vrf.set_status(status); + } + if id != Vrf::DEFAULT_VRFID + && let Some(entry) = model.get_mut(&id) + { + entry.1 = status; + } + } + Change::RemoveDeleted => { + table.remove_deleted_vrfs(iftw); + model.retain(|_, (_, status)| *status != VrfStatus::Deleted); + } + Change::RemoveDeleting => { + table.remove_deleting_vrfs(iftw); + model.retain(|_, (_, status)| *status != VrfStatus::Deleting); + } + } + } + + fn check(table: &VrfTable, model: &Model, at: &str) { + let ids = vrf_ids(); + let all_vnis = vnis(); + + assert_eq!(table.len(), model.len(), "vrf count {at}"); + for id in &ids { + let Ok(vrf) = table.get_vrf(*id) else { + assert!(!model.contains_key(id), "vrf {id} missing {at}"); + continue; + }; + let (vni, status) = model + .get(id) + .unwrap_or_else(|| panic!("vrf {id} unexpected {at}")); + assert_eq!(vrf.vrfid, *id, "vrf {id} filed under the wrong key {at}"); + assert_eq!(vrf.vni, *vni, "vrf {id} vni {at}"); + assert_eq!(vrf.status, *status, "vrf {id} status {at}"); + } + + assert_eq!( + table.by_vni.len(), + model.values().filter(|(vni, _)| vni.is_some()).count(), + "vni index size {at}" + ); + for vni in &all_vnis { + assert_eq!( + table.get_vrfid_by_vni(*vni).ok(), + owner_of(model, *vni), + "vni {vni} index {at}" + ); + assert_eq!( + table.get_vrf_by_vni(*vni).map(|vrf| vrf.vrfid).ok(), + owner_of(model, *vni), + "vni {vni} lookup {at}" + ); + } + + let fibs = table.fibtablew.enter().unwrap_or_else(|| unreachable!()); + let expected_keys = model.len() + model.values().filter(|(v, _)| v.is_some()).count(); + assert_eq!(fibs.len(), expected_keys, "fib table size {at}"); + for id in &ids { + let key = FibKey::from_vrfid(*id); + let Some(fib) = fibs.get_fib(key) else { + assert!(!model.contains_key(id), "no fib for vrf {id} {at}"); + continue; + }; + assert!(fib.is_valid(), "fib for vrf {id} is dead {at}"); + assert_eq!( + fib.get_id(), + Some(key), + "fib for vrf {id} is not its own {at}" + ); + } + for vni in &all_vnis { + let Some(fib) = fibs.get_fib(FibKey::from_vni(*vni)) else { + assert!(owner_of(model, *vni).is_none(), "no fib for vni {vni} {at}"); + continue; + }; + let owner = owner_of(model, *vni) + .unwrap_or_else(|| panic!("fib aliased by vni {vni} with no owner {at}")); + assert!(fib.is_valid(), "fib aliased by vni {vni} is dead {at}"); + assert_eq!( + fib.get_id(), + Some(FibKey::from_vrfid(owner)), + "vni {vni} reaches the wrong fib {at}" + ); + } + drop(fibs); + + assert!(table.contains(Vrf::DEFAULT_VRFID), "no default vrf {at}"); + assert_eq!( + table.get_default_vrf().status, + VrfStatus::Active, + "default vrf not active {at}" + ); + + for id in &ids { + let Some((vni, _)) = model.get(id) else { + continue; + }; + assert_eq!( + table.check_vni(*id).is_ok(), + vni.is_some(), + "check_vni disagrees for vrf {id} {at}" + ); + } + } + + #[test] + fn the_pools_are_the_size_the_generator_thinks() { + assert_eq!(vrf_ids().len(), usize::from(NUM_VRFS)); + assert_eq!(vnis().len(), usize::from(NUM_VNIS)); + assert_eq!(statuses().len(), usize::from(NUM_STATUSES)); + assert_eq!(vrf_ids()[0], Vrf::DEFAULT_VRFID); + } + + #[test] + fn a_vrf_tables_four_views_stay_in_step() { + bolero::check!() + .with_generator(ChangeSequences) + .cloned() + .for_each(|changes: Vec| { + let (fibtw, _fibtr) = FibTableWriter::new(); + let (mut iftw, _iftr) = IfTableWriter::new(); + let mut table = VrfTable::new(fibtw); + let mut model = fresh_model(); + + check(&table, &model, "on a fresh table"); + for (step, change) in changes.iter().enumerate() { + apply(&mut table, &mut iftw, &mut model, change); + check(&table, &model, &format!("at step {step} of {changes:?}")); + } + }); + } +} From 51e81f896805d187cf920a061dd066b181411441 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 13:39:16 -0600 Subject: [PATCH 06/14] fix(frrmi): frame messages correctly - base receive framing on used (rather than the resized) buffer length. - Wait for complete headers, - handle partial bodies, - reject announced bodies over 16 MiB, Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- routing/src/frr/frrmi.rs | 220 ++++++++++++++++++++++++++++++++++----- 1 file changed, 194 insertions(+), 26 deletions(-) diff --git a/routing/src/frr/frrmi.rs b/routing/src/frr/frrmi.rs index 50f9ad802d..a359e6cf5a 100644 --- a/routing/src/frr/frrmi.rs +++ b/routing/src/frr/frrmi.rs @@ -327,6 +327,11 @@ impl Frrmi { return Err(FrrErr::NotConnected); }; loop { + if let Some(announced) = self.readb.oversized() { + error!("Frr-agent announced a {announced}-octet message: refusing it"); + self.readb.clear(); + return Err(FrrErr::DecodeFailure); + } let pending = self.readb.next_read_len(); debug!("Recv data (read:{} pending:{pending})", self.readb.used); match Self::recv(sock, &mut self.readb, pending) { @@ -400,6 +405,10 @@ struct IoBuffer { used: usize, } impl IoBuffer { + const HEADER_LEN: usize = 16; + + const MAX_MSG_LEN: usize = 16 * 1024 * 1024; + #[must_use] #[allow(unused)] pub fn new() -> Self { @@ -426,37 +435,30 @@ impl IoBuffer { self.extend(msg); } - /// Tell the length that a message (encoded as |length|genid|data|) must have. - /// If less than 8 octets have been read it is not possible to know how big the message is yet. #[must_use] fn msg_len(&self) -> Option { - if self.buffer.len() < 8 { - None - } else { - let len_buf = &self.buffer[0..8] - .try_into() - .unwrap_or_else(|_| unreachable!()); - - #[allow(clippy::cast_possible_truncation)] - let msg_len = u64::from_ne_bytes(*len_buf) as usize; - Some(msg_len) + if self.used < Self::HEADER_LEN { + return None; } + let len_buf: &[u8; 8] = &self.buffer[0..8] + .try_into() + .unwrap_or_else(|_| unreachable!()); + + #[allow(clippy::cast_possible_truncation)] + let msg_len = u64::from_ne_bytes(*len_buf) as usize; + Some(msg_len) + } + + #[must_use] + fn oversized(&self) -> Option { + self.msg_len().filter(|len| *len > Self::MAX_MSG_LEN) } - /// Tell the number of octets that should be read next according to the contents of the read buffer - /// to get a message or be able to determine its length. - /// If less than 16 octets have been received, this returns the number needed to have exactly 16. - /// Else, we return the number of octets that are pending to have the complete message. + #[must_use] fn next_read_len(&self) -> usize { - if self.len() < 16 { - 16 - self.len() - } else { - let msg_len = self.msg_len().unwrap_or_else(|| unreachable!()); - if msg_len > (self.len() - 16) { - msg_len - (self.len() - 16) - } else { - 0 - } + match self.msg_len() { + None => Self::HEADER_LEN - self.used, + Some(msg_len) => msg_len.saturating_sub(self.used - Self::HEADER_LEN), } } @@ -464,7 +466,7 @@ impl IoBuffer { #[must_use] fn is_ready(&self) -> bool { match self.msg_len() { - Some(m) => self.len() == m + 16, + Some(msg_len) => self.used - Self::HEADER_LEN == msg_len, None => false, } } @@ -488,3 +490,169 @@ impl IoBuffer { Ok(FrrmiResponse { genid, data }) } } + +#[cfg(test)] +mod framing_properties { + use super::*; + use bolero::{Driver, ValueGenerator}; + use std::ops::Bound::Included; + + const MAX_BODY: u8 = 20; + const MAX_CHUNK: u8 = 24; + const MAX_CHUNKS: u8 = 8; + const MAX_MESSAGES: u8 = 4; + + #[derive(Debug, Clone)] + struct Delivery { + genid: GenId, + body: String, + chunks: Vec, + } + + #[derive(Debug, Clone, Copy, Default)] + struct Deliveries; + + impl ValueGenerator for Deliveries { + type Output = Delivery; + + fn generate(&self, driver: &mut D) -> Option { + let genid = GenId::from(driver.gen_u8(Included(&0), Included(&3))?); + let body_len = usize::from(driver.gen_u8(Included(&0), Included(&MAX_BODY))?); + let body: String = (0..body_len) + .map(|i| char::from(b'a' + u8::try_from(i % 26).unwrap_or_else(|_| unreachable!()))) + .collect(); + + let count = driver.gen_u8(Included(&0), Included(&MAX_CHUNKS))?; + let mut chunks = Vec::with_capacity(usize::from(count)); + for _ in 0..count { + chunks.push(usize::from( + driver.gen_u8(Included(&1), Included(&MAX_CHUNK))?, + )); + } + Some(Delivery { + genid, + body, + chunks, + }) + } + } + + fn connected_pair() -> (UnixStream, Frrmi) { + let (peer, ours) = UnixStream::pair().unwrap_or_else(|e| unreachable!("{e}")); + let frrmi = Frrmi { + sock: Some(ours), + ..Frrmi::default() + }; + (peer, frrmi) + } + + #[derive(Debug, Clone, Copy, Default)] + struct Streams; + + impl ValueGenerator for Streams { + type Output = Vec; + + fn generate(&self, driver: &mut D) -> Option> { + let count = driver.gen_u8(Included(&1), Included(&MAX_MESSAGES))?; + let mut out = Vec::with_capacity(usize::from(count)); + for _ in 0..count { + out.push(Deliveries.generate(driver)?); + } + Some(out) + } + } + + #[test] + fn a_message_survives_any_division_into_reads() { + bolero::check!() + .with_generator(Deliveries) + .cloned() + .for_each(|delivery: Delivery| { + let mut wire = IoBuffer::new(); + wire.serialize(delivery.genid, delivery.body.as_bytes()); + let bytes = wire.buffer; + + let (mut peer, mut frrmi) = connected_pair(); + + let mut sent = 0; + let mut got = None; + let mut sizes = delivery.chunks.iter().copied(); + while sent < bytes.len() { + let take = sizes.next().unwrap_or(usize::MAX).min(bytes.len() - sent); + peer.write_all(&bytes[sent..sent + take]) + .unwrap_or_else(|e| unreachable!("{e}")); + sent += take; + + match frrmi.recv_msg() { + Ok(Some(response)) => { + assert!(got.is_none(), "two messages from one, for {delivery:?}"); + got = Some(response); + } + Ok(None) => (), + Err(e) => panic!("recv failed with {e} for {delivery:?}"), + } + } + + let response = got.unwrap_or_else(|| panic!("no message, for {delivery:?}")); + assert_eq!(response.genid, delivery.genid, "genid for {delivery:?}"); + assert_eq!(response.data, delivery.body, "body for {delivery:?}"); + }); + } + + #[test] + fn a_connection_carries_one_message_after_another() { + bolero::check!() + .with_generator(Streams) + .cloned() + .for_each(|deliveries: Vec| { + let mut bytes = Vec::new(); + for delivery in &deliveries { + let mut wire = IoBuffer::new(); + wire.serialize(delivery.genid, delivery.body.as_bytes()); + bytes.extend_from_slice(&wire.buffer); + } + + let (mut peer, mut frrmi) = connected_pair(); + + let mut sizes = deliveries.iter().flat_map(|d| d.chunks.iter().copied()); + let mut got: Vec<(GenId, String)> = Vec::new(); + let mut sent = 0; + while sent < bytes.len() { + let take = sizes.next().unwrap_or(usize::MAX).min(bytes.len() - sent); + peer.write_all(&bytes[sent..sent + take]) + .unwrap_or_else(|e| unreachable!("{e}")); + sent += take; + + loop { + match frrmi.recv_msg() { + Ok(Some(response)) => got.push((response.genid, response.data)), + Ok(None) => break, + Err(e) => panic!("recv failed with {e} for {deliveries:?}"), + } + } + } + + let want: Vec<(GenId, String)> = deliveries + .iter() + .map(|d| (d.genid, d.body.clone())) + .collect(); + assert_eq!(got, want, "for {deliveries:?}"); + }); + } + + #[test] + fn an_absurd_announced_length_is_refused() { + let mut header = Vec::new(); + header.extend_from_slice(&u64::MAX.to_ne_bytes()); + header.extend_from_slice(&0i64.to_ne_bytes()); + + let (mut peer, mut frrmi) = connected_pair(); + peer.write_all(&header) + .unwrap_or_else(|e| unreachable!("{e}")); + + assert!( + matches!(frrmi.recv_msg(), Err(FrrErr::DecodeFailure)), + "an absurd length must be refused" + ); + } +} From 39064f0eea9e5852fca5735bdb94f80f4c8c372f Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 13:52:23 -0600 Subject: [PATCH 07/14] fix(routing): empty next-hop routes are drops An empty-next-hop route in the rib was rejected by the fib. This allowed traffic to fall through to a less-specific route. We now substitute an explicit drop so every caller preserves consistency. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- routing/src/rib/vrf.rs | 303 +++++++++++++++++++++++++++++++- routing/src/router/rpc_adapt.rs | 6 +- 2 files changed, 301 insertions(+), 8 deletions(-) diff --git a/routing/src/rib/vrf.rs b/routing/src/rib/vrf.rs index 2ad0ca4899..373d4f7d82 100644 --- a/routing/src/rib/vrf.rs +++ b/routing/src/rib/vrf.rs @@ -4,10 +4,11 @@ //! VRF module to store Ipv4 and Ipv6 routing tables use bitflags::bitflags; +use std::borrow::Cow; use std::hash::Hash; use std::net::IpAddr; use std::rc::{Rc, Weak}; -use tracing::debug; +use tracing::{debug, warn}; #[cfg(test)] use common::cliprovider::Frame; @@ -392,6 +393,15 @@ impl Vrf { } } + fn nhops_or_drop<'a>(prefix: &Prefix, nhops: &'a [RouteNhop]) -> Cow<'a, [RouteNhop]> { + if nhops.is_empty() { + warn!("Route to {prefix} has no next-hop: will install it with action drop"); + Cow::Owned(vec![RouteNhop::default()]) + } else { + Cow::Borrowed(nhops) + } + } + ///////////////////////////////////////////////////////////////////////// // Route Insertion ///////////////////////////////////////////////////////////////////////// @@ -403,7 +413,7 @@ impl Vrf { vrf0: Option<&Vrf>, ) { // register next-hops and let the route keep references to the shared nexthops created/found - route.s_nhops = self.register_shared_nhops(nhops); + route.s_nhops = self.register_shared_nhops(&Self::nhops_or_drop(prefix, nhops)); // resolve the new route next-hops. This is only for testing. In prod code, // this method is only used for drop routes which require no resolution. @@ -464,7 +474,7 @@ impl Vrf { rstore: &RmacStore, ) { // register next-hops and let the route keep references to the shared nexthops created/found - route.s_nhops = self.register_shared_nhops(nhops); + route.s_nhops = self.register_shared_nhops(&Self::nhops_or_drop(prefix, nhops)); let rvrf = vrf0.unwrap_or(self); @@ -1140,3 +1150,290 @@ pub mod tests { } } + +#[cfg(test)] +mod vrf_properties { + use super::*; + use crate::fib::fibtype::{FibKey, FibWriter}; + use crate::rib::nexthop::NhopKey; + use bolero::{Driver, ValueGenerator}; + use std::collections::{BTreeMap, BTreeSet}; + use std::ops::Bound::Included; + use std::str::FromStr; + + const NUM_PREFIXES: u8 = 8; + const NUM_NHOPS: u8 = 3; + const MAX_CHANGES: u8 = 10; + const MAX_NHOPS_PER_ROUTE: u8 = 3; + + const ROOT_V4: usize = 0; + const ROOT_V6: usize = 5; + + fn prefixes() -> Vec { + [ + "0.0.0.0/0", + "10.0.0.0/8", + "10.1.0.0/16", + "10.1.2.0/24", + "10.1.2.3/32", + "::/0", + "2001:db8::/32", + "2001:db8:1::/48", + ] + .iter() + .map(|p| Prefix::from_str(p).unwrap_or_else(|_| unreachable!())) + .collect() + } + + fn probes() -> Vec { + [ + "9.9.9.9", + "10.9.9.9", + "10.1.9.9", + "10.1.2.9", + "10.1.2.3", + "2000::1", + "2001:db8::1", + "2001:db8:1::1", + ] + .iter() + .map(|a| IpAddr::from_str(a).unwrap_or_else(|_| unreachable!())) + .collect() + } + + fn nhops() -> Vec { + vec![ + tests::build_test_nhop(Some("10.0.0.1"), Some(1), 0, None), + tests::build_test_nhop(Some("10.0.0.2"), None, 0, None), + tests::build_test_nhop(None, Some(3), 0, None), + ] + } + + #[derive(Debug, Clone)] + enum Change { + AddRoute { + prefix: usize, + nhops: Vec, + }, + DelRoute { + prefix: usize, + }, + SetStale { + value: bool, + }, + RemoveStale, + } + + #[derive(Debug, Clone, Copy, Default)] + struct ChangeSequences; + + fn index(driver: &mut D, count: u8) -> Option { + driver + .gen_u8(Included(&0), Included(&(count - 1))) + .map(usize::from) + } + + impl ValueGenerator for ChangeSequences { + type Output = Vec; + + fn generate(&self, driver: &mut D) -> Option> { + let len = driver.gen_u8(Included(&0), Included(&MAX_CHANGES))?; + let mut out = Vec::with_capacity(usize::from(len)); + for _ in 0..len { + let change = match driver.gen_u8(Included(&0), Included(&3))? { + 0 => { + let prefix = index(driver, NUM_PREFIXES)?; + let count = driver.gen_u8(Included(&0), Included(&MAX_NHOPS_PER_ROUTE))?; + let mut nhops = Vec::with_capacity(usize::from(count)); + for _ in 0..count { + nhops.push(index(driver, NUM_NHOPS)?); + } + Change::AddRoute { prefix, nhops } + } + 1 => Change::DelRoute { + prefix: index(driver, NUM_PREFIXES)?, + }, + 2 => Change::SetStale { + value: driver.produce::()?, + }, + _ => Change::RemoveStale, + }; + out.push(change); + } + Some(out) + } + } + + #[derive(Debug, Clone)] + struct Model { + routes: BTreeMap, bool)>, + } + + impl Model { + fn preset() -> Vec { + vec![NhopKey::with_drop()] + } + + fn new() -> Self { + Self { + routes: BTreeMap::from([ + (ROOT_V4, (Self::preset(), false)), + (ROOT_V6, (Self::preset(), false)), + ]), + } + } + + fn is_root(prefix: usize) -> bool { + prefix == ROOT_V4 || prefix == ROOT_V6 + } + + fn referenced(&self) -> BTreeSet { + self.routes + .values() + .flat_map(|(nhops, _)| nhops.iter().cloned()) + .collect() + } + + fn lpm(&self, addr: &IpAddr, pool: &[Prefix]) -> Option { + self.routes + .keys() + .copied() + .filter(|i| pool[*i].covers_addr(addr)) + .max_by_key(|i| pool[*i].length()) + } + + fn apply(&mut self, change: &Change, pool: &[RouteNhop]) { + match change { + Change::AddRoute { prefix, nhops } => { + let keys = if nhops.is_empty() { + Self::preset() + } else { + nhops.iter().map(|i| pool[*i].key.clone()).collect() + }; + self.routes.insert(*prefix, (keys, false)); + } + Change::DelRoute { prefix } => { + if Self::is_root(*prefix) { + self.routes.insert(*prefix, (Self::preset(), false)); + } else { + self.routes.remove(prefix); + } + } + Change::SetStale { value } => { + for (prefix, (_, stale)) in &mut self.routes { + if !Self::is_root(*prefix) { + *stale = *value; + } + } + } + Change::RemoveStale => { + let stale: Vec = self + .routes + .iter() + .filter_map(|(prefix, (_, stale))| stale.then_some(*prefix)) + .collect(); + for prefix in stale { + self.apply(&Change::DelRoute { prefix }, pool); + } + } + } + } + } + + fn apply_to_vrf(vrf: &mut Vrf, rstore: &RmacStore, change: &Change, pool: &[RouteNhop]) { + let prefixes = prefixes(); + match change { + Change::AddRoute { prefix, nhops } => { + let route = tests::build_test_route(RouteOrigin::Bgp, 20, 100); + let nhops: Vec = nhops.iter().map(|i| pool[*i].clone()).collect(); + vrf.add_route_complete(&prefixes[*prefix], route, &nhops, None, rstore); + } + Change::DelRoute { prefix } => vrf.del_route(prefixes[*prefix], None, rstore), + Change::SetStale { value } => vrf.set_stale(*value), + Change::RemoveStale => vrf.remove_stale_routes(None, rstore), + } + } + + fn check(vrf: &Vrf, model: &Model, at: &str) { + let prefixes = prefixes(); + let probes = probes(); + + let held: BTreeSet = (0..prefixes.len()) + .filter(|i| vrf.get_route(prefixes[*i]).is_some()) + .collect(); + let want: BTreeSet = model.routes.keys().copied().collect(); + assert_eq!(held, want, "route set {at}"); + assert_eq!( + vrf.len_v4() + vrf.len_v6(), + model.routes.len(), + "route count {at}" + ); + + for (prefix, (nhops, stale)) in &model.routes { + let route = vrf + .get_route(prefixes[*prefix]) + .unwrap_or_else(|| panic!("no route for {prefix} {at}")); + let got: Vec = route.s_nhops.iter().map(|s| s.rc.key.clone()).collect(); + assert_eq!(got, *nhops, "next-hops of {prefix} {at}"); + assert_eq!(route.is_stale(), *stale, "stale flag of {prefix} {at}"); + } + + let stored: BTreeSet = vrf.nhstore.iter().map(|rc| rc.key.clone()).collect(); + assert_eq!(stored, model.referenced(), "next-hop store {at}"); + + for probe in &probes { + let want = model + .lpm(probe, &prefixes) + .unwrap_or_else(|| panic!("model has no route for {probe} {at}")); + let (hit, _) = vrf.lpm(*probe); + assert_eq!(hit, prefixes[want], "lpm for {probe} {at}"); + } + + let fibw = vrf.fibw.as_ref().unwrap_or_else(|| unreachable!()); + let fib = fibw.enter().unwrap_or_else(|| unreachable!()); + let mut want_v4 = BTreeSet::new(); + let mut want_v6 = BTreeSet::new(); + for prefix in model.routes.keys() { + match prefixes[*prefix] { + Prefix::IPV4(p) => want_v4.insert(p), + Prefix::IPV6(p) => want_v6.insert(p), + }; + } + let fib_v4: BTreeSet = fib.iter_v4().map(|(prefix, _)| prefix).collect(); + let fib_v6: BTreeSet = fib.iter_v6().map(|(prefix, _)| prefix).collect(); + assert_eq!(fib_v4, want_v4, "fib ipv4 prefixes {at}"); + assert_eq!(fib_v6, want_v6, "fib ipv6 prefixes {at}"); + } + + #[test] + fn the_pools_are_the_size_the_generator_thinks() { + assert_eq!(prefixes().len(), usize::from(NUM_PREFIXES)); + assert_eq!(nhops().len(), usize::from(NUM_NHOPS)); + assert_eq!(prefixes()[ROOT_V4], Prefix::root_v4()); + assert_eq!(prefixes()[ROOT_V6], Prefix::root_v6()); + } + + #[test] + fn a_vrfs_routes_and_next_hops_stay_in_step() { + let pool = nhops(); + let rstore = RmacStore::new(); + + bolero::check!() + .with_generator(ChangeSequences) + .cloned() + .for_each(|changes: Vec| { + let config = RouterVrfConfig::new(1, "test"); + let mut vrf = Vrf::new(&config); + let (fibw, _fibr) = FibWriter::new(FibKey::from_vrfid(1)); + vrf.set_fibw(fibw); + let mut model = Model::new(); + + check(&vrf, &model, "on a fresh vrf"); + for (step, change) in changes.iter().enumerate() { + apply_to_vrf(&mut vrf, &rstore, change, &pool); + model.apply(change, &pool); + check(&vrf, &model, &format!("at step {step} of {changes:?}")); + } + }); + } +} diff --git a/routing/src/router/rpc_adapt.rs b/routing/src/router/rpc_adapt.rs index ff2e39bbc9..42736db975 100644 --- a/routing/src/router/rpc_adapt.rs +++ b/routing/src/router/rpc_adapt.rs @@ -222,12 +222,8 @@ impl Vrf { } } - // If no next-hop was received with the route (or we could not successfully process any), - // install the route anyway with an action drop. This is better than not installing the - // route as that could break consistency (e.g. resolving via a default) and cause a loop. if nhops.is_empty() { - warn!("Route to {prefix} from RPC would have no next-hop. Will inject DROP next-hop"); - nhops.push(RouteNhop::default()); + warn!("Route to {prefix} from RPC has no usable next-hop: will be a DROP route"); } // N.B. route and next-hops are passed separately From 76a184ba75723d5b2d5da603faec4dbc5eadac11 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 14:10:01 -0600 Subject: [PATCH 08/14] test(routing): vrf deletion check - generate VRF status transitions - distinguish preset root-drop routes from ordinary drop routes. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- routing/src/rib/vrf.rs | 102 +++++++++++++++++++++++++++++------------ 1 file changed, 72 insertions(+), 30 deletions(-) diff --git a/routing/src/rib/vrf.rs b/routing/src/rib/vrf.rs index 373d4f7d82..ee7561b47c 100644 --- a/routing/src/rib/vrf.rs +++ b/routing/src/rib/vrf.rs @@ -1165,6 +1165,7 @@ mod vrf_properties { const NUM_NHOPS: u8 = 3; const MAX_CHANGES: u8 = 10; const MAX_NHOPS_PER_ROUTE: u8 = 3; + const NUM_STATUSES: u8 = 3; const ROOT_V4: usize = 0; const ROOT_V6: usize = 5; @@ -1201,6 +1202,10 @@ mod vrf_properties { .collect() } + fn statuses() -> Vec { + vec![VrfStatus::Active, VrfStatus::Deleting, VrfStatus::Deleted] + } + fn nhops() -> Vec { vec![ tests::build_test_nhop(Some("10.0.0.1"), Some(1), 0, None), @@ -1211,17 +1216,11 @@ mod vrf_properties { #[derive(Debug, Clone)] enum Change { - AddRoute { - prefix: usize, - nhops: Vec, - }, - DelRoute { - prefix: usize, - }, - SetStale { - value: bool, - }, + AddRoute { prefix: usize, nhops: Vec }, + DelRoute { prefix: usize }, + SetStale { value: bool }, RemoveStale, + SetStatus { status: usize }, } #[derive(Debug, Clone, Copy, Default)] @@ -1240,7 +1239,7 @@ mod vrf_properties { let len = driver.gen_u8(Included(&0), Included(&MAX_CHANGES))?; let mut out = Vec::with_capacity(usize::from(len)); for _ in 0..len { - let change = match driver.gen_u8(Included(&0), Included(&3))? { + let change = match driver.gen_u8(Included(&0), Included(&4))? { 0 => { let prefix = index(driver, NUM_PREFIXES)?; let count = driver.gen_u8(Included(&0), Included(&MAX_NHOPS_PER_ROUTE))?; @@ -1256,7 +1255,10 @@ mod vrf_properties { 2 => Change::SetStale { value: driver.produce::()?, }, - _ => Change::RemoveStale, + 3 => Change::RemoveStale, + _ => Change::SetStatus { + status: index(driver, NUM_STATUSES)?, + }, }; out.push(change); } @@ -1264,22 +1266,32 @@ mod vrf_properties { } } + #[derive(Debug, Clone, PartialEq)] + struct RouteState { + nhops: Vec, + stale: bool, + preset: bool, + } + #[derive(Debug, Clone)] struct Model { - routes: BTreeMap, bool)>, + routes: BTreeMap, + status: VrfStatus, } impl Model { - fn preset() -> Vec { - vec![NhopKey::with_drop()] + fn preset() -> RouteState { + RouteState { + nhops: vec![NhopKey::with_drop()], + stale: false, + preset: true, + } } fn new() -> Self { Self { - routes: BTreeMap::from([ - (ROOT_V4, (Self::preset(), false)), - (ROOT_V6, (Self::preset(), false)), - ]), + routes: BTreeMap::from([(ROOT_V4, Self::preset()), (ROOT_V6, Self::preset())]), + status: VrfStatus::Active, } } @@ -1290,10 +1302,20 @@ mod vrf_properties { fn referenced(&self) -> BTreeSet { self.routes .values() - .flat_map(|(nhops, _)| nhops.iter().cloned()) + .flat_map(|route| route.nhops.iter().cloned()) .collect() } + fn check_deletion(&mut self) { + let only_presets = self.routes.len() == 2 + && [ROOT_V4, ROOT_V6] + .iter() + .all(|root| self.routes.get(root).is_some_and(|route| route.preset)); + if self.status == VrfStatus::Deleting && only_presets { + self.status = VrfStatus::Deleted; + } + } + fn lpm(&self, addr: &IpAddr, pool: &[Prefix]) -> Option { self.routes .keys() @@ -1305,24 +1327,32 @@ mod vrf_properties { fn apply(&mut self, change: &Change, pool: &[RouteNhop]) { match change { Change::AddRoute { prefix, nhops } => { - let keys = if nhops.is_empty() { - Self::preset() + let nhops = if nhops.is_empty() { + vec![NhopKey::with_drop()] } else { nhops.iter().map(|i| pool[*i].key.clone()).collect() }; - self.routes.insert(*prefix, (keys, false)); + self.routes.insert( + *prefix, + RouteState { + nhops, + stale: false, + preset: false, + }, + ); } Change::DelRoute { prefix } => { if Self::is_root(*prefix) { - self.routes.insert(*prefix, (Self::preset(), false)); + self.routes.insert(*prefix, Self::preset()); } else { self.routes.remove(prefix); } + self.check_deletion(); } Change::SetStale { value } => { - for (prefix, (_, stale)) in &mut self.routes { + for (prefix, route) in &mut self.routes { if !Self::is_root(*prefix) { - *stale = *value; + route.stale = *value; } } } @@ -1330,12 +1360,15 @@ mod vrf_properties { let stale: Vec = self .routes .iter() - .filter_map(|(prefix, (_, stale))| stale.then_some(*prefix)) + .filter_map(|(prefix, route)| route.stale.then_some(*prefix)) .collect(); for prefix in stale { self.apply(&Change::DelRoute { prefix }, pool); } } + Change::SetStatus { status } => { + self.status = statuses()[*status]; + } } } } @@ -1351,6 +1384,7 @@ mod vrf_properties { Change::DelRoute { prefix } => vrf.del_route(prefixes[*prefix], None, rstore), Change::SetStale { value } => vrf.set_stale(*value), Change::RemoveStale => vrf.remove_stale_routes(None, rstore), + Change::SetStatus { status } => vrf.set_status(statuses()[*status]), } } @@ -1369,15 +1403,22 @@ mod vrf_properties { "route count {at}" ); - for (prefix, (nhops, stale)) in &model.routes { + for (prefix, want) in &model.routes { let route = vrf .get_route(prefixes[*prefix]) .unwrap_or_else(|| panic!("no route for {prefix} {at}")); let got: Vec = route.s_nhops.iter().map(|s| s.rc.key.clone()).collect(); - assert_eq!(got, *nhops, "next-hops of {prefix} {at}"); - assert_eq!(route.is_stale(), *stale, "stale flag of {prefix} {at}"); + assert_eq!(got, want.nhops, "next-hops of {prefix} {at}"); + assert_eq!(route.is_stale(), want.stale, "stale flag of {prefix} {at}"); + assert_eq!( + route.is_preset_drop_route(), + want.preset, + "preset-drop-route of {prefix} {at}" + ); } + assert_eq!(vrf.status, model.status, "status {at}"); + let stored: BTreeSet = vrf.nhstore.iter().map(|rc| rc.key.clone()).collect(); assert_eq!(stored, model.referenced(), "next-hop store {at}"); @@ -1409,6 +1450,7 @@ mod vrf_properties { fn the_pools_are_the_size_the_generator_thinks() { assert_eq!(prefixes().len(), usize::from(NUM_PREFIXES)); assert_eq!(nhops().len(), usize::from(NUM_NHOPS)); + assert_eq!(statuses().len(), usize::from(NUM_STATUSES)); assert_eq!(prefixes()[ROOT_V4], Prefix::root_v4()); assert_eq!(prefixes()[ROOT_V6], Prefix::root_v6()); } From 0c48ce5c453c1e9747491184ddefcb25c51ef1af Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 14:30:34 -0600 Subject: [PATCH 09/14] test(routing): prop-test resolution across vrfs Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- routing/src/rib/vrftable.rs | 396 ++++++++++++++++++++++++++++++++++++ 1 file changed, 396 insertions(+) diff --git a/routing/src/rib/vrftable.rs b/routing/src/rib/vrftable.rs index 29f90c336d..3a01b0875e 100644 --- a/routing/src/rib/vrftable.rs +++ b/routing/src/rib/vrftable.rs @@ -1222,3 +1222,399 @@ mod vrftable_properties { }); } } + +#[cfg(test)] +mod crossvrf_properties { + use super::*; + use crate::fib::fibobjects::{EgressObject, FibEntry, PktInstruction}; + use crate::rib::vrf::tests::{build_test_nhop, build_test_route, mk_addr}; + use crate::rib::vrf::{Route, RouteNhop, RouteOrigin}; + use bolero::{Driver, ValueGenerator}; + use lpm::prefix::Prefix; + use net::interface::InterfaceIndex; + use std::collections::BTreeMap; + use std::net::IpAddr; + use std::ops::Bound::Included; + use std::str::FromStr; + + const NUM_VRFS: u8 = 2; + const NUM_VNIS: u8 = 2; + const NUM_UNDERLAY: u8 = 2; + const NUM_OVERLAY: u8 = 2; + const NUM_IFINDEXES: u8 = 3; + const NUM_VIAS: u8 = 3; + const MAX_ROUTES: u8 = 3; + + fn vrf_ids() -> Vec { + (1..=u32::from(NUM_VRFS)).collect() + } + + fn vnis() -> Vec { + (1..=u32::from(NUM_VNIS)) + .map(|i| Vni::new_checked(100 * i).unwrap_or_else(|_| unreachable!())) + .collect() + } + + fn underlay() -> Vec { + ["7.0.0.0/8", "7.1.0.0/16"] + .iter() + .map(|p| Prefix::from_str(p).unwrap_or_else(|_| unreachable!())) + .collect() + } + + fn overlay() -> Vec { + ["10.0.0.0/8", "10.1.0.0/16"] + .iter() + .map(|p| Prefix::from_str(p).unwrap_or_else(|_| unreachable!())) + .collect() + } + + fn ifindexes() -> Vec { + (1..=u32::from(NUM_IFINDEXES)).collect() + } + + fn onlink_addrs() -> Vec { + (1..=NUM_IFINDEXES) + .map(|i| mk_addr(&format!("7.200.0.{i}"))) + .collect() + } + + fn vias() -> Vec { + ["7.0.0.1", "7.1.0.1", "8.0.0.1"] + .iter() + .map(|a| mk_addr(a)) + .collect() + } + + #[derive(Debug, Clone)] + struct Topology { + vrfs: Vec, + underlay: Vec<(usize, usize, bool)>, + overlay: Vec<(usize, usize, usize)>, + later: Option<(usize, usize, bool)>, + selected: Vec, + } + + #[derive(Debug, Clone, Copy, Default)] + struct Topologies; + + fn index(driver: &mut D, count: u8) -> Option { + driver + .gen_u8(Included(&0), Included(&(count - 1))) + .map(usize::from) + } + + impl ValueGenerator for Topologies { + type Output = Topology; + + fn generate(&self, driver: &mut D) -> Option { + let mut vrfs = Vec::with_capacity(usize::from(NUM_VRFS)); + for _ in 0..NUM_VRFS { + vrfs.push(driver.produce::()?); + } + + let count = driver.gen_u8(Included(&0), Included(&MAX_ROUTES))?; + let mut underlay = Vec::with_capacity(usize::from(count)); + for _ in 0..count { + underlay.push(( + index(driver, NUM_UNDERLAY)?, + index(driver, NUM_IFINDEXES)?, + driver.produce::()?, + )); + } + + let count = driver.gen_u8(Included(&0), Included(&MAX_ROUTES))?; + let mut overlay = Vec::with_capacity(usize::from(count)); + for _ in 0..count { + overlay.push(( + index(driver, NUM_VRFS)?, + index(driver, NUM_OVERLAY)?, + index(driver, NUM_VIAS)?, + )); + } + + let later = if driver.produce::()? { + Some(( + index(driver, NUM_UNDERLAY)?, + index(driver, NUM_IFINDEXES)?, + driver.produce::()?, + )) + } else { + None + }; + + let count = driver.gen_u8(Included(&0), Included(&NUM_VNIS))?; + let mut selected = Vec::with_capacity(usize::from(count)); + for _ in 0..count { + selected.push(index(driver, NUM_VNIS)?); + } + + Some(Topology { + vrfs, + underlay, + overlay, + later, + selected, + }) + } + } + + type Underlay = BTreeMap; + + type Overlay = BTreeMap<(usize, usize), usize>; + + fn resolves_to(model: &Underlay, via: usize) -> Option<(u32, bool)> { + let address = vias()[via]; + let prefixes = underlay(); + model + .iter() + .filter(|(prefix, _)| prefixes[**prefix].covers_addr(&address)) + .max_by_key(|(prefix, _)| prefixes[**prefix].length()) + .map(|(_, (ifindex, onlink))| (ifindexes()[*ifindex], *onlink)) + } + + fn expected_entry(model: &Underlay, via: usize) -> FibEntry { + match resolves_to(model, via) { + Some((ifindex, onlink)) => { + let index = usize::try_from(ifindex).unwrap_or_else(|_| unreachable!()) - 1; + let address = if onlink { + onlink_addrs()[index] + } else { + vias()[via] + }; + FibEntry::with_inst(PktInstruction::Egress(EgressObject::new( + InterfaceIndex::try_new(ifindex).ok(), + Some(address), + None, + ))) + } + None => FibEntry::drop_fibentry(), + } + } + + fn underlay_route(ifindex: usize, onlink: bool) -> (Route, Vec) { + let address = onlink.then(|| onlink_addrs()[ifindex].to_string()); + ( + build_test_route(RouteOrigin::Connected, 0, 0), + vec![build_test_nhop( + address.as_deref(), + Some(ifindexes()[ifindex]), + 0, + None, + )], + ) + } + + fn overlay_route(via: usize) -> (Route, Vec) { + ( + build_test_route(RouteOrigin::Bgp, 20, 100), + vec![build_test_nhop( + Some(&vias()[via].to_string()), + None, + 0, + None, + )], + ) + } + + fn realize(topology: &Topology, rstore: &RmacStore) -> (VrfTable, Underlay, Overlay) { + let (fibtw, _fibtr) = FibTableWriter::new(); + let mut table = VrfTable::new(fibtw); + let ids = vrf_ids(); + let all_vnis = vnis(); + + for (vrf, has_vni) in topology.vrfs.iter().enumerate() { + let config = RouterVrfConfig::new(ids[vrf], &format!("vrf{vrf}")) + .set_vni(has_vni.then(|| all_vnis[vrf])); + table + .add_vrf(&config) + .unwrap_or_else(|e| unreachable!("{e}")); + } + + let mut model = Underlay::new(); + for (prefix, ifindex, onlink) in &topology.underlay { + let (route, nhops) = underlay_route(*ifindex, *onlink); + let vrf0 = table + .get_vrf_mut(Vrf::DEFAULT_VRFID) + .unwrap_or_else(|e| unreachable!("{e}")); + vrf0.add_route_complete(&underlay()[*prefix], route, &nhops, None, rstore); + model.insert(*prefix, (*ifindex, *onlink)); + } + + let mut overlay_model = Overlay::new(); + for (vrf, prefix, via) in &topology.overlay { + let (route, nhops) = overlay_route(*via); + let target = table + .get_vrf_mut(ids[*vrf]) + .unwrap_or_else(|e| unreachable!("{e}")); + target.add_route_complete(&overlay()[*prefix], route, &nhops, None, rstore); + overlay_model.insert((*vrf, *prefix), *via); + } + + (table, model, overlay_model) + } + + fn fib_entries(table: &VrfTable, vrfid: VrfId, prefix: Prefix) -> Option> { + let vrf = table.get_vrf(vrfid).unwrap_or_else(|e| unreachable!("{e}")); + let fibw = vrf.fibw.as_ref().unwrap_or_else(|| unreachable!()); + let fib = fibw.enter().unwrap_or_else(|| unreachable!()); + let Prefix::IPV4(wanted) = prefix else { + unreachable!() + }; + fib.iter_v4().find(|(p, _)| *p == wanted).map(|(_, route)| { + route + .iter() + .flat_map(|group| group.entries().iter().cloned()) + .collect() + }) + } + + fn every_entry_is_executable(table: &VrfTable, at: &str) { + for vrf in table.values() { + let fibw = vrf.fibw.as_ref().unwrap_or_else(|| unreachable!()); + let fib = fibw.enter().unwrap_or_else(|| unreachable!()); + for (prefix, route) in fib.iter_v4() { + for group in route.iter() { + assert!(!group.is_empty(), "empty group for {prefix} {at}"); + for entry in group.iter() { + assert!( + entry.is_valid(), + "vrf {} offers unusable {entry:?} for {prefix} {at}", + vrf.vrfid + ); + } + } + } + } + } + + #[test] + fn the_pools_are_the_size_the_generator_thinks() { + assert_eq!(vrf_ids().len(), usize::from(NUM_VRFS)); + assert_eq!(vnis().len(), usize::from(NUM_VNIS)); + assert_eq!(underlay().len(), usize::from(NUM_UNDERLAY)); + assert_eq!(overlay().len(), usize::from(NUM_OVERLAY)); + assert_eq!(ifindexes().len(), usize::from(NUM_IFINDEXES)); + assert_eq!(vias().len(), usize::from(NUM_VIAS)); + const { assert!(NUM_VNIS >= NUM_VRFS) }; + assert_eq!(onlink_addrs().len(), usize::from(NUM_IFINDEXES)); + let all: Underlay = (0..usize::from(NUM_UNDERLAY)) + .map(|p| (p, (0, false))) + .collect(); + assert!(resolves_to(&all, usize::from(NUM_VIAS) - 1).is_none()); + } + + #[test] + fn a_refresh_resolves_other_vrfs_through_the_default_one() { + let rstore = RmacStore::new(); + bolero::check!() + .with_generator(Topologies) + .cloned() + .for_each(|topology: Topology| { + let (mut table, model, routes) = realize(&topology, &rstore); + table.refresh_non_default_fibs(&rstore); + every_entry_is_executable(&table, "after a refresh"); + + let ids = vrf_ids(); + for ((vrf, prefix), via) in &routes { + let got = fib_entries(&table, ids[*vrf], overlay()[*prefix]) + .unwrap_or_else(|| panic!("no fib route for {prefix} in vrf {vrf}")); + assert_eq!( + got, + vec![expected_entry(&model, *via)], + "vrf {vrf} prefix {prefix} via {via}, for {topology:?}" + ); + } + }); + } + + #[test] + fn refreshing_by_vni_touches_only_those_vnis() { + let rstore = RmacStore::new(); + bolero::check!() + .with_generator(Topologies) + .cloned() + .for_each(|topology: Topology| { + let Some(later) = topology.later else { return }; + let (mut table, mut model, routes) = realize(&topology, &rstore); + table.refresh_non_default_fibs(&rstore); + + let ids = vrf_ids(); + let all_vnis = vnis(); + + let before: BTreeMap<(usize, usize), Option>> = routes + .keys() + .map(|(vrf, prefix)| { + ( + (*vrf, *prefix), + fib_entries(&table, ids[*vrf], overlay()[*prefix]), + ) + }) + .collect(); + + let (prefix, ifindex, onlink) = later; + let (route, nhops) = underlay_route(ifindex, onlink); + let vrf0 = table + .get_vrf_mut(Vrf::DEFAULT_VRFID) + .unwrap_or_else(|e| unreachable!("{e}")); + vrf0.add_route_complete(&underlay()[prefix], route, &nhops, None, &rstore); + model.insert(prefix, (ifindex, onlink)); + + let selected: Vec = topology.selected.iter().map(|i| all_vnis[*i]).collect(); + table.refresh_fibs_by_vni(&selected, &rstore); + every_entry_is_executable(&table, "after refreshing by vni"); + + for ((vrf, prefix), via) in &routes { + let got = fib_entries(&table, ids[*vrf], overlay()[*prefix]); + let vni = table + .get_vrf(ids[*vrf]) + .unwrap_or_else(|e| unreachable!("{e}")) + .vni; + if vni.is_some_and(|vni| selected.contains(&vni)) { + assert_eq!( + got, + Some(vec![expected_entry(&model, *via)]), + "refreshed vrf {vrf} prefix {prefix}, for {topology:?}" + ); + } else { + assert_eq!( + got, + before[&(*vrf, *prefix)], + "untouched vrf {vrf} prefix {prefix}, for {topology:?}" + ); + } + } + }); + } + + #[test] + fn a_stale_sweep_empties_every_vrf() { + let rstore = RmacStore::new(); + bolero::check!() + .with_generator(Topologies) + .cloned() + .for_each(|topology: Topology| { + let (mut table, _model, _routes) = realize(&topology, &rstore); + table.refresh_non_default_fibs(&rstore); + + table.set_stale(true); + table.remove_stale_routes(&rstore); + + for vrf in table.values() { + assert_eq!(vrf.len_v4(), 1, "vrf {} kept ipv4 routes", vrf.vrfid); + assert_eq!(vrf.len_v6(), 1, "vrf {} kept ipv6 routes", vrf.vrfid); + for prefix in [Prefix::root_v4(), Prefix::root_v6()] { + let route = vrf + .get_route(prefix) + .unwrap_or_else(|| panic!("vrf {} lost {prefix}", vrf.vrfid)); + assert!( + route.is_preset_drop_route(), + "vrf {} left {prefix} as something other than the preset drop route", + vrf.vrfid + ); + } + } + every_entry_is_executable(&table, "after a stale sweep"); + }); + } +} From a99d7c41d37e994b56a5b29d47b5b909d34385d1 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 14:46:23 -0600 Subject: [PATCH 10/14] fix(routing): remove interface name from next-hop key Interface names come from the kernel. Letting these names say in the key can give the same next-hop different keys. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- routing/src/interfaces/iftablerw.rs | 4 - routing/src/router/cpi.rs | 5 +- routing/src/router/rpc_adapt.rs | 362 ++++++++++++++++++++++++++-- 3 files changed, 340 insertions(+), 31 deletions(-) diff --git a/routing/src/interfaces/iftablerw.rs b/routing/src/interfaces/iftablerw.rs index 29f231098e..db9c641424 100644 --- a/routing/src/interfaces/iftablerw.rs +++ b/routing/src/interfaces/iftablerw.rs @@ -80,10 +80,6 @@ impl IfTableWriter { (IfTableWriter(w), IfTableReader(r)) } #[must_use] - pub fn as_reader(&self) -> IfTableReader { - IfTableReader::new(self.0.clone()) - } - #[must_use] pub fn enter(&self) -> Option> { self.0.enter() } diff --git a/routing/src/router/cpi.rs b/routing/src/router/cpi.rs index 68d9d0a220..b8fe075dda 100644 --- a/routing/src/router/cpi.rs +++ b/routing/src/router/cpi.rs @@ -209,14 +209,13 @@ impl RpcOperation for IpRoute { fn add(&self, db: &mut Self::ObjectStore) -> RpcResultCode { let rmac_store = &db.rmac_store; let vrftable = &mut db.vrftable; - let iftabler = &db.iftw.as_reader(); if self.vrfid == Vrf::DEFAULT_VRFID { let Ok(vrf0) = vrftable.get_vrf_mut(self.vrfid) else { error!("Unable to find default VRF!"); return RpcResultCode::Failure; }; - vrf0.add_route_rpc(self, None, rmac_store, iftabler); + vrf0.add_route_rpc(self, None, rmac_store); vrftable.refresh_non_default_fibs(rmac_store); } else { // this assumes that we always resolve non-default vrfs with the default vrf @@ -229,7 +228,7 @@ impl RpcOperation for IpRoute { error!("Unable to get vrf with id {}", self.vrfid); return RpcResultCode::Failure; }; - vrf.add_route_rpc(self, Some(vrf0), rmac_store, iftabler); + vrf.add_route_rpc(self, Some(vrf0), rmac_store); } RpcResultCode::Ok } diff --git a/routing/src/router/rpc_adapt.rs b/routing/src/router/rpc_adapt.rs index 42736db975..b39f66a479 100644 --- a/routing/src/router/rpc_adapt.rs +++ b/routing/src/router/rpc_adapt.rs @@ -11,7 +11,6 @@ use crate::errors::RouterError; use crate::evpn::{RmacEntry, RmacStore}; -use crate::interfaces::iftablerw::IfTableReader; use crate::rib::encapsulation::{Encapsulation, VxlanEncapsulation}; use crate::rib::nexthop::{FwAction, NhopKey}; use crate::rib::vrf::{Route, RouteFlags, RouteNhop, RouteOrigin, Vrf}; @@ -100,11 +99,7 @@ impl TryFrom<&Rmac> for RmacEntry { impl RouteNhop { #[tracing::instrument(level = "debug")] - fn from_rpc_nhop( - nh: &NextHop, - origin: RouteOrigin, - iftabler: &IfTableReader, - ) -> Result { + fn from_rpc_nhop(nh: &NextHop, origin: RouteOrigin) -> Result { let mut ifindex = nh .ifindex .map(|i| match InterfaceIndex::try_new(i) { @@ -129,22 +124,13 @@ impl RouteNhop { None => None, }; - // lookup interface name - let ifname = match ifindex { - None => None, - Some(k) => iftabler - .enter() - .and_then(|iftable| iftable.get_interface(k).map(|iface| iface.name.clone())), - }; - - // build key for this next hop let key = NhopKey::new( origin, nh.address, ifindex, encap, FwAction::from(nh.fwaction), - ifname, + None, ); // validate next hop from its key @@ -180,13 +166,7 @@ impl Route { } impl Vrf { - pub fn add_route_rpc( - &mut self, - iproute: &IpRoute, - vrf0: Option<&Vrf>, - rstore: &RmacStore, - iftabler: &IfTableReader, - ) { + pub fn add_route_rpc(&mut self, iproute: &IpRoute, vrf0: Option<&Vrf>, rstore: &RmacStore) { let prefix = match Prefix::try_from((iproute.prefix, iproute.prefix_len)) { Ok(p) => p, Err(e) => { @@ -216,7 +196,7 @@ impl Vrf { let route = Route::from_iproute(&prefix, iproute); let mut nhops = Vec::with_capacity(iproute.nhops.len()); for nhop in &iproute.nhops { - match RouteNhop::from_rpc_nhop(nhop, route.origin, iftabler) { + match RouteNhop::from_rpc_nhop(nhop, route.origin) { Ok(nh) => nhops.push(nh), Err(e) => error!("Omitting next-hop {nhop} in route to {prefix}: {e}"), } @@ -241,3 +221,337 @@ impl Vrf { self.del_route(prefix, vrf0, rstore); } } + +#[cfg(test)] +mod rpc_properties { + use super::*; + use crate::fib::fibtype::{FibKey, FibWriter}; + use crate::rib::vrf::RouterVrfConfig; + use bolero::{Driver, ValueGenerator}; + use dplane_rpc::proto::{Ifindex, MaskLen, VrfId}; + use std::net::IpAddr; + use std::ops::Bound::Included; + use std::str::FromStr; + + const NUM_PREFIXES: u8 = 5; + const NUM_ADDRESSES: u8 = 3; + const NUM_IFINDEXES: u8 = 4; + const NUM_VNIS: u8 = 3; + const NUM_RTYPES: u8 = 7; + const MAX_NHOPS: u8 = 3; + + const BAD_PREFIX: usize = 4; + + fn prefixes() -> Vec<(IpAddr, MaskLen)> { + vec![ + ( + IpAddr::from_str("10.0.0.0").unwrap_or_else(|_| unreachable!()), + 8, + ), + ( + IpAddr::from_str("10.1.0.0").unwrap_or_else(|_| unreachable!()), + 16, + ), + ( + IpAddr::from_str("10.1.2.3").unwrap_or_else(|_| unreachable!()), + 32, + ), + ( + IpAddr::from_str("2001:db8::").unwrap_or_else(|_| unreachable!()), + 32, + ), + ( + IpAddr::from_str("10.0.0.0").unwrap_or_else(|_| unreachable!()), + 33, + ), + ] + } + + fn addresses() -> Vec> { + vec![ + None, + Some(IpAddr::from_str("10.0.0.1").unwrap_or_else(|_| unreachable!())), + Some(IpAddr::from_str("7.0.0.1").unwrap_or_else(|_| unreachable!())), + ] + } + + fn ifindexes() -> Vec> { + vec![None, Some(0), Some(2), Some(99)] + } + + fn vnis() -> Vec> { + vec![None, Some(0), Some(3000)] + } + + fn rtypes() -> Vec { + vec![ + RouteType::Local, + RouteType::Connected, + RouteType::Static, + RouteType::Ospf, + RouteType::Isis, + RouteType::Bgp, + RouteType::Other, + ] + } + + #[derive(Debug, Clone)] + struct NhopSpec { + drop: bool, + address: usize, + ifindex: usize, + vni: usize, + vrfid: VrfId, + } + + #[derive(Debug, Clone)] + struct RouteSpec { + prefix: usize, + rtype: usize, + distance: u8, + metric: u32, + nhops: Vec, + } + + #[derive(Debug, Clone, Copy, Default)] + struct Routes; + + fn index(driver: &mut D, count: u8) -> Option { + driver + .gen_u8(Included(&0), Included(&(count - 1))) + .map(usize::from) + } + + impl ValueGenerator for Routes { + type Output = RouteSpec; + + fn generate(&self, driver: &mut D) -> Option { + let prefix = index(driver, NUM_PREFIXES)?; + let rtype = index(driver, NUM_RTYPES)?; + let distance = driver.produce::()?; + let metric = driver.produce::()?; + let count = driver.gen_u8(Included(&0), Included(&MAX_NHOPS))?; + let mut nhops = Vec::with_capacity(usize::from(count)); + for _ in 0..count { + nhops.push(NhopSpec { + drop: driver.produce::()?, + address: index(driver, NUM_ADDRESSES)?, + ifindex: index(driver, NUM_IFINDEXES)?, + vni: index(driver, NUM_VNIS)?, + vrfid: 0, + }); + } + Some(RouteSpec { + prefix, + rtype, + distance, + metric, + nhops, + }) + } + } + + fn wire_nhop(spec: &NhopSpec) -> NextHop { + NextHop { + fwaction: if spec.drop { + ForwardAction::Drop + } else { + ForwardAction::Forward + }, + address: addresses()[spec.address], + ifindex: ifindexes()[spec.ifindex], + vrfid: spec.vrfid, + encap: vnis()[spec.vni].map(|vni| NextHopEncap::VXLAN(VxlanEncap { vni })), + } + } + + fn wire_route(spec: &RouteSpec) -> IpRoute { + let (prefix, prefix_len) = prefixes()[spec.prefix]; + IpRoute { + prefix, + prefix_len, + vrfid: 0, + tableid: 254, + rtype: rtypes()[spec.rtype], + distance: spec.distance, + metric: spec.metric, + nhops: spec.nhops.iter().map(wire_nhop).collect(), + } + } + + fn expected_origin(rtype: RouteType, prefix: &Prefix) -> RouteOrigin { + if rtype == RouteType::Connected && prefix.is_host() { + return RouteOrigin::Local; + } + match rtype { + RouteType::Local => RouteOrigin::Local, + RouteType::Connected => RouteOrigin::Connected, + RouteType::Static => RouteOrigin::Static, + RouteType::Ospf => RouteOrigin::Ospf, + RouteType::Isis => RouteOrigin::Isis, + RouteType::Bgp => RouteOrigin::Bgp, + RouteType::Other => RouteOrigin::Other, + } + } + + fn expected_key(spec: &NhopSpec, origin: RouteOrigin) -> Option { + let raw = ifindexes()[spec.ifindex]; + if raw == Some(0) { + return None; + } + let address = addresses()[spec.address]; + + let encap = match vnis()[spec.vni] { + None => None, + Some(vni) => Some(Encapsulation::Vxlan(VxlanEncapsulation { + vni: Vni::new_checked(vni).ok()?, + remote: address?, + dmac: None, + })), + }; + + let ifindex = if encap.is_some() { + None + } else { + raw.and_then(|i| InterfaceIndex::try_new(i).ok()) + }; + + let fwaction = if spec.drop { + FwAction::Drop + } else { + FwAction::Forward + }; + if fwaction == FwAction::Forward && ifindex.is_none() && address.is_none() { + return None; + } + + Some(NhopKey::new( + origin, address, ifindex, encap, fwaction, None, + )) + } + + fn test_vrf() -> Vrf { + let config = RouterVrfConfig::new(1, "test"); + let mut vrf = Vrf::new(&config); + let (fibw, _fibr) = FibWriter::new(FibKey::from_vrfid(1)); + vrf.set_fibw(fibw); + vrf + } + + #[test] + fn the_pools_are_the_size_the_generator_thinks() { + assert_eq!(prefixes().len(), usize::from(NUM_PREFIXES)); + assert_eq!(addresses().len(), usize::from(NUM_ADDRESSES)); + assert_eq!(ifindexes().len(), usize::from(NUM_IFINDEXES)); + assert_eq!(vnis().len(), usize::from(NUM_VNIS)); + assert_eq!(rtypes().len(), usize::from(NUM_RTYPES)); + + let (prefix, len) = prefixes()[BAD_PREFIX]; + assert!( + Prefix::try_from((prefix, len)).is_err(), + "the bad prefix must not parse" + ); + assert!( + Vni::new_checked(0).is_err(), + "vni zero must not be a valid vni" + ); + assert!( + InterfaceIndex::try_new(0).is_err(), + "interface index zero must not be valid" + ); + for (address, len) in prefixes() { + assert!(Prefix::try_from((address, len)).is_ok_and(|p| !p.is_root()) || len > 32); + } + } + + #[test] + fn a_wire_next_hop_becomes_the_key_the_message_describes() { + bolero::check!() + .with_generator(Routes) + .cloned() + .for_each(|spec: RouteSpec| { + for origin in [RouteOrigin::Local, RouteOrigin::Bgp, RouteOrigin::Connected] { + for nhop in &spec.nhops { + let got = RouteNhop::from_rpc_nhop(&wire_nhop(nhop), origin); + match expected_key(nhop, origin) { + Some(want) => { + let got = got.unwrap_or_else(|e| { + panic!("refused {nhop:?} with {e}, expected {want:?}") + }); + assert_eq!(got.key, want, "for {nhop:?} origin {origin:?}"); + assert_eq!(got.vrfid, nhop.vrfid, "vrfid for {nhop:?}"); + } + None => assert!(got.is_err(), "accepted {nhop:?}, expected refusal"), + } + } + } + }); + } + + #[test] + fn a_wire_route_is_installed_as_the_message_describes() { + bolero::check!() + .with_generator(Routes) + .cloned() + .for_each(|spec: RouteSpec| { + let rstore = RmacStore::new(); + let mut vrf = test_vrf(); + vrf.add_route_rpc(&wire_route(&spec), None, &rstore); + + let (raw, len) = prefixes()[spec.prefix]; + let Ok(prefix) = Prefix::try_from((raw, len)) else { + assert_eq!(vrf.len_v4() + vrf.len_v6(), 2, "for {spec:?}"); + return; + }; + + let origin = expected_origin(rtypes()[spec.rtype], &prefix); + let route = vrf + .get_route(prefix) + .unwrap_or_else(|| panic!("no route for {prefix}, for {spec:?}")); + + assert_eq!(route.origin, origin, "origin for {spec:?}"); + assert_eq!(route.distance, spec.distance, "distance for {spec:?}"); + assert_eq!(route.metric, spec.metric, "metric for {spec:?}"); + + let mut want: Vec = spec + .nhops + .iter() + .filter_map(|nhop| expected_key(nhop, origin)) + .collect(); + if want.is_empty() { + want.push(NhopKey::with_drop()); + } + let got: Vec = route.s_nhops.iter().map(|s| s.rc.key.clone()).collect(); + assert_eq!(got, want, "next-hops for {spec:?}"); + }); + } + + #[test] + fn a_wire_delete_removes_what_the_message_names() { + bolero::check!() + .with_generator(Routes) + .cloned() + .for_each(|spec: RouteSpec| { + let rstore = RmacStore::new(); + let mut vrf = test_vrf(); + let route = wire_route(&spec); + vrf.add_route_rpc(&route, None, &rstore); + vrf.del_route_rpc(&route, None, &rstore); + + let (raw, len) = prefixes()[spec.prefix]; + if let Ok(prefix) = Prefix::try_from((raw, len)) { + assert!( + vrf.get_route(prefix).is_none(), + "route to {prefix} survived deletion, for {spec:?}" + ); + } + assert_eq!(vrf.len_v4() + vrf.len_v6(), 2, "for {spec:?}"); + let keys: Vec = vrf.nhstore.iter().map(|rc| rc.key.clone()).collect(); + assert_eq!( + keys, + vec![NhopKey::with_drop()], + "leftover next-hops for {spec:?}" + ); + }); + } +} From d4993ad3e48ea6370d39ff3fa77791e374da21a3 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 14:56:51 -0600 Subject: [PATCH 11/14] test(routing): prop-test control-plane Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- routing/src/router/cpi.rs | 338 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 338 insertions(+) diff --git a/routing/src/router/cpi.rs b/routing/src/router/cpi.rs index b8fe075dda..08f0fc1bbe 100644 --- a/routing/src/router/cpi.rs +++ b/routing/src/router/cpi.rs @@ -497,3 +497,341 @@ pub fn process_cpi_data(rio: &mut Rio, peer: &SocketAddr, data: &mut Bytes, db: } } } + +#[cfg(test)] +mod cpi_properties { + use super::*; + use crate::atable::atablerw::AtableWriter; + use crate::config::RouterConfig; + use crate::evpn::RmacStore; + use crate::fib::fibobjects::{FibEntry, PktInstruction}; + use crate::fib::fibtable::FibTableWriter; + use crate::interfaces::iftablerw::IfTableWriter; + use crate::interfaces::tests::build_test_iftable; + use crate::rib::encapsulation::Encapsulation; + use crate::rib::vrf::tests::{build_test_nhop, build_test_route}; + use crate::rib::vrf::{RouteOrigin, RouterVrfConfig, VrfStatus}; + use bolero::{Driver, ValueGenerator}; + use dplane_rpc::msg::{ForwardAction, NextHop, VxlanEncap}; + use dplane_rpc::objects::MacAddress; + use lpm::prefix::Prefix; + use net::eth::mac::Mac; + use net::vxlan::Vni; + use std::net::IpAddr; + use std::ops::Bound::Included; + use std::str::FromStr; + + const OVERLAY_VRF: VrfId = 7; + const OVERLAY_VNI: u32 = 3000; + const UNDERLAY_IFINDEX: u32 = 2; + + const NUM_VTEPS: u8 = 2; + const NUM_MACS: u8 = 2; + + fn addr(a: &str) -> IpAddr { + IpAddr::from_str(a).unwrap_or_else(|_| unreachable!()) + } + + fn vteps() -> Vec { + vec![addr("7.0.0.1"), addr("7.0.0.2")] + } + + fn macs() -> Vec<[u8; 6]> { + vec![ + [0x00, 0xaa, 0x00, 0x00, 0x00, 0x01], + [0x00, 0xbb, 0x00, 0x00, 0x00, 0x02], + ] + } + + fn fabric() -> RoutingDb { + let (fibtw, _fibtr) = FibTableWriter::new(); + let (iftw, _iftr) = IfTableWriter::new_with_data(build_test_iftable()); + let (_atw, atabler) = AtableWriter::new(); + let mut db = RoutingDb::new(fibtw, iftw, atabler); + + let vrf0 = db + .vrftable + .get_vrf_mut(Vrf::DEFAULT_VRFID) + .unwrap_or_else(|e| unreachable!("{e}")); + vrf0.add_route_complete( + &Prefix::from_str("7.0.0.0/8").unwrap_or_else(|_| unreachable!()), + build_test_route(RouteOrigin::Connected, 0, 0), + &[build_test_nhop(None, Some(UNDERLAY_IFINDEX), 0, None)], + None, + &RmacStore::new(), + ); + + let vni = Vni::new_checked(OVERLAY_VNI).unwrap_or_else(|_| unreachable!()); + let config = RouterVrfConfig::new(OVERLAY_VRF, "overlay").set_vni(Some(vni)); + db.vrftable + .add_vrf(&config) + .unwrap_or_else(|e| unreachable!("{e}")); + db + } + + fn overlay_route(vrfid: VrfId, prefix: &str, vtep: IpAddr) -> IpRoute { + let (address, len) = prefix.split_once('/').unwrap_or_else(|| unreachable!()); + IpRoute { + prefix: addr(address), + prefix_len: len.parse().unwrap_or_else(|_| unreachable!()), + vrfid, + tableid: 254, + rtype: RouteType::Bgp, + distance: 20, + metric: 100, + nhops: vec![NextHop { + fwaction: ForwardAction::Forward, + address: Some(vtep), + ifindex: None, + vrfid, + encap: Some(NextHopEncap::VXLAN(VxlanEncap { vni: OVERLAY_VNI })), + }], + } + } + + fn rmac_msg(vtep: IpAddr, mac: [u8; 6]) -> Rmac { + Rmac { + address: vtep, + mac: MacAddress::new(mac), + vni: OVERLAY_VNI, + } + } + + fn fib_entries(db: &RoutingDb, vrfid: VrfId, prefix: &str) -> Vec { + let prefix = Prefix::from_str(prefix).unwrap_or_else(|_| unreachable!()); + let Prefix::IPV4(wanted) = prefix else { + unreachable!() + }; + let vrf = db + .vrftable + .get_vrf(vrfid) + .unwrap_or_else(|e| unreachable!("{e}")); + let fibw = vrf.fibw.as_ref().unwrap_or_else(|| unreachable!()); + let fib = fibw.enter().unwrap_or_else(|| unreachable!()); + fib.iter_v4() + .find(|(p, _)| *p == wanted) + .map(|(_, route)| { + route + .iter() + .flat_map(|group| group.entries().iter().cloned()) + .collect() + }) + .unwrap_or_default() + } + + fn entries_are_well_formed(entries: &[FibEntry], at: &str) { + for entry in entries { + assert!(entry.is_valid(), "unusable {entry:?} {at}"); + let drop_at = entry + .iter() + .position(|inst| matches!(inst, PktInstruction::Drop)); + if let Some(index) = drop_at { + assert_eq!(index, 0, "a drop is not first in {entry:?} {at}"); + } + } + } + + #[derive(Debug, Clone, Copy, Default)] + struct Fabrics; + + impl ValueGenerator for Fabrics { + type Output = (usize, usize); + + fn generate(&self, driver: &mut D) -> Option<(usize, usize)> { + let vtep = driver.gen_u8(Included(&0), Included(&(NUM_VTEPS - 1)))?; + let mac = driver.gen_u8(Included(&0), Included(&(NUM_MACS - 1)))?; + Some((usize::from(vtep), usize::from(mac))) + } + } + + #[test] + fn the_pools_are_the_size_the_generator_thinks() { + assert_eq!(vteps().len(), usize::from(NUM_VTEPS)); + assert_eq!(macs().len(), usize::from(NUM_MACS)); + let underlay = Prefix::from_str("7.0.0.0/8").unwrap_or_else(|_| unreachable!()); + for vtep in vteps() { + assert!(underlay.covers_addr(&vtep), "{vtep} is not in the underlay"); + } + } + + #[test] + fn an_overlay_route_drops_until_its_router_mac_arrives() { + bolero::check!().with_generator(Fabrics).cloned().for_each( + |(vtep, mac): (usize, usize)| { + let mut db = fabric(); + let vtep = vteps()[vtep]; + let prefix = "10.0.0.0/24"; + + assert_eq!( + overlay_route(OVERLAY_VRF, prefix, vtep).add(&mut db), + RpcResultCode::Ok + ); + + let before = fib_entries(&db, OVERLAY_VRF, prefix); + entries_are_well_formed(&before, "before the rmac"); + assert!( + before + .iter() + .all(|entry| matches!(entry.iter().next(), Some(PktInstruction::Drop))), + "an overlay route with no router mac must drop, got {before:?}" + ); + + assert_eq!(rmac_msg(vtep, macs()[mac]).add(&mut db), RpcResultCode::Ok); + + let after = fib_entries(&db, OVERLAY_VRF, prefix); + entries_are_well_formed(&after, "after the rmac"); + let expected_mac = Mac::from(macs()[mac]); + for entry in &after { + let mut instructions = entry.iter(); + match instructions.next() { + Some(PktInstruction::Encap(Encapsulation::Vxlan(vxlan))) => { + assert_eq!(vxlan.vni.as_u32(), OVERLAY_VNI, "vni in {entry:?}"); + assert_eq!(vxlan.remote, vtep, "remote in {entry:?}"); + assert_eq!(vxlan.dmac, Some(expected_mac), "dmac in {entry:?}"); + } + other => panic!("expected an encapsulation first, got {other:?}"), + } + match instructions.next() { + Some(PktInstruction::Egress(egress)) => { + assert_eq!( + egress.ifindex().map(InterfaceIndex::to_u32), + Some(UNDERLAY_IFINDEX), + "egress interface in {entry:?}" + ); + assert_eq!( + *egress.address(), + Some(vtep), + "egress address in {entry:?}" + ); + } + other => panic!("expected an egress second, got {other:?}"), + } + assert!(instructions.next().is_none(), "extra work in {entry:?}"); + } + }, + ); + } + + #[test] + fn withdrawing_a_router_mac_leaves_the_route_forwarding() { + bolero::check!().with_generator(Fabrics).cloned().for_each( + |(vtep, mac): (usize, usize)| { + let mut db = fabric(); + let vtep = vteps()[vtep]; + let prefix = "10.0.0.0/24"; + let rmac = rmac_msg(vtep, macs()[mac]); + + overlay_route(OVERLAY_VRF, prefix, vtep).add(&mut db); + rmac.add(&mut db); + let before = fib_entries(&db, OVERLAY_VRF, prefix); + + assert_eq!(rmac.del(&mut db), RpcResultCode::Ok); + db.vrftable.refresh_non_default_fibs(&db.rmac_store); + + let after = fib_entries(&db, OVERLAY_VRF, prefix); + entries_are_well_formed(&after, "after withdrawing the rmac"); + assert_eq!(after, before, "withdrawing a router mac changed the fib"); + }, + ); + } + + #[test] + fn an_unknown_vrf_fails_on_add_and_forgives_on_delete() { + let missing = OVERLAY_VRF + 1; + let prefix = "10.9.0.0/24"; + let vtep = vteps()[0]; + + let mut db = fabric(); + assert_eq!( + overlay_route(missing, prefix, vtep).add(&mut db), + RpcResultCode::Failure + ); + assert_eq!( + overlay_route(missing, prefix, vtep).del(&mut db), + RpcResultCode::Ok, + "a delete for an unknown vrf is forgiven while we have no config" + ); + + db.set_config(RouterConfig::new(1)); + assert!(db.have_config()); + assert_eq!( + overlay_route(missing, prefix, vtep).del(&mut db), + RpcResultCode::Failure, + "once a config is applied the same lookup is a real failure" + ); + } + + #[test] + fn deleting_the_last_route_of_a_dying_vrf_removes_it() { + let mut db = fabric(); + let prefix = "10.0.0.0/24"; + let vtep = vteps()[0]; + let route = overlay_route(OVERLAY_VRF, prefix, vtep); + route.add(&mut db); + + assert_eq!(route.del(&mut db), RpcResultCode::Ok); + assert!(db.vrftable.contains(OVERLAY_VRF)); + + route.add(&mut db); + db.vrftable + .get_vrf_mut(OVERLAY_VRF) + .unwrap_or_else(|e| unreachable!("{e}")) + .set_status(VrfStatus::Deleting); + + assert_eq!(route.del(&mut db), RpcResultCode::Ok); + assert!( + !db.vrftable.contains(OVERLAY_VRF), + "a vrf that became deletable was left behind" + ); + } + + #[test] + fn an_interface_address_is_refused_unless_it_is_usable() { + let cases = [ + (UNDERLAY_IFINDEX, 24, RpcResultCode::Ok), + (UNDERLAY_IFINDEX, 0, RpcResultCode::InvalidRequest), + (UNDERLAY_IFINDEX, 33, RpcResultCode::InvalidRequest), + (0, 24, RpcResultCode::InvalidRequest), + ]; + for (ifindex, mask, want) in cases { + let mut db = fabric(); + let message = IfAddress { + ifname: "eth0".to_string(), + address: addr("10.0.0.1"), + mask_len: mask, + ifindex, + vrfid: Vrf::DEFAULT_VRFID, + }; + let present = |db: &RoutingDb| { + let iftable = db.iftw.enter().unwrap_or_else(|| unreachable!()); + let Ok(index) = InterfaceIndex::try_new(ifindex) else { + return false; + }; + iftable + .get_interface(index) + .is_some_and(|iface| !iface.addresses.is_empty()) + }; + + assert_eq!(message.add(&mut db), want, "adding {message}"); + assert_eq!( + present(&db), + want == RpcResultCode::Ok, + "after adding {message}" + ); + + assert_eq!(message.del(&mut db), want, "deleting {message}"); + assert!(!present(&db), "the address survived its own deletion"); + } + } + + #[test] + fn a_next_hop_in_another_vrf_is_nonlocal() { + let vtep = vteps()[0]; + let mut route = overlay_route(OVERLAY_VRF, "10.0.0.0/24", vtep); + assert!(!nonlocal_nhop(&route), "its own vrf is not nonlocal"); + route.nhops[0].vrfid = Vrf::DEFAULT_VRFID; + assert!(nonlocal_nhop(&route), "another vrf is nonlocal"); + route.nhops.clear(); + assert!(!nonlocal_nhop(&route), "no next-hops, nothing nonlocal"); + } +} From fc6a97c7a1af66651d876a5cec2dd1a3ea393089 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 15:07:20 -0600 Subject: [PATCH 12/14] test(routing): prop-test config renderers Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- routing/src/frr/renderer/mod.rs | 465 ++++++++++++++++++++++++++++++++ 1 file changed, 465 insertions(+) diff --git a/routing/src/frr/renderer/mod.rs b/routing/src/frr/renderer/mod.rs index 64a7f89837..e8712e91e0 100644 --- a/routing/src/frr/renderer/mod.rs +++ b/routing/src/frr/renderer/mod.rs @@ -66,3 +66,468 @@ impl Render for InternalConfig { cfg } } + +#[cfg(test)] +mod renderer_properties { + use super::*; + use bolero::{Driver, ValueGenerator}; + use config::external::overlay::vpc::VpcId; + use config::internal::device::DeviceConfig; + use config::internal::routing::bfd::{ + BFD_DETECT_MULTIPLIER, BFD_RECEIVE_INTERVAL_MS, BFD_TRANSMIT_INTERVAL_MS, BfdPeer, + }; + use config::internal::routing::ospf::{Ospf, OspfInterface, OspfNetwork}; + use config::internal::routing::vrf::{VrfConfig, VrfConfigTable}; + use net::route::RouteTableId; + use net::vxlan::Vni; + use std::net::{IpAddr, Ipv4Addr}; + use std::ops::Bound::Included; + use std::str::FromStr; + + const NUM_ADDRESSES: u8 = 3; + const NUM_NETWORKS: u8 = 4; + const MAX_PEERS: u8 = 3; + const MAX_VRFS: u8 = 3; + + fn addresses() -> Vec { + ["10.0.0.1", "10.0.0.2", "2001:db8::1"] + .iter() + .map(|a| IpAddr::from_str(a).unwrap_or_else(|_| unreachable!())) + .collect() + } + + fn networks() -> Vec { + vec![ + OspfNetwork::Broadcast, + OspfNetwork::NonBroadcast, + OspfNetwork::Point2Point, + OspfNetwork::Point2Multipoint, + ] + } + + fn network_keyword(network: &OspfNetwork) -> &'static str { + match network { + OspfNetwork::Broadcast => "broadcast", + OspfNetwork::NonBroadcast => "non-broadcast", + OspfNetwork::Point2Point => "point-to-point", + OspfNetwork::Point2Multipoint => "point-to-multipoint", + } + } + + #[derive(Debug, Clone, Copy)] + struct PeerSpec { + address: usize, + multihop: bool, + source: Option, + } + + #[derive(Debug, Clone, Copy)] + struct VrfSpec { + ospf: Option, + } + + #[derive(Debug, Clone)] + struct Fabric { + genid: GenId, + peers: Vec, + vrfs: Vec, + } + + #[derive(Debug, Clone, Copy, Default)] + struct Fabrics; + + fn index(driver: &mut D, count: u8) -> Option { + driver + .gen_u8(Included(&0), Included(&(count - 1))) + .map(usize::from) + } + + fn maybe_index(driver: &mut D, count: u8) -> Option { + index(driver, count + 1) + } + + fn drawn(value: usize, count: u8) -> Option { + (value < usize::from(count)).then_some(value) + } + + impl ValueGenerator for Fabrics { + type Output = Fabric; + + fn generate(&self, driver: &mut D) -> Option { + let genid = GenId::from(driver.gen_u8(Included(&1), Included(&9))?); + + let count = driver.gen_u8(Included(&0), Included(&MAX_PEERS))?; + let mut peers = Vec::with_capacity(usize::from(count)); + for _ in 0..count { + peers.push(PeerSpec { + address: index(driver, NUM_ADDRESSES)?, + multihop: driver.produce::()?, + source: drawn(maybe_index(driver, NUM_ADDRESSES)?, NUM_ADDRESSES), + }); + } + + let count = driver.gen_u8(Included(&0), Included(&MAX_VRFS))?; + let mut vrfs = Vec::with_capacity(usize::from(count)); + for _ in 0..count { + vrfs.push(VrfSpec { + ospf: drawn(maybe_index(driver, NUM_ADDRESSES)?, NUM_ADDRESSES), + }); + } + + Some(Fabric { genid, peers, vrfs }) + } + } + + fn peer(spec: PeerSpec) -> BfdPeer { + BfdPeer::new(addresses()[spec.address]) + .set_multihop(spec.multihop) + .set_source(spec.source.map(|i| addresses()[i])) + } + + fn router_id(index: usize) -> Ipv4Addr { + Ipv4Addr::new( + 192, + 168, + 0, + u8::try_from(index + 1).unwrap_or_else(|_| unreachable!()), + ) + } + + fn vrf_name(index: usize) -> String { + format!("VPC-{index}") + } + + fn vpc_id(index: usize) -> VpcId { + VpcId::try_from(format!("vpc{index:02}").as_str()).unwrap_or_else(|_| unreachable!()) + } + + fn vni_for(index: usize) -> Vni { + Vni::new_checked(3000 + u32::try_from(index).unwrap_or_else(|_| unreachable!())) + .unwrap_or_else(|_| unreachable!()) + } + + fn internal_config(fabric: &Fabric) -> InternalConfig { + let mut config = InternalConfig::new("GW1", DeviceConfig::new()); + config.bfd_peers = fabric.peers.iter().copied().map(peer).collect(); + + let mut vrfs = VrfConfigTable::new(); + for (index, spec) in fabric.vrfs.iter().enumerate() { + let mut vrf = VrfConfig::new(&vrf_name(index), Some(vni_for(index)), false) + .set_table_id( + RouteTableId::try_from( + 100 + u32::try_from(index).unwrap_or_else(|_| unreachable!()), + ) + .unwrap_or_else(|_| unreachable!()), + ) + .set_vpc_id(vpc_id(index)); + if spec.ospf.is_some() { + let mut ospf = Ospf::new(router_id(index)); + ospf.set_vrf_name(vrf_name(index)); + vrf.set_ospf(ospf); + } + vrfs.add_vrf_config(vrf) + .unwrap_or_else(|e| unreachable!("{e}")); + } + config.vrfs = vrfs; + config + } + + fn occurrences(haystack: &str, needle: &str) -> usize { + haystack.matches(needle).count() + } + + #[test] + fn the_pools_are_the_size_the_generator_thinks() { + assert_eq!(addresses().len(), usize::from(NUM_ADDRESSES)); + assert_eq!(networks().len(), usize::from(NUM_NETWORKS)); + let count = usize::from(MAX_VRFS); + for derived in [ + (0..count).map(vrf_name).collect::>(), + (0..count).map(|i| format!("{:?}", vpc_id(i))).collect(), + (0..count).map(|i| vni_for(i).to_string()).collect(), + (0..count).map(|i| router_id(i).to_string()).collect(), + ] { + let mut sorted = derived.clone(); + sorted.sort(); + sorted.dedup(); + assert_eq!( + sorted.len(), + derived.len(), + "derived values must be distinct" + ); + } + } + + #[test] + fn a_bfd_peer_renders_the_fields_it_carries() { + bolero::check!() + .with_generator(Fabrics) + .cloned() + .for_each(|fabric: Fabric| { + for spec in &fabric.peers { + let peer = peer(*spec); + let text = peer.render(&()).to_string(); + + assert_eq!( + occurrences(&text, &format!(" peer {}", peer.address)), + 1, + "peer address once, for {spec:?} in {text}" + ); + assert_eq!( + occurrences(&text, " multihop"), + usize::from(spec.multihop), + "multihop iff set, for {spec:?} in {text}" + ); + + let source_shown = spec.multihop && spec.source.is_some(); + assert_eq!( + occurrences(&text, " source "), + usize::from(source_shown), + "a source is rendered only for a multihop peer, for {spec:?} in {text}" + ); + if source_shown { + let source = addresses()[spec.source.unwrap_or_else(|| unreachable!())]; + assert_eq!( + occurrences(&text, &format!(" source {source}")), + 1, + "the source that was set, for {spec:?} in {text}" + ); + } + + for line in [ + " no shutdown".to_string(), + format!(" detect-multiplier {BFD_DETECT_MULTIPLIER}"), + format!(" transmit-interval {BFD_TRANSMIT_INTERVAL_MS}"), + format!(" receive-interval {BFD_RECEIVE_INTERVAL_MS}"), + ] { + assert_eq!(occurrences(&text, &line), 1, "{line} once, in {text}"); + } + } + }); + } + + #[test] + fn a_bfd_section_appears_only_for_peers_it_has() { + bolero::check!() + .with_generator(Fabrics) + .cloned() + .for_each(|fabric: Fabric| { + let peers: Vec = fabric.peers.iter().copied().map(peer).collect(); + let text = peers.render(&()).to_string(); + + if peers.is_empty() { + assert!( + !text.contains("bfd"), + "an empty peer list must render no bfd section, got {text}" + ); + return; + } + + assert_eq!( + occurrences(&text, "\nbfd\n"), + 1, + "one bfd section in {text}" + ); + assert_eq!(occurrences(&text, "\nexit\n"), 1, "one exit in {text}"); + for address in addresses() { + let wanted = peers.iter().filter(|p| p.address == address).count(); + assert_eq!( + occurrences(&text, &format!(" peer {address}")), + wanted, + "{address} appears once per peer that has it, in {text}" + ); + } + }); + } + + #[test] + fn an_ospf_instance_renders_its_router_id_and_vrf() { + bolero::check!() + .with_generator(Fabrics) + .cloned() + .for_each(|fabric: Fabric| { + for (index, _) in fabric.vrfs.iter().enumerate() { + let id = router_id(index); + let name = vrf_name(index); + + let plain = Ospf::new(id).render(&()).to_string(); + assert_eq!( + occurrences(&plain, "router ospf\n"), + 1, + "an ospf instance with no vrf, in {plain}" + ); + assert_eq!( + occurrences(&plain, &format!(" ospf router-id {id}")), + 1, + "the router id, in {plain}" + ); + + let mut in_vrf = Ospf::new(id); + in_vrf.set_vrf_name(name.clone()); + let text = in_vrf.render(&()).to_string(); + assert_eq!( + occurrences(&text, &format!("router ospf vrf {name}")), + 1, + "an ospf instance in a vrf, in {text}" + ); + assert_eq!( + occurrences(&text, &format!(" ospf router-id {id}")), + 1, + "the router id, in {text}" + ); + } + }); + } + + #[test] + fn an_ospf_interface_renders_the_options_it_has() { + bolero::check!() + .with_generator(bolero::produce::<(u8, bool, Option, Option)>()) + .cloned() + .for_each( + |(area, passive, cost, network): (u8, bool, Option, Option)| { + let area = Ipv4Addr::new(0, 0, 0, area); + let network = + network.map(|n| networks()[usize::from(n) % networks().len()].clone()); + + let mut interface = OspfInterface::new(area).set_passive(passive); + if let Some(cost) = cost { + interface = interface.set_cost(cost); + } + if let Some(network) = network.clone() { + interface = interface.set_network(network); + } + let text = interface.render(&()).to_string(); + + assert_eq!( + occurrences(&text, &format!(" ip ospf area {area}")), + 1, + "the area, in {text}" + ); + assert_eq!( + occurrences(&text, " ip ospf passive"), + usize::from(passive), + "passive iff set, in {text}" + ); + assert_eq!( + occurrences(&text, " ip ospf cost "), + usize::from(cost.is_some()), + "cost iff set, in {text}" + ); + if let Some(cost) = cost { + assert_eq!(occurrences(&text, &format!(" ip ospf cost {cost}")), 1); + } + assert_eq!( + occurrences(&text, " ip ospf network "), + usize::from(network.is_some()), + "network iff set, in {text}" + ); + if let Some(network) = &network { + assert_eq!( + occurrences( + &text, + &format!(" ip ospf network {}", network_keyword(network)) + ), + 1, + "the network keyword FRR expects, in {text}" + ); + } + }, + ); + } + + #[test] + fn everything_configured_reaches_the_output() { + bolero::check!() + .with_generator(Fabrics) + .cloned() + .for_each(|fabric: Fabric| { + let config = internal_config(&fabric); + let text = config.render(&fabric.genid).to_string(); + + assert_eq!( + occurrences(&text, &format!("! config for gen {}", fabric.genid)), + 1, + "the generation this config is for, in {text}" + ); + + for address in addresses() { + let wanted = fabric + .peers + .iter() + .filter(|p| addresses()[p.address] == address) + .count(); + assert_eq!( + occurrences(&text, &format!(" peer {address}")), + wanted, + "bfd peer {address}, in {text}" + ); + } + + for (index, spec) in fabric.vrfs.iter().enumerate() { + let name = vrf_name(index); + assert_eq!( + occurrences(&text, &format!("\nvrf {name}\n")), + 1, + "vrf {name} declared once, in {text}" + ); + assert_eq!( + occurrences(&text, &format!(" vni {}", vni_for(index))), + 1, + "vni of {name}, in {text}" + ); + assert_eq!( + occurrences(&text, &format!("router ospf vrf {name}")), + usize::from(spec.ospf.is_some()), + "ospf instance of {name} iff it has one, in {text}" + ); + assert_eq!( + occurrences(&text, &format!(" ospf router-id {}", router_id(index))), + usize::from(spec.ospf.is_some()), + "router id of {name} iff it has ospf, in {text}" + ); + } + }); + } + + #[test] + fn a_vrf_renders_its_wrapper_only_when_it_is_not_the_default() { + bolero::check!() + .with_generator(bolero::produce::<(bool, bool)>()) + .cloned() + .for_each(|(default, has_vni): (bool, bool)| { + let name = if default { "default" } else { "VPC-1" }; + let vni = has_vni.then(|| vni_for(0)); + let text = VrfConfig::new(name, vni, default).render(&()).to_string(); + + let wrapped = usize::from(!default); + assert_eq!( + occurrences(&text, &format!("\nvrf {name}\n")), + wrapped, + "a vrf declaration iff not the default, in {text}" + ); + assert_eq!( + occurrences(&text, "exit-vrf"), + wrapped, + "an exit-vrf iff not the default, in {text}" + ); + assert_eq!( + occurrences(&text, " vni "), + usize::from(has_vni), + "a vni iff it has one, in {text}" + ); + }); + } + + #[test] + fn rendering_is_deterministic() { + bolero::check!() + .with_generator(Fabrics) + .cloned() + .for_each(|fabric: Fabric| { + let once = internal_config(&fabric).render(&fabric.genid).to_string(); + let twice = internal_config(&fabric).render(&fabric.genid).to_string(); + assert_eq!(once, twice, "rendering is not deterministic"); + }); + } +} From 1a8222f08b86683332f07c1e97a3c44b9b9dd7d7 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 15:20:32 -0600 Subject: [PATCH 13/14] test(routing): model-check the interface table Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- routing/src/interfaces/iftablerw.rs | 397 ++++++++++++++++++++++++++++ 1 file changed, 397 insertions(+) diff --git a/routing/src/interfaces/iftablerw.rs b/routing/src/interfaces/iftablerw.rs index db9c641424..c6aabc6c4e 100644 --- a/routing/src/interfaces/iftablerw.rs +++ b/routing/src/interfaces/iftablerw.rs @@ -213,3 +213,400 @@ impl IfTableReaderFactory { #[allow(unsafe_code)] unsafe impl Send for IfTableWriter {} + +#[cfg(test)] +mod iftable_properties { + use super::*; + use crate::fib::fibtable::FibTableWriter; + use crate::interfaces::interface::{Attachment, IfType}; + use crate::rib::vrf::{RouterVrfConfig, Vrf}; + use bolero::{Driver, ValueGenerator}; + use net::interface::address::IfAddr; + use std::collections::{BTreeMap, BTreeSet}; + use std::net::IpAddr; + use std::ops::Bound::Included; + use std::str::FromStr; + + const NUM_IFACES: u8 = 3; + const NUM_VRFS: u8 = 2; + const NUM_ADDRESSES: u8 = 2; + const NUM_STATES: u8 = 3; + const MAX_CHANGES: u8 = 12; + + fn ifindexes() -> Vec { + (1..=u32::from(NUM_IFACES)) + .map(|i| InterfaceIndex::try_new(i).unwrap_or_else(|_| unreachable!())) + .collect() + } + + fn vrf_ids() -> Vec { + (1..=u32::from(NUM_VRFS)).collect() + } + + fn addresses() -> Vec { + ["10.0.0.1", "10.0.0.2"] + .iter() + .map(|a| { + IfAddr::new(IpAddr::from_str(a).unwrap_or_else(|_| unreachable!()), 24) + .unwrap_or_else(|_| unreachable!()) + }) + .collect() + } + + fn states() -> Vec { + vec![IfState::Unknown, IfState::Down, IfState::Up] + } + + #[derive(Debug, Clone)] + enum Change { + AddInterface { iface: usize, renamed: bool }, + ModInterface { iface: usize, renamed: bool }, + DelInterface { iface: usize }, + AddAddress { iface: usize, address: usize }, + DelAddress { iface: usize, address: usize }, + SetOperState { iface: usize, state: usize }, + SetAdminState { iface: usize, state: usize }, + AttachToVrf { iface: usize, vrf: usize }, + Detach { iface: usize }, + DetachVrf { vrf: usize }, + AddVrf { vrf: usize }, + RemoveVrf { vrf: usize }, + } + + #[derive(Debug, Clone, Copy, Default)] + struct ChangeSequences; + + fn index(driver: &mut D, count: u8) -> Option { + driver + .gen_u8(Included(&0), Included(&(count - 1))) + .map(usize::from) + } + + impl ValueGenerator for ChangeSequences { + type Output = Vec; + + fn generate(&self, driver: &mut D) -> Option> { + let len = driver.gen_u8(Included(&0), Included(&MAX_CHANGES))?; + let mut out = Vec::with_capacity(usize::from(len)); + for _ in 0..len { + let iface = index(driver, NUM_IFACES)?; + let change = match driver.gen_u8(Included(&0), Included(&11))? { + 0 => Change::AddInterface { + iface, + renamed: driver.produce::()?, + }, + 1 => Change::ModInterface { + iface, + renamed: driver.produce::()?, + }, + 2 => Change::DelInterface { iface }, + 3 => Change::AddAddress { + iface, + address: index(driver, NUM_ADDRESSES)?, + }, + 4 => Change::DelAddress { + iface, + address: index(driver, NUM_ADDRESSES)?, + }, + 5 => Change::SetOperState { + iface, + state: index(driver, NUM_STATES)?, + }, + 6 => Change::SetAdminState { + iface, + state: index(driver, NUM_STATES)?, + }, + 7 => Change::AttachToVrf { + iface, + vrf: index(driver, NUM_VRFS)?, + }, + 8 => Change::Detach { iface }, + 9 => Change::DetachVrf { + vrf: index(driver, NUM_VRFS)?, + }, + 10 => Change::AddVrf { + vrf: index(driver, NUM_VRFS)?, + }, + _ => Change::RemoveVrf { + vrf: index(driver, NUM_VRFS)?, + }, + }; + out.push(change); + } + Some(out) + } + } + + fn name_of(iface: usize, renamed: bool) -> String { + if renamed { + format!("eth{iface}-renamed") + } else { + format!("eth{iface}") + } + } + + fn config_for(iface: usize, renamed: bool) -> RouterInterfaceConfig { + let mut config = RouterInterfaceConfig::new(&name_of(iface, renamed), ifindexes()[iface]); + config.set_iftype(IfType::Unknown); + config + } + + #[derive(Debug, Clone, PartialEq)] + struct IfaceState { + name: String, + admin: IfState, + oper: IfState, + attached: Option, + addresses: BTreeSet, + } + + #[derive(Debug, Clone)] + struct Model { + interfaces: BTreeMap, + vrfs: BTreeSet, + } + + impl Model { + fn new() -> Self { + Self { + interfaces: BTreeMap::new(), + vrfs: BTreeSet::new(), + } + } + } + + struct World { + iftw: IfTableWriter, + iftr: IfTableReader, + vrftable: VrfTable, + } + + fn world() -> World { + let (fibtw, _fibtr) = FibTableWriter::new(); + let (iftw, iftr) = IfTableWriter::new(); + World { + iftw, + iftr, + vrftable: VrfTable::new(fibtw), + } + } + + fn apply_add(world: &mut World, model: &mut Model, iface: usize, renamed: bool) { + let result = world.iftw.add_interface(config_for(iface, renamed)); + if model.interfaces.contains_key(&iface) { + assert!(result.is_err(), "a duplicate interface was accepted"); + return; + } + assert!(result.is_ok(), "a new interface was refused: {result:?}"); + model.interfaces.insert( + iface, + IfaceState { + name: name_of(iface, renamed), + admin: IfState::Up, + oper: IfState::Unknown, + attached: None, + addresses: BTreeSet::new(), + }, + ); + } + + fn apply_mod(world: &mut World, model: &mut Model, iface: usize, renamed: bool) { + let result = world.iftw.mod_interface(config_for(iface, renamed)); + let Some(state) = model.interfaces.get_mut(&iface) else { + assert!(result.is_err(), "an unknown interface was modified"); + return; + }; + assert!(result.is_ok(), "a known interface was refused: {result:?}"); + state.name = name_of(iface, renamed); + state.admin = IfState::Up; + } + + fn apply_attach(world: &mut World, model: &mut Model, iface: usize, vrf: usize) { + let result = + world + .iftw + .attach_interface_to_vrf(ifindexes()[iface], vrf_ids()[vrf], &world.vrftable); + let attachable = model.interfaces.contains_key(&iface) && model.vrfs.contains(&vrf); + assert_eq!(result.is_ok(), attachable, "attaching {iface} to {vrf}"); + if attachable { + model + .interfaces + .get_mut(&iface) + .unwrap_or_else(|| unreachable!()) + .attached = Some(vrf); + } + } + + fn detach_all_from(model: &mut Model, vrf: usize) { + for state in model.interfaces.values_mut() { + if state.attached == Some(vrf) { + state.attached = None; + } + } + } + + fn apply_remove_vrf(world: &mut World, model: &mut Model, vrf: usize) { + let result = world.vrftable.remove_vrf(vrf_ids()[vrf], &mut world.iftw); + assert_eq!( + result.is_ok(), + model.vrfs.contains(&vrf), + "removing vrf {vrf}" + ); + if model.vrfs.remove(&vrf) { + detach_all_from(model, vrf); + } + } + + fn apply(world: &mut World, model: &mut Model, change: &Change) { + let ifaces = ifindexes(); + let vrfs = vrf_ids(); + match change { + Change::AddInterface { iface, renamed } => apply_add(world, model, *iface, *renamed), + Change::ModInterface { iface, renamed } => apply_mod(world, model, *iface, *renamed), + Change::AttachToVrf { iface, vrf } => apply_attach(world, model, *iface, *vrf), + Change::RemoveVrf { vrf } => apply_remove_vrf(world, model, *vrf), + Change::DelInterface { iface } => { + world.iftw.del_interface(ifaces[*iface]); + model.interfaces.remove(iface); + } + Change::AddAddress { iface, address } => { + world + .iftw + .add_ip_address(ifaces[*iface], addresses()[*address]); + if let Some(state) = model.interfaces.get_mut(iface) { + state.addresses.insert(*address); + } + } + Change::DelAddress { iface, address } => { + world + .iftw + .del_ip_address(ifaces[*iface], addresses()[*address]); + if let Some(state) = model.interfaces.get_mut(iface) { + state.addresses.remove(address); + } + } + Change::SetOperState { iface, state } => { + world + .iftw + .set_iface_oper_state(ifaces[*iface], states()[*state]); + if let Some(entry) = model.interfaces.get_mut(iface) { + entry.oper = states()[*state]; + } + } + Change::SetAdminState { iface, state } => { + world + .iftw + .set_iface_admin_state(ifaces[*iface], states()[*state]); + if let Some(entry) = model.interfaces.get_mut(iface) { + entry.admin = states()[*state]; + } + } + Change::Detach { iface } => { + world.iftw.detach_interface(ifaces[*iface]); + if let Some(state) = model.interfaces.get_mut(iface) { + state.attached = None; + } + } + Change::DetachVrf { vrf } => { + world.iftw.detach_interfaces_from_vrf(vrfs[*vrf]); + detach_all_from(model, *vrf); + } + Change::AddVrf { vrf } => { + let config = RouterVrfConfig::new(vrfs[*vrf], &format!("vrf{vrf}")); + let result = world.vrftable.add_vrf(&config); + assert_eq!( + result.is_ok(), + !model.vrfs.contains(vrf), + "adding vrf {vrf}" + ); + model.vrfs.insert(*vrf); + } + } + } + + fn check(world: &World, model: &Model, at: &str) { + let ifaces = ifindexes(); + let vrfs = vrf_ids(); + let addrs = addresses(); + + for view in [ + world.iftw.enter().unwrap_or_else(|| unreachable!()), + world.iftr.enter().unwrap_or_else(|| unreachable!()), + ] { + assert_eq!(view.len(), model.interfaces.len(), "interface count {at}"); + + for (index, ifindex) in ifaces.iter().enumerate() { + let Some(iface) = view.get_interface(*ifindex) else { + assert!( + !model.interfaces.contains_key(&index), + "interface {index} missing {at}" + ); + continue; + }; + let want = model + .interfaces + .get(&index) + .unwrap_or_else(|| panic!("interface {index} unexpected {at}")); + + assert_eq!(iface.ifindex, *ifindex, "filed under the wrong key {at}"); + assert_eq!(iface.name, want.name, "name of {index} {at}"); + assert_eq!(iface.admin_state, want.admin, "admin state of {index} {at}"); + assert_eq!(iface.oper_state, want.oper, "oper state of {index} {at}"); + + let held: BTreeSet = (0..addrs.len()) + .filter(|i| iface.addresses.contains(&addrs[*i])) + .collect(); + assert_eq!(held, want.addresses, "addresses of {index} {at}"); + assert_eq!( + iface.addresses.len(), + want.addresses.len(), + "stray addresses on {index} {at}" + ); + + match (&iface.attachment, want.attached) { + (None, None) => (), + (Some(Attachment::Vrf(key)), Some(vrf)) => { + assert_eq!(*key, FibKey::Id(vrfs[vrf]), "attachment of {index} {at}"); + } + (got, want) => { + panic!("attachment of {index} is {got:?}, expected {want:?} {at}") + } + } + + if let Some(Attachment::Vrf(FibKey::Id(vrfid))) = &iface.attachment { + assert!( + world.vrftable.contains(*vrfid), + "interface {index} is attached to vrf {vrfid}, which is gone {at}" + ); + } + } + } + } + + #[test] + fn the_pools_are_the_size_the_generator_thinks() { + assert_eq!(ifindexes().len(), usize::from(NUM_IFACES)); + assert_eq!(vrf_ids().len(), usize::from(NUM_VRFS)); + assert_eq!(addresses().len(), usize::from(NUM_ADDRESSES)); + assert_eq!(states().len(), usize::from(NUM_STATES)); + assert!(!vrf_ids().contains(&Vrf::DEFAULT_VRFID)); + assert_ne!(name_of(0, false), name_of(0, true)); + } + + #[test] + fn an_interface_tables_state_and_attachments_stay_in_step() { + bolero::check!() + .with_generator(ChangeSequences) + .cloned() + .for_each(|changes: Vec| { + let mut world = world(); + let mut model = Model::new(); + + check(&world, &model, "on a fresh table"); + for (step, change) in changes.iter().enumerate() { + apply(&mut world, &mut model, change); + check(&world, &model, &format!("at step {step} of {changes:?}")); + } + }); + } +} From bb22af35d58686bf2708704f4f7928f19c94777c Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 15:31:17 -0600 Subject: [PATCH 14/14] test(routing): prop-test the adjacency table Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- routing/src/atable/atablerw.rs | 264 +++++++++++++++++++++++++++++++++ routing/src/atable/resolver.rs | 60 ++++++++ 2 files changed, 324 insertions(+) diff --git a/routing/src/atable/atablerw.rs b/routing/src/atable/atablerw.rs index 3b4ae9eb75..bbbbedf9ef 100644 --- a/routing/src/atable/atablerw.rs +++ b/routing/src/atable/atablerw.rs @@ -82,3 +82,267 @@ impl AtableReaderFactory { AtableReader(self.0.handle()) } } + +#[cfg(test)] +mod atable_properties { + use super::*; + use bolero::{Driver, ValueGenerator}; + use net::eth::mac::Mac; + use std::collections::BTreeMap; + use std::net::IpAddr; + use std::ops::Bound::Included; + use std::str::FromStr; + + const NUM_IFACES: u8 = 2; + const NUM_ADDRESSES: u8 = 3; + const NUM_MACS: u8 = 2; + const MAX_CHANGES: u8 = 12; + + fn ifindexes() -> Vec { + (1..=u32::from(NUM_IFACES)) + .map(|i| InterfaceIndex::try_new(i).unwrap_or_else(|_| unreachable!())) + .collect() + } + + fn addresses() -> Vec { + ["10.0.0.1", "10.0.0.2", "10.0.0.3"] + .iter() + .map(|a| IpAddr::from_str(a).unwrap_or_else(|_| unreachable!())) + .collect() + } + + fn macs() -> Vec { + vec![ + Mac::from([0x00, 0xaa, 0x00, 0x00, 0x00, 0x01]), + Mac::from([0x00, 0xbb, 0x00, 0x00, 0x00, 0x02]), + ] + } + + #[derive(Debug, Clone)] + enum Change { + Add { + iface: usize, + address: usize, + mac: usize, + publish: bool, + }, + Del { + iface: usize, + address: usize, + publish: bool, + }, + Clear { + publish: bool, + }, + Publish, + } + + #[derive(Debug, Clone, Copy, Default)] + struct ChangeSequences; + + fn index(driver: &mut D, count: u8) -> Option { + driver + .gen_u8(Included(&0), Included(&(count - 1))) + .map(usize::from) + } + + impl ValueGenerator for ChangeSequences { + type Output = Vec; + + fn generate(&self, driver: &mut D) -> Option> { + let len = driver.gen_u8(Included(&0), Included(&MAX_CHANGES))?; + let mut out = Vec::with_capacity(usize::from(len)); + for _ in 0..len { + let change = match driver.gen_u8(Included(&0), Included(&3))? { + 0 => Change::Add { + iface: index(driver, NUM_IFACES)?, + address: index(driver, NUM_ADDRESSES)?, + mac: index(driver, NUM_MACS)?, + publish: driver.produce::()?, + }, + 1 => Change::Del { + iface: index(driver, NUM_IFACES)?, + address: index(driver, NUM_ADDRESSES)?, + publish: driver.produce::()?, + }, + 2 => Change::Clear { + publish: driver.produce::()?, + }, + _ => Change::Publish, + }; + out.push(change); + } + Some(out) + } + } + + type Entries = BTreeMap<(usize, usize), usize>; + + #[derive(Debug, Clone, Default)] + struct Model { + appended: Entries, + published: Entries, + } + + impl Model { + fn publish(&mut self) { + self.published = self.appended.clone(); + } + + fn apply(&mut self, change: &Change) { + match change { + Change::Add { + iface, + address, + mac, + publish, + } => { + self.appended.insert((*iface, *address), *mac); + if *publish { + self.publish(); + } + } + Change::Del { + iface, + address, + publish, + } => { + self.appended.remove(&(*iface, *address)); + if *publish { + self.publish(); + } + } + Change::Clear { publish } => { + self.appended.clear(); + if *publish { + self.publish(); + } + } + Change::Publish => self.publish(), + } + } + } + + fn apply_to_table(writer: &mut AtableWriter, change: &Change) { + let ifaces = ifindexes(); + let addrs = addresses(); + match change { + Change::Add { + iface, + address, + mac, + publish, + } => writer.add_adjacency( + Adjacency::new(addrs[*address], ifaces[*iface], macs()[*mac]), + *publish, + ), + Change::Del { + iface, + address, + publish, + } => writer.del_adjacency(addrs[*address], ifaces[*iface], *publish), + Change::Clear { publish } => writer.clear(*publish), + Change::Publish => writer.publish(), + } + } + + fn seen(table: &AdjacencyTable) -> Entries { + let ifaces = ifindexes(); + let addrs = addresses(); + let all = macs(); + let mut out = Entries::new(); + for (iface, ifindex) in ifaces.iter().enumerate() { + for (address, addr) in addrs.iter().enumerate() { + if let Some(adjacency) = table.get_adjacency(*addr, *ifindex) { + let mac = all + .iter() + .position(|m| *m == adjacency.get_mac()) + .unwrap_or_else(|| unreachable!()); + assert_eq!(adjacency.get_ifindex(), *ifindex, "adjacency ifindex"); + assert_eq!(adjacency.get_ip(), *addr, "adjacency address"); + out.insert((iface, address), mac); + } + } + } + assert_eq!( + out.len(), + table.len(), + "the table holds entries outside the pools" + ); + out + } + + #[test] + fn the_pools_are_the_size_the_generator_thinks() { + assert_eq!(ifindexes().len(), usize::from(NUM_IFACES)); + assert_eq!(addresses().len(), usize::from(NUM_ADDRESSES)); + assert_eq!(macs().len(), usize::from(NUM_MACS)); + assert_ne!(macs()[0], macs()[1]); + } + + #[test] + fn a_reader_sees_the_table_as_of_the_last_publish() { + bolero::check!() + .with_generator(ChangeSequences) + .cloned() + .for_each(|changes: Vec| { + let (mut writer, reader) = AtableWriter::new(); + let mut model = Model::default(); + + for (step, change) in changes.iter().enumerate() { + apply_to_table(&mut writer, change); + model.apply(change); + let at = format!("at step {step} of {changes:?}"); + + let view = reader.enter().unwrap_or_else(|| unreachable!()); + assert_eq!(seen(&view), model.published, "reader {at}"); + } + + writer.publish(); + model.publish(); + let view = reader.enter().unwrap_or_else(|| unreachable!()); + assert_eq!(seen(&view), model.appended, "reader after a final publish"); + }); + } + + #[test] + fn a_clear_and_repopulate_is_invisible_until_published() { + let (mut writer, reader) = AtableWriter::new(); + let ifindex = ifindexes()[0]; + let (old, new) = (addresses()[0], addresses()[1]); + + writer.add_adjacency(Adjacency::new(old, ifindex, macs()[0]), true); + assert!( + reader + .enter() + .unwrap_or_else(|| unreachable!()) + .get_adjacency(old, ifindex) + .is_some() + ); + + writer.clear(false); + writer.add_adjacency(Adjacency::new(new, ifindex, macs()[1]), false); + + let view = reader.enter().unwrap_or_else(|| unreachable!()); + assert!( + view.get_adjacency(old, ifindex).is_some(), + "the table emptied under a reader mid-refresh" + ); + assert!( + view.get_adjacency(new, ifindex).is_none(), + "an unpublished addition was visible" + ); + drop(view); + + writer.publish(); + let view = reader.enter().unwrap_or_else(|| unreachable!()); + assert!( + view.get_adjacency(old, ifindex).is_none(), + "the clear was lost" + ); + assert!( + view.get_adjacency(new, ifindex).is_some(), + "the addition was lost" + ); + } +} diff --git a/routing/src/atable/resolver.rs b/routing/src/atable/resolver.rs index c86f342e99..972aa866d5 100644 --- a/routing/src/atable/resolver.rs +++ b/routing/src/atable/resolver.rs @@ -148,6 +148,66 @@ impl AtResolver { } } +#[cfg(test)] +mod resolver_properties { + use super::*; + use netdev::Interface; + + fn interface(index: u32, name: &str) -> Interface { + Interface { + index, + name: name.to_string(), + ..Interface::dummy() + } + } + + #[test] + fn a_device_name_resolves_to_its_own_interface() { + let interfaces = [interface(2, "eth0"), interface(3, "eth1")]; + + for (index, name) in [(2, "eth0"), (3, "eth1")] { + let found = get_interface_ifindex(&interfaces, name) + .unwrap_or_else(|e| unreachable!("{e}")) + .unwrap_or_else(|| panic!("{name} did not resolve")); + assert_eq!(found.to_u32(), index, "{name} resolved to the wrong index"); + } + + assert_eq!( + get_interface_ifindex(&interfaces, "eth2").unwrap_or_else(|e| unreachable!("{e}")), + None, + "an unknown device must resolve to nothing, not to something else" + ); + assert_eq!( + get_interface_ifindex(&[], "eth0").unwrap_or_else(|e| unreachable!("{e}")), + None, + "no interfaces, nothing to resolve to" + ); + } + + #[test] + fn an_interface_index_of_zero_is_an_error_not_a_miss() { + let interfaces = [interface(0, "eth0")]; + assert!( + get_interface_ifindex(&interfaces, "eth0").is_err(), + "index zero must be an error" + ); + assert_eq!( + get_interface_ifindex(&interfaces, "eth1").unwrap_or_else(|e| unreachable!("{e}")), + None, + "a different name is still just a miss" + ); + } + + #[test] + fn a_repeated_device_name_resolves_to_the_first() { + let interfaces = [interface(2, "eth0"), interface(9, "eth0")]; + let found = get_interface_ifindex(&interfaces, "eth0") + .unwrap_or_else(|e| unreachable!("{e}")) + .unwrap_or_else(|| unreachable!()); + assert_eq!(found.to_u32(), 2); + } +} + #[cfg(test)] pub mod tests { use super::*;