You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2019/11/06 01:57:26 UTC
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/
-----------------------------------------------------------
(Updated Nov. 5, 2019, 5:57 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Changes
-------
Changed lambda captures of `this` to a weak pointer instead.
Summary (updated)
-----------------
SSL Wrapper: Implemented send/recv and shutdown.
Bugs: MESOS-10010
https://issues.apache.org/jira/browse/MESOS-10010
Repository: mesos
Description
-------
This completes a fully functional client-side SSL socket.
Needs a bit of cleanup and more error handling though.
Diffs (updated)
-----
3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71666/diff/3/
Changes: https://reviews.apache.org/r/71666/diff/2-3/
Testing (updated)
-------
```
cmake --build . --target libprocess-tests
libprocess-tests --gtest_filter="SSLTest.*"
```
Running libprocess-tests yields:
```
[ FAILED ] SSLTest.ValidDowngrade
[ FAILED ] SSLTest.ValidDowngradeEachProtocol
[ FAILED ] SSLTest.ShutdownThenSend
```
Thanks,
Joseph Wu
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/#review218520
-----------------------------------------------------------
Patch looks great!
Reviews applied: [71659, 71660, 71661, 71662, 71663, 71664, 71665, 71666]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Nov. 6, 2019, 3:41 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2019, 3:41 a.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This completes a fully functional client-side SSL socket.
>
> Needs a bit of cleanup and more error handling though.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71666/diff/4/
>
>
> Testing
> -------
>
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
>
> Running libprocess-tests yields:
> ```
> [ FAILED ] SSLTest.ValidDowngrade
> [ FAILED ] SSLTest.ValidDowngradeEachProtocol
> [ FAILED ] Encryption/NetSocketTest.EOFBeforeRecv/0, where GetParam() = "SSL"
> [ FAILED ] Encryption/NetSocketTest.EOFAfterRecv/0, where GetParam() = "SSL"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/#review218597
-----------------------------------------------------------
Patch looks great!
Reviews applied: [71659, 71660, 71661, 71662, 71663, 71664, 71665, 71666]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Nov. 11, 2019, 11:41 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2019, 11:41 a.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This completes a fully functional client-side SSL socket.
>
> Needs a bit of cleanup and more error handling though.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71666/diff/5/
>
>
> Testing
> -------
>
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
>
> Running libprocess-tests yields:
> ```
> [ FAILED ] SSLTest.ValidDowngrade
> [ FAILED ] SSLTest.ValidDowngradeEachProtocol
> [ FAILED ] Encryption/NetSocketTest.EOFBeforeRecv/0, where GetParam() = "SSL"
> [ FAILED ] Encryption/NetSocketTest.EOFAfterRecv/0, where GetParam() = "SSL"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 5, 2019, 6:38 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.hpp
> > Lines 94 (patched)
> > <https://reviews.apache.org/r/71666/diff/6/?file=2174527#file2174527line94>
> >
> > In this case, we don't really need an actor context, since there isn't any actor state associated with the compute thread. We really just want some context (any context) to dispatch the SSL-related functions onto, right?
> >
> > It would make a bit more sense to me to dispatch these functions without specifying an actor, so that libprocess can run them wherever it pleases.
> >
> > We could consider updating `loop()` to dispatch in all cases, even when no pid is specified. However, I do wonder if we're unknowingly depending on the existing behavior somewhere. In any case, changing loop to always `dispatch()` the iterate and body seems more desirable to me?
> >
> > However, the `loop()` calls below aren't strictly necessary I think. We could accomplish the same thing with dispatches and chained continuations, so we could also just use `dispatch()` directly instead of `loop()`, that might be the simplest thing to do.
> >
> > WDYT?
>
> Joseph Wu wrote:
> I think a UPID/actor is required for any dispatching/looping on libprocess worker threads, so this variable would still exist regardless of how the loops are implemented.
>
> The alternative is to run everything on the event loop thread (or special threads we spin up/acquire out of band?).
Ah I was remembering the version of `defer()` which can be invoked without a pid: https://github.com/apache/mesos/blob/925ad30c0f3b249afe74bdeb1921c5fdbf1c5886/3rdparty/libprocess/include/process/defer.hpp#L275-L283
Actually, I wish we had an overload of `dispatch()` that did something similar. In any case, the `defer()` overload might work here, WDYT?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/#review218948
-----------------------------------------------------------
On Nov. 20, 2019, 12:29 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2019, 12:29 a.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This completes a fully functional client-side SSL socket.
>
> Needs a bit of cleanup and more error handling though.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71666/diff/6/
>
>
> Testing
> -------
>
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
>
> Running libprocess-tests yields:
> ```
> [ FAILED ] SSLTest.ValidDowngrade
> [ FAILED ] SSLTest.ValidDowngradeEachProtocol
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 5, 2019, 10:38 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.hpp
> > Lines 94 (patched)
> > <https://reviews.apache.org/r/71666/diff/6/?file=2174527#file2174527line94>
> >
> > In this case, we don't really need an actor context, since there isn't any actor state associated with the compute thread. We really just want some context (any context) to dispatch the SSL-related functions onto, right?
> >
> > It would make a bit more sense to me to dispatch these functions without specifying an actor, so that libprocess can run them wherever it pleases.
> >
> > We could consider updating `loop()` to dispatch in all cases, even when no pid is specified. However, I do wonder if we're unknowingly depending on the existing behavior somewhere. In any case, changing loop to always `dispatch()` the iterate and body seems more desirable to me?
> >
> > However, the `loop()` calls below aren't strictly necessary I think. We could accomplish the same thing with dispatches and chained continuations, so we could also just use `dispatch()` directly instead of `loop()`, that might be the simplest thing to do.
> >
> > WDYT?
>
> Joseph Wu wrote:
> I think a UPID/actor is required for any dispatching/looping on libprocess worker threads, so this variable would still exist regardless of how the loops are implemented.
>
> The alternative is to run everything on the event loop thread (or special threads we spin up/acquire out of band?).
>
> Greg Mann wrote:
> Ah I was remembering the version of `defer()` which can be invoked without a pid: https://github.com/apache/mesos/blob/925ad30c0f3b249afe74bdeb1921c5fdbf1c5886/3rdparty/libprocess/include/process/defer.hpp#L275-L283
>
> Actually, I wish we had an overload of `dispatch()` that did something similar. In any case, the `defer()` overload might work here, WDYT?
>
> Joseph Wu wrote:
> That overload of `defer()` ends up running things on a thread_local UPID:
> ```
> // Per thread executor pointer. We use a pointer to lazily construct the
> // actual executor.
> extern thread_local Executor* _executor_;
>
> #define __executor__ \
> (_executor_ == nullptr ? _executor_ = new Executor() : _executor_)
> ```
>
> In this case, I think we would end up constructing a single `__executor__` object on the EventLoop thread (since that is where the `defer()` is called), so all socket IO would end up deferred onto the same UPID.
>
> Greg Mann wrote:
> Yea, I think you're right. That seems a bit better to me than constructing a one-off UPID specifically for SSL work, WDYT?
I mean the _same_ UPID (only one). Just one actor would handle all the SSL work.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/#review218948
-----------------------------------------------------------
On Nov. 19, 2019, 4:29 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2019, 4:29 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This completes a fully functional client-side SSL socket.
>
> Needs a bit of cleanup and more error handling though.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71666/diff/6/
>
>
> Testing
> -------
>
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
>
> Running libprocess-tests yields:
> ```
> [ FAILED ] SSLTest.ValidDowngrade
> [ FAILED ] SSLTest.ValidDowngradeEachProtocol
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 5, 2019, 6:38 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.hpp
> > Lines 94 (patched)
> > <https://reviews.apache.org/r/71666/diff/6/?file=2174527#file2174527line94>
> >
> > In this case, we don't really need an actor context, since there isn't any actor state associated with the compute thread. We really just want some context (any context) to dispatch the SSL-related functions onto, right?
> >
> > It would make a bit more sense to me to dispatch these functions without specifying an actor, so that libprocess can run them wherever it pleases.
> >
> > We could consider updating `loop()` to dispatch in all cases, even when no pid is specified. However, I do wonder if we're unknowingly depending on the existing behavior somewhere. In any case, changing loop to always `dispatch()` the iterate and body seems more desirable to me?
> >
> > However, the `loop()` calls below aren't strictly necessary I think. We could accomplish the same thing with dispatches and chained continuations, so we could also just use `dispatch()` directly instead of `loop()`, that might be the simplest thing to do.
> >
> > WDYT?
>
> Joseph Wu wrote:
> I think a UPID/actor is required for any dispatching/looping on libprocess worker threads, so this variable would still exist regardless of how the loops are implemented.
>
> The alternative is to run everything on the event loop thread (or special threads we spin up/acquire out of band?).
>
> Greg Mann wrote:
> Ah I was remembering the version of `defer()` which can be invoked without a pid: https://github.com/apache/mesos/blob/925ad30c0f3b249afe74bdeb1921c5fdbf1c5886/3rdparty/libprocess/include/process/defer.hpp#L275-L283
>
> Actually, I wish we had an overload of `dispatch()` that did something similar. In any case, the `defer()` overload might work here, WDYT?
>
> Joseph Wu wrote:
> That overload of `defer()` ends up running things on a thread_local UPID:
> ```
> // Per thread executor pointer. We use a pointer to lazily construct the
> // actual executor.
> extern thread_local Executor* _executor_;
>
> #define __executor__ \
> (_executor_ == nullptr ? _executor_ = new Executor() : _executor_)
> ```
>
> In this case, I think we would end up constructing a single `__executor__` object on the EventLoop thread (since that is where the `defer()` is called), so all socket IO would end up deferred onto the same UPID.
Yea, I think you're right. That seems a bit better to me than constructing a one-off UPID specifically for SSL work, WDYT?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/#review218948
-----------------------------------------------------------
On Nov. 20, 2019, 12:29 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2019, 12:29 a.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This completes a fully functional client-side SSL socket.
>
> Needs a bit of cleanup and more error handling though.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71666/diff/6/
>
>
> Testing
> -------
>
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
>
> Running libprocess-tests yields:
> ```
> [ FAILED ] SSLTest.ValidDowngrade
> [ FAILED ] SSLTest.ValidDowngradeEachProtocol
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 5, 2019, 10:38 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.hpp
> > Lines 94 (patched)
> > <https://reviews.apache.org/r/71666/diff/6/?file=2174527#file2174527line94>
> >
> > In this case, we don't really need an actor context, since there isn't any actor state associated with the compute thread. We really just want some context (any context) to dispatch the SSL-related functions onto, right?
> >
> > It would make a bit more sense to me to dispatch these functions without specifying an actor, so that libprocess can run them wherever it pleases.
> >
> > We could consider updating `loop()` to dispatch in all cases, even when no pid is specified. However, I do wonder if we're unknowingly depending on the existing behavior somewhere. In any case, changing loop to always `dispatch()` the iterate and body seems more desirable to me?
> >
> > However, the `loop()` calls below aren't strictly necessary I think. We could accomplish the same thing with dispatches and chained continuations, so we could also just use `dispatch()` directly instead of `loop()`, that might be the simplest thing to do.
> >
> > WDYT?
>
> Joseph Wu wrote:
> I think a UPID/actor is required for any dispatching/looping on libprocess worker threads, so this variable would still exist regardless of how the loops are implemented.
>
> The alternative is to run everything on the event loop thread (or special threads we spin up/acquire out of band?).
>
> Greg Mann wrote:
> Ah I was remembering the version of `defer()` which can be invoked without a pid: https://github.com/apache/mesos/blob/925ad30c0f3b249afe74bdeb1921c5fdbf1c5886/3rdparty/libprocess/include/process/defer.hpp#L275-L283
>
> Actually, I wish we had an overload of `dispatch()` that did something similar. In any case, the `defer()` overload might work here, WDYT?
That overload of `defer()` ends up running things on a thread_local UPID:
```
// Per thread executor pointer. We use a pointer to lazily construct the
// actual executor.
extern thread_local Executor* _executor_;
#define __executor__ \
(_executor_ == nullptr ? _executor_ = new Executor() : _executor_)
```
In this case, I think we would end up constructing a single `__executor__` object on the EventLoop thread (since that is where the `defer()` is called), so all socket IO would end up deferred onto the same UPID.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/#review218948
-----------------------------------------------------------
On Nov. 19, 2019, 4:29 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2019, 4:29 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This completes a fully functional client-side SSL socket.
>
> Needs a bit of cleanup and more error handling though.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71666/diff/6/
>
>
> Testing
> -------
>
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
>
> Running libprocess-tests yields:
> ```
> [ FAILED ] SSLTest.ValidDowngrade
> [ FAILED ] SSLTest.ValidDowngradeEachProtocol
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 5, 2019, 10:38 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.hpp
> > Lines 94 (patched)
> > <https://reviews.apache.org/r/71666/diff/6/?file=2174527#file2174527line94>
> >
> > In this case, we don't really need an actor context, since there isn't any actor state associated with the compute thread. We really just want some context (any context) to dispatch the SSL-related functions onto, right?
> >
> > It would make a bit more sense to me to dispatch these functions without specifying an actor, so that libprocess can run them wherever it pleases.
> >
> > We could consider updating `loop()` to dispatch in all cases, even when no pid is specified. However, I do wonder if we're unknowingly depending on the existing behavior somewhere. In any case, changing loop to always `dispatch()` the iterate and body seems more desirable to me?
> >
> > However, the `loop()` calls below aren't strictly necessary I think. We could accomplish the same thing with dispatches and chained continuations, so we could also just use `dispatch()` directly instead of `loop()`, that might be the simplest thing to do.
> >
> > WDYT?
I think a UPID/actor is required for any dispatching/looping on libprocess worker threads, so this variable would still exist regardless of how the loops are implemented.
The alternative is to run everything on the event loop thread (or special threads we spin up/acquire out of band?).
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/#review218948
-----------------------------------------------------------
On Nov. 19, 2019, 4:29 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2019, 4:29 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This completes a fully functional client-side SSL socket.
>
> Needs a bit of cleanup and more error handling though.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71666/diff/6/
>
>
> Testing
> -------
>
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
>
> Running libprocess-tests yields:
> ```
> [ FAILED ] SSLTest.ValidDowngrade
> [ FAILED ] SSLTest.ValidDowngradeEachProtocol
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/#review218948
-----------------------------------------------------------
3rdparty/libprocess/src/ssl/socket_wrapper.hpp
Lines 94 (patched)
<https://reviews.apache.org/r/71666/#comment306923>
In this case, we don't really need an actor context, since there isn't any actor state associated with the compute thread. We really just want some context (any context) to dispatch the SSL-related functions onto, right?
It would make a bit more sense to me to dispatch these functions without specifying an actor, so that libprocess can run them wherever it pleases.
We could consider updating `loop()` to dispatch in all cases, even when no pid is specified. However, I do wonder if we're unknowingly depending on the existing behavior somewhere. In any case, changing loop to always `dispatch()` the iterate and body seems more desirable to me?
However, the `loop()` calls below aren't strictly necessary I think. We could accomplish the same thing with dispatches and chained continuations, so we could also just use `dispatch()` directly instead of `loop()`, that might be the simplest thing to do.
WDYT?
- Greg Mann
On Nov. 20, 2019, 12:29 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2019, 12:29 a.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This completes a fully functional client-side SSL socket.
>
> Needs a bit of cleanup and more error handling though.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71666/diff/6/
>
>
> Testing
> -------
>
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
>
> Running libprocess-tests yields:
> ```
> [ FAILED ] SSLTest.ValidDowngrade
> [ FAILED ] SSLTest.ValidDowngradeEachProtocol
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/#review219053
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On Dec. 16, 2019, 10:05 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2019, 10:05 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This completes a fully functional client-side SSL socket.
>
> Needs a bit of cleanup and more error handling though.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/ssl/openssl_socket.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/openssl_socket.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71666/diff/8/
>
>
> Testing
> -------
>
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
>
> Running libprocess-tests yields:
> ```
> [ FAILED ] SSLTest.ValidDowngrade
> [ FAILED ] SSLTest.ValidDowngradeEachProtocol
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/
-----------------------------------------------------------
(Updated Dec. 16, 2019, 2:05 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Changes
-------
Comment tweak.
Bugs: MESOS-10010
https://issues.apache.org/jira/browse/MESOS-10010
Repository: mesos
Description
-------
This completes a fully functional client-side SSL socket.
Needs a bit of cleanup and more error handling though.
Diffs (updated)
-----
3rdparty/libprocess/src/ssl/openssl_socket.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/openssl_socket.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71666/diff/8/
Changes: https://reviews.apache.org/r/71666/diff/7-8/
Testing
-------
```
cmake --build . --target libprocess-tests
libprocess-tests
```
Running libprocess-tests yields:
```
[ FAILED ] SSLTest.ValidDowngrade
[ FAILED ] SSLTest.ValidDowngradeEachProtocol
```
Thanks,
Joseph Wu
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 13, 2019, 11:41 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/openssl_socket.cpp
> > Lines 478 (patched)
> > <https://reviews.apache.org/r/71666/diff/7/?file=2183605#file2183605line478>
> >
> > Is it possible that a client-initiated renegotiation will lead to an SSL_ERROR_WANT_READ result here? Would we hang in that case?
> >
> > Same question for an SSL_ERROR_WANT_WRITE result in `recv()`.
https://issues.apache.org/jira/browse/MESOS-10070
We have reason to believe the OpenSSL library may handle this transparently for us, but a unit test would make sure of this.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/#review219029
-----------------------------------------------------------
On Dec. 16, 2019, 2:05 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2019, 2:05 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This completes a fully functional client-side SSL socket.
>
> Needs a bit of cleanup and more error handling though.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/ssl/openssl_socket.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/openssl_socket.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71666/diff/8/
>
>
> Testing
> -------
>
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
>
> Running libprocess-tests yields:
> ```
> [ FAILED ] SSLTest.ValidDowngrade
> [ FAILED ] SSLTest.ValidDowngradeEachProtocol
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/#review219029
-----------------------------------------------------------
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 478 (patched)
<https://reviews.apache.org/r/71666/#comment307057>
Is it possible that a client-initiated renegotiation will lead to an SSL_ERROR_WANT_READ result here? Would we hang in that case?
Same question for an SSL_ERROR_WANT_WRITE result in `recv()`.
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 683 (patched)
<https://reviews.apache.org/r/71666/#comment307056>
Maybe s/Server sockets/Listening sockets/
Since accepted server-side sockets will create a UPID.
- Greg Mann
On Dec. 10, 2019, 11:55 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2019, 11:55 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This completes a fully functional client-side SSL socket.
>
> Needs a bit of cleanup and more error handling though.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/ssl/openssl_socket.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/openssl_socket.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71666/diff/7/
>
>
> Testing
> -------
>
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
>
> Running libprocess-tests yields:
> ```
> [ FAILED ] SSLTest.ValidDowngrade
> [ FAILED ] SSLTest.ValidDowngradeEachProtocol
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/
-----------------------------------------------------------
(Updated Dec. 10, 2019, 3:55 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Changes
-------
Rebase on earlier rename.
Bugs: MESOS-10010
https://issues.apache.org/jira/browse/MESOS-10010
Repository: mesos
Description
-------
This completes a fully functional client-side SSL socket.
Needs a bit of cleanup and more error handling though.
Diffs (updated)
-----
3rdparty/libprocess/src/ssl/openssl_socket.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/openssl_socket.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71666/diff/7/
Changes: https://reviews.apache.org/r/71666/diff/6-7/
Testing
-------
```
cmake --build . --target libprocess-tests
libprocess-tests
```
Running libprocess-tests yields:
```
[ FAILED ] SSLTest.ValidDowngrade
[ FAILED ] SSLTest.ValidDowngradeEachProtocol
```
Thanks,
Joseph Wu
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/
-----------------------------------------------------------
(Updated Nov. 19, 2019, 4:29 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Changes
-------
* Added a "compute_thread" so that calls to `SSL_read` and `SSL_write` can be spread onto (many) libprocess worker threads rather than the (one) event loop thread.
* Changed shutdown to happen asynchronously but accept dirty shutdowns.
* Guarded against new send/recv calls after shutdown.
Bugs: MESOS-10010
https://issues.apache.org/jira/browse/MESOS-10010
Repository: mesos
Description
-------
This completes a fully functional client-side SSL socket.
Needs a bit of cleanup and more error handling though.
Diffs (updated)
-----
3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71666/diff/6/
Changes: https://reviews.apache.org/r/71666/diff/5-6/
Testing (updated)
-------
```
cmake --build . --target libprocess-tests
libprocess-tests
```
Running libprocess-tests yields:
```
[ FAILED ] SSLTest.ValidDowngrade
[ FAILED ] SSLTest.ValidDowngradeEachProtocol
```
Thanks,
Joseph Wu
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/#review218610
-----------------------------------------------------------
Notes from an offline review session:
3rdparty/libprocess/src/ssl/socket_wrapper.cpp
Lines 464-465 (patched)
<https://reviews.apache.org/r/71666/#comment306402>
Consider adding UPIDs to spread the load (of SSL encryption/decryption) off the event loop thread.
Because `io::read` and `io::write` always complete on the event loop thread, the continuations from these loops will also run on the event loop thread. In practice, this means all the SSL encryption/decryption happens in the same thread. There is no benefit to forcing everything onto the same thread, as OpenSSL is threadsafe (some caveats on older versions though).
3rdparty/libprocess/src/ssl/socket_wrapper.cpp
Lines 657-663 (patched)
<https://reviews.apache.org/r/71666/#comment306401>
Consider not blocking, but instead just trying to send the shutdown bits once.
- Joseph Wu
On Nov. 11, 2019, 11:41 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2019, 11:41 a.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This completes a fully functional client-side SSL socket.
>
> Needs a bit of cleanup and more error handling though.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71666/diff/5/
>
>
> Testing
> -------
>
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
>
> Running libprocess-tests yields:
> ```
> [ FAILED ] SSLTest.ValidDowngrade
> [ FAILED ] SSLTest.ValidDowngradeEachProtocol
> [ FAILED ] Encryption/NetSocketTest.EOFBeforeRecv/0, where GetParam() = "SSL"
> [ FAILED ] Encryption/NetSocketTest.EOFAfterRecv/0, where GetParam() = "SSL"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/#review218700
-----------------------------------------------------------
Bad review!
Reviews applied: [71666, 71665, 71664, 71663, 71662, 71661, 71660, 71764, 71659]
Error:
2019-11-19 22:48:17 URL:https://reviews.apache.org/r/71665/diff/raw/ [16358/16358] -> "71665.patch" [1]
error: patch failed: 3rdparty/libprocess/src/ssl/socket_wrapper.cpp:30
error: 3rdparty/libprocess/src/ssl/socket_wrapper.cpp: patch does not apply
- Mesos Reviewbot
On Nov. 11, 2019, 7:41 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2019, 7:41 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This completes a fully functional client-side SSL socket.
>
> Needs a bit of cleanup and more error handling though.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71666/diff/5/
>
>
> Testing
> -------
>
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
>
> Running libprocess-tests yields:
> ```
> [ FAILED ] SSLTest.ValidDowngrade
> [ FAILED ] SSLTest.ValidDowngradeEachProtocol
> [ FAILED ] Encryption/NetSocketTest.EOFBeforeRecv/0, where GetParam() = "SSL"
> [ FAILED ] Encryption/NetSocketTest.EOFAfterRecv/0, where GetParam() = "SSL"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/#review218629
-----------------------------------------------------------
Patch looks great!
Reviews applied: [71659, 71764, 71660, 71661, 71662, 71663, 71664, 71665, 71666]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Nov. 11, 2019, 7:41 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2019, 7:41 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This completes a fully functional client-side SSL socket.
>
> Needs a bit of cleanup and more error handling though.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71666/diff/5/
>
>
> Testing
> -------
>
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
>
> Running libprocess-tests yields:
> ```
> [ FAILED ] SSLTest.ValidDowngrade
> [ FAILED ] SSLTest.ValidDowngradeEachProtocol
> [ FAILED ] Encryption/NetSocketTest.EOFBeforeRecv/0, where GetParam() = "SSL"
> [ FAILED ] Encryption/NetSocketTest.EOFAfterRecv/0, where GetParam() = "SSL"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/
-----------------------------------------------------------
(Updated Nov. 11, 2019, 11:41 a.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Changes
-------
Reflected a tweak made in the previous review to handle EOF from `recv`-ing
Bugs: MESOS-10010
https://issues.apache.org/jira/browse/MESOS-10010
Repository: mesos
Description
-------
This completes a fully functional client-side SSL socket.
Needs a bit of cleanup and more error handling though.
Diffs (updated)
-----
3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71666/diff/5/
Changes: https://reviews.apache.org/r/71666/diff/4-5/
Testing
-------
```
cmake --build . --target libprocess-tests
libprocess-tests
```
Running libprocess-tests yields:
```
[ FAILED ] SSLTest.ValidDowngrade
[ FAILED ] SSLTest.ValidDowngradeEachProtocol
[ FAILED ] Encryption/NetSocketTest.EOFBeforeRecv/0, where GetParam() = "SSL"
[ FAILED ] Encryption/NetSocketTest.EOFAfterRecv/0, where GetParam() = "SSL"
```
Thanks,
Joseph Wu
Re: Review Request 71666: SSL Wrapper: Implemented send/recv and
shutdown.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71666/
-----------------------------------------------------------
(Updated Nov. 5, 2019, 7:41 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Changes
-------
Adjusted shutdown logic to avoid deadlocks with the ProcessManager/SocketManager. (Basically, don't use process::loop here)
Bugs: MESOS-10010
https://issues.apache.org/jira/browse/MESOS-10010
Repository: mesos
Description
-------
This completes a fully functional client-side SSL socket.
Needs a bit of cleanup and more error handling though.
Diffs (updated)
-----
3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71666/diff/4/
Changes: https://reviews.apache.org/r/71666/diff/3-4/
Testing (updated)
-------
```
cmake --build . --target libprocess-tests
libprocess-tests
```
Running libprocess-tests yields:
```
[ FAILED ] SSLTest.ValidDowngrade
[ FAILED ] SSLTest.ValidDowngradeEachProtocol
[ FAILED ] Encryption/NetSocketTest.EOFBeforeRecv/0, where GetParam() = "SSL"
[ FAILED ] Encryption/NetSocketTest.EOFAfterRecv/0, where GetParam() = "SSL"
```
Thanks,
Joseph Wu