diff options
author | Zuul <zuul@review.opendev.org> | 2020-11-28 07:36:40 +0000 |
---|---|---|
committer | Gerrit Code Review <review@openstack.org> | 2020-11-28 07:36:40 +0000 |
commit | b96645e2a4580b9ef1b534c5dccf0e00afa7c5f2 (patch) | |
tree | 9098e6ac81b4f11d61511b895eeeadc66cdcc1c4 | |
parent | 0c0c5b12cf05b5dfb2c3f2e8e4799e4acf94a2ea (diff) | |
parent | 4653245ddcf989ebac4b964a41d881d78cf9ae2c (diff) | |
download | nova-b96645e2a4580b9ef1b534c5dccf0e00afa7c5f2.tar.gz |
Merge "Validate id as integer for os-aggregates" into stable/train
-rw-r--r-- | nova/api/openstack/compute/aggregates.py | 41 | ||||
-rw-r--r-- | nova/tests/unit/api/openstack/compute/test_aggregates.py | 65 |
2 files changed, 85 insertions, 21 deletions
diff --git a/nova/api/openstack/compute/aggregates.py b/nova/api/openstack/compute/aggregates.py index 4fce9e9ce2..f522e323ce 100644 --- a/nova/api/openstack/compute/aggregates.py +++ b/nova/api/openstack/compute/aggregates.py @@ -28,6 +28,7 @@ from nova.compute import api as compute from nova import exception from nova.i18n import _ from nova.policies import aggregates as aggr_policies +from nova import utils def _get_context(req): @@ -85,11 +86,17 @@ class AggregateController(wsgi.Controller): return agg - @wsgi.expected_errors(404) + @wsgi.expected_errors((400, 404)) def show(self, req, id): """Shows the details of an aggregate, hosts and metadata included.""" context = _get_context(req) context.can(aggr_policies.POLICY_ROOT % 'show') + + try: + utils.validate_integer(id, 'id') + except exception.InvalidInput as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) + try: aggregate = self.api.get_aggregate(context, id) except exception.AggregateNotFound as e: @@ -108,6 +115,11 @@ class AggregateController(wsgi.Controller): updates['name'] = common.normalize_name(updates['name']) try: + utils.validate_integer(id, 'id') + except exception.InvalidInput as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) + + try: aggregate = self.api.update_aggregate(context, id, updates) except exception.AggregateNameExists as e: raise exc.HTTPConflict(explanation=e.format_message()) @@ -126,6 +138,12 @@ class AggregateController(wsgi.Controller): """Removes an aggregate by id.""" context = _get_context(req) context.can(aggr_policies.POLICY_ROOT % 'delete') + + try: + utils.validate_integer(id, 'id') + except exception.InvalidInput as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) + try: self.api.delete_aggregate(context, id) except exception.AggregateNotFound as e: @@ -136,7 +154,7 @@ class AggregateController(wsgi.Controller): # NOTE(gmann): Returns 200 for backwards compatibility but should be 202 # for representing async API as this API just accepts the request and # request hypervisor driver to complete the same in async mode. - @wsgi.expected_errors((404, 409)) + @wsgi.expected_errors((400, 404, 409)) @wsgi.action('add_host') @validation.schema(aggregates.add_host) def _add_host(self, req, id, body): @@ -145,6 +163,12 @@ class AggregateController(wsgi.Controller): context = _get_context(req) context.can(aggr_policies.POLICY_ROOT % 'add_host') + + try: + utils.validate_integer(id, 'id') + except exception.InvalidInput as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) + try: aggregate = self.api.add_host_to_aggregate(context, id, host) except (exception.AggregateNotFound, @@ -159,7 +183,7 @@ class AggregateController(wsgi.Controller): # NOTE(gmann): Returns 200 for backwards compatibility but should be 202 # for representing async API as this API just accepts the request and # request hypervisor driver to complete the same in async mode. - @wsgi.expected_errors((404, 409)) + @wsgi.expected_errors((400, 404, 409)) @wsgi.action('remove_host') @validation.schema(aggregates.remove_host) def _remove_host(self, req, id, body): @@ -168,6 +192,12 @@ class AggregateController(wsgi.Controller): context = _get_context(req) context.can(aggr_policies.POLICY_ROOT % 'remove_host') + + try: + utils.validate_integer(id, 'id') + except exception.InvalidInput as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) + try: aggregate = self.api.remove_host_from_aggregate(context, id, host) except (exception.AggregateNotFound, exception.AggregateHostNotFound, @@ -189,6 +219,11 @@ class AggregateController(wsgi.Controller): context = _get_context(req) context.can(aggr_policies.POLICY_ROOT % 'set_metadata') + try: + utils.validate_integer(id, 'id') + except exception.InvalidInput as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) + metadata = body["set_metadata"]["metadata"] try: aggregate = self.api.update_aggregate_metadata(context, diff --git a/nova/tests/unit/api/openstack/compute/test_aggregates.py b/nova/tests/unit/api/openstack/compute/test_aggregates.py index eb185849a6..c14f562dc7 100644 --- a/nova/tests/unit/api/openstack/compute/test_aggregates.py +++ b/nova/tests/unit/api/openstack/compute/test_aggregates.py @@ -299,7 +299,7 @@ class AggregateTestCaseV21(test.NoDBTestCase): self.controller.show, self.user_req, "1") - def test_show_with_invalid_id(self): + def test_show_with_bad_aggregate(self): side_effect = exception.AggregateNotFound(aggregate_id='2') with mock.patch.object(self.controller.api, 'get_aggregate', side_effect=side_effect) as mock_get: @@ -307,6 +307,10 @@ class AggregateTestCaseV21(test.NoDBTestCase): self.req, "2") mock_get.assert_called_once_with(self.context, '2') + def test_show_with_invalid_id(self): + self.assertRaises(exc.HTTPBadRequest, self.controller.show, + self.req, 'foo') + def test_update(self): body = {"aggregate": {"name": "new_name", "availability_zone": "nova1"}} @@ -390,10 +394,11 @@ class AggregateTestCaseV21(test.NoDBTestCase): @mock.patch('nova.compute.api.AggregateAPI.update_aggregate') def test_update_with_none_availability_zone(self, mock_update_agg): - agg_id = uuidsentinel.aggregate + agg_id = 173 mock_update_agg.return_value = objects.Aggregate(self.context, name='test', - uuid=agg_id, + uuid=uuidsentinel.agg, + id=agg_id, hosts=[], metadata={}) body = {"aggregate": {"name": "test", @@ -414,6 +419,11 @@ class AggregateTestCaseV21(test.NoDBTestCase): mock_update.assert_called_once_with(self.context, '2', body["aggregate"]) + def test_update_with_invalid_id(self): + body = {"aggregate": {"name": "test_name"}} + self.assertRaises(exc.HTTPBadRequest, self.controller.update, + self.req, 'foo', body=body) + def test_update_with_duplicated_name(self): body = {"aggregate": {"name": "test_name"}} side_effect = exception.AggregateNameExists(aggregate_name="test_name") @@ -433,7 +443,7 @@ class AggregateTestCaseV21(test.NoDBTestCase): def test_update_with_invalid_action(self): with mock.patch.object(self.controller.api, "update_aggregate", side_effect=exception.InvalidAggregateAction( - action='invalid', aggregate_id='agg1', reason= "not empty")): + action='invalid', aggregate_id='1', reason= "not empty")): body = {"aggregate": {"availability_zone": "nova"}} self.assertRaises(exc.HTTPBadRequest, self.controller.update, self.req, "1", body=body) @@ -467,15 +477,20 @@ class AggregateTestCaseV21(test.NoDBTestCase): def test_add_host_with_bad_aggregate(self): side_effect = exception.AggregateNotFound( - aggregate_id="bogus_aggregate") + aggregate_id="2") with mock.patch.object(self.controller.api, 'add_host_to_aggregate', side_effect=side_effect) as mock_add: self.assertRaises(exc.HTTPNotFound, eval(self.add_host), - self.req, "bogus_aggregate", + self.req, "2", body={"add_host": {"host": "host1"}}) - mock_add.assert_called_once_with(self.context, "bogus_aggregate", + mock_add.assert_called_once_with(self.context, "2", "host1") + def test_add_host_with_invalid_id(self): + body = {"add_host": {"host": "host1"}} + self.assertRaises(exc.HTTPBadRequest, eval(self.add_host), + self.req, 'foo', body=body) + def test_add_host_with_bad_host(self): side_effect = exception.ComputeHostNotFound(host="bogus_host") with mock.patch.object(self.controller.api, 'add_host_to_aggregate', @@ -534,16 +549,21 @@ class AggregateTestCaseV21(test.NoDBTestCase): def test_remove_host_with_bad_aggregate(self): side_effect = exception.AggregateNotFound( - aggregate_id="bogus_aggregate") + aggregate_id="2") with mock.patch.object(self.controller.api, 'remove_host_from_aggregate', side_effect=side_effect) as mock_rem: self.assertRaises(exc.HTTPNotFound, eval(self.remove_host), - self.req, "bogus_aggregate", + self.req, "2", body={"remove_host": {"host": "host1"}}) - mock_rem.assert_called_once_with(self.context, "bogus_aggregate", + mock_rem.assert_called_once_with(self.context, "2", "host1") + def test_remove_host_with_invalid_id(self): + body = {"remove_host": {"host": "host1"}} + self.assertRaises(exc.HTTPBadRequest, eval(self.remove_host), + self.req, 'foo', body=body) + def test_remove_host_with_host_not_in_aggregate(self): side_effect = exception.AggregateHostNotFound(aggregate_id="1", host="host1") @@ -621,16 +641,21 @@ class AggregateTestCaseV21(test.NoDBTestCase): def test_set_metadata_with_bad_aggregate(self): body = {"set_metadata": {"metadata": {"foo": "bar"}}} - side_effect = exception.AggregateNotFound(aggregate_id="bad_aggregate") + side_effect = exception.AggregateNotFound(aggregate_id="2") with mock.patch.object(self.controller.api, 'update_aggregate_metadata', side_effect=side_effect) as mock_update: self.assertRaises(exc.HTTPNotFound, eval(self.set_metadata), - self.req, "bad_aggregate", body=body) - mock_update.assert_called_once_with(self.context, "bad_aggregate", + self.req, "2", body=body) + mock_update.assert_called_once_with(self.context, "2", body["set_metadata"]['metadata']) + def test_set_metadata_with_invalid_id(self): + body = {"set_metadata": {"metadata": {"foo": "bar"}}} + self.assertRaises(exc.HTTPBadRequest, eval(self.set_metadata), + self.req, 'foo', body=body) + def test_set_metadata_with_missing_metadata(self): body = {"asdf": {"foo": "bar"}} self.assertRaises(self.bad_request, eval(self.set_metadata), @@ -679,21 +704,25 @@ class AggregateTestCaseV21(test.NoDBTestCase): def test_delete_aggregate_with_bad_aggregate(self): side_effect = exception.AggregateNotFound( - aggregate_id="bogus_aggregate") + aggregate_id="2") with mock.patch.object(self.controller.api, 'delete_aggregate', side_effect=side_effect) as mock_del: self.assertRaises(exc.HTTPNotFound, self.controller.delete, - self.req, "bogus_aggregate") - mock_del.assert_called_once_with(self.context, "bogus_aggregate") + self.req, "2") + mock_del.assert_called_once_with(self.context, "2") + + def test_delete_with_invalid_id(self): + self.assertRaises(exc.HTTPBadRequest, self.controller.delete, + self.req, 'foo') def test_delete_aggregate_with_host(self): with mock.patch.object(self.controller.api, "delete_aggregate", side_effect=exception.InvalidAggregateAction( - action="delete", aggregate_id="agg1", + action="delete", aggregate_id="2", reason="not empty")): self.assertRaises(exc.HTTPBadRequest, self.controller.delete, - self.req, "agg1") + self.req, "2") def test_marshall_aggregate(self): # _marshall_aggregate() just basically turns the aggregate returned |