From 3575d42a309234613d70b1abc62a15266e0d4a77 Mon Sep 17 00:00:00 2001 From: Matt Straathof <11823378+bruuuuuuuce@users.noreply.github.com> Date: Wed, 28 Feb 2024 14:04:49 -0800 Subject: [PATCH 01/14] chore: expose `NewClient` method to end users This will allow users to create a connection, leaving it in idle state, rather than having it automatically try and connect in the background, which is the current behavior of `Dial`. Leaving the connection in idle state is more consistent with how other grpc implementations deal with connections --- clientconn.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clientconn.go b/clientconn.go index 6a44343aea00..72790c6e9788 100644 --- a/clientconn.go +++ b/clientconn.go @@ -117,8 +117,8 @@ func (dcs *defaultConfigSelector) SelectConfig(rpcInfo iresolver.RPCInfo) (*ires }, nil } -// newClient returns a new client in idle mode. -func newClient(target string, opts ...DialOption) (conn *ClientConn, err error) { +// NewClient returns a new client in idle mode. +func NewClient(target string, opts ...DialOption) (conn *ClientConn, err error) { cc := &ClientConn{ target: target, conns: make(map[*addrConn]struct{}), From 5455592d898db554426852b06f0baace67300db0 Mon Sep 17 00:00:00 2001 From: Matt Straathof Date: Wed, 28 Feb 2024 14:14:36 -0800 Subject: [PATCH 02/14] chore: fix build, update usages to use new NewClient naming --- clientconn.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clientconn.go b/clientconn.go index 72790c6e9788..4b26d392c01e 100644 --- a/clientconn.go +++ b/clientconn.go @@ -208,7 +208,7 @@ func NewClient(target string, opts ...DialOption) (conn *ClientConn, err error) // https://github.com/grpc/grpc/blob/master/doc/naming.md. // e.g. to use dns resolver, a "dns:///" prefix should be applied to the target. func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *ClientConn, err error) { - cc, err := newClient(target, opts...) + cc, err := NewClient(target, opts...) if err != nil { return nil, err } From 06bb15f762720d2dc71efc8c6905f6567d167d82 Mon Sep 17 00:00:00 2001 From: Matt Straathof Date: Wed, 28 Feb 2024 16:11:44 -0800 Subject: [PATCH 03/14] fix: use dns resolver when calling NewClient directly --- balancer/rls/config.go | 4 ++-- clientconn.go | 13 +++++++++++-- clientconn_parsed_target_test.go | 4 ++-- dialoptions.go | 10 ++++++++++ resolver/resolver.go | 23 ++++++++++++++++++++--- 5 files changed, 45 insertions(+), 9 deletions(-) diff --git a/balancer/rls/config.go b/balancer/rls/config.go index 439581c78bc1..51f434fef1b1 100644 --- a/balancer/rls/config.go +++ b/balancer/rls/config.go @@ -195,13 +195,13 @@ func parseRLSProto(rlsProto *rlspb.RouteLookupConfig) (*lbConfig, error) { parsedTarget, err := url.Parse(lookupService) if err != nil { // url.Parse() fails if scheme is missing. Retry with default scheme. - parsedTarget, err = url.Parse(resolver.GetDefaultScheme() + ":///" + lookupService) + parsedTarget, err = url.Parse(resolver.GetDefaultSchemeOrPassthrough() + ":///" + lookupService) if err != nil { return nil, fmt.Errorf("rls: invalid target URI in lookup_service %s", lookupService) } } if parsedTarget.Scheme == "" { - parsedTarget.Scheme = resolver.GetDefaultScheme() + parsedTarget.Scheme = resolver.GetDefaultSchemeOrPassthrough() } if resolver.Get(parsedTarget.Scheme) == nil { return nil, fmt.Errorf("rls: unregistered scheme in lookup_service %s", lookupService) diff --git a/clientconn.go b/clientconn.go index 4b26d392c01e..549dad89e9d1 100644 --- a/clientconn.go +++ b/clientconn.go @@ -208,6 +208,10 @@ func NewClient(target string, opts ...DialOption) (conn *ClientConn, err error) // https://github.com/grpc/grpc/blob/master/doc/naming.md. // e.g. to use dns resolver, a "dns:///" prefix should be applied to the target. 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. + // The eagerConnect option is used to tell the ClientChannel that DialContext was called to make + // this channel, and hence tries to connect immediately. + opts = append(opts, WithEagerConnect()) cc, err := NewClient(target, opts...) if err != nil { return nil, err @@ -1740,8 +1744,13 @@ func (cc *ClientConn) parseTargetAndFindResolver() error { // We are here because the user's dial target did not contain a scheme or // specified an unregistered scheme. We should fallback to the default // scheme, except when a custom dialer is specified in which case, we should - // always use passthrough scheme. - defScheme := resolver.GetDefaultScheme() + // always use passthrough scheme. For either case, we need to respect any overridden + // global defaults set by the user. + defScheme := resolver.GetDefaultSchemeOrDns() + if cc.dopts.eagerConnect { + defScheme = resolver.GetDefaultSchemeOrPassthrough() + } + channelz.Infof(logger, cc.channelzID, "fallback to scheme %q", defScheme) canonicalTarget := defScheme + ":///" + cc.target diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index 1ff46aaf08c7..2c5f6c7825e7 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -34,7 +34,7 @@ import ( ) func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { - defScheme := resolver.GetDefaultScheme() + defScheme := resolver.GetDefaultSchemeOrPassthrough() tests := []struct { target string wantParsed resolver.Target @@ -93,7 +93,7 @@ func (s) TestParsedTarget_Failure_WithoutCustomDialer(t *testing.T) { } func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { - defScheme := resolver.GetDefaultScheme() + defScheme := resolver.GetDefaultSchemeOrPassthrough() tests := []struct { target string wantParsed resolver.Target diff --git a/dialoptions.go b/dialoptions.go index a95f86a1f4bf..757230956c48 100644 --- a/dialoptions.go +++ b/dialoptions.go @@ -79,6 +79,7 @@ type dialOptions struct { resolvers []resolver.Builder idleTimeout time.Duration recvBufferPool SharedBufferPool + eagerConnect bool } // DialOption configures how we set up the connection. @@ -487,6 +488,15 @@ func WithUserAgent(s string) DialOption { }) } +// WithEagerConnect tells the ClientConnection that the connection was created directly +// with either Dial or DialContext, meaning that the connection is automatically put into +// a connecting state. +func WithEagerConnect() DialOption { + return newFuncDialOption(func(o *dialOptions) { + o.eagerConnect = true + }) +} + // WithKeepaliveParams returns a DialOption that specifies keepalive parameters // for the client transport. func WithKeepaliveParams(kp keepalive.ClientParameters) DialOption { diff --git a/resolver/resolver.go b/resolver/resolver.go index 95c46452464e..2b12d937b118 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -36,7 +36,9 @@ var ( // m is a map from scheme to resolver builder. m = make(map[string]Builder) // defaultScheme is the default scheme to use. - defaultScheme = "passthrough" + defaultScheme = "" + passthroughScheme = "passthrough" + dnsScheme = "dns" ) // TODO(bar) install dns resolver in init(){}. @@ -72,8 +74,23 @@ func SetDefaultScheme(scheme string) { defaultScheme = scheme } -// GetDefaultScheme gets the default scheme that will be used. -func GetDefaultScheme() string { +// GetDefaultSchemeOrPassthrough gets the default scheme that will be used, if no scheme was +// overridden, defaults to the "passthrough" scheme. This scheme is used when directly calling +// the Dial method to create a client connection. +func GetDefaultSchemeOrPassthrough() string { + if defaultScheme == "" { + return passthroughScheme + } + return defaultScheme +} + +// GetDefaultSchemeOrDns gets the default scheme that will be used, if no scheme was overriden, +// defaults to the "dns" scheme. This is the default scheme when calling NewClient to generate a +// new client connection. +func GetDefaultSchemeOrDns() string { + if defaultScheme == "" { + return dnsScheme + } return defaultScheme } From 3db24a8fc390d08eb9b92bf3149ee64633c6e772 Mon Sep 17 00:00:00 2001 From: Matt Straathof Date: Wed, 28 Feb 2024 16:14:17 -0800 Subject: [PATCH 04/14] chore: fix spelling error --- resolver/resolver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resolver/resolver.go b/resolver/resolver.go index 2b12d937b118..f9932f2a10e4 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -84,7 +84,7 @@ func GetDefaultSchemeOrPassthrough() string { return defaultScheme } -// GetDefaultSchemeOrDns gets the default scheme that will be used, if no scheme was overriden, +// GetDefaultSchemeOrDns gets the default scheme that will be used, if no scheme was overridden, // defaults to the "dns" scheme. This is the default scheme when calling NewClient to generate a // new client connection. func GetDefaultSchemeOrDns() string { From fec57e6d78edfdb5ce5ab163ff2399eb0486c2a7 Mon Sep 17 00:00:00 2001 From: Matt Straathof Date: Wed, 28 Feb 2024 16:26:41 -0800 Subject: [PATCH 05/14] chore: GetDefaultSchemeOrDns -> GetDefaultSchemeOrDNS --- clientconn.go | 2 +- resolver/resolver.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/clientconn.go b/clientconn.go index 549dad89e9d1..d8089c66265b 100644 --- a/clientconn.go +++ b/clientconn.go @@ -1746,7 +1746,7 @@ func (cc *ClientConn) parseTargetAndFindResolver() error { // scheme, except when a custom dialer is specified in which case, we should // always use passthrough scheme. For either case, we need to respect any overridden // global defaults set by the user. - defScheme := resolver.GetDefaultSchemeOrDns() + defScheme := resolver.GetDefaultSchemeOrDNS() if cc.dopts.eagerConnect { defScheme = resolver.GetDefaultSchemeOrPassthrough() } diff --git a/resolver/resolver.go b/resolver/resolver.go index f9932f2a10e4..8a0e05396605 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -84,10 +84,10 @@ func GetDefaultSchemeOrPassthrough() string { return defaultScheme } -// GetDefaultSchemeOrDns gets the default scheme that will be used, if no scheme was overridden, +// GetDefaultSchemeOrDNS gets the default scheme that will be used, if no scheme was overridden, // defaults to the "dns" scheme. This is the default scheme when calling NewClient to generate a // new client connection. -func GetDefaultSchemeOrDns() string { +func GetDefaultSchemeOrDNS() string { if defaultScheme == "" { return dnsScheme } From 50f2e9b56a1045828bb7f4f824c2c59c16c84ffb Mon Sep 17 00:00:00 2001 From: Matt Straathof Date: Fri, 1 Mar 2024 15:07:35 -0800 Subject: [PATCH 06/14] fix: remove all user visisble changes aside from new NewClient func --- balancer/rls/config.go | 4 ++-- clientconn.go | 19 +++++++++++-------- clientconn_parsed_target_test.go | 4 ++-- dialoptions.go | 14 +++----------- internal/internal.go | 3 +++ resolver/resolver.go | 28 +++++++--------------------- 6 files changed, 28 insertions(+), 44 deletions(-) diff --git a/balancer/rls/config.go b/balancer/rls/config.go index 51f434fef1b1..439581c78bc1 100644 --- a/balancer/rls/config.go +++ b/balancer/rls/config.go @@ -195,13 +195,13 @@ func parseRLSProto(rlsProto *rlspb.RouteLookupConfig) (*lbConfig, error) { parsedTarget, err := url.Parse(lookupService) if err != nil { // url.Parse() fails if scheme is missing. Retry with default scheme. - parsedTarget, err = url.Parse(resolver.GetDefaultSchemeOrPassthrough() + ":///" + lookupService) + parsedTarget, err = url.Parse(resolver.GetDefaultScheme() + ":///" + lookupService) if err != nil { return nil, fmt.Errorf("rls: invalid target URI in lookup_service %s", lookupService) } } if parsedTarget.Scheme == "" { - parsedTarget.Scheme = resolver.GetDefaultSchemeOrPassthrough() + parsedTarget.Scheme = resolver.GetDefaultScheme() } if resolver.Get(parsedTarget.Scheme) == nil { return nil, fmt.Errorf("rls: unregistered scheme in lookup_service %s", lookupService) diff --git a/clientconn.go b/clientconn.go index d8089c66265b..4e2bcd28223d 100644 --- a/clientconn.go +++ b/clientconn.go @@ -117,12 +117,11 @@ func (dcs *defaultConfigSelector) SelectConfig(rpcInfo iresolver.RPCInfo) (*ires }, nil } -// NewClient returns a new client in idle mode. -func NewClient(target string, opts ...DialOption) (conn *ClientConn, err error) { +func newClient(target, defaultScheme string, opts ...DialOption) (conn *ClientConn, err error) { cc := &ClientConn{ target: target, conns: make(map[*addrConn]struct{}), - dopts: defaultDialOptions(), + dopts: defaultDialOptions(defaultScheme), czData: new(channelzData), } @@ -191,6 +190,11 @@ func NewClient(target string, opts ...DialOption) (conn *ClientConn, err error) return cc, nil } +// NewClient returns a new client in idle mode. +func NewClient(target string, opts ...DialOption) (conn *ClientConn, err error) { + return newClient(target, "dns", opts...) +} + // DialContext creates a client connection to the given target. By default, it's // a non-blocking dial (the function won't wait for connections to be // established, and connecting happens in the background). To make it a blocking @@ -211,8 +215,7 @@ 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. // The eagerConnect option is used to tell the ClientChannel that DialContext was called to make // this channel, and hence tries to connect immediately. - opts = append(opts, WithEagerConnect()) - cc, err := NewClient(target, opts...) + cc, err := newClient(target, "passthrough", opts...) if err != nil { return nil, err } @@ -1746,9 +1749,9 @@ func (cc *ClientConn) parseTargetAndFindResolver() error { // scheme, except when a custom dialer is specified in which case, we should // always use passthrough scheme. For either case, we need to respect any overridden // global defaults set by the user. - defScheme := resolver.GetDefaultSchemeOrDNS() - if cc.dopts.eagerConnect { - defScheme = resolver.GetDefaultSchemeOrPassthrough() + defScheme := cc.dopts.defScheme + if defScheme == "" || internal.UserSetDefaultScheme { + defScheme = resolver.GetDefaultScheme() } channelz.Infof(logger, cc.channelzID, "fallback to scheme %q", defScheme) diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index 2c5f6c7825e7..1ff46aaf08c7 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -34,7 +34,7 @@ import ( ) func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { - defScheme := resolver.GetDefaultSchemeOrPassthrough() + defScheme := resolver.GetDefaultScheme() tests := []struct { target string wantParsed resolver.Target @@ -93,7 +93,7 @@ func (s) TestParsedTarget_Failure_WithoutCustomDialer(t *testing.T) { } func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { - defScheme := resolver.GetDefaultSchemeOrPassthrough() + defScheme := resolver.GetDefaultScheme() tests := []struct { target string wantParsed resolver.Target diff --git a/dialoptions.go b/dialoptions.go index 757230956c48..19e3de6d4398 100644 --- a/dialoptions.go +++ b/dialoptions.go @@ -79,7 +79,7 @@ type dialOptions struct { resolvers []resolver.Builder idleTimeout time.Duration recvBufferPool SharedBufferPool - eagerConnect bool + defScheme string } // DialOption configures how we set up the connection. @@ -488,15 +488,6 @@ func WithUserAgent(s string) DialOption { }) } -// WithEagerConnect tells the ClientConnection that the connection was created directly -// with either Dial or DialContext, meaning that the connection is automatically put into -// a connecting state. -func WithEagerConnect() DialOption { - return newFuncDialOption(func(o *dialOptions) { - o.eagerConnect = true - }) -} - // WithKeepaliveParams returns a DialOption that specifies keepalive parameters // for the client transport. func WithKeepaliveParams(kp keepalive.ClientParameters) DialOption { @@ -641,7 +632,7 @@ func withHealthCheckFunc(f internal.HealthChecker) DialOption { }) } -func defaultDialOptions() dialOptions { +func defaultDialOptions(defScheme string) dialOptions { return dialOptions{ copts: transport.ConnectOptions{ ReadBufferSize: defaultReadBufSize, @@ -653,6 +644,7 @@ func defaultDialOptions() dialOptions { healthCheckFunc: internal.HealthCheckFunc, idleTimeout: 30 * time.Minute, recvBufferPool: nopBufferPool{}, + defScheme: defScheme, } } diff --git a/internal/internal.go b/internal/internal.go index 5082fdaba961..dec57b466cb1 100644 --- a/internal/internal.go +++ b/internal/internal.go @@ -197,6 +197,9 @@ var ( // FromOutgoingContextRaw returns the un-merged, intermediary contents of metadata.rawMD. FromOutgoingContextRaw any // func(context.Context) (metadata.MD, [][]string, bool) + + // UserSetDefaultScheme is set to true if the user has overridden the default resolver scheme. + UserSetDefaultScheme bool ) // HealthChecker defines the signature of the client-side LB channel health checking function. diff --git a/resolver/resolver.go b/resolver/resolver.go index 8a0e05396605..a437540430d9 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -23,6 +23,7 @@ package resolver import ( "context" "fmt" + "google.golang.org/grpc/internal" "net" "net/url" "strings" @@ -36,9 +37,7 @@ var ( // m is a map from scheme to resolver builder. m = make(map[string]Builder) // defaultScheme is the default scheme to use. - defaultScheme = "" - passthroughScheme = "passthrough" - dnsScheme = "dns" + defaultScheme = "dns" ) // TODO(bar) install dns resolver in init(){}. @@ -65,32 +64,19 @@ func Get(scheme string) Builder { } // SetDefaultScheme sets the default scheme that will be used. The default -// default scheme is "passthrough". +// scheme is "dns". // // NOTE: this function must only be called during initialization time (i.e. in // an init() function), and is not thread-safe. The scheme set last overrides // previously set values. func SetDefaultScheme(scheme string) { + internal.UserSetDefaultScheme = true defaultScheme = scheme } -// GetDefaultSchemeOrPassthrough gets the default scheme that will be used, if no scheme was -// overridden, defaults to the "passthrough" scheme. This scheme is used when directly calling -// the Dial method to create a client connection. -func GetDefaultSchemeOrPassthrough() string { - if defaultScheme == "" { - return passthroughScheme - } - return defaultScheme -} - -// GetDefaultSchemeOrDNS gets the default scheme that will be used, if no scheme was overridden, -// defaults to the "dns" scheme. This is the default scheme when calling NewClient to generate a -// new client connection. -func GetDefaultSchemeOrDNS() string { - if defaultScheme == "" { - return dnsScheme - } +// GetDefaultScheme gets the default scheme that will be used, if no scheme was +// overridden, defaults to the "dns" scheme. +func GetDefaultScheme() string { return defaultScheme } From 5b9e5d347a3a51721d2bc22f1a16b3d1187bd660 Mon Sep 17 00:00:00 2001 From: Matt Straathof Date: Fri, 1 Mar 2024 15:21:02 -0800 Subject: [PATCH 07/14] fix: add internal.UserSetDefaultScheme to clientconn.go --- clientconn.go | 1 + resolver/resolver.go | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/clientconn.go b/clientconn.go index 4e2bcd28223d..c7b28a906842 100644 --- a/clientconn.go +++ b/clientconn.go @@ -712,6 +712,7 @@ func init() { internal.ExitIdleModeForTesting = func(cc *ClientConn) error { return cc.idlenessMgr.ExitIdleMode() } + internal.UserSetDefaultScheme = resolver.GetDefaultScheme() != "dns" } func (cc *ClientConn) maybeApplyDefaultServiceConfig(addrs []resolver.Address) { diff --git a/resolver/resolver.go b/resolver/resolver.go index a437540430d9..9ca7ce2542bc 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -23,7 +23,6 @@ package resolver import ( "context" "fmt" - "google.golang.org/grpc/internal" "net" "net/url" "strings" @@ -70,7 +69,6 @@ func Get(scheme string) Builder { // an init() function), and is not thread-safe. The scheme set last overrides // previously set values. func SetDefaultScheme(scheme string) { - internal.UserSetDefaultScheme = true defaultScheme = scheme } From 461c79163b87a9a5fda8bfc9b0a5e671d64ce2ab Mon Sep 17 00:00:00 2001 From: Matt Straathof Date: Fri, 1 Mar 2024 15:35:28 -0800 Subject: [PATCH 08/14] fix: unit tests failing due to default resolver being dns --- clientconn_parsed_target_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index 1ff46aaf08c7..8cbca8fd4cfc 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -34,7 +34,7 @@ import ( ) func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { - defScheme := resolver.GetDefaultScheme() + defScheme := "passthrough" tests := []struct { target string wantParsed resolver.Target @@ -93,7 +93,7 @@ func (s) TestParsedTarget_Failure_WithoutCustomDialer(t *testing.T) { } func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { - defScheme := resolver.GetDefaultScheme() + defScheme := "passthrough" tests := []struct { target string wantParsed resolver.Target From 9b160136465f8e4bf99e2b667d1caca0b2b7fb2c Mon Sep 17 00:00:00 2001 From: Matt Straathof Date: Wed, 6 Mar 2024 13:09:35 -0800 Subject: [PATCH 09/14] fix: set internal.UserSetDefaultScheme inside of SetDefaultScheme() --- clientconn.go | 3 +- clientconn_parsed_target_test.go | 88 +++++++++++++++---- credentials/google/google_test.go | 10 +-- credentials/google/xds.go | 4 +- credentials/google/xds_test.go | 4 +- internal/internal.go | 2 +- .../{xds_handshake_cluster.go => xds/xds.go} | 2 +- resolver/resolver.go | 10 ++- .../balancer/clusterimpl/balancer_test.go | 6 +- .../balancer/clusterimpl/clusterimpl.go | 6 +- 10 files changed, 96 insertions(+), 39 deletions(-) rename internal/{xds_handshake_cluster.go => xds/xds.go} (98%) diff --git a/clientconn.go b/clientconn.go index c7b28a906842..7bef26f1b185 100644 --- a/clientconn.go +++ b/clientconn.go @@ -712,7 +712,6 @@ func init() { internal.ExitIdleModeForTesting = func(cc *ClientConn) error { return cc.idlenessMgr.ExitIdleMode() } - internal.UserSetDefaultScheme = resolver.GetDefaultScheme() != "dns" } func (cc *ClientConn) maybeApplyDefaultServiceConfig(addrs []resolver.Address) { @@ -1751,7 +1750,7 @@ func (cc *ClientConn) parseTargetAndFindResolver() error { // always use passthrough scheme. For either case, we need to respect any overridden // global defaults set by the user. defScheme := cc.dopts.defScheme - if defScheme == "" || internal.UserSetDefaultScheme { + if internal.UserSetDefaultScheme { defScheme = resolver.GetDefaultScheme() } diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index 8cbca8fd4cfc..95ad64734152 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -33,29 +33,75 @@ import ( "google.golang.org/grpc/resolver" ) +func generateTarget(scheme string, target string) resolver.Target { + return resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", scheme, target))} +} + func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { - defScheme := "passthrough" + dialScheme := resolver.GetDefaultScheme() + newClientScheme := "dns" tests := []struct { - target string - wantParsed resolver.Target + target string + wantDialParse resolver.Target + wantNewClientParse resolver.Target }{ // No scheme is specified. - {target: "://a/b", wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "://a/b"))}}, - {target: "a//b", wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a//b"))}}, + { + target: "://a/b", + wantDialParse: generateTarget(dialScheme, "://a/b"), + wantNewClientParse: generateTarget(newClientScheme, "://a/b"), + }, + { + target: "a//b", + wantDialParse: generateTarget(dialScheme, "a//b"), + wantNewClientParse: generateTarget(newClientScheme, "a//b"), + }, // An unregistered scheme is specified. - {target: "a:///", wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a:///"))}}, - {target: "a:b", wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a:b"))}}, + { + target: "a:///", + wantDialParse: generateTarget(dialScheme, "a:///"), + wantNewClientParse: generateTarget(newClientScheme, "a:///"), + }, + { + target: "a:b", + wantDialParse: generateTarget(dialScheme, "a:b"), + wantNewClientParse: generateTarget(newClientScheme, "a:b"), + }, // A registered scheme is specified. - {target: "dns://a.server.com/google.com", wantParsed: resolver.Target{URL: *testutils.MustParseURL("dns://a.server.com/google.com")}}, - {target: "unix-abstract:/ a///://::!@#$%25^&*()b", wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:/ a///://::!@#$%25^&*()b")}}, - {target: "unix-abstract:passthrough:abc", wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:passthrough:abc")}}, - {target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{URL: *testutils.MustParseURL("passthrough:///unix:///a/b/c")}}, + { + target: "dns://a.server.com/google.com", + wantDialParse: resolver.Target{URL: *testutils.MustParseURL("dns://a.server.com/google.com")}, + wantNewClientParse: resolver.Target{URL: *testutils.MustParseURL("dns://a.server.com/google.com")}, + }, + { + target: "unix-abstract:/ a///://::!@#$%25^&*()b", + wantDialParse: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:/ a///://::!@#$%25^&*()b")}, + wantNewClientParse: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:/ a///://::!@#$%25^&*()b")}, + }, + { + target: "unix-abstract:passthrough:abc", + wantDialParse: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:passthrough:abc")}, + wantNewClientParse: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:passthrough:abc")}, + }, + { + target: "passthrough:///unix:///a/b/c", + wantDialParse: resolver.Target{URL: *testutils.MustParseURL("passthrough:///unix:///a/b/c")}, + wantNewClientParse: resolver.Target{URL: *testutils.MustParseURL("passthrough:///unix:///a/b/c")}, + }, // Cases for `scheme:absolute-path`. - {target: "dns:/a/b/c", wantParsed: resolver.Target{URL: *testutils.MustParseURL("dns:/a/b/c")}}, - {target: "unregistered:/a/b/c", wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "unregistered:/a/b/c"))}}, + { + target: "dns:/a/b/c", + wantDialParse: resolver.Target{URL: *testutils.MustParseURL("dns:/a/b/c")}, + wantNewClientParse: resolver.Target{URL: *testutils.MustParseURL("dns:/a/b/c")}, + }, + { + target: "unregistered:/a/b/c", + wantDialParse: generateTarget(dialScheme, "unregistered:/a/b/c"), + wantNewClientParse: generateTarget(newClientScheme, "unregistered:/a/b/c"), + }, } for _, test := range tests { @@ -66,8 +112,18 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { } defer cc.Close() - if !cmp.Equal(cc.parsedTarget, test.wantParsed) { - t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantParsed) + if !cmp.Equal(cc.parsedTarget, test.wantDialParse) { + t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantDialParse) + } + + cc, err = NewClient(test.target, WithTransportCredentials(insecure.NewCredentials())) + if err != nil { + t.Fatalf("NewClient(%q) failed: %v", test.target, err) + } + defer cc.Close() + + if !cmp.Equal(cc.parsedTarget, test.wantNewClientParse) { + t.Errorf("cc.parsedTarget for newClient target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantNewClientParse) } }) } @@ -93,7 +149,7 @@ func (s) TestParsedTarget_Failure_WithoutCustomDialer(t *testing.T) { } func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { - defScheme := "passthrough" + defScheme := resolver.GetDefaultScheme() tests := []struct { target string wantParsed resolver.Target diff --git a/credentials/google/google_test.go b/credentials/google/google_test.go index 1809d545d0ec..51da7f6b2d41 100644 --- a/credentials/google/google_test.go +++ b/credentials/google/google_test.go @@ -20,11 +20,11 @@ package google import ( "context" + "google.golang.org/grpc/internal/xds" "net" "testing" "google.golang.org/grpc/credentials" - "google.golang.org/grpc/internal" icredentials "google.golang.org/grpc/internal/credentials" "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/resolver" @@ -109,7 +109,7 @@ func (s) TestClientHandshakeBasedOnClusterName(t *testing.T) { { name: "with non-CFE cluster name", ctx: icredentials.NewClientHandshakeInfoContext(context.Background(), credentials.ClientHandshakeInfo{ - Attributes: internal.SetXDSHandshakeClusterName(resolver.Address{}, "lalala").Attributes, + Attributes: xds.SetXDSHandshakeClusterName(resolver.Address{}, "lalala").Attributes, }), // non-CFE backends should use alts. wantTyp: "alts", @@ -117,7 +117,7 @@ func (s) TestClientHandshakeBasedOnClusterName(t *testing.T) { { name: "with CFE cluster name", ctx: icredentials.NewClientHandshakeInfoContext(context.Background(), credentials.ClientHandshakeInfo{ - Attributes: internal.SetXDSHandshakeClusterName(resolver.Address{}, "google_cfe_bigtable.googleapis.com").Attributes, + Attributes: xds.SetXDSHandshakeClusterName(resolver.Address{}, "google_cfe_bigtable.googleapis.com").Attributes, }), // CFE should use tls. wantTyp: "tls", @@ -125,7 +125,7 @@ func (s) TestClientHandshakeBasedOnClusterName(t *testing.T) { { name: "with xdstp CFE cluster name", ctx: icredentials.NewClientHandshakeInfoContext(context.Background(), credentials.ClientHandshakeInfo{ - Attributes: internal.SetXDSHandshakeClusterName(resolver.Address{}, "xdstp://traffic-director-c2p.xds.googleapis.com/envoy.config.cluster.v3.Cluster/google_cfe_bigtable.googleapis.com").Attributes, + Attributes: xds.SetXDSHandshakeClusterName(resolver.Address{}, "xdstp://traffic-director-c2p.xds.googleapis.com/envoy.config.cluster.v3.Cluster/google_cfe_bigtable.googleapis.com").Attributes, }), // CFE should use tls. wantTyp: "tls", @@ -133,7 +133,7 @@ func (s) TestClientHandshakeBasedOnClusterName(t *testing.T) { { name: "with xdstp non-CFE cluster name", ctx: icredentials.NewClientHandshakeInfoContext(context.Background(), credentials.ClientHandshakeInfo{ - Attributes: internal.SetXDSHandshakeClusterName(resolver.Address{}, "xdstp://other.com/envoy.config.cluster.v3.Cluster/google_cfe_bigtable.googleapis.com").Attributes, + Attributes: xds.SetXDSHandshakeClusterName(resolver.Address{}, "xdstp://other.com/envoy.config.cluster.v3.Cluster/google_cfe_bigtable.googleapis.com").Attributes, }), // non-CFE should use atls. wantTyp: "alts", diff --git a/credentials/google/xds.go b/credentials/google/xds.go index 2c5c8b9eee13..8793a6878cd1 100644 --- a/credentials/google/xds.go +++ b/credentials/google/xds.go @@ -20,12 +20,12 @@ package google import ( "context" + "google.golang.org/grpc/internal/xds" "net" "net/url" "strings" "google.golang.org/grpc/credentials" - "google.golang.org/grpc/internal" ) const cfeClusterNamePrefix = "google_cfe_" @@ -63,7 +63,7 @@ func clusterName(ctx context.Context) string { if chi.Attributes == nil { return "" } - cluster, _ := internal.GetXDSHandshakeClusterName(chi.Attributes) + cluster, _ := xds.GetXDSHandshakeClusterName(chi.Attributes) return cluster } diff --git a/credentials/google/xds_test.go b/credentials/google/xds_test.go index 8aeba396a518..9deb40be2739 100644 --- a/credentials/google/xds_test.go +++ b/credentials/google/xds_test.go @@ -20,10 +20,10 @@ package google import ( "context" + "google.golang.org/grpc/internal/xds" "testing" "google.golang.org/grpc/credentials" - "google.golang.org/grpc/internal" icredentials "google.golang.org/grpc/internal/credentials" "google.golang.org/grpc/resolver" ) @@ -31,7 +31,7 @@ import ( func (s) TestIsDirectPathCluster(t *testing.T) { c := func(cluster string) context.Context { return icredentials.NewClientHandshakeInfoContext(context.Background(), credentials.ClientHandshakeInfo{ - Attributes: internal.SetXDSHandshakeClusterName(resolver.Address{}, cluster).Attributes, + Attributes: xds.SetXDSHandshakeClusterName(resolver.Address{}, cluster).Attributes, }) } diff --git a/internal/internal.go b/internal/internal.go index dec57b466cb1..48d24bdb4e69 100644 --- a/internal/internal.go +++ b/internal/internal.go @@ -199,7 +199,7 @@ var ( FromOutgoingContextRaw any // func(context.Context) (metadata.MD, [][]string, bool) // UserSetDefaultScheme is set to true if the user has overridden the default resolver scheme. - UserSetDefaultScheme bool + UserSetDefaultScheme bool = false ) // HealthChecker defines the signature of the client-side LB channel health checking function. diff --git a/internal/xds_handshake_cluster.go b/internal/xds/xds.go similarity index 98% rename from internal/xds_handshake_cluster.go rename to internal/xds/xds.go index e8b492774d1a..b28f376b5033 100644 --- a/internal/xds_handshake_cluster.go +++ b/internal/xds/xds.go @@ -14,7 +14,7 @@ * limitations under the License. */ -package internal +package xds import ( "google.golang.org/grpc/attributes" diff --git a/resolver/resolver.go b/resolver/resolver.go index 9ca7ce2542bc..55f18f7fcdce 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -23,6 +23,7 @@ package resolver import ( "context" "fmt" + "google.golang.org/grpc/internal" "net" "net/url" "strings" @@ -36,7 +37,7 @@ var ( // m is a map from scheme to resolver builder. m = make(map[string]Builder) // defaultScheme is the default scheme to use. - defaultScheme = "dns" + defaultScheme = "passthrough" ) // TODO(bar) install dns resolver in init(){}. @@ -63,17 +64,18 @@ func Get(scheme string) Builder { } // SetDefaultScheme sets the default scheme that will be used. The default -// scheme is "dns". +// scheme is "passthrough". // // NOTE: this function must only be called during initialization time (i.e. in // an init() function), and is not thread-safe. The scheme set last overrides // previously set values. func SetDefaultScheme(scheme string) { defaultScheme = scheme + internal.UserSetDefaultScheme = true } -// GetDefaultScheme gets the default scheme that will be used, if no scheme was -// overridden, defaults to the "dns" scheme. +// GetDefaultScheme gets the default scheme that will be used by grpc.Dial. If +// SetDefaultScheme is never called, the default scheme used by grpc.NewClient is "dns" instead. func GetDefaultScheme() string { return defaultScheme } diff --git a/xds/internal/balancer/clusterimpl/balancer_test.go b/xds/internal/balancer/clusterimpl/balancer_test.go index 1edf3b8b857a..4861f55083db 100644 --- a/xds/internal/balancer/clusterimpl/balancer_test.go +++ b/xds/internal/balancer/clusterimpl/balancer_test.go @@ -22,6 +22,7 @@ import ( "context" "errors" "fmt" + "google.golang.org/grpc/internal/xds" "strings" "testing" "time" @@ -32,7 +33,6 @@ import ( "google.golang.org/grpc/balancer/base" "google.golang.org/grpc/balancer/roundrobin" "google.golang.org/grpc/connectivity" - "google.golang.org/grpc/internal" "google.golang.org/grpc/internal/balancer/stub" "google.golang.org/grpc/internal/grpctest" internalserviceconfig "google.golang.org/grpc/internal/serviceconfig" @@ -464,7 +464,7 @@ func (s) TestClusterNameInAddressAttributes(t *testing.T) { if got, want := addrs1[0].Addr, testBackendAddrs[0].Addr; got != want { t.Fatalf("sc is created with addr %v, want %v", got, want) } - cn, ok := internal.GetXDSHandshakeClusterName(addrs1[0].Attributes) + cn, ok := xds.GetXDSHandshakeClusterName(addrs1[0].Attributes) if !ok || cn != testClusterName { t.Fatalf("sc is created with addr with cluster name %v, %v, want cluster name %v", cn, ok, testClusterName) } @@ -495,7 +495,7 @@ func (s) TestClusterNameInAddressAttributes(t *testing.T) { t.Fatalf("sc is created with addr %v, want %v", got, want) } // New addresses should have the new cluster name. - cn2, ok := internal.GetXDSHandshakeClusterName(addrs2[0].Attributes) + cn2, ok := xds.GetXDSHandshakeClusterName(addrs2[0].Attributes) if !ok || cn2 != testClusterName2 { t.Fatalf("sc is created with addr with cluster name %v, %v, want cluster name %v", cn2, ok, testClusterName2) } diff --git a/xds/internal/balancer/clusterimpl/clusterimpl.go b/xds/internal/balancer/clusterimpl/clusterimpl.go index 407d2deff7d6..d4bc4d879228 100644 --- a/xds/internal/balancer/clusterimpl/clusterimpl.go +++ b/xds/internal/balancer/clusterimpl/clusterimpl.go @@ -26,12 +26,12 @@ package clusterimpl import ( "encoding/json" "fmt" + "google.golang.org/grpc/internal/xds" "sync" "sync/atomic" "google.golang.org/grpc/balancer" "google.golang.org/grpc/connectivity" - "google.golang.org/grpc/internal" "google.golang.org/grpc/internal/balancer/gracefulswitch" "google.golang.org/grpc/internal/buffer" "google.golang.org/grpc/internal/grpclog" @@ -359,7 +359,7 @@ func (b *clusterImplBalancer) NewSubConn(addrs []resolver.Address, opts balancer newAddrs := make([]resolver.Address, len(addrs)) var lID xdsinternal.LocalityID for i, addr := range addrs { - newAddrs[i] = internal.SetXDSHandshakeClusterName(addr, clusterName) + newAddrs[i] = xds.SetXDSHandshakeClusterName(addr, clusterName) lID = xdsinternal.GetLocalityID(newAddrs[i]) } var sc balancer.SubConn @@ -384,7 +384,7 @@ func (b *clusterImplBalancer) UpdateAddresses(sc balancer.SubConn, addrs []resol newAddrs := make([]resolver.Address, len(addrs)) var lID xdsinternal.LocalityID for i, addr := range addrs { - newAddrs[i] = internal.SetXDSHandshakeClusterName(addr, clusterName) + newAddrs[i] = xds.SetXDSHandshakeClusterName(addr, clusterName) lID = xdsinternal.GetLocalityID(newAddrs[i]) } if scw, ok := sc.(*scWrapper); ok { From 87a7cb711302d39c6314be1a6f94edebef5aecae Mon Sep 17 00:00:00 2001 From: Matt Straathof Date: Wed, 6 Mar 2024 13:29:45 -0800 Subject: [PATCH 10/14] chore: run make deps --- go.sum | 1 + 1 file changed, 1 insertion(+) diff --git a/go.sum b/go.sum index 59f64c89528c..0b15ea9668a9 100644 --- a/go.sum +++ b/go.sum @@ -25,6 +25,7 @@ github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeN github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= From d405471f11ec5510bc9369647b901a50f51c5270 Mon Sep 17 00:00:00 2001 From: Matt Straathof Date: Wed, 6 Mar 2024 13:58:18 -0800 Subject: [PATCH 11/14] chore: fix import ordering to fix build --- credentials/google/google_test.go | 2 +- credentials/google/xds_test.go | 2 +- go.sum | 1 - resolver/resolver.go | 2 +- xds/internal/balancer/clusterimpl/balancer_test.go | 2 +- xds/internal/balancer/clusterimpl/clusterimpl.go | 2 +- 6 files changed, 5 insertions(+), 6 deletions(-) diff --git a/credentials/google/google_test.go b/credentials/google/google_test.go index 51da7f6b2d41..12c151ba5428 100644 --- a/credentials/google/google_test.go +++ b/credentials/google/google_test.go @@ -20,13 +20,13 @@ package google import ( "context" - "google.golang.org/grpc/internal/xds" "net" "testing" "google.golang.org/grpc/credentials" icredentials "google.golang.org/grpc/internal/credentials" "google.golang.org/grpc/internal/grpctest" + "google.golang.org/grpc/internal/xds" "google.golang.org/grpc/resolver" ) diff --git a/credentials/google/xds_test.go b/credentials/google/xds_test.go index 9deb40be2739..b62e7a73bc1d 100644 --- a/credentials/google/xds_test.go +++ b/credentials/google/xds_test.go @@ -20,11 +20,11 @@ package google import ( "context" - "google.golang.org/grpc/internal/xds" "testing" "google.golang.org/grpc/credentials" icredentials "google.golang.org/grpc/internal/credentials" + "google.golang.org/grpc/internal/xds" "google.golang.org/grpc/resolver" ) diff --git a/go.sum b/go.sum index 0b15ea9668a9..59f64c89528c 100644 --- a/go.sum +++ b/go.sum @@ -25,7 +25,6 @@ github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeN github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= -github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= diff --git a/resolver/resolver.go b/resolver/resolver.go index 55f18f7fcdce..ccd84d5c9758 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -23,13 +23,13 @@ package resolver import ( "context" "fmt" - "google.golang.org/grpc/internal" "net" "net/url" "strings" "google.golang.org/grpc/attributes" "google.golang.org/grpc/credentials" + "google.golang.org/grpc/internal" "google.golang.org/grpc/serviceconfig" ) diff --git a/xds/internal/balancer/clusterimpl/balancer_test.go b/xds/internal/balancer/clusterimpl/balancer_test.go index 4861f55083db..d7221c32a81a 100644 --- a/xds/internal/balancer/clusterimpl/balancer_test.go +++ b/xds/internal/balancer/clusterimpl/balancer_test.go @@ -22,7 +22,6 @@ import ( "context" "errors" "fmt" - "google.golang.org/grpc/internal/xds" "strings" "testing" "time" @@ -37,6 +36,7 @@ import ( "google.golang.org/grpc/internal/grpctest" internalserviceconfig "google.golang.org/grpc/internal/serviceconfig" "google.golang.org/grpc/internal/testutils" + "google.golang.org/grpc/internal/xds" "google.golang.org/grpc/resolver" xdsinternal "google.golang.org/grpc/xds/internal" "google.golang.org/grpc/xds/internal/testutils/fakeclient" diff --git a/xds/internal/balancer/clusterimpl/clusterimpl.go b/xds/internal/balancer/clusterimpl/clusterimpl.go index d4bc4d879228..bee5d2c97a33 100644 --- a/xds/internal/balancer/clusterimpl/clusterimpl.go +++ b/xds/internal/balancer/clusterimpl/clusterimpl.go @@ -26,7 +26,6 @@ package clusterimpl import ( "encoding/json" "fmt" - "google.golang.org/grpc/internal/xds" "sync" "sync/atomic" @@ -37,6 +36,7 @@ import ( "google.golang.org/grpc/internal/grpclog" "google.golang.org/grpc/internal/grpcsync" "google.golang.org/grpc/internal/pretty" + "google.golang.org/grpc/internal/xds" "google.golang.org/grpc/resolver" "google.golang.org/grpc/serviceconfig" xdsinternal "google.golang.org/grpc/xds/internal" From bfcfd81ae3faeb8f16cfa8d0c8490d761abfb8cd Mon Sep 17 00:00:00 2001 From: Matt Straathof Date: Wed, 6 Mar 2024 14:02:05 -0800 Subject: [PATCH 12/14] chore: fix import ordering for credentials/google/xds.go --- credentials/google/xds.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/credentials/google/xds.go b/credentials/google/xds.go index 8793a6878cd1..cccb22271ee5 100644 --- a/credentials/google/xds.go +++ b/credentials/google/xds.go @@ -20,12 +20,12 @@ package google import ( "context" - "google.golang.org/grpc/internal/xds" "net" "net/url" "strings" "google.golang.org/grpc/credentials" + "google.golang.org/grpc/internal/xds" ) const cfeClusterNamePrefix = "google_cfe_" From 6ad501388d03b918969a94b485b2d61cf1e5bacc Mon Sep 17 00:00:00 2001 From: Matt Straathof Date: Wed, 6 Mar 2024 14:04:30 -0800 Subject: [PATCH 13/14] fix: add package comment to internal/xds/xds.go file --- internal/xds/xds.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/xds/xds.go b/internal/xds/xds.go index b28f376b5033..024c388b7aa7 100644 --- a/internal/xds/xds.go +++ b/internal/xds/xds.go @@ -14,6 +14,8 @@ * limitations under the License. */ +// Package xds contains methods to Get/Set handshake cluster names. It is separated +// out from the top level /internal package to avoid circular dependencies. package xds import ( From ed128dbbb9cc281d39c7c44c9b44d2ad2eedf8a2 Mon Sep 17 00:00:00 2001 From: Matt Straathof Date: Thu, 7 Mar 2024 09:56:36 -0800 Subject: [PATCH 14/14] chore: pr nits, clean up comments, reset resolver state for connection unit tests --- clientconn.go | 2 -- clientconn_parsed_target_test.go | 10 +++++++++- resolver/resolver.go | 2 +- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/clientconn.go b/clientconn.go index 7bef26f1b185..346fc0e14512 100644 --- a/clientconn.go +++ b/clientconn.go @@ -213,8 +213,6 @@ func NewClient(target string, opts ...DialOption) (conn *ClientConn, err error) // e.g. to use dns resolver, a "dns:///" prefix should be applied to the target. 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. - // The eagerConnect option is used to tell the ClientChannel that DialContext was called to make - // this channel, and hence tries to connect immediately. cc, err := newClient(target, "passthrough", opts...) if err != nil { return nil, err diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index 95ad64734152..abb80611eae4 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -28,8 +28,8 @@ import ( "github.com/google/go-cmp/cmp" "google.golang.org/grpc/credentials/insecure" + "google.golang.org/grpc/internal" "google.golang.org/grpc/internal/testutils" - "google.golang.org/grpc/resolver" ) @@ -37,7 +37,14 @@ func generateTarget(scheme string, target string) resolver.Target { return resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", scheme, target))} } +// This is here just in case another test calls the SetDefaultScheme method. +func resetInitialResolverState() { + resolver.SetDefaultScheme("passthrough") + internal.UserSetDefaultScheme = false +} + func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { + resetInitialResolverState() dialScheme := resolver.GetDefaultScheme() newClientScheme := "dns" tests := []struct { @@ -149,6 +156,7 @@ func (s) TestParsedTarget_Failure_WithoutCustomDialer(t *testing.T) { } func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { + resetInitialResolverState() defScheme := resolver.GetDefaultScheme() tests := []struct { target string diff --git a/resolver/resolver.go b/resolver/resolver.go index ccd84d5c9758..202854511b81 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -64,7 +64,7 @@ func Get(scheme string) Builder { } // SetDefaultScheme sets the default scheme that will be used. The default -// scheme is "passthrough". +// scheme is initially set to "passthrough". // // NOTE: this function must only be called during initialization time (i.e. in // an init() function), and is not thread-safe. The scheme set last overrides