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

feat: Add TLS support for gRPC service #3957

Merged
merged 10 commits into from
May 23, 2024
Merged

Conversation

realtaobo
Copy link
Contributor

@realtaobo realtaobo commented May 16, 2024

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

close #3336

What's changed and what's your intention?

  • Add grpc service tls configure
  • use tonic::transport::ServerTlsConfig

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR requires documentation updates.

@github-actions github-actions bot added the docs-not-required This change does not impact docs. label May 16, 2024
@realtaobo realtaobo force-pushed the grpc-tls branch 2 times, most recently from 41d2949 to d61eadf Compare May 17, 2024 07:55
@realtaobo realtaobo marked this pull request as ready for review May 17, 2024 08:14
@realtaobo realtaobo requested a review from a team as a code owner May 17, 2024 08:14
@tisonkun tisonkun requested a review from sunng87 May 18, 2024 00:53
@sunng87
Copy link
Member

sunng87 commented May 18, 2024

It's great to see this! Thank you @realtaobo!

Before we get deep, I want to make sure that actually there are two types of grpc communications in GreptimeDB cluster:

  • the client-server grpc: by which our sdk talks to the frontends.
  • the intra-cluster grpc: by which frontends/datanodes/metasrvs talks to each other.

If we are going to implement both, we need to make sure that they may or must use different tls configuration including certificates/keys/modes. We will need separated configuration items for that.

I will recommend you to do them in separated patches. Perhaps with this one, we focus on the client-server grpc first.

@realtaobo
Copy link
Contributor Author

I will recommend you to do them in separated patches. Perhaps with this one, we focus on the client-server grpc first.

Yes, you're right, and PR becoming clearer. Let me revert the changes of DataNode. And the intra-cluster grpc tls will be impl in next patches.

@realtaobo realtaobo force-pushed the grpc-tls branch 2 times, most recently from 4503c3e to 1471ca4 Compare May 20, 2024 06:43
Copy link

codecov bot commented May 20, 2024

Codecov Report

Attention: Patch coverage is 95.55556% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 85.16%. Comparing base (614643e) to head (1172458).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3957      +/-   ##
==========================================
- Coverage   85.47%   85.16%   -0.31%     
==========================================
  Files         980      980              
  Lines      170117   170158      +41     
==========================================
- Hits       145406   144914     -492     
- Misses      24711    25244     +533     

@sunng87
Copy link
Member

sunng87 commented May 21, 2024

Please let me know when the patch is ready for review @realtaobo , thank you!

@realtaobo
Copy link
Contributor Author

@sunng87 PTAL

@sunng87
Copy link
Member

sunng87 commented May 22, 2024

It mostly looks good to me. If you have time, you may want to look into tonic to see if there is a chance to make certificate reloadable without restarting the server.

@sunng87 sunng87 requested a review from zyy17 May 22, 2024 21:44
src/common/grpc/tests/tls/server.pem Outdated Show resolved Hide resolved
src/datanode/src/service.rs Show resolved Hide resolved
src/frontend/src/service_config/grpc.rs Show resolved Hide resolved
@realtaobo
Copy link
Contributor Author

realtaobo commented May 23, 2024

There seem to be there things to do:

  • Support the intra-cluster grpc communications with TLS, from comment
  • Refactored DatanodeOptions, that is replace some grpc config(max_recv_message_size, etc.) with GrpcOptions, from comment
  • It seems that grpc,mysql,pg are all using the same TLS configuration directly. We could refactor these with: Reading the TLS configuration from config file firstly, from comment

I believe these tasks should be deferred to the future PRs, and we should keep the PR simple for now. @sunng87 @zyy17 How do you think so?

@zyy17
Copy link
Collaborator

zyy17 commented May 23, 2024

There seem to be there things to do:

  • Support the intra-cluster grpc communications with TLS, from comment
  • Refactored DatanodeOptions, that is replace some grpc config(max_recv_message_size, etc.) with GrpcOptions, from comment
  • It seems that grpc,mysql,pg are all using the same TLS configuration directly. We could refactor these with: Reading the TLS configuration from config file firstly, from comment

I believe these tasks should be deferred to the future PRs, and we should keep the PR simple for now. @sunng87 @zyy17 How do you think so?

I think so. Thank you for your summary! 😊

Copy link
Collaborator

@zyy17 zyy17 left a comment

Choose a reason for hiding this comment

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

LGTM

@sunng87 sunng87 added this pull request to the merge queue May 23, 2024
Merged via the queue into GreptimeTeam:main with commit a3a2c8d May 23, 2024
36 checks passed
@realtaobo realtaobo deleted the grpc-tls branch May 24, 2024 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TLS for gRPC service
3 participants