grpc json transcoder: Support `response_body` by euroelessar · Pull Request #10892 · envoyproxy/envoy

euroelessar · 2020-04-22T02:53:08Z

Description: Add support for google.api.Http.response_body field.
Risk Level: medium (new parsing of potentially untrusted user data, but requires config change to be active)
Testing: added unit test
Docs Changes: n/a (body/response_body are part of google api documentation and are not mentioned in envoy docs)
Release Notes: added
Fixes #10870

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2020-04-22T02:54:52Z

+    } else if (tag == 0) {
+      return true;
+    } else {
+      if (!Protobuf::internal::WireFormatLite::SkipField(input, tag)) {


Dependency on internal API worries me, but I didn't find a better way without actually copying the same logic.

Sorry for the late response. Can we add more test coverage for this function? This part isn't covered.

https://344313-65214191-gh.circle-artifacts.com/0/coverage/coverage/proc/self/cwd/source/extensions/filters/http/grpc_json_transcoder/http_body_utils.cc.html#L20

euroelessar · 2020-04-22T02:55:56Z

+        // TODO(elessar): Return error to client.
+        encoder_callbacks_->resetStream();


Is there a way to actually return 500 http status code to client right here if first message is invalid and stop fetching bad body from the upstream?

you can call encoder_callbackds_->sendLocalReply() to return 500.

sendLocalReply is only available as part of decoder_callbacks_, at encoding phase it's too late to call it.

Yes it is usually too late at encodeData, unless you returned StopIteration in encodeHeaders, then you can modify headers and Continue. (which is the case here)

Is there a way to indicate envoy that there will be no more data (end_stream=true)?

Yes, end_stream = true means not more data.

seems no, we have StopIterationAndEndStream in FilterHeaderStatus but not in data. cc @mattklein123?

Yeah I think this is a new use case we haven't hit before. You might be able to hack something up with injectEncodedDataToFilterChain (0 bytes + end_stream), but otherwise I think this would require HCM changes. cc @alyssawilk @snowp to see if I am forgetting about something.

ooh error handling on the response path!

AFIK the only example we have of this is in the HCM responseDataTooLarge.
In theory once we've stared running responses through the filter chain, we can't run them again (i.e. if headers are blocked by some downstream filter and haven't been written to the client, we can send a 500 but e can't send headers through the filter chain a second time)

I think the same way that we have a sendLocalReply for decoder filter callbacks we could have a sendReplyOrResetStream() for encoder filter callbacks which factors out the HCM responseDataTooLarge logic and either ships headers + body directly to the codec, or resets the stream.

I think the same way that we have a sendLocalReply for decoder filter callbacks we could have a sendReplyOrResetStream() for encoder filter callbacks which factors out the HCM responseDataTooLarge logic and either ships headers + body directly to the codec, or resets the stream.

Yeah +1. I would probably just submit this PR the way you have it now with a TODO and then circle back with an HCM change if you so desire.

qiwzhang · 2020-04-22T03:23:05Z

+      http_body.Clear();
      Buffer::ZeroCopyInputStreamImpl stream(std::move(frame.data_));
-      http_body.ParseFromZeroCopyStream(&stream);
+      if (!HttpBodyUtils::parseMessageByFieldPath(&stream, method_->response_body_field_path,


Doing there is only supporting HttpBody response. What about normal JSON response?

You may have to implement it here

https://github.com/grpc-ecosystem/grpc-httpjson-transcoding/blob/master/src/response_to_json_translator.cc#L57

I can submit a version of this change to grpc-httpjson-transcoding library instead & use it both here for HttpBody and there for JSON.
Protobuf spec allows inclusion of the same message field multiple times with an expectation that parser merges them. In order to support this generically I may need to provide ZeroCopyInputStream implementation built on top of the chain of sub-streams. It can be somehow less efficient though hopefully still the same amount of copies.

Thoughts?

Alternatively we could decide to not support this edge case and treat it as an error until somebody complains.

We can make supporting response_body in HttpBody and JSON as two different features. This PR can be just for HttpBody, and file another issue to track JSON feature.

Please log a proper warning or error code for not supported case.

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

stale · 2020-05-06T18:34:57Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

stale · 2020-05-22T06:21:11Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

euroelessar · 2020-05-28T08:48:27Z

Still working on it, will update the CL by end of the week

stale · 2020-06-05T05:06:45Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

stale · 2020-06-26T09:50:31Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

lizan · 2020-07-06T23:09:50Z

@euroelessar the coverage fail is real due to lack of coverage.

/wait

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2020-07-09T09:55:05Z

/retest

repokitteh-read-only · 2020-07-09T09:55:08Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #10892 (comment) was created by @euroelessar.

see: more, trace.

euroelessar · 2020-07-09T19:49:02Z

/retest

repokitteh-read-only · 2020-07-09T19:49:05Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #10892 (comment) was created by @euroelessar.

see: more, trace.

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2020-07-10T01:20:27Z

@lizan Thanks for the patience, I've fixed the coverage for the pieces introduced by this pull request.

euroelessar · 2020-07-10T01:21:45Z

ASAN failure looks weird though, all tests are passed but:

2020-07-10T00:52:26.7829614Z INFO: Build completed successfully, 2422 total actions
2020-07-10T00:52:26.8069132Z $TEST_TMPDIR defined: output root default is '/build/tmp' and max_idle_secs default is '15'.
2020-07-10T00:52:26.8652399Z ls: cannot access '/tmp/tap//tap_*.pb_text': No such file or directory
2020-07-10T00:52:27.5476798Z 
2020-07-10T00:52:27.5557545Z ##[error]Bash exited with code '2'.
2020-07-10T00:52:27.5573440Z ##[section]Finishing: Run CI script

dio · 2020-07-10T03:08:01Z

@euroelessar a fix from @lizan is on its way: #12018.

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2020-07-10T07:03:13Z

Failed coverage test is test/integration/load_stats_integration_test.cc, which looks like unrelated flake

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

grpc json transcoder: Support response_body

ba19dd0

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar requested a review from lizan as a code owner April 22, 2020 02:53

euroelessar commented Apr 22, 2020

View reviewed changes

qiwzhang reviewed Apr 22, 2020

View reviewed changes

mattklein123 assigned lizan Apr 22, 2020

Merge branch 'master' of github.com:envoyproxy/envoy into response-body

a4a3e28

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

stale Bot added the stale stalebot believes this issue/PR has not been touched recently label May 6, 2020

Merge branch 'master' of github.com:envoyproxy/envoy into response-body

a0bcaea

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar mentioned this pull request May 11, 2020

grpc json transcoder: Support response_body for JSON #11136

Closed

fail on response_body without HttpBody

9d573d1

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

stale Bot removed the stale stalebot believes this issue/PR has not been touched recently label May 11, 2020

qiwzhang previously approved these changes May 11, 2020

View reviewed changes

stale Bot added the stale stalebot believes this issue/PR has not been touched recently label May 22, 2020

stale Bot removed the stale stalebot believes this issue/PR has not been touched recently label May 28, 2020

stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 5, 2020

Ruslan Nigmatullin added 2 commits June 11, 2020 18:25

Merge branch 'master' of github.com:envoyproxy/envoy into response-body

bcfa0ae

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

add test for unknowns fields

39591c2

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar dismissed qiwzhang’s stale review via 39591c2 June 12, 2020 02:49

stale Bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 12, 2020

Ruslan Nigmatullin added 2 commits June 11, 2020 20:23

bypass linter

806c9b4

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Merge branch 'master' of github.com:envoyproxy/envoy into response-body

7a7411e

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 26, 2020

Merge branch 'master' of github.com:envoyproxy/envoy into response-body

b0d66d0

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

stale Bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 2, 2020

repokitteh-read-only Bot added the waiting label Jul 6, 2020

Ruslan Nigmatullin added 3 commits July 9, 2020 01:32

moar coverage

eb9837c

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Merge branch 'master' of github.com:envoyproxy/envoy into response-body

ebe3aa3

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

release notes

638e22a

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

repokitteh-read-only Bot removed the waiting label Jul 9, 2020

fix release notes

75177f7

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Ruslan Nigmatullin added 2 commits July 9, 2020 16:30

Merge branch 'master' of github.com:envoyproxy/envoy into response-body

e44bcb1

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

reorder

d5d6854

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Merge branch 'master' of github.com:envoyproxy/envoy into response-body

89d3182

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

lizan approved these changes Jul 10, 2020

View reviewed changes

mattklein123 merged commit d7140cb into envoyproxy:master Jul 14, 2020

scheler pushed a commit to scheler/envoy that referenced this pull request Aug 4, 2020

grpc json transcoder: Support response_body (envoyproxy#10892)

1d81438

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

		// TODO(elessar): Return error to client.
		encoder_callbacks_->resetStream();

Conversation

euroelessar commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale Bot commented May 6, 2020

Uh oh!

stale Bot commented May 22, 2020

Uh oh!

euroelessar commented May 28, 2020

Uh oh!

stale Bot commented Jun 5, 2020

Uh oh!

stale Bot commented Jun 26, 2020

Uh oh!

lizan commented Jul 6, 2020

Uh oh!

euroelessar commented Jul 9, 2020

Uh oh!

repokitteh-read-only Bot commented Jul 9, 2020

Uh oh!

euroelessar commented Jul 9, 2020

Uh oh!

repokitteh-read-only Bot commented Jul 9, 2020

Uh oh!

euroelessar commented Jul 10, 2020

Uh oh!

euroelessar commented Jul 10, 2020

Uh oh!

dio commented Jul 10, 2020

Uh oh!

euroelessar commented Jul 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

euroelessar commented Apr 22, 2020 •

edited

Loading

lizan Apr 27, 2020 •

edited

Loading