Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ext_proc: Returning StopAllIterationAndWatermark prevents whole request processing #15793

Closed
bladedancer opened this issue Mar 31, 2021 · 10 comments
Labels
enhancement Feature requests. Not bugs or questions. stale stalebot believes this issue/PR has not been touched recently

Comments

@bladedancer
Copy link

bladedancer commented Mar 31, 2021

I've an integration scenario where I want to use ext_proc to call out to an external service. This service is going to process the request and may update headers and/or body - e.g. redaction, signature generation, transcoding, etc.

The external service needs the entire request before it can be processed. The problem is ext_proc stops after sending the headers and awaits a response:
https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/http/ext_proc/ext_proc.cc#L91

It needs a response before sending the body but at this point the service has insufficient information to respond. If it was possible to return StopIteration then (as I understand it) the body decoding would proceed and the service could get all the information before responding.

My question for this issue is why does the limitation exist? I've seen the same in the WASM filter proxy-wasm/proxy-wasm-cpp-host#143 and have seen the related discussion on #15496 wonder. Is the reasoning the same for ext_proc?

Or can/will ext_proc support such usecases?

I realize it's not necessarily trivial to support the concurrent requests. I don't necessarily need StopIteration - I need to be able to mutate headers after receiving the body. So options like allowing the header decoding to return Continue and have the data decoding handle processing responses that aren't oneofs would be fine.

@bladedancer bladedancer added the triage Issue requires triage label Mar 31, 2021
@bladedancer bladedancer changed the title ext_proc: Returnign StopAllIterationAndWatermark prevents whole request processing ext_proc: Returnig StopAllIterationAndWatermark prevents whole request processing Mar 31, 2021
@bladedancer bladedancer changed the title ext_proc: Returnig StopAllIterationAndWatermark prevents whole request processing ext_proc: Returning StopAllIterationAndWatermark prevents whole request processing Mar 31, 2021
@wbpcode
Copy link
Member

wbpcode commented Apr 1, 2021

ext_proc is designed to look like this. And wasm did some restrictions.

@snowp
Copy link
Contributor

snowp commented Apr 1, 2021

@gbrail

@gbrail
Copy link
Contributor

gbrail commented Apr 16, 2021

The PR for this is in progress: #15935

@bladedancer
Copy link
Author

@gbrail - I'm reading it right (and I may not be) the logic is send headers and then you either buffer or you watermark waiting for the reply.

If my body is larger than the buffer and I need the entire body before the header response then this will fail, right? Presumably this is expected as it only supports buffered mode at the moment.

When it's streaming I assume this limitation will be removed? I'll be able to stream large bodies unbuffered?

@gbrail
Copy link
Contributor

gbrail commented Apr 16, 2021

If you need to modify the headers only after examining the entire body, then the only way for that to work is for Envoy to buffer the entire body. That's because if you're going to modify the headers, then Envoy can't send any headers until you are done deciding what to modify, and it can't send any bytes of the body until it's sent all the headers. So implementing any kind of streaming processing in ext_proc won't change this.

If you have another use case that could be accomplished using streaming, I'd love to hear about it -- right now I'm not entirely sure what can be accomplished using the existing filter and data flow model and I want to make sure that we have the right use cases.

@bladedancer
Copy link
Author

For my scenario I'm offloading the entire request to the external processor - it'll buffer (if needed) and then stream the modified body back in the processor response. And then on to the next filter in the stack.

I am definitely not familiar enough with the Envoy internals to figure out how this works internally.

@gbrail
Copy link
Contributor

gbrail commented Apr 23, 2021

The original problem has been fixed in #15935.

I'm still interested in understanding how people want to use various body processing modes. For example, if you don't need to modify the headers then a streaming mode would be possible, but if you want to modify it in a streaming way then there are some questions about the Envoy filter chain that I need to get answers for.

@antoniovicente
Copy link
Contributor

cc @alyssawilk

I think there's still the question of how a filter could send request headers and body to an external service that generates a new set of headers and body that is then forwarded to an upstream.

I'm guessing this is possible by returning StopIteration from decodeHeaders and decodeData, and injecting data past this filter once the external service has done the necessary computation and returned alternate request headers and body. Ideally the proxy wouldn't buffer the original body when operating in this mode.

@antoniovicente antoniovicente added enhancement Feature requests. Not bugs or questions. and removed triage Issue requires triage labels May 10, 2021
@github-actions
Copy link

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.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 10, 2021
@github-actions
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions. stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

No branches or pull requests

5 participants