openstack/python-cinderclient
Code Issues Proposed changes

Fix encoding of query parameters

Browse Source 
IETF RFC 3986 classifies "~" as a reserved character [1],
however until python3.7 [2], python's url parsing
used to encode this character.

urllib has seen a lot of churn in various python
releases, and hence we were using a six wrapper
to shield ourselves, however, this backwards-incompatible
change in encoding norms forces us to deal with
the problem at our end.

Cinder's API accepts "~" in both, its encoded
or un-encoded forms. So, let's stop encoding it
within cinderclient, regardless of the version
of python running it.

Also fix an inconsitency around the use of the
generic helper method in utils added in
I3a3ae90cc6011d1aa0cc39db4329d9bc08801904
(cinderclient/utils.py - build_query_param)
to allow for False as a value in the query.

[1] https://tools.ietf.org/html/rfc3986.html
[2] https://docs.python.org/3/library/urllib.parse.html#url-quoting

Change-Id: I89809694ac3e4081ce83fd4f788f9355d6772f59
Closes-Bug: #1784728
This commit is contained in:
Goutham Pacha Ravi 2018-09-13 16:45:45 -06:00
parent 64b2d5d83b
commit 223d754f61
12 changed files with 64 additions and 59 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
cinderclient/apiclient/base.py
View File

@ -28,9 +28,9 @@ import copy
from requests import Response
import six
from six.moves.urllib import parse
from cinderclient.apiclient import exceptions
from cinderclient import utils
from oslo_utils import encodeutils
from oslo_utils import strutils
@ -330,7 +330,7 @@ class CrudManager(BaseManager):
return self._list(
'%(base_url)s%(query)s' % {
'base_url': self.build_url(base_url=base_url, **kwargs),
'query': '?%s' % parse.urlencode(kwargs) if kwargs else '',
'query': utils.build_query_param(kwargs),
},
self.collection_key)
@ -369,7 +369,7 @@ class CrudManager(BaseManager):
rl = self._list(
'%(base_url)s%(query)s' % {
'base_url': self.build_url(base_url=base_url, **kwargs),
'query': '?%s' % parse.urlencode(kwargs) if kwargs else '',
'query': '?%s' % utils.build_query_param(kwargs),
},
self.collection_key)
num = len(rl)

7
cinderclient/base.py
View File

@ -24,7 +24,6 @@ import hashlib
import os
import six
from six.moves.urllib import parse
from cinderclient.apiclient import base as common_base
from cinderclient import exceptions
@ -173,10 +172,8 @@ class Manager(common_base.HookableMixin):
query_params = utils.unicode_key_value_to_string(query_params)
# Transform the dict to a sequence of two-element tuples in fixed
# order, then the encoded string will be consistent in Python 2&3.
query_string = ""
if query_params:
params = sorted(query_params.items(), key=lambda x: x[0])
query_string = "?%s" % parse.urlencode(params)
query_string = utils.build_query_param(query_params, sort=True)
detail = ""
if detailed:

23
cinderclient/tests/unit/test_utils.py
View File

@ -162,6 +162,7 @@ class CaptureStdout(object):
self.read = self.stringio.read
@ddt.ddt
class BuildQueryParamTestCase(test_utils.TestCase):
def test_build_param_without_sort_switch(self):
@ -187,19 +188,17 @@ class BuildQueryParamTestCase(test_utils.TestCase):
expected = "?key1=val1&key2=val2&key3=val3"
self.assertEqual(expected, result)
def test_build_param_with_none(self):
dict_param = {
'key1': 'val1',
'key2': None,
'key3': False,
'key4': ''
}
result_1 = utils.build_query_param(dict_param)
result_2 = utils.build_query_param(None)
@ddt.data({},
None,
{'key1': 'val1', 'key2': None, 'key3': False, 'key4': ''})
def test_build_param_with_nones(self, dict_param):
result = utils.build_query_param(dict_param)
expected = "?key1=val1"
self.assertEqual(expected, result_1)
self.assertFalse(result_2)
expected = ("key1=val1", "key3=False") if dict_param else ()
for exp in expected:
self.assertIn(exp, result)
if not expected:
self.assertEqual("", result)
@ddt.ddt

17
cinderclient/tests/unit/v1/test_shell.py
View File

@ -101,7 +101,7 @@ class ShellTest(utils.TestCase):
def test_list(self):
self.run_command('list')
# NOTE(jdg): we default to detail currently
self.assert_called('GET', '/volumes/detail')
self.assert_called('GET', '/volumes/detail?all_tenants=0')
def test_list_filter_tenant_with_all_tenants(self):
self.run_command('list --tenant=123 --all-tenants 1')
@ -184,11 +184,13 @@ class ShellTest(utils.TestCase):
def test_list_filter_status(self):
self.run_command('list --status=available')
self.assert_called('GET', '/volumes/detail?status=available')
self.assert_called('GET',
'/volumes/detail?all_tenants=0&status=available')
def test_list_filter_display_name(self):
self.run_command('list --display-name=1234')
self.assert_called('GET', '/volumes/detail?display_name=1234')
self.assert_called('GET',
'/volumes/detail?all_tenants=0&display_name=1234')
def test_list_all_tenants(self):
self.run_command('list --all-tenants=1')
@ -200,7 +202,7 @@ class ShellTest(utils.TestCase):
def test_list_limit(self):
self.run_command('list --limit=10')
self.assert_called('GET', '/volumes/detail?limit=10')
self.assert_called('GET', '/volumes/detail?all_tenants=0&limit=10')
def test_show(self):
self.run_command('show 1234')
@ -231,12 +233,13 @@ class ShellTest(utils.TestCase):
def test_snapshot_list_filter_volume_id(self):
self.run_command('snapshot-list --volume-id=1234')
self.assert_called('GET', '/snapshots/detail?volume_id=1234')
self.assert_called('GET',
'/snapshots/detail?all_tenants=0&volume_id=1234')
def test_snapshot_list_filter_status_and_volume_id(self):
self.run_command('snapshot-list --status=available --volume-id=1234')
self.assert_called('GET', '/snapshots/detail?'
'status=available&volume_id=1234')
'all_tenants=0&status=available&volume_id=1234')
def test_rename(self):
# basic rename with positional arguments
@ -483,7 +486,7 @@ class ShellTest(utils.TestCase):
def test_list_transfer(self):
self.run_command('transfer-list')
self.assert_called('GET', '/os-volume-transfer/detail')
self.assert_called('GET', '/os-volume-transfer/detail?all_tenants=0')
def test_list_transfer_all_tenants(self):
self.run_command('transfer-list --all-tenants=1')

2
cinderclient/tests/unit/v2/test_shell.py
View File

@ -1176,7 +1176,7 @@ class ShellTest(utils.TestCase):
def test_list_transfer(self):
self.run_command('transfer-list')
self.assert_called('GET', '/os-volume-transfer/detail')
self.assert_called('GET', '/os-volume-transfer/detail?all_tenants=0')
def test_list_transfer_all_tenants(self):
self.run_command('transfer-list --all-tenants=1')

29
cinderclient/tests/unit/v3/test_shell.py
View File

@ -131,37 +131,37 @@ class ShellTest(utils.TestCase):
{'command':
'list --filters name~=456',
'expected':
'/volumes/detail?name%7E=456'},
'/volumes/detail?name~=456'},
{'command':
u'list --filters name~=Σ',
'expected':
'/volumes/detail?name%7E=%CE%A3'},
'/volumes/detail?name~=%CE%A3'},
# testcases for list group
{'command':
'group-list --filters name=456',
'expected':
'/groups/detail?name=456'},
'/groups/detail?all_tenants=0&name=456'},
{'command':
'group-list --filters status=available',
'expected':
'/groups/detail?status=available'},
'/groups/detail?all_tenants=0&status=available'},
{'command':
'group-list --filters name~=456',
'expected':
'/groups/detail?name%7E=456'},
'/groups/detail?all_tenants=0&name~=456'},
# testcases for list group-snapshot
{'command':
'group-snapshot-list --status=error --filters status=available',
'expected':
'/group_snapshots/detail?status=available'},
'/group_snapshots/detail?all_tenants=0&status=available'},
{'command':
'group-snapshot-list --filters availability_zone=123',
'expected':
'/group_snapshots/detail?availability_zone=123'},
'/group_snapshots/detail?all_tenants=0&availability_zone=123'},
{'command':
'group-snapshot-list --filters status~=available',
'expected':
'/group_snapshots/detail?status%7E=available'},
'/group_snapshots/detail?all_tenants=0&status~=available'},
# testcases for list message
{'command':
'message-list --event_id=123 --filters event_id=456',
@ -174,7 +174,7 @@ class ShellTest(utils.TestCase):
{'command':
'message-list --filters request_id~=123',
'expected':
'/messages?request_id%7E=123'},
'/messages?request_id~=123'},
# testcases for list attachment
{'command':
'attachment-list --volume-id=123 --filters volume_id=456',
@ -187,7 +187,7 @@ class ShellTest(utils.TestCase):
{'command':
'attachment-list --filters volume_id~=456',
'expected':
'/attachments?volume_id%7E=456'},
'/attachments?volume_id~=456'},
# testcases for list backup
{'command':
'backup-list --volume-id=123 --filters volume_id=456',
@ -200,7 +200,7 @@ class ShellTest(utils.TestCase):
{'command':
'backup-list --filters volume_id~=456',
'expected':
'/backups/detail?volume_id%7E=456'},
'/backups/detail?volume_id~=456'},
# testcases for list snapshot
{'command':
'snapshot-list --volume-id=123 --filters volume_id=456',
@ -213,7 +213,7 @@ class ShellTest(utils.TestCase):
{'command':
'snapshot-list --filters volume_id~=456',
'expected':
'/snapshots/detail?volume_id%7E=456'},
'/snapshots/detail?volume_id~=456'},
# testcases for get pools
{'command':
'get-pools --filters name=456 --detail',
@ -632,7 +632,7 @@ class ShellTest(utils.TestCase):
def test_group_list(self):
self.run_command('--os-volume-api-version 3.13 group-list')
self.assert_called_anytime('GET', '/groups/detail')
self.assert_called_anytime('GET', '/groups/detail?all_tenants=0')
def test_group_list__with_all_tenant(self):
self.run_command(
@ -691,7 +691,8 @@ class ShellTest(utils.TestCase):
def test_group_snapshot_list(self):
self.run_command('--os-volume-api-version 3.14 group-snapshot-list')
self.assert_called_anytime('GET', '/group_snapshots/detail')
self.assert_called_anytime('GET',
'/group_snapshots/detail?all_tenants=0')
def test_group_snapshot_show(self):
self.run_command('--os-volume-api-version 3.14 '

18
cinderclient/utils.py
View File

@ -206,15 +206,27 @@ def unicode_key_value_to_string(src):
def build_query_param(params, sort=False):
"""parse list to url query parameters"""
if params is None:
params = {}
if not params:
return ""
if not sort:
param_list = list(params.items())
else:
param_list = list(sorted(params.items()))
query_string = parse.urlencode(
[(k, v) for (k, v) in param_list if v])
[(k, v) for (k, v) in param_list if v not in (None, '')])
# urllib's parse library used to adhere to RFC 2396 until
# python 3.7. The library moved from RFC 2396 to RFC 3986
# for quoting URL strings in python 3.7 and '~' is now
# included in the set of reserved characters. [1]
#
# Below ensures "~" is never encoded. See LP 1784728 [2] for more details.
# [1] https://docs.python.org/3/library/urllib.parse.html#url-quoting
# [2] https://bugs.launchpad.net/python-cinderclient/+bug/1784728
query_string = query_string.replace("%7E=", "~=")
if query_string:
query_string = "?%s" % (query_string,)

2
cinderclient/v1/volume_snapshots.py
View File

@ -107,7 +107,7 @@ class SnapshotManager(base.ManagerWithFind):
:rtype: list of :class:`Snapshot`
"""
query_string = utils.build_query_param(search_opts, True)
query_string = utils.build_query_param(search_opts, sort=True)
detail = ""
if detailed:

2
cinderclient/v1/volumes.py
View File

@ -203,7 +203,7 @@ class VolumeManager(base.ManagerWithFind):
if limit:
search_opts['limit'] = limit
query_string = utils.build_query_param(search_opts, True)
query_string = utils.build_query_param(search_opts, sort=True)
detail = ""
if detailed:

5
cinderclient/v2/limits.py
View File

@ -14,9 +14,8 @@
# limitations under the License.
"""Limits interface (v2 extension)"""
from six.moves.urllib import parse
from cinderclient import base
from cinderclient import utils
class Limits(base.Resource):
@ -95,6 +94,6 @@ class LimitsManager(base.Manager):
if tenant_id:
opts['tenant_id'] = tenant_id
query_string = "?%s" % parse.urlencode(opts) if opts else ""
query_string = utils.build_query_param(opts)
return self._get("/limits%s" % query_string, "limits")

2
cinderclient/v3/group_snapshots.py
View File

@ -96,7 +96,7 @@ class GroupSnapshotManager(base.ManagerWithFind):
:param search_opts: search options
:rtype: list of :class:`GroupSnapshot`
"""
query_string = utils.build_query_param(search_opts)
query_string = utils.build_query_param(search_opts, sort=True)
detail = ""
if detailed:

10
cinderclient/v3/groups.py
View File

@ -14,8 +14,6 @@
# under the License.
"""Group interface (v3 extension)."""
from six.moves.urllib import parse
from cinderclient import api_versions
from cinderclient.apiclient import base as common_base
from cinderclient import base
@ -140,11 +138,7 @@ class GroupManager(base.ManagerWithFind):
:rtype: :class:`Group`
"""
query_params = utils.unicode_key_value_to_string(kwargs)
query_string = ""
if query_params:
params = sorted(query_params.items(), key=lambda x: x[0])
query_string = "?%s" % parse.urlencode(params)
query_string = utils.build_query_param(query_params, sort=True)
return self._get("/groups/%s" % group_id + query_string,
"group")
@ -159,7 +153,7 @@ class GroupManager(base.ManagerWithFind):
if not search_opts:
search_opts = {}
search_opts['list_volume'] = True
query_string = utils.build_query_param(search_opts)
query_string = utils.build_query_param(search_opts, sort=True)
detail = ""
if detailed: