[ext_proc] Add extension point to run custom logic after onReceiveMessage #37916
Conversation
…sage Signed-off-by: pcrao <[email protected]>
pradeepcrao
requested review from
tyxia,
mattklein123 and
yanavlasov
as code owners
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
There was a problem hiding this comment.
Thanks!
Left a few high-level questions to start a discussion.
Also assigning @yanjunxiang-google and @tyxia as code-owners.
/assign @yanjunxiang-google @tyxia
| @@ -328,6 +329,8 @@ message ExternalProcessor { | |||
| // :ref:`mode_override <envoy_v3_api_field_service.ext_proc.v3.ProcessingResponse.mode_override>` is allowed by | |||
| // the ``allowed_override_modes`` allow-list below. | |||
| repeated ProcessingMode allowed_override_modes = 22; | |||
|
|
|||
| config.core.v3.TypedExtensionConfig on_receive_message_decorator = 23; | |||
There was a problem hiding this comment.
Please add a comment describing this extension. This should also clarify what types of extensions are allowed here.
It is unclear when this decorator is executed in the on-receive path (before processing the message or after). I'm assuming the former but this should be clarified.
There was a problem hiding this comment.
A few high-level questions/discussions:
- should this be a repeated field? would there be a need to invoke multiple decorators, or is the expectation that in this case a single decorator that invokes multiple decorators will be used (composed decorators)?
- Will it make sense to have a different decorator for the different steps (onHeaders/onData/onTrailers). I'm assuming not, or rather there should be different callback methods in a single decorator, but I want to make sure this was considered.
- Is this "reinventing" the filter lifecycle? I'm guessing we are not there yet (in terms of requirements), but if this is heading in that direction, then maybe we can adopt a few things from the filter interface.
There was a problem hiding this comment.
Thank you for the review Adi!
The decorator runs after the ext_proc filter has processed the message from the external processor. That way, we can react to the processing_status and handle errors appropriately.
-
It makes more sense for me to use a single decorator that might invoke multiple actions. This is meant to be lightweight, and not fundamentally alter the
ext_procfilter behaviour. -
Having a single decorator for all message types makes more sense. The logic for handling different message types can be implemented in the decorator itself.
-
The decorator
onReceiveMessagemethod takes the side stream response from the external processor, and theprocessing_statusof theext_procfilter as inputs, which is very specific to theext_procfilter. I am not aware of how we can adopt anything from the filter interface for this use case.
There was a problem hiding this comment.
Also, is there a canonical way I can document that the extension config specified here is meant to work with the OnReceiveMessageDecorator interface?
There was a problem hiding this comment.
Thank you for the review Adi!
The decorator runs after the
ext_procfilter has processed the message from the external processor. That way, we can react to theprocessing_statusand handle errors appropriately.
- It makes more sense for me to use a single decorator that might invoke multiple actions. This is meant to be lightweight, and not fundamentally alter the
ext_procfilter behaviour.- Having a single decorator for all message types makes more sense. The logic for handling different message types can be implemented in the decorator itself.
- The decorator
onReceiveMessagemethod takes the side stream response from the external processor, and theprocessing_statusof theext_procfilter as inputs, which is very specific to theext_procfilter. I am not aware of how we can adopt anything from the filter interface for this use case.
I agree that having a single decorator extension makes sense here. Just need to make sure that the right interface for that extension. IMHO it should either have onBefore/onAfter for each of the sub-messages (headers, data, trailers - so it will only invoke the correct method depending on the type of response), or specifically point out the methods similar to what is being done in the filter (e.g., similar to decodeHeaders()/encodeHeaders() that will work on either the headers being passed to the side-stream - the request - or returned from it - the response).
Another minor comment, I'm assuming this refers to receiving a ProcessingResponse message. I think it may be better to rename to on_processing_response and add a reference to that type.
RE the comment about the filters, I can see this being developed (in the future) similar to the way that filters have developed. First there were only downstream filters, and now there's also upstream filters. I think a similar future may be happening for the ext-proc case. I don't think that this is the correct PR to handle this, but it should probably be called out.
Also, is there a canonical way I can document that the extension config specified here is meant to work with the
OnReceiveMessageDecoratorinterface?
Typically one uses the extension-categories to express which extensions can be used in specific locations (see the stateful-session example [here](
Signed-off-by: pcrao <[email protected]>
…_message Signed-off-by: pcrao <[email protected]>
Signed-off-by: pcrao <[email protected]>
Signed-off-by: pcrao <[email protected]>
…_message Signed-off-by: pcrao <[email protected]>
Signed-off-by: pcrao <[email protected]>
…_message Signed-off-by: pcrao <[email protected]>
|
|
||
| // Decorator to introduce custom logic that runs after a message received from | ||
| // the External Processor is processed. | ||
| config.core.v3.TypedExtensionConfig on_processing_response = 23; |
There was a problem hiding this comment.
Current API LGTM, other than the extensions category.
Let's see if we can add a simple extension.
| public: | ||
| virtual ~OnProcessingResponse() = default; | ||
| virtual void | ||
| afterProcessingRequestHeaders(const envoy::service::ext_proc::v3::ProcessingResponse& response, |
There was a problem hiding this comment.
This is an interface and should probably have comments describing when the callbacks are invoked.
Also, not sure if it makes sense to have now, but there might be a need for a PassThrough class, that implements all the methods in a naive way. This way, one can override only the needed methods.
| @@ -1237,26 +1242,50 @@ void Filter::onReceiveMessage(std::unique_ptr<ProcessingResponse>&& r) { | |||
| case ProcessingResponse::ResponseCase::kRequestHeaders: | |||
| setDecoderDynamicMetadata(*response); | |||
| processing_status = decoding_state_.handleHeadersResponse(response->request_headers()); | |||
| if (on_processing_response_ != nullptr) { | |||
| on_processing_response_->afterProcessingRequestHeaders(*response, processing_status, | |||
There was a problem hiding this comment.
Can afterProcessingRequestHeaders() and afterProcessingResponseHeaders() be put under handleHeadersResponse()? That way, there is not too much logic directly under this switch... case .... state machine code.
Same for the handleBodyResponse() and handleTrailerResponse().
|
/wait for feedback to be addressed |
Commit Message:
Additional Description:
Fixes #34501
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features: