From ca85ee4ad6f84b77d95b7ba18c459165078bf2d9 Mon Sep 17 00:00:00 2001 From: Rajesh Tailor Date: Thu, 9 Feb 2023 17:41:04 +0530 Subject: [PATCH] Fix case-sensitivity for metadata keys As of now, if aggregate already has metadata keys 'abc' and user tries to update this using upper case 'ABC' or mixed case ('AbC','Abc' etc), then user gets KeyError exception. This change catch KeyError and raises AggregateMetadataKeyExists exception with 400 error code, when user tries to update case-sensitive metadata key. Closes-Bug: #1538011 Change-Id: Icc418c20e14efa08b356d938839e430223836f80 Signed-off-by: Melanie Witt --- nova/api/openstack/compute/aggregates.py | 3 ++- nova/exception.py | 6 +++++ nova/objects/aggregate.py | 11 +++++++-- .../api/openstack/compute/test_aggregates.py | 13 ++++++++++ nova/tests/unit/objects/test_aggregate.py | 24 +++++++++++++++++++ 5 files changed, 54 insertions(+), 3 deletions(-) diff --git a/nova/api/openstack/compute/aggregates.py b/nova/api/openstack/compute/aggregates.py index bf2ea7f0bf..930d65ea87 100644 --- a/nova/api/openstack/compute/aggregates.py +++ b/nova/api/openstack/compute/aggregates.py @@ -243,7 +243,8 @@ class AggregateController(wsgi.Controller): id, metadata) except exception.AggregateNotFound as e: raise exc.HTTPNotFound(explanation=e.format_message()) - except exception.InvalidAggregateAction as e: + except (exception.InvalidAggregateAction, + exception.AggregateMetadataKeyExists) as e: raise exc.HTTPBadRequest(explanation=e.format_message()) return self._marshall_aggregate(req, aggregate) diff --git a/nova/exception.py b/nova/exception.py index e59b8e3feb..692047368e 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -465,6 +465,12 @@ class InvalidAggregateActionUpdateMeta(InvalidAggregateAction): "%(aggregate_id)s. Reason: %(reason)s.") +class AggregateMetadataKeyExists(NovaException): + msg_fmt = _("Aggregate %(aggregate_id)s already contain metadata " + "key %(key)s.") + code = 400 + + class InvalidSortKey(Invalid): msg_fmt = _("Sort key supplied was not valid.") diff --git a/nova/objects/aggregate.py b/nova/objects/aggregate.py index 011b990f9e..48286c588f 100644 --- a/nova/objects/aggregate.py +++ b/nova/objects/aggregate.py @@ -113,8 +113,15 @@ def _metadata_add_to_db(context, aggregate_id, metadata, max_retries=10, api_models.AggregateMetadata.key.in_(all_keys)) for meta_ref in query.all(): key = meta_ref.key - meta_ref.update({"value": metadata[key]}) - already_existing_keys.add(key) + try: + meta_ref.update({"value": metadata[key]}) + already_existing_keys.add(key) + except KeyError: + # NOTE(ratailor): When user tries updating + # metadata using case-sensitive key, we get + # KeyError. + raise exception.AggregateMetadataKeyExists( + aggregate_id=aggregate_id, key=key) new_entries = [] for key, value in metadata.items(): diff --git a/nova/tests/unit/api/openstack/compute/test_aggregates.py b/nova/tests/unit/api/openstack/compute/test_aggregates.py index 891388590d..d45bdf1cb6 100644 --- a/nova/tests/unit/api/openstack/compute/test_aggregates.py +++ b/nova/tests/unit/api/openstack/compute/test_aggregates.py @@ -669,6 +669,19 @@ class AggregateTestCaseV21(test.NoDBTestCase): self.assertRaises(self.bad_request, eval(self.set_metadata), self.req, "1", body=body) + def test_set_metadata_case_sensitive(self): + body = {"set_metadata": {"metadata": {"foo": "bar"}}} + side_effect = exception.AggregateMetadataKeyExists( + aggregate_id="2", key="foo") + + with mock.patch.object(self.controller.api, + 'update_aggregate_metadata', + side_effect=side_effect) as mock_update: + self.assertRaises(exc.HTTPBadRequest, eval(self.set_metadata), + self.req, "2", body=body) + mock_update.assert_called_once_with( + self.context, "2", body["set_metadata"]["metadata"]) + def test_delete_aggregate(self): with mock.patch.object(self.controller.api, 'delete_aggregate') as mock_del: diff --git a/nova/tests/unit/objects/test_aggregate.py b/nova/tests/unit/objects/test_aggregate.py index 3f01c9613d..726bff5c16 100644 --- a/nova/tests/unit/objects/test_aggregate.py +++ b/nova/tests/unit/objects/test_aggregate.py @@ -175,6 +175,30 @@ class _TestAggregateObject(object): 123, {'toadd': 'myval'}) + @mock.patch('nova.compute.utils.notify_about_aggregate_action') + def test_update_metadata_api_case_sensitive(self, mock_notify): + # Mock context.session.query().filter_by().filter().all() + fake_context = mock.Mock() + mock_query = mock.Mock() + fake_context.session.query.return_value = mock_query + mock_query = mock_query.filter_by.return_value.filter.return_value + + # Return a fake meta_ref that raises a KeyError + fake_meta_ref = mock.Mock() + fake_meta_ref.update.side_effect = KeyError + mock_query.all.return_value = [fake_meta_ref] + + # Create an aggregate in the database + agg = aggregate.Aggregate( + context=self.context, name='agg', metadata={'Abc': 'bar'}) + agg.create() + + # Then replace its context with mock context + agg._context = fake_context + self.assertRaises(exception.AggregateMetadataKeyExists, + agg.update_metadata, + {'abC': 'barbar'}) + @mock.patch('nova.objects.aggregate._host_add_to_db') def test_add_host_api(self, mock_host_add_api): mock_host_add_api.return_value = {'host': 'bar'}