zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From anmolnar <...@git.apache.org>
Subject [GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...
Date Wed, 05 Dec 2018 16:49:04 GMT
Github user anmolnar commented on a diff in the pull request:

    https://github.com/apache/zookeeper/pull/680#discussion_r239138642
  
    --- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
    @@ -446,4 +458,119 @@ private void configureSSLServerSocket(SSLServerSocket sslServerSocket)
{
             LOG.debug("Using Java8-optimized cipher suites for Java version {}", javaVersion);
             return DEFAULT_CIPHERS_JAVA8;
         }
    +
    +    /**
    +     * Enables automatic reloading of the trust store and key store files when they change
on disk.
    +     *
    +     * @throws IOException if creating the FileChangeWatcher objects fails.
    +     */
    +    public void enableCertFileReloading() throws IOException {
    +        LOG.info("enabling cert file reloading");
    +        ZKConfig config = new ZKConfig();
    +        String keyStoreLocation = config.getProperty(sslKeystoreLocationProperty);
    +        if (keyStoreLocation != null && !keyStoreLocation.isEmpty()) {
    +            final Path filePath = Paths.get(keyStoreLocation).toAbsolutePath();
    +            Path parentPath = filePath.getParent();
    +            if (parentPath == null) {
    +                throw new IOException(
    +                        "Key store path does not have a parent: " + filePath);
    +            }
    +            FileChangeWatcher newKeyStoreFileWatcher = new FileChangeWatcher(
    +                    parentPath,
    +                    watchEvent -> {
    +                        handleWatchEvent(filePath, watchEvent);
    +                    });
    +            // stop old watcher if there is one
    +            if (keyStoreFileWatcher != null) {
    +                keyStoreFileWatcher.stop();
    +            }
    +            keyStoreFileWatcher = newKeyStoreFileWatcher;
    +            keyStoreFileWatcher.start();
    +        }
    +        String trustStoreLocation = config.getProperty(sslTruststoreLocationProperty);
    +        if (trustStoreLocation != null && !trustStoreLocation.isEmpty()) {
    +            final Path filePath = Paths.get(trustStoreLocation).toAbsolutePath();
    +            Path parentPath = filePath.getParent();
    +            if (parentPath == null) {
    +                throw new IOException(
    +                        "Trust store path does not have a parent: " + filePath);
    +            }
    +            FileChangeWatcher newTrustStoreFileWatcher = new FileChangeWatcher(
    +                    parentPath,
    +                    watchEvent -> {
    +                        handleWatchEvent(filePath, watchEvent);
    +                    });
    +            // stop old watcher if there is one
    +            if (trustStoreFileWatcher != null) {
    +                trustStoreFileWatcher.stop();
    +            }
    +            trustStoreFileWatcher = newTrustStoreFileWatcher;
    +            trustStoreFileWatcher.start();
    +        }
    +    }
    +
    +    /**
    +     * Disables automatic reloading of the trust store and key store files when they
change on disk.
    +     * Stops background threads and closes WatchService instances.
    +     */
    +    public void disableCertFileReloading() {
    +        if (keyStoreFileWatcher != null) {
    +            keyStoreFileWatcher.stop();
    +            keyStoreFileWatcher = null;
    +        }
    +        if (trustStoreFileWatcher != null) {
    +            trustStoreFileWatcher.stop();
    +            trustStoreFileWatcher = null;
    +        }
    +    }
    +
    +    // Finalizer guardian object, see Effective Java item 7
    +    // TODO: finalize() is deprecated starting with Java 10. This needs to be
    +    // replaced with an explicit shutdown call.
    +    @SuppressWarnings("unused")
    +    private final Object finalizerGuardian = new Object() {
    --- End diff --
    
    Reading the referenced literature about this, I believe it should be better to avoid using
finalizer like this. We might even be better avoid using finalizers entirely. There's no guarantee
when finalizer gets executed, not even guarantee to be executed at all, it has a huge performance
penalty, etc.
    
    We should rather implement an explicit `close()` method with the `AutoClosable` interface
and call it from QuorumPeer's shutdown() method.


---

Mime
View raw message