Fix metadata overall limits bug
Currently metadata limits are checked on a per request basis. If
multiple requests are sent within the per request limits, it is
possible to exceed the overall limits. This patch adds an overall
metadata check to ensure that multiple requests to add metadata to
an account/container will check overall limits before adding
the additional metadata.
This is a backport to the stable/icehouse branch for commit SHA
5b2c27a587
.
Closes-Bug: 1365350
Conflicts:
swift/common/db.py
swift/container/server.py
Change-Id: Id9fca209c9c1216f1949de7108bbe332808f1045
This commit is contained in:
parent
b223322ed1
commit
2c4622a28e
|
@ -153,7 +153,7 @@ class AccountController(object):
|
|||
for key, value in req.headers.iteritems()
|
||||
if is_sys_or_user_meta('account', key))
|
||||
if metadata:
|
||||
broker.update_metadata(metadata)
|
||||
broker.update_metadata(metadata, validate_metadata=True)
|
||||
if created:
|
||||
return HTTPCreated(request=req)
|
||||
else:
|
||||
|
@ -259,7 +259,7 @@ class AccountController(object):
|
|||
for key, value in req.headers.iteritems()
|
||||
if is_sys_or_user_meta('account', key))
|
||||
if metadata:
|
||||
broker.update_metadata(metadata)
|
||||
broker.update_metadata(metadata, validate_metadata=True)
|
||||
return HTTPNoContent(request=req)
|
||||
|
||||
def __call__(self, env, start_response):
|
||||
|
|
|
@ -92,7 +92,10 @@ FORMAT2CONTENT_TYPE = {'plain': 'text/plain', 'json': 'application/json',
|
|||
|
||||
def check_metadata(req, target_type):
|
||||
"""
|
||||
Check metadata sent in the request headers.
|
||||
Check metadata sent in the request headers. This should only check
|
||||
that the metadata in the request given is valid. Checks against
|
||||
account/container overall metadata should be forwarded on to its
|
||||
respective server to be checked.
|
||||
|
||||
:param req: request object
|
||||
:param target_type: str: one of: object, container, or account: indicates
|
||||
|
|
|
@ -31,7 +31,9 @@ import sqlite3
|
|||
|
||||
from swift.common.utils import json, normalize_timestamp, renamer, \
|
||||
mkdirs, lock_parent_directory, fallocate
|
||||
from swift.common.constraints import MAX_META_COUNT, MAX_META_OVERALL_SIZE
|
||||
from swift.common.exceptions import LockTimeout
|
||||
from swift.common.swob import HTTPBadRequest
|
||||
|
||||
|
||||
#: Whether calls will be made to preallocate disk space for database files.
|
||||
|
@ -643,7 +645,35 @@ class DatabaseBroker(object):
|
|||
metadata = {}
|
||||
return metadata
|
||||
|
||||
def update_metadata(self, metadata_updates):
|
||||
@staticmethod
|
||||
def validate_metadata(metadata):
|
||||
"""
|
||||
Validates that metadata_falls within acceptable limits.
|
||||
|
||||
:param metadata: to be validated
|
||||
:raises: HTTPBadRequest if MAX_META_COUNT or MAX_META_OVERALL_SIZE
|
||||
is exceeded
|
||||
"""
|
||||
meta_count = 0
|
||||
meta_size = 0
|
||||
for key, (value, timestamp) in metadata.iteritems():
|
||||
key = key.lower()
|
||||
if value != '' and (key.startswith('x-account-meta') or
|
||||
key.startswith('x-container-meta')):
|
||||
prefix = 'x-account-meta-'
|
||||
if key.startswith('x-container-meta-'):
|
||||
prefix = 'x-container-meta-'
|
||||
key = key[len(prefix):]
|
||||
meta_count = meta_count + 1
|
||||
meta_size = meta_size + len(key) + len(value)
|
||||
if meta_count > MAX_META_COUNT:
|
||||
raise HTTPBadRequest('Too many metadata items; max %d'
|
||||
% MAX_META_COUNT)
|
||||
if meta_size > MAX_META_OVERALL_SIZE:
|
||||
raise HTTPBadRequest('Total metadata too large; max %d'
|
||||
% MAX_META_OVERALL_SIZE)
|
||||
|
||||
def update_metadata(self, metadata_updates, validate_metadata=False):
|
||||
"""
|
||||
Updates the metadata dict for the database. The metadata dict values
|
||||
are tuples of (value, timestamp) where the timestamp indicates when
|
||||
|
@ -676,6 +706,8 @@ class DatabaseBroker(object):
|
|||
value, timestamp = value_timestamp
|
||||
if key not in md or timestamp > md[key][1]:
|
||||
md[key] = value_timestamp
|
||||
if validate_metadata:
|
||||
DatabaseBroker.validate_metadata(md)
|
||||
conn.execute('UPDATE %s_stat SET metadata = ?' % self.db_type,
|
||||
(json.dumps(md),))
|
||||
conn.commit()
|
||||
|
|
|
@ -286,7 +286,7 @@ class ContainerController(object):
|
|||
metadata['X-Container-Sync-To'][0] != \
|
||||
broker.metadata['X-Container-Sync-To'][0]:
|
||||
broker.set_x_container_sync_points(-1, -1)
|
||||
broker.update_metadata(metadata)
|
||||
broker.update_metadata(metadata, validate_metadata=True)
|
||||
resp = self.account_update(req, account, container, broker)
|
||||
if resp:
|
||||
return resp
|
||||
|
@ -473,7 +473,7 @@ class ContainerController(object):
|
|||
metadata['X-Container-Sync-To'][0] != \
|
||||
broker.metadata['X-Container-Sync-To'][0]:
|
||||
broker.set_x_container_sync_points(-1, -1)
|
||||
broker.update_metadata(metadata)
|
||||
broker.update_metadata(metadata, validate_metadata=True)
|
||||
return HTTPNoContent(request=req)
|
||||
|
||||
def __call__(self, env, start_response):
|
||||
|
|
|
@ -32,6 +32,42 @@ from test.functional.tests import load_constraint
|
|||
|
||||
class TestAccount(unittest.TestCase):
|
||||
|
||||
def setUp(self):
|
||||
self.max_meta_count = load_constraint('max_meta_count')
|
||||
self.max_meta_name_length = load_constraint('max_meta_name_length')
|
||||
self.max_meta_overall_size = load_constraint('max_meta_overall_size')
|
||||
self.max_meta_value_length = load_constraint('max_meta_value_length')
|
||||
|
||||
def head(url, token, parsed, conn):
|
||||
conn.request('HEAD', parsed.path, '', {'X-Auth-Token': token})
|
||||
return check_response(conn)
|
||||
resp = retry(head)
|
||||
self.existing_metadata = set([
|
||||
k for k, v in resp.getheaders() if
|
||||
k.lower().startswith('x-account-meta')])
|
||||
|
||||
def tearDown(self):
|
||||
def head(url, token, parsed, conn):
|
||||
conn.request('HEAD', parsed.path, '', {'X-Auth-Token': token})
|
||||
return check_response(conn)
|
||||
resp = retry(head)
|
||||
resp.read()
|
||||
new_metadata = set(
|
||||
[k for k, v in resp.getheaders() if
|
||||
k.lower().startswith('x-account-meta')])
|
||||
|
||||
def clear_meta(url, token, parsed, conn, remove_metadata_keys):
|
||||
headers = {'X-Auth-Token': token}
|
||||
headers.update((k, '') for k in remove_metadata_keys)
|
||||
conn.request('POST', parsed.path, '', headers)
|
||||
return check_response(conn)
|
||||
extra_metadata = list(self.existing_metadata ^ new_metadata)
|
||||
for i in range(0, len(extra_metadata), 90):
|
||||
batch = extra_metadata[i:i + 90]
|
||||
resp = retry(clear_meta, batch)
|
||||
resp.read()
|
||||
self.assertEqual(resp.status // 100, 2)
|
||||
|
||||
def test_metadata(self):
|
||||
if skip:
|
||||
raise SkipTest
|
||||
|
@ -733,6 +769,21 @@ class TestAccount(unittest.TestCase):
|
|||
resp.read()
|
||||
self.assertEqual(resp.status, 400)
|
||||
|
||||
def test_bad_metadata2(self):
|
||||
if skip:
|
||||
raise SkipTest
|
||||
|
||||
def post(url, token, parsed, conn, extra_headers):
|
||||
headers = {'X-Auth-Token': token}
|
||||
headers.update(extra_headers)
|
||||
conn.request('POST', parsed.path, '', headers)
|
||||
return check_response(conn)
|
||||
|
||||
# TODO: Find the test that adds these and remove them.
|
||||
headers = {'x-remove-account-meta-temp-url-key': 'remove',
|
||||
'x-remove-account-meta-temp-url-key-2': 'remove'}
|
||||
resp = retry(post, headers)
|
||||
|
||||
headers = {}
|
||||
for x in xrange(MAX_META_COUNT):
|
||||
headers['X-Account-Meta-%d' % x] = 'v'
|
||||
|
@ -746,6 +797,21 @@ class TestAccount(unittest.TestCase):
|
|||
resp.read()
|
||||
self.assertEqual(resp.status, 400)
|
||||
|
||||
def test_bad_metadata3(self):
|
||||
if skip:
|
||||
raise SkipTest
|
||||
|
||||
def post(url, token, parsed, conn, extra_headers):
|
||||
headers = {'X-Auth-Token': token}
|
||||
headers.update(extra_headers)
|
||||
conn.request('POST', parsed.path, '', headers)
|
||||
return check_response(conn)
|
||||
|
||||
# TODO: Find the test that adds these and remove them.
|
||||
headers = {'x-remove-account-meta-temp-url-key': 'remove',
|
||||
'x-remove-account-meta-temp-url-key-2': 'remove'}
|
||||
resp = retry(post, headers)
|
||||
|
||||
headers = {}
|
||||
header_value = 'k' * MAX_META_VALUE_LENGTH
|
||||
size = 0
|
||||
|
|
|
@ -382,6 +382,16 @@ class TestContainer(unittest.TestCase):
|
|||
resp.read()
|
||||
self.assertEqual(resp.status, 400)
|
||||
|
||||
def test_POST_bad_metadata2(self):
|
||||
if skip:
|
||||
raise SkipTest
|
||||
|
||||
def post(url, token, parsed, conn, extra_headers):
|
||||
headers = {'X-Auth-Token': token}
|
||||
headers.update(extra_headers)
|
||||
conn.request('POST', parsed.path + '/' + self.name, '', headers)
|
||||
return check_response(conn)
|
||||
|
||||
headers = {}
|
||||
for x in xrange(MAX_META_COUNT):
|
||||
headers['X-Container-Meta-%d' % x] = 'v'
|
||||
|
@ -395,6 +405,16 @@ class TestContainer(unittest.TestCase):
|
|||
resp.read()
|
||||
self.assertEqual(resp.status, 400)
|
||||
|
||||
def test_POST_bad_metadata3(self):
|
||||
if skip:
|
||||
raise SkipTest
|
||||
|
||||
def post(url, token, parsed, conn, extra_headers):
|
||||
headers = {'X-Auth-Token': token}
|
||||
headers.update(extra_headers)
|
||||
conn.request('POST', parsed.path + '/' + self.name, '', headers)
|
||||
return check_response(conn)
|
||||
|
||||
headers = {}
|
||||
header_value = 'k' * MAX_META_VALUE_LENGTH
|
||||
size = 0
|
||||
|
|
|
@ -28,11 +28,14 @@ from mock import patch, MagicMock
|
|||
from eventlet.timeout import Timeout
|
||||
|
||||
import swift.common.db
|
||||
from swift.common.constraints import \
|
||||
MAX_META_VALUE_LENGTH, MAX_META_COUNT, MAX_META_OVERALL_SIZE
|
||||
from swift.common.db import chexor, dict_factory, get_db_connection, \
|
||||
DatabaseBroker, DatabaseConnectionError, DatabaseAlreadyExists, \
|
||||
GreenDBConnection
|
||||
from swift.common.utils import normalize_timestamp, mkdirs
|
||||
from swift.common.exceptions import LockTimeout
|
||||
from swift.common.swob import HTTPException
|
||||
|
||||
|
||||
class TestDatabaseConnectionError(unittest.TestCase):
|
||||
|
@ -230,7 +233,7 @@ class TestDatabaseBroker(unittest.TestCase):
|
|||
conn.execute('CREATE TABLE test (one TEXT)')
|
||||
conn.execute('CREATE TABLE test_stat (id TEXT)')
|
||||
conn.execute('INSERT INTO test_stat (id) VALUES (?)',
|
||||
(str(uuid4),))
|
||||
(str(uuid4),))
|
||||
conn.execute('INSERT INTO test (one) VALUES ("1")')
|
||||
conn.commit()
|
||||
stub_called = [False]
|
||||
|
@ -679,6 +682,91 @@ class TestDatabaseBroker(unittest.TestCase):
|
|||
[first_value, first_timestamp])
|
||||
self.assert_('Second' not in broker.metadata)
|
||||
|
||||
@patch.object(DatabaseBroker, 'validate_metadata')
|
||||
def test_validate_metadata_is_called_from_update_metadata(self, mock):
|
||||
broker = self.get_replication_info_tester(metadata=True)
|
||||
first_timestamp = normalize_timestamp(1)
|
||||
first_value = '1'
|
||||
metadata = {'First': [first_value, first_timestamp]}
|
||||
broker.update_metadata(metadata, validate_metadata=True)
|
||||
self.assertTrue(mock.called)
|
||||
|
||||
@patch.object(DatabaseBroker, 'validate_metadata')
|
||||
def test_validate_metadata_is_not_called_from_update_metadata(self, mock):
|
||||
broker = self.get_replication_info_tester(metadata=True)
|
||||
first_timestamp = normalize_timestamp(1)
|
||||
first_value = '1'
|
||||
metadata = {'First': [first_value, first_timestamp]}
|
||||
broker.update_metadata(metadata)
|
||||
self.assertFalse(mock.called)
|
||||
|
||||
def test_metadata_with_max_count(self):
|
||||
metadata = {}
|
||||
for c in xrange(MAX_META_COUNT):
|
||||
key = 'X-Account-Meta-F{0}'.format(c)
|
||||
metadata[key] = ('B', normalize_timestamp(1))
|
||||
key = 'X-Account-Meta-Foo'.format(c)
|
||||
metadata[key] = ('', normalize_timestamp(1))
|
||||
try:
|
||||
DatabaseBroker.validate_metadata(metadata)
|
||||
except HTTPException:
|
||||
self.fail('Unexpected HTTPException')
|
||||
|
||||
def test_metadata_raises_exception_over_max_count(self):
|
||||
metadata = {}
|
||||
for c in xrange(MAX_META_COUNT + 1):
|
||||
key = 'X-Account-Meta-F{0}'.format(c)
|
||||
metadata[key] = ('B', normalize_timestamp(1))
|
||||
message = ''
|
||||
try:
|
||||
DatabaseBroker.validate_metadata(metadata)
|
||||
except HTTPException as e:
|
||||
message = str(e)
|
||||
self.assertEqual(message, '400 Bad Request')
|
||||
|
||||
def test_metadata_with_max_overall_size(self):
|
||||
metadata = {}
|
||||
metadata_value = 'v' * MAX_META_VALUE_LENGTH
|
||||
size = 0
|
||||
x = 0
|
||||
while size < (MAX_META_OVERALL_SIZE - 4
|
||||
- MAX_META_VALUE_LENGTH):
|
||||
size += 4 + MAX_META_VALUE_LENGTH
|
||||
metadata['X-Account-Meta-%04d' % x] = (metadata_value,
|
||||
normalize_timestamp(1))
|
||||
x += 1
|
||||
if MAX_META_OVERALL_SIZE - size > 1:
|
||||
metadata['X-Account-Meta-k'] = (
|
||||
'v' * (MAX_META_OVERALL_SIZE - size - 1),
|
||||
normalize_timestamp(1))
|
||||
try:
|
||||
DatabaseBroker.validate_metadata(metadata)
|
||||
except HTTPException:
|
||||
self.fail('Unexpected HTTPException')
|
||||
|
||||
def test_metadata_raises_exception_over_max_overall_size(self):
|
||||
metadata = {}
|
||||
metadata_value = 'k' * MAX_META_VALUE_LENGTH
|
||||
size = 0
|
||||
x = 0
|
||||
while size < (MAX_META_OVERALL_SIZE - 4
|
||||
- MAX_META_VALUE_LENGTH):
|
||||
size += 4 + MAX_META_VALUE_LENGTH
|
||||
metadata['X-Account-Meta-%04d' % x] = (metadata_value,
|
||||
normalize_timestamp(1))
|
||||
x += 1
|
||||
if MAX_META_OVERALL_SIZE - size > 1:
|
||||
metadata['X-Account-Meta-k'] = (
|
||||
'v' * (MAX_META_OVERALL_SIZE - size - 1),
|
||||
normalize_timestamp(1))
|
||||
metadata['X-Account-Meta-k2'] = ('v', normalize_timestamp(1))
|
||||
message = ''
|
||||
try:
|
||||
DatabaseBroker.validate_metadata(metadata)
|
||||
except HTTPException as e:
|
||||
message = str(e)
|
||||
self.assertEqual(message, '400 Bad Request')
|
||||
|
||||
|
||||
if __name__ == '__main__':
|
||||
unittest.main()
|
||||
|
|
Loading…
Reference in New Issue