envoyproxy / envoyproxy/envoy

[ext_proc] let ext_proc filter follow the grpc spec, wait for trailers before continue the filter chain upon last main stream end_stream=true event.

Open
#37,088 15 comments 0 reactions 3 assignees Claimed by @yanavlasov View on GitHub
area/ext_proc bug no stalebot
Dominant language
C++
Stars
28.9k
Forks
5.6k
Avg merge
1d 22h
Merged PRs (30d)
430

Description

*Title*: ext_proc side grpc stream should follow grpc spec to wait for trailers to end a session.

*Description*:

In Grpc, the trailers carries the grpc-status header terminates a rpc stream. see https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses for the spec and https://carlmastrangelo.com/blog/why-does-grpc-insist-on-trailers for why.

The current impl of closing a stream in ext_proc:
https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/http/ext_proc/client_impl.cc#L75-L78
Our current grpc flow in ext_proc filter ignores the trailers by closeStream and resetStream all together, the later resetStream call would signal the remote server a CANCEL, while clean the sidestream and ignore any possible trailers that might have been sent by ext_proc server.

Instead, for the last de/encode{Headers,data} call, ext_proc filter should fire a half-close (closeStream), and wait until the trailers come back, then call continueProcessing().

In PR #37083 we moved one step towards this goal: split half-close from reset(), but we would like to move the half-close earlier to the filter callbacks, before onDestroy gets called.

Noteworthy that there are special case in GRPC: Headers only response carries grpc-status in response headers, in which case we should proactively close the side stream as well.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.