[
https://issues.apache.org/jira/browse/CASSANDRA-14222?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16357044#comment-16357044
]
Jason Brown commented on CASSANDRA-14222:
-----------------------------------------
Review comments:
- In {{SSLFactory#initHotReloading()}}, the else block is unnecessary and
potentially confusing, if the method happens to be called twice, as the logging
says {{"Disabling hot reloading SSLContext"}}. We won't actually disable the
logging if {{isHotReloadingInitialized}} is true.
- In {{SSLfactory#initHotReloadableFiles()}}, add an additional check for
'enabled' to both server and client lists. That is, if
{{server_encryption_options.enabled}} is false, don't bother adding the
{{HotReloadableFile}} for it. (same for client)
- We typically don't pass around the {{Config}} instance, so for the
{{SSLfactory}} methods please pass the relevant {{EncryptionOptions}}
instances. In {{MessagingService#reloadSslCerts()}}, call the methods on
{{DatabaseDescriptor}} for getting the {{EncryptionOptions}} instances rather
than passing the raw config.
- Instead of having two lists of {{List<HotReloadableFile>}} in {{SSLFactory}},
can we merge it down to one? We can add a flag to {{HotReloadableFile}} to
indicate if it's server or client. Then you could have code like this in
{{SSLFactory#checkCertFilesForHotReloading()}} (suit to taste):
{noformat}
if (hotReloadableFilesForServerCtx.stream().anyMatch(f -> s.isServer() &&
f.shouldReload()))
{
logger.info("Server ssl certificates have been updated. Reseting the
context for new peer connections.");
serverSslContext.set(null);
}
if (hotReloadableFilesForServerCtx.stream().anyMatch(f -> s.isClient() &&
f.shouldReload()))
{
logger.info("Client ssl certificates have been updated. Reseting the
context for new client connections.");
clientSslContext.set(null);
}
{noformat}
nits:
- At the top of {{SSLFactory#getSslContext()}}, you can avoid fetching the
opposite {{SslContext}} from the one you want (server vs. client) by doing this:
{noformat}
SslContext sslContext
if (forServer && (sslContext = serverSslContext.get()) != null)
return sslContext;
if (!forServer && (sslContext = clientSslContext.get()) != null)
return sslContext;
{noformat}
- Logging levels in {{SSLFactory#checkCertFilesForHotReloading()}}:
-- "Checking whether certificates have been updated" should be {{TRACE}}, at
best
-- others should be {{DEBUG}}
> Add hot reloading of SSL Certificates capability to Cassandra
> -------------------------------------------------------------
>
> Key: CASSANDRA-14222
> URL: https://issues.apache.org/jira/browse/CASSANDRA-14222
> Project: Cassandra
> Issue Type: New Feature
> Reporter: Dinesh Joshi
> Assignee: Dinesh Joshi
> Priority: Major
> Fix For: 4.x
>
>
> Cassandra does not currently hot reload SSL certificates. For ease of
> operation it would be useful if we add this capability. This patch would
> watch changes to the truststore & keystore and would reload them when they're
> changed.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
