Adds grpc-web support for envoy. by fengli79 · Pull Request #943 · envoyproxy/envoy

fengli79 · 2017-05-10T21:27:30Z

Add the grpc-web support for envoy.

The HotRestart holds a socket event for the domain socket used for admin purpose, it need to be deleted before the envoy server gets deleted, as the envoy server own the event_base and which already deleted the registered events with the event_base_free().

htuch · 2017-05-11T01:18:17Z

+
+namespace Grpc {
+
+const std::string GrpcWebFilter::GRPC_WEB_CONTENT_TYPE{"application/grpc-web"};


Global statics are disallowed (and you know this ;) )

htuch

As discussed offline, we should add curl to the build recipes as shown in https://github.com/lyft/envoy/blob/master/bazel/EXTERNAL_DEPS.md when it comes time to do the integration tests.

mattklein123 · 2017-05-11T05:26:03Z

  }, this);
 #endif

+  if (buffered_response_data_ && buffered_response_data_->length() > 0) {


Per offline convo with @fengli79 I am working on a proper solution to this part which unfortunately is pretty tricky. Hope to land this by the end of the week. It shouldn't change the rest of the code that much so we can continue to review/test in the interim.

mattklein123 · 2017-05-14T20:36:48Z

Please rebase on #959 once merged.

htuch

Looks generally quite good. But, I think this needs some more detailed comments in various places, see feedback below.

htuch · 2017-05-22T18:59:05Z

+namespace Envoy {
+namespace Grpc {
+
+const uint8_t GrpcWebFilter::GRPC_WEB_TRAILER = 0b10000000;


Nit: personally I find (1 << 7) much clearer than binary literals where I need to count zeroes. But, feel free to keep it this way if you'd like.

Yes, let's keep this.

Please add a comment that this is specifying a "trailer with no compression".

htuch · 2017-05-22T19:02:24Z

+Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, bool) {
+  const Http::HeaderEntry* content_type = headers.ContentType();
+  if (content_type != nullptr &&
+      Http::Headers::get().ContentTypeValues.GrpcWebText == content_type->value().c_str()) {


Based on https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md#protocol-differences-vs-grpc-over-http2, I would expect that grpc-web-text to be a prefix, not exact match. The default is "+proto", but what about the other cases?

I plan add follow up pull requests to support other subtypes soon.

Please add a comment here and the other site to this effect, TODO(fengli79): ...

htuch · 2017-05-22T19:06:39Z

+      Http::Headers::get().ContentTypeValues.GrpcWebText == content_type->value().c_str()) {
+    is_text_request_ = true;
+  }
+  headers.insertContentType().value(Http::Headers::get().ContentTypeValues.Grpc);


From the docs, insertContentType() only inserts a header if it doesn't exist. I'm wondering how this works then, since you have test coverage for this.

If there's no content-type header from the request, the headers.insertContentType() will create a HeaderMapEntry and return it, thus I can change its value.
If there's already a content-type header, the inserContentType() will return the existing HeaderMapEntry.
Note that content-type is an inline header, it doesn't support appending.

That's correct. When content-type exist, it will return reference to existing entry, and following .value sets the value, so it works.

htuch · 2017-05-22T19:10:23Z

+
+  const Http::HeaderEntry* accept = headers.get(Http::Headers::get().Accept);
+  if (accept != nullptr &&
+      Http::Headers::get().ContentTypeValues.GrpcWebText == accept->value().c_str()) {


Same as above comment on grpc-web-text prefix.

htuch · 2017-05-22T19:15:02Z

+  // Parse application/grpc-web-text format.
+  if (data.length() + decoding_buffer_.length() < 4) {
+    decoding_buffer_.move(data);
+    return Http::FilterDataStatus::Continue;


Do you want to use StopIterationAndBuffer here (and have the filter mechanism perform buffering for you until you have 4 bytes), or is there some reason that the filters down the chain should operate on an empty buffer? I think that would simplify the below code as well (you might not even need an explicit decoding_buffer_).

I cannot use StopIterationAndBuffer, as I have to consume the buffer given by the filter completely and put back b64 decoded data in the same buffer. If I let the filter to buffer the data, I will lose the information about the boundary for the b64 decoded data and raw data in the filter buffer.

Right, got it. Do you want to do StopIterationNoBufferin the < 4 branch here, since there's no data for later filters at this point?

That makes sense. Done.

htuch · 2017-05-22T19:30:47Z

+    return Http::FilterDataStatus::Continue;
+  }
+
+  // Encodes the response as base64.


Can you elaborate in some more depth on the transformation that is happening here? My current guess is that we're taking standard gRPC wire format, decoding and deframing, then re-encoding it using the format of gRPC Web? What is the meaning of flags in this context? It's probably not H2 flags, so decoder_ must have figured out what the delimited message flags are? I think lots of verbose comments would help.

More comments added.

htuch · 2017-05-22T19:40:21Z

+    temp->add("\r\n");
+  }, &temp);
+  Buffer::OwnedImpl buffer;
+  buffer.add(&GRPC_WEB_TRAILER, 1);


Please describe that this is a delimited message that is being assembled as per the gRPC spec.

It's not in gRPC spec. Instead, it's defined in gRPC-Web. Trailers are encoded in a frame denoted with 0b1000000 + 4 bytes length in network order. The frame data is CRLF separated headers.

htuch · 2017-05-22T19:42:28Z

+  // Tests request data.
+  Buffer::OwnedImpl request_buffer(MESSAGE, MESSAGE_SIZE);
+  EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_buffer, true));
+  EXPECT_EQ(0, memcmp(MESSAGE, TestUtility::bufferToString(request_buffer).c_str(), MESSAGE_SIZE));


Why not just do string comparison?

Because the buffer may contains \0.

You can do C++ string comparisons on buffers with embedded nulls.

Done. However, I have to wrap the MESSAGE with std::string(MESSAGE, MESSAGE_SIZE).

htuch · 2017-05-22T19:43:07Z

+  Buffer::OwnedImpl request_buffer(MESSAGE, MESSAGE_SIZE);
+  EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_buffer, true));
+  EXPECT_EQ(0, memcmp(MESSAGE, TestUtility::bufferToString(request_buffer).c_str(), MESSAGE_SIZE));
+


What about testing request trailers?

gRPC doesn't use request trailers. And gRPC-Web clients are in browser, also won't send request trailers.
If a request trailer presents, it will be forwarded to the backend without change, and let the backend decide whether to ignore it or reject it.

I think you should still verify this pass-thru behavior, as it's something we intend to have happen and don't want to see a regression for.

htuch

I think I now get grpc-web :) I'll reiterate my earlier comment that it would be helpful if the grpc-web spec was more than just a terse set of bullet points that provide a diff against the normal gRPC H2 framing, but instead provide a full spec. Thanks.

htuch · 2017-05-23T03:15:12Z

+Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, bool) {
+  const Http::HeaderEntry* content_type = headers.ContentType();
+  if (content_type != nullptr &&
+      Http::Headers::get().ContentTypeValues.GrpcWebText == content_type->value().c_str()) {


Please add a comment here and the other site to this effect, TODO(fengli79): ...

htuch · 2017-05-23T03:48:28Z

+  // Parse application/grpc-web-text format.
+  if (data.length() + decoding_buffer_.length() < 4) {
+    decoding_buffer_.move(data);
+    return Http::FilterDataStatus::Continue;


Right, got it. Do you want to do StopIterationNoBufferin the < 4 branch here, since there's no data for later filters at this point?

htuch · 2017-05-23T03:54:29Z

+  // Encodes the decoded frames with base64.
+  for (auto& frame : frames) {
+    Buffer::OwnedImpl temp;
+    temp.add(&frame.flags_, 1);


Can you please add an explanation of what these flags are? Please point to the relevant specification. As a reader, I find it very confusing to have to think about both H2 flags (which clearly these aren't, but the above decode doesn't make clear), normal gRPC message delimiting flags (compression vs. no compression) and the gRPC-web special flags (with the MSB bit). If I find it confusing, I think others will too. Thanks!

Those flags are gRPC frame flags. They are decoded as grpc frames from a grpc backend.
They are documented in the decoder.cc:
https://github.com/lyft/envoy/blob/master/source/common/grpc/codec.h#L49

It's gRPC frame, comes from backend. It's documented in the decoder.h.
https://github.com/lyft/envoy/blob/master/source/common/grpc/codec.h#L49

htuch · 2017-05-23T03:57:43Z

+namespace Envoy {
+namespace Grpc {
+
+const uint8_t GrpcWebFilter::GRPC_WEB_TRAILER = 0b10000000;


Please add a comment that this is specifying a "trailer with no compression".

htuch · 2017-05-23T04:00:51Z

+const char B64_MESSAGE[] = "AAAAABJncnBjLXdlYi10ZXh0LWRhdGE=";
+const size_t B64_MESSAGE_SIZE = 32;
+const char TRAILERS[] = "\x80\x00\x00\x00\x0fgrpc-status:0\r\n";
+const size_t TRAILERS_SIZE = 20;


Can we use sizeof here to make this less fragile, since these are const arrays with known size?

htuch · 2017-05-23T04:02:29Z

+  // Tests request data.
+  Buffer::OwnedImpl request_buffer(MESSAGE, MESSAGE_SIZE);
+  EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_buffer, true));
+  EXPECT_EQ(0, memcmp(MESSAGE, TestUtility::bufferToString(request_buffer).c_str(), MESSAGE_SIZE));


You can do C++ string comparisons on buffers with embedded nulls.

htuch · 2017-05-23T04:03:32Z

+  Buffer::OwnedImpl request_buffer(MESSAGE, MESSAGE_SIZE);
+  EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_buffer, true));
+  EXPECT_EQ(0, memcmp(MESSAGE, TestUtility::bufferToString(request_buffer).c_str(), MESSAGE_SIZE));
+


I think you should still verify this pass-thru behavior, as it's something we intend to have happen and don't want to see a regression for.

htuch · 2017-05-23T04:06:44Z

+                             Http::Headers::get().ContentTypeValues.GrpcWeb);
+  EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false));
+  EXPECT_EQ(Http::Headers::get().ContentTypeValues.Grpc,
+            request_headers.ContentType()->value().c_str());


Where do you check grpc-accept-encoding:identity,deflate,gzip., te:trailers, etc?

htuch · 2017-05-23T04:07:58Z

+const size_t TEXT_MESSAGE_SIZE = 23;
+const char B64_MESSAGE[] = "AAAAABJncnBjLXdlYi10ZXh0LWRhdGE=";
+const size_t B64_MESSAGE_SIZE = 32;
+const char TRAILERS[] = "\x80\x00\x00\x00\x0fgrpc-status:0\r\n";


Should have more than one trailer to verify multi trailer header handling.

ccaraman · 2017-05-23T19:33:54Z

+  const Http::HeaderEntry* content_type = headers.ContentType();
+  if (content_type != nullptr &&
+      Http::Headers::get().ContentTypeValues.GrpcWebText == content_type->value().c_str()) {
+    // Checks whether gRPC-Web client is sending b64 ncoded request.


typo ncoded -> encoded.

Please expand b64.

ccaraman · 2017-05-23T19:42:33Z

+  bool is_text_response_;
+  Buffer::OwnedImpl decoding_buffer_;
+  Decoder decoder_;
+};


nit: new line after this and before the closing bracket for namespace Grpc

ccaraman · 2017-05-23T19:43:06Z

+  buffer.move(temp);
+  encoder_callbacks_->addEncodedData(buffer);
+  return Http::FilterTrailersStatus::Continue;
+}


nit: new line after this line and before bracket closing the namespace Grpc

ccaraman · 2017-05-23T19:45:26Z

+namespace Server {
+namespace Configuration {
+
+HttpFilterFactoryCb GrpcWebFilterConfig::tryCreateFilterFactory(HttpFilterType type,


Please add a test for the filter config loading in https://github.com/lyft/envoy/blob/master/test/server/config/http/config_test.cc

ccaraman · 2017-05-23T19:48:15Z

+  // Implements StreamDecoderFilter.
+  Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap&, bool) override;
+  Http::FilterDataStatus decodeData(Buffer::Instance&, bool) override;
+


nit: remove new line

RomanDzhabarov · 2017-05-23T19:51:16Z

+namespace Envoy {
+namespace Grpc {
+
+// Bit mask denotes a trailers frame of gRPC-Web.


nit: uncompressed trailer

I'm going to add another bit mask 0b00000001 for compression. So it's not necessary to mention the compression here once it gets in.

ok, works for me as long as it's WIP in here.

RomanDzhabarov · 2017-05-23T20:05:51Z

+
+private:
+  static const uint8_t GRPC_WEB_TRAILER;
+  Http::StreamDecoderFilterCallbacks* decoder_callbacks_{};


nit: unused var

RomanDzhabarov · 2017-05-23T20:06:18Z

+  static const uint8_t GRPC_WEB_TRAILER;
+  Http::StreamDecoderFilterCallbacks* decoder_callbacks_{};
+  Http::StreamEncoderFilterCallbacks* encoder_callbacks_{};
+  bool is_text_request_;


for consistency you can init it right here, is_text_request_{} and same with is_text_respose_

RomanDzhabarov · 2017-05-23T20:07:30Z

+                                                                const Json::Object&,
+                                                                const std::string&,
+                                                                Server::Instance&) {
+  if (type != HttpFilterType::Both || name != "grpc_web") {


just a heads up: #994
HTTP filters registration will be changed in ^

htuch · 2017-05-24T04:52:48Z

@fengli79 One final ask before merging - can you switch to the new filter API in #994? Thanks!

htuch · 2017-05-24T17:16:41Z

+namespace Server {
+namespace Configuration {
+
+HttpFilterFactoryCb GrpcWebFilterConfig::tryCreateFilterFactory(HttpFilterType type,


This is still the old filter creation API, can you bump to the new one? Thanks!

htuch

@ccaraman Is this ready to merge?

mattklein123 · 2017-06-06T18:27:51Z

+  }
+
+  // Adds te:trailers to upstream HTTP2 request. It's required for gRPC.
+  headers.addStatic(Http::Headers::get().TE, Http::Headers::get().TEValues.Trailers);


Both of these addStatic() calls can use the O(1) insert varieties since you added the headers to the O(1) map. This would yield better performance. @fengli79 not urgent but I would do a follow up or add to a future grpc-web PR.

Will address in a follow up PR. Also, there are other stuffs need to be enhanced, like add some wrapper methods in Buffer::OwnedImpl to avoid the additional buffer allocation. Today I have to use a temp buffer when processing trailers, as the Buffer::OwnedImpl doesn't allow me to insert data to a particular position.

Automatic merge from submit-queue. [DO NOT MERGE] Auto PR to update dependencies of proxy This PR will be merged automatically once checks are successful. ```release-note none ```

Description: Adds initial support for Swift presentation of HTTP filter on-headers invocations. Risk Level: Moderate Testing: Local end-to-end Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

fengli79 added 10 commits April 12, 2017 14:28

Merge remote-tracking branch 'upstream/master'

312dd58

Change HotRestart::shutdownAdmin() to HotRestart::shutdown().

d0cec57

Local change for thirdparty.cmake, DO NOT commit to upstream!

ab3855f

Merge remote-tracking branch 'upstream/master'

489470a

Add grpc-web support for envoy.

8ecec66

Merge remote-tracking branch 'upstream/master'

8ef4b1c

Merge remote-tracking branch 'upstream/master'

2362192

Sync with upstream.

4c9a291

Fix the trailer encoding of grpc-web.

43297c3

htuch reviewed May 11, 2017

View reviewed changes

mattklein123 reviewed May 11, 2017

View reviewed changes

fengli79 added 3 commits May 11, 2017 16:02

Modify bazel BUILD files to add grpc-web module.

2f20d0d

Merge remote-tracking branch 'upstream/master'

b4f942e

Change to use the Http::Headers singleton for gRPC-Web relative headers.

7a205c9

fengli79 added 13 commits May 15, 2017 17:07

Merge remote-tracking branch 'upstream/master'

9d6b6d8

Merge remote-tracking branch 'upstream/master'

8c958b4

Merge remote-tracking branch 'upstream/master'

6acc12f

Revert source/common/http/conn_manager_impl.cc.

1394d32

Change to use addDecodedData to encode trailers to response body.

ffee1d4

Merge remote-tracking branch 'upstream/master'

7db18bb

Symbolize stack trace for tests.

2d195c3

Add unit tests for grpc-web filter.

539dcf5

Merge remote-tracking branch 'origin/symbolize_stacktrace'

7471364

Add unit tests for grpc-web filter.

6bf1e12

Merge remote-tracking branch 'upstream/master'

e2a5d4e

fix format.

25b9c37

Update unit tests.

f6bca59

htuch reviewed May 22, 2017

View reviewed changes

fengli79 added 2 commits May 22, 2017 14:34

Address comments.

7251c10

Fix format.

a1b6643

htuch reviewed May 23, 2017

View reviewed changes

Address comments.

15244b4

ccaraman reviewed May 23, 2017

View reviewed changes

Address comments.

88e5c63

RomanDzhabarov reviewed May 23, 2017

View reviewed changes

Address comments.

0f93298

htuch previously approved these changes May 24, 2017

View reviewed changes

fengli79 dismissed htuch’s stale review via 3a52ad7 May 24, 2017 17:01

htuch reviewed May 24, 2017

View reviewed changes

fengli79 added 3 commits May 24, 2017 10:38

Merge branch 'master' into master

275cb7e

Change to use NamedHttpFilterConfigFactory to register gRPC-Web filter.

6dc3008

Remove the unused capture of server variable.

ddb90fd

htuch approved these changes May 24, 2017

View reviewed changes

ccaraman approved these changes May 24, 2017

View reviewed changes

htuch merged commit 20bf9e8 into envoyproxy:master May 24, 2017

mattklein123 reviewed Jun 6, 2017

View reviewed changes

mattklein123 unassigned ccaraman Sep 14, 2017


		namespace Grpc {

		const std::string GrpcWebFilter::GRPC_WEB_CONTENT_TYPE{"application/grpc-web"};

Conversation

fengli79 commented May 10, 2017 • 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!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented May 14, 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!

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!

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

Uh oh!

fengli79 commented May 10, 2017 •

edited

Loading

htuch left a comment •

edited

Loading