From 63defd1430492c22ff80ab54b10ef61e21066f8b Mon Sep 17 00:00:00 2001 From: Alistair Coles Date: Thu, 16 Nov 2023 12:47:05 +0000 Subject: [PATCH] s3api: return 503 if mpu complete gets 409 deleting marker During an MPU complete, the s3api first PUTs an SLO manifest and then DELETEs the upload marker in +segments. If the proxy's clock is slow relative to the proxy that created the upload marker, the DELETE will fail with a 409. Previously the 409 would be returned to the client, which for some clients (e.g. aws cli) does not trigger a retry. Both the manifest and upload marker would remain, which would cause the upload to continue to be listed as if still "in progress". Worse, since the complete failed, the client might delete the segments, leaving the manifest in place but with no segments. This patch adds a "look-before-you-leap" pre-check on the upload marker timestamp. If the marker is found to be in the future then neither the manifest PUT nor the marker DELETE are attempted, and the client receives a 503 response. If the pre-check passes but somehow the marker DELETE still fails with a 409, the client will now receive a 503 that will hopefully trigger a retry. Closes-Bug: #2045046 Change-Id: Ie2e0cb75425e00cff533014af6b6fafad89bff94 --- .../s3api/controllers/multi_upload.py | 32 ++++++++-- swift/common/middleware/s3api/s3request.py | 6 +- .../middleware/s3api/test_multi_upload.py | 60 ++++++++++++++++++- 3 files changed, 89 insertions(+), 9 deletions(-) diff --git a/swift/common/middleware/s3api/controllers/multi_upload.py b/swift/common/middleware/s3api/controllers/multi_upload.py index 9e8c2d4127..6bfa6eed51 100644 --- a/swift/common/middleware/s3api/controllers/multi_upload.py +++ b/swift/common/middleware/s3api/controllers/multi_upload.py @@ -68,8 +68,9 @@ import time import six from swift.common import constraints -from swift.common.swob import Range, bytes_to_wsgi, normalize_etag, wsgi_to_str -from swift.common.utils import json, public, reiterate, md5 +from swift.common.swob import Range, bytes_to_wsgi, normalize_etag, \ + wsgi_to_str +from swift.common.utils import json, public, reiterate, md5, Timestamp from swift.common.db import utf8encode from swift.common.request_helpers import get_container_update_override_key, \ get_param @@ -82,7 +83,7 @@ from swift.common.middleware.s3api.s3response import InvalidArgument, \ ErrorResponse, MalformedXML, BadDigest, KeyTooLongError, \ InvalidPart, BucketAlreadyExists, EntityTooSmall, InvalidPartOrder, \ InvalidRequest, HTTPOk, HTTPNoContent, NoSuchKey, NoSuchUpload, \ - NoSuchBucket, BucketAlreadyOwnedByYou + NoSuchBucket, BucketAlreadyOwnedByYou, ServiceUnavailable from swift.common.middleware.s3api.utils import unique_id, \ MULTIUPLOAD_SUFFIX, S3Timestamp, sysmeta_header from swift.common.middleware.s3api.etree import Element, SubElement, \ @@ -96,6 +97,17 @@ MAX_COMPLETE_UPLOAD_BODY_SIZE = 2048 * 1024 def _get_upload_info(req, app, upload_id): + """ + Make a HEAD request for existing upload object metadata. Tries the upload + marker first, and then falls back to the manifest object. + + :param req: an S3Request object. + :param app: the wsgi app. + :param upload_id: the upload id. + :returns: a tuple of (S3Response, boolean) where the boolean is True if the + response is from the upload marker and False otherwise. + :raises: NoSuchUpload if neither the marker nor the manifest were found. + """ container = req.container_name + MULTIUPLOAD_SUFFIX obj = '%s/%s' % (req.object_name, upload_id) @@ -106,7 +118,8 @@ def _get_upload_info(req, app, upload_id): # it off for now... copy_source = req.headers.pop('X-Amz-Copy-Source', None) try: - return req.get_response(app, 'HEAD', container=container, obj=obj) + resp = req.get_response(app, 'HEAD', container=container, obj=obj) + return resp, True except NoSuchKey: # ensure consistent path and policy are logged despite manifest HEAD upload_marker_path = req.environ.get('s3api.backend_path') @@ -115,7 +128,7 @@ def _get_upload_info(req, app, upload_id): resp = req.get_response(app, 'HEAD') if resp.sysmeta_headers.get(sysmeta_header( 'object', 'upload-id')) == upload_id: - return resp + return resp, False except NoSuchKey: pass finally: @@ -655,7 +668,14 @@ class UploadController(Controller): Handles Complete Multipart Upload. """ upload_id = get_param(req, 'uploadId') - resp = _get_upload_info(req, self.app, upload_id) + resp, is_marker = _get_upload_info(req, self.app, upload_id) + if (is_marker and + resp.sw_headers.get('X-Backend-Timestamp') >= Timestamp.now()): + # Somehow the marker was created in the future w.r.t. this thread's + # clock. The manifest PUT may succeed but the subsequent marker + # DELETE will fail, so don't attempt either. + raise ServiceUnavailable + headers = {'Accept': 'application/json', sysmeta_header('object', 'upload-id'): upload_id} for key, val in resp.headers.items(): diff --git a/swift/common/middleware/s3api/s3request.py b/swift/common/middleware/s3api/s3request.py index 33f507628c..da5220046d 100644 --- a/swift/common/middleware/s3api/s3request.py +++ b/swift/common/middleware/s3api/s3request.py @@ -1432,8 +1432,10 @@ class S3Request(swob.Request): raise SlowDown(status='429 Slow Down') raise SlowDown() if resp.status_int == HTTP_CONFLICT: - # TODO: validate that this actually came up out of SLO - raise BrokenMPU() + if self.method == 'GET': + raise BrokenMPU() + else: + raise ServiceUnavailable() raise InternalError('unexpected status code %d' % status) diff --git a/test/unit/common/middleware/s3api/test_multi_upload.py b/test/unit/common/middleware/s3api/test_multi_upload.py index 8337d7cf46..58e4e46c6d 100644 --- a/test/unit/common/middleware/s3api/test_multi_upload.py +++ b/test/unit/common/middleware/s3api/test_multi_upload.py @@ -24,7 +24,7 @@ from six.moves.urllib.parse import parse_qs, quote, quote_plus from swift.common import swob from swift.common.swob import Request -from swift.common.utils import json, md5 +from swift.common.utils import json, md5, Timestamp from test.unit import FakeMemcache, patch_policies from test.unit.common.middleware.s3api import S3ApiTestCase @@ -1649,6 +1649,64 @@ class TestS3ApiMultiUpload(S3ApiTestCase): self.assertEqual(headers.get('X-Object-Meta-Foo'), 'bar') self.assertEqual(headers.get('Content-Type'), 'baz/quux') + def _do_test_object_multipart_upload_complete_marker_in_future( + self, marker_timestamp, now_timestamp): + # verify that clock skew is detected before manifest is created and + # results in a 503 + segment_bucket = '/v1/AUTH_test/bucket+segments' + self.swift.register('HEAD', segment_bucket + '/object/X', + swob.HTTPOk, + {'x-object-meta-foo': 'bar', + 'content-type': 'application/directory', + 'x-object-sysmeta-s3api-has-content-type': 'yes', + 'x-object-sysmeta-s3api-content-type': + 'baz/quux', + 'X-Backend-Timestamp': marker_timestamp.internal}, + None) + req = Request.blank('/bucket/object?uploadId=X', + environ={'REQUEST_METHOD': 'POST'}, + headers={'Authorization': 'AWS test:tester:hmac', + 'Date': self.get_date_header(skew=100), }, + body=XML) + + # marker created in the future + with patch('swift.common.middleware.s3api.controllers.multi_upload.' + 'Timestamp.now', return_value=now_timestamp): + status, headers, body = self.call_s3api(req) + self.assertEqual(status.split()[0], '503') + self.assertEqual('ServiceUnavailable', self._get_error_code(body)) + self.assertEqual(self.swift.calls, [ + ('HEAD', '/v1/AUTH_test'), + ('HEAD', '/v1/AUTH_test/bucket'), + ('HEAD', '/v1/AUTH_test/bucket+segments/object/X')]) + + def test_object_multipart_upload_complete_marker_ts_now(self): + marker_timestamp = now_timestamp = Timestamp.now() + self._do_test_object_multipart_upload_complete_marker_in_future( + marker_timestamp, now_timestamp) + + def test_object_multipart_upload_complete_marker_ts_in_future(self): + marker_timestamp = Timestamp.now() + now_timestamp = Timestamp(float(marker_timestamp) - 1) + self._do_test_object_multipart_upload_complete_marker_in_future( + marker_timestamp, now_timestamp) + + def test_object_multipart_upload_complete_409_on_marker_delete(self): + # verify that clock skew preventing an upload marker DELETE results in + # a 503 (this would be unexpected because there's a check for clock + # skew before the manifest PUT and marker DELETE) + segment_bucket = '/v1/AUTH_test/bucket+segments' + self.swift.register('DELETE', segment_bucket + '/object/X', + swob.HTTPConflict, {}, None) + req = Request.blank('/bucket/object?uploadId=X', + environ={'REQUEST_METHOD': 'POST'}, + headers={'Authorization': 'AWS test:tester:hmac', + 'Date': self.get_date_header(), }, + body=XML) + status, headers, body = self.call_s3api(req) + self.assertEqual(status.split()[0], '503') + self.assertEqual('ServiceUnavailable', self._get_error_code(body)) + def test_object_multipart_upload_complete_old_content_type(self): self.swift.register_unconditionally( 'HEAD', '/v1/AUTH_test/bucket+segments/object/X',