You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2020/04/10 19:30:09 UTC
Review Request 72348: Added logging of peer address in
LibeventSSLSocket accept failures.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72348/
-----------------------------------------------------------
Review request for mesos and Greg Mann.
Bugs: MESOS-10112
https://issues.apache.org/jira/browse/MESOS-10112
Repository: mesos
Description
-------
The caller of LibeventSSLSocket::accept() cannot see who tried to
connect when accept fails, since the accepted socket is not returned.
This adds logging of the peer address when the SSL handshake fails,
in order to improve debugging.
Diffs
-----
3rdparty/libprocess/include/process/address.hpp f23e653aa276e4806a88d8b54f84375ccbaad006
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 7bcc66fb3501d6c120bc13d2fd85bd98dbb5673a
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp dcb6d8e6c82005145c853afa9c24a61d7d0f04a9
Diff: https://reviews.apache.org/r/72348/diff/1/
Testing
-------
make check
Thanks,
Benjamin Mahler
Re: Review Request 72348: Added logging of peer address in
LibeventSSLSocket accept failures.
Posted by Greg Mann <gr...@mesosphere.io>.
> On April 14, 2020, 7:14 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/include/process/address.hpp
> > Lines 344-367 (original), 344-371 (patched)
> > <https://reviews.apache.org/r/72348/diff/1/?file=2217422#file2217422line344>
> >
> > Since `sockaddr_storage` can be cast as `sockaddr_{un,in,in6}`, maybe we want to combine these into a single function? Actually, after looking at some existing callsites of `Address::create()`, I wonder if we should eliminate the overload for `sockaddr_storage`?
>
> Benjamin Mahler wrote:
> Yeah, as discussed offline I debated doing this in this patch, but will instead just add the overload here to simplify backporting and have a follow up patch that removes the sockaddr_storage overload.
sgtm, thanks!!
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72348/#review220312
-----------------------------------------------------------
On April 10, 2020, 7:30 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72348/
> -----------------------------------------------------------
>
> (Updated April 10, 2020, 7:30 p.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-10112
> https://issues.apache.org/jira/browse/MESOS-10112
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The caller of LibeventSSLSocket::accept() cannot see who tried to
> connect when accept fails, since the accepted socket is not returned.
> This adds logging of the peer address when the SSL handshake fails,
> in order to improve debugging.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp f23e653aa276e4806a88d8b54f84375ccbaad006
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 7bcc66fb3501d6c120bc13d2fd85bd98dbb5673a
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp dcb6d8e6c82005145c853afa9c24a61d7d0f04a9
>
>
> Diff: https://reviews.apache.org/r/72348/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 72348: Added logging of peer address in
LibeventSSLSocket accept failures.
Posted by Benjamin Mahler <bm...@apache.org>.
> On April 14, 2020, 7:14 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/include/process/address.hpp
> > Lines 344-367 (original), 344-371 (patched)
> > <https://reviews.apache.org/r/72348/diff/1/?file=2217422#file2217422line344>
> >
> > Since `sockaddr_storage` can be cast as `sockaddr_{un,in,in6}`, maybe we want to combine these into a single function? Actually, after looking at some existing callsites of `Address::create()`, I wonder if we should eliminate the overload for `sockaddr_storage`?
Yeah, as discussed offline I debated doing this in this patch, but will instead just add the overload here to simplify backporting and have a follow up patch that removes the sockaddr_storage overload.
> On April 14, 2020, 7:14 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp
> > Line 1137 (original), 1133 (patched)
> > <https://reviews.apache.org/r/72348/diff/1/?file=2217424#file2217424line1138>
> >
> > Should we include the address in this failure message as well? Maybe it's not as relevant here?
> Maybe it's not as relevant here?
Right, I opted not to since this is a rather serious failure (can't even make an SSL object): no one can connect at all since all SSL_new calls fail, or we're out of memory, or maybe something else I'm not aware of. I think more important is logging why it's failing but having looked into that it's rather convoluted and independent of this patch.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72348/#review220312
-----------------------------------------------------------
On April 10, 2020, 7:30 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72348/
> -----------------------------------------------------------
>
> (Updated April 10, 2020, 7:30 p.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-10112
> https://issues.apache.org/jira/browse/MESOS-10112
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The caller of LibeventSSLSocket::accept() cannot see who tried to
> connect when accept fails, since the accepted socket is not returned.
> This adds logging of the peer address when the SSL handshake fails,
> in order to improve debugging.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp f23e653aa276e4806a88d8b54f84375ccbaad006
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 7bcc66fb3501d6c120bc13d2fd85bd98dbb5673a
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp dcb6d8e6c82005145c853afa9c24a61d7d0f04a9
>
>
> Diff: https://reviews.apache.org/r/72348/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 72348: Added logging of peer address in
LibeventSSLSocket accept failures.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72348/#review220312
-----------------------------------------------------------
3rdparty/libprocess/include/process/address.hpp
Lines 344-367 (original), 344-371 (patched)
<https://reviews.apache.org/r/72348/#comment308651>
Since `sockaddr_storage` can be cast as `sockaddr_{un,in,in6}`, maybe we want to combine these into a single function? Actually, after looking at some existing callsites of `Address::create()`, I wonder if we should eliminate the overload for `sockaddr_storage`?
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp
Line 1137 (original), 1133 (patched)
<https://reviews.apache.org/r/72348/#comment308650>
Should we include the address in this failure message as well? Maybe it's not as relevant here?
- Greg Mann
On April 10, 2020, 7:30 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72348/
> -----------------------------------------------------------
>
> (Updated April 10, 2020, 7:30 p.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-10112
> https://issues.apache.org/jira/browse/MESOS-10112
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The caller of LibeventSSLSocket::accept() cannot see who tried to
> connect when accept fails, since the accepted socket is not returned.
> This adds logging of the peer address when the SSL handshake fails,
> in order to improve debugging.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp f23e653aa276e4806a88d8b54f84375ccbaad006
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 7bcc66fb3501d6c120bc13d2fd85bd98dbb5673a
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp dcb6d8e6c82005145c853afa9c24a61d7d0f04a9
>
>
> Diff: https://reviews.apache.org/r/72348/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 72348: Added logging of peer address in
LibeventSSLSocket accept failures.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72348/#review220357
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On April 10, 2020, 7:30 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72348/
> -----------------------------------------------------------
>
> (Updated April 10, 2020, 7:30 p.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-10112
> https://issues.apache.org/jira/browse/MESOS-10112
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The caller of LibeventSSLSocket::accept() cannot see who tried to
> connect when accept fails, since the accepted socket is not returned.
> This adds logging of the peer address when the SSL handshake fails,
> in order to improve debugging.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp f23e653aa276e4806a88d8b54f84375ccbaad006
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 7bcc66fb3501d6c120bc13d2fd85bd98dbb5673a
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp dcb6d8e6c82005145c853afa9c24a61d7d0f04a9
>
>
> Diff: https://reviews.apache.org/r/72348/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>