Skip to content

health filter: cache degraded health checks #5659

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.

Sign up for GitHub

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

Merged
mattklein123 merged 3 commits into from
Jan 22, 2019
Merged

health filter: cache degraded health checks #5659

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb18077
health filter: cache degraded health checks
Jan 18, 2019
5734e71
format
Jan 18, 2019
abb0bcc
rename cachedResponseCode -> cachedResponse
Jan 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions source/extensions/filters/http/health_check/health_check.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ Http::FilterHeadersStatus HealthCheckFilter::encodeHeaders(Http::HeaderMap& head
if (health_check_request_) {
if (cache_manager_) {
cache_manager_->setCachedResponseCode(
static_cast<Http::Code>(Http::Utility::getResponseStatus(headers)));
static_cast<Http::Code>(Http::Utility::getResponseStatus(headers)),
headers.EnvoyDegraded() != nullptr);
}

headers.insertEnvoyUpstreamHealthCheckedCluster().value(context_.localInfo().clusterName());
Expand All @@ -96,12 +97,15 @@ Http::FilterHeadersStatus HealthCheckFilter::encodeHeaders(Http::HeaderMap& head
void HealthCheckFilter::onComplete() {
ASSERT(handling_);
Http::Code final_status = Http::Code::OK;
bool degraded = false;
if (context_.healthCheckFailed()) {
callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::FailedLocalHealthCheck);
final_status = Http::Code::ServiceUnavailable;
} else {
if (cache_manager_) {
final_status = cache_manager_->getCachedResponseCode();
const auto status_and_degraded = cache_manager_->getCachedResponseCode();
final_status = status_and_degraded.first;
degraded = status_and_degraded.second;
} else if (cluster_min_healthy_percentages_ != nullptr &&
!cluster_min_healthy_percentages_->empty()) {
// Check the status of the specified upstream cluster(s) to determine the right response.
Expand Down Expand Up @@ -143,7 +147,13 @@ void HealthCheckFilter::onComplete() {
}
}

callbacks_->sendLocalReply(final_status, "", nullptr, absl::nullopt);
callbacks_->sendLocalReply(final_status, "",
[degraded](auto& headers) {
if (degraded) {
headers.insertEnvoyDegraded();
}
},
absl::nullopt);
}

} // namespace HealthCheck
Expand Down
8 changes: 6 additions & 2 deletions source/extensions/filters/http/health_check/health_check.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,12 @@ class HealthCheckCacheManager {
public:
HealthCheckCacheManager(Event::Dispatcher& dispatcher, std::chrono::milliseconds timeout);

Http::Code getCachedResponseCode() { return last_response_code_; }
void setCachedResponseCode(Http::Code code) {
std::pair<Http::Code, bool> getCachedResponseCode() {

@mattklein123 mattklein123 Jan 21, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can you rename getCachedResponseCode() and setCachedResponseCode() to something more descriptive now that we are doing more stuff? s/Code/Data or something like that?

return {last_response_code_, last_response_degraded_};
}
void setCachedResponseCode(Http::Code code, bool degraded) {
last_response_code_ = code;
last_response_degraded_ = degraded;
use_cached_response_code_ = true;
}
bool useCachedResponseCode() { return use_cached_response_code_; }
Expand All @@ -41,6 +44,7 @@ class HealthCheckCacheManager {
const std::chrono::milliseconds timeout_;
std::atomic<bool> use_cached_response_code_{};
std::atomic<Http::Code> last_response_code_{};
std::atomic<bool> last_response_degraded_{};
};

typedef std::shared_ptr<HealthCheckCacheManager> HealthCheckCacheManagerSharedPtr;
Expand Down
39 changes: 36 additions & 3 deletions test/extensions/filters/http/health_check/health_check_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ TEST_F(HealthCheckFilterCachingTest, CachedServiceUnavailableCallbackCalled) {
EXPECT_CALL(context_, healthCheckFailed()).WillRepeatedly(Return(false));
EXPECT_CALL(callbacks_.stream_info_, healthCheck(true));
EXPECT_CALL(callbacks_.active_span_, setSampled(false));
cache_manager_->setCachedResponseCode(Http::Code::ServiceUnavailable);
cache_manager_->setCachedResponseCode(Http::Code::ServiceUnavailable, false);

Http::TestHeaderMapImpl health_check_response{{":status", "503"}};
EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true))
Expand All @@ -287,7 +287,7 @@ TEST_F(HealthCheckFilterCachingTest, CachedOkCallbackNotCalled) {
EXPECT_CALL(context_, healthCheckFailed()).WillRepeatedly(Return(false));
EXPECT_CALL(callbacks_.stream_info_, healthCheck(true));
EXPECT_CALL(callbacks_.active_span_, setSampled(false));
cache_manager_->setCachedResponseCode(Http::Code::OK);
cache_manager_->setCachedResponseCode(Http::Code::OK, false);

Http::TestHeaderMapImpl health_check_response{{":status", "200"}};
EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true))
Expand All @@ -313,7 +313,40 @@ TEST_F(HealthCheckFilterCachingTest, All) {
EXPECT_EQ(Http::FilterHeadersStatus::Continue,
filter_->encodeHeaders(service_response_headers, true));

// Verify that the next request uses the cached value.
// Verify that the next request uses the cached value without setting the degraded header.
prepareFilter(true);
EXPECT_CALL(callbacks_.stream_info_,
setResponseFlag(StreamInfo::ResponseFlag::FailedLocalHealthCheck));
EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true))
.Times(1)
.WillRepeatedly(Invoke([&](Http::HeaderMap& headers, bool end_stream) {
filter_->encodeHeaders(headers, end_stream);
EXPECT_STREQ("cluster_name", headers.EnvoyUpstreamHealthCheckedCluster()->value().c_str());
}));
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
filter_->decodeHeaders(request_headers_, true));

// Fire the timer, this should result in the next request going through.
EXPECT_CALL(*cache_timer_, enableTimer(_));
cache_timer_->callback_();
prepareFilter(true);
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, true));
}

TEST_F(HealthCheckFilterCachingTest, DegradedHeader) {
EXPECT_CALL(callbacks_.stream_info_, healthCheck(true)).Times(3);
EXPECT_CALL(callbacks_.active_span_, setSampled(false)).Times(3);

// Verify that the first request goes through.
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, true));
Http::TestHeaderMapImpl service_response_headers{{":status", "503"},
{"x-envoy-degraded", "true"}};
Http::TestHeaderMapImpl health_check_response{{":status", "503"}, {"x-envoy-degraded", ""}};

EXPECT_EQ(Http::FilterHeadersStatus::Continue,
filter_->encodeHeaders(service_response_headers, true));

// Verify that the next request uses the cached value and that the x-envoy-degraded header is set.
prepareFilter(true);
EXPECT_CALL(callbacks_.stream_info_,
setResponseFlag(StreamInfo::ResponseFlag::FailedLocalHealthCheck));
Expand Down