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: add check for ACL when create existing bucket #36978

Merged
merged 4 commits into from Sep 14, 2020

Conversation

cbodley
Copy link
Contributor

@cbodley cbodley commented Sep 3, 2020

some cleanups for #35170:

  • use composition of operator==()s to avoid having to encode buffers for comparison
  • adds operator!=()s
  • changes the create_bucket() policy argument from an optional pointer to a const reference
  • moves the acl owner comparison into operator==()

Fixes: https://tracker.ceph.com/issues/47028

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@BryceCao
Copy link
Contributor

BryceCao commented Sep 4, 2020

LGTM, brevity code.

cbodley and others added 2 commits September 10, 2020 13:37
Signed-off-by: Casey Bodley <cbodley@redhat.com>
Fixes: https://tracker.ceph.com/issues/47028

Signed-off-by: caolei <halei15848934852@163.com>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
Copy link
Contributor Author

cbodley commented Sep 10, 2020

i wrote an s3test case for this and it doesn't appear to work. there's extra code in RGWCreateBucket::execute() that handles this -EEXIST error and falls back to an owner comparison. i don't fully understand the retry logic in if (need_metadata_upload() && existed), but that's probably related to the tempest test failures

@cbodley
Copy link
Contributor Author

cbodley commented Sep 10, 2020

see test cases in ceph/s3-tests#356

RGWStore::create_bucket() only returns EEXIST in case of conflicts that
should be returned to the client as errors. the code that applies
changes to metadata should instead use the s->bucket_exists flag

Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
Copy link
Contributor Author

cbodley commented Sep 11, 2020

@dang it looks like my latest commit resolved the tempest test failures in test_create_container_with_remove_metadata_key and test_create_container_with_remove_metadata_value. can you please review that one?

https://pulpito.ceph.com/cbodley-2020-09-11_14:01:23-rgw-wip-cbodley-testing-distro-basic-smithi/

@cbodley
Copy link
Contributor Author

cbodley commented Sep 11, 2020

still having issues with the multisite test_bucket_create() here. when creating a bucket on a secondary zone, it forwards the request to the master first. after that, we go on to call RGWRados::create_bucket(), but that returns -EEXIST if metadata sync raced to create the bucket instance first

RGWRadosStore::create_bucket() only returns EEXIST errors when a
conflict is detected and the recreation should fail. in other cases,
return success and use the 'bool *existed' flag to notify the caller of
its prior existence

Signed-off-by: Casey Bodley <cbodley@redhat.com>
@dang
Copy link
Contributor

dang commented Sep 14, 2020

How about this:

diff --git a/src/rgw/rgw_sal_rados.cc b/src/rgw/rgw_sal_rados.cc
index 4902e72a84c..ba4168e7320 100644
--- a/src/rgw/rgw_sal_rados.cc
+++ b/src/rgw/rgw_sal_rados.cc
@@ -925,6 +925,7 @@ int RGWRadosStore::create_bucket(RGWUser& u, const rgw_bucket& b,
                                    pmaster_bucket, pmaster_num_shards, exclusive);
     if (ret == -EEXIST) {
       *existed = true;
+      ret = 0;
     } else if (ret != 0) {
       return ret;
     }

@cbodley
Copy link
Contributor Author

cbodley commented Sep 14, 2020

@cbodley cbodley merged commit 7b587a0 into ceph:master Sep 14, 2020
@cbodley cbodley deleted the wip-47028 branch September 14, 2020 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants