http: new body transform filter with the substitution formatter #41612
Conversation
Signed-off-by: wbpcode <[email protected]>
… dev-body-transform
Signed-off-by: wbpcode <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
cc @arkodg cc @agrawroh for you guys may care about this. cc @botengyao for this may helpful for AI traffic routing. |
Signed-off-by: WangBaiping <[email protected]>
Signed-off-by: WangBaiping <[email protected]>
| // Configuration for the transform filter. The request and response transformations are | ||
| // independent and could be configured separately. | ||
| // Only JSON body transformation is supported for now. | ||
| message TransformConfig { |
There was a problem hiding this comment.
nit: move this to be the first message as this is the filter's config.
|
|
||
| // If true and the request headers are transformed, Envoy will re-evaluate the target | ||
| // cluster in the same route. Please ensure the cluster specifier in the route supports | ||
| // dynamic evaluation, e.g., ``envoy.router.cluster_specifier_plugin.matcher``. |
There was a problem hiding this comment.
What would happen if there isn't a cluster-specifier-plugin-matcher defined?
Also, please add a reference to the matcher specification.
There was a problem hiding this comment.
Then this field will no effect. And I added the reference there.
| // to the existing body. If not set, the result of ``body_format_string`` will replace | ||
| // the existing body. | ||
| // True by default. | ||
| google.protobuf.BoolValue patch_format_string = 4; |
There was a problem hiding this comment.
As this is highly coupled with body_format_string and that is optional, would it make sense to move both to an internal message?
| // NOTE the ``RQ_BODY`` and ``RS_BODY`` format specifiers can also be used here. | ||
| config.core.v3.SubstitutionFormatString body_format_string = 3; | ||
|
|
||
| // If set the result of ``body_format_string`` will be treated as a patch and will be merged |
There was a problem hiding this comment.
What does it mean "patch"? does it imply that the format-string will be a regex and will replace subsections of the original body?
If it means that the result will be concatenated, I suggest replacing this to "replace_body" (because the default value should probably be "false"), or convert it to an enum that has "REPLACE_BODY"/"CONCATENATE_AT_END"/something-else (depending on the meaning of "patch").
There was a problem hiding this comment.
SGTM. This is a great suggestion.
| // And except the commonly used format specifiers, there are some additional format specifiers | ||
| // provided by the transform filter: | ||
| // | ||
| // * ``%RQ_BODY(X)%``: the request body. And ``X`` is the JSON pointer to extract a specific |
There was a problem hiding this comment.
Does this require a JSON-body? I may have missed it, but this seems to be a generic transform, and doesn't require JSON-body to be sent.
There was a problem hiding this comment.
JSON body is required. We have documented it that only JSON body is supported for now.
| // * ``%RQ_BODY(X)%``: the request body. And ``X`` is the JSON pointer to extract a specific | ||
| // field from the JSON body. If ``X`` is empty, the whole body will be used. | ||
| // * ``%RS_BODY(X)%``: the response body. And ``X`` is the JSON pointer to extract a specific | ||
| // field from the JSON body. If ``X`` is empty, the whole body will be used. |
There was a problem hiding this comment.
the whole body will be used
FWIW this might cause the headermap to go over the limit, and may be an attack vector, but as this filter doesn't have a security posture, it might be ok.
Also it would be good to clarify whether using this filter implies that all requests/responses are buffered, even if headers_mutations are defined without referencing RQ/RS_BODY. If there's a need to support both buffering and streaming modes, consider adding an enum field specifying the mode (and maybe only support one for now).
There was a problem hiding this comment.
At next few weeks, I will try to support the event stream for the response. We will handle it as stream for that. But for JSON request/response, buffering always be used.
|
|
||
| // Body transformation configuration. If not set, no body transformation will be performed. | ||
| // NOTE the ``RQ_BODY`` and ``RS_BODY`` format specifiers can also be used here. | ||
| config.core.v3.SubstitutionFormatString body_format_string = 3; |
There was a problem hiding this comment.
thoughts on using JSONPatch semantics here ?
https://jsonpatch.com
its used in Envoy Gateway API to edit xDS https://gateway.envoyproxy.io/docs/tasks/extensibility/envoy-patch-policy/#customize-response
There was a problem hiding this comment.
SGTM. But it's will have big effect to the current implementation. I will update the API to keep the patch mode as enum and then we can support it in the future.
Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: code <[email protected]>
Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: code <[email protected]>
Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: code <[email protected]>
Signed-off-by: WangBaiping <[email protected]>
| if (end_stream) { | ||
| decoder_callbacks_->addDecodedData(data, true); | ||
| handleCompleteRequestBody(); | ||
| return Http::FilterDataStatus::Continue; | ||
| } |
There was a problem hiding this comment.
Related but not tied / blocker to this PR, do you have some insights for the streaming JSON parser support without waiting for the whole body once we get the enough data. I think the current nlohmann lib doesn't quite support it, and we could need some SAX style approach.
There was a problem hiding this comment.
Maybe rapidJson could help? But we removed that because suggestions from google. 🤣
Signed-off-by: WangBaiping <[email protected]>
Signed-off-by: WangBaiping <[email protected]>
|
friendly ping for a new review cc @adisuissa |
|
/retest |
4 participants
Commit Message: http: new body transform filter with the substitution formatter
Additional Description:
To close #35783. There is enough context in the #35783. In short sentence, this PR add a transform filter to modify request or response headers by the substitution formatter API.
It's also possible to add body attributes to headers and refresh the routes.
Risk Level: low. New HTTP filter.
Testing: unit/integration.
Docs Changes: added.
Release Notes: added.
Platform Specific Features: n/a.