From a7b3a16e3cdea2860630ad86702d7b4364dacb65 Mon Sep 17 00:00:00 2001 From: Ties de Kock Date: Mon, 8 Jun 2026 20:46:10 +0200 Subject: [PATCH 1/3] Improve route level parser --- examples/route_level_parsing.rs | 83 +++-- src/models/bgp/elem.rs | 56 ++- src/parser/filter.rs | 11 +- src/parser/iters/mod.rs | 14 +- src/parser/iters/route.rs | 594 +++++++++++++++++++++++++------- src/parser/utils.rs | 2 +- 6 files changed, 579 insertions(+), 181 deletions(-) diff --git a/examples/route_level_parsing.rs b/examples/route_level_parsing.rs index 9e7dba0..9b09372 100644 --- a/examples/route_level_parsing.rs +++ b/examples/route_level_parsing.rs @@ -29,21 +29,26 @@ fn main() { let parser = BgpkitParser::new(url).unwrap(); let mut route_count = 0; - for route in parser.into_route_iter().take(1000) { - if route_count < 3 { - // Show first few routes - log::info!( - "Route {}: {} via AS{} (peer: {})", - route_count + 1, - route.prefix, - route.peer_asn, - route.peer_ip - ); - if let Some(ref path) = route.as_path { - log::info!(" AS Path: {}", path); + 'routes: for batch in parser.into_route_iter() { + for route in batch.routes() { + let route = route.unwrap(); + if route_count < 3 { + log::info!( + "Route {}: {} via AS{} (peer: {})", + route_count + 1, + route.prefix, + route.peer_asn, + route.peer_ip + ); + if let Some(path) = route.as_path { + log::info!(" AS Path: {}", path); + } + } + route_count += 1; + if route_count >= 1000 { + break 'routes; } } - route_count += 1; } let route_time = start.elapsed(); log::info!( @@ -89,15 +94,23 @@ fn main() { let filter = bgpkit_parser::Filter::new("peer_asn", "49788").unwrap(); let mut filtered_count = 0; - for route in parser.into_route_iter().take(1000) { - if route.match_filter(&filter) { - filtered_count += 1; - if filtered_count <= 3 { - log::info!( - "Matched filter (peer_asn=49788): {} from AS{}", - route.prefix, - route.peer_asn - ); + let mut seen = 0; + 'routes: for batch in parser.into_route_iter() { + for route in batch.all_routes() { + let route = route.unwrap(); + if route.match_filter(&filter) { + filtered_count += 1; + if filtered_count <= 3 { + log::info!( + "Matched filter (peer_asn=49788): {} from AS{}", + route.prefix, + route.peer_asn + ); + } + } + seen += 1; + if seen >= 1000 { + break 'routes; } } } @@ -114,15 +127,23 @@ fn main() { let as_path_filter = bgpkit_parser::Filter::new("as_path", "1299").unwrap(); let mut as_path_matches = 0; - for route in parser.into_route_iter().take(1000) { - if route.match_filter(&as_path_filter) { - as_path_matches += 1; - if as_path_matches <= 3 { - log::info!( - "AS Path contains 1299: {} - path: {:?}", - route.prefix, - route.as_path.as_ref().map(|p| p.to_string()) - ); + let mut seen = 0; + 'routes: for batch in parser.into_route_iter() { + for route in batch.all_routes() { + let route = route.unwrap(); + if route.match_filter(&as_path_filter) { + as_path_matches += 1; + if as_path_matches <= 3 { + log::info!( + "AS Path contains 1299: {} - path: {:?}", + route.prefix, + route.as_path.map(|p| p.to_string()) + ); + } + } + seen += 1; + if seen >= 1000 { + break 'routes; } } } diff --git a/src/models/bgp/elem.rs b/src/models/bgp/elem.rs index 164e498..77a2bc8 100644 --- a/src/models/bgp/elem.rs +++ b/src/models/bgp/elem.rs @@ -157,15 +157,15 @@ pub struct BgpElem { pub deprecated: Option>, } -/// Lightweight per-prefix route element. +/// Lightweight borrowed per-prefix route element. /// /// This struct is intended for fast scans that only need route identity, /// peer metadata, timestamp, and AS path. Use [`BgpElem`] when you need the /// full set of BGP attributes. Because route elements do not carry /// communities, community filters do not match [`BgpRouteElem`] values. -#[derive(Debug, Clone, PartialEq)] -#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] -pub struct BgpRouteElem { +#[derive(Debug, Clone, Copy, PartialEq)] +#[cfg_attr(feature = "serde", derive(serde::Serialize))] +pub struct BgpRouteElem<'a> { /// The timestamp of the item in floating-point format. pub timestamp: f64, /// The element type of an item. @@ -177,16 +177,47 @@ pub struct BgpRouteElem { pub peer_asn: Asn, /// The network prefix of the item. pub prefix: NetworkPrefix, - /// The optional path representation of the item. + /// The optional borrowed path representation of the item. /// - /// Route-level parsing shares the same AS path across all announced - /// prefixes from a single message. + /// Route-level parsing keeps the AS path on the containing route batch + /// and borrows it for each announced prefix. + pub as_path: Option<&'a AsPath>, +} + +/// Owned lightweight per-prefix route element. +/// +/// Convert [`BgpRouteElem`] into this type when route rows must outlive the +/// route batch they came from. +#[derive(Debug, Clone, PartialEq)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct BgpRouteElemOwned { + pub timestamp: f64, + #[cfg_attr(feature = "serde", serde(rename = "type"))] + pub elem_type: ElemType, + pub peer_ip: IpAddr, + pub peer_asn: Asn, + pub prefix: NetworkPrefix, pub as_path: Option>, } +impl BgpRouteElem<'_> { + pub fn into_owned(self) -> BgpRouteElemOwned { + BgpRouteElemOwned { + timestamp: self.timestamp, + elem_type: self.elem_type, + peer_ip: self.peer_ip, + peer_asn: self.peer_asn, + prefix: self.prefix, + as_path: self.as_path.map(|path| Arc::new(path.clone())), + } + } +} + impl Eq for BgpElem {} -impl Eq for BgpRouteElem {} +impl Eq for BgpRouteElem<'_> {} + +impl Eq for BgpRouteElemOwned {} impl PartialOrd for BgpElem { fn partial_cmp(&self, other: &Self) -> Option { @@ -194,7 +225,7 @@ impl PartialOrd for BgpElem { } } -impl PartialOrd for BgpRouteElem { +impl PartialOrd for BgpRouteElem<'_> { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } @@ -209,7 +240,7 @@ impl Ord for BgpElem { } } -impl Ord for BgpRouteElem { +impl Ord for BgpRouteElem<'_> { fn cmp(&self, other: &Self) -> Ordering { self.timestamp .partial_cmp(&other.timestamp) @@ -324,7 +355,7 @@ impl Display for BgpElem { } } -impl Display for BgpRouteElem { +impl Display for BgpRouteElem<'_> { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { let t = match self.elem_type { ElemType::ANNOUNCE => "A", @@ -534,13 +565,14 @@ mod tests { #[test] fn test_route_elem_display() { + let as_path = AsPath::from_sequence([64496, 64497]); let elem = BgpRouteElem { timestamp: 1.1, elem_type: ElemType::ANNOUNCE, peer_ip: IpAddr::from_str("192.168.1.1").unwrap(), peer_asn: 64496.into(), prefix: NetworkPrefix::from_str("8.8.8.0/24").unwrap(), - as_path: Some(Arc::new(AsPath::from_sequence([64496, 64497]))), + as_path: Some(&as_path), }; assert_eq!( diff --git a/src/parser/filter.rs b/src/parser/filter.rs index bc1f025..d4128e2 100644 --- a/src/parser/filter.rs +++ b/src/parser/filter.rs @@ -676,7 +676,7 @@ impl RouteFilterView for BgpElem { } } -impl RouteFilterView for BgpRouteElem { +impl RouteFilterView for BgpRouteElem<'_> { fn timestamp(&self) -> f64 { self.timestamp } @@ -698,7 +698,7 @@ impl RouteFilterView for BgpRouteElem { } fn as_path(&self) -> Option<&AsPath> { - self.as_path.as_deref() + self.as_path } fn matches_origin_asn(&self, asn: Asn) -> bool { @@ -715,7 +715,7 @@ impl Filterable for BgpElem { } } -impl Filterable for BgpRouteElem { +impl Filterable for BgpRouteElem<'_> { fn match_filter(&self, filter: &Filter) -> bool { match_route_view_filter(self, filter) } @@ -727,7 +727,6 @@ mod tests { use crate::BgpkitParser; use anyhow::Result; use std::str::FromStr; - use std::sync::Arc; fn filter_test_elem() -> BgpElem { BgpElem { @@ -756,14 +755,14 @@ mod tests { } } - fn route_projection(elem: &BgpElem) -> BgpRouteElem { + fn route_projection(elem: &BgpElem) -> BgpRouteElem<'_> { BgpRouteElem { timestamp: elem.timestamp, elem_type: elem.elem_type, peer_ip: elem.peer_ip, peer_asn: elem.peer_asn, prefix: elem.prefix, - as_path: elem.as_path.clone().map(Arc::new), + as_path: elem.as_path.as_ref(), } } diff --git a/src/parser/iters/mod.rs b/src/parser/iters/mod.rs index c8897f7..f31a568 100644 --- a/src/parser/iters/mod.rs +++ b/src/parser/iters/mod.rs @@ -20,7 +20,7 @@ mod update; pub use default::{ElemIterator, RecordIterator}; pub use fallible::{FallibleElemIterator, FallibleRecordIterator}; pub use raw::RawRecordIterator; -pub use route::{FallibleRouteIterator, RouteIterator}; +pub use route::{FallibleRouteIterator, RouteBatch, RouteBatchIter, RouteIterator}; pub use update::{ Bgp4MpUpdate, FallibleUpdateIterator, MrtUpdate, TableDumpV2Entry, UpdateIterator, }; @@ -115,13 +115,13 @@ impl BgpkitParser { UpdateIterator::new(self) } - /// Creates an iterator over lightweight route elements from MRT data. + /// Creates an iterator over lightweight route batches from MRT data. /// - /// This iterator yields [`BgpRouteElem`](crate::models::BgpRouteElem) - /// values and only parses route identity, peer metadata, timestamp, and - /// AS path. Use [`into_elem_iter`](Self::into_elem_iter) when you need - /// the full [`BgpElem`] attribute set. Filters that only depend on route - /// fields are supported; `community` filters do not match route elements. + /// This iterator yields [`RouteBatch`] values. Each batch owns shared + /// route state such as AS path and raw prefix bytes; call + /// [`RouteBatch::routes`] to iterate filtered [`BgpRouteElem`](crate::models::BgpRouteElem) + /// rows. Use [`into_elem_iter`](Self::into_elem_iter) when you need the + /// full [`BgpElem`] attribute set. pub fn into_route_iter(self) -> RouteIterator { RouteIterator::new(self) } diff --git a/src/parser/iters/route.rs b/src/parser/iters/route.rs index 9a1e1ad..ffed7d2 100644 --- a/src/parser/iters/route.rs +++ b/src/parser/iters/route.rs @@ -1,23 +1,23 @@ use crate::error::{ParserError, ParserErrorWithBytes}; use crate::models::*; -use crate::parser::bgp::attributes::{parse_as_path, parse_nlri, AttributeValidationState}; +use crate::parser::bgp::attributes::{parse_as_path, AttributeValidationState}; use crate::parser::bgp::messages::read_and_validate_bgp_marker; use crate::parser::iters::write_mrt_core_dump; use crate::parser::mrt::messages::bgp4mp::bgp4mp_message_payload_len; use crate::parser::mrt::messages::table_dump_v2::rib_entry_min_len; -use crate::parser::{chunk_mrt_record, parse_nlri_list, BgpkitParser, Filterable, ReadUtils}; +use crate::parser::{chunk_mrt_record, try_parse_prefix, BgpkitParser, Filter, Filterable, ReadUtils}; use bytes::{Buf, Bytes}; use ipnet::IpNet; -use log::{error, warn}; +use log::{debug, error, warn}; use std::io::Read; use std::net::IpAddr; use std::sync::Arc; #[derive(Default)] struct RouteAttributes { - as_path: Option>, - announced: Vec, - withdrawn: Vec, + as_path: Option, + announced: Vec, + withdrawn: Vec, } struct RouteAttributeContext<'a> { @@ -28,13 +28,89 @@ struct RouteAttributeContext<'a> { has_standard_nlri: bool, } -fn merge_as_path(as_path: Option, as4_path: Option) -> Option> { - let path = match (as_path, as4_path) { +#[derive(Clone)] +struct RouteNlriBytes { + data: Bytes, + afi: Afi, + add_path: bool, +} + +fn merge_as_path(as_path: Option, as4_path: Option) -> Option { + match (as_path, as4_path) { (None, None) => None, (Some(path), None) | (None, Some(path)) => Some(path), (Some(path), Some(as4_path)) => Some(AsPath::merge_aspath_as4path(&path, &as4_path)), + } +} + +fn parse_route_nlri_bytes( + mut input: Bytes, + afi: &Option, + safi: &Option, + prefixes: &Option<&[NetworkPrefix]>, + reachable: bool, + add_path: bool, +) -> Result, ParserError> { + let first_byte_zero = input.first().map(|b| *b == 0).unwrap_or(false); + + let afi = match afi { + Some(afi) => { + if first_byte_zero { + input.read_afi()? + } else { + *afi + } + } + None => input.read_afi()?, }; - path.map(Arc::new) + let safi = match safi { + Some(safi) => { + if first_byte_zero { + input.read_safi()? + } else { + *safi + } + } + None => input.read_safi()?, + }; + + if afi == Afi::LinkState + || safi == Safi::LinkState + || safi == Safi::LinkStateVpn + || safi == Safi::MplsLabel + { + return Ok(None); + } + + if reachable { + let next_hop_length = input.read_u8()? as usize; + input.has_n_remaining(next_hop_length)?; + input.advance(next_hop_length); + } + + if let Some(prefixes) = prefixes { + if !first_byte_zero { + return Ok(Some(RouteNlriBytes { + data: encode_nlri_prefixes_without_alloc_api(prefixes), + afi, + add_path, + })); + } + } + + if reachable && input.read_u8()? != 0 { + warn!("NLRI reserved byte not 0 (parsing route-level NLRI prefixes)"); + } + + Ok(Some(RouteNlriBytes { + data: input, + afi, + add_path, + })) +} + +fn encode_nlri_prefixes_without_alloc_api(prefixes: &[NetworkPrefix]) -> Bytes { + crate::parser::encode_nlri_prefixes(prefixes) } fn parse_route_attributes( @@ -78,7 +154,7 @@ fn parse_route_attributes( AttrType::AS4_PATH => parse_as_path(attr_data, &AsnLength::Bits32).map(|path| { as4_path = Some(path); }), - AttrType::MP_REACHABLE_NLRI => parse_nlri( + AttrType::MP_REACHABLE_NLRI => parse_route_nlri_bytes( attr_data, &ctx.afi, &ctx.safi, @@ -86,12 +162,12 @@ fn parse_route_attributes( true, add_path, ) - .map(|attr| { - if let AttributeValue::MpReachNlri(nlri) = attr { - announced = nlri.prefixes; + .map(|nlri| { + if let Some(nlri) = nlri { + announced.push(nlri); } }), - AttrType::MP_UNREACHABLE_NLRI => parse_nlri( + AttrType::MP_UNREACHABLE_NLRI => parse_route_nlri_bytes( attr_data, &ctx.afi, &ctx.safi, @@ -99,9 +175,9 @@ fn parse_route_attributes( false, add_path, ) - .map(|attr| { - if let AttributeValue::MpUnreachNlri(nlri) = attr { - withdrawn = nlri.prefixes; + .map(|nlri| { + if let Some(nlri) = nlri { + withdrawn.push(nlri); } }), _ => Ok(()), @@ -131,42 +207,198 @@ fn record_timestamp(common_header: &CommonHeader) -> f64 { } } -struct RouteUpdateIter { +struct NlriPrefixIter { + input: Bytes, + afi: Afi, + add_path: bool, + is_add_path: bool, + use_heuristic: bool, +} + +impl NlriPrefixIter { + fn new(input: Bytes, afi: Afi, add_path: bool) -> Self { + Self { + input, + afi, + add_path, + is_add_path: add_path, + use_heuristic: false, + } + } +} + +impl Iterator for NlriPrefixIter { + type Item = Result; + + fn next(&mut self) -> Option { + if self.input.remaining() == 0 { + return None; + } + + let data = self.input.as_ref(); + if !self.is_add_path && !data.is_empty() && data[0] == 0 { + debug!("NLRI: first byte is 0, treating as Add-Path format"); + self.is_add_path = true; + self.use_heuristic = true; + } + + let (prefix, consumed) = match try_parse_prefix(data, &self.afi, self.is_add_path) { + Ok(result) => result, + Err(_) if self.use_heuristic => { + debug!( + "NLRI: Add-Path heuristic failed, retrying with add_path={}", + self.add_path + ); + self.is_add_path = self.add_path; + self.use_heuristic = false; + match try_parse_prefix(data, &self.afi, self.add_path) { + Ok(result) => result, + Err(err) => return Some(Err(err)), + } + } + Err(err) => return Some(Err(err)), + }; + + self.input.advance(consumed); + Some(Ok(prefix)) + } +} + +#[derive(Clone)] +enum RoutePrefixSource { + One(Option), + Nlri(RouteNlriBytes), +} + +impl RoutePrefixSource { + fn iter(&self) -> RoutePrefixSourceIter { + match self { + RoutePrefixSource::One(prefix) => RoutePrefixSourceIter::One(*prefix), + RoutePrefixSource::Nlri(nlri) => RoutePrefixSourceIter::Nlri(NlriPrefixIter::new( + nlri.data.clone(), + nlri.afi, + nlri.add_path, + )), + } + } +} + +enum RoutePrefixSourceIter { + One(Option), + Nlri(NlriPrefixIter), +} + +impl Iterator for RoutePrefixSourceIter { + type Item = Result; + + fn next(&mut self) -> Option { + match self { + RoutePrefixSourceIter::One(prefix) => prefix.take().map(Ok), + RoutePrefixSourceIter::Nlri(iter) => iter.next(), + } + } +} + +struct RouteBatchPart { + elem_type: ElemType, + source: RoutePrefixSource, + has_as_path: bool, +} + +/// A route-level MRT/BGP batch with shared parsed state. +/// +/// A batch corresponds to one MRT route-bearing record or BGP UPDATE message. +/// Prefixes are parsed lazily from shared `Bytes` slices when iterating routes. +#[derive(Clone)] +pub struct RouteBatch { timestamp: f64, peer_ip: IpAddr, peer_asn: Asn, - as_path: Option>, - announced: - std::iter::Chain, std::vec::IntoIter>, - withdrawn: - std::iter::Chain, std::vec::IntoIter>, - in_withdrawn_phase: bool, + as_path: Option, + parts: Arc<[RouteBatchPart]>, + filters: Arc<[Filter]>, } -impl RouteUpdateIter { - fn next_route(&mut self) -> Option { - if !self.in_withdrawn_phase { - if let Some(prefix) = self.announced.next() { - return Some(BgpRouteElem { - timestamp: self.timestamp, - elem_type: ElemType::ANNOUNCE, - peer_ip: self.peer_ip, - peer_asn: self.peer_asn, - prefix, - as_path: self.as_path.clone(), - }); - } - self.in_withdrawn_phase = true; +impl RouteBatch { + fn new( + timestamp: f64, + peer_ip: IpAddr, + peer_asn: Asn, + as_path: Option, + parts: Vec, + filters: Arc<[Filter]>, + ) -> Self { + Self { + timestamp, + peer_ip, + peer_asn, + as_path, + parts: Arc::from(parts), + filters, } + } - self.withdrawn.next().map(|prefix| BgpRouteElem { - timestamp: self.timestamp, - elem_type: ElemType::WITHDRAW, - peer_ip: self.peer_ip, - peer_asn: self.peer_asn, - prefix, - as_path: None, - }) + pub fn routes(&self) -> RouteBatchIter<'_> { + RouteBatchIter::new(self, true) + } + + pub fn all_routes(&self) -> RouteBatchIter<'_> { + RouteBatchIter::new(self, false) + } +} + +pub struct RouteBatchIter<'a> { + batch: &'a RouteBatch, + part_index: usize, + current: Option, + filtered: bool, +} + +impl<'a> RouteBatchIter<'a> { + fn new(batch: &'a RouteBatch, filtered: bool) -> Self { + Self { + batch, + part_index: 0, + current: None, + filtered, + } + } +} + +impl<'a> Iterator for RouteBatchIter<'a> { + type Item = Result, ParserError>; + + fn next(&mut self) -> Option { + loop { + if self.current.is_none() { + let part = self.batch.parts.get(self.part_index)?; + self.part_index += 1; + self.current = Some(part.source.iter()); + } + + let part = &self.batch.parts[self.part_index - 1]; + let prefix = match self.current.as_mut().and_then(Iterator::next) { + Some(Ok(prefix)) => prefix, + Some(Err(err)) => return Some(Err(err)), + None => { + self.current = None; + continue; + } + }; + + let route = BgpRouteElem { + timestamp: self.batch.timestamp, + elem_type: part.elem_type, + peer_ip: self.batch.peer_ip, + peer_asn: self.batch.peer_asn, + prefix, + as_path: part.has_as_path.then_some(()).and(self.batch.as_path.as_ref()), + }; + + if !self.filtered || route.match_filters(&self.batch.filters) { + return Some(Ok(route)); + } + } } } @@ -222,18 +454,22 @@ fn parse_route_peer_table(mut data: Bytes) -> Result), - Update(RouteUpdateIter), + One(RouteBatch), RibAfi(RouteRibAfiIter), } impl RouteRecordIter { - fn next_route(&mut self) -> Result, ParserError> { - match self { + fn next_batch(&mut self, filters: Arc<[Filter]>) -> Result, ParserError> { + match std::mem::take(self) { RouteRecordIter::Empty => Ok(None), - RouteRecordIter::One(route) => Ok(route.take()), - RouteRecordIter::Update(iter) => Ok(iter.next_route()), - RouteRecordIter::RibAfi(iter) => iter.next_route(), + RouteRecordIter::One(batch) => Ok(Some(batch)), + RouteRecordIter::RibAfi(mut iter) => { + let batch = iter.next_batch(filters)?; + if iter.remaining_entries > 0 { + *self = RouteRecordIter::RibAfi(iter); + } + Ok(batch) + } } } } @@ -249,7 +485,7 @@ struct RouteRibAfiIter { } impl RouteRibAfiIter { - fn next_route(&mut self) -> Result, ParserError> { + fn next_batch(&mut self, filters: Arc<[Filter]>) -> Result, ParserError> { while self.remaining_entries > 0 { if self.data.remaining() < rib_entry_min_len(self.is_add_path) { warn!("early break due to truncated msg while parsing RIB AFI entries"); @@ -294,14 +530,18 @@ impl RouteRibAfiIter { continue; }; - return Ok(Some(BgpRouteElem { - timestamp: originated_time, - elem_type: ElemType::ANNOUNCE, - peer_ip: peer.peer_ip, - peer_asn: peer.peer_asn, - prefix: self.prefix, - as_path: attrs.as_path, - })); + return Ok(Some(RouteBatch::new( + originated_time, + peer.peer_ip, + peer.peer_asn, + attrs.as_path, + vec![RouteBatchPart { + elem_type: ElemType::ANNOUNCE, + source: RoutePrefixSource::One(Some(self.prefix)), + has_as_path: true, + }], + filters, + ))); } Ok(None) @@ -315,15 +555,17 @@ fn parse_bgp_update_routes( timestamp: f64, peer_ip: IpAddr, peer_asn: Asn, -) -> Result { + filters: Arc<[Filter]>, +) -> Result { let withdrawn_len = input.read_u16()? as usize; input.has_n_remaining(withdrawn_len)?; - let withdrawn_prefixes = parse_nlri_list(input.split_to(withdrawn_len), add_path, &Afi::Ipv4)?; + let withdrawn_prefixes = input.split_to(withdrawn_len); let attribute_length = input.read_u16()? as usize; input.has_n_remaining(attribute_length)?; let attribute_bytes = input.split_to(attribute_length); - let announced_prefixes = parse_nlri_list(input, add_path, &Afi::Ipv4)?; + let announced_prefixes = input; + let has_standard_nlri = !announced_prefixes.is_empty(); let attributes = parse_route_attributes( attribute_bytes, asn_len, @@ -333,19 +575,52 @@ fn parse_bgp_update_routes( safi: None, prefixes: None, is_announcement: None, - has_standard_nlri: !announced_prefixes.is_empty(), + has_standard_nlri, }, )?; - Ok(RouteUpdateIter { + let mut parts = Vec::new(); + if !announced_prefixes.is_empty() { + parts.push(RouteBatchPart { + elem_type: ElemType::ANNOUNCE, + source: RoutePrefixSource::Nlri(RouteNlriBytes { + data: announced_prefixes, + afi: Afi::Ipv4, + add_path, + }), + has_as_path: true, + }); + } + parts.extend(attributes.announced.into_iter().map(|nlri| RouteBatchPart { + elem_type: ElemType::ANNOUNCE, + source: RoutePrefixSource::Nlri(nlri), + has_as_path: true, + })); + if !withdrawn_prefixes.is_empty() { + parts.push(RouteBatchPart { + elem_type: ElemType::WITHDRAW, + source: RoutePrefixSource::Nlri(RouteNlriBytes { + data: withdrawn_prefixes, + afi: Afi::Ipv4, + add_path, + }), + has_as_path: false, + }); + } + parts.extend(attributes.withdrawn.into_iter().map(|nlri| RouteBatchPart { + elem_type: ElemType::WITHDRAW, + source: RoutePrefixSource::Nlri(nlri), + has_as_path: false, + })); + + Ok(RouteBatch::new( timestamp, peer_ip, peer_asn, - as_path: attributes.as_path, - announced: announced_prefixes.into_iter().chain(attributes.announced), - withdrawn: withdrawn_prefixes.into_iter().chain(attributes.withdrawn), - in_withdrawn_phase: false, - }) + attributes.as_path, + parts, + filters, + )) } fn parse_bgp_message_routes( @@ -355,6 +630,7 @@ fn parse_bgp_message_routes( timestamp: f64, peer_ip: IpAddr, peer_asn: Asn, + filters: Arc<[Filter]>, ) -> Result { let total_size = data.len(); data.has_n_remaining(19)?; @@ -391,8 +667,8 @@ fn parse_bgp_message_routes( let msg_data = data.split_to(bgp_msg_length); match msg_type { - BgpMessageType::UPDATE => Ok(RouteRecordIter::Update(parse_bgp_update_routes( - msg_data, add_path, asn_len, timestamp, peer_ip, peer_asn, + BgpMessageType::UPDATE => Ok(RouteRecordIter::One(parse_bgp_update_routes( + msg_data, add_path, asn_len, timestamp, peer_ip, peer_asn, filters, )?)), BgpMessageType::OPEN | BgpMessageType::NOTIFICATION | BgpMessageType::KEEPALIVE => { Ok(RouteRecordIter::Empty) @@ -418,6 +694,7 @@ fn parse_bgp4mp_routes( sub_type: u16, mut data: Bytes, timestamp: f64, + filters: Arc<[Filter]>, ) -> Result { let msg_type = Bgp4MpType::try_from(sub_type)?; let Some((asn_len, add_path)) = bgp4mp_asn_len_and_add_path(msg_type) else { @@ -441,7 +718,7 @@ fn parse_bgp4mp_routes( ))); } - parse_bgp_message_routes(data, add_path, &asn_len, timestamp, peer_ip, peer_asn) + parse_bgp_message_routes(data, add_path, &asn_len, timestamp, peer_ip, peer_asn, filters) } fn table_dump_v2_afi_safi(rib_type: TableDumpV2Type) -> Result<(Afi, Safi), ParserError> { @@ -474,7 +751,11 @@ fn is_add_path_rib_type(rib_type: TableDumpV2Type) -> bool { ) } -fn parse_table_dump_routes(sub_type: u16, mut data: Bytes) -> Result { +fn parse_table_dump_routes( + sub_type: u16, + mut data: Bytes, + filters: Arc<[Filter]>, +) -> Result { let afi = match sub_type { 1 => Afi::Ipv4, 2 => Afi::Ipv6, @@ -511,20 +792,25 @@ fn parse_table_dump_routes(sub_type: u16, mut data: Bytes) -> Result, + _filters: Arc<[Filter]>, ) -> Result { let v2_type = TableDumpV2Type::try_from(sub_type)?; match v2_type { @@ -564,22 +850,26 @@ fn parse_table_dump_v2_routes( fn parse_raw_record_route_iter( raw_record: crate::RawMrtRecord, peer_table: &mut Option, + filters: Arc<[Filter]>, ) -> Result { let timestamp = record_timestamp(&raw_record.common_header); match raw_record.common_header.entry_type { EntryType::TABLE_DUMP => parse_table_dump_routes( raw_record.common_header.entry_subtype, raw_record.message_bytes, + filters, ), EntryType::TABLE_DUMP_V2 => parse_table_dump_v2_routes( raw_record.common_header.entry_subtype, raw_record.message_bytes, peer_table, + filters, ), EntryType::BGP4MP | EntryType::BGP4MP_ET => parse_bgp4mp_routes( raw_record.common_header.entry_subtype, raw_record.message_bytes, timestamp, + filters, ), v => Err(ParserError::Unsupported(format!( "unsupported MRT type: {v:?}" @@ -591,30 +881,28 @@ pub struct RouteIterator { parser: BgpkitParser, pending_routes: RouteRecordIter, peer_table: Option, + filters: Arc<[Filter]>, } impl RouteIterator { pub(crate) fn new(parser: BgpkitParser) -> Self { + let filters = Arc::from(parser.filters.clone()); Self { parser, pending_routes: RouteRecordIter::Empty, peer_table: None, + filters, } } } impl Iterator for RouteIterator { - type Item = BgpRouteElem; + type Item = RouteBatch; fn next(&mut self) -> Option { loop { - match self.pending_routes.next_route() { - Ok(Some(route)) => { - if route.match_filters(&self.parser.filters) { - return Some(route); - } - continue; - } + match self.pending_routes.next_batch(Arc::clone(&self.filters)) { + Ok(Some(batch)) => return Some(batch), Ok(None) => {} Err(err) => { error!("parser error: {}", err); @@ -667,7 +955,11 @@ impl Iterator for RouteIterator { }, }; - match parse_raw_record_route_iter(raw_record, &mut self.peer_table) { + match parse_raw_record_route_iter( + raw_record, + &mut self.peer_table, + Arc::clone(&self.filters), + ) { Ok(routes) => { self.pending_routes = routes; } @@ -687,30 +979,28 @@ pub struct FallibleRouteIterator { parser: BgpkitParser, pending_routes: RouteRecordIter, peer_table: Option, + filters: Arc<[Filter]>, } impl FallibleRouteIterator { pub(crate) fn new(parser: BgpkitParser) -> Self { + let filters = Arc::from(parser.filters.clone()); Self { parser, pending_routes: RouteRecordIter::Empty, peer_table: None, + filters, } } } impl Iterator for FallibleRouteIterator { - type Item = Result; + type Item = Result; fn next(&mut self) -> Option { loop { - match self.pending_routes.next_route() { - Ok(Some(route)) => { - if route.match_filters(&self.parser.filters) { - return Some(Ok(route)); - } - continue; - } + match self.pending_routes.next_batch(Arc::clone(&self.filters)) { + Ok(Some(batch)) => return Some(Ok(batch)), Ok(None) => {} Err(error) => { self.pending_routes = RouteRecordIter::Empty; @@ -724,7 +1014,11 @@ impl Iterator for FallibleRouteIterator { Err(e) => return Some(Err(e)), }; - match parse_raw_record_route_iter(raw_record, &mut self.peer_table) { + match parse_raw_record_route_iter( + raw_record, + &mut self.peer_table, + Arc::clone(&self.filters), + ) { Ok(routes) => { self.pending_routes = routes; } @@ -743,8 +1037,8 @@ mod tests { use std::net::{Ipv4Addr, Ipv6Addr}; use std::str::FromStr; - fn route_projection(elem: BgpElem) -> BgpRouteElem { - BgpRouteElem { + fn route_projection(elem: BgpElem) -> BgpRouteElemOwned { + BgpRouteElemOwned { timestamp: elem.timestamp, elem_type: elem.elem_type, peer_ip: elem.peer_ip, @@ -756,14 +1050,30 @@ mod tests { fn collect_route_record_iter( mut iter: RouteRecordIter, - ) -> Result, ParserError> { + ) -> Result, ParserError> { let mut routes = Vec::new(); - while let Some(route) = iter.next_route()? { - routes.push(route); + let filters: Arc<[Filter]> = Arc::from([]); + while let Some(batch) = iter.next_batch(Arc::clone(&filters))? { + for route in batch.all_routes() { + routes.push(route?.into_owned()); + } } Ok(routes) } + fn collect_route_batches(parser: BgpkitParser) -> Vec { + parser + .into_route_iter() + .flat_map(|batch| { + batch + .routes() + .map(|route| route.map(BgpRouteElem::into_owned)) + .collect::>() + }) + .collect::, _>>() + .unwrap() + } + fn route_peer_table_from_peer_index(peer_table: PeerIndexTable) -> RoutePeerTable { let mut peer_ids = peer_table.id_peer_map.keys().copied().collect::>(); peer_ids.sort_unstable(); @@ -1087,19 +1397,17 @@ mod tests { .into_elem_iter() .map(route_projection) .collect::>(); - let routes = route_parser.into_route_iter().collect::>(); + let routes = collect_route_batches(route_parser); assert_eq!(routes, elem_projection, "filters: {filters:?}"); } - fn assert_route_projection(bytes: Vec) -> Vec { + fn assert_route_projection(bytes: Vec) -> Vec { let elem_projection = BgpkitParser::from_reader(Cursor::new(bytes.clone())) .into_elem_iter() .map(route_projection) .collect::>(); - let routes = BgpkitParser::from_reader(Cursor::new(bytes)) - .into_route_iter() - .collect::>(); + let routes = collect_route_batches(BgpkitParser::from_reader(Cursor::new(bytes))); assert_eq!(routes, elem_projection); routes @@ -1131,14 +1439,19 @@ mod tests { .encode() .to_vec(); - let routes = BgpkitParser::from_reader(Cursor::new(bytes)) + let batch = BgpkitParser::from_reader(Cursor::new(bytes)) .into_route_iter() - .collect::>(); + .next() + .unwrap(); + let routes = batch + .all_routes() + .collect::, _>>() + .unwrap(); assert_eq!(routes.len(), 2); - assert!(Arc::ptr_eq( - routes[0].as_path.as_ref().unwrap(), - routes[1].as_path.as_ref().unwrap() + assert!(std::ptr::eq( + routes[0].as_path.unwrap(), + routes[1].as_path.unwrap() )); } @@ -1585,7 +1898,8 @@ mod tests { &AsnLength::Bits16, 1_700_000_000.0, "192.0.2.1".parse().unwrap(), - Asn::new_16bit(64496) + Asn::new_16bit(64496), + Arc::from([]), ) .is_err()); assert!(parse_bgp_message_routes( @@ -1594,7 +1908,8 @@ mod tests { &AsnLength::Bits16, 1_700_000_000.0, "192.0.2.1".parse().unwrap(), - Asn::new_16bit(64496) + Asn::new_16bit(64496), + Arc::from([]), ) .is_err()); @@ -1606,6 +1921,7 @@ mod tests { 1_700_000_000.0, "192.0.2.1".parse().unwrap(), Asn::new_16bit(64496), + Arc::from([]), ) .unwrap(), ) @@ -1620,6 +1936,7 @@ mod tests { 1_700_000_000.0, "192.0.2.1".parse().unwrap(), Asn::new_16bit(64496), + Arc::from([]), ) .unwrap(), ) @@ -1634,6 +1951,7 @@ mod tests { 1_700_000_000.0, "192.0.2.1".parse().unwrap(), Asn::new_16bit(64496), + Arc::from([]), ) .unwrap(), ) @@ -1671,6 +1989,7 @@ mod tests { TableDumpV2Type::RibIpv4Unicast as u16, rib.encode(), &mut no_peer_table, + Arc::from([]), ) .is_err()); @@ -1680,6 +1999,7 @@ mod tests { TableDumpV2Type::RibIpv4Unicast as u16, rib.encode(), &mut empty_peer_table, + Arc::from([]), ) .unwrap(), ) @@ -1696,6 +2016,7 @@ mod tests { TableDumpV2Type::RibIpv4Unicast as u16, truncated.freeze(), &mut empty_peer_table, + Arc::from([]), ) .unwrap(), ) @@ -1731,6 +2052,7 @@ mod tests { TableDumpV2Type::RibIpv4UnicastAddPath as u16, add_path_truncated.freeze(), &mut peer_table, + Arc::from([]), ) .unwrap(), ) @@ -1752,6 +2074,7 @@ mod tests { TableDumpV2Type::RibIpv4Unicast as u16, rib_body, &mut peer_table, + Arc::from([]), ) .unwrap(), ) @@ -1772,9 +2095,7 @@ mod tests { fn route_iterators_preserve_table_dump_v2_routes_before_truncated_attribute_payload() { let (bytes, _rib_body, _peer_table) = table_dump_v2_truncated_attribute_payload(); - let routes = BgpkitParser::from_reader(Cursor::new(bytes.clone())) - .into_route_iter() - .collect::>(); + let routes = collect_route_batches(BgpkitParser::from_reader(Cursor::new(bytes.clone()))); assert_eq!(routes.len(), 1); assert_eq!( routes[0].prefix, @@ -1784,6 +2105,15 @@ mod tests { let fallible_routes = BgpkitParser::from_reader(Cursor::new(bytes)) .into_fallible_route_iter() .collect::, _>>() + .unwrap() + .into_iter() + .flat_map(|batch| { + batch + .routes() + .map(|route| route.map(BgpRouteElem::into_owned)) + .collect::>() + }) + .collect::, _>>() .unwrap(); assert_eq!(fallible_routes, routes); } @@ -1813,13 +2143,11 @@ mod tests { #[test] fn route_iterator_skips_route_parse_errors() { - let routes = BgpkitParser::from_reader(Cursor::new( + let routes = collect_route_batches(BgpkitParser::from_reader(Cursor::new( table_dump_v2_rib_without_peer_table_record() .encode() .to_vec(), - )) - .into_route_iter() - .collect::>(); + ))); assert!(routes.is_empty()); } @@ -1831,6 +2159,15 @@ mod tests { .unwrap() .into_fallible_route_iter() .collect::, _>>() + .unwrap() + .into_iter() + .flat_map(|batch| { + batch + .routes() + .map(|route| route.map(BgpRouteElem::into_owned)) + .collect::>() + }) + .collect::, _>>() .unwrap(); assert_eq!(routes.len(), 1); @@ -1855,6 +2192,15 @@ mod tests { let routes = BgpkitParser::from_reader(Cursor::new(bytes)) .into_fallible_route_iter() .collect::, _>>() + .unwrap() + .into_iter() + .flat_map(|batch| { + batch + .routes() + .map(|route| route.map(BgpRouteElem::into_owned)) + .collect::>() + }) + .collect::, _>>() .unwrap(); assert_eq!(routes.len(), 2); diff --git a/src/parser/utils.rs b/src/parser/utils.rs index e53fdcc..ea458af 100644 --- a/src/parser/utils.rs +++ b/src/parser/utils.rs @@ -179,7 +179,7 @@ pub trait ReadUtils: Buf { /// Parse NLRI prefix from byte slice without consuming bytes. /// Returns (prefix, bytes_consumed) on success. -fn try_parse_prefix( +pub(crate) fn try_parse_prefix( data: &[u8], afi: &Afi, add_path: bool, From 7ed3c7c4bb7cba28e1435ebb93ade6df14dc1fad Mon Sep 17 00:00:00 2001 From: Ties de Kock Date: Mon, 8 Jun 2026 20:58:37 +0200 Subject: [PATCH 2/3] format + clippy --- src/parser/iters/route.rs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/parser/iters/route.rs b/src/parser/iters/route.rs index ffed7d2..b57dff5 100644 --- a/src/parser/iters/route.rs +++ b/src/parser/iters/route.rs @@ -5,7 +5,9 @@ use crate::parser::bgp::messages::read_and_validate_bgp_marker; use crate::parser::iters::write_mrt_core_dump; use crate::parser::mrt::messages::bgp4mp::bgp4mp_message_payload_len; use crate::parser::mrt::messages::table_dump_v2::rib_entry_min_len; -use crate::parser::{chunk_mrt_record, try_parse_prefix, BgpkitParser, Filter, Filterable, ReadUtils}; +use crate::parser::{ + chunk_mrt_record, try_parse_prefix, BgpkitParser, Filter, Filterable, ReadUtils, +}; use bytes::{Buf, Bytes}; use ipnet::IpNet; use log::{debug, error, warn}; @@ -392,7 +394,10 @@ impl<'a> Iterator for RouteBatchIter<'a> { peer_ip: self.batch.peer_ip, peer_asn: self.batch.peer_asn, prefix, - as_path: part.has_as_path.then_some(()).and(self.batch.as_path.as_ref()), + as_path: part + .has_as_path + .then_some(()) + .and(self.batch.as_path.as_ref()), }; if !self.filtered || route.match_filters(&self.batch.filters) { @@ -718,7 +723,9 @@ fn parse_bgp4mp_routes( ))); } - parse_bgp_message_routes(data, add_path, &asn_len, timestamp, peer_ip, peer_asn, filters) + parse_bgp_message_routes( + data, add_path, &asn_len, timestamp, peer_ip, peer_asn, filters, + ) } fn table_dump_v2_afi_safi(rib_type: TableDumpV2Type) -> Result<(Afi, Safi), ParserError> { @@ -1443,10 +1450,7 @@ mod tests { .into_route_iter() .next() .unwrap(); - let routes = batch - .all_routes() - .collect::, _>>() - .unwrap(); + let routes = batch.all_routes().collect::, _>>().unwrap(); assert_eq!(routes.len(), 2); assert!(std::ptr::eq( From 4ba832623c840e53c3c4ce3b0e01a00e15f8a27a Mon Sep 17 00:00:00 2001 From: Ties de Kock Date: Mon, 8 Jun 2026 21:22:01 +0200 Subject: [PATCH 3/3] address copilot feedback w/ codex --- src/models/bgp/elem.rs | 4 ++ src/parser/iters/route.rs | 114 +++++++++++++++++++++++++++++++++++--- src/parser/utils.rs | 50 +++++++---------- 3 files changed, 130 insertions(+), 38 deletions(-) diff --git a/src/models/bgp/elem.rs b/src/models/bgp/elem.rs index 77a2bc8..d9b0c7f 100644 --- a/src/models/bgp/elem.rs +++ b/src/models/bgp/elem.rs @@ -163,6 +163,10 @@ pub struct BgpElem { /// peer metadata, timestamp, and AS path. Use [`BgpElem`] when you need the /// full set of BGP attributes. Because route elements do not carry /// communities, community filters do not match [`BgpRouteElem`] values. +/// +/// With the `serde` feature enabled, this borrowed type only supports +/// `Serialize`; use [`BgpRouteElemOwned`] when you need to `Deserialize` +/// route elements. #[derive(Debug, Clone, Copy, PartialEq)] #[cfg_attr(feature = "serde", derive(serde::Serialize))] pub struct BgpRouteElem<'a> { diff --git a/src/parser/iters/route.rs b/src/parser/iters/route.rs index b57dff5..b4f2fad 100644 --- a/src/parser/iters/route.rs +++ b/src/parser/iters/route.rs @@ -6,7 +6,8 @@ use crate::parser::iters::write_mrt_core_dump; use crate::parser::mrt::messages::bgp4mp::bgp4mp_message_payload_len; use crate::parser::mrt::messages::table_dump_v2::rib_entry_min_len; use crate::parser::{ - chunk_mrt_record, try_parse_prefix, BgpkitParser, Filter, Filterable, ReadUtils, + chunk_mrt_record, looks_like_zero_path_id_add_path, try_parse_prefix, BgpkitParser, Filter, + Filterable, ReadUtils, }; use bytes::{Buf, Bytes}; use ipnet::IpNet; @@ -76,14 +77,29 @@ fn parse_route_nlri_bytes( None => input.read_safi()?, }; - if afi == Afi::LinkState - || safi == Safi::LinkState - || safi == Safi::LinkStateVpn - || safi == Safi::MplsLabel - { + if afi == Afi::LinkState || safi == Safi::LinkState || safi == Safi::LinkStateVpn { return Ok(None); } + if safi == Safi::MplsLabel { + if reachable { + return Ok(None); + } + + let config = LabeledNlriConfig { + add_path, + mode: LabeledNlriMode::MultiLabel, + max_labels: 16, + peer_max_labels: None, + }; + let prefixes = parse_labeled_withdrawal_nlri(&mut input, afi, &config)?; + return Ok(Some(RouteNlriBytes { + data: encode_nlri_prefixes_without_alloc_api(&prefixes), + afi, + add_path, + })); + } + if reachable { let next_hop_length = input.read_u8()? as usize; input.has_n_remaining(next_hop_length)?; @@ -238,8 +254,8 @@ impl Iterator for NlriPrefixIter { } let data = self.input.as_ref(); - if !self.is_add_path && !data.is_empty() && data[0] == 0 { - debug!("NLRI: first byte is 0, treating as Add-Path format"); + if !self.is_add_path && looks_like_zero_path_id_add_path(data) { + debug!("NLRI: first 4 bytes are 0, treating as Add-Path format"); self.is_add_path = true; self.use_heuristic = true; } @@ -1430,6 +1446,88 @@ mod tests { assert!(routes[1].as_path.is_none()); } + #[test] + fn route_iterator_does_not_treat_default_route_as_add_path() { + let mut update = BytesMut::new(); + update.put_u16(0); + update.put_u16(0); + update.put_u8(0); + update.put_u8(32); + update.extend_from_slice(&[1, 2, 3, 4]); + + let batch = parse_bgp_update_routes( + update.freeze(), + false, + &AsnLength::Bits32, + 1_700_000_000.0, + "192.0.2.1".parse().unwrap(), + Asn::new_32bit(64496), + Arc::from([]), + ) + .unwrap(); + let routes = batch + .all_routes() + .map(|route| route.map(BgpRouteElem::into_owned)) + .collect::, _>>() + .unwrap(); + + assert_eq!(routes.len(), 2); + assert_eq!( + routes[0].prefix, + NetworkPrefix::from_str("0.0.0.0/0").unwrap() + ); + assert_eq!( + routes[1].prefix, + NetworkPrefix::from_str("1.2.3.4/32").unwrap() + ); + assert!(routes.iter().all(|route| route.prefix.path_id.is_none())); + } + + #[test] + fn route_iterator_emits_mpls_labeled_mp_unreach_withdrawals() { + let mut mp_unreach = BytesMut::new(); + mp_unreach.put_u16(Afi::Ipv4 as u16); + mp_unreach.put_u8(Safi::MplsLabel as u8); + mp_unreach.put_u8(48); + mp_unreach.extend_from_slice(&[0, 0, 0]); + mp_unreach.extend_from_slice(&[203, 0, 113]); + + let mut attr = BytesMut::new(); + attr.put_u8(AttrFlags::OPTIONAL.bits()); + attr.put_u8(u8::from(AttrType::MP_UNREACHABLE_NLRI)); + attr.put_u8(mp_unreach.len() as u8); + attr.extend_from_slice(&mp_unreach); + + let mut update = BytesMut::new(); + update.put_u16(0); + update.put_u16(attr.len() as u16); + update.extend_from_slice(&attr); + + let batch = parse_bgp_update_routes( + update.freeze(), + false, + &AsnLength::Bits32, + 1_700_000_000.0, + "192.0.2.1".parse().unwrap(), + Asn::new_32bit(64496), + Arc::from([]), + ) + .unwrap(); + let routes = batch + .all_routes() + .map(|route| route.map(BgpRouteElem::into_owned)) + .collect::, _>>() + .unwrap(); + + assert_eq!(routes.len(), 1); + assert_eq!(routes[0].elem_type, ElemType::WITHDRAW); + assert_eq!( + routes[0].prefix, + NetworkPrefix::from_str("203.0.113.0/24").unwrap() + ); + assert!(routes[0].as_path.is_none()); + } + #[test] fn route_iterator_shares_as_path_for_update_announcements() { let bytes = bgp4mp_record( diff --git a/src/parser/utils.rs b/src/parser/utils.rs index ea458af..60b56b0 100644 --- a/src/parser/utils.rs +++ b/src/parser/utils.rs @@ -270,6 +270,10 @@ pub(crate) fn try_parse_prefix( Ok((NetworkPrefix::new(prefix, path_id), pos)) } +pub(crate) fn looks_like_zero_path_id_add_path(data: &[u8]) -> bool { + data.starts_with(&[0, 0, 0, 0]) +} + pub fn parse_nlri_list( mut input: Bytes, add_path: bool, @@ -282,9 +286,9 @@ pub fn parse_nlri_list( while input.remaining() > 0 { let data = input.as_ref(); - // Check heuristic: if not add_path and first byte is 0, likely Add-Path format - if !is_add_path && !data.is_empty() && data[0] == 0 { - debug!("NLRI: first byte is 0, treating as Add-Path format"); + // Check heuristic: a zero path_id can indicate Add-Path format. + if !is_add_path && looks_like_zero_path_id_add_path(data) { + debug!("NLRI: first 4 bytes are 0, treating as Add-Path format"); is_add_path = true; use_heuristic = true; } @@ -432,6 +436,7 @@ impl ComparableRegex { mod tests { use super::*; use bytes::Bytes; + use std::str::FromStr; #[test] fn test_read_u8() { @@ -815,11 +820,11 @@ mod tests { ]; assert_eq!(parse_nlri_list(input, true, &Afi::Ipv4).unwrap(), expected); - // Test the auto-detection of add_path when first byte is 0 - let input = Bytes::from_static(&[0x00, 0x00, 0x00, 0x01, 0x18, 0xC0, 0xA8, 0x01]); + // Test the auto-detection of Add-Path when the path ID is 0. + let input = Bytes::from_static(&[0x00, 0x00, 0x00, 0x00, 0x18, 0xC0, 0xA8, 0x01]); let expected = vec![NetworkPrefix::new( IpNet::V4(Ipv4Net::new(Ipv4Addr::new(192, 168, 1, 0), 24).unwrap()), - Some(1), + Some(0), )]; assert_eq!(parse_nlri_list(input, false, &Afi::Ipv4).unwrap(), expected); } @@ -874,35 +879,20 @@ mod tests { #[test] fn test_parse_nlri_list_non_addpath_with_zero_prefix() { - // Edge case: Non-Add-Path NLRI where the first prefix byte happens to be 0 - // This could trigger a false positive in the heuristic - // Example: /0 default route would start with byte 0 - - // Non-Add-Path NLRI for 0.0.0.0/0 (default route) - // Format: [prefix_len, prefix_bytes...] + // Regression: a non-Add-Path /0 followed by another prefix must not be + // misdetected as a zero-path-ID Add-Path prefix. let input = Bytes::from(vec![ 0x00, // prefix_len = 0 (default route) - // No prefix bytes needed for /0 + 0x20, // prefix_len = 32 bits + 0x01, 0x02, 0x03, 0x04, ]); - // This might trigger the heuristic (first byte is 0) - // The optimized code should handle the retry correctly - let result = parse_nlri_list(input, false, &Afi::Ipv4); + let prefixes = parse_nlri_list(input, false, &Afi::Ipv4).unwrap(); - // Should either parse successfully or return an error - // The key point is it shouldn't panic - match result { - Ok(prefixes) => { - // If parsed, should be the default route - if !prefixes.is_empty() { - assert_eq!(prefixes[0].prefix.prefix_len(), 0); - } - } - Err(_) => { - // Error is acceptable - the heuristic might fail on this edge case - // The important thing is we don't panic - } - } + assert_eq!(prefixes.len(), 2); + assert_eq!(prefixes[0], NetworkPrefix::from_str("0.0.0.0/0").unwrap()); + assert_eq!(prefixes[1], NetworkPrefix::from_str("1.2.3.4/32").unwrap()); + assert!(prefixes.iter().all(|prefix| prefix.path_id.is_none())); } #[test]