From 57d0efcb8528cfc379b55b970478bff41ff78db2 Mon Sep 17 00:00:00 2001 From: Levon Tarver Date: Fri, 10 Jan 2025 19:27:52 +0000 Subject: [PATCH] add precondition to network configuration updates --- Cargo.lock | 1 + common/src/api/external/mod.rs | 9 + nexus/db-queries/Cargo.toml | 1 + .../src/db/datastore/switch_port.rs | 664 ++++++++++-------- nexus/external-api/src/lib.rs | 4 +- .../tasks/sync_switch_configuration.rs | 2 +- nexus/src/app/rack.rs | 1 + nexus/src/app/switch_port.rs | 21 +- nexus/src/external_api/http_entrypoints.rs | 23 +- nexus/tests/integration_tests/endpoints.rs | 1 + nexus/tests/integration_tests/switch_port.rs | 1 + nexus/types/src/external_api/params.rs | 21 +- openapi/nexus.json | 58 +- 13 files changed, 499 insertions(+), 308 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f5e44318bb..18fe2ef417 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5861,6 +5861,7 @@ dependencies = [ "serde", "serde_json", "serde_with", + "sha2", "sled-agent-client", "slog", "slog-error-chain", diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 38a9de0564..bf95d5b70b 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -2340,6 +2340,15 @@ pub struct SwitchPortSettings { pub identity: IdentityMetadata, } +#[derive(Clone, Debug, Deserialize, JsonSchema, Serialize, PartialEq)] +pub struct SwitchPortSettingsWithChecksum { + /// Value of response + pub value: SwitchPortSettingsView, + + /// Checksum of value to use for concurrency control + pub checksum: String, +} + /// 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. diff --git a/nexus/db-queries/Cargo.toml b/nexus/db-queries/Cargo.toml index db2b70488d..60c6458bc5 100644 --- a/nexus/db-queries/Cargo.toml +++ b/nexus/db-queries/Cargo.toml @@ -39,6 +39,7 @@ semver.workspace = true serde.workspace = true serde_json.workspace = true serde_with.workspace = true +sha2.workspace = true sled-agent-client.workspace = true slog.workspace = true slog-error-chain.workspace = true diff --git a/nexus/db-queries/src/db/datastore/switch_port.rs b/nexus/db-queries/src/db/datastore/switch_port.rs index 6b282434cd..c1b48b56ef 100644 --- a/nexus/db-queries/src/db/datastore/switch_port.rs +++ b/nexus/db-queries/src/db/datastore/switch_port.rs @@ -43,6 +43,7 @@ use omicron_common::api::external::{ }; use ref_cast::RefCast; use serde::{Deserialize, Serialize}; +use slog::error; use uuid::Uuid; #[derive(Clone, Debug, Serialize, Deserialize)] @@ -206,7 +207,7 @@ impl DataStore { opctx: &OpContext, params: ¶ms::SwitchPortSettingsCreate, id: Option, - ) -> CreateResult { + ) -> CreateResult<(SwitchPortSettingsCombinedResult, String)> { let err = OptionalError::new(); let conn = self.pool_connection_authorized(opctx).await?; @@ -235,6 +236,11 @@ impl DataStore { SwitchPortSettingsCreateError::ReserveBlock( ReserveBlockError::AddressNotInLot, ) => Error::invalid_request("address not in lot"), + SpsCreateError::JsonSerializationError => { + Error::internal_error( + "unable to serialize final configuration", + ) + } } } else { public_error_from_diesel( @@ -300,7 +306,8 @@ impl DataStore { opctx: &OpContext, params: ¶ms::SwitchPortSettingsCreate, id: Uuid, - ) -> UpdateResult { + ) -> UpdateResult<(SwitchPortSettingsCombinedResult, String)> { + let check_err = OptionalError::new(); let delete_err = OptionalError::new(); let create_err = OptionalError::new(); let conn = self.pool_connection_authorized(opctx).await?; @@ -309,17 +316,32 @@ impl DataStore { // Audit external networking database transaction usage self.transaction_retry_wrapper("switch_port_settings_update") .transaction(&conn, |conn| { + let check_err = check_err.clone(); let delete_err = delete_err.clone(); let create_err = create_err.clone(); let selector = NameOrId::Id(id); async move { + if let Some(expected_checksum) = params.precondition.clone() { + let (_, actual_checksum) = do_switch_port_settings_get(&conn, &selector, check_err.clone()).await?; + if expected_checksum != actual_checksum { + warn!( + &opctx.log, + "expected checksum: {expected_checksum}, found checksum: {actual_checksum}" + ); + + return Err(check_err.bail(Error::conflict("the precondition checksum does not match the existing configuration checksum"))); + } + } do_switch_port_settings_delete(&conn, &selector, delete_err).await?; do_switch_port_settings_create(&conn, Some(id), params, create_err).await } }) .await .map_err(|e| { - if let Some(err) = delete_err.take() { + if let Some(err) = check_err.take() { + err + } + else if let Some(err) = delete_err.take() { match err { SwitchPortSettingsDeleteError::SwitchPortSettingsNotFound => { Error::invalid_request("port settings not found") @@ -340,7 +362,11 @@ impl DataStore { SwitchPortSettingsCreateError::ReserveBlock( ReserveBlockError::AddressNotInLot, ) => Error::invalid_request("address not in lot"), - + SpsCreateError::JsonSerializationError => { + Error::internal_error( + "unable to serialize final configuration", + ) + } } } else { @@ -377,12 +403,7 @@ impl DataStore { &self, opctx: &OpContext, name_or_id: &NameOrId, - ) -> LookupResult { - #[derive(Debug)] - enum SwitchPortSettingsGetError { - NotFound(external::Name), - } - + ) -> LookupResult<(SwitchPortSettingsCombinedResult, String)> { let err = OptionalError::new(); let conn = self.pool_connection_authorized(opctx).await?; @@ -392,271 +413,20 @@ impl DataStore { .transaction(&conn, |conn| { let err = err.clone(); async move { - // get the top level port settings object - use db::schema::switch_port_settings::{ - self, dsl as port_settings_dsl, - }; - use db::schema::{ - switch_port_settings_bgp_peer_config_allow_import::dsl as allow_import_dsl, - switch_port_settings_bgp_peer_config_allow_export::dsl as allow_export_dsl, - switch_port_settings_bgp_peer_config_communities::dsl as bgp_communities_dsl, - }; - - let id = match name_or_id { - NameOrId::Id(id) => *id, - NameOrId::Name(name) => { - let name_str = name.to_string(); - port_settings_dsl::switch_port_settings - .filter(switch_port_settings::time_deleted.is_null()) - .filter(switch_port_settings::name.eq(name_str)) - .select(switch_port_settings::id) - .limit(1) - .first_async::(&conn) - .await - .map_err(|diesel_error| { - err.bail_retryable_or_else(diesel_error, |_| { - SwitchPortSettingsGetError::NotFound( - name.clone(), - ) - }) - })? - } - }; - - let settings: SwitchPortSettings = - port_settings_dsl::switch_port_settings - .filter(switch_port_settings::time_deleted.is_null()) - .filter(switch_port_settings::id.eq(id)) - .select(SwitchPortSettings::as_select()) - .limit(1) - .first_async::(&conn) - .await?; - - // get the port config - use db::schema::switch_port_settings_port_config::{ - self as port_config, dsl as port_config_dsl, - }; - let port: SwitchPortConfig = - port_config_dsl::switch_port_settings_port_config - .filter(port_config::port_settings_id.eq(id)) - .select(SwitchPortConfig::as_select()) - .limit(1) - .first_async::(&conn) - .await?; - - // initialize result - let mut result = - SwitchPortSettingsCombinedResult::new(settings, port); - - // get the link configs - use db::schema::switch_port_settings_link_config::{ - self as link_config, dsl as link_config_dsl, - }; - - result.links = link_config_dsl::switch_port_settings_link_config - .filter(link_config::port_settings_id.eq(id)) - .select(SwitchPortLinkConfig::as_select()) - .load_async::(&conn) - .await?; - - let lldp_link_ids: Vec = result - .links - .iter() - .filter_map(|link| link.lldp_link_config_id) - .collect(); - - use db::schema::lldp_link_config; - result.link_lldp = lldp_link_config::dsl::lldp_link_config - .filter(lldp_link_config::id.eq_any(lldp_link_ids)) - .select(LldpLinkConfig::as_select()) - .limit(1) - .load_async::(&conn) - .await?; - - let tx_eq_ids_and_nulls :Vec>= result - .links - .iter() - .map(|link| link.tx_eq_config_id) - .collect(); - let tx_eq_ids: Vec = tx_eq_ids_and_nulls - .iter() - .cloned() - .flatten() - .collect(); - - use db::schema::tx_eq_config; - let configs = tx_eq_config::dsl::tx_eq_config - .filter(tx_eq_config::id.eq_any(tx_eq_ids)) - .select(TxEqConfig::as_select()) - .limit(1) - .load_async::(&conn) - .await?; - result.tx_eq = tx_eq_ids_and_nulls.iter().map(|x| - if let Some(id) = x { - configs.iter().find(|c| c.id == *id).cloned() - } else { - None - }).collect(); - - // get the interface configs - use db::schema::switch_port_settings_interface_config::{ - self as interface_config, dsl as interface_config_dsl, - }; - - result.interfaces = - interface_config_dsl::switch_port_settings_interface_config - .filter(interface_config::port_settings_id.eq(id)) - .select(SwitchInterfaceConfig::as_select()) - .load_async::(&conn) - .await?; - - use db::schema::switch_vlan_interface_config as vlan_config; - use db::schema::switch_vlan_interface_config::dsl as vlan_dsl; - let interface_ids: Vec = result - .interfaces - .iter() - .map(|interface| interface.id) - .collect(); - - result.vlan_interfaces = vlan_dsl::switch_vlan_interface_config - .filter(vlan_config::interface_config_id.eq_any(interface_ids)) - .select(SwitchVlanInterfaceConfig::as_select()) - .load_async::(&conn) - .await?; - - // get the route configs - use db::schema::switch_port_settings_route_config::{ - self as route_config, dsl as route_config_dsl, - }; - - result.routes = route_config_dsl::switch_port_settings_route_config - .filter(route_config::port_settings_id.eq(id)) - .select(SwitchPortRouteConfig::as_select()) - .load_async::(&conn) - .await?; - - // get the bgp peer configs - use db::schema::switch_port_settings_bgp_peer_config::{ - self as bgp_peer, dsl as bgp_peer_dsl, - }; - - let peers: Vec = - bgp_peer_dsl::switch_port_settings_bgp_peer_config - .filter(bgp_peer::port_settings_id.eq(id)) - .select(SwitchPortBgpPeerConfig::as_select()) - .load_async::(&conn) - .await?; - - for p in peers.iter() { - let allowed_import: ImportExportPolicy = if p.allow_import_list_active { - let db_list: Vec = - allow_import_dsl::switch_port_settings_bgp_peer_config_allow_import - .filter(allow_import_dsl::port_settings_id.eq(id)) - .filter(allow_import_dsl::interface_name.eq(p.interface_name.clone())) - .filter(allow_import_dsl::addr.eq(p.addr)) - .select(SwitchPortBgpPeerConfigAllowImport::as_select()) - .load_async::(&conn) - .await?; - - ImportExportPolicy::Allow(db_list - .into_iter() - .map(|x| x.prefix.into()) - .collect() - ) - } else { - ImportExportPolicy::NoFiltering - }; - - let allowed_export: ImportExportPolicy = if p.allow_export_list_active { - let db_list: Vec = - allow_export_dsl::switch_port_settings_bgp_peer_config_allow_export - .filter(allow_export_dsl::port_settings_id.eq(id)) - .filter(allow_export_dsl::interface_name.eq(p.interface_name.clone())) - .filter(allow_export_dsl::addr.eq(p.addr)) - .select(SwitchPortBgpPeerConfigAllowExport::as_select()) - .load_async::(&conn) - .await?; - - ImportExportPolicy::Allow(db_list - .into_iter() - .map(|x| x.prefix.into()) - .collect() - ) - } else { - ImportExportPolicy::NoFiltering - }; - - let communities: Vec = - bgp_communities_dsl::switch_port_settings_bgp_peer_config_communities - .filter(bgp_communities_dsl::port_settings_id.eq(id)) - .filter(bgp_communities_dsl::interface_name.eq(p.interface_name.clone())) - .filter(bgp_communities_dsl::addr.eq(p.addr)) - .select(SwitchPortBgpPeerConfigCommunity::as_select()) - .load_async::(&conn) - .await?; - - let view = BgpPeerConfig { - port_settings_id: p.port_settings_id, - bgp_config_id: p.bgp_config_id, - interface_name: p.interface_name.clone(), - addr: p.addr, - hold_time: p.hold_time, - idle_hold_time: p.idle_hold_time, - delay_open: p.delay_open, - connect_retry: p.connect_retry, - keepalive: p.keepalive, - remote_asn: p.remote_asn, - min_ttl: p.min_ttl, - md5_auth_key: p.md5_auth_key.clone(), - multi_exit_discriminator: p.multi_exit_discriminator, - local_pref: p.local_pref, - enforce_first_as: p.enforce_first_as, - vlan_id: p.vlan_id, - communities: communities.into_iter().map(|c| c.community.0).collect(), - allowed_import, - allowed_export, - }; - - result.bgp_peers.push(view); + do_switch_port_settings_get(&conn, name_or_id, err).await } - - // get the address configs - use db::schema::switch_port_settings_address_config::{ - self as address_config, dsl as address_config_dsl, - }; - - result.addresses = - address_config_dsl::switch_port_settings_address_config - .filter(address_config::port_settings_id.eq(id)) - .select(SwitchPortAddressConfig::as_select()) - .load_async::(&conn) - .await?; - - Ok(result) - } - }) - .await - .map_err(|e| { - if let Some(err) = err.take() { - match err { - SwitchPortSettingsGetError::NotFound(name) => { - Error::not_found_by_name( - ResourceType::SwitchPortSettings, - &name, - ) - } + }) + .await + .map_err(|e| { + let msg = "switch_port_settings_get 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) } - } else { - let name = name_or_id.to_string(); - public_error_from_diesel( - e, - ErrorHandler::Conflict( - ResourceType::SwitchPortSettings, - &name, - ), - ) - } - }) + }) } // switch ports @@ -859,7 +629,9 @@ impl DataStore { opctx: &OpContext, switch_port_id: Uuid, port_settings_id: Option, - current: UpdatePrecondition, + precondition: UpdatePrecondition< + params::SwitchPortApplySettingsChecksums, + >, ) -> UpdateResult<()> { use db::schema::bgp_config::dsl as bgp_config_dsl; use db::schema::switch_port; @@ -872,6 +644,7 @@ impl DataStore { self.transaction_retry_wrapper("switch_port_set_settings_id") .transaction(&conn, |conn| { let err = err.clone(); + let precondition = precondition.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 @@ -921,13 +694,13 @@ impl DataStore { let msg = "failed to check if bgp peer exists in switch port settings"; match e { diesel::result::Error::NotFound => { - debug!(opctx.log, "{msg}"; "error" => ?e); + debug!(opctx.log, "{msg}"; "error" => ?e); Ok(None) }, _ => { - error!(opctx.log, "{msg}"; "error" => ?e); - Err(err.bail(Error::internal_error(msg))) - } + error!(opctx.log, "{msg}"; "error" => ?e); + Err(err.bail(Error::internal_error(msg))) + } } } }?; @@ -966,7 +739,7 @@ impl DataStore { } // perform the requested update - match current { + match precondition { UpdatePrecondition::DontCare => { diesel::update(switch_port_dsl::switch_port) .filter(switch_port::id.eq(switch_port_id)) @@ -988,13 +761,71 @@ impl DataStore { .execute_async(&conn) .await } - UpdatePrecondition::Value(current_id) => { + UpdatePrecondition::Value(checksums) => { + // get active switch port settings id + let port: SwitchPort = switch_port::table + .filter(switch_port::id.eq(switch_port_id)) + .select(SwitchPort::as_select()) + .get_result_async(&conn) + .await?; + + match (checksums.current_settings_checksum, port.port_settings_id) { + // Caller does not expect any port settings to be applied at this time, + // and there are no port settings applied at this time. OK. + (None, None) => { + // Do nothing + }, + // Caller does not expect any port settings to be applied at this time, + // and there are port settings applied at this time. Not OK. + (None, Some(_)) => { + // Return some form of error (what is the correct error to return when + // a precondition fails?) + return Err(err.bail(Error::conflict("a checksum was not provided but there is a configuration active on this port"))); + }, + // Caller expects port settings to be applied, but there are no + // port settings applied at this time. Not OK. + (Some(_), None) => { + // Return some form of error (what is the correct error to return when + // a precondition fails?) + return Err(err.bail(Error::conflict("a checksum was provided but there is not an active configuration on this port"))); + }, + // Caller expects port settings to be applied at this time, + // and there are port settings applied at this time. OK. + // fetch active port settings, perform checksum and ensure they match + (Some(expected_checksum), Some(active_psid)) => { + // get active switch port settings combined result + let (_, actual_checksum) = do_switch_port_settings_get(&conn, &NameOrId::Id(active_psid), err.clone()).await?; + + // verify checksum of current port settings + if actual_checksum != expected_checksum { + warn!( + &opctx.log, + "expected checksum: {expected_checksum}, found checksum: {actual_checksum}" + ); + + return Err(err.bail(Error::conflict("the active configuration does not match the provided checksum"))); + } + }, + } + + if let Some(desired_psid) = port_settings_id { + // get desired switch port settings combined result + let (_, actual_checksum)= do_switch_port_settings_get(&conn, &NameOrId::Id(desired_psid), err.clone()).await?; + let expected_checksum = checksums.new_settings_checksum; + + if actual_checksum != expected_checksum { + warn!( + &opctx.log, + "expected checksum: {expected_checksum}, found checksum: {actual_checksum}" + ); + + return Err(err.bail(Error::conflict("the desired configuration does not match the provided checksum"))); + } + + } + 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), @@ -1124,6 +955,7 @@ enum SwitchPortSettingsCreateError { AddressLotNotFound, BgpConfigNotFound, ReserveBlock(ReserveBlockError), + JsonSerializationError, } type SpsCreateError = SwitchPortSettingsCreateError; @@ -1132,7 +964,7 @@ async fn do_switch_port_settings_create( id: Option, params: ¶ms::SwitchPortSettingsCreate, err: OptionalError, -) -> Result { +) -> Result<(SwitchPortSettingsCombinedResult, String), diesel::result::Error> { use db::schema::{ address_lot::dsl as address_lot_dsl, bgp_config::dsl as bgp_config_dsl, lldp_link_config::dsl as lldp_link_config_dsl, @@ -1511,7 +1343,10 @@ async fn do_switch_port_settings_create( .get_results_async(conn) .await?; - Ok(result) + let result_json = serde_json::to_string(&result).map_err(|_| { + err.bail(SwitchPortSettingsCreateError::JsonSerializationError) + })?; + Ok((result, sha256sum(&result_json))) } #[derive(Debug)] @@ -1689,6 +1524,259 @@ async fn do_switch_port_settings_delete( Ok(()) } +async fn do_switch_port_settings_get( + conn: &Connection>, + name_or_id: &NameOrId, + err: OptionalError, +) -> Result<(SwitchPortSettingsCombinedResult, String), diesel::result::Error> { + // get the top level port settings object + use db::schema::switch_port_settings::{self, dsl as port_settings_dsl}; + use db::schema::{ + switch_port_settings_bgp_peer_config_allow_export::dsl as allow_export_dsl, + switch_port_settings_bgp_peer_config_allow_import::dsl as allow_import_dsl, + switch_port_settings_bgp_peer_config_communities::dsl as bgp_communities_dsl, + }; + + let id = match name_or_id { + NameOrId::Id(id) => *id, + NameOrId::Name(name) => { + let name_str = name.to_string(); + port_settings_dsl::switch_port_settings + .filter(switch_port_settings::time_deleted.is_null()) + .filter(switch_port_settings::name.eq(name_str)) + .select(switch_port_settings::id) + .limit(1) + .first_async::(conn) + .await + .map_err(|diesel_error| { + err.bail_retryable_or_else(diesel_error, |_| { + Error::not_found_by_name( + ResourceType::SwitchPortSettings, + name, + ) + }) + })? + } + }; + + let settings: SwitchPortSettings = port_settings_dsl::switch_port_settings + .filter(switch_port_settings::time_deleted.is_null()) + .filter(switch_port_settings::id.eq(id)) + .select(SwitchPortSettings::as_select()) + .limit(1) + .first_async::(conn) + .await?; + + // get the port config + use db::schema::switch_port_settings_port_config::{ + self as port_config, dsl as port_config_dsl, + }; + let port: SwitchPortConfig = + port_config_dsl::switch_port_settings_port_config + .filter(port_config::port_settings_id.eq(id)) + .select(SwitchPortConfig::as_select()) + .limit(1) + .first_async::(conn) + .await?; + + // initialize result + let mut result = SwitchPortSettingsCombinedResult::new(settings, port); + + // get the link configs + use db::schema::switch_port_settings_link_config::{ + self as link_config, dsl as link_config_dsl, + }; + + result.links = link_config_dsl::switch_port_settings_link_config + .filter(link_config::port_settings_id.eq(id)) + .select(SwitchPortLinkConfig::as_select()) + .load_async::(conn) + .await?; + + let lldp_link_ids: Vec = result + .links + .iter() + .filter_map(|link| link.lldp_link_config_id) + .collect(); + + use db::schema::lldp_link_config; + result.link_lldp = lldp_link_config::dsl::lldp_link_config + .filter(lldp_link_config::id.eq_any(lldp_link_ids)) + .select(LldpLinkConfig::as_select()) + .limit(1) + .load_async::(conn) + .await?; + + let tx_eq_ids_and_nulls: Vec> = + result.links.iter().map(|link| link.tx_eq_config_id).collect(); + let tx_eq_ids: Vec = + tx_eq_ids_and_nulls.iter().cloned().flatten().collect(); + + use db::schema::tx_eq_config; + let configs = tx_eq_config::dsl::tx_eq_config + .filter(tx_eq_config::id.eq_any(tx_eq_ids)) + .select(TxEqConfig::as_select()) + .limit(1) + .load_async::(conn) + .await?; + result.tx_eq = tx_eq_ids_and_nulls + .iter() + .map(|x| { + if let Some(id) = x { + configs.iter().find(|c| c.id == *id).cloned() + } else { + None + } + }) + .collect(); + + // get the interface configs + use db::schema::switch_port_settings_interface_config::{ + self as interface_config, dsl as interface_config_dsl, + }; + + result.interfaces = + interface_config_dsl::switch_port_settings_interface_config + .filter(interface_config::port_settings_id.eq(id)) + .select(SwitchInterfaceConfig::as_select()) + .load_async::(conn) + .await?; + + use db::schema::switch_vlan_interface_config as vlan_config; + use db::schema::switch_vlan_interface_config::dsl as vlan_dsl; + let interface_ids: Vec = + result.interfaces.iter().map(|interface| interface.id).collect(); + + result.vlan_interfaces = vlan_dsl::switch_vlan_interface_config + .filter(vlan_config::interface_config_id.eq_any(interface_ids)) + .select(SwitchVlanInterfaceConfig::as_select()) + .load_async::(conn) + .await?; + + // get the route configs + use db::schema::switch_port_settings_route_config::{ + self as route_config, dsl as route_config_dsl, + }; + + result.routes = route_config_dsl::switch_port_settings_route_config + .filter(route_config::port_settings_id.eq(id)) + .select(SwitchPortRouteConfig::as_select()) + .load_async::(conn) + .await?; + + // get the bgp peer configs + use db::schema::switch_port_settings_bgp_peer_config::{ + self as bgp_peer, dsl as bgp_peer_dsl, + }; + + let peers: Vec = + bgp_peer_dsl::switch_port_settings_bgp_peer_config + .filter(bgp_peer::port_settings_id.eq(id)) + .select(SwitchPortBgpPeerConfig::as_select()) + .load_async::(conn) + .await?; + + for p in peers.iter() { + let allowed_import: ImportExportPolicy = if p.allow_import_list_active { + let db_list: Vec = + allow_import_dsl::switch_port_settings_bgp_peer_config_allow_import + .filter(allow_import_dsl::port_settings_id.eq(id)) + .filter(allow_import_dsl::interface_name.eq(p.interface_name.clone())) + .filter(allow_import_dsl::addr.eq(p.addr)) + .select(SwitchPortBgpPeerConfigAllowImport::as_select()) + .load_async::(conn) + .await?; + + ImportExportPolicy::Allow( + db_list.into_iter().map(|x| x.prefix.into()).collect(), + ) + } else { + ImportExportPolicy::NoFiltering + }; + + let allowed_export: ImportExportPolicy = if p.allow_export_list_active { + let db_list: Vec = + allow_export_dsl::switch_port_settings_bgp_peer_config_allow_export + .filter(allow_export_dsl::port_settings_id.eq(id)) + .filter(allow_export_dsl::interface_name.eq(p.interface_name.clone())) + .filter(allow_export_dsl::addr.eq(p.addr)) + .select(SwitchPortBgpPeerConfigAllowExport::as_select()) + .load_async::(conn) + .await?; + + ImportExportPolicy::Allow( + db_list.into_iter().map(|x| x.prefix.into()).collect(), + ) + } else { + ImportExportPolicy::NoFiltering + }; + + let communities: Vec = + bgp_communities_dsl::switch_port_settings_bgp_peer_config_communities + .filter(bgp_communities_dsl::port_settings_id.eq(id)) + .filter(bgp_communities_dsl::interface_name.eq(p.interface_name.clone())) + .filter(bgp_communities_dsl::addr.eq(p.addr)) + .select(SwitchPortBgpPeerConfigCommunity::as_select()) + .load_async::(conn) + .await?; + + let view = BgpPeerConfig { + port_settings_id: p.port_settings_id, + bgp_config_id: p.bgp_config_id, + interface_name: p.interface_name.clone(), + addr: p.addr, + hold_time: p.hold_time, + idle_hold_time: p.idle_hold_time, + delay_open: p.delay_open, + connect_retry: p.connect_retry, + keepalive: p.keepalive, + remote_asn: p.remote_asn, + min_ttl: p.min_ttl, + md5_auth_key: p.md5_auth_key.clone(), + multi_exit_discriminator: p.multi_exit_discriminator, + local_pref: p.local_pref, + enforce_first_as: p.enforce_first_as, + vlan_id: p.vlan_id, + communities: communities + .into_iter() + .map(|c| c.community.0) + .collect(), + allowed_import, + allowed_export, + }; + + result.bgp_peers.push(view); + } + + // get the address configs + use db::schema::switch_port_settings_address_config::{ + self as address_config, dsl as address_config_dsl, + }; + + result.addresses = address_config_dsl::switch_port_settings_address_config + .filter(address_config::port_settings_id.eq(id)) + .select(SwitchPortAddressConfig::as_select()) + .load_async::(conn) + .await?; + + let json_result = serde_json::to_string(&result).map_err(|e| { + err.bail(Error::internal_error(&format!( + "unable to serialize stored configuration as json: {e:?}" + ))) + })?; + + Ok((result, sha256sum(&json_result))) +} + +fn sha256sum(data: &str) -> String { + use sha2::{Digest, Sha256}; + let mut hasher = Sha256::new(); + let bytes = data.as_bytes(); + hasher.update(bytes); + let hashed_config = hasher.finalize(); + format!("{hashed_config:x}") +} + #[cfg(test)] mod test { use crate::db::datastore::UpdatePrecondition; @@ -1748,6 +1836,7 @@ mod test { datastore.bgp_config_create(&opctx, &bgp_config).await.unwrap(); let settings = SwitchPortSettingsCreate { + precondition: None, identity: IdentityMetadataCreateParams { name: "test-settings".parse().unwrap(), description: "test settings".into(), @@ -1792,7 +1881,8 @@ mod test { let settings_result = datastore .switch_port_settings_create(&opctx, &settings, None) .await - .unwrap(); + .unwrap() + .0; datastore .switch_port_set_settings_id( diff --git a/nexus/external-api/src/lib.rs b/nexus/external-api/src/lib.rs index e2b53a7e6f..c33b772fcc 100644 --- a/nexus/external-api/src/lib.rs +++ b/nexus/external-api/src/lib.rs @@ -1393,7 +1393,7 @@ pub trait NexusExternalApi { async fn networking_switch_port_settings_create( rqctx: RequestContext, new_settings: TypedBody, - ) -> Result, HttpError>; + ) -> Result, HttpError>; /// Delete switch port settings #[endpoint { @@ -1428,7 +1428,7 @@ pub trait NexusExternalApi { async fn networking_switch_port_settings_view( rqctx: RequestContext, path_params: Path, - ) -> Result, HttpError>; + ) -> Result, HttpError>; /// List switch ports #[endpoint { diff --git a/nexus/src/app/background/tasks/sync_switch_configuration.rs b/nexus/src/app/background/tasks/sync_switch_configuration.rs index 6a82f1634c..67da3807d6 100644 --- a/nexus/src/app/background/tasks/sync_switch_configuration.rs +++ b/nexus/src/app/background/tasks/sync_switch_configuration.rs @@ -182,7 +182,7 @@ impl SwitchPortSettingsManager { changes.push(( location, port, - PortSettingsChange::Apply(Box::new(settings)), + PortSettingsChange::Apply(Box::new(settings.0)), )); } Ok(changes) diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index a9ad97e9c8..67c6fc98e8 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -562,6 +562,7 @@ impl super::Nexus { }; let mut port_settings_params = SwitchPortSettingsCreate { + precondition: None, identity, port_config, groups: vec![], diff --git a/nexus/src/app/switch_port.rs b/nexus/src/app/switch_port.rs index b616531f53..8673197fc1 100644 --- a/nexus/src/app/switch_port.rs +++ b/nexus/src/app/switch_port.rs @@ -28,7 +28,7 @@ impl super::Nexus { self: &Arc, opctx: &OpContext, params: params::SwitchPortSettingsCreate, - ) -> CreateResult { + ) -> CreateResult<(SwitchPortSettingsCombinedResult, String)> { opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; Self::switch_port_settings_validate(¶ms)?; @@ -90,7 +90,7 @@ impl super::Nexus { opctx: &OpContext, params: params::SwitchPortSettingsCreate, id: Option, - ) -> CreateResult { + ) -> CreateResult<(SwitchPortSettingsCombinedResult, String)> { self.db_datastore.switch_port_settings_create(opctx, ¶ms, id).await } @@ -99,7 +99,7 @@ impl super::Nexus { opctx: &OpContext, switch_port_settings_id: Uuid, new_settings: params::SwitchPortSettingsCreate, - ) -> CreateResult { + ) -> CreateResult<(SwitchPortSettingsCombinedResult, String)> { let result = self .db_datastore .switch_port_settings_update( @@ -153,7 +153,7 @@ impl super::Nexus { &self, opctx: &OpContext, name_or_id: &NameOrId, - ) -> LookupResult { + ) -> LookupResult<(SwitchPortSettingsCombinedResult, String)> { opctx.authorize(authz::Action::Read, &authz::FLEET).await?; self.db_datastore.switch_port_settings_get(opctx, name_or_id).await } @@ -189,7 +189,9 @@ impl super::Nexus { opctx: &OpContext, switch_port_id: Uuid, port_settings_id: Option, - current_id: UpdatePrecondition, + precondition: UpdatePrecondition< + params::SwitchPortApplySettingsChecksums, + >, ) -> UpdateResult<()> { opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; self.db_datastore @@ -197,7 +199,7 @@ impl super::Nexus { opctx, switch_port_id, port_settings_id, - current_id, + precondition, ) .await } @@ -229,11 +231,16 @@ impl super::Nexus { } }; + let precondition = match settings.precondition.clone() { + Some(v) => UpdatePrecondition::Value(v), + None => UpdatePrecondition::Null, + }; + self.set_switch_port_settings_id( &opctx, switch_port_id, Some(switch_port_settings_id), - UpdatePrecondition::DontCare, + precondition, ) .await?; diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 740895b7e4..d5eb7b9119 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -48,7 +48,6 @@ use nexus_types::{ shared::{BfdStatus, ProbeInfo}, }, }; -use omicron_common::api::external::http_pagination::data_page_params_for; use omicron_common::api::external::http_pagination::marker_for_name; use omicron_common::api::external::http_pagination::marker_for_name_or_id; use omicron_common::api::external::http_pagination::name_or_id_pagination; @@ -88,6 +87,9 @@ use omicron_common::api::external::TufRepoGetResponse; use omicron_common::api::external::TufRepoInsertResponse; use omicron_common::api::external::VpcFirewallRuleUpdateParams; use omicron_common::api::external::VpcFirewallRules; +use omicron_common::api::external::{ + http_pagination::data_page_params_for, SwitchPortSettingsWithChecksum, +}; use omicron_common::bail_unless; use omicron_uuid_kinds::GenericUuid; use propolis_client::support::tungstenite::protocol::frame::coding::CloseCode; @@ -2805,18 +2807,22 @@ impl NexusExternalApi for NexusExternalApiImpl { async fn networking_switch_port_settings_create( rqctx: RequestContext, new_settings: TypedBody, - ) -> Result, HttpError> { + ) -> Result, HttpError> + { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.context.nexus; let params = new_settings.into_inner(); let opctx = crate::context::op_context_for_external_api(&rqctx).await?; - let result = + let (result, checksum) = nexus.switch_port_settings_post(&opctx, params).await?; let settings: SwitchPortSettingsView = result.into(); - Ok(HttpResponseCreated(settings)) + Ok(HttpResponseCreated(SwitchPortSettingsWithChecksum { + value: settings, + checksum, + })) }; apictx .context @@ -2884,16 +2890,19 @@ impl NexusExternalApi for NexusExternalApiImpl { async fn networking_switch_port_settings_view( rqctx: RequestContext, path_params: Path, - ) -> Result, HttpError> { + ) -> Result, HttpError> { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.context.nexus; let query = path_params.into_inner().port; let opctx = crate::context::op_context_for_external_api(&rqctx).await?; - let settings = + let (settings, checksum) = nexus.switch_port_settings_get(&opctx, &query).await?; - Ok(HttpResponseOk(settings.into())) + Ok(HttpResponseOk(SwitchPortSettingsWithChecksum { + value: settings.into(), + checksum, + })) }; apictx .context diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 466cae17a8..48df21763d 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -578,6 +578,7 @@ pub static DEMO_SWITCH_PORT_SETTINGS_APPLY_URL: Lazy = Lazy::new( ); pub static DEMO_SWITCH_PORT_SETTINGS: Lazy = Lazy::new(|| params::SwitchPortApplySettings { + precondition: None, port_settings: NameOrId::Name("portofino".parse().unwrap()), }); /* TODO requires dpd access diff --git a/nexus/tests/integration_tests/switch_port.rs b/nexus/tests/integration_tests/switch_port.rs index 89ddfe11ac..c98c8e9aaa 100644 --- a/nexus/tests/integration_tests/switch_port.rs +++ b/nexus/tests/integration_tests/switch_port.rs @@ -335,6 +335,7 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { // apply port settings let apply_settings = SwitchPortApplySettings { + precondition: None, port_settings: NameOrId::Name("portofino".parse().unwrap()), }; diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 9b4c2474ad..fece196172 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -1671,6 +1671,8 @@ pub struct SwitchPortSettingsCreate { pub bgp_peers: HashMap, /// Addresses indexed by interface name. pub addresses: HashMap, + /// Optional checksum to provide for detecting conflicting concurrent updates + pub precondition: Option, } impl SwitchPortSettingsCreate { @@ -1686,6 +1688,7 @@ impl SwitchPortSettingsCreate { routes: HashMap::new(), bgp_peers: HashMap::new(), addresses: HashMap::new(), + precondition: None, } } } @@ -2006,10 +2009,26 @@ pub struct SwitchPortPageSelector { /// Parameters for applying settings to switch ports. #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] pub struct SwitchPortApplySettings { - /// A name or id to use when applying switch port settings. + pub precondition: Option, + + /// Name or ID of the desired configuration pub port_settings: NameOrId, } +/// Parameters for applying a precondition to SwitchPortApplySettings +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] +pub struct SwitchPortApplySettingsChecksums { + /// md5 checksum of current SwitchPortApplySettings body + /// This field protects against concurrent modification of `SwitchPortApplySettings` + /// Set to `None` if you expect there to not be any settings currently applied. + pub current_settings_checksum: Option, + + /// md5 checksum of the desired configuration body + /// This field ensures that the port settings you are applying have not been modified + /// since you last viewed them + pub new_settings_checksum: String, +} + // IMAGES /// The source of the underlying image. diff --git a/openapi/nexus.json b/openapi/nexus.json index c0b6a96fcf..e2a341db63 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -7797,7 +7797,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/SwitchPortSettingsView" + "$ref": "#/components/schemas/SwitchPortSettingsWithChecksum" } } } @@ -7863,7 +7863,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/SwitchPortSettingsView" + "$ref": "#/components/schemas/SwitchPortSettingsWithChecksum" } } } @@ -20366,18 +20366,44 @@ "type": "object", "properties": { "port_settings": { - "description": "A name or id to use when applying switch port settings.", + "description": "Name or ID of the desired configuration", "allOf": [ { "$ref": "#/components/schemas/NameOrId" } ] + }, + "precondition": { + "nullable": true, + "allOf": [ + { + "$ref": "#/components/schemas/SwitchPortApplySettingsChecksums" + } + ] } }, "required": [ "port_settings" ] }, + "SwitchPortApplySettingsChecksums": { + "description": "Parameters for applying a precondition to SwitchPortApplySettings", + "type": "object", + "properties": { + "current_settings_checksum": { + "nullable": true, + "description": "md5 checksum of current SwitchPortApplySettings body This field protects against concurrent modification of `SwitchPortApplySettings` Set to `None` if you expect there to not be any settings currently applied.", + "type": "string" + }, + "new_settings_checksum": { + "description": "md5 checksum of the desired configuration body This field ensures that the port settings you are applying have not been modified since you last viewed them", + "type": "string" + } + }, + "required": [ + "new_settings_checksum" + ] + }, "SwitchPortConfig": { "description": "A physical port configuration for a port settings object.", "type": "object", @@ -20690,6 +20716,11 @@ "port_config": { "$ref": "#/components/schemas/SwitchPortConfigCreate" }, + "precondition": { + "nullable": true, + "description": "Optional checksum to provide for detecting conflicting concurrent updates", + "type": "string" + }, "routes": { "description": "Routes indexed by interface name.", "type": "object", @@ -20854,6 +20885,27 @@ "vlan_interfaces" ] }, + "SwitchPortSettingsWithChecksum": { + "type": "object", + "properties": { + "checksum": { + "description": "Checksum of value to use for concurrency control", + "type": "string" + }, + "value": { + "description": "Value of response", + "allOf": [ + { + "$ref": "#/components/schemas/SwitchPortSettingsView" + } + ] + } + }, + "required": [ + "checksum", + "value" + ] + }, "SwitchResultsPage": { "description": "A single page of results", "type": "object",