Refactor Upstream API for setting TCP tunneling mode #35510
Conversation
Signed-off-by: Yan Avlasov <[email protected]>
| @@ -598,9 +598,6 @@ void UpstreamRequest::onPoolReady(std::unique_ptr<GenericUpstream>&& upstream, | |||
| had_upstream_ = true; | |||
| // Have the upstream use the account of the downstream. | |||
| upstream_->setAccount(parent_.callbacks()->account()); | |||
| if (enable_half_close_) { | |||
There was a problem hiding this comment.
This doesn't look like an API rename, it looks like a refactor.
If we're not using enable half close to call enable half close what's it used for?
should it have a rename?
There was a problem hiding this comment.
The enable_half_close_ value is plumbed through the FilterManagerCallbacks::isHalfCloseEnabled() to the filter manager, which determines if local_complete_ is set on encoder end_stream for the upstream filter manager.
The GenericUpstream::enableHalfClose() is setting the TCP proxying mode in H/1 codec which controls how codec behaves when it sees decoder end stream. This code used the half-close flag here to also set the TCP proxying mode, which worked before since the half-close mode was only used in TCP proxy mode.
In #34461 I'm going to enable half-close mode in other cases, so the TCP proxy settings needs to be separate from half-close. So I have moved setting the TCP proxy mode to the dedicated RouterUpstreamRequest class for TCP proxy upstreams.
I have updated PR description accordingly.
yanavlasov
changed the title
| @@ -59,6 +59,20 @@ class TcpConnPool : public GenericConnPool, public Tcp::ConnectionPool::Callback | |||
| class HttpUpstream; | |||
| class CombinedUpstream; | |||
|
|
|||
| class RouterUpstreamRequest : public Router::UpstreamRequest { | |||
Signed-off-by: Yan Avlasov <[email protected]>
This API was introduced by envoyproxy#32991 and it conflates support for half-close with TCP proxying mode in H/1 codec. This interferes with envoyproxy#34461 which enables support for server half-close in H/2 proxying. This PR refactors API to separate enabling of the half-close behavior from TCP proxy behavior. Prior to envoyproxy#34461 half-close semativs were only enabled for TCP proxy upstreams, but envoyproxy#34461 introduces other cases as well. This necessitates setting half-close and TCP proxy modes separately. TCP proxy mode is now enabled in the class dedicated to the TCP proxy upstreams. --------- Signed-off-by: Yan Avlasov <[email protected]> Signed-off-by: Martin Duke <[email protected]>
This API was introduced by envoyproxy#32991 and it conflates support for half-close with TCP proxying mode in H/1 codec. This interferes with envoyproxy#34461 which enables support for server half-close in H/2 proxying. This PR refactors API to separate enabling of the half-close behavior from TCP proxy behavior. Prior to envoyproxy#34461 half-close semativs were only enabled for TCP proxy upstreams, but envoyproxy#34461 introduces other cases as well. This necessitates setting half-close and TCP proxy modes separately. TCP proxy mode is now enabled in the class dedicated to the TCP proxy upstreams. --------- Signed-off-by: Yan Avlasov <[email protected]> Signed-off-by: asingh-g <[email protected]>
Commit message:
This API was introduced by #32991 and it conflates support for half-close with TCP proxying mode in H/1 codec. This interferes with #34461 which enables support for server half-close in H/2 proxying.
This PR refactors API to separate enabling of the half-close behavior from TCP proxy behavior. Prior to #34461 half-close semativs were only enabled for TCP proxy upstreams, but #34461 introduces other cases as well. This necessitates setting half-close and TCP proxy modes separately. TCP proxy mode is now enabled in the class dedicated to the TCP proxy upstreams.
Risk Level: Low
Testing: Unit Tests (with runtime flag enabled)
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A