Validate id as integer for os-aggregates
According to the api-ref, the id passed to calls in os-aggregates is
supposed to be an integer. No function validated this, so any value
passed to these functions would directly reach the DB. While this is
fine for SQLite, making a query with a string for an integer column on
other databases like PostgreSQL results in a DBError exception and thus
a HTTP 500 instead of 400 or 404.
This commit adds validation for the id parameter the same way it's
already done for other endpoints.
Conflicts:
nova/api/openstack/compute/aggregates.py
Changes:
nova/tests/unit/api/openstack/compute/test_aggregates.py
NOTE(stephenfin): Conflicts are due to absence of change
I4ab96095106b38737ed355fcad07e758f8b5a9b0 ("Add image caching API for
aggregates") which we don't want to backport. A test related to this
feature must also be removed.
Change-Id: I83817f7301680801beaee375825f02eda526eda1
Closes-Bug: 1865040
(cherry picked from commit 2e70a1717f)
This commit is contained in:
Johannes Kulik
Stephen Finucane
parent
9223613ad3
commit
4653245ddc
|
|
@ -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:
|
||||
|
|
@ -107,6 +114,11 @@ class AggregateController(wsgi.Controller):
|
|||
if 'name' in updates:
|
||||
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:
|
||||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue