Merge "Check subnet overlapping after add router interface"
This commit is contained in:
commit
ba795c6692
|
@ -13,6 +13,7 @@
|
|||
# under the License.
|
||||
|
||||
import functools
|
||||
import itertools
|
||||
import random
|
||||
|
||||
import netaddr
|
||||
|
@ -564,6 +565,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.
|
||||
|
@ -672,22 +690,8 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
|
|||
subnets = self._core_plugin.get_subnets(context.elevated(),
|
||||
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."""
|
||||
|
@ -768,17 +772,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 = []
|
||||
|
@ -844,6 +838,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:
|
||||
|
@ -962,7 +970,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
|
||||
|
@ -990,18 +998,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):
|
||||
|
|
|
@ -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
|
||||
|
@ -266,6 +267,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):
|
||||
|
||||
|
@ -417,6 +441,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):
|
||||
|
||||
|
@ -573,3 +600,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)
|
||||
|
|
|
@ -514,6 +514,93 @@ class TestL3_NAT_dbonly_mixin(
|
|||
self.assertIsNone(
|
||||
self.db._validate_gw_info(mock.ANY, info, [], router))
|
||||
|
||||
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):
|
||||
|
@ -697,6 +784,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()
|
||||
|
|
Loading…
Reference in New Issue