Supports return json type of response by zyfjeff · Pull Request #6261 · envoyproxy/envoy

zyfjeff · 2019-03-12T06:27:47Z

Signed-off-by: tianqian.zyf tianqian.zyf@alibaba-inc.com

Description: Refactor sendLocalReplay and Supports return json type of response
Risk Level: low
Testing: add new unit tests
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue] #6166

zyfjeff · 2019-03-12T08:35:02Z

/retest

repokitteh-read-only · 2019-03-12T08:35:11Z

🔨 rebuilding ci/circleci: release (failed build)
🔨 rebuilding ci/circleci: asan (failed build)
🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #6261 (comment) was created by @zyfjeff.

see: more, trace.

dnoe · 2019-03-12T15:41:05Z

Let us know whenever you think this is ready for maintainer review by removing the WIP in the title and I will assign reviewers. Thanks!

/wait

zyfjeff · 2019-03-13T12:39:20Z

@dnoe I finished, can review.

dnoe · 2019-03-13T13:04:14Z

@dio can you take a first pass on this?

zyfjeff · 2019-03-15T01:36:20Z

@dio When you have time, help review the code.

dio

Thanks for working on this! Sorry for the delay.

Looks good. A couple of nits.

dio · 2019-03-16T17:08:16Z

api/envoy/data/core/v2alpha/local_reply_body.proto

Superfluous spacing? And do you think core data here is the correct title for it?

dio · 2019-03-16T17:09:34Z

api/envoy/data/core/v2alpha/local_reply_body.proto

Should this be 1 instead of 3?

dio · 2019-03-16T17:10:09Z

api/envoy/data/core/v2alpha/local_reply_body.proto

Can we expand this comment to tell why we need this?

#4705

This issue mentions the need to remove the dependency of RapidJSON. The json conversion uses the method of protobuf to convert, so I defined a protobuf for converting the local reply body. I am not sure there are other ways to do the same thing.

dio · 2019-03-16T17:13:13Z

source/common/http/utility.h

has_json_content_type?

dio · 2019-03-16T17:14:52Z

source/common/http/utility.h

s/An LocalReplyInfo populated from the/A populated LocalReplyInfo object from a/

zyfjeff · 2019-03-18T03:54:26Z

/retest

repokitteh-read-only · 2019-03-18T03:54:30Z

🔨 rebuilding ci/circleci: coverage (failed build)
🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #6261 (comment) was created by @zyfjeff.

see: more, trace.

zyfjeff · 2019-03-18T07:18:41Z

/retest

repokitteh-read-only · 2019-03-18T07:18:45Z

🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #6261 (comment) was created by @zyfjeff.

see: more, trace.

zyfjeff · 2019-03-18T07:43:05Z

@dio PTAL

dio

Looks good! Thanks!

mattklein123

Thanks for this, cool feature. I'm flushing out some initial comments, so let's iterate on these and then once resolved I will take another pass. Thank you!

/wait

mattklein123 · 2019-03-23T16:44:37Z

api/envoy/data/core/v2alpha/local_reply_body.proto

I think maybe call this LocalReply since eventually we might want to include other things in here?

mattklein123 · 2019-03-23T16:44:50Z

api/envoy/data/core/v2alpha/local_reply_body.proto

"Local reply"

mattklein123 · 2019-03-23T16:45:19Z

api/envoy/data/core/v2alpha/local_reply_body.proto

"Used to wrap a local response, and then convert to JSON."

source/common/http/utility.h

mattklein123 · 2019-03-23T16:48:36Z

source/common/http/utility.cc

What's the circular dependency issue here? There must be some way to share this logic without copying? Please try to have a common function that both places call?

Is a dependent circular reference, Http::Utility references grpc::Common(hasGrpcContentType), grpc::Common refers to Http::Utility(getResponseStatus), so a circular reference is generated, so I copied the hasGrpcContentType method directly here. Implementation.

I don't know which file should be placed on the hasGrpcContentType method so that it can be used together by these two places.

We should not copy the code, so please create a new file where we can share the logic.

mattklein123 · 2019-03-23T16:51:06Z

source/common/http/utility.cc

Instead of looking at the content type of the request, shouldn't we be looking at the Accept header here? Also, do we think this overall feature should be configurable? I could go either way, but it definitely needs a release note and needs to be documented.

Sorry, I got it wrong, it should be using Accept header here.

I don't think it needs to be configured here. this is a standard HTTP server that should be done, and we need to constantly improve it.

dio · 2019-07-26T08:59:08Z

@zyfjeff 👌🏽

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

repokitteh-read-only · 2019-07-30T02:08:44Z

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

🐱

Caused by: #6261 was synchronize by zyfjeff.

see: more, trace.

htuch · 2019-07-31T22:23:07Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

  // `case normalization <https://tools.ietf.org/html/rfc3986#section-6.2.2.1>`
  google.protobuf.BoolValue normalize_path = 30;
+
+  // Used to configure the Local reply the type of the body


This is not super clear. Can you write a more detailed description of what this field is doing?

htuch · 2019-07-31T22:23:43Z

api/envoy/data/core/v2alpha/BUILD

    ],
 )
+
+api_proto_library(


I'm not sure if I would consider this a core type. Is this a generic JSON description or very specific to the idea of returning a JSON reply from HCM?

htuch · 2019-07-31T22:24:00Z

api/envoy/data/core/v2alpha/local_reply_configuration.proto

Yeah, Struct would be the canonical representation of JSON in proto.

htuch · 2019-07-31T22:24:07Z

api/envoy/data/core/v2alpha/local_reply_configuration.proto

+
+// [#protodoc-title: Local reply configuration]
+
+// Configure the local reply of the body type


What is the body type?

stale · 2019-08-07T22:27:06Z

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!

stale · 2019-08-15T09:10:15Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

zyfjeff requested a review from snowp as a code owner March 12, 2019 06:27

zyfjeff force-pushed the refactor_send_local_reply branch 3 times, most recently from 5b13657 to d441f50 Compare March 12, 2019 08:31

zyfjeff force-pushed the refactor_send_local_reply branch from d441f50 to 30c5ae5 Compare March 12, 2019 08:37

repokitteh-read-only bot added waiting and removed waiting labels Mar 12, 2019

zyfjeff force-pushed the refactor_send_local_reply branch 2 times, most recently from c9f2d89 to d6b6593 Compare March 13, 2019 11:28

zyfjeff changed the title ~~WIP: Supports return json type of response~~ Supports return json type of response Mar 13, 2019

dnoe assigned dio Mar 13, 2019

dio reviewed Mar 16, 2019

View reviewed changes

zyfjeff force-pushed the refactor_send_local_reply branch from 8998614 to 12f8e62 Compare March 18, 2019 03:07

dio previously approved these changes Mar 22, 2019

View reviewed changes

mattklein123 self-assigned this Mar 22, 2019

mattklein123 requested changes Mar 23, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 23, 2019

zyfjeff dismissed dio’s stale review via 3b439e8 March 25, 2019 04:09

repokitteh-read-only bot removed the waiting label Mar 25, 2019

dio reopened this Jul 26, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 26, 2019

zyfjeff added 15 commits July 30, 2019 10:01

Refactor sendLocalReply

462b70c

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Support return response of json type

7e258ac

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Add unittests and integration test

f533efd

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Fix code review comments

dd9636a

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Fix code review comments

73b2997

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Addeess code review comments

6b19f4a

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Address code review comments

4a67ec8

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Fix merge conflict

e4d6585

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Fix code format

80f4e09

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Fix merge conflict

162c95f

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Refactor local reply

f305fa7

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Fix doc failed

67b56c9

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Fix compile error

20542e1

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Fix doc error

395a0f8

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Fix code review comments

c5c1598

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

zyfjeff force-pushed the refactor_send_local_reply branch from e414ac9 to c5c1598 Compare July 30, 2019 02:08

zyfjeff requested a review from zuercher as a code owner July 30, 2019 02:08

repokitteh-read-only bot removed the waiting label Jul 30, 2019

mattklein123 added the waiting label Jul 30, 2019

mattklein123 mentioned this pull request Jul 30, 2019

Customize response code on upstream reset #7537

Closed

htuch reviewed Jul 31, 2019

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 7, 2019

stale bot closed this Aug 15, 2019

lizan mentioned this pull request Sep 5, 2019

WiP: Local reply mapper #8126

Closed


		// [#protodoc-title: Local reply configuration]

		// Configure the local reply of the body type

Conversation

zyfjeff commented Mar 12, 2019

Uh oh!

zyfjeff commented Mar 12, 2019

Uh oh!

repokitteh-read-only bot commented Mar 12, 2019

Uh oh!

dnoe commented Mar 12, 2019

Uh oh!

zyfjeff commented Mar 13, 2019

Uh oh!

dnoe commented Mar 13, 2019

Uh oh!

zyfjeff commented Mar 15, 2019

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

dio Mar 16, 2019 • 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!

zyfjeff commented Mar 18, 2019

Uh oh!

repokitteh-read-only bot commented Mar 18, 2019

Uh oh!

zyfjeff commented Mar 18, 2019

Uh oh!

repokitteh-read-only bot commented Mar 18, 2019

Uh oh!

zyfjeff commented Mar 18, 2019

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

zyfjeff Mar 25, 2019 • 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!

dio commented Jul 26, 2019

Uh oh!

repokitteh-read-only bot commented Jul 30, 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!

dio Mar 16, 2019 •

edited

Loading

zyfjeff Mar 25, 2019 •

edited

Loading