From 78c9fd9f93df75af4b9ca6500d27422963dc89c4 Mon Sep 17 00:00:00 2001 From: karen chan Date: Fri, 15 Jun 2018 12:44:15 -0700 Subject: [PATCH] Change PUT bucket conflict error When a bucket already exists, PUT returned a BucketAlreadyExists error. AWS S3 returns BucketAlreadyOwnedByYou error instead, so this changes the error returned by swift3. When sending a PUT request to a bucket, if the bucket already exists and is not owned by the user, return 409 conflict error, BucketAlreadyExists. Change-Id: I32a0a9add57ca0e4d667b5eb538dc6ea53359944 Closes-Bug: #1498231 --- .../conf/ceph-known-failures-keystone.yaml | 2 -- .../conf/ceph-known-failures-tempauth.yaml | 2 -- swift/common/middleware/s3api/acl_handlers.py | 2 +- .../s3api/controllers/multi_upload.py | 4 +-- swift/common/middleware/s3api/s3request.py | 21 +++++++++-- test/functional/s3api/test_bucket.py | 35 ++++++++++++------- .../common/middleware/s3api/test_bucket.py | 9 ++++- .../middleware/s3api/test_multi_upload.py | 15 ++++---- 8 files changed, 59 insertions(+), 31 deletions(-) diff --git a/doc/s3api/conf/ceph-known-failures-keystone.yaml b/doc/s3api/conf/ceph-known-failures-keystone.yaml index 5d36f5495b..b1f667fe16 100644 --- a/doc/s3api/conf/ceph-known-failures-keystone.yaml +++ b/doc/s3api/conf/ceph-known-failures-keystone.yaml @@ -24,7 +24,6 @@ ceph_s3: s3tests.functional.test_s3.test_bucket_acl_xml_readacp: {status: KNOWN} s3tests.functional.test_s3.test_bucket_acl_xml_write: {status: KNOWN} s3tests.functional.test_s3.test_bucket_acl_xml_writeacp: {status: KNOWN} - s3tests.functional.test_s3.test_bucket_create_exists: {status: KNOWN} s3tests.functional.test_s3.test_bucket_header_acl_grants: {status: KNOWN} s3tests.functional.test_s3.test_bucket_list_objects_anonymous: {status: KNOWN} s3tests.functional.test_s3.test_bucket_list_objects_anonymous_fail: {status: KNOWN} @@ -141,7 +140,6 @@ ceph_s3: s3tests.functional.test_s3_website.test_website_xredirect_private_relative: {status: KNOWN} s3tests.functional.test_s3_website.test_website_xredirect_public_abs: {status: KNOWN} s3tests.functional.test_s3_website.test_website_xredirect_public_relative: {status: KNOWN} - s3tests.functional.test_s3.test_bucket_configure_recreate: {status: KNOWN} s3tests.functional.test_s3.test_bucket_list_return_data_versioning: {status: KNOWN} s3tests.functional.test_s3.test_bucket_policy: {status: KNOWN} s3tests.functional.test_s3.test_bucket_policy_acl: {status: KNOWN} diff --git a/doc/s3api/conf/ceph-known-failures-tempauth.yaml b/doc/s3api/conf/ceph-known-failures-tempauth.yaml index fa070040be..cc3e1e7aa5 100644 --- a/doc/s3api/conf/ceph-known-failures-tempauth.yaml +++ b/doc/s3api/conf/ceph-known-failures-tempauth.yaml @@ -12,7 +12,6 @@ ceph_s3: s3tests.functional.test_s3.test_bucket_acl_grant_email_notexist: {status: KNOWN} s3tests.functional.test_s3.test_bucket_acl_grant_nonexist_user: {status: KNOWN} s3tests.functional.test_s3.test_bucket_acl_no_grants: {status: KNOWN} - s3tests.functional.test_s3.test_bucket_create_exists: {status: KNOWN} s3tests.functional.test_s3.test_bucket_header_acl_grants: {status: KNOWN} s3tests.functional.test_s3.test_bucket_list_objects_anonymous: {status: KNOWN} s3tests.functional.test_s3.test_bucket_list_objects_anonymous_fail: {status: KNOWN} @@ -119,7 +118,6 @@ ceph_s3: s3tests.functional.test_s3_website.test_website_xredirect_private_relative: {status: KNOWN} s3tests.functional.test_s3_website.test_website_xredirect_public_abs: {status: KNOWN} s3tests.functional.test_s3_website.test_website_xredirect_public_relative: {status: KNOWN} - s3tests.functional.test_s3.test_bucket_configure_recreate: {status: KNOWN} s3tests.functional.test_s3.test_bucket_list_return_data_versioning: {status: KNOWN} s3tests.functional.test_s3.test_bucket_policy: {status: KNOWN} s3tests.functional.test_s3.test_bucket_policy_acl: {status: KNOWN} diff --git a/swift/common/middleware/s3api/acl_handlers.py b/swift/common/middleware/s3api/acl_handlers.py index aa8d5509d4..03bac56ddd 100644 --- a/swift/common/middleware/s3api/acl_handlers.py +++ b/swift/common/middleware/s3api/acl_handlers.py @@ -212,7 +212,7 @@ class BucketAclHandler(BaseAclHandler): # To avoid overwriting the existing bucket's ACL, we send PUT # request first before setting the ACL to make sure that the target # container does not exist. - self.req.get_acl_response(app, 'PUT') + self.req.get_acl_response(app, 'PUT', self.container) # update metadata self.req.bucket_acl = req_acl diff --git a/swift/common/middleware/s3api/controllers/multi_upload.py b/swift/common/middleware/s3api/controllers/multi_upload.py index 626a36204d..6ad06d30d9 100644 --- a/swift/common/middleware/s3api/controllers/multi_upload.py +++ b/swift/common/middleware/s3api/controllers/multi_upload.py @@ -74,7 +74,7 @@ from swift.common.middleware.s3api.s3response import InvalidArgument, \ ErrorResponse, MalformedXML, \ InvalidPart, BucketAlreadyExists, EntityTooSmall, InvalidPartOrder, \ InvalidRequest, HTTPOk, HTTPNoContent, NoSuchKey, NoSuchUpload, \ - NoSuchBucket + NoSuchBucket, BucketAlreadyOwnedByYou from swift.common.middleware.s3api.exception import BadSwiftRequest from swift.common.middleware.s3api.utils import unique_id, \ MULTIUPLOAD_SUFFIX, S3Timestamp, sysmeta_header @@ -361,7 +361,7 @@ class UploadsController(Controller): try: req.get_response(self.app, 'PUT', container, '') - except BucketAlreadyExists: + except (BucketAlreadyExists, BucketAlreadyOwnedByYou): pass obj = '%s/%s' % (req.object_name, upload_id) diff --git a/swift/common/middleware/s3api/s3request.py b/swift/common/middleware/s3api/s3request.py index c8280aa502..9c9950379f 100644 --- a/swift/common/middleware/s3api/s3request.py +++ b/swift/common/middleware/s3api/s3request.py @@ -24,7 +24,7 @@ import six from six.moves.urllib.parse import quote, unquote, parse_qsl import string -from swift.common.utils import split_path +from swift.common.utils import split_path, json from swift.common import swob from swift.common.http import HTTP_OK, HTTP_CREATED, HTTP_ACCEPTED, \ HTTP_NO_CONTENT, HTTP_UNAUTHORIZED, HTTP_FORBIDDEN, HTTP_NOT_FOUND, \ @@ -44,7 +44,7 @@ from swift.common.middleware.s3api.controllers import ServiceController, \ UploadController, UploadsController, VersioningController, \ UnsupportedController, S3AclController, BucketController from swift.common.middleware.s3api.s3response import AccessDenied, \ - InvalidArgument, InvalidDigest, \ + InvalidArgument, InvalidDigest, BucketAlreadyOwnedByYou, \ RequestTimeTooSkewed, S3Response, SignatureDoesNotMatch, \ BucketAlreadyExists, BucketNotEmpty, EntityTooLarge, \ InternalError, NoSuchBucket, NoSuchKey, PreconditionFailed, InvalidRange, \ @@ -1099,6 +1099,20 @@ class S3Request(swob.Request): return code_map[method] + def _bucket_put_accepted_error(self, container, app): + sw_req = self.to_swift_req('HEAD', container, None) + info = get_container_info(sw_req.environ, app) + sysmeta = info.get('sysmeta', {}) + try: + acl = json.loads(sysmeta.get('s3api-acl', + sysmeta.get('swift3-acl', '{}'))) + owner = acl.get('Owner') + except (ValueError, TypeError, KeyError): + owner = None + if owner is None or owner == self.user_id: + raise BucketAlreadyOwnedByYou(container) + raise BucketAlreadyExists(container) + def _swift_error_codes(self, method, container, obj, env, app): """ Returns a dict from expected Swift error codes to the corresponding S3 @@ -1120,7 +1134,8 @@ class S3Request(swob.Request): HTTP_NOT_FOUND: (NoSuchBucket, container), }, 'PUT': { - HTTP_ACCEPTED: (BucketAlreadyExists, container), + HTTP_ACCEPTED: (self._bucket_put_accepted_error, container, + app), }, 'POST': { HTTP_NOT_FOUND: (NoSuchBucket, container), diff --git a/test/functional/s3api/test_bucket.py b/test/functional/s3api/test_bucket.py index 28083da5f7..da43bee0b9 100644 --- a/test/functional/s3api/test_bucket.py +++ b/test/functional/s3api/test_bucket.py @@ -17,6 +17,7 @@ import unittest2 import os import test.functional as tf +from swift.common.utils import config_true_value from swift.common.middleware.s3api.etree import fromstring, tostring, Element, \ SubElement from test.functional.s3api import S3ApiBase @@ -152,25 +153,33 @@ class TestS3ApiBucket(S3ApiBase): self.conn.make_request('PUT', 'bucket') status, headers, body = self.conn.make_request('PUT', 'bucket') self.assertEqual(status, 409) - self.assertEqual(get_error_code(body), 'BucketAlreadyExists') + self.assertEqual(get_error_code(body), 'BucketAlreadyOwnedByYou') - if 's3_access_key2' not in tf.config or \ - 's3_secret_key2' not in tf.config: - raise tf.SkipTest( - 'Cannot test for BucketAlreadyExists with second user; need ' - 's3_access_key2 and s3_secret_key2 configured') - # Other users of the same account get the same error - conn2 = Connection(tf.config['s3_access_key2'], - tf.config['s3_secret_key2'], - tf.config['s3_access_key2']) - status, headers, body = conn2.make_request('PUT', 'bucket') - self.assertEqual(status, 409) - self.assertEqual(get_error_code(body), 'BucketAlreadyExists') + def test_put_bucket_error_key2(self): + if config_true_value(tf.cluster_info['s3api'].get('s3_acl')): + if 's3_access_key2' not in tf.config or \ + 's3_secret_key2' not in tf.config: + raise tf.SkipTest( + 'Cannot test for BucketAlreadyExists with second user; ' + 'need s3_access_key2 and s3_secret_key2 configured') + self.conn.make_request('PUT', 'bucket') + + # Other users of the same account get the same 409 error + conn2 = Connection(tf.config['s3_access_key2'], + tf.config['s3_secret_key2'], + tf.config['s3_access_key2']) + status, headers, body = conn2.make_request('PUT', 'bucket') + self.assertEqual(status, 409) + self.assertEqual(get_error_code(body), 'BucketAlreadyExists') + + def test_put_bucket_error_key3(self): if 's3_access_key3' not in tf.config or \ 's3_secret_key3' not in tf.config: raise tf.SkipTest('Cannot test for AccessDenied; need ' 's3_access_key3 and s3_secret_key3 configured') + + self.conn.make_request('PUT', 'bucket') # If the user can't create buckets, they shouldn't even know # whether the bucket exists. conn3 = Connection(tf.config['s3_access_key3'], diff --git a/test/unit/common/middleware/s3api/test_bucket.py b/test/unit/common/middleware/s3api/test_bucket.py index 8346fcc847..a60d9283a2 100644 --- a/test/unit/common/middleware/s3api/test_bucket.py +++ b/test/unit/common/middleware/s3api/test_bucket.py @@ -15,6 +15,7 @@ import unittest import cgi +import mock from swift.common import swob from swift.common.swob import Request @@ -474,6 +475,12 @@ class TestS3ApiBucket(S3ApiTestCase): code = self._test_method_error('PUT', '/bucket', swob.HTTPForbidden) self.assertEqual(code, 'AccessDenied') code = self._test_method_error('PUT', '/bucket', swob.HTTPAccepted) + self.assertEqual(code, 'BucketAlreadyOwnedByYou') + with mock.patch( + 'swift.common.middleware.s3api.s3request.get_container_info', + return_value={'sysmeta': {'s3api-acl': '{"Owner": "nope"}'}}): + code = self._test_method_error( + 'PUT', '/bucket', swob.HTTPAccepted) self.assertEqual(code, 'BucketAlreadyExists') code = self._test_method_error('PUT', '/bucket', swob.HTTPServerError) self.assertEqual(code, 'InternalError') @@ -499,7 +506,7 @@ class TestS3ApiBucket(S3ApiTestCase): self.assertEqual(code, 'InvalidBucketName') @s3acl(s3acl_only=True) - def test_bucket_PUT_error_non_owner(self): + def test_bucket_PUT_error_non_swift_owner(self): code = self._test_method_error('PUT', '/bucket', swob.HTTPAccepted, env={'swift_owner': False}) self.assertEqual(code, 'AccessDenied') diff --git a/test/unit/common/middleware/s3api/test_multi_upload.py b/test/unit/common/middleware/s3api/test_multi_upload.py index 51cb58f0da..bd9dfea0f1 100644 --- a/test/unit/common/middleware/s3api/test_multi_upload.py +++ b/test/unit/common/middleware/s3api/test_multi_upload.py @@ -34,8 +34,6 @@ from test.unit.common.middleware.s3api.test_s3_acl import s3acl from swift.common.middleware.s3api.utils import sysmeta_header, mktime, \ S3Timestamp from swift.common.middleware.s3api.s3request import MAX_32BIT_INT -from swift.common.middleware.s3api.controllers.multi_upload import \ - MULTIUPLOAD_SUFFIX xml = '' \ '' \ @@ -549,11 +547,14 @@ class TestS3ApiMultiUpload(S3ApiTestCase): self.assertEqual(req_headers.get('X-Object-Meta-Foo'), 'bar') self.assertNotIn('Etag', req_headers) self.assertNotIn('Content-MD5', req_headers) - method, path, _ = self.swift.calls_with_headers[-2] - self.assertEqual(method, 'PUT') - self.assertEqual( - path, - '/v1/AUTH_test/bucket%s' % MULTIUPLOAD_SUFFIX) + self.assertEqual([ + ('HEAD', '/v1/AUTH_test/bucket'), + ('PUT', '/v1/AUTH_test/bucket+segments'), + ('HEAD', '/v1/AUTH_test'), + ('HEAD', '/v1/AUTH_test/bucket+segments'), + ('PUT', '/v1/AUTH_test/bucket+segments/object/X'), + ], self.swift.calls) + self.swift.clear_calls() def test_object_multipart_upload_initiate(self): self._test_object_multipart_upload_initiate({})