upstream: remove thread local cluster after triggering call backs (en…

…voyproxy#8004) Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
zuercher · Aug 23, 2019 · 73c2b64 · 73c2b64
1 parent e958cf9
commit 73c2b64
Show file tree

Hide file tree

Showing 6 changed files with 81 additions and 2 deletions.
diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc
@@ -553,10 +553,10 @@ bool ClusterManagerImpl::removeCluster(const std::string& cluster_name) {
 
       ASSERT(cluster_manager.thread_local_clusters_.count(cluster_name) == 1);
       ENVOY_LOG(debug, "removing TLS cluster {}", cluster_name);
-      cluster_manager.thread_local_clusters_.erase(cluster_name);
       for (auto& cb : cluster_manager.update_callbacks_) {
         cb->onClusterRemoval(cluster_name);
       }
+      cluster_manager.thread_local_clusters_.erase(cluster_name);
     });
   }
 

diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc
@@ -1330,9 +1330,9 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) {
   // tcp connections.
   Http::ConnectionPool::Instance::DrainedCb drained_cb;
   Tcp::ConnectionPool::Instance::DrainedCb drained_cb2;
+  EXPECT_CALL(*callbacks, onClusterRemoval(_)).Times(1);
   EXPECT_CALL(*cp, addDrainedCallback(_)).WillOnce(SaveArg<0>(&drained_cb));
   EXPECT_CALL(*cp2, addDrainedCallback(_)).WillOnce(SaveArg<0>(&drained_cb2));
-  EXPECT_CALL(*callbacks, onClusterRemoval(_)).Times(1);
   EXPECT_TRUE(cluster_manager_->removeCluster("fake_cluster"));
   EXPECT_EQ(nullptr, cluster_manager_->get("fake_cluster"));
   EXPECT_EQ(0UL, cluster_manager_->clusters().size());

diff --git a/test/integration/BUILD b/test/integration/BUILD
@@ -33,6 +33,7 @@ envoy_cc_test_library(
         "//source/common/config:protobuf_link_hacks",
         "//source/common/config:resources_lib",
         "//source/common/protobuf:utility_lib",
+        "//source/extensions/filters/network/redis_proxy:config",
         "//test/common/grpc:grpc_client_integration_lib",
         "//test/test_common:network_utility_lib",
         "//test/test_common:utility_lib",

diff --git a/test/integration/ads_integration.cc b/test/integration/ads_integration.cc
@@ -46,6 +46,17 @@ envoy::api::v2::Cluster AdsIntegrationTest::buildCluster(const std::string& name
                                                                      name));
 }
 
+envoy::api::v2::Cluster AdsIntegrationTest::buildRedisCluster(const std::string& name) {
+  return TestUtility::parseYaml<envoy::api::v2::Cluster>(fmt::format(R"EOF(
+      name: {}
+      connect_timeout: 5s
+      type: EDS
+      eds_cluster_config: {{ eds_config: {{ ads: {{}} }} }}
+      lb_policy: MAGLEV
+    )EOF",
+                                                                     name));
+}
+
 envoy::api::v2::ClusterLoadAssignment
 AdsIntegrationTest::buildClusterLoadAssignment(const std::string& name) {
   return TestUtility::parseYaml<envoy::api::v2::ClusterLoadAssignment>(
@@ -87,6 +98,29 @@ envoy::api::v2::Listener AdsIntegrationTest::buildListener(const std::string& na
       name, Network::Test::getLoopbackAddressString(ipVersion()), stat_prefix, route_config));
 }
 
+envoy::api::v2::Listener AdsIntegrationTest::buildRedisListener(const std::string& name,
+                                                                const std::string& cluster) {
+  return TestUtility::parseYaml<envoy::api::v2::Listener>(fmt::format(
+      R"EOF(
+      name: {}
+      address:
+        socket_address:
+          address: {}
+          port_value: 0
+      filter_chains:
+        filters:
+        - name: envoy.redis_proxy
+          config:
+            settings: 
+              op_timeout: 1s
+            stat_prefix: {}
+            prefix_routes:
+              catch_all_route: 
+                cluster: {}
+    )EOF",
+      name, Network::Test::getLoopbackAddressString(ipVersion()), name, cluster));
+}
+
 envoy::api::v2::RouteConfiguration
 AdsIntegrationTest::buildRouteConfig(const std::string& name, const std::string& cluster) {
   return TestUtility::parseYaml<envoy::api::v2::RouteConfiguration>(fmt::format(R"EOF(

diff --git a/test/integration/ads_integration.h b/test/integration/ads_integration.h
@@ -49,11 +49,15 @@ class AdsIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, public H
 
   envoy::api::v2::Cluster buildCluster(const std::string& name);
 
+  envoy::api::v2::Cluster buildRedisCluster(const std::string& name);
+
   envoy::api::v2::ClusterLoadAssignment buildClusterLoadAssignment(const std::string& name);
 
   envoy::api::v2::Listener buildListener(const std::string& name, const std::string& route_config,
                                          const std::string& stat_prefix = "ads_test");
 
+  envoy::api::v2::Listener buildRedisListener(const std::string& name, const std::string& cluster);
+
   envoy::api::v2::RouteConfiguration buildRouteConfig(const std::string& name,
                                                       const std::string& cluster);
 

diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc
@@ -200,6 +200,46 @@ TEST_P(AdsIntegrationTest, DuplicateInitialClusters) {
   test_server_->waitForCounterGe("cluster_manager.cds.update_rejected", 1);
 }
 
+// Validates that removing a redis cluster does not crash Envoy.
+// Regression test for issue https://github.com/envoyproxy/envoy/issues/7990.
+TEST_P(AdsIntegrationTest, RedisClusterRemoval) {
+  initialize();
+
+  // Send initial configuration with a redis cluster and a redis proxy listener.
+  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "", {}, {}, {}, true));
+  sendDiscoveryResponse<envoy::api::v2::Cluster>(Config::TypeUrl::get().Cluster,
+                                                 {buildRedisCluster("redis_cluster")},
+                                                 {buildRedisCluster("redis_cluster")}, {}, "1");
+
+  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "",
+                                      {"redis_cluster"}, {}, {}));
+  sendDiscoveryResponse<envoy::api::v2::ClusterLoadAssignment>(
+      Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("redis_cluster")},
+      {buildClusterLoadAssignment("redis_cluster")}, {}, "1");
+
+  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "1", {}, {}, {}));
+  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Listener, "", {}, {}, {}));
+  sendDiscoveryResponse<envoy::api::v2::Listener>(
+      Config::TypeUrl::get().Listener, {buildRedisListener("listener_0", "redis_cluster")},
+      {buildRedisListener("listener_0", "redis_cluster")}, {}, "1");
+
+  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "1",
+                                      {"redis_cluster"}, {}, {}));
+
+  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Listener, "1", {}, {}, {}));
+
+  // Validate that redis listener is successfully created.
+  test_server_->waitForCounterGe("listener_manager.listener_create_success", 1);
+
+  // Now send a CDS update, removing redis cluster added above.
+  sendDiscoveryResponse<envoy::api::v2::Cluster>(
+      Config::TypeUrl::get().Cluster, {buildCluster("cluster_2")}, {buildCluster("cluster_2")},
+      {"redis_cluster"}, "2");
+
+  // Validate that the cluster is removed successfully.
+  test_server_->waitForCounterGe("cluster_manager.cluster_removed", 1);
+}
+
 // Validate that the request with duplicate clusters in the subsequent requests (warming clusters)
 // is rejected.
 TEST_P(AdsIntegrationTest, DuplicateWarmingClusters) {