You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by CMoH <gi...@git.apache.org> on 2015/11/02 13:41:39 UTC
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
GitHub user CMoH opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/997
[BROOKLYN-190] Move from Jetty8 to Jetty9
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/CMoH/incubator-brooklyn jetty9
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/997.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #997
----
commit a30a4da45258f85f0ab3375ba0c857cc4096b61a
Author: Ciprian Ciubotariu <ch...@gmx.net>
Date: 2015-10-30T21:10:55Z
[BROOKLYN-190] Upgrade to Jetty9
Move from jetty-8.1.17.v20150415 to 9.2.13.v20150730
commit b7129ebb54edc9077fc11d7062eca29595d04c06
Author: Ciprian Ciubotariu <ch...@gmx.net>
Date: 2015-11-02T12:39:24Z
[BROOKLYN-190] Fix failing SSL test
With Jetty9 the SSL connector throws java.net.SocketException:
Connection reset instead of what the test expected.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#issuecomment-155044582
@neykov I think the above fix addresses your comment. I wanted to merge this during the weekend but I thought I'd wait for your feedback.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43650051
--- Diff: usage/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynWebServerTest.java ---
@@ -146,11 +146,13 @@ public void verifyHttpsFromConfig() throws Exception {
@Test
public void verifyHttpsCiphers() throws Exception {
brooklynProperties.put(BrooklynWebConfig.HTTPS_REQUIRED, true);
- brooklynProperties.put(BrooklynWebConfig.TRANSPORT_PROTOCOLS, "XXX");
- brooklynProperties.put(BrooklynWebConfig.TRANSPORT_CIPHERS, "XXX");
--- End diff --
But why remove the properties? The test is meant to confirm that the properties are actually used to configure the web server.
Having this pass now suggests that the test is not working as expected though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43863597
--- Diff: parent/pom.xml ---
@@ -208,9 +208,9 @@
<version>${swagger.version}</version>
</dependency>
<dependency>
- <groupId>org.eclipse.jetty.orbit</groupId>
- <artifactId>javax.servlet</artifactId>
- <version>${jetty-orbit-javax-servlet.version}</version>
+ <groupId>javax.servlet</groupId>
+ <artifactId>javax.servlet-api</artifactId>
+ <version>${javax-servlet.version}</version>
--- End diff --
* If Brooklyn is using classes from the package then the convention is to explicitly declare it as a dependency in the corresponding module(s)
* If it's only a transitive dependency then its version shouldn't be fixed, unless there's a version conflict as this might lead to unexpected behaviour in future where another version is expected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#issuecomment-155087512
Here is the exception on the client side
```
2015-11-09 16:59:07,318 INFO http client exception
org.apache.brooklyn.util.exceptions.PropagatedRuntimeException:
at org.apache.brooklyn.util.exceptions.Exceptions.propagate(Exceptions.java:103) ~[brooklyn-utils-common-0.9.0-SNAPSHOT.jar:0.9.0-SNAPSHOT]
Caused by: java.net.SocketException: Connection reset
at java.net.SocketInputStream.read(SocketInputStream.java:196) ~[na:1.7.0_80]
```
Why should PR should be squashed? The commits do reflect the amount of work involved in creating the feature, and during review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43995953
--- Diff: usage/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynWebServerTest.java ---
@@ -146,11 +146,13 @@ public void verifyHttpsFromConfig() throws Exception {
@Test
public void verifyHttpsCiphers() throws Exception {
brooklynProperties.put(BrooklynWebConfig.HTTPS_REQUIRED, true);
- brooklynProperties.put(BrooklynWebConfig.TRANSPORT_PROTOCOLS, "XXX");
- brooklynProperties.put(BrooklynWebConfig.TRANSPORT_CIPHERS, "XXX");
--- End diff --
I adjusted the test to match your request above.
However, since the SSL connectors are different between jetty8 and jetty9, the server may choose to abort the connection at different stages. From the results of this test I presume that jetty9 disconnects before starting the SSL handshake, so the client gets a SocketException, while jetty8 delays reading those properties until later on, and therefore the client receives an SSL exception.
For that reason I believe the client exception is dependent of the server version.
The jetty9 server log for this test supports my above reasoning:
```
2015-11-05 11:57:29,631 WARN Exception while notifying connection SslConnection@15c886f3{NEED_WRAP,eio=-1/-1,di=-1} -> HttpConnection@112995a5{IDLE}
org.eclipse.jetty.io.RuntimeIOException: javax.net.ssl.SSLHandshakeException: No appropriate protocol (protocol is disabled or cipher suites are inappropriate)
at org.eclipse.jetty.io.ssl.SslConnection.onOpen(SslConnection.java:150) ~[jetty-io-9.2.13.v20150730.jar:9.2.13.v20150730]
Caused by: javax.net.ssl.SSLHandshakeException: No appropriate protocol (protocol is disabled or cipher suites are inappropriate)
at sun.security.ssl.Handshaker.activate(Handshaker.java:470) ~[na:1.7.0_80]
2015-11-05 11:57:29,635 WARN Exception while notifying connection SslConnection@70d9f5a6{NEED_WRAP,eio=-1/-1,di=-1} -> HttpConnection@758e9f2b{IDLE}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43996036
--- Diff: parent/pom.xml ---
@@ -208,9 +208,9 @@
<version>${swagger.version}</version>
</dependency>
<dependency>
- <groupId>org.eclipse.jetty.orbit</groupId>
- <artifactId>javax.servlet</artifactId>
- <version>${jetty-orbit-javax-servlet.version}</version>
+ <groupId>javax.servlet</groupId>
+ <artifactId>javax.servlet-api</artifactId>
+ <version>${javax-servlet.version}</version>
--- End diff --
I re-added the explicit dependency towards javax.servlet where they used to be.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/997
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43771192
--- Diff: parent/pom.xml ---
@@ -208,9 +208,9 @@
<version>${swagger.version}</version>
</dependency>
<dependency>
- <groupId>org.eclipse.jetty.orbit</groupId>
- <artifactId>javax.servlet</artifactId>
- <version>${jetty-orbit-javax-servlet.version}</version>
+ <groupId>javax.servlet</groupId>
+ <artifactId>javax.servlet-api</artifactId>
+ <version>${javax-servlet.version}</version>
--- End diff --
That was exactly my intention. Why would I add a transitive dependency explicitly, when it is added implicitly by maven?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43634842
--- Diff: camp/camp-server/src/main/java/org/apache/brooklyn/camp/server/rest/CampServer.java ---
@@ -118,7 +120,12 @@ public synchronized void stop() {
public Integer getPort() {
if (server==null) return null;
- return server.getConnectors()[0].getLocalPort();
+ try {
+ NetworkConnector networkConnector = (NetworkConnector) server.getConnectors()[0];
+ return networkConnector.getLocalPort();
+ } catch (ClassCastException ex) {
+ return null;
--- End diff --
Don't get this, why explicitly catch this exception? Lots of other places where the same cast is done, so no point really.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43632335
--- Diff: core/src/test/java/org/apache/brooklyn/core/test/HttpService.java ---
@@ -128,15 +131,21 @@ public HttpService start() throws Exception {
} finally {
keyStoreStream.close();
}
-
+
+ // manually create like seen in XMLs at http://www.eclipse.org/jetty/documentation/current/configuring-ssl.html
SslContextFactory sslContextFactory = new SslContextFactory();
sslContextFactory.setKeyStore(keyStore);
sslContextFactory.setTrustAll(true);
sslContextFactory.setKeyStorePassword("password");
- SslSocketConnector sslSocketConnector = new SslSocketConnector(sslContextFactory);
- sslSocketConnector.setPort(actualPort);
- server.addConnector(sslSocketConnector);
+ HttpConfiguration sslHttpConfig = new HttpConfiguration();
+ sslHttpConfig.setSecureScheme("https");
+ sslHttpConfig.setSecurePort(actualPort);
+
+ ServerConnector httpsConnector = new ServerConnector(server, new SslConnectionFactory(sslContextFactory, "http/1.1"), new HttpConnectionFactory(sslHttpConfig));
--- End diff --
Can use `HttpVersion.HTTP_1_1.asString()` instead.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#issuecomment-154560691
It seems that we are both right. By adding an `invocationCount` parameter to the test under question I've found that "sometimes" we get `SocketException`, and sometimes `SSLHandshakeException`. I've tracked the problem down to a (theoretical) flaw in the way `org.apache.http.conn.ssl.SSLSocketFactory` handles SSL socket connections. However, it may be that this race is visible only when both ends of the socket are on the same host, or (as in our test case) within the same process.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#issuecomment-155101445
Agree that they reflect the process of creating the end result, but don't bring value to the logical history of the repository. The commits that go in master should reflect the changes, split into logical and self contained parts - no point in having commits that overwrite each other. See the [guide](https://brooklyn.incubator.apache.org/developers/how-to-contribute.html#reviews) for more details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#issuecomment-155131139
I've squashed the entire thing in a single commit. Please review and merge, if possible.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#issuecomment-155259008
Looks like all comments were addressed. Tests pass. Merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43634878
--- Diff: parent/pom.xml ---
@@ -208,9 +208,9 @@
<version>${swagger.version}</version>
</dependency>
<dependency>
- <groupId>org.eclipse.jetty.orbit</groupId>
- <artifactId>javax.servlet</artifactId>
- <version>${jetty-orbit-javax-servlet.version}</version>
+ <groupId>javax.servlet</groupId>
+ <artifactId>javax.servlet-api</artifactId>
+ <version>${javax-servlet.version}</version>
--- End diff --
Since this is no longer used in `launcher` can remove it altogether.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#issuecomment-155051017
Thanks @hzbarcea, @CMoH. The test works for me after the changes. Good to merge after squashing.
@CMoH could you share your stacktrace so we have both alternatives in the PR for future reference. Including mine again because its buried in the addressed comments.
```
org.apache.brooklyn.util.exceptions.PropagatedRuntimeException: javax.net.ssl.SSLHandshakeException: Remote host closed connection during handshake
at org.apache.brooklyn.util.exceptions.Exceptions.propagate(Exceptions.java:103)
at org.apache.brooklyn.util.core.http.HttpTool.execAndConsume(HttpTool.java:364)
at org.apache.brooklyn.launcher.BrooklynWebServerTest.verifyHttpsFromConfig(BrooklynWebServerTest.java:171)
at org.apache.brooklyn.launcher.BrooklynWebServerTest.verifyHttpsCiphers(BrooklynWebServerTest.java:153)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:84)
at org.testng.internal.Invoker.invokeMethod(Invoker.java:714)
at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:901)
at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1231)
at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:127)
at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:111)
at org.testng.TestRunner.privateRun(TestRunner.java:767)
at org.testng.TestRunner.run(TestRunner.java:617)
at org.testng.SuiteRunner.runTest(SuiteRunner.java:348)
at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:343)
at org.testng.SuiteRunner.privateRun(SuiteRunner.java:305)
at org.testng.SuiteRunner.run(SuiteRunner.java:254)
at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
at org.testng.TestNG.runSuitesSequentially(TestNG.java:1224)
at org.testng.TestNG.runSuitesLocally(TestNG.java:1149)
at org.testng.TestNG.run(TestNG.java:1057)
at org.testng.remote.RemoteTestNG.run(RemoteTestNG.java:115)
at org.testng.remote.RemoteTestNG.initAndRun(RemoteTestNG.java:207)
at org.testng.remote.RemoteTestNG.main(RemoteTestNG.java:178)
Caused by: javax.net.ssl.SSLHandshakeException: Remote host closed connection during handshake
at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:953)
at sun.security.ssl.SSLSocketImpl.performInitialHandshake(SSLSocketImpl.java:1332)
at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:1359)
at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:1343)
at org.apache.http.conn.ssl.SSLSocketFactory.connectSocket(SSLSocketFactory.java:543)
at org.apache.http.conn.ssl.SSLSocketFactory.connectSocket(SSLSocketFactory.java:409)
at org.apache.http.impl.conn.DefaultClientConnectionOperator.openConnection(DefaultClientConnectionOperator.java:177)
at org.apache.http.impl.conn.ManagedClientConnectionImpl.open(ManagedClientConnectionImpl.java:304)
at org.apache.http.impl.client.DefaultRequestDirector.tryConnect(DefaultRequestDirector.java:611)
at org.apache.http.impl.client.DefaultRequestDirector.execute(DefaultRequestDirector.java:446)
at org.apache.http.impl.client.AbstractHttpClient.doExecute(AbstractHttpClient.java:882)
at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:82)
at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:107)
at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:55)
at org.apache.brooklyn.util.core.http.HttpTool.execAndConsume(HttpTool.java:356)
... 26 more
Caused by: java.io.EOFException: SSL peer shut down incorrectly
at sun.security.ssl.InputRecord.read(InputRecord.java:482)
at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:934)
... 40 more
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r44165746
--- Diff: usage/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynWebServerTest.java ---
@@ -150,10 +151,9 @@ public void verifyHttpsCiphers() throws Exception {
brooklynProperties.put(BrooklynWebConfig.TRANSPORT_CIPHERS, "XXX");
try {
verifyHttpsFromConfig(brooklynProperties);
- fail("Expected to fail due to unsupported ciphers during connection negotiation");
+ fail("Expected to fail due to to unsupported ciphers during connection negotiation");
--- End diff --
@neykov tests pass for me too. Is your stack trace from the client or server side?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43769972
--- Diff: usage/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynWebServerTest.java ---
@@ -146,11 +146,13 @@ public void verifyHttpsFromConfig() throws Exception {
@Test
public void verifyHttpsCiphers() throws Exception {
brooklynProperties.put(BrooklynWebConfig.HTTPS_REQUIRED, true);
- brooklynProperties.put(BrooklynWebConfig.TRANSPORT_PROTOCOLS, "XXX");
- brooklynProperties.put(BrooklynWebConfig.TRANSPORT_CIPHERS, "XXX");
--- End diff --
I must confess I did not understand the intention of this test. Jetty9 is throwing `java.net.SocketException`. Shall we add that to the list? Or just replace the list with this exception?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43637138
--- Diff: usage/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynWebServerTest.java ---
@@ -146,11 +146,13 @@ public void verifyHttpsFromConfig() throws Exception {
@Test
public void verifyHttpsCiphers() throws Exception {
brooklynProperties.put(BrooklynWebConfig.HTTPS_REQUIRED, true);
- brooklynProperties.put(BrooklynWebConfig.TRANSPORT_PROTOCOLS, "XXX");
- brooklynProperties.put(BrooklynWebConfig.TRANSPORT_CIPHERS, "XXX");
--- End diff --
These properties override the default protocols, certificates so that the handshake fails. Can't figure out why the test succeeds now - what's the reason for the exception?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r44043852
--- Diff: usage/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynWebServerTest.java ---
@@ -150,10 +151,9 @@ public void verifyHttpsCiphers() throws Exception {
brooklynProperties.put(BrooklynWebConfig.TRANSPORT_CIPHERS, "XXX");
try {
verifyHttpsFromConfig(brooklynProperties);
- fail("Expected to fail due to unsupported ciphers during connection negotiation");
+ fail("Expected to fail due to to unsupported ciphers during connection negotiation");
--- End diff --
An extra `to`.
@CMoH, checked out the branch and built locally - the test is failing for me, no `SocketException` in the stack trace. Here's my stack trace.
```
2015-11-05 19:45:30,286 WARN Exception while notifying connection SslConnection@7df01c0f{NEED_WRAP,eio=-1/-1,di=-1} -> HttpConnection@42dad75{IDLE}
org.eclipse.jetty.io.RuntimeIOException: javax.net.ssl.SSLHandshakeException: No appropriate protocol (protocol is disabled or cipher suites are inappropriate)
at org.eclipse.jetty.io.ssl.SslConnection.onOpen(SslConnection.java:150) ~[jetty-io-9.2.13.v20150730.jar:9.2.13.v20150730]
Caused by: javax.net.ssl.SSLHandshakeException: No appropriate protocol (protocol is disabled or cipher suites are inappropriate)
at sun.security.ssl.Handshaker.activate(Handshaker.java:470) ~[na:1.7.0_80]
FAILED: verifyHttpsCiphers
org.apache.brooklyn.util.exceptions.PropagatedRuntimeException: javax.net.ssl.SSLHandshakeException: Remote host closed connection during handshake
at org.apache.brooklyn.util.exceptions.Exceptions.propagate(Exceptions.java:103)
at org.apache.brooklyn.util.core.http.HttpTool.execAndConsume(HttpTool.java:364)
at org.apache.brooklyn.launcher.BrooklynWebServerTest.verifyHttpsFromConfig(BrooklynWebServerTest.java:171)
at org.apache.brooklyn.launcher.BrooklynWebServerTest.verifyHttpsCiphers(BrooklynWebServerTest.java:153)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:84)
at org.testng.internal.Invoker.invokeMethod(Invoker.java:714)
at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:901)
at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1231)
at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:127)
at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:111)
at org.testng.TestRunner.privateRun(TestRunner.java:767)
at org.testng.TestRunner.run(TestRunner.java:617)
at org.testng.SuiteRunner.runTest(SuiteRunner.java:348)
at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:343)
at org.testng.SuiteRunner.privateRun(SuiteRunner.java:305)
at org.testng.SuiteRunner.run(SuiteRunner.java:254)
at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
at org.testng.TestNG.runSuitesSequentially(TestNG.java:1224)
at org.testng.TestNG.runSuitesLocally(TestNG.java:1149)
at org.testng.TestNG.run(TestNG.java:1057)
at org.testng.remote.RemoteTestNG.run(RemoteTestNG.java:115)
at org.testng.remote.RemoteTestNG.initAndRun(RemoteTestNG.java:207)
at org.testng.remote.RemoteTestNG.main(RemoteTestNG.java:178)
Caused by: javax.net.ssl.SSLHandshakeException: Remote host closed connection during handshake
at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:953)
at sun.security.ssl.SSLSocketImpl.performInitialHandshake(SSLSocketImpl.java:1332)
at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:1359)
at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:1343)
at org.apache.http.conn.ssl.SSLSocketFactory.connectSocket(SSLSocketFactory.java:543)
at org.apache.http.conn.ssl.SSLSocketFactory.connectSocket(SSLSocketFactory.java:409)
at org.apache.http.impl.conn.DefaultClientConnectionOperator.openConnection(DefaultClientConnectionOperator.java:177)
at org.apache.http.impl.conn.ManagedClientConnectionImpl.open(ManagedClientConnectionImpl.java:304)
at org.apache.http.impl.client.DefaultRequestDirector.tryConnect(DefaultRequestDirector.java:611)
at org.apache.http.impl.client.DefaultRequestDirector.execute(DefaultRequestDirector.java:446)
at org.apache.http.impl.client.AbstractHttpClient.doExecute(AbstractHttpClient.java:882)
at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:82)
at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:107)
at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:55)
at org.apache.brooklyn.util.core.http.HttpTool.execAndConsume(HttpTool.java:356)
... 26 more
Caused by: java.io.EOFException: SSL peer shut down incorrectly
at sun.security.ssl.InputRecord.read(InputRecord.java:482)
at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:934)
... 40 more
```
The original test is working fine (though I suspect the exception list is too generous - will address in a separate PR).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#issuecomment-155359737
Thanks @CMoH, @hzbarcea, looks great, thanks for merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43645408
--- Diff: parent/pom.xml ---
@@ -208,9 +208,9 @@
<version>${swagger.version}</version>
</dependency>
<dependency>
- <groupId>org.eclipse.jetty.orbit</groupId>
- <artifactId>javax.servlet</artifactId>
- <version>${jetty-orbit-javax-servlet.version}</version>
+ <groupId>javax.servlet</groupId>
+ <artifactId>javax.servlet-api</artifactId>
+ <version>${javax-servlet.version}</version>
--- End diff --
`javax.servlet` classes are still used throughout the code, such as in `BrooklynPropertiesSecurityFilter` - was this what you were referring to?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43867603
--- Diff: usage/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynWebServerTest.java ---
@@ -146,11 +146,13 @@ public void verifyHttpsFromConfig() throws Exception {
@Test
public void verifyHttpsCiphers() throws Exception {
brooklynProperties.put(BrooklynWebConfig.HTTPS_REQUIRED, true);
- brooklynProperties.put(BrooklynWebConfig.TRANSPORT_PROTOCOLS, "XXX");
- brooklynProperties.put(BrooklynWebConfig.TRANSPORT_CIPHERS, "XXX");
--- End diff --
The point of the test is to check whether the server actually uses the configured ciphers, protocols. So non-existing ciphers are configured which should cause the handshake to fail.
The exception is coming from the client, connecting to jetty. It should be independent of the jetty version as this is at the ssl connection level which is handled by the JVM.
On my system the root cause is `Caused by: java.io.EOFException: SSL peer shut down incorrectly` because the server can't match the ciphers offered by the client and immediately closes the connection.
So keep the properties and if needed adjust the test to assert the correct exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#issuecomment-154153034
The build succeeds on my machine - I'll investigate some more, but I wonder what might be the difference?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43632393
--- Diff: pom.xml ---
@@ -119,7 +119,8 @@
<sshj.version>0.8.1</sshj.version>
<felix.framework.version>4.4.0</felix.framework.version>
<reflections.version>0.9.9-RC1</reflections.version>
- <jetty.version>8.1.17.v20150415</jetty.version>
+ <!-- jetty.version>8.1.17.v20150415</jetty.version -->
--- End diff --
Remove comment
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43649604
--- Diff: parent/pom.xml ---
@@ -208,9 +208,9 @@
<version>${swagger.version}</version>
</dependency>
<dependency>
- <groupId>org.eclipse.jetty.orbit</groupId>
- <artifactId>javax.servlet</artifactId>
- <version>${jetty-orbit-javax-servlet.version}</version>
+ <groupId>javax.servlet</groupId>
+ <artifactId>javax.servlet-api</artifactId>
+ <version>${javax-servlet.version}</version>
--- End diff --
You left the dependency only in the `dependencyManagement` section in the parent pom, which doesn't actually pull it as a dependency, just declares which version to use in case anyone needs it. If classes from it are still directly referenced, then put it explicitly in the `dependencies` sections in modules that need it. It works now because it's a transitive dependency of jetty.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#issuecomment-153047956
Looks good, minor comments only. Couldn't figure out how does the test verify that the configured ciphers are actually used - what does the test do now? Perhaps it wasn't correct since the beginning and was testing different behaviour?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43634926
--- Diff: usage/launcher/src/main/java/org/apache/brooklyn/launcher/BrooklynWebServer.java ---
@@ -379,13 +381,25 @@ public synchronized void start() throws Exception {
throw new IllegalStateException("Unable to provision port for web console (wanted "+portRange+")");
}
- server = new Server();
- final Connector connector;
+
+ // use a nice name in the thread pool (otherwise this is exactly the same as Server defaults)
+ QueuedThreadPool threadPool = new QueuedThreadPool();
+ threadPool.setName("brooklyn-jetty-server-"+actualPort+"-"+threadPool.getName());
+
+ server = new Server(threadPool);
+ final ServerConnector connector;
+
if (getHttpsEnabled()) {
- connector = new SslSelectChannelConnector(createContextFactory());
+ HttpConfiguration sslHttpConfig = new HttpConfiguration();
+ sslHttpConfig.setSecureScheme("https");
+ sslHttpConfig.setSecurePort(actualPort);
+
+ SslContextFactory sslContextFactory = createContextFactory();
+ connector = new ServerConnector(server, new SslConnectionFactory(sslContextFactory, "http/1.1"), new HttpConnectionFactory(sslHttpConfig));
--- End diff --
Use HttpVersion.HTTP_1_1.asString().
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-190] Move from Jetty8 t...
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/997#discussion_r43645871
--- Diff: usage/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynWebServerTest.java ---
@@ -146,11 +146,13 @@ public void verifyHttpsFromConfig() throws Exception {
@Test
public void verifyHttpsCiphers() throws Exception {
brooklynProperties.put(BrooklynWebConfig.HTTPS_REQUIRED, true);
- brooklynProperties.put(BrooklynWebConfig.TRANSPORT_PROTOCOLS, "XXX");
- brooklynProperties.put(BrooklynWebConfig.TRANSPORT_CIPHERS, "XXX");
--- End diff --
I added the expected exception in the comments within the code: `javax.net.ssl.SSLHandshakeException` due to failure to verify the server certificate.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---