frankgh commented on code in PR #208:
URL: https://github.com/apache/cassandra-sidecar/pull/208#discussion_r2002036554
##########
server/src/main/java/org/apache/cassandra/sidecar/modules/SchedulingModule.java:
##########
@@ -69,8 +73,24 @@ public PeriodicTaskExecutor periodicTaskExecutor(Vertx vertx,
@ProvidesIntoMap
@KeyClassMapKey(PeriodicTaskMapKeys.KeyStoreCheckPeriodicTaskKey.class)
- PeriodicTask keyStoreCheckPeriodicTask(Vertx vertx, Provider<Server>
server, SidecarConfiguration configuration)
+ PeriodicTask keyStoreCheckPeriodicTask(Vertx vertx, Provider<Server>
server, SidecarConfiguration configuration, WebClientOptions webClientOptions)
{
- return new KeyStoreCheckPeriodicTask(vertx, server, configuration);
+ Function<Long, Future<Boolean>> updateServerSSLOptionsFunction =
+ lastModifiedTime ->
server.get().updateSSLOptions(lastModifiedTime).compose(v ->
Future.succeededFuture(true));
+
+ return new KeyStoreCheckPeriodicTask(vertx,
configuration.sslConfiguration(), updateServerSSLOptionsFunction,
"ServerKeyStoreCheckPeriodicTask");
Review Comment:
is the plan to use the utility method here?
```suggestion
return KeyStoreCheckPeriodicTask.forServer(vertx,
configuration.sslConfiguration(), updateServerSSLOptionsFunction);
```
##########
server/src/main/java/org/apache/cassandra/sidecar/tasks/KeyStoreCheckPeriodicTask.java:
##########
@@ -41,25 +41,49 @@ public class KeyStoreCheckPeriodicTask implements
PeriodicTask
private static final Logger LOGGER =
LoggerFactory.getLogger(KeyStoreCheckPeriodicTask.class);
private final Vertx vertx;
- private final Provider<Server> server;
- private final SslConfiguration configuration;
+ private final SslConfiguration sslConfiguration;
+ private final Function<Long, Future<Boolean>> updateSSLOptionsFunction;
private long lastModifiedTime = 0; // records the last modified timestamp
+ private final String taskName;
- public KeyStoreCheckPeriodicTask(Vertx vertx, Provider<Server> server,
SidecarConfiguration configuration)
+ public KeyStoreCheckPeriodicTask(Vertx vertx,
Review Comment:
should we make this protected instead?
```suggestion
protected KeyStoreCheckPeriodicTask(Vertx vertx,
```
##########
server/src/main/java/org/apache/cassandra/sidecar/tasks/KeyStoreCheckPeriodicTask.java:
##########
@@ -145,7 +168,10 @@ protected void maybeRecordLastModifiedTime()
*/
private boolean shouldSkip()
{
- return !configuration.isKeystoreConfigured()
- || !configuration.keystore().reloadStore();
+ if (sslConfiguration == null)
+ return true;
+ KeyStoreConfiguration keyStoreConfiguration =
sslConfiguration.keystore();
+ return !keyStoreConfiguration.isConfigured()
+ || !keyStoreConfiguration.reloadStore();
Review Comment:
let's also revert this
##########
server/src/main/java/org/apache/cassandra/sidecar/tasks/KeyStoreCheckPeriodicTask.java:
##########
@@ -41,25 +41,49 @@ public class KeyStoreCheckPeriodicTask implements
PeriodicTask
private static final Logger LOGGER =
LoggerFactory.getLogger(KeyStoreCheckPeriodicTask.class);
private final Vertx vertx;
- private final Provider<Server> server;
- private final SslConfiguration configuration;
+ private final SslConfiguration sslConfiguration;
+ private final Function<Long, Future<Boolean>> updateSSLOptionsFunction;
private long lastModifiedTime = 0; // records the last modified timestamp
+ private final String taskName;
- public KeyStoreCheckPeriodicTask(Vertx vertx, Provider<Server> server,
SidecarConfiguration configuration)
+ public KeyStoreCheckPeriodicTask(Vertx vertx,
+ SslConfiguration sslConfiguration,
+ Function<Long, Future<Boolean>>
updateSSLOptionsFunction,
+ String taskName)
{
this.vertx = vertx;
- this.server = server;
- this.configuration = configuration.sslConfiguration();
+ this.sslConfiguration = sslConfiguration;
+ this.updateSSLOptionsFunction = updateSSLOptionsFunction;
+ this.taskName = taskName;
+ }
+
+ public static KeyStoreCheckPeriodicTask forServer(Vertx vertx,
+ SslConfiguration
sslConfiguration,
+ Function<Long,
Future<Boolean>> updateSSLOptionsFunction)
+ {
+ return new KeyStoreCheckPeriodicTask(vertx, sslConfiguration,
updateSSLOptionsFunction, "ServerKeyStoreCheckPeriodicTask");
+ }
+
+ public static KeyStoreCheckPeriodicTask forClient(Vertx vertx,
+ SslConfiguration
sslConfiguration,
+ Function<Long,
Future<Boolean>> updateSSLOptionsFunction)
+ {
+ return new KeyStoreCheckPeriodicTask(vertx, sslConfiguration,
updateSSLOptionsFunction, "ClientKeyStoreCheckPeriodicTask");
+ }
+
+ @Override
+ public String name()
+ {
+ return taskName;
}
@Override
public void deploy(Vertx vertx, PeriodicTaskExecutor executor)
{
- if (configuration != null
- && configuration.enabled()
- && configuration.keystore() != null
- && configuration.keystore().isConfigured()
- && configuration.keystore().reloadStore())
+ if (sslConfiguration != null &&
+ sslConfiguration.keystore() != null
+ && sslConfiguration.keystore().isConfigured()
+ && sslConfiguration.keystore().reloadStore())
Review Comment:
I think we should still check for the configuration being enabled here, any
reason to remove the check?
```suggestion
if (sslConfiguration != null
&& configuration.enabled()
&& sslConfiguration.keystore() != null
&& sslConfiguration.keystore().isConfigured()
&& sslConfiguration.keystore().reloadStore())
```
##########
server/src/main/java/org/apache/cassandra/sidecar/modules/SchedulingModule.java:
##########
@@ -69,8 +73,24 @@ public PeriodicTaskExecutor periodicTaskExecutor(Vertx vertx,
@ProvidesIntoMap
@KeyClassMapKey(PeriodicTaskMapKeys.KeyStoreCheckPeriodicTaskKey.class)
- PeriodicTask keyStoreCheckPeriodicTask(Vertx vertx, Provider<Server>
server, SidecarConfiguration configuration)
+ PeriodicTask keyStoreCheckPeriodicTask(Vertx vertx, Provider<Server>
server, SidecarConfiguration configuration, WebClientOptions webClientOptions)
Review Comment:
can we remove web client options here?
```suggestion
PeriodicTask keyStoreCheckPeriodicTask(Vertx vertx, Provider<Server>
server, SidecarConfiguration configuration)
```
##########
server/src/main/java/org/apache/cassandra/sidecar/modules/SchedulingModule.java:
##########
@@ -69,8 +73,24 @@ public PeriodicTaskExecutor periodicTaskExecutor(Vertx vertx,
@ProvidesIntoMap
@KeyClassMapKey(PeriodicTaskMapKeys.KeyStoreCheckPeriodicTaskKey.class)
- PeriodicTask keyStoreCheckPeriodicTask(Vertx vertx, Provider<Server>
server, SidecarConfiguration configuration)
+ PeriodicTask keyStoreCheckPeriodicTask(Vertx vertx, Provider<Server>
server, SidecarConfiguration configuration, WebClientOptions webClientOptions)
{
- return new KeyStoreCheckPeriodicTask(vertx, server, configuration);
+ Function<Long, Future<Boolean>> updateServerSSLOptionsFunction =
+ lastModifiedTime ->
server.get().updateSSLOptions(lastModifiedTime).compose(v ->
Future.succeededFuture(true));
+
+ return new KeyStoreCheckPeriodicTask(vertx,
configuration.sslConfiguration(), updateServerSSLOptionsFunction,
"ServerKeyStoreCheckPeriodicTask");
+ }
+
+ @ProvidesIntoMap
+
@KeyClassMapKey(PeriodicTaskMapKeys.ClientKeyStoreCheckPeriodicTaskKey.class)
+ PeriodicTask clientKeyStoreCheckPeriodicTask(Vertx vertx,
+ SidecarClientProvider
sidecarClientProvider,
+ WebClientOptions
webClientOptions,
+ SidecarConfiguration
configuration)
+ {
+ Function<Long, Future<Boolean>> updateClientSSLOptionsFunction =
+ lastModifiedTime ->
sidecarClientProvider.getHttpClient().updateSSLOptions(webClientOptions.getSslOptions()).compose(v
-> Future.succeededFuture(true));
+
+ return new KeyStoreCheckPeriodicTask(vertx,
configuration.sslConfiguration(), updateClientSSLOptionsFunction,
"ClientKeyStoreCheckPeriodicTask");
Review Comment:
should we use the utility method here?
```suggestion
return KeyStoreCheckPeriodicTask.forClient(vertx,
configuration.sslConfiguration(), updateClientSSLOptionsFunction);
```
##########
server/src/main/java/org/apache/cassandra/sidecar/tasks/KeyStoreCheckPeriodicTask.java:
##########
@@ -82,14 +106,14 @@ public ScheduleDecision scheduleDecision()
@Override
public DurationSpec delay()
{
- return configuration.keystore().checkInterval();
+ return sslConfiguration == null ? DEFAULT_DELAY :
sslConfiguration.keystore().checkInterval();
Review Comment:
this shouldn't be necessary, I wonder what's the change that requires this
change
##########
server/src/main/java/org/apache/cassandra/sidecar/utils/SidecarClientProvider.java:
##########
@@ -89,10 +104,16 @@ public void close()
}
}
+ // Exposing the used http client to be reused on periodic tasks
+ public HttpClient getHttpClient()
Review Comment:
let's not expose the http client here, instead let's just expose a method to
update ssl options.
```suggestion
public Future<Boolean> updateSSLOptions()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
