cleanup(netx): stop using most netxlite resolver internals (#797)

This diff modifies netx to stop using most netxlite resolver internals
but the internal function that creates a new, unwrapped system resolver,
which will be dealt with in a subsequent pull request.

See https://github.com/ooni/probe/issues/2121

internal/engine/netx/netx.go

@@ -63,10 +63,10 @@ func NewResolver(config Config) model.Resolver {
 	if config.BaseResolver == nil {
 		config.BaseResolver = netxlite.NewResolverSystem()
 	}
-	var r model.Resolver = config.BaseResolver
+	r := netxlite.WrapResolver(
-	r = &netxlite.AddressResolver{
+		model.ValidLoggerOrDefault(config.Logger),
-		Resolver: r,
+		config.BaseResolver,
-	}
+	)
 	if config.CacheResolutions {
 		r = &CacheResolver{Resolver: r}
 	}
@@ -80,15 +80,7 @@ func NewResolver(config Config) model.Resolver {
 	if config.BogonIsError {
 		r = &netxlite.BogonResolver{Resolver: r}
 	}
-	r = &netxlite.ErrorWrapperResolver{Resolver: r}
+	return config.Saver.WrapResolver(r) // WAI when config.Saver==nil
-	if config.Logger != nil {
-		r = &netxlite.ResolverLogger{
-			Logger:   config.Logger,
-			Resolver: r,
-		}
-	}
-	r = config.Saver.WrapResolver(r) // WAI when config.Saver==nil
-	return &netxlite.ResolverIDNA{Resolver: r}
 }
 
 // NewDialer creates a new Dialer from the specified config

internal/engine/netx/netx_test.go

@@ -17,198 +17,6 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/tracex"
 )
 
-func TestNewResolverVanilla(t *testing.T) {
-	r := NewResolver(Config{})
-	ir, ok := r.(*netxlite.ResolverIDNA)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	ewr, ok := ir.Resolver.(*netxlite.ErrorWrapperResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	ar, ok := ewr.Resolver.(*netxlite.AddressResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	_, ok = ar.Resolver.(*netxlite.ResolverSystemDoNotInstantiate)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-}
-
-func TestNewResolverSpecificResolver(t *testing.T) {
-	r := NewResolver(Config{
-		BaseResolver: &netxlite.BogonResolver{
-			// not initialized because it doesn't matter in this context
-		},
-	})
-	ir, ok := r.(*netxlite.ResolverIDNA)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	ewr, ok := ir.Resolver.(*netxlite.ErrorWrapperResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	ar, ok := ewr.Resolver.(*netxlite.AddressResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	_, ok = ar.Resolver.(*netxlite.BogonResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-}
-
-func TestNewResolverWithBogonFilter(t *testing.T) {
-	r := NewResolver(Config{
-		BogonIsError: true,
-	})
-	ir, ok := r.(*netxlite.ResolverIDNA)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	ewr, ok := ir.Resolver.(*netxlite.ErrorWrapperResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	br, ok := ewr.Resolver.(*netxlite.BogonResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	ar, ok := br.Resolver.(*netxlite.AddressResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	_, ok = ar.Resolver.(*netxlite.ResolverSystemDoNotInstantiate)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-}
-
-func TestNewResolverWithLogging(t *testing.T) {
-	r := NewResolver(Config{
-		Logger: log.Log,
-	})
-	ir, ok := r.(*netxlite.ResolverIDNA)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	lr, ok := ir.Resolver.(*netxlite.ResolverLogger)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	if lr.Logger != log.Log {
-		t.Fatal("not the logger we expected")
-	}
-	ewr, ok := lr.Resolver.(*netxlite.ErrorWrapperResolver)
-	if !ok {
-		t.Fatalf("not the resolver we expected")
-	}
-	ar, ok := ewr.Resolver.(*netxlite.AddressResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	_, ok = ar.Resolver.(*netxlite.ResolverSystemDoNotInstantiate)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-}
-
-func TestNewResolverWithSaver(t *testing.T) {
-	saver := new(tracex.Saver)
-	r := NewResolver(Config{
-		Saver: saver,
-	})
-	ir, ok := r.(*netxlite.ResolverIDNA)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	sr, ok := ir.Resolver.(*tracex.ResolverSaver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	if sr.Saver != saver {
-		t.Fatal("not the saver we expected")
-	}
-	ewr, ok := sr.Resolver.(*netxlite.ErrorWrapperResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	ar, ok := ewr.Resolver.(*netxlite.AddressResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	_, ok = ar.Resolver.(*netxlite.ResolverSystemDoNotInstantiate)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-}
-
-func TestNewResolverWithReadWriteCache(t *testing.T) {
-	r := NewResolver(Config{
-		CacheResolutions: true,
-	})
-	ir, ok := r.(*netxlite.ResolverIDNA)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	ewr, ok := ir.Resolver.(*netxlite.ErrorWrapperResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	cr, ok := ewr.Resolver.(*CacheResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	if cr.ReadOnly != false {
-		t.Fatal("expected readwrite cache here")
-	}
-	ar, ok := cr.Resolver.(*netxlite.AddressResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	_, ok = ar.Resolver.(*netxlite.ResolverSystemDoNotInstantiate)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-}
-
-func TestNewResolverWithPrefilledReadonlyCache(t *testing.T) {
-	r := NewResolver(Config{
-		DNSCache: map[string][]string{
-			"dns.google.com": {"8.8.8.8"},
-		},
-	})
-	ir, ok := r.(*netxlite.ResolverIDNA)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	ewr, ok := ir.Resolver.(*netxlite.ErrorWrapperResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	cr, ok := ewr.Resolver.(*CacheResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	if cr.ReadOnly != true {
-		t.Fatal("expected readonly cache here")
-	}
-	if cr.Get("dns.google.com")[0] != "8.8.8.8" {
-		t.Fatal("cache not correctly prefilled")
-	}
-	ar, ok := cr.Resolver.(*netxlite.AddressResolver)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	_, ok = ar.Resolver.(*netxlite.ResolverSystemDoNotInstantiate)
-	if !ok {
-		t.Fatal("not the resolver we expected")
-	}
-}
-
 func TestNewTLSDialer(t *testing.T) {
 	t.Run("we always have error wrapping", func(t *testing.T) {
 		server := filtering.NewTLSServer(filtering.TLSActionReset)
@@ -345,30 +153,6 @@ func TestNewDNSClientUnsupportedScheme(t *testing.T) {
 	}
 }
 
-func TestNewDNSClientSystemResolver(t *testing.T) {
-	dnsclient, err := NewDNSClient(
-		Config{}, "system:///")
-	if err != nil {
-		t.Fatal(err)
-	}
-	if _, ok := dnsclient.(*netxlite.ResolverSystemDoNotInstantiate); !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	dnsclient.CloseIdleConnections()
-}
-
-func TestNewDNSClientEmpty(t *testing.T) {
-	dnsclient, err := NewDNSClient(
-		Config{}, "")
-	if err != nil {
-		t.Fatal(err)
-	}
-	if _, ok := dnsclient.(*netxlite.ResolverSystemDoNotInstantiate); !ok {
-		t.Fatal("not the resolver we expected")
-	}
-	dnsclient.CloseIdleConnections()
-}
-
 func TestNewDNSClientPowerdnsDoH(t *testing.T) {
 	dnsclient, err := NewDNSClient(
 		Config{}, "doh://powerdns")

internal/netxlite/bogon.go

@@ -16,6 +16,12 @@ import (
 
 // 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 {
 	Resolver model.Resolver
 }
@@ -26,11 +32,12 @@ var _ model.Resolver = &BogonResolver{}
 func (r *BogonResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) {
 	addrs, err := r.Resolver.LookupHost(ctx, hostname)
 	if err != nil {
-		return nil, err
+		return nil, err // not our responsibility to wrap this error
 	}
 	for _, addr := range addrs {
 		if IsBogon(addr) {
-			return nil, ErrDNSBogon
+			// wrap ErrDNSBogon as documented
+			return nil, newErrWrapper(classifyResolverError, ResolveOperation, ErrDNSBogon)
 		}
 	}
 	return addrs, nil

internal/netxlite/bogon_test.go

@@ -62,6 +62,10 @@ func TestBogonResolver(t *testing.T) {
 			if !errors.Is(err, ErrDNSBogon) {
 				t.Fatal("unexpected err", err)
 			}
+			var ew *ErrWrapper
+			if !errors.As(err, &ew) {
+				t.Fatal("error has not been wrapped")
+			}
 			if len(addrs) > 0 {
 				t.Fatal("expected no addrs")
 			}

internal/netxlite/legacy.go

@@ -9,16 +9,4 @@ package netxlite
 // Deprecated: do not use these names in new code.
 var (
 	NewResolverSystem = newResolverSystem
-	DefaultResolver   = newResolverSystem()
-)
-
-// These types export internal names to legacy ooni/probe-cli code.
-//
-// Deprecated: do not use these names in new code.
-type (
-	ErrorWrapperResolver           = resolverErrWrapper
-	ResolverSystemDoNotInstantiate = resolverSystem // instantiate => crash w/ nil transport
-	ResolverLogger                 = resolverLogger
-	ResolverIDNA                   = resolverIDNA
-	AddressResolver                = resolverShortCircuitIPAddr
 )