HTTP health checker: handle GOAWAY from HTTP2 upstreams

docs/root/version_history/current.rst

@@ -17,6 +17,9 @@ Bug Fixes
 ---------
 *Changes expected to improve the state of the world and are unlikely to have negative effects*
 
+* active health checks: health checks using a TLS transport socket and secrets delivered via :ref:`SDS <config_secret_discovery_service>` will now wait until secrets are loaded before the first health check attempt. This should improve startup times by not having to wait for the :ref:`no_traffic_interval <envoy_v3_api_field_config.core.v3.HealthCheck.no_traffic_interval>` until the next attempt.
+* 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.
+
 * http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests.
 
 Removed Config or Runtime

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;
+    received_no_error_goaway_ = false;
   }
 
   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,50 @@ 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_ && received_no_error_goaway_) {
+    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_));
+
+  // If GOAWAY handling is explicitly disabled via runtime, go back to the old
+  // behavior of ignoring GOAWAY completely.
+  if (parent_.runtime_.snapshot().featureEnabled("health_check.disable_goaway_handling", 0UL)) {
+    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.
+    received_no_error_goaway_ = true;
+    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 +356,8 @@ HttpHealthCheckerImpl::HttpActiveHealthCheckSession::healthCheckResult() {
 }
 
 void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onResponseComplete() {
+  request_in_flight_ = false;
+
   switch (healthCheckResult()) {
   case HealthCheckResult::Succeeded:
     handleSuccess(false);
@@ -345,7 +385,7 @@ bool HttpHealthCheckerImpl::HttpActiveHealthCheckSession::shouldClose() const {
     return false;
   }
 
-  if (!parent_.reuse_connection_) {
+  if (!parent_.reuse_connection_ || received_no_error_goaway_) {
     return true;
   }
 
@@ -375,6 +415,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,29 @@ 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_{};
+    // If true, we received a GOAWAY (NO_ERROR code) and are deferring closing the connection
+    // until the active probe completes.
+    bool received_no_error_goaway_ = false;
+    bool request_in_flight_ = false;
   };
 
   using HttpActiveHealthCheckSessionPtr = std::unique_ptr<HttpActiveHealthCheckSession>;