From 882c4e9a8c438c830face90bf857ece8be4d5264 Mon Sep 17 00:00:00 2001 From: Frode Nordahl Date: Mon, 28 Oct 2019 06:30:05 +0100 Subject: [PATCH] Use separate admin port for Southbound DB on central units Due to how RBAC is implemented in ``ovsdb-server``, we need to create a separate listener for ``ovn-northd`` to connect to. Update docstrings and add some missing unit tests. Replace for/else loop in expected_units_available with List comprehension. Change-Id: I9b663615ddbbe51b9229f7e8f2bef5580489e4c7 --- src/lib/ovsdb.py | 59 ++++++-- src/ovsdb_cluster/peers.py | 53 +++++-- unit_tests/test_lib_ovsdb.py | 86 +++++++++++ unit_tests/test_ovsdb_cluster_peers.py | 194 +++++++++++++++++-------- 4 files changed, 310 insertions(+), 82 deletions(-) diff --git a/src/lib/ovsdb.py b/src/lib/ovsdb.py index 66c0c15..625c752 100644 --- a/src/lib/ovsdb.py +++ b/src/lib/ovsdb.py @@ -48,6 +48,11 @@ class OVSDB(reactive.Endpoint): @property def cluster_local_addr(self): + """Retrieve local address bound to endpoint. + + :returns: IPv4 or IPv6 address bound to endpoint + :rtype: str + """ for relation in self.relations: ng_data = ch_core.hookenv.network_get( self.expand_name('{endpoint_name}'), @@ -58,6 +63,11 @@ class OVSDB(reactive.Endpoint): @property def cluster_remote_addrs(self): + """Retrieve remote addresses bound to remote endpoint. + + :returns: IPv4 or IPv6 addresses bound to remote endpoints. + :rtype: Iterator[str] + """ for relation in self.relations: for unit in relation.units: try: @@ -69,60 +79,81 @@ class OVSDB(reactive.Endpoint): @property def db_nb_port(self): + """Provide port number for OVN Northbound OVSDB. + + :returns: port number for OVN Northbound OVSDB. + :rtype: int + """ return self.DB_NB_PORT @property def db_sb_port(self): + """Provide port number for OVN Southbound OVSDB. + + :returns: port number for OVN Southbound OVSDB. + :rtype: int + """ return self.DB_SB_PORT def db_connection_strs(self, addrs, port, proto='ssl'): - """Provide connection strings + """Provide connection strings. :param port: Port number :type port: int :param proto: Protocol :type proto: str - :returns: List of connection strings - :rtype: Generator[str, None, None] + :returns: connection strings + :rtype: Iterator[str] """ for addr in addrs: yield ':'.join((proto, addr, str(port))) @property def db_nb_connection_strs(self): + """Provide OVN Northbound OVSDB connection strings. + + :returns: OVN Northbound OVSDB connection strings. + :rtpye: Iterator[str] + """ return self.db_connection_strs(self.cluster_remote_addrs, self.db_nb_port) @property def db_sb_connection_strs(self): + """Provide OVN Southbound OVSDB connection strings. + + :returns: OVN Southbound OVSDB connection strings. + :rtpye: Iterator[str] + """ return self.db_connection_strs(self.cluster_remote_addrs, self.db_sb_port) def expected_units_available(self): - """Whether expected units have joined and published data on a relation + """Whether expected units have joined and published data on a relation. NOTE: This does not work for the peer relation, see separate method for that in the peer relation implementation. + :returns: True if expected units have joined and published data, + False otherwise. + :rtype: bool """ if len(self.all_joined_units) == len( list(ch_core.hookenv.expected_related_units( self.expand_name('{endpoint_name}')))): - for relation in self.relations: - for unit in relation.units: - if not unit.received.get('bound-address'): - break - else: - continue - break - else: - return True + bound_addrs = [unit.received.get('bound-address', None) is not None + for relation in self.relations + for unit in relation.units] + return all(bound_addrs) return False def publish_cluster_local_addr(self, addr=None): - """Announce the address we bound our OVSDB Servers to. + """Announce address on relation. This will be used by our peers and clients to build a connection string to the remote cluster. + + :param addr: Override address to announce. + :type addr: Optional[str] """ for relation in self.relations: relation.to_publish['bound-address'] = ( diff --git a/src/ovsdb_cluster/peers.py b/src/ovsdb_cluster/peers.py index c844415..e9eb71f 100644 --- a/src/ovsdb_cluster/peers.py +++ b/src/ovsdb_cluster/peers.py @@ -29,18 +29,51 @@ from .lib import ovsdb as ovsdb class OVSDBClusterPeer(ovsdb.OVSDB): + DB_SB_ADMIN_PORT = 16642 DB_NB_CLUSTER_PORT = 6643 DB_SB_CLUSTER_PORT = 6644 + @property + def db_sb_admin_port(self): + """Provide admin port number for OVN Southbound OVSDB. + + This is a special listener to allow ``ovn-northd`` to connect to an + endpoint without RBAC enabled as there is currently no RBAC profile + allowing ``ovn-northd`` to perform its work. + + :returns: admin port number for OVN Southbound OVSDB. + :rtype: int + """ + return self.DB_SB_ADMIN_PORT + @property def db_nb_cluster_port(self): + """Provide port number for OVN Northbound OVSDB. + + :returns port number for OVN Northbound OVSDB. + :rtype: int + """ return self.DB_NB_CLUSTER_PORT @property def db_sb_cluster_port(self): + """Provide port number for OVN Southbound OVSDB. + + :returns: port number for OVN Southbound OVSDB. + :rtype: int + """ return self.DB_SB_CLUSTER_PORT def expected_peers_available(self): + """Whether expected peers have joined and published data on peer rel. + + NOTE: This does not work for the normal inter-charm relations, please + refer separate method for that in the shared interface library. + + :returns: True if expected peers have joined and published data, + False otherwise. + :rtype: bool + """ if len(self.all_joined_units) == len( list(ch_core.hookenv.expected_peer_units())): for relation in self.relations: @@ -52,13 +85,13 @@ class OVSDBClusterPeer(ovsdb.OVSDB): @property def db_nb_connection_strs(self): - """Return list of Northbound DB connection strings. + """Provide Northbound DB connection strings. We override the parent property because for the peer relation ``cluster_remote_addrs`` does not contain self. - :returns: list of Northbound DB connection strings - :rtype: List[str] + :returns: Northbound DB connection strings + :rtype: Iterator[str] """ return itertools.chain( self.db_connection_strs((self.cluster_local_addr,), @@ -68,19 +101,21 @@ class OVSDBClusterPeer(ovsdb.OVSDB): @property def db_sb_connection_strs(self): - """Return list of Southbound DB connection strings. + """Provide Southbound DB connection strings. We override the parent property because for the peer relation - ``cluster_remote_addrs`` does not contain self. + ``cluster_remote_addrs`` does not contain self. We use a different + port for connecting to the SB DB as there is currently no RBAC profile + that provide the privileges ``ovn-northd`` requires to operate. - :returns: list of Southbound DB connection strings - :rtype: List[str] + :returns: Southbound DB connection strings + :rtype: Iterator[str] """ return itertools.chain( self.db_connection_strs((self.cluster_local_addr,), - self.db_sb_port), + self.db_sb_admin_port), self.db_connection_strs(self.cluster_remote_addrs, - self.db_sb_port)) + self.db_sb_admin_port)) @when('endpoint.{endpoint_name}.joined') def joined(self): diff --git a/unit_tests/test_lib_ovsdb.py b/unit_tests/test_lib_ovsdb.py index 685ace3..5d07d94 100644 --- a/unit_tests/test_lib_ovsdb.py +++ b/unit_tests/test_lib_ovsdb.py @@ -54,6 +54,19 @@ class TestOVSDBLib(test_utils.PatchHelper): self._relations.__iter__.return_value = [relation] return relation.to_publish + def pmock(self, return_value=None): + p = mock.PropertyMock().return_value = return_value + return p + + def test__format_addr(self): + self.assertEquals('1.2.3.4', self.target._format_addr('1.2.3.4')) + self.assertEquals( + '[2001:db8::42]', self.target._format_addr('2001:db8::42')) + with self.assertRaises(ValueError): + self.target._format_addr('999.999.999.999') + with self.assertRaises(ValueError): + self.target._format_addr('2001:db8::g') + def test_cluster_local_addr(self): relation = mock.MagicMock() relation.relation_id = 'some-endpoint:42' @@ -83,6 +96,79 @@ class TestOVSDBLib(test_utils.PatchHelper): self.network_get.assert_called_once_with( 'some-relation', relation_id='some-endpoint:42') + def test_cluster_remote_addr(self): + relation = mock.MagicMock() + relation.relation_id = 'some-endpoint:42' + unit4 = mock.MagicMock() + unit4.received = {'bound-address': '1.2.3.4'} + unit6 = mock.MagicMock() + unit6.received = {'bound-address': '2001:db8::42'} + relation.units.__iter__.return_value = [unit4, unit6] + self.patch_target('_relations') + self._relations.__iter__.return_value = [relation] + self.assertEquals( + sorted(self.target.cluster_remote_addrs), + ['1.2.3.4', '[2001:db8::42]']) + + def test_db_nb_port(self): + self.assertEquals(self.target.db_nb_port, self.target.DB_NB_PORT) + + def test_db_sb_port(self): + self.assertEquals(self.target.db_sb_port, self.target.DB_SB_PORT) + + def test_db_connection_strs(self): + self.assertEquals( + sorted(self.target.db_connection_strs(['a', 'b', 'c'], 123)), + ['ssl:a:123', 'ssl:b:123', 'ssl:c:123']) + + def test_db_nb_connection_strs(self): + relation = mock.MagicMock() + unit = mock.MagicMock() + unit.received = {'bound-address': '1.2.3.4'} + unit2 = mock.MagicMock() + unit2.received = {'bound-address': '2.3.4.5'} + relation.units.__iter__.return_value = [unit, unit2] + self.patch_target('_relations') + self._relations.__iter__.return_value = [relation] + self.assertEquals( + list(self.target.db_nb_connection_strs), + ['ssl:1.2.3.4:6641', 'ssl:2.3.4.5:6641']) + + def test_db_sb_connection_strs(self): + relation = mock.MagicMock() + unit = mock.MagicMock() + unit.received = {'bound-address': '1.2.3.4'} + unit2 = mock.MagicMock() + unit2.received = {'bound-address': '2.3.4.5'} + relation.units.__iter__.return_value = [unit, unit2] + self.patch_target('_relations') + self._relations.__iter__.return_value = [relation] + self.assertEquals( + list(self.target.db_sb_connection_strs), + ['ssl:1.2.3.4:6642', 'ssl:2.3.4.5:6642']) + + def test_expected_units_available(self): + self.patch_object(ovsdb.ch_core.hookenv, 'expected_related_units') + self.expected_related_units.return_value = [ + 'unit/0', 'unit/1', 'unit/2'] + self.assertEquals(self.target.expected_units_available(), False) + relation = mock.MagicMock() + relation.relation_id = 'some-endpoint:42' + unit = mock.MagicMock() + unit.received = {'bound-address': '1.2.3.4'} + unit2 = mock.MagicMock() + unit2.received = {'bound-address': '2.3.4.5'} + unit3 = mock.MagicMock() + unit3.received = {} + relation.units.__iter__.return_value = [unit, unit2, unit3] + self.patch_target('_relations') + self._relations.__iter__.return_value = [relation] + self.target._all_joined_units = [ + 'unit/0', 'unit/1', 'unit/2'] + self.assertEquals(self.target.expected_units_available(), False) + unit3.received = {'bound-address': '6.7.8.9'} + self.assertEquals(self.target.expected_units_available(), True) + def test_publish_cluster_local_addr(self): to_publish = self.patch_topublish() self.target.publish_cluster_local_addr() diff --git a/unit_tests/test_ovsdb_cluster_peers.py b/unit_tests/test_ovsdb_cluster_peers.py index 8f155d9..601066e 100644 --- a/unit_tests/test_ovsdb_cluster_peers.py +++ b/unit_tests/test_ovsdb_cluster_peers.py @@ -12,73 +12,149 @@ # See the License for the specific language governing permissions and # limitations under the License. -# import mock +import mock -# from x import peers +from ovsdb_cluster import peers -# import charms_openstack.test_utils as test_utils +import charms_openstack.test_utils as test_utils -# _hook_args = {} +_hook_args = {} -# class TestOVSDBPeers(test_utils.PatchHelper): +class TestOVSDBClusterPeer(test_utils.PatchHelper): -# def setUp(self): -# super().setUp() -# self.target = ovsdb.OVSDB('some-relation', []) -# self._patches = {} -# self._patches_start = {} + def setUp(self): + super().setUp() + self.target = peers.OVSDBClusterPeer('some-relation', []) + self._patches = {} + self._patches_start = {} -# def tearDown(self): -# self.target = None -# for k, v in self._patches.items(): -# v.stop() -# setattr(self, k, None) -# self._patches = None -# self._patches_start = None + def tearDown(self): + self.target = None + for k, v in self._patches.items(): + v.stop() + setattr(self, k, None) + self._patches = None + self._patches_start = None -# def patch_target(self, attr, return_value=None): -# mocked = mock.patch.object(self.target, attr) -# self._patches[attr] = mocked -# started = mocked.start() -# started.return_value = return_value -# self._patches_start[attr] = started -# setattr(self, attr, started) + def patch_target(self, attr, return_value=None): + mocked = mock.patch.object(self.target, attr) + self._patches[attr] = mocked + started = mocked.start() + started.return_value = return_value + self._patches_start[attr] = started + setattr(self, attr, started) -# def test_expected_peers_available(self): -# # self.patch_target('_all_joined_units') -# self.patch_object(ovsdb.ch_core.hookenv, 'expected_peer_units') -# self.patch_target('_relations') -# self.target._all_joined_units = ['aFakeUnit'] -# self.expected_peer_units.return_value = ['aFakeUnit'] -# relation = mock.MagicMock() -# unit = mock.MagicMock() -# relation.units.__iter__.return_value = [unit] -# self._relations.__iter__.return_value = [relation] -# unit.received.get.return_value = '127.0.0.1' -# self.assertTrue(self.target.expected_peers_available()) -# unit.received.get.assert_called_once_with('bound-address') -# unit.received.get.return_value = '' -# self.assertFalse(self.target.expected_peers_available()) -# self.expected_peer_units.return_value = ['firstFakeUnit', -# 'secondFakeUnit'] -# unit.received.get.return_value = '127.0.0.1' -# self.assertFalse(self.target.expected_peers_available()) + def test_expected_peers_available(self): + # self.patch_target('_all_joined_units') + self.patch_object(peers.ch_core.hookenv, 'expected_peer_units') + self.patch_target('_relations') + self.target._all_joined_units = ['aFakeUnit'] + self.expected_peer_units.return_value = ['aFakeUnit'] + relation = mock.MagicMock() + unit = mock.MagicMock() + relation.units.__iter__.return_value = [unit] + self._relations.__iter__.return_value = [relation] + unit.received.get.return_value = '127.0.0.1' + self.assertTrue(self.target.expected_peers_available()) + unit.received.get.assert_called_once_with('bound-address') + unit.received.get.return_value = '' + self.assertFalse(self.target.expected_peers_available()) + self.expected_peer_units.return_value = ['firstFakeUnit', + 'secondFakeUnit'] + unit.received.get.return_value = '127.0.0.1' + self.assertFalse(self.target.expected_peers_available()) -# def test_joined(self): -# self.patch_object(ovsdb.reactive, 'set_flag') -# self.patch_target('publish_cluster_local_addr') -# self.patch_target('expected_peers_available') -# self.expected_peers_available.__bool__.return_value = False -# self.target.joined() -# self.expected_peers_available.__bool__.assert_called_once_with() -# self.set_flag.assert_called_once_with('some-relation.connected') -# self.publish_cluster_local_addr.assert_called_once_with() -# self.set_flag.reset_mock() -# self.expected_peers_available.__bool__.return_value = True -# self.target.joined() -# self.set_flag.assert_has_calls([ -# mock.call('some-relation.connected'), -# mock.call('some-relation.available'), -# ]) + def test_db_nb_connection_strs(self): + relation = mock.MagicMock() + relation.relation_id = 'some-endpoint:42' + self.patch_target('expand_name') + self.expand_name.return_value = 'some-relation' + self.patch_object(peers.ch_core.hookenv, 'network_get') + self.network_get.return_value = { + 'bind-addresses': [ + { + 'macaddress': '', + 'interfacename': '', + 'addresses': [ + { + 'hostname': '', + 'address': '42.42.42.42', + 'cidr': '' + }, + ], + }, + ], + 'egress-subnets': ['42.42.42.42/32'], + 'ingress-addresses': ['42.42.42.42'], + } + unit = mock.MagicMock() + unit.received = {'bound-address': '1.2.3.4'} + unit2 = mock.MagicMock() + unit2.received = {'bound-address': '2.3.4.5'} + relation.units.__iter__.return_value = [unit, unit2] + self.patch_target('_relations') + self._relations.__iter__.return_value = [relation] + self.assertEquals( + list(self.target.db_nb_connection_strs), + ['ssl:42.42.42.42:6641', + 'ssl:1.2.3.4:6641', + 'ssl:2.3.4.5:6641']) + + def test_db_sb_connection_strs(self): + relation = mock.MagicMock() + relation.relation_id = 'some-endpoint:42' + self.patch_target('expand_name') + self.expand_name.return_value = 'some-relation' + self.patch_object(peers.ch_core.hookenv, 'network_get') + self.network_get.return_value = { + 'bind-addresses': [ + { + 'macaddress': '', + 'interfacename': '', + 'addresses': [ + { + 'hostname': '', + 'address': '42.42.42.42', + 'cidr': '' + }, + ], + }, + ], + 'egress-subnets': ['42.42.42.42/32'], + 'ingress-addresses': ['42.42.42.42'], + } + unit = mock.MagicMock() + unit.received = {'bound-address': '1.2.3.4'} + unit2 = mock.MagicMock() + unit2.received = {'bound-address': '2.3.4.5'} + relation.units.__iter__.return_value = [unit, unit2] + self.patch_target('_relations') + self._relations.__iter__.return_value = [relation] + self.assertEquals( + list(self.target.db_sb_connection_strs), + ['ssl:42.42.42.42:16642', + 'ssl:1.2.3.4:16642', + 'ssl:2.3.4.5:16642']) + + def test_joined(self): + self.patch_object(peers.reactive, 'set_flag') + self.patch_object(peers.reactive, 'is_flag_set') + self.patch_target('publish_cluster_local_addr') + self.patch_target('expected_peers_available') + self.expected_peers_available.return_value = False + self.is_flag_set.return_value = False + self.target.joined() + self.expected_peers_available.assert_called_once_with() + self.set_flag.assert_called_once_with('some-relation.connected') + self.assertFalse(self.publish_cluster_local_addr.called) + self.set_flag.reset_mock() + self.expected_peers_available.return_value = True + self.is_flag_set.return_value = True + self.target.joined() + self.publish_cluster_local_addr.assert_called_once_with() + self.set_flag.assert_has_calls([ + mock.call('some-relation.connected'), + mock.call('some-relation.available'), + ])