health check: gracefully handle GOAWAY in grpc health checker

include/envoy/http/codec.h

@@ -36,6 +36,14 @@ const char MaxResponseHeadersCountOverrideKey[] =
 
 class Stream;
 
+/**
+ * Error codes used to convey the reason for a GOAWAY.
+ */
+enum class GoAwayErrorCode {
+  NoError,
+  Other,
+};
+
 /**
  * Stream encoder options specific to HTTP/1.
  */
@@ -324,7 +332,7 @@ class ConnectionCallbacks {
   /**
    * Fires when the remote indicates "go away." No new streams should be created.
    */
-  virtual void onGoAway() PURE;
+  virtual void onGoAway(GoAwayErrorCode error_code) PURE;
 };
 
 /**

source/common/http/codec_client.h

@@ -131,9 +131,9 @@ class CodecClient : Logger::Loggable<Logger::Id::client>,
               Upstream::HostDescriptionConstSharedPtr host, Event::Dispatcher& dispatcher);
 
   // Http::ConnectionCallbacks
-  void onGoAway() override {
+  void onGoAway(GoAwayErrorCode error_code) override {
     if (codec_callbacks_) {
-      codec_callbacks_->onGoAway();
+      codec_callbacks_->onGoAway(error_code);
     }
   }
 

source/common/http/conn_manager_impl.cc

@@ -464,7 +464,7 @@ void ConnectionManagerImpl::doConnectionClose(
   }
 }
 
-void ConnectionManagerImpl::onGoAway() {
+void ConnectionManagerImpl::onGoAway(GoAwayErrorCode) {
   // Currently we do nothing with remote go away frames. In the future we can decide to no longer
   // push resources if applicable.
 }

source/common/http/conn_manager_impl.h

@@ -84,7 +84,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
   void initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callbacks) override;
 
   // Http::ConnectionCallbacks
-  void onGoAway() override;
+  void onGoAway(GoAwayErrorCode error_code) override;
 
   // Http::ServerConnectionCallbacks
   RequestDecoder& newStream(ResponseEncoder& response_encoder,

source/common/http/http2/codec_impl.cc

@@ -563,6 +563,16 @@ int ConnectionImpl::onBeforeFrameReceived(const nghttp2_frame_hd* hd) {
   return 0;
 }
 
+ABSL_MUST_USE_RESULT
+enum GoAwayErrorCode ngHttp2ErrorCodeToErrorCode(uint32_t code) noexcept {
+  switch (code) {
+  case NGHTTP2_NO_ERROR:
+    return GoAwayErrorCode::NoError;
+  default:
+    return GoAwayErrorCode::Other;
+  }
+}
+
 int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) {
   ENVOY_CONN_LOG(trace, "recv frame type={}", connection_, static_cast<uint64_t>(frame->hd.type));
 
@@ -579,10 +589,11 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) {
 
   // Only raise GOAWAY once, since we don't currently expose stream information. Shutdown
   // notifications are the same as a normal GOAWAY.
+  // TODO: handle multiple GOAWAY frames.
   if (frame->hd.type == NGHTTP2_GOAWAY && !raised_goaway_) {
     ASSERT(frame->hd.stream_id == 0);
     raised_goaway_ = true;
-    callbacks().onGoAway();
+    callbacks().onGoAway(ngHttp2ErrorCodeToErrorCode(frame->goaway.error_code));
     return 0;
   }
 

source/common/http/http2/conn_pool.cc

@@ -24,7 +24,7 @@ ConnPoolImpl::~ConnPoolImpl() { destructAllConnections(); }
 ConnPoolImplBase::ActiveClientPtr ConnPoolImpl::instantiateActiveClient() {
   return std::make_unique<ActiveClient>(*this);
 }
-void ConnPoolImpl::onGoAway(ActiveClient& client) {
+void ConnPoolImpl::onGoAway(ActiveClient& client, Http::GoAwayErrorCode) {
   ENVOY_CONN_LOG(debug, "remote goaway", *client.codec_client_);
   host_->cluster().stats().upstream_cx_close_notify_.inc();
   if (client.state_ != ActiveClient::State::DRAINING) {

source/common/http/http2/conn_pool.h

@@ -52,14 +52,16 @@ class ConnPoolImpl : public ConnPoolImplBase {
     }
 
     // Http::ConnectionCallbacks
-    void onGoAway() override { parent().onGoAway(*this); }
+    void onGoAway(Http::GoAwayErrorCode error_code) override {
+      parent().onGoAway(*this, error_code);
+    }
 
     bool closed_with_active_rq_{};
   };
 
   uint64_t maxRequestsPerConnection();
   void movePrimaryClientToDraining();
-  void onGoAway(ActiveClient& client);
+  void onGoAway(ActiveClient& client, Http::GoAwayErrorCode error_code);
   void onStreamDestroy(ActiveClient& client);
   void onStreamReset(ActiveClient& client, Http::StreamResetReason reason);
 

source/common/upstream/health_checker_impl.cc

@@ -703,19 +703,22 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onInterval() {
 void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onResetStream(Http::StreamResetReason,
                                                                         absl::string_view) {
   const bool expected_reset = expect_reset_;
+  const bool goaway = received_no_error_goaway_;
   resetState();
 
   if (expected_reset) {
     // Stream reset was initiated by us (bogus gRPC response, timeout or cluster host is going
-    // away). In these cases health check failure has already been reported, so just return.
+    // away). In these cases health check failure has already been reported and a GOAWAY (if any)
+    // has already been handled, so just return.
     return;
   }
 
   ENVOY_CONN_LOG(debug, "connection/stream error health_flags={}", *client_,
                  HostUtility::healthFlagsToString(*host_));
 
-  if (!parent_.reuse_connection_) {
-    // Stream reset was unexpected, so we haven't closed the connection yet.
+  if (goaway || !parent_.reuse_connection_) {
+    // Stream reset was unexpected, so we haven't closed the connection
+    // yet in response to a GOAWAY or due to disabled connection reuse.
     client_->close();
   }
 
@@ -727,9 +730,19 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onResetStream(Http::St
   handleFailure(envoy::data::core::v3::NETWORK);
 }
 
-void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onGoAway() {
+void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onGoAway(
+    Http::GoAwayErrorCode error_code) {
   ENVOY_CONN_LOG(debug, "connection going away health_flags={}", *client_,
                  HostUtility::healthFlagsToString(*host_));
+  // If we have an active health check probe and receive a GOAWAY indicating
+  // graceful shutdown, allow the probe to complete before closing the connection.
+  // The connection will be closed when the active check completes or another
+  // terminal condition occurs, such as a timeout or stream reset.
+  if (request_encoder_ && error_code == Http::GoAwayErrorCode::NoError) {
+    received_no_error_goaway_ = true;
+    return;
+  }
+
   // Even if we have active health check probe, fail it on GOAWAY and schedule new one.
   if (request_encoder_) {
     handleFailure(envoy::data::core::v3::NETWORK);
@@ -762,6 +775,9 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onRpcComplete(
     handleFailure(envoy::data::core::v3::ACTIVE);
   }
 
+  // Read the value as we may call resetState() and clear it.
+  const bool goaway = received_no_error_goaway_;
+
   // |end_stream| will be false if we decided to stop healthcheck before HTTP stream has ended -
   // invalid gRPC payload, unexpected message stream or wrong content-type.
   if (end_stream) {
@@ -772,7 +788,7 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onRpcComplete(
     request_encoder_->getStream().resetStream(Http::StreamResetReason::LocalReset);
   }
 
-  if (!parent_.reuse_connection_) {
+  if (!parent_.reuse_connection_ || goaway) {
     client_->close();
   }
 }
@@ -782,13 +798,14 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::resetState() {
   request_encoder_ = nullptr;
   decoder_ = Grpc::Decoder();
   health_check_response_.reset();
+  received_no_error_goaway_ = false;
 }
 
 void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onTimeout() {
   ENVOY_CONN_LOG(debug, "connection/stream timeout health_flags={}", *client_,
                  HostUtility::healthFlagsToString(*host_));
   expect_reset_ = true;
-  if (!parent_.reuse_connection_) {
+  if (received_no_error_goaway_ || !parent_.reuse_connection_) {
     client_->close();
   } else {
     request_encoder_->getStream().resetStream(Http::StreamResetReason::LocalReset);

source/common/upstream/health_checker_impl.h

@@ -323,7 +323,7 @@ class GrpcHealthCheckerImpl : public HealthCheckerImplBase {
     void onBelowWriteBufferLowWatermark() override {}
 
     void onEvent(Network::ConnectionEvent event);
-    void onGoAway();
+    void onGoAway(Http::GoAwayErrorCode error_code);
 
     class ConnectionCallbackImpl : public Network::ConnectionCallbacks {
     public:
@@ -341,7 +341,7 @@ class GrpcHealthCheckerImpl : public HealthCheckerImplBase {
     public:
       HttpConnectionCallbackImpl(GrpcActiveHealthCheckSession& parent) : parent_(parent) {}
       // Http::ConnectionCallbacks
-      void onGoAway() override { parent_.onGoAway(); }
+      void onGoAway(Http::GoAwayErrorCode error_code) override { parent_.onGoAway(error_code); }
 
     private:
       GrpcActiveHealthCheckSession& parent_;
@@ -358,6 +358,9 @@ class GrpcHealthCheckerImpl : public HealthCheckerImplBase {
     // e.g. remote reset. In this case healthcheck status has already been reported, only state
     // cleanup is required.
     bool expect_reset_ = false;
+    // 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;
   };
 
   virtual Http::CodecClientPtr createCodecClient(Upstream::Host::CreateConnectionData& data) PURE;

source/extensions/quic_listeners/quiche/envoy_quic_client_session.cc

@@ -1,5 +1,7 @@
 #include "extensions/quic_listeners/quiche/envoy_quic_client_session.h"
 
+#include "extensions/quic_listeners/quiche/envoy_quic_utils.h"
+
 namespace Envoy {
 namespace Quic {
 
@@ -51,7 +53,7 @@ void EnvoyQuicClientSession::OnGoAway(const quic::QuicGoAwayFrame& frame) {
                  quic::QuicErrorCodeToString(frame.error_code), frame.reason_phrase);
   quic::QuicSpdyClientSession::OnGoAway(frame);
   if (http_connection_callbacks_ != nullptr) {
-    http_connection_callbacks_->onGoAway();
+    http_connection_callbacks_->onGoAway(quicErrorCodeToEnvoyErrorCode(frame.error_code));
   }
 }
 

source/extensions/quic_listeners/quiche/envoy_quic_utils.cc

@@ -90,6 +90,15 @@ Http::StreamResetReason quicErrorCodeToEnvoyResetReason(quic::QuicErrorCode erro
   }
 }
 
+Http::GoAwayErrorCode quicErrorCodeToEnvoyErrorCode(quic::QuicErrorCode error) noexcept {
+  switch (error) {
+  case quic::QUIC_NO_ERROR:
+    return Http::GoAwayErrorCode::NoError;
+  default:
+    return Http::GoAwayErrorCode::Other;
+  }
+}
+
 Network::ConnectionSocketPtr
 createConnectionSocket(Network::Address::InstanceConstSharedPtr& peer_addr,
                        Network::Address::InstanceConstSharedPtr& local_addr,

source/extensions/quic_listeners/quiche/envoy_quic_utils.h

@@ -69,6 +69,10 @@ Http::StreamResetReason quicRstErrorToEnvoyResetReason(quic::QuicRstStreamErrorC
 // Called when underlying QUIC connection is closed either locally or by peer.
 Http::StreamResetReason quicErrorCodeToEnvoyResetReason(quic::QuicErrorCode error);
 
+// Called when a GOAWAY frame is received.
+ABSL_MUST_USE_RESULT
+Http::GoAwayErrorCode quicErrorCodeToEnvoyErrorCode(quic::QuicErrorCode error) noexcept;
+
 // Create a connection socket instance and apply given socket options to the
 // socket. IP_PKTINFO and SO_RXQ_OVFL is always set if supported.
 Network::ConnectionSocketPtr

test/common/http/common.h

@@ -28,7 +28,7 @@ class CodecClientForTest : public Http::CodecClient {
       destroy_cb_(this);
     }
   }
-  void raiseGoAway() { onGoAway(); }
+  void raiseGoAway(Http::GoAwayErrorCode error_code) { onGoAway(error_code); }
   Event::Timer* idleTimer() { return idle_timer_.get(); }
 
   DestroyCb destroy_cb_;

test/common/http/http2/codec_impl_test.cc

@@ -317,7 +317,7 @@ TEST_P(Http2CodecImplTest, ShutdownNotice) {
   EXPECT_CALL(request_decoder_, decodeHeaders_(_, true));
   request_encoder_->encodeHeaders(request_headers, true);
 
-  EXPECT_CALL(client_callbacks_, onGoAway());
+  EXPECT_CALL(client_callbacks_, onGoAway(_));
   server_->shutdownNotice();
   server_->goAway();
 
@@ -1456,7 +1456,7 @@ TEST_P(Http2CodecImplTest, LargeRequestHeadersExceedPerHeaderLimit) {
   request_headers.addCopy("big", long_string);
 
   EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)).Times(0);
-  EXPECT_CALL(client_callbacks_, onGoAway());
+  EXPECT_CALL(client_callbacks_, onGoAway(_));
   server_->shutdownNotice();
   server_->goAway();
   request_encoder_->encodeHeaders(request_headers, true);

test/common/http/http2/conn_pool_test.cc

@@ -971,7 +971,7 @@ TEST_F(Http2ConnPoolImplTest, GoAway) {
   r1.inner_decoder_->decodeHeaders(
       ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true);
 
-  test_clients_[0].codec_client_->raiseGoAway();
+  test_clients_[0].codec_client_->raiseGoAway(Http::GoAwayErrorCode::NoError);
 
   expectClientCreate();
   ActiveTestRequest r2(*this, 1, false);