Skip to content

Commit

Permalink
Various refactoring
Browse files Browse the repository at this point in the history 
Removes an unused function.
Guards an illumos-specific import.
Consolidates (bfd) nexthop enabled functions into one that takes a bool.
Moves RIB locking and looping of prefixes and PrefixChangeNotifications
into RIB removal helper functions to improve pcn batching and
consolidate locations for future RIB batch work.
Removes re-processing of BGP path attributes.
Removes re-looping over routes/paths in a few places.
Eliminates some return types when no callers handled Result.
Adds some TODOs.

Signed-off-by: Trey Aspelund <[email protected]>
  • Loading branch information
@taspelund
taspelund committed Oct 20, 2024
1 parent e71e1bc commit 6523217
Show file tree
Hide file tree
Showing 9 changed files with 211 additions and 184 deletions.
4 changes: 2 additions & 2 deletions bfd/src/sm.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ impl State for Down {
db: rdb::Db,
counters: Arc<SessionCounters>,
) -> Result<(Box<dyn State>, BfdEndpoint)> {
db.disable_nexthop(self.peer);
db.set_nexthop_shutdown(self.peer, true);
loop {
// Get an incoming message
let (_addr, msg) = match self.recv(
Expand Down Expand Up @@ -597,7 +597,7 @@ impl State for Up {
db: rdb::Db,
counters: Arc<SessionCounters>,
) -> Result<(Box<dyn State>, BfdEndpoint)> {
db.enable_nexthop(self.peer);
db.set_nexthop_shutdown(self.peer, false);
loop {
// Get an incoming message
let (_addr, msg) = match self.recv(
Expand Down
4 changes: 3 additions & 1 deletion bgp/src/connection_tcp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::session::FsmEvent;
use crate::to_canonical;
use libc::{c_int, sockaddr_storage};
use mg_common::lock;
use slog::{debug, error, info, trace, warn, Logger};
use slog::{error, info, trace, warn, Logger};
use std::collections::BTreeMap;
use std::io::Read;
use std::io::Write;
Expand All @@ -31,6 +31,8 @@ use libc::{c_void, IPPROTO_IP, IPPROTO_IPV6, IPPROTO_TCP};
#[cfg(target_os = "linux")]
use libc::{IP_MINTTL, TCP_MD5SIG};
#[cfg(target_os = "illumos")]
use slog::debug;
#[cfg(target_os = "illumos")]
use std::collections::HashSet;
#[cfg(target_os = "illumos")]
use std::time::Instant;
Expand Down
107 changes: 69 additions & 38 deletions bgp/src/session.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1504,7 +1504,7 @@ impl<Cnx: BgpConnection + 'static> SessionRunner<Cnx> {

FsmEvent::RouteRefreshNeeded => {
if let Some(remote_id) = lock!(self.session).remote_id {
self.db.mark_bgp_id_stale(remote_id);
self.db.mark_bgp_peer_stale(remote_id);
self.send_route_refresh(&pc.conn);
}
FsmState::Established(pc)
Expand Down Expand Up @@ -1929,7 +1929,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_peer_prefixes(pc.id);
self.db.remove_bgp_peer_prefixes(pc.id);

FsmState::Idle
}
Expand Down Expand Up @@ -2024,9 +2024,14 @@ 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) {
for w in &update.withdrawn {
self.db.remove_peer_prefix(id, w.as_prefix4().into());
}
self.db.remove_bgp_prefixes(
update
.withdrawn
.iter()
.map(|w| rdb::Prefix::from(w.as_prefix4()))
.collect(),
id,
);

let originated = match self.db.get_origin4() {
Ok(value) => value,
Expand All @@ -2037,6 +2042,37 @@ impl<Cnx: BgpConnection + 'static> SessionRunner<Cnx> {
};

if !update.nlri.is_empty() {
// TODO: parse and prefer nexthop in MP_REACH_NLRI
//
// Per RFC 4760:
// """
// The next hop information carried in the MP_REACH_NLRI path attribute
// defines the Network Layer address of the router that SHOULD be used
// as the next hop to the destinations listed in the MP_NLRI attribute
// in the UPDATE message.
//
// [..]
//
// An UPDATE message that carries no NLRI, other than the one encoded in
// the MP_REACH_NLRI attribute, SHOULD NOT carry the NEXT_HOP attribute.
// If such a message contains the NEXT_HOP attribute, the BGP speaker
// that receives the message SHOULD ignore this attribute.
// """
//
// i.e.
// 1) NEXT_HOP SHOULD NOT be sent unless there are no MP_REACH_NLRI
// 2) NEXT_HOP SHOULD be ignored unless there are no MP_REACH_NLRI
//
// The standards do not state whether an implementation can/should send
// IPv4 Unicast prefixes embedded in an MP_REACH_NLRI attribute or in the
// classic NLRI field of an Update message. If we participate in MP-BGP
// and negotiate IPv4 Unicast, it's entirely likely that we'll peer with
// other BGP speakers falling into any of the combinations:
// a) MP not negotiated, IPv4 Unicast in NLRI, NEXT_HOP included
// b) MP negotiated, IPv4 Unicast in NLRI, NEXT_HOP included
// c) MP negotiated, IPv4 Unicast in NLRI, NEXT_HOP not included
// d) MP negotiated, IPv4 Unicast in MP_REACH_NLRI, NEXT_HOP included
// e) MP negotiated, IPv4 Unicast in MP_REACH_NLRI, NEXT_HOP not included
let nexthop = match update.nexthop4() {
Some(nh) => nh,
None => {
Expand All @@ -2051,41 +2087,36 @@ impl<Cnx: BgpConnection + 'static> SessionRunner<Cnx> {
}
};

for n in &update.nlri {
let prefix = n.as_prefix4();
// ignore prefixes we originate
if originated.contains(&prefix) {
continue;
}

let mut as_path = Vec::new();
if let Some(segments_list) = update.as_path() {
for segments in &segments_list {
as_path.extend(segments.value.iter());
}
}

let path = rdb::Path {
nexthop: nexthop.into(),
shutdown: update.graceful_shutdown(),
rib_priority: DEFAULT_RIB_PRIORITY_BGP,
bgp: Some(BgpPathProperties {
origin_as: peer_as,
id,
med: update.multi_exit_discriminator(),
local_pref: update.local_pref(),
as_path,
stale: None,
}),
vlan_id: lock!(self.session).vlan_id,
};

if let Err(e) =
self.db.add_prefix_path(prefix.into(), path.clone(), false)
{
err!(self; "failed to add path {:?} -> {:?}: {e}", prefix, path);
let mut as_path = Vec::new();
if let Some(segments_list) = update.as_path() {
for segments in &segments_list {
as_path.extend(segments.value.iter());
}
}
let path = rdb::Path {
nexthop: nexthop.into(),
shutdown: update.graceful_shutdown(),
rib_priority: DEFAULT_RIB_PRIORITY_BGP,
bgp: Some(BgpPathProperties {
origin_as: peer_as,
id,
med: update.multi_exit_discriminator(),
local_pref: update.local_pref(),
as_path,
stale: None,
}),
vlan_id: lock!(self.session).vlan_id,
};

self.db.add_bgp_prefixes(
update
.nlri
.iter()
.filter(|p| !originated.contains(&p.as_prefix4()))
.map(|n| rdb::Prefix::from(n.as_prefix4()))
.collect(),
path.clone(),
);
}

//TODO(IPv6) iterate through MpReachNlri attributes for IPv6
Expand Down
6 changes: 3 additions & 3 deletions mg-lower/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use slog::{error, info, Logger};
use std::collections::HashSet;
use std::net::Ipv6Addr;
use std::sync::mpsc::{channel, RecvTimeoutError};
use std::sync::{Arc, Mutex};
use std::sync::Arc;
use std::thread::sleep;
use std::time::Duration;

Expand Down Expand Up @@ -160,7 +160,7 @@ fn handle_change(

fn sync_prefix(
tep: Ipv6Addr,
rib_loc: Arc<Mutex<Rib>>,
rib_loc: Rib,
prefix: &Prefix,
dpd: &DpdClient,
ddm: &DdmClient,
Expand All @@ -172,7 +172,7 @@ fn sync_prefix(

// The best routes in the RIB
let mut best: HashSet<RouteHash> = HashSet::new();
if let Some(paths) = rib_loc.lock().unwrap().get(prefix) {
if let Some(paths) = rib_loc.get(prefix) {
for path in paths {
best.insert(RouteHash::for_prefix_path(*prefix, path.clone())?);
}
Expand Down
5 changes: 2 additions & 3 deletions mgd/src/bgp_admin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -418,8 +418,7 @@ pub async fn get_selected(
) -> Result<HttpResponseOk<Rib>, HttpError> {
let rq = request.into_inner();
let ctx = ctx.context();
let rib = get_router!(ctx, rq.asn)?.db.loc_rib();
let selected = rib.lock().unwrap().clone();
let selected = get_router!(ctx, rq.asn)?.db.loc_rib();
Ok(HttpResponseOk(selected.into()))
}

Expand Down Expand Up @@ -797,7 +796,7 @@ pub(crate) mod helpers {
)
.remote_id
.ok_or(Error::NotFound("bgp peer not found".into()))?;
ctx.db.remove_peer_prefixes(id);
ctx.db.remove_bgp_peer_prefixes(id);
ctx.db.remove_bgp_neighbor(addr)?;
get_router!(&ctx, asn)?.delete_session(addr);

Expand Down
13 changes: 4 additions & 9 deletions mgd/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use clap::{Parser, Subcommand};
use mg_common::cli::oxide_cli_style;
use mg_common::stats::MgLowerStats;
use rand::Fill;
use rdb::{BfdPeerConfig, BgpNeighborInfo, BgpRouterInfo, Path};
use rdb::{BfdPeerConfig, BgpNeighborInfo, BgpRouterInfo};
use signal::handle_signals;
use slog::{error, Logger};
use std::collections::BTreeMap;
Expand Down Expand Up @@ -271,14 +271,9 @@ fn initialize_static_routes(db: &rdb::Db) {
let routes = db
.get_static4()
.expect("failed to get static routes from db");
for route in &routes {
let path =
Path::for_static(route.nexthop, route.vlan_id, route.rib_priority);
db.add_prefix_path(route.prefix, path, true)
.unwrap_or_else(|e| {
panic!("failed to initialize static route {route:#?}: {e}")
})
}
db.add_static_routes(&routes).unwrap_or_else(|e| {
panic!("failed to initialize static routes {routes:#?}: {e}")
})
}

fn get_tunnel_endpoint_ula(db: &rdb::Db) -> Ipv6Addr {
Expand Down
22 changes: 8 additions & 14 deletions mgd/src/static_admin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,10 @@ pub async fn static_add_v4_route(
.into_iter()
.map(Into::into)
.collect();
for r in routes {
let path = Path::for_static(r.nexthop, r.vlan_id, r.rib_priority);
ctx.context()
.db
.add_prefix_path(r.prefix, path, true)
.map_err(|e| HttpError::for_internal_error(e.to_string()))?;
}
ctx.context()
.db
.add_static_routes(&routes)
.map_err(|e| HttpError::for_internal_error(e.to_string()))?;
Ok(HttpResponseUpdatedNoContent())
}

Expand All @@ -90,13 +87,10 @@ pub async fn static_remove_v4_route(
.into_iter()
.map(Into::into)
.collect();
for r in routes {
let path = Path::for_static(r.nexthop, r.vlan_id, r.rib_priority);
ctx.context()
.db
.remove_prefix_path(r.prefix, path, true)
.map_err(|e| HttpError::for_internal_error(e.to_string()))?;
}
ctx.context()
.db
.remove_static_routes(routes)
.map_err(|e| HttpError::for_internal_error(e.to_string()))?;
Ok(HttpResponseDeleted())
}

Expand Down
Loading

0 comments on commit 6523217

Please sign in to comment.