From 7263c533da931203bd949c65280b5ded2e7faa1d Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 31 May 2024 17:40:01 +0000 Subject: [PATCH 01/27] Allow multiplexed upstream servers to half close the stream before the downstream. Signed-off-by: Yan Avlasov --- changelogs/current.yaml | 7 + envoy/http/stream_reset_handler.h | 4 +- source/common/http/async_client_impl.cc | 17 +- source/common/http/conn_manager_impl.h | 4 +- source/common/http/conn_pool_base.cc | 1 + source/common/http/filter_manager.cc | 62 ++++- source/common/http/filter_manager.h | 40 ++- source/common/http/http1/codec_impl.cc | 13 +- source/common/http/http1/codec_impl.h | 2 + source/common/http/utility.cc | 2 + source/common/router/router.cc | 17 +- source/common/router/router.h | 5 +- source/common/router/upstream_request.cc | 2 + source/common/runtime/runtime_features.cc | 5 + .../upstreams/http/tcp/upstream_request.cc | 25 +- .../upstreams/http/tcp/upstream_request.h | 2 + test/common/http/async_client_impl_test.cc | 2 + test/common/http/conn_manager_impl_test_2.cc | 53 +++- test/common/router/router_2_test.cc | 6 + test/common/router/router_test.cc | 37 +++ test/integration/http_integration.cc | 8 +- test/integration/http_integration.h | 2 +- .../multiplexed_integration_test.cc | 3 + .../multiplexed_upstream_integration_test.cc | 4 + test/integration/protocol_integration_test.cc | 233 ++++++++++++++++++ .../tcp_tunneling_integration_test.cc | 34 +++ 26 files changed, 554 insertions(+), 36 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index e5dabc406200..23666b2f624c 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -26,6 +26,13 @@ behavior_changes: :ref:`TlvsMetadata type `. This change can be temporarily disabled by setting the runtime flag ``envoy.reloadable_features.use_typed_metadata_in_proxy_protocol_listener`` to ``false``. +- area: http + change: | + Allow HTTP/2 (and HTTP/3) upstream servers to half close the stream before the downstream. This enables bidirectional + gRPC streams where server completes streaming before the client. Behavior of HTTP/1 or TCP proxy upstream servers is + unchanged and the stream is reset if the upstream server completes response before the downstream. The stream is also + reset if the upstream server responds with an error status before the downstream. This behavior is disabled by default + and can be enabled by setting the ``envoy.reloadable_features.allow_multiplexed_upstream_half_close`` runtime key to true. minor_behavior_changes: # *Changes that may cause incompatibilities for some users, but should not for most* diff --git a/envoy/http/stream_reset_handler.h b/envoy/http/stream_reset_handler.h index e52375e06123..09e3b1dd8038 100644 --- a/envoy/http/stream_reset_handler.h +++ b/envoy/http/stream_reset_handler.h @@ -36,7 +36,9 @@ enum class StreamResetReason { // Received payload did not conform to HTTP protocol. ProtocolError, // If the stream was locally reset by the Overload Manager. - OverloadManager + OverloadManager, + // If stream was locally reset due to HTTP/1 upstream half closing before downstream. + Http1PrematureUpstreamHalfClose, }; /** diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index e51d701326d6..f9daa91d13a0 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -145,13 +145,16 @@ void AsyncStreamImpl::encodeHeaders(ResponseHeaderMapPtr&& headers, bool end_str encoded_response_headers_ = true; stream_callbacks_.onHeaders(std::move(headers), end_stream); closeRemote(end_stream); - // At present, the router cleans up stream state as soon as the remote is closed, making a - // half-open local stream unsupported and dangerous. Ensure we close locally to trigger completion - // and keep things consistent. Another option would be to issue a stream reset here if local isn't - // yet closed, triggering cleanup along a more standardized path. However, this would require - // additional logic to handle the response completion and subsequent reset, and run the risk of - // being interpreted as a failure, when in fact no error has necessarily occurred. Gracefully - // closing seems most in-line with behavior elsewhere in Envoy for now. + // At present, the AsyncStream is always fully closed when the server half closes the stream. + // This is the case even when allow_multiplexed_upstream_half_close runtime flag is set, as there + // are currently no known use cases where early server half close needs to be supported. + // + // Always ensure we close locally to trigger completion. Another option would be to issue a stream + // reset here if local isn't yet closed, triggering cleanup along a more standardized path. + // However, this would require additional logic to handle the response completion and subsequent + // reset, and run the risk of being interpreted as a failure, when in fact no error has + // necessarily occurred. Gracefully closing seems most in-line with behavior elsewhere in Envoy + // for now. closeLocal(end_stream); } diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 5f546ed9be3d..ced126265fbe 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -283,7 +283,9 @@ class ConnectionManagerImpl : Logger::Loggable, OptRef tracingConfig() const override; const ScopeTrackedObject& scope() override; OptRef downstreamCallbacks() override { return *this; } - bool isHalfCloseEnabled() override { return false; } + bool isHalfCloseEnabled() override { + return filter_manager_.allowUpstreamHalfClose() ? true : false; + } // DownstreamStreamFilterCallbacks void setRoute(Router::RouteConstSharedPtr route) override; diff --git a/source/common/http/conn_pool_base.cc b/source/common/http/conn_pool_base.cc index ca5e589b7487..f65427fbcc88 100644 --- a/source/common/http/conn_pool_base.cc +++ b/source/common/http/conn_pool_base.cc @@ -177,6 +177,7 @@ void MultiplexedActiveClientBase::onStreamReset(Http::StreamResetReason reason) case StreamResetReason::RemoteRefusedStreamReset: case StreamResetReason::Overflow: case StreamResetReason::ConnectError: + case StreamResetReason::Http1PrematureUpstreamHalfClose: break; } } diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 0116537c7937..3485948de220 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -903,16 +903,28 @@ FilterManager::commonEncodePrefix(ActiveStreamEncoderFilter* filter, bool end_st FilterIterationStartState filter_iteration_start_state) { // Only do base state setting on the initial call. Subsequent calls for filtering do not touch // the base state. - ENVOY_STREAM_LOG(trace, "commonEncodePrefix end_stream: {}, isHalfCloseEnabled: {}", *this, - end_stream, filter_manager_callbacks_.isHalfCloseEnabled()); + ENVOY_STREAM_LOG(trace, + "commonEncodePrefix end_stream: {}, isHalfCloseEnabled: {}, force_close: {}", + *this, end_stream, filter_manager_callbacks_.isHalfCloseEnabled(), + static_cast(state_.force_close_stream_)); if (filter == nullptr) { // half close is enabled in case tcp proxying is done with http1 encoder. In this case, we // should not set the local_complete_ flag to true when end_stream is true. // setting local_complete_ to true will cause any data sent in the upstream direction to be // dropped. - if (end_stream && !filter_manager_callbacks_.isHalfCloseEnabled()) { - ASSERT(!state_.local_complete_); - state_.local_complete_ = true; + if (allow_upstream_half_close_) { + if (end_stream) { + state_.encoder_end_stream_ = true; + if (!filter_manager_callbacks_.isHalfCloseEnabled() || state_.force_close_stream_) { + ASSERT(!state_.local_complete_); + state_.local_complete_ = true; + } + } + } else { + if (end_stream && !filter_manager_callbacks_.isHalfCloseEnabled()) { + ASSERT(!state_.local_complete_); + state_.local_complete_ = true; + } } return encoder_filters_.begin(); } @@ -960,6 +972,8 @@ void DownstreamFilterManager::sendLocalReply( ASSERT(!state_.under_on_local_reply_); const bool is_head_request = state_.is_head_request_; const bool is_grpc_request = state_.is_grpc_request_; + // Local reply closes the stream even if downstream is not half closed. + state_.force_close_stream_ = true; // Stop filter chain iteration if local reply was sent while filter decoding or encoding callbacks // are running. @@ -1284,6 +1298,14 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea const bool modified_end_stream = (end_stream && continue_data_entry == encoder_filters_.end()); state_.non_100_response_headers_encoded_ = true; + if (allow_upstream_half_close_) { + const uint64_t response_status = Http::Utility::getResponseStatus(headers); + if (!(Http::CodeUtility::is2xx(response_status) || Http::CodeUtility::is1xx(response_status))) { + // Even if upstream half close is enabled the stream is closed on error responses from the + // server. + state_.force_close_stream_ = true; + } + } filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream); if (state_.saw_downstream_reset_) { return; @@ -1489,6 +1511,27 @@ void FilterManager::maybeEndEncode(bool end_stream) { if (end_stream) { ASSERT(!state_.remote_encode_complete_); state_.remote_encode_complete_ = true; + if (allow_upstream_half_close_) { + maybeCloseStream(); + } else { + state_.stream_closed_ = true; + filter_manager_callbacks_.endStream(); + } + } +} + +void FilterManager::maybeCloseStream() { + ASSERT(allow_upstream_half_close_); + if (state_.stream_closed_) { + return; + } + + // If upstream half close is enabled then close the stream either when force close + // is set (i.e local reply) or when both server and client half closed. + if (state_.remote_encode_complete_ && + (state_.remote_decode_complete_ || state_.force_close_stream_)) { + state_.stream_closed_ = true; + ENVOY_STREAM_LOG(trace, "closing stream", *this); filter_manager_callbacks_.endStream(); } } @@ -1671,7 +1714,14 @@ Buffer::InstancePtr ActiveStreamEncoderFilter::createBuffer() { Buffer::InstancePtr& ActiveStreamEncoderFilter::bufferedData() { return parent_.buffered_response_data_; } -bool ActiveStreamEncoderFilter::complete() { return parent_.state_.local_complete_; } +bool ActiveStreamEncoderFilter::complete() { + // This is used for determining end_stream flag when iterating encoder filter chain. + // When the upstream half close is enabled the local complete may not be set when + // end stream is observed on the encode path in case upstream half closes before the + // downstream. + return parent_.allow_upstream_half_close_ ? parent_.state_.encoder_end_stream_ + : parent_.state_.local_complete_; +} bool ActiveStreamEncoderFilter::has1xxHeaders() { return parent_.state_.has_1xx_headers_ && !continued_1xx_headers_; } diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 2952dcc07071..1b434496efcf 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -648,7 +648,9 @@ class FilterManager : public ScopeTrackedObject, proxy_100_continue_(proxy_100_continue), buffer_limit_(buffer_limit), filter_chain_factory_(filter_chain_factory), no_downgrade_to_canonical_name_(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.no_downgrade_to_canonical_name")) {} + "envoy.reloadable_features.no_downgrade_to_canonical_name")), + allow_upstream_half_close_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close")) {} ~FilterManager() override { ASSERT(state_.destroyed_); ASSERT(state_.filter_call_state_ == 0); @@ -748,6 +750,9 @@ class FilterManager : public ScopeTrackedObject, void decodeHeaders(RequestHeaderMap& headers, bool end_stream) { state_.remote_decode_complete_ = end_stream; decodeHeaders(nullptr, headers, end_stream); + if (allow_upstream_half_close_) { + maybeCloseStream(); + } } /** @@ -758,6 +763,9 @@ class FilterManager : public ScopeTrackedObject, void decodeData(Buffer::Instance& data, bool end_stream) { state_.remote_decode_complete_ = end_stream; decodeData(nullptr, data, end_stream, FilterIterationStartState::CanStartFromCurrent); + if (allow_upstream_half_close_) { + maybeCloseStream(); + } } /** @@ -767,6 +775,9 @@ class FilterManager : public ScopeTrackedObject, void decodeTrailers(RequestTrailerMap& trailers) { state_.remote_decode_complete_ = true; decodeTrailers(nullptr, trailers); + if (allow_upstream_half_close_) { + maybeCloseStream(); + } } /** @@ -783,6 +794,8 @@ class FilterManager : public ScopeTrackedObject, */ void maybeEndEncode(bool end_stream); + void maybeCloseStream(); + virtual void sendLocalReply(Code code, absl::string_view body, const std::function& modify_headers, const absl::optional grpc_status, @@ -861,19 +874,24 @@ class FilterManager : public ScopeTrackedObject, bool sawDownstreamReset() { return state_.saw_downstream_reset_; } virtual bool shouldLoadShed() { return false; }; + bool allowUpstreamHalfClose() const { return allow_upstream_half_close_; } protected: struct State { State() - : remote_decode_complete_(false), remote_encode_complete_(false), local_complete_(false), - has_1xx_headers_(false), created_filter_chain_(false), is_head_request_(false), - is_grpc_request_(false), non_100_response_headers_encoded_(false), - under_on_local_reply_(false), decoder_filter_chain_aborted_(false), - encoder_filter_chain_aborted_(false), saw_downstream_reset_(false) {} + : remote_decode_complete_(false), remote_encode_complete_(false), + encoder_end_stream_(false), local_complete_(false), has_1xx_headers_(false), + created_filter_chain_(false), is_head_request_(false), is_grpc_request_(false), + non_100_response_headers_encoded_(false), under_on_local_reply_(false), + decoder_filter_chain_aborted_(false), encoder_filter_chain_aborted_(false), + saw_downstream_reset_(false), stream_closed_(false), force_close_stream_(false) {} uint32_t filter_call_state_{0}; - bool remote_decode_complete_ : 1; - bool remote_encode_complete_ : 1; + bool remote_decode_complete_ : 1; // Set when decoder filter chain iteration has completed. + bool remote_encode_complete_ : 1; // Set when encoder filter chain iteration has completed. + bool encoder_end_stream_ : 1; // This is set the first time the end_stream is observed during + // encoder filter chain iteration and used to set the end_stream + // flag when resuming encoder filter chain iteration. bool local_complete_ : 1; // This indicates that local is complete prior to filter processing. // A filter can still stop the stream from being complete as seen // by the codec. @@ -893,6 +911,11 @@ class FilterManager : public ScopeTrackedObject, bool decoder_filter_chain_aborted_ : 1; bool encoder_filter_chain_aborted_ : 1; bool saw_downstream_reset_ : 1; + bool stream_closed_ : 1; // Set when both remote_decode_complete_ and remote_encode_complete_ is + // true observed for the first time and prevents ending the stream + // multiple times. Only set when allow_upstream_half_close is enabled. + bool force_close_stream_ : 1; // Set to indicate that stream should be closed due to either + // local reply or error response from the server. // The following 3 members are booleans rather than part of the space-saving bitfield as they // are passed as arguments to functions expecting bools. Extend State using the bitfield @@ -1086,6 +1109,7 @@ class FilterManager : public ScopeTrackedObject, State state_; const bool no_downgrade_to_canonical_name_{}; + const bool allow_upstream_half_close_{}; }; // The DownstreamFilterManager has explicit handling to send local replies. diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index be3445209e80..05deb8da45f6 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -1436,7 +1436,9 @@ ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, Code [&]() -> void { this->onBelowLowWatermark(); }, [&]() -> void { this->onAboveHighWatermark(); }, []() -> void { /* TODO(adisuissa): handle overflow watermark */ })), - passing_through_proxy_(passing_through_proxy) { + passing_through_proxy_(passing_through_proxy), + force_reset_on_premature_upstream_half_close_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close")) { owned_output_buffer_->setWatermarks(connection.bufferLimit()); // Inform parent output_buffer_ = owned_output_buffer_.get(); @@ -1587,6 +1589,15 @@ CallbackResult ClientConnectionImpl::onMessageCompleteBase() { response.decoder_->decodeData(buffer, true); } + if (force_reset_on_premature_upstream_half_close_) { + // H/1 connections are always reset if upstream is done before downstream. + // When the allow_multiplexed_upstream_half_close is enabled the router filter does not + // reset streams where upstream half closed before downstream. In this case the H/1 codec + // has to reset the stream. + ENVOY_CONN_LOG(trace, "Resetting stream due to premature H/1 upstream close.", connection_); + response.encoder_.runResetCallbacks(StreamResetReason::Http1PrematureUpstreamHalfClose); + } + // Reset to ensure no information from one requests persists to the next. pending_response_.reset(); headers_or_trailers_.emplace(nullptr); diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 59ba6bfd8e99..0eebf9225349 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -686,6 +686,8 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { // True if the upstream connection is pointed at an HTTP/1.1 proxy, and // plaintext HTTP should be sent with fully qualified URLs. bool passing_through_proxy_ = false; + + const bool force_reset_on_premature_upstream_half_close_{}; }; } // namespace Http1 diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index a1a4711b3a5e..0cf15e5c55a4 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -1060,6 +1060,8 @@ const std::string Utility::resetReasonToString(const Http::StreamResetReason res return "protocol error"; case Http::StreamResetReason::OverloadManager: return "overload manager reset"; + case Http::StreamResetReason::Http1PrematureUpstreamHalfClose: + return "HTTP/1 premature upstream half close"; } return ""; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 309eec420e1d..4d065c6cbb6a 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -751,9 +751,9 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, // will never transition from false to true. bool can_use_http3 = !transport_socket_options_ || !transport_socket_options_->http11ProxyInfo().has_value(); - UpstreamRequestPtr upstream_request = - std::make_unique(*this, std::move(generic_conn_pool), can_send_early_data, - can_use_http3, false /*enable_half_close*/); + UpstreamRequestPtr upstream_request = std::make_unique( + *this, std::move(generic_conn_pool), can_send_early_data, can_use_http3, + allow_multiplexed_upstream_half_close_ /*enable_half_close*/); LinkedList::moveIntoList(std::move(upstream_request), upstream_requests_); upstream_requests_.front()->acceptHeadersFromRouter(end_stream); if (streaming_shadows_) { @@ -1441,6 +1441,7 @@ Filter::streamResetReasonToResponseFlag(Http::StreamResetReason reset_reason) { return StreamInfo::CoreResponseFlag::UpstreamConnectionTermination; case Http::StreamResetReason::LocalReset: case Http::StreamResetReason::LocalRefusedStreamReset: + case Http::StreamResetReason::Http1PrematureUpstreamHalfClose: return StreamInfo::CoreResponseFlag::LocalReset; case Http::StreamResetReason::Overflow: return StreamInfo::CoreResponseFlag::UpstreamOverflow; @@ -1722,6 +1723,10 @@ void Filter::onUpstreamMetadata(Http::MetadataMapPtr&& metadata_map) { void Filter::onUpstreamComplete(UpstreamRequest& upstream_request) { if (!downstream_end_stream_) { + if (allow_multiplexed_upstream_half_close_) { + // Continue request if downstream is not done yet. + return; + } upstream_request.resetStream(); } Event::Dispatcher& dispatcher = callbacks_->dispatcher(); @@ -1994,9 +1999,9 @@ void Filter::doRetry(bool can_send_early_data, bool can_use_http3, TimeoutRetry cleanup(); return; } - UpstreamRequestPtr upstream_request = - std::make_unique(*this, std::move(generic_conn_pool), can_send_early_data, - can_use_http3, false /*enable_tcp_tunneling*/); + UpstreamRequestPtr upstream_request = std::make_unique( + *this, std::move(generic_conn_pool), can_send_early_data, can_use_http3, + allow_multiplexed_upstream_half_close_ /*enable_half_close*/); if (include_attempt_count_in_request_) { downstream_headers_->setEnvoyAttemptCount(attempt_count_); diff --git a/source/common/router/router.h b/source/common/router/router.h index 227a8f4a5a11..a0d31620649a 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -304,7 +304,9 @@ class Filter : Logger::Loggable, : config_(config), stats_(stats), grpc_request_(false), exclude_http_code_stats_(false), downstream_response_started_(false), downstream_end_stream_(false), is_retry_(false), request_buffer_overflowed_(false), streaming_shadows_(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.streaming_shadow")) {} + "envoy.reloadable_features.streaming_shadow")), + allow_multiplexed_upstream_half_close_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close")) {} ~Filter() override; @@ -605,6 +607,7 @@ class Filter : Logger::Loggable, bool include_timeout_retry_header_in_request_ : 1; bool request_buffer_overflowed_ : 1; const bool streaming_shadows_ : 1; + const bool allow_multiplexed_upstream_half_close_ : 1; }; class ProdFilter : public Filter { diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index 41a42290d3c0..63527b2c1fac 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -69,7 +69,9 @@ class UpstreamFilterManager : public Http::FilterManager { state().decoder_filter_chain_aborted_ = true; state().encoder_filter_chain_aborted_ = true; state().remote_encode_complete_ = true; + state().encoder_end_stream_ = true; state().local_complete_ = true; + state().force_close_stream_ = true; // TODO(alyssawilk) this should be done through the router to play well with hedging. upstream_request_.parent_.callbacks()->sendLocalReply(code, body, modify_headers, grpc_status, details); diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 23188e83ff59..ef42c8e5af2c 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -165,6 +165,11 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_reresolve_if_no_connections); // compliance restrictions. FALSE_RUNTIME_GUARD(envoy_reloadable_features_google_grpc_disable_tls_13); +// TODO(yanavlasov): Flip to true after prod testing. +// Controls whether a stream stays open when HTTP/2 or HTTP/3 upstream half closes +// before downstream. +FALSE_RUNTIME_GUARD(envoy_reloadable_features_allow_multiplexed_upstream_half_close); + // Block of non-boolean flags. Use of int flags is deprecated. Do not add more. ABSL_FLAG(uint64_t, re2_max_program_size_error_level, 100, ""); // NOLINT ABSL_FLAG(uint64_t, re2_max_program_size_warn_level, // NOLINT diff --git a/source/extensions/upstreams/http/tcp/upstream_request.cc b/source/extensions/upstreams/http/tcp/upstream_request.cc index 2e9a3eb1357b..da44c2bbccd0 100644 --- a/source/extensions/upstreams/http/tcp/upstream_request.cc +++ b/source/extensions/upstreams/http/tcp/upstream_request.cc @@ -33,18 +33,22 @@ void TcpConnPool::onPoolReady(Envoy::Tcp::ConnectionPool::ConnectionDataPtr&& co TcpUpstream::TcpUpstream(Router::UpstreamToDownstream* upstream_request, Envoy::Tcp::ConnectionPool::ConnectionDataPtr&& upstream) - : upstream_request_(upstream_request), upstream_conn_data_(std::move(upstream)) { + : upstream_request_(upstream_request), upstream_conn_data_(std::move(upstream)), + force_reset_on_upstream_half_close_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close")) { upstream_conn_data_->connection().enableHalfClose(true); upstream_conn_data_->addUpstreamCallbacks(*this); } void TcpUpstream::encodeData(Buffer::Instance& data, bool end_stream) { + downstream_complete_ = end_stream; bytes_meter_->addWireBytesSent(data.length()); upstream_conn_data_->connection().write(data, end_stream); } Envoy::Http::Status TcpUpstream::encodeHeaders(const Envoy::Http::RequestHeaderMap&, bool end_stream) { + downstream_complete_ = end_stream; // Headers should only happen once, so use this opportunity to add the proxy // proto header, if configured. const Router::RouteEntry* route_entry = upstream_request_->route().routeEntry(); @@ -76,6 +80,7 @@ Envoy::Http::Status TcpUpstream::encodeHeaders(const Envoy::Http::RequestHeaderM } void TcpUpstream::encodeTrailers(const Envoy::Http::RequestTrailerMap&) { + downstream_complete_ = true; Buffer::OwnedImpl data; upstream_conn_data_->connection().write(data, true); } @@ -94,8 +99,26 @@ void TcpUpstream::resetStream() { } void TcpUpstream::onUpstreamData(Buffer::Instance& data, bool end_stream) { + // In the TCP proxy case the filter manager used to trigger the full stream closure when the + // upstream server half closed its end of the TCP connection. With the + // allow_multiplexed_upstream_half_close enabled filter manager no longer closes stream that were + // half closed by upstream before downstream. To keep the behavior the same for TCP proxy the + // upstream force closes the connection when server half closes. + // + // Save the indicator to close the stream before calling the decodeData since when the + // allow_multiplexed_upstream_half_close is false the call to decodeHeader with end_stream==true + // will delete the TcpUpstream object. NOTE: it this point Envoy can not support half closed TCP + // upstream as there is currently no detection of half closed vs fully closed TCP connections. + bool force_reset = force_reset_on_upstream_half_close_ && end_stream && !downstream_complete_; bytes_meter_->addWireBytesReceived(data.length()); upstream_request_->decodeData(data, end_stream); + // force_reset is true only when allow_multiplexed_upstream_half_close is true and in this case + // the decodeData will never cause the stream to be closed and as such it safe to access + // upstream_request_ + if (force_reset && upstream_request_) { + upstream_request_->onResetStream(Envoy::Http::StreamResetReason::ConnectionTermination, + "half_close_initiated_full_close"); + } } void TcpUpstream::onEvent(Network::ConnectionEvent event) { diff --git a/source/extensions/upstreams/http/tcp/upstream_request.h b/source/extensions/upstreams/http/tcp/upstream_request.h index 3a0df2815529..c85f707e5ba6 100644 --- a/source/extensions/upstreams/http/tcp/upstream_request.h +++ b/source/extensions/upstreams/http/tcp/upstream_request.h @@ -94,6 +94,8 @@ class TcpUpstream : public Router::GenericUpstream, Router::UpstreamToDownstream* upstream_request_; Envoy::Tcp::ConnectionPool::ConnectionDataPtr upstream_conn_data_; StreamInfo::BytesMeterSharedPtr bytes_meter_{std::make_shared()}; + bool downstream_complete_ = false; + const bool force_reset_on_upstream_half_close_{}; }; } // namespace Tcp diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index ae074cd026e2..82d3a7407750 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -1362,6 +1362,7 @@ TEST_F(AsyncClientImplTest, SendDataAfterRemoteClosure) { EXPECT_CALL(stream_encoder_, encodeData(_, _)).Times(0); stream->sendData(*body, true); + dispatcher_.clearDeferredDeleteList(); } TEST_F(AsyncClientImplTest, SendTrailersRemoteClosure) { @@ -1402,6 +1403,7 @@ TEST_F(AsyncClientImplTest, SendTrailersRemoteClosure) { EXPECT_CALL(stream_encoder_, encodeTrailers(_)).Times(0); stream->sendTrailers(trailers); + dispatcher_.clearDeferredDeleteList(); } // Validate behavior when the stream's onHeaders() callback performs a stream diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index 5f7e43036a76..439340ee9cb6 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -20,6 +20,9 @@ namespace Envoy { namespace Http { TEST_F(HttpConnectionManagerImplTest, ResponseBeforeRequestComplete) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "false"}}); setup(false, "envoy-server-test"); setupFilterChain(1, 0); @@ -42,7 +45,49 @@ TEST_F(HttpConnectionManagerImplTest, ResponseBeforeRequestComplete) { decoder_filters_[0]->callbacks_->encodeHeaders(std::move(response_headers), true, "details"); } +TEST_F(HttpConnectionManagerImplTest, ResponseBeforeRequestCompleteWithUpstreamHalfCloseEnabled) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"}}); + setup(false, "envoy-server-test"); + setupFilterChain(1, 0); + + EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::StopIteration)); + startRequest(); + + EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) + .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { + EXPECT_NE(nullptr, headers.Server()); + EXPECT_EQ("envoy-server-test", headers.getServerValue()); + response_encoder_.stream_.codec_callbacks_->onCodecEncodeComplete(); + })); + + ResponseHeaderMapPtr response_headers{new TestResponseHeaderMapImpl{{":status", "200"}}}; + decoder_filters_[0]->callbacks_->streamInfo().setResponseCodeDetails(""); + // Half closing upstream connection does not cause the stream to be reset + decoder_filters_[0]->callbacks_->encodeHeaders(std::move(response_headers), true, "details"); + + EXPECT_CALL(*decoder_filters_[0], onStreamComplete()); + EXPECT_CALL(*decoder_filters_[0], onDestroy()); + EXPECT_CALL(filter_callbacks_.connection_, + close(Network::ConnectionCloseType::FlushWriteAndDelay, _)) + .Times(2); + + // Half closing both downstream and upstream triggers full stream close + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> Http::Status { + Buffer::OwnedImpl fake_data("the end"); + decoder_->decodeData(fake_data, true); + return Http::okStatus(); + })); + Buffer::OwnedImpl fake_input; + conn_manager_->onData(fake_input, false); +} + TEST_F(HttpConnectionManagerImplTest, ResponseBeforeRequestComplete10) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "false"}}); EXPECT_CALL(*codec_, protocol()).WillRepeatedly(Return(Protocol::Http10)); setup(false, "envoy-server-test"); setupFilterChain(1, 0); @@ -67,6 +112,9 @@ TEST_F(HttpConnectionManagerImplTest, ResponseBeforeRequestComplete10) { } TEST_F(HttpConnectionManagerImplTest, ResponseBeforeRequestComplete10NoOptimize) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "false"}}); EXPECT_CALL(runtime_.snapshot_, getBoolean(_, _)).WillRepeatedly(Return(false)); EXPECT_CALL(*codec_, protocol()).WillRepeatedly(Return(Protocol::Http10)); setup(false, "envoy-server-test"); @@ -94,6 +142,9 @@ TEST_F(HttpConnectionManagerImplTest, ResponseBeforeRequestComplete10NoOptimize) } TEST_F(HttpConnectionManagerImplTest, DisconnectOnProxyConnectionDisconnect) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "false"}}); setup(false, "envoy-server-test"); setupFilterChain(1, 0); @@ -520,7 +571,7 @@ TEST_F(HttpConnectionManagerImplTest, ConnectionDuration) { EXPECT_EQ(1U, stats_.named_.downstream_cx_max_duration_reached_.value()); } -TEST_F(HttpConnectionManagerImplTest, IntermediateBufferingEarlyResponse) { +TEST_F(HttpConnectionManagerImplTest, DISABLED_IntermediateBufferingEarlyResponse) { setup(false, ""); setupFilterChain(2, 0); diff --git a/test/common/router/router_2_test.cc b/test/common/router/router_2_test.cc index 4c0fbba8d131..3aa814267209 100644 --- a/test/common/router/router_2_test.cc +++ b/test/common/router/router_2_test.cc @@ -199,6 +199,8 @@ TEST_F(WatermarkTest, UpstreamWatermarks) { Buffer::OwnedImpl data; EXPECT_CALL(encoder_, getStream()).WillOnce(ReturnRef(stream_)); response_decoder_->decodeData(data, true); + // router filter no longer resets the stream due to upstream half closing first + router_->onDestroy(); } // Tests that readDisabled are delayed till upstream response headers arrive. @@ -252,6 +254,8 @@ TEST_F(WatermarkTest, DelayUpstreamReadDisableBeforeResponse1) { Buffer::OwnedImpl data; EXPECT_CALL(callbacks_, encodeData(_, true)); response_decoder_->decodeData(data, true); + // router filter no longer resets the stream due to upstream half closing first + router_->onDestroy(); } // Tests that delayed readDisable is triggered only once if there is 1xx response followed by @@ -302,6 +306,8 @@ TEST_F(WatermarkTest, DelayUpstreamReadDisableBeforeResponse2) { Buffer::OwnedImpl data; EXPECT_CALL(callbacks_, encodeData(_, true)); response_decoder_->decodeData(data, true); + // router filter no longer resets the stream due to upstream half closing first + router_->onDestroy(); } TEST_F(WatermarkTest, FilterWatermarks) { diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 0615f77c7e57..08dfaf51ad90 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -3444,6 +3444,39 @@ TEST_F(RouterTest, NoRetryWithBodyLimit) { response_decoder->decodeHeaders(std::move(response_headers), true); } +TEST_F(RouterTest, NoRetryWithBodyLimitWithUpstreamHalfCloseEnabled) { + // This test half closes upstream connection before downstream. Enabling the + // allow_multiplexed_upstream_half_close flag causes the clean-up sequence to + // change and this requires explicit destruction of the filter before mock + // objects. This test was added to validate that the old behavior is preserved + // when allow_multiplexed_upstream_half_close is false. + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"}}); + NiceMock encoder1; + Http::ResponseDecoder* response_decoder = nullptr; + expectNewStreamWithImmediateEncoder(encoder1, &response_decoder, Http::Protocol::Http10); + + // Set a per route body limit which disallows any buffering. + EXPECT_CALL(callbacks_.route_->route_entry_, retryShadowBufferLimit()).WillOnce(Return(0)); + Http::TestRequestHeaderMapImpl headers{{"x-envoy-retry-on", "5xx"}, {"x-envoy-internal", "true"}}; + HttpTestUtility::addDefaultHeaders(headers); + router_->decodeHeaders(headers, false); + // Unlike RetryUpstreamReset above the data won't be buffered as the body exceeds the buffer limit + EXPECT_CALL(*router_->retry_state_, enabled()).WillOnce(Return(true)); + EXPECT_CALL(callbacks_, addDecodedData(_, _)).Times(0); + Buffer::OwnedImpl body("t"); + router_->decodeData(body, false); + EXPECT_EQ(1U, + callbacks_.route_->route_entry_.virtual_cluster_.stats().upstream_rq_total_.value()); + + Http::ResponseHeaderMapPtr response_headers( + new Http::TestResponseHeaderMapImpl{{":status", "200"}}); + response_decoder->decodeHeaders(std::move(response_headers), true); + // router filter no longer resets the stream due to upstream half closing first + router_->onDestroy(); +} + // Verifies that when the request fails with an upstream reset (per try timeout in this case) // before an upstream host has been established, then the onHostAttempted function will not be // invoked. This ensures that we're not passing a null host to the retry plugins. @@ -3855,6 +3888,10 @@ TEST_F(RouterTest, MaxStreamDurationWithRetryPolicy) { new Http::TestResponseHeaderMapImpl{{":status", "200"}}); response_decoder->decodeHeaders(std::move(response_headers), true); EXPECT_TRUE(verifyHostUpstreamStats(1, 1)); + // With upstream half close enabled router filter no longer resets the stream + // when upstream half closing first. As such it needs to be destroyed explicitly + // to avoid clean up problems in mocks + router_->onDestroy(); } TEST_F(RouterTest, RetryTimeoutDuringRetryDelayWithUpstreamRequestNoHost) { diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 5a83d26a03a6..68c23bb93f37 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -935,7 +935,7 @@ void HttpIntegrationTest::testRouterDownstreamDisconnectBeforeResponseComplete( EXPECT_EQ(512U, response->body().size()); } -void HttpIntegrationTest::testRouterUpstreamResponseBeforeRequestComplete() { +void HttpIntegrationTest::testRouterUpstreamResponseBeforeRequestComplete(uint32_t status_code) { initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); auto encoder_decoder = codec_client_->startRequest(default_request_headers_); @@ -943,6 +943,9 @@ void HttpIntegrationTest::testRouterUpstreamResponseBeforeRequestComplete() { ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); + if (status_code != 0) { + default_response_headers_.setStatus(status_code); + } upstream_request_->encodeHeaders(default_response_headers_, false); upstream_request_->encodeData(512, true); ASSERT_TRUE(response->waitForEndStream()); @@ -965,7 +968,8 @@ void HttpIntegrationTest::testRouterUpstreamResponseBeforeRequestComplete() { EXPECT_EQ(0U, upstream_request_->bodyLength()); EXPECT_TRUE(response->complete()); - EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_EQ(status_code != 0 ? absl::StrCat(status_code) : "200", + response->headers().getStatusValue()); EXPECT_EQ(512U, response->body().size()); } diff --git a/test/integration/http_integration.h b/test/integration/http_integration.h index e293aa4e90c2..1df61752bc27 100644 --- a/test/integration/http_integration.h +++ b/test/integration/http_integration.h @@ -266,7 +266,7 @@ class HttpIntegrationTest : public BaseIntegrationTest { ConnectionCreationFunction* creator = nullptr); void testRouterDownstreamDisconnectBeforeResponseComplete( ConnectionCreationFunction* creator = nullptr); - void testRouterUpstreamResponseBeforeRequestComplete(); + void testRouterUpstreamResponseBeforeRequestComplete(uint32_t status_code = 0); void testTwoRequests(bool force_network_backup = false); void testLargeHeaders(Http::TestRequestHeaderMapImpl request_headers, diff --git a/test/integration/multiplexed_integration_test.cc b/test/integration/multiplexed_integration_test.cc index 4f534ecaf40d..d17128117cbb 100644 --- a/test/integration/multiplexed_integration_test.cc +++ b/test/integration/multiplexed_integration_test.cc @@ -197,6 +197,9 @@ TEST_P(MultiplexedIntegrationTest, RouterUpstreamResponseBeforeRequestComplete) testRouterUpstreamResponseBeforeRequestComplete(); } +TEST_P(MultiplexedIntegrationTest, RouterUpstreamResponseWithErrorBeforeRequestComplete) { + testRouterUpstreamResponseBeforeRequestComplete(500); +} TEST_P(MultiplexedIntegrationTest, Retry) { testRetry(); } TEST_P(MultiplexedIntegrationTest, RetryAttemptCount) { testRetryAttemptCountHeader(); } diff --git a/test/integration/multiplexed_upstream_integration_test.cc b/test/integration/multiplexed_upstream_integration_test.cc index 1c21629e5982..d6cf22728fb8 100644 --- a/test/integration/multiplexed_upstream_integration_test.cc +++ b/test/integration/multiplexed_upstream_integration_test.cc @@ -78,6 +78,10 @@ TEST_P(MultiplexedUpstreamIntegrationTest, RouterUpstreamResponseBeforeRequestCo testRouterUpstreamResponseBeforeRequestComplete(); } +TEST_P(MultiplexedUpstreamIntegrationTest, RouterUpstreamResponseWithErrorBeforeRequestComplete) { + testRouterUpstreamResponseBeforeRequestComplete(400); +} + TEST_P(MultiplexedUpstreamIntegrationTest, Retry) { testRetry(); } TEST_P(MultiplexedUpstreamIntegrationTest, GrpcRetry) { testGrpcRetry(); } diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index fec8f47df756..4409f99968b1 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -4693,6 +4693,239 @@ TEST_P(ProtocolIntegrationTest, InvalidResponseHeaderNameStreamError) { ASSERT_TRUE(fake_upstream_connection_->connected()); } +// Validate that when allow_multiplexed_upstream_half_close is enabled a request with H/2 or H/3 +// upstream is not reset when upstream half closes before downstream and allows downstream to send +// data even if upstream is half closed. +// H/1 upstream always causes the stream to be closed if it responds before downstream. +// This test also causes downstream connection to run out of stream window (in H/2 and H/3 case) +// when processing END_STREAM from the server to make sure the data is not lost in the case. +TEST_P(ProtocolIntegrationTest, ServerHalfCloseBeforeClientWithBufferedResponseData) { + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"); + constexpr uint32_t kStreamWindowSize = 64 * 1024; + // Set buffer limit large enough to accommodate H/2 stream window, so we can cause downstream + // codec to buffer data without pushing back on upstream. + config_helper_.setBufferLimits(kStreamWindowSize * 2, kStreamWindowSize * 2); + + initialize(); + envoy::config::core::v3::Http2ProtocolOptions http2_options = + ::Envoy::Http2::Utility::initializeAndValidateOptions( + envoy::config::core::v3::Http2ProtocolOptions()) + .value(); + http2_options.mutable_initial_stream_window_size()->set_value(kStreamWindowSize); + codec_client_ = makeRawHttpConnection(makeClientConnection(lookupPort("http")), http2_options); + + auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{ + {":method", "POST"}, {":authority", "foo.lyft.com"}, {":path", "/"}, {":scheme", "http"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + + // Stop downstream client from updating the window (or reading for H/1) + request_encoder_->getStream().readDisable(true); + + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, false); + // Make downstream stream to run out of window + upstream_request_->encodeData(kStreamWindowSize, false); + // And cause the rest of data to the downstream to be buffered + upstream_request_->encodeData(kStreamWindowSize / 4, true); + + if (fake_upstreams_[0]->httpType() == Http::CodecType::HTTP1) { + // H/1 upstream always causes the stream to be closed if it responds before downstream. + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + if (downstreamProtocol() == Http::CodecType::HTTP1) { + request_encoder_->getStream().readDisable(false); + ASSERT_TRUE(codec_client_->waitForDisconnect()); + } else if (downstreamProtocol() == Http::CodecType::HTTP2) { + ASSERT_TRUE(response->waitForReset()); + } else if (downstreamProtocol() == Http::CodecType::HTTP3) { + // Unlike H/2 codec H/3 codec attempts to send pending data before the reset + // So it needs window to push the data to the client and then the reset + // which just gets discarded since end_stream has been received just before + // reset. + request_encoder_->getStream().readDisable(false); + ASSERT_TRUE(response->waitForEndStream()); + } + } else if (fake_upstreams_[0]->httpType() == Http::CodecType::HTTP2 || + fake_upstreams_[0]->httpType() == Http::CodecType::HTTP3) { + // H/2 or H/3 upstreams should allow downstream to send data even after upstream has half + // closed. + request_encoder_->getStream().readDisable(false); + if (downstreamProtocol() == Http::CodecType::HTTP1) { + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + ASSERT_EQ(80 * 1024, response->body().length()); + // Codec client does not allow us to gracefully finish the request, since + // as soon it observes completion on H/1 response it closes the entire stream. + // The H2UpstreamHalfCloseBeforeH1Downstream test that uses TCP client to fully + // test this case. + codec_client_->close(); + ASSERT_TRUE(upstream_request_->waitForReset()); + } else if (downstreamProtocol() == Http::CodecType::HTTP2 || + downstreamProtocol() == Http::CodecType::HTTP3) { + ASSERT_TRUE(response->waitForEndStream()); + std::string data(128, 'a'); + ASSERT_TRUE(response->complete()); + ASSERT_EQ(80 * 1024, response->body().length()); + Buffer::OwnedImpl buffer(data); + request_encoder_->encodeData(buffer, true); + ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, 128)); + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + } + } +} + +// Verify that even with upstream half close enabled the error response from +// upstream causes the request to be reset. +TEST_P(ProtocolIntegrationTest, ServerHalfCloseWithErrorBeforeClient) { + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"); + + initialize(); + envoy::config::core::v3::Http2ProtocolOptions http2_options = + ::Envoy::Http2::Utility::initializeAndValidateOptions( + envoy::config::core::v3::Http2ProtocolOptions()) + .value(); + http2_options.mutable_initial_stream_window_size()->set_value(64 * 1024); + codec_client_ = makeRawHttpConnection(makeClientConnection(lookupPort("http")), http2_options); + + auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{ + {":method", "POST"}, {":authority", "foo.lyft.com"}, {":path", "/"}, {":scheme", "http"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "400"}}, true); + + if (fake_upstreams_[0]->httpType() == Http::CodecType::HTTP1) { + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + } else { + ASSERT_TRUE(upstream_request_->waitForReset()); + } + + if (downstreamProtocol() == Http::CodecType::HTTP1) { + ASSERT_TRUE(codec_client_->waitForDisconnect()); + } else if (downstreamProtocol() == Http::CodecType::HTTP2) { + ASSERT_TRUE(response->waitForReset()); + } else if (downstreamProtocol() == Http::CodecType::HTTP3) { + ASSERT_TRUE(response->waitForEndStream()); + } + ASSERT_TRUE(response->complete()); + ASSERT_EQ("400", response->headers().getStatusValue()); +} + +// Same as above but with data sent after the error response. +// Note the behavior is the same as when the allow_multiplexed_upstream_half_close is false. +TEST_P(ProtocolIntegrationTest, ServerHalfCloseBeforeClientWithErrorAndBufferedResponseData) { + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"); + constexpr uint32_t kStreamWindowSize = 64 * 1024; + // Set buffer limit large enough to accommodate H/2 stream window + config_helper_.setBufferLimits(kStreamWindowSize * 2, kStreamWindowSize * 2); + + initialize(); + envoy::config::core::v3::Http2ProtocolOptions http2_options = + ::Envoy::Http2::Utility::initializeAndValidateOptions( + envoy::config::core::v3::Http2ProtocolOptions()) + .value(); + http2_options.mutable_initial_stream_window_size()->set_value(kStreamWindowSize); + codec_client_ = makeRawHttpConnection(makeClientConnection(lookupPort("http")), http2_options); + + auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{ + {":method", "POST"}, {":authority", "foo.lyft.com"}, {":path", "/"}, {":scheme", "http"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + + // Stop downstream client updating the window (or reading for H/1) + request_encoder_->getStream().readDisable(true); + + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "404"}}, false); + // Make downstream stream to run out of window + upstream_request_->encodeData(kStreamWindowSize, false); + // And cause the rest of data to the downstream to be buffered + upstream_request_->encodeData(kStreamWindowSize / 4, true); + + if (fake_upstreams_[0]->httpType() == Http::CodecType::HTTP1) { + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + if (downstreamProtocol() == Http::CodecType::HTTP1) { + request_encoder_->getStream().readDisable(false); + ASSERT_TRUE(codec_client_->waitForDisconnect()); + } else if (downstreamProtocol() == Http::CodecType::HTTP2) { + ASSERT_TRUE(response->waitForReset()); + } else if (downstreamProtocol() == Http::CodecType::HTTP3) { + // Unlike H/2 codec H/3 codec attempts to send pending data before the reset + // So it needs window to push the data to the client and then the reset + // which just gets discarded since end_stream has been received just before + // reset. + request_encoder_->getStream().readDisable(false); + ASSERT_TRUE(response->waitForEndStream()); + } + } else if (fake_upstreams_[0]->httpType() == Http::CodecType::HTTP2 || + fake_upstreams_[0]->httpType() == Http::CodecType::HTTP3) { + request_encoder_->getStream().readDisable(false); + if (downstreamProtocol() == Http::CodecType::HTTP1) { + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + ASSERT_EQ(80 * 1024, response->body().length()); + codec_client_->close(); + ASSERT_TRUE(upstream_request_->waitForReset()); + } else if (downstreamProtocol() == Http::CodecType::HTTP2 || + downstreamProtocol() == Http::CodecType::HTTP3) { + ASSERT_TRUE(upstream_request_->waitForReset()); + ASSERT_TRUE(response->waitForReset()); + } + } +} + +TEST_P(ProtocolIntegrationTest, H2UpstreamHalfCloseBeforeH1Dowstream) { + // This test is only for H/1 downstream and H/2 or H/3 upstream + // Other cases are covered by the ServerHalfCloseBeforeClientWithBufferedResponseData + // It verifies that H/1 downstream request is not reset when H/2 upstream completes the stream + // before the downstream and can still send data to the upstream. + if (downstreamProtocol() != Http::CodecType::HTTP1 || + upstreamProtocol() == Http::CodecType::HTTP1) { + return; + } + + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"); + constexpr uint32_t kStreamChunkSize = 64 * 1024; + config_helper_.setBufferLimits(kStreamChunkSize * 2, kStreamChunkSize * 2); + initialize(); + + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("http")); + + ASSERT_TRUE(tcp_client->write( + "POST / HTTP/1.1\r\nHost: foo.lyft.com\r\nTransfer-Encoding: chunked\r\n\r\n", false, false)); + + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, false); + upstream_request_->encodeData(kStreamChunkSize, false); + upstream_request_->encodeData(kStreamChunkSize / 4, true); + + // Wait for the last chunk to arrive + tcp_client->waitForData("\r\n0\r\n\r\n", false); + ASSERT_TRUE(tcp_client->connected()); + + // Now write data into downstream client after upstream has completed its response and verify that + // upstream receives it. + ASSERT_TRUE(tcp_client->write(absl::StrCat("80\r\n", std::string(0x80, 'A'), "\r\n0\r\n\r\n"), + false, false)); + + ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, 0x80)); + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + tcp_client->close(); +} + TEST_P(DownstreamProtocolIntegrationTest, DuplicatedSchemeHeaders) { disable_client_header_validation_ = true; if (downstreamProtocol() == Http::CodecType::HTTP1) { diff --git a/test/integration/tcp_tunneling_integration_test.cc b/test/integration/tcp_tunneling_integration_test.cc index 63760daba054..820adda0d7b3 100644 --- a/test/integration/tcp_tunneling_integration_test.cc +++ b/test/integration/tcp_tunneling_integration_test.cc @@ -291,6 +291,40 @@ TEST_P(ConnectTerminationIntegrationTest, UpstreamClose) { expected_header_bytes_sent, expected_header_bytes_received); } +TEST_P(ConnectTerminationIntegrationTest, UpstreamCloseWithHalfCloseEnabled) { + // When allow_multiplexed_upstream_half_close is enabled router and filter + // manager do not reset streams where upstream ended before downstream. + // Verify that stream with TCP proxy upstream is closed as soon as the upstream TCP + // connection is half closed even if the allow_multiplexed_upstream_half_close is enabled. + // In this case the stream is reset in the TcpUpstream::onUpstreamData + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"); + initialize(); + + setUpConnection(); + sendBidirectionalData(); + + // Tear down by closing the upstream connection. + ASSERT_TRUE(fake_raw_upstream_connection_->close()); + if (downstream_protocol_ == Http::CodecType::HTTP3) { + // In HTTP/3 end stream will be sent when the upstream connection is closed, and + // STOP_SENDING frame sent instead of reset. + ASSERT_TRUE(response_->waitForEndStream()); + ASSERT_TRUE(response_->waitForReset()); + } else if (downstream_protocol_ == Http::CodecType::HTTP2) { + ASSERT_TRUE(response_->waitForReset()); + } else { + ASSERT_TRUE(codec_client_->waitForDisconnect()); + } + const int expected_wire_bytes_sent = 5; // Envoy sends "hello". + const int expected_wire_bytes_received = 6; // Upstream sends "there!". + // expected_header_bytes_* is 0 as we do not use proxy protocol. + const int expected_header_bytes_sent = 0; + const int expected_header_bytes_received = 0; + checkAccessLogOutput(expected_wire_bytes_sent, expected_wire_bytes_received, + expected_header_bytes_sent, expected_header_bytes_received); +} + TEST_P(ConnectTerminationIntegrationTest, TestTimeout) { enable_timeout_ = true; initialize(); From fd17760d2675fef1c51832b4e268f74eb775da39 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 21 Jun 2024 18:02:20 +0000 Subject: [PATCH 02/27] Address comments Signed-off-by: Yan Avlasov --- source/common/http/async_client_impl.cc | 2 -- source/common/http/conn_manager_impl.h | 4 +-- source/common/http/filter_manager.cc | 20 +++++++++------ source/common/http/filter_manager.h | 25 ++++++++----------- source/common/router/upstream_request.cc | 2 +- .../upstreams/http/tcp/upstream_request.cc | 8 +++--- test/common/http/conn_manager_impl_test_2.cc | 5 +++- test/integration/protocol_integration_test.cc | 2 +- 8 files changed, 35 insertions(+), 33 deletions(-) diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index f9daa91d13a0..3c1e1eb5d3e8 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -146,8 +146,6 @@ void AsyncStreamImpl::encodeHeaders(ResponseHeaderMapPtr&& headers, bool end_str stream_callbacks_.onHeaders(std::move(headers), end_stream); closeRemote(end_stream); // At present, the AsyncStream is always fully closed when the server half closes the stream. - // This is the case even when allow_multiplexed_upstream_half_close runtime flag is set, as there - // are currently no known use cases where early server half close needs to be supported. // // Always ensure we close locally to trigger completion. Another option would be to issue a stream // reset here if local isn't yet closed, triggering cleanup along a more standardized path. diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index ced126265fbe..ae1b3b8ba801 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -283,9 +283,7 @@ class ConnectionManagerImpl : Logger::Loggable, OptRef tracingConfig() const override; const ScopeTrackedObject& scope() override; OptRef downstreamCallbacks() override { return *this; } - bool isHalfCloseEnabled() override { - return filter_manager_.allowUpstreamHalfClose() ? true : false; - } + bool isHalfCloseEnabled() override { return filter_manager_.allowUpstreamHalfClose(); } // DownstreamStreamFilterCallbacks void setRoute(Router::RouteConstSharedPtr route) override; diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 3485948de220..dec8407f9c5d 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -906,7 +906,7 @@ FilterManager::commonEncodePrefix(ActiveStreamEncoderFilter* filter, bool end_st ENVOY_STREAM_LOG(trace, "commonEncodePrefix end_stream: {}, isHalfCloseEnabled: {}, force_close: {}", *this, end_stream, filter_manager_callbacks_.isHalfCloseEnabled(), - static_cast(state_.force_close_stream_)); + static_cast(state_.should_force_close_stream_)); if (filter == nullptr) { // half close is enabled in case tcp proxying is done with http1 encoder. In this case, we // should not set the local_complete_ flag to true when end_stream is true. @@ -915,7 +915,7 @@ FilterManager::commonEncodePrefix(ActiveStreamEncoderFilter* filter, bool end_st if (allow_upstream_half_close_) { if (end_stream) { state_.encoder_end_stream_ = true; - if (!filter_manager_callbacks_.isHalfCloseEnabled() || state_.force_close_stream_) { + if (!filter_manager_callbacks_.isHalfCloseEnabled() || state_.should_force_close_stream_) { ASSERT(!state_.local_complete_); state_.local_complete_ = true; } @@ -973,7 +973,7 @@ void DownstreamFilterManager::sendLocalReply( const bool is_head_request = state_.is_head_request_; const bool is_grpc_request = state_.is_grpc_request_; // Local reply closes the stream even if downstream is not half closed. - state_.force_close_stream_ = true; + state_.should_force_close_stream_ = true; // Stop filter chain iteration if local reply was sent while filter decoding or encoding callbacks // are running. @@ -1303,7 +1303,7 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea if (!(Http::CodeUtility::is2xx(response_status) || Http::CodeUtility::is1xx(response_status))) { // Even if upstream half close is enabled the stream is closed on error responses from the // server. - state_.force_close_stream_ = true; + state_.should_force_close_stream_ = true; } } filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream); @@ -1512,7 +1512,7 @@ void FilterManager::maybeEndEncode(bool end_stream) { ASSERT(!state_.remote_encode_complete_); state_.remote_encode_complete_ = true; if (allow_upstream_half_close_) { - maybeCloseStream(); + checkAndCloseStreamIfFullyClosed(); } else { state_.stream_closed_ = true; filter_manager_callbacks_.endStream(); @@ -1520,8 +1520,12 @@ void FilterManager::maybeEndEncode(bool end_stream) { } } -void FilterManager::maybeCloseStream() { - ASSERT(allow_upstream_half_close_); +void FilterManager::checkAndCloseStreamIfFullyClosed() { + // This function is only used when half close semantics are enabled. + if (!allow_upstream_half_close_) { + return; + } + if (state_.stream_closed_) { return; } @@ -1529,7 +1533,7 @@ void FilterManager::maybeCloseStream() { // If upstream half close is enabled then close the stream either when force close // is set (i.e local reply) or when both server and client half closed. if (state_.remote_encode_complete_ && - (state_.remote_decode_complete_ || state_.force_close_stream_)) { + (state_.remote_decode_complete_ || state_.should_force_close_stream_)) { state_.stream_closed_ = true; ENVOY_STREAM_LOG(trace, "closing stream", *this); filter_manager_callbacks_.endStream(); diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 1b434496efcf..72d552690051 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -659,7 +659,10 @@ class FilterManager : public ScopeTrackedObject, // ScopeTrackedObject void dumpState(std::ostream& os, int indent_level = 0) const override { const char* spaces = spacesForLevel(indent_level); - os << spaces << "FilterManager " << this << DUMP_MEMBER(state_.has_1xx_headers_) << "\n"; + os << spaces << "FilterManager " << this << DUMP_MEMBER(state_.has_1xx_headers_) + << DUMP_MEMBER(state_.remote_decode_complete_) << DUMP_MEMBER(state_.remote_encode_complete_) + << DUMP_MEMBER(state_.encoder_end_stream_) << DUMP_MEMBER(state_.stream_closed_) + << DUMP_MEMBER(state_.should_force_close_stream_) << "\n"; DUMP_DETAILS(filter_manager_callbacks_.requestHeaders()); DUMP_DETAILS(filter_manager_callbacks_.requestTrailers()); @@ -750,9 +753,7 @@ class FilterManager : public ScopeTrackedObject, void decodeHeaders(RequestHeaderMap& headers, bool end_stream) { state_.remote_decode_complete_ = end_stream; decodeHeaders(nullptr, headers, end_stream); - if (allow_upstream_half_close_) { - maybeCloseStream(); - } + checkAndCloseStreamIfFullyClosed(); } /** @@ -763,9 +764,7 @@ class FilterManager : public ScopeTrackedObject, void decodeData(Buffer::Instance& data, bool end_stream) { state_.remote_decode_complete_ = end_stream; decodeData(nullptr, data, end_stream, FilterIterationStartState::CanStartFromCurrent); - if (allow_upstream_half_close_) { - maybeCloseStream(); - } + checkAndCloseStreamIfFullyClosed(); } /** @@ -775,9 +774,7 @@ class FilterManager : public ScopeTrackedObject, void decodeTrailers(RequestTrailerMap& trailers) { state_.remote_decode_complete_ = true; decodeTrailers(nullptr, trailers); - if (allow_upstream_half_close_) { - maybeCloseStream(); - } + checkAndCloseStreamIfFullyClosed(); } /** @@ -794,7 +791,7 @@ class FilterManager : public ScopeTrackedObject, */ void maybeEndEncode(bool end_stream); - void maybeCloseStream(); + void checkAndCloseStreamIfFullyClosed(); virtual void sendLocalReply(Code code, absl::string_view body, const std::function& modify_headers, @@ -884,7 +881,7 @@ class FilterManager : public ScopeTrackedObject, created_filter_chain_(false), is_head_request_(false), is_grpc_request_(false), non_100_response_headers_encoded_(false), under_on_local_reply_(false), decoder_filter_chain_aborted_(false), encoder_filter_chain_aborted_(false), - saw_downstream_reset_(false), stream_closed_(false), force_close_stream_(false) {} + saw_downstream_reset_(false), stream_closed_(false), should_force_close_stream_(false) {} uint32_t filter_call_state_{0}; bool remote_decode_complete_ : 1; // Set when decoder filter chain iteration has completed. @@ -914,8 +911,8 @@ class FilterManager : public ScopeTrackedObject, bool stream_closed_ : 1; // Set when both remote_decode_complete_ and remote_encode_complete_ is // true observed for the first time and prevents ending the stream // multiple times. Only set when allow_upstream_half_close is enabled. - bool force_close_stream_ : 1; // Set to indicate that stream should be closed due to either - // local reply or error response from the server. + bool should_force_close_stream_ : 1; // Set to indicate that stream should be fully closed due + // to either local reply or error response from the server. // The following 3 members are booleans rather than part of the space-saving bitfield as they // are passed as arguments to functions expecting bools. Extend State using the bitfield diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index 63527b2c1fac..69f85d62e50a 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -71,7 +71,7 @@ class UpstreamFilterManager : public Http::FilterManager { state().remote_encode_complete_ = true; state().encoder_end_stream_ = true; state().local_complete_ = true; - state().force_close_stream_ = true; + state().should_force_close_stream_ = true; // TODO(alyssawilk) this should be done through the router to play well with hedging. upstream_request_.parent_.callbacks()->sendLocalReply(code, body, modify_headers, grpc_status, details); diff --git a/source/extensions/upstreams/http/tcp/upstream_request.cc b/source/extensions/upstreams/http/tcp/upstream_request.cc index da44c2bbccd0..e92d8f93df6a 100644 --- a/source/extensions/upstreams/http/tcp/upstream_request.cc +++ b/source/extensions/upstreams/http/tcp/upstream_request.cc @@ -107,9 +107,11 @@ void TcpUpstream::onUpstreamData(Buffer::Instance& data, bool end_stream) { // // Save the indicator to close the stream before calling the decodeData since when the // allow_multiplexed_upstream_half_close is false the call to decodeHeader with end_stream==true - // will delete the TcpUpstream object. NOTE: it this point Envoy can not support half closed TCP - // upstream as there is currently no detection of half closed vs fully closed TCP connections. - bool force_reset = force_reset_on_upstream_half_close_ && end_stream && !downstream_complete_; + // will delete the TcpUpstream object. + // NOTE: it this point Envoy can not support half closed TCP upstream as there is currently no + // distinction between half closed vs fully closed TCP peers. + const bool force_reset = + force_reset_on_upstream_half_close_ && end_stream && !downstream_complete_; bytes_meter_->addWireBytesReceived(data.length()); upstream_request_->decodeData(data, end_stream); // force_reset is true only when allow_multiplexed_upstream_half_close is true and in this case diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index 439340ee9cb6..3e980e2ebf96 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -571,7 +571,10 @@ TEST_F(HttpConnectionManagerImplTest, ConnectionDuration) { EXPECT_EQ(1U, stats_.named_.downstream_cx_max_duration_reached_.value()); } -TEST_F(HttpConnectionManagerImplTest, DISABLED_IntermediateBufferingEarlyResponse) { +TEST_F(HttpConnectionManagerImplTest, IntermediateBufferingEarlyResponse) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "false"}}); setup(false, ""); setupFilterChain(2, 0); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 4409f99968b1..3d85f32950c9 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -4884,7 +4884,7 @@ TEST_P(ProtocolIntegrationTest, ServerHalfCloseBeforeClientWithErrorAndBufferedR } } -TEST_P(ProtocolIntegrationTest, H2UpstreamHalfCloseBeforeH1Dowstream) { +TEST_P(ProtocolIntegrationTest, H2UpstreamHalfCloseBeforeH1Downstream) { // This test is only for H/1 downstream and H/2 or H/3 upstream // Other cases are covered by the ServerHalfCloseBeforeClientWithBufferedResponseData // It verifies that H/1 downstream request is not reset when H/2 upstream completes the stream From 1d8172c92b69272f04d7ff954a94b5312d533099 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Sat, 22 Jun 2024 00:04:38 +0000 Subject: [PATCH 03/27] Address comments Signed-off-by: Yan Avlasov --- test/integration/protocol_integration_test.cc | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 37639a41c8f8..79987902e55d 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -4729,6 +4729,8 @@ TEST_P(ProtocolIntegrationTest, InvalidResponseHeaderNameStreamError) { TEST_P(ProtocolIntegrationTest, ServerHalfCloseBeforeClientWithBufferedResponseData) { config_helper_.addRuntimeOverride( "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"); + useAccessLog("%DURATION% %REQUEST_DURATION% %REQUEST_TX_DURATION% %RESPONSE_DURATION% " + "%RESPONSE_TX_DURATION%"); constexpr uint32_t kStreamWindowSize = 64 * 1024; // Set buffer limit large enough to accommodate H/2 stream window, so we can cause downstream // codec to buffer data without pushing back on upstream. @@ -4802,6 +4804,17 @@ TEST_P(ProtocolIntegrationTest, ServerHalfCloseBeforeClientWithBufferedResponseD ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); } } + + std::string timing = waitForAccessLog(access_log_name_); + if (fake_upstreams_[0]->httpType() != Http::CodecType::HTTP1 && + downstreamProtocol() != Http::CodecType::HTTP1) { + // All duration values should be present (no '-' in the access log) when neither upstream nor + // downstream is H/1 + ASSERT_FALSE(absl::StrContains(timing, '-')); + } else { + // When one the peers is H/1 the stream is reset and request duration values will be unset + ASSERT_TRUE(absl::StrContains(timing, " - - ")); + } } // Verify that even with upstream half close enabled the error response from @@ -4923,6 +4936,8 @@ TEST_P(ProtocolIntegrationTest, H2UpstreamHalfCloseBeforeH1Downstream) { config_helper_.addRuntimeOverride( "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"); + useAccessLog("%DURATION% %REQUEST_DURATION% %REQUEST_TX_DURATION% %RESPONSE_DURATION% " + "%RESPONSE_TX_DURATION%"); constexpr uint32_t kStreamChunkSize = 64 * 1024; config_helper_.setBufferLimits(kStreamChunkSize * 2, kStreamChunkSize * 2); initialize(); @@ -4951,6 +4966,9 @@ TEST_P(ProtocolIntegrationTest, H2UpstreamHalfCloseBeforeH1Downstream) { ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, 0x80)); ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); tcp_client->close(); + std::string timing = waitForAccessLog(access_log_name_); + // All duration values should be present (no '-' in the access log) + ASSERT_FALSE(absl::StrContains(timing, '-')); } TEST_P(DownstreamProtocolIntegrationTest, DuplicatedSchemeHeaders) { From 3ba7cc4d5f40d3cd8373bad5405e4b11b1b54b76 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Mon, 24 Jun 2024 18:45:49 +0000 Subject: [PATCH 04/27] Clarify comment Signed-off-by: Yan Avlasov --- source/common/http/filter_manager.h | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 72d552690051..b9ad871d7083 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -884,14 +884,23 @@ class FilterManager : public ScopeTrackedObject, saw_downstream_reset_(false), stream_closed_(false), should_force_close_stream_(false) {} uint32_t filter_call_state_{0}; - bool remote_decode_complete_ : 1; // Set when decoder filter chain iteration has completed. - bool remote_encode_complete_ : 1; // Set when encoder filter chain iteration has completed. + bool remote_decode_complete_ : 1; // Set when decoder filter chain iteration has completed. All + // decoder filters have completed and end_stream was observed. + bool remote_encode_complete_ : 1; // Set when encoder filter chain iteration has completed. All + // decoder filters have completed and end_stream was observed. + // TODO(yanavlasov): always use encoder_end_stream_ for tracking end_stream state during encoder + // filter iteration. bool encoder_end_stream_ : 1; // This is set the first time the end_stream is observed during - // encoder filter chain iteration and used to set the end_stream - // flag when resuming encoder filter chain iteration. - bool local_complete_ : 1; // This indicates that local is complete prior to filter processing. - // A filter can still stop the stream from being complete as seen - // by the codec. + // encoder filter chain iteration and used only to set the + // end_stream flag when resuming encoder filter chain iteration. + // It is only used when the + // `allow_multiplexed_upstream_half_close` runtime flag is true. + bool local_complete_ : 1; // This indicates that request is complete prior to filter processing. + // This flag terminates decoder filter chain iteration. + // It is also used to track the `end_stream` state during encoder + // filter chain iteration when `allow_multiplexed_upstream_half_close` + // runtime flag is false. A filter can still stop the stream from + // being complete as seen by the codec. // By default, we will assume there are no 1xx. If encode1xxHeaders // is ever called, this is set to true so commonContinue resumes processing the 1xx. bool has_1xx_headers_ : 1; From 6133efcf000b877a1a5a381fa2650b71b95e0ca5 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Mon, 24 Jun 2024 19:46:19 +0000 Subject: [PATCH 05/27] Reuse isHalfCloseEnabled callback Signed-off-by: Yan Avlasov --- source/common/http/conn_manager_impl.cc | 6 ++++-- source/common/http/conn_manager_impl.h | 3 ++- source/common/http/filter_manager.cc | 16 ++++++++-------- source/common/http/filter_manager.h | 8 +++----- 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index d764ae1947d1..aa59e6afe9d7 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -126,8 +126,10 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfigSharedPtr co /*node_id=*/local_info_.node().id(), /*server_name=*/config_->serverName(), /*proxy_status_config=*/config_->proxyStatusConfig())), - max_requests_during_dispatch_(runtime_.snapshot().getInteger( - ConnectionManagerImpl::MaxRequestsPerIoCycle, UINT32_MAX)) { + max_requests_during_dispatch_( + runtime_.snapshot().getInteger(ConnectionManagerImpl::MaxRequestsPerIoCycle, UINT32_MAX)), + allow_upstream_half_close_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close")) { ENVOY_LOG_ONCE_IF( trace, accept_new_http_stream_ == nullptr, "LoadShedPoint envoy.load_shed_points.http_connection_manager_decode_headers is not " diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 4332f7e463b2..8200d6d2eb3c 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -283,7 +283,7 @@ class ConnectionManagerImpl : Logger::Loggable, OptRef tracingConfig() const override; const ScopeTrackedObject& scope() override; OptRef downstreamCallbacks() override { return *this; } - bool isHalfCloseEnabled() override { return filter_manager_.allowUpstreamHalfClose(); } + bool isHalfCloseEnabled() override { return connection_manager_.allow_upstream_half_close_; } // DownstreamStreamFilterCallbacks void setRoute(Router::RouteConstSharedPtr route) override; @@ -638,6 +638,7 @@ class ConnectionManagerImpl : Logger::Loggable, uint32_t requests_during_dispatch_count_{0}; const uint32_t max_requests_during_dispatch_{UINT32_MAX}; Event::SchedulableCallbackPtr deferred_request_processing_callback_; + const bool allow_upstream_half_close_{}; }; } // namespace Http diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index a98a1c139220..9585105ddb06 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -910,16 +910,16 @@ FilterManager::commonEncodePrefix(ActiveStreamEncoderFilter* filter, bool end_st // should not set the local_complete_ flag to true when end_stream is true. // setting local_complete_ to true will cause any data sent in the upstream direction to be // dropped. - if (allow_upstream_half_close_) { + if (filter_manager_callbacks_.isHalfCloseEnabled()) { if (end_stream) { state_.encoder_end_stream_ = true; - if (!filter_manager_callbacks_.isHalfCloseEnabled() || state_.should_force_close_stream_) { + if (state_.should_force_close_stream_) { ASSERT(!state_.local_complete_); state_.local_complete_ = true; } } } else { - if (end_stream && !filter_manager_callbacks_.isHalfCloseEnabled()) { + if (end_stream) { ASSERT(!state_.local_complete_); state_.local_complete_ = true; } @@ -1296,7 +1296,7 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea const bool modified_end_stream = (end_stream && continue_data_entry == encoder_filters_.end()); state_.non_100_response_headers_encoded_ = true; - if (allow_upstream_half_close_) { + if (filter_manager_callbacks_.isHalfCloseEnabled()) { const uint64_t response_status = Http::Utility::getResponseStatus(headers); if (!(Http::CodeUtility::is2xx(response_status) || Http::CodeUtility::is1xx(response_status))) { // Even if upstream half close is enabled the stream is closed on error responses from the @@ -1509,7 +1509,7 @@ void FilterManager::maybeEndEncode(bool end_stream) { if (end_stream) { ASSERT(!state_.remote_encode_complete_); state_.remote_encode_complete_ = true; - if (allow_upstream_half_close_) { + if (filter_manager_callbacks_.isHalfCloseEnabled()) { checkAndCloseStreamIfFullyClosed(); } else { state_.stream_closed_ = true; @@ -1520,7 +1520,7 @@ void FilterManager::maybeEndEncode(bool end_stream) { void FilterManager::checkAndCloseStreamIfFullyClosed() { // This function is only used when half close semantics are enabled. - if (!allow_upstream_half_close_) { + if (!filter_manager_callbacks_.isHalfCloseEnabled()) { return; } @@ -1721,8 +1721,8 @@ bool ActiveStreamEncoderFilter::complete() { // When the upstream half close is enabled the local complete may not be set when // end stream is observed on the encode path in case upstream half closes before the // downstream. - return parent_.allow_upstream_half_close_ ? parent_.state_.encoder_end_stream_ - : parent_.state_.local_complete_; + return parent_.filter_manager_callbacks_.isHalfCloseEnabled() ? parent_.state_.encoder_end_stream_ + : parent_.state_.local_complete_; } bool ActiveStreamEncoderFilter::has1xxHeaders() { return parent_.state_.has_1xx_headers_ && !continued_1xx_headers_; diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index b9ad871d7083..9e6ee3403bf7 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -648,9 +648,7 @@ class FilterManager : public ScopeTrackedObject, proxy_100_continue_(proxy_100_continue), buffer_limit_(buffer_limit), filter_chain_factory_(filter_chain_factory), no_downgrade_to_canonical_name_(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.no_downgrade_to_canonical_name")), - allow_upstream_half_close_(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.allow_multiplexed_upstream_half_close")) {} + "envoy.reloadable_features.no_downgrade_to_canonical_name")) {} ~FilterManager() override { ASSERT(state_.destroyed_); ASSERT(state_.filter_call_state_ == 0); @@ -871,7 +869,8 @@ class FilterManager : public ScopeTrackedObject, bool sawDownstreamReset() { return state_.saw_downstream_reset_; } virtual bool shouldLoadShed() { return false; }; - bool allowUpstreamHalfClose() const { return allow_upstream_half_close_; } + + void stopDecoding() { state_.should_force_close_stream_ = true; } protected: struct State { @@ -1115,7 +1114,6 @@ class FilterManager : public ScopeTrackedObject, State state_; const bool no_downgrade_to_canonical_name_{}; - const bool allow_upstream_half_close_{}; }; // The DownstreamFilterManager has explicit handling to send local replies. From fcf2a7682245020b69c33f596626fa3f81b29161 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Mon, 24 Jun 2024 20:22:03 +0000 Subject: [PATCH 06/27] Add stopDecoding Signed-off-by: Yan Avlasov --- source/common/http/filter_manager.cc | 16 ++++++++-------- source/common/http/filter_manager.h | 10 +++++----- source/common/router/upstream_request.cc | 2 +- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 9585105ddb06..4d1a20ae54b5 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -904,7 +904,7 @@ FilterManager::commonEncodePrefix(ActiveStreamEncoderFilter* filter, bool end_st ENVOY_STREAM_LOG(trace, "commonEncodePrefix end_stream: {}, isHalfCloseEnabled: {}, force_close: {}", *this, end_stream, filter_manager_callbacks_.isHalfCloseEnabled(), - static_cast(state_.should_force_close_stream_)); + static_cast(state_.should_stop_decoding_)); if (filter == nullptr) { // half close is enabled in case tcp proxying is done with http1 encoder. In this case, we // should not set the local_complete_ flag to true when end_stream is true. @@ -913,7 +913,7 @@ FilterManager::commonEncodePrefix(ActiveStreamEncoderFilter* filter, bool end_st if (filter_manager_callbacks_.isHalfCloseEnabled()) { if (end_stream) { state_.encoder_end_stream_ = true; - if (state_.should_force_close_stream_) { + if (state_.should_stop_decoding_) { ASSERT(!state_.local_complete_); state_.local_complete_ = true; } @@ -970,8 +970,8 @@ void DownstreamFilterManager::sendLocalReply( ASSERT(!state_.under_on_local_reply_); const bool is_head_request = state_.is_head_request_; const bool is_grpc_request = state_.is_grpc_request_; - // Local reply closes the stream even if downstream is not half closed. - state_.should_force_close_stream_ = true; + // Local reply stops decoding of downstream request. + stopDecoding(); // Stop filter chain iteration if local reply was sent while filter decoding or encoding callbacks // are running. @@ -1299,9 +1299,9 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea if (filter_manager_callbacks_.isHalfCloseEnabled()) { const uint64_t response_status = Http::Utility::getResponseStatus(headers); if (!(Http::CodeUtility::is2xx(response_status) || Http::CodeUtility::is1xx(response_status))) { - // Even if upstream half close is enabled the stream is closed on error responses from the - // server. - state_.should_force_close_stream_ = true; + // When the upstream half close is enabled the stream decoding is stopped on error responses + // from the server. + stopDecoding(); } } filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream); @@ -1531,7 +1531,7 @@ void FilterManager::checkAndCloseStreamIfFullyClosed() { // If upstream half close is enabled then close the stream either when force close // is set (i.e local reply) or when both server and client half closed. if (state_.remote_encode_complete_ && - (state_.remote_decode_complete_ || state_.should_force_close_stream_)) { + (state_.remote_decode_complete_ || state_.should_stop_decoding_)) { state_.stream_closed_ = true; ENVOY_STREAM_LOG(trace, "closing stream", *this); filter_manager_callbacks_.endStream(); diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 9e6ee3403bf7..20742c55f2d9 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -660,7 +660,7 @@ class FilterManager : public ScopeTrackedObject, os << spaces << "FilterManager " << this << DUMP_MEMBER(state_.has_1xx_headers_) << DUMP_MEMBER(state_.remote_decode_complete_) << DUMP_MEMBER(state_.remote_encode_complete_) << DUMP_MEMBER(state_.encoder_end_stream_) << DUMP_MEMBER(state_.stream_closed_) - << DUMP_MEMBER(state_.should_force_close_stream_) << "\n"; + << DUMP_MEMBER(state_.should_stop_decoding_) << "\n"; DUMP_DETAILS(filter_manager_callbacks_.requestHeaders()); DUMP_DETAILS(filter_manager_callbacks_.requestTrailers()); @@ -870,7 +870,7 @@ class FilterManager : public ScopeTrackedObject, virtual bool shouldLoadShed() { return false; }; - void stopDecoding() { state_.should_force_close_stream_ = true; } + void stopDecoding() { state_.should_stop_decoding_ = true; } protected: struct State { @@ -880,7 +880,7 @@ class FilterManager : public ScopeTrackedObject, created_filter_chain_(false), is_head_request_(false), is_grpc_request_(false), non_100_response_headers_encoded_(false), under_on_local_reply_(false), decoder_filter_chain_aborted_(false), encoder_filter_chain_aborted_(false), - saw_downstream_reset_(false), stream_closed_(false), should_force_close_stream_(false) {} + saw_downstream_reset_(false), stream_closed_(false), should_stop_decoding_(false) {} uint32_t filter_call_state_{0}; bool remote_decode_complete_ : 1; // Set when decoder filter chain iteration has completed. All @@ -919,8 +919,8 @@ class FilterManager : public ScopeTrackedObject, bool stream_closed_ : 1; // Set when both remote_decode_complete_ and remote_encode_complete_ is // true observed for the first time and prevents ending the stream // multiple times. Only set when allow_upstream_half_close is enabled. - bool should_force_close_stream_ : 1; // Set to indicate that stream should be fully closed due - // to either local reply or error response from the server. + bool should_stop_decoding_ : 1; // Set to indicate that decoding on the stream should be stopped + // due to either local reply or error response from the server. // The following 3 members are booleans rather than part of the space-saving bitfield as they // are passed as arguments to functions expecting bools. Extend State using the bitfield diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index ab1e2e1f993d..a4c50dbe82eb 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -71,7 +71,7 @@ class UpstreamFilterManager : public Http::FilterManager { state().remote_encode_complete_ = true; state().encoder_end_stream_ = true; state().local_complete_ = true; - state().should_force_close_stream_ = true; + state().should_stop_decoding_ = true; // TODO(alyssawilk) this should be done through the router to play well with hedging. upstream_request_.parent_.callbacks()->sendLocalReply(code, body, modify_headers, grpc_status, details); From b7b746f6aa79b91694ac10abee31aee052f01627 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 26 Jul 2024 23:41:38 +0000 Subject: [PATCH 07/27] Fixing tests Signed-off-by: Yan Avlasov --- test/common/router/router_test.cc | 10 ++++++++++ test/common/router/router_test_base.cc | 9 +++++++++ test/common/router/router_test_base.h | 2 ++ 3 files changed, 21 insertions(+) diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 80fee1ccd58f..0fd77f0749af 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -3422,6 +3422,11 @@ TEST_F(RouterTest, RetryHttp3UpstreamReset) { } TEST_F(RouterTest, NoRetryWithBodyLimit) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "false"}}); + + recreateFilter(); NiceMock encoder1; Http::ResponseDecoder* response_decoder = nullptr; expectNewStreamWithImmediateEncoder(encoder1, &response_decoder, Http::Protocol::Http10); @@ -3453,6 +3458,7 @@ TEST_F(RouterTest, NoRetryWithBodyLimitWithUpstreamHalfCloseEnabled) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues( {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"}}); + recreateFilter(); NiceMock encoder1; Http::ResponseDecoder* response_decoder = nullptr; expectNewStreamWithImmediateEncoder(encoder1, &response_decoder, Http::Protocol::Http10); @@ -4311,6 +4317,10 @@ TEST_F(RouterTest, InternalRedirectRejectedWithoutCompleteRequest) { EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ .counter("upstream_internal_redirect_failed_total") .value()); + // With upstream half close enabled router filter no longer resets the stream + // when upstream half closing first. As such it needs to be destroyed explicitly + // to avoid clean up problems in mocks + router_->onDestroy(); } TEST_F(RouterTest, InternalRedirectRejectedWithoutLocation) { diff --git a/test/common/router/router_test_base.cc b/test/common/router/router_test_base.cc index 4721d7a01e6d..ec756e36b00c 100644 --- a/test/common/router/router_test_base.cc +++ b/test/common/router/router_test_base.cc @@ -407,5 +407,14 @@ void RouterTestBase::expectNewStreamWithImmediateEncoder(Http::RequestEncoder& e })); } +void RouterTestBase::recreateFilter() { + router_ = std::make_unique(config_, config_->default_stats_); + router_->setDecoderFilterCallbacks(callbacks_); + router_->downstream_connection_.stream_info_.downstream_connection_info_provider_ + ->setLocalAddress(host_address_); + router_->downstream_connection_.stream_info_.downstream_connection_info_provider_ + ->setRemoteAddress(Network::Utility::parseInternetAddressAndPortNoThrow("1.2.3.4:80")); +} + } // namespace Router } // namespace Envoy diff --git a/test/common/router/router_test_base.h b/test/common/router/router_test_base.h index 494fb8cd881f..7f01608ce5fa 100644 --- a/test/common/router/router_test_base.h +++ b/test/common/router/router_test_base.h @@ -90,6 +90,8 @@ class RouterTestBase : public testing::Test { void expectNewStreamWithImmediateEncoder(Http::RequestEncoder& encoder, Http::ResponseDecoder** decoder, Http::Protocol protocol); + // Recreates filter under test after any values that affect its constructor were changed. + void recreateFilter(); Event::SimulatedTimeSystem test_time_; std::string upstream_zone_{"to_az"}; From af60f0bbaeff9bc95e0d753d709924f2071c646d Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Tue, 30 Jul 2024 00:54:45 +0000 Subject: [PATCH 08/27] Address post merge test failures Signed-off-by: Yan Avlasov --- envoy/router/router.h | 2 +- source/common/http/http1/codec_impl.cc | 5 +++-- source/common/http/http1/codec_impl.h | 3 ++- source/common/router/upstream_request.cc | 3 --- source/common/tcp_proxy/upstream.h | 15 ++++++++++++++- .../upstreams/http/http/upstream_request.h | 2 +- .../upstreams/http/tcp/upstream_request.h | 2 +- .../upstreams/http/udp/upstream_request.h | 2 +- .../upstreams/http/udp/upstream_request_test.cc | 2 +- .../multiplexed_upstream_integration_test.cc | 2 ++ test/integration/protocol_integration_test.cc | 1 + 11 files changed, 27 insertions(+), 12 deletions(-) diff --git a/envoy/router/router.h b/envoy/router/router.h index 596fb9ca728d..96031b928662 100644 --- a/envoy/router/router.h +++ b/envoy/router/router.h @@ -1547,7 +1547,7 @@ class GenericUpstream { * will not fully close the connection. This is off by default. * @param enabled Whether to set half-close semantics as enabled or disabled. */ - virtual void enableHalfClose() PURE; + virtual void enableTcpTunneling() PURE; /** * Enable/disable further data from this stream. */ diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 24e3b50b9699..5e054058a996 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -1589,13 +1589,14 @@ CallbackResult ClientConnectionImpl::onMessageCompleteBase() { response.decoder_->decodeData(buffer, true); } - if (force_reset_on_premature_upstream_half_close_) { + if (force_reset_on_premature_upstream_half_close_ && !encode_complete_) { // H/1 connections are always reset if upstream is done before downstream. // When the allow_multiplexed_upstream_half_close is enabled the router filter does not // reset streams where upstream half closed before downstream. In this case the H/1 codec // has to reset the stream. ENVOY_CONN_LOG(trace, "Resetting stream due to premature H/1 upstream close.", connection_); - response.encoder_.runResetCallbacks(StreamResetReason::Http1PrematureUpstreamHalfClose); + response.encoder_.runResetCallbacks(StreamResetReason::Http1PrematureUpstreamHalfClose, + absl::string_view()); } // Reset to ensure no information from one requests persists to the next. diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 0eebf9225349..26e3bfe82d9f 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -618,7 +618,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { Status onStatusBase(const char* data, size_t length) override; // ConnectionImpl Http::Status dispatch(Buffer::Instance& data) override; - void onEncodeComplete() override {} + void onEncodeComplete() override { encode_complete_ = true; } StreamInfo::BytesMeter& getBytesMeter() override { if (pending_response_.has_value()) { return *(pending_response_->encoder_.getStream().bytesMeter()); @@ -688,6 +688,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { bool passing_through_proxy_ = false; const bool force_reset_on_premature_upstream_half_close_{}; + bool encode_complete_{false}; }; } // namespace Http1 diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index a4c50dbe82eb..33ef0d74a59f 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -600,9 +600,6 @@ void UpstreamRequest::onPoolReady(std::unique_ptr&& upstream, had_upstream_ = true; // Have the upstream use the account of the downstream. upstream_->setAccount(parent_.callbacks()->account()); - if (enable_half_close_) { - upstream_->enableHalfClose(); - } host->outlierDetector().putResult(Upstream::Outlier::Result::LocalOriginConnectSuccess); diff --git a/source/common/tcp_proxy/upstream.h b/source/common/tcp_proxy/upstream.h index 1f2135b40532..5c136b033480 100644 --- a/source/common/tcp_proxy/upstream.h +++ b/source/common/tcp_proxy/upstream.h @@ -59,6 +59,20 @@ class TcpConnPool : public GenericConnPool, public Tcp::ConnectionPool::Callback class HttpUpstream; class CombinedUpstream; +class RouterUpstreamRequest : public Router::UpstreamRequest { +public: + using Router::UpstreamRequest::UpstreamRequest; + + void onPoolReady(std::unique_ptr&& upstream, + Upstream::HostDescriptionConstSharedPtr host, + const Network::ConnectionInfoProvider& address_provider, + StreamInfo::StreamInfo& info, absl::optional protocol) override { + upstream->enableTcpTunneling(); + Router::UpstreamRequest::onPoolReady(std::move(upstream), host, address_provider, info, + protocol); + } +}; + class HttpConnPool : public GenericConnPool, public Http::ConnectionPool::Callbacks { public: HttpConnPool(Upstream::ThreadLocalCluster& thread_local_cluster, @@ -67,7 +81,6 @@ class HttpConnPool : public GenericConnPool, public Http::ConnectionPool::Callba Http::StreamDecoderFilterCallbacks&, Http::CodecType type, StreamInfo::StreamInfo& downstream_info); - using RouterUpstreamRequest = Router::UpstreamRequest; using RouterUpstreamRequestPtr = std::unique_ptr; ~HttpConnPool() override; diff --git a/source/extensions/upstreams/http/http/upstream_request.h b/source/extensions/upstreams/http/http/upstream_request.h index db8adfc2cc14..f8c714160774 100644 --- a/source/extensions/upstreams/http/http/upstream_request.h +++ b/source/extensions/upstreams/http/http/upstream_request.h @@ -73,7 +73,7 @@ class HttpUpstream : public Router::GenericUpstream, public Envoy::Http::StreamC void encodeTrailers(const Envoy::Http::RequestTrailerMap& trailers) override { request_encoder_->encodeTrailers(trailers); } - void enableHalfClose() override { request_encoder_->enableTcpTunneling(); } + void enableTcpTunneling() override { request_encoder_->enableTcpTunneling(); } void readDisable(bool disable) override { request_encoder_->getStream().readDisable(disable); } diff --git a/source/extensions/upstreams/http/tcp/upstream_request.h b/source/extensions/upstreams/http/tcp/upstream_request.h index c85f707e5ba6..2237ddbb4b0c 100644 --- a/source/extensions/upstreams/http/tcp/upstream_request.h +++ b/source/extensions/upstreams/http/tcp/upstream_request.h @@ -78,7 +78,7 @@ class TcpUpstream : public Router::GenericUpstream, void encodeMetadata(const Envoy::Http::MetadataMapVector&) override {} Envoy::Http::Status encodeHeaders(const Envoy::Http::RequestHeaderMap&, bool end_stream) override; void encodeTrailers(const Envoy::Http::RequestTrailerMap&) override; - void enableHalfClose() override {} + void enableTcpTunneling() override {} void readDisable(bool disable) override; void resetStream() override; void setAccount(Buffer::BufferMemoryAccountSharedPtr) override {} diff --git a/source/extensions/upstreams/http/udp/upstream_request.h b/source/extensions/upstreams/http/udp/upstream_request.h index 5a17695d269c..34fab9e45da2 100644 --- a/source/extensions/upstreams/http/udp/upstream_request.h +++ b/source/extensions/upstreams/http/udp/upstream_request.h @@ -77,7 +77,7 @@ class UdpUpstream : public Router::GenericUpstream, void encodeTrailers(const Envoy::Http::RequestTrailerMap&) override {} void readDisable(bool) override {} void resetStream() override; - void enableHalfClose() override {} + void enableTcpTunneling() override {} void setAccount(Buffer::BufferMemoryAccountSharedPtr) override {} const StreamInfo::BytesMeterSharedPtr& bytesMeter() override { return bytes_meter_; } diff --git a/test/extensions/upstreams/http/udp/upstream_request_test.cc b/test/extensions/upstreams/http/udp/upstream_request_test.cc index a3fa14becf97..beb3be1d2e3d 100644 --- a/test/extensions/upstreams/http/udp/upstream_request_test.cc +++ b/test/extensions/upstreams/http/udp/upstream_request_test.cc @@ -46,7 +46,7 @@ class UdpUpstreamTest : public ::testing::Test { udp_upstream_ = std::make_unique(&mock_upstream_to_downstream_, std::move(mock_socket), std::move(mock_host), mock_dispatcher_); - EXPECT_NO_THROW(udp_upstream_->enableHalfClose()); + EXPECT_NO_THROW(udp_upstream_->enableTcpTunneling()); } protected: diff --git a/test/integration/multiplexed_upstream_integration_test.cc b/test/integration/multiplexed_upstream_integration_test.cc index d6cf22728fb8..e2270be533a8 100644 --- a/test/integration/multiplexed_upstream_integration_test.cc +++ b/test/integration/multiplexed_upstream_integration_test.cc @@ -75,6 +75,8 @@ TEST_P(MultiplexedUpstreamIntegrationTest, RouterDownstreamDisconnectBeforeRespo } TEST_P(MultiplexedUpstreamIntegrationTest, RouterUpstreamResponseBeforeRequestComplete) { + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "false"); testRouterUpstreamResponseBeforeRequestComplete(); } diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 55a521cd50a9..a4d56490b0c6 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -4788,6 +4788,7 @@ TEST_P(ProtocolIntegrationTest, ServerHalfCloseBeforeClientWithBufferedResponseD // reset. request_encoder_->getStream().readDisable(false); ASSERT_TRUE(response->waitForEndStream()); + codec_client_->close(); } } else if (fake_upstreams_[0]->httpType() == Http::CodecType::HTTP2 || fake_upstreams_[0]->httpType() == Http::CodecType::HTTP3) { From 1f269a49c7544b15869cbea2cbde9d021b71a9c4 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Tue, 30 Jul 2024 14:41:11 +0000 Subject: [PATCH 09/27] Post merge fix Signed-off-by: Yan Avlasov --- source/common/quic/envoy_quic_utils.cc | 2 ++ test/common/router/router_test.cc | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/quic/envoy_quic_utils.cc b/source/common/quic/envoy_quic_utils.cc index 137d5f2b80c2..3b4baaf43fb9 100644 --- a/source/common/quic/envoy_quic_utils.cc +++ b/source/common/quic/envoy_quic_utils.cc @@ -90,6 +90,8 @@ quic::QuicRstStreamErrorCode envoyResetReasonToQuicRstError(Http::StreamResetRea case Http::StreamResetReason::RemoteRefusedStreamReset: case Http::StreamResetReason::RemoteReset: IS_ENVOY_BUG("Remote reset shouldn't be initiated by self."); + case Http::StreamResetReason::Http1PrematureUpstreamHalfClose: + IS_ENVOY_BUG("H/1 premature response reset is not applicable to H/3."); } ENVOY_LOG_MISC(error, absl::StrCat("Unknown reset reason: ", reason)); diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 81fd96c85f8a..3bfbf74d3244 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -3476,7 +3476,7 @@ TEST_F(RouterTest, NoRetryWithBodyLimitWithUpstreamHalfCloseEnabled) { Buffer::OwnedImpl body("t"); router_->decodeData(body, false); EXPECT_EQ(1U, - callbacks_.route_->route_entry_.virtual_cluster_.stats().upstream_rq_total_.value()); + callbacks_.route_->virtual_host_.virtual_cluster_.stats().upstream_rq_total_.value()); Http::ResponseHeaderMapPtr response_headers( new Http::TestResponseHeaderMapImpl{{":status", "200"}}); From 1b961df06e021c5abc6c1f03e479dc5249a3903a Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Tue, 30 Jul 2024 19:08:39 +0000 Subject: [PATCH 10/27] Fix gcc build error Signed-off-by: Yan Avlasov --- source/common/quic/envoy_quic_utils.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source/common/quic/envoy_quic_utils.cc b/source/common/quic/envoy_quic_utils.cc index 3b4baaf43fb9..530a5c87451c 100644 --- a/source/common/quic/envoy_quic_utils.cc +++ b/source/common/quic/envoy_quic_utils.cc @@ -90,8 +90,10 @@ quic::QuicRstStreamErrorCode envoyResetReasonToQuicRstError(Http::StreamResetRea case Http::StreamResetReason::RemoteRefusedStreamReset: case Http::StreamResetReason::RemoteReset: IS_ENVOY_BUG("Remote reset shouldn't be initiated by self."); + break; case Http::StreamResetReason::Http1PrematureUpstreamHalfClose: IS_ENVOY_BUG("H/1 premature response reset is not applicable to H/3."); + break; } ENVOY_LOG_MISC(error, absl::StrCat("Unknown reset reason: ", reason)); From 8ae88a089b17ea6a6e20a2d48928d4da7cc2b8db Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Tue, 30 Jul 2024 20:44:25 +0000 Subject: [PATCH 11/27] WiP Signed-off-by: Yan Avlasov --- docs/root/configuration/observability/access_log/usage.rst | 6 ++++++ source/common/http/filter_manager.cc | 3 +-- source/common/http/filter_manager.h | 1 - source/common/runtime/runtime_features.cc | 2 +- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/root/configuration/observability/access_log/usage.rst b/docs/root/configuration/observability/access_log/usage.rst index 6b9d0fadfcab..5649cef99e20 100644 --- a/docs/root/configuration/observability/access_log/usage.rst +++ b/docs/root/configuration/observability/access_log/usage.rst @@ -418,6 +418,12 @@ The following command operators are supported: * ``us``: Microsecond precision. * ``ns``: Nanosecond precision. + NOTE: enabling independent half-close behavior for H/2 and H/3 protocols can produce + ``*_TX_END`` values lower than ``*_RX_END`` values, in cases where upstream peer has half-closed + its stream before downstream peer. In these cases ``COMMON_DURATION`` value will become negative. + Independent half-close behavior is enabled by setting the + ``envoy.reloadable_features.allow_multiplexed_upstream_half_close`` runtime value to ``true``. + TCP/UDP Not implemented ("-"). diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index fb25eeb9ad06..1e66cec4814d 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -1466,7 +1466,6 @@ void FilterManager::maybeEndEncode(bool end_stream) { if (filter_manager_callbacks_.isHalfCloseEnabled()) { checkAndCloseStreamIfFullyClosed(); } else { - state_.stream_closed_ = true; filter_manager_callbacks_.endStream(); } } @@ -1486,7 +1485,7 @@ void FilterManager::checkAndCloseStreamIfFullyClosed() { // is set (i.e local reply) or when both server and client half closed. if (state_.remote_encode_complete_ && (state_.remote_decode_complete_ || state_.should_stop_decoding_)) { - state_.stream_closed_ = true; + //state_.stream_closed_ = true; ENVOY_STREAM_LOG(trace, "closing stream", *this); filter_manager_callbacks_.endStream(); } diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index edd9f6e8814b..768bf60b90d9 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -769,7 +769,6 @@ class FilterManager : public ScopeTrackedObject, * @param trailers the trailers to decode. */ void decodeTrailers(RequestTrailerMap& trailers) { - state_.remote_decode_complete_ = true; decodeTrailers(nullptr, trailers); checkAndCloseStreamIfFullyClosed(); } diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 1206558f9520..e38f52c5e1c3 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -160,7 +160,7 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_google_grpc_disable_tls_13); // TODO(yanavlasov): Flip to true after prod testing. // Controls whether a stream stays open when HTTP/2 or HTTP/3 upstream half closes // before downstream. -FALSE_RUNTIME_GUARD(envoy_reloadable_features_allow_multiplexed_upstream_half_close); +RUNTIME_GUARD(envoy_reloadable_features_allow_multiplexed_upstream_half_close); // Block of non-boolean flags. Use of int flags is deprecated. Do not add more. ABSL_FLAG(uint64_t, re2_max_program_size_error_level, 100, ""); // NOLINT From 2166fcbbefcab655d8fd959f83d5da628e7900ff Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 2 Aug 2024 19:45:13 +0000 Subject: [PATCH 12/27] Post merge fixes Signed-off-by: Yan Avlasov --- source/common/http/filter_manager.cc | 18 +++----------- source/common/http/filter_manager.h | 30 +++++++++++------------ source/common/router/upstream_request.cc | 4 --- source/common/runtime/runtime_features.cc | 2 +- 4 files changed, 19 insertions(+), 35 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 0bc2955b9fab..efa3cac5502f 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -856,10 +856,8 @@ FilterManager::commonEncodePrefix(ActiveStreamEncoderFilter* filter, bool end_st FilterIterationStartState filter_iteration_start_state) { // Only do base state setting on the initial call. Subsequent calls for filtering do not touch // the base state. - ENVOY_STREAM_LOG(trace, - "commonEncodePrefix end_stream: {}, isHalfCloseEnabled: {}, force_close: {}", - *this, end_stream, filter_manager_callbacks_.isHalfCloseEnabled(), - static_cast(state_.should_stop_decoding_)); + ENVOY_STREAM_LOG(trace, "commonEncodePrefix end_stream: {}, isHalfCloseEnabled: {}", *this, + end_stream, filter_manager_callbacks_.isHalfCloseEnabled()); if (filter == nullptr) { if (end_stream) { ASSERT(!state_.observed_encode_end_stream_); @@ -921,8 +919,6 @@ void DownstreamFilterManager::sendLocalReply( ASSERT(!state_.under_on_local_reply_); const bool is_head_request = state_.is_head_request_; const bool is_grpc_request = state_.is_grpc_request_; - // Local reply stops decoding of downstream request. - stopDecoding(); // Stop filter chain iteration if local reply was sent while filter decoding or encoding callbacks // are running. @@ -1252,7 +1248,7 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea if (!(Http::CodeUtility::is2xx(response_status) || Http::CodeUtility::is1xx(response_status))) { // When the upstream half close is enabled the stream decoding is stopped on error responses // from the server. - stopDecoding(); + // stopDecoding(); } } filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream); @@ -1474,15 +1470,9 @@ void FilterManager::checkAndCloseStreamIfFullyClosed() { return; } - if (state_.stream_closed_) { - return; - } - // If upstream half close is enabled then close the stream either when force close // is set (i.e local reply) or when both server and client half closed. - if (state_.remote_encode_complete_ && - (state_.remote_decode_complete_ || state_.should_stop_decoding_)) { - //state_.stream_closed_ = true; + if (state_.encoder_filter_chain_complete_ && state_.decoder_filter_chain_complete_) { ENVOY_STREAM_LOG(trace, "closing stream", *this); filter_manager_callbacks_.endStream(); } diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 27c8e54bf785..cd4804703fc8 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -657,9 +657,10 @@ class FilterManager : public ScopeTrackedObject, void dumpState(std::ostream& os, int indent_level = 0) const override { const char* spaces = spacesForLevel(indent_level); os << spaces << "FilterManager " << this << DUMP_MEMBER(state_.has_1xx_headers_) - << DUMP_MEMBER(state_.remote_decode_complete_) << DUMP_MEMBER(state_.remote_encode_complete_) - << DUMP_MEMBER(state_.encoder_end_stream_) << DUMP_MEMBER(state_.stream_closed_) - << DUMP_MEMBER(state_.should_stop_decoding_) << "\n"; + << DUMP_MEMBER(state_.decoder_filter_chain_complete_) + << DUMP_MEMBER(state_.encoder_filter_chain_complete_) + << DUMP_MEMBER(state_.observed_decode_end_stream_) + << DUMP_MEMBER(state_.observed_encode_end_stream_) << "\n"; DUMP_DETAILS(filter_manager_callbacks_.requestHeaders()); DUMP_DETAILS(filter_manager_callbacks_.requestTrailers()); @@ -873,19 +874,21 @@ class FilterManager : public ScopeTrackedObject, virtual bool shouldLoadShed() { return false; }; - void stopDecoding() { state_.should_stop_decoding_ = true; } - protected: struct State { State() - : encoder_filter_chain_complete_(false), observed_decode_end_stream_(false), - observed_encode_end_stream_(false), has_1xx_headers_(false), created_filter_chain_(false), - is_head_request_(false), is_grpc_request_(false), - non_100_response_headers_encoded_(false), under_on_local_reply_(false), - decoder_filter_chain_aborted_(false), encoder_filter_chain_aborted_(false), - saw_downstream_reset_(false) {} + : decoder_filter_chain_complete_(false), encoder_filter_chain_complete_(false), + observed_decode_end_stream_(false), observed_encode_end_stream_(false), + has_1xx_headers_(false), created_filter_chain_(false), is_head_request_(false), + is_grpc_request_(false), non_100_response_headers_encoded_(false), + under_on_local_reply_(false), decoder_filter_chain_aborted_(false), + encoder_filter_chain_aborted_(false), saw_downstream_reset_(false) {} uint32_t filter_call_state_{0}; + // Set after decoder filter chain has completed iteration. Prevents further calls to decoder + // filters. + bool decoder_filter_chain_complete_ : 1; + // Set after encoder filter chain has completed iteration. Prevents further calls to encoder // filters. bool encoder_filter_chain_complete_ : 1; @@ -915,11 +918,6 @@ class FilterManager : public ScopeTrackedObject, bool decoder_filter_chain_aborted_ : 1; bool encoder_filter_chain_aborted_ : 1; bool saw_downstream_reset_ : 1; - bool stream_closed_ : 1; // Set when both remote_decode_complete_ and remote_encode_complete_ is - // true observed for the first time and prevents ending the stream - // multiple times. Only set when allow_upstream_half_close is enabled. - bool should_stop_decoding_ : 1; // Set to indicate that decoding on the stream should be stopped - // due to either local reply or error response from the server. // The following 3 members are booleans rather than part of the space-saving bitfield as they // are passed as arguments to functions expecting bools. Extend State using the bitfield diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index 03d1fe6f460c..b14ad939a6be 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -68,10 +68,6 @@ class UpstreamFilterManager : public Http::FilterManager { absl::string_view details) override { state().decoder_filter_chain_aborted_ = true; state().encoder_filter_chain_aborted_ = true; - state().remote_encode_complete_ = true; - state().encoder_end_stream_ = true; - state().local_complete_ = true; - state().should_stop_decoding_ = true; state().encoder_filter_chain_complete_ = true; state().observed_encode_end_stream_ = true; // TODO(alyssawilk) this should be done through the router to play well with hedging. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index cf763a05ae4e..007b8ff36ebd 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -157,7 +157,7 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_google_grpc_disable_tls_13); // TODO(yanavlasov): Flip to true after prod testing. // Controls whether a stream stays open when HTTP/2 or HTTP/3 upstream half closes // before downstream. -RUNTIME_GUARD(envoy_reloadable_features_allow_multiplexed_upstream_half_close); +FALSE_RUNTIME_GUARD(envoy_reloadable_features_allow_multiplexed_upstream_half_close); // Block of non-boolean flags. Use of int flags is deprecated. Do not add more. ABSL_FLAG(uint64_t, re2_max_program_size_error_level, 100, ""); // NOLINT From e2deca2863889fbf5b2ef31f6f4d727f6e3e3d0f Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Thu, 8 Aug 2024 01:14:14 +0000 Subject: [PATCH 13/27] WiP Signed-off-by: Yan Avlasov --- source/common/http/filter_manager.cc | 61 +++++++++++++++----- source/common/http/filter_manager.h | 9 ++- test/common/http/conn_manager_impl_test_2.cc | 40 +++++++++++++ 3 files changed, 94 insertions(+), 16 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index efa3cac5502f..55c4363e91c7 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -527,6 +527,8 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead commonDecodePrefix(filter, FilterIterationStartState::AlwaysStartFromNext); std::list::iterator continue_data_entry = decoder_filters_.end(); + bool last_filter_saw_end_stream = false; + for (; entry != decoder_filters_.end(); entry++) { ASSERT(!(state_.filter_call_state_ & FilterCallState::DecodeHeaders)); state_.filter_call_state_ |= FilterCallState::DecodeHeaders; @@ -534,6 +536,8 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead if ((*entry)->end_stream_) { state_.filter_call_state_ |= FilterCallState::EndOfStream; } + last_filter_saw_end_stream = + (*entry)->end_stream_ && std::next(entry) == decoder_filters_.end(); FilterHeadersStatus status = (*entry)->decodeHeaders(headers, (*entry)->end_stream_); state_.filter_call_state_ &= ~FilterCallState::DecodeHeaders; if ((*entry)->end_stream_) { @@ -606,6 +610,7 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead if (end_stream) { disarmRequestTimeout(); } + maybeEndDecode(last_filter_saw_end_stream); } void FilterManager::decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instance& data, @@ -625,6 +630,7 @@ void FilterManager::decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instan // Filter iteration may start at the current filter. std::list::iterator entry = commonDecodePrefix(filter, filter_iteration_start_state); + bool last_filter_saw_end_stream = false; for (; entry != decoder_filters_.end(); entry++) { // If the filter pointed by entry has stopped for all frame types, return now. @@ -677,6 +683,8 @@ void FilterManager::decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instan state_.filter_call_state_ |= FilterCallState::DecodeData; (*entry)->end_stream_ = end_stream && !filter_manager_callbacks_.requestTrailers(); + last_filter_saw_end_stream = + (*entry)->end_stream_ && std::next(entry) == decoder_filters_.end(); FilterDataStatus status = (*entry)->handle_->decodeData(data, (*entry)->end_stream_); if ((*entry)->end_stream_) { (*entry)->handle_->decodeComplete(); @@ -720,6 +728,7 @@ void FilterManager::decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instan if (end_stream) { disarmRequestTimeout(); } + maybeEndDecode(last_filter_saw_end_stream); } RequestTrailerMap& FilterManager::addDecodedTrailers() { @@ -764,6 +773,7 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra // Filter iteration may start at the current filter. std::list::iterator entry = commonDecodePrefix(filter, FilterIterationStartState::CanStartFromCurrent); + bool last_filter_saw_end_stream = false; for (; entry != decoder_filters_.end(); entry++) { // If the filter pointed by entry has stopped for all frame type, return now. @@ -775,6 +785,8 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra FilterTrailersStatus status = (*entry)->handle_->decodeTrailers(trailers); (*entry)->handle_->decodeComplete(); (*entry)->end_stream_ = true; + last_filter_saw_end_stream = + (*entry)->end_stream_ && std::next(entry) == decoder_filters_.end(); state_.filter_call_state_ &= ~FilterCallState::DecodeTrailers; ENVOY_STREAM_LOG(trace, "decode trailers called: filter={} status={}", *this, (*entry)->filter_context_.config_name, static_cast(status)); @@ -788,11 +800,13 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra processNewlyAddedMetadata(); - if (!(*entry)->commonHandleAfterTrailersCallback(status)) { + if (!(*entry)->commonHandleAfterTrailersCallback(status) && + std::next(entry) != decoder_filters_.end()) { return; } } disarmRequestTimeout(); + maybeEndDecode(last_filter_saw_end_stream); } void FilterManager::decodeMetadata(ActiveStreamDecoderFilter* filter, MetadataMap& metadata_map) { @@ -922,9 +936,9 @@ void DownstreamFilterManager::sendLocalReply( // Stop filter chain iteration if local reply was sent while filter decoding or encoding callbacks // are running. - if (state_.filter_call_state_ & FilterCallState::IsDecodingMask) { - state_.decoder_filter_chain_aborted_ = true; - } else if (state_.filter_call_state_ & FilterCallState::IsEncodingMask) { + /*if (state_.filter_call_state_ & FilterCallState::IsDecodingMask) {*/ + state_.decoder_filter_chain_aborted_ = true; + /*} else*/ if (state_.filter_call_state_ & FilterCallState::IsEncodingMask) { state_.encoder_filter_chain_aborted_ = true; } @@ -1243,14 +1257,6 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea const bool modified_end_stream = (end_stream && continue_data_entry == encoder_filters_.end()); state_.non_100_response_headers_encoded_ = true; - if (filter_manager_callbacks_.isHalfCloseEnabled()) { - const uint64_t response_status = Http::Utility::getResponseStatus(headers); - if (!(Http::CodeUtility::is2xx(response_status) || Http::CodeUtility::is1xx(response_status))) { - // When the upstream half close is enabled the stream decoding is stopped on error responses - // from the server. - // stopDecoding(); - } - } filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream); if (state_.saw_downstream_reset_) { return; @@ -1464,16 +1470,45 @@ void FilterManager::maybeEndEncode(bool end_stream) { } } +void FilterManager::maybeEndDecode(bool end_stream) { + if (end_stream) { + ASSERT(!state_.decoder_filter_chain_complete_); + state_.decoder_filter_chain_complete_ = true; + if (filter_manager_callbacks_.isHalfCloseEnabled() && !stopDecoderFilterChain()) { + checkAndCloseStreamIfFullyClosed(); + } + } +} + void FilterManager::checkAndCloseStreamIfFullyClosed() { // This function is only used when half close semantics are enabled. if (!filter_manager_callbacks_.isHalfCloseEnabled()) { return; } + std::cout << typeid(*this).name() << "::checkAndCloseStreamIfFullyClosed() " + << state_.encoder_filter_chain_complete_ << " " << state_.decoder_filter_chain_complete_ + << " " << state_.decoder_filter_chain_aborted_ << "\n"; + + // When the upstream half close is enabled the stream decoding is stopped on error responses + // from the server. + bool error_response = false; + if (filter_manager_callbacks_.responseHeaders().has_value()) { + const uint64_t response_status = + Http::Utility::getResponseStatus(filter_manager_callbacks_.responseHeaders().ref()); + error_response = + !(Http::CodeUtility::is2xx(response_status) || Http::CodeUtility::is1xx(response_status)); + } + // If upstream half close is enabled then close the stream either when force close // is set (i.e local reply) or when both server and client half closed. - if (state_.encoder_filter_chain_complete_ && state_.decoder_filter_chain_complete_) { + if (state_.encoder_filter_chain_complete_ && + (state_.decoder_filter_chain_complete_ || error_response || + state_.decoder_filter_chain_aborted_)) { ENVOY_STREAM_LOG(trace, "closing stream", *this); + if (error_response) { + state_.decoder_filter_chain_aborted_ = true; + } filter_manager_callbacks_.endStream(); } } diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index cd4804703fc8..84d6cc9bd551 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -751,7 +751,6 @@ class FilterManager : public ScopeTrackedObject, void decodeHeaders(RequestHeaderMap& headers, bool end_stream) { state_.observed_decode_end_stream_ = end_stream; decodeHeaders(nullptr, headers, end_stream); - checkAndCloseStreamIfFullyClosed(); } /** @@ -762,7 +761,6 @@ class FilterManager : public ScopeTrackedObject, void decodeData(Buffer::Instance& data, bool end_stream) { state_.observed_decode_end_stream_ = end_stream; decodeData(nullptr, data, end_stream, FilterIterationStartState::CanStartFromCurrent); - checkAndCloseStreamIfFullyClosed(); } /** @@ -772,7 +770,6 @@ class FilterManager : public ScopeTrackedObject, void decodeTrailers(RequestTrailerMap& trailers) { state_.observed_decode_end_stream_ = true; decodeTrailers(nullptr, trailers); - checkAndCloseStreamIfFullyClosed(); } /** @@ -789,6 +786,12 @@ class FilterManager : public ScopeTrackedObject, */ void maybeEndEncode(bool end_stream); + /** + * If end_stream is true, marks decoding as complete. This is a noop if end_stream is false. + * @param end_stream whether decoding is complete. + */ + void maybeEndDecode(bool end_stream); + void checkAndCloseStreamIfFullyClosed(); virtual void sendLocalReply(Code code, absl::string_view body, diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index 460e6c6d0524..9e9320a983b7 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -1813,6 +1813,46 @@ TEST_F(HttpConnectionManagerImplTest, FilterHeadReply) { conn_manager_->onData(fake_input, false); } +TEST_F(HttpConnectionManagerImplTest, LocalReplyStopsDecoding) { + InSequence s; + setup(false, ""); + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { + decoder_ = &conn_manager_->newStream(response_encoder_); + RequestHeaderMapPtr headers{ + new TestRequestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "POST"}}}; + // Start POST request (end_stream = false) + decoder_->decodeHeaders(std::move(headers), false); + data.drain(4); + return Http::okStatus(); + })); + + setupFilterChain(1, 1); + + EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, false)) + .WillOnce(InvokeWithoutArgs([&]() -> FilterHeadersStatus { + decoder_filters_[0]->callbacks_->sendLocalReply(Code::BadRequest, "Bad request", nullptr, + absl::nullopt, ""); + return FilterHeadersStatus::StopIteration; + })); + + EXPECT_CALL(response_encoder_, streamErrorOnInvalidHttpMessage()).WillOnce(Return(true)); + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); + EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + + EXPECT_CALL(*encoder_filters_[0], encodeComplete()); + EXPECT_CALL(response_encoder_, encodeData(_, true)).WillOnce(Invoke([&](Buffer::Instance&, bool) { + response_encoder_.stream_.codec_callbacks_->onCodecEncodeComplete(); + })); + expectOnDestroy(); + // Kick off the incoming data. + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); +} + // Verify that if an encoded stream has been ended, but gets stopped by a filter chain, we end // up resetting the stream in the doEndStream() path (e.g., via filter reset due to timeout, etc.), // we emit a reset to the codec. From 9e9428f8faa89f6376002c0f15338c72c546d060 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 9 Aug 2024 15:08:09 +0000 Subject: [PATCH 14/27] Make ending filters in the middle of the filter chain work. p1 Signed-off-by: Yan Avlasov --- source/common/http/filter_manager.cc | 9 ++ test/common/http/conn_manager_impl_test_2.cc | 106 +++++++++++++++++++ 2 files changed, 115 insertions(+) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 55c4363e91c7..52fcf0ceaf0f 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -470,15 +470,24 @@ void ActiveStreamDecoderFilter::encodeHeaders(ResponseHeaderMapPtr&& headers, bo absl::string_view details) { parent_.streamInfo().setResponseCodeDetails(details); parent_.filter_manager_callbacks_.setResponseHeaders(std::move(headers)); + if (end_stream && end_stream_) { + parent_.state_.decoder_filter_chain_aborted_ = true; + } parent_.encodeHeaders(nullptr, *parent_.filter_manager_callbacks_.responseHeaders(), end_stream); } void ActiveStreamDecoderFilter::encodeData(Buffer::Instance& data, bool end_stream) { + if (end_stream && end_stream_) { + parent_.state_.decoder_filter_chain_aborted_ = true; + } parent_.encodeData(nullptr, data, end_stream, FilterManager::FilterIterationStartState::CanStartFromCurrent); } void ActiveStreamDecoderFilter::encodeTrailers(ResponseTrailerMapPtr&& trailers) { + if (end_stream_) { + parent_.state_.decoder_filter_chain_aborted_ = true; + } parent_.filter_manager_callbacks_.setResponseTrailers(std::move(trailers)); parent_.encodeTrailers(nullptr, *parent_.filter_manager_callbacks_.responseTrailers()); } diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index 9e9320a983b7..6802518ffe30 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -4347,5 +4347,111 @@ TEST_F(HttpConnectionManagerImplTest, PassMatchUpstreamSchemeHintToStreamInfo) { filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); } +// Validate that incomplete request is terminated when a non terminal filter +// initates encoding of the response (i.e. the cache filter). +// This only works when independent half-close mode is DISABLED. +TEST_F(HttpConnectionManagerImplTest, EncodingByNonTerminalFilter) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "false"}}); + setup(false, ""); + constexpr int total_filters = 3; + constexpr int ecoder_filter_index = 1; + setupFilterChain(total_filters, total_filters); + + EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + + EXPECT_CALL(*decoder_filters_[1], decodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::StopIteration)); + + // Kick off the incomplete request (end_stream == false). + startRequest(false); + + // For encode direction + EXPECT_CALL(*encoder_filters_[2], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + + // Second decoder filter (there are 3 in total) initiates encoding + ResponseHeaderMapPtr response_headers{new TestResponseHeaderMapImpl{{":status", "200"}}}; + decoder_filters_[ecoder_filter_index]->callbacks_->encodeHeaders(std::move(response_headers), + false, "details"); + + EXPECT_CALL(*encoder_filters_[2], encodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*encoder_filters_[2], encodeComplete()); + EXPECT_CALL(*encoder_filters_[1], encodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*encoder_filters_[1], encodeComplete()); + EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeComplete()); + // verify that after the end_stream is observed by the last encoder filter the request is + // completed. + expectOnDestroy(); + + // Second decoder filter then completes encoding with data + Buffer::OwnedImpl fake_response("world"); + decoder_filters_[ecoder_filter_index]->callbacks_->encodeData(fake_response, true); +} + +// Validate that when independent half-close is enabled, encoding end_stream by a +// non-final filter ends the request iff the filter that intiated encoding of the end_stream has +// already observed the request end_stream. +TEST_F(HttpConnectionManagerImplTest, EncodingByNonTerminalFilterWithIndependentHalfClose) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"}}); + setup(false, ""); + constexpr int total_filters = 3; + constexpr int ecoder_filter_index = 1; + setupFilterChain(total_filters, total_filters); + + EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, true)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*decoder_filters_[0], decodeComplete()); + EXPECT_CALL(*decoder_filters_[1], decodeHeaders(_, true)) + .WillOnce(Return(FilterHeadersStatus::StopIteration)); + EXPECT_CALL(*decoder_filters_[1], decodeComplete()); + + // Send complete request. + startRequest(true); + + // For encode direction + EXPECT_CALL(*encoder_filters_[2], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + + // Second decoder filter (there are 3 in total) initiates encoding + ResponseHeaderMapPtr response_headers{new TestResponseHeaderMapImpl{{":status", "200"}}}; + decoder_filters_[ecoder_filter_index]->callbacks_->encodeHeaders(std::move(response_headers), + false, "details"); + + EXPECT_CALL(*encoder_filters_[2], encodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*encoder_filters_[2], encodeComplete()); + EXPECT_CALL(*encoder_filters_[1], encodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*encoder_filters_[1], encodeComplete()); + EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeComplete()); + // Verify that after the end_stream is observed by the last encoder filter the request is + // completed. even though the request end_stream never reached the terminal filter, but was + // observed by the filter that has initiated encoding. + expectOnDestroy(); + + // Second decoder filter then completes encoding with data + Buffer::OwnedImpl fake_response("world"); + decoder_filters_[ecoder_filter_index]->callbacks_->encodeData(fake_response, true); +} + } // namespace Http } // namespace Envoy From 71ce1f0120233ed6197a765864f0aab7435bc683 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 9 Aug 2024 19:46:39 +0000 Subject: [PATCH 15/27] Make encoding filters in the middle of the filter chain work. p2 Signed-off-by: Yan Avlasov --- source/common/http/filter_manager.cc | 21 ++- source/common/http/filter_manager.h | 1 + test/common/http/conn_manager_impl_test_2.cc | 148 +++++++++++++++++- .../http/conn_manager_impl_test_base.cc | 6 +- 4 files changed, 166 insertions(+), 10 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 52fcf0ceaf0f..a4d999f40a1f 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -468,6 +468,7 @@ void ActiveStreamDecoderFilter::encode1xxHeaders(ResponseHeaderMapPtr&& headers) void ActiveStreamDecoderFilter::encodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream, absl::string_view details) { + encoded_end_stream_ = end_stream; parent_.streamInfo().setResponseCodeDetails(details); parent_.filter_manager_callbacks_.setResponseHeaders(std::move(headers)); if (end_stream && end_stream_) { @@ -477,6 +478,7 @@ void ActiveStreamDecoderFilter::encodeHeaders(ResponseHeaderMapPtr&& headers, bo } void ActiveStreamDecoderFilter::encodeData(Buffer::Instance& data, bool end_stream) { + encoded_end_stream_ = end_stream; if (end_stream && end_stream_) { parent_.state_.decoder_filter_chain_aborted_ = true; } @@ -485,6 +487,7 @@ void ActiveStreamDecoderFilter::encodeData(Buffer::Instance& data, bool end_stre } void ActiveStreamDecoderFilter::encodeTrailers(ResponseTrailerMapPtr&& trailers) { + encoded_end_stream_ = true; if (end_stream_) { parent_.state_.decoder_filter_chain_aborted_ = true; } @@ -545,8 +548,6 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead if ((*entry)->end_stream_) { state_.filter_call_state_ |= FilterCallState::EndOfStream; } - last_filter_saw_end_stream = - (*entry)->end_stream_ && std::next(entry) == decoder_filters_.end(); FilterHeadersStatus status = (*entry)->decodeHeaders(headers, (*entry)->end_stream_); state_.filter_call_state_ &= ~FilterCallState::DecodeHeaders; if ((*entry)->end_stream_) { @@ -612,6 +613,9 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead if (end_stream && buffered_request_data_ && continue_data_entry == decoder_filters_.end()) { continue_data_entry = entry; } + last_filter_saw_end_stream = + (end_stream && continue_data_entry == decoder_filters_.end()) && + (std::next(entry) == decoder_filters_.end() || (*entry)->encoded_end_stream_); } maybeContinueDecoding(continue_data_entry); @@ -692,8 +696,6 @@ void FilterManager::decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instan state_.filter_call_state_ |= FilterCallState::DecodeData; (*entry)->end_stream_ = end_stream && !filter_manager_callbacks_.requestTrailers(); - last_filter_saw_end_stream = - (*entry)->end_stream_ && std::next(entry) == decoder_filters_.end(); FilterDataStatus status = (*entry)->handle_->decodeData(data, (*entry)->end_stream_); if ((*entry)->end_stream_) { (*entry)->handle_->decodeComplete(); @@ -719,6 +721,9 @@ void FilterManager::decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instan trailers_added_entry = entry; } + last_filter_saw_end_stream = + end_stream && (std::next(entry) == decoder_filters_.end() || (*entry)->encoded_end_stream_); + if (!(*entry)->commonHandleAfterDataCallback(status, data, state_.decoder_filters_streaming_) && std::next(entry) != decoder_filters_.end()) { // Stop iteration IFF this is not the last filter. If it is the last filter, continue with @@ -794,8 +799,6 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra FilterTrailersStatus status = (*entry)->handle_->decodeTrailers(trailers); (*entry)->handle_->decodeComplete(); (*entry)->end_stream_ = true; - last_filter_saw_end_stream = - (*entry)->end_stream_ && std::next(entry) == decoder_filters_.end(); state_.filter_call_state_ &= ~FilterCallState::DecodeTrailers; ENVOY_STREAM_LOG(trace, "decode trailers called: filter={} status={}", *this, (*entry)->filter_context_.config_name, static_cast(status)); @@ -808,6 +811,12 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra } processNewlyAddedMetadata(); + last_filter_saw_end_stream = + std::next(entry) == decoder_filters_.end() || (*entry)->encoded_end_stream_; + + if (last_filter_saw_end_stream) { + break; + } if (!(*entry)->commonHandleAfterTrailersCallback(status) && std::next(entry) != decoder_filters_.end()) { diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 84d6cc9bd551..44e23186c331 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -283,6 +283,7 @@ struct ActiveStreamDecoderFilter : public ActiveStreamFilterBase, StreamDecoderFilterSharedPtr handle_; bool is_grpc_request_{}; + bool encoded_end_stream_{false}; }; using ActiveStreamDecoderFilterPtr = std::unique_ptr; diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index 6802518ffe30..56897dd5b5f0 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -4348,7 +4348,7 @@ TEST_F(HttpConnectionManagerImplTest, PassMatchUpstreamSchemeHintToStreamInfo) { } // Validate that incomplete request is terminated when a non terminal filter -// initates encoding of the response (i.e. the cache filter). +// initiates encoding of the response (i.e. the cache filter). // This only works when independent half-close mode is DISABLED. TEST_F(HttpConnectionManagerImplTest, EncodingByNonTerminalFilter) { TestScopedRuntime scoped_runtime; @@ -4400,7 +4400,7 @@ TEST_F(HttpConnectionManagerImplTest, EncodingByNonTerminalFilter) { } // Validate that when independent half-close is enabled, encoding end_stream by a -// non-final filter ends the request iff the filter that intiated encoding of the end_stream has +// non-final filter ends the request iff the filter that initiated encoding of the end_stream has // already observed the request end_stream. TEST_F(HttpConnectionManagerImplTest, EncodingByNonTerminalFilterWithIndependentHalfClose) { TestScopedRuntime scoped_runtime; @@ -4453,5 +4453,149 @@ TEST_F(HttpConnectionManagerImplTest, EncodingByNonTerminalFilterWithIndependent decoder_filters_[ecoder_filter_index]->callbacks_->encodeData(fake_response, true); } +// Validate that when independent half-close is enabled, encoding end_stream by a +// non-final filter with incomplete request makes the encoding filter the terminal filter. +// In this case decoding end_stream from the client only reaches the filter that encoded the +// end_stream after which the request is completed. +TEST_F(HttpConnectionManagerImplTest, DecodingByNonTerminalEncoderFilterWithIndependentHalfClose) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"}}); + setup(false, ""); + constexpr int total_filters = 3; + constexpr int ecoder_filter_index = 1; + setupFilterChain(total_filters, total_filters); + + EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*decoder_filters_[1], decodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::StopIteration)); + + // Send incomplete request. + startRequest(false); + + // For encode direction + EXPECT_CALL(*encoder_filters_[2], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + + // Second decoder filter (there are 3 in total) initiates encoding + ResponseHeaderMapPtr response_headers{new TestResponseHeaderMapImpl{{":status", "200"}}}; + decoder_filters_[ecoder_filter_index]->callbacks_->encodeHeaders(std::move(response_headers), + false, "details"); + + EXPECT_CALL(*encoder_filters_[2], encodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*encoder_filters_[2], encodeComplete()); + EXPECT_CALL(*encoder_filters_[1], encodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*encoder_filters_[1], encodeComplete()); + EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeComplete()); + + // Second decoder filter then completes encoding with data + Buffer::OwnedImpl fake_response("world"); + decoder_filters_[ecoder_filter_index]->callbacks_->encodeData(fake_response, true); + + // Request is still be alive with the half-close enabled. + // Verify that once the end_stream from the client reaches the filter that encoded the end_stream + // the request will end. + EXPECT_CALL(*decoder_filters_[0], decodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*decoder_filters_[0], decodeComplete()); + EXPECT_CALL(*decoder_filters_[1], decodeData(_, true)) + .WillOnce(Return(FilterDataStatus::StopIterationNoBuffer)); + EXPECT_CALL(*decoder_filters_[1], decodeComplete()); + expectOnDestroy(); + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { + decoder_->decodeData(data, true); + data.drain(4); + return Http::okStatus(); + })); + + Buffer::OwnedImpl fake_input("5678"); + conn_manager_->onData(fake_input, false); +} + +// Validate that when independent half-close is enabled, encoding end_stream by a +// non-final filter with incomplete request makes the encoding filter the terminal filter. +// In this case decoding end_stream from the client only reaches the filter that encoded the +// end_stream after which the request is completed. +TEST_F(HttpConnectionManagerImplTest, DecodingWithAddedTrailersByNonTerminalEncoderFilter) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"}}); + setup(false, ""); + constexpr int total_filters = 3; + constexpr int ecoder_filter_index = 1; + setupFilterChain(total_filters, total_filters); + + EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*decoder_filters_[1], decodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::StopIteration)); + + // Send incomplete request. + startRequest(false); + + // For encode direction + EXPECT_CALL(*encoder_filters_[2], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + + // Second decoder filter (there are 3 in total) initiates encoding + ResponseHeaderMapPtr response_headers{new TestResponseHeaderMapImpl{{":status", "200"}}}; + decoder_filters_[ecoder_filter_index]->callbacks_->encodeHeaders(std::move(response_headers), + false, "details"); + + EXPECT_CALL(*encoder_filters_[2], encodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*encoder_filters_[2], encodeComplete()); + EXPECT_CALL(*encoder_filters_[1], encodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*encoder_filters_[1], encodeComplete()); + EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeComplete()); + + // Second decoder filter then completes encoding with data + Buffer::OwnedImpl fake_response("world"); + decoder_filters_[ecoder_filter_index]->callbacks_->encodeData(fake_response, true); + + // Request is still be alive with the half-close enabled. + // Verify that once the end_stream from the client reaches the filter that encoded the end_stream + // the request will end. + EXPECT_CALL(*decoder_filters_[0], decodeData(_, true)) + .WillOnce(InvokeWithoutArgs([&]() -> FilterDataStatus { + decoder_filters_[1]->callbacks_->addDecodedTrailers().addCopy(Http::LowerCaseString("foo"), + "bar"); + return FilterDataStatus::Continue; + })); + EXPECT_CALL(*decoder_filters_[0], decodeComplete()); + EXPECT_CALL(*decoder_filters_[1], decodeData(_, false)) + .WillOnce(Return(FilterDataStatus::StopIterationNoBuffer)); + EXPECT_CALL(*decoder_filters_[1], decodeTrailers(_)) + .WillOnce(Return(FilterTrailersStatus::StopIteration)); + EXPECT_CALL(*decoder_filters_[1], decodeComplete()); + expectOnDestroy(); + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { + decoder_->decodeData(data, true); + data.drain(4); + return Http::okStatus(); + })); + + Buffer::OwnedImpl fake_input("5678"); + conn_manager_->onData(fake_input, false); +} + } // namespace Http } // namespace Envoy diff --git a/test/common/http/conn_manager_impl_test_base.cc b/test/common/http/conn_manager_impl_test_base.cc index 4294d2992955..aee79017d3e1 100644 --- a/test/common/http/conn_manager_impl_test_base.cc +++ b/test/common/http/conn_manager_impl_test_base.cc @@ -257,14 +257,16 @@ void HttpConnectionManagerImplMixin::setupFilterChain(int num_decoder_filters, for (int i = 0; i < num_decoder_filters; i++) { auto factory = createDecoderFilterFactoryCb( StreamDecoderFilterSharedPtr{decoder_filters_[req * num_decoder_filters + i]}); - manager.applyFilterFactoryCb({}, factory); + std::string name = absl::StrCat(req * num_decoder_filters + i); + manager.applyFilterFactoryCb({name, name}, factory); applied_filters = true; } for (int i = 0; i < num_encoder_filters; i++) { auto factory = createEncoderFilterFactoryCb( StreamEncoderFilterSharedPtr{encoder_filters_[req * num_encoder_filters + i]}); - manager.applyFilterFactoryCb({}, factory); + std::string name = absl::StrCat(req * num_decoder_filters + i); + manager.applyFilterFactoryCb({name, name}, factory); applied_filters = true; } return applied_filters; From 68c807e8107486807c9c7d808dd2bef054d9e801 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Mon, 12 Aug 2024 14:48:44 +0000 Subject: [PATCH 16/27] Update comments Signed-off-by: Yan Avlasov --- source/common/http/filter_manager.cc | 92 ++++++++++++++-------------- source/common/http/filter_manager.h | 17 ++++- 2 files changed, 61 insertions(+), 48 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index a4d999f40a1f..f514ee075444 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -466,31 +466,29 @@ void ActiveStreamDecoderFilter::encode1xxHeaders(ResponseHeaderMapPtr&& headers) } } +void ActiveStreamDecoderFilter::maybeStopDecoderFilterChain(bool end_stream) { + filter_encoded_end_stream_ = end_stream; + if (end_stream && end_stream_ && !parent_.state_.decoder_filter_chain_complete_) { + parent_.state_.decoder_filter_chain_aborted_ = true; + } +} + void ActiveStreamDecoderFilter::encodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream, absl::string_view details) { - encoded_end_stream_ = end_stream; + maybeStopDecoderFilterChain(end_stream); parent_.streamInfo().setResponseCodeDetails(details); parent_.filter_manager_callbacks_.setResponseHeaders(std::move(headers)); - if (end_stream && end_stream_) { - parent_.state_.decoder_filter_chain_aborted_ = true; - } parent_.encodeHeaders(nullptr, *parent_.filter_manager_callbacks_.responseHeaders(), end_stream); } void ActiveStreamDecoderFilter::encodeData(Buffer::Instance& data, bool end_stream) { - encoded_end_stream_ = end_stream; - if (end_stream && end_stream_) { - parent_.state_.decoder_filter_chain_aborted_ = true; - } + maybeStopDecoderFilterChain(end_stream); parent_.encodeData(nullptr, data, end_stream, FilterManager::FilterIterationStartState::CanStartFromCurrent); } void ActiveStreamDecoderFilter::encodeTrailers(ResponseTrailerMapPtr&& trailers) { - encoded_end_stream_ = true; - if (end_stream_) { - parent_.state_.decoder_filter_chain_aborted_ = true; - } + maybeStopDecoderFilterChain(true); parent_.filter_manager_callbacks_.setResponseTrailers(std::move(trailers)); parent_.encodeTrailers(nullptr, *parent_.filter_manager_callbacks_.responseTrailers()); } @@ -538,8 +536,8 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead std::list::iterator entry = commonDecodePrefix(filter, FilterIterationStartState::AlwaysStartFromNext); std::list::iterator continue_data_entry = decoder_filters_.end(); - - bool last_filter_saw_end_stream = false; + // Terminal filter is either the last one or filter that encoded end_stream. + bool terminal_filter_decoded_end_stream = false; for (; entry != decoder_filters_.end(); entry++) { ASSERT(!(state_.filter_call_state_ & FilterCallState::DecodeHeaders)); @@ -613,9 +611,9 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead if (end_stream && buffered_request_data_ && continue_data_entry == decoder_filters_.end()) { continue_data_entry = entry; } - last_filter_saw_end_stream = + terminal_filter_decoded_end_stream = (end_stream && continue_data_entry == decoder_filters_.end()) && - (std::next(entry) == decoder_filters_.end() || (*entry)->encoded_end_stream_); + (std::next(entry) == decoder_filters_.end() || (*entry)->filter_encoded_end_stream_); } maybeContinueDecoding(continue_data_entry); @@ -623,7 +621,7 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead if (end_stream) { disarmRequestTimeout(); } - maybeEndDecode(last_filter_saw_end_stream); + maybeEndDecode(terminal_filter_decoded_end_stream); } void FilterManager::decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instance& data, @@ -643,7 +641,7 @@ void FilterManager::decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instan // Filter iteration may start at the current filter. std::list::iterator entry = commonDecodePrefix(filter, filter_iteration_start_state); - bool last_filter_saw_end_stream = false; + bool terminal_filter_decoded_end_stream = false; for (; entry != decoder_filters_.end(); entry++) { // If the filter pointed by entry has stopped for all frame types, return now. @@ -721,8 +719,9 @@ void FilterManager::decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instan trailers_added_entry = entry; } - last_filter_saw_end_stream = - end_stream && (std::next(entry) == decoder_filters_.end() || (*entry)->encoded_end_stream_); + terminal_filter_decoded_end_stream = + end_stream && + (std::next(entry) == decoder_filters_.end() || (*entry)->filter_encoded_end_stream_); if (!(*entry)->commonHandleAfterDataCallback(status, data, state_.decoder_filters_streaming_) && std::next(entry) != decoder_filters_.end()) { @@ -742,7 +741,7 @@ void FilterManager::decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instan if (end_stream) { disarmRequestTimeout(); } - maybeEndDecode(last_filter_saw_end_stream); + maybeEndDecode(terminal_filter_decoded_end_stream); } RequestTrailerMap& FilterManager::addDecodedTrailers() { @@ -787,7 +786,7 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra // Filter iteration may start at the current filter. std::list::iterator entry = commonDecodePrefix(filter, FilterIterationStartState::CanStartFromCurrent); - bool last_filter_saw_end_stream = false; + bool terminal_filter_decoded_end_stream = false; for (; entry != decoder_filters_.end(); entry++) { // If the filter pointed by entry has stopped for all frame type, return now. @@ -811,10 +810,10 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra } processNewlyAddedMetadata(); - last_filter_saw_end_stream = - std::next(entry) == decoder_filters_.end() || (*entry)->encoded_end_stream_; + terminal_filter_decoded_end_stream = + std::next(entry) == decoder_filters_.end() || (*entry)->filter_encoded_end_stream_; - if (last_filter_saw_end_stream) { + if (terminal_filter_decoded_end_stream) { break; } @@ -824,7 +823,7 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra } } disarmRequestTimeout(); - maybeEndDecode(last_filter_saw_end_stream); + maybeEndDecode(terminal_filter_decoded_end_stream); } void FilterManager::decodeMetadata(ActiveStreamDecoderFilter* filter, MetadataMap& metadata_map) { @@ -954,9 +953,9 @@ void DownstreamFilterManager::sendLocalReply( // Stop filter chain iteration if local reply was sent while filter decoding or encoding callbacks // are running. - /*if (state_.filter_call_state_ & FilterCallState::IsDecodingMask) {*/ + // Local reply always stops decoder filter chain. state_.decoder_filter_chain_aborted_ = true; - /*} else*/ if (state_.filter_call_state_ & FilterCallState::IsEncodingMask) { + if (state_.filter_call_state_ & FilterCallState::IsEncodingMask) { state_.encoder_filter_chain_aborted_ = true; } @@ -1481,17 +1480,23 @@ void FilterManager::maybeEndEncode(bool end_stream) { ASSERT(!state_.encoder_filter_chain_complete_); state_.encoder_filter_chain_complete_ = true; if (filter_manager_callbacks_.isHalfCloseEnabled()) { + // If independent half close is enabled the stream is closed when both decoder and encoder + // filter chains has completed or were aborted. checkAndCloseStreamIfFullyClosed(); } else { + // Otherwise encoding end_stream always closes the stream (and resets it if request was not + // complete yet). filter_manager_callbacks_.endStream(); } } } -void FilterManager::maybeEndDecode(bool end_stream) { - if (end_stream) { +void FilterManager::maybeEndDecode(bool terminal_filter_decoded_end_stream) { + if (terminal_filter_decoded_end_stream) { ASSERT(!state_.decoder_filter_chain_complete_); state_.decoder_filter_chain_complete_ = true; + // If the decoder filter chain was aborted (i.e. due to local reply) + // we rely on the encoding of end_stream to close the stream. if (filter_manager_callbacks_.isHalfCloseEnabled() && !stopDecoderFilterChain()) { checkAndCloseStreamIfFullyClosed(); } @@ -1503,30 +1508,27 @@ void FilterManager::checkAndCloseStreamIfFullyClosed() { if (!filter_manager_callbacks_.isHalfCloseEnabled()) { return; } - - std::cout << typeid(*this).name() << "::checkAndCloseStreamIfFullyClosed() " - << state_.encoder_filter_chain_complete_ << " " << state_.decoder_filter_chain_complete_ - << " " << state_.decoder_filter_chain_aborted_ << "\n"; - - // When the upstream half close is enabled the stream decoding is stopped on error responses + // When the independent half close is enabled the stream is always closed on error responses // from the server. - bool error_response = false; if (filter_manager_callbacks_.responseHeaders().has_value()) { const uint64_t response_status = Http::Utility::getResponseStatus(filter_manager_callbacks_.responseHeaders().ref()); - error_response = + bool error_response = !(Http::CodeUtility::is2xx(response_status) || Http::CodeUtility::is1xx(response_status)); + // Abort the decoder filter if it has not yet been completed. + if (error_response && !state_.decoder_filter_chain_complete_) { + state_.decoder_filter_chain_aborted_ = true; + } } - // If upstream half close is enabled then close the stream either when force close - // is set (i.e local reply) or when both server and client half closed. + // If independent half close is enabled then close the stream when + // 1. Both encoder and decoder filter chains has completed. + // 2. Encoder filter chain has completed and decoder filter chain was aborted (i.e. local reply). + // There is no need to check for aborted encoder filter chain as the filter will either be + // completed or stream is reset. if (state_.encoder_filter_chain_complete_ && - (state_.decoder_filter_chain_complete_ || error_response || - state_.decoder_filter_chain_aborted_)) { + (state_.decoder_filter_chain_complete_ || state_.decoder_filter_chain_aborted_)) { ENVOY_STREAM_LOG(trace, "closing stream", *this); - if (error_response) { - state_.decoder_filter_chain_aborted_ = true; - } filter_manager_callbacks_.endStream(); } } diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 44e23186c331..f4b5184d9bf1 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -280,10 +280,20 @@ struct ActiveStreamDecoderFilter : public ActiveStreamFilterBase, void requestDataTooLarge(); void requestDataDrained(); + // Check if the filter that encoded end_stream has also decoded end_stream and if true + // stop the decoder filter chain. This will end the request after encoder filter chain + // is completed. + // This allows non-terminal filters (i.e. cache filter) to encode responses when independent + // half-close is enabled. Encoding end_stream effectively makes the filter terminal - decoder + // filer chain will not go past this filter. + void maybeStopDecoderFilterChain(bool end_stream); StreamDecoderFilterSharedPtr handle_; bool is_grpc_request_{}; - bool encoded_end_stream_{false}; + // Indicates that this filter called an encodeXXX method with end_stream == true. + // When independent half close is enabled this filter becomes the terminal filter + // in the decoder filter chain. + bool filter_encoded_end_stream_{false}; }; using ActiveStreamDecoderFilterPtr = std::unique_ptr; @@ -788,10 +798,11 @@ class FilterManager : public ScopeTrackedObject, void maybeEndEncode(bool end_stream); /** - * If end_stream is true, marks decoding as complete. This is a noop if end_stream is false. + * If terminal_filter_decoded_end_stream is true, marks decoding as complete. This is a noop if + * terminal_filter_decoded_end_stream is false. * @param end_stream whether decoding is complete. */ - void maybeEndDecode(bool end_stream); + void maybeEndDecode(bool terminal_filter_decoded_end_stream); void checkAndCloseStreamIfFullyClosed(); From 2d8cb15f6c775b9d197a0411f4d4b128096c625f Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Mon, 12 Aug 2024 15:29:14 +0000 Subject: [PATCH 17/27] post merge fixes Signed-off-by: Yan Avlasov --- source/common/http/filter_manager.cc | 14 ++++++++------ source/common/http/filter_manager.h | 2 +- test/common/http/conn_manager_impl_test_2.cc | 14 +++++++------- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index f514ee075444..ce0fe0cd8576 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -466,29 +466,31 @@ void ActiveStreamDecoderFilter::encode1xxHeaders(ResponseHeaderMapPtr&& headers) } } -void ActiveStreamDecoderFilter::maybeStopDecoderFilterChain(bool end_stream) { - filter_encoded_end_stream_ = end_stream; - if (end_stream && end_stream_ && !parent_.state_.decoder_filter_chain_complete_) { +void ActiveStreamDecoderFilter::maybeMarkDecoderFilterTerminal(bool encoded_end_stream) { + filter_encoded_end_stream_ = encoded_end_stream; + // If this filter encoded end_stream and the decoder filter chain has not yet been finished + // then make this filter terminal. Decoding will not go past this filter. + if (encoded_end_stream && end_stream_ && !parent_.state_.decoder_filter_chain_complete_) { parent_.state_.decoder_filter_chain_aborted_ = true; } } void ActiveStreamDecoderFilter::encodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream, absl::string_view details) { - maybeStopDecoderFilterChain(end_stream); + maybeMarkDecoderFilterTerminal(end_stream); parent_.streamInfo().setResponseCodeDetails(details); parent_.filter_manager_callbacks_.setResponseHeaders(std::move(headers)); parent_.encodeHeaders(nullptr, *parent_.filter_manager_callbacks_.responseHeaders(), end_stream); } void ActiveStreamDecoderFilter::encodeData(Buffer::Instance& data, bool end_stream) { - maybeStopDecoderFilterChain(end_stream); + maybeMarkDecoderFilterTerminal(end_stream); parent_.encodeData(nullptr, data, end_stream, FilterManager::FilterIterationStartState::CanStartFromCurrent); } void ActiveStreamDecoderFilter::encodeTrailers(ResponseTrailerMapPtr&& trailers) { - maybeStopDecoderFilterChain(true); + maybeMarkDecoderFilterTerminal(true); parent_.filter_manager_callbacks_.setResponseTrailers(std::move(trailers)); parent_.encodeTrailers(nullptr, *parent_.filter_manager_callbacks_.responseTrailers()); } diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index f4b5184d9bf1..24070f66956d 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -286,7 +286,7 @@ struct ActiveStreamDecoderFilter : public ActiveStreamFilterBase, // This allows non-terminal filters (i.e. cache filter) to encode responses when independent // half-close is enabled. Encoding end_stream effectively makes the filter terminal - decoder // filer chain will not go past this filter. - void maybeStopDecoderFilterChain(bool end_stream); + void maybeMarkDecoderFilterTerminal(bool encoded_end_stream); StreamDecoderFilterSharedPtr handle_; bool is_grpc_request_{}; diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index 5fa5fdf2c6a3..af92cc9cac90 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -49,7 +49,7 @@ TEST_F(HttpConnectionManagerImplTest, ResponseBeforeRequestCompleteWithUpstreamH TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues( {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"}}); - setup(false, "envoy-server-test"); + setup(); setupFilterChain(1, 0); EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, false)) @@ -541,7 +541,7 @@ TEST_F(HttpConnectionManagerImplTest, DrainConnectionUponCompletionVsOnDrainTime Event::MockTimer* connection_duration_timer = setUpTimer(); EXPECT_CALL(*connection_duration_timer, enableTimer(_, _)); // Set up connection. - setup(false, ""); + setup(); // Create a filter so we can encode responses. MockStreamDecoderFilter* filter = new NiceMock(); @@ -1910,7 +1910,7 @@ TEST_F(HttpConnectionManagerImplTest, FilterHeadReply) { TEST_F(HttpConnectionManagerImplTest, LocalReplyStopsDecoding) { InSequence s; - setup(false, ""); + setup(); EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { decoder_ = &conn_manager_->newStream(response_encoder_); @@ -4449,7 +4449,7 @@ TEST_F(HttpConnectionManagerImplTest, EncodingByNonTerminalFilter) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues( {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "false"}}); - setup(false, ""); + setup(); constexpr int total_filters = 3; constexpr int ecoder_filter_index = 1; setupFilterChain(total_filters, total_filters); @@ -4501,7 +4501,7 @@ TEST_F(HttpConnectionManagerImplTest, EncodingByNonTerminalFilterWithIndependent TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues( {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"}}); - setup(false, ""); + setup(); constexpr int total_filters = 3; constexpr int ecoder_filter_index = 1; setupFilterChain(total_filters, total_filters); @@ -4556,7 +4556,7 @@ TEST_F(HttpConnectionManagerImplTest, DecodingByNonTerminalEncoderFilterWithInde TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues( {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"}}); - setup(false, ""); + setup(); constexpr int total_filters = 3; constexpr int ecoder_filter_index = 1; setupFilterChain(total_filters, total_filters); @@ -4625,7 +4625,7 @@ TEST_F(HttpConnectionManagerImplTest, DecodingWithAddedTrailersByNonTerminalEnco TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues( {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"}}); - setup(false, ""); + setup(); constexpr int total_filters = 3; constexpr int ecoder_filter_index = 1; setupFilterChain(total_filters, total_filters); From e1c00d0fa34444f4a99502f3f801bb1078851271 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Mon, 12 Aug 2024 15:35:27 +0000 Subject: [PATCH 18/27] Update comment Signed-off-by: Yan Avlasov --- source/common/http/filter_manager.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index ce0fe0cd8576..6cee55c87f92 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -467,9 +467,13 @@ void ActiveStreamDecoderFilter::encode1xxHeaders(ResponseHeaderMapPtr&& headers) } void ActiveStreamDecoderFilter::maybeMarkDecoderFilterTerminal(bool encoded_end_stream) { - filter_encoded_end_stream_ = encoded_end_stream; // If this filter encoded end_stream and the decoder filter chain has not yet been finished // then make this filter terminal. Decoding will not go past this filter. + filter_encoded_end_stream_ = encoded_end_stream; + + // If the filter that encoded end_stream has also already observed request (downstream) + // end_stream, but the decoder filter chain has not been completed, then decoding is aborted as + // there is no need to iterate over remaining decoder filters any more. if (encoded_end_stream && end_stream_ && !parent_.state_.decoder_filter_chain_complete_) { parent_.state_.decoder_filter_chain_aborted_ = true; } From 018a3e4ff70a30a05079d62e3e3ab82c87760280 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Mon, 12 Aug 2024 17:09:29 +0000 Subject: [PATCH 19/27] Update comments Signed-off-by: Yan Avlasov --- source/common/http/filter_manager.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 24070f66956d..72e54caef6b1 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -901,11 +901,13 @@ class FilterManager : public ScopeTrackedObject, uint32_t filter_call_state_{0}; // Set after decoder filter chain has completed iteration. Prevents further calls to decoder - // filters. + // filters. This flag is used to determine stream completion when the independent half-close is + // enabled. bool decoder_filter_chain_complete_ : 1; // Set after encoder filter chain has completed iteration. Prevents further calls to encoder - // filters. + // filters. This flag is used to determine stream completion when the independent half-close is + // enabled. bool encoder_filter_chain_complete_ : 1; // Set `true` when the filter manager observes end stream on the decoder path (from downstream From 9a692f32613fd7ae74ab278aa104885bf3d46209 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Thu, 15 Aug 2024 17:31:12 +0000 Subject: [PATCH 20/27] Address comments Signed-off-by: Yan Avlasov --- .../observability/access_log/usage.rst | 2 - .../http/http_connection_management.rst | 19 ++ docs/root/intro/life_of_a_request.rst | 20 +- source/common/http/conn_manager_impl.h | 11 + source/common/http/filter_manager.cc | 86 +++++--- source/common/http/filter_manager.h | 17 +- test/common/http/conn_manager_impl_test_2.cc | 55 +---- test/integration/BUILD | 1 + test/integration/filter_integration_test.cc | 208 ++++++++++++++++++ test/integration/filters/BUILD | 22 ++ .../filters/non_terminal_encoding_filter.cc | 148 +++++++++++++ .../non_terminal_encoding_filter.proto | 21 ++ 12 files changed, 513 insertions(+), 97 deletions(-) create mode 100644 test/integration/filters/non_terminal_encoding_filter.cc create mode 100644 test/integration/filters/non_terminal_encoding_filter.proto diff --git a/docs/root/configuration/observability/access_log/usage.rst b/docs/root/configuration/observability/access_log/usage.rst index 11fb3a28fc1f..157a9c870dff 100644 --- a/docs/root/configuration/observability/access_log/usage.rst +++ b/docs/root/configuration/observability/access_log/usage.rst @@ -431,8 +431,6 @@ The following command operators are supported: NOTE: enabling independent half-close behavior for H/2 and H/3 protocols can produce ``*_TX_END`` values lower than ``*_RX_END`` values, in cases where upstream peer has half-closed its stream before downstream peer. In these cases ``COMMON_DURATION`` value will become negative. - Independent half-close behavior is enabled by setting the - ``envoy.reloadable_features.allow_multiplexed_upstream_half_close`` runtime value to ``true``. TCP/UDP Not implemented ("-"). diff --git a/docs/root/intro/arch_overview/http/http_connection_management.rst b/docs/root/intro/arch_overview/http/http_connection_management.rst index f5ff6df01b25..8238ea39084b 100644 --- a/docs/root/intro/arch_overview/http/http_connection_management.rst +++ b/docs/root/intro/arch_overview/http/http_connection_management.rst @@ -36,6 +36,25 @@ In the case of HTTP/1.1, the codec translates the serial/pipelining capabilities that looks like HTTP/2 to higher layers. This means that the majority of the code does not need to understand whether a stream originated on an HTTP/1.1, HTTP/2, or HTTP/3 connection. +HTTP lifecycle +-------------- +Proxying of the request begins when the downstream HTTP codec has successfully decoded request header map. + +The point at which the proxying completes and the stream is destroyed depends on the upstream protocol and +whether the independent half close is enabled. + +If independent half-close is enabled and the upstream protocol is either HTTP/2 or HTTP/3 protocols the stream +is destroyed after both request and response are complete i.e. reach their respective end-of-stream, +by receiving trailers or the header/body with end-stream set in both directions AND response has +success (2xx) status code. + +For HTTP/1 upstream protocol or if independent half-close is disabled the stream is destroyed when the response +is complete and reaches its end-of-stream, i.e. when trailers or the response header/body with +end-stream set are received, even if the request has not yet completed. If request was incomplete at response +completion, the stream is reset. + +Note that proxying can stop early when an error or timeout occurred or when a peer reset HTTP/2 or HTTP/3 stream. + HTTP header sanitizing ---------------------- diff --git a/docs/root/intro/life_of_a_request.rst b/docs/root/intro/life_of_a_request.rst index 7805f3590f2f..95b15836f1b3 100644 --- a/docs/root/intro/life_of_a_request.rst +++ b/docs/root/intro/life_of_a_request.rst @@ -212,9 +212,12 @@ A brief outline of the life cycle of a request and response using the example co :ref:`opposite order ` from the request, starting at the codec filter, traversing any upstream HTTP filters, then going through the router filter and passing through CustomFilter, before being sent downstream. -12. When the response is complete, the stream is destroyed. Post-request processing will update - stats, write to the access log and finalize trace spans. - +12. If independent half-close is enabled the stream is destroyed after both request and response are + complete (END_STREAM for the HTTP/2 stream is observed in both directions) AND response has success + (2xx) status code. Otherwise the stream is destroyed when the response is complete, even if the + request has not yet completed. Post-request processing will update stats, write to the access log + and finalize trace spans. + We elaborate on each of these steps in the sections below. 1. Listener TCP accept @@ -533,8 +536,15 @@ directions during a request. :ref:`Outlier detection ` status for the endpoint is revised as the request progresses. -A request completes when the upstream response reaches its end-of-stream, i.e. when trailers or the -response header/body with end-stream set are received. This is handled in +The point at which the proxying completes and the stream is destroyed for HTTP/2 and HTTP/3 protocols +is determined by the independent half-close option. If independent half-close is enabled the stream +is destroyed after both request and response are complete i.e. reach their respective end-of-stream, +by receiving trailers or the header/body with end-stream set in both directions AND response has +success (2xx) status code. This is handled in ``FilterManager::checkAndCloseStreamIfFullyClosed()``. + +For HTTP/1 protocol or if independent half-close is disabled the stream is destroyed when the response +is complete and reaches its end-of-stream, i.e. when trailers or the response header/body with +end-stream set are received, even if the request has not yet completed. This is handled in ``Router::Filter::onUpstreamComplete()``. It is possible for a request to terminate early. This may be due to (but not limited to): diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 8496fbdee242..5e295ccd1388 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -640,6 +640,17 @@ class ConnectionManagerImpl : Logger::Loggable, uint32_t requests_during_dispatch_count_{0}; const uint32_t max_requests_during_dispatch_{UINT32_MAX}; Event::SchedulableCallbackPtr deferred_request_processing_callback_; + + // If independent half-close is enabled and the upstream protocol is either HTTP/2 or HTTP/3 + // protocols the stream is destroyed after both request and response are complete i.e. reach their + // respective end-of-stream, by receiving trailers or the header/body with end-stream set in both + // directions AND response has success (2xx) status code. + // + // For HTTP/1 upstream protocol or if independent half-close is disabled the stream is destroyed + // when the response is complete and reaches its end-of-stream, i.e. when trailers or the response + // header/body with end-stream set are received, even if the request has not yet completed. If + // request was incomplete at response completion, the stream is reset. + const bool allow_upstream_half_close_{}; }; diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 6cee55c87f92..b000fbdac65e 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -466,35 +466,33 @@ void ActiveStreamDecoderFilter::encode1xxHeaders(ResponseHeaderMapPtr&& headers) } } -void ActiveStreamDecoderFilter::maybeMarkDecoderFilterTerminal(bool encoded_end_stream) { - // If this filter encoded end_stream and the decoder filter chain has not yet been finished - // then make this filter terminal. Decoding will not go past this filter. - filter_encoded_end_stream_ = encoded_end_stream; - - // If the filter that encoded end_stream has also already observed request (downstream) - // end_stream, but the decoder filter chain has not been completed, then decoding is aborted as - // there is no need to iterate over remaining decoder filters any more. - if (encoded_end_stream && end_stream_ && !parent_.state_.decoder_filter_chain_complete_) { +void ActiveStreamDecoderFilter::stopDecodingIfNonTerminalFilterEncodedEndStream( + bool encoded_end_stream) { + // Encoding end_stream by a non-terminal filters (i.e. cache filter) always causes the decoding to + // be stopped even if independent half-close is enabled. For simplicity, independent half-close is + // allowed only when the terminal (router) filter is encoding the response. + if (encoded_end_stream && !parent_.isTerminalDecoderFilter(*this) && + !parent_.state_.decoder_filter_chain_complete_) { parent_.state_.decoder_filter_chain_aborted_ = true; } } void ActiveStreamDecoderFilter::encodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream, absl::string_view details) { - maybeMarkDecoderFilterTerminal(end_stream); + stopDecodingIfNonTerminalFilterEncodedEndStream(end_stream); parent_.streamInfo().setResponseCodeDetails(details); parent_.filter_manager_callbacks_.setResponseHeaders(std::move(headers)); parent_.encodeHeaders(nullptr, *parent_.filter_manager_callbacks_.responseHeaders(), end_stream); } void ActiveStreamDecoderFilter::encodeData(Buffer::Instance& data, bool end_stream) { - maybeMarkDecoderFilterTerminal(end_stream); + stopDecodingIfNonTerminalFilterEncodedEndStream(end_stream); parent_.encodeData(nullptr, data, end_stream, FilterManager::FilterIterationStartState::CanStartFromCurrent); } void ActiveStreamDecoderFilter::encodeTrailers(ResponseTrailerMapPtr&& trailers) { - maybeMarkDecoderFilterTerminal(true); + stopDecodingIfNonTerminalFilterEncodedEndStream(true); parent_.filter_manager_callbacks_.setResponseTrailers(std::move(trailers)); parent_.encodeTrailers(nullptr, *parent_.filter_manager_callbacks_.responseTrailers()); } @@ -542,7 +540,6 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead std::list::iterator entry = commonDecodePrefix(filter, FilterIterationStartState::AlwaysStartFromNext); std::list::iterator continue_data_entry = decoder_filters_.end(); - // Terminal filter is either the last one or filter that encoded end_stream. bool terminal_filter_decoded_end_stream = false; for (; entry != decoder_filters_.end(); entry++) { @@ -617,9 +614,14 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead if (end_stream && buffered_request_data_ && continue_data_entry == decoder_filters_.end()) { continue_data_entry = entry; } + // The decoder filter chain is finished here if the following is true: + // 1. the last filter has observed the end_stream AND + // 2. no filter, including the last filter, has injected body during header iteration. + // If body was injected the end_stream will be processed in the `decodeData()` method below. + const bool no_body_was_injected = continue_data_entry == decoder_filters_.end(); terminal_filter_decoded_end_stream = - (end_stream && continue_data_entry == decoder_filters_.end()) && - (std::next(entry) == decoder_filters_.end() || (*entry)->filter_encoded_end_stream_); + (std::next(entry) == decoder_filters_.end() && (*entry)->end_stream_) && + no_body_was_injected; } maybeContinueDecoding(continue_data_entry); @@ -725,9 +727,13 @@ void FilterManager::decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instan trailers_added_entry = entry; } - terminal_filter_decoded_end_stream = - end_stream && - (std::next(entry) == decoder_filters_.end() || (*entry)->filter_encoded_end_stream_); + // The decoder filter chain is finished here if the following is true: + // 1. the last filter has observed the end_stream AND + // 2. no filter, including the last filter, has injected trailers during header iteration. + // NOTE: end_stream is set to false above if a filter injected trailers. + // If trailers were injected the end_stream will be processed in the `decodeTrailers()` method + // below. + terminal_filter_decoded_end_stream = end_stream && std::next(entry) == decoder_filters_.end(); if (!(*entry)->commonHandleAfterDataCallback(status, data, state_.decoder_filters_streaming_) && std::next(entry) != decoder_filters_.end()) { @@ -792,7 +798,7 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra // Filter iteration may start at the current filter. std::list::iterator entry = commonDecodePrefix(filter, FilterIterationStartState::CanStartFromCurrent); - bool terminal_filter_decoded_end_stream = false; + bool terminal_filter_reached = false; for (; entry != decoder_filters_.end(); entry++) { // If the filter pointed by entry has stopped for all frame type, return now. @@ -816,10 +822,10 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra } processNewlyAddedMetadata(); - terminal_filter_decoded_end_stream = - std::next(entry) == decoder_filters_.end() || (*entry)->filter_encoded_end_stream_; + // Check if the last filter has processed trailers + terminal_filter_reached = std::next(entry) == decoder_filters_.end(); - if (terminal_filter_decoded_end_stream) { + if (terminal_filter_reached) { break; } @@ -829,7 +835,7 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra } } disarmRequestTimeout(); - maybeEndDecode(terminal_filter_decoded_end_stream); + maybeEndDecode(terminal_filter_reached); } void FilterManager::decodeMetadata(ActiveStreamDecoderFilter* filter, MetadataMap& metadata_map) { @@ -959,11 +965,15 @@ void DownstreamFilterManager::sendLocalReply( // Stop filter chain iteration if local reply was sent while filter decoding or encoding callbacks // are running. - // Local reply always stops decoder filter chain. - state_.decoder_filter_chain_aborted_ = true; - if (state_.filter_call_state_ & FilterCallState::IsEncodingMask) { + if (state_.filter_call_state_ & FilterCallState::IsDecodingMask) { + state_.decoder_filter_chain_aborted_ = true; + } else if (state_.filter_call_state_ & FilterCallState::IsEncodingMask) { state_.encoder_filter_chain_aborted_ = true; } + // When independent half-close is enabled local reply always stops decoder filter chain. + if (filter_manager_callbacks_.isHalfCloseEnabled()) { + state_.decoder_filter_chain_aborted_ = true; + } streamInfo().setResponseCodeDetails(details); StreamFilterBase::LocalReplyData data{code, grpc_status, details, false}; @@ -1510,10 +1520,7 @@ void FilterManager::maybeEndDecode(bool terminal_filter_decoded_end_stream) { } void FilterManager::checkAndCloseStreamIfFullyClosed() { - // This function is only used when half close semantics are enabled. - if (!filter_manager_callbacks_.isHalfCloseEnabled()) { - return; - } + ASSERT(filter_manager_callbacks_.isHalfCloseEnabled()); // When the independent half close is enabled the stream is always closed on error responses // from the server. if (filter_manager_callbacks_.responseHeaders().has_value()) { @@ -1527,6 +1534,21 @@ void FilterManager::checkAndCloseStreamIfFullyClosed() { } } + // Handle the case where the end_stream was received from both downstream client and upstream + // server but a decoder filter added either request body or trailers AND paused decoder filter + // chain iteration. This case is currently handled the same way as if independent half-close is + // disabled, i.e. proxying is stopped as soon as encoding has finished. + // TODO(yanavlasov): to support this case the codec needs to notify HCM that it has closed its low + // level stream + // to avoid use-after-free when HCM cleans-up its ActiveStream + const bool downstream_client_sent_end_stream = decoderObservedEndStream(); + const bool decoder_filter_chain_paused = + !state_.decoder_filter_chain_complete_ && !state_.decoder_filter_chain_aborted_; + if (state_.encoder_filter_chain_complete_ && downstream_client_sent_end_stream && + decoder_filter_chain_paused) { + state_.decoder_filter_chain_aborted_ = true; + } + // If independent half close is enabled then close the stream when // 1. Both encoder and decoder filter chains has completed. // 2. Encoder filter chain has completed and decoder filter chain was aborted (i.e. local reply). @@ -1851,6 +1873,10 @@ void FilterManager::resetStream(StreamResetReason reason, filter_manager_callbacks_.resetStream(reason, transport_failure_reason); } +bool FilterManager::isTerminalDecoderFilter(const ActiveStreamDecoderFilter& filter) const { + return !decoder_filters_.empty() && decoder_filters_.back().get() == &filter; +} + void ActiveStreamFilterBase::resetStream(Http::StreamResetReason reset_reason, absl::string_view transport_failure_reason) { parent_.resetStream(reset_reason, transport_failure_reason); diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 72e54caef6b1..262247372694 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -280,20 +280,13 @@ struct ActiveStreamDecoderFilter : public ActiveStreamFilterBase, void requestDataTooLarge(); void requestDataDrained(); - // Check if the filter that encoded end_stream has also decoded end_stream and if true - // stop the decoder filter chain. This will end the request after encoder filter chain - // is completed. - // This allows non-terminal filters (i.e. cache filter) to encode responses when independent - // half-close is enabled. Encoding end_stream effectively makes the filter terminal - decoder - // filer chain will not go past this filter. - void maybeMarkDecoderFilterTerminal(bool encoded_end_stream); + // Encoding end_stream by a non-terminal filters (i.e. cache filter) always causes the decoding to + // be stopped even if independent half-close is enabled. For simplicity, independent half-close is + // enabled only when the terminal (router) filter is encoding the response. + void stopDecodingIfNonTerminalFilterEncodedEndStream(bool encoded_end_stream); StreamDecoderFilterSharedPtr handle_; bool is_grpc_request_{}; - // Indicates that this filter called an encodeXXX method with end_stream == true. - // When independent half close is enabled this filter becomes the terminal filter - // in the decoder filter chain. - bool filter_encoded_end_stream_{false}; }; using ActiveStreamDecoderFilterPtr = std::unique_ptr; @@ -1063,6 +1056,8 @@ class FilterManager : public ScopeTrackedObject, bool stopDecoderFilterChain() { return state_.decoder_filter_chain_aborted_; } + bool isTerminalDecoderFilter(const ActiveStreamDecoderFilter& filter) const; + FilterManagerCallbacks& filter_manager_callbacks_; Event::Dispatcher& dispatcher_; // This is unset if there is no downstream connection, e.g. for health check or diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index af92cc9cac90..f7a2b55d1246 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -4549,9 +4549,9 @@ TEST_F(HttpConnectionManagerImplTest, EncodingByNonTerminalFilterWithIndependent } // Validate that when independent half-close is enabled, encoding end_stream by a -// non-final filter with incomplete request makes the encoding filter the terminal filter. -// In this case decoding end_stream from the client only reaches the filter that encoded the -// end_stream after which the request is completed. +// non-final filter with incomplete request causes the request to be reset. +// Only the terminal filter (router) is allowed to half-close upstream response before the +// downstream request. TEST_F(HttpConnectionManagerImplTest, DecodingByNonTerminalEncoderFilterWithIndependentHalfClose) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues( @@ -4592,29 +4592,11 @@ TEST_F(HttpConnectionManagerImplTest, DecodingByNonTerminalEncoderFilterWithInde .WillOnce(Return(FilterDataStatus::Continue)); EXPECT_CALL(*encoder_filters_[0], encodeComplete()); + expectOnDestroy(); + // Second decoder filter then completes encoding with data Buffer::OwnedImpl fake_response("world"); decoder_filters_[ecoder_filter_index]->callbacks_->encodeData(fake_response, true); - - // Request is still be alive with the half-close enabled. - // Verify that once the end_stream from the client reaches the filter that encoded the end_stream - // the request will end. - EXPECT_CALL(*decoder_filters_[0], decodeData(_, true)) - .WillOnce(Return(FilterDataStatus::Continue)); - EXPECT_CALL(*decoder_filters_[0], decodeComplete()); - EXPECT_CALL(*decoder_filters_[1], decodeData(_, true)) - .WillOnce(Return(FilterDataStatus::StopIterationNoBuffer)); - EXPECT_CALL(*decoder_filters_[1], decodeComplete()); - expectOnDestroy(); - - EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { - decoder_->decodeData(data, true); - data.drain(4); - return Http::okStatus(); - })); - - Buffer::OwnedImpl fake_input("5678"); - conn_manager_->onData(fake_input, false); } // Validate that when independent half-close is enabled, encoding end_stream by a @@ -4660,36 +4642,11 @@ TEST_F(HttpConnectionManagerImplTest, DecodingWithAddedTrailersByNonTerminalEnco EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) .WillOnce(Return(FilterDataStatus::Continue)); EXPECT_CALL(*encoder_filters_[0], encodeComplete()); + expectOnDestroy(); // Second decoder filter then completes encoding with data Buffer::OwnedImpl fake_response("world"); decoder_filters_[ecoder_filter_index]->callbacks_->encodeData(fake_response, true); - - // Request is still be alive with the half-close enabled. - // Verify that once the end_stream from the client reaches the filter that encoded the end_stream - // the request will end. - EXPECT_CALL(*decoder_filters_[0], decodeData(_, true)) - .WillOnce(InvokeWithoutArgs([&]() -> FilterDataStatus { - decoder_filters_[1]->callbacks_->addDecodedTrailers().addCopy(Http::LowerCaseString("foo"), - "bar"); - return FilterDataStatus::Continue; - })); - EXPECT_CALL(*decoder_filters_[0], decodeComplete()); - EXPECT_CALL(*decoder_filters_[1], decodeData(_, false)) - .WillOnce(Return(FilterDataStatus::StopIterationNoBuffer)); - EXPECT_CALL(*decoder_filters_[1], decodeTrailers(_)) - .WillOnce(Return(FilterTrailersStatus::StopIteration)); - EXPECT_CALL(*decoder_filters_[1], decodeComplete()); - expectOnDestroy(); - - EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { - decoder_->decodeData(data, true); - data.drain(4); - return Http::okStatus(); - })); - - Buffer::OwnedImpl fake_input("5678"); - conn_manager_->onData(fake_input, false); } } // namespace Http diff --git a/test/integration/BUILD b/test/integration/BUILD index 1e2e7c57c4f8..5376140ace83 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -780,6 +780,7 @@ envoy_cc_test( "//test/integration/filters:local_reply_during_encoding_filter_lib", "//test/integration/filters:local_reply_with_metadata_filter_lib", "//test/integration/filters:metadata_control_filter_lib", + "//test/integration/filters:non_terminal_encoding_filter_lib", "//test/integration/filters:on_local_reply_filter_config_lib", "//test/integration/filters:remove_response_headers_lib", "//test/integration/filters:reset_stream_filter_config_lib", diff --git a/test/integration/filter_integration_test.cc b/test/integration/filter_integration_test.cc index 6fd7fa53b2a1..5e7538438595 100644 --- a/test/integration/filter_integration_test.cc +++ b/test/integration/filter_integration_test.cc @@ -34,6 +34,10 @@ class FilterIntegrationTest : public UpstreamDownstreamIntegrationTest { EXPECT_EQ(true, upstream_request_->complete()); } + void testNonTerminalEncodingFilterWithCompleteRequest(); + void testNonTerminalEncodingFilterWithIncompleteRequest(); + void testFilterAddsDataAndTrailersToHeaderOnlyRequest(); + const int count_ = 70; const int size_ = 1000; const int added_decoded_data_size_ = 1; @@ -1360,5 +1364,209 @@ TEST_P(FilterIntegrationTest, LocalReplyFromEncodeMetadata) { cleanupUpstreamAndDownstream(); } + +// Validate that adding trailers during encode/decodeData with end_stream==true works correctly +// with half close enabled. +TEST_P(FilterIntegrationTest, FilterAddsTrailersWithIndependentHalfClose) { + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"); + prependFilter(R"EOF( + name: add-trailers-filter + )EOF"); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = + codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":scheme", "http"}, + {":path", "/test/long/url"}, + {":authority", "sni.lyft.com"}}); + auto request_encoder = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, false); + // The add-body-filter will add trailers to the response + upstream_request_->encodeData(100, true); + + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_EQ(response->body().size(), 100); + // Make sure response trailers added by the filter were received by the client + EXPECT_EQ(response->trailers()->size(), 1); + + Buffer::OwnedImpl data(std::string(100, 'r')); + codec_client_->sendData(*request_encoder, data, true); + + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + ASSERT_TRUE(upstream_request_->complete()); + // Make sure request trailers added by the filter were received by the upstream + EXPECT_EQ(upstream_request_->trailers()->size(), 1); + EXPECT_EQ(upstream_request_->bodyLength(), 100); +} + +void FilterIntegrationTest::testNonTerminalEncodingFilterWithIncompleteRequest() { + // Encoding by non-terminal upstream filter is not supported + if (!testing_downstream_filter_) { + return; + } + prependFilter(R"EOF( + name: non-terminal-encoding-filter + typed_config: + "@type": type.googleapis.com/test.integration.filters.NonTerminalEncodingFilterConfig + where_to_start_encoding: DECODE_HEADERS + encode_body: IN_TIMER_CALLBACK + encode_trailers: IN_TIMER_CALLBACK + )EOF"); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = + codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":scheme", "http"}, + {":path", "/test/long/url"}, + {":authority", "sni.lyft.com"}}); + auto response = std::move(encoder_decoder.second); + + ASSERT_TRUE(response->waitForReset()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_EQ(response->body(), "encoded body"); + EXPECT_EQ(response->trailers()->size(), 1); +} + +// Verify that when non terminal filter encodes end_stream with request still incomplete +// the stream is reset. The behavior is the same with or without independent half-close enabled. +TEST_P(FilterIntegrationTest, NonTerminalEncodingFilterWithIncompleteRequest) { + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "false"); + testNonTerminalEncodingFilterWithIncompleteRequest(); +} + +TEST_P(FilterIntegrationTest, + NonTerminalEncodingFilterWithIncompleteRequestAndIdependentHalfClose) { + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"); + testNonTerminalEncodingFilterWithIncompleteRequest(); +} + +void FilterIntegrationTest::testNonTerminalEncodingFilterWithCompleteRequest() { + // Encoding by non-terminal upstream filter is not supported + if (!testing_downstream_filter_) { + return; + } + prependFilter(R"EOF( + name: non-terminal-encoding-filter + typed_config: + "@type": type.googleapis.com/test.integration.filters.NonTerminalEncodingFilterConfig + where_to_start_encoding: DECODE_DATA + encode_body: SYNCHRONOUSLY + encode_trailers: IN_TIMER_CALLBACK + )EOF"); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = + codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":scheme", "http"}, + {":path", "/test/long/url"}, + {":authority", "sni.lyft.com"}}); + auto request_encoder = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + Buffer::OwnedImpl data(std::string(100, 'r')); + // Complete request and kick off response encoding from decodeData + codec_client_->sendData(*request_encoder, data, true); + + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_FALSE(response->reset()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_EQ(response->body(), "encoded body"); + EXPECT_EQ(response->trailers()->size(), 1); +} + +// Verify that when non terminal filter encodes end_stream with request complete +// the stream is finished gracefully and not reset. The behavior is the same with or without +// independent half-close enabled. +TEST_P(FilterIntegrationTest, NonTerminalEncodingFilterWithCompleteRequest) { + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "false"); + testNonTerminalEncodingFilterWithCompleteRequest(); +} + +TEST_P(FilterIntegrationTest, NonTerminalEncodingFilterWithCompleteRequestAndIdependentHalfClose) { + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"); + testNonTerminalEncodingFilterWithCompleteRequest(); +} + +void FilterIntegrationTest::testFilterAddsDataAndTrailersToHeaderOnlyRequest() { + prependFilter(R"EOF( + name: add-trailers-filter + )EOF"); + prependFilter(R"EOF( + name: add-body-filter-2 + typed_config: + "@type": type.googleapis.com/test.integration.filters.AddBodyFilterConfig + where_to_add_body: ENCODE_HEADERS + body_size: 100 + where_to_stop_and_buffer: DECODE_DATA + )EOF"); + prependFilter(R"EOF( + name: add-body-filter-1 + typed_config: + "@type": type.googleapis.com/test.integration.filters.AddBodyFilterConfig + where_to_add_body: DECODE_HEADERS + body_size: 100 + )EOF"); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + IntegrationStreamDecoderPtr response = + codec_client_->makeHeaderOnlyRequest(default_request_headers_); + + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + // Even though client's request had only headers, the add-body-filter will add body and pause + // filter chain in decodeData, so the upstream should see incomplete request. + ASSERT_FALSE(upstream_request_->complete()); + + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true); + + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + // Make sure response body added by the filter were received by the client + EXPECT_EQ(response->body().size(), 100); + // Note that client will not see reset because Envoy's downstream codec does not + // send RST_STREAM after it observed END_STREAM in both directions. + EXPECT_FALSE(response->reset()); + + if (testing_downstream_filter_) { + // When response is complete the downstream FM will reset the upstream request that was made + // incomplete by adding data and then pausing decoder filter chain. + ASSERT_TRUE(upstream_request_->waitForReset()); + } else { + // The upstream response completes successfully if add-body-filter is an upstream filter. + // This is because the downstream HCM sees completed stream and does not issue reset. + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + } +} + +// Validate that extending request lifetime by adding body and trailers to a header only request +// and then pausing decoder filter chain, results in stream reset when encoding completes. +// The behavior is the same with or without independent half-close enabled. +TEST_P(FilterIntegrationTest, FilterAddsDataToHeaderOnlyRequest) { + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "false"); + testFilterAddsDataAndTrailersToHeaderOnlyRequest(); +} + +TEST_P(FilterIntegrationTest, FilterAddsDataToHeaderOnlyRequestWithIndependentHalfClose) { + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.allow_multiplexed_upstream_half_close", "true"); + testFilterAddsDataAndTrailersToHeaderOnlyRequest(); +} + } // namespace } // namespace Envoy diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index 4c99af8610ba..9b034519084b 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -962,3 +962,25 @@ envoy_cc_test_library( "//source/extensions/filters/network/common:factory_base_lib", ], ) + +envoy_proto_library( + name = "non_terminal_encoding_filter_proto", + srcs = ["non_terminal_encoding_filter.proto"], +) + +envoy_cc_test_library( + name = "non_terminal_encoding_filter_lib", + srcs = [ + "non_terminal_encoding_filter.cc", + ], + deps = [ + ":common_lib", + ":non_terminal_encoding_filter_proto_cc_proto", + "//envoy/http:filter_interface", + "//envoy/registry", + "//envoy/server:filter_config_interface", + "//source/extensions/filters/http/common:factory_base_lib", + "//source/extensions/filters/http/common:pass_through_filter_lib", + "//test/integration:utility_lib", + ], +) diff --git a/test/integration/filters/non_terminal_encoding_filter.cc b/test/integration/filters/non_terminal_encoding_filter.cc new file mode 100644 index 000000000000..e106778bbe9d --- /dev/null +++ b/test/integration/filters/non_terminal_encoding_filter.cc @@ -0,0 +1,148 @@ +#include + +#include "envoy/http/filter.h" +#include "envoy/registry/registry.h" +#include "envoy/server/filter_config.h" + +#include "source/common/buffer/buffer_impl.h" +#include "source/extensions/filters/http/common/factory_base.h" +#include "source/extensions/filters/http/common/pass_through_filter.h" + +#include "test/integration/filters/common.h" +#include "test/integration/filters/non_terminal_encoding_filter.pb.h" +#include "test/integration/filters/non_terminal_encoding_filter.pb.validate.h" +#include "test/integration/utility.h" + +namespace Envoy { + +class NonTerminalEncodingFilterConfig { +public: + NonTerminalEncodingFilterConfig( + test::integration::filters::NonTerminalEncodingFilterConfig::WhereToStartEncoding + where_to_start_encoding, + test::integration::filters::NonTerminalEncodingFilterConfig::HowToEncode encode_body, + test::integration::filters::NonTerminalEncodingFilterConfig::HowToEncode encode_trailers) + : where_to_start_encoding_(where_to_start_encoding), encode_body_(encode_body), + encode_trailers_(encode_trailers) {} + + const test::integration::filters::NonTerminalEncodingFilterConfig::WhereToStartEncoding + where_to_start_encoding_; + const test::integration::filters::NonTerminalEncodingFilterConfig::HowToEncode encode_body_; + const test::integration::filters::NonTerminalEncodingFilterConfig::HowToEncode encode_trailers_; +}; + +// A test filter that adds body data to a request/response without body payload. +class NonTerminalEncodingFilter : public Http::PassThroughFilter { +public: + NonTerminalEncodingFilter(std::shared_ptr config) + : config_(config) {} + + void startEncoding() { + const bool end_stream = + config_->encode_body_ == + test::integration::filters::NonTerminalEncodingFilterConfig::SKIP_ENCODING && + config_->encode_trailers_ == + test::integration::filters::NonTerminalEncodingFilterConfig::SKIP_ENCODING; + Http::ResponseHeaderMapPtr response_headers = std::make_unique( + Http::TestResponseHeaderMapImpl{{":status", "200"}}); + decoder_callbacks_->encodeHeaders(std::move(response_headers), end_stream, ""); + if (config_->encode_body_ != + test::integration::filters::NonTerminalEncodingFilterConfig::SKIP_ENCODING) { + encodeBody(); + } else if (config_->encode_trailers_ != + test::integration::filters::NonTerminalEncodingFilterConfig::SKIP_ENCODING) { + encodeResponseTrailers(); + } + } + + void encodeBody() { + auto encode_lambda = [this]() -> void { + const bool end_stream = + config_->encode_trailers_ == + test::integration::filters::NonTerminalEncodingFilterConfig::SKIP_ENCODING; + Buffer::OwnedImpl buffer("encoded body"); + decoder_callbacks_->encodeData(buffer, end_stream); + if (config_->encode_trailers_ != + test::integration::filters::NonTerminalEncodingFilterConfig::SKIP_ENCODING) { + encodeResponseTrailers(); + } + }; + if (config_->encode_body_ == + test::integration::filters::NonTerminalEncodingFilterConfig::SYNCHRONOUSLY) { + encode_lambda(); + } else { + decoder_callbacks_->dispatcher().post(encode_lambda); + } + } + + void encodeResponseTrailers() { + auto encode_lambda = [this]() -> void { + Http::ResponseTrailerMapPtr response_trailers = + std::make_unique( + Http::TestResponseTrailerMapImpl{{"foo", "bar"}}); + decoder_callbacks_->encodeTrailers(std::move(response_trailers)); + }; + if (config_->encode_trailers_ == + test::integration::filters::NonTerminalEncodingFilterConfig::SYNCHRONOUSLY) { + encode_lambda(); + } else { + decoder_callbacks_->dispatcher().post(encode_lambda); + } + } + + Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap&, bool) override { + if (config_->where_to_start_encoding_ == + test::integration::filters::NonTerminalEncodingFilterConfig::DECODE_HEADERS) { + startEncoding(); + } + + return Http::FilterHeadersStatus::StopIteration; + } + + Http::FilterDataStatus decodeData(Buffer::Instance&, bool) override { + if (config_->where_to_start_encoding_ == + test::integration::filters::NonTerminalEncodingFilterConfig::DECODE_DATA) { + startEncoding(); + return Http::FilterDataStatus::StopIterationNoBuffer; + } + return Http::FilterDataStatus::StopIterationNoBuffer; + } + + Http::FilterTrailersStatus decodeTrailers(Http::RequestTrailerMap&) override { + if (config_->where_to_start_encoding_ == + test::integration::filters::NonTerminalEncodingFilterConfig::DECODE_TRAILERS) { + startEncoding(); + } + return Http::FilterTrailersStatus::StopIteration; + } + +private: + const std::shared_ptr config_; +}; + +class NonTerminalEncodingFilterFactory + : public Extensions::HttpFilters::Common::DualFactoryBase< + test::integration::filters::NonTerminalEncodingFilterConfig> { +public: + NonTerminalEncodingFilterFactory() : DualFactoryBase("non-terminal-encoding-filter") {} + +private: + absl::StatusOr createFilterFactoryFromProtoTyped( + const test::integration::filters::NonTerminalEncodingFilterConfig& proto_config, + const std::string&, DualInfo, Server::Configuration::ServerFactoryContext&) override { + auto filter_config = std::make_shared( + proto_config.where_to_start_encoding(), proto_config.encode_body(), + proto_config.encode_trailers()); + return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamFilter(std::make_shared(filter_config)); + }; + } +}; + +using UpstreamNonTerminalEncodingFilterFactory = NonTerminalEncodingFilterFactory; + +REGISTER_FACTORY(NonTerminalEncodingFilterFactory, + Server::Configuration::NamedHttpFilterConfigFactory); +REGISTER_FACTORY(UpstreamNonTerminalEncodingFilterFactory, + Server::Configuration::UpstreamHttpFilterConfigFactory); +} // namespace Envoy diff --git a/test/integration/filters/non_terminal_encoding_filter.proto b/test/integration/filters/non_terminal_encoding_filter.proto new file mode 100644 index 000000000000..46f9aa84b275 --- /dev/null +++ b/test/integration/filters/non_terminal_encoding_filter.proto @@ -0,0 +1,21 @@ +syntax = "proto3"; + +package test.integration.filters; + +message NonTerminalEncodingFilterConfig { + enum WhereToStartEncoding { + DECODE_HEADERS = 0; + DECODE_DATA = 1; + DECODE_TRAILERS = 2; + } + + enum HowToEncode { + SKIP_ENCODING = 0; + SYNCHRONOUSLY = 1; + IN_TIMER_CALLBACK = 2; + } + + WhereToStartEncoding where_to_start_encoding = 1; + HowToEncode encode_body = 2; + HowToEncode encode_trailers = 3; +} From b9948abdc3aa15b1cd5ca86435cbb5fbd5c9410e Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Thu, 15 Aug 2024 18:17:57 +0000 Subject: [PATCH 21/27] Post merge fix Signed-off-by: Yan Avlasov --- test/common/http/conn_manager_impl_test_base.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/http/conn_manager_impl_test_base.cc b/test/common/http/conn_manager_impl_test_base.cc index 2f88d3ca9a72..87fc4eacb8d8 100644 --- a/test/common/http/conn_manager_impl_test_base.cc +++ b/test/common/http/conn_manager_impl_test_base.cc @@ -261,7 +261,7 @@ void HttpConnectionManagerImplMixin::setupFilterChain(int num_decoder_filters, auto factory = createDecoderFilterFactoryCb( StreamDecoderFilterSharedPtr{decoder_filters_[req * num_decoder_filters + i]}); std::string name = absl::StrCat(req * num_decoder_filters + i); - manager.applyFilterFactoryCb({name, name}, factory); + manager.applyFilterFactoryCb({name}, factory); applied_filters = true; } @@ -269,7 +269,7 @@ void HttpConnectionManagerImplMixin::setupFilterChain(int num_decoder_filters, auto factory = createEncoderFilterFactoryCb( StreamEncoderFilterSharedPtr{encoder_filters_[req * num_encoder_filters + i]}); std::string name = absl::StrCat(req * num_decoder_filters + i); - manager.applyFilterFactoryCb({name, name}, factory); + manager.applyFilterFactoryCb({name}, factory); applied_filters = true; } return applied_filters; From e224738c5dbcca761f7a2c8f2b517c0e0158752e Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Thu, 15 Aug 2024 18:28:32 +0000 Subject: [PATCH 22/27] Update comment Signed-off-by: Yan Avlasov --- test/integration/filters/non_terminal_encoding_filter.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/filters/non_terminal_encoding_filter.cc b/test/integration/filters/non_terminal_encoding_filter.cc index e106778bbe9d..a3ae528c8e7c 100644 --- a/test/integration/filters/non_terminal_encoding_filter.cc +++ b/test/integration/filters/non_terminal_encoding_filter.cc @@ -31,7 +31,7 @@ class NonTerminalEncodingFilterConfig { const test::integration::filters::NonTerminalEncodingFilterConfig::HowToEncode encode_trailers_; }; -// A test filter that adds body data to a request/response without body payload. +// A test filter that encodes a response in a decodeXXX handler. class NonTerminalEncodingFilter : public Http::PassThroughFilter { public: NonTerminalEncodingFilter(std::shared_ptr config) From d5320daa55c9a966e8194110b281c15a319ec4c9 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 16 Aug 2024 14:42:00 +0000 Subject: [PATCH 23/27] Fix docs Signed-off-by: Yan Avlasov --- .../intro/arch_overview/http/http_connection_management.rst | 3 ++- docs/root/intro/life_of_a_request.rst | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/root/intro/arch_overview/http/http_connection_management.rst b/docs/root/intro/arch_overview/http/http_connection_management.rst index 8238ea39084b..24559ca777af 100644 --- a/docs/root/intro/arch_overview/http/http_connection_management.rst +++ b/docs/root/intro/arch_overview/http/http_connection_management.rst @@ -38,9 +38,10 @@ understand whether a stream originated on an HTTP/1.1, HTTP/2, or HTTP/3 connect HTTP lifecycle -------------- + Proxying of the request begins when the downstream HTTP codec has successfully decoded request header map. -The point at which the proxying completes and the stream is destroyed depends on the upstream protocol and +The point at which the proxying completes and the stream is destroyed depends on the upstream protocol and whether the independent half close is enabled. If independent half-close is enabled and the upstream protocol is either HTTP/2 or HTTP/3 protocols the stream diff --git a/docs/root/intro/life_of_a_request.rst b/docs/root/intro/life_of_a_request.rst index 95b15836f1b3..3ea6b1f60ee9 100644 --- a/docs/root/intro/life_of_a_request.rst +++ b/docs/root/intro/life_of_a_request.rst @@ -217,7 +217,7 @@ A brief outline of the life cycle of a request and response using the example co (2xx) status code. Otherwise the stream is destroyed when the response is complete, even if the request has not yet completed. Post-request processing will update stats, write to the access log and finalize trace spans. - + We elaborate on each of these steps in the sections below. 1. Listener TCP accept From b804cb517ee7248e54a3d072aa2ecb265c414428 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 16 Aug 2024 17:22:00 +0000 Subject: [PATCH 24/27] Disable upstream filter tests Signed-off-by: Yan Avlasov --- test/integration/filter_integration_test.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/integration/filter_integration_test.cc b/test/integration/filter_integration_test.cc index 5e7538438595..e83a9d231188 100644 --- a/test/integration/filter_integration_test.cc +++ b/test/integration/filter_integration_test.cc @@ -1502,6 +1502,13 @@ TEST_P(FilterIntegrationTest, NonTerminalEncodingFilterWithCompleteRequestAndIde } void FilterIntegrationTest::testFilterAddsDataAndTrailersToHeaderOnlyRequest() { + // When an upstream filter adds body to the header only request the result observed by + // the upstream server is unpredictable. Sending of the added body races with the + // downstream FM closing the request, as it observed end_stream in both directions. + // This use case is effectively unsupported at this point. + if (!testing_downstream_filter_) { + return; + } prependFilter(R"EOF( name: add-trailers-filter )EOF"); From 4b305d9e6ff1993b6d26db1fa608c36163270178 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 16 Aug 2024 19:49:39 +0000 Subject: [PATCH 25/27] Add coverage Signed-off-by: Yan Avlasov --- test/common/quic/envoy_quic_utils_test.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/common/quic/envoy_quic_utils_test.cc b/test/common/quic/envoy_quic_utils_test.cc index df295c1f2e54..a6d287e2438a 100644 --- a/test/common/quic/envoy_quic_utils_test.cc +++ b/test/common/quic/envoy_quic_utils_test.cc @@ -271,6 +271,9 @@ TEST(EnvoyQuicUtilsTest, EnvoyResetReasonToQuicResetErrorCodeImpossibleCases) { EXPECT_ENVOY_BUG( envoyResetReasonToQuicRstError(Http::StreamResetReason::RemoteRefusedStreamReset), "Remote reset "); + EXPECT_ENVOY_BUG( + envoyResetReasonToQuicRstError(Http::StreamResetReason::Http1PrematureUpstreamHalfClose), + "not applicable"); } TEST(EnvoyQuicUtilsTest, QuicResetErrorToEnvoyResetReason) { From 9155e4957dc2663a3386eca071e54ba28e21a226 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Wed, 21 Aug 2024 15:32:38 +0000 Subject: [PATCH 26/27] Address comments Signed-off-by: Yan Avlasov --- .../arch_overview/http/http_connection_management.rst | 4 ++-- source/common/http/filter_manager.cc | 9 ++------- source/common/http/filter_manager.h | 2 +- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/docs/root/intro/arch_overview/http/http_connection_management.rst b/docs/root/intro/arch_overview/http/http_connection_management.rst index 24559ca777af..79eed998300b 100644 --- a/docs/root/intro/arch_overview/http/http_connection_management.rst +++ b/docs/root/intro/arch_overview/http/http_connection_management.rst @@ -42,7 +42,7 @@ HTTP lifecycle Proxying of the request begins when the downstream HTTP codec has successfully decoded request header map. The point at which the proxying completes and the stream is destroyed depends on the upstream protocol and -whether the independent half close is enabled. +whether independent half close is enabled. If independent half-close is enabled and the upstream protocol is either HTTP/2 or HTTP/3 protocols the stream is destroyed after both request and response are complete i.e. reach their respective end-of-stream, @@ -51,7 +51,7 @@ success (2xx) status code. For HTTP/1 upstream protocol or if independent half-close is disabled the stream is destroyed when the response is complete and reaches its end-of-stream, i.e. when trailers or the response header/body with -end-stream set are received, even if the request has not yet completed. If request was incomplete at response +end-stream set are received, even if the request has not yet completed. If the request was incomplete at response completion, the stream is reset. Note that proxying can stop early when an error or timeout occurred or when a peer reset HTTP/2 or HTTP/3 stream. diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index b000fbdac65e..88b9904578c2 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -825,12 +825,7 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra // Check if the last filter has processed trailers terminal_filter_reached = std::next(entry) == decoder_filters_.end(); - if (terminal_filter_reached) { - break; - } - - if (!(*entry)->commonHandleAfterTrailersCallback(status) && - std::next(entry) != decoder_filters_.end()) { + if (!(*entry)->commonHandleAfterTrailersCallback(status) && !terminal_filter_reached) { return; } } @@ -1526,7 +1521,7 @@ void FilterManager::checkAndCloseStreamIfFullyClosed() { if (filter_manager_callbacks_.responseHeaders().has_value()) { const uint64_t response_status = Http::Utility::getResponseStatus(filter_manager_callbacks_.responseHeaders().ref()); - bool error_response = + const bool error_response = !(Http::CodeUtility::is2xx(response_status) || Http::CodeUtility::is1xx(response_status)); // Abort the decoder filter if it has not yet been completed. if (error_response && !state_.decoder_filter_chain_complete_) { diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 262247372694..945ebf844697 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -282,7 +282,7 @@ struct ActiveStreamDecoderFilter : public ActiveStreamFilterBase, void requestDataDrained(); // Encoding end_stream by a non-terminal filters (i.e. cache filter) always causes the decoding to // be stopped even if independent half-close is enabled. For simplicity, independent half-close is - // enabled only when the terminal (router) filter is encoding the response. + // enabled only when the terminal filter is encoding the response. void stopDecodingIfNonTerminalFilterEncodedEndStream(bool encoded_end_stream); StreamDecoderFilterSharedPtr handle_; From c02ac473e08bae7123009332dcebf853dbfdb2b0 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 23 Aug 2024 23:07:14 +0000 Subject: [PATCH 27/27] Add asserts Signed-off-by: Yan Avlasov --- source/common/http/filter_manager.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 88b9904578c2..be2c9b7af44f 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -541,6 +541,8 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead commonDecodePrefix(filter, FilterIterationStartState::AlwaysStartFromNext); std::list::iterator continue_data_entry = decoder_filters_.end(); bool terminal_filter_decoded_end_stream = false; + ASSERT(!state_.decoder_filter_chain_complete_ || entry == decoder_filters_.end() || + (*entry)->end_stream_); for (; entry != decoder_filters_.end(); entry++) { ASSERT(!(state_.filter_call_state_ & FilterCallState::DecodeHeaders)); @@ -650,6 +652,8 @@ void FilterManager::decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instan std::list::iterator entry = commonDecodePrefix(filter, filter_iteration_start_state); bool terminal_filter_decoded_end_stream = false; + ASSERT(!state_.decoder_filter_chain_complete_ || entry == decoder_filters_.end() || + (*entry)->end_stream_); for (; entry != decoder_filters_.end(); entry++) { // If the filter pointed by entry has stopped for all frame types, return now. @@ -799,6 +803,7 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra std::list::iterator entry = commonDecodePrefix(filter, FilterIterationStartState::CanStartFromCurrent); bool terminal_filter_reached = false; + ASSERT(!state_.decoder_filter_chain_complete_ || entry == decoder_filters_.end()); for (; entry != decoder_filters_.end(); entry++) { // If the filter pointed by entry has stopped for all frame type, return now.