change internal column to use silo id, add and populate default column

Author: david-crespo

Cargo.lock

@@ -305,6 +305,7 @@ sha3 = "0.10.8"
 shell-words = "1.1.0"
 signal-hook = "0.3"
 signal-hook-tokio = { version = "0.3", features = [ "futures-v0_3" ] }
+similar-asserts = "1.5.0"
 sled = "0.34"
 sled-agent-client = { path = "sled-agent-client" }
 sled-hardware = { path = "sled-hardware" }

Cargo.toml

@@ -68,6 +68,7 @@ serde.workspace = true
 serde_json.workspace = true
 serde_urlencoded.workspace = true
 serde_with.workspace = true
+similar-asserts.workspace = true
 sled-agent-client.workspace = true
 slog.workspace = true
 slog-async.workspace = true

nexus/Cargo.toml

@@ -32,39 +32,34 @@ pub struct IpPool {
     #[diesel(embed)]
     pub identity: IpPoolIdentity,
 
-    /// If true, identifies that this IP pool is dedicated to "Control-Plane
-    /// Services", such as Nexus.
-    ///
-    /// Otherwise, this IP pool is intended for usage by customer VMs.
-    pub internal: bool,
-
     /// Child resource generation number, for optimistic concurrency control of
     /// the contained ranges.
     pub rcgen: i64,
 
-    /// Silo, if IP pool is associated with a particular silo. Must be null
-    /// if internal is true. Must be non-null if project_id is non-null. When
-    /// project_id is non-null, silo_id will (naturally) be the ID of the
+    /// Silo, if IP pool is associated with a particular silo. One special use
+    /// for this is  associating a pool with the internal silo oxide-internal,
+    /// which is used for internal services. If there is no silo ID, the
+    /// pool is considered a fleet-wide silo and will be used for allocating
+    /// instance IPs in silos that don't have their own pool. Must be non-
+    /// null if project_id is non-null (this is enforced as a DB constraint).
+    /// When project_id is non-null, silo_id will (naturally) be the ID of the
     /// project's silo.
     pub silo_id: Option<Uuid>,
 
-    /// Project, if IP pool is associated with a particular project. Must be
-    /// null if internal is true.
+    /// Project, if IP pool is associated with a particular project
     pub project_id: Option<Uuid>,
 }
 
 impl IpPool {
     pub fn new(
         pool_identity: &external::IdentityMetadataCreateParams,
-        internal: bool,
         silo_id: Option<Uuid>,
     ) -> Self {
         Self {
             identity: IpPoolIdentity::new(
                 Uuid::new_v4(),
                 pool_identity.clone(),
             ),
-            internal,
             rcgen: 0,
             silo_id,
             project_id: None,

nexus/db-model/src/ip_pool.rs

@@ -452,7 +452,6 @@ table! {
         time_created -> Timestamptz,
         time_modified -> Timestamptz,
         time_deleted -> Nullable<Timestamptz>,
-        internal -> Bool,
         rcgen -> Int8,
         silo_id -> Nullable<Uuid>,
         project_id -> Nullable<Uuid>,
@@ -1131,7 +1130,7 @@ table! {
 ///
 /// This should be updated whenever the schema is changed. For more details,
 /// refer to: schema/crdb/README.adoc
-pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(3, 0, 0);
+pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(3, 0, 2);
 
 allow_tables_to_appear_in_same_query!(
     system_update,

nexus/db-model/src/schema.rs

@@ -14,6 +14,7 @@ use crate::db::collection_insert::DatastoreCollection;
 use crate::db::error::diesel_pool_result_optional;
 use crate::db::error::public_error_from_diesel_pool;
 use crate::db::error::ErrorHandler;
+use crate::db::fixed_data::silo::INTERNAL_SILO_ID;
 use crate::db::identity::Resource;
 use crate::db::lookup::LookupPath;
 use crate::db::model::IpPool;
@@ -27,7 +28,6 @@ use async_bb8_diesel::{AsyncRunQueryDsl, PoolError};
 use chrono::Utc;
 use diesel::prelude::*;
 use ipnetwork::IpNetwork;
-use nexus_types::external_api::params;
 use nexus_types::external_api::shared::IpRange;
 use omicron_common::api::external::http_pagination::PaginatedBy;
 use omicron_common::api::external::CreateResult;
@@ -65,7 +65,8 @@ impl DataStore {
                 &pagparams.map_name(|n| Name::ref_cast(n)),
             ),
         }
-        .filter(dsl::internal.eq(false))
+        // != excludes nulls so we explicitly include them
+        .filter(dsl::silo_id.ne(*INTERNAL_SILO_ID).or(dsl::silo_id.is_null()))
         .filter(dsl::time_deleted.is_null())
         .select(db::model::IpPool::as_select())
         .get_results_async(self.pool_authorized(opctx).await?)
@@ -98,7 +99,8 @@ impl DataStore {
             .ip_pool_name(&name)
             .fetch_for(action)
             .await?;
-        if pool.internal {
+        // Can't look up the internal pool
+        if pool.silo_id == Some(*INTERNAL_SILO_ID) {
             return Err(authz_pool.not_found());
         }
 
@@ -120,7 +122,7 @@ impl DataStore {
 
         // Look up this IP pool by rack ID.
         let (authz_pool, pool) = dsl::ip_pool
-            .filter(dsl::internal.eq(true))
+            .filter(dsl::silo_id.eq(*INTERNAL_SILO_ID))
             .filter(dsl::time_deleted.is_null())
             .select(IpPool::as_select())
             .get_result_async(self.pool_authorized(opctx).await?)
@@ -142,20 +144,15 @@ impl DataStore {
     }
 
     /// Creates a new IP pool.
-    ///
-    /// - If `internal` is set, this IP pool is used for Oxide services.
     pub async fn ip_pool_create(
         &self,
         opctx: &OpContext,
-        new_pool: &params::IpPoolCreate,
-        internal: bool,
-        silo_id: Option<Uuid>,
+        pool: IpPool,
     ) -> CreateResult<IpPool> {
         use db::schema::ip_pool::dsl;
         opctx
             .authorize(authz::Action::CreateChild, &authz::IP_POOL_LIST)
             .await?;
-        let pool = IpPool::new(&new_pool.identity, internal, silo_id);
         let pool_name = pool.name().as_str().to_string();
 
         diesel::insert_into(dsl::ip_pool)
@@ -205,7 +202,10 @@ impl DataStore {
         // in between the above check for children and this query.
         let now = Utc::now();
         let updated_rows = diesel::update(dsl::ip_pool)
-            .filter(dsl::internal.eq(false))
+            // != excludes nulls so we explicitly include them
+            .filter(
+                dsl::silo_id.ne(*INTERNAL_SILO_ID).or(dsl::silo_id.is_null()),
+            )
             .filter(dsl::time_deleted.is_null())
             .filter(dsl::id.eq(authz_pool.id()))
             .filter(dsl::rcgen.eq(db_pool.rcgen))
@@ -237,7 +237,10 @@ impl DataStore {
         use db::schema::ip_pool::dsl;
         opctx.authorize(authz::Action::Modify, authz_pool).await?;
         diesel::update(dsl::ip_pool)
-            .filter(dsl::internal.eq(false))
+            // != excludes nulls so we explicitly include them
+            .filter(
+                dsl::silo_id.ne(*INTERNAL_SILO_ID).or(dsl::silo_id.is_null()),
+            )
             .filter(dsl::id.eq(authz_pool.id()))
             .filter(dsl::time_deleted.is_null())
             .set(updates)

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

@@ -351,7 +351,8 @@ impl DataStore {
         }
         // TODO(2148, 2056): filter only pools accessible by the given
         // project, once specific projects for pools are implemented
-        .filter(dsl::internal.eq(false))
+        // != excludes nulls so we explicitly include them
+        .filter(dsl::silo_id.ne(*INTERNAL_SILO_ID).or(dsl::silo_id.is_null()))
         .filter(dsl::time_deleted.is_null())
         .select(db::model::IpPool::as_select())
         .get_results_async(self.pool_authorized(opctx).await?)

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

@@ -15,6 +15,7 @@ use crate::db::collection_insert::DatastoreCollection;
 use crate::db::error::public_error_from_diesel_pool;
 use crate::db::error::ErrorHandler;
 use crate::db::error::TransactionError;
+use crate::db::fixed_data::silo::INTERNAL_SILO_ID;
 use crate::db::fixed_data::vpc_subnet::DNS_VPC_SUBNET;
 use crate::db::fixed_data::vpc_subnet::NEXUS_VPC_SUBNET;
 use crate::db::fixed_data::vpc_subnet::NTP_VPC_SUBNET;
@@ -556,39 +557,34 @@ impl DataStore {
 
         self.rack_insert(opctx, &db::model::Rack::new(rack_id)).await?;
 
-        let params = external_params::IpPoolCreate {
-            identity: IdentityMetadataCreateParams {
+        let internal_pool = db::model::IpPool::new(
+            &IdentityMetadataCreateParams {
                 name: SERVICE_IP_POOL_NAME.parse::<Name>().unwrap(),
                 description: String::from("IP Pool for Oxide Services"),
             },
-            silo: None,
-        };
-        self.ip_pool_create(
-            opctx, &params, /*internal=*/ true, /*silo_id*/ None,
-        )
-        .await
-        .map(|_| ())
-        .or_else(|e| match e {
-            Error::ObjectAlreadyExists { .. } => Ok(()),
-            _ => Err(e),
-        })?;
+            Some(*INTERNAL_SILO_ID),
+        );
+
+        self.ip_pool_create(opctx, internal_pool).await.map(|_| ()).or_else(
+            |e| match e {
+                Error::ObjectAlreadyExists { .. } => Ok(()),
+                _ => Err(e),
+            },
+        )?;
 
-        let params = external_params::IpPoolCreate {
-            identity: IdentityMetadataCreateParams {
+        let default_pool = db::model::IpPool::new(
+            &IdentityMetadataCreateParams {
                 name: "default".parse::<Name>().unwrap(),
                 description: String::from("default IP pool"),
             },
-            silo: None,
-        };
-        self.ip_pool_create(
-            opctx, &params, /*internal=*/ false, /*silo_id*/ None,
-        )
-        .await
-        .map(|_| ())
-        .or_else(|e| match e {
-            Error::ObjectAlreadyExists { .. } => Ok(()),
-            _ => Err(e),
-        })?;
+            None, // no silo ID, fleet scoped
+        );
+        self.ip_pool_create(opctx, default_pool).await.map(|_| ()).or_else(
+            |e| match e {
+                Error::ObjectAlreadyExists { .. } => Ok(()),
+                _ => Err(e),
+            },
+        )?;
 
         Ok(())
     }
@@ -1104,7 +1100,7 @@ mod test {
         // been allocated as a part of the service IP pool.
         let (.., svc_pool) =
             datastore.ip_pools_service_lookup(&opctx).await.unwrap();
-        assert!(svc_pool.internal);
+        assert_eq!(svc_pool.silo_id, Some(*INTERNAL_SILO_ID));
 
         let observed_ip_pool_ranges = get_all_ip_pool_ranges(&datastore).await;
         assert_eq!(observed_ip_pool_ranges.len(), 1);
@@ -1306,7 +1302,7 @@ mod test {
         // allocated as a part of the service IP pool.
         let (.., svc_pool) =
             datastore.ip_pools_service_lookup(&opctx).await.unwrap();
-        assert!(svc_pool.internal);
+        assert_eq!(svc_pool.silo_id, Some(*INTERNAL_SILO_ID));
 
         let observed_ip_pool_ranges = get_all_ip_pool_ranges(&datastore).await;
         assert_eq!(observed_ip_pool_ranges.len(), 1);

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

@@ -816,6 +816,7 @@ mod tests {
     use crate::context::OpContext;
     use crate::db::datastore::DataStore;
     use crate::db::datastore::SERVICE_IP_POOL_NAME;
+    use crate::db::fixed_data::silo::INTERNAL_SILO_ID;
     use crate::db::identity::Resource;
     use crate::db::model::IpKind;
     use crate::db::model::IpPool;
@@ -862,14 +863,12 @@ mod tests {
         }
 
         async fn create_ip_pool(&self, name: &str, range: IpRange) {
-            let internal = false;
             let pool = IpPool::new(
                 &IdentityMetadataCreateParams {
                     name: String::from(name).parse().unwrap(),
                     description: format!("ip pool {}", name),
                 },
-                internal,
-                None, // silo id
+                Some(*INTERNAL_SILO_ID),
             );
 
             use crate::db::schema::ip_pool::dsl as ip_pool_dsl;