From 9d0df2907eae554b0b778241580a1bd0476eff9c Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Fri, 12 Aug 2016 12:06:44 -0700 Subject: [PATCH] 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 | 247 ++++++++++++++++++++- src/bgp/xmpp_message_builder.cc | 10 + src/control-node/test/network_agent_mock.h | 2 +- 3 files changed, 249 insertions(+), 10 deletions(-) diff --git a/src/bgp/test/bgp_xmpp_inetvpn_test.cc b/src/bgp/test/bgp_xmpp_inetvpn_test.cc index 37ac3b33d21..968c1754496 100644 --- a/src/bgp/test/bgp_xmpp_inetvpn_test.cc +++ b/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 sgids, const LoadBalance &loadBalance, const vector 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(), LoadBalance(), - vector())); + local_pref, med, 0, string(), string(), vector(), + LoadBalance(), vector())); } void VerifyRouteExists(test::NetworkAgentMockPtr agent, string net, @@ -575,30 +577,38 @@ class BgpXmppInetvpn2ControlNodeTest : public ::testing::Test { const vector sgids = vector(), 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())); } void VerifyRouteExists(test::NetworkAgentMockPtr agent, string net, string prefix, string nexthop, const vector 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())); } + void VerifyRouteExists(test::NetworkAgentMockPtr agent, string net, + string prefix, string nexthop, int local_pref, int seq, + const vector sgids) { + TASK_UTIL_EXPECT_TRUE(CheckRoute( + agent, net, prefix, nexthop, local_pref, 0, seq, string(), string(), + sgids, LoadBalance(), vector())); + } + 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(), - lb, vector())); + agent, net, prefix, nexthop, 0, 0, 0, string(), string(), + vector(), lb, vector())); } void VerifyRouteExists(test::NetworkAgentMockPtr agent, string net, string prefix, string nexthop, const vector &communities) { TASK_UTIL_EXPECT_TRUE(CheckRoute( - agent, net, prefix, nexthop, 0, 0, string(), string(), vector(), - LoadBalance(), communities)); + agent, net, prefix, nexthop, 0, 0, 0, string(), string(), + vector(), 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 sgids1 = list_of(8000001)(8000003); + vector 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 diff --git a/src/bgp/xmpp_message_builder.cc b/src/bgp/xmpp_message_builder.cc index 2ab1a53429e..e08aa78e1ef 100644 --- a/src/bgp/xmpp_message_builder.cc +++ b/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) { diff --git a/src/control-node/test/network_agent_mock.h b/src/control-node/test/network_agent_mock.h index 80be3129470..da5b458d01e 100644 --- a/src/control-node/test/network_agent_mock.h +++ b/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 ¶ms)