From 65cafb48f06e13add4fcc38cbb15b3c348c52b40 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 19 Jul 2018 08:47:29 +0700 Subject: [PATCH] Remove the deprecated redis_health_check field Fixes https://github.com/envoyproxy/envoy/issues/3713 Signed-off-by: Dhi Aurrahman --- api/envoy/api/v2/core/health_check.proto | 7 +- .../configuration/health_checkers/redis.rst | 24 ++++-- docs/root/intro/arch_overview/redis.rst | 6 +- source/common/config/cds_json.cc | 6 +- source/common/upstream/health_checker_impl.cc | 8 +- .../health_checkers/redis/utility.h | 12 --- .../health_checkers/redis/config_test.cc | 32 ++++---- .../health_checkers/redis/redis_test.cc | 73 ------------------- 8 files changed, 44 insertions(+), 124 deletions(-) diff --git a/api/envoy/api/v2/core/health_check.proto b/api/envoy/api/v2/core/health_check.proto index 3d21e7d413a1c..94c3f7817962f 100644 --- a/api/envoy/api/v2/core/health_check.proto +++ b/api/envoy/api/v2/core/health_check.proto @@ -152,9 +152,6 @@ message HealthCheck { // TCP health check. TcpHealthCheck tcp_health_check = 9; - // Redis health check. - RedisHealthCheck redis_health_check = 10; - // gRPC health check. GrpcHealthCheck grpc_health_check = 11; @@ -162,6 +159,10 @@ message HealthCheck { CustomHealthCheck custom_health_check = 13; } + reserved 10; // redis_health_check is deprecated by :ref:`custom_health_check + // ` + reserved "redis_health_check"; + // The "no traffic interval" is a special health check interval that is used when a cluster has // never had traffic routed to it. This lower interval allows cluster information to be kept up to // date, without sending a potentially large amount of active health checking traffic for no diff --git a/docs/root/configuration/health_checkers/redis.rst b/docs/root/configuration/health_checkers/redis.rst index 2439982ced8d2..1859c005adb1e 100644 --- a/docs/root/configuration/health_checkers/redis.rst +++ b/docs/root/configuration/health_checkers/redis.rst @@ -3,12 +3,22 @@ Redis ===== -The Redis health checker is a custom health checker which checks Redis upstream hosts. It sends -a Redis PING command and expect a PONG response. The upstream Redis server can respond with -anything other than PONG to cause an immediate active health check failure. Optionally, Envoy can -perform EXISTS on a user-specified key. If the key does not exist it is considered a passing healthcheck. -This allows the user to mark a Redis instance for maintenance by setting the specified -:ref:`key ` to any value and waiting for -traffic to drain. +The Redis health checker is a custom health checker (with :code:`envoy.health_checkers.redis` as name) +which checks Redis upstream hosts. It sends a Redis PING command and expect a PONG response. The upstream +Redis server can respond with anything other than PONG to cause an immediate active health check failure. +Optionally, Envoy can perform EXISTS on a user-specified key. If the key does not exist it is considered a +passing healthcheck. This allows the user to mark a Redis instance for maintenance by setting the +specified :ref:`key ` to any value and waiting +for traffic to drain. + +An example setting for :ref:`custom_health_check ` as a +Redis health checker is shown below: + +.. code-block:: yaml + + custom_health_check: + name: envoy.health_checkers.redis + config: + key: foo * :ref:`v2 API reference ` \ No newline at end of file diff --git a/docs/root/intro/arch_overview/redis.rst b/docs/root/intro/arch_overview/redis.rst index b93830edba161..ff7d4696a4abb 100644 --- a/docs/root/intro/arch_overview/redis.rst +++ b/docs/root/intro/arch_overview/redis.rst @@ -43,8 +43,10 @@ For filter configuration details, see the Redis proxy filter The corresponding cluster definition should be configured with :ref:`ring hash load balancing `. -If active healthchecking is desired, the cluster should be configured with a -:ref:`Redis healthcheck `. +If :ref:`active health checking ` is desired, the +cluster should be configured with a :ref:`custom health check +` which configured as a +:ref:`Redis health checker `. If passive healthchecking is desired, also configure :ref:`outlier detection `. diff --git a/source/common/config/cds_json.cc b/source/common/config/cds_json.cc index cbf9ede748c4b..a7a684a590d62 100644 --- a/source/common/config/cds_json.cc +++ b/source/common/config/cds_json.cc @@ -53,9 +53,11 @@ void CdsJson::translateHealthCheck(const Json::Object& json_health_check, } } else { ASSERT(hc_type == "redis"); - auto* redis_health_check = health_check.mutable_redis_health_check(); + auto* redis_health_check = health_check.mutable_custom_health_check(); + redis_health_check->set_name("envoy.health_checkers.redis"); if (json_health_check.hasObject("redis_key")) { - redis_health_check->set_key(json_health_check.getString("redis_key")); + redis_health_check->mutable_config()->MergeFrom( + MessageUtil::keyValueStruct("key", json_health_check.getString("redis_key"))); } } } diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index cc90ab28aab61..faeeaf1a0672c 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -65,16 +65,10 @@ HealthCheckerFactory::create(const envoy::api::v2::core::HealthCheck& hc_config, } return std::make_shared(cluster, hc_config, dispatcher, runtime, random, std::move(event_logger)); - // Deprecated redis_health_check, preserving using old config until it is removed. - case envoy::api::v2::core::HealthCheck::HealthCheckerCase::kRedisHealthCheck: - ENVOY_LOG(warn, "redis_health_check is deprecated, use custom_health_check instead"); - FALLTHRU; case envoy::api::v2::core::HealthCheck::HealthCheckerCase::kCustomHealthCheck: { auto& factory = Config::Utility::getAndCheckFactory( - hc_config.has_redis_health_check() - ? Extensions::HealthCheckers::HealthCheckerNames::get().RedisHealthChecker - : std::string(hc_config.custom_health_check().name())); + std::string(hc_config.custom_health_check().name())); std::unique_ptr context( new HealthCheckerFactoryContextImpl(cluster, runtime, random, dispatcher, std::move(event_logger))); diff --git a/source/extensions/health_checkers/redis/utility.h b/source/extensions/health_checkers/redis/utility.h index 52b95ca4899a2..f41b95de8f2e9 100644 --- a/source/extensions/health_checkers/redis/utility.h +++ b/source/extensions/health_checkers/redis/utility.h @@ -10,20 +10,8 @@ namespace RedisHealthChecker { namespace { -static const envoy::config::health_checker::redis::v2::Redis translateFromRedisHealthCheck( - const envoy::api::v2::core::HealthCheck::RedisHealthCheck& deprecated_redis_config) { - envoy::config::health_checker::redis::v2::Redis config; - config.set_key(deprecated_redis_config.key()); - return config; -} - static const envoy::config::health_checker::redis::v2::Redis getRedisHealthCheckConfig(const envoy::api::v2::core::HealthCheck& hc_config) { - // TODO(dio): redis_health_check is deprecated. - if (hc_config.has_redis_health_check()) { - return translateFromRedisHealthCheck(hc_config.redis_health_check()); - } - ProtobufTypes::MessagePtr config = ProtobufTypes::MessagePtr{new envoy::config::health_checker::redis::v2::Redis()}; MessageUtil::jsonConvert(hc_config.custom_health_check().config(), *config); diff --git a/test/extensions/health_checkers/redis/config_test.cc b/test/extensions/health_checkers/redis/config_test.cc index 21ef2eb46525f..1f70f8115b030 100644 --- a/test/extensions/health_checkers/redis/config_test.cc +++ b/test/extensions/health_checkers/redis/config_test.cc @@ -39,7 +39,7 @@ TEST(HealthCheckerFactoryTest, createRedis) { .get())); } -TEST(HealthCheckerFactoryTest, createRedisViaUpstreamHealthCheckerFactory) { +TEST(HealthCheckerFactoryTest, createRedisWithoutKey) { const std::string yaml = R"EOF( timeout: 1s interval: 1s @@ -50,22 +50,19 @@ TEST(HealthCheckerFactoryTest, createRedisViaUpstreamHealthCheckerFactory) { custom_health_check: name: envoy.health_checkers.redis config: - key: foo )EOF"; - NiceMock cluster; - Runtime::MockLoader runtime; - Runtime::MockRandomGenerator random; - Event::MockDispatcher dispatcher; - AccessLog::MockAccessLogManager log_manager; - EXPECT_NE(nullptr, dynamic_cast( - Upstream::HealthCheckerFactory::create( - Upstream::parseHealthCheckFromV2Yaml(yaml), cluster, runtime, random, - dispatcher, log_manager) - .get())); + NiceMock context; + + RedisHealthCheckerFactory factory; + EXPECT_NE( + nullptr, + dynamic_cast( + factory.createCustomHealthChecker(Upstream::parseHealthCheckFromV2Yaml(yaml), context) + .get())); } -TEST(HealthCheckerFactoryTest, createRedisWithDeprecatedConfig) { +TEST(HealthCheckerFactoryTest, createRedisViaUpstreamHealthCheckerFactory) { const std::string yaml = R"EOF( timeout: 1s interval: 1s @@ -73,9 +70,10 @@ TEST(HealthCheckerFactoryTest, createRedisWithDeprecatedConfig) { interval_jitter: 1s unhealthy_threshold: 1 healthy_threshold: 1 - # Using the deprecated redis_health_check should work. - redis_health_check: - key: foo + custom_health_check: + name: envoy.health_checkers.redis + config: + key: foo )EOF"; NiceMock cluster; @@ -84,8 +82,6 @@ TEST(HealthCheckerFactoryTest, createRedisWithDeprecatedConfig) { Event::MockDispatcher dispatcher; AccessLog::MockAccessLogManager log_manager; EXPECT_NE(nullptr, dynamic_cast( - // Always use Upstream's HealthCheckerFactory when creating instance using - // deprecated config. Upstream::HealthCheckerFactory::create( Upstream::parseHealthCheckFromV2Yaml(yaml), cluster, runtime, random, dispatcher, log_manager) diff --git a/test/extensions/health_checkers/redis/redis_test.cc b/test/extensions/health_checkers/redis/redis_test.cc index 9aee0c0fdf524..015a7c08c9b5b 100644 --- a/test/extensions/health_checkers/redis/redis_test.cc +++ b/test/extensions/health_checkers/redis/redis_test.cc @@ -30,27 +30,6 @@ class RedisHealthCheckerTest : cluster_(new NiceMock()), event_logger_(new Upstream::MockHealthCheckEventLogger()) {} - void setupExistsHealthcheckDeprecated() { - const std::string yaml = R"EOF( - timeout: 1s - interval: 1s - no_traffic_interval: 5s - interval_jitter: 1s - unhealthy_threshold: 1 - healthy_threshold: 1 - # Using the deprecated redis_health_check should work. - redis_health_check: - key: foo - )EOF"; - - const auto& hc_config = Upstream::parseHealthCheckFromV2Yaml(yaml); - const auto& redis_config = getRedisHealthCheckConfig(hc_config); - - health_checker_.reset( - new RedisHealthChecker(*cluster_, hc_config, redis_config, dispatcher_, runtime_, random_, - Upstream::HealthCheckEventLoggerPtr(event_logger_), *this)); - } - void setup() { const std::string yaml = R"EOF( timeout: 1s @@ -280,58 +259,6 @@ TEST_F(RedisHealthCheckerTest, Exists) { EXPECT_EQ(2UL, cluster_->info_->stats_store_.counter("health_check.failure").value()); } -TEST_F(RedisHealthCheckerTest, ExistsDeprecated) { - InSequence s; - setupExistsHealthcheckDeprecated(); - - cluster_->prioritySet().getMockHostSet(0)->hosts_ = { - Upstream::makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; - - expectSessionCreate(); - expectClientCreate(); - expectExistsRequestCreate(); - health_checker_->start(); - - client_->runHighWatermarkCallbacks(); - client_->runLowWatermarkCallbacks(); - - // Success - EXPECT_CALL(*timeout_timer_, disableTimer()); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - Extensions::NetworkFilters::RedisProxy::RespValuePtr response( - new Extensions::NetworkFilters::RedisProxy::RespValue()); - response->type(Extensions::NetworkFilters::RedisProxy::RespType::Integer); - response->asInteger() = 0; - pool_callbacks_->onResponse(std::move(response)); - - expectExistsRequestCreate(); - interval_timer_->callback_(); - - // Failure, exists - EXPECT_CALL(*event_logger_, logEjectUnhealthy(_, _, _)); - EXPECT_CALL(*timeout_timer_, disableTimer()); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - response.reset(new Extensions::NetworkFilters::RedisProxy::RespValue()); - response->type(Extensions::NetworkFilters::RedisProxy::RespType::Integer); - response->asInteger() = 1; - pool_callbacks_->onResponse(std::move(response)); - - expectExistsRequestCreate(); - interval_timer_->callback_(); - - // Failure, no value - EXPECT_CALL(*timeout_timer_, disableTimer()); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - response.reset(new Extensions::NetworkFilters::RedisProxy::RespValue()); - pool_callbacks_->onResponse(std::move(response)); - - EXPECT_CALL(*client_, close()); - - EXPECT_EQ(3UL, cluster_->info_->stats_store_.counter("health_check.attempt").value()); - EXPECT_EQ(1UL, cluster_->info_->stats_store_.counter("health_check.success").value()); - EXPECT_EQ(2UL, cluster_->info_->stats_store_.counter("health_check.failure").value()); -} - // Tests that redis client will behave appropriately when reuse_connection is false. TEST_F(RedisHealthCheckerTest, NoConnectionReuse) { InSequence s;