From ef344663ce23439393bbd75125ed9a0fe0d981c3 Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Thu, 17 Dec 2020 02:31:49 +0000 Subject: [PATCH 1/7] basic custom action + test Signed-off-by: Snow Pettersen --- include/envoy/http/filter.h | 6 ++ source/common/http/filter_manager.cc | 58 ++++++++---- source/common/http/filter_manager.h | 117 +++++++++++++++--------- test/common/http/filter_manager_test.cc | 45 +++++++++ test/mocks/http/mocks.h | 3 + 5 files changed, 172 insertions(+), 57 deletions(-) diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 4157dfc6819f..8efee7196bda 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -572,6 +572,12 @@ class StreamFilterBase { * onDestroy(). */ virtual void onDestroy() PURE; + + /** + * Called when a match result occurs that isn't handled by the filter manager. + * @param action the resulting match action + */ + virtual void onMatchCallback(const Matcher::Action& ) {} }; /** diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index f0b7618794f4..4aa40aef5527 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -242,7 +242,7 @@ void ActiveStreamFilterBase::clearRouteCache() { parent_.filter_manager_callbacks_.clearRouteCache(); } -void ActiveStreamFilterBase::evaluateMatchTreeWithNewData( +void FilterMatchState::evaluateMatchTreeWithNewData( std::function update_func) { if (match_tree_evaluated_ || !matching_data_) { return; @@ -256,7 +256,12 @@ void ActiveStreamFilterBase::evaluateMatchTreeWithNewData( if (match_tree_evaluated_ && match_result.result_) { if (SkipAction().typeUrl() == match_result.result_->typeUrl()) { - skip_ = true; + callback_handling_filter_->skip_ = true; + if (secondary_filter_) { + secondary_filter_->skip_ = true; + } + } else { + callback_handling_filter_->onMatchCallback(*match_result.result_); } } } @@ -404,11 +409,22 @@ void ActiveStreamDecoderFilter::requestDataTooLarge() { } } -void FilterManager::addStreamDecoderFilterWorker( - StreamDecoderFilterSharedPtr filter, Matcher::MatchTreeSharedPtr matcher, - HttpMatchingDataImplSharedPtr matching_data, bool dual_filter) { - ActiveStreamDecoderFilterPtr wrapper(new ActiveStreamDecoderFilter( - *this, filter, std::move(matcher), std::move(matching_data), dual_filter)); +void FilterManager::addStreamDecoderFilterWorker(StreamDecoderFilterSharedPtr filter, + FilterMatchStateSharedPtr match_state, + bool dual_filter) { + ActiveStreamDecoderFilterPtr wrapper( + new ActiveStreamDecoderFilter(*this, filter, match_state, dual_filter)); + + // If we're a dual handling filter, have the encoding wrapper be the only thing registering itself + // as the handling filter. + if (match_state) { + if (dual_filter) { + match_state->secondary_filter_ = wrapper.get(); + } else { + match_state->callback_handling_filter_ = wrapper.get(); + } + } + filter->setDecoderFilterCallbacks(*wrapper); // Note: configured decoder filters are appended to decoder_filters_. // This means that if filters are configured in the following order (assume all three filters are @@ -421,11 +437,16 @@ void FilterManager::addStreamDecoderFilterWorker( LinkedList::moveIntoListBack(std::move(wrapper), decoder_filters_); } -void FilterManager::addStreamEncoderFilterWorker( - StreamEncoderFilterSharedPtr filter, Matcher::MatchTreeSharedPtr match_tree, - HttpMatchingDataImplSharedPtr matching_data, bool dual_filter) { - ActiveStreamEncoderFilterPtr wrapper(new ActiveStreamEncoderFilter( - *this, filter, std::move(match_tree), std::move(matching_data), dual_filter)); +void FilterManager::addStreamEncoderFilterWorker(StreamEncoderFilterSharedPtr filter, + FilterMatchStateSharedPtr match_state, + bool dual_filter) { + ActiveStreamEncoderFilterPtr wrapper( + new ActiveStreamEncoderFilter(*this, filter, match_state, dual_filter)); + + if (match_state) { + match_state->callback_handling_filter_ = wrapper.get(); + } + filter->setEncoderFilterCallbacks(*wrapper); // Note: configured encoder filters are prepended to encoder_filters_. // This means that if filters are configured in the following order (assume all three filters are @@ -463,8 +484,11 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead std::list::iterator continue_data_entry = decoder_filters_.end(); for (; entry != decoder_filters_.end(); entry++) { - (*entry)->evaluateMatchTreeWithNewData( - [&](auto& matching_data) { matching_data.onRequestHeaders(headers); }); + if ((*entry)->filter_match_state_) { + (*entry)->filter_match_state_->evaluateMatchTreeWithNewData( + [&](auto& matching_data) { matching_data.onRequestHeaders(headers); }); + } + if ((*entry)->skip_) { continue; } @@ -970,8 +994,10 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea std::list::iterator continue_data_entry = encoder_filters_.end(); for (; entry != encoder_filters_.end(); entry++) { - (*entry)->evaluateMatchTreeWithNewData( - [&headers](auto& matching_data) { matching_data.onResponseHeaders(headers); }); + if ((*entry)->filter_match_state_) { + (*entry)->filter_match_state_->evaluateMatchTreeWithNewData( + [&headers](auto& matching_data) { matching_data.onResponseHeaders(headers); }); + } if ((*entry)->skip_) { continue; } diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 21109a82d774..89858ff687af 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -2,6 +2,7 @@ #include +#include "common/http/filter_manager.h" #include "envoy/extensions/filters/common/matcher/action/v3/skip_action.pb.h" #include "envoy/http/filter.h" #include "envoy/http/header_map.h" @@ -116,6 +117,39 @@ class HttpResponseHeadersDataInput : public HttpHeadersDataInputBase {}; +struct ActiveStreamFilterBase; + +/** + * Manages the shared match state between one or two filters. + * The need for this class comes from the fact that a single instantiated filter can be wrapped by + * two different ActiveStreamFilters, one for encoding and one for decoding. Certain match actions + * should be made visible to both wrappers (e.g. the skip action), while other actions should be + * sent to the underlying filter exactly once. + */ +class FilterMatchState { +public: + FilterMatchState(Matcher::MatchTreeSharedPtr match_tree, + HttpMatchingDataImplSharedPtr matching_data) + : match_tree_(std::move(match_tree)), matching_data_(std::move(matching_data)) {} + + void evaluateMatchTreeWithNewData(std::function update_func); + + // Some callbacks should be sent to all filters (e.g. filter specific match actions), while + // others should trigger updates to all filters (e.g. filter skip). The callback_handling_filter_ + // should always be present and is the filter wrapper that handles the per filter actions, while + // secondary_filter_ is present for dual filters to allow setting a value for both wrappers. + // TODO(snowp): Should we instead snap a handle to the underlying filter and a list of filter wrappers? + ActiveStreamFilterBase* callback_handling_filter_{}; + ActiveStreamFilterBase* secondary_filter_{}; + +private: + Matcher::MatchTreeSharedPtr match_tree_; + HttpMatchingDataImplSharedPtr matching_data_; + bool match_tree_evaluated_ : 1; +}; + +using FilterMatchStateSharedPtr = std::shared_ptr; + /** * Base class wrapper for both stream encoder and decoder filters. * @@ -126,14 +160,12 @@ class SkipAction : public Matcher::ActionBase< struct ActiveStreamFilterBase : public virtual StreamFilterCallbacks, Logger::Loggable { ActiveStreamFilterBase(FilterManager& parent, bool dual_filter, - Matcher::MatchTreeSharedPtr match_tree, - HttpMatchingDataImplSharedPtr matching_data) + FilterMatchStateSharedPtr match_state) : parent_(parent), iteration_state_(IterationState::Continue), - match_tree_(std::move(match_tree)), matching_data_(std::move(matching_data)), - iterate_from_current_filter_(false), headers_continued_(false), - continue_headers_continued_(false), end_stream_(false), dual_filter_(dual_filter), - decode_headers_called_(false), encode_headers_called_(false), match_tree_evaluated_(false), - skip_(false) {} + filter_match_state_(std::move(match_state)), iterate_from_current_filter_(false), + headers_continued_(false), continue_headers_continued_(false), end_stream_(false), + dual_filter_(dual_filter), decode_headers_called_(false), encode_headers_called_(false), + match_tree_evaluated_(false), skip_(false) {} // Functions in the following block are called after the filter finishes processing // corresponding data. Those functions handle state updates and data storage (if needed) @@ -166,6 +198,8 @@ struct ActiveStreamFilterBase : public virtual StreamFilterCallbacks, // TODO(soya3129): make this pure when adding impl to encoder filter. virtual void handleMetadataAfterHeadersCallback() PURE; + virtual void onMatchCallback(const Matcher::Action& action) PURE; + // Http::StreamFilterCallbacks const Network::Connection* connection() override; Event::Dispatcher& dispatcher() override; @@ -203,8 +237,6 @@ struct ActiveStreamFilterBase : public virtual StreamFilterCallbacks, return saved_response_metadata_.get(); } - void evaluateMatchTreeWithNewData(std::function update_func); - // A vector to save metadata when the current filter's [de|en]codeMetadata() can not be called, // either because [de|en]codeHeaders() of the current filter returns StopAllIteration or because // [de|en]codeHeaders() adds new metadata to [de|en]code, but we don't know @@ -223,9 +255,7 @@ struct ActiveStreamFilterBase : public virtual StreamFilterCallbacks, FilterManager& parent_; IterationState iteration_state_; - Matcher::MatchTreeSharedPtr match_tree_; - HttpMatchingDataImplSharedPtr matching_data_; - + FilterMatchStateSharedPtr filter_match_state_; // If the filter resumes iteration from a StopAllBuffer/Watermark state, the current filter // hasn't parsed data and trailers. As a result, the filter iteration should start with the // current filter instead of the next one. If true, filter iteration starts with the current @@ -240,6 +270,8 @@ struct ActiveStreamFilterBase : public virtual StreamFilterCallbacks, bool encode_headers_called_ : 1; bool match_tree_evaluated_ : 1; bool skip_ : 1; + + friend FilterMatchState; }; /** @@ -249,11 +281,8 @@ struct ActiveStreamDecoderFilter : public ActiveStreamFilterBase, public StreamDecoderFilterCallbacks, LinkedObject { ActiveStreamDecoderFilter(FilterManager& parent, StreamDecoderFilterSharedPtr filter, - Matcher::MatchTreeSharedPtr match_tree, - HttpMatchingDataImplSharedPtr matching_data, bool dual_filter) - : ActiveStreamFilterBase(parent, dual_filter, std::move(match_tree), - std::move(matching_data)), - handle_(filter) {} + FilterMatchStateSharedPtr match_state, bool dual_filter) + : ActiveStreamFilterBase(parent, dual_filter, std::move(match_state)), handle_(filter) {} // ActiveStreamFilterBase bool canContinue() override; @@ -275,6 +304,9 @@ struct ActiveStreamDecoderFilter : public ActiveStreamFilterBase, void drainSavedRequestMetadata(); // This function is called after the filter calls decodeHeaders() to drain accumulated metadata. void handleMetadataAfterHeadersCallback() override; + void onMatchCallback(const Matcher::Action& action) override { + handle_->onMatchCallback(std::move(action)); + } // Http::StreamDecoderFilterCallbacks void addDecodedData(Buffer::Instance& data, bool streaming) override; @@ -338,11 +370,8 @@ struct ActiveStreamEncoderFilter : public ActiveStreamFilterBase, public StreamEncoderFilterCallbacks, LinkedObject { ActiveStreamEncoderFilter(FilterManager& parent, StreamEncoderFilterSharedPtr filter, - Matcher::MatchTreeSharedPtr match_tree, - HttpMatchingDataImplSharedPtr matching_data, bool dual_filter) - : ActiveStreamFilterBase(parent, dual_filter, std::move(match_tree), - std::move(matching_data)), - handle_(filter) {} + FilterMatchStateSharedPtr match_state, bool dual_filter) + : ActiveStreamFilterBase(parent, dual_filter, std::move(match_state)), handle_(filter) {} // ActiveStreamFilterBase bool canContinue() override { return true; } @@ -355,6 +384,7 @@ struct ActiveStreamEncoderFilter : public ActiveStreamFilterBase, void doData(bool end_stream) override; void drainSavedResponseMetadata(); void handleMetadataAfterHeadersCallback() override; + void onMatchCallback(const Matcher::Action& action) override { handle_->onMatchCallback(action); } void doMetadata() override { if (saved_response_metadata_ != nullptr) { @@ -639,34 +669,40 @@ class FilterManager : public ScopeTrackedObject, // Http::FilterChainFactoryCallbacks void addStreamDecoderFilter(StreamDecoderFilterSharedPtr filter) override { - addStreamDecoderFilterWorker(filter, nullptr, nullptr, false); + addStreamDecoderFilterWorker(filter, nullptr, false); } void addStreamDecoderFilter(StreamDecoderFilterSharedPtr filter, Matcher::MatchTreeSharedPtr match_tree) override { if (match_tree) { - auto matching_data = std::make_shared(); - addStreamDecoderFilterWorker(filter, std::move(match_tree), std::move(matching_data), false); + addStreamDecoderFilterWorker( + filter, + std::make_shared(std::move(match_tree), + std::make_shared()), + false); return; } - addStreamDecoderFilterWorker(filter, nullptr, nullptr, false); + addStreamDecoderFilterWorker(filter, nullptr, false); } void addStreamEncoderFilter(StreamEncoderFilterSharedPtr filter) override { - addStreamEncoderFilterWorker(filter, nullptr, nullptr, false); + addStreamEncoderFilterWorker(filter, nullptr, false); } void addStreamEncoderFilter(StreamEncoderFilterSharedPtr filter, Matcher::MatchTreeSharedPtr match_tree) override { if (match_tree) { - addStreamEncoderFilterWorker(filter, std::move(match_tree), - std::make_shared(), false); + addStreamEncoderFilterWorker( + filter, + std::make_shared(std::move(match_tree), + std::make_shared()), + false); return; } - addStreamEncoderFilterWorker(filter, nullptr, nullptr, false); + addStreamEncoderFilterWorker(filter, nullptr, false); } void addStreamFilter(StreamFilterSharedPtr filter) override { - addStreamDecoderFilterWorker(filter, nullptr, nullptr, true); - addStreamEncoderFilterWorker(filter, nullptr, nullptr, true); + addStreamDecoderFilterWorker(filter, nullptr, true); + addStreamEncoderFilterWorker(filter, nullptr, true); } void addStreamFilter(StreamFilterSharedPtr filter, Matcher::MatchTreeSharedPtr match_tree) override { @@ -675,14 +711,15 @@ class FilterManager : public ScopeTrackedObject, // TODO(snowp): The match tree might be fully evaluated twice, ideally we should expose // the result to both filters after the first match evaluation. if (match_tree) { - auto matching_data = std::make_shared(); - addStreamDecoderFilterWorker(filter, match_tree, matching_data, true); - addStreamEncoderFilterWorker(filter, std::move(match_tree), std::move(matching_data), true); + auto matching_state = std::make_shared( + std::move(match_tree), std::make_shared()); + addStreamDecoderFilterWorker(filter, matching_state, true); + addStreamEncoderFilterWorker(filter, std::move(matching_state), true); return; } - addStreamDecoderFilterWorker(filter, nullptr, nullptr, true); - addStreamEncoderFilterWorker(filter, nullptr, nullptr, true); + addStreamDecoderFilterWorker(filter, nullptr, true); + addStreamEncoderFilterWorker(filter, nullptr, true); } void addAccessLogHandler(AccessLog::InstanceSharedPtr handler) override; @@ -765,11 +802,9 @@ class FilterManager : public ScopeTrackedObject, // TODO(snowp): Make private as filter chain construction is moved into FM. void addStreamDecoderFilterWorker(StreamDecoderFilterSharedPtr filter, - Matcher::MatchTreeSharedPtr match_tree, - HttpMatchingDataImplSharedPtr matching_data, bool dual_filter); + FilterMatchStateSharedPtr match_state, bool dual_filter); void addStreamEncoderFilterWorker(StreamEncoderFilterSharedPtr filter, - Matcher::MatchTreeSharedPtr match_tree, - HttpMatchingDataImplSharedPtr matching_data, bool dual_filter); + FilterMatchStateSharedPtr match_state, bool dual_filter); void disarmRequestTimeout(); diff --git a/test/common/http/filter_manager_test.cc b/test/common/http/filter_manager_test.cc index 08a34e7054f3..defa0c939718 100644 --- a/test/common/http/filter_manager_test.cc +++ b/test/common/http/filter_manager_test.cc @@ -1,3 +1,4 @@ +#include "common/matcher/matcher.h" #include "envoy/http/filter.h" #include "envoy/http/header_map.h" #include "envoy/matcher/matcher.h" @@ -154,6 +155,18 @@ Matcher::MatchTreeSharedPtr createRequestMatchingTree() { return tree; } +struct TestAction : Matcher::ActionBase {}; + +Matcher::MatchTreeSharedPtr createRequestMatchingTreeCustomAction() { + auto tree = std::make_shared>( + std::make_unique("match-header"), absl::nullopt); + + tree->addChild("match", Matcher::OnMatch{ + []() { return std::make_unique(); }, nullptr}); + + return tree; +} + Matcher::MatchTreeSharedPtr createRequestAndResponseMatchingTree() { auto tree = std::make_shared>( std::make_unique("match-header"), absl::nullopt); @@ -248,6 +261,38 @@ TEST_F(FilterManagerTest, MatchTreeSkipActionRequestAndResponseHeaders) { filter_manager_->decodeData(data, true); filter_manager_->destroyFilters(); } + +TEST_F(FilterManagerTest, MatchTreeFilterActionDecodingHeaders) { + initialize(); + + // The filter is added, but since we match on the request header we skip the filter. + std::shared_ptr decoder_filter(new MockStreamDecoderFilter()); + EXPECT_CALL(*decoder_filter, setDecoderFilterCallbacks(_)); + EXPECT_CALL(*decoder_filter, onMatchCallback(_)); + EXPECT_CALL(*decoder_filter, decodeHeaders(_, _)); + EXPECT_CALL(*decoder_filter, decodeComplete()); + EXPECT_CALL(*decoder_filter, onDestroy()); + + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillRepeatedly(Invoke([&](FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamDecoderFilter(decoder_filter, createRequestMatchingTreeCustomAction()); + })); + + RequestHeaderMapPtr grpc_headers{ + new TestRequestHeaderMapImpl{{":authority", "host"}, + {":path", "/"}, + {":method", "GET"}, + {"match-header", "match"}, + {"content-type", "application/grpc"}}}; + + ON_CALL(filter_manager_callbacks_, requestHeaders()) + .WillByDefault(Return(absl::make_optional(std::ref(*grpc_headers)))); + filter_manager_->createFilterChain(); + + filter_manager_->requestHeadersInitialized(); + filter_manager_->decodeHeaders(*grpc_headers, true); + filter_manager_->destroyFilters(); +} } // namespace } // namespace Http } // namespace Envoy \ No newline at end of file diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 5505b702ea04..c9ccf86a9d5f 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -318,6 +318,7 @@ class MockStreamDecoderFilter : public StreamDecoderFilter { // Http::StreamFilterBase MOCK_METHOD(void, onStreamComplete, ()); MOCK_METHOD(void, onDestroy, ()); + MOCK_METHOD(void, onMatchCallback, (const Matcher::Action&)); // Http::StreamDecoderFilter MOCK_METHOD(FilterHeadersStatus, decodeHeaders, (RequestHeaderMap & headers, bool end_stream)); @@ -343,6 +344,7 @@ class MockStreamEncoderFilter : public StreamEncoderFilter { // Http::StreamFilterBase MOCK_METHOD(void, onStreamComplete, ()); MOCK_METHOD(void, onDestroy, ()); + MOCK_METHOD(void, onMatchCallback, (const Matcher::Action&)); // Http::MockStreamEncoderFilter MOCK_METHOD(FilterHeadersStatus, encode100ContinueHeaders, (ResponseHeaderMap & headers)); @@ -364,6 +366,7 @@ class MockStreamFilter : public StreamFilter { // Http::StreamFilterBase MOCK_METHOD(void, onStreamComplete, ()); MOCK_METHOD(void, onDestroy, ()); + MOCK_METHOD(void, onMatchCallback, (const Matcher::Action&)); // Http::StreamDecoderFilter MOCK_METHOD(FilterHeadersStatus, decodeHeaders, (RequestHeaderMap & headers, bool end_stream)); From d848aaf33d78f63c515c99c9b88f46da96cb8b4e Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Thu, 17 Dec 2020 02:33:05 +0000 Subject: [PATCH 2/7] foramt Signed-off-by: Snow Pettersen --- include/envoy/http/filter.h | 2 +- source/common/http/filter_manager.h | 5 +++-- test/common/http/filter_manager_test.cc | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 8efee7196bda..2bf1a3e86ea6 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -577,7 +577,7 @@ class StreamFilterBase { * Called when a match result occurs that isn't handled by the filter manager. * @param action the resulting match action */ - virtual void onMatchCallback(const Matcher::Action& ) {} + virtual void onMatchCallback(const Matcher::Action&) {} }; /** diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 89858ff687af..5a6bfda33d14 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -2,7 +2,6 @@ #include -#include "common/http/filter_manager.h" #include "envoy/extensions/filters/common/matcher/action/v3/skip_action.pb.h" #include "envoy/http/filter.h" #include "envoy/http/header_map.h" @@ -13,6 +12,7 @@ #include "common/common/linked_object.h" #include "common/common/logger.h" #include "common/grpc/common.h" +#include "common/http/filter_manager.h" #include "common/http/header_utility.h" #include "common/http/headers.h" #include "common/local_reply/local_reply.h" @@ -138,7 +138,8 @@ class FilterMatchState { // others should trigger updates to all filters (e.g. filter skip). The callback_handling_filter_ // should always be present and is the filter wrapper that handles the per filter actions, while // secondary_filter_ is present for dual filters to allow setting a value for both wrappers. - // TODO(snowp): Should we instead snap a handle to the underlying filter and a list of filter wrappers? + // TODO(snowp): Should we instead snap a handle to the underlying filter and a list of filter + // wrappers? ActiveStreamFilterBase* callback_handling_filter_{}; ActiveStreamFilterBase* secondary_filter_{}; diff --git a/test/common/http/filter_manager_test.cc b/test/common/http/filter_manager_test.cc index defa0c939718..1ca82a7f4f71 100644 --- a/test/common/http/filter_manager_test.cc +++ b/test/common/http/filter_manager_test.cc @@ -1,4 +1,3 @@ -#include "common/matcher/matcher.h" #include "envoy/http/filter.h" #include "envoy/http/header_map.h" #include "envoy/matcher/matcher.h" @@ -6,6 +5,7 @@ #include "common/http/filter_manager.h" #include "common/matcher/exact_map_matcher.h" +#include "common/matcher/matcher.h" #include "common/stream_info/filter_state_impl.h" #include "common/stream_info/stream_info_impl.h" From d22c8d4938ff1b174f2a577c3e2cf73c3d85a057 Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Thu, 17 Dec 2020 03:04:26 +0000 Subject: [PATCH 3/7] test for dual filter Signed-off-by: Snow Pettersen --- test/common/http/filter_manager_test.cc | 52 ++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/test/common/http/filter_manager_test.cc b/test/common/http/filter_manager_test.cc index 1ca82a7f4f71..a609d1b57d86 100644 --- a/test/common/http/filter_manager_test.cc +++ b/test/common/http/filter_manager_test.cc @@ -167,6 +167,16 @@ Matcher::MatchTreeSharedPtr createRequestMatchingTreeCustomAct return tree; } +Matcher::MatchTreeSharedPtr createResponseMatchingTreeCustomAction() { + auto tree = std::make_shared>( + std::make_unique("match-header"), absl::nullopt); + + tree->addChild("match", Matcher::OnMatch{ + []() { return std::make_unique(); }, nullptr}); + + return tree; +} + Matcher::MatchTreeSharedPtr createRequestAndResponseMatchingTree() { auto tree = std::make_shared>( std::make_unique("match-header"), absl::nullopt); @@ -262,10 +272,10 @@ TEST_F(FilterManagerTest, MatchTreeSkipActionRequestAndResponseHeaders) { filter_manager_->destroyFilters(); } +// Verify that we propagate custom match actions to a decoding filter. TEST_F(FilterManagerTest, MatchTreeFilterActionDecodingHeaders) { initialize(); - // The filter is added, but since we match on the request header we skip the filter. std::shared_ptr decoder_filter(new MockStreamDecoderFilter()); EXPECT_CALL(*decoder_filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(*decoder_filter, onMatchCallback(_)); @@ -293,6 +303,46 @@ TEST_F(FilterManagerTest, MatchTreeFilterActionDecodingHeaders) { filter_manager_->decodeHeaders(*grpc_headers, true); filter_manager_->destroyFilters(); } + +// Verify that we propagate custom match actions exactly once to a dual filter. +TEST_F(FilterManagerTest, MatchTreeFilterActionDualFilter) { + initialize(); + + std::shared_ptr filter(new MockStreamFilter()); + EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); + EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); + EXPECT_CALL(*filter, decodeHeaders(_, true)) + .WillOnce(Invoke([&](auto&, bool) -> FilterHeadersStatus { + ResponseHeaderMapPtr headers{new TestResponseHeaderMapImpl{ + {":status", "200"}, {"match-header", "match"}, {"content-type", "application/grpc"}}}; + filter->decoder_callbacks_->encodeHeaders(std::move(headers), true, "details"); + + return FilterHeadersStatus::StopIteration; + })); + EXPECT_CALL(*filter, onDestroy()); + + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillRepeatedly(Invoke([&](FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamFilter(filter, createResponseMatchingTreeCustomAction()); + })); + + RequestHeaderMapPtr grpc_headers{ + new TestRequestHeaderMapImpl{{":authority", "host"}, + {":path", "/"}, + {":method", "GET"}, + {"match-header", "match"}, + {"content-type", "application/grpc"}}}; + + ON_CALL(filter_manager_callbacks_, requestHeaders()) + .WillByDefault(Return(absl::make_optional(std::ref(*grpc_headers)))); + filter_manager_->createFilterChain(); + + filter_manager_->requestHeadersInitialized(); + EXPECT_CALL(*filter, encodeHeaders(_, true)); + EXPECT_CALL(*filter, onMatchCallback(_)); + filter_manager_->decodeHeaders(*grpc_headers, true); + filter_manager_->destroyFilters(); +} } // namespace } // namespace Http } // namespace Envoy \ No newline at end of file From fe94c53a2044d4b0d97c292696b74e5d10c35c8b Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Thu, 17 Dec 2020 13:28:23 +0000 Subject: [PATCH 4/7] remove self import Signed-off-by: Snow Pettersen --- source/common/http/filter_manager.h | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 5a6bfda33d14..e780b5d2845c 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -12,7 +12,6 @@ #include "common/common/linked_object.h" #include "common/common/logger.h" #include "common/grpc/common.h" -#include "common/http/filter_manager.h" #include "common/http/header_utility.h" #include "common/http/headers.h" #include "common/local_reply/local_reply.h" From a83812941445d98749c2900e430d7c7b0d100e44 Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Thu, 17 Dec 2020 23:02:52 +0000 Subject: [PATCH 5/7] init bool field Signed-off-by: Snow Pettersen --- source/common/http/filter_manager.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index e780b5d2845c..1866196f017b 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -129,7 +129,8 @@ class FilterMatchState { public: FilterMatchState(Matcher::MatchTreeSharedPtr match_tree, HttpMatchingDataImplSharedPtr matching_data) - : match_tree_(std::move(match_tree)), matching_data_(std::move(matching_data)) {} + : match_tree_(std::move(match_tree)), matching_data_(std::move(matching_data)), + match_tree_evaluated_(false) {} void evaluateMatchTreeWithNewData(std::function update_func); From 66e1cd020931bd543f0bd33e09820dc966ec0c74 Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Tue, 12 Jan 2021 14:50:50 +0000 Subject: [PATCH 6/7] store skip result on state, handle filter handle Signed-off-by: Snow Pettersen --- source/common/http/filter_manager.cc | 31 +++++++++++----------------- source/common/http/filter_manager.h | 20 +++++++----------- 2 files changed, 19 insertions(+), 32 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 4aa40aef5527..b9be2ee99be9 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -256,12 +256,9 @@ void FilterMatchState::evaluateMatchTreeWithNewData( if (match_tree_evaluated_ && match_result.result_) { if (SkipAction().typeUrl() == match_result.result_->typeUrl()) { - callback_handling_filter_->skip_ = true; - if (secondary_filter_) { - secondary_filter_->skip_ = true; - } + skip_filter_ = true; } else { - callback_handling_filter_->onMatchCallback(*match_result.result_); + filter_->onMatchCallback(*match_result.result_); } } } @@ -418,11 +415,7 @@ void FilterManager::addStreamDecoderFilterWorker(StreamDecoderFilterSharedPtr fi // If we're a dual handling filter, have the encoding wrapper be the only thing registering itself // as the handling filter. if (match_state) { - if (dual_filter) { - match_state->secondary_filter_ = wrapper.get(); - } else { - match_state->callback_handling_filter_ = wrapper.get(); - } + match_state->filter_ = filter.get(); } filter->setDecoderFilterCallbacks(*wrapper); @@ -444,7 +437,7 @@ void FilterManager::addStreamEncoderFilterWorker(StreamEncoderFilterSharedPtr fi new ActiveStreamEncoderFilter(*this, filter, match_state, dual_filter)); if (match_state) { - match_state->callback_handling_filter_ = wrapper.get(); + match_state->filter_ = filter.get(); } filter->setEncoderFilterCallbacks(*wrapper); @@ -489,7 +482,7 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead [&](auto& matching_data) { matching_data.onRequestHeaders(headers); }); } - if ((*entry)->skip_) { + if ((*entry)->skipFilter()) { continue; } @@ -569,7 +562,7 @@ void FilterManager::decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instan commonDecodePrefix(filter, filter_iteration_start_state); for (; entry != decoder_filters_.end(); entry++) { - if ((*entry)->skip_) { + if ((*entry)->skipFilter()) { continue; } // If the filter pointed by entry has stopped for all frame types, return now. @@ -703,7 +696,7 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra commonDecodePrefix(filter, FilterIterationStartState::CanStartFromCurrent); for (; entry != decoder_filters_.end(); entry++) { - if ((*entry)->skip_) { + if ((*entry)->skipFilter()) { continue; } @@ -735,7 +728,7 @@ void FilterManager::decodeMetadata(ActiveStreamDecoderFilter* filter, MetadataMa commonDecodePrefix(filter, FilterIterationStartState::CanStartFromCurrent); for (; entry != decoder_filters_.end(); entry++) { - if ((*entry)->skip_) { + if ((*entry)->skipFilter()) { continue; } // If the filter pointed by entry has stopped for all frame type, stores metadata and returns. @@ -949,7 +942,7 @@ void FilterManager::encode100ContinueHeaders(ActiveStreamEncoderFilter* filter, std::list::iterator entry = commonEncodePrefix(filter, false, FilterIterationStartState::AlwaysStartFromNext); for (; entry != encoder_filters_.end(); entry++) { - if ((*entry)->skip_) { + if ((*entry)->skipFilter()) { continue; } @@ -998,7 +991,7 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea (*entry)->filter_match_state_->evaluateMatchTreeWithNewData( [&headers](auto& matching_data) { matching_data.onResponseHeaders(headers); }); } - if ((*entry)->skip_) { + if ((*entry)->skipFilter()) { continue; } ASSERT(!(state_.filter_call_state_ & FilterCallState::EncodeHeaders)); @@ -1055,7 +1048,7 @@ void FilterManager::encodeMetadata(ActiveStreamEncoderFilter* filter, commonEncodePrefix(filter, false, FilterIterationStartState::CanStartFromCurrent); for (; entry != encoder_filters_.end(); entry++) { - if ((*entry)->skip_) { + if ((*entry)->skipFilter()) { continue; } // If the filter pointed by entry has stopped for all frame type, stores metadata and returns. @@ -1126,7 +1119,7 @@ void FilterManager::encodeData(ActiveStreamEncoderFilter* filter, Buffer::Instan const bool trailers_exists_at_start = filter_manager_callbacks_.responseTrailers().has_value(); for (; entry != encoder_filters_.end(); entry++) { - if ((*entry)->skip_) { + if ((*entry)->skipFilter()) { continue; } // If the filter pointed by entry has stopped for all frame type, return now. diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 1866196f017b..96393ac1f20e 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -130,23 +130,19 @@ class FilterMatchState { FilterMatchState(Matcher::MatchTreeSharedPtr match_tree, HttpMatchingDataImplSharedPtr matching_data) : match_tree_(std::move(match_tree)), matching_data_(std::move(matching_data)), - match_tree_evaluated_(false) {} + match_tree_evaluated_(false), skip_filter_(false) {} void evaluateMatchTreeWithNewData(std::function update_func); - // Some callbacks should be sent to all filters (e.g. filter specific match actions), while - // others should trigger updates to all filters (e.g. filter skip). The callback_handling_filter_ - // should always be present and is the filter wrapper that handles the per filter actions, while - // secondary_filter_ is present for dual filters to allow setting a value for both wrappers. - // TODO(snowp): Should we instead snap a handle to the underlying filter and a list of filter - // wrappers? - ActiveStreamFilterBase* callback_handling_filter_{}; - ActiveStreamFilterBase* secondary_filter_{}; + StreamFilterBase* filter_{}; + + bool skipFilter() const { return skip_filter_; } private: Matcher::MatchTreeSharedPtr match_tree_; HttpMatchingDataImplSharedPtr matching_data_; bool match_tree_evaluated_ : 1; + bool skip_filter_ : 1; }; using FilterMatchStateSharedPtr = std::shared_ptr; @@ -165,8 +161,7 @@ struct ActiveStreamFilterBase : public virtual StreamFilterCallbacks, : parent_(parent), iteration_state_(IterationState::Continue), filter_match_state_(std::move(match_state)), iterate_from_current_filter_(false), headers_continued_(false), continue_headers_continued_(false), end_stream_(false), - dual_filter_(dual_filter), decode_headers_called_(false), encode_headers_called_(false), - match_tree_evaluated_(false), skip_(false) {} + dual_filter_(dual_filter), decode_headers_called_(false), encode_headers_called_(false) {} // Functions in the following block are called after the filter finishes processing // corresponding data. Those functions handle state updates and data storage (if needed) @@ -237,6 +232,7 @@ struct ActiveStreamFilterBase : public virtual StreamFilterCallbacks, } return saved_response_metadata_.get(); } + bool skipFilter() const { return filter_match_state_ && filter_match_state_->skipFilter(); } // A vector to save metadata when the current filter's [de|en]codeMetadata() can not be called, // either because [de|en]codeHeaders() of the current filter returns StopAllIteration or because @@ -269,8 +265,6 @@ struct ActiveStreamFilterBase : public virtual StreamFilterCallbacks, const bool dual_filter_ : 1; bool decode_headers_called_ : 1; bool encode_headers_called_ : 1; - bool match_tree_evaluated_ : 1; - bool skip_ : 1; friend FilterMatchState; }; From b314178bea964f4c0797c5b3a1838c6b22f1ad71 Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Tue, 12 Jan 2021 19:15:07 +0000 Subject: [PATCH 7/7] update tests to use new optref helper Signed-off-by: Snow Pettersen --- test/common/http/filter_manager_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/http/filter_manager_test.cc b/test/common/http/filter_manager_test.cc index c885fba1fc3d..9e11905482c3 100644 --- a/test/common/http/filter_manager_test.cc +++ b/test/common/http/filter_manager_test.cc @@ -297,7 +297,7 @@ TEST_F(FilterManagerTest, MatchTreeFilterActionDecodingHeaders) { {"content-type", "application/grpc"}}}; ON_CALL(filter_manager_callbacks_, requestHeaders()) - .WillByDefault(Return(absl::make_optional(std::ref(*grpc_headers)))); + .WillByDefault(Return(makeOptRef(*grpc_headers))); filter_manager_->createFilterChain(); filter_manager_->requestHeadersInitialized(); @@ -335,7 +335,7 @@ TEST_F(FilterManagerTest, MatchTreeFilterActionDualFilter) { {"content-type", "application/grpc"}}}; ON_CALL(filter_manager_callbacks_, requestHeaders()) - .WillByDefault(Return(absl::make_optional(std::ref(*grpc_headers)))); + .WillByDefault(Return(makeOptRef(*grpc_headers))); filter_manager_->createFilterChain(); filter_manager_->requestHeadersInitialized();