diff --git a/neutron_vpnaas/services/vpn/agent.py b/neutron_vpnaas/services/vpn/agent.py index 86810aeaa..70bffd65a 100644 --- a/neutron_vpnaas/services/vpn/agent.py +++ b/neutron_vpnaas/services/vpn/agent.py @@ -64,15 +64,20 @@ class VPNAgent(l3_extension.L3AgentExtension): if ri is not None: for device_driver in self.device_drivers: device_driver.create_router(ri) - device_driver.sync(context, [ri.router]) + device_driver.sync(context, [ri]) else: LOG.debug("Router %s was concurrently deleted while " "creating VPN for it", data['id']) def update_router(self, context, data): """Handles router update event""" - for device_driver in self.device_drivers: - device_driver.sync(context, [data]) + ri = self.agent_api.get_router_info(data['id']) + if ri is not None: + for device_driver in self.device_drivers: + device_driver.sync(context, [ri]) + else: + LOG.debug("Router %s was concurrently deleted while " + "updating VPN for it", data['id']) def delete_router(self, context, data): """Handles router delete event""" diff --git a/neutron_vpnaas/services/vpn/device_drivers/ipsec.py b/neutron_vpnaas/services/vpn/device_drivers/ipsec.py index 7ffcaac71..9207918bd 100644 --- a/neutron_vpnaas/services/vpn/device_drivers/ipsec.py +++ b/neutron_vpnaas/services/vpn/device_drivers/ipsec.py @@ -21,10 +21,12 @@ import re import shutil import socket import sys +from typing import List import eventlet import jinja2 import netaddr +from neutron.agent.l3.router_info import RouterInfo from neutron.agent.linux import ip_lib from neutron.agent.linux import utils as agent_utils from neutron_lib.api import validators @@ -863,7 +865,7 @@ class IPsecDriver(device_drivers.DeviceDriver, metaclass=abc.ABCMeta): return router.snat_namespace.name return router.ns_name - def get_router_based_iptables_manager(self, router): + def get_router_based_iptables_manager(self, ri): """Returns router based iptables manager In DVR routers the IPsec VPN service should run inside @@ -877,61 +879,28 @@ class IPsecDriver(device_drivers.DeviceDriver, metaclass=abc.ABCMeta): return the legacy iptables_manager. """ # TODO(pcm): Use router object method to tell if DVR, when available - if router.router['distributed']: - return router.snat_iptables_manager - return router.iptables_manager + if ri.router['distributed']: + return ri.snat_iptables_manager + return ri.iptables_manager - def add_nat_rule(self, router_id, chain, rule, top=False): - """Add nat rule in namespace. + def ensure_nat_rules(self, vpnservice): + """Ensure the required nat rules for ipsec exist in iptables. - :param router_id: router_id - :param chain: a string of chain name - :param rule: a string of rule - :param top: if top is true, the rule - will be placed on the top of chain - Note if there is no router, this method does nothing - """ - router = self.routers.get(router_id) - if not router: - return - iptables_manager = self.get_router_based_iptables_manager(router) - iptables_manager.ipv4['nat'].add_rule(chain, rule, top=top) - - def remove_nat_rule(self, router_id, chain, rule, top=False): - """Remove nat rule in namespace. - - :param router_id: router_id - :param chain: a string of chain name - :param rule: a string of rule - :param top: unused - needed to have same argument with add_nat_rule - """ - router = self.routers.get(router_id) - if not router: - return - iptables_manager = self.get_router_based_iptables_manager(router) - iptables_manager.ipv4['nat'].remove_rule(chain, rule, top=top) - - def iptables_apply(self, router_id): - """Apply IPtables. - - :param router_id: router_id - This method do nothing if there is no router - """ - router = self.routers.get(router_id) - if not router: - return - iptables_manager = self.get_router_based_iptables_manager(router) - iptables_manager.apply() - - def _update_nat(self, vpnservice, func): - """Setting up nat rule in iptables. - - We need to setup nat rule for ipsec packet. :param vpnservice: vpnservices - :param func: self.add_nat_rule or self.remove_nat_rule """ - router_id = vpnservice['router_id'] + LOG.debug("ensure_nat_rules called for router %s", + vpnservice['router_id']) + ri = self.routers.get(vpnservice['router_id']) + if not ri: + LOG.debug("No router info for router %s", vpnservice['router_id']) + return + + iptables_manager = self.get_router_based_iptables_manager(ri) + # clear all existing rules first + LOG.debug("Clearing vpnaas tagged NAT rules for router %s", + ri.router_id) + iptables_manager.ipv4['nat'].clear_rules_by_tag('vpnaas') + for ipsec_site_connection in vpnservice['ipsec_site_connections']: for local_cidr in ipsec_site_connection['local_cidrs']: # This ipsec rule is not needed for ipv6. @@ -939,13 +908,33 @@ class IPsecDriver(device_drivers.DeviceDriver, metaclass=abc.ABCMeta): continue for peer_cidr in ipsec_site_connection['peer_cidrs']: - func(router_id, - 'POSTROUTING', - '-s %s -d %s -m policy ' - '--dir out --pol ipsec ' - '-j ACCEPT ' % (local_cidr, peer_cidr), - top=True) - self.iptables_apply(router_id) + LOG.debug("Adding an ipsec policy NAT rule" + "%s <-> %s to router id %s", + peer_cidr, local_cidr, vpnservice['router_id']) + + iptables_manager.ipv4['nat'].add_rule( + 'POSTROUTING', + '-s %s -d %s -m policy ' + '--dir out --pol ipsec ' + '-j ACCEPT ' % (local_cidr, peer_cidr), + top=True, tag='vpnaas') + + LOG.debug("Applying iptables for router id %s", + vpnservice['router_id']) + iptables_manager.apply() + + @log_helpers.log_method_call + def remove_nat_rules(self, router_id): + """Remove all our iptables rules in namespace. + + :param router_id: router_id + """ + router = self.routers.get(router_id) + if not router: + return + iptables_manager = self.get_router_based_iptables_manager(router) + iptables_manager.ipv4['nat'].clear_rules_by_tag('vpnaas') + iptables_manager.apply() @log_helpers.log_method_call def vpnservice_updated(self, context, **kwargs): @@ -980,28 +969,28 @@ class IPsecDriver(device_drivers.DeviceDriver, metaclass=abc.ABCMeta): process.update_vpnservice(vpnservice) return process - def create_router(self, router): + def create_router(self, router_info: RouterInfo): """Handling create router event. Agent calls this method, when the process namespace is ready. Note: process_id == router_id == vpnservice_id """ - process_id = router.router_id - self.routers[process_id] = router + process_id = router_info.router_id + self.routers[process_id] = router_info if process_id in self.processes: # In case of vpnservice is created # before router's namespace process = self.processes[process_id] - self._update_nat(process.vpnservice, self.add_nat_rule) + self.ensure_nat_rules(process.vpnservice) # Don't run ipsec process for backup HA router - if router.router['ha'] and router.ha_state == 'backup': + if router_info.router['ha'] and router_info.ha_state == 'backup': return process.enable() def destroy_process(self, process_id): """Destroy process. - Disable the process, remove the nat rule, and remove the process + Disable the process, remove the iptables rules, and remove the process manager for the processes that no longer are running vpn service. """ if process_id in self.processes: @@ -1009,7 +998,7 @@ class IPsecDriver(device_drivers.DeviceDriver, metaclass=abc.ABCMeta): process.disable() vpnservice = process.vpnservice if vpnservice: - self._update_nat(vpnservice, self.remove_nat_rule) + self.remove_nat_rules(process_id) del self.processes[process_id] def destroy_router(self, process_id): @@ -1104,11 +1093,11 @@ class IPsecDriver(device_drivers.DeviceDriver, metaclass=abc.ABCMeta): @log_helpers.log_method_call @lockutils.synchronized('vpn-agent', 'neutron-') - def sync(self, context, routers): + def sync(self, context, router_information: List[RouterInfo]): """Sync status with server side. :param context: context object for RPC call - :param routers: Router objects which is created in this sync event + :param router_information: RouterInfo objects with updated state There could be many failure cases should be considered including the followings. @@ -1123,7 +1112,12 @@ class IPsecDriver(device_drivers.DeviceDriver, metaclass=abc.ABCMeta): vpnservices = self.agent_rpc.get_vpn_services_on_host( context, self.host) router_ids = [vpnservice['router_id'] for vpnservice in vpnservices] - sync_router_ids = [router['id'] for router in routers] + sync_router_ids = [] + + for ri in router_information: + # Update our router_info with the updated one + self.routers[ri.router_id] = ri + sync_router_ids.append(ri.router_id) self._sync_vpn_processes(vpnservices, sync_router_ids) self._delete_vpn_processes(sync_router_ids, router_ids) @@ -1140,13 +1134,13 @@ class IPsecDriver(device_drivers.DeviceDriver, metaclass=abc.ABCMeta): vpnservice['router_id'] in sync_router_ids): process = self.ensure_process(vpnservice['router_id'], vpnservice=vpnservice) - self._update_nat(vpnservice, self.add_nat_rule) - router = self.routers.get(vpnservice['router_id']) - if not router: + self.ensure_nat_rules(vpnservice) + ri = self.routers.get(vpnservice['router_id']) + if not ri: continue # For HA router, spawn vpn process on master router # and terminate vpn process on backup router - if router.router['ha'] and router.ha_state == 'backup': + if ri.router['ha'] and ri.ha_state == 'backup': process.disable() else: process.update() diff --git a/neutron_vpnaas/services/vpn/device_drivers/libreswan_ipsec.py b/neutron_vpnaas/services/vpn/device_drivers/libreswan_ipsec.py index 90731f7a4..0c45732d8 100644 --- a/neutron_vpnaas/services/vpn/device_drivers/libreswan_ipsec.py +++ b/neutron_vpnaas/services/vpn/device_drivers/libreswan_ipsec.py @@ -13,10 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. import os -import os.path from neutron.agent.linux import ip_lib - from neutron_vpnaas.services.vpn.device_drivers import ipsec diff --git a/neutron_vpnaas/tests/functional/common/test_scenario.py b/neutron_vpnaas/tests/functional/common/test_scenario.py index dd2f2f14b..89f55f111 100644 --- a/neutron_vpnaas/tests/functional/common/test_scenario.py +++ b/neutron_vpnaas/tests/functional/common/test_scenario.py @@ -523,8 +523,8 @@ class TestIPSecBase(framework.L3AgentTestFramework): local_router_id = site1.router.router_id peer_router_id = site2.router.router_id - self.driver.sync(mock.Mock(), [{'id': local_router_id}, - {'id': peer_router_id}]) + self.driver.sync(mock.Mock(), [site1.router, + site2.router]) self.agent._process_updated_router(site1.router.router) self.agent._process_updated_router(site2.router.router) self.addCleanup(self.driver._delete_vpn_processes, @@ -534,8 +534,7 @@ class TestIPSecBase(framework.L3AgentTestFramework): """Perform a sync on failover agent associated w/backup router.""" self.failover_driver.agent_rpc.get_vpn_services_on_host = mock.Mock( return_value=[site.vpn_service]) - self.failover_driver.sync(mock.Mock(), - [{'id': site.backup_router.router_id}]) + self.failover_driver.sync(mock.Mock(), [site.router]) def check_ping(self, from_site, to_site, instance=0, success=True): if success: diff --git a/neutron_vpnaas/tests/unit/services/vpn/device_drivers/test_ipsec.py b/neutron_vpnaas/tests/unit/services/vpn/device_drivers/test_ipsec.py index eab6ff2b8..1c7106b2b 100644 --- a/neutron_vpnaas/tests/unit/services/vpn/device_drivers/test_ipsec.py +++ b/neutron_vpnaas/tests/unit/services/vpn/device_drivers/test_ipsec.py @@ -35,12 +35,16 @@ from neutron_vpnaas.services.vpn.device_drivers import libreswan_ipsec from neutron_vpnaas.services.vpn.device_drivers import strongswan_ipsec from neutron_vpnaas.tests import base +# Note: process_id == router_id == vpnservice_id + _uuid = uuidutils.generate_uuid +FAKE_UUID = _uuid() FAKE_HOST = 'fake_host' -FAKE_ROUTER_ID = _uuid() +FAKE_ROUTER_ID = FAKE_UUID +FAKE_VPNSERVICE_ID = FAKE_UUID +FAKE_PROCESS_ID = FAKE_UUID FAKE_IPSEC_SITE_CONNECTION1_ID = _uuid() FAKE_IPSEC_SITE_CONNECTION2_ID = _uuid() -FAKE_VPNSERVICE_ID = _uuid() FAKE_IKE_POLICY = { 'ike_version': 'v1', 'encryption_algorithm': 'aes-128', @@ -431,13 +435,13 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): self._make_router_info_for_test() def _make_router_info_for_test(self): - self.router = legacy_router.LegacyRouter(router_id=FAKE_ROUTER_ID, + self.router_info = legacy_router.LegacyRouter(router_id=FAKE_ROUTER_ID, agent=self.agent, **self.ri_kwargs) - self.router.router['distributed'] = False - self.router.iptables_manager.ipv4['nat'] = self.iptables - self.router.iptables_manager.apply = self.apply_mock - self.driver.routers[FAKE_ROUTER_ID] = self.router + self.router_info.router['distributed'] = False + self.router_info.iptables_manager.ipv4['nat'] = self.iptables + self.router_info.iptables_manager.apply = self.apply_mock + self.driver.routers[FAKE_ROUTER_ID] = self.router_info def _test_vpnservice_updated(self, expected_param, **kwargs): with mock.patch.object(self.driver, 'sync') as sync: @@ -449,17 +453,16 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): self._test_vpnservice_updated([]) def test_vpnservice_updated_with_router_info(self): - router_info = {'id': FAKE_ROUTER_ID, 'ha': False} - kwargs = {'router': router_info} - self._test_vpnservice_updated([router_info], **kwargs) + kwargs = {'router': self.router_info} + self._test_vpnservice_updated([self.router_info], **kwargs) def test_create_router(self): process = mock.Mock(openswan_ipsec.OpenSwanProcess) process.vpnservice = self.vpnservice self.driver.processes = { FAKE_ROUTER_ID: process} - self.driver.create_router(self.router) - self._test_add_nat_rule() + self.driver.create_router(self.router_info) + self._test_ensure_nat_rules() process.enable.assert_called_once_with() def test_destroy_router(self): @@ -472,75 +475,89 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): process.disable.assert_called_once_with() self.assertNotIn(process_id, self.driver.processes) - def _test_add_nat_rule(self): - self.router.iptables_manager.ipv4['nat'].assert_has_calls([ + def _test_ensure_nat_rules(self): + self.router_info.iptables_manager.ipv4['nat'].assert_has_calls([ + mock.call.clear_rules_by_tag('vpnaas'), mock.call.add_rule( 'POSTROUTING', '-s 10.0.0.0/24 -d 20.0.0.0/24 -m policy ' '--dir out --pol ipsec -j ACCEPT ', - top=True), + top=True, + tag='vpnaas'), mock.call.add_rule( 'POSTROUTING', '-s 10.0.0.0/24 -d 30.0.0.0/24 -m policy ' '--dir out --pol ipsec -j ACCEPT ', - top=True), + top=True, + tag='vpnaas'), mock.call.add_rule( 'POSTROUTING', '-s 11.0.0.0/24 -d 40.0.0.0/24 -m policy ' '--dir out --pol ipsec -j ACCEPT ', - top=True), + top=True, + tag='vpnaas'), mock.call.add_rule( 'POSTROUTING', '-s 11.0.0.0/24 -d 50.0.0.0/24 -m policy ' '--dir out --pol ipsec -j ACCEPT ', - top=True) + top=True, + tag='vpnaas') ]) - self.router.iptables_manager.apply.assert_called_once_with() + self.router_info.iptables_manager.apply.assert_called_once_with() - def _test_add_nat_rule_with_multiple_locals(self): - self.router.iptables_manager.ipv4['nat'].assert_has_calls([ + def _test_ensure_nat_rules_with_multiple_locals(self): + self.router_info.iptables_manager.ipv4['nat'].assert_has_calls([ + mock.call.clear_rules_by_tag('vpnaas'), mock.call.add_rule( 'POSTROUTING', '-s 10.0.0.0/24 -d 20.0.0.0/24 -m policy ' '--dir out --pol ipsec -j ACCEPT ', - top=True), + top=True, + tag='vpnaas'), mock.call.add_rule( 'POSTROUTING', '-s 10.0.0.0/24 -d 30.0.0.0/24 -m policy ' '--dir out --pol ipsec -j ACCEPT ', - top=True), + top=True, + tag='vpnaas'), mock.call.add_rule( 'POSTROUTING', '-s 11.0.0.0/24 -d 20.0.0.0/24 -m policy ' '--dir out --pol ipsec -j ACCEPT ', - top=True), + top=True, + tag='vpnaas'), mock.call.add_rule( 'POSTROUTING', '-s 11.0.0.0/24 -d 30.0.0.0/24 -m policy ' '--dir out --pol ipsec -j ACCEPT ', - top=True), + top=True, + tag='vpnaas'), mock.call.add_rule( 'POSTROUTING', '-s 12.0.0.0/24 -d 40.0.0.0/24 -m policy ' '--dir out --pol ipsec -j ACCEPT ', - top=True), + top=True, + tag='vpnaas'), mock.call.add_rule( 'POSTROUTING', '-s 12.0.0.0/24 -d 50.0.0.0/24 -m policy ' '--dir out --pol ipsec -j ACCEPT ', - top=True), + top=True, + tag='vpnaas'), mock.call.add_rule( 'POSTROUTING', '-s 13.0.0.0/24 -d 40.0.0.0/24 -m policy ' '--dir out --pol ipsec -j ACCEPT ', - top=True), + top=True, + tag='vpnaas'), mock.call.add_rule( 'POSTROUTING', '-s 13.0.0.0/24 -d 50.0.0.0/24 -m policy ' '--dir out --pol ipsec -j ACCEPT ', - top=True) + top=True, + tag='vpnaas') ]) - self.router.iptables_manager.apply.assert_called_once_with() + self.router_info.iptables_manager.apply.assert_called_once_with() def test_sync(self): fake_vpn_service = FAKE_VPN_SERVICE @@ -550,9 +567,8 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): self.driver._sync_vpn_processes = mock.Mock() self.driver._delete_vpn_processes = mock.Mock() self.driver._cleanup_stale_vpn_processes = mock.Mock() - sync_routers = [{'id': fake_vpn_service['router_id']}] sync_router_ids = [fake_vpn_service['router_id']] - self.driver.sync(context, sync_routers) + self.driver.sync(context, [self.router_info]) self.driver._sync_vpn_processes.assert_called_once_with( [fake_vpn_service], sync_router_ids) self.driver._delete_vpn_processes.assert_called_once_with( @@ -567,16 +583,16 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): with mock.patch.object(self.driver, 'ensure_process') as ensure_p: ensure_p.side_effect = self.fake_ensure_process self.driver._sync_vpn_processes([new_vpnservice], router_id) - self._test_add_nat_rule() + self._test_ensure_nat_rules() self.driver.processes[router_id].update.assert_called_once_with() - def test_add_nat_rules_with_multiple_local_subnets(self): + def test_ensure_nat_rules_with_multiple_local_subnets(self): """Ensure that add nat rule combinations are correct.""" overrides = {'local_cidrs': [['10.0.0.0/24', '11.0.0.0/24'], ['12.0.0.0/24', '13.0.0.0/24']]} self.modify_config_for_test(overrides) - self.driver._update_nat(self.vpnservice, self.driver.add_nat_rule) - self._test_add_nat_rule_with_multiple_locals() + self.driver.ensure_nat_rules(self.vpnservice) + self._test_ensure_nat_rules_with_multiple_locals() def test__sync_vpn_processes_router_with_no_vpn(self): """Test _sync_vpn_processes with a router not hosting vpnservice. @@ -613,14 +629,18 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): is updated, _sync_vpn_processes restart/update the existing vpnservices which are not yet stored in driver.processes. """ - router_id = FAKE_ROUTER_ID self.driver.process_status_cache = {} self.driver.processes = {} with mock.patch.object(self.driver, 'ensure_process') as ensure_p: ensure_p.side_effect = self.fake_ensure_process - self.driver._sync_vpn_processes([self.vpnservice], [router_id]) - self._test_add_nat_rule() - self.driver.processes[router_id].update.assert_called_once_with() + self.driver._sync_vpn_processes( + [self.vpnservice], + [FAKE_ROUTER_ID] + ) + self._test_ensure_nat_rules() + self.driver.processes[ + FAKE_ROUTER_ID + ].update.assert_called_once_with() def test_delete_vpn_processes(self): router_id_no_vpn = _uuid() @@ -671,6 +691,8 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): if process: del self.driver.processes[process_id] + # TODO(crohmann): Add test cases for HARouter and different ha_states + # @ddt [(False, None),(True, 'primary'), (True, 'standby')] def test_sync_update_vpnservice(self): with mock.patch.object(self.driver, 'ensure_process') as ensure_process: @@ -683,12 +705,12 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): self.driver.process_status_cache = {} self.driver.agent_rpc.get_vpn_services_on_host.return_value = [ new_vpn_service] - self.driver.sync(context, [{'id': FAKE_ROUTER_ID}]) + self.driver.sync(context, [self.router_info]) process = self.driver.processes[FAKE_ROUTER_ID] self.assertEqual(new_vpn_service, process.vpnservice) self.driver.agent_rpc.get_vpn_services_on_host.return_value = [ updated_vpn_service] - self.driver.sync(context, [{'id': FAKE_ROUTER_ID}]) + self.driver.sync(context, [self.router_info]) process = self.driver.processes[FAKE_ROUTER_ID] process.update_vpnservice.assert_called_once_with( updated_vpn_service) @@ -710,7 +732,10 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): self.driver.agent_rpc.get_vpn_services_on_host.return_value = [] context = mock.Mock() process_id = _uuid() - self.driver.sync(context, [{'id': process_id}]) + ri = self.router_info + ri.router_id = process_id + ri.router['id'] = process_id + self.driver.sync(context, [self.router_info]) self.assertNotIn(process_id, self.driver.processes) def test_status_updated_on_connection_admin_down(self): @@ -781,7 +806,7 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): def _test_status_handling_for_downed_connection(self, down_status): """Test status handling for downed connection.""" - router_id = self.router.router_id + router_id = self.router_info.router_id connection_id = FAKE_IPSEC_SITE_CONNECTION2_ID self.driver.ensure_process(router_id, self.vpnservice) self._execute.return_value = down_status @@ -794,7 +819,7 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): def _test_status_handling_for_active_connection(self, active_status): """Test status handling for active connection.""" - router_id = self.router.router_id + router_id = self.router_info.router_id connection_id = FAKE_IPSEC_SITE_CONNECTION2_ID self.driver.ensure_process(router_id, self.vpnservice) self._execute.return_value = active_status @@ -809,7 +834,7 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): def _test_status_handling_for_ike_v2_active_connection(self, active_status): """Test status handling for active connection.""" - router_id = self.router.router_id + router_id = self.router_info.router_id connection_id = FAKE_IPSEC_SITE_CONNECTION2_ID ike_policy = {'ike_version': 'v2', 'encryption_algorithm': 'aes-128', @@ -832,7 +857,7 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): def _test_connection_names_handling_for_multiple_subnets(self, active_status): """Test connection names handling for multiple subnets.""" - router_id = self.router.router_id + router_id = self.router_info.router_id process = self.driver.ensure_process(router_id, self.vpnservice) self._execute.return_value = active_status names = process.get_established_connections() @@ -841,7 +866,7 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): def _test_status_handling_for_deleted_connection(self, not_running_status): """Test status handling for deleted connection.""" - router_id = self.router.router_id + router_id = self.router_info.router_id self.driver.ensure_process(router_id, self.vpnservice) self._execute.return_value = not_running_status self.driver.report_status(mock.Mock()) @@ -853,7 +878,7 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): def _test_parse_connection_status(self, not_running_status, active_status, down_status): """Test the status of ipsec-site-connection is parsed correctly.""" - router_id = self.router.router_id + router_id = self.router_info.router_id process = self.driver.ensure_process(router_id, self.vpnservice) self._execute.return_value = not_running_status self.assertFalse(process.active) @@ -873,41 +898,6 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver): def test_fail_getting_namespace_for_unknown_router(self): self.assertFalse(self.driver.get_namespace('bogus_id')) - def test_add_nat_rule(self): - self.driver.add_nat_rule(FAKE_ROUTER_ID, 'fake_chain', - 'fake_rule', True) - self.iptables.add_rule.assert_called_once_with( - 'fake_chain', 'fake_rule', top=True) - - def test_add_nat_rule_with_no_router(self): - self.driver.add_nat_rule( - 'bogus_router_id', - 'fake_chain', - 'fake_rule', - True) - self.assertFalse(self.iptables.add_rule.called) - - def test_remove_rule(self): - self.driver.remove_nat_rule(FAKE_ROUTER_ID, 'fake_chain', - 'fake_rule', True) - self.iptables.remove_rule.assert_called_once_with( - 'fake_chain', 'fake_rule', top=True) - - def test_remove_rule_with_no_router(self): - self.driver.remove_nat_rule( - 'bogus_router_id', - 'fake_chain', - 'fake_rule') - self.assertFalse(self.iptables.remove_rule.called) - - def test_iptables_apply(self): - self.driver.iptables_apply(FAKE_ROUTER_ID) - self.apply_mock.assert_called_once_with() - - def test_iptables_apply_with_no_router(self): - self.driver.iptables_apply('bogus_router_id') - self.assertFalse(self.apply_mock.called) - class IPSecDeviceDVR(BaseIPsecDeviceDriver): @@ -918,21 +908,23 @@ class IPSecDeviceDVR(BaseIPsecDeviceDriver): self._make_dvr_edge_router_info_for_test() def _make_dvr_edge_router_info_for_test(self): - router = dvr_edge_router.DvrEdgeRouter(mock.sentinel.agent, + router_info = dvr_edge_router.DvrEdgeRouter(mock.sentinel.agent, mock.sentinel.myhost, FAKE_ROUTER_ID, **self.ri_kwargs) - router.router['distributed'] = True - router.snat_namespace = dvr_snat_ns.SnatNamespace(router.router['id'], - mock.sentinel.agent, - self.driver, - mock.ANY) - router.snat_namespace.create() - router.snat_iptables_manager = iptables_manager.IptablesManager( + router_info.router['distributed'] = True + router_info.snat_namespace = dvr_snat_ns.SnatNamespace( + router_info.router['id'], + mock.sentinel.agent, + self.driver, + mock.ANY + ) + router_info.snat_namespace.create() + router_info.snat_iptables_manager = iptables_manager.IptablesManager( namespace='snat-' + FAKE_ROUTER_ID, use_ipv6=mock.ANY) - router.snat_iptables_manager.ipv4['nat'] = self.iptables - router.snat_iptables_manager.apply = self.apply_mock - self.driver.routers[FAKE_ROUTER_ID] = router + router_info.snat_iptables_manager.ipv4['nat'] = self.iptables + router_info.snat_iptables_manager.apply = self.apply_mock + self.driver.routers[FAKE_ROUTER_ID] = router_info def test_sync_dvr(self): fake_vpn_service = FAKE_VPN_SERVICE @@ -942,11 +934,10 @@ class IPSecDeviceDVR(BaseIPsecDeviceDriver): self.driver._sync_vpn_processes = mock.Mock() self.driver._delete_vpn_processes = mock.Mock() self.driver._cleanup_stale_vpn_processes = mock.Mock() - sync_routers = [{'id': fake_vpn_service['router_id']}] sync_router_ids = [fake_vpn_service['router_id']] with mock.patch.object(self.driver, 'get_process_status_cache') as process_status: - self.driver.sync(context, sync_routers) + self.driver.sync(context, [self.driver.routers[FAKE_ROUTER_ID]]) self.driver._sync_vpn_processes.assert_called_once_with( [fake_vpn_service], sync_router_ids) self.driver._delete_vpn_processes.assert_called_once_with( @@ -959,22 +950,10 @@ class IPSecDeviceDVR(BaseIPsecDeviceDriver): namespace = self.driver.get_namespace(FAKE_ROUTER_ID) self.assertEqual('snat-' + FAKE_ROUTER_ID, namespace) - def test_add_nat_rule_with_dvr_edge_router(self): - self.driver.add_nat_rule(FAKE_ROUTER_ID, 'fake_chain', - 'fake_rule', True) - self.iptables.add_rule.assert_called_once_with( - 'fake_chain', 'fake_rule', top=True) - - def test_iptables_apply_with_dvr_edge_router(self): - self.driver.iptables_apply(FAKE_ROUTER_ID) + def test_ensure_nat_rules_with_dvr_edge_router(self): + self.driver.ensure_nat_rules(FAKE_VPN_SERVICE) self.apply_mock.assert_called_once_with() - def test_remove_rule_with_dvr_edge_router(self): - self.driver.remove_nat_rule(FAKE_ROUTER_ID, 'fake_chain', - 'fake_rule', True) - self.iptables.remove_rule.assert_called_once_with( - 'fake_chain', 'fake_rule', top=True) - class TestOpenSwanConfigGeneration(BaseIPsecDeviceDriver): @@ -1293,7 +1272,7 @@ class TestOpenSwanProcess(IPSecDeviceLegacy): 'updated_pending_status': True}}) self.assertRaises(vpn_exception.VPNPeerAddressNotResolved, - self.process._get_nexthop, 'foo.peer.addr', + self.process._get_nexthop, 'foo.peer.addr.', 'fake-conn-id') self.assertEqual(expected_connection_status_dict, self.process.connection_status) @@ -1303,7 +1282,7 @@ class TestOpenSwanProcess(IPSecDeviceLegacy): 'updated_pending_status': False}}) self.assertRaises(vpn_exception.VPNPeerAddressNotResolved, - self.process._get_nexthop, 'foo.peer.addr', + self.process._get_nexthop, 'foo.peer.addr.', 'fake-conn-id') self.assertEqual(expected_connection_status_dict, self.process.connection_status) diff --git a/neutron_vpnaas/tests/unit/services/vpn/device_drivers/test_ovn_ipsec.py b/neutron_vpnaas/tests/unit/services/vpn/device_drivers/test_ovn_ipsec.py index 43fa0982d..3f1a1a2d5 100644 --- a/neutron_vpnaas/tests/unit/services/vpn/device_drivers/test_ovn_ipsec.py +++ b/neutron_vpnaas/tests/unit/services/vpn/device_drivers/test_ovn_ipsec.py @@ -197,7 +197,7 @@ class TestOvnStrongSwanDriver(test_ipsec.IPSecDeviceLegacy): 'transit_gateway_ip': '192.168.1.1', } - def test_iptables_apply(self): + def test_ensure_nat_rules(self): """Not applicable for OvnIPsecDriver""" pass @@ -218,19 +218,11 @@ class TestOvnStrongSwanDriver(test_ipsec.IPSecDeviceLegacy): """Not applicable for OvnIPsecDriver""" pass - def test_remove_rule(self): + def test_ensure_nat_rules_with_multiple_local_subnets(self): """Not applicable for OvnIPsecDriver""" pass - def test_add_nat_rules_with_multiple_local_subnets(self): - """Not applicable for OvnIPsecDriver""" - pass - - def _test_add_nat_rule(self): - """Not applicable for OvnIPsecDriver""" - pass - - def test_add_nat_rule(self): + def _test_ensure_nat_rules(self): """Not applicable for OvnIPsecDriver""" pass diff --git a/releasenotes/notes/bug1943449-899ba4711ff3586e.yaml b/releasenotes/notes/bug1943449-899ba4711ff3586e.yaml new file mode 100644 index 000000000..6f6714ffd --- /dev/null +++ b/releasenotes/notes/bug1943449-899ba4711ff3586e.yaml @@ -0,0 +1,11 @@ +--- +prelude: > + Due to an change in the IPtables NAT rule format, with the tag "vpnaas" + upgrading to this release requires either a machine reboot or a move of + all routers from this agent to ensure there is rules of the old format left. +fixes: + - | + Reconciling via the sync method has been improved to ensure no + `ha_state_change` event was missed. + Also all IPtables NAT rules are now tagged "vpnaas" and refreshed on sync + to ensure they are current and there are no duplicates. \ No newline at end of file