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

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

Merged
merged 11 commits into from
Nov 30, 2020
Merged

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

merged 11 commits into from
Nov 30, 2020

Conversation

GarrettGutierrez1
Copy link
Contributor

Fixes #3990 .

@GarrettGutierrez1 GarrettGutierrez1 added this to the 1.34 Release milestone Nov 9, 2020
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

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)

@@ -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 == "" {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

It looks like you have omitted wantWithDialer now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh.. that makes sense. Thanks.

return ret
}

// ParseDialTarget returns the network and address to pass to dialer
func ParseDialTarget(target string) (string, string) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this into the transport?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved that to transport.

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

Choose a reason for hiding this comment

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

Oh.. that makes sense. Thanks.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

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

@@ -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"}},
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this comment.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got rid of redundant ok

networkType := "tcp"
address := addr.Addr
if n, ok := networktype.Get(addr); ok {
n, ok := networktype.Get(addr)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this change.

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 == "" {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted this.

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

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

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.

@easwars easwars assigned dfawley and unassigned GarrettGutierrez1 Nov 12, 2020
@dfawley dfawley changed the title Handle "unix" type when custom dialer is set client: fix "unix" scheme handling for some corner cases Nov 12, 2020
@menghanl menghanl modified the milestones: 1.34 Release, 1.35 Release Nov 17, 2020
// 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"}},
Copy link
Member

Choose a reason for hiding this comment

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

Delete wantWithDialer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this change.

expectedTarget string
}

func tests() []authorityTest {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it a variable named authorityTests

address string
target string
authority string
expectedTarget string
Copy link
Member

Choose a reason for hiding this comment

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

Go uses "want". How about dialTargetWant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to dialTargetWant

@dfawley dfawley assigned GarrettGutierrez1 and unassigned dfawley Nov 17, 2020
@dfawley dfawley assigned dfawley and unassigned GarrettGutierrez1 Nov 24, 2020
@GarrettGutierrez1 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
@stanhu
Copy link

stanhu commented Jan 6, 2021

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?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle "unix" type when custom dialer is set
4 participants