Small IP pools fixes (#4007)

Followup to #3985, closes #4005.

- Add `is_default` to IP pool response
- Inline `ip_pools_fetch` into the one callsite and delete it (per
discussion
#3985 (comment))
- Other small test tweaks suggested by @luqmana

Author: david-crespo

nexus/db-model/src/ip_pool.rs

@@ -66,7 +66,11 @@ impl IpPool {
 
 impl From<IpPool> for views::IpPool {
     fn from(pool: IpPool) -> Self {
-        Self { identity: pool.identity(), silo_id: pool.silo_id }
+        Self {
+            identity: pool.identity(),
+            silo_id: pool.silo_id,
+            is_default: pool.is_default,
+        }
     }
 }
 

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

@@ -5,10 +5,13 @@
 //! [`DataStore`] methods on [`ExternalIp`]s.
 
 use super::DataStore;
+use crate::authz;
+use crate::authz::ApiResource;
 use crate::context::OpContext;
 use crate::db;
 use crate::db::error::public_error_from_diesel_pool;
 use crate::db::error::ErrorHandler;
+use crate::db::lookup::LookupPath;
 use crate::db::model::ExternalIp;
 use crate::db::model::IncompleteExternalIp;
 use crate::db::model::IpKind;
@@ -52,11 +55,29 @@ impl DataStore {
         instance_id: Uuid,
         pool_name: Option<Name>,
     ) -> CreateResult<ExternalIp> {
-        // If we have a pool name, look up the pool by name and return it
-        // as long as its scopes don't conflict with the current scope.
-        // Otherwise, not found.
         let pool = match pool_name {
-            Some(name) => self.ip_pools_fetch(&opctx, &name).await?,
+            Some(name) => {
+                let (.., authz_pool, pool) = LookupPath::new(opctx, &self)
+                    .ip_pool_name(&name)
+                    // any authenticated user can CreateChild on an IP pool. this is
+                    // meant to represent allocating an IP
+                    .fetch_for(authz::Action::CreateChild)
+                    .await?;
+
+                // If the named pool conflicts with user's current scope, i.e.,
+                // if it has a silo and it's different from the current silo,
+                // then as far as IP allocation is concerned, that pool doesn't
+                // exist. If the pool has no silo, it's fleet-scoped and can
+                // always be used.
+                let authz_silo_id = opctx.authn.silo_required()?.id();
+                if let Some(pool_silo_id) = pool.silo_id {
+                    if pool_silo_id != authz_silo_id {
+                        return Err(authz_pool.not_found());
+                    }
+                }
+
+                pool
+            }
             // If no name given, use the default logic
             None => self.ip_pools_fetch_default(&opctx).await?,
         };

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

@@ -6,7 +6,6 @@
 
 use super::DataStore;
 use crate::authz;
-use crate::authz::ApiResource;
 use crate::context::OpContext;
 use crate::db;
 use crate::db::collection_insert::AsyncInsertError;
@@ -16,7 +15,6 @@ 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;
 use crate::db::model::IpPoolRange;
 use crate::db::model::IpPoolUpdate;
@@ -111,32 +109,6 @@ impl DataStore {
             .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))
     }
 
-    /// Looks up an IP pool by name if it does not conflict with your current scope.
-    pub(crate) async fn ip_pools_fetch(
-        &self,
-        opctx: &OpContext,
-        name: &Name,
-    ) -> LookupResult<IpPool> {
-        let authz_silo_id = opctx.authn.silo_required()?.id();
-
-        let (.., authz_pool, pool) = LookupPath::new(opctx, &self)
-            .ip_pool_name(&name)
-            // any authenticated user can CreateChild on an IP pool. this is
-            // meant to represent allocating an IP
-            .fetch_for(authz::Action::CreateChild)
-            .await?;
-
-        // You can't look up a pool by name if it conflicts with your current
-        // scope, i.e., if it has a silo it is different from your current silo
-        if let Some(pool_silo_id) = pool.silo_id {
-            if pool_silo_id != authz_silo_id {
-                return Err(authz_pool.not_found());
-            }
-        }
-
-        Ok(pool)
-    }
-
     /// Looks up an IP pool intended for internal services.
     ///
     /// This method may require an index by Availability Zone in the future.
@@ -465,7 +437,6 @@ mod test {
     use crate::db::datastore::datastore_test;
     use crate::db::model::IpPool;
     use assert_matches::assert_matches;
-    use nexus_db_model::Name;
     use nexus_test_utils::db::test_setup_database;
     use nexus_types::identity::Resource;
     use omicron_common::api::external::{Error, IdentityMetadataCreateParams};
@@ -510,12 +481,13 @@ mod test {
             name: "non-default-for-silo".parse().unwrap(),
             description: "".to_string(),
         };
-        let _ = datastore
+        datastore
             .ip_pool_create(
                 &opctx,
                 IpPool::new(&identity, Some(silo_id), /*default= */ false),
             )
-            .await;
+            .await
+            .expect("Failed to create silo non-default IP pool");
 
         // because that one was not a default, when we ask for the silo default
         // pool, we still get the fleet default
@@ -530,9 +502,10 @@ mod test {
             name: "default-for-silo".parse().unwrap(),
             description: "".to_string(),
         };
-        let _ = datastore
+        datastore
             .ip_pool_create(&opctx, IpPool::new(&identity, Some(silo_id), true))
-            .await;
+            .await
+            .expect("Failed to create silo default IP pool");
 
         // now when we ask for the default pool, we get the one we just made
         let ip_pool = datastore
@@ -541,13 +514,6 @@ mod test {
             .expect("Failed to get silo's default IP pool");
         assert_eq!(ip_pool.name().as_str(), "default-for-silo");
 
-        // if we ask for the fleet default by name, we can still get that one
-        let ip_pool = datastore
-            .ip_pools_fetch(&opctx, &Name("default".parse().unwrap()))
-            .await
-            .expect("Failed to get fleet default IP pool");
-        assert_eq!(ip_pool.id(), fleet_default_pool.id());
-
         // and we can't create a second default pool for the silo
         let identity = IdentityMetadataCreateParams {
             name: "second-default-for-silo".parse().unwrap(),

nexus/tests/integration_tests/ip_pools.rs

@@ -61,6 +61,8 @@ async fn test_ip_pool_basic_crud(cptestctx: &ControlPlaneTestContext) {
     assert_eq!(ip_pools.len(), 1, "Expected to see default IP pool");
 
     assert_eq!(ip_pools[0].identity.name, "default",);
+    assert_eq!(ip_pools[0].silo_id, None);
+    assert!(ip_pools[0].is_default);
 
     // Verify 404 if the pool doesn't exist yet, both for creating or deleting
     let error: HttpErrorResponseBody = NexusRequest::expect_failure(
@@ -288,9 +290,9 @@ async fn test_ip_pool_with_silo(cptestctx: &ControlPlaneTestContext) {
     };
     let created_pool = create_pool(client, &params).await;
     assert_eq!(created_pool.identity.name, "p0");
-    assert!(created_pool.silo_id.is_some());
 
-    let silo_id = created_pool.silo_id.unwrap();
+    let silo_id =
+        created_pool.silo_id.expect("Expected pool to have a silo_id");
 
     // now we'll create another IP pool using that silo ID
     let params = IpPoolCreate {

nexus/types/src/external_api/params.rs

@@ -736,7 +736,7 @@ pub struct IpPoolCreate {
     /// silo can draw from that pool.
     pub silo: Option<NameOrId>,
 
-    /// Whether the IP pool is considered a default pool for its scope (fleet,
+    /// Whether the IP pool is considered a default pool for its scope (fleet
     /// or silo). If a pool is marked default and is associated with a silo,
     /// instances created in that silo will draw IPs from that pool unless
     /// another pool is specified at instance create time.

nexus/types/src/external_api/views.rs

@@ -245,6 +245,7 @@ pub struct IpPool {
     #[serde(flatten)]
     pub identity: IdentityMetadata,
     pub silo_id: Option<Uuid>,
+    pub is_default: bool,
 }
 
 #[derive(Clone, Copy, Debug, Deserialize, Serialize, JsonSchema)]

openapi/nexus.json

@@ -10133,6 +10133,9 @@
             "type": "string",
             "format": "uuid"
           },
+          "is_default": {
+            "type": "boolean"
+          },
           "name": {
             "description": "unique, mutable, user-controlled identifier for each resource",
             "allOf": [
@@ -10160,6 +10163,7 @@
         "required": [
           "description",
           "id",
+          "is_default",
           "name",
           "time_created",
           "time_modified"
@@ -10173,7 +10177,7 @@
             "type": "string"
           },
           "is_default": {
-            "description": "Whether the IP pool is considered a default pool for its scope (fleet, or silo). If a pool is marked default and is associated with a silo, instances created in that silo will draw IPs from that pool unless another pool is specified at instance create time.",
+            "description": "Whether the IP pool is considered a default pool for its scope (fleet or silo). If a pool is marked default and is associated with a silo, instances created in that silo will draw IPs from that pool unless another pool is specified at instance create time.",
             "default": false,
             "type": "boolean"
           },