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>
This commit is contained in:
Peter Portante
Thierry Carrez
parent
4eed6bf5b5
commit
176e41a309
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Reference in New Issue