From cb56ae6aad274ba0cef8b97dcc2130e114260db2 Mon Sep 17 00:00:00 2001 From: Ghanshyam Mann Date: Fri, 8 Jan 2021 16:48:29 -0600 Subject: [PATCH] Fix ForbiddenWithAccelerators to HTTPForbidden for shelve API ForbiddenWithAccelerators is not converted to HTTPForbidden unless that is explicitly converted in API controller. For example: - https://github.com/openstack/nova/blob/46899968619e4ea0ff2ab380977619bb29578d43/nova/api/openstack/compute/migrate_server.py#L154 Otherwise it end up raising 500 because ForbiddenWithAccelerators is not inherrited from nova.exception.Forbidden - https://github.com/openstack/nova/blob/46899968619e4ea0ff2ab380977619bb29578d43/nova/exception.py#L158 and expected_errors() decorator convert it to 500. - https://github.com/openstack/nova/blob/46899968619e4ea0ff2ab380977619bb29578d43/nova/api/openstack/wsgi.py#L689 Except shelve API, all other APIs which can get ForbiddenWithAccelerators via block_accelerators decorator convert it explicitly. If we inherit ForbiddenWithAccelerators from nova.exception.Forbidden then expected_errors() decorator will take care of convertion to HTTPForbidden automatically for all APIs. Also adding tests for APIs can get ForbiddenWithAccelerators. Change-Id: I9335ddb2d72909a110c313d5b609f2be279b18ef --- nova/exception.py | 3 +-- .../api/openstack/compute/test_migrate_server.py | 8 ++++++++ .../api/openstack/compute/test_server_actions.py | 8 ++++++++ .../tests/unit/api/openstack/compute/test_shelve.py | 13 +++++++++++++ .../api/openstack/compute/test_suspend_server.py | 8 ++++++++ 5 files changed, 38 insertions(+), 2 deletions(-) diff --git a/nova/exception.py b/nova/exception.py index 9f4d53db05..a5aa4af3d5 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -155,9 +155,8 @@ class Forbidden(NovaException): code = 403 -class ForbiddenWithAccelerators(NovaException): +class ForbiddenWithAccelerators(Forbidden): msg_fmt = _("Forbidden with instances that have accelerators.") - code = 403 class AdminRequired(Forbidden): diff --git a/nova/tests/unit/api/openstack/compute/test_migrate_server.py b/nova/tests/unit/api/openstack/compute/test_migrate_server.py index 5928fe3e56..62df9fa3d1 100644 --- a/nova/tests/unit/api/openstack/compute/test_migrate_server.py +++ b/nova/tests/unit/api/openstack/compute/test_migrate_server.py @@ -300,6 +300,14 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): expected_exc=webob.exc.HTTPInternalServerError, check_response=False) + @mock.patch('nova.compute.api.API.live_migrate', + side_effect=exception.ForbiddenWithAccelerators) + def test_live_migration_raises_http_forbidden(self, mock_migrate): + body = self._get_migration_body(host='hostname') + self.assertRaises(webob.exc.HTTPForbidden, + self.controller._migrate_live, + self.req, fakes.FAKE_UUID, body=body) + class MigrateServerTestsV225(MigrateServerTestsV21): diff --git a/nova/tests/unit/api/openstack/compute/test_server_actions.py b/nova/tests/unit/api/openstack/compute/test_server_actions.py index c30f825698..03c6901939 100644 --- a/nova/tests/unit/api/openstack/compute/test_server_actions.py +++ b/nova/tests/unit/api/openstack/compute/test_server_actions.py @@ -856,6 +856,14 @@ class ServerActionsControllerTestV21(test.TestCase): self.controller._action_resize, self.req, FAKE_UUID, body=body) + @mock.patch('nova.compute.api.API.resize', + side_effect=exception.ForbiddenWithAccelerators) + def test_resize_raises_http_forbidden(self, mock_resize): + body = dict(resize=dict(flavorRef="http://localhost/3")) + self.assertRaises(webob.exc.HTTPForbidden, + self.controller._action_resize, + self.req, FAKE_UUID, body=body) + def test_confirm_resize_server(self): body = dict(confirmResize=None) diff --git a/nova/tests/unit/api/openstack/compute/test_shelve.py b/nova/tests/unit/api/openstack/compute/test_shelve.py index 9cf87d9c42..765b7729f9 100644 --- a/nova/tests/unit/api/openstack/compute/test_shelve.py +++ b/nova/tests/unit/api/openstack/compute/test_shelve.py @@ -59,6 +59,19 @@ class ShelveControllerTest(test.NoDBTestCase): webob.exc.HTTPConflict, self.controller._shelve, self.req, uuids.fake, {}) + @mock.patch('nova.compute.api.API.shelve') + @mock.patch('nova.api.openstack.common.get_instance') + def test_shelve_raise_http_forbidden( + self, mock_get_instance, mock_shelve, + ): + mock_get_instance.return_value = ( + fake_instance.fake_instance_obj(self.req.environ['nova.context'])) + mock_shelve.side_effect = exception.ForbiddenWithAccelerators + + self.assertRaises( + webob.exc.HTTPForbidden, self.controller._shelve, + self.req, uuids.fake, {}) + @mock.patch('nova.api.openstack.common.get_instance') def test_unshelve_locked_server(self, get_instance_mock): get_instance_mock.return_value = ( diff --git a/nova/tests/unit/api/openstack/compute/test_suspend_server.py b/nova/tests/unit/api/openstack/compute/test_suspend_server.py index 7d6857770e..9b5514d75b 100644 --- a/nova/tests/unit/api/openstack/compute/test_suspend_server.py +++ b/nova/tests/unit/api/openstack/compute/test_suspend_server.py @@ -17,6 +17,7 @@ import webob from nova.api.openstack.compute import suspend_server as \ suspend_server_v21 +from nova import exception from nova import objects from nova.tests.unit.api.openstack.compute import admin_only_action_common from nova.tests.unit.api.openstack import fakes @@ -58,3 +59,10 @@ class SuspendServerTestsV21(admin_only_action_common.CommonTests): def test_actions_with_locked_instance(self): self._test_actions_with_locked_instance(['_suspend', '_resume']) + + @mock.patch('nova.compute.api.API.suspend', + side_effect=exception.ForbiddenWithAccelerators) + def test_suspend_raises_http_forbidden(self, mock_suspend): + self.assertRaises(webob.exc.HTTPForbidden, + self.controller._suspend, + self.req, fakes.FAKE_UUID, body={})