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

[nexus] Endpoint to list IP pools for silo, add is_default to silo-scoped IP pools list #4843

Merged
merged 6 commits into from
Jan 20, 2024
Merged

[nexus] Endpoint to list IP pools for silo, add is_default to silo-scoped IP pools list #4843

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
270133e
first pass at list IP pools for silo endpoint
david-crespo Jan 18, 2024
532dd5a
project IP pools endpoint also includes is_default
david-crespo Jan 18, 2024
06d1fea
/v1/ip-pools/{pool} responds with SiloIpPool too
david-crespo Jan 18, 2024
b489f09
test for ip pools for silo endpoint
david-crespo Jan 19, 2024
7685d47
views::IpPoolSilo -> views::IpPoolSiloLink, params::IpPoolSiloLink ->…
david-crespo Jan 19, 2024
9e8500d
share query to fetch pools for silo
david-crespo Jan 19, 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
4 changes: 2 additions & 2 deletions end-to-end-tests/src/bin/bootstrap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use end_to_end_tests::helpers::{generate_name, get_system_ip_pool};
use omicron_test_utils::dev::poll::{wait_for_condition, CondCheckError};
use oxide_client::types::{
ByteCount, DeviceAccessTokenRequest, DeviceAuthRequest, DeviceAuthVerify,
DiskCreate, DiskSource, IpPoolCreate, IpPoolSiloLink, IpRange, Ipv4Range,
DiskCreate, DiskSource, IpPoolCreate, IpPoolLinkSilo, IpRange, Ipv4Range,
NameOrId, SiloQuotasUpdate,
};
use oxide_client::{
Expand Down Expand Up @@ -51,7 +51,7 @@ async fn main() -> Result<()> {
client
.ip_pool_silo_link()
.pool(pool_name)
.body(IpPoolSiloLink {
.body(IpPoolLinkSilo {
silo: NameOrId::Name(params.silo_name().parse().unwrap()),
is_default: true,
})
Expand Down
2 changes: 1 addition & 1 deletion nexus/db-model/src/ip_pool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ pub struct IpPoolResource {
pub is_default: bool,
}

impl From<IpPoolResource> for views::IpPoolSilo {
impl From<IpPoolResource> for views::IpPoolSiloLink {
fn from(assoc: IpPoolResource) -> Self {
Self {
ip_pool_id: assoc.ip_pool_id,
Expand Down
57 changes: 44 additions & 13 deletions nexus/db-queries/src/db/datastore/ip_pool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,11 @@ impl DataStore {
}

/// List IP pools linked to the current silo
pub async fn silo_ip_pools_list(
pub async fn current_silo_ip_pool_list(
&self,
opctx: &OpContext,
pagparams: &PaginatedBy<'_>,
) -> ListResultVec<db::model::IpPool> {
) -> ListResultVec<(IpPool, IpPoolResource)> {
use db::schema::ip_pool;
use db::schema::ip_pool_resource;

Expand All @@ -108,13 +108,10 @@ impl DataStore {
),
}
.inner_join(ip_pool_resource::table)
.filter(
ip_pool_resource::resource_type
.eq(IpPoolResourceType::Silo)
.and(ip_pool_resource::resource_id.eq(silo_id)),
)
.filter(ip_pool_resource::resource_type.eq(IpPoolResourceType::Silo))
.filter(ip_pool_resource::resource_id.eq(silo_id))
.filter(ip_pool::time_deleted.is_null())
.select(db::model::IpPool::as_select())
.select(<(IpPool, IpPoolResource)>::as_select())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

neat trick. initially I tried (IpPool::as_select(), IpPoolResource::as_select()) but it wanted the above

.get_results_async(&*self.pool_connection_authorized(opctx).await?)
.await
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
Expand Down Expand Up @@ -400,6 +397,39 @@ impl DataStore {
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
}

/// Returns (IpPool, IpPoolResource) so we can know in the calling code
/// whether the pool is default for the silo
pub async fn silo_ip_pool_list(
&self,
opctx: &OpContext,
authz_silo: &authz::Silo,
pagparams: &PaginatedBy<'_>,
) -> ListResultVec<(IpPool, IpPoolResource)> {
use db::schema::ip_pool;
use db::schema::ip_pool_resource;

match pagparams {
PaginatedBy::Id(pagparams) => {
paginated(ip_pool::table, ip_pool::id, pagparams)
}
PaginatedBy::Name(pagparams) => paginated(
ip_pool::table,
ip_pool::name,
&pagparams.map_name(|n| Name::ref_cast(n)),
),
}
.inner_join(ip_pool_resource::table)
.filter(ip_pool_resource::resource_id.eq(authz_silo.id()))
.filter(ip_pool_resource::resource_type.eq(IpPoolResourceType::Silo))
.filter(ip_pool::time_deleted.is_null())
.select(<(IpPool, IpPoolResource)>::as_select())
.load_async::<(IpPool, IpPoolResource)>(
&*self.pool_connection_authorized(opctx).await?,
)
.await
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just realized this query is exactly the same as the one above — the only difference is whether you're passing in a silo or pulling the current silo from auth context. I will extract a shared function.

Copy link
Contributor

Choose a reason for hiding this comment

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

This came up in some of my work too. I just dropped the current version altogether and only left on that takes a given argument. The app layer can pass in the current silo when needed.


pub async fn ip_pool_link_silo(
&self,
opctx: &OpContext,
Expand Down Expand Up @@ -868,7 +898,7 @@ mod test {
.expect("Should list IP pools");
assert_eq!(all_pools.len(), 0);
let silo_pools = datastore
.silo_ip_pools_list(&opctx, &pagbyid)
.current_silo_ip_pool_list(&opctx, &pagbyid)
.await
.expect("Should list silo IP pools");
assert_eq!(silo_pools.len(), 0);
Expand All @@ -893,7 +923,7 @@ mod test {
.expect("Should list IP pools");
assert_eq!(all_pools.len(), 1);
let silo_pools = datastore
.silo_ip_pools_list(&opctx, &pagbyid)
.current_silo_ip_pool_list(&opctx, &pagbyid)
.await
.expect("Should list silo IP pools");
assert_eq!(silo_pools.len(), 0);
Expand Down Expand Up @@ -929,11 +959,12 @@ mod test {

// now it shows up in the silo list
let silo_pools = datastore
.silo_ip_pools_list(&opctx, &pagbyid)
.current_silo_ip_pool_list(&opctx, &pagbyid)
.await
.expect("Should list silo IP pools");
assert_eq!(silo_pools.len(), 1);
assert_eq!(silo_pools[0].id(), pool1_for_silo.id());
assert_eq!(silo_pools[0].0.id(), pool1_for_silo.id());
assert_eq!(silo_pools[0].1.is_default, false);

// linking an already linked silo errors due to PK conflict
let err = datastore
Expand Down Expand Up @@ -998,7 +1029,7 @@ mod test {

// and silo pools list is empty again
let silo_pools = datastore
.silo_ip_pools_list(&opctx, &pagbyid)
.current_silo_ip_pool_list(&opctx, &pagbyid)
.await
.expect("Should list silo IP pools");
assert_eq!(silo_pools.len(), 0);
Expand Down
30 changes: 21 additions & 9 deletions nexus/src/app/ip_pool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,12 @@ impl super::Nexus {
}

/// List IP pools in current silo
pub(crate) async fn silo_ip_pools_list(
pub(crate) async fn current_silo_ip_pool_list(
&self,
opctx: &OpContext,
pagparams: &PaginatedBy<'_>,
) -> ListResultVec<db::model::IpPool> {
self.db_datastore.silo_ip_pools_list(opctx, pagparams).await
) -> ListResultVec<(db::model::IpPool, db::model::IpPoolResource)> {
self.db_datastore.current_silo_ip_pool_list(opctx, pagparams).await
}

// Look up pool by name or ID, but only return it if it's linked to the
Expand All @@ -88,19 +88,19 @@ impl super::Nexus {
&'a self,
opctx: &'a OpContext,
pool: &'a NameOrId,
) -> LookupResult<db::model::IpPool> {
) -> LookupResult<(db::model::IpPool, db::model::IpPoolResource)> {
let (authz_pool, pool) =
self.ip_pool_lookup(opctx, pool)?.fetch().await?;

// 404 if no link is found in the current silo
let link = self.db_datastore.ip_pool_fetch_link(opctx, pool.id()).await;
if link.is_err() {
return Err(authz_pool.not_found());
match link {
Ok(link) => Ok((pool, link)),
Err(_) => Err(authz_pool.not_found()),
}

Ok(pool)
}

/// List silos for a given pool
pub(crate) async fn ip_pool_silo_list(
&self,
opctx: &OpContext,
Expand All @@ -112,11 +112,23 @@ impl super::Nexus {
self.db_datastore.ip_pool_silo_list(opctx, &authz_pool, pagparams).await
}

// List pools for a given silo
pub(crate) async fn silo_ip_pool_list(
&self,
opctx: &OpContext,
silo_lookup: &lookup::Silo<'_>,
pagparams: &PaginatedBy<'_>,
) -> ListResultVec<(db::model::IpPool, db::model::IpPoolResource)> {
let (.., authz_silo) =
silo_lookup.lookup_for(authz::Action::Read).await?;
self.db_datastore.silo_ip_pool_list(opctx, &authz_silo, pagparams).await
}

pub(crate) async fn ip_pool_link_silo(
&self,
opctx: &OpContext,
pool_lookup: &lookup::IpPool<'_>,
silo_link: &params::IpPoolSiloLink,
silo_link: &params::IpPoolLinkSilo,
) -> CreateResult<db::model::IpPoolResource> {
let (authz_pool,) =
pool_lookup.lookup_for(authz::Action::Modify).await?;
Expand Down
81 changes: 69 additions & 12 deletions nexus/src/external_api/http_entrypoints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ pub(crate) fn external_api() -> NexusApiDescription {
api.register(silo_delete)?;
api.register(silo_policy_view)?;
api.register(silo_policy_update)?;
api.register(silo_ip_pool_list)?;

api.register(silo_utilization_view)?;
api.register(silo_utilization_list)?;
Expand Down Expand Up @@ -741,7 +742,7 @@ async fn silo_create(

/// Fetch a silo
///
/// Fetch a silo by name.
/// Fetch a silo by name or ID.
#[endpoint {
method = GET,
path = "/v1/system/silos/{silo}",
Expand All @@ -763,6 +764,48 @@ async fn silo_view(
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
}

/// List IP pools available within silo
#[endpoint {
method = GET,
path = "/v1/system/silos/{silo}/ip-pools",
tags = ["system/silos"],
}]
async fn silo_ip_pool_list(
rqctx: RequestContext<Arc<ServerContext>>,
path_params: Path<params::SiloPath>,
query_params: Query<PaginatedByNameOrId>,
) -> Result<HttpResponseOk<ResultsPage<views::SiloIpPool>>, HttpError> {
let apictx = rqctx.context();
let handler = async {
let opctx = crate::context::op_context_for_external_api(&rqctx).await?;
let nexus = &apictx.nexus;
let path = path_params.into_inner();

let query = query_params.into_inner();
let pag_params = data_page_params_for(&rqctx, &query)?;
let scan_params = ScanByNameOrId::from_query(&query)?;
let paginated_by = name_or_id_pagination(&pag_params, scan_params)?;

let silo_lookup = nexus.silo_lookup(&opctx, path.silo)?;
let pools = nexus
.silo_ip_pool_list(&opctx, &silo_lookup, &paginated_by)
.await?
.iter()
.map(|(pool, silo_link)| views::SiloIpPool {
identity: pool.identity(),
is_default: silo_link.is_default,
})
.collect();

Ok(HttpResponseOk(ScanByNameOrId::results_page(
&query,
pools,
&marker_for_name_or_id,
)?))
};
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
}

/// Delete a silo
///
/// Delete a silo by name.
Expand Down Expand Up @@ -1302,7 +1345,7 @@ async fn project_policy_update(
async fn project_ip_pool_list(
rqctx: RequestContext<Arc<ServerContext>>,
query_params: Query<PaginatedByNameOrId>,
) -> Result<HttpResponseOk<ResultsPage<IpPool>>, HttpError> {
) -> Result<HttpResponseOk<ResultsPage<views::SiloIpPool>>, HttpError> {
let apictx = rqctx.context();
let handler = async {
let nexus = &apictx.nexus;
Expand All @@ -1312,10 +1355,13 @@ async fn project_ip_pool_list(
let paginated_by = name_or_id_pagination(&pag_params, scan_params)?;
let opctx = crate::context::op_context_for_external_api(&rqctx).await?;
let pools = nexus
.silo_ip_pools_list(&opctx, &paginated_by)
.current_silo_ip_pool_list(&opctx, &paginated_by)
.await?
.into_iter()
.map(IpPool::from)
.map(|(pool, silo_link)| views::SiloIpPool {
identity: pool.identity(),
is_default: silo_link.is_default,
})
.collect();
Ok(HttpResponseOk(ScanByNameOrId::results_page(
&query,
Expand All @@ -1335,14 +1381,18 @@ async fn project_ip_pool_list(
async fn project_ip_pool_view(
rqctx: RequestContext<Arc<ServerContext>>,
path_params: Path<params::IpPoolPath>,
) -> Result<HttpResponseOk<views::IpPool>, HttpError> {
) -> Result<HttpResponseOk<views::SiloIpPool>, HttpError> {
let apictx = rqctx.context();
let handler = async {
let opctx = crate::context::op_context_for_external_api(&rqctx).await?;
let nexus = &apictx.nexus;
let pool_selector = path_params.into_inner().pool;
let pool = nexus.silo_ip_pool_fetch(&opctx, &pool_selector).await?;
Ok(HttpResponseOk(IpPool::from(pool)))
let (pool, silo_link) =
nexus.silo_ip_pool_fetch(&opctx, &pool_selector).await?;
Ok(HttpResponseOk(views::SiloIpPool {
identity: pool.identity(),
is_default: silo_link.is_default,
}))
};
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
}
Expand Down Expand Up @@ -1489,7 +1539,14 @@ async fn ip_pool_silo_list(
// option would be to paginate by a composite key representing the (pool,
// resource_type, resource)
query_params: Query<PaginatedById>,
) -> Result<HttpResponseOk<ResultsPage<views::IpPoolSilo>>, HttpError> {
// TODO: this could just list views::Silo -- it's not like knowing silo_id
// and nothing else is particularly useful -- except we also want to say
// whether the pool is marked default on each silo. So one option would
// be to do the same as we did with SiloIpPool -- include is_default on
// whatever the thing is. Still... all we'd have to do to make this usable
// in both places would be to make it { ...IpPool, silo_id, silo_name,
// is_default }
) -> Result<HttpResponseOk<ResultsPage<views::IpPoolSiloLink>>, HttpError> {
Comment on lines +1542 to +1549
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if views::Silo shouldn't just be updated with default_ip_pool: Option<Uuid> and the silo just be returned here. There's only ever one default pool, it seems like a good thing to latch onto the silo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would mean every query for silos would have to join to the ip_pool_resource table and return both the silo and the link so we can tack on the pool ID.

Copy link
Contributor

Choose a reason for hiding this comment

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

Despite the increased cost I still think it'd be a UX improvement for the API. That said, it doesn't need to be done here. I'm not a huge fan of returning the link, but it's fine for now. It gives them all the info they need to get whatever else they need.

let apictx = rqctx.context();
let handler = async {
let opctx = crate::context::op_context_for_external_api(&rqctx).await?;
Expand All @@ -1511,7 +1568,7 @@ async fn ip_pool_silo_list(
Ok(HttpResponseOk(ScanById::results_page(
&query,
assocs,
&|_, x: &views::IpPoolSilo| x.silo_id,
&|_, x: &views::IpPoolSiloLink| x.silo_id,
)?))
};
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
Expand All @@ -1526,8 +1583,8 @@ async fn ip_pool_silo_list(
async fn ip_pool_silo_link(
rqctx: RequestContext<Arc<ServerContext>>,
path_params: Path<params::IpPoolPath>,
resource_assoc: TypedBody<params::IpPoolSiloLink>,
) -> Result<HttpResponseCreated<views::IpPoolSilo>, HttpError> {
resource_assoc: TypedBody<params::IpPoolLinkSilo>,
) -> Result<HttpResponseCreated<views::IpPoolSiloLink>, HttpError> {
let apictx = rqctx.context();
let handler = async {
let opctx = crate::context::op_context_for_external_api(&rqctx).await?;
Expand Down Expand Up @@ -1581,7 +1638,7 @@ async fn ip_pool_silo_update(
rqctx: RequestContext<Arc<ServerContext>>,
path_params: Path<params::IpPoolSiloPath>,
update: TypedBody<params::IpPoolSiloUpdate>,
) -> Result<HttpResponseOk<views::IpPoolSilo>, HttpError> {
) -> Result<HttpResponseOk<views::IpPoolSiloLink>, HttpError> {
let apictx = rqctx.context();
let handler = async {
let opctx = crate::context::op_context_for_external_api(&rqctx).await?;
Expand Down
4 changes: 2 additions & 2 deletions nexus/test-utils/src/resource_helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,9 +246,9 @@ pub async fn link_ip_pool(
is_default: bool,
) {
let link =
params::IpPoolSiloLink { silo: NameOrId::Id(*silo_id), is_default };
params::IpPoolLinkSilo { silo: NameOrId::Id(*silo_id), is_default };
let url = format!("/v1/system/ip-pools/{pool_name}/silos");
object_create::<params::IpPoolSiloLink, views::IpPoolSilo>(
object_create::<params::IpPoolLinkSilo, views::IpPoolSiloLink>(
client, &url, &link,
)
.await;
Expand Down
Loading
Loading