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

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,6 +244,7 @@ class L3_NAT_db_mixin(l3.RouterPluginBase):
             self._check_for_dup_router_subnet(context, router_id,
                                               port['network_id'],
                                               fixed_ips[0]['subnet_id'])
+            with context.session.begin(subtransactions=True):
                 port.update({'device_id': router_id,
                              'device_owner': DEVICE_OWNER_ROUTER_INTF})
         elif 'subnet_id' in interface_info:
@@ -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:

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):

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)

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)

quantum/tests/unit/test_db_plugin.py

@@ -432,19 +432,22 @@ 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
+                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
+            if not no_delete:
                 self._delete('ports', port['port']['id'])
 
 

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: