Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rgw: improve handling of Swift's error messages and limits #15369

Merged
merged 9 commits into from Sep 19, 2017
12 changes: 12 additions & 0 deletions src/common/options.cc
Expand Up @@ -4163,6 +4163,18 @@ std::vector<Option> get_rgw_options() {
.set_default(1 * 1024 * 1024)
.set_description(""),

Option("rgw_max_attr_size", Option::TYPE_UINT, Option::LEVEL_ADVANCED)
.set_default(0)
.set_description("The maximum length of metadata value. 0 skips the check"),

Option("rgw_max_attr_name_len", Option::TYPE_UINT, Option::LEVEL_ADVANCED)
.set_default(0)
.set_description("The maximum length of metadata name. 0 skips the check"),

Option("rgw_max_attrs_num_in_req", Option::TYPE_UINT, Option::LEVEL_ADVANCED)
.set_default(0)
.set_description("The maximum number of metadata items that can be put via single request"),

Option("rgw_override_bucket_index_max_shards", Option::TYPE_UINT, Option::LEVEL_ADVANCED)
.set_default(0)
.set_description(""),
Expand Down
15 changes: 14 additions & 1 deletion src/rgw/rgw_common.cc
Expand Up @@ -118,11 +118,14 @@ rgw_http_errors rgw_http_s3_errors({
rgw_http_errors rgw_http_swift_errors({
{ EACCES, {403, "AccessDenied" }},
{ EPERM, {401, "AccessDenied" }},
{ ENAMETOOLONG, {400, "Metadata name too long" }},
{ ERR_USER_SUSPENDED, {401, "UserSuspended" }},
{ ERR_INVALID_UTF8, {412, "Invalid UTF8" }},
{ ERR_BAD_URL, {412, "Bad URL" }},
{ ERR_NOT_SLO_MANIFEST, {400, "Not an SLO manifest" }},
{ ERR_QUOTA_EXCEEDED, {413, "QuotaExceeded" }},
{ ENOTEMPTY, {409, "There was a conflict when trying "
"to complete your request." }},
/* FIXME(rzarzynski): we need to find a way to apply Swift's error handling
* procedures also for ERR_ZERO_IN_URL. This make a problem as the validation
* is performed very early, even before setting the req_state::proto_flags. */
Expand Down Expand Up @@ -341,7 +344,17 @@ void set_req_state_err(struct req_state* s, int err_no, const string& err_msg)
{
if (s) {
set_req_state_err(s, err_no);
s->err.message = err_msg;
if (s->prot_flags & RGW_REST_SWIFT && !err_msg.empty()) {
/* TODO(rzarzynski): there never ever should be a check like this one.
* It's here only for the sake of the patch's backportability. Further
* commits will move the logic to a per-RGWHandler replacement of
* the end_header() function. Alternativaly, we might consider making
* that just for the dump(). Please take a look on @cbodley's comments
* in PR #10690 (https://github.com/ceph/ceph/pull/10690). */
s->err.err_code = err_msg;
} else {
s->err.message = err_msg;
}
}
}

Expand Down
5 changes: 4 additions & 1 deletion src/rgw/rgw_file.cc
Expand Up @@ -1452,7 +1452,10 @@ namespace rgw {
attrbl.append(val.c_str(), val.size() + 1);
}

rgw_get_request_metadata(s->cct, s->info, attrs);
op_ret = rgw_get_request_metadata(s->cct, s->info, attrs);
if (op_ret < 0) {
goto done;
}
encode_delete_at_attr(delete_at, attrs);

/* Add a custom metadata to expose the information whether an object
Expand Down
43 changes: 34 additions & 9 deletions src/rgw/rgw_op.cc
Expand Up @@ -1639,7 +1639,7 @@ void RGWGetObj::execute()
if (torrent.get_flag())
{
torrent.init(s, store);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you

torrent.get_torrent_file(op_ret, read_op, total_len, bl, obj);
op_ret = torrent.get_torrent_file(read_op, total_len, bl, obj);
if (op_ret < 0)
{
ldout(s->cct, 0) << "ERROR: failed to get_torrent_file ret= " << op_ret
Expand Down Expand Up @@ -2652,7 +2652,10 @@ void RGWCreateBucket::execute()
if (need_metadata_upload()) {
/* It's supposed that following functions WILL NOT change any special
* attributes (like RGW_ATTR_ACL) if they are already present in attrs. */
rgw_get_request_metadata(s->cct, s->info, attrs, false);
op_ret = rgw_get_request_metadata(s->cct, s->info, attrs, false);
if (op_ret < 0) {
return;
}
prepare_add_del_attrs(s->bucket_attrs, rmattr_names, attrs);
populate_with_generic_attrs(s, attrs);

Expand Down Expand Up @@ -2745,7 +2748,10 @@ void RGWCreateBucket::execute()

attrs.clear();

rgw_get_request_metadata(s->cct, s->info, attrs, false);
op_ret = rgw_get_request_metadata(s->cct, s->info, attrs, false);
if (op_ret < 0) {
return;
}
prepare_add_del_attrs(s->bucket_attrs, rmattr_names, attrs);
populate_with_generic_attrs(s, attrs);
op_ret = filter_out_quota_info(attrs, rmattr_names, s->bucket_info.quota);
Expand Down Expand Up @@ -3543,7 +3549,10 @@ void RGWPutObj::execute()
emplace_attr(RGW_ATTR_ETAG, std::move(bl));

populate_with_generic_attrs(s, attrs);
rgw_get_request_metadata(s->cct, s->info, attrs);
op_ret = rgw_get_request_metadata(s->cct, s->info, attrs);
if (op_ret < 0) {
goto done;
}
encode_delete_at_attr(delete_at, attrs);
encode_obj_tags_attr(obj_tags.get(), attrs);

Expand Down Expand Up @@ -3857,7 +3866,10 @@ int RGWPutMetadataAccount::init_processing()
attrs.emplace(RGW_ATTR_ACL, std::move(acl_bl));
}

rgw_get_request_metadata(s->cct, s->info, attrs, false);
op_ret = rgw_get_request_metadata(s->cct, s->info, attrs, false);
if (op_ret < 0) {
return op_ret;
}
prepare_add_del_attrs(orig_attrs, rmattr_names, attrs);
populate_with_generic_attrs(s, attrs);

Expand Down Expand Up @@ -3949,7 +3961,10 @@ void RGWPutMetadataBucket::execute()
return;
}

rgw_get_request_metadata(s->cct, s->info, attrs, false);
op_ret = rgw_get_request_metadata(s->cct, s->info, attrs, false);
if (op_ret < 0) {
return;
}

if (!placement_rule.empty() &&
placement_rule != s->bucket_info.placement_rule) {
Expand Down Expand Up @@ -4036,7 +4051,11 @@ void RGWPutMetadataObject::execute()
return;
}

rgw_get_request_metadata(s->cct, s->info, attrs);
op_ret = rgw_get_request_metadata(s->cct, s->info, attrs);
if (op_ret < 0) {
return;
}

/* check if obj exists, read orig attrs */
op_ret = get_obj_attrs(store, s, obj, orig_attrs);
if (op_ret < 0) {
Expand Down Expand Up @@ -4410,7 +4429,10 @@ int RGWCopyObj::init_common()
dest_policy.encode(aclbl);
emplace_attr(RGW_ATTR_ACL, std::move(aclbl));

rgw_get_request_metadata(s->cct, s->info, attrs);
op_ret = rgw_get_request_metadata(s->cct, s->info, attrs);
if (op_ret < 0) {
return op_ret;
}
populate_with_generic_attrs(s, attrs);

return 0;
Expand Down Expand Up @@ -5114,7 +5136,10 @@ void RGWInitMultipart::execute()
if (op_ret != 0)
return;

rgw_get_request_metadata(s->cct, s->info, attrs);
op_ret = rgw_get_request_metadata(s->cct, s->info, attrs);
if (op_ret < 0) {
return;
}

do {
char buf[33];
Expand Down
65 changes: 50 additions & 15 deletions src/rgw/rgw_op.h
Expand Up @@ -1884,38 +1884,73 @@ static inline void format_xattr(std::string &xattr)
* map(<attr_name, attr_contents>, where attr_name is RGW_ATTR_PREFIX.HTTP_NAME)
* s: The request state
* attrs: will be filled up with attrs mapped as <attr_name, attr_contents>
* On success returns 0.
* On failure returns a negative error code.
*
*/
static inline void rgw_get_request_metadata(CephContext *cct,
struct req_info& info,
map<string, bufferlist>& attrs,
const bool allow_empty_attrs = true)
static inline int rgw_get_request_metadata(CephContext* const cct,
struct req_info& info,
std::map<std::string, ceph::bufferlist>& attrs,
const bool allow_empty_attrs = true)
{
static const std::set<std::string> blacklisted_headers = {
"x-amz-server-side-encryption-customer-algorithm",
"x-amz-server-side-encryption-customer-key",
"x-amz-server-side-encryption-customer-key-md5"
};
map<string, string>::iterator iter;
for (iter = info.x_meta_map.begin(); iter != info.x_meta_map.end(); ++iter) {
const string &name(iter->first);
string &xattr(iter->second);

size_t valid_meta_count = 0;
for (auto& kv : info.x_meta_map) {
const std::string& name = kv.first;
std::string& xattr = kv.second;

if (blacklisted_headers.count(name) == 1) {
lsubdout(cct, rgw, 10) << "skipping x>> " << name << dendl;
continue;
}
if (allow_empty_attrs || !xattr.empty()) {
} else if (allow_empty_attrs || !xattr.empty()) {
lsubdout(cct, rgw, 10) << "x>> " << name << ":" << xattr << dendl;
format_xattr(xattr);
string attr_name(RGW_ATTR_PREFIX);

std::string attr_name(RGW_ATTR_PREFIX);
attr_name.append(name);
map<string, bufferlist>::value_type v(attr_name, bufferlist());
std::pair < map<string, bufferlist>::iterator, bool >
rval(attrs.insert(v));
bufferlist& bl(rval.first->second);

/* Check roughly whether we aren't going behind the limit on attribute
* name. Passing here doesn't guarantee that an OSD will accept that
* as ObjectStore::get_max_attr_name_length() can set the limit even
* lower than the "osd_max_attr_name_len" configurable. */
const size_t max_attr_name_len = \
cct->_conf->get_val<size_t>("rgw_max_attr_name_len");
if (max_attr_name_len && attr_name.length() > max_attr_name_len) {
return -ENAMETOOLONG;
}

/* Similar remarks apply to the check for value size. We're veryfing
* it early at the RGW's side as it's being claimed in /info. */
const size_t max_attr_size = \
cct->_conf->get_val<size_t>("rgw_max_attr_size");
if (max_attr_size && xattr.length() > max_attr_size) {
return -EFBIG;
}

/* Swift allows administrators to limit the number of metadats items
* send _in a single request_. */
const auto rgw_max_attrs_num_in_req = \
cct->_conf->get_val<size_t>("rgw_max_attrs_num_in_req");
if (rgw_max_attrs_num_in_req &&
++valid_meta_count > rgw_max_attrs_num_in_req) {
return -E2BIG;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's logic elsewhere in this commit that turns E2BIG into an error message. It would be much better to generate that error message here. That would result in simpler and easier to understand logic with less chance of breakage - less brittle!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid there is a reason for not doing that. IIRC the rgw_get_request_metadata is a generic routine that is used by both our Swift and S3 implementations while the error message is specific to Swift.

}

auto rval = attrs.emplace(std::move(attr_name), ceph::bufferlist());
/* At the moment the value of the freshly created attribute key-value
* pair is an empty bufferlist. */

ceph::bufferlist& bl = rval.first->second;
bl.append(xattr.c_str(), xattr.size() + 1);
}
}

return 0;
} /* rgw_get_request_metadata */

static inline void encode_delete_at_attr(boost::optional<ceph::real_time> delete_at,
Expand Down