You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2022/04/14 13:15:44 UTC
[GitHub] [camel-quarkus] zhfeng opened a new pull request, #3731: Fix #3730 improve paho-mqtt5 ssl tests
zhfeng opened a new pull request, #3731:
URL: https://github.com/apache/camel-quarkus/pull/3731
<!-- Uncomment and fill this section if your PR is not trivial
[ ] An issue should be filed for the change unless this is a trivial change (fixing a typo or similar). One issue should ideally be fixed by not more than one commit and the other way round, each commit should fix just one issue, without pulling in other changes.
[ ] Each commit in the pull request should have a meaningful and properly spelled subject line and body. Copying the title of the associated issue is typically enough. Please include the issue number in the commit message prefixed by #.
[ ] The pull request description should explain what the pull request does, how, and why. If the info is available in the associated issue or some other external document, a link is enough.
[ ] Phrases like Fix #<issueNumber> or Fixes #<issueNumber> will auto-close the named issue upon merging the pull request. Using them is typically a good idea.
[ ] Please run mvn process-resources -Pformat (and amend the changes if necessary) before sending the pull request.
[ ] Contributor guide is your good friend: https://camel.apache.org/camel-quarkus/latest/contributor-guide.html
-->
--
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
[GitHub] [camel-quarkus] ppalaga merged pull request #3731: Fix #3730 improve paho-mqtt5 ssl tests
Posted by GitBox <gi...@apache.org>.
ppalaga merged PR #3731:
URL: https://github.com/apache/camel-quarkus/pull/3731
--
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
[GitHub] [camel-quarkus] ppalaga commented on a diff in pull request #3731: Fix #3730 improve paho-mqtt5 ssl tests
Posted by 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
[GitHub] [camel-quarkus] ppalaga commented on a diff in pull request #3731: Fix #3730 improve paho-mqtt5 ssl tests
Posted by GitBox <gi...@apache.org>.
ppalaga commented on code in PR #3731:
URL: https://github.com/apache/camel-quarkus/pull/3731#discussion_r850788495
##########
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:
Sorry, should be `throw new RuntimeException("Could not copy "+ keystore +" from the classpath to " + tmpKeystore, e);` - we do not want to swallow the exception.
--
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
[GitHub] [camel-quarkus] ppalaga commented on a diff in pull request #3731: Fix #3730 improve paho-mqtt5 ssl tests
Posted by GitBox <gi...@apache.org>.
ppalaga commented on code in PR #3731:
URL: https://github.com/apache/camel-quarkus/pull/3731#discussion_r850714484
##########
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:
My understanding of reuseForks is that it controls whether JVMs can be reused for individual test classes within the same Maven module.
To make sure, I have added
```
System.out.println("=== pid(" + getClass().getSimpleName() + ") " + ManagementFactory.getRuntimeMXBean().getName());
```
to `Base64Test` and `BeanValidatorTest` and I have run
```
cd integration-tests && mvn clean test -pl :camel-quarkus-integration-test-base64,:camel-quarkus-integration-test-bean-validator
...
=== pid(Base64Test) 111907@terpistone
...
=== pid(BeanValidatorTest) 112141@terpistone
```
So the two tests from two different Maven modules were run in two different JVMs.
> it is reasonable to clean the environment properties after testing.
+1 on generally wanting to cleanup after the test. The questions in this particular case are:
1. Whether the cleanup does what it is supposed to do
2. Whether there might be any unwanted side effects.
3. Whether this is a good practice to be followed by end users
For 1., it depends how we define our intention. Removal of the system props themselves is effective for sure, but I doubt that by doing that we also remove the SSL context instantiated somewhere deep in Paho. Should that be our goal? I don't know, probably not. But if we do not remove the SSL context whose state relies on the props, should we remove the props (and the key store file) at all? Maybe better not before the the test app is shut down?
For 2., `javax.net.ssl.*` properties are not owned by us. They are owned by the JVM. There may exist third party code sensitive to the changes. My impression from what I have read about `javax.net.ssl.*` in the past is, that they are intended to be passed to a JVM at startup and that they are not so much intended to be updated at runtime. Will we break any third party code relying on `javax.net.ssl.*` properties? Probably not (otherwise we'd see failures in the test).
For 3., I'd say our end users should not be setting `javax.net.ssl.*` properties at runtime for reasons named in 2. Removing the key store file and the props may break something in their app. If we keep the code in the test, I think we should at least add a comment saying something like "This may break code relying on `javax.net.ssl.*` properties. Do not do this in production.".
Maybe passing the props from a TestResource to the app when it is starting and removing the key store file after the app is shut down from `QuarkusTestResourceLifecycleManager.stop()` would address all these questions?
--
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
[GitHub] [camel-quarkus] zhfeng commented on a diff in pull request #3731: Fix #3730 improve paho-mqtt5 ssl tests
Posted by GitBox <gi...@apache.org>.
zhfeng commented on code in PR #3731:
URL: https://github.com/apache/camel-quarkus/pull/3731#discussion_r850581679
##########
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 wll add this message.
--
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
[GitHub] [camel-quarkus] zhfeng commented on a diff in pull request #3731: Fix #3730 improve paho-mqtt5 ssl tests
Posted by GitBox <gi...@apache.org>.
zhfeng commented on code in PR #3731:
URL: https://github.com/apache/camel-quarkus/pull/3731#discussion_r850953873
##########
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:
Thanks a lot @ppalaga! I revisit the [paho-mqtt5-component.sslClientProps](https://camel.apache.org/components/3.14.x/paho-mqtt5-component.html#_component_option_sslClientProps) and it seems that we can pass them with ```com.ibm.ssl.keyStore``` and ```com.ibm.ssl.keyStorePassword``` in uri. It should be better to avoid setting ```javax.net.ssl.*``` system properties. I will try it at first.
--
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
[GitHub] [camel-quarkus] zhfeng commented on a diff in pull request #3731: Fix #3730 improve paho-mqtt5 ssl tests
Posted by GitBox <gi...@apache.org>.
zhfeng commented on code in PR #3731:
URL: https://github.com/apache/camel-quarkus/pull/3731#discussion_r850983380
##########
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:
yeah, it works.
--
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
[GitHub] [camel-quarkus] zhfeng commented on a diff in pull request #3731: Fix #3730 improve paho-mqtt5 ssl tests
Posted by GitBox <gi...@apache.org>.
zhfeng commented on code in PR #3731:
URL: https://github.com/apache/camel-quarkus/pull/3731#discussion_r850953873
##########
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:
Thanks a lot @ppalaga! I revisit the [paho-mqtt5-component.sslClientProps](https://camel.apache.org/components/3.14.x/paho-mqtt5-component.html#_component_option_sslClientProps) and it seems that we can pass with them ```com.ibm.ssl.keyStore``` and ```com.ibm.ssl.keyPassword``` in uri. It should be better to avoid setting ```javax.net.ssl.*``` system properties. I will try it at first.
--
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
[GitHub] [camel-quarkus] zhfeng commented on a diff in pull request #3731: Fix #3730 improve paho-mqtt5 ssl tests
Posted by GitBox <gi...@apache.org>.
zhfeng commented on code in PR #3731:
URL: https://github.com/apache/camel-quarkus/pull/3731#discussion_r850580608
##########
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:
Well, @jamesnetherton suggest to unset the keystore properties in https://github.com/apache/camel-quarkus/pull/3709#discussion_r845767127 and I think it is reasonable to clean the environment properties after testing. I'm not very sure that surefire always lauchs a new JVM for each test ? e.g. with ```reuseForks=true```
--
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
[GitHub] [camel-quarkus] aldettinger commented on pull request #3731: Fix #3730 improve paho-mqtt5 ssl tests
Posted by GitBox <gi...@apache.org>.
aldettinger commented on PR #3731:
URL: https://github.com/apache/camel-quarkus/pull/3731#issuecomment-1102479089
Great to see this improvement. Nice jobs @ppalaga and @zhfeng :+1:
--
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