Introducing UDP listener. #5473

conqerAtapple · 2019-01-04T01:11:00Z

Signed-off-by: Jojy G Varghese jojy_varghese@apple.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Base layer for UDP proxying. UDP listener provides a Buffer interface to the upper layers, which in turn can add features/services like filters on top.
Risk Level: Medium
Testing: Unit testing
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]

zyfjeff · 2019-01-04T02:09:22Z

source/common/network/udp_listener_impl.cc

+void UdpListenerImpl::enable() { event_add(&raw_event_, nullptr); }
+
+void UdpListenerImpl::readCallback(int fd, short flags, void* arg) {
+  (void)flags;


UNREFERENCED_PARAMETER?

conqerAtapple · 2019-01-05T06:10:01Z

@mattklein123 @alyssawilk @mpwarres PTAL

This PR is first of the series for UDP proxying. This is extracted from the WIP #5256. I will eventuallu close #5246.

mpwarres

This is cool!! Just a few high-level comments.

mpwarres · 2019-01-07T16:24:44Z

include/envoy/event/dispatcher.h

+   * @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,


Wondering whether we will ever want to support proxying datagrams over Unix domain sockets, in addition to over UDP. (Analogous to current Envoy support for pipes vs. TCP connections.) If so, then it might be more general to name this createDatagramListener. Or maybe UDS datagrams aren't worth supporting, in which case current name SGTM.

@mpwarres Does gQUIC currently use Unix domain sockets for proxying datagrams? I vaguely recall hearing about a potential experiment with them in a QUIC talk.

Not that I'm aware. We have used SOCK_DGRAM + shared memory to allow GFEs (speaking QUIC) to efficiently front for other (non QUIC speaking) servers, but AIUI there the datagrams were just for coordinating shared memory use, not for carrying QUIC traffic. Not sure if @ianswett may have mentioned this in one of his talks, perhaps this one, though I don't see it in the slides. The comment I posted upthread was more hypothetical--I don't have a concrete use case.

I don't have a strong opinion on this, though createDatagramListener() does seem more generic.

mpwarres · 2019-01-07T16:26:13Z

include/envoy/network/listener.h

+   * @param peer_address Network address of the peer.
+   * @param data Data buffer received.
+   */
+  virtual void onData(Address::InstanceConstSharedPtr local_address,


Relaying a comment originally made by ianswett@chromium.org on an earlier listener sketch, which also applies here: from the perspective of QUIC (which will eventually use this), it would be desirable to bundle (local_address, peer_address, data) in a Datagram or Packet object, to accomodate possible future addition of other attributes, e.g. TTL. E.g., something like this.

@mpwarres , @ianswett Do you think we should have it here or in the QUIC filter layer? Do other UDP filters need the Datagram/Packet concept? Example DNS?

Seems fine to just make a struct now to carry the data if we want that? That will avoid a lot of code site changes later to add new fields.

+1, let's totally make it a struct sooner rather than later.

mpwarres · 2019-01-07T16:29:00Z

include/envoy/network/listener.h

+   */
+  virtual void onError(const ErrorCode& err_code, int err_no) PURE;
+};
+


Seems like the listener might also need to be notified when the UDP socket is writable? AIUI, in the TCP case, this interaction is a little hidden: ListenerCallbacks::onNewConnection() provides a Network::Connection, and that Connection is set up with the Dispatcher to receive events when the connection socket is readable or writable: link. The "is readable" transition is already covered by UdpListenerCallbacks::onData(), but seems like there will need to be something somewhere for the "is writable" side, to let the receipient know when it can flush/write more packets.

Good point. So I guess we need another callback onWriteReady ?

I think it depends on the UdpListenerCallbacks vs. ListenerCallbacks question. If you go with the already existing ListenerCallbacks interface like @alyssawilk is suggesting, then it would make sense to reflect writability inside the Connection implementation, i.e. Event::FileReadyType::Write -> UdpConnectionImpl::onWriteReady() which could flush any buffered packets that are waiting to be written, paired with a UdpConnectionImpl::write() implementation that buffers packets when the socket is not currently writable.

If you go with UdpListenerCallbacks, then yeah, I guess there would need to be something like an onWriteReady(), and it would need to supply something to write to (either the Socket, or an interface for writing a packet).

Not sure how to reuse existing ListenerCallbacks for UDP since it has connection concepts. I am leaning towards having onWriteReady in UdpListener. Let me know if you (@alyssawilk) feel strongly about this.

This is related to my comment above about whether we are doing connection oriented or not. IMO I'm leaning towads splitting UDP into 2 types: 1) What you have now, which is geared towards datagram, and 2) One listener type which uses normal listener callbacks, and spits out connections which then supports existing filters. Thoughts?

alyssawilk

Cool! Very excited to see progress here.

alyssawilk · 2019-01-07T16:35:02Z

include/envoy/event/dispatcher.h

+   * 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.


Any chance we can nix bind_to_port (always pass true to base classes)? We're trying to remove it for TCP (#5355) so I'd love to just not support it for UDP from the get go.

makes sense

alyssawilk · 2019-01-07T16:36:08Z

include/envoy/network/listener.h

+  virtual ~UdpListenerCallbacks() = default;
+
+  /**
+   * On first packet received on a UDP socket. This allows the callback handler


super nitty: can we pick udp / UDP / Udp and use consistently?
I'd lean towards udp (Udp iff the first word in a sentence)

alyssawilk · 2019-01-07T16:37:03Z

include/envoy/network/listener.h

+  /**
+   * Called when there is an error event.
+   *
+   * @param error_code `ErrorCode` for tbe error event.


tbe -> the error
I'd skip the backticks around ErrorCode too.

alyssawilk · 2019-01-07T16:39:57Z

source/common/buffer/buffer_impl.cc

+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};


not EINVALIDARG?

I was trying to be consistent with the same case handled elsewhere in the code. But it makes sense to return EINVALIDARG

Thinking about it a little more, here the system call is not invoked so would it be reflective of the real state of the process to change errno ?

alyssawilk · 2019-01-07T16:41:36Z

source/common/buffer/buffer_impl.cc

+  slice.len_ = std::min(slice.len_, static_cast<size_t>(rc));
+  commit(&slice, 1);
+
+  return {static_cast<int>(rc), errno};


unless you can prove that commit doesn't (and never will) tweak errno, I'd suggest either latching errno or passing an explicit 0 in the return here.

makes sense

alyssawilk · 2019-01-07T16:56:39Z

source/common/buffer/buffer_impl.cc

+  const uint64_t num_slices = reserve(max_length, &slice, 1);
+
+  ASSERT(num_slices == 1);
+  // TODO(conqerAtapple): Use os_syscalls


I'd like to structure this code to be recvmmsg friendly and I'm not sure how best to do it. Functionally here we

tell the buffer to read one packet

(eventually) dispatch one packet's worth of data to upper layers

Where I think we're doing to want to

read many packets (via recvmmsg, rx_ring whatever)

create shim buffers around those packets to hand data to upper layers

which just makes me wonder if we want this code to live in the buffer (as it does for TCP) or a utility function (which uses the raw reserved data).

I'm also 100% fine landing this today and moving it later as long as we keep in mind that it's likely to move.

Wondering if we should leave it as is for now. I am still trying to figure out how this will fit into the QUIC implementation APIs. Looking at the chromium source, the API there looks to be recvfrom friendly. Maybe I am reading it wrong. @mpwarres ?

The Chromium QUIC code can do either. QuicPacketReader::ReadAndDispatchPackets() reads packets either via recvmsg or recvmmsg depending on whether the macro MMSG_MORE is set (see QuicPacketReader::ReadAndDispatchPackets()). Then, independent of whether the packets were read individually or as a batch, they are passed one by one into the ProcessPacket() method of either QuicDispatcher (if server) or QuicClientEpollNetworkHelper (if client).

Yeah, I think both for QUIC and for other UDP applications we're going to want to read-many then dispatch-one-at-a-time. While I don't think we want to implement that code right now (launch and iterate!) I just want to make sure we design the APIs for it.

Then again I'm also fine landing this as is and moving it later - I don't personally mind a lot of code churn while we feel our way out as long as we all know what we're getting into :-P

I agree that we need a recvmmsg implementation also (thanks to @mpwarres for clarifying). I am not sure how do we pass configurable parameters (number of messages for recvmmsg) to the listener. At this this I am leaning towards adding TODO for recvmmsg and attack that in my next PR.

alyssawilk · 2019-01-07T17:01:24Z

include/envoy/network/listener.h

+/**
+ * UDP listener callbacks.
+ */
+class UdpListenerCallbacks {


I thought Matt requested on the prior PR that we try to stick to listenercallbacks where possible?

I'd think the current code could be a subclass of ListenerCallbacks, where onAccept was commeted as "accepting new data" rather than "accepting a new connection", we call onNewConnection where we currently do (but with a UdpConnection subclass which may be filled with TODOs, but we pass the local address and the peer address to it) and then hand onData calls to the new connection?

Same comment as above. The current ListenerCallbacks interface exposes connection concept. Inheriting that interface would semantically mean - UdpListenerCallbacks is-a ListenerCallbacks. Not sure if we mean that. What about having a bridge that bridges the UDP listener to the existing ListenerCallbacks? This bridge could be a concrete implementation of a UdpListenerCallbacks interface. Thinking out loud here. Would he happy to hear what you think.

See my comments below.

cmluciano · 2019-01-09T16:37:40Z

include/envoy/network/listener.h

+
+  /**
+   * On first packet received on a UDP socket. This allows the callback handler
+   * to establish filter chain (or any other prepararion).


Suggested change

* to establish filter chain (or any other prepararion).

* to establish filter chain (or any other preparation).

mattklein123

Flushing out some comments at the interface layer. I did not look at the implementation/tests yet. Very cool to see this happening.

mattklein123 · 2019-01-09T17:21:24Z

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,


nit: doc comments. As a larger comment, I would love to remove any type of file IO from the buffer interface moving forward since in the long term I don't think it fits with our FD refactors. Any thoughts on how we might not add it for this case since it's new? cc @brian-pane who is working on a new buffer impl.

mattklein123 · 2019-01-09T17:22:45Z

include/envoy/event/dispatcher.h

+   * @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,


I don't have a strong opinion on this, though createDatagramListener() does seem more generic.

mattklein123 · 2019-01-09T17:25:49Z

include/envoy/network/listener.h

+/**
+ * UDP listener callbacks.
+ */
+class UdpListenerCallbacks {


See my comments below.

mattklein123 · 2019-01-09T17:28:06Z

include/envoy/network/listener.h

+   * @param peer_address Network address of the peer.
+   * @param data Data buffer received.
+   */
+  virtual void onNewConnection(Address::InstanceConstSharedPtr local_address,


I assume for the purpose of this PR, we are looking at stateless vs. connection oriented proxy, right? I guess my question comes down to, at this layer, what does onNewConnection() give us. It seems like there must be internal tracking to figure out whether this is "new", but then how are connections ever going to get cleaned up? Where is the interface for that? It seems like at this datagram layer, we could just have onData() and these are the interfaces for raw datagram listening. If we want connection oriented, could we just use the normal ListenerCallbacks which spit out normal connections which are then UdpConnectionImpl? Thoughts on this?

@mattklein123
Thanks for reviewing.

What about layering the connection concept on top of UDP/datagram? This would allow us to have 1 common buffer layer and then connections (eg QUIC) on top of it. When a connection oriented filter (QUIC) gets the onNewConnection event, it can then decide to do things like 1) prepare the protocol layers 2) Carve a protocol specific Packet and send it up the protocol layer.

The problem is I don't understand how this layer is going to do conn tracking if the API looks like this. I think you need an API to "delete" a connection if that's the case?

@mattklein123 Since this layer does not have a concept of Connection, it doesn't need to track connections. Right? I would think that something like QuicListener(that implements the UdpListenerCallbacks) could be tracking connections. No?

But then I think the onNewConnection API needs to be removed? What does a new connection mean without conn tracking?

Yes that is a good point. The book keeping of if this is the first data packet has to be done somewhere, assuming that the state of first data is important to track. If it is not, then the API onNewConnection need not be there.

Yeah IMO if we are going to build connection oriented on top of this, I would kill onNewConnection and any tracking from this PR.

mattklein123 · 2019-01-09T17:28:54Z

include/envoy/network/listener.h

+   * @param peer_address Network address of the peer.
+   * @param data Data buffer received.
+   */
+  virtual void onData(Address::InstanceConstSharedPtr local_address,


Seems fine to just make a struct now to carry the data if we want that? That will avoid a lot of code site changes later to add new fields.

mattklein123 · 2019-01-09T17:30:10Z

include/envoy/network/listener.h

+   */
+  virtual void onError(const ErrorCode& err_code, int err_no) PURE;
+};
+


This is related to my comment above about whether we are doing connection oriented or not. IMO I'm leaning towads splitting UDP into 2 types: 1) What you have now, which is geared towards datagram, and 2) One listener type which uses normal listener callbacks, and spits out connections which then supports existing filters. Thoughts?

conqerAtapple · 2019-01-09T17:48:25Z

@mattklein123 @mpwarres @alyssawilk @cmluciano

Thanks for reviewing. I am summarizing the changes:

Will fix spellchecks/comments suggestions
Will add UdpPacket for onData (and onNewConnection)
Will add onWriteReady
Move recvFrom from buffer

In my next PR(s):

Add implementation for recvmmsg
Flush out the layering (ListenerCallbacks , UdpListenerCallbacks)

What do you guys think?

alyssawilk

looking better and better!

alyssawilk · 2019-01-14T16:56:34Z

include/envoy/network/listener.h

+ */
+struct UdpData {
+  Address::InstanceConstSharedPtr local_address_;
+  Address::InstanceConstSharedPtr peer_address_;


I suspect we can get away with unique pointers here and I'd prefer to if we can get away with it, but I'm fine sorting out ownership in a follow-up.

good point . Will change it to unique_ptr

@alyssawilk I was looking at it again and it looks like it might be difficult to change local_address to a unique_ptr as it is derived from the listener's Socket object. So I am thinking we could just have the peer_address as unique_ptr. It will convey the right ownership semantics but will look odd. WDYT?

If this is a per-packet UdpData struct, should it use a shared pointer address from the socket?

I think the common case for QUIC-as-a-large-scale-server (or DNS too) is you're going to have a listener on 0.0.0.0:whatever_port which gets packets for a bunch of addresses. Imagine we've got a quic listener on 0.0.0.0, which gets a packet for local_address 1.2.3.4 We stick 1.2.3.4 in local_address based on the UDP message headers, hand the local_address up the stack, some other object snags a shared pointer assuming that's the local address for the connection. Meanwhile another packet comes in from 6.7.8.9. We could create a new shared pointer for each packet for the server case, but I think we're better off perf-optimizing for the common case, having an impl (not even an unique pointer) in the per-packet data.

That said you can totally TODO this and we can revisit later :-)

Wanted to clarify that I was talking about local_address that is from the listening socket. peer_address could be unique for each packet as its derived from the UDP packet. I assume you mean the same but just wanted to be clear :)

@alyssawilk I have updated comments based on ☝️

alyssawilk · 2019-01-14T17:03:06Z

include/envoy/network/listener.h

+   * @param error_code ErrorCode for the error event.
+   * @param errno System error number.
+   */
+  virtual void onError(const ErrorCode& err_code, int err_no) PURE;


super nitty, but can you use full variable names?
error_code error_number etc.?

I think there's still a few of these outstanding. Mind fixing up?

I fixed them (latest commits) but looking at your comment again, I missed fixing err_no as you suggested. Will fix that.

alyssawilk · 2019-01-14T17:07:04Z

include/envoy/event/dispatcher.h

+   * @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,


I was about to ask that we be consistent with Udp vs Datagram when I realized you were totally doing this right per mpw@'s advice.
I think when we get to docs it's worth doccing up that this is meant to be generic (potentially extending to datagrams over sockets) where the listener is for UdpSockets for others who have the same question :-P

I think I take back my comment (sorry the the churn :-} ): since everything else in the PR refers to "Udp", it seems a little out of place and inconsistent to use "Datagram" here, better to go with "Udp" which you had originally.

alyssawilk · 2019-01-14T17:13:22Z

test/common/network/listener_impl_test.cc

+/**
+ * Tests UDP listener for actual destination and data.
+ */
+TEST_P(ListenerImplTest, UseActualDstUdp) {


now that we have udp_listener_impl.* files, we should probably move shared test code to a common utility class and put these tests in /test/common/network/udp_listener_impl_test, yeah?

conqerAtapple · 2019-01-22T17:18:46Z

@alyssawilk PR is ready for another review. PTAL.

conqerAtapple · 2019-01-23T16:09:47Z

@mattklein123 @alyssawilk ping

alyssawilk · 2019-01-23T16:21:21Z

Oh sorry, missed this yesterday. I'm out this morning but I'll try to do a pass this afternoon and sorry for the delay

alyssawilk

I think we've got all my concerns TODOd other than that one naming nit. @mattklein123 / @mpwarres anything else you want to land here or should we launch/land and iterate?

mpwarres

Looks great, from my perspective! Since this is packet-level, we can layer the QuicListener on top of this.

mpwarres · 2019-01-24T18:24:11Z

include/envoy/event/dispatcher.h

+   * @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,


I think I take back my comment (sorry the the churn :-} ): since everything else in the PR refers to "Udp", it seems a little out of place and inconsistent to use "Datagram" here, better to go with "Udp" which you had originally.

mpwarres · 2019-01-24T18:25:47Z

include/envoy/network/listener.h

+ * TODO(conqerAtapple): Maybe this belongs inside the UdpListenerCallbacks
+ * class.
+ */
+struct UdpData {


optional naming suggestion (feel free to disregard): UdpPacket

mpwarres · 2019-01-24T19:53:46Z

include/envoy/network/listener.h

+ */
+class UdpListenerCallbacks {
+public:
+  enum class ErrorCode { SYSCALL_ERROR, UNKNOWN_ERROR };


nit: I think Envoy uses CamelCase for enum value names

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

- Removed bind_to_port from UdpListener ctor. - Spellchecks and comments - Added EV_WRITE flag for udp listener Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

conqerAtapple · 2019-01-26T17:07:00Z

@mattklein123 Thanks for review. I have addressed all comments , except the FileEvent one as I am not very clear on how the enable/disable functionality would work.

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

mattklein123

LGTM with small comment (though I think you have a CI issue)

/wait

mattklein123 · 2019-01-27T16:43:36Z

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;


We are somewhat splitting hairs here, but IMO this is not a const operation on the dispatcher, it does modify its state. Instead of changing this to a const method can you pass a non-const Dispatcher to where you need it? This is what is done elsewhere.

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

conqerAtapple · 2019-01-28T00:04:16Z

@mattklein123 I have addressed all comments now. I think coverage tests are failing because of memory issues.

mattklein123 · 2019-01-28T16:41:03Z

/retest

repokitteh-read-only · 2019-01-28T16:41:08Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #5473 (comment) was created by @mattklein123.

see: more, trace.

mattklein123

This is awesome. LGTM modulo a few remaining const reverts. Thank you!

/wait

mattklein123 · 2019-01-28T16:42:48Z

source/common/event/dispatcher_impl.h

@@ -30,7 +30,7 @@ class DispatcherImpl : Logger::Loggable<Logger::Id::main>, public Dispatcher {
  /**
   * @return event_base& the libevent base.
   */
-  event_base& base() { return *base_; }
+  event_base& base() const { return *base_; }


Can you revert this const change also and a couple of the other ones you added?

I will remove them but I think they should be const :)

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

conqerAtapple · 2019-01-29T05:27:56Z

@mattklein123 Thanks again for the reviews. I think all reviews are addressed now.

conqerAtapple · 2019-01-29T16:09:00Z

@mattklein123 The release check has once again failed with server terminated abruptly

mattklein123 · 2019-01-29T16:21:48Z

@conqerAtapple we are working on the CI issues, in the meantime you can rerun failed tests yourself, like so:

/retest

repokitteh-read-only · 2019-01-29T16:21:53Z

🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #5473 (comment) was created by @mattklein123.

see: more, trace.

mattklein123

Awesome!!

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com> Signed-off-by: Jojy George Varghese <jojy.varghese@gmail.com>

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com> Signed-off-by: Fred Douglas <fredlas@google.com>

conqerAtapple requested a review from lizan as a code owner January 4, 2019 01:11

zyfjeff reviewed Jan 4, 2019

View reviewed changes

mattklein123 self-assigned this Jan 6, 2019

alyssawilk self-assigned this Jan 7, 2019

mpwarres reviewed Jan 7, 2019

View reviewed changes

alyssawilk reviewed Jan 7, 2019

View reviewed changes

mpwarres mentioned this pull request Jan 9, 2019

Bootstrap and configuration of monolithic QuicListener. #5549

Closed

cmluciano reviewed Jan 9, 2019

View reviewed changes

mattklein123 reviewed Jan 9, 2019

View reviewed changes

mattklein123 added the waiting label Jan 11, 2019

repokitteh-read-only bot removed the waiting label Jan 12, 2019

alyssawilk reviewed Jan 14, 2019

View reviewed changes

alyssawilk reviewed Jan 23, 2019

View reviewed changes

mpwarres previously approved these changes Jan 24, 2019

View reviewed changes

conqerAtapple added 11 commits January 24, 2019 15:29

Introducing UDP listener.

0509ee3

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Removed MSG_CONFIRM flag for sendto method.

eca712d

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Added more tests for UDP listener.

37232e4

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Fixed recvfrom arguments.

3c7e6f6

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Taking care of -EAGAIN.

6389f68

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Added support for mock buffer.

bd240a5

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Added support for onError callback.

3a973f1

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Removed move operator.

6803aee

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Addressed some review comments.

eed7d49

- Removed bind_to_port from UdpListener ctor. - Spellchecks and comments - Added EV_WRITE flag for udp listener Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Added onWriteReady implementation.

0bcfbb8

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Added echo test for UDP listener.

a1afecf

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

repokitteh-read-only bot removed the waiting label Jan 26, 2019

mattklein123 added the waiting label Jan 27, 2019

Changed to FileEvent.

3c1f89a

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

repokitteh-read-only bot removed the waiting label Jan 27, 2019

mattklein123 requested changes Jan 27, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 27, 2019

Removed extraneous parenthesis.

ce18742

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

repokitteh-read-only bot removed the waiting label Jan 27, 2019

mattklein123 added the waiting label Jan 27, 2019

Removed const'ness for Dispatcher in listener.

972819a

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

repokitteh-read-only bot removed the waiting label Jan 27, 2019

mattklein123 requested changes Jan 28, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 28, 2019

Removed const Dispatcher method.

3de381d

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

repokitteh-read-only bot removed the waiting label Jan 29, 2019

mattklein123 approved these changes Jan 29, 2019

View reviewed changes

mattklein123 merged commit 6c78f5b into envoyproxy:master Jan 30, 2019

conqerAtapple deleted the udp_listener branch January 30, 2019 00:24

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Jan 31, 2019

Introducing UDP listener. (envoyproxy#5473)

c8943fd

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com> Signed-off-by: Jojy George Varghese <jojy.varghese@gmail.com>

fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019

Introducing UDP listener. (envoyproxy#5473)

0ce6578

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com> Signed-off-by: Fred Douglas <fredlas@google.com>

	* to establish filter chain (or any other prepararion).
	* to establish filter chain (or any other preparation).

Introducing UDP listener. #5473

Introducing UDP listener. #5473

Conversation

conqerAtapple commented Jan 4, 2019

Choose a reason for hiding this comment

conqerAtapple commented Jan 5, 2019

mpwarres left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conqerAtapple Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conqerAtapple Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conqerAtapple Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conqerAtapple commented Jan 9, 2019

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conqerAtapple commented Jan 22, 2019

conqerAtapple commented Jan 23, 2019

alyssawilk commented Jan 23, 2019

alyssawilk left a comment

Choose a reason for hiding this comment

mpwarres left a comment

conqerAtapple Jan 8, 2019 •

edited

Loading

conqerAtapple Jan 7, 2019 •

edited

Loading

conqerAtapple Jan 9, 2019 •

edited

Loading