From 675ea9c35ef73765828e3db4b636f13465a76596 Mon Sep 17 00:00:00 2001
From: Brian Haley <bhaley@redhat.com>
Date: Thu, 10 Jun 2021 16:42:36 -0400
Subject: [PATCH] Support creating members without a subnet ID

Since subnet ID is an optional API argument, if not
available the provider driver will now attempt to look
it up via the pool ID that is required.

Closes-bug: #1896677
Change-Id: Iec17b36ce38be89f83a45ea3ef4c652e837c69c5
---
 ovn_octavia_provider/driver.py                | 23 ++++++++------
 ovn_octavia_provider/helper.py                |  8 +++++
 ovn_octavia_provider/tests/functional/base.py | 12 ++++++--
 .../tests/functional/test_driver.py           | 30 +++++++++++++++++--
 .../tests/unit/test_driver.py                 | 20 +++++++++++--
 .../tests/unit/test_helper.py                 | 27 +++++++++++++++++
 ...ate-without-subnetid-0b4e3aa6ac453f28.yaml |  7 +++++
 7 files changed, 112 insertions(+), 15 deletions(-)
 create mode 100644 releasenotes/notes/support-member-create-without-subnetid-0b4e3aa6ac453f28.yaml

diff --git a/ovn_octavia_provider/driver.py b/ovn_octavia_provider/driver.py
index 5a42cbf5..dd8ea0f4 100644
--- a/ovn_octavia_provider/driver.py
+++ b/ovn_octavia_provider/driver.py
@@ -213,6 +213,8 @@ class OvnProviderDriver(driver_base.ProviderDriver):
 
     def _ip_version_differs(self, member):
         _, ovn_lb = self._ovn_helper._find_ovn_lb_by_pool_id(member.pool_id)
+        if not ovn_lb:
+            return False
         lb_vip = ovn_lb.external_ids[ovn_const.LB_EXT_IDS_VIP_KEY]
         return netaddr.IPNetwork(lb_vip).version != (
             netaddr.IPNetwork(member.address).version)
@@ -223,13 +225,16 @@ class OvnProviderDriver(driver_base.ProviderDriver):
         if self._ip_version_differs(member):
             raise ovn_exc.IPVersionsMixingNotSupportedError()
         admin_state_up = member.admin_state_up
-        if (isinstance(member.subnet_id, o_datamodels.UnsetType) or
-                not member.subnet_id):
-            msg = _('Subnet is required for Member creation '
-                    'with OVN Provider Driver')
-            raise driver_exceptions.UnsupportedOptionError(
-                user_fault_string=msg,
-                operator_fault_string=msg)
+        subnet_id = member.subnet_id
+        if (isinstance(subnet_id, o_datamodels.UnsetType) or not subnet_id):
+            subnet_id = self._ovn_helper._get_subnet_from_pool(member.pool_id)
+            if not subnet_id:
+                msg = _('Subnet is required, or Loadbalancer associated with '
+                        'Pool must have a subnet, for Member creation '
+                        'with OVN Provider Driver')
+                raise driver_exceptions.UnsupportedOptionError(
+                    user_fault_string=msg,
+                    operator_fault_string=msg)
 
         if isinstance(admin_state_up, o_datamodels.UnsetType):
             admin_state_up = True
@@ -237,7 +242,7 @@ class OvnProviderDriver(driver_base.ProviderDriver):
                         'address': member.address,
                         'protocol_port': member.protocol_port,
                         'pool_id': member.pool_id,
-                        'subnet_id': member.subnet_id,
+                        'subnet_id': subnet_id,
                         'admin_state_up': admin_state_up}
         request = {'type': ovn_const.REQ_TYPE_MEMBER_CREATE,
                    'info': request_info}
@@ -249,7 +254,7 @@ class OvnProviderDriver(driver_base.ProviderDriver):
         request_info = {'id': member.member_id,
                         'address': member.address,
                         'pool_id': member.pool_id,
-                        'subnet_id': member.subnet_id,
+                        'subnet_id': subnet_id,
                         'action': ovn_const.REQ_INFO_MEMBER_ADDED}
         request = {'type': ovn_const.REQ_TYPE_HANDLE_MEMBER_DVR,
                    'info': request_info}
diff --git a/ovn_octavia_provider/helper.py b/ovn_octavia_provider/helper.py
index 9693b011..01b01ed6 100644
--- a/ovn_octavia_provider/helper.py
+++ b/ovn_octavia_provider/helper.py
@@ -466,6 +466,14 @@ class OvnProviderHelper():
             ovn_lb = self._find_ovn_lb_with_pool_key(pool_key)
         return pool_key, ovn_lb
 
+    def _get_subnet_from_pool(self, pool_id):
+        pool = self._octavia_driver_lib.get_pool(pool_id)
+        if not pool:
+            return
+        lb = self._octavia_driver_lib.get_loadbalancer(pool.loadbalancer_id)
+        if lb and lb.vip_subnet_id:
+            return lb.vip_subnet_id
+
     def _execute_commands(self, commands):
         with self.ovn_nbdb_api.transaction(check_error=True) as txn:
             for command in commands:
diff --git a/ovn_octavia_provider/tests/functional/base.py b/ovn_octavia_provider/tests/functional/base.py
index 08309677..0406345a 100644
--- a/ovn_octavia_provider/tests/functional/base.py
+++ b/ovn_octavia_provider/tests/functional/base.py
@@ -696,7 +696,7 @@ class TestOvnOctaviaBase(base.TestOVNFunctionalBase,
         return listeners
 
     def _create_member_and_validate(self, lb_data, pool_id, subnet_id,
-                                    network_id, address):
+                                    network_id, address, expected_subnet=None):
         self._o_driver_lib.update_loadbalancer_status.reset_mock()
         pool = self._get_pool_from_lb_data(lb_data, pool_id=pool_id)
         pool_status = {'id': pool.pool_id,
@@ -704,7 +704,15 @@ class TestOvnOctaviaBase(base.TestOVNFunctionalBase,
                        'operating_status': o_constants.ONLINE}
 
         m_member = self._create_member_model(pool.pool_id, subnet_id, address)
-        pool.members.append(m_member)
+        # The "expected" member value, which might be different from what
+        # we pass to member_create(), for example, if an expected_subnet
+        # was given.
+        if expected_subnet:
+            e_member = copy.deepcopy(m_member)
+            e_member.subnet_id = expected_subnet
+        else:
+            e_member = m_member
+        pool.members.append(e_member)
 
         self.ovn_driver.member_create(m_member)
         self._update_ls_refs(lb_data, network_id)
diff --git a/ovn_octavia_provider/tests/functional/test_driver.py b/ovn_octavia_provider/tests/functional/test_driver.py
index aefe811f..421821ce 100644
--- a/ovn_octavia_provider/tests/functional/test_driver.py
+++ b/ovn_octavia_provider/tests/functional/test_driver.py
@@ -154,13 +154,39 @@ class TestOvnOctaviaProviderDriver(ovn_base.TestOvnOctaviaBase):
         self._delete_member_and_validate(lb_data, pool_TCP_id, net20,
                                          '20.0.0.6')
 
-        # Test creating Member without subnet
-        m_member = self._create_member_model(pool_TCP_id,
+        # Deleting the pool should also delete the members.
+        self._delete_pool_and_validate(lb_data, "p_TCP")
+
+        # Delete the whole LB.
+        self._delete_load_balancer_and_validate(lb_data)
+
+    def test_member_no_subnet(self):
+        self._o_driver_lib.get_pool.return_value = None
+
+        # Test creating Member without subnet and unknown pool
+        m_member = self._create_member_model('pool_from_nowhere',
                                              None,
                                              '30.0.0.7', 80)
         self.assertRaises(o_exceptions.UnsupportedOptionError,
                           self.ovn_driver.member_create, m_member)
 
+        lb_data = self._create_load_balancer_and_validate(
+            {'vip_network': 'vip_network',
+             'cidr': '10.0.0.0/24'})
+
+        # TCP Pool
+        self._create_pool_and_validate(lb_data, "p_TCP", protocol='TCP')
+        pool_TCP_id = lb_data['pools'][0].pool_id
+
+        self._o_driver_lib.get_pool.return_value = lb_data['pools'][0]
+        self._o_driver_lib.get_loadbalancer.return_value = lb_data['model']
+
+        # Test creating Member without subnet but with pool
+        self._create_member_and_validate(
+            lb_data, pool_TCP_id, None,
+            lb_data['vip_net_info'][0], '10.0.0.10',
+            expected_subnet=lb_data['vip_net_info'][1])
+
         # Deleting the pool should also delete the members.
         self._delete_pool_and_validate(lb_data, "p_TCP")
 
diff --git a/ovn_octavia_provider/tests/unit/test_driver.py b/ovn_octavia_provider/tests/unit/test_driver.py
index 45b5bf7a..aacddc7c 100644
--- a/ovn_octavia_provider/tests/unit/test_driver.py
+++ b/ovn_octavia_provider/tests/unit/test_driver.py
@@ -215,6 +215,10 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase):
             ovn_helper.OvnProviderHelper,
             '_find_ovn_lb_with_pool_key',
             return_value=self.ovn_lb).start()
+        self.mock_get_subnet_from_pool = mock.patch.object(
+            ovn_helper.OvnProviderHelper,
+            '_get_subnet_from_pool',
+            return_value=None).start()
 
     def test__ip_version_differs(self):
         self.assertFalse(self.driver._ip_version_differs(self.ref_member))
@@ -228,7 +232,7 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase):
             mock.call('pool_%s' % self.pool_id),
             mock.call('pool_%s:D' % self.pool_id)])
 
-    def test_member_create(self):
+    def _test_member_create(self, member):
         info = {'id': self.ref_member.member_id,
                 'address': self.ref_member.address,
                 'protocol_port': self.ref_member.protocol_port,
@@ -246,12 +250,15 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase):
         expected_dict_dvr = {
             'type': ovn_const.REQ_TYPE_HANDLE_MEMBER_DVR,
             'info': info_dvr}
-        self.driver.member_create(self.ref_member)
+        self.driver.member_create(member)
         expected = [
             mock.call(expected_dict),
             mock.call(expected_dict_dvr)]
         self.mock_add_request.assert_has_calls(expected)
 
+    def test_member_create(self):
+        self._test_member_create(self.ref_member)
+
     def test_member_create_failure(self):
         self.assertRaises(exceptions.UnsupportedOptionError,
                           self.driver.member_create, self.fail_member)
@@ -279,6 +286,15 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase):
         self.assertRaises(exceptions.UnsupportedOptionError,
                           self.driver.member_create, self.ref_member)
 
+    def test_member_create_no_subnet_provided_get_from_pool(self):
+        self.driver._ovn_helper._get_subnet_from_pool.return_value = (
+            self.ref_member.subnet_id)
+        member = copy.copy(self.ref_member)
+        member.subnet_id = data_models.UnsetType()
+        self._test_member_create(member)
+        member.subnet_id = None
+        self._test_member_create(member)
+
     def test_member_create_monitor_opts(self):
         self.ref_member.monitor_address = '172.20.20.1'
         self.assertRaises(exceptions.UnsupportedOptionError,
diff --git a/ovn_octavia_provider/tests/unit/test_helper.py b/ovn_octavia_provider/tests/unit/test_helper.py
index 33ae64c8..e38b621e 100644
--- a/ovn_octavia_provider/tests/unit/test_helper.py
+++ b/ovn_octavia_provider/tests/unit/test_helper.py
@@ -15,6 +15,7 @@ import copy
 from unittest import mock
 
 from neutronclient.common import exceptions as n_exc
+from octavia_lib.api.drivers import data_models
 from octavia_lib.api.drivers import exceptions
 from octavia_lib.common import constants
 from oslo_utils import uuidutils
@@ -290,6 +291,32 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase):
         found = f(self.ovn_lb.id)
         self.assertListEqual(found, [self.ovn_lb, udp_lb, sctp_lb])
 
+    def test__get_subnet_from_pool(self):
+        f = self.helper._get_subnet_from_pool
+
+        lb = data_models.LoadBalancer(
+            loadbalancer_id=self.loadbalancer_id,
+            name='The LB',
+            vip_address=self.vip_address,
+            vip_subnet_id=self.vip_subnet_id,
+            vip_network_id=self.vip_network_id)
+
+        lb_pool = data_models.Pool(
+            loadbalancer_id=self.loadbalancer_id,
+            name='The pool',
+            pool_id=self.pool_id,
+            protocol='TCP')
+
+        with mock.patch.object(self.helper, '_octavia_driver_lib') as dlib:
+            dlib.get_pool.return_value = None
+            found = f('not_found')
+            self.assertIsNone(found)
+
+            dlib.get_pool.return_value = lb_pool
+            dlib.get_loadbalancer.return_value = lb
+            found = f(self.pool_id)
+            self.assertEqual(found, lb.vip_subnet_id)
+
     def test__get_or_create_ovn_lb_no_lb_found(self):
         self.mock_find_ovn_lbs.stop()
         self.helper.ovn_nbdb_api.db_find_rows.return_value.\
diff --git a/releasenotes/notes/support-member-create-without-subnetid-0b4e3aa6ac453f28.yaml b/releasenotes/notes/support-member-create-without-subnetid-0b4e3aa6ac453f28.yaml
new file mode 100644
index 00000000..b799c7a8
--- /dev/null
+++ b/releasenotes/notes/support-member-create-without-subnetid-0b4e3aa6ac453f28.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Creating members without specifying a subnet ID is now supported.
+    Since the subnet ID is an optional API argument, if not given
+    the provider driver will now attempt to look it up via the pool
+    ID that is a required argument.