Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add grpc proxying to enable bring-your-own-proto #3277

Merged
merged 22 commits into from Jun 13, 2021
Merged

Add grpc proxying to enable bring-your-own-proto #3277

merged 22 commits into from Jun 13, 2021

Conversation

yaron2
Copy link
Member

@yaron2 yaron2 commented Jun 9, 2021

Closes #3231.
Closes #3096.
Closes #3065.

This PR enables users to work with their existing gRPC protos for service invocation, while still utilizing Dapr's service invocation features like mTLS, API auth, telemetry, access lists etc.

Notes for the reviewer

High level changes made in this PR

  1. Access lists functionality moved from pkg/configuration to pkg/acl. This is for the better and ACLs now live in a dedicated and reusable package.
  2. Adding Stream trace middleware for gRPC under grpc_diagnostics.go to support users with gRPC streams.
  3. Added the use of a gRPC transparent handler that listens to requests for services that Dapr doesn't know about (user services). This is done through the user of this package which I've reviewed. Once the review process is done, I'll make another commit to migrate the relevant code to Dapr with appropriate attributions. The reason is to remove the dependency ownership.
  4. Marked this feature as experimental, meaning it's opt-in until it graduates to a stable level.
  5. Added unit tests

Two phase review

In order to keep the list of files changed small for the actual code change, I'm waiting with adding the e2e tests. Once the initial review is done, the following will be added:

  1. E2E test
  2. Code for the transparent proxy handler

@yaron2 yaron2 changed the title add grpc proxying to enable bring-your-own-proto Add grpc proxying to enable bring-your-own-proto Jun 9, 2021
@daixiang0
Copy link
Member

Could you share more about why choose this grpc proxy and what changes have made?

Copy link
Member

@artursouza artursouza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please, address the comments above.

@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #3277 (dca6e41) into master (62ad4a3) will decrease coverage by 0.07%.
The diff coverage is 58.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3277      +/-   ##
==========================================
- Coverage   59.36%   59.28%   -0.08%     
==========================================
  Files          87       91       +4     
  Lines        7806     7973     +167     
==========================================
+ Hits         4634     4727      +93     
- Misses       2855     2919      +64     
- Partials      317      327      +10     
Impacted Files Coverage Δ
pkg/actors/actors.go 57.11% <0.00%> (ø)
pkg/config/configuration.go 71.01% <ø> (-2.79%) ⬇️
pkg/grpc/api.go 61.13% <0.00%> (+3.22%) ⬆️
pkg/messaging/direct_messaging.go 25.00% <0.00%> (-1.97%) ⬇️
pkg/runtime/cli.go 4.54% <0.00%> (ø)
pkg/runtime/runtime.go 51.98% <0.00%> (-0.32%) ⬇️
pkg/grpc/server.go 18.80% <8.33%> (-1.20%) ⬇️
pkg/diagnostics/grpc_tracing.go 64.66% <31.57%> (-15.15%) ⬇️
pkg/acl/acl.go 62.24% <62.24%> (ø)
pkg/grpc/grpc.go 48.27% <66.66%> (+0.90%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62ad4a3...dca6e41. Read the comment docs.

@yaron2
Copy link
Member Author

yaron2 commented Jun 10, 2021

Could you share more about why choose this grpc proxy and what changes have made?

I chose this library as it's complete functionality wise, very well tested, and has no dependencies other than Go stdlib and grpc libraries.

@yaron2
Copy link
Member Author

yaron2 commented Jun 10, 2021

LGTM. Please, address the comments above.

Ok, I'll now move into the second phase which is E2E and the porting of the proxy library.

@yaron2
Copy link
Member Author

yaron2 commented Jun 10, 2021

Proxy porting is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants