From 09f354fb7fa060235d88d1a7e3c49aa06a5b5a86 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 12 Jan 2024 13:20:04 -0600 Subject: [PATCH 1/4] fix list ip pools by deduping results of left outer join --- nexus/db-queries/src/db/datastore/ip_pool.rs | 1 + nexus/tests/integration_tests/ip_pools.rs | 89 +++++++++++++++----- 2 files changed, 71 insertions(+), 19 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index f51f54d592..581b0f1f8d 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -81,6 +81,7 @@ impl DataStore { ) .filter(ip_pool::time_deleted.is_null()) .select(IpPool::as_select()) + .distinct() .get_results_async(&*self.pool_connection_authorized(opctx).await?) .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index 5682df2c3a..22346713b4 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -17,6 +17,7 @@ use nexus_test_utils::http_testing::RequestBuilder; use nexus_test_utils::resource_helpers::create_instance; use nexus_test_utils::resource_helpers::create_ip_pool; use nexus_test_utils::resource_helpers::create_project; +use nexus_test_utils::resource_helpers::create_silo; use nexus_test_utils::resource_helpers::link_ip_pool; use nexus_test_utils::resource_helpers::object_create; use nexus_test_utils::resource_helpers::object_create_error; @@ -36,6 +37,7 @@ use nexus_types::external_api::params::IpPoolUpdate; use nexus_types::external_api::shared::IpRange; use nexus_types::external_api::shared::Ipv4Range; use nexus_types::external_api::shared::Ipv6Range; +use nexus_types::external_api::shared::SiloIdentityMode; use nexus_types::external_api::views::IpPool; use nexus_types::external_api::views::IpPoolRange; use nexus_types::external_api::views::IpPoolSilo; @@ -43,6 +45,7 @@ use nexus_types::external_api::views::Silo; use nexus_types::identity::Resource; use omicron_common::api::external::IdentityMetadataUpdateParams; use omicron_common::api::external::NameOrId; +use omicron_common::api::external::SimpleIdentity; use omicron_common::api::external::{IdentityMetadataCreateParams, Name}; use omicron_nexus::TestInterfaces; use sled_agent_client::TestInterfaces as SledTestInterfaces; @@ -62,16 +65,7 @@ async fn test_ip_pool_basic_crud(cptestctx: &ControlPlaneTestContext) { let ip_pool_ranges_url = format!("{}/ranges", ip_pool_url); let ip_pool_add_range_url = format!("{}/add", ip_pool_ranges_url); - // Verify the list of IP pools is empty - let ip_pools = NexusRequest::iter_collection_authn::( - client, - ip_pools_url, - "", - None, - ) - .await - .expect("Failed to list IP Pools") - .all_items; + let ip_pools = get_ip_pools(&client).await; assert_eq!(ip_pools.len(), 0, "Expected empty list of IP pools"); // Verify 404 if the pool doesn't exist yet, both for creating or deleting @@ -102,15 +96,7 @@ async fn test_ip_pool_basic_crud(cptestctx: &ControlPlaneTestContext) { assert_eq!(created_pool.identity.name, pool_name); assert_eq!(created_pool.identity.description, description); - let list = NexusRequest::iter_collection_authn::( - client, - ip_pools_url, - "", - None, - ) - .await - .expect("Failed to list IP Pools") - .all_items; + let list = get_ip_pools(client).await; assert_eq!(list.len(), 1, "Expected exactly 1 IP pool"); assert_pools_eq(&created_pool, &list[0]); @@ -212,6 +198,71 @@ async fn test_ip_pool_basic_crud(cptestctx: &ControlPlaneTestContext) { .expect("Expected to be able to delete an empty IP Pool"); } +async fn get_ip_pools(client: &ClientTestContext) -> Vec { + NexusRequest::iter_collection_authn::( + client, + "/v1/system/ip-pools", + "", + None, + ) + .await + .expect("Failed to list IP Pools") + .all_items +} + +// this test exists primarily because of a bug in the initial implementation +// where we included a copy of each pool in the list response for every +// associated silo instead of deduping the result of the left outer join +#[nexus_test] +async fn test_ip_pool_list_dedupe(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + + let ip_pools = get_ip_pools(&client).await; + assert_eq!(ip_pools.len(), 0); + + let range1 = IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 51), + std::net::Ipv4Addr::new(10, 0, 0, 52), + ) + .unwrap(), + ); + let (pool1, ..) = create_ip_pool(client, "pool1", Some(range1)).await; + let range2 = IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 53), + std::net::Ipv4Addr::new(10, 0, 0, 54), + ) + .unwrap(), + ); + let (pool2, ..) = create_ip_pool(client, "pool2", Some(range2)).await; + + let ip_pools = get_ip_pools(&client).await; + assert_eq!(ip_pools.len(), 2); + assert_eq!(ip_pools[0].identity.id, pool1.id()); + assert_eq!(ip_pools[1].identity.id, pool2.id()); + + // create 3 silos and link + let silo1 = + create_silo(&client, "silo1", true, SiloIdentityMode::SamlJit).await; + link_ip_pool(client, "pool1", &silo1.id(), false).await; + // linking pool2 here only, just for variety + link_ip_pool(client, "pool2", &silo1.id(), false).await; + + let silo2 = + create_silo(&client, "silo2", true, SiloIdentityMode::SamlJit).await; + link_ip_pool(client, "pool1", &silo2.id(), true).await; + + let silo3 = + create_silo(&client, "silo3", true, SiloIdentityMode::SamlJit).await; + link_ip_pool(client, "pool1", &silo3.id(), true).await; + + let ip_pools = get_ip_pools(&client).await; + assert_eq!(ip_pools.len(), 2); + assert_eq!(ip_pools[0].identity.id, pool1.id()); + assert_eq!(ip_pools[1].identity.id, pool2.id()); +} + /// The internal IP pool, defined by its association with the internal silo, /// cannot be interacted with through the operator API. CRUD operations should /// all 404 except fetch by name or ID. From 6c0518c5cf5c12d7ccf3fc18568ee62d930d12be Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 12 Jan 2024 16:25:16 -0600 Subject: [PATCH 2/4] use service IP pool name to filter out internal pool from normal endpoints --- nexus/db-queries/src/db/datastore/ip_pool.rs | 35 ++++++-------------- 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index 581b0f1f8d..0137f61cf1 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -10,10 +10,10 @@ use crate::context::OpContext; use crate::db; use crate::db::collection_insert::AsyncInsertError; use crate::db::collection_insert::DatastoreCollection; +use crate::db::datastore::SERVICE_IP_POOL_NAME; use crate::db::error::public_error_from_diesel; use crate::db::error::public_error_from_diesel_lookup; use crate::db::error::ErrorHandler; -use crate::db::fixed_data::silo::INTERNAL_SILO_ID; use crate::db::identity::Resource; use crate::db::model::ExternalIp; use crate::db::model::IpKind; @@ -56,7 +56,6 @@ impl DataStore { pagparams: &PaginatedBy<'_>, ) -> ListResultVec { use db::schema::ip_pool; - use db::schema::ip_pool_resource; opctx .authorize(authz::Action::ListChildren, &authz::IP_POOL_LIST) @@ -71,17 +70,9 @@ impl DataStore { &pagparams.map_name(|n| Name::ref_cast(n)), ), } - .left_outer_join(ip_pool_resource::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::name.ne(SERVICE_IP_POOL_NAME)) .filter(ip_pool::time_deleted.is_null()) .select(IpPool::as_select()) - .distinct() .get_results_async(&*self.pool_connection_authorized(opctx).await?) .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) @@ -234,23 +225,24 @@ impl DataStore { opctx: &OpContext, ) -> LookupResult<(authz::IpPool, IpPool)> { use db::schema::ip_pool; - use db::schema::ip_pool_resource; opctx .authorize(authz::Action::ListChildren, &authz::IP_POOL_LIST) .await?; + // TODO: just use LookupPath, come on + // let (.., authz_pool, pool) = db::lookup::LookupPath::new(&opctx, self) + // .ip_pool_name(*SERVICE_IP_POOL_NAME.parse().unwrap()) + // .lookup_for(authz::Action::Read) + // .await?; + // Ok((authz_pool, pool)) + // Look up IP pool by its association with the internal silo. // We assume there is only one pool for that silo, or at least, // if there is more than one, it doesn't matter which one we pick. let (authz_pool, pool) = ip_pool::table - .inner_join(ip_pool_resource::table) .filter(ip_pool::time_deleted.is_null()) - .filter( - ip_pool_resource::resource_type - .eq(IpPoolResourceType::Silo) - .and(ip_pool_resource::resource_id.eq(*INTERNAL_SILO_ID)), - ) + .filter(ip_pool::name.eq(SERVICE_IP_POOL_NAME)) .select(IpPool::as_select()) .get_result_async(&*self.pool_connection_authorized(opctx).await?) .await @@ -375,15 +367,10 @@ impl DataStore { authz_pool: &authz::IpPool, ) -> LookupResult { use db::schema::ip_pool; - use db::schema::ip_pool_resource; ip_pool::table - .inner_join(ip_pool_resource::table) .filter(ip_pool::id.eq(authz_pool.id())) - .filter( - ip_pool_resource::resource_type.eq(IpPoolResourceType::Silo), - ) - .filter(ip_pool_resource::resource_id.eq(*INTERNAL_SILO_ID)) + .filter(ip_pool::name.eq(SERVICE_IP_POOL_NAME)) .filter(ip_pool::time_deleted.is_null()) .select(ip_pool::id) .first_async::( From d9d87012c5dc9769ba544123a3173bf4a6ebdeeb Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 12 Jan 2024 16:42:42 -0600 Subject: [PATCH 3/4] use name lookup helper to look up thing by name! --- nexus/db-queries/src/db/datastore/ip_pool.rs | 41 ++------------------ nexus/tests/integration_tests/ip_pools.rs | 4 +- 2 files changed, 6 insertions(+), 39 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index 0137f61cf1..c9fdb5f0ee 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -15,6 +15,7 @@ use crate::db::error::public_error_from_diesel; use crate::db::error::public_error_from_diesel_lookup; use crate::db::error::ErrorHandler; use crate::db::identity::Resource; +use crate::db::lookup::LookupPath; use crate::db::model::ExternalIp; use crate::db::model::IpKind; use crate::db::model::IpPool; @@ -217,49 +218,15 @@ impl DataStore { }) } - /// Looks up an IP pool intended for internal services. + /// Look up IP pool intended for internal services by its well-known name. /// /// This method may require an index by Availability Zone in the future. pub async fn ip_pools_service_lookup( &self, opctx: &OpContext, ) -> LookupResult<(authz::IpPool, IpPool)> { - use db::schema::ip_pool; - - opctx - .authorize(authz::Action::ListChildren, &authz::IP_POOL_LIST) - .await?; - - // TODO: just use LookupPath, come on - // let (.., authz_pool, pool) = db::lookup::LookupPath::new(&opctx, self) - // .ip_pool_name(*SERVICE_IP_POOL_NAME.parse().unwrap()) - // .lookup_for(authz::Action::Read) - // .await?; - // Ok((authz_pool, pool)) - - // Look up IP pool by its association with the internal silo. - // We assume there is only one pool for that silo, or at least, - // if there is more than one, it doesn't matter which one we pick. - let (authz_pool, pool) = ip_pool::table - .filter(ip_pool::time_deleted.is_null()) - .filter(ip_pool::name.eq(SERVICE_IP_POOL_NAME)) - .select(IpPool::as_select()) - .get_result_async(&*self.pool_connection_authorized(opctx).await?) - .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) - .map(|ip_pool| { - ( - authz::IpPool::new( - authz::FLEET, - ip_pool.id(), - LookupType::ByCompositeId( - "Service IP Pool".to_string(), - ), - ), - ip_pool, - ) - })?; - Ok((authz_pool, pool)) + let name = SERVICE_IP_POOL_NAME.parse().unwrap(); + LookupPath::new(&opctx, self).ip_pool_name(&Name(name)).fetch().await } /// Creates a new IP pool. diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index 22346713b4..d97eda9a0b 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -211,8 +211,8 @@ async fn get_ip_pools(client: &ClientTestContext) -> Vec { } // this test exists primarily because of a bug in the initial implementation -// where we included a copy of each pool in the list response for every -// associated silo instead of deduping the result of the left outer join +// where we included a duplicate of each pool in the list response for every +// associated silo #[nexus_test] async fn test_ip_pool_list_dedupe(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; From 5896550880e2e1539dcf30f8f19b3d77b4c9629d Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 16 Jan 2024 11:42:59 -0600 Subject: [PATCH 4/4] fix auth test, which I think is more correct now --- nexus/tests/integration_tests/endpoints.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index b7b838ca50..11bfa34c5f 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -1048,7 +1048,7 @@ pub static VERIFY_ENDPOINTS: Lazy> = Lazy::new(|| { // IP Pool endpoint (Oxide services) VerifyEndpoint { url: &DEMO_IP_POOL_SERVICE_URL, - visibility: Visibility::Public, + visibility: Visibility::Protected, unprivileged_access: UnprivilegedAccess::None, allowed_methods: vec![ AllowedMethod::Get @@ -1058,7 +1058,7 @@ pub static VERIFY_ENDPOINTS: Lazy> = Lazy::new(|| { // IP Pool ranges endpoint (Oxide services) VerifyEndpoint { url: &DEMO_IP_POOL_SERVICE_RANGES_URL, - visibility: Visibility::Public, + visibility: Visibility::Protected, unprivileged_access: UnprivilegedAccess::None, allowed_methods: vec![ AllowedMethod::Get @@ -1068,7 +1068,7 @@ pub static VERIFY_ENDPOINTS: Lazy> = Lazy::new(|| { // IP Pool ranges/add endpoint (Oxide services) VerifyEndpoint { url: &DEMO_IP_POOL_SERVICE_RANGES_ADD_URL, - visibility: Visibility::Public, + visibility: Visibility::Protected, unprivileged_access: UnprivilegedAccess::None, allowed_methods: vec![ AllowedMethod::Post( @@ -1080,7 +1080,7 @@ pub static VERIFY_ENDPOINTS: Lazy> = Lazy::new(|| { // IP Pool ranges/delete endpoint (Oxide services) VerifyEndpoint { url: &DEMO_IP_POOL_SERVICE_RANGES_DEL_URL, - visibility: Visibility::Public, + visibility: Visibility::Protected, unprivileged_access: UnprivilegedAccess::None, allowed_methods: vec![ AllowedMethod::Post(