The change here worth another PR as it at least seems risky and need a test case for here. Why did you need this?

This is a tricky one.
When a gRPC server responds with 200, and then it sends a trailer with an error,
as in the GrpcJsonTranscoderIntegrationTest.UnaryGetErrorInTrailerConvertedToJson test,
or as it happens in this code:

func myHandler(w http.ResponseWriter, req *http.Request) {
  w.Header().Set("content-type", "application/grpc")
  w.Header().Set("trailer", "grpc-status")
  w.WriteHeader(200)
  // Then something goes wrong and we send a trailer with error:
  w.Header().Set("grpc-status", "5")
}

the transcoder is expected to send something like this:

$ echo -ne 'GET /func HTTP/1.1\r\nHost: localhost:8888\r\n\r\n' | nc localhost 8888
HTTP/1.1 404 Not Found
content-type: application/json
content-length: 10
date: Sat, 24 Aug 2019 20:56:19 GMT
x-envoy-upstream-service-time: 2
server: envoy

{"code":5}

However without this patch, first it sends the reply body with chunked transfer-encoding, and then it sends headers:

a
{"code":5}
HTTP/1.1 404 Not Found
content-type: application/json
content-length: 0
date: Sat, 24 Aug 2019 20:27:03 GMT
x-envoy-upstream-service-time: 12
server: envoy

(This "a" is the chunk size)

Supposedly when it does encodeData() in the if (EncodeTrailers) branch, it assumes that the headers were already sent, and maybe there was some part of reply-body sent, so it just dumps data as is.

Maybe there is a better option for fixing this, and yes I will also add a HttpConnectionManagerImplTest for this.

Great, thanks for the detailed write up. Yeah from this description it is a clear bug in HCM, can you please split that part out (with tests in HttpConnectionManagerImplTest) to another PR?

I've removed the changes in HCM and disabled the feature in case when there is a separate trailer frame.

To sum it ip, an upstream has three ways to report an error:

1. Send a single header frame with end_stream flag which is also a trailer.
   That's what happens when an unary function returns an error.

2. Send a header frame, send some data, and then send an error in a trailer.
   We don't transcode the error into JSON because that would replace the data sent by upstream.

3. Send a header frame, and then send an error in a trailer.
   This can happen when a gRPC sends metadata first, and then it encounters some error, e.g.

func (this *myServer) Func(...) (*pb.Ret, error) {
        ...
	grpc.SendHeader(ctx, someMetadata)
        ...
	return nil, someError
}

We don't support this yet, because HCM doesn't buffer data added from encodeTrailers.

Do you mean to say "This also adds google.grpc.Status to the available proto descriptors for the user-provide protobuf descriptor set"?

It's google.rpc.Status: https://github.com/googleapis/googleapis/blob/master/google/rpc/status.proto

I've removed this line since there is not need to tell users what they don't need to do, and added an example of what they have to do.

(Not actionable in this PR) cc @jmarantz adding extensions here is probably not ideal, perhaps we should support kind of NOLINT comment annotation.

I'm not grokking the example or this paragraph. Where does Dummy appear? In config? On the wire?

In the proto generating the descriptor set for transcoding.

Docs in this file already have two examples (for the match_incoming_request_route and the ignored_query_parameters options) that show user proto files used for making their proto descriptor.
And this is just another example of what should be written in user proto files.

This Dummy type is not used anywhere, but it's required as its contents ensures dependencies on types that have to be in a proto descriptor set.

for api approval: @mattklein123 since @htuch is OOO, do you have suggestion here? IMO this is enough given the examples in other fields does similar thing.

TBH I don't understand this example either, but I don't have a lot of context. Perhaps you could describe what the output looks like given a particular response?

Here is some context:
Upstream server responds with an error in the following headers frame:

grpc-status: 5
grpc-status-details-bin: CAUSElJlc291cmNlIG5vdCBmb3VuZA

The grpc-status-details-bin header contains base64 encoded protobuf google.rpc.Status message. We transcode it into

HTTP/1.1 404 Not Found
grpc-status: 5
content-type: application/json
...

{"code":5,"message":"Resource not found"}

Now, google.rpc.Status has the optional "details" field, which can hold arbitrary user-defined types. E.g. grpc-status-details-bin: CAUSBUVycm9yGjYKKXR5cGUuZ29vZ2xlYXBpcy5jb20vaGVsbG93b3JsZC5IZWxsb1JlcGx5EgkKB2RldGFpbHM will be transcoded into

{"code":5,"message":"Error","details":[{"@type":"type.googleapis.com/helloworld.HelloReply","message":"details"}]}

Since Envoy knows nothing about this helloworld.HelloReply type, it should be in the proto descriptor set provided via config. And the example says "make sure that your details types are in the the proto descriptor set". "If those details types aren't used in any message types of your service, make a dummy message type for them".

@ascheglov actually, I don't think you really need the message dummy, only a include of error_details.proto or just pass error_details.proto to protoc when you generate the descriptor should work. protoc doesn't drop unused message descriptors when it generate them.

Can you potentially just update the description/docs to read more like #8009 (comment)? That makes sense to me.

@ascheglov actually, I don't think you really need the message dummy, only a include of error_details.proto or just pass error_details.proto to protoc when you generate the descriptor should work. protoc doesn't drop unused message descriptors when it generate them.

Yes, that's true. With protoc --include_imports it's enough to just add an import.

@mattklein123 done