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

transport,grpc: Integrate delegating resolver and introduce dial options for target host resolution #7881

Open
wants to merge 55 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 44 commits
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
619dcd4
Change proxy behaviour
eshitachandwani Dec 3, 2024
65b33bd
change delegating resolver
eshitachandwani Dec 4, 2024
94364e2
Improving code
eshitachandwani Dec 5, 2024
9f6a067
correct import
eshitachandwani Dec 5, 2024
5ce86d0
improve tests
eshitachandwani Dec 5, 2024
fbef3a4
address comments
eshitachandwani Dec 5, 2024
342c332
add warning and httpfunc test
eshitachandwani Dec 5, 2024
8726188
add warning and httpfunc test
eshitachandwani Dec 5, 2024
1cfb089
from delegating_pr
eshitachandwani Dec 7, 2024
32d9de5
improve code
eshitachandwani Dec 8, 2024
34b902f
improve code
eshitachandwani Dec 8, 2024
11c8fb1
improve
eshitachandwani Dec 9, 2024
b4c9980
correct tests
eshitachandwani Dec 18, 2024
93fc3e1
trying tests
eshitachandwani Dec 18, 2024
e22ad1d
tests
eshitachandwani Dec 19, 2024
b515565
test
eshitachandwani Dec 19, 2024
7ae2797
correct test environment
eshitachandwani Dec 20, 2024
5ce4658
Correct pr
eshitachandwani Dec 23, 2024
a292b63
Merge branch 'master' into proxy_pr2
eshitachandwani Dec 23, 2024
4483e95
rebase
eshitachandwani Dec 23, 2024
d4f6215
comment
eshitachandwani Dec 23, 2024
333a68c
proxy testutils refactor
eshitachandwani Dec 23, 2024
cacf058
correct vet.sh
eshitachandwani Dec 23, 2024
7c5b1b3
correct vet
eshitachandwani Dec 23, 2024
cb6b09c
Merge branch 'grpc:master' into proxy_pr2
eshitachandwani Dec 27, 2024
ff05ee3
something
eshitachandwani Dec 27, 2024
104cd18
working tests manual resolver
eshitachandwani Dec 27, 2024
fb23cca
working tests manual resolver
eshitachandwani Dec 27, 2024
ef927ce
test
eshitachandwani Dec 27, 2024
bc5efc3
trying test without manual resolver
eshitachandwani Jan 2, 2025
876f09e
e2e tests
eshitachandwani Jan 3, 2025
3e20010
correct vet
eshitachandwani Jan 3, 2025
6827a62
correct e2e tests
eshitachandwani Jan 3, 2025
9acd8de
correct vet
eshitachandwani Jan 3, 2025
8f5055e
vet
eshitachandwani Jan 3, 2025
bbd7f02
vet
eshitachandwani Jan 3, 2025
128da2c
addresses comments
eshitachandwani Jan 7, 2025
7cb9ec8
add package comment
eshitachandwani Jan 7, 2025
0d055b3
correct timeout
eshitachandwani Jan 7, 2025
07b9344
format
eshitachandwani Jan 8, 2025
f75662d
address comments
eshitachandwani Jan 9, 2025
2df2f2e
new test for env variable
eshitachandwani Jan 10, 2025
9e52674
test
eshitachandwani Jan 10, 2025
7d230bc
remove mocking and check enviornment
eshitachandwani Jan 10, 2025
27cc055
comment for httpproxy
eshitachandwani Jan 10, 2025
227c748
address comments
eshitachandwani Jan 14, 2025
bdbe501
remove helper
eshitachandwani Jan 15, 2025
8a43a91
proxy helper function
eshitachandwani Jan 15, 2025
a73d511
proxy helper function
eshitachandwani Jan 15, 2025
0c154cf
proxy helper function
eshitachandwani Jan 15, 2025
d396d31
change proxy_utils
eshitachandwani Jan 16, 2025
fd088d1
change proxy_utils
eshitachandwani Jan 16, 2025
32fa83c
change proxy_utils
eshitachandwani Jan 16, 2025
382bcb1
addr comments
eshitachandwani Jan 16, 2025
69037b0
add userSet option
eshitachandwani Jan 17, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion clientconn.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,12 @@ func Dial(target string, opts ...DialOption) (*ClientConn, error) {
func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *ClientConn, err error) {
// At the end of this method, we kick the channel out of idle, rather than
// waiting for the first rpc.
opts = append([]DialOption{withDefaultScheme("passthrough")}, opts...)
//
// WithTargetResolutionEnabled dial option in `grpc.Dial` ensures that it
// preserves behavior: when default scheme passthrough is used, skip
// hostname resolution, when "dns" is used for resolution, perform
// resolution on the client.
opts = append([]DialOption{withDefaultScheme("passthrough"), WithTargetResolutionEnabled()}, opts...)
arjan-bal marked this conversation as resolved.
Show resolved Hide resolved
arjan-bal marked this conversation as resolved.
Show resolved Hide resolved
cc, err := NewClient(target, opts...)
if err != nil {
return nil, err
Expand Down Expand Up @@ -1322,6 +1327,7 @@ func (ac *addrConn) tryAllAddrs(ctx context.Context, addrs []resolver.Address, c
if err == nil {
return nil
}

dfawley marked this conversation as resolved.
Show resolved Hide resolved
if firstConnErr == nil {
firstConnErr = err
}
Expand Down
29 changes: 23 additions & 6 deletions dialoptions.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ type dialOptions struct {
idleTimeout time.Duration
defaultScheme string
maxCallAttempts int
targetResolutionEnabled bool // Specifies if target hostnames should be resolved when proxying is enabled.
useProxy bool // Specifies if a server should be connected via proxy.
}

// DialOption configures how we set up the connection.
Expand Down Expand Up @@ -377,7 +379,21 @@ func WithInsecure() DialOption {
// later release.
func WithNoProxy() DialOption {
return newFuncDialOption(func(o *dialOptions) {
o.copts.UseProxy = false
arjan-bal marked this conversation as resolved.
Show resolved Hide resolved
o.useProxy = false
})
}

// WithTargetResolutionEnabled returns a DialOption which enables target
// resolution on client when a proxy is used along with the the "dns" scheme.
// This is ignored if WithNoProxy is used.
//
// # Experimental
//
// Notice: This API is EXPERIMENTAL and may be changed or removed in a
// later release.
func WithTargetResolutionEnabled() DialOption {
arjan-bal marked this conversation as resolved.
Show resolved Hide resolved
dfawley marked this conversation as resolved.
Show resolved Hide resolved
return newFuncDialOption(func(o *dialOptions) {
o.targetResolutionEnabled = true
})
}

Expand Down Expand Up @@ -667,14 +683,15 @@ func defaultDialOptions() dialOptions {
copts: transport.ConnectOptions{
ReadBufferSize: defaultReadBufSize,
WriteBufferSize: defaultWriteBufSize,
UseProxy: true,
UserAgent: grpcUA,
BufferPool: mem.DefaultBufferPool(),
},
bs: internalbackoff.DefaultExponential,
idleTimeout: 30 * time.Minute,
defaultScheme: "dns",
maxCallAttempts: defaultMaxCallAttempts,
bs: internalbackoff.DefaultExponential,
idleTimeout: 30 * time.Minute,
defaultScheme: "dns",
maxCallAttempts: defaultMaxCallAttempts,
useProxy: true,
targetResolutionEnabled: false,
}
}

Expand Down
12 changes: 7 additions & 5 deletions internal/transport/http2_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import (
"google.golang.org/grpc/internal/grpcsync"
"google.golang.org/grpc/internal/grpcutil"
imetadata "google.golang.org/grpc/internal/metadata"
"google.golang.org/grpc/internal/proxyattributes"
istatus "google.golang.org/grpc/internal/status"
isyscall "google.golang.org/grpc/internal/syscall"
"google.golang.org/grpc/internal/transport/networktype"
Expand Down Expand Up @@ -153,8 +154,12 @@ type http2Client struct {
logger *grpclog.PrefixLogger
}

func dial(ctx context.Context, fn func(context.Context, string) (net.Conn, error), addr resolver.Address, useProxy bool, grpcUA string) (net.Conn, error) {
func dial(ctx context.Context, fn func(context.Context, string) (net.Conn, error), addr resolver.Address, grpcUA string) (net.Conn, error) {
address := addr.Addr

if opts, present := proxyattributes.Get(addr); present {
return proxyDial(ctx, addr, grpcUA, opts)
}
dfawley marked this conversation as resolved.
Show resolved Hide resolved
networkType, ok := networktype.Get(addr)
if fn != nil {
// Special handling for unix scheme with custom dialer. Back in the day,
Expand All @@ -177,9 +182,6 @@ func dial(ctx context.Context, fn func(context.Context, string) (net.Conn, error
if !ok {
networkType, address = parseDialTarget(address)
}
if networkType == "tcp" && useProxy {
return proxyDial(ctx, address, grpcUA)
}
return internal.NetDialerWithTCPKeepalive().DialContext(ctx, networkType, address)
}

Expand Down Expand Up @@ -217,7 +219,7 @@ func NewHTTP2Client(connectCtx, ctx context.Context, addr resolver.Address, opts
// address specific arbitrary data to reach custom dialers and credential handshakers.
connectCtx = icredentials.NewClientHandshakeInfoContext(connectCtx, credentials.ClientHandshakeInfo{Attributes: addr.Attributes})

conn, err := dial(connectCtx, opts.Dialer, addr, opts.UseProxy, opts.UserAgent)
conn, err := dial(connectCtx, opts.Dialer, addr, opts.UserAgent)
if err != nil {
if opts.FailOnNonTempDialError {
return nil, connectionErrorf(isTemporary(err), err, "transport: error while dialing: %v", err)
Expand Down
63 changes: 15 additions & 48 deletions internal/transport/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,34 +30,16 @@ import (
"net/url"

"google.golang.org/grpc/internal"
"google.golang.org/grpc/internal/proxyattributes"
"google.golang.org/grpc/resolver"
)

const proxyAuthHeaderKey = "Proxy-Authorization"

var (
// The following variable will be overwritten in the tests.
httpProxyFromEnvironment = http.ProxyFromEnvironment
)

func mapAddress(address string) (*url.URL, error) {
req := &http.Request{
URL: &url.URL{
Scheme: "https",
Host: address,
},
}
url, err := httpProxyFromEnvironment(req)
if err != nil {
return nil, err
}
return url, nil
}

// To read a response from a net.Conn, http.ReadResponse() takes a bufio.Reader.
// It's possible that this reader reads more than what's need for the response and stores
// those bytes in the buffer.
// bufConn wraps the original net.Conn and the bufio.Reader to make sure we don't lose the
// bytes in the buffer.
// It's possible that this reader reads more than what's need for the response
// and stores those bytes in the buffer. bufConn wraps the original net.Conn
// and the bufio.Reader to make sure we don't lose the bytes in the buffer.
type bufConn struct {
net.Conn
r io.Reader
Expand All @@ -72,7 +54,7 @@ func basicAuth(username, password string) string {
return base64.StdEncoding.EncodeToString([]byte(auth))
}

func doHTTPConnectHandshake(ctx context.Context, conn net.Conn, backendAddr string, proxyURL *url.URL, grpcUA string) (_ net.Conn, err error) {
func doHTTPConnectHandshake(ctx context.Context, conn net.Conn, grpcUA string, opts proxyattributes.Options) (_ net.Conn, err error) {
defer func() {
if err != nil {
conn.Close()
Expand All @@ -81,15 +63,15 @@ func doHTTPConnectHandshake(ctx context.Context, conn net.Conn, backendAddr stri

req := &http.Request{
Method: http.MethodConnect,
URL: &url.URL{Host: backendAddr},
URL: &url.URL{Host: opts.ConnectAddr},
Header: map[string][]string{"User-Agent": {grpcUA}},
}
if t := proxyURL.User; t != nil {
u := t.Username()
p, _ := t.Password()

if user := opts.User; user != (url.Userinfo{}) {
Copy link
Member

Choose a reason for hiding this comment

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

This comparison with a zero-initialized Userinfo looks a little off to me. Should this field in opts be a pointer instead to determine whether it was set intentionally?

Or should it check if either Username() or Password() are non-empty?

I'm not sure exactly what the right behavior is, but this seems questionable, as it's including any unexported fields in the comparison.

Related but potential existing problem to investigate: what if username is set but password is not? Is that valid? If so, is the proper behavior to encode it as username:, as it does currently, or should the : be omitted?

Copy link
Member Author

Choose a reason for hiding this comment

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

The RFC doesn't clearly mention what is the correct syntax if password is absent , so I think either way should be fine, but adding a semicolon like username: would be more explicit in indication username has ended and the password is empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added a UserSet option as changing it to a pointer can allow the caller to modify the use info.

u := user.Username()
p, _ := user.Password()
dfawley marked this conversation as resolved.
Show resolved Hide resolved
req.Header.Add(proxyAuthHeaderKey, "Basic "+basicAuth(u, p))
}

if err := sendHTTPRequest(ctx, req, conn); err != nil {
return nil, fmt.Errorf("failed to write the HTTP request: %v", err)
}
Expand Down Expand Up @@ -117,28 +99,13 @@ func doHTTPConnectHandshake(ctx context.Context, conn net.Conn, backendAddr stri
return conn, nil
}

// proxyDial dials, connecting to a proxy first if necessary. Checks if a proxy
// is necessary, dials, does the HTTP CONNECT handshake, and returns the
// connection.
func proxyDial(ctx context.Context, addr string, grpcUA string) (net.Conn, error) {
newAddr := addr
proxyURL, err := mapAddress(addr)
if err != nil {
return nil, err
}
if proxyURL != nil {
newAddr = proxyURL.Host
}

conn, err := internal.NetDialerWithTCPKeepalive().DialContext(ctx, "tcp", newAddr)
// proxyDial establishes a TCP connection to the specified address and performs an HTTP CONNECT handshake.
func proxyDial(ctx context.Context, addr resolver.Address, grpcUA string, opts proxyattributes.Options) (net.Conn, error) {
conn, err := internal.NetDialerWithTCPKeepalive().DialContext(ctx, "tcp", addr.Addr)
if err != nil {
return nil, err
}
if proxyURL == nil {
// proxy is disabled if proxyURL is nil.
return conn, err
}
return doHTTPConnectHandshake(ctx, conn, addr, proxyURL, grpcUA)
return doHTTPConnectHandshake(ctx, conn, grpcUA, opts)
}

func sendHTTPRequest(ctx context.Context, req *http.Request, conn net.Conn) error {
Expand Down
Loading
Loading