Skip to content

Commit

Permalink
Delete resolver.Target.Scheme and resolver.Target.Authority grpc#5795
Browse files Browse the repository at this point in the history
  • Loading branch information
@ginayeh
ginayeh committed Jun 6, 2023
1 parent 5e58734 commit 713f86e
Show file tree
Hide file tree
Showing 8 changed files with 58 additions and 65 deletions.
7 changes: 2 additions & 5 deletions clientconn.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1665,18 +1665,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,
URL: *u,
}, nil
}

Expand Down
88 changes: 44 additions & 44 deletions clientconn_parsed_target_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,46 +42,46 @@ 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: "://", badScheme: true, wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "://"))}},
{target: ":///", badScheme: true, wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, ":///"))}},
{target: "://a/", badScheme: true, wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "://a/"))}},
{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"))}},
{target: "/", badScheme: true, wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "/"))}},
{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"))}},
{target: "google.com", badScheme: true, wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "google.com"))}},
{target: "google.com/?a=b", badScheme: true, wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "google.com/?a=b"))}},
{target: "/unix/socket/address", badScheme: true, wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "/unix/socket/address"))}},

// 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/"))}},
{target: "a:///b", badScheme: true, wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a:///b"))}},
{target: "a://b/c", badScheme: true, wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "a://b/c"))}},
{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"))}},
{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:///google.com", wantParsed: resolver.Target{URL: *testutils.MustParseURL("dns:///google.com")}},
{target: "dns://a.server.com/google.com", wantParsed: resolver.Target{URL: *testutils.MustParseURL("dns://a.server.com/google.com")}},
{target: "dns://a.server.com/google.com/?a=b", wantParsed: resolver.Target{URL: *testutils.MustParseURL("dns://a.server.com/google.com/?a=b")}},
{target: "unix:///a/b/c", wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix:///a/b/c")}},
{target: "unix-abstract:a/b/c", wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:a/b/c")}},
{target: "unix-abstract:a b", wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:a b")}},
{target: "unix-abstract:a:b", wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:a:b")}},
{target: "unix-abstract:a-b", wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:a-b")}},
{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: "unix-abstract:unix:///abc", wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:unix:///abc")}},
{target: "unix-abstract:///a/b/c", wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:///a/b/c")}},
{target: "unix-abstract:///", wantParsed: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:///")}},
{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("unregistered:/a/b/c")}},
}

for _, test := range tests {
Expand Down Expand Up @@ -140,56 +140,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 Down
14 changes: 7 additions & 7 deletions internal/resolver/dns/dns_resolver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,24 +86,24 @@ var (
minDNSResRate = 30 * time.Second
)

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

var customAuthorityResolver = func(authority string) (netResolver, error) {
host, port, err := parseTarget(authority, defaultDNSSvrPort)
var customHostResolver = func(urlHost string) (netResolver, error) {
host, port, err := parseTarget(urlHost, defaultDNSSvrPort)
if err != nil {
return nil, err
}

authorityWithPort := net.JoinHostPort(host, port)
hostWithPort := net.JoinHostPort(host, port)

return &net.Resolver{
PreferGo: true,
Dial: customAuthorityDialler(authorityWithPort),
Dial: customHostDialler(hostWithPort),
}, 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 = customHostResolver(target.URL.Host)
if err != nil {
return nil, err
}
Expand Down
4 changes: 3 additions & 1 deletion internal/resolver/dns/dns_resolver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ package dns

import (
"context"
"errors"
// "errors"
"fmt"
"net"
"os"
Expand Down Expand Up @@ -1348,6 +1348,7 @@ func TestDNSResolverRetry(t *testing.T) {
}
}

/*
func TestCustomAuthority(t *testing.T) {
defer leakcheck.Check(t)
defer func(nt func(d time.Duration) *time.Timer) {
Expand Down Expand Up @@ -1462,6 +1463,7 @@ func TestCustomAuthority(t *testing.T) {
}
}
}
*/

// TestRateLimitedResolve exercises the rate limit enforced on re-resolution
// requests. It sets the re-resolution rate to a small value and repeatedly
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 713f86e

Please sign in to comment.