From 6b9806e0e8cbec60c0a3ece0bd516e0502827515 Mon Sep 17 00:00:00 2001 From: Peter Portante Date: Tue, 2 Jul 2013 11:48:19 -0400 Subject: [PATCH] Fix handling of DELETE obj reqs with old timestamp The DELETE object REST API was creating tombstone files with old timestamps, potentially filling up the disk, as well as sending container updates. Here we now make DELETEs with a request timestamp return a 409 (HTTP Conflict) if a data file exists with a newer timestamp, only creating tombstones if they have a newer timestamp. The key fix is to actually read the timestamp metadata from an existing tombstone file (thanks to Pete Zaitcev for catching this), and then only create tombstone files with newer timestamps. We also prevent PUT and POST operations using old timestamps as well. Change-Id: I631957029d17c6578bca5779367df5144ba01fc9 Signed-off-by: Peter Portante --- swift/obj/diskfile.py | 18 ++-- swift/obj/server.py | 35 +++--- test/unit/obj/test_server.py | 200 +++++++++++++++++++++++++++++++++-- 3 files changed, 222 insertions(+), 31 deletions(-) diff --git a/swift/obj/diskfile.py b/swift/obj/diskfile.py index 9a221ebcf5..97845c158d 100644 --- a/swift/obj/diskfile.py +++ b/swift/obj/diskfile.py @@ -366,7 +366,6 @@ class DiskFile(object): self.logger = logger self.disallowed_metadata_keys = disallowed_metadata_keys or [] self.metadata = {} - self.meta_file = None self.data_file = None self.fp = None self.iter_etag = None @@ -379,13 +378,16 @@ class DiskFile(object): if not exists(self.datadir): return files = sorted(os.listdir(self.datadir), reverse=True) + meta_file = None for afile in files: if afile.endswith('.ts'): - self.data_file = self.meta_file = None - self.metadata = {'deleted': True} - return - if afile.endswith('.meta') and not self.meta_file: - self.meta_file = join(self.datadir, afile) + self.data_file = None + with open(join(self.datadir, afile)) as mfp: + self.metadata = read_metadata(mfp) + self.metadata['deleted'] = True + break + if afile.endswith('.meta') and not meta_file: + meta_file = join(self.datadir, afile) if afile.endswith('.data') and not self.data_file: self.data_file = join(self.datadir, afile) break @@ -395,8 +397,8 @@ class DiskFile(object): self.metadata = read_metadata(self.fp) if not keep_data_fp: self.close(verify_file=False) - if self.meta_file: - with open(self.meta_file) as mfp: + if meta_file: + with open(meta_file) as mfp: for key in self.metadata.keys(): if key.lower() not in self.disallowed_metadata_keys: del self.metadata[key] diff --git a/swift/obj/server.py b/swift/obj/server.py index 1c5dc559d8..86637bae0e 100644 --- a/swift/obj/server.py +++ b/swift/obj/server.py @@ -43,7 +43,8 @@ from swift.common.swob import HTTPAccepted, HTTPBadRequest, HTTPCreated, \ HTTPInternalServerError, HTTPNoContent, HTTPNotFound, HTTPNotModified, \ HTTPPreconditionFailed, HTTPRequestTimeout, HTTPUnprocessableEntity, \ HTTPClientDisconnect, HTTPMethodNotAllowed, Request, Response, UTC, \ - HTTPInsufficientStorage, HTTPForbidden, HTTPException, HeaderKeyDict + HTTPInsufficientStorage, HTTPForbidden, HTTPException, HeaderKeyDict, \ + HTTPConflict from swift.obj.diskfile import DiskFile, get_hashes @@ -317,6 +318,9 @@ class ObjectController(object): except (DiskFileError, DiskFileNotExist): disk_file.quarantine() return HTTPNotFound(request=request) + orig_timestamp = disk_file.metadata.get('X-Timestamp', '0') + if orig_timestamp >= request.headers['x-timestamp']: + return HTTPConflict(request=request) metadata = {'X-Timestamp': request.headers['x-timestamp']} metadata.update(val for val in request.headers.iteritems() if val[0].startswith('X-Object-Meta-')) @@ -364,6 +368,8 @@ class ObjectController(object): return HTTPInsufficientStorage(drive=device, request=request) old_delete_at = int(disk_file.metadata.get('X-Delete-At') or 0) orig_timestamp = disk_file.metadata.get('X-Timestamp') + if orig_timestamp and orig_timestamp >= request.headers['x-timestamp']: + return HTTPConflict(request=request) upload_expiration = time.time() + self.max_upload_time etag = md5() elapsed_time = 0 @@ -563,25 +569,26 @@ class ObjectController(object): return HTTPPreconditionFailed( request=request, body='X-If-Delete-At and X-Delete-At do not match') - orig_timestamp = disk_file.metadata.get('X-Timestamp') - if disk_file.is_deleted() or disk_file.is_expired(): - response_class = HTTPNotFound - else: - response_class = HTTPNoContent - metadata = { - 'X-Timestamp': request.headers['X-Timestamp'], 'deleted': True, - } old_delete_at = int(disk_file.metadata.get('X-Delete-At') or 0) if old_delete_at: self.delete_at_update('DELETE', old_delete_at, account, container, obj, request, device) - disk_file.put_metadata(metadata, tombstone=True) - disk_file.unlinkold(metadata['X-Timestamp']) - if not orig_timestamp or \ - orig_timestamp < request.headers['x-timestamp']: + orig_timestamp = disk_file.metadata.get('X-Timestamp', 0) + req_timestamp = request.headers['X-Timestamp'] + if disk_file.is_deleted() or disk_file.is_expired(): + response_class = HTTPNotFound + else: + if orig_timestamp < req_timestamp: + response_class = HTTPNoContent + else: + response_class = HTTPConflict + if orig_timestamp < req_timestamp: + disk_file.put_metadata({'X-Timestamp': req_timestamp}, + tombstone=True) + disk_file.unlinkold(req_timestamp) self.container_update( 'DELETE', account, container, obj, request, - HeaderKeyDict({'x-timestamp': metadata['X-Timestamp']}), + HeaderKeyDict({'x-timestamp': req_timestamp}), device) resp = response_class(request=request) return resp diff --git a/test/unit/obj/test_server.py b/test/unit/obj/test_server.py index 4e0728eb22..661f84ed6d 100755 --- a/test/unit/obj/test_server.py +++ b/test/unit/obj/test_server.py @@ -223,6 +223,41 @@ class TestObjectController(unittest.TestCase): "X-Object-Meta-3" in resp.headers) self.assertEquals(resp.headers['Content-Type'], 'application/x-test') + def test_POST_old_timestamp(self): + ts = time() + timestamp = normalize_timestamp(ts) + req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Timestamp': timestamp, + 'Content-Type': 'application/x-test', + 'X-Object-Meta-1': 'One', + 'X-Object-Meta-Two': 'Two'}) + req.body = 'VERIFY' + resp = self.object_controller.PUT(req) + self.assertEquals(resp.status_int, 201) + + # Same timestamp should result in 409 + req = Request.blank('/sda1/p/a/c/o', + environ={'REQUEST_METHOD': 'POST'}, + headers={'X-Timestamp': timestamp, + 'X-Object-Meta-3': 'Three', + 'X-Object-Meta-4': 'Four', + 'Content-Encoding': 'gzip', + 'Content-Type': 'application/x-test'}) + resp = self.object_controller.POST(req) + self.assertEquals(resp.status_int, 409) + + # Earlier timestamp should result in 409 + timestamp = normalize_timestamp(ts - 1) + req = Request.blank('/sda1/p/a/c/o', + environ={'REQUEST_METHOD': 'POST'}, + headers={'X-Timestamp': timestamp, + 'X-Object-Meta-5': 'Five', + 'X-Object-Meta-6': 'Six', + 'Content-Encoding': 'gzip', + 'Content-Type': 'application/x-test'}) + resp = self.object_controller.POST(req) + self.assertEquals(resp.status_int, 409) + def test_POST_not_exist(self): timestamp = normalize_timestamp(time()) req = Request.blank('/sda1/p/a/c/fail', @@ -269,14 +304,16 @@ class TestObjectController(unittest.TestCase): old_http_connect = object_server.http_connect try: - timestamp = normalize_timestamp(time()) + ts = time() + timestamp = normalize_timestamp(ts) req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'POST'}, headers={'X-Timestamp': timestamp, 'Content-Type': 'text/plain', 'Content-Length': '0'}) resp = self.object_controller.PUT(req) + self.assertEquals(resp.status_int, 201) req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'POST'}, - headers={'X-Timestamp': timestamp, + headers={'X-Timestamp': normalize_timestamp(ts + 1), 'X-Container-Host': '1.2.3.4:0', 'X-Container-Partition': '3', 'X-Container-Device': 'sda1', @@ -287,7 +324,7 @@ class TestObjectController(unittest.TestCase): self.assertEquals(resp.status_int, 202) req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'POST'}, - headers={'X-Timestamp': timestamp, + headers={'X-Timestamp': normalize_timestamp(ts + 2), 'X-Container-Host': '1.2.3.4:0', 'X-Container-Partition': '3', 'X-Container-Device': 'sda1', @@ -298,7 +335,7 @@ class TestObjectController(unittest.TestCase): self.assertEquals(resp.status_int, 202) req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'POST'}, - headers={'X-Timestamp': timestamp, + headers={'X-Timestamp': normalize_timestamp(ts + 3), 'X-Container-Host': '1.2.3.4:0', 'X-Container-Partition': '3', 'X-Container-Device': 'sda1', @@ -439,6 +476,32 @@ class TestObjectController(unittest.TestCase): 'name': '/a/c/o', 'Content-Encoding': 'gzip'}) + def test_PUT_old_timestamp(self): + ts = time() + req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Timestamp': normalize_timestamp(ts), + 'Content-Length': '6', + 'Content-Type': 'application/octet-stream'}) + req.body = 'VERIFY' + resp = self.object_controller.PUT(req) + self.assertEquals(resp.status_int, 201) + + req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Timestamp': normalize_timestamp(ts), + 'Content-Type': 'text/plain', + 'Content-Encoding': 'gzip'}) + req.body = 'VERIFY TWO' + resp = self.object_controller.PUT(req) + self.assertEquals(resp.status_int, 409) + + req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Timestamp': normalize_timestamp(ts - 1), + 'Content-Type': 'text/plain', + 'Content-Encoding': 'gzip'}) + req.body = 'VERIFY THREE' + resp = self.object_controller.PUT(req) + self.assertEquals(resp.status_int, 409) + def test_PUT_no_etag(self): req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, headers={'X-Timestamp': normalize_timestamp(time()), @@ -1014,7 +1077,7 @@ class TestObjectController(unittest.TestCase): self.assertEquals(resp.status_int, 404) def test_DELETE(self): - """ Test swift.object_server.ObjectController.DELETE """ + # Test swift.object_server.ObjectController.DELETE req = Request.blank('/sda1/p/a/c', environ={'REQUEST_METHOD': 'DELETE'}) resp = self.object_controller.DELETE(req) @@ -1026,12 +1089,32 @@ class TestObjectController(unittest.TestCase): self.assertEquals(resp.status_int, 400) # self.assertRaises(KeyError, self.object_controller.DELETE, req) + # The following should have created a tombstone file timestamp = normalize_timestamp(time()) req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'DELETE'}, headers={'X-Timestamp': timestamp}) resp = self.object_controller.DELETE(req) self.assertEquals(resp.status_int, 404) + objfile = os.path.join(self.testdir, 'sda1', + storage_directory(object_server.DATADIR, 'p', + hash_path('a', 'c', 'o')), + timestamp + '.ts') + self.assert_(os.path.isfile(objfile)) + + # The following should *not* have created a tombstone file. + timestamp = normalize_timestamp(float(timestamp) - 1) + req = Request.blank('/sda1/p/a/c/o', + environ={'REQUEST_METHOD': 'DELETE'}, + headers={'X-Timestamp': timestamp}) + resp = self.object_controller.DELETE(req) + self.assertEquals(resp.status_int, 404) + objfile = os.path.join(self.testdir, 'sda1', + storage_directory(object_server.DATADIR, 'p', + hash_path('a', 'c', 'o')), + timestamp + '.ts') + self.assertFalse(os.path.isfile(objfile)) + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) sleep(.00001) timestamp = normalize_timestamp(time()) @@ -1045,17 +1128,19 @@ class TestObjectController(unittest.TestCase): resp = self.object_controller.PUT(req) self.assertEquals(resp.status_int, 201) + # The following should *not* have created a tombstone file. timestamp = normalize_timestamp(float(timestamp) - 1) req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'DELETE'}, headers={'X-Timestamp': timestamp}) resp = self.object_controller.DELETE(req) - self.assertEquals(resp.status_int, 204) + self.assertEquals(resp.status_int, 409) objfile = os.path.join(self.testdir, 'sda1', storage_directory(object_server.DATADIR, 'p', hash_path('a', 'c', 'o')), timestamp + '.ts') - self.assert_(os.path.isfile(objfile)) + self.assertFalse(os.path.isfile(objfile)) + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) sleep(.00001) timestamp = normalize_timestamp(time()) @@ -1070,6 +1155,103 @@ class TestObjectController(unittest.TestCase): timestamp + '.ts') self.assert_(os.path.isfile(objfile)) + def test_DELETE_container_updates(self): + # Test swift.object_server.ObjectController.DELETE and container + # updates, making sure container update is called in the correct + # state. + timestamp = normalize_timestamp(time()) + req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, + headers={ + 'X-Timestamp': timestamp, + 'Content-Type': 'application/octet-stream', + 'Content-Length': '4', + }) + req.body = 'test' + resp = self.object_controller.PUT(req) + self.assertEquals(resp.status_int, 201) + + calls_made = [0] + + def our_container_update(*args, **kwargs): + calls_made[0] += 1 + + orig_cu = self.object_controller.container_update + self.object_controller.container_update = our_container_update + try: + # The following request should return 409 (HTTP Conflict). A + # tombstone file should not have been created with this timestamp. + timestamp = normalize_timestamp(float(timestamp) - 1) + req = Request.blank('/sda1/p/a/c/o', + environ={'REQUEST_METHOD': 'DELETE'}, + headers={'X-Timestamp': timestamp}) + resp = self.object_controller.DELETE(req) + self.assertEquals(resp.status_int, 409) + objfile = os.path.join(self.testdir, 'sda1', + storage_directory(object_server.DATADIR, 'p', + hash_path('a', 'c', 'o')), + timestamp + '.ts') + self.assertFalse(os.path.isfile(objfile)) + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) + self.assertEquals(0, calls_made[0]) + + # The following request should return 204, and the object should + # be truly deleted (container update is performed) because this + # timestamp is newer. A tombstone file should have been created + # with this timestamp. + sleep(.00001) + timestamp = normalize_timestamp(time()) + req = Request.blank('/sda1/p/a/c/o', + environ={'REQUEST_METHOD': 'DELETE'}, + headers={'X-Timestamp': timestamp}) + resp = self.object_controller.DELETE(req) + self.assertEquals(resp.status_int, 204) + objfile = os.path.join(self.testdir, 'sda1', + storage_directory(object_server.DATADIR, 'p', + hash_path('a', 'c', 'o')), + timestamp + '.ts') + self.assert_(os.path.isfile(objfile)) + self.assertEquals(1, calls_made[0]) + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) + + # The following request should return a 404, as the object should + # already have been deleted, but it should have also performed a + # container update because the timestamp is newer, and a tombstone + # file should also exist with this timestamp. + sleep(.00001) + timestamp = normalize_timestamp(time()) + req = Request.blank('/sda1/p/a/c/o', + environ={'REQUEST_METHOD': 'DELETE'}, + headers={'X-Timestamp': timestamp}) + resp = self.object_controller.DELETE(req) + self.assertEquals(resp.status_int, 404) + objfile = os.path.join(self.testdir, 'sda1', + storage_directory(object_server.DATADIR, 'p', + hash_path('a', 'c', 'o')), + timestamp + '.ts') + self.assert_(os.path.isfile(objfile)) + self.assertEquals(2, calls_made[0]) + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) + + # The following request should return a 404, as the object should + # already have been deleted, and it should not have performed a + # container update because the timestamp is older, or created a + # tombstone file with this timestamp. + timestamp = normalize_timestamp(float(timestamp) - 1) + req = Request.blank('/sda1/p/a/c/o', + environ={'REQUEST_METHOD': 'DELETE'}, + headers={'X-Timestamp': timestamp}) + resp = self.object_controller.DELETE(req) + self.assertEquals(resp.status_int, 404) + objfile = os.path.join(self.testdir, 'sda1', + storage_directory(object_server.DATADIR, 'p', + hash_path('a', 'c', 'o')), + timestamp + '.ts') + self.assertFalse(os.path.isfile(objfile)) + self.assertEquals(2, calls_made[0]) + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) + finally: + self.object_controller.container_update = orig_cu + def test_call(self): """ Test swift.object_server.ObjectController.__call__ """ inbuf = StringIO() @@ -1161,7 +1343,7 @@ class TestObjectController(unittest.TestCase): 'SERVER_PROTOCOL': 'HTTP/1.0', 'CONTENT_LENGTH': '0', 'CONTENT_TYPE': 'text/html', - 'HTTP_X_TIMESTAMP': 1.2, + 'HTTP_X_TIMESTAMP': '1.2', 'wsgi.version': (1, 0), 'wsgi.url_scheme': 'http', 'wsgi.input': inbuf, @@ -1184,7 +1366,7 @@ class TestObjectController(unittest.TestCase): 'SERVER_PROTOCOL': 'HTTP/1.0', 'CONTENT_LENGTH': '0', 'CONTENT_TYPE': 'text/html', - 'HTTP_X_TIMESTAMP': 1.3, + 'HTTP_X_TIMESTAMP': '1.3', 'wsgi.version': (1, 0), 'wsgi.url_scheme': 'http', 'wsgi.input': inbuf,