From 8226761889841595326f1b91b016a54ea4ad98fd Mon Sep 17 00:00:00 2001 From: David Hadas Date: Wed, 26 Jun 2013 08:23:00 +0300 Subject: [PATCH] Deleted account respond as non existing accounts Currently clients can not distinguish between non existing accounts (which can be created) and accounts marked for deletion, which has not yet been reaped and therefore cannot be re-created until reaped. Following this patch, if an account is marked as deleted but hasn't been reaped and is still on disk, responses will include a status header: 'X-Account-Status' = 'Deleted' Fixes:Bug #1188609 Change-Id: Ibd39965ae3f5d45fd78f130e0e31f5a0141a8633 --- swift/account/server.py | 29 +++++++++++++++----- swift/proxy/controllers/base.py | 22 +++++++++++---- test/unit/__init__.py | 6 ++-- test/unit/account/test_server.py | 47 ++++++++++++++++++++++++++++++++ test/unit/proxy/test_server.py | 22 +++++++++++---- 5 files changed, 105 insertions(+), 21 deletions(-) diff --git a/swift/account/server.py b/swift/account/server.py index 4e913a33a5..1ec8244e45 100644 --- a/swift/account/server.py +++ b/swift/account/server.py @@ -24,7 +24,7 @@ from eventlet import Timeout import swift.common.db from swift.account.utils import account_listing_response, \ account_listing_content_type -from swift.common.db import AccountBroker +from swift.common.db import AccountBroker, DatabaseConnectionError from swift.common.utils import get_logger, get_param, hash_path, public, \ normalize_timestamp, storage_directory, config_true_value, \ validate_device_partition, json, timing_stats @@ -74,6 +74,20 @@ class AccountController(object): db_path = os.path.join(self.root, drive, db_dir, hsh + '.db') return AccountBroker(db_path, account=account, logger=self.logger) + def _deleted_response(self, broker, req, resp, body=''): + # We are here since either the account does not exist or + # it exists but marked for deletion. + headers = {} + # Try to check if account exists and is marked for deletion + try: + if broker.is_status_deleted(): + # Account does exist and is marked for deletion + headers = {'X-Account-Status': 'Deleted'} + except DatabaseConnectionError: + # Account does not exist! + pass + return resp(request=req, headers=headers, charset='utf-8', body=body) + @public @timing_stats() def DELETE(self, req): @@ -92,9 +106,9 @@ class AccountController(object): content_type='text/plain') broker = self._get_account_broker(drive, part, account) if broker.is_deleted(): - return HTTPNotFound(request=req) + return self._deleted_response(broker, req, HTTPNotFound) broker.delete_db(req.headers['x-timestamp']) - return HTTPNoContent(request=req) + return self._deleted_response(broker, req, HTTPNoContent) @public @timing_stats() @@ -140,7 +154,8 @@ class AccountController(object): except swift.common.db.DatabaseAlreadyExists: pass elif broker.is_status_deleted(): - return HTTPForbidden(request=req, body='Recently deleted') + return self._deleted_response(broker, req, HTTPForbidden, + body='Recently deleted') else: created = broker.is_deleted() broker.update_put_timestamp(timestamp) @@ -185,7 +200,7 @@ class AccountController(object): broker.pending_timeout = 0.1 broker.stale_reads_ok = True if broker.is_deleted(): - return HTTPNotFound(request=req) + return self._deleted_response(broker, req, HTTPNotFound) info = broker.get_info() headers = { 'X-Account-Container-Count': info['container_count'], @@ -238,7 +253,7 @@ class AccountController(object): broker.pending_timeout = 0.1 broker.stale_reads_ok = True if broker.is_deleted(): - return HTTPNotFound(request=req) + return self._deleted_response(broker, req, HTTPNotFound) return account_listing_response(account, req, out_content_type, broker, limit, marker, end_marker, prefix, delimiter) @@ -286,7 +301,7 @@ class AccountController(object): return HTTPInsufficientStorage(drive=drive, request=req) broker = self._get_account_broker(drive, part, account) if broker.is_deleted(): - return HTTPNotFound(request=req) + return self._deleted_response(broker, req, HTTPNotFound) timestamp = normalize_timestamp(req.headers['x-timestamp']) metadata = {} metadata.update((key, (value, timestamp)) diff --git a/swift/proxy/controllers/base.py b/swift/proxy/controllers/base.py index 0082d9368b..4de4408b1d 100644 --- a/swift/proxy/controllers/base.py +++ b/swift/proxy/controllers/base.py @@ -689,7 +689,8 @@ class Controller(object): resp = conn.getresponse() if not is_informational(resp.status) and \ not is_server_error(resp.status): - return resp.status, resp.reason, resp.read() + return resp.status, resp.reason, resp.getheaders(), \ + resp.read() elif resp.status == HTTP_INSUFFICIENT_STORAGE: self.error_limit(node, _('ERROR Insufficient Storage')) except (Exception, Timeout): @@ -722,13 +723,14 @@ class Controller(object): head, query_string, self.app.logger.thread_locals) response = [resp for resp in pile if resp] while len(response) < len(start_nodes): - response.append((HTTP_SERVICE_UNAVAILABLE, '', '')) - statuses, reasons, bodies = zip(*response) + response.append((HTTP_SERVICE_UNAVAILABLE, '', '', '')) + statuses, reasons, resp_headers, bodies = zip(*response) return self.best_response(req, statuses, reasons, bodies, - '%s %s' % (self.server_type, req.method)) + '%s %s' % (self.server_type, req.method), + headers=resp_headers) def best_response(self, req, statuses, reasons, bodies, server_type, - etag=None): + etag=None, headers=None): """ Given a list of responses from several servers, choose the best to return to the API. @@ -739,6 +741,7 @@ class Controller(object): :param bodies: bodies of each response :param server_type: type of server the responses came from :param etag: etag + :param headers: headers of each response :returns: swob.Response object with the correct status, body, etc. set """ resp = Response(request=req) @@ -751,6 +754,8 @@ class Controller(object): status_index = statuses.index(status) resp.status = '%s %s' % (status, reasons[status_index]) resp.body = bodies[status_index] + if headers: + update_headers(resp, headers[status_index]) if etag: resp.headers['etag'] = etag.strip('"') return resp @@ -922,6 +927,7 @@ class Controller(object): statuses = [] reasons = [] bodies = [] + source_headers = [] sources = [] newest = config_true_value(req.headers.get('x-newest', 'f')) for node in self.iter_nodes(ring, partition): @@ -950,11 +956,13 @@ class Controller(object): statuses.append(HTTP_NOT_FOUND) reasons.append('') bodies.append('') + source_headers.append('') self.close_swift_conn(possible_source) else: statuses.append(possible_source.status) reasons.append(possible_source.reason) bodies.append('') + source_headers.append('') sources.append((possible_source, node)) if not newest: # one good source is enough break @@ -962,6 +970,7 @@ class Controller(object): statuses.append(possible_source.status) reasons.append(possible_source.reason) bodies.append(possible_source.read()) + source_headers.append(possible_source.getheaders()) if possible_source.status == HTTP_INSUFFICIENT_STORAGE: self.error_limit(node, _('ERROR Insufficient Storage')) elif is_server_error(possible_source.status): @@ -995,7 +1004,8 @@ class Controller(object): res.content_type = source.getheader('Content-Type') if not res: res = self.best_response(req, statuses, reasons, bodies, - '%s %s' % (server_type, req.method)) + '%s %s' % (server_type, req.method), + headers=source_headers) try: (account, container) = split_path(req.path_info, 1, 2) _set_info_cache(self.app, req.environ, account, container, res) diff --git a/test/unit/__init__.py b/test/unit/__init__.py index 207380aff8..e8bd43b05f 100644 --- a/test/unit/__init__.py +++ b/test/unit/__init__.py @@ -402,8 +402,10 @@ def fake_http_connect(*code_iter, **kwargs): 'x-object-meta-test': 'testing', 'x-delete-at': '9876543210', 'etag': etag, - 'x-works': 'yes', - 'x-account-container-count': kwargs.get('count', 12345)} + 'x-works': 'yes'} + if self.status // 100 == 2: + headers['x-account-container-count'] = \ + kwargs.get('count', 12345) if not self.timestamp: del headers['x-timestamp'] try: diff --git a/test/unit/account/test_server.py b/test/unit/account/test_server.py index 22902da825..3b8f109a15 100644 --- a/test/unit/account/test_server.py +++ b/test/unit/account/test_server.py @@ -49,6 +49,7 @@ class TestAccountController(unittest.TestCase): 'HTTP_X_TIMESTAMP': '0'}) resp = self.controller.DELETE(req) self.assertEquals(resp.status_int, 404) + self.assertTrue('X-Account-Status' not in resp.headers) def test_DELETE_empty(self): req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'PUT', @@ -58,6 +59,7 @@ class TestAccountController(unittest.TestCase): 'HTTP_X_TIMESTAMP': '1'}) resp = self.controller.DELETE(req) self.assertEquals(resp.status_int, 204) + self.assertEquals(resp.headers['X-Account-Status'], 'Deleted') def test_DELETE_not_empty(self): req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'PUT', @@ -74,6 +76,7 @@ class TestAccountController(unittest.TestCase): resp = self.controller.DELETE(req) # We now allow deleting non-empty accounts self.assertEquals(resp.status_int, 204) + self.assertEquals(resp.headers['X-Account-Status'], 'Deleted') def test_DELETE_now_empty(self): req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'PUT', @@ -99,6 +102,7 @@ class TestAccountController(unittest.TestCase): 'HTTP_X_TIMESTAMP': '1'}) resp = self.controller.DELETE(req) self.assertEquals(resp.status_int, 204) + self.assertEquals(resp.headers['X-Account-Status'], 'Deleted') def test_DELETE_invalid_partition(self): req = Request.blank('/sda1/./a', environ={'REQUEST_METHOD': 'DELETE', @@ -123,9 +127,29 @@ class TestAccountController(unittest.TestCase): self.assertEquals(resp.status_int, 507) def test_HEAD_not_found(self): + # Test the case in which account does not exist (can be recreated) req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'HEAD'}) resp = self.controller.HEAD(req) self.assertEquals(resp.status_int, 404) + self.assertTrue('X-Account-Status' not in resp.headers) + + # Test the case in which account was deleted but not yet reaped + req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'PUT', + 'HTTP_X_TIMESTAMP': '0'}) + self.controller.PUT(req) + req = Request.blank('/sda1/p/a/c1', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Put-Timestamp': '1', + 'X-Delete-Timestamp': '0', + 'X-Object-Count': '0', + 'X-Bytes-Used': '0'}) + self.controller.PUT(req) + req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'DELETE', + 'HTTP_X_TIMESTAMP': '1'}) + resp = self.controller.DELETE(req) + req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'HEAD'}) + resp = self.controller.HEAD(req) + self.assertEquals(resp.status_int, 404) + self.assertEquals(resp.headers['X-Account-Status'], 'Deleted') def test_HEAD_empty_account(self): req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'PUT', @@ -219,6 +243,7 @@ class TestAccountController(unittest.TestCase): 'X-Timestamp': normalize_timestamp(0)}) resp = self.controller.PUT(req) self.assertEquals(resp.status_int, 404) + self.assertTrue('X-Account-Status' not in resp.headers) def test_PUT(self): req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'PUT', @@ -244,6 +269,7 @@ class TestAccountController(unittest.TestCase): resp = self.controller.PUT(req) self.assertEquals(resp.status_int, 403) self.assertEquals(resp.body, 'Recently deleted') + self.assertEquals(resp.headers['X-Account-Status'], 'Deleted') def test_PUT_GET_metadata(self): # Set metadata header @@ -388,11 +414,32 @@ class TestAccountController(unittest.TestCase): 'HTTP_X_TIMESTAMP': '2'}) resp = self.controller.POST(req) self.assertEquals(resp.status_int, 404) + self.assertEquals(resp.headers['X-Account-Status'], 'Deleted') def test_GET_not_found_plain(self): + # Test the case in which account does not exist (can be recreated) req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'GET'}) resp = self.controller.GET(req) self.assertEquals(resp.status_int, 404) + self.assertTrue('X-Account-Status' not in resp.headers) + + # Test the case in which account was deleted but not yet reaped + req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'PUT', + 'HTTP_X_TIMESTAMP': '0'}) + self.controller.PUT(req) + req = Request.blank('/sda1/p/a/c1', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Put-Timestamp': '1', + 'X-Delete-Timestamp': '0', + 'X-Object-Count': '0', + 'X-Bytes-Used': '0'}) + self.controller.PUT(req) + req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'DELETE', + 'HTTP_X_TIMESTAMP': '1'}) + resp = self.controller.DELETE(req) + req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'GET'}) + resp = self.controller.GET(req) + self.assertEquals(resp.status_int, 404) + self.assertEquals(resp.headers['X-Account-Status'], 'Deleted') def test_GET_not_found_json(self): req = Request.blank('/sda1/p/a?format=json', diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index c5e2dab653..7f56de9a6b 100644 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -350,12 +350,12 @@ class TestController(unittest.TestCase): # Test the internal representation in memcache # 'container_count' changed from 0 to None cache_key = get_account_memcache_key(self.account) - container_info = {'status': 404, - 'container_count': None, # internally keep None - 'total_object_count': None, - 'bytes': None, - 'meta': {}} - self.assertEquals(container_info, + account_info = {'status': 404, + 'container_count': None, # internally keep None + 'total_object_count': None, + 'bytes': None, + 'meta': {}} + self.assertEquals(account_info, self.memcache.get(cache_key)) set_http_connect() @@ -2279,6 +2279,16 @@ class TestObjectController(unittest.TestCase): node_iter=iter(node_list))) self.assertEqual(node_list, got_nodes) + def test_best_response_sets_headers(self): + controller = proxy_server.ObjectController(self.app, 'account', + 'container', 'object') + req = Request.blank('/a/c/o', environ={'REQUEST_METHOD': 'GET'}) + resp = controller.best_response(req, [200] * 3, ['OK'] * 3, [''] * 3, + 'Object', headers=[{'X-Test': '1'}, + {'X-Test': '2'}, + {'X-Test': '3'}]) + self.assertEquals(resp.headers['X-Test'], '1') + def test_best_response_sets_etag(self): controller = proxy_server.ObjectController(self.app, 'account', 'container', 'object')