refactor(netxlite): restructure dialer tests (#486)

While there, add one more test checking for whether the internal
CA bundle we use can actually be loaded.

Part of https://github.com/ooni/probe/issues/1591

internal/netxlite/certifi_test.go

@@ -0,0 +1,13 @@
+package netxlite
+
+import (
+	"crypto/x509"
+	"testing"
+)
+
+func TestPEMCerts(t *testing.T) {
+	pool := x509.NewCertPool()
+	if !pool.AppendCertsFromPEM([]byte(pemcerts)) {
+		t.Fatal("cannot load pemcerts")
+	}
+}

internal/netxlite/dialer.go

@@ -19,7 +19,16 @@ type Dialer interface {
 	CloseIdleConnections()
 }
 
-// NewDialerWithResolver creates a dialer using the given resolver and logger.
+// NewDialerWithResolver creates a new Dialer. The returned Dialer
+// has the following properties:
+//
+// 1. logs events using the given logger
+//
+// 2. resolves domain names using the givern resolver
+//
+// 3. wraps errors
+//
+// 4. has a configured connect timeout
 func NewDialerWithResolver(logger Logger, resolver Resolver) Dialer {
 	return &dialerLogger{
 		Dialer: &dialerResolver{
@@ -36,24 +45,30 @@ func NewDialerWithResolver(logger Logger, resolver Resolver) Dialer {
 	}
 }
 
-// NewDialerWithoutResolver creates a dialer that uses the given
-// logger and fails with ErrNoResolver when it is passed a domain name.
+// NewDialerWithoutResolver is like NewDialerWithResolver except that
+// it will fail with ErrNoResolver if passed a domain name.
 func NewDialerWithoutResolver(logger Logger) Dialer {
 	return NewDialerWithResolver(logger, &nullResolver{})
 }
 
-// underlyingDialer is the Dialer we use by default.
-var underlyingDialer = &net.Dialer{
-	Timeout:   15 * time.Second,
-	KeepAlive: 15 * time.Second,
+// dialerSystem dials using Go stdlib.
+type dialerSystem struct {
+	// timeout is the OPTIONAL timeout used for testing.
+	timeout time.Duration
 }
 
-// dialerSystem dials using Go stdlib.
-type dialerSystem struct{}
+// newUnderlyingDialer creates a new underlying dialer.
+func (d *dialerSystem) newUnderlyingDialer() *net.Dialer {
+	t := d.timeout
+	if t <= 0 {
+		t = 15 * time.Second
+	}
+	return &net.Dialer{Timeout: t}
+}
 
 // DialContext implements Dialer.DialContext.
 func (d *dialerSystem) DialContext(ctx context.Context, network, address string) (net.Conn, error) {
-	return underlyingDialer.DialContext(ctx, network, address)
+	return d.newUnderlyingDialer().DialContext(ctx, network, address)
 }
 
 // CloseIdleConnections implements Dialer.CloseIdleConnections.
@@ -61,8 +76,6 @@ func (d *dialerSystem) CloseIdleConnections() {
 	// nothing
 }
 
-var defaultDialer Dialer = &dialerSystem{}
-
 // 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 {

internal/netxlite/dialer_test.go

@@ -14,252 +14,284 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/netxlite/mocks"
 )
 
-func TestDialerSystemCloseIdleConnections(t *testing.T) {
-	d := &dialerSystem{}
-	d.CloseIdleConnections() // should not crash
+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)
+		if logger.Logger != log.Log {
+			t.Fatal("invalid logger")
+		}
+		reso := logger.Dialer.(*dialerResolver)
+		if _, okay := reso.Resolver.(*nullResolver); !okay {
+			t.Fatal("invalid Resolver type")
+		}
+		logger = reso.Dialer.(*dialerLogger)
+		if logger.Logger != log.Log {
+			t.Fatal("invalid logger")
+		}
+		errWrapper := logger.Dialer.(*dialerErrWrapper)
+		_ = errWrapper.Dialer.(*dialerSystem)
+	})
 }
 
-func TestDialerResolverNoPort(t *testing.T) {
-	dialer := &dialerResolver{Dialer: defaultDialer, Resolver: DefaultResolver}
-	conn, err := dialer.DialContext(context.Background(), "tcp", "ooni.nu")
-	if err == nil || !strings.HasSuffix(err.Error(), "missing port in address") {
-		t.Fatal("not the error we expected", err)
-	}
-	if conn != nil {
-		t.Fatal("expected a nil conn here")
-	}
+func TestDialerSystem(t *testing.T) {
+	t.Run("has a default timeout of 15 seconds", func(t *testing.T) {
+		d := &dialerSystem{}
+		ud := d.newUnderlyingDialer()
+		if ud.Timeout != 15*time.Second {
+			t.Fatal("invalid default timeout")
+		}
+	})
+
+	t.Run("we can change the default timeout for testing", func(t *testing.T) {
+		d := &dialerSystem{timeout: 1 * time.Second}
+		ud := d.newUnderlyingDialer()
+		if ud.Timeout != 1*time.Second {
+			t.Fatal("invalid default timeout")
+		}
+	})
+
+	t.Run("CloseIdleConnections", func(t *testing.T) {
+		d := &dialerSystem{}
+		d.CloseIdleConnections() // should not crash
+	})
+
+	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")
+		}
+	})
 }
 
-func TestDialerResolverLookupHostAddress(t *testing.T) {
-	dialer := &dialerResolver{Dialer: defaultDialer, Resolver: &mocks.Resolver{
-		MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
-			return nil, errors.New("we should not call this function")
-		},
-	}}
-	addrs, err := dialer.lookupHost(context.Background(), "1.1.1.1")
-	if err != nil {
-		t.Fatal(err)
-	}
-	if len(addrs) != 1 || addrs[0] != "1.1.1.1" {
-		t.Fatal("not the result we expected")
-	}
-}
+func TestDialerResolver(t *testing.T) {
+	t.Run("DialContext", func(t *testing.T) {
+		t.Run("without a port", func(t *testing.T) {
+			d := &dialerResolver{
+				Dialer:   &dialerSystem{},
+				Resolver: &resolverSystem{},
+			}
+			conn, err := d.DialContext(context.Background(), "tcp", "ooni.nu")
+			if err == nil || !strings.HasSuffix(err.Error(), "missing port in address") {
+				t.Fatal("not the error we expected", err)
+			}
+			if conn != nil {
+				t.Fatal("expected a nil conn here")
+			}
+		})
 
-func TestDialerResolverLookupHostFailure(t *testing.T) {
-	expected := errors.New("mocked error")
-	dialer := &dialerResolver{Dialer: defaultDialer, Resolver: &mocks.Resolver{
-		MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
-			return nil, expected
-		},
-	}}
-	ctx := context.Background()
-	conn, err := dialer.DialContext(ctx, "tcp", "dns.google.com:853")
-	if !errors.Is(err, expected) {
-		t.Fatal("not the error we expected", err)
-	}
-	if conn != nil {
-		t.Fatal("expected nil conn")
-	}
-}
-
-func TestDialerResolverDialForSingleIPFails(t *testing.T) {
-	dialer := &dialerResolver{Dialer: &mocks.Dialer{
-		MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
-			return nil, io.EOF
-		},
-	}, 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")
-	}
-	if conn != nil {
-		t.Fatal("expected nil conn")
-	}
-}
-
-func TestDialerResolverDialForManyIPFails(t *testing.T) {
-	dialer := &dialerResolver{
-		Dialer: &mocks.Dialer{
-			MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
-				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 := dialer.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")
-	}
-}
-
-func TestDialerResolverDialForManyIPSuccess(t *testing.T) {
-	dialer := &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
-		},
-	}, 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 := dialer.DialContext(context.Background(), "tcp", "dot.dns:853")
-	if err != nil {
-		t.Fatal("expected nil error here")
-	}
-	if conn == nil {
-		t.Fatal("expected non-nil conn")
-	}
-	conn.Close()
-}
-
-func TestDialerResolverCloseIdleConnections(t *testing.T) {
-	var (
-		calledDialer   bool
-		calledResolver bool
-	)
-	d := &dialerResolver{
-		Dialer: &mocks.Dialer{
-			MockCloseIdleConnections: func() {
-				calledDialer = true
-			},
-		},
-		Resolver: &mocks.Resolver{
-			MockCloseIdleConnections: func() {
-				calledResolver = true
-			},
-		},
-	}
-	d.CloseIdleConnections()
-	if !calledDialer || !calledResolver {
-		t.Fatal("not called")
-	}
-}
-
-func TestDialerLoggerSuccess(t *testing.T) {
-	d := &dialerLogger{
-		Dialer: &mocks.Dialer{
-			MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
-				return &mocks.Conn{
-					MockClose: func() error {
-						return nil
+		t.Run("handles dialing error correctly for single IP address", func(t *testing.T) {
+			d := &dialerResolver{
+				Dialer: &mocks.Dialer{
+					MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
+						return nil, io.EOF
 					},
-				}, nil
+				},
+				Resolver: &nullResolver{},
+			}
+			conn, err := d.DialContext(context.Background(), "tcp", "1.1.1.1:853")
+			if !errors.Is(err, io.EOF) {
+				t.Fatal("not the error we expected")
+			}
+			if conn != nil {
+				t.Fatal("expected nil conn")
+			}
+		})
+
+		t.Run("handles dialing error correctly for many IP addresses", func(t *testing.T) {
+			d := &dialerResolver{
+				Dialer: &mocks.Dialer{
+					MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
+						return nil, io.EOF
+					},
+				},
+				Resolver: &nullResolver{},
+			}
+			conn, err := d.DialContext(context.Background(), "tcp", "1.1.1.1:853")
+			if !errors.Is(err, io.EOF) {
+				t.Fatal("not the error we expected")
+			}
+			if conn != nil {
+				t.Fatal("expected nil conn")
+			}
+		})
+
+		t.Run("handles dialing success correctly for many IP addresses", func(t *testing.T) {
+			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
+					},
+				}, 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 err != nil {
+				t.Fatal(err)
+			}
+			if conn == nil {
+				t.Fatal("expected non-nil conn")
+			}
+			conn.Close()
+		})
+	})
+
+	t.Run("lookupHost", func(t *testing.T) {
+		t.Run("handles addresses correctly", func(t *testing.T) {
+			dialer := &dialerResolver{
+				Dialer:   &dialerSystem{},
+				Resolver: &nullResolver{},
+			}
+			addrs, err := dialer.lookupHost(context.Background(), "1.1.1.1")
+			if err != nil {
+				t.Fatal(err)
+			}
+			if len(addrs) != 1 || addrs[0] != "1.1.1.1" {
+				t.Fatal("not the result we expected")
+			}
+		})
+
+		t.Run("fails correctly on lookup error", func(t *testing.T) {
+			dialer := &dialerResolver{
+				Dialer:   &dialerSystem{},
+				Resolver: &nullResolver{},
+			}
+			ctx := context.Background()
+			conn, err := dialer.DialContext(ctx, "tcp", "dns.google.com:853")
+			if !errors.Is(err, ErrNoResolver) {
+				t.Fatal("not the error we expected", err)
+			}
+			if conn != nil {
+				t.Fatal("expected nil conn")
+			}
+		})
+	})
+
+	t.Run("CloseIdleConnections", func(t *testing.T) {
+		var (
+			calledDialer   bool
+			calledResolver bool
+		)
+		d := &dialerResolver{
+			Dialer: &mocks.Dialer{
+				MockCloseIdleConnections: func() {
+					calledDialer = true
+				},
 			},
-		},
-		Logger: log.Log,
-	}
-	conn, err := d.DialContext(context.Background(), "tcp", "www.google.com:443")
-	if err != nil {
-		t.Fatal(err)
-	}
-	if conn == nil {
-		t.Fatal("expected non-nil conn here")
-	}
-	conn.Close()
-}
-
-func TestDialerLoggerFailure(t *testing.T) {
-	d := &dialerLogger{
-		Dialer: &mocks.Dialer{
-			MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
-				return nil, io.EOF
+			Resolver: &mocks.Resolver{
+				MockCloseIdleConnections: func() {
+					calledResolver = true
+				},
 			},
-		},
-		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 TestDialerLoggerCloseIdleConnections(t *testing.T) {
-	var (
-		calledDialer bool
-	)
-	d := &dialerLogger{
-		Dialer: &mocks.Dialer{
-			MockCloseIdleConnections: func() {
-				calledDialer = true
-			},
-		},
-	}
-	d.CloseIdleConnections()
-	if !calledDialer {
-		t.Fatal("not called")
-	}
-}
-
-func TestUnderlyingDialerHasTimeout(t *testing.T) {
-	expected := 15 * time.Second
-	if underlyingDialer.Timeout != expected {
-		t.Fatal("unexpected timeout value")
-	}
-}
-
-func TestNewDialerWithoutResolverChain(t *testing.T) {
-	dlr := NewDialerWithoutResolver(log.Log)
-	dlog, okay := dlr.(*dialerLogger)
-	if !okay {
-		t.Fatal("invalid type")
-	}
-	if dlog.Logger != log.Log {
-		t.Fatal("invalid logger")
-	}
-	dreso, okay := dlog.Dialer.(*dialerResolver)
-	if !okay {
-		t.Fatal("invalid type")
-	}
-	if _, okay := dreso.Resolver.(*nullResolver); !okay {
-		t.Fatal("invalid Resolver type")
-	}
-	dlog, okay = dreso.Dialer.(*dialerLogger)
-	if !okay {
-		t.Fatal("invalid type")
-	}
-	if dlog.Logger != log.Log {
-		t.Fatal("invalid logger")
-	}
-	dew, okay := dlog.Dialer.(*dialerErrWrapper)
-	if !okay {
-		t.Fatal("invalid type")
-	}
-	if _, okay := dew.Dialer.(*dialerSystem); !okay {
-		t.Fatal("invalid type")
-	}
-}
-
-func TestNewSingleUseDialerWorksAsIntended(t *testing.T) {
-	conn := &mocks.Conn{}
-	d := NewSingleUseDialer(conn)
-	outconn, err := d.DialContext(context.Background(), "", "")
-	if err != nil {
-		t.Fatal(err)
-	}
-	if conn != outconn {
-		t.Fatal("invalid outconn")
-	}
-	for i := 0; i < 4; i++ {
-		outconn, err = d.DialContext(context.Background(), "", "")
-		if !errors.Is(err, ErrNoConnReuse) {
-			t.Fatal("not the error we expected", err)
 		}
-		if outconn != nil {
-			t.Fatal("expected nil outconn here")
+		d.CloseIdleConnections()
+		if !calledDialer || !calledResolver {
+			t.Fatal("not called")
 		}
-	}
+	})
+}
+
+func TestDialerLogger(t *testing.T) {
+	t.Run("DialContext", func(t *testing.T) {
+		t.Run("handles success correctly", func(t *testing.T) {
+			d := &dialerLogger{
+				Dialer: &mocks.Dialer{
+					MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
+						return &mocks.Conn{
+							MockClose: func() error {
+								return nil
+							},
+						}, nil
+					},
+				},
+				Logger: log.Log,
+			}
+			conn, err := d.DialContext(context.Background(), "tcp", "www.google.com:443")
+			if err != nil {
+				t.Fatal(err)
+			}
+			if conn == nil {
+				t.Fatal("expected non-nil conn here")
+			}
+			conn.Close()
+		})
+
+		t.Run("handles failure correctly", func(t *testing.T) {
+			d := &dialerLogger{
+				Dialer: &mocks.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")
+			}
+		})
+	})
+
+	t.Run("CloseIdleConnections", func(t *testing.T) {
+		var (
+			calledDialer bool
+		)
+		d := &dialerLogger{
+			Dialer: &mocks.Dialer{
+				MockCloseIdleConnections: func() {
+					calledDialer = true
+				},
+			},
+		}
+		d.CloseIdleConnections()
+		if !calledDialer {
+			t.Fatal("not called")
+		}
+	})
+}
+
+func TestDialerSingleUse(t *testing.T) {
+	t.Run("works as intended", func(t *testing.T) {
+		conn := &mocks.Conn{}
+		d := NewSingleUseDialer(conn)
+		outconn, err := d.DialContext(context.Background(), "", "")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if conn != outconn {
+			t.Fatal("invalid outconn")
+		}
+		for i := 0; i < 4; i++ {
+			outconn, err = d.DialContext(context.Background(), "", "")
+			if !errors.Is(err, ErrNoConnReuse) {
+				t.Fatal("not the error we expected", err)
+			}
+			if outconn != nil {
+				t.Fatal("expected nil outconn here")
+			}
+		}
+	})
+
+	t.Run("CloseIdleConnections", func(t *testing.T) {
+		d := &dialerSingleUse{}
+		d.CloseIdleConnections() // does not crash
+	})
 }
 
 func TestDialerErrWrapper(t *testing.T) {

internal/netxlite/legacy.go

@@ -10,7 +10,7 @@ import (
 
 // These vars export internal names to legacy ooni/probe-cli code.
 var (
-	DefaultDialer        = defaultDialer
+	DefaultDialer        = &dialerSystem{}
 	DefaultTLSHandshaker = defaultTLSHandshaker
 	NewConnUTLS          = newConnUTLS
 )

internal/netxlite/tls.go

@@ -73,7 +73,7 @@ func TLSCipherSuiteString(value uint16) string {
 func NewDefaultCertPool() *x509.CertPool {
 	pool := x509.NewCertPool()
 	// Assumption: AppendCertsFromPEM cannot fail because we
-	// run this function already in the generate.go file
+	// have a test in certify_test.go that guarantees that
 	pool.AppendCertsFromPEM([]byte(pemcerts))
 	return pool
 }

internal/netxlite/tls_test.go

@@ -310,7 +310,7 @@ func TestTLSDialerDialTLSContextFailureSplitHostPort(t *testing.T) {
 func TestTLSDialerDialTLSContextFailureDialing(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	cancel() // immediately fail
-	dialer := tlsDialer{Dialer: defaultDialer}
+	dialer := tlsDialer{Dialer: &dialerSystem{}}
 	conn, err := dialer.DialTLSContext(ctx, "tcp", "www.google.com:443")
 	if err == nil || !strings.HasSuffix(err.Error(), "operation was canceled") {
 		t.Fatal("not the error we expected", err)