Skip to content

Commit

Permalink
cleanup: usages of resolver.Target.Endpoint
Browse files Browse the repository at this point in the history
* feat(resolver): create Target.ToEndpoint()

** fix(resolver): strip leading "/" from URL.Opaque

** fix(resolver): document stripped "/" in ToEndpoint

* fix(internal): remove reliance on Target.Endpoint

** fix(dns_resolver_test): add escape for percent char

** feat(testutils): create helper function for dns_resolver_test

** fix(testutils/parse_url): return output of url.Parse

** fix(testutils/parse_url): simplify documentation
  • Loading branch information
kylejb committed Dec 16, 2022
1 parent ae86ff4 commit 978441f
Show file tree
Hide file tree
Showing 10 changed files with 69 additions and 25 deletions.
4 changes: 2 additions & 2 deletions balancer/grpclb/grpclb.go
Expand Up @@ -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.ToEndpoint(),
target: opt.Target.ToEndpoint(),
opt: opt,
fallbackTimeout: b.fallbackTimeout,
doneCh: make(chan struct{}),
Expand Down
2 changes: 1 addition & 1 deletion balancer/rls/balancer.go
Expand Up @@ -448,7 +448,7 @@ func (b *rlsBalancer) sendNewPickerLocked() {
}
picker := &rlsPicker{
kbm: b.lbCfg.kbMap,
origEndpoint: b.bopts.Target.Endpoint,
origEndpoint: b.bopts.Target.ToEndpoint(),
lb: b,
defaultPolicy: b.defaultPolicy,
ctrlCh: b.ctrlCh,
Expand Down
2 changes: 1 addition & 1 deletion clientconn.go
Expand Up @@ -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.ToEndpoint(), cc.target, cc.dopts)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion examples/features/load_balancing/client/main.go
Expand Up @@ -111,7 +111,7 @@ type exampleResolver struct {
}

func (r *exampleResolver) start() {
addrStrs := r.addrsStore[r.target.Endpoint]
addrStrs := r.addrsStore[r.target.ToEndpoint()]
addrs := make([]resolver.Address, len(addrStrs))
for i, s := range addrStrs {
addrs[i] = resolver.Address{Addr: s}
Expand Down
2 changes: 1 addition & 1 deletion examples/features/name_resolving/client/main.go
Expand Up @@ -119,7 +119,7 @@ type exampleResolver struct {
}

func (r *exampleResolver) start() {
addrStrs := r.addrsStore[r.target.Endpoint]
addrStrs := r.addrsStore[r.target.ToEndpoint()]
addrs := make([]resolver.Address, len(addrStrs))
for i, s := range addrStrs {
addrs[i] = resolver.Address{Addr: s}
Expand Down
2 changes: 1 addition & 1 deletion internal/resolver/dns/dns_resolver.go
Expand Up @@ -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.ToEndpoint(), defaultPort)
if err != nil {
return nil, err
}
Expand Down
27 changes: 13 additions & 14 deletions internal/resolver/dns/dns_resolver_test.go
Expand Up @@ -23,7 +23,6 @@ import (
"errors"
"fmt"
"net"
"net/url"
"os"
"reflect"
"strings"
Expand Down Expand Up @@ -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{Endpoint: a.target, URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", a.target))}, cc, resolver.BuildOptions{})
if err != nil {
t.Fatalf("%v\n", err)
}
Expand Down Expand Up @@ -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{Endpoint: test.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)
}
Expand Down Expand Up @@ -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{Endpoint: a.target, URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", a.target))}, cc, resolver.BuildOptions{})
if err != nil {
t.Fatalf("%v\n", err)
}
Expand Down Expand Up @@ -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{Endpoint: a.target, URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", a.target))}, cc, resolver.BuildOptions{})
if err != nil {
t.Fatalf("%v\n", err)
}
Expand Down Expand Up @@ -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{Endpoint: v.target, URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", v.target))}, cc, resolver.BuildOptions{})
if err != nil {
t.Fatalf("%v\n", err)
}
Expand Down Expand Up @@ -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},
Expand All @@ -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{Endpoint: v.addr, URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", v.addr))}, cc, resolver.BuildOptions{})
if err == nil {
r.Close()
}
Expand Down Expand Up @@ -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{Endpoint: a.target, URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", a.target))}, cc, resolver.BuildOptions{DisableServiceConfig: a.disableServiceConfig})
if err != nil {
t.Fatalf("%v\n", err)
}
Expand Down Expand Up @@ -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{Endpoint: "ipv4.single.fake", URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", "ipv4.single.fake"))}, cc, resolver.BuildOptions{})
if err != nil {
t.Fatalf("%v\n", err)
}
Expand Down Expand Up @@ -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{Endpoint: target, URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", target))}, cc, resolver.BuildOptions{})
if err != nil {
t.Fatalf("%v\n", err)
}
Expand Down Expand Up @@ -1443,7 +1442,7 @@ func TestCustomAuthority(t *testing.T) {
target := resolver.Target{
Endpoint: "foo.bar.com",
Authority: a.authority,
URL: url.URL{Host: a.authority},
URL: *testutils.MustParseURL(fmt.Sprintf("scheme://%s/foo.bar.com", a.authority)),
}
r, err := b.Build(target, cc, resolver.BuildOptions{})

Expand Down Expand Up @@ -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{Endpoint: target, URL: *testutils.MustParseURL(fmt.Sprintf("scheme:///%s", target))}, cc, resolver.BuildOptions{})
if err != nil {
t.Fatalf("resolver.Build() returned error: %v\n", err)
}
Expand Down Expand Up @@ -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{Endpoint: 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)
}
Expand Down
4 changes: 2 additions & 2 deletions internal/resolver/passthrough/passthrough.go
Expand Up @@ -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.ToEndpoint() == "" && opts.Dialer == nil {
return nil, errors.New("passthrough: received empty target in Build()")
}
r := &passthroughResolver{
Expand All @@ -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.ToEndpoint()}}})
}

func (*passthroughResolver) ResolveNow(o resolver.ResolveNowOptions) {}
Expand Down
34 changes: 34 additions & 0 deletions internal/testutils/parse_url.go
@@ -0,0 +1,34 @@
/*
*
* Copyright 2022 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

package testutils

import (
"fmt"
"net/url"
)

// MustParseURL attempts to parse the provided target using url.Parse()
// and panics if the url parsing fails.
func MustParseURL(target string) *url.URL {
u, err := url.Parse(target)
if err != nil {
panic(fmt.Sprintf("Error parsing target(%s): %v", target, err))
}
return u
}
15 changes: 13 additions & 2 deletions resolver/resolver.go
Expand Up @@ -24,6 +24,7 @@ import (
"context"
"net"
"net/url"
"strings"

"google.golang.org/grpc/attributes"
"google.golang.org/grpc/credentials"
Expand Down Expand Up @@ -247,8 +248,7 @@ 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.
// Deprecated: use ToEndpoint() instead.
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
Expand All @@ -257,6 +257,17 @@ type Target struct {
URL url.URL
}

// ToEndpoint retrieves endpoint without leading "/" from either `URL.Path`
// or `URL.Opaque`. The latter is set when the former is empty.
func (t Target) ToEndpoint() string {
endpoint := t.URL.Path
if endpoint == "" {
endpoint = t.URL.Opaque
}
// Leading "/" is stripped to support existing resolver implementations.
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.
Expand Down

0 comments on commit 978441f

Please sign in to comment.