Skip to content

Commit fb4c746

Browse files
authored
[nexus] don't choke on alert globs that don't match anything (#10789)
Currently, the `alert_dispatcher` background task will log a bunch of errors if any alert receiver exists that has a glob subscription that does not currently match any existing alert classes. This is because reprocessing those glob subscriptions attempts to insert an empty `Vec` of exact subscriptions into the database (because, you know, the glob doesn't generate any subscriptions), and inserting an empty `Vec` inside of a `DatastoreCollection::insert_resource` results in an attempt to execute invalid SQL syntax (see #10788). Ideally, we'd actually do something about #10788, but since the argument to `DatastoreCollection::insert_resource` is a wad of arbitrary SQL, I couldn't immediately think of a way for it to recognize when the query passed to it is not actually inserting anything. Therefore, I fixed it the stupid way: by checking if the `Vec` of subscriptions is empty in ` add_exact_subscription_batch_on_conn`, and just returning immediately. This is somewhat sad to have to do, but at least it fixes #10775.
1 parent 892ea87 commit fb4c746

1 file changed

Lines changed: 70 additions & 0 deletions

File tree

nexus/db-queries/src/db/datastore/alert_rx.rs

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -696,6 +696,21 @@ impl DataStore {
696696
subscriptions: Vec<AlertRxSubscription>,
697697
conn: &async_bb8_diesel::Connection<DbConnection>,
698698
) -> Result<Vec<AlertRxSubscription>, TransactionError<Error>> {
699+
// XXX(eliza): this is a rather sad workaround for
700+
// https://github.com/oxidecomputer/omicron/issues/10788, in which it
701+
// turns out that `DatastoreCollection::insert_resource` will basically
702+
// do just about the worst thing possible when given an empty list of
703+
// resources to insert. Ideally, we would make it not fail with a SQL
704+
// syntax error in that case, but since the function's argument is "a
705+
// glob of more or less completely arbitrary Diesel which is probably
706+
// not entirely unlike an INSERT statement", it is not immediately clear
707+
// to me how it would be able to detect that you are trying to insert
708+
// nothing. So, we'll guard against that here for now.
709+
if subscriptions.is_empty() {
710+
// Inserting nothing does nothing, returning nothing!
711+
return Ok(Vec::new());
712+
}
713+
699714
<AlertReceiver as DatastoreCollection<AlertRxSubscription>>::insert_resource(
700715
rx_id.into_untyped_uuid(),
701716
diesel::insert_into(subscription_dsl::alert_subscription)
@@ -1296,6 +1311,15 @@ mod test {
12961311
"test.quux.**",
12971312
)
12981313
.await;
1314+
// Not actually subscribed to anything!
1315+
let _test_nonexistent_class = create_rx(
1316+
&datastore,
1317+
&opctx,
1318+
&mut all_rxs,
1319+
"test-nonexistent-class",
1320+
"test.nonexistent.*",
1321+
)
1322+
.await;
12991323

13001324
// Before we check whether the receivers are subscribed to the expected
13011325
// event classes, we must generate exact subscriptions for their globs.
@@ -1493,4 +1517,50 @@ mod test {
14931517
db.terminate().await;
14941518
logctx.cleanup_successful();
14951519
}
1520+
1521+
#[tokio::test]
1522+
async fn test_insert_empty_subscription_batch() {
1523+
let logctx =
1524+
dev::test_setup_log("test_insert_empty_subscription_batch");
1525+
let db = TestDatabase::new_with_datastore(&logctx.log).await;
1526+
let (opctx, datastore) = (db.opctx(), db.datastore());
1527+
1528+
let rx = create_receiver(datastore, opctx, "test", Vec::new()).await.rx;
1529+
1530+
let conn = datastore
1531+
.pool_connection_for_tests()
1532+
.await
1533+
.expect("cant get ye conn");
1534+
1535+
let result = datastore
1536+
.add_exact_subscription_batch_on_conn(rx.id(), Vec::new(), &conn)
1537+
.await
1538+
.expect("creating an empty subscription batch shouldn't kill you");
1539+
1540+
assert!(
1541+
result.is_empty(),
1542+
"returned subscription batch should be empty, but found: {:?}",
1543+
result
1544+
);
1545+
1546+
let (authz_rx, _) = LookupPath::new(opctx, datastore)
1547+
.alert_receiver_id(rx.id())
1548+
.fetch()
1549+
.await
1550+
.expect("rx should exist");
1551+
1552+
let (subscriptions, _) = datastore
1553+
.webhook_rx_config_fetch(opctx, &authz_rx)
1554+
.await
1555+
.expect("alert receiver config should exist");
1556+
1557+
assert!(
1558+
subscriptions.is_empty(),
1559+
"alert receiver config should have no subscriptions, \
1560+
but found: {subscriptions:?}",
1561+
);
1562+
1563+
db.terminate().await;
1564+
logctx.cleanup_successful();
1565+
}
14961566
}

0 commit comments

Comments
 (0)