grpc: better RFC 3986 compliant target parsing #4817

easwars · 2021-09-24T23:34:54Z

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"

clientconn.go

dfawley · 2021-09-29T17:19:40Z

clientconn.go

+	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
+	}


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.

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.

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?

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?

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.

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.

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

credentials/credentials.go

internal/grpcutil/grpcutil.go

test/authority_test.go

xds/internal/xdsclient/v2/client_test.go

menghanl · 2021-09-29T17:54:34Z

balancer/grpclb/grpclb.go

@@ -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, "/"),


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?

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

easwars · 2021-09-29T22:44:07Z

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.

menghanl · 2021-10-06T18:33:29Z

And add back the support for server name from creds

easwars · 2021-10-07T00:22:12Z

And add back the support for server name from creds

This is also done.

easwars · 2021-10-07T00:26:00Z

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 · 2021-10-07T00:28:59Z

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.

dfawley · 2021-10-08T16:12:40Z

clientconn.go

+	authorityFromDialOption := ""
+	if cc.dopts.authority != "" {
+		authorityFromDialOption = cc.dopts.authority
+	}


Simplify: authorityFromDialOption := cc.dopts.authority

dfawley · 2021-10-08T16:13:29Z

clientconn.go

+	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)
+	}


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.

dfawley · 2021-10-08T16:15:25Z

clientconn.go

+// - 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() {


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.

dfawley · 2021-10-08T16:19:12Z

clientconn.go

+		cc.authority = authorityFromDialOption
+	case authorityFromCreds != "":
+		cc.authority = authorityFromCreds
+	case strings.HasPrefix(cc.target, "unix:") || strings.HasPrefix(cc.target, "unix-abstract:"):


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

dfawley · 2021-10-08T16:22:06Z

clientconn_parsed_target_test.go

-		{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"}},


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

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

dfawley · 2021-10-08T16:22:49Z

clientconn_parsed_target_test.go

+		{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: ""}},


Why did you delete the other two test cases?

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.

dfawley · 2021-10-08T16:26:14Z

resolver/resolver.go

+	// 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


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?

dfawley · 2021-10-08T16:26:36Z

resolver/resolver.go

 	Scheme    string
 	Authority string
 	Endpoint  string


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

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

dfawley · 2021-10-08T16:30:07Z

credentials/credentials.go

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


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

Reworded it a little.

dfawley · 2021-10-08T16:32:11Z

credentials/credentials.go

+	// 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.


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.

- make determineAuthority() a func instead of a method on ClientConn - return error from Dial() if authority from creds and dialOption do not match - simplify

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 含有非法字符下划线会导致解析失败。

easwars requested review from dfawley and menghanl September 24, 2021 23:35

easwars assigned menghanl and dfawley Sep 24, 2021

easwars added Type: API Change Breaking API changes (experimental APIs only!) Type: Behavior Change Behavior changes not categorized as bugs Type: Documentation Documentation or examples Type: Internal Cleanup Refactors, etc labels Sep 24, 2021

easwars added this to the 1.42 Release milestone Sep 24, 2021

grpc: increase compliance with RFC 3986 while parsing the dial target

fb12d07

easwars force-pushed the url_parse_attempt_3 branch from de6a1bd to fb12d07 Compare September 24, 2021 23:40

easwars added 4 commits September 27, 2021 08:46

fix resolver in load_balancing example

cee1b44

fix resolver in name_resolving example

d19cd9b

fix typo

0d0c1a5

make unix target canonical in test

c98608a

easwars mentioned this pull request Sep 29, 2021

[meta]: authority issues #4717

Closed

5 tasks

dfawley reviewed Sep 29, 2021

View reviewed changes

dfawley assigned easwars and unassigned dfawley Sep 29, 2021

menghanl reviewed Sep 29, 2021

View reviewed changes

easwars added 5 commits September 29, 2021 14:07

add Unparsed field to resolver.Target

fb4b9c0

make getServerName a method on cc instead of ac

2750421

some comments

c522792

use passthrough whenever custom dialer is used

4f56e2e

resolver builder

ae490d0

easwars assigned dfawley and unassigned menghanl and easwars Sep 29, 2021

strip leading "/" from endpoint for backward compatibility

99fdc43

easwars force-pushed the url_parse_attempt_3 branch from 7c4733f to 002aa36 Compare October 6, 2021 23:42

review comments

8546581

easwars force-pushed the url_parse_attempt_3 branch from 002aa36 to 8546581 Compare October 7, 2021 00:08

dfawley reviewed Oct 8, 2021

View reviewed changes

dfawley assigned easwars and unassigned dfawley Oct 8, 2021

easwars added 5 commits October 11, 2021 15:36

review comments

78fcb25

- make determineAuthority() a func instead of a method on ClientConn - return error from Dial() if authority from creds and dialOption do not match - simplify

edit todo for optional resolver interface

4fb05c3

ClientHandshake comments

c6057e5

deprecate old fields of resolver.Target and rename ParsedURL to URL

f230c53

validate parsed URL in unit tests

5f96946

easwars assigned dfawley and unassigned easwars Oct 11, 2021

dfawley approved these changes Oct 14, 2021

View reviewed changes

dfawley removed their assignment Oct 14, 2021

menghanl approved these changes Oct 14, 2021

View reviewed changes

easwars merged commit aaff9e7 into grpc:master Oct 14, 2021

easwars deleted the url_parse_attempt_3 branch October 14, 2021 21:54

howardjohn mentioned this pull request Oct 21, 2021

Under documented breaking change in unix domain handling in v1.42 #4894

Closed

hexfusion mentioned this pull request Nov 24, 2021

*: Upgrade to use go.opentelemetry.io/otel@v1.2.0 etcd-io/etcd#13498

Merged

nak3 mentioned this pull request Nov 25, 2021

Do not append the port to authority knative/serving#12350

Merged

yamamoto-febc mentioned this pull request Dec 9, 2021

アドレスの解析処理の改善 sacloud/autoscaler#245

Closed

This was referenced Dec 10, 2021

Setting just servername to :authority pseudo header in client when using TLS. moby/buildkit#2518

Merged

Possible regression in client credential validation/handling? moby/buildkit#2510

Closed

github-actions bot locked as resolved and limited conversation to collaborators Apr 27, 2022

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

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

grpc: better RFC 3986 compliant target parsing #4817

grpc: better RFC 3986 compliant target parsing #4817

Conversation

easwars commented Sep 24, 2021 • edited by menghanl Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Sep 29, 2021

menghanl commented Oct 6, 2021

easwars commented Oct 7, 2021

easwars commented Oct 7, 2021

easwars commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Sep 24, 2021 •

edited by menghanl

Loading