ext_proc: Store header mutations as filter state, to share across filters. about envoy HOT 17 CLOSED

Checkpoint: Have a offline discussion with @htuch @tyxia @stevenzzzz . For now, we agreed on " simple embedded extension point" is the best fit (it is cleaner, less action-at-a-distance across the filter pipeline than the filter state callback option). We should consider just having single extension point to accommodate ext_proc extensibility needs, rather than multiple extension points plugged in.

from envoy.

/assign @htuch

from envoy.

@tyxia for my perspective, it's hard to say this is a common feature for ext_proc filter.

The key of the inheritance way is to call back to the onReceivedMessage of OSS ext_proc.

from envoy.

Except the subclassing, other possible ways I can thought of may be:

1. a simple embedded extension point to inject additional logic during calling onReceiveMessage.
2. filter state, yeah, filter state again. At lots of different positions of Envoy, we use filter state to propagate some control options. Then, it's also possible here to use the filter state to propagate a specific callbacks/callable object to the ext_proc.

......

from envoy.

Take the stateful session as an example, https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/filters/http/stateful_session/v3/stateful_session.proto,
the stateful session self is an HTTP filter. And it define a new extension point in itself's API to let developers to implement the logic they want.

Specific to ext_proc, you can define a new extension point in the ext_proc (and related intefaces). Then load the related extension when loading ext_proc config. And finally, execute the extension in the onReceiveMessage method.

But this is still complex, compared to your simple requirement. So I will recommend the filter state way. To set a callback or function or some similar things, and execute in the onReceiveMessage method.

to re-clarify, what I am looking for is the actual header mutations applied by ext proc filter, excluding those mutations ignored/rejected by ext proc filters.

You can access both configuration and the received message from ext_proc server, so, I think this should also be easy?

from envoy.

Hah, I just found you are googler. You should have lots of Envoy experts that you could consult to. 😆

from envoy.

cc: @stevenzzzz @tyxia @yanavlasov @yurykats @htuch

from envoy.

@envoyproxy/envoy-triage @envoyproxy/envoy-maintainers Would love to get SGTM or feedback on this proposal from any triagers or maintainer before adding PR.

from envoy.

This sound a little weird to me...

Considering you are using the Envoy as a lib, may I assume that you can extend the Envoy by register new extension? Then I will suggest you to write a custom_ext_proc filter witch will inherit from ext_proc filter but only override the onReceiveMessage (and may the setDecoderFilterCallbacks and setEncoderFilterCallbacks to get filter callbacks if you cannot access them in your onReceiveMessage) method. Then you can do anything you want in the onReceiveMessage method. And at the end of your method, you can call back to the onReceiveMessage of OSS ext_proc.

This should not be too complex, I think. 🤔

from envoy.

In my opinion, the key considerations here are: (1) Is propagating mutation a natural responsibility of ext_proc (2)Will adding it be generally beneficial to OSS/ext_proc users

On one hand, we could argue that mutation (if happened) is indeed done by ext_proc. But then from design perspective this proposal probably should think about mutation in general, rather than header only. How about body and trailer mutation

Alternative approach if this FR is not generally desired in OSS: (1)before the ext_proc(i.e.,mutation),install a simple filter to store the original data (e.g., header) (2)after ext_proc, compute the mutation at the place where mutation diff is needed.

from envoy.

This sound a little weird to me...

Considering you are using the Envoy as a lib, may I assume that you can extend the Envoy by register new extension? Then I will suggest you to write a custom_ext_proc filter witch will inherit from ext_proc filter but only override the onReceiveMessage (and may the setDecoderFilterCallbacks and setEncoderFilterCallbacks to get filter callbacks if you cannot access them in your onReceiveMessage) method. Then you can do anything you want in the onReceiveMessage method. And at the end of your method, you can call back to the onReceiveMessage ext_proc.

This should not be too complex, I think. 🤔

That sounds pretty reasonable. cc: @yurykats

from envoy.

This sound a little weird to me...

Considering you are using the Envoy as a lib, may I assume that you can extend the Envoy by register new extension? Then I will suggest you to write a custom_ext_proc filter witch will inherit from ext_proc filter but only override the onReceiveMessage (and may the setDecoderFilterCallbacks and setEncoderFilterCallbacks to get filter callbacks if you cannot access them in your onReceiveMessage) method. Then you can do anything you want in the onReceiveMessage method. And at the end of your method, you can call back to the onReceiveMessage ext_proc.

This should not be too complex, I think. 🤔

@wbpcode It is a interesting idea. However, one concern is that: creating custom ext_proc filter (I.e., subclassing)could lead to diverge in the long term.
e.g., (1)Which product use which ext_proc ; (2)effort on consistency : For example to the example above, if OSS add new feature/fix to onReceiveMessage , we need to sync it in custom ext_proc

As long as the feature is generally beneficial to OSS, we should try to keep single source of truth. cc @htuch

from envoy.

This sound a little weird to me...
Considering you are using the Envoy as a lib, may I assume that you can extend the Envoy by register new extension? Then I will suggest you to write a custom_ext_proc filter witch will inherit from ext_proc filter but only override the onReceiveMessage (and may the setDecoderFilterCallbacks and setEncoderFilterCallbacks to get filter callbacks if you cannot access them in your onReceiveMessage) method. Then you can do anything you want in the onReceiveMessage method. And at the end of your method, you can call back to the onReceiveMessage ext_proc.
This should not be too complex, I think. 🤔

@wbpcode It is a interesting idea. However, one concern is that: creating custom ext_proc could lead to diverge in the long term. e.g., (1)Which product use which ext_proc ; (2)effort on consistency : For example to the example above, if OSS add new feature/fix to onReceiveMessage , we need to sync it in custom ext_proc

As long as the feature is generally beneficial to OSS, we should try to keep single source of truth. cc @htuch

I actually think @wbpcode 's idea is not bad.
the subclass only latch the returned mutations to filterState and the desired goal is achieved. call parent::onReceiveMessage to rule out any "sync" concern.

from envoy.

Not a huge fan of subclassing, this can be a pretty fragile way to impedance match OSS and in-house code, and introducing a new extension, but I think we definitely could structure this sensibly with callbacks and the ability to control what happens to headers during onReceiveMessage. Basically what @wbpcode said but without subclassing and adding a new extension. I agree that hard coding the header mutations in filter state as a fixed behavior is not very reusable outside our use case.

from envoy.

@wbpcode Can you kindly share an example of "embedded extension point"?

Also, to re-clarify, what I am looking for is the actual header mutations applied by ext proc filter, excluding those mutations ignored/rejected by ext proc filters. And I am specifically interested in a way extracting the mutations directly, and trying to avoid an option deriving the mutations by diffing the post-mutations headers with pre-mutations headermap. (Simply because the "diffing" option can be more easily implemented in our application code without an Envoy change, yet it has drawback of being more CPU/RAM expensive than the "direct extraction" options).

from envoy.

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

from envoy.

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

from envoy.