feat: reloadable tls client config #7230
Conversation
github-actions
bot
added
size/M
docs-not-required
shuiyisong
force-pushed
the
feat/reload_tls_in_gRPC_client
branch
from
caf3366 to
cfcf157
Compare
shuiyisong
requested review from
a team,
waynexia and
zhongzc
as code owners
evenyag
requested review from
MichaelScofield
and removed request for
zhongzc
| pub fn maybe_watch_server_tls_config( | ||
| tls_server_config: Arc<ReloadableTlsServerConfig>, | ||
| ) -> Result<()> { | ||
| common_grpc::reloadable_tls::maybe_watch_tls_config(tls_server_config, || {}).map_err(|e| { |
There was a problem hiding this comment.
If server TLS config is changed, do we need to proactively close all connected client channels?
There was a problem hiding this comment.
The newly request would be served using the new TLS config in MySQL and PG, but the connected/alive connection stays unaffected. What do you think, should we actively close the connection to force refresh TLS change? @sunng87
There was a problem hiding this comment.
I think we don't need to. The previous connection should work seamlessly because all state is already in memory.
There was a problem hiding this comment.
But doesn't this make TLS reloading less meaningful? Is the primary concern that the server reload TLS about potential certificate leak?
| info!("Spawning background task for watching TLS cert/key file changes"); | ||
| std::thread::spawn(move || { | ||
| let _watcher = watcher; | ||
| while let Ok(res) = rx.recv() { |
There was a problem hiding this comment.
nit: Should we ignore the error here? Or add log
Signed-off-by: shuiyisong <[email protected]>
Signed-off-by: shuiyisong <[email protected]>
Signed-off-by: shuiyisong <[email protected]>
Signed-off-by: shuiyisong <[email protected]>
Signed-off-by: shuiyisong <[email protected]>
Signed-off-by: shuiyisong <[email protected]>
Signed-off-by: shuiyisong <[email protected]>
Signed-off-by: shuiyisong <[email protected]>
Signed-off-by: shuiyisong <[email protected]>
shuiyisong
force-pushed
the
feat/reload_tls_in_gRPC_client
branch
from
bfdaeda to
1e60e34
Compare
|
To be clear, in this patch we only changed intra-cluster grpc client to reload certificate. The grpc server is not covered, right? |
Yes, this only change the gRPC client(channel manager) to be able to reload the TLS change. The gRPC server of the database is not changed. |
|
This can introduce an issue if all clients reloads certs automatically but server doesn't. The updated client will not be able to connect to server without a restart. |
sunng87
left a comment
sunng87
left a comment
There was a problem hiding this comment.
Confirmed with @shuiyisong , this will be used for communication between database and external services like authentication providers or something.
But we still need to ensure the cert reload will work if intra-cluster grpc is configured with tls.
4 participants
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
This patch introduces the ability for
channel_managerto use a re-loadable client TLS config to automatically reload the TLS files when changed.The
ReloadableTlsServerConfigis now extracted intoreloadable_tlsfor reuse.PR Checklist
Please convert it to a draft if some of the following conditions are not met.