From 3e3db262d0ba39aceedfcbc1548972c87117e5c3 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 30 Aug 2019 13:12:13 -0400 Subject: [PATCH 1/3] Fix invalid access of a ClusterMap iterator when updating a warming cluster. Signed-off-by: Andres Guedez --- .../common/upstream/cluster_manager_impl.cc | 5 +- .../upstream/cluster_manager_impl_test.cc | 72 +++++++++++++++++++ test/common/upstream/utility.h | 18 ++--- 3 files changed, 86 insertions(+), 9 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index cad4bd02d679..8e75586dba43 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -487,9 +487,12 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust if (existing_active_cluster != active_clusters_.end() || existing_warming_cluster != warming_clusters_.end()) { + const auto cluster_it = (existing_active_cluster != active_clusters_.end()) + ? existing_active_cluster + : existing_warming_cluster; // The following init manager remove call is a NOP in the case we are already initialized. It's // just kept here to avoid additional logic. - init_helper_.removeCluster(*existing_active_cluster->second->cluster_); + init_helper_.removeCluster(*cluster_it->second->cluster_); cm_stats_.cluster_modified_.inc(); } else { cm_stats_.cluster_added_.inc(); diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index f9632b12da76..6fbc2e5afea2 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -1234,6 +1234,78 @@ TEST_F(ClusterManagerImplTest, RemoveWarmingCluster) { EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster1.get())); } +TEST_F(ClusterManagerImplTest, ModifyWarmingCluster) { + time_system_.setSystemTime(std::chrono::milliseconds(1234567891234)); + create(defaultConfig()); + + InSequence s; + ReadyWatcher initialized; + EXPECT_CALL(initialized, ready()); + cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); + + // Add a "fake_cluster" in warming state. + std::shared_ptr cluster1 = + std::make_shared>(); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)) + .WillOnce(Return(std::make_pair(cluster1, nullptr))); + EXPECT_CALL(*cluster1, initializePhase()).Times(0); + EXPECT_CALL(*cluster1, initialize(_)); + EXPECT_TRUE( + cluster_manager_->addOrUpdateCluster(defaultStaticCluster("fake_cluster"), "version1")); + checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 0 /*active*/, 1 /*warming*/); + EXPECT_EQ(nullptr, cluster_manager_->get("fake_cluster")); + checkConfigDump(R"EOF( + dynamic_warming_clusters: + - version_info: "version1" + cluster: + name: "fake_cluster" + type: STATIC + connect_timeout: 0.25s + hosts: + - socket_address: + address: "127.0.0.1" + port_value: 11001 + last_updated: + seconds: 1234567891 + nanos: 234000000 + )EOF"); + + // Update the warming cluster that was just added. + std::shared_ptr cluster2 = + std::make_shared>(); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)) + .WillOnce(Return(std::make_pair(cluster2, nullptr))); + EXPECT_CALL(*cluster2, initializePhase()).Times(0); + EXPECT_CALL(*cluster2, initialize(_)); + EXPECT_TRUE(cluster_manager_->addOrUpdateCluster( + parseClusterFromV2Json(fmt::sprintf(kDefaultStaticClusterTmpl, "fake_cluster", + R"EOF( +"socket_address": { + "address": "127.0.0.1", + "port_value": 11002 +})EOF")), + "version2")); + checkStats(1 /*added*/, 1 /*modified*/, 0 /*removed*/, 0 /*active*/, 1 /*warming*/); + checkConfigDump(R"EOF( + dynamic_warming_clusters: + - version_info: "version2" + cluster: + name: "fake_cluster" + type: STATIC + connect_timeout: 0.25s + hosts: + - socket_address: + address: "127.0.0.1" + port_value: 11002 + last_updated: + seconds: 1234567891 + nanos: 234000000 + )EOF"); + + EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster1.get())); + EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster2.get())); +} + // Verify that shutting down the cluster manager destroys warming clusters. TEST_F(ClusterManagerImplTest, ShutdownWithWarming) { create(defaultConfig()); diff --git a/test/common/upstream/utility.h b/test/common/upstream/utility.h index b5da2071d09b..b41b9cbfd280 100644 --- a/test/common/upstream/utility.h +++ b/test/common/upstream/utility.h @@ -15,8 +15,7 @@ namespace Envoy { namespace Upstream { namespace { -inline std::string defaultStaticClusterJson(const std::string& name) { - return fmt::sprintf(R"EOF( +constexpr static const char* kDefaultStaticClusterTmpl = R"EOF( { "name": "%s", "connect_timeout": "0.250s", @@ -24,15 +23,18 @@ inline std::string defaultStaticClusterJson(const std::string& name) { "lb_policy": "round_robin", "hosts": [ { - "socket_address": { - "address": "127.0.0.1", - "port_value": 11001 - } + %s, } ] } - )EOF", - name); + )EOF"; + +inline std::string defaultStaticClusterJson(const std::string& name) { + return fmt::sprintf(kDefaultStaticClusterTmpl, name, R"EOF( +"socket_address": { + "address": "127.0.0.1", + "port_value": 11001 +})EOF"); } inline envoy::config::bootstrap::v2::Bootstrap From 38e515af6938ef48727a5c6ba702c0cbbbe26301 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 30 Aug 2019 14:53:56 -0400 Subject: [PATCH 2/3] Avoid removing warming clusters from the init helper. This properly fixes the invalid iterator access whe removing clusters from the init helper. Warming clusters are never added to the init helper; added an assertion to validate this invariant. Signed-off-by: Andres Guedez --- .../common/upstream/cluster_manager_impl.cc | 21 +++++++++++++------ source/common/upstream/cluster_manager_impl.h | 6 ++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 8e75586dba43..806999febd3c 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -487,12 +487,21 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust if (existing_active_cluster != active_clusters_.end() || existing_warming_cluster != warming_clusters_.end()) { - const auto cluster_it = (existing_active_cluster != active_clusters_.end()) - ? existing_active_cluster - : existing_warming_cluster; - // The following init manager remove call is a NOP in the case we are already initialized. It's - // just kept here to avoid additional logic. - init_helper_.removeCluster(*cluster_it->second->cluster_); + if (existing_active_cluster != active_clusters_.end()) { + // The following init manager remove call is a NOP in the case we are already initialized. + // It's just kept here to avoid additional logic. + init_helper_.removeCluster(*existing_active_cluster->second->cluster_); + } else { + // Validate that warming clusters are not added to the init_helper_. + for (const std::list& cluster_list : + {std::cref(init_helper_.primary_init_clusters_), + std::cref(init_helper_.secondary_init_clusters_)}) { + ASSERT(!std::any_of(cluster_list.begin(), cluster_list.end(), + [&existing_warming_cluster](Cluster* cluster) { + return existing_warming_cluster->second->cluster_.get() == cluster; + })); + } + } cm_stats_.cluster_modified_.inc(); } else { cm_stats_.cluster_added_.inc(); diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index e45e9752372f..cb45bb14aca7 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -90,6 +90,9 @@ class ProdClusterManagerFactory : public ClusterManagerFactory { Singleton::Manager& singleton_manager_; }; +// For friend declaration in ClusterManagerInitHelper. +class ClusterManagerImpl; + /** * This is a helper class used during cluster management initialization. Dealing with primary * clusters, secondary clusters, and CDS, is quite complicated, so this makes it easier to test. @@ -129,6 +132,9 @@ class ClusterManagerInitHelper : Logger::Loggable { State state() const { return state_; } private: + // To enable invariant assertions on the cluster lists. + friend ClusterManagerImpl; + void initializeSecondaryClusters(); void maybeFinishInitialize(); void onClusterInit(Cluster& cluster); From cf6ffcc0ce601885aa2ee7c17e52602c3448ed01 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 30 Aug 2019 21:10:46 -0400 Subject: [PATCH 3/3] Add comment regarding compiler optimization of assertion loop. Signed-off-by: Andres Guedez --- source/common/upstream/cluster_manager_impl.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 806999febd3c..8a1b32cb90d8 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -493,6 +493,7 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust init_helper_.removeCluster(*existing_active_cluster->second->cluster_); } else { // Validate that warming clusters are not added to the init_helper_. + // NOTE: This loop is compiled out in optimized builds. for (const std::list& cluster_list : {std::cref(init_helper_.primary_init_clusters_), std::cref(init_helper_.secondary_init_clusters_)}) {