refactor: migrate apitool from netx to netxlite (#496)

I discovered which transport were used by apitool and made sure he gets the same transports now. While there, I discovered an issue with ooni/oohttp that has been fixed with cba9b1ce5e.

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

go.mod

@@ -28,7 +28,7 @@ require (
 	github.com/miekg/dns v1.1.42
 	github.com/mitchellh/go-wordwrap v1.0.1
 	github.com/montanaflynn/stats v0.6.6
-	github.com/ooni/oohttp v0.0.0-20210901122724-bbe70ef3c22a
+	github.com/ooni/oohttp v0.0.0-20210908222758-cba9b1ce5ecf
 	github.com/ooni/probe-assets v0.3.1
 	github.com/ooni/psiphon v0.8.0
 	github.com/oschwald/geoip2-golang v1.5.0
@@ -43,7 +43,7 @@ require (
 	gitlab.com/yawning/utls.git v0.0.12-1
 	golang.org/x/crypto v0.0.0-20210817164053-32db794688a5 // indirect
 	golang.org/x/mod v0.5.0 // indirect
-	golang.org/x/net v0.0.0-20210903162142-ad29c8ab022f
+	golang.org/x/net v0.0.0-20210908191846-a5e095526f91
 	golang.org/x/sys v0.0.0-20210817190340-bfb29a6856f2
 	golang.org/x/text v0.3.7 // indirect
 	golang.org/x/tools v0.1.5 // indirect

go.sum

@@ -393,8 +393,8 @@ github.com/onsi/gomega v1.10.1/go.mod h1:iN09h71vgCQne3DLsj+A5owkum+a2tYe+TOCB1y
 github.com/onsi/gomega v1.10.3/go.mod h1:V9xEwhxec5O8UDM77eCW8vLymOMltsqPVYWrpDsH8xc=
 github.com/onsi/gomega v1.13.0 h1:7lLHu94wT9Ij0o6EWWclhu0aOh32VxhkwEJvzuWPeak=
 github.com/onsi/gomega v1.13.0/go.mod h1:lRk9szgn8TxENtWd0Tp4c3wjlRfMTMH27I+3Je41yGY=
-github.com/ooni/oohttp v0.0.0-20210901122724-bbe70ef3c22a h1:Vhcn8oTWHy/3SIKcUI2eb3XKWHy74779iCTrMGNCuao=
+github.com/ooni/oohttp v0.0.0-20210908222758-cba9b1ce5ecf h1:eM6zmPpaeIISc57lv0aN4dDHDEXc5ev9yhpXAQTAEJM=
-github.com/ooni/oohttp v0.0.0-20210901122724-bbe70ef3c22a/go.mod h1:kgtoj+Dn4bmx09hEUgbPI7YX0gkWlu+fz2I0S5auyX4=
+github.com/ooni/oohttp v0.0.0-20210908222758-cba9b1ce5ecf/go.mod h1:kgtoj+Dn4bmx09hEUgbPI7YX0gkWlu+fz2I0S5auyX4=
 github.com/ooni/probe-assets v0.3.1 h1:6PDcoJTICJxL8PdeM0+a3ZfkTWrFfCn90fUqTWR0LDA=
 github.com/ooni/probe-assets v0.3.1/go.mod h1:N0PyNM3aadlYDDCFXAPzs54HC54+MZA/4/xnCtd9EAo=
 github.com/ooni/psiphon v0.8.0 h1:digldztBlINi3HWuxdK4gFhkiaheAoDVjZN/ApZHWBM=
@@ -712,8 +712,8 @@ golang.org/x/net v0.0.0-20210316092652-d523dce5a7f4/go.mod h1:RBQZq4jEuRlivfhVLd
 golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4/go.mod h1:p54w0d4576C0XHj96bSt6lcn1PtDYWL6XObtHCRCNQM=
 golang.org/x/net v0.0.0-20210428140749-89ef3d95e781/go.mod h1:OJAsFXCWl8Ukc7SiCT/9KSuxbyM7479/AVlXFRxuMCk=
 golang.org/x/net v0.0.0-20210805182204-aaa1db679c0d/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
-golang.org/x/net v0.0.0-20210903162142-ad29c8ab022f h1:w6wWR0H+nyVpbSAQbzVEIACVyr/h8l/BEkY6Sokc7Eg=
+golang.org/x/net v0.0.0-20210908191846-a5e095526f91 h1:E8wdt+zBjoxD3MA65wEc3pl25BsTi7tbkpwc4ANThjc=
-golang.org/x/net v0.0.0-20210903162142-ad29c8ab022f/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
+golang.org/x/net v0.0.0-20210908191846-a5e095526f91/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
 golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
 golang.org/x/oauth2 v0.0.0-20181017192945-9dcd33a902f4/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
 golang.org/x/oauth2 v0.0.0-20181203162652-d668ce993890/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=

internal/cmd/apitool/main.go

@@ -18,14 +18,14 @@ import (
 	"github.com/apex/log"
 	"github.com/ooni/probe-cli/v3/internal/atomicx"
 	"github.com/ooni/probe-cli/v3/internal/engine/httpx"
-	"github.com/ooni/probe-cli/v3/internal/engine/netx"
 	"github.com/ooni/probe-cli/v3/internal/engine/probeservices"
 	"github.com/ooni/probe-cli/v3/internal/kvstore"
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
 	"github.com/ooni/probe-cli/v3/internal/version"
 )
 
 func newclient() probeservices.Client {
-	txp := netx.NewHTTPTransport(netx.Config{Logger: log.Log})
+	txp := netxlite.NewHTTPTransportStdlib(log.Log)
 	ua := fmt.Sprintf("apitool/%s ooniprobe-engine/%s", version.Version, version.Version)
 	return probeservices.Client{
 		Client: httpx.Client{

internal/engine/netx/dialer/dialer.go

@@ -44,7 +44,7 @@ type Config struct {
 	// Bug
 	//
 	// This implementation cannot properly account for the bytes that are sent by
-	// persistent connections, because they strick to the counters set when the
+	// persistent connections, because they stick to the counters set when the
 	// connection was established. This typically means we miss the bytes sent and
 	// received when submitting a measurement. Such bytes are specifically not
 	// seen by the experiment specific byte counter.

internal/engine/netx/httptransport/useragent.go

@@ -1,19 +1,9 @@
 package httptransport
 
-import "net/http"
+import (
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
+)
 
 // UserAgentTransport is a transport that ensures that we always
 // set an OONI specific default User-Agent header.
-type UserAgentTransport struct {
+type UserAgentTransport = netxlite.UserAgentTransport
-	RoundTripper
-}
-
-// RoundTrip implements RoundTripper.RoundTrip
-func (txp UserAgentTransport) RoundTrip(req *http.Request) (*http.Response, error) {
-	if req.Header.Get("User-Agent") == "" {
-		req.Header.Set("User-Agent", "miniooni/0.1.0-dev")
-	}
-	return txp.RoundTripper.RoundTrip(req)
-}
-
-var _ RoundTripper = UserAgentTransport{}

internal/engine/netx/httptransport/useragent_test.go

@@ -1,51 +0,0 @@
-package httptransport_test
-
-import (
-	"net/http"
-	"net/url"
-	"testing"
-
-	"github.com/ooni/probe-cli/v3/internal/engine/netx/httptransport"
-)
-
-func TestUserAgentWithDefault(t *testing.T) {
-	txp := httptransport.UserAgentTransport{
-		RoundTripper: httptransport.FakeTransport{
-			Resp: &http.Response{StatusCode: 200},
-		},
-	}
-	req := &http.Request{URL: &url.URL{
-		Scheme: "https",
-		Host:   "www.google.com",
-		Path:   "/",
-	}}
-	req.Header = http.Header{}
-	resp, err := txp.RoundTrip(req)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if resp.Request.Header.Get("User-Agent") != "miniooni/0.1.0-dev" {
-		t.Fatal("not the User-Agent we expected")
-	}
-}
-
-func TestUserAgentWithExplicitValue(t *testing.T) {
-	txp := httptransport.UserAgentTransport{
-		RoundTripper: httptransport.FakeTransport{
-			Resp: &http.Response{StatusCode: 200},
-		},
-	}
-	req := &http.Request{URL: &url.URL{
-		Scheme: "https",
-		Host:   "www.google.com",
-		Path:   "/",
-	}}
-	req.Header = http.Header{"User-Agent": []string{"antani-client/0.1.1"}}
-	resp, err := txp.RoundTrip(req)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if resp.Request.Header.Get("User-Agent") != "antani-client/0.1.1" {
-		t.Fatal("not the User-Agent we expected")
-	}
-}

internal/engine/netx/netx.go

@@ -250,7 +250,7 @@ func NewHTTPTransport(config Config) HTTPRoundTripper {
 		txp = httptransport.SaverTransactionHTTPTransport{
 			RoundTripper: txp, Saver: config.HTTPSaver}
 	}
-	txp = httptransport.UserAgentTransport{RoundTripper: txp}
+	txp = &httptransport.UserAgentTransport{HTTPTransport: txp}
 	return txp
 }
 

internal/engine/netx/netx_test.go

@@ -461,11 +461,11 @@ func TestNewTLSDialerWithNoTLSVerifyAndNoConfig(t *testing.T) {
 
 func TestNewVanilla(t *testing.T) {
 	txp := netx.NewHTTPTransport(netx.Config{})
-	uatxp, ok := txp.(httptransport.UserAgentTransport)
+	uatxp, ok := txp.(*httptransport.UserAgentTransport)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}
-	if _, ok := uatxp.RoundTripper.(*http.Transport); !ok {
+	if _, ok := uatxp.HTTPTransport.(*http.Transport); !ok {
 		t.Fatal("not the transport we expected")
 	}
 }
@@ -518,11 +518,11 @@ func TestNewWithByteCounter(t *testing.T) {
 	txp := netx.NewHTTPTransport(netx.Config{
 		ByteCounter: counter,
 	})
-	uatxp, ok := txp.(httptransport.UserAgentTransport)
+	uatxp, ok := txp.(*httptransport.UserAgentTransport)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}
-	bctxp, ok := uatxp.RoundTripper.(httptransport.ByteCountingTransport)
+	bctxp, ok := uatxp.HTTPTransport.(httptransport.ByteCountingTransport)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}
@@ -538,11 +538,11 @@ func TestNewWithLogger(t *testing.T) {
 	txp := netx.NewHTTPTransport(netx.Config{
 		Logger: log.Log,
 	})
-	uatxp, ok := txp.(httptransport.UserAgentTransport)
+	uatxp, ok := txp.(*httptransport.UserAgentTransport)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}
-	ltxp, ok := uatxp.RoundTripper.(*netxlite.HTTPTransportLogger)
+	ltxp, ok := uatxp.HTTPTransport.(*netxlite.HTTPTransportLogger)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}
@@ -559,11 +559,11 @@ func TestNewWithSaver(t *testing.T) {
 	txp := netx.NewHTTPTransport(netx.Config{
 		HTTPSaver: saver,
 	})
-	uatxp, ok := txp.(httptransport.UserAgentTransport)
+	uatxp, ok := txp.(*httptransport.UserAgentTransport)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}
-	stxptxp, ok := uatxp.RoundTripper.(httptransport.SaverTransactionHTTPTransport)
+	stxptxp, ok := uatxp.HTTPTransport.(httptransport.SaverTransactionHTTPTransport)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}

internal/netxlite/http.go

@@ -106,6 +106,9 @@ func (txp *httpTransportConnectionsCloser) CloseIdleConnections() {
 // necessary to perform sane measurements with tracing. We will be
 // able to possibly relax this requirement after we change the
 // way in which we perform measurements.
+//
+// The returned transport will set a default user agent if the
+// request has not already set a user agent.
 func NewHTTPTransport(logger Logger, dialer Dialer, tlsDialer TLSDialer) HTTPTransport {
 	// Using oohttp to support any TLS library.
 	txp := oohttp.DefaultTransport.(*oohttp.Transport).Clone()
@@ -137,13 +140,15 @@ func NewHTTPTransport(logger Logger, dialer Dialer, tlsDialer TLSDialer) HTTPTra
 
 	// Ensure we correctly forward CloseIdleConnections and compose
 	// with a logging transport thus enabling logging.
-	return &httpTransportLogger{
+	return &httpUserAgentTransport{
-		HTTPTransport: &httpTransportConnectionsCloser{
+		HTTPTransport: &httpTransportLogger{
-			HTTPTransport: &oohttp.StdlibTransport{Transport: txp},
+			HTTPTransport: &httpTransportConnectionsCloser{
-			Dialer:        dialer,
+				HTTPTransport: &oohttp.StdlibTransport{Transport: txp},
-			TLSDialer:     tlsDialer,
+				Dialer:        dialer,
+				TLSDialer:     tlsDialer,
+			},
+			Logger: logger,
 		},
-		Logger: logger,
 	}
 }
 
@@ -233,3 +238,29 @@ func (c *httpTLSConnWithReadTimeout) Read(b []byte) (int, error) {
 	defer c.TLSConn.SetReadDeadline(time.Time{})
 	return c.TLSConn.Read(b)
 }
+
+// httpUserAgentTransport is a transport that ensures that we always
+// set an OONI specific default User-Agent header.
+type httpUserAgentTransport struct {
+	HTTPTransport
+}
+
+const defaultHTTPUserAgent = "miniooni/0.1.0-dev"
+
+func (txp *httpUserAgentTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	if req.Header.Get("User-Agent") == "" {
+		req.Header.Set("User-Agent", defaultHTTPUserAgent)
+	}
+	return txp.HTTPTransport.RoundTrip(req)
+}
+
+var _ HTTPTransport = &httpUserAgentTransport{}
+
+// NewHTTPTransportStdlib creates a new HTTPTransport that uses
+// the Go standard library for all operations, including DNS
+// resolutions and TLS handshakes.
+func NewHTTPTransportStdlib(logger Logger) HTTPTransport {
+	dialer := NewDialerWithResolver(logger, NewResolverStdlib(logger))
+	tlsDialer := NewTLSDialer(dialer, NewTLSHandshakerStdlib(logger))
+	return NewHTTPTransport(logger, dialer, tlsDialer)
+}

internal/netxlite/http_test.go

@@ -202,7 +202,7 @@ func TestNewHTTPTransport(t *testing.T) {
 					called.Add(1)
 				},
 			},
-			Resolver: NewResolverSystem(log.Log),
+			Resolver: NewResolverStdlib(log.Log),
 		}
 		td := NewTLSDialer(d, NewTLSHandshakerStdlib(log.Log))
 		txp := NewHTTPTransport(log.Log, d, td)
@@ -224,7 +224,8 @@ func TestNewHTTPTransport(t *testing.T) {
 		d := &mocks.Dialer{}
 		td := &mocks.TLSDialer{}
 		txp := NewHTTPTransport(log.Log, d, td)
-		logger := txp.(*httpTransportLogger)
+		ua := txp.(*httpUserAgentTransport)
+		logger := ua.HTTPTransport.(*httpTransportLogger)
 		if logger.Logger != log.Log {
 			t.Fatal("invalid logger")
 		}
@@ -423,3 +424,78 @@ func TestHTTPTLSDialerWithReadTimeout(t *testing.T) {
 		}
 	})
 }
+
+func TestHTTPUserAgentTransport(t *testing.T) {
+	t.Run("CloseIdleConnections", func(t *testing.T) {
+		var called bool
+		txp := &httpUserAgentTransport{
+			HTTPTransport: &mocks.HTTPTransport{
+				MockCloseIdleConnections: func() {
+					called = true
+				},
+			},
+		}
+		txp.CloseIdleConnections()
+		if !called {
+			t.Fatal("not called")
+		}
+	})
+
+	t.Run("RoundTrip", func(t *testing.T) {
+		t.Run("without an user-agent", func(t *testing.T) {
+			var ua string
+			txp := &httpUserAgentTransport{
+				HTTPTransport: &mocks.HTTPTransport{
+					MockRoundTrip: func(req *http.Request) (*http.Response, error) {
+						ua = req.Header.Get("User-Agent")
+						return &http.Response{}, nil
+					},
+				},
+			}
+			txp.RoundTrip(&http.Request{Header: make(http.Header)})
+			if ua != defaultHTTPUserAgent {
+				t.Fatal("not the expected user-agent")
+			}
+		})
+
+		t.Run("with an user-agent", func(t *testing.T) {
+			var ua string
+			expected := "antani/1.0"
+			txp := &httpUserAgentTransport{
+				HTTPTransport: &mocks.HTTPTransport{
+					MockRoundTrip: func(req *http.Request) (*http.Response, error) {
+						ua = req.Header.Get("User-Agent")
+						return &http.Response{}, nil
+					},
+				},
+			}
+			txp.RoundTrip(&http.Request{
+				Header: http.Header{
+					"User-Agent": {expected},
+				},
+			})
+			if ua != expected {
+				t.Fatal("not the expected user-agent")
+			}
+		})
+	})
+}
+
+func TestNewHTTPTransportStdlib(t *testing.T) {
+	// What to test about this factory?
+	txp := NewHTTPTransportStdlib(log.Log)
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel() // immediately!
+	req, err := http.NewRequestWithContext(ctx, "GET", "http://x.org", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	resp, err := txp.RoundTrip(req)
+	if !errors.Is(err, context.Canceled) {
+		t.Fatal("unexpected err", err)
+	}
+	if resp != nil {
+		t.Fatal("unexpected resp")
+	}
+	txp.CloseIdleConnections()
+}

internal/netxlite/integration_test.go

@@ -21,7 +21,7 @@ func TestResolver(t *testing.T) {
 	t.Run("works as intended", func(t *testing.T) {
 		// TODO(bassosimone): this is actually an integration
 		// test but how to test this case?
-		r := netxlite.NewResolverSystem(log.Log)
+		r := netxlite.NewResolverStdlib(log.Log)
 		defer r.CloseIdleConnections()
 		addrs, err := r.LookupHost(context.Background(), "dns.google.com")
 		if err != nil {
@@ -39,7 +39,7 @@ func TestHTTPTransport(t *testing.T) {
 	}
 
 	t.Run("works as intended", func(t *testing.T) {
-		d := netxlite.NewDialerWithResolver(log.Log, netxlite.NewResolverSystem(log.Log))
+		d := netxlite.NewDialerWithResolver(log.Log, netxlite.NewResolverStdlib(log.Log))
 		td := netxlite.NewTLSDialer(d, netxlite.NewTLSHandshakerStdlib(log.Log))
 		txp := netxlite.NewHTTPTransport(log.Log, d, td)
 		client := &http.Client{Transport: txp}
@@ -61,7 +61,7 @@ func TestHTTP3Transport(t *testing.T) {
 		d := netxlite.NewQUICDialerWithResolver(
 			netxlite.NewQUICListener(),
 			log.Log,
-			netxlite.NewResolverSystem(log.Log),
+			netxlite.NewResolverStdlib(log.Log),
 		)
 		txp := netxlite.NewHTTP3Transport(log.Log, d, &tls.Config{})
 		client := &http.Client{Transport: txp}
@@ -119,7 +119,7 @@ func TestQUICDialer(t *testing.T) {
 	t.Run("can guess the SNI and ALPN when using a domain name for web", func(t *testing.T) {
 		d := netxlite.NewQUICDialerWithResolver(
 			netxlite.NewQUICListener(), log.Log,
-			netxlite.NewResolverSystem(log.Log),
+			netxlite.NewResolverStdlib(log.Log),
 		)
 		ctx := context.Background()
 		sess, err := d.DialContext(

internal/netxlite/legacy.go

@@ -32,6 +32,7 @@ type (
 	TLSHandshakerLogger       = tlsHandshakerLogger
 	DialerSystem              = dialerSystem
 	TLSDialerLegacy           = tlsDialer
+	UserAgentTransport        = httpUserAgentTransport
 )
 
 // ResolverLegacy performs domain name resolutions.

internal/netxlite/quic_test.go

@@ -256,7 +256,7 @@ func TestQUICDialerResolver(t *testing.T) {
 		t.Run("on success", func(t *testing.T) {
 			tlsConfig := &tls.Config{}
 			dialer := &quicDialerResolver{
-				Resolver: NewResolverSystem(log.Log),
+				Resolver: NewResolverStdlib(log.Log),
 				Dialer: &quicDialerQUICGo{
 					QUICListener: &quicListenerStdlib{},
 				}}
@@ -275,7 +275,7 @@ func TestQUICDialerResolver(t *testing.T) {
 		t.Run("with missing port", func(t *testing.T) {
 			tlsConfig := &tls.Config{}
 			dialer := &quicDialerResolver{
-				Resolver: NewResolverSystem(log.Log),
+				Resolver: NewResolverStdlib(log.Log),
 				Dialer:   &quicDialerQUICGo{}}
 			sess, err := dialer.DialContext(
 				context.Background(), "udp", "www.google.com",
@@ -312,7 +312,7 @@ func TestQUICDialerResolver(t *testing.T) {
 			// to establish a connection leads to a failure
 			tlsConf := &tls.Config{}
 			dialer := &quicDialerResolver{
-				Resolver: NewResolverSystem(log.Log),
+				Resolver: NewResolverStdlib(log.Log),
 				Dialer: &quicDialerQUICGo{
 					QUICListener: &quicListenerStdlib{},
 				}}
@@ -336,7 +336,7 @@ func TestQUICDialerResolver(t *testing.T) {
 			var gotTLSConfig *tls.Config
 			tlsConfig := &tls.Config{}
 			dialer := &quicDialerResolver{
-				Resolver: NewResolverSystem(log.Log),
+				Resolver: NewResolverStdlib(log.Log),
 				Dialer: &mocks.QUICDialer{
 					MockDialContext: func(ctx context.Context, network, address string,
 						tlsConfig *tls.Config, quicConfig *quic.Config) (quic.EarlySession, error) {

internal/netxlite/resolver.go

@@ -25,7 +25,7 @@ type Resolver interface {
 	CloseIdleConnections()
 }
 
-// NewResolverSystem creates a new resolver using system
+// NewResolverStdlib creates a new resolver using system
 // facilities for resolving domain names (e.g., getaddrinfo).
 //
 // The resolver will provide the following guarantees:
@@ -41,7 +41,7 @@ type Resolver interface {
 //
 // 5. enforces reasonable timeouts (
 // see https://github.com/ooni/probe/issues/1726).
-func NewResolverSystem(logger Logger) Resolver {
+func NewResolverStdlib(logger Logger) Resolver {
 	return &resolverIDNA{
 		Resolver: &resolverLogger{
 			Resolver: &resolverShortCircuitIPAddr{

internal/netxlite/resolver_test.go

@@ -16,7 +16,7 @@ import (
 )
 
 func TestNewResolverSystem(t *testing.T) {
-	resolver := NewResolverSystem(log.Log)
+	resolver := NewResolverStdlib(log.Log)
 	idna := resolver.(*resolverIDNA)
 	logger := idna.Resolver.(*resolverLogger)
 	if logger.Logger != log.Log {