From 9f66878c43870a117a96b16d1995877e39501438 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 24 Jan 2024 11:15:19 +0100 Subject: [PATCH] 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 https://github.com/ooni/probe/issues/1517. --- internal/cmd/oohelper/oohelper.go | 2 +- internal/enginelocate/iplookup.go | 3 ++- internal/netemx/example_test.go | 2 +- internal/netemx/http3_test.go | 2 +- internal/netemx/oohelperd.go | 8 ++++---- internal/netxlite/http3.go | 7 +++---- internal/netxlite/http3_test.go | 6 ++++-- internal/netxlite/httpquirks.go | 7 +++---- internal/netxlite/httpquirks_test.go | 6 ++++-- internal/oohelperd/handler.go | 10 +++++----- 10 files changed, 28 insertions(+), 25 deletions(-) diff --git a/internal/cmd/oohelper/oohelper.go b/internal/cmd/oohelper/oohelper.go index 37dbe27a04..2b0c4de682 100644 --- 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 index 3693b0a112..b618a38d68 100644 --- 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 index 4fc8662367..317439881d 100644 --- 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 index bc11c54cb0..f736938113 100644 --- 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 index 4947bd951e..fc22d5f565 100644 --- 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 index 21caa3a91b..8603d3b9aa 100644 --- 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 index b0583e5865..9374dfee61 100644 --- 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 index b18b9786e1..271b6e3b95 100644 --- 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 index 0056197e55..88974b4167 100644 --- 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 index c14b0a5e8f..88e3c4a070 100644 --- 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,