health check: fix additional regression from #6765 (#6942)

mattklein123 · web-flow · commit 6dd4b6f83119 · 2019-05-14T17:01:48.000-07:00
If we inline delete a host during a failure callback we need to
account for the connection being cleaned up prior to handling
'connection: close' headers.

Signed-off-by: Matt Klein &lt;mklein@lyft.com&gt;
diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc
@@ -195,8 +195,8 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onInterval() {
     expect_reset_ = false;
   }
 
-  request_encoder_ = &client_->newStream(*this);
-  request_encoder_->getStream().addCallbacks(*this);
+  Http::StreamEncoder* request_encoder = &client_->newStream(*this);
+  request_encoder->getStream().addCallbacks(*this);
 
   Http::HeaderMapImpl request_headers{
       {Http::Headers::get().Method, "GET"},
@@ -209,8 +209,7 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onInterval() {
   stream_info.setDownstreamRemoteAddress(local_address_);
   stream_info.onUpstreamHostSelected(host_);
   parent_.request_headers_parser_->evaluateHeaders(request_headers, stream_info);
-  request_encoder_->encodeHeaders(request_headers, true);
-  request_encoder_ = nullptr;
+  request_encoder->encodeHeaders(request_headers, true);
 }
 
 void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onResetStream(Http::StreamResetReason,
@@ -269,13 +268,16 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onResponseComplete() {
     break;
   }
 
-  if ((response_headers_->Connection() &&
-       absl::EqualsIgnoreCase(response_headers_->Connection()->value().getStringView(),
-                              Http::Headers::get().ConnectionValues.Close)) ||
-      (response_headers_->ProxyConnection() && protocol_ != Http::Protocol::Http2 &&
-       absl::EqualsIgnoreCase(response_headers_->ProxyConnection()->value().getStringView(),
-                              Http::Headers::get().ConnectionValues.Close)) ||
-      !parent_.reuse_connection_) {
+  // It is possible for this session to have been deferred destroyed inline in handleFailure()
+  // above so make sure we still have a connection that we might need to close.
+  if (client_ != nullptr &&
+      ((response_headers_->Connection() &&
+        absl::EqualsIgnoreCase(response_headers_->Connection()->value().getStringView(),
+                               Http::Headers::get().ConnectionValues.Close)) ||
+       (response_headers_->ProxyConnection() && protocol_ != Http::Protocol::Http2 &&
+        absl::EqualsIgnoreCase(response_headers_->ProxyConnection()->value().getStringView(),
+                               Http::Headers::get().ConnectionValues.Close)) ||
+       !parent_.reuse_connection_)) {
     client_->close();
   }
 
diff --git a/source/common/upstream/health_checker_impl.h b/source/common/upstream/health_checker_impl.h
@@ -111,7 +111,6 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase {
     ConnectionCallbackImpl connection_callback_impl_{*this};
     HttpHealthCheckerImpl& parent_;
     Http::CodecClientPtr client_;
-    Http::StreamEncoder* request_encoder_{};
     Http::HeaderMapPtr response_headers_;
     const std::string& hostname_;
     const Http::Protocol protocol_;
diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc
@@ -1271,7 +1271,7 @@ TEST_F(HttpHealthCheckerImplTest, SuccessStartFailedFailFirstLogError) {
 }
 
 // Verify that removal during a failure callback works.
-TEST_F(HttpHealthCheckerImplTest, HttpFailRemoveHostInCallback) {
+TEST_F(HttpHealthCheckerImplTest, HttpFailRemoveHostInCallbackNoClose) {
   setupNoServiceValidationHC();
   cluster_->prioritySet().getMockHostSet(0)->hosts_ = {
       makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")};
@@ -1292,6 +1292,28 @@ TEST_F(HttpHealthCheckerImplTest, HttpFailRemoveHostInCallback) {
   respond(0, "503", false);
 }
 
+// Verify that removal during a failure callback works with connection close.
+TEST_F(HttpHealthCheckerImplTest, HttpFailRemoveHostInCallbackClose) {
+  setupNoServiceValidationHC();
+  cluster_->prioritySet().getMockHostSet(0)->hosts_ = {
+      makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")};
+  expectSessionCreate();
+  expectStreamCreate(0);
+  EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_));
+  health_checker_->start();
+
+  EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Changed))
+      .WillOnce(Invoke([&](HostSharedPtr host, HealthTransition) {
+        cluster_->prioritySet().getMockHostSet(0)->hosts_ = {};
+        cluster_->prioritySet().runUpdateCallbacks(0, {}, {host});
+      }));
+  EXPECT_CALL(*event_logger_, logEjectUnhealthy(_, _, _));
+  EXPECT_CALL(*test_sessions_[0]->interval_timer_, enableTimer(_)).Times(0);
+  EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()).Times(0);
+  EXPECT_CALL(*event_logger_, logUnhealthy(_, _, _, true));
+  respond(0, "503", true);
+}
+
 TEST_F(HttpHealthCheckerImplTest, HttpFail) {
   setupNoServiceValidationHC();
   cluster_->prioritySet().getMockHostSet(0)->hosts_ = {
diff --git a/test/integration/eds_integration_test.cc b/test/integration/eds_integration_test.cc
@@ -103,7 +103,8 @@ TEST_P(EdsIntegrationTest, RemoveAfterHcFail) {
 
   // Fail HC and verify the host is gone.
   waitForNextUpstreamRequest();
-  upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "503"}}, true);
+  upstream_request_->encodeHeaders(
+      Http::TestHeaderMapImpl{{":status", "503"}, {"connection", "close"}}, true);
   test_server_->waitForGaugeEq("cluster.cluster_0.membership_healthy", 0);
   EXPECT_EQ(0, test_server_->gauge("cluster.cluster_0.membership_total")->value());
 }

Original file line number	Diff line number	Diff line change
`@@ -103,7 +103,8 @@ TEST_P(EdsIntegrationTest, RemoveAfterHcFail) {`
`103`	`103`
`104`	`104`	`// Fail HC and verify the host is gone.`
`105`	`105`	`waitForNextUpstreamRequest();`
`106`		`- upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "503"}}, true);`
	`106`	`+ upstream_request_->encodeHeaders(`
	`107`	`+ Http::TestHeaderMapImpl{{":status", "503"}, {"connection", "close"}}, true);`
`107`	`108`	`test_server_->waitForGaugeEq("cluster.cluster_0.membership_healthy", 0);`
`108`	`109`	`EXPECT_EQ(0, test_server_->gauge("cluster.cluster_0.membership_total")->value());`
`109`	`110`	`}`