commit 1f4ec235cdfd8c868f2d6458532f9dc32c00b8ca
Author: Peter Portante <peter.portante@redhat.com>
Date: Fri Jul 26 15:03:34 2013 -0400
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 <peter.portante@redhat.com>
diff --git a/swift/obj/server.py b/swift/obj/server.py
index fc23ea2..f416162 100644
--- a/swift/obj/server.py
+++ b/swift/obj/server.py
@@ -46,7 +46,7 @@ from swift.common.swob import HTTPAccepted, HTTPBadRequest, HTTPCreated, \
HTTPInternalServerError, HTTPNoContent, HTTPNotFound, HTTPNotModified, \
HTTPPreconditionFailed, HTTPRequestTimeout, HTTPUnprocessableEntity, \
HTTPClientDisconnect, HTTPMethodNotAllowed, Request, Response, UTC, \
- HTTPInsufficientStorage, multi_range_iterator
+ HTTPInsufficientStorage, multi_range_iterator, HTTPConflict
DATADIR = 'objects'
@@ -121,7 +121,6 @@ class DiskFile(object):
self.tmppath = None
self.logger = logger
self.metadata = {}
- self.meta_file = None
self.data_file = None
self.fp = None
self.iter_etag = None
@@ -133,15 +132,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
@@ -149,8 +151,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]
@@ -594,6 +596,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-'))
@@ -639,6 +644,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
@@ -863,23 +870,26 @@ class ObjectController(object):
return HTTPPreconditionFailed(
request=request,
body='X-If-Delete-At and X-Delete-At do not match')
- orig_timestamp = file.metadata.get('X-Timestamp')
- if file.is_deleted() or file.is_expired():
- response_class = HTTPNotFound
- metadata = {
- 'X-Timestamp': request.headers['X-Timestamp'], 'deleted': True,
- }
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_metadata(metadata, tombstone=True)
- file.unlinkold(metadata['X-Timestamp'])
- if not orig_timestamp or \
- orig_timestamp < request.headers['x-timestamp']:
+ orig_timestamp = file.metadata.get('X-Timestamp', 0)
+ req_timestamp = request.headers['X-Timestamp']
+ if file.is_deleted() or file.is_expired():
+ response_class = HTTPNotFound
+ else:
+ if orig_timestamp < req_timestamp:
+ response_class = HTTPNoContent
+ else:
+ response_class = HTTPConflict
+ if orig_timestamp < req_timestamp:
+ file.put_metadata({'X-Timestamp': req_timestamp},
+ tombstone=True)
+ file.unlinkold(req_timestamp)
self.container_update(
'DELETE', account, container, obj, request.headers,
- {'x-timestamp': metadata['X-Timestamp'],
+ {'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 8ee266b..b354b97 100755
--- a/test/unit/obj/test_server.py
+++ b/test/unit/obj/test_server.py
@@ -509,6 +509,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',
@@ -555,11 +590,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,
@@ -571,6 +610,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,
@@ -582,6 +623,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,
@@ -718,6 +761,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()),
@@ -1306,12 +1375,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.exists(objfile))
+ self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1)
sleep(.00001)
timestamp = normalize_timestamp(time())
@@ -1325,17 +1414,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())
@@ -1350,6 +1441,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()