From 17032cea56b99f349671ba4ca15ca4d1ed9c959c Mon Sep 17 00:00:00 2001 From: Trey Aspelund Date: Tue, 2 Jun 2026 16:11:06 -0600 Subject: [PATCH 1/4] bgp_apply: add failing test for #772 Signed-off-by: Trey Aspelund --- mgd/src/bgp_admin.rs | 132 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 132 insertions(+) diff --git a/mgd/src/bgp_admin.rs b/mgd/src/bgp_admin.rs index fdec1a44..36724536 100644 --- a/mgd/src/bgp_admin.rs +++ b/mgd/src/bgp_admin.rs @@ -2748,4 +2748,136 @@ mod tests { 1, ); } + + /// Regression test for https://github.com/oxidecomputer/maghemite/issues/772 + /// + /// Applying an unnumbered peer under one ASN and then re-applying the same + /// peer (same interface/group) under a *different* ASN must move the peer + /// to the new ASN, not silently leave it attached to the old ASN. Today + /// `do_bgp_apply` keys the diff on interface only (ignoring ASN), so the + /// re-apply mutates the existing ASN-123 neighbor in place instead of + /// re-homing it under ASN 456. + #[tokio::test] + async fn apply_change_unnumbered_peer_asn() { + use mg_api_types_versions::v4::bgp::config::Ipv4UnicastConfig; + use mg_api_types_versions::v4::bgp::config::Ipv6UnicastConfig; + use mg_api_types_versions::v4::bgp::policy::{ + ImportExportPolicy4, ImportExportPolicy6, + }; + use mg_api_types_versions::v8::bgp::config::{ + ApplyRequest as ApplyRequestV8, BgpPeerParameters, + UnnumberedBgpPeerConfig, + }; + + let tmpdir = temp_dir(); + let tmpdir = format!( + "{}/maghemite-test/apply_change_unnumbered_peer_asn", + 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_change_unnumbered_peer_asn.log", + ); + + let db = get_test_db("apply_change_unnumbered_peer_asn", log.clone()) + .unwrap(); + let ctx = Arc::new(HandlerContext { + #[cfg(all(feature = "mg-lower", target_os = "illumos"))] + tep: Ipv6Addr::UNSPECIFIED, + bgp: BgpContext::new( + Arc::new(Mutex::new(SessionMap::new())), + log.clone(), + ), + bfd: BfdContext::new(log.clone()), + log: log.clone(), + db: (*db).clone(), + mg_lower_stats: Arc::new(MgLowerStats::default()), + stats_server_running: Mutex::new(false), + oximeter_port: 0, + }); + + // Build an unnumbered peer on qsfp0 with the given hold_time. + let peer = |hold_time: u64| UnnumberedBgpPeerConfig { + interface: String::from("tfportqsfp0_0"), + name: String::from("unnumbered-qsfp0"), + router_lifetime: 1, + parameters: BgpPeerParameters { + hold_time, + idle_hold_time: 3, + delay_open: 0, + connect_retry: 3, + keepalive: 2, + resolution: 100, + 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, + }, + }; + + let req = |asn: u32, hold_time: u64| { + let mut unnumbered_peers = HashMap::new(); + unnumbered_peers + .insert(String::from("qsfp0"), vec![peer(hold_time)]); + ApplyRequestV8 { + asn, + originate: Vec::default(), + checker: None, + shaper: None, + peers: HashMap::default(), + unnumbered_peers, + } + }; + + // Apply the peer under ASN 123. + do_bgp_apply(&ctx, req(123, 6)) + .await + .expect("apply asn 123"); + + let nbrs = ctx + .db + .get_unnumbered_bgp_neighbors() + .expect("get unnumbered neighbors"); + assert_eq!(nbrs.len(), 1); + assert_eq!(nbrs[0].asn, 123); + assert_eq!(nbrs[0].parameters.hold_time, 6); + + // Re-apply the same peer under ASN 456 with a new hold_time. + do_bgp_apply(&ctx, req(456, 10)) + .await + .expect("apply asn 456"); + + // The peer must have moved to ASN 456, not stayed on ASN 123. + let nbrs = ctx + .db + .get_unnumbered_bgp_neighbors() + .expect("get unnumbered neighbors"); + assert_eq!(nbrs.len(), 1, "expected exactly one unnumbered neighbor"); + assert_eq!(nbrs[0].asn, 456, "peer should be re-homed under ASN 456"); + assert_eq!(nbrs[0].parameters.hold_time, 10); + } } From 2a6bc19d794f1a23efcd472faa45a1a75e2a8fea Mon Sep 17 00:00:00 2001 From: Trey Aspelund Date: Tue, 2 Jun 2026 15:47:08 -0600 Subject: [PATCH 2/4] bgp_apply: del old routers, walk unnumbered groups The bgp_apply API enpdoint is meant to be idempotent and to fully define the state of the world from a BGP config standpoint. This updates the bgp_apply API handler to delete all existing BGP routers except for any with an ASN matching the API request. This also adds missing logic to collect unnumbered peer groups, rather than comparing unnumbered peers against numbered peer groups. Signed-off-by: Trey Aspelund --- mgd/src/bgp_admin.rs | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/mgd/src/bgp_admin.rs b/mgd/src/bgp_admin.rs index 36724536..333a8fe9 100644 --- a/mgd/src/bgp_admin.rs +++ b/mgd/src/bgp_admin.rs @@ -1235,9 +1235,6 @@ 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:?}") ); @@ -1285,6 +1282,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 +1300,21 @@ 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. + // If the request only has one ASN, then all others should be removed. + 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 { + ctx.db.remove_bgp_router(old_asn).map_err(Error::Db)?; } } @@ -1509,6 +1525,9 @@ async fn do_bgp_apply( } } + // 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()))?; From 0f2c12ba6e70cbf65509bbfe76e224461d9a21ae Mon Sep 17 00:00:00 2001 From: Trey Aspelund Date: Tue, 2 Jun 2026 16:27:02 -0600 Subject: [PATCH 3/4] bgp_apply: use asn in nbr hash, delete before add The Nbr/Unbr types scoped to do_bgp_apply() were manually implementing PartialEq and Hash using just the IP/Interface fields from the peers in the API request, which discarded the ASN. In the case of an ASN update, a peer needs the ASN to distinguish between (Peer, old ASN) and (Peer, new ASN) so it doesn't get put into the to_modify bucket. This removes the manual trait impls in lieu of deriving them. With that in place, to_add gets (Peer, new ASN) and to_remove gets (Peer, old ASN). However, because deletes are processed last, the peer is removed in the final state rather than added to the new ASN. Moving the deletes ahead of the adds/changes resolves the order of operations problem, resulting in (Peer, new ASN) as the final state. Signed-off-by: Trey Aspelund --- mgd/src/bgp_admin.rs | 108 +++++++++++++++++-------------------------- 1 file changed, 42 insertions(+), 66 deletions(-) diff --git a/mgd/src/bgp_admin.rs b/mgd/src/bgp_admin.rs index 333a8fe9..df8b504e 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, @@ -1239,42 +1239,18 @@ async fn do_bgp_apply( "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() @@ -1348,7 +1324,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(), @@ -1356,14 +1332,32 @@ 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 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() + }; + } + let mut nbr_config = Vec::new(); for nbr in to_add { let cfg = peers @@ -1409,24 +1403,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 { @@ -1446,7 +1422,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(), @@ -1454,14 +1430,27 @@ 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 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() + }; + } + let mut nbr_config = Vec::new(); for nbr in to_add { let cfg = peers @@ -1510,19 +1499,6 @@ 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 From 599e493354f57bbf7bbe7f25645e5c39dba85ffb Mon Sep 17 00:00:00 2001 From: Trey Aspelund Date: Tue, 2 Jun 2026 16:52:02 -0600 Subject: [PATCH 4/4] bgp_apply: consolidate deletes in do_delete_router Extract do_delete_router so router deletion prunes the router's neighbors instead of orphaning them, and have bgp_apply reuse it for stale-router cleanup rather than reaping routers inline. Add tests for ASN changes, empty applies, and router deletion. Signed-off-by: Trey Aspelund --- mgd/src/bgp_admin.rs | 514 ++++++++++++++++++++++++------------------- 1 file changed, 293 insertions(+), 221 deletions(-) diff --git a/mgd/src/bgp_admin.rs b/mgd/src/bgp_admin.rs index df8b504e..653d1aa7 100644 --- a/mgd/src/bgp_admin.rs +++ b/mgd/src/bgp_admin.rs @@ -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 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -1282,15 +1313,17 @@ async fn do_bgp_apply( } } - // Treat bgp_apply endpoint as idempotent. - // If the request only has one ASN, then all others should be removed. + // 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 { - ctx.db.remove_bgp_router(old_asn).map_err(Error::Db)?; + do_delete_router(ctx, old_asn).await?; } } @@ -1347,15 +1380,6 @@ async fn do_bgp_apply( &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() - }; } let mut nbr_config = Vec::new(); @@ -1440,15 +1464,6 @@ async fn do_bgp_apply( 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() - }; } let mut nbr_config = Vec::new(); @@ -2605,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; @@ -2626,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( @@ -2655,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, + } + } - do_bgp_apply(&ctx, req.clone().into()) + /// 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)], + )]), + ) + } + + #[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, @@ -2734,10 +2786,9 @@ 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, @@ -2746,133 +2797,154 @@ mod tests { /// Regression test for https://github.com/oxidecomputer/maghemite/issues/772 /// - /// Applying an unnumbered peer under one ASN and then re-applying the same - /// peer (same interface/group) under a *different* ASN must move the peer - /// to the new ASN, not silently leave it attached to the old ASN. Today - /// `do_bgp_apply` keys the diff on interface only (ignoring ASN), so the - /// re-apply mutates the existing ASN-123 neighbor in place instead of - /// re-homing it under ASN 456. + /// 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_unnumbered_peer_asn() { - use mg_api_types_versions::v4::bgp::config::Ipv4UnicastConfig; - use mg_api_types_versions::v4::bgp::config::Ipv6UnicastConfig; - use mg_api_types_versions::v4::bgp::policy::{ - ImportExportPolicy4, ImportExportPolicy6, - }; - use mg_api_types_versions::v8::bgp::config::{ - ApplyRequest as ApplyRequestV8, BgpPeerParameters, - UnnumberedBgpPeerConfig, - }; + async fn apply_change_peer_asn() { + let ctx = test_ctx("apply_change_peer_asn"); - let tmpdir = temp_dir(); - let tmpdir = format!( - "{}/maghemite-test/apply_change_unnumbered_peer_asn", - 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_change_unnumbered_peer_asn.log", - ); - - let db = get_test_db("apply_change_unnumbered_peer_asn", log.clone()) - .unwrap(); - let ctx = Arc::new(HandlerContext { - #[cfg(all(feature = "mg-lower", target_os = "illumos"))] - tep: Ipv6Addr::UNSPECIFIED, - bgp: BgpContext::new( - Arc::new(Mutex::new(SessionMap::new())), - log.clone(), - ), - bfd: BfdContext::new(log.clone()), - log: log.clone(), - db: (*db).clone(), - mg_lower_stats: Arc::new(MgLowerStats::default()), - stats_server_running: Mutex::new(false), - oximeter_port: 0, - }); - - // Build an unnumbered peer on qsfp0 with the given hold_time. - let peer = |hold_time: u64| UnnumberedBgpPeerConfig { - interface: String::from("tfportqsfp0_0"), - name: String::from("unnumbered-qsfp0"), - router_lifetime: 1, - parameters: BgpPeerParameters { - hold_time, - idle_hold_time: 3, - delay_open: 0, - connect_retry: 3, - keepalive: 2, - resolution: 100, - 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, - }, - }; - - let req = |asn: u32, hold_time: u64| { - let mut unnumbered_peers = HashMap::new(); - unnumbered_peers - .insert(String::from("qsfp0"), vec![peer(hold_time)]); - ApplyRequestV8 { - asn, - originate: Vec::default(), - checker: None, - shaper: None, - peers: HashMap::default(), - unnumbered_peers, - } - }; - - // Apply the peer under ASN 123. - do_bgp_apply(&ctx, req(123, 6)) + do_bgp_apply(&ctx, mixed_req(123, 6)) .await .expect("apply asn 123"); - let nbrs = ctx + 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!(nbrs.len(), 1); - assert_eq!(nbrs[0].asn, 123); - assert_eq!(nbrs[0].parameters.hold_time, 6); - - // Re-apply the same peer under ASN 456 with a new hold_time. - do_bgp_apply(&ctx, req(456, 10)) + 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"); - // The peer must have moved to ASN 456, not stayed on ASN 123. - let nbrs = ctx + // 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!(nbrs.len(), 1, "expected exactly one unnumbered neighbor"); - assert_eq!(nbrs[0].asn, 456, "peer should be re-homed under ASN 456"); - assert_eq!(nbrs[0].parameters.hold_time, 10); + 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", + ); } }