-
Notifications
You must be signed in to change notification settings - Fork 5.5k
socket: IP_FREEBIND support for listeners and upstream connections. #2922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| # Freebind testing | ||
|
|
||
| To manually validate the `IP_FREEBIND` behavior in Envoy, you can launch Envoy with | ||
| [freebind.yaml](freebind.yaml). | ||
|
|
||
| The listener free bind behavior can be verified with: | ||
|
|
||
| 1. `envoy -c ./configs/freebind/freebind.yaml -l trace` | ||
| 2. `sudo ifconfig lo:1 192.168.42.1/30 up` | ||
| 3. `nc -v -l 0.0.0.0 10001` | ||
|
|
||
| To cleanup run `sudo ifconfig lo:1 down`. | ||
|
|
||
| TODO(htuch): Steps to verify upstream behavior. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| admin: | ||
| access_log_path: /tmp/admin_access.log | ||
| address: | ||
| socket_address: { address: 127.0.0.1, port_value: 9901 } | ||
|
|
||
| static_resources: | ||
| listeners: | ||
| - name: listener_0 | ||
| address: | ||
| socket_address: { address: 192.168.42.1, port_value: 10000 } | ||
| freebind: true | ||
| filter_chains: | ||
| - filters: | ||
| - name: envoy.http_connection_manager | ||
| config: | ||
| stat_prefix: ingress_http | ||
| route_config: | ||
| name: local_route | ||
| virtual_hosts: | ||
| - name: local_service | ||
| domains: ["*"] | ||
| routes: | ||
| - match: { prefix: "/" } | ||
| route: { cluster: service_local } | ||
| http_filters: | ||
| - name: envoy.router | ||
| clusters: | ||
| - name: service_local | ||
| connect_timeout: 30s | ||
| type: STATIC | ||
| lb_policy: ROUND_ROBIN | ||
| hosts: | ||
| - socket_address: | ||
| address: 127.0.0.1 | ||
| port_value: 10001 | ||
| # TODO(htuch): Figure out how to do end-to-end testing with | ||
| # outgoing connections and free bind. | ||
| # upstream_bind_config: | ||
| # source_address: | ||
| # address: 192.168.43.1 | ||
| # freebind: true | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,13 +55,14 @@ class Socket { | |
| */ | ||
| virtual void hashKey(std::vector<uint8_t>& key) const PURE; | ||
| }; | ||
| typedef std::unique_ptr<Option> OptionPtr; | ||
| typedef std::shared_ptr<std::vector<OptionPtr>> OptionsSharedPtr; | ||
| typedef std::shared_ptr<const Option> OptionConstSharedPtr; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you comment on why this needs to be a shared_ptr (not unique)? I skimmed the code but didn't see where/why they need to be shared.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1, why dos this need to be shared?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now an immutable object which benefits from shared const ownership; we do this already for address instances and some other cases.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see where you need the shared_ptr in the vector, the reason I didn't like was a shared_ptr of shared_ptr vector |
||
| typedef std::vector<OptionConstSharedPtr> Options; | ||
| typedef std::shared_ptr<Options> OptionsSharedPtr; | ||
|
|
||
| /** | ||
| * Add a socket option visitor for later retrieval with options(). | ||
| */ | ||
| virtual void addOption(OptionPtr&&) PURE; | ||
| virtual void addOption(const OptionConstSharedPtr&) PURE; | ||
|
|
||
| /** | ||
| * @return the socket options stored earlier with addOption() calls, if any. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| #include "common/network/socket_option_impl.h" | ||
|
|
||
| #include "envoy/common/exception.h" | ||
|
|
||
| #include "common/api/os_sys_calls_impl.h" | ||
| #include "common/common/assert.h" | ||
| #include "common/network/address_impl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Network { | ||
|
|
||
| bool SocketOptionImpl::setOption(Socket& socket, Socket::SocketState state) const { | ||
| if (state == Socket::SocketState::PreBind) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please also move the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will leave a TODO, I think this is orthogonal and that setsockopt doesn't cause any #ifdef tangle. So, in the interest of unblocking #2719 will skip for now. |
||
| if (freebind_.has_value()) { | ||
| const int should_freebind = freebind_.value() ? 1 : 0; | ||
| const int error = | ||
| setIpSocketOption(socket, ENVOY_SOCKET_IP_FREEBIND, ENVOY_SOCKET_IPV6_FREEBIND, | ||
| &should_freebind, sizeof(should_freebind)); | ||
| if (error != 0) { | ||
| ENVOY_LOG(warn, "Setting IP_FREEBIND on listener socket failed: {}", strerror(error)); | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| int SocketOptionImpl::setIpSocketOption(Socket& socket, SocketOptionName ipv4_optname, | ||
| SocketOptionName ipv6_optname, const void* optval, | ||
| socklen_t optlen) { | ||
| auto& os_syscalls = Api::OsSysCallsSingleton::get(); | ||
|
|
||
| // If this isn't IP, we're out of luck. | ||
| Address::InstanceConstSharedPtr address; | ||
| const Address::Ip* ip = nullptr; | ||
| try { | ||
| // We have local address when the socket is used in a listener but have to | ||
| // infer the IP from the socket FD when initiating connections. | ||
| // TODO(htuch): Figure out a way to obtain a consistent interface for IP | ||
| // version from socket. | ||
| if (socket.localAddress()) { | ||
| ip = socket.localAddress()->ip(); | ||
| } else { | ||
| address = Address::addressFromFd(socket.fd()); | ||
| ip = address->ip(); | ||
| } | ||
| } catch (const EnvoyException&) { | ||
| // Ignore, we get here because we failed in getsockname(). | ||
| // TODO(htuch): We should probably clean up this logic to avoid relying on exceptions. | ||
| } | ||
| if (ip == nullptr) { | ||
| ENVOY_LOG(warn, "Failed to set IP socket option on non-IP socket"); | ||
| return ENOTSUP; | ||
| } | ||
|
|
||
| // If the FD is v4, we can only try the IPv4 variant. | ||
| if (ip->version() == Network::Address::IpVersion::v4) { | ||
| if (!ipv4_optname) { | ||
| ENVOY_LOG(warn, "Unsupported IPv4 socket option"); | ||
| return ENOTSUP; | ||
| } | ||
| return os_syscalls.setsockopt(socket.fd(), IPPROTO_IP, ipv4_optname.value(), optval, optlen); | ||
| } | ||
|
|
||
| // If the FD is v6, we first try the IPv6 variant if the platfrom supports it and fallback to the | ||
| // IPv4 variant otherwise. | ||
| ASSERT(ip->version() == Network::Address::IpVersion::v6); | ||
| if (ipv6_optname) { | ||
| return os_syscalls.setsockopt(socket.fd(), IPPROTO_IPV6, ipv6_optname.value(), optval, optlen); | ||
| } | ||
| if (ipv4_optname) { | ||
| return os_syscalls.setsockopt(socket.fd(), IPPROTO_IP, ipv4_optname.value(), optval, optlen); | ||
| } | ||
| ENVOY_LOG(warn, "Unsupported IPv6 socket option"); | ||
| return ENOTSUP; | ||
| } | ||
|
|
||
| } // namespace Network | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| #pragma once | ||
|
|
||
| #include <netinet/in.h> | ||
| #include <netinet/ip.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; | ||
|
|
||
| #ifdef IP_FREEBIND | ||
| #define ENVOY_SOCKET_IP_FREEBIND Network::SocketOptionName(IP_FREEBIND) | ||
| #else | ||
| #define ENVOY_SOCKET_IP_FREEBIND Network::SocketOptionName() | ||
| #endif | ||
|
|
||
| #ifdef IPV6_FREEBIND | ||
| #define ENVOY_SOCKET_IPV6_FREEBIND Network::SocketOptionName(IPV6_FREEBIND) | ||
| #else | ||
| #define ENVOY_SOCKET_IPV6_FREEBIND Network::SocketOptionName() | ||
| #endif | ||
|
|
||
| class SocketOptionImpl : public Socket::Option, Logger::Loggable<Logger::Id::connection> { | ||
| public: | ||
| SocketOptionImpl(absl::optional<bool> freebind) : freebind_(freebind) {} | ||
|
|
||
| // Socket::Option | ||
| bool setOption(Socket& socket, Socket::SocketState state) const override; | ||
| // The common socket options don't require a hash key. | ||
| void hashKey(std::vector<uint8_t>&) const override {} | ||
|
|
||
| /** | ||
| * Set a socket option that applies at both IPv4 and IPv6 socket levels. When the underlying FD | ||
| * is IPv6, this function will attempt to set at IPv6 unless the platform only supports the | ||
| * option at the IPv4 level. | ||
| * @param socket. | ||
| * @param ipv4_optname SocketOptionName for IPv4 level. Set to empty if not supported on | ||
| * platform. | ||
| * @param ipv6_optname SocketOptionName for IPv6 level. Set to empty if not supported on | ||
| * platform. | ||
| * @param optval as per setsockopt(2). | ||
| * @param optlen as per setsockopt(2). | ||
| * @return int as per setsockopt(2). ENOTSUP is returned if the option is not supported on the | ||
| * platform for fd after the above option level fallback semantics are taken into account or the | ||
| * socket is non-IP. | ||
| */ | ||
| static int setIpSocketOption(Socket& socket, SocketOptionName ipv4_optname, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a non-actionable thought/comment: but I do feel like this series of changes is increasingly putting more UNIX/Linux stuff directly in the codebase and I wish we were thinking a bit more about x-platform. I would not worry about it now but just raising as food for thought.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this PR deals reasonably with the POSIXy side of the world. I agree that in general though, we would benefit from having something like What are the other non-POSIXy platforms we care about besides Windows?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Realistically I think only Windows right now. (I agree this is a very nice abstraction around POSIX socket options that might not exist on the host system.)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for platform work early while it's not too painful. |
||
| SocketOptionName ipv6_optname, const void* optval, socklen_t optlen); | ||
|
|
||
| private: | ||
| const absl::optional<bool> freebind_; | ||
| }; | ||
|
|
||
| } // 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.
Quick drive by: Can we test this config (just that it loads) in our config tests? Along with the original_dst config? From a quick look it doesn't seem like they are being tested for config sanity.
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'll take this as a follow up action item, I have a WiP to fix this, but it's a bit complicated because we're using a MockListenerComponentFactory, which is not compatible with socket options. I'd like to unblock #2719.
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.
OK SGTM.
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.
+1 - let's avoid text config because when it stops working we won't be alerted but a TODO is fine.
I think we do need to be able to test socket options, esp with all the other PRs in flight