Merge "Fix metadata overall limits bug"

swift/account/server.py

@@ -156,7 +156,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:
@@ -249,7 +249,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):

swift/common/constraints.py

@@ -101,7 +101,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

swift/common/db.py

@@ -30,9 +30,11 @@ from tempfile import mkstemp
 from eventlet import sleep, Timeout
 import sqlite3
 
+from swift.common.constraints import MAX_META_COUNT, MAX_META_OVERALL_SIZE
 from swift.common.utils import json, Timestamp, renamer, \
     mkdirs, lock_parent_directory, fallocate
 from swift.common.exceptions import LockTimeout
+from swift.common.swob import HTTPBadRequest
 
 
 #: Whether calls will be made to preallocate disk space for database files.
@@ -719,7 +721,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
@@ -752,6 +782,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()

swift/container/server.py

@@ -374,7 +374,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
@@ -551,7 +551,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):

test/functional/test_account.py

@@ -774,6 +774,21 @@ class TestAccount(unittest.TestCase):
         resp.read()
         self.assertEqual(resp.status, 400)
 
+    def test_bad_metadata2(self):
+        if tf.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(self.max_meta_count):
             headers['X-Account-Meta-%d' % x] = 'v'
@@ -787,6 +802,16 @@ class TestAccount(unittest.TestCase):
         resp.read()
         self.assertEqual(resp.status, 400)
 
+    def test_bad_metadata3(self):
+        if tf.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)
+
         headers = {}
         header_value = 'k' * self.max_meta_value_length
         size = 0

test/functional/test_container.py

@@ -401,6 +401,16 @@ class TestContainer(unittest.TestCase):
         resp.read()
         self.assertEqual(resp.status, 400)
 
+    def test_POST_bad_metadata2(self):
+        if tf.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(self.max_meta_count):
             headers['X-Container-Meta-%d' % x] = 'v'
@@ -414,6 +424,16 @@ class TestContainer(unittest.TestCase):
         resp.read()
         self.assertEqual(resp.status, 400)
 
+    def test_POST_bad_metadata3(self):
+        if tf.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' * self.max_meta_value_length
         size = 0

test/unit/common/test_db.py

@@ -32,11 +32,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, PICKLE_PROTOCOL
 from swift.common.utils import normalize_timestamp, mkdirs, json, Timestamp
 from swift.common.exceptions import LockTimeout
+from swift.common.swob import HTTPException
 
 from test.unit import with_tempdir
 
@@ -655,7 +658,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]
@@ -1107,6 +1110,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()