Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

http: support passing match result action to filter #14462

Merged
merged 8 commits into from
Jan 13, 2021
Merged

http: support passing match result action to filter #14462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ef34466
basic custom action + test
snowp Dec 17, 2020
d848aaf
foramt
snowp Dec 17, 2020
d22c8d4
test for dual filter
snowp Dec 17, 2020
fe94c53
remove self import
snowp Dec 17, 2020
a838129
init bool field
snowp Dec 17, 2020
66e1cd0
store skip result on state, handle filter handle
snowp Jan 12, 2021
007ff2d
Merge remote-tracking branch 'envoy/master' into filter-action-match
snowp Jan 12, 2021
b314178
update tests to use new optref helper
snowp Jan 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions include/envoy/http/filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,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&) {}
};

/**
Expand Down
67 changes: 43 additions & 24 deletions source/common/http/filter_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ void ActiveStreamFilterBase::clearRouteCache() {
parent_.filter_manager_callbacks_.clearRouteCache();
}

void ActiveStreamFilterBase::evaluateMatchTreeWithNewData(
void FilterMatchState::evaluateMatchTreeWithNewData(
std::function<void(HttpMatchingDataImpl&)> update_func) {
if (match_tree_evaluated_ || !matching_data_) {
return;
Expand All @@ -258,7 +258,9 @@ void ActiveStreamFilterBase::evaluateMatchTreeWithNewData(

if (match_tree_evaluated_ && match_result.result_) {
if (SkipAction().typeUrl() == match_result.result_->typeUrl()) {
skip_ = true;
skip_filter_ = true;
} else {
filter_->onMatchCallback(*match_result.result_);
}
}
}
Expand Down Expand Up @@ -418,11 +420,18 @@ void ActiveStreamDecoderFilter::requestDataTooLarge() {
}
}

void FilterManager::addStreamDecoderFilterWorker(
StreamDecoderFilterSharedPtr filter, Matcher::MatchTreeSharedPtr<HttpMatchingData> 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.
Comment on lines +429 to +430
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused by this. Doesn't this need to be configurable or potentially context specific? Or the idea here is that a single filter registered twice would then only get the callbacks once and it doesn't actually matter whether they are in the encoding or decoding path? And this is really only needed for state that has to go through to the underlying impl like skip? Is skip the only case where this matters?

Given that the pointers are going to be the same if they are both set (right?), could the skip case be handled by just calling a method on the filter base which somehow knows how to set skip for both encoding and decoding direction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that we have two distinct filter wrappers around the same underlying filter: https://github.com/envoyproxy/envoy/blob/master/source/common/http/filter_manager.h#L705-L708

As a result, we need both wrappers to know about the fact that a skip action has been evaluated, since the skip action isn't associated with a specific filter wrapper, but to the filter itself. This here is one strategy where we have the FilterMatchState responsible for doing the matching and setting the skip_ flag on each of the wrappers. I can imagine another approach where we instead have the wrappers check the FilterMatchState to see whether it should skip?

For the filter action in the dual filter case we have two wrappers but we only want to notify the filter once about the match action, so we can get away with just arbitrarily picking one of the wrappers to use. Alternatively we could just hold onto a reference to the underlying filter instead: that way we're bypassing the wrappers in this case and things might be a bit clearer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can imagine another approach where we instead have the wrappers check the FilterMatchState to see whether it should skip?

I think this is what I would do since the state is shared?

Alternatively we could just hold onto a reference to the underlying filter instead: that way we're bypassing the wrappers in this case and things might be a bit clearer?

Yeah +1.

So yeah personally I would make the above changes but if you don't can you add some more comments somewhere leaving a trail about this?

/wait

if (match_state) {
match_state->filter_ = filter.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
Expand All @@ -435,11 +444,16 @@ void FilterManager::addStreamDecoderFilterWorker(
LinkedList::moveIntoListBack(std::move(wrapper), decoder_filters_);
}

void FilterManager::addStreamEncoderFilterWorker(
StreamEncoderFilterSharedPtr filter, Matcher::MatchTreeSharedPtr<HttpMatchingData> 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->filter_ = filter.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
Expand Down Expand Up @@ -477,9 +491,12 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead
std::list<ActiveStreamDecoderFilterPtr>::iterator continue_data_entry = decoder_filters_.end();

for (; entry != decoder_filters_.end(); entry++) {
(*entry)->evaluateMatchTreeWithNewData(
[&](auto& matching_data) { matching_data.onRequestHeaders(headers); });
if ((*entry)->skip_) {
if ((*entry)->filter_match_state_) {
(*entry)->filter_match_state_->evaluateMatchTreeWithNewData(
[&](auto& matching_data) { matching_data.onRequestHeaders(headers); });
}

if ((*entry)->skipFilter()) {
continue;
}

Expand Down Expand Up @@ -563,7 +580,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.
Expand Down Expand Up @@ -697,7 +714,7 @@ void FilterManager::decodeTrailers(ActiveStreamDecoderFilter* filter, RequestTra
commonDecodePrefix(filter, FilterIterationStartState::CanStartFromCurrent);

for (; entry != decoder_filters_.end(); entry++) {
if ((*entry)->skip_) {
if ((*entry)->skipFilter()) {
continue;
}

Expand Down Expand Up @@ -729,7 +746,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.
Expand Down Expand Up @@ -939,7 +956,7 @@ void FilterManager::encode100ContinueHeaders(ActiveStreamEncoderFilter* filter,
std::list<ActiveStreamEncoderFilterPtr>::iterator entry =
commonEncodePrefix(filter, false, FilterIterationStartState::AlwaysStartFromNext);
for (; entry != encoder_filters_.end(); entry++) {
if ((*entry)->skip_) {
if ((*entry)->skipFilter()) {
continue;
}

Expand Down Expand Up @@ -984,9 +1001,11 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea
std::list<ActiveStreamEncoderFilterPtr>::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)->skip_) {
if ((*entry)->filter_match_state_) {
(*entry)->filter_match_state_->evaluateMatchTreeWithNewData(
[&headers](auto& matching_data) { matching_data.onResponseHeaders(headers); });
}
if ((*entry)->skipFilter()) {
continue;
}
ASSERT(!(state_.filter_call_state_ & FilterCallState::EncodeHeaders));
Expand Down Expand Up @@ -1043,7 +1062,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.
Expand Down Expand Up @@ -1114,7 +1133,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.
Expand Down
Loading