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

HTTP health checks negotiated ALPN whenever TLS was in use and then
discarded the result, always speaking whatever `codec_client_type` said.
That made heterogeneous and mid-migration upstreams impossible to health
check with a single configuration, and let contradictory config
(`codec_client_type: HTTP2` with `tls_options.alpn_protocols:
["http/1.1"]`) load cleanly and then fail forever.

Use the negotiated protocol to select the health check codec, with
`codec_client_type` retained as the value used when nothing is
negotiated, i.e. plaintext health checks and peers that do not do ALPN.
A protocol this health checker cannot speak also falls back to
`codec_client_type`, so upstreams that route health checks by a custom
ALPN identifier are unaffected.

The ALPN offered on health check connections is deliberately left
unchanged, so no configuration that works today changes behavior on the
wire.

Since the codec can only be chosen once the handshake has completed, a
TLS health check connection is now connected first and the codec client
attached on the Connected event, mirroring what `HttpConnPoolImplMixed`
already does on the data plane. Plaintext and HTTP/3 health checks keep
creating the codec client up front.

Guarded by
`envoy.reloadable_features.health_check_use_negotiated_protocol`.

Fixes #46848

Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <huabing@tetrate.io>

Author: zhaohuabing

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,30 @@ HttpHealthCheckerImpl::HttpActiveHealthCheckSession::HttpActiveHealthCheckSessio
 
 HttpHealthCheckerImpl::HttpActiveHealthCheckSession::~HttpActiveHealthCheckSession() {
   ASSERT(client_ == nullptr);
+  ASSERT(pending_connection_ == nullptr);
 }
 
 void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onDeferredDelete() {
+  // If there is an active request or a connection being established it will get reset, so make
+  // sure we ignore the reset. This must be set before closing, since close() raises LocalClose
+  // synchronously.
+  expect_reset_ = true;
+  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 +301,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 +312,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 +584,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 +619,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();