From 91c0a628f0aadd99ae5edb10ba69d2c2bcf29cd1 Mon Sep 17 00:00:00 2001 From: Casey Bodley Date: Sat, 17 Feb 2024 11:42:12 -0500 Subject: [PATCH] rgw/iam: use retry_raced_user_write() for User/AccessKey apis Signed-off-by: Casey Bodley --- src/rgw/rgw_rest_iam.h | 19 +++ src/rgw/rgw_rest_iam_group.cc | 38 ++++-- src/rgw/rgw_rest_iam_user.cc | 128 ++++++++++-------- src/rgw/rgw_rest_user_policy.cc | 233 ++++++++++++++++---------------- 4 files changed, 232 insertions(+), 186 deletions(-) diff --git a/src/rgw/rgw_rest_iam.h b/src/rgw/rgw_rest_iam.h index 0c2b262bd62b7..ef0e170c38924 100644 --- a/src/rgw/rgw_rest_iam.h +++ b/src/rgw/rgw_rest_iam.h @@ -8,6 +8,7 @@ #include "rgw_auth.h" #include "rgw_auth_filters.h" #include "rgw_rest.h" +#include "rgw_sal.h" #include "rgw_xml.h" @@ -33,6 +34,24 @@ int forward_iam_request_to_master(const DoutPrefixProvider* dpp, RGWXMLDecoder::XMLParser& parser, req_info& req, optional_yield y); +/// Perform an atomic read-modify-write operation on the given user metadata. +/// Racing writes are detected here as ECANCELED errors, where we reload the +/// updated user metadata and retry the operation. +template F> +int retry_raced_user_write(const DoutPrefixProvider* dpp, optional_yield y, + rgw::sal::User* u, const F& f) +{ + int r = f(); + for (int i = 0; i < 10 && r == -ECANCELED; ++i) { + u->get_version_tracker().clear(); + r = u->load_user(dpp, y); + if (r >= 0) { + r = f(); + } + } + return r; +} + /// Perform an atomic read-modify-write operation on the given group metadata. /// Racing writes are detected here as ECANCELED errors, where we reload the /// updated group metadata and retry the operation. diff --git a/src/rgw/rgw_rest_iam_group.cc b/src/rgw/rgw_rest_iam_group.cc index 307ba8937baf8..9819cbb53a00d 100644 --- a/src/rgw/rgw_rest_iam_group.cc +++ b/src/rgw/rgw_rest_iam_group.cc @@ -903,14 +903,18 @@ void RGWAddUserToGroup_IAM::execute(optional_yield y) } } - RGWUserInfo& info = user->get_info(); - RGWUserInfo old_info = info; + op_ret = retry_raced_user_write(this, y, user.get(), + [this, y] { + RGWUserInfo& info = user->get_info(); + RGWUserInfo old_info = info; - const bool inserted = info.group_ids.insert(group.id).second; - if (inserted) { - constexpr bool exclusive = false; - op_ret = user->store_user(this, y, exclusive, &old_info); - } + if (!info.group_ids.insert(group.id).second) { + return 0; // nothing to do, return success + } + + constexpr bool exclusive = false; + return user->store_user(this, y, exclusive, &old_info); + }); } void RGWAddUserToGroup_IAM::send_response() @@ -1036,16 +1040,20 @@ void RGWRemoveUserFromGroup_IAM::execute(optional_yield y) } } - RGWUserInfo& info = user->get_info(); - RGWUserInfo old_info = info; + op_ret = retry_raced_user_write(this, y, user.get(), + [this, y] { + RGWUserInfo& info = user->get_info(); + RGWUserInfo old_info = info; - if (auto id = info.group_ids.find(group.id); - id != info.group_ids.end()) { - info.group_ids.erase(id); + auto id = info.group_ids.find(group.id); + if (id == info.group_ids.end()) { + return 0; // nothing to do, return success + } + info.group_ids.erase(id); - constexpr bool exclusive = false; - op_ret = user->store_user(this, y, exclusive, &old_info); - } + constexpr bool exclusive = false; + return user->store_user(this, y, exclusive, &old_info); + }); } void RGWRemoveUserFromGroup_IAM::send_response() diff --git a/src/rgw/rgw_rest_iam_user.cc b/src/rgw/rgw_rest_iam_user.cc index 8dd6ada3077a3..90a50e3949e5a 100644 --- a/src/rgw/rgw_rest_iam_user.cc +++ b/src/rgw/rgw_rest_iam_user.cc @@ -426,18 +426,26 @@ void RGWUpdateUser_IAM::execute(optional_yield y) } } - RGWUserInfo& info = user->get_info(); - RGWUserInfo old_info = info; - - if (!new_path.empty()) { - info.path = new_path; - } - if (!new_username.empty()) { - info.display_name = new_username; - } - - constexpr bool exclusive = false; - op_ret = user->store_user(this, y, exclusive, &old_info); + op_ret = retry_raced_user_write(this, y, user.get(), + [this, y] { + RGWUserInfo& info = user->get_info(); + RGWUserInfo old_info = info; + + if (!new_path.empty()) { + info.path = new_path; + } + if (!new_username.empty()) { + info.display_name = new_username; + } + + if (info.path == old_info.path && + info.display_name == old_info.display_name) { + return 0; // no changes to write + } + + constexpr bool exclusive = false; + return user->store_user(this, y, exclusive, &old_info); + }); } void RGWUpdateUser_IAM::send_response() @@ -901,20 +909,17 @@ int RGWCreateAccessKey_IAM::forward_to_master(optional_yield y, void RGWCreateAccessKey_IAM::execute(optional_yield y) { - RGWUserInfo& info = user->get_info(); - RGWUserInfo old_info = info; - std::optional max_keys; { // read account's access key limit RGWAccountInfo account; rgw::sal::Attrs attrs; // unused RGWObjVersionTracker objv; // unused - op_ret = driver->load_account_by_id(this, y, info.account_id, + op_ret = driver->load_account_by_id(this, y, user->get_info().account_id, account, attrs, objv); if (op_ret < 0) { ldpp_dout(this, 4) << "failed to load iam account " - << info.account_id << ": " << cpp_strerror(op_ret) << dendl; + << user->get_info().account_id << ": " << cpp_strerror(op_ret) << dendl; return; } if (account.max_access_keys >= 0) { // max < 0 means unlimited @@ -939,17 +944,22 @@ void RGWCreateAccessKey_IAM::execute(optional_yield y) } } - info.access_keys[key.id] = key; + op_ret = retry_raced_user_write(this, y, user.get(), + [this, y, &max_keys] { + RGWUserInfo& info = user->get_info(); + RGWUserInfo old_info = info; - // check the current count against account limit - if (max_keys && std::cmp_greater(info.access_keys.size(), *max_keys)) { - s->err.message = fmt::format("Access key limit {} exceeded", *max_keys); - op_ret = -ERR_LIMIT_EXCEEDED; - return; - } + info.access_keys[key.id] = key; + + // check the current count against account limit + if (max_keys && std::cmp_greater(info.access_keys.size(), *max_keys)) { + s->err.message = fmt::format("Access key limit {} exceeded", *max_keys); + return -ERR_LIMIT_EXCEEDED; + } - constexpr bool exclusive = false; - op_ret = user->store_user(this, y, exclusive, &old_info); + constexpr bool exclusive = false; + return user->store_user(this, y, exclusive, &old_info); + }); } void RGWCreateAccessKey_IAM::send_response() @@ -1093,20 +1103,6 @@ int RGWUpdateAccessKey_IAM::forward_to_master(optional_yield y, void RGWUpdateAccessKey_IAM::execute(optional_yield y) { - RGWUserInfo& info = user->get_info(); - RGWUserInfo old_info = info; - - auto key = info.access_keys.find(access_key_id); - if (key == info.access_keys.end()) { - s->err.message = "No such AccessKeyId in the user"; - op_ret = -ERR_NO_SUCH_ENTITY; - return; - } - - if (key->second.active == new_status) { - return; // nothing to do, return success - } - const rgw::SiteConfig& site = *s->penv.site; if (!site.is_meta_master()) { op_ret = forward_to_master(y, site); @@ -1115,10 +1111,26 @@ void RGWUpdateAccessKey_IAM::execute(optional_yield y) } } - key->second.active = new_status; + op_ret = retry_raced_user_write(this, y, user.get(), + [this, y] { + RGWUserInfo& info = user->get_info(); + RGWUserInfo old_info = info; + + auto key = info.access_keys.find(access_key_id); + if (key == info.access_keys.end()) { + s->err.message = "No such AccessKeyId in the user"; + return -ERR_NO_SUCH_ENTITY; + } + + if (key->second.active == new_status) { + return 0; // nothing to do, return success + } - constexpr bool exclusive = false; - op_ret = user->store_user(this, y, exclusive, &old_info); + key->second.active = new_status; + + constexpr bool exclusive = false; + return user->store_user(this, y, exclusive, &old_info); + }); } void RGWUpdateAccessKey_IAM::send_response() @@ -1244,23 +1256,25 @@ void RGWDeleteAccessKey_IAM::execute(optional_yield y) } } - RGWUserInfo& info = user->get_info(); - RGWUserInfo old_info = info; + op_ret = retry_raced_user_write(this, y, user.get(), + [this, y, &site] { + RGWUserInfo& info = user->get_info(); + RGWUserInfo old_info = info; - auto key = info.access_keys.find(access_key_id); - if (key == info.access_keys.end()) { - if (!site.is_meta_master()) { - return; // delete succeeded on the master - } - s->err.message = "No such AccessKeyId in the user"; - op_ret = -ERR_NO_SUCH_ENTITY; - return; - } + auto key = info.access_keys.find(access_key_id); + if (key == info.access_keys.end()) { + if (!site.is_meta_master()) { + return 0; // delete succeeded on the master + } + s->err.message = "No such AccessKeyId in the user"; + return -ERR_NO_SUCH_ENTITY; + } - info.access_keys.erase(key); + info.access_keys.erase(key); - constexpr bool exclusive = false; - op_ret = user->store_user(this, y, exclusive, &old_info); + constexpr bool exclusive = false; + return user->store_user(this, y, exclusive, &old_info); + }); } void RGWDeleteAccessKey_IAM::send_response() diff --git a/src/rgw/rgw_rest_user_policy.cc b/src/rgw/rgw_rest_user_policy.cc index 2dc10711ea599..0da2552fc2f06 100644 --- a/src/rgw/rgw_rest_user_policy.cc +++ b/src/rgw/rgw_rest_user_policy.cc @@ -134,52 +134,58 @@ int RGWPutUserPolicy::get_params() void RGWPutUserPolicy::execute(optional_yield y) { - op_ret = rgw_forward_request_to_master(this, *s->penv.site, s->user->get_id(), - nullptr, nullptr, s->info, y); - if (op_ret < 0) { - ldpp_dout(this, 0) << "ERROR: forward_request_to_master returned ret=" << op_ret << dendl; - return; - } - + // validate the policy document try { const rgw::IAM::Policy p( s->cct, s->user->get_tenant(), policy, s->cct->_conf.get_val("rgw_policy_reject_invalid_principals")); - std::map policies; - if (auto it = user->get_attrs().find(RGW_ATTR_USER_POLICY); it != user->get_attrs().end()) { - decode(policies, it->second); - } - bufferlist in_bl; - policies[policy_name] = policy; - constexpr unsigned int USER_POLICIES_MAX_NUM = 100; - const unsigned int max_num = s->cct->_conf->rgw_user_policies_max_num < 0 ? - USER_POLICIES_MAX_NUM : s->cct->_conf->rgw_user_policies_max_num; - if (policies.size() > max_num) { - ldpp_dout(this, 4) << "IAM user policies has reached the num config: " - << max_num << ", cant add another" << dendl; - op_ret = -ERR_LIMIT_EXCEEDED; - s->err.message = - "The number of IAM user policies should not exceed allowed limit " - "of " + - std::to_string(max_num) + " policies."; - return; - } - encode(policies, in_bl); - user->get_attrs()[RGW_ATTR_USER_POLICY] = in_bl; - - op_ret = user->store_user(s, s->yield, false); - if (op_ret < 0) { - op_ret = -ERR_INTERNAL_ERROR; - } - } catch (buffer::error& err) { - ldpp_dout(this, 0) << "ERROR: failed to decode user policies" << dendl; - op_ret = -EIO; - } catch (rgw::IAM::PolicyParseException& e) { + } catch (const rgw::IAM::PolicyParseException& e) { ldpp_dout(this, 5) << "failed to parse policy: " << e.what() << dendl; s->err.message = e.what(); op_ret = -ERR_MALFORMED_DOC; + return; + } + + op_ret = rgw_forward_request_to_master(this, *s->penv.site, s->user->get_id(), + nullptr, nullptr, s->info, y); + if (op_ret < 0) { + ldpp_dout(this, 0) << "ERROR: forward_request_to_master returned ret=" << op_ret << dendl; + return; } + op_ret = retry_raced_user_write(this, y, user.get(), + [this, y] { + rgw::sal::Attrs& attrs = user->get_attrs(); + std::map policies; + if (auto it = attrs.find(RGW_ATTR_USER_POLICY); it != attrs.end()) try { + decode(policies, it->second); + } catch (const buffer::error& err) { + ldpp_dout(this, 0) << "ERROR: failed to decode user policies" << dendl; + return -EIO; + } + + policies[policy_name] = policy; + + constexpr unsigned int USER_POLICIES_MAX_NUM = 100; + const unsigned int max_num = s->cct->_conf->rgw_user_policies_max_num < 0 ? + USER_POLICIES_MAX_NUM : s->cct->_conf->rgw_user_policies_max_num; + if (policies.size() > max_num) { + ldpp_dout(this, 4) << "IAM user policies has reached the num config: " + << max_num << ", cant add another" << dendl; + s->err.message = + "The number of IAM user policies should not exceed allowed limit " + "of " + + std::to_string(max_num) + " policies."; + return -ERR_LIMIT_EXCEEDED; + } + + bufferlist bl; + encode(policies, bl); + attrs[RGW_ATTR_USER_POLICY] = std::move(bl); + + return user->store_user(s, y, false); + }); + if (op_ret == 0) { s->formatter->open_object_section_in_ns("PutUserPolicyResponse", RGW_REST_IAM_XMLNS); s->formatter->open_object_section("ResponseMetadata"); @@ -319,33 +325,32 @@ void RGWDeleteUserPolicy::execute(optional_yield y) ldpp_dout(this, 0) << "ERROR: forward_request_to_master returned ret=" << op_ret << dendl; } - std::map policies; - if (auto it = user->get_attrs().find(RGW_ATTR_USER_POLICY); it != user->get_attrs().end()) { - bufferlist out_bl = it->second; - try { - decode(policies, out_bl); - } catch (buffer::error& err) { - ldpp_dout(this, 0) << "ERROR: failed to decode user policies" << dendl; - op_ret = -EIO; - return; - } - } + op_ret = retry_raced_user_write(this, y, user.get(), + [this, y] { + rgw::sal::Attrs& attrs = user->get_attrs(); + std::map policies; + if (auto it = attrs.find(RGW_ATTR_USER_POLICY); it != attrs.end()) try { + decode(policies, it->second); + } catch (const buffer::error& err) { + ldpp_dout(this, 0) << "ERROR: failed to decode user policies" << dendl; + return -EIO; + } - auto policy = policies.find(policy_name); - if (policy == policies.end()) { - s->err.message = "No such PolicyName on the user"; - op_ret = -ERR_NO_SUCH_ENTITY; - return; - } + auto policy = policies.find(policy_name); + if (policy == policies.end()) { + s->err.message = "No such PolicyName on the user"; + return -ERR_NO_SUCH_ENTITY; + } + policies.erase(policy); + + bufferlist bl; + encode(policies, bl); + attrs[RGW_ATTR_USER_POLICY] = std::move(bl); - bufferlist in_bl; - policies.erase(policy); - encode(policies, in_bl); - user->get_attrs()[RGW_ATTR_USER_POLICY] = in_bl; + return user->store_user(s, y, false); + }); - op_ret = user->store_user(s, s->yield, false); if (op_ret < 0) { - op_ret = -ERR_INTERNAL_ERROR; return; } @@ -407,14 +412,7 @@ int RGWAttachUserPolicy_IAM::forward_to_master(optional_yield y, const rgw::Site void RGWAttachUserPolicy_IAM::execute(optional_yield y) { - const rgw::SiteConfig& site = *s->penv.site; - if (!site.is_meta_master()) { - op_ret = forward_to_master(y, site); - if (op_ret) { - return; - } - } - + // validate the policy arn try { const auto p = rgw::IAM::get_managed_policy(s->cct, policy_arn); if (!p) { @@ -422,31 +420,40 @@ void RGWAttachUserPolicy_IAM::execute(optional_yield y) s->err.message = "The requested PolicyArn is not recognized"; return; } - - rgw::IAM::ManagedPolicies policies; - auto& attrs = user->get_attrs(); - if (auto it = attrs.find(RGW_ATTR_MANAGED_POLICY); it != attrs.end()) { - decode(policies, it->second); - } - policies.arns.insert(policy_arn); - - bufferlist in_bl; - encode(policies, in_bl); - attrs[RGW_ATTR_MANAGED_POLICY] = in_bl; - - op_ret = user->store_user(this, s->yield, false); - if (op_ret < 0) { - op_ret = -ERR_INTERNAL_ERROR; - } - } catch (buffer::error& err) { - ldpp_dout(this, 0) << "ERROR: failed to decode user policies" << dendl; - op_ret = -EIO; - } catch (rgw::IAM::PolicyParseException& e) { + } catch (const rgw::IAM::PolicyParseException& e) { ldpp_dout(this, 5) << "failed to parse policy: " << e.what() << dendl; s->err.message = e.what(); op_ret = -ERR_MALFORMED_DOC; + return; } + const rgw::SiteConfig& site = *s->penv.site; + if (!site.is_meta_master()) { + op_ret = forward_to_master(y, site); + if (op_ret) { + return; + } + } + + op_ret = retry_raced_user_write(this, y, user.get(), + [this, y] { + rgw::sal::Attrs& attrs = user->get_attrs(); + rgw::IAM::ManagedPolicies policies; + if (auto it = attrs.find(RGW_ATTR_MANAGED_POLICY); it != attrs.end()) try { + decode(policies, it->second); + } catch (buffer::error& err) { + ldpp_dout(this, 0) << "ERROR: failed to decode user policies" << dendl; + return -EIO; + } + policies.arns.insert(policy_arn); + + bufferlist bl; + encode(policies, bl); + attrs[RGW_ATTR_MANAGED_POLICY] = std::move(bl); + + return user->store_user(this, y, false); + }); + if (op_ret == 0) { s->formatter->open_object_section_in_ns("AttachUserPolicyResponse", RGW_REST_IAM_XMLNS); s->formatter->open_object_section("ResponseMetadata"); @@ -533,32 +540,30 @@ void RGWDetachUserPolicy_IAM::execute(optional_yield y) } } - try { - rgw::IAM::ManagedPolicies policies; - auto& attrs = user->get_attrs(); - if (auto it = attrs.find(RGW_ATTR_MANAGED_POLICY); it != attrs.end()) { - decode(policies, it->second); - } - - auto i = policies.arns.find(policy_arn); - if (i == policies.arns.end()) { - op_ret = ERR_NO_SUCH_ENTITY; - return; - } - policies.arns.erase(i); - - bufferlist in_bl; - encode(policies, in_bl); - attrs[RGW_ATTR_MANAGED_POLICY] = in_bl; - - op_ret = user->store_user(this, s->yield, false); - if (op_ret < 0) { - op_ret = -ERR_INTERNAL_ERROR; - } - } catch (buffer::error& err) { - ldpp_dout(this, 0) << "ERROR: failed to decode user policies" << dendl; - op_ret = -EIO; - } + op_ret = retry_raced_user_write(this, y, user.get(), + [this, y] { + rgw::sal::Attrs& attrs = user->get_attrs(); + rgw::IAM::ManagedPolicies policies; + if (auto it = attrs.find(RGW_ATTR_MANAGED_POLICY); it != attrs.end()) try { + decode(policies, it->second); + } catch (const buffer::error& err) { + ldpp_dout(this, 0) << "ERROR: failed to decode user policies" << dendl; + return -EIO; + } + + auto i = policies.arns.find(policy_arn); + if (i == policies.arns.end()) { + s->err.message = "No such PolicyArn on the user"; + return ERR_NO_SUCH_ENTITY; + } + policies.arns.erase(i); + + bufferlist bl; + encode(policies, bl); + attrs[RGW_ATTR_MANAGED_POLICY] = std::move(bl); + + return user->store_user(this, y, false); + }); if (op_ret == 0) { s->formatter->open_object_section_in_ns("DetachUserPolicyResponse", RGW_REST_IAM_XMLNS); -- 2.39.5