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

@@ -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 {

src/bgp/bgp_server.h

@@ -75,11 +75,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);
     virtual BgpPeer *FindPeer(TcpSession::Endpoint remote) const;
-    BgpPeer *FindNextPeer(
-        TcpSession::Endpoint remote = TcpSession::Endpoint()) const;
+    BgpPeer *FindExactPeer(const BgpPeer *peer) const;
 
     void Shutdown();
 
@@ -265,7 +265,7 @@ class BgpServer {
     typedef std::vector<AdminDownCb> AdminDownListenersList;
     typedef std::vector<ASNUpdateCb> ASNUpdateListenersList;
     typedef std::vector<IdentifierUpdateCb> IdentifierUpdateListenersList;
-    typedef std::map<TcpSession::Endpoint, BgpPeer *> EndpointToBgpPeerList;
+    typedef std::multimap<TcpSession::Endpoint, BgpPeer *> EndpointPeerList;
 
     void RoutingInstanceMgrDeletionComplete(RoutingInstanceMgr *mgr);
     void FillPeerStats(const BgpPeer *peer) const;
@@ -295,7 +295,7 @@ class BgpServer {
     tbb::atomic<uint32_t> num_up_bgpaas_peer_;
     tbb::atomic<uint32_t> deleting_bgpaas_count_;
     BgpPeerList peer_list_;
-    EndpointToBgpPeerList endpoint_peer_list_;
+    EndpointPeerList endpoint_peer_list_;
 
     boost::scoped_ptr<LifetimeManager> lifetime_manager_;
     boost::scoped_ptr<DeleteActor> deleter_;

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;

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<BgpNeighborResp> *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;
 }

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<BgpNeighborResp> *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();
-    }
-}

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<BgpNeighborResp> *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(); }

src/bgp/test/bgp_config_test.cc

@@ -525,7 +525,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));
@@ -552,9 +552,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));
@@ -566,12 +563,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");
@@ -581,8 +577,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, "<config>", "<delete>");
+    boost::replace_all(content, "</config>", "</delete>");
+    EXPECT_TRUE(parser_.Parse(content));
+    task_util::WaitForIdle();
+
+    // Ensure that the instance is deleted
+    TASK_UTIL_EXPECT_EQ(static_cast<RoutingInstance *>(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()));
 
@@ -926,7 +995,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();