refactor: move quic dns dialing to netxlite (#408)

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

internal/engine/netx/netx.go

@@ -174,7 +174,7 @@ func NewQUICDialer(config Config) QUICDialer {
 	if config.TLSSaver != nil {
 		d = quicdialer.HandshakeSaver{Saver: config.TLSSaver, Dialer: d}
 	}
-	d = &quicdialer.DNSDialer{Resolver: config.FullResolver, Dialer: d}
+	d = &netxlite.QUICDialerResolver{Resolver: config.FullResolver, Dialer: d}
 	var dialer QUICDialer = &httptransport.QUICWrapperDialer{Dialer: d}
 	return dialer
 }

internal/engine/netx/quicdialer/dns.go

@@ -1,57 +0,0 @@
-package quicdialer
-
-import (
-	"context"
-	"crypto/tls"
-	"net"
-
-	"github.com/lucas-clemente/quic-go"
-	"github.com/ooni/probe-cli/v3/internal/netxlite"
-)
-
-// DNSDialer is a dialer that uses the configured Resolver to resolve a
-// domain name to IP addresses
-type DNSDialer struct {
-	Dialer   ContextDialer
-	Resolver Resolver
-}
-
-// DialContext implements ContextDialer.DialContext
-func (d DNSDialer) DialContext(
-	ctx context.Context, network, host string,
-	tlsCfg *tls.Config, cfg *quic.Config) (quic.EarlySession, error) {
-	onlyhost, onlyport, err := net.SplitHostPort(host)
-	if err != nil {
-		return nil, err
-	}
-	// TODO(kelmenhorst): Should this be somewhere else?
-	// failure if tlsCfg is nil but that should not happen
-	if tlsCfg.ServerName == "" {
-		tlsCfg.ServerName = onlyhost
-	}
-	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)
-		sess, err := d.Dialer.DialContext(
-			ctx, network, target, tlsCfg, cfg)
-		if err == nil {
-			return sess, nil
-		}
-		errorslist = append(errorslist, err)
-	}
-	// TODO(bassosimone): maybe ReduceErrors could be in netx/internal.
-	return nil, netxlite.ReduceErrors(errorslist)
-}
-
-// LookupHost implements Resolver.LookupHost
-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)
-}

internal/engine/netx/quicdialer/dns_test.go

@@ -1,149 +0,0 @@
-package quicdialer_test
-
-import (
-	"context"
-	"crypto/tls"
-	"errors"
-	"net"
-	"strconv"
-	"strings"
-	"testing"
-
-	"github.com/lucas-clemente/quic-go"
-	"github.com/ooni/probe-cli/v3/internal/engine/netx/quicdialer"
-	"github.com/ooni/probe-cli/v3/internal/netxlite"
-)
-
-type MockableResolver struct {
-	Addresses []string
-	Err       error
-}
-
-func (r MockableResolver) LookupHost(ctx context.Context, host string) ([]string, error) {
-	return r.Addresses, r.Err
-}
-
-func TestDNSDialerSuccess(t *testing.T) {
-	tlsConf := &tls.Config{NextProtos: []string{"h3"}}
-	dialer := quicdialer.DNSDialer{
-		Resolver: new(net.Resolver), Dialer: &netxlite.QUICDialerQUICGo{
-			QUICListener: &netxlite.QUICListenerStdlib{},
-		}}
-	sess, err := dialer.DialContext(
-		context.Background(), "udp", "www.google.com:443",
-		tlsConf, &quic.Config{})
-	if err != nil {
-		t.Fatal("unexpected error", err)
-	}
-	if sess == nil {
-		t.Fatal("non nil sess expected")
-	}
-}
-
-func TestDNSDialerNoPort(t *testing.T) {
-	tlsConf := &tls.Config{NextProtos: []string{"h3"}}
-	dialer := quicdialer.DNSDialer{
-		Resolver: new(net.Resolver), Dialer: &netxlite.QUICDialerQUICGo{}}
-	sess, err := dialer.DialContext(
-		context.Background(), "udp", "www.google.com",
-		tlsConf, &quic.Config{})
-	if err == nil {
-		t.Fatal("expected an error here")
-	}
-	if sess != nil {
-		t.Fatal("expected a nil sess here")
-	}
-	if err.Error() != "address www.google.com: missing port in address" {
-		t.Fatal("not the error we expected")
-	}
-}
-
-func TestDNSDialerLookupHostAddress(t *testing.T) {
-	dialer := quicdialer.DNSDialer{Resolver: MockableResolver{
-		Err: errors.New("mocked error"),
-	}}
-	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 TestDNSDialerLookupHostFailure(t *testing.T) {
-	tlsConf := &tls.Config{NextProtos: []string{"h3"}}
-	expected := errors.New("mocked error")
-	dialer := quicdialer.DNSDialer{Resolver: MockableResolver{
-		Err: expected,
-	}}
-	sess, err := dialer.DialContext(
-		context.Background(), "udp", "dns.google.com:853",
-		tlsConf, &quic.Config{})
-	if !errors.Is(err, expected) {
-		t.Fatal("not the error we expected")
-	}
-	if sess != nil {
-		t.Fatal("expected nil sess")
-	}
-}
-
-func TestDNSDialerInvalidPort(t *testing.T) {
-	tlsConf := &tls.Config{NextProtos: []string{"h3"}}
-	dialer := quicdialer.DNSDialer{
-		Resolver: new(net.Resolver), Dialer: &netxlite.QUICDialerQUICGo{
-			QUICListener: &netxlite.QUICListenerStdlib{},
-		}}
-	sess, err := dialer.DialContext(
-		context.Background(), "udp", "www.google.com:0",
-		tlsConf, &quic.Config{})
-	if err == nil {
-		t.Fatal("expected an error here")
-	}
-	if sess != nil {
-		t.Fatal("expected nil sess")
-	}
-	if !strings.HasSuffix(err.Error(), "sendto: invalid argument") &&
-		!strings.HasSuffix(err.Error(), "sendto: can't assign requested address") {
-		t.Fatal("not the error we expected")
-	}
-}
-
-func TestDNSDialerInvalidPortSyntax(t *testing.T) {
-	tlsConf := &tls.Config{NextProtos: []string{"h3"}}
-	dialer := quicdialer.DNSDialer{
-		Resolver: new(net.Resolver), Dialer: &netxlite.QUICDialerQUICGo{
-			QUICListener: &netxlite.QUICListenerStdlib{},
-		}}
-	sess, err := dialer.DialContext(
-		context.Background(), "udp", "www.google.com:port",
-		tlsConf, &quic.Config{})
-	if err == nil {
-		t.Fatal("expected an error here")
-	}
-	if sess != nil {
-		t.Fatal("expected nil sess")
-	}
-	if !errors.Is(err, strconv.ErrSyntax) {
-		t.Fatal("not the error we expected")
-	}
-}
-
-func TestDNSDialerDialEarlyFails(t *testing.T) {
-	tlsConf := &tls.Config{NextProtos: []string{"h3"}}
-	expected := errors.New("mocked DialEarly error")
-	dialer := quicdialer.DNSDialer{
-		Resolver: new(net.Resolver), Dialer: MockDialer{Err: expected}}
-	sess, err := dialer.DialContext(
-		context.Background(), "udp", "www.google.com:443",
-		tlsConf, &quic.Config{})
-	if err == nil {
-		t.Fatal("expected an error here")
-	}
-	if sess != nil {
-		t.Fatal("expected nil sess")
-	}
-	if !errors.Is(err, expected) {
-		t.Fatal("not the error we expected")
-	}
-}

internal/netxlite/dialer.go

@@ -55,7 +55,7 @@ func (d *DialerResolver) DialContext(ctx context.Context, network, address strin
 		}
 		errorslist = append(errorslist, err)
 	}
-	return nil, ReduceErrors(errorslist)
+	return nil, reduceErrors(errorslist)
 }
 
 // lookupHost performs a domain name resolution.

internal/netxlite/legacy.go

@@ -7,7 +7,7 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/engine/netx/errorx"
 )
 
-// ReduceErrors finds a known error in a list of errors since
+// reduceErrors finds a known error in a list of errors since
 // it's probably most relevant.
 //
 // Deprecation warning
@@ -16,7 +16,7 @@ import (
 //
 // 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 {
+func reduceErrors(errorslist []error) error {
 	if len(errorslist) == 0 {
 		return nil
 	}

internal/netxlite/legacy_test.go

@@ -9,14 +9,14 @@ import (
 
 func TestReduceErrors(t *testing.T) {
 	t.Run("no errors", func(t *testing.T) {
-		result := ReduceErrors(nil)
+		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})
+		result := reduceErrors([]error{err})
 		if result != err {
 			t.Fatal("wrong result")
 		}
@@ -24,7 +24,7 @@ func TestReduceErrors(t *testing.T) {
 	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})
+		result := reduceErrors([]error{err1, err2})
 		if result.Error() != "mocked error #1" {
 			t.Fatal("wrong result")
 		}
@@ -38,7 +38,7 @@ func TestReduceErrors(t *testing.T) {
 			Failure: errorx.FailureConnectionRefused,
 		}
 		err4 := errors.New("mocked error #3")
-		result := ReduceErrors([]error{err1, err2, err3, err4})
+		result := reduceErrors([]error{err1, err2, err3, err4})
 		if result.Error() != errorx.FailureConnectionRefused {
 			t.Fatal("wrong result")
 		}

internal/netxlite/quic.go

@@ -19,15 +19,6 @@ type QUICContextDialer interface {
 		tlsConfig *tls.Config, quicConfig *quic.Config) (quic.EarlySession, error)
 }
 
-// QUICDialer dials QUIC connections.
-type QUICDialer interface {
-	// DialContext establishes a new QUIC session using the given
-	// network and address. The tlsConfig and the quicConfig arguments
-	// MUST NOT be nil. Returns either the session or an error.
-	Dial(network, address string, tlsConfig *tls.Config,
-		quicConfig *quic.Config) (quic.EarlySession, error)
-}
-
 // QUICListener listens for QUIC connections.
 type QUICListener interface {
 	// Listen creates a new listening PacketConn.
@@ -100,3 +91,55 @@ func (sess *quicSessionOwnsConn) CloseWithError(
 	sess.conn.Close()
 	return err
 }
+
+// QUICDialerResolver is a dialer that uses the configured Resolver
+// to resolve a domain name to IP addrs.
+type QUICDialerResolver struct {
+	// Dialer is the underlying QUIC dialer.
+	Dialer QUICContextDialer
+
+	// Resolver is the underlying resolver.
+	Resolver Resolver
+}
+
+// DialContext implements QUICContextDialer.DialContext
+func (d *QUICDialerResolver) DialContext(
+	ctx context.Context, network, address string,
+	tlsConfig *tls.Config, quicConfig *quic.Config) (quic.EarlySession, error) {
+	onlyhost, onlyport, err := net.SplitHostPort(address)
+	if err != nil {
+		return nil, err
+	}
+	// TODO(kelmenhorst): Should this be somewhere else?
+	// failure if tlsCfg is nil but that should not happen
+	if tlsConfig.ServerName == "" {
+		tlsConfig.ServerName = onlyhost
+	}
+	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)
+		sess, err := d.Dialer.DialContext(
+			ctx, network, target, tlsConfig, quicConfig)
+		if err == nil {
+			return sess, nil
+		}
+		errorslist = append(errorslist, err)
+	}
+	return nil, reduceErrors(errorslist)
+}
+
+// lookupHost performs a domain name resolution.
+func (d *QUICDialerResolver) lookupHost(ctx context.Context, hostname string) ([]string, error) {
+	if net.ParseIP(hostname) != nil {
+		return []string{hostname}, nil
+	}
+	return d.Resolver.LookupHost(ctx, hostname)
+}

internal/netxlite/quic_test.go

@@ -133,3 +133,95 @@ func TestQUICDialerWorksAsIntended(t *testing.T) {
 		log.Fatal(err)
 	}
 }
+
+func TestQUICDialerResolverSuccess(t *testing.T) {
+	tlsConfig := &tls.Config{NextProtos: []string{"h3"}}
+	dialer := &QUICDialerResolver{
+		Resolver: &net.Resolver{}, Dialer: &QUICDialerQUICGo{
+			QUICListener: &QUICListenerStdlib{},
+		}}
+	sess, err := dialer.DialContext(
+		context.Background(), "udp", "www.google.com:443",
+		tlsConfig, &quic.Config{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	<-sess.HandshakeComplete().Done()
+	if err := sess.CloseWithError(0, ""); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestQUICDialerResolverNoPort(t *testing.T) {
+	tlsConfig := &tls.Config{NextProtos: []string{"h3"}}
+	dialer := &QUICDialerResolver{
+		Resolver: new(net.Resolver), Dialer: &QUICDialerQUICGo{}}
+	sess, err := dialer.DialContext(
+		context.Background(), "udp", "www.google.com",
+		tlsConfig, &quic.Config{})
+	if err == nil || !strings.HasSuffix(err.Error(), "missing port in address") {
+		t.Fatal("not the error we expected")
+	}
+	if sess != nil {
+		t.Fatal("expected a nil sess here")
+	}
+}
+
+func TestQUICDialerResolverLookupHostAddress(t *testing.T) {
+	dialer := &QUICDialerResolver{Resolver: &netxmocks.Resolver{
+		MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+			// We should not arrive here and call this function but if we do then
+			// there is going to be an error that fails this test.
+			return nil, errors.New("mocked error")
+		},
+	}}
+	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 TestQUICDialerResolverLookupHostFailure(t *testing.T) {
+	tlsConfig := &tls.Config{NextProtos: []string{"h3"}}
+	expected := errors.New("mocked error")
+	dialer := &QUICDialerResolver{Resolver: &netxmocks.Resolver{
+		MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+			return nil, expected
+		},
+	}}
+	sess, err := dialer.DialContext(
+		context.Background(), "udp", "dns.google.com:853",
+		tlsConfig, &quic.Config{})
+	if !errors.Is(err, expected) {
+		t.Fatal("not the error we expected")
+	}
+	if sess != nil {
+		t.Fatal("expected nil sess")
+	}
+}
+
+func TestQUICDialerResolverInvalidPort(t *testing.T) {
+	// This test allows us to check for the case where every attempt
+	// to establish a connection leads to a failure
+	tlsConf := &tls.Config{NextProtos: []string{"h3"}}
+	dialer := &QUICDialerResolver{
+		Resolver: new(net.Resolver), Dialer: &QUICDialerQUICGo{
+			QUICListener: &QUICListenerStdlib{},
+		}}
+	sess, err := dialer.DialContext(
+		context.Background(), "udp", "www.google.com:0",
+		tlsConf, &quic.Config{})
+	if err == nil {
+		t.Fatal("expected an error here")
+	}
+	if !strings.HasSuffix(err.Error(), "sendto: invalid argument") &&
+		!strings.HasSuffix(err.Error(), "sendto: can't assign requested address") {
+		t.Fatal("not the error we expected", err)
+	}
+	if sess != nil {
+		t.Fatal("expected nil sess")
+	}
+}