Refine processing of export only route targets

Do not use export only targets to determine origin VN.
Do not import any export only targets when processing a connection.

Change-Id: Iab2c7afe3b4566193290a386b3d26d3c6272cdc2
Closes-Bug: 1378957

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<string> *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);
         }
     }

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

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_,

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<BgpInstanceConfigTest> ri1_cfg;
+    ri1_cfg.reset(BgpTestUtil::CreateBgpInstanceConfig("ri1",
+            "target:100:1", "target:100:1 target:100:99", "vn1", 1));
+    scoped_ptr<BgpInstanceConfigTest> 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() { }
 };