[GitHub] [camel-quarkus] ppalaga commented on a diff in pull request #3731: Fix #3730 improve paho-mqtt5 ssl tests

[GitBox <gi...@apache.org>]

ppalaga commented on code in PR #3731:
URL: https://github.com/apache/camel-quarkus/pull/3731#discussion_r850526241


##########
integration-tests/paho-mqtt5/src/main/java/org/apache/camel/quarkus/component/paho/mqtt5/it/PahoMqtt5Resource.java:
##########
@@ -68,13 +70,17 @@ public class PahoMqtt5Resource {
     public String consumePahoMessage(
             @PathParam("protocol") String protocol,
             @PathParam("queueName") String queueName) {
+        String tmpKeystore = null;
+
         if ("ssl".equals(protocol)) {
-            setKeyStore(keystore, password);
+            tmpKeystore = setKeyStore(keystore, password);
         }
+
         String result = consumerTemplate.receiveBody("paho-mqtt5:" + queueName + "?brokerUrl=" + brokerUrl(protocol), 5000,
                 String.class);
-        if ("ssl".equals(protocol)) {
-            removeKeyStore(keystore);
+
+        if ("ssl".equals(protocol) && tmpKeystore != null) {
+            removeKeyStore(tmpKeystore);

Review Comment:
   I still wonder why removing the keyStore is important. Could you please explain the motivation? Would other tests' expectations in this Maven module somehow clash with those system props being set permanently? 
   Tests in other modules should not matter because surefire always bootstraps a new JVM for running tests within a module.
   
   If the keystore removal is really important, than we should perhaps enclose receiveBody in a `try` block and call `removeKeyStore()` in `finally` like you do below.



##########
integration-tests/paho-mqtt5/src/main/java/org/apache/camel/quarkus/component/paho/mqtt5/it/PahoMqtt5Resource.java:
##########
@@ -159,18 +167,22 @@ private String brokerUrl(String protocol) {
         return ConfigProvider.getConfig().getValue("paho5.broker." + protocol + ".url", String.class);
     }
 
-    private void setKeyStore(String keystore, String password) {
-        InputStream in = Thread.currentThread().getContextClassLoader().getResourceAsStream(keystore);
+    private String setKeyStore(String keystore, String password) {
+        String tmpKeystore = null;
+
+        try (InputStream in = Thread.currentThread().getContextClassLoader().getResourceAsStream(keystore);) {
+            tmpKeystore = File.createTempFile("keystore-", ".jks").getPath();
+            Files.copy(in, Paths.get(tmpKeystore), StandardCopyOption.REPLACE_EXISTING);
+
+            System.setProperty("javax.net.ssl.keyStore", tmpKeystore);
+            System.setProperty("javax.net.ssl.keyStorePassword", password);
+            System.setProperty("javax.net.ssl.trustStore", tmpKeystore);
+            System.setProperty("javax.net.ssl.trustStorePassword", password);
 
-        try {
-            Files.copy(in, Paths.get(keystore));
         } catch (Exception e) {

Review Comment:
   I think it would be better to fail with a clear message if we cannot copy.
   
   ```suggestion
           } catch (Exception e) {
               throw new RuntimeException("Could not copy "+ keystore +" from the classpath to " + tmpKeystore);
   ```



-- 
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org