chore(netxlite): isolate and annotate quirky functions (#1269)

This diff isolates and annotates netxlite quirky functions such that
ooni/probe#2534 will be easier.

This work is also useful to ooni/probe#2531.

While there, commit workaround for issue
ooni/probe#2535.

internal/cmd/apitool/main.go

@@ -26,6 +26,8 @@ import (
 )
 
 func newclient() probeservices.Client {
+	// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPTransportStdlib has QUIRKS but we
+	// don't actually care about those QUIRKS in this context
 	txp := netxlite.NewHTTPTransportStdlib(log.Log)
 	ua := fmt.Sprintf("apitool/%s ooniprobe-engine/%s", version.Version, version.Version)
 	return probeservices.Client{

internal/cmd/oohelper/oohelper.go

@@ -29,6 +29,7 @@ func init() {
 	// puzzling https://github.com/ooni/probe/issues/1409 issue.
 	const resolverURL = "https://8.8.8.8/dns-query"
 	resolver = netxlite.NewParallelDNSOverHTTPSResolver(log.Log, resolverURL)
+	// TODO(https://github.com/ooni/probe/issues/2534): the NewHTTPClientWithResolver func has QUIRKS but we don't care.
 	httpClient = netxlite.NewHTTPClientWithResolver(log.Log, resolver)
 }
 

internal/dslx/httptcp.go

@@ -26,6 +26,9 @@ type httpTransportTCPFunc struct{}
 // Apply implements Func
 func (f *httpTransportTCPFunc) Apply(
 	ctx context.Context, input *TCPConnection) *Maybe[*HTTPTransport] {
+	// TODO(https://github.com/ooni/probe/issues/2534): here we're using the QUIRKY netxlite.NewHTTPTransport
+	// function, but we can probably avoid using it, given that this code is
+	// not using tracing and does not care about those quirks.
 	httpTransport := netxlite.NewHTTPTransport(
 		input.Logger,
 		netxlite.NewSingleUseDialer(input.Conn),

internal/dslx/httptls.go

@@ -26,6 +26,9 @@ type httpTransportTLSFunc struct{}
 // Apply implements Func.
 func (f *httpTransportTLSFunc) Apply(
 	ctx context.Context, input *TLSConnection) *Maybe[*HTTPTransport] {
+	// TODO(https://github.com/ooni/probe/issues/2534): here we're using the QUIRKY netxlite.NewHTTPTransport
+	// function, but we can probably avoid using it, given that this code is
+	// not using tracing and does not care about those quirks.
 	httpTransport := netxlite.NewHTTPTransport(
 		input.Logger,
 		netxlite.NewNullDialer(),

internal/enginelocate/iplookup.go

@@ -87,6 +87,8 @@ func (c ipLookupClient) doWithCustomFunc(
 	// Implementation note: we MUST use an HTTP client that we're
 	// sure IS NOT using any proxy. To this end, we construct a
 	// client ourself that we know is not proxied.
+	// TODO(https://github.com/ooni/probe/issues/2534): the NewHTTPTransportWithResolver has QUIRKS but
+	// we don't care about them in this context
 	txp := netxlite.NewHTTPTransportWithResolver(c.Logger, c.Resolver)
 	clnt := &http.Client{Transport: txp}
 	defer clnt.CloseIdleConnections()

internal/enginenetx/http.go

@@ -51,6 +51,9 @@ func NewHTTPTransport(
 	dialer = netxlite.MaybeWrapWithProxyDialer(dialer, proxyURL)
 	handshaker := netxlite.NewTLSHandshakerStdlib(logger)
 	tlsDialer := netxlite.NewTLSDialer(dialer, handshaker)
+	// TODO(https://github.com/ooni/probe/issues/2534): here we're using the QUIRKY netxlite.NewHTTPTransport
+	// function, but we can probably avoid using it, given that this code is
+	// not using tracing and does not care about those quirks.
 	txp := netxlite.NewHTTPTransport(logger, dialer, tlsDialer)
 	txp = bytecounter.WrapHTTPTransport(txp, counter)
 	return &HTTPTransport{txp}

internal/engineresolver/factory.go

@@ -86,6 +86,9 @@ func newChildResolverHTTPS(
 		)
 		thx := netxlite.NewTLSHandshakerStdlib(logger)
 		tlsDialer := netxlite.NewTLSDialer(dialer, thx)
+		// TODO(https://github.com/ooni/probe/issues/2534): here we're using the QUIRKY netxlite.NewHTTPTransport
+		// function, but we can probably avoid using it, given that this code is
+		// not using tracing and does not care about those quirks.
 		txp = netxlite.NewHTTPTransport(logger, dialer, tlsDialer)
 	case true:
 		txp = netxlite.NewHTTP3TransportStdlib(logger)

internal/experiment/ndt7/ndt7.go

@@ -77,6 +77,10 @@ type Measurer struct {
 
 func (m *Measurer) discover(
 	ctx context.Context, sess model.ExperimentSession) (*mlablocatev2.NDT7Result, error) {
+	// Implementation note: here we cannot use the session's HTTP client because it MAY be proxied
+	// and instead we need to connect directly to M-Lab's locate service.
+	//
+	// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS and maybe here it doesn't matter?
 	httpClient := netxlite.NewHTTPClientStdlib(sess.Logger())
 	defer httpClient.CloseIdleConnections()
 	client := mlablocatev2.NewClient(httpClient, sess.Logger(), sess.UserAgent())

internal/experiment/webconnectivitylte/cleartextflow.go

@@ -137,6 +137,9 @@ func (t *CleartextFlow) Run(parentCtx context.Context, index int64) error {
 	}
 
 	// create HTTP transport
+	// TODO(https://github.com/ooni/probe/issues/2534): here we're using the QUIRKY netxlite.NewHTTPTransport
+	// function, but we can probably avoid using it, given that this code is
+	// not using tracing and does not care about those quirks.
 	httpTransport := netxlite.NewHTTPTransport(
 		t.Logger,
 		netxlite.NewSingleUseDialer(tcpConn),

internal/experiment/webconnectivitylte/secureflow.go

@@ -171,6 +171,9 @@ func (t *SecureFlow) Run(parentCtx context.Context, index int64) error {
 	}
 
 	// create HTTP transport
+	// TODO(https://github.com/ooni/probe/issues/2534): here we're using the QUIRKY netxlite.NewHTTPTransport
+	// function, but we can probably avoid using it, given that this code is
+	// not using tracing and does not care about those quirks.
 	httpTransport := netxlite.NewHTTPTransport(
 		t.Logger,
 		netxlite.NewNullDialer(),

internal/experiment/webconnectivityqa/badssl_test.go

@@ -38,6 +38,7 @@ func TestBadSSLConditions(t *testing.T) {
 			tc.testCase.Configure(env)
 
 			env.Do(func() {
+				// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 				client := netxlite.NewHTTPClientStdlib(log.Log)
 				req := runtimex.Try1(http.NewRequest("GET", tc.testCase.Input, nil))
 				resp, err := client.Do(req)

internal/experiment/webconnectivityqa/httpdiff_test.go

@@ -25,6 +25,7 @@ func TestHTTPDiffWithConsistentDNS(t *testing.T) {
 			tc.Configure(env)
 
 			env.Do(func() {
+				// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 				client := netxlite.NewHTTPClientStdlib(log.Log)
 				req := runtimex.Try1(http.NewRequest("GET", "http://www.example.com/", nil))
 				resp, err := client.Do(req)
@@ -58,6 +59,7 @@ func TestHTTPDiffWithInconsistentDNS(t *testing.T) {
 
 			env.Do(func() {
 				t.Run("there is blockpage spoofing", func(t *testing.T) {
+					// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 					client := netxlite.NewHTTPClientStdlib(log.Log)
 					req := runtimex.Try1(http.NewRequest("GET", "http://www.example.com/", nil))
 					resp, err := client.Do(req)

internal/experiment/webconnectivityqa/redirect_test.go

@@ -66,6 +66,7 @@ func TestRedirectWithConsistentDNSAndThenConnectionReset(t *testing.T) {
 
 				for _, URL := range urls {
 					t.Run(fmt.Sprintf("for URL %s", URL), func(t *testing.T) {
+						// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 						client := netxlite.NewHTTPClientStdlib(log.Log)
 						req := runtimex.Try1(http.NewRequest("GET", URL, nil))
 						resp, err := client.Do(req)
@@ -140,6 +141,7 @@ func TestRedirectWithConsistentDNSAndThenEOF(t *testing.T) {
 
 				for _, URL := range urls {
 					t.Run(fmt.Sprintf("for URL %s", URL), func(t *testing.T) {
+						// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 						client := netxlite.NewHTTPClientStdlib(log.Log)
 						req := runtimex.Try1(http.NewRequest("GET", URL, nil))
 						resp, err := client.Do(req)
@@ -176,6 +178,7 @@ func TestRedirectWithConsistentDNSAndThenTimeout(t *testing.T) {
 					t.Run(fmt.Sprintf("for URL %s", URL), func(t *testing.T) {
 						ctx, cancel := context.WithTimeout(context.Background(), time.Second)
 						defer cancel()
+						// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 						client := netxlite.NewHTTPClientStdlib(log.Log)
 						req := runtimex.Try1(http.NewRequestWithContext(ctx, "GET", URL, nil))
 						resp, err := client.Do(req)

internal/experiment/webconnectivityqa/run.go

@@ -34,6 +34,7 @@ func RunTestCase(measurer model.ExperimentMeasurer, tc *TestCase) error {
 	var err error
 	env.Do(func() {
 		// create an HTTP client inside the env.Do function so we're using netem
+		// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 		httpClient := netxlite.NewHTTPClientStdlib(prefixLogger)
 		arguments := &model.ExperimentArgs{
 			Callbacks:   model.NewPrinterCallbacks(prefixLogger),

internal/experiment/webconnectivityqa/tlsblocking_test.go

@@ -23,6 +23,7 @@ func TestBlockingTLSConnectionResetWithConsistentDNS(t *testing.T) {
 		urls := []string{"https://www.example.com/", "https://www.example.com/"}
 		for _, URL := range urls {
 			t.Run(fmt.Sprintf("for %s", URL), func(t *testing.T) {
+				// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 				client := netxlite.NewHTTPClientStdlib(log.Log)
 				req, err := http.NewRequest("GET", URL, nil)
 				if err != nil {
@@ -51,6 +52,7 @@ func TestBlockingTLSConnectionResetWithInconsistentDNS(t *testing.T) {
 		urls := []string{"https://www.example.com/", "https://www.example.com/"}
 		for _, URL := range urls {
 			t.Run(fmt.Sprintf("for %s", URL), func(t *testing.T) {
+				// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 				client := netxlite.NewHTTPClientStdlib(log.Log)
 				req, err := http.NewRequest("GET", URL, nil)
 				if err != nil {

internal/netemx/example_test.go

@@ -66,6 +66,7 @@ func Example_dpiRule() {
 		reso := netxlite.NewStdlibResolver(model.DiscardLogger)
 
 		// create the HTTP client
+		// TODO(https://github.com/ooni/probe/issues/2534): the NewHTTPClientWithResolver func has QUIRKS but we don't care.
 		client := netxlite.NewHTTPClientWithResolver(model.DiscardLogger, reso)
 
 		// create the HTTP request
@@ -310,6 +311,7 @@ func Example_exampleWebServerWithInternetScenario() {
 	defer env.Close()
 
 	env.Do(func() {
+		// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 		client := netxlite.NewHTTPClientStdlib(log.Log)
 
 		req, err := http.NewRequest("GET", "https://www.example.com/", nil)
@@ -389,6 +391,7 @@ func Example_ooniAPIWithInternetScenario() {
 	defer env.Close()
 
 	env.Do(func() {
+		// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 		client := netxlite.NewHTTPClientStdlib(log.Log)
 
 		req, err := http.NewRequest("GET", "https://api.ooni.io/api/v1/test-helpers", nil)
@@ -419,6 +422,7 @@ func Example_oohelperdWithInternetScenario() {
 	defer env.Close()
 
 	env.Do(func() {
+		// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 		client := netxlite.NewHTTPClientStdlib(log.Log)
 		thRequest := []byte(`{"http_request": "https://www.example.com/","http_request_headers":{},"tcp_connect":["93.184.216.34:443"]}`)
 
@@ -452,6 +456,7 @@ func Example_ubuntuGeoIPWithInternetScenario() {
 	defer env.Close()
 
 	env.Do(func() {
+		// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 		client := netxlite.NewHTTPClientStdlib(log.Log)
 
 		req, err := http.NewRequest("GET", "https://geoip.ubuntu.com/lookup", nil)
@@ -482,6 +487,7 @@ func Example_examplePublicBlockpage() {
 	defer env.Close()
 
 	env.Do(func() {
+		// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 		client := netxlite.NewHTTPClientStdlib(log.Log)
 
 		req, err := http.NewRequest("GET", "http://"+netemx.AddressPublicBlockpage+"/", nil)
@@ -523,6 +529,8 @@ func Example_exampleURLShortener() {
 	defer env.Close()
 
 	env.Do(func() {
+		// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPTransportStdlib has QUIRKS but we
+		// don't actually care about those QUIRKS in this context
 		client := netxlite.NewHTTPTransportStdlib(log.Log)
 
 		req, err := http.NewRequest("GET", "https://bit.ly/21645", nil)

internal/netemx/http_test.go

@@ -25,6 +25,7 @@ func TestHTTPCleartextServerFactory(t *testing.T) {
 	env.AddRecordToAllResolvers("www.example.com", "", AddressWwwExampleCom)
 
 	env.Do(func() {
+		// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 		client := netxlite.NewHTTPClientStdlib(log.Log)
 		req := runtimex.Try1(http.NewRequest("GET", "http://www.example.com/", nil))
 		resp, err := client.Do(req)

internal/netemx/https_test.go

@@ -27,6 +27,7 @@ func TestHTTPSecureServerFactory(t *testing.T) {
 		env.AddRecordToAllResolvers("www.example.com", "", AddressWwwExampleCom)
 
 		env.Do(func() {
+			// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 			client := netxlite.NewHTTPClientStdlib(log.Log)
 			req := runtimex.Try1(http.NewRequest("GET", "https://www.example.com/", nil))
 			resp, err := client.Do(req)
@@ -66,6 +67,7 @@ func TestHTTPSecureServerFactory(t *testing.T) {
 		env.AddRecordToAllResolvers("www.example.com", "", AddressWwwExampleCom)
 
 		env.Do(func() {
+			// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 			client := netxlite.NewHTTPClientStdlib(log.Log)
 			req := runtimex.Try1(http.NewRequest("GET", "https://www.example.com/", nil))
 			resp, err := client.Do(req)

internal/netemx/oohelperd.go

@@ -49,6 +49,8 @@ func (f *OOHelperDFactory) NewHandler(env NetStackServerFactoryEnv, unet *netem.
 		cookieJar, _ := cookiejar.New(&cookiejar.Options{
 			PublicSuffixList: publicsuffix.List,
 		})
+		// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPTransportStdlib is QUIRKY but we probably
+		// don't care about using a QUIRKY function here
 		return &http.Client{
 			Transport:     netx.NewHTTPTransportStdlib(logger),
 			CheckRedirect: nil,

internal/netemx/oohelperd_test.go

@@ -33,6 +33,7 @@ func TestOOHelperDHandler(t *testing.T) {
 
 		//log.SetLevel(log.DebugLevel)
 
+		// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 		httpClient := netxlite.NewHTTPClientStdlib(log.Log)
 
 		req, err := http.NewRequest(http.MethodPost, "https://0.th.ooni.org/", bytes.NewReader(thReqRaw))

internal/netemx/qaenv_test.go

@@ -92,6 +92,7 @@ func TestQAEnv(t *testing.T) {
 		env.Do(func() {
 			// create client, which will use the underlying client stack's
 			// DialContext method to dial connections
+			// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 			client := netxlite.NewHTTPClientStdlib(model.DiscardLogger)
 
 			// create request using a domain that has been configured in the
@@ -211,6 +212,7 @@ func TestQAEnv(t *testing.T) {
 		env.Do(func() {
 			// create client, which will use the underlying client stack's
 			// DialContext method to dial connections
+			// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 			client := netxlite.NewHTTPClientStdlib(model.DiscardLogger)
 
 			// create the request
@@ -257,6 +259,7 @@ func TestQAEnv(t *testing.T) {
 		env.Do(func() {
 			// create client, which will use the underlying client stack's
 			// DialContext method to dial connections
+			// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 			client := netxlite.NewHTTPClientStdlib(model.DiscardLogger)
 
 			// create the request

internal/netxlite/dnsoverhttps_test.go

@@ -15,6 +15,7 @@ import (
 
 func TestNewDNSOverHTTPSTransport(t *testing.T) {
 	const URL = "https://1.1.1.1/dns-query"
+	// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here
 	clnt := NewHTTPClientStdlib(model.DiscardLogger)
 	txp := NewDNSOverHTTPSTransport(clnt, URL)
 	ew := txp.(*dnsTransportErrWrapper)
@@ -29,6 +30,8 @@ func TestNewDNSOverHTTPSTransport(t *testing.T) {
 
 func TestNewDNSOverHTTPSTransportWithHTTPTransport(t *testing.T) {
 	const URL = "https://1.1.1.1/dns-query"
+	// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPTransportStdlib has QUIRKS but we
+	// don't actually care about those QUIRKS in this context
 	httpTxp := NewHTTPTransportStdlib(model.DiscardLogger)
 	txp := NewDNSOverHTTPSTransportWithHTTPTransport(httpTxp, URL)
 	ew := txp.(*dnsTransportErrWrapper)

internal/netxlite/httpfactory.go

@@ -0,0 +1,12 @@
+package netxlite
+
+import (
+	"net/http"
+
+	"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})
+}