From d8e6722c45b0912aeea2a128273e0e9ef3bed670 Mon Sep 17 00:00:00 2001 From: David Ames Date: Tue, 27 Feb 2018 10:06:21 -0800 Subject: [PATCH] Always register the VIP with keystone catalog Resolve_address is used to register an API charm's keystone catalog entry. Resolve_address was checking if the unit was clustered and if not falling back to its private-address. This is incorrect behavior. If a VIP is set it should always register the VIP with the keystone catalog. The fallout from this is manifested in two bugs: Bug#1750915 and Bug#1749280. In first, keystone is creating and passing SSL certificates for the local unit's private-ip and only subsequently for the VIP cause apache to fail to start. In the second, a race condition exists where clients of the API charm relying on catalog entries get disrupted when the catalog entry changes. This fix removes the is_clustered check from resolve_address. The gating on is_clustered should happen elsewhere in the charm not at registration time with keystone. Change-Id: Ieaee4fbcd1b45b95ddca4ec3a9a09d4522b09b03 Partial-Bug: #1750915 Partial-Bug: #1749280 --- charms_openstack/ip.py | 16 ++++++++-------- unit_tests/__init__.py | 4 ++++ unit_tests/test_charms_openstack_ip.py | 26 +++++++++++--------------- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/charms_openstack/ip.py b/charms_openstack/ip.py index 00b90a6..cf8c6eb 100644 --- a/charms_openstack/ip.py +++ b/charms_openstack/ip.py @@ -18,7 +18,6 @@ import netaddr import charmhelpers.core.hookenv as hookenv import charmhelpers.contrib.network.ip as net_ip -import charmhelpers.contrib.hahelpers.cluster as cluster import charms.reactive.bus PUBLIC = 'public' @@ -113,11 +112,13 @@ def _resolve_network_cidr(ip_address): def resolve_address(endpoint_type=PUBLIC, override=True): """Return unit address depending on net config. - If unit is clustered with vip(s) and has net splits defined, return vip on - correct network. If clustered with no nets defined, return primary vip. + If unit is has vip(s) configured and has net splits defined, return vip on + correct network. If vip configured with no nets defined, return primary + vip. - If not clustered, return unit address ensuring address is on configured net - split if one is configured, or a Juju 2.0 extra-binding has been used. + If no vip is configured, return unit address ensuring address is on + configured net split if one is configured, or a Juju 2.0 extra-binding has + been used. :param endpoint_type: Network endpoing type :param override: Accept hostname overrides or not @@ -136,9 +137,8 @@ def resolve_address(endpoint_type=PUBLIC, override=True): net_addr = hookenv.config(net_type) net_fallback = ADDRESS_MAP[endpoint_type]['fallback'] binding = ADDRESS_MAP[endpoint_type]['binding'] - clustered = cluster.is_clustered() - if clustered and vips: + if vips: if net_addr: for vip in vips: if net_ip.is_address_in_network(net_addr, vip): @@ -179,6 +179,6 @@ def resolve_address(endpoint_type=PUBLIC, override=True): if resolved_address is None: raise ValueError("Unable to resolve a suitable IP address based on " "charm state and configuration. (net_type=%s, " - "clustered=%s)" % (net_type, clustered)) + % (net_type)) return resolved_address diff --git a/unit_tests/__init__.py b/unit_tests/__init__.py index 541fe4a..f7c6366 100644 --- a/unit_tests/__init__.py +++ b/unit_tests/__init__.py @@ -14,6 +14,7 @@ import sys import mock +import os # mock out some charmhelpers libraries as they have apt install side effects apt_pkg = mock.MagicMock() @@ -64,6 +65,9 @@ charmhelpers.contrib.openstack.utils.OPENSTACK_RELEASES = ( 'pike', ) +# charms.reactive uses hookenv.charm_dir which must return a directory +charmhelpers.core.hookenv.charm_dir.return_value = os.path.curdir + def _fake_retry(num_retries, base_delay=0, exc_type=Exception): def _retry_on_exception_inner_1(f): diff --git a/unit_tests/test_charms_openstack_ip.py b/unit_tests/test_charms_openstack_ip.py index d017af7..6627f47 100644 --- a/unit_tests/test_charms_openstack_ip.py +++ b/unit_tests/test_charms_openstack_ip.py @@ -51,7 +51,6 @@ class TestCharmOpenStackIp(utils.BaseTestCase): self.resolve_address.assert_called_once_with(ip.INTERNAL) def test_resolve_address(self): - self.patch_object(ip.cluster, 'is_clustered') self.patch_object(ip.hookenv, 'config') self.patch_object(ip.hookenv, 'network_get_primary_address') self.patch_object(ip.net_ip, 'is_address_in_network') @@ -63,7 +62,7 @@ class TestCharmOpenStackIp(utils.BaseTestCase): # what it was called with. calls_list = [] _config = { - 'vip': 'vip-address', + 'vip': None, 'prefer-ipv6': False, 'os-public-network': 'the-public-network', 'os-public-hostname': None, @@ -80,10 +79,9 @@ class TestCharmOpenStackIp(utils.BaseTestCase): # Juju pre 2.0 behaviour where network-get is not implemented self.network_get_primary_address.side_effect = NotImplementedError - # first test, if not clustered, that the function uses unit_get() and + # first test, if no VIP, that the function uses unit_get() and # get_address_in_network to get a real address. # for the default PUBLIC endpoint - self.is_clustered.return_value = False self.get_address_in_network.return_value = 'got-address' self.unit_get.return_value = 'unit-get-address' addr = ip.resolve_address() @@ -97,18 +95,18 @@ class TestCharmOpenStackIp(utils.BaseTestCase): self.get_address_in_network.assert_called_once_with( 'the-public-network', 'unit-get-address') - # second test: not clustered, prefer-ipv6 is True + # second test: no vip, prefer-ipv6 is True _config['prefer-ipv6'] = True calls_list = [] self.get_ipv6_addr.return_value = ['ipv6-addr'] self.get_address_in_network.reset_mock() addr = ip.resolve_address() - self.get_ipv6_addr.assert_called_once_with(exc_list=['vip-address']) + self.get_ipv6_addr.assert_called_once_with(exc_list=None) self.get_address_in_network.assert_called_once_with( 'the-public-network', 'ipv6-addr') - # Third test: clustered, and config(...) returns None - self.is_clustered.return_value = True + # Third test: vip, and config(...) returns None + _config['vip'] = 'vip-address' _config['os-public-network'] = None calls_list = [] addr = ip.resolve_address() @@ -141,7 +139,6 @@ class TestCharmOpenStackIp(utils.BaseTestCase): addr = ip.resolve_address() def test_resolve_address_network_binding(self): - self.patch_object(ip.cluster, 'is_clustered') self.patch_object(ip.hookenv, 'config') self.patch_object(ip.hookenv, 'network_get_primary_address') self.patch_object(ip.net_ip, 'is_address_in_network') @@ -154,7 +151,7 @@ class TestCharmOpenStackIp(utils.BaseTestCase): # what it was called with. calls_list = [] _config = { - 'vip': 'vip1 vip2', + 'vip': None, 'prefer-ipv6': False, 'os-public-network': None, 'os-public-hostname': None, @@ -168,10 +165,9 @@ class TestCharmOpenStackIp(utils.BaseTestCase): self.config.side_effect = fake_config - # first test, if not clustered, that the function uses unit_get + # first test, if no vip, that the function uses unit_get # network_get_primary_address to get a real address. # for the default PUBLIC endpoint - self.is_clustered.return_value = False self.network_get_primary_address.return_value = 'got-address' self._resolve_network_cidr.return_value = 'cidr' self.unit_get.return_value = 'unit-get-address' @@ -187,7 +183,7 @@ class TestCharmOpenStackIp(utils.BaseTestCase): 'public' ) - # second test: not clustered, prefer-ipv6 is True, ensure + # second test: no vip, prefer-ipv6 is True, ensure # that ipv6 address is fallback and network-get is still # used to determine the public endpoint binding _config['prefer-ipv6'] = True @@ -195,7 +191,7 @@ class TestCharmOpenStackIp(utils.BaseTestCase): self.get_ipv6_addr.return_value = ['ipv6-addr'] self.get_address_in_network.reset_mock() addr = ip.resolve_address() - self.get_ipv6_addr.assert_called_once_with(exc_list=['vip1', 'vip2']) + self.get_ipv6_addr.assert_called_once_with(exc_list=None) self.network_get_primary_address.assert_called_with( 'public' ) @@ -206,7 +202,7 @@ class TestCharmOpenStackIp(utils.BaseTestCase): self.is_address_in_network.side_effect = _fake_addr_in_net # Third test: clustered - self.is_clustered.return_value = True + _config['vip'] = 'vip1 vip2' calls_list = [] addr = ip.resolve_address() self.assertEqual(calls_list, [('os-public-hostname',),