http: add composite filter allowing dynamic filter selection#15198
http: add composite filter allowing dynamic filter selection#15198htuch merged 62 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
|
I think there are a few open comment threads. Are you still making changes? LMK when it's ready to review again. /wait-any |
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
mattklein123
left a comment
There was a problem hiding this comment.
Super awesome. Just some small comments and then I think we can ship!
/wait
| // headers. This makes it possible to use different filters or filter configurations based on the | ||
| // incoming request. | ||
| // | ||
| // This is intended to used with |
There was a problem hiding this comment.
| // This is intended to used with | |
| // This is intended to be used with |
| this, in order to delegate all the data to the specified filter, the decision must be made based | ||
| on just the request headers. | ||
|
|
||
| Delegation can fail for a few reasons: the match result was not ready in time for the filter |
There was a problem hiding this comment.
This can't happen anymore given that we require only request headers, right?
| Conditional Filter Configuration | ||
| -------------------------------- | ||
|
|
||
| There is some support for having the filter configuration used to change based on the incoming |
There was a problem hiding this comment.
| There is some support for having the filter configuration used to change based on the incoming | |
| There is some support for having the filter configuration used change based on the incoming |
| auto requirements = std::make_unique< | ||
| envoy::extensions::filters::common::dependency::v3::MatchingRequirements>(); | ||
|
|
||
| requirements->mutable_data_input_allow_list()->add_type_url( |
There was a problem hiding this comment.
Can you add a small human readable comment on what this does?
| // Helper that returns `filter->func(args...)` if the filter is not null, returning `rval` | ||
| // otherwise. | ||
| template <class FilterPtrT, class FuncT, class RValT, class... Args> | ||
| RValT delegateFilterAction(FilterPtrT& filter, FuncT func, RValT rval, Args&&... args) { |
There was a problem hiding this comment.
nit: I might call this delegateFilterActionOr to match similar things like value_or in optional, etc.
| // Use these to track whether we are allowed to insert a specific kind of filter. These mainly | ||
| // serve to surface an easier to understand error, as attempting to insert a filter at a later | ||
| // time will result in various FM assertions firing. | ||
| // TODO(snowp): Instead of validating this via ASSERTs, we should be able to validate that the | ||
| // match tree is only going to fire when we can actually inject a filter. |
There was a problem hiding this comment.
This is partially fixed, right? I was actually going to ask for ASSERTs also, so IMO it's not terrible to just keep this but should the comment be updated?
| if (!wrapper.errors_.empty()) { | ||
| stats_.filter_delegation_error_.inc(); | ||
| ENVOY_LOG(debug, "failed to create delegated filter {}", | ||
| accumulateToString<absl::Status>( | ||
| wrapper.errors_, [](const auto& status) { return status.ToString(); })); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Maybe this is an issue with the matching engine that needs to be fixed, but shouldn't this return an error code which should tell the engine to actually do something like fail the request? We can discuss later about fail open vs. fail closed, but it seems like it shouldn't just be a nop? wdyt?
There was a problem hiding this comment.
Yeah that's a good point, I think in the short term we could local reply here with a 500 or something, but maybe it would be nicer with direct integration with the matching framework? Or maybe just a success bool so that we can avoid calling decodeHeaders etc. if matching resulted in a local reply?
There was a problem hiding this comment.
IMO I would have it be built-in to the matching engine with a return code vs. void, but fine if you want to TODO here to fix that later and for now just do a local reply.
There was a problem hiding this comment.
Turns out we can't do a local reply here since the associated filter callback is invoked:
[2021-04-12 20:53:43.183][48][error][envoy_bug] [source/common/http/filter_manager.cc:523] envoy bug failure: !continue_iteration || !state_.local_complete_. Details: Filter did not return StopAll or StopIteration after sending a local reply.
Seems like we need some HCM changes to make this work, wdyt about landing this as is with a TODO and then follow up once we improve the onMatchCallback API?
There was a problem hiding this comment.
Sure that's fine, thanks for checking.
|
@htuch you have pending changes requested, ptal |
…oxy#15198) Adds a new filter that makes use of the new matching API to allow specifying a filter configuration to use for specific match results. Once a configuration is selected, the desired filter is created and all callbacks delegated to the specific filter. Risk Level: Low, new filter Testing: IT + UT Docs Changes: Inline proto comments Signed-off-by: Snow Pettersen <snowp@lyft.com> Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>
…oxy#15198) Adds a new filter that makes use of the new matching API to allow specifying a filter configuration to use for specific match results. Once a configuration is selected, the desired filter is created and all callbacks delegated to the specific filter. Risk Level: Low, new filter Testing: IT + UT Docs Changes: Inline proto comments Signed-off-by: Snow Pettersen <snowp@lyft.com>
Adds a new filter that makes use of the new matching API to allow specifying a filter configuration to use for specific match
results. Once a configuration is selected, the desired filter is created and all callbacks delegated to the specific filter.
Risk Level: Low, new filter
Testing: IT + UT
Docs Changes: Inline proto comments
Release Notes: wip