tcp_tunneling: fix half close on upstream trailers (envoyproxy#32325)

When TCP tunneling, if the HTTP upstream is sending trailers, the downstream doesn't get any indication of this. To support half close semantics, fixing this with runtime guard so that FIN is sent downstream in such cases.
Risk Level: medium
Testing: unit tests, integration tests
Docs Changes: none
Release Notes: none
Platform Specific Features: none
Runtime guard: ``envoy.reloadable_features.tcp_tunneling_send_downstream_fin_on_upstream_trailers``

Signed-off-by: ohadvano <[email protected]>

Author: ohadvano

changelogs/current.yaml

@@ -85,6 +85,12 @@ bug_fixes:
     Fix crash due to uncaught exception when the operating system does not support an address type (such as IPv6) that is
     received in an mTLS client cert IP SAN. These SANs will be ignored. This applies only when using formatter
     ``%DOWNSTREAM_PEER_IP_SAN%``.
+- area: tcp_proxy
+  change: |
+    When tunneling TCP over HTTP, closed the downstream connection (for writing only) when upstream trailers are read
+    to support half close semantics during TCP tunneling.
+    This behavioral change can be temporarily reverted by setting runtime guard
+    ``envoy.reloadable_features.tcp_tunneling_send_downstream_fin_on_upstream_trailers`` to false.
 - area: jwt_authn
   change: |
     Fixed JWT extractor, which concatenated headers with a comma, resultig in invalid tokens.

source/common/runtime/runtime_features.cc

@@ -87,6 +87,7 @@ RUNTIME_GUARD(envoy_reloadable_features_skip_dns_lookup_for_proxied_requests);
 RUNTIME_GUARD(envoy_reloadable_features_ssl_transport_failure_reason_format);
 RUNTIME_GUARD(envoy_reloadable_features_stateful_session_encode_ttl_in_cookie);
 RUNTIME_GUARD(envoy_reloadable_features_stop_decode_metadata_on_local_reply);
+RUNTIME_GUARD(envoy_reloadable_features_tcp_tunneling_send_downstream_fin_on_upstream_trailers);
 RUNTIME_GUARD(envoy_reloadable_features_test_feature_true);
 RUNTIME_GUARD(envoy_reloadable_features_thrift_allow_negative_field_ids);
 RUNTIME_GUARD(envoy_reloadable_features_thrift_connection_draining);

source/common/tcp_proxy/upstream.h

@@ -8,6 +8,7 @@
 #include "envoy/upstream/thread_local_cluster.h"
 #include "envoy/upstream/upstream.h"
 
+#include "source/common/buffer/buffer_impl.h"
 #include "source/common/common/dump_state_utils.h"
 #include "source/common/http/codec_client.h"
 #include "source/common/router/header_parser.h"
@@ -196,6 +197,12 @@ class HttpUpstream : public GenericUpstream, protected Http::StreamCallbacks {
     void decodeTrailers(Http::ResponseTrailerMapPtr&& trailers) override {
       parent_.config_.propagateResponseTrailers(std::move(trailers),
                                                 parent_.downstream_info_.filterState());
+      if (Runtime::runtimeFeatureEnabled(
+              "envoy.reloadable_features.tcp_tunneling_send_downstream_fin_on_upstream_trailers")) {
+        Buffer::OwnedImpl data;
+        parent_.upstream_callbacks_.onUpstreamData(data, /* end_stream = */ true);
+      }
+
       parent_.doneReading();
     }
     void decodeMetadata(Http::MetadataMapPtr&&) override {}

test/common/tcp_proxy/upstream_test.cc

@@ -201,6 +201,28 @@ TEST_P(HttpUpstreamTest, UpstreamTrailersMarksDoneReading) {
   this->upstream_->responseDecoder().decodeTrailers(std::move(trailers));
 }
 
+TEST_P(HttpUpstreamTest, UpstreamTrailersPropagateFinDownstream) {
+  setupUpstream();
+  EXPECT_CALL(encoder_.stream_, resetStream(_)).Times(0);
+  upstream_->doneWriting();
+  EXPECT_CALL(callbacks_, onUpstreamData(BufferStringEqual(""), true));
+  Http::ResponseTrailerMapPtr trailers{new Http::TestResponseTrailerMapImpl{{"key", "value"}}};
+  upstream_->responseDecoder().decodeTrailers(std::move(trailers));
+}
+
+TEST_P(HttpUpstreamTest, UpstreamTrailersDontPropagateFinDownstreamWhenFeatureDisabled) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues(
+      {{"envoy.reloadable_features.tcp_tunneling_send_downstream_fin_on_upstream_trailers",
+        "false"}});
+  setupUpstream();
+  EXPECT_CALL(encoder_.stream_, resetStream(_)).Times(0);
+  upstream_->doneWriting();
+  EXPECT_CALL(callbacks_, onUpstreamData(_, _)).Times(0);
+  Http::ResponseTrailerMapPtr trailers{new Http::TestResponseTrailerMapImpl{{"key", "value"}}};
+  upstream_->responseDecoder().decodeTrailers(std::move(trailers));
+}
+
 class HttpUpstreamRequestEncoderTest : public testing::TestWithParam<Http::CodecType> {
 public:
   HttpUpstreamRequestEncoderTest() {

test/integration/tcp_tunneling_integration_test.cc

@@ -1313,6 +1313,34 @@ TEST_P(TcpTunnelingIntegrationTest, CopyResponseTrailers) {
   EXPECT_THAT(waitForAccessLog(access_log_filename), testing::HasSubstr(trailer_value));
 }
 
+TEST_P(TcpTunnelingIntegrationTest, DownstreamFinOnUpstreamTrailers) {
+  if (upstreamProtocol() == Http::CodecType::HTTP1) {
+    return;
+  }
+
+  initialize();
+
+  tcp_client_ = makeTcpConnection(lookupPort("tcp_proxy"));
+  ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_));
+  ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_));
+  ASSERT_TRUE(upstream_request_->waitForHeadersComplete());
+
+  upstream_request_->encodeHeaders(default_response_headers_, false);
+  sendBidiData(fake_upstream_connection_);
+
+  // Send trailers
+  const std::string trailer_value = "trailer-value";
+  Http::TestResponseTrailerMapImpl response_trailers{{"test-trailer-name", trailer_value}};
+  upstream_request_->encodeTrailers(response_trailers);
+
+  // Upstream trailers should close the downstream connection for writing.
+  tcp_client_->waitForHalfClose();
+
+  // Close Connection
+  tcp_client_->close();
+  ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_));
+}
+
 TEST_P(TcpTunnelingIntegrationTest, CloseUpstreamFirst) {
   initialize();