From edd5eb29d7d6041ae0b16b78cbcb89f9dd95309f Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Tue, 13 May 2025 13:00:26 -0700 Subject: [PATCH] s3api: Allow PUT with `if-none-match: *` Swift already supports that much, at least. AWS used to not support any conditional PUTs, but that's changed somewhat recently; see - https://aws.amazon.com/about-aws/whats-new/2024/08/amazon-s3-conditional-writes/ - https://aws.amazon.com/about-aws/whats-new/2024/11/amazon-s3-functionality-conditional-writes/ Drive-By: Fix retry of a CompleteMultipartUpload with changed parts; it should 404 rather than succeed in writing the new manifest. Change-Id: I2e57dacb342b5758f16b502bb91372a2443d0182 --- .../conf/ceph-known-failures-keystone.yaml | 2 - .../conf/ceph-known-failures-tempauth.yaml | 2 - .../s3api/controllers/multi_upload.py | 30 +++- swift/common/middleware/s3api/s3request.py | 7 +- test/functional/s3api/test_multi_upload.py | 60 ++++++++ test/functional/s3api/test_object.py | 14 +- test/s3api/test_conditional_writes.py | 142 ++++++++++++++++++ .../middleware/s3api/test_multi_upload.py | 22 +-- 8 files changed, 246 insertions(+), 33 deletions(-) create mode 100644 test/s3api/test_conditional_writes.py diff --git a/doc/s3api/conf/ceph-known-failures-keystone.yaml b/doc/s3api/conf/ceph-known-failures-keystone.yaml index 7c5ce7f184..69f0e76ffb 100644 --- a/doc/s3api/conf/ceph-known-failures-keystone.yaml +++ b/doc/s3api/conf/ceph-known-failures-keystone.yaml @@ -87,8 +87,6 @@ ceph_s3: s3tests.functional.test_s3.test_put_object_ifmatch_overwrite_existed_good: {status: KNOWN} s3tests.functional.test_s3.test_put_object_ifnonmatch_failed: {status: KNOWN} s3tests.functional.test_s3.test_put_object_ifnonmatch_good: {status: KNOWN} - s3tests.functional.test_s3.test_put_object_ifnonmatch_nonexisted_good: {status: KNOWN} - s3tests.functional.test_s3.test_put_object_ifnonmatch_overwrite_existed_failed: {status: KNOWN} s3tests.functional.test_s3.test_set_cors: {status: KNOWN} s3tests.functional.test_s3.test_stress_bucket_acls_changes: {status: KNOWN} s3tests.functional.test_s3.test_versioned_concurrent_object_create_concurrent_remove: {status: KNOWN} diff --git a/doc/s3api/conf/ceph-known-failures-tempauth.yaml b/doc/s3api/conf/ceph-known-failures-tempauth.yaml index 5a8459893c..86dc1dbdb5 100644 --- a/doc/s3api/conf/ceph-known-failures-tempauth.yaml +++ b/doc/s3api/conf/ceph-known-failures-tempauth.yaml @@ -234,8 +234,6 @@ ceph_s3: s3tests_boto3.functional.test_s3.test_put_object_ifmatch_overwrite_existed_good: {status: KNOWN} s3tests_boto3.functional.test_s3.test_put_object_ifnonmatch_failed: {status: KNOWN} s3tests_boto3.functional.test_s3.test_put_object_ifnonmatch_good: {status: KNOWN} - s3tests_boto3.functional.test_s3.test_put_object_ifnonmatch_nonexisted_good: {status: KNOWN} - s3tests_boto3.functional.test_s3.test_put_object_ifnonmatch_overwrite_existed_failed: {status: KNOWN} s3tests_boto3.functional.test_s3.test_put_tags_acl_public: {status: KNOWN} s3tests_boto3.functional.test_s3.test_set_cors: {status: KNOWN} s3tests_boto3.functional.test_s3.test_set_tagging: {status: KNOWN} diff --git a/swift/common/middleware/s3api/controllers/multi_upload.py b/swift/common/middleware/s3api/controllers/multi_upload.py index 07e71a12af..565c625351 100644 --- a/swift/common/middleware/s3api/controllers/multi_upload.py +++ b/swift/common/middleware/s3api/controllers/multi_upload.py @@ -80,7 +80,8 @@ from swift.common.middleware.s3api.s3response import InvalidArgument, \ ErrorResponse, MalformedXML, BadDigest, KeyTooLongError, \ InvalidPart, BucketAlreadyExists, EntityTooSmall, InvalidPartOrder, \ InvalidRequest, HTTPOk, HTTPNoContent, NoSuchKey, NoSuchUpload, \ - NoSuchBucket, BucketAlreadyOwnedByYou, ServiceUnavailable + NoSuchBucket, BucketAlreadyOwnedByYou, ServiceUnavailable, \ + PreconditionFailed, S3NotImplemented from swift.common.middleware.s3api.utils import unique_id, \ MULTIUPLOAD_SUFFIX, S3Timestamp, sysmeta_header from swift.common.middleware.s3api.etree import Element, SubElement, \ @@ -646,6 +647,14 @@ class UploadController(Controller): Handles Complete Multipart Upload. """ upload_id = get_param(req, 'uploadId') + # Check for conditional requests before getting upload info so the + # headers can't bleed into the HEAD + if req.headers.get('If-None-Match', '*') != '*' or any( + h in req.headers for h in ( + 'If-Match', 'If-Modified-Since', 'If-Unmodified-Since')): + raise S3NotImplemented( + 'Conditional uploads are not supported.') + resp, is_marker = _get_upload_info(req, self.app, upload_id) if (is_marker and resp.sw_headers.get('X-Backend-Timestamp') >= Timestamp.now()): @@ -735,13 +744,17 @@ class UploadController(Controller): s3_etag = '%s-%d' % (s3_etag_hasher.hexdigest(), len(manifest)) s3_etag_header = sysmeta_header('object', 'etag') - if resp.sysmeta_headers.get(s3_etag_header) == s3_etag: - # This header should only already be present if the upload marker - # has been cleaned up and the current target uses the same - # upload-id; assuming the segments to use haven't changed, the work - # is already done + # This header should only already be present if the upload marker + # has been cleaned up and the current target uses the same upload-id + already_uploaded_s3_etag = resp.sysmeta_headers.get(s3_etag_header) + if already_uploaded_s3_etag == s3_etag: + # If the segments to use haven't changed, the work is already done return HTTPOk(body=_make_complete_body(req, s3_etag, False), content_type='application/xml') + elif already_uploaded_s3_etag: + # If the header's present but *doesn't* match, upload-id is + # no longer valid + raise NoSuchUpload(upload_id=upload_id) headers[s3_etag_header] = s3_etag # Leave base header value blank; SLO will populate c_etag = '; s3_etag=%s' % s3_etag @@ -796,7 +809,10 @@ class UploadController(Controller): continue body.append(chunk) body = json.loads(b''.join(body)) - if body['Response Status'] != '201 Created': + if body['Response Status'] == \ + '412 Precondition Failed': + raise PreconditionFailed + elif body['Response Status'] != '201 Created': for seg, err in body['Errors']: if err == too_small_message: raise EntityTooSmall() diff --git a/swift/common/middleware/s3api/s3request.py b/swift/common/middleware/s3api/s3request.py index 2673e65b4e..1061c2282b 100644 --- a/swift/common/middleware/s3api/s3request.py +++ b/swift/common/middleware/s3api/s3request.py @@ -918,9 +918,10 @@ class S3Request(swob.Request): raise InvalidArgument('Content-Length', self.environ['CONTENT_LENGTH']) - if self.method == 'PUT' and any(h in self.headers for h in ( - 'If-Match', 'If-None-Match', - 'If-Modified-Since', 'If-Unmodified-Since')): + if self.method == 'PUT' and ( + any(h in self.headers for h in ( + 'If-Match', 'If-Modified-Since', 'If-Unmodified-Since')) + or self.headers.get('If-None-Match', '*') != '*'): raise S3NotImplemented( 'Conditional object PUTs are not supported.') diff --git a/test/functional/s3api/test_multi_upload.py b/test/functional/s3api/test_multi_upload.py index 74cbdff0fb..3aa205bbb0 100644 --- a/test/functional/s3api/test_multi_upload.py +++ b/test/functional/s3api/test_multi_upload.py @@ -729,6 +729,66 @@ class TestS3ApiMultiUpload(S3ApiBase): query=query) self.assertEqual(get_error_code(body), 'InvalidPart') + def test_complete_multi_upload_conditional(self): + bucket = 'bucket' + key = 'obj' + self.conn.make_request('PUT', bucket) + query = 'uploads' + status, headers, body = \ + self.conn.make_request('POST', bucket, key, query=query) + elem = fromstring(body, 'InitiateMultipartUploadResult') + upload_id = elem.find('UploadId').text + + query = 'partNumber=1&uploadId=%s' % upload_id + status, headers, body = \ + self.conn.make_request('PUT', bucket, key, query=query) + part_etag = headers['etag'] + xml = self._gen_comp_xml([part_etag]) + + for headers in [ + {'If-Match': part_etag}, + {'If-Match': '*'}, + {'If-None-Match': part_etag}, + {'If-Modified-Since': 'Wed, 21 Oct 2015 07:28:00 GMT'}, + {'If-Unmodified-Since': 'Wed, 21 Oct 2015 07:28:00 GMT'}, + ]: + with self.subTest(headers=headers): + query = 'uploadId=%s' % upload_id + status, _, body = self.conn.make_request( + 'POST', bucket, key, body=xml, + query=query, headers=headers) + self.assertEqual(status, 501) + self.assertEqual(get_error_code(body), 'NotImplemented') + + # Can do basic existence checks, though + headers = {'If-None-Match': '*'} + query = 'uploadId=%s' % upload_id + status, _, body = self.conn.make_request( + 'POST', bucket, key, body=xml, + query=query, headers=headers) + self.assertEqual(status, 200) + + # And it'll prevent overwrites + query = 'uploads' + status, headers, body = \ + self.conn.make_request('POST', bucket, key, query=query) + elem = fromstring(body, 'InitiateMultipartUploadResult') + upload_id = elem.find('UploadId').text + + query = 'partNumber=1&uploadId=%s' % upload_id + status, headers, body = \ + self.conn.make_request('PUT', bucket, key, query=query) + part_etag = headers['etag'] + xml = self._gen_comp_xml([part_etag]) + + headers = {'If-None-Match': '*'} + query = 'uploadId=%s' % upload_id + status, _, body = self.conn.make_request( + 'POST', bucket, key, body=xml, + query=query, headers=headers) + self.assertEqual(status, 412) + self.assertEqual(get_error_code(body), 'PreconditionFailed') + def test_complete_upload_min_segment_size(self): bucket = 'bucket' key = 'obj' diff --git a/test/functional/s3api/test_object.py b/test/functional/s3api/test_object.py index eefe4888dc..eb2b3b13d3 100644 --- a/test/functional/s3api/test_object.py +++ b/test/functional/s3api/test_object.py @@ -310,7 +310,7 @@ class TestS3ApiObject(S3ApiBase): def test_put_object_conditional_requests(self): obj = 'object' content = b'abcdefghij' - headers = {'If-None-Match': '*'} + headers = {'If-None-Match': 'asdf'} status, headers, body = \ self.conn.make_request('PUT', self.bucket, obj, headers, content) self.assertEqual(status, 501) @@ -335,6 +335,18 @@ class TestS3ApiObject(S3ApiBase): self.conn.make_request('HEAD', self.bucket, obj, {}, '') self.assertEqual(status, 404) + # But this will + headers = {'If-None-Match': '*'} + status, headers, body = \ + self.conn.make_request('PUT', self.bucket, obj, headers, content) + self.assertEqual(status, 200) + + # And the if-none-match prevents overwrites + headers = {'If-None-Match': '*'} + status, headers, body = \ + self.conn.make_request('PUT', self.bucket, obj, headers, content) + self.assertEqual(status, 412) + def test_put_object_expect(self): obj = 'object' content = b'abcdefghij' diff --git a/test/s3api/test_conditional_writes.py b/test/s3api/test_conditional_writes.py new file mode 100644 index 0000000000..c7986b3156 --- /dev/null +++ b/test/s3api/test_conditional_writes.py @@ -0,0 +1,142 @@ +# Copyright (c) 2025 Nvidia +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from test.s3api import BaseS3TestCaseWithBucket, status_from_error, \ + code_from_error +from botocore.exceptions import ClientError + + +class TestConditionalWrites(BaseS3TestCaseWithBucket): + def test_if_none_match_star_simple_put(self): + client = self.get_s3_client(1) + key_name = self.create_name('if-none-match-simple') + # Can create new object fine + resp = client.put_object( + Bucket=self.bucket_name, + Key=key_name, + IfNoneMatch='*', + Body=b'', + ) + self.assertEqual(200, resp['ResponseMetadata']['HTTPStatusCode']) + # But overwrite is blocked + with self.assertRaises(ClientError) as caught: + client.put_object( + Bucket=self.bucket_name, + Key=key_name, + IfNoneMatch='*', + Body=b'', + ) + self.assertEqual(412, status_from_error(caught.exception)) + self.assertEqual('PreconditionFailed', + code_from_error(caught.exception)) + + def test_if_none_match_star_mpu(self): + client = self.get_s3_client(1) + key_name = self.create_name('if-none-match-mpu') + + create_mpu_resp = client.create_multipart_upload( + Bucket=self.bucket_name, Key=key_name) + self.assertEqual(200, create_mpu_resp[ + 'ResponseMetadata']['HTTPStatusCode']) + upload_id = create_mpu_resp['UploadId'] + parts = [] + for part_num in range(1, 4): + part_resp = client.upload_part( + Body=b'x' * 5 * 1024 * 1024, + Bucket=self.bucket_name, Key=key_name, + PartNumber=part_num, UploadId=upload_id) + self.assertEqual(200, part_resp[ + 'ResponseMetadata']['HTTPStatusCode']) + parts.append({ + 'ETag': part_resp['ETag'], + 'PartNumber': part_num, + }) + + # Nothing there, so complete succeeds + complete_mpu_resp = client.complete_multipart_upload( + Bucket=self.bucket_name, + Key=key_name, + MultipartUpload={'Parts': parts[:2]}, + UploadId=upload_id, + IfNoneMatch='*', + ) + self.assertEqual(200, complete_mpu_resp[ + 'ResponseMetadata']['HTTPStatusCode']) + + # Retrying with more parts fails + with self.assertRaises(ClientError) as caught: + client.complete_multipart_upload( + Bucket=self.bucket_name, + Key=key_name, + MultipartUpload={'Parts': parts}, + UploadId=upload_id, + IfNoneMatch='*', + ) + self.assertEqual(404, status_from_error(caught.exception)) + self.assertEqual('NoSuchUpload', + code_from_error(caught.exception)) + + # Ditto fewer + with self.assertRaises(ClientError) as caught: + client.complete_multipart_upload( + Bucket=self.bucket_name, + Key=key_name, + MultipartUpload={'Parts': parts[:1]}, + UploadId=upload_id, + IfNoneMatch='*', + ) + self.assertEqual(404, status_from_error(caught.exception)) + self.assertEqual('NoSuchUpload', + code_from_error(caught.exception)) + + # Can retry with all the same parts and 200 though + complete_mpu_resp = client.complete_multipart_upload( + Bucket=self.bucket_name, + Key=key_name, + MultipartUpload={'Parts': parts[:2]}, + UploadId=upload_id, + IfNoneMatch='*', + ) + self.assertEqual(200, complete_mpu_resp[ + 'ResponseMetadata']['HTTPStatusCode']) + + # Can still start a new upload + create_mpu_resp = client.create_multipart_upload( + Bucket=self.bucket_name, Key=key_name) + self.assertEqual(200, create_mpu_resp[ + 'ResponseMetadata']['HTTPStatusCode']) + upload_id = create_mpu_resp['UploadId'] + # And upload parts + part_resp = client.upload_part( + Body=b'', Bucket=self.bucket_name, Key=key_name, + PartNumber=1, UploadId=upload_id) + self.assertEqual(200, part_resp[ + 'ResponseMetadata']['HTTPStatusCode']) + parts = [{ + 'ETag': part_resp['ETag'], + 'PartNumber': 1, + }] + # But completion will be blocked + with self.assertRaises(ClientError) as caught: + client.complete_multipart_upload( + Bucket=self.bucket_name, + Key=key_name, + MultipartUpload={'Parts': parts}, + UploadId=upload_id, + IfNoneMatch='*', + ) + self.assertEqual(412, status_from_error(caught.exception)) + self.assertEqual('PreconditionFailed', + code_from_error(caught.exception)) diff --git a/test/unit/common/middleware/s3api/test_multi_upload.py b/test/unit/common/middleware/s3api/test_multi_upload.py index 422f742544..0107238324 100644 --- a/test/unit/common/middleware/s3api/test_multi_upload.py +++ b/test/unit/common/middleware/s3api/test_multi_upload.py @@ -1589,10 +1589,9 @@ class TestS3ApiMultiUpload(BaseS3ApiMultiUpload, S3ApiTestCase): 'Content-MD5': content_md5, }, body=XML) status, headers, body = self.call_s3api(req) - elem = fromstring(body, 'CompleteMultipartUploadResult') - self.assertNotIn('Etag', headers) - self.assertEqual(elem.find('ETag').text, S3_ETAG) - self.assertEqual(status.split()[0], '200') + elem = fromstring(body, 'Error') + self.assertEqual(elem.find('Code').text, 'NoSuchUpload') + self.assertEqual(status.split()[0], '404') self.assertEqual(self.swift.calls, [ # Bucket exists @@ -1602,24 +1601,11 @@ class TestS3ApiMultiUpload(BaseS3ApiMultiUpload, S3ApiTestCase): ('HEAD', '/v1/AUTH_test/bucket+segments/object/X'), # But the object does, and with the same upload ID ('HEAD', '/v1/AUTH_test/bucket/object'), - # Create the SLO - ('PUT', '/v1/AUTH_test/bucket/object' - '?heartbeat=on&multipart-manifest=put'), - # Retry deleting the marker for the sake of completeness - ('DELETE', '/v1/AUTH_test/bucket+segments/object/X') + # And then we bail ]) self.assertEqual(req.environ['swift.backend_path'], '/v1/AUTH_test/bucket+segments/object/X') - _, _, headers = self.swift.calls_with_headers[-2] - self.assertEqual(headers.get('X-Object-Meta-Foo'), 'bar') - self.assertEqual(headers.get('Content-Type'), 'baz/quux') - # SLO will provide a base value - override_etag = '; s3_etag=%s' % S3_ETAG.strip('"') - h = 'X-Object-Sysmeta-Container-Update-Override-Etag' - self.assertEqual(headers.get(h), override_etag) - self.assertEqual(headers.get('X-Object-Sysmeta-S3Api-Upload-Id'), 'X') - def test_object_multipart_upload_retry_complete_upload_id_mismatch(self): content_md5 = base64.b64encode(md5( XML.encode('ascii'), usedforsecurity=False).digest())