Merge lp:~billy-olsen/charm-helpers/better-address-endpoint-overrides into lp:charm-helpers

[Billy Olsen]

[lp:~billy-olsen/charm-helpers/better-address-endpoint-overrides]

387. By Billy Olsen on 2015-06-04

Add templating to the os-public-hostname override per jjo's bug comment.

[Jorge Niedbalski (niedbalski)]

Thanks billy for the proposal.

I think this approach is much simpler, +1 from my side.

review: Approve

[Jorge Niedbalski (niedbalski)]

The jinja package requires to be installed on the system, in case of partial importing of this helper, this will not work. Please see: charmhelpers.core.templating. I

This could be replaced by a standard string replacement. see pydoc format.

Thanks billy.

review: Needs Fixing

[lp:~billy-olsen/charm-helpers/better-address-endpoint-overrides]

388. By Billy Olsen on 2015-06-04

Per niedbalski's review remove jinja2 dependency and use simple string
formatting. Additionally break out the override check into its own
function.

[Jorge Niedbalski (niedbalski)]

review: Approve

=== modified file 'charmhelpers/contrib/openstack/ip.py'
--- charmhelpers/contrib/openstack/ip.py	2015-02-12 22:30:09 +0000
+++ charmhelpers/contrib/openstack/ip.py	2015-06-04 22:17:24 +0000
@@ -17,6 +17,7 @@
 from charmhelpers.core.hookenv import (
     config,
     unit_get,
+    service_name,
 )
 from charmhelpers.contrib.network.ip import (
     get_address_in_network,
@@ -26,8 +27,6 @@
 )
 from charmhelpers.contrib.hahelpers.cluster import is_clustered
 
-from functools import partial
-
 PUBLIC = 'public'
 INTERNAL = 'int'
 ADMIN = 'admin'
@@ -35,15 +34,18 @@
 ADDRESS_MAP = {
     PUBLIC: {
         'config': 'os-public-network',
-        'fallback': 'public-address'
+        'fallback': 'public-address',
+        'override': 'os-public-hostname',
     },
     INTERNAL: {
         'config': 'os-internal-network',
-        'fallback': 'private-address'
+        'fallback': 'private-address',
+        'override': 'os-internal-hostname',
     },
     ADMIN: {
         'config': 'os-admin-network',
-        'fallback': 'private-address'
+        'fallback': 'private-address',
+        'override': 'os-admin-hostname',
     }
 }
 
@@ -57,15 +59,50 @@
     :param endpoint_type: str endpoint type to resolve.
     :param returns: str base URL for services on the current service unit.
     """
-    scheme = 'http'
-    if 'https' in configs.complete_contexts():
-        scheme = 'https'
+    scheme = _get_scheme(configs)
+
     address = resolve_address(endpoint_type)
     if is_ipv6(address):
         address = "[{}]".format(address)
+
     return '%s://%s' % (scheme, address)
 
 
+def _get_scheme(configs):
+    """Returns the scheme to use for the url (either http or https)
+    depending upon whether https is in the configs value.
+
+    :param configs: OSTemplateRenderer config templating object to inspect
+                    for a complete https context.
+    :returns: either 'http' or 'https' depending on whether https is
+              configured within the configs context.
+    """
+    scheme = 'http'
+    if configs and 'https' in configs.complete_contexts():
+        scheme = 'https'
+    return scheme
+
+
+def _get_address_override(endpoint_type=PUBLIC):
+    """Returns any address overrides that the user has defined based on the
+    endpoint type.
+
+    Note: this function allows for the service name to be inserted into the
+    address if the user specifies {service_name}.somehost.org.
+
+    :param endpoint_type: the type of endpoint to retrieve the override
+                          value for.
+    :returns: any endpoint address or hostname that the user has overridden
+              or None if an override is not present.
+    """
+    override_key = ADDRESS_MAP[endpoint_type]['override']
+    addr_override = config(override_key)
+    if not addr_override:
+        return None
+    else:
+        return addr_override.format(service_name=service_name())
+
+
 def resolve_address(endpoint_type=PUBLIC):
     """Return unit address depending on net config.
 
@@ -77,7 +114,10 @@
 
     :param endpoint_type: Network endpoing type
     """
-    resolved_address = None
+    resolved_address = _get_address_override(endpoint_type)
+    if resolved_address:
+        return resolved_address
+
     vips = config('vip')
     if vips:
         vips = vips.split()
@@ -109,38 +149,3 @@
                          "clustered=%s)" % (net_type, clustered))
 
     return resolved_address
-
-
-def endpoint_url(configs, url_template, port, endpoint_type=PUBLIC,
-                 override=None):
-    """Returns the correct endpoint URL to advertise to Keystone.
-
-    This method provides the correct endpoint URL which should be advertised to
-    the keystone charm for endpoint creation. This method allows for the url to
-    be overridden to force a keystone endpoint to have specific URL for any of
-    the defined scopes (admin, internal, public).
-
-    :param configs: OSTemplateRenderer config templating object to inspect
-                    for a complete https context.
-    :param url_template: str format string for creating the url template. Only
-                         two values will be passed - the scheme+hostname
-                        returned by the canonical_url and the port.
-    :param endpoint_type: str endpoint type to resolve.
-    :param override: str the name of the config option which overrides the
-                     endpoint URL defined by the charm itself. None will
-                     disable any overrides (default).
-    """
-    if override:
-        # Return any user-defined overrides for the keystone endpoint URL.
-        user_value = config(override)
-        if user_value:
-            return user_value.strip()
-
-    return url_template % (canonical_url(configs, endpoint_type), port)
-
-
-public_endpoint = partial(endpoint_url, endpoint_type=PUBLIC)
-
-internal_endpoint = partial(endpoint_url, endpoint_type=INTERNAL)
-
-admin_endpoint = partial(endpoint_url, endpoint_type=ADMIN)

=== modified file 'tests/contrib/openstack/test_ip.py'
--- tests/contrib/openstack/test_ip.py	2015-02-12 22:30:09 +0000
+++ tests/contrib/openstack/test_ip.py	2015-06-04 22:17:24 +0000
@@ -7,7 +7,8 @@
     'config',
     'unit_get',
     'get_address_in_network',
-    'is_clustered'
+    'is_clustered',
+    'service_name',
 ]
 
 
@@ -94,6 +95,25 @@
         self.test_config.set('vip', '10.5.3.1')
         self.assertRaises(ValueError, ip.resolve_address)
 
+    def test_resolve_address_override(self):
+        self.test_config.set('os-public-hostname', 'public.example.com')
+        addr = ip.resolve_address()
+        self.assertEqual('public.example.com', addr)
+
+    def test_resolve_address_override_template(self):
+        self.test_config.set('os-public-hostname',
+                             '{service_name}.example.com')
+        self.service_name.return_value = 'foo'
+        addr = ip.resolve_address()
+        self.assertEqual('foo.example.com', addr)
+
+    @patch.object(ip, 'get_ipv6_addr', lambda *args, **kwargs: ['::1'])
+    def test_resolve_address_ipv6_fallback(self):
+        self.test_config.set('prefer-ipv6', True)
+        self.is_clustered.return_value = False
+        ip.resolve_address()
+        self.get_address_in_network.assert_called_with(None, '::1')
+
     @patch.object(ip, 'resolve_address')
     def test_canonical_url_http(self, resolve_address):
         resolve_address.return_value = 'unit1'
@@ -110,31 +130,8 @@
         self.assertTrue(ip.canonical_url(configs),
                         'https://unit1')
 
-    def test_endpoint_url(self):
-        exp_url = 'https://public.example.com'
-        self.test_config.set('public-url', exp_url)
-        configs = MagicMock()
-        configs.complete_contexts.return_value = ['https']
-        endpoint = ip.endpoint_url(configs, '%s:%s', 443, ip.PUBLIC,
-                                   'public-url')
-        self.assertEqual(exp_url, endpoint)
-
-    @patch.object(ip, 'resolve_address')
-    def test_endpoint_url_no_override(self, resolve_address):
-        resolve_address.return_value = 'unit1'
-        configs = MagicMock()
-        configs.complete_contexts.return_value = ['https']
-        endpoint = ip.endpoint_url(configs, '%s:%s/v1/AUTH_$(tenant_id)s', 443,
-                                   ip.INTERNAL, 'internal-url')
-        self.assertEqual('https://unit1:443/v1/AUTH_$(tenant_id)s', endpoint)
-
-    @patch.object(ip, 'resolve_address')
-    @patch.object(ip, 'is_ipv6')
-    def test_endpoint_url_no_override_ipv6(self, is_ipv6, resolve_address):
-        resolve_address.return_value = 'unit1'
-        is_ipv6.return_value = True
-        configs = MagicMock()
-        configs.complete_contexts.return_value = ['https']
-        endpoint = ip.endpoint_url(configs, '%s:%s', 443,
-                                   ip.ADMIN, 'admin-url')
-        self.assertEquals('https://[unit1]:443', endpoint)
+    @patch.object(ip, 'is_ipv6', lambda *args: True)
+    @patch.object(ip, 'resolve_address')
+    def test_canonical_url_ipv6(self, resolve_address):
+        resolve_address.return_value = 'unit1'
+        self.assertTrue(ip.canonical_url(None), 'http://[unit1]')