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 all 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
86 changes: 40 additions & 46 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 @@ -79,47 +79,6 @@ impl DataStore {
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
}

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

// From the developer user's point of view, we treat IP pools linked to
// their silo as silo resources, so they can list them if they can list
// silo children
let authz_silo =
opctx.authn.silo_required().internal_context("listing IP pools")?;
opctx.authorize(authz::Action::ListChildren, &authz_silo).await?;

let silo_id = authz_silo.id();

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_type
.eq(IpPoolResourceType::Silo)
.and(ip_pool_resource::resource_id.eq(silo_id)),
)
.filter(ip_pool::time_deleted.is_null())
.select(db::model::IpPool::as_select())
.get_results_async(&*self.pool_connection_authorized(opctx).await?)
.await
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
}

/// Look up whether the given pool is available to users in the current
/// silo, i.e., whether there is an entry in the association table linking
/// the pool with that silo
Expand Down Expand Up @@ -400,6 +359,37 @@ 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(&*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 @@ -867,8 +857,11 @@ mod test {
.await
.expect("Should list IP pools");
assert_eq!(all_pools.len(), 0);

let authz_silo = opctx.authn.silo_required().unwrap();

let silo_pools = datastore
.silo_ip_pools_list(&opctx, &pagbyid)
.silo_ip_pool_list(&opctx, &authz_silo, &pagbyid)
.await
.expect("Should list silo IP pools");
assert_eq!(silo_pools.len(), 0);
Expand All @@ -893,7 +886,7 @@ mod test {
.expect("Should list IP pools");
assert_eq!(all_pools.len(), 1);
let silo_pools = datastore
.silo_ip_pools_list(&opctx, &pagbyid)
.silo_ip_pool_list(&opctx, &authz_silo, &pagbyid)
.await
.expect("Should list silo IP pools");
assert_eq!(silo_pools.len(), 0);
Expand Down Expand Up @@ -929,11 +922,12 @@ mod test {

// now it shows up in the silo list
let silo_pools = datastore
.silo_ip_pools_list(&opctx, &pagbyid)
.silo_ip_pool_list(&opctx, &authz_silo, &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 +992,7 @@ mod test {

// and silo pools list is empty again
let silo_pools = datastore
.silo_ip_pools_list(&opctx, &pagbyid)
.silo_ip_pool_list(&opctx, &authz_silo, &pagbyid)
.await
.expect("Should list silo IP pools");
assert_eq!(silo_pools.len(), 0);
Expand Down
47 changes: 38 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 @@ -20,6 +20,7 @@ use omicron_common::api::external::CreateResult;
use omicron_common::api::external::DataPageParams;
use omicron_common::api::external::DeleteResult;
use omicron_common::api::external::Error;
use omicron_common::api::external::InternalContext;
use omicron_common::api::external::ListResultVec;
use omicron_common::api::external::LookupResult;
use omicron_common::api::external::NameOrId;
Expand Down Expand Up @@ -74,12 +75,20 @@ 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)> {
let authz_silo =
opctx.authn.silo_required().internal_context("listing IP pools")?;

// From the developer user's point of view, we treat IP pools linked to
// their silo as silo resources, so they can list them if they can list
// silo children
opctx.authorize(authz::Action::ListChildren, &authz_silo).await?;

self.db_datastore.silo_ip_pool_list(opctx, &authz_silo, pagparams).await
}

// Look up pool by name or ID, but only return it if it's linked to the
Expand All @@ -88,19 +97,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 @@ -109,14 +118,34 @@ impl super::Nexus {
) -> ListResultVec<db::model::IpPoolResource> {
let (.., authz_pool) =
pool_lookup.lookup_for(authz::Action::ListChildren).await?;

// check ability to list silos in general
opctx.authorize(authz::Action::ListChildren, &authz::FLEET).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.

Noticed this check was missing here

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?;
// check ability to list pools in general
opctx
.authorize(authz::Action::ListChildren, &authz::IP_POOL_LIST)
.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
Loading
Loading