Revert "upstream: enhance upstream filter state (envoyproxy#25917)" (e…

…nvoyproxy#26880)

This reverts commit dd39b3a.

Signed-off-by: Ryan Northey <[email protected]>

envoy/stream_info/BUILD

@@ -32,10 +32,7 @@ envoy_cc_library(
 envoy_cc_library(
     name = "filter_state_interface",
     hdrs = ["filter_state.h"],
-    external_deps = [
-        "abseil_optional",
-        "abseil_status",
-    ],
+    external_deps = ["abseil_optional"],
     deps = [
         "//source/common/common:utility_lib",
         "//source/common/protobuf",

envoy/stream_info/filter_state.h

@@ -9,7 +9,6 @@
 #include "source/common/common/utility.h"
 #include "source/common/protobuf/protobuf.h"
 
-#include "absl/status/status.h"
 #include "absl/strings/string_view.h"
 #include "absl/types/optional.h"
 
@@ -116,8 +115,6 @@ class FilterState {
 
   virtual ~FilterState() = default;
 
-  virtual std::string serializeAsString(int indent_level = 0) const PURE;
-
   /**
    * @param data_name the name of the data being set.
    * @param data an owning pointer to the data to be stored.
@@ -207,17 +204,6 @@ class FilterState {
    */
   virtual FilterStateSharedPtr parent() const PURE;
 
-  /**
-   * @param ancestor the longer-lifetime filter state which should be added as an ancestor to this
-   * instance.
-   * @return success status.
-   *
-   * Succeeds iff:
-   * 1. ancestor.lifeSpan() > this->lifeSpan() along with all parents of this.
-   * 2. this and ancestor share no keys.
-   */
-  virtual absl::Status addAncestor(const FilterStateSharedPtr& ancestor) PURE;
-
   /**
    * @return filter objects that are shared with the upstream connection.
    **/

source/common/network/connection_impl.cc

@@ -885,8 +885,7 @@ ClientConnectionImpl::ClientConnectionImpl(
     const Network::TransportSocketOptionsConstSharedPtr& transport_options)
     : ConnectionImpl(dispatcher, std::move(socket), std::move(transport_socket), stream_info_,
                      false),
-      stream_info_(dispatcher_.timeSource(), socket_->connectionInfoProviderSharedPtr(),
-                   StreamInfo::FilterState::LifeSpan::Connection) {
+      stream_info_(dispatcher_.timeSource(), socket_->connectionInfoProviderSharedPtr()) {
 
   stream_info_.setUpstreamInfo(std::make_shared<StreamInfo::UpstreamInfoImpl>());
 

source/common/quic/envoy_quic_client_session.cc

@@ -57,8 +57,7 @@ EnvoyQuicClientSession::EnvoyQuicClientSession(
           std::make_shared<QuicSslConnectionInfo>(*this),
           std::make_unique<StreamInfo::StreamInfoImpl>(
               dispatcher.timeSource(),
-              connection->connectionSocket()->connectionInfoProviderSharedPtr(),
-              StreamInfo::FilterState::LifeSpan::Connection)),
+              connection->connectionSocket()->connectionInfoProviderSharedPtr())),
       quic::QuicSpdyClientSession(config, supported_versions, connection.release(), server_id,
                                   crypto_config.get(), push_promise_index),
       crypto_config_(crypto_config), crypto_stream_factory_(crypto_stream_factory),

source/common/router/upstream_request.cc

@@ -110,9 +110,6 @@ UpstreamRequest::UpstreamRequest(RouterFilterInterface& parent,
     }
   }
   stream_info_.setUpstreamInfo(std::make_shared<StreamInfo::UpstreamInfoImpl>());
-  // Upstream filters may need to set filter state.
-  stream_info_.upstreamInfo()->setUpstreamFilterState(std::make_shared<StreamInfo::FilterStateImpl>(
-      StreamInfo::FilterState::LifeSpan::FilterChain));
   stream_info_.route_ = parent.callbacks()->route();
   parent_.callbacks()->streamInfo().setUpstreamInfo(stream_info_.upstreamInfo());
 
@@ -634,7 +631,7 @@ void UpstreamRequest::onPoolFailure(ConnectionPool::PoolFailureReason reason,
 void UpstreamRequest::onPoolReady(std::unique_ptr<GenericUpstream>&& upstream,
                                   Upstream::HostDescriptionConstSharedPtr host,
                                   const Network::ConnectionInfoProvider& address_provider,
-                                  StreamInfo::StreamInfo& connection_stream_info,
+                                  StreamInfo::StreamInfo& info,
                                   absl::optional<Http::Protocol> protocol) {
   // This may be called under an existing ScopeTrackerScopeState but it will unwind correctly.
   ScopeTrackerScopeState scope(&parent_.callbacks()->scope(), parent_.callbacks()->dispatcher());
@@ -670,35 +667,33 @@ void UpstreamRequest::onPoolReady(std::unique_ptr<GenericUpstream>&& upstream,
   }
 
   StreamInfo::UpstreamInfo& upstream_info = *stream_info_.upstreamInfo();
-  if (connection_stream_info.upstreamInfo()) {
-    auto& upstream_timing = connection_stream_info.upstreamInfo()->upstreamTiming();
+  if (info.upstreamInfo()) {
+    auto& upstream_timing = info.upstreamInfo()->upstreamTiming();
     upstreamTiming().upstream_connect_start_ = upstream_timing.upstream_connect_start_;
     upstreamTiming().upstream_connect_complete_ = upstream_timing.upstream_connect_complete_;
     upstreamTiming().upstream_handshake_complete_ = upstream_timing.upstream_handshake_complete_;
-    upstream_info.setUpstreamNumStreams(
-        connection_stream_info.upstreamInfo()->upstreamNumStreams());
+    upstream_info.setUpstreamNumStreams(info.upstreamInfo()->upstreamNumStreams());
   }
 
-  // The upstream connection may have filter state created by upstream network filters. This
-  // connection filter state must be an ancestor of the FilterChain upstream filter state.
-  auto filter_state_status =
-      upstream_info.upstreamFilterState()->addAncestor(connection_stream_info.filterState());
-  ENVOY_BUG(filter_state_status.ok(),
-            absl::StrCat("Failed to add connection filter state: ", filter_state_status.message()));
-
+  // Upstream filters might have already created/set a filter state.
+  const StreamInfo::FilterStateSharedPtr& filter_state = info.filterState();
+  if (!filter_state) {
+    upstream_info.setUpstreamFilterState(
+        std::make_shared<StreamInfo::FilterStateImpl>(StreamInfo::FilterState::LifeSpan::Request));
+  } else {
+    upstream_info.setUpstreamFilterState(filter_state);
+  }
   upstream_info.setUpstreamLocalAddress(address_provider.localAddress());
   upstream_info.setUpstreamRemoteAddress(address_provider.remoteAddress());
-  upstream_info.setUpstreamSslConnection(
-      connection_stream_info.downstreamAddressProvider().sslConnection());
+  upstream_info.setUpstreamSslConnection(info.downstreamAddressProvider().sslConnection());
 
-  if (connection_stream_info.downstreamAddressProvider().connectionID().has_value()) {
-    upstream_info.setUpstreamConnectionId(
-        connection_stream_info.downstreamAddressProvider().connectionID().value());
+  if (info.downstreamAddressProvider().connectionID().has_value()) {
+    upstream_info.setUpstreamConnectionId(info.downstreamAddressProvider().connectionID().value());
   }
 
-  if (connection_stream_info.downstreamAddressProvider().interfaceName().has_value()) {
+  if (info.downstreamAddressProvider().interfaceName().has_value()) {
     upstream_info.setUpstreamInterfaceName(
-        connection_stream_info.downstreamAddressProvider().interfaceName().value());
+        info.downstreamAddressProvider().interfaceName().value());
   }
 
   stream_info_.setUpstreamBytesMeter(upstream_->bytesMeter());

source/common/stream_info/filter_state_impl.cc

@@ -1,28 +1,10 @@
-#include "filter_state_impl.h"
 #include "source/common/stream_info/filter_state_impl.h"
 
 #include "envoy/common/exception.h"
 
 namespace Envoy {
 namespace StreamInfo {
 
-std::string FilterStateImpl::serializeAsString(int indent_level) const {
-  std::string out = "";
-  std::string base_indent(2 * indent_level, ' ');
-  absl::StrAppend(&out, base_indent, "At life span ", life_span_, "\n");
-  const std::string indent = "  ";
-  for (const auto& [name, object] : data_storage_) {
-    const auto obj_ser = object->data_->serializeAsString();
-    absl::StrAppend(&out, base_indent, indent, name, ": ", obj_ser.value_or(""), "\n", base_indent,
-                    indent, indent, "shared: ", object->stream_sharing_, "\n", base_indent, indent,
-                    indent, "readOnly: ", object->state_type_ == FilterState::StateType::ReadOnly,
-                    "\n");
-  }
-  std::string parent_value = parent_ ? parent_->serializeAsString() : "";
-  absl::StrAppend(&out, parent_value);
-  return out;
-}
-
 void FilterStateImpl::setData(absl::string_view data_name, std::shared_ptr<Object> data,
                               FilterState::StateType state_type, FilterState::LifeSpan life_span,
                               StreamSharingMayImpactPooling stream_sharing) {
@@ -150,8 +132,7 @@ void FilterStateImpl::maybeCreateParent(ParentAccessMode parent_access_mode) {
   }
   if (absl::holds_alternative<FilterStateSharedPtr>(ancestor_)) {
     FilterStateSharedPtr ancestor = absl::get<FilterStateSharedPtr>(ancestor_);
-    if ((ancestor == nullptr && parent_access_mode == ParentAccessMode::ReadWrite) ||
-        (ancestor != nullptr && ancestor->lifeSpan() != life_span_ + 1)) {
+    if (ancestor == nullptr || ancestor->lifeSpan() != life_span_ + 1) {
       parent_ = std::make_shared<FilterStateImpl>(ancestor, FilterState::LifeSpan(life_span_ + 1));
     } else {
       parent_ = ancestor;
@@ -180,39 +161,5 @@ void FilterStateImpl::maybeCreateParent(ParentAccessMode parent_access_mode) {
   parent_ = lazy_create_ancestor.first;
 }
 
-absl::Status FilterStateImpl::addAncestor(const FilterStateSharedPtr& ancestor) {
-  if (!ancestor) {
-    return absl::OkStatus();
-  }
-  // Make sure that the ancestor has an acceptable life span.
-  if (ancestor->lifeSpan() <= life_span_) {
-    return absl::FailedPreconditionError(absl::StrCat("Ancestor lifespan ", ancestor->lifeSpan(),
-                                                      " must be larger than current lifespan ",
-                                                      life_span_));
-  }
-  // Make sure there's no data conflicts between `this` and the proposed ancestor.
-  for (const auto& [key, _] : data_storage_) {
-    if (ancestor->hasDataWithName(key)) {
-      return absl::FailedPreconditionError(absl::StrCat("Ancestor and this share a key: ", key));
-    }
-  }
-  // Add the ancestor to the pre-existing parent/ancestor.
-  if (parent_) {
-    if (parent_.get() == ancestor.get()) {
-      return absl::OkStatus();
-    }
-    auto status = parent_->addAncestor(ancestor);
-    if (status.ok()) {
-      return status;
-    }
-    return absl::FailedPreconditionError(
-        absl::StrCat("Could not add ancestor to pre-existing ancestor: ", status.message()));
-  }
-  // There are no pre-existing parents/ancestors, so we add it here.
-  ancestor_ = ancestor;
-  maybeCreateParent(ParentAccessMode::ReadOnly);
-  return absl::OkStatus();
-}
-
 } // namespace StreamInfo
 } // namespace Envoy

source/common/stream_info/filter_state_impl.h

@@ -39,8 +39,6 @@ class FilterStateImpl : public FilterState {
     maybeCreateParent(ParentAccessMode::ReadOnly);
   }
 
-  std::string serializeAsString(int indent_level = 0) const override;
-
   // FilterState
   void setData(
       absl::string_view data_name, std::shared_ptr<Object> data, FilterState::StateType state_type,
@@ -51,7 +49,6 @@ class FilterStateImpl : public FilterState {
   Object* getDataMutableGeneric(absl::string_view data_name) override;
   std::shared_ptr<Object> getDataSharedMutableGeneric(absl::string_view data_name) override;
   bool hasDataAtOrAboveLifeSpan(FilterState::LifeSpan life_span) const override;
-  absl::Status addAncestor(const FilterStateSharedPtr& ancestor) override;
   FilterState::ObjectsPtr objectsSharedWithUpstreamConnection() const override;
 
   FilterState::LifeSpan lifeSpan() const override { return life_span_; }

source/common/stream_info/stream_info_impl.h

@@ -284,8 +284,7 @@ struct StreamInfoImpl : public StreamInfo {
     os << spaces << "StreamInfoImpl " << this << DUMP_OPTIONAL_MEMBER(protocol_)
        << DUMP_OPTIONAL_MEMBER(response_code_) << DUMP_OPTIONAL_MEMBER(response_code_details_)
        << DUMP_OPTIONAL_MEMBER(attempt_count_) << DUMP_MEMBER(health_check_request_)
-       << DUMP_MEMBER(route_name_)
-       << DUMP_MEMBER_AS(filter_state_, filter_state_->serializeAsString(indent_level));
+       << DUMP_MEMBER(route_name_);
     DUMP_DETAILS(upstream_info_);
   }
 

source/extensions/filters/http/common/factory_base.h

@@ -78,27 +78,6 @@ class FactoryBase : public CommonFactoryBase<ConfigProto, RouteConfigProto>,
                                     Server::Configuration::FactoryContext& context) PURE;
 };
 
-template <class ConfigProto, class RouteConfigProto = ConfigProto>
-class UpstreamFactoryBase : public CommonFactoryBase<ConfigProto, RouteConfigProto>,
-                            public Server::Configuration::UpstreamHttpFilterConfigFactory {
-public:
-  UpstreamFactoryBase(const std::string& name)
-      : CommonFactoryBase<ConfigProto, RouteConfigProto>(name) {}
-
-  Envoy::Http::FilterFactoryCb createFilterFactoryFromProto(
-      const Protobuf::Message& proto_config, const std::string& stats_prefix,
-      Server::Configuration::UpstreamHttpFactoryContext& context) override {
-    return createFilterFactoryFromProtoTyped(
-        MessageUtil::downcastAndValidate<const ConfigProto&>(
-            proto_config, context.getServerFactoryContext().messageValidationVisitor()),
-        stats_prefix, context);
-  }
-
-  virtual Envoy::Http::FilterFactoryCb createFilterFactoryFromProtoTyped(
-      const ConfigProto& proto_config, const std::string& stats_prefix,
-      Server::Configuration::UpstreamHttpFactoryContext& context) PURE;
-};
-
 template <class ConfigProto, class RouteConfigProto = ConfigProto>
 class DualFactoryBase : public CommonFactoryBase<ConfigProto, RouteConfigProto>,
                         public Server::Configuration::NamedHttpFilterConfigFactory,