From 65e716b82cd719a4a63b582eed3380b85229b8e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Per=20Otterstr=C3=B6m?= Date: Mon, 30 Mar 2020 08:30:20 +0200 Subject: [PATCH] Use java.time and java.nio in SSL ReloadWatcher Closing TODO. --- .../exporter/netty/ssl/ReloadWatcher.java | 66 ++++++++++++------- .../exporter/netty/ssl/SslContextFactory.java | 2 +- .../exporter/netty/ssl/TestReloadWatcher.java | 44 ++++++++----- .../exporter/netty/ssl/TestSslSupport.java | 20 ++++-- 4 files changed, 83 insertions(+), 49 deletions(-) diff --git a/common/src/main/java/com/zegelin/cassandra/exporter/netty/ssl/ReloadWatcher.java b/common/src/main/java/com/zegelin/cassandra/exporter/netty/ssl/ReloadWatcher.java index 958d43f..358a06f 100644 --- a/common/src/main/java/com/zegelin/cassandra/exporter/netty/ssl/ReloadWatcher.java +++ b/common/src/main/java/com/zegelin/cassandra/exporter/netty/ssl/ReloadWatcher.java @@ -5,61 +5,66 @@ import org.slf4j.LoggerFactory; import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.FileTime; +import java.time.Duration; +import java.time.Instant; import java.util.Collection; import java.util.Objects; -import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import java.util.stream.Stream; -// TODO: Switch to using java.time.* classes (Instant, Duration, etc) and java.nio.Path public class ReloadWatcher { private static final Logger logger = LoggerFactory.getLogger(ReloadWatcher.class); - private static final long RELOAD_MARGIN_MILLIS = 1000; - private final long intervalInMs; - private final Collection files; + private static final Duration RELOAD_MARGIN = Duration.ofMillis(1000); + private final Duration reloadInterval; + private final Collection files; - private long nextReloadAt; - private long reloadedAt; + private Instant nextReloadAt; + private Instant reloadedAt; public ReloadWatcher(final HttpServerOptions httpServerOptions) { - intervalInMs = TimeUnit.SECONDS.toMillis(httpServerOptions.sslReloadIntervalInSeconds); + reloadInterval = Duration.ofSeconds(httpServerOptions.sslReloadIntervalInSeconds); files = Stream.of(httpServerOptions.sslServerKeyFile, httpServerOptions.sslServerKeyPasswordFile, httpServerOptions.sslServerCertificateFile, httpServerOptions.sslTrustedCertificateFile) .filter(Objects::nonNull) + .map(File::toPath) .collect(Collectors.toSet()); logger.info("Watching {} for changes every {} seconds.", this.files, httpServerOptions.sslReloadIntervalInSeconds); - reset(System.currentTimeMillis()); + reset(Instant.now()); } - private void reset(final long now) { - // Create a 1 second margin to compensate for poor resolution of File.lastModified() - reloadedAt = now - RELOAD_MARGIN_MILLIS; + private void reset(final Instant now) { + // Create a 1 second margin to compensate for poor resolution of Files.getLastModifiedTime() + reloadedAt = now.minus(RELOAD_MARGIN); - nextReloadAt = now + intervalInMs; + nextReloadAt = now.plus(reloadInterval); logger.debug("Next reload at {}.", nextReloadAt); } public synchronized void forceReload() { - if (!enabled()) { + if (disabled()) { return; } logger.info("Forced reload of exporter certificates on next scrape."); - reloadedAt = 0L; - nextReloadAt = 0L; + reloadedAt = Instant.EPOCH; + nextReloadAt = Instant.EPOCH; } boolean needReload() { - if (!enabled()) { + if (disabled()) { return false; } - final long now = System.currentTimeMillis(); + final Instant now = Instant.now(); if (timeToPoll(now)) { return reallyNeedReload(now); @@ -68,15 +73,15 @@ boolean needReload() { return false; } - private boolean enabled() { - return intervalInMs > 0; + private boolean disabled() { + return reloadInterval.isZero(); } - private boolean timeToPoll(final long now) { - return now > nextReloadAt; + private boolean timeToPoll(final Instant now) { + return now.isAfter(nextReloadAt); } - private synchronized boolean reallyNeedReload(final long now) { + private synchronized boolean reallyNeedReload(final Instant now) { if (timeToPoll(now)) { try { return anyFileModified(); @@ -88,6 +93,19 @@ private synchronized boolean reallyNeedReload(final long now) { } private boolean anyFileModified() { - return files.stream().anyMatch(f -> f.lastModified() > reloadedAt); + return files.stream() + .map(this::getLastModifiedTimeSafe) + .map(FileTime::toMillis) + .map(Instant::ofEpochMilli) + .anyMatch(lastModifiedAt -> lastModifiedAt.isAfter(reloadedAt)); + } + + private FileTime getLastModifiedTimeSafe(final Path path) { + try { + return Files.getLastModifiedTime(path); + } catch (IOException e) { + logger.warn("Unable to get modification time of file {} - forcing reload.", path, e); + return FileTime.from(Instant.MAX); + } } } diff --git a/common/src/main/java/com/zegelin/cassandra/exporter/netty/ssl/SslContextFactory.java b/common/src/main/java/com/zegelin/cassandra/exporter/netty/ssl/SslContextFactory.java index b1530cc..817a35b 100644 --- a/common/src/main/java/com/zegelin/cassandra/exporter/netty/ssl/SslContextFactory.java +++ b/common/src/main/java/com/zegelin/cassandra/exporter/netty/ssl/SslContextFactory.java @@ -85,7 +85,7 @@ private String getKeyPassword() { } private SslContextBuilder getSelfSignedContextBuilder() { - logger.warn("Running exporter in SSL mode with insecure self-signed certificate"); + logger.warn("Running exporter in SSL mode with insecure self-signed certificate."); try { final SelfSignedCertificate ssc = new SelfSignedCertificate(); diff --git a/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestReloadWatcher.java b/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestReloadWatcher.java index fcba7de..43a6241 100644 --- a/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestReloadWatcher.java +++ b/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestReloadWatcher.java @@ -7,11 +7,14 @@ import java.io.File; import java.io.IOException; import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.FileTime; +import java.time.Instant; import static org.assertj.core.api.Assertions.assertThat; public class TestReloadWatcher { - public static final long INITIAL_FILE_AGE_MILLIS = 5000; + public static final FileTime INITIAL_FILE_MODIFICATION_TIME = FileTime.from(Instant.now().minusSeconds(5)); public static final long SLEEP_MILLIS = 1001; private HttpServerOptions options; @@ -26,16 +29,16 @@ public void before() throws IOException { options.sslServerCertificateFile = givenTemporaryFile("server-cert"); options.sslTrustedCertificateFile = givenTemporaryFile("trusted-cert"); - options.sslServerKeyFile.setLastModified(System.currentTimeMillis() - INITIAL_FILE_AGE_MILLIS); - options.sslServerCertificateFile.setLastModified(System.currentTimeMillis() - INITIAL_FILE_AGE_MILLIS); - options.sslTrustedCertificateFile.setLastModified(System.currentTimeMillis() - INITIAL_FILE_AGE_MILLIS); + Files.setLastModifiedTime(options.sslServerKeyFile.toPath(), INITIAL_FILE_MODIFICATION_TIME); + Files.setLastModifiedTime(options.sslServerCertificateFile.toPath(), INITIAL_FILE_MODIFICATION_TIME); + Files.setLastModifiedTime(options.sslTrustedCertificateFile.toPath(), INITIAL_FILE_MODIFICATION_TIME); watcher = new ReloadWatcher(options); } @Test - public void testNoImmediateReload() { - options.sslServerKeyFile.setLastModified(System.currentTimeMillis()); + public void testNoImmediateReload() throws IOException { + touch(options.sslServerKeyFile); assertThat(watcher.needReload()).isFalse(); } @@ -48,11 +51,11 @@ public void testNoReloadWhenFilesAreUntouched() throws InterruptedException { } @Test - public void testReloadOnceWhenFilesAreTouched() throws InterruptedException { + public void testReloadOnceWhenFilesAreTouched() throws Exception { Thread.sleep(SLEEP_MILLIS); - options.sslServerKeyFile.setLastModified(System.currentTimeMillis()); - options.sslServerCertificateFile.setLastModified(System.currentTimeMillis()); + touch(options.sslServerKeyFile); + touch(options.sslServerCertificateFile); Thread.sleep(SLEEP_MILLIS); @@ -63,15 +66,16 @@ public void testReloadOnceWhenFilesAreTouched() throws InterruptedException { assertThat(watcher.needReload()).isFalse(); } - // Verify that we reload certificates on next pass again in case files are modified + // Verify that we compensate for poor time resolution of Files.getLastModifiedTime(). + // In other words, make sure that we reload certificates on next pass again in case files are modified // just as we check for reload. @Test - public void testReloadAgainWhenFilesAreTouchedJustAfterReload() throws InterruptedException { + public void testReloadAgainWhenFilesAreTouchedJustAfterReload() throws Exception { Thread.sleep(SLEEP_MILLIS); - options.sslServerKeyFile.setLastModified(System.currentTimeMillis()); + touch(options.sslServerKeyFile); assertThat(watcher.needReload()).isTrue(); - options.sslServerCertificateFile.setLastModified(System.currentTimeMillis()); + touch(options.sslServerCertificateFile); Thread.sleep(SLEEP_MILLIS); @@ -88,20 +92,24 @@ public void testReloadWhenForced() throws InterruptedException { } @Test - public void testNoReloadWhenDisabled() throws InterruptedException { + public void testNoReloadWhenDisabled() throws Exception { options.sslReloadIntervalInSeconds = 0; watcher = new ReloadWatcher(options); Thread.sleep(SLEEP_MILLIS); - options.sslServerKeyFile.setLastModified(System.currentTimeMillis()); + touch(options.sslServerKeyFile); assertThat(watcher.needReload()).isFalse(); } private File givenTemporaryFile(String filename) throws IOException { - File file = File.createTempFile(filename, "tmp"); - Files.write(file.toPath(), "dummy".getBytes()); + Path file = Files.createTempFile(filename, "tmp"); + Files.write(file, "dummy".getBytes()); - return file; + return file.toFile(); + } + + private void touch(File file) throws IOException { + Files.setLastModifiedTime(file.toPath(), FileTime.from(Instant.now())); } } diff --git a/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestSslSupport.java b/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestSslSupport.java index 00a2a8e..61a2ede 100644 --- a/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestSslSupport.java +++ b/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestSslSupport.java @@ -15,8 +15,12 @@ import org.testng.annotations.Test; import java.io.File; +import java.io.IOException; import java.net.InetSocketAddress; import java.net.URL; +import java.nio.file.Files; +import java.nio.file.attribute.FileTime; +import java.time.Instant; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; @@ -105,7 +109,7 @@ public void testSslOptionalWithValidation() { @Test - public void testSslReload() throws InterruptedException { + public void testSslReload() throws Exception { serverOptions.sslMode = SslMode.ENABLE; serverOptions.sslServerKeyFile = givenResource("cert/key.pem"); serverOptions.sslServerCertificateFile = givenResource("cert/cert.pem"); @@ -116,7 +120,7 @@ public void testSslReload() throws InterruptedException { SslContext unexpectedContext = sslSupport.getSslContext(); Thread.sleep(1001); - serverOptions.sslServerKeyFile.setLastModified(System.currentTimeMillis()); + touch(serverOptions.sslServerKeyFile); sslSupport.maybeAddHandler(socketChannel); @@ -124,7 +128,7 @@ public void testSslReload() throws InterruptedException { } @Test - public void testSslNoReloadOnFailure() throws InterruptedException { + public void testSslNoReloadOnFailure() throws Exception { serverOptions.sslMode = SslMode.ENABLE; serverOptions.sslServerKeyFile = givenResource("cert/key.pem"); serverOptions.sslServerCertificateFile = givenResource("cert/cert.pem"); @@ -135,7 +139,7 @@ public void testSslNoReloadOnFailure() throws InterruptedException { SslContext expectedContext = sslSupport.getSslContext(); Thread.sleep(1001); - serverOptions.sslServerKeyFile.setLastModified(System.currentTimeMillis()); + touch(serverOptions.sslServerKeyFile); serverOptions.sslServerCertificateFile = null; sslSupport.maybeAddHandler(socketChannel); @@ -144,7 +148,7 @@ public void testSslNoReloadOnFailure() throws InterruptedException { } @Test - public void testSslReloadDisabled() throws InterruptedException { + public void testSslReloadDisabled() throws Exception { serverOptions.sslMode = SslMode.ENABLE; serverOptions.sslServerKeyFile = givenResource("cert/key.pem"); serverOptions.sslServerCertificateFile = givenResource("cert/cert.pem"); @@ -154,7 +158,7 @@ public void testSslReloadDisabled() throws InterruptedException { SslContext expectedContext = sslSupport.getSslContext(); Thread.sleep(1001); - serverOptions.sslServerKeyFile.setLastModified(System.currentTimeMillis()); + touch(serverOptions.sslServerKeyFile); sslSupport.maybeAddHandler(socketChannel); @@ -165,4 +169,8 @@ private File givenResource(String resource) { URL url = this.getClass().getResource("/" + resource); return new File(url.getFile()); } + + private void touch(File file) throws IOException { + Files.setLastModifiedTime(file.toPath(), FileTime.from(Instant.now())); + } }