Initial schema updates disallowing default IP Pool for the internal silo

- Add database constraints that ensure that we can't set a default pool
  for the Oxide internal silo. Add tests for this specifically.
- This is part of #8948, but does not resolve it.

Author: bnaecker

nexus/db-model/src/ip_pool.rs

@@ -153,7 +153,16 @@ impl_enum_type!(
     Silo => b"silo"
 );
 
-#[derive(Queryable, Insertable, Selectable, Clone, Debug)]
+#[derive(
+    AsChangeset,
+    Queryable,
+    Insertable,
+    Selectable,
+    Clone,
+    Copy,
+    Debug,
+    PartialEq,
+)]
 #[diesel(table_name = ip_pool_resource)]
 pub struct IpPoolResource {
     pub ip_pool_id: Uuid,

nexus/db-model/src/schema_versions.rs

@@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock};
 ///
 /// This must be updated when you change the database schema.  Refer to
 /// schema/crdb/README.adoc in the root of this repository for details.
-pub const SCHEMA_VERSION: Version = Version::new(186, 0, 0);
+pub const SCHEMA_VERSION: Version = Version::new(187, 0, 0);
 
 /// List of all past database schema versions, in *reverse* order
 ///
@@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock<Vec<KnownVersion>> = LazyLock::new(|| {
         // |  leaving the first copy as an example for the next person.
         // v
         // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
+        KnownVersion::new(187, "no-default-pool-for-internal-silo"),
         KnownVersion::new(186, "nexus-generation"),
         KnownVersion::new(185, "populate-db-metadata-nexus"),
         KnownVersion::new(184, "store-silo-admin-group-name"),

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

@@ -27,6 +27,7 @@ use crate::db::queries::ip_pool::FilterOverlappingIpRanges;
 use async_bb8_diesel::AsyncRunQueryDsl;
 use chrono::Utc;
 use diesel::prelude::*;
+use diesel::result::DatabaseErrorKind;
 use diesel::result::Error as DieselError;
 use ipnetwork::IpNetwork;
 use nexus_db_errors::ErrorHandler;
@@ -587,23 +588,40 @@ impl DataStore {
 
         let conn = self.pool_connection_authorized(opctx).await?;
 
+        // We always insert and update the record on conflicts.
+        //
+        // This lets us use the database constraints for a few checks, such as
+        // assigning more than one default pool for a silo, and ensuring that
+        // there is no default at all for the internal silo.
         let result = diesel::insert_into(dsl::ip_pool_resource)
-            .values(ip_pool_resource.clone())
+            .values(ip_pool_resource)
+            .on_conflict((dsl::ip_pool_id, dsl::resource_id, dsl::resource_type))
+            .do_update()
+            .set(ip_pool_resource)
             .get_result_async(&*conn)
             .await
             .map_err(|e| {
-                public_error_from_diesel(
-                    e,
-                    ErrorHandler::Conflict(
-                        ResourceType::IpPoolResource,
-                        &format!(
-                            "ip_pool_id: {:?}, resource_id: {:?}, resource_type: {:?}",
-                            ip_pool_resource.ip_pool_id,
-                            ip_pool_resource.resource_id,
-                            ip_pool_resource.resource_type,
+                match e {
+                    // Specifically catch conflicts on the unique index which
+                    // ensures at most one default IP Pool per silo.
+                    DieselError::DatabaseError(DatabaseErrorKind::UniqueViolation, ref info)
+                        if info.constraint_name() == Some("one_default_ip_pool_per_resource") =>
+                    {
+                        public_error_from_diesel(
+                            e,
+                            ErrorHandler::Conflict(
+                                ResourceType::IpPoolResource,
+                                &format!(
+                                    "ip_pool_id: {}, resource_id: {}, resource_type: {:?}",
+                                    ip_pool_resource.ip_pool_id,
+                                    ip_pool_resource.resource_id,
+                                    ip_pool_resource.resource_type,
+                                ),
+                            )
                         )
-                    ),
-                )
+                    }
+                    _ => public_error_from_diesel(e, ErrorHandler::Server),
+                }
             })?;
 
         if ip_pool_resource.is_default {
@@ -1360,7 +1378,7 @@ mod test {
             is_default: false,
         };
         datastore
-            .ip_pool_link_silo(&opctx, link_body.clone())
+            .ip_pool_link_silo(&opctx, link_body)
             .await
             .expect("Failed to associate IP pool with silo");
 
@@ -1378,12 +1396,12 @@ mod test {
         assert_eq!(silo_pools[0].0.id(), pool1_for_silo.id());
         assert_eq!(silo_pools[0].1.is_default, false);
 
-        // linking an already linked silo errors due to PK conflict
-        let err = datastore
+        // linking an already linked silo is fine
+        let new = datastore
             .ip_pool_link_silo(&opctx, link_body)
             .await
-            .expect_err("Creating the same link again should conflict");
-        assert_matches!(err, Error::ObjectAlreadyExists { .. });
+            .expect("Creating the same link again should not conflict");
+        assert_eq!(new, link_body);
 
         // now make it default
         datastore
@@ -1489,9 +1507,6 @@ mod test {
             assert_eq!(is_internal, Ok(false));
 
             // now link it to the current silo, and it is still not internal.
-            //
-            // We're only making the IPv4 pool the default right now. See
-            // https://github.com/oxidecomputer/omicron/issues/8884 for more.
             let silo_id = opctx.authn.silo_required().unwrap().id();
             let is_default = matches!(version, IpVersion::V4);
             let link = IpPoolResource {
@@ -1514,6 +1529,79 @@ mod test {
         logctx.cleanup_successful();
     }
 
+    #[tokio::test]
+    async fn cannot_set_default_ip_pool_for_internal_silo() {
+        let logctx =
+            dev::test_setup_log("cannot_set_default_ip_pool_for_internal_silo");
+        let db = TestDatabase::new_with_datastore(&logctx.log).await;
+        let (opctx, datastore) = (db.opctx(), db.datastore());
+
+        for version in [IpVersion::V4, IpVersion::V6] {
+            // confirm internal pools appear as internal
+            let (authz_pool, pool) = datastore
+                .ip_pools_service_lookup(&opctx, version)
+                .await
+                .unwrap();
+            assert_eq!(pool.ip_version, version);
+
+            let is_internal =
+                datastore.ip_pool_is_internal(&opctx, &authz_pool).await;
+            assert_eq!(is_internal, Ok(true));
+
+            // Try to link it as the default.
+            let (authz_silo, ..) =
+                nexus_db_lookup::LookupPath::new(&opctx, datastore)
+                    .silo_id(nexus_types::silo::INTERNAL_SILO_ID)
+                    .lookup_for(authz::Action::Read)
+                    .await
+                    .expect("Should be able to lookup internal silo");
+            let link = IpPoolResource {
+                ip_pool_id: authz_pool.id(),
+                resource_type: IpPoolResourceType::Silo,
+                resource_id: authz_silo.id(),
+                is_default: true,
+            };
+            let Err(e) = datastore.ip_pool_link_silo(opctx, link).await else {
+                panic!(
+                    "should have failed to link IP Pool to internal silo as a default"
+                );
+            };
+            let Error::InternalError { internal_message } = &e else {
+                panic!("should have received an internal error");
+            };
+            assert!(
+                internal_message.contains("failed to satisfy CHECK constraint"),
+                "Expected a CHECK constraint violation, found: {}",
+                internal_message,
+            );
+
+            // We can link it if it's not the default.
+            let link = IpPoolResource { is_default: false, ..link };
+            datastore.ip_pool_link_silo(opctx, link).await.expect(
+                "Should be able to link non-default pool to internal silo",
+            );
+
+            // Try to set it to the default, and ensure that this also fails.
+            let Err(e) = datastore
+                .ip_pool_set_default(opctx, &authz_pool, &authz_silo, true)
+                .await
+            else {
+                panic!("should have failed to set internal pool to default");
+            };
+            let Error::InternalError { internal_message } = &e else {
+                panic!("should have received an internal error");
+            };
+            assert!(
+                internal_message.contains("failed to satisfy CHECK constraint"),
+                "Expected a CHECK constraint violation, found: {}",
+                internal_message,
+            );
+        }
+
+        db.terminate().await;
+        logctx.cleanup_successful();
+    }
+
     // We're breaking out the utilization tests for IPv4 and IPv6 pools, since
     // pools only contain one version now.
     //

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

@@ -991,9 +991,9 @@ impl DataStore {
                 },
                 version,
             );
-
+            // Create the pool, and link it to the internal silo if needed. But
+            // we cannot set a default.
             let internal_pool_id = internal_pool.id();
-
             let internal_created = self
                 .ip_pool_create(opctx, internal_pool)
                 .await
@@ -1002,25 +1002,14 @@ impl DataStore {
                     Error::ObjectAlreadyExists { .. } => Ok(false),
                     _ => Err(e),
                 })?;
-
-            // make default for the internal silo. only need to do this if
-            // the create went through, i.e., if it wasn't already there
-            //
-            // TODO-completeness: We're linking both IP pools here, but only the
-            // IPv4 pool is set as a default. We need a way for the operator to
-            // control this, either at RSS or through the API. An alternative is
-            // to not set a default at all, even though both are linked.
-            //
-            // See https://github.com/oxidecomputer/omicron/issues/8884
             if internal_created {
-                let is_default = matches!(version, IpVersion::V4);
                 self.ip_pool_link_silo(
                     opctx,
                     db::model::IpPoolResource {
                         ip_pool_id: internal_pool_id,
                         resource_type: db::model::IpPoolResourceType::Silo,
                         resource_id: INTERNAL_SILO_ID,
-                        is_default,
+                        is_default: false,
                     },
                 )
                 .await?;

nexus/src/app/silo.rs

@@ -50,6 +50,7 @@ impl super::Nexus {
         let silo = self.silo_lookup(opctx, NameOrId::Id(silo.id()))?;
         Ok(silo)
     }
+
     pub fn silo_lookup<'a>(
         &'a self,
         opctx: &'a OpContext,

schema/crdb/dbinit.sql

@@ -2110,7 +2110,20 @@ CREATE TABLE IF NOT EXISTS omicron.public.ip_pool_resource (
 
     -- resource_type is redundant because resource IDs are globally unique, but
     -- logically it belongs here
-    PRIMARY KEY (ip_pool_id, resource_type, resource_id)
+    PRIMARY KEY (ip_pool_id, resource_type, resource_id),
+
+    -- Check that there are no default pools for the internal silo
+    CONSTRAINT internal_silo_has_no_default_pool CHECK (
+        -- A = linked to internal, B = (not default)
+        -- A -> B iff ¬A v B
+        -- ¬(linked to internal silo) OR (not default)
+        NOT (
+            resource_type = 'silo' AND
+            resource_id = '001de000-5110-4000-8000-000000000001'
+        )
+        OR NOT is_default
+    )
+
 );
 
 -- a given resource can only have one default ip pool
@@ -2123,6 +2136,7 @@ CREATE UNIQUE INDEX IF NOT EXISTS one_default_ip_pool_per_resource ON omicron.pu
 CREATE INDEX IF NOT EXISTS ip_pool_resource_id ON omicron.public.ip_pool_resource (
     resource_id
 );
+
 CREATE INDEX IF NOT EXISTS ip_pool_resource_ip_pool_id ON omicron.public.ip_pool_resource (
     ip_pool_id
 );
@@ -6602,7 +6616,7 @@ INSERT INTO omicron.public.db_metadata (
     version,
     target_version
 ) VALUES
-    (TRUE, NOW(), NOW(), '186.0.0', NULL)
+    (TRUE, NOW(), NOW(), '187.0.0', NULL)
 ON CONFLICT DO NOTHING;
 
 COMMIT;

schema/crdb/no-default-pool-for-internal-silo/up01.sql

@@ -0,0 +1,9 @@
+/*
+ * Ensure we have no default pool for the internal silo.
+ */
+SET LOCAL disallow_full_table_scans = 'off';
+UPDATE omicron.public.ip_pool_resource
+SET is_default = FALSE
+WHERE
+    resource_type = 'silo' AND
+    resource_id = '001de000-5110-4000-8000-000000000001';

schema/crdb/no-default-pool-for-internal-silo/up02.sql

@@ -0,0 +1,18 @@
+/*
+ * Add check constraint ensuring that, if the pool is linked
+ * to the internal Oxide services silo, it's not marked as
+ * a default pool.
+ */
+ALTER TABLE IF EXISTS
+omicron.public.ip_pool_resource
+ADD CONSTRAINT IF NOT EXISTS
+internal_silo_has_no_default_pool CHECK (
+    -- A = linked to internal, B = (not default)
+    -- A -> B iff ¬A v B
+    -- ¬(linked to internal silo) OR (not default)
+    NOT (
+        resource_type = 'silo' AND
+        resource_id = '001de000-5110-4000-8000-000000000001'
+    )
+    OR NOT is_default
+);