Skip filter state update when composite filter is in upstream (envoyp…

…roxy#33540) This is to address issue: envoyproxy#33343 Signed-off-by: Yanjun Xiang <[email protected]>
zhxie · Apr 22, 2024 · 14b87a6 · 14b87a6
1 parent 31e9f55
commit 14b87a6
Show file tree

Hide file tree

Showing 5 changed files with 56 additions and 6 deletions.
diff --git a/docs/root/configuration/http/http_filters/composite_filter.rst b/docs/root/configuration/http/http_filters/composite_filter.rst
@@ -20,6 +20,7 @@ incremented.
 
 This filter adds a map of the delegated filter name (of the action that is matched )and the root config filter name to the filter state with key
 ``envoy.extensions.filters.http.composite.matched_actions``
+This filter state is not emitted when the filter is configured in the upstream filter chain.
 
 Contains a map of pairs `FILTER_CONFIG_NAME:ACTION_NAME`:
 

diff --git a/source/extensions/filters/http/composite/config.cc b/source/extensions/filters/http/composite/config.cc
@@ -19,8 +19,8 @@ absl::StatusOr<Http::FilterFactoryCb> CompositeFilterFactory::createFilterFactor
   auto stats = std::make_shared<FilterStats>(
       FilterStats{ALL_COMPOSITE_FILTER_STATS(POOL_COUNTER_PREFIX(dual_info.scope, prefix))});
 
-  return [stats](Http::FilterChainFactoryCallbacks& callbacks) -> void {
-    auto filter = std::make_shared<Filter>(*stats, callbacks.dispatcher());
+  return [stats, dual_info](Http::FilterChainFactoryCallbacks& callbacks) -> void {
+    auto filter = std::make_shared<Filter>(*stats, callbacks.dispatcher(), dual_info.is_upstream);
     callbacks.addStreamFilter(filter);
     callbacks.addAccessLogHandler(filter);
   };

diff --git a/source/extensions/filters/http/composite/filter.cc b/source/extensions/filters/http/composite/filter.cc
@@ -142,6 +142,9 @@ void Filter::onMatchCallback(const Matcher::Action& action) {
 
 void Filter::updateFilterState(Http::StreamFilterCallbacks* callback,
                                const std::string& filter_name, const std::string& action_name) {
+  if (isUpstream()) {
+    return;
+  }
   auto* info = callback->streamInfo().filterState()->getDataMutable<MatchedActionInfo>(
       MatchedActionsFilterStateKey);
   if (info != nullptr) {

diff --git a/source/extensions/filters/http/composite/filter.h b/source/extensions/filters/http/composite/filter.h
@@ -56,8 +56,9 @@ class Filter : public Http::StreamFilter,
                public AccessLog::Instance,
                Logger::Loggable<Logger::Id::filter> {
 public:
-  Filter(FilterStats& stats, Event::Dispatcher& dispatcher)
-      : dispatcher_(dispatcher), decoded_headers_(false), stats_(stats) {}
+  Filter(FilterStats& stats, Event::Dispatcher& dispatcher, bool is_upstream)
+      : dispatcher_(dispatcher), decoded_headers_(false), stats_(stats), is_upstream_(is_upstream) {
+  }
 
   // Http::StreamDecoderFilter
   Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers,
@@ -109,6 +110,8 @@ class Filter : public Http::StreamFilter,
     }
   }
 
+  bool isUpstream() const { return is_upstream_; }
+
 private:
   friend FactoryCallbacksWrapper;
 
@@ -165,6 +168,8 @@ class Filter : public Http::StreamFilter,
   Http::StreamEncoderFilterCallbacks* encoder_callbacks_{};
   Http::StreamDecoderFilterCallbacks* decoder_callbacks_{};
   FilterStats& stats_;
+  // Filter in the upstream filter chain.
+  bool is_upstream_;
 };
 
 } // namespace Composite

diff --git a/test/extensions/filters/http/composite/filter_test.cc b/test/extensions/filters/http/composite/filter_test.cc
@@ -20,9 +20,10 @@ namespace {
 
 using Envoy::Protobuf::util::MessageDifferencer;
 
-class FilterTest : public ::testing::Test {
+class CompositeFilterTest : public ::testing::Test {
 public:
-  FilterTest() : filter_(stats_, decoder_callbacks_.dispatcher()) {
+  CompositeFilterTest(bool is_upstream)
+      : filter_(stats_, decoder_callbacks_.dispatcher(), is_upstream) {
     filter_.setDecoderFilterCallbacks(decoder_callbacks_);
     filter_.setEncoderFilterCallbacks(encoder_callbacks_);
   }
@@ -100,6 +101,11 @@ class FilterTest : public ::testing::Test {
       {"response-trailer", "something-else"}};
 };
 
+class FilterTest : public CompositeFilterTest {
+public:
+  FilterTest() : CompositeFilterTest(false) {}
+};
+
 TEST_F(FilterTest, StreamEncoderFilterDelegation) {
   auto stream_filter = std::make_shared<Http::MockStreamEncoderFilter>();
   StreamInfo::FilterStateSharedPtr filter_state =
@@ -640,6 +646,41 @@ TEST_F(FilterTest, MatchingActionShouldNotCollitionWithOtherRootFilter) {
   filter_.onDestroy();
 }
 
+class UpstreamFilterTest : public CompositeFilterTest {
+public:
+  UpstreamFilterTest() : CompositeFilterTest(true) {}
+};
+
+TEST_F(UpstreamFilterTest, StreamEncoderFilterDelegationUpstream) {
+  auto stream_filter = std::make_shared<Http::MockStreamEncoderFilter>();
+  StreamInfo::FilterStateSharedPtr filter_state =
+      std::make_shared<StreamInfo::FilterStateImpl>(StreamInfo::FilterState::LifeSpan::Connection);
+  ON_CALL(encoder_callbacks_, filterConfigName()).WillByDefault(testing::Return("rootFilterName"));
+  ON_CALL(encoder_callbacks_.stream_info_, filterState())
+      .WillByDefault(testing::ReturnRef(filter_state));
+
+  auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) {
+    cb.addStreamEncoderFilter(stream_filter);
+  };
+
+  EXPECT_CALL(*stream_filter, setEncoderFilterCallbacks(_));
+  ExecuteFilterAction action(factory_callback, "actionName");
+  EXPECT_CALL(success_counter_, inc());
+  filter_.onMatchCallback(action);
+
+  auto* info = filter_state->getDataMutable<MatchedActionInfo>(MatchedActionsFilterStateKey);
+  EXPECT_EQ(nullptr, info);
+
+  doAllDecodingCallbacks();
+  expectDelegatedEncoding(*stream_filter);
+  doAllEncodingCallbacks();
+
+  EXPECT_CALL(*stream_filter, onStreamComplete());
+  EXPECT_CALL(*stream_filter, onDestroy());
+  filter_.onStreamComplete();
+  filter_.onDestroy();
+}
+
 } // namespace
 } // namespace Composite
 } // namespace HttpFilters