Skip to content

Commit

Permalink
s3_client: Check if response contains error
Browse files Browse the repository at this point in the history
S3 can reply with error even if it's replying using status 200. Most of
our parsing is done independent of status code (we're trying to check
error fields every time). But in case of delete objects reply we're not
trying to parse error fields if the status is 200. This commit fixes
this and before any attempt at parsing the response is done we're
checking if the 'Error.Code' field is present.

(cherry picked from commit df299db)
  • Loading branch information
Lazin authored and vbotbuildovich committed Oct 16, 2023
1 parent 061011e commit 5f08cd4
Showing 1 changed file with 24 additions and 3 deletions.
27 changes: 24 additions & 3 deletions src/v/cloud_storage_clients/s3_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,15 @@

#include <boost/beast/core/error.hpp>
#include <boost/beast/http/field.hpp>
#include <boost/beast/http/status.hpp>
#include <boost/property_tree/ptree.hpp>
#include <boost/property_tree/xml_parser.hpp>
#include <gnutls/crypto.h>

#include <bit>
#include <exception>
#include <utility>
#include <variant>

namespace cloud_storage_clients {

Expand Down Expand Up @@ -812,10 +814,22 @@ ss::future<> s3_client::do_delete_object(
});
}

static auto iobuf_to_delete_objects_result(iobuf&& buf) {
static std::variant<client::delete_objects_result, rest_error_response>
iobuf_to_delete_objects_result(iobuf&& buf) {
auto root = util::iobuf_to_ptree(std::move(buf), s3_log);
auto result = client::delete_objects_result{};
try {
if (root.count("Error.Code") > 0) {
// This is an error response. S3 can reply with 200 error code and
// error response in the body.
constexpr const char* empty = "";
auto code = root.get<ss::sstring>("Error.Code", empty);
auto msg = root.get<ss::sstring>("Error.Message", empty);
auto rid = root.get<ss::sstring>("Error.RequestId", empty);
auto res = root.get<ss::sstring>("Error.Resource", empty);
rest_error_response err(code, msg, rid, res);
return err;
}
for (auto const& [tag, value] : root.get_child("DeleteResult")) {
if (tag != "Error") {
continue;
Expand Down Expand Up @@ -880,8 +894,15 @@ auto s3_client::do_delete_objects(
return parse_rest_error_response<delete_objects_result>(
status, std::move(res));
}
return ss::make_ready_future<delete_objects_result>(
iobuf_to_delete_objects_result(std::move(res)));
auto parse_result = iobuf_to_delete_objects_result(
std::move(res));
if (std::holds_alternative<client::delete_objects_result>(
parse_result)) {
return ss::make_ready_future<delete_objects_result>(
std::get<client::delete_objects_result>(parse_result));
}
return ss::make_exception_future<delete_objects_result>(
std::get<rest_error_response>(parse_result));
});
});
}
Expand Down

0 comments on commit 5f08cd4

Please sign in to comment.