From 0509ee3e367011677d96c1ae65c9e5937eea655f Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Thu, 3 Jan 2019 17:05:27 -0800 Subject: [PATCH 01/32] Introducing UDP listener. Signed-off-by: Jojy G Varghese --- include/envoy/buffer/buffer.h | 3 + include/envoy/event/dispatcher.h | 10 ++ include/envoy/network/listener.h | 32 ++++ source/common/buffer/buffer_impl.cc | 25 +++ source/common/buffer/buffer_impl.h | 2 + source/common/event/dispatcher_impl.cc | 8 + source/common/event/dispatcher_impl.h | 4 +- source/common/network/BUILD | 9 +- source/common/network/base_listener_impl.cc | 35 ++++ source/common/network/base_listener_impl.h | 30 ++++ source/common/network/listener_impl.cc | 48 +++--- source/common/network/listener_impl.h | 24 +-- source/common/network/udp_listener_impl.cc | 119 +++++++++++++ source/common/network/udp_listener_impl.h | 31 ++++ .../network/dubbo_proxy/buffer_helper.h | 3 + test/common/network/listener_impl_test.cc | 159 +++++++++++++++++- test/mocks/event/mocks.h | 8 + test/mocks/network/mocks.cc | 3 + test/mocks/network/mocks.h | 25 +++ 19 files changed, 532 insertions(+), 46 deletions(-) create mode 100644 source/common/network/base_listener_impl.cc create mode 100644 source/common/network/base_listener_impl.h create mode 100644 source/common/network/udp_listener_impl.cc create mode 100644 source/common/network/udp_listener_impl.h diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index 1bc200839b82a..94fd167b9e1eb 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -168,6 +168,9 @@ class Instance { */ virtual Api::SysCallIntResult read(int fd, uint64_t max_length) PURE; + virtual Api::SysCallIntResult recvFrom(int fd, uint64_t max_length, + sockaddr_storage& peer_address, + socklen_t& sock_length) PURE; /** * Reserve space in the buffer. * @param length supplies the amount of space to reserve. diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 9802ce3c64e82..5f89bbcd04518 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -110,6 +110,16 @@ class Dispatcher { Network::ListenerCallbacks& cb, bool bind_to_port, bool hand_off_restored_destination_connections) PURE; + /** + * Create a logical udp listener on a specific port. + * @param socket supplies the socket to listen on. + * @param cb supplies the udp listener callbacks to invoke for listener events. + * @param bind_to_port controls whether the listener binds to a transport port or not. + * @return Network::ListenerPtr a new listener that is owned by the caller. + */ + virtual Network::ListenerPtr createUdpListener(Network::Socket& socket, + Network::UdpListenerCallbacks& cb, + bool bind_to_port) PURE; /** * Allocate a timer. @see Timer for docs on how to use the timer. * @param cb supplies the callback to invoke when the timer fires. diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index ae34c3151117f..5933211f7b6be 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -114,6 +114,38 @@ class ListenerCallbacks { virtual void onNewConnection(ConnectionPtr&& new_connection) PURE; }; +/** + * UDP listener callbacks. + */ +class UdpListenerCallbacks { +public: + virtual ~UdpListenerCallbacks() = default; + + // TODO(conqerAtapple): Refactor `Connection` to accommodate UDP/QUIC. + + /** + * On first packet received on a UDP socket. This allows the callback handler + * to establish filter chain (or any other prepararion). + * + * @param local_address Local bound socket network address. + * @param peer_address Network address of the peer. + * @param data Data buffer received. + */ + virtual void onNewConnection(Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address, + Buffer::InstancePtr&& data) PURE; + /** + * Called whenever data is received by the underlying Udp socket. + * + * @param local_address Local bound socket network address. + * @param peer_address Network address of the peer. + * @param data Data buffer received. + */ + virtual void onData(Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address, + Buffer::InstancePtr&& data) PURE; +}; + /** * An abstract socket listener. Free the listener to stop listening on the socket. */ diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 192920f6b0575..cf5a3be3a998c 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -110,6 +110,31 @@ void OwnedImpl::move(Instance& rhs, uint64_t length) { static_cast(rhs).postProcess(); } +Api::SysCallIntResult OwnedImpl::recvFrom(int fd, uint64_t max_length, sockaddr_storage& peer_addr, + socklen_t& addr_len) { + if (max_length == 0) { + return {0, 0}; + } + + memset(&peer_addr, 0, sizeof(sockaddr_storage)); + + RawSlice slice; + const uint64_t num_slices = reserve(max_length, &slice, 1); + + ASSERT(num_slices == 1); + // TODO(conqerAtapple): Use os_syscalls + const ssize_t rc = ::recvfrom(fd, slice.mem_, max_length, 0, + reinterpret_cast(&peer_addr), &addr_len); + if (rc < 0) { + return {static_cast(rc), errno}; + } + + slice.len_ = std::min(slice.len_, static_cast(rc)); + commit(&slice, 1); + + return {static_cast(rc), errno}; +} + Api::SysCallIntResult OwnedImpl::read(int fd, uint64_t max_length) { if (max_length == 0) { return {0, 0}; diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index e215d53ea81c8..f068aabf3dc21 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -83,6 +83,8 @@ class OwnedImpl : public LibEventInstance { void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; Api::SysCallIntResult read(int fd, uint64_t max_length) override; + Api::SysCallIntResult recvFrom(int fd, uint64_t max_length, sockaddr_storage& peer_address, + socklen_t& sock_length) override; uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; ssize_t search(const void* data, uint64_t size, size_t start) const override; Api::SysCallIntResult write(int fd) override; diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index be1134964e91c..d7b55b5ae820b 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -19,6 +19,7 @@ #include "common/network/connection_impl.h" #include "common/network/dns_impl.h" #include "common/network/listener_impl.h" +#include "common/network/udp_listener_impl.h" #include "event2/event.h" @@ -119,6 +120,13 @@ DispatcherImpl::createListener(Network::Socket& socket, Network::ListenerCallbac hand_off_restored_destination_connections)}; } +Network::ListenerPtr DispatcherImpl::createUdpListener(Network::Socket& socket, + Network::UdpListenerCallbacks& cb, + bool bind_to_port) { + ASSERT(isThreadSafe()); + return Network::ListenerPtr{new Network::UdpListenerImpl(*this, socket, cb, bind_to_port)}; +} + TimerPtr DispatcherImpl::createTimer(TimerCb cb) { ASSERT(isThreadSafe()); return scheduler_->createTimer(cb); diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index f8edabdc72185..24581a335eb7c 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -30,7 +30,7 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { /** * @return event_base& the libevent base. */ - event_base& base() { return *base_; } + event_base& base() const { return *base_; } // Event::Dispatcher TimeSystem& timeSystem() override { return time_system_; } @@ -51,6 +51,8 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { Network::ListenerPtr createListener(Network::Socket& socket, Network::ListenerCallbacks& cb, bool bind_to_port, bool hand_off_restored_destination_connections) override; + Network::ListenerPtr createUdpListener(Network::Socket& socket, Network::UdpListenerCallbacks& cb, + bool bind_to_port) override; TimerPtr createTimer(TimerCb cb) override; void deferredDelete(DeferredDeletablePtr&& to_delete) override; void exit() override; diff --git a/source/common/network/BUILD b/source/common/network/BUILD index 6d39b662a7400..5f356c4f73887 100644 --- a/source/common/network/BUILD +++ b/source/common/network/BUILD @@ -139,9 +139,15 @@ envoy_cc_library( envoy_cc_library( name = "listener_lib", srcs = [ + "base_listener_impl.cc", "listener_impl.cc", + "udp_listener_impl.cc", + ], + hdrs = [ + "base_listener_impl.h", + "listener_impl.h", + "udp_listener_impl.h", ], - hdrs = ["listener_impl.h"], deps = [ ":address_lib", ":io_socket_handle_lib", @@ -151,6 +157,7 @@ envoy_cc_library( "//include/envoy/network:listener_interface", "//include/envoy/stats:stats_interface", "//include/envoy/stats:stats_macros", + "//source/common/buffer:buffer_lib", "//source/common/common:assert_lib", "//source/common/common:empty_string", "//source/common/common:linked_object", diff --git a/source/common/network/base_listener_impl.cc b/source/common/network/base_listener_impl.cc new file mode 100644 index 0000000000000..5b20e83aa073e --- /dev/null +++ b/source/common/network/base_listener_impl.cc @@ -0,0 +1,35 @@ +#include "common/network/base_listener_impl.h" + +#include + +#include "envoy/common/exception.h" + +#include "common/common/assert.h" +#include "common/common/empty_string.h" +#include "common/common/fmt.h" +#include "common/event/dispatcher_impl.h" +#include "common/event/file_event_impl.h" +#include "common/network/address_impl.h" + +#include "event2/listener.h" + +namespace Envoy { +namespace Network { + +Address::InstanceConstSharedPtr BaseListenerImpl::getLocalAddress(int fd) { + return Address::addressFromFd(fd); +} + +BaseListenerImpl::BaseListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket) + : local_address_(nullptr), dispatcher_(dispatcher), socket_(socket) { + const auto ip = socket.localAddress()->ip(); + + // Only use the listen socket's local address for new connections if it is not the all hosts + // address (e.g., 0.0.0.0 for IPv4). + if (!(ip && ip->isAnyAddress())) { + local_address_ = socket.localAddress(); + } +} + +} // namespace Network +} // namespace Envoy diff --git a/source/common/network/base_listener_impl.h b/source/common/network/base_listener_impl.h new file mode 100644 index 0000000000000..8188c01cefc09 --- /dev/null +++ b/source/common/network/base_listener_impl.h @@ -0,0 +1,30 @@ +#pragma once + +#include "envoy/network/listener.h" + +#include "common/event/dispatcher_impl.h" +#include "common/event/libevent.h" +#include "common/network/listen_socket_impl.h" + +#include "event2/event.h" + +namespace Envoy { +namespace Network { + +/** + * Base libevent implementation of Network::Listener. + */ +class BaseListenerImpl : public Listener { +public: + BaseListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket); + +protected: + virtual Address::InstanceConstSharedPtr getLocalAddress(int fd); + + Address::InstanceConstSharedPtr local_address_; + const Event::DispatcherImpl& dispatcher_; + Socket& socket_; +}; + +} // namespace Network +} // namespace Envoy diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index 4493f4f790fc5..5b6fcce6d5704 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -17,10 +17,6 @@ namespace Envoy { namespace Network { -Address::InstanceConstSharedPtr ListenerImpl::getLocalAddress(int fd) { - return Address::addressFromFd(fd); -} - void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* remote_addr, int remote_addr_len, void* arg) { ListenerImpl* listener = static_cast(arg); @@ -51,35 +47,31 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* listener->hand_off_restored_destination_connections_); } -ListenerImpl::ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb, - bool bind_to_port, bool hand_off_restored_destination_connections) - : local_address_(nullptr), cb_(cb), - hand_off_restored_destination_connections_(hand_off_restored_destination_connections), - listener_(nullptr) { - const auto ip = socket.localAddress()->ip(); +void ListenerImpl::setupServerSocket(const Event::DispatcherImpl& dispatcher, Socket& socket) { + listener_.reset(evconnlistener_new(&dispatcher.base(), listenCallback, this, 0, -1, socket.fd())); - // Only use the listen socket's local address for new connections if it is not the all hosts - // address (e.g., 0.0.0.0 for IPv4). - if (!(ip && ip->isAnyAddress())) { - local_address_ = socket.localAddress(); + if (!listener_) { + throw CreateListenerException( + fmt::format("cannot listen on socket: {}", socket.localAddress()->asString())); } - if (bind_to_port) { - listener_.reset(evconnlistener_new(&dispatcher.base(), listenCallback, this, 0, -1, - socket.ioHandle().fd())); - - if (!listener_) { - throw CreateListenerException( - fmt::format("cannot listen on socket: {}", socket.localAddress()->asString())); - } + if (!Network::Socket::applyOptions(socket.options(), socket, + envoy::api::v2::core::SocketOption::STATE_LISTENING)) { + throw CreateListenerException(fmt::format("cannot set post-listen socket option on socket: {}", + socket.localAddress()->asString())); + } - if (!Network::Socket::applyOptions(socket.options(), socket, - envoy::api::v2::core::SocketOption::STATE_LISTENING)) { - throw CreateListenerException(fmt::format( - "cannot set post-listen socket option on socket: {}", socket.localAddress()->asString())); - } + evconnlistener_set_error_cb(listener_.get(), errorCallback); +} - evconnlistener_set_error_cb(listener_.get(), errorCallback); +ListenerImpl::ListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, + ListenerCallbacks& cb, bool bind_to_port, + bool hand_off_restored_destination_connections) + : BaseListenerImpl(dispatcher, socket), cb_(cb), + hand_off_restored_destination_connections_(hand_off_restored_destination_connections), + listener_(nullptr) { + if (bind_to_port) { + setupServerSocket(dispatcher, socket); } } diff --git a/source/common/network/listener_impl.h b/source/common/network/listener_impl.h index 26dff27ce24f7..3fd7931609422 100644 --- a/source/common/network/listener_impl.h +++ b/source/common/network/listener_impl.h @@ -1,38 +1,32 @@ #pragma once -#include "envoy/network/listener.h" - -#include "common/event/dispatcher_impl.h" -#include "common/event/libevent.h" -#include "common/network/listen_socket_impl.h" - -#include "event2/event.h" +#include "base_listener_impl.h" namespace Envoy { namespace Network { /** - * libevent implementation of Network::Listener. + * libevent implementation of Network::Listener for TCP. + * TODO(conqerAtapple): Consider renaming the class to `TcpListenerImpl`. */ -class ListenerImpl : public Listener { +class ListenerImpl : public BaseListenerImpl { public: - ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb, + ListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb, bool bind_to_port, bool hand_off_restored_destination_connections); - void disable(); - void enable(); + void disable() override; + void enable() override; protected: - virtual Address::InstanceConstSharedPtr getLocalAddress(int fd); + void setupServerSocket(const Event::DispatcherImpl& dispatcher, Socket& socket); - Address::InstanceConstSharedPtr local_address_; ListenerCallbacks& cb_; const bool hand_off_restored_destination_connections_; private: - static void errorCallback(evconnlistener* listener, void* context); static void listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* remote_addr, int remote_addr_len, void* arg); + static void errorCallback(evconnlistener* listener, void* context); Event::Libevent::ListenerPtr listener_; }; diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc new file mode 100644 index 0000000000000..d6cdedbd759c9 --- /dev/null +++ b/source/common/network/udp_listener_impl.cc @@ -0,0 +1,119 @@ +#include "common/network/udp_listener_impl.h" + +#include + +#include + +#include "envoy/buffer/buffer.h" +#include "envoy/common/exception.h" + +#include "common/buffer/buffer_impl.h" +#include "common/common/assert.h" +#include "common/common/empty_string.h" +#include "common/common/fmt.h" +#include "common/event/dispatcher_impl.h" +#include "common/network/address_impl.h" + +#include "event2/listener.h" + +namespace Envoy { +namespace Network { + +UdpListenerImpl::UdpListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, + UdpListenerCallbacks& cb, bool bind_to_port) + : BaseListenerImpl(dispatcher, socket), cb_(cb), is_first_(true) { + if (bind_to_port) { + event_assign(&raw_event_, &dispatcher.base(), socket.fd(), EV_READ | EV_PERSIST, readCallback, + this); + event_add(&raw_event_, nullptr); + + if (!Network::Socket::applyOptions(socket.options(), socket, + envoy::api::v2::core::SocketOption::STATE_BOUND)) { + throw CreateListenerException(fmt::format("cannot set post-bound socket option on socket: {}", + socket.localAddress()->asString())); + } + } +} + +void UdpListenerImpl::disable() { event_del(&raw_event_); } + +void UdpListenerImpl::enable() { event_add(&raw_event_, nullptr); } + +void UdpListenerImpl::readCallback(int fd, short flags, void* arg) { + (void)flags; + + UdpListenerImpl* instance = static_cast(arg); + ASSERT(instance); + + // TODO(conqerAtAppple): Make this configurable or get from system. + constexpr uint64_t const read_length = 16384; + Buffer::InstancePtr buffer(new Buffer::OwnedImpl()); + sockaddr_storage addr; + socklen_t addr_len; + + Api::SysCallIntResult result = buffer->recvFrom(fd, read_length, addr, addr_len); + if (result.rc_ < 0) { + // TODO(conqerAtApple): Call error callback. + RELEASE_ASSERT(false, fmt::format("recvfrom returned rc: {}, error: {}", result.rc_, + strerror(result.errno_))); + } + + Address::InstanceConstSharedPtr local_address = instance->socket_.localAddress(); + RELEASE_ASSERT( + addr_len, + fmt::format( + "Unable to get remote address for fd: {}, local address: {}. address length is 0 ", fd, + local_address->asString())); + + Address::InstanceConstSharedPtr peer_address; + + // TODO(conqerAtApple): Current implementation of Address::addressFromSockAddr + // cannot be used here unfortunately. This should belong in Address namespace. + switch (addr.ss_family) { + case AF_INET: { + const struct sockaddr_in* sin = reinterpret_cast(&addr); + ASSERT(AF_INET == sin->sin_family); + peer_address = std::make_shared(sin); + break; + } + case AF_INET6: { + const struct sockaddr_in6* sin6 = reinterpret_cast(&addr); + ASSERT(AF_INET6 == sin6->sin6_family); + if (IN6_IS_ADDR_V4MAPPED(&sin6->sin6_addr)) { +#if defined(__APPLE__) + struct sockaddr_in sin = { + {}, AF_INET, sin6->sin6_port, {sin6->sin6_addr.__u6_addr.__u6_addr32[3]}, {}}; +#else + struct sockaddr_in sin = {AF_INET, sin6->sin6_port, {sin6->sin6_addr.s6_addr32[3]}, {}}; +#endif + peer_address = std::make_shared(&sin); + } else { + peer_address = std::make_shared(*sin6, true); + } + } + + break; + default: + RELEASE_ASSERT(false, + fmt::format("Unsupported address family: {}, local address: {}, receive size: " + "{}, address length: {}", + addr.ss_family, local_address->asString(), result.rc_, addr_len)); + } + + RELEASE_ASSERT((peer_address != nullptr), + fmt::format("Unable to get remote address for fd: {}, local address: {} ", fd, + local_address->asString())); + + RELEASE_ASSERT((local_address != nullptr), + fmt::format("Unable to get local address for fd: {}", fd)); + + bool expected = true; + if (instance->is_first_.compare_exchange_strong(expected, false)) { + instance->cb_.onNewConnection(local_address, peer_address, std::move(buffer)); + } else { + instance->cb_.onData(local_address, peer_address, std::move(buffer)); + } +} + +} // namespace Network +} // namespace Envoy diff --git a/source/common/network/udp_listener_impl.h b/source/common/network/udp_listener_impl.h new file mode 100644 index 0000000000000..61382c5cbfd0c --- /dev/null +++ b/source/common/network/udp_listener_impl.h @@ -0,0 +1,31 @@ +#pragma once + +#include "common/event/event_impl_base.h" + +#include "base_listener_impl.h" + +namespace Envoy { +namespace Network { + +/** + * libevent implementation of Network::Listener for UDP. + */ +class UdpListenerImpl : public BaseListenerImpl, public Event::ImplBase { +public: + UdpListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb, + bool bind_to_port); + + virtual void disable() override; + virtual void enable() override; + +protected: + UdpListenerCallbacks& cb_; + +private: + static void readCallback(int fd, short flags, void* arg); + + std::atomic is_first_; +}; + +} // namespace Network +} // namespace Envoy diff --git a/source/extensions/filters/network/dubbo_proxy/buffer_helper.h b/source/extensions/filters/network/dubbo_proxy/buffer_helper.h index 8b78cc7ff2875..a826f8383bf56 100644 --- a/source/extensions/filters/network/dubbo_proxy/buffer_helper.h +++ b/source/extensions/filters/network/dubbo_proxy/buffer_helper.h @@ -53,6 +53,9 @@ class BufferWrapper : public Buffer::Instance { void move(Buffer::Instance&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } void move(Buffer::Instance&, uint64_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } Api::SysCallIntResult read(int, uint64_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + Api::SysCallIntResult recvFrom(int, uint64_t, sockaddr_storage&, socklen_t&) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } uint64_t reserve(uint64_t, Buffer::RawSlice*, uint64_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 8c1933876e54a..c718faf6b1e27 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -1,5 +1,9 @@ +#include +#include + #include "common/network/address_impl.h" #include "common/network/listener_impl.h" +#include "common/network/udp_listener_impl.h" #include "common/network/utility.h" #include "test/mocks/network/mocks.h" @@ -73,6 +77,15 @@ class TestListenerImpl : public ListenerImpl { MOCK_METHOD1(getLocalAddress, Address::InstanceConstSharedPtr(int fd)); }; +class TestUdpListenerImpl : public UdpListenerImpl { +public: + TestUdpListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb, + bool bind_to_port) + : UdpListenerImpl(dispatcher, socket, cb, bind_to_port) {} + + MOCK_METHOD1(getLocalAddress, Address::InstanceConstSharedPtr(int fd)); +}; + class ListenerImplTest : public testing::TestWithParam { protected: ListenerImplTest() @@ -81,6 +94,63 @@ class ListenerImplTest : public testing::TestWithParam { Network::Test::getCanonicalLoopbackAddress(version_), Address::SocketType::Stream)), api_(Api::createApiForTest(stats_store_)), dispatcher_(test_time_.timeSystem(), *api_) {} + SocketPtr getSocket(Address::SocketType type, const Address::InstanceConstSharedPtr& address, + const Network::Socket::OptionsSharedPtr& options, bool bind) { + if (type == Address::SocketType::Stream) { + using NetworkSocketTraitType = NetworkSocketTrait; + return std::make_unique>(address, options, bind); + } else if (type == Address::SocketType::Datagram) { + using NetworkSocketTraitType = NetworkSocketTrait; + return std::make_unique>(address, options, bind); + } + + return nullptr; + } + + // TODO(conqerAtapple): Move this to a common place(address.h?) + void getSocketAddressInfo(const Socket& socket, sockaddr_storage& addr, socklen_t& sz) { + memset(&addr, 0, sizeof(addr)); + + auto const* ip = socket.localAddress()->ip(); + if (!ip) { + sz = 0; + return; + } + + if (version_ == Address::IpVersion::v4) { + addr.ss_family = AF_INET; + auto const* ipv4 = ip->ipv4(); + if (!ipv4) { + sz = 0; + return; + } + + sockaddr_in* addrv4 = reinterpret_cast(&addr); + addrv4->sin_port = htons(ip->port()); + addrv4->sin_addr.s_addr = ipv4->address(); + + sz = sizeof(sockaddr_in); + } else if (version_ == Address::IpVersion::v6) { + addr.ss_family = AF_INET6; + auto const* ipv6 = ip->ipv6(); + if (!ipv6) { + sz = 0; + return; + } + + sockaddr_in6* addrv6 = reinterpret_cast(&addr); + addrv6->sin6_port = htons(ip->port()); + + const auto address = ipv6->address(); + memcpy(static_cast(&addrv6->sin6_addr.s6_addr), static_cast(&address), + sizeof(absl::uint128)); + + sz = sizeof(sockaddr_in6); + } else { + sz = 0; + } + } + const Address::IpVersion version_; const Address::InstanceConstSharedPtr alt_address_; Stats::IsolatedStoreImpl stats_store_; @@ -134,7 +204,7 @@ TEST_P(ListenerImplTest, SetListeningSocketOptionsError) { socket.localAddress()->asString())); } -TEST_P(ListenerImplTest, UseActualDst) { +TEST_P(ListenerImplTest, UseActualDstTcp) { Stats::IsolatedStoreImpl stats_store; Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); @@ -171,6 +241,93 @@ TEST_P(ListenerImplTest, UseActualDst) { dispatcher_.run(Event::Dispatcher::RunType::Block); } +TEST_P(ListenerImplTest, UseActualDstUdp) { + SocketPtr server_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, true); + + ASSERT_NE(server_socket, nullptr); + + auto const* server_ip = server_socket->localAddress()->ip(); + ASSERT_NE(server_ip, nullptr); + + Network::MockUdpListenerCallbacks listener_callbacks; + Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks, + true); + + SocketPtr client_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, false); + + const int client_sockfd = client_socket->fd(); + sockaddr_storage server_addr; + socklen_t addr_len; + + getSocketAddressInfo(*client_socket.get(), server_addr, addr_len); + ASSERT_GT(addr_len, 0); + + if (version_ == Address::IpVersion::v4) { + struct sockaddr_in* servaddr = reinterpret_cast(&server_addr); + servaddr->sin_port = htons(server_ip->port()); + } else if (version_ == Address::IpVersion::v6) { + struct sockaddr_in6* servaddr = reinterpret_cast(&server_addr); + servaddr->sin6_port = htons(server_ip->port()); + } + + const std::string first("first"); + const std::string second("second"); + + auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, first.length()); + + send_rc = ::sendto(client_sockfd, second.c_str(), second.length(), MSG_CONFIRM, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, second.length()); + + EXPECT_CALL(listener_callbacks, onNewConnection_(_, _, _)) + .WillOnce( + Invoke([&](Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { + ASSERT_NE(local_address, nullptr); + + ASSERT_NE(peer_address, nullptr); + ASSERT_NE(peer_address->ip(), nullptr); + + ASSERT_EQ(local_address->asString(), server_socket->localAddress()->asString()); + + ASSERT_EQ(peer_address->ip()->addressAsString(), + client_socket->localAddress()->ip()->addressAsString()); + + EXPECT_EQ(*local_address, *server_socket->localAddress()); + ASSERT_EQ(data->toString(), first); + })); + + EXPECT_CALL(listener_callbacks, onData_(_, _, _)) + .WillOnce( + Invoke([&](Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { + ASSERT_NE(local_address, nullptr); + + ASSERT_NE(peer_address, nullptr); + ASSERT_NE(peer_address->ip(), nullptr); + + ASSERT_EQ(local_address->asString(), server_socket->localAddress()->asString()); + + ASSERT_EQ(peer_address->ip()->addressAsString(), + client_socket->localAddress()->ip()->addressAsString()); + + EXPECT_EQ(*local_address, *server_socket->localAddress()); + ASSERT_EQ(data->toString(), second); + + dispatcher_.exit(); + })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); +} + TEST_P(ListenerImplTest, WildcardListenerUseActualDst) { Stats::IsolatedStoreImpl stats_store; Network::TcpListenSocket socket(Network::Test::getAnyAddress(version_), nullptr, true); diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 6f11b90b1cc56..d92b39fb80b94 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -65,6 +65,11 @@ class MockDispatcher : public Dispatcher { createListener_(socket, cb, bind_to_port, hand_off_restored_destination_connections)}; } + Network::ListenerPtr createUdpListener(Network::Socket& socket, Network::UdpListenerCallbacks& cb, + bool bind_to_port) override { + return Network::ListenerPtr{createUdpListener_(socket, cb, bind_to_port)}; + } + Event::TimerPtr createTimer(Event::TimerCb cb) override { return Event::TimerPtr{createTimer_(cb)}; } @@ -101,6 +106,9 @@ class MockDispatcher : public Dispatcher { Network::Listener*(Network::Socket& socket, Network::ListenerCallbacks& cb, bool bind_to_port, bool hand_off_restored_destination_connections)); + MOCK_METHOD3(createUdpListener_, + Network::Listener*(Network::Socket& socket, Network::UdpListenerCallbacks& cb, + bool bind_to_port)); MOCK_METHOD1(createTimer_, Timer*(Event::TimerCb cb)); MOCK_METHOD1(deferredDelete_, void(DeferredDeletable* to_delete)); MOCK_METHOD0(exit, void()); diff --git a/test/mocks/network/mocks.cc b/test/mocks/network/mocks.cc index c91d083ef175f..538c5c36cd312 100644 --- a/test/mocks/network/mocks.cc +++ b/test/mocks/network/mocks.cc @@ -78,6 +78,9 @@ MockFilter::~MockFilter() {} MockListenerCallbacks::MockListenerCallbacks() {} MockListenerCallbacks::~MockListenerCallbacks() {} +MockUdpListenerCallbacks::MockUdpListenerCallbacks() {} +MockUdpListenerCallbacks::~MockUdpListenerCallbacks() {} + MockDrainDecision::MockDrainDecision() {} MockDrainDecision::~MockDrainDecision() {} diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index d0dc937cea47b..41411c44e6a88 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -117,6 +117,30 @@ class MockListenerCallbacks : public ListenerCallbacks { MOCK_METHOD1(onNewConnection_, void(ConnectionPtr& conn)); }; +class MockUdpListenerCallbacks : public UdpListenerCallbacks { +public: + MockUdpListenerCallbacks(); + ~MockUdpListenerCallbacks(); + + void onNewConnection(Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address, + Buffer::InstancePtr&& data) override { + onNewConnection_(local_address, peer_address, data.get()); + } + + void onData(Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address, Buffer::InstancePtr&& data) override { + onData_(local_address, peer_address, data.get()); + } + + MOCK_METHOD3(onNewConnection_, + void(Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data)); + + MOCK_METHOD3(onData_, void(Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data)); +}; + class MockDrainDecision : public DrainDecision { public: MockDrainDecision(); @@ -295,6 +319,7 @@ class MockConnectionHandler : public ConnectionHandler { MOCK_METHOD0(numConnections, uint64_t()); MOCK_METHOD1(addListener, void(ListenerConfig& config)); + MOCK_METHOD1(addUdpListener, void(ListenerConfig& config)); MOCK_METHOD1(findListenerByAddress, Network::Listener*(const Network::Address::Instance& address)); MOCK_METHOD1(removeListeners, void(uint64_t listener_tag)); From eca712d7a5b5683a49bbe06ebe4cdb043138c233 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Thu, 3 Jan 2019 21:30:39 -0800 Subject: [PATCH 02/32] Removed MSG_CONFIRM flag for sendto method. Signed-off-by: Jojy G Varghese --- test/common/network/listener_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index c718faf6b1e27..996ed5558bf12 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -282,7 +282,7 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { ASSERT_EQ(send_rc, first.length()); - send_rc = ::sendto(client_sockfd, second.c_str(), second.length(), MSG_CONFIRM, + send_rc = ::sendto(client_sockfd, second.c_str(), second.length(), 0, reinterpret_cast(&server_addr), addr_len); ASSERT_EQ(send_rc, second.length()); From 37232e4ada4e5e8027a9ff8be7ceb38438dfda73 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Fri, 4 Jan 2019 00:04:34 -0800 Subject: [PATCH 03/32] Added more tests for UDP listener. Signed-off-by: Jojy G Varghese --- test/common/network/listener_impl_test.cc | 144 +++++++++++++++++++--- 1 file changed, 126 insertions(+), 18 deletions(-) diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 996ed5558bf12..63c2b9ca49cf5 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -276,6 +276,7 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { const std::string first("first"); const std::string second("second"); + const std::string third("third"); auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, reinterpret_cast(&server_addr), addr_len); @@ -287,21 +288,32 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { ASSERT_EQ(send_rc, second.length()); + send_rc = ::sendto(client_sockfd, third.c_str(), third.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, third.length()); + + auto validateCallParams = [&](Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address) { + ASSERT_NE(local_address, nullptr); + + ASSERT_NE(peer_address, nullptr); + ASSERT_NE(peer_address->ip(), nullptr); + + ASSERT_EQ(local_address->asString(), server_socket->localAddress()->asString()); + + ASSERT_EQ(peer_address->ip()->addressAsString(), + client_socket->localAddress()->ip()->addressAsString()); + + EXPECT_EQ(*local_address, *server_socket->localAddress()); + }; + EXPECT_CALL(listener_callbacks, onNewConnection_(_, _, _)) .WillOnce( Invoke([&](Address::InstanceConstSharedPtr local_address, Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { - ASSERT_NE(local_address, nullptr); - - ASSERT_NE(peer_address, nullptr); - ASSERT_NE(peer_address->ip(), nullptr); + validateCallParams(local_address, peer_address); - ASSERT_EQ(local_address->asString(), server_socket->localAddress()->asString()); - - ASSERT_EQ(peer_address->ip()->addressAsString(), - client_socket->localAddress()->ip()->addressAsString()); - - EXPECT_EQ(*local_address, *server_socket->localAddress()); ASSERT_EQ(data->toString(), first); })); @@ -309,18 +321,114 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { .WillOnce( Invoke([&](Address::InstanceConstSharedPtr local_address, Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { - ASSERT_NE(local_address, nullptr); + validateCallParams(local_address, peer_address); - ASSERT_NE(peer_address, nullptr); - ASSERT_NE(peer_address->ip(), nullptr); + ASSERT_EQ(data->toString(), second); + })) + .WillOnce( + Invoke([&](Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { + validateCallParams(local_address, peer_address); - ASSERT_EQ(local_address->asString(), server_socket->localAddress()->asString()); + ASSERT_EQ(data->toString(), third); - ASSERT_EQ(peer_address->ip()->addressAsString(), - client_socket->localAddress()->ip()->addressAsString()); + dispatcher_.exit(); + })); - EXPECT_EQ(*local_address, *server_socket->localAddress()); - ASSERT_EQ(data->toString(), second); + dispatcher_.run(Event::Dispatcher::RunType::Block); +} + +TEST_P(ListenerImplTest, UdpListenerEnableDisable) { + SocketPtr server_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, true); + + ASSERT_NE(server_socket, nullptr); + + auto const* server_ip = server_socket->localAddress()->ip(); + ASSERT_NE(server_ip, nullptr); + + Network::MockUdpListenerCallbacks listener_callbacks; + Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks, + true); + + SocketPtr client_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, false); + + const int client_sockfd = client_socket->fd(); + sockaddr_storage server_addr; + socklen_t addr_len; + + getSocketAddressInfo(*client_socket.get(), server_addr, addr_len); + ASSERT_GT(addr_len, 0); + + if (version_ == Address::IpVersion::v4) { + struct sockaddr_in* servaddr = reinterpret_cast(&server_addr); + servaddr->sin_port = htons(server_ip->port()); + } else if (version_ == Address::IpVersion::v6) { + struct sockaddr_in6* servaddr = reinterpret_cast(&server_addr); + servaddr->sin6_port = htons(server_ip->port()); + } + + const std::string first("first"); + const std::string second("second"); + const std::string third("third"); + + listener.disable(); + + auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, first.length()); + + send_rc = ::sendto(client_sockfd, second.c_str(), second.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, second.length()); + + send_rc = ::sendto(client_sockfd, third.c_str(), third.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, third.length()); + + auto validateCallParams = [&](Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address) { + ASSERT_NE(local_address, nullptr); + + ASSERT_NE(peer_address, nullptr); + ASSERT_NE(peer_address->ip(), nullptr); + + ASSERT_EQ(local_address->asString(), server_socket->localAddress()->asString()); + + ASSERT_EQ(peer_address->ip()->addressAsString(), + client_socket->localAddress()->ip()->addressAsString()); + + EXPECT_EQ(*local_address, *server_socket->localAddress()); + }; + + Event::TimerPtr timer = dispatcher_.createTimer([&] { dispatcher_.exit(); }); + + timer->enableTimer(std::chrono::milliseconds(2000)); + + EXPECT_CALL(listener_callbacks, onNewConnection_(_, _, _)).Times(0); + + EXPECT_CALL(listener_callbacks, onData_(_, _, _)).Times(0); + + dispatcher_.run(Event::Dispatcher::RunType::Block); + + listener.enable(); + + EXPECT_CALL(listener_callbacks, onNewConnection_(_, _, _)).Times(1); + EXPECT_CALL(listener_callbacks, onData_(_, _, _)) + .Times(2) + .WillOnce(Return()) + .WillOnce( + Invoke([&](Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { + validateCallParams(local_address, peer_address); + + ASSERT_EQ(data->toString(), third); dispatcher_.exit(); })); From 3c7e6f66321d2516487a16b3a0dd334d65bd67da Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Fri, 4 Jan 2019 10:55:26 -0800 Subject: [PATCH 04/32] Fixed recvfrom arguments. Signed-off-by: Jojy G Varghese --- source/common/buffer/buffer_impl.cc | 3 ++- source/common/network/udp_listener_impl.cc | 12 +++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index cf5a3be3a998c..6261c8436c5a6 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -116,7 +116,8 @@ Api::SysCallIntResult OwnedImpl::recvFrom(int fd, uint64_t max_length, sockaddr_ return {0, 0}; } - memset(&peer_addr, 0, sizeof(sockaddr_storage)); + addr_len = sizeof(sockaddr_storage); + memset(&peer_addr, 0, addr_len); RawSlice slice; const uint64_t num_slices = reserve(max_length, &slice, 1); diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index d6cdedbd759c9..9f6660fbd45b3 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -40,7 +40,7 @@ void UdpListenerImpl::disable() { event_del(&raw_event_); } void UdpListenerImpl::enable() { event_add(&raw_event_, nullptr); } void UdpListenerImpl::readCallback(int fd, short flags, void* arg) { - (void)flags; + RELEASE_ASSERT((flags == EV_READ), fmt::format("Unexpected flags for callback: {}", flags)); UdpListenerImpl* instance = static_cast(arg); ASSERT(instance); @@ -54,13 +54,12 @@ void UdpListenerImpl::readCallback(int fd, short flags, void* arg) { Api::SysCallIntResult result = buffer->recvFrom(fd, read_length, addr, addr_len); if (result.rc_ < 0) { // TODO(conqerAtApple): Call error callback. - RELEASE_ASSERT(false, fmt::format("recvfrom returned rc: {}, error: {}", result.rc_, - strerror(result.errno_))); } Address::InstanceConstSharedPtr local_address = instance->socket_.localAddress(); + RELEASE_ASSERT( - addr_len, + addr_len > 0, fmt::format( "Unable to get remote address for fd: {}, local address: {}. address length is 0 ", fd, local_address->asString())); @@ -74,6 +73,7 @@ void UdpListenerImpl::readCallback(int fd, short flags, void* arg) { const struct sockaddr_in* sin = reinterpret_cast(&addr); ASSERT(AF_INET == sin->sin_family); peer_address = std::make_shared(sin); + break; } case AF_INET6: { @@ -90,14 +90,16 @@ void UdpListenerImpl::readCallback(int fd, short flags, void* arg) { } else { peer_address = std::make_shared(*sin6, true); } + + break; } - break; default: RELEASE_ASSERT(false, fmt::format("Unsupported address family: {}, local address: {}, receive size: " "{}, address length: {}", addr.ss_family, local_address->asString(), result.rc_, addr_len)); + break; } RELEASE_ASSERT((peer_address != nullptr), From 6389f68d9269fcd5f5101f0f451e43e8283e1419 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Fri, 4 Jan 2019 11:15:24 -0800 Subject: [PATCH 05/32] Taking care of -EAGAIN. Signed-off-by: Jojy G Varghese --- source/common/network/udp_listener_impl.cc | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index 9f6660fbd45b3..db61f85148281 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -50,11 +50,19 @@ void UdpListenerImpl::readCallback(int fd, short flags, void* arg) { Buffer::InstancePtr buffer(new Buffer::OwnedImpl()); sockaddr_storage addr; socklen_t addr_len; + Api::SysCallIntResult result; + + do { + result = buffer->recvFrom(fd, read_length, addr, addr_len); + if (result.rc_ < 0) { + if (result.rc_ == -EAGAIN) { + continue; + } + // TODO(conqerAtApple): Call error callback. + } - Api::SysCallIntResult result = buffer->recvFrom(fd, read_length, addr, addr_len); - if (result.rc_ < 0) { - // TODO(conqerAtApple): Call error callback. - } + break; + } while (true); Address::InstanceConstSharedPtr local_address = instance->socket_.localAddress(); From bd240a58bd002e1e8db13f4526a92beae2d44fb5 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Fri, 4 Jan 2019 15:06:02 -0800 Subject: [PATCH 06/32] Added support for mock buffer. Signed-off-by: Jojy G Varghese --- source/common/network/udp_listener_impl.cc | 10 ++- source/common/network/udp_listener_impl.h | 4 ++ test/common/network/listener_impl_test.cc | 75 +++++++++++++++++++++- 3 files changed, 86 insertions(+), 3 deletions(-) diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index db61f85148281..13275a2165038 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -7,7 +7,6 @@ #include "envoy/buffer/buffer.h" #include "envoy/common/exception.h" -#include "common/buffer/buffer_impl.h" #include "common/common/assert.h" #include "common/common/empty_string.h" #include "common/common/fmt.h" @@ -39,6 +38,10 @@ void UdpListenerImpl::disable() { event_del(&raw_event_); } void UdpListenerImpl::enable() { event_add(&raw_event_, nullptr); } +Buffer::InstancePtr UdpListenerImpl::getBufferImpl() { + return std::make_unique(); +} + void UdpListenerImpl::readCallback(int fd, short flags, void* arg) { RELEASE_ASSERT((flags == EV_READ), fmt::format("Unexpected flags for callback: {}", flags)); @@ -47,7 +50,9 @@ void UdpListenerImpl::readCallback(int fd, short flags, void* arg) { // TODO(conqerAtAppple): Make this configurable or get from system. constexpr uint64_t const read_length = 16384; - Buffer::InstancePtr buffer(new Buffer::OwnedImpl()); + Buffer::InstancePtr buffer = std::move(instance->getBufferImpl()); + ASSERT(buffer); + sockaddr_storage addr; socklen_t addr_len; Api::SysCallIntResult result; @@ -59,6 +64,7 @@ void UdpListenerImpl::readCallback(int fd, short flags, void* arg) { continue; } // TODO(conqerAtApple): Call error callback. + return; } break; diff --git a/source/common/network/udp_listener_impl.h b/source/common/network/udp_listener_impl.h index 61382c5cbfd0c..2206a0b8b5008 100644 --- a/source/common/network/udp_listener_impl.h +++ b/source/common/network/udp_listener_impl.h @@ -1,5 +1,6 @@ #pragma once +#include "common/buffer/buffer_impl.h" #include "common/event/event_impl_base.h" #include "base_listener_impl.h" @@ -18,6 +19,9 @@ class UdpListenerImpl : public BaseListenerImpl, public Event::ImplBase { virtual void disable() override; virtual void enable() override; + // Useful for testing/mocking. + virtual Buffer::InstancePtr getBufferImpl(); + protected: UdpListenerCallbacks& cb_; diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 63c2b9ca49cf5..003d4f8c05194 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -77,13 +77,25 @@ class TestListenerImpl : public ListenerImpl { MOCK_METHOD1(getLocalAddress, Address::InstanceConstSharedPtr(int fd)); }; +class MockResultBufferImpl : public Buffer::OwnedImpl { +public: + MockResultBufferImpl(const Api::SysCallIntResult& result) : result_(result) {} + + Api::SysCallIntResult recvFrom(int, uint64_t, sockaddr_storage&, socklen_t&) override { + return result_; + } + +private: + const Api::SysCallIntResult result_; +}; + class TestUdpListenerImpl : public UdpListenerImpl { public: TestUdpListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb, bool bind_to_port) : UdpListenerImpl(dispatcher, socket, cb, bind_to_port) {} - MOCK_METHOD1(getLocalAddress, Address::InstanceConstSharedPtr(int fd)); + MOCK_METHOD0(getBufferImpl, Buffer::InstancePtr()); }; class ListenerImplTest : public testing::TestWithParam { @@ -255,6 +267,10 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks, true); + EXPECT_CALL(listener, getBufferImpl()).WillRepeatedly(Invoke([&]() { + return std::make_unique(); + })); + SocketPtr client_socket = getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), nullptr, false); @@ -352,6 +368,10 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks, true); + EXPECT_CALL(listener, getBufferImpl()).WillRepeatedly(Invoke([&]() { + return std::make_unique(); + })); + SocketPtr client_socket = getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), nullptr, false); @@ -436,6 +456,59 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { dispatcher_.run(Event::Dispatcher::RunType::Block); } +TEST_P(ListenerImplTest, UdpListenerRecvFromError) { + SocketPtr server_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, true); + + ASSERT_NE(server_socket, nullptr); + + auto const* server_ip = server_socket->localAddress()->ip(); + ASSERT_NE(server_ip, nullptr); + + Network::MockUdpListenerCallbacks listener_callbacks; + Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks, + true); + + EXPECT_CALL(listener, getBufferImpl()).WillRepeatedly(Invoke([&]() { + return std::make_unique(Api::SysCallIntResult{-1, -1}); + })); + + SocketPtr client_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, false); + + const int client_sockfd = client_socket->fd(); + sockaddr_storage server_addr; + socklen_t addr_len; + + getSocketAddressInfo(*client_socket.get(), server_addr, addr_len); + ASSERT_GT(addr_len, 0); + + if (version_ == Address::IpVersion::v4) { + struct sockaddr_in* servaddr = reinterpret_cast(&server_addr); + servaddr->sin_port = htons(server_ip->port()); + } else if (version_ == Address::IpVersion::v6) { + struct sockaddr_in6* servaddr = reinterpret_cast(&server_addr); + servaddr->sin6_port = htons(server_ip->port()); + } + + const std::string first("first"); + + auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, first.length()); + + Event::TimerPtr timer = dispatcher_.createTimer([&] { dispatcher_.exit(); }); + + timer->enableTimer(std::chrono::milliseconds(2000)); + + EXPECT_CALL(listener_callbacks, onNewConnection_(_, _, _)).Times(0); + + dispatcher_.run(Event::Dispatcher::RunType::Block); +} + TEST_P(ListenerImplTest, WildcardListenerUseActualDst) { Stats::IsolatedStoreImpl stats_store; Network::TcpListenSocket socket(Network::Test::getAnyAddress(version_), nullptr, true); From 3a973f174e7b19fca50b7b4c0148ebe1cfd47b2e Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Fri, 4 Jan 2019 18:48:59 -0800 Subject: [PATCH 07/32] Added support for onError callback. Signed-off-by: Jojy G Varghese --- include/envoy/network/listener.h | 12 +++++-- source/common/network/udp_listener_impl.cc | 1 + source/common/network/udp_listener_impl.h | 2 ++ test/common/network/listener_impl_test.cc | 40 ++++++++++++++++++++-- test/mocks/network/mocks.h | 4 +++ 5 files changed, 54 insertions(+), 5 deletions(-) diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index 5933211f7b6be..648dd577a5718 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -119,9 +119,9 @@ class ListenerCallbacks { */ class UdpListenerCallbacks { public: - virtual ~UdpListenerCallbacks() = default; + enum class ErrorCode { SYSCALL_ERROR, UNKNOWN_ERROR }; - // TODO(conqerAtapple): Refactor `Connection` to accommodate UDP/QUIC. + virtual ~UdpListenerCallbacks() = default; /** * On first packet received on a UDP socket. This allows the callback handler @@ -144,6 +144,14 @@ class UdpListenerCallbacks { virtual void onData(Address::InstanceConstSharedPtr local_address, Address::InstanceConstSharedPtr peer_address, Buffer::InstancePtr&& data) PURE; + + /** + * Called when there is an error event. + * + * @param error_code `ErrorCode` for tbe error event. + * @param errno System error number. + */ + virtual void onError(const ErrorCode& err_code, int err_no) PURE; }; /** diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index 13275a2165038..afb0e9d513341 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -64,6 +64,7 @@ void UdpListenerImpl::readCallback(int fd, short flags, void* arg) { continue; } // TODO(conqerAtApple): Call error callback. + instance->cb_.onError(UdpListenerCallbacks::ErrorCode::SYSCALL_ERROR, result.errno_); return; } diff --git a/source/common/network/udp_listener_impl.h b/source/common/network/udp_listener_impl.h index 2206a0b8b5008..b4b000b5ec6ca 100644 --- a/source/common/network/udp_listener_impl.h +++ b/source/common/network/udp_listener_impl.h @@ -1,5 +1,7 @@ #pragma once +#include + #include "common/buffer/buffer_impl.h" #include "common/event/event_impl_base.h" diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 003d4f8c05194..05bc81deb81b7 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -253,7 +253,11 @@ TEST_P(ListenerImplTest, UseActualDstTcp) { dispatcher_.run(Event::Dispatcher::RunType::Block); } +/** + * Tests UDP listener for actual destination and data. + */ TEST_P(ListenerImplTest, UseActualDstUdp) { + // Setup server socket SocketPtr server_socket = getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); @@ -263,6 +267,7 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { auto const* server_ip = server_socket->localAddress()->ip(); ASSERT_NE(server_ip, nullptr); + // Setup callback handler and listener. Network::MockUdpListenerCallbacks listener_callbacks; Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks, true); @@ -271,6 +276,7 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { return std::make_unique(); })); + // Setup client socket. SocketPtr client_socket = getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), nullptr, false); @@ -290,6 +296,9 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { servaddr->sin6_port = htons(server_ip->port()); } + // We send 3 packets + // - First one should invoke `onNewConnection` callback. + // - Second and third one should invoke `onData` callback. const std::string first("first"); const std::string second("second"); const std::string third("third"); @@ -354,7 +363,11 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { dispatcher_.run(Event::Dispatcher::RunType::Block); } +/** + * Tests UDP listener's `enable` and `disable` APIs. + */ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { + // Setup server socket SocketPtr server_socket = getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); @@ -364,6 +377,7 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { auto const* server_ip = server_socket->localAddress()->ip(); ASSERT_NE(server_ip, nullptr); + // Setup callback handler and listener. Network::MockUdpListenerCallbacks listener_callbacks; Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks, true); @@ -372,6 +386,7 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { return std::make_unique(); })); + // Setup client socket. SocketPtr client_socket = getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), nullptr, false); @@ -391,6 +406,13 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { servaddr->sin6_port = htons(server_ip->port()); } + // We first disable the listener and then send three packets. + // - With the listener disabled, we expect that none of the callbacks will be + // called. + // - When the listener is enabled back, we expect the callbacks to be called + // such that: + // - First one should invoke `onNewConnection` callback. + // - Second and third one should invoke `onData` callback. const std::string first("first"); const std::string second("second"); const std::string third("third"); @@ -456,7 +478,11 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { dispatcher_.run(Event::Dispatcher::RunType::Block); } +/** + * Tests UDP listebe's error callback. + */ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { + // Setup server socket SocketPtr server_socket = getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); @@ -466,6 +492,7 @@ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { auto const* server_ip = server_socket->localAddress()->ip(); ASSERT_NE(server_ip, nullptr); + // Setup callback handler and listener. Network::MockUdpListenerCallbacks listener_callbacks; Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks, true); @@ -493,6 +520,8 @@ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { servaddr->sin6_port = htons(server_ip->port()); } + // When the `receive` system call returns an error, we expect the `onError` + // callback callwed with `SYSCALL_ERROR` parameter. const std::string first("first"); auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, @@ -500,11 +529,16 @@ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { ASSERT_EQ(send_rc, first.length()); - Event::TimerPtr timer = dispatcher_.createTimer([&] { dispatcher_.exit(); }); + EXPECT_CALL(listener_callbacks, onNewConnection_(_, _, _)).Times(0); - timer->enableTimer(std::chrono::milliseconds(2000)); + EXPECT_CALL(listener_callbacks, onError_(_, _)) + .Times(1) + .WillOnce(Invoke([&](const UdpListenerCallbacks::ErrorCode& err_code, int err) -> void { + ASSERT_EQ(err_code, UdpListenerCallbacks::ErrorCode::SYSCALL_ERROR); + ASSERT_EQ(err, -1); - EXPECT_CALL(listener_callbacks, onNewConnection_(_, _, _)).Times(0); + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); } diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 41411c44e6a88..6e2ed85ad29c9 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -133,12 +133,16 @@ class MockUdpListenerCallbacks : public UdpListenerCallbacks { onData_(local_address, peer_address, data.get()); } + void onError(const ErrorCode& err_code, int err) override { onError_(err_code, err); } + MOCK_METHOD3(onNewConnection_, void(Address::InstanceConstSharedPtr local_address, Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data)); MOCK_METHOD3(onData_, void(Address::InstanceConstSharedPtr local_address, Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data)); + + MOCK_METHOD2(onError_, void(const ErrorCode& err_code, int err)); }; class MockDrainDecision : public DrainDecision { From 6803aee479656b095aca28ffd3566976ee4afe52 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Fri, 4 Jan 2019 18:59:27 -0800 Subject: [PATCH 08/32] Removed move operator. Signed-off-by: Jojy G Varghese --- source/common/network/udp_listener_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index afb0e9d513341..775e748645b22 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -50,7 +50,7 @@ void UdpListenerImpl::readCallback(int fd, short flags, void* arg) { // TODO(conqerAtAppple): Make this configurable or get from system. constexpr uint64_t const read_length = 16384; - Buffer::InstancePtr buffer = std::move(instance->getBufferImpl()); + Buffer::InstancePtr buffer = instance->getBufferImpl(); ASSERT(buffer); sockaddr_storage addr; From eed7d49c83bbc118191e98dc9c687c7bda9e55cb Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Thu, 10 Jan 2019 08:28:03 -0800 Subject: [PATCH 09/32] Addressed some review comments. - Removed bind_to_port from UdpListener ctor. - Spellchecks and comments - Added EV_WRITE flag for udp listener Signed-off-by: Jojy G Varghese --- include/envoy/event/dispatcher.h | 4 +- include/envoy/network/listener.h | 10 ++--- source/common/buffer/buffer_impl.cc | 2 +- source/common/event/dispatcher_impl.cc | 5 +-- source/common/event/dispatcher_impl.h | 4 +- source/common/network/udp_listener_impl.cc | 51 ++++++++++++++-------- source/common/network/udp_listener_impl.h | 9 ++-- test/common/network/listener_impl_test.cc | 14 +++--- test/mocks/event/mocks.h | 11 +++-- 9 files changed, 59 insertions(+), 51 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 5f89bbcd04518..9dff305341368 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -114,12 +114,10 @@ class Dispatcher { * Create a logical udp listener on a specific port. * @param socket supplies the socket to listen on. * @param cb supplies the udp listener callbacks to invoke for listener events. - * @param bind_to_port controls whether the listener binds to a transport port or not. * @return Network::ListenerPtr a new listener that is owned by the caller. */ virtual Network::ListenerPtr createUdpListener(Network::Socket& socket, - Network::UdpListenerCallbacks& cb, - bool bind_to_port) PURE; + Network::UdpListenerCallbacks& cb) PURE; /** * Allocate a timer. @see Timer for docs on how to use the timer. * @param cb supplies the callback to invoke when the timer fires. diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index 648dd577a5718..d058aaf906b85 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -115,7 +115,7 @@ class ListenerCallbacks { }; /** - * UDP listener callbacks. + * Udp listener callbacks. */ class UdpListenerCallbacks { public: @@ -124,8 +124,8 @@ class UdpListenerCallbacks { virtual ~UdpListenerCallbacks() = default; /** - * On first packet received on a UDP socket. This allows the callback handler - * to establish filter chain (or any other prepararion). + * On first packet received on a udp socket. This allows the callback handler + * to establish filter chain (or any other preparation). * * @param local_address Local bound socket network address. * @param peer_address Network address of the peer. @@ -135,7 +135,7 @@ class UdpListenerCallbacks { Address::InstanceConstSharedPtr peer_address, Buffer::InstancePtr&& data) PURE; /** - * Called whenever data is received by the underlying Udp socket. + * Called whenever data is received by the underlying udp socket. * * @param local_address Local bound socket network address. * @param peer_address Network address of the peer. @@ -148,7 +148,7 @@ class UdpListenerCallbacks { /** * Called when there is an error event. * - * @param error_code `ErrorCode` for tbe error event. + * @param error_code ErrorCode for the error event. * @param errno System error number. */ virtual void onError(const ErrorCode& err_code, int err_no) PURE; diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 6261c8436c5a6..19a639d0ceaa4 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -133,7 +133,7 @@ Api::SysCallIntResult OwnedImpl::recvFrom(int fd, uint64_t max_length, sockaddr_ slice.len_ = std::min(slice.len_, static_cast(rc)); commit(&slice, 1); - return {static_cast(rc), errno}; + return {static_cast(rc), 0}; } Api::SysCallIntResult OwnedImpl::read(int fd, uint64_t max_length) { diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index d7b55b5ae820b..ced376e638dc5 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -121,10 +121,9 @@ DispatcherImpl::createListener(Network::Socket& socket, Network::ListenerCallbac } Network::ListenerPtr DispatcherImpl::createUdpListener(Network::Socket& socket, - Network::UdpListenerCallbacks& cb, - bool bind_to_port) { + Network::UdpListenerCallbacks& cb) { ASSERT(isThreadSafe()); - return Network::ListenerPtr{new Network::UdpListenerImpl(*this, socket, cb, bind_to_port)}; + return Network::ListenerPtr{new Network::UdpListenerImpl(*this, socket, cb)}; } TimerPtr DispatcherImpl::createTimer(TimerCb cb) { diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 24581a335eb7c..fc0cf5326401d 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -51,8 +51,8 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { Network::ListenerPtr createListener(Network::Socket& socket, Network::ListenerCallbacks& cb, bool bind_to_port, bool hand_off_restored_destination_connections) override; - Network::ListenerPtr createUdpListener(Network::Socket& socket, Network::UdpListenerCallbacks& cb, - bool bind_to_port) override; + Network::ListenerPtr createUdpListener(Network::Socket& socket, + Network::UdpListenerCallbacks& cb) override; TimerPtr createTimer(TimerCb cb) override; void deferredDelete(DeferredDeletablePtr&& to_delete) override; void exit() override; diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index 775e748645b22..ad215c284de20 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -19,18 +19,16 @@ namespace Envoy { namespace Network { UdpListenerImpl::UdpListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, - UdpListenerCallbacks& cb, bool bind_to_port) + UdpListenerCallbacks& cb) : BaseListenerImpl(dispatcher, socket), cb_(cb), is_first_(true) { - if (bind_to_port) { - event_assign(&raw_event_, &dispatcher.base(), socket.fd(), EV_READ | EV_PERSIST, readCallback, - this); - event_add(&raw_event_, nullptr); - - if (!Network::Socket::applyOptions(socket.options(), socket, - envoy::api::v2::core::SocketOption::STATE_BOUND)) { - throw CreateListenerException(fmt::format("cannot set post-bound socket option on socket: {}", - socket.localAddress()->asString())); - } + event_assign(&raw_event_, &dispatcher.base(), socket.fd(), EV_READ | EV_WRITE | EV_PERSIST, + eventCallback, this); + event_add(&raw_event_, nullptr); + + if (!Network::Socket::applyOptions(socket.options(), socket, + envoy::api::v2::core::SocketOption::STATE_BOUND)) { + throw CreateListenerException(fmt::format("cannot set post-bound socket option on socket: {}", + socket.localAddress()->asString())); } } @@ -42,15 +40,26 @@ Buffer::InstancePtr UdpListenerImpl::getBufferImpl() { return std::make_unique(); } -void UdpListenerImpl::readCallback(int fd, short flags, void* arg) { - RELEASE_ASSERT((flags == EV_READ), fmt::format("Unexpected flags for callback: {}", flags)); +void UdpListenerImpl::eventCallback(int fd, short flags, void* arg) { + RELEASE_ASSERT((flags & (EV_READ | EV_WRITE)), + fmt::format("Unexpected flags for callback: {}", flags)); UdpListenerImpl* instance = static_cast(arg); ASSERT(instance); + if (flags & EV_READ) { + instance->handleReadCallback(fd); + } + + if (flags & EV_WRITE) { + instance->handleWriteCallback(fd); + } +} + +void UdpListenerImpl::handleReadCallback(int fd) { // TODO(conqerAtAppple): Make this configurable or get from system. constexpr uint64_t const read_length = 16384; - Buffer::InstancePtr buffer = instance->getBufferImpl(); + Buffer::InstancePtr buffer = getBufferImpl(); ASSERT(buffer); sockaddr_storage addr; @@ -64,14 +73,14 @@ void UdpListenerImpl::readCallback(int fd, short flags, void* arg) { continue; } // TODO(conqerAtApple): Call error callback. - instance->cb_.onError(UdpListenerCallbacks::ErrorCode::SYSCALL_ERROR, result.errno_); + cb_.onError(UdpListenerCallbacks::ErrorCode::SYSCALL_ERROR, result.errno_); return; } break; } while (true); - Address::InstanceConstSharedPtr local_address = instance->socket_.localAddress(); + Address::InstanceConstSharedPtr local_address = socket_.localAddress(); RELEASE_ASSERT( addr_len > 0, @@ -125,12 +134,16 @@ void UdpListenerImpl::readCallback(int fd, short flags, void* arg) { fmt::format("Unable to get local address for fd: {}", fd)); bool expected = true; - if (instance->is_first_.compare_exchange_strong(expected, false)) { - instance->cb_.onNewConnection(local_address, peer_address, std::move(buffer)); + if (is_first_.compare_exchange_strong(expected, false)) { + cb_.onNewConnection(local_address, peer_address, std::move(buffer)); } else { - instance->cb_.onData(local_address, peer_address, std::move(buffer)); + cb_.onData(local_address, peer_address, std::move(buffer)); } } +void UdpListenerImpl::handleWriteCallback(int fd) { + (void)fd; +} + } // namespace Network } // namespace Envoy diff --git a/source/common/network/udp_listener_impl.h b/source/common/network/udp_listener_impl.h index b4b000b5ec6ca..bf4b50ea60140 100644 --- a/source/common/network/udp_listener_impl.h +++ b/source/common/network/udp_listener_impl.h @@ -15,8 +15,8 @@ namespace Network { */ class UdpListenerImpl : public BaseListenerImpl, public Event::ImplBase { public: - UdpListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb, - bool bind_to_port); + UdpListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, + UdpListenerCallbacks& cb); virtual void disable() override; virtual void enable() override; @@ -25,10 +25,13 @@ class UdpListenerImpl : public BaseListenerImpl, public Event::ImplBase { virtual Buffer::InstancePtr getBufferImpl(); protected: + void handleWriteCallback(int fd); + void handleReadCallback(int fd); + UdpListenerCallbacks& cb_; private: - static void readCallback(int fd, short flags, void* arg); + static void eventCallback(int fd, short flags, void* arg); std::atomic is_first_; }; diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 05bc81deb81b7..29bda64152786 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -91,9 +91,8 @@ class MockResultBufferImpl : public Buffer::OwnedImpl { class TestUdpListenerImpl : public UdpListenerImpl { public: - TestUdpListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb, - bool bind_to_port) - : UdpListenerImpl(dispatcher, socket, cb, bind_to_port) {} + TestUdpListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb) + : UdpListenerImpl(dispatcher, socket, cb) {} MOCK_METHOD0(getBufferImpl, Buffer::InstancePtr()); }; @@ -269,8 +268,7 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { // Setup callback handler and listener. Network::MockUdpListenerCallbacks listener_callbacks; - Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks, - true); + Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); EXPECT_CALL(listener, getBufferImpl()).WillRepeatedly(Invoke([&]() { return std::make_unique(); @@ -379,8 +377,7 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { // Setup callback handler and listener. Network::MockUdpListenerCallbacks listener_callbacks; - Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks, - true); + Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); EXPECT_CALL(listener, getBufferImpl()).WillRepeatedly(Invoke([&]() { return std::make_unique(); @@ -494,8 +491,7 @@ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { // Setup callback handler and listener. Network::MockUdpListenerCallbacks listener_callbacks; - Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks, - true); + Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); EXPECT_CALL(listener, getBufferImpl()).WillRepeatedly(Invoke([&]() { return std::make_unique(Api::SysCallIntResult{-1, -1}); diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index d92b39fb80b94..55ef6403498a0 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -65,9 +65,9 @@ class MockDispatcher : public Dispatcher { createListener_(socket, cb, bind_to_port, hand_off_restored_destination_connections)}; } - Network::ListenerPtr createUdpListener(Network::Socket& socket, Network::UdpListenerCallbacks& cb, - bool bind_to_port) override { - return Network::ListenerPtr{createUdpListener_(socket, cb, bind_to_port)}; + Network::ListenerPtr createUdpListener(Network::Socket& socket, + Network::UdpListenerCallbacks& cb) override { + return Network::ListenerPtr{createUdpListener_(socket, cb)}; } Event::TimerPtr createTimer(Event::TimerCb cb) override { @@ -106,9 +106,8 @@ class MockDispatcher : public Dispatcher { Network::Listener*(Network::Socket& socket, Network::ListenerCallbacks& cb, bool bind_to_port, bool hand_off_restored_destination_connections)); - MOCK_METHOD3(createUdpListener_, - Network::Listener*(Network::Socket& socket, Network::UdpListenerCallbacks& cb, - bool bind_to_port)); + MOCK_METHOD2(createUdpListener_, + Network::Listener*(Network::Socket& socket, Network::UdpListenerCallbacks& cb)); MOCK_METHOD1(createTimer_, Timer*(Event::TimerCb cb)); MOCK_METHOD1(deferredDelete_, void(DeferredDeletable* to_delete)); MOCK_METHOD0(exit, void()); From 0bcfbb8628f14940702f2ce0fe5123cb12fc98de Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Thu, 10 Jan 2019 17:09:12 -0800 Subject: [PATCH 10/32] Added `onWriteReady` implementation. Signed-off-by: Jojy G Varghese --- include/envoy/network/listener.h | 2 ++ source/common/network/udp_listener_impl.cc | 8 ++++++- test/common/network/listener_impl_test.cc | 27 +++++++++++++++++----- test/mocks/network/mocks.h | 6 +++++ 4 files changed, 36 insertions(+), 7 deletions(-) diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index d058aaf906b85..8e128d3055854 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -145,6 +145,8 @@ class UdpListenerCallbacks { Address::InstanceConstSharedPtr peer_address, Buffer::InstancePtr&& data) PURE; + virtual void onWriteReady(const Socket& socket) PURE; + /** * Called when there is an error event. * diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index ad215c284de20..72c72944e1a80 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -57,6 +57,9 @@ void UdpListenerImpl::eventCallback(int fd, short flags, void* arg) { } void UdpListenerImpl::handleReadCallback(int fd) { + RELEASE_ASSERT(fd == socket_.fd(), + fmt::format("Invalid socket descriptor received in callback {}", fd)); + // TODO(conqerAtAppple): Make this configurable or get from system. constexpr uint64_t const read_length = 16384; Buffer::InstancePtr buffer = getBufferImpl(); @@ -142,7 +145,10 @@ void UdpListenerImpl::handleReadCallback(int fd) { } void UdpListenerImpl::handleWriteCallback(int fd) { - (void)fd; + RELEASE_ASSERT(fd == socket_.fd(), + fmt::format("Invalid socket descriptor received in callback {}", fd)); + + cb_.onWriteReady(socket_); } } // namespace Network diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 29bda64152786..4b13fad76d879 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -323,9 +323,9 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { ASSERT_NE(peer_address, nullptr); ASSERT_NE(peer_address->ip(), nullptr); - ASSERT_EQ(local_address->asString(), server_socket->localAddress()->asString()); + EXPECT_EQ(local_address->asString(), server_socket->localAddress()->asString()); - ASSERT_EQ(peer_address->ip()->addressAsString(), + EXPECT_EQ(peer_address->ip()->addressAsString(), client_socket->localAddress()->ip()->addressAsString()); EXPECT_EQ(*local_address, *server_socket->localAddress()); @@ -337,7 +337,7 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { validateCallParams(local_address, peer_address); - ASSERT_EQ(data->toString(), first); + EXPECT_EQ(data->toString(), first); })); EXPECT_CALL(listener_callbacks, onData_(_, _, _)) @@ -346,18 +346,22 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { validateCallParams(local_address, peer_address); - ASSERT_EQ(data->toString(), second); + EXPECT_EQ(data->toString(), second); })) .WillOnce( Invoke([&](Address::InstanceConstSharedPtr local_address, Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { validateCallParams(local_address, peer_address); - ASSERT_EQ(data->toString(), third); + EXPECT_EQ(data->toString(), third); dispatcher_.exit(); })); + EXPECT_CALL(listener_callbacks, onWriteReady_(_)) + .WillRepeatedly( + Invoke([&](const Socket& socket) { EXPECT_EQ(socket.fd(), server_socket->fd()); })); + dispatcher_.run(Event::Dispatcher::RunType::Block); } @@ -454,6 +458,8 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { EXPECT_CALL(listener_callbacks, onData_(_, _, _)).Times(0); + EXPECT_CALL(listener_callbacks, onWriteReady_(_)).Times(0); + dispatcher_.run(Event::Dispatcher::RunType::Block); listener.enable(); @@ -467,11 +473,15 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { validateCallParams(local_address, peer_address); - ASSERT_EQ(data->toString(), third); + EXPECT_EQ(data->toString(), third); dispatcher_.exit(); })); + EXPECT_CALL(listener_callbacks, onWriteReady_(_)) + .WillRepeatedly( + Invoke([&](const Socket& socket) { EXPECT_EQ(socket.fd(), server_socket->fd()); })); + dispatcher_.run(Event::Dispatcher::RunType::Block); } @@ -527,6 +537,11 @@ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { EXPECT_CALL(listener_callbacks, onNewConnection_(_, _, _)).Times(0); + EXPECT_CALL(listener_callbacks, onWriteReady_(_)) + .Times(1) + .WillRepeatedly( + Invoke([&](const Socket& socket) { EXPECT_EQ(socket.fd(), server_socket->fd()); })); + EXPECT_CALL(listener_callbacks, onError_(_, _)) .Times(1) .WillOnce(Invoke([&](const UdpListenerCallbacks::ErrorCode& err_code, int err) -> void { diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 6e2ed85ad29c9..fd7022cccf6c4 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -133,6 +133,10 @@ class MockUdpListenerCallbacks : public UdpListenerCallbacks { onData_(local_address, peer_address, data.get()); } + void onWriteReady(const Socket& socket) override { + onWriteReady_(socket); + } + void onError(const ErrorCode& err_code, int err) override { onError_(err_code, err); } MOCK_METHOD3(onNewConnection_, @@ -142,6 +146,8 @@ class MockUdpListenerCallbacks : public UdpListenerCallbacks { MOCK_METHOD3(onData_, void(Address::InstanceConstSharedPtr local_address, Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data)); + MOCK_METHOD1(onWriteReady_, void(const Socket& socket)); + MOCK_METHOD2(onError_, void(const ErrorCode& err_code, int err)); }; From a1afecf00c3c010e1396c25d455c2311b06b637c Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Fri, 11 Jan 2019 14:09:38 -0800 Subject: [PATCH 11/32] Added echo test for UDP listener. Signed-off-by: Jojy G Varghese --- source/common/network/udp_listener_impl.cc | 9 +- test/common/network/listener_impl_test.cc | 211 +++++++++++++++++---- 2 files changed, 180 insertions(+), 40 deletions(-) diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index 72c72944e1a80..92980072c64a7 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -71,17 +71,12 @@ void UdpListenerImpl::handleReadCallback(int fd) { do { result = buffer->recvFrom(fd, read_length, addr, addr_len); - if (result.rc_ < 0) { - if (result.rc_ == -EAGAIN) { - continue; - } - // TODO(conqerAtApple): Call error callback. + if ((result.rc_ < 0) && (result.rc_ != -EAGAIN)) { cb_.onError(UdpListenerCallbacks::ErrorCode::SYSCALL_ERROR, result.errno_); return; } - break; - } while (true); + } while (result.rc_ == -EAGAIN); Address::InstanceConstSharedPtr local_address = socket_.localAddress(); diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 4b13fad76d879..a74cb1f785611 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -1,5 +1,7 @@ +#include #include #include +#include #include "common/network/address_impl.h" #include "common/network/listener_impl.h" @@ -119,15 +121,15 @@ class ListenerImplTest : public testing::TestWithParam { } // TODO(conqerAtapple): Move this to a common place(address.h?) - void getSocketAddressInfo(const Socket& socket, sockaddr_storage& addr, socklen_t& sz) { - memset(&addr, 0, sizeof(addr)); - - auto const* ip = socket.localAddress()->ip(); + void getSocketAddressInfo(const Address::Ip* ip, uint32_t port, sockaddr_storage& addr, + socklen_t& sz) { if (!ip) { sz = 0; return; } + memset(&addr, 0, sizeof(addr)); + if (version_ == Address::IpVersion::v4) { addr.ss_family = AF_INET; auto const* ipv4 = ip->ipv4(); @@ -137,7 +139,7 @@ class ListenerImplTest : public testing::TestWithParam { } sockaddr_in* addrv4 = reinterpret_cast(&addr); - addrv4->sin_port = htons(ip->port()); + addrv4->sin_port = htons(port); addrv4->sin_addr.s_addr = ipv4->address(); sz = sizeof(sockaddr_in); @@ -150,7 +152,7 @@ class ListenerImplTest : public testing::TestWithParam { } sockaddr_in6* addrv6 = reinterpret_cast(&addr); - addrv6->sin6_port = htons(ip->port()); + addrv6->sin6_port = htons(port); const auto address = ipv6->address(); memcpy(static_cast(&addrv6->sin6_addr.s6_addr), static_cast(&address), @@ -162,6 +164,18 @@ class ListenerImplTest : public testing::TestWithParam { } } + void getSocketAddressInfo(const Socket& socket, uint32_t port, sockaddr_storage& addr, + socklen_t& sz) { + getSocketAddressInfo(socket.localAddress()->ip(), port, addr, sz); + } + + void getSocketAddressInfo(Address::InstanceConstSharedPtr address, uint32_t port, + sockaddr_storage& addr, socklen_t& sz) { + ASSERT(address); + + getSocketAddressInfo(address->ip(), port, addr, sz); + } + const Address::IpVersion version_; const Address::InstanceConstSharedPtr alt_address_; Stats::IsolatedStoreImpl stats_store_; @@ -283,17 +297,9 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { sockaddr_storage server_addr; socklen_t addr_len; - getSocketAddressInfo(*client_socket.get(), server_addr, addr_len); + getSocketAddressInfo(*client_socket.get(),server_ip->port(), server_addr, addr_len); ASSERT_GT(addr_len, 0); - if (version_ == Address::IpVersion::v4) { - struct sockaddr_in* servaddr = reinterpret_cast(&server_addr); - servaddr->sin_port = htons(server_ip->port()); - } else if (version_ == Address::IpVersion::v6) { - struct sockaddr_in6* servaddr = reinterpret_cast(&server_addr); - servaddr->sin6_port = htons(server_ip->port()); - } - // We send 3 packets // - First one should invoke `onNewConnection` callback. // - Second and third one should invoke `onData` callback. @@ -366,9 +372,9 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { } /** - * Tests UDP listener's `enable` and `disable` APIs. + * Tests UDP listener for read and write callbacks with actual data. */ -TEST_P(ListenerImplTest, UdpListenerEnableDisable) { +TEST_P(ListenerImplTest, UdpEcho) { // Setup server socket SocketPtr server_socket = getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), @@ -396,17 +402,164 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { sockaddr_storage server_addr; socklen_t addr_len; - getSocketAddressInfo(*client_socket.get(), server_addr, addr_len); + getSocketAddressInfo(*client_socket.get(), server_ip->port(), server_addr, addr_len); ASSERT_GT(addr_len, 0); - if (version_ == Address::IpVersion::v4) { - struct sockaddr_in* servaddr = reinterpret_cast(&server_addr); - servaddr->sin_port = htons(server_ip->port()); - } else if (version_ == Address::IpVersion::v6) { - struct sockaddr_in6* servaddr = reinterpret_cast(&server_addr); - servaddr->sin6_port = htons(server_ip->port()); + // We send 2 packets and exptect it to echo. + const std::string first("first"); + const std::string second("second"); + + auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, first.length()); + + send_rc = ::sendto(client_sockfd, second.c_str(), second.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, second.length()); + + auto validateCallParams = [&](Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address) { + ASSERT_NE(local_address, nullptr); + + ASSERT_NE(peer_address, nullptr); + ASSERT_NE(peer_address->ip(), nullptr); + + EXPECT_EQ(local_address->asString(), server_socket->localAddress()->asString()); + + EXPECT_EQ(peer_address->ip()->addressAsString(), + client_socket->localAddress()->ip()->addressAsString()); + + EXPECT_EQ(*local_address, *server_socket->localAddress()); + }; + + Event::TimerPtr timer = dispatcher_.createTimer([&] { dispatcher_.exit(); }); + + timer->enableTimer(std::chrono::milliseconds(2000)); + + // For unit test purposes, we assume that the data was received in order. + + Address::InstanceConstSharedPtr test_peer_address; + + std::vector server_received_data; + EXPECT_CALL(listener_callbacks, onNewConnection_(_, _, _)) + .WillOnce( + Invoke([&](Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { + validateCallParams(local_address, peer_address); + + test_peer_address = peer_address; + + const std::string data_str = data->toString(); + EXPECT_EQ(data_str, first); + + server_received_data.push_back(data_str); + })); + + EXPECT_CALL(listener_callbacks, onData_(_, _, _)) + .WillOnce( + Invoke([&](Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { + validateCallParams(local_address, peer_address); + + const std::string data_str = data->toString(); + EXPECT_EQ(data_str, second); + + server_received_data.push_back(data_str); + })); + + + EXPECT_CALL(listener_callbacks, onWriteReady_(_)) + .WillRepeatedly(Invoke([&](const Socket& socket) { + EXPECT_EQ(socket.fd(), server_socket->fd()); + + sockaddr_storage client_addr; + socklen_t client_addr_len; + const uint32_t peer_port = test_peer_address->ip()->port(); + + getSocketAddressInfo(test_peer_address, peer_port, client_addr, client_addr_len); + ASSERT_GT(client_addr_len, 0); + + for (const auto& data : server_received_data) { + const std::string::size_type data_size = data.length() + 1; + uint64_t total_sent = 0; + + do { + auto send_rc = + ::sendto(socket.fd(), data.c_str() + total_sent, data_size - total_sent, 0, + reinterpret_cast(&client_addr), client_addr_len); + + if (send_rc > 0) { + total_sent += send_rc; + } else if (send_rc != -EAGAIN) { + break; + } + } while ((send_rc == -EAGAIN) || (total_sent < data_size)); + + EXPECT_EQ(total_sent, data_size); + } + + server_received_data.clear(); + })); + + Buffer::OwnedImpl client_buffer; + Api::SysCallIntResult result; + + for (const auto& data : server_received_data) { + const std::string::size_type data_size = data.length() + 1; + std::string::size_type remaining = data_size; + do { + result = client_buffer.read(client_socket->fd(), remaining); + if (result.rc_ > 0) { + remaining -= result.rc_; + } else if (result.rc_ != -EAGAIN) { + break; + } + } while ((result.rc_ == -EAGAIN) || (remaining)); + + EXPECT_EQ(remaining, 0); + + EXPECT_EQ(client_buffer.toString(), data); } + dispatcher_.run(Event::Dispatcher::RunType::Block); +} + +/** + * Tests UDP listener's `enable` and `disable` APIs. + */ +TEST_P(ListenerImplTest, UdpListenerEnableDisable) { + // Setup server socket + SocketPtr server_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, true); + + ASSERT_NE(server_socket, nullptr); + + auto const* server_ip = server_socket->localAddress()->ip(); + ASSERT_NE(server_ip, nullptr); + + // Setup callback handler and listener. + Network::MockUdpListenerCallbacks listener_callbacks; + Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); + + EXPECT_CALL(listener, getBufferImpl()).WillRepeatedly(Invoke([&]() { + return std::make_unique(); + })); + + // Setup client socket. + SocketPtr client_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, false); + + const int client_sockfd = client_socket->fd(); + sockaddr_storage server_addr; + socklen_t addr_len; + + getSocketAddressInfo(*client_socket.get(), server_ip->port(), server_addr, addr_len); + ASSERT_GT(addr_len, 0); + // We first disable the listener and then send three packets. // - With the listener disabled, we expect that none of the callbacks will be // called. @@ -515,17 +668,9 @@ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { sockaddr_storage server_addr; socklen_t addr_len; - getSocketAddressInfo(*client_socket.get(), server_addr, addr_len); + getSocketAddressInfo(*client_socket.get(), server_ip->port(), server_addr, addr_len); ASSERT_GT(addr_len, 0); - if (version_ == Address::IpVersion::v4) { - struct sockaddr_in* servaddr = reinterpret_cast(&server_addr); - servaddr->sin_port = htons(server_ip->port()); - } else if (version_ == Address::IpVersion::v6) { - struct sockaddr_in6* servaddr = reinterpret_cast(&server_addr); - servaddr->sin6_port = htons(server_ip->port()); - } - // When the `receive` system call returns an error, we expect the `onError` // callback callwed with `SYSCALL_ERROR` parameter. const std::string first("first"); From c375b542be861cfd4efa49deceaf5dacefa19c4b Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Fri, 11 Jan 2019 18:29:13 -0800 Subject: [PATCH 12/32] Added recvFrom in UdpListener. Signed-off-by: Jojy G Varghese --- source/common/network/udp_listener_impl.cc | 48 +++++++++++++++------- source/common/network/udp_listener_impl.h | 7 +++- test/common/network/listener_impl_test.cc | 46 ++++++++++----------- 3 files changed, 60 insertions(+), 41 deletions(-) diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index 92980072c64a7..e8f8b6009fb9d 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -36,8 +36,30 @@ void UdpListenerImpl::disable() { event_del(&raw_event_); } void UdpListenerImpl::enable() { event_add(&raw_event_, nullptr); } -Buffer::InstancePtr UdpListenerImpl::getBufferImpl() { - return std::make_unique(); +UdpListenerImpl::ReceiveResult UdpListenerImpl::doRecvFrom(sockaddr_storage& peer_addr, + socklen_t& addr_len) { + constexpr uint64_t const read_length = 16384; + + Buffer::InstancePtr buffer = std::make_unique(); + + addr_len = sizeof(sockaddr_storage); + memset(&peer_addr, 0, addr_len); + + Buffer::RawSlice slice; + const uint64_t num_slices = buffer->reserve(read_length, &slice, 1); + + ASSERT(num_slices == 1); + // TODO(conqerAtapple): Use os_syscalls + const ssize_t rc = ::recvfrom(socket_.fd(), slice.mem_, read_length, 0, + reinterpret_cast(&peer_addr), &addr_len); + if (rc < 0) { + return ReceiveResult{Api::SysCallIntResult{static_cast(rc), errno}, nullptr}; + } + + slice.len_ = std::min(slice.len_, static_cast(rc)); + buffer->commit(&slice, 1); + + return ReceiveResult{Api::SysCallIntResult{static_cast(rc), 0}, std::move(buffer)}; } void UdpListenerImpl::eventCallback(int fd, short flags, void* arg) { @@ -60,23 +82,18 @@ void UdpListenerImpl::handleReadCallback(int fd) { RELEASE_ASSERT(fd == socket_.fd(), fmt::format("Invalid socket descriptor received in callback {}", fd)); - // TODO(conqerAtAppple): Make this configurable or get from system. - constexpr uint64_t const read_length = 16384; - Buffer::InstancePtr buffer = getBufferImpl(); - ASSERT(buffer); - sockaddr_storage addr; socklen_t addr_len; - Api::SysCallIntResult result; + ReceiveResult recv_result; do { - result = buffer->recvFrom(fd, read_length, addr, addr_len); - if ((result.rc_ < 0) && (result.rc_ != -EAGAIN)) { - cb_.onError(UdpListenerCallbacks::ErrorCode::SYSCALL_ERROR, result.errno_); + recv_result = doRecvFrom(addr, addr_len); + if ((recv_result.result_.rc_ < 0) && (recv_result.result_.rc_ != -EAGAIN)) { + cb_.onError(UdpListenerCallbacks::ErrorCode::SYSCALL_ERROR, recv_result.result_.errno_); return; } - } while (result.rc_ == -EAGAIN); + } while (recv_result.result_.rc_ == -EAGAIN); Address::InstanceConstSharedPtr local_address = socket_.localAddress(); @@ -120,7 +137,8 @@ void UdpListenerImpl::handleReadCallback(int fd) { RELEASE_ASSERT(false, fmt::format("Unsupported address family: {}, local address: {}, receive size: " "{}, address length: {}", - addr.ss_family, local_address->asString(), result.rc_, addr_len)); + addr.ss_family, local_address->asString(), recv_result.result_.rc_, + addr_len)); break; } @@ -133,9 +151,9 @@ void UdpListenerImpl::handleReadCallback(int fd) { bool expected = true; if (is_first_.compare_exchange_strong(expected, false)) { - cb_.onNewConnection(local_address, peer_address, std::move(buffer)); + cb_.onNewConnection(local_address, peer_address, std::move(recv_result.buffer_)); } else { - cb_.onData(local_address, peer_address, std::move(buffer)); + cb_.onData(local_address, peer_address, std::move(recv_result.buffer_)); } } diff --git a/source/common/network/udp_listener_impl.h b/source/common/network/udp_listener_impl.h index bf4b50ea60140..1bae76864ecb4 100644 --- a/source/common/network/udp_listener_impl.h +++ b/source/common/network/udp_listener_impl.h @@ -21,8 +21,13 @@ class UdpListenerImpl : public BaseListenerImpl, public Event::ImplBase { virtual void disable() override; virtual void enable() override; + struct ReceiveResult { + Api::SysCallIntResult result_; + Buffer::InstancePtr buffer_; + }; + // Useful for testing/mocking. - virtual Buffer::InstancePtr getBufferImpl(); + virtual ReceiveResult doRecvFrom(sockaddr_storage& peer_addr, socklen_t& addr_len); protected: void handleWriteCallback(int fd); diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index a74cb1f785611..df7bd2ee370e7 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -79,24 +79,16 @@ class TestListenerImpl : public ListenerImpl { MOCK_METHOD1(getLocalAddress, Address::InstanceConstSharedPtr(int fd)); }; -class MockResultBufferImpl : public Buffer::OwnedImpl { -public: - MockResultBufferImpl(const Api::SysCallIntResult& result) : result_(result) {} - - Api::SysCallIntResult recvFrom(int, uint64_t, sockaddr_storage&, socklen_t&) override { - return result_; - } - -private: - const Api::SysCallIntResult result_; -}; - class TestUdpListenerImpl : public UdpListenerImpl { public: TestUdpListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb) : UdpListenerImpl(dispatcher, socket, cb) {} - MOCK_METHOD0(getBufferImpl, Buffer::InstancePtr()); + MOCK_METHOD2(doRecvFrom, UdpListenerImpl::ReceiveResult(sockaddr_storage& peer_addr, socklen_t& addr_len)); + + UdpListenerImpl::ReceiveResult doRecvFrom_(sockaddr_storage& peer_addr, socklen_t& addr_len) { + return UdpListenerImpl::doRecvFrom(peer_addr, addr_len); + } }; class ListenerImplTest : public testing::TestWithParam { @@ -284,9 +276,10 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { Network::MockUdpListenerCallbacks listener_callbacks; Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); - EXPECT_CALL(listener, getBufferImpl()).WillRepeatedly(Invoke([&]() { - return std::make_unique(); - })); + EXPECT_CALL(listener, doRecvFrom(_, _)) + .WillRepeatedly(Invoke([&](sockaddr_storage& peer_addr, socklen_t& addr_len) { + return listener.doRecvFrom_(peer_addr, addr_len); + })); // Setup client socket. SocketPtr client_socket = @@ -389,9 +382,10 @@ TEST_P(ListenerImplTest, UdpEcho) { Network::MockUdpListenerCallbacks listener_callbacks; Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); - EXPECT_CALL(listener, getBufferImpl()).WillRepeatedly(Invoke([&]() { - return std::make_unique(); - })); + EXPECT_CALL(listener, doRecvFrom(_, _)) + .WillRepeatedly(Invoke([&](sockaddr_storage& peer_addr, socklen_t& addr_len) { + return listener.doRecvFrom_(peer_addr, addr_len); + })); // Setup client socket. SocketPtr client_socket = @@ -544,9 +538,10 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { Network::MockUdpListenerCallbacks listener_callbacks; Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); - EXPECT_CALL(listener, getBufferImpl()).WillRepeatedly(Invoke([&]() { - return std::make_unique(); - })); + EXPECT_CALL(listener, doRecvFrom(_, _)) + .WillRepeatedly(Invoke([&](sockaddr_storage& peer_addr, socklen_t& addr_len) { + return listener.doRecvFrom_(peer_addr, addr_len); + })); // Setup client socket. SocketPtr client_socket = @@ -656,9 +651,10 @@ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { Network::MockUdpListenerCallbacks listener_callbacks; Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); - EXPECT_CALL(listener, getBufferImpl()).WillRepeatedly(Invoke([&]() { - return std::make_unique(Api::SysCallIntResult{-1, -1}); - })); + EXPECT_CALL(listener, doRecvFrom(_, _)) + .WillRepeatedly(Invoke([&](sockaddr_storage&, socklen_t&) { + return UdpListenerImpl::ReceiveResult{{-1, -1}, nullptr}; + })); SocketPtr client_socket = getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), From 8e49e6b5c32e5edce6cc4e7a0d6ef4a0bfe5cdc0 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Fri, 11 Jan 2019 18:45:01 -0800 Subject: [PATCH 13/32] Removed recvFrom from Buffer. Signed-off-by: Jojy G Varghese --- include/envoy/buffer/buffer.h | 3 - include/envoy/network/listener.h | 16 ++--- source/common/buffer/buffer_impl.cc | 26 -------- source/common/buffer/buffer_impl.h | 2 - source/common/network/udp_listener_impl.cc | 9 +-- source/common/network/udp_listener_impl.h | 2 - .../network/dubbo_proxy/buffer_helper.h | 3 - test/common/network/listener_impl_test.cc | 62 ++++--------------- test/mocks/network/mocks.h | 14 +---- 9 files changed, 20 insertions(+), 117 deletions(-) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index 94fd167b9e1eb..1bc200839b82a 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -168,9 +168,6 @@ class Instance { */ virtual Api::SysCallIntResult read(int fd, uint64_t max_length) PURE; - virtual Api::SysCallIntResult recvFrom(int fd, uint64_t max_length, - sockaddr_storage& peer_address, - socklen_t& sock_length) PURE; /** * Reserve space in the buffer. * @param length supplies the amount of space to reserve. diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index 8e128d3055854..1e89e77f3c120 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -123,17 +123,6 @@ class UdpListenerCallbacks { virtual ~UdpListenerCallbacks() = default; - /** - * On first packet received on a udp socket. This allows the callback handler - * to establish filter chain (or any other preparation). - * - * @param local_address Local bound socket network address. - * @param peer_address Network address of the peer. - * @param data Data buffer received. - */ - virtual void onNewConnection(Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address, - Buffer::InstancePtr&& data) PURE; /** * Called whenever data is received by the underlying udp socket. * @@ -145,6 +134,11 @@ class UdpListenerCallbacks { Address::InstanceConstSharedPtr peer_address, Buffer::InstancePtr&& data) PURE; + /** + * Called when the underlying socket is ready for write. + * + * @param socket Underlying server socket for the listener. + */ virtual void onWriteReady(const Socket& socket) PURE; /** diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 19a639d0ceaa4..192920f6b0575 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -110,32 +110,6 @@ void OwnedImpl::move(Instance& rhs, uint64_t length) { static_cast(rhs).postProcess(); } -Api::SysCallIntResult OwnedImpl::recvFrom(int fd, uint64_t max_length, sockaddr_storage& peer_addr, - socklen_t& addr_len) { - if (max_length == 0) { - return {0, 0}; - } - - addr_len = sizeof(sockaddr_storage); - memset(&peer_addr, 0, addr_len); - - RawSlice slice; - const uint64_t num_slices = reserve(max_length, &slice, 1); - - ASSERT(num_slices == 1); - // TODO(conqerAtapple): Use os_syscalls - const ssize_t rc = ::recvfrom(fd, slice.mem_, max_length, 0, - reinterpret_cast(&peer_addr), &addr_len); - if (rc < 0) { - return {static_cast(rc), errno}; - } - - slice.len_ = std::min(slice.len_, static_cast(rc)); - commit(&slice, 1); - - return {static_cast(rc), 0}; -} - Api::SysCallIntResult OwnedImpl::read(int fd, uint64_t max_length) { if (max_length == 0) { return {0, 0}; diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index f068aabf3dc21..e215d53ea81c8 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -83,8 +83,6 @@ class OwnedImpl : public LibEventInstance { void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; Api::SysCallIntResult read(int fd, uint64_t max_length) override; - Api::SysCallIntResult recvFrom(int fd, uint64_t max_length, sockaddr_storage& peer_address, - socklen_t& sock_length) override; uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; ssize_t search(const void* data, uint64_t size, size_t start) const override; Api::SysCallIntResult write(int fd) override; diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index e8f8b6009fb9d..8019a24f99f29 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -20,7 +20,7 @@ namespace Network { UdpListenerImpl::UdpListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb) - : BaseListenerImpl(dispatcher, socket), cb_(cb), is_first_(true) { + : BaseListenerImpl(dispatcher, socket), cb_(cb) { event_assign(&raw_event_, &dispatcher.base(), socket.fd(), EV_READ | EV_WRITE | EV_PERSIST, eventCallback, this); event_add(&raw_event_, nullptr); @@ -149,12 +149,7 @@ void UdpListenerImpl::handleReadCallback(int fd) { RELEASE_ASSERT((local_address != nullptr), fmt::format("Unable to get local address for fd: {}", fd)); - bool expected = true; - if (is_first_.compare_exchange_strong(expected, false)) { - cb_.onNewConnection(local_address, peer_address, std::move(recv_result.buffer_)); - } else { - cb_.onData(local_address, peer_address, std::move(recv_result.buffer_)); - } + cb_.onData(local_address, peer_address, std::move(recv_result.buffer_)); } void UdpListenerImpl::handleWriteCallback(int fd) { diff --git a/source/common/network/udp_listener_impl.h b/source/common/network/udp_listener_impl.h index 1bae76864ecb4..126269cbea1d9 100644 --- a/source/common/network/udp_listener_impl.h +++ b/source/common/network/udp_listener_impl.h @@ -37,8 +37,6 @@ class UdpListenerImpl : public BaseListenerImpl, public Event::ImplBase { private: static void eventCallback(int fd, short flags, void* arg); - - std::atomic is_first_; }; } // namespace Network diff --git a/source/extensions/filters/network/dubbo_proxy/buffer_helper.h b/source/extensions/filters/network/dubbo_proxy/buffer_helper.h index a826f8383bf56..8b78cc7ff2875 100644 --- a/source/extensions/filters/network/dubbo_proxy/buffer_helper.h +++ b/source/extensions/filters/network/dubbo_proxy/buffer_helper.h @@ -53,9 +53,6 @@ class BufferWrapper : public Buffer::Instance { void move(Buffer::Instance&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } void move(Buffer::Instance&, uint64_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } Api::SysCallIntResult read(int, uint64_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } - Api::SysCallIntResult recvFrom(int, uint64_t, sockaddr_storage&, socklen_t&) override { - NOT_IMPLEMENTED_GCOVR_EXCL_LINE; - } uint64_t reserve(uint64_t, Buffer::RawSlice*, uint64_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index df7bd2ee370e7..8c23cb14934b4 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -84,7 +84,8 @@ class TestUdpListenerImpl : public UdpListenerImpl { TestUdpListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb) : UdpListenerImpl(dispatcher, socket, cb) {} - MOCK_METHOD2(doRecvFrom, UdpListenerImpl::ReceiveResult(sockaddr_storage& peer_addr, socklen_t& addr_len)); + MOCK_METHOD2(doRecvFrom, + UdpListenerImpl::ReceiveResult(sockaddr_storage& peer_addr, socklen_t& addr_len)); UdpListenerImpl::ReceiveResult doRecvFrom_(sockaddr_storage& peer_addr, socklen_t& addr_len) { return UdpListenerImpl::doRecvFrom(peer_addr, addr_len); @@ -290,15 +291,12 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { sockaddr_storage server_addr; socklen_t addr_len; - getSocketAddressInfo(*client_socket.get(),server_ip->port(), server_addr, addr_len); + getSocketAddressInfo(*client_socket.get(), server_ip->port(), server_addr, addr_len); ASSERT_GT(addr_len, 0); - // We send 3 packets - // - First one should invoke `onNewConnection` callback. - // - Second and third one should invoke `onData` callback. + // We send 2 packets const std::string first("first"); const std::string second("second"); - const std::string third("third"); auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, reinterpret_cast(&server_addr), addr_len); @@ -330,29 +328,20 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { EXPECT_EQ(*local_address, *server_socket->localAddress()); }; - EXPECT_CALL(listener_callbacks, onNewConnection_(_, _, _)) - .WillOnce( - Invoke([&](Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { - validateCallParams(local_address, peer_address); - - EXPECT_EQ(data->toString(), first); - })); - EXPECT_CALL(listener_callbacks, onData_(_, _, _)) .WillOnce( Invoke([&](Address::InstanceConstSharedPtr local_address, Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { validateCallParams(local_address, peer_address); - EXPECT_EQ(data->toString(), second); + EXPECT_EQ(data->toString(), first); })) .WillOnce( Invoke([&](Address::InstanceConstSharedPtr local_address, Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { validateCallParams(local_address, peer_address); - EXPECT_EQ(data->toString(), third); + EXPECT_EQ(data->toString(), second); dispatcher_.exit(); })); @@ -437,19 +426,6 @@ TEST_P(ListenerImplTest, UdpEcho) { Address::InstanceConstSharedPtr test_peer_address; std::vector server_received_data; - EXPECT_CALL(listener_callbacks, onNewConnection_(_, _, _)) - .WillOnce( - Invoke([&](Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { - validateCallParams(local_address, peer_address); - - test_peer_address = peer_address; - - const std::string data_str = data->toString(); - EXPECT_EQ(data_str, first); - - server_received_data.push_back(data_str); - })); EXPECT_CALL(listener_callbacks, onData_(_, _, _)) .WillOnce( @@ -463,7 +439,6 @@ TEST_P(ListenerImplTest, UdpEcho) { server_received_data.push_back(data_str); })); - EXPECT_CALL(listener_callbacks, onWriteReady_(_)) .WillRepeatedly(Invoke([&](const Socket& socket) { EXPECT_EQ(socket.fd(), server_socket->fd()); @@ -555,16 +530,12 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { getSocketAddressInfo(*client_socket.get(), server_ip->port(), server_addr, addr_len); ASSERT_GT(addr_len, 0); - // We first disable the listener and then send three packets. + // We first disable the listener and then send two packets. // - With the listener disabled, we expect that none of the callbacks will be // called. // - When the listener is enabled back, we expect the callbacks to be called - // such that: - // - First one should invoke `onNewConnection` callback. - // - Second and third one should invoke `onData` callback. const std::string first("first"); const std::string second("second"); - const std::string third("third"); listener.disable(); @@ -578,11 +549,6 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { ASSERT_EQ(send_rc, second.length()); - send_rc = ::sendto(client_sockfd, third.c_str(), third.length(), 0, - reinterpret_cast(&server_addr), addr_len); - - ASSERT_EQ(send_rc, third.length()); - auto validateCallParams = [&](Address::InstanceConstSharedPtr local_address, Address::InstanceConstSharedPtr peer_address) { ASSERT_NE(local_address, nullptr); @@ -602,8 +568,6 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { timer->enableTimer(std::chrono::milliseconds(2000)); - EXPECT_CALL(listener_callbacks, onNewConnection_(_, _, _)).Times(0); - EXPECT_CALL(listener_callbacks, onData_(_, _, _)).Times(0); EXPECT_CALL(listener_callbacks, onWriteReady_(_)).Times(0); @@ -612,7 +576,6 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { listener.enable(); - EXPECT_CALL(listener_callbacks, onNewConnection_(_, _, _)).Times(1); EXPECT_CALL(listener_callbacks, onData_(_, _, _)) .Times(2) .WillOnce(Return()) @@ -621,7 +584,7 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { validateCallParams(local_address, peer_address); - EXPECT_EQ(data->toString(), third); + EXPECT_EQ(data->toString(), second); dispatcher_.exit(); })); @@ -651,10 +614,9 @@ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { Network::MockUdpListenerCallbacks listener_callbacks; Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); - EXPECT_CALL(listener, doRecvFrom(_, _)) - .WillRepeatedly(Invoke([&](sockaddr_storage&, socklen_t&) { - return UdpListenerImpl::ReceiveResult{{-1, -1}, nullptr}; - })); + EXPECT_CALL(listener, doRecvFrom(_, _)).WillRepeatedly(Invoke([&](sockaddr_storage&, socklen_t&) { + return UdpListenerImpl::ReceiveResult{{-1, -1}, nullptr}; + })); SocketPtr client_socket = getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), @@ -676,7 +638,7 @@ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { ASSERT_EQ(send_rc, first.length()); - EXPECT_CALL(listener_callbacks, onNewConnection_(_, _, _)).Times(0); + EXPECT_CALL(listener_callbacks, onData_(_, _, _)).Times(0); EXPECT_CALL(listener_callbacks, onWriteReady_(_)) .Times(1) diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index fd7022cccf6c4..0dbe429000753 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -122,27 +122,15 @@ class MockUdpListenerCallbacks : public UdpListenerCallbacks { MockUdpListenerCallbacks(); ~MockUdpListenerCallbacks(); - void onNewConnection(Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address, - Buffer::InstancePtr&& data) override { - onNewConnection_(local_address, peer_address, data.get()); - } - void onData(Address::InstanceConstSharedPtr local_address, Address::InstanceConstSharedPtr peer_address, Buffer::InstancePtr&& data) override { onData_(local_address, peer_address, data.get()); } - void onWriteReady(const Socket& socket) override { - onWriteReady_(socket); - } + void onWriteReady(const Socket& socket) override { onWriteReady_(socket); } void onError(const ErrorCode& err_code, int err) override { onError_(err_code, err); } - MOCK_METHOD3(onNewConnection_, - void(Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data)); - MOCK_METHOD3(onData_, void(Address::InstanceConstSharedPtr local_address, Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data)); From 0749e32b7495583eeac566bc329aa46a1546b3f4 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Fri, 11 Jan 2019 20:35:41 -0800 Subject: [PATCH 14/32] Fix tests. Signed-off-by: Jojy G Varghese --- test/common/network/listener_impl_test.cc | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 8c23cb14934b4..91f5cee4e277f 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -308,11 +308,6 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { ASSERT_EQ(send_rc, second.length()); - send_rc = ::sendto(client_sockfd, third.c_str(), third.length(), 0, - reinterpret_cast(&server_addr), addr_len); - - ASSERT_EQ(send_rc, third.length()); - auto validateCallParams = [&](Address::InstanceConstSharedPtr local_address, Address::InstanceConstSharedPtr peer_address) { ASSERT_NE(local_address, nullptr); @@ -422,7 +417,6 @@ TEST_P(ListenerImplTest, UdpEcho) { timer->enableTimer(std::chrono::milliseconds(2000)); // For unit test purposes, we assume that the data was received in order. - Address::InstanceConstSharedPtr test_peer_address; std::vector server_received_data; @@ -433,6 +427,18 @@ TEST_P(ListenerImplTest, UdpEcho) { Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { validateCallParams(local_address, peer_address); + test_peer_address = peer_address; + + const std::string data_str = data->toString(); + EXPECT_EQ(data_str, first); + + server_received_data.push_back(data_str); + })) + .WillOnce( + Invoke([&](Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { + validateCallParams(local_address, peer_address); + const std::string data_str = data->toString(); EXPECT_EQ(data_str, second); @@ -445,6 +451,8 @@ TEST_P(ListenerImplTest, UdpEcho) { sockaddr_storage client_addr; socklen_t client_addr_len; + + ASSERT_NE(test_peer_address, nullptr); const uint32_t peer_port = test_peer_address->ip()->port(); getSocketAddressInfo(test_peer_address, peer_port, client_addr, client_addr_len); From 7ae3884ea303b3e73473acd1d2cfd236e202cad6 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Sun, 13 Jan 2019 14:39:36 -0800 Subject: [PATCH 15/32] Added UdpData structure. Signed-off-by: Jojy G Varghese --- include/envoy/network/listener.h | 21 ++++-- source/common/network/udp_listener_impl.cc | 2 +- test/common/network/listener_impl_test.cc | 74 ++++++++++------------ test/mocks/network/mocks.h | 8 +-- 4 files changed, 50 insertions(+), 55 deletions(-) diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index 1e89e77f3c120..8f451107aa730 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -114,6 +114,19 @@ class ListenerCallbacks { virtual void onNewConnection(ConnectionPtr&& new_connection) PURE; }; +/** + * Utility struct that encapsulates the information from a udp socket's + * recvfrom/recvmmdg call. + * + * TODO (conqerAtapple): Maybe this belongs inside the UdpListenerCallbacks + * class. + */ +struct UdpData { + Address::InstanceConstSharedPtr local_address_; + Address::InstanceConstSharedPtr peer_address_; + Buffer::InstancePtr buffer_; +}; + /** * Udp listener callbacks. */ @@ -126,13 +139,9 @@ class UdpListenerCallbacks { /** * Called whenever data is received by the underlying udp socket. * - * @param local_address Local bound socket network address. - * @param peer_address Network address of the peer. - * @param data Data buffer received. + * @param data UdpData from the underlying socket. */ - virtual void onData(Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address, - Buffer::InstancePtr&& data) PURE; + virtual void onData(const UdpData& data) PURE; /** * Called when the underlying socket is ready for write. diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index 8019a24f99f29..38979f4aa5e13 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -149,7 +149,7 @@ void UdpListenerImpl::handleReadCallback(int fd) { RELEASE_ASSERT((local_address != nullptr), fmt::format("Unable to get local address for fd: {}", fd)); - cb_.onData(local_address, peer_address, std::move(recv_result.buffer_)); + cb_.onData(UdpData{local_address, peer_address, std::move(recv_result.buffer_)}); } void UdpListenerImpl::handleWriteCallback(int fd) { diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 91f5cee4e277f..696fa10822a5b 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -323,23 +323,19 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { EXPECT_EQ(*local_address, *server_socket->localAddress()); }; - EXPECT_CALL(listener_callbacks, onData_(_, _, _)) - .WillOnce( - Invoke([&](Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { - validateCallParams(local_address, peer_address); + EXPECT_CALL(listener_callbacks, onData_(_)) + .WillOnce(Invoke([&](const UdpData& data) -> void { + validateCallParams(data.local_address_, data.peer_address_); - EXPECT_EQ(data->toString(), first); - })) - .WillOnce( - Invoke([&](Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { - validateCallParams(local_address, peer_address); + EXPECT_EQ(data.buffer_->toString(), first); + })) + .WillOnce(Invoke([&](const UdpData& data) -> void { + validateCallParams(data.local_address_, data.peer_address_); - EXPECT_EQ(data->toString(), second); + EXPECT_EQ(data.buffer_->toString(), second); - dispatcher_.exit(); - })); + dispatcher_.exit(); + })); EXPECT_CALL(listener_callbacks, onWriteReady_(_)) .WillRepeatedly( @@ -421,29 +417,25 @@ TEST_P(ListenerImplTest, UdpEcho) { std::vector server_received_data; - EXPECT_CALL(listener_callbacks, onData_(_, _, _)) - .WillOnce( - Invoke([&](Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { - validateCallParams(local_address, peer_address); + EXPECT_CALL(listener_callbacks, onData_(_)) + .WillOnce(Invoke([&](const UdpData& data) -> void { + validateCallParams(data.local_address_, data.peer_address_); - test_peer_address = peer_address; + test_peer_address = data.peer_address_; - const std::string data_str = data->toString(); - EXPECT_EQ(data_str, first); + const std::string data_str = data.buffer_->toString(); + EXPECT_EQ(data_str, first); - server_received_data.push_back(data_str); - })) - .WillOnce( - Invoke([&](Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { - validateCallParams(local_address, peer_address); + server_received_data.push_back(data_str); + })) + .WillOnce(Invoke([&](const UdpData& data) -> void { + validateCallParams(data.local_address_, data.peer_address_); - const std::string data_str = data->toString(); - EXPECT_EQ(data_str, second); + const std::string data_str = data.buffer_->toString(); + EXPECT_EQ(data_str, second); - server_received_data.push_back(data_str); - })); + server_received_data.push_back(data_str); + })); EXPECT_CALL(listener_callbacks, onWriteReady_(_)) .WillRepeatedly(Invoke([&](const Socket& socket) { @@ -576,7 +568,7 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { timer->enableTimer(std::chrono::milliseconds(2000)); - EXPECT_CALL(listener_callbacks, onData_(_, _, _)).Times(0); + EXPECT_CALL(listener_callbacks, onData_(_)).Times(0); EXPECT_CALL(listener_callbacks, onWriteReady_(_)).Times(0); @@ -584,18 +576,16 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { listener.enable(); - EXPECT_CALL(listener_callbacks, onData_(_, _, _)) + EXPECT_CALL(listener_callbacks, onData_(_)) .Times(2) .WillOnce(Return()) - .WillOnce( - Invoke([&](Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data) -> void { - validateCallParams(local_address, peer_address); + .WillOnce(Invoke([&](const UdpData& data) -> void { + validateCallParams(data.local_address_, data.peer_address_); - EXPECT_EQ(data->toString(), second); + EXPECT_EQ(data.buffer_->toString(), second); - dispatcher_.exit(); - })); + dispatcher_.exit(); + })); EXPECT_CALL(listener_callbacks, onWriteReady_(_)) .WillRepeatedly( @@ -646,7 +636,7 @@ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { ASSERT_EQ(send_rc, first.length()); - EXPECT_CALL(listener_callbacks, onData_(_, _, _)).Times(0); + EXPECT_CALL(listener_callbacks, onData_(_)).Times(0); EXPECT_CALL(listener_callbacks, onWriteReady_(_)) .Times(1) diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 0dbe429000753..5ec2f8183e568 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -122,17 +122,13 @@ class MockUdpListenerCallbacks : public UdpListenerCallbacks { MockUdpListenerCallbacks(); ~MockUdpListenerCallbacks(); - void onData(Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address, Buffer::InstancePtr&& data) override { - onData_(local_address, peer_address, data.get()); - } + void onData(const UdpData& data) override { onData_(data); } void onWriteReady(const Socket& socket) override { onWriteReady_(socket); } void onError(const ErrorCode& err_code, int err) override { onError_(err_code, err); } - MOCK_METHOD3(onData_, void(Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address, Buffer::Instance* data)); + MOCK_METHOD1(onData_, void(const UdpData& data)); MOCK_METHOD1(onWriteReady_, void(const Socket& socket)); From 4c25d18c1359fa6dc595dfba0bdcf8aa02d978e4 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Sun, 13 Jan 2019 14:58:25 -0800 Subject: [PATCH 16/32] s/createUdpListener/createDatagramListener Signed-off-by: Jojy G Varghese --- include/envoy/event/dispatcher.h | 4 ++-- source/common/event/dispatcher_impl.cc | 4 ++-- source/common/event/dispatcher_impl.h | 4 ++-- test/mocks/event/mocks.h | 8 ++++---- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 9dff305341368..c0de6b9d072a2 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -116,8 +116,8 @@ class Dispatcher { * @param cb supplies the udp listener callbacks to invoke for listener events. * @return Network::ListenerPtr a new listener that is owned by the caller. */ - virtual Network::ListenerPtr createUdpListener(Network::Socket& socket, - Network::UdpListenerCallbacks& cb) PURE; + virtual Network::ListenerPtr createDatagramListener(Network::Socket& socket, + Network::UdpListenerCallbacks& cb) PURE; /** * Allocate a timer. @see Timer for docs on how to use the timer. * @param cb supplies the callback to invoke when the timer fires. diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index ced376e638dc5..0fd565b0f77ea 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -120,8 +120,8 @@ DispatcherImpl::createListener(Network::Socket& socket, Network::ListenerCallbac hand_off_restored_destination_connections)}; } -Network::ListenerPtr DispatcherImpl::createUdpListener(Network::Socket& socket, - Network::UdpListenerCallbacks& cb) { +Network::ListenerPtr DispatcherImpl::createDatagramListener(Network::Socket& socket, + Network::UdpListenerCallbacks& cb) { ASSERT(isThreadSafe()); return Network::ListenerPtr{new Network::UdpListenerImpl(*this, socket, cb)}; } diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index fc0cf5326401d..3788b9a2578ab 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -51,8 +51,8 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { Network::ListenerPtr createListener(Network::Socket& socket, Network::ListenerCallbacks& cb, bool bind_to_port, bool hand_off_restored_destination_connections) override; - Network::ListenerPtr createUdpListener(Network::Socket& socket, - Network::UdpListenerCallbacks& cb) override; + Network::ListenerPtr createDatagramListener(Network::Socket& socket, + Network::UdpListenerCallbacks& cb) override; TimerPtr createTimer(TimerCb cb) override; void deferredDelete(DeferredDeletablePtr&& to_delete) override; void exit() override; diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 55ef6403498a0..539f661d71e3a 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -65,9 +65,9 @@ class MockDispatcher : public Dispatcher { createListener_(socket, cb, bind_to_port, hand_off_restored_destination_connections)}; } - Network::ListenerPtr createUdpListener(Network::Socket& socket, - Network::UdpListenerCallbacks& cb) override { - return Network::ListenerPtr{createUdpListener_(socket, cb)}; + Network::ListenerPtr createDatagramListener(Network::Socket& socket, + Network::UdpListenerCallbacks& cb) override { + return Network::ListenerPtr{createDatagramListener_(socket, cb)}; } Event::TimerPtr createTimer(Event::TimerCb cb) override { @@ -106,7 +106,7 @@ class MockDispatcher : public Dispatcher { Network::Listener*(Network::Socket& socket, Network::ListenerCallbacks& cb, bool bind_to_port, bool hand_off_restored_destination_connections)); - MOCK_METHOD2(createUdpListener_, + MOCK_METHOD2(createDatagramListener_, Network::Listener*(Network::Socket& socket, Network::UdpListenerCallbacks& cb)); MOCK_METHOD1(createTimer_, Timer*(Event::TimerCb cb)); MOCK_METHOD1(deferredDelete_, void(DeferredDeletable* to_delete)); From e85347d156ae7e94c362ee4f9758ce6f7289ee92 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Mon, 14 Jan 2019 08:28:58 -0800 Subject: [PATCH 17/32] Adding some comments for ideas to be discussed. Signed-off-by: Jojy G Varghese --- include/envoy/network/listener.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index 8f451107aa730..b1c198f2662cb 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -125,6 +125,10 @@ struct UdpData { Address::InstanceConstSharedPtr local_address_; Address::InstanceConstSharedPtr peer_address_; Buffer::InstancePtr buffer_; + // TODO (conquerAtapple): + // Add UdpReader here so that the callback handler can + // then use the reader to do multiple reads(recvmmsg) once the OS notifies it + // has data. }; /** @@ -147,6 +151,8 @@ class UdpListenerCallbacks { * Called when the underlying socket is ready for write. * * @param socket Underlying server socket for the listener. + * + * TODO (conqerAtapple): Maybe we need a UdpWriter here instead of Socket. */ virtual void onWriteReady(const Socket& socket) PURE; From 17ba1237051c656d3b83ab99a64d99e38c8217f6 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Wed, 16 Jan 2019 11:33:37 -0800 Subject: [PATCH 18/32] Moved udp tests to its own file. Signed-off-by: Jojy G Varghese --- test/common/network/BUILD | 18 + test/common/network/listener_impl_test.cc | 497 +--------------- test/common/network/udp_listener_impl_test.cc | 535 ++++++++++++++++++ 3 files changed, 554 insertions(+), 496 deletions(-) create mode 100644 test/common/network/udp_listener_impl_test.cc diff --git a/test/common/network/BUILD b/test/common/network/BUILD index 61637c16b76d2..0dc910b23e19a 100644 --- a/test/common/network/BUILD +++ b/test/common/network/BUILD @@ -160,6 +160,24 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "udp_listener_impl_test", + srcs = ["udp_listener_impl_test.cc"], + deps = [ + "//source/common/event:dispatcher_lib", + "//source/common/network:address_lib", + "//source/common/network:listener_lib", + "//source/common/network:utility_lib", + "//source/common/stats:stats_lib", + "//test/mocks/network:network_mocks", + "//test/mocks/server:server_mocks", + "//test/test_common:environment_lib", + "//test/test_common:network_utility_lib", + "//test/test_common:test_time_lib", + "//test/test_common:utility_lib", + ], +) + envoy_cc_test( name = "resolver_test", srcs = ["resolver_impl_test.cc"], diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 696fa10822a5b..07c88b8b33e10 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -1,11 +1,5 @@ -#include -#include -#include -#include - #include "common/network/address_impl.h" #include "common/network/listener_impl.h" -#include "common/network/udp_listener_impl.h" #include "common/network/utility.h" #include "test/mocks/network/mocks.h" @@ -79,19 +73,6 @@ class TestListenerImpl : public ListenerImpl { MOCK_METHOD1(getLocalAddress, Address::InstanceConstSharedPtr(int fd)); }; -class TestUdpListenerImpl : public UdpListenerImpl { -public: - TestUdpListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb) - : UdpListenerImpl(dispatcher, socket, cb) {} - - MOCK_METHOD2(doRecvFrom, - UdpListenerImpl::ReceiveResult(sockaddr_storage& peer_addr, socklen_t& addr_len)); - - UdpListenerImpl::ReceiveResult doRecvFrom_(sockaddr_storage& peer_addr, socklen_t& addr_len) { - return UdpListenerImpl::doRecvFrom(peer_addr, addr_len); - } -}; - class ListenerImplTest : public testing::TestWithParam { protected: ListenerImplTest() @@ -100,75 +81,6 @@ class ListenerImplTest : public testing::TestWithParam { Network::Test::getCanonicalLoopbackAddress(version_), Address::SocketType::Stream)), api_(Api::createApiForTest(stats_store_)), dispatcher_(test_time_.timeSystem(), *api_) {} - SocketPtr getSocket(Address::SocketType type, const Address::InstanceConstSharedPtr& address, - const Network::Socket::OptionsSharedPtr& options, bool bind) { - if (type == Address::SocketType::Stream) { - using NetworkSocketTraitType = NetworkSocketTrait; - return std::make_unique>(address, options, bind); - } else if (type == Address::SocketType::Datagram) { - using NetworkSocketTraitType = NetworkSocketTrait; - return std::make_unique>(address, options, bind); - } - - return nullptr; - } - - // TODO(conqerAtapple): Move this to a common place(address.h?) - void getSocketAddressInfo(const Address::Ip* ip, uint32_t port, sockaddr_storage& addr, - socklen_t& sz) { - if (!ip) { - sz = 0; - return; - } - - memset(&addr, 0, sizeof(addr)); - - if (version_ == Address::IpVersion::v4) { - addr.ss_family = AF_INET; - auto const* ipv4 = ip->ipv4(); - if (!ipv4) { - sz = 0; - return; - } - - sockaddr_in* addrv4 = reinterpret_cast(&addr); - addrv4->sin_port = htons(port); - addrv4->sin_addr.s_addr = ipv4->address(); - - sz = sizeof(sockaddr_in); - } else if (version_ == Address::IpVersion::v6) { - addr.ss_family = AF_INET6; - auto const* ipv6 = ip->ipv6(); - if (!ipv6) { - sz = 0; - return; - } - - sockaddr_in6* addrv6 = reinterpret_cast(&addr); - addrv6->sin6_port = htons(port); - - const auto address = ipv6->address(); - memcpy(static_cast(&addrv6->sin6_addr.s6_addr), static_cast(&address), - sizeof(absl::uint128)); - - sz = sizeof(sockaddr_in6); - } else { - sz = 0; - } - } - - void getSocketAddressInfo(const Socket& socket, uint32_t port, sockaddr_storage& addr, - socklen_t& sz) { - getSocketAddressInfo(socket.localAddress()->ip(), port, addr, sz); - } - - void getSocketAddressInfo(Address::InstanceConstSharedPtr address, uint32_t port, - sockaddr_storage& addr, socklen_t& sz) { - ASSERT(address); - - getSocketAddressInfo(address->ip(), port, addr, sz); - } - const Address::IpVersion version_; const Address::InstanceConstSharedPtr alt_address_; Stats::IsolatedStoreImpl stats_store_; @@ -194,17 +106,6 @@ TEST_P(ListenerImplTest, SetListeningSocketOptionsSuccess) { TestListenerImpl listener(dispatcher_, socket, listener_callbacks, true, false); } -// Test that socket options are set after the listener is setup. -TEST_P(ListenerImplTest, UdpSetListeningSocketOptionsSuccess) { - Network::MockListenerCallbacks listener_callbacks; - Network::MockConnectionHandler connection_handler; - - Network::UdpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(version_), nullptr, - true); - std::shared_ptr option = std::make_shared(); - socket.addOption(option); -} - // Test that an exception is thrown if there is an error setting socket options. TEST_P(ListenerImplTest, SetListeningSocketOptionsError) { Network::MockListenerCallbacks listener_callbacks; @@ -222,7 +123,7 @@ TEST_P(ListenerImplTest, SetListeningSocketOptionsError) { socket.localAddress()->asString())); } -TEST_P(ListenerImplTest, UseActualDstTcp) { +TEST_P(ListenerImplTest, UseActualDst) { Stats::IsolatedStoreImpl stats_store; Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); @@ -259,402 +160,6 @@ TEST_P(ListenerImplTest, UseActualDstTcp) { dispatcher_.run(Event::Dispatcher::RunType::Block); } -/** - * Tests UDP listener for actual destination and data. - */ -TEST_P(ListenerImplTest, UseActualDstUdp) { - // Setup server socket - SocketPtr server_socket = - getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), - nullptr, true); - - ASSERT_NE(server_socket, nullptr); - - auto const* server_ip = server_socket->localAddress()->ip(); - ASSERT_NE(server_ip, nullptr); - - // Setup callback handler and listener. - Network::MockUdpListenerCallbacks listener_callbacks; - Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); - - EXPECT_CALL(listener, doRecvFrom(_, _)) - .WillRepeatedly(Invoke([&](sockaddr_storage& peer_addr, socklen_t& addr_len) { - return listener.doRecvFrom_(peer_addr, addr_len); - })); - - // Setup client socket. - SocketPtr client_socket = - getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), - nullptr, false); - - const int client_sockfd = client_socket->fd(); - sockaddr_storage server_addr; - socklen_t addr_len; - - getSocketAddressInfo(*client_socket.get(), server_ip->port(), server_addr, addr_len); - ASSERT_GT(addr_len, 0); - - // We send 2 packets - const std::string first("first"); - const std::string second("second"); - - auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, - reinterpret_cast(&server_addr), addr_len); - - ASSERT_EQ(send_rc, first.length()); - - send_rc = ::sendto(client_sockfd, second.c_str(), second.length(), 0, - reinterpret_cast(&server_addr), addr_len); - - ASSERT_EQ(send_rc, second.length()); - - auto validateCallParams = [&](Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address) { - ASSERT_NE(local_address, nullptr); - - ASSERT_NE(peer_address, nullptr); - ASSERT_NE(peer_address->ip(), nullptr); - - EXPECT_EQ(local_address->asString(), server_socket->localAddress()->asString()); - - EXPECT_EQ(peer_address->ip()->addressAsString(), - client_socket->localAddress()->ip()->addressAsString()); - - EXPECT_EQ(*local_address, *server_socket->localAddress()); - }; - - EXPECT_CALL(listener_callbacks, onData_(_)) - .WillOnce(Invoke([&](const UdpData& data) -> void { - validateCallParams(data.local_address_, data.peer_address_); - - EXPECT_EQ(data.buffer_->toString(), first); - })) - .WillOnce(Invoke([&](const UdpData& data) -> void { - validateCallParams(data.local_address_, data.peer_address_); - - EXPECT_EQ(data.buffer_->toString(), second); - - dispatcher_.exit(); - })); - - EXPECT_CALL(listener_callbacks, onWriteReady_(_)) - .WillRepeatedly( - Invoke([&](const Socket& socket) { EXPECT_EQ(socket.fd(), server_socket->fd()); })); - - dispatcher_.run(Event::Dispatcher::RunType::Block); -} - -/** - * Tests UDP listener for read and write callbacks with actual data. - */ -TEST_P(ListenerImplTest, UdpEcho) { - // Setup server socket - SocketPtr server_socket = - getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), - nullptr, true); - - ASSERT_NE(server_socket, nullptr); - - auto const* server_ip = server_socket->localAddress()->ip(); - ASSERT_NE(server_ip, nullptr); - - // Setup callback handler and listener. - Network::MockUdpListenerCallbacks listener_callbacks; - Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); - - EXPECT_CALL(listener, doRecvFrom(_, _)) - .WillRepeatedly(Invoke([&](sockaddr_storage& peer_addr, socklen_t& addr_len) { - return listener.doRecvFrom_(peer_addr, addr_len); - })); - - // Setup client socket. - SocketPtr client_socket = - getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), - nullptr, false); - - const int client_sockfd = client_socket->fd(); - sockaddr_storage server_addr; - socklen_t addr_len; - - getSocketAddressInfo(*client_socket.get(), server_ip->port(), server_addr, addr_len); - ASSERT_GT(addr_len, 0); - - // We send 2 packets and exptect it to echo. - const std::string first("first"); - const std::string second("second"); - - auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, - reinterpret_cast(&server_addr), addr_len); - - ASSERT_EQ(send_rc, first.length()); - - send_rc = ::sendto(client_sockfd, second.c_str(), second.length(), 0, - reinterpret_cast(&server_addr), addr_len); - - ASSERT_EQ(send_rc, second.length()); - - auto validateCallParams = [&](Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address) { - ASSERT_NE(local_address, nullptr); - - ASSERT_NE(peer_address, nullptr); - ASSERT_NE(peer_address->ip(), nullptr); - - EXPECT_EQ(local_address->asString(), server_socket->localAddress()->asString()); - - EXPECT_EQ(peer_address->ip()->addressAsString(), - client_socket->localAddress()->ip()->addressAsString()); - - EXPECT_EQ(*local_address, *server_socket->localAddress()); - }; - - Event::TimerPtr timer = dispatcher_.createTimer([&] { dispatcher_.exit(); }); - - timer->enableTimer(std::chrono::milliseconds(2000)); - - // For unit test purposes, we assume that the data was received in order. - Address::InstanceConstSharedPtr test_peer_address; - - std::vector server_received_data; - - EXPECT_CALL(listener_callbacks, onData_(_)) - .WillOnce(Invoke([&](const UdpData& data) -> void { - validateCallParams(data.local_address_, data.peer_address_); - - test_peer_address = data.peer_address_; - - const std::string data_str = data.buffer_->toString(); - EXPECT_EQ(data_str, first); - - server_received_data.push_back(data_str); - })) - .WillOnce(Invoke([&](const UdpData& data) -> void { - validateCallParams(data.local_address_, data.peer_address_); - - const std::string data_str = data.buffer_->toString(); - EXPECT_EQ(data_str, second); - - server_received_data.push_back(data_str); - })); - - EXPECT_CALL(listener_callbacks, onWriteReady_(_)) - .WillRepeatedly(Invoke([&](const Socket& socket) { - EXPECT_EQ(socket.fd(), server_socket->fd()); - - sockaddr_storage client_addr; - socklen_t client_addr_len; - - ASSERT_NE(test_peer_address, nullptr); - const uint32_t peer_port = test_peer_address->ip()->port(); - - getSocketAddressInfo(test_peer_address, peer_port, client_addr, client_addr_len); - ASSERT_GT(client_addr_len, 0); - - for (const auto& data : server_received_data) { - const std::string::size_type data_size = data.length() + 1; - uint64_t total_sent = 0; - - do { - auto send_rc = - ::sendto(socket.fd(), data.c_str() + total_sent, data_size - total_sent, 0, - reinterpret_cast(&client_addr), client_addr_len); - - if (send_rc > 0) { - total_sent += send_rc; - } else if (send_rc != -EAGAIN) { - break; - } - } while ((send_rc == -EAGAIN) || (total_sent < data_size)); - - EXPECT_EQ(total_sent, data_size); - } - - server_received_data.clear(); - })); - - Buffer::OwnedImpl client_buffer; - Api::SysCallIntResult result; - - for (const auto& data : server_received_data) { - const std::string::size_type data_size = data.length() + 1; - std::string::size_type remaining = data_size; - do { - result = client_buffer.read(client_socket->fd(), remaining); - if (result.rc_ > 0) { - remaining -= result.rc_; - } else if (result.rc_ != -EAGAIN) { - break; - } - } while ((result.rc_ == -EAGAIN) || (remaining)); - - EXPECT_EQ(remaining, 0); - - EXPECT_EQ(client_buffer.toString(), data); - } - - dispatcher_.run(Event::Dispatcher::RunType::Block); -} - -/** - * Tests UDP listener's `enable` and `disable` APIs. - */ -TEST_P(ListenerImplTest, UdpListenerEnableDisable) { - // Setup server socket - SocketPtr server_socket = - getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), - nullptr, true); - - ASSERT_NE(server_socket, nullptr); - - auto const* server_ip = server_socket->localAddress()->ip(); - ASSERT_NE(server_ip, nullptr); - - // Setup callback handler and listener. - Network::MockUdpListenerCallbacks listener_callbacks; - Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); - - EXPECT_CALL(listener, doRecvFrom(_, _)) - .WillRepeatedly(Invoke([&](sockaddr_storage& peer_addr, socklen_t& addr_len) { - return listener.doRecvFrom_(peer_addr, addr_len); - })); - - // Setup client socket. - SocketPtr client_socket = - getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), - nullptr, false); - - const int client_sockfd = client_socket->fd(); - sockaddr_storage server_addr; - socklen_t addr_len; - - getSocketAddressInfo(*client_socket.get(), server_ip->port(), server_addr, addr_len); - ASSERT_GT(addr_len, 0); - - // We first disable the listener and then send two packets. - // - With the listener disabled, we expect that none of the callbacks will be - // called. - // - When the listener is enabled back, we expect the callbacks to be called - const std::string first("first"); - const std::string second("second"); - - listener.disable(); - - auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, - reinterpret_cast(&server_addr), addr_len); - - ASSERT_EQ(send_rc, first.length()); - - send_rc = ::sendto(client_sockfd, second.c_str(), second.length(), 0, - reinterpret_cast(&server_addr), addr_len); - - ASSERT_EQ(send_rc, second.length()); - - auto validateCallParams = [&](Address::InstanceConstSharedPtr local_address, - Address::InstanceConstSharedPtr peer_address) { - ASSERT_NE(local_address, nullptr); - - ASSERT_NE(peer_address, nullptr); - ASSERT_NE(peer_address->ip(), nullptr); - - ASSERT_EQ(local_address->asString(), server_socket->localAddress()->asString()); - - ASSERT_EQ(peer_address->ip()->addressAsString(), - client_socket->localAddress()->ip()->addressAsString()); - - EXPECT_EQ(*local_address, *server_socket->localAddress()); - }; - - Event::TimerPtr timer = dispatcher_.createTimer([&] { dispatcher_.exit(); }); - - timer->enableTimer(std::chrono::milliseconds(2000)); - - EXPECT_CALL(listener_callbacks, onData_(_)).Times(0); - - EXPECT_CALL(listener_callbacks, onWriteReady_(_)).Times(0); - - dispatcher_.run(Event::Dispatcher::RunType::Block); - - listener.enable(); - - EXPECT_CALL(listener_callbacks, onData_(_)) - .Times(2) - .WillOnce(Return()) - .WillOnce(Invoke([&](const UdpData& data) -> void { - validateCallParams(data.local_address_, data.peer_address_); - - EXPECT_EQ(data.buffer_->toString(), second); - - dispatcher_.exit(); - })); - - EXPECT_CALL(listener_callbacks, onWriteReady_(_)) - .WillRepeatedly( - Invoke([&](const Socket& socket) { EXPECT_EQ(socket.fd(), server_socket->fd()); })); - - dispatcher_.run(Event::Dispatcher::RunType::Block); -} - -/** - * Tests UDP listebe's error callback. - */ -TEST_P(ListenerImplTest, UdpListenerRecvFromError) { - // Setup server socket - SocketPtr server_socket = - getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), - nullptr, true); - - ASSERT_NE(server_socket, nullptr); - - auto const* server_ip = server_socket->localAddress()->ip(); - ASSERT_NE(server_ip, nullptr); - - // Setup callback handler and listener. - Network::MockUdpListenerCallbacks listener_callbacks; - Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); - - EXPECT_CALL(listener, doRecvFrom(_, _)).WillRepeatedly(Invoke([&](sockaddr_storage&, socklen_t&) { - return UdpListenerImpl::ReceiveResult{{-1, -1}, nullptr}; - })); - - SocketPtr client_socket = - getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), - nullptr, false); - - const int client_sockfd = client_socket->fd(); - sockaddr_storage server_addr; - socklen_t addr_len; - - getSocketAddressInfo(*client_socket.get(), server_ip->port(), server_addr, addr_len); - ASSERT_GT(addr_len, 0); - - // When the `receive` system call returns an error, we expect the `onError` - // callback callwed with `SYSCALL_ERROR` parameter. - const std::string first("first"); - - auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, - reinterpret_cast(&server_addr), addr_len); - - ASSERT_EQ(send_rc, first.length()); - - EXPECT_CALL(listener_callbacks, onData_(_)).Times(0); - - EXPECT_CALL(listener_callbacks, onWriteReady_(_)) - .Times(1) - .WillRepeatedly( - Invoke([&](const Socket& socket) { EXPECT_EQ(socket.fd(), server_socket->fd()); })); - - EXPECT_CALL(listener_callbacks, onError_(_, _)) - .Times(1) - .WillOnce(Invoke([&](const UdpListenerCallbacks::ErrorCode& err_code, int err) -> void { - ASSERT_EQ(err_code, UdpListenerCallbacks::ErrorCode::SYSCALL_ERROR); - ASSERT_EQ(err, -1); - - dispatcher_.exit(); - })); - - dispatcher_.run(Event::Dispatcher::RunType::Block); -} - TEST_P(ListenerImplTest, WildcardListenerUseActualDst) { Stats::IsolatedStoreImpl stats_store; Network::TcpListenSocket socket(Network::Test::getAnyAddress(version_), nullptr, true); diff --git a/test/common/network/udp_listener_impl_test.cc b/test/common/network/udp_listener_impl_test.cc new file mode 100644 index 0000000000000..45f10667dd7a5 --- /dev/null +++ b/test/common/network/udp_listener_impl_test.cc @@ -0,0 +1,535 @@ +#include +#include +#include + +#include "common/network/address_impl.h" +#include "common/network/udp_listener_impl.h" +#include "common/network/utility.h" + +#include "test/mocks/network/mocks.h" +#include "test/mocks/server/mocks.h" +#include "test/test_common/environment.h" +#include "test/test_common/network_utility.h" +#include "test/test_common/test_time.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::_; +using testing::Invoke; +using testing::Return; + +namespace Envoy { +namespace Network { + +class TestUdpListenerImpl : public UdpListenerImpl { +public: + TestUdpListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb) + : UdpListenerImpl(dispatcher, socket, cb) {} + + MOCK_METHOD2(doRecvFrom, + UdpListenerImpl::ReceiveResult(sockaddr_storage& peer_addr, socklen_t& addr_len)); + + UdpListenerImpl::ReceiveResult doRecvFrom_(sockaddr_storage& peer_addr, socklen_t& addr_len) { + return UdpListenerImpl::doRecvFrom(peer_addr, addr_len); + } +}; + +class ListenerImplTest : public testing::TestWithParam { +protected: + ListenerImplTest() + : version_(GetParam()), + alt_address_(Network::Test::findOrCheckFreePort( + Network::Test::getCanonicalLoopbackAddress(version_), Address::SocketType::Stream)), + api_(Api::createApiForTest(stats_store_)), dispatcher_(test_time_.timeSystem(), *api_) {} + + SocketPtr getSocket(Address::SocketType type, const Address::InstanceConstSharedPtr& address, + const Network::Socket::OptionsSharedPtr& options, bool bind) { + if (type == Address::SocketType::Stream) { + using NetworkSocketTraitType = NetworkSocketTrait; + return std::make_unique>(address, options, bind); + } else if (type == Address::SocketType::Datagram) { + using NetworkSocketTraitType = NetworkSocketTrait; + return std::make_unique>(address, options, bind); + } + + return nullptr; + } + + // TODO(conqerAtapple): Move this to a common place(address.h?) + void getSocketAddressInfo(const Address::Ip* ip, uint32_t port, sockaddr_storage& addr, + socklen_t& sz) { + if (!ip) { + sz = 0; + return; + } + + memset(&addr, 0, sizeof(addr)); + + if (version_ == Address::IpVersion::v4) { + addr.ss_family = AF_INET; + auto const* ipv4 = ip->ipv4(); + if (!ipv4) { + sz = 0; + return; + } + + sockaddr_in* addrv4 = reinterpret_cast(&addr); + addrv4->sin_port = htons(port); + addrv4->sin_addr.s_addr = ipv4->address(); + + sz = sizeof(sockaddr_in); + } else if (version_ == Address::IpVersion::v6) { + addr.ss_family = AF_INET6; + auto const* ipv6 = ip->ipv6(); + if (!ipv6) { + sz = 0; + return; + } + + sockaddr_in6* addrv6 = reinterpret_cast(&addr); + addrv6->sin6_port = htons(port); + + const auto address = ipv6->address(); + memcpy(static_cast(&addrv6->sin6_addr.s6_addr), static_cast(&address), + sizeof(absl::uint128)); + + sz = sizeof(sockaddr_in6); + } else { + sz = 0; + } + } + + void getSocketAddressInfo(const Socket& socket, uint32_t port, sockaddr_storage& addr, + socklen_t& sz) { + getSocketAddressInfo(socket.localAddress()->ip(), port, addr, sz); + } + + void getSocketAddressInfo(Address::InstanceConstSharedPtr address, uint32_t port, + sockaddr_storage& addr, socklen_t& sz) { + ASSERT(address); + + getSocketAddressInfo(address->ip(), port, addr, sz); + } + + const Address::IpVersion version_; + const Address::InstanceConstSharedPtr alt_address_; + Stats::IsolatedStoreImpl stats_store_; + Api::ApiPtr api_; + DangerousDeprecatedTestTime test_time_; + Event::DispatcherImpl dispatcher_; +}; +INSTANTIATE_TEST_CASE_P(IpVersions, ListenerImplTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + +// Test that socket options are set after the listener is setup. +TEST_P(ListenerImplTest, UdpSetListeningSocketOptionsSuccess) { + Network::MockListenerCallbacks listener_callbacks; + Network::MockConnectionHandler connection_handler; + + Network::UdpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(version_), nullptr, + true); + std::shared_ptr option = std::make_shared(); + socket.addOption(option); +} + +/** + * Tests UDP listener for actual destination and data. + */ +TEST_P(ListenerImplTest, UseActualDstUdp) { + // Setup server socket + SocketPtr server_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, true); + + ASSERT_NE(server_socket, nullptr); + + auto const* server_ip = server_socket->localAddress()->ip(); + ASSERT_NE(server_ip, nullptr); + + // Setup callback handler and listener. + Network::MockUdpListenerCallbacks listener_callbacks; + Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); + + EXPECT_CALL(listener, doRecvFrom(_, _)) + .WillRepeatedly(Invoke([&](sockaddr_storage& peer_addr, socklen_t& addr_len) { + return listener.doRecvFrom_(peer_addr, addr_len); + })); + + // Setup client socket. + SocketPtr client_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, false); + + const int client_sockfd = client_socket->fd(); + sockaddr_storage server_addr; + socklen_t addr_len; + + getSocketAddressInfo(*client_socket.get(), server_ip->port(), server_addr, addr_len); + ASSERT_GT(addr_len, 0); + + // We send 2 packets + const std::string first("first"); + const std::string second("second"); + + auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, first.length()); + + send_rc = ::sendto(client_sockfd, second.c_str(), second.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, second.length()); + + auto validateCallParams = [&](Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address) { + ASSERT_NE(local_address, nullptr); + + ASSERT_NE(peer_address, nullptr); + ASSERT_NE(peer_address->ip(), nullptr); + + EXPECT_EQ(local_address->asString(), server_socket->localAddress()->asString()); + + EXPECT_EQ(peer_address->ip()->addressAsString(), + client_socket->localAddress()->ip()->addressAsString()); + + EXPECT_EQ(*local_address, *server_socket->localAddress()); + }; + + EXPECT_CALL(listener_callbacks, onData_(_)) + .WillOnce(Invoke([&](const UdpData& data) -> void { + validateCallParams(data.local_address_, data.peer_address_); + + EXPECT_EQ(data.buffer_->toString(), first); + })) + .WillOnce(Invoke([&](const UdpData& data) -> void { + validateCallParams(data.local_address_, data.peer_address_); + + EXPECT_EQ(data.buffer_->toString(), second); + + dispatcher_.exit(); + })); + + EXPECT_CALL(listener_callbacks, onWriteReady_(_)) + .WillRepeatedly( + Invoke([&](const Socket& socket) { EXPECT_EQ(socket.fd(), server_socket->fd()); })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); +} + +/** + * Tests UDP listener for read and write callbacks with actual data. + */ +TEST_P(ListenerImplTest, UdpEcho) { + // Setup server socket + SocketPtr server_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, true); + + ASSERT_NE(server_socket, nullptr); + + auto const* server_ip = server_socket->localAddress()->ip(); + ASSERT_NE(server_ip, nullptr); + + // Setup callback handler and listener. + Network::MockUdpListenerCallbacks listener_callbacks; + Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); + + EXPECT_CALL(listener, doRecvFrom(_, _)) + .WillRepeatedly(Invoke([&](sockaddr_storage& peer_addr, socklen_t& addr_len) { + return listener.doRecvFrom_(peer_addr, addr_len); + })); + + // Setup client socket. + SocketPtr client_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, false); + + const int client_sockfd = client_socket->fd(); + sockaddr_storage server_addr; + socklen_t addr_len; + + getSocketAddressInfo(*client_socket.get(), server_ip->port(), server_addr, addr_len); + ASSERT_GT(addr_len, 0); + + // We send 2 packets and exptect it to echo. + const std::string first("first"); + const std::string second("second"); + + auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, first.length()); + + send_rc = ::sendto(client_sockfd, second.c_str(), second.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, second.length()); + + auto validateCallParams = [&](Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address) { + ASSERT_NE(local_address, nullptr); + + ASSERT_NE(peer_address, nullptr); + ASSERT_NE(peer_address->ip(), nullptr); + + EXPECT_EQ(local_address->asString(), server_socket->localAddress()->asString()); + + EXPECT_EQ(peer_address->ip()->addressAsString(), + client_socket->localAddress()->ip()->addressAsString()); + + EXPECT_EQ(*local_address, *server_socket->localAddress()); + }; + + Event::TimerPtr timer = dispatcher_.createTimer([&] { dispatcher_.exit(); }); + + timer->enableTimer(std::chrono::milliseconds(2000)); + + // For unit test purposes, we assume that the data was received in order. + Address::InstanceConstSharedPtr test_peer_address; + + std::vector server_received_data; + + EXPECT_CALL(listener_callbacks, onData_(_)) + .WillOnce(Invoke([&](const UdpData& data) -> void { + validateCallParams(data.local_address_, data.peer_address_); + + test_peer_address = data.peer_address_; + + const std::string data_str = data.buffer_->toString(); + EXPECT_EQ(data_str, first); + + server_received_data.push_back(data_str); + })) + .WillOnce(Invoke([&](const UdpData& data) -> void { + validateCallParams(data.local_address_, data.peer_address_); + + const std::string data_str = data.buffer_->toString(); + EXPECT_EQ(data_str, second); + + server_received_data.push_back(data_str); + })); + + EXPECT_CALL(listener_callbacks, onWriteReady_(_)) + .WillRepeatedly(Invoke([&](const Socket& socket) { + EXPECT_EQ(socket.fd(), server_socket->fd()); + + sockaddr_storage client_addr; + socklen_t client_addr_len; + + ASSERT_NE(test_peer_address, nullptr); + const uint32_t peer_port = test_peer_address->ip()->port(); + + getSocketAddressInfo(test_peer_address, peer_port, client_addr, client_addr_len); + ASSERT_GT(client_addr_len, 0); + + for (const auto& data : server_received_data) { + const std::string::size_type data_size = data.length() + 1; + uint64_t total_sent = 0; + + do { + auto send_rc = + ::sendto(socket.fd(), data.c_str() + total_sent, data_size - total_sent, 0, + reinterpret_cast(&client_addr), client_addr_len); + + if (send_rc > 0) { + total_sent += send_rc; + } else if (send_rc != -EAGAIN) { + break; + } + } while ((send_rc == -EAGAIN) || (total_sent < data_size)); + + EXPECT_EQ(total_sent, data_size); + } + + server_received_data.clear(); + })); + + Buffer::OwnedImpl client_buffer; + Api::SysCallIntResult result; + + for (const auto& data : server_received_data) { + const std::string::size_type data_size = data.length() + 1; + std::string::size_type remaining = data_size; + do { + result = client_buffer.read(client_socket->fd(), remaining); + if (result.rc_ > 0) { + remaining -= result.rc_; + } else if (result.rc_ != -EAGAIN) { + break; + } + } while ((result.rc_ == -EAGAIN) || (remaining)); + + EXPECT_EQ(remaining, 0); + + EXPECT_EQ(client_buffer.toString(), data); + } + + dispatcher_.run(Event::Dispatcher::RunType::Block); +} + +/** + * Tests UDP listener's `enable` and `disable` APIs. + */ +TEST_P(ListenerImplTest, UdpListenerEnableDisable) { + // Setup server socket + SocketPtr server_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, true); + + ASSERT_NE(server_socket, nullptr); + + auto const* server_ip = server_socket->localAddress()->ip(); + ASSERT_NE(server_ip, nullptr); + + // Setup callback handler and listener. + Network::MockUdpListenerCallbacks listener_callbacks; + Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); + + EXPECT_CALL(listener, doRecvFrom(_, _)) + .WillRepeatedly(Invoke([&](sockaddr_storage& peer_addr, socklen_t& addr_len) { + return listener.doRecvFrom_(peer_addr, addr_len); + })); + + // Setup client socket. + SocketPtr client_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, false); + + const int client_sockfd = client_socket->fd(); + sockaddr_storage server_addr; + socklen_t addr_len; + + getSocketAddressInfo(*client_socket.get(), server_ip->port(), server_addr, addr_len); + ASSERT_GT(addr_len, 0); + + // We first disable the listener and then send two packets. + // - With the listener disabled, we expect that none of the callbacks will be + // called. + // - When the listener is enabled back, we expect the callbacks to be called + const std::string first("first"); + const std::string second("second"); + + listener.disable(); + + auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, first.length()); + + send_rc = ::sendto(client_sockfd, second.c_str(), second.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, second.length()); + + auto validateCallParams = [&](Address::InstanceConstSharedPtr local_address, + Address::InstanceConstSharedPtr peer_address) { + ASSERT_NE(local_address, nullptr); + + ASSERT_NE(peer_address, nullptr); + ASSERT_NE(peer_address->ip(), nullptr); + + ASSERT_EQ(local_address->asString(), server_socket->localAddress()->asString()); + + ASSERT_EQ(peer_address->ip()->addressAsString(), + client_socket->localAddress()->ip()->addressAsString()); + + EXPECT_EQ(*local_address, *server_socket->localAddress()); + }; + + Event::TimerPtr timer = dispatcher_.createTimer([&] { dispatcher_.exit(); }); + + timer->enableTimer(std::chrono::milliseconds(2000)); + + EXPECT_CALL(listener_callbacks, onData_(_)).Times(0); + + EXPECT_CALL(listener_callbacks, onWriteReady_(_)).Times(0); + + dispatcher_.run(Event::Dispatcher::RunType::Block); + + listener.enable(); + + EXPECT_CALL(listener_callbacks, onData_(_)) + .Times(2) + .WillOnce(Return()) + .WillOnce(Invoke([&](const UdpData& data) -> void { + validateCallParams(data.local_address_, data.peer_address_); + + EXPECT_EQ(data.buffer_->toString(), second); + + dispatcher_.exit(); + })); + + EXPECT_CALL(listener_callbacks, onWriteReady_(_)) + .WillRepeatedly( + Invoke([&](const Socket& socket) { EXPECT_EQ(socket.fd(), server_socket->fd()); })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); +} + +/** + * Tests UDP listebe's error callback. + */ +TEST_P(ListenerImplTest, UdpListenerRecvFromError) { + // Setup server socket + SocketPtr server_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, true); + + ASSERT_NE(server_socket, nullptr); + + auto const* server_ip = server_socket->localAddress()->ip(); + ASSERT_NE(server_ip, nullptr); + + // Setup callback handler and listener. + Network::MockUdpListenerCallbacks listener_callbacks; + Network::TestUdpListenerImpl listener(dispatcher_, *server_socket.get(), listener_callbacks); + + EXPECT_CALL(listener, doRecvFrom(_, _)).WillRepeatedly(Invoke([&](sockaddr_storage&, socklen_t&) { + return UdpListenerImpl::ReceiveResult{{-1, -1}, nullptr}; + })); + + SocketPtr client_socket = + getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), + nullptr, false); + + const int client_sockfd = client_socket->fd(); + sockaddr_storage server_addr; + socklen_t addr_len; + + getSocketAddressInfo(*client_socket.get(), server_ip->port(), server_addr, addr_len); + ASSERT_GT(addr_len, 0); + + // When the `receive` system call returns an error, we expect the `onError` + // callback callwed with `SYSCALL_ERROR` parameter. + const std::string first("first"); + + auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, + reinterpret_cast(&server_addr), addr_len); + + ASSERT_EQ(send_rc, first.length()); + + EXPECT_CALL(listener_callbacks, onData_(_)).Times(0); + + EXPECT_CALL(listener_callbacks, onWriteReady_(_)) + .Times(1) + .WillRepeatedly( + Invoke([&](const Socket& socket) { EXPECT_EQ(socket.fd(), server_socket->fd()); })); + + EXPECT_CALL(listener_callbacks, onError_(_, _)) + .Times(1) + .WillOnce(Invoke([&](const UdpListenerCallbacks::ErrorCode& err_code, int err) -> void { + ASSERT_EQ(err_code, UdpListenerCallbacks::ErrorCode::SYSCALL_ERROR); + ASSERT_EQ(err, -1); + + dispatcher_.exit(); + })); + + dispatcher_.run(Event::Dispatcher::RunType::Block); +} + +} // namespace Network +} // namespace Envoy From cd8930e87fed619822c3d1cd59945974fd492e31 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Wed, 16 Jan 2019 11:56:01 -0800 Subject: [PATCH 19/32] Parameter names. Signed-off-by: Jojy G Varghese --- include/envoy/network/listener.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index b1c198f2662cb..31c375c233a1c 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -162,7 +162,7 @@ class UdpListenerCallbacks { * @param error_code ErrorCode for the error event. * @param errno System error number. */ - virtual void onError(const ErrorCode& err_code, int err_no) PURE; + virtual void onError(const ErrorCode& error_code, int error_no) PURE; }; /** From f1286736a57512367fd7df96e794299ef8fd8016 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Thu, 17 Jan 2019 08:36:22 -0800 Subject: [PATCH 20/32] Added more comments for things to discuss. Signed-off-by: Jojy G Varghese --- include/envoy/network/listener.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index 31c375c233a1c..b60ea9165deb6 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -128,7 +128,9 @@ struct UdpData { // TODO (conquerAtapple): // Add UdpReader here so that the callback handler can // then use the reader to do multiple reads(recvmmsg) once the OS notifies it - // has data. + // has data. We could also just return a `ReaderFactory` that returns either a + // `recvfrom` reader (with peer information) or a `read/recvmmsg` reader. This + // is still being flushed out (Jan, 2019). }; /** From b83ff51462a92f6fb0457be9ec96a8e0d9cebfb5 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Fri, 18 Jan 2019 10:38:25 -0800 Subject: [PATCH 21/32] Added TODO for reconsidering ownership semantics. Signed-off-by: Jojy G Varghese --- include/envoy/network/listener.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index b60ea9165deb6..0a4ecb56ed8d0 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -118,14 +118,14 @@ class ListenerCallbacks { * Utility struct that encapsulates the information from a udp socket's * recvfrom/recvmmdg call. * - * TODO (conqerAtapple): Maybe this belongs inside the UdpListenerCallbacks + * TODO(conqerAtapple): Maybe this belongs inside the UdpListenerCallbacks * class. */ struct UdpData { Address::InstanceConstSharedPtr local_address_; - Address::InstanceConstSharedPtr peer_address_; + Address::InstanceConstSharedPtr peer_address_; // TODO(conquerAtapple): Fix ownership semantics. Buffer::InstancePtr buffer_; - // TODO (conquerAtapple): + // TODO(conquerAtapple): // Add UdpReader here so that the callback handler can // then use the reader to do multiple reads(recvmmsg) once the OS notifies it // has data. We could also just return a `ReaderFactory` that returns either a @@ -154,7 +154,7 @@ class UdpListenerCallbacks { * * @param socket Underlying server socket for the listener. * - * TODO (conqerAtapple): Maybe we need a UdpWriter here instead of Socket. + * TODO(conqerAtapple): Maybe we need a UdpWriter here instead of Socket. */ virtual void onWriteReady(const Socket& socket) PURE; From e8003b2b616f840401154a5fa118ad6a08f3f984 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Wed, 23 Jan 2019 19:21:39 -0800 Subject: [PATCH 22/32] Fixed onError API parameter names. Signed-off-by: Jojy G Varghese --- include/envoy/network/listener.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index 0a4ecb56ed8d0..c1ef312ea16cf 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -162,9 +162,9 @@ class UdpListenerCallbacks { * Called when there is an error event. * * @param error_code ErrorCode for the error event. - * @param errno System error number. + * @param error_number System error number. */ - virtual void onError(const ErrorCode& error_code, int error_no) PURE; + virtual void onError(const ErrorCode& error_code, int error_number) PURE; }; /** From 84b6e68599965b3b0d1c7d23cb15af2c4fe397b0 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Thu, 24 Jan 2019 12:43:45 -0800 Subject: [PATCH 23/32] Code review addressed (@mpwarres). Signed-off-by: Jojy G Varghese --- include/envoy/event/dispatcher.h | 4 ++-- include/envoy/network/listener.h | 2 +- source/common/event/dispatcher_impl.cc | 4 ++-- source/common/event/dispatcher_impl.h | 4 ++-- source/common/network/udp_listener_impl.cc | 2 +- test/common/network/udp_listener_impl_test.cc | 4 ++-- test/mocks/event/mocks.h | 8 ++++---- 7 files changed, 14 insertions(+), 14 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index c0de6b9d072a2..9dff305341368 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -116,8 +116,8 @@ class Dispatcher { * @param cb supplies the udp listener callbacks to invoke for listener events. * @return Network::ListenerPtr a new listener that is owned by the caller. */ - virtual Network::ListenerPtr createDatagramListener(Network::Socket& socket, - Network::UdpListenerCallbacks& cb) PURE; + virtual Network::ListenerPtr createUdpListener(Network::Socket& socket, + Network::UdpListenerCallbacks& cb) PURE; /** * Allocate a timer. @see Timer for docs on how to use the timer. * @param cb supplies the callback to invoke when the timer fires. diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index c1ef312ea16cf..87ead62ef2b33 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -138,7 +138,7 @@ struct UdpData { */ class UdpListenerCallbacks { public: - enum class ErrorCode { SYSCALL_ERROR, UNKNOWN_ERROR }; + enum class ErrorCode { SyscallError, UnknownError }; virtual ~UdpListenerCallbacks() = default; diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 0fd565b0f77ea..ced376e638dc5 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -120,8 +120,8 @@ DispatcherImpl::createListener(Network::Socket& socket, Network::ListenerCallbac hand_off_restored_destination_connections)}; } -Network::ListenerPtr DispatcherImpl::createDatagramListener(Network::Socket& socket, - Network::UdpListenerCallbacks& cb) { +Network::ListenerPtr DispatcherImpl::createUdpListener(Network::Socket& socket, + Network::UdpListenerCallbacks& cb) { ASSERT(isThreadSafe()); return Network::ListenerPtr{new Network::UdpListenerImpl(*this, socket, cb)}; } diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 3788b9a2578ab..fc0cf5326401d 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -51,8 +51,8 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { Network::ListenerPtr createListener(Network::Socket& socket, Network::ListenerCallbacks& cb, bool bind_to_port, bool hand_off_restored_destination_connections) override; - Network::ListenerPtr createDatagramListener(Network::Socket& socket, - Network::UdpListenerCallbacks& cb) override; + Network::ListenerPtr createUdpListener(Network::Socket& socket, + Network::UdpListenerCallbacks& cb) override; TimerPtr createTimer(TimerCb cb) override; void deferredDelete(DeferredDeletablePtr&& to_delete) override; void exit() override; diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index 38979f4aa5e13..86b551bc6b9c2 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -89,7 +89,7 @@ void UdpListenerImpl::handleReadCallback(int fd) { do { recv_result = doRecvFrom(addr, addr_len); if ((recv_result.result_.rc_ < 0) && (recv_result.result_.rc_ != -EAGAIN)) { - cb_.onError(UdpListenerCallbacks::ErrorCode::SYSCALL_ERROR, recv_result.result_.errno_); + cb_.onError(UdpListenerCallbacks::ErrorCode::SyscallError, recv_result.result_.errno_); return; } diff --git a/test/common/network/udp_listener_impl_test.cc b/test/common/network/udp_listener_impl_test.cc index 45f10667dd7a5..d9710d7d404c6 100644 --- a/test/common/network/udp_listener_impl_test.cc +++ b/test/common/network/udp_listener_impl_test.cc @@ -504,7 +504,7 @@ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { ASSERT_GT(addr_len, 0); // When the `receive` system call returns an error, we expect the `onError` - // callback callwed with `SYSCALL_ERROR` parameter. + // callback callwed with `SyscallError` parameter. const std::string first("first"); auto send_rc = ::sendto(client_sockfd, first.c_str(), first.length(), 0, @@ -522,7 +522,7 @@ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { EXPECT_CALL(listener_callbacks, onError_(_, _)) .Times(1) .WillOnce(Invoke([&](const UdpListenerCallbacks::ErrorCode& err_code, int err) -> void { - ASSERT_EQ(err_code, UdpListenerCallbacks::ErrorCode::SYSCALL_ERROR); + ASSERT_EQ(err_code, UdpListenerCallbacks::ErrorCode::SyscallError); ASSERT_EQ(err, -1); dispatcher_.exit(); diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 539f661d71e3a..55ef6403498a0 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -65,9 +65,9 @@ class MockDispatcher : public Dispatcher { createListener_(socket, cb, bind_to_port, hand_off_restored_destination_connections)}; } - Network::ListenerPtr createDatagramListener(Network::Socket& socket, - Network::UdpListenerCallbacks& cb) override { - return Network::ListenerPtr{createDatagramListener_(socket, cb)}; + Network::ListenerPtr createUdpListener(Network::Socket& socket, + Network::UdpListenerCallbacks& cb) override { + return Network::ListenerPtr{createUdpListener_(socket, cb)}; } Event::TimerPtr createTimer(Event::TimerCb cb) override { @@ -106,7 +106,7 @@ class MockDispatcher : public Dispatcher { Network::Listener*(Network::Socket& socket, Network::ListenerCallbacks& cb, bool bind_to_port, bool hand_off_restored_destination_connections)); - MOCK_METHOD2(createDatagramListener_, + MOCK_METHOD2(createUdpListener_, Network::Listener*(Network::Socket& socket, Network::UdpListenerCallbacks& cb)); MOCK_METHOD1(createTimer_, Timer*(Event::TimerCb cb)); MOCK_METHOD1(deferredDelete_, void(DeferredDeletable* to_delete)); From a84a96d2600c0b5604f568e95d2eb33d2334f3da Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Thu, 24 Jan 2019 22:10:48 -0800 Subject: [PATCH 24/32] Fixed merge issues. Signed-off-by: Jojy G Varghese --- source/common/network/listener_impl.cc | 2 +- source/common/network/udp_listener_impl.cc | 10 +++--- test/common/network/udp_listener_impl_test.cc | 33 ++++++++++--------- 3 files changed, 24 insertions(+), 21 deletions(-) diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index 5b6fcce6d5704..93a020d632cc9 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -48,7 +48,7 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* } void ListenerImpl::setupServerSocket(const Event::DispatcherImpl& dispatcher, Socket& socket) { - listener_.reset(evconnlistener_new(&dispatcher.base(), listenCallback, this, 0, -1, socket.fd())); + listener_.reset(evconnlistener_new(&dispatcher.base(), listenCallback, this, 0, -1, socket.ioHandle().fd())); if (!listener_) { throw CreateListenerException( diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index 86b551bc6b9c2..2bb339fce3ba1 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -21,8 +21,8 @@ namespace Network { UdpListenerImpl::UdpListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb) : BaseListenerImpl(dispatcher, socket), cb_(cb) { - event_assign(&raw_event_, &dispatcher.base(), socket.fd(), EV_READ | EV_WRITE | EV_PERSIST, - eventCallback, this); + event_assign(&raw_event_, &dispatcher.base(), socket.ioHandle().fd(), + EV_READ | EV_WRITE | EV_PERSIST, eventCallback, this); event_add(&raw_event_, nullptr); if (!Network::Socket::applyOptions(socket.options(), socket, @@ -50,7 +50,7 @@ UdpListenerImpl::ReceiveResult UdpListenerImpl::doRecvFrom(sockaddr_storage& pee ASSERT(num_slices == 1); // TODO(conqerAtapple): Use os_syscalls - const ssize_t rc = ::recvfrom(socket_.fd(), slice.mem_, read_length, 0, + const ssize_t rc = ::recvfrom(socket_.ioHandle().fd(), slice.mem_, read_length, 0, reinterpret_cast(&peer_addr), &addr_len); if (rc < 0) { return ReceiveResult{Api::SysCallIntResult{static_cast(rc), errno}, nullptr}; @@ -79,7 +79,7 @@ void UdpListenerImpl::eventCallback(int fd, short flags, void* arg) { } void UdpListenerImpl::handleReadCallback(int fd) { - RELEASE_ASSERT(fd == socket_.fd(), + RELEASE_ASSERT(fd == socket_.ioHandle().fd(), fmt::format("Invalid socket descriptor received in callback {}", fd)); sockaddr_storage addr; @@ -153,7 +153,7 @@ void UdpListenerImpl::handleReadCallback(int fd) { } void UdpListenerImpl::handleWriteCallback(int fd) { - RELEASE_ASSERT(fd == socket_.fd(), + RELEASE_ASSERT(fd == socket_.ioHandle().fd(), fmt::format("Invalid socket descriptor received in callback {}", fd)); cb_.onWriteReady(socket_); diff --git a/test/common/network/udp_listener_impl_test.cc b/test/common/network/udp_listener_impl_test.cc index d9710d7d404c6..0510bc4e3b728 100644 --- a/test/common/network/udp_listener_impl_test.cc +++ b/test/common/network/udp_listener_impl_test.cc @@ -163,7 +163,7 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), nullptr, false); - const int client_sockfd = client_socket->fd(); + const int client_sockfd = client_socket->ioHandle().fd(); sockaddr_storage server_addr; socklen_t addr_len; @@ -214,8 +214,9 @@ TEST_P(ListenerImplTest, UseActualDstUdp) { })); EXPECT_CALL(listener_callbacks, onWriteReady_(_)) - .WillRepeatedly( - Invoke([&](const Socket& socket) { EXPECT_EQ(socket.fd(), server_socket->fd()); })); + .WillRepeatedly(Invoke([&](const Socket& socket) { + EXPECT_EQ(socket.ioHandle().fd(), server_socket->ioHandle().fd()); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); } @@ -248,7 +249,7 @@ TEST_P(ListenerImplTest, UdpEcho) { getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), nullptr, false); - const int client_sockfd = client_socket->fd(); + const int client_sockfd = client_socket->ioHandle().fd(); sockaddr_storage server_addr; socklen_t addr_len; @@ -315,7 +316,7 @@ TEST_P(ListenerImplTest, UdpEcho) { EXPECT_CALL(listener_callbacks, onWriteReady_(_)) .WillRepeatedly(Invoke([&](const Socket& socket) { - EXPECT_EQ(socket.fd(), server_socket->fd()); + EXPECT_EQ(socket.ioHandle().fd(), server_socket->ioHandle().fd()); sockaddr_storage client_addr; socklen_t client_addr_len; @@ -331,9 +332,9 @@ TEST_P(ListenerImplTest, UdpEcho) { uint64_t total_sent = 0; do { - auto send_rc = - ::sendto(socket.fd(), data.c_str() + total_sent, data_size - total_sent, 0, - reinterpret_cast(&client_addr), client_addr_len); + auto send_rc = ::sendto( + socket.ioHandle().fd(), data.c_str() + total_sent, data_size - total_sent, 0, + reinterpret_cast(&client_addr), client_addr_len); if (send_rc > 0) { total_sent += send_rc; @@ -355,7 +356,7 @@ TEST_P(ListenerImplTest, UdpEcho) { const std::string::size_type data_size = data.length() + 1; std::string::size_type remaining = data_size; do { - result = client_buffer.read(client_socket->fd(), remaining); + result = client_buffer.read(client_socket->ioHandle().fd(), remaining); if (result.rc_ > 0) { remaining -= result.rc_; } else if (result.rc_ != -EAGAIN) { @@ -399,7 +400,7 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), nullptr, false); - const int client_sockfd = client_socket->fd(); + const int client_sockfd = client_socket->ioHandle().fd(); sockaddr_storage server_addr; socklen_t addr_len; @@ -464,8 +465,9 @@ TEST_P(ListenerImplTest, UdpListenerEnableDisable) { })); EXPECT_CALL(listener_callbacks, onWriteReady_(_)) - .WillRepeatedly( - Invoke([&](const Socket& socket) { EXPECT_EQ(socket.fd(), server_socket->fd()); })); + .WillRepeatedly(Invoke([&](const Socket& socket) { + EXPECT_EQ(socket.ioHandle().fd(), server_socket->ioHandle().fd()); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); } @@ -496,7 +498,7 @@ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { getSocket(Address::SocketType::Datagram, Network::Test::getCanonicalLoopbackAddress(version_), nullptr, false); - const int client_sockfd = client_socket->fd(); + const int client_sockfd = client_socket->ioHandle().fd(); sockaddr_storage server_addr; socklen_t addr_len; @@ -516,8 +518,9 @@ TEST_P(ListenerImplTest, UdpListenerRecvFromError) { EXPECT_CALL(listener_callbacks, onWriteReady_(_)) .Times(1) - .WillRepeatedly( - Invoke([&](const Socket& socket) { EXPECT_EQ(socket.fd(), server_socket->fd()); })); + .WillRepeatedly(Invoke([&](const Socket& socket) { + EXPECT_EQ(socket.ioHandle().fd(), server_socket->ioHandle().fd()); + })); EXPECT_CALL(listener_callbacks, onError_(_, _)) .Times(1) From ee78778d33c0ebf3f83e25625b2d980933b7b056 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Thu, 24 Jan 2019 22:27:48 -0800 Subject: [PATCH 25/32] Fix format. Signed-off-by: Jojy G Varghese --- source/common/network/listener_impl.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index 93a020d632cc9..7b91c9dc0fbce 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -48,7 +48,8 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* } void ListenerImpl::setupServerSocket(const Event::DispatcherImpl& dispatcher, Socket& socket) { - listener_.reset(evconnlistener_new(&dispatcher.base(), listenCallback, this, 0, -1, socket.ioHandle().fd())); + listener_.reset( + evconnlistener_new(&dispatcher.base(), listenCallback, this, 0, -1, socket.ioHandle().fd())); if (!listener_) { throw CreateListenerException( From 9bcfacfe572e8178271de6cf49dc2af400d9f241 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Sat, 26 Jan 2019 07:51:48 -0800 Subject: [PATCH 26/32] Fixed EAGAIN handling. Signed-off-by: Jojy G Varghese --- source/common/network/udp_listener_impl.cc | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index 2bb339fce3ba1..fcd3609e3e8d0 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -83,17 +83,15 @@ void UdpListenerImpl::handleReadCallback(int fd) { fmt::format("Invalid socket descriptor received in callback {}", fd)); sockaddr_storage addr; - socklen_t addr_len; - ReceiveResult recv_result; + socklen_t addr_len = 0; - do { - recv_result = doRecvFrom(addr, addr_len); - if ((recv_result.result_.rc_ < 0) && (recv_result.result_.rc_ != -EAGAIN)) { + ReceiveResult recv_result = doRecvFrom(addr, addr_len); + if ((recv_result.result_.rc_ < 0)) { + if (recv_result.result_.rc_ != -EAGAIN) { cb_.onError(UdpListenerCallbacks::ErrorCode::SyscallError, recv_result.result_.errno_); - return; } - - } while (recv_result.result_.rc_ == -EAGAIN); + return; + } Address::InstanceConstSharedPtr local_address = socket_.localAddress(); From 4f8de99152ae3d07c77789f54f2ede80958fc395 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Sat, 26 Jan 2019 08:59:19 -0800 Subject: [PATCH 27/32] Fixed asserts. Signed-off-by: Jojy G Varghese --- source/common/network/udp_listener_impl.cc | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index fcd3609e3e8d0..00cbe25f79235 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -63,8 +63,7 @@ UdpListenerImpl::ReceiveResult UdpListenerImpl::doRecvFrom(sockaddr_storage& pee } void UdpListenerImpl::eventCallback(int fd, short flags, void* arg) { - RELEASE_ASSERT((flags & (EV_READ | EV_WRITE)), - fmt::format("Unexpected flags for callback: {}", flags)); + ASSERT((flags & (EV_READ | EV_WRITE))); UdpListenerImpl* instance = static_cast(arg); ASSERT(instance); @@ -79,8 +78,7 @@ void UdpListenerImpl::eventCallback(int fd, short flags, void* arg) { } void UdpListenerImpl::handleReadCallback(int fd) { - RELEASE_ASSERT(fd == socket_.ioHandle().fd(), - fmt::format("Invalid socket descriptor received in callback {}", fd)); + ASSERT(fd == socket_.ioHandle().fd()); sockaddr_storage addr; socklen_t addr_len = 0; @@ -151,8 +149,7 @@ void UdpListenerImpl::handleReadCallback(int fd) { } void UdpListenerImpl::handleWriteCallback(int fd) { - RELEASE_ASSERT(fd == socket_.ioHandle().fd(), - fmt::format("Invalid socket descriptor received in callback {}", fd)); + ASSERT(fd == socket_.ioHandle().fd()); cb_.onWriteReady(socket_); } From bc2695ab9f1a3d7899a4157afc9d06b3444e325f Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Sat, 26 Jan 2019 09:01:45 -0800 Subject: [PATCH 28/32] fixed typo. Signed-off-by: Jojy G Varghese --- include/envoy/network/listener.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index 87ead62ef2b33..06a9d7cc22637 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -116,7 +116,7 @@ class ListenerCallbacks { /** * Utility struct that encapsulates the information from a udp socket's - * recvfrom/recvmmdg call. + * recvfrom/recvmmsg call. * * TODO(conqerAtapple): Maybe this belongs inside the UdpListenerCallbacks * class. From 3c1f89ac403e6084526d629d6d4a75dbc129e2af Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Sat, 26 Jan 2019 23:59:45 -0800 Subject: [PATCH 29/32] Changed to FileEvent. Signed-off-by: Jojy G Varghese --- include/envoy/event/dispatcher.h | 2 +- source/common/event/dispatcher_impl.cc | 2 +- source/common/event/dispatcher_impl.h | 2 +- source/common/event/file_event_impl.cc | 2 +- source/common/event/file_event_impl.h | 2 +- source/common/network/udp_listener_impl.cc | 149 +++++++++--------- source/common/network/udp_listener_impl.h | 12 +- test/common/network/udp_listener_impl_test.cc | 27 +--- test/mocks/event/mocks.h | 6 +- 9 files changed, 98 insertions(+), 106 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 9dff305341368..83e05bba0ad4f 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -90,7 +90,7 @@ class Dispatcher { * initially listen on. */ virtual FileEventPtr createFileEvent(int fd, FileReadyCb cb, FileTriggerType trigger, - uint32_t events) PURE; + uint32_t events) const PURE; /** * @return Filesystem::WatcherPtr a filesystem watcher owned by the caller. diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index ced376e638dc5..96b2cd8ac8326 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -102,7 +102,7 @@ Network::DnsResolverSharedPtr DispatcherImpl::createDnsResolver( } FileEventPtr DispatcherImpl::createFileEvent(int fd, FileReadyCb cb, FileTriggerType trigger, - uint32_t events) { + uint32_t events) const { ASSERT(isThreadSafe()); return FileEventPtr{new FileEventImpl(*this, fd, cb, trigger, events)}; } diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index fc0cf5326401d..48524494fcbc8 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -46,7 +46,7 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { Network::DnsResolverSharedPtr createDnsResolver( const std::vector& resolvers) override; FileEventPtr createFileEvent(int fd, FileReadyCb cb, FileTriggerType trigger, - uint32_t events) override; + uint32_t events) const override; Filesystem::WatcherPtr createFilesystemWatcher() override; Network::ListenerPtr createListener(Network::Socket& socket, Network::ListenerCallbacks& cb, bool bind_to_port, diff --git a/source/common/event/file_event_impl.cc b/source/common/event/file_event_impl.cc index bc1cf9ad5ab4b..671865077ceb9 100644 --- a/source/common/event/file_event_impl.cc +++ b/source/common/event/file_event_impl.cc @@ -10,7 +10,7 @@ namespace Envoy { namespace Event { -FileEventImpl::FileEventImpl(DispatcherImpl& dispatcher, int fd, FileReadyCb cb, +FileEventImpl::FileEventImpl(const DispatcherImpl& dispatcher, int fd, FileReadyCb cb, FileTriggerType trigger, uint32_t events) : cb_(cb), base_(&dispatcher.base()), fd_(fd), trigger_(trigger) { assignEvents(events); diff --git a/source/common/event/file_event_impl.h b/source/common/event/file_event_impl.h index feac017b79b92..3d8f233960897 100644 --- a/source/common/event/file_event_impl.h +++ b/source/common/event/file_event_impl.h @@ -16,7 +16,7 @@ namespace Event { */ class FileEventImpl : public FileEvent, ImplBase { public: - FileEventImpl(DispatcherImpl& dispatcher, int fd, FileReadyCb cb, FileTriggerType trigger, + FileEventImpl(const DispatcherImpl& dispatcher, int fd, FileReadyCb cb, FileTriggerType trigger, uint32_t events); // Event::FileEvent diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index 00cbe25f79235..44bc2a43b0b81 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -2,8 +2,6 @@ #include -#include - #include "envoy/buffer/buffer.h" #include "envoy/common/exception.h" @@ -21,9 +19,11 @@ namespace Network { UdpListenerImpl::UdpListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb) : BaseListenerImpl(dispatcher, socket), cb_(cb) { - event_assign(&raw_event_, &dispatcher.base(), socket.ioHandle().fd(), - EV_READ | EV_WRITE | EV_PERSIST, eventCallback, this); - event_add(&raw_event_, nullptr); + file_event_ = dispatcher_.createFileEvent( + socket.ioHandle().fd(), [this](uint32_t events) -> void { onSocketEvent(events); }, + Event::FileTriggerType::Edge, Event::FileReadyType::Read | Event::FileReadyType::Write); + + ASSERT(file_event_); if (!Network::Socket::applyOptions(socket.options(), socket, envoy::api::v2::core::SocketOption::STATE_BOUND)) { @@ -32,9 +32,16 @@ UdpListenerImpl::UdpListenerImpl(const Event::DispatcherImpl& dispatcher, Socket } } -void UdpListenerImpl::disable() { event_del(&raw_event_); } +UdpListenerImpl::~UdpListenerImpl() { + disable(); + file_event_.reset(); +} -void UdpListenerImpl::enable() { event_add(&raw_event_, nullptr); } +void UdpListenerImpl::disable() { file_event_->setEnabled(0); } + +void UdpListenerImpl::enable() { + file_event_->setEnabled(Event::FileReadyType::Read | Event::FileReadyType::Write); +} UdpListenerImpl::ReceiveResult UdpListenerImpl::doRecvFrom(sockaddr_storage& peer_addr, socklen_t& addr_len) { @@ -62,97 +69,95 @@ UdpListenerImpl::ReceiveResult UdpListenerImpl::doRecvFrom(sockaddr_storage& pee return ReceiveResult{Api::SysCallIntResult{static_cast(rc), 0}, std::move(buffer)}; } -void UdpListenerImpl::eventCallback(int fd, short flags, void* arg) { - ASSERT((flags & (EV_READ | EV_WRITE))); +void UdpListenerImpl::onSocketEvent(short flags) { + ASSERT((flags & (Event::FileReadyType::Read | Event::FileReadyType::Write))); - UdpListenerImpl* instance = static_cast(arg); - ASSERT(instance); - - if (flags & EV_READ) { - instance->handleReadCallback(fd); + if (flags & Event::FileReadyType::Read) { + handleReadCallback(); } - if (flags & EV_WRITE) { - instance->handleWriteCallback(fd); + if (flags & Event::FileReadyType::Write) { + handleWriteCallback(); } } -void UdpListenerImpl::handleReadCallback(int fd) { - ASSERT(fd == socket_.ioHandle().fd()); - +void UdpListenerImpl::handleReadCallback() { sockaddr_storage addr; socklen_t addr_len = 0; - ReceiveResult recv_result = doRecvFrom(addr, addr_len); - if ((recv_result.result_.rc_ < 0)) { - if (recv_result.result_.rc_ != -EAGAIN) { - cb_.onError(UdpListenerCallbacks::ErrorCode::SyscallError, recv_result.result_.errno_); + do { + ReceiveResult recv_result = doRecvFrom(addr, addr_len); + if ((recv_result.result_.rc_ < 0)) { + if (recv_result.result_.errno_ != EAGAIN) { + cb_.onError(UdpListenerCallbacks::ErrorCode::SyscallError, recv_result.result_.errno_); + } + return; + } + + if ((recv_result.result_.rc_ == 0)) { + return; } - return; - } - Address::InstanceConstSharedPtr local_address = socket_.localAddress(); + Address::InstanceConstSharedPtr local_address = socket_.localAddress(); - RELEASE_ASSERT( - addr_len > 0, - fmt::format( - "Unable to get remote address for fd: {}, local address: {}. address length is 0 ", fd, - local_address->asString())); + RELEASE_ASSERT( + addr_len > 0, + fmt::format( + "Unable to get remote address for fd: {}, local address: {}. address length is 0 ", + socket_.ioHandle().fd(), local_address->asString())); - Address::InstanceConstSharedPtr peer_address; + Address::InstanceConstSharedPtr peer_address; - // TODO(conqerAtApple): Current implementation of Address::addressFromSockAddr - // cannot be used here unfortunately. This should belong in Address namespace. - switch (addr.ss_family) { - case AF_INET: { - const struct sockaddr_in* sin = reinterpret_cast(&addr); - ASSERT(AF_INET == sin->sin_family); - peer_address = std::make_shared(sin); + // TODO(conqerAtApple): Current implementation of Address::addressFromSockAddr + // cannot be used here unfortunately. This should belong in Address namespace. + switch (addr.ss_family) { + case AF_INET: { + const struct sockaddr_in* sin = reinterpret_cast(&addr); + ASSERT(AF_INET == sin->sin_family); + peer_address = std::make_shared(sin); - break; - } - case AF_INET6: { - const struct sockaddr_in6* sin6 = reinterpret_cast(&addr); - ASSERT(AF_INET6 == sin6->sin6_family); - if (IN6_IS_ADDR_V4MAPPED(&sin6->sin6_addr)) { + break; + } + case AF_INET6: { + const struct sockaddr_in6* sin6 = reinterpret_cast(&addr); + ASSERT(AF_INET6 == sin6->sin6_family); + if (IN6_IS_ADDR_V4MAPPED(&sin6->sin6_addr)) { #if defined(__APPLE__) - struct sockaddr_in sin = { - {}, AF_INET, sin6->sin6_port, {sin6->sin6_addr.__u6_addr.__u6_addr32[3]}, {}}; + struct sockaddr_in sin = { + {}, AF_INET, sin6->sin6_port, {sin6->sin6_addr.__u6_addr.__u6_addr32[3]}, {}}; #else - struct sockaddr_in sin = {AF_INET, sin6->sin6_port, {sin6->sin6_addr.s6_addr32[3]}, {}}; + struct sockaddr_in sin = {AF_INET, sin6->sin6_port, {sin6->sin6_addr.s6_addr32[3]}, {}}; #endif - peer_address = std::make_shared(&sin); - } else { - peer_address = std::make_shared(*sin6, true); + peer_address = std::make_shared(&sin); + } else { + peer_address = std::make_shared(*sin6, true); + } + + break; } - break; - } + default: + RELEASE_ASSERT(false, + fmt::format("Unsupported address family: {}, local address: {}, receive size: " + "{}, address length: {}", + addr.ss_family, local_address->asString(), recv_result.result_.rc_, + addr_len)); + break; + } - default: - RELEASE_ASSERT(false, - fmt::format("Unsupported address family: {}, local address: {}, receive size: " - "{}, address length: {}", - addr.ss_family, local_address->asString(), recv_result.result_.rc_, - addr_len)); - break; - } + RELEASE_ASSERT((peer_address != nullptr), + fmt::format("Unable to get remote address for fd: {}, local address: {} ", + socket_.ioHandle().fd(), local_address->asString())); - RELEASE_ASSERT((peer_address != nullptr), - fmt::format("Unable to get remote address for fd: {}, local address: {} ", fd, - local_address->asString())); + RELEASE_ASSERT((local_address != nullptr), + fmt::format("Unable to get local address for fd: {}", socket_.ioHandle().fd())); - RELEASE_ASSERT((local_address != nullptr), - fmt::format("Unable to get local address for fd: {}", fd)); + cb_.onData(UdpData{local_address, peer_address, std::move(recv_result.buffer_)}); - cb_.onData(UdpData{local_address, peer_address, std::move(recv_result.buffer_)}); + } while (true); } -void UdpListenerImpl::handleWriteCallback(int fd) { - ASSERT(fd == socket_.ioHandle().fd()); - - cb_.onWriteReady(socket_); -} +void UdpListenerImpl::handleWriteCallback() { cb_.onWriteReady(socket_); } } // namespace Network } // namespace Envoy diff --git a/source/common/network/udp_listener_impl.h b/source/common/network/udp_listener_impl.h index 126269cbea1d9..bb0297094f055 100644 --- a/source/common/network/udp_listener_impl.h +++ b/source/common/network/udp_listener_impl.h @@ -4,6 +4,7 @@ #include "common/buffer/buffer_impl.h" #include "common/event/event_impl_base.h" +#include "common/event/file_event_impl.h" #include "base_listener_impl.h" @@ -13,11 +14,13 @@ namespace Network { /** * libevent implementation of Network::Listener for UDP. */ -class UdpListenerImpl : public BaseListenerImpl, public Event::ImplBase { +class UdpListenerImpl : public BaseListenerImpl { public: UdpListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb); + ~UdpListenerImpl(); + virtual void disable() override; virtual void enable() override; @@ -30,13 +33,14 @@ class UdpListenerImpl : public BaseListenerImpl, public Event::ImplBase { virtual ReceiveResult doRecvFrom(sockaddr_storage& peer_addr, socklen_t& addr_len); protected: - void handleWriteCallback(int fd); - void handleReadCallback(int fd); + void handleWriteCallback(); + void handleReadCallback(); UdpListenerCallbacks& cb_; private: - static void eventCallback(int fd, short flags, void* arg); + void onSocketEvent(short flags); + Event::FileEventPtr file_event_; }; } // namespace Network diff --git a/test/common/network/udp_listener_impl_test.cc b/test/common/network/udp_listener_impl_test.cc index 0510bc4e3b728..3251d1b9775ed 100644 --- a/test/common/network/udp_listener_impl_test.cc +++ b/test/common/network/udp_listener_impl_test.cc @@ -338,10 +338,13 @@ TEST_P(ListenerImplTest, UdpEcho) { if (send_rc > 0) { total_sent += send_rc; - } else if (send_rc != -EAGAIN) { + if (total_sent >= data_size) { + break; + } + } else if (errno != EAGAIN) { break; } - } while ((send_rc == -EAGAIN) || (total_sent < data_size)); + } while (((send_rc < 0) && (errno == EAGAIN)) || (total_sent < data_size)); EXPECT_EQ(total_sent, data_size); } @@ -349,26 +352,6 @@ TEST_P(ListenerImplTest, UdpEcho) { server_received_data.clear(); })); - Buffer::OwnedImpl client_buffer; - Api::SysCallIntResult result; - - for (const auto& data : server_received_data) { - const std::string::size_type data_size = data.length() + 1; - std::string::size_type remaining = data_size; - do { - result = client_buffer.read(client_socket->ioHandle().fd(), remaining); - if (result.rc_ > 0) { - remaining -= result.rc_; - } else if (result.rc_ != -EAGAIN) { - break; - } - } while ((result.rc_ == -EAGAIN) || (remaining)); - - EXPECT_EQ(remaining, 0); - - EXPECT_EQ(client_buffer.toString(), data); - } - dispatcher_.run(Event::Dispatcher::RunType::Block); } diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 55ef6403498a0..4f5df54b429ce 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -50,7 +50,7 @@ class MockDispatcher : public Dispatcher { } FileEventPtr createFileEvent(int fd, FileReadyCb cb, FileTriggerType trigger, - uint32_t events) override { + uint32_t events) const override { return FileEventPtr{createFileEvent_(fd, cb, trigger, events)}; } @@ -99,8 +99,8 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD1(createDnsResolver, Network::DnsResolverSharedPtr( const std::vector& resolvers)); - MOCK_METHOD4(createFileEvent_, - FileEvent*(int fd, FileReadyCb cb, FileTriggerType trigger, uint32_t events)); + MOCK_CONST_METHOD4(createFileEvent_, + FileEvent*(int fd, FileReadyCb cb, FileTriggerType trigger, uint32_t events)); MOCK_METHOD0(createFilesystemWatcher_, Filesystem::Watcher*()); MOCK_METHOD4(createListener_, Network::Listener*(Network::Socket& socket, Network::ListenerCallbacks& cb, From ce1874260c59d64e7685e733e575edacaacc1c15 Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Sun, 27 Jan 2019 13:39:35 -0800 Subject: [PATCH 30/32] Removed extraneous parenthesis. Signed-off-by: Jojy G Varghese --- source/common/network/udp_listener_impl.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index 44bc2a43b0b81..d86528a003c0a 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -94,7 +94,8 @@ void UdpListenerImpl::handleReadCallback() { return; } - if ((recv_result.result_.rc_ == 0)) { + if (recv_result.result_.rc_ == 0) { + // TODO(conqerAtapple): Is zero length packet interesting? return; } From 972819ac540e7a34b4a8aa226e0f77627478959b Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Sun, 27 Jan 2019 14:31:29 -0800 Subject: [PATCH 31/32] Removed const'ness for Dispatcher in listener. Signed-off-by: Jojy G Varghese --- include/envoy/event/dispatcher.h | 2 +- source/common/event/dispatcher_impl.cc | 2 +- source/common/event/dispatcher_impl.h | 2 +- source/common/network/base_listener_impl.cc | 2 +- source/common/network/base_listener_impl.h | 4 ++-- source/common/network/listener_impl.cc | 5 ++--- source/common/network/listener_impl.h | 2 +- source/common/network/udp_listener_impl.cc | 2 +- source/common/network/udp_listener_impl.h | 3 +-- test/mocks/event/mocks.h | 6 +++--- 10 files changed, 14 insertions(+), 16 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 83e05bba0ad4f..9dff305341368 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -90,7 +90,7 @@ class Dispatcher { * initially listen on. */ virtual FileEventPtr createFileEvent(int fd, FileReadyCb cb, FileTriggerType trigger, - uint32_t events) const PURE; + uint32_t events) PURE; /** * @return Filesystem::WatcherPtr a filesystem watcher owned by the caller. diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 96b2cd8ac8326..ced376e638dc5 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -102,7 +102,7 @@ Network::DnsResolverSharedPtr DispatcherImpl::createDnsResolver( } FileEventPtr DispatcherImpl::createFileEvent(int fd, FileReadyCb cb, FileTriggerType trigger, - uint32_t events) const { + uint32_t events) { ASSERT(isThreadSafe()); return FileEventPtr{new FileEventImpl(*this, fd, cb, trigger, events)}; } diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 48524494fcbc8..fc0cf5326401d 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -46,7 +46,7 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { Network::DnsResolverSharedPtr createDnsResolver( const std::vector& resolvers) override; FileEventPtr createFileEvent(int fd, FileReadyCb cb, FileTriggerType trigger, - uint32_t events) const override; + uint32_t events) override; Filesystem::WatcherPtr createFilesystemWatcher() override; Network::ListenerPtr createListener(Network::Socket& socket, Network::ListenerCallbacks& cb, bool bind_to_port, diff --git a/source/common/network/base_listener_impl.cc b/source/common/network/base_listener_impl.cc index 5b20e83aa073e..7340dd693bc5d 100644 --- a/source/common/network/base_listener_impl.cc +++ b/source/common/network/base_listener_impl.cc @@ -20,7 +20,7 @@ Address::InstanceConstSharedPtr BaseListenerImpl::getLocalAddress(int fd) { return Address::addressFromFd(fd); } -BaseListenerImpl::BaseListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket) +BaseListenerImpl::BaseListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket) : local_address_(nullptr), dispatcher_(dispatcher), socket_(socket) { const auto ip = socket.localAddress()->ip(); diff --git a/source/common/network/base_listener_impl.h b/source/common/network/base_listener_impl.h index 8188c01cefc09..454e685300410 100644 --- a/source/common/network/base_listener_impl.h +++ b/source/common/network/base_listener_impl.h @@ -16,13 +16,13 @@ namespace Network { */ class BaseListenerImpl : public Listener { public: - BaseListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket); + BaseListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket); protected: virtual Address::InstanceConstSharedPtr getLocalAddress(int fd); Address::InstanceConstSharedPtr local_address_; - const Event::DispatcherImpl& dispatcher_; + Event::DispatcherImpl& dispatcher_; Socket& socket_; }; diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index 7b91c9dc0fbce..58d10a0e73c4f 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -65,9 +65,8 @@ void ListenerImpl::setupServerSocket(const Event::DispatcherImpl& dispatcher, So evconnlistener_set_error_cb(listener_.get(), errorCallback); } -ListenerImpl::ListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, - ListenerCallbacks& cb, bool bind_to_port, - bool hand_off_restored_destination_connections) +ListenerImpl::ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb, + bool bind_to_port, bool hand_off_restored_destination_connections) : BaseListenerImpl(dispatcher, socket), cb_(cb), hand_off_restored_destination_connections_(hand_off_restored_destination_connections), listener_(nullptr) { diff --git a/source/common/network/listener_impl.h b/source/common/network/listener_impl.h index 3fd7931609422..c1679273c0f0e 100644 --- a/source/common/network/listener_impl.h +++ b/source/common/network/listener_impl.h @@ -11,7 +11,7 @@ namespace Network { */ class ListenerImpl : public BaseListenerImpl { public: - ListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb, + ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb, bool bind_to_port, bool hand_off_restored_destination_connections); void disable() override; diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index d86528a003c0a..0998896651486 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -16,7 +16,7 @@ namespace Envoy { namespace Network { -UdpListenerImpl::UdpListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, +UdpListenerImpl::UdpListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb) : BaseListenerImpl(dispatcher, socket), cb_(cb) { file_event_ = dispatcher_.createFileEvent( diff --git a/source/common/network/udp_listener_impl.h b/source/common/network/udp_listener_impl.h index bb0297094f055..2596a686b07e1 100644 --- a/source/common/network/udp_listener_impl.h +++ b/source/common/network/udp_listener_impl.h @@ -16,8 +16,7 @@ namespace Network { */ class UdpListenerImpl : public BaseListenerImpl { public: - UdpListenerImpl(const Event::DispatcherImpl& dispatcher, Socket& socket, - UdpListenerCallbacks& cb); + UdpListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, UdpListenerCallbacks& cb); ~UdpListenerImpl(); diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 4f5df54b429ce..55ef6403498a0 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -50,7 +50,7 @@ class MockDispatcher : public Dispatcher { } FileEventPtr createFileEvent(int fd, FileReadyCb cb, FileTriggerType trigger, - uint32_t events) const override { + uint32_t events) override { return FileEventPtr{createFileEvent_(fd, cb, trigger, events)}; } @@ -99,8 +99,8 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD1(createDnsResolver, Network::DnsResolverSharedPtr( const std::vector& resolvers)); - MOCK_CONST_METHOD4(createFileEvent_, - FileEvent*(int fd, FileReadyCb cb, FileTriggerType trigger, uint32_t events)); + MOCK_METHOD4(createFileEvent_, + FileEvent*(int fd, FileReadyCb cb, FileTriggerType trigger, uint32_t events)); MOCK_METHOD0(createFilesystemWatcher_, Filesystem::Watcher*()); MOCK_METHOD4(createListener_, Network::Listener*(Network::Socket& socket, Network::ListenerCallbacks& cb, From 3de381d43672dd705c4cd1447c0e4696b69ce56c Mon Sep 17 00:00:00 2001 From: Jojy G Varghese Date: Mon, 28 Jan 2019 21:26:54 -0800 Subject: [PATCH 32/32] Removed const Dispatcher method. Signed-off-by: Jojy G Varghese --- source/common/event/dispatcher_impl.h | 2 +- source/common/event/file_event_impl.cc | 2 +- source/common/event/file_event_impl.h | 2 +- source/common/network/listener_impl.cc | 2 +- source/common/network/listener_impl.h | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index fc0cf5326401d..c13108809df8d 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -30,7 +30,7 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { /** * @return event_base& the libevent base. */ - event_base& base() const { return *base_; } + event_base& base() { return *base_; } // Event::Dispatcher TimeSystem& timeSystem() override { return time_system_; } diff --git a/source/common/event/file_event_impl.cc b/source/common/event/file_event_impl.cc index 671865077ceb9..bc1cf9ad5ab4b 100644 --- a/source/common/event/file_event_impl.cc +++ b/source/common/event/file_event_impl.cc @@ -10,7 +10,7 @@ namespace Envoy { namespace Event { -FileEventImpl::FileEventImpl(const DispatcherImpl& dispatcher, int fd, FileReadyCb cb, +FileEventImpl::FileEventImpl(DispatcherImpl& dispatcher, int fd, FileReadyCb cb, FileTriggerType trigger, uint32_t events) : cb_(cb), base_(&dispatcher.base()), fd_(fd), trigger_(trigger) { assignEvents(events); diff --git a/source/common/event/file_event_impl.h b/source/common/event/file_event_impl.h index 3d8f233960897..feac017b79b92 100644 --- a/source/common/event/file_event_impl.h +++ b/source/common/event/file_event_impl.h @@ -16,7 +16,7 @@ namespace Event { */ class FileEventImpl : public FileEvent, ImplBase { public: - FileEventImpl(const DispatcherImpl& dispatcher, int fd, FileReadyCb cb, FileTriggerType trigger, + FileEventImpl(DispatcherImpl& dispatcher, int fd, FileReadyCb cb, FileTriggerType trigger, uint32_t events); // Event::FileEvent diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index 58d10a0e73c4f..998b52c25c466 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -47,7 +47,7 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* listener->hand_off_restored_destination_connections_); } -void ListenerImpl::setupServerSocket(const Event::DispatcherImpl& dispatcher, Socket& socket) { +void ListenerImpl::setupServerSocket(Event::DispatcherImpl& dispatcher, Socket& socket) { listener_.reset( evconnlistener_new(&dispatcher.base(), listenCallback, this, 0, -1, socket.ioHandle().fd())); diff --git a/source/common/network/listener_impl.h b/source/common/network/listener_impl.h index c1679273c0f0e..d1df0ad7c1621 100644 --- a/source/common/network/listener_impl.h +++ b/source/common/network/listener_impl.h @@ -18,7 +18,7 @@ class ListenerImpl : public BaseListenerImpl { void enable() override; protected: - void setupServerSocket(const Event::DispatcherImpl& dispatcher, Socket& socket); + void setupServerSocket(Event::DispatcherImpl& dispatcher, Socket& socket); ListenerCallbacks& cb_; const bool hand_off_restored_destination_connections_;