Adding MultiActions for composite filters. WIP #26251

jstraceski · 2023-03-22T13:29:59Z

Commit Message: Adding a Multi Action Execution action to the composite filter.
Additional Description: Executes multiple actions as one route path in the filter.
Risk Level: n/a
Testing: test/extensions/filters/http/composite/composite_filter_integration_test.cc
Docs Changes: TBD
Release Notes: TBD
Platform Specific Features: N/A

repokitteh-read-only · 2023-03-22T13:30:12Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #26251 was opened by jstraceski.

see: more, trace.

tyxia · 2023-03-22T14:34:55Z

source/extensions/filters/http/composite/action.h

+    : public Matcher::ActionBase<
+          envoy::extensions::filters::http::composite::v3::ExecuteFilterMultiAction> {
+public:
+  explicit ExecuteFilterMultiAction(Http::FilterFactoryCb callbacks)


Here, the function arg should be a list of callbacks`?

Signed-off-by: Joseph Straceski <jstraceski@google.com>

htuch

Thanks, looks good, couple of questions.

htuch · 2023-03-23T04:34:04Z

source/extensions/filters/http/composite/filter.cc

@@ -12,6 +12,16 @@ namespace Composite {
 namespace {
 // Helper that returns `filter->func(args...)` if the filter is not null, returning `rval`
 // otherwise.
+template <class FilterPtrT, class FuncT, class RValT, class... Args>


I'm wondering at the C++ code level why we need to distinguish single action vs. multiple actions with templates etc. Why not have the core implementation support multiple action, and when we instantiation the single action extension it sets N=1?

When I added this originally I looked into supporting delegating to multiple filters, but found that in order to preserve the existing filter API behavior you need to manage pausing iteration which blows up the complexity of this. Maybe now with the FilterManager being extracted out of the HCM this is something we can do, though I suspect it will make this much more complicated than it is right now

@htuch this had occurred to me when implementing the vectorized version of this class. I think that if we can define the type of behavior to expect when iterating over multiple filters, i.e. some default behavior. We can add upon that in the future and create more complex logic if necessary. Something I can discuss with @tyxia.

Yeah. Not suggesting we do the full thing that @snowp points out, but what I'm suggesting is to simplify your existing implementation. You have some template and conditional logic on whether this is a single or multi action. Why not just have it always the second case with N=1?

htuch · 2023-03-23T04:34:29Z

api/envoy/extensions/filters/http/composite/v3/composite.proto

@@ -37,3 +37,9 @@ message Composite {
 message ExecuteFilterAction {
  config.core.v3.TypedExtensionConfig typed_config = 1;
 }
+
+
+message ExecuteFilterMultiAction {


I feel we should have some better documentation here explaining what this is, why you would use it, how etc.

snowp · 2023-03-23T12:38:13Z

source/extensions/filters/http/composite/filter.cc

+RValT delegateAllFilterActionOr(std::vector<FilterPtrT> filter_list, FuncT func, RValT rval, Args&&... args) {
+  RValT value = rval;
+  for (auto filter : filter_list) {
+    value = ((*filter).*func)(std::forward<Args>(args)...);


How would this work if one of the intermediate filters rely on pausing filter iteration? Seems like we're going to return whatever the latest filter returns, which seems counter-intuitive

I was testing locally and noticed this, would reverse order be preferable? I was thinking about possible alternatives to this, some sort of logic for chaining subsequent actions? Or alternatively, I think the best solution would maybe be to store the most recent status calls in the StreamFilterWrapper sub-filters and just return the last one.

If you want this to work with any existing filter you'll end up having to maintain the existing behavior wrt filter control (pausing, resuming, etc), so I think you'll need to actually manage the filter start/stop/etc of each filter, possibly by reusing FilterManager to manage an internal set of filters

If you are ok with not supporting all filters this could be less powerful and we could document the limitations, but that makes this less useful than it could be

cc @alyssawilk @mattklein123 since filter iteration is complicated

Coming in late to this but why are we reimplementing filter manager logic in a filter? I'd lean towards reexamining the design here if that's the plan.

@alyssawilk, we are attempting to implement a mechanism primarily to run multiple ext_proc filter calls per route, i.e. multiple actions per filter. I am relatively new to the envoy code base and was unaware of this in regards to the composite filter.
That makes sense though.
I'll look into if this is possible through leveraging the FilterManager but another solution may be to come up with a different way to call multiple ext_proc filters.

Talked to Tianyu about this yesterday and I'm happy to whiteboard data plane options/complexity with anyone else interested. As usual I'll defer to Harvey/Adi if we go the control plane route :-)

The most interesting option that I don't fully understand is "dynamic HTTP filter chains", where somehow we augment the HTTP filter chain instantiation with per-route configuration. @alyssawilk do you have links or a dump of history on this one?

alas I don't have context on that one. I will say being able to enable and disable filters per route, or do custom filter chains per route seems entirely reasonable and if Envoy currently isn't flexible enough to do that in some dimension I'd be interested in making sure we fix that. I just think there's easier ways than having layered filter managers :-)

Here #14722 and this doc https://docs.google.com/document/d/1dOa_v1eMA4F61f0jdFkxtbcO-_TYD76oMfGoY7nzZhk/edit#heading=h.9v2rpr8krgni might be able to provide some context.

There has been asks for this for a while though I don't think we ever landed on a robust solution for it since there are a bunch of edge cases to consider, like what happens if the route changes mid-filter

Composite filters were an attempt at solving the "optional/branching filter" idea, with a focus on being deliverable via ECDS for a single filter. The idea was to let a separate system run an ECDS server and inject logic into the filter chain (at a pre-specified position in the filter chain) and deliver the matching requirement independent of other configuration, hence bundling the matching + filter config into something ECDS could deliver.

If we wanted the match component to live in the route configuration, I think we could get something working relatively easily by making use of https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/common/matcher/action/v3/skip_action.proto#envoy-v3-api-msg-extensions-filters-common-matcher-action-v3-skipfilter and having it read the decision from the per filter configuration or metadata from the route action. You could imagine configuring a sequence of ext_proc filters with something like

skip D unless route.metadata == "doTheThing"
skip E unless route.metadata == "doTheThing"
skip F unless route.metadata == "doTheThing"

snowp · 2023-03-23T12:39:26Z

source/extensions/filters/http/composite/filter.cc

@@ -12,6 +12,16 @@ namespace Composite {
 namespace {
 // Helper that returns `filter->func(args...)` if the filter is not null, returning `rval`
 // otherwise.
+template <class FilterPtrT, class FuncT, class RValT, class... Args>


When I added this originally I looked into supporting delegating to multiple filters, but found that in order to preserve the existing filter API behavior you need to manage pausing iteration which blows up the complexity of this. Maybe now with the FilterManager being extracted out of the HCM this is something we can do, though I suspect it will make this much more complicated than it is right now

tyxia · 2023-03-23T14:28:02Z

/assign @tyxia

tyxia · 2023-03-23T14:32:05Z

Thank you @htuch and @snowp for early feedback! This is still WIP and we will add more details and address the comments soon.

@jstraceski , good progress since yesterday's sync! Let's fix the format issue to get CI/CD running. I will send some command to you offline. btw, you can add WIP in the PR title so people will notice it is Work In Progress.

…scriptions. Signed-off-by: Joseph Straceski <jstraceski@google.com>

Signed-off-by: Joseph Straceski <jstraceski@google.com>

github-actions · 2023-04-29T20:01:27Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

htuch · 2023-04-30T19:00:40Z

@jstraceski @tyxia should we close this out in favor of the current approach you folks are taking to composition?

tyxia · 2023-05-11T01:53:05Z

@jstraceski @tyxia should we close this out in favor of the current approach you folks are taking to composition?

Yes, we will not proceed with this approach for our project at this moment.

The potential value of this PR is 1) allow multi-action/filters that doesn't need filter state management (like setResponseCode Filter). The value of this is limited and we need a big warning in the proto. 2) The setup PR for potential future multi-action that requires inner filter chain management.
Or we can just close this out.

jstraceski requested review from snowp and mattklein123 as code owners March 22, 2023 13:29

repokitteh-read-only bot added the api label Mar 22, 2023

repokitteh-read-only bot assigned adisuissa Mar 22, 2023

jstraceski marked this pull request as draft March 22, 2023 13:37

tyxia reviewed Mar 22, 2023

View reviewed changes

Adding MultiActions for composite filters.

6f7b26e

Signed-off-by: Joseph Straceski <jstraceski@google.com>

jstraceski force-pushed the MultipleAction branch from 3c642c3 to 6f7b26e Compare March 23, 2023 03:38

htuch reviewed Mar 23, 2023

View reviewed changes

snowp reviewed Mar 23, 2023

View reviewed changes

repokitteh-read-only bot assigned tyxia Mar 23, 2023

jstraceski force-pushed the MultipleAction branch from fa4c1e0 to 6f7b26e Compare March 24, 2023 14:54

jstraceski changed the title ~~Adding MultiActions for composite filters.~~ Adding MultiActions for composite filters. WIP Mar 24, 2023

jstraceski added 3 commits March 24, 2023 16:57

Adding test cases, fixing integration test, formatting, and adding de…

5e3a489

…scriptions. Signed-off-by: Joseph Straceski <jstraceski@google.com>

Fixing integration test.

7862311

Signed-off-by: Joseph Straceski <jstraceski@google.com>

Formatting changes

6a4920d

Signed-off-by: Joseph Straceski <jstraceski@google.com>

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 29, 2023

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 30, 2023

jstraceski closed this May 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding MultiActions for composite filters. WIP #26251

Adding MultiActions for composite filters. WIP #26251

jstraceski commented Mar 22, 2023

repokitteh-read-only bot commented Mar 22, 2023

tyxia Mar 22, 2023 •

edited

htuch left a comment

htuch Mar 23, 2023

snowp Mar 23, 2023

jstraceski Mar 23, 2023

htuch Mar 24, 2023

htuch Mar 23, 2023

snowp Mar 23, 2023

jstraceski Mar 23, 2023

snowp Mar 23, 2023

alyssawilk Mar 28, 2023

jstraceski Mar 28, 2023

alyssawilk Mar 30, 2023

htuch Mar 30, 2023

alyssawilk Mar 30, 2023

tyxia Mar 30, 2023

snowp Mar 30, 2023

snowp Mar 23, 2023

tyxia commented Mar 23, 2023

tyxia commented Mar 23, 2023

github-actions bot commented Apr 29, 2023

htuch commented Apr 30, 2023

tyxia commented May 11, 2023 •

edited

Adding MultiActions for composite filters. WIP #26251

Adding MultiActions for composite filters. WIP #26251

Conversation

jstraceski commented Mar 22, 2023

repokitteh-read-only bot commented Mar 22, 2023

tyxia Mar 22, 2023 • edited

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxia commented Mar 23, 2023

tyxia commented Mar 23, 2023

github-actions bot commented Apr 29, 2023

htuch commented Apr 30, 2023

tyxia commented May 11, 2023 • edited

tyxia Mar 22, 2023 •

edited

tyxia commented May 11, 2023 •

edited