openstack
/
neutron-vpnaas
Code Issues Proposed changes

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
This commit is contained in:
Christian Rohmann 2022-11-08 10:41:05 +01:00
parent f5aa16a305
commit 6e1280c304
7 changed files with 184 additions and 206 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
neutron_vpnaas/services/vpn/agent.py
View File

@ -64,15 +64,20 @@ class VPNAgent(l3_extension.L3AgentExtension):
if ri is not None: if ri is not None:
for device_driver in self.device_drivers: for device_driver in self.device_drivers:
device_driver.create_router(ri) device_driver.create_router(ri)
device_driver.sync(context, [ri.router]) device_driver.sync(context, [ri])
else: else:
LOG.debug("Router %s was concurrently deleted while " LOG.debug("Router %s was concurrently deleted while "
"creating VPN for it", data['id']) "creating VPN for it", data['id'])
def update_router(self, context, data): def update_router(self, context, data):
"""Handles router update event""" """Handles router update event"""
for device_driver in self.device_drivers: ri = self.agent_api.get_router_info(data['id'])
device_driver.sync(context, [data]) 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): def delete_router(self, context, data):
"""Handles router delete event""" """Handles router delete event"""

140
neutron_vpnaas/services/vpn/device_drivers/ipsec.py
View File

@ -21,10 +21,12 @@ import re
import shutil import shutil
import socket import socket
import sys import sys
from typing import List
import eventlet import eventlet
import jinja2 import jinja2
import netaddr import netaddr
from neutron.agent.l3.router_info import RouterInfo
from neutron.agent.linux import ip_lib from neutron.agent.linux import ip_lib
from neutron.agent.linux import utils as agent_utils from neutron.agent.linux import utils as agent_utils
from neutron_lib.api import validators 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.snat_namespace.name
return router.ns_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 """Returns router based iptables manager
In DVR routers the IPsec VPN service should run inside 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. return the legacy iptables_manager.
""" """
# TODO(pcm): Use router object method to tell if DVR, when available # TODO(pcm): Use router object method to tell if DVR, when available
if router.router['distributed']: if ri.router['distributed']:
return router.snat_iptables_manager return ri.snat_iptables_manager
return router.iptables_manager return ri.iptables_manager
def add_nat_rule(self, router_id, chain, rule, top=False): def ensure_nat_rules(self, vpnservice):
"""Add nat rule in namespace. """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 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 ipsec_site_connection in vpnservice['ipsec_site_connections']:
for local_cidr in ipsec_site_connection['local_cidrs']: for local_cidr in ipsec_site_connection['local_cidrs']:
# This ipsec rule is not needed for ipv6. # This ipsec rule is not needed for ipv6.
@ -939,13 +908,33 @@ class IPsecDriver(device_drivers.DeviceDriver, metaclass=abc.ABCMeta):
continue continue
for peer_cidr in ipsec_site_connection['peer_cidrs']: for peer_cidr in ipsec_site_connection['peer_cidrs']:
func(router_id, LOG.debug("Adding an ipsec policy NAT rule"
'POSTROUTING', "%s <-> %s to router id %s",
'-s %s -d %s -m policy ' peer_cidr, local_cidr, vpnservice['router_id'])
'--dir out --pol ipsec '
'-j ACCEPT ' % (local_cidr, peer_cidr), iptables_manager.ipv4['nat'].add_rule(
top=True) 'POSTROUTING',
self.iptables_apply(router_id) '-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 @log_helpers.log_method_call
def vpnservice_updated(self, context, **kwargs): def vpnservice_updated(self, context, **kwargs):
@ -980,28 +969,28 @@ class IPsecDriver(device_drivers.DeviceDriver, metaclass=abc.ABCMeta):
process.update_vpnservice(vpnservice) process.update_vpnservice(vpnservice)
return process return process
def create_router(self, router): def create_router(self, router_info: RouterInfo):
"""Handling create router event. """Handling create router event.
Agent calls this method, when the process namespace is ready. Agent calls this method, when the process namespace is ready.
Note: process_id == router_id == vpnservice_id Note: process_id == router_id == vpnservice_id
""" """
process_id = router.router_id process_id = router_info.router_id
self.routers[process_id] = router self.routers[process_id] = router_info
if process_id in self.processes: if process_id in self.processes:
# In case of vpnservice is created # In case of vpnservice is created
# before router's namespace # before router's namespace
process = self.processes[process_id] 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 # 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 return
process.enable() process.enable()
def destroy_process(self, process_id): def destroy_process(self, process_id):
"""Destroy process. """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. manager for the processes that no longer are running vpn service.
""" """
if process_id in self.processes: if process_id in self.processes:
@ -1009,7 +998,7 @@ class IPsecDriver(device_drivers.DeviceDriver, metaclass=abc.ABCMeta):
process.disable() process.disable()
vpnservice = process.vpnservice vpnservice = process.vpnservice
if vpnservice: if vpnservice:
self._update_nat(vpnservice, self.remove_nat_rule) self.remove_nat_rules(process_id)
del self.processes[process_id] del self.processes[process_id]
def destroy_router(self, 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 @log_helpers.log_method_call
@lockutils.synchronized('vpn-agent', 'neutron-') @lockutils.synchronized('vpn-agent', 'neutron-')
def sync(self, context, routers): def sync(self, context, router_information: List[RouterInfo]):
"""Sync status with server side. """Sync status with server side.
:param context: context object for RPC call :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 There could be many failure cases should be
considered including the followings. 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( vpnservices = self.agent_rpc.get_vpn_services_on_host(
context, self.host) context, self.host)
router_ids = [vpnservice['router_id'] for vpnservice in vpnservices] 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._sync_vpn_processes(vpnservices, sync_router_ids)
self._delete_vpn_processes(sync_router_ids, 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): vpnservice['router_id'] in sync_router_ids):
process = self.ensure_process(vpnservice['router_id'], process = self.ensure_process(vpnservice['router_id'],
vpnservice=vpnservice) vpnservice=vpnservice)
self._update_nat(vpnservice, self.add_nat_rule) self.ensure_nat_rules(vpnservice)
router = self.routers.get(vpnservice['router_id']) ri = self.routers.get(vpnservice['router_id'])
if not router: if not ri:
continue continue
# For HA router, spawn vpn process on master router # For HA router, spawn vpn process on master router
# and terminate vpn process on backup 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() process.disable()
else: else:
process.update() process.update()

2
neutron_vpnaas/services/vpn/device_drivers/libreswan_ipsec.py
View File

@ -13,10 +13,8 @@
# License for the specific language governing permissions and limitations # License for the specific language governing permissions and limitations
# under the License. # under the License.
import os import os
import os.path
from neutron.agent.linux import ip_lib from neutron.agent.linux import ip_lib
from neutron_vpnaas.services.vpn.device_drivers import ipsec from neutron_vpnaas.services.vpn.device_drivers import ipsec

7
neutron_vpnaas/tests/functional/common/test_scenario.py
View File

@ -523,8 +523,8 @@ class TestIPSecBase(framework.L3AgentTestFramework):
local_router_id = site1.router.router_id local_router_id = site1.router.router_id
peer_router_id = site2.router.router_id peer_router_id = site2.router.router_id
self.driver.sync(mock.Mock(), [{'id': local_router_id}, self.driver.sync(mock.Mock(), [site1.router,
{'id': peer_router_id}]) site2.router])
self.agent._process_updated_router(site1.router.router) self.agent._process_updated_router(site1.router.router)
self.agent._process_updated_router(site2.router.router) self.agent._process_updated_router(site2.router.router)
self.addCleanup(self.driver._delete_vpn_processes, 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.""" """Perform a sync on failover agent associated w/backup router."""
self.failover_driver.agent_rpc.get_vpn_services_on_host = mock.Mock( self.failover_driver.agent_rpc.get_vpn_services_on_host = mock.Mock(
return_value=[site.vpn_service]) return_value=[site.vpn_service])
self.failover_driver.sync(mock.Mock(), self.failover_driver.sync(mock.Mock(), [site.router])
[{'id': site.backup_router.router_id}])
def check_ping(self, from_site, to_site, instance=0, success=True): def check_ping(self, from_site, to_site, instance=0, success=True):
if success: if success:

205
neutron_vpnaas/tests/unit/services/vpn/device_drivers/test_ipsec.py
View File

@ -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.services.vpn.device_drivers import strongswan_ipsec
from neutron_vpnaas.tests import base from neutron_vpnaas.tests import base
# Note: process_id == router_id == vpnservice_id
_uuid = uuidutils.generate_uuid _uuid = uuidutils.generate_uuid
FAKE_UUID = _uuid()
FAKE_HOST = 'fake_host' 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_CONNECTION1_ID = _uuid()
FAKE_IPSEC_SITE_CONNECTION2_ID = _uuid() FAKE_IPSEC_SITE_CONNECTION2_ID = _uuid()
FAKE_VPNSERVICE_ID = _uuid()
FAKE_IKE_POLICY = { FAKE_IKE_POLICY = {
'ike_version': 'v1', 'ike_version': 'v1',
'encryption_algorithm': 'aes-128', 'encryption_algorithm': 'aes-128',
@ -431,13 +435,13 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver):
self._make_router_info_for_test() self._make_router_info_for_test()
def _make_router_info_for_test(self): 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, agent=self.agent,
**self.ri_kwargs) **self.ri_kwargs)
self.router.router['distributed'] = False self.router_info.router['distributed'] = False
self.router.iptables_manager.ipv4['nat'] = self.iptables self.router_info.iptables_manager.ipv4['nat'] = self.iptables
self.router.iptables_manager.apply = self.apply_mock self.router_info.iptables_manager.apply = self.apply_mock
self.driver.routers[FAKE_ROUTER_ID] = self.router self.driver.routers[FAKE_ROUTER_ID] = self.router_info
def _test_vpnservice_updated(self, expected_param, **kwargs): def _test_vpnservice_updated(self, expected_param, **kwargs):
with mock.patch.object(self.driver, 'sync') as sync: with mock.patch.object(self.driver, 'sync') as sync:
@ -449,17 +453,16 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver):
self._test_vpnservice_updated([]) self._test_vpnservice_updated([])
def test_vpnservice_updated_with_router_info(self): def test_vpnservice_updated_with_router_info(self):
router_info = {'id': FAKE_ROUTER_ID, 'ha': False} kwargs = {'router': self.router_info}
kwargs = {'router': router_info} self._test_vpnservice_updated([self.router_info], **kwargs)
self._test_vpnservice_updated([router_info], **kwargs)
def test_create_router(self): def test_create_router(self):
process = mock.Mock(openswan_ipsec.OpenSwanProcess) process = mock.Mock(openswan_ipsec.OpenSwanProcess)
process.vpnservice = self.vpnservice process.vpnservice = self.vpnservice
self.driver.processes = { self.driver.processes = {
FAKE_ROUTER_ID: process} FAKE_ROUTER_ID: process}
self.driver.create_router(self.router) self.driver.create_router(self.router_info)
self._test_add_nat_rule() self._test_ensure_nat_rules()
process.enable.assert_called_once_with() process.enable.assert_called_once_with()
def test_destroy_router(self): def test_destroy_router(self):
@ -472,75 +475,89 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver):
process.disable.assert_called_once_with() process.disable.assert_called_once_with()
self.assertNotIn(process_id, self.driver.processes) self.assertNotIn(process_id, self.driver.processes)
def _test_add_nat_rule(self): def _test_ensure_nat_rules(self):
self.router.iptables_manager.ipv4['nat'].assert_has_calls([ self.router_info.iptables_manager.ipv4['nat'].assert_has_calls([
mock.call.clear_rules_by_tag('vpnaas'),
mock.call.add_rule( mock.call.add_rule(
'POSTROUTING', 'POSTROUTING',
'-s 10.0.0.0/24 -d 20.0.0.0/24 -m policy ' '-s 10.0.0.0/24 -d 20.0.0.0/24 -m policy '
'--dir out --pol ipsec -j ACCEPT ', '--dir out --pol ipsec -j ACCEPT ',
top=True), top=True,
tag='vpnaas'),
mock.call.add_rule( mock.call.add_rule(
'POSTROUTING', 'POSTROUTING',
'-s 10.0.0.0/24 -d 30.0.0.0/24 -m policy ' '-s 10.0.0.0/24 -d 30.0.0.0/24 -m policy '
'--dir out --pol ipsec -j ACCEPT ', '--dir out --pol ipsec -j ACCEPT ',
top=True), top=True,
tag='vpnaas'),
mock.call.add_rule( mock.call.add_rule(
'POSTROUTING', 'POSTROUTING',
'-s 11.0.0.0/24 -d 40.0.0.0/24 -m policy ' '-s 11.0.0.0/24 -d 40.0.0.0/24 -m policy '
'--dir out --pol ipsec -j ACCEPT ', '--dir out --pol ipsec -j ACCEPT ',
top=True), top=True,
tag='vpnaas'),
mock.call.add_rule( mock.call.add_rule(
'POSTROUTING', 'POSTROUTING',
'-s 11.0.0.0/24 -d 50.0.0.0/24 -m policy ' '-s 11.0.0.0/24 -d 50.0.0.0/24 -m policy '
'--dir out --pol ipsec -j ACCEPT ', '--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): def _test_ensure_nat_rules_with_multiple_locals(self):
self.router.iptables_manager.ipv4['nat'].assert_has_calls([ self.router_info.iptables_manager.ipv4['nat'].assert_has_calls([
mock.call.clear_rules_by_tag('vpnaas'),
mock.call.add_rule( mock.call.add_rule(
'POSTROUTING', 'POSTROUTING',
'-s 10.0.0.0/24 -d 20.0.0.0/24 -m policy ' '-s 10.0.0.0/24 -d 20.0.0.0/24 -m policy '
'--dir out --pol ipsec -j ACCEPT ', '--dir out --pol ipsec -j ACCEPT ',
top=True), top=True,
tag='vpnaas'),
mock.call.add_rule( mock.call.add_rule(
'POSTROUTING', 'POSTROUTING',
'-s 10.0.0.0/24 -d 30.0.0.0/24 -m policy ' '-s 10.0.0.0/24 -d 30.0.0.0/24 -m policy '
'--dir out --pol ipsec -j ACCEPT ', '--dir out --pol ipsec -j ACCEPT ',
top=True), top=True,
tag='vpnaas'),
mock.call.add_rule( mock.call.add_rule(
'POSTROUTING', 'POSTROUTING',
'-s 11.0.0.0/24 -d 20.0.0.0/24 -m policy ' '-s 11.0.0.0/24 -d 20.0.0.0/24 -m policy '
'--dir out --pol ipsec -j ACCEPT ', '--dir out --pol ipsec -j ACCEPT ',
top=True), top=True,
tag='vpnaas'),
mock.call.add_rule( mock.call.add_rule(
'POSTROUTING', 'POSTROUTING',
'-s 11.0.0.0/24 -d 30.0.0.0/24 -m policy ' '-s 11.0.0.0/24 -d 30.0.0.0/24 -m policy '
'--dir out --pol ipsec -j ACCEPT ', '--dir out --pol ipsec -j ACCEPT ',
top=True), top=True,
tag='vpnaas'),
mock.call.add_rule( mock.call.add_rule(
'POSTROUTING', 'POSTROUTING',
'-s 12.0.0.0/24 -d 40.0.0.0/24 -m policy ' '-s 12.0.0.0/24 -d 40.0.0.0/24 -m policy '
'--dir out --pol ipsec -j ACCEPT ', '--dir out --pol ipsec -j ACCEPT ',
top=True), top=True,
tag='vpnaas'),
mock.call.add_rule( mock.call.add_rule(
'POSTROUTING', 'POSTROUTING',
'-s 12.0.0.0/24 -d 50.0.0.0/24 -m policy ' '-s 12.0.0.0/24 -d 50.0.0.0/24 -m policy '
'--dir out --pol ipsec -j ACCEPT ', '--dir out --pol ipsec -j ACCEPT ',
top=True), top=True,
tag='vpnaas'),
mock.call.add_rule( mock.call.add_rule(
'POSTROUTING', 'POSTROUTING',
'-s 13.0.0.0/24 -d 40.0.0.0/24 -m policy ' '-s 13.0.0.0/24 -d 40.0.0.0/24 -m policy '
'--dir out --pol ipsec -j ACCEPT ', '--dir out --pol ipsec -j ACCEPT ',
top=True), top=True,
tag='vpnaas'),
mock.call.add_rule( mock.call.add_rule(
'POSTROUTING', 'POSTROUTING',
'-s 13.0.0.0/24 -d 50.0.0.0/24 -m policy ' '-s 13.0.0.0/24 -d 50.0.0.0/24 -m policy '
'--dir out --pol ipsec -j ACCEPT ', '--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): def test_sync(self):
fake_vpn_service = FAKE_VPN_SERVICE fake_vpn_service = FAKE_VPN_SERVICE
@ -550,9 +567,8 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver):
self.driver._sync_vpn_processes = mock.Mock() self.driver._sync_vpn_processes = mock.Mock()
self.driver._delete_vpn_processes = mock.Mock() self.driver._delete_vpn_processes = mock.Mock()
self.driver._cleanup_stale_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']] 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( self.driver._sync_vpn_processes.assert_called_once_with(
[fake_vpn_service], sync_router_ids) [fake_vpn_service], sync_router_ids)
self.driver._delete_vpn_processes.assert_called_once_with( 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: with mock.patch.object(self.driver, 'ensure_process') as ensure_p:
ensure_p.side_effect = self.fake_ensure_process ensure_p.side_effect = self.fake_ensure_process
self.driver._sync_vpn_processes([new_vpnservice], router_id) 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() 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.""" """Ensure that add nat rule combinations are correct."""
overrides = {'local_cidrs': [['10.0.0.0/24', '11.0.0.0/24'], overrides = {'local_cidrs': [['10.0.0.0/24', '11.0.0.0/24'],
['12.0.0.0/24', '13.0.0.0/24']]} ['12.0.0.0/24', '13.0.0.0/24']]}
self.modify_config_for_test(overrides) self.modify_config_for_test(overrides)
self.driver._update_nat(self.vpnservice, self.driver.add_nat_rule) self.driver.ensure_nat_rules(self.vpnservice)
self._test_add_nat_rule_with_multiple_locals() self._test_ensure_nat_rules_with_multiple_locals()
def test__sync_vpn_processes_router_with_no_vpn(self): def test__sync_vpn_processes_router_with_no_vpn(self):
"""Test _sync_vpn_processes with a router not hosting vpnservice. """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 is updated, _sync_vpn_processes restart/update the existing vpnservices
which are not yet stored in driver.processes. which are not yet stored in driver.processes.
""" """
router_id = FAKE_ROUTER_ID
self.driver.process_status_cache = {} self.driver.process_status_cache = {}
self.driver.processes = {} self.driver.processes = {}
with mock.patch.object(self.driver, 'ensure_process') as ensure_p: with mock.patch.object(self.driver, 'ensure_process') as ensure_p:
ensure_p.side_effect = self.fake_ensure_process ensure_p.side_effect = self.fake_ensure_process
self.driver._sync_vpn_processes([self.vpnservice], [router_id]) self.driver._sync_vpn_processes(
self._test_add_nat_rule() [self.vpnservice],
self.driver.processes[router_id].update.assert_called_once_with() [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): def test_delete_vpn_processes(self):
router_id_no_vpn = _uuid() router_id_no_vpn = _uuid()
@ -671,6 +691,8 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver):
if process: if process:
del self.driver.processes[process_id] 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): def test_sync_update_vpnservice(self):
with mock.patch.object(self.driver, with mock.patch.object(self.driver,
'ensure_process') as ensure_process: 'ensure_process') as ensure_process:
@ -683,12 +705,12 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver):
self.driver.process_status_cache = {} self.driver.process_status_cache = {}
self.driver.agent_rpc.get_vpn_services_on_host.return_value = [ self.driver.agent_rpc.get_vpn_services_on_host.return_value = [
new_vpn_service] 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] process = self.driver.processes[FAKE_ROUTER_ID]
self.assertEqual(new_vpn_service, process.vpnservice) self.assertEqual(new_vpn_service, process.vpnservice)
self.driver.agent_rpc.get_vpn_services_on_host.return_value = [ self.driver.agent_rpc.get_vpn_services_on_host.return_value = [
updated_vpn_service] 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 = self.driver.processes[FAKE_ROUTER_ID]
process.update_vpnservice.assert_called_once_with( process.update_vpnservice.assert_called_once_with(
updated_vpn_service) updated_vpn_service)
@ -710,7 +732,10 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver):
self.driver.agent_rpc.get_vpn_services_on_host.return_value = [] self.driver.agent_rpc.get_vpn_services_on_host.return_value = []
context = mock.Mock() context = mock.Mock()
process_id = _uuid() 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) self.assertNotIn(process_id, self.driver.processes)
def test_status_updated_on_connection_admin_down(self): 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): def _test_status_handling_for_downed_connection(self, down_status):
"""Test status handling for downed connection.""" """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 connection_id = FAKE_IPSEC_SITE_CONNECTION2_ID
self.driver.ensure_process(router_id, self.vpnservice) self.driver.ensure_process(router_id, self.vpnservice)
self._execute.return_value = down_status self._execute.return_value = down_status
@ -794,7 +819,7 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver):
def _test_status_handling_for_active_connection(self, active_status): def _test_status_handling_for_active_connection(self, active_status):
"""Test status handling for active connection.""" """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 connection_id = FAKE_IPSEC_SITE_CONNECTION2_ID
self.driver.ensure_process(router_id, self.vpnservice) self.driver.ensure_process(router_id, self.vpnservice)
self._execute.return_value = active_status self._execute.return_value = active_status
@ -809,7 +834,7 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver):
def _test_status_handling_for_ike_v2_active_connection(self, def _test_status_handling_for_ike_v2_active_connection(self,
active_status): active_status):
"""Test status handling for active connection.""" """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 connection_id = FAKE_IPSEC_SITE_CONNECTION2_ID
ike_policy = {'ike_version': 'v2', ike_policy = {'ike_version': 'v2',
'encryption_algorithm': 'aes-128', 'encryption_algorithm': 'aes-128',
@ -832,7 +857,7 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver):
def _test_connection_names_handling_for_multiple_subnets(self, def _test_connection_names_handling_for_multiple_subnets(self,
active_status): active_status):
"""Test connection names handling for multiple subnets.""" """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) process = self.driver.ensure_process(router_id, self.vpnservice)
self._execute.return_value = active_status self._execute.return_value = active_status
names = process.get_established_connections() names = process.get_established_connections()
@ -841,7 +866,7 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver):
def _test_status_handling_for_deleted_connection(self, def _test_status_handling_for_deleted_connection(self,
not_running_status): not_running_status):
"""Test status handling for deleted connection.""" """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.driver.ensure_process(router_id, self.vpnservice)
self._execute.return_value = not_running_status self._execute.return_value = not_running_status
self.driver.report_status(mock.Mock()) self.driver.report_status(mock.Mock())
@ -853,7 +878,7 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver):
def _test_parse_connection_status(self, not_running_status, def _test_parse_connection_status(self, not_running_status,
active_status, down_status): active_status, down_status):
"""Test the status of ipsec-site-connection is parsed correctly.""" """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) process = self.driver.ensure_process(router_id, self.vpnservice)
self._execute.return_value = not_running_status self._execute.return_value = not_running_status
self.assertFalse(process.active) self.assertFalse(process.active)
@ -873,41 +898,6 @@ class IPSecDeviceLegacy(BaseIPsecDeviceDriver):
def test_fail_getting_namespace_for_unknown_router(self): def test_fail_getting_namespace_for_unknown_router(self):
self.assertFalse(self.driver.get_namespace('bogus_id')) 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): class IPSecDeviceDVR(BaseIPsecDeviceDriver):
@ -918,21 +908,23 @@ class IPSecDeviceDVR(BaseIPsecDeviceDriver):
self._make_dvr_edge_router_info_for_test() self._make_dvr_edge_router_info_for_test()
def _make_dvr_edge_router_info_for_test(self): 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, mock.sentinel.myhost,
FAKE_ROUTER_ID, FAKE_ROUTER_ID,
**self.ri_kwargs) **self.ri_kwargs)
router.router['distributed'] = True router_info.router['distributed'] = True
router.snat_namespace = dvr_snat_ns.SnatNamespace(router.router['id'], router_info.snat_namespace = dvr_snat_ns.SnatNamespace(
mock.sentinel.agent, router_info.router['id'],
self.driver, mock.sentinel.agent,
mock.ANY) self.driver,
router.snat_namespace.create() mock.ANY
router.snat_iptables_manager = iptables_manager.IptablesManager( )
router_info.snat_namespace.create()
router_info.snat_iptables_manager = iptables_manager.IptablesManager(
namespace='snat-' + FAKE_ROUTER_ID, use_ipv6=mock.ANY) namespace='snat-' + FAKE_ROUTER_ID, use_ipv6=mock.ANY)
router.snat_iptables_manager.ipv4['nat'] = self.iptables router_info.snat_iptables_manager.ipv4['nat'] = self.iptables
router.snat_iptables_manager.apply = self.apply_mock router_info.snat_iptables_manager.apply = self.apply_mock
self.driver.routers[FAKE_ROUTER_ID] = router self.driver.routers[FAKE_ROUTER_ID] = router_info
def test_sync_dvr(self): def test_sync_dvr(self):
fake_vpn_service = FAKE_VPN_SERVICE fake_vpn_service = FAKE_VPN_SERVICE
@ -942,11 +934,10 @@ class IPSecDeviceDVR(BaseIPsecDeviceDriver):
self.driver._sync_vpn_processes = mock.Mock() self.driver._sync_vpn_processes = mock.Mock()
self.driver._delete_vpn_processes = mock.Mock() self.driver._delete_vpn_processes = mock.Mock()
self.driver._cleanup_stale_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']] sync_router_ids = [fake_vpn_service['router_id']]
with mock.patch.object(self.driver, with mock.patch.object(self.driver,
'get_process_status_cache') as process_status: '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( self.driver._sync_vpn_processes.assert_called_once_with(
[fake_vpn_service], sync_router_ids) [fake_vpn_service], sync_router_ids)
self.driver._delete_vpn_processes.assert_called_once_with( self.driver._delete_vpn_processes.assert_called_once_with(
@ -959,22 +950,10 @@ class IPSecDeviceDVR(BaseIPsecDeviceDriver):
namespace = self.driver.get_namespace(FAKE_ROUTER_ID) namespace = self.driver.get_namespace(FAKE_ROUTER_ID)
self.assertEqual('snat-' + FAKE_ROUTER_ID, namespace) self.assertEqual('snat-' + FAKE_ROUTER_ID, namespace)
def test_add_nat_rule_with_dvr_edge_router(self): def test_ensure_nat_rules_with_dvr_edge_router(self):
self.driver.add_nat_rule(FAKE_ROUTER_ID, 'fake_chain', self.driver.ensure_nat_rules(FAKE_VPN_SERVICE)
'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)
self.apply_mock.assert_called_once_with() 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): class TestOpenSwanConfigGeneration(BaseIPsecDeviceDriver):
@ -1293,7 +1272,7 @@ class TestOpenSwanProcess(IPSecDeviceLegacy):
'updated_pending_status': True}}) 'updated_pending_status': True}})
self.assertRaises(vpn_exception.VPNPeerAddressNotResolved, self.assertRaises(vpn_exception.VPNPeerAddressNotResolved,
self.process._get_nexthop, 'foo.peer.addr', self.process._get_nexthop, 'foo.peer.addr.',
'fake-conn-id') 'fake-conn-id')
self.assertEqual(expected_connection_status_dict, self.assertEqual(expected_connection_status_dict,
self.process.connection_status) self.process.connection_status)
@ -1303,7 +1282,7 @@ class TestOpenSwanProcess(IPSecDeviceLegacy):
'updated_pending_status': False}}) 'updated_pending_status': False}})
self.assertRaises(vpn_exception.VPNPeerAddressNotResolved, self.assertRaises(vpn_exception.VPNPeerAddressNotResolved,
self.process._get_nexthop, 'foo.peer.addr', self.process._get_nexthop, 'foo.peer.addr.',
'fake-conn-id') 'fake-conn-id')
self.assertEqual(expected_connection_status_dict, self.assertEqual(expected_connection_status_dict,
self.process.connection_status) self.process.connection_status)

14
neutron_vpnaas/tests/unit/services/vpn/device_drivers/test_ovn_ipsec.py
View File

@ -197,7 +197,7 @@ class TestOvnStrongSwanDriver(test_ipsec.IPSecDeviceLegacy):
'transit_gateway_ip': '192.168.1.1', 'transit_gateway_ip': '192.168.1.1',
} }
def test_iptables_apply(self): def test_ensure_nat_rules(self):
"""Not applicable for OvnIPsecDriver""" """Not applicable for OvnIPsecDriver"""
pass pass
@ -218,19 +218,11 @@ class TestOvnStrongSwanDriver(test_ipsec.IPSecDeviceLegacy):
"""Not applicable for OvnIPsecDriver""" """Not applicable for OvnIPsecDriver"""
pass pass
def test_remove_rule(self): def test_ensure_nat_rules_with_multiple_local_subnets(self):
"""Not applicable for OvnIPsecDriver""" """Not applicable for OvnIPsecDriver"""
pass pass
def test_add_nat_rules_with_multiple_local_subnets(self): def _test_ensure_nat_rules(self):
"""Not applicable for OvnIPsecDriver"""
pass
def _test_add_nat_rule(self):
"""Not applicable for OvnIPsecDriver"""
pass
def test_add_nat_rule(self):
"""Not applicable for OvnIPsecDriver""" """Not applicable for OvnIPsecDriver"""
pass pass

11
releasenotes/notes/bug1943449-899ba4711ff3586e.yaml Normal file
View File

@ -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.