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

Filter can't add data from encodeTrailers #8402

Closed
ascheglov opened this issue Sep 26, 2019 · 0 comments · Fixed by #8404
Closed

Filter can't add data from encodeTrailers #8402

ascheglov opened this issue Sep 26, 2019 · 0 comments · Fixed by #8404
Labels
enhancement Feature requests. Not bugs or questions.
Milestone

Comments

@ascheglov
Copy link
Contributor

Calling addEncodedData from the encodeTrailers callback fails when encodeHeaders had previously returned StopIteration.

A filter (e.g. the grpc-json transcoder) wants to turn a headers-only response into a response with a body. And StreamEncoderFilterCallbacks::addEncodedData allows that:

 1) If a headers only response needs to be turned into a response with a body, this method can
 be called to add body in the encodeHeaders() callback. Subsequent filters will receive
 encodeHeaders(..., false) followed by encodeData(..., true). This works both in the direct
 iteration as well as the continuation case.

However it doesn't work when upstream sends a header frame and a trailer frame:
The filter returns StopIteration from encodeHeaders.
In encodeTrailers, it calls addEncodedData.
ConnectionManagerImpl::ActiveStream::addEncodedData has a special branch for EncodeTrailers, which calls encodeData with the comment:

// In this case we need to inline dispatch the data to further filters. If those filters
// choose to buffer/stop iteration that's fine.

Now there are two cases:

  1. if there is another filter, encodeData will call its encodeData callback, and then fail the ASSERT(headers_continued_) in ActiveStreamFilterBase::commonHandleAfterDataCallback - this filter never had its encodeHeaders called.
  2. if there is no filter to iterate, ActiveStream::encodeData will call response_encoder_->encodeData and write response body before writing any headers.

For HTTP/1.1 downstream it will assume chunked transfer-encoding and write something like:

4
body
HTTP/1.1 200 OK
other: headers

How to fix that:

For EncodeTrailers, the ActiveStream::addEncodedData should buffer data when filter stopped iteration.

ascheglov pushed a commit to ascheglov/envoy that referenced this issue Sep 26, 2019
Now filters can return StopIteration from encodeHeaders,
and then call addEncodedData from encodeTrailers.

This allows the JsonTranscoderFilter to properly transcode gRPC status
in trailer headers into a JSON reply body.

Risk Level: Low
Testing: unit-tests, integration tests in gRPC JSON transcoder filter.
Documentation: n/a
Release notes: n/a
Fixes envoyproxy#8402

Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>
@mattklein123 mattklein123 added the enhancement Feature requests. Not bugs or questions. label Sep 27, 2019
@mattklein123 mattklein123 added this to the 1.12.0 milestone Sep 30, 2019
mattklein123 pushed a commit that referenced this issue Oct 1, 2019
Now filters can return StopIteration from encodeHeaders,
and then call addEncodedData from encodeTrailers.

This allows the JsonTranscoderFilter to properly transcode gRPC status
in trailer headers into a JSON reply body.

Risk Level: Low
Testing: unit-tests, integration tests in gRPC JSON transcoder filter.
Documentation: n/a
Release notes: n/a
Fixes #8402

Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>
danzh2010 pushed a commit to danzh2010/envoy that referenced this issue Oct 4, 2019
…yproxy#8404)

Now filters can return StopIteration from encodeHeaders,
and then call addEncodedData from encodeTrailers.

This allows the JsonTranscoderFilter to properly transcode gRPC status
in trailer headers into a JSON reply body.

Risk Level: Low
Testing: unit-tests, integration tests in gRPC JSON transcoder filter.
Documentation: n/a
Release notes: n/a
Fixes envoyproxy#8402

Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants