From: Dan Wendlandt Date: Fri, 31 Aug 2012 11:29:12 +0000 (-0700) Subject: prevent invalid deletion of ports using by L3 devices X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=8fb4e6efe8dcf1987d41917f71da8db345647321;p=openstack-build%2Fneutron-build.git prevent invalid deletion of ports using by L3 devices bug 1039947 The bug noticed that an admin user could delete a port that stored the underlying IP allocation for a floating IP. This patch prevents the direction deletion of ports via the API for ports that are used as router interfaces, router gateways, of for floating IPs. Add a unit test to check such an invalid delete, and also updates unit tests to avoid them tripping over the new checks. Change-Id: Ief28e3181583428d55259275a7c21151a4a4fa9b --- diff --git a/quantum/db/l3_db.py b/quantum/db/l3_db.py index f33b76bfd..89a2974e8 100644 --- a/quantum/db/l3_db.py +++ b/quantum/db/l3_db.py @@ -153,7 +153,7 @@ class L3_NAT_db_mixin(l3.RouterPluginBase): with context.session.begin(subtransactions=True): router.update({'gw_port_id': None}) context.session.add(router) - self.delete_port(context, gw_port['id']) + self.delete_port(context, gw_port['id'], l3_port_check=False) if network_id is not None and (gw_port is None or gw_port['network_id'] != network_id): @@ -169,7 +169,7 @@ class L3_NAT_db_mixin(l3.RouterPluginBase): 'name': ''}}) if not len(gw_port['fixed_ips']): - self.delete_port(context, gw_port['id']) + self.delete_port(context, gw_port['id'], l3_port_check=False) msg = ('No IPs available for external network %s' % network_id) raise q_exc.BadRequest(resource='router', msg=msg) @@ -244,8 +244,9 @@ class L3_NAT_db_mixin(l3.RouterPluginBase): self._check_for_dup_router_subnet(context, router_id, port['network_id'], fixed_ips[0]['subnet_id']) - port.update({'device_id': router_id, - 'device_owner': DEVICE_OWNER_ROUTER_INTF}) + with context.session.begin(subtransactions=True): + port.update({'device_id': router_id, + 'device_owner': DEVICE_OWNER_ROUTER_INTF}) elif 'subnet_id' in interface_info: subnet_id = interface_info['subnet_id'] subnet = self._get_subnet(context, subnet_id) @@ -277,7 +278,13 @@ class L3_NAT_db_mixin(l3.RouterPluginBase): msg = "Either subnet_id or port_id must be specified" raise q_exc.BadRequest(resource='router', msg=msg) if 'port_id' in interface_info: - port_db = self._get_port(context, interface_info['port_id']) + port_id = interface_info['port_id'] + port_db = self._get_port(context, port_id) + if not (port_db['device_owner'] == DEVICE_OWNER_ROUTER_INTF and + port_db['device_id'] == router_id): + raise w_exc.HTTPNotFound("Router %(router_id)s does not have " + " an interface with id %(port_id)s" + % locals()) if 'subnet_id' in interface_info: port_subnet_id = port_db['fixed_ips'][0]['subnet_id'] if port_subnet_id != interface_info['subnet_id']: @@ -288,7 +295,7 @@ class L3_NAT_db_mixin(l3.RouterPluginBase): if port_db['device_id'] != router_id: raise w_exc.HTTPConflict("port_id %s not used by router" % port_db['id']) - self.delete_port(context, port_db['id']) + self.delete_port(context, port_db['id'], l3_port_check=False) elif 'subnet_id' in interface_info: subnet_id = interface_info['subnet_id'] subnet = self._get_subnet(context, subnet_id) @@ -303,7 +310,7 @@ class L3_NAT_db_mixin(l3.RouterPluginBase): for p in ports: if p['fixed_ips'][0]['subnet_id'] == subnet_id: - self.delete_port(context, p['id']) + self.delete_port(context, p['id'], l3_port_check=False) found = True break except exc.NoResultFound: @@ -461,7 +468,7 @@ class L3_NAT_db_mixin(l3.RouterPluginBase): if not external_port['fixed_ips']: msg = "Unable to find any IP address on external network" # remove the external port - self.delete_port(context, external_port['id']) + self.delete_port(context, external_port['id'], l3_port_check=False) raise q_exc.BadRequest(resource='floatingip', msg=msg) floating_ip_address = external_port['fixed_ips'][0]['ip_address'] @@ -484,7 +491,7 @@ class L3_NAT_db_mixin(l3.RouterPluginBase): except Exception: LOG.exception("Floating IP association failed") # Remove the port created for internal purposes - self.delete_port(context, external_port['id']) + self.delete_port(context, external_port['id'], l3_port_check=False) raise return self._make_floatingip_dict(floatingip_db) @@ -504,7 +511,8 @@ class L3_NAT_db_mixin(l3.RouterPluginBase): floatingip = self._get_floatingip(context, id) with context.session.begin(subtransactions=True): context.session.delete(floatingip) - self.delete_port(context, floatingip['floating_port_id']) + self.delete_port(context, floatingip['floating_port_id'], + l3_port_check=False) def get_floatingip(self, context, id, fields=None): floatingip = self._get_floatingip(context, id) @@ -515,6 +523,21 @@ class L3_NAT_db_mixin(l3.RouterPluginBase): self._make_floatingip_dict, filters=filters, fields=fields) + def prevent_l3_port_deletion(self, context, port_id): + """ Checks to make sure a port is allowed to be deleted, raising + an exception if this is not the case. This should be called by + any plugin when the API requests the deletion of a port, since + some ports for L3 are not intended to be deleted directly via a + DELETE to /ports, but rather via other API calls that perform the + proper deletion checks. + """ + port_db = self._get_port(context, port_id) + if port_db['device_owner'] in [DEVICE_OWNER_ROUTER_INTF, + DEVICE_OWNER_ROUTER_GW, + DEVICE_OWNER_FLOATINGIP]: + raise l3.L3PortInUse(port_id=port_id, + device_owner=port_db['device_owner']) + def disassociate_floatingips(self, context, port_id): with context.session.begin(subtransactions=True): try: diff --git a/quantum/extensions/l3.py b/quantum/extensions/l3.py index 88246e6ab..47c4866e3 100644 --- a/quantum/extensions/l3.py +++ b/quantum/extensions/l3.py @@ -50,7 +50,13 @@ class ExternalGatewayForFloatingIPNotFound(qexception.NotFound): class FloatingIPPortAlreadyAssociated(qexception.InUse): - message = _("Port %(port_id) already has a floating IP associated with it") + message = _("Port %(port_id)s already has a floating IP" + " associated with it") + + +class L3PortInUse(qexception.InUse): + message = _("Port %(port_id)s has owner %(device_owner)s and therefore" + " cannot be deleted directly via the port API.") def _validate_uuid_or_none(data, valid_values=None): diff --git a/quantum/plugins/linuxbridge/lb_quantum_plugin.py b/quantum/plugins/linuxbridge/lb_quantum_plugin.py index 7ec7c3fe8..98839b8dd 100644 --- a/quantum/plugins/linuxbridge/lb_quantum_plugin.py +++ b/quantum/plugins/linuxbridge/lb_quantum_plugin.py @@ -357,6 +357,11 @@ class LinuxBridgePluginV2(db_base_plugin_v2.QuantumDbPluginV2, binding.vlan_id) return port - def delete_port(self, context, id): + def delete_port(self, context, id, l3_port_check=True): + + # if needed, check to see if this is a port owned by + # and l3-router. If so, we should prevent deletion. + if l3_port_check: + self.prevent_l3_port_deletion(context, id) self.disassociate_floatingips(context, id) return super(LinuxBridgePluginV2, self).delete_port(context, id) diff --git a/quantum/plugins/openvswitch/ovs_quantum_plugin.py b/quantum/plugins/openvswitch/ovs_quantum_plugin.py index f8e2702a3..7614d78ac 100644 --- a/quantum/plugins/openvswitch/ovs_quantum_plugin.py +++ b/quantum/plugins/openvswitch/ovs_quantum_plugin.py @@ -423,6 +423,11 @@ class OVSQuantumPluginV2(db_base_plugin_v2.QuantumDbPluginV2, binding.physical_id) return port - def delete_port(self, context, id): + def delete_port(self, context, id, l3_port_check=True): + + # if needed, check to see if this is a port owned by + # and l3-router. If so, we should prevent deletion. + if l3_port_check: + self.prevent_l3_port_deletion(context, id) self.disassociate_floatingips(context, id) return super(OVSQuantumPluginV2, self).delete_port(context, id) diff --git a/quantum/tests/unit/test_db_plugin.py b/quantum/tests/unit/test_db_plugin.py index 15ffbf503..73184f236 100644 --- a/quantum/tests/unit/test_db_plugin.py +++ b/quantum/tests/unit/test_db_plugin.py @@ -432,20 +432,23 @@ class QuantumDbPluginV2TestCase(unittest2.TestCase): self._delete('subnets', subnet['subnet']['id']) @contextlib.contextmanager - def port(self, subnet=None, fixed_ips=None, fmt='json', **kwargs): + def port(self, subnet=None, fixed_ips=None, fmt='json', no_delete=False, + **kwargs): if not subnet: with self.subnet() as subnet: net_id = subnet['subnet']['network_id'] port = self._make_port(fmt, net_id, fixed_ips=fixed_ips, **kwargs) yield port - self._delete('ports', port['port']['id']) + if not no_delete: + self._delete('ports', port['port']['id']) else: net_id = subnet['subnet']['network_id'] port = self._make_port(fmt, net_id, fixed_ips=fixed_ips, **kwargs) yield port - self._delete('ports', port['port']['id']) + if not no_delete: + self._delete('ports', port['port']['id']) class TestBasicGet(QuantumDbPluginV2TestCase): diff --git a/quantum/tests/unit/test_l3_plugin.py b/quantum/tests/unit/test_l3_plugin.py index c757efed1..a60864900 100644 --- a/quantum/tests/unit/test_l3_plugin.py +++ b/quantum/tests/unit/test_l3_plugin.py @@ -220,7 +220,9 @@ class TestL3NatPlugin(db_base_plugin_v2.QuantumDbPluginV2, l3_db.L3_NAT_db_mixin): supported_extension_aliases = ["os-quantum-router"] - def delete_port(self, context, id): + def delete_port(self, context, id, l3_port_check=True): + if l3_port_check: + self.prevent_l3_port_deletion(context, id) self.disassociate_floatingips(context, id) return super(TestL3NatPlugin, self).delete_port(context, id) @@ -332,7 +334,7 @@ class L3NatDBTestCase(test_db_plugin.QuantumDbPluginV2TestCase): def test_router_add_interface_port(self): with self.router() as r: - with self.port() as p: + with self.port(no_delete=True) as p: body = self._router_interface_action('add', r['router']['id'], None, @@ -344,6 +346,12 @@ class L3NatDBTestCase(test_db_plugin.QuantumDbPluginV2TestCase): body = self._show('ports', p['port']['id']) self.assertEquals(body['port']['device_id'], r['router']['id']) + # clean-up + self._router_interface_action('remove', + r['router']['id'], + None, + p['port']['id']) + def test_router_add_interface_dup_subnet1(self): with self.router() as r: with self.subnet() as s: @@ -365,7 +373,7 @@ class L3NatDBTestCase(test_db_plugin.QuantumDbPluginV2TestCase): def test_router_add_interface_dup_subnet2(self): with self.router() as r: with self.subnet() as s: - with self.port(subnet=s) as p1: + with self.port(subnet=s, no_delete=True) as p1: with self.port(subnet=s) as p2: self._router_interface_action('add', r['router']['id'], @@ -377,6 +385,11 @@ class L3NatDBTestCase(test_db_plugin.QuantumDbPluginV2TestCase): p2['port']['id'], expected_code= exc.HTTPBadRequest.code) + # clean-up + self._router_interface_action('remove', + r['router']['id'], + None, + p1['port']['id']) def test_router_add_interface_no_data(self): with self.router() as r: @@ -435,7 +448,7 @@ class L3NatDBTestCase(test_db_plugin.QuantumDbPluginV2TestCase): def test_router_remove_router_interface_wrong_subnet_returns_409(self): with self.router() as r: with self.subnet() as s: - with self.port() as p: + with self.port(no_delete=True) as p: self._router_interface_action('add', r['router']['id'], None, @@ -445,11 +458,16 @@ class L3NatDBTestCase(test_db_plugin.QuantumDbPluginV2TestCase): s['subnet']['id'], p['port']['id'], exc.HTTPConflict.code) + #remove properly to clean-up + self._router_interface_action('remove', + r['router']['id'], + None, + p['port']['id']) - def test_router_remove_router_interface_wrong_port_returns_409(self): + def test_router_remove_router_interface_wrong_port_returns_404(self): with self.router() as r: with self.subnet() as s: - with self.port() as p: + with self.port(no_delete=True) as p: self._router_interface_action('add', r['router']['id'], None, @@ -461,7 +479,12 @@ class L3NatDBTestCase(test_db_plugin.QuantumDbPluginV2TestCase): r['router']['id'], None, p2['port']['id'], - exc.HTTPConflict.code) + exc.HTTPNotFound.code) + # remove correct interface to cleanup + self._router_interface_action('remove', + r['router']['id'], + None, + p['port']['id']) # remove extra port created self._delete('ports', p2['port']['id']) @@ -608,6 +631,16 @@ class L3NatDBTestCase(test_db_plugin.QuantumDbPluginV2TestCase): {'port_id': port_id}}, expected_code=exc.HTTPConflict.code) + def test_floating_ip_direct_port_delete_returns_409(self): + found = False + with self.floatingip_with_assoc() as fip: + for p in self._list('ports')['ports']: + if p['device_owner'] == 'network:floatingip': + self._delete('ports', p['id'], + expected_code=exc.HTTPConflict.code) + found = True + self.assertTrue(found) + def test_create_floatingip_no_ext_gateway_return_404(self): with self.subnet() as public_sub: with self.port() as private_port: