HTTP health checker: handle GOAWAY from HTTP2 upstreams

docs/root/version_history/current.rst

@@ -17,6 +17,8 @@ Bug Fixes
 ---------
 *Changes expected to improve the state of the world and are unlikely to have negative effects*
 
+* active http health checks: properly handles HTTP/2 GOAWAY frames from the upstream. Previously a GOAWAY frame due to a graceful listener drain could cause improper failed health checks due to streams being refused by the upstream on a connection that is going away. To revert to old GOAWAY handling behavior, set the runtime feature `envoy.reloadable_features.health_check.graceful_goaway_handling` to false.
+
 * http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests.
 
 Removed Config or Runtime

source/common/runtime/runtime_features.cc

@@ -73,6 +73,7 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.fix_wildcard_matching",
     "envoy.reloadable_features.fixed_connection_close",
     "envoy.reloadable_features.hcm_stream_error_on_invalid_message",
+    "envoy.reloadable_features.health_check.graceful_goaway_handling",
     "envoy.reloadable_features.http_default_alpn",
     "envoy.reloadable_features.http_match_on_all_headers",
     "envoy.reloadable_features.http_set_copy_replace_all_headers",

source/common/upstream/health_checker_impl.cc

@@ -256,11 +256,14 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onInterval() {
                                            parent_.transportSocketMatchMetadata().get());
     client_.reset(parent_.createCodecClient(conn));
     client_->addConnectionCallbacks(connection_callback_impl_);
+    client_->setCodecConnectionCallbacks(http_connection_callback_impl_);
     expect_reset_ = false;
+    reuse_connection_ = parent_.reuse_connection_;
   }
 
   Http::RequestEncoder* request_encoder = &client_->newStream(*this);
   request_encoder->getStream().addCallbacks(*this);
+  request_in_flight_ = true;
 
   const auto request_headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>(
       {{Http::Headers::get().Method, "GET"},
@@ -279,15 +282,51 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onInterval() {
 
 void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onResetStream(Http::StreamResetReason,
                                                                         absl::string_view) {
+  request_in_flight_ = false;
+  ENVOY_CONN_LOG(debug, "connection/stream error health_flags={}", *client_,
+                 HostUtility::healthFlagsToString(*host_));
+  if (client_ && !reuse_connection_) {
+    client_->close();
+  }
+
   if (expect_reset_) {
     return;
   }
 
-  ENVOY_CONN_LOG(debug, "connection/stream error health_flags={}", *client_,
-                 HostUtility::healthFlagsToString(*host_));
   handleFailure(envoy::data::core::v3::NETWORK);
 }
 
+void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onGoAway(
+    Http::GoAwayErrorCode error_code) {
+  ENVOY_CONN_LOG(debug, "connection going away health_flags={}", *client_,
+                 HostUtility::healthFlagsToString(*host_));
+
+  // Runtime guard around graceful handling of NO_ERROR GOAWAY handling. The old behavior is to
+  // ignore GOAWAY completely.
+  if (!parent_.runtime_.snapshot().featureEnabled(
+          "envoy.reloadable_features.health_check.graceful_goaway_handling", 100UL)) {
+    return;
+  }
+
+  if (request_in_flight_ && error_code == Http::GoAwayErrorCode::NoError) {
+    // The server is starting a graceful shutdown. Allow the in flight request
+    // to finish without treating this as a health check error, and then
+    // reconnect.
+    reuse_connection_ = false;
+    return;
+  }
+
+  if (request_in_flight_) {
+    // Record this as a failed health check.
+    handleFailure(envoy::data::core::v3::NETWORK);
+  }
+
+  if (client_) {
+    expect_reset_ = true;
+    client_->close();
+  }
+}
+
 HttpHealthCheckerImpl::HttpActiveHealthCheckSession::HealthCheckResult
 HttpHealthCheckerImpl::HttpActiveHealthCheckSession::healthCheckResult() {
   uint64_t response_code = Http::Utility::getResponseStatus(*response_headers_);
@@ -318,6 +357,8 @@ HttpHealthCheckerImpl::HttpActiveHealthCheckSession::healthCheckResult() {
 }
 
 void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onResponseComplete() {
+  request_in_flight_ = false;
+
   switch (healthCheckResult()) {
   case HealthCheckResult::Succeeded:
     handleSuccess(false);
@@ -345,7 +386,7 @@ bool HttpHealthCheckerImpl::HttpActiveHealthCheckSession::shouldClose() const {
     return false;
   }
 
-  if (!parent_.reuse_connection_) {
+  if (!reuse_connection_) {
     return true;
   }
 
@@ -375,6 +416,7 @@ bool HttpHealthCheckerImpl::HttpActiveHealthCheckSession::shouldClose() const {
 }
 
 void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onTimeout() {
+  request_in_flight_ = false;
   if (client_) {
     host_->setActiveHealthFailureType(Host::ActiveHealthFailureType::TIMEOUT);
     ENVOY_CONN_LOG(debug, "connection/stream timeout health_flags={}", *client_,

source/common/upstream/health_checker_impl.h

@@ -105,6 +105,7 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase {
     void onBelowWriteBufferLowWatermark() override {}
 
     void onEvent(Network::ConnectionEvent event);
+    void onGoAway(Http::GoAwayErrorCode error_code);
 
     class ConnectionCallbackImpl : public Network::ConnectionCallbacks {
     public:
@@ -118,14 +119,27 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase {
       HttpActiveHealthCheckSession& parent_;
     };
 
+    class HttpConnectionCallbackImpl : public Http::ConnectionCallbacks {
+    public:
+      HttpConnectionCallbackImpl(HttpActiveHealthCheckSession& parent) : parent_(parent) {}
+      // Http::ConnectionCallbacks
+      void onGoAway(Http::GoAwayErrorCode error_code) override { parent_.onGoAway(error_code); }
+
+    private:
+      HttpActiveHealthCheckSession& parent_;
+    };
+
     ConnectionCallbackImpl connection_callback_impl_{*this};
+    HttpConnectionCallbackImpl http_connection_callback_impl_{*this};
     HttpHealthCheckerImpl& parent_;
     Http::CodecClientPtr client_;
     Http::ResponseHeaderMapPtr response_headers_;
     const std::string& hostname_;
     const Http::Protocol protocol_;
     Network::Address::InstanceConstSharedPtr local_address_;
     bool expect_reset_{};
+    bool reuse_connection_ = false;
+    bool request_in_flight_ = false;
   };
 
   using HttpActiveHealthCheckSessionPtr = std::unique_ptr<HttpActiveHealthCheckSession>;