Relax strict adherence to RTC RFC for eBGP peers

Current implementation of RTC (RFC 4684) advertises VPN routes to
exactly 1 eBGP peer in a given ASN even when multiple peers have
advertised an RTC route for a route target.  Expectation is that
the peer in question will advertise the VPN routes to it's iBGP
peers.  While this is technically sufficient, it could result in
sub-optimal multi-path within the neighbor AS in some scenarios.

Fix is to change implementation to send VPN routes to all peers by
default and provide a knob to limit the number of external paths in
future if/when required. IOW, the default value of the TBD knob is
infinity.

Tweak existing tests based on the above behavior change. Note that
test PathSelection3 was incorrect to begin with - there's nothing
in the RFC which says that VPN routes should only be sent to eBGP
peers and not to iBGP peers.  The behavior was just a side effect
of how the eBGP logic was implemented.

Note that JUNOS implementation of RTC advertises VPN routes to only
the best eBGP peer by default, but provides an external-paths knob
to modify this behavior.

Change-Id: I7a4101f4a63de1febaf2b56d0daa325f9042b244
Closes-Bug: 1509945

src/bgp/routing-instance/rtarget_group_mgr.cc

@@ -125,16 +125,13 @@ void RTargetGroupMgr::BuildRTargetDistributionGraph(BgpTable *table,
             break;
         if (!path->GetPeer() || path->GetPeer()->IsXmppPeer())
             continue;
-        const BgpPeer *peer = static_cast<const BgpPeer *>(path->GetPeer());
-        BgpProto::BgpPeerType peer_type = peer->PeerType();
 
+        const BgpPeer *peer = static_cast<const BgpPeer *>(path->GetPeer());
         std::pair<RtGroup::InterestedPeerList::iterator, bool> ret =
             peer_list.insert(std::pair<const BgpPeer *,
                  RtGroup::RTargetRouteList>(peer, RtGroup::RTargetRouteList()));
         assert(ret.second);
         ret.first->second.insert(rt);
-        if (peer_type == BgpProto::EBGP)
-            break;
     }
 
     RTargetPeerSync(table, rt, id, dbstate, &peer_list);

src/bgp/test/bgp_xmpp_rtarget_test.cc

@@ -3198,7 +3198,11 @@ TEST_F(BgpXmppRTargetTest, DisableEnableRouteTargetProcessing6) {
 // RTargetRoute i.e. the one with the lowest router id. The other peer will
 // receive the VPN route via IBGP.
 //
-TEST_F(BgpXmppRTargetTest, PathSelection1) {
+// This test was disabled when default behavior of route target filtering
+// got modified via launchpad bug 1509945. It can be re-enabled when a knob
+// to limit the number of external paths is added.
+//
+TEST_F(BgpXmppRTargetTest, DISABLED_PathSelection1a) {
     Unconfigure();
     task_util::WaitForIdle();
     Configure(64496, 64496, 64497);
@@ -3224,6 +3228,39 @@ TEST_F(BgpXmppRTargetTest, PathSelection1) {
     DeleteInetRoute(mx_.get(), NULL, "blue", BuildPrefix());
 }
 
+//
+// If a RTargetRoute is received from EBGP peers that are in same AS, VPN
+// routes should be sent to all the peers that sent the best path for the
+// RTargetRoute, not just the one with the lowest router-id.
+//
+// This test was added when the default behavior of route target filtering
+// got modified via launchpad bug 1509945.
+//
+TEST_F(BgpXmppRTargetTest, PathSelection1b) {
+    Unconfigure();
+    task_util::WaitForIdle();
+    Configure(64496, 64496, 64497);
+    SubscribeAgents("blue", 1);
+    task_util::WaitForIdle();
+
+    AddRouteTarget(cn1_.get(), "blue", "target:1:1001");
+    AddRouteTarget(cn2_.get(), "blue", "target:1:1001");
+
+    RTargetRoute *rtgt_rt =
+        VerifyRTargetRouteExists(mx_.get(), "64496:target:1:1001");
+    TASK_UTIL_EXPECT_EQ(2, rtgt_rt->count());
+
+    AddInetRoute(mx_.get(), NULL, "blue", BuildPrefix());
+
+    uint32_t mx_id = mx_->bgp_identifier();
+    BgpRoute *rt1 = VerifyInetRouteExists(cn1_.get(), "blue", BuildPrefix());
+    TASK_UTIL_EXPECT_EQ(mx_id, rt1->BestPath()->GetPeer()->bgp_identifier());
+    BgpRoute *rt2 = VerifyInetRouteExists(cn2_.get(), "blue", BuildPrefix());
+    TASK_UTIL_EXPECT_EQ(mx_id, rt2->BestPath()->GetPeer()->bgp_identifier());
+
+    DeleteInetRoute(mx_.get(), NULL, "blue", BuildPrefix());
+}
+
 //
 // If different RTargetRoutes for same RouteTarget are received from different
 // EBGP peers, VPN routes should be sent to each peer.
@@ -3253,7 +3290,7 @@ TEST_F(BgpXmppRTargetTest, PathSelection2) {
 
 //
 // If RTargetRoute is received from an EBGP peer and an IBGP peer, VPN routes
-// should be sent only to the EBGP peer.
+// should be sent to the both the EBGP and IBGP peers.
 //
 TEST_F(BgpXmppRTargetTest, PathSelection3) {
     Unconfigure();
@@ -3272,7 +3309,7 @@ TEST_F(BgpXmppRTargetTest, PathSelection3) {
     AddInetRoute(mx_.get(), NULL, "blue", BuildPrefix());
 
     VerifyInetRouteExists(cn1_.get(), "blue", BuildPrefix());
-    VerifyInetRouteNoExists(cn2_.get(), "blue", BuildPrefix());
+    VerifyInetRouteExists(cn2_.get(), "blue", BuildPrefix());
 
     DeleteInetRoute(mx_.get(), NULL, "blue", BuildPrefix());
 }