cleanup(netx): another batch of small/simple cleanups (#789)

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

internal/engine/netx/dialer.go

@@ -1,63 +0,0 @@
-package netx
-
-import (
-	"net/url"
-
-	"github.com/ooni/probe-cli/v3/internal/bytecounter"
-	"github.com/ooni/probe-cli/v3/internal/model"
-	"github.com/ooni/probe-cli/v3/internal/netxlite"
-	"github.com/ooni/probe-cli/v3/internal/tracex"
-)
-
-// dialerConfig contains the settings for New.
-type dialerConfig struct {
-	// ContextByteCounting optionally configures context-based
-	// byte counting. By default we don't do that.
-	//
-	// Use WithExperimentByteCounter and WithSessionByteCounter
-	// to assign byte counters to a context. The code will use
-	// corresponding, private functions to access the configured
-	// byte counters and will notify them about I/O events.
-	//
-	// Bug
-	//
-	// This implementation cannot properly account for the bytes that are sent by
-	// persistent connections, because they stick to the counters set when the
-	// connection was established. This typically means we miss the bytes sent and
-	// received when submitting a measurement. Such bytes are specifically not
-	// seen by the experiment specific byte counter.
-	//
-	// For this reason, this implementation may be heavily changed/removed.
-	ContextByteCounting bool
-
-	// DialSaver is the optional saver for dialing events. If not
-	// set, we will not save any dialing event.
-	DialSaver *tracex.Saver
-
-	// Logger is the optional logger. If not set, there
-	// will be no logging from the new dialer.
-	Logger model.DebugLogger
-
-	// ProxyURL is the optional proxy URL.
-	ProxyURL *url.URL
-
-	// ReadWriteSaver is like DialSaver but for I/O events.
-	ReadWriteSaver *tracex.Saver
-}
-
-// newDialer creates a new Dialer from the specified config and resolver.
-func newDialer(config *dialerConfig, resolver model.Resolver) model.Dialer {
-	var logger model.DebugLogger = model.DiscardLogger
-	if config.Logger != nil {
-		logger = config.Logger
-	}
-	d := netxlite.NewDialerWithResolver(
-		logger, resolver, config.DialSaver.NewConnectObserver(),
-		config.ReadWriteSaver.NewReadWriteObserver(),
-	)
-	d = &netxlite.MaybeProxyDialer{ProxyURL: config.ProxyURL, Dialer: d}
-	if config.ContextByteCounting {
-		d = &bytecounter.ContextAwareDialer{Dialer: d}
-	}
-	return d
-}

internal/engine/netx/dialer_test.go

@@ -1,48 +0,0 @@
-package netx
-
-import (
-	"net/http"
-	"net/url"
-	"testing"
-
-	"github.com/apex/log"
-	"github.com/ooni/probe-cli/v3/internal/bytecounter"
-	"github.com/ooni/probe-cli/v3/internal/netxlite"
-	"github.com/ooni/probe-cli/v3/internal/tracex"
-)
-
-func TestNewCreatesTheExpectedChain(t *testing.T) {
-	saver := &tracex.Saver{}
-	dlr := newDialer(&dialerConfig{
-		ContextByteCounting: true,
-		DialSaver:           saver,
-		Logger:              log.Log,
-		ProxyURL:            &url.URL{},
-		ReadWriteSaver:      saver,
-	}, netxlite.DefaultResolver)
-	bcd, ok := dlr.(*bytecounter.ContextAwareDialer)
-	if !ok {
-		t.Fatal("not a byteCounterDialer")
-	}
-	_, ok = bcd.Dialer.(*netxlite.MaybeProxyDialer)
-	if !ok {
-		t.Fatal("not a proxyDialer")
-	}
-	// We can safely stop here: the rest is tested by
-	// the internal/netxlite package
-}
-
-func TestDialerNewSuccess(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skip test in short mode")
-	}
-	log.SetLevel(log.DebugLevel)
-	d := newDialer(&dialerConfig{Logger: log.Log}, netxlite.DefaultResolver)
-	txp := &http.Transport{DialContext: d.DialContext}
-	client := &http.Client{Transport: txp}
-	resp, err := client.Get("http://www.google.com")
-	if err != nil {
-		t.Fatal(err)
-	}
-	resp.Body.Close()
-}

internal/engine/netx/netx.go

@@ -22,7 +22,6 @@
 package netx
 
 import (
-	"context"
 	"crypto/tls"
 	"crypto/x509"
 	"errors"
@@ -55,7 +54,7 @@ type Config struct {
 	QUICDialer          model.QUICDialer     // default: quicdialer.DNSDialer
 	HTTP3Enabled        bool                 // default: disabled
 	HTTPSaver           *tracex.Saver        // default: not saving HTTP
-	Logger              model.DebugLogger    // default: no logging
+	Logger              model.Logger         // default: no logging
 	NoTLSVerify         bool                 // default: perform TLS verify
 	ProxyURL            *url.URL             // default: no proxy
 	ReadWriteSaver      *tracex.Saver        // default: not saving read/write
@@ -65,11 +64,6 @@ type Config struct {
 	TLSSaver            *tracex.Saver        // default: not saving TLS
 }
 
-type tlsHandshaker interface {
-	Handshake(ctx context.Context, conn net.Conn, config *tls.Config) (
-		net.Conn, tls.ConnectionState, error)
-}
-
 var defaultCertPool *x509.CertPool = netxlite.NewDefaultCertPool()
 
 // NewResolver creates a new resolver from the specified config
@@ -110,13 +104,16 @@ func NewDialer(config Config) model.Dialer {
 	if config.FullResolver == nil {
 		config.FullResolver = NewResolver(config)
 	}
-	return newDialer(&dialerConfig{
+	logger := model.ValidLoggerOrDefault(config.Logger)
-		ContextByteCounting: config.ContextByteCounting,
+	d := netxlite.NewDialerWithResolver(
-		DialSaver:           config.DialSaver,
+		logger, config.FullResolver, config.DialSaver.NewConnectObserver(),
-		Logger:              config.Logger,
+		config.ReadWriteSaver.NewReadWriteObserver(),
-		ProxyURL:            config.ProxyURL,
+	)
-		ReadWriteSaver:      config.ReadWriteSaver,
+	d = netxlite.NewMaybeProxyDialer(d, config.ProxyURL)
-	}, config.FullResolver)
+	if config.ContextByteCounting {
+		d = &bytecounter.ContextAwareDialer{Dialer: d}
+	}
+	return d
 }
 
 // NewQUICDialer creates a new DNS Dialer for QUIC, with the resolver from the specified config
@@ -124,11 +121,9 @@ func NewQUICDialer(config Config) model.QUICDialer {
 	if config.FullResolver == nil {
 		config.FullResolver = NewResolver(config)
 	}
+	// TODO(bassosimone): we should count the bytes consumed by this QUIC dialer
 	ql := config.ReadWriteSaver.WrapQUICListener(netxlite.NewQUICListener())
-	var logger model.DebugLogger = model.DiscardLogger
+	logger := model.ValidLoggerOrDefault(config.Logger)
-	if config.Logger != nil {
-		logger = config.Logger
-	}
 	return netxlite.NewQUICDialerWithResolver(ql, logger, config.FullResolver, config.TLSSaver)
 }
 
@@ -137,7 +132,7 @@ func NewTLSDialer(config Config) model.TLSDialer {
 	if config.Dialer == nil {
 		config.Dialer = NewDialer(config)
 	}
-	var h tlsHandshaker = &netxlite.TLSHandshakerConfigurable{}
+	var h model.TLSHandshaker = &netxlite.TLSHandshakerConfigurable{}
 	h = &netxlite.ErrorWrapperTLSHandshaker{TLSHandshaker: h}
 	if config.Logger != nil {
 		h = &netxlite.TLSHandshakerLogger{DebugLogger: config.Logger, TLSHandshaker: h}
@@ -170,12 +165,10 @@ func NewHTTPTransport(config Config) model.HTTPTransport {
 	if config.QUICDialer == nil {
 		config.QUICDialer = NewQUICDialer(config)
 	}
-
 	tInfo := allTransportsInfo[config.HTTP3Enabled]
 	txp := tInfo.Factory(httpTransportConfig{
 		Dialer: config.Dialer, QUICDialer: config.QUICDialer, TLSDialer: config.TLSDialer,
 		TLSConfig: config.TLSConfig})
-
 	if config.ByteCounter != nil {
 		txp = &bytecounter.HTTPTransport{
 			Counter: config.ByteCounter, HTTPTransport: txp}

internal/model/logger.go

@@ -69,3 +69,12 @@ func ErrorToStringOrOK(err error) string {
 	}
 	return "ok"
 }
+
+// ValidLoggerOrDefault is a factory that either returns the logger
+// provided as argument, if not nil, or DiscardLogger.
+func ValidLoggerOrDefault(logger Logger) Logger {
+	if logger != nil {
+		return logger
+	}
+	return DiscardLogger
+}

internal/model/logger_test.go

@@ -31,3 +31,19 @@ func TestErrorToStringOrOK(t *testing.T) {
 		}
 	})
 }
+
+func TestValidLoggerOrDefault(t *testing.T) {
+	t.Run("with nil argument", func(t *testing.T) {
+		out := ValidLoggerOrDefault(nil)
+		if out != DiscardLogger {
+			t.Fatal("unexpected result")
+		}
+	})
+
+	t.Run("with non-nil argument", func(t *testing.T) {
+		in := &logDiscarder{}
+		if ValidLoggerOrDefault(in) != in {
+			t.Fatal("unexpected result")
+		}
+	})
+}

internal/netxlite/legacy.go

@@ -8,30 +8,19 @@ package netxlite
 //
 // Deprecated: do not use these names in new code.
 var (
-	DefaultDialer        = &DialerSystem{}
+	DefaultDialer     = &DialerSystem{}
-	DefaultTLSHandshaker = defaultTLSHandshaker
+	NewResolverSystem = newResolverSystem
-	NewResolverSystem    = newResolverSystem
+	DefaultResolver   = newResolverSystem()
-	NewConnUTLS          = newConnUTLS
-	DefaultResolver      = newResolverSystem()
 )
 
 // These types export internal names to legacy ooni/probe-cli code.
 //
 // Deprecated: do not use these names in new code.
 type (
-	DialerResolver                 = dialerResolver
-	DialerLogger                   = dialerLogger
 	HTTPTransportWrapper           = httpTransportConnectionsCloser
 	HTTPTransportLogger            = httpTransportLogger
-	ErrorWrapperDialer             = dialerErrWrapper
-	ErrorWrapperQUICListener       = quicListenerErrWrapper
-	ErrorWrapperQUICDialer         = quicDialerErrWrapper
 	ErrorWrapperResolver           = resolverErrWrapper
 	ErrorWrapperTLSHandshaker      = tlsHandshakerErrWrapper
-	QUICListenerStdlib             = quicListenerStdlib
-	QUICDialerQUICGo               = quicDialerQUICGo
-	QUICDialerResolver             = quicDialerResolver
-	QUICDialerLogger               = quicDialerLogger
 	ResolverSystemDoNotInstantiate = resolverSystem // instantiate => crash w/ nil transport
 	ResolverLogger                 = resolverLogger
 	ResolverIDNA                   = resolverIDNA

internal/netxlite/tls.go

@@ -215,9 +215,6 @@ func (h *tlsHandshakerConfigurable) newConn(conn net.Conn, config *tls.Config) (
 	return tls.Client(conn, config), nil
 }
 
-// defaultTLSHandshaker is the default TLS handshaker.
-var defaultTLSHandshaker = &tlsHandshakerConfigurable{}
-
 // tlsHandshakerLogger is a TLSHandshaker with logging.
 type tlsHandshakerLogger struct {
 	TLSHandshaker model.TLSHandshaker