From f54b30deb24fc2aba6952eb26d1e63216560fcd7 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Wed, 6 Nov 2019 11:24:48 -0800 Subject: [PATCH 1/7] dns: stop polling for updates; use UpdateState API --- internal/resolver/dns/dns_resolver.go | 123 +++--------- internal/resolver/dns/dns_resolver_test.go | 221 +++++++++------------ 2 files changed, 128 insertions(+), 216 deletions(-) diff --git a/internal/resolver/dns/dns_resolver.go b/internal/resolver/dns/dns_resolver.go index 65f231c12cbc..be9321ee8ba9 100644 --- a/internal/resolver/dns/dns_resolver.go +++ b/internal/resolver/dns/dns_resolver.go @@ -32,9 +32,7 @@ import ( "sync" "time" - "google.golang.org/grpc/backoff" "google.golang.org/grpc/grpclog" - internalbackoff "google.golang.org/grpc/internal/backoff" "google.golang.org/grpc/internal/grpcrand" "google.golang.org/grpc/resolver" ) @@ -49,7 +47,6 @@ func init() { const ( defaultPort = "443" - defaultFreq = time.Minute * 30 defaultDNSSvrPort = "53" golang = "GO" // txtPrefix is the prefix string to be prepended to the host name for txt record lookup. @@ -99,13 +96,10 @@ var customAuthorityResolver = func(authority string) (netResolver, error) { // NewBuilder creates a dnsBuilder which is used to factory DNS resolvers. func NewBuilder() resolver.Builder { - return &dnsBuilder{minFreq: defaultFreq} + return &dnsBuilder{} } -type dnsBuilder struct { - // minimum frequency of polling the DNS server. - minFreq time.Duration -} +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) { @@ -115,33 +109,20 @@ func (b *dnsBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts } // IP address. - if net.ParseIP(host) != nil { - host, _ = formatIP(host) - addr := []resolver.Address{{Addr: host + ":" + port}} - i := &ipResolver{ - cc: cc, - ip: addr, - rn: make(chan struct{}, 1), - q: make(chan struct{}), - } - cc.NewAddress(addr) - go i.watcher() - return i, nil + if ipAddr, ok := formatIP(host); ok { + addr := []resolver.Address{{Addr: ipAddr + ":" + port}} + cc.UpdateState(resolver.State{Addresses: addr}) + return deadResolver{}, nil } // DNS address (non-IP). ctx, cancel := context.WithCancel(context.Background()) - bc := backoff.DefaultConfig - bc.MaxDelay = b.minFreq d := &dnsResolver{ - freq: b.minFreq, - backoff: internalbackoff.Exponential{Config: bc}, host: host, port: port, ctx: ctx, cancel: cancel, cc: cc, - t: time.NewTimer(0), rn: make(chan struct{}, 1), disableServiceConfig: opts.DisableServiceConfig, } @@ -157,6 +138,7 @@ func (b *dnsBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts d.wg.Add(1) go d.watcher() + d.ResolveNow(resolver.ResolveNowOption{}) return d, nil } @@ -171,53 +153,23 @@ type netResolver interface { LookupTXT(ctx context.Context, name string) (txts []string, err error) } -// ipResolver watches for the name resolution update for an IP address. -type ipResolver struct { - cc resolver.ClientConn - ip []resolver.Address - // rn channel is used by ResolveNow() to force an immediate resolution of the target. - rn chan struct{} - q chan struct{} -} +// deadResolver is a resolver that does nothing. +type deadResolver struct{} -// ResolveNow resend the address it stores, no resolution is needed. -func (i *ipResolver) ResolveNow(opt resolver.ResolveNowOptions) { - select { - case i.rn <- struct{}{}: - default: - } -} +func (deadResolver) ResolveNow(_ resolver.ResolveNowOption) {} -// Close closes the ipResolver. -func (i *ipResolver) Close() { - close(i.q) -} - -func (i *ipResolver) watcher() { - for { - select { - case <-i.rn: - i.cc.NewAddress(i.ip) - case <-i.q: - return - } - } -} +func (deadResolver) Close() {} // dnsResolver watches for the name resolution update for a non-IP target. type dnsResolver struct { - freq time.Duration - backoff internalbackoff.Exponential - retryCount int - host string - port string - resolver netResolver - ctx context.Context - cancel context.CancelFunc - cc resolver.ClientConn + host string + port string + resolver netResolver + ctx context.Context + cancel context.CancelFunc + cc resolver.ClientConn // rn channel is used by ResolveNow() to force an immediate resolution of the target. rn chan struct{} - t *time.Timer // wg is used to enforce Close() to return after the watcher() goroutine has finished. // Otherwise, data race will be possible. [Race Example] in dns_resolver_test we // replace the real lookup functions with mocked ones to facilitate testing. @@ -229,7 +181,7 @@ type dnsResolver struct { } // ResolveNow invoke an immediate resolution of the target that this dnsResolver watches. -func (d *dnsResolver) ResolveNow(opt resolver.ResolveNowOptions) { +func (d *dnsResolver) ResolveNow(resolver.ResolveNowOptions) { select { case d.rn <- struct{}{}: default: @@ -240,7 +192,6 @@ func (d *dnsResolver) ResolveNow(opt resolver.ResolveNowOptions) { func (d *dnsResolver) Close() { d.cancel() d.wg.Wait() - d.t.Stop() } func (d *dnsResolver) watcher() { @@ -249,29 +200,11 @@ func (d *dnsResolver) watcher() { select { case <-d.ctx.Done(): return - case <-d.t.C: case <-d.rn: - if !d.t.Stop() { - // Before resetting a timer, it should be stopped to prevent racing with - // reads on it's channel. - <-d.t.C - } } - result, sc := d.lookup() - // Next lookup should happen within an interval defined by d.freq. It may be - // more often due to exponential retry on empty address list. - if len(result) == 0 { - d.retryCount++ - d.t.Reset(d.backoff.Backoff(d.retryCount)) - } else { - d.retryCount = 0 - d.t.Reset(d.freq) - } - if sc != "" { // We get empty string when disabled or the TXT lookup failed. - d.cc.NewServiceConfig(sc) - } - d.cc.NewAddress(result) + state := d.lookup() + d.cc.UpdateState(*state) // Sleep to prevent excessive re-resolutions. Incoming resolution requests // will be queued in d.rn. @@ -352,15 +285,17 @@ func (d *dnsResolver) lookupHost() []resolver.Address { return newAddrs } -func (d *dnsResolver) lookup() ([]resolver.Address, string) { - newAddrs := d.lookupSRV() +func (d *dnsResolver) lookup() *resolver.State { + srv := d.lookupSRV() + state := &resolver.State{ + Addresses: append(d.lookupHost(), srv...), + } // Support fallback to non-balancer address. - newAddrs = append(newAddrs, d.lookupHost()...) - if d.disableServiceConfig { - return newAddrs, "" + if !d.disableServiceConfig { + sc := canaryingSC(d.lookupTXT()) + state.ServiceConfig = d.cc.ParseServiceConfig(sc) } - sc := d.lookupTXT() - return newAddrs, canaryingSC(sc) + return state } // formatIP returns ok = false if addr is not a valid textual representation of an IP address. diff --git a/internal/resolver/dns/dns_resolver_test.go b/internal/resolver/dns/dns_resolver_test.go index f8083b2026e5..fea0904d3134 100644 --- a/internal/resolver/dns/dns_resolver_test.go +++ b/internal/resolver/dns/dns_resolver_test.go @@ -35,14 +35,11 @@ import ( ) func TestMain(m *testing.M) { - // Set a valid duration for the re-resolution rate only for tests which are - // actually testing that feature. - dc := replaceDNSResRate(time.Duration(0)) - defer dc() - - cleanup := replaceNetFunc(nil) + // Set a non-zero duration only for tests which are actually testing that + // feature. + replaceDNSResRate(time.Duration(0)) // No nead to clean up since we os.Exit + replaceNetFunc(nil) // No nead to clean up since we os.Exit code := m.Run() - cleanup() os.Exit(code) } @@ -51,47 +48,52 @@ const ( ) type testClientConn struct { - target string - m1 sync.Mutex - addrs []resolver.Address - a int // how many times NewAddress() has been called - m2 sync.Mutex - sc string - s int + target string + m1 sync.Mutex + m2 sync.Mutex + sc string + state resolver.State + updateStateCalls int } func (t *testClientConn) UpdateState(s resolver.State) { - panic("unused") + t.m1.Lock() + defer t.m1.Unlock() + t.state = s + t.updateStateCalls++ } func (t *testClientConn) NewAddress(addresses []resolver.Address) { + panic("unused") +} + +func (t *testClientConn) getState() (resolver.State, int) { t.m1.Lock() defer t.m1.Unlock() - t.addrs = addresses - t.a++ + return t.state, t.updateStateCalls } -func (t *testClientConn) getAddress() ([]resolver.Address, int) { +func (t *testClientConn) getSC() (string, int) { t.m1.Lock() defer t.m1.Unlock() - return t.addrs, t.a + sc := "" + if t.state.ServiceConfig != nil { + sc = t.state.ServiceConfig.Config.(unparsedServiceConfig).config + } + return sc, t.updateStateCalls } func (t *testClientConn) NewServiceConfig(serviceConfig string) { - t.m2.Lock() - defer t.m2.Unlock() - t.sc = serviceConfig - t.s++ + panic("unused") } -func (t *testClientConn) getSc() (string, int) { - t.m2.Lock() - defer t.m2.Unlock() - return t.sc, t.s +type unparsedServiceConfig struct { + serviceconfig.Config + config string } -func (t *testClientConn) ParseServiceConfig(string) *serviceconfig.ParseResult { - panic("not implemented") +func (t *testClientConn) ParseServiceConfig(s string) *serviceconfig.ParseResult { + return &serviceconfig.ParseResult{Config: unparsedServiceConfig{config: s}} } func (t *testClientConn) ReportError(error) { @@ -671,7 +673,7 @@ func testDNSResolver(t *testing.T) { }, { "srv.ipv4.single.fake", - []resolver.Address{{Addr: "2.4.6.8" + colonDefaultPort}}, + []resolver.Address{{Addr: "2.4.6.8" + colonDefaultPort}, {Addr: "1.2.3.4:1234", Type: resolver.GRPCLB, ServerName: "ipv4.single.fake"}}, generateSC("srv.ipv4.single.fake"), }, { @@ -789,36 +791,26 @@ func testDNSResolverWithSRV(t *testing.T) { if err != nil { t.Fatalf("%v\n", err) } - var addrs []resolver.Address + defer r.Close() + var state resolver.State var cnt int - for { - addrs, cnt = cc.getAddress() + for i := 0; i < 2000; i++ { + state, cnt = cc.getState() if cnt > 0 { break } time.Sleep(time.Millisecond) } - var sc string - if a.scWant != "" { - for { - sc, cnt = cc.getSc() - if cnt > 0 { - break - } - time.Sleep(time.Millisecond) - } - } else { - // A new service config should never be produced; call getSc once - // just in case. - sc, _ = cc.getSc() + if cnt == 0 { + t.Fatalf("UpdateState not called after 2s; aborting") } - if !reflect.DeepEqual(a.addrWant, addrs) { - t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, addrs, a.addrWant) + if !reflect.DeepEqual(a.addrWant, state.Addresses) { + t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, state.Addresses, a.addrWant) } + sc, _ := cc.getSC() if !reflect.DeepEqual(a.scWant, sc) { t.Errorf("Resolved service config of target: %q = %+v, want %+v\n", a.target, sc, a.scWant) } - r.Close() } } @@ -867,55 +859,47 @@ func testDNSResolveNow(t *testing.T) { if err != nil { t.Fatalf("%v\n", err) } - var addrs []resolver.Address + defer r.Close() + var state resolver.State var cnt int - for { - addrs, cnt = cc.getAddress() + for i := 0; i < 2000; i++ { + state, cnt = cc.getState() if cnt > 0 { break } time.Sleep(time.Millisecond) } - var sc string - for { - sc, cnt = cc.getSc() - if cnt > 0 { - break - } - time.Sleep(time.Millisecond) + if cnt == 0 { + t.Fatalf("UpdateState not called after 2s; aborting. state=%v", state) } - if !reflect.DeepEqual(a.addrWant, addrs) { - t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, addrs, a.addrWant) + if !reflect.DeepEqual(a.addrWant, state.Addresses) { + t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, state.Addresses, a.addrWant) } + sc, _ := cc.getSC() if !reflect.DeepEqual(a.scWant, sc) { t.Errorf("Resolved service config of target: %q = %+v, want %+v\n", a.target, sc, a.scWant) } + revertTbl := mutateTbl(a.target) r.ResolveNow(resolver.ResolveNowOptions{}) - for i := 0; i < 1000; i++ { - addrs, cnt = cc.getAddress() - // Break if the address list changes or enough redundant updates happen. - if !reflect.DeepEqual(addrs, a.addrWant) || cnt > 10 { + for i := 0; i < 2000; i++ { + state, cnt = cc.getState() + if cnt == 2 { break } time.Sleep(time.Millisecond) } - for i := 0; i < 1000; i++ { - sc, cnt = cc.getSc() - // Break if the service config changes or enough redundant updates happen. - if !reflect.DeepEqual(sc, a.scWant) || cnt > 10 { - break - } - time.Sleep(time.Millisecond) + if cnt != 2 { + t.Fatalf("UpdateState not called after 2s; aborting. state=%v", state) } - if !reflect.DeepEqual(a.addrNext, addrs) { - t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, addrs, a.addrNext) + sc, _ = cc.getSC() + if !reflect.DeepEqual(a.addrNext, state.Addresses) { + t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, state.Addresses, a.addrNext) } if !reflect.DeepEqual(a.scNext, sc) { t.Errorf("Resolved service config of target: %q = %+v, want %+v\n", a.target, sc, a.scNext) } revertTbl() - r.Close() } } @@ -946,29 +930,26 @@ func testIPResolver(t *testing.T) { if err != nil { t.Fatalf("%v\n", err) } - var addrs []resolver.Address + var state resolver.State var cnt int for { - addrs, cnt = cc.getAddress() + state, cnt = cc.getState() if cnt > 0 { break } time.Sleep(time.Millisecond) } - if !reflect.DeepEqual(v.want, addrs) { - t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", v.target, addrs, v.want) + if !reflect.DeepEqual(v.want, state.Addresses) { + t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", v.target, state.Addresses, v.want) } r.ResolveNow(resolver.ResolveNowOptions{}) - for { - addrs, cnt = cc.getAddress() - if cnt == 2 { - break + for i := 0; i < 50; i++ { + state, cnt = cc.getState() + if cnt > 1 { + t.Fatalf("Unexpected second call by resolver to UpdateState. state: %v", state) } time.Sleep(time.Millisecond) } - if !reflect.DeepEqual(v.want, addrs) { - t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", v.target, addrs, v.want) - } r.Close() } } @@ -1034,29 +1015,25 @@ func TestDisableServiceConfig(t *testing.T) { b := NewBuilder() cc := &testClientConn{target: a.target} r, err := b.Build(resolver.Target{Endpoint: a.target}, cc, resolver.BuildOptions{DisableServiceConfig: a.disableServiceConfig}) + defer r.Close() if err != nil { t.Fatalf("%v\n", err) } var cnt int var sc string - // First wait for addresses. We know service configs are reported - // first, so once addresses have been reported, we can then check to - // see whether any configs have been reported.. for i := 0; i < 1000; i++ { - _, cnt = cc.getAddress() + sc, cnt = cc.getSC() if cnt > 0 { break } time.Sleep(time.Millisecond) } - sc, cnt = cc.getSc() - if a.disableServiceConfig && cnt > 0 { - t.Errorf("Resolver reported a service config even though lookups are disabled: sc=%v, cnt=%v", sc, cnt) + if cnt == 0 { + t.Fatalf("UpdateState not called after 2s; aborting. sc=%v", sc) } if !reflect.DeepEqual(a.scWant, sc) { t.Errorf("Resolved service config of target: %q = %+v, want %+v\n", a.target, sc, a.scWant) } - r.Close() } } @@ -1068,49 +1045,49 @@ func TestDNSResolverRetry(t *testing.T) { if err != nil { t.Fatalf("%v\n", err) } - var addrs []resolver.Address - for { - addrs, _ = cc.getAddress() - if len(addrs) == 1 { + defer r.Close() + var state resolver.State + for i := 0; i < 2000; i++ { + state, _ = cc.getState() + if len(state.Addresses) == 1 { break } time.Sleep(time.Millisecond) } + if len(state.Addresses) != 1 { + t.Fatalf("UpdateState not called with 1 address after 2s; aborting. state=%v", state) + } want := []resolver.Address{{Addr: "1.2.3.4" + colonDefaultPort}} - if !reflect.DeepEqual(want, addrs) { - t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", target, addrs, want) + if !reflect.DeepEqual(want, state.Addresses) { + t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", target, state.Addresses, want) } // mutate the host lookup table so the target has 0 address returned. revertTbl := mutateTbl(target) // trigger a resolve that will get empty address list r.ResolveNow(resolver.ResolveNowOptions{}) - for { - addrs, _ = cc.getAddress() - if len(addrs) == 0 { + for i := 0; i < 2000; i++ { + state, _ = cc.getState() + if len(state.Addresses) == 0 { break } time.Sleep(time.Millisecond) } + if len(state.Addresses) != 0 { + t.Fatalf("UpdateState not called with 0 address after 2s; aborting. state=%v", state) + } revertTbl() // wait for the retry to happen in two seconds. - timer := time.NewTimer(2 * time.Second) -loop: - for { - select { - case <-timer.C: - break loop - default: - addrs, _ = cc.getAddress() - if len(addrs) != 0 { - break loop - } - time.Sleep(time.Millisecond) + r.ResolveNow(resolver.ResolveNowOption{}) + for i := 0; i < 2000; i++ { + state, _ = cc.getState() + if len(state.Addresses) == 1 { + break } + time.Sleep(time.Millisecond) } - if !reflect.DeepEqual(want, addrs) { - t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", target, addrs, want) + if !reflect.DeepEqual(want, state.Addresses) { + t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", target, state.Addresses, want) } - r.Close() } func TestCustomAuthority(t *testing.T) { @@ -1297,16 +1274,16 @@ func TestRateLimitedResolve(t *testing.T) { } wantAddrs := []resolver.Address{{Addr: "1.2.3.4" + colonDefaultPort}, {Addr: "5.6.7.8" + colonDefaultPort}} - var gotAddrs []resolver.Address + var state resolver.State for { var cnt int - gotAddrs, cnt = cc.getAddress() + state, cnt = cc.getState() if cnt > 0 { break } time.Sleep(time.Millisecond) } - if !reflect.DeepEqual(gotAddrs, wantAddrs) { - t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", target, gotAddrs, wantAddrs) + if !reflect.DeepEqual(state.Addresses, wantAddrs) { + t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", target, state.Addresses, wantAddrs) } } From ccfc87f88e32ef5807d7103d5540156a85e2f66c Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Mon, 11 Nov 2019 08:31:41 -0800 Subject: [PATCH 2/7] fix vet --- internal/resolver/dns/dns_resolver_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/resolver/dns/dns_resolver_test.go b/internal/resolver/dns/dns_resolver_test.go index fea0904d3134..ac31e40ae303 100644 --- a/internal/resolver/dns/dns_resolver_test.go +++ b/internal/resolver/dns/dns_resolver_test.go @@ -50,8 +50,6 @@ const ( type testClientConn struct { target string m1 sync.Mutex - m2 sync.Mutex - sc string state resolver.State updateStateCalls int } @@ -1019,6 +1017,7 @@ func TestDisableServiceConfig(t *testing.T) { if err != nil { t.Fatalf("%v\n", err) } + defer r.Close() var cnt int var sc string for i := 0; i < 1000; i++ { From 419ea05c7fbbfc1c3cfabeb1045c3c1596546df0 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Thu, 21 Nov 2019 10:59:22 -0800 Subject: [PATCH 3/7] fix rebase --- internal/resolver/dns/dns_resolver.go | 4 +- internal/resolver/dns/dns_resolver_test.go | 65 +++++++++------------- 2 files changed, 28 insertions(+), 41 deletions(-) diff --git a/internal/resolver/dns/dns_resolver.go b/internal/resolver/dns/dns_resolver.go index be9321ee8ba9..ca87b98bf24a 100644 --- a/internal/resolver/dns/dns_resolver.go +++ b/internal/resolver/dns/dns_resolver.go @@ -138,7 +138,7 @@ func (b *dnsBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts d.wg.Add(1) go d.watcher() - d.ResolveNow(resolver.ResolveNowOption{}) + d.ResolveNow(resolver.ResolveNowOptions{}) return d, nil } @@ -156,7 +156,7 @@ type netResolver interface { // deadResolver is a resolver that does nothing. type deadResolver struct{} -func (deadResolver) ResolveNow(_ resolver.ResolveNowOption) {} +func (deadResolver) ResolveNow(resolver.ResolveNowOptions) {} func (deadResolver) Close() {} diff --git a/internal/resolver/dns/dns_resolver_test.go b/internal/resolver/dns/dns_resolver_test.go index ac31e40ae303..8ff6f3f5b0d9 100644 --- a/internal/resolver/dns/dns_resolver_test.go +++ b/internal/resolver/dns/dns_resolver_test.go @@ -71,20 +71,17 @@ func (t *testClientConn) getState() (resolver.State, int) { return t.state, t.updateStateCalls } -func (t *testClientConn) getSC() (string, int) { - t.m1.Lock() - defer t.m1.Unlock() - sc := "" - if t.state.ServiceConfig != nil { - sc = t.state.ServiceConfig.Config.(unparsedServiceConfig).config - } - return sc, t.updateStateCalls -} - func (t *testClientConn) NewServiceConfig(serviceConfig string) { panic("unused") } +func scFromState(s resolver.State) string { + if s.ServiceConfig != nil { + return s.ServiceConfig.Config.(unparsedServiceConfig).config + } + return "" +} + type unparsedServiceConfig struct { serviceconfig.Config config string @@ -671,7 +668,7 @@ func testDNSResolver(t *testing.T) { }, { "srv.ipv4.single.fake", - []resolver.Address{{Addr: "2.4.6.8" + colonDefaultPort}, {Addr: "1.2.3.4:1234", Type: resolver.GRPCLB, ServerName: "ipv4.single.fake"}}, + []resolver.Address{{Addr: "2.4.6.8" + colonDefaultPort}}, generateSC("srv.ipv4.single.fake"), }, { @@ -698,32 +695,22 @@ func testDNSResolver(t *testing.T) { if err != nil { t.Fatalf("%v\n", err) } - var addrs []resolver.Address + var state resolver.State var cnt int - for { - addrs, cnt = cc.getAddress() + for i := 0; i < 2000; i++ { + state, cnt = cc.getState() if cnt > 0 { break } time.Sleep(time.Millisecond) } - var sc string - if a.scWant != "" { - for { - sc, cnt = cc.getSc() - if cnt > 0 { - break - } - time.Sleep(time.Millisecond) - } - } else { - // A new service config should never be produced; call getSc once - // just in case. - sc, _ = cc.getSc() + if cnt == 0 { + t.Fatalf("UpdateState not called after 2s; aborting") } - if !reflect.DeepEqual(a.addrWant, addrs) { - t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, addrs, a.addrWant) + if !reflect.DeepEqual(a.addrWant, state.Addresses) { + t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, state.Addresses, a.addrWant) } + sc := scFromState(state) if !reflect.DeepEqual(a.scWant, sc) { t.Errorf("Resolved service config of target: %q = %+v, want %+v\n", a.target, sc, a.scWant) } @@ -754,7 +741,7 @@ func testDNSResolverWithSRV(t *testing.T) { }, { "srv.ipv4.single.fake", - []resolver.Address{{Addr: "1.2.3.4:1234", Type: resolver.GRPCLB, ServerName: "ipv4.single.fake"}, {Addr: "2.4.6.8" + colonDefaultPort}}, + []resolver.Address{{Addr: "2.4.6.8" + colonDefaultPort}, {Addr: "1.2.3.4:1234", Type: resolver.GRPCLB, ServerName: "ipv4.single.fake"}}, generateSC("srv.ipv4.single.fake"), }, { @@ -805,7 +792,7 @@ func testDNSResolverWithSRV(t *testing.T) { if !reflect.DeepEqual(a.addrWant, state.Addresses) { t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, state.Addresses, a.addrWant) } - sc, _ := cc.getSC() + sc := scFromState(state) if !reflect.DeepEqual(a.scWant, sc) { t.Errorf("Resolved service config of target: %q = %+v, want %+v\n", a.target, sc, a.scWant) } @@ -873,7 +860,7 @@ func testDNSResolveNow(t *testing.T) { if !reflect.DeepEqual(a.addrWant, state.Addresses) { t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, state.Addresses, a.addrWant) } - sc, _ := cc.getSC() + sc := scFromState(state) if !reflect.DeepEqual(a.scWant, sc) { t.Errorf("Resolved service config of target: %q = %+v, want %+v\n", a.target, sc, a.scWant) } @@ -890,7 +877,7 @@ func testDNSResolveNow(t *testing.T) { if cnt != 2 { t.Fatalf("UpdateState not called after 2s; aborting. state=%v", state) } - sc, _ = cc.getSC() + sc = scFromState(state) if !reflect.DeepEqual(a.addrNext, state.Addresses) { t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, state.Addresses, a.addrNext) } @@ -1013,23 +1000,23 @@ func TestDisableServiceConfig(t *testing.T) { b := NewBuilder() cc := &testClientConn{target: a.target} r, err := b.Build(resolver.Target{Endpoint: a.target}, cc, resolver.BuildOptions{DisableServiceConfig: a.disableServiceConfig}) - defer r.Close() if err != nil { t.Fatalf("%v\n", err) } defer r.Close() var cnt int - var sc string - for i := 0; i < 1000; i++ { - sc, cnt = cc.getSC() + var state resolver.State + for i := 0; i < 2000; i++ { + state, cnt = cc.getState() if cnt > 0 { break } time.Sleep(time.Millisecond) } if cnt == 0 { - t.Fatalf("UpdateState not called after 2s; aborting. sc=%v", sc) + t.Fatalf("UpdateState not called after 2s; aborting") } + sc := scFromState(state) if !reflect.DeepEqual(a.scWant, sc) { t.Errorf("Resolved service config of target: %q = %+v, want %+v\n", a.target, sc, a.scWant) } @@ -1076,7 +1063,7 @@ func TestDNSResolverRetry(t *testing.T) { } revertTbl() // wait for the retry to happen in two seconds. - r.ResolveNow(resolver.ResolveNowOption{}) + r.ResolveNow(resolver.ResolveNowOptions{}) for i := 0; i < 2000; i++ { state, _ = cc.getState() if len(state.Addresses) == 1 { From 21945293f9efbf550cf9b6b627cce1d495614f55 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Thu, 21 Nov 2019 11:26:58 -0800 Subject: [PATCH 4/7] set service config error if lookup errors --- internal/resolver/dns/dns_resolver.go | 19 +++++++++++-------- internal/resolver/dns/dns_resolver_test.go | 3 +++ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/internal/resolver/dns/dns_resolver.go b/internal/resolver/dns/dns_resolver.go index ca87b98bf24a..e71b6206bbc4 100644 --- a/internal/resolver/dns/dns_resolver.go +++ b/internal/resolver/dns/dns_resolver.go @@ -35,6 +35,7 @@ import ( "google.golang.org/grpc/grpclog" "google.golang.org/grpc/internal/grpcrand" "google.golang.org/grpc/resolver" + "google.golang.org/grpc/serviceconfig" ) // EnableSRVLookups controls whether the DNS resolver attempts to fetch gRPCLB @@ -247,11 +248,12 @@ func (d *dnsResolver) lookupSRV() []resolver.Address { return newAddrs } -func (d *dnsResolver) lookupTXT() string { +func (d *dnsResolver) lookupTXT() *serviceconfig.ParseResult { ss, err := d.resolver.LookupTXT(d.ctx, txtPrefix+d.host) if err != nil { - grpclog.Infof("grpc: failed dns TXT record lookup due to %v.\n", err) - return "" + err = fmt.Errorf("error from DNS TXT record lookup: %v", err) + grpclog.Infoln("grpc:", err) + return &serviceconfig.ParseResult{Err: err} } var res string for _, s := range ss { @@ -260,10 +262,12 @@ func (d *dnsResolver) lookupTXT() string { // TXT record must have "grpc_config=" attribute in order to be used as service config. if !strings.HasPrefix(res, txtAttribute) { - grpclog.Warningf("grpc: TXT record %v missing %v attribute", res, txtAttribute) - return "" + grpclog.Warningf("grpc: DNS TXT record %v missing %v attribute", res, txtAttribute) + // This is not an error; it is the equivalent of not having a service config. + return nil } - return strings.TrimPrefix(res, txtAttribute) + sc := canaryingSC(strings.TrimPrefix(res, txtAttribute)) + return d.cc.ParseServiceConfig(sc) } func (d *dnsResolver) lookupHost() []resolver.Address { @@ -292,8 +296,7 @@ func (d *dnsResolver) lookup() *resolver.State { } // Support fallback to non-balancer address. if !d.disableServiceConfig { - sc := canaryingSC(d.lookupTXT()) - state.ServiceConfig = d.cc.ParseServiceConfig(sc) + state.ServiceConfig = d.lookupTXT() } return state } diff --git a/internal/resolver/dns/dns_resolver_test.go b/internal/resolver/dns/dns_resolver_test.go index 8ff6f3f5b0d9..8eca5d7ff91b 100644 --- a/internal/resolver/dns/dns_resolver_test.go +++ b/internal/resolver/dns/dns_resolver_test.go @@ -77,6 +77,9 @@ func (t *testClientConn) NewServiceConfig(serviceConfig string) { func scFromState(s resolver.State) string { if s.ServiceConfig != nil { + if s.ServiceConfig.Err != nil { + return "" + } return s.ServiceConfig.Config.(unparsedServiceConfig).config } return "" From 9ac9d20d4c01dd6e1ec83ea6e44c837ede927588 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Fri, 22 Nov 2019 12:56:51 -0800 Subject: [PATCH 5/7] embed ClientConn in testClientConn --- internal/resolver/dns/dns_resolver_test.go | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/internal/resolver/dns/dns_resolver_test.go b/internal/resolver/dns/dns_resolver_test.go index 8eca5d7ff91b..b82e9aed706a 100644 --- a/internal/resolver/dns/dns_resolver_test.go +++ b/internal/resolver/dns/dns_resolver_test.go @@ -48,10 +48,11 @@ const ( ) type testClientConn struct { - target string - m1 sync.Mutex - state resolver.State - updateStateCalls int + resolver.ClientConn // For unimplemented functions + target string + m1 sync.Mutex + state resolver.State + updateStateCalls int } func (t *testClientConn) UpdateState(s resolver.State) { @@ -61,20 +62,12 @@ func (t *testClientConn) UpdateState(s resolver.State) { t.updateStateCalls++ } -func (t *testClientConn) NewAddress(addresses []resolver.Address) { - panic("unused") -} - func (t *testClientConn) getState() (resolver.State, int) { t.m1.Lock() defer t.m1.Unlock() return t.state, t.updateStateCalls } -func (t *testClientConn) NewServiceConfig(serviceConfig string) { - panic("unused") -} - func scFromState(s resolver.State) string { if s.ServiceConfig != nil { if s.ServiceConfig.Err != nil { From 4fa7ed37efcba882a5558f996eb84750954ad6dc Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Fri, 22 Nov 2019 12:59:06 -0800 Subject: [PATCH 6/7] less reflect --- internal/resolver/dns/dns_resolver_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/resolver/dns/dns_resolver_test.go b/internal/resolver/dns/dns_resolver_test.go index b82e9aed706a..be3ae2aa3117 100644 --- a/internal/resolver/dns/dns_resolver_test.go +++ b/internal/resolver/dns/dns_resolver_test.go @@ -707,7 +707,7 @@ func testDNSResolver(t *testing.T) { t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, state.Addresses, a.addrWant) } sc := scFromState(state) - if !reflect.DeepEqual(a.scWant, sc) { + if a.scWant != sc { t.Errorf("Resolved service config of target: %q = %+v, want %+v\n", a.target, sc, a.scWant) } r.Close() @@ -789,7 +789,7 @@ func testDNSResolverWithSRV(t *testing.T) { t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, state.Addresses, a.addrWant) } sc := scFromState(state) - if !reflect.DeepEqual(a.scWant, sc) { + if a.scWant != sc { t.Errorf("Resolved service config of target: %q = %+v, want %+v\n", a.target, sc, a.scWant) } } @@ -857,7 +857,7 @@ func testDNSResolveNow(t *testing.T) { t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, state.Addresses, a.addrWant) } sc := scFromState(state) - if !reflect.DeepEqual(a.scWant, sc) { + if a.scWant != sc { t.Errorf("Resolved service config of target: %q = %+v, want %+v\n", a.target, sc, a.scWant) } @@ -877,7 +877,7 @@ func testDNSResolveNow(t *testing.T) { if !reflect.DeepEqual(a.addrNext, state.Addresses) { t.Errorf("Resolved addresses of target: %q = %+v, want %+v\n", a.target, state.Addresses, a.addrNext) } - if !reflect.DeepEqual(a.scNext, sc) { + if a.scNext != sc { t.Errorf("Resolved service config of target: %q = %+v, want %+v\n", a.target, sc, a.scNext) } revertTbl() @@ -968,7 +968,7 @@ func TestResolveFunc(t *testing.T) { r.Close() } if !reflect.DeepEqual(err, v.want) { - t.Errorf("Build(%q, cc, resolver.BuildOptions{}) = %v, want %v", v.addr, err, v.want) + t.Errorf("Build(%q, cc, _) = %v, want %v", v.addr, err, v.want) } } } @@ -1013,7 +1013,7 @@ func TestDisableServiceConfig(t *testing.T) { t.Fatalf("UpdateState not called after 2s; aborting") } sc := scFromState(state) - if !reflect.DeepEqual(a.scWant, sc) { + if a.scWant != sc { t.Errorf("Resolved service config of target: %q = %+v, want %+v\n", a.target, sc, a.scWant) } } From 99ec4fd2f7e2d7c97d346c380e51c2cc840e8920 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Fri, 22 Nov 2019 13:09:35 -0800 Subject: [PATCH 7/7] fallback comment --- internal/resolver/dns/dns_resolver.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/resolver/dns/dns_resolver.go b/internal/resolver/dns/dns_resolver.go index e71b6206bbc4..bc89eee58c9f 100644 --- a/internal/resolver/dns/dns_resolver.go +++ b/internal/resolver/dns/dns_resolver.go @@ -294,7 +294,6 @@ func (d *dnsResolver) lookup() *resolver.State { state := &resolver.State{ Addresses: append(d.lookupHost(), srv...), } - // Support fallback to non-balancer address. if !d.disableServiceConfig { state.ServiceConfig = d.lookupTXT() }