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

Proposed by Billy Olsen
Status: Merged
Merged at revision: 383
Proposed branch: lp:~billy-olsen/charm-helpers/better-address-endpoint-overrides
Merge into: lp:charm-helpers
Diff against target: 224 lines (+75/-73)
2 files modified
charmhelpers/contrib/openstack/ip.py (+49/-44)
tests/contrib/openstack/test_ip.py (+26/-29)
To merge this branch: bzr merge lp:~billy-olsen/charm-helpers/better-address-endpoint-overrides
Reviewer Review Type Date Requested Status
Jorge Niedbalski (community) Approve
Felipe Reyes Pending
Edward Hope-Morley Pending
charmers Pending
Review via email: mp+261000@code.launchpad.net

This proposal supersedes a proposal from 2015-05-29.

To post a comment you must log in.
387. By Billy Olsen

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

Revision history for this message
Jorge Niedbalski (niedbalski) wrote :

Thanks billy for the proposal.

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

review: Approve
Revision history for this message
Jorge Niedbalski (niedbalski) wrote :

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
388. By Billy Olsen

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

Revision history for this message
Billy Olsen (billy-olsen) :
Revision history for this message
Jorge Niedbalski (niedbalski) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charmhelpers/contrib/openstack/ip.py'
2--- charmhelpers/contrib/openstack/ip.py 2015-02-12 22:30:09 +0000
3+++ charmhelpers/contrib/openstack/ip.py 2015-06-04 22:17:24 +0000
4@@ -17,6 +17,7 @@
5 from charmhelpers.core.hookenv import (
6 config,
7 unit_get,
8+ service_name,
9 )
10 from charmhelpers.contrib.network.ip import (
11 get_address_in_network,
12@@ -26,8 +27,6 @@
13 )
14 from charmhelpers.contrib.hahelpers.cluster import is_clustered
15
16-from functools import partial
17-
18 PUBLIC = 'public'
19 INTERNAL = 'int'
20 ADMIN = 'admin'
21@@ -35,15 +34,18 @@
22 ADDRESS_MAP = {
23 PUBLIC: {
24 'config': 'os-public-network',
25- 'fallback': 'public-address'
26+ 'fallback': 'public-address',
27+ 'override': 'os-public-hostname',
28 },
29 INTERNAL: {
30 'config': 'os-internal-network',
31- 'fallback': 'private-address'
32+ 'fallback': 'private-address',
33+ 'override': 'os-internal-hostname',
34 },
35 ADMIN: {
36 'config': 'os-admin-network',
37- 'fallback': 'private-address'
38+ 'fallback': 'private-address',
39+ 'override': 'os-admin-hostname',
40 }
41 }
42
43@@ -57,15 +59,50 @@
44 :param endpoint_type: str endpoint type to resolve.
45 :param returns: str base URL for services on the current service unit.
46 """
47- scheme = 'http'
48- if 'https' in configs.complete_contexts():
49- scheme = 'https'
50+ scheme = _get_scheme(configs)
51+
52 address = resolve_address(endpoint_type)
53 if is_ipv6(address):
54 address = "[{}]".format(address)
55+
56 return '%s://%s' % (scheme, address)
57
58
59+def _get_scheme(configs):
60+ """Returns the scheme to use for the url (either http or https)
61+ depending upon whether https is in the configs value.
62+
63+ :param configs: OSTemplateRenderer config templating object to inspect
64+ for a complete https context.
65+ :returns: either 'http' or 'https' depending on whether https is
66+ configured within the configs context.
67+ """
68+ scheme = 'http'
69+ if configs and 'https' in configs.complete_contexts():
70+ scheme = 'https'
71+ return scheme
72+
73+
74+def _get_address_override(endpoint_type=PUBLIC):
75+ """Returns any address overrides that the user has defined based on the
76+ endpoint type.
77+
78+ Note: this function allows for the service name to be inserted into the
79+ address if the user specifies {service_name}.somehost.org.
80+
81+ :param endpoint_type: the type of endpoint to retrieve the override
82+ value for.
83+ :returns: any endpoint address or hostname that the user has overridden
84+ or None if an override is not present.
85+ """
86+ override_key = ADDRESS_MAP[endpoint_type]['override']
87+ addr_override = config(override_key)
88+ if not addr_override:
89+ return None
90+ else:
91+ return addr_override.format(service_name=service_name())
92+
93+
94 def resolve_address(endpoint_type=PUBLIC):
95 """Return unit address depending on net config.
96
97@@ -77,7 +114,10 @@
98
99 :param endpoint_type: Network endpoing type
100 """
101- resolved_address = None
102+ resolved_address = _get_address_override(endpoint_type)
103+ if resolved_address:
104+ return resolved_address
105+
106 vips = config('vip')
107 if vips:
108 vips = vips.split()
109@@ -109,38 +149,3 @@
110 "clustered=%s)" % (net_type, clustered))
111
112 return resolved_address
113-
114-
115-def endpoint_url(configs, url_template, port, endpoint_type=PUBLIC,
116- override=None):
117- """Returns the correct endpoint URL to advertise to Keystone.
118-
119- This method provides the correct endpoint URL which should be advertised to
120- the keystone charm for endpoint creation. This method allows for the url to
121- be overridden to force a keystone endpoint to have specific URL for any of
122- the defined scopes (admin, internal, public).
123-
124- :param configs: OSTemplateRenderer config templating object to inspect
125- for a complete https context.
126- :param url_template: str format string for creating the url template. Only
127- two values will be passed - the scheme+hostname
128- returned by the canonical_url and the port.
129- :param endpoint_type: str endpoint type to resolve.
130- :param override: str the name of the config option which overrides the
131- endpoint URL defined by the charm itself. None will
132- disable any overrides (default).
133- """
134- if override:
135- # Return any user-defined overrides for the keystone endpoint URL.
136- user_value = config(override)
137- if user_value:
138- return user_value.strip()
139-
140- return url_template % (canonical_url(configs, endpoint_type), port)
141-
142-
143-public_endpoint = partial(endpoint_url, endpoint_type=PUBLIC)
144-
145-internal_endpoint = partial(endpoint_url, endpoint_type=INTERNAL)
146-
147-admin_endpoint = partial(endpoint_url, endpoint_type=ADMIN)
148
149=== modified file 'tests/contrib/openstack/test_ip.py'
150--- tests/contrib/openstack/test_ip.py 2015-02-12 22:30:09 +0000
151+++ tests/contrib/openstack/test_ip.py 2015-06-04 22:17:24 +0000
152@@ -7,7 +7,8 @@
153 'config',
154 'unit_get',
155 'get_address_in_network',
156- 'is_clustered'
157+ 'is_clustered',
158+ 'service_name',
159 ]
160
161
162@@ -94,6 +95,25 @@
163 self.test_config.set('vip', '10.5.3.1')
164 self.assertRaises(ValueError, ip.resolve_address)
165
166+ def test_resolve_address_override(self):
167+ self.test_config.set('os-public-hostname', 'public.example.com')
168+ addr = ip.resolve_address()
169+ self.assertEqual('public.example.com', addr)
170+
171+ def test_resolve_address_override_template(self):
172+ self.test_config.set('os-public-hostname',
173+ '{service_name}.example.com')
174+ self.service_name.return_value = 'foo'
175+ addr = ip.resolve_address()
176+ self.assertEqual('foo.example.com', addr)
177+
178+ @patch.object(ip, 'get_ipv6_addr', lambda *args, **kwargs: ['::1'])
179+ def test_resolve_address_ipv6_fallback(self):
180+ self.test_config.set('prefer-ipv6', True)
181+ self.is_clustered.return_value = False
182+ ip.resolve_address()
183+ self.get_address_in_network.assert_called_with(None, '::1')
184+
185 @patch.object(ip, 'resolve_address')
186 def test_canonical_url_http(self, resolve_address):
187 resolve_address.return_value = 'unit1'
188@@ -110,31 +130,8 @@
189 self.assertTrue(ip.canonical_url(configs),
190 'https://unit1')
191
192- def test_endpoint_url(self):
193- exp_url = 'https://public.example.com'
194- self.test_config.set('public-url', exp_url)
195- configs = MagicMock()
196- configs.complete_contexts.return_value = ['https']
197- endpoint = ip.endpoint_url(configs, '%s:%s', 443, ip.PUBLIC,
198- 'public-url')
199- self.assertEqual(exp_url, endpoint)
200-
201- @patch.object(ip, 'resolve_address')
202- def test_endpoint_url_no_override(self, resolve_address):
203- resolve_address.return_value = 'unit1'
204- configs = MagicMock()
205- configs.complete_contexts.return_value = ['https']
206- endpoint = ip.endpoint_url(configs, '%s:%s/v1/AUTH_$(tenant_id)s', 443,
207- ip.INTERNAL, 'internal-url')
208- self.assertEqual('https://unit1:443/v1/AUTH_$(tenant_id)s', endpoint)
209-
210- @patch.object(ip, 'resolve_address')
211- @patch.object(ip, 'is_ipv6')
212- def test_endpoint_url_no_override_ipv6(self, is_ipv6, resolve_address):
213- resolve_address.return_value = 'unit1'
214- is_ipv6.return_value = True
215- configs = MagicMock()
216- configs.complete_contexts.return_value = ['https']
217- endpoint = ip.endpoint_url(configs, '%s:%s', 443,
218- ip.ADMIN, 'admin-url')
219- self.assertEquals('https://[unit1]:443', endpoint)
220+ @patch.object(ip, 'is_ipv6', lambda *args: True)
221+ @patch.object(ip, 'resolve_address')
222+ def test_canonical_url_ipv6(self, resolve_address):
223+ resolve_address.return_value = 'unit1'
224+ self.assertTrue(ip.canonical_url(None), 'http://[unit1]')

Subscribers

People subscribed via source and target branches