diff --git a/gnocchiclient/tests/unit/test_resource_id_encoding.py b/gnocchiclient/tests/unit/test_resource_id_encoding.py new file mode 100644 index 0000000..f1b26e8 --- /dev/null +++ b/gnocchiclient/tests/unit/test_resource_id_encoding.py @@ -0,0 +1,95 @@ +# +# 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. + +from unittest import mock + +import testtools + +from gnocchiclient import utils +from gnocchiclient.v1 import metric +from gnocchiclient.v1 import resource + +# A Cinder pool name: '@' and '#' are both meaningful in a URL, and an +# unencoded '#' truncates the path so the request hits another resource. +POOL = "ceph-aa@hybrid-rep3#hybrid-rep3" +ENCODED = "ceph-aa%40hybrid-rep3%23hybrid-rep3" + + +class TestEncodeResourceId(testtools.TestCase): + def test_encode(self): + self.assertEqual(ENCODED, utils.encode_resource_id(POOL)) + self.assertEqual("a-uuid-stays-as-is", + utils.encode_resource_id("a-uuid-stays-as-is")) + self.assertEqual("with%2Fslash", + utils.encode_resource_id("with/slash")) + + +class TestResourceUrls(testtools.TestCase): + def setUp(self): + super().setUp() + self.manager = resource.ResourceManager(mock.Mock()) + + def test_get(self): + with mock.patch.object(self.manager, "_get") as m: + self.manager.get("volume_provider_pool", POOL) + self.assertEqual("v1/resource/volume_provider_pool/" + ENCODED, + m.call_args[0][0]) + + def test_get_history(self): + with mock.patch.object(self.manager, "_get") as m: + self.manager.get("volume_provider_pool", POOL, history=True) + self.assertEqual( + "v1/resource/volume_provider_pool/" + ENCODED + "/history", + m.call_args[0][0]) + + def test_history(self): + with mock.patch.object(self.manager, "_get") as m: + self.manager.history("volume_provider_pool", POOL) + self.assertTrue(m.call_args[0][0].startswith( + "v1/resource/volume_provider_pool/" + ENCODED + "/history?")) + + def test_update(self): + with mock.patch.object(self.manager, "_patch") as m: + self.manager.update("volume_provider_pool", POOL, + {"provider": "ceph-aa@hybrid-rep3"}) + self.assertEqual("v1/resource/volume_provider_pool/" + ENCODED, + m.call_args[0][0]) + + def test_delete(self): + with mock.patch.object(self.manager, "_delete") as m: + self.manager.delete(POOL) + self.assertEqual("v1/resource/generic/" + ENCODED, + m.call_args[0][0]) + + +class TestMetricUrls(testtools.TestCase): + def setUp(self): + super().setUp() + self.manager = metric.MetricManager(mock.Mock()) + + def test_get_by_name(self): + with mock.patch.object(self.manager, "_get") as m: + self.manager.get("volume.provider.pool.capacity.total", + resource_id=POOL) + self.assertEqual( + "v1/resource/generic/" + ENCODED + + "/metric/volume.provider.pool.capacity.total", + m.call_args[0][0]) + + def test_create_on_resource(self): + with mock.patch.object(self.manager, "_post") as m: + m.return_value.json.return_value = [{}] + self.manager.create(name="volume.provider.pool.capacity.total", + resource_id=POOL) + self.assertEqual("v1/resource/generic/" + ENCODED + "/metric/", + m.call_args[0][0]) diff --git a/gnocchiclient/utils.py b/gnocchiclient/utils.py index c50a029..2ca3233 100644 --- a/gnocchiclient/utils.py +++ b/gnocchiclient/utils.py @@ -95,6 +95,18 @@ def dict_from_parsed_args(parsed_args, attrs): return d +def encode_resource_id(resource_id): + """Percent-encode a resource id for use in a URL path segment. + + Resource ids are free-form strings on the server side (anything that + is not a UUID gets hashed into one), and real-world ids carry + characters that mean something in a URL: Cinder pool names are + "@#", so an unencoded id truncates at '#' and + the request lands on a different (nonexistent) resource. + """ + return urllib.parse.quote(str(resource_id), safe="") + + def dict_to_querystring(objs): strings = [] for k, values in sorted(objs.items()): diff --git a/gnocchiclient/v1/metric.py b/gnocchiclient/v1/metric.py index e855790..8e257bb 100644 --- a/gnocchiclient/v1/metric.py +++ b/gnocchiclient/v1/metric.py @@ -77,7 +77,8 @@ def get(self, metric, resource_id=None): self._ensure_metric_is_uuid(metric) url = self.metric_url + metric else: - url = (self.resource_url % resource_id) + metric + url = (self.resource_url % + utils.encode_resource_id(resource_id)) + metric return self._get(url).json() # FIXME(jd): This is what create will be after debtcollector warnings have @@ -114,7 +115,7 @@ def _create_new(self, name=None, archive_policy_name=None, "Metric name is required if resource_id is set") return self._post( - self.resource_url % resource_id, + self.resource_url % utils.encode_resource_id(resource_id), headers={'Content-Type': "application/json"}, data=ujson.dumps({name: metric})).json()[0] @@ -168,7 +169,7 @@ def create(self, metric=None, refetch_metric=True, del metric['resource_id'] metric = {metric_name: metric} metric = self._post( - self.resource_url % resource_id, + self.resource_url % utils.encode_resource_id(resource_id), headers={'Content-Type': "application/json"}, data=ujson.dumps(metric)) return self.get(metric_name, resource_id) @@ -186,7 +187,8 @@ def delete(self, metric, resource_id=None): self._ensure_metric_is_uuid(metric) url = self.metric_url + metric else: - url = self.resource_url % resource_id + metric + url = (self.resource_url % + utils.encode_resource_id(resource_id)) + metric self._delete(url) def add_measures(self, metric, measures, resource_id=None): @@ -204,7 +206,9 @@ def add_measures(self, metric, measures, resource_id=None): self._ensure_metric_is_uuid(metric) url = self.metric_url + metric + "/measures" else: - url = self.resource_url % resource_id + metric + "/measures" + url = (self.resource_url % + utils.encode_resource_id(resource_id) + + metric + "/measures") return self._post( url, headers={'Content-Type': "application/json"}, data=ujson.dumps(measures)) @@ -272,7 +276,9 @@ def get_measures(self, metric, start=None, stop=None, aggregation=None, self._ensure_metric_is_uuid(metric) url = self.metric_url + metric + "/measures" else: - url = self.resource_url % resource_id + metric + "/measures" + url = (self.resource_url % + utils.encode_resource_id(resource_id) + + metric + "/measures") measures = self._get(url, params=params).json() return [(iso8601.parse_date(ts), g, value) for ts, g, value in measures] diff --git a/gnocchiclient/v1/resource.py b/gnocchiclient/v1/resource.py index cc2103b..50dc2a5 100644 --- a/gnocchiclient/v1/resource.py +++ b/gnocchiclient/v1/resource.py @@ -64,7 +64,8 @@ def get(self, resource_type, resource_id, history=False): :type history: bool """ history = "/history" if history else "" - url = self.url + "%s/%s%s" % (resource_type, resource_id, history) + url = self.url + "%s/%s%s" % ( + resource_type, utils.encode_resource_id(resource_id), history) return self._get(url).json() def history(self, resource_type, resource_id, details=False, @@ -88,8 +89,9 @@ def history(self, resource_type, resource_id, details=False, """ params = utils.build_pagination_options(details, False, limit, marker, sorts) - url = "%s%s/%s/history?%s" % (self.url, resource_type, resource_id, - utils.dict_to_querystring(params)) + url = "%s%s/%s/history?%s" % ( + self.url, resource_type, utils.encode_resource_id(resource_id), + utils.dict_to_querystring(params)) return self._get(url).json() def create(self, resource_type, resource): @@ -116,7 +118,8 @@ def update(self, resource_type, resource_id, resource): :type resource: dict """ return self._patch( - self.url + resource_type + "/" + resource_id, + self.url + resource_type + "/" + + utils.encode_resource_id(resource_id), headers={'Content-Type': "application/json"}, data=ujson.dumps(resource)).json() @@ -126,7 +129,8 @@ def delete(self, resource_id): :param resource_id: ID of the resource :type resource_id: str """ - self._delete(self.url + "generic/" + resource_id) + self._delete(self.url + "generic/" + + utils.encode_resource_id(resource_id)) def batch_delete(self, query, resource_type="generic"): """Delete a batch of resources based on attribute values.