Always process HTTP headers before processing HTTP body.

[PiotrSikora]

Signed-off-by: Piotr Sikora [email protected]

[mathetake]

looks fine but I would like to know then why FilterHeadersStatus::StopIteration exists?

[mathetake]

one thing I can think of is other SDKs (TinyGo, Rust) has only two enum values (0, 1) and 1 corresponds to StopIteration.

[PiotrSikora]

I honestly don't know why FilterHeadersStatus::StopIteration exists, it breaks all assumptions about HTTP processing I know. This will fix TinyGo and Rust SDKs to properly pause all requests when they return Pause.

[mathetake]

so we would better thinking about removing FilterHeadersStatus::StopIteration enum value in order to make these repositories proxy-agnostic?

[mathetake]

anyway, LGTM from me

[PiotrSikora]

Yeah, but that's pending ABI update (proxy-wasm/spec#1). I'll work on finalizing that document once we fix the few outstanding bugs in Proxy-Wasm in Envoy.

[gbrail]

This certainly fixes some problems that I've seen, but I wonder if we know enough about all the various use cases for this to be a good thing -- perhaps it's better to change this just in the Rust SDK, since the C++ SDK already supports all the return codes, and lots of existing Envoy filters do indeed use StopIteration.

[mathetake]

and lots of existing Envoy filters do indeed use StopIteration.

cc @lizan

[mathetake]

verified that only a few existing native filters use FilterHeadersStatus::StopAllIterationAndWatermark. I'm now a little bit confused 😕

[PiotrSikora]

@gbrail but this is not specific to Rust SDK.

Notably, if you try to resume processing after returning StopIteration from headers, Envoy will crash because of this assert:

[source/common/http/filter_manager.cc:56] assert failure: !canIterate(). Details: Attempting to continue iteration while the IterationState is already Continue

so any malicious Proxy-Wasm plugin could crash Envoy without this change.

Also, I honestly don't understand why this flow exists in the first place. It's extremely error-prone, and the few Envoy filters that return StopIteration buffer request body within the filter itself until the request is unpaused, which is something we definitely don't want to do within Proxy-Wasm plugins.

[bianpengyuan]

Envoy now considers local_reply + non-StopInteration as a bug: https://github.com/envoyproxy/envoy/blob/d61fdbabbf0010a1769c10a87b63e32969f6efb6/source/common/http/filter_manager.cc#L511

Should we add StopInterationAndWaterMark in that bug statement?

[bianpengyuan]

@PiotrSikora ^^

[PiotrSikora]

Envoy now considers local_reply + non-StopInteration as a bug: https://github.com/envoyproxy/envoy/blob/d61fdbabbf0010a1769c10a87b63e32969f6efb6/source/common/http/filter_manager.cc#L511

Should we add StopInterationAndWaterMark in that bug statement?

Yeah, StopInteration alone is too strong of a requirement there, IMHO.

Alternatively, if Envoy doesn't want to make this change, then we can track whether or not the local reply was sent and adjust the status as needed.

cc @asraa

[asraa]

There was some discussion about this in the PR: envoyproxy/envoy#14416 (comment)

The condition we want to avoid is local_reply + continue. But, you should still be able to continue safely even if you return StopIterationAndWatermark (see release build test). We can change that to just local_reply + Continue, I think there just wasn't a rationale.

[PiotrSikora]

@asraa StopAllIterationAndWatermark is also used by ext_authz and ext_proc, and I've run into crashes because of that ENVOY_BUG already, so we should definitely relax this restriction and test for what you really wanted to prevent (i.e. assert status != FilterHeadersStatus::Continue).

[asraa]

Generally, I am fine with loosening the condition to the minimal constraint. I can make a PR.
Are either of these filters sending a local reply and then StopAllIterationAndWatermark? It didn't seem to me, but I could be mistaking the flow.

Are people using debug builds? If so, that kind of defeats the purpose of these ENVOY_BUGs that aren't meant to crash users, but just inform them they might be doing something wrong.

Unrelated, looking at the !canIterate mentioned above, I think we should change that to be able to continue safely and just log the error.

[PiotrSikora]

ext_authz sends local reply followed by StopAllIterationAndWatermark.

In Wasm, we only return Continue or StopAllIterationAndWatermark, and never StopIteration, since the semantics of the latter are completely broken, IMHO.

I think there is a lot of value in ENVOY_BUG, but it probably shouldn't result in a crash until all first-party extenions are clean of those bugs, and all invalid ENVOY_BUG conditions are fixed.

[asraa]

Sounds good, I'll work on a PR to fix and try to reproduce ext_authz sending local reply and then stopall.

Thanks for the info about WASM, in that case, the minimal constraint fits.

It only results in crashes in debug mode though (in which case you are susceptible to faulty ASSERTs), do you think they should just be logging critical in debug mode too?

[PiotrSikora]

It only results in crashes in debug mode though (in which case you are susceptible to faulty ASSERTs), do you think they should just be logging critical in debug mode too?

Probably for next release or two... or at least make them non-crashing in the release branches, and leave crashes in main.