Skip to content

WIP: UDP proxy - basic listener.#5256

Closed
conqerAtapple wants to merge 14 commits intoenvoyproxy:masterfrom
conqerAtapple:udp_proxy
Closed

WIP: UDP proxy - basic listener.#5256
conqerAtapple wants to merge 14 commits intoenvoyproxy:masterfrom
conqerAtapple:udp_proxy

Conversation

@conqerAtapple
Copy link
Contributor

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: Adds UDP listener and basic infrastructure for UDP proxy.
Risk Level: medium
Testing: Unit tests (TODO)
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue]
[Optional Deprecated:]

@conqerAtapple
Copy link
Contributor Author

@alyssawilk @mattklein123 @mpwarres @cmluciano

First attempt at the udp stack. Wanted to put some basic structures out there to get feedback.

@zuercher
Copy link
Member

Not going to assign anyone at the moment, so this is a WIP. Note that many of the maintainers are at Kubecon and so might not be super responsive this week.

@zuercher zuercher added the no stalebot Disables stalebot from closing an issue label Dec 11, 2018
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay UDP progress! Sorry about review lag - I forgot to tag my calendar with OOO around EnvoyCon :-P

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at some layer we're going to want to have the Udp stack have the equivalent of the existing ListenerCallbacks. While it's true that there's no UDP accept, the concepts "I have a new connection" and "I have gone through a series of filters acting at a very low level before I formally accept the connection" may still apply. Looking at Envoy listener filters, there's proxy proto and tls inspector. I think we can have similar functionality for UDP - we have our own hacked version of the proxy protocol header which we achieve via packet munging and Envoy will want an equivalent, and you can totally SNI-sniff QUIC.

Envoy current listener filters do work by delaying accept and doing peek, which is so not working for UDP :-P That said in-house there are areas where we do light-weight packet queueing until we have enough data we can tell how to proceed, so I think a logical equivalent of listener filters and "peek" do still kind of work and we will want them at the end of the day.

I'm fine landing this as-is, with a TODO to investigate sharing ListenerCallback in future once we have more of a handle of how things are going, but I figure it's at least worth discussing now. It may be worth at least considering if we can reuse more of the existing listener code now, with just config rejection if anyone tries to insert a tcp listener filter on the UDP stack.

@mpwarres @mattklein123

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, my thinking was to share as many of the public interfaces we have now as possible until we figure out we absolutely can't do it. Could we start potentially with that being the goal?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also the original_src filter I'm writing (#5337). It should work just as well with UDP as with TCP as long as something can figure out the source (i.e. UDP version of proxy protocol) prior to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alyssawilk I think the UDP listener should be able to do what you suggested. Since at the UDP listener layer, we are still dealing with "Buffer", we should be able to stack filters. Right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now's also a good time to think about how we want to add build options.

I'm happy to have UDP default on (so our CI builds it) but make it easy for folks to opt out (especially to make things less painful for our windows build, while we work on cross-platform support. I strongly suspect QUIC specifically but also likely UDP in general are going to work for Linux for a first pass. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No real preference for me on whether we allow compile out on a first pass. Maybe as a follow up? I'm fine either way but definitely a good callout.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I need to see more of what we plan to do here before I can give a super useful review, sorry :-/

@mattklein123 mattklein123 self-assigned this Dec 14, 2018
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a high level skim, this is basically what I had in mind for how we would get started. per @alyssawilk though, could you potentially summarize where you want to get to with this PR so that we can help review? We have had lots of discussions in other docs about I think these three options:

  1. Static proxy with no filters (e.g., DNS) ("1a light")
  2. Static proxy with filters (e.g., DNS with some transform/inspection) ("1a")
  3. Stateful proxy ("1b")

@mattklein123
Copy link
Member

@conqerAtapple can you ping when this is ready for review again?

/wait

@conqerAtapple
Copy link
Contributor Author

@mattklein123 @alyssawilk
I think I am at a stage where the basic constructs:

  • Udp listener : passes UDP packets up the stack
  • Udp listener callbacks: Interface that acts as callback for UDP buffer events on a socket

I have the UDP listener hooked to the existing ConnectionHandler. There are two paths from here:

A)

  • Remove the UDP listener code in ConnectionHandler
  • Stop this PR at the UDP listener level and have another PR that continues the filter path. By this I mean, hooking up the UDP filter framework.
  • I might need help with deciding if the existing ConnectionHandler is the right place for the UDP filter stack.

B) Continue the PR till we have a UDP filter stack.

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
@mattklein123
Copy link
Member

@conqerAtapple I would be in favor of multiple small PRs. If there is a good stopping point where we can review code and have tests that cover it, I would say let's stop? If we can do that and you can get this into final form for review that would be awesome.

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
@mattklein123
Copy link
Member

I'm going to go ahead and close this out since I think we are working through individual PRs at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no stalebot Disables stalebot from closing an issue waiting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants