diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index c8dfb2137cd..5b10badc89e 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -517,17 +517,29 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase): raise n_exc.BadRequest(resource='router', msg=msg) return port_id_specified, subnet_id_specified - def _add_interface_by_port(self, context, router, port_id, owner): - with context.session.begin(subtransactions=True): - port = self._core_plugin._get_port(context, port_id) - if port['device_id']: - raise n_exc.PortInUse(net_id=port['network_id'], - port_id=port['id'], - device_id=port['device_id']) + def _check_router_port(self, context, port_id, device_id): + port = self._core_plugin.get_port(context, port_id) + if port['device_id'] != device_id: + raise n_exc.PortInUse(net_id=port['network_id'], + port_id=port['id'], + device_id=port['device_id']) + if not port['fixed_ips']: + msg = _('Router port must have at least one fixed IP') + raise n_exc.BadRequest(resource='router', msg=msg) + return port - if not port['fixed_ips']: - msg = _('Router port must have at least one fixed IP') - raise n_exc.BadRequest(resource='router', msg=msg) + def _add_interface_by_port(self, context, router, port_id, owner): + # Update owner before actual process in order to avoid the + # case where a port might get attached to a router without the + # owner successfully updating due to an unavailable backend. + self._check_router_port(context, port_id, '') + self._core_plugin.update_port( + context, port_id, {'port': {'device_id': router.id, + 'device_owner': owner}}) + + with context.session.begin(subtransactions=True): + # check again within transaction to mitigate race + 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): @@ -558,8 +570,6 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase): msg = _("Cannot have multiple " "IPv4 subnets on router port") raise n_exc.BadRequest(resource='router', msg=msg) - - port.update({'device_id': router.id, 'device_owner': owner}) return port, subnets def _port_has_ipv6_address(self, port): diff --git a/neutron/tests/unit/extensions/test_l3.py b/neutron/tests/unit/extensions/test_l3.py index 9d95a18f0bf..eda6a70d416 100644 --- a/neutron/tests/unit/extensions/test_l3.py +++ b/neutron/tests/unit/extensions/test_l3.py @@ -1175,24 +1175,31 @@ class L3NatTestCaseBase(L3NatTestCaseMixin): self.assertIn('port_id', body) def test_router_add_interface_port(self): - with self.router() as r: - with self.port() as p: - body = self._router_interface_action('add', - r['router']['id'], - None, - p['port']['id']) - self.assertIn('port_id', body) - self.assertEqual(body['port_id'], p['port']['id']) + orig_update_port = self.plugin.update_port + with self.router() as r, ( + self.port()) as p, ( + mock.patch.object(self.plugin, 'update_port')) as update_port: + update_port.side_effect = orig_update_port + body = self._router_interface_action('add', + r['router']['id'], + None, + p['port']['id']) + self.assertIn('port_id', body) + self.assertEqual(p['port']['id'], body['port_id']) + expected_port_update = { + 'device_owner': l3_constants.DEVICE_OWNER_ROUTER_INTF, + 'device_id': r['router']['id']} + update_port.assert_called_with( + mock.ANY, p['port']['id'], {'port': expected_port_update}) + # fetch port and confirm device_id + body = self._show('ports', p['port']['id']) + self.assertEqual(r['router']['id'], body['port']['device_id']) - # fetch port and confirm device_id - body = self._show('ports', p['port']['id']) - self.assertEqual(body['port']['device_id'], r['router']['id']) - - # clean-up - self._router_interface_action('remove', - r['router']['id'], - None, - p['port']['id']) + # clean-up + self._router_interface_action('remove', + r['router']['id'], + None, + p['port']['id']) def test_router_add_interface_multiple_ipv4_subnet_port_returns_400(self): """Test adding router port with multiple IPv4 subnets fails.