Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiplexed upstream servers to half close the stream before the downstream #34461

Merged
merged 37 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
7263c53
Allow multiplexed upstream servers to half close the stream before th…
yanavlasov May 31, 2024
fd17760
Address comments
yanavlasov Jun 21, 2024
723ed65
Merge branch 'main' into allow-upstream-half-close
yanavlasov Jun 21, 2024
1d8172c
Address comments
yanavlasov Jun 22, 2024
3ba7cc4
Clarify comment
yanavlasov Jun 24, 2024
6133efc
Reuse isHalfCloseEnabled callback
yanavlasov Jun 24, 2024
fcf2a76
Add stopDecoding
yanavlasov Jun 24, 2024
f24bc34
Merge branch 'main' into allow-upstream-half-close
yanavlasov Jun 25, 2024
0e881dc
Merge branch 'main' into allow-upstream-half-close
yanavlasov Jun 28, 2024
b7b746f
Fixing tests
yanavlasov Jul 26, 2024
af60f0b
Address post merge test failures
yanavlasov Jul 30, 2024
01ae68b
Merge branch 'main' into allow-upstream-half-close
yanavlasov Jul 30, 2024
1f269a4
Post merge fix
yanavlasov Jul 30, 2024
1b961df
Fix gcc build error
yanavlasov Jul 30, 2024
8ae88a0
WiP
yanavlasov Jul 30, 2024
d8aabba
Merge branch 'main' into allow-upstream-half-close
yanavlasov Aug 2, 2024
2166fcb
Post merge fixes
yanavlasov Aug 2, 2024
e2deca2
WiP
yanavlasov Aug 8, 2024
9e9428f
Make ending filters in the middle of the filter chain work. p1
yanavlasov Aug 9, 2024
71ce1f0
Make encoding filters in the middle of the filter chain work. p2
yanavlasov Aug 9, 2024
68c807e
Update comments
yanavlasov Aug 12, 2024
9c308ba
Merge branch 'main' into allow-upstream-half-close
yanavlasov Aug 12, 2024
2d8cb15
post merge fixes
yanavlasov Aug 12, 2024
e1c00d0
Update comment
yanavlasov Aug 12, 2024
018a3e4
Update comments
yanavlasov Aug 12, 2024
9a692f3
Address comments
yanavlasov Aug 15, 2024
bd30f81
Merge branch 'main' into allow-upstream-half-close
yanavlasov Aug 15, 2024
b9948ab
Post merge fix
yanavlasov Aug 15, 2024
e224738
Update comment
yanavlasov Aug 15, 2024
e4ae6b4
Merge branch 'main' into allow-upstream-half-close
yanavlasov Aug 16, 2024
d5320da
Fix docs
yanavlasov Aug 16, 2024
b804cb5
Disable upstream filter tests
yanavlasov Aug 16, 2024
4b305d9
Add coverage
yanavlasov Aug 16, 2024
9155e49
Address comments
yanavlasov Aug 21, 2024
49b6162
Merge branch 'main' into allow-upstream-half-close
yanavlasov Aug 23, 2024
c02ac47
Add asserts
yanavlasov Aug 23, 2024
d965ff6
Merge branch 'main' into allow-upstream-half-close
yanavlasov Aug 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ behavior_changes:
change: |
Change ``OnLogDownstreamStart``, ``OnLogDownstreamPeriodic`` and ``OnLog`` methods so that user can get the request/response's
headers and trailers when producing access log.
- 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.
- area: http
change: |
Added HTTP1-safe option for :ref:`max_connection_duration
Expand Down
4 changes: 4 additions & 0 deletions docs/root/configuration/observability/access_log/usage.rst
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,10 @@ 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.

TCP/UDP
Not implemented ("-").

Expand Down
20 changes: 20 additions & 0 deletions docs/root/intro/arch_overview/http/http_connection_management.rst
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,26 @@ 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.
KBaichoo marked this conversation as resolved.
Show resolved Hide resolved

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
KBaichoo marked this conversation as resolved.
Show resolved Hide resolved
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
----------------------

Expand Down
18 changes: 14 additions & 4 deletions docs/root/intro/life_of_a_request.rst
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,11 @@ A brief outline of the life cycle of a request and response using the example co
:ref:`opposite order <arch_overview_http_filters_ordering>` 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.

Expand Down Expand Up @@ -533,8 +536,15 @@ directions during a request.
:ref:`Outlier detection <arch_overview_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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great docs!

``Router::Filter::onUpstreamComplete()``.

It is possible for a request to terminate early. This may be due to (but not limited to):
Expand Down
4 changes: 3 additions & 1 deletion envoy/http/stream_reset_handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
KBaichoo marked this conversation as resolved.
Show resolved Hide resolved
};

/**
Expand Down
15 changes: 8 additions & 7 deletions source/common/http/async_client_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -149,13 +149,14 @@ 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.
//
// 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);
}

Expand Down
6 changes: 4 additions & 2 deletions source/common/http/conn_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
Expand Down
14 changes: 13 additions & 1 deletion source/common/http/conn_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
OptRef<const Tracing::Config> tracingConfig() const override;
const ScopeTrackedObject& scope() override;
OptRef<DownstreamStreamFilterCallbacks> downstreamCallbacks() override { return *this; }
bool isHalfCloseEnabled() override { return false; }
bool isHalfCloseEnabled() override { return connection_manager_.allow_upstream_half_close_; }

// DownstreamStreamFilterCallbacks
void setRoute(Router::RouteConstSharedPtr route) override;
Expand Down Expand Up @@ -640,6 +640,18 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
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_{};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have half close behavior commented anywhere? comment link to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added docs and more comments.

};

} // namespace Http
Expand Down
1 change: 1 addition & 0 deletions source/common/http/conn_pool_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ void MultiplexedActiveClientBase::onStreamReset(Http::StreamResetReason reason)
case StreamResetReason::RemoteRefusedStreamReset:
case StreamResetReason::Overflow:
case StreamResetReason::ConnectError:
case StreamResetReason::Http1PrematureUpstreamHalfClose:
break;
}
}
Expand Down
Loading