Fix unspecified bahavior on GET /servers/detail?tenant_id=X as admin
When an admin calls /v2.1/servers/detail?tenant_id=XX, then the `get_all` method of nova.compute.api.API is called with 2 conflicting search options: {'tenant_id': XX, 'project_id': YY}. But because, latter on, in that `get_all` method we define a dict called filter_mapping, on which we iter upon, which value takes precedence depends on the order in which the dict is iterated upon. As the order in unpredictable and varies between Py2 and Py3, this is problematic. Especially, on Python 2 hash randomization is disabled by default but it's enabled by default on Py3 (unless the PYTHONHASHSEED env var is set to a fixed value) [0] The (unreliable) order we iterate on items of that `filter_mapping` dict is why the Tempest test_list_servers_by_admin_with_specified_tenant test randomly fails on Py35. This patch ensures that, if the all_tenants search option is not set, then the `tenant_id` search option is ignored. Note that this *is* the current behavior on Py27, as documented in lp:#1185290 and tested by Tempest here [1]. [0] https://docs.python.org/dev/using/cmdline.html#cmdoption-R [1]fe1a8e289c/tempest/api/compute/admin/test_servers.py (L96)
Change-Id: I1a74ef0f16da14444029c0f184b433df367ffb41 Closes-Bug: #1659811 (cherry picked from commitf702762934
)
This commit is contained in:
parent
6087675d1b
commit
1c992c7a17
|
@ -323,6 +323,11 @@ class ServersController(wsgi.Controller):
|
|||
context.can(server_policies.SERVERS % 'index:get_all_tenants')
|
||||
elevated = context.elevated()
|
||||
else:
|
||||
# As explained in lp:#1185290, if `all_tenants` is not passed
|
||||
# we must ignore the `tenant_id` search option. As explained
|
||||
# in a above code comment, any change to this behavior would
|
||||
# require a microversion bump.
|
||||
search_opts.pop('tenant_id', None)
|
||||
if context.project_id:
|
||||
search_opts['project_id'] = context.project_id
|
||||
else:
|
||||
|
|
|
@ -779,6 +779,7 @@ class ServersControllerTest(ControllerTest):
|
|||
def test_tenant_id_filter_no_admin_context(self):
|
||||
def fake_get_all(context, search_opts=None, **kwargs):
|
||||
self.assertIsNotNone(search_opts)
|
||||
self.assertNotIn('tenant_id', search_opts)
|
||||
self.assertEqual(search_opts['project_id'], 'fake')
|
||||
return [fakes.stub_instance_obj(100)]
|
||||
|
||||
|
@ -788,6 +789,21 @@ class ServersControllerTest(ControllerTest):
|
|||
servers = self.controller.index(req)['servers']
|
||||
self.assertEqual(len(servers), 1)
|
||||
|
||||
def test_tenant_id_filter_admin_context(self):
|
||||
""""Test tenant_id search opt is dropped if all_tenants is not set."""
|
||||
def fake_get_all(context, search_opts=None, **kwargs):
|
||||
self.assertIsNotNone(search_opts)
|
||||
self.assertNotIn('tenant_id', search_opts)
|
||||
self.assertEqual('fake', search_opts['project_id'])
|
||||
return [fakes.stub_instance_obj(100)]
|
||||
|
||||
req = self.req('/fake/servers?tenant_id=newfake',
|
||||
use_admin_context=True)
|
||||
with mock.patch.object(compute_api.API, 'get_all') as mock_get:
|
||||
mock_get.side_effect = fake_get_all
|
||||
servers = self.controller.index(req)['servers']
|
||||
self.assertEqual(len(servers), 1)
|
||||
|
||||
def test_all_tenants_param_normal(self):
|
||||
def fake_get_all(context, search_opts=None, **kwargs):
|
||||
self.assertNotIn('project_id', search_opts)
|
||||
|
|
Loading…
Reference in New Issue