From fb12d0787468f7bb7ff7ef210812cd0a1775a3d9 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Fri, 24 Sep 2021 14:45:19 -0700 Subject: [PATCH 01/23] grpc: increase compliance with RFC 3986 while parsing the dial target --- balancer/grpclb/grpclb.go | 3 +- balancer/rls/internal/config.go | 8 +- clientconn.go | 140 ++++++++++++++----- clientconn_authority_test.go | 17 +-- clientconn_parsed_target_test.go | 66 ++++----- credentials/credentials.go | 14 +- dialoptions.go | 3 +- internal/grpcutil/grpcutil.go | 21 +++ internal/grpcutil/target.go | 89 ------------ internal/resolver/dns/dns_resolver.go | 2 +- internal/resolver/passthrough/passthrough.go | 7 +- internal/transport/http2_client.go | 20 ++- xds/internal/resolver/xds_resolver.go | 2 + xds/internal/xdsclient/v2/client_test.go | 2 +- 14 files changed, 209 insertions(+), 185 deletions(-) create mode 100644 internal/grpcutil/grpcutil.go delete mode 100644 internal/grpcutil/target.go diff --git a/balancer/grpclb/grpclb.go b/balancer/grpclb/grpclb.go index adf59611160..e10f1ac1d74 100644 --- a/balancer/grpclb/grpclb.go +++ b/balancer/grpclb/grpclb.go @@ -26,6 +26,7 @@ import ( "context" "errors" "fmt" + "strings" "sync" "time" @@ -135,7 +136,7 @@ func (b *lbBuilder) Build(cc balancer.ClientConn, opt balancer.BuildOptions) bal lb := &lbBalancer{ cc: newLBCacheClientConn(cc), - target: opt.Target.Endpoint, + target: strings.TrimPrefix(opt.Target.Endpoint, "/"), opt: opt, fallbackTimeout: b.fallbackTimeout, doneCh: make(chan struct{}), diff --git a/balancer/rls/internal/config.go b/balancer/rls/internal/config.go index a3deb8906c9..6fca18cd0b6 100644 --- a/balancer/rls/internal/config.go +++ b/balancer/rls/internal/config.go @@ -22,15 +22,16 @@ import ( "bytes" "encoding/json" "fmt" + "net/url" "time" "github.com/golang/protobuf/jsonpb" "github.com/golang/protobuf/ptypes" durationpb "github.com/golang/protobuf/ptypes/duration" + "google.golang.org/grpc/balancer" "google.golang.org/grpc/balancer/rls/internal/keys" rlspb "google.golang.org/grpc/balancer/rls/internal/proto/grpc_lookup_v1" - "google.golang.org/grpc/internal/grpcutil" "google.golang.org/grpc/resolver" "google.golang.org/grpc/serviceconfig" ) @@ -201,7 +202,10 @@ func (*rlsBB) ParseConfig(c json.RawMessage) (serviceconfig.LoadBalancingConfig, if lookupService == "" { return nil, fmt.Errorf("rls: empty lookup_service in service config {%+v}", string(c)) } - parsedTarget := grpcutil.ParseTarget(lookupService, false) + parsedTarget, err := url.Parse(lookupService) + if err != nil { + return nil, fmt.Errorf("rls: invalid target URI in lookup_service {%s}", lookupService) + } if parsedTarget.Scheme == "" { parsedTarget.Scheme = resolver.GetDefaultScheme() } diff --git a/clientconn.go b/clientconn.go index 34cc4c948db..5854783238c 100644 --- a/clientconn.go +++ b/clientconn.go @@ -23,6 +23,7 @@ import ( "errors" "fmt" "math" + "net/url" "reflect" "strings" "sync" @@ -37,7 +38,6 @@ import ( "google.golang.org/grpc/internal/backoff" "google.golang.org/grpc/internal/channelz" "google.golang.org/grpc/internal/grpcsync" - "google.golang.org/grpc/internal/grpcutil" iresolver "google.golang.org/grpc/internal/resolver" "google.golang.org/grpc/internal/transport" "google.golang.org/grpc/keepalive" @@ -248,37 +248,30 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * } // Determine the resolver to use. - cc.parsedTarget = grpcutil.ParseTarget(cc.target, cc.dopts.copts.Dialer != nil) - channelz.Infof(logger, cc.channelzID, "parsed scheme: %q", cc.parsedTarget.Scheme) - resolverBuilder := cc.getResolver(cc.parsedTarget.Scheme) - if resolverBuilder == nil { - // If resolver builder is still nil, the parsed target's scheme is - // not registered. Fallback to default resolver and set Endpoint to - // the original target. - channelz.Infof(logger, cc.channelzID, "scheme %q not registered, fallback to default scheme", cc.parsedTarget.Scheme) - cc.parsedTarget = resolver.Target{ - Scheme: resolver.GetDefaultScheme(), - Endpoint: target, - } - resolverBuilder = cc.getResolver(cc.parsedTarget.Scheme) - if resolverBuilder == nil { - return nil, fmt.Errorf("could not get resolver for default scheme: %q", cc.parsedTarget.Scheme) - } + resolverBuilder, err := cc.parseTargetAndFindResolver() + if err != nil { + return nil, err } - creds := cc.dopts.copts.TransportCredentials - if creds != nil && creds.Info().ServerName != "" { - cc.authority = creds.Info().ServerName - } else if cc.dopts.insecure && cc.dopts.authority != "" { - cc.authority = cc.dopts.authority - } else if strings.HasPrefix(cc.target, "unix:") || strings.HasPrefix(cc.target, "unix-abstract:") { + // 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 name resolver 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 == "unx-abstract" { cc.authority = "localhost" - } else if strings.HasPrefix(cc.parsedTarget.Endpoint, ":") { - cc.authority = "localhost" + cc.parsedTarget.Endpoint - } else { - // Use endpoint from "scheme://authority/endpoint" as the default - // authority for ClientConn. - cc.authority = cc.parsedTarget.Endpoint + } + // 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 } if cc.dopts.scChan != nil && !scSet { @@ -902,10 +895,7 @@ func (ac *addrConn) tryUpdateAddrs(addrs []resolver.Address) bool { // ac.state is Ready, try to find the connected address. var curAddrFound bool for _, a := range addrs { - // a.ServerName takes precedent over ClientConn authority, if present. - if a.ServerName == "" { - a.ServerName = ac.cc.authority - } + a.ServerName = ac.getServerName(a) if reflect.DeepEqual(ac.curAddr, a) { curAddrFound = true break @@ -919,6 +909,25 @@ func (ac *addrConn) tryUpdateAddrs(addrs []resolver.Address) bool { return curAddrFound } +// getServerName determines the serverName to be used in the connection +// handshake. The default value for the serverName is the authority on the +// ClientConn, which either comes from the user's dial target or through an +// authority override specified using the WithAuthority dial option. Name +// resolvers can specify a per-address override for the serverName through the +// resolver.Address.ServerName field which is used only if the WithAuthority +// dial option was not used. The rationale is that per-address authority +// overrides specified by the name resolver can represent a security risk, while +// an override specified by the user is more dependable since they probably know +// what they are doing. +func (ac *addrConn) getServerName(addr resolver.Address) string { + if ac.cc.dopts.authority == "" { + if addr.ServerName != "" { + return addr.ServerName + } + } + return ac.cc.authority +} + func getMethodConfig(sc *ServiceConfig, method string) MethodConfig { if sc == nil { return MethodConfig{} @@ -1275,11 +1284,7 @@ func (ac *addrConn) createTransport(addr resolver.Address, copts transport.Conne prefaceReceived := grpcsync.NewEvent() connClosed := grpcsync.NewEvent() - // addr.ServerName takes precedent over ClientConn authority, if present. - if addr.ServerName == "" { - addr.ServerName = ac.cc.authority - } - + addr.ServerName = ac.getServerName(addr) hctx, hcancel := context.WithCancel(ac.ctx) hcStarted := false // protected by ac.mu @@ -1621,3 +1626,62 @@ func (cc *ClientConn) connectionError() error { defer cc.lceMu.Unlock() return cc.lastConnectionError } + +func (cc *ClientConn) parseTargetAndFindResolver() (resolver.Builder, error) { + channelz.Infof(logger, cc.channelzID, "original dial target is: %q", cc.target) + 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 + } + + // 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 no scheme was specified in the user's dial target and + // a custom dialer was specified. In the latter case, we should always use + // passthrough scheme. + defScheme := resolver.GetDefaultScheme() + if parsedTarget.Scheme == "" && cc.dopts.copts.Dialer != nil { + defScheme = "passthrough" + } + channelz.Infof(logger, cc.channelzID, "fallback to scheme %q", defScheme) + canonicalTarget := defScheme + ":///" + cc.target + + parsedTarget, err = parseTarget(canonicalTarget) + if err != nil { + channelz.Infof(logger, cc.channelzID, "dial target %q parse failed: %v", canonicalTarget, err) + return nil, err + } + channelz.Infof(logger, cc.channelzID, "parsed dial target is: %+v", parsedTarget) + rb = cc.getResolver(parsedTarget.Scheme) + if rb == nil { + return nil, fmt.Errorf("could not get resolver for default scheme: %q", parsedTarget.Scheme) + } + cc.parsedTarget = parsedTarget + return rb, nil +} + +// parseTarget uses RFC 3986 semantics to parse the given target into a +// resolver.Target struct containing scheme, authority and endpoint. Query +// params are stripped from the endpoint. +func parseTarget(target string) (resolver.Target, error) { + u, err := url.Parse(target) + if err != nil { + return resolver.Target{}, err + } + path := u.Path + if path == "" { + path = u.Opaque + } + return resolver.Target{ + Scheme: u.Scheme, + Authority: u.Host, + Endpoint: path, + }, nil +} diff --git a/clientconn_authority_test.go b/clientconn_authority_test.go index 5cd705e2d4f..9a28fe0f8a8 100644 --- a/clientconn_authority_test.go +++ b/clientconn_authority_test.go @@ -47,10 +47,12 @@ func (s) TestClientConnAuthority(t *testing.T) { wantAuthority: "Non-Existent.Server:8080", }, { - name: "override-via-creds", - target: "Non-Existent.Server:8080", - opts: []DialOption{WithTransportCredentials(creds)}, - wantAuthority: serverNameOverride, + 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-WithAuthority", @@ -59,11 +61,10 @@ func (s) TestClientConnAuthority(t *testing.T) { wantAuthority: "authority-override", }, { - name: "override-via-creds-and-WithAuthority", - target: "Non-Existent.Server:8080", - // WithAuthority override works only for insecure creds. + name: "override-via-creds-and-WithAuthority", + target: "Non-Existent.Server:8080", opts: []DialOption{WithTransportCredentials(creds), WithAuthority("authority-override")}, - wantAuthority: serverNameOverride, + wantAuthority: "authority-override", }, { name: "unix relative", diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index fda06f9fa14..08a1f9ae530 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -35,45 +35,43 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { wantParsed resolver.Target }{ // No scheme is specified. - {target: "", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ""}}, - {target: "://", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://"}}, - {target: ":///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///"}}, - {target: "://a/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://a/"}}, - {target: ":///a", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///a"}}, - {target: "://a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://a/b"}}, - {target: "/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/"}}, - {target: "a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a/b"}}, - {target: "a//b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a//b"}}, - {target: "google.com", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com"}}, - {target: "google.com/?a=b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com/?a=b"}}, - {target: "/unix/socket/address", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/unix/socket/address"}}, + {target: "", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/"}}, + {target: "://", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://"}}, + {target: ":///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///"}}, + {target: "://a/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://a/"}}, + {target: ":///a", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///a"}}, + {target: "://a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://a/b"}}, + {target: "/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "//"}}, + {target: "a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a/b"}}, + {target: "a//b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a//b"}}, + {target: "google.com", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/google.com"}}, + {target: "google.com/?a=b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/google.com/"}}, + {target: "/unix/socket/address", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "//unix/socket/address"}}, // An unregistered scheme is specified. - {target: "a:///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:///"}}, - {target: "a://b/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b/"}}, - {target: "a:///b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:///b"}}, - {target: "a://b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b/c"}}, - {target: "a:b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:b"}}, - {target: "a:/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:/b"}}, - {target: "a://b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b"}}, + {target: "a:///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:///"}}, + {target: "a://b/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a://b/"}}, + {target: "a:///b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:///b"}}, + {target: "a://b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a://b/c"}}, + {target: "a:b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:b"}}, + {target: "a:/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:/b"}}, + {target: "a://b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a://b"}}, // A registered scheme is specified. - {target: "dns:///google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "google.com"}}, - {target: "dns://a.server.com/google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "google.com"}}, - {target: "dns://a.server.com/google.com/?a=b", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "google.com/?a=b"}}, + {target: "dns:///google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "/google.com"}}, + {target: "dns://a.server.com/google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "/google.com"}}, + {target: "dns://a.server.com/google.com/?a=b", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "/google.com/"}}, {target: "unix:///a/b/c", wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}}, {target: "unix-abstract:a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a/b/c"}}, {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///://::!@#$%^&*()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: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"}}, {target: "unix-abstract:///", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "/"}}, - {target: "unix-abstract://authority", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "//authority"}}, - {target: "unix://domain", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unix://domain"}}, - {target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "unix:///a/b/c"}}, + {target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "/unix:///a/b/c"}}, } for _, test := range tests { @@ -94,7 +92,9 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { func (s) TestParsedTarget_Failure_WithoutCustomDialer(t *testing.T) { targets := []string{ "unix://a/b/c", + "unix://authority", "unix-abstract://authority/a/b/c", + "unix-abstract://authority", } for _, target := range targets { @@ -118,13 +118,13 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { // different behaviors with a custom dialer. { target: "unix:a/b/c", - wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unix:a/b/c"}, + wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c"}, wantDialerAddress: "unix:a/b/c", }, { target: "unix:/a/b/c", - wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unix:/a/b/c"}, - wantDialerAddress: "unix:/a/b/c", + wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}, + wantDialerAddress: "unix:///a/b/c", }, { target: "unix:///a/b/c", @@ -133,22 +133,22 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { }, { target: "dns:///127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "/127.0.0.1:50051"}, wantDialerAddress: "127.0.0.1:50051", }, { target: ":///127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///127.0.0.1:50051"}, wantDialerAddress: ":///127.0.0.1:50051", }, { target: "dns://authority/127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: "dns", Authority: "authority", Endpoint: "127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: "dns", Authority: "authority", Endpoint: "/127.0.0.1:50051"}, wantDialerAddress: "127.0.0.1:50051", }, { target: "://authority/127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://authority/127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://authority/127.0.0.1:50051"}, wantDialerAddress: "://authority/127.0.0.1:50051", }, } diff --git a/credentials/credentials.go b/credentials/credentials.go index 7eee7e4ec12..6db99da46d9 100644 --- a/credentials/credentials.go +++ b/credentials/credentials.go @@ -140,6 +140,12 @@ type TransportCredentials interface { // Additionally, ClientHandshakeInfo data will be available via the context // passed to this call. // + // The second argument to this method is the ClientConn's authority, usually + // derived from the user's dial target. Implementations should use this as + // the server name during the authentication handshake. In rare cases, + // implementations may choose to ignore this value and use a value acquired + // through other means. + // // If the returned net.Conn is closed, it MUST close the net.Conn provided. ClientHandshake(context.Context, string, net.Conn) (net.Conn, AuthInfo, error) // ServerHandshake does the authentication handshake for servers. It returns @@ -153,9 +159,11 @@ type TransportCredentials interface { Info() ProtocolInfo // Clone makes a copy of this TransportCredentials. Clone() TransportCredentials - // OverrideServerName overrides the server name used to verify the hostname on the returned certificates from the server. - // gRPC internals also use it to override the virtual hosting name if it is set. - // It must be called before dialing. Currently, this is only used by grpclb. + // OverrideServerName overrides the server name used to verify the hostname + // on the returned certificates from the server. + // + // This is deprecated. Users are recommended to use the WithAuthority dial + // option instead. OverrideServerName(string) error } diff --git a/dialoptions.go b/dialoptions.go index 7a497237bbd..5f7b7a164ce 100644 --- a/dialoptions.go +++ b/dialoptions.go @@ -482,8 +482,7 @@ func WithChainStreamInterceptor(interceptors ...StreamClientInterceptor) DialOpt } // WithAuthority returns a DialOption that specifies the value to be used as the -// :authority pseudo-header. This value only works with WithInsecure and has no -// effect if TransportCredentials are present. +// :authority pseudo-header and as the server name in authentication handshake. func WithAuthority(a string) DialOption { return newFuncDialOption(func(o *dialOptions) { o.authority = a diff --git a/internal/grpcutil/grpcutil.go b/internal/grpcutil/grpcutil.go new file mode 100644 index 00000000000..ff0ace320ee --- /dev/null +++ b/internal/grpcutil/grpcutil.go @@ -0,0 +1,21 @@ +/* + * + * Copyright 2021 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +// Package grpcutil provides a bunch of utility functions to be used across the +// gRPC codebase. +package grpcutil diff --git a/internal/grpcutil/target.go b/internal/grpcutil/target.go deleted file mode 100644 index 8833021da02..00000000000 --- a/internal/grpcutil/target.go +++ /dev/null @@ -1,89 +0,0 @@ -/* - * - * Copyright 2020 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -// Package grpcutil provides a bunch of utility functions to be used across the -// gRPC codebase. -package grpcutil - -import ( - "strings" - - "google.golang.org/grpc/resolver" -) - -// split2 returns the values from strings.SplitN(s, sep, 2). -// If sep is not found, it returns ("", "", false) instead. -func split2(s, sep string) (string, string, bool) { - spl := strings.SplitN(s, sep, 2) - if len(spl) < 2 { - return "", "", false - } - return spl[0], spl[1], true -} - -// ParseTarget splits target into a resolver.Target struct containing scheme, -// authority and endpoint. skipUnixColonParsing indicates that the parse should -// not parse "unix:[path]" cases. This should be true in cases where a custom -// dialer is present, to prevent a behavior change. -// -// If target is not a valid scheme://authority/endpoint as specified in -// https://github.com/grpc/grpc/blob/master/doc/naming.md, -// it returns {Endpoint: target}. -func ParseTarget(target string, skipUnixColonParsing bool) (ret resolver.Target) { - var ok bool - if strings.HasPrefix(target, "unix-abstract:") { - if strings.HasPrefix(target, "unix-abstract://") { - // Maybe, with Authority specified, try to parse it - var remain string - ret.Scheme, remain, _ = split2(target, "://") - ret.Authority, ret.Endpoint, ok = split2(remain, "/") - if !ok { - // No Authority, add the "//" back - ret.Endpoint = "//" + remain - } else { - // Found Authority, add the "/" back - ret.Endpoint = "/" + ret.Endpoint - } - } else { - // Without Authority specified, split target on ":" - ret.Scheme, ret.Endpoint, _ = split2(target, ":") - } - return ret - } - ret.Scheme, ret.Endpoint, ok = split2(target, "://") - if !ok { - if strings.HasPrefix(target, "unix:") && !skipUnixColonParsing { - // Handle the "unix:[local/path]" and "unix:[/absolute/path]" cases, - // because splitting on :// only handles the - // "unix://[/absolute/path]" case. Only handle if the dialer is nil, - // to avoid a behavior change with custom dialers. - return resolver.Target{Scheme: "unix", Endpoint: target[len("unix:"):]} - } - return resolver.Target{Endpoint: target} - } - ret.Authority, ret.Endpoint, ok = split2(ret.Endpoint, "/") - if !ok { - return resolver.Target{Endpoint: target} - } - if ret.Scheme == "unix" { - // Add the "/" back in the unix case, so the unix resolver receives the - // actual endpoint in the "unix://[/absolute/path]" case. - ret.Endpoint = "/" + ret.Endpoint - } - return ret -} diff --git a/internal/resolver/dns/dns_resolver.go b/internal/resolver/dns/dns_resolver.go index 75301c51491..82fad8600fd 100644 --- a/internal/resolver/dns/dns_resolver.go +++ b/internal/resolver/dns/dns_resolver.go @@ -116,7 +116,7 @@ type dnsBuilder struct{} // Build creates and starts a DNS resolver that watches the name resolution of the target. func (b *dnsBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { - host, port, err := parseTarget(target.Endpoint, defaultPort) + host, port, err := parseTarget(strings.TrimPrefix(target.Endpoint, "/"), defaultPort) if err != nil { return nil, err } diff --git a/internal/resolver/passthrough/passthrough.go b/internal/resolver/passthrough/passthrough.go index 520d9229e1e..1f1c4eaeb5e 100644 --- a/internal/resolver/passthrough/passthrough.go +++ b/internal/resolver/passthrough/passthrough.go @@ -20,13 +20,18 @@ // name without scheme back to gRPC as resolved address. package passthrough -import "google.golang.org/grpc/resolver" +import ( + "strings" + + "google.golang.org/grpc/resolver" +) const scheme = "passthrough" type passthroughBuilder struct{} func (*passthroughBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { + target.Endpoint = strings.TrimPrefix(target.Endpoint, "/") r := &passthroughResolver{ target: target, cc: cc, diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index ea3babb118b..d1c2fa0a89f 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -25,6 +25,7 @@ import ( "math" "net" "net/http" + "path/filepath" "strconv" "strings" "sync" @@ -146,13 +147,20 @@ func dial(ctx context.Context, fn func(context.Context, string) (net.Conn, error address := addr.Addr networkType, ok := networktype.Get(addr) if fn != nil { + // Special handling for unix scheme with custom dialer. Back in the day, + // we did not have a unix resolver and therefore targets with a unix + // scheme would end up using the passthrough resolver. So, user's used a + // custom dialer in this case and expected the original dial target to + // be passed to the custom dialer. Now, we have a unix resolver. But if + // a custom dialer is specified, we want to retain the old behavior in + // terms of the address being passed to the custom dialer. if networkType == "unix" && !strings.HasPrefix(address, "\x00") { - // For backward compatibility, if the user dialed "unix:///path", - // the passthrough resolver would be used and the user's custom - // dialer would see "unix:///path". Since the unix resolver is used - // and the address is now "/path", prepend "unix://" so the user's - // custom dialer sees the same address. - return fn(ctx, "unix://"+address) + // Supported unix targets are either "unix://absolute-path" or + // "unix:relative-path". + if filepath.IsAbs(address) { + return fn(ctx, "unix://"+address) + } + return fn(ctx, "unix:"+address) } return fn(ctx, address) } diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index 19ee01773e8..d4b29503627 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -22,6 +22,7 @@ package resolver import ( "errors" "fmt" + "strings" "google.golang.org/grpc/credentials" "google.golang.org/grpc/internal/grpclog" @@ -61,6 +62,7 @@ type xdsResolverBuilder struct { // The xds bootstrap process is performed (and a new xds client is built) every // time an xds resolver is built. func (b *xdsResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { + t.Endpoint = strings.TrimPrefix(t.Endpoint, "/") r := &xdsResolver{ target: t, cc: cc, diff --git a/xds/internal/xdsclient/v2/client_test.go b/xds/internal/xdsclient/v2/client_test.go index ed4322b0dc5..fc3fa821a15 100644 --- a/xds/internal/xdsclient/v2/client_test.go +++ b/xds/internal/xdsclient/v2/client_test.go @@ -608,7 +608,7 @@ func (s) TestV2ClientWatchWithoutStream(t *testing.T) { } defer sCleanup() - const scheme = "xds_client_test_whatever" + const scheme = "xds-client-test-whatever" rb := manual.NewBuilderWithScheme(scheme) rb.InitialState(resolver.State{Addresses: []resolver.Address{{Addr: "no.such.server"}}}) From cee1b4442c7464b613a74b4e138196a64f1ed0f6 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Mon, 27 Sep 2021 08:46:10 -0700 Subject: [PATCH 02/23] fix resolver in load_balancing example --- examples/features/load_balancing/client/main.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/examples/features/load_balancing/client/main.go b/examples/features/load_balancing/client/main.go index 1578df16671..31935278849 100644 --- a/examples/features/load_balancing/client/main.go +++ b/examples/features/load_balancing/client/main.go @@ -23,6 +23,7 @@ import ( "context" "fmt" "log" + "strings" "time" "google.golang.org/grpc" @@ -93,6 +94,7 @@ func main() { type exampleResolverBuilder struct{} func (*exampleResolverBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { + target.Endpoint = strings.TrimPrefix(target.Endpoint, "/") r := &exampleResolver{ target: target, cc: cc, From d19cd9bb51d64cbec219c87aaae0adc72b6fd927 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Mon, 27 Sep 2021 08:58:29 -0700 Subject: [PATCH 03/23] fix resolver in name_resolving example --- examples/features/name_resolving/client/main.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/examples/features/name_resolving/client/main.go b/examples/features/name_resolving/client/main.go index 1c56dcce15d..3bb285417d1 100644 --- a/examples/features/name_resolving/client/main.go +++ b/examples/features/name_resolving/client/main.go @@ -23,6 +23,7 @@ import ( "context" "fmt" "log" + "strings" "time" "google.golang.org/grpc" @@ -99,6 +100,7 @@ func main() { type exampleResolverBuilder struct{} func (*exampleResolverBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { + target.Endpoint = strings.TrimPrefix(target.Endpoint, "/") r := &exampleResolver{ target: target, cc: cc, From 0d0c1a5d083a8f88ac7445c2abbfd9ff4b95dac3 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Mon, 27 Sep 2021 09:28:12 -0700 Subject: [PATCH 04/23] fix typo --- clientconn.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clientconn.go b/clientconn.go index 5854783238c..b31496333e4 100644 --- a/clientconn.go +++ b/clientconn.go @@ -263,7 +263,7 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * if strings.HasPrefix(cc.authority, ":") { cc.authority = "localhost" + cc.authority } - if cc.parsedTarget.Scheme == "unix" || cc.parsedTarget.Scheme == "unx-abstract" { + 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 From c98608a91220791a14d9d912399725f28e9dd84c Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Mon, 27 Sep 2021 09:40:36 -0700 Subject: [PATCH 05/23] make unix target canonical in test --- test/authority_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/authority_test.go b/test/authority_test.go index 15afa759c90..0f823bdbd1b 100644 --- a/test/authority_test.go +++ b/test/authority_test.go @@ -103,10 +103,11 @@ var authorityTests = []authorityTest{ authority: "localhost", }, { - name: "UnixAbsolute", - address: "/tmp/sock.sock", - target: "unix:/tmp/sock.sock", - authority: "localhost", + name: "UnixAbsolute", + address: "/tmp/sock.sock", + target: "unix:/tmp/sock.sock", + authority: "localhost", + dialTargetWant: "unix:///tmp/sock.sock", }, { name: "UnixAbsoluteAlternate", From fb4b9c0078dc61e82e3e6687a77e269ab2f70b2a Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Wed, 29 Sep 2021 14:07:20 -0700 Subject: [PATCH 06/23] add `Unparsed` field to `resolver.Target` --- clientconn.go | 1 + clientconn_parsed_target_test.go | 80 ++++++++++++++++---------------- resolver/resolver.go | 30 +++++++----- 3 files changed, 59 insertions(+), 52 deletions(-) diff --git a/clientconn.go b/clientconn.go index b31496333e4..9229f4a98ce 100644 --- a/clientconn.go +++ b/clientconn.go @@ -1629,6 +1629,7 @@ func (cc *ClientConn) connectionError() error { func (cc *ClientConn) parseTargetAndFindResolver() (resolver.Builder, error) { channelz.Infof(logger, cc.channelzID, "original dial target is: %q", cc.target) + defer func() { cc.parsedTarget.Unparsed = cc.target }() parsedTarget, err := parseTarget(cc.target) if err != nil { channelz.Infof(logger, cc.channelzID, "dial target %q parse failed: %v", cc.target, err) diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index 08a1f9ae530..e7c01a8c88f 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -35,43 +35,43 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { wantParsed resolver.Target }{ // No scheme is specified. - {target: "", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/"}}, - {target: "://", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://"}}, - {target: ":///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///"}}, - {target: "://a/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://a/"}}, - {target: ":///a", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///a"}}, - {target: "://a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://a/b"}}, - {target: "/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "//"}}, - {target: "a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a/b"}}, - {target: "a//b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a//b"}}, - {target: "google.com", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/google.com"}}, - {target: "google.com/?a=b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/google.com/"}}, - {target: "/unix/socket/address", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "//unix/socket/address"}}, + {target: "", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/", Unparsed: ""}}, + {target: "://", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://", Unparsed: "://"}}, + {target: ":///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///", Unparsed: ":///"}}, + {target: "://a/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://a/", Unparsed: "://a/"}}, + {target: ":///a", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///a", Unparsed: ":///a"}}, + {target: "://a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://a/b", Unparsed: "://a/b"}}, + {target: "/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "//", Unparsed: "/"}}, + {target: "a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a/b", Unparsed: "a/b"}}, + {target: "a//b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a//b", Unparsed: "a//b"}}, + {target: "google.com", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/google.com", Unparsed: "google.com"}}, + {target: "google.com/?a=b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/google.com/", Unparsed: "google.com/?a=b"}}, + {target: "/unix/socket/address", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "//unix/socket/address", Unparsed: "/unix/socket/address"}}, // An unregistered scheme is specified. - {target: "a:///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:///"}}, - {target: "a://b/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a://b/"}}, - {target: "a:///b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:///b"}}, - {target: "a://b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a://b/c"}}, - {target: "a:b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:b"}}, - {target: "a:/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:/b"}}, - {target: "a://b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a://b"}}, + {target: "a:///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:///", Unparsed: "a:///"}}, + {target: "a://b/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a://b/", Unparsed: "a://b/"}}, + {target: "a:///b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:///b", Unparsed: "a:///b"}}, + {target: "a://b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a://b/c", Unparsed: "a://b/c"}}, + {target: "a:b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:b", Unparsed: "a:b"}}, + {target: "a:/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:/b", Unparsed: "a:/b"}}, + {target: "a://b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a://b", Unparsed: "a://b"}}, // A registered scheme is specified. - {target: "dns:///google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "/google.com"}}, - {target: "dns://a.server.com/google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "/google.com"}}, - {target: "dns://a.server.com/google.com/?a=b", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "/google.com/"}}, - {target: "unix:///a/b/c", wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}}, - {target: "unix-abstract:a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a/b/c"}}, - {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: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"}}, - {target: "unix-abstract:///", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "/"}}, - {target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "/unix:///a/b/c"}}, + {target: "dns:///google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "/google.com", Unparsed: "dns:///google.com"}}, + {target: "dns://a.server.com/google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "/google.com", Unparsed: "dns://a.server.com/google.com"}}, + {target: "dns://a.server.com/google.com/?a=b", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "/google.com/", Unparsed: "dns://a.server.com/google.com/?a=b"}}, + {target: "unix:///a/b/c", wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c", Unparsed: "unix:///a/b/c"}}, + {target: "unix-abstract:a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a/b/c", Unparsed: "unix-abstract:a/b/c"}}, + {target: "unix-abstract:a b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a b", Unparsed: "unix-abstract:a b"}}, + {target: "unix-abstract:a:b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a:b", Unparsed: "unix-abstract:a:b"}}, + {target: "unix-abstract:a-b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a-b", Unparsed: "unix-abstract:a-b"}}, + {target: "unix-abstract:/ a///://::!@#$%25^&*()b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "/ a///://::!@", Unparsed: "unix-abstract:/ a///://::!@#$%25^&*()b"}}, + {target: "unix-abstract:passthrough:abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "passthrough:abc", Unparsed: "unix-abstract:passthrough:abc"}}, + {target: "unix-abstract:unix:///abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "unix:///abc", Unparsed: "unix-abstract:unix:///abc"}}, + {target: "unix-abstract:///a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "/a/b/c", Unparsed: "unix-abstract:///a/b/c"}}, + {target: "unix-abstract:///", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "/", Unparsed: "unix-abstract:///"}}, + {target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "/unix:///a/b/c", Unparsed: "passthrough:///unix:///a/b/c"}}, } for _, test := range tests { @@ -118,37 +118,37 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { // different behaviors with a custom dialer. { 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", Unparsed: "unix:a/b/c"}, wantDialerAddress: "unix:a/b/c", }, { 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", Unparsed: "unix:/a/b/c"}, wantDialerAddress: "unix:///a/b/c", }, { 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", Unparsed: "unix:///a/b/c"}, wantDialerAddress: "unix:///a/b/c", }, { target: "dns:///127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "/127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "/127.0.0.1:50051", Unparsed: "dns:///127.0.0.1:50051"}, wantDialerAddress: "127.0.0.1:50051", }, { target: ":///127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///127.0.0.1:50051", Unparsed: ":///127.0.0.1:50051"}, wantDialerAddress: ":///127.0.0.1:50051", }, { target: "dns://authority/127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: "dns", Authority: "authority", Endpoint: "/127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: "dns", Authority: "authority", Endpoint: "/127.0.0.1:50051", Unparsed: "dns://authority/127.0.0.1:50051"}, wantDialerAddress: "127.0.0.1:50051", }, { target: "://authority/127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://authority/127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://authority/127.0.0.1:50051", Unparsed: "://authority/127.0.0.1:50051"}, wantDialerAddress: "://authority/127.0.0.1:50051", }, } diff --git a/resolver/resolver.go b/resolver/resolver.go index 6a9d234a597..8206d3263f9 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -204,25 +204,31 @@ type ClientConn interface { // Target represents a target for gRPC, as specified in: // https://github.com/grpc/grpc/blob/master/doc/naming.md. -// It is parsed from the target string that gets passed into Dial or DialContext by the user. And -// grpc passes it to the resolver and the balancer. +// It is parsed from the target string that gets passed into Dial or DialContext +// by the user. And gRPC passes it to the resolver and the balancer. // -// If the target follows the naming spec, and the parsed scheme is registered with grpc, we will -// parse the target string according to the spec. e.g. "dns://some_authority/foo.bar" will be parsed -// into &Target{Scheme: "dns", Authority: "some_authority", Endpoint: "foo.bar"} +// If the target follows the naming spec, and the parsed scheme is registered +// with gRPC, we will parse the target string according to the spec. If the +// target does not contain a scheme or if the parsed scheme is not registered +// (i.e. no corresponding resolver available to resolve the endpoint), we will +// apply the default scheme, and will attempt to reparse it. // -// If the target does not contain a scheme, we will apply the default scheme, and set the Target to -// be the full target string. e.g. "foo.bar" will be parsed into -// &Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "foo.bar"}. +// Examples: // -// If the parsed scheme is not registered (i.e. no corresponding resolver available to resolve the -// endpoint), we set the Scheme to be the default scheme, and set the Endpoint to be the full target -// string. e.g. target string "unknown_scheme://authority/endpoint" will be parsed into -// &Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "unknown_scheme://authority/endpoint"}. +// - "dns://some_authority/foo.bar" +// Target{Scheme: "dns", Authority: "some_authority", Endpoint: "/foo.bar"} +// - "foo.bar" +// Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "/foo.bar"} +// - "unknown_scheme://authority/endpoint" +// Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "/unknown_scheme://authority/endpoint"} +// +// Unparsed will be set to the user's original dial target. Any query params +// specified in the dial target can be accessed though this field. type Target struct { Scheme string Authority string Endpoint string + Unparsed string } // Builder creates a resolver that will be used to watch name resolution updates. From 2750421e6be8109970c76b02e77a9ee60c712e76 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Wed, 29 Sep 2021 14:31:50 -0700 Subject: [PATCH 07/23] make getServerName a method on cc instead of ac --- clientconn.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/clientconn.go b/clientconn.go index 9229f4a98ce..8cdc6780592 100644 --- a/clientconn.go +++ b/clientconn.go @@ -895,7 +895,7 @@ func (ac *addrConn) tryUpdateAddrs(addrs []resolver.Address) bool { // ac.state is Ready, try to find the connected address. var curAddrFound bool for _, a := range addrs { - a.ServerName = ac.getServerName(a) + a.ServerName = ac.cc.getServerName(a) if reflect.DeepEqual(ac.curAddr, a) { curAddrFound = true break @@ -919,13 +919,14 @@ func (ac *addrConn) tryUpdateAddrs(addrs []resolver.Address) bool { // overrides specified by the name resolver can represent a security risk, while // an override specified by the user is more dependable since they probably know // what they are doing. -func (ac *addrConn) getServerName(addr resolver.Address) string { - if ac.cc.dopts.authority == "" { - if addr.ServerName != "" { - return addr.ServerName - } +func (cc *ClientConn) getServerName(addr resolver.Address) string { + if cc.dopts.authority != "" { + return cc.dopts.authority + } + if addr.ServerName != "" { + return addr.ServerName } - return ac.cc.authority + return cc.authority } func getMethodConfig(sc *ServiceConfig, method string) MethodConfig { @@ -1284,7 +1285,7 @@ func (ac *addrConn) createTransport(addr resolver.Address, copts transport.Conne prefaceReceived := grpcsync.NewEvent() connClosed := grpcsync.NewEvent() - addr.ServerName = ac.getServerName(addr) + addr.ServerName = ac.cc.getServerName(addr) hctx, hcancel := context.WithCancel(ac.ctx) hcStarted := false // protected by ac.mu From c5227923751a7e58fec4ec4f9f98d49ee92714b8 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Wed, 29 Sep 2021 14:50:06 -0700 Subject: [PATCH 08/23] some comments --- credentials/credentials.go | 11 ++++++----- internal/grpcutil/grpcutil.go | 3 +-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/credentials/credentials.go b/credentials/credentials.go index 6db99da46d9..30a2b876f60 100644 --- a/credentials/credentials.go +++ b/credentials/credentials.go @@ -142,9 +142,10 @@ type TransportCredentials interface { // // The second argument to this method is the ClientConn's authority, usually // derived from the user's dial target. Implementations should use this as - // the server name during the authentication handshake. In rare cases, - // implementations may choose to ignore this value and use a value acquired - // through other means. + // the server name during the authentication handshake. Implementations may + // choose to ignore this value and use a value acquired through other means, + // in which case they must make sure that the value is acquired through + // secure means and that a possible attacker cannot tamper with the value. // // If the returned net.Conn is closed, it MUST close the net.Conn provided. ClientHandshake(context.Context, string, net.Conn) (net.Conn, AuthInfo, error) @@ -162,8 +163,8 @@ type TransportCredentials interface { // OverrideServerName overrides the server name used to verify the hostname // on the returned certificates from the server. // - // This is deprecated. Users are recommended to use the WithAuthority dial - // option instead. + // Deprecated: use grpc.WithAuthority instead. Will be supported + // throughout 1.x. OverrideServerName(string) error } diff --git a/internal/grpcutil/grpcutil.go b/internal/grpcutil/grpcutil.go index ff0ace320ee..e2f948e8f4f 100644 --- a/internal/grpcutil/grpcutil.go +++ b/internal/grpcutil/grpcutil.go @@ -16,6 +16,5 @@ * */ -// Package grpcutil provides a bunch of utility functions to be used across the -// gRPC codebase. +// Package grpcutil provides utility functions used across the gRPC codebase. package grpcutil From 4f56e2ea080c54b236b1ca94ada111672f554746 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Wed, 29 Sep 2021 15:00:58 -0700 Subject: [PATCH 09/23] use passthrough whenever custom dialer is used --- clientconn.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/clientconn.go b/clientconn.go index 8cdc6780592..bc1158487cb 100644 --- a/clientconn.go +++ b/clientconn.go @@ -1645,11 +1645,10 @@ func (cc *ClientConn) parseTargetAndFindResolver() (resolver.Builder, 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 no scheme was specified in the user's dial target and - // a custom dialer was specified. In the latter case, we should always use - // passthrough scheme. + // scheme, except when a custom dialer is specified in which case, we should + // always use passthrough scheme. defScheme := resolver.GetDefaultScheme() - if parsedTarget.Scheme == "" && cc.dopts.copts.Dialer != nil { + if cc.dopts.copts.Dialer != nil { defScheme = "passthrough" } channelz.Infof(logger, cc.channelzID, "fallback to scheme %q", defScheme) From ae490d04638384b65eb5eb38dd357cd2e6cb5bfe Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Wed, 29 Sep 2021 15:41:33 -0700 Subject: [PATCH 10/23] resolver builder --- clientconn.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clientconn.go b/clientconn.go index bc1158487cb..13cc8e152e9 100644 --- a/clientconn.go +++ b/clientconn.go @@ -257,7 +257,7 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * // 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 name resolver to return the + // 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, ":") { From 99fdc43d466f95720c9d2fc31dfbd1a17aca0d07 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Wed, 29 Sep 2021 17:00:07 -0700 Subject: [PATCH 11/23] strip leading "/" from endpoint for backward compatibility --- balancer/grpclb/grpclb.go | 3 +- clientconn.go | 21 +++- clientconn_parsed_target_test.go | 101 ++++++++++-------- .../features/load_balancing/client/main.go | 2 - .../features/name_resolving/client/main.go | 2 - internal/resolver/dns/dns_resolver.go | 2 +- internal/resolver/passthrough/passthrough.go | 3 - internal/resolver/unix/unix.go | 15 ++- resolver/resolver.go | 3 +- xds/internal/resolver/xds_resolver.go | 2 - 10 files changed, 91 insertions(+), 63 deletions(-) diff --git a/balancer/grpclb/grpclb.go b/balancer/grpclb/grpclb.go index e10f1ac1d74..adf59611160 100644 --- a/balancer/grpclb/grpclb.go +++ b/balancer/grpclb/grpclb.go @@ -26,7 +26,6 @@ import ( "context" "errors" "fmt" - "strings" "sync" "time" @@ -136,7 +135,7 @@ func (b *lbBuilder) Build(cc balancer.ClientConn, opt balancer.BuildOptions) bal lb := &lbBalancer{ cc: newLBCacheClientConn(cc), - target: strings.TrimPrefix(opt.Target.Endpoint, "/"), + target: opt.Target.Endpoint, opt: opt, fallbackTimeout: b.fallbackTimeout, doneCh: make(chan struct{}), diff --git a/clientconn.go b/clientconn.go index 13cc8e152e9..2dfffcdf3e9 100644 --- a/clientconn.go +++ b/clientconn.go @@ -1630,7 +1630,6 @@ func (cc *ClientConn) connectionError() error { func (cc *ClientConn) parseTargetAndFindResolver() (resolver.Builder, error) { channelz.Infof(logger, cc.channelzID, "original dial target is: %q", cc.target) - defer func() { cc.parsedTarget.Unparsed = cc.target }() parsedTarget, err := parseTarget(cc.target) if err != nil { channelz.Infof(logger, cc.channelzID, "dial target %q parse failed: %v", cc.target, err) @@ -1676,13 +1675,25 @@ func parseTarget(target string) (resolver.Target, error) { if err != nil { return resolver.Target{}, err } - path := u.Path - if path == "" { - path = u.Opaque + // For targets of the form "[scheme]://[authority]/endpoint, the endpoint + // value returned from url.Parse() contains a leading "/". Although this is + // in accordance with RFC 3986, we do not want to break existing resolver + // implementations which expect the endpoint without the leading "/". So, we + // end up stripping the leading "/" here. But this will result in an + // incorrect parsing for something like "unix:///path/to/socket". Since we + // own the "unix" resolver, we can workaround in the unix resolver by using + // the `ParsedURL` field instead of the `Endpoint` field. + endpoint := u.Path + if endpoint == "" { + endpoint = u.Opaque + } + if strings.HasPrefix(target, fmt.Sprintf("%s://", u.Scheme)) { + endpoint = strings.TrimPrefix(endpoint, "/") } return resolver.Target{ Scheme: u.Scheme, Authority: u.Host, - Endpoint: path, + Endpoint: endpoint, + ParsedURL: u, }, nil } diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index e7c01a8c88f..5b94178afa4 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -25,6 +25,9 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "google.golang.org/grpc/resolver" ) @@ -35,43 +38,43 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { wantParsed resolver.Target }{ // No scheme is specified. - {target: "", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/", Unparsed: ""}}, - {target: "://", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://", Unparsed: "://"}}, - {target: ":///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///", Unparsed: ":///"}}, - {target: "://a/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://a/", Unparsed: "://a/"}}, - {target: ":///a", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///a", Unparsed: ":///a"}}, - {target: "://a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://a/b", Unparsed: "://a/b"}}, - {target: "/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "//", Unparsed: "/"}}, - {target: "a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a/b", Unparsed: "a/b"}}, - {target: "a//b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a//b", Unparsed: "a//b"}}, - {target: "google.com", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/google.com", Unparsed: "google.com"}}, - {target: "google.com/?a=b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/google.com/", Unparsed: "google.com/?a=b"}}, - {target: "/unix/socket/address", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "//unix/socket/address", Unparsed: "/unix/socket/address"}}, + {target: "", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ""}}, + {target: "://", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://"}}, + {target: ":///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///"}}, + {target: "://a/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://a/"}}, + {target: ":///a", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///a"}}, + {target: "://a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://a/b"}}, + {target: "/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/"}}, + {target: "a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a/b"}}, + {target: "a//b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a//b"}}, + {target: "google.com", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com"}}, + {target: "google.com/?a=b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com/"}}, + {target: "/unix/socket/address", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/unix/socket/address"}}, // An unregistered scheme is specified. - {target: "a:///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:///", Unparsed: "a:///"}}, - {target: "a://b/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a://b/", Unparsed: "a://b/"}}, - {target: "a:///b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:///b", Unparsed: "a:///b"}}, - {target: "a://b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a://b/c", Unparsed: "a://b/c"}}, - {target: "a:b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:b", Unparsed: "a:b"}}, - {target: "a:/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:/b", Unparsed: "a:/b"}}, - {target: "a://b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a://b", Unparsed: "a://b"}}, + {target: "a:///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:///"}}, + {target: "a://b/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b/"}}, + {target: "a:///b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:///b"}}, + {target: "a://b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b/c"}}, + {target: "a:b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:b"}}, + {target: "a:/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:/b"}}, + {target: "a://b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b"}}, // A registered scheme is specified. - {target: "dns:///google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "/google.com", Unparsed: "dns:///google.com"}}, - {target: "dns://a.server.com/google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "/google.com", Unparsed: "dns://a.server.com/google.com"}}, - {target: "dns://a.server.com/google.com/?a=b", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "/google.com/", Unparsed: "dns://a.server.com/google.com/?a=b"}}, - {target: "unix:///a/b/c", wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c", Unparsed: "unix:///a/b/c"}}, - {target: "unix-abstract:a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a/b/c", Unparsed: "unix-abstract:a/b/c"}}, - {target: "unix-abstract:a b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a b", Unparsed: "unix-abstract:a b"}}, - {target: "unix-abstract:a:b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a:b", Unparsed: "unix-abstract:a:b"}}, - {target: "unix-abstract:a-b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a-b", Unparsed: "unix-abstract:a-b"}}, - {target: "unix-abstract:/ a///://::!@#$%25^&*()b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "/ a///://::!@", Unparsed: "unix-abstract:/ a///://::!@#$%25^&*()b"}}, - {target: "unix-abstract:passthrough:abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "passthrough:abc", Unparsed: "unix-abstract:passthrough:abc"}}, - {target: "unix-abstract:unix:///abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "unix:///abc", Unparsed: "unix-abstract:unix:///abc"}}, - {target: "unix-abstract:///a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "/a/b/c", Unparsed: "unix-abstract:///a/b/c"}}, - {target: "unix-abstract:///", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "/", Unparsed: "unix-abstract:///"}}, - {target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "/unix:///a/b/c", Unparsed: "passthrough:///unix:///a/b/c"}}, + {target: "dns:///google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "google.com"}}, + {target: "dns://a.server.com/google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "google.com"}}, + {target: "dns://a.server.com/google.com/?a=b", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "google.com/"}}, + {target: "unix:///a/b/c", wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c"}}, + {target: "unix-abstract:a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a/b/c"}}, + {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: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"}}, + {target: "unix-abstract:///", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: ""}}, + {target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "unix:///a/b/c"}}, } for _, test := range tests { @@ -82,8 +85,8 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { } defer cc.Close() - if gotParsed := cc.parsedTarget; gotParsed != test.wantParsed { - t.Errorf("cc.parsedTarget = %+v, want %+v", gotParsed, test.wantParsed) + if !cmp.Equal(cc.parsedTarget, test.wantParsed, cmpopts.IgnoreFields(resolver.Target{}, "ParsedURL")) { + t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantParsed) } }) } @@ -118,39 +121,49 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { // different behaviors with a custom dialer. { target: "unix:a/b/c", - wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c", Unparsed: "unix:a/b/c"}, + wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c"}, wantDialerAddress: "unix:a/b/c", }, { target: "unix:/a/b/c", - wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c", Unparsed: "unix:/a/b/c"}, + wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}, wantDialerAddress: "unix:///a/b/c", }, { target: "unix:///a/b/c", - wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c", Unparsed: "unix:///a/b/c"}, + wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c"}, wantDialerAddress: "unix:///a/b/c", }, { target: "dns:///127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "/127.0.0.1:50051", Unparsed: "dns:///127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "127.0.0.1:50051"}, wantDialerAddress: "127.0.0.1:50051", }, { target: ":///127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///127.0.0.1:50051", Unparsed: ":///127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///127.0.0.1:50051"}, wantDialerAddress: ":///127.0.0.1:50051", }, { target: "dns://authority/127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: "dns", Authority: "authority", Endpoint: "/127.0.0.1:50051", Unparsed: "dns://authority/127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: "dns", Authority: "authority", Endpoint: "127.0.0.1:50051"}, wantDialerAddress: "127.0.0.1:50051", }, { target: "://authority/127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://authority/127.0.0.1:50051", Unparsed: "://authority/127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://authority/127.0.0.1:50051"}, wantDialerAddress: "://authority/127.0.0.1:50051", }, + { + target: "/unix/socket/address", + wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/unix/socket/address"}, + wantDialerAddress: "/unix/socket/address", + }, + { + target: "passthrough://a.server.com/google.com", + wantParsed: resolver.Target{Scheme: "passthrough", Authority: "a.server.com", Endpoint: "google.com"}, + wantDialerAddress: "google.com", + }, } for _, test := range tests { @@ -175,8 +188,8 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { case <-time.After(time.Second): t.Fatal("timeout when waiting for custom dialer to be invoked") } - if gotParsed := cc.parsedTarget; gotParsed != test.wantParsed { - t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, gotParsed, test.wantParsed) + if !cmp.Equal(cc.parsedTarget, test.wantParsed, cmpopts.IgnoreFields(resolver.Target{}, "ParsedURL")) { + t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantParsed) } }) } diff --git a/examples/features/load_balancing/client/main.go b/examples/features/load_balancing/client/main.go index 31935278849..1578df16671 100644 --- a/examples/features/load_balancing/client/main.go +++ b/examples/features/load_balancing/client/main.go @@ -23,7 +23,6 @@ import ( "context" "fmt" "log" - "strings" "time" "google.golang.org/grpc" @@ -94,7 +93,6 @@ func main() { type exampleResolverBuilder struct{} func (*exampleResolverBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { - target.Endpoint = strings.TrimPrefix(target.Endpoint, "/") r := &exampleResolver{ target: target, cc: cc, diff --git a/examples/features/name_resolving/client/main.go b/examples/features/name_resolving/client/main.go index 3bb285417d1..1c56dcce15d 100644 --- a/examples/features/name_resolving/client/main.go +++ b/examples/features/name_resolving/client/main.go @@ -23,7 +23,6 @@ import ( "context" "fmt" "log" - "strings" "time" "google.golang.org/grpc" @@ -100,7 +99,6 @@ func main() { type exampleResolverBuilder struct{} func (*exampleResolverBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { - target.Endpoint = strings.TrimPrefix(target.Endpoint, "/") r := &exampleResolver{ target: target, cc: cc, diff --git a/internal/resolver/dns/dns_resolver.go b/internal/resolver/dns/dns_resolver.go index 82fad8600fd..75301c51491 100644 --- a/internal/resolver/dns/dns_resolver.go +++ b/internal/resolver/dns/dns_resolver.go @@ -116,7 +116,7 @@ type dnsBuilder struct{} // Build creates and starts a DNS resolver that watches the name resolution of the target. func (b *dnsBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { - host, port, err := parseTarget(strings.TrimPrefix(target.Endpoint, "/"), defaultPort) + host, port, err := parseTarget(target.Endpoint, defaultPort) if err != nil { return nil, err } diff --git a/internal/resolver/passthrough/passthrough.go b/internal/resolver/passthrough/passthrough.go index 1f1c4eaeb5e..56db8e9ff35 100644 --- a/internal/resolver/passthrough/passthrough.go +++ b/internal/resolver/passthrough/passthrough.go @@ -21,8 +21,6 @@ package passthrough import ( - "strings" - "google.golang.org/grpc/resolver" ) @@ -31,7 +29,6 @@ const scheme = "passthrough" type passthroughBuilder struct{} func (*passthroughBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { - target.Endpoint = strings.TrimPrefix(target.Endpoint, "/") r := &passthroughResolver{ target: target, cc: cc, diff --git a/internal/resolver/unix/unix.go b/internal/resolver/unix/unix.go index 0d5a811ddfa..1ecc51de7fa 100644 --- a/internal/resolver/unix/unix.go +++ b/internal/resolver/unix/unix.go @@ -37,7 +37,20 @@ func (b *builder) Build(target resolver.Target, cc resolver.ClientConn, _ resolv if target.Authority != "" { return nil, fmt.Errorf("invalid (non-empty) authority: %v", target.Authority) } - addr := resolver.Address{Addr: target.Endpoint} + + // gRPC was parsing the dial target manually before PR #4817, and we + // switched to using url.Parse() in that PR. To avoid breaking existing + // resolver implementations we ended up stripping the leading "/" from the + // endpoint. This obviously does not work for the "unix" scheme. Hence we + // end up using the parsed URL instead. + endpoint := target.Endpoint + if u := target.ParsedURL; u != nil { + endpoint = u.Path + if endpoint == "" { + endpoint = u.Opaque + } + } + addr := resolver.Address{Addr: endpoint} if b.scheme == unixAbstractScheme { // prepend "\x00" to address for unix-abstract addr.Addr = "\x00" + addr.Addr diff --git a/resolver/resolver.go b/resolver/resolver.go index 8206d3263f9..98498dfd5c5 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -23,6 +23,7 @@ package resolver import ( "context" "net" + "net/url" "google.golang.org/grpc/attributes" "google.golang.org/grpc/credentials" @@ -228,7 +229,7 @@ type Target struct { Scheme string Authority string Endpoint string - Unparsed string + ParsedURL *url.URL } // Builder creates a resolver that will be used to watch name resolution updates. diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index d4b29503627..19ee01773e8 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -22,7 +22,6 @@ package resolver import ( "errors" "fmt" - "strings" "google.golang.org/grpc/credentials" "google.golang.org/grpc/internal/grpclog" @@ -62,7 +61,6 @@ type xdsResolverBuilder struct { // The xds bootstrap process is performed (and a new xds client is built) every // time an xds resolver is built. func (b *xdsResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { - t.Endpoint = strings.TrimPrefix(t.Endpoint, "/") r := &xdsResolver{ target: t, cc: cc, From f578ae530958a5cef19e34d74ef48ecad9741e62 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Wed, 29 Sep 2021 17:43:37 -0700 Subject: [PATCH 12/23] moved these tests to clientconn_parsed_target_test.go --- resolver_conn_wrapper_test.go | 33 --------------------------------- 1 file changed, 33 deletions(-) diff --git a/resolver_conn_wrapper_test.go b/resolver_conn_wrapper_test.go index 81c5b9ea874..1036946ad1e 100644 --- a/resolver_conn_wrapper_test.go +++ b/resolver_conn_wrapper_test.go @@ -21,8 +21,6 @@ package grpc import ( "context" "errors" - "fmt" - "net" "strings" "testing" "time" @@ -36,37 +34,6 @@ import ( "google.golang.org/grpc/status" ) -// The target string with unknown scheme should be kept unchanged and passed to -// the dialer. -func (s) TestDialParseTargetUnknownScheme(t *testing.T) { - for _, test := range []struct { - targetStr string - want string - }{ - {"/unix/socket/address", "/unix/socket/address"}, - - // For known scheme. - {"passthrough://a.server.com/google.com", "google.com"}, - } { - dialStrCh := make(chan string, 1) - cc, err := Dial(test.targetStr, WithInsecure(), WithDialer(func(addr string, _ time.Duration) (net.Conn, error) { - select { - case dialStrCh <- addr: - default: - } - return nil, fmt.Errorf("test dialer, always error") - })) - if err != nil { - t.Fatalf("Failed to create ClientConn: %v", err) - } - got := <-dialStrCh - cc.Close() - if got != test.want { - t.Errorf("Dial(%q), dialer got %q, want %q", test.targetStr, got, test.want) - } - } -} - const happyBalancerName = "happy balancer" func init() { From 1fa26dca154a96b571b5207942db36fad83b8ff6 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Wed, 29 Sep 2021 17:49:15 -0700 Subject: [PATCH 13/23] revert unwanted import change --- internal/resolver/passthrough/passthrough.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/resolver/passthrough/passthrough.go b/internal/resolver/passthrough/passthrough.go index 56db8e9ff35..520d9229e1e 100644 --- a/internal/resolver/passthrough/passthrough.go +++ b/internal/resolver/passthrough/passthrough.go @@ -20,9 +20,7 @@ // name without scheme back to gRPC as resolved address. package passthrough -import ( - "google.golang.org/grpc/resolver" -) +import "google.golang.org/grpc/resolver" const scheme = "passthrough" From 499f60350588e8add9b9274755161105fe43b7cc Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Wed, 29 Sep 2021 17:56:21 -0700 Subject: [PATCH 14/23] update comment for `ParsedURL` --- resolver/resolver.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/resolver/resolver.go b/resolver/resolver.go index 98498dfd5c5..125a2108a26 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -222,13 +222,14 @@ type ClientConn interface { // Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "/foo.bar"} // - "unknown_scheme://authority/endpoint" // Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "/unknown_scheme://authority/endpoint"} -// -// Unparsed will be set to the user's original dial target. Any query params -// specified in the dial target can be accessed though this field. type Target struct { Scheme string Authority string Endpoint string + // ParsedURL contains the parsed dial target with an optional default scheme + // added to it if the original dial target contained no scheme or containted + // an unregistered scheme. Any query params specified in the original dial + // target can be accessed from here. ParsedURL *url.URL } From c5c43834e8726ff8b37099192a22a774570a5826 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Thu, 30 Sep 2021 15:41:01 -0700 Subject: [PATCH 15/23] add tests for `scheme:absolute-path` --- clientconn_parsed_target_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index 5b94178afa4..45059f7c457 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -75,6 +75,10 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { {target: "unix-abstract:///a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a/b/c"}}, {target: "unix-abstract:///", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: ""}}, {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: "unregistered:/a/b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unregistered:/a/b/c"}}, } for _, test := range tests { From 5cff70708597789dd36dec9b4f979bf9901c9a27 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Thu, 30 Sep 2021 16:51:47 -0700 Subject: [PATCH 16/23] return error if ParsedURL is nil in unix resolver --- internal/resolver/unix/unix.go | 12 ++++++------ resolver/resolver.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/resolver/unix/unix.go b/internal/resolver/unix/unix.go index 1ecc51de7fa..25eef3c4680 100644 --- a/internal/resolver/unix/unix.go +++ b/internal/resolver/unix/unix.go @@ -43,12 +43,12 @@ func (b *builder) Build(target resolver.Target, cc resolver.ClientConn, _ resolv // resolver implementations we ended up stripping the leading "/" from the // endpoint. This obviously does not work for the "unix" scheme. Hence we // end up using the parsed URL instead. - endpoint := target.Endpoint - if u := target.ParsedURL; u != nil { - endpoint = u.Path - if endpoint == "" { - endpoint = u.Opaque - } + if target.ParsedURL == nil { + return nil, fmt.Errorf("nil ParsedURL in received target: %+v", target) + } + endpoint := target.ParsedURL.Path + if endpoint == "" { + endpoint = target.ParsedURL.Opaque } addr := resolver.Address{Addr: endpoint} if b.scheme == unixAbstractScheme { diff --git a/resolver/resolver.go b/resolver/resolver.go index 125a2108a26..6248c389af6 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -227,7 +227,7 @@ type Target struct { Authority string Endpoint string // ParsedURL contains the parsed dial target with an optional default scheme - // added to it if the original dial target contained no scheme or containted + // added to it if the original dial target contained no scheme or contained // an unregistered scheme. Any query params specified in the original dial // target can be accessed from here. ParsedURL *url.URL From c36957ff6901ee3e98b3b1fd3be9600c2d8ce4b6 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Fri, 1 Oct 2021 14:43:17 -0700 Subject: [PATCH 17/23] store the parsed url by value in `resolver.Target` --- clientconn.go | 2 +- internal/resolver/unix/unix.go | 3 --- resolver/resolver.go | 2 +- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/clientconn.go b/clientconn.go index 2dfffcdf3e9..89f43edfc83 100644 --- a/clientconn.go +++ b/clientconn.go @@ -1694,6 +1694,6 @@ func parseTarget(target string) (resolver.Target, error) { Scheme: u.Scheme, Authority: u.Host, Endpoint: endpoint, - ParsedURL: u, + ParsedURL: *u, }, nil } diff --git a/internal/resolver/unix/unix.go b/internal/resolver/unix/unix.go index 25eef3c4680..e43a68c3b22 100644 --- a/internal/resolver/unix/unix.go +++ b/internal/resolver/unix/unix.go @@ -43,9 +43,6 @@ func (b *builder) Build(target resolver.Target, cc resolver.ClientConn, _ resolv // resolver implementations we ended up stripping the leading "/" from the // endpoint. This obviously does not work for the "unix" scheme. Hence we // end up using the parsed URL instead. - if target.ParsedURL == nil { - return nil, fmt.Errorf("nil ParsedURL in received target: %+v", target) - } endpoint := target.ParsedURL.Path if endpoint == "" { endpoint = target.ParsedURL.Opaque diff --git a/resolver/resolver.go b/resolver/resolver.go index 6248c389af6..0ae636fd976 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -230,7 +230,7 @@ type Target struct { // added to it if the original dial target contained no scheme or contained // an unregistered scheme. Any query params specified in the original dial // target can be accessed from here. - ParsedURL *url.URL + ParsedURL url.URL } // Builder creates a resolver that will be used to watch name resolution updates. From 8546581c645f322e87df2e1f234beee95f1ecaf3 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Mon, 4 Oct 2021 18:07:33 -0700 Subject: [PATCH 18/23] review comments --- clientconn.go | 85 +++++++++++++++++++++----------- clientconn_authority_test.go | 10 ++-- clientconn_parsed_target_test.go | 6 +-- credentials/credentials.go | 6 ++- resolver/resolver.go | 6 +-- 5 files changed, 70 insertions(+), 43 deletions(-) diff --git a/clientconn.go b/clientconn.go index 89f43edfc83..7a48f7e7784 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,49 @@ func parseTarget(target string) (resolver.Target, error) { ParsedURL: *u, }, nil } + +// Determine channel authority. The order of precedence is as follows: +// - user specified authority override using `WithAuthority` dial option +// - creds' notion of server name for the authentication handshake +// - 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 + 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 != "" && authorityFromDialOption != "") && 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 authorityFromDialOption != "": + cc.authority = authorityFromDialOption + case authorityFromCreds != "": + cc.authority = authorityFromCreds + 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 9a28fe0f8a8..1d29eab2110 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", diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index 45059f7c457..423c9d7c53f 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/credentials/credentials.go b/credentials/credentials.go index 30a2b876f60..a90dca09c40 100644 --- a/credentials/credentials.go +++ b/credentials/credentials.go @@ -160,8 +160,10 @@ type TransportCredentials interface { Info() ProtocolInfo // Clone makes a copy of this TransportCredentials. Clone() TransportCredentials - // OverrideServerName overrides the server name used to verify the hostname - // on the returned certificates from the server. + // OverrideServerName specifies the value used for the following: + // - verifying the hostname on the returned certificates + // - as SNI in the client's handshake to support virtual hosting + // - as the value for `:authority` header at stream creation time // // Deprecated: use grpc.WithAuthority instead. Will be supported // throughout 1.x. diff --git a/resolver/resolver.go b/resolver/resolver.go index 0ae636fd976..3b0730e9ea3 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 From 78fcb25a0bec4726640587d4838d035b327d3ec6 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Mon, 11 Oct 2021 15:36:26 -0700 Subject: [PATCH 19/23] review comments - make determineAuthority() a func instead of a method on ClientConn - return error from Dial() if authority from creds and dialOption do not match - simplify --- clientconn.go | 30 +++++++++++++++--------------- clientconn_authority_test.go | 17 +++++++++++++++-- 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/clientconn.go b/clientconn.go index 7a48f7e7784..297338e7a3c 100644 --- a/clientconn.go +++ b/clientconn.go @@ -252,7 +252,10 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * if err != nil { return nil, err } - cc.determineAuthority() + cc.authority, err = determineAuthority(cc.parsedTarget.Endpoint, cc.target, cc.dopts) + if err != nil { + return nil, err + } channelz.Infof(logger, cc.channelzID, "Channel authority set to %q", cc.authority) if cc.dopts.scChan != nil && !scSet { @@ -1683,7 +1686,7 @@ func parseTarget(target string) (resolver.Target, error) { // - user specified authority override using `WithAuthority` dial option // - creds' notion of server name for the authentication handshake // - endpoint from dial target of the form "scheme://[authority]/endpoint" -func (cc *ClientConn) determineAuthority() { +func determineAuthority(endpoint, target string, dopts dialOptions) (string, error) { // 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). @@ -1695,15 +1698,12 @@ func (cc *ClientConn) determineAuthority() { // interface for the insecure case // - WithAuthority() dial option support for secure credentials authorityFromCreds := "" - if creds := cc.dopts.copts.TransportCredentials; creds != nil && creds.Info().ServerName != "" { + if creds := dopts.copts.TransportCredentials; creds != nil && creds.Info().ServerName != "" { authorityFromCreds = creds.Info().ServerName } - authorityFromDialOption := "" - if cc.dopts.authority != "" { - authorityFromDialOption = cc.dopts.authority - } + authorityFromDialOption := dopts.authority if (authorityFromCreds != "" && authorityFromDialOption != "") && 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) + return "", fmt.Errorf("ClientConn's authority from transport creds %q and dial option %q don't match", authorityFromCreds, authorityFromDialOption) } // TODO: Define an optional interface on the resolver builder to return the @@ -1711,16 +1711,16 @@ func (cc *ClientConn) determineAuthority() { // get rid of the special cases for unix and `:port`. switch { case authorityFromDialOption != "": - cc.authority = authorityFromDialOption + return authorityFromDialOption, nil case authorityFromCreds != "": - cc.authority = authorityFromCreds - 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 + return authorityFromCreds, nil + case strings.HasPrefix(target, "unix:") || strings.HasPrefix(target, "unix-abstract:"): + return "localhost", nil + case strings.HasPrefix(endpoint, ":"): + return "localhost" + endpoint, nil default: // Use endpoint from "scheme://authority/endpoint" as the default // authority for ClientConn. - cc.authority = cc.parsedTarget.Endpoint + return endpoint, nil } } diff --git a/clientconn_authority_test.go b/clientconn_authority_test.go index 1d29eab2110..7a77de64c57 100644 --- a/clientconn_authority_test.go +++ b/clientconn_authority_test.go @@ -61,8 +61,8 @@ 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", + opts: []DialOption{WithTransportCredentials(creds), WithAuthority(serverNameOverride)}, + wantAuthority: serverNameOverride, }, { name: "unix relative", @@ -119,3 +119,16 @@ func (s) TestClientConnAuthority(t *testing.T) { }) } } + +func (s) TestClientConnAuthority_CredsAndDialOptionMismatch(t *testing.T) { + serverNameOverride := "over.write.server.name" + creds, err := credentials.NewClientTLSFromFile(testdata.Path("x509/server_ca_cert.pem"), serverNameOverride) + if err != nil { + t.Fatalf("credentials.NewClientTLSFromFile(_, %q) failed: %v", err, serverNameOverride) + } + opts := []DialOption{WithTransportCredentials(creds), WithAuthority("authority-override")} + if cc, err := Dial("Non-Existent.Server:8000", opts...); err == nil { + cc.Close() + t.Fatal("grpc.Dial() succeeded when expected to fail") + } +} From 4fb05c38dee315318274247a80ba8fb08a98f116 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Mon, 11 Oct 2021 15:47:30 -0700 Subject: [PATCH 20/23] edit todo for optional resolver interface --- clientconn.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/clientconn.go b/clientconn.go index 297338e7a3c..cdc9a03e3f8 100644 --- a/clientconn.go +++ b/clientconn.go @@ -1706,21 +1706,22 @@ func determineAuthority(endpoint, target string, dopts dialOptions) (string, err return "", fmt.Errorf("ClientConn's authority from transport creds %q and dial option %q don't match", 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 authorityFromDialOption != "": return authorityFromDialOption, nil case authorityFromCreds != "": return authorityFromCreds, nil case strings.HasPrefix(target, "unix:") || strings.HasPrefix(target, "unix-abstract:"): + // TODO: remove when the unix resolver implements optional interface to + // return channel authority. return "localhost", nil case strings.HasPrefix(endpoint, ":"): return "localhost" + endpoint, nil default: - // Use endpoint from "scheme://authority/endpoint" as the default - // authority for ClientConn. + // TODO: Define an optional interface on the resolver builder to return + // the channel authority given the user's dial target. For resolvers + // which don't implement this interface, we will use the endpoint from + // "scheme://authority/endpoint" as the default authority. return endpoint, nil } } From c6057e50a16b5b5de26f33d7335fa0f57bcf3914 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Mon, 11 Oct 2021 15:59:12 -0700 Subject: [PATCH 21/23] ClientHandshake comments --- credentials/credentials.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/credentials/credentials.go b/credentials/credentials.go index a90dca09c40..a671107584f 100644 --- a/credentials/credentials.go +++ b/credentials/credentials.go @@ -140,12 +140,10 @@ type TransportCredentials interface { // Additionally, ClientHandshakeInfo data will be available via the context // passed to this call. // - // The second argument to this method is the ClientConn's authority, usually - // derived from the user's dial target. Implementations should use this as - // the server name during the authentication handshake. Implementations may - // choose to ignore this value and use a value acquired through other means, - // in which case they must make sure that the value is acquired through - // secure means and that a possible attacker cannot tamper with the value. + // The second argument to this method is the `:authority` header value used + // while creating new streams on this connection after authentication + // succeeds. Implementations must use this as the server name during the + // authentication handshake. // // If the returned net.Conn is closed, it MUST close the net.Conn provided. ClientHandshake(context.Context, string, net.Conn) (net.Conn, AuthInfo, error) From f230c5323a1a86a5f2359e31acbd24d050901ecc Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Mon, 11 Oct 2021 16:05:11 -0700 Subject: [PATCH 22/23] deprecate old fields of resolver.Target and rename `ParsedURL` to `URL` --- clientconn.go | 4 ++-- clientconn_parsed_target_test.go | 4 ++-- internal/resolver/unix/unix.go | 4 ++-- resolver/resolver.go | 16 ++++++++++------ 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/clientconn.go b/clientconn.go index cdc9a03e3f8..eacf5fd8fcc 100644 --- a/clientconn.go +++ b/clientconn.go @@ -1668,7 +1668,7 @@ func parseTarget(target string) (resolver.Target, error) { // end up stripping the leading "/" here. But this will result in an // incorrect parsing for something like "unix:///path/to/socket". Since we // own the "unix" resolver, we can workaround in the unix resolver by using - // the `ParsedURL` field instead of the `Endpoint` field. + // the `URL` field instead of the `Endpoint` field. endpoint := u.Path if endpoint == "" { endpoint = u.Opaque @@ -1678,7 +1678,7 @@ func parseTarget(target string) (resolver.Target, error) { Scheme: u.Scheme, Authority: u.Host, Endpoint: endpoint, - ParsedURL: *u, + URL: *u, }, nil } diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index 423c9d7c53f..e39e37c1929 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -89,7 +89,7 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { } defer cc.Close() - if !cmp.Equal(cc.parsedTarget, test.wantParsed, cmpopts.IgnoreFields(resolver.Target{}, "ParsedURL")) { + if !cmp.Equal(cc.parsedTarget, test.wantParsed, cmpopts.IgnoreFields(resolver.Target{}, "URL")) { t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantParsed) } }) @@ -192,7 +192,7 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { case <-time.After(time.Second): t.Fatal("timeout when waiting for custom dialer to be invoked") } - if !cmp.Equal(cc.parsedTarget, test.wantParsed, cmpopts.IgnoreFields(resolver.Target{}, "ParsedURL")) { + if !cmp.Equal(cc.parsedTarget, test.wantParsed, cmpopts.IgnoreFields(resolver.Target{}, "URL")) { t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantParsed) } }) diff --git a/internal/resolver/unix/unix.go b/internal/resolver/unix/unix.go index e43a68c3b22..20852e59df2 100644 --- a/internal/resolver/unix/unix.go +++ b/internal/resolver/unix/unix.go @@ -43,9 +43,9 @@ func (b *builder) Build(target resolver.Target, cc resolver.ClientConn, _ resolv // resolver implementations we ended up stripping the leading "/" from the // endpoint. This obviously does not work for the "unix" scheme. Hence we // end up using the parsed URL instead. - endpoint := target.ParsedURL.Path + endpoint := target.URL.Path if endpoint == "" { - endpoint = target.ParsedURL.Opaque + endpoint = target.URL.Opaque } addr := resolver.Address{Addr: endpoint} if b.scheme == unixAbstractScheme { diff --git a/resolver/resolver.go b/resolver/resolver.go index 3b0730e9ea3..9116897b463 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -223,14 +223,18 @@ type ClientConn interface { // - "unknown_scheme://authority/endpoint" // Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "unknown_scheme://authority/endpoint"} type Target struct { - Scheme string + // Deprecated: use URL.Scheme instead. + Scheme string + // Deprecated: use URL.Host instead. Authority string - Endpoint string - // ParsedURL contains the parsed dial target with an optional default scheme - // added to it if the original dial target contained no scheme or contained - // an unregistered scheme. Any query params specified in the original dial + // Deprecated: use URL.Path or URL.Opaque instead. The latter is set when + // the former is empty. + Endpoint string + // URL contains the parsed dial target with an optional default scheme added + // to it if the original dial target contained no scheme or contained an + // unregistered scheme. Any query params specified in the original dial // target can be accessed from here. - ParsedURL url.URL + URL url.URL } // Builder creates a resolver that will be used to watch name resolution updates. From 5f969462038a3e5b876f61fa0d71100f6ff2a4ff Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Mon, 11 Oct 2021 16:52:37 -0700 Subject: [PATCH 23/23] validate parsed URL in unit tests --- clientconn_parsed_target_test.go | 94 ++++++++++++++++---------------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index e39e37c1929..e41fafe0966 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -22,11 +22,11 @@ import ( "context" "errors" "net" + "net/url" "testing" "time" "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" "google.golang.org/grpc/resolver" ) @@ -38,47 +38,47 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { wantParsed resolver.Target }{ // No scheme is specified. - {target: "", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ""}}, - {target: "://", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://"}}, - {target: ":///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///"}}, - {target: "://a/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://a/"}}, - {target: ":///a", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///a"}}, - {target: "://a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://a/b"}}, - {target: "/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/"}}, - {target: "a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a/b"}}, - {target: "a//b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a//b"}}, - {target: "google.com", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com"}}, - {target: "google.com/?a=b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com/"}}, - {target: "/unix/socket/address", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/unix/socket/address"}}, + {target: "", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "", URL: url.URL{Scheme: defScheme, Path: "/"}}}, + {target: "://", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://", URL: url.URL{Scheme: defScheme, Path: "/://"}}}, + {target: ":///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///", URL: url.URL{Scheme: defScheme, Path: "/:///"}}}, + {target: "://a/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://a/", URL: url.URL{Scheme: defScheme, Path: "/://a/"}}}, + {target: ":///a", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///a", URL: url.URL{Scheme: defScheme, Path: "/:///a"}}}, + {target: "://a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://a/b", URL: url.URL{Scheme: defScheme, Path: "/://a/b"}}}, + {target: "/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/", URL: url.URL{Scheme: defScheme, Path: "//"}}}, + {target: "a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a/b", URL: url.URL{Scheme: defScheme, Path: "/a/b"}}}, + {target: "a//b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a//b", URL: url.URL{Scheme: defScheme, Path: "/a//b"}}}, + {target: "google.com", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com", URL: url.URL{Scheme: defScheme, Path: "/google.com"}}}, + {target: "google.com/?a=b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com/", URL: url.URL{Scheme: defScheme, Path: "/google.com/", RawQuery: "a=b"}}}, + {target: "/unix/socket/address", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/unix/socket/address", URL: url.URL{Scheme: defScheme, Path: "//unix/socket/address"}}}, // An unregistered scheme is specified. - {target: "a:///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:///"}}, - {target: "a://b/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b/"}}, - {target: "a:///b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:///b"}}, - {target: "a://b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b/c"}}, - {target: "a:b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:b"}}, - {target: "a:/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:/b"}}, - {target: "a://b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b"}}, + {target: "a:///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:///", URL: url.URL{Scheme: defScheme, Path: "/a:///"}}}, + {target: "a://b/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b/", URL: url.URL{Scheme: defScheme, Path: "/a://b/"}}}, + {target: "a:///b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:///b", URL: url.URL{Scheme: defScheme, Path: "/a:///b"}}}, + {target: "a://b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b/c", URL: url.URL{Scheme: defScheme, Path: "/a://b/c"}}}, + {target: "a:b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:b", URL: url.URL{Scheme: defScheme, Path: "/a:b"}}}, + {target: "a:/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:/b", URL: url.URL{Scheme: defScheme, Path: "/a:/b"}}}, + {target: "a://b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b", URL: url.URL{Scheme: defScheme, Path: "/a://b"}}}, // A registered scheme is specified. - {target: "dns:///google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "google.com"}}, - {target: "dns://a.server.com/google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "google.com"}}, - {target: "dns://a.server.com/google.com/?a=b", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "google.com/"}}, - {target: "unix:///a/b/c", wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c"}}, - {target: "unix-abstract:a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a/b/c"}}, - {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: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"}}, - {target: "unix-abstract:///", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: ""}}, - {target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "unix:///a/b/c"}}, + {target: "dns:///google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "google.com", URL: url.URL{Scheme: "dns", Path: "/google.com"}}}, + {target: "dns://a.server.com/google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "google.com", URL: url.URL{Scheme: "dns", Host: "a.server.com", Path: "/google.com"}}}, + {target: "dns://a.server.com/google.com/?a=b", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "google.com/", URL: url.URL{Scheme: "dns", Host: "a.server.com", Path: "/google.com/", RawQuery: "a=b"}}}, + {target: "unix:///a/b/c", wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c", URL: url.URL{Scheme: "unix", Path: "/a/b/c"}}}, + {target: "unix-abstract:a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a/b/c", URL: url.URL{Scheme: "unix-abstract", Path: "", Opaque: "a/b/c"}}}, + {target: "unix-abstract:a b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a b", URL: url.URL{Scheme: "unix-abstract", Path: "", Opaque: "a b"}}}, + {target: "unix-abstract:a:b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a:b", URL: url.URL{Scheme: "unix-abstract", Path: "", Opaque: "a:b"}}}, + {target: "unix-abstract:a-b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a-b", URL: url.URL{Scheme: "unix-abstract", Path: "", Opaque: "a-b"}}}, + {target: "unix-abstract:/ a///://::!@#$%25^&*()b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: " a///://::!@", URL: url.URL{Scheme: "unix-abstract", Path: "/ a///://::!@", RawPath: "/ a///://::!@", Fragment: "$%^&*()b", RawFragment: "$%25^&*()b"}}}, + {target: "unix-abstract:passthrough:abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "passthrough:abc", URL: url.URL{Scheme: "unix-abstract", Path: "", Opaque: "passthrough:abc"}}}, + {target: "unix-abstract:unix:///abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "unix:///abc", URL: url.URL{Scheme: "unix-abstract", Path: "", Opaque: "unix:///abc"}}}, + {target: "unix-abstract:///a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a/b/c", URL: url.URL{Scheme: "unix-abstract", Path: "/a/b/c"}}}, + {target: "unix-abstract:///", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "", URL: url.URL{Scheme: "unix-abstract", Path: "/"}}}, + {target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "unix:///a/b/c", URL: url.URL{Scheme: "passthrough", Path: "/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: "unregistered:/a/b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unregistered:/a/b/c"}}, + {target: "dns:/a/b/c", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "a/b/c", URL: url.URL{Scheme: "dns", Path: "/a/b/c"}}}, + {target: "unregistered:/a/b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unregistered:/a/b/c", URL: url.URL{Scheme: defScheme, Path: "/unregistered:/a/b/c"}}}, } for _, test := range tests { @@ -89,7 +89,7 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { } defer cc.Close() - if !cmp.Equal(cc.parsedTarget, test.wantParsed, cmpopts.IgnoreFields(resolver.Target{}, "URL")) { + if !cmp.Equal(cc.parsedTarget, test.wantParsed) { t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantParsed) } }) @@ -125,47 +125,47 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { // different behaviors with a custom dialer. { 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", URL: url.URL{Scheme: "unix", Opaque: "a/b/c"}}, wantDialerAddress: "unix:a/b/c", }, { 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", URL: url.URL{Scheme: "unix", Path: "/a/b/c"}}, wantDialerAddress: "unix:///a/b/c", }, { 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", URL: url.URL{Scheme: "unix", Path: "/a/b/c"}}, wantDialerAddress: "unix:///a/b/c", }, { target: "dns:///127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "127.0.0.1:50051", URL: url.URL{Scheme: "dns", Path: "/127.0.0.1:50051"}}, wantDialerAddress: "127.0.0.1:50051", }, { target: ":///127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///127.0.0.1:50051", URL: url.URL{Scheme: defScheme, Path: "/:///127.0.0.1:50051"}}, wantDialerAddress: ":///127.0.0.1:50051", }, { target: "dns://authority/127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: "dns", Authority: "authority", Endpoint: "127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: "dns", Authority: "authority", Endpoint: "127.0.0.1:50051", URL: url.URL{Scheme: "dns", Host: "authority", Path: "/127.0.0.1:50051"}}, wantDialerAddress: "127.0.0.1:50051", }, { target: "://authority/127.0.0.1:50051", - wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://authority/127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://authority/127.0.0.1:50051", URL: url.URL{Scheme: defScheme, Path: "/://authority/127.0.0.1:50051"}}, wantDialerAddress: "://authority/127.0.0.1:50051", }, { target: "/unix/socket/address", - wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/unix/socket/address"}, + wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/unix/socket/address", URL: url.URL{Scheme: defScheme, Path: "//unix/socket/address"}}, wantDialerAddress: "/unix/socket/address", }, { target: "passthrough://a.server.com/google.com", - wantParsed: resolver.Target{Scheme: "passthrough", Authority: "a.server.com", Endpoint: "google.com"}, + wantParsed: resolver.Target{Scheme: "passthrough", Authority: "a.server.com", Endpoint: "google.com", URL: url.URL{Scheme: "passthrough", Host: "a.server.com", Path: "/google.com"}}, wantDialerAddress: "google.com", }, } @@ -192,7 +192,7 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { case <-time.After(time.Second): t.Fatal("timeout when waiting for custom dialer to be invoked") } - if !cmp.Equal(cc.parsedTarget, test.wantParsed, cmpopts.IgnoreFields(resolver.Target{}, "URL")) { + if !cmp.Equal(cc.parsedTarget, test.wantParsed) { t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantParsed) } })