diff --git a/routing/src/atable/atablerw.rs b/routing/src/atable/atablerw.rs index 3b4ae9eb75..c173b63caf 100644 --- a/routing/src/atable/atablerw.rs +++ b/routing/src/atable/atablerw.rs @@ -82,3 +82,298 @@ impl AtableReaderFactory { AtableReader(self.0.handle()) } } + +/// Model-based properties over the adjacency table and its left-right wrapper. +/// +/// The table is a map. What is not map-like is the *publishing*: `add_adjacency`, `del_adjacency` +/// and `clear` all take a `publish` flag, and `AtResolver::refresh_atable_from_proc` relies on it -- +/// it clears the table with `publish: false`, adds every entry it found with `publish: false`, and +/// publishes once at the end. So a reader must never observe the cleared-but-not-yet-repopulated +/// state. If it could, the egress stage would find an empty adjacency table on every refresh and +/// have no destination mac for anything. +#[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]), + ] + } + + /// One change, over indices into the pools above. Every mutation carries the writer's `publish` + /// flag, since whether a change is visible yet is the point. + #[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, + } + + /// Draws sequences of [`Change`]s. + #[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) + } + } + + /// The adjacencies, by `(interface, address)` index pair, to mac index. + type Entries = BTreeMap<(usize, usize), usize>; + + /// Two states, which is the whole point: what the writer has appended, and what a reader is + /// entitled to see. + #[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, + } => { + // an adjacency is keyed by the interface and address it carries, so re-learning + // one with a different mac replaces it + 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(), + } + } + + /// Everything a reader can see of the table, as index pairs. + 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!()); + // and the adjacency agrees with the key it was found under + 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 + } + + /// The pools and the constants that index them agree. + #[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)); + // the macs must be distinguishable, or a replacement would not be visible + assert_ne!(macs()[0], macs()[1]); + } + + /// A reader sees the table as of the last publish, and never an intermediate state. + /// + /// The unpublished half is what `AtResolver::refresh_atable_from_proc` depends on: it clears and + /// repopulates the whole table without publishing, so between the two a reader must still see + /// the previous contents rather than nothing. + #[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}"); + } + + // and once everything is published, the reader sees everything appended + writer.publish(); + model.publish(); + let view = reader.enter().unwrap_or_else(|| unreachable!()); + assert_eq!(seen(&view), model.appended, "reader after a final publish"); + }); + } + + /// A refresh that clears and repopulates without publishing is invisible until it does. + /// + /// The same claim as above, spelled out in the shape the resolver actually uses, because that is + /// the sequence whose failure would empty the adjacency table under the egress stage. + #[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() + ); + + // a refresh: clear, repopulate, and only then publish + 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..c97cfc1991 100644 --- a/routing/src/atable/resolver.rs +++ b/routing/src/atable/resolver.rs @@ -148,6 +148,80 @@ impl AtResolver { } } +/// Properties over the one part of the resolver that does not need `/proc`. +/// +/// `refresh_atable_from_proc` reads the kernel's ARP table and its interface list, so it cannot be +/// driven from a test. What can is the step between them: resolving the device name an ARP entry +/// carries to an interface index. Every ARP entry the kernel reports goes through it, and an entry +/// it cannot resolve is dropped -- so getting it wrong loses adjacencies silently. +#[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() + } + } + + /// A device name resolves to the index of the interface that has it, and to nothing otherwise. + #[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" + ); + } + + /// An interface index of zero is refused rather than turned into an adjacency. + /// + /// `InterfaceIndex` is non-zero, and the kernel should never report a zero index -- but the + /// resolver takes the number from outside, so the distinction between "no such device" and "a + /// device whose index we cannot represent" has to survive: the first is `Ok(None)` and drops the + /// entry quietly, the second is an error and says why. + #[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" + ); + } + + /// The first interface with a name wins, and a later one with the same name does not shadow it. + #[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::*; diff --git a/routing/src/cli/display.rs b/routing/src/cli/display.rs index 9c40338de0..e400c9793e 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,40 @@ 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 { +/// Print a next-hop's resolvers, and theirs, and so on down. +/// +/// `path` holds the next-hops between the one being displayed and this one. A resolver already on +/// that path closes a resolution loop: we name it and stop, since the recursion has nothing new to +/// show and would otherwise run until `depth` overflowed -- which it did, panicking from inside a +/// `Display` impl that both the CLI and the warning about resolution loops go through. +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 +181,14 @@ 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 { +// +// `path` guards against a resolution loop, as in `fmt_nhop_resolvers` above. +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 +204,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 +214,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 +229,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 +430,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 +448,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/fib/fibobjects.rs b/routing/src/fib/fibobjects.rs index 70c7b25151..4830ebd96f 100644 --- a/routing/src/fib/fibobjects.rs +++ b/routing/src/fib/fibobjects.rs @@ -262,3 +262,197 @@ 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; + + // Values come from a small alphabet, so that several egress objects disagreeing about the same + // field is the common case rather than a rarity. Deciding between them is the whole of what + // `squash` does; independently drawn values would almost never collide. + 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!())) + } + + // Zero selects `None`, which is one of the choices for every field: an unresolved ifindex or a + // missing address is exactly what the merge rules are about. The draw is separate from the + // choice so that a driver running out of input stays distinct from a generated `None`. + 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))? { + // `Local` carries a distinguishable index so that order preservation can be checked + // rather than merely counted. + 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() + } + + /// Squashing an entry leaves everything that is not an egress exactly as it was. + /// + /// The instructions before the egress are what gets executed on the way out -- encapsulation + /// above all -- so reordering or dropping one of them changes what goes on the wire. + #[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(); + // A single instruction is returned untouched, egress or not. + if entry.len() == 1 { + assert_eq!(squashed, entry); + return; + } + let after: Vec = others(&squashed).into_iter().cloned().collect(); + assert_eq!(after, before, "for {entry:?}"); + }); + } + + /// What comes out has at most one egress, and it is last. + /// + /// Last because `FibEntry::is_valid` requires it: a multi-instruction entry is only usable if + /// its final instruction is an egress with a known interface. + #[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:?}"); + } + }); + } + + /// The merged egress is the first interface, the last address and the first name. + /// + /// The asymmetry is deliberate and load-bearing: `rib2fib` relies on it so that the address of + /// a recursive next-hop reaches the fib while the interface of the nearest resolver wins. An + /// oracle worked out from the input directly, rather than by folding with `merge` again. + #[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:?}"); + } + // An egress survives exactly when one of the inputs knew an interface. With + // none, there is nowhere to send the packet and the egress is dropped -- which + // is what leaves the entry to be refused by `is_valid`. + None => assert!(ifindex.is_none(), "for {entry:?}"), + } + }); + } + + /// Squashing twice is squashing once. + /// + /// Worth pinning because the entries are built by accumulating down a resolution chain and + /// squashed at the end of it; a squash that drifted on a second application would make the + /// result depend on how many times the chain was walked. + #[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/fib/fibtable.rs b/routing/src/fib/fibtable.rs index 5fbab2a74a..ef62d9c024 100644 --- a/routing/src/fib/fibtable.rs +++ b/routing/src/fib/fibtable.rs @@ -39,9 +39,14 @@ impl FibTable { self.entries.insert(id, entry); } /// Delete a `Fib`, by unregistering a `FibReaderFactory` for it + /// + /// Every key that reaches the fib goes, not just its own: a fib registered under a [`Vni`] is + /// reachable by that alias too, and an alias must not outlive the fib it names. Each entry + /// records the identity of the fib it points at, so the table can find its own aliases rather + /// than relying on the caller to remember which vni a fib was registered under. 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 +149,14 @@ 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)); - } + /// Remove the fib for `vrfid`, and with it every key that reached it. + /// + /// This used to take the fib's [`Vni`] so as to drop that alias as well, which made a leaked + /// alias a matter of the caller passing the right thing. [`FibTable::del_fib`] now finds the + /// aliases itself. + pub fn del_fib(&mut self, vrfid: VrfId) { + self.0 + .append(FibTableChange::Del(FibKey::from_vrfid(vrfid))); self.0.publish(); } } @@ -234,3 +241,215 @@ impl FibTableReader { Ok(FibReader::rc_from_rc_rhandle(rhandle)) } } + +/// Model-based properties over a [`FibTable`]. +/// +/// The table is a map, so most of it is uninteresting. The part that is not is the **vni alias**: a +/// fib is reachable both by its own [`FibKey::Id`] and, optionally, by a [`FibKey::Vni`] pointing +/// at the same entry. Nothing in the table ties the two together -- `del_fib` removes the alias +/// only because the caller passes the vni it was registered with -- so an alias outliving its fib +/// is the failure worth generating for. +#[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() + } + + /// Every key a generated table could be looked up by. + fn keys() -> Vec { + vrf_ids() + .into_iter() + .map(FibKey::from_vrfid) + .chain(vnis().into_iter().map(FibKey::from_vni)) + .collect() + } + + /// One change, as [`FibTableWriter`] exposes them, over indices into the pools above. + #[derive(Debug, Clone)] + enum Change { + AddFib { vrf: usize, vni: Option }, + RegisterByVni { vrf: usize, vni: usize }, + UnregisterVni { vni: usize }, + DelFib { vrf: usize }, + } + + /// Draws sequences of [`Change`]s. + #[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 => { + // A fib created with no vni and aliased later, or never, is the ordinary + // case, so it is worth drawing. Drawn as one index over `NUM_VNIS + 1` + // with the last meaning "none": a helper returning `Option>` + // cannot tell "no vni" from the driver running out of input. + 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) + } + } + + /// Which fib each key should reach, by vrf id. The table stripped of its left-right wrapping, + /// its `Arc` sharing and its reader factories. + type Model = BTreeMap; + + /// The writers behind a generated table, which the harness has to keep alive: a `FibReader` + /// whose `FibWriter` is gone cannot be entered, and that would look like an alias fault. + struct Fibs { + live: BTreeMap, + /// Writers displaced by a second `add_fib` for the same vrf. Nothing in the table points at + /// them any more, but they are not destroyed either, so they are parked rather than + /// dropped -- dropping one is not what production does on a replacement. + 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); + // the table refuses to alias a fib it does not hold + 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); + // every key that reached this fib goes, alias included + model.retain(|_, named| *named != vrf); + // production destroys the fib once the table no longer names it, which is what + // makes a leaked alias observable: it would hand out a reader that cannot be + // entered + if let Some(writer) = fibs.live.remove(&vrf) { + writer.destroy(); + } + } + } + } + + /// The pools and the constants that index them agree. + #[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)); + } + + /// Every key a fib table holds reaches a live fib, and reaches it under its own identity. + /// + /// Two things at once, and the second is the point. A `FibKey::Vni` is an alias for a + /// `FibKey::Id`, and the thread-local read-handle cache keys on the identity the table reports + /// for a key, not on the key asked for -- so an alias reporting the wrong identity would have + /// two threads caching handles to different fibs under one name. Nothing else checks that the + /// alias and the entry it aliases stay in step. + #[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/fibtype.rs b/routing/src/fib/fibtype.rs index ea368bb36b..f2d597c40f 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -504,3 +504,412 @@ impl FibReaderFactory { FibReader(self.0.handle()) } } + +/// Model-based properties over a [`Fib`] driven through its writer. +/// +/// The generator here is the one the pipeline harness will want: it produces a *populated* fib, +/// reached the way production reaches one -- a sequence of `FibChange`s through a `FibWriter` -- +/// rather than by reaching into the tries. Everything the fib is asked afterwards is checked +/// against a model kept alongside it. +#[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; + + /// The drop next-hop, first in [`nhop_keys`]. The store creates its group at construction and + /// refuses to delete it. + const DROP_KEY: usize = 0; + /// `0.0.0.0/0` and `::/0`, at these indices in [`prefixes`]. A fib always carries a route for + /// both: `Fib::lpm` has no answer for an address nothing covers, and says so with an + /// `unreachable!()` on the forwarding path. + const ROOT_V4: usize = 0; + const ROOT_V6: usize = 5; + + /// The next-hop keys a generated fib may mention. + /// + /// A small pool on purpose. What is worth exercising is the collisions -- a route pinning a + /// group against deletion, a registration mutating a group two routes share -- and collisions + /// need a small pool to happen often. The drop key is in it because the rib does register + /// groups under it, and because the store treats it as permanent. + 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), + ] + } + + /// The prefixes a generated fib may carry routes for. Nested on purpose, so a longest match + /// has something to be longer than, and both roots so that deleting one is reachable. + 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() + } + + /// Addresses to look up: one inside each level of the nesting, and one outside all of them in + /// each family, so a lookup that falls through to the root is exercised too. + 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() + } + + /// The entries a generated group may be built from. + 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() + } + + /// One change, as the writer API exposes them, over indices into the pools above. + #[derive(Debug, Clone)] + enum Change { + RegisterGroup { key: usize, entries: Vec }, + UnregisterGroup { key: usize }, + AddRoute { prefix: usize, keys: Vec }, + DelRoute { prefix: usize }, + } + + /// Draws sequences of [`Change`]s. + #[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> { + // Deliberately able to draw none: an empty group and a route with no next-hops are both + // things the fib is supposed to refuse, and refusing is behaviour worth checking. + 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) + } + } + + /// What the fib should hold, tracked beside it. + /// + /// Not a reimplementation: two maps, which is the fib with the tries, the group store's + /// reference counting and its `UnsafeCell` sharing all taken away. What the model does have to + /// get right is which changes the fib *refuses*, and that is the part worth writing down -- + /// three guards in three files decide it, and none of them says so where the next one can see. + #[derive(Debug, Clone)] + struct Model { + /// next-hop key index -> the entries of its group. + groups: BTreeMap>, + /// prefix index -> the next-hop keys of its route, in order. + routes: BTreeMap>, + } + + impl Model { + /// A fresh fib: a drop group, and a route to it for each root. + 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)) + } + + /// Drop every group no route points at. The store does this by reference count; here the + /// routes are the reference count. + 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 } => { + // a group with no entries is refused: a route reaching one would leave the + // forwarder with nothing to execute + if entries.is_empty() { + return; + } + let entries = entries.iter().map(|i| pool[*i].clone()).collect(); + self.groups.insert(*key, entries); + } + Change::UnregisterGroup { key } => { + // the drop group is permanent, and a group a route still names is pinned + if *key == DROP_KEY || self.referenced(*key) { + return; + } + self.groups.remove(key); + } + Change::AddRoute { prefix, keys } => { + // a route with no next-hops is refused, and so is one naming a group that was + // never registered -- whole, not in part + if keys.is_empty() || keys.iter().any(|k| !self.groups.contains_key(k)) { + return; + } + // note: no purge here. Replacing a route releases the old route's hold on its + // groups, but the fib leaves them in the store until something purges. + self.routes.insert(*prefix, keys.clone()); + } + Change::DelRoute { prefix } => { + // a root route is not deleted but reset to drop, so that a lookup always has + // an answer + 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(); + } + } + } + } + + /// The longest prefix carrying a route that covers `addr`. + 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()) + } + + /// The entries a route offers: its groups' entries, concatenated in next-hop key order. + 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]), + } + } + + /// The pools and the constants that index them agree. + #[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" + ); + } + } + + /// After any sequence of changes, a fib answers every lookup the way the model says. + /// + /// This is the whole of the fib's read path against an independent account of its contents: + /// which prefix the lookup lands on, and which entries the route there offers. It covers the + /// group store's sharing too, since registering a group under a key two routes name has to + /// change what both of them offer. + #[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()); + } + } + }); + } + + /// A lookup always lands on a route with at least one entry to execute. + /// + /// `Fib::lpm_entry_prefix` panics outright on a route with none -- "hit route without + /// fibgroups/entries. This is a bug." -- on the forwarding path, for every packet that reaches + /// it. The invariant that saves it is held jointly by three guards in three files: the store + /// refuses an empty group, the writer refuses a route with no next-hop keys, and + /// `FibRoute::from_nhopkeys` refuses a route naming a group that is not registered. Nothing + /// states the invariant they add up to, so state it here. + #[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:?}"); + // and the index arithmetic that picks among them is total over that range + for index in 0..route.len() { + let _ = route.get_fibentry(index); + } + } + }); + } + + /// A reader sees what the writer sees, once every change has been published. + #[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:?}"); + } + }); + } +} 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/frr/frrmi.rs b/routing/src/frr/frrmi.rs index 50f9ad802d..c8311e71e2 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,14 @@ struct IoBuffer { used: usize, } impl IoBuffer { + /// Octets of `|length|genid|` ahead of every message body. + const HEADER_LEN: usize = 16; + + /// The largest message body we will accept off the wire. Responses from the frr-agent are a + /// status word or an error message, so this is generous by orders of magnitude; it is here to + /// bound the allocation, not to constrain the protocol. + const MAX_MSG_LEN: usize = 16 * 1024 * 1024; + #[must_use] #[allow(unused)] pub fn new() -> Self { @@ -412,6 +425,10 @@ impl IoBuffer { self.buffer.clear(); self.used = 0; } + /// The size of the buffer. Meaningful on the **write** side only, where `serialize` fills it: + /// there, `buffer.len()` is the size of the message and `used` is how much of it has gone out. + /// On the read side `recv` resizes the buffer to the size of the read it is about to attempt, so + /// `buffer.len()` says what we hoped for and only `used` says what arrived. #[must_use] fn len(&self) -> usize { self.buffer.len() @@ -426,37 +443,45 @@ 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. + /// Tell the length that a message (encoded as |length|genid|data|) must have, once the whole + /// header has been received. `None` until then. + /// + /// Off `used`, not off `buffer.len()`: see [`IoBuffer::len`]. Reading the length out of a + /// buffer sized for a read that has not happened yet takes whatever `resize` zero-filled for + /// the tail of the header, so a half-arrived header reads as a complete one announcing a + /// shorter message. #[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) + } + + /// The announced message length, if it is one we refuse to accept. + /// + /// The length prefix comes off the wire and `recv` resizes the read buffer to it, so without a + /// bound a confused or hostile frr-agent could ask us to allocate up to `u64::MAX`. + #[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. + + /// Tell the number of octets that should be read next according to what has been received so + /// far, to get a message or be able to determine its length. + /// Until the whole header has arrived, this returns the number needed to complete it. + /// After that, the number of octets still 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 +489,9 @@ impl IoBuffer { #[must_use] fn is_ready(&self) -> bool { match self.msg_len() { - Some(m) => self.len() == m + 16, + // subtracting rather than adding 16: the announced length is peer-supplied, and + // `m + 16` overflows for one close enough to `usize::MAX` + Some(msg_len) => self.used - Self::HEADER_LEN == msg_len, None => false, } } @@ -488,3 +515,195 @@ impl IoBuffer { Ok(FrrmiResponse { genid, data }) } } + +/// Properties over the frrmi wire framing. +/// +/// `IoBuffer` exists to cope with partial reads and writes on a stream socket -- so the property +/// worth having is that the message survives *any* division of its octets into reads. The oracle is +/// the message itself: serialize, deliver the bytes in generated chunks, and what comes back out +/// must be what went in. +#[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; + + /// A message, and the way the peer's octets happen to arrive. + #[derive(Debug, Clone)] + struct Delivery { + genid: GenId, + /// The response body. Empty is worth generating: a zero-length body makes every octet of + /// the length prefix zero, which is the value a half-read header is indistinguishable from. + body: String, + /// Sizes of the successive writes the peer makes. Applied in order and clamped to what is + /// left; anything still unsent after the list runs out goes in one final write. + chunks: Vec, + } + + /// Draws [`Delivery`]s. + #[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, + }) + } + } + + /// A `Frrmi` reading from one end of a socket pair, with the peer's end alongside it. + 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) + } + + /// Draws a sequence of [`Delivery`]s to send down one connection. + #[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) + } + } + + /// A message survives being delivered in any sequence of chunks. + #[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:?}"); + }); + } + + /// A connection carries one message after another, however the octets are divided. + /// + /// The chunking runs over the whole stream rather than over each message, so a single write may + /// straddle a message boundary or carry several messages at once. `deserialize` clears the read + /// buffer, and this is what says nothing is left in it to confuse the message after. + #[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(); + + // one chunk list over the whole stream, taken from the deliveries in turn + 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; + + // drain: a single write may have completed more than one message + 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:?}"); + }); + } + + /// A message longer than we will accept is refused, not allocated for. + /// + /// The length prefix is whatever the peer put on the wire, and `recv` resizes the read buffer to + /// it. Announcing `u64::MAX` used to make that resize overflow its own length arithmetic. + #[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" + ); + } +} diff --git a/routing/src/frr/renderer/mod.rs b/routing/src/frr/renderer/mod.rs index 64a7f89837..75de135fa6 100644 --- a/routing/src/frr/renderer/mod.rs +++ b/routing/src/frr/renderer/mod.rs @@ -66,3 +66,531 @@ impl Render for InternalConfig { cfg } } + +/// Properties over the FRR config renderers. +/// +/// A renderer's failure mode is not a crash: it is a BFD session, an OSPF area or a vni that never +/// gets configured because a line was not emitted, or that gets configured twice. Neither shows up +/// anywhere but in FRR's own state. Each renderer had one test that printed its output and asserted +/// nothing, so `ospf`, `bfd` and this module were at zero coverage. +/// +/// A round-trip oracle would need an FRR parser and is not worth building. What is worth checking is +/// weaker and still catches both failure modes: **every value the config holds appears in the +/// output, and no value it does not hold appears.** That is enough to catch an omitted field, a +/// field emitted when it should not be, and a top-level renderer that forgot to call a sub-renderer. +#[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, + ] + } + + /// The keyword FRR expects for each network type, written out here rather than taken from + /// `OspfNetwork::rendered`, so a wrong pairing is visible. + 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", + } + } + + /// A BFD peer as the generator describes it. + #[derive(Debug, Clone, Copy)] + struct PeerSpec { + address: usize, + multihop: bool, + source: Option, + } + + /// A vrf as the generator describes it. + /// + /// Name, table id, vni and vpc id are all derived from its position rather than generated. + /// `VrfConfigTable` is a multi-index map with a *unique* index over each of them, and an + /// `Option` field's `None` counts as a value there -- so it can hold at most one vrf without a + /// vni, and at most one without a vpc id. Production satisfies that (the default vrf has + /// neither; every other vrf is a vpc vrf and has both), but the types do not say so, and + /// `add_vrf_config` calls a collision "a bug". So the harness gives every vrf its own, and the + /// "renders it only when it has one" cases are checked against `VrfConfig` directly below. + #[derive(Debug, Clone, Copy)] + struct VrfSpec { + ospf: Option, + } + + #[derive(Debug, Clone)] + struct Fabric { + genid: GenId, + peers: Vec, + vrfs: Vec, + } + + /// Draws [`Fabric`]s. + #[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) + } + + /// An index into a pool of `count`, or `count` itself to mean "absent". + /// + /// One draw with a sentinel rather than an `Option>`, whose outer `None` -- the driver + /// running out of input -- cannot be told from the inner one. + fn maybe_index(driver: &mut D, count: u8) -> Option { + index(driver, count + 1) + } + + /// Read a [`maybe_index`] draw back as an option. + 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])) + } + + /// A router id for vrf `index`, distinct per vrf so it can be looked for in the output. + 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}") + } + + /// A vpc id for vrf `index`. + /// + /// Every non-default vrf needs one: `VrfConfigTable` holds a unique index over `vpc_id`, so two + /// vrfs without one collide. Real configs always have them -- non-default vrfs are vpc vrfs -- + /// but it is not obvious from the type, which takes an `Option`. + 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 + } + + /// How many times `needle` appears in `haystack`. + fn occurrences(haystack: &str, needle: &str) -> usize { + haystack.matches(needle).count() + } + + /// The pools and the constants that index them agree. + #[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)); + // the derived names, table ids and vnis must be distinct, or `add_vrf_config` would refuse + // them and the harness would be testing its own collision handling + 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" + ); + } + } + + /// A BFD peer renders every field it carries, and none it does not. + /// + /// Note the one rule that is not "render what is set": a source address is emitted only for a + /// multihop peer. A single-hop peer with a source silently loses it, which is deliberate -- + /// FRR has nowhere to put it -- and worth having written down. + #[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}" + ); + } + + // and the timing parameters FRR needs to bring the session up at all + 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}"); + } + } + }); + } + + /// A BFD section appears only when there are peers, and holds each of them once. + #[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}" + ); + } + }); + } + + /// An OSPF instance renders its router id, and its vrf only when it has one. + #[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}" + ); + } + }); + } + + /// An OSPF interface renders its area, and each option only when it is set. + #[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}" + ); + } + }, + ); + } + + /// Everything the config holds reaches the rendered output. + /// + /// This is the property the per-renderer ones cannot give: a sub-renderer that works perfectly is + /// no use if the top level never calls it, and an object silently missing from an FRR config is + /// a session or a vni that never comes up. + #[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}" + ); + } + }); + } + + /// A vrf renders its own name and vni only when it should. + /// + /// The default vrf is the interesting case: its configuration belongs at the top level of the + /// FRR config, so it must *not* be wrapped in `vrf ` / `exit-vrf`. Wrapping it would put + /// the underlay's static routes and vni into a vrf that FRR does not have. + #[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}" + ); + }); + } + + /// Rendering the same config twice gives the same text. + /// + /// Worth its own property because the output is handed to `frr-reload.py`, which diffs it against + /// what FRR is running. A rendering that varied -- an unordered table iterated, say -- would look + /// like a configuration change on every pass and reload FRR for nothing. + #[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"); + }); + } +} diff --git a/routing/src/interfaces/iftablerw.rs b/routing/src/interfaces/iftablerw.rs index 29f231098e..df2bf78ea0 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() } @@ -217,3 +213,465 @@ impl IfTableReaderFactory { #[allow(unsafe_code)] unsafe impl Send for IfTableWriter {} + +/// Model-based properties over the interface table. +/// +/// The table is a map, but two things about it are not: an interface's *attachment* names a vrf, +/// which lives in a different structure and can be removed underneath it; and a reconfiguration has +/// to leave the runtime state -- addresses, attachment, operational state -- alone, since none of it +/// comes from the configuration that is being replaced. +#[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; + + /// Interface indices. `InterfaceIndex` is non-zero, so these start at one. + fn ifindexes() -> Vec { + (1..=u32::from(NUM_IFACES)) + .map(|i| InterfaceIndex::try_new(i).unwrap_or_else(|_| unreachable!())) + .collect() + } + + /// Non-default vrf ids. The default vrf is 0 and `VrfTable::new` makes it; it is left out so + /// that removing a vrf is always allowed. + 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] + } + + /// One change, over indices into the pools above. + #[derive(Debug, Clone)] + enum Change { + /// Add an interface. `renamed` picks between two names so that a modification is visible. + 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, + }, + } + + /// Draws sequences of [`Change`]s. + #[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 + } + + /// What the model believes about one interface. + #[derive(Debug, Clone, PartialEq)] + struct IfaceState { + name: String, + admin: IfState, + oper: IfState, + /// The vrf it is attached to, by index into [`vrf_ids`]. + 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(), + } + } + } + + /// The table, the vrfs it attaches to, and the reader that sees both. + 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), + } + } + + /// Add an interface, which is refused if one with that index is already there. + 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(), + }, + ); + } + + /// Reconfigure an interface. + /// + /// The configuration is replaced; the runtime state is not. Addresses, the vrf attachment and + /// the operational state are all learned out of band, and a reconfiguration knows nothing about + /// any of them. + 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; + } + + /// Attach an interface to a vrf. Both halves have to be there: the interface, and a vrf with a + /// fib whose id can be named. + 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); + } + } + + /// Detach every interface attached to `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; + } + } + } + + /// Remove a vrf, which detaches the interfaces that were attached to it. + 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]); + // an address for an interface we do not have is dropped, and the caller is not + // told: the error is logged inside `absorb_first` and goes no further + 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}") + } + } + + // and the vrf it names still exists. Nothing in the types ties an attachment to the + // life of the vrf it points at; `VrfTable::remove_vrf` detaching them is the whole + // of it + 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}" + ); + } + } + } + } + + /// The pools and the constants that index them agree. + #[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)); + // the default vrf is excluded, so every vrf in the pool can be removed + assert!(!vrf_ids().contains(&Vrf::DEFAULT_VRFID)); + assert_ne!(name_of(0, false), name_of(0, true)); + } + + /// After any sequence of changes, the interface table holds what the model says -- and no + /// interface is left attached to a vrf that has gone. + #[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:?}")); + } + }); + } +} diff --git a/routing/src/rib/nexthop.rs b/routing/src/rib/nexthop.rs index f4a52e0582..4196ffccc1 100644 --- a/routing/src/rib/nexthop.rs +++ b/routing/src/rib/nexthop.rs @@ -60,6 +60,12 @@ pub struct NhopKey { pub ifindex: Option, pub encap: Option, pub fwaction: FwAction, + /// The name of `ifindex`'s interface, for diagnostics only. + /// + /// Not populated when a next-hop is learned over the CPI, and it must not be: the name has to + /// be looked up against the interface table, which is populated out of band, so a key carrying + /// one would differ before and after we learn about the interface -- two next-hops where there + /// is one. See `RouteNhop::from_rpc_nhop`, and the same argument for a vxlan dmac above it. pub ifname: Option, } @@ -161,6 +167,20 @@ impl Hash for Nhop { } } +/// A next-hop's address in memory, used to tell next-hops apart in the visited sets that the +/// walks over the resolver graph keep. +/// +/// Address rather than key: keys are unique within one [`NhopStore`], but a next-hop may hold +/// resolvers that belong to another one, where an equal key would name a different object. A +/// resolution loop is a loop in the object graph, and next-hops live in `Rc`s, which do not move. +pub(crate) type NhopId = *const Nhop; + +/// The next-hops a walk over the resolver graph has already visited. +/// +/// A `Vec` rather than a set: resolver chains are a handful of next-hops long and fan out by two +/// or three, so a linear scan costs less than hashing. +pub(crate) type Visited = Vec; + impl Nhop { /// Create a new Nhop object from a key object fn from_key(key: &NhopKey) -> Self { @@ -184,23 +204,47 @@ 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. + /// This next-hop's identity for the visited sets of the walks over the resolver graph. + pub(crate) fn id(&self) -> NhopId { + std::ptr::from_ref(self) + } + + /// Tell if a next-hop resolves via another, `checked`: that is, whether `checked` is reachable + /// from `self` along resolver edges, `self` included. + /// + /// This is the guard against routing loops. `lazy_resolve` refuses an edge from `a` to `r` + /// exactly when `r.resolves_with(a)`, since such an edge would close a cycle, and a cycle in + /// the resolver graph would send the walks over it round for ever. + /// + /// The walk is total whether or not the graph already holds a cycle, because `visited` stops + /// it from entering any next-hop twice. That matters for two reasons: the guard should not + /// depend on the very invariant it exists to maintain, and a diamond-shaped graph is otherwise + /// re-walked once per path through it. fn resolves_with(&self, checked: &Nhop) -> bool { + self.resolves_with_rec(checked, &mut Visited::new()) + } + + fn resolves_with_rec(&self, checked: &Nhop, visited: &mut Visited) -> bool { // resolve to oneself is forbidden if self.key == checked.key { error!("Loop detected for next-hop {}!", self.key); return true; } + // a next-hop already visited leads nowhere new: either we are inside a cycle, or we + // reached it by another path and have already looked at everything beyond it. + // a next-hop already visited leads nowhere new: either we are inside a cycle, or we + // reached it by another path and have already looked at everything beyond it. + 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 @@ -264,8 +308,15 @@ impl Nhop { } /// Auxiliary recursive method used by `Nhop::quick_resolve()`. + /// + /// `visited` guards against a resolution loop, as in `resolves_with` above. #[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 +348,7 @@ impl Nhop { self.key.ifname.clone(), )); } else { - r.quick_resolve_rec(result); + r.quick_resolve_rec(result, visited); } } } @@ -311,7 +362,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,4 +1095,282 @@ mod tests { a.add_resolver(&checked); assert!(a.resolves_with(checked.as_ref())); } + + #[cfg_attr(not(emulated), traced_test)] + #[test] + /// Displaying a next-hop caught in a resolution loop terminates, and says which edge closes it. + /// + /// It used to walk the resolvers with an untracked `u8` depth, so a loop recursed until that + /// depth overflowed -- a panic raised from inside a `Display` impl that both the CLI and the + /// warning this module logs about resolution loops go through. + 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)] +mod fibgroup_properties { + use super::*; + use crate::fib::fibobjects::FibEntry; + use bolero::{Driver, ValueGenerator}; + use std::ops::Bound::Included; + + const MAX_NODES: u8 = 6; + const MAX_RESOLVERS: u8 = 2; + + /// A next-hop graph, given as an adjacency list over node indices. + /// + /// **Cycles included**, self-loops among them. A routing loop is exactly a cycle here, and + /// both walks over the resolver graph have to survive one -- `resolves_with` because it is the + /// guard that keeps cycles out and so cannot presume its own success, and + /// `build_nhop_fibgroup_rec` because nothing in the types ties it to that guard. + #[derive(Debug, Clone)] + struct Graph { + /// `edges[i]` are the nodes that `i` resolves via, in the order they were wired. + edges: Vec>, + /// Whether node `i` knows an interface, and so needs no resolving. + grounded: Vec, + } + + impl Graph { + /// Which nodes are reachable from `start`, itself included. A plain closure over the + /// adjacency list, computed without asking any next-hop anything. + fn reachable_from(&self, start: usize) -> Vec { + 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; + } + stack.extend_from_slice(&self.edges[node]); + } + seen + } + } + + /// Draws [`Graph`]s. + #[derive(Debug, Clone, Copy, Default)] + struct Graphs; + + impl ValueGenerator for Graphs { + type Output = Graph; + + fn generate(&self, driver: &mut D) -> Option { + let nodes = usize::from(driver.gen_u8(Included(&1), Included(&MAX_NODES))?); + let last = u8::try_from(nodes - 1).ok()?; + let mut edges = Vec::with_capacity(nodes); + let mut grounded = Vec::with_capacity(nodes); + 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 { + resolvers.push(usize::from(driver.gen_u8(Included(&0), Included(&last))?)); + } + edges.push(resolvers); + grounded.push(driver.produce::()?); + } + Some(Graph { edges, grounded }) + } + } + + // Build the graph in an `NhopStore`, returning the next-hops by index. + fn realize(graph: &Graph) -> (NhopStore, Vec>) { + let mut store = NhopStore::new(); + 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 graph.grounded[index] { + key.ifindex = Some( + InterfaceIndex::try_new(u32::from(raw) + 1) + .unwrap_or_else(|_| unreachable!()), + ); + } + store.add_nhop(&key) + }) + .collect(); + + for (from, resolvers) in graph.edges.iter().enumerate() { + for to in resolvers { + nodes[from].add_resolver(&nodes[*to]); + } + } + (store, nodes) + } + + // The oracle: every *simple* root-to-leaf path, its next-hops' instructions concatenated, + // squashed, and kept if the forwarder could execute it. + // + // Enumerated over the generated adjacency list rather than by walking the recursion under + // test. "Simple" is the loop guard restated: a path that would revisit a node stops there and + // contributes nothing, because going round a loop is not forwarding. + 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 mut entry = prefix.clone(); + entry.extend_from_slice(&nodes[from].instructions.borrow()); + + if graph.edges[from].is_empty() { + // A next-hop with neither an interface nor a way to reach one contributes nothing. + if !nodes[from].must_be_resolved() { + entry.squash(); + if entry.is_valid() { + out.push(entry); + } + } + } else { + for to in &graph.edges[from] { + expected(graph, nodes, *to, path, &entry, out); + } + } + + path.pop(); + } + + /// A next-hop's fib group is one entry per usable resolution path, and never empty. + #[test] + fn a_fibgroup_is_the_usable_paths_through_the_graph() { + let rstore = RmacStore::new(); + bolero::check!() + .with_generator(Graphs) + .cloned() + .for_each(|graph: Graph| { + let (_store, nodes) = realize(&graph); + for node in &nodes { + node.build_nhop_instructions(&rstore); + } + + let mut want = Vec::new(); + expected( + &graph, + &nodes, + 0, + &mut Vec::new(), + &FibEntry::new(), + &mut want, + ); + if want.is_empty() { + // Nothing usable: the group carries a drop so packets are not misrouted. + want.push(FibEntry::drop_fibentry()); + } + + let got = nodes[0].build_nhop_fibgroup(); + assert_eq!(got.entries(), &want, "for {graph:?}"); + }); + } + + /// Every entry a fib group offers is one the forwarder can execute. + /// + /// `FibEntry::is_valid` is the written-down form of that, and `rib2fib` filters on it -- but + /// the drop injected when nothing is usable bypasses the filter, so it is worth asserting over + /// the group rather than trusting the one call site. + #[test] + fn every_entry_in_a_fibgroup_is_usable() { + let rstore = RmacStore::new(); + bolero::check!() + .with_generator(Graphs) + .cloned() + .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 {graph:?}"); + for entry in group.iter() { + assert!(entry.is_valid(), "unusable entry {entry:?} for {graph:?}"); + } + }); + } + + /// A next-hop every one of whose paths loops back gets a drop, not an infinite walk. + /// + /// The general case falls out of the two properties above -- they only terminate because the + /// walk does -- but a routing loop is the failure this guard exists for, so it is worth one + /// case that says so in as many words. + #[test] + fn a_next_hop_in_a_resolution_loop_drops() { + let rstore = RmacStore::new(); + let mut store = NhopStore::new(); + + // 7.0.0.1 -> 8.0.0.2 -> 9.0.0.3 -> 7.0.0.1, and no way out to an interface. + 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" + ); + } + + /// `resolves_with` answers reachability in the resolver graph. + /// + /// That is the whole of what the loop guard rests on: `lazy_resolve` refuses an edge from `a` + /// to `r` exactly when `r.resolves_with(a)`, which is to say when `a` is already reachable + /// from `r` and the edge would close a cycle. Checked against a closure computed over the + /// adjacency list, which asks no next-hop anything -- and, now that the graphs may contain + /// cycles, over graphs where `resolves_with` has to terminate on its own account. + #[test] + fn resolves_with_answers_reachability() { + bolero::check!() + .with_generator(Graphs) + .cloned() + .for_each(|graph: Graph| { + let (_store, nodes) = realize(&graph); + for (from, node) in nodes.iter().enumerate() { + let reachable = graph.reachable_from(from); + for (to, other) in nodes.iter().enumerate() { + assert_eq!( + node.resolves_with(other), + reachable[to], + "{from} -> {to}, for {graph:?}" + ); + } + } + }); + } + + /// A next-hop always resolves via itself, which is what makes the guard refuse a self-loop. + #[test] + fn a_next_hop_resolves_via_itself() { + bolero::check!() + .with_generator(Graphs) + .cloned() + .for_each(|graph: Graph| { + let (_store, nodes) = realize(&graph); + for node in &nodes { + assert!(node.resolves_with(node)); + } + }); + } } diff --git a/routing/src/rib/rib2fib.rs b/routing/src/rib/rib2fib.rs index 99ae73cd0f..738c921062 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; @@ -103,8 +103,44 @@ impl Nhop { ////////////////////////////////////////////////////////////////////// /// Recursive helper to build [`FibGroup`] for a next-hop. We accumulate /// a next-hop's packet instructions with those of its resolvers. + /// + /// `path` holds the next-hops between the root of the walk and this one. A next-hop that turns + /// up on its own resolution path closes a routing loop: following it would recurse until the + /// stack ran out, so we stop there and contribute nothing. If that leaves no usable path at + /// all, `build_nhop_fibgroup` injects a drop, which is what a packet caught in a routing loop + /// should meet anyway. + /// + /// This makes the walk safe on any graph rather than only on the acyclic ones that + /// `Nhop::resolves_with` lets `lazy_resolve` build. The two guards are deliberately + /// independent: nothing in the types ties this recursion to the one that used to be its only + /// protection, and a caller wiring resolvers by another route would lose it silently. ////////////////////////////////////////////////////////////////////// - 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(); + } + + ////////////////////////////////////////////////////////////////////// + /// The body of [`Nhop::build_nhop_fibgroup_rec`], for a next-hop known not to be on its own + /// resolution path already. Split out so that the push and the pop of `path` sit next to each + /// other and no early return here can leave the path unbalanced. + ////////////////////////////////////////////////////////////////////// + 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 +172,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 +185,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()); diff --git a/routing/src/rib/vrf.rs b/routing/src/rib/vrf.rs index 6739a9d915..8abb46186d 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; @@ -113,7 +114,7 @@ impl ShimNhop { } } -#[derive(Copy, Clone, PartialEq)] +#[derive(Copy, Clone, Debug, PartialEq)] #[allow(unused)] pub enum VrfStatus { Active, @@ -392,6 +393,27 @@ impl Vrf { } } + ///////////////////////////////////////////////////////////////////////// + /// The next-hops to install for a route, substituting an explicit drop if there are none. + /// + /// A route with no next-hops cannot forward anything, and leaving it out of the table + /// altogether is worse than installing it as a drop: the rib would keep it while the fib + /// declined it -- `FibWriter::add_fibroute` refuses a route with no next-hop keys -- so a + /// packet matching it would fall through to a shorter prefix in the fib and be forwarded + /// somewhere else rather than dropped. Resolving via a default is how a routing loop starts. + /// + /// `add_route_rpc` used to do this, for the one path that could produce an empty list. Doing + /// it here means no caller can skip it. + ///////////////////////////////////////////////////////////////////////// + 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 +425,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 +486,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 +1162,401 @@ pub mod tests { } } + +/// Model-based properties over a [`Vrf`]'s route table. +/// +/// Two structures move together on every route change: the route tries, and the [`NhopStore`] whose +/// entries the routes hold `Rc`s into. A route that comes or goes has to leave the store holding +/// exactly the next-hops the remaining routes name -- one too many is a leak that keeps a stale +/// fib group alive, one too few and a route names a next-hop nothing will resolve. A third, the +/// vrf's `Fib`, is written through on the same calls and has to end up describing the same +/// prefixes. +#[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 NUM_STATUSES: u8 = 3; + + /// `0.0.0.0/0` and `::/0`, at these indices in [`prefixes`]. A vrf always carries a route for + /// both: `Vrf::lpm` has no answer otherwise and says so with an `unreachable!()`, and + /// `check_deletion` reaches for them by name. + const ROOT_V4: usize = 0; + const ROOT_V6: usize = 5; + + /// The prefixes a generated vrf may carry routes for. Nested, and both roots. + 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() + } + + /// Addresses to look up: inside each level of the nesting, and outside all of them. + 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 statuses() -> Vec { + vec![VrfStatus::Active, VrfStatus::Deleting, VrfStatus::Deleted] + } + + /// The next-hops a generated route may use. Small, so that routes share them: sharing is what + /// makes the store's reference counting do any work. + 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), + ] + } + + /// One change, over indices into the pools above. + #[derive(Debug, Clone)] + enum Change { + /// A route. `nhops` may be empty: `add_route_complete` takes whatever the control plane + /// hands it, and a route with nowhere to go is the interesting end of that. + AddRoute { + prefix: usize, + nhops: Vec, + }, + DelRoute { + prefix: usize, + }, + SetStale { + value: bool, + }, + RemoveStale, + /// Move the vrf's status. Worth generating because `del_route` calls `check_deletion`, + /// whose whole body sits behind `status == Deleting` -- so without this, the transition + /// that `VrfTable::remove_deleting_vrfs` depends on never runs at all. + SetStatus { + status: usize, + }, + } + + /// Draws sequences of [`Change`]s. + #[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(&4))? { + 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::()?, + }, + 3 => Change::RemoveStale, + _ => Change::SetStatus { + status: index(driver, NUM_STATUSES)?, + }, + }; + out.push(change); + } + Some(out) + } + } + + /// Which prefixes carry routes, and for each the next-hops it names and whether it is stale. + /// + /// The two root routes are the preset drop routes a vrf is born with; they are never removed, + /// only reset, and `set_stale` skips them. + /// What the model believes about one route. + #[derive(Debug, Clone, PartialEq)] + struct RouteState { + /// The next-hop keys it names, in order. Keys rather than pool indices, because the preset + /// drop route names one the generator cannot ask for. + nhops: Vec, + stale: bool, + /// Whether this is still the drop route a vrf installs for each root on creation. + /// + /// Distinct from "names the drop next-hop": a generated route for a root prefix with no + /// next-hops ends up naming it too, but carries a real origin, distance and metric, so + /// `Route::is_preset_drop_route` says no -- and `check_deletion` asks that question. + preset: bool, + } + + #[derive(Debug, Clone)] + struct Model { + routes: BTreeMap, + status: VrfStatus, + } + + impl Model { + /// The route a vrf installs for each root on creation: drop, so that a lookup always has an + /// answer. + fn preset() -> RouteState { + RouteState { + nhops: vec![NhopKey::with_drop()], + stale: false, + preset: true, + } + } + + fn new() -> Self { + Self { + routes: BTreeMap::from([(ROOT_V4, Self::preset()), (ROOT_V6, Self::preset())]), + status: VrfStatus::Active, + } + } + + fn is_root(prefix: usize) -> bool { + prefix == ROOT_V4 || prefix == ROOT_V6 + } + + /// Every next-hop key the routes name -- the set the store should hold, no more and no less. + fn referenced(&self) -> BTreeSet { + self.routes + .values() + .flat_map(|route| route.nhops.iter().cloned()) + .collect() + } + + /// `Vrf::check_deletion`, which `del_route` calls: a vrf on its way out becomes deletable + /// once the only routes left are the two preset drop ones. + 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; + } + } + + /// The longest prefix carrying a route that covers `addr`. + 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 } => { + // a route with no next-hops is installed as a drop rather than left out + let nhops = if nhops.is_empty() { + vec![NhopKey::with_drop()] + } else { + nhops.iter().map(|i| pool[*i].key.clone()).collect() + }; + self.routes.insert( + *prefix, + RouteState { + nhops, + stale: false, + preset: false, + }, + ); + } + Change::DelRoute { prefix } => { + if Self::is_root(*prefix) { + // a root route is reset to the preset drop route, not removed + self.routes.insert(*prefix, Self::preset()); + } else { + self.routes.remove(prefix); + } + self.check_deletion(); + } + Change::SetStale { value } => { + // `Vrf::set_stale` skips a route whose prefix is a root, and separately one + // that is still a preset drop route. Generated routes are never the latter, so + // here the root test is the whole of it. + for (prefix, route) in &mut self.routes { + if !Self::is_root(*prefix) { + route.stale = *value; + } + } + } + Change::RemoveStale => { + let stale: Vec = self + .routes + .iter() + .filter_map(|(prefix, route)| route.stale.then_some(*prefix)) + .collect(); + for prefix in stale { + self.apply(&Change::DelRoute { prefix }, pool); + } + } + Change::SetStatus { status } => { + // the vrf under test is not the default one, so the move always takes + self.status = statuses()[*status]; + } + } + } + } + + 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), + Change::SetStatus { status } => vrf.set_status(statuses()[*status]), + } + } + + /// Check the vrf, its next-hop store and its fib against the one model. + fn check(vrf: &Vrf, model: &Model, at: &str) { + let prefixes = prefixes(); + let probes = probes(); + + // 1. the route tries hold exactly the prefixes the model says + 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}" + ); + + // 2. each route names the next-hops the model says, in order + 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, 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}" + ); + } + + // the status, and with it `check_deletion`: a vrf on its way out becomes deletable exactly + // when the only routes left are the two preset drop ones + assert_eq!(vrf.status, model.status, "status {at}"); + + // 3. the next-hop store holds exactly the next-hops the routes name. One too many is a + // leak that keeps a stale fib group alive; one too few and a route names something + // nothing will resolve + let stored: BTreeSet = vrf.nhstore.iter().map(|rc| rc.key.clone()).collect(); + assert_eq!(stored, model.referenced(), "next-hop store {at}"); + + // 4. lpm resolves for every address, and lands where the model says + 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}"); + } + + // 5. the fib describes the same prefixes as the vrf. Otherwise a packet matching a route + // the fib never heard about falls through to a shorter prefix and is forwarded + // somewhere else rather than dropped + 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}"); + } + + /// The pools and the constants that index them agree. + #[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!(statuses().len(), usize::from(NUM_STATUSES)); + assert_eq!(prefixes()[ROOT_V4], Prefix::root_v4()); + assert_eq!(prefixes()[ROOT_V6], Prefix::root_v6()); + } + + /// After any sequence of route changes, the vrf, its next-hop store and its fib agree. + #[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/rib/vrftable.rs b/routing/src/rib/vrftable.rs index d3035239ec..b1110fdecf 100644 --- a/routing/src/rib/vrftable.rs +++ b/routing/src/rib/vrftable.rs @@ -176,6 +176,17 @@ impl VrfTable { vrfid: VrfId, iftablew: &mut IfTableWriter, ) -> Result<(), RouterError> { + // The default vrf is not removable. `Vrf::set_status` says as much and keeps the default + // vrf `Active` so that the sweeps below never pick it up, but nothing stopped a caller + // naming it here -- and `get_default_vrf` treats the default vrf's existence as given, + // with an `unreachable!()` rather than an error. + 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 { @@ -189,7 +200,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(); } @@ -937,3 +948,787 @@ mod tests { test_vrf_fibgroup(build_test_vrf_nhops_partially_resolved()); } } + +/// Model-based properties over a [`VrfTable`]. +/// +/// The vrf table is where four key spaces have to agree: `by_id`, `by_vni`, each [`Vrf`]'s own +/// `vni` field, and the fib table's two -- `FibKey::Id` and the `FibKey::Vni` alias. Nothing holds +/// them together but the table's own methods doing the right number of things in the right order, +/// so what is generated here is sequences of those methods, and what is checked is that all four +/// still describe the same set of vrfs afterwards. +#[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; + + /// Vrf ids a generated table may hold, the default among them: `remove_vrf` takes any id, and + /// whether it should take that one is exactly the question worth generating for. + 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] + } + + /// One change, as [`VrfTable`] exposes them, over indices into the pools above. + #[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, + } + + /// Draws sequences of [`Change`]s. + #[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 => { + // `NUM_VNIS` means "created without a vni", which is the ordinary case. + // One draw rather than an `Option>`, which cannot be told apart + // from the driver running out of input. + 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) + } + } + + /// Which vrfs exist, and for each the vni it carries and the status it is in. + /// + /// One map, from which all four of the table's views are derivable -- which is the point. The + /// table keeps them as four separate structures updated by hand; if any method updates three + /// of them, this says which one it missed. + 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); + // refused if the id is taken, or if the vni is + 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) { + // another vrf holds it: refused. The same vrf already holds it: nothing to do + Some(_) => (), + // otherwise the vrf drops whatever vni it had and takes this one -- but only + // if it exists at all + 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); + // the default vrf is refused + 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); + } + // the default vrf's status is fixed: it is what keeps the sweeps below from + // deleting it + 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); + } + } + } + + /// Check every view of the table against the one model, and against each other. + fn check(table: &VrfTable, model: &Model, at: &str) { + let ids = vrf_ids(); + let all_vnis = vnis(); + + // 1. by_id holds exactly the vrfs the model says, each carrying what the model says + 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}"); + } + + // 2. by_vni is exactly the inverse of the vnis the vrfs carry -- no stale entry left by a + // removal, and none missing after a vni was set + 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}" + ); + } + + // 3. the fib table holds a fib for every vrf, under its id and under its vni if it has + // one, and every one of those keys reaches a live fib with the right identity + 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); + + // 4. the default vrf is always there and always active. `get_default_vrf` treats that as + // given, with an `unreachable!()` rather than an error + assert!(table.contains(Vrf::DEFAULT_VRFID), "no default vrf {at}"); + assert_eq!( + table.get_default_vrf().status, + VrfStatus::Active, + "default vrf not active {at}" + ); + + // 5. the table's own consistency check agrees. `check_vni` is the in-tree half of this + // oracle; it should pass for a vrf with a vni and fail for one without + 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}" + ); + } + } + + /// The pools and the constants that index them agree. + #[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); + } + + /// After any sequence of changes, every view of the vrf table still describes the same vrfs. + #[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:?}")); + } + }); + } +} + +/// Properties over resolution **across** vrfs. +/// +/// `VrfTable::refresh_non_default_fibs` and `refresh_fibs_by_vni` hand the default vrf to every +/// other vrf as its resolution vrf, which is how a next-hop in an overlay vrf reaches an interface +/// the underlay knows about. Nothing exercised that path: both functions, `set_stale`, +/// `remove_stale_routes` and `values_mut_except_default` were at zero coverage, and every generated +/// next-hop graph so far has lived inside a single vrf. +#[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; // non-default vrfs + 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; + + /// Non-default vrf ids. The default vrf is 0 and `VrfTable::new` makes it. + 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() + } + + /// Prefixes the default vrf carries routes for. Nested, so a longest match has to be chosen. + 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() + } + + /// Prefixes the other vrfs carry routes for. + 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() + } + + /// On-link addresses a directly connected underlay next-hop may carry, one per interface. + /// + /// Whether the resolving next-hop has an address of its own is what decides which address ends + /// up in the fib, and `rib2fib` is explicit that the deeper one wins: the next-hop being + /// resolved supplies the layer-2 target "unless a next-hop deeper in the resolution chain + /// provides one of its own". Generating both cases is what lets this property tell + /// `EgressObject::merge`'s rule -- first ifindex, last address -- from its inverse. + fn onlink_addrs() -> Vec { + (1..=NUM_IFINDEXES) + .map(|i| mk_addr(&format!("7.200.0.{i}"))) + .collect() + } + + /// Addresses an overlay route may point via. `8.0.0.1` is covered by no underlay prefix, so it + /// reaches only the default vrf's root -- whose preset next-hop is a drop. + fn vias() -> Vec { + ["7.0.0.1", "7.1.0.1", "8.0.0.1"] + .iter() + .map(|a| mk_addr(a)) + .collect() + } + + /// A generated arrangement of vrfs and routes. + #[derive(Debug, Clone)] + struct Topology { + /// Whether each non-default vrf carries a vni. Vrf `i` gets vni `i`, so two vrfs can never + /// ask for the same one -- `add_vrf` rightly refuses that, and it is the `VrfTable` + /// property's business rather than this one's. + vrfs: Vec, + /// Routes in the default vrf, as (underlay prefix index, ifindex index, on-link). Directly + /// connected: the next-hop carries an interface, and an address of its own when `on-link`. + underlay: Vec<(usize, usize, bool)>, + /// Routes in the other vrfs, as (vrf index, overlay prefix index, via index). Recursive: + /// the next-hop carries an address and no interface, so it has to be resolved. + overlay: Vec<(usize, usize, usize)>, + /// One more default-vrf route, applied after the first refresh, so that a later refresh has + /// something to notice. + later: Option<(usize, usize, bool)>, + /// Vnis to pass to `refresh_fibs_by_vni`. + selected: Vec, + } + + /// Draws [`Topology`]s. + #[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, + }) + } + } + + /// The default vrf's routes, as the model sees them: prefix index -> (ifindex index, on-link). + /// A later route for a prefix replaces an earlier one, as the trie does. + type Underlay = BTreeMap; + + /// The other vrfs' routes: (vrf index, overlay prefix index) -> via index. A map for the same + /// reason: the generator may name one prefix twice, and only the last route survives. + type Overlay = BTreeMap<(usize, usize), usize>; + + /// The interface the default vrf leads to for `via`, or `None` if `via` reaches only the root -- + /// whose preset next-hop is a drop, so nothing is reachable through it. + /// + /// A longest-prefix match over the generated route list, asking no next-hop anything. + 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)) + } + + /// What the fib should hold for an overlay route pointing via `via`. + /// + /// One entry, and the shape of it is the whole point of resolving recursively: the interface + /// comes from the *resolver* in the default vrf, and the address stays that of the next-hop that + /// needed resolving. `EgressObject::merge` keeping the first ifindex and the last address is + /// what produces that, and `rib2fib` explains why it must -- otherwise the egress stage would + /// resolve the packet's own destination at layer 2, which is only right when it is on-link. + fn expected_entry(model: &Underlay, via: usize) -> FibEntry { + match resolves_to(model, via) { + // the interface comes from the resolver; the address is the resolver's own if it has + // one, and otherwise stays that of the next-hop that needed resolving + 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, + )], + ) + } + + /// Build the table described by `topology`, without refreshing anything yet. + 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); + // deliberately inserted with no resolution vrf, so the route starts resolved against + // its own vrf -- where the address reaches nothing. Only the table-level refresh can + // put it right, which is what makes the refresh load-bearing here. + 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) + } + + /// The fib entries a vrf offers for one overlay prefix, or `None` if it has no such route. + 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() + }) + } + + /// Every entry in every fib of every vrf is one the forwarder can execute. + /// + /// This is the rib-to-fib contract: `FibEntry::is_valid` is the written-down half of it, and + /// `rib2fib` filters on it -- but the drop injected for an empty group bypasses that filter, and + /// nothing checked the whole table at once. + 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 + ); + } + } + } + } + } + + /// The pools and the constants that index them agree. + #[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)); + // vrf `i` takes vni `i`, so there must be at least as many vnis as vrfs + const { assert!(NUM_VNIS >= NUM_VRFS) }; + // the last via must be reachable through no underlay prefix, so that the unresolvable case + // is generated + 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()); + } + + /// A refresh resolves every other vrf's next-hops through the default vrf. + /// + /// The interface has to come from the default vrf's route, and the address has to stay that of + /// the next-hop being resolved. + #[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:?}" + ); + } + }); + } + + /// `refresh_fibs_by_vni` refreshes the vrfs whose vni is named, and leaves the rest alone. + #[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(); + + // what each overlay route offers before anything changes + let before: BTreeMap<(usize, usize), Option>> = routes + .keys() + .map(|(vrf, prefix)| { + ( + (*vrf, *prefix), + fib_entries(&table, ids[*vrf], overlay()[*prefix]), + ) + }) + .collect(); + + // change the underlay, then refresh only the named vnis + 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:?}" + ); + } + } + }); + } + + /// Marking everything stale and sweeping leaves every vrf with only its preset drop routes. + /// + /// The default vrf is swept separately from the rest, since it is the resolution vrf for them. + #[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"); + }); + } +} diff --git a/routing/src/router/cpi.rs b/routing/src/router/cpi.rs index 68d9d0a220..ddec4dc920 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 } @@ -498,3 +497,390 @@ pub fn process_cpi_data(rio: &mut Rio, peer: &SocketAddr, data: &mut Bytes, db: } } } + +/// Properties over the control-plane operations, driven through a whole [`RoutingDb`]. +/// +/// This is the layer above `rpc_adapt`: it picks the vrf, decides what a lookup failure means, and +/// chains an operation's effects into the rest of the database -- an rmac arriving refreshes the +/// fibs of the vrfs on its vni, and deleting a route can delete the vrf with it. Almost none of it +/// was covered. +#[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; + + /// The vrf the overlay routes live in, and the vni it is reachable by. + const OVERLAY_VRF: VrfId = 7; + const OVERLAY_VNI: u32 = 3000; + /// The interface the underlay route egresses on. + 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!()) + } + + /// Remote vtep addresses an overlay next-hop may point at. Both are covered by the underlay + /// route installed below, so reachability is never the reason a route fails to resolve. + 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], + ] + } + + /// A database with an interface table, an underlay route in the default vrf towards the vteps, + /// and one overlay vrf on [`OVERLAY_VNI`]. + 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); + + // the underlay: 7.0.0.0/8 out of an interface, so a vtep address resolves + 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 + } + + /// An overlay route: prefix reachable by vxlan to `vtep` on [`OVERLAY_VNI`]. + 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, + } + } + + /// The entries a vrf's fib offers for one prefix. + 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() + } + + /// Every entry is one the forwarder can execute, and any `Drop` in it comes before anything + /// that would act on the packet. + /// + /// The second half is why an entry like `[Drop, Egress]` -- which resolution does produce, for + /// an encapsulation that could not be completed -- is safe: the forwarder stops at the first + /// instruction that finishes the packet, so a `Drop` reached first means nothing after it runs. + 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}"); + } + } + } + + /// Draws a `(vtep, mac)` pair. + #[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))) + } + } + + /// The pools and the constants that index them agree. + #[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)); + // every vtep must be reachable through the underlay route, so a failure to resolve is only + // ever about the router mac + 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"); + } + } + + /// An overlay route drops until the router mac for its vtep arrives, and encapsulates after. + /// + /// This is the evpn data path end to end: the route resolves through the underlay either way, + /// but the encapsulation cannot be completed without a router mac, so until one arrives the + /// only safe thing is to drop. When `Rmac::add` stores one it refreshes the fibs on that vni, + /// which is what turns the entry into an encapsulate-then-egress. + #[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 + ); + + // no router mac yet: the encapsulation cannot be completed + 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:?}"); + } + }, + ); + } + + /// Withdrawing a router mac does not stop traffic. + /// + /// `Rmac::del` marks the entry stale rather than removing it, and `VxlanEncapsulation::resolve` + /// accepts a stale one -- "ok if we found a mac, even if the entry is stale". Forwarding to a + /// mac that may have moved beats blackholing the vni while the control plane catches up. + #[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); + // the fib is only rebuilt on the next refresh, so ask for one + 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"); + }, + ); + } + + /// A route for a vrf we do not have fails on add, and succeeds on delete until we have a config. + /// + /// The asymmetry is deliberate: a delete for a vrf we never had is a route we do not have + /// either, so reporting failure would leave frr retrying something already true. Once a config + /// has been applied there is no such excuse, and the same lookup is a real failure. + #[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" + ); + } + + /// Deleting the last route of a vrf on its way out takes the vrf with it. + #[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); + + // while the vrf is active, deleting its routes leaves it in place + 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); + + // now the same delete empties it, which makes it deletable, which removes it + assert_eq!(route.del(&mut db), RpcResultCode::Ok); + assert!( + !db.vrftable.contains(OVERLAY_VRF), + "a vrf that became deletable was left behind" + ); + } + + /// An interface address is refused unless both the mask and the interface index are usable. + #[test] + fn an_interface_address_is_refused_unless_it_is_usable() { + let cases = [ + // (ifindex, mask, expected) + (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"); + } + } + + /// `nonlocal_nhop` spots a route whose next-hops live in another vrf. + #[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"); + } +} diff --git a/routing/src/router/rpc_adapt.rs b/routing/src/router/rpc_adapt.rs index ff2e39bbc9..b735b27522 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,21 @@ 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 + // build key for this next hop. + // + // No interface name: it would have to be looked up from `ifindex` against the interface + // table, which is populated out of band, so the same next-hop off the wire would key + // differently before and after we learn about the interface -- two `Nhop`s, two fib groups, + // for one next-hop. This is the same reasoning that keeps a vxlan dmac out of the key, + // written down above: a next-hop key has to be immutable for keying purposes, so nothing + // derived from mutable state outside it belongs in one. let key = NhopKey::new( origin, nh.address, ifindex, encap, FwAction::from(nh.fwaction), - ifname, + None, ); // validate next hop from its key @@ -180,13 +174,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,18 +204,18 @@ 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}"), } } - // 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 no next-hop was received with the route, or none of them could be processed, the + // route is still installed -- with an action drop, which `Vrf::nhops_or_drop` substitutes. + // Not installing it would break consistency (e.g. resolving via a default) and cause a + // loop. Warn here rather than there, since only this layer can tell the two cases apart. 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 @@ -245,3 +233,387 @@ impl Vrf { self.del_route(prefix, vrf0, rstore); } } + +/// Properties over the translation from control-plane messages into routing state. +/// +/// This is where the routing stack parses input it does not control: `IpRoute`s and their next-hops +/// arrive from FRR over the CPI, and everything downstream is built from whatever this module makes +/// of them. It had no test coverage at all. +/// +/// The oracle throughout is the wire message: what the key should hold, and which next-hops should +/// be refused, worked out from the fields rather than by rerunning the conversion. +#[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; + + /// The prefix of the last entry in [`prefixes`] cannot be built, so the "message names a prefix + /// we cannot parse" path is generated. + const BAD_PREFIX: usize = 4; + + /// `(address, mask length)` pairs as they arrive on the wire, including one that is not a + /// prefix at all. + 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, + ), + // a host prefix, which turns a connected route into a local one + ( + IpAddr::from_str("10.1.2.3").unwrap_or_else(|_| unreachable!()), + 32, + ), + ( + IpAddr::from_str("2001:db8::").unwrap_or_else(|_| unreachable!()), + 32, + ), + // 33 bits of an ipv4 address: no such prefix + ( + IpAddr::from_str("10.0.0.0").unwrap_or_else(|_| unreachable!()), + 33, + ), + ] + } + + /// Next-hop addresses. `None` is on the wire too, for an interface-only next-hop or a drop. + 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!())), + ] + } + + /// Next-hop interface indices, as raw wire values: absent, the invalid zero, one the interface + /// table knows, and one it does not. + fn ifindexes() -> Vec> { + vec![None, Some(0), Some(2), Some(99)] + } + + /// Encapsulation vnis: absent, the invalid zero, and a usable one. + 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, + ] + } + + /// One next-hop as it arrives, over indices into the pools above. + #[derive(Debug, Clone)] + struct NhopSpec { + drop: bool, + address: usize, + ifindex: usize, + vni: usize, + vrfid: VrfId, + } + + /// One route as it arrives. + #[derive(Debug, Clone)] + struct RouteSpec { + prefix: usize, + rtype: usize, + distance: u8, + metric: u32, + nhops: Vec, + } + + /// Draws [`RouteSpec`]s. + #[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::()?; + // deliberately able to draw none: a route with no next-hops is on the wire, and the + // comment in `add_route_rpc` is about exactly that + 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(), + } + } + + /// The origin the route should be recorded with. + /// + /// The pairs are written out rather than deferred to `From`, so that a wrong pairing + /// is visible. The one rule that is not a pairing: a *connected* route to a single host is the + /// address of one of our own interfaces, so it is recorded as `Local` -- which is what makes + /// `build_pkt_instructions` emit a local-delivery instruction instead of an egress. + 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, + } + } + + /// The key the next-hop should produce, or `None` if it should be refused. + /// + /// Four reasons to refuse, each worked out from the wire fields: interface index zero, a vni + /// that is not one, a vxlan next-hop that does not say which vtep to send to, and a forwarding + /// next-hop with neither an interface nor an address -- which is nowhere to send anything. + 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, + })), + }; + + // an encapsulated next-hop is reached by the underlay, so whatever interface the message + // named for it is ignored + 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; + } + + // no interface name: the conversion has no interface table to look one up in, which is + // what keeps one wire next-hop from keying two ways + 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 + } + + /// The pools and the constants that index them agree, and each refusal is reachable. + #[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" + ); + // the delete property relies on no pool prefix being a root, since a root route is reset + // rather than removed + for (address, len) in prefixes() { + assert!(Prefix::try_from((address, len)).is_ok_and(|p| !p.is_root()) || len > 32); + } + } + + /// A next-hop off the wire is refused for exactly the reasons it should be, and otherwise + /// yields the key the message describes. + #[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"), + } + } + } + }); + } + + /// A route off the wire is installed as the message describes, with the next-hops that survived + /// translation -- or a drop next-hop if none did. + #[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 { + // a prefix we cannot parse installs nothing: only the two preset root routes + 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() { + // nothing usable: the route is still installed, as a drop + 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:?}"); + }); + } + + /// Deleting the route the message names removes it; a prefix we cannot parse removes nothing. + #[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:?}" + ); + } + // no pool prefix is a root, so nothing but the two preset root routes is left + assert_eq!(vrf.len_v4() + vrf.len_v6(), 2, "for {spec:?}"); + // and the next-hop store is left holding only what the root routes name + let keys: Vec = vrf.nhstore.iter().map(|rc| rc.key.clone()).collect(); + assert_eq!( + keys, + vec![NhopKey::with_drop()], + "leftover next-hops for {spec:?}" + ); + }); + } +}