Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add downstream PROXY protocol support #19

Closed
wants to merge 1 commit into from

Conversation

contrun
Copy link

@contrun contrun commented Apr 15, 2021

This PR add downstream PROXY protocol support. Reading from the source of github.com/mastercactapus/proxyprotocol, it seems to me that the matcher will not blockingly wait even if there is not enough data. But I need to verify that and find a way to reuse github.com/mastercactapus/proxyprotocol.

// Then saves the terminated proxy protocol to `terminated_proxy_connection`.
func (m MatchPROXYProtocol) Match(cx *layer4.Connection) (bool, error) {
// We use `cx.Conn` instead of `cx` here as proxyprotocol already only peeks into the packet.
conn := proxyprotocol.NewConn(cx.Conn, time.Time{})
Copy link
Author

Choose a reason for hiding this comment

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

Here proxyprotocol will read a few bytes from the underlying OS FD. It is OK if we eventually use conn. But other matchers will not work as there will be a few bytes discarded. I can't conn := proxyprotocol.NewConn(cx, time.Time{}) either. It would cause a infinite calling loop of conn.Read() -> cx.Conn.Read() -> conn.Read() as both layer4.Connection and proxyprotocol.NewConn embed the same net.Conn. Unfortunately github.com/mastercactapus/proxyprotocol and other golang PROXY protocol library do not provide a easy way to read from a io.Reader and then return the PROXY protocol's header or an error. This will make our life easier as we already have buffered IO with layer4.Connection. I find it hard to reuse existing libraries. What are @mholt and @mastercactapus 's opinions on this issue? Should I just write a parser for PROXY protocol just like what @mholt did for TLS?

Choose a reason for hiding this comment

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

If you can get access to the underlying buffered reader, you could use Parse: func Parse(r *bufio.Reader) (Header, error) that should work as-is.

Otherwise, I could look at adding a ReadHeader(io.Reader) (Header, error) function to the library. The difficulty is in the need to call Read byte-by-byte for the V1 header since it's newline terminated.

In either case, you'd need to layer or at least re-use the same bufio.Reader to preserve the extra bytes. Alternatively, a ReadHeader method could return any buffered bytes as a slice, similar to json.Decoder

Copy link
Owner

Choose a reason for hiding this comment

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

I would recommend passing in cx here, since a few bytes may be buffered from a prior matcher, as you mentioned.

Indeed, though, this can cause a bit of a hanging problem as described in #18. I hope to be pushing a fix for that today, so follow that issue and I'll try to follow up here if I remember. :)

Copy link
Owner

@mholt mholt Apr 16, 2021

Choose a reason for hiding this comment

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

@contrun Ok, fixed in 5f9948f. Here's how it'll look in your PR:

  • Pass cx into NewConn().
  • If you need a separate Connection that doesn't wrap itself, do something like cx = cx.Wrap(conn).

Let me know how that goes!

Copy link
Author

Choose a reason for hiding this comment

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

@mholt Thank you for your work in 5f9948f . I tried the Wrap function, it's working great. I added a json configuration which works on my machine. The only remaining issue for me is that conn := proxyprotocol.NewConn(cx, time.Time{}) seems to disable timeout configuration of the underlying connection. I think the signature here https://pkg.go.dev/github.com/pires/go-proxyproto#NewConn is more reasonable than https://pkg.go.dev/github.com/mastercactapus/proxyprotocol?utm_source=godoc#NewConn , as it does not require us to set a timeout interval. I'd like to hear @mastercactapus 's opinion.

Choose a reason for hiding this comment

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

Hmm, if no deadline is set (or is zero) expected behavior IMO would be to not set (thus not un-set) the deadline, I'd consider that a bug.
mastercactapus/proxyprotocol#4

I'll try to get a fix pushed up for that.

Copy link
Owner

Choose a reason for hiding this comment

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

You're awesome, thanks!

Choose a reason for hiding this comment

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

Have this in a branch: https://github.com/mastercactapus/proxyprotocol/tree/use-io-reader

  • Parse now takes an io.Reader and using ReadByte if available otherwise falling back to Read
  • Deadline methods will no longer be called if time.Time{} is passed to NewConn
  • A new WrapConn method will simply do the wrap and block/read/parse the header immediately and is intended to replace NewConn
  • A new WrapConnReader can be used to specify the reader for the connection (e.g., use existing *bufio.Reader)

Trying to handle the deadline stuff with delayed parsing just seems like a bad idea as I'm looking at it. Especially since the deadline methods should be able to be used concurrently, and thus there are issues with synchronizing that on top of determining expected behavior.

Not 100% on it all yet, would love feedback on what options/changes seem to fit best.

@mholt
Copy link
Owner

mholt commented Jun 19, 2021

@contrun Do you want to finish this up?

@mholt
Copy link
Owner

mholt commented Jul 16, 2021

Closing due to inactivity and merge conflict

@mholt mholt closed this Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants