You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jinmei Liao <ji...@pivotal.io> on 2016/10/03 17:52:36 UTC
Review Request 52488: GEODE-420: fix Pulse test when not using any
SSLConfig
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52488/
-----------------------------------------------------------
Review request for geode, Jared Stewart, Kevin Duling, Kirk Lund, and Udo Kohlmeyer.
Repository: geode
Description
-------
GEODE-420: fix Pulse test when not using any SSLConfig
Diffs
-----
geode-core/src/main/java/org/apache/geode/management/internal/JettyHelper.java 089dbacb056da84d6d6ceb36dad6f09b9ce73890
geode-pulse/src/test/java/org/apache/geode/tools/pulse/testbed/driver/PulseUITest.java a2365a2dfa3e221fa50664c3f8d96fae2bd93c86
geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/PulseAbstractTest.java 5024250f8ff3867e0f527d7ef5fbbd501e2d64ee
Diff: https://reviews.apache.org/r/52488/diff/
Testing
-------
precheckin running
Thanks,
Jinmei Liao
Re: Review Request 52488: GEODE-420: fix Pulse test when not using any
SSLConfig
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52488/#review151255
-----------------------------------------------------------
Ship it!
Ship It!
- Udo Kohlmeyer
On Oct. 3, 2016, 11:20 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52488/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2016, 11:20 p.m.)
>
>
> Review request for geode, Jared Stewart, Kevin Duling, Kirk Lund, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-420: fix Pulse test when not using any SSLConfig
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/JettyHelper.java 089dbacb056da84d6d6ceb36dad6f09b9ce73890
> geode-pulse/src/test/java/org/apache/geode/tools/pulse/testbed/driver/PulseUITest.java a2365a2dfa3e221fa50664c3f8d96fae2bd93c86
> geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/PulseAbstractTest.java 5024250f8ff3867e0f527d7ef5fbbd501e2d64ee
>
> Diff: https://reviews.apache.org/r/52488/diff/
>
>
> Testing
> -------
>
> precheckin passed
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 52488: GEODE-420: fix Pulse test when not using any
SSLConfig
Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52488/#review151256
-----------------------------------------------------------
Ship it!
Ship It!
- Jared Stewart
On Oct. 3, 2016, 11:20 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52488/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2016, 11:20 p.m.)
>
>
> Review request for geode, Jared Stewart, Kevin Duling, Kirk Lund, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-420: fix Pulse test when not using any SSLConfig
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/JettyHelper.java 089dbacb056da84d6d6ceb36dad6f09b9ce73890
> geode-pulse/src/test/java/org/apache/geode/tools/pulse/testbed/driver/PulseUITest.java a2365a2dfa3e221fa50664c3f8d96fae2bd93c86
> geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/PulseAbstractTest.java 5024250f8ff3867e0f527d7ef5fbbd501e2d64ee
>
> Diff: https://reviews.apache.org/r/52488/diff/
>
>
> Testing
> -------
>
> precheckin passed
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 52488: GEODE-420: fix Pulse test when not using any
SSLConfig
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52488/
-----------------------------------------------------------
(Updated Oct. 3, 2016, 11:20 p.m.)
Review request for geode, Jared Stewart, Kevin Duling, Kirk Lund, and Udo Kohlmeyer.
Changes
-------
use default SSLConfig instead of passing in null
test passed
Repository: geode
Description
-------
GEODE-420: fix Pulse test when not using any SSLConfig
Diffs
-----
geode-core/src/main/java/org/apache/geode/management/internal/JettyHelper.java 089dbacb056da84d6d6ceb36dad6f09b9ce73890
geode-pulse/src/test/java/org/apache/geode/tools/pulse/testbed/driver/PulseUITest.java a2365a2dfa3e221fa50664c3f8d96fae2bd93c86
geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/PulseAbstractTest.java 5024250f8ff3867e0f527d7ef5fbbd501e2d64ee
Diff: https://reviews.apache.org/r/52488/diff/
Testing (updated)
-------
precheckin passed
Thanks,
Jinmei Liao
Re: Review Request 52488: GEODE-420: fix Pulse test when not using any
SSLConfig
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52488/
-----------------------------------------------------------
(Updated Oct. 3, 2016, 11:18 p.m.)
Review request for geode, Jared Stewart, Kevin Duling, Kirk Lund, and Udo Kohlmeyer.
Repository: geode
Description
-------
GEODE-420: fix Pulse test when not using any SSLConfig
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/management/internal/JettyHelper.java 089dbacb056da84d6d6ceb36dad6f09b9ce73890
geode-pulse/src/test/java/org/apache/geode/tools/pulse/testbed/driver/PulseUITest.java a2365a2dfa3e221fa50664c3f8d96fae2bd93c86
geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/PulseAbstractTest.java 5024250f8ff3867e0f527d7ef5fbbd501e2d64ee
Diff: https://reviews.apache.org/r/52488/diff/
Testing
-------
precheckin running
Thanks,
Jinmei Liao
Re: Review Request 52488: GEODE-420: fix Pulse test when not using any
SSLConfig
Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52488/#review151196
-----------------------------------------------------------
Ship it!
Ship It!
- Jared Stewart
On Oct. 3, 2016, 5:52 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52488/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2016, 5:52 p.m.)
>
>
> Review request for geode, Jared Stewart, Kevin Duling, Kirk Lund, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-420: fix Pulse test when not using any SSLConfig
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/JettyHelper.java 089dbacb056da84d6d6ceb36dad6f09b9ce73890
> geode-pulse/src/test/java/org/apache/geode/tools/pulse/testbed/driver/PulseUITest.java a2365a2dfa3e221fa50664c3f8d96fae2bd93c86
> geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/PulseAbstractTest.java 5024250f8ff3867e0f527d7ef5fbbd501e2d64ee
>
> Diff: https://reviews.apache.org/r/52488/diff/
>
>
> Testing
> -------
>
> precheckin running
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 52488: GEODE-420: fix Pulse test when not using any
SSLConfig
Posted by Jinmei Liao <ji...@pivotal.io>.
> On Oct. 3, 2016, 6:54 p.m., Udo Kohlmeyer wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/JettyHelper.java, line 200
> > <https://reviews.apache.org/r/52488/diff/1/?file=1518865#file1518865line200>
> >
> > I don't think you should pass null. The SSLConfigurationFactory will always provide you with a SSLConfig object. This object might be a ssl-enabled=false, but that is ok. I think this should should be replaced with:
> >
> > SSLConfigurationFactory.setDistributionConfig(new DistributionConfig);
> > final Server jetty = JettyHelper.initJetty(null, 8090, SSLConfigurationFactory.getSSLConfigForComponent(SecurableCommunicationChannel.WEB));
DistributionConfig is an interface, and if I use DistributionConfigImpl(), I get compiling error as well. I understand your concern of "checking null", but by forcing callers to pass in a valid SSLConfig while there is no easy way for them to get a default-not-enabled one gives the caller unnecessary burden. We can either allow null here, or have SSLConfigurationFactory add a method to return a default-nothing-enabled config?
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52488/#review151207
-----------------------------------------------------------
On Oct. 3, 2016, 5:52 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52488/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2016, 5:52 p.m.)
>
>
> Review request for geode, Jared Stewart, Kevin Duling, Kirk Lund, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-420: fix Pulse test when not using any SSLConfig
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/JettyHelper.java 089dbacb056da84d6d6ceb36dad6f09b9ce73890
> geode-pulse/src/test/java/org/apache/geode/tools/pulse/testbed/driver/PulseUITest.java a2365a2dfa3e221fa50664c3f8d96fae2bd93c86
> geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/PulseAbstractTest.java 5024250f8ff3867e0f527d7ef5fbbd501e2d64ee
>
> Diff: https://reviews.apache.org/r/52488/diff/
>
>
> Testing
> -------
>
> precheckin running
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 52488: GEODE-420: fix Pulse test when not using any
SSLConfig
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52488/#review151207
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/management/internal/JettyHelper.java (line 72)
<https://reviews.apache.org/r/52488/#comment219481>
Eventhough it might be "good practice" to check for null,I don't think we should check for null. There will always be a SSLConfig object and we should expect one. We should rather fix the issue if the SSLConfig is not correctly configured.
geode-core/src/main/java/org/apache/geode/management/internal/JettyHelper.java (line 198)
<https://reviews.apache.org/r/52488/#comment219479>
I don't think you should pass null. The SSLConfigurationFactory will always provide you with a SSLConfig object. This object might be a ssl-enabled=false, but that is ok. I think this should should be replaced with:
SSLConfigurationFactory.setDistributionConfig(new DistributionConfig);
final Server jetty = JettyHelper.initJetty(null, 8090, SSLConfigurationFactory.getSSLConfigForComponent(SecurableCommunicationChannel.WEB));
- Udo Kohlmeyer
On Oct. 3, 2016, 5:52 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52488/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2016, 5:52 p.m.)
>
>
> Review request for geode, Jared Stewart, Kevin Duling, Kirk Lund, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-420: fix Pulse test when not using any SSLConfig
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/JettyHelper.java 089dbacb056da84d6d6ceb36dad6f09b9ce73890
> geode-pulse/src/test/java/org/apache/geode/tools/pulse/testbed/driver/PulseUITest.java a2365a2dfa3e221fa50664c3f8d96fae2bd93c86
> geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/PulseAbstractTest.java 5024250f8ff3867e0f527d7ef5fbbd501e2d64ee
>
> Diff: https://reviews.apache.org/r/52488/diff/
>
>
> Testing
> -------
>
> precheckin running
>
>
> Thanks,
>
> Jinmei Liao
>
>