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

@@ -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
+        :returns: connection strings
-        :rtype: Generator[str, None, None]
+        :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:
+            bound_addrs = [unit.received.get('bound-address', None) is not None
-                for unit in relation.units:
+                           for relation in self.relations
-                    if not unit.received.get('bound-address'):
+                           for unit in relation.units]
-                        break
+            return all(bound_addrs)
-                else:
-                    continue
-                break
-            else:
-                return True
         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'] = (

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
+        :returns: Northbound DB connection strings
-        :rtype: List[str]
+        :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
+        :returns: Southbound DB connection strings
-        :rtype: List[str]
+        :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):

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()

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):
+    def setUp(self):
-#         super().setUp()
+        super().setUp()
-#         self.target = ovsdb.OVSDB('some-relation', [])
+        self.target = peers.OVSDBClusterPeer('some-relation', [])
-#         self._patches = {}
+        self._patches = {}
-#         self._patches_start = {}
+        self._patches_start = {}
 
-#     def tearDown(self):
+    def tearDown(self):
-#         self.target = None
+        self.target = None
-#         for k, v in self._patches.items():
+        for k, v in self._patches.items():
-#             v.stop()
+            v.stop()
-#             setattr(self, k, None)
+            setattr(self, k, None)
-#         self._patches = None
+        self._patches = None
-#         self._patches_start = None
+        self._patches_start = None
 
-#     def patch_target(self, attr, return_value=None):
+    def patch_target(self, attr, return_value=None):
-#         mocked = mock.patch.object(self.target, attr)
+        mocked = mock.patch.object(self.target, attr)
-#         self._patches[attr] = mocked
+        self._patches[attr] = mocked
-#         started = mocked.start()
+        started = mocked.start()
-#         started.return_value = return_value
+        started.return_value = return_value
-#         self._patches_start[attr] = started
+        self._patches_start[attr] = started
-#         setattr(self, attr, started)
+        setattr(self, attr, started)
 
-#     def test_expected_peers_available(self):
+    def test_expected_peers_available(self):
-#         # self.patch_target('_all_joined_units')
+        # self.patch_target('_all_joined_units')
-#         self.patch_object(ovsdb.ch_core.hookenv, 'expected_peer_units')
+        self.patch_object(peers.ch_core.hookenv, 'expected_peer_units')
-#         self.patch_target('_relations')
+        self.patch_target('_relations')
-#         self.target._all_joined_units = ['aFakeUnit']
+        self.target._all_joined_units = ['aFakeUnit']
-#         self.expected_peer_units.return_value = ['aFakeUnit']
+        self.expected_peer_units.return_value = ['aFakeUnit']
-#         relation = mock.MagicMock()
+        relation = mock.MagicMock()
-#         unit = mock.MagicMock()
+        unit = mock.MagicMock()
-#         relation.units.__iter__.return_value = [unit]
+        relation.units.__iter__.return_value = [unit]
-#         self._relations.__iter__.return_value = [relation]
+        self._relations.__iter__.return_value = [relation]
-#         unit.received.get.return_value = '127.0.0.1'
+        unit.received.get.return_value = '127.0.0.1'
-#         self.assertTrue(self.target.expected_peers_available())
+        self.assertTrue(self.target.expected_peers_available())
-#         unit.received.get.assert_called_once_with('bound-address')
+        unit.received.get.assert_called_once_with('bound-address')
-#         unit.received.get.return_value = ''
+        unit.received.get.return_value = ''
-#         self.assertFalse(self.target.expected_peers_available())
+        self.assertFalse(self.target.expected_peers_available())
-#         self.expected_peer_units.return_value = ['firstFakeUnit',
+        self.expected_peer_units.return_value = ['firstFakeUnit',
-#                                                  'secondFakeUnit']
+                                                 'secondFakeUnit']
-#         unit.received.get.return_value = '127.0.0.1'
+        unit.received.get.return_value = '127.0.0.1'
-#         self.assertFalse(self.target.expected_peers_available())
+        self.assertFalse(self.target.expected_peers_available())
 
-#     def test_joined(self):
+    def test_db_nb_connection_strs(self):
-#         self.patch_object(ovsdb.reactive, 'set_flag')
+        relation = mock.MagicMock()
-#         self.patch_target('publish_cluster_local_addr')
+        relation.relation_id = 'some-endpoint:42'
-#         self.patch_target('expected_peers_available')
+        self.patch_target('expand_name')
-#         self.expected_peers_available.__bool__.return_value = False
+        self.expand_name.return_value = 'some-relation'
-#         self.target.joined()
+        self.patch_object(peers.ch_core.hookenv, 'network_get')
-#         self.expected_peers_available.__bool__.assert_called_once_with()
+        self.network_get.return_value = {
-#         self.set_flag.assert_called_once_with('some-relation.connected')
+            'bind-addresses': [
-#         self.publish_cluster_local_addr.assert_called_once_with()
+                {
-#         self.set_flag.reset_mock()
+                    'macaddress': '',
-#         self.expected_peers_available.__bool__.return_value = True
+                    'interfacename': '',
-#         self.target.joined()
+                    'addresses': [
-#         self.set_flag.assert_has_calls([
+                        {
-#             mock.call('some-relation.connected'),
+                            'hostname': '',
-#             mock.call('some-relation.available'),
+                            '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'),
+        ])