Revert "Support upstream http filters with tcp tunneling (envoyproxy#…

…27183)" (envoyproxy#32980) This reverts commit 50e9108. Signed-off-by: Alyssa Wilk <[email protected]>
RyanTheOptimist · Mar 19, 2024 · 04835af · 04835af
1 parent 8365ca8
commit 04835af
Show file tree

Hide file tree

Showing 47 changed files with 193 additions and 1,185 deletions.
diff --git a/envoy/router/router.h b/envoy/router/router.h
@@ -1526,17 +1526,6 @@ class GenericUpstream {
    * @param trailers supplies the trailers to encode.
    */
   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.
-  /**
-   * Enable half-close semantics on the upstream connection. 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;
   /**
    * Enable/disable further data from this stream.
    */

diff --git a/envoy/tcp/BUILD b/envoy/tcp/BUILD
@@ -26,7 +26,6 @@ envoy_cc_library(
         "//envoy/http:header_evaluator",
         "//envoy/tcp:conn_pool_interface",
         "//envoy/upstream:upstream_interface",
-        "//source/common/router:router_lib",
         "@envoy_api//envoy/extensions/filters/network/tcp_proxy/v3:pkg_cc_proto",
     ],
 )

diff --git a/envoy/tcp/upstream.h b/envoy/tcp/upstream.h
@@ -2,14 +2,11 @@
 
 #include "envoy/buffer/buffer.h"
 #include "envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.pb.h"
-#include "envoy/http/filter.h"
 #include "envoy/http/header_evaluator.h"
 #include "envoy/stream_info/stream_info.h"
 #include "envoy/tcp/conn_pool.h"
 #include "envoy/upstream/upstream.h"
 
-#include "source/common/router/router.h"
-
 namespace Envoy {
 
 namespace Upstream {
@@ -51,17 +48,14 @@ class TunnelingConfigHelper {
   virtual void
   propagateResponseTrailers(Http::ResponseTrailerMapPtr&& trailers,
                             const StreamInfo::FilterStateSharedPtr& filter_state) const PURE;
-  virtual const Envoy::Router::FilterConfig& routerFilterConfig() const PURE;
-  virtual Server::Configuration::ServerFactoryContext& serverFactoryContext() const PURE;
 };
 
 using TunnelingConfigHelperOptConstRef = OptRef<const TunnelingConfigHelper>;
 
 // An API for wrapping either a TCP or an HTTP connection pool.
-class GenericConnPool : public Event::DeferredDeletable,
-                        public Logger::Loggable<Logger::Id::router> {
+class GenericConnPool : public Logger::Loggable<Logger::Id::router> {
 public:
-  ~GenericConnPool() override = default;
+  virtual ~GenericConnPool() = default;
 
   /**
    * Called to create a TCP connection or HTTP stream for "CONNECT" streams.
@@ -111,9 +105,9 @@ class GenericConnectionPoolCallbacks {
 
 // Interface for a generic Upstream, which can communicate with a TCP or HTTP
 // upstream.
-class GenericUpstream : public Event::DeferredDeletable {
+class GenericUpstream {
 public:
-  ~GenericUpstream() override = default;
+  virtual ~GenericUpstream() = default;
 
   /**
    * Enable/disable further data from this stream.
@@ -181,7 +175,6 @@ class GenericConnPoolFactory : public Envoy::Config::TypedFactory {
                         TunnelingConfigHelperOptConstRef config,
                         Upstream::LoadBalancerContext* context,
                         Tcp::ConnectionPool::UpstreamCallbacks& upstream_callbacks,
-                        Http::StreamDecoderFilterCallbacks& stream_decoder_callbacks,
                         StreamInfo::StreamInfo& downstream_info) const PURE;
 };
 

diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h
@@ -314,7 +314,6 @@ 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; }
 
     // DownstreamStreamFilterCallbacks
     void setRoute(Router::RouteConstSharedPtr route) override;

diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc
@@ -901,17 +901,9 @@ 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());
   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;
-    }
+    ASSERT(!state_.local_complete_);
+    state_.local_complete_ = end_stream;
     return encoder_filters_.begin();
   }
 

diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h
@@ -545,11 +545,6 @@ class FilterManagerCallbacks {
    * Returns the DownstreamStreamFilterCallbacks for downstream HTTP filters.
    */
   virtual OptRef<DownstreamStreamFilterCallbacks> downstreamCallbacks() { return {}; }
-  /**
-   * Returns if close from the upstream is to be handled with half-close semantics.
-   * This is used for HTTP/1.1 codec.
-   */
-  virtual bool isHalfCloseEnabled() PURE;
 };
 
 /**

diff --git a/source/common/router/router.cc b/source/common/router/router.cc
@@ -744,9 +744,8 @@ 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<UpstreamRequest>(*this, std::move(generic_conn_pool), can_send_early_data,
-                                        can_use_http3, false /*enable_half_close*/);
+  UpstreamRequestPtr upstream_request = std::make_unique<UpstreamRequest>(
+      *this, std::move(generic_conn_pool), can_send_early_data, can_use_http3);
   LinkedList::moveIntoList(std::move(upstream_request), upstream_requests_);
   upstream_requests_.front()->acceptHeadersFromRouter(end_stream);
   if (streaming_shadows_) {
@@ -1988,9 +1987,8 @@ void Filter::doRetry(bool can_send_early_data, bool can_use_http3, TimeoutRetry
     cleanup();
     return;
   }
-  UpstreamRequestPtr upstream_request =
-      std::make_unique<UpstreamRequest>(*this, std::move(generic_conn_pool), can_send_early_data,
-                                        can_use_http3, false /*enable_tcp_tunneling*/);
+  UpstreamRequestPtr upstream_request = std::make_unique<UpstreamRequest>(
+      *this, std::move(generic_conn_pool), can_send_early_data, can_use_http3);
 
   if (include_attempt_count_in_request_) {
     downstream_headers_->setEnvoyAttemptCount(attempt_count_);

diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc
@@ -80,8 +80,7 @@ class UpstreamFilterManager : public Http::FilterManager {
 
 UpstreamRequest::UpstreamRequest(RouterFilterInterface& parent,
                                  std::unique_ptr<GenericConnPool>&& conn_pool,
-                                 bool can_send_early_data, bool can_use_http3,
-                                 bool enable_half_close)
+                                 bool can_send_early_data, bool can_use_http3)
     : parent_(parent), conn_pool_(std::move(conn_pool)),
       stream_info_(parent_.callbacks()->dispatcher().timeSource(), nullptr),
       start_time_(parent_.callbacks()->dispatcher().timeSource().monotonicTime()),
@@ -94,8 +93,7 @@ UpstreamRequest::UpstreamRequest(RouterFilterInterface& parent,
       cleaned_up_(false), had_upstream_(false),
       stream_options_({can_send_early_data, can_use_http3}), grpc_rq_success_deferred_(false),
       upstream_wait_for_response_headers_before_disabling_read_(Runtime::runtimeFeatureEnabled(
-          "envoy.reloadable_features.upstream_wait_for_response_headers_before_disabling_read")),
-      enable_half_close_(enable_half_close) {
+          "envoy.reloadable_features.upstream_wait_for_response_headers_before_disabling_read")) {
   if (auto tracing_config = parent_.callbacks()->tracingConfig(); tracing_config.has_value()) {
     if (tracing_config->spawnUpstreamSpan() || parent_.config().start_child_span_) {
       span_ = parent_.callbacks()->activeSpan().spawnChild(
@@ -260,8 +258,7 @@ void UpstreamRequest::decode1xxHeaders(Http::ResponseHeaderMapPtr&& headers) {
 // on to the router.
 void UpstreamRequest::decodeHeaders(Http::ResponseHeaderMapPtr&& headers, bool end_stream) {
   ASSERT(headers.get());
-  ENVOY_STREAM_LOG(trace, "end_stream: {}, upstream response headers:\n{}", *parent_.callbacks(),
-                   end_stream, *headers);
+  ENVOY_STREAM_LOG(trace, "upstream response headers:\n{}", *parent_.callbacks(), *headers);
   ScopeTrackerScopeState scope(&parent_.callbacks()->scope(), parent_.callbacks()->dispatcher());
 
   resetPerTryIdleTimer();
@@ -584,9 +581,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);
 

diff --git a/source/common/router/upstream_request.h b/source/common/router/upstream_request.h
@@ -62,27 +62,29 @@ class UpstreamCodecFilter;
  * UpstreamCodecFilter. This is accomplished via the UpstreamStreamFilterCallbacks
  * interface, with the UpstreamFilterManager acting as intermediary.
  *
+ * UpstreamRequest is marked as final because no subclasses are expected.
+ * This class is intended to be used as-is without any specialized inheritance.
  */
-class UpstreamRequest : public Logger::Loggable<Logger::Id::router>,
-                        public UpstreamToDownstream,
-                        public LinkedObject<UpstreamRequest>,
-                        public GenericConnectionPoolCallbacks,
-                        public Event::DeferredDeletable {
+class UpstreamRequest final : public Logger::Loggable<Logger::Id::router>,
+                              public UpstreamToDownstream,
+                              public LinkedObject<UpstreamRequest>,
+                              public GenericConnectionPoolCallbacks,
+                              public Event::DeferredDeletable {
 public:
   UpstreamRequest(RouterFilterInterface& parent, std::unique_ptr<GenericConnPool>&& conn_pool,
-                  bool can_send_early_data, bool can_use_http3, bool enable_half_close);
+                  bool can_send_early_data, bool can_use_http3);
   ~UpstreamRequest() override;
   void deleteIsPending() override { cleanUp(); }
 
   // To be called from the destructor, or prior to deferred delete.
   void cleanUp();
 
-  virtual void acceptHeadersFromRouter(bool end_stream);
-  virtual void acceptDataFromRouter(Buffer::Instance& data, bool end_stream);
+  void acceptHeadersFromRouter(bool end_stream);
+  void acceptDataFromRouter(Buffer::Instance& data, bool end_stream);
   void acceptTrailersFromRouter(Http::RequestTrailerMap& trailers);
   void acceptMetadataFromRouter(Http::MetadataMapPtr&& metadata_map_ptr);
 
-  virtual void resetStream();
+  void resetStream();
   void setupPerTryTimeout();
   void maybeEndDecode(bool end_stream);
   void onUpstreamHostSelected(Upstream::HostDescriptionConstSharedPtr host, bool pool_success);
@@ -256,7 +258,6 @@ class UpstreamRequest : public Logger::Loggable<Logger::Id::router>,
   Http::ConnectionPool::Instance::StreamOptions stream_options_;
   bool grpc_rq_success_deferred_ : 1;
   bool upstream_wait_for_response_headers_before_disabling_read_ : 1;
-  bool enable_half_close_ : 1;
 };
 
 class UpstreamRequestFilterManagerCallbacks : public Http::FilterManagerCallbacks,
@@ -370,7 +371,7 @@ class UpstreamRequestFilterManagerCallbacks : public Http::FilterManagerCallback
   void setUpstreamToDownstream(UpstreamToDownstream& upstream_to_downstream_interface) override {
     upstream_request_.upstream_interface_ = upstream_to_downstream_interface;
   }
-  bool isHalfCloseEnabled() override { return upstream_request_.enable_half_close_; }
+
   Http::RequestTrailerMapPtr trailers_;
   Http::ResponseHeaderMapPtr informational_headers_;
   Http::ResponseHeaderMapPtr response_headers_;

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -130,9 +130,6 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_runtime_initialized);
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_always_use_v6);
 // TODO(wbpcode) complete remove this feature is no one use it.
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_refresh_rtt_after_request);
-// TODO(vikaschoudhary16) flip this to true only after all the
-// TcpProxy::Filter::HttpStreamDecoderFilterCallbacks are implemented or commented as unnecessary
-FALSE_RUNTIME_GUARD(envoy_restart_features_upstream_http_filters_with_tcp_proxy);
 // TODO(danzh) false deprecate it once QUICHE has its own enable/disable flag.
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_quic_reject_all);
 // TODO(suniltheta): Once the newly added http async technique is stabilized move it under

diff --git a/source/common/runtime/runtime_features.h b/source/common/runtime/runtime_features.h
@@ -26,8 +26,6 @@ void maybeSetRuntimeGuard(absl::string_view name, bool value);
 void maybeSetDeprecatedInts(absl::string_view name, uint32_t value);
 constexpr absl::string_view defer_processing_backedup_streams =
     "envoy.reloadable_features.defer_processing_backedup_streams";
-constexpr absl::string_view upstream_http_filters_with_tcp_proxy =
-    "envoy.restart_features.upstream_http_filters_with_tcp_proxy";
 
 } // namespace Runtime
 } // namespace Envoy
diff --git a/source/common/tcp_proxy/BUILD b/source/common/tcp_proxy/BUILD
@@ -17,22 +17,15 @@ envoy_cc_library(
         "upstream.h",
     ],
     deps = [
-        "//envoy/http:header_map_interface",
-        "//envoy/router:router_ratelimit_interface",
         "//envoy/tcp:conn_pool_interface",
         "//envoy/tcp:upstream_interface",
         "//envoy/upstream:cluster_manager_interface",
         "//envoy/upstream:load_balancer_interface",
-        "//source/common/http:async_client_lib",
         "//source/common/http:codec_client_lib",
-        "//source/common/http:hash_policy_lib",
         "//source/common/http:header_map_lib",
         "//source/common/http:headers_lib",
         "//source/common/http:utility_lib",
-        "//source/common/network:utility_lib",
         "//source/common/router:header_parser_lib",
-        "//source/common/router:router_lib",
-        "//source/common/router:shadow_writer_lib",
     ],
 )