Skip to content

Commit

Permalink
Use socket addr pair to distinguish between BGP sessions
Browse files Browse the repository at this point in the history 
BGP-ID is insufficient to distinguish between BGP sessions, as it's
perfectly valid/reasonable to have multiple sessions to the same peer.
Or an operator can (incorrectly or otherwise) assign the same BGP-ID to
multiple routers.
This ensures that Session-A to Peer-A going down does not impact
Session-B to Peer-A.

Fixes: #241

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
  • Loading branch information
@taspelund
taspelund committed Oct 25, 2024
1 parent 985a9fb commit 8e5d38f
Show file tree
Hide file tree
Showing 7 changed files with 127 additions and 57 deletions.
2 changes: 1 addition & 1 deletion bgp/src/connection.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ pub trait BgpConnection: Send + Clone {
/// Return the socket address of the peer for this connection.
fn peer(&self) -> SocketAddr;

// Return the local address being used for the connection.
/// Return the local socket address for this connection.
fn local(&self) -> Option<SocketAddr>;

fn set_min_ttl(&self, ttl: u8) -> Result<(), Error>;
Expand Down
51 changes: 37 additions & 14 deletions bgp/src/session.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ use crate::router::Router;
use crate::to_canonical;
use mg_common::{dbg, err, inf, trc, wrn};
use mg_common::{lock, read_lock, write_lock};
use rdb::{Asn, BgpPathProperties, Db, ImportExportPolicy, Prefix, Prefix4};
use rdb::{
Asn, BgpPathProperties, Db, ImportExportPolicy, Prefix, Prefix4,
SocketAddrPair,
};
pub use rdb::{DEFAULT_RIB_PRIORITY_BGP, DEFAULT_ROUTE_PRIORITY};
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -342,7 +345,7 @@ pub struct SessionInfo {
/// The ASN of the remote peer.
pub remote_asn: Option<u32>,

/// The ASN of the remote peer.
/// The Router-ID of the remote peer.
pub remote_id: Option<u32>,

/// Minimum acceptable TTL value for incomming BGP packets.
Expand Down Expand Up @@ -500,12 +503,12 @@ pub struct SessionRunner<Cnx: BgpConnection> {
pub clock: Clock,

pub session: Arc<Mutex<SessionInfo>>,
pub bind_addr: Option<SocketAddr>,
event_rx: Receiver<FsmEvent<Cnx>>,
state: Arc<Mutex<FsmStateKind>>,
last_state_change: Mutex<Instant>,
asn: Asn,
id: u32,
bind_addr: Option<SocketAddr>,
shutdown: AtomicBool,
running: AtomicBool,
db: Db,
Expand Down Expand Up @@ -1338,7 +1341,7 @@ impl<Cnx: BgpConnection + 'static> SessionRunner<Cnx> {
lock!(self.clock.timers.hold_timer).reset();
inf!(self; "update received: {m:#?}");
let peer_as = lock!(self.session).remote_asn.unwrap_or(0);
self.apply_update(m.clone(), pc.id, peer_as);
self.apply_update(m.clone(), &pc, peer_as);
lock!(self.message_history).receive(m.into());
self.counters
.updates_received
Expand Down Expand Up @@ -1504,10 +1507,12 @@ impl<Cnx: BgpConnection + 'static> SessionRunner<Cnx> {
}

FsmEvent::RouteRefreshNeeded => {
if let Some(remote_id) = lock!(self.session).remote_id {
self.db.mark_bgp_peer_stale(remote_id);
self.send_route_refresh(&pc.conn);
}
let conn = SocketAddrPair {
local: pc.conn.local(),
peer: pc.conn.peer(),
};
self.db.mark_bgp_peer_stale(conn);
self.send_route_refresh(&pc.conn);
FsmState::Established(pc)
}

Expand Down Expand Up @@ -1929,13 +1934,21 @@ impl<Cnx: BgpConnection + 'static> SessionRunner<Cnx> {
write_lock!(self.fanout).remove_egress(self.neighbor.host.ip());

// remove peer prefixes from db
self.db.remove_bgp_peer_prefixes(pc.id);
self.db.remove_bgp_peer_prefixes(SocketAddrPair {
local: pc.conn.local(),
peer: pc.conn.peer(),
});

FsmState::Idle
}

/// Apply an update by adding it to our RIB.
fn apply_update(&self, mut update: UpdateMessage, id: u32, peer_as: u32) {
fn apply_update(
&self,
mut update: UpdateMessage,
pc: &PeerConnection<Cnx>,
peer_as: u32,
) {
if let Err(e) = self.check_update(&update) {
wrn!(
self;
Expand Down Expand Up @@ -1983,7 +1996,7 @@ impl<Cnx: BgpConnection + 'static> SessionRunner<Cnx> {
update.nlri.retain(|x| message_policy.contains(x));
};

self.update_rib(&update, id, peer_as);
self.update_rib(&update, pc, peer_as);

// NOTE: for now we are only acting as an edge router. This means we
// do not redistribute announcements. If this changes, uncomment
Expand Down Expand Up @@ -2023,14 +2036,23 @@ impl<Cnx: BgpConnection + 'static> SessionRunner<Cnx> {
}

/// Update this router's RIB based on an update message from a peer.
fn update_rib(&self, update: &UpdateMessage, id: u32, peer_as: u32) {
fn update_rib(
&self,
update: &UpdateMessage,
pc: &PeerConnection<Cnx>,
peer_as: u32,
) {
let conn = SocketAddrPair {
local: pc.conn.local(),
peer: pc.conn.peer(),
};
self.db.remove_bgp_prefixes(
update
.withdrawn
.iter()
.map(|w| rdb::Prefix::from(w.as_prefix4()))
.collect(),
id,
conn.clone(),
);

let originated = match self.db.get_origin4() {
Expand Down Expand Up @@ -2099,7 +2121,8 @@ impl<Cnx: BgpConnection + 'static> SessionRunner<Cnx> {
rib_priority: DEFAULT_RIB_PRIORITY_BGP,
bgp: Some(BgpPathProperties {
origin_as: peer_as,
id,
conn: conn.clone(),
id: pc.id,
med: update.multi_exit_discriminator(),
local_pref: update.local_pref(),
as_path,
Expand Down
21 changes: 9 additions & 12 deletions mgd/src/bgp_admin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use dropshot::{
};
use http::status::StatusCode;
use mg_common::lock;
use rdb::{Asn, BgpRouterInfo, ImportExportPolicy, Prefix};
use rdb::{Asn, BgpRouterInfo, ImportExportPolicy, Prefix, SocketAddrPair};
use slog::info;
use std::collections::{BTreeMap, HashMap, HashSet};
use std::hash::{Hash, Hasher};
Expand Down Expand Up @@ -779,17 +779,14 @@ pub(crate) mod helpers {
) -> Result<HttpResponseDeleted, Error> {
info!(ctx.log, "remove neighbor: {}", addr);

let id = lock!(
get_router!(ctx, asn)?
.get_session(addr)
.ok_or(Error::NotFound(
"session for bgp peer not found".into()
))?
.session
)
.remote_id
.ok_or(Error::NotFound("bgp peer not found".into()))?;
ctx.db.remove_bgp_peer_prefixes(id);
let session = get_router!(ctx, asn)?
.get_session(addr)
.ok_or(Error::NotFound("session for bgp peer not found".into()))?;
let conn = SocketAddrPair {
local: session.bind_addr,
peer: session.neighbor.host,
};
ctx.db.remove_bgp_peer_prefixes(conn);
ctx.db.remove_bgp_neighbor(addr)?;
get_router!(&ctx, asn)?.delete_session(addr);

Expand Down
19 changes: 19 additions & 0 deletions openapi/mg-admin.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1261,6 +1261,9 @@
"minimum": 0
}
},
"conn": {
"$ref": "#/components/schemas/SocketAddrPair"
},
"id": {
"type": "integer",
"format": "uint32",
Expand Down Expand Up @@ -1291,6 +1294,7 @@
},
"required": [
"as_path",
"conn",
"id",
"origin_as"
]
Expand Down Expand Up @@ -3143,6 +3147,21 @@
"code"
]
},
"SocketAddrPair": {
"type": "object",
"properties": {
"local": {
"nullable": true,
"type": "string"
},
"peer": {
"type": "string"
}
},
"required": [
"peer"
]
},
"StaticRoute4": {
"type": "object",
"properties": {
Expand Down
18 changes: 16 additions & 2 deletions rdb/src/bestpath.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,17 +109,28 @@ pub fn bestpaths(
#[cfg(test)]
mod test {
use std::collections::BTreeSet;
use std::net::{IpAddr, SocketAddr};
use std::str::FromStr;

use super::bestpaths;
use crate::{
db::Rib, BgpPathProperties, Path, Prefix, Prefix4,
DEFAULT_RIB_PRIORITY_BGP, DEFAULT_RIB_PRIORITY_STATIC,
db::Rib, types::SocketAddrPair, BgpPathProperties, Path, Prefix,
Prefix4, DEFAULT_RIB_PRIORITY_BGP, DEFAULT_RIB_PRIORITY_STATIC,
};

#[test]
fn test_bestpath() {
let mut rib = Rib::default();
let target: Prefix4 = "198.51.100.0/24".parse().unwrap();
let bgp_port = 179u16;
let local_sa =
SocketAddr::new(IpAddr::from_str("203.0.113.2").unwrap(), 4444);
let remote_sa =
SocketAddr::new(IpAddr::from_str("203.0.113.0").unwrap(), bgp_port);
let conn = SocketAddrPair {
local: Some(local_sa),
peer: remote_sa,
};

// The best path for an empty RIB should be empty
const MAX_ECMP_FANOUT: usize = 2;
Expand All @@ -133,6 +144,7 @@ mod test {
shutdown: false,
bgp: Some(BgpPathProperties {
origin_as: 470,
conn: conn.clone(),
id: 47,
med: Some(75),
local_pref: Some(100),
Expand All @@ -154,6 +166,7 @@ mod test {
shutdown: false,
bgp: Some(BgpPathProperties {
origin_as: 480,
conn: conn.clone(),
id: 48,
med: Some(75),
local_pref: Some(100),
Expand All @@ -178,6 +191,7 @@ mod test {
shutdown: false,
bgp: Some(BgpPathProperties {
origin_as: 490,
conn: conn.clone(),
id: 49,
med: Some(100),
local_pref: Some(100),
Expand Down
Loading

0 comments on commit 8e5d38f

Please sign in to comment.