From 002aa36eab8ad1ae64cd71772f42cbc0ab020366 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Mon, 4 Oct 2021 18:07:33 -0700 Subject: [PATCH] review comments --- clientconn.go | 89 +++++++++++++++++++++----------- clientconn_authority_test.go | 12 ++--- clientconn_parsed_target_test.go | 6 +-- resolver/resolver.go | 6 +-- 4 files changed, 71 insertions(+), 42 deletions(-) diff --git a/clientconn.go b/clientconn.go index 89f43edfc83a..52b514c0c70c 100644 --- a/clientconn.go +++ b/clientconn.go @@ -252,27 +252,8 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * if err != nil { return nil, err } - - // Determine the authority for the channel. For now, we will use the parsed - // endpoint from the user's dial target (with a leading "/" stripped), as - // the default value. We do have some special handling for targets with unix - // scheme and empty hostname (":port"). - // TODO: Define an optional interface on the resolver builder to return the - // authority given the user's dial target. - cc.authority = strings.TrimPrefix(cc.parsedTarget.Endpoint, "/") - if strings.HasPrefix(cc.authority, ":") { - cc.authority = "localhost" + cc.authority - } - if cc.parsedTarget.Scheme == "unix" || cc.parsedTarget.Scheme == "unix-abstract" { - cc.authority = "localhost" - } - // The only authority override supported at this point in time is the one - // using the WithAuthority dial option, and this trumps over any per-address - // overrides specified by the name resolver in the - // resolver.Address.ServerName field. - if cc.dopts.authority != "" { - cc.authority = cc.dopts.authority - } + cc.determineAuthority() + channelz.Infof(logger, cc.channelzID, "Channel authority set to %q", cc.authority) if cc.dopts.scChan != nil && !scSet { // Blocking wait for the initial service config. @@ -1630,16 +1611,18 @@ func (cc *ClientConn) connectionError() error { func (cc *ClientConn) parseTargetAndFindResolver() (resolver.Builder, error) { channelz.Infof(logger, cc.channelzID, "original dial target is: %q", cc.target) + + var rb resolver.Builder parsedTarget, err := parseTarget(cc.target) if err != nil { channelz.Infof(logger, cc.channelzID, "dial target %q parse failed: %v", cc.target, err) } else { channelz.Infof(logger, cc.channelzID, "parsed dial target is: %+v", parsedTarget) - } - rb := cc.getResolver(parsedTarget.Scheme) - if rb != nil { - cc.parsedTarget = parsedTarget - return rb, nil + rb = cc.getResolver(parsedTarget.Scheme) + if rb != nil { + cc.parsedTarget = parsedTarget + return rb, nil + } } // We are here because the user's dial target did not contain a scheme or @@ -1687,9 +1670,7 @@ func parseTarget(target string) (resolver.Target, error) { if endpoint == "" { endpoint = u.Opaque } - if strings.HasPrefix(target, fmt.Sprintf("%s://", u.Scheme)) { - endpoint = strings.TrimPrefix(endpoint, "/") - } + endpoint = strings.TrimPrefix(endpoint, "/") return resolver.Target{ Scheme: u.Scheme, Authority: u.Host, @@ -1697,3 +1678,53 @@ func parseTarget(target string) (resolver.Target, error) { ParsedURL: *u, }, nil } + +// Determine channel authority. The order of precedence is as follows: +// - creds' notion of server name for the authentication handshake +// - user specified authority override using `WithAuthority` dial option +// - endpoint from dial target of the form "scheme://[authority]/endpoint" +func (cc *ClientConn) determineAuthority() { + // Historically, we had two options for users to specify the serverName or + // authority for a channel. One was through the transport credentials + // (either in its constructor, or through the OverrideServerName() method). + // The other option (for cases where WithInsecure() dial option was used) + // was to use the WithAuthority() dial option. + // + // A few things have changed since: + // - `insecure` package with an implementation of the `TransportCredentials` + // interface for the insecure case + // - WithAuthority() dial option support for secure credentials + // + // Even though OverrideServerName() is now marked as deprecated, we will + // give it higher priority over WithAuthority() so as to not introduce a + // behavior change. + authorityFromCreds := "" + if creds := cc.dopts.copts.TransportCredentials; creds != nil && creds.Info().ServerName != "" { + authorityFromCreds = creds.Info().ServerName + } + authorityFromDialOption := "" + if cc.dopts.authority != "" { + authorityFromDialOption = cc.dopts.authority + } + if authorityFromCreds != "" && authorityFromCreds != authorityFromDialOption { + channelz.Warningf(logger, cc.channelzID, "ClientConn's authority from transport creds %q and dial option %q don't match. Will use the former.", authorityFromCreds, authorityFromDialOption) + } + + // TODO: Define an optional interface on the resolver builder to return the + // channel authority given the user's dial target. Once we have this, we can + // get rid of the special cases for unix and `:port`. + switch { + case authorityFromCreds != "": + cc.authority = authorityFromCreds + case authorityFromDialOption != "": + cc.authority = authorityFromDialOption + case strings.HasPrefix(cc.target, "unix:") || strings.HasPrefix(cc.target, "unix-abstract:"): + cc.authority = "localhost" + case strings.HasPrefix(cc.parsedTarget.Endpoint, ":"): + cc.authority = "localhost" + cc.parsedTarget.Endpoint + default: + // Use endpoint from "scheme://authority/endpoint" as the default + // authority for ClientConn. + cc.authority = cc.parsedTarget.Endpoint + } +} diff --git a/clientconn_authority_test.go b/clientconn_authority_test.go index 9a28fe0f8a8b..e1b79a6a8c34 100644 --- a/clientconn_authority_test.go +++ b/clientconn_authority_test.go @@ -47,12 +47,10 @@ func (s) TestClientConnAuthority(t *testing.T) { wantAuthority: "Non-Existent.Server:8080", }, { - name: "override-via-creds", - target: "Non-Existent.Server:8080", - opts: []DialOption{WithTransportCredentials(creds)}, - // Overriding authority from the transport credentials is not - // supported anymore. - wantAuthority: "Non-Existent.Server:8080", + name: "override-via-creds", + target: "Non-Existent.Server:8080", + opts: []DialOption{WithTransportCredentials(creds)}, + wantAuthority: serverNameOverride, }, { name: "override-via-WithAuthority", @@ -64,7 +62,7 @@ func (s) TestClientConnAuthority(t *testing.T) { name: "override-via-creds-and-WithAuthority", target: "Non-Existent.Server:8080", opts: []DialOption{WithTransportCredentials(creds), WithAuthority("authority-override")}, - wantAuthority: "authority-override", + wantAuthority: serverNameOverride, }, { name: "unix relative", diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index 45059f7c457a..423c9d7c53fe 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -69,7 +69,7 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { {target: "unix-abstract:a b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a b"}}, {target: "unix-abstract:a:b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a:b"}}, {target: "unix-abstract:a-b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a-b"}}, - {target: "unix-abstract:/ a///://::!@#$%25^&*()b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "/ a///://::!@"}}, + {target: "unix-abstract:/ a///://::!@#$%25^&*()b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: " a///://::!@"}}, {target: "unix-abstract:passthrough:abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "passthrough:abc"}}, {target: "unix-abstract:unix:///abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "unix:///abc"}}, {target: "unix-abstract:///a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a/b/c"}}, @@ -77,7 +77,7 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { {target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "unix:///a/b/c"}}, // Cases for `scheme:absolute-path`. - {target: "dns:/a/b/c", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "/a/b/c"}}, + {target: "dns:/a/b/c", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "a/b/c"}}, {target: "unregistered:/a/b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unregistered:/a/b/c"}}, } @@ -130,7 +130,7 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { }, { target: "unix:/a/b/c", - wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}, + wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c"}, wantDialerAddress: "unix:///a/b/c", }, { diff --git a/resolver/resolver.go b/resolver/resolver.go index 0ae636fd976a..3b0730e9ea37 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -217,11 +217,11 @@ type ClientConn interface { // Examples: // // - "dns://some_authority/foo.bar" -// Target{Scheme: "dns", Authority: "some_authority", Endpoint: "/foo.bar"} +// Target{Scheme: "dns", Authority: "some_authority", Endpoint: "foo.bar"} // - "foo.bar" -// Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "/foo.bar"} +// Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "foo.bar"} // - "unknown_scheme://authority/endpoint" -// Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "/unknown_scheme://authority/endpoint"} +// Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "unknown_scheme://authority/endpoint"} type Target struct { Scheme string Authority string