You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "bschuchardt (GitHub)" <gi...@apache.org> on 2019/01/14 21:05:44 UTC
[GitHub] [geode] bschuchardt opened pull request #3075: Feature/geode
2113e Implement SSL over NIO
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
This removes use of Old-IO in peer-to-peer communications, enabling
the use of SSL with New-IO Buffers and SocketChannels by way of the JDK's
SSLEngine.
The first commit is the original implementation. The second commit
fixes the problems in that commit. If you've already reviewed the old
code you can probably just look at the second commit.
- position tracking for MsgReader has been moved into NioSslEngine and
NioPlainEngine because the Ssl engine sometimes needs to allocate a new
buffer and adjust the tracked read/process positions in the buffer
- the same buffer expansion methods in Buffers was being used for
read-side and write-side buffers but buffers being used for reads need
to be handled differently than buffers being used for writing
There is commented-out debugging code that I'll remove when these
changes pass the Pull Request Stress Tests.
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "kirklund (GitHub)" <gi...@apache.org>.
You might want to just use <pre> tags around lines of code:
```
* <pre>
* wrappedBuffer = filter.ensureWrappedCapacity(amount, wrappedBuffer, etc.);
* unwrappedBuffer = filter.readAtLeast(channel, amount, wrappedBuffer, etc.);
* </pre>
```
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] galen-pivotal commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
It's a little weird to me that we would use the buffer after expanding it. And also, if expanding a buffer actually returns a new buffer, should the old caller have to free the old one or expect that it has been freed as a result of expanding and the new one being returned?
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Recommend deleting commented-out code.
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] galen-pivotal commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
I'm pretty sure this can be a local variable. Also, it's never released and should be once the handshake is over.
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] bschuchardt commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
I made it an instance variable for testing.
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "kirklund (GitHub)" <gi...@apache.org>.
I'm not sure you should use 10 SECONDS. GeodeAwaitility defaults to 5 MINUTES which is larger than it needs to be just to avoid false failures on amazingly slow machines or long GC pauses in the middle of a test.
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] galen-pivotal commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
This class does a lot of work in `ensureWrappedCapacity`, but `NioSslEngine` doesn't do any. How come? What exactly is this ensuring?
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] galen-pivotal commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
It would be nice to have more integration tests of `NioSslEngine`, specifically writing over a real socket to make sure your socket-handling logic is right.
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "kirklund (GitHub)" <gi...@apache.org>.
I would change this catch-block to use a SharedErrorCollector. Add this rule:
```
@Rule
public SharedErrorCollector errorCollector = new SharedErrorCollector();
```
And then in any catch-block use this:
```
errorCollector.addError(e);
```
The errorCollector can be used from any thread in any dunit VM. After the test method executes, the errorCollector will fail with every stack that was added to it.
If you want the test to fail sooner, then you still need to rethrow or set-and-check a volatile. You could use ExecutorServiceRule which is just an ExecutorService that invokes shutdownNow in the tearDown. Then you could return a Future to the test method. If the Runnable throws Error/Runtime or Callable throws anything then any calls to Future get() should rethrow it wrapped in an ExecutionException.
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] bschuchardt commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
NioSslEngine doesn't have to increase the size of network buffers because their size is established by the SslContext.
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] bschuchardt commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
You don't have to free ByteBuffers
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] galen-pivotal commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
Should we check that the channel is closed here?
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] bschuchardt commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
I haven't been able to get Mockito to do verification on close() - it always goes to an abstract superclass of SocketChannel and throws an NPE.
doNothing().when(mockChannel).close();
verify(mockChannel, times(1)).close();
java.lang.NullPointerException
at java.nio.channels.spi.AbstractInterruptibleChannel.close(AbstractInterruptibleChannel.java:111)
at org.apache.geode.internal.net.NioSslEngineTest.closeWhenSocketWriteError(NioSslEngineTest.java:372)
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] bschuchardt commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
That's done in SSLSocketIntegrationTest
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] galen-pivotal commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
You shouldn't need to specify 5 minutes, that's the default with `GeodeAwaitility`.
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] bschuchardt closed pull request #3075: Feature/geode
2113e Implement SSL over NIO
Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
[ pull request closed by bschuchardt ]
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] bschuchardt commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
This test is for unmanaged buffers. Testing the pooling code in Buffers needs to be added sometime but I didn't need it for testing buffer expansion.
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #3075:
Feature/geode 2113e Implement SSL over NIO
Posted by "kirklund (GitHub)" <gi...@apache.org>.
You might want to just use pre tags around lines of code:
```
* <pre>
* wrappedBuffer = filter.ensureWrappedCapacity(amount, wrappedBuffer, etc.);
* unwrappedBuffer = filter.readAtLeast(channel, amount, wrappedBuffer, etc.);
* </pre>
```
[ Full content available at: https://github.com/apache/geode/pull/3075 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org