rgw : add the check for bucket handler if http args exist object_exclusive sub_resource #38741

BryceCao · 2020-12-31T10:24:42Z

Fixes: https://tracker.ceph.com/issues/48727
Signed-off-by: caolei halei15848934852@163.com

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 · 2020-12-31T10:43:30Z

i check all over the sub_resource of bucket and object, "append", "torrent", "uploadId","uploads", "partNumber", "versionId" are object_exclusive, and "cors","notification","location","logging","lifecycle","delete","versions","versioning","website","requestPayment","position","policyStatus","publicAccessBlock" are bucket_exclusive , "acl", "tagging" are shared. So the nonstandard request wont pass.

dang · 2021-01-07T15:39:38Z

src/rgw/rgw_rest_s3.cc

@@ -4878,7 +4878,7 @@ RGWHandler_REST* RGWRESTMgr_S3::get_handler(rgw::sal::RGWRadosStore *store,
  } else {
    if (s->init_state.url_bucket.empty()) {
      handler = new RGWHandler_REST_Service_S3(auth_registry, enable_sts, enable_iam, enable_pubsub);
-    } else if (rgw::sal::RGWObject::empty(s->object.get())) {
+    } else if (rgw::sal::RGWObject::empty(s->object.get()) && !s->info.args.exist_obj_excl_sub_resource()) {


This is the right idea, I think, but I don't think we want to dispatch into the object handler if we have no object. How about re-arranging it so it looks like this:

if (!object.empty()) { handler = object_hander; } else if (exist_obj_excl_sub_resource()) { return NULL; } else { handler = bucket_handler; }

ofriedma · 2021-01-07T15:46:12Z

src/rgw/rgw_common.h

+  bool exist_obj_excl_sub_resource() const {
+    char* obj_sub_resource[] = {"append",  "torrent",    "uploadId",
+                                "uploads", "partNumber", "versionId"};
+    for (int i = 0; i != 6; i++) {


6 should be obj_sub_resource.size()

BryceCao · 2021-01-08T08:59:39Z

@dang Thx, it would be unsafe and Inefficient to dispatch into the object handler when object is empty, i hava corrected the code.

BryceCao · 2021-01-08T08:59:46Z

@ofriedma thank you for advance in code elegance. this part has been modified.

ofriedma · 2021-01-10T11:22:29Z

@BryceCao
One last thing sorry I didn't mention can you please add comment near the complete sub resource (not yours) comment that if someone adds new object sub resource, it should be added to the new structure too.
Other than that LGTM

src/rgw/rgw_rest_s3.cc

BryceCao · 2021-01-12T09:06:34Z

@ofriedma comment has been added.

cbodley · 2021-01-29T17:03:17Z

this appears to be failing test_list_multipart_upload:

======================================================================
ERROR: s3tests_boto3.functional.test_s3.test_list_multipart_upload
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ubuntu/cephtest/s3-tests/virtualenv/lib/python3.6/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/ubuntu/cephtest/s3-tests/s3tests_boto3/functional/test_s3.py", line 7244, in test_list_multipart_upload
    response = client.list_multipart_uploads(Bucket=bucket_name)
  File "/home/ubuntu/cephtest/s3-tests/virtualenv/lib/python3.6/site-packages/botocore/client.py", line 357, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/home/ubuntu/cephtest/s3-tests/virtualenv/lib/python3.6/site-packages/botocore/client.py", line 676, in _make_api_call
    raise error_class(parsed_response, operation_name)
botocore.exceptions.ClientError: An error occurred (MethodNotAllowed) when calling the ListMultipartUploads operation: Unknown
-------------------- >> begin captured logging << --------------------

?uploads is a bucket resource, isn't it?

BryceCao · 2021-01-30T02:02:01Z

this appears to be failing test_list_multipart_upload:

======================================================================
ERROR: s3tests_boto3.functional.test_s3.test_list_multipart_upload
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ubuntu/cephtest/s3-tests/virtualenv/lib/python3.6/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/ubuntu/cephtest/s3-tests/s3tests_boto3/functional/test_s3.py", line 7244, in test_list_multipart_upload
    response = client.list_multipart_uploads(Bucket=bucket_name)
  File "/home/ubuntu/cephtest/s3-tests/virtualenv/lib/python3.6/site-packages/botocore/client.py", line 357, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/home/ubuntu/cephtest/s3-tests/virtualenv/lib/python3.6/site-packages/botocore/client.py", line 676, in _make_api_call
    raise error_class(parsed_response, operation_name)
botocore.exceptions.ClientError: An error occurred (MethodNotAllowed) when calling the ListMultipartUploads operation: Unknown
-------------------- >> begin captured logging << --------------------

?uploads is a bucket resource, isn't it?

Oh, I made a mistake here. ?uploads is a shared resource, because 'POST /{Key}?uploads' can create a MultipartUpload.

…usive sub_resource Fixes: https://tracker.ceph.com/issues/48727 Signed-off-by: caolei <halei15848934852@163.com>

harishmunjulur · 2021-03-04T19:07:13Z

http://pulpito.ceph.com/hmunjulu-2021-03-02_21:00:43-rgw-wip-hmunjulu01-testing-2021-03-02-1039-distro-basic-smithi/

https://trello.com/c/tExJC3nH

github-actions bot added the rgw label Dec 31, 2020

BryceCao mentioned this pull request Dec 31, 2020

rgw: fix the bug that the s3cmd abortmp operation may delete the bucket by mistake. #38731

Closed

dang reviewed Jan 7, 2021

View reviewed changes

ofriedma reviewed Jan 7, 2021

View reviewed changes

BryceCao force-pushed the wip-standardize-request branch 3 times, most recently from acfd483 to b7b0cd6 Compare January 8, 2021 05:50

dang approved these changes Jan 8, 2021

View reviewed changes

ofriedma added the bug-fix label Jan 10, 2021

qwer506 reviewed Jan 12, 2021

View reviewed changes

src/rgw/rgw_rest_s3.cc Show resolved Hide resolved

BryceCao force-pushed the wip-standardize-request branch from b7b0cd6 to 5748a05 Compare January 12, 2021 09:02

ofriedma added the needs-qa label Jan 21, 2021

harishmunjulur added the wip-hmunjulu03-testing label Jan 27, 2021

cbodley removed needs-qa wip-hmunjulu03-testing labels Jan 29, 2021

rgw : add the check for bucket handler if http args exist object_excl…

52a59ad

…usive sub_resource Fixes: https://tracker.ceph.com/issues/48727 Signed-off-by: caolei <halei15848934852@163.com>

BryceCao force-pushed the wip-standardize-request branch from 5748a05 to 52a59ad Compare January 30, 2021 02:09

cbodley added the needs-qa label Jan 30, 2021

harishmunjulur added the wip-hmunjulu01-testing label Feb 4, 2021

harishmunjulur merged commit 9d15a4e into ceph:master Mar 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rgw : add the check for bucket handler if http args exist object_exclusive sub_resource #38741

rgw : add the check for bucket handler if http args exist object_exclusive sub_resource #38741

BryceCao commented Dec 31, 2020

BryceCao commented Dec 31, 2020

dang Jan 7, 2021 •

edited by tchaikov

ofriedma Jan 7, 2021

BryceCao commented Jan 8, 2021

BryceCao commented Jan 8, 2021

ofriedma commented Jan 10, 2021

BryceCao commented Jan 12, 2021

cbodley commented Jan 29, 2021

BryceCao commented Jan 30, 2021

harishmunjulur commented Mar 4, 2021

rgw : add the check for bucket handler if http args exist object_exclusive sub_resource #38741

rgw : add the check for bucket handler if http args exist object_exclusive sub_resource #38741

Conversation

BryceCao commented Dec 31, 2020

Checklist

BryceCao commented Dec 31, 2020

dang Jan 7, 2021 • edited by tchaikov

Choose a reason for hiding this comment

ofriedma Jan 7, 2021

Choose a reason for hiding this comment

BryceCao commented Jan 8, 2021

BryceCao commented Jan 8, 2021

ofriedma commented Jan 10, 2021

BryceCao commented Jan 12, 2021

cbodley commented Jan 29, 2021

BryceCao commented Jan 30, 2021

harishmunjulur commented Mar 4, 2021

dang Jan 7, 2021 •

edited by tchaikov