From d308890ac6cb66a8a6b2f64121a2ecf32b1052bd Mon Sep 17 00:00:00 2001
From: Nischal Sheth <nsheth@juniper.net>
Date: Tue, 17 May 2016 13:55:15 -0700
Subject: [PATCH] Use subset of rtarget eBGP paths when advertising vpn routes

Only consider paths that are learnt from same neighbor AS as
best eBGP path.

Note that different RTC routes for the same RT can and will
result in VPN routes being advertised to more than 1 AS. This
is correct behavior.

Change-Id: I7a08c452c9ee5788fdd615e58a096796752070e0
Closes-Bug: 1582933
---
 src/bgp/bgp_path.cc                           |  9 ++++++
 src/bgp/bgp_path.h                            |  1 +
 src/bgp/routing-instance/rtarget_group_mgr.cc |  9 ++++++
 src/bgp/test/bgp_xmpp_rtarget_test.cc         | 30 +++++++++++++++++++
 4 files changed, 49 insertions(+)

diff --git a/src/bgp/bgp_path.cc b/src/bgp/bgp_path.cc
index 5e29d8f51af..6f8ad49a092 100644
--- a/src/bgp/bgp_path.cc
+++ b/src/bgp/bgp_path.cc
@@ -128,6 +128,15 @@ int BgpPath::PathCompare(const BgpPath &rhs, bool allow_ecmp) const {
     return 0;
 }
 
+bool BgpPath::PathSameNeighborAs(const BgpPath &rhs) const {
+    const BgpAttr *rattr = rhs.GetAttr();
+    if (!peer_ || peer_->PeerType() != BgpProto::EBGP)
+        return false;
+    if (!rhs.peer_ || rhs.peer_->PeerType() != BgpProto::EBGP)
+        return false;
+    return (attr_->neighbor_as() == rattr->neighbor_as());
+}
+
 void BgpPath::UpdatePeerRefCount(int count) const {
     if (!peer_)
         return;
diff --git a/src/bgp/bgp_path.h b/src/bgp/bgp_path.h
index c5014c74b1b..653be290730 100644
--- a/src/bgp/bgp_path.h
+++ b/src/bgp/bgp_path.h
@@ -126,6 +126,7 @@ class BgpPath : public Path {
 
     // Select one path over other
     int PathCompare(const BgpPath &rhs, bool allow_ecmp) const;
+    bool PathSameNeighborAs(const BgpPath &rhs) const;
 
 private:
     const IPeer *peer_;
diff --git a/src/bgp/routing-instance/rtarget_group_mgr.cc b/src/bgp/routing-instance/rtarget_group_mgr.cc
index 5b6b652bfdf..4c457bdeb97 100644
--- a/src/bgp/routing-instance/rtarget_group_mgr.cc
+++ b/src/bgp/routing-instance/rtarget_group_mgr.cc
@@ -115,6 +115,7 @@ void RTargetGroupMgr::BuildRTargetDistributionGraph(BgpTable *table,
         return;
     }
 
+    const BgpPath *best_ebgp_path = NULL;
     for (Route::PathList::iterator it = rt->GetPathList().begin();
          it != rt->GetPathList().end(); it++) {
         BgpPath *path = static_cast<BgpPath *>(it.operator->());
@@ -124,6 +125,14 @@ void RTargetGroupMgr::BuildRTargetDistributionGraph(BgpTable *table,
             continue;
 
         const BgpPeer *peer = static_cast<const BgpPeer *>(path->GetPeer());
+        if (peer->PeerType() == BgpProto::EBGP) {
+            if (!best_ebgp_path) {
+                best_ebgp_path = path;
+            } else if (!best_ebgp_path->PathSameNeighborAs(*path)) {
+                continue;
+            }
+        }
+
         std::pair<RtGroup::InterestedPeerList::iterator, bool> ret =
             peer_list.insert(std::pair<const BgpPeer *,
                  RtGroup::RTargetRouteList>(peer, RtGroup::RTargetRouteList()));
diff --git a/src/bgp/test/bgp_xmpp_rtarget_test.cc b/src/bgp/test/bgp_xmpp_rtarget_test.cc
index 376ffdbb9af..3914246f604 100644
--- a/src/bgp/test/bgp_xmpp_rtarget_test.cc
+++ b/src/bgp/test/bgp_xmpp_rtarget_test.cc
@@ -3382,6 +3382,36 @@ TEST_F(BgpXmppRTargetTest, PathSelection5) {
     DeleteInetRoute(mx_.get(), NULL, "blue", BuildPrefix());
 }
 
+//
+// If RTargetRoute is received from EBGP peers in different ASes, VPN routes
+// should be sent to peers in AS of the best EBGP path.
+//
+TEST_F(BgpXmppRTargetTest, PathSelection6) {
+    Unconfigure();
+    task_util::WaitForIdle();
+    Configure(64496, 64497, 64498);
+    agent_a_1_->Subscribe("blue", 1);
+
+    AddRouteTarget(cn1_.get(), "blue", "target:1:1001");
+    AddRouteTarget(cn2_.get(), "blue", "target:1:1001");
+    task_util::WaitForIdle();
+
+    RTargetRoute *rtgt_rt =
+        VerifyRTargetRouteExists(mx_.get(), "64496:target:1:1001");
+    TASK_UTIL_EXPECT_EQ(2, rtgt_rt->count());
+    VerifyRTargetRouteNoExists(mx_.get(), "64497:target:1:1001");
+
+    AddInetRoute(mx_.get(), NULL, "blue", BuildPrefix());
+
+    BgpRoute *inet_rt =
+        VerifyInetRouteExists(cn1_.get(), "blue", BuildPrefix());
+    usleep(50000);
+    TASK_UTIL_EXPECT_EQ(1, inet_rt->count());
+    VerifyInetRouteNoExists(cn2_.get(), "blue", BuildPrefix());
+
+    DeleteInetRoute(mx_.get(), NULL, "blue", BuildPrefix());
+}
+
 //
 // Local AS is different than the global AS.
 // Verify that RTarget route prefixes use local AS while the route target