diff --git a/Cargo.lock b/Cargo.lock index 47f8253951d..50d988212ec 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7100,6 +7100,7 @@ dependencies = [ "scim2-rs", "serde", "serde_json", + "slog-error-chain", "trust-quorum-types", "tufaceous-artifact", "uuid", @@ -7802,6 +7803,7 @@ dependencies = [ "sled-hardware-types", "slog-error-chain", "strum 0.27.2", + "thiserror 2.0.18", "tough", "tufaceous-artifact", "url", diff --git a/nexus/db-model/src/switch_port.rs b/nexus/db-model/src/switch_port.rs index 1184fe6e222..dc35dad575a 100644 --- a/nexus/db-model/src/switch_port.rs +++ b/nexus/db-model/src/switch_port.rs @@ -26,7 +26,10 @@ use serde::{Deserialize, Serialize}; use sled_agent_types::early_networking::ImportExportPolicy; use sled_agent_types::early_networking::PortFec; use sled_agent_types::early_networking::PortSpeed; +use sled_agent_types::early_networking::RouterLifetimeConfig; +use sled_agent_types::early_networking::RouterPeerType; use sled_agent_types::early_networking::SwitchSlot; +use std::net::IpAddr; use uuid::Uuid; impl_enum_type!( @@ -784,12 +787,23 @@ impl SwitchPortBgpPeerConfig { interface_name: Name, p: &networking_types::BgpPeer, ) -> Self { + // Numbered peers are represented as a non-NULL `addr` and an arbitrary + // `router_lifetime` (we just use the default). Unnumbered are + // represented as a NULL `addr` and their desired `router_lifetime`. + let (addr, router_lifetime) = match p.addr { + RouterPeerType::Numbered { ip } => { + (Some(IpAddr::from(ip)), RouterLifetimeConfig::default()) + } + RouterPeerType::Unnumbered { router_lifetime } => { + (None, router_lifetime) + } + }; Self { id: Uuid::new_v4(), port_settings_id, bgp_config_id, interface_name, - addr: p.addr.map(|a| a.into()), + addr: addr.map(From::from), hold_time: p.hold_time.into(), idle_hold_time: p.idle_hold_time.into(), delay_open: p.delay_open.into(), @@ -812,7 +826,7 @@ impl SwitchPortBgpPeerConfig { _ => true, }, vlan_id: p.vlan_id.map(|x| x.into()), - router_lifetime: p.router_lifetime.into(), + router_lifetime: router_lifetime.as_u16().into(), } } } diff --git a/nexus/db-queries/src/db/datastore/bgp.rs b/nexus/db-queries/src/db/datastore/bgp.rs index edf612d6fca..86968a08087 100644 --- a/nexus/db-queries/src/db/datastore/bgp.rs +++ b/nexus/db-queries/src/db/datastore/bgp.rs @@ -18,12 +18,14 @@ use nexus_db_model::{ }; use nexus_types::external_api::networking; use nexus_types::identity::Resource; +use omicron_common::api::external; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::{ CreateResult, DeleteResult, Error, ListResultVec, LookupResult, NameOrId, ResourceType, }; use ref_cast::RefCast; +use sled_agent_types::early_networking::RouterPeerType; use sled_agent_types::early_networking::SwitchSlot; use std::net::IpAddr; use uuid::Uuid; @@ -832,26 +834,22 @@ impl DataStore { } /// Look up communities for a BGP peer. - /// - /// For numbered peers, pass `Some(addr)`. For unnumbered peers, pass `None` - /// (the function will query using the sentinel value 0.0.0.0/32). pub async fn communities_for_peer( &self, opctx: &OpContext, port_settings_id: Uuid, - interface_name: &str, - addr: Option, + interface_name: &external::Name, + addr: RouterPeerType, ) -> ListResultVec { use nexus_db_schema::schema::switch_port_settings_bgp_peer_config_communities::dsl; - // For unnumbered peers (addr is None), use UNSPECIFIED as sentinel - let db_addr: IpNetwork = addr - .unwrap_or_else(|| IpAddr::V4(std::net::Ipv4Addr::UNSPECIFIED)) - .into(); + // For unnumbered peers (addr is None), use sentinel value + let db_addr: IpNetwork = + addr.ip_squashing_unnumbered_to_sentinel().into(); let results = dsl::switch_port_settings_bgp_peer_config_communities .filter(dsl::port_settings_id.eq(port_settings_id)) - .filter(dsl::interface_name.eq(interface_name.to_owned())) + .filter(dsl::interface_name.eq(interface_name.to_string())) .filter(dsl::addr.eq(db_addr)) .load_async(&*self.pool_connection_authorized(opctx).await?) .await @@ -865,25 +863,22 @@ impl DataStore { } /// Look up allowed exports for a BGP peer. - /// - /// For numbered peers, pass `Some(addr)`. For unnumbered peers, pass `None`. pub async fn allow_export_for_peer( &self, opctx: &OpContext, port_settings_id: Uuid, - interface_name: &str, - addr: Option, + interface_name: &external::Name, + addr: RouterPeerType, ) -> LookupResult>> { use nexus_db_schema::schema::switch_port_settings_bgp_peer_config as db_peer; use nexus_db_schema::schema::switch_port_settings_bgp_peer_config::dsl as peer_dsl; use nexus_db_schema::schema::switch_port_settings_bgp_peer_config_allow_export as db_allow; use nexus_db_schema::schema::switch_port_settings_bgp_peer_config_allow_export::dsl; - // For unnumbered peers (addr is None), use UNSPECIFIED as sentinel - // for the allow_export table (which has non-nullable addr) - let db_addr: IpNetwork = addr - .unwrap_or_else(|| IpAddr::V4(std::net::Ipv4Addr::UNSPECIFIED)) - .into(); + // For unnumbered peers (addr is None), use sentinel value for the + // allow_export table (which has non-nullable addr) + let db_addr: IpNetwork = + addr.ip_squashing_unnumbered_to_sentinel().into(); let conn = self.pool_connection_authorized(opctx).await?; let err = OptionalError::new(); @@ -893,24 +888,27 @@ impl DataStore { async move { // Query the main peer config table. For unnumbered peers, // addr is NULL; for numbered peers, addr matches. - let active = if let Some(addr) = addr { - let addr = IpNetwork::from(addr); - peer_dsl::switch_port_settings_bgp_peer_config - .filter(db_peer::port_settings_id.eq(port_settings_id)) - .filter(db_peer::addr.eq(addr)) - .select(db_peer::allow_export_list_active) - .limit(1) - .first_async::(&conn) - .await - } else { - peer_dsl::switch_port_settings_bgp_peer_config - .filter(db_peer::port_settings_id.eq(port_settings_id)) - .filter(db_peer::addr.is_null()) - .filter(db_peer::interface_name.eq(interface_name.to_owned())) - .select(db_peer::allow_export_list_active) - .limit(1) - .first_async::(&conn) - .await + let active = match addr { + RouterPeerType::Numbered { ip } => { + let addr = IpNetwork::from(IpAddr::from(ip)); + peer_dsl::switch_port_settings_bgp_peer_config + .filter(db_peer::port_settings_id.eq(port_settings_id)) + .filter(db_peer::addr.eq(addr)) + .select(db_peer::allow_export_list_active) + .limit(1) + .first_async::(&conn) + .await + } + RouterPeerType::Unnumbered { .. } => { + peer_dsl::switch_port_settings_bgp_peer_config + .filter(db_peer::port_settings_id.eq(port_settings_id)) + .filter(db_peer::addr.is_null()) + .filter(db_peer::interface_name.eq(interface_name.to_string())) + .select(db_peer::allow_export_list_active) + .limit(1) + .first_async::(&conn) + .await + } }; let active = active.map_err(|e| { @@ -937,7 +935,7 @@ impl DataStore { ) .filter( db_allow::interface_name - .eq(interface_name.to_owned()), + .eq(interface_name.to_string()), ) .filter(db_allow::addr.eq(db_addr)) .load_async(&conn) @@ -960,25 +958,22 @@ impl DataStore { } /// Look up allowed imports for a BGP peer. - /// - /// For numbered peers, pass `Some(addr)`. For unnumbered peers, pass `None`. pub async fn allow_import_for_peer( &self, opctx: &OpContext, port_settings_id: Uuid, - interface_name: &str, - addr: Option, + interface_name: &external::Name, + addr: RouterPeerType, ) -> LookupResult>> { use nexus_db_schema::schema::switch_port_settings_bgp_peer_config as db_peer; use nexus_db_schema::schema::switch_port_settings_bgp_peer_config::dsl as peer_dsl; use nexus_db_schema::schema::switch_port_settings_bgp_peer_config_allow_import as db_allow; use nexus_db_schema::schema::switch_port_settings_bgp_peer_config_allow_import::dsl; - // For unnumbered peers (addr is None), use UNSPECIFIED as sentinel - // for the allow_import table (which has non-nullable addr) - let db_addr: IpNetwork = addr - .unwrap_or_else(|| IpAddr::V4(std::net::Ipv4Addr::UNSPECIFIED)) - .into(); + // For unnumbered peers (addr is None), use sentinel value for the + // allow_import table (which has non-nullable addr) + let db_addr: IpNetwork = + addr.ip_squashing_unnumbered_to_sentinel().into(); let err = OptionalError::new(); let conn = self.pool_connection_authorized(opctx).await?; @@ -989,24 +984,27 @@ impl DataStore { async move { // Query the main peer config table. For unnumbered peers, // addr is NULL; for numbered peers, addr matches. - let active = if let Some(addr) = addr { - let addr = IpNetwork::from(addr); - peer_dsl::switch_port_settings_bgp_peer_config - .filter(db_peer::port_settings_id.eq(port_settings_id)) - .filter(db_peer::addr.eq(addr)) - .select(db_peer::allow_import_list_active) - .limit(1) - .first_async::(&conn) - .await - } else { - peer_dsl::switch_port_settings_bgp_peer_config - .filter(db_peer::port_settings_id.eq(port_settings_id)) - .filter(db_peer::addr.is_null()) - .filter(db_peer::interface_name.eq(interface_name.to_owned())) - .select(db_peer::allow_import_list_active) - .limit(1) - .first_async::(&conn) - .await + let active = match addr { + RouterPeerType::Numbered { ip } => { + let addr = IpNetwork::from(IpAddr::from(ip)); + peer_dsl::switch_port_settings_bgp_peer_config + .filter(db_peer::port_settings_id.eq(port_settings_id)) + .filter(db_peer::addr.eq(addr)) + .select(db_peer::allow_import_list_active) + .limit(1) + .first_async::(&conn) + .await + } + RouterPeerType::Unnumbered { .. } => { + peer_dsl::switch_port_settings_bgp_peer_config + .filter(db_peer::port_settings_id.eq(port_settings_id)) + .filter(db_peer::addr.is_null()) + .filter(db_peer::interface_name.eq(interface_name.to_string())) + .select(db_peer::allow_import_list_active) + .limit(1) + .first_async::(&conn) + .await + } }; let active = active.map_err(|e| { @@ -1033,7 +1031,7 @@ impl DataStore { ) .filter( db_allow::interface_name - .eq(interface_name.to_owned()), + .eq(interface_name.to_string()), ) .filter(db_allow::addr.eq(db_addr)) .load_async(&conn) diff --git a/nexus/db-queries/src/db/datastore/switch_port.rs b/nexus/db-queries/src/db/datastore/switch_port.rs index 0eb876b1bb3..8bf25eba10f 100644 --- a/nexus/db-queries/src/db/datastore/switch_port.rs +++ b/nexus/db-queries/src/db/datastore/switch_port.rs @@ -2,9 +2,6 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use std::collections::BTreeMap; -use std::net::{IpAddr, Ipv4Addr}; - use super::DataStore; use crate::context::OpContext; use crate::db::datastore::UpdatePrecondition; @@ -35,6 +32,7 @@ use nexus_db_model::{ SwitchPortBgpPeerConfigCommunity, }; use nexus_types::external_api::networking; +use nexus_types::external_api::networking::router_peer_type_try_from_old_representation; use nexus_types::identity::Resource; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::{ @@ -44,20 +42,26 @@ use omicron_common::api::external::{ use ref_cast::RefCast; use serde::{Deserialize, Serialize}; use sled_agent_types::early_networking::ImportExportPolicy; +use sled_agent_types::early_networking::RouterPeerType; use sled_agent_types::early_networking::SwitchSlot; +use slog_error_chain::InlineErrorChain; +use std::collections::BTreeMap; +use std::net::IpAddr; use uuid::Uuid; -/// This is a wrapper around [`networking::BgpPeer`], with two additions: +/// This is a wrapper around [`networking::BgpPeer`], with three additions: /// /// * [`BgpPeerFromDb::bgp_config_id`] is guaranteed to be an ID, unlike the /// inner [`networking::BgpPeer::bgp_config`] which might be an ID or might be /// a [`Name`]. -/// * Adds [`BgpPeerFromDb::port_settings_id`], which doesn't exist in -/// inner [`networking::BgpPeer`]. +/// * Adds [`BgpPeerFromDb::port_settings_id`] and +/// [`BgpPeerFromDb::interface_name`], which don't exist in the inner +/// [`networking::BgpPeer`]. #[derive(Clone, Debug)] pub struct BgpPeerFromDb { port_settings_id: Uuid, bgp_config_id: Uuid, + interface_name: external::Name, inner: networking::BgpPeer, } @@ -79,6 +83,10 @@ impl BgpPeerFromDb { pub fn as_bgp_peer(&self) -> &networking::BgpPeer { &self.inner } + + pub fn interface_name(&self) -> &external::Name { + &self.interface_name + } } // Helper to build a `BgpPeerFromDb`. This is a struct instead of a function so @@ -92,20 +100,38 @@ struct BgpPeerFromDbBuilder<'a> { } impl BgpPeerFromDbBuilder<'_> { - fn build(self) -> BgpPeerFromDb { + fn build(self) -> Result { let Self { peer_config: p, communities, allowed_import, allowed_export, } = self; - BgpPeerFromDb { + + // This will fail if we have a non-NULL but invalid address in the DB, + // or if we have an unnumbered address with a too-large + // `router_lifetime`. We should have CHECK constraints to prevent both + // of these, but don't today. + let addr = router_peer_type_try_from_old_representation( + p.addr.map(|a| a.ip()), + *p.router_lifetime, + ) + .map_err(|err| { + format!( + "invalid database contents for BGP peer {} ({:?}): {}", + p.bgp_config_id, + p.addr, + InlineErrorChain::new(&err), + ) + })?; + + Ok(BgpPeerFromDb { port_settings_id: p.port_settings_id, bgp_config_id: p.bgp_config_id, + interface_name: p.interface_name.clone().into(), inner: networking::BgpPeer { bgp_config: p.bgp_config_id.into(), - interface_name: p.interface_name.clone().into(), - addr: p.addr.map(|a| a.ip()), + addr, hold_time: p.hold_time.into(), idle_hold_time: p.idle_hold_time.into(), delay_open: p.delay_open.into(), @@ -120,12 +146,11 @@ impl BgpPeerFromDbBuilder<'_> { local_pref: p.local_pref.map(From::from), enforce_first_as: p.enforce_first_as, vlan_id: p.vlan_id.map(From::from), - router_lifetime: p.router_lifetime.into(), communities, allowed_import, allowed_export, }, - } + }) } } @@ -269,20 +294,7 @@ impl DataStore { .await .map_err(|e| { if let Some(err) = err.take() { - match err { - SpsCreateError::AddressLotNotFound => { - Error::invalid_request("AddressLot not found") - } - SpsCreateError::BgpConfigNotFound => { - Error::invalid_request("BGP config not found") - } - SwitchPortSettingsCreateError::ReserveBlock( - ReserveBlockError::AddressUnavailable, - ) => Error::invalid_request("address unavailable"), - SwitchPortSettingsCreateError::ReserveBlock( - ReserveBlockError::AddressNotInLot, - ) => Error::invalid_request("address not in lot"), - } + Error::from(err) } else { public_error_from_diesel( e, @@ -361,7 +373,9 @@ impl DataStore { let selector = NameOrId::Id(id); async move { do_switch_port_settings_delete(&conn, &selector, delete_err).await?; - do_switch_port_settings_create(&conn, Some(id), params, create_err).await + do_switch_port_settings_create( + &conn, Some(id), params, create_err, + ).await } }) .await @@ -374,21 +388,7 @@ impl DataStore { } } else if let Some(err) = create_err.take() { - match err { - SpsCreateError::AddressLotNotFound => { - Error::invalid_request("AddressLot not found") - } - SpsCreateError::BgpConfigNotFound => { - Error::invalid_request("BGP config not found") - } - SwitchPortSettingsCreateError::ReserveBlock( - ReserveBlockError::AddressUnavailable, - ) => Error::invalid_request("address unavailable"), - SwitchPortSettingsCreateError::ReserveBlock( - ReserveBlockError::AddressNotInLot, - ) => Error::invalid_request("address not in lot"), - - } + Error::from(err) } else { public_error_from_diesel(e, ErrorHandler::Server) @@ -428,6 +428,7 @@ impl DataStore { #[derive(Debug)] enum SwitchPortSettingsGetError { NotFound(NameOrId), + InternalError(String), } let err = OptionalError::new(); @@ -639,10 +640,11 @@ impl DataStore { .await?; for p in peers.iter() { - // For unnumbered peers (addr is None), use the sentinel value - // (UNSPECIFIED address) for lookups since that's how they're stored. + // For unnumbered peers (addr is None), use the sentinel + // value (UNSPECIFIED address) for lookups since that's how + // they're stored. let lookup_addr: IpNetwork = p.addr.unwrap_or_else(|| { - IpNetwork::from(IpAddr::V4(Ipv4Addr::UNSPECIFIED)) + IpNetwork::from(RouterPeerType::UNNUMBERED_SENTINEL) }); let allowed_import: ImportExportPolicy = if p.allow_import_list_active { @@ -692,7 +694,7 @@ impl DataStore { .load_async::(&conn) .await?; - result.bgp_peers.push(BgpPeerFromDbBuilder { + let peer_result = BgpPeerFromDbBuilder { peer_config: p, communities: communities .into_iter() @@ -700,7 +702,20 @@ impl DataStore { .collect(), allowed_import, allowed_export, - }.build()); + }.build(); + let peer = match peer_result { + Ok(peer) => peer, + Err(message) => { + return Err( + err.bail( + SwitchPortSettingsGetError::InternalError( + message, + ) + ) + ); + } + }; + result.bgp_peers.push(peer); } // get the address configs @@ -729,6 +744,9 @@ impl DataStore { NameOrId::Name(name) => Error::not_found_by_name(ResourceType::SwitchPortSettings, &name), } } + SwitchPortSettingsGetError::InternalError(cause) => { + Error::internal_error(cause) + } } } else { let name = name_or_id.to_string(); @@ -1198,8 +1216,30 @@ enum SwitchPortSettingsCreateError { AddressLotNotFound, BgpConfigNotFound, ReserveBlock(ReserveBlockError), + InternalError(String), +} + +impl From for Error { + fn from(err: SwitchPortSettingsCreateError) -> Self { + match err { + SwitchPortSettingsCreateError::AddressLotNotFound => { + Error::invalid_request("AddressLot not found") + } + SwitchPortSettingsCreateError::BgpConfigNotFound => { + Error::invalid_request("BGP config not found") + } + SwitchPortSettingsCreateError::ReserveBlock( + ReserveBlockError::AddressUnavailable, + ) => Error::invalid_request("address unavailable"), + SwitchPortSettingsCreateError::ReserveBlock( + ReserveBlockError::AddressNotInLot, + ) => Error::invalid_request("address not in lot"), + SwitchPortSettingsCreateError::InternalError(cause) => { + Error::internal_error(cause) + } + } + } } -type SpsCreateError = SwitchPortSettingsCreateError; async fn do_switch_port_settings_create( conn: &Connection>, @@ -1228,7 +1268,6 @@ async fn do_switch_port_settings_create( Some(id) => SwitchPortSettings::with_id(id, ¶ms.identity), None => SwitchPortSettings::new(¶ms.identity), }; - //let port_settings = SwitchPortSettings::new(¶ms.identity); let db_port_settings: SwitchPortSettings = diesel::insert_into(port_settings_dsl::switch_port_settings) .values(port_settings.clone()) @@ -1413,11 +1452,9 @@ async fn do_switch_port_settings_create( let mut bgp_peer_config = Vec::new(); for peer_config in ¶ms.bgp_peers { for p in &peer_config.peers { - // Track peers for policy lookup. For unnumbered peers (addr is None), - // use UNSPECIFIED as the sentinel value. - let lookup_addr = - p.addr.unwrap_or(IpAddr::V4(Ipv4Addr::UNSPECIFIED)); - peer_by_addr.insert(lookup_addr, &p); + // Track peers for policy lookup. + peer_by_addr + .insert(p.addr.ip_squashing_unnumbered_to_sentinel(), &p); use nexus_db_schema::schema::bgp_config; let bgp_config_id = match &p.bgp_config { NameOrId::Id(id) => bgp_config::table @@ -1451,11 +1488,10 @@ async fn do_switch_port_settings_create( } }; - // For unnumbered peers (addr is None), use UNSPECIFIED as a sentinel - // value in the communities/import/export tables since addr is part - // of the primary key. - let db_addr: IpNetwork = - p.addr.unwrap_or(IpAddr::V4(Ipv4Addr::UNSPECIFIED)).into(); + // Convert peer addresses (which may be unnumbered) into our db + // representation (replacing unnumbered with a sentinel value). + let db_addr = + IpNetwork::from(p.addr.ip_squashing_unnumbered_to_sentinel()); if let ImportExportPolicy::Allow(list) = &p.allowed_import { let id = port_settings.identity.id; @@ -1532,33 +1568,37 @@ async fn do_switch_port_settings_create( .await?; for p in db_bgp_peers.into_iter() { - // Lookup policies and communities for peers. For unnumbered peers (addr - // is None), use UNSPECIFIED as the lookup key. - let lookup_addr = - p.addr.map(|a| a.ip()).unwrap_or(IpAddr::V4(Ipv4Addr::UNSPECIFIED)); - let (allowed_import, allowed_export, communities) = ( - peer_by_addr - .get(&lookup_addr) - .map(|x| x.allowed_import.clone()) - .unwrap_or(ImportExportPolicy::NoFiltering), - peer_by_addr - .get(&lookup_addr) - .map(|x| x.allowed_export.clone()) - .unwrap_or(ImportExportPolicy::NoFiltering), - peer_by_addr - .get(&lookup_addr) - .map(|x| x.communities.clone()) - .unwrap_or(Vec::new()), - ); - result.bgp_peers.push( - BgpPeerFromDbBuilder { - peer_config: &p, - communities, - allowed_import, - allowed_export, + // Lookup policies and communities for peers. `peer_by_addr` is keyed by + // a non-optional `IpAddr`, where unnumbered peers are mapped to a + // sentinel value. + let lookup_addr = p + .addr + .map(|a| a.ip()) + .unwrap_or(RouterPeerType::UNNUMBERED_SENTINEL); + let Some(peer) = peer_by_addr.get(&lookup_addr) else { + return Err(err.bail( + SwitchPortSettingsCreateError::InternalError(format!( + "unexpectedly missing peer {} (addr: {:?})", + p.bgp_config_id, p.addr, + )), + )); + }; + let peer_result = BgpPeerFromDbBuilder { + peer_config: &p, + communities: peer.communities.clone(), + allowed_import: peer.allowed_import.clone(), + allowed_export: peer.allowed_export.clone(), + } + .build(); + let peer = match peer_result { + Ok(peer) => peer, + Err(message) => { + return Err(err.bail( + SwitchPortSettingsCreateError::InternalError(message), + )); } - .build(), - ); + }; + result.bgp_peers.push(peer); } let mut address_config = Vec::new(); @@ -1872,6 +1912,8 @@ mod test { }; use omicron_test_utils::dev; use sled_agent_types::early_networking::ImportExportPolicy; + use sled_agent_types::early_networking::RouterLifetimeConfig; + use sled_agent_types::early_networking::RouterPeerType; use sled_agent_types::early_networking::SwitchSlot; use std::{collections::HashMap, str::FromStr}; use uuid::Uuid; @@ -1934,31 +1976,57 @@ mod test { bgp_peers: vec![BgpPeerConfig { link_name: Name::from_str("phy0") .expect("phy0 should be a valid link name"), - peers: vec![BgpPeer { - bgp_config: NameOrId::Name( - "test-bgp-config".parse().unwrap(), - ), - interface_name: "qsfp0".parse().unwrap(), - addr: Some( - "192.168.1.1".parse::().unwrap(), - ), - hold_time: 0, - idle_hold_time: 0, - delay_open: 0, - connect_retry: 0, - keepalive: 0, - remote_asn: None, - min_ttl: None, - md5_auth_key: None, - multi_exit_discriminator: None, - communities: Vec::new(), - local_pref: None, - enforce_first_as: false, - allowed_export: ImportExportPolicy::NoFiltering, - allowed_import: ImportExportPolicy::NoFiltering, - vlan_id: None, - router_lifetime: 0, - }], + peers: vec![ + BgpPeer { + bgp_config: bgp_config.identity.name.clone().into(), + addr: RouterPeerType::Numbered { + ip: "192.168.1.1".parse().unwrap(), + }, + hold_time: 0, + idle_hold_time: 0, + delay_open: 0, + connect_retry: 0, + keepalive: 0, + remote_asn: None, + min_ttl: None, + md5_auth_key: None, + multi_exit_discriminator: None, + communities: Vec::new(), + local_pref: None, + enforce_first_as: false, + allowed_export: ImportExportPolicy::Allow(vec![ + "fd00::/64".parse().unwrap(), + "192.168.1.0/24".parse().unwrap(), + ]), + allowed_import: ImportExportPolicy::NoFiltering, + vlan_id: None, + }, + BgpPeer { + bgp_config: bgp_config.identity.name.clone().into(), + addr: RouterPeerType::Unnumbered { + router_lifetime: RouterLifetimeConfig::new(123) + .unwrap(), + }, + hold_time: 0, + idle_hold_time: 0, + delay_open: 0, + connect_retry: 0, + keepalive: 0, + remote_asn: None, + min_ttl: None, + md5_auth_key: None, + multi_exit_discriminator: None, + communities: Vec::new(), + local_pref: None, + enforce_first_as: false, + allowed_export: ImportExportPolicy::NoFiltering, + allowed_import: ImportExportPolicy::Allow(vec![ + "fd00:1234::/64".parse().unwrap(), + "192.168.3.0/24".parse().unwrap(), + ]), + vlan_id: None, + }, + ], }], addresses: vec![], }; @@ -2143,19 +2211,26 @@ mod test { } } - assert_eq!(settings.bgp_peers.len(), db_settings.bgp_peers.len()); - let mut db_peers = HashMap::new(); + assert_eq!( + settings.bgp_peers[0].peers.len(), + db_settings.bgp_peers.len() + ); - for peer in db_settings.bgp_peers { - db_peers.insert(peer.inner.interface_name.clone(), peer); - } + // Build a map of peer address -> peer so we can pair the db peers up + // with our settings in the loop below. + let db_peers = db_settings + .bgp_peers + .into_iter() + .map(|peer| (peer.inner.addr, peer)) + .collect::>(); for config in settings.bgp_peers { - let db_peer = db_peers - .get(&config.link_name) - .expect("requested peer should be present"); - for mut peer in config.peers { + let mut db_peer = db_peers + .get(&peer.addr) + .expect("requested peer should be present") + .clone(); + match peer.bgp_config { NameOrId::Id(id) => { assert_eq!(db_peer.bgp_config_id, id); @@ -2182,13 +2257,25 @@ mod test { // `db_peer.inner.bgp_config` is always populated with an ID. peer.bgp_config = NameOrId::Id(db_peer.bgp_config_id); - // TODO-correctness We don't faithfully persist - // `interface_name`, and should probably remove the field - // entirely - // (https://github.com/oxidecomputer/omicron/issues/10104). - // For now, manually set the field to match so we can assert_eq - // over the entire struct below. - peer.interface_name = db_peer.inner.interface_name.clone(); + // TODO-correctness `ImportExportPolicy::Allow(_)` should + // probably use `BTreeSet<_>` instead of `Vec<_>`; for now, sort + // both so our comparison below doesn't fail spuriously due to + // "same contents, different order". + // + // https://github.com/oxidecomputer/omicron/issues/10121 + for policy in [ + &mut peer.allowed_import, + &mut peer.allowed_export, + &mut db_peer.inner.allowed_import, + &mut db_peer.inner.allowed_export, + ] { + match policy { + ImportExportPolicy::NoFiltering => (), + ImportExportPolicy::Allow(ip_nets) => { + ip_nets.sort_unstable(); + } + } + } assert_eq!(peer, db_peer.inner); } diff --git a/nexus/external-api/Cargo.toml b/nexus/external-api/Cargo.toml index 5d9a6cbc757..f70f1307584 100644 --- a/nexus/external-api/Cargo.toml +++ b/nexus/external-api/Cargo.toml @@ -31,6 +31,7 @@ schemars.workspace = true scim2-rs.workspace = true serde.workspace = true serde_json.workspace = true +slog-error-chain.workspace = true trust-quorum-types.workspace = true tufaceous-artifact.workspace = true uuid.workspace = true diff --git a/nexus/external-api/src/lib.rs b/nexus/external-api/src/lib.rs index e6416f360df..9c4cf6004d4 100644 --- a/nexus/external-api/src/lib.rs +++ b/nexus/external-api/src/lib.rs @@ -32,6 +32,7 @@ use nexus_types_versions::v2026_01_16_00; use nexus_types_versions::v2026_01_16_01; use nexus_types_versions::v2026_01_22_00; use nexus_types_versions::v2026_01_30_01; +use nexus_types_versions::v2026_02_13_01; use omicron_common::address::IpRange; use omicron_common::api::external::{ http_pagination::{ @@ -41,6 +42,7 @@ use omicron_common::api::external::{ *, }; use openapiv3::OpenAPI; +use slog_error_chain::InlineErrorChain; /// Types that convert to/from `omicron-common` types and thus cannot live in /// `nexus-types-versions`. These will go away once `omicron-common-versions` @@ -79,6 +81,7 @@ api_versions!([ // | date-based version should be at the top of the list. // v // (next_yyyy_mm_dd_nn, IDENT), + (2026_04_16_00, STRONGER_BGP_UNNUMBERED_TYPES), (2026_03_25_00, SUBNET_POOL_UTILIZATION_REMAINING), (2026_03_24_00, ADD_ICMPV6_FIREWALL_SUPPORT), (2026_03_23_00, RENAME_PREFIX_LEN), @@ -4577,7 +4580,7 @@ pub trait NexusExternalApi { method = POST, path = "/v1/system/networking/switch-port-settings", tags = ["system/networking"], - versions = VERSION_BGP_UNNUMBERED_PEERS.., + versions = VERSION_STRONGER_BGP_UNNUMBERED_TYPES.., }] async fn networking_switch_port_settings_create( rqctx: RequestContext, @@ -4587,6 +4590,35 @@ pub trait NexusExternalApi { HttpError, >; + #[endpoint { + operation_id = "networking_switch_port_settings_create", + method = POST, + path = "/v1/system/networking/switch-port-settings", + tags = ["system/networking"], + versions = VERSION_BGP_UNNUMBERED_PEERS..VERSION_STRONGER_BGP_UNNUMBERED_TYPES, + }] + async fn networking_switch_port_settings_create_v2026_02_13_01( + rqctx: RequestContext, + new_settings: TypedBody< + v2026_02_13_01::networking::SwitchPortSettingsCreate, + >, + ) -> Result< + HttpResponseCreated, + HttpError, + > { + Self::networking_switch_port_settings_create( + rqctx, + new_settings.try_map(TryFrom::try_from).map_err(|err| { + HttpError::for_bad_request( + None, + InlineErrorChain::new(&err).to_string(), + ) + })?, + ) + .await + .map(|response| response.map(From::from)) + } + /// Create switch port settings (old version with required BgpPeer.addr) #[endpoint { operation_id = "networking_switch_port_settings_create", @@ -4604,7 +4636,7 @@ pub trait NexusExternalApi { HttpResponseCreated, HttpError, > { - match Self::networking_switch_port_settings_create( + match Self::networking_switch_port_settings_create_v2026_02_13_01( rqctx, new_settings.map(Into::into), ) @@ -4649,13 +4681,32 @@ pub trait NexusExternalApi { method = GET, path = "/v1/system/networking/switch-port-settings/{port}", tags = ["system/networking"], - versions = VERSION_BGP_UNNUMBERED_PEERS.., + versions = VERSION_STRONGER_BGP_UNNUMBERED_TYPES.., }] async fn networking_switch_port_settings_view( rqctx: RequestContext, path_params: Path, ) -> Result, HttpError>; + #[endpoint { + operation_id = "networking_switch_port_settings_view", + method = GET, + path = "/v1/system/networking/switch-port-settings/{port}", + tags = ["system/networking"], + versions = VERSION_BGP_UNNUMBERED_PEERS..VERSION_STRONGER_BGP_UNNUMBERED_TYPES, + }] + async fn networking_switch_port_settings_view_v2026_02_13_01( + rqctx: RequestContext, + path_params: Path, + ) -> Result< + HttpResponseOk, + HttpError, + > { + Self::networking_switch_port_settings_view(rqctx, path_params) + .await + .map(|response| response.map(From::from)) + } + /// Get information about switch port (old version with required BgpPeer.addr) #[endpoint { operation_id = "networking_switch_port_settings_view", @@ -4671,8 +4722,11 @@ pub trait NexusExternalApi { HttpResponseOk, HttpError, > { - match Self::networking_switch_port_settings_view(rqctx, path_params) - .await + match Self::networking_switch_port_settings_view_v2026_02_13_01( + rqctx, + path_params, + ) + .await { Ok(HttpResponseOk(result)) => { Ok(HttpResponseOk(result.try_into()?)) diff --git a/nexus/src/app/background/tasks/sync_switch_configuration.rs b/nexus/src/app/background/tasks/sync_switch_configuration.rs index cfcabc0a04d..8b55c17fe76 100644 --- a/nexus/src/app/background/tasks/sync_switch_configuration.rs +++ b/nexus/src/app/background/tasks/sync_switch_configuration.rs @@ -47,7 +47,7 @@ use nexus_types::identity::{Asset, Resource}; use omicron_common::OMICRON_DPD_TAG; use omicron_common::{ address::{Ipv6Subnet, get_sled_address}, - api::external::DataPageParams, + api::external::{DataPageParams, Name}, }; use rdb_types::{Prefix, Prefix4, Prefix6}; use serde_json::json; @@ -77,11 +77,12 @@ use std::{ hash::Hash, net::{IpAddr, Ipv4Addr, Ipv6Addr}, str::FromStr, - sync::Arc, + sync::{Arc, LazyLock}, }; const DPD_TAG: Option<&'static str> = Some(OMICRON_DPD_TAG); -const PHY0: &str = "phy0"; +static PHY0: LazyLock = + LazyLock::new(|| "phy0".parse().expect("phy0 is a valid Name")); // This is more of an implementation detail of the BGP implementation. It // defines the maximum time the peering engine will wait for external messages @@ -539,6 +540,7 @@ impl BackgroundTask for SwitchPortSettingsManager { for peer in &settings.bgp_peers { let bgp_config_id = peer.bgp_config_id(); let port_settings_id = peer.port_settings_id(); + let interface_name = peer.interface_name(); let peer = peer.as_bgp_peer(); // since we only have one bgp config per switch, we only need to fetch it once @@ -634,22 +636,12 @@ impl BackgroundTask for SwitchPortSettingsManager { bgp_announce_prefixes.insert(bgp_config.bgp_announce_set_id, prefixes); } - let ttl = peer.min_ttl; - - // Determine if this is a numbered or unnumbered peer - // (None or unspecified address = unnumbered) - let peer_addr = match peer.addr { - Some(addr) if !addr.is_unspecified() => Some(addr), - _ => None, - }; - - // Numbered peer - identified by address //TODO consider awaiting in parallel and joining let communities = match self.datastore.communities_for_peer( opctx, port_settings_id, - &peer.interface_name.to_string(), - peer_addr, + interface_name, + peer.addr, ).await { Ok(cs) => cs, Err(e) => { @@ -672,8 +664,8 @@ impl BackgroundTask for SwitchPortSettingsManager { let allow_import = match self.datastore.allow_import_for_peer( opctx, port_settings_id, - &peer.interface_name.to_string(), - peer_addr, + interface_name, + peer.addr, ).await { Ok(cs) => cs, Err(e) => { @@ -740,8 +732,8 @@ impl BackgroundTask for SwitchPortSettingsManager { let allow_export = match self.datastore.allow_export_for_peer( opctx, port_settings_id, - &peer.interface_name.to_string(), - peer_addr, + interface_name, + peer.addr, ).await { Ok(cs) => cs, Err(e) => { @@ -805,106 +797,105 @@ impl BackgroundTask for SwitchPortSettingsManager { None => MgImportExportPolicy6::NoFiltering, }; - // numbered peer - if let Some(addr) = peer_addr { - // now that the peer passes the above validations, add it to the list for configuration - let peer_config = BgpPeerConfig { - name: format!("{}", addr), - host: format!("{}:179", addr), - hold_time: peer.hold_time.into(), - idle_hold_time: peer.idle_hold_time.into(), - delay_open: peer.delay_open.into(), - connect_retry: peer.connect_retry.into(), - keepalive: peer.keepalive.into(), - resolution: BGP_SESSION_RESOLUTION, - passive: false, - remote_asn: peer.remote_asn, - min_ttl: ttl, - md5_auth_key: peer.md5_auth_key.clone(), - multi_exit_discriminator: peer.multi_exit_discriminator, - local_pref: peer.local_pref, - enforce_first_as: peer.enforce_first_as, - communities: communities.into_iter().map(|c| c.community.0).collect(), - ipv4_unicast: Some(Ipv4UnicastConfig{ - nexthop: None, - import_policy: import_policy4, - export_policy: export_policy4, - }), - ipv6_unicast: Some(Ipv6UnicastConfig{ - nexthop: None, - import_policy: import_policy6, - export_policy: export_policy6, - }), - vlan_id: peer.vlan_id, - //TODO plumb these out to the external API - connect_retry_jitter: Some(JitterRange { - max: 1.0, - min: 0.75, - }), - deterministic_collision_resolution: false, - idle_hold_jitter: None, - }; + match peer.addr { + // Numbered peer - identified by address + RouterPeerType::Numbered { ip } => { + // now that the peer passes the above validations, add it to the list for configuration + let peer_config = BgpPeerConfig { + name: format!("{ip}"), + host: format!("{ip}:179"), + hold_time: peer.hold_time.into(), + idle_hold_time: peer.idle_hold_time.into(), + delay_open: peer.delay_open.into(), + connect_retry: peer.connect_retry.into(), + keepalive: peer.keepalive.into(), + resolution: BGP_SESSION_RESOLUTION, + passive: false, + remote_asn: peer.remote_asn, + min_ttl: peer.min_ttl, + md5_auth_key: peer.md5_auth_key.clone(), + multi_exit_discriminator: peer.multi_exit_discriminator, + local_pref: peer.local_pref, + enforce_first_as: peer.enforce_first_as, + communities: communities.into_iter().map(|c| c.community.0).collect(), + ipv4_unicast: Some(Ipv4UnicastConfig{ + nexthop: None, + import_policy: import_policy4, + export_policy: export_policy4, + }), + ipv6_unicast: Some(Ipv6UnicastConfig{ + nexthop: None, + import_policy: import_policy6, + export_policy: export_policy6, + }), + vlan_id: peer.vlan_id, + //TODO plumb these out to the external API + connect_retry_jitter: Some(JitterRange { + max: 1.0, + min: 0.75, + }), + deterministic_collision_resolution: false, + idle_hold_jitter: None, + }; - // update the stored vec if it exists, create a new on if it doesn't exist - match peers.entry(port.port_name.clone().to_string()) { - Entry::Occupied(mut occupied_entry) => { - occupied_entry.get_mut().push(peer_config); - }, - Entry::Vacant(vacant_entry) => { - vacant_entry.insert(vec![peer_config]); - }, + // update the stored vec if it exists, create a new on if it doesn't exist + match peers.entry(port.port_name.clone().to_string()) { + Entry::Occupied(mut occupied_entry) => { + occupied_entry.get_mut().push(peer_config); + }, + Entry::Vacant(vacant_entry) => { + vacant_entry.insert(vec![peer_config]); + }, + } } - } - // unnumbered peer - else { // Unnumbered peer - identified by interface - // For unnumbered peers, we use NoFiltering policies as the - // communities/import/export tables are keyed by address - let peer_config = UnnumberedBgpPeerConfig { - name: format!("unnumbered-{}", port.port_name), - interface: format!("tfport{}_0", port.port_name), - hold_time: peer.hold_time.into(), - idle_hold_time: peer.idle_hold_time.into(), - delay_open: peer.delay_open.into(), - connect_retry: peer.connect_retry.into(), - keepalive: peer.keepalive.into(), - resolution: BGP_SESSION_RESOLUTION, - passive: false, - remote_asn: peer.remote_asn, - min_ttl: ttl, - md5_auth_key: peer.md5_auth_key.clone(), - multi_exit_discriminator: peer.multi_exit_discriminator, - local_pref: peer.local_pref, - enforce_first_as: peer.enforce_first_as, - communities: communities.into_iter().map(|c| c.community.0).collect(), - ipv4_unicast: Some(Ipv4UnicastConfig{ - nexthop: None, - import_policy: import_policy4, - export_policy: export_policy4, - }), - ipv6_unicast: Some(Ipv6UnicastConfig{ - nexthop: None, - import_policy: import_policy6, - export_policy: export_policy6, - }), - vlan_id: peer.vlan_id, - connect_retry_jitter: Some(JitterRange { - max: 1.0, - min: 0.75, - }), - deterministic_collision_resolution: false, - idle_hold_jitter: None, - router_lifetime: peer.router_lifetime, - }; + RouterPeerType::Unnumbered { router_lifetime } => { + let peer_config = UnnumberedBgpPeerConfig { + name: format!("unnumbered-{}", port.port_name), + interface: format!("tfport{}_0", port.port_name), + hold_time: peer.hold_time.into(), + idle_hold_time: peer.idle_hold_time.into(), + delay_open: peer.delay_open.into(), + connect_retry: peer.connect_retry.into(), + keepalive: peer.keepalive.into(), + resolution: BGP_SESSION_RESOLUTION, + passive: false, + remote_asn: peer.remote_asn, + min_ttl: peer.min_ttl, + md5_auth_key: peer.md5_auth_key.clone(), + multi_exit_discriminator: peer.multi_exit_discriminator, + local_pref: peer.local_pref, + enforce_first_as: peer.enforce_first_as, + communities: communities.into_iter().map(|c| c.community.0).collect(), + ipv4_unicast: Some(Ipv4UnicastConfig{ + nexthop: None, + import_policy: import_policy4, + export_policy: export_policy4, + }), + ipv6_unicast: Some(Ipv6UnicastConfig{ + nexthop: None, + import_policy: import_policy6, + export_policy: export_policy6, + }), + vlan_id: peer.vlan_id, + connect_retry_jitter: Some(JitterRange { + max: 1.0, + min: 0.75, + }), + deterministic_collision_resolution: false, + idle_hold_jitter: None, + router_lifetime: router_lifetime.as_u16(), + }; - // update the stored vec if it exists, create a new on if it doesn't exist - match unnumbered_peers.entry(port.port_name.clone().to_string()) { - Entry::Occupied(mut occupied_entry) => { - occupied_entry.get_mut().push(peer_config); - }, - Entry::Vacant(vacant_entry) => { - vacant_entry.insert(vec![peer_config]); - }, + // update the stored vec if it exists, create a new on if it doesn't exist + match unnumbered_peers.entry(port.port_name.clone().to_string()) { + Entry::Occupied(mut occupied_entry) => { + occupied_entry.get_mut().push(peer_config); + }, + Entry::Vacant(vacant_entry) => { + vacant_entry.insert(vec![peer_config]); + }, + } } } } @@ -1192,24 +1183,13 @@ impl BackgroundTask for SwitchPortSettingsManager { ; for peer in port_config.bgp_peers.iter_mut() { - // For unnumbered peers, pass None - // - // TODO-cleanup Push `RouterPeerAddress` down to all the - // datastore methods below instead of an `Option`. - let peer_addr_for_lookup = match peer.addr { - RouterPeerType::Unnumbered { .. } => None, - RouterPeerType::Numbered { ip } => { - Some(IpAddr::from(ip)) - } - }; - peer.communities = match self .datastore .communities_for_peer( opctx, port.port_settings_id.unwrap(), - PHY0, //TODO https://github.com/oxidecomputer/omicron/issues/3062 - peer_addr_for_lookup, + &PHY0, //TODO https://github.com/oxidecomputer/omicron/issues/3062 + peer.addr, ).await { Ok(cs) => cs.iter().map(|c| c.community.0).collect(), Err(e) => { @@ -1226,8 +1206,8 @@ impl BackgroundTask for SwitchPortSettingsManager { let allow_import = match self.datastore.allow_import_for_peer( opctx, port.port_settings_id.unwrap(), - PHY0, //TODO https://github.com/oxidecomputer/omicron/issues/3062 - peer_addr_for_lookup, + &PHY0, //TODO https://github.com/oxidecomputer/omicron/issues/3062 + peer.addr, ).await { Ok(cs) => cs, Err(e) => { @@ -1250,8 +1230,8 @@ impl BackgroundTask for SwitchPortSettingsManager { let allow_export = match self.datastore.allow_export_for_peer( opctx, port.port_settings_id.unwrap(), - PHY0, //TODO https://github.com/oxidecomputer/omicron/issues/3062 - peer_addr_for_lookup, + &PHY0, //TODO https://github.com/oxidecomputer/omicron/issues/3062 + peer.addr, ).await { Ok(cs) => cs, Err(e) => { diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index a1436989bb9..7a2b2c8b72f 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -46,8 +46,6 @@ use sled_agent_client::types::AddSledRequest; use sled_agent_client::types::StartSledAgentRequest; use sled_agent_client::types::StartSledAgentRequestBody; use sled_agent_types::early_networking::LldpAdminStatus; -use sled_agent_types::early_networking::RouterLifetimeConfig; -use sled_agent_types::early_networking::RouterPeerType; use sled_hardware_types::BaseboardId; use slog_error_chain::InlineErrorChain; @@ -588,44 +586,26 @@ impl super::Nexus { let peers: Vec = uplink_config .bgp_peers .iter() - .map(|r| { - // TODO-cleanup Extend stronger types out to the external - // API (omicron#9832). - // - // For now, squash unnumbered back to None, and fill in a - // default router_lifetime for numbered. - let (addr, router_lifetime) = match r.addr { - RouterPeerType::Unnumbered { router_lifetime } => { - (None, router_lifetime.as_u16()) - } - RouterPeerType::Numbered { ip } => ( - Some(ip.into()), - RouterLifetimeConfig::default().as_u16(), - ), - }; - networking::BgpPeer { - bgp_config: NameOrId::Name( - format!("as{}", r.asn).parse().unwrap(), - ), - interface_name: link_name.clone(), - addr, - hold_time: r.hold_time() as u32, - idle_hold_time: r.idle_hold_time() as u32, - delay_open: r.delay_open() as u32, - connect_retry: r.connect_retry() as u32, - keepalive: r.keepalive() as u32, - remote_asn: r.remote_asn, - min_ttl: r.min_ttl, - md5_auth_key: r.md5_auth_key.clone(), - multi_exit_discriminator: r.multi_exit_discriminator, - local_pref: r.local_pref, - enforce_first_as: r.enforce_first_as, - communities: r.communities.clone(), - allowed_import: r.allowed_import.clone(), - allowed_export: r.allowed_export.clone(), - vlan_id: r.vlan_id, - router_lifetime, - } + .map(|r| networking::BgpPeer { + bgp_config: NameOrId::Name( + format!("as{}", r.asn).parse().unwrap(), + ), + addr: r.addr, + hold_time: r.hold_time() as u32, + idle_hold_time: r.idle_hold_time() as u32, + delay_open: r.delay_open() as u32, + connect_retry: r.connect_retry() as u32, + keepalive: r.keepalive() as u32, + remote_asn: r.remote_asn, + min_ttl: r.min_ttl, + md5_auth_key: r.md5_auth_key.clone(), + multi_exit_discriminator: r.multi_exit_discriminator, + local_pref: r.local_pref, + enforce_first_as: r.enforce_first_as, + communities: r.communities.clone(), + allowed_import: r.allowed_import.clone(), + allowed_export: r.allowed_export.clone(), + vlan_id: r.vlan_id, }) .collect(); diff --git a/nexus/src/app/switch_port.rs b/nexus/src/app/switch_port.rs index 80325fd8738..0d0e9338fcb 100644 --- a/nexus/src/app/switch_port.rs +++ b/nexus/src/app/switch_port.rs @@ -19,6 +19,7 @@ use omicron_common::api::external::{ self, CreateResult, DataPageParams, DeleteResult, Error, ListResultVec, LookupResult, Name, NameOrId, UpdateResult, }; +use sled_agent_types::early_networking::RouterPeerType; use sled_agent_types::early_networking::SwitchSlot; use std::sync::Arc; use uuid::Uuid; @@ -63,9 +64,11 @@ impl super::Nexus { for p in x.peers.iter() { if let Some(ref key) = p.md5_auth_key { let peer_id = match p.addr { - Some(addr) => format!("peer {}", addr), - None => { - format!("unnumbered peer on {}", p.interface_name) + RouterPeerType::Numbered { ip } => { + format!("peer {ip}") + } + RouterPeerType::Unnumbered { .. } => { + format!("unnumbered peer {}", p.bgp_config) } }; if key.len() > 80 { diff --git a/nexus/tests/integration_tests/switch_port.rs b/nexus/tests/integration_tests/switch_port.rs index 37ae94b56fa..87493d84c0f 100644 --- a/nexus/tests/integration_tests/switch_port.rs +++ b/nexus/tests/integration_tests/switch_port.rs @@ -4,8 +4,6 @@ //! Integration tests for operating on Ports -use std::str::FromStr; - use http::StatusCode; use http::method::Method; use nexus_test_utils::http_testing::{AuthnMode, NexusRequest, RequestBuilder}; @@ -24,8 +22,10 @@ use omicron_common::api::external::{ NameOrId, }; use oxnet::IpNet; -use sled_agent_types::early_networking::ImportExportPolicy; -use sled_agent_types::early_networking::SwitchSlot; +use sled_agent_types::early_networking::{ImportExportPolicy, RouterPeerType}; +use sled_agent_types::early_networking::{RouterLifetimeConfig, SwitchSlot}; +use std::net::IpAddr; +use std::str::FromStr; type ControlPlaneTestContext = nexus_test_utils::ControlPlaneTestContext; @@ -315,8 +315,9 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { // Numbered peer - identified by address BgpPeer { bgp_config: NameOrId::Name("as47".parse().unwrap()), - interface_name: "phy0".parse().unwrap(), - addr: Some("1.2.3.4".parse().unwrap()), + addr: RouterPeerType::Numbered { + ip: "1.2.3.4".parse().unwrap(), + }, hold_time: 6, idle_hold_time: 6, delay_open: 0, @@ -332,13 +333,13 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { allowed_export: ImportExportPolicy::NoFiltering, allowed_import: ImportExportPolicy::NoFiltering, vlan_id: None, - router_lifetime: 0, }, - // Unnumbered peer - identified by interface only (addr is None) + // Unnumbered peer - identified by link from parent `BgpPeerConfig` BgpPeer { bgp_config: NameOrId::Name("as47".parse().unwrap()), - interface_name: "phy0".parse().unwrap(), - addr: None, + addr: RouterPeerType::Unnumbered { + router_lifetime: RouterLifetimeConfig::new(123).unwrap(), + }, hold_time: 6, idle_hold_time: 6, delay_open: 0, @@ -358,7 +359,6 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { "192.168.0.0/16".parse().unwrap(), ]), vlan_id: None, - router_lifetime: 0, }, ], }); @@ -381,11 +381,11 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { let numbered_peer = created .bgp_peers .iter() - .find(|p| p.addr.is_some()) + .find_map(|p| p.addr.ip_squashing_unnumbered_to_none()) .expect("Should have a numbered peer"); assert_eq!( - numbered_peer.addr, - Some("1.2.3.4".parse().unwrap()), + numbered_peer, + "1.2.3.4".parse::().unwrap(), "Numbered peer should have addr 1.2.3.4" ); @@ -393,11 +393,13 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { let unnumbered_peer = created .bgp_peers .iter() - .find(|p| p.addr.is_none()) + .find(|p| p.addr.is_unnumbered()) .expect("Should have an unnumbered peer"); assert_eq!( - unnumbered_peer.addr, None, - "Unnumbered peer should have no addr" + unnumbered_peer.addr, + RouterPeerType::Unnumbered { + router_lifetime: RouterLifetimeConfig::new(123).unwrap(), + } ); assert_eq!( unnumbered_peer.remote_asn, @@ -451,8 +453,14 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { let roundtrip_unnumbered = roundtrip .bgp_peers .iter() - .find(|p| p.addr.is_none()) + .find(|p| p.addr.is_unnumbered()) .expect("Roundtrip should have an unnumbered peer"); + assert_eq!( + roundtrip_unnumbered.addr, + RouterPeerType::Unnumbered { + router_lifetime: RouterLifetimeConfig::new(123).unwrap(), + } + ); assert_eq!(roundtrip_unnumbered.remote_asn, Some(65000)); assert_eq!(roundtrip_unnumbered.communities, vec![65000]); diff --git a/nexus/types/versions/Cargo.toml b/nexus/types/versions/Cargo.toml index 31ddc9f0ae3..cad0b3b4ed4 100644 --- a/nexus/types/versions/Cargo.toml +++ b/nexus/types/versions/Cargo.toml @@ -33,6 +33,7 @@ serde.workspace = true serde_json.workspace = true slog-error-chain.workspace = true strum.workspace = true +thiserror.workspace = true tough.workspace = true tufaceous-artifact.workspace = true url.workspace = true diff --git a/nexus/types/versions/src/bgp_unnumbered_peers/networking.rs b/nexus/types/versions/src/bgp_unnumbered_peers/networking.rs index 1f14770cb5a..f0adfcced10 100644 --- a/nexus/types/versions/src/bgp_unnumbered_peers/networking.rs +++ b/nexus/types/versions/src/bgp_unnumbered_peers/networking.rs @@ -396,25 +396,6 @@ pub struct SwitchPortSettingsCreate { pub addresses: Vec, } -impl SwitchPortSettingsCreate { - pub fn new(identity: IdentityMetadataCreateParams) -> Self { - Self { - identity, - port_config: - crate::v2025_11_20_00::networking::SwitchPortConfigCreate { - geometry: - crate::v2025_11_20_00::networking::SwitchPortGeometry::Qsfp28x1, - }, - groups: Vec::new(), - links: Vec::new(), - interfaces: Vec::new(), - routes: Vec::new(), - bgp_peers: Vec::new(), - addresses: Vec::new(), - } - } -} - impl From for SwitchPortSettingsCreate { diff --git a/nexus/types/versions/src/impls/networking.rs b/nexus/types/versions/src/impls/networking.rs index 5a9c86fd381..0831b35c920 100644 --- a/nexus/types/versions/src/impls/networking.rs +++ b/nexus/types/versions/src/impls/networking.rs @@ -3,6 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use crate::latest; +use omicron_common::api::external::IdentityMetadataCreateParams; use oxnet::IpNet; impl From for latest::networking::AddressLotBlockCreate { @@ -51,3 +52,20 @@ impl latest::networking::AggregateBgpMessageHistory { Self { switch_histories } } } + +impl latest::networking::SwitchPortSettingsCreate { + pub fn new(identity: IdentityMetadataCreateParams) -> Self { + Self { + identity, + port_config: latest::networking::SwitchPortConfigCreate { + geometry: latest::networking::SwitchPortGeometry::Qsfp28x1, + }, + groups: Vec::new(), + links: Vec::new(), + interfaces: Vec::new(), + routes: Vec::new(), + bgp_peers: Vec::new(), + addresses: Vec::new(), + } + } +} diff --git a/nexus/types/versions/src/latest.rs b/nexus/types/versions/src/latest.rs index 7aab6ceaa2f..b38148305cd 100644 --- a/nexus/types/versions/src/latest.rs +++ b/nexus/types/versions/src/latest.rs @@ -281,11 +281,7 @@ pub mod networking { pub use crate::v2026_02_13_01::networking::BgpConfigCreate; pub use crate::v2026_02_13_01::networking::BgpExported; pub use crate::v2026_02_13_01::networking::BgpImported; - pub use crate::v2026_02_13_01::networking::BgpPeer; - pub use crate::v2026_02_13_01::networking::BgpPeerConfig; pub use crate::v2026_02_13_01::networking::BgpPeerStatus; - pub use crate::v2026_02_13_01::networking::SwitchPortSettings; - pub use crate::v2026_02_13_01::networking::SwitchPortSettingsCreate; pub use crate::v2026_03_06_01::networking::BfdSessionDisable; pub use crate::v2026_03_06_01::networking::BfdSessionEnable; @@ -295,6 +291,13 @@ pub mod networking { pub use crate::v2026_03_06_01::networking::LoopbackAddressPath; pub use crate::v2026_03_06_01::networking::SwitchPort; pub use crate::v2026_03_06_01::networking::SwitchPortSelector; + + pub use crate::v2026_04_16_00::networking::BgpPeer; + pub use crate::v2026_04_16_00::networking::BgpPeerConfig; + pub use crate::v2026_04_16_00::networking::BgpPeerConversionError; + pub use crate::v2026_04_16_00::networking::SwitchPortSettings; + pub use crate::v2026_04_16_00::networking::SwitchPortSettingsCreate; + pub use crate::v2026_04_16_00::networking::router_peer_type_try_from_old_representation; } pub mod oxql { diff --git a/nexus/types/versions/src/lib.rs b/nexus/types/versions/src/lib.rs index ae02c281c59..c16bcd11709 100644 --- a/nexus/types/versions/src/lib.rs +++ b/nexus/types/versions/src/lib.rs @@ -77,3 +77,5 @@ pub mod v2026_03_14_00; pub mod v2026_03_23_00; #[path = "subnet_pool_utilization_remaining/mod.rs"] pub mod v2026_03_25_00; +#[path = "stronger_bgp_unnumbered_types/mod.rs"] +pub mod v2026_04_16_00; diff --git a/nexus/types/versions/src/stronger_bgp_unnumbered_types/mod.rs b/nexus/types/versions/src/stronger_bgp_unnumbered_types/mod.rs new file mode 100644 index 00000000000..f4cb8cdd2c3 --- /dev/null +++ b/nexus/types/versions/src/stronger_bgp_unnumbered_types/mod.rs @@ -0,0 +1,7 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Version `STRONGER_BGP_UNNUMBERED_TYPES` of the Nexus external API. + +pub mod networking; diff --git a/nexus/types/versions/src/stronger_bgp_unnumbered_types/networking.rs b/nexus/types/versions/src/stronger_bgp_unnumbered_types/networking.rs new file mode 100644 index 00000000000..e44ea5b3b96 --- /dev/null +++ b/nexus/types/versions/src/stronger_bgp_unnumbered_types/networking.rs @@ -0,0 +1,629 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Networking types for the `STRONGER_BGP_UNNUMBERED_TYPES` version. +//! +//! * Change fields of [`BgpPeer`] to give stronger guarantees: +//! * [`BgpPeer::addr`] is now [`RouterPeerType`] instead of +//! `Option` (which permitted three distinct representations of +//! "unnumbered": `None`, `Some(0.0.0.0)`, and `Some(::)`). +//! * `BgpPeer::router_lifetime` moved from being a top-level field to being +//! nested inside the [`RouterPeerType::Unnumbered`] variant, and its type +//! is now [`RouterLifetimeConfig`] instead of `u16`, adding enforcement of +//! bounds. +//! * Remove `BgpPeer::interface_name` (omicron#10104). +//! * Define new versions of types that transitively include [`BgpPeer`]: +//! * [`BgpPeerConfig`] +//! * [`SwitchPortSettings`] +//! * [`SwitchPortSettingsCreate`] + +use crate::v2025_11_20_00::networking::{ + AddressConfig, LinkConfigCreate, RouteConfig, SwitchInterfaceConfigCreate, + SwitchPortConfigCreate, +}; +use omicron_common::api::external; +use omicron_common::api::external::IdentityMetadata; +use omicron_common::api::external::IdentityMetadataCreateParams; +use omicron_common::api::external::Name; +use omicron_common::api::external::NameOrId; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; +use sled_agent_types::early_networking::ImportExportPolicy; +use sled_agent_types::early_networking::InvalidIpAddrError; +use sled_agent_types::early_networking::RouterLifetimeConfig; +use sled_agent_types::early_networking::RouterLifetimeConfigError; +use sled_agent_types::early_networking::RouterPeerIpAddr; +use sled_agent_types::early_networking::RouterPeerIpAddrError; +use sled_agent_types::early_networking::RouterPeerType; +use std::net::IpAddr; + +/// A BGP peer configuration for an interface. Includes the set of announcements +/// that will be advertised to the peer. The `bgp_config` parameter is a +/// reference to global BGP parameters. +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] +pub struct BgpPeer { + /// The global BGP configuration used for establishing a session with this + /// peer. + pub bgp_config: NameOrId, + + /// The address of the host to peer with, or specifying the configuration of + /// an unnumbered BGP session. + pub addr: RouterPeerType, + + /// How long to hold peer connections between keepalives (seconds). + pub hold_time: u32, + + /// How long to hold a peer in idle before attempting a new session + /// (seconds). + pub idle_hold_time: u32, + + /// How long to delay sending an open request after establishing a TCP + /// session (seconds). + pub delay_open: u32, + + /// How long to wait between TCP connection retries (seconds). + pub connect_retry: u32, + + /// How often to send keepalive requests (seconds). + pub keepalive: u32, + + /// Require that a peer has a specified ASN. + pub remote_asn: Option, + + /// Require messages from a peer have a minimum IP time to live field. + pub min_ttl: Option, + + /// Use the given key for TCP-MD5 authentication with the peer. + pub md5_auth_key: Option, + + /// Apply the provided multi-exit discriminator (MED) updates sent to the peer. + pub multi_exit_discriminator: Option, + + /// Include the provided communities in updates sent to the peer. + pub communities: Vec, + + /// Apply a local preference to routes received from this peer. + pub local_pref: Option, + + /// Enforce that the first AS in paths received from this peer is the peer's AS. + pub enforce_first_as: bool, + + /// Define import policy for a peer. + pub allowed_import: ImportExportPolicy, + + /// Define export policy for a peer. + pub allowed_export: ImportExportPolicy, + + /// Associate a VLAN ID with a peer. + pub vlan_id: Option, +} + +#[derive(Debug, thiserror::Error)] +pub enum BgpPeerConversionError { + #[error(transparent)] + RouterPeerIpAddr(#[from] RouterPeerIpAddrError), + #[error(transparent)] + RouterLifetimeConfig(#[from] RouterLifetimeConfigError), +} + +/// Convert from our previous representations of BGP peer addresses: an optional +/// IP address and an unvalidated `router_lifetime`. +/// +/// Converts IPs of both `None` and `Some(UNSPECIFIED)` to +/// [`RouterPeerType::Unnumbered`], and converts other `Some(ip)` values to +/// [`RouterPeerType::Numbered`] (discarding `router_lifetime`, as it only +/// applies to unnumbered peers). Fails if given an invalid IP (e.g., a loopback +/// or multicast address) or if given an IP that maps to +/// [`RouterPeerType::Unnumbered`] and `router_lifetime` is an invalid +/// [`RouterLifetimeConfig`]. +pub fn router_peer_type_try_from_old_representation( + ip: Option, + router_lifetime: u16, +) -> Result { + match ip.map(RouterPeerIpAddr::try_from) { + // The expected cases: We either have a valid peer IP or `None` (an + // unnumbered peer). + Some(Ok(ip)) => Ok(RouterPeerType::Numbered { ip }), + None => { + let router_lifetime = RouterLifetimeConfig::new(router_lifetime)?; + Ok(RouterPeerType::Unnumbered { router_lifetime }) + } + + // Unexpected cases: If `ip` is `Some(UNSPECIFIED)`, we'll treat that as + // `unnumbered`, because `UNSPECIFIED` was previously used as the + // sentinel value for unnumbered peers in some contexts. For any other + // error case, we want to reject this conversion - the peer IP isn't + // valid. + Some(Err(err)) => match err.err { + InvalidIpAddrError::UnspecifiedAddress => { + let router_lifetime = + RouterLifetimeConfig::new(router_lifetime)?; + Ok(RouterPeerType::Unnumbered { router_lifetime }) + } + InvalidIpAddrError::LoopbackAddress + | InvalidIpAddrError::MulticastAddress + | InvalidIpAddrError::Ipv4Broadcast + | InvalidIpAddrError::Ipv6UnicastLinkLocal + | InvalidIpAddrError::Ipv4MappedIpv6 => Err(err.into()), + }, + } +} + +impl TryFrom for BgpPeer { + type Error = BgpPeerConversionError; + + fn try_from( + value: crate::v2026_02_13_01::networking::BgpPeer, + ) -> Result { + let addr = router_peer_type_try_from_old_representation( + value.addr, + value.router_lifetime, + )?; + Ok(Self { + bgp_config: value.bgp_config, + addr, + hold_time: value.hold_time, + idle_hold_time: value.idle_hold_time, + delay_open: value.delay_open, + connect_retry: value.connect_retry, + keepalive: value.keepalive, + remote_asn: value.remote_asn, + min_ttl: value.min_ttl, + md5_auth_key: value.md5_auth_key, + multi_exit_discriminator: value.multi_exit_discriminator, + communities: value.communities, + local_pref: value.local_pref, + enforce_first_as: value.enforce_first_as, + allowed_import: value.allowed_import, + allowed_export: value.allowed_export, + vlan_id: value.vlan_id, + }) + } +} + +impl From for crate::v2026_02_13_01::networking::BgpPeer { + fn from(value: BgpPeer) -> Self { + // We have to fill in some valid name for `interface_name` when + // converting back to the old BgpPeer version, but the field is gone + // because we were never actually using it. We'll use + // `deprecated-field`: this should stand out if any person looks at it + // and (hopefully!) convey that the field is no longer needed. + let interface_name = + "deprecated-field".parse().expect("constant is a valid Name"); + + let (addr, router_lifetime) = match value.addr { + RouterPeerType::Numbered { ip } => { + // The previous `BgpPeer` always contained a `router_lifetime`, + // but only used it if the peer was unnumbered. We can fill in + // any arbitrary value here - just use the default. + (Some(ip.into()), RouterLifetimeConfig::default().as_u16()) + } + RouterPeerType::Unnumbered { router_lifetime } => { + (None, router_lifetime.as_u16()) + } + }; + + Self { + bgp_config: value.bgp_config, + interface_name, + addr, + hold_time: value.hold_time, + idle_hold_time: value.idle_hold_time, + delay_open: value.delay_open, + connect_retry: value.connect_retry, + keepalive: value.keepalive, + remote_asn: value.remote_asn, + min_ttl: value.min_ttl, + md5_auth_key: value.md5_auth_key, + multi_exit_discriminator: value.multi_exit_discriminator, + communities: value.communities, + local_pref: value.local_pref, + enforce_first_as: value.enforce_first_as, + allowed_import: value.allowed_import, + allowed_export: value.allowed_export, + vlan_id: value.vlan_id, + router_lifetime, + } + } +} + +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct BgpPeerConfig { + /// Link that the peer is reachable on. + /// On ports that are not broken out, this is always phy0. + /// On a 2x breakout the options are phy0 and phy1, on 4x + /// phy0-phy3, etc. + pub link_name: Name, + + pub peers: Vec, +} + +impl TryFrom + for BgpPeerConfig +{ + type Error = BgpPeerConversionError; + + fn try_from( + value: crate::v2026_02_13_01::networking::BgpPeerConfig, + ) -> Result { + let peers = value + .peers + .into_iter() + .map(TryFrom::try_from) + .collect::>()?; + + Ok(Self { link_name: value.link_name, peers }) + } +} + +/// Parameters for creating switch port settings. Switch port settings are the +/// central data structure for setting up external networking. Switch port +/// settings include link, interface, route, address and dynamic network +/// protocol configuration. +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct SwitchPortSettingsCreate { + #[serde(flatten)] + pub identity: IdentityMetadataCreateParams, + + pub port_config: SwitchPortConfigCreate, + + #[serde(default)] + pub groups: Vec, + + /// Link configurations. + pub links: Vec, + + /// Interface configurations. + #[serde(default)] + pub interfaces: Vec, + + /// Route configurations. + #[serde(default)] + pub routes: Vec, + + /// BGP peer configurations. + #[serde(default)] + pub bgp_peers: Vec, + + /// Address configurations. + pub addresses: Vec, +} + +impl TryFrom + for SwitchPortSettingsCreate +{ + type Error = BgpPeerConversionError; + + fn try_from( + value: crate::v2026_02_13_01::networking::SwitchPortSettingsCreate, + ) -> Result { + let bgp_peers = value + .bgp_peers + .into_iter() + .map(TryFrom::try_from) + .collect::>()?; + + Ok(Self { + identity: value.identity, + port_config: value.port_config, + groups: value.groups, + links: value.links, + interfaces: value.interfaces, + routes: value.routes, + bgp_peers, + addresses: value.addresses, + }) + } +} + +/// This structure contains all port settings information in one place. It's a +/// convenience data structure for getting a complete view of a particular +/// port's settings. +// TODO: several fields below embed `external::*` types directly from +// `omicron-common`, which means their serialized shape is not truly frozen. +// Once `omicron-common-versions` exists, replace these with version-local +// copies of the types to ensure the initial version's wire format is +// immutable. +#[derive(Clone, Debug, Deserialize, JsonSchema, Serialize, PartialEq)] +pub struct SwitchPortSettings { + #[serde(flatten)] + pub identity: IdentityMetadata, + + /// Switch port settings included from other switch port settings groups. + pub groups: Vec, + + /// Layer 1 physical port settings. + pub port: external::SwitchPortConfig, + + /// Layer 2 link settings. + pub links: Vec, + + /// Layer 3 interface settings. + pub interfaces: Vec, + + /// Vlan interface settings. + pub vlan_interfaces: Vec, + + /// IP route settings. + pub routes: Vec, + + /// BGP peer settings. + pub bgp_peers: Vec, + + /// Layer 3 IP address settings. + pub addresses: Vec, +} + +impl From + for crate::v2026_02_13_01::networking::SwitchPortSettings +{ + fn from(value: SwitchPortSettings) -> Self { + Self { + identity: value.identity, + groups: value.groups, + port: value.port, + links: value.links, + interfaces: value.interfaces, + vlan_interfaces: value.vlan_interfaces, + routes: value.routes, + bgp_peers: value.bgp_peers.into_iter().map(From::from).collect(), + addresses: value.addresses, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::net::{Ipv4Addr, Ipv6Addr}; + + /// Helper to build a minimal old-format `BgpPeer` for conversion tests. + fn make_old_bgp_peer( + addr: Option, + router_lifetime: u16, + ) -> crate::v2026_02_13_01::networking::BgpPeer { + crate::v2026_02_13_01::networking::BgpPeer { + bgp_config: NameOrId::Name("test-config".parse().unwrap()), + interface_name: "phy0".parse().unwrap(), + addr, + hold_time: 6, + idle_hold_time: 6, + delay_open: 0, + connect_retry: 3, + keepalive: 2, + remote_asn: None, + min_ttl: None, + md5_auth_key: None, + multi_exit_discriminator: None, + communities: Vec::new(), + local_pref: None, + enforce_first_as: false, + allowed_import: ImportExportPolicy::NoFiltering, + allowed_export: ImportExportPolicy::NoFiltering, + vlan_id: None, + router_lifetime, + } + } + + #[test] + fn test_valid_router_peer_type_try_from_old_representation() { + let cases: Vec<(&str, Option, u16, RouterPeerType)> = vec![ + ( + "numbered IPv4", + Some("192.168.1.1".parse().unwrap()), + 0, + RouterPeerType::Numbered { ip: "192.168.1.1".parse().unwrap() }, + ), + ( + "numbered IPv6", + Some("fd00::1".parse().unwrap()), + 0, + RouterPeerType::Numbered { ip: "fd00::1".parse().unwrap() }, + ), + ( + "unnumbered via None", + None, + 300, + RouterPeerType::Unnumbered { + router_lifetime: RouterLifetimeConfig::new(300).unwrap(), + }, + ), + ( + "unnumbered via IPv4 UNSPECIFIED sentinel", + Some(IpAddr::V4(Ipv4Addr::UNSPECIFIED)), + 300, + RouterPeerType::Unnumbered { + router_lifetime: RouterLifetimeConfig::new(300).unwrap(), + }, + ), + ( + "unnumbered via IPv6 UNSPECIFIED sentinel", + Some(IpAddr::V6(Ipv6Addr::UNSPECIFIED)), + 300, + RouterPeerType::Unnumbered { + router_lifetime: RouterLifetimeConfig::new(300).unwrap(), + }, + ), + ]; + + for (label, addr, router_lifetime, expected) in cases { + let result = router_peer_type_try_from_old_representation( + addr, + router_lifetime, + ) + .unwrap_or_else(|e| panic!("{label}: unexpected error: {e:?}")); + assert_eq!(result, expected, "{label}"); + } + } + + #[test] + fn test_invalid_router_peer_type_try_from_old_representation() { + let ip_error_cases: Vec<(&str, IpAddr, InvalidIpAddrError)> = vec![ + ( + "IPv4 loopback", + IpAddr::V4(Ipv4Addr::LOCALHOST), + InvalidIpAddrError::LoopbackAddress, + ), + ( + "IPv6 loopback", + IpAddr::V6(Ipv6Addr::LOCALHOST), + InvalidIpAddrError::LoopbackAddress, + ), + ( + "multicast", + "224.0.0.1".parse().unwrap(), + InvalidIpAddrError::MulticastAddress, + ), + ( + "broadcast", + IpAddr::V4(Ipv4Addr::BROADCAST), + InvalidIpAddrError::Ipv4Broadcast, + ), + ( + "IPv6 link-local", + "fe80::1".parse().unwrap(), + InvalidIpAddrError::Ipv6UnicastLinkLocal, + ), + ( + "IPv4-mapped IPv6", + "::ffff:192.168.0.1".parse().unwrap(), + InvalidIpAddrError::Ipv4MappedIpv6, + ), + ]; + + for (label, ip, expected_err) in ip_error_cases { + let err = router_peer_type_try_from_old_representation(Some(ip), 0) + .expect_err(&format!("{label}: should have failed")); + match err { + BgpPeerConversionError::RouterPeerIpAddr( + RouterPeerIpAddrError { err, .. }, + ) => assert_eq!( + std::mem::discriminant(&err), + std::mem::discriminant(&expected_err), + "{label}: wrong error variant: {err:?}" + ), + other => panic!("{label}: wrong error type: {other:?}"), + } + } + + // router_lifetime too large — via None and via UNSPECIFIED sentinel + for (label, addr) in [ + ("None", None), + ("UNSPECIFIED", Some(IpAddr::V4(Ipv4Addr::UNSPECIFIED))), + ] { + let err = router_peer_type_try_from_old_representation(addr, 9001) + .expect_err(&format!( + "router_lifetime 9001 with {label} addr should fail" + )); + assert!( + matches!(err, BgpPeerConversionError::RouterLifetimeConfig(_)), + "{label}: wrong error type: {err:?}" + ); + } + } + + #[test] + fn test_bgp_peer_conversion_numbered_preserves_fields() { + let ip: IpAddr = "10.0.0.1".parse().unwrap(); + let old = make_old_bgp_peer(Some(ip), 0); + let new = BgpPeer::try_from(old.clone()).unwrap(); + + assert_eq!( + new.addr, + RouterPeerType::Numbered { ip: ip.try_into().unwrap() } + ); + assert_eq!(new.bgp_config, old.bgp_config); + assert_eq!(new.hold_time, old.hold_time); + assert_eq!(new.idle_hold_time, old.idle_hold_time); + assert_eq!(new.delay_open, old.delay_open); + assert_eq!(new.connect_retry, old.connect_retry); + assert_eq!(new.keepalive, old.keepalive); + assert_eq!(new.remote_asn, old.remote_asn); + assert_eq!(new.enforce_first_as, old.enforce_first_as); + assert_eq!(new.allowed_import, old.allowed_import); + assert_eq!(new.allowed_export, old.allowed_export); + } + + #[test] + fn test_bgp_peer_conversion_unnumbered_preserves_fields() { + let old = make_old_bgp_peer(None, 300); + let new = BgpPeer::try_from(old.clone()).unwrap(); + + assert_eq!( + new.addr, + RouterPeerType::Unnumbered { + router_lifetime: RouterLifetimeConfig::new(300).unwrap() + } + ); + assert_eq!(new.bgp_config, old.bgp_config); + assert_eq!(new.hold_time, old.hold_time); + } + + #[test] + fn test_bgp_peer_reverse_numbered() { + let ip: IpAddr = "10.0.0.1".parse().unwrap(); + let new = BgpPeer::try_from(make_old_bgp_peer(Some(ip), 123)).unwrap(); + let back: crate::v2026_02_13_01::networking::BgpPeer = new.into(); + + assert_eq!(back.addr, Some(ip)); + + // router_lifetime is filled with the default for numbered peers + assert_eq!( + back.router_lifetime, + RouterLifetimeConfig::default().as_u16() + ); + + // interface_name gets the placeholder + assert_eq!(back.interface_name.as_str(), "deprecated-field"); + } + + #[test] + fn test_bgp_peer_reverse_unnumbered() { + let new = BgpPeer::try_from(make_old_bgp_peer(None, 300)).unwrap(); + let back: crate::v2026_02_13_01::networking::BgpPeer = new.into(); + + assert_eq!(back.addr, None); + assert_eq!(back.router_lifetime, 300); + } + + #[test] + fn test_bgp_peer_round_trip_numbered() { + let ip: IpAddr = "10.0.0.1".parse().unwrap(); + let original = make_old_bgp_peer(Some(ip), 0); + let new = BgpPeer::try_from(original.clone()).unwrap(); + let back: crate::v2026_02_13_01::networking::BgpPeer = new.into(); + + // addr and all non-removed fields should survive the round trip + assert_eq!(back.addr, original.addr); + assert_eq!(back.bgp_config, original.bgp_config); + assert_eq!(back.hold_time, original.hold_time); + assert_eq!(back.communities, original.communities); + assert_eq!(back.allowed_import, original.allowed_import); + } + + #[test] + fn test_bgp_peer_round_trip_unnumbered() { + let original = make_old_bgp_peer(None, 300); + let new = BgpPeer::try_from(original.clone()).unwrap(); + let back: crate::v2026_02_13_01::networking::BgpPeer = new.into(); + + assert_eq!(back.addr, original.addr); + assert_eq!(back.router_lifetime, original.router_lifetime); + assert_eq!(back.bgp_config, original.bgp_config); + assert_eq!(back.hold_time, original.hold_time); + } + + #[test] + fn test_bgp_peer_round_trip_sentinel_becomes_none() { + // Old clients might send Some(0.0.0.0) for unnumbered. After round + // trip, this normalizes to None (the canonical representation). + let original = + make_old_bgp_peer(Some(IpAddr::V4(Ipv4Addr::UNSPECIFIED)), 300); + let new = BgpPeer::try_from(original).unwrap(); + let back: crate::v2026_02_13_01::networking::BgpPeer = new.into(); + + assert_eq!(back.addr, None, "UNSPECIFIED should normalize to None"); + assert_eq!(back.router_lifetime, 300); + } +} diff --git a/openapi/nexus/nexus-2026032500.0.0-cf1221.json.gitstub b/openapi/nexus/nexus-2026032500.0.0-cf1221.json.gitstub new file mode 100644 index 00000000000..2914b100282 --- /dev/null +++ b/openapi/nexus/nexus-2026032500.0.0-cf1221.json.gitstub @@ -0,0 +1 @@ +254a0c51bc0beecb79c8a9dfccce8e7bc35b5ca4:openapi/nexus/nexus-2026032500.0.0-cf1221.json diff --git a/openapi/nexus/nexus-2026032500.0.0-cf1221.json b/openapi/nexus/nexus-2026041600.0.0-5b9d15.json similarity index 99% rename from openapi/nexus/nexus-2026032500.0.0-cf1221.json rename to openapi/nexus/nexus-2026041600.0.0-5b9d15.json index 72d5a2e5d37..0a17193d4f9 100644 --- a/openapi/nexus/nexus-2026032500.0.0-cf1221.json +++ b/openapi/nexus/nexus-2026041600.0.0-5b9d15.json @@ -7,7 +7,7 @@ "url": "https://oxide.computer", "email": "api@oxide.computer" }, - "version": "2026032500.0.0" + "version": "2026041600.0.0" }, "paths": { "/device/auth": { @@ -17362,14 +17362,16 @@ }, "BgpMessageHistory": {}, "BgpPeer": { - "description": "A BGP peer configuration for an interface. Includes the set of announcements that will be advertised to the peer identified by `addr`. The `bgp_config` parameter is a reference to global BGP parameters. The `interface_name` indicates what interface the peer should be contacted on.", + "description": "A BGP peer configuration for an interface. Includes the set of announcements that will be advertised to the peer. The `bgp_config` parameter is a reference to global BGP parameters.", "type": "object", "properties": { "addr": { - "nullable": true, - "description": "The address of the host to peer with. If not provided, this is an unnumbered BGP session that will be established over the interface specified by `interface_name`.", - "type": "string", - "format": "ip" + "description": "The address of the host to peer with, or specifying the configuration of an unnumbered BGP session.", + "allOf": [ + { + "$ref": "#/components/schemas/RouterPeerType" + } + ] }, "allowed_export": { "description": "Define export policy for a peer.", @@ -17405,7 +17407,7 @@ } }, "connect_retry": { - "description": "How long to to wait between TCP connection retries (seconds).", + "description": "How long to wait between TCP connection retries (seconds).", "type": "integer", "format": "uint32", "minimum": 0 @@ -17432,14 +17434,6 @@ "format": "uint32", "minimum": 0 }, - "interface_name": { - "description": "The name of interface to peer on. This is relative to the port configuration this BGP peer configuration is a part of. For example this value could be phy0 to refer to a primary physical interface. Or it could be vlan47 to refer to a VLAN interface.", - "allOf": [ - { - "$ref": "#/components/schemas/Name" - } - ] - }, "keepalive": { "description": "How often to send keepalive requests (seconds).", "type": "integer", @@ -17479,12 +17473,6 @@ "format": "uint32", "minimum": 0 }, - "router_lifetime": { - "description": "Router lifetime in seconds for unnumbered BGP peers.", - "type": "integer", - "format": "uint16", - "minimum": 0 - }, "vlan_id": { "nullable": true, "description": "Associate a VLAN ID with a peer.", @@ -17494,6 +17482,7 @@ } }, "required": [ + "addr", "allowed_export", "allowed_import", "bgp_config", @@ -17503,9 +17492,7 @@ "enforce_first_as", "hold_time", "idle_hold_time", - "interface_name", - "keepalive", - "router_lifetime" + "keepalive" ] }, "BgpPeerConfig": { @@ -26511,6 +26498,67 @@ } ] }, + "RouterLifetimeConfig": { + "description": "Router lifetime in seconds for unnumbered BGP peers", + "type": "integer", + "format": "uint16", + "minimum": 0, + "maximum": 9000 + }, + "RouterPeerIpAddr": { + "type": "string", + "format": "ip" + }, + "RouterPeerType": { + "oneOf": [ + { + "type": "object", + "properties": { + "router_lifetime": { + "description": "Router lifetime in seconds for unnumbered BGP peers.", + "allOf": [ + { + "$ref": "#/components/schemas/RouterLifetimeConfig" + } + ] + }, + "type": { + "type": "string", + "enum": [ + "unnumbered" + ] + } + }, + "required": [ + "router_lifetime", + "type" + ] + }, + { + "type": "object", + "properties": { + "ip": { + "description": "IP address for numbered BGP peers.", + "allOf": [ + { + "$ref": "#/components/schemas/RouterPeerIpAddr" + } + ] + }, + "type": { + "type": "string", + "enum": [ + "numbered" + ] + } + }, + "required": [ + "ip", + "type" + ] + } + ] + }, "RouterRoute": { "description": "A route defines a rule that governs where traffic should be sent based on its destination.", "type": "object", diff --git a/openapi/nexus/nexus-latest.json b/openapi/nexus/nexus-latest.json index bb7ce584356..26cedc7492d 120000 --- a/openapi/nexus/nexus-latest.json +++ b/openapi/nexus/nexus-latest.json @@ -1 +1 @@ -nexus-2026032500.0.0-cf1221.json \ No newline at end of file +nexus-2026041600.0.0-5b9d15.json \ No newline at end of file diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 31c660741e6..c3a3959ee06 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -3728,6 +3728,8 @@ CREATE TABLE IF NOT EXISTS omicron.public.switch_port_settings_bgp_peer_config ( allow_export_list_active BOOLEAN NOT NULL DEFAULT false, vlan_id INT4, id UUID NOT NULL, + -- TODO-correctness This should have a CHECK constraint that enforces the + -- upper bound we enforce on the Rust side. router_lifetime INT4 NOT NULL DEFAULT 0, PRIMARY KEY (id) diff --git a/sled-agent/types/versions/src/bgp_v6/early_networking.rs b/sled-agent/types/versions/src/bgp_v6/early_networking.rs index 2d423d77bc2..2d7ad4e2c8f 100644 --- a/sled-agent/types/versions/src/bgp_v6/early_networking.rs +++ b/sled-agent/types/versions/src/bgp_v6/early_networking.rs @@ -468,6 +468,9 @@ impl RouterLifetimeConfig { // Maximum valid router lifetime is 9000 seconds (2.5 hours) per RFC 4861 const MAX: u16 = 9000; + /// Construct a new `RouterLifetimeConfig` + /// + /// Fails if `v` is above the maximum allowable value. pub fn new(v: u16) -> Result { if v > Self::MAX { return Err(RouterLifetimeConfigError::ValueTooLarge); @@ -528,12 +531,12 @@ impl JsonSchema for RouterLifetimeConfig { #[derive(Debug, thiserror::Error)] pub enum RouterLifetimeConfigError { #[error( - "router lifetime config cannot be greater than {}", + "router_lifetime cannot be greater than {}", RouterLifetimeConfig::MAX )] ValueTooLarge, #[error( - "max path value must be an integer between {} and {}", + "router_lifetime must be an integer between {} and {}", RouterLifetimeConfig::DEFAULT, RouterLifetimeConfig::MAX )] diff --git a/sled-agent/types/versions/src/impls/early_networking.rs b/sled-agent/types/versions/src/impls/early_networking.rs index 02c258b6938..53b95bd0c89 100644 --- a/sled-agent/types/versions/src/impls/early_networking.rs +++ b/sled-agent/types/versions/src/impls/early_networking.rs @@ -192,6 +192,19 @@ impl RouterPeerType { /// we choose this sentinel value. pub const UNNUMBERED_SENTINEL: IpAddr = IpAddr::V4(Ipv4Addr::UNSPECIFIED); + /// Returns true if `Self` describes a numbered peer; false otherwise. + pub fn is_numbered(&self) -> bool { + match self { + Self::Unnumbered { .. } => false, + Self::Numbered { .. } => true, + } + } + + /// Returns true if `Self` describes an unnumbered peer; false otherwise. + pub fn is_unnumbered(&self) -> bool { + !self.is_numbered() + } + /// Squash this address down to an [`IpAddr`] by converting /// [`RouterPeerType::Unnumbered`] to /// [`RouterPeerType::UNNUMBERED_SENTINEL`]. @@ -204,6 +217,18 @@ impl RouterPeerType { Self::Numbered { ip } => ip.into(), } } + + /// Squash this address down to an [`Option`] by converting + /// [`RouterPeerType::Unnumbered`] to `None`. + /// + /// Uses of this function probably indicate places where we could consider + /// using stronger types. + pub fn ip_squashing_unnumbered_to_none(&self) -> Option { + match *self { + Self::Unnumbered { .. } => None, + Self::Numbered { ip } => Some(ip.into()), + } + } } impl UplinkAddress {