Skip to content

Commit

Permalink
Switch to IpAddr for identifying BGP session
Browse files Browse the repository at this point in the history 
Switch from SocketAddrPair to IpAddr for distinguishing between BGP
sessions. A BGP peer is identified by the IP we are connecting to,
and multiple TCP sessions via the same IP isn't something we're
looking to support. So instead of trying to handle all the cases
where the local SocketAddr may or may not be available, simplify
things by using the configured peer IP which will always be
present regardless of the peer's FSM State.

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
  • Loading branch information
@taspelund
taspelund committed Nov 1, 2024
1 parent 75b03e1 commit 5a862da
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 108 deletions.
18 changes: 5 additions & 13 deletions bgp/src/session.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,7 @@ 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,
SocketAddrPair,
};
use rdb::{Asn, BgpPathProperties, Db, ImportExportPolicy, Prefix, Prefix4};
pub use rdb::{DEFAULT_RIB_PRIORITY_BGP, DEFAULT_ROUTE_PRIORITY};
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -1507,8 +1504,7 @@ impl<Cnx: BgpConnection + 'static> SessionRunner<Cnx> {
}

FsmEvent::RouteRefreshNeeded => {
let conn = SocketAddrPair::new(pc.conn.local(), pc.conn.peer());
self.db.mark_bgp_peer_stale(conn);
self.db.mark_bgp_peer_stale(pc.conn.peer().ip());
self.send_route_refresh(&pc.conn);
FsmState::Established(pc)
}
Expand Down Expand Up @@ -1931,10 +1927,7 @@ 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(SocketAddrPair::new(
pc.conn.local(),
pc.conn.peer(),
));
self.db.remove_bgp_peer_prefixes(&pc.conn.peer().ip());

FsmState::Idle
}
Expand Down Expand Up @@ -2039,14 +2032,13 @@ impl<Cnx: BgpConnection + 'static> SessionRunner<Cnx> {
pc: &PeerConnection<Cnx>,
peer_as: u32,
) {
let conn = SocketAddrPair::new(pc.conn.local(), pc.conn.peer());
self.db.remove_bgp_prefixes(
update
.withdrawn
.iter()
.map(|w| rdb::Prefix::from(w.as_prefix4()))
.collect(),
conn.clone(),
&pc.conn.peer().ip(),
);

let originated = match self.db.get_origin4() {
Expand Down Expand Up @@ -2115,7 +2107,7 @@ impl<Cnx: BgpConnection + 'static> SessionRunner<Cnx> {
rib_priority: DEFAULT_RIB_PRIORITY_BGP,
bgp: Some(BgpPathProperties {
origin_as: peer_as,
conn: conn.clone(),
peer: pc.conn.peer().ip(),
id: pc.id,
med: update.multi_exit_discriminator(),
local_pref: update.local_pref(),
Expand Down
9 changes: 2 additions & 7 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, SocketAddrPair};
use rdb::{Asn, BgpRouterInfo, ImportExportPolicy, Prefix};
use slog::info;
use std::collections::{BTreeMap, HashMap, HashSet};
use std::hash::{Hash, Hasher};
Expand Down Expand Up @@ -779,12 +779,7 @@ pub(crate) mod helpers {
) -> Result<HttpResponseDeleted, Error> {
info!(ctx.log, "remove neighbor: {}", addr);

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

Expand Down
26 changes: 6 additions & 20 deletions openapi/mg-admin.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1261,9 +1261,6 @@
"minimum": 0
}
},
"conn": {
"$ref": "#/components/schemas/SocketAddrPair"
},
"id": {
"type": "integer",
"format": "uint32",
Expand All @@ -1286,6 +1283,10 @@
"format": "uint32",
"minimum": 0
},
"peer": {
"type": "string",
"format": "ip"
},
"stale": {
"nullable": true,
"type": "string",
Expand All @@ -1294,9 +1295,9 @@
},
"required": [
"as_path",
"conn",
"id",
"origin_as"
"origin_as",
"peer"
]
},
"BgpPeerConfig": {
Expand Down Expand Up @@ -3147,21 +3148,6 @@
"code"
]
},
"SocketAddrPair": {
"type": "object",
"properties": {
"local": {
"nullable": true,
"type": "string"
},
"peer": {
"type": "string"
}
},
"required": [
"peer"
]
},
"StaticRoute4": {
"type": "object",
"properties": {
Expand Down
30 changes: 14 additions & 16 deletions rdb/src/bestpath.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,25 +109,23 @@ pub fn bestpaths(
#[cfg(test)]
mod test {
use std::collections::BTreeSet;
use std::net::{IpAddr, SocketAddr};
use std::net::IpAddr;
use std::str::FromStr;

use super::bestpaths;
use crate::{
db::Rib, types::SocketAddrPair, BgpPathProperties, Path, Prefix,
Prefix4, DEFAULT_RIB_PRIORITY_BGP, DEFAULT_RIB_PRIORITY_STATIC,
db::Rib, 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::new(Some(local_sa), remote_sa);
let remote_ip1 = IpAddr::from_str("203.0.113.1").unwrap();
let remote_ip2 = IpAddr::from_str("203.0.113.2").unwrap();
let remote_ip3 = IpAddr::from_str("203.0.113.3").unwrap();
let remote_ip4 = IpAddr::from_str("203.0.113.4").unwrap();

// The best path for an empty RIB should be empty
const MAX_ECMP_FANOUT: usize = 2;
Expand All @@ -136,12 +134,12 @@ mod test {

// Add one path and make sure we get it back
let path1 = Path {
nexthop: "203.0.113.1".parse().unwrap(),
nexthop: remote_ip1,
rib_priority: DEFAULT_RIB_PRIORITY_BGP,
shutdown: false,
bgp: Some(BgpPathProperties {
origin_as: 470,
conn: conn.clone(),
peer: remote_ip1,
id: 47,
med: Some(75),
local_pref: Some(100),
Expand All @@ -158,12 +156,12 @@ mod test {

// Add another path to the same prefix and make sure bestpath returns both
let mut path2 = Path {
nexthop: "203.0.113.2".parse().unwrap(),
nexthop: remote_ip2,
rib_priority: DEFAULT_RIB_PRIORITY_BGP,
shutdown: false,
bgp: Some(BgpPathProperties {
origin_as: 480,
conn: conn.clone(),
peer: remote_ip2,
id: 48,
med: Some(75),
local_pref: Some(100),
Expand All @@ -183,12 +181,12 @@ mod test {
// - results are limited to 2 paths when max is 2
// - we get all three paths back wihen max is 3
let mut path3 = Path {
nexthop: "203.0.113.3".parse().unwrap(),
nexthop: remote_ip3,
rib_priority: DEFAULT_RIB_PRIORITY_BGP,
shutdown: false,
bgp: Some(BgpPathProperties {
origin_as: 490,
conn: conn.clone(),
peer: remote_ip3,
id: 49,
med: Some(100),
local_pref: Some(100),
Expand Down Expand Up @@ -236,7 +234,7 @@ mod test {
// - path 4 wins over BGP paths with lower RIB priority
// - path 4 wins over BGP paths with equal RIB priority
let mut path4 = Path {
nexthop: "203.0.113.4".parse().unwrap(),
nexthop: remote_ip4,
rib_priority: u8::MAX,
shutdown: false,
bgp: None,
Expand Down
Loading

0 comments on commit 5a862da

Please sign in to comment.