client: fix "unix" scheme handling for some corner cases #4021

GarrettGutierrez1 · 2020-11-09T17:09:16Z

Fixes #3990 .

dfawley

This appears to be missing one case: passthrough:///unix:///a/b/c needs to be handled by the transport's default dialer (i.e. it sees unix: at the start and behaves as it used to before #3890)

dfawley · 2020-11-09T23:53:22Z

internal/grpcutil/target.go

@@ -60,9 +60,17 @@ func ParseTarget(target string, skipUnixColonParsing bool) (ret resolver.Target)
 		return resolver.Target{Endpoint: target}
 	}
 	if ret.Scheme == "unix" {
+		// Prevents behavior change in "unix:///[...]" case.
+		if skipUnixColonParsing && ret.Authority == "" {


Why does this check skipUnixColonParsing? That only impacts unix:<path>, right?

The check is still there just moved (and'd with the =="unix" check). This is so I don't append a "/" to the address when it isn't going to the resolver, since that is only so the resolver gets the right address.

dfawley · 2020-11-09T23:55:16Z

internal/grpcutil/target_test.go

+		{targetStr: "unix:/a/b/c", want: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}, wantWithDialer: resolver.Target{Scheme: "", Authority: "", Endpoint: "unix:/a/b/c"}},
+		{targetStr: "unix:///a/b/c", want: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}, wantWithDialer: resolver.Target{Scheme: "", Authority: "", Endpoint: "unix:///a/b/c"}},
+
+		{targetStr: "passthrough:///unix:///a/b/c", want: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}, wantWithDialer: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "unix:///a/b/c"}},


I think this should always be Scheme: "passthrough", Endpoint: "unix:///a/b/c"

Fixed this, that is what it is in both cases now.

It looks like you have omitted wantWithDialer now.

With wantWithDialer omitted, the test sets it to want and just expects the same thing in both cases.

Oh.. that makes sense. Thanks.

dfawley · 2020-11-11T23:43:49Z

internal/grpcutil/target.go

 	return ret
 }
+
+// ParseDialTarget returns the network and address to pass to dialer
+func ParseDialTarget(target string) (string, string) {


Can you move this into the transport?

Moved that to transport.

dfawley · 2020-11-11T23:44:08Z

internal/grpcutil/target_test.go

+		{targetStr: "unix:/a/b/c", want: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}, wantWithDialer: resolver.Target{Scheme: "", Authority: "", Endpoint: "unix:/a/b/c"}},
+		{targetStr: "unix:///a/b/c", want: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}, wantWithDialer: resolver.Target{Scheme: "", Authority: "", Endpoint: "unix:///a/b/c"}},
+
+		{targetStr: "passthrough:///unix:///a/b/c", want: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}, wantWithDialer: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "unix:///a/b/c"}},


Oh.. that makes sense. Thanks.

dfawley · 2020-11-12T00:21:36Z

internal/grpcutil/target.go

+			ret.Endpoint = "/" + ret.Endpoint
+		} else {
+			// Custom dialer should receive "unix:///[...]".
+			return resolver.Target{Endpoint: target}


This should not be here; instead make the transport invoke the user's dialer after prepending "unix://" to the address string.

Removed that part and am now appending to the address in transport.

dfawley

Please add an end-to-end test that uses a custom dialer with "unix:///" and "passthrough:///unix:///" to confirm everything is working correctly ("unix:///" appears in the address passed to the dialer for both).

dfawley · 2020-11-12T18:32:34Z

internal/grpcutil/target_test.go

@@ -70,7 +70,7 @@ func TestParseTargetString(t *testing.T) {
 		// If we can only parse part of the target.
 		{targetStr: "://", want: resolver.Target{Scheme: "", Authority: "", Endpoint: "://"}},
 		{targetStr: "unix://domain", want: resolver.Target{Scheme: "", Authority: "", Endpoint: "unix://domain"}},
-		{targetStr: "unix://a/b/c", want: resolver.Target{Scheme: "unix", Authority: "a", Endpoint: "/b/c"}, wantWithDialer: resolver.Target{Scheme: "unix", Authority: "a", Endpoint: "b/c"}},
+		{targetStr: "unix://a/b/c", want: resolver.Target{Scheme: "unix", Authority: "a", Endpoint: "b/c"}},


This should be Endpoint: "/b/c" since we have the 3-slashes form. It doesn't matter in practice since the authority is non-empty, so it will be an error either way, but that's how it should be.

It's "/b/c" now due to the revert of the +"/" change.

dfawley · 2020-11-12T18:36:12Z

internal/transport/http2_client.go

+	n, ok := networktype.Get(addr)
+	if fn != nil {
+		if ok && n == "unix" {
+			return fn(ctx, "unix:///"+address)


This deserves a comment. Something like: 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". Re-add "unix://" here since we now support the "unix" scheme by default, which strips this prefix.

Added this comment.

dfawley · 2020-11-12T18:36:57Z

internal/transport/http2_client.go

-	if n, ok := networktype.Get(addr); ok {
+	n, ok := networktype.Get(addr)
+	if fn != nil {
+		if ok && n == "unix" {


ok is redundant - if !ok, n will be ""

Got rid of redundant ok

dfawley · 2020-11-12T18:41:46Z

internal/transport/http2_client.go

 	networkType := "tcp"
 	address := addr.Addr
-	if n, ok := networktype.Get(addr); ok {
+	n, ok := networktype.Get(addr)


networkType, ok := ... - the "tcp" default is no longer used (see below if !ok).

Made this change.

dfawley · 2020-11-12T18:43:04Z

internal/grpcutil/target.go

-	if ret.Scheme == "unix" {
-		// Add the "/" back in the unix case, so the unix resolver receives the
-		// actual endpoint.
+	if ret.Scheme == "unix" && !skipUnixColonParsing && ret.Authority == "" {


Did this need to change? I think this can be reverted.

Reverted this.

dfawley

Please add an end-to-end test that uses a custom dialer with "unix:///" and "passthrough:///unix:///" to confirm everything is working correctly ("unix:///" appears in the address passed to the dialer for both).

GarrettGutierrez1 · 2020-11-12T20:33:46Z

Please add an end-to-end test that uses a custom dialer with "unix:///" and "passthrough:///unix:///" to confirm everything is working correctly ("unix:///" appears in the address passed to the dialer for both).

End-to-end test added. Only oddity is authority in "passthrough:///unix:///path" case is "unix:///path" which I confirmed is the case before the change. This is because the result of the target parsing is {Scheme: "passthrough", Authority: "", Endpoint: "unix:///path"} and then DialContext() executes an else condition that sets the authority to the endpoint.

dfawley · 2020-11-17T23:38:40Z

internal/grpcutil/target_test.go

+		// behaviors with a custom dialer, to prevent behavior changes with custom dialers.
+		{targetStr: "unix:a/b/c", want: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c"}, wantWithDialer: resolver.Target{Scheme: "", Authority: "", Endpoint: "unix:a/b/c"}},
+		{targetStr: "unix:/a/b/c", want: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}, wantWithDialer: resolver.Target{Scheme: "", Authority: "", Endpoint: "unix:/a/b/c"}},
+		{targetStr: "unix:///a/b/c", want: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}, wantWithDialer: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}},


Delete wantWithDialer?

Made this change.

dfawley · 2020-11-17T23:40:56Z

test/authority_test.go

+	expectedTarget string
+}
+
+func tests() []authorityTest {


Use a variable not a function. Also, need a better name than tests since this is package-global.

OR, make this non-global and put it in TestUnix, with a runUnixCustomDialerTest or something for the other tests.

Made it a variable named authorityTests

dfawley · 2020-11-17T23:44:08Z

test/authority_test.go

+	address        string
+	target         string
+	authority      string
+	expectedTarget string


Go uses "want". How about dialTargetWant?

Changed it to dialTargetWant

stanhu · 2021-01-06T07:06:18Z

Thanks, I tried to update to grpc-go v1.34.0, but tests were failing due to #3990. I verified that c456688 verified fixed the problem. Would you mind tagging an update with this?

Fixed unix behavior in various cases

06c753c

GarrettGutierrez1 added this to the 1.34 Release milestone Nov 9, 2020

dfawley reviewed Nov 9, 2020

View reviewed changes

GarrettGutierrez1 added 2 commits November 11, 2020 13:12

Handle passthrough case in transport

b9b89fd

Removed redundant condition

8fbca88

dfawley assigned GarrettGutierrez1 Nov 11, 2020

Refined triple slash case

62ccdd1

dfawley requested changes Nov 12, 2020

View reviewed changes

Handle unix:///a/b/c in transport dialer

e3221bd

dfawley requested changes Nov 12, 2020

View reviewed changes

End to end test added

69696f9

easwars assigned dfawley and unassigned GarrettGutierrez1 Nov 12, 2020

Merge branch 'master' into unix-type-handling

ecbb2d3

dfawley changed the title ~~Handle "unix" type when custom dialer is set~~ client: fix "unix" scheme handling for some corner cases Nov 12, 2020

GarrettGutierrez1 added 2 commits November 12, 2020 13:53

Merge branch 'master' into unix-type-handling

7472fbc

Comment to test

e602bd7

menghanl modified the milestones: 1.34 Release, 1.35 Release Nov 17, 2020

dfawley reviewed Nov 17, 2020

View reviewed changes

dfawley assigned GarrettGutierrez1 and unassigned dfawley Nov 17, 2020

rework tests

894d568

dfawley assigned dfawley and unassigned GarrettGutierrez1 Nov 24, 2020

dfawley approved these changes Nov 24, 2020

View reviewed changes

lisongmin mentioned this pull request Nov 28, 2020

The cpu of kubelet is always 100% after containerd.service restart kubernetes/kubernetes#95727

Closed

Merge branch 'master' into unix-type-handling

72a6753

GarrettGutierrez1 merged commit c456688 into grpc:master Nov 30, 2020

davidkhala pushed a commit to Hyperledger-TWGC/grpc that referenced this pull request Dec 7, 2020

client: fix "unix" scheme handling for some corner cases (grpc#4021)

2c6e737

dfawley added the Cherry Pick label Jan 6, 2021

menghanl pushed a commit that referenced this pull request Jan 11, 2021

client: fix "unix" scheme handling for some corner cases (#4021)

39242ac

menghanl removed this from the 1.35 Release milestone Jan 11, 2021

menghanl removed the Cherry Pick label Jan 11, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jul 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: fix "unix" scheme handling for some corner cases #4021

client: fix "unix" scheme handling for some corner cases #4021

GarrettGutierrez1 commented Nov 9, 2020

dfawley left a comment

dfawley Nov 9, 2020

GarrettGutierrez1 Nov 11, 2020

dfawley Nov 9, 2020

GarrettGutierrez1 Nov 11, 2020

dfawley Nov 11, 2020

GarrettGutierrez1 Nov 11, 2020

dfawley Nov 11, 2020

dfawley Nov 11, 2020

GarrettGutierrez1 Nov 12, 2020

dfawley Nov 11, 2020

dfawley Nov 12, 2020

GarrettGutierrez1 Nov 12, 2020

dfawley left a comment

dfawley Nov 12, 2020

GarrettGutierrez1 Nov 12, 2020

dfawley Nov 12, 2020

GarrettGutierrez1 Nov 12, 2020

dfawley Nov 12, 2020

GarrettGutierrez1 Nov 12, 2020

dfawley Nov 12, 2020

GarrettGutierrez1 Nov 12, 2020

dfawley Nov 12, 2020

GarrettGutierrez1 Nov 12, 2020

dfawley left a comment

GarrettGutierrez1 commented Nov 12, 2020

dfawley Nov 17, 2020

GarrettGutierrez1 Nov 20, 2020

dfawley Nov 17, 2020

GarrettGutierrez1 Nov 20, 2020

dfawley Nov 17, 2020

GarrettGutierrez1 Nov 20, 2020

stanhu commented Jan 6, 2021

client: fix "unix" scheme handling for some corner cases #4021

client: fix "unix" scheme handling for some corner cases #4021

Conversation

GarrettGutierrez1 commented Nov 9, 2020

dfawley left a 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

dfawley left a 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

dfawley left a comment

Choose a reason for hiding this comment

GarrettGutierrez1 commented Nov 12, 2020

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

stanhu commented Jan 6, 2021