Skip to content

Conversation

@shuiyisong
Copy link
Contributor

@shuiyisong shuiyisong commented Nov 14, 2025

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_manager to use a re-loadable client TLS config to automatically reload the TLS files when changed.
The ReloadableTlsServerConfig is now extracted into reloadable_tls for reuse.

PR Checklist

Please convert it to a draft if some of the following conditions are not met.

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR requires documentation updates.
  • API changes are backward compatible.
  • Schema or data changes are backward compatible.

@github-actions github-actions bot added size/M docs-not-required This change does not impact docs. labels Nov 14, 2025
@shuiyisong shuiyisong force-pushed the feat/reload_tls_in_gRPC_client branch from caf3366 to cfcf157 Compare November 17, 2025 06:58
@shuiyisong shuiyisong marked this pull request as ready for review November 17, 2025 06:58
@shuiyisong shuiyisong requested review from a team, waynexia and zhongzc as code owners November 17, 2025 06:58
@evenyag evenyag requested review from MichaelScofield and removed request for zhongzc November 18, 2025 06:49
@fengys1996 fengys1996 self-requested a review November 18, 2025 07:40
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| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If server TLS config is changed, do we need to proactively close all connected client channels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@shuiyisong shuiyisong requested a review from discord9 as a code owner November 19, 2025 03:32
info!("Spawning background task for watching TLS cert/key file changes");
std::thread::spawn(move || {
let _watcher = watcher;
while let Ok(res) = rx.recv() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should we ignore the error here? Or add log

@shuiyisong shuiyisong force-pushed the feat/reload_tls_in_gRPC_client branch from bfdaeda to 1e60e34 Compare November 20, 2025 06:15
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. size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants