New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
grpc: better RFC 3986 compliant target parsing #4817
Changes from 5 commits
fb12d07
cee1b44
d19cd9b
0d0c1a5
c98608a
fb4b9c0
2750421
c522792
4f56e2e
ae490d0
99fdc43
f578ae5
1fa26dc
7936989
499f603
c5c4383
5cff707
c36957f
8546581
78fcb25
4fb05c3
c6057e5
f230c53
5f96946
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is talking about the leading There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactored this logic into a separate method. The comment is also gone now. |
||
// 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. | ||
dfawley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cc.authority = strings.TrimPrefix(cc.parsedTarget.Endpoint, "/") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this still necessary? I thought the leading slash is stripped in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not required anymore. Deleted it. |
||
if strings.HasPrefix(cc.authority, ":") { | ||
cc.authority = "localhost" + cc.authority | ||
menghanl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
if cc.parsedTarget.Scheme == "unix" || cc.parsedTarget.Scheme == "unix-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 | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we eliminate this by making sure the addresses output by the unix resolver set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason we don't want to rely on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, none of our balancers care, except rls/grpclb. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is your gripe here only about the special handling for unix or something else? Also, once we have the optional interface on the resolver builder to get the authority, we will not need this special case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, for the special handling of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These special checks remain for now. But as promised I will take care of them in a follow up PR where I will introduce the optional interface on the resolver builder. |
||
// 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. | ||
dfawley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 { | ||
dfawley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if ac.cc.dopts.authority == "" { | ||
if addr.ServerName != "" { | ||
return addr.ServerName | ||
} | ||
} | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Thanks. |
||
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" | ||
} | ||
dfawley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this a behavior change? "not supported" and "not recommended" and "deprecated" is fine but we shouldn't change behavior that users may be relying upon. If we're reasonably sure nobody is using this (IIRC we added it only for grpclb?) then maybe we can try it and be ready to revert it, but if it's easy enough to keep the behavior, then we should. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Supporting this would mean adding another condition to the logic in Again, using the value specified through
So, if the transport creds uses the value specified through this call in its handshake, that should be good enough, right? This is what our TLS creds implementation currently does. https://github.com/grpc/grpc-go/blob/master/credentials/tls.go#L76-L83 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That logic seems fine, but then the comment should be a little different here. It should say the channel authority is not impacted, but the credentials implementation may still validate using that authority instead of the one provided by the channel. However, is this okay for security reasons, to have the channel thinking the authority is one thing, but the credentials silently using a different authority? It's a bit of a weird thing in the first place, but it seems like it should notify the channel if it's happening. Should we at least deprecate all things related to the credentials override? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should keep supporting this, unless it's too messy. But it sounds not too bad to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re-added the support. |
||
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", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a behavior change.
We should try to keep the value of the existing fields unchanged.
Should gRPC trim the leading slash? What's the reason to not do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unix:/absolute-path
parses asScheme: "unix" Endpoint: "/path"
and we cannot remove it in that case.Endpoint
according to the RFC does contain the leading slash, and implementations in C strip the leading/
.