Skip to content

Commit

Permalink
Improve error handling on S3 HEAD requests. (#5380)
Browse files Browse the repository at this point in the history
[SC-59519](https://app.shortcut.com/tiledb-inc/story/59519/s3-is-object-masks-failures)

[SC-59575](https://app.shortcut.com/tiledb-inc/story/59575/improve-error-message-when-headobject-fails-with-status-301)

* In `S3::is_object` we return that the object does not exist if the
`HeadObject` API call fails for any reason (like unauthorized or wrong
region). This PR changes to return `false` only if `HeadObject` fails
with a 404 status, and return the failure to the user in other cases.
  * Azure and GCS already do that.
* In S3 operations that perform `HeadObject` (`is_object` and
`object_size`), if the operation returns a 301 (Permanent Redirect)
status, we add an explanatory note to the error message suggesting that
the configured region might be incorrect. This will be helpful because
while most operations return a detailed error message, `HeadObject`
cannot have a response body.

---
TYPE: BUG
DESC: Fixed `tiledb_vfs_is_file` masking failures on S3 by returning
false.

---
TYPE: IMPROVEMENT
DESC: Added additional context in the error messages of operations that
likely failed due to region mismatch.
  • Loading branch information
teo-tsirpanis authored Nov 21, 2024
1 parent e651f9a commit a34830c
Showing 1 changed file with 40 additions and 4 deletions.
44 changes: 40 additions & 4 deletions tiledb/sm/filesystem/s3.cc
Original file line number Diff line number Diff line change
Expand Up @@ -910,8 +910,31 @@ Status S3::is_object(
if (request_payer_ != Aws::S3::Model::RequestPayer::NOT_SET)
head_object_request.SetRequestPayer(request_payer_);
auto head_object_outcome = client_->HeadObject(head_object_request);
*exists = head_object_outcome.IsSuccess();

if (!head_object_outcome.IsSuccess()) {
if (head_object_outcome.GetError().GetResponseCode() ==
Aws::Http::HttpResponseCode::NOT_FOUND) {
*exists = false;
return Status::Ok();
}

// Because this is a HEAD request, its response will not contain a detailed
// message. Try to provide more information to the user depending on the
// status code.
std::string additional_context;
if (head_object_outcome.GetError().GetResponseCode() ==
Aws::Http::HttpResponseCode::MOVED_PERMANENTLY) {
additional_context =
" The bucket might be located in another region; you can set it with "
"the 'vfs.s3.region' option.";
}
return LOG_STATUS(Status_S3Error(
"Failed to check for existence of object s3://" + bucket_name + "/" +
object_key + outcome_error_message(head_object_outcome) +
additional_context));
}

*exists = true;
return Status::Ok();
}

Expand Down Expand Up @@ -1029,10 +1052,23 @@ Status S3::object_size(const URI& uri, uint64_t* nbytes) const {
head_object_request.SetRequestPayer(request_payer_);
auto head_object_outcome = client_->HeadObject(head_object_request);

if (!head_object_outcome.IsSuccess())
if (!head_object_outcome.IsSuccess()) {
// Because this is a HEAD request, its response will not contain a detailed
// message. Try to provide more information to the user depending on the
// status code.
std::string additional_context;
if (head_object_outcome.GetError().GetResponseCode() ==
Aws::Http::HttpResponseCode::MOVED_PERMANENTLY) {
additional_context =
" The bucket might be located in another region; you can set it with "
"the 'vfs.s3.region' option.";
}
return LOG_STATUS(Status_S3Error(
"Cannot retrieve S3 object size; Error while listing file " +
uri.to_string() + outcome_error_message(head_object_outcome)));
std::string(
"Cannot retrieve S3 object size; Error while listing file s3://") +
uri.to_string() + outcome_error_message(head_object_outcome) +
additional_context));
}
*nbytes =
static_cast<uint64_t>(head_object_outcome.GetResult().GetContentLength());

Expand Down

0 comments on commit a34830c

Please sign in to comment.