Add filters for Buf Connect RPC protocol. #24836

jchadwick-buf · 2023-01-09T18:14:56Z

Signed-off-by: John Chadwick jchadwick@buf.build

Implements two Connect filters:

connect_stats: Similar to grpc_stats. Counts messages and success/failure as stats.
connect_grpc_bridge: Provides a bridge from Connect clients to gRPC servers.

The Buf Connect protocol provides similar features to gRPC, but supports simple HTTP/1.1 requests for unary procedure calls and does not require trailers for streaming procedure calls.

The stats filter is similar to grpc_stats, as mentioned prior. It could potentially be rolled into grpc_stats, although the Connect protocol does differ from gRPC in a fair few ways, so I do anticipate the resulting code would be a little hard to follow. The bridge filter is not exactly analogous to any existing gRPC filters. For streaming Buf Connect requests, it is somewhat similar to the grpc_web filter, but for unary Buf Connect requests, it's more similar to grpc_http1_bridge.

The bridge filter is mainly useful for customers who are interested in transitioning to using Connect incrementally, whereas the stats filter is useful for anyone who wants to ensure that their stats covers usage of the Connect protocol, either independently or in addition to gRPC.

Closes #22679.

repokitteh-read-only · 2023-01-09T18:15:04Z

Hi @jchadwick-buf, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #24836 was opened by jchadwick-buf.

see: more, trace.

repokitteh-read-only · 2023-01-09T18:15:05Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #24836 was opened by jchadwick-buf.

see: more, trace.

repokitteh-read-only · 2023-01-09T18:15:21Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #24836 was opened by jchadwick-buf.

see: more, trace.

akshayjshah · 2023-01-09T19:01:35Z

@mattklein123 appreciate you taking the time to chat with us today and maybe sponsor this contribution. @jchadwick-buf is going to highlight areas of the diff which could use a maintainer's eye and point out the few areas where we still have a little work to do.

source/extensions/filters/http/connect_grpc_bridge/end_stream_response.cc

source/extensions/filters/http/connect_stats/connect_stats_filter.cc

source/extensions/filters/http/connect_stats/response_frame_counter.cc

source/extensions/filters/http/connect_grpc_bridge/config.cc

mattklein123

At a high level this makes sense. I left some comments and I'm happy to sponsor. Thanks.

/wait

source/extensions/filters/http/connect_grpc_bridge/config.cc

source/extensions/filters/http/connect_grpc_bridge/end_stream_response.cc

source/extensions/filters/http/connect_stats/connect_stats_filter.cc

source/extensions/filters/http/connect_stats/response_frame_counter.cc

Signed-off-by: John Chadwick <jchadwick@buf.build>

- Merges connect_stats with grpc_stats. - Merges unary+streaming filters in connect-grpc bridge. - Moves to using Protobuf Struct for JSON encoding/decoding. - Improves coverage of test suite. Signed-off-by: John Chadwick <jchadwick@buf.build>

Signed-off-by: John Chadwick <jchadwick@buf.build>

gRPC requires the :te header to be set to trailers. It was missing for the streaming case, which didn't bother gRPC-go, but it did break grpcpp. Signed-off-by: John Chadwick <jchadwick@buf.build>

Signed-off-by: John Chadwick <jchadwick@buf.build>

jchadwick-buf · 2023-01-26T19:16:45Z

I don't know why it says I added and then removed reviewers: I didn't, I only hit "Ready for Review." In any case, I think this is ready for review now. I've tested it in many scenarios and I've tried to cover the code with an adequate amount of tests. If desired, I can split this into more PRs, merge/rebase commits, etc. I tried to avoid too much rebasing for the moment so that the progres/changes were not obscured.

adisuissa

Thanks for your contribution!

I'm not sure if this should be part of contrib or extensions. @mattklein123 do you have any take on this?

adisuissa · 2023-01-30T14:14:54Z

api/envoy/extensions/filters/http/grpc_stats/v3/config.proto

+
+  // If true, the filter will instrument `Buf Connect <https://connect.build/>` protocol messages in
+  // addition to gRPC.
+  bool enable_buf_connect_support = 6;


I might be missing something here, but the filter is for GrpcStats, and Buf-Connect is a different protocol, so shouldn't there be a BufConnectStats filter (that has the relevant config)?

What we wound up doing was merging Buf Connect support into the gRPC stats filter due to the overlap between the protocols. (Connect is semantically compatible with gRPC, so it's somewhat akin to say, gRPC-web.) I'm open to trying to make this make more sense. Context: #24836 (comment)

Given the similarity of the protocols I think it's reasonable to have it be part of this filter. We can potentially make this more clear in the docs. I will take a look.

Is there any reason to actually have this be configurable? It's turned on by content-type, right? Can we just turn it on automatically and then just document that it is supported somewhere in the top level filter documentation?

mattklein123 · 2023-02-01T22:48:23Z

Sorry for the review delay. I will get back to this within the next week.

mattklein123

Thanks at a high level this LGTM. Given the alpha status I think it's fine to ship and iterate with some real customers.

Note that if we want to call this eventually robust to untrusted data we should have some limited fuzzing of all of the relevant parsing code. This can be done later.

Thank you!

/wait

mattklein123 · 2023-02-07T19:30:59Z

api/envoy/extensions/filters/http/grpc_stats/v3/config.proto

+
+  // If true, the filter will instrument `Buf Connect <https://connect.build/>` protocol messages in
+  // addition to gRPC.
+  bool enable_buf_connect_support = 6;


Is there any reason to actually have this be configurable? It's turned on by content-type, right? Can we just turn it on automatically and then just document that it is supported somewhere in the top level filter documentation?

mattklein123 · 2023-02-07T19:32:53Z

docs/root/configuration/http/http_filters/connect_grpc_bridge_filter.rst

@@ -0,0 +1,11 @@
+.. _config_http_filters_connect_grpc_bridge:


Can you also update relevant docs here: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/other_protocols/grpc

And ref link back to this filter where appropriate.

I just now added the bridge to the list, is that what you mean? Or should it have a section just for connect?

mattklein123 · 2023-02-07T19:42:03Z

test/extensions/filters/http/connect_grpc_bridge/connect_grpc_bridge_integration_test.cc

+namespace Envoy {
+namespace {
+
+class ConnectIntegrationTest : public testing::TestWithParam<Network::Address::IpVersion>,


Can you have an integration test for the actual end to end flow for both unary and streaming?

In general can you also check the code coverage report and make sure you are reasonably covering all of the code?

Done.

I did try to ensure the important cases are covered, and we have >96% on everything. We can aim for 100% if you would prefer though. I think the main tricky case is trying to find a way to get WKT JSON encoding to fail.

Signed-off-by: John Chadwick <jchadwick@buf.build>

mattklein123

Great, thanks!

jchadwick-buf · 2023-02-08T20:03:28Z

Thanks! Github says workflows need approval still. Did I need to do something there?

repokitteh-read-only bot added the api label Jan 9, 2023

repokitteh-read-only bot assigned adisuissa Jan 9, 2023

jchadwick-buf commented Jan 9, 2023

View reviewed changes

source/extensions/filters/http/connect_grpc_bridge/end_stream_response.cc Outdated Show resolved Hide resolved

jchadwick-buf commented Jan 9, 2023

View reviewed changes

source/extensions/filters/http/connect_stats/connect_stats_filter.cc Outdated Show resolved Hide resolved

jchadwick-buf commented Jan 9, 2023

View reviewed changes

source/extensions/filters/http/connect_stats/response_frame_counter.cc Outdated Show resolved Hide resolved

jchadwick-buf commented Jan 9, 2023

View reviewed changes

source/extensions/filters/http/connect_grpc_bridge/config.cc Outdated Show resolved Hide resolved

mattklein123 self-assigned this Jan 9, 2023

akshayjshah mentioned this pull request Jan 9, 2023

Add support for Connect RPC protocol #22679

Closed

mattklein123 requested changes Jan 10, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 10, 2023

repokitteh-read-only bot removed the waiting label Jan 20, 2023

jchadwick-buf force-pushed the connect-filters branch 3 times, most recently from 200619f to b983ece Compare January 25, 2023 14:05

jchadwick-buf added 12 commits January 25, 2023 21:43

Buf Connect filters proof-of-concept.

01e2618

Signed-off-by: John Chadwick <jchadwick@buf.build>

fixup: Fix proto format issue.

c604e19

Signed-off-by: John Chadwick <jchadwick@buf.build>

fixup: Updates to address feedback.

18baf8e

- Merges connect_stats with grpc_stats. - Merges unary+streaming filters in connect-grpc bridge. - Moves to using Protobuf Struct for JSON encoding/decoding. - Improves coverage of test suite. Signed-off-by: John Chadwick <jchadwick@buf.build>

fixup: Small tweaks (doc stub, dead constants)

c48e13c

Signed-off-by: John Chadwick <jchadwick@buf.build>

fixup: Always set TE for streaming connection.

c5ca881

gRPC requires the :te header to be set to trailers. It was missing for the streaming case, which didn't bother gRPC-go, but it did break grpcpp. Signed-off-by: John Chadwick <jchadwick@buf.build>

fixup: Fix incongruities in Connect stat counts.

2043eac

Signed-off-by: John Chadwick <jchadwick@buf.build>

fixup: Treat missing Connect error code as error.

30f4c2e

Signed-off-by: John Chadwick <jchadwick@buf.build>

fixup: Expand gRPC Stats tests.

2f5b62c

Signed-off-by: John Chadwick <jchadwick@buf.build>

fixup: Expand Connect-gRPC Bridge tests.

cb0b42c

Signed-off-by: John Chadwick <jchadwick@buf.build>

fixup: Add tests for EndStreamResponse.

6d223f8

Signed-off-by: John Chadwick <jchadwick@buf.build>

fixup: Do not handle headers as trailer metadata.

4759dc7

Signed-off-by: John Chadwick <jchadwick@buf.build>

fixup: Better test suite coverage.

f2afca9

Signed-off-by: John Chadwick <jchadwick@buf.build>

jchadwick-buf force-pushed the connect-filters branch from f9b6e1b to 21930ca Compare January 26, 2023 02:45

fixup: Fix dependency on google.protobuf in test.

271aaa9

Signed-off-by: John Chadwick <jchadwick@buf.build>

jchadwick-buf force-pushed the connect-filters branch from 21930ca to 271aaa9 Compare January 26, 2023 05:56

jchadwick-buf marked this pull request as ready for review January 26, 2023 12:44

jchadwick-buf requested review from kyessenov and lizan as code owners January 26, 2023 12:44

jchadwick-buf requested review from mattklein123 and removed request for lizan and kyessenov January 26, 2023 12:44

adisuissa reviewed Jan 30, 2023

View reviewed changes

joshcarp mentioned this pull request Feb 2, 2023

Proxy support connectrpc/connect-go#450

Closed

mattklein123 requested changes Feb 7, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 7, 2023

jchadwick-buf added 4 commits February 7, 2023 20:52

Additional docs for Connect support.

b83f97f

Signed-off-by: John Chadwick <jchadwick@buf.build>

Remove enable_buf_connect_support option.

e734c65

Signed-off-by: John Chadwick <jchadwick@buf.build>

Fix sometimes-missing response content-type.

6204ea1

Signed-off-by: John Chadwick <jchadwick@buf.build>

Add integration tests for Connect request flows.

089d792

Signed-off-by: John Chadwick <jchadwick@buf.build>

repokitteh-read-only bot removed the waiting label Feb 8, 2023

Streaming E2E test should check eos frame.

565cbc7

Signed-off-by: John Chadwick <jchadwick@buf.build>

mattklein123 approved these changes Feb 8, 2023

View reviewed changes

repokitteh-read-only bot removed the api label Feb 8, 2023

mattklein123 enabled auto-merge (squash) February 8, 2023 16:18

mattklein123 merged commit 455409c into envoyproxy:main Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add filters for Buf Connect RPC protocol. #24836

Add filters for Buf Connect RPC protocol. #24836

jchadwick-buf commented Jan 9, 2023 •

edited

Loading

repokitteh-read-only bot commented Jan 9, 2023

repokitteh-read-only bot commented Jan 9, 2023

repokitteh-read-only bot commented Jan 9, 2023

akshayjshah commented Jan 9, 2023

mattklein123 left a comment

jchadwick-buf commented Jan 26, 2023

adisuissa left a comment

adisuissa Jan 30, 2023

jchadwick-buf Jan 30, 2023 •

edited

Loading

mattklein123 Jan 30, 2023

mattklein123 Feb 7, 2023

jchadwick-buf Feb 8, 2023

mattklein123 commented Feb 1, 2023

mattklein123 left a comment

mattklein123 Feb 7, 2023

mattklein123 Feb 7, 2023

jchadwick-buf Feb 8, 2023 •

edited

Loading

mattklein123 Feb 7, 2023

jchadwick-buf Feb 8, 2023

mattklein123 left a comment

jchadwick-buf commented Feb 8, 2023

		@@ -0,0 +1,11 @@
		.. _config_http_filters_connect_grpc_bridge:

Add filters for Buf Connect RPC protocol. #24836

Add filters for Buf Connect RPC protocol. #24836

Conversation

jchadwick-buf commented Jan 9, 2023 • edited Loading

repokitteh-read-only bot commented Jan 9, 2023

repokitteh-read-only bot commented Jan 9, 2023

repokitteh-read-only bot commented Jan 9, 2023

akshayjshah commented Jan 9, 2023

mattklein123 left a comment

Choose a reason for hiding this comment

jchadwick-buf commented Jan 26, 2023

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchadwick-buf Jan 30, 2023 • 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

mattklein123 commented Feb 1, 2023

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

jchadwick-buf Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

jchadwick-buf commented Feb 8, 2023

jchadwick-buf commented Jan 9, 2023 •

edited

Loading

jchadwick-buf Jan 30, 2023 •

edited

Loading

jchadwick-buf Feb 8, 2023 •

edited

Loading