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 8 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
5 changes: 2 additions & 3 deletions clientconn.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,11 +226,11 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *
// At the end of this method, we kick the channel out of idle, rather than
// waiting for the first rpc.
//
// WithTargetResolutionEnabled dial option in `grpc.Dial` ensures that it
// WithLocalDNSResolution 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...)
opts = append([]DialOption{withDefaultScheme("passthrough"), WithLocalDNSResolution()}, opts...)
cc, err := NewClient(target, opts...)
if err != nil {
return nil, err
Expand Down Expand Up @@ -1327,7 +1327,6 @@ func (ac *addrConn) tryAllAddrs(ctx context.Context, addrs []resolver.Address, c
if err == nil {
return nil
}

if firstConnErr == nil {
firstConnErr = err
}
Expand Down
25 changes: 13 additions & 12 deletions dialoptions.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ type dialOptions struct {
idleTimeout time.Duration
defaultScheme string
maxCallAttempts int
targetResolutionEnabled bool // Specifies if target hostnames should be resolved when proxying is enabled.
enableLocalDNSResolution bool // Specifies if target hostnames should be resolved when proxying is enabled.
useProxy bool // Specifies if a server should be connected via proxy.
}

Expand Down Expand Up @@ -383,17 +383,18 @@ func WithNoProxy() DialOption {
})
}

// 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.
// WithLocalDNSResolution forces local DNS name resolution even when a proxy is
// specified in the environment. By default, the server name is provided
// directly to the proxy as part of the CONNECT handshake. 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 {
func WithLocalDNSResolution() DialOption {
return newFuncDialOption(func(o *dialOptions) {
o.targetResolutionEnabled = true
o.enableLocalDNSResolution = true
})
}

Expand Down Expand Up @@ -686,12 +687,12 @@ func defaultDialOptions() dialOptions {
UserAgent: grpcUA,
BufferPool: mem.DefaultBufferPool(),
},
bs: internalbackoff.DefaultExponential,
idleTimeout: 30 * time.Minute,
defaultScheme: "dns",
maxCallAttempts: defaultMaxCallAttempts,
useProxy: true,
targetResolutionEnabled: false,
bs: internalbackoff.DefaultExponential,
idleTimeout: 30 * time.Minute,
defaultScheme: "dns",
maxCallAttempts: defaultMaxCallAttempts,
useProxy: true,
enableLocalDNSResolution: false,
}
}

Expand Down
arjan-bal marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
*
* Copyright 2024 gRPC authors.
* Copyright 2023 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -16,7 +16,6 @@
*
*/

// Package testutils contains helpers for transport tests.
package testutils
dfawley marked this conversation as resolved.
Show resolved Hide resolved

import (
Expand All @@ -28,12 +27,8 @@
"net/http"
"testing"
"time"

"google.golang.org/grpc/internal/testutils"
)

const defaultTestTimeout = 10 * time.Second

// ProxyServer represents a test proxy server.
dfawley marked this conversation as resolved.
Show resolved Hide resolved
type ProxyServer struct {
lis net.Listener
Expand All @@ -43,6 +38,8 @@
Addr string // Address of the proxy
}

const defaultTestTimeout = 10 * time.Second

// Stop closes the ProxyServer and its connections to client and server.
func (p *ProxyServer) stop() {
p.lis.Close()
Expand All @@ -57,12 +54,12 @@
func (p *ProxyServer) handleRequest(t *testing.T, in net.Conn, waitForServerHello bool) {
req, err := http.ReadRequest(bufio.NewReader(in))
if err != nil {
t.Errorf("failed to read CONNECT req: %v", err)
return
}

Check warning on line 59 in internal/testutils/proxy.go

View check run for this annotation

Codecov / codecov/patch

internal/testutils/proxy.go#L57-L59

Added lines #L57 - L59 were not covered by tests
if req.Method != http.MethodConnect {
t.Errorf("unexpected Method %q, want %q", req.Method, http.MethodConnect)
}

Check warning on line 62 in internal/testutils/proxy.go

View check run for this annotation

Codecov / codecov/patch

internal/testutils/proxy.go#L61-L62

Added lines #L61 - L62 were not covered by tests
p.onRequest(req)

t.Logf("Dialing to %s", req.URL.Host)
Expand All @@ -85,11 +82,11 @@
b := make([]byte, 50)
bytesRead, err := out.Read(b)
if err != nil {
t.Errorf("Got error while reading server hello: %v", err)
in.Close()
out.Close()
return
}

Check warning on line 89 in internal/testutils/proxy.go

View check run for this annotation

Codecov / codecov/patch

internal/testutils/proxy.go#L85-L89

Added lines #L85 - L89 were not covered by tests
buf.Write(b[0:bytesRead])
}
p.in = in
Expand All @@ -104,10 +101,10 @@
// stop it, and returns the proxy's listener and helper channels.
dfawley marked this conversation as resolved.
Show resolved Hide resolved
func HTTPProxy(t *testing.T, reqCheck func(*http.Request), waitForServerHello bool) *ProxyServer {
t.Helper()
pLis, err := testutils.LocalTCPListener()
pLis, err := LocalTCPListener()
if err != nil {
t.Fatalf("failed to listen: %v", err)
}

Check warning on line 107 in internal/testutils/proxy.go

View check run for this annotation

Codecov / codecov/patch

internal/testutils/proxy.go#L106-L107

Added lines #L106 - L107 were not covered by tests

p := &ProxyServer{
lis: pLis,
Expand All @@ -128,6 +125,6 @@
}()
t.Logf("Started proxy at: %q", pLis.Addr())
t.Cleanup(p.stop)
p.Addr = fmt.Sprintf("localhost:%d", testutils.ParsePort(t, pLis.Addr().String()))
p.Addr = fmt.Sprintf("localhost:%d", ParsePort(t, pLis.Addr().String()))
dfawley marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Also why can't this just be pLis.Addr().String()? There should be a comment explaining why it's doing all this extra stuff to seemingly just convert localhost:123 into localhost:123.

Copy link
Member Author

Choose a reason for hiding this comment

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

pLis..Addr() will be a IP address, we convert it to localhost:port to make sure the DNS resolver is working correctly for the proxy and connecting to the proxy server.

return p
}
7 changes: 3 additions & 4 deletions internal/transport/http2_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,6 @@ type http2Client struct {

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)
}
networkType, ok := networktype.Get(addr)
if fn != nil {
// Special handling for unix scheme with custom dialer. Back in the day,
Expand All @@ -182,6 +178,9 @@ func dial(ctx context.Context, fn func(context.Context, string) (net.Conn, error
if !ok {
networkType, address = parseDialTarget(address)
}
if opts, present := proxyattributes.Get(addr); present {
return proxyDial(ctx, addr, grpcUA, opts)
}
return internal.NetDialerWithTCPKeepalive().DialContext(ctx, networkType, address)
}

Expand Down
21 changes: 10 additions & 11 deletions internal/transport/proxy_ext_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import (
"google.golang.org/grpc/internal/resolver/delegatingresolver"
"google.golang.org/grpc/internal/stubserver"
"google.golang.org/grpc/internal/testutils"
transporttestutils "google.golang.org/grpc/internal/transport/testutils"
testgrpc "google.golang.org/grpc/interop/grpc_testing"
"google.golang.org/grpc/resolver"
"google.golang.org/grpc/resolver/manual"
Expand Down Expand Up @@ -88,7 +87,7 @@ func (s) TestGRPCDialWithProxy(t *testing.T) {
t.Errorf(" Unexpected request host: %s , want = %s ", got, want)
}
}
pServer := transporttestutils.HTTPProxy(t, reqCheck, false)
pServer := testutils.HTTPProxy(t, reqCheck, false)

// Overwrite the function in the test and restore them in defer.
hpfe := func(req *http.Request) (*url.URL, error) {
Expand Down Expand Up @@ -145,7 +144,7 @@ func (s) TestGRPCDialWithDNSAndProxy(t *testing.T) {
t.Errorf("isIPAddr(%q) = %t, want = %t", host, got, want)
}
}
pServer := transporttestutils.HTTPProxy(t, reqCheck, false)
pServer := testutils.HTTPProxy(t, reqCheck, false)

// Overwrite the function in the test and restore them in defer.
hpfe := func(req *http.Request) (*url.URL, error) {
Expand Down Expand Up @@ -200,7 +199,7 @@ func (s) TestNewClientWithProxy(t *testing.T) {
t.Errorf(" Unexpected request host: %s , want = %s ", got, want)
}
}
pServer := transporttestutils.HTTPProxy(t, reqCheck, false)
pServer := testutils.HTTPProxy(t, reqCheck, false)

// Overwrite the function in the test and restore them in defer.
hpfe := func(req *http.Request) (*url.URL, error) {
Expand Down Expand Up @@ -254,7 +253,7 @@ func (s) TestNewClientWithProxyAndCustomResolver(t *testing.T) {
t.Errorf("isIPAddr(%q) = %t, want = %t", host, got, want)
}
}
pServer := transporttestutils.HTTPProxy(t, reqCheck, false)
pServer := testutils.HTTPProxy(t, reqCheck, false)

// Overwrite the function in the test and restore them in defer.
hpfe := func(req *http.Request) (*url.URL, error) {
Expand Down Expand Up @@ -297,7 +296,7 @@ func (s) TestNewClientWithProxyAndCustomResolver(t *testing.T) {
}

// Tests the scenario where grpc.NewClient is used with the default "dns"
// resolver and the dial option grpc.WithTargetResolutionEnabled() is set,
// resolver and the dial option grpc.WithLocalDNSResolution() is set,
// enabling target resolution on the client. The test verifies that target
// resolution happens on the client by sending resolved target URI in HTTP
// CONNECT request, the proxy URI is resolved correctly, and the connection is
Expand All @@ -316,7 +315,7 @@ func (s) TestNewClientWithProxyAndTargetResolutionEnabled(t *testing.T) {
t.Errorf("isIPAddr(%q) = %t, want = %t", host, got, want)
}
}
pServer := transporttestutils.HTTPProxy(t, reqCheck, false)
pServer := testutils.HTTPProxy(t, reqCheck, false)

// Overwrite the function in the test and restore them in defer.
hpfe := func(req *http.Request) (*url.URL, error) {
Expand All @@ -335,7 +334,7 @@ func (s) TestNewClientWithProxyAndTargetResolutionEnabled(t *testing.T) {

ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
conn, err := grpc.NewClient(unresolvedTargetURI, grpc.WithTargetResolutionEnabled(), grpc.WithTransportCredentials(insecure.NewCredentials()))
conn, err := grpc.NewClient(unresolvedTargetURI, grpc.WithLocalDNSResolution(), grpc.WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
t.Fatalf("grpc.NewClient(%s) failed: %v", unresolvedTargetURI, err)
}
Expand All @@ -361,7 +360,7 @@ func (s) TestNewClientWithNoProxy(t *testing.T) {
backend := startBackendServer(t)
unresolvedTargetURI := fmt.Sprintf("localhost:%d", testutils.ParsePort(t, backend.Address))
reqCheck := func(_ *http.Request) { t.Error("proxy server should not have received a Connect request") }
pServer := transporttestutils.HTTPProxy(t, reqCheck, false)
pServer := testutils.HTTPProxy(t, reqCheck, false)

// Overwrite the function in the test and restore them in defer.
hpfe := func(req *http.Request) (*url.URL, error) {
Expand Down Expand Up @@ -406,7 +405,7 @@ func (s) TestNewClientWithContextDialer(t *testing.T) {
backend := startBackendServer(t)
unresolvedTargetURI := fmt.Sprintf("localhost:%d", testutils.ParsePort(t, backend.Address))
reqCheck := func(_ *http.Request) { t.Error("proxy server should not have received a Connect request") }
pServer := transporttestutils.HTTPProxy(t, reqCheck, false)
pServer := testutils.HTTPProxy(t, reqCheck, false)

// Overwrite the function in the test and restore them in defer.
hpfe := func(req *http.Request) (*url.URL, error) {
Expand Down Expand Up @@ -475,7 +474,7 @@ func (s) TestBasicAuthInNewClientWithProxy(t *testing.T) {
t.Errorf("unexpected auth %q (%q), want %q (%q)", got, gotDecoded, wantProxyAuthStr, wantDecoded)
}
}
pServer := transporttestutils.HTTPProxy(t, reqCheck, false)
pServer := testutils.HTTPProxy(t, reqCheck, false)

t.Setenv("HTTPS_PROXY", user+":"+password+"@"+pServer.Addr)

Expand Down
3 changes: 1 addition & 2 deletions internal/transport/proxy_test.go
arjan-bal marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import (

"google.golang.org/grpc/internal/proxyattributes"
"google.golang.org/grpc/internal/testutils"
transporttestutils "google.golang.org/grpc/internal/transport/testutils"
"google.golang.org/grpc/resolver"
)

Expand All @@ -54,7 +53,7 @@ func (s) TestHTTPConnectWithServerHello(t *testing.T) {
t.Error(err)
}
}
pServer := transporttestutils.HTTPProxy(t, reqCheck, true)
pServer := testutils.HTTPProxy(t, reqCheck, true)

msg := []byte{4, 3, 5, 2}
recvBuf := make([]byte, len(msg))
Expand Down
4 changes: 2 additions & 2 deletions resolver_wrapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,14 @@ func (ccr *ccResolverWrapper) start() error {
}
var err error
// The delegating resolver is used unless:
// - A custom dialer is provided via WithContextDialer dialoption.
// - A custom dialer is provided via WithContextDialer dialoption or
// - Proxy usage is disabled through WithNoProxy dialoption.
// In these cases, the resolver is built based on the scheme of target,
// using the appropriate resolver builder.
if ccr.cc.dopts.copts.Dialer != nil || !ccr.cc.dopts.useProxy {
ccr.resolver, err = ccr.cc.resolverBuilder.Build(ccr.cc.parsedTarget, ccr, opts)
} else {
ccr.resolver, err = delegatingresolver.New(ccr.cc.parsedTarget, ccr, opts, ccr.cc.resolverBuilder, ccr.cc.dopts.targetResolutionEnabled)
ccr.resolver, err = delegatingresolver.New(ccr.cc.parsedTarget, ccr, opts, ccr.cc.resolverBuilder, ccr.cc.dopts.enableLocalDNSResolution)
}
errCh <- err
})
Expand Down
Loading