Initial schema updates disallowing default IP Pool for the internal silo (#8964)

bnaecker · web-flow · commit c79764437569 · 2025-09-03T22:00:57.000Z
- 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.
diff --git a/nexus/db-model/src/ip_pool.rs b/nexus/db-model/src/ip_pool.rs
@@ -153,7 +153,7 @@ impl_enum_type!(
     Silo => b"silo"
 );
 
-#[derive(Queryable, Insertable, Selectable, Clone, Debug)]
+#[derive(Queryable, Insertable, Selectable, Clone, Copy, Debug, PartialEq)]
 #[diesel(table_name = ip_pool_resource)]
 pub struct IpPoolResource {
     pub ip_pool_id: Uuid,
diff --git a/nexus/db-model/src/schema_versions.rs b/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"),
diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/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;
@@ -86,6 +87,16 @@ impl ServiceIpPools {
     }
 }
 
+// Constraint used to ensure we don't set a default IP Pool for the internal
+// silo.
+const INTERNAL_SILO_DEFAULT_CONSTRAINT: &'static str =
+    "internal_silo_has_no_default_pool";
+
+// Error message emitted when we attempt to set a default IP Pool for the
+// internal silo.
+const INTERNAL_SILO_DEFAULT_ERROR: &'static str =
+    "The internal Silo cannot have a default IP Pool";
+
 impl DataStore {
     /// List IP Pools
     pub async fn ip_pools_list(
@@ -588,22 +599,34 @@ impl DataStore {
         let conn = self.pool_connection_authorized(opctx).await?;
 
         let result = diesel::insert_into(dsl::ip_pool_resource)
-            .values(ip_pool_resource.clone())
+            .values(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 {
+                    // Catch the check constraint ensuring the internal silo has
+                    // no default pool
+                    DieselError::DatabaseError(DatabaseErrorKind::CheckViolation, ref info)
+                        if info.constraint_name() == Some(INTERNAL_SILO_DEFAULT_CONSTRAINT) =>
+                    {
+                        Error::invalid_request(INTERNAL_SILO_DEFAULT_ERROR)
+                    }
+                    DieselError::DatabaseError(DatabaseErrorKind::UniqueViolation, _) => {
+                        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 {
@@ -885,21 +908,47 @@ impl DataStore {
                     IpPoolResourceUpdateError::FailedToUnsetDefault(err),
                 )) => public_error_from_diesel(err, ErrorHandler::Server),
                 Some(TxnError::Database(err)) => {
-                    public_error_from_diesel(err, ErrorHandler::Server)
+                    match err {
+                        // Catch the check constraint ensuring the internal silo has
+                        // no default pool
+                        DieselError::DatabaseError(
+                            DatabaseErrorKind::CheckViolation,
+                            ref info,
+                        ) if info.constraint_name()
+                            == Some(INTERNAL_SILO_DEFAULT_CONSTRAINT) =>
+                        {
+                            Error::invalid_request(INTERNAL_SILO_DEFAULT_ERROR)
+                        }
+                        _ => {
+                            public_error_from_diesel(err, ErrorHandler::Server)
+                        }
+                    }
                 }
                 None => {
-                    public_error_from_diesel(
-                        e,
-                        ErrorHandler::NotFoundByLookup(
-                            ResourceType::IpPoolResource,
-                            // TODO: would be nice to put the actual names and/or ids in
-                            // here but LookupType on each of the two silos doesn't have
-                            // a nice to_string yet or a way of composing them
-                            LookupType::ByCompositeId(
-                                "(pool, silo)".to_string(),
+                    match e {
+                        // Catch the check constraint ensuring the internal silo has
+                        // no default pool
+                        DieselError::DatabaseError(
+                            DatabaseErrorKind::CheckViolation,
+                            ref info,
+                        ) if info.constraint_name()
+                            == Some(INTERNAL_SILO_DEFAULT_CONSTRAINT) =>
+                        {
+                            Error::invalid_request(INTERNAL_SILO_DEFAULT_ERROR)
+                        }
+                        _ => public_error_from_diesel(
+                            e,
+                            ErrorHandler::NotFoundByLookup(
+                                ResourceType::IpPoolResource,
+                                // TODO: would be nice to put the actual names and/or ids in
+                                // here but LookupType on each of the two silos doesn't have
+                                // a nice to_string yet or a way of composing them
+                                LookupType::ByCompositeId(
+                                    "(pool, silo)".to_string(),
+                                ),
                             ),
                         ),
-                    )
+                    }
                 }
             })
     }
@@ -1269,12 +1318,13 @@ mod test {
     use std::num::NonZeroU32;
 
     use crate::authz;
+    use crate::db::datastore::ip_pool::INTERNAL_SILO_DEFAULT_ERROR;
     use crate::db::model::{
         IpPool, IpPoolResource, IpPoolResourceType, Project,
     };
     use crate::db::pub_test_utils::TestDatabase;
     use assert_matches::assert_matches;
-    use nexus_db_model::IpVersion;
+    use nexus_db_model::{IpPoolIdentity, IpVersion};
     use nexus_types::external_api::params;
     use nexus_types::identity::Resource;
     use omicron_common::address::{IpRange, Ipv4Range, Ipv6Range};
@@ -1283,6 +1333,7 @@ mod test {
         DataPageParams, Error, IdentityMetadataCreateParams, LookupType,
     };
     use omicron_test_utils::dev;
+    use uuid::Uuid;
 
     #[tokio::test]
     async fn test_default_ip_pools() {
@@ -1360,7 +1411,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");
 
@@ -1489,9 +1540,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 +1562,87 @@ 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 ip_version in [IpVersion::V4, IpVersion::V6] {
+            // Make some new pool.
+            let params = IpPool {
+                identity: IpPoolIdentity::new(
+                    Uuid::new_v4(),
+                    IdentityMetadataCreateParams {
+                        name: format!("test-pool-{}", ip_version)
+                            .parse()
+                            .unwrap(),
+                        description: String::new(),
+                    },
+                ),
+                ip_version,
+                rcgen: 0,
+            };
+            let pool = datastore
+                .ip_pool_create(&opctx, params)
+                .await
+                .expect("Should be able to create pool");
+            assert_eq!(pool.ip_version, ip_version);
+            let authz_pool =
+                nexus_db_lookup::LookupPath::new(&opctx, datastore)
+                    .ip_pool_id(pool.id())
+                    .lookup_for(authz::Action::Read)
+                    .await
+                    .expect("Should be able to lookup new IP Pool")
+                    .0;
+
+            // 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::InvalidRequest { message } = &e else {
+                panic!("should have received an invalid request, got: {:?}", e);
+            };
+            assert_eq!(message.external_message(), INTERNAL_SILO_DEFAULT_ERROR);
+
+            // 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::InvalidRequest { message } = &e else {
+                panic!("should have received an invalid request, got: {:?}", e);
+            };
+            assert_eq!(message.external_message(), INTERNAL_SILO_DEFAULT_ERROR);
+        }
+
+        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.
     //
diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/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?;
diff --git a/nexus/src/app/silo.rs b/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,
diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql
@@ -2110,7 +2110,17 @@ 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 (
+        NOT (
+            resource_type = 'silo' AND
+            resource_id = '001de000-5110-4000-8000-000000000001' AND
+            is_default
+        )
+    )
+
 );
 
 -- a given resource can only have one default ip pool
@@ -2123,6 +2133,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 +6613,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;
diff --git a/schema/crdb/no-default-pool-for-internal-silo/up01.sql b/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';
diff --git a/schema/crdb/no-default-pool-for-internal-silo/up02.sql b/schema/crdb/no-default-pool-for-internal-silo/up02.sql
@@ -0,0 +1,15 @@
+/*
+ * 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
+omicron.public.ip_pool_resource
+ADD CONSTRAINT IF NOT EXISTS
+internal_silo_has_no_default_pool CHECK (
+    NOT (
+        resource_type = 'silo' AND
+        resource_id = '001de000-5110-4000-8000-000000000001' AND
+        is_default
+    )
+);

Original file line number	Diff line number	Diff line change
`@@ -50,6 +50,7 @@ impl super::Nexus {`
`50`	`50`	`let silo = self.silo_lookup(opctx, NameOrId::Id(silo.id()))?;`
`51`	`51`	`Ok(silo)`
`52`	`52`	`}`
	`53`	`+`
`53`	`54`	`pub fn silo_lookup<'a>(`
`54`	`55`	`&'a self,`
`55`	`56`	`opctx: &'a OpContext,`