mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 70991: Updated `Socket::connect()` API according to maintainer feedback.
Date Wed, 03 Jul 2019 16:02:11 GMT

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



Overall approach looks good, thanks for the update!

Some comments below, but I feel comfortable at this point with other reviewers looking things
over.


3rdparty/libprocess/include/process/ssl/tls_config.hpp
Lines 26-28 (patched)
<https://reviews.apache.org/r/70991/#comment303515>

    Stale comment? Connect requires the tls config, right?



3rdparty/libprocess/include/process/ssl/tls_config.hpp
Lines 62-63 (patched)
<https://reviews.apache.org/r/70991/#comment303516>

    Some guidance on what these are and how one should use them would be helpful, probably
on the variables here (or if on the types above, perhaps a comment here pointing the reader
to the above explanations)



3rdparty/libprocess/include/process/ssl/tls_config.hpp
Lines 82 (patched)
<https://reviews.apache.org/r/70991/#comment303517>

    `LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME=libprocess` seems a little strange, can we
call the "old" way `legacy` to signal that it's deprecated and we want to remove it?



3rdparty/libprocess/src/http.cpp
Line 1452 (original), 1454-1465 (patched)
<https://reviews.apache.org/r/70991/#comment303514>

    How about:
    
    ```
      Future<Nothing> connected = [&]() {
        switch (scheme) {
          case Scheme::HTTP:
            return socket->connect(address);
    #ifdef USE_SSL_SOCKET
          case Scheme::HTTPS:
            return socket->connect(
                address,
                network::openssl::create_tls_client_config(peer_hostname));
    #endif
        }
        UNREACHABLE(); // If needed to silence compiler.
      }();
    ```



3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp
Lines 518 (patched)
<https://reviews.apache.org/r/70991/#comment303513>

    Ditto here w.r.t. EXIT



3rdparty/libprocess/src/posix/poll_socket.cpp
Lines 167 (patched)
<https://reviews.apache.org/r/70991/#comment303512>

    EXIT will not produce a stack trace, can you use LOG(FATAL) here so that we can see who
made the incorrect call?
    
    Generally, we don't use EXIT for programming errors for this reason



3rdparty/libprocess/src/process.cpp
Lines 1453 (patched)
<https://reviews.apache.org/r/70991/#comment303511>

    s/std:://



3rdparty/libprocess/src/tests/http_tests.cpp
Line 263 (original), 264-276 (patched)
<https://reviews.apache.org/r/70991/#comment303518>

    Ditto here, can use the lambda approach?



3rdparty/libprocess/src/tests/socket_tests.cpp
Lines 68 (patched)
<https://reviews.apache.org/r/70991/#comment303509>

    no need for the break anymore?



3rdparty/libprocess/src/tests/socket_tests.cpp
Lines 73 (patched)
<https://reviews.apache.org/r/70991/#comment303519>

    Any comment on why we don't pass a hostname here or if we should? Do these tests even
exercise the SSL socket? Will the branch be executed?



3rdparty/libprocess/src/tests/socket_tests.cpp
Lines 75 (patched)
<https://reviews.apache.org/r/70991/#comment303510>

    ditto here, no need for the break?



3rdparty/libprocess/src/tests/ssl_client.cpp
Line 147 (original), 147-157 (patched)
<https://reviews.apache.org/r/70991/#comment303520>

    Ditto here for assignment from result of a lambda?



3rdparty/libprocess/src/tests/ssl_client.cpp
Lines 155 (patched)
<https://reviews.apache.org/r/70991/#comment303521>

    Ditto here on a comment about why we don't pass hostname here



3rdparty/libprocess/src/tests/ssl_tests.cpp
Lines 559 (patched)
<https://reviews.apache.org/r/70991/#comment303523>

    For all these spots passing None(), should we explain?
    
    ```
      const Future<Nothing> connect = client.connect(
          server_address.get(),
          // E.g. Don't care about hostname certificate validation?
          openssl::create_tls_client_config(None()));
    ```



3rdparty/libprocess/src/tests/ssl_tests.cpp
Lines 1008-1022 (patched)
<https://reviews.apache.org/r/70991/#comment303524>

    Should we also verify the configure/verify error cases surfacing correctly as errors?


- Benjamin Mahler


On July 2, 2019, 5:50 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70991/
> -----------------------------------------------------------
> 
> (Updated July 2, 2019, 5:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9878
>     https://issues.apache.org/jira/browse/MESOS-9878
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Reworked the API of `Socket::connect()` according to the following
> boundary conditions requested by libprocess maintainers:
> 
>  * It shall be possible to use custom connection options when
>    connecting with a SSL socket.
>  * When libprocess is compiled without SSL support, neither the
>    declaration of the TLS configuration object nor the `connnect()`
>    overload that accepts the TLS configuration should be available.
>  * Passing just the servername is not an acceptable short-hand for
>    using the default TLS configuration together with that servername.
>  * When the incorrect overload is selected (i.e. passing TLS config
>    to a poll socket or omitting TLS configuration for a TLS socket),
>    the program should abort.
> 
> This commit changes the API of `Socket::connect()` according to the
> requirements above. In particular:
> 
>  * A new class `openssl::TLSClientConfig` is introduced when libprocess
>    is compiled with ssl support.
>  * A new overload
>    `Socket::connect(const Address&, const TLSClientConfig&)` is
>    introduced when libprocess is compiled with ssl support.
>  * All call sites are adjusted to check the socket kind before calling
>    `connect()`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/Makefile.am 1ddcc2d5a30f7bf3914138e497a9b228b515cd29 
>   3rdparty/libprocess/include/process/socket.hpp 4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83

>   3rdparty/libprocess/include/process/ssl/tls_config.hpp PRE-CREATION 
>   3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d 
>   3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
>   3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0 
>   3rdparty/libprocess/src/poll_socket.hpp 15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7 
>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0

>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 7e2229a9ed815727500bd457356e5531607fa6cf

>   3rdparty/libprocess/src/posix/poll_socket.cpp 74acb6942682a9d9626df81b303eba0a1c24ecf7

>   3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0 
>   3rdparty/libprocess/src/tests/http_tests.cpp 4d372943a2d417d24d06444ec2e72909fb348017

>   3rdparty/libprocess/src/tests/socket_tests.cpp b09ae23a551c6587656b2d5f6f58c5267e8e0088

>   3rdparty/libprocess/src/tests/ssl_client.cpp de87b3b89c84d17f2ebba1f09e9ec682f139aace

>   3rdparty/libprocess/src/tests/ssl_tests.cpp 5d360221937e68da185754f0633fa41a217c7107

> 
> 
> Diff: https://reviews.apache.org/r/70991/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


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