zhtaoxiang commented on code in PR #12325:
URL: https://github.com/apache/pinot/pull/12325#discussion_r1468946352
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/TlsUtils.java:
##########
@@ -322,6 +351,119 @@ public static SslContext buildServerContext(TlsConfig
tlsConfig) {
}
}
+ /**
+ * check if the key store or trust store path is null or has file scheme.
+ *
+ * @param keyOrTrustStorePath key store or trust store path in String format.
+ */
+ public static boolean isKeyOrTrustStorePathNullOrHasFileScheme(String
keyOrTrustStorePath) {
+ try {
+ return keyOrTrustStorePath == null
+ ||
makeKeyOrTrustStoreUrl(keyOrTrustStorePath).toURI().getScheme().startsWith(FILE_SCHEME);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Enables auto renewal of SSLFactory when
+ * 1. the {@link SSLFactory} is created with a key manager and trust manager
swappable
+ * 2. the key store is null or a local file
+ * 3. the trust store is null or a local file
+ * 4. the key store or trust store file changes.
+ * @param sslFactory the {@link SSLFactory} to enable key manager and trust
manager auto renewal
+ * @param tlsConfig the {@link TlsConfig} to get the key store and trust
store information
+ */
+ public static void enableAutoRenewalFromFileStoreForSSLFactory(SSLFactory
sslFactory, TlsConfig tlsConfig) {
+ enableAutoRenewalFromFileStoreForSSLFactory(sslFactory,
+ tlsConfig.getKeyStoreType(), tlsConfig.getKeyStorePath(),
tlsConfig.getKeyStorePassword(),
+ tlsConfig.getTrustStoreType(), tlsConfig.getTrustStorePath(),
tlsConfig.getTrustStorePassword(),
+ null, null);
+ }
+
+ private static void enableAutoRenewalFromFileStoreForSSLFactory(
+ SSLFactory sslFactory,
+ String keyStoreType, String keyStorePath, String keyStorePassword,
+ String trustStoreType, String trustStorePath, String trustStorePassword,
+ String sslContextProtocol, SecureRandom secureRandom) {
+ try {
+ URL keyStoreURL = keyStorePath == null ? null :
makeKeyOrTrustStoreUrl(keyStorePath);
+ URL trustStoreURL = trustStorePath == null ? null :
makeKeyOrTrustStoreUrl(trustStorePath);
+ if (keyStoreURL != null) {
+ Preconditions.checkArgument(
+ keyStoreURL.toURI().getScheme().startsWith(FILE_SCHEME),
+ "key store path must be a local file path or null when SSL auto
renew is enabled");
+ Preconditions.checkArgument(
+ sslFactory.getKeyManager().isPresent()
+ && sslFactory.getKeyManager().get() instanceof
HotSwappableX509ExtendedKeyManager,
+ "key manager of the existing SSLFactory must be swappable"
+ );
+ }
+ if (trustStoreURL != null) {
+ Preconditions.checkArgument(
+ trustStoreURL.toURI().getScheme().startsWith(FILE_SCHEME),
+ "trust store path must be a local file path or null when SSL auto
renew is enabled");
+ Preconditions.checkArgument(
+ sslFactory.getTrustManager().isPresent()
+ && sslFactory.getTrustManager().get() instanceof
HotSwappableX509ExtendedTrustManager,
+ "trust manager of the existing SSLFactory must be swappable"
+ );
+ }
+ // The reloadSslFactoryWhenFileStoreChanges is a blocking call, so we
need to create a new thread to run it.
+ // Creating a new thread to run the reloadSslFactoryWhenFileStoreChanges
is costly; however, unless we
+ // invoke the createAutoRenewedSSLFactoryFromFileStore method crazily,
this should not be a problem.
+ Executors.newSingleThreadExecutor().execute(() -> {
Review Comment:
That is a good point. Initially I had the same concern.
> Since we call it when we initialize GrpcQueryClient. It looks that we will
create GrpcQueryClient every time we hit broker query api
However, the constructor of `GrpcQueryClient` is only used here:
https://github.com/apache/pinot/blob/master/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/GrpcBrokerRequestHandler.java#L153,
and the constructor of `GrpcQueryServer` is only used here
https://github.com/apache/pinot/blob/master/pinot-server/src/main/java/org/apache/pinot/server/starter/ServerInstance.java#L159,
they will only be created limited number of times. If no new usage pattern
added, there will be no problem for the current implementation.
Let's take a step back first. In theory, there are two ways to handle files
updates:
1. Push based solution: the one that's implemented now.
2. Pull based solution: add a background periodic task that monitors the ssl
files and make the change if needed.
In either case, we need to create a task to wait for push event or pull
changes. If the function is invoked numerous times, then equal number of tasks
need to be added.
To solve the issue, I am thinking about reuse the same SSLFactory instance
if key and trust store files are the same for different invokation of the same
method. This will solve the problem if not many different key and trust store
files are used - which will normally be the case in practice.
WDYT?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
