openstack
/
neutron
Code Issues Proposed changes

Check subnet overlapping after add router interface 

When simultaneous attempts are made to add an interface
to the same router including overlapping networks in cidrs,
both attempts are successful. There is a check to avoid this
overlap but is performed when creating the network interface
and it is done over the ports already attached to the router,
so at this moment the check is not able to detect the
overlapping. Furthermore, the create_port operation over the
ML2 plugin  must be executed in isolated transactions, so
trying to control the execution context or adding additional
steps to the transaction is not feasible.

This patch checks once the RouterPort is created on the
neutron database if there is more than one overlapping port,
triggering in that case the exception that will remove the
the culprit of overlapping.

Note: Added clean of l3_obj.RouterPort to avoid errors on
setUp of test class L3TestCase, pick from [1]. Also added
cfg allow_overlapping_ips to True for fullstack job, this one
had been deprecated and enabled by default in newer releases
but added in order to run the backported tests over this
stable branch.

Conflicts:
       neutron/db/l3_db.py
       neutron/tests/unit/db/test_l3_db.py
(manually cherry picked from commit 1abb77d7a6)

[1] https://review.opendev.org/c/openstack/neutron/+/804846/18/neutron/tests/unit/db/test_l3_db.py#612

Closes-Bug: #1987666
Change-Id: I7cec8b53e72e7abf34012906e6adfecf079525af
(cherry picked from commit 1abb77d7a6)
This commit is contained in:
Fernando Royo 2022-09-23 20:53:09 +02:00
parent 93e7091a13
commit 98abcb6eac
4 changed files with 255 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

108
neutron/db/l3_db.py
View File

@ -13,6 +13,7 @@
# under the License.
import functools
import itertools
import random
import netaddr
@ -504,6 +505,23 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
ip_address_change = not ip_addresses == new_ip_addresses
return ip_address_change
def _raise_on_subnets_overlap(self, subnet_1, subnet_2):
cidr = subnet_1['cidr']
ipnet = netaddr.IPNetwork(cidr)
new_cidr = subnet_2['cidr']
new_ipnet = netaddr.IPNetwork(new_cidr)
match1 = netaddr.all_matching_cidrs(new_ipnet, [cidr])
match2 = netaddr.all_matching_cidrs(ipnet, [new_cidr])
if match1 or match2:
data = {'subnet_cidr': new_cidr,
'subnet_id': subnet_2['id'],
'cidr': cidr,
'sub_id': subnet_1['id']}
msg = (_("Cidr %(subnet_cidr)s of subnet "
"%(subnet_id)s overlaps with cidr %(cidr)s "
"of subnet %(sub_id)s") % data)
raise n_exc.BadRequest(resource='router', msg=msg)
def _ensure_router_not_in_use(self, context, router_id):
"""Ensure that no internal network interface is attached
to the router.
@ -604,22 +622,8 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
subnets = self._core_plugin.get_subnets(
utils.get_elevated_context(context), filters=id_filter)
for sub in subnets:
cidr = sub['cidr']
ipnet = netaddr.IPNetwork(cidr)
for s in new_subnets:
new_cidr = s['cidr']
new_ipnet = netaddr.IPNetwork(new_cidr)
match1 = netaddr.all_matching_cidrs(new_ipnet, [cidr])
match2 = netaddr.all_matching_cidrs(ipnet, [new_cidr])
if match1 or match2:
data = {'subnet_cidr': new_cidr,
'subnet_id': s['id'],
'cidr': cidr,
'sub_id': sub['id']}
msg = (_("Cidr %(subnet_cidr)s of subnet "
"%(subnet_id)s overlaps with cidr %(cidr)s "
"of subnet %(sub_id)s") % data)
raise n_exc.BadRequest(resource='router', msg=msg)
self._raise_on_subnets_overlap(sub, s)
def _get_device_owner(self, context, router=None):
"""Get device_owner for the specified router."""
@ -700,17 +704,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
port = self._check_router_port(context, port_id, router.id)
# Only allow one router port with IPv6 subnets per network id
if self._port_has_ipv6_address(port):
for existing_port in (rp.port for rp in router.attached_ports):
if (existing_port['network_id'] == port['network_id'] and
self._port_has_ipv6_address(existing_port)):
msg = _("Cannot have multiple router ports with the "
"same network id if both contain IPv6 "
"subnets. Existing port %(p)s has IPv6 "
"subnet(s) and network id %(nid)s")
raise n_exc.BadRequest(resource='router', msg=msg % {
'p': existing_port['id'],
'nid': existing_port['network_id']})
self._validate_one_router_ipv6_port_per_network(router, port)
fixed_ips = list(port['fixed_ips'])
subnets = []
@ -776,6 +770,20 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
if netaddr.IPNetwork(fixed_ip['ip_address']).version == 6:
return True
def _validate_one_router_ipv6_port_per_network(self, router, port):
if self._port_has_ipv6_address(port):
for existing_port in (rp.port for rp in router.attached_ports):
if (existing_port["id"] != port["id"] and
existing_port["network_id"] == port["network_id"] and
self._port_has_ipv6_address(existing_port)):
msg = _("Router already contains IPv6 port %(p)s "
"belonging to network id %(nid)s. Only one IPv6 port "
"from the same network subnet can be connected to a "
"router.")
raise n_exc.BadRequest(resource='router', msg=msg % {
'p': existing_port['id'],
'nid': existing_port['network_id']})
def _find_ipv6_router_port_by_network(self, context, router, net_id):
router_dev_owner = self._get_device_owner(context, router)
for port in router.attached_ports:
@ -879,7 +887,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
port=port,
interface_info=interface_info)
self._add_router_port(
context, port['id'], router.id, device_owner)
context, port['id'], router, device_owner)
gw_ips = []
gw_network_id = None
@ -906,18 +914,58 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
subnets[-1]['id'], [subnet['id'] for subnet in subnets])
@db_api.retry_if_session_inactive()
def _add_router_port(self, context, port_id, router_id, device_owner):
def _add_router_port(self, context, port_id, router, device_owner):
l3_obj.RouterPort(
context,
port_id=port_id,
router_id=router_id,
router_id=router.id,
port_type=device_owner
).create()
# NOTE(froyo): Check after create the RouterPort if we have generated
# an overlapping. Like creation of port is an ML2 plugin command it
# runs in an isolated transaction so we could not control there the
# addition of ports to different subnets that collides in cidrs. So
# make the check here an trigger the overlaping that will remove all
# created items.
router_ports = l3_obj.RouterPort.get_objects(
context, router_id=router.id)
if len(router_ports) > 1:
subnets_id = []
for rp in router_ports:
port = port_obj.Port.get_object(context.elevated(),
id=rp.port_id)
if port:
# Only allow one router port with IPv6 subnets per network
# id
self._validate_one_router_ipv6_port_per_network(
router, port)
subnets_id.extend([fixed_ip['subnet_id']
for fixed_ip in port['fixed_ips']])
else:
raise l3_exc.RouterInterfaceNotFound(
router_id=router.id, port_id=rp.port_id)
if subnets_id:
id_filter = {'id': subnets_id}
subnets = self._core_plugin.get_subnets(context.elevated(),
filters=id_filter)
# Ignore temporary Prefix Delegation CIDRs
subnets = [
s
for s in subnets
if s["cidr"] != constants.PROVISIONAL_IPV6_PD_PREFIX
]
for sub1, sub2 in itertools.combinations(subnets, 2):
self._raise_on_subnets_overlap(sub1, sub2)
# Update owner after actual process again in order to
# make sure the records in routerports table and ports
# table are consistent.
self._core_plugin.update_port(
context, port_id, {'port': {'device_id': router_id,
context, port_id, {'port': {'device_id': router.id,
'device_owner': device_owner}})
def _check_router_interface_not_in_use(self, router_id, subnet):

4
neutron/tests/fullstack/resources/config.py
View File

@ -83,6 +83,10 @@ class NeutronConfigFixture(ConfigFixture):
'host': rabbitmq_environment.host,
'vhost': rabbitmq_environment.vhost},
'api_workers': str(env_desc.api_workers),
# deprecated and enabled by default in future releases,
# added here in order to run job backported tests backported
# with this feature enabled
'allow_overlapping_ips': 'True',
},
'database': {
'connection': connection,

31
neutron/tests/fullstack/test_l3_agent.py
View File

@ -18,6 +18,7 @@ import time
import netaddr
from neutron_lib import constants
from neutronclient.common import exceptions
from oslo_utils import uuidutils
from neutron.agent.l3 import ha_router
@ -265,6 +266,29 @@ class TestL3Agent(base.BaseFullStackTestCase):
return (_is_filter_set(constants.INGRESS_DIRECTION) and
_is_filter_set(constants.EGRESS_DIRECTION))
def _test_concurrent_router_subnet_attachment_overlapping_cidr(self,
ha=False):
tenant_id = uuidutils.generate_uuid()
subnet_cidr = '10.100.0.0/24'
network1 = self.safe_client.create_network(
tenant_id, name='foo-network1')
subnet1 = self.safe_client.create_subnet(
tenant_id, network1['id'], subnet_cidr)
network2 = self.safe_client.create_network(
tenant_id, name='foo-network2')
subnet2 = self.safe_client.create_subnet(
tenant_id, network2['id'], subnet_cidr)
router = self.safe_client.create_router(tenant_id, ha=ha)
funcs = [self.safe_client.add_router_interface,
self.safe_client.add_router_interface]
args = [(router['id'], subnet1['id']), (router['id'], subnet2['id'])]
self.assertRaises(
exceptions.BadRequest,
self._simulate_concurrent_requests_process_and_raise,
funcs,
args)
class TestLegacyL3Agent(TestL3Agent):
@ -410,6 +434,9 @@ class TestLegacyL3Agent(TestL3Agent):
def test_router_fip_qos_after_admin_state_down_up(self):
self._router_fip_qos_after_admin_state_down_up()
def test_concurrent_router_subnet_attachment_overlapping_cidr_(self):
self._test_concurrent_router_subnet_attachment_overlapping_cidr()
class TestHAL3Agent(TestL3Agent):
@ -560,3 +587,7 @@ class TestHAL3Agent(TestL3Agent):
def test_router_fip_qos_after_admin_state_down_up(self):
self._router_fip_qos_after_admin_state_down_up(ha=True)
def test_concurrent_router_subnet_attachment_overlapping_cidr_(self):
self._test_concurrent_router_subnet_attachment_overlapping_cidr(
ha=True)

142
neutron/tests/unit/db/test_l3_db.py
View File

@ -31,6 +31,7 @@ import testtools
from neutron.db import l3_db
from neutron.db.models import l3 as l3_models
from neutron.db import models_v2
from neutron.objects import base as base_obj
from neutron.objects import network as network_obj
from neutron.objects import ports as port_obj
@ -341,6 +342,93 @@ class TestL3_NAT_dbonly_mixin(
cb_payload.metadata.get('network_id'))
self.assertEqual(router_id, cb_payload.resource_id)
def test__raise_on_subnets_overlap_does_not_raise(self):
subnets = [
{'id': uuidutils.generate_uuid(),
'cidr': '10.1.0.0/24'},
{'id': uuidutils.generate_uuid(),
'cidr': '10.2.0.0/24'}]
self.db._raise_on_subnets_overlap(subnets[0], subnets[1])
def test__raise_on_subnets_overlap_raises(self):
subnets = [
{'id': uuidutils.generate_uuid(),
'cidr': '10.1.0.0/20'},
{'id': uuidutils.generate_uuid(),
'cidr': '10.1.10.0/24'}]
self.assertRaises(
n_exc.BadRequest, self.db._raise_on_subnets_overlap, subnets[0],
subnets[1])
def test__validate_one_router_ipv6_port_per_network(self):
port = models_v2.Port(
id=uuidutils.generate_uuid(),
network_id='foo_network',
fixed_ips=[models_v2.IPAllocation(
ip_address=str(netaddr.IPNetwork(
'2001:db8::/32').ip + 1),
subnet_id='foo_subnet')])
rports = [l3_models.RouterPort(router_id='foo_router', port=port)]
router = l3_models.Router(
id='foo_router', attached_ports=rports, route_list=[],
gw_port_id=None)
new_port = models_v2.Port(
id=uuidutils.generate_uuid(),
network_id='foo_network2',
fixed_ips=[models_v2.IPAllocation(
ip_address=str(netaddr.IPNetwork(
'2001:db8::/32').ip + 2),
subnet_id='foo_subnet')])
self.db._validate_one_router_ipv6_port_per_network(
router, new_port)
def test__validate_one_router_ipv6_port_per_network_mix_ipv4_ipv6(self):
port = models_v2.Port(
id=uuidutils.generate_uuid(),
network_id='foo_network',
fixed_ips=[models_v2.IPAllocation(
ip_address=str(netaddr.IPNetwork(
'10.1.10.0/24').ip + 1),
subnet_id='foo_subnet')])
rports = [l3_models.RouterPort(router_id='foo_router', port=port)]
router = l3_models.Router(
id='foo_router', attached_ports=rports, route_list=[],
gw_port_id=None)
new_port = models_v2.Port(
id=uuidutils.generate_uuid(),
network_id='foo_network',
fixed_ips=[models_v2.IPAllocation(
ip_address=str(netaddr.IPNetwork(
'2001:db8::/32').ip + 2),
subnet_id='foo_subnet')])
self.db._validate_one_router_ipv6_port_per_network(
router, new_port)
def test__validate_one_router_ipv6_port_per_network_failed(self):
port = models_v2.Port(
id=uuidutils.generate_uuid(),
network_id='foo_network',
fixed_ips=[models_v2.IPAllocation(
ip_address=str(netaddr.IPNetwork(
'2001:db8::/32').ip + 1),
subnet_id='foo_subnet')])
rports = [l3_models.RouterPort(router_id='foo_router', port=port)]
router = l3_models.Router(
id='foo_router', attached_ports=rports, route_list=[],
gw_port_id=None)
new_port = models_v2.Port(
id=uuidutils.generate_uuid(),
network_id='foo_network',
fixed_ips=[models_v2.IPAllocation(
ip_address=str(netaddr.IPNetwork(
'2001:db8::/32').ip + 2),
subnet_id='foo_subnet')])
self.assertRaises(
n_exc.BadRequest,
self.db._validate_one_router_ipv6_port_per_network,
router,
new_port)
class L3_NAT_db_mixin(base.BaseTestCase):
def setUp(self):
@ -436,6 +524,10 @@ class L3TestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
self.ctx, network_id=self.network['network']['id'])
network_obj.Network.get_object(
self.ctx, id=self.network['network']['id']).delete()
router_ports = l3_obj.RouterPort.get_objects(
self.ctx, **{'router_id': self.router['id']})
for router_port in router_ports:
router_port.delete()
l3_obj.Router.get_object(self.ctx, id=self.router['id']).delete()
def create_router(self, router):
@ -520,6 +612,56 @@ class L3TestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
mock_log.warning.not_called_once()
self._check_routerports((True, False))
@mock.patch.object(l3_db.L3_NAT_dbonly_mixin,
'_check_for_dup_router_subnets')
@mock.patch.object(l3_db.L3_NAT_dbonly_mixin,
'_raise_on_subnets_overlap')
def test_add_router_interface_by_port_overlap_detected(
self, mock_raise_on_subnets_overlap, mock_check_dup):
# NOTE(froyo): On a normal behaviour this overlapping would be detected
# by _check_for_dup_router_subnets, in order to evalue the code
# implemented to cover the race condition when two ports are added
# simultaneously using colliding cidrs we need to "fake" this method
# to overpass it and check we achieve the code part that cover the case
mock_check_dup.return_value = True
network2 = self.create_network('network2')
subnet = self.create_subnet(network2, '1.1.1.1', '1.1.1.0/24')
ipa = str(netaddr.IPNetwork(subnet['subnet']['cidr']).ip + 10)
fixed_ips = [{'subnet_id': subnet['subnet']['id'], 'ip_address': ipa}]
port = self.create_port(
network2['network']['id'], {'fixed_ips': fixed_ips})
self.mixin.add_router_interface(
self.ctx, self.router['id'],
interface_info={'port_id': port['port']['id']})
mock_raise_on_subnets_overlap.assert_not_called()
self.mixin.add_router_interface(
self.ctx, self.router['id'],
interface_info={'port_id': self.ports[0]['port']['id']})
mock_raise_on_subnets_overlap.assert_called_once()
@mock.patch.object(l3_db.L3_NAT_dbonly_mixin,
'_check_for_dup_router_subnets')
@mock.patch.object(l3_db.L3_NAT_dbonly_mixin,
'_raise_on_subnets_overlap')
def test_add_router_interface_by_subnet_overlap_detected(
self, mock_raise_on_subnets_overlap, mock_check_dup):
# NOTE(froyo): On a normal behaviour this overlapping would be detected
# by _check_for_dup_router_subnets, in order to evalue the code
# implemented to cover the race condition when two ports are added
# simultaneously using colliding cidrs we need to "fake" this method
# to overpass it and check we achieve the code part that cover the case
mock_check_dup.return_value = True
network2 = self.create_network('network2')
subnet = self.create_subnet(network2, '1.1.1.1', '1.1.1.0/24')
self.mixin.add_router_interface(
self.ctx, self.router['id'],
interface_info={'subnet_id': subnet['subnet']['id']})
mock_raise_on_subnets_overlap.assert_not_called()
self.mixin.add_router_interface(
self.ctx, self.router['id'],
interface_info={'subnet_id': self.subnets[0]['subnet']['id']})
mock_raise_on_subnets_overlap.assert_called_once()
@mock.patch.object(port_obj, 'LOG')
def test_remove_router_interface_by_subnet_removed_rport(self, mock_log):
self._add_router_interfaces()