From 3b41808b8624f5442d65ba638d1a2c1bdc22be00 Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Mon, 1 Feb 2016 01:17:22 -0800 Subject: [PATCH] Only prevent l3 port deletion if router exists This patch modifies the prevent_l3_port_deletion method to actually look up the router_id in the device_owner field to confirm that the router exists before claiming the port is in use. This will allow users to delete ports that may have been orphaned due to race conditions in the cleanup of router interfaces. Partial-Bug: #1540271 Change-Id: Ieffe632f3f3098baf202d3795ab5182982e234bd --- neutron/db/l3_db.py | 59 +++++++++++++++++++------ neutron/tests/unit/db/test_l3_db.py | 55 +++++++++++++++++++++++ neutron/tests/unit/db/test_l3_dvr_db.py | 3 ++ 3 files changed, 104 insertions(+), 13 deletions(-) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index e4d11fd8a86..a11a362b083 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -1154,6 +1154,20 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase): return self._get_collection_count(context, FloatingIP, filters=filters) + def _router_exists(self, context, router_id): + try: + self.get_router(context.elevated(), router_id) + return True + except l3.RouterNotFound: + return False + + def _floating_ip_exists(self, context, floating_ip_id): + try: + self.get_floatingip(context, floating_ip_id) + return True + except l3.FloatingIPNotFound: + return False + def prevent_l3_port_deletion(self, context, port_id): """Checks to make sure a port is allowed to be deleted. @@ -1168,19 +1182,38 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase): except n_exc.PortNotFound: # non-existent ports don't need to be protected from deletion return - if port['device_owner'] in self.router_device_owners: - # Raise port in use only if the port has IP addresses - # Otherwise it's a stale port that can be removed - fixed_ips = port['fixed_ips'] - if fixed_ips: - reason = _('has device owner %s') % port['device_owner'] - raise n_exc.ServicePortInUse(port_id=port['id'], - reason=reason) - else: - LOG.debug("Port %(port_id)s has owner %(port_owner)s, but " - "no IP address, so it can be deleted", - {'port_id': port['id'], - 'port_owner': port['device_owner']}) + if port['device_owner'] not in self.router_device_owners: + return + # Raise port in use only if the port has IP addresses + # Otherwise it's a stale port that can be removed + fixed_ips = port['fixed_ips'] + if not fixed_ips: + LOG.debug("Port %(port_id)s has owner %(port_owner)s, but " + "no IP address, so it can be deleted", + {'port_id': port['id'], + 'port_owner': port['device_owner']}) + return + # NOTE(kevinbenton): we also check to make sure that the + # router still exists. It's possible for HA router interfaces + # to remain after the router is deleted if they encounter an + # error during deletion. + # Elevated context in case router is owned by another tenant + if port['device_owner'] == DEVICE_OWNER_FLOATINGIP: + if not self._floating_ip_exists(context, port['device_id']): + LOG.debug("Floating IP %(f_id)s corresponding to port " + "%(port_id)s no longer exists, allowing deletion.", + {'f_id': port['device_id'], 'port_id': port['id']}) + return + elif not self._router_exists(context, port['device_id']): + LOG.debug("Router %(router_id)s corresponding to port " + "%(port_id)s no longer exists, allowing deletion.", + {'router_id': port['device_id'], + 'port_id': port['id']}) + return + + reason = _('has device owner %s') % port['device_owner'] + raise n_exc.ServicePortInUse(port_id=port['id'], + reason=reason) def disassociate_floatingips(self, context, port_id): """Disassociate all floating IPs linked to specific port. diff --git a/neutron/tests/unit/db/test_l3_db.py b/neutron/tests/unit/db/test_l3_db.py index 5694e6808e2..8d7972c4c39 100644 --- a/neutron/tests/unit/db/test_l3_db.py +++ b/neutron/tests/unit/db/test_l3_db.py @@ -14,8 +14,11 @@ # limitations under the License. import mock +import testtools +from neutron.common import exceptions as n_exc from neutron.db import l3_db +from neutron.extensions import l3 from neutron import manager from neutron.tests import base @@ -130,3 +133,55 @@ class TestL3_NAT_dbonly_mixin(base.BaseTestCase): self.assertEqual([({'id': 'id1'}, 'scope1'), ({'id': 'id2'}, 'scope2'), ({'id': 'id3'}, 'scope3')], result) + + @mock.patch.object(manager.NeutronManager, 'get_plugin') + def test_prevent_l3_port_deletion_port_not_found(self, gp): + # port not found doesn't prevent + gp.return_value.get_port.side_effect = n_exc.PortNotFound(port_id='1') + self.db.prevent_l3_port_deletion(None, None) + + @mock.patch.object(manager.NeutronManager, 'get_plugin') + def test_prevent_l3_port_device_owner_not_router(self, gp): + # ignores other device owners + gp.return_value.get_port.return_value = {'device_owner': 'cat'} + self.db.prevent_l3_port_deletion(None, None) + + @mock.patch.object(manager.NeutronManager, 'get_plugin') + def test_prevent_l3_port_no_fixed_ips(self, gp): + # without fixed IPs is allowed + gp.return_value.get_port.return_value = { + 'device_owner': 'network:router_interface', 'fixed_ips': [], + 'id': 'f' + } + self.db.prevent_l3_port_deletion(None, None) + + @mock.patch.object(manager.NeutronManager, 'get_plugin') + def test_prevent_l3_port_no_router(self, gp): + # without router is allowed + gp.return_value.get_port.return_value = { + 'device_owner': 'network:router_interface', + 'device_id': '44', 'id': 'f', + 'fixed_ips': [{'ip_address': '1.1.1.1', 'subnet_id': '4'}]} + self.db.get_router = mock.Mock() + self.db.get_router.side_effect = l3.RouterNotFound(router_id='44') + self.db.prevent_l3_port_deletion(mock.Mock(), None) + + @mock.patch.object(manager.NeutronManager, 'get_plugin') + def test_prevent_l3_port_existing_router(self, gp): + gp.return_value.get_port.return_value = { + 'device_owner': 'network:router_interface', + 'device_id': 'some_router', 'id': 'f', + 'fixed_ips': [{'ip_address': '1.1.1.1', 'subnet_id': '4'}]} + self.db.get_router = mock.Mock() + with testtools.ExpectedException(n_exc.ServicePortInUse): + self.db.prevent_l3_port_deletion(mock.Mock(), None) + + @mock.patch.object(manager.NeutronManager, 'get_plugin') + def test_prevent_l3_port_existing_floating_ip(self, gp): + gp.return_value.get_port.return_value = { + 'device_owner': 'network:floatingip', + 'device_id': 'some_flip', 'id': 'f', + 'fixed_ips': [{'ip_address': '1.1.1.1', 'subnet_id': '4'}]} + self.db.get_floatingip = mock.Mock() + with testtools.ExpectedException(n_exc.ServicePortInUse): + self.db.prevent_l3_port_deletion(mock.Mock(), None) diff --git a/neutron/tests/unit/db/test_l3_dvr_db.py b/neutron/tests/unit/db/test_l3_dvr_db.py index 750c8ec795d..698bf2aa63b 100644 --- a/neutron/tests/unit/db/test_l3_dvr_db.py +++ b/neutron/tests/unit/db/test_l3_dvr_db.py @@ -181,6 +181,7 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): plugin = mock.Mock() gp.return_value = plugin plugin.get_port.return_value = port + self.mixin._router_exists = mock.Mock(return_value=True) self.assertRaises(exceptions.ServicePortInUse, self.mixin.prevent_l3_port_deletion, self.ctx, @@ -190,6 +191,7 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): port = { 'id': 'my_port_id', 'fixed_ips': mock.ANY, + 'device_id': 'r_id', 'device_owner': l3_const.DEVICE_OWNER_AGENT_GW } self._test_prepare_direct_delete_dvr_internal_ports(port) @@ -198,6 +200,7 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): port = { 'id': 'my_port_id', 'fixed_ips': mock.ANY, + 'device_id': 'r_id', 'device_owner': l3_const.DEVICE_OWNER_ROUTER_SNAT } self._test_prepare_direct_delete_dvr_internal_ports(port)