Merge pull request ceph#60227 from clwluvw/zonegroup-delbucket

cbodley · web-flow · commit 83c9201e53c0 · 2025-04-23T14:02:16.000-04:00
rgw: skip empty check on non-owned buckets by zonegroup

Reviewed-by: Casey Bodley &lt;cbodley@redhat.com&gt;
Reviewed-by: Anthony D'Atri &lt;anthony.datri@gmail.com&gt;
diff --git a/doc/radosgw/adminops.rst b/doc/radosgw/adminops.rst
@@ -1510,13 +1510,27 @@ Request Parameters
 :Example: ``foo_bucket``
 :Required: Yes
 
+``tenant``
+
+:Description: The tenant under which the bucket is to be removed.
+:Type: String
+:Example: ``tenant1``
+:Required: No
+
 ``purge-objects``
 
 :Description: Remove a buckets objects before deletion.
 :Type: Boolean
 :Example: True [False]
 :Required: No
 
+``bypass-gc``
+
+:Description: Bypass garbage collection.
+:Type: Boolean
+:Example: True [False]
+:Required: No
+
 Response Entities
 ~~~~~~~~~~~~~~~~~
 
diff --git a/src/common/options/rgw.yaml.in b/src/common/options/rgw.yaml.in
@@ -1025,6 +1025,8 @@ options:
   level: advanced
   desc: Path prefix to be used for accessing RGW RESTful admin API.
   fmt_desc: The entry point for an admin request URL.
+  long_desc: Note that multisite replication requires the value admin,
+    so this option must be left at the default in such deployments.
   default: admin
   services:
   - rgw
diff --git a/src/rgw/driver/motr/rgw_sal_motr.cc b/src/rgw/driver/motr/rgw_sal_motr.cc
@@ -557,12 +557,15 @@ int MotrBucket::remove(const DoutPrefixProvider *dpp, bool delete_children, opti
   params.list_versions = true;
   params.allow_unordered = true;
 
+  const bool own_bucket = store->get_zone()->get_zonegroup().get_id() == info.zonegroup;
+
   ListResults results;
+  results.is_truncated = own_bucket; // if we don't have the index, we're done
 
   // 1. Check if Bucket has objects.
   // If bucket contains objects and delete_children is true, delete all objects.
   // Else throw error that bucket is not empty.
-  do {
+  while (results.is_truncated) {
     results.objs.clear();
 
     // Check if bucket has objects.
@@ -591,12 +594,14 @@ int MotrBucket::remove(const DoutPrefixProvider *dpp, bool delete_children, opti
 	      return ret;
       }
     }
-  } while(results.is_truncated);
+  }
 
   // 2. Abort Mp uploads on the bucket.
-  ret = abort_multiparts(dpp, store->ctx());
-  if (ret < 0) {
-    return ret;
+  if (own_bucket) {
+    ret = abort_multiparts(dpp, store->ctx());
+    if (ret < 0) {
+      return ret;
+    }
   }
 
   // 3. Remove mp index??
@@ -608,9 +613,11 @@ int MotrBucket::remove(const DoutPrefixProvider *dpp, bool delete_children, opti
   }
 
   // 4. Sync user stats.
-  ret = this->sync_owner_stats(dpp, y);
-  if (ret < 0) {
-     ldout(store->ctx(), 1) << "WARNING: failed sync user stats before bucket delete. ret=" <<  ret << dendl;
+  if (own_bucket) {
+    ret = this->sync_owner_stats(dpp, y);
+    if (ret < 0) {
+      ldout(store->ctx(), 1) << "WARNING: failed sync user stats before bucket delete. ret=" <<  ret << dendl;
+    }
   }
 
   // 5. Remove the bucket from user info index. (unlink user)
diff --git a/src/rgw/driver/rados/rgw_bucket.cc b/src/rgw/driver/rados/rgw_bucket.cc
@@ -1440,9 +1440,10 @@ int RGWBucketAdminOp::check_index(rgw::sal::Driver* driver,
   return 0;
 }
 
-int RGWBucketAdminOp::remove_bucket(rgw::sal::Driver* driver, RGWBucketAdminOpState& op_state,
+int RGWBucketAdminOp::remove_bucket(rgw::sal::Driver* driver, const rgw::SiteConfig& site,
+                                    RGWBucketAdminOpState& op_state,
 				    optional_yield y, const DoutPrefixProvider *dpp, 
-                                    bool bypass_gc, bool keep_index_consistent)
+                                    bool bypass_gc, bool keep_index_consistent, bool forwarded_request)
 {
   std::unique_ptr<rgw::sal::Bucket> bucket;
 
@@ -1452,10 +1453,36 @@ int RGWBucketAdminOp::remove_bucket(rgw::sal::Driver* driver, RGWBucketAdminOpSt
   if (ret < 0)
     return ret;
 
+  // check if the bucket is owned by my zonegroup, if not, refuse to remove it
+  if (!forwarded_request && bucket->get_info().zonegroup != driver->get_zone()->get_zonegroup().get_id()) {
+    ldpp_dout(dpp, 0) << "ERROR: The bucket's zonegroup does not match. "
+                      << "Removal is not allowed. Please execute this operation "
+                      << "on the bucket's zonegroup: " << bucket->get_info().zonegroup << dendl;
+    return -ERR_PERMANENT_REDIRECT;
+  }
+
   if (bypass_gc)
     ret = bucket->remove_bypass_gc(op_state.get_max_aio(), keep_index_consistent, y, dpp);
   else
     ret = bucket->remove(dpp, op_state.will_delete_children(), y);
+  if (ret < 0)
+    return ret;
+
+  // forward to master zonegroup
+  const std::string delpath = "/admin/bucket";
+  RGWEnv env;
+  env.set("REQUEST_METHOD", "DELETE");
+  env.set("SCRIPT_URI", delpath);
+  env.set("REQUEST_URI", delpath);
+  env.set("QUERY_STRING", fmt::format("bucket={}&tenant={}", bucket->get_name(), bucket->get_tenant()));
+  req_info req(dpp->get_cct(), &env);
+
+  ret = rgw_forward_request_to_master(dpp, site, bucket->get_owner(), nullptr, nullptr, req, y);
+  if (ret < 0) {
+    ldpp_dout(dpp, 0) << "ERROR: failed to forward request to master zonegroup: "
+                      << ret << dendl;
+    return ret;
+  }
 
   return ret;
 }
diff --git a/src/rgw/driver/rados/rgw_bucket.h b/src/rgw/driver/rados/rgw_bucket.h
@@ -392,8 +392,8 @@ class RGWBucketAdminOp {
   static int check_index_unlinked(rgw::sal::RadosStore* driver, RGWBucketAdminOpState& op_state,
                                   RGWFormatterFlusher& flusher, const DoutPrefixProvider *dpp);
 
-  static int remove_bucket(rgw::sal::Driver* driver, RGWBucketAdminOpState& op_state, optional_yield y,
-			   const DoutPrefixProvider *dpp, bool bypass_gc = false, bool keep_index_consistent = true);
+  static int remove_bucket(rgw::sal::Driver* driver, const rgw::SiteConfig& site, RGWBucketAdminOpState& op_state, optional_yield y,
+			   const DoutPrefixProvider *dpp, bool bypass_gc = false, bool keep_index_consistent = true, bool forwarded_request = false);
   static int remove_object(rgw::sal::Driver* driver, RGWBucketAdminOpState& op_state, const DoutPrefixProvider *dpp, optional_yield y);
   static int info(rgw::sal::Driver* driver, RGWBucketAdminOpState& op_state, RGWFormatterFlusher& flusher, optional_yield y, const DoutPrefixProvider *dpp);
   static int limit_check(rgw::sal::Driver* driver, RGWBucketAdminOpState& op_state,
diff --git a/src/rgw/driver/rados/rgw_rados.cc b/src/rgw/driver/rados/rgw_rados.cc
@@ -5512,6 +5512,12 @@ int RGWRados::restore_obj_from_cloud(RGWLCCloudTierCtx& tier_ctx,
 
 int RGWRados::check_bucket_empty(const DoutPrefixProvider *dpp, RGWBucketInfo& bucket_info, optional_yield y)
 {
+  const auto& zonegroup = svc.site->get_zonegroup();
+  if (bucket_info.zonegroup != zonegroup.id) {
+    // don't check on this zone, the bucket's data belongs to another zonegroup
+    return 0;
+  }
+
   constexpr uint NUM_ENTRIES = 1000u;
 
   rgw_obj_index_key marker;
diff --git a/src/rgw/driver/rados/rgw_rest_bucket.cc b/src/rgw/driver/rados/rgw_rest_bucket.cc
@@ -216,36 +216,24 @@ class RGWOp_Bucket_Remove : public RGWRESTOp {
 
 void RGWOp_Bucket_Remove::execute(optional_yield y)
 {
-  std::string bucket_name;
-  bool delete_children;
-  std::unique_ptr<rgw::sal::Bucket> bucket;
+  std::string bucket_name, tenant;
+  bool delete_children, bypass_gc;
 
   RESTArgs::get_string(s, "bucket", bucket_name, &bucket_name);
+  RESTArgs::get_string(s, "tenant", tenant, &tenant);
   RESTArgs::get_bool(s, "purge-objects", false, &delete_children);
+  RESTArgs::get_bool(s, "bypass-gc", false, &bypass_gc);
 
-  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) << "forward_request_to_master returned ret=" << op_ret << dendl;
-    if (op_ret == -ENOENT) {
-      /* adjust error, we want to return with NoSuchBucket and not
-       * NoSuchKey */
-      op_ret = -ERR_NO_SUCH_BUCKET;
-    }
-    return;
-  }
+  RGWBucketAdminOpState op_state;
+  op_state.set_bucket_name(bucket_name);
+  op_state.set_tenant(tenant);
+  op_state.set_delete_children(delete_children);
 
-  op_ret = driver->load_bucket(s, rgw_bucket("", bucket_name),
-                               &bucket, y);
-  if (op_ret < 0) {
-    ldpp_dout(this, 0) << "get_bucket returned ret=" << op_ret << dendl;
-    if (op_ret == -ENOENT) {
-      op_ret = -ERR_NO_SUCH_BUCKET;
-    }
-    return;
-  }
+  // Check if the request is being forwarded by looking for the "rgwx-zonegroup" argument
+  // As this is an admin endpoint, checking by system_request is not sufficient
+  const bool is_forwarded = s->info.args.exists(RGW_SYS_PARAM_PREFIX "zonegroup");
 
-  op_ret = bucket->remove(s, delete_children, s->yield);
+  op_ret = RGWBucketAdminOp::remove_bucket(driver, *s->penv.site, op_state, y, s, bypass_gc, true, is_forwarded);
 }
 
 class RGWOp_Set_Bucket_Quota : public RGWRESTOp {
diff --git a/src/rgw/driver/rados/rgw_sal_rados.cc b/src/rgw/driver/rados/rgw_sal_rados.cc
@@ -375,9 +375,12 @@ int RadosBucket::remove(const DoutPrefixProvider* dpp,
   params.list_versions = true;
   params.allow_unordered = true;
 
+  const bool own_bucket = store->get_zone()->get_zonegroup().get_id() == info.zonegroup;
+
   ListResults results;
+  results.is_truncated = own_bucket; // if we don't have the index, we're done
 
-  do {
+  while (results.is_truncated) {
     results.objs.clear();
 
     ret = list(dpp, params, 1000, results, y);
@@ -399,11 +402,13 @@ int RadosBucket::remove(const DoutPrefixProvider* dpp,
 	return ret;
       }
     }
-  } while(results.is_truncated);
+  }
 
-  ret = abort_multiparts(dpp, store->ctx(), y);
-  if (ret < 0) {
-    return ret;
+  if (own_bucket) {
+    ret = abort_multiparts(dpp, store->ctx(), y);
+    if (ret < 0) {
+      return ret;
+    }
   }
 
   // remove lifecycle config, if any (XXX note could be made generic)
@@ -438,9 +443,11 @@ int RadosBucket::remove(const DoutPrefixProvider* dpp,
   }
 
   librados::Rados& rados = *store->getRados()->get_rados_handle();
-  ret = store->ctl()->bucket->sync_owner_stats(dpp, rados, info.owner, info, y, nullptr);
-  if (ret < 0) {
-     ldout(store->ctx(), 1) << "WARNING: failed sync user stats before bucket delete. ret=" <<  ret << dendl;
+  if (own_bucket) {
+    ret = store->ctl()->bucket->sync_owner_stats(dpp, rados, info.owner, info, y, nullptr);
+    if (ret < 0) {
+      ldout(store->ctx(), 1) << "WARNING: failed sync user stats before bucket delete. ret=" <<  ret << dendl;
+    }
   }
 
   RGWObjVersionTracker ot;
diff --git a/src/rgw/radosgw-admin/radosgw-admin.cc b/src/rgw/radosgw-admin/radosgw-admin.cc
@@ -6693,7 +6693,6 @@ int main(int argc, const char **argv)
                                         OPT::USER_SUSPEND, OPT::SUBUSER_CREATE,
                                         OPT::SUBUSER_MODIFY, OPT::SUBUSER_RM,
                                         OPT::BUCKET_LINK, OPT::BUCKET_UNLINK,
-                                        OPT::BUCKET_RM,
                                         OPT::BUCKET_CHOWN, OPT::METADATA_PUT,
                                         OPT::METADATA_RM, OPT::MFA_CREATE,
                                         OPT::MFA_REMOVE, OPT::MFA_RESYNC,
@@ -9143,14 +9142,14 @@ int main(int argc, const char **argv)
 
   if (opt_cmd == OPT::BUCKET_RM) {
     if (!inconsistent_index) {
-      RGWBucketAdminOp::remove_bucket(driver, bucket_op, null_yield, dpp(), bypass_gc, true);
+      RGWBucketAdminOp::remove_bucket(driver, *site, bucket_op, null_yield, dpp(), bypass_gc, true, false);
     } else {
       if (!yes_i_really_mean_it) {
 	cerr << "using --inconsistent_index can corrupt the bucket index " << std::endl
 	<< "do you really mean it? (requires --yes-i-really-mean-it)" << std::endl;
 	return 1;
       }
-      RGWBucketAdminOp::remove_bucket(driver, bucket_op, null_yield, dpp(), bypass_gc, false);
+      RGWBucketAdminOp::remove_bucket(driver, *site, bucket_op, null_yield, dpp(), bypass_gc, false, false);
     }
   }
 
diff --git a/src/rgw/rgw_auth.cc b/src/rgw/rgw_auth.cc
@@ -368,13 +368,6 @@ uint32_t rgw_perms_from_aclspec_default_strategy(
 }
 
 
-static inline const std::string make_spec_item(const std::string& tenant,
-                                               const std::string& id)
-{
-  return tenant + ":" + id;
-}
-
-
 static inline std::pair<bool, rgw::auth::Engine::result_t>
 strategy_handle_rejected(rgw::auth::Engine::result_t&& engine_result,
                          const rgw::auth::Strategy::Control policy,
diff --git a/src/rgw/rgw_op.cc b/src/rgw/rgw_op.cc
@@ -3867,14 +3867,19 @@ void RGWDeleteBucket::execute(optional_yield y)
     }
   }
 
-  op_ret = s->bucket->sync_owner_stats(this, y, nullptr);
-  if ( op_ret < 0) {
-     ldpp_dout(this, 1) << "WARNING: failed to sync user stats before bucket delete: op_ret= " << op_ret << dendl;
-  }
+  const bool own_bucket = s->penv.site->get_zonegroup().get_id() == s->bucket->get_info().zonegroup;
 
-  op_ret = s->bucket->check_empty(this, y);
-  if (op_ret < 0) {
-    return;
+  if (own_bucket) {
+    // only if we own the bucket
+    op_ret = s->bucket->sync_owner_stats(this, y, nullptr);
+    if (op_ret < 0) {
+      ldpp_dout(this, 1) << "WARNING: failed to sync user stats before bucket delete: op_ret= " << op_ret << dendl;
+    }
+
+    op_ret = s->bucket->check_empty(this, y);
+    if (op_ret < 0) {
+      return;
+    }
   }
 
   op_ret = rgw_forward_request_to_master(this, *s->penv.site, s->owner.id,
@@ -3888,9 +3893,11 @@ void RGWDeleteBucket::execute(optional_yield y)
     return;
   }
 
-  op_ret = rgw_remove_sse_s3_bucket_key(s, y);
-  if (op_ret != 0) {
-      // do nothing; it will already have been logged
+  if (own_bucket) {
+    op_ret = rgw_remove_sse_s3_bucket_key(s, y);
+    if (op_ret != 0) {
+        // do nothing; it will already have been logged
+    }
   }
 
   op_ret = s->bucket->remove(this, false, y);
diff --git a/src/test/rgw/rgw_multi/tests.py b/src/test/rgw/rgw_multi/tests.py
@@ -683,21 +683,21 @@ def test_bucket_recreate():
         assert check_all_buckets_exist(zone, buckets)
 
 def test_bucket_remove():
-    zonegroup = realm.master_zonegroup()
-    zonegroup_conns = ZonegroupConns(zonegroup)
-    buckets, zone_bucket = create_bucket_per_zone(zonegroup_conns)
-    zonegroup_meta_checkpoint(zonegroup)
+    for zonegroup in realm.current_period.zonegroups:
+        zonegroup_conns = ZonegroupConns(zonegroup)
+        buckets, zone_buckets = create_bucket_per_zone(zonegroup_conns)
+        zonegroup_meta_checkpoint(zonegroup)
 
-    for zone in zonegroup_conns.zones:
-        assert check_all_buckets_exist(zone, buckets)
+        for zone in zonegroup_conns.zones:
+            assert check_all_buckets_exist(zone, buckets)
 
-    for zone, bucket_name in zone_bucket:
-        zone.conn.delete_bucket(bucket_name)
+        for zone, bucket_name in zone_buckets:
+            zone.conn.delete_bucket(bucket_name)
 
-    zonegroup_meta_checkpoint(zonegroup)
+        zonegroup_meta_checkpoint(zonegroup)
 
-    for zone in zonegroup_conns.zones:
-        assert check_all_buckets_dont_exist(zone, buckets)
+        for zone in zonegroup_conns.zones:
+            assert check_all_buckets_dont_exist(zone, buckets)
 
 def get_bucket(zone, bucket_name):
     return zone.conn.get_bucket(bucket_name)
