From 15c2ca55f02e1785f999a6790d9a5ff033449bcb Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Thu, 23 May 2013 18:53:51 -0700 Subject: [PATCH] Fix faked-out account GET for JSON and XML. If account autocreation is on and the proxy receives a GET request for a nonexistent account, it'll fake up a response that makes it look as if the account exists, but without reifying that account into sqlite DB files. That faked-out response was just fine as long as you wanted a text/plain response, but it didn't handle the case of format=json or format=xml; in those cases, the response would still be text/plain. This can break clients, and certainly causes crashes in swift3. Now, those responses match as closely as possible. The code for generating an account-listing response has been pulled into (the new) swift.account.utils module, and both the fake response and the real response use it, thus ensuring that they can't accidentally diverge. There's also a new probe test for that non-divergence. Also, cleaned up a redundant matching of the Accept header in the code for generating the account listing. Note that some of the added tests here pass with or without this code change; they were added because the code I was changing (parts of the real account GET) wasn't covered by tests. Bug 1183169 Change-Id: I2a3b8e5d9053e4d0280a320f31efa7c90c94bb06 --- swift/account/server.py | 67 ++-------- swift/account/utils.py | 121 ++++++++++++++++++ swift/common/utils.py | 2 +- swift/proxy/controllers/account.py | 22 ++-- .../test_account_get_fake_responses_match.py | 115 +++++++++++++++++ test/unit/account/test_server.py | 6 + test/unit/proxy/test_server.py | 63 +++++++++ 7 files changed, 325 insertions(+), 71 deletions(-) create mode 100644 swift/account/utils.py create mode 100755 test/probe/test_account_get_fake_responses_match.py diff --git a/swift/account/server.py b/swift/account/server.py index b1cc32ad63..53a7f2a8ea 100644 --- a/swift/account/server.py +++ b/swift/account/server.py @@ -18,11 +18,12 @@ from __future__ import with_statement import os import time import traceback -from xml.sax import saxutils 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.utils import get_logger, get_param, hash_path, public, \ normalize_timestamp, storage_directory, config_true_value, \ @@ -33,7 +34,7 @@ from swift.common.db_replicator import ReplicatorRpc from swift.common.swob import HTTPAccepted, HTTPBadRequest, \ HTTPCreated, HTTPForbidden, HTTPInternalServerError, \ HTTPMethodNotAllowed, HTTPNoContent, HTTPNotFound, \ - HTTPPreconditionFailed, HTTPConflict, Request, Response, \ + HTTPPreconditionFailed, HTTPConflict, Request, \ HTTPInsufficientStorage, HTTPNotAcceptable @@ -219,17 +220,13 @@ class AccountController(object): ACCOUNT_LISTING_LIMIT) marker = get_param(req, 'marker', '') end_marker = get_param(req, 'end_marker') - query_format = get_param(req, 'format') except UnicodeDecodeError, err: return HTTPBadRequest(body='parameters not utf8', content_type='text/plain', request=req) - if query_format: - req.accept = FORMAT2CONTENT_TYPE.get(query_format.lower(), - FORMAT2CONTENT_TYPE['plain']) - out_content_type = req.accept.best_match( - ['text/plain', 'application/json', 'application/xml', 'text/xml']) - if not out_content_type: - return HTTPNotAcceptable(request=req) + out_content_type, error = account_listing_content_type(req) + if error: + return error + if self.mount_check and not check_mount(self.root, drive): return HTTPInsufficientStorage(drive=drive, request=req) broker = self._get_account_broker(drive, part, account) @@ -237,53 +234,9 @@ class AccountController(object): broker.stale_reads_ok = True if broker.is_deleted(): return HTTPNotFound(request=req) - info = broker.get_info() - resp_headers = { - 'X-Account-Container-Count': info['container_count'], - 'X-Account-Object-Count': info['object_count'], - 'X-Account-Bytes-Used': info['bytes_used'], - 'X-Timestamp': info['created_at'], - 'X-PUT-Timestamp': info['put_timestamp']} - resp_headers.update((key, value) - for key, (value, timestamp) in - broker.metadata.iteritems() if value != '') - account_list = broker.list_containers_iter(limit, marker, end_marker, - prefix, delimiter) - if out_content_type == 'application/json': - data = [] - for (name, object_count, bytes_used, is_subdir) in account_list: - if is_subdir: - data.append({'subdir': name}) - else: - data.append({'name': name, 'count': object_count, - 'bytes': bytes_used}) - account_list = json.dumps(data) - elif out_content_type.endswith('/xml'): - output_list = ['', - '' % account] - for (name, object_count, bytes_used, is_subdir) in account_list: - name = saxutils.escape(name) - if is_subdir: - output_list.append('' % name) - else: - item = '%s%s' \ - '%s' % \ - (name, object_count, bytes_used) - output_list.append(item) - output_list.append('') - account_list = '\n'.join(output_list) - else: - if not account_list: - resp_headers['Content-Type'] = req.accept.best_match( - ['text/plain', 'application/json', - 'application/xml', 'text/xml']) - return HTTPNoContent(request=req, headers=resp_headers, - charset='utf-8') - account_list = '\n'.join(r[0] for r in account_list) + '\n' - ret = Response(body=account_list, request=req, headers=resp_headers) - ret.content_type = out_content_type - ret.charset = 'utf-8' - return ret + return account_listing_response(account, req, out_content_type, broker, + limit, marker, end_marker, prefix, + delimiter) @public @timing_stats() diff --git a/swift/account/utils.py b/swift/account/utils.py new file mode 100644 index 0000000000..20dbc0aa6d --- /dev/null +++ b/swift/account/utils.py @@ -0,0 +1,121 @@ +# Copyright (c) 2010-2013 OpenStack, LLC. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import time +from xml.sax import saxutils + +from swift.common.constraints import FORMAT2CONTENT_TYPE +from swift.common.swob import HTTPOk, HTTPNoContent, HTTPNotAcceptable, \ + HTTPBadRequest +from swift.common.utils import get_param, json, normalize_timestamp + + +class FakeAccountBroker(object): + """ + Quacks like an account broker, but doesn't actually do anything. Responds + like an account broker would for a real, empty account with no metadata. + """ + def get_info(self): + now = normalize_timestamp(time.time()) + return {'container_count': 0, + 'object_count': 0, + 'bytes_used': 0, + 'created_at': now, + 'put_timestamp': now} + + def list_containers_iter(self, *_, **__): + return [] + + @property + def metadata(self): + return {} + + +def account_listing_content_type(req): + """ + Figure out the content type of an account-listing response. + + Returns a 2-tuple: (content_type, error). Only one of them will be set; + the other will be None. + """ + try: + query_format = get_param(req, 'format') + except UnicodeDecodeError: + return (None, HTTPBadRequest(body='parameters not utf8', + content_type='text/plain', request=req)) + if query_format: + req.accept = FORMAT2CONTENT_TYPE.get(query_format.lower(), + FORMAT2CONTENT_TYPE['plain']) + content_type = req.accept.best_match( + ['text/plain', 'application/json', 'application/xml', 'text/xml']) + if not content_type: + return (None, HTTPNotAcceptable(request=req)) + else: + return (content_type, None) + + +def account_listing_response(account, req, response_content_type, broker=None, + limit='', marker='', end_marker='', prefix='', + delimiter=''): + if broker is None: + broker = FakeAccountBroker() + + info = broker.get_info() + resp_headers = { + 'X-Account-Container-Count': info['container_count'], + 'X-Account-Object-Count': info['object_count'], + 'X-Account-Bytes-Used': info['bytes_used'], + 'X-Timestamp': info['created_at'], + 'X-PUT-Timestamp': info['put_timestamp']} + resp_headers.update((key, value) + for key, (value, timestamp) in + broker.metadata.iteritems() if value != '') + + account_list = broker.list_containers_iter(limit, marker, end_marker, + prefix, delimiter) + if response_content_type == 'application/json': + data = [] + for (name, object_count, bytes_used, is_subdir) in account_list: + if is_subdir: + data.append({'subdir': name}) + else: + data.append({'name': name, 'count': object_count, + 'bytes': bytes_used}) + account_list = json.dumps(data) + elif response_content_type.endswith('/xml'): + output_list = ['', + '' % account] + for (name, object_count, bytes_used, is_subdir) in account_list: + name = saxutils.escape(name) + if is_subdir: + output_list.append('' % name) + else: + item = '%s%s' \ + '%s' % \ + (name, object_count, bytes_used) + output_list.append(item) + output_list.append('') + account_list = '\n'.join(output_list) + else: + if not account_list: + resp = HTTPNoContent(request=req, headers=resp_headers) + resp.content_type = response_content_type + resp.charset = 'utf-8' + return resp + account_list = '\n'.join(r[0] for r in account_list) + '\n' + ret = HTTPOk(body=account_list, request=req, headers=resp_headers) + ret.content_type = response_content_type + ret.charset = 'utf-8' + return ret diff --git a/swift/common/utils.py b/swift/common/utils.py index fbf92eb1ec..6c012b80b8 100644 --- a/swift/common/utils.py +++ b/swift/common/utils.py @@ -1021,7 +1021,7 @@ def storage_directory(datadir, partition, hash): def hash_path(account, container=None, object=None, raw_digest=False): """ - Get the connonical hash for an account/container/object + Get the canonical hash for an account/container/object :param account: Account :param container: Container diff --git a/swift/proxy/controllers/account.py b/swift/proxy/controllers/account.py index 959b336a78..d3ddaa816f 100644 --- a/swift/proxy/controllers/account.py +++ b/swift/proxy/controllers/account.py @@ -24,15 +24,15 @@ # These shenanigans are to ensure all related objects can be garbage # collected. We've seen objects hang around forever otherwise. -import time from urllib import unquote -from swift.common.utils import normalize_timestamp, public +from swift.account.utils import account_listing_response, \ + account_listing_content_type +from swift.common.utils import public from swift.common.constraints import check_metadata, MAX_ACCOUNT_NAME_LENGTH from swift.common.http import HTTP_NOT_FOUND from swift.proxy.controllers.base import Controller, get_account_memcache_key -from swift.common.swob import HTTPBadRequest, HTTPMethodNotAllowed, \ - HTTPNoContent +from swift.common.swob import HTTPBadRequest, HTTPMethodNotAllowed class AccountController(Controller): @@ -59,15 +59,11 @@ class AccountController(Controller): req, _('Account'), self.app.account_ring, partition, req.path_info.rstrip('/')) if resp.status_int == HTTP_NOT_FOUND and self.app.account_autocreate: - # Fake a response - headers = {'Content-Length': '0', - 'Accept-Ranges': 'bytes', - 'Content-Type': 'text/plain; charset=utf-8', - 'X-Timestamp': normalize_timestamp(time.time()), - 'X-Account-Bytes-Used': '0', - 'X-Account-Container-Count': '0', - 'X-Account-Object-Count': '0'} - resp = HTTPNoContent(request=req, headers=headers) + content_type, error = account_listing_content_type(req) + if error: + return error + return account_listing_response(self.account_name, req, + content_type) return resp @public diff --git a/test/probe/test_account_get_fake_responses_match.py b/test/probe/test_account_get_fake_responses_match.py new file mode 100755 index 0000000000..ede99e79fc --- /dev/null +++ b/test/probe/test_account_get_fake_responses_match.py @@ -0,0 +1,115 @@ +#!/usr/bin/python -u +# Copyright (c) 2010-2013 OpenStack, LLC. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import httplib +import re +import unittest + +from swiftclient import client, get_auth +from test.probe.common import kill_servers, reset_environment +from urlparse import urlparse + + +class TestAccountGetFakeResponsesMatch(unittest.TestCase): + + def setUp(self): + (self.pids, self.port2server, self.account_ring, self.container_ring, + self.object_ring, self.url, self.token, + self.account, self.configs) = reset_environment() + self.url, self.token = get_auth( + 'http://127.0.0.1:8080/auth/v1.0', 'admin:admin', 'admin') + + def tearDown(self): + kill_servers(self.port2server, self.pids) + + def _account_path(self, account): + _, _, path, _, _, _ = urlparse(self.url) + + basepath, _ = path.rsplit('/', 1) + return basepath + '/' + account + + def _get(self, *a, **kw): + kw['method'] = 'GET' + return self._account_request(*a, **kw) + + def _account_request(self, account, method, headers=None): + if headers is None: + headers = {} + headers['X-Auth-Token'] = self.token + + scheme, netloc, path, _, _, _ = urlparse(self.url) + host, port = netloc.split(':') + port = int(port) + + conn = httplib.HTTPConnection(host, port) + conn.request(method, self._account_path(account), headers=headers) + resp = conn.getresponse() + if resp.status // 100 != 2: + raise Exception("Unexpected status %s\n%s" % + (resp.status, resp.read())) + + response_headers = dict(resp.getheaders()) + response_body = resp.read() + resp.close() + return response_headers, response_body + + def test_main(self): + # Two accounts: "real" and "fake". The fake one doesn't have any .db + # files on disk; the real one does. The real one is empty. + # + # Make sure the important response fields match. + + real_acct = "AUTH_real" + fake_acct = "AUTH_fake" + + self._account_request(real_acct, 'POST', + {'X-Account-Meta-Bert': 'Ernie'}) + + # text + real_headers, real_body = self._get(real_acct) + fake_headers, fake_body = self._get(fake_acct) + + self.assertEqual(real_body, fake_body) + self.assertEqual(real_headers['content-type'], + fake_headers['content-type']) + + # json + real_headers, real_body = self._get( + real_acct, headers={'Accept': 'application/json'}) + fake_headers, fake_body = self._get( + fake_acct, headers={'Accept': 'application/json'}) + + self.assertEqual(real_body, fake_body) + self.assertEqual(real_headers['content-type'], + fake_headers['content-type']) + + # xml + real_headers, real_body = self._get( + real_acct, headers={'Accept': 'application/xml'}) + fake_headers, fake_body = self._get( + fake_acct, headers={'Accept': 'application/xml'}) + + # the account name is in the XML response + real_body = re.sub('AUTH_\w{4}', 'AUTH_someaccount', real_body) + fake_body = re.sub('AUTH_\w{4}', 'AUTH_someaccount', fake_body) + + self.assertEqual(real_body, fake_body) + self.assertEqual(real_headers['content-type'], + fake_headers['content-type']) + + +if __name__ == '__main__': + unittest.main() diff --git a/test/unit/account/test_server.py b/test/unit/account/test_server.py index 4c018b4a15..255ac71dc6 100644 --- a/test/unit/account/test_server.py +++ b/test/unit/account/test_server.py @@ -406,6 +406,8 @@ class TestAccountController(unittest.TestCase): req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'GET'}) resp = self.controller.GET(req) self.assertEquals(resp.status_int, 204) + self.assertEquals(resp.headers['Content-Type'], + 'text/plain; charset=utf-8') def test_GET_empty_account_json(self): req = Request.blank('/sda1/p/a?format=json', @@ -415,6 +417,8 @@ class TestAccountController(unittest.TestCase): environ={'REQUEST_METHOD': 'GET'}) resp = self.controller.GET(req) self.assertEquals(resp.status_int, 200) + self.assertEquals(resp.headers['Content-Type'], + 'application/json; charset=utf-8') def test_GET_empty_account_xml(self): req = Request.blank('/sda1/p/a?format=xml', @@ -424,6 +428,8 @@ class TestAccountController(unittest.TestCase): environ={'REQUEST_METHOD': 'GET'}) resp = self.controller.GET(req) self.assertEquals(resp.status_int, 200) + self.assertEquals(resp.headers['Content-Type'], + 'application/xml; charset=utf-8') def test_GET_over_limit(self): req = Request.blank('/sda1/p/a?limit=%d' % diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index 603e5643ad..aba79b8ac9 100644 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -5683,6 +5683,69 @@ class TestAccountController(unittest.TestCase): test_status_map((204, 500, 404), 400) +class TestAccountControllerFakeGetResponse(unittest.TestCase): + """ + Test all the faked-out GET responses for accounts that don't exist. They + have to match the responses for empty accounts that really exist. + """ + def setUp(self): + self.app = proxy_server.Application(None, FakeMemcache(), + account_ring=FakeRing(), + container_ring=FakeRing(), + object_ring=FakeRing) + self.app.memcache = FakeMemcacheReturnsNone() + self.controller = proxy_server.AccountController(self.app, 'acc') + self.controller.app.account_autocreate = True + + def test_GET_autocreate_accept_json(self): + with save_globals(): + set_http_connect(404) # however many backends we ask, they all 404 + req = Request.blank('/a', headers={'Accept': 'application/json'}) + + resp = self.controller.GET(req) + self.assertEqual(200, resp.status_int) + self.assertEqual('application/json; charset=utf-8', + resp.headers['Content-Type']) + self.assertEqual("[]", resp.body) + + def test_GET_autocreate_format_json(self): + with save_globals(): + set_http_connect(404) # however many backends we ask, they all 404 + req = Request.blank('/a?format=json') + + resp = self.controller.GET(req) + self.assertEqual(200, resp.status_int) + self.assertEqual('application/json; charset=utf-8', + resp.headers['Content-Type']) + self.assertEqual("[]", resp.body) + + def test_GET_autocreate_accept_xml(self): + with save_globals(): + set_http_connect(404) # however many backends we ask, they all 404 + req = Request.blank('/a', headers={"Accept": "text/xml"}) + + resp = self.controller.GET(req) + self.assertEqual(200, resp.status_int) + self.assertEqual('text/xml; charset=utf-8', + resp.headers['Content-Type']) + empty_xml_listing = ('\n' + '\n') + self.assertEqual(empty_xml_listing, resp.body) + + def test_GET_autocreate_format_xml(self): + with save_globals(): + set_http_connect(404) # however many backends we ask, they all 404 + req = Request.blank('/a?format=xml') + + resp = self.controller.GET(req) + self.assertEqual(200, resp.status_int) + self.assertEqual('application/xml; charset=utf-8', + resp.headers['Content-Type']) + empty_xml_listing = ('\n' + '\n') + self.assertEqual(empty_xml_listing, resp.body) + + class FakeObjectController(object): def __init__(self):