From 524169697a217e7187c2b376a7a03f82a7433d9f 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 df39d22cb88..b4500fad3c9 100644
--- a/src/bgp/bgp_path.cc
+++ b/src/bgp/bgp_path.cc
@@ -118,6 +118,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