new default IP pool logic with fallback to higher levels

Author: david-crespo

nexus/db-model/src/ip_pool.rs

@@ -48,12 +48,15 @@ pub struct IpPool {
 
     /// Project, if IP pool is associated with a particular project
     pub project_id: Option<Uuid>,
+
+    pub default: bool,
 }
 
 impl IpPool {
     pub fn new(
         pool_identity: &external::IdentityMetadataCreateParams,
         silo_id: Option<Uuid>,
+        default: bool,
     ) -> Self {
         Self {
             identity: IpPoolIdentity::new(
@@ -63,6 +66,7 @@ impl IpPool {
             rcgen: 0,
             silo_id,
             project_id: None,
+            default,
         }
     }
 }

nexus/db-model/src/schema.rs

@@ -455,6 +455,7 @@ table! {
         rcgen -> Int8,
         silo_id -> Nullable<Uuid>,
         project_id -> Nullable<Uuid>,
+        default -> Bool,
     }
 }
 

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

@@ -58,6 +58,13 @@ impl DataStore {
         let name = pool_name.unwrap_or_else(|| {
             Name(ExternalName::from_str("default").unwrap())
         });
+
+        // TODO: this is the whole thing -- update this call to EITHER look up
+        // the pool by name (only allowing access to pools matching the scope of
+        // the current project, i.e., you can pick a pool by name only if it's
+        // scoped to your project, silo, or the whole fleet) or use the default
+        // logic in ip_pool_fetch_default_for
+
         let (.., pool) = self
             .ip_pools_fetch_for(opctx, authz::Action::CreateChild, &name)
             .await?;

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

@@ -37,11 +37,9 @@ use omicron_common::api::external::Error;
 use omicron_common::api::external::ListResultVec;
 use omicron_common::api::external::LookupResult;
 use omicron_common::api::external::LookupType;
-use omicron_common::api::external::Name as ExternalName;
 use omicron_common::api::external::ResourceType;
 use omicron_common::api::external::UpdateResult;
 use ref_cast::RefCast;
-use std::str::FromStr;
 use uuid::Uuid;
 
 impl DataStore {
@@ -74,18 +72,42 @@ impl DataStore {
         .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))
     }
 
-    /// Looks up the default IP pool by name.
+    /// Looks up the default IP pool for a given scope, i.e., a given
+    /// combination of silo and project ID (or none). If there is no default at
+    /// a given scope, fall back up a level. There should always be a default at
+    /// fleet level, though this query can theoretically fail.
     pub async fn ip_pools_fetch_default_for(
         &self,
         opctx: &OpContext,
         action: authz::Action,
-    ) -> LookupResult<(authz::IpPool, IpPool)> {
-        self.ip_pools_fetch_for(
-            opctx,
-            action,
-            &Name(ExternalName::from_str("default").unwrap()),
-        )
-        .await
+        silo_id: Option<Uuid>,
+        project_id: Option<Uuid>,
+    ) -> LookupResult<IpPool> {
+        use db::schema::ip_pool::dsl;
+        opctx.authorize(action, &authz::IP_POOL_LIST).await?;
+
+        dsl::ip_pool
+            .filter(dsl::silo_id.eq(silo_id).or(dsl::silo_id.is_null()))
+            .filter(
+                dsl::project_id.eq(project_id).or(dsl::project_id.is_null()),
+            )
+            .filter(dsl::default.eq(true))
+            .filter(dsl::time_deleted.is_null())
+            // this will sort by most specific first, i.e.,
+            //
+            //   (silo, project)
+            //   (silo, null)
+            //   (null, null)
+            //
+            // then by only taking the first result, we get the most specific one
+            .order((
+                dsl::project_id.asc().nulls_last(),
+                dsl::silo_id.asc().nulls_last(),
+            ))
+            .select(IpPool::as_select())
+            .first_async::<IpPool>(self.pool_authorized(opctx).await?)
+            .await
+            .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))
     }
 
     /// Looks up an IP pool by name.
@@ -429,3 +451,94 @@ impl DataStore {
         }
     }
 }
+
+#[cfg(test)]
+mod test {
+    use crate::authz;
+    use crate::db::datastore::datastore_test;
+    use crate::db::model::IpPool;
+    use nexus_test_utils::db::test_setup_database;
+    use nexus_types::identity::Resource;
+    use omicron_common::api::external::IdentityMetadataCreateParams;
+    use omicron_test_utils::dev;
+
+    #[tokio::test]
+    async fn test_default_ip_pools() {
+        let logctx = dev::test_setup_log("test_default_ip_pools");
+        let mut db = test_setup_database(&logctx.log).await;
+        let (opctx, datastore) = datastore_test(&logctx, &db).await;
+
+        let action = authz::Action::ListChildren;
+
+        // we start out with the default fleet-level pool already created,
+        // so when we ask for the fleet default (no silo or project) we get it back
+        let fleet_default_pool = datastore
+            .ip_pools_fetch_default_for(&opctx, action, None, None)
+            .await
+            .unwrap();
+
+        assert_eq!(fleet_default_pool.identity.name.as_str(), "default");
+        assert!(fleet_default_pool.default);
+        assert_eq!(fleet_default_pool.silo_id, None);
+        assert_eq!(fleet_default_pool.project_id, None);
+
+        // now the interesting thing is that when we fetch the default pool for
+        // a particular silo or a particular project, if those scopes do not
+        // have a default IP pool, we will still get back the fleet default
+
+        // default for "current" silo is still the fleet default one because it
+        // has no default of its own
+        let silo_id = opctx.authn.silo_required().unwrap().id();
+        let ip_pool = datastore
+            .ip_pools_fetch_default_for(&opctx, action, Some(silo_id), None)
+            .await
+            .expect("Failed to get silo's default IP pool");
+        assert_eq!(ip_pool.id(), fleet_default_pool.id());
+
+        // create a non-default pool for the silo
+        let identity = IdentityMetadataCreateParams {
+            name: "non-default-for-silo".parse().unwrap(),
+            description: "".to_string(),
+        };
+        let _ = datastore
+            .ip_pool_create(
+                &opctx,
+                IpPool::new(&identity, Some(silo_id), /*default= */ false),
+            )
+            .await;
+
+        // because that one was not a default, when we ask for silo default
+        // pool, we still get the fleet default
+        let ip_pool = datastore
+            .ip_pools_fetch_default_for(&opctx, action, Some(silo_id), None)
+            .await
+            .expect("Failed to get fleet default IP pool");
+        assert_eq!(ip_pool.id(), fleet_default_pool.id());
+
+        // now create a default pool for the silo
+        let identity = IdentityMetadataCreateParams {
+            name: "default-for-silo".parse().unwrap(),
+            description: "".to_string(),
+        };
+        let _ = datastore
+            .ip_pool_create(&opctx, IpPool::new(&identity, Some(silo_id), true))
+            .await;
+
+        // now when we ask for the silo default pool, we get the one we just made
+        let ip_pool = datastore
+            .ip_pools_fetch_default_for(&opctx, action, Some(silo_id), None)
+            .await
+            .expect("Failed to get silo's default IP pool");
+        assert_eq!(ip_pool.name().as_str(), "default-for-silo");
+
+        // and of course, if we ask for the fleet default again we still get that one
+        let ip_pool = datastore
+            .ip_pools_fetch_default_for(&opctx, action, None, None)
+            .await
+            .expect("Failed to get fleet default IP pool");
+        assert_eq!(ip_pool.id(), fleet_default_pool.id());
+
+        db.cleanup().await.unwrap();
+        logctx.cleanup_successful();
+    }
+}

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

@@ -563,6 +563,7 @@ impl DataStore {
                 description: String::from("IP Pool for Oxide Services"),
             },
             Some(*INTERNAL_SILO_ID),
+            true, // default for internal silo
         );
 
         self.ip_pool_create(opctx, internal_pool).await.map(|_| ()).or_else(
@@ -578,6 +579,7 @@ impl DataStore {
                 description: String::from("default IP pool"),
             },
             None, // no silo ID, fleet scoped
+            true, // default for fleet
         );
         self.ip_pool_create(opctx, default_pool).await.map(|_| ()).or_else(
             |e| match e {

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

@@ -869,6 +869,7 @@ mod tests {
                     description: format!("ip pool {}", name),
                 },
                 Some(*INTERNAL_SILO_ID),
+                true,
             );
 
             use crate::db::schema::ip_pool::dsl as ip_pool_dsl;
@@ -916,11 +917,13 @@ mod tests {
         }
 
         async fn default_pool_id(&self) -> Uuid {
-            let (.., pool) = self
+            let pool = self
                 .db_datastore
                 .ip_pools_fetch_default_for(
                     &self.opctx,
                     crate::authz::Action::ListChildren,
+                    None,
+                    None,
                 )
                 .await
                 .expect("Failed to lookup default ip pool");

nexus/src/app/ip_pool.rs

@@ -66,7 +66,11 @@ impl super::Nexus {
             }
             _ => None,
         };
-        let pool = db::model::IpPool::new(&pool_params.identity, silo_id);
+        let pool = db::model::IpPool::new(
+            &pool_params.identity,
+            silo_id,
+            pool_params.default,
+        );
         self.db_datastore.ip_pool_create(opctx, pool).await
     }
 

nexus/src/app/sagas/instance_create.rs

@@ -826,8 +826,14 @@ async fn sic_allocate_instance_snat_ip(
     let instance_id = sagactx.lookup::<Uuid>("instance_id")?;
     let ip_id = sagactx.lookup::<Uuid>("snat_ip_id")?;
 
-    let (.., pool) = datastore
-        .ip_pools_fetch_default_for(&opctx, authz::Action::CreateChild)
+    let pool = datastore
+        .ip_pools_fetch_default_for(
+            &opctx,
+            authz::Action::CreateChild,
+            // TODO: get these values right
+            None,
+            None,
+        )
         .await
         .map_err(ActionError::action_failed)?;
     let pool_id = pool.identity.id;

nexus/test-utils/src/resource_helpers.rs

@@ -141,6 +141,7 @@ pub async fn create_ip_pool(
                 description: String::from("an ip pool"),
             },
             silo: None,
+            default: false,
         },
     )
     .await;

nexus/tests/integration_tests/endpoints.rs

@@ -455,6 +455,8 @@ lazy_static! {
                 description: String::from("an IP pool"),
             },
             silo: None,
+            // TODO: this might error due to the unique index
+            default: true,
         };
     pub static ref DEMO_IP_POOL_PROJ_URL: String =
         format!("/v1/ip-pools/{}?project={}", *DEMO_IP_POOL_NAME, *DEMO_PROJECT_NAME);