diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index 1bcb3ac229..f630bbbeac 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -1889,7 +1889,8 @@ allow_tables_to_appear_in_same_query!( allow_tables_to_appear_in_same_query!( switch_port, - switch_port_settings_bgp_peer_config + switch_port_settings_bgp_peer_config, + bgp_config ); allow_tables_to_appear_in_same_query!(disk, virtual_provisioning_resource); diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 649355f8e4..d0542874fb 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -17,7 +17,7 @@ use std::collections::BTreeMap; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(89, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(90, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy> = Lazy::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(90, "lookup-bgp-config-by-asn"), KnownVersion::new(89, "collapse_lldp_settings"), KnownVersion::new(88, "route-local-pref"), KnownVersion::new(87, "add-clickhouse-server-enum-variants"), diff --git a/nexus/db-queries/src/db/datastore/bgp.rs b/nexus/db-queries/src/db/datastore/bgp.rs index f4bea0f605..fdb9629543 100644 --- a/nexus/db-queries/src/db/datastore/bgp.rs +++ b/nexus/db-queries/src/db/datastore/bgp.rs @@ -28,7 +28,7 @@ use ref_cast::RefCast; use uuid::Uuid; impl DataStore { - pub async fn bgp_config_set( + pub async fn bgp_config_create( &self, opctx: &OpContext, config: ¶ms::BgpConfigCreate, @@ -37,80 +37,187 @@ impl DataStore { use db::schema::{ bgp_announce_set, bgp_announce_set::dsl as announce_set_dsl, }; - use diesel::sql_types; - use diesel::IntoSql; let conn = self.pool_connection_authorized(opctx).await?; - self.transaction_retry_wrapper("bgp_config_set") - .transaction(&conn, |conn| async move { - let announce_set_id: Uuid = match &config.bgp_announce_set_id { - NameOrId::Name(name) => { - announce_set_dsl::bgp_announce_set + let err = OptionalError::new(); + self.transaction_retry_wrapper("bgp_config_create") + .transaction(&conn, |conn| { + + let err = err.clone(); + async move { + let announce_set_id = match config.bgp_announce_set_id.clone() { + // Resolve Name to UUID + NameOrId::Name(name) => announce_set_dsl::bgp_announce_set .filter(bgp_announce_set::time_deleted.is_null()) .filter(bgp_announce_set::name.eq(name.to_string())) .select(bgp_announce_set::id) .limit(1) .first_async::(&conn) - .await? + .await + .map_err(|e| { + let msg = "failed to lookup announce set by name"; + error!(opctx.log, "{msg}"; "error" => ?e); + + match e { + diesel::result::Error::NotFound => { + err.bail(Error::not_found_by_name( + ResourceType::BgpAnnounceSet, + &name, + )) + } + _ => err.bail(Error::internal_error(msg)), + + } + }), + + // We cannot assume that the provided UUID is actually real. + // Lookup the parent record by UUID to verify that it is valid. + NameOrId::Id(id) => announce_set_dsl::bgp_announce_set + .filter(bgp_announce_set::time_deleted.is_null()) + .filter(bgp_announce_set::id.eq(id)) + .select(bgp_announce_set::id) + .limit(1) + .first_async::(&conn) + .await + .map_err(|e| { + let msg = "failed to lookup announce set by id"; + error!(opctx.log, "{msg}"; "error" => ?e); + + match e { + diesel::result::Error::NotFound => { + err.bail(Error::not_found_by_id( + ResourceType::BgpAnnounceSet, + &id, + )) + } + _ => err.bail(Error::internal_error(msg)), + + } + }), + }?; + + let config = + BgpConfig::from_config_create(config, announce_set_id); + + // Idempotency: + // Check to see if an exact match for the config already exists + let query = dsl::bgp_config + .filter(dsl::name.eq(config.name().to_string())) + .filter(dsl::asn.eq(config.asn)) + .filter(dsl::bgp_announce_set_id.eq(config.bgp_announce_set_id)) + .into_boxed(); + + let query = match config.vrf.clone() { + Some(v) => query.filter(dsl::vrf.eq(v)), + None => query.filter(dsl::vrf.is_null()), + }; + + let query = match config.shaper.clone() { + Some(v) => query.filter(dsl::shaper.eq(v)), + None => query.filter(dsl::shaper.is_null()), + }; + + let query = match config.checker.clone() { + Some(v) => query.filter(dsl::checker.eq(v)), + None => query.filter(dsl::checker.is_null()), + }; + + let matching_config = match query + .filter(dsl::time_deleted.is_null()) + .select(BgpConfig::as_select()) + .first_async::(&conn) + .await { + Ok(v) => Ok(Some(v)), + Err(e) => { + match e { + diesel::result::Error::NotFound => { + info!(opctx.log, "no matching bgp config found"); + Ok(None) + } + _ => { + let msg = "error while checking if bgp config exists"; + error!(opctx.log, "{msg}"; "error" => ?e); + Err(err.bail(Error::internal_error(msg))) + } + } + } + }?; + + // If so, we're done! + if let Some(existing_config) = matching_config { + return Ok(existing_config); } - NameOrId::Id(id) => *id, - }; - let config = - BgpConfig::from_config_create(config, announce_set_id); - - let matching_entry_subquery = dsl::bgp_config - .filter(dsl::name.eq(Name::from(config.name().clone()))) - .filter(dsl::time_deleted.is_null()) - .select(dsl::name); - - // SELECT exactly the values we're trying to INSERT, but only - // if it does not already exist. - let new_entry_subquery = diesel::dsl::select(( - config.id().into_sql::(), - config.name().to_string().into_sql::(), - config - .description() - .to_string() - .into_sql::(), - config.asn.into_sql::(), - config.bgp_announce_set_id.into_sql::(), - config - .vrf - .clone() - .into_sql::>(), - Utc::now().into_sql::(), - Utc::now().into_sql::(), - )) - .filter(diesel::dsl::not(diesel::dsl::exists( - matching_entry_subquery, - ))); - - diesel::insert_into(dsl::bgp_config) - .values(new_entry_subquery) - .into_columns(( - dsl::id, - dsl::name, - dsl::description, - dsl::asn, - dsl::bgp_announce_set_id, - dsl::vrf, - dsl::time_created, - dsl::time_modified, - )) - .execute_async(&conn) - .await?; + // TODO: remove once per-switch-multi-asn support is added + // Bail if a conflicting config for this ASN already exists. + // This is a temporary measure until multi-asn-per-switch is supported. + let configs_with_asn: Vec = dsl::bgp_config + .filter(dsl::asn.eq(config.asn)) + .filter(dsl::time_deleted.is_null()) + .select(BgpConfig::as_select()) + .load_async(&conn) + .await?; + + if !configs_with_asn.is_empty() { + error!( + opctx.log, + "different config for asn already exists"; + "asn" => ?config.asn, + "requested_config" => ?config, + "conflicting_configs" => ?configs_with_asn + ); + return Err(err.bail(Error::conflict("cannot have more than one configuration per ASN"))); + } - dsl::bgp_config - .filter(dsl::name.eq(Name::from(config.name().clone()))) - .filter(dsl::time_deleted.is_null()) - .select(BgpConfig::as_select()) - .limit(1) - .first_async(&conn) - .await + diesel::insert_into(dsl::bgp_config) + .values(config.clone()) + .returning(BgpConfig::as_returning()) + .get_result_async(&conn) + .await + .map_err(|e | { + let msg = "failed to insert bgp config"; + error!(opctx.log, "{msg}"; "error" => ?e); + + match e { + diesel::result::Error::DatabaseError(kind, _) => { + match kind { + diesel::result::DatabaseErrorKind::UniqueViolation => { + err.bail(Error::conflict("a field that must be unique conflicts with an existing record")) + }, + // technically we don't use Foreign Keys but it doesn't hurt to match on them + // instead of returning a 500 by default in the event that we do switch to Foreign Keys + diesel::result::DatabaseErrorKind::ForeignKeyViolation => { + err.bail(Error::conflict("an id field references an object that does not exist")) + } + diesel::result::DatabaseErrorKind::NotNullViolation => { + err.bail(Error::invalid_request("a required field was not provided")) + } + diesel::result::DatabaseErrorKind::CheckViolation => { + err.bail(Error::invalid_request("one or more fields are not valid values")) + }, + _ => err.bail(Error::internal_error(msg)), + } + } + _ => err.bail(Error::internal_error(msg)), + } + }) + } }) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + .map_err(|e|{ + let msg = "bgp_config_create failed"; + if let Some(err) = err.take() { + error!(opctx.log, "{msg}"; "error" => ?err); + err + } else { + // The transaction handler errors along with any errors emitted via "?" + // will fall through to here. These errors should truly be 500s + // because they are an internal hiccup that likely was not triggered by + // user input. + error!(opctx.log, "{msg}"; "error" => ?e); + public_error_from_diesel(e, ErrorHandler::Server) + } + }) } pub async fn bgp_config_delete( @@ -124,11 +231,6 @@ impl DataStore { use db::schema::switch_port_settings_bgp_peer_config as sps_bgp_peer_config; use db::schema::switch_port_settings_bgp_peer_config::dsl as sps_bgp_peer_config_dsl; - #[derive(Debug)] - enum BgpConfigDeleteError { - ConfigInUse, - } - let err = OptionalError::new(); let conn = self.pool_connection_authorized(opctx).await?; self.transaction_retry_wrapper("bgp_config_delete") @@ -138,26 +240,60 @@ impl DataStore { let name_or_id = sel.name_or_id.clone(); let id: Uuid = match name_or_id { - NameOrId::Id(id) => id, - NameOrId::Name(name) => { + NameOrId::Id(id) => bgp_config_dsl::bgp_config + .filter(bgp_config::id.eq(id)) + .select(bgp_config::id) + .limit(1) + .first_async::(&conn) + .await + .map_err(|e| { + let msg = "failed to lookup bgp config by id"; + error!(opctx.log, "{msg}"; "error" => ?e); + + match e { + diesel::result::Error::NotFound => { + err.bail(Error::not_found_by_id( + ResourceType::BgpConfig, + &id, + )) + } + _ => err.bail(Error::internal_error(msg)), + + } + }), + NameOrId::Name(name) => bgp_config_dsl::bgp_config - .filter(bgp_config::name.eq(name.to_string())) - .select(bgp_config::id) - .limit(1) - .first_async::(&conn) - .await? - } - }; + .filter(bgp_config::name.eq(name.to_string())) + .select(bgp_config::id) + .limit(1) + .first_async::(&conn) + .await + .map_err(|e| { + let msg = "failed to lookup bgp config by name"; + error!(opctx.log, "{msg}"; "error" => ?e); + + match e { + diesel::result::Error::NotFound => { + err.bail(Error::not_found_by_name( + ResourceType::BgpConfig, + &name, + )) + } + _ => err.bail(Error::internal_error(msg)), + + } + }), + }?; let count = sps_bgp_peer_config_dsl::switch_port_settings_bgp_peer_config - .filter(sps_bgp_peer_config::bgp_config_id.eq(id)) - .count() - .execute_async(&conn) - .await?; + .filter(sps_bgp_peer_config::bgp_config_id.eq(id)) + .count() + .execute_async(&conn) + .await?; if count > 0 { - return Err(err.bail(BgpConfigDeleteError::ConfigInUse)); + return Err(err.bail(Error::conflict("BGP Config is in use and cannot be deleted"))); } diesel::update(bgp_config_dsl::bgp_config) @@ -171,13 +307,12 @@ impl DataStore { }) .await .map_err(|e| { + let msg = "bgp_config_delete failed"; if let Some(err) = err.take() { - match err { - BgpConfigDeleteError::ConfigInUse => { - Error::invalid_request("BGP config in use") - } - } + error!(opctx.log, "{msg}"; "error" => ?err); + err } else { + error!(opctx.log, "{msg}"; "error" => ?e); public_error_from_diesel(e, ErrorHandler::Server) } }) @@ -194,24 +329,45 @@ impl DataStore { let name_or_id = name_or_id.clone(); - let config = match name_or_id { + match name_or_id { NameOrId::Name(name) => dsl::bgp_config .filter(bgp_config::name.eq(name.to_string())) .select(BgpConfig::as_select()) .limit(1) .first_async::(&*conn) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)), + .map_err(|e| { + let msg = "failed to lookup bgp config by name"; + error!(opctx.log, "{msg}"; "error" => ?e); + + match e { + diesel::result::Error::NotFound => { + Error::not_found_by_name( + ResourceType::BgpConfig, + &name, + ) + } + _ => Error::internal_error(msg), + } + }), NameOrId::Id(id) => dsl::bgp_config .filter(bgp_config::id.eq(id)) .select(BgpConfig::as_select()) .limit(1) .first_async::(&*conn) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)), - }?; + .map_err(|e| { + let msg = "failed to lookup bgp config by id"; + error!(opctx.log, "{msg}"; "error" => ?e); - Ok(config) + match e { + diesel::result::Error::NotFound => { + Error::not_found_by_id(ResourceType::BgpConfig, &id) + } + _ => Error::internal_error(msg), + } + }), + } } pub async fn bgp_config_list( @@ -237,10 +393,42 @@ impl DataStore { .select(BgpConfig::as_select()) .load_async(&*conn) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + .map_err(|e| { + error!(opctx.log, "bgp_config_list failed"; "error" => ?e); + public_error_from_diesel(e, ErrorHandler::Server) + }) + } + + pub async fn bgp_announce_set_list( + &self, + opctx: &OpContext, + pagparams: &PaginatedBy<'_>, + ) -> ListResultVec { + use db::schema::bgp_announce_set::dsl; + + let conn = self.pool_connection_authorized(opctx).await?; + + match pagparams { + PaginatedBy::Id(pagparams) => { + paginated(dsl::bgp_announce_set, dsl::id, &pagparams) + } + PaginatedBy::Name(pagparams) => paginated( + dsl::bgp_announce_set, + dsl::name, + &pagparams.map_name(|n| Name::ref_cast(n)), + ), + } + .filter(dsl::time_deleted.is_null()) + .select(BgpAnnounceSet::as_select()) + .load_async(&*conn) + .await + .map_err(|e| { + error!(opctx.log, "bgp_announce_set_list failed"; "error" => ?e); + public_error_from_diesel(e, ErrorHandler::Server) + }) } - pub async fn bgp_announce_list( + pub async fn bgp_announcement_list( &self, opctx: &OpContext, sel: ¶ms::BgpAnnounceSetSelector, @@ -250,11 +438,6 @@ impl DataStore { bgp_announcement::dsl as announce_dsl, }; - #[derive(Debug)] - enum BgpAnnounceListError { - AnnounceSetNotFound(Name), - } - let err = OptionalError::new(); let conn = self.pool_connection_authorized(opctx).await?; self.transaction_retry_wrapper("bgp_announce_list") @@ -264,7 +447,26 @@ impl DataStore { let name_or_id = sel.name_or_id.clone(); let announce_id: Uuid = match name_or_id { - NameOrId::Id(id) => id, + NameOrId::Id(id) => announce_set_dsl::bgp_announce_set + .filter(bgp_announce_set::time_deleted.is_null()) + .filter(bgp_announce_set::id.eq(id)) + .select(bgp_announce_set::id) + .limit(1) + .first_async::(&conn) + .await + .map_err(|e| { + let msg = "failed to lookup announce set by id"; + error!(opctx.log, "{msg}"; "error" => ?e); + + match e { + diesel::result::Error::NotFound => err + .bail(Error::not_found_by_id( + ResourceType::BgpAnnounceSet, + &id, + )), + _ => err.bail(Error::internal_error(msg)), + } + }), NameOrId::Name(name) => { announce_set_dsl::bgp_announce_set .filter( @@ -278,15 +480,23 @@ impl DataStore { .first_async::(&conn) .await .map_err(|e| { - err.bail_retryable_or( - e, - BgpAnnounceListError::AnnounceSetNotFound( - Name::from(name.clone()), - ) - ) - })? + let msg = + "failed to lookup announce set by name"; + error!(opctx.log, "{msg}"; "error" => ?e); + + match e { + diesel::result::Error::NotFound => err + .bail(Error::not_found_by_name( + ResourceType::BgpAnnounceSet, + &name, + )), + _ => { + err.bail(Error::internal_error(msg)) + } + } + }) } - }; + }?; let result = announce_dsl::bgp_announcement .filter(announce_dsl::announce_set_id.eq(announce_id)) @@ -299,21 +509,18 @@ impl DataStore { }) .await .map_err(|e| { + error!(opctx.log, "bgp_announce_list failed"; "error" => ?e); if let Some(err) = err.take() { - match err { - BgpAnnounceListError::AnnounceSetNotFound(name) => { - Error::not_found_by_name( - ResourceType::BgpAnnounceSet, - &name, - ) - } - } + err } else { public_error_from_diesel(e, ErrorHandler::Server) } }) } + // TODO: it seems this logic actually performs a find OR create for an announce set, and then replaces its child announcements. + // This might be changed in omicron#6016 to an api that creates an announce set then allows adding / removal of announcements + // to match how our other APIs work. pub async fn bgp_update_announce_set( &self, opctx: &OpContext, @@ -383,9 +590,16 @@ impl DataStore { Ok((db_as, db_annoucements)) }) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + .map_err(|e| { + let msg = "bgp_update_announce_set failed"; + error!(opctx.log, "{msg}"; "error" => ?e); + public_error_from_diesel(e, ErrorHandler::Server) + }) } + // TODO: it seems this logic actually performs a create OR update of an announce set and its child announcements + // (for example, it will add missing announcements). This might be changed in omicron#6016 to an api that creates an announce set + // then allows adding / removal of announcements to match how our other APIs work. pub async fn bgp_create_announce_set( &self, opctx: &OpContext, @@ -466,7 +680,11 @@ impl DataStore { Ok((db_as, db_annoucements)) }) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + .map_err(|e| { + let msg = "bgp_create_announce_set failed"; + error!(opctx.log, "{msg}"; "error" => ?e); + public_error_from_diesel(e, ErrorHandler::Server) + }) } pub async fn bgp_delete_announce_set( @@ -481,11 +699,6 @@ impl DataStore { use db::schema::bgp_config; use db::schema::bgp_config::dsl as bgp_config_dsl; - #[derive(Debug)] - enum BgpAnnounceSetDeleteError { - AnnounceSetInUse, - } - let conn = self.pool_connection_authorized(opctx).await?; let name_or_id = sel.name_or_id.clone(); @@ -496,18 +709,56 @@ impl DataStore { let name_or_id = name_or_id.clone(); async move { let id: Uuid = match name_or_id { + NameOrId::Id(id) => announce_set_dsl::bgp_announce_set + .filter(bgp_announce_set::time_deleted.is_null()) + .filter(bgp_announce_set::id.eq(id)) + .select(bgp_announce_set::id) + .limit(1) + .first_async::(&conn) + .await + .map_err(|e| { + let msg = "failed to lookup announce set by id"; + error!(opctx.log, "{msg}"; "error" => ?e); + + match e { + diesel::result::Error::NotFound => err + .bail(Error::not_found_by_id( + ResourceType::BgpAnnounceSet, + &id, + )), + _ => err.bail(Error::internal_error(msg)), + } + }), NameOrId::Name(name) => { announce_set_dsl::bgp_announce_set + .filter( + bgp_announce_set::time_deleted.is_null(), + ) .filter( bgp_announce_set::name.eq(name.to_string()), ) .select(bgp_announce_set::id) .limit(1) .first_async::(&conn) - .await? + .await + .map_err(|e| { + let msg = + "failed to lookup announce set by name"; + error!(opctx.log, "{msg}"; "error" => ?e); + + match e { + diesel::result::Error::NotFound => err + .bail(Error::not_found_by_name( + ResourceType::BgpAnnounceSet, + &name, + )), + _ => { + err.bail(Error::internal_error(msg)) + } + } + }) } - NameOrId::Id(id) => id, - }; + }?; let count = bgp_config_dsl::bgp_config .filter(bgp_config::bgp_announce_set_id.eq(id)) @@ -516,9 +767,9 @@ impl DataStore { .await?; if count > 0 { - return Err(err.bail( - BgpAnnounceSetDeleteError::AnnounceSetInUse, - )); + return Err( + err.bail(Error::conflict("announce set in use")) + ); } diesel::update(announce_set_dsl::bgp_announce_set) @@ -537,13 +788,12 @@ impl DataStore { }) .await .map_err(|e| { + let msg = "bgp_delete_announce_set failed"; if let Some(err) = err.take() { - match err { - BgpAnnounceSetDeleteError::AnnounceSetInUse => { - Error::invalid_request("BGP announce set in use") - } - } + error!(opctx.log, "{msg}"; "error" => ?err); + err } else { + error!(opctx.log, "{msg}"; "error" => ?e); public_error_from_diesel(e, ErrorHandler::Server) } }) @@ -563,7 +813,11 @@ impl DataStore { .select(BgpPeerView::as_select()) .load_async(&*self.pool_connection_authorized(opctx).await?) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + .map_err(|e| { + let msg = "bgp_peer_configs failed"; + error!(opctx.log, "{msg}"; "error" => ?e); + public_error_from_diesel(e, ErrorHandler::Server) + })?; Ok(results) } @@ -583,7 +837,11 @@ impl DataStore { .filter(dsl::addr.eq(addr)) .load_async(&*self.pool_connection_authorized(opctx).await?) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + .map_err(|e| { + let msg = "communities_for_peer failed"; + error!(opctx.log, "{msg}"; "error" => ?e); + public_error_from_diesel(e, ErrorHandler::Server) + })?; Ok(results) } @@ -601,24 +859,40 @@ impl DataStore { use db::schema::switch_port_settings_bgp_peer_config_allow_export::dsl; let conn = self.pool_connection_authorized(opctx).await?; - let result = self - .transaction_retry_wrapper("bgp_allow_export_for_peer") - .transaction(&conn, |conn| async move { - let active = 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?; - - if !active { - return Ok(None); - } + let err = OptionalError::new(); + self.transaction_retry_wrapper("bgp_allow_export_for_peer") + .transaction(&conn, |conn| { + let err = err.clone(); + async move { + let active = 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 + .map_err(|e| { + let msg = "failed to lookup export settings for peer"; + error!(opctx.log, "{msg}"; "error" => ?e); + + match e { + diesel::result::Error::NotFound => { + let not_found_msg = format!("peer with {addr} not found for port settings {port_settings_id}"); + err.bail(Error::non_resourcetype_not_found(not_found_msg)) + }, + _ => err.bail(Error::internal_error(msg)), + } + })?; + + if !active { + return Ok(None); + } - let list = - dsl::switch_port_settings_bgp_peer_config_allow_export - .filter(db_allow::port_settings_id.eq(port_settings_id)) + let list = + dsl::switch_port_settings_bgp_peer_config_allow_export + .filter( + db_allow::port_settings_id.eq(port_settings_id), + ) .filter( db_allow::interface_name .eq(interface_name.to_owned()), @@ -627,12 +901,20 @@ impl DataStore { .load_async(&conn) .await?; - Ok(Some(list)) + Ok(Some(list)) + } }) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; - - Ok(result) + .map_err(|e| { + let msg = "allow_export_for_peer failed"; + if let Some(err) = err.take() { + error!(opctx.log, "{msg}"; "error" => ?err); + err + } else { + error!(opctx.log, "{msg}"; "error" => ?e); + public_error_from_diesel(e, ErrorHandler::Server) + } + }) } pub async fn allow_import_for_peer( @@ -647,25 +929,42 @@ impl DataStore { use db::schema::switch_port_settings_bgp_peer_config_allow_import as db_allow; use db::schema::switch_port_settings_bgp_peer_config_allow_import::dsl; + let err = OptionalError::new(); let conn = self.pool_connection_authorized(opctx).await?; - let result = self - .transaction_retry_wrapper("bgp_allow_export_for_peer") - .transaction(&conn, |conn| async move { - let active = 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?; - - if !active { - return Ok(None); - } + self + .transaction_retry_wrapper("bgp_allow_import_for_peer") + .transaction(&conn, |conn| { + let err = err.clone(); + async move { + let active = 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 + .map_err(|e| { + let msg = "failed to lookup import settings for peer"; + error!(opctx.log, "{msg}"; "error" => ?e); + + match e { + diesel::result::Error::NotFound => { + let not_found_msg = format!("peer with {addr} not found for port settings {port_settings_id}"); + err.bail(Error::non_resourcetype_not_found(not_found_msg)) + }, + _ => err.bail(Error::internal_error(msg)), + } + })?; + + if !active { + return Ok(None); + } - let list = - dsl::switch_port_settings_bgp_peer_config_allow_import - .filter(db_allow::port_settings_id.eq(port_settings_id)) + let list = + dsl::switch_port_settings_bgp_peer_config_allow_import + .filter( + db_allow::port_settings_id.eq(port_settings_id), + ) .filter( db_allow::interface_name .eq(interface_name.to_owned()), @@ -674,11 +973,19 @@ impl DataStore { .load_async(&conn) .await?; - Ok(Some(list)) + Ok(Some(list)) + } }) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; - - Ok(result) + .map_err(|e| { + let msg = "allow_import_for_peer failed"; + if let Some(err) = err.take() { + error!(opctx.log, "{msg}"; "error" => ?err); + err + } else { + error!(opctx.log, "{msg}"; "error" => ?e); + public_error_from_diesel(e, ErrorHandler::Server) + } + }) } } diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 13c3708e4a..2cd21754f8 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -366,6 +366,7 @@ impl DataStore { } } +#[derive(Clone, Copy, Debug)] pub enum UpdatePrecondition { DontCare, Null, diff --git a/nexus/db-queries/src/db/datastore/switch_port.rs b/nexus/db-queries/src/db/datastore/switch_port.rs index 504e6cf936..2e09c1ac13 100644 --- a/nexus/db-queries/src/db/datastore/switch_port.rs +++ b/nexus/db-queries/src/db/datastore/switch_port.rs @@ -31,7 +31,7 @@ use diesel::{ use diesel_dtrace::DTraceConnection; use ipnetwork::IpNetwork; use nexus_db_model::{ - SqlU16, SqlU32, SqlU8, SwitchPortBgpPeerConfigAllowExport, + BgpConfig, SqlU16, SqlU32, SqlU8, SwitchPortBgpPeerConfigAllowExport, SwitchPortBgpPeerConfigAllowImport, SwitchPortBgpPeerConfigCommunity, }; use nexus_types::external_api::params; @@ -333,6 +333,7 @@ impl DataStore { SwitchPortSettingsCreateError::ReserveBlock( ReserveBlockError::AddressNotInLot, ) => Error::invalid_request("address not in lot"), + } } else { @@ -828,45 +829,158 @@ impl DataStore { port_settings_id: Option, current: UpdatePrecondition, ) -> UpdateResult<()> { + use db::schema::bgp_config::dsl as bgp_config_dsl; use db::schema::switch_port; use db::schema::switch_port::dsl as switch_port_dsl; + use db::schema::switch_port_settings_bgp_peer_config::dsl as bgp_peer_dsl; let conn = self.pool_connection_authorized(opctx).await?; - match current { - UpdatePrecondition::DontCare => { - diesel::update(switch_port_dsl::switch_port) - .filter(switch_port::id.eq(switch_port_id)) - .set(switch_port::port_settings_id.eq(port_settings_id)) - .execute_async(&*conn) - .await - .map_err(|e| { - public_error_from_diesel(e, ErrorHandler::Server) - })?; - } - UpdatePrecondition::Null => { - diesel::update(switch_port_dsl::switch_port) - .filter(switch_port::id.eq(switch_port_id)) - .filter(switch_port::port_settings_id.is_null()) - .set(switch_port::port_settings_id.eq(port_settings_id)) - .execute_async(&*conn) - .await - .map_err(|e| { - public_error_from_diesel(e, ErrorHandler::Server) - })?; - } - UpdatePrecondition::Value(current_id) => { - diesel::update(switch_port_dsl::switch_port) - .filter(switch_port::id.eq(switch_port_id)) - .filter(switch_port::port_settings_id.eq(current_id)) - .set(switch_port::port_settings_id.eq(port_settings_id)) - .execute_async(&*conn) - .await - .map_err(|e| { - public_error_from_diesel(e, ErrorHandler::Server) - })?; - } - } + let err = OptionalError::new(); + self.transaction_retry_wrapper("switch_port_set_settings_id") + .transaction(&conn, |conn| { + let err = err.clone(); + async move { + // TODO: remove once per-switch-multi-asn support is added + // Bail if user attempts to assign multiple ASNs to a switch via switch port settings + // This is a temporary measure until multi-asn-per-switch is supported. + + // what switch are we adding a configuration to? + let switch = switch_port_dsl::switch_port + .filter(switch_port_dsl::id.eq(switch_port_id)) + .select(switch_port_dsl::switch_location) + .limit(1) + .first_async::(&conn) + .await + .map_err(|e: diesel::result::Error| { + let msg = "failed to look up switch port by id"; + error!(opctx.log, "{msg}"; "error" => ?e); + match e { + diesel::result::Error::NotFound => { + err.bail(Error::not_found_by_id( + ResourceType::SwitchPort, + &switch_port_id, + )) + } + _ => err.bail(Error::internal_error(msg)), + } + })?; + + // if we're setting a port settings id (and therefore activating a configuration + // on a port) we need to make sure there aren't any conflicting bgp configurations + if let Some(psid) = port_settings_id { + let bgp_config: Option = + match bgp_peer_dsl::switch_port_settings_bgp_peer_config + .inner_join( + bgp_config_dsl::bgp_config + .on(bgp_peer_dsl::bgp_config_id + .eq(bgp_config_dsl::id)), + ) + .filter( + bgp_peer_dsl::port_settings_id + .eq(psid), + ) + .select(BgpConfig::as_select()) + .limit(1) + .first_async::(&conn) + .await { + Ok(v) => Ok(Some(v)), + Err(e) => { + let msg = "failed to check if bgp peer exists in switch port settings"; + error!(opctx.log, "{msg}"; "error" => ?e); + match e { + diesel::result::Error::NotFound => { + Ok(None) + } + _ => Err(err.bail(Error::internal_error(msg))), + } + } + }?; + + // find all port settings for the targeted switch + // switch port + // inner join bgp peer on port settings id + // inner join bgp config on bgp config id + // filter switch location eq switch + // filter port settings id not null + // filter asn doesn't equal our asn + + if let Some(config) = bgp_config { + let conflicting_bgp_configs: Vec = switch_port_dsl::switch_port + .inner_join( + bgp_peer_dsl::switch_port_settings_bgp_peer_config + .on(bgp_peer_dsl::port_settings_id + .nullable() + .eq(switch_port_dsl::port_settings_id)), + ) + .inner_join(bgp_config_dsl::bgp_config.on( + bgp_peer_dsl::bgp_config_id.eq(bgp_config_dsl::id), + )) + .filter(switch_port_dsl::switch_location.eq(switch)) + .filter(switch_port_dsl::port_settings_id.is_not_null()) + .filter(bgp_config_dsl::asn.ne(config.asn)) + .select(BgpConfig::as_select()) + .load_async(&conn) + .await?; + + if !conflicting_bgp_configs.is_empty() { + return Err(err.bail(Error::conflict("a different asn is already configured on this switch"))); + } + } + + } + + // perform the requested update + match current { + UpdatePrecondition::DontCare => { + diesel::update(switch_port_dsl::switch_port) + .filter(switch_port::id.eq(switch_port_id)) + .set( + switch_port::port_settings_id + .eq(port_settings_id), + ) + .execute_async(&conn) + .await + } + UpdatePrecondition::Null => { + diesel::update(switch_port_dsl::switch_port) + .filter(switch_port::id.eq(switch_port_id)) + .filter(switch_port::port_settings_id.is_null()) + .set( + switch_port::port_settings_id + .eq(port_settings_id), + ) + .execute_async(&conn) + .await + } + UpdatePrecondition::Value(current_id) => { + diesel::update(switch_port_dsl::switch_port) + .filter(switch_port::id.eq(switch_port_id)) + .filter( + switch_port::port_settings_id + .eq(current_id), + ) + .set( + switch_port::port_settings_id + .eq(port_settings_id), + ) + .execute_async(&conn) + .await + } + } + } + }) + .await + .map_err(|e| { + let msg = "switch_port_set_settings_id failed"; + if let Some(err) = err.take() { + error!(opctx.log, "{msg}"; "error" => ?err); + err + } else { + error!(opctx.log, "{msg}"; "error" => ?e); + public_error_from_diesel(e, ErrorHandler::Server) + } + })?; Ok(()) } @@ -945,10 +1059,10 @@ impl DataStore { .eq(route_config_dsl::port_settings_id.nullable())), ) .select(SwitchPort::as_select()) - // TODO: #3592 Correctness - // In single rack deployments there are only 64 ports. We'll need - // pagination in the future, or maybe a way to constrain the query to - // a rack? + // TODO: #3592 Correctness + // In single rack deployments there are only 64 ports. We'll need + // pagination in the future, or maybe a way to constrain the query to + // a rack? .limit(64) .union( switch_port_dsl::switch_port @@ -957,7 +1071,7 @@ impl DataStore { bgp_peer_config_dsl::switch_port_settings_bgp_peer_config .on(switch_port_dsl::port_settings_id .eq(bgp_peer_config_dsl::port_settings_id.nullable()), - ), + ), ) .select(SwitchPort::as_select()) .limit(64), @@ -1148,18 +1262,18 @@ async fn do_switch_port_settings_create( NameOrId::Name(name) => { let name = name.to_string(); bgp_config_dsl::bgp_config - .filter(bgp_config::time_deleted.is_null()) - .filter(bgp_config::name.eq(name)) - .select(bgp_config::id) - .limit(1) - .first_async::(conn) - .await - .map_err(|diesel_error| { - err.bail_retryable_or( - diesel_error, - SwitchPortSettingsCreateError::BgpConfigNotFound - ) - })? + .filter(bgp_config::time_deleted.is_null()) + .filter(bgp_config::name.eq(name)) + .select(bgp_config::id) + .limit(1) + .first_async::(conn) + .await + .map_err(|diesel_error| { + err.bail_retryable_or( + diesel_error, + SwitchPortSettingsCreateError::BgpConfigNotFound + ) + })? } }; @@ -1177,9 +1291,9 @@ async fn do_switch_port_settings_create( .collect(); diesel::insert_into(allow_import_dsl::switch_port_settings_bgp_peer_config_allow_import) - .values(to_insert) - .execute_async(conn) - .await?; + .values(to_insert) + .execute_async(conn) + .await?; } if let ImportExportPolicy::Allow(list) = &p.allowed_export { @@ -1196,9 +1310,9 @@ async fn do_switch_port_settings_create( .collect(); diesel::insert_into(allow_export_dsl::switch_port_settings_bgp_peer_config_allow_export) - .values(to_insert) - .execute_async(conn) - .await?; + .values(to_insert) + .execute_async(conn) + .await?; } if !p.communities.is_empty() { @@ -1216,9 +1330,9 @@ async fn do_switch_port_settings_create( .collect(); diesel::insert_into(bgp_communities_dsl::switch_port_settings_bgp_peer_config_communities) - .values(to_insert) - .execute_async(conn) - .await?; + .values(to_insert) + .execute_async(conn) + .await?; } bgp_peer_config.push(SwitchPortBgpPeerConfig::new( @@ -1229,6 +1343,7 @@ async fn do_switch_port_settings_create( )); } } + let db_bgp_peers: Vec = diesel::insert_into(bgp_peer_dsl::switch_port_settings_bgp_peer_config) .values(bgp_peer_config) @@ -1282,18 +1397,18 @@ async fn do_switch_port_settings_create( NameOrId::Name(name) => { let name = name.to_string(); address_lot_dsl::address_lot - .filter(address_lot::time_deleted.is_null()) - .filter(address_lot::name.eq(name)) - .select(address_lot::id) - .limit(1) - .first_async::(conn) - .await - .map_err(|diesel_error| { - err.bail_retryable_or( - diesel_error, - SwitchPortSettingsCreateError::AddressLotNotFound - ) - })? + .filter(address_lot::time_deleted.is_null()) + .filter(address_lot::name.eq(name)) + .select(address_lot::id) + .limit(1) + .first_async::(conn) + .await + .map_err(|diesel_error| { + err.bail_retryable_or( + diesel_error, + SwitchPortSettingsCreateError::AddressLotNotFound + ) + })? } }; // TODO: Reduce DB round trips needed for reserving ip blocks @@ -1353,18 +1468,18 @@ async fn do_switch_port_settings_delete( NameOrId::Name(name) => { let name = name.to_string(); port_settings_dsl::switch_port_settings - .filter(switch_port_settings::time_deleted.is_null()) - .filter(switch_port_settings::name.eq(name)) - .select(switch_port_settings::id) - .limit(1) - .first_async::(conn) - .await - .map_err(|diesel_error| { - err.bail_retryable_or( - diesel_error, - SwitchPortSettingsDeleteError::SwitchPortSettingsNotFound - ) - })? + .filter(switch_port_settings::time_deleted.is_null()) + .filter(switch_port_settings::name.eq(name)) + .select(switch_port_settings::id) + .limit(1) + .first_async::(conn) + .await + .map_err(|diesel_error| { + err.bail_retryable_or( + diesel_error, + SwitchPortSettingsDeleteError::SwitchPortSettingsNotFound + ) + })? } }; @@ -1559,7 +1674,7 @@ mod test { shaper: None, }; - datastore.bgp_config_set(&opctx, &bgp_config).await.unwrap(); + datastore.bgp_config_create(&opctx, &bgp_config).await.unwrap(); let settings = SwitchPortSettingsCreate { identity: IdentityMetadataCreateParams { diff --git a/nexus/src/app/background/tasks/sync_switch_configuration.rs b/nexus/src/app/background/tasks/sync_switch_configuration.rs index 154384dd5e..f86bb1a782 100644 --- a/nexus/src/app/background/tasks/sync_switch_configuration.rs +++ b/nexus/src/app/background/tasks/sync_switch_configuration.rs @@ -565,7 +565,7 @@ impl BackgroundTask for SwitchPortSettingsManager { if !bgp_announce_prefixes.contains_key(&bgp_config.bgp_announce_set_id) { let announcements = match self .datastore - .bgp_announce_list( + .bgp_announcement_list( opctx, ¶ms::BgpAnnounceSetSelector { name_or_id: bgp_config diff --git a/nexus/src/app/bgp.rs b/nexus/src/app/bgp.rs index d192f1ccf9..31a0faa663 100644 --- a/nexus/src/app/bgp.rs +++ b/nexus/src/app/bgp.rs @@ -16,13 +16,13 @@ use omicron_common::api::external::{ use std::net::IpAddr; impl super::Nexus { - pub async fn bgp_config_set( + pub async fn bgp_config_create( &self, opctx: &OpContext, config: ¶ms::BgpConfigCreate, ) -> CreateResult { opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; - let result = self.db_datastore.bgp_config_set(opctx, config).await?; + let result = self.db_datastore.bgp_config_create(opctx, config).await?; Ok(result) } @@ -69,13 +69,13 @@ impl super::Nexus { Ok(result) } - pub async fn bgp_announce_list( + pub async fn bgp_announce_set_list( &self, opctx: &OpContext, - sel: ¶ms::BgpAnnounceSetSelector, - ) -> ListResultVec { + pagparams: &PaginatedBy<'_>, + ) -> ListResultVec { opctx.authorize(authz::Action::Read, &authz::FLEET).await?; - self.db_datastore.bgp_announce_list(opctx, sel).await + self.db_datastore.bgp_announce_set_list(opctx, pagparams).await } pub async fn bgp_delete_announce_set( @@ -89,6 +89,15 @@ impl super::Nexus { Ok(result) } + pub async fn bgp_announcement_list( + &self, + opctx: &OpContext, + sel: ¶ms::BgpAnnounceSetSelector, + ) -> ListResultVec { + opctx.authorize(authz::Action::Read, &authz::FLEET).await?; + self.db_datastore.bgp_announcement_list(opctx, sel).await + } + pub async fn bgp_peer_status( &self, opctx: &OpContext, diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index 019314c527..f3c0031327 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -510,7 +510,7 @@ impl super::Nexus { match self .db_datastore - .bgp_config_set( + .bgp_config_create( &opctx, &BgpConfigCreate { identity: IdentityMetadataCreateParams { diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 015fe11e3a..e11256f06e 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -285,6 +285,8 @@ pub(crate) fn external_api() -> NexusApiDescription { api.register(networking_bgp_announce_set_delete)?; api.register(networking_bgp_message_history)?; + api.register(networking_bgp_announcement_list)?; + api.register(networking_bfd_enable)?; api.register(networking_bfd_disable)?; api.register(networking_bfd_status)?; @@ -3866,7 +3868,7 @@ async fn networking_bgp_config_create( let nexus = &apictx.context.nexus; let config = config.into_inner(); let opctx = crate::context::op_context_for_external_api(&rqctx).await?; - let result = nexus.bgp_config_set(&opctx, &config).await?; + let result = nexus.bgp_config_create(&opctx, &config).await?; Ok(HttpResponseCreated::(result.into())) }; apictx @@ -4044,7 +4046,7 @@ async fn networking_bgp_config_delete( /// set with the one specified. #[endpoint { method = PUT, - path = "/v1/system/networking/bgp-announce", + path = "/v1/system/networking/bgp-announce-set", tags = ["system/networking"], }] async fn networking_bgp_announce_set_update( @@ -4066,24 +4068,28 @@ async fn networking_bgp_announce_set_update( .await } -//TODO pagination? the normal by-name/by-id stuff does not work here -/// Get originated routes for a BGP configuration +/// List BGP announce sets #[endpoint { method = GET, - path = "/v1/system/networking/bgp-announce", + path = "/v1/system/networking/bgp-announce-set", tags = ["system/networking"], }] async fn networking_bgp_announce_set_list( rqctx: RequestContext, - query_params: Query, -) -> Result>, HttpError> { + query_params: Query< + PaginatedByNameOrId, + >, +) -> Result>, HttpError> { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.context.nexus; - let sel = query_params.into_inner(); + let query = query_params.into_inner(); + let pag_params = data_page_params_for(&rqctx, &query)?; + let scan_params = ScanByNameOrId::from_query(&query)?; + let paginated_by = name_or_id_pagination(&pag_params, scan_params)?; let opctx = crate::context::op_context_for_external_api(&rqctx).await?; let result = nexus - .bgp_announce_list(&opctx, &sel) + .bgp_announce_set_list(&opctx, &paginated_by) .await? .into_iter() .map(|p| p.into()) @@ -4100,17 +4106,17 @@ async fn networking_bgp_announce_set_list( /// Delete BGP announce set #[endpoint { method = DELETE, - path = "/v1/system/networking/bgp-announce", + path = "/v1/system/networking/bgp-announce-set/{name_or_id}", tags = ["system/networking"], }] async fn networking_bgp_announce_set_delete( rqctx: RequestContext, - selector: Query, + path_params: Path, ) -> Result { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.context.nexus; - let sel = selector.into_inner(); + let sel = path_params.into_inner(); let opctx = crate::context::op_context_for_external_api(&rqctx).await?; nexus.bgp_delete_announce_set(&opctx, &sel).await?; Ok(HttpResponseUpdatedNoContent {}) @@ -4122,6 +4128,40 @@ async fn networking_bgp_announce_set_delete( .await } +// TODO: is pagination necessary here? How large do we expect the list of +// announcements to become in real usage? +/// Get originated routes for a specified BGP announce set +#[endpoint { + method = GET, + path = "/v1/system/networking/bgp-announce-set/{name_or_id}/announcement", + tags = ["system/networking"], +}] +async fn networking_bgp_announcement_list( + rqctx: RequestContext, + path_params: Path, +) -> Result>, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.context.nexus; + let sel = path_params.into_inner(); + let opctx = crate::context::op_context_for_external_api(&rqctx).await?; + + let result = nexus + .bgp_announcement_list(&opctx, &sel) + .await? + .into_iter() + .map(|p| p.into()) + .collect(); + + Ok(HttpResponseOk(result)) + }; + apictx + .context + .external_latencies + .instrument_dropshot_handler(&rqctx, handler) + .await +} + /// Enable a BFD session #[endpoint { method = POST, diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 381d59e073..9703004c73 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -573,7 +573,7 @@ pub static DEMO_BGP_CONFIG: Lazy = shaper: None, }); pub const DEMO_BGP_ANNOUNCE_SET_URL: &'static str = - "/v1/system/networking/bgp-announce?name_or_id=a-bag-of-addrs"; + "/v1/system/networking/bgp-announce-set"; pub static DEMO_BGP_ANNOUNCE: Lazy = Lazy::new(|| params::BgpAnnounceSetCreate { identity: IdentityMetadataCreateParams { @@ -585,6 +585,10 @@ pub static DEMO_BGP_ANNOUNCE: Lazy = network: "10.0.0.0/16".parse().unwrap(), }], }); +pub const DEMO_BGP_ANNOUNCE_SET_DELETE_URL: &'static str = + "/v1/system/networking/bgp-announce-set/a-bag-of-addrs"; +pub const DEMO_BGP_ANNOUNCEMENT_URL: &'static str = + "/v1/system/networking/bgp-announce-set/a-bag-of-addrs/announcement"; pub const DEMO_BGP_STATUS_URL: &'static str = "/v1/system/networking/bgp-status"; pub const DEMO_BGP_EXPORTED_URL: &'static str = @@ -2274,6 +2278,7 @@ pub static VERIFY_ENDPOINTS: Lazy> = Lazy::new(|| { AllowedMethod::GetNonexistent ], }, + VerifyEndpoint { url: &DEMO_BGP_CONFIG_CREATE_URL, visibility: Visibility::Public, @@ -2295,11 +2300,28 @@ pub static VERIFY_ENDPOINTS: Lazy> = Lazy::new(|| { AllowedMethod::Put( serde_json::to_value(&*DEMO_BGP_ANNOUNCE).unwrap(), ), - AllowedMethod::GetNonexistent, + AllowedMethod::Get, + ], + }, + + VerifyEndpoint { + url: &DEMO_BGP_ANNOUNCE_SET_DELETE_URL, + visibility: Visibility::Public, + unprivileged_access: UnprivilegedAccess::None, + allowed_methods: vec![ AllowedMethod::Delete ], }, + VerifyEndpoint { + url: &DEMO_BGP_ANNOUNCEMENT_URL, + visibility: Visibility::Public, + unprivileged_access: UnprivilegedAccess::None, + allowed_methods: vec![ + AllowedMethod::GetNonexistent, + ], + }, + VerifyEndpoint { url: &DEMO_BGP_STATUS_URL, visibility: Visibility::Public, diff --git a/nexus/tests/integration_tests/switch_port.rs b/nexus/tests/integration_tests/switch_port.rs index e1d52e464a..92c44eddad 100644 --- a/nexus/tests/integration_tests/switch_port.rs +++ b/nexus/tests/integration_tests/switch_port.rs @@ -76,7 +76,7 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { NexusRequest::objects_post( client, - "/v1/system/networking/bgp-announce", + "/v1/system/networking/bgp-announce-set", &announce_set, ) .authn_as(AuthnMode::PrivilegedUser) diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index 053f56cf5c..bde11e2de3 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -178,9 +178,10 @@ networking_allow_list_view GET /v1/system/networking/allow-li networking_bfd_disable POST /v1/system/networking/bfd-disable networking_bfd_enable POST /v1/system/networking/bfd-enable networking_bfd_status GET /v1/system/networking/bfd-status -networking_bgp_announce_set_delete DELETE /v1/system/networking/bgp-announce -networking_bgp_announce_set_list GET /v1/system/networking/bgp-announce -networking_bgp_announce_set_update PUT /v1/system/networking/bgp-announce +networking_bgp_announce_set_delete DELETE /v1/system/networking/bgp-announce-set/{name_or_id} +networking_bgp_announce_set_list GET /v1/system/networking/bgp-announce-set +networking_bgp_announce_set_update PUT /v1/system/networking/bgp-announce-set +networking_bgp_announcement_list GET /v1/system/networking/bgp-announce-set/{name_or_id}/announcement networking_bgp_config_create POST /v1/system/networking/bgp networking_bgp_config_delete DELETE /v1/system/networking/bgp networking_bgp_config_list GET /v1/system/networking/bgp diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 82fa01121e..83897cbd1d 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -1629,6 +1629,13 @@ pub struct BgpAnnounceSetCreate { pub announcement: Vec, } +/// Optionally select a BGP announce set by a name or id. +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] +pub struct OptionalBgpAnnounceSetSelector { + /// A name or id to use when s electing BGP port settings + pub name_or_id: Option, +} + /// Select a BGP announce set by a name or id. #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] pub struct BgpAnnounceSetSelector { diff --git a/openapi/nexus.json b/openapi/nexus.json index 1e4113face..285dcd82bb 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -6533,22 +6533,48 @@ } } }, - "/v1/system/networking/bgp-announce": { + "/v1/system/networking/bgp-announce-set": { "get": { "tags": [ "system/networking" ], - "summary": "Get originated routes for a BGP configuration", + "summary": "List BGP announce sets", "operationId": "networking_bgp_announce_set_list", "parameters": [ + { + "in": "query", + "name": "limit", + "description": "Maximum number of items returned by a single call", + "schema": { + "nullable": true, + "type": "integer", + "format": "uint32", + "minimum": 1 + } + }, { "in": "query", "name": "name_or_id", - "description": "A name or id to use when selecting BGP port settings", - "required": true, + "description": "A name or id to use when s electing BGP port settings", "schema": { "$ref": "#/components/schemas/NameOrId" } + }, + { + "in": "query", + "name": "page_token", + "description": "Token returned by previous call to retrieve the subsequent page", + "schema": { + "nullable": true, + "type": "string" + } + }, + { + "in": "query", + "name": "sort_by", + "schema": { + "$ref": "#/components/schemas/NameOrIdSortMode" + } } ], "responses": { @@ -6557,10 +6583,10 @@ "content": { "application/json": { "schema": { - "title": "Array_of_BgpAnnouncement", + "title": "Array_of_BgpAnnounceSet", "type": "array", "items": { - "$ref": "#/components/schemas/BgpAnnouncement" + "$ref": "#/components/schemas/BgpAnnounceSet" } } } @@ -6572,6 +6598,9 @@ "5XX": { "$ref": "#/components/responses/Error" } + }, + "x-dropshot-pagination": { + "required": [] } }, "put": { @@ -6609,7 +6638,9 @@ "$ref": "#/components/responses/Error" } } - }, + } + }, + "/v1/system/networking/bgp-announce-set/{name_or_id}": { "delete": { "tags": [ "system/networking" @@ -6618,7 +6649,7 @@ "operationId": "networking_bgp_announce_set_delete", "parameters": [ { - "in": "query", + "in": "path", "name": "name_or_id", "description": "A name or id to use when selecting BGP port settings", "required": true, @@ -6640,6 +6671,48 @@ } } }, + "/v1/system/networking/bgp-announce-set/{name_or_id}/announcement": { + "get": { + "tags": [ + "system/networking" + ], + "summary": "Get originated routes for a specified BGP announce set", + "operationId": "networking_bgp_announcement_list", + "parameters": [ + { + "in": "path", + "name": "name_or_id", + "description": "A name or id to use when selecting BGP port settings", + "required": true, + "schema": { + "$ref": "#/components/schemas/NameOrId" + } + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "Array_of_BgpAnnouncement", + "type": "array", + "items": { + "$ref": "#/components/schemas/BgpAnnouncement" + } + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/v1/system/networking/bgp-exported": { "get": { "tags": [ diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 5e1ba048b6..baef38e44f 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -2781,6 +2781,10 @@ CREATE UNIQUE INDEX IF NOT EXISTS lookup_bgp_config_by_name ON omicron.public.bg ) WHERE time_deleted IS NULL; +CREATE INDEX IF NOT EXISTS lookup_bgp_config_by_asn ON omicron.public.bgp_config ( + asn +) WHERE time_deleted IS NULL; + CREATE TABLE IF NOT EXISTS omicron.public.bgp_announce_set ( id UUID PRIMARY KEY, name STRING(63) NOT NULL, @@ -4208,7 +4212,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '89.0.0', NULL) + (TRUE, NOW(), NOW(), '90.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/schema/crdb/lookup-bgp-config-by-asn/up01.sql b/schema/crdb/lookup-bgp-config-by-asn/up01.sql new file mode 100644 index 0000000000..e886015a29 --- /dev/null +++ b/schema/crdb/lookup-bgp-config-by-asn/up01.sql @@ -0,0 +1,3 @@ +CREATE INDEX IF NOT EXISTS lookup_bgp_config_by_asn ON omicron.public.bgp_config ( + asn +) WHERE time_deleted IS NULL;