openstack/swift
Code Issues Proposed changes

Change PUT bucket conflict error

Browse Source 
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
This commit is contained in:
karen chan 2018-06-15 12:44:15 -07:00 committed by Thiago da Silva
parent 5a8cfd6e06
commit 78c9fd9f93
8 changed files with 59 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
doc/s3api/conf/ceph-known-failures-keystone.yaml
View File

@ -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}

2
doc/s3api/conf/ceph-known-failures-tempauth.yaml
View File

@ -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}

2
swift/common/middleware/s3api/acl_handlers.py
View File

@ -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

4
swift/common/middleware/s3api/controllers/multi_upload.py
View File

@ -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)

21
swift/common/middleware/s3api/s3request.py
View File

@ -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),

35
test/functional/s3api/test_bucket.py
View File

@ -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'],

9
test/unit/common/middleware/s3api/test_bucket.py
View File

@ -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')

15
test/unit/common/middleware/s3api/test_multi_upload.py
View File

@ -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 = '<CompleteMultipartUpload>' \
'<Part>' \
@ -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({})