PUT link 404s instead of 500 when the link doesn't exist

oxidecomputer · Dec 14, 2023 · dc9de62 · dc9de62
1 parent 32fcbc4
commit dc9de62
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 16 deletions.
diff --git 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<IpPoolResource> {
         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
@@ -155,6 +155,28 @@ where
         .unwrap()
 }
 
+pub async fn object_put_error<InputType>(
+    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::<HttpErrorResponseBody>()
+    .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
@@ -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::<Silo>()
-        .await
-        .identity
-        .id;
+    let silo_id = object_get::<Silo>(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, &params, 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, &params).await;
 
     // making the same one default again is not an error