Support proxy with custom dialer #1098

menghanl · 2017-03-01T18:49:42Z

Add a function to create custom dialer to do dial and HTTP CONNECT handshake.

dfawley · 2017-03-06T20:56:18Z

proxy/proxy.go

+	// MapAddress is called before we connect to the target address.
+	// It can be used to programmatically override the address that we will connect to.
+	// It returns the address of the proxy, and the header to be sent in the request.
+	MapAddress(ctx context.Context, address string) (string, map[string][]string, error)


Can the return value be an http.Header instead of a map?

I'm trying to avoid introduce http as a dependency of the exported APIs.
I'm using http.Header in the implementation though...
I can change this if you think the dependency is OK.

dfawley · 2017-03-06T21:05:35Z

proxy/proxy.go

+// as indicated by the environment variables HTTP_PROXY, HTTPS_PROXY and NO_PROXY
+// (or the lowercase versions thereof) when mapping address, and does HTTP connect handshake on
+// the connection.
+func NewEnvironmentVariableHTTPConnectProxy() Proxyer {


Will this be used by default? If not, would this be confusing for users, since basically everything respects these environment variables?

gRPC should check those env variables by default. And this will be the implementation of the default behavior in gRPC.

dfawley · 2017-03-06T22:57:39Z

proxy/proxy.go

+var ErrIneffective = errors.New("MapAddress function is not effective")
+
+// Proxyer defines the interface gRPC uses to map the proxy address.
+type Proxyer interface {


I would prefer "Proxy" (even though it stutters), or "Interface". There are examples of both in the go standard libraries (hash.Hash; sort.Interface and heap.Interface).

The change containing this type was reverted

dfawley · 2017-03-06T23:00:05Z

proxy/proxy.go

+ *
+ */
+
+// Package proxy defines interfaces to support proxyies in gRPC.


... and an implementation of a proxy that uses environment variables for configuration.

(e.g.)

dfawley · 2017-03-06T23:01:30Z

proxy/proxy.go

+)
+
+// ErrIneffective indicates the mapper function is not effective.
+var ErrIneffective = errors.New("MapAddress function is not effective")


Optional: ErrDisabled? Indicates the proxy is not enabled?

dfawley · 2017-03-06T23:05:30Z

proxy/proxy.go

+	return doHTTPConnectHandshake(ctx, conn, addr, header)
+}
+
+type bufConn struct {


Please add a comment explaining why this type exists.

dfawley · 2017-03-06T23:12:18Z

proxy/proxy.go

+}
+
+func (pm *environmentProxyMapper) Handshake(ctx context.Context, conn net.Conn, addr string, header http.Header) (net.Conn, error) {
+	return doHTTPConnectHandshake(ctx, conn, addr, header)


It looks like it should be possible to share additional code (header formulation, bufConn creation) and limit the version-specific code to just sending the request. For maintenance, in general, it would be better to minimize version-specific code.

Done.
Was thinking that we will eventually get rid of the specific code for old versions, and to git rid of that, we can simply delete those files ;)

dfawley · 2017-03-06T23:14:42Z

proxy/util_pre_go16.go

+	r := bufio.NewReader(conn)
+	resp, err := http.ReadResponse(r, nil)
+	if err != nil {
+		return conn, fmt.Errorf("reading server HTTP response: %v", err)


Return nil, ?

Disregard if this becomes shared code per earlier comment.

dfawley · 2017-03-06T23:19:17Z

test/end2end_test.go

+		t.Fatalf("%v.CloseSend() got %v, want %v", stream, err, nil)
+	}
+	if _, err := stream.Recv(); err != io.EOF {
+		t.Fatalf("%v failed to complele the FullDuplexCall: %v", stream, err)


complele -> complete

dfawley · 2017-03-06T23:36:35Z

transport/http2_client.go

+		newAddr = addr
+	}
+
+	conn, err := fn(ctx, newAddr)


What if the proxyer was responsible for connecting? In that case, the proxyer would be mostly a wrapper, and the logic around here could be completely offloaded to it. I.e.:

package proxy type Dialer interface { Dial(ctx context.Context, addr string, dialFunc func(context.Context, string) (net.Conn, error)) (net.Conn, error) }

A proxy's Dial method would dial, determine the headers and new address if necessary, and perform whatever handshaking is required. If the proxy was disabled/ineffective, it would be responsible for doing simply: "return dialFunc(ctx, addr)". This avoids the need to pass around the headers / new address / disabled error, and should allow more flexibility in what Proxies could implement.

Refactored as discussed

markdroth

I'm not very fluent in Go yet, but the overall approach here looks fine for now.

I do think that we need to have some follow-up discussions about how the name resolution and load balancing code is structured in Go relative to the other languages, and that might affect some of this code. But that issue also affects a lot of existing code as well, so it doesn't need to block merging this PR.

Anyway, I'd say that once someone who is more fluent in Go has reviewed this, it should be fine to submit.

Thanks for doing this!

menghanl · 2017-04-07T00:35:08Z

ping

ejona86 · 2017-04-07T00:48:53Z

transport/proxy.go

+	req := (&http.Request{
+		Method: http.MethodConnect,
+		URL:    &url.URL{Host: addr},
+		Header: map[string][]string{"User-Agent": {"gRPC-go"}},


Is this missing the Host header, or will that be copied from the URL automatically by go?

Yes, if will be copied from the URL.

ejona86 · 2017-04-07T00:51:11Z

transport/proxy.go

+	}
+	resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		return nil, fmt.Errorf("failed to do connect handshake, status code: %s", resp.Status)


I'd highly suggest including 1k or so of the response body (or have a way to log it). It helps a ton when debugging failures.

This reverts commit 99fa459.

sathieu · 2017-10-18T09:30:23Z

Is there a dumb dialer which doesn't use proxy envs? (for moby/moby#34825). Thanks

menghanl · 2017-10-18T17:29:55Z

You can override the dialer with WithDialer().

grpc.WithDialer(func(addr string, timeout time.Duration) (net.Conn, error) {
  return net.DialTimeout("tcp", addr, timeout)
})

menghanl force-pushed the connect_handshaker branch 4 times, most recently from ca88a5e to eccdd15 Compare March 1, 2017 22:36

menghanl mentioned this pull request Mar 1, 2017

Support proxy #1095

Closed

menghanl requested a review from markdroth March 1, 2017 23:23

menghanl force-pushed the connect_handshaker branch from 356db94 to 1394ad5 Compare March 2, 2017 01:57

menghanl requested review from ejona86, MakMukhi and dfawley March 6, 2017 20:01

dfawley reviewed Mar 6, 2017

View reviewed changes

menghanl force-pushed the connect_handshaker branch 4 times, most recently from fc67b61 to a9c135c Compare March 8, 2017 19:34

menghanl added the 1.1 Release label Mar 13, 2017

markdroth approved these changes Mar 13, 2017

View reviewed changes

menghanl force-pushed the connect_handshaker branch 3 times, most recently from 23885fe to 68f5e21 Compare March 14, 2017 00:31

dfawley added 1.3 and removed 1.2 labels Mar 20, 2017

menghanl force-pushed the connect_handshaker branch 2 times, most recently from 7d6c251 to 7976d92 Compare March 27, 2017 21:47

menghanl force-pushed the connect_handshaker branch from ed19663 to b61bb39 Compare March 28, 2017 18:12

menghanl force-pushed the connect_handshaker branch from b61bb39 to 802045b Compare April 5, 2017 18:13

ejona86 approved these changes Apr 7, 2017

View reviewed changes

menghanl added 21 commits April 17, 2017 14:01

add NewEnvironmentProxyMapper to proxy

771ab1f

change default behavior on proxy

7a6d0ea

Revert "change default behavior on proxy"

2e3b9ed

This reverts commit 99fa459.

install default proxy if no dialer is specified

45ef7fa

fix comments

d8ff8c7

fix test

dc06062

move to transport package, working

9432c0f

move tests to proxy_test

f7449b5

change signature of dialer in newProxyDialer

0d90a8d

fix failing tests

e3f8eb6

use const instead of string directly

0225b23

modify tests

a71e969

fix import error

3936a0d

remove useless code from race_test.go

094008f

print body of response when failed to do connect

a100181

defer body.close()

a36fcb5

move proxy and dialer related code to grpc package

2e13fa1

use grpcUA in proxy

0ff8a9e

fix ua in proxy test

91e3e4b

keep check dial!=nil in http2_client

65e0982

const and date

b20836f

menghanl force-pushed the connect_handshaker branch from 9d7c174 to b20836f Compare April 17, 2017 21:02

dfawley approved these changes Apr 17, 2017

View reviewed changes

menghanl merged commit 955c867 into grpc:master Apr 17, 2017

menghanl deleted the connect_handshaker branch April 17, 2017 23:08

menghanl added the Type: Feature New features or improvements in behavior label Apr 22, 2017

thaJeztah mentioned this pull request Nov 3, 2017

Swarm worker cannot connect to master if proxy is configured moby/moby#35395

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support proxy with custom dialer #1098

Support proxy with custom dialer #1098

menghanl commented Mar 1, 2017 •

edited

Loading

dfawley Mar 6, 2017

menghanl Mar 8, 2017

dfawley Mar 6, 2017

menghanl Mar 8, 2017

dfawley Mar 6, 2017

menghanl Mar 8, 2017

dfawley Mar 6, 2017

menghanl Mar 8, 2017

dfawley Mar 6, 2017

menghanl Mar 8, 2017

dfawley Mar 6, 2017

menghanl Mar 8, 2017

dfawley Mar 6, 2017

menghanl Mar 8, 2017

dfawley Mar 6, 2017

menghanl Mar 8, 2017

dfawley Mar 6, 2017

menghanl Mar 8, 2017

dfawley Mar 6, 2017

menghanl Mar 8, 2017

markdroth left a comment

menghanl commented Apr 7, 2017

ejona86 Apr 7, 2017

menghanl Apr 7, 2017

ejona86 Apr 7, 2017

menghanl Apr 7, 2017

sathieu commented Oct 18, 2017

menghanl commented Oct 18, 2017

Support proxy with custom dialer #1098

Support proxy with custom dialer #1098

Conversation

menghanl commented Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

menghanl commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sathieu commented Oct 18, 2017

menghanl commented Oct 18, 2017

menghanl commented Mar 1, 2017 •

edited

Loading