From 179fc43eb5bff6b2cd09bb5e90aacb39aa6aee2d Mon Sep 17 00:00:00 2001
From: Aymeric Ducroquetz <aymeric.ducroquetz@ovhcloud.com>
Date: Fri, 15 Apr 2022 15:35:01 +0200
Subject: [PATCH] s3api: Improve error message when bucket is not empty

When the versioning is enabled (or suspended), AWS specifies
in the error message that all versions should be deleted.

Change-Id: I3da9469a5cfed031a2cee85e1dfcd78bbe54695a
---
 .../middleware/s3api/controllers/bucket.py    |  8 +++-
 swift/common/middleware/s3api/s3response.py   |  6 +++
 .../common/middleware/s3api/test_bucket.py    | 44 ++++++++++++++++++-
 3 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/swift/common/middleware/s3api/controllers/bucket.py b/swift/common/middleware/s3api/controllers/bucket.py
index bd71bf058c..e9b0fdf54e 100644
--- a/swift/common/middleware/s3api/controllers/bucket.py
+++ b/swift/common/middleware/s3api/controllers/bucket.py
@@ -32,7 +32,8 @@ from swift.common.middleware.s3api.etree import Element, SubElement, \
 from swift.common.middleware.s3api.s3response import \
     HTTPOk, S3NotImplemented, InvalidArgument, \
     MalformedXML, InvalidLocationConstraint, NoSuchBucket, \
-    BucketNotEmpty, InternalError, ServiceUnavailable, NoSuchKey
+    BucketNotEmpty, VersionedBucketNotEmpty, InternalError, \
+    ServiceUnavailable, NoSuchKey
 from swift.common.middleware.s3api.utils import MULTIUPLOAD_SUFFIX
 
 MAX_PUT_BUCKET_BODY_SIZE = 10240
@@ -53,7 +54,10 @@ class BucketController(Controller):
         try:
             resp = req.get_response(self.app, 'HEAD')
             if int(resp.sw_headers['X-Container-Object-Count']) > 0:
-                raise BucketNotEmpty()
+                if resp.sw_headers.get('X-Container-Sysmeta-Versions-Enabled'):
+                    raise VersionedBucketNotEmpty()
+                else:
+                    raise BucketNotEmpty()
             # FIXME: This extra HEAD saves unexpected segment deletion
             # but if a complete multipart upload happen while cleanup
             # segment container below, completed object may be missing its
diff --git a/swift/common/middleware/s3api/s3response.py b/swift/common/middleware/s3api/s3response.py
index ca629e103b..5d971dd113 100644
--- a/swift/common/middleware/s3api/s3response.py
+++ b/swift/common/middleware/s3api/s3response.py
@@ -334,6 +334,12 @@ class BucketNotEmpty(ErrorResponse):
     _msg = 'The bucket you tried to delete is not empty'
 
 
+class VersionedBucketNotEmpty(BucketNotEmpty):
+    _msg = 'The bucket you tried to delete is not empty. ' \
+           'You must delete all versions in the bucket.'
+    _code = 'BucketNotEmpty'
+
+
 class CredentialsNotSupported(ErrorResponse):
     _status = '400 Bad Request'
     _msg = 'This request does not support credentials.'
diff --git a/test/unit/common/middleware/s3api/test_bucket.py b/test/unit/common/middleware/s3api/test_bucket.py
index cba3d903ba..6b6b1dc73e 100644
--- a/test/unit/common/middleware/s3api/test_bucket.py
+++ b/test/unit/common/middleware/s3api/test_bucket.py
@@ -1369,10 +1369,50 @@ class TestS3ApiBucket(S3ApiTestCase):
         self.assertEqual(code, 'InternalError')
 
         # bucket not empty is now validated at s3api
+        self.swift._responses.get(('HEAD', '/v1/AUTH_test/bucket'))
         self.swift.register('HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent,
                             {'X-Container-Object-Count': '1'}, None)
-        code = self._test_method_error('DELETE', '/bucket', swob.HTTPConflict)
-        self.assertEqual(code, 'BucketNotEmpty')
+        req = Request.blank('/bucket',
+                            environ={'REQUEST_METHOD': 'DELETE'},
+                            headers={'Authorization': 'AWS test:tester:hmac',
+                                     'Date': self.get_date_header()})
+        status, _headers, body = self.call_s3api(req)
+        self.assertEqual('409 Conflict', status)
+        self.assertEqual('BucketNotEmpty', self._get_error_code(body))
+        self.assertNotIn('You must delete all versions in the bucket',
+                         self._get_error_message(body))
+
+    @s3acl
+    def test_bucket_DELETE_error_with_enabled_versioning(self):
+        self.swift.register('HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent,
+                            {'X-Container-Object-Count': '1',
+                             'X-Container-Sysmeta-Versions-Enabled': 'True'},
+                            None)
+        req = Request.blank('/bucket',
+                            environ={'REQUEST_METHOD': 'DELETE'},
+                            headers={'Authorization': 'AWS test:tester:hmac',
+                                     'Date': self.get_date_header()})
+        status, _headers, body = self.call_s3api(req)
+        self.assertEqual('409 Conflict', status)
+        self.assertEqual('BucketNotEmpty', self._get_error_code(body))
+        self.assertIn('You must delete all versions in the bucket',
+                      self._get_error_message(body))
+
+    @s3acl
+    def test_bucket_DELETE_error_with_suspended_versioning(self):
+        self.swift.register('HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent,
+                            {'X-Container-Object-Count': '1',
+                             'X-Container-Sysmeta-Versions-Enabled': 'False'},
+                            None)
+        req = Request.blank('/bucket',
+                            environ={'REQUEST_METHOD': 'DELETE'},
+                            headers={'Authorization': 'AWS test:tester:hmac',
+                                     'Date': self.get_date_header()})
+        status, _headers, body = self.call_s3api(req)
+        self.assertEqual('409 Conflict', status)
+        self.assertEqual('BucketNotEmpty', self._get_error_code(body))
+        self.assertIn('You must delete all versions in the bucket',
+                      self._get_error_message(body))
 
     @s3acl
     def test_bucket_DELETE(self):