diff --git a/ovn_octavia_provider/driver.py b/ovn_octavia_provider/driver.py index a625a64c..081764d9 100644 --- a/ovn_octavia_provider/driver.py +++ b/ovn_octavia_provider/driver.py @@ -245,11 +245,15 @@ class OvnProviderDriver(driver_base.ProviderDriver): admin_state_up = member.admin_state_up 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: + subnet_id, subnet_cidr = self._ovn_helper._get_subnet_from_pool( + member.pool_id) + if not (subnet_id and + self._ovn_helper._check_ip_in_subnet(member.address, + subnet_cidr)): msg = _('Subnet is required, or Loadbalancer associated with ' 'Pool must have a subnet, for Member creation ' - 'with OVN Provider Driver') + 'with OVN Provider Driver if it is not the same as ' + 'LB VIP subnet') raise driver_exceptions.UnsupportedOptionError( user_fault_string=msg, operator_fault_string=msg) @@ -285,8 +289,18 @@ class OvnProviderDriver(driver_base.ProviderDriver): # the member is deleted, Octavia send the object without subnet_id. 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( + subnet_id, subnet_cidr = self._ovn_helper._get_subnet_from_pool( member.pool_id) + if not (subnet_id and + self._ovn_helper._check_ip_in_subnet(member.address, + subnet_cidr)): + msg = _('Subnet is required, or Loadbalancer associated with ' + 'Pool must have a subnet, for Member deletion if it is' + 'with OVN Provider Driver if it is not the same as ' + 'LB VIP subnet') + raise driver_exceptions.UnsupportedOptionError( + user_fault_string=msg, + operator_fault_string=msg) request_info = {'id': member.member_id, 'address': member.address, @@ -315,8 +329,18 @@ class OvnProviderDriver(driver_base.ProviderDriver): # the member is updated, Octavia send the object without subnet_id. subnet_id = old_member.subnet_id if (isinstance(subnet_id, o_datamodels.UnsetType) or not subnet_id): - subnet_id = self._ovn_helper._get_subnet_from_pool( + subnet_id, subnet_cidr = self._ovn_helper._get_subnet_from_pool( old_member.pool_id) + if not (subnet_id and + self._ovn_helper._check_ip_in_subnet(new_member.address, + subnet_cidr)): + msg = _('Subnet is required, or Loadbalancer associated with ' + 'Pool must have a subnet, for Member update ' + 'with OVN Provider Driver if it is not the same as ' + 'LB VIP subnet') + raise driver_exceptions.UnsupportedOptionError( + user_fault_string=msg, + operator_fault_string=msg) # Validate monitoring options if present self._check_member_monitor_options(new_member) @@ -353,19 +377,23 @@ class OvnProviderDriver(driver_base.ProviderDriver): subnet_id = member.subnet_id if (isinstance(subnet_id, o_datamodels.UnsetType) or not subnet_id): - pool_subnet_id = ( - pool_subnet_id - if pool_subnet_id - else self._ovn_helper._get_subnet_from_pool(pool_id)) - # NOTE(mjozefcz): We need to have subnet_id information. if not pool_subnet_id: + pool_subnet_id, pool_subnet_cidr = ( + self._ovn_helper._get_subnet_from_pool(pool_id)) + if pool_subnet_id: + if (self._ovn_helper._check_ip_in_subnet( + member.address, pool_subnet_cidr)): + member.subnet_id = pool_subnet_id + # NOTE(mjozefcz): We need to have subnet_id information. + if not member.subnet_id: msg = _('Subnet is required, or Loadbalancer associated ' 'with Pool must have a subnet, for Member ' - 'creation with OVN Provider Driver') + 'batch update with OVN Provider Driver if it is ' + 'not the same as LB VIP subnet') raise driver_exceptions.UnsupportedOptionError( user_fault_string=msg, operator_fault_string=msg) - member.subnet_id = pool_subnet_id + admin_state_up = member.admin_state_up if isinstance(admin_state_up, o_datamodels.UnsetType): admin_state_up = True diff --git a/ovn_octavia_provider/helper.py b/ovn_octavia_provider/helper.py index 0c0a5cbb..258d3431 100644 --- a/ovn_octavia_provider/helper.py +++ b/ovn_octavia_provider/helper.py @@ -490,13 +490,25 @@ class OvnProviderHelper(): ovn_lb = self._find_ovn_lb_with_pool_key(pool_key) return pool_key, ovn_lb + def _check_ip_in_subnet(self, ip, subnet): + return (netaddr.IPAddress(ip) in netaddr.IPNetwork(subnet)) + def _get_subnet_from_pool(self, pool_id): pool = self._octavia_driver_lib.get_pool(pool_id) if not pool: - return + return None, None lb = self._octavia_driver_lib.get_loadbalancer(pool.loadbalancer_id) if lb and lb.vip_subnet_id: - return lb.vip_subnet_id + neutron_client = clients.get_neutron_client() + try: + subnet = neutron_client.show_subnet(lb.vip_subnet_id) + vip_subnet_cidr = subnet['subnet']['cidr'] + except n_exc.NotFound: + LOG.warning('Subnet %s not found while trying to ' + 'fetch its data.', lb.vip_subnet_id) + return None, None + return lb.vip_subnet_id, vip_subnet_cidr + return None, None def _execute_commands(self, commands): with self.ovn_nbdb_api.transaction(check_error=True) as txn: diff --git a/ovn_octavia_provider/tests/functional/base.py b/ovn_octavia_provider/tests/functional/base.py index 661c8b2d..8886d4fb 100644 --- a/ovn_octavia_provider/tests/functional/base.py +++ b/ovn_octavia_provider/tests/functional/base.py @@ -62,12 +62,14 @@ class TestOvnOctaviaBase(base.TestOVNFunctionalBase, self.fake_neutron_client.show_port = self._mock_show_port self.fake_neutron_client.delete_port.return_value = True self._local_net_cache = {} + self._local_cidr_cache = {} self._local_port_cache = {'ports': []} self.core_plugin = directory.get_plugin() def _mock_show_subnet(self, subnet_id): subnet = {} subnet['network_id'] = self._local_net_cache[subnet_id] + subnet['cidr'] = self._local_cidr_cache[subnet_id] return {'subnet': subnet} def _mock_list_ports(self, **kwargs): @@ -253,6 +255,7 @@ class TestOvnOctaviaBase(base.TestOVNFunctionalBase, cidr) subnet = self.deserialize(self.fmt, res)['subnet'] self._local_net_cache[subnet['id']] = n1['network']['id'] + self._local_cidr_cache[subnet['id']] = subnet['cidr'] port = self._make_port(self.fmt, n1['network']['id']) if router_id: diff --git a/ovn_octavia_provider/tests/unit/test_driver.py b/ovn_octavia_provider/tests/unit/test_driver.py index ed8aa5fc..2bc79652 100644 --- a/ovn_octavia_provider/tests/unit/test_driver.py +++ b/ovn_octavia_provider/tests/unit/test_driver.py @@ -227,7 +227,11 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase): self.mock_get_subnet_from_pool = mock.patch.object( ovn_helper.OvnProviderHelper, '_get_subnet_from_pool', - return_value=None).start() + return_value=(None, None)).start() + self.mock_check_ip_in_subnet = mock.patch.object( + ovn_helper.OvnProviderHelper, + '_check_ip_in_subnet', + return_value=True).start() def test_check_for_allowed_cidrs_exception(self): self.assertRaises(exceptions.UnsupportedOptionError, @@ -301,7 +305,18 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase): 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) + self.ref_member.subnet_id, '198.52.100.0/24') + self.driver._ovn_helper._check_ip_in_subnet.return_value = False + self.ref_member.subnet_id = data_models.UnsetType() + self.assertRaises(exceptions.UnsupportedOptionError, + self.driver.member_create, self.ref_member) + self.ref_member.subnet_id = None + self.assertRaises(exceptions.UnsupportedOptionError, + self.driver.member_create, self.ref_member) + + def test_member_create_no_subnet_provided_get_from_pool_failed(self): + self.driver._ovn_helper._get_subnet_from_pool.return_value = ( + self.ref_member.subnet_id, '198.52.100.0/24') member = copy.copy(self.ref_member) member.subnet_id = data_models.UnsetType() self._test_member_create(member) @@ -349,7 +364,7 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase): def test_member_update_missing_subnet_id(self): self.driver._ovn_helper._get_subnet_from_pool.return_value = ( - self.ref_member.subnet_id) + self.ref_member.subnet_id, '198.52.100.0/24') info = {'id': self.update_member.member_id, 'address': self.ref_member.address, 'protocol_port': self.ref_member.protocol_port, @@ -366,7 +381,7 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase): def test_member_update_unset_admin_state_up(self): self.driver._ovn_helper._get_subnet_from_pool.return_value = ( - self.ref_member.subnet_id) + self.ref_member.subnet_id, '198.52.100.0/24') self.update_member.admin_state_up = data_models.UnsetType() info = {'id': self.update_member.member_id, 'address': self.ref_member.address, @@ -428,10 +443,19 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase): def test_member_batch_update_missing_subnet_id_get_from_pool(self): self.driver._ovn_helper._get_subnet_from_pool.return_value = ( - self.ref_member.subnet_id) + self.ref_member.subnet_id, '198.52.100.0/24') self.ref_member.subnet_id = None self.driver.member_batch_update(self.pool_id, [self.ref_member]) + def test_member_batch_update_missing_subnet_id_get_from_pool_fail(self): + self.driver._ovn_helper._get_subnet_from_pool.return_value = ( + self.ref_member.subnet_id, '198.52.100.0/24') + self.driver._ovn_helper._check_ip_in_subnet.return_value = False + self.ref_member.subnet_id = None + self.assertRaises(exceptions.UnsupportedOptionError, + self.driver.member_batch_update, + self.pool_id, [self.ref_member]) + def test_member_update_failure(self): self.assertRaises(exceptions.UnsupportedOptionError, self.driver.member_update, self.ref_member, @@ -468,7 +492,7 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase): def test_member_delete_missing_subnet_id(self): self.driver._ovn_helper._get_subnet_from_pool.return_value = ( - self.ref_member.subnet_id) + self.ref_member.subnet_id, '198.52.100.0/24') info = {'id': self.ref_member.member_id, 'address': self.ref_member.address, 'protocol_port': self.ref_member.protocol_port, diff --git a/ovn_octavia_provider/tests/unit/test_helper.py b/ovn_octavia_provider/tests/unit/test_helper.py index 3e2b3916..8b20084e 100644 --- a/ovn_octavia_provider/tests/unit/test_helper.py +++ b/ovn_octavia_provider/tests/unit/test_helper.py @@ -376,7 +376,11 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): found = f(self.ovn_lb.id, protocol='tcp') self.assertEqual(found, self.ovn_lb) - def test__get_subnet_from_pool(self): + @mock.patch('ovn_octavia_provider.common.clients.get_neutron_client') + def test__get_subnet_from_pool(self, net_cli): + net_cli.return_value.show_subnet.return_value = { + 'subnet': {'cidr': '10.22.33.0/24'}} + f = self.helper._get_subnet_from_pool lb = data_models.LoadBalancer( @@ -395,12 +399,12 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): with mock.patch.object(self.helper, '_octavia_driver_lib') as dlib: dlib.get_pool.return_value = None found = f('not_found') - self.assertIsNone(found) + self.assertEqual((None, None), 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) + self.assertEqual(found, (lb.vip_subnet_id, '10.22.33.0/24')) def test__get_subnet_from_pool_lb_no_vip_subnet_id(self): f = self.helper._get_subnet_from_pool @@ -420,12 +424,12 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): with mock.patch.object(self.helper, '_octavia_driver_lib') as dlib: dlib.get_pool.return_value = None found = f('not_found') - self.assertIsNone(found) + self.assertEqual((None, None), found) dlib.get_pool.return_value = lb_pool dlib.get_loadbalancer.return_value = lb found = f(self.pool_id) - self.assertIsNone(found) + self.assertEqual((None, None), found) def test__get_or_create_ovn_lb_no_lb_found(self): self.mock_find_ovn_lbs.stop()