]> git.apps.os.sepia.ceph.com Git - ceph.git/commitdiff
rgw/iam: use retry_raced_user_write() for User/AccessKey apis
authorCasey Bodley <cbodley@redhat.com>
Sat, 17 Feb 2024 16:42:12 +0000 (11:42 -0500)
committerCasey Bodley <cbodley@redhat.com>
Fri, 12 Apr 2024 19:34:29 +0000 (15:34 -0400)
Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit 91c0a628f0aadd99ae5edb10ba69d2c2bcf29cd1)

src/rgw/rgw_rest_iam.h
src/rgw/rgw_rest_iam_group.cc
src/rgw/rgw_rest_iam_user.cc
src/rgw/rgw_rest_user_policy.cc

index 0c2b262bd62b7614a43b09baacfb84692543118f..ef0e170c38924883051e64cc1a66e53d6e4978da 100644 (file)
@@ -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 <std::invocable<> 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.
index 307ba8937baf8bef0e95301ab3c3c2ba7537cd58..9819cbb53a00da6248431727f7404226b57af92a 100644 (file)
@@ -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()
index 8dd6ada3077a37d4eee4c2141d6c28f1c0390325..90a50e3949e5ae30bd250661fad80d65021ccd94 100644 (file)
@@ -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<int> 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()
index 2dc10711ea599ab79aefbd6a90891cefabbecff8..0da2552fc2f066572d3551f36e30e7cc6776969d 100644 (file)
@@ -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<bool>("rgw_policy_reject_invalid_principals"));
-    std::map<std::string, std::string> 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<std::string, std::string> 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<std::string, std::string> 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<std::string, std::string> 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);