From dbce53f1dd11c4eb2b4ff6e3609c9d7d04ef6159 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 | 30 +++----- src/bgp/routing-instance/peer_manager.cc | 18 +---- src/bgp/routing-instance/peer_manager.h | 4 - src/bgp/test/bgp_config_test.cc | 95 ++++++++++++++++++++---- 7 files changed, 122 insertions(+), 72 deletions(-) diff --git a/src/bgp/bgp_server.cc b/src/bgp/bgp_server.cc index 11ca3e23555..9634aa2d458 100644 --- a/src/bgp/bgp_server.cc +++ b/src/bgp/bgp_server.cc @@ -436,35 +436,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 0d595a3cebb..2f1fb131b12 100644 --- a/src/bgp/bgp_server.h +++ b/src/bgp/bgp_server.h @@ -64,11 +64,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(); @@ -240,7 +240,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); @@ -268,7 +268,7 @@ class BgpServer { tbb::atomic num_up_bgpaas_peer_; tbb::atomic closing_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 4c8aef3ba07..eae8d923277 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 1dd141a408b..425fc2fa72a 100644 --- a/src/bgp/bgp_show_neighbor.cc +++ b/src/bgp/bgp_show_neighbor.cc @@ -15,30 +15,24 @@ 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); - - BgpPeer *peer = bsc->bgp_server->FindNextPeer(); - while (peer) { - if (search_string.empty() || - (peer->peer_basename().find(search_string) != string::npos) || - (peer->peer_address_string().find(search_string) != string::npos) || - (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 (!search_string.empty() && + (peer->peer_basename().find(search_string) == string::npos) && + (peer->peer_address_string().find(search_string) == string::npos) && + (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 cc0b2f88f7f..6ac8e84b282 100644 --- a/src/bgp/routing-instance/peer_manager.cc +++ b/src/bgp/routing-instance/peer_manager.cc @@ -55,7 +55,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. @@ -259,19 +259,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 { - BgpPeerKey key = BgpPeerKey(); - while (const BgpPeer *peer = NextPeer(key)) { - if (search_string.empty() || - (peer->peer_basename().find(search_string) != string::npos) || - (peer->peer_address_string().find(search_string) != string::npos) || - (search_string == "deleted" && peer->IsDeleted())) { - BgpNeighborResp bnr; - peer->FillNeighborInfo(bsc, &bnr, summary); - bnr_list->push_back(bnr); - } - } -} diff --git a/src/bgp/routing-instance/peer_manager.h b/src/bgp/routing-instance/peer_manager.h index 248c8e5481c..5bdb3e9ee93 100644 --- a/src/bgp/routing-instance/peer_manager.h +++ b/src/bgp/routing-instance/peer_manager.h @@ -41,10 +41,6 @@ class PeerManager { virtual BgpPeer *NextPeer(BgpPeerKey &key); virtual const BgpPeer *NextPeer(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 2c2ec0c8112..8a89e78de20 100644 --- a/src/bgp/test/bgp_config_test.cc +++ b/src/bgp/test/bgp_config_test.cc @@ -432,7 +432,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)); @@ -459,9 +459,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)); @@ -473,12 +470,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"); @@ -488,8 +484,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())); @@ -722,7 +791,7 @@ TEST_F(BgpConfigTest, BGPaaSNeighbors7) { 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();