envoyproxy · mattklein123 · Jul 19, 2018 · Jul 19, 2018
diff --git a/api/envoy/api/v2/core/health_check.proto b/api/envoy/api/v2/core/health_check.proto
@@ -152,16 +152,17 @@ 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;
 
     // Custom health check.
     CustomHealthCheck custom_health_check = 13;
   }
 
+  reserved 10; // redis_health_check is deprecated by :ref:`custom_health_check
+               // <envoy_api_field_core.HealthCheck.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
@@ -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 <envoy_api_field_config.health_checker.redis.v2.Redis.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 <envoy_api_field_config.health_checker.redis.v2.Redis.key>` to any value and waiting
+for traffic to drain.
+
+An example setting for :ref:`custom_health_check <envoy_api_msg_core.HealthCheck.CustomHealthCheck>` 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 <envoy_api_msg_core.HealthCheck.CustomHealthCheck>`
diff --git 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 <config_cluster_manager_cluster_lb_type>`.
 
-If active healthchecking is desired, the cluster should be configured with a
-:ref:`Redis healthcheck <config_cluster_manager_cluster_hc>`.
+If :ref:`active health checking <arch_overview_health_checking>` is desired, the
+cluster should be configured with a :ref:`custom health check
+<envoy_api_field_core.HealthCheck.custom_health_check>` which configured as a
+:ref:`Redis health checker <config_health_checkers_redis>`.
 
 If passive healthchecking is desired, also configure
 :ref:`outlier detection <config_cluster_manager_cluster_outlier_detection_summary>`.

diff --git 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
@@ -65,16 +65,10 @@ HealthCheckerFactory::create(const envoy::api::v2::core::HealthCheck& hc_config,
     }
     return std::make_shared<ProdGrpcHealthCheckerImpl>(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<Server::Configuration::CustomHealthCheckerFactory>(
-            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<Server::Configuration::HealthCheckerFactoryContext> 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
@@ -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
@@ -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,32 +50,30 @@ TEST(HealthCheckerFactoryTest, createRedisViaUpstreamHealthCheckerFactory) {
     custom_health_check:
       name: envoy.health_checkers.redis
       config:
-        key: foo
     )EOF";
 
-  NiceMock<Upstream::MockCluster> cluster;
-  Runtime::MockLoader runtime;
-  Runtime::MockRandomGenerator random;
-  Event::MockDispatcher dispatcher;
-  AccessLog::MockAccessLogManager log_manager;
-  EXPECT_NE(nullptr, dynamic_cast<CustomRedisHealthChecker*>(
-                         Upstream::HealthCheckerFactory::create(
-                             Upstream::parseHealthCheckFromV2Yaml(yaml), cluster, runtime, random,
-                             dispatcher, log_manager)
-                             .get()));
+  NiceMock<Server::Configuration::MockHealthCheckerFactoryContext> context;
+
+  RedisHealthCheckerFactory factory;
+  EXPECT_NE(
+      nullptr,
+      dynamic_cast<CustomRedisHealthChecker*>(
+          factory.createCustomHealthChecker(Upstream::parseHealthCheckFromV2Yaml(yaml), context)
+              .get()));
 }
 
-TEST(HealthCheckerFactoryTest, createRedisWithDeprecatedConfig) {
+TEST(HealthCheckerFactoryTest, createRedisViaUpstreamHealthCheckerFactory) {
   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
+    custom_health_check:
+      name: envoy.health_checkers.redis
+      config:
+        key: foo
     )EOF";
 
   NiceMock<Upstream::MockCluster> cluster;
@@ -84,8 +82,6 @@ TEST(HealthCheckerFactoryTest, createRedisWithDeprecatedConfig) {
   Event::MockDispatcher dispatcher;
   AccessLog::MockAccessLogManager log_manager;
   EXPECT_NE(nullptr, dynamic_cast<CustomRedisHealthChecker*>(
-                         // 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
@@ -30,27 +30,6 @@ class RedisHealthCheckerTest
       : cluster_(new NiceMock<Upstream::MockCluster>()),
         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;