exporting underlying Connection

[mschneider82]

this blocks implementing the listener in traefik

traefik/traefik#7320

If you accept it, please create a new version tag, thank you!

closes #50

[coveralls]

Coverage increased (+0.1%) to 93.46% when pulling 24616a4 on mschneider82:master into 7962c3f on pires:master.

[pires]

I'm trying to understand the issue that drives this change so this feedback is preliminary and assumes the problem can only be addressed by exposing the underlying net.Conn.

From the top of my head, I think a better option here would be for the attribute not to be exported but for an accessor (getter) to be added to Conn, say

func (p *Conn) Raw() net.Conn {
    return p.conn
}

[mschneider82]

A getter is fine to me. Should I change my pr or will you add it to master? (Raw() needs a comment)

[pires]

Go ahead! You started the work, after all. Thank you!

[pires]

Also, can you please add tests that validate casts to, at least, TCPConn?

[mschneider82]

What do you think? Or change it to Raw() ?

[pires]

I think this is good as we have other pieces of code that validate casts as well 👍 @emersion do you want to comment?

[pires]

LGTM! If no discussion arises, will merge over the weekend.

[emersion]

If you accept it, please create a new version tag, thank you!

FWIW, you can upgrade to a specific commit with a need for a new version:

go get github.com/pires/go-proxyproto@<commit>

[pires]

Thank you very much for your contribution, @mschneider82!

[guysv]

Hey. say I called the proxy wrapper's Read(), then unwrapped it to get the underlying connection, then tried to read from that underlying connection. Aren't I going to miss data that might reside in the proxy wrapper's buffered reader's internal buffer?

I modified the new test like so, and it no longer pass

diff --git a/protocol_test.go b/protocol_test.go
index daadd37..e8601d3 100644
--- a/protocol_test.go
+++ b/protocol_test.go
@@ -487,7 +487,7 @@ func Test_ConnectionCasts(t *testing.T) {
                t.Fatalf("err: %v", err)
        }

-       policyFunc := func(upstream net.Addr) (Policy, error) { return REQUIRE, nil }
+       policyFunc := func(upstream net.Addr) (Policy, error) { return IGNORE, nil }

        pl := &Listener{Listener: l, Policy: policyFunc}

@@ -497,7 +497,7 @@ func Test_ConnectionCasts(t *testing.T) {
                        t.Fatalf("err: %v", err)
                }
                defer conn.Close()
-               conn.Write([]byte("ping"))
+               conn.Write([]byte("pingpong"))
        }()

        conn, err := pl.Accept()
@@ -506,8 +506,14 @@ func Test_ConnectionCasts(t *testing.T) {
        }
        defer conn.Close()

+       outbuf := make([]byte, 4)
+       n, err := conn.Read(outbuf)
+       if n != 4 {
+               t.Fatal("err: failed to read from connection wrapper", err)
+       }
+
        proxyprotoConn := conn.(*Conn)
-       _, ok := proxyprotoConn.TCPConn()
+       tcpconn, ok := proxyprotoConn.TCPConn()
        if !ok {
                t.Fatal("err: should be a tcp connection")
        }
@@ -519,6 +525,11 @@ func Test_ConnectionCasts(t *testing.T) {
        if ok {
                t.Fatal("err: should be a tcp connection not unix")
        }
+
+       n, err = tcpconn.Read(outbuf)
+       if n != 4 {
+               t.Fatal("err: failed to read from underlying connection", err)
+       }
 }

[emersion]

Indeed, that sounds correct. We should probably document that.

[guysv]

Also, I guess we could provide a method to drain the internal buffer (however that is done) because someone probably will be interested in that data.

[emersion]

This isn't really possible, once some data has been read from the underlying net.Conn, there's no way to put it back as if it wasn't read.

[guysv]

Oh yes. of course. I was talking about exposing api to retrieve the data, and let the caller figure out what to do with it. I did not mean to get the data back into the connection.

[emersion]

Users can read the buffered data from the proxyproto.Conn. Or am I missing something?

[guysv]

Well, without calling the internal reader's Reader.Buffered you can't tell how much you can read without triggering another read from the connection itself.

[emersion]

Oh, so you mean that you want to get the underlying bufio.Reader?

[guysv]

Just proxying Reader.Buffered will do too. I just wonder if wouldn't you guys prefer to have a unified method that will both drain any remaining data from the internal buffer, and return the underlying conn. I'd be happy to send a PR for both approaches.

[emersion]

I'm worried about people asking to add methods for other bufio.Reader functions in the future.

The stdlib just exposes the bufio.Reader in these situations. I think it would be fine to do the same.