Add sort_key white list for server list/detail
Currently all the DB columns exposed in the sort_key parameter. That means the supported sort keys will be changed when the DB schema changed. This patch will strict the supported sort keys in a limited list. Only the keys which are in the API representation will be supported. For avoid to break the user hardly, the keys which aren't supported from now will be ignored when in the request. For other invalid sort keys, the 400 will be returned, which matches the behavior as before. The sort_key in the JSON-Schema will be enum. But due to there is no way to describe ignore few of enum values in the schema. So we put all ignored keys in the enum also. Then we will filter those keys out in the python code. For readability, we put all the ignored sort keys in the variable 'SERVER_LIST_IGNORE_SORT_KEY'. partial implement of bp add-whitelist-for-server-list-filter-sort-parameters Change-Id: I4af9b0eb2166ce14bde4ef4f8a1f9e9707f2c5cf
This commit is contained in:
Kevin_Zheng
parent
8211f36a5b
commit
3421ddea29
|
|
@ -249,6 +249,32 @@ JOINED_TABLE_QUERY_PARAMS_SERVERS = {
|
|||
'pci_devices': common_param
|
||||
}
|
||||
|
||||
# These fields are valid values for sort_keys before we start
|
||||
# using schema validation, but are considered to be bad values
|
||||
# and disabled to use. In order to avoid backward incompatibility,
|
||||
# they are ignored instead of return HTTP 400.
|
||||
SERVER_LIST_IGNORE_SORT_KEY = [
|
||||
'architecture', 'cell_name', 'cleaned', 'default_ephemeral_device',
|
||||
'default_swap_device', 'deleted', 'deleted_at', 'disable_terminate',
|
||||
'ephemeral_gb', 'ephemeral_key_uuid', 'id', 'key_data', 'launched_on',
|
||||
'locked', 'memory_mb', 'os_type', 'reservation_id', 'root_gb',
|
||||
'shutdown_terminate', 'user_data', 'vcpus', 'vm_mode'
|
||||
]
|
||||
|
||||
|
||||
VALID_SORT_KEYS = {
|
||||
"type": "string",
|
||||
"enum": ['access_ip_v4', 'access_ip_v6', 'auto_disk_config',
|
||||
'availability_zone', 'config_drive', 'created_at',
|
||||
'display_description', 'display_name', 'host', 'hostname',
|
||||
'image_ref', 'instance_type_id', 'kernel_id', 'key_name',
|
||||
'launch_index', 'launched_at', 'locked_by', 'node', 'power_state',
|
||||
'progress', 'project_id', 'ramdisk_id', 'root_device_name',
|
||||
'task_state', 'terminated_at', 'updated_at', 'user_id', 'uuid',
|
||||
'vm_state'] +
|
||||
SERVER_LIST_IGNORE_SORT_KEY
|
||||
}
|
||||
|
||||
query_params_v21 = {
|
||||
'type': 'object',
|
||||
'properties': {
|
||||
|
|
@ -294,7 +320,7 @@ query_params_v21 = {
|
|||
'accessIPv6': common_regex_param,
|
||||
'auto_disk_config': common_regex_param,
|
||||
'progress': common_regex_param,
|
||||
'sort_key': common_param,
|
||||
'sort_key': multi_params(VALID_SORT_KEYS),
|
||||
'sort_dir': common_param,
|
||||
'all_tenants': common_param,
|
||||
'deleted': common_param,
|
||||
|
|
|
|||
|
|
@ -14,6 +14,7 @@
|
|||
# License for the specific language governing permissions and limitations
|
||||
# under the License.
|
||||
|
||||
import copy
|
||||
from oslo_log import log as logging
|
||||
import oslo_messaging as messaging
|
||||
from oslo_utils import strutils
|
||||
|
|
@ -325,6 +326,9 @@ class ServersController(wsgi.Controller):
|
|||
|
||||
limit, marker = common.get_limit_and_marker(req)
|
||||
sort_keys, sort_dirs = common.get_sort_params(req.params)
|
||||
sort_keys, sort_dirs = remove_invalid_sort_keys(
|
||||
context, sort_keys, sort_dirs,
|
||||
schema_servers.SERVER_LIST_IGNORE_SORT_KEY, ('host', 'node'))
|
||||
|
||||
expected_attrs = ['pci_devices']
|
||||
if is_detail:
|
||||
|
|
@ -1205,6 +1209,29 @@ def remove_invalid_options(context, search_options, allowed_search_options):
|
|||
search_options.pop(opt, None)
|
||||
|
||||
|
||||
def remove_invalid_sort_keys(context, sort_keys, sort_dirs,
|
||||
blacklist, admin_only_fields):
|
||||
key_list = copy.deepcopy(sort_keys)
|
||||
for key in key_list:
|
||||
# NOTE(Kevin Zheng): We are intend to remove the sort_key
|
||||
# in the blacklist and its' corresponding sort_dir, since
|
||||
# the sort_key and sort_dir are not strict to be provide
|
||||
# in pairs in the current implement, sort_dirs could be
|
||||
# less than sort_keys, in order to avoid IndexError, we
|
||||
# only pop sort_dir when number of sort_dirs is no less
|
||||
# than the sort_key index.
|
||||
if key in blacklist:
|
||||
if len(sort_dirs) > sort_keys.index(key):
|
||||
sort_dirs.pop(sort_keys.index(key))
|
||||
sort_keys.pop(sort_keys.index(key))
|
||||
elif key in admin_only_fields and not context.is_admin:
|
||||
msg = _("Only administrators can sort servers "
|
||||
"by %s") % key
|
||||
raise exc.HTTPForbidden(explanation=msg)
|
||||
|
||||
return sort_keys, sort_dirs
|
||||
|
||||
|
||||
class Servers(extensions.V21APIExtensionBase):
|
||||
"""Servers."""
|
||||
|
||||
|
|
|
|||
|
|
@ -622,7 +622,7 @@ class ServersControllerTest(ControllerTest):
|
|||
def test_get_server_details_with_limit_and_other_params(self):
|
||||
req = self.req('/fake/servers/detail'
|
||||
'?limit=3&blah=2:t'
|
||||
'&sort_key=id1&sort_dir=asc')
|
||||
'&sort_key=uuid&sort_dir=asc')
|
||||
res = self.controller.detail(req)
|
||||
|
||||
servers = res['servers']
|
||||
|
|
@ -636,7 +636,7 @@ class ServersControllerTest(ControllerTest):
|
|||
self.assertEqual('/v2/fake/servers/detail', href_parts.path)
|
||||
params = urlparse.parse_qs(href_parts.query)
|
||||
expected = {'limit': ['3'],
|
||||
'sort_key': ['id1'], 'sort_dir': ['asc'],
|
||||
'sort_key': ['uuid'], 'sort_dir': ['asc'],
|
||||
'marker': [fakes.get_fake_uuid(2)]}
|
||||
self.assertThat(params, matchers.DictMatches(expected))
|
||||
|
||||
|
|
@ -684,6 +684,59 @@ class ServersControllerTest(ControllerTest):
|
|||
self.assertRaises(exception.ValidationError,
|
||||
self.controller.index, req)
|
||||
|
||||
def test_get_servers_invalid_sort_key(self):
|
||||
req = self.req('/fake/servers?sort_key=foo&sort_dir=desc')
|
||||
self.assertRaises(exception.ValidationError,
|
||||
self.controller.index, req)
|
||||
|
||||
@mock.patch.object(compute_api.API, 'get_all')
|
||||
def test_get_servers_ignore_sort_key(self, mock_get):
|
||||
req = self.req('/fake/servers?sort_key=vcpus&sort_dir=asc')
|
||||
self.controller.index(req)
|
||||
mock_get.assert_called_once_with(
|
||||
mock.ANY, search_opts=mock.ANY, limit=mock.ANY, marker=mock.ANY,
|
||||
expected_attrs=mock.ANY, sort_keys=[], sort_dirs=[])
|
||||
|
||||
@mock.patch.object(compute_api.API, 'get_all')
|
||||
def test_get_servers_ignore_sort_key_only_one_dir(self, mock_get):
|
||||
req = self.req(
|
||||
'/fake/servers?sort_key=user_id&sort_key=vcpus&sort_dir=asc')
|
||||
self.controller.index(req)
|
||||
mock_get.assert_called_once_with(
|
||||
mock.ANY, search_opts=mock.ANY, limit=mock.ANY, marker=mock.ANY,
|
||||
expected_attrs=mock.ANY, sort_keys=['user_id'],
|
||||
sort_dirs=['asc'])
|
||||
|
||||
@mock.patch.object(compute_api.API, 'get_all')
|
||||
def test_get_servers_ignore_sort_key_with_no_sort_dir(self, mock_get):
|
||||
req = self.req('/fake/servers?sort_key=vcpus&sort_key=user_id')
|
||||
self.controller.index(req)
|
||||
mock_get.assert_called_once_with(
|
||||
mock.ANY, search_opts=mock.ANY, limit=mock.ANY, marker=mock.ANY,
|
||||
expected_attrs=mock.ANY, sort_keys=['user_id'], sort_dirs=[])
|
||||
|
||||
@mock.patch.object(compute_api.API, 'get_all')
|
||||
def test_get_servers_ignore_sort_key_with_bad_sort_dir(self, mock_get):
|
||||
req = self.req('/fake/servers?sort_key=vcpus&sort_dir=bad_dir')
|
||||
self.controller.index(req)
|
||||
mock_get.assert_called_once_with(
|
||||
mock.ANY, search_opts=mock.ANY, limit=mock.ANY, marker=mock.ANY,
|
||||
expected_attrs=mock.ANY, sort_keys=[], sort_dirs=[])
|
||||
|
||||
def test_get_servers_non_admin_with_admin_only_sort_key(self):
|
||||
req = self.req('/fake/servers?sort_key=host&sort_dir=desc')
|
||||
self.assertRaises(webob.exc.HTTPForbidden,
|
||||
self.controller.index, req)
|
||||
|
||||
@mock.patch.object(compute_api.API, 'get_all')
|
||||
def test_get_servers_admin_with_admin_only_sort_key(self, mock_get):
|
||||
req = self.req('/fake/servers?sort_key=node&sort_dir=desc',
|
||||
use_admin_context=True)
|
||||
self.controller.detail(req)
|
||||
mock_get.assert_called_once_with(
|
||||
mock.ANY, search_opts=mock.ANY, limit=mock.ANY, marker=mock.ANY,
|
||||
expected_attrs=mock.ANY, sort_keys=['node'], sort_dirs=['desc'])
|
||||
|
||||
def test_get_servers_with_bad_option(self):
|
||||
server_uuid = uuids.fake
|
||||
|
||||
|
|
@ -1388,8 +1441,8 @@ class ServersControllerTestV29(ServersControllerTest):
|
|||
@mock.patch.object(compute_api.API, 'get_all')
|
||||
def test_get_servers_remove_non_search_options(self, get_all_mock):
|
||||
req = fakes.HTTPRequestV21.blank('/servers'
|
||||
'?sort_key=id1&sort_dir=asc'
|
||||
'&sort_key=id2&sort_dir=desc'
|
||||
'?sort_key=uuid&sort_dir=asc'
|
||||
'&sort_key=user_id&sort_dir=desc'
|
||||
'&limit=1&marker=123',
|
||||
use_admin_context=True)
|
||||
self.controller.index(req)
|
||||
|
|
|
|||
Loading…
Reference in New Issue