From ae24ba644c4fd71992c45c7ee5311eebdf25266e Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 2 Jun 2022 13:50:34 +0200 Subject: [PATCH] cleanup(netx): another batch of small/simple cleanups (#789) See https://github.com/ooni/probe/issues/2121 --- internal/engine/netx/dialer.go | 63 ----------------------------- internal/engine/netx/dialer_test.go | 48 ---------------------- internal/engine/netx/netx.go | 35 +++++++--------- internal/model/logger.go | 9 +++++ internal/model/logger_test.go | 16 ++++++++ internal/netxlite/legacy.go | 17 ++------ internal/netxlite/tls.go | 3 -- 7 files changed, 42 insertions(+), 149 deletions(-) delete mode 100644 internal/engine/netx/dialer.go delete mode 100644 internal/engine/netx/dialer_test.go diff --git a/internal/engine/netx/dialer.go b/internal/engine/netx/dialer.go deleted file mode 100644 index 25693a3..0000000 --- a/internal/engine/netx/dialer.go +++ /dev/null @@ -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 -} diff --git a/internal/engine/netx/dialer_test.go b/internal/engine/netx/dialer_test.go deleted file mode 100644 index 65830b3..0000000 --- a/internal/engine/netx/dialer_test.go +++ /dev/null @@ -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() -} diff --git a/internal/engine/netx/netx.go b/internal/engine/netx/netx.go index a5048b3..476c78b 100644 --- a/internal/engine/netx/netx.go +++ b/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{ - ContextByteCounting: config.ContextByteCounting, - DialSaver: config.DialSaver, - Logger: config.Logger, - ProxyURL: config.ProxyURL, - ReadWriteSaver: config.ReadWriteSaver, - }, config.FullResolver) + logger := model.ValidLoggerOrDefault(config.Logger) + d := netxlite.NewDialerWithResolver( + logger, config.FullResolver, config.DialSaver.NewConnectObserver(), + config.ReadWriteSaver.NewReadWriteObserver(), + ) + d = netxlite.NewMaybeProxyDialer(d, config.ProxyURL) + 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 - if config.Logger != nil { - logger = config.Logger - } + logger := model.ValidLoggerOrDefault(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} diff --git a/internal/model/logger.go b/internal/model/logger.go index 9ee383c..78fbc99 100644 --- a/internal/model/logger.go +++ b/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 +} diff --git a/internal/model/logger_test.go b/internal/model/logger_test.go index 343ddf1..56f06c5 100644 --- a/internal/model/logger_test.go +++ b/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") + } + }) +} diff --git a/internal/netxlite/legacy.go b/internal/netxlite/legacy.go index b30ad57..0316f79 100644 --- a/internal/netxlite/legacy.go +++ b/internal/netxlite/legacy.go @@ -8,30 +8,19 @@ package netxlite // // Deprecated: do not use these names in new code. var ( - DefaultDialer = &DialerSystem{} - DefaultTLSHandshaker = defaultTLSHandshaker - NewResolverSystem = newResolverSystem - NewConnUTLS = newConnUTLS - DefaultResolver = newResolverSystem() + DefaultDialer = &DialerSystem{} + 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 ( - 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 diff --git a/internal/netxlite/tls.go b/internal/netxlite/tls.go index 9a3dd97..3cecd4e 100644 --- a/internal/netxlite/tls.go +++ b/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