Supports proto subtypes for gRPC-Web. by fengli79 · Pull Request #1017 · envoyproxy/envoy

fengli79 · 2017-05-25T17:52:10Z

Supports proto subtypes for gRPC-Web.
Includes: application/grpc-web+proto and application/grpc-web-text+proto.
TODO(fengli): Adds json and thrift once the spec is finalized.

htuch

Can you elaborate a bit somewhere on what the subtypes mean? I didn't get a clear idea from the gRPC-web doc what's going on. For example, naively I would believe that gRPC messages are always protos, why do we have a version without proto and a +proto content type?

fengli79 · 2017-05-26T01:54:42Z

@htuch It's elaborated here: https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md#protocol-differences-vs-grpc-over-http2

gRPC-Web spec allows using other data format over the wire, even today only protobuf is implemented.
Subtypes of the gRPC-Web content type is denoted by corresponding suffixes, such as +proto, +json and +thrift. Protobuf will be the default data format when no suffix presents.

The sender should specify a concrete subtype, and receiver should tolerant and default to protobuf when a subtype does not present.

htuch · 2017-05-26T02:23:49Z

I'm still confused by that description. If I send grpc-web+thrift, what happens upstream? Does Envoy convert thrift to proto? Is that possible in general for all the serialization formats? My naive understanding is that upstream (regular gRPC) is only proto.

Apologies if these questions seem silly, but I think this is a question that folks will have in general when trying to understand the relationship between grpc-web and gRPC, I'm just front loading the confusion :) This goes back to my point about the existing spec being a terse diff - there's no rationale in the spec which elaborates on why design decisions are made and what the use case is for different features.

fengli79 · 2017-05-26T02:44:07Z

@wenbozhu @htuch Yes, gRPC-Web will convert thrift to proto. However, if the gRPC backend can consume thrift, no conversion needed. See https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#requests
gRPC also supports custom data format, although today everyone only uses protobuf.
Ideally the downstream and upstream should use the same sub data format, but if they are different, it makes sense to get the intermedia layer to do necessary conversion.

htuch

Thanks for the rationale. Can you please add the explanation you provided in the GH reply to my request for clarification somewhere in the code base or docs? Thanks.

htuch · 2017-05-26T03:01:36Z

GrpcWebProto

htuch · 2017-05-26T03:06:18Z

You could rewrite this as a for loop over an array literal for ContentTypeValues to reduce the boiler plate (it could be like 7 LoC). Also, should we have a negative test for when a protocol isn't supported?

htuch · 2017-05-26T03:06:33Z

Should this be a TEST_P and consider both protocols names?

application/grpc-web+proto and application/grpc-web-text+proto.

htuch

LGTM, just some minor comments on tests.

htuch · 2017-05-31T16:10:29Z

-  GrpcWebFilter filter_;
-  NiceMock<Http::MockStreamEncoderFilterCallbacks> encoder_callbacks_;
-};
+  const std::string& request_content_type() { return std::get<0>(GetParam()); }


Nit: const std::string& request_content_type() const { ..

htuch · 2017-05-31T16:10:41Z

-  EXPECT_EQ(Http::Headers::get().TEValues.Trailers, request_headers.TE()->value().c_str());
-  EXPECT_EQ(Http::Headers::get().GrpcAcceptEncodingValues.Default,
-            request_headers.GrpcAcceptEncoding()->value().c_str());
+  const std::string& request_accept() { return std::get<1>(GetParam()); }


htuch · 2017-05-31T16:11:12Z

-  EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(request_trailers));
-  EXPECT_STREQ("0", request_trailers.GrpcStatus()->value().c_str());
-  EXPECT_STREQ("ok", request_trailers.GrpcMessage()->value().c_str());
+  bool is_binary_request() {


Nit: bool isBinaryRequest() const

htuch · 2017-05-31T16:11:25Z

-  EXPECT_EQ("200", response_headers.get_(Http::Headers::get().Status.get()));
-  EXPECT_EQ(Http::Headers::get().ContentTypeValues.GrpcWeb,
-            response_headers.ContentType()->value().c_str());
+  bool accept_text_response() {


accept_text_response/accept_binary_response aren't updated yet..

htuch · 2017-05-31T16:14:17Z

  // Tests response headers.
  Http::TestHeaderMapImpl response_headers;
  response_headers.addViaCopy(Http::Headers::get().Status, "200");
-  response_headers.addViaCopy(Http::Headers::get().ContentType,


Where is this now done?

Still wondering about this one..

It's not required at this moment, as the downstream content-type is gRPC-Web instead of gRPC. I plan to create some pull requests to add more error handling, may include additional check on the backend response content-type.

htuch · 2017-05-31T16:14:43Z

-  EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(response_buffer, false));
-  EXPECT_EQ(std::string(B64_MESSAGE, B64_MESSAGE_SIZE),
-            TestUtility::bufferToString(response_buffer));
-  response_buffer.drain(response_buffer.length());


Where is this now done?

It's not needed, as the trailers uses a separate buffer in the test.

htuch · 2017-05-31T16:55:35Z

+  NiceMock<Http::MockStreamEncoderFilterCallbacks> encoder_callbacks_;
+};
+
+TEST_F(GrpcWebFilterTest, SupportedContentTypes) {


How come this is under GrpcWebFilterTest? It will run a combinatorial number of times but isn't itself parameterized.

It's a TEST_F, will be run only once.
Here's the test log. FYI.

[==========] Running 17 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 1 test from GrpcWebFilterTest
[ RUN ] GrpcWebFilterTest.SupportedContentTypes
[ OK ] GrpcWebFilterTest.SupportedContentTypes (1 ms)
[----------] 1 test from GrpcWebFilterTest (1 ms total)

[----------] 16 tests from Unary/GrpcWebFilterTest
[ RUN ] Unary/GrpcWebFilterTest.Unary/0
[ OK ] Unary/GrpcWebFilterTest.Unary/0 (0 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/1
[ OK ] Unary/GrpcWebFilterTest.Unary/1 (1 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/2
[ OK ] Unary/GrpcWebFilterTest.Unary/2 (0 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/3
[ OK ] Unary/GrpcWebFilterTest.Unary/3 (0 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/4
[ OK ] Unary/GrpcWebFilterTest.Unary/4 (1 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/5
[ OK ] Unary/GrpcWebFilterTest.Unary/5 (0 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/6
[ OK ] Unary/GrpcWebFilterTest.Unary/6 (1 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/7
[ OK ] Unary/GrpcWebFilterTest.Unary/7 (0 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/8
[ OK ] Unary/GrpcWebFilterTest.Unary/8 (0 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/9
[ OK ] Unary/GrpcWebFilterTest.Unary/9 (1 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/10
[ OK ] Unary/GrpcWebFilterTest.Unary/10 (0 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/11
[ OK ] Unary/GrpcWebFilterTest.Unary/11 (0 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/12
[ OK ] Unary/GrpcWebFilterTest.Unary/12 (1 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/13
[ OK ] Unary/GrpcWebFilterTest.Unary/13 (0 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/14
[ OK ] Unary/GrpcWebFilterTest.Unary/14 (1 ms)
[ RUN ] Unary/GrpcWebFilterTest.Unary/15
[ OK ] Unary/GrpcWebFilterTest.Unary/15 (0 ms)
[----------] 16 tests from Unary/GrpcWebFilterTest (6 ms total)

[----------] Global test environment tear-down
[==========] 17 tests from 2 test cases ran. (7 ms total)
[ PASSED ] 17 tests.

htuch · 2017-05-31T16:56:12Z

  // Tests response headers.
  Http::TestHeaderMapImpl response_headers;
  response_headers.addViaCopy(Http::Headers::get().Status, "200");
-  response_headers.addViaCopy(Http::Headers::get().ContentType,


Still wondering about this one..

htuch · 2017-05-31T16:57:02Z

-  EXPECT_EQ("200", response_headers.get_(Http::Headers::get().Status.get()));
-  EXPECT_EQ(Http::Headers::get().ContentTypeValues.GrpcWeb,
-            response_headers.ContentType()->value().c_str());
+  bool accept_text_response() {


accept_text_response/accept_binary_response aren't updated yet..

htuch

@ccaraman @RomanDzhabarov @junr03 Can one of you take a look at this?

Automatic merge from submit-queue. Remove GetDestinationIpPort from HTTP filter. **What this PR does / why we need it**: **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: **Release note**: ```release-note ```

Description: given we have gotten no additional information from the code deleted in this PR, I am deleting to make readability better. This code was previously added to help investigate #1016 and #1017. Risk Level: low Testing: no-op in terms of functionality. Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** There was a bug in the translation of OpenAI->OpenAI layer where the body mutation is broken when forceBodyMutation = true. forceBodyMutation becomes true when stream_options is not present in the original body and we lacked the test case for it. **Related Issues/PRs (if applicable)** Closes #1017 --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

htuch reviewed May 25, 2017

View reviewed changes

htuch reviewed May 26, 2017

View reviewed changes

fengli79 added 2 commits May 30, 2017 13:37

Supports proto subtypes for gRPC-Web. Includes:

0c68999

application/grpc-web+proto and application/grpc-web-text+proto.

Address comments.

b037f67

htuch reviewed May 31, 2017

View reviewed changes

fengli79 added 2 commits May 31, 2017 09:43

Address comments.

ff4acdb

Address comments.

f6f0214

htuch reviewed May 31, 2017

View reviewed changes

Address comments.

ddc1ef8

htuch approved these changes May 31, 2017

View reviewed changes

ccaraman self-requested a review May 31, 2017 22:15

ccaraman approved these changes May 31, 2017

View reviewed changes

htuch merged commit 4c47ca0 into envoyproxy:master Jun 1, 2017

Conversation

fengli79 commented May 25, 2017

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

fengli79 commented May 26, 2017

Uh oh!

htuch commented May 26, 2017

Uh oh!

fengli79 commented May 26, 2017

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants