Embiggen the goodness of maghemite (#385)

* Cleanup key in rib_in when all paths are removed

Fixes: #369

Signed-off-by: Trey Aspelund <[email protected]>

* Use proper path comparison during route path deletion

Signed-off-by: Trey Aspelund <[email protected]>

* bgp: Don't apply ImportExportPolicy to withdrawn nlri

Import/Export filters are meant to modify which advertised prefixes
are allowed. For Import, this is simply an allow-list that accepts a
subset of the advertised nlri in a received update. For Export, this
is an allow-list that accepts a subset of the locally originated nlri.
In neither case do you want to apply these filters to the list of
withdrawn nlri, as this can result in stale routes if a legitimate
withdrawal is not sent or received.

Fixes: #330

Signed-off-by: Trey Aspelund <[email protected]>

* Various refactoring

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]>

* Add tests for RIB insertion/removal

Signed-off-by: Trey Aspelund <[email protected]>

* make clippy happy

* Remove duplicate lock! macro

mgd/src/bgp_admin.rs was re-defining lock! identically to the one in
mg-common.  Remove this dupe definition and just import from mg-common.

Signed-off-by: Trey Aspelund <[email protected]>

* Move completely over to use of lock! macro

Replace all old instances of .lock().unwrap() with lock!()

Signed-off-by: Trey Aspelund <[email protected]>

* Move log module from bgp into mg_common

Signed-off-by: Trey Aspelund <[email protected]>

* Move bgp test to mg_common::log

Signed-off-by: Trey Aspelund <[email protected]>

* Make clippy happy

Actually run the same cargo clippy command as CI, so I can see
errors locally :/

Signed-off-by: Trey Aspelund <[email protected]>

* add breadcrumb for bgp-id issue

* Make remove_prefix_path closure more intuitive

Signed-off-by: Trey Aspelund <[email protected]>

* remove pcn todo

* Use sled transaction for static route persistent db

Move over to using transaction semantics for static route db updates.
Also change remove_static_routes to take a reference, aligning with
add_static_routes.

Signed-off-by: Trey Aspelund <[email protected]>

---------

Signed-off-by: Trey Aspelund <[email protected]>

bfd/Cargo.toml

@@ -5,6 +5,7 @@ edition = "2021"
 
 [dependencies]
 rdb = { path = "../rdb" }
+mg-common.workspace = true
 slog.workspace = true
 slog-bunyan.workspace = true
 slog-async.workspace = true

bfd/src/sm.rs

@@ -8,6 +8,7 @@ use crate::{
 };
 use crate::{err, SessionCounters};
 use anyhow::{anyhow, Result};
+use mg_common::lock;
 use slog::{warn, Logger};
 use std::net::IpAddr;
 use std::sync::atomic::{AtomicBool, Ordering};
@@ -197,7 +198,7 @@ impl StateMachine {
             // Get what we need from peer info, holding the lock a briefly as
             // possible.
             let (_delay, demand_mode, your_discriminator) = {
-                let r = remote.lock().unwrap();
+                let r = lock!(remote);
                 (
                     DeferredDelay(r.required_min_rx),
                     r.demand_mode,
@@ -305,7 +306,7 @@ pub(crate) trait State: Sync + Send {
         log: Logger,
     ) {
         let state = self.state();
-        let your_discriminator = remote.lock().unwrap().discriminator;
+        let your_discriminator = lock!(remote).discriminator;
 
         let mut pkt = packet::Control {
             desired_min_tx: local.desired_min_tx.as_micros() as u32,
@@ -438,7 +439,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(
@@ -597,7 +598,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(

bfd/src/util.rs

@@ -3,6 +3,7 @@
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
 use crate::{packet, PeerInfo};
+use mg_common::lock;
 use std::sync::{Arc, Mutex};
 use std::time::Duration;
 
@@ -117,7 +118,7 @@ macro_rules! err {
 }
 
 pub fn update_peer_info(remote: &Arc<Mutex<PeerInfo>>, msg: &packet::Control) {
-    let mut r = remote.lock().unwrap();
+    let mut r = lock!(remote);
     r.desired_min_tx = Duration::from_micros(msg.desired_min_tx.into());
     r.required_min_rx = Duration::from_micros(msg.required_min_rx.into());
     r.discriminator = msg.my_discriminator;

bgp/Cargo.toml

@@ -8,8 +8,6 @@ rdb = { path = "../rdb" }
 nom.workspace = true
 num_enum.workspace = true
 slog.workspace = true
-slog-bunyan.workspace = true
-slog-async.workspace = true
 thiserror.workspace = true
 serde.workspace = true
 schemars.workspace = true

bgp/src/clock.rs

@@ -108,35 +108,35 @@ impl Clock {
     ) {
         Self::step(
             resolution,
-            &timers.connect_retry_timer.lock().unwrap(),
+            &lock!(timers.connect_retry_timer),
             FsmEvent::ConnectRetryTimerExpires,
             s.clone(),
             &log,
         );
         Self::step(
             resolution,
-            &timers.keepalive_timer.lock().unwrap(),
+            &lock!(timers.keepalive_timer),
             FsmEvent::KeepaliveTimerExpires,
             s.clone(),
             &log,
         );
         Self::step(
             resolution,
-            &timers.hold_timer.lock().unwrap(),
+            &lock!(timers.hold_timer),
             FsmEvent::HoldTimerExpires,
             s.clone(),
             &log,
         );
         Self::step(
             resolution,
-            &timers.idle_hold_timer.lock().unwrap(),
+            &lock!(timers.idle_hold_timer),
             FsmEvent::IdleHoldTimerExpires,
             s.clone(),
             &log,
         );
         Self::step(
             resolution,
-            &timers.delay_open_timer.lock().unwrap(),
+            &lock!(timers.delay_open_timer),
             FsmEvent::DelayOpenTimerExpires,
             s.clone(),
             &log,

bgp/src/connection_tcp.rs

@@ -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;
@@ -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;
@@ -269,7 +271,7 @@ impl BgpConnection for BgpConnectionTcp {
 
     #[allow(unused_variables)]
     fn set_min_ttl(&self, ttl: u8) -> Result<(), Error> {
-        let conn = self.conn.lock().unwrap();
+        let conn = lock!(self.conn);
         match conn.as_ref() {
             None => Err(Error::NotConnected),
             Some(conn) => {
@@ -313,7 +315,7 @@ impl BgpConnection for BgpConnectionTcp {
         key: [u8; MAX_MD5SIG_KEYLEN],
     ) -> Result<(), Error> {
         info!(self.log, "setting md5 auth for {}", self.peer);
-        let conn = self.conn.lock().unwrap();
+        let conn = lock!(self.conn);
         let fd = match conn.as_ref() {
             None => return Err(Error::NotConnected),
             Some(c) => c.as_raw_fd(),
@@ -329,7 +331,7 @@ impl BgpConnection for BgpConnectionTcp {
         key: [u8; MAX_MD5SIG_KEYLEN],
     ) -> Result<(), Error> {
         info!(self.log, "setting md5 auth for {}", self.peer);
-        let conn = self.conn.lock().unwrap();
+        let conn = lock!(self.conn);
         match conn.as_ref() {
             None => return Err(Error::NotConnected),
             Some(c) => {
@@ -594,7 +596,7 @@ impl BgpConnectionTcp {
 
     #[cfg(target_os = "illumos")]
     fn md5_sig_drop(&self) {
-        let guard = self.sas.lock().unwrap();
+        let guard = lock!(self.sas);
         if let Some(ref sas) = *guard {
             for (local, peer) in sas.associations.iter() {
                 for (a, b) in sa_set(*local, *peer) {
@@ -642,7 +644,7 @@ impl BgpConnectionTcp {
         locals: Vec<SocketAddr>,
         peer: SocketAddr,
     ) -> Result<(), Error> {
-        let mut guard = self.sas.lock().unwrap();
+        let mut guard = lock!(self.sas);
         match &mut *guard {
             Some(sas) => {
                 for local in locals.into_iter() {
@@ -705,7 +707,7 @@ impl BgpConnectionTcp {
         // we may accept a connection from a client (as opposed to the client)
         // accepting a connection from us, and that will result in the
         // association set increasing according to the source port of the client.
-        let guard = sas.lock().unwrap();
+        let guard = lock!(sas);
         if let Some(ref sas) = *guard {
             for (local, peer) in sas.associations.iter() {
                 for (a, b) in sa_set(*local, *peer) {

bgp/src/lib.rs

@@ -11,7 +11,6 @@ pub mod connection_tcp;
 pub mod dispatcher;
 pub mod error;
 pub mod fanout;
-pub mod log;
 pub mod messages;
 pub mod policy;
 pub mod router;

bgp/src/policy.rs

@@ -403,23 +403,13 @@ pub fn load_checker(program_source: &str) -> Result<AST, Error> {
 
 #[cfg(test)]
 mod test {
-    use slog::Drain;
-
     use crate::messages::{
         Community, PathAttribute, PathAttributeType, PathAttributeTypeCode,
         PathAttributeValue,
     };
 
     use super::*;
-
-    fn log() -> Logger {
-        let drain = slog_bunyan::new(std::io::stdout()).build().fuse();
-        let drain = slog_async::Async::new(drain)
-            .chan_size(0x8000)
-            .build()
-            .fuse();
-        slog::Logger::root(drain, slog::o!())
-    }
+    use mg_common::log::init_logger;
 
     #[test]
     fn open_require_4byte_as() {
@@ -432,13 +422,15 @@ mod test {
                 .unwrap();
         let ast = load_checker(&source).unwrap();
         let result =
-            check_incoming_open(m, &ast, asn.into(), addr, log()).unwrap();
+            check_incoming_open(m, &ast, asn.into(), addr, init_logger())
+                .unwrap();
         assert_eq!(result, CheckerResult::Drop);
 
         // check that open messages with the 4-octet AS capability code get accepted
         let m = OpenMessage::new4(asn.into(), 30, 1701);
         let result =
-            check_incoming_open(m, &ast, asn.into(), addr, log()).unwrap();
+            check_incoming_open(m, &ast, asn.into(), addr, init_logger())
+                .unwrap();
         assert_eq!(result, CheckerResult::Accept);
     }
 
@@ -459,12 +451,14 @@ mod test {
             std::fs::read_to_string("../bgp/policy/policy-check0.rhai")
                 .unwrap();
         let ast = load_checker(&source).unwrap();
-        let result = check_incoming_update(m, &ast, asn, addr, log()).unwrap();
+        let result =
+            check_incoming_update(m, &ast, asn, addr, init_logger()).unwrap();
         assert_eq!(result, CheckerResult::Drop);
 
         // check that messages without the no-export community are accepted
         let m = UpdateMessage::default();
-        let result = check_incoming_update(m, &ast, asn, addr, log()).unwrap();
+        let result =
+            check_incoming_update(m, &ast, asn, addr, init_logger()).unwrap();
         assert_eq!(result, CheckerResult::Accept);
     }
 
@@ -478,9 +472,14 @@ mod test {
             std::fs::read_to_string("../bgp/policy/policy-shape0.rhai")
                 .unwrap();
         let ast = load_shaper(&source).unwrap();
-        let result =
-            shape_outgoing_open(m.clone(), &ast, asn.into(), addr, log())
-                .unwrap();
+        let result = shape_outgoing_open(
+            m.clone(),
+            &ast,
+            asn.into(),
+            addr,
+            init_logger(),
+        )
+        .unwrap();
         m.add_four_octet_as(74);
         assert_eq!(result, ShaperResult::Emit(m.into()));
     }
@@ -503,7 +502,8 @@ mod test {
                 .unwrap();
         let ast = load_shaper(&source).unwrap();
         let result =
-            shape_outgoing_update(m.clone(), &ast, asn, addr, log()).unwrap();
+            shape_outgoing_update(m.clone(), &ast, asn, addr, init_logger())
+                .unwrap();
         m.add_community(Community::UserDefined(1701));
         assert_eq!(result, ShaperResult::Emit(m.into()));
     }
@@ -528,18 +528,23 @@ mod test {
         let ast = load_shaper(&source).unwrap();
 
         // ASN 100 should not have any changes
-        let result: UpdateMessage =
-            shape_outgoing_update(originated.clone(), &ast, 100, addr, log())
-                .unwrap()
-                .unwrap()
-                .try_into()
-                .unwrap();
+        let result: UpdateMessage = shape_outgoing_update(
+            originated.clone(),
+            &ast,
+            100,
+            addr,
+            init_logger(),
+        )
+        .unwrap()
+        .unwrap()
+        .try_into()
+        .unwrap();
 
         assert_eq!(result, originated.clone());
 
         // ASN 65402 should have only the 10.128./16 prefix
         let result: UpdateMessage =
-            shape_outgoing_update(originated, &ast, 65402, addr, log())
+            shape_outgoing_update(originated, &ast, 65402, addr, init_logger())
                 .unwrap()
                 .unwrap()
                 .try_into()