use one unique partial index for silo_group

Author: jmpesp

common/src/sql/dbinit.sql

@@ -288,15 +288,11 @@ CREATE TABLE omicron.public.silo_group (
     external_id TEXT NOT NULL
 );
 
-CREATE INDEX ON omicron.public.silo_group (
-    silo_id
-) WHERE
-    time_deleted IS NULL;
-
 CREATE UNIQUE INDEX ON omicron.public.silo_group (
     silo_id,
     external_id
-);
+) WHERE
+    time_deleted IS NULL;
 
 /*
  * Silo group membership

nexus/src/db/datastore/silo_group.rs

@@ -36,10 +36,30 @@ impl DataStore {
         opctx.authorize(authz::Action::CreateChild, authz_silo).await?;
 
         use db::schema::silo_group::dsl;
+
+        // Without the on_conflict below, simultaneous attempts to ensure the
+        // same SiloGroup will see one of them fail. This can occur if two users
+        // are logging in at the same time, and both are part of IdP groups that
+        // do not yet exist in our database.
+        //
+        // Currently there is a unique partial index on silo_group, which
+        // because it is partial has a WHERE clause. diesel does not support
+        // creating queries that contain:
+        //
+        //   ON CONFLICT (...) WHERE ... DO NOTHING
+        //
+        // meaning we have to use `on_conflict_do_nothing`. In the future when
+        // diesel does support this, uncomment (and probably fix) the code
+        // below.
+        //
+        // Issue https://github.com/oxidecomputer/omicron/issues/1545 was opened
+        // to track this.
         Ok(diesel::insert_into(dsl::silo_group)
             .values(silo_group)
-            .on_conflict((dsl::silo_id, dsl::external_id))
-            .do_nothing()
+            //.on_conflict((dsl::silo_id, dsl::external_id))
+            //  .where(dsl::time_deleted.is_null())
+            //.do_nothing()
+            .on_conflict_do_nothing()
             .returning(SiloGroup::as_returning()))
     }
 

nexus/tests/integration_tests/silos.rs

@@ -1310,7 +1310,6 @@ async fn test_silo_delete_clean_up_groups(cptestctx: &ControlPlaneTestContext) {
         nexus.datastore().clone(),
     );
 
-    // Create an admin user
     let (authz_silo, db_silo) = LookupPath::new(&opctx, &nexus.datastore())
         .silo_name(&silo.identity.name.into())
         .fetch()
@@ -1363,3 +1362,56 @@ async fn test_silo_delete_clean_up_groups(cptestctx: &ControlPlaneTestContext) {
         .await
         .expect_err("user found");
 }
+
+// Test ensuring the same group from different users
+#[nexus_test]
+async fn test_ensure_same_silo_group(cptestctx: &ControlPlaneTestContext) {
+    let client = &cptestctx.external_client;
+    let nexus = &cptestctx.server.apictx.nexus;
+
+    // Create a silo
+    let silo =
+        create_silo(&client, "test-silo", true, shared::UserProvisionType::Jit)
+            .await;
+
+    let opctx = OpContext::for_tests(
+        cptestctx.logctx.log.new(o!()),
+        nexus.datastore().clone(),
+    );
+
+    let (authz_silo, db_silo) = LookupPath::new(&opctx, &nexus.datastore())
+        .silo_name(&silo.identity.name.into())
+        .fetch()
+        .await
+        .unwrap();
+
+    // Add the first user with a group membership
+    let _silo_user_1 = nexus
+        .silo_user_from_authenticated_subject(
+            &nexus.opctx_external_authn(),
+            &authz_silo,
+            &db_silo,
+            &AuthenticatedSubject {
+                external_id: "user1@company.com".into(),
+                groups: vec!["sre".into()],
+            },
+        )
+        .await
+        .expect("silo_user_from_authenticated_subject 1")
+        .unwrap();
+
+    // Add the first user with a group membership
+    let _silo_user_2 = nexus
+        .silo_user_from_authenticated_subject(
+            &nexus.opctx_external_authn(),
+            &authz_silo,
+            &db_silo,
+            &AuthenticatedSubject {
+                external_id: "user2@company.com".into(),
+                groups: vec!["sre".into()],
+            },
+        )
+        .await
+        .expect("silo_user_from_authenticated_subject 2")
+        .unwrap();
+}