From 8a0beee808faf428459cd90dddd43877b7441d47 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 23 Jun 2021 15:53:12 +0200 Subject: [PATCH] refactor: start pivoting netx (#396) What do I mean by pivoting? Netx is currently organized by row: ``` | dialer | quicdialer | resolver | ... saving | | | | ... errorwrapping | | | | ... logging | | | | ... mocking/sys | | | | ... ``` Every row needs to implement saving, errorwrapping, logging, mocking (or adapting to the system or to some underlying library). This causes cross package dependencies and, in turn, complexity. For example, we need the `trace` package for supporting saving. And `dialer`, `quickdialer`, et al. need to depend on such a package. The same goes for errorwrapping. This arrangement further complicates testing. For example, I am currently working on https://github.com/ooni/probe/issues/1505 and I realize it need to repeat integration tests in multiple places. Let's say instead we pivot the above matrix as follows: ``` | saving | errorwrapping | logging | ... dialer | | | | ... quicdialer | | | | ... logging | | | | ... mocking/sys | | | | ... ... ``` In this way, now every row contains everything related to a specific action to perform. We can now share code without relying on extra support packages. What's more, we can write tests and, judding from the way in which things are made, it seems we only need integration testing in `errorwrapping` because it's where data quality matters whereas, in all other cases, unit testing is fine. I am going, therefore, to proceed with these changes and "pivot" `netx`. Hopefully, it won't be too painful. --- .../cmd/oohelperd/internal/internal_test.go | 4 +- internal/engine/experiment/ndt7/dial.go | 5 +- internal/engine/legacy/netx/resolver.go | 3 +- internal/engine/netx/dialer/dialer.go | 7 +- internal/engine/netx/dialer/dialer_test.go | 5 +- internal/engine/netx/dialer/dns.go | 71 ----------- internal/engine/netx/dialer/logging.go | 23 ---- internal/engine/netx/dialer/logging_test.go | 30 ----- internal/engine/netx/dialer/system.go | 12 -- internal/engine/netx/dialer/system_test.go | 28 ----- internal/engine/netx/netx.go | 7 +- internal/engine/netx/netx_test.go | 19 +-- internal/engine/netx/quicdialer/dns.go | 4 +- internal/engine/netx/resolver/chain_test.go | 3 +- .../engine/netx/resolver/integration_test.go | 7 +- internal/engine/netx/resolver/logging.go | 30 ----- internal/engine/netx/resolver/logging_test.go | 23 ---- internal/engine/netx/resolver/system.go | 29 ----- internal/engine/netx/resolver/system_test.go | 25 ---- internal/netxlite/dialer.go | 86 +++++++++++++ .../dns_test.go => netxlite/dialer_test.go} | 114 +++++++++--------- internal/netxlite/doc.go | 46 +++++++ internal/netxlite/legacy.go | 39 ++++++ internal/netxlite/legacy_test.go | 49 ++++++++ internal/netxlite/logger.go | 7 ++ internal/netxlite/resolver.go | 78 ++++++++++++ internal/netxlite/resolver_test.go | 56 +++++++++ 27 files changed, 457 insertions(+), 353 deletions(-) delete mode 100644 internal/engine/netx/dialer/dns.go delete mode 100644 internal/engine/netx/dialer/logging.go delete mode 100644 internal/engine/netx/dialer/logging_test.go delete mode 100644 internal/engine/netx/dialer/system.go delete mode 100644 internal/engine/netx/dialer/system_test.go delete mode 100644 internal/engine/netx/resolver/logging.go delete mode 100644 internal/engine/netx/resolver/logging_test.go delete mode 100644 internal/engine/netx/resolver/system.go delete mode 100644 internal/engine/netx/resolver/system_test.go create mode 100644 internal/netxlite/dialer.go rename internal/{engine/netx/dialer/dns_test.go => netxlite/dialer_test.go} (54%) create mode 100644 internal/netxlite/doc.go create mode 100644 internal/netxlite/legacy.go create mode 100644 internal/netxlite/legacy_test.go create mode 100644 internal/netxlite/logger.go create mode 100644 internal/netxlite/resolver.go create mode 100644 internal/netxlite/resolver_test.go diff --git a/internal/cmd/oohelperd/internal/internal_test.go b/internal/cmd/oohelperd/internal/internal_test.go index 34cdffd..04d71e8 100644 --- a/internal/cmd/oohelperd/internal/internal_test.go +++ b/internal/cmd/oohelperd/internal/internal_test.go @@ -11,8 +11,8 @@ import ( "testing" "github.com/ooni/probe-cli/v3/internal/cmd/oohelperd/internal" - "github.com/ooni/probe-cli/v3/internal/engine/netx/resolver" "github.com/ooni/probe-cli/v3/internal/iox" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) const simplerequest = `{ @@ -56,7 +56,7 @@ func TestWorkingAsIntended(t *testing.T) { Client: http.DefaultClient, Dialer: new(net.Dialer), MaxAcceptableBody: 1 << 24, - Resolver: resolver.SystemResolver{}, + Resolver: netxlite.ResolverSystem{}, } srv := httptest.NewServer(handler) defer srv.Close() diff --git a/internal/engine/experiment/ndt7/dial.go b/internal/engine/experiment/ndt7/dial.go index 31645d7..8a89b1a 100644 --- a/internal/engine/experiment/ndt7/dial.go +++ b/internal/engine/experiment/ndt7/dial.go @@ -10,6 +10,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine/model" "github.com/ooni/probe-cli/v3/internal/engine/netx/dialer" "github.com/ooni/probe-cli/v3/internal/engine/netx/resolver" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) type dialManager struct { @@ -33,8 +34,8 @@ func newDialManager(ndt7URL string, logger model.Logger, userAgent string) dialM } func (mgr dialManager) dialWithTestName(ctx context.Context, testName string) (*websocket.Conn, error) { - var reso resolver.Resolver = resolver.SystemResolver{} - reso = resolver.LoggingResolver{Resolver: reso, Logger: mgr.logger} + var reso resolver.Resolver = netxlite.ResolverSystem{} + reso = netxlite.ResolverLogger{Resolver: reso, Logger: mgr.logger} dlr := dialer.New(&dialer.Config{ ContextByteCounting: true, Logger: mgr.logger, diff --git a/internal/engine/legacy/netx/resolver.go b/internal/engine/legacy/netx/resolver.go index 0b685bf..7b5c574 100644 --- a/internal/engine/legacy/netx/resolver.go +++ b/internal/engine/legacy/netx/resolver.go @@ -12,6 +12,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine/legacy/netx/handlers" "github.com/ooni/probe-cli/v3/internal/engine/legacy/netx/modelx" "github.com/ooni/probe-cli/v3/internal/engine/netx/resolver" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) var ( @@ -158,7 +159,7 @@ func resolverWrapTransport(txp resolver.RoundTripper) resolver.EmitterResolver { } func newResolverSystem() resolver.EmitterResolver { - return resolverWrapResolver(resolver.SystemResolver{}) + return resolverWrapResolver(netxlite.ResolverSystem{}) } func newResolverUDP(dialer resolver.Dialer, address string) resolver.EmitterResolver { diff --git a/internal/engine/netx/dialer/dialer.go b/internal/engine/netx/dialer/dialer.go index 1bcd272..e436172 100644 --- a/internal/engine/netx/dialer/dialer.go +++ b/internal/engine/netx/dialer/dialer.go @@ -6,6 +6,7 @@ import ( "net/url" "github.com/ooni/probe-cli/v3/internal/engine/netx/trace" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) // Dialer establishes network connections. @@ -64,10 +65,10 @@ type Config struct { // New creates a new Dialer from the specified config and resolver. func New(config *Config, resolver Resolver) Dialer { - var d Dialer = systemDialer + var d Dialer = netxlite.DefaultDialer d = &errorWrapperDialer{Dialer: d} if config.Logger != nil { - d = &loggingDialer{Dialer: d, Logger: config.Logger} + d = &netxlite.DialerLogger{Dialer: d, Logger: config.Logger} } if config.DialSaver != nil { d = &saverDialer{Dialer: d, Saver: config.DialSaver} @@ -75,7 +76,7 @@ func New(config *Config, resolver Resolver) Dialer { if config.ReadWriteSaver != nil { d = &saverConnDialer{Dialer: d, Saver: config.ReadWriteSaver} } - d = &dnsDialer{Resolver: resolver, Dialer: d} + d = &netxlite.DialerResolver{Resolver: resolver, Dialer: d} d = &proxyDialer{ProxyURL: config.ProxyURL, Dialer: d} if config.ContextByteCounting { d = &byteCounterDialer{Dialer: d} diff --git a/internal/engine/netx/dialer/dialer_test.go b/internal/engine/netx/dialer/dialer_test.go index b861be0..57202d2 100644 --- a/internal/engine/netx/dialer/dialer_test.go +++ b/internal/engine/netx/dialer/dialer_test.go @@ -7,6 +7,7 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/engine/netx/trace" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) func TestNewCreatesTheExpectedChain(t *testing.T) { @@ -30,7 +31,7 @@ func TestNewCreatesTheExpectedChain(t *testing.T) { if !ok { t.Fatal("not a proxyDialer") } - dnsd, ok := pd.Dialer.(*dnsDialer) + dnsd, ok := pd.Dialer.(*netxlite.DialerResolver) if !ok { t.Fatal("not a dnsDialer") } @@ -42,7 +43,7 @@ func TestNewCreatesTheExpectedChain(t *testing.T) { if !ok { t.Fatal("not a saverDialer") } - ld, ok := sd.Dialer.(*loggingDialer) + ld, ok := sd.Dialer.(*netxlite.DialerLogger) if !ok { t.Fatal("not a loggingDialer") } diff --git a/internal/engine/netx/dialer/dns.go b/internal/engine/netx/dialer/dns.go deleted file mode 100644 index 0062954..0000000 --- a/internal/engine/netx/dialer/dns.go +++ /dev/null @@ -1,71 +0,0 @@ -package dialer - -import ( - "context" - "errors" - "net" - "strings" - - "github.com/ooni/probe-cli/v3/internal/engine/netx/errorx" -) - -// dnsDialer is a dialer that uses the configured Resolver to resolver a -// domain name to IP addresses, and the configured Dialer to connect. -type dnsDialer struct { - Dialer - Resolver Resolver -} - -// DialContext implements Dialer.DialContext. -func (d *dnsDialer) DialContext(ctx context.Context, network, address string) (net.Conn, error) { - onlyhost, onlyport, err := net.SplitHostPort(address) - if err != nil { - return nil, err - } - var addrs []string - addrs, err = d.lookupHost(ctx, onlyhost) - if err != nil { - return nil, err - } - var errorslist []error - for _, addr := range addrs { - target := net.JoinHostPort(addr, onlyport) - conn, err := d.Dialer.DialContext(ctx, network, target) - if err == nil { - return conn, nil - } - errorslist = append(errorslist, err) - } - return nil, ReduceErrors(errorslist) -} - -// ReduceErrors finds a known error in a list of errors since it's probably most relevant. -func ReduceErrors(errorslist []error) error { - if len(errorslist) == 0 { - return nil - } - // If we have a known error, let's consider this the real error - // since it's probably most relevant. Otherwise let's return the - // first considering that (1) local resolvers likely will give - // us IPv4 first and (2) also our resolver does that. So, in case - // the user has no IPv6 connectivity, an IPv6 error is going to - // appear later in the list of errors. - for _, err := range errorslist { - var wrapper *errorx.ErrWrapper - if errors.As(err, &wrapper) && !strings.HasPrefix( - err.Error(), "unknown_failure", - ) { - return err - } - } - // TODO(bassosimone): handle this case in a better way - return errorslist[0] -} - -// lookupHost performs a domain name resolution. -func (d *dnsDialer) lookupHost(ctx context.Context, hostname string) ([]string, error) { - if net.ParseIP(hostname) != nil { - return []string{hostname}, nil - } - return d.Resolver.LookupHost(ctx, hostname) -} diff --git a/internal/engine/netx/dialer/logging.go b/internal/engine/netx/dialer/logging.go deleted file mode 100644 index cb53b93..0000000 --- a/internal/engine/netx/dialer/logging.go +++ /dev/null @@ -1,23 +0,0 @@ -package dialer - -import ( - "context" - "net" - "time" -) - -// loggingDialer is a Dialer with logging -type loggingDialer struct { - Dialer - Logger Logger -} - -// DialContext implements Dialer.DialContext -func (d *loggingDialer) DialContext(ctx context.Context, network, address string) (net.Conn, error) { - d.Logger.Debugf("dial %s/%s...", address, network) - start := time.Now() - conn, err := d.Dialer.DialContext(ctx, network, address) - stop := time.Now() - d.Logger.Debugf("dial %s/%s... %+v in %s", address, network, err, stop.Sub(start)) - return conn, err -} diff --git a/internal/engine/netx/dialer/logging_test.go b/internal/engine/netx/dialer/logging_test.go deleted file mode 100644 index f1b88b3..0000000 --- a/internal/engine/netx/dialer/logging_test.go +++ /dev/null @@ -1,30 +0,0 @@ -package dialer - -import ( - "context" - "errors" - "io" - "net" - "testing" - - "github.com/apex/log" - "github.com/ooni/probe-cli/v3/internal/engine/netx/mockablex" -) - -func TestLoggingDialerFailure(t *testing.T) { - d := &loggingDialer{ - Dialer: mockablex.Dialer{ - MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) { - return nil, io.EOF - }, - }, - Logger: log.Log, - } - conn, err := d.DialContext(context.Background(), "tcp", "www.google.com:443") - if !errors.Is(err, io.EOF) { - t.Fatal("not the error we expected") - } - if conn != nil { - t.Fatal("expected nil conn here") - } -} diff --git a/internal/engine/netx/dialer/system.go b/internal/engine/netx/dialer/system.go deleted file mode 100644 index 37f2258..0000000 --- a/internal/engine/netx/dialer/system.go +++ /dev/null @@ -1,12 +0,0 @@ -package dialer - -import ( - "net" - "time" -) - -// systemDialer is the underlying net.Dialer. -var systemDialer = &net.Dialer{ - Timeout: 15 * time.Second, - KeepAlive: 15 * time.Second, -} diff --git a/internal/engine/netx/dialer/system_test.go b/internal/engine/netx/dialer/system_test.go deleted file mode 100644 index a21073a..0000000 --- a/internal/engine/netx/dialer/system_test.go +++ /dev/null @@ -1,28 +0,0 @@ -package dialer - -import ( - "strings" - "testing" - "time" - - "github.com/ooni/psiphon/oopsi/golang.org/x/net/context" -) - -func TestSystemDialerWorks(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - cancel() // fail immediately - conn, err := systemDialer.DialContext(ctx, "tcp", "8.8.8.8:853") - if err == nil || !strings.HasSuffix(err.Error(), "operation was canceled") { - t.Fatal("not the error we expected", err) - } - if conn != nil { - t.Fatal("expected nil conn here") - } -} - -func TestSystemDialerHasTimeout(t *testing.T) { - expected := 15 * time.Second - if systemDialer.Timeout != expected { - t.Fatal("unexpected timeout value") - } -} diff --git a/internal/engine/netx/netx.go b/internal/engine/netx/netx.go index 0a3bcb8..13a766e 100644 --- a/internal/engine/netx/netx.go +++ b/internal/engine/netx/netx.go @@ -39,6 +39,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine/netx/tlsdialer" "github.com/ooni/probe-cli/v3/internal/engine/netx/tlsx" "github.com/ooni/probe-cli/v3/internal/engine/netx/trace" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) // Logger is the logger assumed by this package @@ -114,7 +115,7 @@ var defaultCertPool *x509.CertPool = tlsx.NewDefaultCertPool() // NewResolver creates a new resolver from the specified config func NewResolver(config Config) Resolver { if config.BaseResolver == nil { - config.BaseResolver = resolver.SystemResolver{} + config.BaseResolver = netxlite.ResolverSystem{} } var r Resolver = config.BaseResolver r = resolver.AddressResolver{Resolver: r} @@ -133,7 +134,7 @@ func NewResolver(config Config) Resolver { } r = resolver.ErrorWrapperResolver{Resolver: r} if config.Logger != nil { - r = resolver.LoggingResolver{Logger: config.Logger, Resolver: r} + r = netxlite.ResolverLogger{Logger: config.Logger, Resolver: r} } if config.ResolveSaver != nil { r = resolver.SaverResolver{Resolver: r, Saver: config.ResolveSaver} @@ -317,7 +318,7 @@ func NewDNSClientWithOverrides(config Config, URL, hostOverride, SNIOverride, } switch resolverURL.Scheme { case "system": - c.Resolver = resolver.SystemResolver{} + c.Resolver = netxlite.ResolverSystem{} return c, nil case "https": config.TLSConfig.NextProtos = []string{"h2", "http/1.1"} diff --git a/internal/engine/netx/netx_test.go b/internal/engine/netx/netx_test.go index bf41ea9..dbca660 100644 --- a/internal/engine/netx/netx_test.go +++ b/internal/engine/netx/netx_test.go @@ -15,6 +15,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine/netx/tlsdialer" "github.com/ooni/probe-cli/v3/internal/engine/netx/tlsx" "github.com/ooni/probe-cli/v3/internal/engine/netx/trace" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) func TestNewResolverVanilla(t *testing.T) { @@ -31,7 +32,7 @@ func TestNewResolverVanilla(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - _, ok = ar.Resolver.(resolver.SystemResolver) + _, ok = ar.Resolver.(netxlite.ResolverSystem) if !ok { t.Fatal("not the resolver we expected") } @@ -81,7 +82,7 @@ func TestNewResolverWithBogonFilter(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - _, ok = ar.Resolver.(resolver.SystemResolver) + _, ok = ar.Resolver.(netxlite.ResolverSystem) if !ok { t.Fatal("not the resolver we expected") } @@ -95,7 +96,7 @@ func TestNewResolverWithLogging(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - lr, ok := ir.Resolver.(resolver.LoggingResolver) + lr, ok := ir.Resolver.(netxlite.ResolverLogger) if !ok { t.Fatal("not the resolver we expected") } @@ -110,7 +111,7 @@ func TestNewResolverWithLogging(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - _, ok = ar.Resolver.(resolver.SystemResolver) + _, ok = ar.Resolver.(netxlite.ResolverSystem) if !ok { t.Fatal("not the resolver we expected") } @@ -140,7 +141,7 @@ func TestNewResolverWithSaver(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - _, ok = ar.Resolver.(resolver.SystemResolver) + _, ok = ar.Resolver.(netxlite.ResolverSystem) if !ok { t.Fatal("not the resolver we expected") } @@ -169,7 +170,7 @@ func TestNewResolverWithReadWriteCache(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - _, ok = ar.Resolver.(resolver.SystemResolver) + _, ok = ar.Resolver.(netxlite.ResolverSystem) if !ok { t.Fatal("not the resolver we expected") } @@ -203,7 +204,7 @@ func TestNewResolverWithPrefilledReadonlyCache(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - _, ok = ar.Resolver.(resolver.SystemResolver) + _, ok = ar.Resolver.(netxlite.ResolverSystem) if !ok { t.Fatal("not the resolver we expected") } @@ -597,7 +598,7 @@ func TestNewDNSClientSystemResolver(t *testing.T) { if err != nil { t.Fatal(err) } - if _, ok := dnsclient.Resolver.(resolver.SystemResolver); !ok { + if _, ok := dnsclient.Resolver.(netxlite.ResolverSystem); !ok { t.Fatal("not the resolver we expected") } dnsclient.CloseIdleConnections() @@ -609,7 +610,7 @@ func TestNewDNSClientEmpty(t *testing.T) { if err != nil { t.Fatal(err) } - if _, ok := dnsclient.Resolver.(resolver.SystemResolver); !ok { + if _, ok := dnsclient.Resolver.(netxlite.ResolverSystem); !ok { t.Fatal("not the resolver we expected") } dnsclient.CloseIdleConnections() diff --git a/internal/engine/netx/quicdialer/dns.go b/internal/engine/netx/quicdialer/dns.go index 5f390e2..9d83be6 100644 --- a/internal/engine/netx/quicdialer/dns.go +++ b/internal/engine/netx/quicdialer/dns.go @@ -6,7 +6,7 @@ import ( "net" "github.com/lucas-clemente/quic-go" - "github.com/ooni/probe-cli/v3/internal/engine/netx/dialer" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) // DNSDialer is a dialer that uses the configured Resolver to resolve a @@ -45,7 +45,7 @@ func (d DNSDialer) DialContext( errorslist = append(errorslist, err) } // TODO(bassosimone): maybe ReduceErrors could be in netx/internal. - return nil, dialer.ReduceErrors(errorslist) + return nil, netxlite.ReduceErrors(errorslist) } // LookupHost implements Resolver.LookupHost diff --git a/internal/engine/netx/resolver/chain_test.go b/internal/engine/netx/resolver/chain_test.go index e6aa1ae..3a15045 100644 --- a/internal/engine/netx/resolver/chain_test.go +++ b/internal/engine/netx/resolver/chain_test.go @@ -5,12 +5,13 @@ import ( "testing" "github.com/ooni/probe-cli/v3/internal/engine/netx/resolver" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) func TestChainLookupHost(t *testing.T) { r := resolver.ChainResolver{ Primary: resolver.NewFakeResolverThatFails(), - Secondary: resolver.SystemResolver{}, + Secondary: netxlite.ResolverSystem{}, } if r.Address() != "" { t.Fatal("invalid address") diff --git a/internal/engine/netx/resolver/integration_test.go b/internal/engine/netx/resolver/integration_test.go index 28526dd..39cebb9 100644 --- a/internal/engine/netx/resolver/integration_test.go +++ b/internal/engine/netx/resolver/integration_test.go @@ -8,6 +8,7 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/engine/netx/resolver" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) func init() { @@ -18,7 +19,7 @@ func testresolverquick(t *testing.T, reso resolver.Resolver) { if testing.Short() { t.Skip("skip test in short mode") } - reso = resolver.LoggingResolver{Logger: log.Log, Resolver: reso} + reso = netxlite.ResolverLogger{Logger: log.Log, Resolver: reso} addrs, err := reso.LookupHost(context.Background(), "dns.google.com") if err != nil { t.Fatal(err) @@ -44,7 +45,7 @@ func testresolverquickidna(t *testing.T, reso resolver.Resolver) { t.Skip("skip test in short mode") } reso = resolver.IDNAResolver{ - resolver.LoggingResolver{Logger: log.Log, Resolver: reso}, + netxlite.ResolverLogger{Logger: log.Log, Resolver: reso}, } addrs, err := reso.LookupHost(context.Background(), "яндекс.рф") if err != nil { @@ -56,7 +57,7 @@ func testresolverquickidna(t *testing.T, reso resolver.Resolver) { } func TestNewResolverSystem(t *testing.T) { - reso := resolver.SystemResolver{} + reso := netxlite.ResolverSystem{} testresolverquick(t, reso) testresolverquickidna(t, reso) } diff --git a/internal/engine/netx/resolver/logging.go b/internal/engine/netx/resolver/logging.go deleted file mode 100644 index 40e17f2..0000000 --- a/internal/engine/netx/resolver/logging.go +++ /dev/null @@ -1,30 +0,0 @@ -package resolver - -import ( - "context" - "time" -) - -// Logger is the logger assumed by this package -type Logger interface { - Debugf(format string, v ...interface{}) - Debug(message string) -} - -// LoggingResolver is a resolver that emits events -type LoggingResolver struct { - Resolver - Logger Logger -} - -// LookupHost returns the IP addresses of a host -func (r LoggingResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) { - r.Logger.Debugf("resolve %s...", hostname) - start := time.Now() - addrs, err := r.Resolver.LookupHost(ctx, hostname) - stop := time.Now() - r.Logger.Debugf("resolve %s... (%+v, %+v) in %s", hostname, addrs, err, stop.Sub(start)) - return addrs, err -} - -var _ Resolver = LoggingResolver{} diff --git a/internal/engine/netx/resolver/logging_test.go b/internal/engine/netx/resolver/logging_test.go deleted file mode 100644 index 4cc18da..0000000 --- a/internal/engine/netx/resolver/logging_test.go +++ /dev/null @@ -1,23 +0,0 @@ -package resolver_test - -import ( - "context" - "testing" - - "github.com/apex/log" - "github.com/ooni/probe-cli/v3/internal/engine/netx/resolver" -) - -func TestLoggingResolver(t *testing.T) { - r := resolver.LoggingResolver{ - Logger: log.Log, - Resolver: resolver.NewFakeResolverThatFails(), - } - addrs, err := r.LookupHost(context.Background(), "www.google.com") - if err == nil { - t.Fatal("expected an error here") - } - if addrs != nil { - t.Fatal("expected nil addr here") - } -} diff --git a/internal/engine/netx/resolver/system.go b/internal/engine/netx/resolver/system.go deleted file mode 100644 index 6c0c049..0000000 --- a/internal/engine/netx/resolver/system.go +++ /dev/null @@ -1,29 +0,0 @@ -package resolver - -import ( - "context" - "net" -) - -// SystemResolver is the system resolver. -type SystemResolver struct{} - -// LookupHost implements Resolver.LookupHost. -func (r SystemResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) { - return net.DefaultResolver.LookupHost(ctx, hostname) -} - -// Network implements Resolver.Network. -func (r SystemResolver) Network() string { - return "system" -} - -// Address implements Resolver.Address. -func (r SystemResolver) Address() string { - return "" -} - -// Default is the resolver we use by default. -var Default = SystemResolver{} - -var _ Resolver = SystemResolver{} diff --git a/internal/engine/netx/resolver/system_test.go b/internal/engine/netx/resolver/system_test.go deleted file mode 100644 index a30fb34..0000000 --- a/internal/engine/netx/resolver/system_test.go +++ /dev/null @@ -1,25 +0,0 @@ -package resolver_test - -import ( - "context" - "testing" - - "github.com/ooni/probe-cli/v3/internal/engine/netx/resolver" -) - -func TestSystemResolverLookupHost(t *testing.T) { - r := resolver.SystemResolver{} - if r.Network() != "system" { - t.Fatal("invalid Network") - } - if r.Address() != "" { - t.Fatal("invalid Address") - } - addrs, err := r.LookupHost(context.Background(), "dns.google.com") - if err != nil { - t.Fatal(err) - } - if addrs == nil { - t.Fatal("expected non-nil result here") - } -} diff --git a/internal/netxlite/dialer.go b/internal/netxlite/dialer.go new file mode 100644 index 0000000..200d50d --- /dev/null +++ b/internal/netxlite/dialer.go @@ -0,0 +1,86 @@ +package netxlite + +import ( + "context" + "net" + "time" +) + +// Dialer establishes network connections. +type Dialer interface { + // DialContext behaves like net.Dialer.DialContext. + DialContext(ctx context.Context, network, address string) (net.Conn, error) +} + +// DefaultDialer is the Dialer we use by default. +var DefaultDialer = &net.Dialer{ + Timeout: 15 * time.Second, + KeepAlive: 15 * time.Second, +} + +var _ Dialer = DefaultDialer + +// DialerResolver is a dialer that uses the configured Resolver to resolver a +// domain name to IP addresses, and the configured Dialer to connect. +type DialerResolver struct { + // Dialer is the underlying Dialer. + Dialer Dialer + + // Resolver is the underlying Resolver. + Resolver Resolver +} + +var _ Dialer = &DialerResolver{} + +// DialContext implements Dialer.DialContext. +func (d *DialerResolver) DialContext(ctx context.Context, network, address string) (net.Conn, error) { + onlyhost, onlyport, err := net.SplitHostPort(address) + if err != nil { + return nil, err + } + var addrs []string + addrs, err = d.lookupHost(ctx, onlyhost) + if err != nil { + return nil, err + } + // TODO(bassosimone): here we should be using multierror rather + // than just calling ReduceErrors. We are not ready to do that + // yet, though. To do that, we need first to modify nettests so + // that we actually avoid dialing when measuring. + var errorslist []error + for _, addr := range addrs { + target := net.JoinHostPort(addr, onlyport) + conn, err := d.Dialer.DialContext(ctx, network, target) + if err == nil { + return conn, nil + } + errorslist = append(errorslist, err) + } + return nil, ReduceErrors(errorslist) +} + +// lookupHost performs a domain name resolution. +func (d *DialerResolver) lookupHost(ctx context.Context, hostname string) ([]string, error) { + if net.ParseIP(hostname) != nil { + return []string{hostname}, nil + } + return d.Resolver.LookupHost(ctx, hostname) +} + +// DialerLogger is a Dialer with logging +type DialerLogger struct { + Dialer + Logger Logger +} + +var _ Dialer = &DialerLogger{} + +// DialContext implements Dialer.DialContext +func (d *DialerLogger) DialContext(ctx context.Context, network, address string) (net.Conn, error) { + d.Logger.Debugf("dial %s/%s...", address, network) + start := time.Now() + conn, err := d.Dialer.DialContext(ctx, network, address) + stop := time.Now() + d.Logger.Debugf("dial %s/%s... %+v in %s", address, network, err, stop.Sub(start)) + return conn, err +} diff --git a/internal/engine/netx/dialer/dns_test.go b/internal/netxlite/dialer_test.go similarity index 54% rename from internal/engine/netx/dialer/dns_test.go rename to internal/netxlite/dialer_test.go index bdbdf68..e07a81d 100644 --- a/internal/engine/netx/dialer/dns_test.go +++ b/internal/netxlite/dialer_test.go @@ -1,18 +1,20 @@ -package dialer +package netxlite import ( "context" "errors" "io" "net" + "strings" "testing" + "time" - "github.com/ooni/probe-cli/v3/internal/engine/netx/errorx" + "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/engine/netx/mockablex" ) -func TestDNSDialerNoPort(t *testing.T) { - dialer := &dnsDialer{Dialer: new(net.Dialer), Resolver: new(net.Resolver)} +func TestDialerResolverNoPort(t *testing.T) { + dialer := &DialerResolver{Dialer: new(net.Dialer), Resolver: DefaultResolver} conn, err := dialer.DialContext(context.Background(), "tcp", "antani.ooni.nu") if err == nil { t.Fatal("expected an error here") @@ -22,8 +24,8 @@ func TestDNSDialerNoPort(t *testing.T) { } } -func TestDNSDialerLookupHostAddress(t *testing.T) { - dialer := &dnsDialer{Dialer: new(net.Dialer), Resolver: MockableResolver{ +func TestDialerResolverLookupHostAddress(t *testing.T) { + dialer := &DialerResolver{Dialer: new(net.Dialer), Resolver: MockableResolver{ Err: errors.New("mocked error"), }} addrs, err := dialer.lookupHost(context.Background(), "1.1.1.1") @@ -35,9 +37,9 @@ func TestDNSDialerLookupHostAddress(t *testing.T) { } } -func TestDNSDialerLookupHostFailure(t *testing.T) { +func TestDialerResolverLookupHostFailure(t *testing.T) { expected := errors.New("mocked error") - dialer := &dnsDialer{Dialer: new(net.Dialer), Resolver: MockableResolver{ + dialer := &DialerResolver{Dialer: new(net.Dialer), Resolver: MockableResolver{ Err: expected, }} conn, err := dialer.DialContext(context.Background(), "tcp", "dns.google.com:853") @@ -58,12 +60,20 @@ func (r MockableResolver) LookupHost(ctx context.Context, host string) ([]string return r.Addresses, r.Err } -func TestDNSDialerDialForSingleIPFails(t *testing.T) { - dialer := &dnsDialer{Dialer: mockablex.Dialer{ +func (r MockableResolver) Network() string { + return "mockable" +} + +func (r MockableResolver) Address() string { + return "" +} + +func TestDialerResolverDialForSingleIPFails(t *testing.T) { + dialer := &DialerResolver{Dialer: mockablex.Dialer{ MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) { return nil, io.EOF }, - }, Resolver: new(net.Resolver)} + }, Resolver: DefaultResolver} conn, err := dialer.DialContext(context.Background(), "tcp", "1.1.1.1:853") if !errors.Is(err, io.EOF) { t.Fatal("not the error we expected") @@ -73,8 +83,8 @@ func TestDNSDialerDialForSingleIPFails(t *testing.T) { } } -func TestDNSDialerDialForManyIPFails(t *testing.T) { - dialer := &dnsDialer{ +func TestDialerResolverDialForManyIPFails(t *testing.T) { + dialer := &DialerResolver{ Dialer: mockablex.Dialer{ MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) { return nil, io.EOF @@ -91,8 +101,8 @@ func TestDNSDialerDialForManyIPFails(t *testing.T) { } } -func TestDNSDialerDialForManyIPSuccess(t *testing.T) { - dialer := &dnsDialer{Dialer: mockablex.Dialer{ +func TestDialerResolverDialForManyIPSuccess(t *testing.T) { + dialer := &DialerResolver{Dialer: mockablex.Dialer{ MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) { return &mockablex.Conn{ MockClose: func() error { @@ -113,43 +123,39 @@ func TestDNSDialerDialForManyIPSuccess(t *testing.T) { conn.Close() } -func TestReduceErrors(t *testing.T) { - t.Run("no errors", func(t *testing.T) { - result := ReduceErrors(nil) - if result != nil { - t.Fatal("wrong result") - } - }) - - t.Run("single error", func(t *testing.T) { - err := errors.New("mocked error") - result := ReduceErrors([]error{err}) - if result != err { - t.Fatal("wrong result") - } - }) - - t.Run("multiple errors", func(t *testing.T) { - err1 := errors.New("mocked error #1") - err2 := errors.New("mocked error #2") - result := ReduceErrors([]error{err1, err2}) - if result.Error() != "mocked error #1" { - t.Fatal("wrong result") - } - }) - - t.Run("multiple errors with meaningful ones", func(t *testing.T) { - err1 := errors.New("mocked error #1") - err2 := &errorx.ErrWrapper{ - Failure: "unknown_failure: antani", - } - err3 := &errorx.ErrWrapper{ - Failure: errorx.FailureConnectionRefused, - } - err4 := errors.New("mocked error #3") - result := ReduceErrors([]error{err1, err2, err3, err4}) - if result.Error() != errorx.FailureConnectionRefused { - t.Fatal("wrong result") - } - }) +func TestDialerLoggerFailure(t *testing.T) { + d := &DialerLogger{ + Dialer: mockablex.Dialer{ + MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) { + return nil, io.EOF + }, + }, + Logger: log.Log, + } + conn, err := d.DialContext(context.Background(), "tcp", "www.google.com:443") + if !errors.Is(err, io.EOF) { + t.Fatal("not the error we expected") + } + if conn != nil { + t.Fatal("expected nil conn here") + } +} + +func TestDefaultDialerWorks(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // fail immediately + conn, err := DefaultDialer.DialContext(ctx, "tcp", "8.8.8.8:853") + if err == nil || !strings.HasSuffix(err.Error(), "operation was canceled") { + t.Fatal("not the error we expected", err) + } + if conn != nil { + t.Fatal("expected nil conn here") + } +} + +func TestDefaultDialerHasTimeout(t *testing.T) { + expected := 15 * time.Second + if DefaultDialer.Timeout != expected { + t.Fatal("unexpected timeout value") + } } diff --git a/internal/netxlite/doc.go b/internal/netxlite/doc.go new file mode 100644 index 0000000..59dafb3 --- /dev/null +++ b/internal/netxlite/doc.go @@ -0,0 +1,46 @@ +// Package netxlite contains network extensions. +// +// This package is the basic networking building block that you +// should be using every time you need networking. +// +// Naming and history +// +// Previous versions of this package were called netx. Compared to such +// versions this package is lightweight because it does not contain code +// to perform the measurements, hence its name. +// +// Design +// +// We want to potentially be able to observe each low-level operation +// separately, even though this is not done by this package. This is +// the use case where we are performing measurements. +// +// We also want to be able to use this package in a more casual way +// without having to compose each operation separately. This, instead, is +// the use case where we're communicating with the OONI backend. +// +// We want to optionally provide detailed logging of every operation, +// thus users can use `-v` to obtain OONI logs. +// +// We also want to mock any underlying dependency for testing. +// +// Operations +// +// This package implements the following operations: +// +// 1. establishing a TCP connection; +// +// 2. performing a domain name resolution; +// +// 3. performing the TLS handshake; +// +// 4. performing the QUIC handshake; +// +// 5. dialing with TCP, TLS, and QUIC (where in this context dialing +// means combining domain name resolution and "connecting"); +// +// 6. performing HTTP, HTTP2, and HTTP3 round trips. +// +// Operations 1, 2, 3, and 4 are used when we perform measurements, +// while 5 and 6 are mostly used when speaking with our backend. +package netxlite diff --git a/internal/netxlite/legacy.go b/internal/netxlite/legacy.go new file mode 100644 index 0000000..670320c --- /dev/null +++ b/internal/netxlite/legacy.go @@ -0,0 +1,39 @@ +package netxlite + +import ( + "errors" + "strings" + + "github.com/ooni/probe-cli/v3/internal/engine/netx/errorx" +) + +// ReduceErrors finds a known error in a list of errors since +// it's probably most relevant. +// +// Deprecation warning +// +// Albeit still used, this function is now DEPRECATED. +// +// In perspective, we would like to transition to a scenario where +// full dialing is NOT used for measurements and we return a multierror here. +func ReduceErrors(errorslist []error) error { + if len(errorslist) == 0 { + return nil + } + // If we have a known error, let's consider this the real error + // since it's probably most relevant. Otherwise let's return the + // first considering that (1) local resolvers likely will give + // us IPv4 first and (2) also our resolver does that. So, in case + // the user has no IPv6 connectivity, an IPv6 error is going to + // appear later in the list of errors. + for _, err := range errorslist { + var wrapper *errorx.ErrWrapper + if errors.As(err, &wrapper) && !strings.HasPrefix( + err.Error(), "unknown_failure", + ) { + return err + } + } + // TODO(bassosimone): handle this case in a better way + return errorslist[0] +} diff --git a/internal/netxlite/legacy_test.go b/internal/netxlite/legacy_test.go new file mode 100644 index 0000000..a6d7254 --- /dev/null +++ b/internal/netxlite/legacy_test.go @@ -0,0 +1,49 @@ +package netxlite + +import ( + "errors" + "testing" + + "github.com/ooni/probe-cli/v3/internal/engine/netx/errorx" +) + +func TestReduceErrors(t *testing.T) { + t.Run("no errors", func(t *testing.T) { + result := ReduceErrors(nil) + if result != nil { + t.Fatal("wrong result") + } + }) + + t.Run("single error", func(t *testing.T) { + err := errors.New("mocked error") + result := ReduceErrors([]error{err}) + if result != err { + t.Fatal("wrong result") + } + }) + + t.Run("multiple errors", func(t *testing.T) { + err1 := errors.New("mocked error #1") + err2 := errors.New("mocked error #2") + result := ReduceErrors([]error{err1, err2}) + if result.Error() != "mocked error #1" { + t.Fatal("wrong result") + } + }) + + t.Run("multiple errors with meaningful ones", func(t *testing.T) { + err1 := errors.New("mocked error #1") + err2 := &errorx.ErrWrapper{ + Failure: "unknown_failure: antani", + } + err3 := &errorx.ErrWrapper{ + Failure: errorx.FailureConnectionRefused, + } + err4 := errors.New("mocked error #3") + result := ReduceErrors([]error{err1, err2, err3, err4}) + if result.Error() != errorx.FailureConnectionRefused { + t.Fatal("wrong result") + } + }) +} diff --git a/internal/netxlite/logger.go b/internal/netxlite/logger.go new file mode 100644 index 0000000..089a36d --- /dev/null +++ b/internal/netxlite/logger.go @@ -0,0 +1,7 @@ +package netxlite + +// Logger is the interface we expect from a logger. +type Logger interface { + // Debugf formats and emits a debug message. + Debugf(format string, v ...interface{}) +} diff --git a/internal/netxlite/resolver.go b/internal/netxlite/resolver.go new file mode 100644 index 0000000..99d07e8 --- /dev/null +++ b/internal/netxlite/resolver.go @@ -0,0 +1,78 @@ +package netxlite + +import ( + "context" + "net" + "time" +) + +// Resolver performs domain name resolutions. +type Resolver interface { + // LookupHost behaves like net.Resolver.LookupHost. + LookupHost(ctx context.Context, hostname string) (addrs []string, err error) +} + +// ResolverSystem is the system resolver. +type ResolverSystem struct{} + +var _ Resolver = ResolverSystem{} + +// LookupHost implements Resolver.LookupHost. +func (r ResolverSystem) LookupHost(ctx context.Context, hostname string) ([]string, error) { + return net.DefaultResolver.LookupHost(ctx, hostname) +} + +// Network implements Resolver.Network. +func (r ResolverSystem) Network() string { + return "system" +} + +// Address implements Resolver.Address. +func (r ResolverSystem) Address() string { + return "" +} + +// DefaultResolver is the resolver we use by default. +var DefaultResolver = ResolverSystem{} + +// ResolverLogger is a resolver that emits events +type ResolverLogger struct { + Resolver + Logger Logger +} + +var _ Resolver = ResolverLogger{} + +// LookupHost returns the IP addresses of a host +func (r ResolverLogger) LookupHost(ctx context.Context, hostname string) ([]string, error) { + r.Logger.Debugf("resolve %s...", hostname) + start := time.Now() + addrs, err := r.Resolver.LookupHost(ctx, hostname) + stop := time.Now() + r.Logger.Debugf("resolve %s... (%+v, %+v) in %s", hostname, addrs, err, stop.Sub(start)) + return addrs, err +} + +type resolverNetworker interface { + Network() string +} + +// Network implements Resolver.Network. +func (r ResolverLogger) Network() string { + if rn, ok := r.Resolver.(resolverNetworker); ok { + return rn.Network() + } + return "logger" +} + +type resolverAddresser interface { + Address() string +} + +// Address implements Resolver.Address. +func (r ResolverLogger) Address() string { + if ra, ok := r.Resolver.(resolverAddresser); ok { + return ra.Address() + } + return "" +} diff --git a/internal/netxlite/resolver_test.go b/internal/netxlite/resolver_test.go new file mode 100644 index 0000000..7490589 --- /dev/null +++ b/internal/netxlite/resolver_test.go @@ -0,0 +1,56 @@ +package netxlite + +import ( + "context" + "net" + "testing" + + "github.com/apex/log" +) + +func TestResolverSystemLookupHost(t *testing.T) { + r := ResolverSystem{} + if r.Network() != "system" { + t.Fatal("invalid Network") + } + if r.Address() != "" { + t.Fatal("invalid Address") + } + addrs, err := r.LookupHost(context.Background(), "dns.google.com") + if err != nil { + t.Fatal(err) + } + if addrs == nil { + t.Fatal("expected non-nil result here") + } +} + +func TestResolverLoggerWithFailure(t *testing.T) { + r := ResolverLogger{ + Logger: log.Log, + Resolver: DefaultResolver, + } + if r.Network() != "system" { + t.Fatal("invalid Network") + } + if r.Address() != "" { + t.Fatal("invalid Address") + } + addrs, err := r.LookupHost(context.Background(), "nonexistent.antani") + if err == nil { + t.Fatal("expected an error here") + } + if addrs != nil { + t.Fatal("expected nil addr here") + } +} + +func TestResolverLoggerDefaultNetworkAddress(t *testing.T) { + r := &ResolverLogger{Logger: log.Log, Resolver: &net.Resolver{}} + if r.Network() != "logger" { + t.Fatal("invalid Network") + } + if r.Address() != "" { + t.Fatal("invalid Address") + } +}