feat(netxlite): add flexible HTTP transport factory

internal/netxlite/httpfactory.go

@@ -1,12 +1,89 @@
 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'll 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.
+//
+// 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.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) {
+			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 uses the default set in github.com/ooni/oohttp, i.e., false.
+func HTTPTransportOptionDisableCompression(value bool) HTTPTransportOption {
+	return func(txp *oohttp.Transport) {
+		txp.DisableCompression = value
+	}
 }

internal/netxlite/httpfactory_test.go

@@ -0,0 +1,166 @@
+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
+		// by applying the expected changes to a cloned default transport
+		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",
+				"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")
+		}
+	})
+
+	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})
+}