Improve the reconciling for VPNaaS

* Update the router (holding RouterInfo objects) with current data
   on update / sync. Since the sync method should reconcile out of
   any state, we need to update the RouterInfo we store locally in
   the driver to ensure we have not missed e.g. a ha_state_change.

 * Consistently use RouterInfo instead of some mix of dict and Router
   and RouterInfo.

 * Ensure NAT rules are current by using a tag to clean them all and
   then re-create the currently required rules before applying them via
   iptables manager. This ensures there are no dangling rules or duplicates.

Co-Authored-By: Niklas Schwarz <niklas.schwarz@inovex.de>
Closes-Bug: https://bugs.launchpad.net/neutron/+bug/1943449

Change-Id: I378ba5a0b500110ce5f9293a885730c0a62578b0

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"""

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

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
 
 

neutron_vpnaas/tests/functional/common/ovn_base.py

@@ -381,7 +381,7 @@ class TestOvnVPNAgentBase(base.TestOVNFunctionalBase):
             version=self.vpn_service_driver.agent_rpc.target.version)
         prepared_mock.cast.assert_called_once_with(
             self.context, 'vpnservice_updated',
-            router={'id': site.router['id']})
+            router={'router_id': site.router['id']})
 
         # Mock the agent->controller RPCs. Let them return data from the
         # actual VPN plugin

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:

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)

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
 

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.