Clean up IP pool resolution from NameOrId
#5674
Conversation
| @@ -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; | |||
There was a problem hiding this comment.
getting this out of here is a good thing
| @@ -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; | |||
There was a problem hiding this comment.
getting this out of here is a good thing
| let (.., pool) = LookupPath::new(opctx, &self) | ||
| .ip_pool_id(authz_pool.id()) | ||
| .fetch_for(authz::Action::CreateChild) | ||
| .await?; |
There was a problem hiding this comment.
this fetch was not necessary at all — replaced by opctx.authorize call in resolve_pool_for_allocation
| .0, | ||
| ), | ||
| None => None, | ||
| }; |
There was a problem hiding this comment.
While overall I think this PR is a net improvement already, I might try to roll up this logic further with what's in resolve_pool_for_allocation because it is awkward to split up this part of the resolution of the pool from the rest.
| @@ -84,33 +83,14 @@ impl DataStore { | |||
| opctx: &OpContext, | |||
| ip_id: Uuid, | |||
| probe_id: Uuid, | |||
| pool_name: Option<NameOrId>, | |||
| pool: Option<authz::IpPool>, | |||
There was a problem hiding this comment.
Mind giving allocate_instance_snat_ip similar treatment so we're consistent across everything here
There was a problem hiding this comment.
Ah yes. I didn’t look hard enough.
Followup to #5660, specifically #5660 (comment). I didn't do it quite as suggested, basically because I think datastore functions being aware of
NameOrIdis a bad thing as that is, to me, an API concept that we should have resolved to something real by the time we get to the datastore functions. This is still a little odd and a little redundant, but it is cleaner and more consistent.In each case (floating IP create, probe create, instance ephemeral IP allocated) we are doing the following:
Option<NameOrId>intoOption<authz::IpPool>(this is a DB query of course)resolve_pool_for_allocation, whicha. If a pool is specified, makes sure it's linked to the current silo, otherwise 404
b. If a pool is not specified, get the default pool for the current silo
c. Either way, make sure we have
CreateChildon the pool (we always do if we're authenticated at all, but that could change in the future)authz::IpPooland can get on with whatever allocation