From 32bf00fb627f7b9f781a029ae5b9def11be0c0c2 Mon Sep 17 00:00:00 2001 From: Gina Yeh Date: Tue, 6 Jun 2023 10:40:05 -0700 Subject: [PATCH] Delete resolver.Target.Scheme and resolver.Target.Authority --- clientconn.go | 10 +-- clientconn_parsed_target_test.go | 75 +++++-------------- internal/resolver/dns/dns_resolver.go | 12 +-- internal/resolver/dns/dns_resolver_test.go | 9 +-- resolver/resolver.go | 4 - xds/googledirectpath/googlec2p.go | 2 - .../balancer/clusterresolver/priority_test.go | 2 +- .../clusterresolver/resource_resolver_dns.go | 2 +- 8 files changed, 35 insertions(+), 81 deletions(-) diff --git a/clientconn.go b/clientconn.go index a27c0573dc1d..49e20cd9d2b4 100644 --- a/clientconn.go +++ b/clientconn.go @@ -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: diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index e957bca78c1e..c40f98cccab2 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -23,7 +23,6 @@ import ( "errors" "fmt" "net" - "net/url" "testing" "time" @@ -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 { @@ -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 { @@ -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", }, } @@ -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) { diff --git a/internal/resolver/dns/dns_resolver.go b/internal/resolver/dns/dns_resolver.go index 09a667f33cb0..5fe4a1d83c10 100644 --- a/internal/resolver/dns/dns_resolver.go +++ b/internal/resolver/dns/dns_resolver.go @@ -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 @@ -103,7 +103,7 @@ var customAuthorityResolver = func(authority string) (netResolver, error) { return &net.Resolver{ PreferGo: true, - Dial: customAuthorityDialler(authorityWithPort), + Dial: addressDialer(authorityWithPort), }, nil } @@ -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 } diff --git a/internal/resolver/dns/dns_resolver_test.go b/internal/resolver/dns/dns_resolver_test.go index d67ee7d080bf..a66ffffd3ce1 100644 --- a/internal/resolver/dns/dns_resolver_test.go +++ b/internal/resolver/dns/dns_resolver_test.go @@ -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 { @@ -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{}) diff --git a/resolver/resolver.go b/resolver/resolver.go index 353c10b69a5b..1b7f382e3a03 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -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 diff --git a/xds/googledirectpath/googlec2p.go b/xds/googledirectpath/googlec2p.go index 20891c7a4cb8..f8f749835c24 100644 --- a/xds/googledirectpath/googlec2p.go +++ b/xds/googledirectpath/googlec2p.go @@ -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) } @@ -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{ diff --git a/xds/internal/balancer/clusterresolver/priority_test.go b/xds/internal/balancer/clusterresolver/priority_test.go index 68325a31c17e..cbd53537b4d3 100644 --- a/xds/internal/balancer/clusterresolver/priority_test.go +++ b/xds/internal/balancer/clusterresolver/priority_test.go @@ -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(): diff --git a/xds/internal/balancer/clusterresolver/resource_resolver_dns.go b/xds/internal/balancer/clusterresolver/resource_resolver_dns.go index 06af9cc6df32..4146959e22aa 100644 --- a/xds/internal/balancer/clusterresolver/resource_resolver_dns.go +++ b/xds/internal/balancer/clusterresolver/resource_resolver_dns.go @@ -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