From 1c670069a175c22fadd342b5a105b72d130b299e 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 f6dd3523235..9bd0a354e31 100644
--- a/src/bgp/bgp_path.cc
+++ b/src/bgp/bgp_path.cc
@@ -114,6 +114,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 159d45e22ed..99660e0c33f 100644
--- a/src/bgp/bgp_path.h
+++ b/src/bgp/bgp_path.h
@@ -153,6 +153,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 9da2457492f..eb85c229047 100644
--- a/src/bgp/routing-instance/rtarget_group_mgr.cc
+++ b/src/bgp/routing-instance/rtarget_group_mgr.cc
@@ -114,6 +114,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->());
@@ -123,6 +124,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