mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benno Evers <bev...@mesosphere.com>
Subject Re: Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.
Date Tue, 19 Nov 2019 13:30:38 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71660/#review218670
-----------------------------------------------------------




3rdparty/libprocess/src/CMakeLists.txt
Lines 23 (patched)
<https://reviews.apache.org/r/71660/#comment306536>

    I'm not sure I completely understand the rationale for the exclusivity here (and elsewhere
in the review). Intuitively, I'd expect
    
    ```
    OpenSSL >= 1.1 found =>  Enable SocketWrapper
    Libevent enabled => Enable LibeventSSLSocket
    --enable-ssl => Require either of the above to be enabled
    ```
    
    In particular it doesn't seem to be too far-fetched that someone might want to use libevent
but not the libevent ssl socket in the future, due to bugs like MESOS-9867.



3rdparty/libprocess/src/ssl/socket_wrapper.hpp
Lines 24 (patched)
<https://reviews.apache.org/r/71660/#comment306537>

    Why is this derived from `PollSocketImpl`, rather than just from `SocketImpl`?



3rdparty/libprocess/src/ssl/socket_wrapper.hpp
Lines 25 (patched)
<https://reviews.apache.org/r/71660/#comment306539>

    This is mostly bike-shedding, so feel free to drop this issue if you disagree, but I'm
not a huge fan of the name `SSLSocketWrapper`:
    
    First, the name would suggest to me that the constructor argument `int_fd s` is already
a SSL socket that is being wrapped by this class.
    
    Second, the class doesn *wrap* a Socket, it *is* a SocketImpl.
    
    Finally, regarding the other implementations of `SocketImpl`, it seems like `OpenSSLSocketImpl`
would be a bit more consistent.


- Benno Evers


On Nov. 13, 2019, 7:03 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71660/
> -----------------------------------------------------------
> 
> (Updated Nov. 13, 2019, 7:03 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-10009
>     https://issues.apache.org/jira/browse/MESOS-10009
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This creates some new files implementing the SocketImpl class,
> in preparation for implementing generic SSL sockets.
> 
> The new class is used whenever SSL is enabled, but Libevent is not.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 641251af5631e9b3928dfb282cdbc266ba76572e 
>   3rdparty/libprocess/src/CMakeLists.txt 40c8ef935104bab4ea2f8b2b5919450b55165f60 
>   3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3 
>   3rdparty/libprocess/src/socket.cpp 606a1c46e50936251c29af4b996007c480b2a135 
>   3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION 
>   3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71660/diff/3/
> 
> 
> Testing
> -------
> 
> cmake .. -ENABLE_SSL=1   # No Libevent here!
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message