From 405baef29ffae96bcf5410d5d93217f219a1bfda Mon Sep 17 00:00:00 2001 From: "watanabe.isao" Date: Fri, 30 Jan 2015 17:56:24 +0900 Subject: [PATCH] Improve validate of remove_router_interface Improve validate of remove_router_interface as described in API referrence [1]. [1]http://developer.openstack.org/api-ref-networking-v2.html PUT: /v2.0/routers/{router_id}/add_router_interface PUT: /v2.0/routers/{router_id}/remove_router_interface Also, add missing unit tests to cover a [add; delete] time [none; subnet id only; port id only; both ids] matrices. Change-Id: Ie4ddf61c4a4930a58d0817119566d2285a394065 Closes-Bug: #1416308 --- neutron/db/l3_db.py | 26 +++++++++------- neutron/db/l3_dvr_db.py | 14 +++++---- neutron/tests/unit/test_l3_plugin.py | 45 +++++++++++++++++++++++++++- 3 files changed, 68 insertions(+), 17 deletions(-) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index 5d7d01781..0adfec991 100644 --- a/neutron/db/l3_db.py +++ b/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: - msg = _("Cannot specify both subnet-id and port-id") - raise n_exc.BadRequest(resource='router', msg=msg) + if not for_removal: + if port_id_specified and subnet_id_specified: + 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: - msg = _("Either subnet_id or port_id must be specified") - raise n_exc.BadRequest(resource='router', msg=msg) + remove_by_port, remove_by_subnet = ( + self._validate_interface_info(interface_info, for_removal=True) + ) 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) diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index f630cae53..a5e988ea1 100644 --- a/neutron/db/l3_dvr_db.py +++ b/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: - msg = _("Either subnet_id or port_id must be specified") - raise n_exc.BadRequest(resource='router', msg=msg) - + remove_by_port, remove_by_subnet = ( + self._validate_interface_info(interface_info, for_removal=True) + ) 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) diff --git a/neutron/tests/unit/test_l3_plugin.py b/neutron/tests/unit/test_l3_plugin.py index 7bf9e6f10..5f4d8e384 100644 --- a/neutron/tests/unit/test_l3_plugin.py +++ b/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(): -- 2.45.2