Refactor Upstream API for setting TCP tunneling mode (envoyproxy#35510)

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]>

envoy/router/router.h

@@ -1544,15 +1544,13 @@ class GenericUpstream {
   virtual void encodeTrailers(const Http::RequestTrailerMap& trailers) PURE;
 
   // TODO(vikaschoudhary16): Remove this api.
-  // This api is only used to enable half-close semantics on the upstream connection.
-  // This ideally should be done via calling connection.enableHalfClose() but since TcpProxy
-  // does not have access to the upstream connection, it is done via this api for now.
+  // This api is only used to enable TCP tunneling semantics in the upstream codec.
+  // TCP proxy extension uses this API when proxyingn TCP tunnel via HTTP CONNECT or POST.
   /**
-   * Enable half-close semantics on the upstream connection. Reading a remote half-close
+   * Enable TCP tunneling semantics on the upstream codec. Reading a remote half-close
    * 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.
    */

source/common/router/upstream_request.cc

@@ -596,9 +596,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_) {
-    upstream_->enableHalfClose();
-  }
 
   host->outlierDetector().putResult(Upstream::Outlier::Result::LocalOriginConnectSuccess);
 

source/common/tcp_proxy/upstream.h

@@ -59,6 +59,23 @@ class TcpConnPool : public GenericConnPool, public Tcp::ConnectionPool::Callback
 class HttpUpstream;
 class CombinedUpstream;
 
+// This class is specific to TCP proxy connection pool and enables TCP proxying mode
+// for HTTP upstreams. This is currently only needed for HTTP/1 client codec that half closes
+// upstream network connection after encoding end_stream in TCP proxy (i.e. via CONNECT).
+class RouterUpstreamRequest : public Router::UpstreamRequest {
+public:
+  using Router::UpstreamRequest::UpstreamRequest;
+
+  void onPoolReady(std::unique_ptr<Router::GenericUpstream>&& upstream,
+                   Upstream::HostDescriptionConstSharedPtr host,
+                   const Network::ConnectionInfoProvider& address_provider,
+                   StreamInfo::StreamInfo& info, absl::optional<Http::Protocol> 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 +84,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<RouterUpstreamRequest>;
   ~HttpConnPool() override;
 

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); }
 

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 {}

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_; }
 

test/extensions/upstreams/http/udp/upstream_request_test.cc

@@ -46,7 +46,7 @@ class UdpUpstreamTest : public ::testing::Test {
     udp_upstream_ =
         std::make_unique<UdpUpstream>(&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: