diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index d766fa75d6..b98e171989 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -508,10 +508,8 @@ impl DataStore { ) -> UpdateResult { use db::schema::ip_pool_resource::dsl; - // TODO: correct auth check - opctx - .authorize(authz::Action::CreateChild, &authz::IP_POOL_LIST) - .await?; + opctx.authorize(authz::Action::Modify, authz_ip_pool).await?; + opctx.authorize(authz::Action::Modify, authz_silo).await?; let ip_pool_id = authz_ip_pool.id(); let silo_id = authz_silo.id(); @@ -580,7 +578,6 @@ impl DataStore { } } - // TODO: test that this errors if the link doesn't exist already let updated_link = diesel::update(dsl::ip_pool_resource) .filter(dsl::resource_id.eq(silo_id)) .filter(dsl::ip_pool_id.eq(ip_pool_id)) @@ -592,8 +589,20 @@ impl DataStore { Ok(updated_link) }) .await - .map_err(|e| { - Error::internal_error(&format!("Transaction error: {:?}", e)) + .map_err(|e| match e { + TransactionError::CustomError( + IpPoolResourceUpdateError::FailedToUnsetDefault(e), + ) => public_error_from_diesel(e, ErrorHandler::Server), + TransactionError::Database(e) => public_error_from_diesel( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::IpPoolResource, + // TODO: would be nice to put the actual names and/or ids in + // here but LookupType on each of the two silos doesn't have + // a nice to_string yet or a way of composing them + LookupType::ByCompositeId(format!("(pool, silo)")), + ), + ), }) } diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 278a02a336..c2516a1509 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -155,6 +155,28 @@ where .unwrap() } +pub async fn object_put_error( + client: &ClientTestContext, + path: &str, + input: &InputType, + status: StatusCode, +) -> HttpErrorResponseBody +where + InputType: serde::Serialize, +{ + NexusRequest::new( + RequestBuilder::new(client, Method::PUT, path) + .body(Some(&input)) + .expect_status(Some(status)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body::() + .unwrap() +} + pub async fn object_delete(client: &ClientTestContext, path: &str) { NexusRequest::object_delete(client, path) .authn_as(AuthnMode::PrivilegedUser) diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index 1313ea37b9..770796affc 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -25,6 +25,7 @@ use nexus_test_utils::resource_helpers::object_delete_error; use nexus_test_utils::resource_helpers::object_get; use nexus_test_utils::resource_helpers::object_get_error; use nexus_test_utils::resource_helpers::object_put; +use nexus_test_utils::resource_helpers::object_put_error; use nexus_test_utils::resource_helpers::objects_list_page_authz; use nexus_test_utils_macros::nexus_test; use nexus_types::external_api::params; @@ -306,12 +307,7 @@ async fn test_ip_pool_silo_link(cptestctx: &ControlPlaneTestContext) { // get silo ID so we can test association by ID as well let silo_url = format!("/v1/system/silos/{}", cptestctx.silo_name); - let silo_id = NexusRequest::object_get(client, &silo_url) - .authn_as(AuthnMode::PrivilegedUser) - .execute_and_parse_unwrap::() - .await - .identity - .id; + let silo_id = object_get::(client, &silo_url).await.identity.id; let assocs_p0 = silos_for_pool(client, "p0").await; let silo_link = @@ -383,7 +379,19 @@ async fn test_ip_pool_update_default(cptestctx: &ControlPlaneTestContext) { let silos_p1 = silos_for_pool(client, "p1").await; assert_eq!(silos_p1.items.len(), 0); - // associated both pools with the test silo + // put 404s if link doesn't exist yet + let params = IpPoolSiloUpdate { is_default: true }; + let p0_silo_url = + format!("/v1/system/ip-pools/p0/silos/{}", cptestctx.silo_name); + let error = + object_put_error(client, &p0_silo_url, ¶ms, StatusCode::NOT_FOUND) + .await; + assert_eq!( + error.message, + "not found: ip-pool-resource with id \"(pool, silo)\"" + ); + + // associate both pools with the test silo let silo = NameOrId::Name(cptestctx.silo_name.clone()); let params = params::IpPoolSiloLink { silo: silo.clone(), is_default: false }; @@ -403,8 +411,6 @@ async fn test_ip_pool_update_default(cptestctx: &ControlPlaneTestContext) { // make p0 default let params = IpPoolSiloUpdate { is_default: true }; - let p0_silo_url = - format!("/v1/system/ip-pools/p0/silos/{}", cptestctx.silo_name); let _: IpPoolSilo = object_put(client, &p0_silo_url, ¶ms).await; // making the same one default again is not an error