openstack/neutron
Code Issues Proposed changes

[DVR] Set arp entries only for IPs from the correct subnet

Browse Source 
When dvr router is processing internal ports it is checking all
ports connected to the subnet and adding permanent arp entries for
all fixed IPs and allowed address pairs from those ports in the qrouter
namespace.
But port can have fixed IPs from different subnets, e.g. from IPv4 and
IPv6 subnet and until now Neutron wasn't checking subnet_id of the
fixed_ip address nor ip version of the allowed address pair's IP
address. That resulted in adding arp entries for all IPs through all
interfaces, e.g. IPv4 address was added as it's reachable through
interface connected to the IPv6 subnet.

This patch adds checking of the subnet_id for fixed_ips and ip version
for the allowed address pairs configured on the port to avoid that
problem.

Closes-Bug: #1936980
Change-Id: Id5afad7af74d69f8b4159163d23807a1cf032733
This commit is contained in:
Slawek Kaplonski
2021-07-23 15:56:29 +02:00
parent 4b62244daa
commit 771fdc0b07
5 changed files with 43 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
neutron/agent/l3/dvr_edge_ha_router.py
View File

@@ -42,7 +42,7 @@ class DvrEdgeHaRouter(dvr_edge_router.DvrEdgeRouter,
router_info.RouterInfo.internal_network_added(self, port)
for subnet in port['subnets']:
self._set_subnet_arp_info(subnet['id'])
self._set_subnet_arp_info(subnet)
self._snat_redirect_add_from_port(port)
if not self.get_ex_gw_port() or not self._is_this_snat_host():

44
neutron/agent/l3/dvr_local_router.py
View File

@@ -346,52 +346,56 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase):
device_exists = device.exists()
return device, device_exists
def _set_subnet_arp_info(self, subnet_id):
def _set_subnet_arp_info(self, subnet):
"""Set ARP info retrieved from Plugin for existing ports."""
# TODO(Carl) Can we eliminate the need to make this RPC while
# processing a router.
subnet_ports = self.agent.get_ports_by_subnet(subnet_id)
subnet_ports = self.agent.get_ports_by_subnet(subnet['id'])
ignored_device_owners = (
lib_constants.ROUTER_INTERFACE_OWNERS +
tuple(common_utils.get_dvr_allowed_address_pair_device_owners()))
device, device_exists = self.get_arp_related_dev(subnet_id)
device, device_exists = self.get_arp_related_dev(subnet['id'])
subnet_ip_version = netaddr.IPNetwork(subnet['cidr']).version
for p in subnet_ports:
if p['device_owner'] not in ignored_device_owners:
for fixed_ip in p['fixed_ips']:
self._update_arp_entry(fixed_ip['ip_address'],
p['mac_address'],
subnet_id,
'add',
device=device,
device_exists=device_exists)
if fixed_ip['subnet_id'] == subnet['id']:
self._update_arp_entry(fixed_ip['ip_address'],
p['mac_address'],
subnet['id'],
'add',
device=device,
device_exists=device_exists)
for allowed_address_pair in p.get('allowed_address_pairs', []):
if ('/' not in str(allowed_address_pair['ip_address']) or
common_utils.is_cidr_host(
allowed_address_pair['ip_address'])):
ip_address = common_utils.cidr_to_ip(
allowed_address_pair['ip_address'])
self._update_arp_entry(
ip_address,
allowed_address_pair['mac_address'],
subnet_id,
'add',
device=device,
device_exists=device_exists)
ip_version = common_utils.get_ip_version(ip_address)
if ip_version == subnet_ip_version:
self._update_arp_entry(
ip_address,
allowed_address_pair['mac_address'],
subnet['id'],
'add',
device=device,
device_exists=device_exists)
# subnet_ports does not have snat port if the port is still unbound
# by the time this function is called. So ensure to add arp entry
# for snat port if port details are updated in router info.
for p in self.get_snat_interfaces():
for fixed_ip in p['fixed_ips']:
if fixed_ip['subnet_id'] == subnet_id:
if fixed_ip['subnet_id'] == subnet['id']:
self._update_arp_entry(fixed_ip['ip_address'],
p['mac_address'],
subnet_id,
subnet['id'],
'add',
device=device,
device_exists=device_exists)
self._process_arp_cache_for_internal_port(subnet_id)
self._process_arp_cache_for_internal_port(subnet['id'])
@staticmethod
def _get_snat_idx(ip_cidr):
@@ -508,7 +512,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase):
# external_gateway port or the agent_mode.
ex_gw_port = self.get_ex_gw_port()
for subnet in port['subnets']:
self._set_subnet_arp_info(subnet['id'])
self._set_subnet_arp_info(subnet)
if ex_gw_port:
# Check for address_scopes here if gateway exists.
address_scopes_match = self._check_if_address_scopes_match(

5
neutron/tests/functional/agent/l3/test_dvr_router.py
View File

@@ -1007,8 +1007,11 @@ class TestDvrRouter(DvrRouterTestFramework, framework.L3AgentTestFramework):
router_info = self.generate_dvr_router_info(enable_snat=True)
expected_neighbors = ['35.4.1.10', '10.0.0.10', '10.200.0.3']
allowed_address_net = netaddr.IPNetwork('10.100.0.0/30')
subnet_id = router_info['_interfaces'][0]['fixed_ips'][0]['subnet_id']
port_data = {
'fixed_ips': [{'ip_address': expected_neighbors[0]}],
'fixed_ips': [
{'ip_address': expected_neighbors[0],
'subnet_id': subnet_id}],
'mac_address': 'fa:3e:aa:bb:cc:dd',
'device_owner': DEVICE_OWNER_COMPUTE,
'allowed_address_pairs': [

2
neutron/tests/unit/agent/l3/test_agent.py
View File

@@ -681,7 +681,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
ri._add_interface_route_to_fip_ns = mock.Mock()
ri.internal_network_added(port)
self.assertEqual(2, ri._internal_network_added.call_count)
ri._set_subnet_arp_info.assert_called_once_with(subnet_id)
ri._set_subnet_arp_info.assert_called_once_with({'id': subnet_id})
ri._internal_network_added.assert_called_with(
dvr_snat_ns.SnatNamespace.get_snat_ns_name(ri.router['id']),
sn_port['network_id'],

17
neutron/tests/unit/agent/l3/test_dvr_local_router.py
View File

@@ -559,10 +559,19 @@ class TestDvrRouterOperations(base.BaseTestCase):
ri = dvr_router.DvrLocalRouter(HOSTNAME, **self.ri_kwargs)
ports = ri.router.get(lib_constants.INTERFACE_KEY, [])
subnet_id = l3_test_common.get_subnet_id(ports[0])
subnet = {
'id': subnet_id,
'cidr': '1.2.3.0/24'
}
ri.router['_snat_router_interfaces'] = [{
'mac_address': 'fa:16:3e:80:8d:80',
'fixed_ips': [{'subnet_id': subnet_id,
'ip_address': '1.2.3.10'}]}]
'fixed_ips': [
{'subnet_id': subnet_id,
'ip_address': '1.2.3.10'},
{'subnet_id': _uuid(),
'ip_address': '2001:db8::1'}
]
}]
test_ports = [{'mac_address': '00:11:22:33:44:55',
'device_owner': lib_constants.DEVICE_OWNER_DHCP,
@@ -591,7 +600,7 @@ class TestDvrRouterOperations(base.BaseTestCase):
'cidr': '1.2.3.0/24'}]
with mock.patch.object(ri,
'_process_arp_cache_for_internal_port') as parp:
ri._set_subnet_arp_info(subnet_id)
ri._set_subnet_arp_info(subnet)
self.assertEqual(1, parp.call_count)
self.mock_ip_dev.neigh.add.assert_has_calls([
mock.call('1.2.3.4', '00:11:22:33:44:55'),
@@ -600,7 +609,7 @@ class TestDvrRouterOperations(base.BaseTestCase):
# Test negative case
router['distributed'] = False
ri._set_subnet_arp_info(subnet_id)
ri._set_subnet_arp_info(subnet)
self.mock_ip_dev.neigh.add.never_called()
def test_add_arp_entry(self):