From aa776f8880c97522fd7b4b1dc41a45baecd383e4 Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Fri, 3 Mar 2017 09:36:02 -0800 Subject: [PATCH] Make BGPaaS config processing more defensive Consider the following scenario: Peer1 has source port p1 Peer2 has source port p2 Configuration for Peer1 is changed to have port p2 i.e. both Peer1 and Peer2 have port p2. Existing implementation removes Peer2 from it's port to peer mapping table under the assumption that the port for Peer2 will soon change to a new value. However, if the port for Peer1 is now changed back to p1, the port to peer mapping table no longer has an entry that maps p2 to Peer2. Fix by using a multimap instead of map to maintain port to peer mapping. It's expected that duplicate port values may only be seen in transient situations when the ST is being restarted and/or has a bug. This change lets the CN get to a sane configuration once any configuration issues are fixed. Change-Id: If48e4c9648d7ea5a8a121c1a46e7f10ce7451936 Closes-Bug: 1669871 --- src/bgp/bgp_server.cc | 37 +++++---- src/bgp/bgp_server.h | 8 +- src/bgp/bgp_session_manager.cc | 2 +- src/bgp/bgp_show_neighbor.cc | 29 +++----- src/bgp/routing-instance/peer_manager.cc | 19 +---- src/bgp/routing-instance/peer_manager.h | 5 -- src/bgp/test/bgp_config_test.cc | 95 ++++++++++++++++++++---- 7 files changed, 122 insertions(+), 73 deletions(-) diff --git a/src/bgp/bgp_server.cc b/src/bgp/bgp_server.cc index 99085bca71f..8b08a9721ac 100644 --- a/src/bgp/bgp_server.cc +++ b/src/bgp/bgp_server.cc @@ -484,35 +484,42 @@ BgpPeer *BgpServer::FindPeer(const string &name) { return (loc != peer_list_.end() ? loc->second : NULL); } +BgpPeer *BgpServer::FindNextPeer(const string &name) { + BgpPeerList::iterator loc = peer_list_.upper_bound(name); + return (loc != peer_list_.end() ? loc->second : NULL); +} + void BgpServer::InsertPeer(TcpSession::Endpoint remote, BgpPeer *peer) { if (!remote.port() && remote.address().is_unspecified()) return; - - EndpointToBgpPeerList::iterator loc = endpoint_peer_list_.find(remote); - if (loc != endpoint_peer_list_.end()) { - loc->second->Clear(BgpProto::Notification::PeerDeconfigured); - endpoint_peer_list_.erase(loc); - } endpoint_peer_list_.insert(make_pair(remote, peer)); } void BgpServer::RemovePeer(TcpSession::Endpoint remote, BgpPeer *peer) { - EndpointToBgpPeerList::iterator loc = endpoint_peer_list_.find(remote); - if (loc != endpoint_peer_list_.end() && loc->second == peer) { - endpoint_peer_list_.erase(loc); + EndpointPeerList::iterator loc = endpoint_peer_list_.lower_bound(remote); + while (loc != endpoint_peer_list_.end() && loc->first == remote) { + if (loc->second == peer) { + endpoint_peer_list_.erase(loc); + break; + } + ++loc; } } BgpPeer *BgpServer::FindPeer(TcpSession::Endpoint remote) const { - EndpointToBgpPeerList::const_iterator loc = - endpoint_peer_list_.find(remote); + EndpointPeerList::const_iterator loc = endpoint_peer_list_.find(remote); return (loc == endpoint_peer_list_.end() ? NULL : loc->second); } -BgpPeer *BgpServer::FindNextPeer(TcpSession::Endpoint remote) const { - EndpointToBgpPeerList::const_iterator loc = - endpoint_peer_list_.upper_bound(remote); - return (loc == endpoint_peer_list_.end() ? NULL : loc->second); +BgpPeer *BgpServer::FindExactPeer(const BgpPeer *peer) const { + EndpointPeerList::const_iterator loc = + endpoint_peer_list_.lower_bound(peer->endpoint()); + while (loc != endpoint_peer_list_.end() && loc->first == peer->endpoint()) { + if (loc->second == peer) + return loc->second; + ++loc; + } + return NULL; } const string &BgpServer::localname() const { diff --git a/src/bgp/bgp_server.h b/src/bgp/bgp_server.h index 373a39c0d8d..9797d222af9 100644 --- a/src/bgp/bgp_server.h +++ b/src/bgp/bgp_server.h @@ -73,11 +73,11 @@ class BgpServer { int RegisterPeer(BgpPeer *peer); void UnregisterPeer(BgpPeer *peer); BgpPeer *FindPeer(const std::string &name); + BgpPeer *FindNextPeer(const std::string &name = std::string()); void InsertPeer(TcpSession::Endpoint remote, BgpPeer *peer); void RemovePeer(TcpSession::Endpoint remote, BgpPeer *peer); BgpPeer *FindPeer(TcpSession::Endpoint remote) const; - BgpPeer *FindNextPeer( - TcpSession::Endpoint remote = TcpSession::Endpoint()) const; + BgpPeer *FindExactPeer(const BgpPeer *peer) const; void Shutdown(); @@ -263,7 +263,7 @@ class BgpServer { typedef std::vector AdminDownListenersList; typedef std::vector ASNUpdateListenersList; typedef std::vector IdentifierUpdateListenersList; - typedef std::map EndpointToBgpPeerList; + typedef std::multimap EndpointPeerList; void RoutingInstanceMgrDeletionComplete(RoutingInstanceMgr *mgr); void FillPeerStats(const BgpPeer *peer) const; @@ -293,7 +293,7 @@ class BgpServer { tbb::atomic num_up_bgpaas_peer_; tbb::atomic deleting_bgpaas_count_; BgpPeerList peer_list_; - EndpointToBgpPeerList endpoint_peer_list_; + EndpointPeerList endpoint_peer_list_; boost::scoped_ptr lifetime_manager_; boost::scoped_ptr deleter_; diff --git a/src/bgp/bgp_session_manager.cc b/src/bgp/bgp_session_manager.cc index cf4f44a9987..40cc8a0fabe 100644 --- a/src/bgp/bgp_session_manager.cc +++ b/src/bgp/bgp_session_manager.cc @@ -99,7 +99,7 @@ bool BgpSessionManager::ProcessWriteReady(TcpSessionPtr tcp_session) { // // Search for a matching BgpPeer. // First look for a matching address in the master instance. -// Then look for a matching port in the EndpointToBgpPeerList in BgpServer. +// Then look for a matching port in the EndpointPeerList in BgpServer. // BgpPeer *BgpSessionManager::FindPeer(Endpoint remote) { BgpPeer *peer = NULL; diff --git a/src/bgp/bgp_show_neighbor.cc b/src/bgp/bgp_show_neighbor.cc index 6eaa26d183f..4637dbdd067 100644 --- a/src/bgp/bgp_show_neighbor.cc +++ b/src/bgp/bgp_show_neighbor.cc @@ -18,30 +18,25 @@ using std::string; using std::vector; // -// Build the list of BgpPeers in one shot for now. Look at the master instance -// and at all peers in the BgpServer's EndpointToBgpPeerList. The latter is the -// list of all bgpaas peers in non-master instances. +// Build the list of BgpPeers in one shot for now. // static bool FillBgpNeighborInfoList(const BgpSandeshContext *bsc, bool summary, uint32_t page_limit, uint32_t iter_limit, const string &start_neighbor, const string &search_string, vector *show_list, string *next_instance) { - const RoutingInstanceMgr *rim = bsc->bgp_server->routing_instance_mgr(); - const RoutingInstance *rtinstance = rim->GetDefaultRoutingInstance(); - rtinstance->peer_manager()->FillBgpNeighborInfo( - bsc, show_list, search_string, summary); - regex search_expr(search_string); - const BgpPeer *peer = bsc->bgp_server->FindNextPeer(); - while (peer) { - if ((regex_search(peer->peer_basename(), search_expr)) || - (regex_search(peer->peer_address_string(), search_expr)) || - (search_string == "deleted" && peer->IsDeleted())) { - BgpNeighborResp bnr; - peer->FillNeighborInfo(bsc, &bnr, summary); - show_list->push_back(bnr); + + for (const BgpPeer *peer = bsc->bgp_server->FindNextPeer(); peer != NULL; + peer = bsc->bgp_server->FindNextPeer(peer->peer_name())) { + if ((!regex_search(peer->peer_basename(), search_expr)) && + (!regex_search(peer->peer_address_string(), search_expr)) && + (search_string != "deleted" || !peer->IsDeleted())) { + continue; } - peer = bsc->bgp_server->FindNextPeer(peer->endpoint()); + + BgpNeighborResp bnr; + peer->FillNeighborInfo(bsc, &bnr, summary); + show_list->push_back(bnr); } return true; } diff --git a/src/bgp/routing-instance/peer_manager.cc b/src/bgp/routing-instance/peer_manager.cc index d1b743f64fa..6769aa9d90e 100644 --- a/src/bgp/routing-instance/peer_manager.cc +++ b/src/bgp/routing-instance/peer_manager.cc @@ -59,7 +59,7 @@ BgpPeer *PeerManager::PeerLocate(BgpServer *server, // // Resurrect the BgpPeer with given name if we have configuration for it. -// Also insert it into the BgpServer's EndpointToBgpPeerList - in case it's +// Also insert it into the BgpServer's EndpointPeerList - in case it is // a BGPaaS peer. This needs to happen here since we would not have been // able to do it from BgpServer::ConfigUpdater::ProcessNeighborConfig since // old incarnation of the BgpPeer still existed at that point. @@ -260,20 +260,3 @@ const string &PeerManager::name() const { BgpServer *PeerManager::server() const { return instance_->server(); } - -void PeerManager::FillBgpNeighborInfo(const BgpSandeshContext *bsc, - vector *bnr_list, const string &search_string, - bool summary) const { - regex search_expr(search_string); - BgpPeerKey key = BgpPeerKey(); - while (const BgpPeer *peer = NextPeer(key)) { - if ((regex_search(peer->peer_basename(), search_expr)) || - (regex_search(peer->peer_address_string(), search_expr)) || - (search_string == "deleted" && peer->IsDeleted())) { - BgpNeighborResp bnr; - peer->FillNeighborInfo(bsc, &bnr, summary); - bnr_list->push_back(bnr); - } - key = peer->peer_key(); - } -} diff --git a/src/bgp/routing-instance/peer_manager.h b/src/bgp/routing-instance/peer_manager.h index fc0a9013afd..a36c40d92a1 100644 --- a/src/bgp/routing-instance/peer_manager.h +++ b/src/bgp/routing-instance/peer_manager.h @@ -40,11 +40,6 @@ class PeerManager { void ClearAllPeers(); const BgpPeer *NextPeer(const BgpPeerKey &key) const; - - void FillBgpNeighborInfo(const BgpSandeshContext *bsc, - std::vector *sbnr_list, - const std::string &search_string, bool summary) const; - size_t GetNeighborCount(std::string up_or_down); size_t size() { return peers_by_key_.size(); } diff --git a/src/bgp/test/bgp_config_test.cc b/src/bgp/test/bgp_config_test.cc index 1b9645e5313..1fca068a43e 100644 --- a/src/bgp/test/bgp_config_test.cc +++ b/src/bgp/test/bgp_config_test.cc @@ -521,7 +521,7 @@ TEST_F(BgpConfigTest, BGPaaSNeighbors2) { TASK_UTIL_EXPECT_EQ(0, server_.num_bgpaas_peer()); } -TEST_F(BgpConfigTest, BGPaaSNeighbors3) { +TEST_F(BgpConfigTest, BGPaaSNeighbors3a) { string content; content = FileRead("controller/src/bgp/testdata/config_test_36a.xml"); EXPECT_TRUE(parser_.Parse(content)); @@ -548,9 +548,6 @@ TEST_F(BgpConfigTest, BGPaaSNeighbors3) { TASK_UTIL_EXPECT_EQ(1025, peer2->peer_port()); TASK_UTIL_EXPECT_EQ(peer2, server_.FindPeer(peer2->endpoint())); - // Save the old endpoint for peer1. - TcpSession::Endpoint old_peer1_endpoint = peer1->endpoint(); - // Set test::vm1 port to be same as port for test:vm2. content = FileRead("controller/src/bgp/testdata/config_test_36c.xml"); EXPECT_TRUE(parser_.Parse(content)); @@ -562,12 +559,11 @@ TEST_F(BgpConfigTest, BGPaaSNeighbors3) { // Verify that the port is identical for test:vm2. TASK_UTIL_EXPECT_EQ(1025, peer2->peer_port()); - // Verify that test:vm1 is inserted into BgpServer::EndpointToBgpPeerList. - // Verify that there's no entry for the old remote endpoint for test:vm1. - // Note that test:vm2 is removed from BgpServer::EndpointToBgpPeerList when - // test:vm1 is inserted with the same remote endpoint. - TASK_UTIL_EXPECT_EQ(peer1, server_.FindPeer(peer1->endpoint())); - TASK_UTIL_EXPECT_TRUE(server_.FindPeer(old_peer1_endpoint) == NULL); + // Verify that test:vm1 is inserted into BgpServer::EndpointPeerList. + // Verify that test:vm2 is inserted into BgpServer::EndpointPeerList. + // Both peers have the same endpoint. + TASK_UTIL_EXPECT_EQ(peer1, server_.FindExactPeer(peer1)); + TASK_UTIL_EXPECT_EQ(peer2, server_.FindExactPeer(peer2)); // Set test::vm2 port to be same as old port for test:vm1. content = FileRead("controller/src/bgp/testdata/config_test_36d.xml"); @@ -577,8 +573,81 @@ TEST_F(BgpConfigTest, BGPaaSNeighbors3) { // Verify that the port is updated for test:vm2. TASK_UTIL_EXPECT_EQ(1024, peer2->peer_port()); - // Verify that test:vm1 is inserted into BgpServer::EndpointToBgpPeerList. - // Verify that test:vm2 is inserted into BgpServer::EndpointToBgpPeerList. + // Verify that test:vm1 is inserted into BgpServer::EndpointPeerList. + // Verify that test:vm2 is inserted into BgpServer::EndpointPeerList. + TASK_UTIL_EXPECT_EQ(peer1, server_.FindPeer(peer1->endpoint())); + TASK_UTIL_EXPECT_EQ(peer2, server_.FindPeer(peer2->endpoint())); + + // Cleanup. + boost::replace_all(content, "", ""); + boost::replace_all(content, "", ""); + EXPECT_TRUE(parser_.Parse(content)); + task_util::WaitForIdle(); + + // Ensure that the instance is deleted + TASK_UTIL_EXPECT_EQ(static_cast(NULL), + server_.routing_instance_mgr()->GetRoutingInstance("test")); + TASK_UTIL_EXPECT_EQ(0, server_.num_bgp_peer()); + TASK_UTIL_EXPECT_EQ(0, server_.num_bgpaas_peer()); +} + +TEST_F(BgpConfigTest, BGPaaSNeighbors3b) { + string content; + content = FileRead("controller/src/bgp/testdata/config_test_36a.xml"); + EXPECT_TRUE(parser_.Parse(content)); + task_util::WaitForIdle(); + + RoutingInstance *rti = + server_.routing_instance_mgr()->GetRoutingInstance("test"); + TASK_UTIL_ASSERT_TRUE(rti != NULL); + TASK_UTIL_EXPECT_EQ(2, rti->peer_manager_size()); + TASK_UTIL_EXPECT_EQ(0, server_.num_bgp_peer()); + TASK_UTIL_EXPECT_EQ(2, server_.num_bgpaas_peer()); + + // Verify that the port is set for test:vm1. + TASK_UTIL_EXPECT_TRUE( + rti->peer_manager()->PeerLookup("test:vm1:0") != NULL); + BgpPeer *peer1 = rti->peer_manager()->PeerLookup("test:vm1:0"); + TASK_UTIL_EXPECT_EQ(1024, peer1->peer_port()); + TASK_UTIL_EXPECT_EQ(peer1, server_.FindPeer(peer1->endpoint())); + + // Verify that the port is set for test:vm2. + TASK_UTIL_EXPECT_TRUE( + rti->peer_manager()->PeerLookup("test:vm2:0") != NULL); + BgpPeer *peer2 = rti->peer_manager()->PeerLookup("test:vm2:0"); + TASK_UTIL_EXPECT_EQ(1025, peer2->peer_port()); + TASK_UTIL_EXPECT_EQ(peer2, server_.FindPeer(peer2->endpoint())); + + // Set test::vm1 port to be same as port for test:vm2. + content = FileRead("controller/src/bgp/testdata/config_test_36c.xml"); + EXPECT_TRUE(parser_.Parse(content)); + task_util::WaitForIdle(); + + // Verify that the port is updated for test:vm1. + TASK_UTIL_EXPECT_EQ(1025, peer1->peer_port()); + + // Verify that the port is identical for test:vm2. + TASK_UTIL_EXPECT_EQ(1025, peer2->peer_port()); + + // Verify that test:vm1 is inserted into BgpServer::EndpointPeerList. + // Verify that test:vm2 is inserted into BgpServer::EndpointPeerList. + // Both peers have the same endpoint. + TASK_UTIL_EXPECT_EQ(peer1, server_.FindExactPeer(peer1)); + TASK_UTIL_EXPECT_EQ(peer2, server_.FindExactPeer(peer2)); + + // Set test::vm1 port back to be same as original port for test:vm1. + content = FileRead("controller/src/bgp/testdata/config_test_36a.xml"); + EXPECT_TRUE(parser_.Parse(content)); + task_util::WaitForIdle(); + + // Verify that the port is updated for test:vm1. + TASK_UTIL_EXPECT_EQ(1024, peer1->peer_port()); + + // Verify that the port is unchanged for test:vm2. + TASK_UTIL_EXPECT_EQ(1025, peer2->peer_port()); + + // Verify that test:vm1 is inserted into BgpServer::EndpointPeerList. + // Verify that test:vm2 is inserted into BgpServer::EndpointPeerList. TASK_UTIL_EXPECT_EQ(peer1, server_.FindPeer(peer1->endpoint())); TASK_UTIL_EXPECT_EQ(peer2, server_.FindPeer(peer2->endpoint())); @@ -922,7 +991,7 @@ TEST_F(BgpConfigTest, BGPaaSNeighbors9) { TASK_UTIL_EXPECT_TRUE(server_.FindPeer(peer2->endpoint()) == NULL); // Recreate neighbor config. The old peers should still be around - // but should not be in the BgpServer::EndpointToBgpPeerList. + // but should not be in the BgpServer::EndpointPeerList. content = FileRead("controller/src/bgp/testdata/config_test_36a.xml"); EXPECT_TRUE(parser_.Parse(content)); task_util::WaitForIdle();