Percent-encode resource ids in URL paths

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])

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
+    "<host>@<backend>#<pool>", 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()):

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]

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.