From 03c30e3e25ca8ec288a01ed4a12e2b5c4226ef67 Mon Sep 17 00:00:00 2001 From: Casey Bodley Date: Sat, 17 Feb 2024 14:51:11 -0500 Subject: [PATCH] rgw/iam: use retry_raced_role_write() for Role apis Signed-off-by: Casey Bodley --- src/rgw/rgw_rest_iam.h | 19 +++++++ src/rgw/rgw_rest_role.cc | 108 +++++++++++++++++++++++---------------- src/rgw/rgw_role.h | 1 + 3 files changed, 85 insertions(+), 43 deletions(-) diff --git a/src/rgw/rgw_rest_iam.h b/src/rgw/rgw_rest_iam.h index ef0e170c389..00f6ff7dfc4 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_role.h" #include "rgw_sal.h" #include "rgw_xml.h" @@ -72,6 +73,24 @@ int retry_raced_group_write(const DoutPrefixProvider* dpp, optional_yield y, return r; } +/// Perform an atomic read-modify-write operation on the given role metadata. +/// Racing writes are detected here as ECANCELED errors, where we reload the +/// updated group metadata and retry the operation. +template F> +int retry_raced_role_write(const DoutPrefixProvider* dpp, optional_yield y, + rgw::sal::RGWRole* role, const F& f) +{ + int r = f(); + for (int i = 0; i < 10 && r == -ECANCELED; ++i) { + role->get_objv_tracker().clear(); + r = role->get_by_id(dpp, y); + if (r >= 0) { + r = f(); + } + } + return r; +} + class RGWHandler_REST_IAM : public RGWHandler_REST { const rgw::auth::StrategyRegistry& auth_registry; bufferlist bl_post_body; diff --git a/src/rgw/rgw_rest_role.cc b/src/rgw/rgw_rest_role.cc index 295881ce9b9..79c5c4bbac0 100644 --- a/src/rgw/rgw_rest_role.cc +++ b/src/rgw/rgw_rest_role.cc @@ -471,8 +471,11 @@ void RGWModifyRoleTrustPolicy::execute(optional_yield y) } } - role->update_trust_policy(trust_policy); - op_ret = role->update(this, y); + op_ret = retry_raced_role_write(this, y, role.get(), + [this, y] { + role->update_trust_policy(trust_policy); + return role->update(this, y); + }); s->formatter->open_object_section("UpdateAssumeRolePolicyResponse"); s->formatter->open_object_section("ResponseMetadata"); @@ -594,8 +597,11 @@ void RGWPutRolePolicy::execute(optional_yield y) } } - role->set_perm_policy(policy_name, perm_policy); - op_ret = role->update(this, y); + op_ret = retry_raced_role_write(this, y, role.get(), + [this, y] { + role->set_perm_policy(policy_name, perm_policy); + return role->update(this, y); + }); if (op_ret == 0) { s->formatter->open_object_section("PutRolePolicyResponse"); @@ -718,15 +724,17 @@ void RGWDeleteRolePolicy::execute(optional_yield y) } } - op_ret = role->delete_policy(this, policy_name); - if (op_ret == -ENOENT) { - op_ret = -ERR_NO_ROLE_FOUND; - return; - } - - if (op_ret == 0) { - op_ret = role->update(this, y); - } + op_ret = retry_raced_role_write(this, y, role.get(), + [this, y] { + int r = role->delete_policy(this, policy_name); + if (r == -ENOENT) { + s->err.message = "The requested PolicyName was not found"; + return -ERR_NO_SUCH_ENTITY; + } else if (r == 0) { + r = role->update(this, y); + } + return r; + }); s->formatter->open_object_section("DeleteRolePoliciesResponse"); s->formatter->open_object_section("ResponseMetadata"); @@ -782,10 +790,14 @@ void RGWTagRole::execute(optional_yield y) } } - op_ret = role->set_tags(this, tags); - if (op_ret == 0) { - op_ret = role->update(this, y); - } + op_ret = retry_raced_role_write(this, y, role.get(), + [this, y] { + int r = role->set_tags(this, tags); + if (r == 0) { + r = role->update(this, y); + } + return r; + }); if (op_ret == 0) { s->formatter->open_object_section("TagRoleResponse"); @@ -882,8 +894,11 @@ void RGWUntagRole::execute(optional_yield y) } } - role->erase_tags(untag); - op_ret = role->update(this, y); + op_ret = retry_raced_role_write(this, y, role.get(), + [this, y] { + role->erase_tags(untag); + return role->update(this, y); + }); if (op_ret == 0) { s->formatter->open_object_section("UntagRoleResponse"); @@ -939,16 +954,18 @@ void RGWUpdateRole::execute(optional_yield y) } } - if (description) { - role->get_info().description = std::move(*description); - } - role->update_max_session_duration(max_session_duration); - if (!role->validate_max_session_duration(this)) { - op_ret = -EINVAL; - return; - } + op_ret = retry_raced_role_write(this, y, role.get(), + [this, y] { + if (description) { + role->get_info().description = std::move(*description); + } + role->update_max_session_duration(max_session_duration); + if (!role->validate_max_session_duration(this)) { + return -EINVAL; + } - op_ret = role->update(this, y); + return role->update(this, y); + }); s->formatter->open_object_section("UpdateRoleResponse"); s->formatter->open_object_section("UpdateRoleResult"); @@ -1056,12 +1073,15 @@ void RGWAttachRolePolicy_IAM::execute(optional_yield y) return; } - // insert the policy arn. if it's already there, just return success - auto &policies = role->get_info().managed_policies; - const bool inserted = policies.arns.insert(policy_arn).second; - if (inserted) { - op_ret = role->update(this, y); - } + op_ret = retry_raced_role_write(this, y, role.get(), + [this, y] { + // insert the policy arn. if it's already there, just return success + auto &policies = role->get_info().managed_policies; + if (!policies.arns.insert(policy_arn).second) { + return 0; + } + return role->update(this, y); + }); if (op_ret == 0) { s->formatter->open_object_section_in_ns("AttachRolePolicyResponse", RGW_REST_IAM_XMLNS); @@ -1136,15 +1156,17 @@ void RGWDetachRolePolicy_IAM::execute(optional_yield y) } } - auto &policies = role->get_info().managed_policies; - auto p = policies.arns.find(policy_arn); - if (p == policies.arns.end()) { - op_ret = -ERR_NO_SUCH_ENTITY; - s->err.message = "The requested PolicyArn is not attached to the role"; - return; - } - policies.arns.erase(p); - op_ret = role->update(this, y); + op_ret = retry_raced_role_write(this, y, role.get(), + [this, y] { + auto &policies = role->get_info().managed_policies; + auto p = policies.arns.find(policy_arn); + if (p == policies.arns.end()) { + s->err.message = "The requested PolicyArn is not attached to the role"; + return -ERR_NO_SUCH_ENTITY; + } + policies.arns.erase(p); + return role->update(this, y); + }); if (op_ret == 0) { s->formatter->open_object_section_in_ns("DetachRolePolicyResponse", RGW_REST_IAM_XMLNS); diff --git a/src/rgw/rgw_role.h b/src/rgw/rgw_role.h index 9f49351d6e6..585f7239735 100644 --- a/src/rgw/rgw_role.h +++ b/src/rgw/rgw_role.h @@ -135,6 +135,7 @@ public: const std::string& get_create_date() const { return info.creation_date; } const std::string& get_assume_role_policy() const { return info.trust_policy;} const uint64_t& get_max_session_duration() const { return info.max_session_duration; } + RGWObjVersionTracker& get_objv_tracker() { return info.objv_tracker; } const RGWObjVersionTracker& get_objv_tracker() const { return info.objv_tracker; } const real_time& get_mtime() const { return info.mtime; } std::map& get_attrs() { return info.attrs; } -- 2.39.5