grpc-json: add option to convert gRPC status into JSON body (#3383) #8009

ascheglov · 2019-08-22T16:46:48Z

When trailer indicates a gRPC error and there was no HTTP body, with the convert_grpc_status option enabled, take google.rpc.Status from the grpc-status-details-bin header and use it as a JSON body.
If there was no such header, make google.rpc.Status out of the grpc-status and grpc-message headers.

This also adds google.rpc.Status to user-provided protobuf descriptor.

Risk Level: Small-medium
Testing: Added unit and integration tests tests, tested manually.
Docs Changes:
Added field description in api/envoy/config/filter/http/transcoder/v2/transcoder.proto
Release Notes:
Fixes #3383

…xy#3383) When trailer indicates a gRPC error and there was no HTTP body, with the convert_grpc_status option enabled, take google.rpc.Status from the grpc-status-details-bin header and use it as a JSON body. If there was no such header, make google.rpc.Status out of the grpc-status and grpc-message headers. This also adds google.rpc.Status to user-provided protobuf descriptor. Risk Level: Small-medium Testing: Added unit and integration tests tests, tested manually. Docs Changes: Added field description in api/envoy/config/filter/http/transcoder/v2/transcoder.proto Release Notes: Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

lizan

This is awesome! Thanks for working on it.

source/common/grpc/common.cc

lizan · 2019-08-22T18:31:13Z

source/common/http/conn_manager_impl.cc

@@ -1501,7 +1501,8 @@ void ConnectionManagerImpl::ActiveStream::addEncodedData(ActiveStreamEncoderFilt
                                                         Buffer::Instance& data, bool streaming) {
  if (state_.filter_call_state_ == 0 ||
      (state_.filter_call_state_ & FilterCallState::EncodeHeaders) ||
-      (state_.filter_call_state_ & FilterCallState::EncodeData)) {
+      (state_.filter_call_state_ & FilterCallState::EncodeData) ||


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:

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.

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.

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.

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

With this only trailer-only responses are supported. This should be reverted when HCM can buffer data added from |encodeTrailers|. Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

repokitteh-read-only · 2019-08-28T10:08:08Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8009 was synchronize by ascheglov.

see: more, trace.

Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

repokitteh-read-only · 2019-08-28T10:14:31Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8009 was synchronize by ascheglov.

see: more, trace.

htuch · 2019-08-28T15:01:28Z

api/envoy/config/filter/http/transcoder/v2/transcoder.proto

+  // from the ``grpc-status-details-bin`` header and use it as JSON body.
+  // If there was no such header, make ``google.rpc.Status`` out of the ``grpc-status`` and
+  // ``grpc-message`` headers.
+  // This also adds ``google.rpc.Status`` to user-provided protobuf descriptor.


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.

lizan · 2019-08-28T21:54:41Z

tools/check_format.py

-SERIALIZE_AS_STRING_WHITELIST = ("./test/common/protobuf/utility_test.cc",
-                                 "./test/common/grpc/codec_test.cc")
+SERIALIZE_AS_STRING_WHITELIST = (
+    "./source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc",


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

Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

lizan

LGTM, one nit

source/common/protobuf/protobuf.h

lizan · 2019-09-03T20:52:25Z

@htuch for API.

htuch

Looks good, but confused by this comment block (deliberately reading it without any other review context to pretend to be an end user).
/wait

htuch · 2019-09-03T23:06:49Z

api/envoy/config/filter/http/transcoder/v2/transcoder.proto

+  //       google.rpc.RequestInfo request_info = 1;
+  //     }
+  //
+  // If a gRPC service uses the types from the ``google/rpc/error_details.proto``, its proto files


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

include/envoy/http/header_map.h

Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

itayd · 2019-09-19T05:12:03Z

/checkowners
/rk-ping

repokitteh-read-only · 2019-09-19T05:12:06Z

🙀 Error while processing event:

evaluation error
error: github.com/repokitteh/modules/ownerscheck.star:45:29: undefined: a

🐱

Caused by: a #8009 (comment) was created by @itayd.

see: more, trace.

itayd · 2019-09-19T05:12:51Z

/checkowners
/rk-ping

repokitteh-read-only · 2019-09-19T05:12:55Z

pong

🐱

Caused by: a #8009 (comment) was created by @itayd.

see: more, trace.

itayd · 2019-09-19T05:22:46Z

/checkowners
/rk-ping

repokitteh-read-only · 2019-09-19T05:22:49Z

pong

🐱

Caused by: a #8009 (comment) was created by @itayd.

see: more, trace.

itayd · 2019-09-19T05:24:06Z

/checkowners
/rk-ping

repokitteh-read-only · 2019-09-19T05:24:10Z

🙀 Error while processing event:

evaluation error
error: function _rk_error error: path contains forbidden characters

🐱

Caused by: a #8009 (comment) was created by @itayd.

see: more, trace.

itayd · 2019-09-19T05:24:58Z

/checkowners
/rk-ping

repokitteh-read-only · 2019-09-19T05:25:02Z

🙀 Error while processing event:

evaluation error
error: function _rk_error error: path contains forbidden characters

🐱

Caused by: a #8009 (comment) was created by @itayd.

see: more, trace.

itayd · 2019-09-19T05:25:10Z

/checkowners
/rk-ping

repokitteh-read-only · 2019-09-19T05:25:13Z

🙀 Error while processing event:

evaluation error
error: function _rk_error error: path contains forbidden characters

🐱

Caused by: a #8009 (comment) was created by @itayd.

see: more, trace.

itayd · 2019-09-19T05:25:40Z

/checkowners
/rk-ping

repokitteh-read-only · 2019-09-19T05:25:44Z

pong

🐱

Caused by: a #8009 (comment) was created by @itayd.

see: more, trace.

lizan · 2019-09-19T05:27:27Z

@itayd I'm about to merge this, lmk when you're done debugging the bot

itayd · 2019-09-19T05:34:22Z

/checkowners
/rk-ping

repokitteh-read-only · 2019-09-19T05:34:26Z

pong

🐱

Caused by: a #8009 (comment) was created by @itayd.

see: more, trace.

itayd · 2019-09-19T05:34:56Z

/checkowners
/rk-ping

repokitteh-read-only · 2019-09-19T05:35:00Z

pong

🐱

Caused by: a #8009 (comment) was created by @itayd.

see: more, trace.

itayd · 2019-09-19T05:35:52Z

sorry for the noise! but i found the repokitteh ownerscheck problem. thanks.

…e that the returned content type is `application/json`. This is required because the response body is an empty JSON body by default. Risk Level: Low Testing: Modified integration test to catch this bug. Docs Changes: None Release Notes: None Fixes: envoyproxy#5011 Ref: envoyproxy#8158 Ref: envoyproxy#8009 Signed-off-by: Teju Nareddy <nareddyt@google.com>

* When trailer indicates a gRPC error and there was no HTTP body, ensure that the returned content type is `application/json`. This is required because the response body is an empty JSON body by default. Risk Level: Low Testing: Modified integration test to catch this bug. Docs Changes: None Release Notes: None Fixes: #5011 Ref: #8158 Ref: #8009 Signed-off-by: Teju Nareddy <nareddyt@google.com>

…xy#3383) (envoyproxy#8009) When trailer indicates a gRPC error and there was no HTTP body, with the `convert_grpc_status option` enabled, take `google.rpc.Status` from the `grpc-status-details-bin` header and use it as a JSON body. If there was no such header, make `google.rpc.Status` out of the `grpc-status` and `grpc-message` headers. This also adds `google.rpc.Status` to user-provided protobuf descriptor. Risk Level: Small-medium Testing: Added unit and integration tests tests, tested manually. Docs Changes: Added field description in api/envoy/config/filter/http/transcoder/v2/transcoder.proto Release Notes: Fixes envoyproxy#3383 Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

…oxy#8312) * When trailer indicates a gRPC error and there was no HTTP body, ensure that the returned content type is `application/json`. This is required because the response body is an empty JSON body by default. Risk Level: Low Testing: Modified integration test to catch this bug. Docs Changes: None Release Notes: None Fixes: envoyproxy#5011 Ref: envoyproxy#8158 Ref: envoyproxy#8009 Signed-off-by: Teju Nareddy <nareddyt@google.com>

…xy#3383) (envoyproxy#8009) When trailer indicates a gRPC error and there was no HTTP body, with the `convert_grpc_status option` enabled, take `google.rpc.Status` from the `grpc-status-details-bin` header and use it as a JSON body. If there was no such header, make `google.rpc.Status` out of the `grpc-status` and `grpc-message` headers. This also adds `google.rpc.Status` to user-provided protobuf descriptor. Risk Level: Small-medium Testing: Added unit and integration tests tests, tested manually. Docs Changes: Added field description in api/envoy/config/filter/http/transcoder/v2/transcoder.proto Release Notes: Fixes envoyproxy#3383 Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

mattklein123 assigned lizan Aug 22, 2019

lizan reviewed Aug 22, 2019

View reviewed changes

Anatoly Scheglov added 3 commits August 27, 2019 22:36

fix wrong content-type; remove headers; more tests

948682e

Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

renames according to CC; nullopt

e64ae6a

Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

refactor tests

039e5bf

Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

lizan added the waiting label Aug 28, 2019

Remove support for errors in separate trailer frames.

b9ec933

With this only trailer-only responses are supported. This should be reverted when HCM can buffer data added from |encodeTrailers|. Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

repokitteh-read-only bot removed the waiting label Aug 28, 2019

fix a typo

b00edd0

Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

htuch reviewed Aug 28, 2019

View reviewed changes

lizan reviewed Aug 28, 2019

View reviewed changes

lizan previously approved these changes Aug 28, 2019

View reviewed changes

docs on details types

db62f47

Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

ascheglov dismissed lizan’s stale review via db62f47 August 30, 2019 11:23

lizan reviewed Aug 30, 2019

View reviewed changes

source/common/protobuf/protobuf.h Show resolved Hide resolved

lizan approved these changes Sep 3, 2019

View reviewed changes

htuch previously requested changes Sep 3, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 3, 2019

lizan previously approved these changes Sep 9, 2019

View reviewed changes

mattklein123 self-assigned this Sep 9, 2019

mattklein123 removed the waiting label Sep 9, 2019

mattklein123 reviewed Sep 10, 2019

View reviewed changes

include/envoy/http/header_map.h Outdated Show resolved Hide resolved

mattklein123 added the waiting label Sep 10, 2019

example of transcoded grpc-status-details-bin header

c81ae67

Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

ascheglov dismissed lizan’s stale review via c81ae67 September 11, 2019 10:51

repokitteh-read-only bot removed the waiting label Sep 11, 2019

lizan merged commit 219e8b9 into envoyproxy:master Sep 19, 2019

lizan mentioned this pull request Sep 19, 2019

Grpc transcoding: set content-type to json for tailer-only #8158

Closed

This was referenced Sep 20, 2019

Fix grpc transcoding content type not matching response body #8312

Merged

grpc-json: Handle streaming backend error cases correctly #8315

Closed

grpc-json: add option to convert gRPC status into JSON body (#3383) #8009

grpc-json: add option to convert gRPC status into JSON body (#3383) #8009

Conversation

ascheglov commented Aug 22, 2019

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Aug 28, 2019

repokitteh-read-only bot commented Aug 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

lizan commented Sep 3, 2019

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itayd commented Sep 19, 2019

repokitteh-read-only bot commented Sep 19, 2019

itayd commented Sep 19, 2019

repokitteh-read-only bot commented Sep 19, 2019

itayd commented Sep 19, 2019

repokitteh-read-only bot commented Sep 19, 2019

itayd commented Sep 19, 2019

repokitteh-read-only bot commented Sep 19, 2019

itayd commented Sep 19, 2019

repokitteh-read-only bot commented Sep 19, 2019

itayd commented Sep 19, 2019

repokitteh-read-only bot commented Sep 19, 2019

itayd commented Sep 19, 2019

repokitteh-read-only bot commented Sep 19, 2019

lizan commented Sep 19, 2019

itayd commented Sep 19, 2019

repokitteh-read-only bot commented Sep 19, 2019

itayd commented Sep 19, 2019

repokitteh-read-only bot commented Sep 19, 2019

itayd commented Sep 19, 2019