diff --git a/mgd/src/bgp_admin.rs b/mgd/src/bgp_admin.rs index fdec1a44..653d1aa7 100644 --- a/mgd/src/bgp_admin.rs +++ b/mgd/src/bgp_admin.rs @@ -55,7 +55,7 @@ use mg_common::lock; use rdb::{Asn, RibExt}; use slog::Logger; use std::collections::{BTreeMap, HashMap, HashSet}; -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::net::{IpAddr, Ipv6Addr, SocketAddr, SocketAddrV6}; use std::sync::{ Arc, Mutex, @@ -176,18 +176,49 @@ pub async fn delete_router( request: Query, ) -> Result { let rq = request.into_inner(); - let ctx = ctx.context(); - let db = ctx.db.clone(); + do_delete_router(ctx.context(), rq.asn).await?; + Ok(HttpResponseUpdatedNoContent()) +} - db.remove_bgp_router(rq.asn) - .map_err(|e| HttpError::for_internal_error(format!("{e}")))?; +async fn do_delete_router( + ctx: &Arc, + asn: u32, +) -> Result<(), Error> { + // Remove any neighbors homed under this ASN first, otherwise they are + // orphaned in the database when the router goes away (the neighbor trees + // are keyed on peer address/interface, not ASN, so nothing else prunes + // them). See https://github.com/oxidecomputer/maghemite/issues/772. + let numbered: Vec<_> = ctx + .db + .get_bgp_neighbors() + .map_err(Error::Db)? + .into_iter() + .filter(|x| x.asn == asn) + .collect(); + for nbr in numbered { + helpers::remove_neighbor(ctx.clone(), asn, nbr.host.ip()).await?; + } + + let unnumbered: Vec<_> = ctx + .db + .get_unnumbered_bgp_neighbors() + .map_err(Error::Db)? + .into_iter() + .filter(|x| x.asn == asn) + .collect(); + for nbr in unnumbered { + helpers::remove_unnumbered_neighbor(ctx.clone(), asn, &nbr.interface) + .await?; + } + + ctx.db.remove_bgp_router(asn).map_err(Error::Db)?; let mut routers = lock!(ctx.bgp.router); - if let Some(r) = routers.remove(&rq.asn) { + if let Some(r) = routers.remove(&asn) { r.shutdown() }; - Ok(HttpResponseUpdatedNoContent()) + Ok(()) } // Neighbors ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -1235,49 +1266,22 @@ async fn do_bgp_apply( ) -> Result { let log = ctx.log.clone(); - // Validate originate prefixes before processing - validate_prefixes(&rq.originate)?; - bgp_log!(log, info, "bgp apply: {rq:#?}"; "params" => format!("{rq:?}") ); - #[derive(Debug, Eq)] + #[derive(Debug, Eq, Hash, PartialEq)] struct Nbr { addr: IpAddr, asn: u32, } - impl Hash for Nbr { - fn hash(&self, state: &mut H) { - self.addr.hash(state); - } - } - - impl PartialEq for Nbr { - fn eq(&self, other: &Nbr) -> bool { - self.addr.eq(&other.addr) - } - } - - #[derive(Debug, Eq)] + #[derive(Debug, Eq, Hash, PartialEq)] struct Unbr { interface: String, asn: u32, } - impl Hash for Unbr { - fn hash(&self, state: &mut H) { - self.interface.hash(state); - } - } - - impl PartialEq for Unbr { - fn eq(&self, other: &Unbr) -> bool { - self.interface.eq(&other.interface) - } - } - let groups = ctx .db .get_bgp_neighbors() @@ -1285,6 +1289,13 @@ async fn do_bgp_apply( .into_iter() .map(|x| x.group) .collect::>(); + let ugroups = ctx + .db + .get_unnumbered_bgp_neighbors() + .map_err(Error::Db)? + .into_iter() + .map(|x| x.group) + .collect::>(); // Turn any peer groups that are resident in the db but not in the apply // request into empty groups so the difference functions below remove @@ -1296,9 +1307,23 @@ async fn do_bgp_apply( } } let mut upeers = rq.unnumbered_peers.clone(); - for g in &groups { - if !upeers.contains_key(g) { - upeers.insert(g.clone(), Vec::default()); + for u in &ugroups { + if !upeers.contains_key(u) { + upeers.insert(u.clone(), Vec::default()); + } + } + + // Treat bgp_apply endpoint as idempotent: the request carries a single + // ASN, so any other router (and its neighbors) is stale and must be torn + // down completely. Routing this through do_delete_router keeps all router + // teardown (db rows, in-memory router, sessions, neighbors) in one place. + let routers = ctx + .db + .get_bgp_routers() + .map_err(|e| HttpError::for_internal_error(format!("{e}")))?; + for (old_asn, _router) in routers { + if rq.asn != old_asn { + do_delete_router(ctx, old_asn).await?; } } @@ -1332,7 +1357,7 @@ async fn do_bgp_apply( }) .collect(); - let specified_unbr_ifxs: HashSet = peers + let requested_unbr_ifxs: HashSet = peers .iter() .map(|x| Unbr { interface: x.interface.clone(), @@ -1340,14 +1365,23 @@ async fn do_bgp_apply( }) .collect(); - let to_delete = current_unbr_ifxs.difference(&specified_unbr_ifxs); - let to_add = specified_unbr_ifxs.difference(¤t_unbr_ifxs); - let to_modify = current_unbr_ifxs.intersection(&specified_unbr_ifxs); + let to_delete = current_unbr_ifxs.difference(&requested_unbr_ifxs); + let to_add = requested_unbr_ifxs.difference(¤t_unbr_ifxs); + let to_modify = current_unbr_ifxs.intersection(&requested_unbr_ifxs); bgp_log!(log, info, "unbr: current {current:#?}"); bgp_log!(log, info, "unbr: adding {to_add:#?}"); bgp_log!(log, info, "unbr: removing {to_delete:#?}"); + for nbr in to_delete { + helpers::remove_unnumbered_neighbor( + ctx.clone(), + nbr.asn, + &nbr.interface, + ) + .await?; + } + let mut nbr_config = Vec::new(); for nbr in to_add { let cfg = peers @@ -1393,24 +1427,6 @@ async fn do_bgp_apply( true, // ensure mode: create or update as needed )?; } - - for nbr in to_delete { - helpers::remove_unnumbered_neighbor( - ctx.clone(), - nbr.asn, - &nbr.interface, - ) - .await?; - - let mut routers = lock!(ctx.bgp.router); - let mut remove = false; - if let Some(r) = routers.get(&nbr.asn) { - remove = lock!(r.sessions).is_empty(); - } - if remove && let Some(r) = routers.remove(&nbr.asn) { - r.shutdown() - }; - } } for (group, peers) in &peers { @@ -1430,7 +1446,7 @@ async fn do_bgp_apply( }) .collect(); - let specified_nbr_addrs: HashSet = peers + let requested_nbr_addrs: HashSet = peers .iter() .map(|x| Nbr { addr: x.host.ip(), @@ -1438,14 +1454,18 @@ async fn do_bgp_apply( }) .collect(); - let to_delete = current_nbr_addrs.difference(&specified_nbr_addrs); - let to_add = specified_nbr_addrs.difference(¤t_nbr_addrs); - let to_modify = current_nbr_addrs.intersection(&specified_nbr_addrs); + let to_delete = current_nbr_addrs.difference(&requested_nbr_addrs); + let to_add = requested_nbr_addrs.difference(¤t_nbr_addrs); + let to_modify = current_nbr_addrs.intersection(&requested_nbr_addrs); bgp_log!(log, info, "nbr: current {current:#?}"); bgp_log!(log, info, "nbr: adding {to_add:#?}"); bgp_log!(log, info, "nbr: removing {to_delete:#?}"); + for nbr in to_delete { + helpers::remove_neighbor(ctx.clone(), nbr.asn, nbr.addr).await?; + } + let mut nbr_config = Vec::new(); for nbr in to_add { let cfg = peers @@ -1494,21 +1514,11 @@ async fn do_bgp_apply( true, )?; } - - for nbr in to_delete { - helpers::remove_neighbor(ctx.clone(), nbr.asn, nbr.addr).await?; - - let mut routers = lock!(ctx.bgp.router); - let mut remove = false; - if let Some(r) = routers.get(&nbr.asn) { - remove = lock!(r.sessions).is_empty(); - } - if remove && let Some(r) = routers.remove(&nbr.asn) { - r.shutdown() - }; - } } + // Validate originate prefixes before processing + validate_prefixes(&rq.originate)?; + get_router!(ctx, rq.asn)? .set_origin4(rq.originate.clone().into_iter().collect()) .map_err(|e| HttpError::for_internal_error(e.to_string()))?; @@ -2610,14 +2620,19 @@ pub(crate) mod helpers { #[cfg(test)] mod tests { - use super::do_bgp_apply; + use super::{do_bgp_apply, do_delete_router}; use crate::{ admin::HandlerContext, bfd_admin::BfdContext, bgp_admin::BgpContext, }; use bgp::router::SessionMap; - use mg_api_types_versions::v1; - use mg_api_types_versions::v1::bgp::config::{ - ApplyRequest, BgpPeerConfig, BgpPeerParameters, + use mg_api_types_versions::v4::bgp::config::{ + Ipv4UnicastConfig, Ipv6UnicastConfig, + }; + use mg_api_types_versions::v4::bgp::policy::{ + ImportExportPolicy4, ImportExportPolicy6, + }; + use mg_api_types_versions::v8::bgp::config::{ + ApplyRequest, BgpPeerConfig, BgpPeerParameters, UnnumberedBgpPeerConfig, }; use mg_common::{println_nopipe, stats::MgLowerStats}; use rdb::test::get_test_db; @@ -2631,23 +2646,19 @@ mod tests { sync::{Arc, Mutex}, }; - #[tokio::test] - async fn apply_remove_entire_group() { + /// Build a fresh handler context backed by an isolated on-disk test db. + fn test_ctx(name: &str) -> Arc { let tmpdir = temp_dir(); - let tmpdir = format!( - "{}/maghemite-test/apply_remove_entire_group", - tmpdir.to_str().unwrap() - ); + let tmpdir = + format!("{}/maghemite-test/{name}", tmpdir.to_str().unwrap()); if std::fs::exists(&tmpdir).unwrap() { remove_dir_all(&tmpdir).unwrap(); } create_dir_all(&tmpdir).unwrap(); println_nopipe!("tmpdir is {tmpdir}"); - let log = - mg_common::log::init_file_logger("apply_remove_entire_group.log"); - - let db = get_test_db("apply_remove_entire_group", log.clone()).unwrap(); - let ctx = Arc::new(HandlerContext { + let log = mg_common::log::init_file_logger(&format!("{name}.log")); + let db = get_test_db(name, log.clone()).unwrap(); + Arc::new(HandlerContext { #[cfg(all(feature = "mg-lower", target_os = "illumos"))] tep: Ipv6Addr::UNSPECIFIED, bgp: BgpContext::new( @@ -2660,78 +2671,114 @@ mod tests { mg_lower_stats: Arc::new(MgLowerStats::default()), stats_server_running: Mutex::new(false), oximeter_port: 0, - }); + }) + } - let mut peers = HashMap::new(); - peers.insert( - String::from("qsfp0"), - vec![BgpPeerConfig { - host: SocketAddr::new("203.0.113.1".parse().unwrap(), 179), - name: String::from("bob"), - parameters: BgpPeerParameters { - hold_time: 3, - idle_hold_time: 1, - delay_open: 1, - connect_retry: 1, - keepalive: 1, - resolution: 1, - passive: false, - remote_asn: None, - min_ttl: None, - md5_auth_key: None, - multi_exit_discriminator: None, - communities: Vec::default(), - local_pref: None, - enforce_first_as: false, - allow_import: - v1::bgp::policy::ImportExportPolicy::NoFiltering, - allow_export: - v1::bgp::policy::ImportExportPolicy::NoFiltering, - vlan_id: None, - }, - }], - ); - peers.insert( - String::from("qsfp1"), - vec![BgpPeerConfig { - host: SocketAddr::new("203.0.113.2".parse().unwrap(), 179), - name: String::from("alice"), - parameters: BgpPeerParameters { - hold_time: 3, - idle_hold_time: 1, - delay_open: 1, - connect_retry: 1, - keepalive: 1, - resolution: 1, - passive: false, - remote_asn: None, - min_ttl: None, - md5_auth_key: None, - multi_exit_discriminator: None, - communities: Vec::default(), - local_pref: None, - enforce_first_as: false, - allow_import: - v1::bgp::policy::ImportExportPolicy::NoFiltering, - allow_export: - v1::bgp::policy::ImportExportPolicy::NoFiltering, - vlan_id: None, - }, - }], - ); + /// Common peer parameters with both address families enabled. + fn params(hold_time: u64) -> BgpPeerParameters { + BgpPeerParameters { + hold_time, + idle_hold_time: 1, + delay_open: 1, + connect_retry: 1, + keepalive: 1, + resolution: 1, + passive: false, + remote_asn: None, + min_ttl: None, + md5_auth_key: None, + multi_exit_discriminator: None, + communities: Vec::default(), + local_pref: None, + enforce_first_as: false, + vlan_id: None, + ipv4_unicast: Some(Ipv4UnicastConfig { + nexthop: None, + import_policy: ImportExportPolicy4::NoFiltering, + export_policy: ImportExportPolicy4::NoFiltering, + }), + ipv6_unicast: Some(Ipv6UnicastConfig { + nexthop: None, + import_policy: ImportExportPolicy6::NoFiltering, + export_policy: ImportExportPolicy6::NoFiltering, + }), + deterministic_collision_resolution: false, + idle_hold_jitter: None, + connect_retry_jitter: None, + src_addr: None, + src_port: None, + } + } + + fn numbered(ip: &str, name: &str, hold_time: u64) -> BgpPeerConfig { + BgpPeerConfig { + host: SocketAddr::new(ip.parse().unwrap(), 179), + name: name.into(), + parameters: params(hold_time), + } + } - let mut req = ApplyRequest { - asn: 47, + fn unnumbered( + interface: &str, + name: &str, + hold_time: u64, + ) -> UnnumberedBgpPeerConfig { + UnnumberedBgpPeerConfig { + interface: interface.into(), + name: name.into(), + router_lifetime: 1, + parameters: params(hold_time), + } + } + + /// Assemble an apply request from group -> peer lists. + fn apply_req( + asn: u32, + peers: HashMap>, + unnumbered_peers: HashMap>, + ) -> ApplyRequest { + ApplyRequest { + asn, originate: Vec::default(), checker: None, shaper: None, peers, - }; + unnumbered_peers, + } + } + + /// Apply request carrying both a numbered and an unnumbered peer under + /// `asn`, so a single test exercises both code paths at once. + fn mixed_req(asn: u32, hold_time: u64) -> ApplyRequest { + apply_req( + asn, + HashMap::from([( + "qsfp0".into(), + vec![numbered("203.0.113.1", "bob", hold_time)], + )]), + HashMap::from([( + "qsfp1".into(), + vec![unnumbered("tfportqsfp1_0", "u0", hold_time)], + )]), + ) + } - do_bgp_apply(&ctx, req.clone().into()) + #[tokio::test] + async fn apply_remove_entire_group() { + let ctx = test_ctx("apply_remove_entire_group"); + + let mut req = apply_req( + 47, + HashMap::from([ + ("qsfp0".into(), vec![numbered("203.0.113.1", "bob", 3)]), + ("qsfp1".into(), vec![numbered("203.0.113.2", "alice", 3)]), + ]), + HashMap::default(), + ); + + do_bgp_apply(&ctx, req.clone()) .await .expect("bgp apply request"); - assert_eq!( ctx.db.get_bgp_neighbors().expect("get bgp neighbors").len(), 2, @@ -2739,13 +2786,165 @@ mod tests { req.peers.remove("qsfp0"); - do_bgp_apply(&ctx, req.clone().into()) + do_bgp_apply(&ctx, req.clone()) .await .expect("bgp apply request"); + assert_eq!( + ctx.db.get_bgp_neighbors().expect("get bgp neighbors").len(), + 1, + ); + } + + /// Regression test for https://github.com/oxidecomputer/maghemite/issues/772 + /// + /// Re-applying the same peers (same address/interface and group) under a + /// *different* ASN must move them to the new ASN rather than leave them + /// attached to the old one. Numbered neighbors are keyed on peer IP and + /// unnumbered on interface — neither carries an ASN in its db key — so this + /// covers both: the old entries are deleted and re-added under the new ASN, + /// and the now-empty old router is torn down. + #[tokio::test] + async fn apply_change_peer_asn() { + let ctx = test_ctx("apply_change_peer_asn"); + + do_bgp_apply(&ctx, mixed_req(123, 6)) + .await + .expect("apply asn 123"); + let num = ctx.db.get_bgp_neighbors().expect("get bgp neighbors"); + let unum = ctx + .db + .get_unnumbered_bgp_neighbors() + .expect("get unnumbered neighbors"); + assert_eq!(num.len(), 1); + assert_eq!(num[0].asn, 123); + assert_eq!(num[0].parameters.hold_time, 6); + assert_eq!(unum.len(), 1); + assert_eq!(unum[0].asn, 123); + assert_eq!(unum[0].parameters.hold_time, 6); + + do_bgp_apply(&ctx, mixed_req(456, 10)) + .await + .expect("apply asn 456"); + + // Both peers must have moved to ASN 456, not stayed on ASN 123. + let num = ctx.db.get_bgp_neighbors().expect("get bgp neighbors"); + let unum = ctx + .db + .get_unnumbered_bgp_neighbors() + .expect("get unnumbered neighbors"); + assert_eq!(num.len(), 1, "expected exactly one numbered neighbor"); + assert_eq!(num[0].asn, 456, "numbered peer should re-home under 456"); + assert_eq!(num[0].parameters.hold_time, 10); + assert_eq!(unum.len(), 1, "expected exactly one unnumbered neighbor"); + assert_eq!( + unum[0].asn, 456, + "unnumbered peer should re-home under 456" + ); + assert_eq!(unum[0].parameters.hold_time, 10); + + // The now-empty old router should be gone, leaving only ASN 456. + let routers = ctx.bgp.router.lock().unwrap(); + assert_eq!( + ctx.db + .get_bgp_routers() + .expect("get routers") + .into_keys() + .collect::>(), + vec![456], + ); + assert!(!routers.contains_key(&123)); + assert!(routers.contains_key(&456)); + } + + /// Regression test for https://github.com/oxidecomputer/maghemite/issues/772 + /// + /// Applying a config with empty peer maps for an ASN that previously had + /// peers must drain those peers (treating apply as the desired full state), + /// rather than no-op'ing because no group was named in the request. The + /// numbered and unnumbered group sets are tracked independently, so this + /// exercises both. + #[tokio::test] + async fn apply_empty_removes_peers() { + let ctx = test_ctx("apply_empty_removes_peers"); + + do_bgp_apply(&ctx, mixed_req(123, 6)) + .await + .expect("apply with peers"); + assert_eq!( + ctx.db.get_bgp_neighbors().expect("get bgp neighbors").len(), + 1, + ); + assert_eq!( + ctx.db + .get_unnumbered_bgp_neighbors() + .expect("get unnumbered neighbors") + .len(), + 1, + ); + + // Re-apply ASN 123 with no peers at all. + do_bgp_apply( + &ctx, + apply_req(123, HashMap::default(), HashMap::default()), + ) + .await + .expect("apply empty"); + assert_eq!( + ctx.db.get_bgp_neighbors().expect("get bgp neighbors").len(), + 0, + "empty apply should drain previously-configured numbered peers", + ); + assert_eq!( + ctx.db + .get_unnumbered_bgp_neighbors() + .expect("get unnumbered neighbors") + .len(), + 0, + "empty apply should drain previously-configured unnumbered peers", + ); + } + + /// Regression test for https://github.com/oxidecomputer/maghemite/issues/772 + /// + /// Deleting a router must not leave its neighbors orphaned in the database, + /// for either numbered or unnumbered neighbors (the unnumbered case being + /// the one originally reported in the issue). + #[tokio::test] + async fn delete_router_removes_neighbors() { + let ctx = test_ctx("delete_router_removes_neighbors"); + + do_bgp_apply(&ctx, mixed_req(123, 6)) + .await + .expect("apply with peers"); assert_eq!( ctx.db.get_bgp_neighbors().expect("get bgp neighbors").len(), 1, ); + assert_eq!( + ctx.db + .get_unnumbered_bgp_neighbors() + .expect("get unnumbered neighbors") + .len(), + 1, + ); + + do_delete_router(&ctx, 123).await.expect("delete router"); + + assert!(ctx.db.get_bgp_routers().expect("get routers").is_empty()); + assert!(!ctx.bgp.router.lock().unwrap().contains_key(&123)); + assert_eq!( + ctx.db.get_bgp_neighbors().expect("get bgp neighbors").len(), + 0, + "deleting a router must not orphan its numbered neighbors", + ); + assert_eq!( + ctx.db + .get_unnumbered_bgp_neighbors() + .expect("get unnumbered neighbors") + .len(), + 0, + "deleting a router must not orphan its unnumbered neighbors", + ); } }