Unparseable endpoint URL's should raise friendly error

fixes bug #1058494

Change-Id: Id89c530e2f4e7dcf0db03515afb8b2a85fbf8077

keystone/catalog/backends/sql.py

@@ -16,6 +16,7 @@
 # under the License.
 
 from keystone import catalog
+from keystone.catalog import core
 from keystone.common import sql
 from keystone.common.sql import migration
 from keystone import config
@@ -158,10 +159,11 @@ class Catalog(sql.Base, catalog.Driver):
             internal_url = ep['internalurl'].replace('$(', '%(')
             public_url = ep['publicurl'].replace('$(', '%(')
             admin_url = ep['adminurl'].replace('$(', '%(')
-            catalog[region][srv_type]['id'] = ep['id']
-            catalog[region][srv_type]['name'] = srv_name
-            catalog[region][srv_type]['publicURL'] = public_url % d
-            catalog[region][srv_type]['adminURL'] = admin_url % d
-            catalog[region][srv_type]['internalURL'] = internal_url % d
+            srv_type = catalog[region][srv_type]
+            srv_type['id'] = ep['id']
+            srv_type['name'] = srv_name
+            srv_type['publicURL'] = core.format_url(public_url, d)
+            srv_type['adminURL'] = core.format_url(admin_url, d)
+            srv_type['internalURL'] = core.format_url(internal_url, d)
 
         return catalog

keystone/catalog/backends/templated.py

@@ -17,6 +17,7 @@
 import os.path
 
 from keystone.catalog.backends import kvs
+from keystone.catalog import core
 from keystone.common import logging
 from keystone import config
 
@@ -120,6 +121,6 @@ class TemplatedCatalog(kvs.Catalog):
                 o[region][service] = {}
                 for k, v in service_ref.iteritems():
                     v = v.replace('$(', '%(')
-                    o[region][service][k] = v % d
+                    o[region][service][k] = core.format_url(v, d)
 
         return o

keystone/catalog/core.py

@@ -19,6 +19,7 @@
 
 import uuid
 
+from keystone.common import logging
 from keystone.common import manager
 from keystone.common import wsgi
 from keystone import config
@@ -29,6 +30,27 @@ from keystone import token
 
 
 CONF = config.CONF
+LOG = logging.getLogger(__name__)
+
+
+def format_url(url, data):
+    """Helper Method for all Backend Catalog's to Deal with URLS"""
+    try:
+        result = url % data
+    except KeyError as e:
+        LOG.error("Malformed endpoint %s - unknown key %s" %
+                  (url, str(e)))
+        raise exception.MalformedEndpoint(endpoint=url)
+    except TypeError as e:
+        LOG.error("Malformed endpoint %s - type mismatch %s \
+                  (are you missing brackets ?)" %
+                  (url, str(e)))
+        raise exception.MalformedEndpoint(endpoint=url)
+    except ValueError as e:
+        LOG.error("Malformed endpoint %s - incomplete format \
+                  (are you missing a type notifier ?)" % url)
+        raise exception.MalformedEndpoint(endpoint=url)
+    return result
 
 
 class Manager(manager.Manager):

keystone/exception.py

@@ -123,6 +123,10 @@ class UnexpectedError(Error):
     title = 'Internal Server Error'
 
 
+class MalformedEndpoint(UnexpectedError):
+    """Malformed endpoint URL (see ERROR log for details): %(endpoint)s"""
+
+
 class NotImplemented(Error):
     """The action you have requested has not been implemented."""
     code = 501

tests/test_backend.py

@@ -18,7 +18,9 @@ import datetime
 import uuid
 import default_fixtures
 
+from keystone.catalog import core
 from keystone import exception
+from keystone import test
 from keystone.openstack.common import timeutils
 
 
@@ -760,6 +762,20 @@ class TokenTests(object):
                                         for x in xrange(2)])
 
 
+class CommonHelperTests(test.TestCase):
+    def test_format_helper_raises_malformed_on_missing_key(self):
+        with self.assertRaises(exception.MalformedEndpoint):
+            core.format_url("http://%(foo)s/%(bar)s", {"foo": "1"})
+
+    def test_format_helper_raises_malformed_on_wrong_type(self):
+        with self.assertRaises(exception.MalformedEndpoint):
+            core.format_url("http://%foo%s", {"foo": "1"})
+
+    def test_format_helper_raises_malformed_on_incomplete_format(self):
+        with self.assertRaises(exception.MalformedEndpoint):
+            core.format_url("http://%(foo)", {"foo": "1"})
+
+
 class CatalogTests(object):
     def test_service_crud(self):
         new_service = {

tests/test_backend_sql.py

@@ -166,3 +166,14 @@ class SqlCatalog(test.TestCase, test_backend.CatalogTests):
     def tearDown(self):
         sql_util.teardown_test_database()
         super(SqlCatalog, self).tearDown()
+
+    def test_malformed_catalog_throws_error(self):
+        self.catalog_api.create_service('a', {"id": "a", "desc": "a1",
+                                        "name": "b"})
+        badurl = "http://192.168.1.104:$(compute_port)s/v2/$(tenant)s"
+        self.catalog_api.create_endpoint('b', {"id": "b", "region": "b1",
+                                         "service_id": "a", "adminurl": badurl,
+                                         "internalurl": badurl,
+                                         "publicurl": badurl})
+        with self.assertRaises(exception.MalformedEndpoint):
+            self.catalog_api.get_catalog('fake-user', 'fake-tenant')

tests/test_backend_templated.py

@@ -62,6 +62,12 @@ class TestTemplatedCatalog(test.TestCase, test_backend.CatalogTests):
         catalog_ref = self.catalog_api.get_catalog('foo', 'bar')
         self.assertDictEqual(catalog_ref, self.DEFAULT_FIXTURE)
 
+    def test_malformed_catalog_throws_error(self):
+        self.catalog_api.templates['RegionOne']['compute']['adminURL'] = \
+            'http://localhost:$(compute_port)s/v1.1/$(tenant)s'
+        with self.assertRaises(exception.MalformedEndpoint):
+            self.catalog_api.get_catalog('fake-user', 'fake-tenant')
+
     def test_create_endpoint_404(self):
         self.assertRaises(exception.NotImplemented,
                           self.catalog_api.create_endpoint,