Retain unique route attributes when packing into a xmpp message

This bug is a regression introduced by changes made for bug 1591399.

Since an xmpp message now contains routes with different attributes,
Community and ExtCommunity need to processed for each route that is
added to the message.  Otherwise, all routes that get packed into a
message will accidentally be advertised with some of the attributes
of the first route in the message.

Change-Id: Id34e32e3ff106e97c2536b1d0a72772387ca6a76
Closes-Bug: 1612766

src/bgp/test/bgp_xmpp_inetvpn_test.cc

@@ -531,7 +531,7 @@ class BgpXmppInetvpn2ControlNodeTest : public ::testing::Test {
     }
 
     bool CheckRoute(test::NetworkAgentMockPtr agent, string net,
-        string prefix, string nexthop, int local_pref, int med,
+        string prefix, string nexthop, int local_pref, int med, int seq,
         const string &encap, const string &origin_vn, const vector<int> sgids,
         const LoadBalance &loadBalance, const vector<string> communities) {
         const autogen::ItemType *rt = agent->RouteLookup(net, prefix);
@@ -543,6 +543,8 @@ class BgpXmppInetvpn2ControlNodeTest : public ::testing::Test {
             return false;
         if (med && rt->entry.med != med)
             return false;
+        if (seq && rt->entry.sequence_number != seq)
+            return false;
         if (!origin_vn.empty() && rt->entry.virtual_network != origin_vn)
             return false;
         if (!sgids.empty() &&
@@ -565,8 +567,8 @@ class BgpXmppInetvpn2ControlNodeTest : public ::testing::Test {
     void VerifyRouteExists(test::NetworkAgentMockPtr agent, string net,
         string prefix, string nexthop, int local_pref, int med) {
         TASK_UTIL_EXPECT_TRUE(CheckRoute(agent, net, prefix, nexthop,
-            local_pref, med, string(), string(), vector<int>(), LoadBalance(),
-            vector<string>()));
+            local_pref, med, 0, string(), string(), vector<int>(),
+            LoadBalance(), vector<string>()));
     }
 
     void VerifyRouteExists(test::NetworkAgentMockPtr agent, string net,
@@ -575,30 +577,38 @@ class BgpXmppInetvpn2ControlNodeTest : public ::testing::Test {
         const vector<int> sgids = vector<int>(),
         const LoadBalance lb = LoadBalance()) {
         TASK_UTIL_EXPECT_TRUE(CheckRoute(
-            agent, net, prefix, nexthop, local_pref, 0, encap, origin_vn,
+            agent, net, prefix, nexthop, local_pref, 0, 0, encap, origin_vn,
             sgids, lb, vector<string>()));
     }
 
     void VerifyRouteExists(test::NetworkAgentMockPtr agent, string net,
         string prefix, string nexthop, const vector<int> sgids) {
         TASK_UTIL_EXPECT_TRUE(CheckRoute(
-            agent, net, prefix, nexthop, 0, 0, string(), string(), sgids,
+            agent, net, prefix, nexthop, 0, 0, 0, string(), string(), sgids,
             LoadBalance(), vector<string>()));
     }
 
+    void VerifyRouteExists(test::NetworkAgentMockPtr agent, string net,
+        string prefix, string nexthop, int local_pref, int seq,
+        const vector<int> sgids) {
+        TASK_UTIL_EXPECT_TRUE(CheckRoute(
+            agent, net, prefix, nexthop, local_pref, 0, seq, string(), string(),
+            sgids, LoadBalance(), vector<string>()));
+    }
+
     void VerifyRouteExists(test::NetworkAgentMockPtr agent, string net,
         string prefix, string nexthop, const LoadBalance &lb) {
         TASK_UTIL_EXPECT_TRUE(CheckRoute(
-            agent, net, prefix, nexthop, 0, 0, string(), string(), vector<int>(),
-            lb, vector<string>()));
+            agent, net, prefix, nexthop, 0, 0, 0, string(), string(),
+            vector<int>(), lb, vector<string>()));
     }
 
 
     void VerifyRouteExists(test::NetworkAgentMockPtr agent, string net,
         string prefix, string nexthop, const vector<string> &communities) {
         TASK_UTIL_EXPECT_TRUE(CheckRoute(
-            agent, net, prefix, nexthop, 0, 0, string(), string(), vector<int>(),
-            LoadBalance(), communities));
+            agent, net, prefix, nexthop, 0, 0, 0, string(), string(),
+            vector<int>(), LoadBalance(), communities));
     }
 
     void VerifyRouteNoExists(test::NetworkAgentMockPtr agent, string net,
@@ -1537,6 +1547,7 @@ TEST_F(BgpXmppInetvpn2ControlNodeTest, MultipleRouteAddDelete3) {
 // Large xmpp update messages even though attributes for each route are
 // different.
 // Small bgp update messages since attributes for each route are different.
+// Each route has a different local pref and med.
 //
 TEST_F(BgpXmppInetvpn2ControlNodeTest, MultipleRouteAddDelete4) {
     Configure();
@@ -1878,6 +1889,224 @@ TEST_F(BgpXmppInetvpn2ControlNodeTest, MultipleRouteAddDelete5) {
     agent_b_->SessionDown();
 }
 
+//
+// Multiple routes are exchanged correctly.
+// Large xmpp update messages even though attributes for each route are
+// different.
+// Small bgp update messages since attributes for each route are different.
+// Each route has a different local pref and sequence number.
+// Odd and even routes have different security groups.
+//
+TEST_F(BgpXmppInetvpn2ControlNodeTest, MultipleRouteAddDelete6) {
+    Configure();
+    task_util::WaitForIdle();
+
+    // Create XMPP Agent A connected to XMPP server X.
+    agent_a_.reset(
+        new test::NetworkAgentMock(&evm_, "agent-a", xs_x_->GetPort(),
+            "127.0.0.1", "127.0.0.1"));
+    TASK_UTIL_EXPECT_TRUE(agent_a_->IsEstablished());
+
+    // Create XMPP Agent B connected to XMPP server Y.
+    agent_b_.reset(
+        new test::NetworkAgentMock(&evm_, "agent-b", xs_y_->GetPort(),
+            "127.0.0.2", "127.0.0.2"));
+    TASK_UTIL_EXPECT_TRUE(agent_b_->IsEstablished());
+
+    // Verify table walk count for blue.inet.0.
+    BgpTable *blue_x = VerifyTableExists(bs_x_, "blue.inet.0");
+    BgpTable *blue_y = VerifyTableExists(bs_y_, "blue.inet.0");
+    TASK_UTIL_EXPECT_EQ(0, blue_x->walk_complete_count());
+    TASK_UTIL_EXPECT_EQ(0, blue_y->walk_complete_count());
+    task_util::WaitForIdle();
+
+    // Register to blue instance
+    agent_a_->Subscribe("blue", 1);
+    agent_b_->Subscribe("blue", 1);
+    task_util::WaitForIdle();
+
+    // Verify that subscribe completed.
+    TASK_UTIL_EXPECT_EQ(1, blue_x->walk_complete_count());
+    TASK_UTIL_EXPECT_EQ(1, blue_y->walk_complete_count());
+    task_util::WaitForIdle();
+
+    // Pause update generation on X.
+    bs_x_->scheduling_group_manager()->DisableGroups();
+
+    // Add routes from agent A.
+    test::NextHops next_hops;
+    test::NextHop next_hop("192.168.1.1");
+    next_hops.push_back(next_hop);
+    vector<int> sgids1 = list_of(8000001)(8000003);
+    vector<int> sgids2 = list_of(8000002)(8000004);
+    for (int idx = 0; idx < kRouteCount; ++idx) {
+        int lpref = idx + 1;
+        int seq = idx + 1;
+        if (idx % 2 == 0) {
+            test::RouteAttributes attributes(lpref, seq, sgids2);
+            agent_a_->AddRoute("blue", BuildPrefix(idx), next_hops, attributes);
+        } else {
+            test::RouteAttributes attributes(lpref, seq, sgids1);
+            agent_a_->AddRoute("blue", BuildPrefix(idx), next_hops, attributes);
+        }
+    }
+    task_util::WaitForIdle();
+
+    // Verify that blue.inet.0 and bgp.l3vpn.0 output queues have built up on X.
+    VerifyOutputQueueDepth(bs_x_, 2 * kRouteCount);
+    task_util::WaitForIdle();
+
+    // Pause update generation on Y.
+    bs_y_->scheduling_group_manager()->DisableGroups();
+
+    // Resume update generation on X.
+    bs_x_->scheduling_group_manager()->EnableGroups();
+    task_util::WaitForIdle();
+
+    // Verify that blue.inet.0 and bgp.l3vpn.0 output queues are drained on X.
+    VerifyOutputQueueDepth(bs_x_, 0);
+    task_util::WaitForIdle();
+
+    // Verify that blue.inet.0 output queues have built up on Y.
+    VerifyOutputQueueDepth(bs_y_, kRouteCount);
+    task_util::WaitForIdle();
+
+    // Resume update generation on Y.
+    bs_y_->scheduling_group_manager()->EnableGroups();
+    task_util::WaitForIdle();
+
+    // Verify that blue.inet.0 output queues are drained on Y.
+    VerifyOutputQueueDepth(bs_y_, 0);
+    task_util::WaitForIdle();
+
+    // Verify that routes showed up on agents A and B.
+    for (int idx = 0; idx < kRouteCount; ++idx) {
+        int lpref = idx + 1;
+        int seq = idx + 1;
+        if (idx % 2 == 0) {
+            VerifyRouteExists(agent_a_, "blue", BuildPrefix(idx), "192.168.1.1",
+                lpref, seq, sgids2);
+            VerifyRouteExists(agent_b_, "blue", BuildPrefix(idx), "192.168.1.1",
+                lpref, seq, sgids2);
+        } else {
+            VerifyRouteExists(agent_a_, "blue", BuildPrefix(idx), "192.168.1.1",
+                lpref, seq, sgids1);
+            VerifyRouteExists(agent_b_, "blue", BuildPrefix(idx), "192.168.1.1",
+                lpref, seq, sgids1);
+        }
+    }
+
+    // Pause update generation on server X.
+    bs_x_->scheduling_group_manager()->DisableGroups();
+
+    // Delete routes from agent A.
+    for (int idx = 0; idx < kRouteCount; ++idx) {
+        agent_a_->DeleteRoute("blue", BuildPrefix(idx));
+    }
+    task_util::WaitForIdle();
+
+    // Verify that blue.inet.0 and bgp.l3vpn.0 output queues have built up on X.
+    VerifyOutputQueueDepth(bs_x_, 2 * kRouteCount);
+    task_util::WaitForIdle();
+
+    // Pause update generation on Y.
+    bs_y_->scheduling_group_manager()->DisableGroups();
+
+    // Resume update generation on server X.
+    bs_x_->scheduling_group_manager()->EnableGroups();
+    task_util::WaitForIdle();
+
+    // Verify that blue.inet.0 and bgp.l3vpn.0 output queues are drained on X.
+    VerifyOutputQueueDepth(bs_x_, 0);
+
+    // Verify that blue.inet.0 output queues have built up on Y.
+    VerifyOutputQueueDepth(bs_y_, kRouteCount);
+    task_util::WaitForIdle();
+
+    // Resume update generation on Y.
+    bs_y_->scheduling_group_manager()->EnableGroups();
+    task_util::WaitForIdle();
+
+    // Verify that blue.inet.0 output queues are drained on Y.
+    VerifyOutputQueueDepth(bs_y_, 0);
+
+    // Verify that routes are deleted at agents A and B.
+    for (int idx = 0; idx < kRouteCount; ++idx) {
+        VerifyRouteNoExists(agent_a_, "blue", BuildPrefix(idx));
+        VerifyRouteNoExists(agent_b_, "blue", BuildPrefix(idx));
+    }
+
+    // Unregister to blue instance
+    agent_a_->Unsubscribe("blue");
+    agent_b_->Unsubscribe("blue");
+
+    // Verify bgp update counters.
+    const BgpPeer *peer_xy = VerifyPeerExists(bs_x_, bs_y_);
+    const BgpPeer *peer_yx = VerifyPeerExists(bs_y_, bs_x_);
+    TASK_UTIL_EXPECT_EQ(
+        peer_xy->get_rx_route_reach(), peer_yx->get_tx_route_reach());
+    TASK_UTIL_EXPECT_EQ(
+        peer_xy->get_tx_route_reach(), peer_yx->get_rx_route_reach());
+    TASK_UTIL_EXPECT_EQ(
+        peer_xy->get_rx_route_unreach(), peer_yx->get_tx_route_unreach());
+    TASK_UTIL_EXPECT_EQ(
+        peer_xy->get_tx_route_unreach(), peer_yx->get_rx_route_unreach());
+    TASK_UTIL_EXPECT_EQ(
+        peer_xy->get_rx_end_of_rib(), peer_yx->get_tx_end_of_rib());
+    TASK_UTIL_EXPECT_EQ(
+        peer_xy->get_tx_end_of_rib(), peer_yx->get_rx_end_of_rib());
+    TASK_UTIL_EXPECT_EQ(
+        peer_xy->get_rx_route_total(), peer_yx->get_tx_route_total());
+    TASK_UTIL_EXPECT_EQ(
+        peer_xy->get_tx_route_total(), peer_yx->get_rx_route_total());
+
+    // Verify bgp reach/unreach, end-of-rib and total counts.
+    // 1 route-target and kRouteCount inet-vpn routes.
+    // 2 end-of-ribs - one for route-target and one for inet-vpn.
+    TASK_UTIL_EXPECT_EQ(1 + kRouteCount, peer_xy->get_tx_route_reach());
+    TASK_UTIL_EXPECT_EQ(1 + kRouteCount, peer_xy->get_tx_route_unreach());
+    TASK_UTIL_EXPECT_EQ(2, peer_xy->get_tx_end_of_rib());
+    TASK_UTIL_EXPECT_EQ(
+        2 * (1 + kRouteCount) + 2, peer_xy->get_tx_route_total());
+
+    // Verify bgp update message counters.
+    // X->Y : 2 route-target (1 advertise, 1 withdraw) +
+    //        515 inet-vpn (512 advertise, 3 withdraw) +
+    //        2 end-of-rib (1 route-target, 1 inet-vpn)
+    // Y->X : 2 route-target (1 advertise, 1 withdraw) +
+    //        2 end-of-rib (1 route-target, 1 inet-vpn)
+    TASK_UTIL_EXPECT_EQ(peer_xy->get_rx_update(), peer_yx->get_tx_update());
+    TASK_UTIL_EXPECT_EQ(peer_xy->get_tx_update(), peer_yx->get_rx_update());
+    TASK_UTIL_EXPECT_EQ(4 + 3 + kRouteCount, peer_xy->get_tx_update());
+    TASK_UTIL_EXPECT_EQ(4, peer_yx->get_tx_update());
+
+    // Verify xmpp update counters.
+    const BgpXmppChannel *xc_a =
+        VerifyXmppChannelExists(cm_x_.get(), "agent-a");
+    const BgpXmppChannel *xc_b =
+        VerifyXmppChannelExists(cm_y_.get(), "agent-b");
+    TASK_UTIL_EXPECT_EQ(0 + kRouteCount, xc_a->get_rx_route_reach());
+    TASK_UTIL_EXPECT_EQ(0 + kRouteCount, xc_a->get_tx_route_reach());
+    TASK_UTIL_EXPECT_EQ(0 + kRouteCount, xc_a->get_rx_route_unreach());
+    TASK_UTIL_EXPECT_EQ(0 + kRouteCount, xc_a->get_tx_route_unreach());
+    TASK_UTIL_EXPECT_EQ(0, xc_b->get_rx_route_reach());
+    TASK_UTIL_EXPECT_EQ(0 + kRouteCount, xc_b->get_tx_route_reach());
+    TASK_UTIL_EXPECT_EQ(0, xc_b->get_rx_route_unreach());
+    TASK_UTIL_EXPECT_EQ(0 + kRouteCount, xc_b->get_tx_route_unreach());
+
+    // Verify xmpp update message counters.
+    // agent-a: 16 (kRouteCount/BgpXmppMessage::kMaxReachCount) advertise +
+    //           2 (kRouteCount/BgpXmppMessage::kMaxUnreachCount) withdraw
+    // agent-b: 16 (kRouteCount/BgpXmppMessage::kMaxReachCount) advertise +
+    //           2 (kRouteCount/BgpXmppMessage::kMaxUnreachCount) withdraw
+    TASK_UTIL_EXPECT_EQ(18, xc_a->get_tx_update());
+    TASK_UTIL_EXPECT_EQ(18, xc_b->get_tx_update());
+
+    // Close the sessions.
+    agent_a_->SessionDown();
+    agent_b_->SessionDown();
+}
+
 //
 // Generate big update message.
 // Each route has a very large number of attributes such that only a single

src/bgp/xmpp_message_builder.cc

@@ -87,6 +87,7 @@ class BgpXmppMessage : public Message {
     bool AddMcastRoute(const BgpRoute *route, const RibOutAttr *roattr);
 
     void ProcessCommunity(const Community *community) {
+        community_list_.clear();
         if (community == NULL)
             return;
         BOOST_FOREACH(uint32_t value, community->communities()) {
@@ -95,6 +96,9 @@ class BgpXmppMessage : public Message {
     }
 
     void ProcessExtCommunity(const ExtCommunity *ext_community) {
+        sequence_number_ = 0;
+        security_group_list_.clear();
+        load_balance_attribute_ = LoadBalance::LoadBalanceAttribute();
         if (ext_community == NULL)
             return;
 
@@ -176,6 +180,12 @@ bool BgpXmppMessage::AddRoute(const BgpRoute *route, const RibOutAttr *roattr) {
     if (!is_reachable_ && num_unreach_route_ >= kMaxUnreachCount)
         return false;
 
+    if (is_reachable_) {
+        const BgpAttr *attr = roattr->attr();
+        ProcessCommunity(attr->community());
+        ProcessExtCommunity(attr->ext_community());
+    }
+
     if (table_->family() == Address::ERMVPN) {
         return AddMcastRoute(route, roattr);
     } else if (table_->family() == Address::EVPN) {

src/control-node/test/network_agent_mock.h

@@ -106,7 +106,7 @@ struct RouteAttributes {
           communities(community) {
     }
     RouteAttributes(const LoadBalance::LoadBalanceAttribute &lba)
-        : local_pref(kDefaultLocalPref), sequence(kDefaultSequence),
+        : local_pref(kDefaultLocalPref), med(0), sequence(kDefaultSequence),
           loadBalanceAttribute(lba) {
     }
     RouteAttributes(const RouteParams &params)