Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grpc: better RFC 3986 compliant target parsing #4817

Merged
merged 24 commits into from Oct 14, 2021

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Sep 24, 2021

Summary of changes

  • Use url.Parse to parse dial target
    • If parsed scheme is not registered, or scheme is not found in the dial target, fallback to default scheme
  • Allow WithAuthority to override the :authority and serverName for cases where transport creds are used
    • When specified, this takes precedence over any per-address overrides specified through resolver.Address.ServerName
  • Continue to support overriding the channel's authority with the server name configured on the transport creds. But this happens only when WithAuthority is not used.

Addresses #4717

RELEASE NOTES:

  • Support WithAuthority for secure credentials * Add ParsedURL field to resolver.Target to store parsed dial target * Mark TransportCredentials.OverrideServerName method as deprecated * Breaks "unix://relative-path"

@easwars easwars requested review from dfawley and menghanl Sep 24, 2021
@easwars easwars added Type: API Change Type: Behavior Change Type: Documentation Type: Internal Cleanup labels Sep 24, 2021
@easwars easwars added this to the 1.42 Release milestone Sep 24, 2021
@easwars easwars mentioned this pull request Sep 29, 2021
5 tasks
clientconn.go Outdated Show resolved Hide resolved
clientconn.go Outdated
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
}
Copy link
Contributor

@dfawley dfawley Sep 29, 2021

Choose a reason for hiding this comment

The 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 ServerName to localhost? If so, let's do that instead.. I would rather avoid as many special cases as possible in generic places.

Copy link
Contributor Author

@easwars easwars Sep 30, 2021

Choose a reason for hiding this comment

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

The reason we don't want to rely on the resolver.Address.ServerName to determine the channel's authority at this point in time is because if we do that then we wont be able to pass the correct authority to balancer.Build(). The resolver.Address.ServerName is better used as an override.

Copy link
Contributor

@dfawley dfawley Sep 30, 2021

Choose a reason for hiding this comment

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

But for the unix resolver, the balancer is going to be pick first, which doesn't/shouldn't care about the authority. Most balancers shouldn't care, right -- just RLS/gRPCLB?

Copy link
Contributor Author

@easwars easwars Sep 30, 2021

Choose a reason for hiding this comment

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

Right, none of our balancers care, except rls/grpclb.
But given we are passing that to the balancer in its BuildOptions and the Target() method on the balancer.ClientConn is deprecated in favor of the former, we should ideally set the correct value there, irrespective of whether or not one of our balancers cares about it. Wdyt?

Copy link
Contributor Author

@easwars easwars Sep 30, 2021

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@dfawley dfawley Oct 4, 2021

Choose a reason for hiding this comment

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

Yes, for the special handling of unix and unix-abstract. We shouldn't need to do this if we set ServerName in those resolvers instead. Any balancer used with those schemes shouldn't care about the authority. Only RLS and gRPCLB care about the "channel authority" (so they can use the same one with their backends), and it doesn't make sense to use those with a unix address.

Copy link
Contributor Author

@easwars easwars Oct 7, 2021

Choose a reason for hiding this comment

The 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.

clientconn.go Outdated Show resolved Hide resolved
clientconn.go Outdated Show resolved Hide resolved
clientconn.go Outdated Show resolved Hide resolved
credentials/credentials.go Outdated Show resolved Hide resolved
credentials/credentials.go Outdated Show resolved Hide resolved
internal/grpcutil/grpcutil.go Outdated Show resolved Hide resolved
test/authority_test.go Show resolved Hide resolved
xds/internal/xdsclient/v2/client_test.go Show resolved Hide resolved
@dfawley dfawley assigned easwars and unassigned dfawley Sep 29, 2021
@@ -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, "/"),
Copy link
Contributor

@menghanl menghanl Sep 29, 2021

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?

Copy link
Contributor Author

@easwars easwars Sep 30, 2021

Choose a reason for hiding this comment

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

unix:/absolute-path parses as Scheme: "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 /.

clientconn.go Outdated Show resolved Hide resolved
@easwars
Copy link
Contributor Author

@easwars easwars commented Sep 29, 2021

Also, added an Unparsed field to resolver.Target to store the user's original dial target. Resolver implementations relying on query params in the dial target can get it from this field.

@easwars easwars assigned dfawley and unassigned menghanl and easwars Sep 29, 2021
@menghanl
Copy link
Contributor

@menghanl menghanl commented Oct 6, 2021

And add back the support for server name from creds

@easwars
Copy link
Contributor Author

@easwars easwars commented Oct 7, 2021

And add back the support for server name from creds

This is also done.

@easwars
Copy link
Contributor Author

@easwars easwars commented Oct 7, 2021

I think I've taken care of all comments. Please take one more pass. I've updated the PR description and also the release notes in there.

What is left is:

  • adding support for the optional interface on the resolver builder to return channel authority. Once we support this, we can get rid of the special case for unix and :port
  • Deprecate resolver.Address.ServerName and specify the per-address override through attributes instead

@easwars
Copy link
Contributor Author

@easwars easwars commented Oct 7, 2021

Also, one important thing that I have changed in the last commit(s) is that WithAuthority now wins over everything else. This should not break too many existing users since WithAuthority was marked as only supported for the insecure case. If someone is broken, I would say sorry too bad. I think it definitely makes things easier/cleaner to have WithAuthority win over everything else given that we are deprecating OverrideServerName and are encouraging users to use WithAuthority instead.

Please let me know if you feel strongly about this.

clientconn.go Outdated
authorityFromDialOption := ""
if cc.dopts.authority != "" {
authorityFromDialOption = cc.dopts.authority
}
Copy link
Contributor

@dfawley dfawley Oct 8, 2021

Choose a reason for hiding this comment

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

Simplify: authorityFromDialOption := cc.dopts.authority

Copy link
Contributor Author

@easwars easwars Oct 11, 2021

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

@easwars easwars Oct 11, 2021

Choose a reason for hiding this comment

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

Done.

clientconn.go Outdated
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)
}
Copy link
Contributor

@dfawley dfawley Oct 8, 2021

Choose a reason for hiding this comment

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

I think we should return an error here. The Creds+WithAuthority combination wasn't even allowed in the past so now that we're allowing it I'd rather fail if they are both specified and not consistent.

Copy link
Contributor Author

@easwars easwars Oct 11, 2021

Choose a reason for hiding this comment

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

Done.

clientconn.go Outdated
// - 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() {
Copy link
Contributor

@dfawley dfawley Oct 8, 2021

Choose a reason for hiding this comment

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

I think this may be cleaner if you passed dopts as a parameter, returned the authority, and didn't make this a method. I'd rather see cc.authority := determineAuthority(cc.dopts) than cc.determineAuthority() since in the former case I can reason about what might and might not be happening, whereas in the latter case I need to inspect cc.determineAuthority to determine what all it might be doing.

Copy link
Contributor Author

@easwars easwars Oct 11, 2021

Choose a reason for hiding this comment

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

Done.

clientconn.go Outdated
cc.authority = authorityFromDialOption
case authorityFromCreds != "":
cc.authority = authorityFromCreds
case strings.HasPrefix(cc.target, "unix:") || strings.HasPrefix(cc.target, "unix-abstract:"):
Copy link
Contributor

@dfawley dfawley Oct 8, 2021

Choose a reason for hiding this comment

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

// TODO: remove when the unix resolver returns addresses with ServerName set and we make that affect the authority used for RPCs on that connection or something.

Copy link
Contributor Author

@easwars easwars Oct 11, 2021

Choose a reason for hiding this comment

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

Done.

{target: "dns://a.server.com/google.com/?a=b", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "google.com/?a=b"}},
{target: "unix:///a/b/c", wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}},
{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"}},
Copy link
Contributor

@dfawley dfawley Oct 8, 2021

Choose a reason for hiding this comment

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

Let's validate the URL (at least URL.Path) here?

Copy link
Contributor Author

@easwars easwars Oct 11, 2021

Choose a reason for hiding this comment

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

Since I marked the older fields as deprecated, I added the URL to every case.

{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: ""}},
Copy link
Contributor

@dfawley dfawley Oct 8, 2021

Choose a reason for hiding this comment

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

Why did you delete the other two test cases?

Copy link
Contributor Author

@easwars easwars Oct 11, 2021

Choose a reason for hiding this comment

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

We do not support unix*://<relative-path> as per our naming guide. Earlier these used to parse differently. But now, they parse with resolver.Target.Authority set. This means that the unix* resolver will reject this because it does not support an authority. These cases are now moved to the TestParsedTarget_Failure_WithoutCustomDialer test.

// 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
Copy link
Contributor

@dfawley dfawley Oct 8, 2021

Choose a reason for hiding this comment

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

Let's bikeshed: IMO this field should be called simply URL. The Parsed part is obvious to me since a url.URL is never "unparsed". @menghanl, do you have an opinion?

Copy link
Contributor Author

@easwars easwars Oct 11, 2021

Choose a reason for hiding this comment

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

Done.

Scheme string
Authority string
Endpoint string
Copy link
Contributor

@dfawley dfawley Oct 8, 2021

Choose a reason for hiding this comment

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

Let's // Deprecated these. (How are they undocumented?)

Copy link
Contributor Author

@easwars easwars Oct 11, 2021

Choose a reason for hiding this comment

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

Marked them as deprecated and pointed to the fields inside url.URL.

// The second argument to this method is the ClientConn's authority, usually
// derived from the user's dial target. Implementations should use this as
Copy link
Contributor

@dfawley dfawley Oct 8, 2021

Choose a reason for hiding this comment

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

Hmm, ClientConn's authority? But what if the ServerName on the address overrides it?

Copy link
Contributor Author

@easwars easwars Oct 11, 2021

Choose a reason for hiding this comment

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

Reworded it a little.

// 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.
Copy link
Contributor

@dfawley dfawley Oct 8, 2021

Choose a reason for hiding this comment

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

Holy cow, we should never, ever recommend (or even mention) ignoring this field. Implementations MUST honor this value, period. Otherwise the :authority header won't match and who knows what problems that will cause. Just change the previous sentence to Implementations must use this as the server name during the authentication handshake. and leave it at that. If someone really, really knows what they're doing and they want to abuse this, then that's fine and we won't support them.

Copy link
Contributor Author

@easwars easwars Oct 11, 2021

Choose a reason for hiding this comment

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

Done.

@dfawley dfawley assigned easwars and unassigned dfawley Oct 8, 2021
easwars added 5 commits Oct 11, 2021
- make determineAuthority() a func instead of a method on ClientConn
- return error from Dial() if authority from creds and dialOption do not
  match
- simplify
@easwars easwars assigned dfawley and unassigned easwars Oct 11, 2021
@dfawley dfawley removed their assignment Oct 14, 2021
@easwars easwars merged commit aaff9e7 into grpc:master Oct 14, 2021
9 of 10 checks passed
@easwars easwars deleted the url_parse_attempt_3 branch Oct 14, 2021
Allenxuxu added a commit to Allenxuxu/stark that referenced this issue Dec 11, 2021
grpc/grpc-go#4717
https://github.com/grpc/grpc-go/releases/tag/v1.42.0
grpc/grpc-go#4817
从 1.42.0 之后,grpc-go 使用 url.Parse 来解析 dial target ,即 "scheme://xxx/xxx" , 之前的 secheme 含有非法字符下划线会导致解析失败。
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: API Change Type: Behavior Change Type: Documentation Type: Internal Cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants