From 36583eb9bdf0692790b0b6046a00e5b060da6c93 Mon Sep 17 00:00:00 2001 From: Casey Bodley Date: Fri, 25 Apr 2025 21:02:29 -0400 Subject: [PATCH] rgw/rest: RGWRESTConn::forward() prefers to return http errors callers need to distinguish between transport errors (a failure to forward the request) and http errors (successfully forwarded and got a response). forward() was losing this information by mapping any http errors to errnos use tl::expected to differentiate between transport errors and http errors, with the latter being the successful/expected case Signed-off-by: Casey Bodley --- src/rgw/radosgw-admin/radosgw-admin.cc | 34 +++++++++---- src/rgw/rgw_op.cc | 8 ++- src/rgw/rgw_period_puller.cc | 7 ++- src/rgw/rgw_rest_client.cc | 19 ++++---- src/rgw/rgw_rest_client.h | 5 +- src/rgw/rgw_rest_conn.cc | 67 ++++++++++++++------------ src/rgw/rgw_rest_conn.h | 11 +++-- src/rgw/rgw_rest_iam.cc | 8 ++- 8 files changed, 100 insertions(+), 59 deletions(-) diff --git a/src/rgw/radosgw-admin/radosgw-admin.cc b/src/rgw/radosgw-admin/radosgw-admin.cc index 8540261776d1b..819f8bf3fe1d8 100644 --- a/src/rgw/radosgw-admin/radosgw-admin.cc +++ b/src/rgw/radosgw-admin/radosgw-admin.cc @@ -1902,14 +1902,21 @@ static int send_to_remote_gateway(RGWRESTConn* conn, req_info& info, ceph::bufferlist response; rgw_user user; - int ret = conn->forward(dpp(), user, info, MAX_REST_RESPONSE, &in_data, &response, null_yield); + auto result = conn->forward(dpp(), user, info, MAX_REST_RESPONSE, &in_data, &response, null_yield); + if (!result) { + return result.error(); + } + int ret = rgw_http_error_to_errno(*result); + if (ret < 0) { + return ret; + } - int parse_ret = parser.parse(response.c_str(), response.length()); - if (parse_ret < 0) { + ret = parser.parse(response.c_str(), response.length()); + if (ret < 0) { cerr << "failed to parse response" << std::endl; - return parse_ret; + return ret; } - return ret; + return 0; } static int send_to_url(const string& url, @@ -1930,14 +1937,21 @@ static int send_to_url(const string& url, RGWRESTSimpleRequest req(g_ceph_context, info.method, url, NULL, ¶ms, opt_region); bufferlist response; - int ret = req.forward_request(dpp(), key, info, MAX_REST_RESPONSE, &in_data, &response, null_yield); + auto result = req.forward_request(dpp(), key, info, MAX_REST_RESPONSE, &in_data, &response, null_yield); + if (!result) { + return result.error(); + } + int ret = rgw_http_error_to_errno(*result); + if (ret < 0) { + return ret; + } - int parse_ret = parser.parse(response.c_str(), response.length()); - if (parse_ret < 0) { + ret = parser.parse(response.c_str(), response.length()); + if (ret < 0) { cout << "failed to parse response" << std::endl; - return parse_ret; + return ret; } - return ret; + return 0; } static int send_to_remote_or_url(RGWRESTConn *conn, const string& url, diff --git a/src/rgw/rgw_op.cc b/src/rgw/rgw_op.cc index 44095eab1f74a..925a85e6691d8 100644 --- a/src/rgw/rgw_op.cc +++ b/src/rgw/rgw_op.cc @@ -155,8 +155,12 @@ int rgw_forward_request_to_master(const DoutPrefixProvider* dpp, creds, site.get_zonegroup().id, zg->second.api_name}; bufferlist outdata; constexpr size_t max_response_size = 128 * 1024; // we expect a very small response - int ret = conn.forward(dpp, effective_owner, req, - max_response_size, indata, &outdata, y); + auto result = conn.forward(dpp, effective_owner, req, + max_response_size, indata, &outdata, y); + if (!result) { + return result.error(); + } + int ret = rgw_http_error_to_errno(*result); if (ret < 0) { return ret; } diff --git a/src/rgw/rgw_period_puller.cc b/src/rgw/rgw_period_puller.cc index 5b6d218f69cd6..54137a0b0d384 100644 --- a/src/rgw/rgw_period_puller.cc +++ b/src/rgw/rgw_period_puller.cc @@ -4,6 +4,7 @@ #include "rgw_rados.h" #include "rgw_zone.h" #include "rgw_rest_conn.h" +#include "rgw_http_errors.h" #include "common/ceph_json.h" #include "common/errno.h" @@ -40,7 +41,11 @@ int pull_period(const DoutPrefixProvider *dpp, RGWRESTConn* conn, const std::str bufferlist data; #define MAX_REST_RESPONSE (128 * 1024) - int r = conn->forward(dpp, user, info, MAX_REST_RESPONSE, nullptr, &data, y); + auto result = conn->forward(dpp, user, info, MAX_REST_RESPONSE, nullptr, &data, y); + if (!result) { + return result.error(); + } + int r = rgw_http_error_to_errno(*result); if (r < 0) { return r; } diff --git a/src/rgw/rgw_rest_client.cc b/src/rgw/rgw_rest_client.cc index a5f8cb61a3286..a360d4a725167 100644 --- a/src/rgw/rgw_rest_client.cc +++ b/src/rgw/rgw_rest_client.cc @@ -364,7 +364,8 @@ static void scope_from_api_name(const DoutPrefixProvider *dpp, } } -int RGWRESTSimpleRequest::forward_request(const DoutPrefixProvider *dpp, const RGWAccessKey& key, const req_info& info, size_t max_response, bufferlist *inbl, bufferlist *outbl, optional_yield y, std::string service) +auto RGWRESTSimpleRequest::forward_request(const DoutPrefixProvider *dpp, const RGWAccessKey& key, const req_info& info, size_t max_response, bufferlist *inbl, bufferlist *outbl, optional_yield y, std::string service) + -> tl::expected { string date_str; @@ -410,7 +411,7 @@ int RGWRESTSimpleRequest::forward_request(const DoutPrefixProvider *dpp, const R int ret = sign_request(dpp, key, region, s, new_env, new_info, nullptr); if (ret < 0) { ldpp_dout(dpp, 0) << "ERROR: failed to sign request" << dendl; - return ret; + return tl::unexpected(ret); } if (s == "iam") { @@ -452,13 +453,11 @@ int RGWRESTSimpleRequest::forward_request(const DoutPrefixProvider *dpp, const R method = new_info.method; url = new_url; - int r = process(dpp, y); - if (r < 0) { - if (http_status == 0) { - // no http status, generally means the service is not available - r = -ERR_SERVICE_UNAVAILABLE; - } - return r; + std::ignore = process(dpp, y); + + if (http_status == 0) { + // no http status, generally means the service is not available + return tl::unexpected(-ERR_SERVICE_UNAVAILABLE); } response.append((char)0); /* NULL terminate response */ @@ -467,7 +466,7 @@ int RGWRESTSimpleRequest::forward_request(const DoutPrefixProvider *dpp, const R *outbl = std::move(response); } - return status; + return http_status; } class RGWRESTStreamOutCB : public RGWGetDataCB { diff --git a/src/rgw/rgw_rest_client.h b/src/rgw/rgw_rest_client.h index ffedcc17a94a3..2e4a7e2ab9f43 100644 --- a/src/rgw/rgw_rest_client.h +++ b/src/rgw/rgw_rest_client.h @@ -3,6 +3,7 @@ #pragma once +#include "include/expected.hpp" #include "rgw_http_client.h" class RGWGetDataCB; @@ -65,7 +66,9 @@ public: param_vec_t *_headers, param_vec_t *_params, std::optional _api_name) : RGWHTTPSimpleRequest(_cct, _method, _url, _headers, _params), api_name(_api_name) {} - int forward_request(const DoutPrefixProvider *dpp, const RGWAccessKey& key, const req_info& info, size_t max_response, bufferlist *inbl, bufferlist *outbl, optional_yield y, std::string service=""); + // return the http status of the response or an error code from the transport + auto forward_request(const DoutPrefixProvider *dpp, const RGWAccessKey& key, const req_info& info, size_t max_response, bufferlist *inbl, bufferlist *outbl, optional_yield y, std::string service="") + -> tl::expected; }; class RGWWriteDrainCB { diff --git a/src/rgw/rgw_rest_conn.cc b/src/rgw/rgw_rest_conn.cc index 46dd7323c449f..6ba846ff3d748 100644 --- a/src/rgw/rgw_rest_conn.cc +++ b/src/rgw/rgw_rest_conn.cc @@ -3,6 +3,7 @@ #include "rgw_zone.h" #include "rgw_rest_conn.h" +#include "rgw_http_errors.h" #include "rgw_sal.h" #include "rgw_rados.h" @@ -153,57 +154,63 @@ void RGWRESTConn::populate_params(param_vec_t& params, const rgw_owner* uid, con populate_zonegroup(params, zonegroup); } -int RGWRESTConn::forward(const DoutPrefixProvider *dpp, const rgw_owner& uid, const req_info& info, size_t max_response, bufferlist *inbl, bufferlist *outbl, optional_yield y) +auto RGWRESTConn::forward(const DoutPrefixProvider *dpp, const rgw_owner& uid, + const req_info& info, size_t max_response, + bufferlist *inbl, bufferlist *outbl, optional_yield y) + -> tl::expected { - int ret = 0; - static constexpr int NUM_ENPOINT_IOERROR_RETRIES = 20; for (int tries = 0; tries < NUM_ENPOINT_IOERROR_RETRIES; tries++) { string url; - ret = get_url(url); - if (ret < 0) - return ret; + int ret = get_url(url); + if (ret < 0) { + return tl::unexpected(ret); + } param_vec_t params; populate_params(params, &uid, self_zone_group); RGWRESTSimpleRequest req(cct, info.method, url, NULL, ¶ms, api_name); - ret = req.forward_request(dpp, key, info, max_response, inbl, outbl, y); - if (ret == -EIO) { - set_url_unconnectable(url); - if (tries < NUM_ENPOINT_IOERROR_RETRIES - 1) { - ldpp_dout(dpp, 20) << __func__ << "(): failed to forward request. retries=" << tries << dendl; - continue; - } + auto result = req.forward_request(dpp, key, info, max_response, inbl, outbl, y); + if (result) { + return result; + } else if (result.error() != -EIO) { + return result; + } + set_url_unconnectable(url); + if (tries < NUM_ENPOINT_IOERROR_RETRIES - 1) { + ldpp_dout(dpp, 20) << __func__ << "(): failed to forward request. retries=" << tries << dendl; } - break; } - return ret; + return tl::unexpected(-EIO); } -int RGWRESTConn::forward_iam_request(const DoutPrefixProvider *dpp, const req_info& info, size_t max_response, bufferlist *inbl, bufferlist *outbl, optional_yield y) +auto RGWRESTConn::forward_iam(const DoutPrefixProvider *dpp, const req_info& info, + size_t max_response, bufferlist *inbl, + bufferlist *outbl, optional_yield y) + -> tl::expected { - int ret = 0; - static constexpr int NUM_ENPOINT_IOERROR_RETRIES = 20; for (int tries = 0; tries < NUM_ENPOINT_IOERROR_RETRIES; tries++) { string url; - ret = get_url(url); - if (ret < 0) - return ret; + int ret = get_url(url); + if (ret < 0) { + return tl::unexpected(ret); + } param_vec_t params; std::string service = "iam"; RGWRESTSimpleRequest req(cct, info.method, url, NULL, ¶ms, api_name); // coverity[uninit_use_in_call:SUPPRESS] - ret = req.forward_request(dpp, key, info, max_response, inbl, outbl, y, service); - if (ret == -EIO) { - set_url_unconnectable(url); - if (tries < NUM_ENPOINT_IOERROR_RETRIES - 1) { - ldpp_dout(dpp, 20) << __func__ << "(): failed to forward request. retries=" << tries << dendl; - continue; - } + auto result = req.forward_request(dpp, key, info, max_response, inbl, outbl, y, service); + if (result) { + return result; + } else if (result.error() != -EIO) { + return result; + } + set_url_unconnectable(url); + if (tries < NUM_ENPOINT_IOERROR_RETRIES - 1) { + ldpp_dout(dpp, 20) << __func__ << "(): failed to forward request. retries=" << tries << dendl; } - break; } - return ret; + return tl::unexpected(-EIO); } int RGWRESTConn::put_obj_send_init(const rgw_obj& obj, const rgw_http_param_pair *extra_params, RGWRESTStreamS3PutObj **req) diff --git a/src/rgw/rgw_rest_conn.h b/src/rgw/rgw_rest_conn.h index 21bfdeb893002..e0461aa10afda 100644 --- a/src/rgw/rgw_rest_conn.h +++ b/src/rgw/rgw_rest_conn.h @@ -130,11 +130,16 @@ public: virtual void populate_params(param_vec_t& params, const rgw_owner* uid, const std::string& zonegroup); /* sync request */ - int forward(const DoutPrefixProvider *dpp, const rgw_owner& uid, const req_info& info, size_t max_response, bufferlist *inbl, bufferlist *outbl, optional_yield y); + auto forward(const DoutPrefixProvider *dpp, const rgw_owner& uid, + const req_info& info, size_t max_response, + bufferlist *inbl, bufferlist *outbl, optional_yield y) + -> tl::expected; /* sync request */ - int forward_iam_request(const DoutPrefixProvider *dpp, const req_info& info, size_t max_response, bufferlist *inbl, bufferlist *outbl, optional_yield y); - + auto forward_iam(const DoutPrefixProvider *dpp, const req_info& info, + size_t max_response, bufferlist *inbl, + bufferlist *outbl, optional_yield y) + -> tl::expected; /* async requests */ int put_obj_send_init(const rgw_obj& obj, const rgw_http_param_pair *extra_params, RGWRESTStreamS3PutObj **req); diff --git a/src/rgw/rgw_rest_iam.cc b/src/rgw/rgw_rest_iam.cc index 32ff7ac0a2282..5f9ae9c3a866d 100644 --- a/src/rgw/rgw_rest_iam.cc +++ b/src/rgw/rgw_rest_iam.cc @@ -304,8 +304,12 @@ int forward_iam_request_to_master(const DoutPrefixProvider* dpp, std::move(creds), zg->second.id, zg->second.api_name}; bufferlist outdata; constexpr size_t max_response_size = 128 * 1024; // we expect a very small response - int ret = conn.forward_iam_request(dpp, req, max_response_size, - &indata, &outdata, y); + auto result = conn.forward_iam(dpp, req, max_response_size, + &indata, &outdata, y); + if (!result) { + return result.error(); + } + int ret = rgw_http_error_to_errno(*result); if (ret < 0) { return ret; } -- 2.39.5