diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index 59cdf18108..afc01a33bb 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -871,7 +871,8 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase): def _build_routers_list(self, context, routers, gw_ports): for router in routers: gw_port_id = router['gw_port_id'] - if gw_port_id: + # Collect gw ports only if available + if gw_port_id and gw_ports.get(gw_port_id): router['gw_port'] = gw_ports[gw_port_id] return routers @@ -904,6 +905,11 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase): gw_ports = dict((gw_port['id'], gw_port) for gw_port in self.get_sync_gw_ports(context, gw_port_ids)) + # NOTE(armando-migliaccio): between get_routers and get_sync_gw_ports + # gw ports may get deleted, which means that router_dicts may contain + # ports that gw_ports does not; we should rebuild router_dicts, but + # letting the callee check for missing gw_ports sounds like a good + # defensive approach regardless return self._build_routers_list(context, router_dicts, gw_ports) def _get_sync_floating_ips(self, context, router_ids): diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index c272dd2a77..b73ad85182 100644 --- a/neutron/db/l3_dvr_db.py +++ b/neutron/db/l3_dvr_db.py @@ -237,7 +237,8 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin, for rtr in routers: gw_port_id = rtr['gw_port_id'] - if gw_port_id: + # Collect gw ports only if available + if gw_port_id and gw_ports.get(gw_port_id): rtr['gw_port'] = gw_ports[gw_port_id] if 'enable_snat' in rtr[l3.EXTERNAL_GW_INFO]: rtr['enable_snat'] = ( diff --git a/neutron/db/l3_gwmode_db.py b/neutron/db/l3_gwmode_db.py index d0ec612fbd..dce6cafe90 100644 --- a/neutron/db/l3_gwmode_db.py +++ b/neutron/db/l3_gwmode_db.py @@ -65,7 +65,8 @@ class L3_NAT_dbonly_mixin(l3_db.L3_NAT_dbonly_mixin): def _build_routers_list(self, context, routers, gw_ports): for rtr in routers: gw_port_id = rtr['gw_port_id'] - if gw_port_id: + # Collect gw ports only if available + if gw_port_id and gw_ports.get(gw_port_id): rtr['gw_port'] = gw_ports[gw_port_id] # Add enable_snat key rtr['enable_snat'] = rtr[EXTERNAL_GW_INFO]['enable_snat'] diff --git a/neutron/tests/unit/db/test_l3_dvr_db.py b/neutron/tests/unit/db/test_l3_dvr_db.py index c0329ee6fa..5cec5587b2 100644 --- a/neutron/tests/unit/db/test_l3_dvr_db.py +++ b/neutron/tests/unit/db/test_l3_dvr_db.py @@ -167,3 +167,9 @@ class L3DvrTestCase(base.BaseTestCase): self.mixin._create_gw_port( self.ctx, router_id, router_db, mock.ANY) self.assertFalse(cs.call_count) + + def test_build_routers_list_with_gw_port_mismatch(self): + routers = [{'gw_port_id': 'foo_gw_port_id', 'id': 'foo_router_id'}] + gw_ports = {} + routers = self.mixin._build_routers_list(self.ctx, routers, gw_ports) + self.assertIsNone(routers[0].get('gw_port')) diff --git a/neutron/tests/unit/test_extension_ext_gw_mode.py b/neutron/tests/unit/test_extension_ext_gw_mode.py index 48c5aec635..a9e97066ab 100644 --- a/neutron/tests/unit/test_extension_ext_gw_mode.py +++ b/neutron/tests/unit/test_extension_ext_gw_mode.py @@ -288,6 +288,15 @@ class TestL3GwModeMixin(base.BaseTestCase): self.assertEqual(FAKE_GW_PORT_ID, router['gw_port']['id']) self.assertFalse(router.get('enable_snat')) + def test_build_routers_list_with_gw_port_mismatch(self): + router_dict = self.target_object._make_router_dict(self.router) + routers = self.target_object._build_routers_list( + self.context, [router_dict], {}) + self.assertEqual(1, len(routers)) + router = routers[0] + self.assertIsNone(router.get('gw_port')) + self.assertIsNone(router.get('enable_snat')) + class ExtGwModeIntTestCase(test_db_plugin.NeutronDbPluginV2TestCase, test_l3_plugin.L3NatTestCaseMixin): diff --git a/neutron/tests/unit/test_l3_plugin.py b/neutron/tests/unit/test_l3_plugin.py index 50f7ba7dc8..f1bc02b7e7 100644 --- a/neutron/tests/unit/test_l3_plugin.py +++ b/neutron/tests/unit/test_l3_plugin.py @@ -313,6 +313,19 @@ class TestL3NatAgentSchedulingServicePlugin(TestL3NatServicePlugin, supported_extension_aliases = ["router", "l3_agent_scheduler"] +class L3NATdbonlyMixinTestCase(base.BaseTestCase): + + def setUp(self): + super(L3NATdbonlyMixinTestCase, self).setUp() + self.mixin = l3_db.L3_NAT_dbonly_mixin() + + def test_build_routers_list_with_gw_port_mismatch(self): + routers = [{'gw_port_id': 'foo_gw_port_id', 'id': 'foo_router_id'}] + gw_ports = {} + routers = self.mixin._build_routers_list(mock.ANY, routers, gw_ports) + self.assertIsNone(routers[0].get('gw_port')) + + class L3NatTestCaseMixin(object): def _create_router(self, fmt, tenant_id, name=None,