refactor(netxlite): use *Netx for the system resolver (ooni#1248)

This diff modifies how we construct netxlite's system resolver such that
public functions use the *Netx equivalents.

While there, recognize that there wasn't enough testing for the optional
wrappers provided by `model.DNSTransportWrapper` and that we are not
using this functionality. So, rather than writing new tests for this
functionality, we can actually just drop it and simplify the codebase.

While there, recognize that `netxlite.WrapDNSTransport` could easily
become private.

While there, recognize that `./legacy/netx` needs lots of public
function being exported by `netxlite` but we don't need to expose that
many implementation details to new code that will be using `*Netx`. So,
make sure all the new methods we create for `*Netx` are actually private
methods. (Ideally, the API surface of `netxlite` should be smaller; we
would not be able to get there for quite some time, but we can at least
avoid increasing the API surface.)

The general idea for which I am pushing here is to have additional
clarity about dependencies, to better analyze the requirements of non
measuring code for ooni/probe#2531.

internal/cmd/gardener/internal/dnsreport/dnsreport.go

@@ -8,7 +8,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"net"
-	"net/http"
 	"net/url"
 	"os"
 	"path/filepath"
@@ -254,19 +253,8 @@ func (s *Subcommand) dnsLookupHost(domain string) ([]string, error) {
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 	defer cancel()
 
-	// create DNS transport using HTTP default client
-	dnsTransport := netxlite.WrapDNSTransport(&netxlite.DNSOverHTTPSTransport{
-		Client:       http.DefaultClient,
-		Decoder:      &netxlite.DNSDecoderMiekg{},
-		URL:          s.DNSOverHTTPSServerURL,
-		HostOverride: "",
-	})
-
-	// create DNS resolver
-	dnsResolver := netxlite.WrapResolver(
-		log.Log,
-		netxlite.NewUnwrappedParallelResolver(dnsTransport),
-	)
+	dnsResolver := netxlite.NewParallelDNSOverHTTPSResolver(log.Log, s.DNSOverHTTPSServerURL)
+	defer dnsResolver.CloseIdleConnections()
 
 	// lookup for both A and AAAA entries
 	return dnsResolver.LookupHost(ctx, domain)

internal/model/netx.go

@@ -105,12 +105,6 @@ type DNSEncoder interface {
 	Encode(domain string, qtype uint16, padding bool) DNSQuery
 }
 
-// DNSTransportWrapper is a type that takes in input a DNSTransport
-// and returns in output a wrapped DNSTransport.
-type DNSTransportWrapper interface {
-	WrapDNSTransport(txp DNSTransport) DNSTransport
-}
-
 // DNSTransport represents an abstract DNS transport.
 type DNSTransport interface {
 	// RoundTrip sends a DNS query and receives the reply.

internal/netxlite/dialer_test.go

@@ -24,7 +24,7 @@ func TestNewDialerWithStdlibResolver(t *testing.T) {
 	}
 	// typecheck the resolver
 	reso := logger.Dialer.(*dialerResolverWithTracing)
-	typecheckForSystemResolver(t, reso.Resolver, model.DiscardLogger)
+	typeCheckForSystemResolver(t, reso.Resolver, model.DiscardLogger)
 	// typecheck the dialer
 	logger = reso.Dialer.(*dialerLogger)
 	if logger.DebugLogger != model.DiscardLogger {

internal/netxlite/dnsovergetaddrinfo.go

@@ -27,9 +27,14 @@ type dnsOverGetaddrinfoTransport struct {
 	provider *MaybeCustomUnderlyingNetwork
 }
 
+func (netx *Netx) newDNSOverGetaddrinfoTransport() model.DNSTransport {
+	return &dnsOverGetaddrinfoTransport{provider: netx.maybeCustomUnderlyingNetwork()}
+}
+
 // NewDNSOverGetaddrinfoTransport creates a new dns-over-getaddrinfo transport.
 func NewDNSOverGetaddrinfoTransport() model.DNSTransport {
-	return &dnsOverGetaddrinfoTransport{}
+	netx := &Netx{Underlying: nil}
+	return netx.newDNSOverGetaddrinfoTransport()
 }
 
 var _ model.DNSTransport = &dnsOverGetaddrinfoTransport{}

internal/netxlite/dnsoverhttps.go

@@ -46,13 +46,13 @@ func NewUnwrappedDNSOverHTTPSTransport(client model.HTTPClient, URL string) *DNS
 // NewDNSOverHTTPSTransport is like NewUnwrappedDNSOverHTTPSTransport but
 // returns an already wrapped DNSTransport.
 func NewDNSOverHTTPSTransport(client model.HTTPClient, URL string) model.DNSTransport {
-	return WrapDNSTransport(NewUnwrappedDNSOverHTTPSTransport(client, URL))
+	return wrapDNSTransport(NewUnwrappedDNSOverHTTPSTransport(client, URL))
 }
 
 // NewDNSOverHTTPSTransportWithHTTPTransport is like NewDNSOverHTTPSTransport
 // but takes in input an HTTPTransport rather than an HTTPClient.
 func NewDNSOverHTTPSTransportWithHTTPTransport(txp model.HTTPTransport, URL string) model.DNSTransport {
-	return WrapDNSTransport(NewUnwrappedDNSOverHTTPSTransport(NewHTTPClient(txp), URL))
+	return wrapDNSTransport(NewUnwrappedDNSOverHTTPSTransport(NewHTTPClient(txp), URL))
 }
 
 // NewUnwrappedDNSOverHTTPSTransportWithHostOverride creates a new DNSOverHTTPSTransport

internal/netxlite/dnstransport.go

@@ -10,20 +10,11 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
-// WrapDNSTransport wraps a DNSTransport to provide error wrapping. This function will
-// apply all the provided wrappers around the default transport wrapping. If any of the
-// wrappers is nil, we just silently and gracefully ignore it.
-func WrapDNSTransport(txp model.DNSTransport,
-	wrappers ...model.DNSTransportWrapper) (out model.DNSTransport) {
+// wrapDNSTransport wraps a DNSTransport to provide error wrapping.
+func wrapDNSTransport(txp model.DNSTransport) (out model.DNSTransport) {
 	out = &dnsTransportErrWrapper{
 		DNSTransport: txp,
 	}
-	for _, wrapper := range wrappers {
-		if wrapper == nil {
-			continue // skip as documented
-		}
-		out = wrapper.WrapDNSTransport(out) // compose with user-provided wrappers
-	}
 	return
 }
 

internal/netxlite/dnstransport_test.go

@@ -10,37 +10,10 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
-type dnsTransportExtensionFirst struct {
-	model.DNSTransport
-}
-
-type dnsTransportWrapperFirst struct{}
-
-func (*dnsTransportWrapperFirst) WrapDNSTransport(txp model.DNSTransport) model.DNSTransport {
-	return &dnsTransportExtensionFirst{txp}
-}
-
-type dnsTransportExtensionSecond struct {
-	model.DNSTransport
-}
-
-type dnsTransportWrapperSecond struct{}
-
-func (*dnsTransportWrapperSecond) WrapDNSTransport(txp model.DNSTransport) model.DNSTransport {
-	return &dnsTransportExtensionSecond{txp}
-}
-
 func TestWrapDNSTransport(t *testing.T) {
 	orig := &mocks.DNSTransport{}
-	extensions := []model.DNSTransportWrapper{
-		&dnsTransportWrapperFirst{},
-		nil, // explicitly test for documented use case
-		&dnsTransportWrapperSecond{},
-	}
-	txp := WrapDNSTransport(orig, extensions...)
-	ext2 := txp.(*dnsTransportExtensionSecond)
-	ext1 := ext2.DNSTransport.(*dnsTransportExtensionFirst)
-	errWrapper := ext1.DNSTransport.(*dnsTransportErrWrapper)
+	txp := wrapDNSTransport(orig)
+	errWrapper := txp.(*dnsTransportErrWrapper)
 	underlying := errWrapper.DNSTransport
 	if orig != underlying {
 		t.Fatal("unexpected underlying transport")

internal/netxlite/netx.go

@@ -17,30 +17,21 @@ type Netx struct {
 	Underlying model.UnderlyingNetwork
 }
 
-// tproxyNilSafeProvider wraps the [model.UnderlyingNetwork] using a [tproxyNilSafeProvider].
-func (n *Netx) tproxyNilSafeProvider() *MaybeCustomUnderlyingNetwork {
-	return &MaybeCustomUnderlyingNetwork{n.Underlying}
-}
-
-// NewStdlibResolver is like [netxlite.NewStdlibResolver] but the constructed [model.Resolver]
-// uses the [model.UnderlyingNetwork] configured inside the [Netx] structure.
-func (n *Netx) NewStdlibResolver(logger model.DebugLogger, wrappers ...model.DNSTransportWrapper) model.Resolver {
-	unwrapped := &resolverSystem{
-		t: WrapDNSTransport(&dnsOverGetaddrinfoTransport{provider: n.tproxyNilSafeProvider()}, wrappers...),
-	}
-	return WrapResolver(logger, unwrapped)
+// maybeCustomUnderlyingNetwork wraps the [model.UnderlyingNetwork] using a [*MaybeCustomUnderlyingNetwork].
+func (netx *Netx) maybeCustomUnderlyingNetwork() *MaybeCustomUnderlyingNetwork {
+	return &MaybeCustomUnderlyingNetwork{netx.Underlying}
 }
 
 // NewDialerWithResolver is like [netxlite.NewDialerWithResolver] but the constructed [model.Dialer]
 // uses the [model.UnderlyingNetwork] configured inside the [Netx] structure.
 func (n *Netx) NewDialerWithResolver(dl model.DebugLogger, r model.Resolver, w ...model.DialerWrapper) model.Dialer {
-	return WrapDialer(dl, r, &DialerSystem{provider: n.tproxyNilSafeProvider()}, w...)
+	return WrapDialer(dl, r, &DialerSystem{provider: n.maybeCustomUnderlyingNetwork()}, w...)
 }
 
 // NewUDPListener is like [netxlite.NewUDPListener] but the constructed [model.UDPListener]
 // uses the [model.UnderlyingNetwork] configured inside the [Netx] structure.
 func (n *Netx) NewUDPListener() model.UDPListener {
-	return &udpListenerErrWrapper{&udpListenerStdlib{provider: n.tproxyNilSafeProvider()}}
+	return &udpListenerErrWrapper{&udpListenerStdlib{provider: n.maybeCustomUnderlyingNetwork()}}
 }
 
 // NewQUICDialerWithResolver is like [netxlite.NewQUICDialerWithResolver] but the constructed
@@ -49,15 +40,15 @@ func (n *Netx) NewQUICDialerWithResolver(listener model.UDPListener, logger mode
 	resolver model.Resolver, wrappers ...model.QUICDialerWrapper) (outDialer model.QUICDialer) {
 	baseDialer := &quicDialerQUICGo{
 		UDPListener: listener,
-		provider:    n.tproxyNilSafeProvider(),
+		provider:    n.maybeCustomUnderlyingNetwork(),
 	}
 	return WrapQUICDialer(logger, resolver, baseDialer, wrappers...)
 }
 
 // NewTLSHandshakerStdlib is like [netxlite.NewTLSHandshakerStdlib] but the constructed [model.TLSHandshaker]
 // uses the [model.UnderlyingNetwork] configured inside the [Netx] structure.
 func (n *Netx) NewTLSHandshakerStdlib(logger model.DebugLogger) model.TLSHandshaker {
-	return newTLSHandshakerLogger(&tlsHandshakerConfigurable{provider: n.tproxyNilSafeProvider()}, logger)
+	return newTLSHandshakerLogger(&tlsHandshakerConfigurable{provider: n.maybeCustomUnderlyingNetwork()}, logger)
 }
 
 // NewHTTPTransportStdlib is like [netxlite.NewHTTPTransportStdlib] but the constructed [model.HTTPTransport]

internal/netxlite/resolvercore.go

@@ -24,29 +24,39 @@ import (
 var ErrNoDNSTransport = errors.New("operation requires a DNS transport")
 
 // NewStdlibResolver creates a new Resolver by combining WrapResolver
-// with an internal "stdlib" resolver type. The list of optional wrappers
-// allow to wrap the underlying getaddrinfo transport. Any nil wrapper
-// will be silently ignored by the code that performs the wrapping.
-func NewStdlibResolver(logger model.DebugLogger, wrappers ...model.DNSTransportWrapper) model.Resolver {
-	return WrapResolver(logger, NewUnwrappedStdlibResolver(wrappers...))
+// with an internal "stdlib" resolver type.
+func (netx *Netx) NewStdlibResolver(logger model.DebugLogger) model.Resolver {
+	return WrapResolver(logger, netx.newUnwrappedStdlibResolver())
+}
+
+// NewStdlibResolver is equivalent to creating an empty [*Netx]
+// and callings its NewStdlibResolver method.
+func NewStdlibResolver(logger model.DebugLogger) model.Resolver {
+	netx := &Netx{Underlying: nil}
+	return netx.NewStdlibResolver(logger)
 }
 
 // NewParallelDNSOverHTTPSResolver creates a new DNS over HTTPS resolver
 // that uses the standard library for all operations. This function constructs
 // all the building blocks and calls WrapResolver on the returned resolver.
 func NewParallelDNSOverHTTPSResolver(logger model.DebugLogger, URL string) model.Resolver {
 	client := &http.Client{Transport: NewHTTPTransportStdlib(logger)}
-	txp := WrapDNSTransport(NewUnwrappedDNSOverHTTPSTransport(client, URL))
+	txp := wrapDNSTransport(NewUnwrappedDNSOverHTTPSTransport(client, URL))
 	return WrapResolver(logger, NewUnwrappedParallelResolver(txp))
 }
 
+func (netx *Netx) newUnwrappedStdlibResolver() model.Resolver {
+	return &resolverSystem{
+		t: wrapDNSTransport(netx.newDNSOverGetaddrinfoTransport()),
+	}
+}
+
 // NewUnwrappedStdlibResolver returns a new, unwrapped resolver using the standard
 // library (i.e., getaddrinfo if possible and &net.Resolver{} otherwise). As the name
 // implies, this function returns an unwrapped resolver.
-func NewUnwrappedStdlibResolver(wrappers ...model.DNSTransportWrapper) model.Resolver {
-	return &resolverSystem{
-		t: WrapDNSTransport(NewDNSOverGetaddrinfoTransport(), wrappers...),
-	}
+func NewUnwrappedStdlibResolver() model.Resolver {
+	netx := &Netx{Underlying: nil}
+	return netx.newUnwrappedStdlibResolver()
 }
 
 // NewSerialUDPResolver creates a new Resolver using DNS-over-UDP
@@ -61,13 +71,9 @@ func NewUnwrappedStdlibResolver(wrappers ...model.DNSTransportWrapper) model.Res
 // - dialer is the dialer to create and connect UDP conns
 //
 // - address is the server address (e.g., 1.1.1.1:53)
-//
-// - wrappers is the optional list of wrappers to wrap the underlying
-// transport.  Any nil wrapper will be silently ignored.
-func NewSerialUDPResolver(logger model.DebugLogger, dialer model.Dialer,
-	address string, wrappers ...model.DNSTransportWrapper) model.Resolver {
+func NewSerialUDPResolver(logger model.DebugLogger, dialer model.Dialer, address string) model.Resolver {
 	return WrapResolver(logger, NewUnwrappedSerialResolver(
-		WrapDNSTransport(NewUnwrappedDNSOverUDPTransport(dialer, address), wrappers...),
+		wrapDNSTransport(NewUnwrappedDNSOverUDPTransport(dialer, address)),
 	))
 }
 
@@ -81,13 +87,9 @@ func NewSerialUDPResolver(logger model.DebugLogger, dialer model.Dialer,
 // - dialer is the dialer to create and connect UDP conns
 //
 // - address is the server address (e.g., 1.1.1.1:53)
-//
-// - wrappers is the optional list of wrappers to wrap the underlying
-// transport.  Any nil wrapper will be silently ignored.
-func NewParallelUDPResolver(logger model.DebugLogger, dialer model.Dialer,
-	address string, wrappers ...model.DNSTransportWrapper) model.Resolver {
+func NewParallelUDPResolver(logger model.DebugLogger, dialer model.Dialer, address string) model.Resolver {
 	return WrapResolver(logger, NewUnwrappedParallelResolver(
-		WrapDNSTransport(NewUnwrappedDNSOverUDPTransport(dialer, address), wrappers...),
+		wrapDNSTransport(NewUnwrappedDNSOverUDPTransport(dialer, address)),
 	))
 }
 

internal/netxlite/resolvercore_test.go

@@ -17,7 +17,7 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/testingx"
 )
 
-func typecheckForSystemResolver(t *testing.T, resolver model.Resolver, logger model.DebugLogger) {
+func typeCheckForSystemResolver(t *testing.T, resolver model.Resolver, logger model.DebugLogger) {
 	idna := resolver.(*resolverIDNA)
 	loggerReso := idna.Resolver.(*resolverLogger)
 	if loggerReso.Logger != logger {
@@ -32,7 +32,7 @@ func typecheckForSystemResolver(t *testing.T, resolver model.Resolver, logger mo
 
 func TestNewResolverSystem(t *testing.T) {
 	resolver := NewStdlibResolver(model.DiscardLogger)
-	typecheckForSystemResolver(t, resolver, model.DiscardLogger)
+	typeCheckForSystemResolver(t, resolver, model.DiscardLogger)
 }
 
 func TestNewSerialUDPResolver(t *testing.T) {

internal/testingx/tlssniproxy.go

@@ -17,7 +17,7 @@ import (
 // TLSSNIProxyNetx is how [TLSSNIProxy] views [*netxlite.Netx].
 type TLSSNIProxyNetx interface {
 	NewDialerWithResolver(dl model.DebugLogger, r model.Resolver, w ...model.DialerWrapper) model.Dialer
-	NewStdlibResolver(logger model.DebugLogger, wrappers ...model.DNSTransportWrapper) model.Resolver
+	NewStdlibResolver(logger model.DebugLogger) model.Resolver
 }
 
 // TLSSNIProxy is a proxy using the SNI to figure out where to connect to.