-
Notifications
You must be signed in to change notification settings - Fork 5.5k
healthcheck: Allow setting host header value of HTTP health checker #2591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
8c7cc00
cec5592
0dbc51e
428e1c5
10c08ef
b3f2c8f
5937131
f38a619
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -220,6 +220,29 @@ class HttpHealthCheckerImplTest : public testing::Test { | |
| }); | ||
| } | ||
|
|
||
| void setupServiceValidationWithCustomHostValueHC(const std::string& host) { | ||
| std::string json = fmt::format(R"EOF( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we drop v1 support, let's make this a v2 YAML config for testing?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I have it in v2 YAML on b3f2c8f. Thanks for reviewing @mattklein123! |
||
| {{ | ||
| "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()); | ||
|
|
@@ -426,6 +449,50 @@ 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_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", _)); | ||
| 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<std::string> 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, SuccessServiceCheckWithCustomHostValue) { | ||
| std::string host = "www.envoyproxy.io"; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: |
||
| 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_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", _)); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need v1 support? If so we need to add v1 docs. Otherwise I would just drop it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the v1 support in the following commit 5937131