Skip to content

Commit

Permalink
chore(cache): better forbidden error message (#9313)
Browse files Browse the repository at this point in the history
### 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 <[email protected]>
  • Loading branch information
chris-olszewski and tknickman authored Oct 22, 2024
1 parent 54357a9 commit 5c45126
Show file tree
Hide file tree
Showing 6 changed files with 71 additions and 19 deletions.
3 changes: 3 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/turborepo-api-client/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
7 changes: 4 additions & 3 deletions crates/turborepo-api-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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"
);
}

Expand All @@ -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]
Expand Down
2 changes: 2 additions & 0 deletions crates/turborepo-cache/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
75 changes: 59 additions & 16 deletions crates/turborepo-cache/src/http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,7 @@ impl HTTPCache {

tracing::debug!("uploading {}", hash);

match self
.client
self.client
.put_artifact(
hash,
progress,
Expand All @@ -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)]
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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");
}
}
2 changes: 2 additions & 0 deletions crates/turborepo-cache/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<turborepo_api_client::Error> for CacheError {
Expand Down

0 comments on commit 5c45126

Please sign in to comment.