openstack/neutron
Code Issues Proposed changes

Check router routes connectivity when GW port is updated

Browse Source 
When the router GW interface is updated or deleted, the routes should
be checked in other to prove all of them have connectivity to any of
the router interface subnets.

In a router, all routes nexthops must have connectivity with one
interface subnet. If not, this route cannot send the packets to this
route gateway; therefore this route becomes invalid.

Closes-Bug: #1925368

Change-Id: I7ce93b863b0dc0d4a2376fcfd602d2facb6fb2d0
This commit is contained in:
Rodolfo Alonso Hernandez 2021-05-05 17:26:23 +00:00
parent e12ba06e8b
commit 5fb5653ffe
4 changed files with 163 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
neutron/db/extraroute_db.py
View File

@ -85,8 +85,18 @@ class ExtraRoute_dbonly_mixin(l3_db.L3_NAT_dbonly_mixin):
routes=routes,
reason=_('the nexthop is used by router'))
def _validate_routes(self, context,
router_id, routes):
def _validate_routes(self, context, router_id, routes, cidrs=None,
ip_addresses=None):
"""Validate a router routes with its interface subnets CIDRs and IPs
If any route cannot reach any subnet CIDR from any interface or the
route nethop match any interface IP address, this route is invalid.
:param context: Neutron request context
:param router_id: router ID
:param routes: router routes (list of dictionaries)
:param cidrs: (optional) list of CIDRs (strings)
:param ip_addresses: (optional) list of IP addresses (strings)
"""
if len(routes) > cfg.CONF.max_routes:
raise xroute_exc.RoutesExhausted(
router_id=router_id,
@ -94,17 +104,20 @@ class ExtraRoute_dbonly_mixin(l3_db.L3_NAT_dbonly_mixin):
context = context.elevated()
filters = {'device_id': [router_id]}
ports = self._core_plugin.get_ports(context, filters)
cidrs = []
ips = []
for port in ports:
for ip in port['fixed_ips']:
cidrs.append(self._core_plugin.get_subnet(
context, ip['subnet_id'])['cidr'])
ips.append(ip['ip_address'])
cidrs = cidrs or []
ip_addresses = ip_addresses or []
if not (cidrs or ip_addresses):
ports = self._core_plugin.get_ports(context, filters)
for port in ports:
for ip in port['fixed_ips']:
cidrs.append(self._core_plugin.get_subnet(
context, ip['subnet_id'])['cidr'])
ip_addresses.append(ip['ip_address'])
for route in routes:
self._validate_routes_nexthop(
cidrs, ips, routes, route['nexthop'])
cidrs, ip_addresses, routes, route['nexthop'])
def _update_extra_routes(self, context, router, routes):
self._validate_routes(context, router['id'], routes)

50
neutron/db/l3_db.py
View File

@ -341,24 +341,48 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
)
router_port.create()
def _validate_gw_info(self, context, gw_port, info, ext_ips):
def _validate_gw_info(self, context, info, ext_ips, router):
network_id = info['network_id'] if info else None
gw_subnets = []
if network_id:
network_db = self._core_plugin._get_network(context, network_id)
if not network_db.external:
msg = _("Network %s is not an external network") % network_id
raise n_exc.BadRequest(resource='router', msg=msg)
if ext_ips:
subnets = self._core_plugin.get_subnets_by_network(context,
network_id)
for s in subnets:
if not s['gateway_ip']:
gw_subnets = network_db.subnets
for ext_ip in ext_ips:
for subnet in network_db.subnets:
if not subnet.gateway_ip:
continue
for ext_ip in ext_ips:
if ext_ip.get('ip_address') == s['gateway_ip']:
msg = _("External IP %s is the same as the "
"gateway IP") % ext_ip.get('ip_address')
raise n_exc.BadRequest(resource='router', msg=msg)
if ext_ip.get('ip_address') == subnet.gateway_ip:
msg = _("External IP %s is the same as the "
"gateway IP") % ext_ip.get('ip_address')
raise n_exc.BadRequest(resource='router', msg=msg)
method_validate_routes = getattr(self, '_validate_routes', None)
if not method_validate_routes or not router.route_list:
return network_id
cidrs = []
ip_addresses = []
# All attached ports but the GW port, if exists, that will change
# because the GW information is being updated.
attached_ports = [rp.port for rp in router.attached_ports if
rp.port.id != router.gw_port_id]
for port in attached_ports:
for ip in port.fixed_ips:
cidrs.append(self._core_plugin.get_subnet(
context.elevated(), ip.subnet_id)['cidr'])
ip_addresses.append(ip.ip_address)
# NOTE(ralonsoh): the GW port is not updated yet and the its IP address
# won't be added to "ip_addresses", only the subnet CIDRs.
for gw_subnet in gw_subnets:
cidrs.append(gw_subnet.cidr)
method_validate_routes(context, router.id, router.route_list,
cidrs=cidrs, ip_addresses=ip_addresses)
return network_id
# NOTE(yamamoto): This method is an override point for plugins
@ -470,10 +494,10 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
def _update_router_gw_info(self, context, router_id, info, router=None):
router = router or self._get_router(context, router_id)
gw_port = router.gw_port
ext_ips = info.get('external_fixed_ips') if info else []
ext_ips = info.get('external_fixed_ips', []) if info else []
ext_ip_change = self._check_for_external_ip_change(
context, gw_port, ext_ips)
network_id = self._validate_gw_info(context, gw_port, info, ext_ips)
network_id = self._validate_gw_info(context, info, ext_ips, router)
if gw_port and ext_ip_change and gw_port['network_id'] == network_id:
self._update_current_gw_port(context, router_id, router,
ext_ips)

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

@ -24,14 +24,17 @@ from neutron_lib import constants as n_const
from neutron_lib import context
from neutron_lib.db import api as db_api
from neutron_lib import exceptions as n_exc
from neutron_lib.exceptions import extraroute as xroute_exc
from neutron_lib.plugins import constants as plugin_constants
from neutron_lib.plugins import directory
from neutron_lib.plugins import utils as plugin_utils
from oslo_utils import uuidutils
import testtools
from neutron.db import extraroute_db
from neutron.db import l3_db
from neutron.db.models import l3 as l3_models
from neutron.db import models_v2
from neutron.extensions import segment as segment_ext
from neutron.objects import base as base_obj
from neutron.objects import network as network_obj
@ -48,7 +51,12 @@ class TestL3_NAT_dbonly_mixin(
def setUp(self, *args, **kwargs):
super(TestL3_NAT_dbonly_mixin, self).setUp(*args, **kwargs)
self.db = l3_db.L3_NAT_dbonly_mixin()
# "extraroute_db.ExtraRoute_dbonly_mixin" inherits from
# "l3_db.L3_NAT_dbonly_mixin()", the class under test. This is used
# instead to test the validation of router routes and GW change because
# implements "_validate_routes".
self.db = extraroute_db.ExtraRoute_dbonly_mixin()
self.ctx = mock.Mock()
def test__each_port_having_fixed_ips_none(self):
"""Be sure the method returns an empty list when None is passed"""
@ -416,6 +424,93 @@ class TestL3_NAT_dbonly_mixin(
cb_payload.metadata.get('network_id'))
self.assertEqual(router_id, cb_payload.resource_id)
def _create_router(self, gw_port=True, num_ports=2, create_routes=True):
# GW CIDR: 10.0.0.0/24
# Interface CIDRS: 10.0.1.0/24, 10.0.2.0/24, etc.
router_id = uuidutils.generate_uuid()
port_gw_cidr = netaddr.IPNetwork('10.0.0.0/24')
rports = []
if gw_port:
port_gw = models_v2.Port(
id=uuidutils.generate_uuid(),
fixed_ips=[models_v2.IPAllocation(
ip_address=str(port_gw_cidr.ip + 1))])
rports.append(l3_models.RouterPort(router_id=router_id,
port=port_gw))
else:
port_gw = None
port_cidrs = []
port_subnets = []
for idx in range(num_ports):
cidr = port_gw_cidr.cidr.next(idx + 1)
port = models_v2.Port(
id=uuidutils.generate_uuid(),
fixed_ips=[models_v2.IPAllocation(
ip_address=str(cidr.ip + 1))])
port_cidrs.append(cidr)
rports.append(l3_models.RouterPort(router_id=router_id, port=port))
port_subnets.append({'cidr': str(cidr)})
routes = []
if create_routes:
for cidr in [*port_cidrs, port_gw_cidr]:
routes.append(l3_models.RouterRoute(
destination=str(cidr.next(100)),
nexthop=str(cidr.ip + 10)))
return (l3_models.Router(
id=router_id, attached_ports=rports, route_list=routes,
gw_port_id=port_gw.id if port_gw else None), port_subnets)
def test__validate_gw_info(self):
gw_network = mock.Mock(subnets=[mock.Mock(cidr='10.0.0.0/24')],
external=True)
router, port_subnets = self._create_router(gw_port=False)
info = {'network_id': 'net_id'}
with mock.patch.object(self.db._core_plugin, '_get_network',
return_value=gw_network), \
mock.patch.object(self.db._core_plugin, 'get_subnet',
side_effect=port_subnets):
self.assertEqual(
'net_id',
self.db._validate_gw_info(self.ctx, info, [], router))
def test__validate_gw_info_no_route_connectivity(self):
gw_network = mock.Mock(subnets=[mock.Mock(cidr='10.50.0.0/24')],
external=True)
router, port_subnets = self._create_router(gw_port=False)
info = {'network_id': 'net_id'}
with mock.patch.object(self.db._core_plugin, '_get_network',
return_value=gw_network), \
mock.patch.object(self.db._core_plugin, 'get_subnet',
side_effect=port_subnets):
self.assertRaises(
xroute_exc.InvalidRoutes, self.db._validate_gw_info, self.ctx,
info, [], router)
def test__validate_gw_info_delete_gateway(self):
router, port_subnets = self._create_router()
info = {'network_id': None}
with mock.patch.object(self.db._core_plugin, '_get_network',
return_value=None), \
mock.patch.object(self.db._core_plugin, 'get_subnet',
side_effect=port_subnets):
self.assertRaises(
xroute_exc.InvalidRoutes, self.db._validate_gw_info, self.ctx,
info, [], router)
def test__validate_gw_info_delete_gateway_no_route(self):
gw_network = mock.Mock(subnets=[mock.Mock(cidr='10.50.0.0/24')],
external=True)
router, port_subnets = self._create_router(create_routes=False)
info = {'network_id': None}
with mock.patch.object(self.db._core_plugin, '_get_network',
return_value=gw_network), \
mock.patch.object(self.db._core_plugin, 'get_subnet',
side_effect=port_subnets):
self.assertIsNone(
self.db._validate_gw_info(mock.ANY, info, [], router))
class L3_NAT_db_mixin(base.BaseTestCase):
def setUp(self):

6
releasenotes/notes/validate-router-gw-and-routes-af45c89c52612028.yaml Normal file
View File

@ -0,0 +1,6 @@
---
features:
- |
Reject any router route or gateway update if not all route nexthops have
connectivity with any gateway subnets CIDRs; in other words, all route
nexthops IP addresses should belong to one gateway subnet CIDR.