grpc: cleanup parse target and authority tests #4787
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| /* | ||
| * | ||
| * Copyright 2021 gRPC authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * | ||
| */ | ||
|
|
||
| package grpc | ||
|
|
||
| import ( | ||
| "context" | ||
| "net" | ||
| "testing" | ||
|
|
||
| "google.golang.org/grpc/credentials" | ||
| "google.golang.org/grpc/testdata" | ||
| ) | ||
|
|
||
| func (s) TestClientConnAuthority(t *testing.T) { | ||
| serverNameOverride := "over.write.server.name" | ||
| creds, err := credentials.NewClientTLSFromFile(testdata.Path("x509/server_ca_cert.pem"), serverNameOverride) | ||
| if err != nil { | ||
| t.Fatalf("credentials.NewClientTLSFromFile(_, %q) failed: %v", err, serverNameOverride) | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| target string | ||
| opts []DialOption | ||
| wantAuthority string | ||
| }{ | ||
| { | ||
| name: "default", | ||
| target: "Non-Existent.Server:8080", | ||
| opts: []DialOption{WithInsecure()}, | ||
| wantAuthority: "Non-Existent.Server:8080", | ||
| }, | ||
| { | ||
| name: "override-via-creds", | ||
| target: "Non-Existent.Server:8080", | ||
| opts: []DialOption{WithTransportCredentials(creds)}, | ||
| wantAuthority: serverNameOverride, | ||
| }, | ||
| { | ||
| name: "override-via-WithAuthority", | ||
| target: "Non-Existent.Server:8080", | ||
| opts: []DialOption{WithInsecure(), WithAuthority("authority-override")}, | ||
| wantAuthority: "authority-override", | ||
| }, | ||
| { | ||
| name: "override-via-creds-and-WithAuthority", | ||
| target: "Non-Existent.Server:8080", | ||
| // WithAuthority override works only for insecure creds. | ||
| opts: []DialOption{WithTransportCredentials(creds), WithAuthority("authority-override")}, | ||
| wantAuthority: serverNameOverride, | ||
| }, | ||
| { | ||
| name: "unix relative", | ||
| target: "unix:sock.sock", | ||
| opts: []DialOption{WithInsecure()}, | ||
| wantAuthority: "localhost", | ||
| }, | ||
| { | ||
| name: "unix relative with custom dialer", | ||
| target: "unix:sock.sock", | ||
| opts: []DialOption{WithInsecure(), WithContextDialer(func(ctx context.Context, addr string) (net.Conn, error) { | ||
| return (&net.Dialer{}).DialContext(ctx, "", addr) | ||
| })}, | ||
| wantAuthority: "localhost", | ||
| }, | ||
| { | ||
| name: "unix absolute", | ||
| target: "unix:/sock.sock", | ||
| opts: []DialOption{WithInsecure()}, | ||
| wantAuthority: "localhost", | ||
| }, | ||
| { | ||
| name: "unix absolute with custom dialer", | ||
| target: "unix:///sock.sock", | ||
| opts: []DialOption{WithInsecure(), WithContextDialer(func(ctx context.Context, addr string) (net.Conn, error) { | ||
| return (&net.Dialer{}).DialContext(ctx, "", addr) | ||
| })}, | ||
| wantAuthority: "localhost", | ||
| }, | ||
| { | ||
| name: "localhost colon port", | ||
| target: "localhost:50051", | ||
| opts: []DialOption{WithInsecure()}, | ||
| wantAuthority: "localhost:50051", | ||
| }, | ||
| { | ||
| name: "colon port", | ||
| target: ":50051", | ||
| opts: []DialOption{WithInsecure()}, | ||
| wantAuthority: "localhost:50051", | ||
| }, | ||
| } | ||
|
|
||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| cc, err := Dial(test.target, test.opts...) | ||
| if err != nil { | ||
| t.Fatalf("Dial(%q) failed: %v", test.target, err) | ||
| } | ||
| defer cc.Close() | ||
| if cc.authority != test.wantAuthority { | ||
| t.Fatalf("cc.authority = %q, want %q", cc.authority, test.wantAuthority) | ||
| } | ||
| }) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,183 @@ | ||
| /* | ||
| * | ||
| * Copyright 2021 gRPC authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * | ||
| */ | ||
|
|
||
| package grpc | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "net" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "google.golang.org/grpc/resolver" | ||
| ) | ||
|
|
||
| func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { | ||
| defScheme := resolver.GetDefaultScheme() | ||
| tests := []struct { | ||
| target string | ||
| wantParsed resolver.Target | ||
| }{ | ||
| // No scheme is specified. | ||
| {target: "", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ""}}, | ||
| {target: "://", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://"}}, | ||
| {target: ":///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///"}}, | ||
| {target: "://a/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://a/"}}, | ||
| {target: ":///a", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///a"}}, | ||
| {target: "://a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://a/b"}}, | ||
| {target: "/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/"}}, | ||
| {target: "a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a/b"}}, | ||
| {target: "a//b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a//b"}}, | ||
| {target: "google.com", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com"}}, | ||
| {target: "google.com/?a=b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com/?a=b"}}, | ||
| {target: "/unix/socket/address", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/unix/socket/address"}}, | ||
|
|
||
| // An unregistered scheme is specified. | ||
| {target: "a:///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:///"}}, | ||
| {target: "a://b/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b/"}}, | ||
| {target: "a:///b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:///b"}}, | ||
| {target: "a://b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b/c"}}, | ||
| {target: "a:b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:b"}}, | ||
| {target: "a:/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:/b"}}, | ||
| {target: "a://b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b"}}, | ||
|
|
||
| // A registered scheme is specified. | ||
| {target: "dns:///google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "google.com"}}, | ||
| {target: "dns://a.server.com/google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "google.com"}}, | ||
| {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: "unix-abstract:a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a/b/c"}}, | ||
| {target: "unix-abstract:a b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a b"}}, | ||
| {target: "unix-abstract:a:b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a:b"}}, | ||
| {target: "unix-abstract:a-b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a-b"}}, | ||
| {target: "unix-abstract:/ a///://::!@#$%^&*()b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "/ a///://::!@#$%^&*()b"}}, | ||
| {target: "unix-abstract:passthrough:abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "passthrough:abc"}}, | ||
| {target: "unix-abstract:unix:///abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "unix:///abc"}}, | ||
| {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: "/"}}, | ||
| {target: "unix-abstract://authority", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "//authority"}}, | ||
| {target: "unix://domain", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unix://domain"}}, | ||
| {target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "unix:///a/b/c"}}, | ||
| } | ||
|
|
||
| for _, test := range tests { | ||
| t.Run(test.target, func(t *testing.T) { | ||
| cc, err := Dial(test.target, WithInsecure()) | ||
| if err != nil { | ||
| t.Fatalf("Dial(%q) failed: %v", test.target, err) | ||
| } | ||
| defer cc.Close() | ||
|
|
||
| if gotParsed := cc.parsedTarget; gotParsed != test.wantParsed { | ||
| t.Errorf("cc.parsedTarget = %+v, want %+v", gotParsed, test.wantParsed) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func (s) TestParsedTarget_Failure_WithoutCustomDialer(t *testing.T) { | ||
| targets := []string{ | ||
| "unix://a/b/c", | ||
| "unix-abstract://authority/a/b/c", | ||
| } | ||
|
|
||
| for _, target := range targets { | ||
| t.Run(target, func(t *testing.T) { | ||
| if cc, err := Dial(target, WithInsecure()); err == nil { | ||
dfawley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| defer cc.Close() | ||
| t.Fatalf("Dial(%q) succeeded cc.parsedTarget = %+v, expected to fail", target, cc.parsedTarget) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func (s) TestParsedTarget_WithCustomDialer(t *testing.T) { | ||
| defScheme := resolver.GetDefaultScheme() | ||
| tests := []struct { | ||
| target string | ||
| wantParsed resolver.Target | ||
| wantDialerAddress string | ||
| }{ | ||
| // unix:[local_path], unix:[/absolute], and unix://[/absolute] have | ||
| // different behaviors with a custom dialer. | ||
| { | ||
| target: "unix:a/b/c", | ||
| wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unix:a/b/c"}, | ||
| wantDialerAddress: "unix:a/b/c", | ||
| }, | ||
| { | ||
| target: "unix:/a/b/c", | ||
| wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unix:/a/b/c"}, | ||
| wantDialerAddress: "unix:/a/b/c", | ||
| }, | ||
| { | ||
| target: "unix:///a/b/c", | ||
|
There was a problem hiding this comment. Should we test a few things that aren't There was a problem hiding this comment. We don't currently have any special behavior for non-unix schemes when a custom dialer is specified. Are you saying we should add tests for the dns scheme now and make sure things don't break when we make it the default? There was a problem hiding this comment. We probably shouldn't be changing any parsing from what we do today when a custom dialer is involved, not just There was a problem hiding this comment. Done. Added some tests for dns and "no scheme". |
||
| wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}, | ||
| wantDialerAddress: "unix:///a/b/c", | ||
| }, | ||
| { | ||
| target: "dns:///127.0.0.1:50051", | ||
| wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "127.0.0.1:50051"}, | ||
| wantDialerAddress: "127.0.0.1:50051", | ||
| }, | ||
| { | ||
| target: ":///127.0.0.1:50051", | ||
| wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///127.0.0.1:50051"}, | ||
| wantDialerAddress: ":///127.0.0.1:50051", | ||
| }, | ||
| { | ||
| target: "dns://authority/127.0.0.1:50051", | ||
| wantParsed: resolver.Target{Scheme: "dns", Authority: "authority", Endpoint: "127.0.0.1:50051"}, | ||
| wantDialerAddress: "127.0.0.1:50051", | ||
| }, | ||
| { | ||
| target: "://authority/127.0.0.1:50051", | ||
| wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://authority/127.0.0.1:50051"}, | ||
| wantDialerAddress: "://authority/127.0.0.1:50051", | ||
| }, | ||
| } | ||
|
|
||
| for _, test := range tests { | ||
| t.Run(test.target, func(t *testing.T) { | ||
| addrCh := make(chan string, 1) | ||
| dialer := func(ctx context.Context, address string) (net.Conn, error) { | ||
| addrCh <- address | ||
|
There was a problem hiding this comment. I'm slightly concerned this could lead to a goroutine hang if this gets called three times before we close the ClientConn. You might be able to stimulate this by putting in a long enough sleep into the test after the select where we read from the channel. Blocking on the context inside a select here should remove that possibility. Maybe this is practically impossible given the timings (a long enough backoff), but it still seems worthwhile. There was a problem hiding this comment. As per our offline discussion, the |
||
| return nil, errors.New("dialer error") | ||
| } | ||
|
|
||
| cc, err := Dial(test.target, WithInsecure(), WithContextDialer(dialer)) | ||
| if err != nil { | ||
| t.Fatalf("Dial(%q) failed: %v", test.target, err) | ||
| } | ||
| defer cc.Close() | ||
|
|
||
| select { | ||
| case addr := <-addrCh: | ||
| if addr != test.wantDialerAddress { | ||
| t.Fatalf("address in custom dialer is %q, want %q", addr, test.wantDialerAddress) | ||
| } | ||
| case <-time.After(time.Second): | ||
| t.Fatal("timeout when waiting for custom dialer to be invoked") | ||
| } | ||
| if gotParsed := cc.parsedTarget; gotParsed != test.wantParsed { | ||
| t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, gotParsed, test.wantParsed) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be
Scheme: "unix", Endpoint: "//domain"? This is what happens withunix-abstractabove.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are currently handling
unix-abstract://[authority/]endpointspecially ingrpcutil.ParseTarget. But we don't do the same forunixscheme. See this block of code: https://github.com/grpc/grpc-go/blob/master/internal/grpcutil/target.go#L79-L82But the
unixresolver (which owns both schemes) does not like any authority being specified and will fail theresolver.Buildcall if one is specified.So, when we move to using
url.Parse(), both these targets will parse as follows (without custom dialer):Scheme: "unix-abstract", Authority: "", Endpoint: "authority"andScheme: "unix", Authority: "", Endpoint: "domain"which is really the correct interpretation as per RFC 3986. And since we will also add a field to
resolver.Targetto store the original dial target specified by the user, we could do some extra handling in the unix resolver if required. What do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current behavior doesn't actually work with a non-custom dialer, so we should be able to make it do whatever we want. Well, not whatever we want, we should make sure it is compliant with https://github.com/grpc/grpc/blob/master/doc/naming.md.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.