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

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:
+            if version_id is not None:
-                query['version-id'] = req.params['versionId']
+                query['version-id'] = version_id
                 query['symlink'] = 'get'
 
             resp = req.get_response(self.app, query=query)

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',
+        self.swift.register(
-                            swob.HTTPNoContent, {
+            'HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent, {
-                                'X-Container-Sysmeta-Versions-Enabled': True},
+                SYSMETA_VERSIONS_CONT: '\x00versions\x00bucket',
-                            None)
+                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)

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',
+                           'get_container_info', return_value=fake_info):
-                           return_value={'status': 204}):
                     func(*args, **kwargs)
             except AssertionError:
                 # Make traceback message to clarify the assertion