Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up IP pool resolution from NameOrId #5674

Merged
merged 3 commits into from
May 1, 2024
Merged

Clean up IP pool resolution from NameOrId #5674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4224404
avoid a DB fetch during floating IP allocation. sort of
david-crespo Apr 30, 2024
3b98875
extract resolve_pool_for_allocation function
david-crespo Apr 30, 2024
a4183a1
do the same thing for probe IP allocation
david-crespo Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 39 additions & 78 deletions nexus/db-queries/src/db/datastore/external_ip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting this out of here is a good thing

use omicron_common::api::external::ResourceType;
use omicron_common::api::external::UpdateResult;
use omicron_uuid_kinds::OmicronZoneUuid;
Expand Down Expand Up @@ -84,33 +83,14 @@ impl DataStore {
opctx: &OpContext,
ip_id: Uuid,
probe_id: Uuid,
pool_name: Option<NameOrId>,
pool: Option<authz::IpPool>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind giving allocate_instance_snat_ip similar treatment so we're consistent across everything here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes. I didn’t look hard enough.

Copy link
Contributor Author

@david-crespo david-crespo May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting this into yet another PR (#5678) because it fixes #5043 and changes semantics.

) -> CreateResult<ExternalIp> {
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
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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<authz::IpPool>,
) -> LookupResult<authz::IpPool> {
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,
Expand All @@ -239,29 +222,7 @@ impl DataStore {
) -> CreateResult<ExternalIp> {
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?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fetch was not necessary at all — replaced by opctx.authorize call in resolve_pool_for_allocation


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(
Expand All @@ -270,15 +231,15 @@ impl DataStore {
&identity.description,
project_id,
ip,
pool_id,
authz_pool.id(),
)
} else {
IncompleteExternalIp::for_floating(
ip_id,
&Name(identity.name),
&identity.description,
project_id,
pool_id,
authz_pool.id(),
)
};

Expand Down
8 changes: 3 additions & 5 deletions nexus/db-queries/src/db/datastore/probe.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting this out of here is a good thing

use nexus_types::identity::Resource;
use omicron_common::api::external::http_pagination::PaginatedBy;
use omicron_common::api::external::CreateResult;
Expand Down Expand Up @@ -278,20 +277,19 @@ impl super::DataStore {
&self,
opctx: &OpContext,
authz_project: &authz::Project,
new_probe: &params::ProbeCreate,
probe: &Probe,
ip_pool: Option<authz::IpPool>,
) -> CreateResult<Probe> {
//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?;

Expand Down
5 changes: 1 addition & 4 deletions nexus/src/app/external_ip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
15 changes: 14 additions & 1 deletion nexus/src/app/probe.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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) =
Expand Down
Loading