From e22960fd718c4594bc8a504a12aba3f6a52175bf Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Mon, 20 May 2019 21:54:40 -0700 Subject: [PATCH] s3api: Fix ETag when copying a MU part from another MU Previously, we'd preserve the sysmeta that we wrote down with the original multipart-upload to track its S3-style etag on the new part, causing it to have an ETag like `-`. Later, when the client tried to complete the new multipart-upload, it would send that etag back to the server, which would reject the request because the ETag didn't look like a normal MD5. Now, have s3api include blank values in the copy request to overwrite the source sysmeta, and treat a blank etag override the same as a missing one. Change-Id: Id33a7ab9d0b8f33fede73eae540d6137708e1218 Closes-Bug: #1829959 --- .../s3api/controllers/multi_upload.py | 9 ++ swift/common/middleware/s3api/s3response.py | 2 +- test/functional/s3api/test_multi_upload.py | 95 +++++++++++++------ .../middleware/s3api/test_multi_upload.py | 10 ++ 4 files changed, 86 insertions(+), 30 deletions(-) diff --git a/swift/common/middleware/s3api/controllers/multi_upload.py b/swift/common/middleware/s3api/controllers/multi_upload.py index 729b8602ff..6d7bed7bd0 100644 --- a/swift/common/middleware/s3api/controllers/multi_upload.py +++ b/swift/common/middleware/s3api/controllers/multi_upload.py @@ -175,6 +175,15 @@ class PartController(Controller): req.headers['Range'] = rng del req.headers['X-Amz-Copy-Source-Range'] + if 'X-Amz-Copy-Source' in req.headers: + # Clear some problematic headers that might be on the source + req.headers.update({ + sysmeta_header('object', 'etag'): '', + 'X-Object-Sysmeta-Swift3-Etag': '', # for legacy data + 'X-Object-Sysmeta-Slo-Etag': '', + 'X-Object-Sysmeta-Slo-Size': '', + 'X-Object-Sysmeta-Container-Update-Override-Etag': '', + }) resp = req.get_response(self.app) if 'X-Amz-Copy-Source' in req.headers: diff --git a/swift/common/middleware/s3api/s3response.py b/swift/common/middleware/s3api/s3response.py index b71efa7f18..a34465a3d2 100644 --- a/swift/common/middleware/s3api/s3response.py +++ b/swift/common/middleware/s3api/s3response.py @@ -136,7 +136,7 @@ class S3Response(S3ResponseBase, swob.Response): # Check whether we stored the AWS-style etag on upload override_etag = s3_sysmeta_headers.get( sysmeta_header('object', 'etag')) - if override_etag is not None: + if override_etag not in (None, ''): # Multipart uploads in AWS have ETags like # - headers['etag'] = override_etag diff --git a/test/functional/s3api/test_multi_upload.py b/test/functional/s3api/test_multi_upload.py index 3fed5fb571..33b9e322e3 100644 --- a/test/functional/s3api/test_multi_upload.py +++ b/test/functional/s3api/test_multi_upload.py @@ -281,20 +281,6 @@ class TestS3ApiMultiUpload(S3ApiBase): self.assertEqual(self.min_segment_size, int(p.find('Size').text)) etags.append(p.find('ETag').text) - # Abort Multipart Uploads - # note that uploads[1] has part data while uploads[2] does not - for key, upload_id in uploads[1:]: - query = 'uploadId=%s' % upload_id - status, headers, body = \ - self.conn.make_request('DELETE', bucket, key, query=query) - self.assertEqual(status, 204) - self.assertCommonResponseHeaders(headers) - self.assertTrue('content-type' in headers) - self.assertEqual(headers['content-type'], - 'text/html; charset=UTF-8') - self.assertTrue('content-length' in headers) - self.assertEqual(headers['content-length'], '0') - # Complete Multipart Upload key, upload_id = uploads[0] xml = self._gen_comp_xml(etags) @@ -329,10 +315,61 @@ class TestS3ApiMultiUpload(S3ApiBase): swift_etag = '"%s"' % md5(concatted_etags).hexdigest() # TODO: GET via swift api, check against swift_etag + # Upload Part Copy -- MU as source + key, upload_id = uploads[1] + status, headers, body, resp_etag = \ + self._upload_part_copy(bucket, keys[0], bucket, + key, upload_id, part_num=2) + self.assertEqual(status, 200) + self.assertCommonResponseHeaders(headers) + self.assertIn('content-type', headers) + self.assertEqual(headers['content-type'], 'application/xml') + self.assertIn('content-length', headers) + self.assertEqual(headers['content-length'], str(len(body))) + self.assertNotIn('etag', headers) + elem = fromstring(body, 'CopyPartResult') + + last_modified = elem.find('LastModified').text + self.assertIsNotNone(last_modified) + + exp_content = 'a' * self.min_segment_size + etag = md5(exp_content).hexdigest() + self.assertEqual(resp_etag, etag) + + # Also check that the etag is correct in part listings + query = 'uploadId=%s' % upload_id + status, headers, body = \ + self.conn.make_request('GET', bucket, key, query=query) + self.assertEqual(status, 200) + self.assertCommonResponseHeaders(headers) + self.assertTrue('content-type' in headers) + self.assertEqual(headers['content-type'], 'application/xml') + self.assertTrue('content-length' in headers) + self.assertEqual(headers['content-length'], str(len(body))) + elem = fromstring(body, 'ListPartsResult') + self.assertEqual(len(elem.findall('Part')), 2) + self.assertEqual(elem.findall('Part')[1].find('PartNumber').text, '2') + self.assertEqual(elem.findall('Part')[1].find('ETag').text, + '"%s"' % etag) + + # Abort Multipart Uploads + # note that uploads[1] has part data while uploads[2] does not + for key, upload_id in uploads[1:]: + query = 'uploadId=%s' % upload_id + status, headers, body = \ + self.conn.make_request('DELETE', bucket, key, query=query) + self.assertEqual(status, 204) + self.assertCommonResponseHeaders(headers) + self.assertTrue('content-type' in headers) + self.assertEqual(headers['content-type'], + 'text/html; charset=UTF-8') + self.assertTrue('content-length' in headers) + self.assertEqual(headers['content-length'], '0') + # Check object def check_obj(req_headers, exp_status): status, headers, body = \ - self.conn.make_request('HEAD', bucket, key, req_headers) + self.conn.make_request('HEAD', bucket, keys[0], req_headers) self.assertEqual(status, exp_status) self.assertCommonResponseHeaders(headers) self.assertIn('content-length', headers) @@ -364,20 +401,20 @@ class TestS3ApiMultiUpload(S3ApiBase): self.assertEqual(status, 200) elem = fromstring(body, 'ListBucketResult') - resp_objects = elem.findall('./Contents') - self.assertEqual(len(list(resp_objects)), 1) - for o in resp_objects: - self.assertEqual(o.find('Key').text, key) - self.assertIsNotNone(o.find('LastModified').text) - self.assertRegexpMatches( - o.find('LastModified').text, - r'^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$') - self.assertEqual(o.find('ETag').text, exp_etag) - self.assertEqual(o.find('Size').text, str(exp_size)) - self.assertIsNotNone(o.find('StorageClass').text is not None) - self.assertEqual(o.find('Owner/ID').text, self.conn.user_id) - self.assertEqual(o.find('Owner/DisplayName').text, - self.conn.user_id) + resp_objects = list(elem.findall('./Contents')) + self.assertEqual(len(resp_objects), 1) + o = resp_objects[0] + self.assertEqual(o.find('Key').text, keys[0]) + self.assertIsNotNone(o.find('LastModified').text) + self.assertRegexpMatches( + o.find('LastModified').text, + r'^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$') + self.assertEqual(o.find('ETag').text, exp_etag) + self.assertEqual(o.find('Size').text, str(exp_size)) + self.assertIsNotNone(o.find('StorageClass').text) + self.assertEqual(o.find('Owner/ID').text, self.conn.user_id) + self.assertEqual(o.find('Owner/DisplayName').text, + self.conn.user_id) def test_initiate_multi_upload_error(self): bucket = 'bucket' diff --git a/test/unit/common/middleware/s3api/test_multi_upload.py b/test/unit/common/middleware/s3api/test_multi_upload.py index cff18c2759..a34abf9fad 100644 --- a/test/unit/common/middleware/s3api/test_multi_upload.py +++ b/test/unit/common/middleware/s3api/test_multi_upload.py @@ -1651,6 +1651,16 @@ class TestS3ApiMultiUpload(S3ApiTestCase): _, _, headers = self.swift.calls_with_headers[-1] self.assertEqual(headers['X-Copy-From'], '/src_bucket/src_obj') self.assertEqual(headers['Content-Length'], '0') + # Some headers *need* to get cleared in case we're copying from + # another multipart upload + for header in ( + 'X-Object-Sysmeta-S3api-Etag', + 'X-Object-Sysmeta-Slo-Etag', + 'X-Object-Sysmeta-Slo-Size', + 'X-Object-Sysmeta-Container-Update-Override-Etag', + 'X-Object-Sysmeta-Swift3-Etag', + ): + self.assertEqual(headers[header], '') @s3acl(s3acl_only=True) def test_upload_part_copy_acl_with_owner_permission(self):