-
Notifications
You must be signed in to change notification settings - Fork 316
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
Conversation
41d2949
to
d61eadf
Compare
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:
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. |
Yes, you're right, and PR becoming clearer. Let me revert the changes of |
4503c3e
to
1471ca4
Compare
Codecov ReportAttention: Patch coverage is
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 |
Please let me know when the patch is ready for review @realtaobo , thank you! |
@sunng87 PTAL |
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. |
There seem to be there things to do:
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! 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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?
tonic::transport::ServerTlsConfig
Checklist