refactor(oohelperd): depend on netxlite.Netx only (#1466)

This diff modifies netxlite such that the oohelperd only depends on a netxlite.Netx instance. The overall goal here is to refactor `oohelperd` to only depend on `netxlite.Netx` such that we can remove the code duplication between how we instantiate `oohelperd.Handler` in `oohelperd` and how we instantiate it inside `netemx`. In turn, by doing this, we would ensure we have the same `oohelperd` behavior for QA and production. In turn, with this guarantee, we can write QA tests that ensure we're correctly dealing with 127.0.0.1. The reference issue is ooni/probe#1517.
ooni · Jan 24, 2024 · 9f66878 · 9f66878
1 parent 70861a6
commit 9f66878
Show file tree

Hide file tree

Showing 10 changed files with 28 additions and 25 deletions.
diff --git a/internal/cmd/oohelper/oohelper.go b/internal/cmd/oohelper/oohelper.go
@@ -31,7 +31,7 @@ func init() {
 	netx := &netxlite.Netx{}
 	resolver = netx.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)
+	httpClient = netxlite.NewHTTPClientWithResolver(netx, log.Log, resolver)
 }
 
 func main() {

diff --git a/internal/enginelocate/iplookup.go b/internal/enginelocate/iplookup.go
@@ -97,7 +97,8 @@ func (c ipLookupClient) doWithCustomFunc(
 	// 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)
+	netx := &netxlite.Netx{}
+	txp := netxlite.NewHTTPTransportWithResolver(netx, c.Logger, c.Resolver)
 	clnt := &http.Client{Transport: txp}
 	defer clnt.CloseIdleConnections()
 	ip, err := fn(ctx, clnt, c.Logger, c.UserAgent, c.Resolver)

diff --git a/internal/netemx/example_test.go b/internal/netemx/example_test.go
@@ -85,7 +85,7 @@ func Example_dpiRule() {
 
 		// 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)
+		client := netxlite.NewHTTPClientWithResolver(netx, model.DiscardLogger, reso)
 
 		// create the HTTP request
 		req := runtimex.Try1(http.NewRequest("GET", "https://example.com", nil))

diff --git a/internal/netemx/http3_test.go b/internal/netemx/http3_test.go
@@ -28,7 +28,7 @@ func TestHTTP3ServerFactory(t *testing.T) {
 
 		env.Do(func() {
 			netx := &netxlite.Netx{}
-			client := netxlite.NewHTTP3ClientWithResolver(log.Log, netx.NewStdlibResolver(log.Log))
+			client := netxlite.NewHTTP3ClientWithResolver(netx, log.Log, netx.NewStdlibResolver(log.Log))
 			req := runtimex.Try1(http.NewRequest("GET", "https://www.example.com/", nil))
 			resp, err := client.Do(req)
 			if err != nil {

diff --git a/internal/netemx/oohelperd.go b/internal/netemx/oohelperd.go
@@ -45,8 +45,8 @@ func (f *OOHelperDFactory) NewHandler(env NetStackServerFactoryEnv, unet *netem.
 
 	handler.NewHTTPClient = func(logger model.Logger) model.HTTPClient {
 		return oohelperd.NewHTTPClientWithTransportFactory(
-			logger,
-			func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport {
+			netx, logger,
+			func(netx *netxlite.Netx, dl model.DebugLogger, r model.Resolver) model.HTTPTransport {
 				dialer := netx.NewDialerWithResolver(dl, r)
 				tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl))
 				// TODO(https://github.com/ooni/probe/issues/2534): NewHTTPTransport is QUIRKY but
@@ -58,8 +58,8 @@ func (f *OOHelperDFactory) NewHandler(env NetStackServerFactoryEnv, unet *netem.
 
 	handler.NewHTTP3Client = func(logger model.Logger) model.HTTPClient {
 		return oohelperd.NewHTTPClientWithTransportFactory(
-			logger,
-			func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport {
+			netx, logger,
+			func(netx *netxlite.Netx, dl model.DebugLogger, r model.Resolver) model.HTTPTransport {
 				qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r)
 				return netxlite.NewHTTP3Transport(dl, qd, nil)
 			},

diff --git a/internal/netxlite/http3.go b/internal/netxlite/http3.go
@@ -73,14 +73,13 @@ func (netx *Netx) NewHTTP3TransportStdlib(logger model.DebugLogger) model.HTTPTr
 
 // NewHTTPTransportWithResolver creates a new HTTPTransport using http3
 // that uses the given logger and the given resolver.
-func NewHTTP3TransportWithResolver(logger model.DebugLogger, reso model.Resolver) model.HTTPTransport {
-	netx := &Netx{}
+func NewHTTP3TransportWithResolver(netx *Netx, logger model.DebugLogger, reso model.Resolver) model.HTTPTransport {
 	qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), logger, reso)
 	return NewHTTP3Transport(logger, qd, nil)
 }
 
 // NewHTTP3ClientWithResolver creates a new HTTP3Transport using the
 // given resolver and then from that builds an HTTPClient.
-func NewHTTP3ClientWithResolver(logger model.Logger, reso model.Resolver) model.HTTPClient {
-	return NewHTTPClient(NewHTTP3TransportWithResolver(logger, reso))
+func NewHTTP3ClientWithResolver(netx *Netx, logger model.Logger, reso model.Resolver) model.HTTPClient {
+	return NewHTTPClient(NewHTTP3TransportWithResolver(netx, logger, reso))
 }
diff --git a/internal/netxlite/http3_test.go b/internal/netxlite/http3_test.go
@@ -133,15 +133,17 @@ func TestNewHTTP3TransportStdlib(t *testing.T) {
 
 func TestNewHTTP3TransportWithResolver(t *testing.T) {
 	t.Run("creates the correct type chain", func(t *testing.T) {
+		netx := &Netx{}
 		reso := &mocks.Resolver{}
-		txp := NewHTTP3TransportWithResolver(model.DiscardLogger, reso)
+		txp := NewHTTP3TransportWithResolver(netx, model.DiscardLogger, reso)
 		verifyTypeChainForHTTP3(t, txp, model.DiscardLogger, nil, nil, reso)
 	})
 }
 
 func TestNewHTTP3ClientWithResolver(t *testing.T) {
+	netx := &Netx{}
 	reso := &mocks.Resolver{}
-	clnt := NewHTTP3ClientWithResolver(model.DiscardLogger, reso)
+	clnt := NewHTTP3ClientWithResolver(netx, model.DiscardLogger, reso)
 	ewc, ok := clnt.(*httpClientErrWrapper)
 	if !ok {
 		t.Fatal("expected *httpClientErrWrapper")

diff --git a/internal/netxlite/httpquirks.go b/internal/netxlite/httpquirks.go
@@ -19,8 +19,7 @@ import (
 // the stdlib for everything but the given resolver.
 //
 // This function behavior is QUIRKY as documented in [NewHTTPTransport].
-func NewHTTPTransportWithResolver(logger model.DebugLogger, reso model.Resolver) model.HTTPTransport {
-	netx := &Netx{}
+func NewHTTPTransportWithResolver(netx *Netx, logger model.DebugLogger, reso model.Resolver) model.HTTPTransport {
 	dialer := netx.NewDialerWithResolver(logger, reso)
 	thx := netx.NewTLSHandshakerStdlib(logger)
 	tlsDialer := NewTLSDialer(dialer, thx)
@@ -123,8 +122,8 @@ func NewHTTPClientStdlib(logger model.DebugLogger) model.HTTPClient {
 // given resolver and then from that builds an HTTPClient.
 //
 // This function behavior is QUIRKY as documented in [NewHTTPTransport].
-func NewHTTPClientWithResolver(logger model.Logger, reso model.Resolver) model.HTTPClient {
-	return NewHTTPClient(NewHTTPTransportWithResolver(logger, reso))
+func NewHTTPClientWithResolver(netx *Netx, logger model.Logger, reso model.Resolver) model.HTTPClient {
+	return NewHTTPClient(NewHTTPTransportWithResolver(netx, logger, reso))
 }
 
 // NewHTTPClient creates a new, wrapped HTTPClient using the given transport.

diff --git a/internal/netxlite/httpquirks_test.go b/internal/netxlite/httpquirks_test.go
@@ -20,7 +20,8 @@ func TestNewHTTPTransportWithResolver(t *testing.T) {
 			return nil, expected
 		},
 	}
-	txp := NewHTTPTransportWithResolver(model.DiscardLogger, reso)
+	netx := &Netx{}
+	txp := NewHTTPTransportWithResolver(netx, model.DiscardLogger, reso)
 	req, err := http.NewRequest("GET", "http://x.org", nil)
 	if err != nil {
 		t.Fatal(err)
@@ -140,7 +141,8 @@ func TestNewHTTPClientStdlib(t *testing.T) {
 
 func TestNewHTTPClientWithResolver(t *testing.T) {
 	reso := &mocks.Resolver{}
-	clnt := NewHTTPClientWithResolver(model.DiscardLogger, reso)
+	netx := &Netx{}
+	clnt := NewHTTPClientWithResolver(netx, model.DiscardLogger, reso)
 	ewc, ok := clnt.(*httpClientErrWrapper)
 	if !ok {
 		t.Fatal("expected *httpClientErrWrapper")

diff --git a/internal/oohelperd/handler.go b/internal/oohelperd/handler.go
@@ -82,14 +82,14 @@ func NewHandler() *Handler {
 			// TODO(https://github.com/ooni/probe/issues/2534): the NewHTTPTransportWithResolver has QUIRKS and
 			// we should evaluate whether we can avoid using it here
 			return NewHTTPClientWithTransportFactory(
-				logger,
+				netx, logger,
 				netxlite.NewHTTPTransportWithResolver,
 			)
 		},
 
 		NewHTTP3Client: func(logger model.Logger) model.HTTPClient {
 			return NewHTTPClientWithTransportFactory(
-				logger,
+				netx, logger,
 				netxlite.NewHTTP3TransportWithResolver,
 			)
 		},
@@ -223,8 +223,8 @@ func newCookieJar() *cookiejar.Jar {
 // NewHTTPClientWithTransportFactory creates a new HTTP client
 // using the given [model.HTTPTransport] factory.
 func NewHTTPClientWithTransportFactory(
-	logger model.Logger,
-	txpFactory func(model.DebugLogger, model.Resolver) model.HTTPTransport,
+	netx *netxlite.Netx, logger model.Logger,
+	txpFactory func(*netxlite.Netx, model.DebugLogger, model.Resolver) model.HTTPTransport,
 ) model.HTTPClient {
 	// If the DoH resolver we're using insists that a given domain maps to
 	// bogons, make sure we're going to fail the HTTP measurement.
@@ -249,7 +249,7 @@ func NewHTTPClientWithTransportFactory(
 	// https://github.com/ooni/probe/issues/2488 for additional
 	// context and pointers to the relevant measurements.
 	client := &http.Client{
-		Transport:     txpFactory(logger, reso),
+		Transport:     txpFactory(netx, logger, reso),
 		CheckRedirect: nil,
 		Jar:           newCookieJar(),
 		Timeout:       0,