Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upstream: fix invalid access of ClusterMap iterator during warming cluster modification #8106

Merged
merged 3 commits into from
Sep 3, 2019
Merged

upstream: fix invalid access of ClusterMap iterator during warming cluster modification #8106

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e3db26
Fix invalid access of a ClusterMap iterator when updating a warming c…
AndresGuedez Aug 30, 2019
38e515a
Avoid removing warming clusters from the init helper.
AndresGuedez Aug 30, 2019
cf6ffcc
Add comment regarding compiler optimization of assertion loop.
AndresGuedez Aug 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion source/common/upstream/cluster_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow where we add a cluster to init helper that isn't from active_clusters_.. do you have some code pointers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clusters are loaded into the warming_clusters_ map once all clusters are initialized. See https://github.com/envoyproxy/envoy/blob/master/source/common/upstream/cluster_manager_impl.cc#L508-L510.

When a modification to a cluster is attempted prior to the cluster moving to the active_clusters_ map, the undefined behavior access fixed above is encountered.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, but not spotting where this is added to init_helper_ after this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dug deeper based on your feedback and verified that warming clusters are not added to the init_helper_. I've modified the logic accordingly.

? 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();
Expand Down
72 changes: 72 additions & 0 deletions test/common/upstream/cluster_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<MockClusterMockPrioritySet> cluster1 =
std::make_shared<NiceMock<MockClusterMockPrioritySet>>();
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<MockClusterMockPrioritySet> cluster2 =
std::make_shared<NiceMock<MockClusterMockPrioritySet>>();
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());
Expand Down
18 changes: 10 additions & 8 deletions test/common/upstream/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,26 @@ 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",
"type": "static",
"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
Expand Down