openstack/python-manilaclient
Code Issues Proposed changes

Stop encoding "~" in 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.

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

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

Closes-Bug: #1785283
Change-Id: I6df5d543ae94ed1fa966c8019a52e9fca19e387e
This commit is contained in:
Goutham Pacha Ravi 2018-10-08 15:09:39 -07:00
parent 4132b2186a
commit 80ec2919ca
20 changed files with 73 additions and 172 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
manilaclient/base.py
View File

@ -22,7 +22,6 @@ Base utilities to build API operation managers and objects on top of.
import contextlib
import hashlib
import os
from six.moves.urllib import parse
from manilaclient.common import cliutils
from manilaclient import exceptions
@ -180,14 +179,11 @@ class Manager(utils.HookableMixin):
return self.resource_class(self, body)
def _build_query_string(self, search_opts):
query_string = ""
if search_opts:
search_opts = utils.unicode_key_value_to_string(search_opts)
params = sorted(
[(k, v) for (k, v) in search_opts.items() if v])
if params:
query_string = "?%s" % parse.urlencode(params)
search_opts = search_opts or {}
search_opts = utils.unicode_key_value_to_string(search_opts)
params = sorted(
[(k, v) for (k, v) in search_opts.items() if v])
query_string = "?%s" % utils.safe_urlencode(params) if params else ''
return query_string

6
manilaclient/common/apiclient/base.py
View File

@ -28,10 +28,10 @@ import copy
from oslo_utils import strutils
import six
from six.moves.urllib import parse
from manilaclient.common._i18n import _
from manilaclient.common.apiclient import exceptions
from manilaclient import utils
def getid(obj):
@ -339,7 +339,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': '?%s' % utils.safe_urlencode(kwargs) if kwargs else ''
},
self.collection_key)
@ -378,7 +378,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.safe_urlencode(kwargs) if kwargs else ''
},
self.collection_key)
num = len(rl)

4
manilaclient/tests/unit/v2/test_shares.py
View File

@ -297,8 +297,8 @@ class SharesTest(utils.TestCase):
cs.shares.list(detailed=False, search_opts=search_opts)
cs.assert_called(
'GET',
'/shares?description%7E=fake_description&fake_int=1&'
'fake_str=fake_str_value&is_public=True&name%7E=fake_name')
'/shares?description~=fake_description&fake_int=1&'
'fake_str=fake_str_value&is_public=True&name~=fake_name')
@ddt.data(('id', 'b4991315-eb7d-43ec-979e-5715d4399827', True),
('id', 'b4991315-eb7d-43ec-979e-5715d4399827', False),

40
manilaclient/tests/unit/v2/test_shell.py
View File

@ -21,7 +21,6 @@ import itertools
import mock
from oslo_utils import strutils
import six
from six.moves.urllib import parse
from manilaclient import api_versions
from manilaclient import client
@ -32,6 +31,7 @@ from manilaclient import exceptions
from manilaclient import shell
from manilaclient.tests.unit import utils as test_utils
from manilaclient.tests.unit.v2 import fakes
from manilaclient import utils
from manilaclient.v2 import messages
from manilaclient.v2 import security_services
from manilaclient.v2 import share_instances
@ -258,7 +258,7 @@ class ShellTest(test_utils.TestCase):
'fake_name')
self.assert_called(
'GET',
'/shares/detail?name%7E=fake_name')
'/shares/detail?name~=fake_name')
def test_list_filter_by_inexact_description(self):
for separator in self.separators:
@ -266,7 +266,7 @@ class ShellTest(test_utils.TestCase):
'fake_description')
self.assert_called(
'GET',
'/shares/detail?description%7E=fake_description')
'/shares/detail?description~=fake_description')
def test_list_filter_by_inexact_unicode_name(self):
for separator in self.separators:
@ -274,7 +274,7 @@ class ShellTest(test_utils.TestCase):
u'ффф')
self.assert_called(
'GET',
'/shares/detail?name%7E=%D1%84%D1%84%D1%84')
'/shares/detail?name~=%D1%84%D1%84%D1%84')
def test_list_filter_by_inexact_unicode_description(self):
for separator in self.separators:
@ -282,7 +282,7 @@ class ShellTest(test_utils.TestCase):
u'ффф')
self.assert_called(
'GET',
'/shares/detail?description%7E=%D1%84%D1%84%D1%84')
'/shares/detail?description~=%D1%84%D1%84%D1%84')
def test_list_filter_by_share_type_not_found(self):
for separator in self.separators:
@ -291,7 +291,7 @@ class ShellTest(test_utils.TestCase):
self.run_command,
'list --share-type' + separator + 'not_found_expected',
)
self.assert_called('GET', '/types?is_public=all&all_tenants=1')
self.assert_called('GET', '/types?all_tenants=1&is_public=all')
def test_list_with_limit(self):
for separator in self.separators:
@ -846,7 +846,7 @@ class ShellTest(test_utils.TestCase):
'fake_name')
self.assert_called(
'GET',
'/snapshots/detail?name%7E=fake_name')
'/snapshots/detail?name~=fake_name')
def test_list_snapshots_filter_by_inexact_description(self):
for separator in self.separators:
@ -854,7 +854,7 @@ class ShellTest(test_utils.TestCase):
'fake_description')
self.assert_called(
'GET',
'/snapshots/detail?description%7E=fake_description')
'/snapshots/detail?description~=fake_description')
def test_list_snapshots_filter_by_inexact_unicode_name(self):
for separator in self.separators:
@ -862,7 +862,7 @@ class ShellTest(test_utils.TestCase):
u'ффф')
self.assert_called(
'GET',
'/snapshots/detail?name%7E=%D1%84%D1%84%D1%84')
'/snapshots/detail?name~=%D1%84%D1%84%D1%84')
def test_list_snapshots_filter_by_inexact_unicode_description(self):
for separator in self.separators:
@ -870,7 +870,7 @@ class ShellTest(test_utils.TestCase):
u'ффф')
self.assert_called(
'GET',
'/snapshots/detail?description%7E=%D1%84%D1%84%D1%84')
'/snapshots/detail?description~=%D1%84%D1%84%D1%84')
def test_list_snapshots_with_sort_dir_verify_keys(self):
aliases = ['--sort_dir', '--sort-dir']
@ -1520,8 +1520,8 @@ class ShellTest(test_utils.TestCase):
command_str += ' --%(key)s=%(value)s' % {'key': key,
'value': value}
self.run_command(command_str)
query = parse.urlencode(sorted([(k.replace('-', '_'), v) for (k, v)
in list(filters.items())]))
query = utils.safe_urlencode(sorted([(k.replace('-', '_'), v) for
(k, v) in filters.items()]))
self.assert_called(
'GET',
'/share-networks/detail?%s' % query,
@ -1536,7 +1536,7 @@ class ShellTest(test_utils.TestCase):
'fake_name')
self.assert_called(
'GET',
'/share-networks/detail?name%7E=fake_name')
'/share-networks/detail?name~=fake_name')
def test_share_network_list_filter_by_inexact_description(self):
for separator in self.separators:
@ -1544,7 +1544,7 @@ class ShellTest(test_utils.TestCase):
'fake_description')
self.assert_called(
'GET',
'/share-networks/detail?description%7E=fake_description')
'/share-networks/detail?description~=fake_description')
def test_share_network_list_filter_by_inexact_unicode_name(self):
for separator in self.separators:
@ -1552,7 +1552,7 @@ class ShellTest(test_utils.TestCase):
u'ффф')
self.assert_called(
'GET',
'/share-networks/detail?name%7E=%D1%84%D1%84%D1%84')
'/share-networks/detail?name~=%D1%84%D1%84%D1%84')
def test_share_network_list_filter_by_inexact_unicode_description(self):
for separator in self.separators:
@ -1560,7 +1560,7 @@ class ShellTest(test_utils.TestCase):
u'ффф')
self.assert_called(
'GET',
'/share-networks/detail?description%7E=%D1%84%D1%84%D1%84')
'/share-networks/detail?description~=%D1%84%D1%84%D1%84')
def test_share_network_security_service_add(self):
self.run_command('share-network-security-service-add fake_share_nw '
@ -2152,7 +2152,7 @@ class ShellTest(test_utils.TestCase):
'fake_name')
self.assert_called(
'GET',
'/share-groups/detail?name%7E=fake_name')
'/share-groups/detail?name~=fake_name')
def test_share_group_list_filter_by_inexact_description(self):
for separator in self.separators:
@ -2160,7 +2160,7 @@ class ShellTest(test_utils.TestCase):
'fake_description')
self.assert_called(
'GET',
'/share-groups/detail?description%7E=fake_description')
'/share-groups/detail?description~=fake_description')
def test_share_group_list_filter_by_inexact_unicode_name(self):
for separator in self.separators:
@ -2168,7 +2168,7 @@ class ShellTest(test_utils.TestCase):
u'ффф')
self.assert_called(
'GET',
'/share-groups/detail?name%7E=%D1%84%D1%84%D1%84')
'/share-groups/detail?name~=%D1%84%D1%84%D1%84')
def test_share_group_list_filter_by_inexact_unicode_description(self):
for separator in self.separators:
@ -2176,7 +2176,7 @@ class ShellTest(test_utils.TestCase):
u'ффф')
self.assert_called(
'GET',
'/share-groups/detail?description%7E=%D1%84%D1%84%D1%84')
'/share-groups/detail?description~=%D1%84%D1%84%D1%84')
def test_share_group_show(self):
self.run_command('share-group-show 1234')

2
manilaclient/tests/unit/v2/test_types.py
View File

@ -153,7 +153,7 @@ class TypesTest(utils.TestCase):
def test_list_types_search_by_extra_specs(self):
search_opts = {'extra_specs': {'aa': 'bb'}}
cs.share_types.list(search_opts=search_opts)
expect = '/types?is_public=all&extra_specs=%7B%27aa%27%3A+%27bb%27%7D'
expect = '/types?extra_specs=%7B%27aa%27%3A+%27bb%27%7D&is_public=all'
cs.assert_called('GET', expect)
@ddt.data(*get_valid_type_create_data_2_0())

22
manilaclient/utils.py
View File

@ -11,6 +11,7 @@
# under the License.
import six
from six.moves.urllib import parse
class HookableMixin(object):
@ -69,3 +70,24 @@ def unicode_key_value_to_string(src):
if isinstance(src, list):
return [unicode_key_value_to_string(l) for l in src]
return _encode(src)
def safe_urlencode(params_dict):
"""Workaround incompatible change to urllib.parse
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]
This utility ensures "~" is never encoded.
See LP 1785283 [2] for more details.
[1] https://docs.python.org/3/library/urllib.parse.html#url-quoting
[2] https://bugs.launchpad.net/python-manilaclient/+bug/1785283
:param params_dict can be a list of (k,v) tuples, or a dictionary
"""
parsed_params = parse.urlencode(params_dict)
return parsed_params.replace("%7E", "~")

14
manilaclient/v2/messages.py
View File

@ -11,12 +11,6 @@
# under the License.
"""Asynchronous User Message interface."""
try:
from urllib import urlencode # noqa
except ImportError:
from urllib.parse import urlencode # noqa
from manilaclient import api_versions
from manilaclient import base
from manilaclient.common.apiclient import base as common_base
@ -76,13 +70,7 @@ class MessageManager(base.ManagerWithFind):
raise ValueError('sort_dir must be one of the following: %s.'
% ', '.join(constants.SORT_DIR_VALUES))
if search_opts:
query_string = urlencode(
sorted([(k, v) for (k, v) in list(search_opts.items()) if v]))
if query_string:
query_string = "?%s" % (query_string,)
else:
query_string = ''
query_string = self._build_query_string(search_opts)
path = RESOURCES_PATH + query_string
return self._list(path, RESOURCES_NAME)

15
manilaclient/v2/scheduler_stats.py
View File

@ -12,12 +12,9 @@
# License for the specific language governing permissions and limitations
# under the License.
import six.moves.urllib.parse as urlparse
from manilaclient import base
from manilaclient.common.apiclient import base as common_base
RESOURCES_PATH = '/scheduler-stats/pools'
RESOURCES_NAME = 'pools'
@ -37,17 +34,7 @@ class PoolManager(base.Manager):
:rtype: list of :class:`Pool`
"""
if search_opts is None:
search_opts = {}
if search_opts:
query_string = urlparse.urlencode(
sorted([(k, v) for (k, v) in list(search_opts.items()) if v]))
if query_string:
query_string = "?%s" % (query_string,)
else:
query_string = ''
query_string = self._build_query_string(search_opts)
if detailed:
path = '%(resources_path)s/detail%(query)s' % {
'resources_path': RESOURCES_PATH,

13
manilaclient/v2/security_services.py
View File

@ -13,11 +13,6 @@
# License for the specific language governing permissions and limitations
# under the License.
try:
from urllib import urlencode # noqa
except ImportError:
from urllib.parse import urlencode # noqa
from manilaclient import base
from manilaclient.common.apiclient import base as common_base
from manilaclient import exceptions
@ -152,13 +147,7 @@ class SecurityServiceManager(base.ManagerWithFind):
:rtype: list of :class:`SecurityService`
"""
if search_opts:
query_string = urlencode(
sorted([(k, v) for (k, v) in list(search_opts.items()) if v]))
if query_string:
query_string = "?%s" % query_string
else:
query_string = ''
query_string = self._build_query_string(search_opts)
if detailed:
path = RESOURCES_PATH + "/detail" + query_string

12
manilaclient/v2/services.py
View File

@ -13,11 +13,6 @@
# License for the specific language governing permissions and limitations
# under the License.
try:
from urllib import urlencode # noqa
except ImportError:
from urllib.parse import urlencode # noqa
from manilaclient import api_versions
from manilaclient import base
from manilaclient.common.apiclient import base as common_base
@ -46,12 +41,7 @@ class ServiceManager(base.Manager):
:rtype: list of :class:`Service`
"""
query_string = ''
if search_opts:
query_string = urlencode(
sorted([(k, v) for (k, v) in search_opts.items() if v]))
if query_string:
query_string = "?%s" % query_string
query_string = self._build_query_string(search_opts)
return self._list(resource_path + query_string, RESOURCE_NAME)
@api_versions.wraps("1.0", "2.6")

21
manilaclient/v2/share_access_rules.py
View File

@ -17,20 +17,16 @@
# limitations under the License.
"""Interface for share access rules extension."""
from six.moves.urllib import parse
from manilaclient import api_versions
from manilaclient import base
from manilaclient.common.apiclient import base as common_base
from manilaclient import utils
RESOURCE_PATH = '/share-access-rules/%s'
RESOURCE_NAME = 'access'
RESOURCES_METADATA_PATH = '/share-access-rules/%s/metadata'
RESOURCE_METADATA_PATH = '/share-access-rules/%s/metadata/%s'
RESOURCE_LIST_PATH = '/share-access-rules?share_id=%s'
RESOURCE_LIST_PATH = '/share-access-rules'
class ShareAccessRule(common_base.Resource):
@ -86,14 +82,9 @@ class ShareAccessRuleManager(base.ManagerWithFind):
self._delete(url)
@api_versions.wraps("2.45")
def access_list(self, share, search_opts):
query_string = ""
if search_opts:
search_opts = utils.unicode_key_value_to_string(search_opts)
params = sorted(
[(k, v) for (k, v) in list(search_opts.items()) if v])
if params:
query_string = "&%s" % parse.urlencode(params)
url = RESOURCE_LIST_PATH % common_base.getid(share) + query_string
def access_list(self, share, search_opts=None):
search_opts = search_opts or {}
search_opts['share_id'] = common_base.getid(share)
query_string = self._build_query_string(search_opts)
url = RESOURCE_LIST_PATH + query_string
return self._list(url, 'access_list')

3
manilaclient/v2/share_group_snapshots.py
View File

@ -102,8 +102,7 @@ class ShareGroupSnapshotManager(base.ManagerWithFind):
:rtype: list of :class:`ShareGroupSnapshot`
"""
if not search_opts:
search_opts = {}
search_opts = search_opts or {}
if sort_key is not None:
if sort_key in constants.SHARE_GROUP_SNAPSHOT_SORT_KEY_VALUES:

3
manilaclient/v2/share_groups.py
View File

@ -139,8 +139,7 @@ class ShareGroupManager(base.ManagerWithFind):
:rtype: list of :class:`ShareGroup`
"""
if not search_opts:
search_opts = {}
search_opts = search_opts or {}
if sort_key is not None:
if sort_key in constants.SHARE_GROUP_SORT_KEY_VALUES:

15
manilaclient/v2/share_networks.py
View File

@ -13,14 +13,10 @@
# License for the specific language governing permissions and limitations
# under the License.
from six.moves.urllib import parse
from manilaclient import api_versions
from manilaclient import base
from manilaclient.common.apiclient import base as common_base
from manilaclient import exceptions
from manilaclient import utils
RESOURCES_PATH = '/share-networks'
RESOURCE_PATH = "/share-networks/%s"
@ -222,16 +218,7 @@ class ShareNetworkManager(base.ManagerWithFind):
:rtype: list of :class:`NetworkInfo`
"""
if not search_opts:
search_opts = {}
query_string = ""
if search_opts:
search_opts = utils.unicode_key_value_to_string(search_opts)
params = sorted(
[(k, v) for (k, v) in list(search_opts.items()) if v])
if params:
query_string = "?%s" % parse.urlencode(params)
query_string = self._build_query_string(search_opts)
if detailed:
path = RESOURCES_PATH + "/detail" + query_string

12
manilaclient/v2/share_servers.py
View File

@ -13,11 +13,6 @@
# License for the specific language governing permissions and limitations
# under the License.
try:
from urllib import urlencode # noqa
except ImportError:
from urllib.parse import urlencode # noqa
from manilaclient import base
from manilaclient.common.apiclient import base as common_base
@ -89,10 +84,5 @@ class ShareServerManager(base.ManagerWithFind):
:rtype: list of :class:`ShareServer`
"""
query_string = ''
if search_opts:
opts = sorted(
[(k, v) for (k, v) in search_opts.items() if v])
query_string = urlencode(opts)
query_string = '?' + query_string if query_string else ''
query_string = self._build_query_string(search_opts)
return self._list(RESOURCES_PATH + query_string, RESOURCES_NAME)

5
manilaclient/v2/share_snapshot_export_locations.py
View File

@ -13,11 +13,6 @@
# License for the specific language governing permissions and limitations
# under the License.
try:
from urllib import urlencode # noqa
except ImportError:
from urllib.parse import urlencode # noqa
from manilaclient import api_versions
from manilaclient import base
from manilaclient.common.apiclient import base as common_base

5
manilaclient/v2/share_snapshot_instance_export_locations.py
View File

@ -13,11 +13,6 @@
# License for the specific language governing permissions and limitations
# under the License.
try:
from urllib import urlencode # noqa
except ImportError:
from urllib.parse import urlencode # noqa
from manilaclient import api_versions
from manilaclient import base
from manilaclient.common.apiclient import base as common_base

14
manilaclient/v2/share_snapshots.py
View File

@ -14,13 +14,10 @@
# under the License.
"""Interface for shares extension."""
from six.moves.urllib import parse
from manilaclient import api_versions
from manilaclient import base
from manilaclient.common.apiclient import base as common_base
from manilaclient.common import constants
from manilaclient import utils
class ShareSnapshot(common_base.Resource):
@ -132,8 +129,7 @@ class ShareSnapshotManager(base.ManagerWithFind):
:param sort_dir: Sort direction, should be 'desc' or 'asc'.
:rtype: list of :class:`ShareSnapshot`
"""
if search_opts is None:
search_opts = {}
search_opts = search_opts or {}
if sort_key is not None:
if sort_key in constants.SNAPSHOT_SORT_KEY_VALUES:
@ -151,13 +147,7 @@ class ShareSnapshotManager(base.ManagerWithFind):
'sort_dir must be one of the following: %s.'
% ', '.join(constants.SORT_DIR_VALUES))
query_string = ""
if search_opts:
search_opts = utils.unicode_key_value_to_string(search_opts)
params = sorted(
[(k, v) for (k, v) in list(search_opts.items()) if v])
if params:
query_string = "?%s" % parse.urlencode(params)
query_string = self._build_query_string(search_opts)
if detailed:
path = "/snapshots/detail%s" % (query_string,)

15
manilaclient/v2/share_types.py
View File

@ -17,13 +17,11 @@
"""
Share Type interface.
"""
from six.moves.urllib import parse
from manilaclient import api_versions
from manilaclient import base
from manilaclient.common.apiclient import base as common_base
from manilaclient import exceptions
from manilaclient import utils
class ShareType(common_base.Resource):
@ -114,17 +112,10 @@ class ShareTypeManager(base.ManagerWithFind):
:rtype: list of :class:`ShareType`.
"""
query_string = ''
search_opts = search_opts or {}
if show_all:
query_string = '?is_public=all'
if search_opts:
search_opts = utils.unicode_key_value_to_string(search_opts)
params = sorted(
[(k, v) for (k, v) in list(search_opts.items()) if v])
if params and query_string:
query_string += "&%s" % parse.urlencode(params)
if params and not query_string:
query_string += "?%s" % parse.urlencode(params)
search_opts['is_public'] = 'all'
query_string = self._build_query_string(search_opts)
return self._list("/types%s" % query_string, "share_types")
def show(self, share_type):

10
manilaclient/v2/shares.py
View File

@ -19,7 +19,6 @@ import ipaddress
from oslo_utils import uuidutils
import re
import six
from six.moves.urllib import parse
import string
from manilaclient import api_versions
@ -27,7 +26,6 @@ from manilaclient import base
from manilaclient.common.apiclient import base as common_base
from manilaclient.common import constants
from manilaclient import exceptions
from manilaclient import utils
from manilaclient.v2 import share_instances
@ -398,13 +396,7 @@ class ShareManager(base.ManagerWithFind):
else:
search_opts['export_location_path'] = export_location
query_string = ""
if search_opts:
search_opts = utils.unicode_key_value_to_string(search_opts)
params = sorted(
[(k, v) for (k, v) in list(search_opts.items()) if v])
if params:
query_string = "?%s" % parse.urlencode(params)
query_string = self._build_query_string(search_opts)
if detailed:
path = "/shares/detail%s" % (query_string,)