feat(netxlite): add flexible HTTP transport factory (#1270)

Now that we've clearly labeled and packaged technical debt, we can copy
existing technical-debt-ridden code and modify it to obtain a flexible
factory for creating HTTP transports, `NewHTTPTransportWithOptions`.

In particular, this factory uses sensible defaults for measuring and
there are options that you can pass it to customize details such as the
backend proxy that we previously unconditionally configured.

More in detail, this is a side-by-side comparison between new code's
defaults and old code:

| Setting Name | httpquirks.go (old code) | httpfactory.go (new code) |
| ------------------- | ------------------------ |
-------------------------- |
| .Proxy | nil | nil |
| .MaxConnsPerHost | 1 | ooni/oohttp's default |
| .DisableCompression | true | true |
| .ForceAttemptHTTP2 | true | true |

So, basically, the biggest change is that we've removed the limitation
of the max number of connections per host being set to 1. In any case,
the new code allows to configure each of these fields.

This factory will be the starting point for having custom network
functions for the engine in line with
ooni/probe#2531.

While there, acknowledge that `NewHTTPClient` contains technical debt
because it does unconditionally disable cookies, to document that and
move it inside of the proper file (`httpquirks.go`).

internal/netxlite/httpfactory.go

@@ -1,12 +1,96 @@
 package netxlite
 
 import (
-	"net/http"
+	"net/url"
 
+	oohttp "github.com/ooni/oohttp"
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
-// NewHTTPClient creates a new, wrapped HTTPClient using the given transport.
-func NewHTTPClient(txp model.HTTPTransport) model.HTTPClient {
-	return WrapHTTPClient(&http.Client{Transport: txp})
+// HTTPTransportOption is an initialization option for [NewHTTPTransport].
+type HTTPTransportOption func(txp *oohttp.Transport)
+
+// NewHTTPTransport is the high-level factory to create a [model.HTTPTransport] using
+// github.com/ooni/oohttp as the HTTP library with HTTP/1.1 and HTTP2 support.
+//
+// This transport is suitable for HTTP2 and HTTP/1.1 using any TLS
+// library, including, e.g., github.com/ooni/oocrypto.
+//
+// This factory clones the default github.com/ooni/oohttp transport and
+// configures the provided dialer and TLS dialer by setting the .DialContext
+// and .DialTLSContext fields of the transport. We also wrap the provided
+// dialers to address https://github.com/ooni/probe/issues/1609.
+//
+// Apart from that, the only non-default options set by this factory are these:
+//
+// 1. the .Proxy field is set to nil, so by default we DO NOT honour the
+// HTTP_PROXY and HTTPS_PROXY environment variables, which is required if
+// we want to use this code for measuring;
+//
+// 2. the .ForceAttemptHTTP2 field is set to true;
+//
+// 3. the .DisableCompression field is set to true, again required if we
+// want to use this code for measuring, and we should make sure the defaults
+// we're using are suitable for measuring, since the impact of making a
+// mistake in measuring code is a data quality issue 😅.
+//
+// The returned transport supports logging and error wrapping because
+// internally this function calls [WrapHTTPTransport] before we return.
+//
+// This factory is the RECOMMENDED way of creating a [model.HTTPTransport].
+func NewHTTPTransportWithOptions(logger model.Logger,
+	dialer model.Dialer, tlsDialer model.TLSDialer, options ...HTTPTransportOption) model.HTTPTransport {
+	// Using oohttp to support any TLS library.
+	txp := oohttp.DefaultTransport.(*oohttp.Transport).Clone()
+
+	// This wrapping ensures that we always have a timeout when we
+	// are using HTTP; see https://github.com/ooni/probe/issues/1609.
+	dialer = &httpDialerWithReadTimeout{dialer}
+	txp.DialContext = dialer.DialContext
+	tlsDialer = &httpTLSDialerWithReadTimeout{tlsDialer}
+	txp.DialTLSContext = tlsDialer.DialTLSContext
+
+	// As documented, disable proxies and force HTTP/2
+	txp.DisableCompression = true
+	txp.Proxy = nil
+	txp.ForceAttemptHTTP2 = true
+
+	// Apply all the required options
+	for _, option := range options {
+		option(txp)
+	}
+
+	// Return a fully wrapped HTTP transport
+	return WrapHTTPTransport(logger, &httpTransportConnectionsCloser{
+		HTTPTransport: &httpTransportStdlib{&oohttp.StdlibTransport{Transport: txp}},
+		Dialer:        dialer,
+		TLSDialer:     tlsDialer,
+	})
+}
+
+// HTTPTransportOptionProxyURL configures the transport to use the given proxyURL
+// or disables proxying (already the default) if the proxyURL is nil.
+func HTTPTransportOptionProxyURL(proxyURL *url.URL) HTTPTransportOption {
+	return func(txp *oohttp.Transport) {
+		txp.Proxy = func(r *oohttp.Request) (*url.URL, error) {
+			// "If Proxy is nil or returns a nil *URL, no proxy is used."
+			return proxyURL, nil
+		}
+	}
+}
+
+// HTTPTransportOptionMaxConnsPerHost configures the .MaxConnPerHosts field, which
+// otherwise uses the default set in github.com/ooni/oohttp.
+func HTTPTransportOptionMaxConnsPerHost(value int) HTTPTransportOption {
+	return func(txp *oohttp.Transport) {
+		txp.MaxConnsPerHost = value
+	}
+}
+
+// HTTPTransportOptionDisableCompression configures the .DisableCompression field, which
+// otherwise is set to true, so that this code is ready for measuring out of the box.
+func HTTPTransportOptionDisableCompression(value bool) HTTPTransportOption {
+	return func(txp *oohttp.Transport) {
+		txp.DisableCompression = value
+	}
 }

internal/netxlite/httpfactory_test.go

@@ -0,0 +1,169 @@
+package netxlite
+
+import (
+	"crypto/tls"
+	"net/url"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	oohttp "github.com/ooni/oohttp"
+	"github.com/ooni/probe-cli/v3/internal/mocks"
+	"github.com/ooni/probe-cli/v3/internal/model"
+)
+
+func TestNewHTTPTransportWithOptions(t *testing.T) {
+
+	t.Run("make sure that we get the correct types and settings", func(t *testing.T) {
+		expectDialer := &mocks.Dialer{}
+		expectTLSDialer := &mocks.TLSDialer{}
+		expectLogger := model.DiscardLogger
+		txp := NewHTTPTransportWithOptions(expectLogger, expectDialer, expectTLSDialer)
+
+		// undo the results of the netxlite.WrapTransport function
+		txpLogger := txp.(*httpTransportLogger)
+		if txpLogger.Logger != expectLogger {
+			t.Fatal("invalid logger")
+		}
+		txpErrWrapper := txpLogger.HTTPTransport.(*httpTransportErrWrapper)
+
+		// make sure we correctly configured dialer and TLS dialer
+		txpCloser := txpErrWrapper.HTTPTransport.(*httpTransportConnectionsCloser)
+		timeoutDialer := txpCloser.Dialer.(*httpDialerWithReadTimeout)
+		childDialer := timeoutDialer.Dialer
+		if childDialer != expectDialer {
+			t.Fatal("invalid dialer")
+		}
+		timeoutTLSDialer := txpCloser.TLSDialer.(*httpTLSDialerWithReadTimeout)
+		childTLSDialer := timeoutTLSDialer.TLSDialer
+		if childTLSDialer != expectTLSDialer {
+			t.Fatal("invalid TLS dialer")
+		}
+
+		// make sure there's the stdlib adapter
+		stdlibAdapter := txpCloser.HTTPTransport.(*httpTransportStdlib)
+		oohttpStdlibAdapter := stdlibAdapter.StdlibTransport
+		underlying := oohttpStdlibAdapter.Transport
+
+		// now let's check that everything is configured as intended
+		expectedTxp := oohttp.DefaultTransport.(*oohttp.Transport).Clone()
+		diff := cmp.Diff(
+			expectedTxp,
+			underlying,
+			cmpopts.IgnoreUnexported(oohttp.Transport{}),
+			cmpopts.IgnoreUnexported(tls.Config{}),
+			cmpopts.IgnoreFields(
+				oohttp.Transport{},
+				"DialContext",
+				"DialTLSContext",
+				"DisableCompression",
+				"Proxy",
+				"ForceAttemptHTTP2",
+			),
+		)
+		if diff != "" {
+			t.Fatal(diff)
+		}
+
+		// finish checking by explicitly inspecting the fields we modify
+		if underlying.DialContext == nil {
+			t.Fatal("expected non-nil .DialContext")
+		}
+		if underlying.DialTLSContext == nil {
+			t.Fatal("expected non-nil .DialTLSContext")
+		}
+		if underlying.Proxy != nil {
+			t.Fatal("expected nil .Proxy")
+		}
+		if !underlying.ForceAttemptHTTP2 {
+			t.Fatal("expected true .ForceAttemptHTTP2")
+		}
+		if !underlying.DisableCompression {
+			t.Fatal("expected true .DisableCompression")
+		}
+	})
+
+	unwrap := func(txp model.HTTPTransport) *oohttp.Transport {
+		txpLogger := txp.(*httpTransportLogger)
+		txpErrWrapper := txpLogger.HTTPTransport.(*httpTransportErrWrapper)
+		txpCloser := txpErrWrapper.HTTPTransport.(*httpTransportConnectionsCloser)
+		stdlibAdapter := txpCloser.HTTPTransport.(*httpTransportStdlib)
+		oohttpStdlibAdapter := stdlibAdapter.StdlibTransport
+		return oohttpStdlibAdapter.Transport
+	}
+
+	t.Run("make sure HTTPTransportOptionProxyURL is WAI", func(t *testing.T) {
+		runWithURL := func(expectedURL *url.URL) {
+			expectDialer := &mocks.Dialer{}
+			expectTLSDialer := &mocks.TLSDialer{}
+			expectLogger := model.DiscardLogger
+			txp := NewHTTPTransportWithOptions(
+				expectLogger,
+				expectDialer,
+				expectTLSDialer,
+				HTTPTransportOptionProxyURL(expectedURL),
+			)
+			underlying := unwrap(txp)
+			if underlying.Proxy == nil {
+				t.Fatal("expected non-nil .Proxy")
+			}
+			got, err := underlying.Proxy(&oohttp.Request{})
+			if err != nil {
+				t.Fatal(err)
+			}
+			if got != expectedURL {
+				t.Fatal("not the expected URL")
+			}
+		}
+
+		runWithURL(&url.URL{})
+
+		runWithURL(nil)
+	})
+
+	t.Run("make sure HTTPTransportOptionMaxConnsPerHost is WAI", func(t *testing.T) {
+		runWithValue := func(expectedValue int) {
+			expectDialer := &mocks.Dialer{}
+			expectTLSDialer := &mocks.TLSDialer{}
+			expectLogger := model.DiscardLogger
+			txp := NewHTTPTransportWithOptions(
+				expectLogger,
+				expectDialer,
+				expectTLSDialer,
+				HTTPTransportOptionMaxConnsPerHost(expectedValue),
+			)
+			underlying := unwrap(txp)
+			got := underlying.MaxConnsPerHost
+			if got != expectedValue {
+				t.Fatal("not the expected value")
+			}
+		}
+
+		runWithValue(100)
+
+		runWithValue(10)
+	})
+
+	t.Run("make sure HTTPTransportDisableCompression is WAI", func(t *testing.T) {
+		runWithValue := func(expectedValue bool) {
+			expectDialer := &mocks.Dialer{}
+			expectTLSDialer := &mocks.TLSDialer{}
+			expectLogger := model.DiscardLogger
+			txp := NewHTTPTransportWithOptions(
+				expectLogger,
+				expectDialer,
+				expectTLSDialer,
+				HTTPTransportOptionDisableCompression(expectedValue),
+			)
+			underlying := unwrap(txp)
+			got := underlying.DisableCompression
+			if got != expectedValue {
+				t.Fatal("not the expected value")
+			}
+		}
+
+		runWithValue(true)
+
+		runWithValue(false)
+	})
+}

internal/netxlite/httpquirks.go

@@ -5,8 +5,12 @@ package netxlite
 //
 // Ideally, we should not modify this code or apply minimal and obvious changes.
 //
+// TODO(https://github.com/ooni/probe/issues/2534)
+//
 
 import (
+	"net/http"
+
 	oohttp "github.com/ooni/oohttp"
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
@@ -129,3 +133,12 @@ func NewHTTPClientStdlib(logger model.DebugLogger) model.HTTPClient {
 func NewHTTPClientWithResolver(logger model.Logger, reso model.Resolver) model.HTTPClient {
 	return NewHTTPClient(NewHTTPTransportWithResolver(logger, reso))
 }
+
+// NewHTTPClient creates a new, wrapped HTTPClient using the given transport.
+//
+// This function behavior is QUIRKY because it does not configure a cookie jar, which
+// is probably not the right thing to do in many cases, but legacy code MAY depend
+// on this behavior. TODO(https://github.com/ooni/probe/issues/2534).
+func NewHTTPClient(txp model.HTTPTransport) model.HTTPClient {
+	return WrapHTTPClient(&http.Client{Transport: txp})
+}