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()