Turning upstream response into header only downstream response

[snowp]

I've been trying to use a StreamEncodingFilter to turn a bad upstream response into a header only gRPC response that sets grpc-message to something useful. The point is to provide a understandable error message rather than having the gRPC client attempt to parse what I can tell from the headers is not going to be the correct format.

The problem I'm running into is that I'm not able to get the filter to not respond with any DATA frames.

Tried a few things:

1. draining the buffer in each encodeData. This still results in a zero length DATA frame:

2018/10/20 12:00:24 http2: Framer 0xc4201a0000: read DATA flags=END_STREAM stream=7 len=0 data=""

3. use sendLocalReply on the decoder callbacks while inside encodeHeaders. This hits the following assert:

    ASSERT(!(state_.filter_call_state_ & FilterCallState::EncodeHeaders));

and presumably this is a bad idea since it will loop back into my encoder filter again.

Is there a way to do what I'm accomplishing? If not, would it be a reasonable thing to add?

[mattklein123]

Yeah, this is a reasonable thing to add. It would require some thinking on the best way to do this. Fundamentally, the filter needs to be able to change the end_stream field during propagation. This won't be trivial.

As an aside, have you tried just having the 0 length data frame sent with end stream true? I suspect most clients will deal with this correctly without complaining, but I'm not sure.

[snowp]

I think the data frame is being set with end stream true already since the upstream response doesn't have any trailers to begin with (I'm transforming http/1.1 into gRPC). The Go client in particular complains about missing trailers:

2018/10/20 12:05:10 call failed: rpc error: code = Internal desc = server closed the stream without sending trailers

The go-grpc debug log line that I posted seems to agree (note the flags=END_STREAM part):

2018/10/20 12:00:24 http2: Framer 0xc4201a0000: read DATA flags=END_STREAM stream=7 len=0 data=""

Does Envoy even allow me to modify the flags on h/2 frames? Or was your suggestion to have upstream set the flag somehow?

[snowp]

Would a basic outline of the implementation look something like this:

Add a function to the callbacks called endStream() or headersOnly() that can be called during encodeHeaders that will change end_stream to true for subsequent filters and prevent encodeData from being called

We could do this by storing an early_end_stream_ flag on ActiveStream that gets set to true when the callback function is called. The end_stream parameter in encodeHeaders would be determined by evaluating end_stream || early_end_decoding_stream_.

We'd check for an early_end_stream_ when receiving data/trailers from upstream, and drop it on the floor if we've already completed the request. This might involve clearing out the buffers? Since we're not longer just shuffling them from upstream -> downstream.

I'm sure I'm missing some details here, but if this sounds reasonable I'd be happy to spend some time to get it working.

[mattklein123]

The Go client in particular complains about missing trailers:

OK, nevermind then. :)

Does Envoy even allow me to modify the flags on h/2 frames? Or was your suggestion to have upstream set the flag somehow?

No, we will have to make a code change as you suggest.

I'm sure I'm missing some details here, but if this sounds reasonable I'd be happy to spend some time to get it working.

I think what you are proposing would work, but I wonder if it would be simpler to have this be a return code from decode/encode headers. Basically something like "continue iteration and end stream." This would avoid introducing another callback function. @alyssawilk WDYT?

[alyssawilk]

Yeah, I think adding a response code or changing bool end_stream -> bool& end_stream is the way to go. The one other thing we'll have to be careful about is either draining or closing the upstream connection if we send a local end stream without consuming the whole HTTP/1.1 error body.

[lizan]

I think the data frame is being set with end stream true already since the upstream response doesn't have any trailers to begin with (I'm transforming http/1.1 into gRPC). The Go client in particular complains about missing trailers:

This is described in gRPC HTTP2 protocol mapping:
https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses

So a gRPC response in H2 frames should be either:
HEADERS, DATA, HEADERS (end_stream)
or
HEADERS (end_stream)

Is there a way to do what I'm accomplishing? If not, would it be a reasonable thing to add?

Sorry I'm late on this, I think you can use addEncodedTrailers to add grpc-status/grpc-message after the empty DATA frame to make the gRPC client happy. Though I agree #4885 is more useful in general :-)

[snowp]

I think you can use addEncodedTrailers to add grpc-status/grpc-message after the empty DATA frame to make the gRPC client happy.

Yep, that's what I'm currently doing. It works but it means setting a flag in encodeHeaders so that I can call drain and addEncodedTrailers in encodeData. Being able to return immediately makes the API much cleaner