diff --git a/nexus/db-queries/src/db/datastore/external_ip.rs b/nexus/db-queries/src/db/datastore/external_ip.rs index b8d8fe8829..c3cd45669f 100644 --- a/nexus/db-queries/src/db/datastore/external_ip.rs +++ b/nexus/db-queries/src/db/datastore/external_ip.rs @@ -50,7 +50,6 @@ use omicron_common::api::external::Error; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; -use omicron_common::api::external::NameOrId; use omicron_common::api::external::ResourceType; use omicron_common::api::external::UpdateResult; use omicron_uuid_kinds::OmicronZoneUuid; @@ -84,33 +83,14 @@ impl DataStore { opctx: &OpContext, ip_id: Uuid, probe_id: Uuid, - pool_name: Option, + pool: Option, ) -> CreateResult { - let pool = match pool_name { - Some(NameOrId::Name(name)) => { - let (.., pool) = LookupPath::new(opctx, &self) - .ip_pool_name(&name.into()) - .fetch_for(authz::Action::CreateChild) - .await?; - pool - } - Some(NameOrId::Id(id)) => { - let (.., pool) = LookupPath::new(opctx, &self) - .ip_pool_id(id) - .fetch_for(authz::Action::CreateChild) - .await?; - pool - } - // If no name given, use the default pool - None => { - let (.., pool) = self.ip_pools_fetch_default(&opctx).await?; - pool - } - }; - - let pool_id = pool.identity.id; - let data = - IncompleteExternalIp::for_ephemeral_probe(ip_id, probe_id, pool_id); + let authz_pool = self.resolve_pool_for_allocation(&opctx, pool).await?; + let data = IncompleteExternalIp::for_ephemeral_probe( + ip_id, + probe_id, + authz_pool.id(), + ); self.allocate_external_ip(opctx, data).await } @@ -140,33 +120,9 @@ impl DataStore { // - At most MAX external IPs per instance // Naturally, we now *need* to destroy the ephemeral IP if the newly alloc'd // IP was not attached, including on idempotent success. - let pool = match pool { - Some(authz_pool) => { - let (.., pool) = LookupPath::new(opctx, &self) - .ip_pool_id(authz_pool.id()) - // any authenticated user can CreateChild on an IP pool. this is - // meant to represent allocating an IP - .fetch_for(authz::Action::CreateChild) - .await?; - - // If this pool is not linked to the current silo, 404 - // As name resolution happens one layer up, we need to use the *original* - // authz Pool. - if self.ip_pool_fetch_link(opctx, pool.id()).await.is_err() { - return Err(authz_pool.not_found()); - } - pool - } - // If no name given, use the default logic - None => { - let (.., pool) = self.ip_pools_fetch_default(&opctx).await?; - pool - } - }; - - let pool_id = pool.identity.id; - let data = IncompleteExternalIp::for_ephemeral(ip_id, pool_id); + let authz_pool = self.resolve_pool_for_allocation(&opctx, pool).await?; + let data = IncompleteExternalIp::for_ephemeral(ip_id, authz_pool.id()); // We might not be able to acquire a new IP, but in the event of an // idempotent or double attach this failure is allowed. @@ -228,6 +184,33 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } + /// If a pool is specified, make sure it's linked to this silo. If a pool is + /// not specified, fetch the default pool for this silo. Once the pool is + /// resolved (by either method) do an auth check. Then return the pool. + async fn resolve_pool_for_allocation( + &self, + opctx: &OpContext, + pool: Option, + ) -> LookupResult { + let authz_pool = match pool { + Some(authz_pool) => { + self.ip_pool_fetch_link(opctx, authz_pool.id()) + .await + .map_err(|_| authz_pool.not_found())?; + + authz_pool + } + // If no pool specified, use the default logic + None => { + let (authz_pool, ..) = + self.ip_pools_fetch_default(&opctx).await?; + authz_pool + } + }; + opctx.authorize(authz::Action::CreateChild, &authz_pool).await?; + Ok(authz_pool) + } + /// Allocates a floating IP address for instance usage. pub async fn allocate_floating_ip( &self, @@ -239,29 +222,7 @@ impl DataStore { ) -> CreateResult { let ip_id = Uuid::new_v4(); - // This implements the same pattern as in `allocate_instance_ephemeral_ip` to - // check that a chosen pool is valid from within the current silo. - let pool = match pool { - Some(authz_pool) => { - let (.., pool) = LookupPath::new(opctx, &self) - .ip_pool_id(authz_pool.id()) - .fetch_for(authz::Action::CreateChild) - .await?; - - if self.ip_pool_fetch_link(opctx, pool.id()).await.is_err() { - return Err(authz_pool.not_found()); - } - - pool - } - // If no name given, use the default logic - None => { - let (.., pool) = self.ip_pools_fetch_default(&opctx).await?; - pool - } - }; - - let pool_id = pool.id(); + let authz_pool = self.resolve_pool_for_allocation(&opctx, pool).await?; let data = if let Some(ip) = ip { IncompleteExternalIp::for_floating_explicit( @@ -270,7 +231,7 @@ impl DataStore { &identity.description, project_id, ip, - pool_id, + authz_pool.id(), ) } else { IncompleteExternalIp::for_floating( @@ -278,7 +239,7 @@ impl DataStore { &Name(identity.name), &identity.description, project_id, - pool_id, + authz_pool.id(), ) }; diff --git a/nexus/db-queries/src/db/datastore/probe.rs b/nexus/db-queries/src/db/datastore/probe.rs index f1e737e353..a96f857163 100644 --- a/nexus/db-queries/src/db/datastore/probe.rs +++ b/nexus/db-queries/src/db/datastore/probe.rs @@ -15,7 +15,6 @@ use diesel::{ExpressionMethods, QueryDsl, SelectableHelper}; use nexus_db_model::IncompleteNetworkInterface; use nexus_db_model::Probe; use nexus_db_model::VpcSubnet; -use nexus_types::external_api::params; use nexus_types::identity::Resource; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::CreateResult; @@ -278,20 +277,19 @@ impl super::DataStore { &self, opctx: &OpContext, authz_project: &authz::Project, - new_probe: ¶ms::ProbeCreate, + probe: &Probe, + ip_pool: Option, ) -> CreateResult { //TODO in transaction use db::schema::probe::dsl; let pool = self.pool_connection_authorized(opctx).await?; - let probe = Probe::from_create(new_probe, authz_project.id()); - let _eip = self .allocate_probe_ephemeral_ip( opctx, Uuid::new_v4(), probe.id(), - new_probe.ip_pool.clone().map(Into::into), + ip_pool, ) .await?; diff --git a/nexus/src/app/external_ip.rs b/nexus/src/app/external_ip.rs index 09142d239e..62b8c4cbd6 100644 --- a/nexus/src/app/external_ip.rs +++ b/nexus/src/app/external_ip.rs @@ -111,13 +111,10 @@ impl super::Nexus { let params::FloatingIpCreate { identity, pool, ip } = params; + // resolve NameOrId into authz::IpPool let pool = match pool { Some(pool) => Some( self.ip_pool_lookup(opctx, &pool)? - // every authenticated user has CreateChild on IP pools - // because they need to be able to allocate IPs from them. - // The check that the pool is linked to the current silo - // happens inside allocate_floating_ip .lookup_for(authz::Action::CreateChild) .await? .0, diff --git a/nexus/src/app/probe.rs b/nexus/src/app/probe.rs index e85c040a28..41ea4eece2 100644 --- a/nexus/src/app/probe.rs +++ b/nexus/src/app/probe.rs @@ -60,9 +60,22 @@ impl super::Nexus { let (.., authz_project) = project_lookup.lookup_for(authz::Action::CreateChild).await?; + // resolve NameOrId into authz::IpPool + let pool = match &new_probe_params.ip_pool { + Some(pool) => Some( + self.ip_pool_lookup(opctx, &pool)? + .lookup_for(authz::Action::CreateChild) + .await? + .0, + ), + None => None, + }; + + let new_probe = + Probe::from_create(new_probe_params, authz_project.id()); let probe = self .db_datastore - .probe_create(opctx, &authz_project, new_probe_params) + .probe_create(opctx, &authz_project, &new_probe, pool) .await?; let (.., sled) =