From f379b96dd2a301d1cfa70562454ec957d8341125 Mon Sep 17 00:00:00 2001 From: Igor Degtiarov Date: Mon, 1 Dec 2014 15:51:25 +0200 Subject: [PATCH] [MongoDB] Fix bug with 'bad' chars in metadatas keys MongoDB has two restrictions on fields names, they cannot contain '.' and start with '$'. In resource_metadata field we can get any data, so it could be dict with any keys, and if some key contain i.e. dots sample with this metadata wouldn't be stored in MongoDB. To protect samples from missing this patch propose to check metadata for occurrence of dots and '$' in keys if metadata is dict, and to transform dict with dots before storing it in MongoDB. Transformation means that we make dict more nested and new node appears where there is dot in dict key. It means that if we get metadata as a dict {'a.b.c': v} before storing in MongoDB it would be transform in {'a': {'b': {'c': v}}}. Closes-Bug: 1246264 Change-Id: I279ae67cd24d4dadd7d7e58007e7c5c297d91772 --- ceilometer/storage/impl_db2.py | 8 +- ceilometer/storage/impl_mongodb.py | 9 ++- ceilometer/storage/mongo/utils.py | 76 ++++++++++++++++++- ceilometer/storage/pymongo_base.py | 7 +- .../api/v2/test_list_meters_scenarios.py | 37 +++++++-- .../api/v2/test_list_resources_scenarios.py | 4 +- .../tests/storage/test_storage_scenarios.py | 52 +++++++++++-- 7 files changed, 170 insertions(+), 23 deletions(-) diff --git a/ceilometer/storage/impl_db2.py b/ceilometer/storage/impl_db2.py index 955db0e528..fd2e343a08 100644 --- a/ceilometer/storage/impl_db2.py +++ b/ceilometer/storage/impl_db2.py @@ -208,6 +208,9 @@ class Connection(pymongo_base.Connection): ceilometer.meter.meter_message_from_counter """ # Record the updated resource metadata + data = copy.deepcopy(data) + data['resource_metadata'] = pymongo_utils.improve_keys( + data.pop('resource_metadata')) self.db.resource.update( {'_id': data['resource_id']}, {'$set': {'project_id': data['project_id'], @@ -255,7 +258,7 @@ class Connection(pymongo_base.Connection): if pagination: raise ceilometer.NotImplementedError('Pagination not implemented') - metaquery = metaquery or {} + metaquery = pymongo_utils.improve_keys(metaquery, metaquery=True) or {} q = {} if user is not None: @@ -302,7 +305,8 @@ class Connection(pymongo_base.Connection): last_sample_timestamp=last_ts, source=latest_meter['source'], user_id=latest_meter['user_id'], - metadata=latest_meter['resource_metadata']) + metadata=pymongo_utils.unquote_keys( + latest_meter['resource_metadata'])) def get_meter_statistics(self, sample_filter, period=None, groupby=None, aggregate=None): diff --git a/ceilometer/storage/impl_mongodb.py b/ceilometer/storage/impl_mongodb.py index 278e52e8ec..73a45e7738 100644 --- a/ceilometer/storage/impl_mongodb.py +++ b/ceilometer/storage/impl_mongodb.py @@ -483,6 +483,9 @@ class Connection(pymongo_base.Connection): # unconditionally insert sample timestamps and resource metadata # (in the update case, this must be conditional on the sample not # being out-of-order) + data = copy.deepcopy(data) + data['resource_metadata'] = pymongo_utils.improve_keys( + data.pop('resource_metadata')) resource = self.db.resource.find_and_modify( {'_id': data['resource_id']}, {'$set': {'project_id': data['project_id'], @@ -729,7 +732,7 @@ class Connection(pymongo_base.Connection): first_sample_timestamp=resource['first_timestamp'], last_sample_timestamp=resource['last_timestamp'], source=resource['source'], - metadata=resource['metadata']) + metadata=pymongo_utils.unquote_keys(resource['metadata'])) finally: self.db[out].drop() @@ -762,7 +765,7 @@ class Connection(pymongo_base.Connection): last_sample_timestamp=r.get('last_sample_timestamp', self._APOCALYPSE), source=r['source'], - metadata=r['metadata']) + metadata=pymongo_utils.unquote_keys(r['metadata'])) def get_resources(self, user=None, project=None, source=None, start_timestamp=None, start_timestamp_op=None, @@ -784,7 +787,7 @@ class Connection(pymongo_base.Connection): if pagination: raise ceilometer.NotImplementedError('Pagination not implemented') - metaquery = metaquery or {} + metaquery = pymongo_utils.improve_keys(metaquery, metaquery=True) or {} query = {} if user is not None: diff --git a/ceilometer/storage/mongo/utils.py b/ceilometer/storage/mongo/utils.py index c5f5506ccf..40e1eb216b 100644 --- a/ceilometer/storage/mongo/utils.py +++ b/ceilometer/storage/mongo/utils.py @@ -25,6 +25,7 @@ from oslo.config import cfg from oslo.utils import netutils import pymongo import six +from six.moves.urllib import parse from ceilometer.openstack.common.gettextutils import _ from ceilometer.openstack.common import log @@ -146,11 +147,82 @@ def make_query_from_filter(sample_filter, require_meter=True): # so the samples call metadata resource_metadata, so we convert # to that. - q.update(dict(('resource_%s' % k, v) - for (k, v) in six.iteritems(sample_filter.metaquery))) + q.update(dict( + ('resource_%s' % k, v) for (k, v) in six.iteritems( + improve_keys(sample_filter.metaquery, metaquery=True)))) return q +def quote_key(key, reverse=False): + """Prepare key for storage data in MongoDB. + + :param key: key that should be quoted + :param reverse: boolean, True --- if we need a reverse order of the keys + parts + :return: iter of quoted part of the key + """ + r = -1 if reverse else 1 + + for k in key.split('.')[::r]: + if k.startswith('$'): + k = parse.quote(k) + yield k + + +def improve_keys(data, metaquery=False): + """Improves keys in dict if they contained '.' or started with '$'. + + :param data: is a dictionary where keys need to be checked and improved + :param metaquery: boolean, if True dots are not escaped from the keys + :return: improved dictionary if keys contained dots or started with '$': + {'a.b': 'v'} -> {'a': {'b': 'v'}} + {'$ab': 'v'} -> {'%24ab': 'v'} + """ + if not isinstance(data, dict): + return data + + if metaquery: + for key in data.iterkeys(): + if '.$' in key: + key_list = [] + for k in quote_key(key): + key_list.append(k) + new_key = '.'.join(key_list) + data[new_key] = data.pop(key) + else: + for key, value in data.items(): + if isinstance(value, dict): + improve_keys(value) + if '.' in key: + new_dict = {} + for k in quote_key(key, reverse=True): + new = {} + new[k] = new_dict if new_dict else data.pop(key) + new_dict = new + data.update(new_dict) + else: + if key.startswith('$'): + new_key = parse.quote(key) + data[new_key] = data.pop(key) + return data + + +def unquote_keys(data): + """Restores initial view of 'quoted' keys in dictionary data + + :param data: is a dictionary + :return: data with restored keys if they were 'quoted'. + """ + if isinstance(data, dict): + for key, value in data.items(): + if isinstance(value, dict): + unquote_keys(value) + if key.startswith('%24'): + k = parse.unquote(key) + data[k] = data.pop(key) + return data + + class ConnectionPool(object): def __init__(self): diff --git a/ceilometer/storage/pymongo_base.py b/ceilometer/storage/pymongo_base.py index 32613e5f3a..3543327a7b 100644 --- a/ceilometer/storage/pymongo_base.py +++ b/ceilometer/storage/pymongo_base.py @@ -65,7 +65,7 @@ class Connection(base.Connection): if pagination: raise ceilometer.NotImplementedError('Pagination not implemented') - metaquery = metaquery or {} + metaquery = pymongo_utils.improve_keys(metaquery, metaquery=True) or {} q = {} if user is not None: @@ -138,4 +138,9 @@ class Connection(base.Connection): s['counter_unit'] = s.get('counter_unit', '') # Tolerate absence of recorded_at in older datapoints s['recorded_at'] = s.get('recorded_at') + # Check samples for metadata and "unquote" key if initially it + # was started with '$'. + if s.get('resource_metadata'): + s['resource_metadata'] = pymongo_utils.unquote_keys( + s.get('resource_metadata')) yield models.Sample(**s) diff --git a/ceilometer/tests/api/v2/test_list_meters_scenarios.py b/ceilometer/tests/api/v2/test_list_meters_scenarios.py index b72cafe6ee..e202efaccc 100644 --- a/ceilometer/tests/api/v2/test_list_meters_scenarios.py +++ b/ceilometer/tests/api/v2/test_list_meters_scenarios.py @@ -159,7 +159,9 @@ class TestListMeters(v2.FunctionalTest, 'properties': { 'prop_1': 'prop_value', 'prop_2': {'sub_prop_1': - 'sub_prop_value'} + 'sub_prop_value'}, + 'prop.3': {'$sub_prop.2': + 'sub_prop_value2'} }, 'size': 0, 'util': 0.58, @@ -298,6 +300,22 @@ class TestListMeters(v2.FunctionalTest, self.assertEqual('self.sample4', metadata['tag']) self.assertEqual('prop_value', metadata['properties.prop_1']) + def test_list_meters_with_dict_metadata_with_dot_dollar_in_key(self): + data = self.get_json('/meters/meter.mine', + q=[{'field': + 'metadata.properties.prop.3.$sub_prop.2', + 'op': 'eq', + 'value': 'sub_prop_value2', + }]) + self.assertEqual(1, len(data)) + self.assertEqual('resource-id4', data[0]['resource_id']) + metadata = data[0]['resource_metadata'] + self.assertIsNotNone(metadata) + self.assertEqual('self.sample4', metadata['tag']) + self.assertEqual('prop_value', metadata['properties.prop_1']) + self.assertEqual('sub_prop_value', + metadata['properties.prop_2:sub_prop_1']) + def test_get_one_sample(self): sample_id = self.messages[1]['message_id'] data = self.get_json('/samples/%s' % sample_id) @@ -347,13 +365,16 @@ class TestListMeters(v2.FunctionalTest, u'type': u'gauge', u'unit': u'', u'source': u'test_source1', - u'metadata': {u'display_name': u'test-server', - u'properties.prop_2:sub_prop_1': u'sub_prop_value', - u'util': u'0.58', - u'tag': u'self.sample4', - u'properties.prop_1': u'prop_value', - u'is_public': u'True', - u'size': u'0'} + u'metadata': { + u'display_name': u'test-server', + u'properties.prop_2:sub_prop_1': u'sub_prop_value', + u'util': u'0.58', + u'tag': u'self.sample4', + u'properties.prop_1': u'prop_value', + u'is_public': u'True', + u'size': u'0', + u'properties.prop:3:$sub_prop:2': u'sub_prop_value2', + } }], data) def test_list_meters_metadata_query(self): diff --git a/ceilometer/tests/api/v2/test_list_resources_scenarios.py b/ceilometer/tests/api/v2/test_list_resources_scenarios.py index 22a1ec5f5d..d5e928715a 100644 --- a/ceilometer/tests/api/v2/test_list_resources_scenarios.py +++ b/ceilometer/tests/api/v2/test_list_resources_scenarios.py @@ -461,7 +461,7 @@ class TestListResources(v2.FunctionalTest, timestamp=datetime.datetime(2012, 7, 2, 10, 40), resource_metadata={'display_name': 'test-server', 'tag': 'self.sample', - 'dict_properties': {'key': 'value'}, + 'dict_properties': {'key.$1': {'$key': 'val'}}, 'not_ignored_list': ['returned'], }, source='test', @@ -474,7 +474,7 @@ class TestListResources(v2.FunctionalTest, data = self.get_json('/resources') metadata = data[0]['metadata'] - self.assertEqual([(u'dict_properties.key', u'value'), + self.assertEqual([(u'dict_properties.key:$1:$key', u'val'), (u'display_name', u'test-server'), (u'not_ignored_list', u"['returned']"), (u'tag', u'self.sample')], diff --git a/ceilometer/tests/storage/test_storage_scenarios.py b/ceilometer/tests/storage/test_storage_scenarios.py index 645cf0721b..756a5fd478 100644 --- a/ceilometer/tests/storage/test_storage_scenarios.py +++ b/ceilometer/tests/storage/test_storage_scenarios.py @@ -126,13 +126,26 @@ class DBTestBase(tests_db.TestBase): class ResourceTest(DBTestBase, tests_db.MixinTestsWithBackendScenarios): + def prepare_data(self): + super(ResourceTest, self).prepare_data() + + self.msgs.append(self.create_and_store_sample( + timestamp=datetime.datetime(2012, 7, 2, 10, 39), + user_id='mongodb_test', + resource_id='resource-id-mongo_bad_key', + project_id='project-id-test', + metadata={'display.name': {'name.$1': 'test-server1', + '$name_2': 'test-server2'}, + 'tag': 'self.counter'}, + source='test-4' + )) def test_get_resources(self): expected_first_sample_timestamp = datetime.datetime(2012, 7, 2, 10, 39) expected_last_sample_timestamp = datetime.datetime(2012, 7, 2, 10, 40) msgs_sources = [msg['source'] for msg in self.msgs] resources = list(self.conn.get_resources()) - self.assertEqual(9, len(resources)) + self.assertEqual(10, len(resources)) for resource in resources: if resource.resource_id != 'resource-id': continue @@ -172,7 +185,8 @@ class ResourceTest(DBTestBase, def test_get_resources_end_timestamp(self): timestamp = datetime.datetime(2012, 7, 2, 10, 42) expected = set(['resource-id', 'resource-id-alternate', - 'resource-id-5', 'resource-id-7']) + 'resource-id-5', 'resource-id-7', + 'resource-id-mongo_bad_key']) resources = list(self.conn.get_resources(end_timestamp=timestamp)) resource_ids = [r.resource_id for r in resources] @@ -262,9 +276,15 @@ class ResourceTest(DBTestBase, resources = list(self.conn.get_resources(metaquery=q)) self.assertEqual(9, len(resources)) + def test_get_resources_by_metaquery_key_with_dot_in_metadata(self): + q = {'metadata.display.name.$name_2': 'test-server2', + 'metadata.display.name.name.$1': 'test-server1'} + resources = list(self.conn.get_resources(metaquery=q)) + self.assertEqual(1, len(resources)) + def test_get_resources_by_empty_metaquery(self): resources = list(self.conn.get_resources(metaquery={})) - self.assertEqual(9, len(resources)) + self.assertEqual(10, len(resources)) def test_get_resources_most_recent_metadata_all(self): resources = self.conn.get_resources() @@ -459,6 +479,20 @@ class MeterTestPagination(DBTestBase, class RawSampleTest(DBTestBase, tests_db.MixinTestsWithBackendScenarios): + def prepare_data(self): + super(RawSampleTest, self).prepare_data() + + self.msgs.append(self.create_and_store_sample( + timestamp=datetime.datetime(2012, 7, 2, 10, 39), + user_id='mongodb_test', + resource_id='resource-id-mongo_bad_key', + project_id='project-id-test', + metadata={'display.name': {'name.$1': 'test-server1', + '$name_2': 'test-server2'}, + 'tag': 'self.counter'}, + source='test-4' + )) + def test_get_samples_limit_zero(self): f = storage.SampleFilter() results = list(self.conn.get_samples(f, limit=0)) @@ -530,6 +564,14 @@ class RawSampleTest(DBTestBase, del d['recorded_at'] self.assertIn(d, self.msgs) + def test_get_samples_by_metaquery_key_with_dot_in_metadata(self): + q = {'metadata.display.name.name.$1': 'test-server1', + 'metadata.display.name.$name_2': 'test-server2'} + f = storage.SampleFilter(metaquery=q) + results = list(self.conn.get_samples(f)) + self.assertIsNotNone(results) + self.assertEqual(1, len(results)) + def test_get_samples_by_start_time(self): timestamp = datetime.datetime(2012, 7, 2, 10, 41) f = storage.SampleFilter( @@ -643,9 +685,9 @@ class RawSampleTest(DBTestBase, self.conn.clear_expired_metering_data(3 * 60) f = storage.SampleFilter(meter='instance') results = list(self.conn.get_samples(f)) - self.assertEqual(11, len(results)) + self.assertEqual(12, len(results)) results = list(self.conn.get_resources()) - self.assertEqual(9, len(results)) + self.assertEqual(10, len(results)) @tests_db.run_with('sqlite', 'hbase', 'db2') def test_clear_metering_data_with_alarms(self):