[net] proxy : Added DialContext() to Dialer interface
Martin Garton (Gerrit)
Martin Garton has uploaded this change for review.
proxy : Added DialContext() to Dialer interface Added DialContext() function to allow dialling a socks5 connection with a context, similar to net.DialContext in the standard library. Change-Id: I0ae42b0cd72fa1cb3183f0279e8c1e31c8297521 --- M proxy/direct.go M proxy/per_host.go M proxy/per_host_test.go M proxy/proxy.go M proxy/socks5.go 5 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/proxy/direct.go b/proxy/direct.go
index 4c5ad88..3dfd7a6 100644
--- a/proxy/direct.go
+++ b/proxy/direct.go
@@ -5,6 +5,7 @@
package proxy
import (
+ "context"
"net"
)
@@ -16,3 +17,7 @@
func (direct) Dial(network, addr string) (net.Conn, error) {
return net.Dial(network, addr)
}
+func (direct) DialContext(ctx context.Context, network, addr string) (net.Conn, error) {
+ var d net.Dialer
+ return d.DialContext(ctx, network, addr)
+}
diff --git a/proxy/per_host.go b/proxy/per_host.go
index f540b19..741a431 100644
--- a/proxy/per_host.go
+++ b/proxy/per_host.go
@@ -5,6 +5,7 @@
package proxy
import (
+ "context"
"net"
"strings"
)
@@ -41,6 +42,15 @@
return p.dialerForRequest(host).Dial(network, addr)
}
+func (p *PerHost) DialContext(ctx context.Context, network, addr string) (c net.Conn, err error) {
+ host, _, err := net.SplitHostPort(addr)
+ if err != nil {
+ return nil, err
+ }
+
+ return p.dialerForRequest(host).DialContext(ctx, network, addr)
+}
+
func (p *PerHost) dialerForRequest(host string) Dialer {
if ip := net.ParseIP(host); ip != nil {
for _, net := range p.bypassNetworks {
diff --git a/proxy/per_host_test.go b/proxy/per_host_test.go
index a7d8095..da7fe56 100644
--- a/proxy/per_host_test.go
+++ b/proxy/per_host_test.go
@@ -5,6 +5,7 @@
package proxy
import (
+ "context"
"errors"
"net"
"reflect"
@@ -20,6 +21,10 @@
return nil, errors.New("recordingProxy")
}
+func (r *recordingProxy) DialContext(ctx context.Context, network, addr string) (net.Conn, error) {
+ return r.Dial(network, addr)
+}
+
func TestPerHost(t *testing.T) {
var def, bypass recordingProxy
perHost := NewPerHost(&def, &bypass)
diff --git a/proxy/proxy.go b/proxy/proxy.go
index 78a8b7b..1ca08b3 100644
--- a/proxy/proxy.go
+++ b/proxy/proxy.go
@@ -7,6 +7,7 @@
package proxy // import "golang.org/x/net/proxy"
import (
+ "context"
"errors"
"net"
"net/url"
@@ -17,6 +18,8 @@
type Dialer interface {
// Dial connects to the given address via the proxy.
Dial(network, addr string) (c net.Conn, err error)
+ // Dial connects to the given address via the proxy using the provided context.
+ DialContext(ctx context.Context, network, addr string) (c net.Conn, err error)
}
// Auth contains authentication parameters that specific Dialers may require.
diff --git a/proxy/socks5.go b/proxy/socks5.go
index 973f57f..76fe66f 100644
--- a/proxy/socks5.go
+++ b/proxy/socks5.go
@@ -5,6 +5,7 @@
package proxy
import (
+ "context"
"errors"
"io"
"net"
@@ -62,13 +63,18 @@
// Dial connects to the address addr on the network net via the SOCKS5 proxy.
func (s *socks5) Dial(network, addr string) (net.Conn, error) {
+ return s.DialContext(context.Background(), network, addr)
+}
+
+// Dial connects to the address addr on the network net via the SOCKS5 proxy using the provided Context.
+func (s *socks5) DialContext(ctx context.Context, network, addr string) (net.Conn, error) {
switch network {
case "tcp", "tcp6", "tcp4":
default:
return nil, errors.New("proxy: no support for SOCKS5 proxy connections of type " + network)
}
- conn, err := s.forward.Dial(s.network, s.addr)
+ conn, err := s.forward.DialContext(ctx, s.network, s.addr)
if err != nil {
return nil, err
}
To view, visit change 37641. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick posted comments on this change.
Patch set 1:
(5 comments)
Patch Set #1, Line 7:
proxy : Added DialContext() to Dialer interfaceproxy: add context support with new DialContext methods
Patch Set #1, Line 9:
Added DialContext() function to allow dialling a socks5 connectiondrop the ()
typo: dialing
Missing reference to open bug:
Updates golang/go#nnnnn
DialContext(ctx context.Context, network, addr string) (c net.Conn, err error) }
adding a method to an existing interface will break people using proxy.Dialer on their own types without those methods.
that is, it breaks people calling https://godoc.org/golang.org/x/net/proxy#RegisterDialerType
that's probably not okay.
s.connect(conn, addr); err != nil { conn.Close() return nil, err }this part needs ctx loving too
To view, visit change 37641. To unsubscribe, visit settings.
Martin Garton (Gerrit)
Martin Garton posted comments on this change.
Patch set 1:
(1 comment)
DialContext(ctx context.Context, network, addr string) (c net.Conn, err error) }
adding a method to an existing interface will break people using proxy.Dial
Thanks and good point; I had overlooked RegisterDialerType, and there are at least a handful of real uses in the wild that can easily be found.
Unfortunately, I now can't see a way to sensibly implement what is needed, short of duplicating almost the whole package and making a Context variant. Adding a separate interface such as DialerContext doesn't really help. The fact is, for any Dialer type to be able to use Contexts, all of them need to support it. I'm kinda stuck and will probably abandon this CL.
To view, visit change 37641. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick posted comments on this change.
Patch Set #1, Line 17:
func SOCKS5(network, addr string, auth *Auth, forward Dialer) (Dialer, error) {if this returned a concrete type instead of an interface, we could just add methods at will.
Rename the "socks5" type to "SOCKS5Dialer" maybe?
Oh, gross: https://godoc.org/golang.org/x/net/proxy#NewPerHost is named "NewFoo" to return a Foo, but SOCKS5 doesn't have the "New" suffix.
The other thing we can do is just add a new interface:
type ContextDialer interface { DialContext(...) }And document that we use that interface if available, and all our implementations return a Dialer but it also implements ContextDialer.
All the options suck here.
Or we just break API compatibility, but I don't know who uses this.
To view, visit change 37641. To unsubscribe, visit settings.
Martin Garton (Gerrit)
Martin Garton uploaded patch set #2 to this change.
proxy : Added DialContext to Dialer interface Added DialContext function to allow dialing a socks5 connection with a context, similar to net.DialContext in the standard library. Unfortunately this breaks backwards compatibility for users who have implemented proxy.Dialer. There appears to be no reasonable way to avoid this. Updates golang/go#19354 Change-Id: I0ae42b0cd72fa1cb3183f0279e8c1e31c8297521 --- M proxy/direct.go M proxy/per_host.go M proxy/per_host_test.go M proxy/proxy.go M proxy/proxy_test.go M proxy/socks5.go 6 files changed, 77 insertions(+), 2 deletions(-)
"
import (
+ "context"
"errors"
"net"
"net/url"
@@ -17,6 +18,8 @@
type Dialer interface {
// Dial connects to the given address via the proxy.
Dial(network, addr string) (c net.Conn, err error)
+ // Dial connects to the given address via the proxy using the provided context.
+ DialContext(ctx context.Context, network, addr string) (c net.Conn, err error)
}
// Auth contains authentication parameters that specific Dialers may require.
diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go
index c19a5c0..db305cc 100644
--- a/proxy/proxy_test.go
+++ b/proxy/proxy_test.go
@@ -5,12 +5,14 @@
package proxy
import (
+ "context"
"io"
"net"
"net/url"
"strconv"
"sync"
"testing"
+ "time"
)
func TestFromURL(t *testing.T) {
@@ -79,6 +81,30 @@
wg.Wait()
}
+func TestSOCKS5DialContextTimeout(t *testing.T) {
+ gateway, err := net.Listen("tcp", "127.0.0.1:0")
+ if err != nil {
+ t.Fatalf("net.Listen failed: %v", err)
+ }
+ defer gateway.Close()
+ proxy, err := SOCKS5("tcp", gateway.Addr().String(), nil, Direct)
+ if err != nil {
+ t.Fatalf("SOCKS5 failed: %v", err)
+ }
+ ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
+ defer cancel()
+
+ c, err := proxy.DialContext(ctx, "tcp", "dummy:1234")
+ if err == nil {
+ c.Close()
+ t.Fatal("context should have timed o")
+ } else {
+ if err != ctx.Err() {
+ t.Fatalf("Expected error '%v' but got '%v'", ctx.Err, err)
+ }
+ }
+}
+
func socks5Gateway(t *testing.T, gateway, endSystem net.Listener, typ byte, wg *sync.WaitGroup) {
defer wg.Done()
diff --git a/proxy/socks5.go b/proxy/socks5.go
index 973f57f..d999ee6 100644
--- a/proxy/socks5.go
+++ b/proxy/socks5.go
@@ -5,10 +5,12 @@
package proxy
import (
+ "context"
"errors"
"io"
"net"
"strconv"
+ "time"
)
// SOCKS5 returns a Dialer that makes SOCKSv5 connections to the given address
@@ -48,6 +50,10 @@
socks5IP6 = 4
)
+var (
+ inThePast = time.Unix(1483516789, 0)
+)
+
var socks5Errors = []string{
"",
"general failure",
@@ -62,23 +68,43 @@
// Dial connects to the address addr on the network net via the SOCKS5 proxy.
func (s *socks5) Dial(network, addr string) (net.Conn, error) {
+ return s.DialContext(context.Background(), network, addr)
+}
+
+// Dial connects to the address addr on the network net via the SOCKS5 proxy using the provided Context.
+func (s *socks5) DialContext(ctx context.Context, network, addr string) (net.Conn, error) {
switch network {
case "tcp", "tcp6", "tcp4":
default:
return nil, errors.New("proxy: no support for SOCKS5 proxy connections of type " + network)
}
- conn, err := s.forward.Dial(s.network, s.addr)
+ conn, err := s.forward.DialContext(ctx, s.network, s.addr)
if err != nil {
return nil, err
}
- if err := s.connect(conn, addr); err != nil {
+ if err := s.connectCtx(ctx, conn, addr); err != nil {
conn.Close()
return nil, err
}
return conn, nil
}
+func (s socks5) connectCtx(ctx context.Context, conn net.Conn, target string) error {
+ connectErr := make(chan error)
+ go func() {
+ connectErr <- s.connect(conn, target)
+ }()
+ select {
+ case <-ctx.Done():
+ conn.SetDeadline(inThePast)
+ <-connectErr
+ return ctx.Err()
+ case err := <-connectErr:
+ return err
+ }
+}
+
// connect takes an existing connection to a socks5 proxy server,
// and commands the server to extend that connection to target,
// which must be a canonical address with a host and port.
To view, visit change 37641. To unsubscribe, visit settings.
Mikio Hara (Gerrit)
Mikio Hara posted comments on this change.
Patch set 2:
I'm not sure why not adding a new type that satisfies the existing proxy.Dialer interface and has DialContext and Dial methods. You can use the new type with type assertion without breaking the existing applications. For example,
type SOCKS5Dialer { ... } func (d *SOCKS5Dialer) Dial( ... ) (net.Conn, error) func (d *SOCKS5Dialer) DialContext( ... ) (net.Conn, error)
when someone wants HAProxy proxy protocol v1/2 clients,
type HAProxyDialer { ...} func (d *HAProxyDialer) Dial( ... ) (net.Conn, error) func (d *HAProxyDialer) DialContext( ... ) (net.Conn, error)
and more like TLSProxyDialer, QUICProxyDialer, GUEProxyDialer.
Well, perhaps having x/net/socks package might be an option if the net/http package really requires SOCKS5 client-side implementation.
Martin Garton (Gerrit)
Martin Garton posted comments on this change.
Patch set 2:
Patch Set 2:
I'm not sure why not adding a new type that satisfies the existing proxy.Dialer interface and has DialContext and Dial methods. You can use the new type with type assertion without breaking the existing applications. For example,
What would the final parameter of would SOCKS5Dialer.DialContext()?
In the non-context case, it's Dialer but in the context case it would need to be context capable because this package chains Dialers. This means that by using a different interface, we'd end up duplicating a lot more.
Well, perhaps having x/net/socks package might be an option if the net/http package really requires SOCKS5 client-side implementation.
Do you mean creating a simpler socks5 package that perhaps doesn't support Dialer chaining?
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick posted comments on this change.
Patch set 2:
If we're going to break the API on people, then we should clean it up to Go best practices at the same time.
And Go best practices is that interfaces should be both small and only describe what is needed (consumed by the application), not what is implemented by the package.
So we should only require users give us a Dialer that can Dial with a context and not have both methods in the interface.
That also means we should not return interface types from functions and should return concrete types instead.
Mikio Hara (Gerrit)
Mikio Hara posted comments on this change.
Patch set 2:
What would the final parameter of would SOCKS5Dialer.DialContext()?
Just sent https://go-review.googlesource.com/c/38278/ for discussion.
Do you mean creating a simpler socks5 package that perhaps doesn't support Dialer chaining?
I'm not sure what do you mean by "Dialer chaining", but if it means stuff that stacks various protocol entities up for dialing, proxy.Dialer might be insufficient to control each protocol. It might be better to use concrete types instead.
Martin Garton (Gerrit)
Martin Garton posted comments on this change.
Patch set 2:
Patch Set 2:
What would the final parameter of would SOCKS5Dialer.DialContext()?
Just sent https://go-review.googlesource.com/c/38278/ for discussion.
Do you mean creating a simpler socks5 package that perhaps doesn't support Dialer chaining?
I'm not sure what do you mean by "Dialer chaining", but if it means stuff that stacks various protocol entities up for dialing, proxy.Dialer might be insufficient to control each protocol. It might be better to use concrete types instead.
By "chaining", I mean that the proxy package constructor functions (such as proxy.SOCKS5) take a proxy.Dialer as well as returning a proxy.Dialer
This is a useful feature of the package worth preserving IMO.
In your CL I notice that you have omitted this in favour of using a net.Dialer. That might suit some needs, but proxy.Dialer is more flexible I think, and allows things like proxy.PerHost to exist easily. I'm not sure how that would fit into your implementation?
Martin Garton (Gerrit)
Patch Set 2:
If we're going to break the API on people, then we should clean it up to Go best practices at the same time.
And Go best practices is that interfaces should be both small and only describe what is needed (consumed by the application), not what is implemented by the package.
So we should only require users give us a Dialer that can Dial with a context and not have both methods in the interface.
That also means we should not return interface types from functions and should return concrete types instead.
Happy to give that a go.
We should remember though that the API breakage in the current patch set breaks things for people who have created their own proxy.Dialer implementation (few) but if we only allowed a dialling with a context, a wider group would be affected I think.
Thoughts before I have a go at that?
Mikio Hara (Gerrit)
Mikio Hara posted comments on this change.
Patch set 2:
This is a useful feature of the package worth preserving IMO.
I'm fine to add such stuff into the proxy package if necessary. What I want to see is the separation; I personally don't want to bloat the proxy package with various protocol implementations. The proxy package provides the semantics and minimum required primitive, and each protocol implementation package (like x/net/socks) satisfies the semantics. If the net/http package really requires SOCKS5, it just does vendor x/net/socks, not x/net/proxy.
Martin Garton (Gerrit)
Martin Garton posted comments on this change.
Patch set 2:
Patch Set 2:
This is a useful feature of the package worth preserving IMO.
I'm fine to add such stuff into the proxy package if necessary. What I want to see is the separation; I personally don't want to bloat the proxy package with various protocol implementations. The proxy package provides the semantics and minimum required primitive, and each protocol implementation package (like x/net/socks) satisfies the semantics. If the net/http package really requires SOCKS5, it just does vendor x/net/socks, not x/net/proxy.
Makes total sense to me. Should that happen in a bunch of different CLs though? This one (or at least my original goal when submitting) was allowing the use of Context for the proxy package (not just for the socks5 part). I realise there is another goal of suitably vendoring what's needed for the purposes of net/http.
Mikio Hara (Gerrit)
Mikio Hara posted comments on this change.
Patch set 2:
Should that happen in a bunch of different CLs though?
Yes. This change or issue 19354 should focus on the package proxy API surface which provides the semantics of forward proxy connection setup. The API should not break the existing applications and annoy future applications.
Martin Garton (Gerrit)
Martin Garton uploaded patch set #3 to this change.
proxy : Added DialContext to Dialer interface
Added DialContext function to allow dialing a socks5 connection
with a context, similar to net.DialContext in the standard library.
Unfortunately this breaks backwards compatibility for users who have
implemented proxy.Dialer. There appears to be no reasonable way to
avoid this.
Updates golang/go#19354
Change-Id: I0ae42b0cd72fa1cb3183f0279e8c1e31c8297521
---
M proxy/direct.go
M proxy/per_host.go
M proxy/per_host_test.go
M proxy/proxy.go
M proxy/proxy_test.go
M proxy/socks5.go
6 files changed, 52 insertions(+), 2 deletions(-)
To view, visit change 37641. To unsubscribe, or for help writing mail filters, visit settings.
Martin Garton (Gerrit)
Martin Garton uploaded patch set #4 to this change.
proxy : Added DialContext to Dialer interface
Added DialContext function to allow dialing a socks5 connection
with a context, similar to net.DialContext in the standard library.
Unfortunately this breaks backwards compatibility for users who have
implemented proxy.Dialer. There appears to be no reasonable way to
avoid this.
Updates golang/go#19354
Change-Id: I0ae42b0cd72fa1cb3183f0279e8c1e31c8297521
---
M proxy/direct.go
M proxy/per_host.go
M proxy/per_host_test.go
M proxy/proxy.go
M proxy/socks5.go
5 files changed, 25 insertions(+), 2 deletions(-)
To view, visit change 37641. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
2 comments:
Patch Set #4, Line 7:
proxy : Added DialContext to Dialer interface
// A Dialer is a means to establish a connection.
type Dialer interface {
// Dial connects to the given address via the proxy.
Dial(network, addr string) (c net.Conn, err error)
// Dial connects to the given address via the proxy using the provided context.
DialContext(ctx context.Context, network, addr string) (c net.Conn, err error)
}
We can't expand interfaces. And this is the wrong use of an interface, too.
Interfaces are for describing things that are consumed. They don't describe things that are provided.
To view, visit change 37641. To unsubscribe, or for help writing mail filters, visit settings.
Martin Garton (Gerrit)
If we can't expand the interface, I can't see a good way to do this because of things like Dialer chaining and the proxy.FromURL function that returns Dialer
As you said yourself some time ago, "All the options suck here."
If there are any completely different alternative suggestions anyone has that might fix these issues in other ways, I'd be happy to have a go.
Failing that, maybe forking this and maintaining something elsewhere might be the answer?
To view, visit change 37641. To unsubscribe, or for help writing mail filters, visit settings.
Mikio Hara (Gerrit)
the existing api surface came from some experiment at the x/exp repository. probably it's time to revisit proxy api surface because there are certain needs: https://github.com/golang/go/issues?utf8=%E2%9C%93&q=is%3Aissue+proxy+in%3Atitle+is%3Aopen+proxy+in%3Atitle
i think it's better not to break the existing api surface even though the stuff in the x/net repository is free from the go1compat promise because https://godoc.org/golang.org/x/net/proxy shows there are >200 packages relying on the existing api surface.
so, my suggestion would be a) taking a look at the existing issues, b) designing new api surface, c) then pushing a cl implementing the new api surface to review process.
To view, visit change 37641. To unsubscribe, or for help writing mail filters, visit settings.
Leon Klingele (Gerrit)
1 comment:
// A Dialer is a means to establish a connection.
type Dialer interface {
// Dial connects to the given address via the proxy.
Dial(network, addr string) (c net.Conn, err error)
// Dial connects to the given address via the proxy using the provided context.
DialContext(ctx context.Context, network, addr string) (c net.Conn, err error)
}
We can't expand interfaces. And this is the wrong use of an interface, too. […]
Hey Brad, what do you suggest instead? Would be great to get this merged.
To view, visit change 37641. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Sorry, but redesigning this isn't my priority at the moment.
But this as-is doesn't work. If you have time, you can work on a new design. It'll probably involve concrete types for dialers. Returning interface types was a mistake.
Patch set 4:Code-Review -2
To view, visit change 37641. To unsubscribe, or for help writing mail filters, visit settings.