From 8c7cc00266a4ea578ae3354b7a513cb643c462a3 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Sat, 10 Feb 2018 14:06:25 +0700 Subject: [PATCH 1/8] Allow to set host for http healtcheck request Signed-off-by: Dhi Aurrahman --- source/common/config/cds_json.cc | 3 +++ source/common/json/config_schemas.cc | 1 + source/common/upstream/health_checker_impl.cc | 5 +++-- source/common/upstream/health_checker_impl.h | 1 + test/common/upstream/health_checker_impl_test.cc | 3 ++- 5 files changed, 10 insertions(+), 3 deletions(-) diff --git a/source/common/config/cds_json.cc b/source/common/config/cds_json.cc index a09c8a2b4fb18..4468e6f254eeb 100644 --- a/source/common/config/cds_json.cc +++ b/source/common/config/cds_json.cc @@ -33,6 +33,9 @@ void CdsJson::translateHealthCheck(const Json::Object& json_health_check, const std::string hc_type = json_health_check.getString("type"); if (hc_type == "http") { health_check.mutable_http_health_check()->set_path(json_health_check.getString("path")); + if (json_health_check.hasObject("host")) { + health_check.mutable_http_health_check()->set_host(json_health_check.getString("host")); + } if (json_health_check.hasObject("service_name")) { health_check.mutable_http_health_check()->set_service_name( json_health_check.getString("service_name")); diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index dbc25ba9c3430..79c1e3c567182 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -1415,6 +1415,7 @@ const std::string Json::Schema::CLUSTER_HEALTH_CHECK_SCHEMA(R"EOF( "exclusiveMinimum" : true }, "path" : {"type" : "string"}, + "host" : {"type" : "string"}, "send" : { "type" : "array", "items" : {"$ref" : "#/definitions/health_check_bytes"} diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index 1e53bd9b52644..32f3cfbbe97e6 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -281,7 +281,7 @@ HttpHealthCheckerImpl::HttpHealthCheckerImpl(const Cluster& cluster, Runtime::Loader& runtime, Runtime::RandomGenerator& random) : HealthCheckerImplBase(cluster, config, dispatcher, runtime, random), - path_(config.http_health_check().path()) { + path_(config.http_health_check().path()), host_name_(config.http_health_check().host()) { if (!config.http_health_check().service_name().empty()) { service_name_.value(config.http_health_check().service_name()); } @@ -332,7 +332,8 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onInterval() { Http::HeaderMapImpl request_headers{ {Http::Headers::get().Method, "GET"}, - {Http::Headers::get().Host, parent_.cluster_.info()->name()}, + {Http::Headers::get().Host, + parent_.host_name_.empty() ? parent_.cluster_.info()->name() : parent_.host_name_}, {Http::Headers::get().Path, parent_.path_}, {Http::Headers::get().UserAgent, Http::Headers::get().UserAgentValues.EnvoyHealthChecker}}; diff --git a/source/common/upstream/health_checker_impl.h b/source/common/upstream/health_checker_impl.h index 6d06c37909ab6..6faaff172ec20 100644 --- a/source/common/upstream/health_checker_impl.h +++ b/source/common/upstream/health_checker_impl.h @@ -232,6 +232,7 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase { } const std::string path_; + const std::string host_name_; Optional service_name_; }; diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index 8cc72604239d5..6abf4cddcd08c 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -209,7 +209,8 @@ class HttpHealthCheckerImplTest : public testing::Test { "interval_jitter_ms": 1000, "unhealthy_threshold": 2, "healthy_threshold": 2, - "path": "/healthcheck" + "path": "/healthcheck", + "host": "hello" } )EOF"; From cec55925cd1cec5053aa61eee9b722a3704fa30f Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 12 Feb 2018 18:05:08 +0700 Subject: [PATCH 2/8] Add test for custom header value Signed-off-by: Dhi Aurrahman --- .../upstream/health_checker_impl_test.cc | 56 ++++++++++++++++++- 1 file changed, 54 insertions(+), 2 deletions(-) diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index 6abf4cddcd08c..f155cf6860b09 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -209,8 +209,7 @@ class HttpHealthCheckerImplTest : public testing::Test { "interval_jitter_ms": 1000, "unhealthy_threshold": 2, "healthy_threshold": 2, - "path": "/healthcheck", - "host": "hello" + "path": "/healthcheck" } )EOF"; @@ -221,6 +220,28 @@ class HttpHealthCheckerImplTest : public testing::Test { }); } + void setupServiceValidationWithCustomHostValueHC(const std::string &host) { + std::string json = fmt::format(R"EOF( + {{ + "type": "http", + "timeout_ms": 1000, + "interval_ms": 1000, + "service_name": "locations", + "interval_jitter_ms": 1000, + "unhealthy_threshold": 2, + "healthy_threshold": 2, + "path": "/healthcheck", + "host": "{0}" + }} + )EOF", host); + + health_checker_.reset(new TestHttpHealthCheckerImpl(*cluster_, parseHealthCheckFromJson(json), + dispatcher_, runtime_, random_)); + health_checker_->addHostCheckCompleteCb([this](HostSharedPtr host, bool changed_state) -> void { + onHostStatus(host, changed_state); + }); + } + void expectSessionCreate() { // Expectations are in LIFO order. TestSessionPtr new_test_session(new TestSession()); @@ -439,6 +460,37 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheck) { EXPECT_TRUE(cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthy()); } +TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckWithCustomHostValue) { + std::string host = "www.envoyproxy.io"; + setupServiceValidationWithCustomHostValueHC(host); + EXPECT_CALL(runtime_.snapshot_, featureEnabled("health_check.verify_cluster", 100)) + .WillOnce(Return(true)); + + EXPECT_CALL(*this, onHostStatus(_, false)).Times(1); + + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + cluster_->info_->stats().upstream_cx_total_.inc(); + expectSessionCreate(); + expectStreamCreate(0); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_)); + EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) + .WillOnce(Invoke([&](const Http::HeaderMap& headers, bool) { + EXPECT_TRUE(headers.Host()); + EXPECT_EQ(headers.Host()->value().c_str(), std::string(host)); + })); + health_checker_->start(); + + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.max_interval", _)); + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.min_interval", _)) + .WillOnce(Return(45000)); + EXPECT_CALL(*test_sessions_[0]->interval_timer_, enableTimer(std::chrono::milliseconds(45000))); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()); + Optional health_checked_cluster("locations-production-iad"); + respond(0, "200", false, true, false, health_checked_cluster); + EXPECT_TRUE(cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthy()); +} + TEST_F(HttpHealthCheckerImplTest, ServiceDoesNotMatchFail) { setupServiceValidationHC(); EXPECT_CALL(runtime_.snapshot_, featureEnabled("health_check.verify_cluster", 100)) From 0dbc51e918f1aa71c9db4cdbbfa5c6a1613251a2 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 12 Feb 2018 18:10:14 +0700 Subject: [PATCH 3/8] Add check when the host value is not being set Signed-off-by: Dhi Aurrahman --- test/common/upstream/health_checker_impl_test.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index f155cf6860b09..d337159124957 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -448,6 +448,12 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheck) { expectSessionCreate(); expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_)); + EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) + .WillOnce(Invoke([&](const Http::HeaderMap& headers, bool) { + EXPECT_TRUE(headers.Host()); + EXPECT_EQ(headers.Host()->value().c_str(), std::string("fake_cluster")); + EXPECT_EQ(headers.Path()->value().c_str(), std::string("/healthcheck")); + })); health_checker_->start(); EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.max_interval", _)); @@ -478,6 +484,7 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckWithCustomHostValue) { .WillOnce(Invoke([&](const Http::HeaderMap& headers, bool) { EXPECT_TRUE(headers.Host()); EXPECT_EQ(headers.Host()->value().c_str(), std::string(host)); + EXPECT_EQ(headers.Path()->value().c_str(), std::string("/healthcheck")); })); health_checker_->start(); From 428e1c5d9be496bcbed59155a1754c22e33f99fe Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 13 Feb 2018 05:26:37 +0700 Subject: [PATCH 4/8] Add more expectations Signed-off-by: Dhi Aurrahman --- .../upstream/health_checker_impl_test.cc | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index d337159124957..fc908f2577801 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -220,7 +220,7 @@ class HttpHealthCheckerImplTest : public testing::Test { }); } - void setupServiceValidationWithCustomHostValueHC(const std::string &host) { + void setupServiceValidationWithCustomHostValueHC(const std::string& host) { std::string json = fmt::format(R"EOF( {{ "type": "http", @@ -233,7 +233,8 @@ class HttpHealthCheckerImplTest : public testing::Test { "path": "/healthcheck", "host": "{0}" }} - )EOF", host); + )EOF", + host); health_checker_.reset(new TestHttpHealthCheckerImpl(*cluster_, parseHealthCheckFromJson(json), dispatcher_, runtime_, random_)); @@ -449,11 +450,14 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheck) { expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_)); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const Http::HeaderMap& headers, bool) { - EXPECT_TRUE(headers.Host()); - EXPECT_EQ(headers.Host()->value().c_str(), std::string("fake_cluster")); - EXPECT_EQ(headers.Path()->value().c_str(), std::string("/healthcheck")); - })); + .WillOnce(Invoke([&](const Http::HeaderMap& headers, bool) { + EXPECT_TRUE(headers.Host()); + EXPECT_TRUE(headers.Path()); + EXPECT_NE(nullptr, headers.Host()); + EXPECT_NE(nullptr, headers.Path()); + EXPECT_EQ(headers.Host()->value().c_str(), std::string("fake_cluster")); + EXPECT_EQ(headers.Path()->value().c_str(), std::string("/healthcheck")); + })); health_checker_->start(); EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.max_interval", _)); @@ -481,11 +485,14 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckWithCustomHostValue) { expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_)); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const Http::HeaderMap& headers, bool) { - EXPECT_TRUE(headers.Host()); - EXPECT_EQ(headers.Host()->value().c_str(), std::string(host)); - EXPECT_EQ(headers.Path()->value().c_str(), std::string("/healthcheck")); - })); + .WillOnce(Invoke([&](const Http::HeaderMap& headers, bool) { + EXPECT_TRUE(headers.Host()); + EXPECT_TRUE(headers.Path()); + EXPECT_NE(nullptr, headers.Host()); + EXPECT_NE(nullptr, headers.Path()); + EXPECT_EQ(headers.Host()->value().c_str(), std::string(host)); + EXPECT_EQ(headers.Path()->value().c_str(), std::string("/healthcheck")); + })); health_checker_->start(); EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.max_interval", _)); From 10c08ef955b1a316e07e8d3043f1d9f5e4690cdc Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 13 Feb 2018 05:45:44 +0700 Subject: [PATCH 5/8] Rename the var; Add to raw release note. Signed-off-by: Dhi Aurrahman --- RAW_RELEASE_NOTES.md | 2 ++ source/common/upstream/health_checker_impl.cc | 4 ++-- source/common/upstream/health_checker_impl.h | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/RAW_RELEASE_NOTES.md b/RAW_RELEASE_NOTES.md index de3ae4e5e49b6..a51e3b6ff48dc 100644 --- a/RAW_RELEASE_NOTES.md +++ b/RAW_RELEASE_NOTES.md @@ -62,3 +62,5 @@ final version. namespace can be used by prepending '@' to a socket path. * Added `GEORADIUS_RO` and `GEORADIUSBYMEMBER_RO` to the Redis command splitter whitelist. * Added support for trusting additional hops in the X-Forwarded-For request header. +* Added setting host header value for http health check request. + diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index 32f3cfbbe97e6..114f0c34d29af 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -281,7 +281,7 @@ HttpHealthCheckerImpl::HttpHealthCheckerImpl(const Cluster& cluster, Runtime::Loader& runtime, Runtime::RandomGenerator& random) : HealthCheckerImplBase(cluster, config, dispatcher, runtime, random), - path_(config.http_health_check().path()), host_name_(config.http_health_check().host()) { + path_(config.http_health_check().path()), host_value_(config.http_health_check().host()) { if (!config.http_health_check().service_name().empty()) { service_name_.value(config.http_health_check().service_name()); } @@ -333,7 +333,7 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onInterval() { Http::HeaderMapImpl request_headers{ {Http::Headers::get().Method, "GET"}, {Http::Headers::get().Host, - parent_.host_name_.empty() ? parent_.cluster_.info()->name() : parent_.host_name_}, + parent_.host_value_.empty() ? parent_.cluster_.info()->name() : parent_.host_value_}, {Http::Headers::get().Path, parent_.path_}, {Http::Headers::get().UserAgent, Http::Headers::get().UserAgentValues.EnvoyHealthChecker}}; diff --git a/source/common/upstream/health_checker_impl.h b/source/common/upstream/health_checker_impl.h index 6faaff172ec20..021710e0fc506 100644 --- a/source/common/upstream/health_checker_impl.h +++ b/source/common/upstream/health_checker_impl.h @@ -232,7 +232,7 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase { } const std::string path_; - const std::string host_name_; + const std::string host_value_; Optional service_name_; }; From b3f2c8fd8089caad7c87faeaeb423daac60ec9dd Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 13 Feb 2018 07:52:53 +0700 Subject: [PATCH 6/8] Use yaml instead of json in test Signed-off-by: Dhi Aurrahman --- .../upstream/health_checker_impl_test.cc | 31 +++++++++++-------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index fc908f2577801..fee237521ce17 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -49,6 +49,12 @@ envoy::api::v2::core::HealthCheck parseHealthCheckFromJson(const std::string& js return health_check; } +envoy::api::v2::core::HealthCheck parseHealthCheckFromYaml(const std::string& yaml_string) { + envoy::api::v2::core::HealthCheck health_check; + MessageUtil::loadFromYaml(yaml_string, health_check); + return health_check; +} + envoy::api::v2::core::HealthCheck createGrpcHealthCheckConfig() { envoy::api::v2::core::HealthCheck health_check; health_check.mutable_timeout()->set_seconds(1); @@ -221,22 +227,20 @@ class HttpHealthCheckerImplTest : public testing::Test { } void setupServiceValidationWithCustomHostValueHC(const std::string& host) { - std::string json = fmt::format(R"EOF( - {{ - "type": "http", - "timeout_ms": 1000, - "interval_ms": 1000, - "service_name": "locations", - "interval_jitter_ms": 1000, - "unhealthy_threshold": 2, - "healthy_threshold": 2, - "path": "/healthcheck", - "host": "{0}" - }} + std::string yaml = fmt::format(R"EOF( + timeout: 1s + interval: 1s + interval_jitter: 1s + unhealthy_threshold: 2 + healthy_threshold: 2 + http_health_check: + service_name: locations + path: /healthcheck + host: {0} )EOF", host); - health_checker_.reset(new TestHttpHealthCheckerImpl(*cluster_, parseHealthCheckFromJson(json), + health_checker_.reset(new TestHttpHealthCheckerImpl(*cluster_, parseHealthCheckFromYaml(yaml), dispatcher_, runtime_, random_)); health_checker_->addHostCheckCompleteCb([this](HostSharedPtr host, bool changed_state) -> void { onHostStatus(host, changed_state); @@ -473,6 +477,7 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheck) { TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckWithCustomHostValue) { std::string host = "www.envoyproxy.io"; setupServiceValidationWithCustomHostValueHC(host); + // requires non-empty `service_name` in config. EXPECT_CALL(runtime_.snapshot_, featureEnabled("health_check.verify_cluster", 100)) .WillOnce(Return(true)); From 59371317b26b299b2e663f2dcb6d81d4bb3bfcd5 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 13 Feb 2018 07:54:33 +0700 Subject: [PATCH 7/8] Remove v1 support for setting host header value Signed-off-by: Dhi Aurrahman --- source/common/config/cds_json.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/source/common/config/cds_json.cc b/source/common/config/cds_json.cc index 4468e6f254eeb..a09c8a2b4fb18 100644 --- a/source/common/config/cds_json.cc +++ b/source/common/config/cds_json.cc @@ -33,9 +33,6 @@ void CdsJson::translateHealthCheck(const Json::Object& json_health_check, const std::string hc_type = json_health_check.getString("type"); if (hc_type == "http") { health_check.mutable_http_health_check()->set_path(json_health_check.getString("path")); - if (json_health_check.hasObject("host")) { - health_check.mutable_http_health_check()->set_host(json_health_check.getString("host")); - } if (json_health_check.hasObject("service_name")) { health_check.mutable_http_health_check()->set_service_name( json_health_check.getString("service_name")); From f38a6199405ef33cc61c3597bc68df3ca8865b01 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 13 Feb 2018 11:40:50 +0700 Subject: [PATCH 8/8] Remove host from json config schema Signed-off-by: Dhi Aurrahman --- source/common/json/config_schemas.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 79c1e3c567182..dbc25ba9c3430 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -1415,7 +1415,6 @@ const std::string Json::Schema::CLUSTER_HEALTH_CHECK_SCHEMA(R"EOF( "exclusiveMinimum" : true }, "path" : {"type" : "string"}, - "host" : {"type" : "string"}, "send" : { "type" : "array", "items" : {"$ref" : "#/definitions/health_check_bytes"}