From 3cd88debdc55ab32944000add4078583896f46b8 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 8 Sep 2021 21:19:51 +0200 Subject: [PATCH] netxlite: improve docs, tests, and code quality (#493) * netxlite: improve docs, tests, and code quality * better documentation * more strict testing of dialer (especially make sure we document the quirk in https://github.com/ooni/probe/issues/1779 and we have tests to guarantee we don't screw up here) * introduce NewErrWrapper factory for creating errors so we have confidence we are creating them correctly Part of https://github.com/ooni/probe/issues/1591 --- internal/netxlite/certifi_test.go | 10 +- internal/netxlite/dialer.go | 105 +++---- internal/netxlite/dialer_test.go | 280 ++++++++++++++++--- internal/netxlite/errorsx/errwrapper.go | 25 ++ internal/netxlite/errorsx/errwrapper_test.go | 62 ++++ internal/netxlite/mocks/logger.go | 18 ++ internal/netxlite/mocks/logger_test.go | 31 ++ internal/netxlite/quic.go | 28 +- internal/netxlite/quirks.go | 4 + internal/netxlite/quirks_test.go | 16 +- internal/netxlite/resolver.go | 7 +- internal/netxlite/tls.go | 7 +- 12 files changed, 452 insertions(+), 141 deletions(-) create mode 100644 internal/netxlite/mocks/logger.go create mode 100644 internal/netxlite/mocks/logger_test.go diff --git a/internal/netxlite/certifi_test.go b/internal/netxlite/certifi_test.go index 6851ca4..3432f0e 100644 --- a/internal/netxlite/certifi_test.go +++ b/internal/netxlite/certifi_test.go @@ -6,8 +6,10 @@ import ( ) func TestPEMCerts(t *testing.T) { - pool := x509.NewCertPool() - if !pool.AppendCertsFromPEM([]byte(pemcerts)) { - t.Fatal("cannot load pemcerts") - } + t.Run("we can successfully load the bundled certificates", func(t *testing.T) { + pool := x509.NewCertPool() + if !pool.AppendCertsFromPEM([]byte(pemcerts)) { + t.Fatal("cannot load pemcerts") + } + }) } diff --git a/internal/netxlite/dialer.go b/internal/netxlite/dialer.go index d402f27..b78894e 100644 --- a/internal/netxlite/dialer.go +++ b/internal/netxlite/dialer.go @@ -22,13 +22,25 @@ type Dialer interface { // NewDialerWithResolver creates a new Dialer. The returned Dialer // has the following properties: // -// 1. logs events using the given logger +// 1. logs events using the given logger; // -// 2. resolves domain names using the givern resolver +// 2. resolves domain names using the givern resolver; // -// 3. wraps errors +// 3. when using a resolver, each available enpoint is tried +// sequentially. On error, the code will return what it believes +// to be the most representative error in the pack. Most often, +// such an error is the first one that occurred. Choosing the +// error to return using this logic is a QUIRK that we owe +// to the original implementation of netx. We cannot change +// this behavior until all the legacy code that relies on +// it has been migrated to more sane patterns. // -// 4. has a configured connect timeout +// Removing this quirk from the codebase is documented as +// TODO(https://github.com/ooni/probe/issues/1779). +// +// 4. wraps errors; +// +// 5. has a configured connect timeout. func NewDialerWithResolver(logger Logger, resolver Resolver) Dialer { return &dialerLogger{ Dialer: &dialerResolver{ @@ -51,45 +63,46 @@ func NewDialerWithoutResolver(logger Logger) Dialer { return NewDialerWithResolver(logger, &nullResolver{}) } -// dialerSystem dials using Go stdlib. +// dialerSystem uses system facilities to perform domain name +// resolution and guarantees we have a dialer timeout. type dialerSystem struct { // timeout is the OPTIONAL timeout used for testing. timeout time.Duration } -// newUnderlyingDialer creates a new underlying dialer. +var _ Dialer = &dialerSystem{} + +const dialerDefaultTimeout = 15 * time.Second + func (d *dialerSystem) newUnderlyingDialer() *net.Dialer { t := d.timeout if t <= 0 { - t = 15 * time.Second + t = dialerDefaultTimeout } return &net.Dialer{Timeout: t} } -// DialContext implements Dialer.DialContext. func (d *dialerSystem) DialContext(ctx context.Context, network, address string) (net.Conn, error) { return d.newUnderlyingDialer().DialContext(ctx, network, address) } -// CloseIdleConnections implements Dialer.CloseIdleConnections. func (d *dialerSystem) CloseIdleConnections() { - // nothing + // nothing to do here } -// dialerResolver is a dialer that uses the configured Resolver to resolver a -// domain name to IP addresses, and the configured Dialer to connect. +// dialerResolver combines dialing with domain name resolution. type dialerResolver struct { - // Dialer is the underlying Dialer. - Dialer Dialer - - // Resolver is the underlying Resolver. - Resolver Resolver + Dialer + Resolver } var _ Dialer = &dialerResolver{} -// DialContext implements Dialer.DialContext. func (d *dialerResolver) DialContext(ctx context.Context, network, address string) (net.Conn, error) { + // QUIRK: this routine and the related routines in quirks.go cannot + // be changed easily until we use events tracing to measure. + // + // Reference issue: TODO(https://github.com/ooni/probe/issues/1779). onlyhost, onlyport, err := net.SplitHostPort(address) if err != nil { return nil, err @@ -98,12 +111,6 @@ func (d *dialerResolver) DialContext(ctx context.Context, network, address strin 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. - // - // See also the quirks.go file. This is clearly a QUIRK. addrs = quirkSortIPAddrs(addrs) var errorslist []error for _, addr := range addrs { @@ -117,7 +124,7 @@ func (d *dialerResolver) DialContext(ctx context.Context, network, address strin return nil, quirkReduceErrors(errorslist) } -// lookupHost performs a domain name resolution. +// lookupHost ensures we correctly handle IP addresses. func (d *dialerResolver) lookupHost(ctx context.Context, hostname string) ([]string, error) { if net.ParseIP(hostname) != nil { return []string{hostname}, nil @@ -125,7 +132,6 @@ func (d *dialerResolver) lookupHost(ctx context.Context, hostname string) ([]str return d.Resolver.LookupHost(ctx, hostname) } -// CloseIdleConnections implements Dialer.CloseIdleConnections. func (d *dialerResolver) CloseIdleConnections() { d.Dialer.CloseIdleConnections() d.Resolver.CloseIdleConnections() @@ -134,10 +140,10 @@ func (d *dialerResolver) CloseIdleConnections() { // dialerLogger is a Dialer with logging. type dialerLogger struct { // Dialer is the underlying dialer. - Dialer Dialer + Dialer // Logger is the underlying logger. - Logger Logger + Logger // operationSuffix is appended to the operation name. // @@ -150,7 +156,6 @@ type dialerLogger struct { 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/%s...", d.operationSuffix, address, network) start := time.Now() @@ -166,7 +171,6 @@ func (d *dialerLogger) DialContext(ctx context.Context, network, address string) return conn, nil } -// CloseIdleConnections implements Dialer.CloseIdleConnections. func (d *dialerLogger) CloseIdleConnections() { d.Dialer.CloseIdleConnections() } @@ -189,7 +193,6 @@ type dialerSingleUse struct { var _ Dialer = &dialerSingleUse{} -// DialContext implements Dialer.DialContext. func (s *dialerSingleUse) DialContext(ctx context.Context, network string, addr string) (net.Conn, error) { defer s.Unlock() s.Lock() @@ -201,79 +204,57 @@ func (s *dialerSingleUse) DialContext(ctx context.Context, network string, addr return conn, nil } -// CloseIdleConnections closes idle connections. func (s *dialerSingleUse) CloseIdleConnections() { - // nothing + // nothing to do } -// TODO(bassosimone): introduce factory for creating errors and -// write tests that ensure the factory works correctly. - // dialerErrWrapper is a dialer that performs error wrapping. The connection // returned by the DialContext function will also perform error wrapping. type dialerErrWrapper struct { - // Dialer is the underlying dialer. Dialer } var _ Dialer = &dialerErrWrapper{} -// DialContext implements Dialer.DialContext. func (d *dialerErrWrapper) DialContext(ctx context.Context, network, address string) (net.Conn, error) { conn, err := d.Dialer.DialContext(ctx, network, address) if err != nil { - return nil, &errorsx.ErrWrapper{ - Failure: errorsx.ClassifyGenericError(err), - Operation: errorsx.ConnectOperation, - WrappedErr: err, - } + return nil, errorsx.NewErrWrapper( + errorsx.ClassifyGenericError, errorsx.ConnectOperation, err) } return &dialerErrWrapperConn{Conn: conn}, nil } // dialerErrWrapperConn is a net.Conn that performs error wrapping. type dialerErrWrapperConn struct { - // Conn is the underlying connection. net.Conn } var _ net.Conn = &dialerErrWrapperConn{} -// Read implements net.Conn.Read. func (c *dialerErrWrapperConn) Read(b []byte) (int, error) { count, err := c.Conn.Read(b) if err != nil { - return 0, &errorsx.ErrWrapper{ - Failure: errorsx.ClassifyGenericError(err), - Operation: errorsx.ReadOperation, - WrappedErr: err, - } + return 0, errorsx.NewErrWrapper( + errorsx.ClassifyGenericError, errorsx.ReadOperation, err) } return count, nil } -// Write implements net.Conn.Write. func (c *dialerErrWrapperConn) Write(b []byte) (int, error) { count, err := c.Conn.Write(b) if err != nil { - return 0, &errorsx.ErrWrapper{ - Failure: errorsx.ClassifyGenericError(err), - Operation: errorsx.WriteOperation, - WrappedErr: err, - } + return 0, errorsx.NewErrWrapper( + errorsx.ClassifyGenericError, errorsx.WriteOperation, err) } return count, nil } -// Close implements net.Conn.Close. func (c *dialerErrWrapperConn) Close() error { err := c.Conn.Close() if err != nil { - return &errorsx.ErrWrapper{ - Failure: errorsx.ClassifyGenericError(err), - Operation: errorsx.CloseOperation, - WrappedErr: err, - } + return errorsx.NewErrWrapper( + errorsx.ClassifyGenericError, errorsx.CloseOperation, err) } return nil } diff --git a/internal/netxlite/dialer_test.go b/internal/netxlite/dialer_test.go index 93ccfa4..d340b4a 100644 --- a/internal/netxlite/dialer_test.go +++ b/internal/netxlite/dialer_test.go @@ -6,6 +6,7 @@ import ( "io" "net" "strings" + "sync" "testing" "time" @@ -16,8 +17,8 @@ import ( func TestNewDialer(t *testing.T) { t.Run("produces a chain with the expected types", func(t *testing.T) { - dlr := NewDialerWithoutResolver(log.Log) - logger := dlr.(*dialerLogger) + d := NewDialerWithoutResolver(log.Log) + logger := d.(*dialerLogger) if logger.Logger != log.Log { t.Fatal("invalid logger") } @@ -35,54 +36,76 @@ func TestNewDialer(t *testing.T) { } func TestDialerSystem(t *testing.T) { - t.Run("has a default timeout of 15 seconds", func(t *testing.T) { + t.Run("has a default timeout", func(t *testing.T) { d := &dialerSystem{} ud := d.newUnderlyingDialer() - if ud.Timeout != 15*time.Second { - t.Fatal("invalid default timeout") + if ud.Timeout != dialerDefaultTimeout { + t.Fatal("unexpected default timeout") } }) - t.Run("we can change the default timeout for testing", func(t *testing.T) { - d := &dialerSystem{timeout: 1 * time.Second} + t.Run("we can change the timeout for testing", func(t *testing.T) { + const smaller = 1 * time.Second + d := &dialerSystem{timeout: smaller} ud := d.newUnderlyingDialer() - if ud.Timeout != 1*time.Second { - t.Fatal("invalid default timeout") + if ud.Timeout != smaller { + t.Fatal("unexpected timeout") } }) t.Run("CloseIdleConnections", func(t *testing.T) { d := &dialerSystem{} - d.CloseIdleConnections() // should not crash + d.CloseIdleConnections() // to avoid missing coverage }) - t.Run("DialContext with canceled context", func(t *testing.T) { - d := &dialerSystem{} - ctx, cancel := context.WithCancel(context.Background()) - cancel() // immediately! - conn, err := d.DialContext(ctx, "tcp", "dns.google:443") - if err == nil || err.Error() != "dial tcp: operation was canceled" { - t.Fatal("unexpected err", err) - } - if conn != nil { - t.Fatal("unexpected conn") - } + t.Run("DialContext", func(t *testing.T) { + t.Run("with canceled context", func(t *testing.T) { + d := &dialerSystem{} + ctx, cancel := context.WithCancel(context.Background()) + cancel() // immediately! + conn, err := d.DialContext(ctx, "tcp", "dns.google:443") + if err == nil || err.Error() != "dial tcp: operation was canceled" { + t.Fatal("unexpected err", err) + } + if conn != nil { + t.Fatal("unexpected conn") + } + }) + + t.Run("enforces the configured timeout", func(t *testing.T) { + const timeout = 1 * time.Millisecond + d := &dialerSystem{timeout: timeout} + ctx := context.Background() + start := time.Now() + conn, err := d.DialContext(ctx, "tcp", "dns.google:443") + stop := time.Now() + if err == nil || !strings.HasSuffix(err.Error(), "i/o timeout") { + t.Fatal(err) + } + if conn != nil { + t.Fatal("unexpected conn") + } + if stop.Sub(start) > 100*time.Millisecond { + t.Fatal("undable to enforce timeout") + } + }) }) } func TestDialerResolver(t *testing.T) { t.Run("DialContext", func(t *testing.T) { - t.Run("without a port", func(t *testing.T) { + t.Run("fails without a port", func(t *testing.T) { d := &dialerResolver{ Dialer: &dialerSystem{}, Resolver: &resolverSystem{}, } - conn, err := d.DialContext(context.Background(), "tcp", "ooni.nu") + const missingPort = "ooni.nu" + conn, err := d.DialContext(context.Background(), "tcp", missingPort) if err == nil || !strings.HasSuffix(err.Error(), "missing port in address") { - t.Fatal("not the error we expected", err) + t.Fatal("unexpected err", err) } if conn != nil { - t.Fatal("expected a nil conn here") + t.Fatal("unexpected conn") } }) @@ -111,9 +134,13 @@ func TestDialerResolver(t *testing.T) { return nil, io.EOF }, }, - Resolver: &nullResolver{}, + Resolver: &mocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + return []string{"1.1.1.1", "8.8.8.8"}, nil + }, + }, } - conn, err := d.DialContext(context.Background(), "tcp", "1.1.1.1:853") + conn, err := d.DialContext(context.Background(), "tcp", "dot.dns:853") if !errors.Is(err, io.EOF) { t.Fatal("not the error we expected") } @@ -123,16 +150,18 @@ func TestDialerResolver(t *testing.T) { }) t.Run("handles dialing success correctly for many IP addresses", func(t *testing.T) { + expectedConn := &mocks.Conn{ + MockClose: func() error { + return nil + }, + } d := &dialerResolver{ Dialer: &mocks.Dialer{ MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) { - return &mocks.Conn{ - MockClose: func() error { - return nil - }, - }, nil + return expectedConn, nil }, - }, Resolver: &mocks.Resolver{ + }, + Resolver: &mocks.Resolver{ MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { return []string{"1.1.1.1", "8.8.8.8"}, nil }, @@ -142,11 +171,166 @@ func TestDialerResolver(t *testing.T) { if err != nil { t.Fatal(err) } - if conn == nil { - t.Fatal("expected non-nil conn") + if conn != expectedConn { + t.Fatal("unexpected conn") } conn.Close() }) + + t.Run("calls the underlying dialer sequentially", func(t *testing.T) { + // This test is fundamental to the following + // TODO(https://github.com/ooni/probe/issues/1779) + mu := &sync.Mutex{} + d := &dialerResolver{ + Dialer: &mocks.Dialer{ + MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) { + // It should not happen to have parallel dials with + // this implementation. When we have parallelism greater + // than one, this code will lock forever and we'll see + // a failed test and see we broke the QUIRK. + defer mu.Unlock() + mu.Lock() + return nil, io.EOF + }, + }, + Resolver: &mocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + return []string{"1.1.1.1", "8.8.8.8"}, nil + }, + }, + } + conn, err := d.DialContext(context.Background(), "tcp", "dot.dns:853") + if !errors.Is(err, io.EOF) { + t.Fatal("not the error we expected") + } + if conn != nil { + t.Fatal("expected nil conn") + } + }) + + t.Run("attempts with IPv4 addresses before IPv6 addresses", func(t *testing.T) { + // This test is fundamental to the following + // TODO(https://github.com/ooni/probe/issues/1779) + mu := &sync.Mutex{} + var attempts []string + d := &dialerResolver{ + Dialer: &mocks.Dialer{ + MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) { + // It should not happen to have parallel dials with + // this implementation. When we have parallelism greater + // than one, this code will lock forever and we'll see + // a failed test and see we broke the QUIRK. + defer mu.Unlock() + attempts = append(attempts, address) + mu.Lock() + return nil, io.EOF + }, + }, + Resolver: &mocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + return []string{"2001:4860:4860::8888", "8.8.8.8"}, nil + }, + }, + } + conn, err := d.DialContext(context.Background(), "tcp", "dot.dns:853") + if !errors.Is(err, io.EOF) { + t.Fatal("not the error we expected") + } + if conn != nil { + t.Fatal("expected nil conn") + } + mu.Lock() + asExpected := (attempts[0] == "8.8.8.8:853" && + attempts[1] == "[2001:4860:4860::8888]:853") + mu.Unlock() + if !asExpected { + t.Fatal("addresses not reordered") + } + }) + + t.Run("returns the first meaningful error if there is one", func(t *testing.T) { + // This test is fundamental to the following + // TODO(https://github.com/ooni/probe/issues/1779) + mu := &sync.Mutex{} + errorsList := []error{ + errors.New("a mocked error"), + errorsx.NewErrWrapper( + errorsx.ClassifyGenericError, + errorsx.CloseOperation, + io.EOF, + ), + } + var errorIdx int + d := &dialerResolver{ + Dialer: &mocks.Dialer{ + MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) { + // It should not happen to have parallel dials with + // this implementation. When we have parallelism greater + // than one, this code will lock forever and we'll see + // a failed test and see we broke the QUIRK. + defer mu.Unlock() + err := errorsList[errorIdx] + errorIdx++ + mu.Lock() + return nil, err + }, + }, + Resolver: &mocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + return []string{"2001:4860:4860::8888", "8.8.8.8"}, nil + }, + }, + } + conn, err := d.DialContext(context.Background(), "tcp", "dot.dns:853") + if err == nil || err.Error() != errorsx.FailureEOFError { + t.Fatal("unexpected err", err) + } + if conn != nil { + t.Fatal("expected nil conn") + } + }) + + t.Run("though ignores the unknown failures", func(t *testing.T) { + // This test is fundamental to the following + // TODO(https://github.com/ooni/probe/issues/1779) + mu := &sync.Mutex{} + errorsList := []error{ + errors.New("a mocked error"), + errorsx.NewErrWrapper( + errorsx.ClassifyGenericError, + errorsx.CloseOperation, + errors.New("antani"), + ), + } + var errorIdx int + d := &dialerResolver{ + Dialer: &mocks.Dialer{ + MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) { + // It should not happen to have parallel dials with + // this implementation. When we have parallelism greater + // than one, this code will lock forever and we'll see + // a failed test and see we broke the QUIRK. + defer mu.Unlock() + err := errorsList[errorIdx] + errorIdx++ + mu.Lock() + return nil, err + }, + }, + Resolver: &mocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + return []string{"2001:4860:4860::8888", "8.8.8.8"}, nil + }, + }, + } + conn, err := d.DialContext(context.Background(), "tcp", "dot.dns:853") + if err == nil || err.Error() != "a mocked error" { + t.Fatal("unexpected err", err) + } + if conn != nil { + t.Fatal("expected nil conn") + } + }) }) t.Run("lookupHost", func(t *testing.T) { @@ -207,6 +391,12 @@ func TestDialerResolver(t *testing.T) { func TestDialerLogger(t *testing.T) { t.Run("DialContext", func(t *testing.T) { t.Run("handles success correctly", func(t *testing.T) { + var count int + lo := &mocks.Logger{ + MockDebugf: func(format string, v ...interface{}) { + count++ + }, + } d := &dialerLogger{ Dialer: &mocks.Dialer{ MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) { @@ -217,7 +407,7 @@ func TestDialerLogger(t *testing.T) { }, nil }, }, - Logger: log.Log, + Logger: lo, } conn, err := d.DialContext(context.Background(), "tcp", "www.google.com:443") if err != nil { @@ -227,16 +417,25 @@ func TestDialerLogger(t *testing.T) { t.Fatal("expected non-nil conn here") } conn.Close() + if count != 2 { + t.Fatal("not enough log calls") + } }) t.Run("handles failure correctly", func(t *testing.T) { + var count int + lo := &mocks.Logger{ + MockDebugf: func(format string, v ...interface{}) { + count++ + }, + } d := &dialerLogger{ Dialer: &mocks.Dialer{ MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) { return nil, io.EOF }, }, - Logger: log.Log, + Logger: lo, } conn, err := d.DialContext(context.Background(), "tcp", "www.google.com:443") if !errors.Is(err, io.EOF) { @@ -245,6 +444,9 @@ func TestDialerLogger(t *testing.T) { if conn != nil { t.Fatal("expected nil conn here") } + if count != 2 { + t.Fatal("not enough log calls") + } }) }) @@ -290,7 +492,7 @@ func TestDialerSingleUse(t *testing.T) { t.Run("CloseIdleConnections", func(t *testing.T) { d := &dialerSingleUse{} - d.CloseIdleConnections() // does not crash + d.CloseIdleConnections() // to have the coverage }) } @@ -472,5 +674,5 @@ func TestNewNullDialer(t *testing.T) { if conn != nil { t.Fatal("expected nil conn") } - dialer.CloseIdleConnections() // does not crash + dialer.CloseIdleConnections() // to have coverage } diff --git a/internal/netxlite/errorsx/errwrapper.go b/internal/netxlite/errorsx/errwrapper.go index cd5d31c..81c3ac2 100644 --- a/internal/netxlite/errorsx/errwrapper.go +++ b/internal/netxlite/errorsx/errwrapper.go @@ -67,3 +67,28 @@ func (e *ErrWrapper) Unwrap() error { func (e *ErrWrapper) MarshalJSON() ([]byte, error) { return json.Marshal(e.Failure) } + +// Classifier is the type of function that performs classification. +type Classifier func(err error) string + +// NewErrWrapper creates a new ErrWrapper using the given +// classifier, operation name, and underlying error. +// +// This function panics if classifier is nil, or operation +// is the empty string or error is nil. +func NewErrWrapper(c Classifier, op string, err error) *ErrWrapper { + if c == nil { + panic("nil classifier") + } + if op == "" { + panic("empty op") + } + if err == nil { + panic("nil err") + } + return &ErrWrapper{ + Failure: c(err), + Operation: op, + WrappedErr: err, + } +} diff --git a/internal/netxlite/errorsx/errwrapper_test.go b/internal/netxlite/errorsx/errwrapper_test.go index b2d41e0..d7986b8 100644 --- a/internal/netxlite/errorsx/errwrapper_test.go +++ b/internal/netxlite/errorsx/errwrapper_test.go @@ -5,6 +5,8 @@ import ( "errors" "io" "testing" + + "github.com/ooni/probe-cli/v3/internal/atomicx" ) func TestErrWrapper(t *testing.T) { @@ -40,3 +42,63 @@ func TestErrWrapper(t *testing.T) { } }) } + +func TestNewErrWrapper(t *testing.T) { + t.Run("panics if the classifier is nil", func(t *testing.T) { + recovered := &atomicx.Int64{} + func() { + defer func() { + if recover() != nil { + recovered.Add(1) + } + }() + NewErrWrapper(nil, CloseOperation, io.EOF) + }() + if recovered.Load() != 1 { + t.Fatal("did not panic") + } + }) + + t.Run("panics if the operation is empty", func(t *testing.T) { + recovered := &atomicx.Int64{} + func() { + defer func() { + if recover() != nil { + recovered.Add(1) + } + }() + NewErrWrapper(ClassifyGenericError, "", io.EOF) + }() + if recovered.Load() != 1 { + t.Fatal("did not panic") + } + }) + + t.Run("panics if the error is nil", func(t *testing.T) { + recovered := &atomicx.Int64{} + func() { + defer func() { + if recover() != nil { + recovered.Add(1) + } + }() + NewErrWrapper(ClassifyGenericError, CloseOperation, nil) + }() + if recovered.Load() != 1 { + t.Fatal("did not panic") + } + }) + + t.Run("otherwise, works as intended", func(t *testing.T) { + ew := NewErrWrapper(ClassifyGenericError, CloseOperation, io.EOF) + if ew.Failure != FailureEOFError { + t.Fatal("unexpected failure") + } + if ew.Operation != CloseOperation { + t.Fatal("unexpected operation") + } + if ew.WrappedErr != io.EOF { + t.Fatal("unexpected WrappedErr") + } + }) +} diff --git a/internal/netxlite/mocks/logger.go b/internal/netxlite/mocks/logger.go new file mode 100644 index 0000000..d6696af --- /dev/null +++ b/internal/netxlite/mocks/logger.go @@ -0,0 +1,18 @@ +package mocks + +// Logger allows mocking a logger. +type Logger struct { + MockDebug func(message string) + + MockDebugf func(format string, v ...interface{}) +} + +// Debug calls MockDebug. +func (lo *Logger) Debug(message string) { + lo.MockDebug(message) +} + +// Debugf calls MockDebugf. +func (lo *Logger) Debugf(format string, v ...interface{}) { + lo.MockDebugf(format, v...) +} diff --git a/internal/netxlite/mocks/logger_test.go b/internal/netxlite/mocks/logger_test.go new file mode 100644 index 0000000..6572b10 --- /dev/null +++ b/internal/netxlite/mocks/logger_test.go @@ -0,0 +1,31 @@ +package mocks + +import "testing" + +func TestLogger(t *testing.T) { + t.Run("Debug", func(t *testing.T) { + var called bool + lo := &Logger{ + MockDebug: func(message string) { + called = true + }, + } + lo.Debug("antani") + if !called { + t.Fatal("not called") + } + }) + + t.Run("Debugf", func(t *testing.T) { + var called bool + lo := &Logger{ + MockDebugf: func(message string, v ...interface{}) { + called = true + }, + } + lo.Debugf("antani", 1, 2, 3, 4) + if !called { + t.Fatal("not called") + } + }) +} diff --git a/internal/netxlite/quic.go b/internal/netxlite/quic.go index 856130c..5fe5e28 100644 --- a/internal/netxlite/quic.go +++ b/internal/netxlite/quic.go @@ -337,11 +337,8 @@ var _ QUICListener = &quicListenerErrWrapper{} func (qls *quicListenerErrWrapper) Listen(addr *net.UDPAddr) (quicx.UDPLikeConn, error) { pconn, err := qls.QUICListener.Listen(addr) if err != nil { - return nil, &errorsx.ErrWrapper{ - Failure: errorsx.ClassifyGenericError(err), - Operation: errorsx.QUICListenOperation, - WrappedErr: err, - } + return nil, errorsx.NewErrWrapper( + errorsx.ClassifyGenericError, errorsx.QUICListenOperation, err) } return &quicErrWrapperUDPLikeConn{pconn}, nil } @@ -358,11 +355,8 @@ var _ quicx.UDPLikeConn = &quicErrWrapperUDPLikeConn{} func (c *quicErrWrapperUDPLikeConn) WriteTo(p []byte, addr net.Addr) (int, error) { count, err := c.UDPLikeConn.WriteTo(p, addr) if err != nil { - return 0, &errorsx.ErrWrapper{ - Failure: errorsx.ClassifyGenericError(err), - Operation: errorsx.WriteToOperation, - WrappedErr: err, - } + return 0, errorsx.NewErrWrapper( + errorsx.ClassifyGenericError, errorsx.WriteToOperation, err) } return count, nil } @@ -371,11 +365,8 @@ func (c *quicErrWrapperUDPLikeConn) WriteTo(p []byte, addr net.Addr) (int, error func (c *quicErrWrapperUDPLikeConn) ReadFrom(b []byte) (int, net.Addr, error) { n, addr, err := c.UDPLikeConn.ReadFrom(b) if err != nil { - return 0, nil, &errorsx.ErrWrapper{ - Failure: errorsx.ClassifyGenericError(err), - Operation: errorsx.ReadFromOperation, - WrappedErr: err, - } + return 0, nil, errorsx.NewErrWrapper( + errorsx.ClassifyGenericError, errorsx.ReadFromOperation, err) } return n, addr, nil } @@ -391,11 +382,8 @@ func (d *quicDialerErrWrapper) DialContext( tlsCfg *tls.Config, cfg *quic.Config) (quic.EarlySession, error) { sess, err := d.QUICDialer.DialContext(ctx, network, host, tlsCfg, cfg) if err != nil { - return nil, &errorsx.ErrWrapper{ - Failure: errorsx.ClassifyQUICHandshakeError(err), - Operation: errorsx.QUICHandshakeOperation, - WrappedErr: err, - } + return nil, errorsx.NewErrWrapper( + errorsx.ClassifyQUICHandshakeError, errorsx.QUICHandshakeOperation, err) } return sess, nil } diff --git a/internal/netxlite/quirks.go b/internal/netxlite/quirks.go index a70ee2a..6774c1e 100644 --- a/internal/netxlite/quirks.go +++ b/internal/netxlite/quirks.go @@ -29,6 +29,8 @@ import ( // // This is CLEARLY a QUIRK anyway. There may code depending on how // we do things here and it's tricky to remove this behavior. +// +// See TODO(https://github.com/ooni/probe/issues/1779). func quirkReduceErrors(errorslist []error) error { if len(errorslist) == 0 { return nil @@ -49,6 +51,8 @@ func quirkReduceErrors(errorslist []error) error { // // It saddens me to have this quirk, but it is here to pair // with quirkReduceErrors, which assumes that . +// +// See TODO(https://github.com/ooni/probe/issues/1779). func quirkSortIPAddrs(addrs []string) (out []string) { isIPv6 := func(x string) bool { // This check for identifying IPv6 is discussed diff --git a/internal/netxlite/quirks_test.go b/internal/netxlite/quirks_test.go index 4f60345..f12d20b 100644 --- a/internal/netxlite/quirks_test.go +++ b/internal/netxlite/quirks_test.go @@ -35,12 +35,16 @@ func TestQuirkReduceErrors(t *testing.T) { t.Run("multiple errors with meaningful ones", func(t *testing.T) { err1 := errors.New("mocked error #1") - err2 := &errorsx.ErrWrapper{ - Failure: "unknown_failure: antani", - } - err3 := &errorsx.ErrWrapper{ - Failure: errorsx.FailureConnectionRefused, - } + err2 := errorsx.NewErrWrapper( + errorsx.ClassifyGenericError, + errorsx.CloseOperation, + errors.New("antani"), + ) + err3 := errorsx.NewErrWrapper( + errorsx.ClassifyGenericError, + errorsx.CloseOperation, + errorsx.ECONNREFUSED, + ) err4 := errors.New("mocked error #3") result := quirkReduceErrors([]error{err1, err2, err3, err4}) if result.Error() != errorsx.FailureConnectionRefused { diff --git a/internal/netxlite/resolver.go b/internal/netxlite/resolver.go index 785887c..d8a3e49 100644 --- a/internal/netxlite/resolver.go +++ b/internal/netxlite/resolver.go @@ -197,11 +197,8 @@ var _ Resolver = &resolverErrWrapper{} func (r *resolverErrWrapper) LookupHost(ctx context.Context, hostname string) ([]string, error) { addrs, err := r.Resolver.LookupHost(ctx, hostname) if err != nil { - return nil, &errorsx.ErrWrapper{ - Failure: errorsx.ClassifyResolverError(err), - Operation: errorsx.ResolveOperation, - WrappedErr: err, - } + return nil, errorsx.NewErrWrapper( + errorsx.ClassifyResolverError, errorsx.ResolveOperation, err) } return addrs, nil } diff --git a/internal/netxlite/tls.go b/internal/netxlite/tls.go index e2af24e..c112945 100644 --- a/internal/netxlite/tls.go +++ b/internal/netxlite/tls.go @@ -338,11 +338,8 @@ func (h *tlsHandshakerErrWrapper) Handshake( ) (net.Conn, tls.ConnectionState, error) { tlsconn, state, err := h.TLSHandshaker.Handshake(ctx, conn, config) if err != nil { - return nil, tls.ConnectionState{}, &errorsx.ErrWrapper{ - Failure: errorsx.ClassifyTLSHandshakeError(err), - Operation: errorsx.TLSHandshakeOperation, - WrappedErr: err, - } + return nil, tls.ConnectionState{}, errorsx.NewErrWrapper( + errorsx.ClassifyTLSHandshakeError, errorsx.TLSHandshakeOperation, err) } return tlsconn, state, nil }