diff --git a/src/bgp/bgp_config_ifmap.cc b/src/bgp/bgp_config_ifmap.cc index f26c35a85f4..19b735942ba 100644 --- a/src/bgp/bgp_config_ifmap.cc +++ b/src/bgp/bgp_config_ifmap.cc @@ -654,9 +654,12 @@ static bool GetInstanceTargetRouteTarget(DBGraph *graph, IFMapNode *node, // // Fill in all the export route targets for a routing-instance. The input // IFMapNode represents the routing-instance. We traverse the graph edges -// and look for instance-target adjacencies. If the instance-target has is -// an export target i.e. it's not import-only, add the route-target to the -// vector. +// and look for instance-target adjacencies. If the instance-target is an +// export and import target, add it to the vector. +// +// Note that we purposely skip adding export only targets to the vector. +// Reasoning is that export only targets are manually configured by users +// and hence should not be imported based on policy. // static void GetRoutingInstanceExportTargets(DBGraph *graph, IFMapNode *node, vector *target_list) { @@ -671,7 +674,7 @@ static void GetRoutingInstanceExportTargets(DBGraph *graph, IFMapNode *node, if (!itarget) continue; const autogen::InstanceTargetType &itt = itarget->data(); - if (itt.import_export != "import") + if (itt.import_export.empty()) target_list->push_back(target); } } diff --git a/src/bgp/routing-instance/routing_instance.cc b/src/bgp/routing-instance/routing_instance.cc index 2452ae5d507..a8b3c4a8d66 100644 --- a/src/bgp/routing-instance/routing_instance.cc +++ b/src/bgp/routing-instance/routing_instance.cc @@ -92,9 +92,15 @@ const RoutingInstance *RoutingInstanceMgr::GetDefaultRoutingInstance() const { // Go through all export targets for the RoutingInstance and add an entry for // each one to the InstanceTargetMap. // +// Ignore export targets that are not in the import list. This is done since +// export only targets are managed by user and the same export only target can +// be configured on multiple virtual networks. +// void RoutingInstanceMgr::InstanceTargetAdd(RoutingInstance *rti) { for (RoutingInstance::RouteTargetList::const_iterator it = rti->GetExportList().begin(); it != rti->GetExportList().end(); ++it) { + if (rti->GetImportList().find(*it) == rti->GetImportList().end()) + continue; target_map_.insert(make_pair(*it, rti)); } } @@ -105,9 +111,14 @@ void RoutingInstanceMgr::InstanceTargetAdd(RoutingInstance *rti) { // entries in the InstanceTargetMap for a given export target. Hence we need // to make sure that we only remove the entry that matches the RoutingInstance. // +// Ignore export targets that are not in the import list. They shouldn't be in +// in the map because of the same check in InstanceTargetAdd. +// void RoutingInstanceMgr::InstanceTargetRemove(const RoutingInstance *rti) { for (RoutingInstance::RouteTargetList::const_iterator it = rti->GetExportList().begin(); it != rti->GetExportList().end(); ++it) { + if (rti->GetImportList().find(*it) == rti->GetImportList().end()) + continue; for (InstanceTargetMap::iterator loc = target_map_.find(*it); loc != target_map_.end() && loc->first == *it; ++loc) { if (loc->second == rti) { diff --git a/src/bgp/test/bgp_config_test.cc b/src/bgp/test/bgp_config_test.cc index 56c473f825a..2bfef908ae1 100644 --- a/src/bgp/test/bgp_config_test.cc +++ b/src/bgp/test/bgp_config_test.cc @@ -1146,7 +1146,7 @@ TEST_F(BgpConfigTest, Instances3) { // Verify number of export and import targets in green. TASK_UTIL_EXPECT_EQ(1, green->GetExportList().size()); - TASK_UTIL_EXPECT_EQ(4, green->GetImportList().size()); + TASK_UTIL_EXPECT_EQ(3, green->GetImportList().size()); // Change the connection to a unidirectional one from green to red. autogen::ConnectionType *connection_type1 = new autogen::ConnectionType; @@ -1180,7 +1180,7 @@ TEST_F(BgpConfigTest, Instances3) { // Verify number of export and import targets in green. TASK_UTIL_EXPECT_EQ(1, green->GetExportList().size()); - TASK_UTIL_EXPECT_EQ(4, green->GetImportList().size()); + TASK_UTIL_EXPECT_EQ(3, green->GetImportList().size()); // Clean up. ifmap_test_util::IFMapMsgUnlink(&config_db_, diff --git a/src/bgp/test/routing_instance_mgr_test.cc b/src/bgp/test/routing_instance_mgr_test.cc index 221d9429db3..a714a4e143a 100644 --- a/src/bgp/test/routing_instance_mgr_test.cc +++ b/src/bgp/test/routing_instance_mgr_test.cc @@ -733,6 +733,54 @@ TEST_F(RoutingInstanceMgrTest, VnIndexByExtCommunity12) { TASK_UTIL_EXPECT_EQ(0, GetVnIndexByExtCommunity(ext_community_12y)); } +// +// Two RIs with a common export only target, different VNs. +// The 2 RIs also have their unique import + export targets. +// Lookup based on the common export target should fail. +// Lookup based on the unique target should succeed. +// Lookup based on the both targets should succeed. +// +TEST_F(RoutingInstanceMgrTest, VnIndexByExtCommunity13) { + scoped_ptr ri1_cfg; + ri1_cfg.reset(BgpTestUtil::CreateBgpInstanceConfig("ri1", + "target:100:1", "target:100:1 target:100:99", "vn1", 1)); + scoped_ptr ri2_cfg; + ri2_cfg.reset(BgpTestUtil::CreateBgpInstanceConfig("ri2", + "target:100:2", "target:100:99 target:100:2", "vn2", 2)); + + CreateRoutingInstance(ri1_cfg.get()); + CreateRoutingInstance(ri2_cfg.get()); + + // Lookup based on common target should fail. + ExtCommunityPtr ext_community = GetExtCommunity("target:100:99"); + TASK_UTIL_EXPECT_EQ(0, GetVnIndexByExtCommunity(ext_community)); + + // Lookup based on unique targets should succeed. + ExtCommunityPtr ext_community1 = GetExtCommunity("target:100:1"); + TASK_UTIL_EXPECT_EQ(1, GetVnIndexByExtCommunity(ext_community1)); + ExtCommunityPtr ext_community2 = GetExtCommunity("target:100:2"); + TASK_UTIL_EXPECT_EQ(2, GetVnIndexByExtCommunity(ext_community2)); + + // Lookup based on common and unique targets should succeed. + ExtCommunityPtr ext_community1x = + GetExtCommunity("target:100:1 target:100:99"); + ExtCommunityPtr ext_community1y = + GetExtCommunity("target:100:99 target:100:1"); + TASK_UTIL_EXPECT_EQ(1, GetVnIndexByExtCommunity(ext_community1x)); + TASK_UTIL_EXPECT_EQ(1, GetVnIndexByExtCommunity(ext_community1y)); + + // Lookup based on common and unique targets should succeed. + ExtCommunityPtr ext_community2x = + GetExtCommunity("target:100:2 target:100:99"); + ExtCommunityPtr ext_community2y = + GetExtCommunity("target:100:99 target:100:2"); + TASK_UTIL_EXPECT_EQ(2, GetVnIndexByExtCommunity(ext_community2x)); + TASK_UTIL_EXPECT_EQ(2, GetVnIndexByExtCommunity(ext_community2y)); + + DeleteRoutingInstance(ri1_cfg.get()); + DeleteRoutingInstance(ri2_cfg.get()); +} + class TestEnvironment : public ::testing::Environment { virtual ~TestEnvironment() { } };