diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index 1ec724002f..a82bcc168d 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -49,26 +49,36 @@ impl DataStore { opctx: &OpContext, pagparams: &PaginatedBy<'_>, ) -> ListResultVec { - 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)) diff --git a/nexus/db-queries/src/db/datastore/project.rs b/nexus/db-queries/src/db/datastore/project.rs index 97abbbe102..f2b5ce1aed 100644 --- a/nexus/db-queries/src/db/datastore/project.rs +++ b/nexus/db-queries/src/db/datastore/project.rs @@ -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; @@ -337,24 +338,32 @@ impl DataStore { authz_project: &authz::Project, pagparams: &PaginatedBy<'_>, ) -> ListResultVec { - 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 diff --git a/nexus/src/app/ip_pool.rs b/nexus/src/app/ip_pool.rs index 22b99a3b07..d04c60b622 100644 --- a/nexus/src/app/ip_pool.rs +++ b/nexus/src/app/ip_pool.rs @@ -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; @@ -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 @@ -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, diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index ffbfca1d8e..d2bcf5fb86 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -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"), diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index 5084ab4f6f..3729437c78 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -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; @@ -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; @@ -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; @@ -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); @@ -305,30 +307,33 @@ async fn test_ip_pool_with_silo(cptestctx: &ControlPlaneTestContext) { let _created_pool = create_pool(client, ¶ms).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(¶ms)) + .expect_status(Some(StatusCode::NOT_FOUND)), ) .authn_as(AuthnMode::PrivilegedUser) .execute() .await .unwrap() - .parsed_body() + .parsed_body::() .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( @@ -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, ¶ms) .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap(); + .execute_and_parse_unwrap::() + .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"), + ¶ms, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await; // Add an IP range to mypool let mypool_range = IpRange::V4(