From 02d79c592164e9e365336659c32bae7e504b0185 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Mon, 16 Mar 2020 17:18:28 +0000 Subject: [PATCH] Follow-ups for host_status:unknown-only policy rule This is a follow up patch to address nits on change I55bf78e63f68f8167249edc3327b024d9ecb0af2 Part of blueprint policy-rule-for-host-status-unknown Change-Id: I0660fbd6fbc0b0a8bb4198d8870d93725eb1e5d9 --- nova/api/openstack/compute/views/servers.py | 19 ++++--- nova/policies/servers.py | 10 +++- nova/tests/functional/test_policy.py | 61 ++++++++++----------- 3 files changed, 47 insertions(+), 43 deletions(-) diff --git a/nova/api/openstack/compute/views/servers.py b/nova/api/openstack/compute/views/servers.py index 344c680605..9747cfa249 100644 --- a/nova/api/openstack/compute/views/servers.py +++ b/nova/api/openstack/compute/views/servers.py @@ -185,15 +185,16 @@ class ViewBuilder(common.ViewBuilder): @staticmethod def _get_host_status_unknown_only(context): - # We will use the unknown_only variable to tell us what host status we - # can show, if any: - # * unknown_only = False means we can show any host status. - # * unknown_only = True means that we can only show host - # status: UNKNOWN. If the host status is anything other than - # UNKNOWN, we will not include the host_status field in the - # response. - # * unknown_only = None means we cannot show host status at all and - # we will not include the host_status field in the response. + """We will use the unknown_only variable to tell us what host status we + can show, if any: + * unknown_only = False means we can show any host status. + * unknown_only = True means that we can only show host + status: UNKNOWN. If the host status is anything other than + UNKNOWN, we will not include the host_status field in the + response. + * unknown_only = None means we cannot show host status at all and + we will not include the host_status field in the response. + """ unknown_only = None # Check show:host_status policy first because if it passes, we know we # can show any host status and need not check the more restrictive diff --git a/nova/policies/servers.py b/nova/policies/servers.py index 5354d3bcbb..d1262299df 100644 --- a/nova/policies/servers.py +++ b/nova/policies/servers.py @@ -124,7 +124,7 @@ API responses which are also controlled by this policy rule, like the } ]), policy.DocumentedRuleDefault( - SERVERS % 'show:host_status:unknown-only', + SERVERS % 'show:host_status:unknown-only', base.RULE_ADMIN_API, """ Show a server with additional host status information, only if host status is @@ -145,6 +145,14 @@ allow everyone. { 'method': 'GET', 'path': '/servers/detail' + }, + { + 'method': 'PUT', + 'path': '/servers/{server_id}' + }, + { + 'method': 'POST', + 'path': '/servers/{server_id}/action (rebuild)' } ]), policy.DocumentedRuleDefault( diff --git a/nova/tests/functional/test_policy.py b/nova/tests/functional/test_policy.py index e70bca2f71..6f08c64328 100644 --- a/nova/tests/functional/test_policy.py +++ b/nova/tests/functional/test_policy.py @@ -11,10 +11,10 @@ # under the License. import datetime -import functools from oslo_utils import timeutils +import nova.policies.base from nova import test from nova.tests import fixtures as nova_fixtures from nova.tests.functional import fixtures as func_fixtures @@ -58,7 +58,7 @@ class HostStatusPolicyTestCase(test.TestCase, # all users are allowed to see UNKNOWN host status only. self.policy.set_rules({ self.host_status_rule: 'rule:admin_api', - self.host_status_unknown_only_rule: '@'}, + self.host_status_unknown_only_rule: nova.policies.base.RULE_ANY}, # This is needed to avoid nulling out the rest of default policy. overwrite=False) # Create a server as a normal non-admin user. @@ -85,16 +85,18 @@ class HostStatusPolicyTestCase(test.TestCase, reset_state = {'os-resetState': {'state': 'active'}} self.admin_api.post_server_action(server['id'], reset_state) - def _test_host_status_unknown_only(self, admin_func, func): - # Get server as admin. - server = self._get_server(admin_func()) + def _test_host_status_unknown_only(self, func_name, *args): + admin_func = getattr(self.admin_api, func_name) + func = getattr(self.api, func_name) + # Run the operation as admin and extract the server from the response. + server = self._get_server(admin_func(*args)) # We need to wait for ACTIVE if this was a post rebuild server action, # else a subsequent rebuild request will fail with a 409 in the API. self._wait_for_state_change(server, 'ACTIVE') # Verify admin can see the host status UP. self.assertEqual('UP', server['host_status']) # Get server as normal non-admin user. - server = self._get_server(func()) + server = self._get_server(func(*args)) self._wait_for_state_change(server, 'ACTIVE') # Verify non-admin do not receive the host_status field because it is # not UNKNOWN. @@ -105,49 +107,47 @@ class HostStatusPolicyTestCase(test.TestCase, minutes_from_now = timeutils.utcnow() + datetime.timedelta(minutes=30) timeutils.set_time_override(override_time=minutes_from_now) self.addCleanup(timeutils.clear_time_override) - # Get server as admin. - server = self._get_server(admin_func()) + # Run the operation as admin and extract the server from the response. + server = self._get_server(admin_func(*args)) + # Verify admin can see the host status UNKNOWN. + self.assertEqual('UNKNOWN', server['host_status']) # Now that the compute service is down, the rebuild will not ever # complete. But we're only interested in what would be returned from # the API post rebuild action, so reset the state to ACTIVE to allow # the next rebuild request to go through without a 409 error. self._set_server_state_active(server) - # Verify admin can see the host status UNKNOWN. - self.assertEqual('UNKNOWN', server['host_status']) - # Get server as normal non-admin user. - server = self._get_server(func()) - self._set_server_state_active(server) + # Run the operation as a normal non-admin user and extract the server + # from the response. + server = self._get_server(func(*args)) # Verify non-admin can see the host status UNKNOWN too. self.assertEqual('UNKNOWN', server['host_status']) + self._set_server_state_active(server) # Now, adjust the policy to make it so only admin are allowed to see # UNKNOWN host status only. self.policy.set_rules({ self.host_status_unknown_only_rule: 'rule:admin_api'}, overwrite=False) - # Get server as normal non-admin user. - server = self._get_server(func()) - self._set_server_state_active(server) + # Run the operation as a normal non-admin user and extract the server + # from the response. + server = self._get_server(func(*args)) # Verify non-admin do not receive the host_status field. self.assertNotIn('host_status', server) + self._set_server_state_active(server) # Verify that admin will not receive ths host_status field if the # API microversion < 2.16. with utils.temporary_mutation(self.admin_api, microversion='2.15'): - server = self._get_server(admin_func()) + server = self._get_server(admin_func(*args)) self.assertNotIn('host_status', server) def test_get_server_host_status_unknown_only(self): server = self._setup_host_status_unknown_only_test() # GET /servers/{server_id} - admin_func = functools.partial(self.admin_api.get_server, server['id']) - func = functools.partial(self.api.get_server, server['id']) - self._test_host_status_unknown_only(admin_func, func) + self._test_host_status_unknown_only('get_server', server['id']) def test_get_servers_detail_host_status_unknown_only(self): self._setup_host_status_unknown_only_test() # GET /servers/detail - admin_func = functools.partial(self.admin_api.get_servers) - func = functools.partial(self.api.get_servers) - self._test_host_status_unknown_only(admin_func, func) + self._test_host_status_unknown_only('get_servers') def test_put_server_host_status_unknown_only(self): # The host_status field is returned from PUT /servers/{server_id} @@ -156,11 +156,9 @@ class HostStatusPolicyTestCase(test.TestCase, self.admin_api.microversion = '2.75' server = self._setup_host_status_unknown_only_test(networks='none') # PUT /servers/{server_id} - an_update = {'server': {'name': 'host-status-unknown-only'}} - admin_func = functools.partial(self.admin_api.put_server, server['id'], - an_update) - func = functools.partial(self.api.put_server, server['id'], an_update) - self._test_host_status_unknown_only(admin_func, func) + update = {'server': {'name': 'host-status-unknown-only'}} + self._test_host_status_unknown_only('put_server', server['id'], + update) def test_post_server_rebuild_host_status_unknown_only(self): # The host_status field is returned from POST @@ -170,8 +168,5 @@ class HostStatusPolicyTestCase(test.TestCase, server = self._setup_host_status_unknown_only_test(networks='none') # POST /servers/{server_id}/action (rebuild) rebuild = {'rebuild': {'imageRef': self.image_uuid}} - admin_func = functools.partial(self.admin_api.post_server_action, - server['id'], rebuild) - func = functools.partial(self.api.post_server_action, server['id'], - rebuild) - self._test_host_status_unknown_only(admin_func, func) + self._test_host_status_unknown_only('post_server_action', server['id'], + rebuild)