diff --git a/daemon/src/convert.rs b/daemon/src/convert.rs index 9d471c56..fd10a95d 100644 --- a/daemon/src/convert.rs +++ b/daemon/src/convert.rs @@ -4289,6 +4289,41 @@ pub(crate) struct PathBinaryFlags { pub only_binary: bool, } +/// NEXT_HOP / MP_REACH are stored separately from attributes in the RIB. +/// Rebuild them for ListPath, including the NLRI inside MP_REACH. +fn path_nexthop_attribute( + net: &Nlri, + family: Family, + nexthop: Option, +) -> Option { + if family == Family::IPV4 && matches!(nexthop, Some(packet::bgp::Nexthop::V4(_))) { + return Attribute::new_with_bin(Attribute::NEXTHOP, nexthop.unwrap().to_bytes()); + } + if family == Family::IPV4 && nexthop.is_none() { + return None; + } + let mut nh = nexthop.map(|n| n.to_bytes()).unwrap_or_default(); + if matches!( + family, + Family::IPV4_FLOWSPEC + | Family::IPV6_FLOWSPEC + | Family::IPV4_FLOWSPEC_VPN + | Family::IPV6_FLOWSPEC_VPN + ) { + nh.clear(); + } else if matches!(family, Family::IPV4_VPN | Family::IPV6_VPN) { + let mut vpn_nh = vec![0; 8]; + vpn_nh.extend(nh); + nh = vpn_nh; + } + let mut body = family.afi().to_be_bytes().to_vec(); + body.extend([family.safi(), nh.len() as u8]); + body.extend(nh); + body.push(0); + body.extend(net.encode_to_bytes()); + Attribute::new_with_bin(Attribute::MP_REACH, body) +} + pub(crate) fn destination_to_api( d: rustybgp_table::DestinationEntry, family: Family, @@ -4299,37 +4334,69 @@ pub(crate) fn destination_to_api( paths: d .paths .into_iter() - .map(|p| api::Path { - nlri: if binary.only_binary { - None - } else { - Some(nlri_to_api(&d.net)) - }, - family: Some(family_to_api(family)), - identifier: p.remote_path_id, - age: Some(prost_types::Timestamp { - seconds: p.timestamp as i64, - nanos: 0, - }), - pattrs: if binary.only_binary { - vec![] - } else { - p.attr.iter().map(attr_to_api).collect() - }, - validation: p.validation.map(rpki_validation_to_api), - stale: p.stale, - filtered: p.filtered, - nlri_binary: if binary.nlri_binary { - d.net.encode_to_bytes() - } else { + .map(|p| { + let nh_attr = path_nexthop_attribute(&d.net, family, p.nexthop); + let attrs = p + .attr + .iter() + .filter(|a| !matches!(a.code(), Attribute::NEXTHOP | Attribute::MP_REACH)) + .chain(nh_attr.iter()); + let pattrs = if binary.only_binary { vec![] - }, - pattrs_binary: if binary.attr_binary { - p.attr.iter().map(|a| a.encode_to_bytes()).collect() } else { - vec![] - }, - ..Default::default() + attrs + .clone() + .map(|a| { + if a.code() != Attribute::MP_REACH { + return attr_to_api(a); + } + let next_hops = match p.nexthop { + Some(packet::bgp::Nexthop::V6LinkLocal(global, local)) => { + vec![global.to_string(), local.to_string()] + } + Some(nh) => vec![nh.addr().to_string()], + None => vec![], + }; + api::Attribute { + attr: Some(api::attribute::Attr::MpReach( + api::MpReachNlriAttribute { + family: Some(family_to_api(family)), + next_hops, + nlris: vec![nlri_to_api(&d.net)], + }, + )), + } + }) + .collect() + }; + api::Path { + nlri: if binary.only_binary { + None + } else { + Some(nlri_to_api(&d.net)) + }, + family: Some(family_to_api(family)), + identifier: p.remote_path_id, + age: Some(prost_types::Timestamp { + seconds: p.timestamp as i64, + nanos: 0, + }), + pattrs, + validation: p.validation.map(rpki_validation_to_api), + stale: p.stale, + filtered: p.filtered, + nlri_binary: if binary.nlri_binary { + d.net.encode_to_bytes() + } else { + vec![] + }, + pattrs_binary: if binary.attr_binary { + attrs.map(|a| a.encode_to_bytes()).collect() + } else { + vec![] + }, + ..Default::default() + } }) .collect(), } diff --git a/daemon/src/event/export.rs b/daemon/src/event/export.rs index 9db98f26..038a6d7f 100644 --- a/daemon/src/event/export.rs +++ b/daemon/src/event/export.rs @@ -505,12 +505,13 @@ impl NlriSink for AdjOutSink { _dest_id: u32, nlri: packet::Nlri, _path_id: u32, - _nexthop: Option, + nexthop: Option, attr: Arc>, source: &Arc, ) { let entry = table::PathEntry { source: Arc::clone(source), + nexthop, remote_path_id: 0, timestamp: 0u32, attr, diff --git a/daemon/src/event/mod.rs b/daemon/src/event/mod.rs index 14ff9972..014667c1 100644 --- a/daemon/src/event/mod.rs +++ b/daemon/src/event/mod.rs @@ -9278,6 +9278,108 @@ mod tests { } } + #[tokio::test] + async fn list_path_adj_in_restores_structured_and_binary_nexthops() { + let svc = make_grpc_service(); + let peer: IpAddr = "10.0.0.2".parse().unwrap(); + let source = Arc::new(table::Source::new( + peer, + "10.0.0.1".parse().unwrap(), + 65002, + 65001, + Ipv4Addr::new(10, 0, 0, 2), + PeerRole::Ebgp, + )); + let cases = [ + (Family::IPV4, "198.51.100.0/24", "192.0.2.1"), + (Family::IPV6, "2001:db8:1::/48", "2001:db8::1"), + ]; + for (family, prefix, nexthop) in cases { + let net: packet::Nlri = prefix.parse().unwrap(); + let nh = if family == Family::IPV4 { + bgp::Nexthop::V4(nexthop.parse().unwrap()) + } else { + bgp::Nexthop::V6(nexthop.parse().unwrap()) + }; + assert!(!svc.tables.insert_route( + source.clone(), + family, + packet::PathNlri { + nlri: net.clone(), + path_id: 0, + }, + Some(nh), + Arc::new(vec![ + packet::Attribute::new_with_value(packet::Attribute::ORIGIN, 0).unwrap(), + ]), + None, + 0, + )); + + let request = api::ListPathRequest { + table_type: api::TableType::AdjIn as i32, + name: peer.to_string(), + family: Some(convert::family_to_api(family)), + enable_nlri_binary: true, + enable_attribute_binary: true, + ..Default::default() + }; + let mut stream = svc + .list_path(tonic::Request::new(request.clone())) + .await + .unwrap() + .into_inner(); + let destination = stream.next().await.unwrap().unwrap().destination.unwrap(); + assert!(stream.next().await.is_none()); + let path = &destination.paths[0]; + assert_eq!(path.nlri_binary, net.encode_to_bytes()); + assert!(path.nlri.is_some()); + if family == Family::IPV4 { + assert!(path.pattrs.iter().any(|a| matches!( + &a.attr, + Some(api::attribute::Attr::NextHop(nh)) if nh.next_hop == nexthop + ))); + let mut expected = vec![0x40, packet::Attribute::NEXTHOP, 4]; + expected.extend(nexthop.parse::().unwrap().octets()); + assert!(path.pattrs_binary.contains(&expected)); + } else { + assert!(path.pattrs.iter().any(|a| matches!( + &a.attr, + Some(api::attribute::Attr::MpReach(mp)) + if mp.next_hops == [nexthop] && mp.nlris.len() == 1 + ))); + assert!(path.pattrs_binary.iter().any(|bytes| { + bytes.starts_with(&[0x80, packet::Attribute::MP_REACH]) + && bytes.get(3..7) == Some(&[0, 2, 1, 16][..]) + && bytes.get(7..23) + == Some(&nexthop.parse::().unwrap().octets()[..]) + })); + } + + let mut only_binary = request; + only_binary.enable_nlri_binary = false; + only_binary.enable_attribute_binary = false; + only_binary.enable_only_binary = true; + let mut stream = svc + .list_path(tonic::Request::new(only_binary)) + .await + .unwrap() + .into_inner(); + let path = &stream + .next() + .await + .unwrap() + .unwrap() + .destination + .unwrap() + .paths[0]; + assert!(path.nlri.is_none()); + assert!(path.pattrs.is_empty()); + assert_eq!(path.nlri_binary, net.encode_to_bytes()); + assert!(!path.pattrs_binary.is_empty()); + } + } + #[tokio::test] async fn add_path_inserts_route() { let svc = make_grpc_service(); diff --git a/daemon/src/table_manager.rs b/daemon/src/table_manager.rs index d0dbce61..131e5525 100644 --- a/daemon/src/table_manager.rs +++ b/daemon/src/table_manager.rs @@ -365,7 +365,10 @@ impl TableManager { net.path_id, nh, post_policy_attr, - Some(original_attr), + Some(table::OriginalPath { + attr: original_attr, + nexthop, + }), filtered, nexthop_invalid_flag, pl, @@ -1279,6 +1282,7 @@ impl TableShard { ) { let paths = self.rtable.collect_adj_in_paths(peer, None, false); for (family, net, remote_path_id, mut nh, source, original_attr, timestamp) in paths { + let original_nexthop = nh; let old_nh = self.rtable .lookup_nexthop(source.remote_addr, family, &net, remote_path_id); @@ -1333,7 +1337,10 @@ impl TableShard { remote_path_id, nh, post_policy_attr, - Some(original_attr), + Some(table::OriginalPath { + attr: original_attr, + nexthop: original_nexthop, + }), filtered, nexthop_invalid_flag, None, diff --git a/table/src/lib.rs b/table/src/lib.rs index af546312..48af1321 100644 --- a/table/src/lib.rs +++ b/table/src/lib.rs @@ -172,10 +172,11 @@ pub struct DestinationEntry { /// for gRPC `ListPath` responses and similar inspection APIs. /// /// Contains display fields (timestamp, RPKI validation, policy state) that are -/// not needed for route distribution. The nexthop is intentionally absent; it -/// is embedded in the serialised UPDATE attributes for the API response. +/// not needed for route distribution. The nexthop is kept separately from the +/// attributes in the RIB and must be restored in API responses. pub struct PathEntry { pub source: Arc, + pub nexthop: Option, /// AddPath path identifier received from the peer (0 when AddPath is not in use). /// AddPath path identifier received from the peer (0 when AddPath is not in use). pub remote_path_id: u32, @@ -279,11 +280,19 @@ struct RibEntry { /// allocation as `path.attr`, so storing it costs only a reference-count /// increment. original_attr: Arc>, + /// Pre-import-policy nexthop, which may differ from `path.nexthop`. + original_nexthop: Option, remote_path_id: u32, timestamp: u32, flags: u8, } +/// Pre-import-policy path data kept for Adj-RIB-In and policy re-evaluation. +pub struct OriginalPath { + pub attr: Arc>, + pub nexthop: Option, +} + /// Returns true if `attrs` contains the LLGR_STALE well-known community (0xFFFF0006). fn has_llgr_stale_community(attrs: &[packet::Attribute]) -> bool { const LLGR_STALE: u32 = 0xffff_0006; @@ -979,7 +988,7 @@ impl Table { path_id: e.remote_path_id, }, attr: e.original_attr.clone(), - nexthop: e.path.nexthop, + nexthop: e.original_nexthop, timestamp: e.timestamp, }) }) @@ -1058,7 +1067,7 @@ impl Table { *fam, net.clone(), entry.remote_path_id, - entry.path.nexthop, + entry.original_nexthop, Arc::clone(&entry.path.source), Arc::clone(&entry.original_attr), entry.timestamp, @@ -1080,6 +1089,7 @@ impl Table { .filter(|p| enable_filtered || !p.is_filtered()) .map(|p| PathEntry { source: p.path.source.clone(), + nexthop: p.path.nexthop, remote_path_id: p.remote_path_id, timestamp: p.timestamp, attr: p.path.attr.clone(), @@ -1102,6 +1112,7 @@ impl Table { .filter(|p| enable_filtered || !p.is_filtered()) .map(|p| PathEntry { source: p.path.source.clone(), + nexthop: p.original_nexthop, remote_path_id: p.remote_path_id, timestamp: p.timestamp, attr: p.original_attr.clone(), @@ -1130,6 +1141,7 @@ impl Table { best.into_iter() .map(|p| PathEntry { source: p.path.source.clone(), + nexthop: p.original_nexthop, remote_path_id: 0, timestamp: p.timestamp, attr: p.original_attr.clone(), @@ -1190,7 +1202,7 @@ impl Table { remote_id: u32, nexthop: Option, attr: Arc>, - original_attr: Option>>, + original: Option, filtered: bool, nexthop_invalid: bool, prefix_limit: Option<(u32, &Arc)>, @@ -1270,7 +1282,10 @@ impl Table { .as_ref() .map_or_else(|| dst.alloc_path_id(), |old| old.path.local_path_id); - let original_attr = original_attr.unwrap_or_else(|| Arc::clone(&attr)); + let original = original.unwrap_or_else(|| OriginalPath { + attr: Arc::clone(&attr), + nexthop, + }); let entry = RibEntry { path: Path { @@ -1279,7 +1294,8 @@ impl Table { nexthop, attr, }, - original_attr, + original_attr: original.attr, + original_nexthop: original.nexthop, remote_path_id: remote_id, timestamp, flags, @@ -5633,7 +5649,10 @@ mod tests { 0, nh(), post_policy.clone(), - Some(original.clone()), + Some(OriginalPath { + attr: original.clone(), + nexthop: nh(), + }), false, false, None, @@ -5692,6 +5711,56 @@ mod tests { assert_eq!(adj_in[0].paths[0].attr, attr); } + #[test] + fn adj_in_preserves_received_nexthop_after_import_rewrite() { + let source = source(1, 65001, 65000, 1); + let original_nexthop = Some(bgp::Nexthop::V4(Ipv4Addr::new(192, 0, 2, 1))); + let rewritten_nexthop = Some(bgp::Nexthop::V4(Ipv4Addr::new(198, 51, 100, 1))); + let mut table = Table::new(0); + table.insert( + source.clone(), + Family::IPV4, + nlri(203, 0, 113, 0, 24), + 0, + rewritten_nexthop, + empty_attrs(), + Some(OriginalPath { + attr: empty_attrs(), + nexthop: original_nexthop, + }), + false, + false, + None, + 0, + ); + + let global: Vec<_> = table + .destinations(TableQuery::Global, Family::IPV4, vec![], false) + .collect(); + let adj_in: Vec<_> = table + .destinations( + TableQuery::AdjIn(source.remote_addr), + Family::IPV4, + vec![], + false, + ) + .collect(); + assert_eq!(global[0].paths[0].nexthop, rewritten_nexthop); + assert_eq!(adj_in[0].paths[0].nexthop, original_nexthop); + assert_eq!( + table.iter_reach(Family::IPV4).next().unwrap().nexthop, + original_nexthop + ); + assert_eq!( + table.iter_reach_post(Family::IPV4).next().unwrap().nexthop, + rewritten_nexthop + ); + assert_eq!( + table.collect_adj_in_paths(source.remote_addr, Some(Family::IPV4), false)[0].3, + original_nexthop + ); + } + #[test] fn iter_reach_returns_original_attr() { // iter_reach() is used by BMP RouteMonitoring and must carry pre-policy attrs. @@ -5708,7 +5777,10 @@ mod tests { 0, nh(), post_policy, - Some(original.clone()), + Some(OriginalPath { + attr: original.clone(), + nexthop: nh(), + }), false, false, None, @@ -5915,7 +5987,10 @@ mod tests { 0, nh(), post_import.clone(), - Some(original.clone()), + Some(OriginalPath { + attr: original.clone(), + nexthop: nh(), + }), false, false, None,