From 57b632fbb52c84e02fefa307ada45081d86b23a6 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Thu, 1 Mar 2018 13:01:49 +0000 Subject: [PATCH] Fix object-server to not 400 all expirer DELETEs In the related changes, we switched to using Timestamp.normal representations for the X-If-Delete-At header. However, the object-server required that the header be an int, and the trailing '.00000' would cause trip the "Bad X-If-Delete-At header value" error handling. Now, we'll convert both the expirer header and the stored X-Delete-At to Timestamps, even though we expect them to have no fractional value. Note that we *could* have changed the expirer to continue sending headers that are valid ints, but Timestamps are already the normal Swift-way of passing and comparing times -- we should use that. Related-Change: Ibf61eb1f767a48cb457dd494e1f7c12acfd205de Related-Change: Ie82622625d13177e08a363686ec632f63d24f4e9 Change-Id: Ida22c1c8c5bf21bdc72c33e225e75fb750f8444b --- swift/obj/expirer.py | 1 + swift/obj/server.py | 4 +-- test/unit/obj/test_server.py | 62 ++++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 2 deletions(-) diff --git a/swift/obj/expirer.py b/swift/obj/expirer.py index d5c94aa9e6..afa3fcb301 100644 --- a/swift/obj/expirer.py +++ b/swift/obj/expirer.py @@ -382,6 +382,7 @@ class ObjectExpirer(Daemon): '%(account)s %(container)s %(obj)s: %(err)s' % { 'account': task_account, 'container': task_container, 'obj': task_object, 'err': str(err.resp.status_int)}) + self.logger.debug(err.resp.body) except (Exception, Timeout) as err: self.logger.increment('errors') self.logger.exception( diff --git a/swift/obj/server.py b/swift/obj/server.py index 788480d7b1..ecf8b33442 100644 --- a/swift/obj/server.py +++ b/swift/obj/server.py @@ -1060,10 +1060,10 @@ class ObjectController(BaseStorageServer): else: response_class = HTTPConflict response_timestamp = max(orig_timestamp, req_timestamp) - orig_delete_at = int(orig_metadata.get('X-Delete-At') or 0) + orig_delete_at = Timestamp(orig_metadata.get('X-Delete-At') or 0) try: req_if_delete_at_val = request.headers['x-if-delete-at'] - req_if_delete_at = int(req_if_delete_at_val) + req_if_delete_at = Timestamp(req_if_delete_at_val) except KeyError: pass except ValueError: diff --git a/test/unit/obj/test_server.py b/test/unit/obj/test_server.py index 4fce9cb0e3..0571a80724 100644 --- a/test/unit/obj/test_server.py +++ b/test/unit/obj/test_server.py @@ -6064,6 +6064,68 @@ class TestObjectController(unittest.TestCase): except OSError as err: self.assertEqual(err.errno, errno.ENOENT) + def test_x_if_delete_at_formats(self): + policy = POLICIES.get_by_index(0) + test_time = time() + put_time = test_time + delete_time = test_time + 1 + delete_at_timestamp = int(test_time + 10000) + delete_at_container = str( + delete_at_timestamp / + self.object_controller.expiring_objects_container_divisor * + self.object_controller.expiring_objects_container_divisor) + + def do_test(if_delete_at, expected_status): + req = Request.blank( + '/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Timestamp': normalize_timestamp(put_time), + 'X-Delete-At': str(delete_at_timestamp), + 'X-Delete-At-Container': delete_at_container, + 'Content-Length': '4', + 'Content-Type': 'application/octet-stream'}) + req.body = 'TEST' + + # Mock out async_update so we don't get any async_pending files. + with mock.patch.object(self.object_controller, 'async_update'): + resp = req.get_response(self.object_controller) + self.assertEqual(resp.status_int, 201) + + req = Request.blank( + '/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'DELETE'}, + headers={'X-Timestamp': normalize_timestamp(delete_time), + 'X-Backend-Clean-Expiring-Object-Queue': 'false', + 'X-If-Delete-At': if_delete_at}) + # Again, we don't care about async_pending files (for this test) + with mock.patch.object(self.object_controller, 'async_update'): + resp = req.get_response(self.object_controller) + self.assertEqual(resp.status_int, expected_status) + + # Clean up the tombstone + objfile = self.df_mgr.get_diskfile('sda1', 'p', 'a', 'c', 'o', + policy=policy) + files = os.listdir(objfile._datadir) + self.assertEqual(len(files), 1, + 'Expected to find one file, got %r' % files) + if expected_status == 204: + self.assertTrue(files[0].endswith('.ts'), + 'Expected a tombstone, found %r' % files[0]) + else: + self.assertTrue(files[0].endswith('.data'), + 'Expected a data file, found %r' % files[0]) + os.unlink(os.path.join(objfile._datadir, files[0])) + + # More as a reminder than anything else + self.assertIsInstance(delete_at_timestamp, int) + + do_test(str(delete_at_timestamp), 204) + do_test(str(delete_at_timestamp) + ':', 400) + do_test(Timestamp(delete_at_timestamp).isoformat, 400) + do_test(Timestamp(delete_at_timestamp).normal, 204) + do_test(Timestamp(delete_at_timestamp, delta=1).normal, 412) + do_test(Timestamp(delete_at_timestamp, delta=-1).normal, 412) + do_test(Timestamp(delete_at_timestamp, offset=1).internal, 412) + do_test(Timestamp(delete_at_timestamp, offset=15).internal, 412) + def test_DELETE_but_expired(self): test_time = time() + 10000 delete_at_timestamp = int(test_time + 100)