-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Pausing filter chain on http request body doesn't work for dynamic forward proxy. It works when envoy is reverse-proxy. #17514
Comments
Just to get a bit more detail on this, it looks like you tried to stop iteration on the request body but your logs show that after DNS succeeded the router continued processing request headers. If you tell Envoy to stop iteration on the body it doesn't stop processing headers, but it should hold back the body until you're done processing that. Are you seeing the body processed as well or was there a misunderstanding on what you're trying to pause? |
Thank you for looking into this, @alyssawilk. Yes, your description matches my intention. In other words, I want to sanitize request body before it's sent to upstream. It's ok if the request headers are continued and sent to upstream. For "Are you seeing the body processed as well", I think so. The whole request body were sent to upstream without waiting for the callout reply. This problem only happens when envoy is running as dynamic forward proxy. With the same filter code, if I run envoy as reverse-proxy, the request body was held until I do resume_http_request() in the callout reply. That's why I suspect it's a bug in dynamic forward proxy when DNS is completed and filters are continued. |
hm, the logs pasted only show forwarding the headers. any chance you have a more complete trace of this happening? |
Yes. Here is the complete trace and my filter code:
And below is my filter code in rust, which is based on https://github.com/proxy-wasm/proxy-wasm-rust-sdk/blob/master/examples/http_auth_random.rs
|
I'm no wasm expert but I think the DFP is doing the correct thing here, so maybe @mathetake or @PiotrSikora can take a look at the rust? From a C++ perspective, DFP does call commonContinue() when it gets the DNS resolution, but commonContinue should detect that the wasm filter returned StopIterationAndSomething and continue to not forward data. I believe #17580 regression tests that DFP is working as expected. |
DFP calls I'm not sure how DFP could continue, but not forward data? |
@dandlake do you have DFP configured after or before Wasm? I think it should work if you put DFP before Wasm, since then DFP will pause and resume before passing request to the next filter, so it won't interfere with the Wasm code. Basically, I think (but I didn't verify) that with HTTP filters
But with HTTP filters
|
I had Wasm filter before DFP, and yes, reversing the wasm and DFP filters works. Thank you for the suggestion, @PiotrSikora! While this works, do you see any downside of putting Wasm after DFP? I tested a few cases on my side and it seems fine, but want to see if you foresee any potential issue. On the other hand, do you think step 5 above can be fixed so that request headers are continued(in step 4) but request body remains paused(since an earlier filter paused it)? Not sure if this question is for @alyssawilk or @PiotrSikora. Any other non-wasm filter can potentially return StopIterationAndSomething for request body. Do you want to support this case? or do you require other filters stay after DFP? |
@dandlake yeah, it's definitely a bug (so please leave this open) and the reverse order should be supported, I just wanted to unblock you, since I imagine that putting DFP first should work for 95% of use cases. |
continueDecoding is a complicated function. It's a per filter call* so should undo the pause of that specific filter. *hhttps://github.com/envoyproxy/envoy/blob/main/source/common/http/filter_manager.cc#L69 |
@alyssawilk right, but the "per filter" envoy/source/common/http/filter_manager.cc Lines 321 to 332 in 79ade4a
|
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. |
@alyssawilk, @PiotrSikora Should I keep this issue open or should I leave it and let github close it? |
Definitely leave it open, but I'm not sure if @alyssawilk or anybody else is actively working on it. |
yeah it's backburner for me so left it unassigned but I'll tag no-stalebot. |
I have a HTTP WASM filter where I make a http callout in the request body callback. After the callout, I return FilterDataStatus::StopIterationAndBuffer(which is 1) for this callback and want envoy to pause and wait for the callout reply to decide to stop or continue.
This works when envoy is reverse-proxy. However, when envoy is dynamic-forward-proxy, the pause didn't happen for the request body and the filter chain continued for the request body.
Looking at the log, for dynamic forward proxy, it's possible that after DNS is resolved asynchronously(which was triggered by http request headers), when it tried to continue the filter chain, it didn't check for the filter chain's current status and continued even when a previous filter returned FilterDataStatus::StopIterationAndBuffer.
In the above log, when "load DNS cache complete, continuing", looks like it didn't check the fact that a filter previously returned 1(FilterDataStatus::StopIterationAndBuffer) for the request body. Can someone confirm if this is a bug? I can provide the repro code if needed.
The text was updated successfully, but these errors were encountered: