From a757264d546387de5363491532ab056c8bf108ae Mon Sep 17 00:00:00 2001 From: Peter Portante Date: Fri, 26 Jul 2013 15:36:14 -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: I15119314b3f8693c9862ee8b8fc9f86aeaa8333d Signed-off-by: Peter Portante --- swift/obj/server.py | 65 ++++++++------- test/unit/obj/test_server.py | 194 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 228 insertions(+), 31 deletions(-) diff --git a/swift/obj/server.py b/swift/obj/server.py index ec59dce..76ac561 100644 --- a/swift/obj/server.py +++ b/swift/obj/server.py @@ -29,7 +29,7 @@ from contextlib import contextmanager from webob import Request, Response, UTC from webob.exc import HTTPAccepted, HTTPBadRequest, HTTPCreated, \ - HTTPInternalServerError, HTTPNoContent, HTTPNotFound, \ + HTTPInternalServerError, HTTPNoContent, HTTPNotFound, HTTPConflict, \ HTTPNotModified, HTTPPreconditionFailed, \ HTTPRequestTimeout, HTTPUnprocessableEntity, HTTPMethodNotAllowed from xattr import getxattr, setxattr @@ -121,7 +121,6 @@ class DiskFile(object): self.tmpdir = os.path.join(path, device, 'tmp') self.logger = logger self.metadata = {} - self.meta_file = None self.data_file = None self.fp = None self.iter_etag = None @@ -132,15 +131,18 @@ class DiskFile(object): if not os.path.exists(self.datadir): return files = sorted(os.listdir(self.datadir), reverse=True) - for file in files: - if file.endswith('.ts'): - self.data_file = self.meta_file = None - self.metadata = {'deleted': True} - return - if file.endswith('.meta') and not self.meta_file: - self.meta_file = os.path.join(self.datadir, file) - if file.endswith('.data') and not self.data_file: - self.data_file = os.path.join(self.datadir, file) + meta_file = None + for afile in files: + if afile.endswith('.ts'): + self.data_file = None + with open(os.path.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 = os.path.join(self.datadir, afile) + if afile.endswith('.data') and not self.data_file: + self.data_file = os.path.join(self.datadir, afile) break if not self.data_file: return @@ -148,8 +150,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 DISALLOWED_HEADERS: del self.metadata[key] @@ -532,6 +534,9 @@ class ObjectController(object): except (DiskFileError, DiskFileNotExist): file.quarantine() return HTTPNotFound(request=request) + orig_timestamp = 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].lower().startswith('x-object-meta-')) @@ -584,6 +589,8 @@ class ObjectController(object): file = DiskFile(self.devices, device, partition, account, container, obj, self.logger, disk_chunk_size=self.disk_chunk_size) orig_timestamp = 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() upload_size = 0 @@ -814,7 +821,6 @@ class ObjectController(object): if self.mount_check and not check_mount(self.devices, device): self.logger.increment('DELETE.errors') return HTTPInsufficientStorage(drive=device, request=request) - response_class = HTTPNoContent file = DiskFile(self.devices, device, partition, account, container, obj, self.logger, disk_chunk_size=self.disk_chunk_size) if 'x-if-delete-at' in request.headers and \ @@ -823,23 +829,26 @@ class ObjectController(object): self.logger.timing_since('DELETE.timing', start_time) return HTTPPreconditionFailed(request=request, body='X-If-Delete-At and X-Delete-At do not match') - orig_timestamp = file.metadata.get('X-Timestamp') + old_delete_at = int(file.metadata.get('X-Delete-At') or 0) + if old_delete_at: + self.delete_at_update('DELETE', old_delete_at, account, + container, obj, request.headers, device) + orig_timestamp = file.metadata.get('X-Timestamp', 0) + req_timestamp = request.headers['X-Timestamp'] if file.is_deleted(): response_class = HTTPNotFound - metadata = { - 'X-Timestamp': request.headers['X-Timestamp'], 'deleted': True, - } - with file.mkstemp() as (fd, tmppath): - old_delete_at = int(file.metadata.get('X-Delete-At') or 0) - if old_delete_at: - self.delete_at_update('DELETE', old_delete_at, account, - container, obj, request.headers, device) - file.put(fd, tmppath, metadata, extension='.ts') - file.unlinkold(metadata['X-Timestamp']) - if not orig_timestamp or \ - orig_timestamp < request.headers['x-timestamp']: + else: + if orig_timestamp < req_timestamp: + response_class = HTTPNoContent + else: + response_class = HTTPConflict + if orig_timestamp < req_timestamp: + metadata = {'X-Timestamp': req_timestamp} + with file.mkstemp() as (fd, tmppath): + file.put(fd, tmppath, metadata, extension='.ts') + file.unlinkold(req_timestamp) self.container_update('DELETE', account, container, obj, - request.headers, {'x-timestamp': metadata['X-Timestamp'], + request.headers, {'x-timestamp': req_timestamp, 'x-trans-id': request.headers.get('x-trans-id', '-')}, device) resp = response_class(request=request) diff --git a/test/unit/obj/test_server.py b/test/unit/obj/test_server.py index 0d94dba..b5e4e99 100644 --- a/test/unit/obj/test_server.py +++ b/test/unit/obj/test_server.py @@ -428,6 +428,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', @@ -474,11 +509,15 @@ 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) + + timestamp = normalize_timestamp(ts + 1) req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'POST'}, headers={'X-Timestamp': timestamp, @@ -490,6 +529,8 @@ class TestObjectController(unittest.TestCase): object_server.http_connect = mock_http_connect(202) resp = self.object_controller.POST(req) self.assertEquals(resp.status_int, 202) + + timestamp = normalize_timestamp(ts + 2) req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'POST'}, headers={'X-Timestamp': timestamp, @@ -501,6 +542,8 @@ class TestObjectController(unittest.TestCase): object_server.http_connect = mock_http_connect(202, with_exc=True) resp = self.object_controller.POST(req) self.assertEquals(resp.status_int, 202) + + timestamp = normalize_timestamp(ts + 3) req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'POST'}, headers={'X-Timestamp': timestamp, @@ -637,6 +680,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()), @@ -1225,12 +1294,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()) @@ -1244,17 +1333,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.exists(objfile)) + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) sleep(.00001) timestamp = normalize_timestamp(time()) @@ -1269,6 +1360,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() -- 1.8.1.4