diff --git a/core/pom.xml b/core/pom.xml index 8c99423b1b..cecf765007 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -142,5 +142,10 @@ ${guava.version} test + + org.awaitility + awaitility + test + diff --git a/core/src/main/java/io/confluent/rest/ApplicationServer.java b/core/src/main/java/io/confluent/rest/ApplicationServer.java index ede4308dac..490acecd1a 100644 --- a/core/src/main/java/io/confluent/rest/ApplicationServer.java +++ b/core/src/main/java/io/confluent/rest/ApplicationServer.java @@ -54,6 +54,7 @@ public final class ApplicationServer extends Server { private final T config; private final ApplicationGroup applications; private final SslContextFactory sslContextFactory; + private FileWatcher sslKeystoreFileWatcher; private List connectors = new ArrayList<>(); @@ -177,6 +178,9 @@ private void finalizeHandlerCollection(HandlerCollection handlers, HandlerCollec protected void doStop() throws Exception { super.doStop(); applications.doStop(); + if (sslKeystoreFileWatcher != null) { + sslKeystoreFileWatcher.shutdown(); + } } protected final void doStart() throws Exception { @@ -269,7 +273,9 @@ private SslContextFactory createSslContextFactory(RestConfig config) { if (config.getBoolean(RestConfig.SSL_KEYSTORE_RELOAD_CONFIG)) { Path watchLocation = getWatchLocation(config); try { - FileWatcher.onFileChange(watchLocation, () -> { + // create and shutdown a sslKeystoreFileWatcher for each Application, so that + // all Applications in the same JVM don't use the same shared threadpool + sslKeystoreFileWatcher = FileWatcher.onFileChange(watchLocation, () -> { // Need to reset the key store path for symbolic link case sslContextFactory.setKeyStorePath( config.getString(RestConfig.SSL_KEYSTORE_LOCATION_CONFIG) diff --git a/core/src/main/java/io/confluent/rest/FileWatcher.java b/core/src/main/java/io/confluent/rest/FileWatcher.java index c129847a95..e80652402e 100644 --- a/core/src/main/java/io/confluent/rest/FileWatcher.java +++ b/core/src/main/java/io/confluent/rest/FileWatcher.java @@ -35,7 +35,9 @@ // reference https://gist.github.com/danielflower/f54c2fe42d32356301c68860a4ab21ed public class FileWatcher implements Runnable { private static final Logger log = LoggerFactory.getLogger(FileWatcher.class); - private static final ExecutorService executor = Executors.newFixedThreadPool(1, + + // don't have static shared threadpool for all FileWatchers in the JVM + private final ExecutorService executor = Executors.newFixedThreadPool(1, new ThreadFactory() { public Thread newThread(Runnable r) { Thread t = Executors.defaultThreadFactory().newThread(r); @@ -67,10 +69,11 @@ public FileWatcher(Path file, Callback callback) throws IOException { * Starts watching a file calls the callback when it is changed. * A shutdown hook is registered to stop watching. */ - public static void onFileChange(Path file, Callback callback) throws IOException { + public static FileWatcher onFileChange(Path file, Callback callback) throws IOException { log.info("Configure watch file change: " + file); FileWatcher fileWatcher = new FileWatcher(file, callback); - executor.submit(fileWatcher); + fileWatcher.executor.submit(fileWatcher); + return fileWatcher; } public void run() { diff --git a/core/src/test/java/io/confluent/rest/ApiHeadersTest.java b/core/src/test/java/io/confluent/rest/ApiHeadersTest.java index 29ab4e4783..bf24d7345b 100644 --- a/core/src/test/java/io/confluent/rest/ApiHeadersTest.java +++ b/core/src/test/java/io/confluent/rest/ApiHeadersTest.java @@ -45,6 +45,7 @@ import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Test; +import org.junit.rules.TemporaryFolder; public class ApiHeadersTest { @@ -56,11 +57,16 @@ public class ApiHeadersTest { private static String clientKeystoreLocation; private static TestApplication app; + // Use a temporary folder so that .jks files created by this test are isolated + // and deleted when the test is done + public static TemporaryFolder tempFolder = new TemporaryFolder(); + @BeforeClass public static void setUp() throws Exception { - final File trustStore = File.createTempFile("ApiHeadersTest-truststore", ".jks"); - final File clientKeystore = File.createTempFile("ApiHeadersTest-client-keystore", ".jks"); - final File serverKeystore = File.createTempFile("ApiHeadersTest-server-keystore", ".jks"); + tempFolder.create(); + final File trustStore = File.createTempFile("ApiHeadersTest-truststore", ".jks", tempFolder.getRoot()); + final File clientKeystore = File.createTempFile("ApiHeadersTest-client-keystore", ".jks", tempFolder.getRoot()); + final File serverKeystore = File.createTempFile("ApiHeadersTest-server-keystore", ".jks", tempFolder.getRoot()); clientKeystoreLocation = clientKeystore.getAbsolutePath(); @@ -84,6 +90,7 @@ public static void teardown() throws Exception { if (app != null) { app.stop(); } + tempFolder.delete(); } @Test diff --git a/core/src/test/java/io/confluent/rest/SslTest.java b/core/src/test/java/io/confluent/rest/SslTest.java index 97858163c1..64f40bccc6 100644 --- a/core/src/test/java/io/confluent/rest/SslTest.java +++ b/core/src/test/java/io/confluent/rest/SslTest.java @@ -18,6 +18,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; +import java.util.concurrent.TimeUnit; import org.apache.http.client.ClientProtocolException; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpGet; @@ -30,8 +31,10 @@ import org.apache.kafka.common.config.types.Password; import org.apache.kafka.test.TestSslUtils; import org.apache.kafka.test.TestSslUtils.CertificateBuilder; -import org.junit.Before; +import org.junit.AfterClass; +import org.junit.BeforeClass; import org.junit.Test; +import org.junit.rules.TemporaryFolder; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -60,28 +63,49 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; +import static org.awaitility.Awaitility.await; +import static org.junit.Assert.fail; public class SslTest { private static final Logger log = LoggerFactory.getLogger(SslTest.class); - private File trustStore; - private File clientKeystore; - private File serverKeystore; - private File serverKeystoreBak; - private File serverKeystoreErr; + private static File trustStore; + private static File clientKeystore; + private static File serverKeystore; + private static File serverKeystoreBak; + private static File serverKeystoreErr; public static final String SSL_PASSWORD = "test1234"; public static final String EXPECTED_200_MSG = "Response status must be 200."; - public static final int CERT_RELOAD_WAIT_TIME = 30000; - - @Before - public void setUp() throws Exception { + public static final int CERT_RELOAD_WAIT_TIME = 20000; + + private static TemporaryFolder tempFolder; + + @BeforeClass + public static void setUp() throws Exception { + + /* + * To make this test less flakey + * - 1 don't create keystore files for every test method + * - 2 cleanup keystore files on test exist so they don't have to be considerd by the FileWatcher + * - 3 updated the FileWatcher and Application class to not have a single shared threadpool to + * watch for changed files. + * + * By default temp files are not cleaned when up. Which isn't normally a problem unless you are + * testing the ability of rest-utils apps to notice and reload updated ssl keystore files. + * + * Turns out the temp dir that Java+MacOs was continually using on my local machine had 1500 + * files in it. Also the Java "FileWatcher" for Mac works via polling a directory, + * this seems to have added to the flakeyness. + */ + tempFolder = new TemporaryFolder(); + tempFolder.create(); try { - trustStore = File.createTempFile("SslTest-truststore", ".jks"); - clientKeystore = File.createTempFile("SslTest-client-keystore", ".jks"); - serverKeystore = File.createTempFile("SslTest-server-keystore", ".jks"); - serverKeystoreBak = File.createTempFile("SslTest-server-keystore", ".jks.bak"); - serverKeystoreErr = File.createTempFile("SslTest-server-keystore", ".jks.err"); + trustStore = File.createTempFile("SslTest-truststore", ".jks", tempFolder.getRoot()); + clientKeystore = File.createTempFile("SslTest-client-keystore", ".jks", tempFolder.getRoot()); + serverKeystore = File.createTempFile("SslTest-server-keystore", ".jks", tempFolder.getRoot()); + serverKeystoreBak = File.createTempFile("SslTest-server-keystore", ".jks.bak", tempFolder.getRoot()); + serverKeystoreErr = File.createTempFile("SslTest-server-keystore", ".jks.err", tempFolder.getRoot()); } catch (IOException ioe) { throw new RuntimeException("Unable to create temporary files for trust stores and keystores."); } @@ -95,7 +119,12 @@ public void setUp() throws Exception { createWrongKeystoreWithCert(serverKeystoreErr, "server", certs); } - private void createKeystoreWithCert(File file, String alias, Map certs) throws Exception { + @AfterClass + public static void teardown() { + tempFolder.delete(); + } + + private static void createKeystoreWithCert(File file, String alias, Map certs) throws Exception { KeyPair keypair = TestSslUtils.generateKeyPair("RSA"); CertificateBuilder certificateBuilder = new CertificateBuilder(30, "SHA1withRSA"); X509Certificate cCert = certificateBuilder.sanDnsName("localhost") @@ -128,7 +157,7 @@ private void enableSslClientAuth(Properties props) { props.put(RestConfig.SSL_CLIENT_AUTH_CONFIG, true); } - private void createWrongKeystoreWithCert(File file, String alias, Map certs) throws Exception { + private static void createWrongKeystoreWithCert(File file, String alias, Map certs) throws Exception { KeyPair keypair = TestSslUtils.generateKeyPair("RSA"); CertificateBuilder certificateBuilder = new CertificateBuilder(30, "SHA1withRSA"); X509Certificate cCert = certificateBuilder.sanDnsName("fail") @@ -176,30 +205,44 @@ public void testHttpsWithAutoReload() throws Exception { SslTestApplication app = new SslTestApplication(config); try { app.start(); - int statusCode = makeGetRequest(httpsUri + "/test", + int startingCode = makeGetRequest(httpsUri + "/test", clientKeystore.getAbsolutePath(), SSL_PASSWORD, SSL_PASSWORD); - assertEquals(EXPECTED_200_MSG, 200, statusCode); + assertEquals(EXPECTED_200_MSG, 200, startingCode); assertMetricsCollected(); // verify reload -- override the server keystore with a wrong one Files.copy(serverKeystoreErr.toPath(), serverKeystore.toPath(), StandardCopyOption.REPLACE_EXISTING); - Thread.sleep(CERT_RELOAD_WAIT_TIME); - boolean hitError = false; - try { - makeGetRequest(httpsUri + "/test", - clientKeystore.getAbsolutePath(), SSL_PASSWORD, SSL_PASSWORD); - } catch (Exception e) { - System.out.println(e); - hitError = true; - } + log.info("\tKeystore reload test : Applied bad keystore file"); + + await().pollInterval(2, TimeUnit.SECONDS).atMost(30, TimeUnit.SECONDS).untilAsserted( () -> { + boolean hitError = false; + try { + log.info("\tKeystore reload test : Awaiting failed https connection"); + makeGetRequest(httpsUri + "/test", clientKeystore.getAbsolutePath(), SSL_PASSWORD, SSL_PASSWORD); + } catch (Exception e) { + System.out.println(e); + hitError = true; + } + assertTrue("Expecting to hit an error with new server cert", hitError); + }); // verify reload -- override the server keystore with a correct one Files.copy(serverKeystoreBak.toPath(), serverKeystore.toPath(), StandardCopyOption.REPLACE_EXISTING); - Thread.sleep(CERT_RELOAD_WAIT_TIME); - statusCode = makeGetRequest(httpsUri + "/test", - clientKeystore.getAbsolutePath(), SSL_PASSWORD, SSL_PASSWORD); - assertEquals(EXPECTED_200_MSG, 200, statusCode); - assertEquals("expect hit error with new server cert", true, hitError); + log.info("\tKeystore reload test : keystore set back to good value"); + + await().pollInterval(2, TimeUnit.SECONDS).atMost(30, TimeUnit.SECONDS).untilAsserted( () -> { + try { + log.info("\tKeystore reload test : Awaiting a valid https connection"); + int statusCode = makeGetRequest(httpsUri + "/test", clientKeystore.getAbsolutePath(), SSL_PASSWORD, SSL_PASSWORD); + assertEquals(EXPECTED_200_MSG, 200, statusCode); + log.info("\tKeystore reload test : Valid connection found"); + } + catch (Exception e) { + fail(); + // we have to wait for the good key to take affect + } + }); + } finally { if (app != null) { app.stop(); @@ -332,7 +375,7 @@ public void testHttpsWithAuthAndBadClientCert() throws Exception { app.start(); // create a new client cert that isn't in the server's trust store. - File untrustedClient = File.createTempFile("SslTest-client-keystore", ".jks"); + File untrustedClient = File.createTempFile("SslTest-client-keystore", ".jks", tempFolder.getRoot()); Map certs = new HashMap<>(); createKeystoreWithCert(untrustedClient, "client", certs); try { diff --git a/pom.xml b/pom.xml index b41764f03f..60cb18e37d 100644 --- a/pom.xml +++ b/pom.xml @@ -55,6 +55,7 @@ 2.2.0 24.0-jre checkstyle/suppressions.xml + 4.0.3 @@ -182,6 +183,12 @@ jersey-test-framework-provider-jetty ${jersey.version} + + + org.awaitility + awaitility + ${awaitility.version} +