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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
06c753c
Fixed unix behavior in various cases
GarrettGutierrez1 Nov 9, 2020
b9b89fd
Handle passthrough case in transport
GarrettGutierrez1 Nov 11, 2020
8fbca88
Removed redundant condition
GarrettGutierrez1 Nov 11, 2020
62ccdd1
Refined triple slash case
GarrettGutierrez1 Nov 12, 2020
e3221bd
Handle unix:///a/b/c in transport dialer
GarrettGutierrez1 Nov 12, 2020
69696f9
End to end test added
GarrettGutierrez1 Nov 12, 2020
ecbb2d3
Merge branch 'master' into unix-type-handling
GarrettGutierrez1 Nov 12, 2020
7472fbc
Merge branch 'master' into unix-type-handling
GarrettGutierrez1 Nov 12, 2020
e602bd7
Comment to test
GarrettGutierrez1 Nov 12, 2020
894d568
rework tests
GarrettGutierrez1 Nov 20, 2020
72a6753
Merge branch 'master' into unix-type-handling
GarrettGutierrez1 Nov 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions internal/grpcutil/target.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

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.

}
// Add the "/" back in the unix case, so the unix resolver receives the
// actual endpoint.
ret.Endpoint = "/" + ret.Endpoint
}
// Prevents behavior change in "passthrough:///unix:///a/b/c" case.
if !skipUnixColonParsing && ret.Scheme == "passthrough" && strings.HasPrefix(ret.Endpoint, "unix:") {
return ParseTarget(ret.Endpoint, false)
}
return ret
}
12 changes: 8 additions & 4 deletions internal/grpcutil/target_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,17 +70,21 @@ 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"}},
{targetStr: "a:b", want: resolver.Target{Scheme: "", Authority: "", Endpoint: "a:b"}},
{targetStr: "a/b", want: resolver.Target{Scheme: "", Authority: "", Endpoint: "a/b"}},
{targetStr: "a:/b", want: resolver.Target{Scheme: "", Authority: "", Endpoint: "a:/b"}},
{targetStr: "a//b", want: resolver.Target{Scheme: "", Authority: "", Endpoint: "a//b"}},
{targetStr: "a://b", want: resolver.Target{Scheme: "", Authority: "", Endpoint: "a://b"}},

// Unix cases without custom dialer.
// unix:[local_path] and unix:[/absolute] have different behaviors with
// a custom dialer, to prevent behavior changes with custom dialers.
{targetStr: "unix:domain", want: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "domain"}, wantWithDialer: resolver.Target{Scheme: "", Authority: "", Endpoint: "unix:domain"}},
{targetStr: "unix:/domain", want: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/domain"}, wantWithDialer: resolver.Target{Scheme: "", Authority: "", Endpoint: "unix:/domain"}},
// unix:[local_path], unix:[/absolute], and unix://[/absolute] have different
// 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: "", 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.

} {
got := ParseTarget(test.targetStr, false)
if got != test.want {
Expand Down
5 changes: 5 additions & 0 deletions internal/resolver/unix/unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
package unix

import (
"fmt"

"google.golang.org/grpc/internal/transport/networktype"
"google.golang.org/grpc/resolver"
)
Expand All @@ -29,6 +31,9 @@ const scheme = "unix"
type builder struct{}

func (*builder) Build(target resolver.Target, cc resolver.ClientConn, _ resolver.BuildOptions) (resolver.Resolver, error) {
if target.Authority != "" {
return nil, fmt.Errorf("invalid (non-empty) authority: %v", target.Authority)
}
cc.UpdateState(resolver.State{Addresses: []resolver.Address{networktype.Set(resolver.Address{Addr: target.Endpoint}, "unix")}})
return &nopResolver{}, nil
}
Expand Down