diff --git a/nova/api/openstack/compute/views/servers.py b/nova/api/openstack/compute/views/servers.py index 087bc27091..4e0cd1264c 100644 --- a/nova/api/openstack/compute/views/servers.py +++ b/nova/api/openstack/compute/views/servers.py @@ -29,6 +29,7 @@ from nova import context as nova_context from nova import exception from nova.network.security_group import openstack_driver from nova import objects +from nova.objects import fields from nova.objects import virtual_interface from nova.policies import extended_server_attributes as esa_policies from nova.policies import flavor_extra_specs as fes_policies @@ -184,6 +185,34 @@ class ViewBuilder(common.ViewBuilder): context, instance) return ret + @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. + 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 + # show:host_status:unknown-only policy. + if context.can( + servers_policies.SERVERS % 'show:host_status', + fatal=False): + unknown_only = False + # If we are not allowed to show any/all host status, check if we can at + # least show only the host status: UNKNOWN. + elif context.can( + servers_policies.SERVERS % + 'show:host_status:unknown-only', + fatal=False): + unknown_only = True + return unknown_only + def show(self, request, instance, extend_address=True, show_extra_specs=None, show_AZ=True, show_config_drive=True, show_extended_attr=None, show_host_status=None, @@ -330,12 +359,21 @@ class ViewBuilder(common.ViewBuilder): add_delete_on_termination) if (api_version_request.is_supported(request, min_version='2.16')): if show_host_status is None: - show_host_status = context.can( - servers_policies.SERVERS % 'show:host_status', fatal=False) - if show_host_status: - host_status = self.compute_api.get_instance_host_status( - instance) - server["server"]['host_status'] = host_status + unknown_only = self._get_host_status_unknown_only(context) + # If we're not allowed by policy to show host status at all, + # don't bother requesting instance host status from the compute + # API. + if unknown_only is not None: + host_status = self.compute_api.get_instance_host_status( + instance) + # If we are allowed to show host status of some kind, set + # the host status field only if: + # * unknown_only = False, meaning we can show any status + # OR + # * if unknown_only = True and host_status == UNKNOWN + if (not unknown_only or + host_status == fields.HostStatus.UNKNOWN): + server["server"]['host_status'] = host_status if api_version_request.is_supported(request, min_version="2.9"): server["server"]["locked"] = (True if instance["locked_by"] @@ -402,10 +440,13 @@ class ViewBuilder(common.ViewBuilder): bdms=bdms, cell_down_support=cell_down_support) - if (api_version_request.is_supported(request, min_version='2.16') and - context.can(servers_policies.SERVERS % 'show:host_status', - fatal=False)): - self._add_host_status(list(servers_dict["servers"]), instances) + if api_version_request.is_supported(request, min_version='2.16'): + unknown_only = self._get_host_status_unknown_only(context) + # If we're not allowed by policy to show host status at all, don't + # bother requesting instance host status from the compute API. + if unknown_only is not None: + self._add_host_status(list(servers_dict["servers"]), instances, + unknown_only=unknown_only) self._add_security_grps(request, list(servers_dict["servers"]), instances) @@ -575,7 +616,7 @@ class ViewBuilder(common.ViewBuilder): return fault_dict - def _add_host_status(self, servers, instances): + def _add_host_status(self, servers, instances, unknown_only=False): """Adds the ``host_status`` field to the list of servers This method takes care to filter instances from down cells since they @@ -585,6 +626,7 @@ class ViewBuilder(common.ViewBuilder): body; this list is modified by reference by updating the server dicts within the list :param instances: list of Instance objects + :param unknown_only: whether to show only UNKNOWN host status """ # Filter out instances from down cells which do not have a host field. instances = [instance for instance in instances if 'host' in instance] @@ -594,7 +636,12 @@ class ViewBuilder(common.ViewBuilder): # Filter out anything that is not in the resulting dict because # we had to filter the list of instances above for down cells. if server['id'] in host_statuses: - server['host_status'] = host_statuses[server['id']] + host_status = host_statuses[server['id']] + if unknown_only and host_status != fields.HostStatus.UNKNOWN: + # Filter servers that are not allowed by policy to see + # host_status values other than UNKNOWN. + continue + server['host_status'] = host_status def _add_security_grps(self, req, servers, instances, create_request=False): diff --git a/nova/policies/servers.py b/nova/policies/servers.py index 47cf30be52..1cc5f05277 100644 --- a/nova/policies/servers.py +++ b/nova/policies/servers.py @@ -95,6 +95,10 @@ rules = [ """ Show a server with additional host status information. +This means host_status will be shown irrespective of status value. If showing +only host_status UNKNOWN is desired, use the +``os_compute_api:servers:show:host_status:unknown-only`` policy rule. + Microvision 2.75 added the ``host_status`` attribute in the ``PUT /servers/{server_id}`` and ``POST /servers/{server_id}/action (rebuild)`` API responses which are also controlled by this policy rule, like the @@ -118,6 +122,30 @@ API responses which are also controlled by this policy rule, like the 'path': '/servers/{server_id}/action (rebuild)' } ]), + policy.DocumentedRuleDefault( + SERVERS % 'show:host_status:unknown-only', + base.RULE_ADMIN_API, + """ +Show a server with additional host status information, only if host status is +UNKNOWN. + +This policy rule will only be enforced when the +``os_compute_api:servers:show:host_status`` policy rule does not pass for the +request. An example policy configuration could be where the +``os_compute_api:servers:show:host_status`` rule is set to allow admin-only and +the ``os_compute_api:servers:show:host_status:unknown-only`` rule is set to +allow everyone. +""", + [ + { + 'method': 'GET', + 'path': '/servers/{server_id}' + }, + { + 'method': 'GET', + 'path': '/servers/detail' + } + ]), policy.DocumentedRuleDefault( SERVERS % 'create', RULE_AOO, diff --git a/nova/tests/functional/test_policy.py b/nova/tests/functional/test_policy.py new file mode 100644 index 0000000000..23658d5fa1 --- /dev/null +++ b/nova/tests/functional/test_policy.py @@ -0,0 +1,181 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import datetime +import functools + +from oslo_utils import timeutils + +from nova import test +from nova.tests import fixtures as nova_fixtures +from nova.tests.functional import fixtures as func_fixtures +from nova.tests.functional import integrated_helpers +from nova.tests.unit.image import fake as fake_image +from nova.tests.unit import policy_fixture +from nova import utils + + +class HostStatusPolicyTestCase(test.TestCase, + integrated_helpers.InstanceHelperMixin): + """Tests host_status policies behavior in the API.""" + host_status_rule = 'os_compute_api:servers:show:host_status' + host_status_unknown_only_rule = ( + 'os_compute_api:servers:show:host_status:unknown-only') + image_uuid = '155d900f-4e14-4e4c-a73d-069cbf4541e6' + + def setUp(self): + super(HostStatusPolicyTestCase, self).setUp() + # Setup the standard fixtures. + fake_image.stub_out_image_service(self) + self.addCleanup(fake_image.FakeImageService_reset) + self.useFixture(nova_fixtures.NeutronFixture(self)) + self.useFixture(func_fixtures.PlacementFixture()) + self.useFixture(policy_fixture.RealPolicyFixture()) + + # Start the services. + self.start_service('conductor') + self.start_service('scheduler') + self.compute = self.start_service('compute') + api_fixture = self.useFixture(nova_fixtures.OSAPIFixture( + api_version='v2.1')) + self.api = api_fixture.api + self.admin_api = api_fixture.admin_api + # The host_status field is returned starting in microversion 2.16. + self.api.microversion = '2.16' + self.admin_api.microversion = '2.16' + + def _setup_host_status_unknown_only_test(self, networks=None): + # Set policy such that admin are allowed to see any/all host status and + # 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: '@'}, + # This is needed to avoid nulling out the rest of default policy. + overwrite=False) + # Create a server as a normal non-admin user. + # In microversion 2.36 the /images proxy API was deprecated, so + # specifiy the image_uuid directly. + kwargs = {'image_uuid': self.image_uuid} + if networks: + # Starting with microversion 2.37 the networks field is required. + kwargs['networks'] = networks + server = self._build_minimal_create_server_request( + self.api, 'test_host_status_unknown_only', **kwargs) + server = self.api.post_server({'server': server}) + server = self._wait_for_state_change(self.admin_api, server, 'ACTIVE') + return server + + @staticmethod + def _get_server(resp): + # Get a server whether it's a single server or a list of one server. + server = resp if not isinstance(resp, list) else resp[0] + # The PUT /servers/{server_id} response has a 'server' attribute. + if 'server' in server: + server = server['server'] + return server + + def _set_server_state_active(self, server): + # Needed for being able to issue multiple rebuild requests while the + # compute service is down. + 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()) + # 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(self.admin_api, 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()) + self._wait_for_state_change(self.admin_api, server, 'ACTIVE') + # Verify non-admin do not receive the host_status field because it is + # not UNKNOWN. + self.assertNotIn('host_status', server) + # Stop the compute service to trigger UNKNOWN host_status. + self.compute.stop() + # Advance time by 30 minutes so nova considers service as down. + 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()) + # 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) + # Verify non-admin can see the host status UNKNOWN too. + self.assertEqual('UNKNOWN', server['host_status']) + # 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) + # Verify non-admin do not receive the host_status field. + self.assertNotIn('host_status', 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()) + 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) + + 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) + + def test_put_server_host_status_unknown_only(self): + # The host_status field is returned from PUT /servers/{server_id} + # starting in microversion 2.75. + self.api.microversion = '2.75' + 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) + + def test_post_server_rebuild_host_status_unknown_only(self): + # The host_status field is returned from POST + # /servers/{server_id}/action (rebuild) starting in microversion 2.75. + self.api.microversion = '2.75' + self.admin_api.microversion = '2.75' + 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) diff --git a/nova/tests/unit/api/openstack/compute/test_serversV21.py b/nova/tests/unit/api/openstack/compute/test_serversV21.py index dad71e89ee..07d2910392 100644 --- a/nova/tests/unit/api/openstack/compute/test_serversV21.py +++ b/nova/tests/unit/api/openstack/compute/test_serversV21.py @@ -18,6 +18,7 @@ import collections import copy import datetime import ddt +import functools import fixtures import iso8601 @@ -2150,6 +2151,21 @@ class ServersControllerTestV216(ServersControllerTest): return server_dict + @mock.patch('nova.compute.api.API.get_instance_host_status') + def _verify_host_status_policy_behavior(self, func, mock_get_host_status): + # Set policy to disallow both host_status cases and verify we don't + # call the get_instance_host_status compute RPC API. + rules = { + 'os_compute_api:servers:show:host_status': '!', + 'os_compute_api:servers:show:host_status:unknown-only': '!', + } + orig_rules = policy.get_rules() + policy.set_rules(oslo_policy.Rules.from_dict(rules), overwrite=False) + func() + mock_get_host_status.assert_not_called() + # Restore the original rules. + policy.set_rules(orig_rules) + def test_show(self): image_bookmark = "http://localhost/%s/images/10" % self.project_id flavor_bookmark = "http://localhost/%s/flavors/2" % self.project_id @@ -2160,6 +2176,8 @@ class ServersControllerTestV216(ServersControllerTest): flavor_bookmark, progress=0) self.assertThat(res_dict, matchers.DictMatches(expected_server)) + func = functools.partial(self.controller.show, req, FAKE_UUID) + self._verify_host_status_policy_behavior(func) def test_detail(self): def fake_get_all(context, search_opts=None, @@ -2215,6 +2233,9 @@ class ServersControllerTestV216(ServersControllerTest): # 2 servers in the response are using the same host). self.mock_get_instance_host_status.assert_called_once() + func = functools.partial(self.controller.detail, req) + self._verify_host_status_policy_behavior(func) + class ServersControllerTestV219(ServersControllerTest): wsgi_api_version = '2.19' diff --git a/nova/tests/unit/fake_policy.py b/nova/tests/unit/fake_policy.py index 688db4e736..b57ce60d67 100644 --- a/nova/tests/unit/fake_policy.py +++ b/nova/tests/unit/fake_policy.py @@ -18,6 +18,7 @@ policy_data = """ "context_is_admin": "role:admin or role:administrator", "os_compute_api:servers:show:host_status": "", + "os_compute_api:servers:show:host_status:unknown-only": "", "os_compute_api:servers:allow_all_filters": "", "os_compute_api:servers:migrations:force_complete": "", "os_compute_api:os-admin-actions:inject_network_info": "", diff --git a/nova/tests/unit/test_policy.py b/nova/tests/unit/test_policy.py index 3a77b632f5..25405c2fff 100644 --- a/nova/tests/unit/test_policy.py +++ b/nova/tests/unit/test_policy.py @@ -297,6 +297,7 @@ class RealRolePolicyTestCase(test.NoDBTestCase): "os_compute_api:servers:index:get_all_tenants", "os_compute_api:servers:allow_all_filters", "os_compute_api:servers:show:host_status", +"os_compute_api:servers:show:host_status:unknown-only", "os_compute_api:servers:migrations:force_complete", "os_compute_api:servers:migrations:delete", "os_compute_api:os-admin-actions:reset_network", diff --git a/releasenotes/notes/host_status_unknown_policy-839cfda56b610d39.yaml b/releasenotes/notes/host_status_unknown_policy-839cfda56b610d39.yaml new file mode 100644 index 0000000000..3b64d87aa5 --- /dev/null +++ b/releasenotes/notes/host_status_unknown_policy-839cfda56b610d39.yaml @@ -0,0 +1,17 @@ +--- +features: + - | + A new policy rule ``os_compute_api:servers:show:host_status:unknown-only`` + has been added to control whether a user can view a server host status of + ``UNKNOWN`` in the following APIs: + + * ``GET /servers/{server_id}`` if using API microversion >= 2.16 + * ``GET /servers/detail`` if using API microversion >= 2.16 + * ``PUT /servers/{server_id}`` if using API microversion >= 2.75 + * ``POST /servers/{server_id}/action`` (rebuild) if using API microversion + >= 2.75 + + This is different than the ``os_compute_api:servers:show:host_status`` + policy rule which controls whether a user can view all possible host + status in the aforementioned APIs including ``UP``, ``DOWN``, + ``MAINTENANCE``, and ``UNKNOWN``.