From 1af995f0e81994671583ce0fff4135164be11b6a Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Thu, 23 Apr 2020 17:21:22 -0700 Subject: [PATCH] s3api: Check whether versioning is enabled more Previously, attempting to GET, HEAD, or DELETE an object with a non-null version-id would cause 500s, with logs complaining about how version-aware operations require that the container is versioned Now, we'll early-return with a 404 (on GET or HEAD) or 204 (on DELETE). Change-Id: I46bfd4ae7d49657a94734962c087f350e758fead Closes-Bug: 1874295 --- .../middleware/s3api/controllers/obj.py | 24 ++++- test/unit/common/middleware/s3api/test_obj.py | 91 ++++++++++++++++++- .../common/middleware/s3api/test_s3_acl.py | 16 +++- 3 files changed, 118 insertions(+), 13 deletions(-) diff --git a/swift/common/middleware/s3api/controllers/obj.py b/swift/common/middleware/s3api/controllers/obj.py index 293b147029..716c837b69 100644 --- a/swift/common/middleware/s3api/controllers/obj.py +++ b/swift/common/middleware/s3api/controllers/obj.py @@ -26,7 +26,7 @@ from swift.common.middleware.versioned_writes.object_versioning import \ from swift.common.middleware.s3api.utils import S3Timestamp, sysmeta_header from swift.common.middleware.s3api.controllers.base import Controller from swift.common.middleware.s3api.s3response import S3NotImplemented, \ - InvalidRange, NoSuchKey, InvalidArgument, HTTPNoContent, \ + InvalidRange, NoSuchKey, NoSuchVersion, InvalidArgument, HTTPNoContent, \ PreconditionFailed @@ -88,7 +88,15 @@ class ObjectController(Controller): if version_id not in ('null', None) and \ 'object_versioning' not in get_swift_info(): raise S3NotImplemented() + query = {} if version_id is None else {'version-id': version_id} + if version_id not in ('null', None): + container_info = req.get_container_info(self.app) + if not container_info.get( + 'sysmeta', {}).get('versions-container', ''): + # Versioning has never been enabled + raise NoSuchVersion(object_name, version_id) + resp = req.get_response(self.app, query=query) if req.method == 'HEAD': @@ -193,17 +201,25 @@ class ObjectController(Controller): 'object_versioning' not in get_swift_info(): raise S3NotImplemented() + version_id = req.params.get('versionId') + if version_id not in ('null', None): + container_info = req.get_container_info(self.app) + if not container_info.get( + 'sysmeta', {}).get('versions-container', ''): + # Versioning has never been enabled + return HTTPNoContent(headers={'x-amz-version-id': version_id}) + try: try: query = req.gen_multipart_manifest_delete_query( - self.app, version=req.params.get('versionId')) + self.app, version=version_id) except NoSuchKey: query = {} req.headers['Content-Type'] = None # Ignore client content-type - if 'versionId' in req.params: - query['version-id'] = req.params['versionId'] + if version_id is not None: + query['version-id'] = version_id query['symlink'] = 'get' resp = req.get_response(self.app, query=query) diff --git a/test/unit/common/middleware/s3api/test_obj.py b/test/unit/common/middleware/s3api/test_obj.py index 363a1b2cbb..3b80cc3553 100644 --- a/test/unit/common/middleware/s3api/test_obj.py +++ b/test/unit/common/middleware/s3api/test_obj.py @@ -34,7 +34,7 @@ from swift.common.middleware.s3api.subresource import ACL, User, encode_acl, \ from swift.common.middleware.s3api.etree import fromstring from swift.common.middleware.s3api.utils import mktime, S3Timestamp from swift.common.middleware.versioned_writes.object_versioning import \ - DELETE_MARKER_CONTENT_TYPE + DELETE_MARKER_CONTENT_TYPE, SYSMETA_VERSIONS_CONT, SYSMETA_VERSIONS_ENABLED class TestS3ApiObj(S3ApiTestCase): @@ -402,6 +402,10 @@ class TestS3ApiObj(S3ApiTestCase): @s3acl def test_object_GET_version_id(self): + self.swift.register( + 'HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent, + {SYSMETA_VERSIONS_CONT: '\x00versions\x00bucket'}, None) + # GET current version req = Request.blank('/bucket/object?versionId=null', environ={'REQUEST_METHOD': 'GET'}, @@ -452,6 +456,28 @@ class TestS3ApiObj(S3ApiTestCase): status, headers, body = self.call_s3api(req) self.assertEqual(status.split()[0], '404') + @s3acl(versioning_enabled=False) + def test_object_GET_with_version_id_but_not_enabled(self): + # Version not found + self.swift.register( + 'HEAD', '/v1/AUTH_test/bucket', + swob.HTTPNoContent, {}, None) + req = Request.blank('/bucket/object?versionId=A', + environ={'REQUEST_METHOD': 'GET'}, + headers={'Authorization': 'AWS test:tester:hmac', + 'Date': self.get_date_header()}) + status, headers, body = self.call_s3api(req) + self.assertEqual(status.split()[0], '404') + elem = fromstring(body, 'Error') + self.assertEqual(elem.find('Code').text, 'NoSuchVersion') + self.assertEqual(elem.find('Key').text, 'object') + self.assertEqual(elem.find('VersionId').text, 'A') + expected_calls = [] + if not self.swift.s3_acl: + expected_calls.append(('HEAD', '/v1/AUTH_test/bucket')) + # NB: No actual backend GET! + self.assertEqual(expected_calls, self.swift.calls) + @s3acl def test_object_PUT_error(self): code = self._test_method_error('PUT', '/bucket/object', @@ -1100,6 +1126,9 @@ class TestS3ApiObj(S3ApiTestCase): def test_object_DELETE_old_version_id(self): self.swift.register('HEAD', '/v1/AUTH_test/bucket/object', swob.HTTPOk, self.response_headers, None) + self.swift.register( + 'HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent, + {SYSMETA_VERSIONS_CONT: '\x00versions\x00bucket'}, None) resp_headers = {'X-Object-Current-Version-Id': '1574360804.34906'} self.swift.register('DELETE', '/v1/AUTH_test/bucket/object' '?symlink=get&version-id=1574358170.12293', @@ -1111,6 +1140,7 @@ class TestS3ApiObj(S3ApiTestCase): status, headers, body = self.call_s3api(req) self.assertEqual(status.split()[0], '204') self.assertEqual([ + ('HEAD', '/v1/AUTH_test/bucket'), ('HEAD', '/v1/AUTH_test/bucket/object' '?symlink=get&version-id=1574358170.12293'), ('DELETE', '/v1/AUTH_test/bucket/object' @@ -1118,6 +1148,11 @@ class TestS3ApiObj(S3ApiTestCase): ], self.swift.calls) def test_object_DELETE_current_version_id(self): + self.swift.register( + 'HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent, { + SYSMETA_VERSIONS_CONT: '\x00versions\x00bucket', + SYSMETA_VERSIONS_ENABLED: True}, + None) self.swift.register('HEAD', '/v1/AUTH_test/bucket/object', swob.HTTPOk, self.response_headers, None) resp_headers = {'X-Object-Current-Version-Id': 'null'} @@ -1142,6 +1177,7 @@ class TestS3ApiObj(S3ApiTestCase): status, headers, body = self.call_s3api(req) self.assertEqual(status.split()[0], '204') self.assertEqual([ + ('HEAD', '/v1/AUTH_test/bucket'), ('HEAD', '/v1/AUTH_test/bucket/object' '?symlink=get&version-id=1574358170.12293'), ('DELETE', '/v1/AUTH_test/bucket/object' @@ -1152,6 +1188,22 @@ class TestS3ApiObj(S3ApiTestCase): '?version-id=1574341899.21751'), ], self.swift.calls) + @s3acl(versioning_enabled=False) + def test_object_DELETE_with_version_id_but_not_enabled(self): + self.swift.register('HEAD', '/v1/AUTH_test/bucket', + swob.HTTPNoContent, {}, None) + req = Request.blank('/bucket/object?versionId=1574358170.12293', + method='DELETE', headers={ + 'Authorization': 'AWS test:tester:hmac', + 'Date': self.get_date_header()}) + status, headers, body = self.call_s3api(req) + self.assertEqual(status.split()[0], '204') + expected_calls = [] + if not self.swift.s3_acl: + expected_calls.append(('HEAD', '/v1/AUTH_test/bucket')) + # NB: No actual backend DELETE! + self.assertEqual(expected_calls, self.swift.calls) + def test_object_DELETE_version_id_not_implemented(self): req = Request.blank('/bucket/object?versionId=1574358170.12293', method='DELETE', headers={ @@ -1164,6 +1216,11 @@ class TestS3ApiObj(S3ApiTestCase): self.assertEqual(status.split()[0], '501', body) def test_object_DELETE_current_version_id_is_delete_marker(self): + self.swift.register( + 'HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent, { + SYSMETA_VERSIONS_CONT: '\x00versions\x00bucket', + SYSMETA_VERSIONS_ENABLED: True}, + None) self.swift.register('HEAD', '/v1/AUTH_test/bucket/object', swob.HTTPOk, self.response_headers, None) resp_headers = {'X-Object-Current-Version-Id': 'null'} @@ -1184,6 +1241,7 @@ class TestS3ApiObj(S3ApiTestCase): status, headers, body = self.call_s3api(req) self.assertEqual(status.split()[0], '204') self.assertEqual([ + ('HEAD', '/v1/AUTH_test/bucket'), ('HEAD', '/v1/AUTH_test/bucket/object' '?symlink=get&version-id=1574358170.12293'), ('DELETE', '/v1/AUTH_test/bucket/object' @@ -1193,6 +1251,11 @@ class TestS3ApiObj(S3ApiTestCase): ], self.swift.calls) def test_object_DELETE_current_version_id_is_missing(self): + self.swift.register( + 'HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent, { + SYSMETA_VERSIONS_CONT: '\x00versions\x00bucket', + SYSMETA_VERSIONS_ENABLED: True}, + None) self.swift.register('HEAD', '/v1/AUTH_test/bucket/object', swob.HTTPOk, self.response_headers, None) resp_headers = {'X-Object-Current-Version-Id': 'null'} @@ -1223,6 +1286,7 @@ class TestS3ApiObj(S3ApiTestCase): status, headers, body = self.call_s3api(req) self.assertEqual(status.split()[0], '204') self.assertEqual([ + ('HEAD', '/v1/AUTH_test/bucket'), ('HEAD', '/v1/AUTH_test/bucket/object' '?symlink=get&version-id=1574358170.12293'), ('DELETE', '/v1/AUTH_test/bucket/object' @@ -1236,6 +1300,11 @@ class TestS3ApiObj(S3ApiTestCase): ], self.swift.calls) def test_object_DELETE_current_version_id_GET_error(self): + self.swift.register( + 'HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent, { + SYSMETA_VERSIONS_CONT: '\x00versions\x00bucket', + SYSMETA_VERSIONS_ENABLED: True}, + None) self.swift.register('HEAD', '/v1/AUTH_test/bucket/object', swob.HTTPOk, self.response_headers, None) resp_headers = {'X-Object-Current-Version-Id': 'null'} @@ -1251,6 +1320,7 @@ class TestS3ApiObj(S3ApiTestCase): status, headers, body = self.call_s3api(req) self.assertEqual(status.split()[0], '500') self.assertEqual([ + ('HEAD', '/v1/AUTH_test/bucket'), ('HEAD', '/v1/AUTH_test/bucket/object' '?symlink=get&version-id=1574358170.12293'), ('DELETE', '/v1/AUTH_test/bucket/object' @@ -1260,6 +1330,11 @@ class TestS3ApiObj(S3ApiTestCase): ], self.swift.calls) def test_object_DELETE_current_version_id_PUT_error(self): + self.swift.register( + 'HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent, { + SYSMETA_VERSIONS_CONT: '\x00versions\x00bucket', + SYSMETA_VERSIONS_ENABLED: True}, + None) self.swift.register('HEAD', '/v1/AUTH_test/bucket/object', swob.HTTPOk, self.response_headers, None) resp_headers = {'X-Object-Current-Version-Id': 'null'} @@ -1283,6 +1358,7 @@ class TestS3ApiObj(S3ApiTestCase): status, headers, body = self.call_s3api(req) self.assertEqual(status.split()[0], '500') self.assertEqual([ + ('HEAD', '/v1/AUTH_test/bucket'), ('HEAD', '/v1/AUTH_test/bucket/object' '?symlink=get&version-id=1574358170.12293'), ('DELETE', '/v1/AUTH_test/bucket/object' @@ -1325,10 +1401,13 @@ class TestS3ApiObj(S3ApiTestCase): 'X-Object-Version-Id': '1574701081.61553'} self.swift.register('DELETE', '/v1/AUTH_test/bucket/object', swob.HTTPNoContent, resp_headers, None) - self.swift.register('HEAD', '/v1/AUTH_test/bucket', - swob.HTTPNoContent, { - 'X-Container-Sysmeta-Versions-Enabled': True}, - None) + self.swift.register( + 'HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent, { + SYSMETA_VERSIONS_CONT: '\x00versions\x00bucket', + SYSMETA_VERSIONS_ENABLED: True}, + None) + self.swift.register('HEAD', '/v1/AUTH_test/\x00versions\x00bucket', + swob.HTTPNoContent, {}, None) self.swift.register('HEAD', '/v1/AUTH_test/bucket/object', swob.HTTPNotFound, self.response_headers, None) req = Request.blank('/bucket/object?versionId=1574701081.61553', @@ -1338,10 +1417,12 @@ class TestS3ApiObj(S3ApiTestCase): status, headers, body = self.call_s3api(req) self.assertEqual(status.split()[0], '204') self.assertEqual([ + ('HEAD', '/v1/AUTH_test/bucket'), ('HEAD', '/v1/AUTH_test/bucket/object' '?symlink=get&version-id=1574701081.61553'), ('HEAD', '/v1/AUTH_test'), ('HEAD', '/v1/AUTH_test/bucket'), + ('HEAD', '/v1/AUTH_test/\x00versions\x00bucket'), ('DELETE', '/v1/AUTH_test/bucket/object' '?symlink=get&version-id=1574701081.61553'), ], self.swift.calls) diff --git a/test/unit/common/middleware/s3api/test_s3_acl.py b/test/unit/common/middleware/s3api/test_s3_acl.py index 48f5439162..da0472a669 100644 --- a/test/unit/common/middleware/s3api/test_s3_acl.py +++ b/test/unit/common/middleware/s3api/test_s3_acl.py @@ -34,13 +34,16 @@ from test.unit.common.middleware.s3api import FakeSwift XMLNS_XSI = 'http://www.w3.org/2001/XMLSchema-instance' -def s3acl(func=None, s3acl_only=False): +def s3acl(func=None, s3acl_only=False, versioning_enabled=True): """ NOTE: s3acl decorator needs an instance of s3api testing framework. (i.e. An instance for first argument is necessary) """ if func is None: - return functools.partial(s3acl, s3acl_only=s3acl_only) + return functools.partial( + s3acl, + s3acl_only=s3acl_only, + versioning_enabled=versioning_enabled) @functools.wraps(func) def s3acl_decorator(*args, **kwargs): @@ -57,9 +60,14 @@ def s3acl(func=None, s3acl_only=False): # @patch(xxx) # def test_xxxx(self) + fake_info = {'status': 204} + if versioning_enabled: + fake_info['sysmeta'] = { + 'versions-container': '\x00versions\x00bucket', + } + with patch('swift.common.middleware.s3api.s3request.' - 'get_container_info', - return_value={'status': 204}): + 'get_container_info', return_value=fake_info): func(*args, **kwargs) except AssertionError: # Make traceback message to clarify the assertion