From 614a933b43b62a738421e54007d8f9368b115b99 Mon Sep 17 00:00:00 2001 From: "Huabing (Robin) Zhao" Date: Sun, 23 Aug 2026 19:47:14 -0700 Subject: [PATCH] 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 Signed-off-by: Huabing (Robin) Zhao --- api/envoy/config/core/v3/health_check.proto | 4 + .../health_check__alpn-negotiated-codec.rst | 9 + source/common/runtime/runtime_features.cc | 1 + source/extensions/health_checkers/http/BUILD | 1 + .../http/health_checker_impl.cc | 133 ++++- .../http/health_checker_impl.h | 45 +- test/common/upstream/BUILD | 1 + test/common/upstream/health_check_fuzz.cc | 4 +- .../upstream/health_check_fuzz_test_utils.cc | 42 +- .../upstream/health_check_fuzz_test_utils.h | 8 +- .../upstream/health_checker_impl_test.cc | 477 +++++++++++++++++- test/integration/BUILD | 1 + .../health_check_integration_test.cc | 119 ++++- 13 files changed, 779 insertions(+), 66 deletions(-) create mode 100644 changelogs/current/minor_behavior_changes/health_check__alpn-negotiated-codec.rst diff --git a/api/envoy/config/core/v3/health_check.proto b/api/envoy/config/core/v3/health_check.proto index a4ed6e9181898..6aaeb1db59e5e 100644 --- a/api/envoy/config/core/v3/health_check.proto +++ b/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 diff --git a/changelogs/current/minor_behavior_changes/health_check__alpn-negotiated-codec.rst b/changelogs/current/minor_behavior_changes/health_check__alpn-negotiated-codec.rst new file mode 100644 index 0000000000000..de2e56c8b6a80 --- /dev/null +++ b/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 +`. 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``. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index a34d868adcc5e..f8d136c03c003 100644 --- a/source/common/runtime/runtime_features.cc +++ b/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); diff --git a/source/extensions/health_checkers/http/BUILD b/source/extensions/health_checkers/http/BUILD index b759a789d7568..d9fef80aedec7 100644 --- a/source/extensions/health_checkers/http/BUILD +++ b/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", diff --git a/source/extensions/health_checkers/http/health_checker_impl.cc b/source/extensions/health_checkers/http/health_checker_impl.cc index 9c5da0df55ea8..6c65242d030fb 100644 --- a/source/extensions/health_checkers/http/health_checker_impl.cc +++ b/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,9 +235,11 @@ 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; @@ -227,6 +247,16 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onDeferredDelete() { } } +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 diff --git a/source/extensions/health_checkers/http/health_checker_impl.h b/source/extensions/health_checkers/http/health_checker_impl.h index 9fce3c06b090b..953b72a7d9db2 100644 --- a/source/extensions/health_checkers/http/health_checker_impl.h +++ b/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,15 +163,22 @@ 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; @@ -153,7 +186,8 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase { using HttpActiveHealthCheckSessionPtr = std::unique_ptr; - 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 diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD index 30846bdcd303b..59eb929df9a82 100644 --- a/test/common/upstream/BUILD +++ b/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", diff --git a/test/common/upstream/health_check_fuzz.cc b/test/common/upstream/health_check_fuzz.cc index d31a4d3660eb8..c95ee13ed98e9 100644 --- a/test/common/upstream/health_check_fuzz.cc +++ b/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(); diff --git a/test/common/upstream/health_check_fuzz_test_utils.cc b/test/common/upstream/health_check_fuzz_test_utils.cc index 178cacbd7a89d..81661f7fa8339 100644 --- a/test/common/upstream/health_check_fuzz_test_utils.cc +++ b/test/common/upstream/health_check_fuzz_test_utils.cc @@ -35,27 +35,27 @@ void HttpHealthCheckerImplTestBase::expectClientCreate( connection_index_.pop_front(); return test_sessions_[index]->client_connection_; })); - EXPECT_CALL(*health_checker_, createCodecClient_(_)) - .WillRepeatedly( - Invoke([&](Upstream::Host::CreateConnectionData& conn_data) -> Http::CodecClient* { - if (!health_check_map.empty()) { - const auto& health_check_config = - health_check_map.at(conn_data.host_description_->address()->asString()); - // To make sure health checker checks the correct port. - EXPECT_EQ(health_check_config.port_value(), - conn_data.host_description_->healthCheckAddress()->ip()->port()); - } - uint32_t index = codec_index_.front(); - codec_index_.pop_front(); - TestSession& test_session = *test_sessions_[index]; - std::shared_ptr cluster{ - new NiceMock()}; - Event::MockDispatcher dispatcher_; - test_session.codec_client_ = new CodecClientForTest( - Http::CodecType::HTTP1, std::move(conn_data.connection_), test_session.codec_, - nullptr, Upstream::makeTestHost(cluster, "tcp://127.0.0.1:9000"), dispatcher_); - return test_session.codec_client_; - })); + EXPECT_CALL(*health_checker_, createCodecClient_(_, _)) + .WillRepeatedly(Invoke([&](Upstream::Host::CreateConnectionData& conn_data, + Http::CodecType) -> Http::CodecClient* { + if (!health_check_map.empty()) { + const auto& health_check_config = + health_check_map.at(conn_data.host_description_->address()->asString()); + // To make sure health checker checks the correct port. + EXPECT_EQ(health_check_config.port_value(), + conn_data.host_description_->healthCheckAddress()->ip()->port()); + } + uint32_t index = codec_index_.front(); + codec_index_.pop_front(); + TestSession& test_session = *test_sessions_[index]; + std::shared_ptr cluster{ + new NiceMock()}; + Event::MockDispatcher dispatcher_; + test_session.codec_client_ = new CodecClientForTest( + Http::CodecType::HTTP1, std::move(conn_data.connection_), test_session.codec_, nullptr, + Upstream::makeTestHost(cluster, "tcp://127.0.0.1:9000"), dispatcher_); + return test_session.codec_client_; + })); } void HttpHealthCheckerImplTestBase::expectStreamCreate(size_t index) { diff --git a/test/common/upstream/health_check_fuzz_test_utils.h b/test/common/upstream/health_check_fuzz_test_utils.h index 3812305ffc8a0..135a66d8e08cc 100644 --- a/test/common/upstream/health_check_fuzz_test_utils.h +++ b/test/common/upstream/health_check_fuzz_test_utils.h @@ -32,12 +32,14 @@ class TestHttpHealthCheckerImpl : public HttpHealthCheckerImpl { public: using HttpHealthCheckerImpl::HttpHealthCheckerImpl; - Http::CodecClient* createCodecClient(Upstream::Host::CreateConnectionData& conn_data) override { - return createCodecClient_(conn_data); + Http::CodecClient* createCodecClient(Upstream::Host::CreateConnectionData& conn_data, + Http::CodecType codec_type) override { + return createCodecClient_(conn_data, codec_type); }; // HttpHealthCheckerImpl - MOCK_METHOD(Http::CodecClient*, createCodecClient_, (Upstream::Host::CreateConnectionData&)); + MOCK_METHOD(Http::CodecClient*, createCodecClient_, + (Upstream::Host::CreateConnectionData&, Http::CodecType)); Http::CodecType codecClientType() { return codec_client_type_; } }; diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index 59de57a0e205f..704fe8d6a5a6a 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -34,6 +34,7 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/server/health_checker_factory_context.h" +#include "test/mocks/ssl/mocks.h" #include "test/mocks/upstream/cluster_info.h" #include "test/mocks/upstream/cluster_priority_set.h" #include "test/mocks/upstream/health_check_event_logger.h" @@ -106,12 +107,14 @@ class TestHttpHealthCheckerImpl : public HttpHealthCheckerImpl { public: using HttpHealthCheckerImpl::HttpHealthCheckerImpl; - Http::CodecClient* createCodecClient(Upstream::Host::CreateConnectionData& conn_data) override { - return createCodecClient_(conn_data); + Http::CodecClient* createCodecClient(Upstream::Host::CreateConnectionData& conn_data, + Http::CodecType codec_type) override { + return createCodecClient_(conn_data, codec_type); }; // HttpHealthCheckerImpl - MOCK_METHOD(Http::CodecClient*, createCodecClient_, (Upstream::Host::CreateConnectionData&)); + MOCK_METHOD(Http::CodecClient*, createCodecClient_, + (Upstream::Host::CreateConnectionData&, Http::CodecType)); Http::CodecType codecClientType() { return codec_client_type_; } }; @@ -129,6 +132,8 @@ class HttpHealthCheckerImplTest : public Event::TestUsingSimulatedTime, NiceMock request_encoder_; Http::ResponseDecoder* stream_response_callbacks_{}; CodecClientForTest* codec_client_{}; + // The codec type the health checker asked for when creating the codec client. + Http::CodecType requested_codec_type_{Http::CodecType::HTTP1}; }; using TestSessionPtr = std::unique_ptr; @@ -683,27 +688,28 @@ class HttpHealthCheckerImplTest : public Event::TestUsingSimulatedTime, connection_index_.pop_front(); return test_sessions_[index]->client_connection_; })); - EXPECT_CALL(*health_checker_, createCodecClient_(_)) - .WillRepeatedly( - Invoke([&](Upstream::Host::CreateConnectionData& conn_data) -> Http::CodecClient* { - if (!health_check_map.empty()) { - const auto& health_check_config = - health_check_map.at(conn_data.host_description_->address()->asString()); - // To make sure health checker checks the correct port. - EXPECT_EQ(health_check_config.port_value(), - conn_data.host_description_->healthCheckAddress()->ip()->port()); - } - const uint32_t index = codec_index_.front(); - codec_index_.pop_front(); - TestSession& test_session = *test_sessions_[index]; - std::shared_ptr cluster{ - new NiceMock()}; - Event::MockDispatcher dispatcher_; - test_session.codec_client_ = new CodecClientForTest( - Http::CodecType::HTTP1, std::move(conn_data.connection_), test_session.codec_, - nullptr, Upstream::makeTestHost(cluster, "tcp://127.0.0.1:9000"), dispatcher_); - return test_session.codec_client_; - })); + EXPECT_CALL(*health_checker_, createCodecClient_(_, _)) + .WillRepeatedly(Invoke([&](Upstream::Host::CreateConnectionData& conn_data, + Http::CodecType codec_type) -> Http::CodecClient* { + if (!health_check_map.empty()) { + const auto& health_check_config = + health_check_map.at(conn_data.host_description_->address()->asString()); + // To make sure health checker checks the correct port. + EXPECT_EQ(health_check_config.port_value(), + conn_data.host_description_->healthCheckAddress()->ip()->port()); + } + const uint32_t index = codec_index_.front(); + codec_index_.pop_front(); + TestSession& test_session = *test_sessions_[index]; + test_session.requested_codec_type_ = codec_type; + std::shared_ptr cluster{ + new NiceMock()}; + Event::MockDispatcher dispatcher_; + test_session.codec_client_ = new CodecClientForTest( + Http::CodecType::HTTP1, std::move(conn_data.connection_), test_session.codec_, + nullptr, Upstream::makeTestHost(cluster, "tcp://127.0.0.1:9000"), dispatcher_); + return test_session.codec_client_; + })); } void expectStreamCreate(size_t index) { @@ -771,6 +777,30 @@ class HttpHealthCheckerImplTest : public Event::TestUsingSimulatedTime, void expectSessionCreate() { expectSessionCreate(health_checker_map_); } void expectClientCreate(size_t index) { expectClientCreate(index, health_checker_map_); } + // Makes the connection for `index` look like a TLS connection that negotiates `alpn`. The health + // checker then defers creating the codec client until the handshake completes, so that the codec + // can be chosen from the negotiated protocol. + void expectTlsConnection(size_t index, const std::string& alpn) { + TestSession& test_session = *test_sessions_[index]; + Ssl::ConnectionInfoConstSharedPtr ssl_info = + std::make_shared>(); + ON_CALL(*test_session.client_connection_, ssl()).WillByDefault(Return(ssl_info)); + ON_CALL(*test_session.client_connection_, nextProtocol()).WillByDefault(Return(alpn)); + } + + // Completes the handshake on the connection for `index`, which is when the codec client is + // created and the health check request is sent. + void completeHandshake(size_t index) { + test_sessions_[index]->client_connection_->raiseEvent(Network::ConnectionEvent::Connected); + } + + // The codec mock for `index` is normally owned by the codec client created for that session. In + // tests where the handshake never completes no codec client is created, so nothing takes + // ownership of it. + void expectNoCodecClient(size_t index) { + testing::Mock::AllowLeak(test_sessions_[index]->codec_); + } + void expectSuccessStartFailedFailFirst( const std::optional& health_checked_cluster = std::optional()) { cluster_->prioritySet().getMockHostSet(0)->hosts_ = { @@ -1611,6 +1641,401 @@ TEST_F(HttpHealthCheckerImplTest, TlsOptions) { health_checker_->start(); } +// A connection that negotiates h2 is health checked over HTTP/2 even though HTTP/1.1 is +// configured. +TEST_F(HttpHealthCheckerImplTest, AlpnNegotiatedHttp2WithHttp1Configured) { + setupNoServiceValidationHC(); + EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Unchanged)); + + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + cluster_->info_->trafficStats()->upstream_cx_total_.inc(); + expectSessionCreate(); + expectTlsConnection(0, "h2"); + expectStreamCreate(0); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + health_checker_->start(); + + // The codec cannot be chosen until the handshake completes. + EXPECT_EQ(nullptr, test_sessions_[0]->codec_client_); + completeHandshake(0); + EXPECT_EQ(Http::CodecType::HTTP2, test_sessions_[0]->requested_codec_type_); + + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.max_interval", _)); + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.min_interval", _)) + .WillOnce(Return(45000)); + EXPECT_CALL(*test_sessions_[0]->interval_timer_, + enableTimer(std::chrono::milliseconds(45000), _)); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()); + respond(0, "200", false, false, true); + EXPECT_EQ(Host::Health::Healthy, + cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->coarseHealth()); +} + +// The mirror image: a connection that negotiates http/1.1 is health checked over HTTP/1.1 even +// though HTTP/2 is configured. +TEST_F(HttpHealthCheckerImplTest, AlpnNegotiatedHttp1WithHttp2Configured) { + setupNoServiceValidationHCWithHttp2(); + EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Unchanged)); + + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + cluster_->info_->trafficStats()->upstream_cx_total_.inc(); + expectSessionCreate(); + expectTlsConnection(0, "http/1.1"); + expectStreamCreate(0); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + health_checker_->start(); + + completeHandshake(0); + EXPECT_EQ(Http::CodecType::HTTP1, test_sessions_[0]->requested_codec_type_); + + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.max_interval", _)); + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.min_interval", _)) + .WillOnce(Return(45000)); + EXPECT_CALL(*test_sessions_[0]->interval_timer_, + enableTimer(std::chrono::milliseconds(45000), _)); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()); + respond(0, "200", false, false, true); + EXPECT_EQ(Host::Health::Healthy, + cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->coarseHealth()); +} + +// A peer that does not do ALPN falls back to the configured codec. +TEST_F(HttpHealthCheckerImplTest, AlpnNotNegotiatedFallsBackToConfiguredCodec) { + setupNoServiceValidationHCWithHttp2(); + EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Unchanged)); + + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + cluster_->info_->trafficStats()->upstream_cx_total_.inc(); + expectSessionCreate(); + expectTlsConnection(0, ""); + expectStreamCreate(0); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + health_checker_->start(); + + completeHandshake(0); + EXPECT_EQ(Http::CodecType::HTTP2, test_sessions_[0]->requested_codec_type_); + + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.max_interval", _)); + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.min_interval", _)) + .WillOnce(Return(45000)); + EXPECT_CALL(*test_sessions_[0]->interval_timer_, + enableTimer(std::chrono::milliseconds(45000), _)); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()); + respond(0, "200", false, false, true); + EXPECT_EQ(Host::Health::Healthy, + cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->coarseHealth()); +} + +// A protocol this health checker cannot speak also falls back to the configured codec. +TEST_F(HttpHealthCheckerImplTest, AlpnNegotiatedUnknownProtocolFallsBackToConfiguredCodec) { + setupNoServiceValidationHC(); + EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Unchanged)); + + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + cluster_->info_->trafficStats()->upstream_cx_total_.inc(); + expectSessionCreate(); + expectTlsConnection(0, "h3"); + expectStreamCreate(0); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + health_checker_->start(); + + completeHandshake(0); + EXPECT_EQ(Http::CodecType::HTTP1, test_sessions_[0]->requested_codec_type_); + + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.max_interval", _)); + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.min_interval", _)) + .WillOnce(Return(45000)); + EXPECT_CALL(*test_sessions_[0]->interval_timer_, + enableTimer(std::chrono::milliseconds(45000), _)); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()); + respond(0, "200", false, false, true); + EXPECT_EQ(Host::Health::Healthy, + cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->coarseHealth()); +} + +// A plaintext connection cannot negotiate anything, so the codec client is created up front and +// the request is sent while the connection is still being established, as it always has been. +TEST_F(HttpHealthCheckerImplTest, PlaintextConnectionUsesConfiguredCodecUpFront) { + setupNoServiceValidationHCWithHttp2(); + EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Unchanged)); + + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + cluster_->info_->trafficStats()->upstream_cx_total_.inc(); + expectSessionCreate(); + // No ssl() on the connection, and nextProtocol() is never consulted. + EXPECT_CALL(*test_sessions_[0]->client_connection_, nextProtocol()).Times(0); + expectStreamCreate(0); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + health_checker_->start(); + + // No Connected event was needed. + ASSERT_NE(nullptr, test_sessions_[0]->codec_client_); + EXPECT_EQ(Http::CodecType::HTTP2, test_sessions_[0]->requested_codec_type_); + + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.max_interval", _)); + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.min_interval", _)) + .WillOnce(Return(45000)); + EXPECT_CALL(*test_sessions_[0]->interval_timer_, + enableTimer(std::chrono::milliseconds(45000), _)); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()); + respond(0, "200", false, false, true); + EXPECT_EQ(Host::Health::Healthy, + cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->coarseHealth()); +} + +// An HTTP/3 health check keeps creating the codec client up front: the QUIC stack picks the ALPN +// protocol itself. +TEST_F(HttpHealthCheckerImplTest, Http3ConfiguredCreatesCodecClientUpFront) { + const std::string yaml = R"EOF( + timeout: 1s + interval: 1s + no_traffic_interval: 5s + unhealthy_threshold: 2 + healthy_threshold: 2 + http_health_check: + path: /healthcheck + codec_client_type: Http3 + )EOF"; + allocHealthChecker(yaml); + addCompletionCallback(); + + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + expectSessionCreate(); + expectTlsConnection(0, "h2"); + EXPECT_CALL(*test_sessions_[0]->client_connection_, nextProtocol()).Times(0); + expectStreamCreate(0); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + health_checker_->start(); + + ASSERT_NE(nullptr, test_sessions_[0]->codec_client_); + EXPECT_EQ(Http::CodecType::HTTP3, test_sessions_[0]->requested_codec_type_); +} + +// When the guard is disabled the negotiated protocol is ignored and the codec client is created up +// front, as it was before. +TEST_F(HttpHealthCheckerImplTest, AlpnNegotiationDisabledByRuntimeGuard) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.health_check_use_negotiated_protocol", "false"}}); + + setupNoServiceValidationHC(); + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + expectSessionCreate(); + expectTlsConnection(0, "h2"); + EXPECT_CALL(*test_sessions_[0]->client_connection_, nextProtocol()).Times(0); + expectStreamCreate(0); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + health_checker_->start(); + + ASSERT_NE(nullptr, test_sessions_[0]->codec_client_); + EXPECT_EQ(Http::CodecType::HTTP1, test_sessions_[0]->requested_codec_type_); +} + +// A connection that fails to establish is reported as a network failure straight away, rather than +// waiting for the health check to time out. +TEST_F(HttpHealthCheckerImplTest, AlpnPendingConnectionRemoteClose) { + setupNoServiceValidationHCOneUnhealthy(); + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + expectSessionCreate(); + expectTlsConnection(0, "h2"); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + health_checker_->start(); + + expectNoCodecClient(0); + + // The handshake never completes; the peer closes the connection instead. No stream is created. + EXPECT_CALL(*test_sessions_[0]->codec_, newStream(_)).Times(0); + EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Changed)); + EXPECT_CALL(*test_sessions_[0]->interval_timer_, enableTimer(_, _)); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()); + EXPECT_CALL(event_logger_, logUnhealthy(_, _, _, true, _)); + EXPECT_CALL(event_logger_, logEjectUnhealthy(_, _, _, _)); + test_sessions_[0]->client_connection_->raiseEvent(Network::ConnectionEvent::RemoteClose); + + EXPECT_EQ(Host::Health::Unhealthy, + cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->coarseHealth()); + EXPECT_EQ(1UL, cluster_->info_->stats_store_.counter("health_check.network_failure").value()); + EXPECT_FALSE(cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthFlagGet( + Host::HealthFlag::ACTIVE_HC_TIMEOUT)); +} + +// A handshake that never completes is reported as a timeout, and the connection being established +// is aborted. +TEST_F(HttpHealthCheckerImplTest, AlpnPendingConnectionTimeout) { + setupNoServiceValidationHCOneUnhealthy(); + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + expectSessionCreate(); + expectTlsConnection(0, "h2"); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + health_checker_->start(); + + expectNoCodecClient(0); + + EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Changed)); + EXPECT_CALL(*test_sessions_[0]->client_connection_, close(Network::ConnectionCloseType::Abort)); + EXPECT_CALL(*test_sessions_[0]->interval_timer_, enableTimer(_, _)); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()); + EXPECT_CALL(event_logger_, logUnhealthy(_, _, _, true, _)); + EXPECT_CALL(event_logger_, logEjectUnhealthy(_, _, _, _)); + test_sessions_[0]->timeout_timer_->invokeCallback(); + + EXPECT_EQ(Host::Health::Unhealthy, + cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->coarseHealth()); + EXPECT_TRUE(cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthFlagGet( + Host::HealthFlag::ACTIVE_HC_TIMEOUT)); + // Exactly one failure is recorded: the abort that the timeout triggers is expected. + EXPECT_EQ(1UL, cluster_->info_->stats_store_.counter("health_check.failure").value()); + + // The next interval establishes a fresh connection. + expectClientCreate(0); + expectTlsConnection(0, "h2"); + expectStreamCreate(0); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + test_sessions_[0]->interval_timer_->invokeCallback(); + completeHandshake(0); + EXPECT_EQ(Http::CodecType::HTTP2, test_sessions_[0]->requested_codec_type_); +} + +// A transport socket that reports the handshake as complete via ConnectedZeroRtt is handled the +// same as Connected; otherwise the session would sit on the pending connection until it timed out. +TEST_F(HttpHealthCheckerImplTest, AlpnConnectedZeroRttSelectsNegotiatedCodec) { + setupNoServiceValidationHC(); + EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Unchanged)); + + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + cluster_->info_->trafficStats()->upstream_cx_total_.inc(); + expectSessionCreate(); + expectTlsConnection(0, "h2"); + expectStreamCreate(0); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + health_checker_->start(); + + test_sessions_[0]->client_connection_->raiseEvent(Network::ConnectionEvent::ConnectedZeroRtt); + EXPECT_EQ(Http::CodecType::HTTP2, test_sessions_[0]->requested_codec_type_); + + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.max_interval", _)); + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.min_interval", _)) + .WillOnce(Return(45000)); + EXPECT_CALL(*test_sessions_[0]->interval_timer_, + enableTimer(std::chrono::milliseconds(45000), _)); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()); + respond(0, "200", false, false, true); + EXPECT_EQ(Host::Health::Healthy, + cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->coarseHealth()); +} + +// A locally closed pending connection is reported as a network failure, same as a remote close. +TEST_F(HttpHealthCheckerImplTest, AlpnPendingConnectionLocalClose) { + setupNoServiceValidationHCOneUnhealthy(); + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + expectSessionCreate(); + expectTlsConnection(0, "h2"); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + health_checker_->start(); + + expectNoCodecClient(0); + + EXPECT_CALL(*test_sessions_[0]->codec_, newStream(_)).Times(0); + EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Changed)); + EXPECT_CALL(*test_sessions_[0]->interval_timer_, enableTimer(_, _)); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()); + EXPECT_CALL(event_logger_, logUnhealthy(_, _, _, true, _)); + EXPECT_CALL(event_logger_, logEjectUnhealthy(_, _, _, _)); + test_sessions_[0]->client_connection_->raiseEvent(Network::ConnectionEvent::LocalClose); + + EXPECT_EQ(1UL, cluster_->info_->stats_store_.counter("health_check.network_failure").value()); +} + +// The negotiated codec is decided once per connection: a reused connection does not renegotiate. +TEST_F(HttpHealthCheckerImplTest, AlpnNegotiatedCodecReusedAcrossIntervals) { + setupNoServiceValidationHC(); + EXPECT_CALL(*this, onHostStatus(_, _)).Times(testing::AnyNumber()); + + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + cluster_->info_->trafficStats()->upstream_cx_total_.inc(); + expectSessionCreate(); + expectTlsConnection(0, "h2"); + expectStreamCreate(0); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + health_checker_->start(); + completeHandshake(0); + EXPECT_EQ(Http::CodecType::HTTP2, test_sessions_[0]->requested_codec_type_); + Http::CodecClient* first_codec_client = test_sessions_[0]->codec_client_; + + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.max_interval", _)) + .Times(testing::AnyNumber()); + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.min_interval", _)) + .Times(testing::AnyNumber()); + EXPECT_CALL(*test_sessions_[0]->interval_timer_, enableTimer(_, _)); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()); + respond(0, "200", false, false, true); + + // The second interval reuses the connection: no new connection, no new codec client, and the + // request is sent without waiting for another handshake. + EXPECT_CALL(dispatcher_, createClientConnection_(_, _, _, _)).Times(0); + EXPECT_CALL(*health_checker_, createCodecClient_(_, _)).Times(0); + expectStreamCreate(0); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + test_sessions_[0]->interval_timer_->invokeCallback(); + EXPECT_EQ(first_codec_client, test_sessions_[0]->codec_client_); +} + +// The health checker is destroyed while a handshake is still in flight. +TEST_F(HttpHealthCheckerImplTest, AlpnPendingConnectionDeletedWhileConnecting) { + setupNoServiceValidationHC(); + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + expectSessionCreate(); + expectTlsConnection(0, "h2"); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + health_checker_->start(); + + expectNoCodecClient(0); + + // The connection being established is aborted, and the abort is not reported as a failure. + EXPECT_CALL(*test_sessions_[0]->client_connection_, close(Network::ConnectionCloseType::Abort)); + health_checker_.reset(); + EXPECT_EQ(0UL, cluster_->info_->stats_store_.counter("health_check.failure").value()); +} + +// Removing the host from the health check completion callback while the pending connection is +// being torn down must not use freed memory. +TEST_F(HttpHealthCheckerImplTest, AlpnPendingConnectionHostRemovedInFailureCallback) { + setupNoServiceValidationHCOneUnhealthy(); + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + expectSessionCreate(); + expectTlsConnection(0, "h2"); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); + health_checker_->start(); + + expectNoCodecClient(0); + + EXPECT_CALL(*this, onHostStatus(_, _)).WillOnce(Invoke([&](HostSharedPtr host, HealthTransition) { + cluster_->prioritySet().getMockHostSet(0)->hosts_.clear(); + cluster_->prioritySet().getMockHostSet(0)->runCallbacks({}, {host}); + })); + EXPECT_CALL(*test_sessions_[0]->interval_timer_, enableTimer(_, _)).Times(testing::AnyNumber()); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()).Times(testing::AnyNumber()); + EXPECT_CALL(event_logger_, logUnhealthy(_, _, _, true, _)); + EXPECT_CALL(event_logger_, logEjectUnhealthy(_, _, _, _)); + test_sessions_[0]->client_connection_->raiseEvent(Network::ConnectionEvent::RemoteClose); + + EXPECT_EQ(1UL, cluster_->info_->stats_store_.counter("health_check.network_failure").value()); +} + TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckSetsCorrectLastHcPassTime) { const std::string host = "fake_cluster"; const std::string path = "/healthcheck"; @@ -2769,7 +3194,7 @@ TEST_F(HttpHealthCheckerImplTest, AddDisableHC) { TestSessionPtr new_test_session(new TestSession()); test_sessions_.emplace_back(std::move(new_test_session)); EXPECT_CALL(dispatcher_, createClientConnection_(_, _, _, _)).Times(0); - EXPECT_CALL(*health_checker_, createCodecClient_(_)).Times(0); + EXPECT_CALL(*health_checker_, createCodecClient_(_, _)).Times(0); envoy::config::endpoint::v3::Endpoint::HealthCheckConfig health_check_config; health_check_config.set_disable_active_health_check(true); @@ -3672,7 +4097,7 @@ class TestProdHttpHealthChecker : public ProdHttpHealthCheckerImpl { Upstream::Host::CreateConnectionData data; data.connection_ = std::move(connection); data.host_description_ = std::make_shared>(); - return std::unique_ptr(createCodecClient(data)); + return std::unique_ptr(createCodecClient(data, codec_client_type_)); } }; diff --git a/test/integration/BUILD b/test/integration/BUILD index a1bac17cc8e76..69e72dff53ba0 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -2812,6 +2812,7 @@ envoy_cc_test( "//test/common/http/http2:http2_frame", "//test/config:v2_link_hacks", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/transport_sockets/tls/v3:pkg_cc_proto", "@envoy_api//envoy/type/v3:pkg_cc_proto", ], ) diff --git a/test/integration/health_check_integration_test.cc b/test/integration/health_check_integration_test.cc index 9c2407ef7c837..4d0a99b835404 100644 --- a/test/integration/health_check_integration_test.cc +++ b/test/integration/health_check_integration_test.cc @@ -1,6 +1,7 @@ #include #include "envoy/config/core/v3/health_check.pb.h" +#include "envoy/extensions/transport_sockets/tls/v3/tls.pb.h" #include "envoy/type/v3/range.pb.h" #include "source/common/upstream/health_discovery_service.h" @@ -86,7 +87,12 @@ class HealthCheckIntegrationTestBase : public HttpIntegrationTest { for (auto& cluster : clusters_) { auto config = upstreamConfig(); config.upstream_protocol_ = upstream_protocol_; - cluster.host_upstream_ = std::make_unique(0, version_, config); + // A TLS host upstream is needed to exercise ALPN negotiation; createUpstreamTlsContext() + // offers the ALPN identifier matching `config.upstream_protocol_`. + cluster.host_upstream_ = + host_upstream_tls_ ? std::make_unique(createUpstreamTlsContext(config), 0, + version_, config) + : std::make_unique(0, version_, config); cluster.external_host_upstream_ = std::make_unique(0, version_, config); cluster.cluster_ = ConfigHelper::buildStaticCluster( cluster.name_, cluster.host_upstream_->localAddress()->ip()->port(), @@ -131,12 +137,32 @@ class HealthCheckIntegrationTestBase : public HttpIntegrationTest { return health_check; } + // Adds an upstream TLS transport socket to `cluster`, offering `alpn_protocols` on the handshake. + void addUpstreamTls(envoy::config::cluster::v3::Cluster& cluster, + const std::vector& alpn_protocols) { + envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; + // The test certs are for *.lyft.com, so make sure SNI matches. + tls_context.set_sni("foo.lyft.com"); + tls_context.mutable_common_tls_context() + ->mutable_validation_context() + ->mutable_trusted_ca() + ->set_filename( + TestEnvironment::runfilesPath("test/config/integration/certs/upstreamcacert.pem")); + for (const auto& alpn : alpn_protocols) { + tls_context.mutable_common_tls_context()->add_alpn_protocols(alpn); + } + cluster.mutable_transport_socket()->set_name("envoy.transport_sockets.tls"); + std::ignore = cluster.mutable_transport_socket()->mutable_typed_config()->PackFrom(tls_context); + } + // The number of clusters and their names must match the clusters in the CDS integration test // configuration. static constexpr size_t clusters_num_ = 2; std::array clusters_{{{"cluster_1"}, {"cluster_2"}}}; Network::Address::IpVersion ip_version_; Http::CodecType upstream_protocol_; + // Whether the health checked host upstreams terminate TLS. + bool host_upstream_tls_{false}; }; struct HttpHealthCheckIntegrationTestParams { @@ -1068,5 +1094,96 @@ TEST_P(HttpHealthCheckIntegrationTest, SingleEndpointHealthyHttpWithBinaryPayloa EXPECT_EQ(0, test_server_->counter("cluster.cluster_1.health_check.failure")->value()); } +// Health checking over TLS, where the codec is selected from the protocol negotiated by ALPN +// rather than from `codec_client_type`. +class HttpHealthCheckAlpnIntegrationTest + : public testing::TestWithParam, + public HealthCheckIntegrationTestBase { +public: + HttpHealthCheckAlpnIntegrationTest() : HealthCheckIntegrationTestBase(GetParam()) {} + + void TearDown() override { + cleanupHostConnections(); + cleanUpXdsConnection(); + } + + // Brings up a TLS host upstream that speaks `upstream_protocol` and offers the matching ALPN + // identifier, health checked by a cluster that offers `cluster_alpn_protocols` and is configured + // for `codec_client_type`. + void initTlsHealthCheck(Http::CodecType upstream_protocol, + const std::vector& cluster_alpn_protocols, + envoy::type::v3::CodecClientType codec_client_type, + uint32_t timeout_seconds = 30) { + host_upstream_tls_ = true; + upstream_protocol_ = upstream_protocol; + initialize(); + + auto& cluster_data = clusters_[0]; + addUpstreamTls(cluster_data.cluster_, cluster_alpn_protocols); + auto* health_check = addHealthCheck(cluster_data.cluster_); + health_check->mutable_timeout()->set_seconds(timeout_seconds); + health_check->mutable_http_health_check()->set_path("/healthcheck"); + health_check->mutable_http_health_check()->set_codec_client_type(codec_client_type); + + EXPECT_TRUE(compareDiscoveryRequest(Config::TestTypeUrl::get().Cluster, "", {}, {}, {}, true)); + sendDiscoveryResponse(Config::TestTypeUrl::get().Cluster, + {cluster_data.cluster_}, + {cluster_data.cluster_}, {}, "55"); + } + + // Waits for a health check probe on the host upstream and answers it with a 200. The upstream + // only produces a stream if Envoy spoke the protocol the upstream negotiated, which is what + // makes these tests discriminating. + void expectHealthCheckProbeAndRespond() { + auto& cluster_data = clusters_[0]; + ASSERT_TRUE(cluster_data.host_upstream_->waitForHttpConnection( + *dispatcher_, cluster_data.host_fake_connection_)); + ASSERT_TRUE(cluster_data.host_fake_connection_->waitForNewStream(*dispatcher_, + cluster_data.host_stream_)); + ASSERT_TRUE(cluster_data.host_stream_->waitForEndStream(*dispatcher_)); + EXPECT_EQ(cluster_data.host_stream_->headers().getPathValue(), "/healthcheck"); + + cluster_data.host_stream_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, + true); + test_server_->waitForCounter("cluster.cluster_1.health_check.success", Ge(1)); + EXPECT_EQ(0, test_server_->counter("cluster.cluster_1.health_check.failure")->value()); + } +}; + +INSTANTIATE_TEST_SUITE_P(IpVersions, HttpHealthCheckAlpnIntegrationTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + +// The handshake settles on h2, so the health check is sent over HTTP/2 even though HTTP/1.1 is +// configured. Before the negotiated protocol was honored, Envoy sent HTTP/1.1 here and the HTTP/2 +// upstream never produced a stream. +TEST_P(HttpHealthCheckAlpnIntegrationTest, NegotiatedHttp2WithHttp1Configured) { + initTlsHealthCheck(Http::CodecType::HTTP2, {"h2", "http/1.1"}, + envoy::type::v3::CodecClientType::HTTP1); + expectHealthCheckProbeAndRespond(); +} + +// The mirror image: the handshake settles on http/1.1, so the health check is sent over HTTP/1.1 +// even though HTTP/2 is configured. +TEST_P(HttpHealthCheckAlpnIntegrationTest, NegotiatedHttp1WithHttp2Configured) { + initTlsHealthCheck(Http::CodecType::HTTP1, {"h2", "http/1.1"}, + envoy::type::v3::CodecClientType::HTTP2); + expectHealthCheckProbeAndRespond(); +} + +// With the runtime guard disabled, the negotiated protocol is ignored: Envoy speaks HTTP/1.1 to an +// upstream that negotiated h2, and the health check fails. +TEST_P(HttpHealthCheckAlpnIntegrationTest, RuntimeGuardDisabledIgnoresNegotiatedProtocol) { + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.health_check_use_negotiated_protocol", "false"); + // A short timeout so that speaking the wrong protocol is reported promptly: the HTTP/2 upstream + // simply never answers the HTTP/1.1 probe. + initTlsHealthCheck(Http::CodecType::HTTP2, {"h2", "http/1.1"}, + envoy::type::v3::CodecClientType::HTTP1, /*timeout_seconds=*/1); + + test_server_->waitForCounter("cluster.cluster_1.health_check.failure", Ge(1)); + EXPECT_EQ(0, test_server_->counter("cluster.cluster_1.health_check.success")->value()); +} + } // namespace } // namespace Envoy