diff --git a/crates/bench/benches/full_table.rs b/crates/bench/benches/full_table.rs index 4c55887..b7a13b0 100644 --- a/crates/bench/benches/full_table.rs +++ b/crates/bench/benches/full_table.rs @@ -185,7 +185,7 @@ fn into_table(wire: &[Vec], routes: usize, threads: usize) { let wire = decoded.attributes_wire.as_deref().expect("kept"); if !update.announced_routes.is_empty() { let attrs = table.attrs().intern_received(wire, None).expect("valid"); - table.announce(peers[at], &attrs, &update.announced_routes); + table.announce_paths(peers[at], &attrs, &update.announced_routes); } for attribute in &update.path_attributes { if let PathAttribute::MpReachNlri(mp) = attribute { @@ -193,7 +193,7 @@ fn into_table(wire: &[Vec], routes: usize, threads: usize) { .attrs() .intern_received(wire, Some(&mp.next_hop)) .expect("valid"); - table.announce(peers[at], &attrs, &mp.prefixes); + table.announce_paths(peers[at], &attrs, &mp.prefixes); } } } diff --git a/crates/bench/benches/real_table.rs b/crates/bench/benches/real_table.rs index c357cd8..5c98fe8 100644 --- a/crates/bench/benches/real_table.rs +++ b/crates/bench/benches/real_table.rs @@ -214,9 +214,9 @@ fn sharded_table(dump: &Dump, peers: &[usize], parsed: &[Option], th let started = Instant::now(); let mut all = 0usize; let mut hops = 0u64; - table.for_each_route(|_, _, attrs, _| { + table.for_each_route(|route| { all += 1; - hops += u64::from(attrs.key().as_path_len); + hops += u64::from(route.attrs.key().as_path_len); }); row( "read every route and its attributes", @@ -345,6 +345,8 @@ fn followed_table(dump: &Dump, peers: &[usize], parsed: &[Option], t } Event::Withdrawn { .. } => counts.1 += 1, Event::Reset | Event::Synced => counts.2 += 1, + // Not given to a follower of the best routes. + Event::Routes { .. } => {} }; // Reading while the table fills. diff --git a/crates/bench/src/lib.rs b/crates/bench/src/lib.rs index f074a99..bbe4854 100644 --- a/crates/bench/src/lib.rs +++ b/crates/bench/src/lib.rs @@ -25,7 +25,7 @@ use bgp_packet::message::{ NextHopPathAttribute, NlriNextHop, OriginPathAttribute, PathAttribute, UpdateMessage, }; use bgp_primitives::constants::{AddressFamilyId, Afi, Safi}; -use bgp_primitives::ip_prefix::IpPrefix; +use bgp_primitives::ip_prefix::{IpPrefix, Nlri}; use bgp_primitives::parser::ParserContext; use bytes::BytesMut; @@ -231,7 +231,7 @@ pub fn announcements(prefixes: &Prefixes, shape: Shape, peer: u32) -> Vec Vec ParserContext { ParserContext { four_octet_asn: Some(true), ibgp: false, + add_path: Vec::new(), } } diff --git a/crates/integration-tests/examples/fake_peer.rs b/crates/integration-tests/examples/fake_peer.rs index ac38be2..d986fa0 100644 --- a/crates/integration-tests/examples/fake_peer.rs +++ b/crates/integration-tests/examples/fake_peer.rs @@ -135,7 +135,7 @@ fn announce(prefix: IpPrefix, as_path: Vec, communities: Vec<(u16, u16)>) - UpdateMessage { withdrawn_routes: vec![], path_attributes, - announced_routes: vec![prefix], + announced_routes: vec![prefix.into()], } } @@ -155,9 +155,10 @@ async fn echo_back(peer: &mut TestPeer, u: &UpdateMessage, asn: u32) -> Result<( println!(" not echoing: NO_EXPORT"); return Ok(()); } - for prefix in &u.announced_routes { + for nlri in &u.announced_routes { + let prefix = nlri.prefix; peer.send(&Message::Update(announce( - *prefix, + prefix, as_path.clone(), communities.clone(), ))) diff --git a/crates/integration-tests/src/test_peer.rs b/crates/integration-tests/src/test_peer.rs index 1987ffc..d17db01 100644 --- a/crates/integration-tests/src/test_peer.rs +++ b/crates/integration-tests/src/test_peer.rs @@ -2,7 +2,7 @@ use std::net::{Ipv4Addr, SocketAddr}; use bgp_packet::message::{BGP_HEADER_LEN, Message, UpdateMessage}; use bgp_packet::open::{Capability, OpenMessage, OpenOption}; -use bgp_primitives::constants::{Afi, Safi}; +use bgp_primitives::constants::{Afi, AfiSafi, Safi}; use bgp_primitives::parser::ParserContext; use bytes::BytesMut; use eyre::{Result, bail, eyre}; @@ -14,6 +14,11 @@ use tokio::net::TcpStream; pub struct TestPeer { stream: TcpStream, pub ctx: ParserContext, + /// The address families for which this peer sends path identifiers + /// with its prefixes (ADD-PATH, RFC 7911). What it expects to be sent + /// is `ctx.add_path`. A test sets both to match the capabilities it + /// exchanged. + pub send_add_path: Vec, /// Whether the OPEN we sent offered four-octet AS numbers. They are in /// use only if the server's did too. offered_four_octet_asn: bool, @@ -31,14 +36,19 @@ impl TestPeer { Self { stream, ctx: ParserContext::default(), + send_add_path: Vec::new(), offered_four_octet_asn: true, } } /// Send a BGP message. pub async fn send(&mut self, msg: &Message) -> Result<()> { + let send_ctx = ParserContext { + add_path: self.send_add_path.clone(), + ..self.ctx.clone() + }; let mut buf = BytesMut::new(); - msg.to_wire(&self.ctx, &mut buf)?; + msg.to_wire(&send_ctx, &mut buf)?; self.stream.write_all(&buf).await?; Ok(()) } diff --git a/crates/integration-tests/src/test_server.rs b/crates/integration-tests/src/test_server.rs index fa01299..5765ff6 100644 --- a/crates/integration-tests/src/test_server.rs +++ b/crates/integration-tests/src/test_server.rs @@ -83,6 +83,8 @@ impl TestServer { pub struct ReceivedRoute { pub prefix: IpPrefix, pub peer: PeerIndex, + /// The peer's identifier for the route; zero without ADD-PATH. + pub path_id: u32, /// The route's attributes, without MP_REACH_NLRI. pub attributes: Vec, /// The next hop, if the route came in MP_REACH_NLRI. @@ -127,6 +129,8 @@ impl RouteChanges { pending.push_back(RouteChange::Announced { prefix, peer }) } Event::Withdrawn { prefix } => pending.push_back(RouteChange::Withdrawn { prefix }), + // Not given to a follower of the best routes. + Event::Routes { .. } => {} Event::Reset | Event::Synced => {} }; while !self.reader.poll(1024, &mut read).caught_up {} @@ -137,14 +141,15 @@ impl TestServer { /// Every route in the server's table. pub fn received_routes(&self) -> Vec { let mut routes = Vec::new(); - self.rib.for_each_route(|prefix, peer, attrs, best| { - let (attributes, next_hop) = attrs.attributes(); + self.rib.for_each_route(|route| { + let (attributes, next_hop) = route.attrs.attributes(); routes.push(ReceivedRoute { - prefix: *prefix, - peer, + prefix: route.prefix, + peer: route.peer, + path_id: route.path_id, attributes, next_hop, - best, + best: route.best, }); }); routes diff --git a/crates/integration-tests/tests/add_path.rs b/crates/integration-tests/tests/add_path.rs new file mode 100644 index 0000000..55d175f --- /dev/null +++ b/crates/integration-tests/tests/add_path.rs @@ -0,0 +1,288 @@ +//! End-to-end tests of ADD-PATH (RFC 7911): a peer sending several routes +//! for one prefix, told apart by path identifiers. + +use std::net::{Ipv4Addr, SocketAddr}; +use std::time::Duration; + +use bgp_integration_tests::test_peer::TestPeer; +use bgp_integration_tests::test_server::TestServer; +use bgp_packet::message::{ + AsPathAttribute, Message, NextHopPathAttribute, OriginPathAttribute, PathAttribute, + UpdateMessage, +}; +use bgp_packet::open::{AddPathEntry, AddPathSendRecv, Capability, OpenMessage}; +use bgp_primitives::constants::AfiSafi; +use bgp_primitives::ip_prefix::{IpPrefix, Nlri}; +use bgp_rib::{LocalRoute, LocalRouteAttrs}; +use bgp_server::config::PeerConfig; +use bgp_server::family::{FamilySet, SUPPORTED}; +use eyre::Result; +use serde_json::Value; + +const LOCAL_ASN: u32 = 65000; +const PEER_ASN: u32 = 65001; +const TIMEOUT: Duration = Duration::from_secs(5); + +fn prefix(s: &str) -> IpPrefix { + IpPrefix::try_from(s).unwrap() +} + +/// A server on which connections from this machine are those of a +/// configured peer, with ADD-PATH offered to it as `add_path` says. +async fn server_offering( + add_path: Option, + originate: &[&str], +) -> Result { + let originate: Vec = originate + .iter() + .map(|p| LocalRoute::new(prefix(p), LocalRouteAttrs::default())) + .collect(); + TestServer::start_with(LOCAL_ASN, |c| { + c.peers = vec![PeerConfig { + name: "route-server-client".into(), + endpoint: SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 1), + remote_asn: PEER_ASN, + hold_time_secs: 90, + local_address: None, + families: FamilySet::supported(), + add_path, + }]; + c.originate = originate; + // Keep the dialer out of the way. + c.connect_retry = Duration::from_secs(3600); + }) + .await +} + +/// A session from a peer whose OPEN offers ADD-PATH as `offer` says, for +/// IPv4 unicast. Returns the peer and the server's OPEN. +async fn peer_offering( + server: &TestServer, + offer: Option, +) -> Result<(TestPeer, OpenMessage)> { + let mut capabilities = vec![Capability::FourByteAsn { asn: PEER_ASN }]; + capabilities.extend(SUPPORTED.into_iter().map(Capability::multiprotocol)); + if let Some(send_recv) = offer { + capabilities.push(Capability::AddPath { + entries: vec![AddPathEntry { + afi: AfiSafi::IPV4_UNICAST.afi, + safi: AfiSafi::IPV4_UNICAST.safi, + send_recv, + }], + }); + } + let mut peer = TestPeer::connect(server.addr).await?; + let open = peer + .handshake_with(PEER_ASN, Ipv4Addr::new(10, 0, 0, 1), 90, capabilities) + .await?; + tokio::time::timeout(TIMEOUT, async { + while server.registry.list().is_empty() { + tokio::time::sleep(Duration::from_millis(5)).await; + } + }) + .await?; + Ok((peer, open)) +} + +fn add_path_offered(open: &OpenMessage) -> Vec<(AfiSafi, AddPathSendRecv)> { + open.capabilities() + .filter_map(|capability| match capability { + Capability::AddPath { entries } => Some(entries), + _ => None, + }) + .flatten() + .map(|entry| (entry.family(), entry.send_recv)) + .collect() +} + +fn announcement(nlri: Nlri, as_path: &[u32]) -> Message { + Message::Update(UpdateMessage { + withdrawn_routes: vec![], + path_attributes: vec![ + PathAttribute::Origin(OriginPathAttribute::IGP), + AsPathAttribute::from_asns(as_path.to_vec()), + PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::new(192, 0, 2, 1))), + ], + announced_routes: vec![nlri], + }) +} + +async fn wait_for_routes(server: &TestServer, routes: usize) -> Result<()> { + tokio::time::timeout(TIMEOUT, async { + while server.rib.len() != routes { + tokio::time::sleep(Duration::from_millis(5)).await; + } + }) + .await?; + Ok(()) +} + +/// The routes the server holds, as (path identifier, AS path, best). +fn held(server: &TestServer) -> Vec<(u32, Vec, bool)> { + let mut held: Vec<(u32, Vec, bool)> = server + .received_routes() + .into_iter() + .map(|route| { + let path = route + .attributes + .iter() + .find_map(|attribute| match attribute { + PathAttribute::AsPath(path) => Some(path.segments[0].path.clone()), + _ => None, + }) + .unwrap_or_default(); + (route.path_id, path, route.best) + }) + .collect(); + held.sort(); + held +} + +/// A peer that sends several routes for one prefix: each is kept, the best +/// of them is chosen, and each is withdrawn on its own. +#[tokio::test] +async fn test_several_routes_for_a_prefix_from_one_peer() -> Result<()> { + let server = server_offering(Some(AddPathSendRecv::Receive), &[]).await?; + let (base, _http) = server.serve_http().await?; + let (mut peer, open) = peer_offering(&server, Some(AddPathSendRecv::Send)).await?; + + // The server offered to receive, for every family it offers. + assert_eq!( + add_path_offered(&open), + SUPPORTED.map(|family| (family, AddPathSendRecv::Receive)) + ); + // In use for the one family the peer sends for, and in that direction. + let session = &server.registry.list()[0]; + assert_eq!( + session.add_path_receive, + [AfiSafi::IPV4_UNICAST].into_iter().collect() + ); + assert!(session.add_path_send.is_empty()); + peer.send_add_path = vec![AfiSafi::IPV4_UNICAST]; + + let p = prefix("203.0.113.0/24"); + let path = |path_id| Nlri { prefix: p, path_id }; + peer.send(&announcement(path(1), &[PEER_ASN, 64999, 64998])) + .await?; + peer.send(&announcement(path(2), &[PEER_ASN, 64998])) + .await?; + wait_for_routes(&server, 2).await?; + assert_eq!( + held(&server), + [ + (1, vec![PEER_ASN, 64999, 64998], false), + (2, vec![PEER_ASN, 64998], true), + ] + ); + assert_eq!(server.rib.prefix_count(), 1); + + // The API tells them apart too. + let routes: Value = reqwest::get(format!("{base}/api/routes/received")) + .await? + .json() + .await?; + let mut ids: Vec = routes + .as_array() + .unwrap() + .iter() + .map(|route| route["path_id"].as_u64().unwrap()) + .collect(); + ids.sort_unstable(); + assert_eq!(ids, [1, 2]); + let peers: Value = reqwest::get(format!("{base}/api/peers")) + .await? + .json() + .await?; + assert_eq!( + peers[0]["add_path_receive"], + serde_json::json!(["IPv4 unicast"]) + ); + + // Announcing a path again replaces that path only. + peer.send(&announcement(path(1), &[PEER_ASN])).await?; + tokio::time::timeout(TIMEOUT, async { + while held(&server)[0].1.len() != 1 { + tokio::time::sleep(Duration::from_millis(5)).await; + } + }) + .await?; + assert_eq!( + held(&server), + [(1, vec![PEER_ASN], true), (2, vec![PEER_ASN, 64998], false),] + ); + + // And withdrawing one leaves the other. + peer.send(&Message::Update(UpdateMessage { + withdrawn_routes: vec![path(1)], + path_attributes: vec![], + announced_routes: vec![], + })) + .await?; + wait_for_routes(&server, 1).await?; + assert_eq!(held(&server), [(2, vec![PEER_ASN, 64998], true)]); + + // When the session ends, all of its routes go. + peer.send(&announcement(path(3), &[PEER_ASN, 1, 2, 3])) + .await?; + wait_for_routes(&server, 2).await?; + drop(peer); + wait_for_routes(&server, 0).await?; + Ok(()) +} + +/// ADD-PATH changes how prefixes are written. It must be in use only when +/// both sides said so; a peer that offers it to a server that does not is +/// spoken to, and listened to, in plain BGP. +#[tokio::test] +async fn test_add_path_is_not_used_unless_both_sides_offer_it() -> Result<()> { + let server = server_offering(None, &["10.100.0.0/16"]).await?; + let (mut peer, open) = peer_offering(&server, Some(AddPathSendRecv::Both)).await?; + assert!(add_path_offered(&open).is_empty()); + let session = &server.registry.list()[0]; + assert!(session.add_path_receive.is_empty() && session.add_path_send.is_empty()); + + // Plain prefixes both ways. + let update = tokio::time::timeout(TIMEOUT, peer.recv_update()).await??; + assert_eq!(update.announced_routes, vec![prefix("10.100.0.0/16")]); + peer.send(&announcement(prefix("203.0.113.0/24").into(), &[PEER_ASN])) + .await?; + wait_for_routes(&server, 1).await?; + assert_eq!(held(&server), [(0, vec![PEER_ASN], true)]); + + // And the other way round: offered by the server, not by the peer. + let server = server_offering(Some(AddPathSendRecv::Both), &["10.100.0.0/16"]).await?; + let (mut peer, open) = peer_offering(&server, None).await?; + assert_eq!(add_path_offered(&open).len(), SUPPORTED.len()); + let update = tokio::time::timeout(TIMEOUT, peer.recv_update()).await??; + assert_eq!(update.announced_routes, vec![prefix("10.100.0.0/16")]); + Ok(()) +} + +/// Where the peer offered to receive and the server to send, the server's +/// prefixes carry a path identifier, and a peer that reads them so +/// understands them. +#[tokio::test] +async fn test_prefixes_are_sent_with_path_identifiers_when_agreed() -> Result<()> { + let server = server_offering(Some(AddPathSendRecv::Both), &["10.100.0.0/16"]).await?; + let (mut peer, _) = peer_offering(&server, Some(AddPathSendRecv::Receive)).await?; + let session = &server.registry.list()[0]; + assert_eq!( + session.add_path_send, + [AfiSafi::IPV4_UNICAST].into_iter().collect() + ); + assert!(session.add_path_receive.is_empty()); + + // Read as agreed: with identifiers. + peer.ctx.add_path = vec![AfiSafi::IPV4_UNICAST]; + let update = tokio::time::timeout(TIMEOUT, peer.recv_update()).await??; + assert_eq!(update.announced_routes.len(), 1); + assert_eq!(update.announced_routes[0].prefix, prefix("10.100.0.0/16")); + + // The peer did not offer to send, so the server reads its prefixes + // plain. + peer.send(&announcement(prefix("203.0.113.0/24").into(), &[PEER_ASN])) + .await?; + wait_for_routes(&server, 1).await?; + assert_eq!(held(&server), [(0, vec![PEER_ASN], true)]); + Ok(()) +} diff --git a/crates/integration-tests/tests/announce.rs b/crates/integration-tests/tests/announce.rs index 4a46498..f879dbc 100644 --- a/crates/integration-tests/tests/announce.rs +++ b/crates/integration-tests/tests/announce.rs @@ -298,7 +298,10 @@ async fn test_received_routes_removed_when_session_closes() -> Result<()> { AsPathAttribute::from_asns(vec![PEER_ASN]), PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::new(10, 0, 0, 1))), ], - announced_routes: vec![prefix("203.0.113.0/24"), prefix("198.51.100.0/24")], + announced_routes: vec![ + prefix("203.0.113.0/24").into(), + prefix("198.51.100.0/24").into(), + ], }, )) .await?; diff --git a/crates/integration-tests/tests/basic_connection.rs b/crates/integration-tests/tests/basic_connection.rs index 1ad0d53..bce60c0 100644 --- a/crates/integration-tests/tests/basic_connection.rs +++ b/crates/integration-tests/tests/basic_connection.rs @@ -218,7 +218,7 @@ async fn test_message_split_across_a_keepalive_tick_is_still_one_message() -> Re Message::Update(UpdateMessage { withdrawn_routes: vec![], path_attributes: mandatory_attributes(), - announced_routes: vec![prefix], + announced_routes: vec![prefix.into()], }) .to_wire(&peer.ctx, &mut update)?; @@ -340,7 +340,7 @@ async fn test_server_accepts_update() -> Result<()> { withdrawn_routes: vec![], path_attributes: mandatory_attributes(), // 10.0.0.0/8 — one byte of prefix for a /8. - announced_routes: vec![IpPrefix::new(AddressFamilyId::Ipv4, vec![10], 8)?], + announced_routes: vec![IpPrefix::new(AddressFamilyId::Ipv4, vec![10], 8)?.into()], }); peer.send(&update).await?; @@ -366,7 +366,7 @@ async fn test_server_accepts_ipv6_update() -> Result<()> { afi: Afi::IPV6, safi: Safi::UNICAST, next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()), - prefixes: vec![prefix], + prefixes: vec![prefix.into()], }, )), announced_routes: vec![], @@ -396,7 +396,7 @@ async fn test_rib_records_ipv4_announce() -> Result<()> { peer.send(&Message::Update(UpdateMessage { withdrawn_routes: vec![], path_attributes: mandatory_attributes(), - announced_routes: vec![prefix], + announced_routes: vec![prefix.into()], })) .await?; @@ -430,14 +430,14 @@ async fn test_rib_records_ipv4_withdraw() -> Result<()> { peer.send(&Message::Update(UpdateMessage { withdrawn_routes: vec![], path_attributes: mandatory_attributes(), - announced_routes: vec![prefix], + announced_routes: vec![prefix.into()], })) .await?; tokio::time::timeout(Duration::from_secs(1), events.recv()).await??; // Now withdraw. peer.send(&Message::Update(UpdateMessage { - withdrawn_routes: vec![prefix], + withdrawn_routes: vec![prefix.into()], path_attributes: vec![], announced_routes: vec![], })) @@ -475,7 +475,7 @@ async fn test_rib_records_ipv6_announce() -> Result<()> { afi: Afi::IPV6, safi: Safi::UNICAST, next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()), - prefixes: vec![prefix], + prefixes: vec![prefix.into()], }, )), announced_routes: vec![], @@ -516,7 +516,7 @@ async fn test_rib_records_ipv6_withdraw() -> Result<()> { afi: Afi::IPV6, safi: Safi::UNICAST, next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()), - prefixes: vec![prefix], + prefixes: vec![prefix.into()], }, )), announced_routes: vec![], @@ -530,7 +530,7 @@ async fn test_rib_records_ipv6_withdraw() -> Result<()> { path_attributes: vec![PathAttribute::MpUnreachNlri(MpUnreachNlriPathAttribute { afi: Afi::IPV6, safi: Safi::UNICAST, - prefixes: vec![prefix], + prefixes: vec![prefix.into()], })], announced_routes: vec![], })) diff --git a/crates/integration-tests/tests/best_route.rs b/crates/integration-tests/tests/best_route.rs index 36db847..b334fa3 100644 --- a/crates/integration-tests/tests/best_route.rs +++ b/crates/integration-tests/tests/best_route.rs @@ -10,7 +10,7 @@ use bgp_packet::message::{ AsPathAttribute, Message, NextHopPathAttribute, OriginPathAttribute, PathAttribute, UpdateMessage, }; -use bgp_primitives::ip_prefix::IpPrefix; +use bgp_primitives::ip_prefix::{IpPrefix, Nlri}; use eyre::Result; use serde_json::Value; @@ -41,13 +41,13 @@ fn announcement(prefixes: &[IpPrefix], as_path: &[u32]) -> Message { AsPathAttribute::from_asns(as_path.to_vec()), PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::new(192, 0, 2, 1))), ], - announced_routes: prefixes.to_vec(), + announced_routes: Nlri::plain(prefixes.iter().copied()), }) } fn withdrawal(prefixes: &[IpPrefix]) -> Message { Message::Update(UpdateMessage { - withdrawn_routes: prefixes.to_vec(), + withdrawn_routes: Nlri::plain(prefixes.iter().copied()), path_attributes: vec![], announced_routes: vec![], }) diff --git a/crates/integration-tests/tests/collision.rs b/crates/integration-tests/tests/collision.rs index bd030ae..192d37a 100644 --- a/crates/integration-tests/tests/collision.rs +++ b/crates/integration-tests/tests/collision.rs @@ -41,6 +41,7 @@ async fn server_dialing(listener: &TcpListener) -> Result { hold_time_secs: 90, local_address: None, families: FamilySet::supported(), + add_path: None, }]; }) .await @@ -236,7 +237,7 @@ async fn test_restarted_peer_replaces_stale_session_when_enabled() -> Result<()> AsPathAttribute::from_asns(vec![PEER_ASN]), PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::new(10, 0, 0, 9))), ], - announced_routes: vec![IpPrefix::try_from("203.0.113.0/24")?], + announced_routes: vec![IpPrefix::try_from("203.0.113.0/24")?.into()], })) .await?; tokio::time::timeout(TIMEOUT, async { diff --git a/crates/integration-tests/tests/dialer.rs b/crates/integration-tests/tests/dialer.rs index ab233b2..7fd5652 100644 --- a/crates/integration-tests/tests/dialer.rs +++ b/crates/integration-tests/tests/dialer.rs @@ -32,6 +32,7 @@ async fn server_dialing(listener: &TcpListener, remote_asn: u32) -> Result Result<()> { hold_time_secs: 90, local_address: None, families: FamilySet::supported(), + add_path: None, }]; }) .await?; @@ -232,6 +234,7 @@ async fn test_local_address_is_used_as_the_source() -> Result<()> { hold_time_secs: 90, local_address: Some(Ipv4Addr::LOCALHOST.into()), families: FamilySet::supported(), + add_path: None, }]; }) .await?; @@ -248,6 +251,7 @@ async fn test_local_address_is_used_as_the_source() -> Result<()> { hold_time_secs: 90, local_address: Some(Ipv4Addr::new(192, 0, 2, 1).into()), families: FamilySet::supported(), + add_path: None, }]; }) .await?; diff --git a/crates/integration-tests/tests/http_api.rs b/crates/integration-tests/tests/http_api.rs index 00e1493..31fc7c2 100644 --- a/crates/integration-tests/tests/http_api.rs +++ b/crates/integration-tests/tests/http_api.rs @@ -163,7 +163,7 @@ async fn test_received_routes_summary() -> Result<()> { PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::new(10, 0, 0, 1))), PathAttribute::Communities(CommunitiesPathAttribute(vec![(65001, 7)])), ], - announced_routes: vec![prefix("203.0.113.0/24")], + announced_routes: vec![prefix("203.0.113.0/24").into()], })) .await?; diff --git a/crates/integration-tests/tests/negotiation.rs b/crates/integration-tests/tests/negotiation.rs index e1c2ecf..092e648 100644 --- a/crates/integration-tests/tests/negotiation.rs +++ b/crates/integration-tests/tests/negotiation.rs @@ -61,10 +61,10 @@ fn dual_stack_update() -> UpdateMessage { afi: Afi::IPV6, safi: Safi::UNICAST, next_hop: NlriNextHop::Ipv6(Ipv6Addr::new(0x2001, 0xdb8, 0, 0, 0, 0, 0, 1)), - prefixes: vec![prefix("2001:db8:1::/48")], + prefixes: vec![prefix("2001:db8:1::/48").into()], }), ], - announced_routes: vec![prefix("192.0.2.0/24")], + announced_routes: vec![prefix("192.0.2.0/24").into()], } } @@ -189,6 +189,7 @@ async fn test_no_family_in_common_refuses_the_session() -> Result<()> { hold_time_secs: 90, local_address: None, families: only(AfiSafi::IPV6_UNICAST), + add_path: None, }]; // Keep the dialer out of the way. c.connect_retry = Duration::from_secs(3600); @@ -299,7 +300,7 @@ async fn test_a_peer_without_capabilities_gets_two_octet_as_paths() -> Result<() AsPathAttribute::from_asns(vec![PEER_ASN, 64512]), PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::new(192, 0, 2, 1))), ], - announced_routes: vec![prefix("192.0.2.0/24")], + announced_routes: vec![prefix("192.0.2.0/24").into()], })) .await?; let routes = tokio::time::timeout(TIMEOUT, async { diff --git a/crates/packet/src/message.rs b/crates/packet/src/message.rs index 281eb2e..2659ebc 100644 --- a/crates/packet/src/message.rs +++ b/crates/packet/src/message.rs @@ -16,7 +16,7 @@ use strum::EnumDiscriminants; use crate::update_decode::UpdateError; use bgp_primitives::constants::{AddressFamilyId, Afi, AfiSafi, Safi}; -use bgp_primitives::ip_prefix::IpPrefix; +use bgp_primitives::ip_prefix::{IpPrefix, Nlri}; use bgp_primitives::parser::BgpParserError; use bgp_primitives::parser::EncodeError; use bgp_primitives::parser::ParserContext; @@ -292,9 +292,9 @@ impl RouteRefreshMessage { /// Represents a BGP Update message. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct UpdateMessage { - pub withdrawn_routes: Vec, + pub withdrawn_routes: Vec, pub path_attributes: Vec, - pub announced_routes: Vec, + pub announced_routes: Vec, } impl UpdateMessage { @@ -314,12 +314,13 @@ impl UpdateMessage { pub fn to_wire(&self, ctx: &ParserContext, out: &mut BytesMut) -> Result<(), EncodeError> { // Withdrawn routes. - let withdrawn_len = prefix_list_wire_len(&self.withdrawn_routes)?; + // The prefixes in the body are IPv4 unicast (RFC 4271 §4.3). + let with_path_id = ctx.has_add_path(AfiSafi::IPV4_UNICAST); + let withdrawn_len = nlri_wire_len(&self.withdrawn_routes, with_path_id); + let withdrawn_len = u16::try_from(withdrawn_len) + .map_err(|_| too_long("withdrawn routes", withdrawn_len))?; out.put_u16(withdrawn_len); - for prefix in &self.withdrawn_routes { - out.put_u8(prefix.length()); - out.put(prefix.bytes()); - } + write_nlri(&self.withdrawn_routes, with_path_id, out); // Path attributes. let pa_total: u32 = self @@ -335,10 +336,7 @@ impl UpdateMessage { } // NLRI (consumes the remainder of the message). - for prefix in &self.announced_routes { - out.put_u8(prefix.length()); - out.put(prefix.bytes()); - } + write_nlri(&self.announced_routes, with_path_id, out); Ok(()) } @@ -346,26 +344,32 @@ impl UpdateMessage { pub fn wire_len(&self, ctx: &ParserContext) -> Result { // 2 bytes for the withdrawn length field + 2 bytes for the total path // attribute length field. + let with_path_id = ctx.has_add_path(AfiSafi::IPV4_UNICAST); let mut total: u32 = 4; - for prefix in &self.withdrawn_routes { - total += 1 + prefix.bytes().len() as u32; - } + total += nlri_wire_len(&self.withdrawn_routes, with_path_id); for pa in &self.path_attributes { total += pa.wire_len(ctx)? as u32; } - for prefix in &self.announced_routes { - total += 1 + prefix.bytes().len() as u32; - } + total += nlri_wire_len(&self.announced_routes, with_path_id); u16::try_from(total).map_err(|_| too_long("UPDATE message", total)) } } -/// Computes the total wire length of a list of IPv4/IPv6 NLRI prefixes as they -/// appear in the withdrawn or NLRI sections of an UPDATE message (each prefix -/// is encoded as a 1-byte length followed by `ceil(length/8)` prefix bytes). -fn prefix_list_wire_len(prefixes: &[IpPrefix]) -> Result { - let total: u32 = prefixes.iter().map(|p| 1 + p.bytes().len() as u32).sum(); - u16::try_from(total).map_err(|_| too_long("prefix list", total)) +/// The length on the wire of a list of prefixes as an UPDATE names them: +/// for each, its path identifier if the session uses them for the family +/// (RFC 7911 §3), its length in bits, and the bytes that length covers. +fn nlri_wire_len(nlri: &[Nlri], with_path_id: bool) -> u32 { + nlri.iter().map(|n| n.wire_len(with_path_id) as u32).sum() +} + +fn write_nlri(nlri: &[Nlri], with_path_id: bool, out: &mut BytesMut) { + for n in nlri { + if with_path_id { + out.put_u32(n.path_id); + } + out.put_u8(n.prefix.length()); + out.put(n.prefix.bytes()); + } } /// A parser failure as text for a log line. @@ -1479,17 +1483,36 @@ impl NlriNextHop { } } -// parse_prefix is a helper function that implements an NLRI parser for the given AFI. -pub(crate) fn parse_prefix( +/// Parses one prefix as an UPDATE names it: with a path identifier in +/// front if the session uses them for the family (RFC 7911 §3). +pub(crate) fn parse_nlri( afi: AddressFamilyId, + with_path_id: bool, buf: &[u8], -) -> IResult<&[u8], IpPrefix, BgpParserError<&[u8]>> { +) -> IResult<&[u8], Nlri, BgpParserError<&[u8]>> { + let (buf, path_id) = if with_path_id { be_u32(buf)? } else { (buf, 0) }; let (buf, prefix_len) = be_u8(buf)?; let byte_len = prefix_len.div_ceil(8); let (buf, prefix_bytes) = nom::bytes::take(byte_len as usize).parse(buf)?; let prefix = IpPrefix::new(afi, prefix_bytes, prefix_len) .map_err(|e| Failure(BgpParserError::Malformed(e.to_string())))?; - Ok((buf, prefix)) + Ok((buf, Nlri { prefix, path_id })) +} + +/// Parses a field that is nothing but prefixes, to its end. +fn parse_nlri_list( + afi: AddressFamilyId, + with_path_id: bool, + mut buf: &[u8], +) -> IResult<&[u8], Vec, BgpParserError<&[u8]>> { + // Not many0: that stops quietly at a prefix it cannot read. + let mut nlri = Vec::new(); + while !buf.is_empty() { + let (rest, parsed) = parse_nlri(afi, with_path_id, buf)?; + nlri.push(parsed); + buf = rest; + } + Ok((buf, nlri)) } /// The IP version of the prefixes that make up the NLRI of `family`. @@ -1518,7 +1541,7 @@ pub struct MpReachNlriPathAttribute { pub safi: Safi, /// Next hop address (either IPv4 or IPv6 for now). pub next_hop: NlriNextHop, - pub prefixes: Vec, + pub prefixes: Vec, } impl MpReachNlriPathAttribute { @@ -1527,12 +1550,14 @@ impl MpReachNlriPathAttribute { } pub fn from_wire<'a>( - _: &ParserContext, + ctx: &ParserContext, buf: &'a [u8], ) -> IResult<&'a [u8], Self, BgpParserError<&'a [u8]>> { let (buf, afi) = be_u16(buf).map(|(buf, afi)| (buf, Afi(afi)))?; let (buf, safi) = be_u8(buf).map(|(buf, safi)| (buf, Safi(safi)))?; - let version = ip_version_of_nlri(AfiSafi::new(afi, safi))?; + let family = AfiSafi::new(afi, safi); + let version = ip_version_of_nlri(family)?; + let with_path_id = ctx.has_add_path(family); let (buf, nh_len) = be_u8(buf)?; let (buf, nh_bytes) = nom::bytes::take(usize::from(nh_len)).parse(buf)?; @@ -1569,7 +1594,7 @@ impl MpReachNlriPathAttribute { // Reserved byte (formerly the SNPA count), for every family. let (buf, _) = be_u8(buf)?; - let (buf, prefixes) = nom::multi::many0(|buf| parse_prefix(version, buf)).parse(buf)?; + let (buf, prefixes) = parse_nlri_list(version, with_path_id, buf)?; Ok(( buf, @@ -1588,22 +1613,17 @@ impl MpReachNlriPathAttribute { out.put_u8(self.next_hop.wire_len()); self.next_hop.to_wire(ctx, out)?; out.put_u8(0); - for prefix in &self.prefixes { - out.put_u8(prefix.length()); - out.put(prefix.bytes()); - } + write_nlri(&self.prefixes, ctx.has_add_path(self.family()), out); Ok(()) } - pub fn wire_len(&self, _: &ParserContext) -> Result { - Ok(4_u16 - + self.next_hop.wire_len() as u16 - + 1 // Reserved byte (SNPA). - + self - .prefixes - .iter() - .map(|p| 1 + p.bytes().len() as u16) - .sum::()) + pub fn wire_len(&self, ctx: &ParserContext) -> Result { + // AFI, SAFI, next hop length, next hop, reserved byte, prefixes. + let total = 4 + + u32::from(self.next_hop.wire_len()) + + 1 + + nlri_wire_len(&self.prefixes, ctx.has_add_path(self.family())); + u16::try_from(total).map_err(|_| too_long("MP_REACH_NLRI", total)) } } @@ -1611,7 +1631,7 @@ impl MpReachNlriPathAttribute { pub struct MpUnreachNlriPathAttribute { pub afi: Afi, pub safi: Safi, - pub prefixes: Vec, + pub prefixes: Vec, } impl MpUnreachNlriPathAttribute { @@ -1620,14 +1640,16 @@ impl MpUnreachNlriPathAttribute { } pub fn from_wire<'a>( - _: &ParserContext, + ctx: &ParserContext, buf: &'a [u8], ) -> IResult<&'a [u8], Self, BgpParserError<&'a [u8]>> { let (buf, afi) = be_u16(buf).map(|(buf, afi)| (buf, Afi(afi)))?; let (buf, safi) = be_u8(buf).map(|(buf, safi)| (buf, Safi(safi)))?; - let version = ip_version_of_nlri(AfiSafi::new(afi, safi))?; + let family = AfiSafi::new(afi, safi); + let version = ip_version_of_nlri(family)?; + let with_path_id = ctx.has_add_path(family); - let (buf, prefixes) = nom::multi::many0(|buf| parse_prefix(version, buf)).parse(buf)?; + let (buf, prefixes) = parse_nlri_list(version, with_path_id, buf)?; Ok(( buf, @@ -1639,22 +1661,16 @@ impl MpUnreachNlriPathAttribute { )) } - pub fn to_wire(&self, _: &ParserContext, out: &mut BytesMut) -> Result<(), EncodeError> { + pub fn to_wire(&self, ctx: &ParserContext, out: &mut BytesMut) -> Result<(), EncodeError> { out.put_u16(self.afi.0); out.put_u8(self.safi.0); - for prefix in &self.prefixes { - out.put_u8(prefix.length()); - out.put(prefix.bytes()); - } + write_nlri(&self.prefixes, ctx.has_add_path(self.family()), out); Ok(()) } - pub fn wire_len(&self, _: &ParserContext) -> Result { - Ok(3 + self - .prefixes - .iter() - .map(|p| 1 + p.bytes().len() as u16) - .sum::()) + pub fn wire_len(&self, ctx: &ParserContext) -> Result { + let total = 3 + nlri_wire_len(&self.prefixes, ctx.has_add_path(self.family())); + u16::try_from(total).map_err(|_| too_long("MP_UNREACH_NLRI", total)) } } @@ -1721,8 +1737,8 @@ mod tests { }; use bgp_primitives::{ - constants::{AddressFamilyId, Afi, Safi}, - ip_prefix::IpPrefix, + constants::{AddressFamilyId, Afi, AfiSafi, Safi}, + ip_prefix::{IpPrefix, Nlri}, parser::ParserContext, }; @@ -1755,6 +1771,7 @@ mod tests { ParserContext { four_octet_asn: Some(true), ibgp: false, + add_path: Vec::new(), } } @@ -1877,7 +1894,9 @@ mod tests { link_local: ipv6!("fe80::12"), }, prefixes: vec![ - IpPrefix::new(AddressFamilyId::Ipv6, [0x20, 0x01, 0x0d, 0xb8], 32).unwrap() + IpPrefix::new(AddressFamilyId::Ipv6, [0x20, 0x01, 0x0d, 0xb8], 32) + .unwrap() + .into() ] }) ); @@ -1892,7 +1911,9 @@ mod tests { afi: Afi::IPV6, safi: Safi::UNICAST, prefixes: vec![ - IpPrefix::new(AddressFamilyId::Ipv6, [0x20, 0x01, 0x0d, 0xb8], 32).unwrap() + IpPrefix::new(AddressFamilyId::Ipv6, [0x20, 0x01, 0x0d, 0xb8], 32) + .unwrap() + .into() ], }) ); @@ -2031,7 +2052,7 @@ mod tests { UpdateMessage { withdrawn_routes: vec![], path_attributes: sample_ipv4_path_attributes(), - announced_routes: vec![v4_prefix(8, &[0x0a])], + announced_routes: vec![v4_prefix(8, &[0x0a]).into()], } ); @@ -2046,7 +2067,7 @@ mod tests { 0x00, 0x00, ], UpdateMessage { - withdrawn_routes: vec![v4_prefix(8, &[0x0a])], + withdrawn_routes: vec![v4_prefix(8, &[0x0a]).into()], path_attributes: vec![], announced_routes: vec![], } @@ -2069,11 +2090,11 @@ mod tests { 0x18, 0xc0, 0xa8, 0x01, ], UpdateMessage { - withdrawn_routes: vec![v4_prefix(8, &[0x0a])], + withdrawn_routes: vec![v4_prefix(8, &[0x0a]).into()], path_attributes: sample_ipv4_path_attributes(), announced_routes: vec![ - v4_prefix(16, &[0xac, 0x10]), - v4_prefix(24, &[0xc0, 0xa8, 0x01]), + v4_prefix(16, &[0xac, 0x10]).into(), + v4_prefix(24, &[0xc0, 0xa8, 0x01]).into(), ], } ); @@ -2099,6 +2120,75 @@ mod tests { ); // Rejects trailing garbage after the NLRI section (a partial prefix). + /// With ADD-PATH in use for a family, each of its prefixes has a + /// four-octet path identifier in front (RFC 7911 §3): in the body for + /// IPv4 unicast, in MP_REACH_NLRI and MP_UNREACH_NLRI for the others. + #[test] + fn prefixes_carry_a_path_identifier_where_add_path_is_in_use() { + let plain = default_v6_context(); + let v4_only = ParserContext { + add_path: vec![AfiSafi::IPV4_UNICAST], + ..default_v6_context() + }; + let v6_only = ParserContext { + add_path: vec![AfiSafi::IPV6_UNICAST], + ..default_v6_context() + }; + + // Withdrawing 10.0.0.0/8, path 7, in the body. + #[rustfmt::skip] + let body = [ + 0x00, 0x06, // withdrawn routes length + 0x00, 0x00, 0x00, 0x07, 0x08, 0x0a, // path 7, 10.0.0.0/8 + 0x00, 0x00, // no attributes + ]; + let (_, update) = UpdateMessage::from_wire(&v4_only, &body).unwrap(); + assert_eq!( + update.withdrawn_routes, + vec![Nlri { + prefix: v4_prefix(8, &[0x0a]), + path_id: 7, + }] + ); + let mut out = BytesMut::new(); + update.to_wire(&v4_only, &mut out).unwrap(); + assert_eq!(&out[..], &body[..]); + assert_eq!(usize::from(update.wire_len(&v4_only).unwrap()), body.len()); + // The same route on a session without ADD-PATH has no identifier. + let mut out = BytesMut::new(); + update.to_wire(&plain, &mut out).unwrap(); + assert_eq!(&out[..], [0x00, 0x02, 0x08, 0x0a, 0x00, 0x00]); + // And ADD-PATH for IPv6 says nothing about the IPv4 prefixes in + // the body: read that way, these bytes are not prefixes at all. + assert!(UpdateMessage::from_wire(&v6_only, &body).is_err()); + + // Two routes for 2001:db8::/32, paths 1 and 2, withdrawn. + #[rustfmt::skip] + let unreach = [ + 0x00, 0x02, 0x01, + 0x00, 0x00, 0x00, 0x01, 0x20, 0x20, 0x01, 0x0d, 0xb8, + 0x00, 0x00, 0x00, 0x02, 0x20, 0x20, 0x01, 0x0d, 0xb8, + ]; + let (rest, parsed) = MpUnreachNlriPathAttribute::from_wire(&v6_only, &unreach).unwrap(); + assert!(rest.is_empty()); + assert_eq!( + parsed + .prefixes + .iter() + .map(|n| n.path_id) + .collect::>(), + [1, 2] + ); + assert_eq!(parsed.prefixes[0].prefix, parsed.prefixes[1].prefix); + let mut out = BytesMut::new(); + parsed.to_wire(&v6_only, &mut out).unwrap(); + assert_eq!(&out[..], &unreach[..]); + assert_eq!( + usize::from(parsed.wire_len(&v6_only).unwrap()), + unreach.len() + ); + } + // Found by the property tests in tests/properties.rs. /// A prefix longer than its address family allows was accepted, and @@ -2173,6 +2263,7 @@ mod tests { let two_octet = ParserContext { four_octet_asn: Some(false), ibgp: false, + add_path: Vec::new(), }; let wire = [0xfd, 0xe8, 192, 0, 2, 1]; let (rest, parsed) = AggregatorPathAttribute::from_wire(&two_octet, &wire).unwrap(); @@ -2250,10 +2341,12 @@ mod tests { let two_octet = ParserContext { four_octet_asn: Some(false), ibgp: false, + add_path: Vec::new(), }; let four_octet = ParserContext { four_octet_asn: Some(true), ibgp: false, + add_path: Vec::new(), }; let wire = |attribute: &PathAttribute, ctx: &ParserContext| { let mut out = BytesMut::new(); diff --git a/crates/packet/src/open.rs b/crates/packet/src/open.rs index 229074f..a40be99 100644 --- a/crates/packet/src/open.rs +++ b/crates/packet/src/open.rs @@ -290,11 +290,10 @@ pub enum Capability { FourByteAsn { asn: u32, } = 65, - /// Additional Path (RFC 7911). + /// ADD-PATH (RFC 7911): for each address family listed, whether the + /// speaker can send several routes for a prefix, receive them, or both. AddPath { - afi: Afi, - safi: Safi, - send_recv: AddPathSendRecv, + entries: Vec, } = 69, /// Enhanced Route Refresh (RFC 7313). EnhancedRouteRefresh = 70, @@ -526,18 +525,30 @@ impl Capability { Ok(Self::FourByteAsn { asn }) } v if v == CapabilityDiscriminants::AddPath as u8 => { - let (cap_buf, afi) = be_u16(cap_buf)?; - let (cap_buf, safi) = be_u8(cap_buf)?; - let (cap_buf, send_recv) = be_u8(cap_buf)?; - let afi = Afi(afi); - let safi = Safi(safi); - let send_recv = AddPathSendRecv::try_from(send_recv).map_err(|e| fail!("{}", e))?; - require_empty!(cap_buf, "AddPath"); - Ok(Self::AddPath { - afi, - safi, - send_recv, - }) + // A list of families, four octets each. None at all says + // nothing and is let through: it is also what is left of a + // capability whose families were all ignored below. + if !cap_buf.len().is_multiple_of(4) { + return Err(fail!( + "ADD-PATH capability of {} octets is not a list of families", + cap_buf.len() + )); + } + let entries = cap_buf + .as_chunks::<4>() + .0 + .iter() + .filter_map(|entry| { + // A family with a Send/Receive value that is not + // defined is ignored (RFC 7911 §4). + Some(AddPathEntry { + afi: Afi(u16::from_be_bytes([entry[0], entry[1]])), + safi: Safi(entry[2]), + send_recv: AddPathSendRecv::try_from(entry[3]).ok()?, + }) + }) + .collect(); + Ok(Self::AddPath { entries }) } v if v == CapabilityDiscriminants::EnhancedRouteRefresh as u8 => { require_empty!(cap_buf, "EnhancedRouteRefresh"); @@ -655,14 +666,12 @@ impl Capability { Capability::FourByteAsn { asn } => { out.put_u32(*asn); } - Capability::AddPath { - afi, - safi, - send_recv, - } => { - out.put_u16(afi.0); - out.put_u8(safi.0); - out.put_u8(send_recv.into()); + Capability::AddPath { entries } => { + for entry in entries { + out.put_u16(entry.afi.0); + out.put_u8(entry.safi.0); + out.put_u8((&entry.send_recv).into()); + } } Capability::EnhancedRouteRefresh => {} Capability::LongLivedGracefulRestart(llgrs) => { @@ -696,7 +705,7 @@ impl Capability { Capability::BgpRole { .. } => 1, Capability::GracefulRestart { entries, .. } => 2 + 4 * entries.len(), Capability::FourByteAsn { .. } => 4, - Capability::AddPath { .. } => 4, + Capability::AddPath { entries } => 4 * entries.len(), Capability::EnhancedRouteRefresh => 0, Capability::LongLivedGracefulRestart(entries) => 7 * entries.len(), Capability::UnknownCapability { value, .. } => value.len(), @@ -777,7 +786,21 @@ impl TryFrom for OrfSendRecv { } } -#[derive(Debug, Clone, Serialize, Deserialize, EnumDiscriminants, PartialEq, Eq)] +/// One address family in an ADD-PATH capability. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct AddPathEntry { + pub afi: Afi, + pub safi: Safi, + pub send_recv: AddPathSendRecv, +} + +impl AddPathEntry { + pub fn family(&self) -> AfiSafi { + AfiSafi::new(self.afi, self.safi) + } +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, EnumDiscriminants, PartialEq, Eq)] #[repr(u8)] pub enum AddPathSendRecv { Receive = 1, @@ -785,6 +808,19 @@ pub enum AddPathSendRecv { Both = 3, } +impl AddPathSendRecv { + /// Whether the speaker that advertised this can send several routes + /// for a prefix. + pub fn sends(self) -> bool { + matches!(self, Self::Send | Self::Both) + } + + /// Whether the speaker that advertised this can receive them. + pub fn receives(self) -> bool { + matches!(self, Self::Receive | Self::Both) + } +} + impl TryFrom for AddPathSendRecv { type Error = InvalidValue; @@ -899,8 +935,8 @@ mod tests { use eyre::Result; use crate::open::{ - AddPathSendRecv, Capability, ExtendedNextHop, GracefulRestartEntry, MultiLabelEntry, - OpenMessage, OpenOption, + AddPathEntry, AddPathSendRecv, Capability, ExtendedNextHop, GracefulRestartEntry, + MultiLabelEntry, OpenMessage, OpenOption, }; use bgp_primitives::{ @@ -929,6 +965,7 @@ mod tests { ParserContext { four_octet_asn: Some(true), ibgp: false, + add_path: Vec::new(), } } @@ -1121,9 +1158,11 @@ mod tests { ( &[0x45, 0x04, 0x00, 0x19, 0x46, 0x03], Capability::AddPath { - afi: evpn.0, - safi: evpn.1, - send_recv: AddPathSendRecv::Both, + entries: vec![AddPathEntry { + afi: evpn.0, + safi: evpn.1, + send_recv: AddPathSendRecv::Both, + }], }, ), ( @@ -1154,6 +1193,48 @@ mod tests { assert!(Capability::from_wire(&ctx, &[0x01, 0x03, 0x00, 0x01, 0x00]).is_err()); } + /// One ADD-PATH capability lists every family it is for. Reading only + /// one made the OPEN of a router with ADD-PATH for both IPv4 and IPv6 + /// malformed, so the session never came up. + #[test] + fn add_path_capability_lists_several_families() { + let ctx = ParserContext::default(); + // IPv4 unicast: receive. IPv6 unicast: send and receive. + let wire = [0x45, 0x08, 0x00, 0x01, 0x01, 0x01, 0x00, 0x02, 0x01, 0x03]; + let (rest, cap) = Capability::from_wire(&ctx, &wire).unwrap(); + assert!(rest.is_empty()); + assert_eq!( + cap, + Capability::AddPath { + entries: vec![ + AddPathEntry { + afi: Afi::IPV4, + safi: Safi::UNICAST, + send_recv: AddPathSendRecv::Receive, + }, + AddPathEntry { + afi: Afi::IPV6, + safi: Safi::UNICAST, + send_recv: AddPathSendRecv::Both, + }, + ], + } + ); + let mut out = BytesMut::new(); + cap.to_wire(&ctx, &mut out).unwrap(); + assert_eq!(&out[..], &wire[..]); + + // A family with a Send/Receive value nobody defined is left out; + // the others stand. + let odd = [0x45, 0x08, 0x00, 0x01, 0x01, 0x07, 0x00, 0x02, 0x01, 0x02]; + let (_, cap) = Capability::from_wire(&ctx, &odd).unwrap(); + assert!( + matches!(&cap, Capability::AddPath { entries } if entries.len() == 1 && entries[0].afi == Afi::IPV6) + ); + // Not a whole number of families: malformed. + assert!(Capability::from_wire(&ctx, &[0x45, 0x05, 0, 1, 1, 1, 0]).is_err()); + } + // Found by the property tests in tests/properties.rs: both were written // differently from how they are read, so an OPEN of ours carrying one // would have been malformed. diff --git a/crates/packet/src/update_decode.rs b/crates/packet/src/update_decode.rs index 28eeb5d..34a84f5 100644 --- a/crates/packet/src/update_decode.rs +++ b/crates/packet/src/update_decode.rs @@ -21,14 +21,14 @@ use std::collections::HashSet; -use bgp_primitives::constants::AddressFamilyId; -use bgp_primitives::ip_prefix::IpPrefix; +use bgp_primitives::constants::{AddressFamilyId, AfiSafi}; +use bgp_primitives::ip_prefix::Nlri; use bgp_primitives::parser::{BgpParserError, ParserContext}; use crate::errors::{BgpError, DecodeError, UpdateMsgSubcode}; use crate::message::{ MpUnreachNlriPathAttribute, PathAttribute, PathAttributeDiscriminants, PathAttributeFlags, - UpdateMessage, defined_flags, describe, parse_prefix, + UpdateMessage, defined_flags, describe, parse_nlri, }; /// How an error in an UPDATE was handled, short of resetting the session. @@ -112,12 +112,15 @@ fn reset(subcode: UpdateMsgSubcode, data: Vec, detail: String) -> DecodeErro } } -/// Parses a field that is nothing but prefixes. An error here cannot be -/// recovered from: the prefixes are what treat-as-withdraw would act on. -fn prefixes(field: &str, mut buf: &[u8]) -> Result, DecodeError> { +/// Parses a field of the message body that is nothing but prefixes. An +/// error here cannot be recovered from: the prefixes are what +/// treat-as-withdraw would act on. +fn prefixes(ctx: &ParserContext, field: &str, mut buf: &[u8]) -> Result, DecodeError> { + // The prefixes in the body are IPv4 unicast (RFC 4271 §4.3). + let with_path_id = ctx.has_add_path(AfiSafi::IPV4_UNICAST); let mut prefixes = Vec::new(); while !buf.is_empty() { - let (rest, prefix) = parse_prefix(AddressFamilyId::Ipv4, buf).map_err(|e| { + let (rest, prefix) = parse_nlri(AddressFamilyId::Ipv4, with_path_id, buf).map_err(|e| { reset( UpdateMsgSubcode::InvalidNetworkField, vec![], @@ -165,8 +168,8 @@ impl UpdateMessage { let attributes_buf = &body[start..end]; let nlri_buf = &body[end..]; - let withdrawn_routes = prefixes("withdrawn routes", withdrawn_buf)?; - let announced_routes = prefixes("NLRI", nlri_buf)?; + let withdrawn_routes = prefixes(ctx, "withdrawn routes", withdrawn_buf)?; + let announced_routes = prefixes(ctx, "NLRI", nlri_buf)?; let mut errors = Vec::new(); let mut kept_wire = Vec::with_capacity(attributes_buf.len()); @@ -437,7 +440,7 @@ fn as_withdrawal(update: UpdateMessage) -> UpdateMessage { withdrawn_routes.extend(update.announced_routes); let mut unreach: Vec = Vec::new(); - let mut withdraw = |afi, safi, prefixes: Vec| match unreach + let mut withdraw = |afi, safi, prefixes: Vec| match unreach .iter_mut() .find(|u| u.afi == afi && u.safi == safi) { @@ -469,7 +472,8 @@ fn as_withdrawal(update: UpdateMessage) -> UpdateMessage { mod tests { use std::net::Ipv4Addr; - use bgp_primitives::constants::{Afi, AfiSafi, Safi}; + use bgp_primitives::constants::{Afi, Safi}; + use bgp_primitives::ip_prefix::IpPrefix; use super::*; use crate::message::{ @@ -490,6 +494,7 @@ mod tests { ParserContext { four_octet_asn: Some(true), ibgp: false, + add_path: Vec::new(), } } @@ -497,6 +502,7 @@ mod tests { ParserContext { four_octet_asn: Some(true), ibgp: true, + add_path: Vec::new(), } } @@ -615,6 +621,7 @@ mod tests { let two_octet = ParserContext { four_octet_asn: Some(false), ibgp: false, + add_path: Vec::new(), }; let attributes = vec![ origin(), @@ -846,6 +853,7 @@ mod tests { let ctx = ParserContext { four_octet_asn: Some(false), ibgp: false, + add_path: Vec::new(), }; // On this session AS_PATH is two octets wide. let attributes = |as4_path: Vec| { @@ -905,7 +913,7 @@ mod tests { [PathAttribute::MpUnreachNlri(MpUnreachNlriPathAttribute { afi: Afi::IPV6, safi: Safi::UNICAST, - prefixes: vec![v6], + prefixes: vec![v6.into()], })], "{case}" ); diff --git a/crates/packet/tests/properties.rs b/crates/packet/tests/properties.rs index 4316354..be11ea9 100644 --- a/crates/packet/tests/properties.rs +++ b/crates/packet/tests/properties.rs @@ -25,12 +25,12 @@ use bgp_packet::message::{ PathAttributeFlags, RouteRefreshMessage, UpdateMessage, }; use bgp_packet::open::{ - AddPathSendRecv, BgpRole, BgpSecHeader, Capability, ExtendedNextHop, GracefulRestartEntry, - LongLivedGracefulRestart, MultiLabelEntry, OpenMessage, OpenOption, OrfSendRecv, OrfType, - OutboundRouteFilterEntry, + AddPathEntry, AddPathSendRecv, BgpRole, BgpSecHeader, Capability, ExtendedNextHop, + GracefulRestartEntry, LongLivedGracefulRestart, MultiLabelEntry, OpenMessage, OpenOption, + OrfSendRecv, OrfType, OutboundRouteFilterEntry, }; -use bgp_primitives::constants::{AddressFamilyId, Afi, Safi}; -use bgp_primitives::ip_prefix::IpPrefix; +use bgp_primitives::constants::{AddressFamilyId, Afi, AfiSafi, Safi}; +use bgp_primitives::ip_prefix::{IpPrefix, Nlri}; use bgp_primitives::parser::{EncodeError, ParserContext}; use bytes::BytesMut; use proptest::collection::vec; @@ -41,6 +41,9 @@ use proptest::prelude::*; struct Session { four_octet_asn: bool, ibgp: bool, + /// Whether prefixes carry path identifiers (RFC 7911), for IPv4 and + /// for IPv6 unicast. + add_path: [bool; 2], } impl Session { @@ -48,14 +51,28 @@ impl Session { ParserContext { four_octet_asn: Some(self.four_octet_asn), ibgp: self.ibgp, + add_path: [AfiSafi::IPV4_UNICAST, AfiSafi::IPV6_UNICAST] + .into_iter() + .zip(self.add_path) + .filter_map(|(family, in_use)| in_use.then_some(family)) + .collect(), } } + + fn has_add_path(self, version: AddressFamilyId, safi: Safi) -> bool { + safi == Safi::UNICAST + && match version { + AddressFamilyId::Ipv4 => self.add_path[0], + AddressFamilyId::Ipv6 => self.add_path[1], + } + } } fn session() -> impl Strategy { - any::<(bool, bool)>().prop_map(|(four_octet_asn, ibgp)| Session { + any::<(bool, bool, [bool; 2])>().prop_map(|(four_octet_asn, ibgp, add_path)| Session { four_octet_asn, ibgp, + add_path, }) } @@ -73,6 +90,23 @@ fn prefix(version: AddressFamilyId) -> impl Strategy { }) } +/// Prefixes as an UPDATE names them. Without ADD-PATH for the family there +/// is no identifier on the wire, and it reads back as zero. +fn nlri( + session: Session, + version: AddressFamilyId, + safi: Safi, +) -> impl Strategy> { + let with_path_id = session.has_add_path(version, safi); + vec( + (prefix(version), any::()).prop_map(move |(prefix, path_id)| Nlri { + prefix, + path_id: if with_path_id { path_id } else { 0 }, + }), + 0..5, + ) +} + fn afi() -> impl Strategy { any::().prop_map(Afi) } @@ -111,33 +145,41 @@ fn extended_community() -> impl Strategy { }) } -fn mp_reach() -> impl Strategy { - let ip_safi = prop_oneof![Just(Safi::UNICAST), Just(Safi::MULTICAST)]; - let v4 = ( - ip_safi.clone(), - any::<[u8; 4]>(), - vec(prefix(AddressFamilyId::Ipv4), 0..5), - ) +fn mp_reach(session: Session) -> impl Strategy { + let ip_safi = || prop_oneof![Just(Safi::UNICAST), Just(Safi::MULTICAST)]; + let v4 = ip_safi() + .prop_flat_map(move |safi| { + ( + Just(safi), + any::<[u8; 4]>(), + nlri(session, AddressFamilyId::Ipv4, safi), + ) + }) .prop_map(|(safi, next_hop, prefixes)| MpReachNlriPathAttribute { afi: Afi::IPV4, safi, next_hop: NlriNextHop::Ipv4(Ipv4Addr::from(next_hop)), prefixes, }); - let v6_next_hop = prop_oneof![ - any::<[u8; 16]>().prop_map(|a| NlriNextHop::Ipv6(Ipv6Addr::from(a))), - (any::<[u8; 16]>(), any::<[u8; 16]>()).prop_map(|(global, link_local)| { - NlriNextHop::Ipv6WithLl { - global: Ipv6Addr::from(global), - link_local: Ipv6Addr::from(link_local), - } - }), - ]; - let v6 = ( - ip_safi, - v6_next_hop, - vec(prefix(AddressFamilyId::Ipv6), 0..5), - ) + let v6_next_hop = || { + prop_oneof![ + any::<[u8; 16]>().prop_map(|a| NlriNextHop::Ipv6(Ipv6Addr::from(a))), + (any::<[u8; 16]>(), any::<[u8; 16]>()).prop_map(|(global, link_local)| { + NlriNextHop::Ipv6WithLl { + global: Ipv6Addr::from(global), + link_local: Ipv6Addr::from(link_local), + } + }), + ] + }; + let v6 = ip_safi() + .prop_flat_map(move |safi| { + ( + Just(safi), + v6_next_hop(), + nlri(session, AddressFamilyId::Ipv6, safi), + ) + }) .prop_map(|(safi, next_hop, prefixes)| MpReachNlriPathAttribute { afi: Afi::IPV6, safi, @@ -147,9 +189,9 @@ fn mp_reach() -> impl Strategy { prop_oneof![v4, v6] } -fn mp_unreach() -> impl Strategy { - let of = |version: AddressFamilyId| { - vec(prefix(version), 0..5).prop_map(move |prefixes| MpUnreachNlriPathAttribute { +fn mp_unreach(session: Session) -> impl Strategy { + let of = move |version: AddressFamilyId| { + nlri(session, version, Safi::UNICAST).prop_map(move |prefixes| MpUnreachNlriPathAttribute { afi: version.into(), safi: Safi::UNICAST, prefixes, @@ -237,8 +279,8 @@ fn path_attributes(session: Session) -> impl Strategy vec(any::<(u16, u16)>(), 1..8) .prop_map(|c| PathAttribute::Communities(CommunitiesPathAttribute(c))), ), - proptest::option::of(mp_reach().prop_map(PathAttribute::MpReachNlri)), - proptest::option::of(mp_unreach().prop_map(PathAttribute::MpUnreachNlri)), + proptest::option::of(mp_reach(session).prop_map(PathAttribute::MpReachNlri)), + proptest::option::of(mp_unreach(session).prop_map(PathAttribute::MpUnreachNlri)), proptest::option::of( vec(extended_community(), 1..6).prop_map(|extended_communities| { PathAttribute::ExtendedCommunities(ExtendedCommunitiesPathAttribute { @@ -289,9 +331,9 @@ fn path_attributes(session: Session) -> impl Strategy fn update(session: Session) -> impl Strategy { ( - vec(prefix(AddressFamilyId::Ipv4), 0..5), + nlri(session, AddressFamilyId::Ipv4, Safi::UNICAST), path_attributes(session), - vec(prefix(AddressFamilyId::Ipv4), 0..5), + nlri(session, AddressFamilyId::Ipv4, Safi::UNICAST), ) .prop_map( |(withdrawn_routes, path_attributes, announced_routes)| UpdateMessage { @@ -367,10 +409,15 @@ fn capability() -> impl Strategy { } }), any::().prop_map(|asn| Capability::FourByteAsn { asn }), - (afi(), safi(), add_path).prop_map(|(afi, safi, send_recv)| Capability::AddPath { - afi, - safi, - send_recv, + vec((afi(), safi(), add_path), 1..4).prop_map(|entries| Capability::AddPath { + entries: entries + .into_iter() + .map(|(afi, safi, send_recv)| AddPathEntry { + afi, + safi, + send_recv, + }) + .collect(), }), Just(Capability::EnhancedRouteRefresh), vec((afi(), safi(), any::(), 0u32..(1 << 24)), 0..4).prop_map(|entries| { diff --git a/crates/primitives/src/ip_prefix.rs b/crates/primitives/src/ip_prefix.rs index f18f16b..620532a 100644 --- a/crates/primitives/src/ip_prefix.rs +++ b/crates/primitives/src/ip_prefix.rs @@ -124,6 +124,52 @@ impl IpPrefix { } } +/// A prefix as an UPDATE names it: the prefix and, on a session where +/// ADD-PATH is in use for the address family (RFC 7911), the identifier +/// that tells apart the several routes a peer may announce for it. +/// +/// Whether the identifier is on the wire is a matter of what the session +/// negotiated, not of its value. Where it is not, it is zero here. +#[derive(Debug, PartialEq, Eq, Clone, Copy, Hash, Serialize, Deserialize)] +pub struct Nlri { + pub prefix: IpPrefix, + pub path_id: u32, +} + +impl Nlri { + /// `prefixes` as they are named on a session without ADD-PATH. + pub fn plain(prefixes: impl IntoIterator) -> Vec { + prefixes.into_iter().map(Self::from).collect() + } + + /// The bytes this takes on the wire, with or without its identifier. + pub fn wire_len(&self, with_path_id: bool) -> usize { + 1 + self.prefix.bytes().len() + if with_path_id { 4 } else { 0 } + } +} + +impl From for Nlri { + fn from(prefix: IpPrefix) -> Self { + Self { prefix, path_id: 0 } + } +} + +/// So that a list of these compares with a list of plain prefixes. +impl PartialEq for Nlri { + fn eq(&self, prefix: &IpPrefix) -> bool { + self.path_id == 0 && self.prefix == *prefix + } +} + +impl Display for Nlri { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self.path_id { + 0 => write!(f, "{}", self.prefix), + id => write!(f, "{} (path {id})", self.prefix), + } + } +} + impl TryFrom<&str> for IpPrefix { type Error = PrefixError; diff --git a/crates/primitives/src/parser.rs b/crates/primitives/src/parser.rs index cf1c9f0..9dc48e0 100644 --- a/crates/primitives/src/parser.rs +++ b/crates/primitives/src/parser.rs @@ -1,8 +1,10 @@ use nom::error::{ErrorKind, ParseError}; + +use crate::constants::AfiSafi; use thiserror::Error; /// What a parser has to know about the session a message belongs to. -#[derive(Debug, Default)] +#[derive(Debug, Clone, Default)] pub struct ParserContext { /// Whether AS numbers are four octets wide on this session (RFC 6793). /// `None` until the OPENs have been exchanged. @@ -11,6 +13,17 @@ pub struct ParserContext { /// only between internal peers and are dropped when an external one /// sends them (RFC 7606 §7). pub ibgp: bool, + /// The address families whose prefixes carry a path identifier in the + /// direction this context is for (RFC 7911). What a session sends and + /// what it receives are negotiated separately, so a session has one + /// context for reading and one for writing. + pub add_path: Vec, +} + +impl ParserContext { + pub fn has_add_path(&self, family: AfiSafi) -> bool { + self.add_path.contains(&family) + } } /// Why a value could not be written to the wire. diff --git a/crates/rib/src/table/attrs.rs b/crates/rib/src/table/attrs.rs index db2d8b3..48c5a31 100644 --- a/crates/rib/src/table/attrs.rs +++ b/crates/rib/src/table/attrs.rs @@ -175,6 +175,7 @@ fn four_octet() -> ParserContext { ParserContext { four_octet_asn: Some(true), ibgp: true, + add_path: Vec::new(), } } diff --git a/crates/rib/src/table/journal.rs b/crates/rib/src/table/journal.rs index 6e9310c..fb4be0a 100644 --- a/crates/rib/src/table/journal.rs +++ b/crates/rib/src/table/journal.rs @@ -34,6 +34,16 @@ //! A new reader starts the same way: `Reset`, every best route, `Synced`, //! and changes from then on. //! +//! # All routes, or only the best +//! +//! There are two journals in each shard. One records changes of best route, +//! for followers that want one route for each prefix: the forwarding table, +//! and peers that are sent the best route. The other records every change +//! to any route, for followers that want them all: peers that are sent +//! several routes for a prefix (ADD-PATH, RFC 7911), or a choice made for +//! them alone. A [`Reader`] follows one or the other. The second journal +//! costs nothing while nobody follows it. +//! //! # Order //! //! For any one prefix, a reader is given its states in the order they came @@ -49,7 +59,7 @@ use bgp_primitives::ip_prefix::IpPrefix; use foldhash::fast::RandomState; use hashbrown::HashMap; -use super::{AttrSet, PeerIndex, RibTable, Shard, read, write}; +use super::{AttrSet, PeerIndex, RibTable, RouteRef, Row, Shard, read, write}; /// A cursor that holds nothing back: a reader slot that is free, or a /// reader that has not read this shard yet. @@ -115,9 +125,10 @@ impl Journal { } } - /// Notes that the best route of `prefix`, which has a row, changed. - /// Returns the sequence number for the row to remember. - pub(super) fn best_changed(&mut self, prefix: IpPrefix) -> u64 { + /// Notes a change to `prefix`, which has a row: of its best route, or + /// of any of its routes, whichever this journal is for. Returns the + /// sequence number for the row to remember. + pub(super) fn changed(&mut self, prefix: IpPrefix) -> u64 { self.seq += 1; if self.readers > 0 { self.entries.push_back(Entry { @@ -128,13 +139,13 @@ impl Journal { self.seq } - /// As [`Journal::best_changed`], for a prefix whose row was just made: + /// As [`Journal::changed`], for a prefix whose row was just made: /// if it had gone and is back, the note that it had gone is void. pub(super) fn appeared(&mut self, prefix: IpPrefix) -> u64 { if !self.gone.is_empty() { self.gone.remove(&prefix); } - self.best_changed(prefix) + self.changed(prefix) } /// Notes that `prefix` lost its last route and has no row any more. @@ -160,25 +171,63 @@ impl Journal { } } +/// Which of a shard's two journals. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum Kind { + /// Changes of best route. + Best, + /// Changes of any route. + All, +} + +impl Kind { + /// The sequence number `row` remembers for this journal. + fn seq_of(self, row: &Row) -> u64 { + match self { + Self::Best => row.changed, + Self::All => row.touched, + } + } +} + impl Shard { - /// Whether `entry` still says something: it is the latest change of - /// its prefix. - fn is_live(&self, entry: &Entry) -> bool { - let hash = self.hasher.hash_one(entry.prefix); - match self.rows.find(hash, |row| row.prefix == entry.prefix) { - Some(row) => row.changed == entry.seq, - None => self.journal.gone.get(&entry.prefix) == Some(&entry.seq), + fn journal_of(&self, kind: Kind) -> &Journal { + match kind { + Kind::Best => &self.journal, + Kind::All => &self.all_journal, } } - /// Keeps the journal from growing. Called after every change to the + fn journal_of_mut(&mut self, kind: Kind) -> &mut Journal { + match kind { + Kind::Best => &mut self.journal, + Kind::All => &mut self.all_journal, + } + } + + /// Whether `entry` still says something: it is the latest change of + /// its prefix. + fn is_live(&self, kind: Kind, entry: &Entry) -> bool { + let hash = self.hasher.hash_one(entry.prefix); + match self.rows.find(hash, |row| row.prefix == entry.prefix) { + Some(row) => kind.seq_of(row) == entry.seq, + None => self.journal_of(kind).gone.get(&entry.prefix) == Some(&entry.seq), + } + } + + /// Keeps the journals from growing. Called after every change to the /// shard; does nothing until there is something worth doing. - pub(super) fn tidy_journal(&mut self) { - if self.journal.entries.len() <= self.journal.limits.tidy_at { + pub(super) fn tidy_journals(&mut self) { + self.tidy_journal(Kind::Best); + self.tidy_journal(Kind::All); + } + + fn tidy_journal(&mut self, kind: Kind) { + if self.journal_of(kind).entries.len() <= self.journal_of(kind).limits.tidy_at { return; } // What every reader has passed can go. - let journal = &mut self.journal; + let journal = self.journal_of_mut(kind); let passed = journal .cursors .iter() @@ -193,23 +242,23 @@ impl Shard { journal.entries.pop_front(); } - let limit = 2 * (self.rows.len() + self.journal.limits.slack); - if self.journal.entries.len() <= limit { + let limit = 2 * (self.rows.len() + self.journal_of(kind).limits.slack); + if self.journal_of(kind).entries.len() <= limit { return; } // Dead entries: their prefix has changed again since. This looks // every entry up, but only once the journal has doubled, so the // cost per change stays small. - let mut entries = std::mem::take(&mut self.journal.entries); - entries.retain(|entry| self.is_live(entry)); - self.journal.entries = entries; + let mut entries = std::mem::take(&mut self.journal_of_mut(kind).entries); + entries.retain(|entry| self.is_live(kind, entry)); + self.journal_of_mut(kind).entries = entries; - if self.journal.entries.len() <= limit { + if self.journal_of(kind).entries.len() <= limit { return; } // Still too long: some reader is not reading, and prefixes keep // coming and going. It loses its place. - let journal = &mut self.journal; + let journal = self.journal_of_mut(kind); for entry in journal.entries.drain(..limit / 2) { Journal::drop_entry(&mut journal.gone, &entry); journal.floor = entry.seq; @@ -217,6 +266,31 @@ impl Shard { } } +/// The routes of one prefix, as a follower of all routes is given them. +#[derive(Clone, Copy)] +pub struct Routes<'a>(&'a Row); + +impl<'a> Routes<'a> { + pub fn iter(&self) -> impl Iterator> + use<'a> { + self.0.routes() + } + + /// How many there are: at least one. + pub fn len(&self) -> usize { + self.0.paths.len() + } + + pub fn is_empty(&self) -> bool { + self.len() == 0 + } +} + +impl std::fmt::Debug for Routes<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_list().entries(self.iter()).finish() + } +} + /// What a [`Reader`] is given. #[derive(Debug, Clone, Copy)] pub enum Event<'a> { @@ -225,12 +299,22 @@ pub enum Event<'a> { /// reader is given, and what a reader that fell too far behind is /// given in place of what it missed. Reset, - /// This is now the best route for `prefix`. + /// This is now the best route for `prefix`. Given to a follower of the + /// best routes. Best { prefix: IpPrefix, peer: PeerIndex, + /// The peer's identifier for the route: zero unless the peer + /// sends several routes for a prefix (ADD-PATH, RFC 7911). + path_id: u32, attrs: &'a Arc, }, + /// These are now all the routes for `prefix`: whatever was known of its + /// routes before is replaced. Given to a follower of all routes. + Routes { + prefix: IpPrefix, + routes: Routes<'a>, + }, /// `prefix` has no route any more. Withdrawn { prefix: IpPrefix }, /// The table that began with [`Event::Reset`] is complete: a prefix @@ -261,6 +345,8 @@ enum Phase { /// the place up, so that the journals need not keep entries for it. pub struct Reader { table: Arc, + /// Which journal it follows. + kind: Kind, slot: usize, /// Where this reader has read to in each shard; the copy in the shard /// is for the shard's benefit. @@ -275,25 +361,42 @@ impl RibTable { /// A new follower of this table's best routes. It is given the whole /// table first and every change after. pub fn reader(self: &Arc) -> Reader { - self.register(Phase::Resync { - next: 0, - announced: false, - }) + self.register( + Kind::Best, + Phase::Resync { + next: 0, + announced: false, + }, + ) } /// A new follower that is given only the changes from now on, for /// those who want the news and not the table. If it falls too far /// behind it is reset like any other, and is then given the table. pub fn reader_from_now(self: &Arc) -> Reader { - let mut reader = self.register(Phase::Follow { next: 0 }); + let mut reader = self.register(Kind::Best, Phase::Follow { next: 0 }); for (index, shard) in self.shards.iter().enumerate() { let shard = read(shard); - reader.set_cursor(index, &shard, shard.journal.seq); + reader.set_cursor(index, &shard, shard.journal_of(Kind::Best).seq); } reader } - fn register(self: &Arc, phase: Phase) -> Reader { + /// A new follower of every route in the table, not only the best: it + /// is told whenever any route of a prefix changes, with all the + /// prefix's routes ([`Event::Routes`]). The whole table first, like + /// [`RibTable::reader`]. + pub fn reader_of_all_routes(self: &Arc) -> Reader { + self.register( + Kind::All, + Phase::Resync { + next: 0, + announced: false, + }, + ) + } + + fn register(self: &Arc, kind: Kind, phase: Phase) -> Reader { let mut slots = self .reader_slots .lock() @@ -307,7 +410,8 @@ impl RibTable { }; slots[slot] = true; for shard in &self.shards { - let journal = &mut write(shard).journal; + let mut shard = write(shard); + let journal = shard.journal_of_mut(kind); while journal.cursors.len() <= slot { journal.cursors.push(AtomicU64::new(NO_CURSOR)); } @@ -317,6 +421,7 @@ impl RibTable { self.readers.fetch_add(1, Ordering::AcqRel); Reader { table: Arc::clone(self), + kind, slot, cursors: vec![NO_CURSOR; self.shards.len()], phase, @@ -330,7 +435,10 @@ impl RibTable { pub fn journal_len(&self) -> usize { self.shards .iter() - .map(|shard| read(shard).journal.len()) + .map(|shard| { + let shard = read(shard); + shard.journal.len() + shard.all_journal.len() + }) .sum() } @@ -346,7 +454,8 @@ impl RibTable { impl Drop for Reader { fn drop(&mut self) { for shard in &self.table.shards { - let journal = &mut write(shard).journal; + let mut shard = write(shard); + let journal = shard.journal_of_mut(self.kind); journal.cursors[self.slot].store(NO_CURSOR, Ordering::Release); journal.readers -= 1; if journal.readers == 0 { @@ -477,18 +586,38 @@ impl Reader { } } + /// What this reader is told about a row. + fn event_for<'a>(&self, row: &'a Row) -> Event<'a> { + match self.kind { + Kind::Best => { + let best = row.route(usize::from(row.best)); + Event::Best { + prefix: row.prefix, + peer: best.peer, + path_id: best.path_id, + attrs: best.attrs, + } + } + Kind::All => Event::Routes { + prefix: row.prefix, + routes: Routes(row), + }, + } + } + /// Gives up this reader's place in every journal, before a /// resynchronisation takes a new one. fn forget_place(&mut self) { for (index, shard) in self.table.shards.iter().enumerate() { - read(shard).journal.cursors[self.slot].store(NO_CURSOR, Ordering::Release); + read(shard).journal_of(self.kind).cursors[self.slot] + .store(NO_CURSOR, Ordering::Release); self.cursors[index] = NO_CURSOR; } } fn set_cursor(&mut self, index: usize, shard: &Shard, cursor: u64) { self.cursors[index] = cursor; - shard.journal.cursors[self.slot].store(cursor, Ordering::Release); + shard.journal_of(self.kind).cursors[self.slot].store(cursor, Ordering::Release); } /// Gives out every best route in a shard and takes a place at the end @@ -498,14 +627,9 @@ impl Reader { let table = Arc::clone(&self.table); let shard = read(&table.shards[index]); for row in &shard.rows { - let best = &row.paths[usize::from(row.best)]; - visit(Event::Best { - prefix: row.prefix, - peer: best.peer(), - attrs: &best.attrs, - }); + visit(self.event_for(row)); } - self.set_cursor(index, &shard, shard.journal.seq); + self.set_cursor(index, &shard, shard.journal_of(self.kind).seq); shard.rows.len() } @@ -514,7 +638,7 @@ impl Reader { fn drain(&mut self, index: usize, max: usize, visit: &mut impl FnMut(Event<'_>)) -> Drained { let table = Arc::clone(&self.table); let shard = read(&table.shards[index]); - let journal = &shard.journal; + let journal = shard.journal_of(self.kind); let mut cursor = self.cursors[index]; if cursor < journal.floor { return Drained::Lagged; @@ -537,13 +661,8 @@ impl Reader { cursor = entry.seq; let hash = shard.hasher.hash_one(entry.prefix); match shard.rows.find(hash, |row| row.prefix == entry.prefix) { - Some(row) if row.changed == entry.seq => { - let best = &row.paths[usize::from(row.best)]; - visit(Event::Best { - prefix: row.prefix, - peer: best.peer(), - attrs: &best.attrs, - }); + Some(row) if self.kind.seq_of(row) == entry.seq => { + visit(self.event_for(row)); events += 1; } None if journal.gone.get(&entry.prefix) == Some(&entry.seq) => { diff --git a/crates/rib/src/table/journal_tests.rs b/crates/rib/src/table/journal_tests.rs index 716ffdf..b7e2415 100644 --- a/crates/rib/src/table/journal_tests.rs +++ b/crates/rib/src/table/journal_tests.rs @@ -17,23 +17,58 @@ struct Mirror { best: HashMap)>, /// What was held before a reset and has not been heard of since. stale: HashMap)>, + /// For a follower of all routes: every route of every prefix, as + /// (peer, path identifier, attributes, whether best). + routes: HashMap>, resets: usize, synced: usize, best_events: usize, + routes_events: usize, withdrawn_events: usize, } +/// A route as a mirror holds it. The attributes are held, so that the +/// address used to compare them stays theirs. +type Held = (PeerIndex, u32, Arc, bool); + +fn same_routes(a: &[Held], b: &[Held]) -> bool { + a.len() == b.len() + && a.iter() + .zip(b) + .all(|(a, b)| (a.0, a.1, a.3) == (b.0, b.1, b.3) && Arc::ptr_eq(&a.2, &b.2)) +} + +fn sorted(mut routes: Vec) -> Vec { + routes.sort_by_key(|(peer, path_id, _, _)| (*peer, *path_id)); + routes +} + impl Mirror { fn apply(&mut self, event: Event<'_>) { match event { Event::Reset => { self.resets += 1; self.stale = std::mem::take(&mut self.best); + self.routes.clear(); + } + Event::Routes { prefix, routes } => { + self.routes_events += 1; + assert!(!routes.is_empty(), "a prefix with no routes is withdrawn"); + self.routes.insert( + prefix, + sorted( + routes + .iter() + .map(|r| (r.peer, r.path_id, Arc::clone(r.attrs), r.best)) + .collect(), + ), + ); } Event::Best { prefix, peer, attrs, + .. } => { self.best_events += 1; self.stale.remove(&prefix); @@ -42,6 +77,7 @@ impl Mirror { Event::Withdrawn { prefix } => { self.withdrawn_events += 1; self.best.remove(&prefix); + self.routes.remove(&prefix); } Event::Synced => { self.synced += 1; @@ -81,6 +117,34 @@ impl Mirror { } } +impl Mirror { + /// For the mirror of a follower of all routes: it holds every route in + /// the table, and knows which is best. + #[track_caller] + fn assert_has_all_routes_of(&self, table: &RibTable) { + let mut expected: HashMap> = HashMap::new(); + table.for_each_route(|r| { + expected.entry(r.prefix).or_default().push(( + r.peer, + r.path_id, + Arc::clone(r.attrs), + r.best, + )); + }); + assert_eq!(self.routes.len(), expected.len(), "prefixes"); + for (prefix, routes) in expected { + let mirrored = self + .routes + .get(&prefix) + .unwrap_or_else(|| panic!("{prefix} is in the table and not in the mirror")); + assert!( + same_routes(mirrored, &sorted(routes)), + "{prefix}: the mirror's routes are not the table's" + ); + } + } +} + fn nth_prefix(n: u32) -> IpPrefix { IpPrefix::new(AddressFamilyId::Ipv4, (n << 8).to_be_bytes(), 24).unwrap() } @@ -149,6 +213,63 @@ fn a_reader_from_now_is_given_the_changes_and_not_the_table() { assert_eq!((best, withdrawn), (vec![new], vec![old])); } +/// A change to a route that is not the best is nothing to a follower of +/// the best routes, and is what a follower of all routes is there for. +#[test] +fn a_follower_of_all_routes_is_told_of_every_change() { + let fixture = Fixture::new(&[external(1), external(2)]); + let p = prefix("10.0.0.0/8"); + fixture.announce(0, &p, &Route::via(&[1])); + + let mut best = fixture.table.reader(); + let mut all = fixture.table.reader_of_all_routes(); + let (mut best_mirror, mut all_mirror) = (Mirror::default(), Mirror::default()); + best_mirror.catch_up(&mut best, 100); + all_mirror.catch_up(&mut all, 100); + assert_eq!((best_mirror.best_events, all_mirror.routes_events), (1, 1)); + + // A worse route from another peer, then two more from that peer under + // other path identifiers, one of them changed, one withdrawn. + fixture.announce(1, &p, &Route::via(&[2, 3, 4])); + fixture.announce_path(1, &p, 7, &Route::via(&[2, 3, 5])); + fixture.announce_path(1, &p, 8, &Route::via(&[2, 3, 6])); + fixture.announce_path(1, &p, 7, &Route::via(&[2, 9, 5])); + fixture.withdraw_path(1, &p, 8); + + assert_eq!( + best.poll(100, |event| panic!("unexpected {event:?}")) + .events, + 0 + ); + all_mirror.catch_up(&mut all, 100); + // All of it between two reads is one event, with the routes as they + // are now. + assert_eq!(all_mirror.routes_events, 2); + assert_eq!(all_mirror.routes[&p].len(), 3); + all_mirror.assert_has_all_routes_of(&fixture.table); + + // The last route going is a withdrawal for both. + fixture.table.remove_peer(fixture.peers[1]); + fixture.withdraw(0, &p); + best_mirror.catch_up(&mut best, 100); + all_mirror.catch_up(&mut all, 100); + assert_eq!( + (best_mirror.withdrawn_events, all_mirror.withdrawn_events), + (1, 1) + ); + all_mirror.assert_has_all_routes_of(&fixture.table); + + // Nobody following all routes: that journal keeps nothing. A change + // that is not of the best route is then recorded nowhere. + drop(all); + fixture.announce(0, &p, &Route::via(&[1])); + fixture.announce(1, &p, &Route::via(&[2, 3])); + let before = fixture.table.journal_len(); + fixture.announce(1, &p, &Route::via(&[2, 4])); + fixture.announce_path(1, &p, 7, &Route::via(&[2, 5])); + assert_eq!(fixture.table.journal_len(), before); +} + /// A follower wants what the table has now, not how it got there. #[test] fn many_changes_to_a_prefix_between_reads_are_one_event() { @@ -396,11 +517,13 @@ enum Op { Announce { peer: usize, prefix: usize, + path_id: u32, route: Route, }, Withdraw { peer: usize, prefix: usize, + path_id: u32, }, Flap { peer: usize, @@ -417,13 +540,17 @@ enum Op { const PEERS: usize = 4; const PREFIXES: usize = 12; -const READERS: usize = 3; +// The first three follow the best routes, the others all routes. +const READERS: usize = 5; +const OF_BEST: usize = 3; fn op() -> impl Strategy { prop_oneof![ - 8 => (0..PEERS, 0..PREFIXES, route()) - .prop_map(|(peer, prefix, route)| Op::Announce { peer, prefix, route }), - 5 => (0..PEERS, 0..PREFIXES).prop_map(|(peer, prefix)| Op::Withdraw { peer, prefix }), + 8 => (0..PEERS, 0..PREFIXES, 0u32..3, route()).prop_map( + |(peer, prefix, path_id, route)| Op::Announce { peer, prefix, path_id, route } + ), + 5 => (0..PEERS, 0..PREFIXES, 0u32..3) + .prop_map(|(peer, prefix, path_id)| Op::Withdraw { peer, prefix, path_id }), 1 => (0..PEERS).prop_map(|peer| Op::Flap { peer }), // Reader 0 is polled rarely: it is the one that falls behind. 4 => (1..READERS, 1usize..6).prop_map(|(reader, budget)| Op::Poll { reader, budget }), @@ -450,41 +577,52 @@ proptest! { let limits = Limits { tidy_at: 3, slack: 1 }; let mut fixture = Fixture::on(RibTable::with_journal_limits(shards, limits), &infos); let prefixes: Vec = (0..PREFIXES as u32).map(nth_prefix).collect(); + let new_reader = |table: &Arc, index: usize| { + if index < OF_BEST { table.reader() } else { table.reader_of_all_routes() } + }; + let check = |index: usize, mirror: &Mirror, table: &RibTable| { + if index < OF_BEST { + mirror.assert_matches(table); + } else { + mirror.assert_has_all_routes_of(table); + } + }; let mut readers: Vec<(Reader, Mirror)> = (0..READERS) - .map(|_| (fixture.table.reader(), Mirror::default())) + .map(|index| (new_reader(&fixture.table, index), Mirror::default())) .collect(); for op in ops { match op { - Op::Announce { peer, prefix, route } => { - fixture.announce(peer, &prefixes[prefix], &route); + Op::Announce { peer, prefix, path_id, route } => { + fixture.announce_path(peer, &prefixes[prefix], path_id, &route); } - Op::Withdraw { peer, prefix } => { - fixture.withdraw(peer, &prefixes[prefix]); + Op::Withdraw { peer, prefix, path_id } => { + fixture.withdraw_path(peer, &prefixes[prefix], path_id); } Op::Flap { peer } => { fixture.table.remove_peer(fixture.peers[peer]); fixture.peers[peer] = fixture.table.add_peer(infos[peer]).unwrap(); } - Op::Poll { reader, budget } => { - let (reader, mirror) = &mut readers[reader]; + Op::Poll { reader: index, budget } => { + let (reader, mirror) = &mut readers[index]; let progress = reader.poll(budget, |event| mirror.apply(event)); if progress.caught_up { - mirror.assert_matches(&fixture.table); + check(index, mirror, &fixture.table); } } Op::Replace { reader } => { - readers[reader] = (fixture.table.reader(), Mirror::default()); + readers[reader] = (new_reader(&fixture.table, reader), Mirror::default()); } } // Never much longer than the table, whatever the readers do. - let bound = shards * (2 * (PREFIXES + limits.slack) + limits.tidy_at + PREFIXES); + // Two journals to a shard. + let bound = 2 * shards * (2 * (PREFIXES + limits.slack) + limits.tidy_at + PREFIXES); prop_assert!(fixture.table.journal_len() <= bound); } - for (reader, mirror) in &mut readers { + for (index, (reader, mirror)) in readers.iter_mut().enumerate() { mirror.catch_up(reader, 5); - mirror.assert_matches(&fixture.table); + check(index, mirror, &fixture.table); prop_assert_eq!(mirror.resets, mirror.synced); } } diff --git a/crates/rib/src/table/mod.rs b/crates/rib/src/table/mod.rs index 5b44a1d..40e7313 100644 --- a/crates/rib/src/table/mod.rs +++ b/crates/rib/src/table/mod.rs @@ -35,14 +35,14 @@ use std::net::{IpAddr, Ipv4Addr}; use std::sync::atomic::{AtomicU64, AtomicUsize}; use std::sync::{Arc, Mutex, PoisonError, RwLock, RwLockReadGuard, RwLockWriteGuard}; -use bgp_primitives::ip_prefix::IpPrefix; +use bgp_primitives::ip_prefix::{IpPrefix, Nlri}; use foldhash::fast::RandomState; use hashbrown::HashTable; use hashbrown::hash_table::Entry; use tokio::sync::Notify; pub use attrs::{AttrSet, AttrStore, SelectionKey}; -pub use journal::{Event, Progress, Reader}; +pub use journal::{Event, Progress, Reader, Routes}; /// What route selection needs to know about a peer. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -127,16 +127,119 @@ struct Row { /// The journal's sequence number for the latest change of this row's /// best route; see [`journal`]. changed: u64, - /// The routes, one from each peer that has announced the prefix, in one - /// run of memory: finding a peer's route and choosing the best are - /// scans of it. + /// The journal's sequence number for the latest change of any of this + /// row's routes, kept only while someone follows all routes. + touched: u64, + /// The routes, in one run of memory: finding a peer's route and + /// choosing the best are scans of it. One from each peer that has + /// announced the prefix, or several from a peer that sends more than one + /// (ADD-PATH, RFC 7911). paths: Vec, + /// The path identifier of each route in `paths`, for telling apart the + /// routes of a peer that sends several. Empty while every route has + /// the identifier zero, which is all routes of peers that send one + /// each: they pay nothing for this. + path_ids: Vec, } impl Row { - fn position(&self, peer: PeerIndex) -> Option { + fn new(prefix: IpPrefix, path: Path, path_id: u32, changed: u64, touched: u64) -> Self { + Self { + prefix, + best: 0, + changed, + touched, + paths: vec![path], + path_ids: if path_id == 0 { vec![] } else { vec![path_id] }, + } + } + + fn path_id(&self, index: usize) -> u32 { + self.path_ids.get(index).copied().unwrap_or(0) + } + + /// Where the route from `peer` with the identifier `path_id` is. + fn position(&self, peer: PeerIndex, path_id: u32) -> Option { + self.paths + .iter() + .enumerate() + .position(|(index, path)| path.peer() == peer && self.path_id(index) == path_id) + } + + /// Where a route from `peer` is, whatever its identifier. + fn position_of_peer(&self, peer: PeerIndex) -> Option { self.paths.iter().position(|path| path.peer() == peer) } + + fn push(&mut self, path: Path, path_id: u32) { + if path_id != 0 && self.path_ids.is_empty() { + self.path_ids = vec![0; self.paths.len()]; + } + push_tight(&mut self.paths, path); + if !self.path_ids.is_empty() { + push_tight(&mut self.path_ids, path_id); + } + } + + /// Takes out the route at `at`; the last route moves into its place. + fn swap_remove(&mut self, at: usize) -> Path { + if !self.path_ids.is_empty() { + self.path_ids.swap_remove(at); + } + self.paths.swap_remove(at) + } + + fn route(&self, index: usize) -> RouteRef<'_> { + let path = &self.paths[index]; + RouteRef { + prefix: self.prefix, + peer: path.peer(), + path_id: self.path_id(index), + attrs: &path.attrs, + best: index == usize::from(self.best), + } + } + + fn routes(&self) -> impl Iterator> { + (0..self.paths.len()).map(|index| self.route(index)) + } +} + +/// A route in the table, looked at in place. +#[derive(Debug, Clone, Copy)] +pub struct RouteRef<'a> { + pub prefix: IpPrefix, + /// The peer the route is from. + pub peer: PeerIndex, + /// The peer's identifier for the route: zero unless the peer sends + /// several routes for a prefix (ADD-PATH, RFC 7911). + pub path_id: u32, + pub attrs: &'a Arc, + /// Whether it is the route chosen for its prefix. + pub best: bool, +} + +impl RouteRef<'_> { + /// The route, to keep. + pub fn to_owned(&self) -> Route { + Route { + prefix: self.prefix, + peer: self.peer, + path_id: self.path_id, + attrs: Arc::clone(self.attrs), + best: self.best, + } + } +} + +/// A route taken out of the table to keep; see [`RouteRef`]. +#[derive(Debug, Clone)] +pub struct Route { + pub prefix: IpPrefix, + pub peer: PeerIndex, + pub path_id: u32, + pub attrs: Arc, + pub best: bool, } struct Shard { @@ -150,6 +253,9 @@ struct Shard { scratch: select::Scratch, /// Which prefixes' best routes have changed, for those who follow. journal: journal::Journal, + /// Which prefixes have had any route change, for those who follow all + /// the routes and not only the best. + all_journal: journal::Journal, } /// The routing table. Shared between threads by reference; every method @@ -196,6 +302,7 @@ impl RibTable { hasher: hasher.clone(), scratch: select::Scratch::default(), journal: journal::Journal::new(limits), + all_journal: journal::Journal::new(limits), }) }) .collect(), @@ -249,7 +356,7 @@ impl RibTable { for shard in &self.shards { let mut shard = write(shard); outcome += shard.remove_peer(peer); - shard.tidy_journal(); + shard.tidy_journals(); if let Some(slot) = shard.peers.get_mut(peer.as_usize()) { *slot = None; } @@ -257,7 +364,7 @@ impl RibTable { if let Some(slot) = peers.get_mut(peer.as_usize()) { *slot = None; } - if outcome.best_changed > 0 { + if outcome != Outcome::default() { self.wake_readers(); } outcome @@ -271,21 +378,40 @@ impl RibTable { peer: PeerIndex, attrs: &Arc, prefixes: &[IpPrefix], + ) -> Outcome { + self.announce_each( + peer, + attrs, + prefixes.iter().map(|prefix| Nlri::from(*prefix)), + ) + } + + /// As [`RibTable::announce`], for a peer that may send several routes + /// for a prefix (ADD-PATH, RFC 7911): each route is known by its prefix + /// and path identifier together, and replaces only the route the peer + /// had announced with both the same. + pub fn announce_paths(&self, peer: PeerIndex, attrs: &Arc, nlri: &[Nlri]) -> Outcome { + self.announce_each(peer, attrs, nlri.iter().copied()) + } + + fn announce_each( + &self, + peer: PeerIndex, + attrs: &Arc, + nlri: impl ExactSizeIterator, ) -> Outcome { let mut outcome = Outcome::default(); - self.by_shard(prefixes, |shard, placed| { + self.by_shard(nlri, |shard, placed| { let Some(info) = shard.peers.get(peer.as_usize()).copied().flatten() else { // The peer was removed while this was on its way. return; }; let preference = select::preference(attrs.key(), &info); - for (hash, prefix) in placed { - if let Some(prefix) = prefix { - outcome += shard.announce(*hash, *prefix, peer, preference, attrs); - } + for (hash, nlri) in placed { + outcome += shard.announce(*hash, *nlri, peer, preference, attrs); } }); - if outcome.best_changed > 0 { + if outcome != Outcome::default() { self.wake_readers(); } outcome @@ -294,48 +420,62 @@ impl RibTable { /// Withdraws `peer`'s routes for `prefixes`. Withdrawing a route that /// is not there does nothing. pub fn withdraw(&self, peer: PeerIndex, prefixes: &[IpPrefix]) -> Outcome { + self.withdraw_each(peer, prefixes.iter().map(|prefix| Nlri::from(*prefix))) + } + + /// As [`RibTable::withdraw`], for routes known by prefix and path + /// identifier (ADD-PATH, RFC 7911). + pub fn withdraw_paths(&self, peer: PeerIndex, nlri: &[Nlri]) -> Outcome { + self.withdraw_each(peer, nlri.iter().copied()) + } + + fn withdraw_each(&self, peer: PeerIndex, nlri: impl ExactSizeIterator) -> Outcome { let mut outcome = Outcome::default(); - self.by_shard(prefixes, |shard, placed| { - for (hash, prefix) in placed { - if let Some(prefix) = prefix { - outcome += shard.withdraw(*hash, prefix, peer); - } + self.by_shard(nlri, |shard, placed| { + for (hash, nlri) in placed { + outcome += shard.withdraw(*hash, nlri, peer); } }); - if outcome.best_changed > 0 { + if outcome != Outcome::default() { self.wake_readers(); } outcome } - /// Calls `apply` once for each shard that some of `prefixes` fall in, - /// with that shard locked and those prefixes. + /// Calls `apply` once for each shard that some of `nlri` fall in, with + /// that shard locked and those prefixes. fn by_shard( &self, - prefixes: &[IpPrefix], - mut apply: impl FnMut(&mut Shard, &[(u64, Option)]), + nlri: impl ExactSizeIterator, + mut apply: impl FnMut(&mut Shard, &[(u64, Nlri)]), ) { let shards = self.shards.len() as u64; // The high bits pick the shard; the table inside uses the low ones. let shard_of = |hash: u64| ((hash >> 32) * shards) >> 32; + let hashed = |nlri: Nlri| (self.hasher.hash_one(nlri.prefix), nlri); // An UPDATE has a handful of prefixes; they are sorted on the stack. const INLINE: usize = 32; - let mut inline = [(0u64, prefixes.first().copied()); INLINE]; + let count = nlri.len(); + let mut nlri = nlri.peekable(); + let Some(first) = nlri.peek().copied() else { + return; + }; + let mut inline = [(0u64, first); INLINE]; let mut spilled = Vec::new(); - let placed: &mut [(u64, Option)] = if prefixes.len() <= INLINE { - &mut inline[..prefixes.len()] + let placed: &mut [(u64, Nlri)] = if count <= INLINE { + for (slot, nlri) in inline.iter_mut().zip(nlri) { + *slot = hashed(nlri); + } + &mut inline[..count] } else { - spilled.resize(prefixes.len(), (0, None)); + spilled.extend(nlri.map(hashed)); &mut spilled }; - for (slot, prefix) in placed.iter_mut().zip(prefixes) { - *slot = (self.hasher.hash_one(prefix), Some(*prefix)); - } placed.sort_unstable_by_key(|(hash, _)| shard_of(*hash)); for run in placed.chunk_by(|a, b| shard_of(a.0) == shard_of(b.0)) { let mut shard = write(&self.shards[shard_of(run[0].0) as usize]); apply(&mut shard, run); - shard.tidy_journal(); + shard.tidy_journals(); } } @@ -361,22 +501,10 @@ impl RibTable { }) } - /// Every route for `prefix`, with whether it is the best one. - pub fn routes(&self, prefix: &IpPrefix) -> Vec<(PeerIndex, Arc, bool)> { - self.with_row(prefix, |row| { - row.paths - .iter() - .enumerate() - .map(|(i, path)| { - ( - path.peer(), - Arc::clone(&path.attrs), - i == usize::from(row.best), - ) - }) - .collect() - }) - .unwrap_or_default() + /// Every route for `prefix`. + pub fn routes(&self, prefix: &IpPrefix) -> Vec { + self.with_row(prefix, |row| row.routes().map(|r| r.to_owned()).collect()) + .unwrap_or_default() } fn with_row(&self, prefix: &IpPrefix, read_row: impl FnOnce(&Row) -> T) -> Option { @@ -401,43 +529,30 @@ impl RibTable { } } - /// Up to `limit` routes, with whether each is its prefix's best: every - /// route of as many prefixes as fit, in no particular order. For showing - /// some of a table that is too large to show all of. - pub fn some_routes(&self, limit: usize) -> Vec<(IpPrefix, PeerIndex, Arc, bool)> { + /// Up to `limit` routes: every route of as many prefixes as fit, in no + /// particular order. For showing some of a table that is too large to + /// show all of. + pub fn some_routes(&self, limit: usize) -> Vec { let mut routes = Vec::new(); for shard in &self.shards { for row in &read(shard).rows { - for (i, path) in row.paths.iter().enumerate() { + for route in row.routes() { if routes.len() == limit { return routes; } - routes.push(( - row.prefix, - path.peer(), - Arc::clone(&path.attrs), - i == usize::from(row.best), - )); + routes.push(route.to_owned()); } } } routes } - /// Calls `visit` with every route in the table and whether it is its - /// prefix's best. As [`RibTable::for_each_best`], without stopping the - /// table. - pub fn for_each_route(&self, mut visit: impl FnMut(&IpPrefix, PeerIndex, &Arc, bool)) { + /// Calls `visit` with every route in the table. As + /// [`RibTable::for_each_best`], without stopping the table. + pub fn for_each_route(&self, mut visit: impl FnMut(RouteRef<'_>)) { for shard in &self.shards { for row in &read(shard).rows { - for (i, path) in row.paths.iter().enumerate() { - visit( - &row.prefix, - path.peer(), - &path.attrs, - i == usize::from(row.best), - ); - } + row.routes().for_each(&mut visit); } } } @@ -467,11 +582,12 @@ impl Shard { fn announce( &mut self, hash: u64, - prefix: IpPrefix, + nlri: Nlri, peer: PeerIndex, preference: u64, attrs: &Arc, ) -> Outcome { + let Nlri { prefix, path_id } = nlri; let hasher = &self.hasher; let entry = self.rows.entry( hash, @@ -481,12 +597,9 @@ impl Shard { let path = Path::new(preference, peer, attrs); let row = match entry { Entry::Vacant(vacant) => { - vacant.insert(Row { - prefix, - best: 0, - changed: self.journal.appeared(prefix), - paths: vec![path], - }); + let changed = self.journal.appeared(prefix); + let touched = self.all_journal.appeared(prefix); + vacant.insert(Row::new(prefix, path, path_id, changed, touched)); self.routes += 1; return Outcome { added: 1, @@ -499,9 +612,9 @@ impl Shard { let best = usize::from(row.best); let best_before = row.paths[best].preference(); - match row.position(peer) { + let (outcome, best_changed) = match row.position(peer, path_id) { None => { - push_tight(&mut row.paths, path); + row.push(path, path_id); self.routes += 1; let added = row.paths.len() - 1; // Better than the best in the first steps of selection is @@ -520,20 +633,21 @@ impl Shard { } else { select::choose(row, &self.peers, &mut self.scratch) }; - if best_changed { - row.changed = self.journal.best_changed(prefix); - } - Outcome { + let outcome = Outcome { added: 1, - best_changed: best_changed.into(), ..Outcome::default() - } + }; + (outcome, best_changed) } Some(at) => { let before = std::mem::replace(&mut row.paths[at], path); let same = Arc::ptr_eq(&before.attrs, attrs) && before.preference() == preference; let best_changed = if same { - false + // Nothing changed at all. + return Outcome { + replaced: 1, + ..Outcome::default() + }; } else if at == best { // The best route itself changed. If it got better in // the first steps it is still the best, but it is a @@ -551,30 +665,37 @@ impl Shard { } else { select::choose(row, &self.peers, &mut self.scratch) }; - if best_changed { - row.changed = self.journal.best_changed(prefix); - } - Outcome { + let outcome = Outcome { replaced: 1, - best_changed: best_changed.into(), ..Outcome::default() - } + }; + (outcome, best_changed) } + }; + if best_changed { + row.changed = self.journal.changed(prefix); + } + row.touched = self.all_journal.changed(prefix); + Outcome { + best_changed: best_changed.into(), + ..outcome } } - fn withdraw(&mut self, hash: u64, prefix: &IpPrefix, peer: PeerIndex) -> Outcome { + fn withdraw(&mut self, hash: u64, nlri: &Nlri, peer: PeerIndex) -> Outcome { + let prefix = &nlri.prefix; let Ok(mut entry) = self.rows.find_entry(hash, |row| row.prefix == *prefix) else { return Outcome::default(); }; let row = entry.get_mut(); - let Some(at) = row.position(peer) else { + let Some(at) = row.position(peer, nlri.path_id) else { return Outcome::default(); }; self.routes -= 1; if row.paths.len() == 1 { entry.remove(); self.journal.disappeared(*prefix); + self.all_journal.disappeared(*prefix); return Outcome { removed: 1, best_changed: 1, @@ -583,8 +704,9 @@ impl Shard { } let best_changed = Self::remove_path(row, at, &self.peers, &mut self.scratch); if best_changed { - row.changed = self.journal.best_changed(*prefix); + row.changed = self.journal.changed(*prefix); } + row.touched = self.all_journal.changed(*prefix); Outcome { removed: 1, best_changed: best_changed.into(), @@ -603,7 +725,7 @@ impl Shard { let best = usize::from(row.best); let best_before = row.paths[best].preference(); let last = row.paths.len() - 1; - let removed = row.paths.swap_remove(at); + let removed = row.swap_remove(at); if at == best { select::choose(row, peers, scratch); return true; @@ -626,19 +748,25 @@ impl Shard { let peers = &self.peers; let scratch = &mut self.scratch; let journal = &mut self.journal; + let all_journal = &mut self.all_journal; self.rows.retain(|row| { - let Some(at) = row.position(peer) else { - return true; - }; - outcome.removed += 1; - if row.paths.len() == 1 { - outcome.best_changed += 1; - journal.disappeared(row.prefix); - return false; + let mut best_changed = false; + // A peer that sends several routes for a prefix has several + // here. + while let Some(at) = row.position_of_peer(peer) { + outcome.removed += 1; + if row.paths.len() == 1 { + outcome.best_changed += 1; + journal.disappeared(row.prefix); + all_journal.disappeared(row.prefix); + return false; + } + best_changed |= Self::remove_path(row, at, peers, scratch); + row.touched = all_journal.changed(row.prefix); } - if Self::remove_path(row, at, peers, scratch) { + if best_changed { outcome.best_changed += 1; - row.changed = journal.best_changed(row.prefix); + row.changed = journal.changed(row.prefix); } true }); diff --git a/crates/rib/src/table/select.rs b/crates/rib/src/table/select.rs index 29d93b4..f841eb7 100644 --- a/crates/rib/src/table/select.rs +++ b/crates/rib/src/table/select.rs @@ -126,25 +126,29 @@ pub(super) fn settles_tie(row: &Row, added: usize, peers: &[Option]) - Some(TieBreak::of(row, added as u16, peers) < TieBreak::of(row, row.best, peers)) } -/// Steps 5 to 7, and then the peer's number so that the choice never -/// depends on the order routes arrived in. The least wins. +/// Steps 5 to 7, then the peer's number, and then the path identifier for +/// a peer that sent several routes: so that the choice never depends on the +/// order routes arrived in. The least wins. #[derive(PartialEq, Eq, PartialOrd, Ord)] struct TieBreak { internal: bool, router_id: u32, address: std::net::IpAddr, peer: u16, + path_id: u32, } impl TieBreak { fn of(row: &Row, index: u16, peers: &[Option]) -> Self { let peer = row.paths[usize::from(index)].peer(); + let path_id = row.path_id(usize::from(index)); match peers.get(peer.as_usize()).copied().flatten() { Some(info) => Self { internal: info.ibgp, router_id: info.router_id.into(), address: info.address, peer: peer.0, + path_id, }, // A peer on its way out; anything known is preferred to it. None => Self { @@ -152,6 +156,7 @@ impl TieBreak { router_id: u32::MAX, address: std::net::Ipv6Addr::from(u128::MAX).into(), peer: peer.0, + path_id, }, } } diff --git a/crates/rib/src/table/tests.rs b/crates/rib/src/table/tests.rs index 7379c44..762c31b 100644 --- a/crates/rib/src/table/tests.rs +++ b/crates/rib/src/table/tests.rs @@ -6,7 +6,7 @@ use bgp_packet::message::{ AsPathAttribute, LocalPrefPathAttribute, MultiExitDiscPathAttribute, NextHopPathAttribute, OriginPathAttribute, PathAttribute, }; -use bgp_primitives::ip_prefix::IpPrefix; +use bgp_primitives::ip_prefix::{IpPrefix, Nlri}; use proptest::prelude::*; use super::*; @@ -105,6 +105,58 @@ impl Fixture { self.table.withdraw(self.peers[peer], &[*prefix]) } + /// As `announce`, from a peer that sends several routes for a prefix + /// and calls this one `path_id`. + pub(super) fn announce_path( + &self, + peer: usize, + prefix: &IpPrefix, + path_id: u32, + route: &Route, + ) -> Outcome { + let attrs = self + .table + .attrs() + .intern(&route.attributes(), None) + .unwrap(); + let nlri = Nlri { + prefix: *prefix, + path_id, + }; + self.table.announce_paths(self.peers[peer], &attrs, &[nlri]) + } + + pub(super) fn withdraw_path(&self, peer: usize, prefix: &IpPrefix, path_id: u32) -> Outcome { + let nlri = Nlri { + prefix: *prefix, + path_id, + }; + self.table.withdraw_paths(self.peers[peer], &[nlri]) + } + + /// The best route for `prefix`: which peer (by order added) it is from + /// and that peer's identifier for it. + pub(super) fn best_path(&self, prefix: &IpPrefix) -> Option<(usize, u32)> { + let best = self.table.routes(prefix).into_iter().find(|r| r.best)?; + let peer = self.peers.iter().position(|p| *p == best.peer)?; + Some((peer, best.path_id)) + } + + /// The routes held for `prefix`, as (peer, path identifier), in order. + pub(super) fn held(&self, prefix: &IpPrefix) -> Vec<(usize, u32)> { + let mut held: Vec<(usize, u32)> = self + .table + .routes(prefix) + .iter() + .map(|r| { + let peer = self.peers.iter().position(|p| *p == r.peer).unwrap(); + (peer, r.path_id) + }) + .collect(); + held.sort_unstable(); + held + } + /// Which peer (by order added) the best route for `prefix` is from. fn best(&self, prefix: &IpPrefix) -> Option { let (peer, _) = self.table.best(prefix)?; @@ -397,9 +449,9 @@ fn readers_see_every_route_and_which_is_best() { let mut all = 0; let mut marked_best = 0; - fixture.table.for_each_route(|_, _, _, is_best| { + fixture.table.for_each_route(|route| { all += 1; - marked_best += usize::from(is_best); + marked_best += usize::from(route.best); }); assert_eq!((all, marked_best), (3, 2)); @@ -408,15 +460,66 @@ fn readers_see_every_route_and_which_is_best() { assert!( routes .iter() - .any(|(peer, _, best)| *peer == fixture.peers[1] && *best) + .any(|route| route.peer == fixture.peers[1] && route.best) ); // Routes with the same attributes share one set. - let (_, shared, _) = &fixture.table.routes(&b)[0]; - assert!( - routes - .iter() - .any(|(_, attrs, _)| Arc::ptr_eq(attrs, shared)) + let shared = &fixture.table.routes(&b)[0].attrs; + assert!(routes.iter().any(|route| Arc::ptr_eq(&route.attrs, shared))); +} + +/// With ADD-PATH a peer sends several routes for one prefix and tells them +/// apart by a path identifier. Each is a route of its own: kept, replaced +/// and withdrawn by prefix and identifier together. +#[test] +fn a_peer_can_have_several_routes_for_a_prefix() { + let fixture = Fixture::new(&[external(1), external(2)]); + let p = prefix("10.0.0.0/8"); + + fixture.announce_path(0, &p, 1, &Route::via(&[1, 2, 3])); + let second = fixture.announce_path(0, &p, 2, &Route::via(&[1, 4])); + assert_eq!( + (second.added, second.replaced, second.best_changed), + (1, 0, 1) ); + fixture.announce(1, &p, &Route::via(&[2, 5, 6, 7])); + assert_eq!(fixture.held(&p), [(0, 1), (0, 2), (1, 0)]); + assert_eq!(fixture.best_path(&p), Some((0, 2)), "the shorter path"); + assert_eq!((fixture.table.len(), fixture.table.prefix_count()), (3, 1)); + + // Announcing a path again replaces that path and no other. + let replaced = fixture.announce_path(0, &p, 1, &Route::via(&[1])); + assert_eq!((replaced.added, replaced.replaced), (0, 1)); + assert_eq!(fixture.held(&p), [(0, 1), (0, 2), (1, 0)]); + assert_eq!(fixture.best_path(&p), Some((0, 1))); + + // Withdrawing one leaves the peer's other. + assert_eq!(fixture.withdraw_path(0, &p, 1).removed, 1); + assert_eq!(fixture.held(&p), [(0, 2), (1, 0)]); + assert_eq!(fixture.best_path(&p), Some((0, 2))); + // A path that is not there is not withdrawn: not the same prefix + // under another identifier, and not the peer's plain route. + assert_eq!(fixture.withdraw_path(0, &p, 9), Outcome::default()); + assert_eq!(fixture.withdraw(0, &p), Outcome::default()); + + // When the session ends, all of the peer's routes go. + fixture.announce_path(0, &p, 3, &Route::via(&[1, 8, 9])); + let outcome = fixture.table.remove_peer(fixture.peers[0]); + assert_eq!((outcome.removed, outcome.best_changed), (2, 1)); + assert_eq!(fixture.held(&p), [(1, 0)]); +} + +/// Two routes from one peer that nothing else tells apart: the lower path +/// identifier is chosen, whichever arrived first. +#[test] +fn equal_routes_from_one_peer_are_chosen_by_path_identifier() { + let p = prefix("10.0.0.0/8"); + for order in [[5, 3], [3, 5]] { + let fixture = Fixture::new(&[external(1)]); + for path_id in order { + fixture.announce_path(0, &p, path_id, &Route::via(&[1, path_id])); + } + assert_eq!(fixture.best_path(&p), Some((0, 3)), "arriving {order:?}"); + } } // --------------------------------------------------------------------------- @@ -425,7 +528,11 @@ fn readers_see_every_route_and_which_is_best() { /// Route selection written the plain way: all routes of a prefix, step by /// step, with no shortcuts. The table takes shortcuts and must agree. -fn chosen_plainly(routes: &BTreeMap, peers: &[PeerInfo]) -> Option { +/// Routes are known by peer and path identifier. +fn chosen_plainly( + routes: &BTreeMap<(usize, u32), Route>, + peers: &[PeerInfo], +) -> Option<(usize, u32)> { let local_pref = |peer: usize, route: &Route| { if peers[peer].ibgp { route.local_pref.unwrap_or(DEFAULT_LOCAL_PREF) @@ -433,10 +540,10 @@ fn chosen_plainly(routes: &BTreeMap, peers: &[PeerInfo]) -> Option DEFAULT_LOCAL_PREF } }; - let mut left: Vec<(usize, &Route)> = - routes.iter().map(|(peer, route)| (*peer, route)).collect(); - let most = left.iter().map(|(p, r)| local_pref(*p, r)).max()?; - left.retain(|(p, r)| local_pref(*p, r) == most); + let mut left: Vec<((usize, u32), &Route)> = + routes.iter().map(|(k, route)| (*k, route)).collect(); + let most = left.iter().map(|(k, r)| local_pref(k.0, r)).max()?; + left.retain(|(k, r)| local_pref(k.0, r) == most); let shortest = left.iter().map(|(_, r)| r.path.len()).min()?; left.retain(|(_, r)| r.path.len() == shortest); let lowest = left.iter().map(|(_, r)| r.origin).min()?; @@ -451,14 +558,17 @@ fn chosen_plainly(routes: &BTreeMap, peers: &[PeerInfo]) -> Option } left.retain(|(_, r)| med(r) == lowest_med[&neighbor(r)]); - left.into_iter().map(|(peer, _)| peer).min_by_key(|&peer| { - ( - peers[peer].ibgp, - peers[peer].router_id, - peers[peer].address, - peer, - ) - }) + left.into_iter() + .map(|(key, _)| key) + .min_by_key(|&(peer, path_id)| { + ( + peers[peer].ibgp, + peers[peer].router_id, + peers[peer].address, + peer, + path_id, + ) + }) } #[derive(Debug, Clone)] @@ -466,21 +576,24 @@ enum Op { Announce { peer: usize, prefix: usize, + path_id: u32, route: Route, }, Withdraw { peer: usize, prefix: usize, + path_id: u32, }, /// The session goes down and comes straight back. - Flap { - peer: usize, - }, + Flap { peer: usize }, } const PEERS: usize = 6; // Few prefixes, so that each gathers routes from most peers. const PREFIXES: usize = 2; +// Most routes have the identifier zero, as without ADD-PATH; some peers +// send a second and third route. +const PATH_IDS: u32 = 3; pub(super) fn route() -> impl Strategy { // Few values of everything, so that ties at every step are common: @@ -499,11 +612,17 @@ pub(super) fn route() -> impl Strategy { }) } +fn path_id() -> impl Strategy { + prop_oneof![3 => Just(0), 2 => 1..PATH_IDS] +} + fn op() -> impl Strategy { prop_oneof![ - 6 => (0..PEERS, 0..PREFIXES, route()) - .prop_map(|(peer, prefix, route)| Op::Announce { peer, prefix, route }), - 3 => (0..PEERS, 0..PREFIXES).prop_map(|(peer, prefix)| Op::Withdraw { peer, prefix }), + 6 => (0..PEERS, 0..PREFIXES, path_id(), route()).prop_map( + |(peer, prefix, path_id, route)| Op::Announce { peer, prefix, path_id, route } + ), + 3 => (0..PEERS, 0..PREFIXES, path_id()) + .prop_map(|(peer, prefix, path_id)| Op::Withdraw { peer, prefix, path_id }), 1 => (0..PEERS).prop_map(|peer| Op::Flap { peer }), ] } @@ -528,27 +647,25 @@ proptest! { let prefixes: Vec = (0..PREFIXES) .map(|i| prefix(&format!("10.{i}.0.0/16"))) .collect(); - let mut model: Vec> = vec![BTreeMap::new(); PREFIXES]; + let mut model: Vec> = vec![BTreeMap::new(); PREFIXES]; for op in ops { - let before: Vec> = model + let before: Vec> = model .iter() - .map(|routes| { - chosen_plainly(routes, &infos).map(|peer| (peer, routes[&peer].clone())) - }) + .map(|routes| chosen_plainly(routes, &infos).map(|key| (key, routes[&key].clone()))) .collect(); let outcome = match &op { - Op::Announce { peer, prefix, route } => { - model[*prefix].insert(*peer, route.clone()); - fixture.announce(*peer, &prefixes[*prefix], route) + Op::Announce { peer, prefix, path_id, route } => { + model[*prefix].insert((*peer, *path_id), route.clone()); + fixture.announce_path(*peer, &prefixes[*prefix], *path_id, route) } - Op::Withdraw { peer, prefix } => { - model[*prefix].remove(peer); - fixture.withdraw(*peer, &prefixes[*prefix]) + Op::Withdraw { peer, prefix, path_id } => { + model[*prefix].remove(&(*peer, *path_id)); + fixture.withdraw_path(*peer, &prefixes[*prefix], *path_id) } Op::Flap { peer } => { for routes in &mut model { - routes.remove(peer); + routes.retain(|(from, _), _| from != peer); } let outcome = fixture.table.remove_peer(fixture.peers[*peer]); fixture.peers[*peer] = fixture.table.add_peer(infos[*peer]).unwrap(); @@ -559,18 +676,13 @@ proptest! { let mut best_changes = 0; for (i, routes) in model.iter().enumerate() { let expected = chosen_plainly(routes, &infos); - prop_assert_eq!(fixture.best(&prefixes[i]), expected, "after {:?}", &op); - let now = expected.map(|peer| (peer, routes[&peer].clone())); + prop_assert_eq!(fixture.best_path(&prefixes[i]), expected, "after {:?}", &op); + let now = expected.map(|key| (key, routes[&key].clone())); best_changes += usize::from(now != before[i]); - - let mut held: Vec = fixture - .table - .routes(&prefixes[i]) - .iter() - .map(|(peer, _, _)| fixture.peers.iter().position(|p| p == peer).unwrap()) - .collect(); - held.sort_unstable(); - prop_assert_eq!(held, routes.keys().copied().collect::>()); + prop_assert_eq!( + fixture.held(&prefixes[i]), + routes.keys().copied().collect::>() + ); } prop_assert_eq!(outcome.best_changed, best_changes, "after {:?}", &op); prop_assert_eq!( diff --git a/crates/server/proto/config.proto b/crates/server/proto/config.proto index af33453..73ca2d6 100644 --- a/crates/server/proto/config.proto +++ b/crates/server/proto/config.proto @@ -97,6 +97,22 @@ message Peer { // it too (RFC 4760). Defaults to all of them when empty. A session with // no family in common is refused. repeated AddressFamily address_families = 6; + + // ADD-PATH (RFC 7911): whether to take several routes for one prefix + // from this peer, to send it several, or both. It is in use, for each + // direction and address family, only if the peer agrees. Defaults to + // off: one route for each prefix, as in plain BGP. + AddPath add_path = 7; +} + +// The directions ADD-PATH can be offered for. +enum AddPath { + ADD_PATH_OFF = 0; + // Accept several routes for a prefix from the peer. + ADD_PATH_RECEIVE = 1; + // Send the peer several routes for a prefix. + ADD_PATH_SEND = 2; + ADD_PATH_BOTH = 3; } // An address family routes can be exchanged for. diff --git a/crates/server/src/advertise.rs b/crates/server/src/advertise.rs index 8aa81c3..61def4d 100644 --- a/crates/server/src/advertise.rs +++ b/crates/server/src/advertise.rs @@ -15,7 +15,7 @@ use bgp_packet::message::{ NextHopPathAttribute, NlriNextHop, PathAttribute, UpdateMessage, }; use bgp_primitives::constants::{AddressFamilyId, Afi, Safi}; -use bgp_primitives::ip_prefix::IpPrefix; +use bgp_primitives::ip_prefix::{IpPrefix, Nlri}; use bgp_rib::{LocalRib, LocalRibEvent, LocalRoute, RibStopped}; use tokio::sync::broadcast::error::RecvError; use tokio::task::JoinHandle; @@ -181,7 +181,7 @@ pub fn build_announce( match (route.prefix.address_family(), next_hop) { (AddressFamilyId::Ipv4, IpAddr::V4(nh)) => { path_attributes.push(PathAttribute::NextHop(NextHopPathAttribute(nh))); - announced_routes.push(route.prefix); + announced_routes.push(route.prefix.into()); } (AddressFamilyId::Ipv6, IpAddr::V6(_)) => {} _ => return None, @@ -202,7 +202,7 @@ pub fn build_announce( afi: Afi::IPV6, safi: Safi::UNICAST, next_hop: NlriNextHop::Ipv6(nh), - prefixes: vec![route.prefix], + prefixes: vec![route.prefix.into()], })); } path_attributes.extend(as4_path); @@ -225,7 +225,7 @@ pub fn build_announce( pub fn build_withdraw(prefix: &IpPrefix) -> UpdateMessage { match prefix.address_family() { AddressFamilyId::Ipv4 => UpdateMessage { - withdrawn_routes: vec![*prefix], + withdrawn_routes: vec![Nlri::from(*prefix)], path_attributes: vec![], announced_routes: vec![], }, @@ -234,7 +234,7 @@ pub fn build_withdraw(prefix: &IpPrefix) -> UpdateMessage { path_attributes: vec![PathAttribute::MpUnreachNlri(MpUnreachNlriPathAttribute { afi: Afi::IPV6, safi: Safi::UNICAST, - prefixes: vec![*prefix], + prefixes: vec![Nlri::from(*prefix)], })], announced_routes: vec![], }, @@ -302,6 +302,7 @@ mod tests { let ctx = ParserContext { four_octet_asn: Some(true), ibgp: false, + add_path: Vec::new(), }; let msg = Message::Update(update.clone()); let mut buf = BytesMut::new(); @@ -360,6 +361,7 @@ mod tests { let ctx = ParserContext { four_octet_asn: Some(false), ibgp: false, + add_path: Vec::new(), }; let mut buf = BytesMut::new(); Message::Update(update).to_wire(&ctx, &mut buf).unwrap(); diff --git a/crates/server/src/config.rs b/crates/server/src/config.rs index de1c58d..b22ebfb2 100644 --- a/crates/server/src/config.rs +++ b/crates/server/src/config.rs @@ -4,6 +4,7 @@ use std::time::Duration; use bgp_packet::community; use bgp_packet::message::OriginPathAttribute; +use bgp_packet::open::AddPathSendRecv; use bgp_primitives::constants::AfiSafi; use bgp_primitives::ip_prefix::IpPrefix; use bgp_rib::{LocalRoute, LocalRouteAttrs}; @@ -49,6 +50,9 @@ pub struct PeerConfig { pub local_address: Option, /// The address families offered to this peer. pub families: FamilySet, + /// The directions ADD-PATH (RFC 7911) is offered to this peer for, on + /// every one of `families`. `None` offers it for neither. + pub add_path: Option, } impl Config { @@ -172,6 +176,14 @@ fn parse_peer(peer: Peer) -> Result { .collect::>()? }; + let add_path = match peer.add_path.enum_value() { + Ok(proto::AddPath::ADD_PATH_OFF) => None, + Ok(proto::AddPath::ADD_PATH_RECEIVE) => Some(AddPathSendRecv::Receive), + Ok(proto::AddPath::ADD_PATH_SEND) => Some(AddPathSendRecv::Send), + Ok(proto::AddPath::ADD_PATH_BOTH) => Some(AddPathSendRecv::Both), + Err(_) => return Err(PeerProblem::AddPath), + }; + Ok(PeerConfig { name: peer.name, endpoint, @@ -179,6 +191,7 @@ fn parse_peer(peer: Peer) -> Result { hold_time_secs, local_address, families, + add_path, }) } @@ -339,6 +352,27 @@ mod tests { ); } + #[test] + fn add_path_is_off_unless_a_peer_asks_for_it() { + let add_path = |body: &str| { + let config = parse(&format!( + "{BASE}peers {{ name: \"r1\" endpoint {{ address: \"192.0.2.2\" }} remote_asn: 1 {body} }}" + )) + .unwrap(); + config.peers[0].add_path + }; + assert_eq!(add_path(""), None); + assert_eq!(add_path("add_path: ADD_PATH_OFF"), None); + assert_eq!( + add_path("add_path: ADD_PATH_RECEIVE"), + Some(AddPathSendRecv::Receive) + ); + assert_eq!( + add_path("add_path: ADD_PATH_BOTH"), + Some(AddPathSendRecv::Both) + ); + } + #[test] fn bad_peers_and_endpoints_are_rejected_with_the_peer_named() { assert_eq!( diff --git a/crates/server/src/connection.rs b/crates/server/src/connection.rs index 7616c0d..bb28153 100644 --- a/crates/server/src/connection.rs +++ b/crates/server/src/connection.rs @@ -16,6 +16,7 @@ use bgp_packet::errors::{BgpError, DecodeError, MsgHeaderSubcode}; use bgp_packet::message::{ BGP_HEADER_LEN, BGP_MAX_MSG_LEN, Decoded, Message, PathAttribute, UpdateMessage, }; +use bgp_packet::open::AddPathSendRecv; use bgp_packet::update_decode::UpdateError; use bgp_primitives::parser::ParserContext; use bgp_rib::PeerId; @@ -63,6 +64,8 @@ pub struct SessionConfig { pub expected_asn: Option, /// The address families to offer the peer. pub families: FamilySet, + /// The directions to offer the peer ADD-PATH for (RFC 7911). + pub add_path: Option, } /// Runs a session on an accepted connection and logs how it ended. @@ -126,12 +129,14 @@ impl Session { hold_time_secs: config.hold_time_secs, expected_asn: config.expected_asn, families: config.families.clone(), + add_path: config.add_path, }); let session = Session { fsm, frames: FrameReader::new(reader), writer, ctx: ParserContext::default(), + send_ctx: ParserContext::default(), hold_timer: None, keepalive_timer: None, collision: None, @@ -230,9 +235,14 @@ struct Session { fsm: Fsm, frames: FrameReader, writer: OwnedWriteHalf, - /// How messages are encoded and decoded: the width of AS numbers is - /// known once the OPENs are exchanged. + /// How messages from the peer are decoded: the width of AS numbers and + /// where prefixes carry path identifiers are known once the OPENs are + /// exchanged. ctx: ParserContext, + /// How messages to the peer are encoded. Its own context: whether path + /// identifiers are sent is settled apart from whether they are + /// received (RFC 7911 §4). + send_ctx: ParserContext, hold_timer: Option>>, keepalive_timer: Option, /// This connection's slot in the collision table, once admitted. @@ -344,8 +354,15 @@ impl Session { self.keepalive_timer = Some(interval_at(Instant::now() + every, every)); } Action::Admit(negotiated) => { - self.ctx.four_octet_asn = Some(negotiated.four_octet_asn); - self.ctx.ibgp = negotiated.peer_asn == self.config.local_asn; + self.ctx = ParserContext { + four_octet_asn: Some(negotiated.four_octet_asn), + ibgp: negotiated.peer_asn == self.config.local_asn, + add_path: negotiated.add_path_receive.iter().collect(), + }; + self.send_ctx = ParserContext { + add_path: negotiated.add_path_send.iter().collect(), + ..self.ctx.clone() + }; match self.config.collisions.admit( self.config.router_id, negotiated.peer_identifier, @@ -359,6 +376,8 @@ impl Session { negotiated_hold_time = negotiated.hold_time_secs, four_octet_asn = negotiated.four_octet_asn, families = %negotiated.families, + add_path_receive = %negotiated.add_path_receive, + add_path_send = %negotiated.add_path_send, direction = self.config.direction.as_str(), "OPEN received, transitioning to OpenConfirm" ); @@ -469,7 +488,7 @@ impl Session { info!(peer_addr = %self.peer_addr, error = %notification.error, "Sending NOTIFICATION, closing session"); } let mut buf = BytesMut::new(); - msg.to_wire(&self.ctx, &mut buf)?; + msg.to_wire(&self.send_ctx, &mut buf)?; self.writer.write_all(&buf).await?; MESSAGES_SENT .with_label_values(&[message_type_name(msg)]) @@ -713,6 +732,7 @@ mod tests { peer_name: None, expected_asn: None, families: FamilySet::supported(), + add_path: None, }; let (mut session, _) = Session::open(stream, peer_addr, config).unwrap(); @@ -750,7 +770,9 @@ mod tests { withdrawn_routes: vec![], path_attributes: vec![], announced_routes: vec![ - bgp_primitives::ip_prefix::IpPrefix::try_from("10.0.0.0/8").unwrap(), + bgp_primitives::ip_prefix::IpPrefix::try_from("10.0.0.0/8") + .unwrap() + .into(), ], }; routes::apply(&table, peer, &update, None).unwrap(); diff --git a/crates/server/src/dialer.rs b/crates/server/src/dialer.rs index 5ae3bee..2100dde 100644 --- a/crates/server/src/dialer.rs +++ b/crates/server/src/dialer.rs @@ -38,6 +38,7 @@ pub fn spawn_dialers( peer_name: Some(peer.name.clone()), expected_asn: Some(peer.remote_asn), families: peer.families.clone(), + add_path: peer.add_path, ..base.clone() }; tokio::spawn(dial_loop(Arc::new(peer.clone()), config, connect_retry)) diff --git a/crates/server/src/error.rs b/crates/server/src/error.rs index 7f2547d..9639006 100644 --- a/crates/server/src/error.rs +++ b/crates/server/src/error.rs @@ -156,6 +156,8 @@ pub enum PeerProblem { AddressFamilyMismatch, #[error("address_families has a value that is not an address family")] AddressFamily, + #[error("add_path has a value that is not one of its settings")] + AddPath, } /// What is wrong with one `originate` entry. diff --git a/crates/server/src/family.rs b/crates/server/src/family.rs index c45e28c..dbc6913 100644 --- a/crates/server/src/family.rs +++ b/crates/server/src/family.rs @@ -253,22 +253,25 @@ mod tests { fn mixed_update() -> UpdateMessage { UpdateMessage { - withdrawn_routes: vec![prefix("198.51.100.0/24")], + withdrawn_routes: vec![prefix("198.51.100.0/24").into()], path_attributes: vec![ PathAttribute::Origin(OriginPathAttribute::IGP), PathAttribute::MpReachNlri(MpReachNlriPathAttribute { afi: Afi::IPV6, safi: Safi::UNICAST, next_hop: NlriNextHop::Ipv6(Ipv6Addr::LOCALHOST), - prefixes: vec![prefix("2001:db8:1::/48"), prefix("2001:db8:2::/48")], + prefixes: vec![ + prefix("2001:db8:1::/48").into(), + prefix("2001:db8:2::/48").into(), + ], }), PathAttribute::MpUnreachNlri(MpUnreachNlriPathAttribute { afi: Afi::IPV6, safi: Safi::UNICAST, - prefixes: vec![prefix("2001:db8:3::/48")], + prefixes: vec![prefix("2001:db8:3::/48").into()], }), ], - announced_routes: vec![prefix("192.0.2.0/24")], + announced_routes: vec![prefix("192.0.2.0/24").into()], } } diff --git a/crates/server/src/fsm.rs b/crates/server/src/fsm.rs index 2870a09..20d12e4 100644 --- a/crates/server/src/fsm.rs +++ b/crates/server/src/fsm.rs @@ -16,7 +16,7 @@ use std::time::Duration; use bgp_packet::errors::{BgpError, DecodeError, OpenMsgSubcode}; use bgp_packet::message::{AS_TRANS, BGP4_VERSION, Message, UpdateMessage}; -use bgp_packet::open::{Capability, OpenMessage, OpenOption}; +use bgp_packet::open::{AddPathEntry, AddPathSendRecv, Capability, OpenMessage, OpenOption}; use bgp_rib::table::TooManyPeers; @@ -64,6 +64,9 @@ pub struct FsmConfig { pub expected_asn: Option, /// The address families to offer the peer. Must not be empty. pub families: FamilySet, + /// The directions to offer ADD-PATH (RFC 7911) for, on each of + /// `families`; `None` for neither. + pub add_path: Option, } /// What the two OPENs settled. @@ -78,6 +81,13 @@ pub struct Negotiated { /// The address families both sides offered: the ones routes may be /// exchanged for. Never empty. pub families: FamilySet, + /// The families for which the peer sends us several routes for a + /// prefix, each with a path identifier (ADD-PATH, RFC 7911): we offered + /// to receive them and the peer offered to send them. + pub add_path_receive: FamilySet, + /// The families for which we send the peer path identifiers: we + /// offered to send and the peer to receive. + pub add_path_send: FamilySet, } /// Something that happened to the connection. @@ -272,12 +282,34 @@ impl Fsm { return self.fail(SessionError::NoCommonAddressFamily { ours, theirs }); } + // ADD-PATH is in use in a direction, for a family the session + // carries, when one side offered to send and the other to receive + // (RFC 7911 §4). + let (mut add_path_receive, mut add_path_send) = + (FamilySet::default(), FamilySet::default()); + if let Some(ours) = self.config.add_path { + let theirs = open.capabilities().flat_map(|capability| match capability { + Capability::AddPath { entries } => entries.as_slice(), + _ => &[], + }); + for entry in theirs.filter(|entry| families.contains(entry.family())) { + if ours.receives() && entry.send_recv.sends() { + add_path_receive.insert(entry.family()); + } + if ours.sends() && entry.send_recv.receives() { + add_path_send.insert(entry.family()); + } + } + } + let negotiated = Negotiated { peer_asn, peer_identifier: open.identifier, hold_time_secs: self.config.hold_time_secs.min(open.hold_time), four_octet_asn: capability_asn.is_some(), families, + add_path_receive, + add_path_send, }; // With a hold time of zero the timer will not run once Established, // but a peer that never confirms must still be given up on. @@ -349,6 +381,19 @@ fn build_open(config: &FsmConfig) -> Message { // not accept routes of a family that was not offered (RFC 4760 §8). capabilities.extend(config.families.iter().map(Capability::multiprotocol)); capabilities.push(Capability::RouteRefresh); + if let Some(send_recv) = config.add_path { + capabilities.push(Capability::AddPath { + entries: config + .families + .iter() + .map(|family| AddPathEntry { + afi: family.afi, + safi: family.safi, + send_recv, + }) + .collect(), + }); + } Message::Open(OpenMessage { version: BGP4_VERSION, asn: asn_field, @@ -396,6 +441,7 @@ mod tests { hold_time_secs: 90, expected_asn: None, families: FamilySet::supported(), + add_path: None, } } @@ -488,6 +534,8 @@ mod tests { four_octet_asn: false, // The peer listed no families, so it speaks IPv4 unicast. families: only(AfiSafi::IPV4_UNICAST), + add_path_receive: FamilySet::default(), + add_path_send: FamilySet::default(), }; assert!(matches!( &actions[..], @@ -612,6 +660,85 @@ mod tests { )); } + /// ADD-PATH is in use for a family and a direction only where one side + /// offered to send and the other to receive. + #[test] + fn add_path_is_in_use_where_one_side_sends_and_the_other_receives() { + use AddPathSendRecv::{Both, Receive, Send}; + let negotiated = |ours: Option, + theirs: Vec<(AfiSafi, AddPathSendRecv)>| { + let (mut fsm, first) = Fsm::new(FsmConfig { + add_path: ours, + ..config() + }); + let mut capabilities: Vec = SUPPORTED + .into_iter() + .map(Capability::multiprotocol) + .collect(); + if !theirs.is_empty() { + capabilities.push(Capability::AddPath { + entries: theirs + .into_iter() + .map(|(family, send_recv)| AddPathEntry { + afi: family.afi, + safi: family.safi, + send_recv, + }) + .collect(), + }); + } + let open = OpenMessage { + options: vec![OpenOption::Capabilities(capabilities)], + ..peer_open(90) + }; + fsm.handle(Event::Message(Message::Open(open))); + let n = fsm.negotiated().unwrap().clone(); + (first, n.add_path_receive, n.add_path_send) + }; + let (v4, v6) = (AfiSafi::IPV4_UNICAST, AfiSafi::IPV6_UNICAST); + let none = FamilySet::default; + + // We offer it for every family we offer, in the directions asked. + let (first, receive, send) = negotiated(Some(Receive), vec![(v4, Both), (v6, Receive)]); + let Action::Send(open) = &first[0] else { + panic!("expected OPEN first") + }; + assert!( + capabilities(open).contains(&&Capability::AddPath { + entries: SUPPORTED + .into_iter() + .map(|family| AddPathEntry { + afi: family.afi, + safi: family.safi, + send_recv: Receive, + }) + .collect(), + }) + ); + // The peer sends for IPv4 only; we did not offer to send at all. + assert_eq!((receive, send), (only(v4), none())); + + let (_, receive, send) = negotiated(Some(Both), vec![(v4, Send), (v6, Receive)]); + assert_eq!((receive, send), (only(v4), only(v6))); + + // Not offered by us: not in use, whatever the peer offers, and not + // in our OPEN. + let (first, receive, send) = negotiated(None, vec![(v4, Both)]); + let Action::Send(open) = &first[0] else { + panic!("expected OPEN first") + }; + assert!( + !capabilities(open) + .iter() + .any(|c| matches!(c, Capability::AddPath { .. })) + ); + assert_eq!((receive, send), (none(), none())); + + // Not offered by the peer: not in use either. + let (_, receive, send) = negotiated(Some(Both), vec![]); + assert_eq!((receive, send), (none(), none())); + } + #[test] fn routes_of_a_family_that_was_not_negotiated_are_ignored() { // The peer lists no families, so only IPv4 unicast is in use. @@ -623,9 +750,9 @@ mod tests { afi: Afi::IPV6, safi: Safi::UNICAST, next_hop: NlriNextHop::Ipv6(std::net::Ipv6Addr::LOCALHOST), - prefixes: vec![IpPrefix::try_from("2001:db8::/32").unwrap()], + prefixes: vec![IpPrefix::try_from("2001:db8::/32").unwrap().into()], })], - announced_routes: vec![v4], + announced_routes: vec![v4.into()], }; let actions = fsm.handle(Event::Message(Message::Update(update))); diff --git a/crates/server/src/routes.rs b/crates/server/src/routes.rs index afdc4d1..16bac38 100644 --- a/crates/server/src/routes.rs +++ b/crates/server/src/routes.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use bgp_packet::message::{NlriNextHop, PathAttribute, UpdateMessage}; -use bgp_primitives::ip_prefix::IpPrefix; +use bgp_primitives::ip_prefix::Nlri; use bgp_primitives::parser::EncodeError; use bgp_rib::table::{AttrSet, Outcome, PeerIndex, RibTable}; @@ -13,7 +13,9 @@ use bgp_rib::table::{AttrSet, Outcome, PeerIndex, RibTable}; /// /// An UPDATE names prefixes in four places: withdrawn routes and NLRI in the /// message body, which are IPv4, and the MP_UNREACH_NLRI and MP_REACH_NLRI -/// attributes for every other family (RFC 4760). +/// attributes for every other family (RFC 4760). Where the peer sends +/// several routes for a prefix (ADD-PATH, RFC 7911), each is named by its +/// prefix and path identifier together, and is a route of its own. /// /// `attributes_wire` is the attributes as the decoder kept them, if it did /// (see `DecodedUpdate::attributes_wire`). With it the table finds the @@ -27,11 +29,11 @@ pub(crate) fn apply( let mut outcome = Outcome::default(); if !update.withdrawn_routes.is_empty() { - outcome += table.withdraw(peer, &update.withdrawn_routes); + outcome += table.withdraw_paths(peer, &update.withdrawn_routes); } for attribute in &update.path_attributes { if let PathAttribute::MpUnreachNlri(mp) = attribute { - outcome += table.withdraw(peer, &mp.prefixes); + outcome += table.withdraw_paths(peer, &mp.prefixes); } } @@ -43,9 +45,9 @@ pub(crate) fn apply( } table.attrs().intern(&update.path_attributes, next_hop) }; - let mut announce = |prefixes: &[IpPrefix], next_hop| -> Result<(), EncodeError> { - if !prefixes.is_empty() { - outcome += table.announce(peer, &attributes(next_hop)?, prefixes); + let mut announce = |nlri: &[Nlri], next_hop| -> Result<(), EncodeError> { + if !nlri.is_empty() { + outcome += table.announce_paths(peer, &attributes(next_hop)?, nlri); } Ok(()) }; diff --git a/crates/server/src/server.rs b/crates/server/src/server.rs index 0790ef0..96a6419 100644 --- a/crates/server/src/server.rs +++ b/crates/server/src/server.rs @@ -120,6 +120,7 @@ impl BgpServer { peer_name: None, expected_asn: None, families: FamilySet::supported(), + add_path: None, }; // The tasks the server cannot do without. None of them ends while @@ -157,7 +158,8 @@ impl BgpServer { match result { Ok((stream, peer_addr)) => { // Inbound connections from a configured peer inherit its - // name, expected ASN, hold time and address families. + // name, expected ASN, hold time, address families and + // ADD-PATH setting. let mut config = session_config.clone(); if let Some(peer) = self .config @@ -169,6 +171,7 @@ impl BgpServer { config.expected_asn = Some(peer.remote_asn); config.hold_time_secs = peer.hold_time_secs; config.families = peer.families.clone(); + config.add_path = peer.add_path; } tokio::spawn(handle_connection(stream, peer_addr, Arc::new(config))); } diff --git a/crates/server/src/session.rs b/crates/server/src/session.rs index c92d722..c4030e5 100644 --- a/crates/server/src/session.rs +++ b/crates/server/src/session.rs @@ -66,6 +66,11 @@ pub struct SessionHandle { /// The address families negotiated with the peer. Routes of any other /// family are neither sent to it nor accepted from it. pub families: FamilySet, + /// The families for which the peer sends us several routes for a + /// prefix (ADD-PATH, RFC 7911), and those for which we send it path + /// identifiers. + pub add_path_receive: FamilySet, + pub add_path_send: FamilySet, /// The peer's number in the routing table: what its routes there are /// marked with. pub rib_peer: PeerIndex, @@ -94,6 +99,8 @@ impl SessionHandle { established_at: SystemTime::now(), four_octet_asn: negotiated.four_octet_asn, families: negotiated.families.clone(), + add_path_receive: negotiated.add_path_receive.clone(), + add_path_send: negotiated.add_path_send.clone(), rib_peer, outbound, } diff --git a/crates/web/src/api.rs b/crates/web/src/api.rs index 844fefd..95d4a31 100644 --- a/crates/web/src/api.rs +++ b/crates/web/src/api.rs @@ -11,8 +11,7 @@ use axum::http::StatusCode; use axum::response::{IntoResponse, Response}; use bgp_packet::community; use bgp_packet::message::{NlriNextHop, OriginPathAttribute, PathAttribute}; -use bgp_primitives::ip_prefix::IpPrefix; -use bgp_rib::table::{AttrSet, PeerIndex}; +use bgp_rib::table::{PeerIndex, Route}; use bgp_rib::{LocalRoute, PeerId, RibStopped}; use bgp_server::session::{ConfiguredPeer, DialState, SessionHandle, SessionRegistry}; use serde::{Deserialize, Serialize}; @@ -61,6 +60,11 @@ pub struct PeerView { pub four_octet_asn: Option, /// The address families negotiated with the peer, e.g. `IPv4 unicast`. pub address_families: Option>, + /// The families for which the peer sends us several routes for a + /// prefix (ADD-PATH, RFC 7911), and those for which we send it path + /// identifiers. + pub add_path_receive: Option>, + pub add_path_send: Option>, /// Why the last connection attempt or session ended, while in Backoff. pub last_error: Option, pub retry_at_unix: Option, @@ -79,6 +83,8 @@ impl From<&SessionHandle> for PeerView { uptime_secs: Some(s.established_at.elapsed().map(|d| d.as_secs()).unwrap_or(0)), four_octet_asn: Some(s.four_octet_asn), address_families: Some(s.families.iter().map(|f| f.to_string()).collect()), + add_path_receive: Some(s.add_path_receive.iter().map(|f| f.to_string()).collect()), + add_path_send: Some(s.add_path_send.iter().map(|f| f.to_string()).collect()), last_error: None, retry_at_unix: None, } @@ -105,6 +111,8 @@ impl PeerView { uptime_secs: None, four_octet_asn: None, address_families: None, + add_path_receive: None, + add_path_send: None, last_error, retry_at_unix, } @@ -235,6 +243,9 @@ pub struct ReceivedRouteView { pub peer_addr: String, pub peer_asn: u32, pub prefix: String, + /// The peer's identifier for the route: zero unless the peer sends + /// several routes for a prefix (ADD-PATH, RFC 7911). + pub path_id: u32, /// Whether this is the route chosen for its prefix. pub best: bool, pub summary: AttrSummary, @@ -260,20 +271,15 @@ impl PeerNames { /// A view of one route. `None` if the route's session has just ended: /// the route is on its way out of the table. - pub(crate) fn view( - &self, - prefix: &IpPrefix, - peer: PeerIndex, - attrs: &AttrSet, - best: bool, - ) -> Option { - let peer = self.0.get(&peer)?; - let (attributes, next_hop) = attrs.attributes(); + pub(crate) fn view(&self, route: &Route) -> Option { + let peer = self.0.get(&route.peer)?; + let (attributes, next_hop) = route.attrs.attributes(); Some(ReceivedRouteView { peer_addr: peer.addr.to_string(), peer_asn: peer.asn, - prefix: prefix.to_string(), - best, + prefix: route.prefix.to_string(), + path_id: route.path_id, + best: route.best, summary: summarize(&attributes, next_hop.as_ref()), attributes, }) @@ -376,7 +382,7 @@ pub async fn received_routes( .rib .some_routes(query.limit.unwrap_or(DEFAULT_ROUTE_LIMIT)) .iter() - .filter_map(|(prefix, peer, attrs, best)| names.view(prefix, *peer, attrs, *best)) + .filter_map(|route| names.view(route)) .collect(); routes.sort_by(|a, b| (&a.prefix, &a.peer_addr).cmp(&(&b.prefix, &b.peer_addr))); Json(routes) diff --git a/crates/web/src/events.rs b/crates/web/src/events.rs index a81474b..050c5b3 100644 --- a/crates/web/src/events.rs +++ b/crates/web/src/events.rs @@ -8,7 +8,7 @@ use axum::extract::State; use axum::response::sse::{Event, KeepAlive, Sse}; use bgp_primitives::ip_prefix::IpPrefix; use bgp_rib::LocalRibEvent; -use bgp_rib::table::{self as journal, AttrSet, PeerIndex}; +use bgp_rib::table::{self as journal, Route}; use bgp_server::session::SessionEvent; use futures::stream::{Stream, StreamExt}; use serde::Serialize; @@ -124,9 +124,16 @@ fn best_routes(state: &AppState) -> impl Stream + use<> { journal::Event::Best { prefix, peer, + path_id, attrs, } if !syncing => { - changes.push(Change::Best(prefix, peer, Arc::clone(attrs))); + changes.push(Change::Best(Route { + prefix, + peer, + path_id, + attrs: Arc::clone(attrs), + best: true, + })); } journal::Event::Withdrawn { prefix } if !syncing => { changes.push(Change::Withdrawn(prefix)); @@ -136,12 +143,10 @@ fn best_routes(state: &AppState) -> impl Stream + use<> { let names = PeerNames::of(®istry); for change in changes { let event = match change { - Change::Best(prefix, peer, attrs) => { - match names.view(&prefix, peer, &attrs, true) { - Some(route) => WireEvent::BestChanged { route }, - None => continue, - } - } + Change::Best(route) => match names.view(&route) { + Some(route) => WireEvent::BestChanged { route }, + None => continue, + }, Change::Withdrawn(prefix) => WireEvent::BestWithdrawn { prefix: prefix.to_string(), }, @@ -168,7 +173,7 @@ fn best_routes(state: &AppState) -> impl Stream + use<> { const BEST_ROUTE_BUFFER: usize = 256; enum Change { - Best(IpPrefix, PeerIndex, Arc), + Best(Route), Withdrawn(IpPrefix), /// The client's reader fell too far behind the table. Lagged, diff --git a/crates/web/static/statusz.html b/crates/web/static/statusz.html index a1afdf4..4d1f3f8 100644 --- a/crates/web/static/statusz.html +++ b/crates/web/static/statusz.html @@ -134,6 +134,15 @@ View: variables, status json, < } const STATE_CLASS = { Established: 'ok', Connecting: 'warn', Backoff: 'bad', Idle: 'dim' }; + // Where several routes for a prefix are exchanged (ADD-PATH), and which way. + function addPath(p) { + const rx = p.add_path_receive || [], tx = p.add_path_send || []; + if (!rx.length && !tx.length) return ''; + const parts = []; + if (rx.length) parts.push(`receive ${esc(rx.join(', '))}`); + if (tx.length) parts.push(`send ${esc(tx.join(', '))}`); + return ` · add-path: ${parts.join('; ')}`; + } function renderPeers() { const b = $('peers-body'); $('peers-count').textContent = `${state.peers.filter(p => p.state === 'Established').length} of ${state.peers.length} established`; @@ -146,7 +155,7 @@ View: variables, status json, < ${esc(p.state)}${up ? (p.direction === 'outbound' ? 'us' : 'peer') : DASH} ${up ? esc(p.local_addr) : DASH}${up ? fmtTime(p.established_at_unix) : DASH} ${up ? age(p.established_at_unix) : DASH}${up ? (p.four_octet_asn ? 'yes' : 'no') : DASH} - ${up ? esc((p.address_families || []).join(', ')) : DASH}${detail}`; + ${up ? esc((p.address_families || []).join(', ')) + addPath(p) : DASH}${detail}`; }).join('')); } @@ -168,7 +177,7 @@ View: variables, status json, < : `${state.received.length}${st ? `, for ${st.received_prefixes} prefixes` : ''}`; if (!state.received.length) { setHTML(b, 'No routes received from peers.'); return; } setHTML(b, state.received.map(r => { const s = r.summary; return ` - ${esc(r.prefix)}${esc(r.peer_addr)} AS${r.peer_asn}${asPath(s.as_path)} + ${esc(r.prefix)}${esc(r.peer_addr)} AS${r.peer_asn}${r.path_id ? ` path ${r.path_id}` : ''}${asPath(s.as_path)} ${s.next_hop ? esc(s.next_hop) : DASH}${s.origin ? esc(s.origin) : DASH}${s.med ?? DASH}${s.local_pref ?? DASH} ${commList(s.communities, true)}${commList(s.large_communities)}${r.best ? 'best' : ''}`; }).join('')); } diff --git a/fuzz/fuzz_targets/message.rs b/fuzz/fuzz_targets/message.rs index e5df3a1..9bdef17 100644 --- a/fuzz/fuzz_targets/message.rs +++ b/fuzz/fuzz_targets/message.rs @@ -1,5 +1,5 @@ -//! Raw bytes, as they come off a TCP connection. The first byte picks the -//! session's AS number width and whether the peer is internal. +//! Raw bytes, as they come off a TCP connection. The first byte picks what the +//! session settled: see `bgp_fuzz::Session`. #![no_main] use libfuzzer_sys::fuzz_target; @@ -8,5 +8,5 @@ fuzz_target!(|data: &[u8]| { let Some((selector, wire)) = data.split_first() else { return; }; - bgp_fuzz::decode_and_check(selector & 1 == 1, selector & 0x80 != 0, wire); + bgp_fuzz::decode_and_check(bgp_fuzz::Session::from_bits(*selector), wire); }); diff --git a/fuzz/fuzz_targets/message_body.rs b/fuzz/fuzz_targets/message_body.rs index 73ab70b..47dae99 100644 --- a/fuzz/fuzz_targets/message_body.rs +++ b/fuzz/fuzz_targets/message_body.rs @@ -1,7 +1,7 @@ //! A message body behind a correct header, so that the fuzzer spends its //! time in the OPEN, UPDATE and NOTIFICATION parsers and not on guessing -//! the marker and length. The first byte picks the AS number width, the -//! message type and whether the peer is internal. +//! the marker and length. The first byte picks the message type and what +//! the session settled: see `bgp_fuzz::Session`. #![no_main] use libfuzzer_sys::fuzz_target; @@ -14,11 +14,12 @@ fuzz_target!(|data: &[u8]| { return; }; let body = &body[..body.len().min(MAX_BODY)]; - let msg_type = 1 + (selector >> 1) % 5; + // Bits 1 to 3; the others pick the session. + let msg_type = 1 + ((selector >> 1) & 0x07) % 5; let mut wire = vec![0xff; 16]; wire.extend_from_slice(&((19 + body.len()) as u16).to_be_bytes()); wire.push(msg_type); wire.extend_from_slice(body); - bgp_fuzz::decode_and_check(selector & 1 == 1, selector & 0x80 != 0, &wire); + bgp_fuzz::decode_and_check(bgp_fuzz::Session::from_bits(*selector), &wire); }); diff --git a/fuzz/src/lib.rs b/fuzz/src/lib.rs index ed22dd3..c6a6b50 100644 --- a/fuzz/src/lib.rs +++ b/fuzz/src/lib.rs @@ -1,21 +1,53 @@ //! What the fuzz targets have in common. use bgp_packet::message::Message; +use bgp_primitives::constants::AfiSafi; use bgp_primitives::parser::{EncodeError, ParserContext}; use bytes::BytesMut; -/// Decodes `wire` as a peer's message on a session with the given AS number -/// width and kind of peer. Whatever the bytes, this must return: a panic is a way for a peer +/// What a session settled that decoding depends on. +#[derive(Debug, Clone, Copy)] +pub struct Session { + pub four_octet_asn: bool, + pub ibgp: bool, + /// Whether prefixes carry path identifiers (RFC 7911), for IPv4 and for + /// IPv6 unicast. + pub add_path: [bool; 2], +} + +impl Session { + /// A session picked by bits of `selector`: bit 0 for the AS number + /// width, bits 5 and 6 for ADD-PATH, bit 7 for an internal peer. Bits 1 + /// to 4 are left for the target. + pub fn from_bits(selector: u8) -> Self { + Self { + four_octet_asn: selector & 0x01 != 0, + add_path: [selector & 0x20 != 0, selector & 0x40 != 0], + ibgp: selector & 0x80 != 0, + } + } + + fn context(self) -> ParserContext { + ParserContext { + four_octet_asn: Some(self.four_octet_asn), + ibgp: self.ibgp, + add_path: [AfiSafi::IPV4_UNICAST, AfiSafi::IPV6_UNICAST] + .into_iter() + .zip(self.add_path) + .filter_map(|(family, in_use)| in_use.then_some(family)) + .collect(), + } + } +} + +/// Decodes `wire` as a peer's message on `session`. Whatever the bytes, this must return: a panic is a way for a peer /// to take a session down. /// /// A message that is accepted must also be one we can show, write, and read /// back as itself. Anything else means the decoder built a value the rest /// of the program cannot rely on. -pub fn decode_and_check(four_octet_asn: bool, ibgp: bool, wire: &[u8]) { - let ctx = ParserContext { - four_octet_asn: Some(four_octet_asn), - ibgp, - }; +pub fn decode_and_check(session: Session, wire: &[u8]) { + let ctx = session.context(); let Ok(decoded) = Message::decode_with_report(&ctx, wire) else { return; }; diff --git a/testdata/server.textpb b/testdata/server.textpb index 63dfe35..de2f8d6 100644 --- a/testdata/server.textpb +++ b/testdata/server.textpb @@ -17,6 +17,9 @@ peers { # Offer only these families to the peer. Left out, all are offered. address_families: IPV4_UNICAST address_families: IPV6_UNICAST + # Take several routes for a prefix from this peer, if it offers to send + # them (ADD-PATH). Also ADD_PATH_SEND and ADD_PATH_BOTH. Off by default. + add_path: ADD_PATH_RECEIVE } connect_retry_seconds: 5