Don't add unformatted project-specific endpoints to catalog

If a token is domain-scoped, there is no project defined.
Hence for any catalog entries, we should skip any endpoints
that need a project id in order to be formatted correctly.

Co-Authored-By: Henry Nash <henryn@linux.vnet.ibm.com>
Change-Id: I3617a2509bfc4213f136b5c867c40d478a70ded8
Closes-Bug: #1261468

6 files changed, 148 insertions, 12 deletions

diff --git a/keystone/catalog/backends/sql.py b/keystone/catalog/backends/sql.py
index 8ab823059..aad8f6bf1 100644
--- a/keystone/catalog/backends/sql.py
+++ b/keystone/catalog/backends/sql.py
@@ -269,10 +269,29 @@ class Catalog(catalog.Driver):
         return ref.to_dict()
 
     def get_catalog(self, user_id, tenant_id):
+        """Retrieve and format the V2 service catalog.
+
+        :param user_id: The id of the user who has been authenticated for
+            creating service catalog.
+        :param tenant_id: The id of the project. 'tenant_id' will be None
+            in the case this being called to create a catalog to go in a
+            domain scoped token. In this case, any endpoint that requires
+            a tenant_id as part of their URL will be skipped (as would a whole
+            service if, as a consequence, it has no valid endpoints).
+
+        :returns: A nested dict representing the service catalog or an
+                  empty dict.
+
+        """
         substitutions = dict(
             itertools.chain(six.iteritems(CONF),
                             six.iteritems(CONF.eventlet_server)))
-        substitutions.update({'tenant_id': tenant_id, 'user_id': user_id})
+        substitutions.update({'user_id': user_id})
+        silent_keyerror_failures = []
+        if tenant_id:
+            substitutions.update({'tenant_id': tenant_id})
+        else:
+            silent_keyerror_failures = ['tenant_id']
 
         session = sql.get_session()
         endpoints = (session.query(Endpoint).
@@ -285,7 +304,13 @@ class Catalog(catalog.Driver):
             if not endpoint.service['enabled']:
                 continue
             try:
-                url = core.format_url(endpoint['url'], substitutions)
+                formatted_url = core.format_url(
+                    endpoint['url'], substitutions,
+                    silent_keyerror_failures=silent_keyerror_failures)
+                if formatted_url is not None:
+                    url = formatted_url
+                else:
+                    continue
             except exception.MalformedEndpoint:
                 continue  # this failure is already logged in format_url()
 
@@ -304,11 +329,27 @@ class Catalog(catalog.Driver):
         return catalog
 
     def get_v3_catalog(self, user_id, tenant_id):
+        """Retrieve and format the current V3 service catalog.
+
+        :param user_id: The id of the user who has been authenticated for
+            creating service catalog.
+        :param tenant_id: The id of the project. 'tenant_id' will be None in
+            the case this being called to create a catalog to go in a domain
+            scoped token. In this case, any endpoint that requires a
+            tenant_id as part of their URL will be skipped.
+
+        :returns: A list representing the service catalog or an empty list
+
+        """
         d = dict(
             itertools.chain(six.iteritems(CONF),
                             six.iteritems(CONF.eventlet_server)))
-        d.update({'tenant_id': tenant_id,
-                  'user_id': user_id})
+        d.update({'user_id': user_id})
+        silent_keyerror_failures = []
+        if tenant_id:
+            d.update({'tenant_id': tenant_id})
+        else:
+            silent_keyerror_failures = ['tenant_id']
 
         session = sql.get_session()
         services = (session.query(Service).filter(Service.enabled == true()).
@@ -322,12 +363,20 @@ class Catalog(catalog.Driver):
                 del endpoint['enabled']
                 endpoint['region'] = endpoint['region_id']
                 try:
-                    endpoint['url'] = core.format_url(endpoint['url'], d)
+                    formatted_url = core.format_url(
+                        endpoint['url'], d,
+                        silent_keyerror_failures=silent_keyerror_failures)
+                    if formatted_url:
+                        endpoint['url'] = formatted_url
+                    else:
+                        continue
                 except exception.MalformedEndpoint:
                     continue  # this failure is already logged in format_url()
 
                 yield endpoint
 
+        # TODO(davechen): If there is service with no endpoints, we should skip
+        # the service instead of keeping it in the catalog, see bug #1436704.
         def make_v3_service(svc):
             eps = list(make_v3_endpoints(svc.endpoints))
             service = {'endpoints': eps, 'id': svc.id, 'type': svc.type}
diff --git a/keystone/catalog/backends/templated.py b/keystone/catalog/backends/templated.py
index d3ee105d6..5707f932f 100644
--- a/keystone/catalog/backends/templated.py
+++ b/keystone/catalog/backends/templated.py
@@ -107,19 +107,44 @@ class Catalog(kvs.Catalog):
             raise
 
     def get_catalog(self, user_id, tenant_id):
+        """Retrieve and format the V2 service catalog.
+
+        :param user_id: The id of the user who has been authenticated for
+            creating service catalog.
+        :param tenant_id: The id of the project. 'tenant_id' will be None in
+            the case this being called to create a catalog to go in a domain
+            scoped token. In this case, any endpoint that requires a tenant_id
+            as part of their URL will be skipped.
+
+        :returns: A nested dict representing the service catalog or an
+                  empty dict.
+
+        """
         substitutions = dict(
             itertools.chain(six.iteritems(CONF),
                             six.iteritems(CONF.eventlet_server)))
-        substitutions.update({'tenant_id': tenant_id, 'user_id': user_id})
+        substitutions.update({'user_id': user_id})
+        silent_keyerror_failures = []
+        if tenant_id:
+            substitutions.update({'tenant_id': tenant_id})
+        else:
+            silent_keyerror_failures = ['tenant_id']
 
         catalog = {}
+        # TODO(davechen): If there is service with no endpoints, we should
+        # skip the service instead of keeping it in the catalog.
+        # see bug #1436704.
         for region, region_ref in six.iteritems(self.templates):
             catalog[region] = {}
             for service, service_ref in six.iteritems(region_ref):
                 service_data = {}
                 try:
                     for k, v in six.iteritems(service_ref):
-                        service_data[k] = core.format_url(v, substitutions)
+                        formatted_value = core.format_url(
+                            v, substitutions,
+                            silent_keyerror_failures=silent_keyerror_failures)
+                        if formatted_value:
+                            service_data[k] = formatted_value
                 except exception.MalformedEndpoint:
                     continue  # this failure is already logged in format_url()
                 catalog[region][service] = service_data
diff --git a/keystone/catalog/core.py b/keystone/catalog/core.py
index d4d6e6008..d31e924fe 100644
--- a/keystone/catalog/core.py
+++ b/keystone/catalog/core.py
@@ -37,11 +37,13 @@ LOG = log.getLogger(__name__)
 MEMOIZE = cache.get_memoization_decorator(section='catalog')
 
 
-def format_url(url, substitutions):
+def format_url(url, substitutions, silent_keyerror_failures=None):
     """Formats a user-defined URL with the given substitutions.
 
     :param string url: the URL to be formatted
     :param dict substitutions: the dictionary used for substitution
+    :param list silent_keyerror_failures: keys for which we should be silent
+        if there is a KeyError exception on substitution attempt
     :returns: a formatted URL
 
     """
@@ -54,6 +56,7 @@ def format_url(url, substitutions):
     substitutions = utils.WhiteListedItemFilter(
         WHITELISTED_PROPERTIES,
         substitutions)
+    allow_keyerror = silent_keyerror_failures or []
     try:
         result = url.replace('$(', '%(') % substitutions
     except AttributeError:
@@ -61,10 +64,14 @@ def format_url(url, substitutions):
                   {"url": url})
         raise exception.MalformedEndpoint(endpoint=url)
     except KeyError as e:
-        LOG.error(_LE("Malformed endpoint %(url)s - unknown key %(keyerror)s"),
-                  {"url": url,
-                   "keyerror": e})
-        raise exception.MalformedEndpoint(endpoint=url)
+        if not e.args or e.args[0] not in allow_keyerror:
+            LOG.error(_LE("Malformed endpoint %(url)s - unknown key "
+                          "%(keyerror)s"),
+                      {"url": url,
+                       "keyerror": e})
+            raise exception.MalformedEndpoint(endpoint=url)
+        else:
+            result = None
     except TypeError as e:
         LOG.error(_LE("Malformed endpoint '%(url)s'. The following type error "
                       "occurred during string substitution: %(typeerror)s"),
diff --git a/keystone/tests/unit/catalog/test_core.py b/keystone/tests/unit/catalog/test_core.py
index 99a34280b..ee49dfbaf 100644
--- a/keystone/tests/unit/catalog/test_core.py
+++ b/keystone/tests/unit/catalog/test_core.py
@@ -72,3 +72,17 @@ class FormatUrlTests(testtools.TestCase):
                           core.format_url,
                           url_template,
                           values)
+
+    def test_substitution_with_allowed_keyerror(self):
+        # No value of 'tenant_id' is passed into url_template.
+        # mod: format_url will return None instead of raising
+        # "MalformedEndpoint" exception.
+        # This is intentional behavior since we don't want to skip
+        # all the later endpoints once there is an URL of endpoint
+        # trying to replace 'tenant_id' with None.
+        url_template = ('http://$(public_bind_host)s:$(admin_port)d/'
+                        '$(tenant_id)s/$(user_id)s')
+        values = {'public_bind_host': 'server', 'admin_port': 9090,
+                  'user_id': 'B'}
+        self.assertIsNone(core.format_url(url_template, values,
+                          silent_keyerror_failures=['tenant_id']))
diff --git a/keystone/tests/unit/test_backend_templated.py b/keystone/tests/unit/test_backend_templated.py
index a1c15fb11..89e72992b 100644
--- a/keystone/tests/unit/test_backend_templated.py
+++ b/keystone/tests/unit/test_backend_templated.py
@@ -120,6 +120,31 @@ class TestTemplatedCatalog(tests.TestCase, test_backend.CatalogTests):
              'id': '1'}]
         self.assert_catalogs_equal(exp_catalog, catalog_ref)
 
+    def test_get_catalog_ignores_endpoints_with_invalid_urls(self):
+        user_id = uuid.uuid4().hex
+        # If the URL has no 'tenant_id' to substitute, we will skip the
+        # endpoint which contains this kind of URL.
+        catalog_ref = self.catalog_api.get_v3_catalog(user_id, tenant_id=None)
+        exp_catalog = [
+            {'endpoints': [],
+             'type': 'compute',
+             'name': "'Compute Service'",
+             'id': '2'},
+            {'endpoints': [
+                {'interface': 'admin',
+                 'region': 'RegionOne',
+                 'url': 'http://localhost:35357/v2.0'},
+                {'interface': 'public',
+                 'region': 'RegionOne',
+                 'url': 'http://localhost:5000/v2.0'},
+                {'interface': 'internal',
+                 'region': 'RegionOne',
+                 'url': 'http://localhost:35357/v2.0'}],
+             'type': 'identity',
+             'name': "'Identity Service'",
+             'id': '1'}]
+        self.assert_catalogs_equal(exp_catalog, catalog_ref)
+
     def test_list_regions_filtered_by_parent_region_id(self):
         self.skipTest('Templated backend does not support hints')
 
diff --git a/keystone/tests/unit/test_v3_catalog.py b/keystone/tests/unit/test_v3_catalog.py
index 4e30cb0d9..44afce311 100644
--- a/keystone/tests/unit/test_v3_catalog.py
+++ b/keystone/tests/unit/test_v3_catalog.py
@@ -15,6 +15,8 @@
 import copy
 import uuid
 
+from testtools import matchers
+
 from keystone import catalog
 from keystone.tests import unit as tests
 from keystone.tests.unit.ksfixtures import database
@@ -676,6 +678,20 @@ class TestCatalogAPISQL(tests.TestCase):
         # all three appear in the backend
         self.assertEqual(3, len(self.catalog_api.list_endpoints()))
 
+        # create another valid endpoint - tenant_id will be replaced
+        ref = self.new_endpoint_ref(self.service_id)
+        ref['url'] = 'http://keystone/%(tenant_id)s'
+        self.catalog_api.create_endpoint(ref['id'], ref)
+
+        # there are two valid endpoints, positive check
+        catalog = self.catalog_api.get_v3_catalog(user_id, tenant_id)
+        self.assertThat(catalog[0]['endpoints'], matchers.HasLength(2))
+
+        # If the URL has no 'tenant_id' to substitute, we will skip the
+        # endpoint which contains this kind of URL, negative check.
+        catalog = self.catalog_api.get_v3_catalog(user_id, tenant_id=None)
+        self.assertThat(catalog[0]['endpoints'], matchers.HasLength(1))
+
     def test_get_catalog_always_returns_service_name(self):
         user_id = uuid.uuid4().hex
         tenant_id = uuid.uuid4().hex