From d0d94c03872fe5096e17139e507620396a486680 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 fabe80691e0..ca3b2018e8d 100644
--- a/src/bgp/bgp_path.cc
+++ b/src/bgp/bgp_path.cc
@@ -125,6 +125,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());
+}
+
 BgpSecondaryPath::BgpSecondaryPath(const IPeer *peer, uint32_t path_id,
         PathSource src, const BgpAttrPtr ptr, uint32_t flags, uint32_t label)
     : BgpPath(peer, path_id, src, ptr, flags, label) {
diff --git a/src/bgp/bgp_path.h b/src/bgp/bgp_path.h
index ca6ce8880c2..0f911fcedcc 100644
--- a/src/bgp/bgp_path.h
+++ b/src/bgp/bgp_path.h
@@ -123,6 +123,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 7f4e80ae1bd..2e5b03e4cfe 100644
--- a/src/bgp/routing-instance/rtarget_group_mgr.cc
+++ b/src/bgp/routing-instance/rtarget_group_mgr.cc
@@ -118,6 +118,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->());
@@ -127,6 +128,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 c0e152639f2..d889373462b 100644
--- a/src/bgp/test/bgp_xmpp_rtarget_test.cc
+++ b/src/bgp/test/bgp_xmpp_rtarget_test.cc
@@ -3366,6 +3366,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