Skip to content

Conversation

@igorwwwwwwwwwwwwwwwwwwww
Copy link
Contributor

The current behaviour of Read is to discard the underlying error and return ErrNoProxyProtocol. That makes it tricky to debug what is wrong.

In my case, I was trying to nest tls.NewListener inside a proxyproto.Listener but got the ordering wrong, the underlying error was:

tls: first record does not look like a TLS handshake

This patch surfaces that error to the caller.

Note: It may be a good idea to do the same thing with the other .Peek() calls too.

@pires
Copy link
Owner

pires commented May 6, 2020

Hey @igorwwwwwwwwwwwwwwwwwwww! Thanks for the report and patch. Could you add a test that mimics your scenario so we see the TLS error actually bubbling up? And since we're at it, could you also please change all other Peek() calls to return an error?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 91.176% when pulling fb3158b on igorwwwwwwwwwwwwwwwwwwww:patch-1 into 62dfc14 on pires:master.

@igorwwwwwwwwwwwwwwwwwwww
Copy link
Contributor Author

@pires I've added a test with a reader that returns a custom error.

I also added some logic to handle EOF differently. Some tests are expecting EOF to return ErrNoProxyProtocol, and that seems reasonable.

The error handling got a bit gnarly, so if you have suggestions for how to refactor this, I'm happy to do so.

Copy link
Owner

@pires pires left a comment

Choose a reason for hiding this comment

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

Your code seems to be easier to read that what I have so no worries there, thanks!

Copy link
Owner

@pires pires left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution!

@pires pires added the bug label May 7, 2020
@pires pires merged commit a55d19c into pires:master May 7, 2020
@pires
Copy link
Owner

pires commented May 7, 2020

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants