From 5d54aa9c5fc3f342b67b3803818059ed73503dd7 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 5 Jun 2022 21:58:34 +0200 Subject: [PATCH] cleanup: move caching resolvers from netx to netxlite (#799) Now that we have properly refactored the caching resolvers we can move them into netxlite as optional resolvers created using the proper abstract factories we just added. This diff reduces the complexity and the code size of netx. See https://github.com/ooni/probe/issues/2121. --- internal/engine/netx/netx.go | 4 +-- internal/netxlite/bogon.go | 34 +++++++++---------- internal/netxlite/bogon_test.go | 18 +++++----- .../resolvercache.go} | 7 ++-- .../resolvercache_test.go} | 7 ++-- 5 files changed, 34 insertions(+), 36 deletions(-) rename internal/{engine/netx/cacheresolver.go => netxlite/resolvercache.go} (95%) rename internal/{engine/netx/cacheresolver_test.go => netxlite/resolvercache_test.go} (97%) diff --git a/internal/engine/netx/netx.go b/internal/engine/netx/netx.go index 7301cc6e94..b46b2890e0 100644 --- a/internal/engine/netx/netx.go +++ b/internal/engine/netx/netx.go @@ -67,8 +67,8 @@ func NewResolver(config Config) model.Resolver { model.ValidLoggerOrDefault(config.Logger), config.BaseResolver, ) - r = MaybeWrapWithCachingResolver(config.CacheResolutions, r) - r = MaybeWrapWithStaticDNSCache(config.DNSCache, r) + r = netxlite.MaybeWrapWithCachingResolver(config.CacheResolutions, r) + r = netxlite.MaybeWrapWithStaticDNSCache(config.DNSCache, r) r = netxlite.MaybeWrapWithBogonResolver(config.BogonIsError, r) return config.Saver.WrapResolver(r) // WAI when config.Saver==nil } diff --git a/internal/netxlite/bogon.go b/internal/netxlite/bogon.go index 2e23e706ba..33ce86bb0e 100644 --- a/internal/netxlite/bogon.go +++ b/internal/netxlite/bogon.go @@ -3,7 +3,8 @@ package netxlite // // Bogon // -// This file helps us to decide if an IPAddr is a bogon. +// This file helps us to decide if an IPAddr is a bogon as well as +// a way to create a resolver that fails when resolving bogons. // import ( @@ -17,29 +18,28 @@ import ( // MaybeWrapWithBogonResolver wraps the given resolver with a BogonResolver // iff the provided boolean flag is true. Otherwise, this factory just returns // the provided resolver to the caller without any wrapping. +// +// The returned resolver returns a wrapped ErrDNSBogon if there's a bogon error. +// +// BUG: This resolver currently only implements LookupHost. All the other +// lookup methods will always return ErrNoDNSTransport. func MaybeWrapWithBogonResolver(enabled bool, reso model.Resolver) model.Resolver { if enabled { - reso = &BogonResolver{Resolver: reso} + reso = &bogonResolver{Resolver: reso} } return reso } -// BogonResolver is a bogon aware resolver. When a bogon is encountered in +// bogonResolver is a bogon aware resolver. When a bogon is encountered in // a reply, this resolver will return ErrDNSBogon. -// -// This resolver is not part of the default chain created by WrapResolver -// therefore it returns errors that have already been wrapped. -// -// BUG: This resolver currently only implements LookupHost. All the other -// lookup methods will always return ErrNoDNSTransport. -type BogonResolver struct { +type bogonResolver struct { Resolver model.Resolver } -var _ model.Resolver = &BogonResolver{} +var _ model.Resolver = &bogonResolver{} // LookupHost implements Resolver.LookupHost -func (r *BogonResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) { +func (r *bogonResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) { addrs, err := r.Resolver.LookupHost(ctx, hostname) if err != nil { return nil, err // not our responsibility to wrap this error @@ -54,29 +54,29 @@ func (r *BogonResolver) LookupHost(ctx context.Context, hostname string) ([]stri } // LookupHTTPS implements Resolver.LookupHTTPS -func (r *BogonResolver) LookupHTTPS(ctx context.Context, hostname string) (*model.HTTPSSvc, error) { +func (r *bogonResolver) LookupHTTPS(ctx context.Context, hostname string) (*model.HTTPSSvc, error) { // TODO(bassosimone): decide whether we want to implement this method or not return nil, ErrNoDNSTransport } // LookupNS implements Resolver.LookupNS -func (r *BogonResolver) LookupNS(ctx context.Context, hostname string) ([]*net.NS, error) { +func (r *bogonResolver) LookupNS(ctx context.Context, hostname string) ([]*net.NS, error) { // TODO(bassosimone): decide whether we want to implement this method or not return nil, ErrNoDNSTransport } // Network implements Resolver.Network -func (r *BogonResolver) Network() string { +func (r *bogonResolver) Network() string { return r.Resolver.Network() } // Address implements Resolver.Address -func (r *BogonResolver) Address() string { +func (r *bogonResolver) Address() string { return r.Resolver.Address() } // CloseIdleConnections implements Resolver.CloseIdleConnections -func (r *BogonResolver) CloseIdleConnections() { +func (r *bogonResolver) CloseIdleConnections() { r.Resolver.CloseIdleConnections() } diff --git a/internal/netxlite/bogon_test.go b/internal/netxlite/bogon_test.go index 8884afdc88..5c45caf240 100644 --- a/internal/netxlite/bogon_test.go +++ b/internal/netxlite/bogon_test.go @@ -13,7 +13,7 @@ func TestMaybeWrapWithBogonResolver(t *testing.T) { t.Run("with enabled equal to true", func(t *testing.T) { underlying := &mocks.Resolver{} reso := MaybeWrapWithBogonResolver(true, underlying) - bogoreso := reso.(*BogonResolver) + bogoreso := reso.(*bogonResolver) if bogoreso.Resolver != underlying { t.Fatal("did not wrap") } @@ -32,7 +32,7 @@ func TestBogonResolver(t *testing.T) { t.Run("LookupHost", func(t *testing.T) { t.Run("with failure", func(t *testing.T) { expected := errors.New("mocked") - reso := &BogonResolver{ + reso := &bogonResolver{ Resolver: &mocks.Resolver{ MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { return nil, expected @@ -51,7 +51,7 @@ func TestBogonResolver(t *testing.T) { t.Run("with success and no bogon", func(t *testing.T) { expected := []string{"8.8.8.8", "149.112.112.112"} - reso := &BogonResolver{ + reso := &bogonResolver{ Resolver: &mocks.Resolver{ MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { return expected, nil @@ -69,7 +69,7 @@ func TestBogonResolver(t *testing.T) { }) t.Run("with success and bogon", func(t *testing.T) { - reso := &BogonResolver{ + reso := &bogonResolver{ Resolver: &mocks.Resolver{ MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { return []string{"8.8.8.8", "10.34.34.35", "149.112.112.112"}, nil @@ -93,7 +93,7 @@ func TestBogonResolver(t *testing.T) { t.Run("LookupHTTPS", func(t *testing.T) { ctx := context.Background() - reso := &BogonResolver{} + reso := &bogonResolver{} https, err := reso.LookupHTTPS(ctx, "dns.google") if !errors.Is(err, ErrNoDNSTransport) { t.Fatal("unexpected err", err) @@ -105,7 +105,7 @@ func TestBogonResolver(t *testing.T) { t.Run("LookupNS", func(t *testing.T) { ctx := context.Background() - reso := &BogonResolver{} + reso := &bogonResolver{} ns, err := reso.LookupNS(ctx, "dns.google") if !errors.Is(err, ErrNoDNSTransport) { t.Fatal("unexpected err", err) @@ -117,7 +117,7 @@ func TestBogonResolver(t *testing.T) { t.Run("Network", func(t *testing.T) { expected := "antani" - reso := &BogonResolver{ + reso := &bogonResolver{ Resolver: &mocks.Resolver{ MockNetwork: func() string { return expected @@ -131,7 +131,7 @@ func TestBogonResolver(t *testing.T) { t.Run("Address", func(t *testing.T) { expected := "antani" - reso := &BogonResolver{ + reso := &bogonResolver{ Resolver: &mocks.Resolver{ MockAddress: func() string { return expected @@ -145,7 +145,7 @@ func TestBogonResolver(t *testing.T) { t.Run("CloseIdleConnections", func(t *testing.T) { var called bool - reso := &BogonResolver{ + reso := &bogonResolver{ Resolver: &mocks.Resolver{ MockCloseIdleConnections: func() { called = true diff --git a/internal/engine/netx/cacheresolver.go b/internal/netxlite/resolvercache.go similarity index 95% rename from internal/engine/netx/cacheresolver.go rename to internal/netxlite/resolvercache.go index 0979c4e994..76ff5ae5b4 100644 --- a/internal/engine/netx/cacheresolver.go +++ b/internal/netxlite/resolvercache.go @@ -1,4 +1,4 @@ -package netx +package netxlite import ( "context" @@ -6,7 +6,6 @@ import ( "sync" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/netxlite" ) // MaybeWrapWithCachingResolver wraps the provided resolver with a resolver @@ -111,10 +110,10 @@ func (r *cacheResolver) CloseIdleConnections() { // LookupHTTPS implements model.Resolver.LookupHTTPS. func (r *cacheResolver) LookupHTTPS(ctx context.Context, domain string) (*model.HTTPSSvc, error) { - return nil, netxlite.ErrNoDNSTransport + return nil, ErrNoDNSTransport } // LookupNS implements model.Resolver.LookupNS. func (r *cacheResolver) LookupNS(ctx context.Context, domain string) ([]*net.NS, error) { - return nil, netxlite.ErrNoDNSTransport + return nil, ErrNoDNSTransport } diff --git a/internal/engine/netx/cacheresolver_test.go b/internal/netxlite/resolvercache_test.go similarity index 97% rename from internal/engine/netx/cacheresolver_test.go rename to internal/netxlite/resolvercache_test.go index 4bda96c8a5..748a8365da 100644 --- a/internal/engine/netx/cacheresolver_test.go +++ b/internal/netxlite/resolvercache_test.go @@ -1,4 +1,4 @@ -package netx +package netxlite import ( "context" @@ -7,7 +7,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/ooni/probe-cli/v3/internal/model/mocks" - "github.com/ooni/probe-cli/v3/internal/netxlite" ) func TestMaybeWrapWithCachingResolver(t *testing.T) { @@ -184,7 +183,7 @@ func TestCacheResolver(t *testing.T) { t.Run("LookupHTTPS", func(t *testing.T) { reso := &cacheResolver{} https, err := reso.LookupHTTPS(context.Background(), "dns.google") - if !errors.Is(err, netxlite.ErrNoDNSTransport) { + if !errors.Is(err, ErrNoDNSTransport) { t.Fatal("unexpected err", err) } if https != nil { @@ -195,7 +194,7 @@ func TestCacheResolver(t *testing.T) { t.Run("LookupNS", func(t *testing.T) { reso := &cacheResolver{} ns, err := reso.LookupNS(context.Background(), "dns.google") - if !errors.Is(err, netxlite.ErrNoDNSTransport) { + if !errors.Is(err, ErrNoDNSTransport) { t.Fatal("unexpected err", err) } if len(ns) != 0 {