From 3fd8f294937d5d84ec7ff85ccfb4936d597fef88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89douard=20Thuleau?= Date: 2017年1月27日 18:09:11 +0100 Subject: [PATCH] Filtering BGP VPN list with resource associations For db based drivers, use the Neutron db query hooks mechanism to apply resource association filters on query results. That improvement permits to remove two mandatory methods of the driver interface 'find_bgpvpns_for_network' and 'find_bgpvpns_for_router' which can be replaced by the 'get_bgpvpns' method with appropriated filters. For non db based drivers, improves utils method to filter resources with a list of values instead of one value only. In that case, the resource attribute value must contains filtered values at least (like it's done for db based drivers). Change-Id: Icbbb7ba704fbb3c025aabbddbb5121a2ecd852cc Closes-Bug: #1659895 Implements blueprint api-find-bgpvpn-for-net --- networking_bgpvpn/neutron/db/bgpvpn_db.py | 36 ++-- .../neutron/services/common/utils.py | 11 +- networking_bgpvpn/neutron/services/plugin.py | 17 +- .../service_drivers/bagpipe/bagpipe.py | 10 +- .../services/service_drivers/driver_api.py | 15 -- .../opencontrail/opencontrail.py | 10 -- .../service_drivers/opendaylight/odl.py | 8 +- networking_bgpvpn/tests/unit/db/test_db.py | 154 +++++++++++++++--- .../tests/unit/services/common/__init__.py | 0 .../tests/unit/services/common/test_utils.py | 61 +++++++ ...resource-association-2acdbc5b59d1a40a.yaml | 4 + 11 files changed, 252 insertions(+), 74 deletions(-) create mode 100644 networking_bgpvpn/tests/unit/services/common/__init__.py create mode 100644 networking_bgpvpn/tests/unit/services/common/test_utils.py create mode 100644 releasenotes/notes/filtering-on-resource-association-2acdbc5b59d1a40a.yaml diff --git a/networking_bgpvpn/neutron/db/bgpvpn_db.py b/networking_bgpvpn/neutron/db/bgpvpn_db.py index 5e7becba..eb82550d 100644 --- a/networking_bgpvpn/neutron/db/bgpvpn_db.py +++ b/networking_bgpvpn/neutron/db/bgpvpn_db.py @@ -132,6 +132,26 @@ class BGPVPNPluginDb(common_db_mixin.CommonDbMixin): } return self._fields(res, fields) + def _list_bgpvpns_result_filter_hook(self, query, filters): + values = filters and filters.get('networks', []) + if values: + query = query.join(BGPVPNNetAssociation) + query = query.filter(BGPVPNNetAssociation.network_id.in_(values)) + + values = filters and filters.get('routers', []) + if values: + query = query.join(BGPVPNRouterAssociation) + query = query.filter(BGPVPNRouterAssociation.router_id.in_(values)) + + return query + + common_db_mixin.CommonDbMixin.register_model_query_hook( + BGPVPN, + "bgpvpn_filter_by_resource_association", + None, + None, + '_list_bgpvpns_result_filter_hook') + def create_bgpvpn(self, context, bgpvpn): rt = utils.rtrd_list2str(bgpvpn['route_targets']) i_rt = utils.rtrd_list2str(bgpvpn['import_targets']) @@ -194,15 +214,6 @@ class BGPVPNPluginDb(common_db_mixin.CommonDbMixin): context.session.delete(bgpvpn_db) return bgpvpn - def find_bgpvpns_for_network(self, context, network_id, bgpvpn_type=None): - # Note : we could use added backref in the network table - query = (context.session.query(BGPVPN). - join(BGPVPNNetAssociation). - filter(BGPVPNNetAssociation.network_id == network_id)) - if bgpvpn_type is not None: - query = query.filter(BGPVPN.type == bgpvpn_type) - return [self._make_bgpvpn_dict(bgpvpn) for bgpvpn in query.all()] - def _make_net_assoc_dict(self, net_assoc_db, fields=None): res = {'id': net_assoc_db['id'], 'tenant_id': net_assoc_db['tenant_id'], @@ -259,13 +270,6 @@ class BGPVPNPluginDb(common_db_mixin.CommonDbMixin): context.session.delete(net_assoc_db) return net_assoc - def find_bgpvpns_for_router(self, context, router_id): - query = (context.session.query(BGPVPN). - join(BGPVPNRouterAssociation). - filter(BGPVPNRouterAssociation.router_id == router_id)) - - return [self._make_bgpvpn_dict(bgpvpn) for bgpvpn in query.all()] - def _make_router_assoc_dict(self, router_assoc_db, fields=None): res = {'id': router_assoc_db['id'], 'tenant_id': router_assoc_db['tenant_id'], diff --git a/networking_bgpvpn/neutron/services/common/utils.py b/networking_bgpvpn/neutron/services/common/utils.py index 6f1513d5..5318358f 100644 --- a/networking_bgpvpn/neutron/services/common/utils.py +++ b/networking_bgpvpn/neutron/services/common/utils.py @@ -41,10 +41,13 @@ def filter_resource(resource, filters=None): filters = {} for key, value in filters.items(): if key in resource.keys(): - if isinstance(value, list): - if resource[key] not in value: - return False - elif resource[key] != value: + if not isinstance(value, list): + value = [value] + if isinstance(resource[key], list): + resource_value = resource[key] + else: + resource_value = [resource[key]] + if not set(value).issubset(set(resource_value)): return False return True diff --git a/networking_bgpvpn/neutron/services/plugin.py b/networking_bgpvpn/neutron/services/plugin.py index fac042fd..a2f9e939 100644 --- a/networking_bgpvpn/neutron/services/plugin.py +++ b/networking_bgpvpn/neutron/services/plugin.py @@ -70,12 +70,21 @@ class BGPVPNPlugin(bgpvpn.BGPVPNPluginBase): network_id = kwargs.get('network_id') router_id = kwargs.get('router_id') try: - routers_bgpvpns = self.driver.find_bgpvpns_for_router(context, - router_id) + routers_bgpvpns = self.driver.get_bgpvpns( + context, + filters={ + 'routers': [router_id], + }, + ) except bgpvpn.BGPVPNRouterAssociationNotSupported: return - nets_bgpvpns = self.driver.find_bgpvpns_for_network( - context, network_id, bgpvpn_type=constants.BGPVPN_L3) + nets_bgpvpns = self.driver.get_bgpvpns( + context, + filters={ + 'networks': [network_id], + 'type': [constants.BGPVPN_L3], + }, + ) if routers_bgpvpns and nets_bgpvpns: msg = _('It is not allowed to add an interface to a router if ' diff --git a/networking_bgpvpn/neutron/services/service_drivers/bagpipe/bagpipe.py b/networking_bgpvpn/neutron/services/service_drivers/bagpipe/bagpipe.py index 3295f793..f5e52b7d 100644 --- a/networking_bgpvpn/neutron/services/service_drivers/bagpipe/bagpipe.py +++ b/networking_bgpvpn/neutron/services/service_drivers/bagpipe/bagpipe.py @@ -271,9 +271,13 @@ class BaGPipeBGPVPNDriver(driver_api.BGPVPNDriver): def _bgpvpns_for_network(self, context, network_id): return ( - self.bgpvpn_db.find_bgpvpns_for_network(context, network_id) - or self.retrieve_bgpvpns_of_router_assocs_by_network(context, - network_id) + self.bgpvpn_db.get_bgpvpns( + context, + filters={ + 'networks': [network_id], + }, + ) or self.retrieve_bgpvpns_of_router_assocs_by_network(context, + network_id) ) def _retrieve_bgpvpn_network_info_for_port(self, context, port): diff --git a/networking_bgpvpn/neutron/services/service_drivers/driver_api.py b/networking_bgpvpn/neutron/services/service_drivers/driver_api.py index a76436c2..82e06a8f 100644 --- a/networking_bgpvpn/neutron/services/service_drivers/driver_api.py +++ b/networking_bgpvpn/neutron/services/service_drivers/driver_api.py @@ -70,10 +70,6 @@ class BGPVPNDriverBase(object): def delete_net_assoc(self, context, assoc_id, bgpvpn_id): pass - @abc.abstractmethod - def find_bgpvpns_for_network(self, context, network_id, bgpvpn_type=None): - pass - @abc.abstractmethod def create_router_assoc(self, context, bgpvpn_id, router_association): pass @@ -90,10 +86,6 @@ class BGPVPNDriverBase(object): def delete_router_assoc(self, context, assoc_id, bgpvpn_id): pass - @abc.abstractmethod - def find_bgpvpns_for_router(self, context, router_id): - pass - @six.add_metaclass(abc.ABCMeta) class BGPVPNDriverDBMixin(BGPVPNDriverBase): @@ -158,10 +150,6 @@ class BGPVPNDriverDBMixin(BGPVPNDriverBase): bgpvpn_id) self.delete_net_assoc_postcommit(context, net_assoc) - def find_bgpvpns_for_network(self, context, network_id, bgpvpn_type=None): - return self.bgpvpn_db.find_bgpvpns_for_network( - context, network_id, bgpvpn_type) - def create_router_assoc(self, context, bgpvpn_id, router_association): with context.session.begin(subtransactions=True): assoc = self.bgpvpn_db.create_router_assoc(context, bgpvpn_id, @@ -184,9 +172,6 @@ class BGPVPNDriverDBMixin(BGPVPNDriverBase): bgpvpn_id) self.delete_router_assoc_postcommit(context, router_assoc) - def find_bgpvpns_for_router(self, context, router_id): - return self.bgpvpn_db.find_bgpvpns_for_router(context, router_id) - @abc.abstractmethod def create_bgpvpn_postcommit(self, context, bgpvpn): pass diff --git a/networking_bgpvpn/neutron/services/service_drivers/opencontrail/opencontrail.py b/networking_bgpvpn/neutron/services/service_drivers/opencontrail/opencontrail.py index dc7fbb49..3106c9f3 100644 --- a/networking_bgpvpn/neutron/services/service_drivers/opencontrail/opencontrail.py +++ b/networking_bgpvpn/neutron/services/service_drivers/opencontrail/opencontrail.py @@ -406,12 +406,6 @@ class OpenContrailBGPVPNDriver(driver_api.BGPVPNDriverBase): return net_assoc - def find_bgpvpns_for_network(self, context, network_id, bgpvpn_type=None): - # TODO(matrohon) : get bgpvpns from the kv store - raise bgpvpn_ext.BGPVPNFindFromNetNotSupported( - driver=OPENCONTRAIL_BGPVPN_DRIVER_NAME, - net_id=network_id) - def create_router_assoc(self, context, bgpvpn_id, router_association): raise bgpvpn_ext.BGPVPNRouterAssociationNotSupported( driver=OPENCONTRAIL_BGPVPN_DRIVER_NAME) @@ -427,7 +421,3 @@ class OpenContrailBGPVPNDriver(driver_api.BGPVPNDriverBase): def delete_router_assoc(self, context, assoc_id, bgpvpn_id): raise bgpvpn_ext.BGPVPNRouterAssociationNotSupported( driver=OPENCONTRAIL_BGPVPN_DRIVER_NAME) - - def find_bgpvpns_for_router(self, context, router_id): - raise bgpvpn_ext.BGPVPNRouterAssociationNotSupported( - driver=OPENCONTRAIL_BGPVPN_DRIVER_NAME) diff --git a/networking_bgpvpn/neutron/services/service_drivers/opendaylight/odl.py b/networking_bgpvpn/neutron/services/service_drivers/opendaylight/odl.py index 40fd18db..267369af 100644 --- a/networking_bgpvpn/neutron/services/service_drivers/opendaylight/odl.py +++ b/networking_bgpvpn/neutron/services/service_drivers/opendaylight/odl.py @@ -74,8 +74,12 @@ class OpenDaylightBgpvpnDriver(driver_api.BGPVPNDriver): self.client.sendjson('put', url, {BGPVPNS[:-1]: bgpvpn}) def create_net_assoc_precommit(self, context, net_assoc): - bgpvpns = self.bgpvpn_db.find_bgpvpns_for_network( - context, net_assoc['network_id']) + bgpvpns = self.bgpvpn_db.get_bgpvpns( + context, + filters={ + 'networks': [net_assoc['network_id']], + }, + ) if len(bgpvpns)> 1: raise bgpvpn_ext.BGPVPNNetworkAssocExistsAnotherBgpvpn( driver=OPENDAYLIGHT_BGPVPN_DRIVER_NAME, diff --git a/networking_bgpvpn/tests/unit/db/test_db.py b/networking_bgpvpn/tests/unit/db/test_db.py index 04cf5c32..b6ab6973 100644 --- a/networking_bgpvpn/tests/unit/db/test_db.py +++ b/networking_bgpvpn/tests/unit/db/test_db.py @@ -23,6 +23,10 @@ from networking_bgpvpn.neutron.services.common import constants from networking_bgpvpn.tests.unit.services import test_plugin +def _id_list(list): + return [bgpvpn['id'] for bgpvpn in list] + + class BgpvpnDBTestCase(test_plugin.BgpvpnTestCaseMixin): def setUp(self): @@ -113,10 +117,12 @@ class BgpvpnDBTestCase(test_plugin.BgpvpnTestCaseMixin): self.assertEqual([], bgpvpn2['route_distinguishers']) # find bgpvpn by network_id - bgpvpn3 = self.plugin_db.find_bgpvpns_for_network( + bgpvpn3 = self.plugin_db.get_bgpvpns( self.ctx, - net['network']['id'] - ) + filters={ + 'networks': [net['network']['id']], + }, + ) self.assertEqual(1, len(bgpvpn3)) self.assertEqual(bgpvpn2['id'], bgpvpn3[0]['id']) @@ -201,7 +207,7 @@ class BgpvpnDBTestCase(test_plugin.BgpvpnTestCaseMixin): id, {'tenant_id': self._tenant_id, 'network_id': net_id}) - def test_db_find_bgpvpn_for_net(self): + def test_db_find_bgpvpn_for_associated_network(self): with self.network() as net, \ self.bgpvpn(type=constants.BGPVPN_L2) as bgpvpn_l2, \ self.bgpvpn() as bgpvpn_l3, \ @@ -211,25 +217,36 @@ class BgpvpnDBTestCase(test_plugin.BgpvpnTestCaseMixin): net['network']['id']): net_id = net['network']['id'] - def _id_list(list): - return [bgpvpn['id'] for bgpvpn in list] - bgpvpn_id_list = _id_list( - self.plugin_db.find_bgpvpns_for_network(self.ctx, net_id)) - self.assertIn(bgpvpn_l2['bgpvpn']['id'], - bgpvpn_id_list) - self.assertIn(bgpvpn_l3['bgpvpn']['id'], - bgpvpn_id_list) - bgpvpn_l2_id_list = _id_list( - self.plugin_db.find_bgpvpns_for_network( - self.ctx, net_id, bgpvpn_type=constants.BGPVPN_L2)) + self.plugin_db.get_bgpvpns( + self.ctx, + filters={'networks': [net_id]}, + ) + ) + self.assertIn(bgpvpn_l2['bgpvpn']['id'], bgpvpn_id_list) + self.assertIn(bgpvpn_l3['bgpvpn']['id'], bgpvpn_id_list) + bgpvpn_l2_id_list = _id_list( + self.plugin_db.get_bgpvpns( + self.ctx, + filters={ + 'networks': [net_id], + 'type': [constants.BGPVPN_L2], + }, + ) + ) self.assertIn(bgpvpn_l2['bgpvpn']['id'], bgpvpn_l2_id_list) self.assertNotIn(bgpvpn_l3['bgpvpn']['id'], bgpvpn_l2_id_list) bgpvpn_l3_id_list = _id_list( - self.plugin_db.find_bgpvpns_for_network( - self.ctx, net_id, bgpvpn_type=constants.BGPVPN_L3)) + self.plugin_db.get_bgpvpns( + self.ctx, + filters={ + 'networks': [net_id], + 'type': [constants.BGPVPN_L3], + }, + ) + ) self.assertNotIn(bgpvpn_l2['bgpvpn']['id'], bgpvpn_l3_id_list[0]) self.assertIn(bgpvpn_l3['bgpvpn']['id'], bgpvpn_l3_id_list[0]) @@ -253,14 +270,16 @@ class BgpvpnDBTestCase(test_plugin.BgpvpnTestCaseMixin): bgpvpn = self.plugin_db.get_bgpvpn(self.ctx, id) self.assertEqual([], bgpvpn['routers']) - def test_db_find_bgpvpn_for_router(self): + def test_db_find_bgpvpn_for_associated_router(self): with self.router(tenant_id=self._tenant_id) as router: router_id = router['router']['id'] with self.bgpvpn() as bgpvpn: id = bgpvpn['bgpvpn']['id'] with self.assoc_router(id, router_id=router_id): - bgpvpn_list = self.plugin_db.\ - find_bgpvpns_for_router(self.ctx, router_id) + bgpvpn_list = self.plugin_db.get_bgpvpns( + self.ctx, + filters={'routers': [router_id]}, + ) self.assertEqual(id, bgpvpn_list[0]['id']) def test_db_delete_router(self): @@ -272,3 +291,98 @@ class BgpvpnDBTestCase(test_plugin.BgpvpnTestCaseMixin): do_disassociate=False) bgpvpn_db = self.plugin_db.get_bgpvpn(self.ctx, id) self.assertEqual([], bgpvpn_db['routers']) + + def test_db_list_bgpvpn_filtering_associated_resources(self): + with self.network() as network1, \ + self.network() as network2, \ + self.router(tenant_id=self._tenant_id) as router1, \ + self.router(tenant_id=self._tenant_id) as router2, \ + self.bgpvpn() as bgpvpn1, \ + self.bgpvpn() as bgpvpn2, \ + self.bgpvpn() as bgpvpn3, \ + self.assoc_net(bgpvpn1['bgpvpn']['id'], + network1['network']['id']), \ + self.assoc_router(bgpvpn3['bgpvpn']['id'], + router1['router']['id']), \ + self.assoc_net(bgpvpn2['bgpvpn']['id'], + network2['network']['id']), \ + self.assoc_router(bgpvpn2['bgpvpn']['id'], + router2['router']['id']): + network1_id = network1['network']['id'] + network2_id = network2['network']['id'] + router1_id = router1['router']['id'] + router2_id = router2['router']['id'] + + bgpvpn_id_list = _id_list( + self.plugin_db.get_bgpvpns( + self.ctx, + filters={ + 'networks': [network1_id], + }, + ) + ) + self.assertIn(bgpvpn1['bgpvpn']['id'], bgpvpn_id_list) + self.assertNotIn(bgpvpn2['bgpvpn']['id'], bgpvpn_id_list) + self.assertNotIn(bgpvpn3['bgpvpn']['id'], bgpvpn_id_list) + + bgpvpn_id_list = _id_list( + self.plugin_db.get_bgpvpns( + self.ctx, + filters={ + 'networks': [network1_id, network2_id], + }, + ) + ) + self.assertIn(bgpvpn1['bgpvpn']['id'], bgpvpn_id_list) + self.assertIn(bgpvpn2['bgpvpn']['id'], bgpvpn_id_list) + self.assertNotIn(bgpvpn3['bgpvpn']['id'], bgpvpn_id_list) + + bgpvpn_id_list = _id_list( + self.plugin_db.get_bgpvpns( + self.ctx, + filters={ + 'routers': [router1_id], + }, + ) + ) + self.assertNotIn(bgpvpn1['bgpvpn']['id'], bgpvpn_id_list) + self.assertNotIn(bgpvpn2['bgpvpn']['id'], bgpvpn_id_list) + self.assertIn(bgpvpn3['bgpvpn']['id'], bgpvpn_id_list) + + bgpvpn_id_list = _id_list( + self.plugin_db.get_bgpvpns( + self.ctx, + filters={ + 'routers': [router1_id, router2_id], + }, + ) + ) + self.assertNotIn(bgpvpn1['bgpvpn']['id'], bgpvpn_id_list) + self.assertIn(bgpvpn2['bgpvpn']['id'], bgpvpn_id_list) + self.assertIn(bgpvpn3['bgpvpn']['id'], bgpvpn_id_list) + + bgpvpn_id_list = _id_list( + self.plugin_db.get_bgpvpns( + self.ctx, + filters={ + 'networks': [network1_id], + 'routers': [router1_id], + }, + ) + ) + self.assertNotIn(bgpvpn1['bgpvpn']['id'], bgpvpn_id_list) + self.assertNotIn(bgpvpn2['bgpvpn']['id'], bgpvpn_id_list) + self.assertNotIn(bgpvpn3['bgpvpn']['id'], bgpvpn_id_list) + + bgpvpn_id_list = _id_list( + self.plugin_db.get_bgpvpns( + self.ctx, + filters={ + 'networks': [network2_id], + 'routers': [router2_id], + }, + ) + ) + self.assertNotIn(bgpvpn1['bgpvpn']['id'], bgpvpn_id_list) + self.assertIn(bgpvpn2['bgpvpn']['id'], bgpvpn_id_list) + self.assertNotIn(bgpvpn3['bgpvpn']['id'], bgpvpn_id_list) diff --git a/networking_bgpvpn/tests/unit/services/common/__init__.py b/networking_bgpvpn/tests/unit/services/common/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/networking_bgpvpn/tests/unit/services/common/test_utils.py b/networking_bgpvpn/tests/unit/services/common/test_utils.py new file mode 100644 index 00000000..981d5d27 --- /dev/null +++ b/networking_bgpvpn/tests/unit/services/common/test_utils.py @@ -0,0 +1,61 @@ +# Copyright (c) 2017 Juniper Networks, Inc. +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from neutron.tests import base + +from networking_bgpvpn.neutron.services.common.utils import filter_resource + + +class TestFilterResource(base.BaseTestCase): + _fake_resource_string = { + 'fake_attribute': 'fake_value1', + } + _fake_resource_list = { + 'fake_attribute': ['fake_value1', 'fake_value2', 'fake_value3'], + } + + def test_filter_resource_succeeds_with_one_value(self): + filters = { + 'fake_attribute': 'fake_value1', + } + self.assertTrue(filter_resource(self._fake_resource_string, filters)) + self.assertTrue(filter_resource(self._fake_resource_list, filters)) + + def test_filter_resource_fails_with_one_value(self): + filters = { + 'fake_attribute': 'wrong_fake_value1', + } + self.assertFalse(filter_resource(self._fake_resource_string, filters)) + self.assertFalse(filter_resource(self._fake_resource_list, filters)) + + def test_filter_resource_succeeds_with_list_of_values(self): + filters = { + 'fake_attribute': ['fake_value1'], + } + self.assertTrue(filter_resource(self._fake_resource_string, filters)) + filters = { + 'fake_attribute': ['fake_value1', 'fake_value2'], + } + self.assertTrue(filter_resource(self._fake_resource_list, filters)) + + def test_filter_resource_fails_with_list_of_values(self): + filters = { + 'fake_attribute': ['wrong_fake_value1'], + } + self.assertFalse(filter_resource(self._fake_resource_string, filters)) + filters = { + 'fake_attribute': ['wrong_fake_value1', 'fake_value2'], + } + self.assertFalse(filter_resource(self._fake_resource_list, filters)) diff --git a/releasenotes/notes/filtering-on-resource-association-2acdbc5b59d1a40a.yaml b/releasenotes/notes/filtering-on-resource-association-2acdbc5b59d1a40a.yaml new file mode 100644 index 00000000..fedb57b4 --- /dev/null +++ b/releasenotes/notes/filtering-on-resource-association-2acdbc5b59d1a40a.yaml @@ -0,0 +1,4 @@ +--- +features: + - The API now supports filtering BGPVPN resources based + on the networks or routers they are associated with.

AltStyle によって変換されたページ (->オリジナル) /