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):

if (hotReloadableFilesForServerCtx.stream().anyMatch(f -> s.isServer() && 
    logger.info("Server ssl certificates have been updated. Reseting the 
context for new peer connections.");

if (hotReloadableFilesForServerCtx.stream().anyMatch(f -> s.isClient() && 
    logger.info("Client ssl certificates have been updated. Reseting the 
context for new client connections.");

- At the top of {{SSLFactory#getSslContext()}}, you can avoid fetching the 
opposite {{SslContext}} from the one you want (server vs. client) by doing this:
SslContext sslContext
if (forServer && (sslContext = serverSslContext.get()) != null)
   return sslContext;
if (!forServer && (sslContext = clientSslContext.get()) != null)
    return sslContext;

- Logging levels in {{SSLFactory#checkCertFilesForHotReloading()}}: 
-- "Checking whether certificates have been updated" should be {{TRACE}}, at 
-- 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

To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org

Reply via email to