From 324c2db10a1e9ad634556808528dd4b1e208f8b1 Mon Sep 17 00:00:00 2001 From: Ryan Goodfellow Date: Fri, 13 Oct 2023 20:58:04 -0700 Subject: [PATCH] bgp-config as announce-set owner instead of peers --- common/src/api/external/mod.rs | 3 - nexus/db-model/src/bgp.rs | 11 ++- nexus/db-model/src/schema.rs | 2 +- nexus/db-model/src/switch_port.rs | 15 +--- nexus/db-queries/src/db/datastore/bgp.rs | 70 ++++++++++++------- .../src/db/datastore/switch_port.rs | 10 ++- .../app/sagas/switch_port_settings_apply.rs | 4 +- nexus/src/app/switch_port.rs | 2 +- nexus/tests/integration_tests/endpoints.rs | 1 + nexus/tests/integration_tests/switch_port.rs | 37 +++++----- nexus/types/src/external_api/params.rs | 2 + openapi/nexus.json | 10 ++- schema/crdb/8.0.0/{up.sql => up1.sql} | 0 schema/crdb/8.0.0/up2.sql | 2 + schema/crdb/dbinit.sql | 4 +- 15 files changed, 91 insertions(+), 82 deletions(-) rename schema/crdb/8.0.0/{up.sql => up1.sql} (100%) create mode 100644 schema/crdb/8.0.0/up2.sql diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index ef6faa0e55..fcea57220d 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -2469,9 +2469,6 @@ pub struct SwitchPortBgpPeerConfig { /// The port settings object this BGP configuration belongs to. pub port_settings_id: Uuid, - /// The id for the set of prefixes announced in this peer configuration. - pub bgp_announce_set_id: Uuid, - /// The id of the global BGP configuration referenced by this peer /// configuration. pub bgp_config_id: Uuid, diff --git a/nexus/db-model/src/bgp.rs b/nexus/db-model/src/bgp.rs index cd0932beb8..cc9ebfb4f5 100644 --- a/nexus/db-model/src/bgp.rs +++ b/nexus/db-model/src/bgp.rs @@ -28,6 +28,7 @@ pub struct BgpConfig { #[diesel(embed)] pub identity: BgpConfigIdentity, pub asn: SqlU32, + pub bgp_announce_set_id: Uuid, pub vrf: Option, } @@ -41,8 +42,11 @@ impl Into for BgpConfig { } } -impl From for BgpConfig { - fn from(c: params::BgpConfigCreate) -> BgpConfig { +impl BgpConfig { + pub fn from_config_create( + c: ¶ms::BgpConfigCreate, + bgp_announce_set_id: Uuid, + ) -> BgpConfig { BgpConfig { identity: BgpConfigIdentity::new( Uuid::new_v4(), @@ -52,7 +56,8 @@ impl From for BgpConfig { }, ), asn: c.asn.into(), - vrf: c.vrf.map(|x| x.to_string()), + bgp_announce_set_id, + vrf: c.vrf.as_ref().map(|x| x.to_string()), } } } diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index f16b2f3609..ec9fed5f33 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -200,7 +200,6 @@ table! { table! { switch_port_settings_bgp_peer_config (port_settings_id, interface_name, addr) { port_settings_id -> Uuid, - bgp_announce_set_id -> Uuid, bgp_config_id -> Uuid, interface_name -> Text, addr -> Inet, @@ -216,6 +215,7 @@ table! { time_modified -> Timestamptz, time_deleted -> Nullable, asn -> Int8, + bgp_announce_set_id -> Uuid, vrf -> Nullable, } } diff --git a/nexus/db-model/src/switch_port.rs b/nexus/db-model/src/switch_port.rs index 8f81883301..086508e0e0 100644 --- a/nexus/db-model/src/switch_port.rs +++ b/nexus/db-model/src/switch_port.rs @@ -439,11 +439,6 @@ impl Into for SwitchPortRouteConfig { #[diesel(table_name = switch_port_settings_bgp_peer_config)] pub struct SwitchPortBgpPeerConfig { pub port_settings_id: Uuid, - - //TODO(ry) this should be associated with the BGP configuration - // not an individual peer. - pub bgp_announce_set_id: Uuid, - pub bgp_config_id: Uuid, pub interface_name: String, pub addr: IpNetwork, @@ -452,18 +447,11 @@ pub struct SwitchPortBgpPeerConfig { impl SwitchPortBgpPeerConfig { pub fn new( port_settings_id: Uuid, - bgp_announce_set_id: Uuid, bgp_config_id: Uuid, interface_name: String, addr: IpNetwork, ) -> Self { - Self { - port_settings_id, - bgp_announce_set_id, - bgp_config_id, - interface_name, - addr, - } + Self { port_settings_id, bgp_config_id, interface_name, addr } } } @@ -471,7 +459,6 @@ impl Into for SwitchPortBgpPeerConfig { fn into(self) -> external::SwitchPortBgpPeerConfig { external::SwitchPortBgpPeerConfig { port_settings_id: self.port_settings_id, - bgp_announce_set_id: self.bgp_announce_set_id, bgp_config_id: self.bgp_config_id, interface_name: self.interface_name.clone(), addr: self.addr.ip(), diff --git a/nexus/db-queries/src/db/datastore/bgp.rs b/nexus/db-queries/src/db/datastore/bgp.rs index 898545b678..d5b842b29e 100644 --- a/nexus/db-queries/src/db/datastore/bgp.rs +++ b/nexus/db-queries/src/db/datastore/bgp.rs @@ -27,26 +27,45 @@ impl DataStore { config: ¶ms::BgpConfigCreate, ) -> CreateResult { use db::schema::bgp_config::dsl; + use db::schema::{ + bgp_announce_set, bgp_announce_set::dsl as announce_set_dsl, + }; let pool = self.pool_connection_authorized(opctx).await?; - let config: BgpConfig = config.clone().into(); - - let result = diesel::insert_into(dsl::bgp_config) - .values(config.clone()) - .returning(BgpConfig::as_returning()) - .get_result_async(&*pool) - .await - .map_err(|e| { - public_error_from_diesel( - e, - ErrorHandler::Conflict( - ResourceType::BgpConfig, - &config.id().to_string(), - ), - ) - })?; - - Ok(result) + /* + + #[derive(Debug)] + enum BgpConfigSetError { + AnnounceSetNotFound, + } + type TxnError = TransactionError; + */ + + pool.transaction_async(|conn| async move { + let id: Uuid = match &config.bgp_announce_set_id { + 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? + } + NameOrId::Id(id) => *id, + }; + + let config = BgpConfig::from_config_create(config, id); + + let result = diesel::insert_into(dsl::bgp_config) + .values(config.clone()) + .returning(BgpConfig::as_returning()) + .get_result_async(&conn) + .await?; + Ok(result) + }) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } pub async fn bgp_config_delete( @@ -278,8 +297,8 @@ impl DataStore { use db::schema::bgp_announce_set::dsl as announce_set_dsl; use db::schema::bgp_announcement::dsl as bgp_announcement_dsl; - 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; + use db::schema::bgp_config; + use db::schema::bgp_config::dsl as bgp_config_dsl; #[derive(Debug)] enum BgpAnnounceSetDeleteError { @@ -303,12 +322,11 @@ impl DataStore { NameOrId::Id(id) => id, }; - let count = - sps_bgp_peer_config_dsl::switch_port_settings_bgp_peer_config - .filter(sps_bgp_peer_config::bgp_announce_set_id.eq(id)) - .count() - .execute_async(&conn) - .await?; + let count = bgp_config_dsl::bgp_config + .filter(bgp_config::bgp_announce_set_id.eq(id)) + .count() + .execute_async(&conn) + .await?; if count > 0 { return Err(TxnError::CustomError( diff --git a/nexus/db-queries/src/db/datastore/switch_port.rs b/nexus/db-queries/src/db/datastore/switch_port.rs index bedd8c19d9..9d28a31013 100644 --- a/nexus/db-queries/src/db/datastore/switch_port.rs +++ b/nexus/db-queries/src/db/datastore/switch_port.rs @@ -142,7 +142,7 @@ impl DataStore { ) -> CreateResult { use db::schema::{ address_lot::dsl as address_lot_dsl, - bgp_announce_set::dsl as bgp_announce_set_dsl, + //XXX ANNOUNCE bgp_announce_set::dsl as bgp_announce_set_dsl, bgp_config::dsl as bgp_config_dsl, lldp_service_config::dsl as lldp_config_dsl, switch_port_settings::dsl as port_settings_dsl, @@ -158,7 +158,7 @@ impl DataStore { #[derive(Debug)] enum SwitchPortSettingsCreateError { AddressLotNotFound, - BgpAnnounceSetNotFound, + //XXX ANNOUNCE BgpAnnounceSetNotFound, BgpConfigNotFound, ReserveBlock(ReserveBlockError), } @@ -298,6 +298,7 @@ impl DataStore { let mut bgp_peer_config = Vec::new(); for (interface_name, p) in ¶ms.bgp_peers { + /* XXX ANNOUNCE use db::schema::bgp_announce_set; let announce_set_id = match &p.bgp_announce_set { NameOrId::Id(id) => *id, @@ -317,6 +318,7 @@ impl DataStore { })? } }; + */ use db::schema::bgp_config; let bgp_config_id = match &p.bgp_config { @@ -340,7 +342,6 @@ impl DataStore { bgp_peer_config.push(SwitchPortBgpPeerConfig::new( psid, - announce_set_id, bgp_config_id, interface_name.clone(), p.addr.into(), @@ -420,9 +421,6 @@ impl DataStore { }) .await .map_err(|e| match e { - TxnError::CustomError(SpsCreateError::BgpAnnounceSetNotFound) => { - Error::invalid_request("BGP announce set not found") - } TxnError::CustomError(SpsCreateError::AddressLotNotFound) => { Error::invalid_request("AddressLot not found") } diff --git a/nexus/src/app/sagas/switch_port_settings_apply.rs b/nexus/src/app/sagas/switch_port_settings_apply.rs index da596b17ce..a77b43965f 100644 --- a/nexus/src/app/sagas/switch_port_settings_apply.rs +++ b/nexus/src/app/sagas/switch_port_settings_apply.rs @@ -379,7 +379,7 @@ pub(crate) async fn ensure_switch_port_bgp_settings( .bgp_announce_list( &opctx, ¶ms::BgpAnnounceSetSelector { - name_or_id: NameOrId::Id(peer.bgp_announce_set_id), + name_or_id: NameOrId::Id(config.bgp_announce_set_id), }, ) .await @@ -854,7 +854,7 @@ pub(crate) async fn bootstore_update( .bgp_announce_list( &opctx, ¶ms::BgpAnnounceSetSelector { - name_or_id: NameOrId::Id(p.bgp_announce_set_id), + name_or_id: NameOrId::Id(bgp_config.bgp_announce_set_id), }, ) .await diff --git a/nexus/src/app/switch_port.rs b/nexus/src/app/switch_port.rs index 3911bfcaa3..537f52d5df 100644 --- a/nexus/src/app/switch_port.rs +++ b/nexus/src/app/switch_port.rs @@ -352,7 +352,7 @@ impl super::Nexus { .bgp_announce_list( &opctx, ¶ms::BgpAnnounceSetSelector { - name_or_id: p.bgp_announce_set_id.into(), + name_or_id: bgp_config.bgp_announce_set_id.into(), }, ) .await?; diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 27cb30c24b..8fba22fb2f 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -429,6 +429,7 @@ lazy_static! { name: "as47".parse().unwrap(), description: "BGP config for AS47".into(), }, + bgp_announce_set_id: NameOrId::Name("instances".parse().unwrap()), asn: 47, vrf: None, }; diff --git a/nexus/tests/integration_tests/switch_port.rs b/nexus/tests/integration_tests/switch_port.rs index f65ab10b43..b6b9b193e5 100644 --- a/nexus/tests/integration_tests/switch_port.rs +++ b/nexus/tests/integration_tests/switch_port.rs @@ -57,42 +57,43 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { .await .unwrap(); - // Create BGP config - let bgp_config = BgpConfigCreate { + // Create BGP announce set + let announce_set = BgpAnnounceSetCreate { identity: IdentityMetadataCreateParams { - name: "as47".parse().unwrap(), - description: "autonomous system 47".into(), + name: "instances".parse().unwrap(), + description: "autonomous system 47 announcements".into(), }, - asn: 47, - vrf: None, + announcement: vec![BgpAnnouncementCreate { + address_lot_block: NameOrId::Name("parkinglot".parse().unwrap()), + network: "1.2.3.0/24".parse().unwrap(), + }], }; NexusRequest::objects_post( client, - "/v1/system/networking/bgp", - &bgp_config, + "/v1/system/networking/bgp-announce", + &announce_set, ) .authn_as(AuthnMode::PrivilegedUser) .execute() .await .unwrap(); - // Create BGP announce set - let announce_set = BgpAnnounceSetCreate { + // Create BGP config + let bgp_config = BgpConfigCreate { identity: IdentityMetadataCreateParams { - name: "instances".parse().unwrap(), - description: "autonomous system 47 announcements".into(), + name: "as47".parse().unwrap(), + description: "autonomous system 47".into(), }, - announcement: vec![BgpAnnouncementCreate { - address_lot_block: NameOrId::Name("parkinglot".parse().unwrap()), - network: "1.2.3.0/24".parse().unwrap(), - }], + bgp_announce_set_id: NameOrId::Name("instances".parse().unwrap()), + asn: 47, + vrf: None, }; NexusRequest::objects_post( client, - "/v1/system/networking/bgp-announce", - &announce_set, + "/v1/system/networking/bgp", + &bgp_config, ) .authn_as(AuthnMode::PrivilegedUser) .execute() diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 633bb8d5c6..fa25a576a7 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -1494,6 +1494,8 @@ pub struct BgpConfigCreate { /// The autonomous system number of this BGP configuration. pub asn: u32, + pub bgp_announce_set_id: NameOrId, + /// Optional virtual routing and forwarding identifier for this BGP /// configuration. pub vrf: Option, diff --git a/openapi/nexus.json b/openapi/nexus.json index 009a168164..2ff907eb67 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -8309,6 +8309,9 @@ "format": "uint32", "minimum": 0 }, + "bgp_announce_set_id": { + "$ref": "#/components/schemas/NameOrId" + }, "description": { "type": "string" }, @@ -8327,6 +8330,7 @@ }, "required": [ "asn", + "bgp_announce_set_id", "description", "name" ] @@ -14336,11 +14340,6 @@ "type": "string", "format": "ip" }, - "bgp_announce_set_id": { - "description": "The id for the set of prefixes announced in this peer configuration.", - "type": "string", - "format": "uuid" - }, "bgp_config_id": { "description": "The id of the global BGP configuration referenced by this peer configuration.", "type": "string", @@ -14358,7 +14357,6 @@ }, "required": [ "addr", - "bgp_announce_set_id", "bgp_config_id", "interface_name", "port_settings_id" diff --git a/schema/crdb/8.0.0/up.sql b/schema/crdb/8.0.0/up1.sql similarity index 100% rename from schema/crdb/8.0.0/up.sql rename to schema/crdb/8.0.0/up1.sql diff --git a/schema/crdb/8.0.0/up2.sql b/schema/crdb/8.0.0/up2.sql new file mode 100644 index 0000000000..0b6a44b4fd --- /dev/null +++ b/schema/crdb/8.0.0/up2.sql @@ -0,0 +1,2 @@ +ALTER TABLE omicron.public.bgp_config ADD COLUMN IF NOT EXISTS bgp_announce_set_id UUID NOT NULL; +ALTER TABLE omicron.public.switch_port_settings_bgp_peer_config DROP COLUMN IF EXISTS bgp_announce_set_id; diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index d074a0af53..024ff24638 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -2442,7 +2442,6 @@ CREATE TABLE IF NOT EXISTS omicron.public.switch_port_settings_route_config ( CREATE TABLE IF NOT EXISTS omicron.public.switch_port_settings_bgp_peer_config ( port_settings_id UUID, - bgp_announce_set_id UUID NOT NULL, bgp_config_id UUID NOT NULL, interface_name TEXT, addr INET, @@ -2459,7 +2458,8 @@ CREATE TABLE IF NOT EXISTS omicron.public.bgp_config ( time_modified TIMESTAMPTZ NOT NULL, time_deleted TIMESTAMPTZ, asn INT8 NOT NULL, - vrf TEXT + vrf TEXT, + bgp_announce_set_id UUID NOT NULL ); CREATE UNIQUE INDEX IF NOT EXISTS lookup_bgp_config_by_name ON omicron.public.bgp_config (