socket: add SO_KEEPALIVE option for upstream connections. #3042
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| #include "common/network/tcp_keepalive_option_impl.h" | ||
|
|
||
| #include "common/api/os_sys_calls_impl.h" | ||
| #include "common/network/address_impl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Network { | ||
| bool TcpKeepaliveOptionImpl::setOption(Network::Socket& socket, | ||
| Network::Socket::SocketState state) const { | ||
| if (state == Socket::SocketState::PreBind) { | ||
| return setTcpKeepalive(socket, keepalive_config_.keepalive_probes_, | ||
| keepalive_config_.keepalive_time_, | ||
| keepalive_config_.keepalive_interval_); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| bool TcpKeepaliveOptionImpl::setTcpKeepalive(Socket& socket, absl::optional<int> keepalive_probes, | ||
| absl::optional<int> keepalive_time, | ||
| absl::optional<int> keepalive_interval) { | ||
| int error; | ||
| error = setSocketOption(socket, SOL_SOCKET, ENVOY_SOCKET_SO_KEEPALIVE, 1); | ||
|
Member
There was a problem hiding this comment. Q: does there need to be a way to configure it to explicitly disable the socket option, ie set it to a value of zero? I think it would only be needed if there's a way to make the default (at the OS level) enabled. Is that a thing we need to do?
Contributor
Author
There was a problem hiding this comment. Under Linux there's no way to turn it on at the OS level, only tweak the parameters. It appears that under FreeBSD and OSX it's possible to turn it on for everyone with "net.inet.tcp.always_keepalive", but I don't know if it's possible to explicitly disable it using SO_KEEPALIVE=0 in that case. |
||
| if (error != 0) { | ||
| ENVOY_LOG(warn, "Setting SO_KEEPALIVE on socket failed: {}", strerror(errno)); | ||
| return false; | ||
| } | ||
| error = setSocketOption(socket, IPPROTO_TCP, ENVOY_SOCKET_TCP_KEEPCNT, keepalive_probes); | ||
| if (error != 0) { | ||
| ENVOY_LOG(warn, "Setting keepalive_probes failed: {}", strerror(errno)); | ||
| return false; | ||
| } | ||
| error = setSocketOption(socket, IPPROTO_TCP, ENVOY_SOCKET_TCP_KEEPIDLE, keepalive_time); | ||
| if (error != 0) { | ||
| ENVOY_LOG(warn, "Setting keepalive_time failed: {}", strerror(errno)); | ||
| return false; | ||
| } | ||
| error = setSocketOption(socket, IPPROTO_TCP, ENVOY_SOCKET_TCP_KEEPINTVL, keepalive_interval); | ||
| if (error != 0) { | ||
| ENVOY_LOG(warn, "Setting keepalive_interval failed: {}", strerror(errno)); | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| int TcpKeepaliveOptionImpl::setSocketOption(Socket& socket, int level, | ||
| Network::SocketOptionName optname, | ||
| absl::optional<int> optional_value) { | ||
| if (optional_value.has_value()) { | ||
| return setSocketOption(socket, level, optname, optional_value.value()); | ||
| } else { | ||
| return 0; | ||
| } | ||
| } | ||
|
|
||
| int TcpKeepaliveOptionImpl::setSocketOption(Socket& socket, int level, | ||
| Network::SocketOptionName optname, int value) { | ||
| if (!optname.has_value()) { | ||
| errno = ENOTSUP; | ||
| return -1; | ||
| } | ||
| auto& os_syscalls = Api::OsSysCallsSingleton::get(); | ||
| return os_syscalls.setsockopt(socket.fd(), level, optname.value(), &value, sizeof(value)); | ||
|
Member
There was a problem hiding this comment. These options should all be of type/size int, not uint32_t. See http://man7.org/linux/man-pages/man7/tcp.7.html
Member
There was a problem hiding this comment. I feel like we're redoing https://github.com/envoyproxy/envoy/blob/master/source/common/network/socket_option_impl.h here. I realize these are TCP options, there is also #2793 where I've asked to add in some equivalent support for TCP options. Do you think it's reasonable to come up with some refactoring to avoid redoing how socket options are set, platform features are handled and the various branches tested?
Contributor
Author
There was a problem hiding this comment. I'm going to change SocketOptionName to be absl::optional<std::pair<int, int>>, so it can contain the "level" as well as "optname." That should make it possible to clean up the socket option code a bit and maybe make a common Socket::Option implementation, which takes a SocketOptionName and value as constructor args, possible. That might then allow TCP Keepalive, and other simple options, to be implemented with as a factory that produces a vector of options. SocketOptionImpl has special handling for checking if the socket it's being applied to is IP, and setting different options for IPV6 vs 4. I think that's going to have to remain slightly special (although it might be able to use the generic implementation under the hood.)
Member
There was a problem hiding this comment. Awesome. @bmetzdorf for visibility, so that you folks don't collide on this. I could merge #2793 as is if that works for you both.
Contributor
Author
There was a problem hiding this comment. Yeah, that works for me.
Contributor
There was a problem hiding this comment. SGTM, I can address the open aesthetic items from #2793 in another PR. |
||
| } | ||
|
|
||
| } // namespace Network | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| #pragma once | ||
|
|
||
| #include <netinet/in.h> | ||
| #include <netinet/ip.h> | ||
| #include <netinet/tcp.h> | ||
| #include <sys/socket.h> | ||
|
|
||
| #include "envoy/network/listen_socket.h" | ||
|
|
||
| #include "common/common/logger.h" | ||
|
|
||
| #include "absl/types/optional.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Network { | ||
|
|
||
| // Optional variant of setsockopt(2) optname. The idea here is that if the option is not supported | ||
| // on a platform, we can make this the empty value. This allows us to avoid proliferation of #ifdef. | ||
| typedef absl::optional<int> SocketOptionName; | ||
|
Member
There was a problem hiding this comment. This should go in a common header somewhere, instead of copy/pasting it. I think it might be nice to put all of the socket option preprocessor checking into a single header file. Or better yet, put the declarations in a header, and the definitions in a .cc so all the system headers for the constants don't need to be included in large parts of the codebase. Thoughts on that idea @htuch ?
Contributor
Author
There was a problem hiding this comment. I was considering making this an absl::optional<pair<int, int>> so that it can contain the "level" as well as "optname" value. I think I'd definitely want to do that if I make these a single set of definitions, which I agree might be cleaner. |
||
|
|
||
| #ifdef SO_KEEPALIVE | ||
| #define ENVOY_SOCKET_SO_KEEPALIVE Network::SocketOptionName(SO_KEEPALIVE) | ||
| #else | ||
| #define ENVOY_SOCKET_SO_KEEPALIVE Network::SocketOptionName() | ||
| #endif | ||
|
|
||
| #ifdef TCP_KEEPCNT | ||
| #define ENVOY_SOCKET_TCP_KEEPCNT Network::SocketOptionName(TCP_KEEPCNT) | ||
| #else | ||
| #define ENVOY_SOCKET_TCP_KEEPCNT Network::SocketOptionName() | ||
| #endif | ||
|
|
||
| #ifdef TCP_KEEPIDLE | ||
| #define ENVOY_SOCKET_TCP_KEEPIDLE Network::SocketOptionName(TCP_KEEPIDLE) | ||
| #elif TCP_KEEPALIVE // MacOS uses a different name from Linux for just this option. | ||
| #define ENVOY_SOCKET_TCP_KEEPIDLE Network::SocketOptionName(TCP_KEEPALIVE) | ||
| #else | ||
| #define ENVOY_SOCKET_TCP_KEEPIDLE Network::SocketOptionName() | ||
| #endif | ||
|
|
||
| #ifdef TCP_KEEPINTVL | ||
| #define ENVOY_SOCKET_TCP_KEEPINTVL Network::SocketOptionName(TCP_KEEPINTVL) | ||
| #else | ||
| #define ENVOY_SOCKET_TCP_KEEPINTVL Network::SocketOptionName() | ||
| #endif | ||
|
|
||
| class TcpKeepaliveOptionImpl : public Socket::Option, Logger::Loggable<Logger::Id::connection> { | ||
| public: | ||
| TcpKeepaliveOptionImpl(Network::TcpKeepaliveConfig keepalive_config) | ||
| : keepalive_config_(keepalive_config) {} | ||
|
|
||
| // Socket::Option | ||
| bool setOption(Socket& socket, Socket::SocketState state) const override; | ||
|
|
||
| // The tcp keepalive options don't require a hash key. | ||
| void hashKey(std::vector<uint8_t>&) const override {} | ||
|
|
||
| static bool setTcpKeepalive(Socket& socket, absl::optional<int> keepalive_probes, | ||
| absl::optional<int> keepalive_time, | ||
| absl::optional<int> keepalive_interval); | ||
|
|
||
| private: | ||
| Network::TcpKeepaliveConfig keepalive_config_; | ||
| static int setSocketOption(Socket& socket, int level, Network::SocketOptionName optname, | ||
| absl::optional<int> value); | ||
| static int setSocketOption(Socket& socket, int level, Network::SocketOptionName optname, | ||
| int value); | ||
| }; | ||
| } // namespace Network | ||
| } // namespace Envoy | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document the macos behavior. It looks like you added support for the right socket option, but please document that the other two must be unset in the config on macos.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I swear I replied to this, but the comment seems to be missing now.
The reason for the single special case is that MacOS uses a different name for that one constant (TCP_KEEPIDLE vs TCP_KEEPALIVE.) The other constants all have the same name as for Linux. All of these settings are valid for both Linux and MacOS. I've added a comment to that effect in the header where they're defined.