From 5c4512663852f07657dbbb10fe4eba73fb566226 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Tue, 22 Oct 2024 17:46:20 -0400 Subject: [PATCH] chore(cache): better forbidden error message (#9313) ### Description This PR makes the error message for the case that the provided token does not have [permission to upload an artifact](https://vercel.com/docs/rest-api/endpoints/artifacts#upload-a-cache-artifact-response-codes). Currently this case leads to us printing `WARNING failed to contact remote cache: unknown status forbidden: Not authorized` the PR will change this to `WARNING Insufficient permissions to write to remote cache. Please verify that your role has write access for Remote Cache Artifact at https://vercel.com/docs/accounts/team-members-and-roles/access-roles/team-level-roles` This does not replace the more detailed warnings when we [get a cache status](https://github.com/vercel/turborepo/blob/main/crates/turborepo-api-client/src/lib.rs#L266). These errors get printed as warnings via the [async cache](https://github.com/vercel/turborepo/blob/main/crates/turborepo-cache/src/async_cache.rs#L87) ### Testing Instructions Added unit tests for error messages in this case. --------- Co-authored-by: Thomas Knickman --- Cargo.lock | 3 ++ crates/turborepo-api-client/Cargo.toml | 1 + crates/turborepo-api-client/src/lib.rs | 7 +-- crates/turborepo-cache/Cargo.toml | 2 + crates/turborepo-cache/src/http.rs | 75 ++++++++++++++++++++------ crates/turborepo-cache/src/lib.rs | 2 + 6 files changed, 71 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 45424d4e23596..64c5cacc0eaff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6131,6 +6131,7 @@ dependencies = [ "chrono", "http 0.2.11", "httpmock", + "insta", "lazy_static", "port_scanner", "regex", @@ -6190,6 +6191,7 @@ dependencies = [ "camino", "futures", "hmac", + "insta", "libc", "os_str_bytes", "path-clean", @@ -6211,6 +6213,7 @@ dependencies = [ "turbopath", "turborepo-analytics", "turborepo-api-client", + "turborepo-vercel-api", "turborepo-vercel-api-mock", "zstd", ] diff --git a/crates/turborepo-api-client/Cargo.toml b/crates/turborepo-api-client/Cargo.toml index c0fff6459900b..2022a5bacc920 100644 --- a/crates/turborepo-api-client/Cargo.toml +++ b/crates/turborepo-api-client/Cargo.toml @@ -13,6 +13,7 @@ rustls-tls = ["reqwest/rustls-tls-native-roots"] anyhow = { workspace = true } http = "0.2.9" httpmock = { workspace = true } +insta = { workspace = true } port_scanner = { workspace = true } test-case = { workspace = true } turborepo-vercel-api-mock = { workspace = true } diff --git a/crates/turborepo-api-client/src/lib.rs b/crates/turborepo-api-client/src/lib.rs index f81e95c820a47..67b3d284e65c2 100644 --- a/crates/turborepo-api-client/src/lib.rs +++ b/crates/turborepo-api-client/src/lib.rs @@ -809,6 +809,7 @@ mod test { use anyhow::Result; use bytes::Bytes; + use insta::assert_snapshot; use turborepo_vercel_api_mock::start_test_server; use url::Url; @@ -886,9 +887,9 @@ mod test { .unwrap(), ); let err = APIClient::handle_403(response).await; - assert_eq!( + assert_snapshot!( err.to_string(), - "unable to parse 'this isn't valid JSON' as JSON: expected ident at line 1 column 2" + @"unable to parse 'this isn't valid JSON' as JSON: expected ident at line 1 column 2" ); } @@ -900,7 +901,7 @@ mod test { .unwrap(), ); let err = APIClient::handle_403(response).await; - assert_eq!(err.to_string(), "unknown status forbidden: Not authorized"); + assert_snapshot!(err.to_string(), @"unknown status forbidden: Not authorized"); } #[tokio::test] diff --git a/crates/turborepo-cache/Cargo.toml b/crates/turborepo-cache/Cargo.toml index 08ea0eed08933..5407fb706a531 100644 --- a/crates/turborepo-cache/Cargo.toml +++ b/crates/turborepo-cache/Cargo.toml @@ -13,10 +13,12 @@ rustls-tls = ["turborepo-api-client/rustls-tls"] [dev-dependencies] anyhow = { workspace = true, features = ["backtrace"] } futures = { workspace = true } +insta = { workspace = true } libc = "0.2.146" port_scanner = { workspace = true } tempfile = { workspace = true } test-case = { workspace = true } +turborepo-vercel-api = { workspace = true } turborepo-vercel-api-mock = { workspace = true } [lints] diff --git a/crates/turborepo-cache/src/http.rs b/crates/turborepo-cache/src/http.rs index c4936526241a9..d8f89973bf568 100644 --- a/crates/turborepo-cache/src/http.rs +++ b/crates/turborepo-cache/src/http.rs @@ -105,8 +105,7 @@ impl HTTPCache { tracing::debug!("uploading {}", hash); - match self - .client + self.client .put_artifact( hash, progress, @@ -118,19 +117,9 @@ impl HTTPCache { self.api_auth.team_slug.as_deref(), ) .await - { - Ok(_) => { - tracing::debug!("uploaded {}", hash); - Ok(()) - } - Err(turborepo_api_client::Error::ReqwestError(e)) if e.is_timeout() => { - Err(CacheError::TimeoutError(hash.to_string())) - } - Err(turborepo_api_client::Error::ReqwestError(e)) if e.is_connect() => { - Err(CacheError::ConnectError) - } - Err(e) => Err(e.into()), - } + .map_err(|err| Self::convert_api_error(hash, err))?; + tracing::debug!("uploaded {}", hash); + Ok(()) } #[tracing::instrument(skip_all)] @@ -278,14 +267,30 @@ impl HTTPCache { let mut cache_reader = CacheReader::from_reader(body, true)?; cache_reader.restore(root) } + + fn convert_api_error(hash: &str, err: turborepo_api_client::Error) -> CacheError { + match err { + turborepo_api_client::Error::ReqwestError(e) if e.is_timeout() => { + CacheError::TimeoutError(hash.to_string()) + } + turborepo_api_client::Error::ReqwestError(e) if e.is_connect() => { + CacheError::ConnectError + } + turborepo_api_client::Error::UnknownStatus { code, .. } if code == "forbidden" => { + CacheError::ForbiddenRemoteCacheWrite + } + e => e.into(), + } + } } #[cfg(test)] mod test { - use std::time::Duration; + use std::{backtrace::Backtrace, time::Duration}; use anyhow::Result; use futures::future::try_join_all; + use insta::assert_snapshot; use tempfile::tempdir; use turbopath::AbsoluteSystemPathBuf; use turborepo_analytics::start_analytics; @@ -381,4 +386,42 @@ mod test { Ok(()) } + + #[test] + fn test_forbidden_error() { + let err = HTTPCache::convert_api_error( + "hash", + turborepo_api_client::Error::UnknownStatus { + code: "forbidden".into(), + message: "Not authorized".into(), + backtrace: Backtrace::capture(), + }, + ); + assert_snapshot!(err.to_string(), @"Insufficient permissions to write to remote cache. Please verify that your role has write access for Remote Cache Artifact at https://vercel.com/docs/accounts/team-members-and-roles/access-roles/team-level-roles?resource=Remote+Cache+Artifact"); + } + + #[test] + fn test_unknown_status() { + let err = HTTPCache::convert_api_error( + "hash", + turborepo_api_client::Error::UnknownStatus { + code: "unknown".into(), + message: "Special message".into(), + backtrace: Backtrace::capture(), + }, + ); + assert_snapshot!(err.to_string(), @"failed to contact remote cache: unknown status unknown: Special message"); + } + + #[test] + fn test_cache_disabled() { + let err = HTTPCache::convert_api_error( + "hash", + turborepo_api_client::Error::CacheDisabled { + status: turborepo_vercel_api::CachingStatus::Disabled, + message: "Cache disabled".into(), + }, + ); + assert_snapshot!(err.to_string(), @"failed to contact remote cache: Cache disabled"); + } } diff --git a/crates/turborepo-cache/src/lib.rs b/crates/turborepo-cache/src/lib.rs index e839ff2608d32..c05f4dcce5edd 100644 --- a/crates/turborepo-cache/src/lib.rs +++ b/crates/turborepo-cache/src/lib.rs @@ -83,6 +83,8 @@ pub enum CacheError { ConfigCacheInvalidBase, #[error("Unable to hash config cache inputs")] ConfigCacheError, + #[error("Insufficient permissions to write to remote cache. Please verify that your role has write access for Remote Cache Artifact at https://vercel.com/docs/accounts/team-members-and-roles/access-roles/team-level-roles?resource=Remote+Cache+Artifact")] + ForbiddenRemoteCacheWrite, } impl From for CacheError {