Merge "Improve validate of remove_router_interface"

neutron/db/l3_db.py

@@ -490,15 +490,16 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase):
         # NOTE(armando-migliaccio): in the base case this is invariant
         return DEVICE_OWNER_ROUTER_INTF
 
-    def _validate_interface_info(self, interface_info):
+    def _validate_interface_info(self, interface_info, for_removal=False):
         port_id_specified = interface_info and 'port_id' in interface_info
         subnet_id_specified = interface_info and 'subnet_id' in interface_info
         if not (port_id_specified or subnet_id_specified):
             msg = _("Either subnet_id or port_id must be specified")
             raise n_exc.BadRequest(resource='router', msg=msg)
-        if port_id_specified and subnet_id_specified:
+        if not for_removal:
-            msg = _("Cannot specify both subnet-id and port-id")
+            if port_id_specified and subnet_id_specified:
-            raise n_exc.BadRequest(resource='router', msg=msg)
+                msg = _("Cannot specify both subnet-id and port-id")
+                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):
@@ -568,7 +569,9 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase):
         if add_by_port:
             port = self._add_interface_by_port(
                 context, router, interface_info['port_id'], device_owner)
-        elif add_by_sub:
+        # add_by_subnet is not used here, because the validation logic of
+        # _validate_interface_info ensures that either of add_by_* is True.
+        else:
             port = self._add_interface_by_subnet(
                 context, router, interface_info['subnet_id'], device_owner)
 
@@ -654,17 +657,20 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase):
                                                   subnet_id=subnet_id)
 
     def remove_router_interface(self, context, router_id, interface_info):
-        if not interface_info:
+        remove_by_port, remove_by_subnet = (
-            msg = _("Either subnet_id or port_id must be specified")
+            self._validate_interface_info(interface_info, for_removal=True)
-            raise n_exc.BadRequest(resource='router', msg=msg)
+        )
         port_id = interface_info.get('port_id')
         subnet_id = interface_info.get('subnet_id')
         device_owner = self._get_device_owner(context, router_id)
-        if port_id:
+        if remove_by_port:
             port, subnet = self._remove_interface_by_port(context, router_id,
                                                           port_id, subnet_id,
                                                           device_owner)
-        elif subnet_id:
+        # remove_by_subnet is not used here, because the validation logic of
+        # _validate_interface_info ensures that at least one of remote_by_*
+        # is True.
+        else:
             port, subnet = self._remove_interface_by_subnet(
                 context, router_id, subnet_id, device_owner)
 

neutron/db/l3_dvr_db.py

@@ -317,19 +317,21 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin,
         return router_interface_info
 
     def remove_router_interface(self, context, router_id, interface_info):
-        if not interface_info:
+        remove_by_port, remove_by_subnet = (
-            msg = _("Either subnet_id or port_id must be specified")
+            self._validate_interface_info(interface_info, for_removal=True)
-            raise n_exc.BadRequest(resource='router', msg=msg)
+        )
-
         port_id = interface_info.get('port_id')
         subnet_id = interface_info.get('subnet_id')
         router = self._get_router(context, router_id)
         device_owner = self._get_device_owner(context, router)
 
-        if port_id:
+        if remove_by_port:
             port, subnet = self._remove_interface_by_port(
                 context, router_id, port_id, subnet_id, device_owner)
-        elif subnet_id:
+        # remove_by_subnet is not used here, because the validation logic of
+        # _validate_interface_info ensures that at least one of remote_by_*
+        # is True.
+        else:
             port, subnet = self._remove_interface_by_subnet(
                 context, router_id, subnet_id, device_owner)
 

neutron/tests/unit/test_l3_plugin.py

@@ -389,7 +389,7 @@ class L3NatTestCaseMixin(object):
         interface_data = {}
         if subnet_id:
             interface_data.update({'subnet_id': subnet_id})
-        if port_id and (action != 'add' or not subnet_id):
+        if port_id:
             interface_data.update({'port_id': port_id})
 
         req = self.new_action_request('routers', interface_data, router_id,
@@ -1181,6 +1181,17 @@ class L3NatTestCaseBase(L3NatTestCaseMixin):
                                           expected_code=exc.
                                           HTTPBadRequest.code)
 
+    def test_router_add_interface_with_both_ids_returns_400(self):
+        with self.router() as r:
+            with self.subnet() as s:
+                with self.port(subnet=s) as p:
+                    self._router_interface_action('add',
+                                                  r['router']['id'],
+                                                  s['subnet']['id'],
+                                                  p['port']['id'],
+                                                  expected_code=exc.
+                                                  HTTPBadRequest.code)
+
     def test_router_add_gateway_dup_subnet1_returns_400(self):
         with self.router() as r:
             with self.subnet() as s:
@@ -1424,6 +1435,25 @@ class L3NatTestCaseBase(L3NatTestCaseMixin):
                                                   None,
                                                   p['port']['id'])
 
+    def test_router_remove_interface_nothing_returns_400(self):
+        with self.router() as r:
+            with self.subnet() as s:
+                with self.port(subnet=s) as p:
+                    self._router_interface_action('add',
+                                                  r['router']['id'],
+                                                  None,
+                                                  p['port']['id'])
+                    self._router_interface_action('remove',
+                                                  r['router']['id'],
+                                                  None,
+                                                  None,
+                                                  exc.HTTPBadRequest.code)
+                    #remove properly to clean-up
+                    self._router_interface_action('remove',
+                                                  r['router']['id'],
+                                                  None,
+                                                  p['port']['id'])
+
     def test_router_remove_interface_returns_200(self):
         with self.router() as r:
             with self.port() as p:
@@ -1437,6 +1467,19 @@ class L3NatTestCaseBase(L3NatTestCaseMixin):
                                               p['port']['id'],
                                               expected_body=body)
 
+    def test_router_remove_interface_with_both_ids_returns_200(self):
+        with self.router() as r:
+            with self.subnet() as s:
+                with self.port(subnet=s) as p:
+                    self._router_interface_action('add',
+                                                  r['router']['id'],
+                                                  None,
+                                                  p['port']['id'])
+                    self._router_interface_action('remove',
+                                                  r['router']['id'],
+                                                  s['subnet']['id'],
+                                                  p['port']['id'])
+
     def test_router_remove_interface_wrong_port_returns_404(self):
         with self.router() as r:
             with self.subnet():