diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 7c4ac14f1bee..78bfade0a9d3 100644 --- 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 index a7ada1599f80..29908486a15f 100644 --- 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 index 60ababd92f17..96e499dce644 100644 --- 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 index cf62f423aaad..48a8af72bd5e 100644 --- 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(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( @@ -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(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(fmt::format(R"EOF( diff --git a/test/integration/ads_integration.h b/test/integration/ads_integration.h index 024bcf04d607..628550fb9cea 100644 --- 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 index 9479194d73be..92357a7a24bb 100644 --- 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(Config::TypeUrl::get().Cluster, + {buildRedisCluster("redis_cluster")}, + {buildRedisCluster("redis_cluster")}, {}, "1"); + + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "", + {"redis_cluster"}, {}, {})); + sendDiscoveryResponse( + 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( + 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( + 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) {