Skip to content

Commit

Permalink
Delete resolver.Target.Scheme and resolver.Target.Authority
Browse files Browse the repository at this point in the history
  • Loading branch information
@ginayeh
ginayeh committed Jun 9, 2023
1 parent 7a7caf3 commit 32bf00f
Show file tree
Hide file tree
Showing 8 changed files with 35 additions and 81 deletions.
10 changes: 3 additions & 7 deletions clientconn.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1807,19 +1807,15 @@ func (cc *ClientConn) parseTargetAndFindResolver() error {
}

// parseTarget uses RFC 3986 semantics to parse the given target into a
// resolver.Target struct containing scheme, authority and url. Query
// params are stripped from the endpoint.
// resolver.Target struct containing url. Query params are stripped from the
// endpoint.
func parseTarget(target string) (resolver.Target, error) {
u, err := url.Parse(target)
if err != nil {
return resolver.Target{}, err
}

return resolver.Target{
Scheme: u.Scheme,
Authority: u.Host,
URL: *u,
}, nil
return resolver.Target{URL: *u}, nil
}

// Determine channel authority. The order of precedence is as follows:
Expand Down
75 changes: 20 additions & 55 deletions clientconn_parsed_target_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"errors"
"fmt"
"net"
"net/url"
"testing"
"time"

Expand All @@ -42,46 +41,22 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {
wantParsed resolver.Target
}{
// No scheme is specified.
{target: "://", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "://"))}},
{target: ":///", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, ":///"))}},
{target: "://a/", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "://a/"))}},
{target: ":///a", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, ":///a"))}},
{target: "://a/b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "://a/b"))}},
{target: "/", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "/"))}},
{target: "a/b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a/b"))}},
{target: "a//b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a//b"))}},
{target: "google.com", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "google.com"))}},
{target: "google.com/?a=b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "google.com/?a=b"))}},
{target: "/unix/socket/address", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "/unix/socket/address"))}},
{target: "://a/b", badScheme: true, wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "://a/b"))}},
{target: "a//b", badScheme: true, wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a//b"))}},

// An unregistered scheme is specified.
{target: "a:///", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a:///"))}},
{target: "a://b/", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a://b/"))}},
{target: "a:///b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a:///b"))}},
{target: "a://b/c", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a://b/c"))}},
{target: "a:b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a:b"))}},
{target: "a:/b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a:/b"))}},
{target: "a://b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a://b"))}},
{target: "a:///", badScheme: true, wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a:///"))}},
{target: "a:b", badScheme: true, wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a:b"))}},

// A registered scheme is specified.
{target: "dns:///google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "", URL: *testutils.MustParseURL("dns:///google.com")}},
{target: "dns://a.server.com/google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", URL: *testutils.MustParseURL("dns://a.server.com/google.com")}},
{target: "dns://a.server.com/google.com/?a=b", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", URL: *testutils.MustParseURL("dns://a.server.com/google.com/?a=b")}},
{target: "unix:///a/b/c", wantParsed: resolver.Target{Scheme: "unix", Authority: "", URL: *testutils.MustParseURL("unix:///a/b/c")}},
{target: "unix-abstract:a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", URL: *testutils.MustParseURL("unix-abstract:a/b/c")}},
{target: "unix-abstract:a b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", URL: *testutils.MustParseURL("unix-abstract:a b")}},
{target: "unix-abstract:a:b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", URL: *testutils.MustParseURL("unix-abstract:a:b")}},
{target: "unix-abstract:a-b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", URL: *testutils.MustParseURL("unix-abstract:a-b")}},
{target: "unix-abstract:/ a///://::!@#$%25^&*()b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", URL: *testutils.MustParseURL("unix-abstract:/ a///://::!@#$%25^&*()b")}},
{target: "unix-abstract:passthrough:abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", URL: *testutils.MustParseURL("unix-abstract:passthrough:abc")}},
{target: "unix-abstract:unix:///abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", URL: *testutils.MustParseURL("unix-abstract:unix:///abc")}},
{target: "unix-abstract:///a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", URL: *testutils.MustParseURL("unix-abstract:///a/b/c")}},
{target: "unix-abstract:///", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", URL: *testutils.MustParseURL("unix-abstract:///")}},
{target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", URL: *testutils.MustParseURL("passthrough:///unix:///a/b/c")}},
{target: "dns://a.server.com/google.com", wantParsed: resolver.Target{URL: *testutils.MustParseURL("dns://a.server.com/google.com")}},
{target: "unix-abstract:/ a///://::!@#$%25^&*()b", wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:/ a///://::!@#$%25^&*()b")}},
{target: "unix-abstract:passthrough:abc", wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:passthrough:abc")}},
{target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{URL: *testutils.MustParseURL("passthrough:///unix:///a/b/c")}},

// Cases for `scheme:absolute-path`.
{target: "dns:/a/b/c", wantParsed: resolver.Target{Scheme: "dns", Authority: "", URL: *testutils.MustParseURL("dns:/a/b/c")}},
{target: "unregistered:/a/b/c", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL("unregistered:/a/b/c")}},
{target: "dns:/a/b/c", wantParsed: resolver.Target{URL: *testutils.MustParseURL("dns:/a/b/c")}},
{target: "unregistered:/a/b/c", badScheme: true, wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "unregistered:/a/b/c"))}},
}

for _, test := range tests {
Expand All @@ -90,11 +65,6 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {
if test.badScheme {
target = defScheme + ":///" + target
}
url, err := url.Parse(target)
if err != nil {
t.Fatalf("Unexpected error parsing URL: %v", err)
}
test.wantParsed.URL = *url

cc, err := Dial(test.target, WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
Expand Down Expand Up @@ -140,56 +110,56 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) {
// different behaviors with a custom dialer.
{
target: "unix:a/b/c",
wantParsed: resolver.Target{Scheme: "unix", Authority: "", URL: *testutils.MustParseURL("unix:a/b/c")},
wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix:a/b/c")},
wantDialerAddress: "unix:a/b/c",
},
{
target: "unix:/a/b/c",
wantParsed: resolver.Target{Scheme: "unix", Authority: "", URL: *testutils.MustParseURL("unix:/a/b/c")},
wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix:/a/b/c")},
wantDialerAddress: "unix:///a/b/c",
},
{
target: "unix:///a/b/c",
wantParsed: resolver.Target{Scheme: "unix", Authority: "", URL: *testutils.MustParseURL("unix:///a/b/c")},
wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix:///a/b/c")},
wantDialerAddress: "unix:///a/b/c",
},
{
target: "dns:///127.0.0.1:50051",
wantParsed: resolver.Target{Scheme: "dns", Authority: "", URL: *testutils.MustParseURL("dns:///127.0.0.1:50051")},
wantParsed: resolver.Target{URL: *testutils.MustParseURL("dns:///127.0.0.1:50051")},
wantDialerAddress: "127.0.0.1:50051",
},
{
target: ":///127.0.0.1:50051",
badScheme: true,
wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, ":///127.0.0.1:50051"))},
wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, ":///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", URL: *testutils.MustParseURL("dns://authority/127.0.0.1:50051")},
wantParsed: resolver.Target{URL: *testutils.MustParseURL("dns://authority/127.0.0.1:50051")},
wantDialerAddress: "127.0.0.1:50051",
},
{
target: "://authority/127.0.0.1:50051",
badScheme: true,
wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "://authority/127.0.0.1:50051"))},
wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "://authority/127.0.0.1:50051"))},
wantDialerAddress: "://authority/127.0.0.1:50051",
},
{
target: "/unix/socket/address",
badScheme: true,
wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "/unix/socket/address"))},
wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "/unix/socket/address"))},
wantDialerAddress: "/unix/socket/address",
},
{
target: "",
badScheme: true,
wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, ""))},
wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, ""))},
wantDialerAddress: "",
},
{
target: "passthrough://a.server.com/google.com",
wantParsed: resolver.Target{Scheme: "passthrough", Authority: "a.server.com", URL: *testutils.MustParseURL("passthrough://a.server.com/google.com")},
wantParsed: resolver.Target{URL: *testutils.MustParseURL("passthrough://a.server.com/google.com")},
wantDialerAddress: "google.com",
},
}
Expand All @@ -200,11 +170,6 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) {
if test.badScheme {
target = defScheme + ":///" + target
}
url, err := url.Parse(target)
if err != nil {
t.Fatalf("Unexpected error parsing URL: %v", err)
}
test.wantParsed.URL = *url

addrCh := make(chan string, 1)
dialer := func(ctx context.Context, address string) (net.Conn, error) {
Expand Down
12 changes: 6 additions & 6 deletions internal/resolver/dns/dns_resolver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,14 @@ var (
minDNSResRate = 30 * time.Second
)

var customAuthorityDialler = func(authority string) func(ctx context.Context, network, address string) (net.Conn, error) {
return func(ctx context.Context, network, address string) (net.Conn, error) {
var addressDialer = func(address string) func(context.Context, string, string) (net.Conn, error) {
return func(ctx context.Context, network, _ string) (net.Conn, error) {
var dialer net.Dialer
return dialer.DialContext(ctx, network, authority)
return dialer.DialContext(ctx, network, address)
}
}

var customAuthorityResolver = func(authority string) (netResolver, error) {
var newNetResolver = func(authority string) (netResolver, error) {
host, port, err := parseTarget(authority, defaultDNSSvrPort)
if err != nil {
return nil, err
Expand All @@ -103,7 +103,7 @@ var customAuthorityResolver = func(authority string) (netResolver, error) {

return &net.Resolver{
PreferGo: true,
Dial: customAuthorityDialler(authorityWithPort),
Dial: addressDialer(authorityWithPort),
}, nil
}

Expand Down Expand Up @@ -143,7 +143,7 @@ func (b *dnsBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts
if target.URL.Host == "" {
d.resolver = defaultResolver
} else {
d.resolver, err = customAuthorityResolver(target.URL.Host)
d.resolver, err = newNetResolver(target.URL.Host)
if err != nil {
return nil, err
}
Expand Down
9 changes: 4 additions & 5 deletions internal/resolver/dns/dns_resolver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1419,14 +1419,14 @@ func TestCustomAuthority(t *testing.T) {
true,
},
}
oldCustomAuthorityDialler := customAuthorityDialler
oldAddressDialer := addressDialer
defer func() {
customAuthorityDialler = oldCustomAuthorityDialler
addressDialer = oldAddressDialer
}()

for _, a := range tests {
errChan := make(chan error, 1)
customAuthorityDialler = func(authority string) func(ctx context.Context, network, address string) (net.Conn, error) {
addressDialer = func(authority string) func(ctx context.Context, network, address string) (net.Conn, error) {
if authority != a.authorityWant {
errChan <- fmt.Errorf("wrong custom authority passed to resolver. input: %s expected: %s actual: %s", a.authority, a.authorityWant, authority)
} else {
Expand All @@ -1441,8 +1441,7 @@ func TestCustomAuthority(t *testing.T) {
b := NewBuilder()
cc := &testClientConn{target: mockEndpointTarget, errChan: make(chan error, 1)}
target := resolver.Target{
Authority: a.authority,
URL: *testutils.MustParseURL(fmt.Sprintf("scheme://%s/%s", a.authority, mockEndpointTarget)),
URL: *testutils.MustParseURL(fmt.Sprintf("scheme://%s/%s", a.authority, mockEndpointTarget)),
}
r, err := b.Build(target, cc, resolver.BuildOptions{})

Expand Down
4 changes: 0 additions & 4 deletions resolver/resolver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,10 +264,6 @@ type ClientConn interface {
// - "unknown_scheme://authority/endpoint"
// Target{Scheme: resolver.GetDefaultScheme(), Endpoint: "unknown_scheme://authority/endpoint"}
type Target struct {
// Deprecated: use URL.Scheme instead.
Scheme string
// Deprecated: use URL.Host instead.
Authority string
// URL contains the parsed dial target with an optional default scheme added
// to it if the original dial target contained no scheme or contained an
// unregistered scheme. Any query params specified in the original dial
Expand Down
2 changes: 0 additions & 2 deletions xds/googledirectpath/googlec2p.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ func (c2pResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, opts

if !runDirectPath() {
// If not xDS, fallback to DNS.
t.Scheme = dnsName
t.URL.Scheme = dnsName
return resolver.Get(dnsName).Build(t, cc, opts)
}
Expand Down Expand Up @@ -144,7 +143,6 @@ func (c2pResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, opts
}

// Create and return an xDS resolver.
t.Scheme = xdsName
t.URL.Scheme = xdsName
if envconfig.XDSFederation {
t = resolver.Target{
Expand Down
2 changes: 1 addition & 1 deletion xds/internal/balancer/clusterresolver/priority_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,7 @@ func (s) TestFallbackToDNS(t *testing.T) {
defer ctxCancel()
select {
case target := <-dnsTargetCh:
if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", URL: *testutils.MustParseURL("dns:///" + testDNSTarget)}); diff != "" {
if diff := cmp.Diff(target, resolver.Target{URL: *testutils.MustParseURL("dns:///" + testDNSTarget)}); diff != "" {
t.Fatalf("got unexpected DNS target to watch, diff (-got, +want): %v", diff)
}
case <-ctx.Done():
Expand Down
2 changes: 1 addition & 1 deletion xds/internal/balancer/clusterresolver/resource_resolver_dns.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func newDNSResolver(target string, topLevelResolver topLevelResolver) *dnsDiscov
return ret
}

r, err := newDNS(resolver.Target{Scheme: "dns", URL: *u}, ret, resolver.BuildOptions{})
r, err := newDNS(resolver.Target{URL: *u}, ret, resolver.BuildOptions{})
if err != nil {
topLevelResolver.onError(fmt.Errorf("failed to build DNS resolver for target %q: %v", target, err))
return ret
Expand Down

0 comments on commit 32bf00f

Please sign in to comment.