diff --git a/Cargo.toml b/Cargo.toml index 95815a7..bdd3928 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "routecore" -version = "0.5.0-rc1-dev" +version = "0.5.0-rc1" authors = ["NLnet Labs "] categories = ["network-programming"] description = "A Library with Building Blocks for BGP Routing" diff --git a/Changelog.md b/Changelog.md index 7c86d51..c9aeb06 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,17 +1,104 @@ -## 0.5.0-rc0 +## 0.5.0-rc1 -Released 2024-05-29. +Released 2024-06-10. + +This release features a lot of changes, big and small. The list below is not +exhaustive but tries to highlight and describe the bigger (and perhaps, more +important) ones. -TODO Breaking changes +* Several common basic types (e.g. `Asn` and `Prefix`) are moved out of + _routecore_ to the new _inetnum_ crate. + +* Refactor of PathAttributes + + The introduction of `PaMap` and `RouteWorkshop` (see below) required refactoring + of the `PathAttributes` enum and related types. Most significantly, an entire + layer in the enum was removed, changing how one matches on variants. + Furthermore some of the types in those variants slightly changed. + +* Overhaul of address family related code + + The types describing address families, and related traits to parse and compose + NLRI of such families, have severely changed. This eliminates any possible + ambiguity regarding the address family of announcements/withdrawals in UPDATE + messages at compile time. + + * Supported address families are now explicitly and exhaustively enumerated + in the `AfiSafiType` enum. As a SAFI by itself does not carry much meaning, + the separate `SAFI` enum is removed. Almost all of the code now works + based on the more descriptive `AfiSafiType`. + * ADD-PATH is now supported on all supported address families, i.e. the + `AfiSafiType` includes `~AddPath` variants for every single AFI+SAFI + combination. + * This now allows for e.g. efficient iterators over NLRI that are generic + over the `AfiSafiType`. + But, as a possible downside, this moves the task of determining what + address family a set of NLRI holds to the caller in order to then use the + correctly typed iterator. The less efficient, 'easier to use' iterators + returning an enum instead of a distinct NLRI type are therefore still + available. + New -Bug fixes +* RouteWorkshop / PaMap + + This release introduces the `RouteWorkshop` to create UPDATE messages based on + an NLRI and a set of attributes. For creation, inspection and manipulation of + those attributes, the `PaMap` is introduced. These new types work in + conjunction with the existing `UpdateBuilder`. + +* BGP FSM (absorbed from _rotonda-fsm_) + + _routecore_ now contains the code to enable for actual BGP sessions, i.e. the + BGP FSM and related machinery. By pulling this in into _routecore_ allows for + less dependency juggling, easier development iterations and more sensible code + in all parts. All of this has some rough edges and is prone to changes on the + near future. + + The _rotonda-fsm_ crate for now is left as-is. + + +* Route Selection fundamentals + + This release introduces a first attempt at providing handles to perform the + BGP Decision Process as described in RFC4271, colloquially known as 'route + selection' or 'best path selection'. + + Most of the heavy-lifting for this comes from implementing `Ord` on a wrapper + struct holding a 'route' (i.e., `PaMap`) and additional information to allow + tie-breaking as described in the RFC. + + As the tie-breaking in RFC4271 is actually broken and not totally ordered, we + aim to provide a certain degree of flexibility in the tie-breaking process by + means of different `OrdStrat` ordering strategies. + Other changes +* Feature flags + + After splitting of parts of _routecore_ into the _inetnum_ crate, the default + features set resulted in an almost empty library. Therefore the `bgp` flag is + now on by default, and we introduced an `fsm` flag to enable the BGP FSM code + absorbed from _rotonda-fsm_. + + +Known limitations + +* Constructed UPDATE messages are MultiProtocol-only + + With regards to announcing and withdrawing NLRI, the `UpdateBuilder` is currently + limited to putting everything in the MultiProtocol path attributes + (MP_REACH_NLRI, MP_UNREACH_NLRI), so even for IPv4 Unicast. + + Note that this behaviour is considered preferable as it leads to somewhat more + flexibility/resilience on the protocol level. But in case one of the peers + does not signal the capability of doing IPv4 Unicast in MultiProtocol + attributes, we should allow creation of PDUs in the traditional form anyway, + so we plan to reintroduce this functionality. ## 0.4.0 diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index 3735296..54f7c30 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -181,7 +181,7 @@ dependencies = [ [[package]] name = "routecore" -version = "0.4.1-dev" +version = "0.5.0-rc1" dependencies = [ "arbitrary", "bytes", diff --git a/fuzz/fuzz_targets/path_selection.rs b/fuzz/fuzz_targets/path_selection.rs index c2b1490..2e2cd84 100644 --- a/fuzz/fuzz_targets/path_selection.rs +++ b/fuzz/fuzz_targets/path_selection.rs @@ -1,8 +1,14 @@ #![no_main] use std::cmp::Ordering::*; +use std::fmt::Debug; use libfuzzer_sys::fuzz_target; -use routecore::bgp::{path_attributes::PaMap, path_selection::{OrdRoute, Rfc4271, SkipMed, TiebreakerInfo}}; +use routecore::bgp::{ + path_attributes::PaMap, + path_selection::{OrdStrat, OrdRoute, Rfc4271, SkipMed, TiebreakerInfo, + best_backup, best_backup_generic, best + } +}; fn verify_ord(a: &T, b: &T, c: &T) where T: Ord @@ -36,6 +42,71 @@ fn verify_ord(a: &T, b: &T, c: &T) } } +fn verify_path_selection<'a, T, OS, I>(candidates: I) + where + I: Clone + Iterator, + T: Debug + Ord + core::borrow::Borrow>, + OS: Debug + OrdStrat, +{ + let best1 = best(candidates.clone()).unwrap(); + let (best2, backup2) = best_backup(candidates.clone()); + let best2 = best2.unwrap(); + + assert_eq!(best1.borrow(), best2.borrow()); + assert_eq!(best1.borrow().pa_map(), best2.borrow().pa_map()); + assert_eq!(best1.borrow().tiebreakers(), best2.borrow().tiebreakers()); + + let backup2 = match backup2 { + Some(route) => route, + None => { + let mut iter = candidates.clone(); + let mut cur = iter.next().unwrap(); + for c in iter { + assert_eq!(cur.borrow().inner(), c.borrow().inner()); + cur = c; + } + return; + } + }; + + assert_ne!( + (best2.borrow().tiebreakers(), best2.borrow().pa_map()), + (backup2.borrow().tiebreakers(), backup2.borrow().pa_map()), + ); + + let (best_gen, backup_gen) = verify_path_selection_generic(candidates); + let (best_gen, _backup_gen) = (best_gen.unwrap(), backup_gen.unwrap()); + + assert_eq!(best2, best_gen); + + // Because best_backup_generic can't deal with duplicates, this won't + // always hold: + //assert_eq!(backup2, backup_gen); + +} + +fn verify_path_selection_generic(candidates: I) -> (Option, Option) +where + I: Iterator, + T: Debug + Ord +{ + + let (best, backup) = best_backup_generic( + // create tuples of the OrdRoute and a unique 'id' + candidates.enumerate().map(|(idx, c)| (c, idx)) + ); + // because the returned values are tuples as well, we can now compare + // those (instead of comparing the OrdRoute). With the unique IDs + // attached, we can check whether `best` and `backup` are not equal in + // terms of content. + assert!(best.is_some()); + assert!(backup.is_some()); + assert_ne!(best, backup); + + + (best.map(|t| t.0), backup.map(|t| t.0)) +} + // XXX while doing fuzz_target!(|data: &[u8]| ... and then creating an // Unstructured from `data` can be useful, the Debug output becomes quite // useless. It'll be just a bunch of bytes without any structure. @@ -65,8 +136,9 @@ fuzz_target!(|data: ( Err(_) => return, }; - //dbg!("skipmed"); verify_ord(&a, &b, &c); + verify_path_selection([&a, &b, &c, &b, &c, &a].into_iter()); + //verify_path_selection_generic([&a, &b, &c].into_iter()); //dbg!("rfc4271"); /* diff --git a/src/bgp/path_selection.rs b/src/bgp/path_selection.rs index 86d0205..b254a4b 100644 --- a/src/bgp/path_selection.rs +++ b/src/bgp/path_selection.rs @@ -12,7 +12,13 @@ use super::{ }; /// Wrapper type used for comparison and path selection. -#[derive(Copy, Clone, Debug)] +/// +/// Be aware that the `PartialEq` implementation of `OrdRoute` is specifically +/// tailored towards its `Ord` implementation, to do the BGP Decision Process. +/// Comparing two `OrdRoute`s, wrapping two routes with different sets of path +/// attributes, might still yield 'equal'. Instead, consider comparing on the +/// `PaMap` and/or `Tiebreakerinfo` directly (or [`fn inner`]). +#[derive(Copy, Clone, Debug, Hash)] pub struct OrdRoute<'a, OS> { pa_map: &'a PaMap, tiebreakers: TiebreakerInfo, @@ -53,6 +59,22 @@ impl<'a, OS> OrdRoute<'a, OS> { Ok(self) } + + + /// Returns the `TiebreakerInfo`. + pub fn tiebreakers(&self) -> TiebreakerInfo { + self.tiebreakers + } + + /// Returns a reference to the Path Attributes. + pub fn pa_map(&self) -> &PaMap { + self.pa_map + } + + /// Returns a tuple of the actual content of this OrdRoute. + pub fn inner(&self) -> (TiebreakerInfo, &PaMap) { + (self.tiebreakers, self.pa_map()) + } } impl<'a, OS: OrdStrat> OrdRoute<'a, OS> { @@ -386,6 +408,24 @@ pub struct TiebreakerInfo { peer_addr: net::IpAddr, } +impl TiebreakerInfo { + pub fn new( + source: RouteSource, + degree_of_preference: Option, + local_asn: Asn, + bgp_identifier: BgpIdentifier, + peer_addr: net::IpAddr, + ) -> Self { + Self { + source, + degree_of_preference, + local_asn, + bgp_identifier, + peer_addr, + } + } +} + /// Describes whether the route was learned externally or internally. #[derive(Copy, Clone, Debug, Eq, Hash, PartialEq, Ord, PartialOrd)] #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] @@ -452,55 +492,55 @@ impl fmt::Display for DecisionErrorType { //------------ Helpers ------------------------------------------------------- -pub fn preferred<'a, OS: OrdStrat>( - a: OrdRoute<'a, OS>, b: OrdRoute<'a, OS> -) -> OrdRoute<'a, OS> { + +/// Returns the preferred route. +/// +/// Note this method works on anything `T: Ord` and is thus not limited to +/// `OrdRoute`. Hence one can pass in a tuple of `(OrdRoute, L)` as long as +/// `L` implements `Ord`, to include and get back any local value like an ID. +/// This is consistent with [`fn best`] and [`fn best_backup_generic`]. +pub fn preferred(a: T, b: T) -> T { cmp::min(a,b) } -pub fn best<'a, OS: 'a + OrdStrat, I>(it: I) -> Option +/// Selects the preferred ('best') route. +/// +/// Note this method works on an iterator yielding anything `T: Ord`, so not +/// limited to `OrdRoute`. It is in that sense consistent with [`fn +/// best_backup_generic`], i.e. one can pass in tuples of `OrdRoute` and +/// something else that implements `Ord`. +pub fn best(it: I) -> Option where - I: Iterator> + T: Ord, + I: Iterator, { it.min() } -pub fn best_with_strat<'a, OS, AltOS, I>(it: I) --> Option> -where - OS: 'a + OrdStrat, - AltOS: 'a + OrdStrat, - I: Iterator>, -{ - it.map(|r| r.into_strat()).min() -} - - -// XXX would this be convenient? -pub fn best_alt<'a, OS: OrdStrat>( - _pamaps: impl Iterator, - _tiebreakers: impl Iterator -) -> Option> { - unimplemented!() -} - -pub fn best_backup_vec<'a, OS: OrdStrat>( - it: impl Iterator> -) -> (Option>, Option>) -{ - let mut sorted = it.collect::>(); - sorted.sort(); - let mut iter = sorted.into_iter(); - - let best = iter.next(); - let backup = iter.next(); - - (best, backup) -} - -pub fn best_backup<'a, OS: OrdStrat>( - it: impl Iterator> -) -> (Option>, Option>) +/// Alternative, generic version of `fn best_backup`. +/// +/// This method takes any iterator yielding items implementing Ord. As such, +/// it has little to do with routes or path selection per se. +/// +/// Note that because of this genericness, we have no access to methods or +/// members of `T`, and thus are unable to compare actual contents such as a +/// `PaMap` or the `TiebreakerInfo` when comparing `OrdRoute`s. This means the +/// method can not check for any duplicate route information between `T`s, and +/// really only order them. The caller therefore has to make sure to pass in +/// an iterator that does not yield any duplicate routes. +/// +/// This method enables the caller to attach additional information, as long +/// as it implements `Ord`. For example, one can pass in an iterator over +/// tuples of `OrdRoute` and something else. As long as the `OrdRoute` is the +/// first member of that tuple and no duplicates are yielded from the +/// iterator, the additional information is not used in the ordering process +/// but is returned together with the 'best' and 'backup' tuples. This can be +/// useful when the caller needs to relate routes to local IDs or something +/// alike. +pub fn best_backup_generic(it: I) -> (Option, Option) +where + I: Iterator, + T: Ord { let mut best = None; let mut backup = None; @@ -510,7 +550,7 @@ pub fn best_backup<'a, OS: OrdStrat>( None => { best = Some(c); continue } Some(cur_best) => { if c < cur_best { - // c is preferred over current + // c is preferred over current best best = Some(c); backup = Some(cur_best); continue; @@ -519,13 +559,93 @@ pub fn best_backup<'a, OS: OrdStrat>( best = Some(cur_best); // c is not better than best, now check backup - if let Some(bkup) = backup.take() { - if c < bkup { - // c is preferred over backup - backup = Some(c); - continue; + match backup.take() { + None => { backup = Some(c); } + Some(cur_backup) => { + if c < cur_backup { + // c is preferred over current backup + backup = Some(c); + } else { + // put it back in + backup = Some(cur_backup); + } + + } + } + } + } + } + + (best, backup) +} + + +// Attaches an index to a (generic) T, only used internally in _best_backup. +type RouteWithIndex = (usize, T); + +// Internal version doing the heavy lifting for both best_backup and +// best_backup_position. Returns both the routes themselves (T) and their +// index (usize). +fn _best_backup<'a, I, T, OS>(it: I) + -> (Option>, Option>) +where + OS: OrdStrat, + I: Iterator, + T: Ord + core::borrow::Borrow> +{ + let mut best: Option> = None; + let mut backup: Option> = None; + + for (idx, c) in it.enumerate() { + match best.take() { + None => { best = Some((idx, c)); continue } + Some((idx_best, cur_best)) => { + if c < cur_best { + // c is preferred over current best + best = Some((idx, c)); + backup = Some((idx_best, cur_best)); + continue; + } + // put it back in + best = Some((idx_best, cur_best)); + + // c is not better than best, now check backup + match backup.take() { + None => { + // Before we set the backup route, ensure it is not + // the same route as best. + // We compare the actual contents of the OrdRoute + // here, i.e. the TiebreakerInfo and PaMap. If we'd + // simply compare the OrdRoutes themselves, we'd be + // testing whether or not they are considered equal in + // terms of path preference, NOT in terms of content. + // If for example we have a candidate backup path with + // a different AS_PATH but of equal length as the best + // path, the OrdRoutes are considered equal even + // though the actual path attributes differ. + // + + // `best` is always Some(..) at this point, but we do + // an `if let` instead of an `unwrap` anyway. + if let Some((_, cur_best)) = best.as_ref() { + if cur_best.borrow().inner() != c.borrow().inner() + { + backup = Some((idx, c)); + } + } + } + Some((idx_backup, cur_backup)) => { + if c < cur_backup { + // c is preferred over current backup + // check if it is not the same route as 'best' + if best.as_ref().map(|t| &t.1) != Some(&c) { + backup = Some((idx, c)); + continue; + } + } + // put it back in + backup = Some((idx_backup, cur_backup)); } - backup = Some(bkup); } } } @@ -534,27 +654,41 @@ pub fn best_backup<'a, OS: OrdStrat>( (best, backup) } +/// Returns the 'best' and second-best path. +/// +/// If the iterator passed in contains no paths, `(None, None)` is returned. +/// If the iterator yields only a single item, that will be the 'best' path, +/// and the 'backup' will be None, i.e. `(Some(best), None)`. +/// +/// In all other cases (an iterator yielding two or more non-identical +/// values), both members of the tuple should be `Some(..)`. +/// +/// The returned 'best' path is the same as the path returned by [`fn best`]. +pub fn best_backup<'a, I, T, OS>(it: I) -> (Option, Option) +where + OS: OrdStrat, + I: Iterator, + T: Ord + core::borrow::Borrow> +{ + let (best, backup) = _best_backup(it); + (best.map(|b| b.1), backup.map(|b| b.1)) +} -pub fn best_multistrat<'a, OS1: 'a + OrdStrat, OS2: 'a + OrdStrat, I>(it: I) --> Option<(OrdRoute<'a, OS1>, OrdRoute<'a, OS2>)> -where - I: Clone + Iterator>, +/// Returns the index of the best and backup paths in the passed iterator. +pub fn best_backup_position<'a, I, T, OS>(it: I) + -> (Option, Option) +where + OS: OrdStrat, + I: Iterator, + T: Ord + core::borrow::Borrow> { - let res1 = best(it.clone()); - let res1 = match res1 { - Some(r) => r, - None => return None - }; - - // Given that res1 is not None, `it` is non-empty. - // For a non-empty collection of OrdRoutes, there is always a best, so we - // can unwrap(). - let res2 = best_with_strat::<'_, _, OS2, _>(it).unwrap(); - - Some((res1, res2)) + let (best, backup) = _best_backup(it); + (best.map(|b| b.0), backup.map(|b| b.0)) } + + //------------ Tests --------------------------------------------------------- #[cfg(test)] @@ -617,7 +751,7 @@ mod tests { #[test] - fn helpers() { + fn helpers_ordroute() { let tiebreakers = TiebreakerInfo { source: RouteSource::Ebgp, @@ -634,31 +768,46 @@ mod tests { let mut b_pamap = PaMap::empty(); b_pamap.set(Origin(OriginType::Egp)); - b_pamap.set(HopPath::from([10, 25, 30])); + b_pamap.set(HopPath::from([10, 25, 30, 40])); b_pamap.set(MultiExitDisc(50)); let mut c_pamap = PaMap::empty(); c_pamap.set(Origin(OriginType::Egp)); - c_pamap.set(HopPath::from([80, 90, 100])); + c_pamap.set(HopPath::from([80, 90, 100, 200, 300])); let candidates = [ - OrdRoute::rfc4271(&a_pamap, tiebreakers).unwrap(), - OrdRoute::rfc4271(&b_pamap, tiebreakers).unwrap(), - OrdRoute::rfc4271(&c_pamap, tiebreakers).unwrap(), + OrdRoute::skip_med(&a_pamap, tiebreakers).unwrap(), + OrdRoute::skip_med(&b_pamap, tiebreakers).unwrap(), + OrdRoute::skip_med(&a_pamap, tiebreakers).unwrap(), + //OrdRoute::skip_med(&c_pamap, tiebreakers).unwrap(), ]; - let best1 = best(candidates.into_iter()); - let (best2, backup2) = best_backup_vec(candidates.into_iter()); - let (best3, backup3) = best_backup(candidates.into_iter()); + let best1 = best(candidates.iter().cloned()).unwrap(); + let (best2, backup2) = best_backup(candidates.iter()); + let (best2, backup2) = (best2.unwrap(), backup2.unwrap()); - assert_eq!(best1, best2); - assert_eq!(best2, best3); - assert_eq!(backup2, backup3); - assert_ne!(best2, backup2); - assert_ne!(best3, backup3); + assert_eq!(best1.pa_map(), best2.pa_map()); + assert_eq!(best1.tiebreakers(), best2.tiebreakers()); - dbg!(&best1); + dbg!(&best2); dbg!(&backup2); + + assert_ne!( + (best2.pa_map(), best2.tiebreakers()), + (backup2.pa_map(), backup2.tiebreakers()) + ); + } + /* + #[test] + fn helpers_generic() { + let (a,b,c) = ("2".to_string(), "1".to_string(), "3".to_string()); + //let (a,b,c) = (2, 1, 3); + let values = vec![&a, &b, &c]; + let (best, backup) = best_backup(values.into_iter()); + assert_eq!(best, Some(&b)); + assert_eq!(backup, Some(&a)); + } + */ }