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