[bgp-config] move lookups from datastore

[nicolaskagami]

• Moves bgp-config lookups out of datastore and into the app layer.
• Ensures the announce set exists within a transaction when creating / updating.

[nicolaskagami]

This is relatively minimal change that moves the lookups from the the datastore while keeping the guarantees around announce set existing.

I've adapted the bgp_config_create to receive the db::model::BgpConfig, a pattern I saw in a few other places.

The only possible concern I have is if we shouldn't do the proper lookup mechanics when initializing the rack in nexus/src/app/rack.rs for whatever reason. If fine, an argument could be made to call the app layer bgp_config_create directly from rack_initialize, since it's already in app and I don't think we wan to have different business logic for it.

[david-crespo]

Looks good! A couple small suggestions from bot review:

• assert matching config ID instead of config name in test
• remove a redundant DB fetch query

diff --git a/nexus/db-queries/src/db/datastore/switch_port.rs b/nexus/db-queries/src/db/datastore/switch_port.rs
index 74e4e3de1a..d22d355d66 100644
--- a/nexus/db-queries/src/db/datastore/switch_port.rs
+++ b/nexus/db-queries/src/db/datastore/switch_port.rs
@@ -2666,19 +2666,14 @@
                     NameOrId::Name(name) => {
                         let (.., authz_bgp_config) =
                             LookupPath::new(&opctx, datastore)
-                                .bgp_config_name_owned(name.clone().into())
+                                .bgp_config_name_owned(name.into())
                                 .lookup_for(nexus_auth::authz::Action::Read)
                                 .await
                                 .expect("lookup bgp config");
 
-                        let db_bgp_config = datastore
-                            .bgp_config_get(opctx, authz_bgp_config.id())
-                            .await
-                            .expect("bgp config should be present in db");
-
                         assert_eq!(
-                            db_bgp_config.identity.name,
-                            nexus_db_model::Name(name)
+                            db_peer.bgp_config_id(),
+                            authz_bgp_config.id()
                         );
                     }
                 }
diff --git a/nexus/src/app/bgp.rs b/nexus/src/app/bgp.rs
index 59e7b7fe69..b9c8a80e2c 100644
--- a/nexus/src/app/bgp.rs
+++ b/nexus/src/app/bgp.rs
@@ -46,12 +46,12 @@
     ) -> LookupResult<BgpConfig> {
         opctx.authorize(authz::Action::Read, &authz::FLEET).await?;
 
-        let (.., authz_bgp_config, _) = self
-            .bgp_config_lookup(opctx, name_or_id.clone())?
+        let (.., db_bgp_config) = self
+            .bgp_config_lookup(opctx, name_or_id)?
             .fetch_for(authz::Action::Read)
             .await?;
 
-        self.db_datastore.bgp_config_get(opctx, authz_bgp_config.id()).await
+        Ok(db_bgp_config)
     }
 
     pub async fn bgp_config_list(

TOCTOU risk

I was curious about the TOCTOU risk of moving the lookups out of the transaction. Here is a 🤖 summary of why it's not an issue here (after the above fix).

Endpoint/code path	Concurrent event	Behavior
Create BGP config	Announce set is deleted or recreated after name lookup	The resolved UUID is rechecked inside the create transaction. A replacement with the same name has a different UUID and is not used.
Get BGP config	Config is deleted while GET runs	There is now one read. If deletion happens first, GET returns 404. If GET reads first, it returns the config it read.
Update BGP config	New announce set is deleted after lookup	Its UUID is rechecked inside the update transaction. The update cannot leave a reference to a deleted announce set.
Update BGP config	Target config itself is deleted after lookup	The update finds no active row, but currently maps that result to a 500. This is a pre-existing rough edge, not introduced by this PR. It should eventually use NotFoundByResource.
Delete BGP config	Another object reuses the same name	Delete uses the originally resolved UUID, so it cannot delete the replacement object.
Delete BGP config	A switch-port setting starts referencing it concurrently	The reference check and deletion are in one serializable transaction. Either the reference wins and deletion reports “in use,” or deletion wins and creating the reference fails/retries.
Delete BGP config	Another request deletes it concurrently	The later operation updates the already-deleted row and succeeds as an idempotent no-op.
Rack initialization	Announce set changes between lookup and config creation	Same protection as the create endpoint: stable UUID plus an in-transaction existence check.
Switch-configuration background task	Config disappears while being loaded	It performs one lookup by UUID. Missing configurations are logged and skipped. Normally deletion is prevented while a switch-port setting still references the config.

One more fix

After putting together the above table I found one more suggestion: 404 instead of 500ing when a BGP config gets deleted in the middle of an update.

diff --git a/nexus/db-queries/src/db/datastore/bgp.rs b/nexus/db-queries/src/db/datastore/bgp.rs
index 7287b4f81b..dcc7887da8 100644
--- a/nexus/db-queries/src/db/datastore/bgp.rs
+++ b/nexus/db-queries/src/db/datastore/bgp.rs
@@ -256,7 +256,10 @@
                     err
                 } else {
                     error!(opctx.log, "{msg}"; "error" => ?e);
-                    public_error_from_diesel(e, ErrorHandler::Server)
+                    public_error_from_diesel(
+                        e,
+                        ErrorHandler::NotFoundByResource(authz_bgp_config),
+                    )
                 }
             })
     }
@@ -1122,7 +1125,7 @@
 
         // Update the BGP config
         datastore
-            .bgp_config_update(&opctx, &authz_bgp_config, update)
+            .bgp_config_update(&opctx, &authz_bgp_config, update.clone())
             .await
             .expect("update bgp config");
 
@@ -1140,6 +1143,22 @@
         assert_eq!(bgp_config.bgp_announce_set_id, new_announce_id);
         assert_eq!(bgp_config.max_paths.0, new_max_paths.as_u8());
 
+        // Simulate deletion after the app layer has resolved the config but
+        // before the datastore update begins.
+        datastore
+            .bgp_config_delete(&opctx, &authz_bgp_config)
+            .await
+            .expect("delete bgp config");
+
+        let err = datastore
+            .bgp_config_update(&opctx, &authz_bgp_config, update)
+            .await
+            .expect_err("updating a deleted BGP config should fail");
+        assert!(
+            matches!(err, Error::ObjectNotFound { .. }),
+            "unexpected error: {err:?}"
+        );
+
         db.terminate().await;
         logctx.cleanup_successful();
     }

[nicolaskagami]

assert matching config ID instead of config name in test

👍

• remove a redundant DB fetch query

I didn't do this initially because there is an explicit filter there for time_deleted.is_null() which I'm not sure applies in the lookup. Are you sure that's ok?

[david-crespo]

Yup, the lookups do the soft delete check:

omicron/nexus/db-macros/src/lookup.rs

Lines 834 to 838 in 5e2a6a6

	let soft_delete_filter = if config.soft_deletes {
	quote! { .filter(dsl::time_deleted.is_null()) }
	} else {
	quote! {}
	};

omicron/nexus/db-lookup/src/lookup.rs

Lines 894 to 900 in 5e2a6a6

	lookup_resource! {
	name = "BgpConfig",
	ancestors = [],
	lookup_by_name = true,
	soft_deletes = true,
	primary_key_columns = [ { column_name = "id", uuid_kind = BgpConfigKind} ]
	}

[jgallagher]

LGTM.

The only possible concern I have is if we shouldn't do the proper lookup mechanics when initializing the rack in nexus/src/app/rack.rs for whatever reason. If fine, an argument could be made to call the app layer bgp_config_create directly from rack_initialize, since it's already in app and I don't think we wan to have different business logic for it.

AFAIK authz / lookups all work fine during rack initialization. It looks like every other call in that method goes directly to the datastore. I hope this isn't true but it easily could be: some app-level methods might expect rack initialization to have already happened in some way (unwrapping an option? assuming some db table is populated? whatever), so maybe it's safer to only call datastore methods if we're worried about that?

[jgallagher]

Can we get the announce set ID from the return value of the create above (line 416) instead of having to look it up here? I guess we'd have to look it up inside the ObjectAlreadyExists error branch since that doesn't return an ID :(

[nicolaskagami]

Yep. But I'm inclined to use the app version given your comment.