From 3930549b38c0fc4cd94a95efccf7cef5f90515fd Mon Sep 17 00:00:00 2001 From: "Kyle J. Burda" Date: Tue, 24 Jan 2023 15:03:56 -0500 Subject: [PATCH] resolver: replace resolver.Target.Endpoint field with Endpoint() method (#5852) Fixes https://github.com/grpc/grpc-go/issues/5796 --- balancer/grpclb/grpclb.go | 4 +- balancer/rls/balancer.go | 2 +- clientconn.go | 19 +--- clientconn_parsed_target_test.go | 90 ++++++++++--------- .../features/load_balancing/client/main.go | 2 +- .../features/name_resolving/client/main.go | 2 +- internal/resolver/dns/dns_resolver.go | 2 +- internal/resolver/dns/dns_resolver_test.go | 31 ++++--- internal/resolver/passthrough/passthrough.go | 4 +- resolver/resolver.go | 22 ++++- .../balancer/clusterresolver/priority_test.go | 5 +- .../clusterresolver/resource_resolver_dns.go | 3 +- .../clusterresolver/resource_resolver_test.go | 17 ++-- xds/internal/resolver/xds_resolver_test.go | 18 ++-- 14 files changed, 114 insertions(+), 107 deletions(-) diff --git a/balancer/grpclb/grpclb.go b/balancer/grpclb/grpclb.go index dd15810d0aeb..6d698229a342 100644 --- a/balancer/grpclb/grpclb.go +++ b/balancer/grpclb/grpclb.go @@ -136,8 +136,8 @@ func (b *lbBuilder) Build(cc balancer.ClientConn, opt balancer.BuildOptions) bal lb := &lbBalancer{ cc: newLBCacheClientConn(cc), - dialTarget: opt.Target.Endpoint, - target: opt.Target.Endpoint, + dialTarget: opt.Target.Endpoint(), + target: opt.Target.Endpoint(), opt: opt, fallbackTimeout: b.fallbackTimeout, doneCh: make(chan struct{}), diff --git a/balancer/rls/balancer.go b/balancer/rls/balancer.go index f0cff9ac4455..b2f97a9509ed 100644 --- a/balancer/rls/balancer.go +++ b/balancer/rls/balancer.go @@ -481,7 +481,7 @@ func (b *rlsBalancer) sendNewPickerLocked() { } picker := &rlsPicker{ kbm: b.lbCfg.kbMap, - origEndpoint: b.bopts.Target.Endpoint, + origEndpoint: b.bopts.Target.Endpoint(), lb: b, defaultPolicy: b.defaultPolicy, ctrlCh: b.ctrlCh, diff --git a/clientconn.go b/clientconn.go index 6ead8a6f1e9f..d607d4e9e243 100644 --- a/clientconn.go +++ b/clientconn.go @@ -256,7 +256,7 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * if err != nil { return nil, err } - cc.authority, err = determineAuthority(cc.parsedTarget.Endpoint, cc.target, cc.dopts) + cc.authority, err = determineAuthority(cc.parsedTarget.Endpoint(), cc.target, cc.dopts) if err != nil { return nil, err } @@ -1587,30 +1587,17 @@ func (cc *ClientConn) parseTargetAndFindResolver() (resolver.Builder, error) { } // parseTarget uses RFC 3986 semantics to parse the given target into a -// resolver.Target struct containing scheme, authority and endpoint. Query +// resolver.Target struct containing scheme, authority and 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 } - // For targets of the form "[scheme]://[authority]/endpoint, the endpoint - // value returned from url.Parse() contains a leading "/". Although this is - // in accordance with RFC 3986, we do not want to break existing resolver - // implementations which expect the endpoint without the leading "/". So, we - // end up stripping the leading "/" here. But this will result in an - // incorrect parsing for something like "unix:///path/to/socket". Since we - // own the "unix" resolver, we can workaround in the unix resolver by using - // the `URL` field instead of the `Endpoint` field. - endpoint := u.Path - if endpoint == "" { - endpoint = u.Opaque - } - endpoint = strings.TrimPrefix(endpoint, "/") + return resolver.Target{ Scheme: u.Scheme, Authority: u.Host, - Endpoint: endpoint, URL: *u, }, nil } diff --git a/clientconn_parsed_target_test.go b/clientconn_parsed_target_test.go index 8f832a2c7cb4..e957bca78c1e 100644 --- a/clientconn_parsed_target_test.go +++ b/clientconn_parsed_target_test.go @@ -21,6 +21,7 @@ package grpc import ( "context" "errors" + "fmt" "net" "net/url" "testing" @@ -28,6 +29,7 @@ import ( "github.com/google/go-cmp/cmp" "google.golang.org/grpc/credentials/insecure" + "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/resolver" ) @@ -40,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: "", Endpoint: "://"}}, - {target: ":///", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///"}}, - {target: "://a/", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://a/"}}, - {target: ":///a", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///a"}}, - {target: "://a/b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://a/b"}}, - {target: "/", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/"}}, - {target: "a/b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a/b"}}, - {target: "a//b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a//b"}}, - {target: "google.com", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com"}}, - {target: "google.com/?a=b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com/"}}, - {target: "/unix/socket/address", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/unix/socket/address"}}, + {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"))}}, // An unregistered scheme is specified. - {target: "a:///", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:///"}}, - {target: "a://b/", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b/"}}, - {target: "a:///b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:///b"}}, - {target: "a://b/c", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b/c"}}, - {target: "a:b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:b"}}, - {target: "a:/b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:/b"}}, - {target: "a://b", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b"}}, + {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"))}}, // 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/"}}, - {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///://::!@#$%25^&*()b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: " a///://::!@"}}, - {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: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "unix:///a/b/c"}}, + {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")}}, // Cases for `scheme:absolute-path`. - {target: "dns:/a/b/c", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "a/b/c"}}, - {target: "unregistered:/a/b/c", badScheme: true, wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unregistered:/a/b/c"}}, + {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")}}, } for _, test := range tests { @@ -138,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: "", Endpoint: "a/b/c"}, + wantParsed: resolver.Target{Scheme: "unix", Authority: "", URL: *testutils.MustParseURL("unix:a/b/c")}, wantDialerAddress: "unix:a/b/c", }, { target: "unix:/a/b/c", - wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c"}, + wantParsed: resolver.Target{Scheme: "unix", Authority: "", URL: *testutils.MustParseURL("unix:/a/b/c")}, wantDialerAddress: "unix:///a/b/c", }, { target: "unix:///a/b/c", - wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c"}, + wantParsed: resolver.Target{Scheme: "unix", Authority: "", 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: "", Endpoint: "127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: "dns", Authority: "", 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: "", Endpoint: ":///127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: defScheme, Authority: "", 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", Endpoint: "127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: "dns", Authority: "authority", 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: "", Endpoint: "://authority/127.0.0.1:50051"}, + wantParsed: resolver.Target{Scheme: defScheme, Authority: "", 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: "", Endpoint: "/unix/socket/address"}, + wantParsed: resolver.Target{Scheme: defScheme, Authority: "", URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "/unix/socket/address"))}, wantDialerAddress: "/unix/socket/address", }, { target: "", badScheme: true, - wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ""}, + wantParsed: resolver.Target{Scheme: defScheme, Authority: "", 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", Endpoint: "google.com"}, + wantParsed: resolver.Target{Scheme: "passthrough", Authority: "a.server.com", URL: *testutils.MustParseURL("passthrough://a.server.com/google.com")}, wantDialerAddress: "google.com", }, } diff --git a/examples/features/load_balancing/client/main.go b/examples/features/load_balancing/client/main.go index 2caecb7b3d32..6e3d1fc86fe3 100644 --- a/examples/features/load_balancing/client/main.go +++ b/examples/features/load_balancing/client/main.go @@ -111,7 +111,7 @@ type exampleResolver struct { } func (r *exampleResolver) start() { - addrStrs := r.addrsStore[r.target.Endpoint] + addrStrs := r.addrsStore[r.target.Endpoint()] addrs := make([]resolver.Address, len(addrStrs)) for i, s := range addrStrs { addrs[i] = resolver.Address{Addr: s} diff --git a/examples/features/name_resolving/client/main.go b/examples/features/name_resolving/client/main.go index ad6b310b6de7..2766611ba795 100644 --- a/examples/features/name_resolving/client/main.go +++ b/examples/features/name_resolving/client/main.go @@ -119,7 +119,7 @@ type exampleResolver struct { } func (r *exampleResolver) start() { - addrStrs := r.addrsStore[r.target.Endpoint] + addrStrs := r.addrsStore[r.target.Endpoint()] addrs := make([]resolver.Address, len(addrStrs)) for i, s := range addrStrs { addrs[i] = resolver.Address{Addr: s} diff --git a/internal/resolver/dns/dns_resolver.go b/internal/resolver/dns/dns_resolver.go index b08ac30adfef..09a667f33cb0 100644 --- a/internal/resolver/dns/dns_resolver.go +++ b/internal/resolver/dns/dns_resolver.go @@ -116,7 +116,7 @@ type dnsBuilder struct{} // Build creates and starts a DNS resolver that watches the name resolution of the target. func (b *dnsBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { - host, port, err := parseTarget(target.Endpoint, defaultPort) + host, port, err := parseTarget(target.Endpoint(), defaultPort) 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 6bfcf299b33c..d67ee7d080bf 100644 --- a/internal/resolver/dns/dns_resolver_test.go +++ b/internal/resolver/dns/dns_resolver_test.go @@ -23,7 +23,6 @@ import ( "errors" "fmt" "net" - "net/url" "os" "reflect" "strings" @@ -735,7 +734,7 @@ func testDNSResolver(t *testing.T) { for _, a := range tests { b := NewBuilder() cc := &testClientConn{target: a.target} - r, err := b.Build(resolver.Target{Endpoint: a.target}, cc, resolver.BuildOptions{}) + r, err := b.Build(resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", a.target))}, cc, resolver.BuildOptions{}) if err != nil { t.Fatalf("%v\n", err) } @@ -807,7 +806,7 @@ func TestDNSResolverExponentialBackoff(t *testing.T) { cc := &testClientConn{target: test.target} // Cause ClientConn to return an error. cc.updateStateErr = balancer.ErrBadResolverState - r, err := b.Build(resolver.Target{Endpoint: test.target}, cc, resolver.BuildOptions{}) + r, err := b.Build(resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", test.target))}, cc, resolver.BuildOptions{}) if err != nil { t.Fatalf("Error building resolver for target %v: %v", test.target, err) } @@ -962,7 +961,7 @@ func testDNSResolverWithSRV(t *testing.T) { for _, a := range tests { b := NewBuilder() cc := &testClientConn{target: a.target} - r, err := b.Build(resolver.Target{Endpoint: a.target}, cc, resolver.BuildOptions{}) + r, err := b.Build(resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", a.target))}, cc, resolver.BuildOptions{}) if err != nil { t.Fatalf("%v\n", err) } @@ -1046,7 +1045,7 @@ func testDNSResolveNow(t *testing.T) { for _, a := range tests { b := NewBuilder() cc := &testClientConn{target: a.target} - r, err := b.Build(resolver.Target{Endpoint: a.target}, cc, resolver.BuildOptions{}) + r, err := b.Build(resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", a.target))}, cc, resolver.BuildOptions{}) if err != nil { t.Fatalf("%v\n", err) } @@ -1124,7 +1123,7 @@ func testIPResolver(t *testing.T) { for _, v := range tests { b := NewBuilder() cc := &testClientConn{target: v.target} - r, err := b.Build(resolver.Target{Endpoint: v.target}, cc, resolver.BuildOptions{}) + r, err := b.Build(resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", v.target))}, cc, resolver.BuildOptions{}) if err != nil { t.Fatalf("%v\n", err) } @@ -1175,7 +1174,7 @@ func TestResolveFunc(t *testing.T) { {"[2001:db8:a0b:12f0::1]:21", nil}, {":80", nil}, {"127.0.0...1:12345", nil}, - {"[fe80::1%lo0]:80", nil}, + {"[fe80::1%25lo0]:80", nil}, {"golang.org:http", nil}, {"[2001:db8::1]:http", nil}, {"[2001:db8::1]:", errEndsWithColon}, @@ -1187,7 +1186,7 @@ func TestResolveFunc(t *testing.T) { b := NewBuilder() for _, v := range tests { cc := &testClientConn{target: v.addr, errChan: make(chan error, 1)} - r, err := b.Build(resolver.Target{Endpoint: v.addr}, cc, resolver.BuildOptions{}) + r, err := b.Build(resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", v.addr))}, cc, resolver.BuildOptions{}) if err == nil { r.Close() } @@ -1226,7 +1225,7 @@ func TestDisableServiceConfig(t *testing.T) { for _, a := range tests { b := NewBuilder() cc := &testClientConn{target: a.target} - r, err := b.Build(resolver.Target{Endpoint: a.target}, cc, resolver.BuildOptions{DisableServiceConfig: a.disableServiceConfig}) + r, err := b.Build(resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", a.target))}, cc, resolver.BuildOptions{DisableServiceConfig: a.disableServiceConfig}) if err != nil { t.Fatalf("%v\n", err) } @@ -1264,7 +1263,7 @@ func TestTXTError(t *testing.T) { envconfig.TXTErrIgnore = ignore b := NewBuilder() cc := &testClientConn{target: "ipv4.single.fake"} // has A records but not TXT records. - r, err := b.Build(resolver.Target{Endpoint: "ipv4.single.fake"}, cc, resolver.BuildOptions{}) + r, err := b.Build(resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", "ipv4.single.fake"))}, cc, resolver.BuildOptions{}) if err != nil { t.Fatalf("%v\n", err) } @@ -1300,7 +1299,7 @@ func TestDNSResolverRetry(t *testing.T) { b := NewBuilder() target := "ipv4.single.fake" cc := &testClientConn{target: target} - r, err := b.Build(resolver.Target{Endpoint: target}, cc, resolver.BuildOptions{}) + r, err := b.Build(resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", target))}, cc, resolver.BuildOptions{}) if err != nil { t.Fatalf("%v\n", err) } @@ -1438,12 +1437,12 @@ func TestCustomAuthority(t *testing.T) { } } + mockEndpointTarget := "foo.bar.com" b := NewBuilder() - cc := &testClientConn{target: "foo.bar.com", errChan: make(chan error, 1)} + cc := &testClientConn{target: mockEndpointTarget, errChan: make(chan error, 1)} target := resolver.Target{ - Endpoint: "foo.bar.com", Authority: a.authority, - URL: url.URL{Host: a.authority}, + URL: *testutils.MustParseURL(fmt.Sprintf("scheme://%s/%s", a.authority, mockEndpointTarget)), } r, err := b.Build(target, cc, resolver.BuildOptions{}) @@ -1501,7 +1500,7 @@ func TestRateLimitedResolve(t *testing.T) { b := NewBuilder() cc := &testClientConn{target: target} - r, err := b.Build(resolver.Target{Endpoint: target}, cc, resolver.BuildOptions{}) + r, err := b.Build(resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", target))}, cc, resolver.BuildOptions{}) if err != nil { t.Fatalf("resolver.Build() returned error: %v\n", err) } @@ -1610,7 +1609,7 @@ func TestReportError(t *testing.T) { cc := &testClientConn{target: target, errChan: make(chan error)} totalTimesCalledError := 0 b := NewBuilder() - r, err := b.Build(resolver.Target{Endpoint: target}, cc, resolver.BuildOptions{}) + r, err := b.Build(resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", target))}, cc, resolver.BuildOptions{}) if err != nil { t.Fatalf("Error building resolver for target %v: %v", target, err) } diff --git a/internal/resolver/passthrough/passthrough.go b/internal/resolver/passthrough/passthrough.go index c6e08221ff64..afac56572ad5 100644 --- a/internal/resolver/passthrough/passthrough.go +++ b/internal/resolver/passthrough/passthrough.go @@ -31,7 +31,7 @@ const scheme = "passthrough" type passthroughBuilder struct{} func (*passthroughBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { - if target.Endpoint == "" && opts.Dialer == nil { + if target.Endpoint() == "" && opts.Dialer == nil { return nil, errors.New("passthrough: received empty target in Build()") } r := &passthroughResolver{ @@ -52,7 +52,7 @@ type passthroughResolver struct { } func (r *passthroughResolver) start() { - r.cc.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: r.target.Endpoint}}}) + r.cc.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: r.target.Endpoint()}}}) } func (*passthroughResolver) ResolveNow(o resolver.ResolveNowOptions) {} diff --git a/resolver/resolver.go b/resolver/resolver.go index 967cbc7373ab..654e9ce69f4a 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -24,6 +24,7 @@ import ( "context" "net" "net/url" + "strings" "google.golang.org/grpc/attributes" "google.golang.org/grpc/credentials" @@ -247,9 +248,6 @@ type Target struct { Scheme string // Deprecated: use URL.Host instead. Authority string - // Deprecated: use URL.Path or URL.Opaque instead. The latter is set when - // the former is empty. - Endpoint 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 @@ -257,6 +255,24 @@ type Target struct { URL url.URL } +// Endpoint retrieves endpoint without leading "/" from either `URL.Path` +// or `URL.Opaque`. The latter is used when the former is empty. +func (t Target) Endpoint() string { + endpoint := t.URL.Path + if endpoint == "" { + endpoint = t.URL.Opaque + } + // For targets of the form "[scheme]://[authority]/endpoint, the endpoint + // value returned from url.Parse() contains a leading "/". Although this is + // in accordance with RFC 3986, we do not want to break existing resolver + // implementations which expect the endpoint without the leading "/". So, we + // end up stripping the leading "/" here. But this will result in an + // incorrect parsing for something like "unix:///path/to/socket". Since we + // own the "unix" resolver, we can workaround in the unix resolver by using + // the `URL` field. + return strings.TrimPrefix(endpoint, "/") +} + // Builder creates a resolver that will be used to watch name resolution updates. type Builder interface { // Build creates a new resolver for the given target. diff --git a/xds/internal/balancer/clusterresolver/priority_test.go b/xds/internal/balancer/clusterresolver/priority_test.go index b2cc0c9f2097..984215a86e6a 100644 --- a/xds/internal/balancer/clusterresolver/priority_test.go +++ b/xds/internal/balancer/clusterresolver/priority_test.go @@ -62,7 +62,8 @@ func setupTestEDS(t *testing.T, initChild *internalserviceconfig.BalancerConfig) xdsC := fakeclient.NewClientWithName(testBalancerNameFooBar) cc := testutils.NewTestClientConn(t) builder := balancer.Get(Name) - edsb := builder.Build(cc, balancer.BuildOptions{Target: resolver.Target{Endpoint: testEDSServcie}}) + // TODO: @kylejb will fix typo for 'testEDSServcie' in another PR + edsb := builder.Build(cc, balancer.BuildOptions{Target: resolver.Target{URL: *testutils.MustParseURL("dns:///" + testEDSServcie)}}) if edsb == nil { t.Fatalf("builder.Build(%s) failed and returned nil", Name) } @@ -853,7 +854,7 @@ func (s) TestFallbackToDNS(t *testing.T) { defer ctxCancel() select { case target := <-dnsTargetCh: - if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", Endpoint: testDNSTarget}); diff != "" { + if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", 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 7a639f51a5d9..703b00811dfa 100644 --- a/xds/internal/balancer/clusterresolver/resource_resolver_dns.go +++ b/xds/internal/balancer/clusterresolver/resource_resolver_dns.go @@ -21,6 +21,7 @@ package clusterresolver import ( "fmt" + "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/resolver" "google.golang.org/grpc/serviceconfig" ) @@ -50,7 +51,7 @@ func newDNSResolver(target string, topLevelResolver *resourceResolver) *dnsDisco target: target, topLevelResolver: topLevelResolver, } - r, err := newDNS(resolver.Target{Scheme: "dns", Endpoint: target}, ret, resolver.BuildOptions{}) + r, err := newDNS(resolver.Target{Scheme: "dns", URL: *testutils.MustParseURL("dns:///" + target)}, ret, resolver.BuildOptions{}) if err != nil { select { case <-topLevelResolver.updateChannel: diff --git a/xds/internal/balancer/clusterresolver/resource_resolver_test.go b/xds/internal/balancer/clusterresolver/resource_resolver_test.go index 8c90ed0e1cd4..f20482e30288 100644 --- a/xds/internal/balancer/clusterresolver/resource_resolver_test.go +++ b/xds/internal/balancer/clusterresolver/resource_resolver_test.go @@ -24,9 +24,10 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/resolver" "google.golang.org/grpc/resolver/manual" - "google.golang.org/grpc/xds/internal/testutils" + xdstestutils "google.golang.org/grpc/xds/internal/testutils" "google.golang.org/grpc/xds/internal/testutils/fakeclient" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource" ) @@ -40,10 +41,10 @@ var ( ) func init() { - clab1 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil) + clab1 := xdstestutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil) clab1.AddLocality(testSubZones[0], 1, 0, testEndpointAddrs[:1], nil) testEDSUpdates = append(testEDSUpdates, parseEDSRespProtoForTesting(clab1.Build())) - clab2 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil) + clab2 := xdstestutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil) clab2.AddLocality(testSubZones[1], 1, 0, testEndpointAddrs[1:2], nil) testEDSUpdates = append(testEDSUpdates, parseEDSRespProtoForTesting(clab2.Build())) } @@ -156,7 +157,7 @@ func (s) TestResourceResolverOneDNSResource(t *testing.T) { { name: "watch DNS", target: testDNSTarget, - wantTarget: resolver.Target{Scheme: "dns", Endpoint: testDNSTarget}, + wantTarget: resolver.Target{Scheme: "dns", URL: *testutils.MustParseURL("dns:///" + testDNSTarget)}, addrs: []resolver.Address{{Addr: "1.1.1.1"}, {Addr: "2.2.2.2"}}, want: []priorityConfig{{ mechanism: DiscoveryMechanism{ @@ -614,7 +615,7 @@ func (s) TestResourceResolverEDSAndDNS(t *testing.T) { } select { case target := <-dnsTargetCh: - if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", Endpoint: testDNSTarget}); diff != "" { + if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", URL: *testutils.MustParseURL("dns:///" + testDNSTarget)}); diff != "" { t.Fatalf("got unexpected DNS target to watch, diff (-got, +want): %v", diff) } case <-ctx.Done(): @@ -719,7 +720,7 @@ func (s) TestResourceResolverChangeFromEDSToDNS(t *testing.T) { }}) select { case target := <-dnsTargetCh: - if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", Endpoint: testDNSTarget}); diff != "" { + if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", URL: *testutils.MustParseURL("dns:///" + testDNSTarget)}); diff != "" { t.Fatalf("got unexpected DNS target to watch, diff (-got, +want): %v", diff) } case <-ctx.Done(): @@ -786,7 +787,7 @@ func (s) TestResourceResolverError(t *testing.T) { } select { case target := <-dnsTargetCh: - if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", Endpoint: testDNSTarget}); diff != "" { + if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", URL: *testutils.MustParseURL("dns:///" + testDNSTarget)}); diff != "" { t.Fatalf("got unexpected DNS target to watch, diff (-got, +want): %v", diff) } case <-ctx.Done(): @@ -847,7 +848,7 @@ func (s) TestResourceResolverDNSResolveNow(t *testing.T) { defer ctxCancel() select { case target := <-dnsTargetCh: - if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", Endpoint: testDNSTarget}); diff != "" { + if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", 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/resolver/xds_resolver_test.go b/xds/internal/resolver/xds_resolver_test.go index af7729390660..deb7018ca617 100644 --- a/xds/internal/resolver/xds_resolver_test.go +++ b/xds/internal/resolver/xds_resolver_test.go @@ -75,7 +75,7 @@ const ( defaultTestShortTimeout = 100 * time.Microsecond ) -var target = resolver.Target{Endpoint: targetStr, URL: url.URL{Scheme: "xds", Path: "/" + targetStr}} +var target = resolver.Target{URL: *testutils.MustParseURL("xds:///" + targetStr)} var routerFilter = xdsresource.HTTPFilter{Name: "rtr", Filter: httpfilter.Get(router.TypeURL)} var routerFilterList = []xdsresource.HTTPFilter{routerFilter} @@ -907,14 +907,14 @@ func (s) TestResolverRemovedWithRPCs(t *testing.T) { { "cds_experimental": { "cluster": "test-cluster-1" - } - } - ] - } - } - } - } - ] + } + } + ] + } + } + } + } + ] }`) if !internal.EqualServiceConfigForTesting(rState.ServiceConfig.Config, wantSCParsed.Config) { t.Errorf("Received unexpected service config")