health check: use the ALPN-negotiated protocol for HTTP health checks

api/envoy/config/core/v3/health_check.proto

@@ -153,6 +153,10 @@ message HealthCheck {
     repeated type.v3.Int64Range retriable_statuses = 12;
 
     // Use specified application protocol for health checks.
+    //
+    // When the health check connection negotiates a protocol via ALPN, the negotiated protocol
+    // selects the codec instead and this field is only used when nothing is negotiated, i.e. for
+    // plaintext health checks and peers that do not support ALPN.
     type.v3.CodecClientType codec_client_type = 10 [(validate.rules).enum = {defined_only: true}];
 
     // An optional service name parameter which is used to validate the identity of

changelogs/current/minor_behavior_changes/health_check__alpn-negotiated-codec.rst

@@ -0,0 +1,9 @@
+HTTP health checks now select their codec from the protocol negotiated by ALPN, instead of always
+using :ref:`codec_client_type
+<envoy_v3_api_field_config.core.v3.HealthCheck.HttpHealthCheck.codec_client_type>`. A health check
+connection that negotiates ``h2`` is checked over HTTP/2 and one that negotiates ``http/1.1`` over
+HTTP/1.1; ``codec_client_type`` is now the value used when nothing is negotiated, i.e. for
+plaintext health checks and peers that do not do ALPN. The ALPN offered on health check
+connections is unchanged, and gRPC health checks are unaffected since gRPC requires HTTP/2. This
+behavior can be reverted by setting runtime guard
+``envoy.reloadable_features.health_check_use_negotiated_protocol`` to ``false``.

source/common/runtime/runtime_features.cc

@@ -74,6 +74,7 @@ RUNTIME_GUARD(envoy_reloadable_features_grpc_side_stream_flow_control);
 RUNTIME_GUARD(envoy_reloadable_features_happy_eyeballs_sort_non_ip_addresses);
 RUNTIME_GUARD(envoy_reloadable_features_header_mutation_url_encode_query_params);
 RUNTIME_GUARD(envoy_reloadable_features_health_check_after_cluster_warming);
+RUNTIME_GUARD(envoy_reloadable_features_health_check_use_negotiated_protocol);
 RUNTIME_GUARD(envoy_reloadable_features_hide_transport_failure_reason_in_response_body);
 RUNTIME_GUARD(envoy_reloadable_features_http1_close_connection_on_zombie_stream_complete);
 RUNTIME_GUARD(envoy_reloadable_features_http2_discard_host_header);

source/extensions/health_checkers/http/BUILD

@@ -19,6 +19,7 @@ envoy_cc_extension(
     deps = [
         "//source/common/http:codec_client_lib",
         "//source/common/http:response_decoder_impl_base",
+        "//source/common/http:utility_lib",
         "//source/common/upstream:health_checker_lib",
         "//source/common/upstream:host_utility_lib",
         "//source/extensions/health_checkers/common:health_checker_base_lib",

source/extensions/health_checkers/http/health_checker_impl.cc

@@ -19,6 +19,7 @@
 #include "source/common/grpc/common.h"
 #include "source/common/http/header_map_impl.h"
 #include "source/common/http/header_utility.h"
+#include "source/common/http/utility.h"
 #include "source/common/network/address_impl.h"
 #include "source/common/network/socket_impl.h"
 #include "source/common/network/utility.h"
@@ -43,6 +44,23 @@ getMethod(const envoy::config::core::v3::RequestMethod config_method) {
   return config_method;
 }
 
+bool useNegotiatedProtocol() {
+  return Runtime::runtimeFeatureEnabled(
+      "envoy.reloadable_features.health_check_use_negotiated_protocol");
+}
+
+// Maps the protocol negotiated by ALPN to a codec type, falling back to `default_codec_type` when
+// nothing was negotiated or the negotiated protocol is not one this health checker can speak.
+Http::CodecType codecTypeFromAlpn(absl::string_view alpn, Http::CodecType default_codec_type) {
+  if (alpn == Http::Utility::AlpnNames::get().Http11) {
+    return Http::CodecType::HTTP1;
+  }
+  if (alpn == Http::Utility::AlpnNames::get().Http2) {
+    return Http::CodecType::HTTP2;
+  }
+  return default_codec_type;
+}
+
 } // namespace
 
 Upstream::HealthCheckerSharedPtr HttpHealthCheckerFactory::createCustomHealthChecker(
@@ -200,7 +218,7 @@ Http::Protocol codecClientTypeToProtocol(Http::CodecType codec_client_type) {
   PANIC_DUE_TO_CORRUPT_ENUM
 }
 
-Http::Protocol HttpHealthCheckerImpl::protocol() const {
+Http::Protocol HttpHealthCheckerImpl::configuredProtocol() const {
   return codecClientTypeToProtocol(codec_client_type_);
 }
 
@@ -217,16 +235,28 @@ HttpHealthCheckerImpl::HttpActiveHealthCheckSession::HttpActiveHealthCheckSessio
 
 HttpHealthCheckerImpl::HttpActiveHealthCheckSession::~HttpActiveHealthCheckSession() {
   ASSERT(client_ == nullptr);
+  ASSERT(pending_connection_ == nullptr);
 }
 
 void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onDeferredDelete() {
+  resetPendingConnection();
   if (client_) {
     // If there is an active request it will get reset, so make sure we ignore the reset.
     expect_reset_ = true;
     client_->close(Network::ConnectionCloseType::Abort);
   }
 }
 
+void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::resetPendingConnection() {
+  if (pending_connection_ == nullptr) {
+    return;
+  }
+  pending_connection_->removeConnectionCallbacks(pending_connection_callback_impl_);
+  pending_connection_->close(Network::ConnectionCloseType::Abort);
+  pending_host_description_.reset();
+  parent_.dispatcher_.deferredDelete(std::move(pending_connection_));
+}
+
 void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::decodeHeaders(
     Http::ResponseHeaderMapPtr&& headers, bool end_stream) {
   ASSERT(!response_headers_);
@@ -269,6 +299,7 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onEvent(Network::Conne
 // TODO(lilika) : Support connection pooling
 void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onInterval() {
   if (!client_) {
+    ASSERT(pending_connection_ == nullptr);
     Upstream::Host::CreateConnectionData conn =
         host_->createHealthCheckConnection(parent_.dispatcher_, parent_.transportSocketOptions(),
                                            parent_.transportSocketMatchMetadata().get());
@@ -279,13 +310,90 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onInterval() {
       handleFailure(envoy::data::core::v3::NETWORK);
       return;
     }
-    client_.reset(parent_.createCodecClient(conn));
-    client_->addConnectionCallbacks(connection_callback_impl_);
-    client_->setCodecConnectionCallbacks(http_connection_callback_impl_);
+
+    // ALPN is only negotiated on secure transports, and a QUIC connection reports no negotiated
+    // protocol, so HTTP/3 has nothing to select a codec from. In every other case the configured
+    // codec is the only possible answer, so the codec client is created up front and the request
+    // is sent while the connection is still being established, as it has always been.
+    if (useNegotiatedProtocol() && parent_.codec_client_type_ != Http::CodecType::HTTP3 &&
+        conn.connection_->ssl() != nullptr) {
+      // Reset these before connecting: a leftover `expect_reset_` from a previous timeout would
+      // otherwise suppress the failure for this attempt.
+      expect_reset_ = false;
+      reuse_connection_ = parent_.reuse_connection_;
+      pending_host_description_ = conn.host_description_;
+      pending_connection_ = std::move(conn.connection_);
+      pending_connection_->addConnectionCallbacks(pending_connection_callback_impl_);
+      // Apply the connection settings that the codec client would otherwise have applied before
+      // connecting, so that the connect and the handshake behave as they did when the codec client
+      // was created up front.
+      pending_connection_->detectEarlyCloseWhenReadDisabled(false);
+      pending_connection_->noDelay(true);
+      // The codec is chosen from the negotiated protocol and the request is sent once the
+      // connection is established. See onPendingConnectionEvent().
+      pending_connection_->connect();
+      return;
+    }
+
+    attachCodecClient(conn, parent_.codec_client_type_);
     expect_reset_ = false;
     reuse_connection_ = parent_.reuse_connection_;
   }
 
+  sendRequest();
+}
+
+void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::attachCodecClient(
+    Upstream::Host::CreateConnectionData& data, Http::CodecType codec_type) {
+  client_.reset(parent_.createCodecClient(data, codec_type));
+  client_->addConnectionCallbacks(connection_callback_impl_);
+  client_->setCodecConnectionCallbacks(http_connection_callback_impl_);
+  protocol_ = codecClientTypeToProtocol(codec_type);
+}
+
+void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onPendingConnectionEvent(
+    Network::ConnectionEvent event) {
+  ASSERT(pending_connection_ != nullptr);
+
+  if (event == Network::ConnectionEvent::RemoteClose ||
+      event == Network::ConnectionEvent::LocalClose) {
+    ENVOY_CONN_LOG(debug, "connect failure reason={} health_flags={}", *pending_connection_,
+                   pending_connection_->transportFailureReason(),
+                   HostUtility::healthFlagsToString(*host_));
+    pending_connection_->removeConnectionCallbacks(pending_connection_callback_impl_);
+    pending_host_description_.reset();
+    parent_.dispatcher_.deferredDelete(std::move(pending_connection_));
+    if (!expect_reset_) {
+      // handleFailure() may deferred delete this session, so nothing may be touched afterwards.
+      handleFailure(envoy::data::core::v3::NETWORK);
+    }
+    return;
+  }
+
+  // Both Connected and ConnectedZeroRtt mean the handshake is done.
+  if (event != Network::ConnectionEvent::Connected &&
+      event != Network::ConnectionEvent::ConnectedZeroRtt) {
+    return;
+  }
+
+  // The negotiated protocol - if any - is now known. Anything other than a protocol this health
+  // checker can speak falls back to the configured codec.
+  const std::string alpn = pending_connection_->nextProtocol();
+  const Http::CodecType codec_type = codecTypeFromAlpn(alpn, parent_.codec_client_type_);
+  ENVOY_CONN_LOG(debug, "health check negotiated alpn='{}', using {}", *pending_connection_, alpn,
+                 Http::Utility::getProtocolString(codecClientTypeToProtocol(codec_type)));
+
+  // Hand the established connection over to a codec client. Adding and removing connection
+  // callbacks while this event is being delivered is safe, and the codec client will simply see
+  // the same Connected event once this returns.
+  pending_connection_->removeConnectionCallbacks(pending_connection_callback_impl_);
+  Upstream::Host::CreateConnectionData data{std::move(pending_connection_),
+                                            std::move(pending_host_description_)};
+  attachCodecClient(data, codec_type);
+  sendRequest();
+}
+
+void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::sendRequest() {
   Http::RequestEncoder* request_encoder = &client_->newStream(*this);
   request_encoder->getStream().addCallbacks(*this);
   request_in_flight_ = true;
@@ -474,6 +582,15 @@ bool HttpHealthCheckerImpl::HttpActiveHealthCheckSession::shouldClose() const {
 
 void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onTimeout() {
   request_in_flight_ = false;
+  if (pending_connection_) {
+    ENVOY_CONN_LOG(debug, "connect timeout health_flags={}", *pending_connection_,
+                   HostUtility::healthFlagsToString(*host_));
+    // The caller records the timeout as a failure. resetPendingConnection() detaches the callbacks
+    // before closing, so the close it triggers is not reported a second time.
+    resetPendingConnection();
+    return;
+  }
+
   if (client_) {
     ENVOY_CONN_LOG(debug, "connection/stream timeout health_flags={}", *client_,
                    HostUtility::healthFlagsToString(*host_));
@@ -500,10 +617,10 @@ HttpHealthCheckerImpl::codecClientType(const envoy::type::v3::CodecClientType& t
 }
 
 Http::CodecClient*
-ProdHttpHealthCheckerImpl::createCodecClient(Upstream::Host::CreateConnectionData& data) {
-  return new Http::CodecClientProd(codec_client_type_, std::move(data.connection_),
-                                   data.host_description_, dispatcher_, random_generator_,
-                                   transportSocketOptions());
+ProdHttpHealthCheckerImpl::createCodecClient(Upstream::Host::CreateConnectionData& data,
+                                             Http::CodecType codec_type) {
+  return new Http::CodecClientProd(codec_type, std::move(data.connection_), data.host_description_,
+                                   dispatcher_, random_generator_, transportSocketOptions());
 }
 
 } // namespace Upstream

source/extensions/health_checkers/http/health_checker_impl.h

@@ -51,8 +51,9 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase {
                         Server::Configuration::HealthCheckerFactoryContext& context,
                         HealthCheckEventLoggerPtr&& event_logger);
 
-  // Returns the HTTP protocol used for the health checker.
-  Http::Protocol protocol() const;
+  // Returns the HTTP protocol derived from `codec_client_type`. Note that a session whose codec was
+  // selected from the ALPN-negotiated protocol may be speaking something else.
+  Http::Protocol configuredProtocol() const;
 
   /**
    * Utility class checking if given http status matches configured expectations.
@@ -87,6 +88,14 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase {
     enum class HealthCheckResult { Succeeded, Degraded, Failed, Retriable };
     HealthCheckResult healthCheckResult(uint64_t response_code);
     bool shouldClose() const;
+    // Attaches a codec client of `codec_type` to `data`'s connection and wires up the callbacks.
+    void attachCodecClient(Upstream::Host::CreateConnectionData& data, Http::CodecType codec_type);
+    // Encodes the health check request on `client_`. Requires `client_ != nullptr`.
+    void sendRequest();
+    // Handles events on a connection that has not been handed to a codec client yet.
+    void onPendingConnectionEvent(Network::ConnectionEvent event);
+    // Aborts and disposes of `pending_connection_`, if any.
+    void resetPendingConnection();
 
     // ActiveHealthCheckSession
     void onInterval() override;
@@ -126,6 +135,23 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase {
       HttpActiveHealthCheckSession& parent_;
     };
 
+    // Callbacks for a connection that is still being established and has no codec client yet.
+    // Deliberately distinct from ConnectionCallbackImpl so that every codec-driven callback can
+    // continue to assume `client_ != nullptr`.
+    class PendingConnectionCallbackImpl : public Network::ConnectionCallbacks {
+    public:
+      PendingConnectionCallbackImpl(HttpActiveHealthCheckSession& parent) : parent_(parent) {}
+      // Network::ConnectionCallbacks
+      void onEvent(Network::ConnectionEvent event) override {
+        parent_.onPendingConnectionEvent(event);
+      }
+      void onAboveWriteBufferHighWatermark() override {}
+      void onBelowWriteBufferLowWatermark() override {}
+
+    private:
+      HttpActiveHealthCheckSession& parent_;
+    };
+
     class HttpConnectionCallbackImpl : public Http::ConnectionCallbacks {
     public:
       HttpConnectionCallbackImpl(HttpActiveHealthCheckSession& parent) : parent_(parent) {}
@@ -137,23 +163,31 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase {
     };
 
     ConnectionCallbackImpl connection_callback_impl_{*this};
+    PendingConnectionCallbackImpl pending_connection_callback_impl_{*this};
     HttpConnectionCallbackImpl http_connection_callback_impl_{*this};
     HttpHealthCheckerImpl& parent_;
     Http::CodecClientPtr client_;
+    // Set while a connection is being established and the codec has not been chosen yet. Mutually
+    // exclusive with `client_`.
+    Network::ClientConnectionPtr pending_connection_;
+    HostDescriptionConstSharedPtr pending_host_description_;
     Http::ResponseHeaderMapPtr response_headers_;
     Buffer::InstancePtr response_body_;
     const std::string& hostname_;
     Network::ConnectionInfoProviderSharedPtr local_connection_info_provider_;
     // Keep small members (bools and enums) at the end of class, to reduce alignment overhead.
-    const Http::Protocol protocol_;
+    // Not const: when the codec is chosen from the negotiated ALPN protocol this is updated to
+    // match what the connection actually speaks.
+    Http::Protocol protocol_;
     bool expect_reset_ : 1 = false;
     bool reuse_connection_ : 1 = false;
     bool request_in_flight_ : 1 = false;
   };
 
   using HttpActiveHealthCheckSessionPtr = std::unique_ptr<HttpActiveHealthCheckSession>;
 
-  virtual Http::CodecClient* createCodecClient(Upstream::Host::CreateConnectionData& data) PURE;
+  virtual Http::CodecClient* createCodecClient(Upstream::Host::CreateConnectionData& data,
+                                               Http::CodecType codec_type) PURE;
 
   // HealthCheckerImplBase
   ActiveHealthCheckSessionPtr makeSession(HostSharedPtr host) override {
@@ -188,7 +222,8 @@ class ProdHttpHealthCheckerImpl : public HttpHealthCheckerImpl {
   using HttpHealthCheckerImpl::HttpHealthCheckerImpl;
 
   // HttpHealthCheckerImpl
-  Http::CodecClient* createCodecClient(Upstream::Host::CreateConnectionData& data) override;
+  Http::CodecClient* createCodecClient(Upstream::Host::CreateConnectionData& data,
+                                       Http::CodecType codec_type) override;
 };
 
 } // namespace Upstream

test/common/upstream/BUILD

@@ -347,6 +347,7 @@ envoy_cc_test(
         "//test/mocks/protobuf:protobuf_mocks",
         "//test/mocks/runtime:runtime_mocks",
         "//test/mocks/server:health_checker_factory_context_mocks",
+        "//test/mocks/ssl:ssl_mocks",
         "//test/mocks/upstream:cluster_info_mocks",
         "//test/mocks/upstream:cluster_priority_set_mocks",
         "//test/mocks/upstream:health_check_event_logger_mocks",

test/common/upstream/health_check_fuzz.cc

@@ -142,8 +142,8 @@ void HttpHealthCheckFuzz::respond(test::common::upstream::Respond respond, bool
   response_headers->setStatus(status);
 
   // Responding with http can cause client to close, if so create a new one.
-  const bool client_will_close =
-      Http::HeaderUtility::shouldCloseConnection(health_checker_->protocol(), *response_headers);
+  const bool client_will_close = Http::HeaderUtility::shouldCloseConnection(
+      health_checker_->configuredProtocol(), *response_headers);
 
   // Check if there is a response body.
   bool has_response_body = !respond.http_respond().body().empty();