extensions: add grpc_http1_reverse_bridge filter #5315

snowp · 2018-12-15T21:10:10Z

Adds a filter that allows converting a gRPC request into an HTTP/1.1
request with a custom content-type. Allows a vanilla HTTP/1.1 upstream
to handle incoming requests by reading and responding with protobuf
messages in binary octet format.

For now this shields the upstream from any gRPC association: the filter
removes the gRPC specific message prefix and manages the conversion of
the HTTP status code into grpc-status.

Signed-off-by: Snow Pettersen [email protected]

Risk Level: Low, new optional filter
Testing: UTs and integration tests
Docs Changes: n/a for now, will add comprehensive docs
Release Notes: n/a
Fixes #5300

Adds a filter that allows converting a gRPC request into an HTTP/1.1 request with a custom content-type. Allows a vanilla HTTP/1.1 upstream to handle incoming requests by reading and responding with protobuf messages in binary octet format. For now this shields the upstream from any gRPC association: the filter removes the gRPC specific message prefix and manages the conversion of the HTTP status code into grpc-status. Signed-off-by: Snow Pettersen <[email protected]>

snowp · 2018-12-15T21:11:44Z

Open for suggesting on naming. grpc_shim is what we refer to it internally, but I'm not super happy about it. I wanted to call it a bridge, but with the existing bridge filter it made it rather confusing to have both a grpc_http_bridge and http_grpc_bridge filter.

We've been running a variation of this filter internally for a few months now, and it's been working just fine.

Signed-off-by: Snow Pettersen <[email protected]>

ramaraochavali · 2018-12-16T02:36:51Z

Should we call this envoy.http1_grpc_bridge (or envoy.grpc_http1_reverse_bridge if it causes confusion - actually it would have been nice if the http1->gRPC was called envoy.http1_grpc_bridge which would have explained source->target conversion well) similar to how we call the http1->gRPC bridge envoy.grpc_http1_bridge?

Signed-off-by: Snow Pettersen <[email protected]>

lizan · 2018-12-16T20:08:01Z

source/extensions/filters/http/grpc_shim/grpc_shim.cc

+    // was unsuccessful. Since we're guaranteed at this point to have a valid response
+    // (unless upstream lied in content-type) we attempt to return a well-formed gRPC
+    // response body.
+    const auto length = htonl(buffer.length() + buffer_.length());


use Grpc::Encoder

lizan · 2018-12-16T20:14:54Z

source/extensions/filters/http/grpc_shim/grpc_shim.cc

+  // If this is a gRPC request and this is the start of DATA, remove the first 5
+  // bytes. These correspond to the gRPC data header.
+  if (enabled_ && !prefix_stripped_) {
+    buffer.drain(5);


This will crash with malicious downstream when buffer is < 5 bytes length.

Btw, aren't this messing up Content-Length in header?

Ah yeah, you're right. Should be easy enough to +/- 5 the content size and to fail the request if the request body size is < 5.

lizan · 2018-12-16T20:17:20Z

test/extensions/filters/http/grpc_shim/grpc_shim_test.cc

+    EXPECT_EQ(17, buffer.length());
+    EXPECT_THAT(trailers, HeaderValueOf(Http::Headers::get().GrpcStatus, "0"));
+
+    char null;


use Grpc::Decoder and test against Grpc::Frame?

That's so much better than my manual byte fiddling! I'll switch over that

lizan · 2018-12-16T20:18:39Z

source/extensions/extensions_build_config.bzl

@@ -31,6 +31,7 @@ EXTENSIONS = {
    "envoy.filters.http.grpc_http1_bridge":             "//source/extensions/filters/http/grpc_http1_bridge:config",
    "envoy.filters.http.grpc_json_transcoder":          "//source/extensions/filters/http/grpc_json_transcoder:config",
    "envoy.filters.http.grpc_web":                      "//source/extensions/filters/http/grpc_web:config",
+    "envoy.filters.http.grpc_shim":                      "//source/extensions/filters/http/grpc_shim:config",


Also an entry in CODEOWNERS.

Signed-off-by: Snow Pettersen <[email protected]>

lizan

As this is basically doing the opposite of grpc_http1_bridge, can we have an integration test against that so make sure they interoperates?

lizan · 2018-12-17T23:00:46Z

source/extensions/filters/http/grpc_shim/grpc_shim.cc

+    const auto length = htonl(buffer.length() + buffer_.length());
+
+    std::array<uint8_t, Grpc::GRPC_FRAME_HEADER_SIZE> frame;
+    Grpc::Encoder().newFrame(Grpc::GRPC_FH_DEFAULT, htonl(length), frame);


I don't think you need htonl here and above. newFrame takes care of that.

Oh boy, I had just length here and it caused the tests to fail, didn't realize that I was already htonling on L146. At this point I'm just round tripping back to host 🤦‍♂️

lizan · 2018-12-17T23:04:57Z

source/common/grpc/codec.h

@@ -13,6 +13,8 @@ const uint8_t GRPC_FH_DEFAULT = 0b0u;
 // Last bit for a compressed message.
 const uint8_t GRPC_FH_COMPRESSED = 0b1u;

+const uint64_t GRPC_FRAME_HEADER_SIZE = sizeof(uint8_t) + sizeof(uint32_t);


nit: constexpr

lizan · 2018-12-17T23:08:09Z

source/extensions/filters/http/grpc_shim/grpc_shim.cc

+
+#include "absl/strings/str_cat.h"
+
+namespace {


you can put this inside namespace GrpcShim so you don't need to prefix everything with Envoy::

lizan · 2018-12-17T23:10:11Z

source/extensions/filters/http/grpc_shim/grpc_shim.cc

+
+  // Buffer the response in a mutable buffer: we need to determine the size of the response
+  // and modify it later on.
+  buffer_.move(buffer);


Seems this is basically buffer everything in the filter, so there will be no flow control here, would it make more sense just use content-length and pass through data without buffering?

The problem I ran into with using content-type was that large responses wouldn't have the content-type set due to being chunked.

Wondering if it makes sense to make the filter buffer mutable? That way we could use the standard buffering mechanism to get flow control but also be able to inject the frame header at the end when we know the size of it?

snowp · 2018-12-18T17:11:50Z

I want to make sure they interoperate but they don't at the moment: grpc1_http_bridge doesn't manage the frame header. That said, maybe I should guard the frame header bits behind a config flag so that the default for both filters is the same, then test the interop on that. Later on I intend to make grpc1_http_bridge also optionally manage the frame header, at which point a similar interoperability test can be added.

EDIT: Actually seems like this isn't an issue, since we're going through a valid gRPC format between the filters in either case. I've added the option now so I'll just test both cases.

Signed-off-by: Snow Pettersen <[email protected]>

snowp · 2018-12-18T21:12:51Z

Unfortunately I think the two filters are not interoperable within the same filter chain at the moment, due to how addEncodedTrailers works: the trailers won't be applied to the subsequent filters until the onData chain has completed, which will never happen with the bridge filter because it buffers the data until it receives trailers.

These will still be interoperable between envoy processes (which is how I intend to use them together), since now the trailers will come in through the http2 codec which is able to trigger onTrailers while the onData chain has been stopped.

It might make sense to make this work within a filter chain (maybe addEncodedTrailers should trigger onTrailers using the dispatcher instead of relying on the filter chain to end?) but that's definitely out of the scope of this PR.

Signed-off-by: Snow Pettersen <[email protected]>

snowp · 2018-12-18T22:20:35Z

api/envoy/config/filter/http/grpc_shim/v2alpha1/grpc_shim.proto

+  // strip the gRPC frame from the request, and add it back in to the response. This will
+  // hide the gRPC semantics from the upstream, allowing it to receive and respond with a
+  // simple binary encoded protobuf.
+  bool withhold_grpc_frames = 2;


I like having this defaulted to false because it means that both the http1 bridge and this leaves the frame header alone by default.

lizan · 2018-12-18T22:40:48Z

I think you can make the interop test using two listener / HCMs within same process of Envoy, no?

snowp · 2018-12-18T22:43:47Z

Oh that's a great idea, I'll give that a go.

lizan

Thanks, this looks better modulo a few nits, and test. Are we OK with the grpc_shim name or any other suggestion?

lizan · 2018-12-18T22:43:45Z

test/extensions/filters/http/grpc_shim/grpc_shim_test.cc

@@ -0,0 +1,449 @@
+#include <netinet/in.h>


is this needed?

lizan · 2018-12-18T22:44:00Z

source/extensions/filters/http/grpc_shim/grpc_shim.cc

@@ -0,0 +1,182 @@
+#include "extensions/filters/http/grpc_shim/grpc_shim.h"
+
+#include <netinet/in.h>


is this needed?

lizan · 2018-12-18T22:44:15Z

source/extensions/filters/http/grpc_shim/grpc_shim.cc

+
+#include <netinet/in.h>
+
+#include "envoy/common/platform.h"


lizan · 2018-12-18T22:44:26Z

source/extensions/filters/http/grpc_shim/grpc_shim.cc

+
+#include "extensions/filters/http/well_known_names.h"
+
+#include "absl/strings/str_cat.h"


snowp · 2018-12-19T17:31:43Z

@lizan I'm thinking of switching to @ramaraochavali's suggestion of grpc_http1_reverse_bridge to indicate it's relationship with the other bridge. Does that sound reasonable to you?

Signed-off-by: Snow Pettersen <[email protected]>

lizan

Thanks!

/retest

repokitteh-read-only · 2018-12-27T20:31:13Z

🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #5315 (review) was submitted by @lizan.

see: more, trace.

mattklein123

Can we get more complete docs for this before merging? We should add an overview page here: https://www.envoyproxy.io/docs/envoy/latest/configuration/http_filters/http_filters and probably also describe the different conversion options here https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/grpc? Thank you!

/wait

Signed-off-by: Snow Pettersen <[email protected]>

mattklein123

Thanks for adding the extra docs. Very cool!

mattklein123 · 2019-01-03T02:44:02Z

docs/root/configuration/http_filters/grpc_http1_reverse_bridge_filter.rst

+gRPC frame header management
+----------------------------
+
+By setting the withhold_grpc_frame option, the filer will assume that the upstream does not understand any gRPC semantics


typo "filer"

super nit: as long as you are in here again can you try to flow doc lines to ~100 col if possible? Same in other files.

mattklein123 · 2019-01-03T02:45:05Z

docs/root/intro/version_history.rst

@@ -4,6 +4,7 @@ Version history
 1.10.0 (pending)
 ================
 * http: added new grpc_http1_reverse_bridge filter for converting gRPC requests into HTTP/1.1 requests.
+* access log: added a new flag for upstream retry count exceeded.


nit: alpha order

Signed-off-by: Snow Pettersen <[email protected]>

mattklein123 · 2019-01-03T20:47:07Z

@snowp LGTM, sorry, you lost the merge race again. Can you merge master one more time?

/wait

Signed-off-by: Snow Pettersen <[email protected]>

mattklein123

Thank you!

Signed-off-by: Snow Pettersen <[email protected]>

snowp · 2019-01-03T22:35:37Z

/retest

repokitteh-read-only · 2019-01-03T22:35:40Z

🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #5315 (comment) was created by @snowp.

see: more, trace.

Adds a filter that allows converting a gRPC request into an HTTP/1.1 request with a custom content-type. Allows a vanilla HTTP/1.1 upstream to handle incoming requests by reading and responding with protobuf messages in binary octet format. For now this shields the upstream from any gRPC association: the filter removes the gRPC specific message prefix and manages the conversion of the HTTP status code into grpc-status. Signed-off-by: Snow Pettersen <[email protected]> Signed-off-by: Fred Douglas <[email protected]>

snowp added 2 commits December 15, 2018 13:12

spelling

2c31790

Signed-off-by: Snow Pettersen <[email protected]>

clang tidy + asan

8cb184b

Signed-off-by: Snow Pettersen <[email protected]>

snowp added 2 commits December 16, 2018 09:53

more asan/tidy

0e53362

Signed-off-by: Snow Pettersen <[email protected]>

shared -> unique

45a5fb1

Signed-off-by: Snow Pettersen <[email protected]>

lizan reviewed Dec 16, 2018

View reviewed changes

alyssawilk assigned lizan Dec 17, 2018

snowp added 3 commits December 17, 2018 15:49

use grpc encoder/decoder, add code owner

f1d432d

Signed-off-by: Snow Pettersen <[email protected]>

reject downstream requests with too small data

891b98b

Signed-off-by: Snow Pettersen <[email protected]>

header order

b8cb892

Signed-off-by: Snow Pettersen <[email protected]>

lizan reviewed Dec 17, 2018

View reviewed changes

lizan added the waiting label Dec 17, 2018

snowp added 2 commits December 18, 2018 13:41

make frame header behavior configurable

eb4ab10

Signed-off-by: Snow Pettersen <[email protected]>

rename config flag + add UT

3218180

Signed-off-by: Snow Pettersen <[email protected]>

snowp added 2 commits December 18, 2018 16:18

move anonymous namespace

8a3bac7

Signed-off-by: Snow Pettersen <[email protected]>

no htonl and use constexpr

0007785

Signed-off-by: Snow Pettersen <[email protected]>

repokitteh-read-only bot removed the waiting label Dec 18, 2018

snowp added 2 commits December 18, 2018 16:26

buildifier

33cc878

Signed-off-by: Snow Pettersen <[email protected]>

clang-format

96684aa

Signed-off-by: Snow Pettersen <[email protected]>

snowp commented Dec 18, 2018

View reviewed changes

lizan reviewed Dec 18, 2018

View reviewed changes

lizan added the waiting label Dec 19, 2018

rename to reverse bridge

95da2a1

Signed-off-by: Snow Pettersen <[email protected]>

repokitteh-read-only bot removed the waiting label Dec 27, 2018

lizan previously approved these changes Dec 27, 2018

View reviewed changes

mattklein123 requested changes Dec 28, 2018

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 28, 2018

mattklein123 self-assigned this Dec 28, 2018

add documentation

6538ec0

Signed-off-by: Snow Pettersen <[email protected]>

snowp dismissed lizan’s stale review via 6538ec0 January 2, 2019 20:33

repokitteh-read-only bot removed the waiting label Jan 2, 2019

snowp added 4 commits January 2, 2019 15:58

fix docs

fa18e56

Signed-off-by: Snow Pettersen <[email protected]>

Merge remote-tracking branch 'origin/master' into grpc-shim

5aacc33

Signed-off-by: Snow Pettersen <[email protected]>

use new EndsWith

44699cd

Signed-off-by: Snow Pettersen <[email protected]>

header order

0e5b40f

Signed-off-by: Snow Pettersen <[email protected]>

mattklein123 requested changes Jan 3, 2019

View reviewed changes

mattklein123 added the waiting label Jan 3, 2019

alpha order, other doc nits

47e3eda

Signed-off-by: Snow Pettersen <[email protected]>

repokitteh-read-only bot removed the waiting label Jan 3, 2019

repokitteh-read-only bot added the waiting label Jan 3, 2019

snowp added 2 commits January 3, 2019 15:51

Merge remote-tracking branch 'origin/master' into grpc-shim

8264de3

Signed-off-by: Snow Pettersen <[email protected]>

alpha release notes

fcb394f

Signed-off-by: Snow Pettersen <[email protected]>

repokitteh-read-only bot removed the waiting label Jan 3, 2019

mattklein123 previously approved these changes Jan 3, 2019

View reviewed changes

add java proto options

f4909fe

Signed-off-by: Snow Pettersen <[email protected]>

snowp dismissed mattklein123’s stale review via f4909fe January 3, 2019 21:45

mattklein123 approved these changes Jan 4, 2019

View reviewed changes

mattklein123 merged commit a97e138 into envoyproxy:master Jan 4, 2019

		@@ -0,0 +1,182 @@
		#include "extensions/filters/http/grpc_shim/grpc_shim.h"

		#include <netinet/in.h>


		#include "extensions/filters/http/well_known_names.h"

		#include "absl/strings/str_cat.h"

extensions: add grpc_http1_reverse_bridge filter #5315

extensions: add grpc_http1_reverse_bridge filter #5315

Conversation

snowp commented Dec 15, 2018

snowp commented Dec 15, 2018 • edited Loading

ramaraochavali commented Dec 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowp Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowp commented Dec 18, 2018 • edited Loading

snowp commented Dec 18, 2018

Choose a reason for hiding this comment

lizan commented Dec 18, 2018

snowp commented Dec 18, 2018

lizan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowp commented Dec 19, 2018

lizan left a comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Dec 27, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Jan 3, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

snowp commented Jan 3, 2019

repokitteh-read-only bot commented Jan 3, 2019

snowp commented Dec 15, 2018 •

edited

Loading

snowp Dec 17, 2018 •

edited

Loading

snowp commented Dec 18, 2018 •

edited

Loading

lizan left a comment •

edited

Loading