grpc-json: Handle streaming backend error cases correctly by ascheglov · Pull Request #8323 · envoyproxy/envoy

ascheglov · 2019-09-22T21:58:19Z

When a streaming backend returns an error in a trailer-only response,
with the 'convert_grpc_status' option enabled,
instead of writing the [] (empty array) in reply-body
transcode the error in the headers and reply with {"code":...}.

Risk Level: Low
Testing: added an integraion test, tested manually.
Documentation: n/a
Release notes: n/a
Fixes #8315

When a streaming backend returns an error in a trailer-only response, with the 'convert_grpc_status' option enabled, instead of writing the [] (empty array) in reply-body transcode the error in the headers and reply with {"code":...}. Risk Level: Low Testing: added an integraion test, tested manually. Documentation: n/a Release notes: n/a Fixes envoyproxy#8315 Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

ascheglov · 2019-09-23T09:47:11Z

On second thought, this could be improved.

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

qiwzhang · 2019-09-23T16:12:13Z

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

  const absl::optional<Grpc::Status::GrpcStatus> grpc_status =
      Grpc::Common::getGrpcStatus(trailers);
  bool status_converted_to_json = grpc_status && maybeConvertGrpcStatus(*grpc_status, trailers);
+  if (status_converted_to_json) {


The code is very hard to follow with so many if-else. Can we split them into two functions, one handling all logic of convert_to_json=true case, and another handling false case. Not need to worry about sharing code between them.

Done, moved all the gRPC-status-to-JSON code into a separate function.

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

qiwzhang · 2019-09-24T15:59:47Z

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


+  response_headers_->Status()->value(Grpc::Utility::grpcToHttpStatus(grpc_status));
+
+  if (is_trailers_only_response) {


not need to have this if. non_trailer_only_response has been bailed out in line 560

true, however I'm planning to remove that if (!is_trailers_only_response) return in next PR, which would fix the http connection manager so that it would allow to add data from the encodeTrailers handler.

does that mean you're going to handle status to json conversion in non trailers_only_response case?

Yes, I'll remove this in #8404 .

We can transcode status into json as long as upstream doesn't send its own reply body.
That includes a single headers frame with the end_stream flag (i.e. trailers-only), and two headers frames (headers+trailers).

Removed it.

qiwzhang · 2019-09-24T16:01:08Z

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

+  // remove Trailer headers if the client connection was http/1
+  if (encoder_callbacks_->streamInfo().protocol() != Http::Protocol::Http2) {
+    static const Http::LowerCaseString trailer_key = Http::LowerCaseString("trailer");
+    response_headers_->remove(trailer_key);


do we need to remove "trailer" header for non_json_converting case?

yes, and it's already there, in encodeTrailers

lizan

Great, thanks!

lizan · 2019-09-24T19:49:48Z

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

+
+  // remove Trailer headers if the client connection was http/1
+  if (encoder_callbacks_->streamInfo().protocol() != Http::Protocol::Http2) {
+    static const Http::LowerCaseString trailer_key = Http::LowerCaseString("trailer");


(I know this were here before) since you're here, can you make this pointer or extract to a method with CONSTRUCT_ON_FIRST_USE, so doesn't cause static-initialization fiasco?

nareddyt · 2019-09-24T20:25:39Z

#8312 was merged, could you ensure your changes still work as expected with the new tests?

…us-for-streams

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

ascheglov · 2019-09-25T11:20:17Z

#8312 was merged, could you ensure your changes still work as expected with the new tests?

I've checked it and there is no need to change anything.

For streaming responses, the "content-type" header will be set twice, but that's alright because setReference is cheap.

ascheglov · 2019-09-26T21:06:13Z

Apparently the asan build is stuck. How do I restart it?

nareddyt · 2019-09-26T23:59:56Z

Apparently the asan build is stuck. How do I restart it?

Push an empty commit: https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#triggering-ci-re-run-without-making-changes

lizan · 2019-09-27T00:53:48Z

Can you merge master? You commit doesn't contain asan job config in azp.

…us-for-streams

ascheglov · 2019-09-27T12:46:24Z

/retest

repokitteh-read-only · 2019-09-27T12:46:27Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #8323 (comment) was created by @ascheglov.

see: more, trace.

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

…us-for-streams

…#8323) When a streaming backend returns an error in a trailer-only response, with the 'convert_grpc_status' option enabled, instead of writing the [] (empty array) in reply-body transcode the error in the headers and reply with {"code":...}. Risk Level: Low Testing: added an integraion test, tested manually. Documentation: n/a Release notes: n/a Fixes envoyproxy#8315 Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>

separate the cases when gRPC status is converted to JSON and when it'…

c4e17fe

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

qiwzhang reviewed Sep 23, 2019

View reviewed changes

alyssawilk assigned lizan and qiwzhang Sep 23, 2019

qiwzhang previously approved these changes Sep 23, 2019

View reviewed changes

move all of the status converting code in a separate function

dc806e9

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

ascheglov dismissed qiwzhang’s stale review via dc806e9 September 24, 2019 12:21

qiwzhang reviewed Sep 24, 2019

View reviewed changes

qiwzhang previously approved these changes Sep 24, 2019

View reviewed changes

lizan reviewed Sep 24, 2019

View reviewed changes

Anatoly Scheglov added 2 commits September 25, 2019 12:42

Merge remote-tracking branch 'upstream/master' into feature/grpc-stat…

b95aa4a

…us-for-streams

use CONSTRUCT_ON_FIRST_USE

b60bbcd

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

ascheglov dismissed qiwzhang’s stale review via b60bbcd September 25, 2019 11:13

Merge remote-tracking branch 'upstream/master' into feature/grpc-stat…

033f8bd

…us-for-streams

Anatoly Scheglov added 2 commits September 27, 2019 15:48

Kick CI

274d148

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

Merge remote-tracking branch 'upstream/master' into feature/grpc-stat…

68835b9

…us-for-streams

ascheglov requested review from lizan and qiwzhang October 2, 2019 09:38

lizan approved these changes Oct 3, 2019

View reviewed changes

lizan merged commit 492e5f7 into envoyproxy:master Oct 3, 2019


		response_headers_->Status()->value(Grpc::Utility::grpcToHttpStatus(grpc_status));

		if (is_trailers_only_response) {

Conversation

ascheglov commented Sep 22, 2019

Uh oh!

ascheglov commented Sep 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nareddyt commented Sep 24, 2019

Uh oh!

ascheglov commented Sep 25, 2019

Uh oh!

ascheglov commented Sep 26, 2019

Uh oh!

nareddyt commented Sep 26, 2019

Uh oh!

lizan commented Sep 27, 2019

Uh oh!

ascheglov commented Sep 27, 2019

Uh oh!

repokitteh-read-only bot commented Sep 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants