Skip to content

Commit

Permalink
get all ip_pools integration tests passing
Browse files Browse the repository at this point in the history
  • Loading branch information
@david-crespo
david-crespo committed Oct 11, 2023
1 parent 7259531 commit 4c8c5ea
Show file tree
Hide file tree
Showing 5 changed files with 97 additions and 46 deletions.
30 changes: 20 additions & 10 deletions nexus/db-queries/src/db/datastore/ip_pool.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,26 +49,36 @@ impl DataStore {
opctx: &OpContext,
pagparams: &PaginatedBy<'_>,
) -> ListResultVec<IpPool> {
use db::schema::ip_pool::dsl;
use db::schema::ip_pool;
use db::schema::ip_pool_resource;

opctx
.authorize(authz::Action::ListChildren, &authz::IP_POOL_LIST)
.await?;
// TODO: boy I hope we can paginate a join without too much trouble
match pagparams {
PaginatedBy::Id(pagparams) => {
paginated(dsl::ip_pool, dsl::id, pagparams)
paginated(ip_pool::table, ip_pool::id, pagparams)
}
PaginatedBy::Name(pagparams) => paginated(
dsl::ip_pool,
dsl::name,
ip_pool::table,
ip_pool::name,
&pagparams.map_name(|n| Name::ref_cast(n)),
),
}
// != excludes nulls so we explicitly include them
// TODO: join to join table to exclude internal
// .filter(dsl::silo_id.ne(*INTERNAL_SILO_ID).or(dsl::silo_id.is_null()))
.filter(dsl::time_deleted.is_null())
.select(db::model::IpPool::as_select())
// TODO: make sure this join is compatible with pagination logic
.left_outer_join(ip_pool_resource::table)
// TODO: this filter is so unwieldy and confusing (see all the checks
// at the nexus layer too) that it makes me want to just put a boolean
// internal column on the ip_pool table
.filter(
ip_pool_resource::resource_id
.ne(*INTERNAL_SILO_ID)
// resource_id is not nullable -- null here means the
// pool has no entry in the join table
.or(ip_pool_resource::resource_id.is_null()),
)
.filter(ip_pool::time_deleted.is_null())
.select(IpPool::as_select())
.get_results_async(&*self.pool_connection_authorized(opctx).await?)
.await
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
Expand Down
29 changes: 19 additions & 10 deletions nexus/db-queries/src/db/datastore/project.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ use crate::db::pagination::paginated;
use async_bb8_diesel::{AsyncConnection, AsyncRunQueryDsl};
use chrono::Utc;
use diesel::prelude::*;
use nexus_db_model::IpPoolResourceType;
use omicron_common::api::external::http_pagination::PaginatedBy;
use omicron_common::api::external::CreateResult;
use omicron_common::api::external::DeleteResult;
Expand Down Expand Up @@ -337,24 +338,32 @@ impl DataStore {
authz_project: &authz::Project,
pagparams: &PaginatedBy<'_>,
) -> ListResultVec<db::model::IpPool> {
use db::schema::ip_pool::dsl;
use db::schema::ip_pool;
use db::schema::ip_pool_resource;

opctx.authorize(authz::Action::ListChildren, authz_project).await?;

let silo_id = opctx.authn.silo_required().unwrap().id();

match pagparams {
PaginatedBy::Id(pagparams) => {
paginated(dsl::ip_pool, dsl::id, pagparams)
paginated(ip_pool::table, ip_pool::id, pagparams)
}
PaginatedBy::Name(pagparams) => paginated(
dsl::ip_pool,
dsl::name,
ip_pool::table,
ip_pool::name,
&pagparams.map_name(|n| Name::ref_cast(n)),
),
}
// TODO(2148, 2056): filter only pools accessible by the given
// project, once specific projects for pools are implemented
// != excludes nulls so we explicitly include them
// TODO: filter out internal the right way
// .filter(dsl::silo_id.ne(*INTERNAL_SILO_ID).or(dsl::silo_id.is_null()))
.filter(dsl::time_deleted.is_null())
// TODO: make sure this join is compatible with pagination logic
.inner_join(ip_pool_resource::table)
.filter(
(ip_pool_resource::resource_type
.eq(IpPoolResourceType::Silo)
.and(ip_pool_resource::resource_id.eq(silo_id)))
.or(ip_pool_resource::resource_type.eq(IpPoolResourceType::Fleet)),
)
.filter(ip_pool::time_deleted.is_null())
.select(db::model::IpPool::as_select())
.get_results_async(&*self.pool_connection_authorized(opctx).await?)
.await
Expand Down
17 changes: 15 additions & 2 deletions nexus/src/app/ip_pool.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use nexus_db_model::IpPool;
use nexus_db_queries::authz;
use nexus_db_queries::context::OpContext;
use nexus_db_queries::db;
use nexus_db_queries::db::fixed_data::silo::INTERNAL_SILO_ID;
// use nexus_db_queries::db::fixed_data::silo::INTERNAL_SILO_ID;
use nexus_db_queries::db::lookup;
use nexus_db_queries::db::lookup::LookupPath;
use nexus_db_queries::db::model::Name;
Expand All @@ -27,7 +27,7 @@ use omicron_common::api::external::ResourceType;
use omicron_common::api::external::UpdateResult;
use ref_cast::RefCast;

fn is_internal(pool: &IpPool) -> bool {
fn is_internal(_pool: &IpPool) -> bool {
// pool.silo_id == Some(*INTERNAL_SILO_ID)
// TODO: this is no longer a simple function of the pool itself
false
Expand Down Expand Up @@ -71,6 +71,19 @@ impl super::Nexus {
// TODO: check for perms on specified resource
let (.., authz_pool) =
pool_lookup.lookup_for(authz::Action::Modify).await?;
match ip_pool_resource.resource_type {
params::IpPoolResourceType::Silo => {
self.silo_lookup(
&opctx,
NameOrId::Id(ip_pool_resource.resource_id),
)?
.lookup_for(authz::Action::Read)
.await?;
}
params::IpPoolResourceType::Fleet => {
// hope we don't need to be assured of the fleet's existence
}
};
self.db_datastore
.ip_pool_associate_resource(
opctx,
Expand Down
2 changes: 1 addition & 1 deletion nexus/test-utils/src/resource_helpers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ pub async fn create_ip_pool(
)
.await;

// match previous behavior, makes pool available for use anywhere in fleet
// make pool available for use anywhere in fleet
let _: () = object_create(
client,
&format!("/v1/system/ip-pools/{pool_name}/associate"),
Expand Down
65 changes: 42 additions & 23 deletions nexus/tests/integration_tests/ip_pools.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use dropshot::test_util::ClientTestContext;
use dropshot::HttpErrorResponseBody;
use http::method::Method;
use http::StatusCode;
use nexus_db_queries::db::fixed_data::FLEET_ID;
use nexus_test_utils::http_testing::AuthnMode;
use nexus_test_utils::http_testing::NexusRequest;
use nexus_test_utils::http_testing::RequestBuilder;
Expand All @@ -17,6 +18,7 @@ use nexus_test_utils::resource_helpers::{
create_instance, create_instance_with,
};
use nexus_test_utils_macros::nexus_test;
use nexus_types::external_api::params;
use nexus_types::external_api::params::ExternalIpCreate;
use nexus_types::external_api::params::InstanceDiskAttachment;
use nexus_types::external_api::params::InstanceNetworkInterfaceAttachment;
Expand All @@ -33,6 +35,7 @@ use omicron_common::api::external::{IdentityMetadataCreateParams, Name};
use omicron_nexus::TestInterfaces;
use sled_agent_client::TestInterfaces as SledTestInterfaces;
use std::collections::HashSet;
use uuid::Uuid;

type ControlPlaneTestContext =
nexus_test_utils::ControlPlaneTestContext<omicron_nexus::Server>;
Expand All @@ -59,8 +62,7 @@ async fn test_ip_pool_basic_crud(cptestctx: &ControlPlaneTestContext) {
.expect("Failed to list IP Pools")
.all_items;
assert_eq!(ip_pools.len(), 1, "Expected to see default IP pool");

assert_eq!(ip_pools[0].identity.name, "default",);
assert_eq!(ip_pools[0].identity.name, "default");
// assert_eq!(ip_pools[0].silo_id, None);
// assert!(ip_pools[0].is_default);

Expand Down Expand Up @@ -305,30 +307,33 @@ async fn test_ip_pool_with_silo(cptestctx: &ControlPlaneTestContext) {
let _created_pool = create_pool(client, &params).await;
// assert_eq!(created_pool.silo_id.unwrap(), silo_id);

// expect 404 if the specified silo doesn't exist
let bad_silo_params = IpPoolCreate {
identity: IdentityMetadataCreateParams {
name: String::from("p2").parse().unwrap(),
description: String::from(""),
},
// silo: Some(NameOrId::Name(
// String::from("not-a-thing").parse().unwrap(),
// )),
// is_default: false,
let nonexistent_silo_id = Uuid::new_v4();
// expect 404 on association if the specified silo doesn't exist
let params = params::IpPoolResource {
resource_id: nonexistent_silo_id,
resource_type: params::IpPoolResourceType::Silo,
is_default: false,
};
let error: HttpErrorResponseBody = NexusRequest::new(
RequestBuilder::new(client, Method::POST, "/v1/system/ip-pools")
.body(Some(&bad_silo_params))
.expect_status(Some(StatusCode::NOT_FOUND)),
let error = NexusRequest::new(
RequestBuilder::new(
client,
Method::POST,
"/v1/system/ip-pools/p1/associate",
)
.body(Some(&params))
.expect_status(Some(StatusCode::NOT_FOUND)),
)
.authn_as(AuthnMode::PrivilegedUser)
.execute()
.await
.unwrap()
.parsed_body()
.parsed_body::<HttpErrorResponseBody>()
.unwrap();

assert_eq!(error.message, "not found: silo with name \"not-a-thing\"");
assert_eq!(
error.message,
format!("not found: silo with id \"{nonexistent_silo_id}\"")
);
}

async fn create_pool(
Expand Down Expand Up @@ -691,14 +696,28 @@ async fn test_ip_pool_list_usable_by_project(
name: String::from(mypool_name).parse().unwrap(),
description: String::from("right on cue"),
},
// silo: None,
// is_default: false,
};
NexusRequest::objects_post(client, ip_pools_url, &params)
.authn_as(AuthnMode::PrivilegedUser)
.execute()
.await
.unwrap();
.execute_and_parse_unwrap::<IpPool>()
.await;

// add to fleet since we can't add to project yet
// TODO: could do silo, might as well? need the ID, though. at least
// until I make it so you can specify the resource by name
let params = params::IpPoolResource {
resource_id: *FLEET_ID,
resource_type: params::IpPoolResourceType::Fleet,
is_default: false,
};
let _ = NexusRequest::objects_post(
client,
&format!("/v1/system/ip-pools/{mypool_name}/associate"),
&params,
)
.authn_as(AuthnMode::PrivilegedUser)
.execute()
.await;

// Add an IP range to mypool
let mypool_range = IpRange::V4(
Expand Down

0 comments on commit 4c8c5ea

Please sign in to comment.