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.
2022-06-05 21:58:34 +02:00
parent 6b85dfce88
commit 5d54aa9c5f
5 changed files with 34 additions and 36 deletions
@@ -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()
 }

@@ -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
@@ -0,0 +1,119 @@
+package netxlite
+
+import (
+	"context"
+	"net"
+	"sync"
+
+	"github.com/ooni/probe-cli/v3/internal/model"
+)
+
+// MaybeWrapWithCachingResolver wraps the provided resolver with a resolver
+// that remembers the result of previous successful resolutions, if the enabled
+// argument is true. Otherwise, we return the unmodified provided resolver.
+//
+// Bug: the returned resolver only applies caching to LookupHost and any other
+// lookup operation returns ErrNoDNSTransport to the caller.
+func MaybeWrapWithCachingResolver(enabled bool, reso model.Resolver) model.Resolver {
+	if enabled {
+		reso = &cacheResolver{
+			cache:    map[string][]string{},
+			mu:       sync.Mutex{},
+			readOnly: false,
+			resolver: reso,
+		}
+	}
+	return reso
+}
+
+// MaybeWrapWithStaticDNSCache wraps the provided resolver with a resolver that
+// checks the given cache before issuing queries to the underlying DNS resolver.
+//
+// Bug: the returned resolver only applies caching to LookupHost and any other
+// lookup operation returns ErrNoDNSTransport to the caller.
+func MaybeWrapWithStaticDNSCache(cache map[string][]string, reso model.Resolver) model.Resolver {
+	if len(cache) > 0 {
+		reso = &cacheResolver{
+			cache:    cache,
+			mu:       sync.Mutex{},
+			readOnly: true,
+			resolver: reso,
+		}
+	}
+	return reso
+}
+
+// cacheResolver implements CachingResolver and StaticDNSCache.
+type cacheResolver struct {
+	// cache is the underlying DNS cache.
+	cache map[string][]string
+
+	// mu provides mutual exclusion.
+	mu sync.Mutex
+
+	// readOnly means that we won't cache the result of successful resolutions.
+	readOnly bool
+
+	// resolver is the underlying resolver.
+	resolver model.Resolver
+}
+
+var _ model.Resolver = &cacheResolver{}
+
+// LookupHost implements model.Resolver.LookupHost
+func (r *cacheResolver) LookupHost(
+	ctx context.Context, hostname string) ([]string, error) {
+	if entry := r.get(hostname); entry != nil {
+		return entry, nil
+	}
+	entry, err := r.resolver.LookupHost(ctx, hostname)
+	if err != nil {
+		return nil, err
+	}
+	if !r.readOnly {
+		r.set(hostname, entry)
+	}
+	return entry, nil
+}
+
+// get gets the currently configured entry for domain, or nil
+func (r *cacheResolver) get(domain string) []string {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+	return r.cache[domain]
+}
+
+// set sets a valid inside the cache iff readOnly is false.
+func (r *cacheResolver) set(domain string, addresses []string) {
+	r.mu.Lock()
+	if r.cache == nil {
+		r.cache = make(map[string][]string)
+	}
+	r.cache[domain] = addresses
+	r.mu.Unlock()
+}
+
+// Address implements model.Resolver.Address.
+func (r *cacheResolver) Address() string {
+	return r.resolver.Address()
+}
+
+// Network implements model.Resolver.Network.
+func (r *cacheResolver) Network() string {
+	return r.resolver.Network()
+}
+
+// CloseIdleConnections implements model.Resolver.CloseIdleConnections.
+func (r *cacheResolver) CloseIdleConnections() {
+	r.resolver.CloseIdleConnections()
+}
+
+// LookupHTTPS implements model.Resolver.LookupHTTPS.
+func (r *cacheResolver) LookupHTTPS(ctx context.Context, domain string) (*model.HTTPSSvc, error) {
+	return nil, ErrNoDNSTransport
+}
+
+// LookupNS implements model.Resolver.LookupNS.
+func (r *cacheResolver) LookupNS(ctx context.Context, domain string) ([]*net.NS, error) {
+	return nil, ErrNoDNSTransport
+}
@@ -0,0 +1,205 @@
+package netxlite
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/ooni/probe-cli/v3/internal/model/mocks"
+)
+
+func TestMaybeWrapWithCachingResolver(t *testing.T) {
+	t.Run("with enable equal to true", func(t *testing.T) {
+		underlying := &mocks.Resolver{}
+		reso := MaybeWrapWithCachingResolver(true, underlying)
+		cachereso := reso.(*cacheResolver)
+		if cachereso.resolver != underlying {
+			t.Fatal("did not wrap correctly")
+		}
+	})
+
+	t.Run("with enable equal to false", func(t *testing.T) {
+		underlying := &mocks.Resolver{}
+		reso := MaybeWrapWithCachingResolver(false, underlying)
+		if reso != underlying {
+			t.Fatal("unexpected result")
+		}
+	})
+}
+
+func TestMaybeWrapWithStaticDNSCache(t *testing.T) {
+	t.Run("when the cache is not empty", func(t *testing.T) {
+		cachedDomain := "dns.google"
+		expectedEntry := []string{"8.8.8.8", "8.8.4.4"}
+		underlyingCache := make(map[string][]string)
+		underlyingCache[cachedDomain] = expectedEntry
+		underlyingReso := &mocks.Resolver{}
+		reso := MaybeWrapWithStaticDNSCache(underlyingCache, underlyingReso)
+		cachereso := reso.(*cacheResolver)
+		if diff := cmp.Diff(cachereso.cache, underlyingCache); diff != "" {
+			t.Fatal(diff)
+		}
+		if cachereso.resolver != underlyingReso {
+			t.Fatal("unexpected underlying resolver")
+		}
+	})
+
+	t.Run("when the cache is empty", func(t *testing.T) {
+		underlyingCache := make(map[string][]string)
+		underlyingReso := &mocks.Resolver{}
+		reso := MaybeWrapWithStaticDNSCache(underlyingCache, underlyingReso)
+		if reso != underlyingReso {
+			t.Fatal("unexpected result")
+		}
+	})
+
+	t.Run("when the cache is nil", func(t *testing.T) {
+		var underlyingCache map[string][]string
+		underlyingReso := &mocks.Resolver{}
+		reso := MaybeWrapWithStaticDNSCache(underlyingCache, underlyingReso)
+		if reso != underlyingReso {
+			t.Fatal("unexpected result")
+		}
+	})
+}
+
+func TestCacheResolver(t *testing.T) {
+	t.Run("LookupHost", func(t *testing.T) {
+		t.Run("cache miss and failure", func(t *testing.T) {
+			expected := errors.New("mocked error")
+			r := &mocks.Resolver{
+				MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+					return nil, expected
+				},
+			}
+			cache := &cacheResolver{resolver: r}
+			addrs, err := cache.LookupHost(context.Background(), "www.google.com")
+			if !errors.Is(err, expected) {
+				t.Fatal("not the error we expected")
+			}
+			if addrs != nil {
+				t.Fatal("expected nil addrs here")
+			}
+			if cache.get("www.google.com") != nil {
+				t.Fatal("expected empty cache here")
+			}
+		})
+
+		t.Run("cache hit", func(t *testing.T) {
+			expected := errors.New("mocked error")
+			r := &mocks.Resolver{
+				MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+					return nil, expected
+				},
+			}
+			cache := &cacheResolver{resolver: r}
+			cache.set("dns.google.com", []string{"8.8.8.8"})
+			addrs, err := cache.LookupHost(context.Background(), "dns.google.com")
+			if err != nil {
+				t.Fatal(err)
+			}
+			if len(addrs) != 1 || addrs[0] != "8.8.8.8" {
+				t.Fatal("not the result we expected")
+			}
+		})
+
+		t.Run("cache miss and success with readwrite cache", func(t *testing.T) {
+			r := &mocks.Resolver{
+				MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+					return []string{"8.8.8.8"}, nil
+				},
+			}
+			cache := &cacheResolver{resolver: r}
+			addrs, err := cache.LookupHost(context.Background(), "dns.google.com")
+			if err != nil {
+				t.Fatal(err)
+			}
+			if len(addrs) != 1 || addrs[0] != "8.8.8.8" {
+				t.Fatal("not the result we expected")
+			}
+			if cache.get("dns.google.com")[0] != "8.8.8.8" {
+				t.Fatal("expected full cache here")
+			}
+		})
+
+		t.Run("cache miss and success with readonly cache", func(t *testing.T) {
+			r := &mocks.Resolver{
+				MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+					return []string{"8.8.8.8"}, nil
+				},
+			}
+			cache := &cacheResolver{resolver: r, readOnly: true}
+			addrs, err := cache.LookupHost(context.Background(), "dns.google.com")
+			if err != nil {
+				t.Fatal(err)
+			}
+			if len(addrs) != 1 || addrs[0] != "8.8.8.8" {
+				t.Fatal("not the result we expected")
+			}
+			if cache.get("dns.google.com") != nil {
+				t.Fatal("expected empty cache here")
+			}
+		})
+
+		t.Run("Address", func(t *testing.T) {
+			underlying := &mocks.Resolver{
+				MockAddress: func() string {
+					return "x"
+				},
+			}
+			reso := &cacheResolver{resolver: underlying}
+			if reso.Address() != "x" {
+				t.Fatal("unexpected result")
+			}
+		})
+
+		t.Run("Network", func(t *testing.T) {
+			underlying := &mocks.Resolver{
+				MockNetwork: func() string {
+					return "x"
+				},
+			}
+			reso := &cacheResolver{resolver: underlying}
+			if reso.Network() != "x" {
+				t.Fatal("unexpected result")
+			}
+		})
+
+		t.Run("CloseIdleConnections", func(t *testing.T) {
+			var called bool
+			underlying := &mocks.Resolver{
+				MockCloseIdleConnections: func() {
+					called = true
+				},
+			}
+			reso := &cacheResolver{resolver: underlying}
+			reso.CloseIdleConnections()
+			if !called {
+				t.Fatal("not called")
+			}
+		})
+
+		t.Run("LookupHTTPS", func(t *testing.T) {
+			reso := &cacheResolver{}
+			https, err := reso.LookupHTTPS(context.Background(), "dns.google")
+			if !errors.Is(err, ErrNoDNSTransport) {
+				t.Fatal("unexpected err", err)
+			}
+			if https != nil {
+				t.Fatal("expected nil")
+			}
+		})
+
+		t.Run("LookupNS", func(t *testing.T) {
+			reso := &cacheResolver{}
+			ns, err := reso.LookupNS(context.Background(), "dns.google")
+			if !errors.Is(err, ErrNoDNSTransport) {
+				t.Fatal("unexpected err", err)
+			}
+			if len(ns) != 0 {
+				t.Fatal("expected zero length slice")
+			}
+		})
+	})
+}