From 0f6b12232d7ef4b58578b7b55b212bdda0dc9682 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 14 Sep 2023 16:46:01 +0200 Subject: [PATCH] chore(netxlite): isolate and annotate quirky functions (#1269) This diff isolates and annotates netxlite quirky functions such that https://github.com/ooni/probe/issues/2534 will be easier. This work is also useful to https://github.com/ooni/probe/issues/2531. While there, commit workaround for issue https://github.com/ooni/probe/issues/2535. --- internal/cmd/apitool/main.go | 2 + internal/cmd/oohelper/oohelper.go | 1 + internal/dslx/httptcp.go | 3 ++ internal/dslx/httptls.go | 3 ++ internal/enginelocate/iplookup.go | 2 + internal/enginenetx/http.go | 3 ++ internal/engineresolver/factory.go | 3 ++ internal/experiment/ndt7/ndt7.go | 4 ++ .../webconnectivitylte/cleartextflow.go | 3 ++ .../webconnectivitylte/secureflow.go | 3 ++ .../webconnectivityqa/badssl_test.go | 1 + .../webconnectivityqa/httpdiff_test.go | 2 + .../webconnectivityqa/redirect_test.go | 3 ++ internal/experiment/webconnectivityqa/run.go | 1 + .../webconnectivityqa/tlsblocking_test.go | 2 + internal/netemx/example_test.go | 8 ++++ internal/netemx/http_test.go | 1 + internal/netemx/https_test.go | 2 + internal/netemx/oohelperd.go | 2 + internal/netemx/oohelperd_test.go | 1 + internal/netemx/qaenv_test.go | 3 ++ internal/netxlite/dnsoverhttps_test.go | 3 ++ internal/netxlite/httpfactory.go | 12 ++++++ internal/netxlite/{http.go => httpquirks.go} | 43 ++++++------------- .../{http_test.go => httpquirks_test.go} | 10 ----- internal/netxlite/httpwrap.go | 27 ++++++++++++ internal/netxlite/httpwrap_test.go | 16 +++++++ internal/netxlite/integration_test.go | 6 +++ internal/netxlite/netx_test.go | 2 + internal/netxlite/tproxy_test.go | 1 + internal/oohelperd/handler.go | 2 + internal/testingx/httptestx_test.go | 6 +++ internal/throttling/throttling_test.go | 3 ++ .../tutorial/netxlite/chapter07/README.md | 3 ++ internal/tutorial/netxlite/chapter07/main.go | 3 ++ 35 files changed, 151 insertions(+), 39 deletions(-) create mode 100644 internal/netxlite/httpfactory.go rename internal/netxlite/{http.go => httpquirks.go} (82%) rename internal/netxlite/{http_test.go => httpquirks_test.go} (93%) create mode 100644 internal/netxlite/httpwrap.go create mode 100644 internal/netxlite/httpwrap_test.go diff --git a/internal/cmd/apitool/main.go b/internal/cmd/apitool/main.go index dd9290dd4b..1f8490171c 100644 --- a/internal/cmd/apitool/main.go +++ b/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{ diff --git a/internal/cmd/oohelper/oohelper.go b/internal/cmd/oohelper/oohelper.go index 132194c6de..a10a5ad886 100644 --- a/internal/cmd/oohelper/oohelper.go +++ b/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) } diff --git a/internal/dslx/httptcp.go b/internal/dslx/httptcp.go index 618e88a25f..2571322130 100644 --- a/internal/dslx/httptcp.go +++ b/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), diff --git a/internal/dslx/httptls.go b/internal/dslx/httptls.go index bbd8cd9068..9c1448ce0a 100644 --- a/internal/dslx/httptls.go +++ b/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(), diff --git a/internal/enginelocate/iplookup.go b/internal/enginelocate/iplookup.go index e82adb1415..42c6411f74 100644 --- a/internal/enginelocate/iplookup.go +++ b/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() diff --git a/internal/enginenetx/http.go b/internal/enginenetx/http.go index 771fdd2e82..e4d56df5bb 100644 --- a/internal/enginenetx/http.go +++ b/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} diff --git a/internal/engineresolver/factory.go b/internal/engineresolver/factory.go index 125b13c08c..d6e02c1254 100644 --- a/internal/engineresolver/factory.go +++ b/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) diff --git a/internal/experiment/ndt7/ndt7.go b/internal/experiment/ndt7/ndt7.go index 0356f757a8..cf8d772966 100644 --- a/internal/experiment/ndt7/ndt7.go +++ b/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()) diff --git a/internal/experiment/webconnectivitylte/cleartextflow.go b/internal/experiment/webconnectivitylte/cleartextflow.go index 7233c66818..fb51486bf4 100644 --- a/internal/experiment/webconnectivitylte/cleartextflow.go +++ b/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), diff --git a/internal/experiment/webconnectivitylte/secureflow.go b/internal/experiment/webconnectivitylte/secureflow.go index c161e76257..6e0f2bf8b6 100644 --- a/internal/experiment/webconnectivitylte/secureflow.go +++ b/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(), diff --git a/internal/experiment/webconnectivityqa/badssl_test.go b/internal/experiment/webconnectivityqa/badssl_test.go index 195e45fc01..6fff9e2741 100644 --- a/internal/experiment/webconnectivityqa/badssl_test.go +++ b/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) diff --git a/internal/experiment/webconnectivityqa/httpdiff_test.go b/internal/experiment/webconnectivityqa/httpdiff_test.go index 54e80e253d..dcc0af78b6 100644 --- a/internal/experiment/webconnectivityqa/httpdiff_test.go +++ b/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) diff --git a/internal/experiment/webconnectivityqa/redirect_test.go b/internal/experiment/webconnectivityqa/redirect_test.go index a6418c1375..fbf73881bb 100644 --- a/internal/experiment/webconnectivityqa/redirect_test.go +++ b/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) diff --git a/internal/experiment/webconnectivityqa/run.go b/internal/experiment/webconnectivityqa/run.go index 55321f9236..e9c44f7777 100644 --- a/internal/experiment/webconnectivityqa/run.go +++ b/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), diff --git a/internal/experiment/webconnectivityqa/tlsblocking_test.go b/internal/experiment/webconnectivityqa/tlsblocking_test.go index abd555c91b..9676a6df2c 100644 --- a/internal/experiment/webconnectivityqa/tlsblocking_test.go +++ b/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 { diff --git a/internal/netemx/example_test.go b/internal/netemx/example_test.go index c02a16de85..efeb38ccfa 100644 --- a/internal/netemx/example_test.go +++ b/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) diff --git a/internal/netemx/http_test.go b/internal/netemx/http_test.go index 246298584b..cbb4022764 100644 --- a/internal/netemx/http_test.go +++ b/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) diff --git a/internal/netemx/https_test.go b/internal/netemx/https_test.go index 640d4e33e3..1aeaf5fe01 100644 --- a/internal/netemx/https_test.go +++ b/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) diff --git a/internal/netemx/oohelperd.go b/internal/netemx/oohelperd.go index cdc6ffa58a..9baa2588fb 100644 --- a/internal/netemx/oohelperd.go +++ b/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, diff --git a/internal/netemx/oohelperd_test.go b/internal/netemx/oohelperd_test.go index 1a1f64ab89..eff7916ec8 100644 --- a/internal/netemx/oohelperd_test.go +++ b/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)) diff --git a/internal/netemx/qaenv_test.go b/internal/netemx/qaenv_test.go index 1277ec45fe..7fe1ced258 100644 --- a/internal/netemx/qaenv_test.go +++ b/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 diff --git a/internal/netxlite/dnsoverhttps_test.go b/internal/netxlite/dnsoverhttps_test.go index a442164aea..dd3d06fa87 100644 --- a/internal/netxlite/dnsoverhttps_test.go +++ b/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) diff --git a/internal/netxlite/httpfactory.go b/internal/netxlite/httpfactory.go new file mode 100644 index 0000000000..3e69de9808 --- /dev/null +++ b/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}) +} diff --git a/internal/netxlite/http.go b/internal/netxlite/httpquirks.go similarity index 82% rename from internal/netxlite/http.go rename to internal/netxlite/httpquirks.go index 38c736d865..7ab3af1f62 100644 --- a/internal/netxlite/http.go +++ b/internal/netxlite/httpquirks.go @@ -1,18 +1,20 @@ package netxlite // -// HTTP/1.1 and HTTP2 code +// QUIRKy Legacy HTTP code and behavior assumed by ./legacy/netx 😅 +// +// Ideally, we should not modify this code or apply minimal and obvious changes. // import ( - "net/http" - oohttp "github.com/ooni/oohttp" "github.com/ooni/probe-cli/v3/internal/model" ) // NewHTTPTransportWithResolver creates a new HTTP transport using // 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 { dialer := NewDialerWithResolver(logger, reso) thx := NewTLSHandshakerStdlib(logger) @@ -43,9 +45,6 @@ func NewHTTPTransportWithResolver(logger model.DebugLogger, reso model.Resolver) // necessary to perform sane measurements with tracing. We will be // able to possibly relax this requirement after we change the // way in which we perform measurements. -// -// This factory and NewHTTPTransportStdlib are the recommended -// ways of creating a new HTTPTransport. func NewHTTPTransport(logger model.DebugLogger, dialer model.Dialer, tlsDialer model.TLSDialer) model.HTTPTransport { return WrapHTTPTransport(logger, newOOHTTPBaseTransport(dialer, tlsDialer)) } @@ -54,6 +53,8 @@ func NewHTTPTransport(logger model.DebugLogger, dialer model.Dialer, tlsDialer m // to create a new, suitable HTTPTransport for HTTP2 and HTTP/1.1. // // This factory uses github.com/ooni/oohttp, hence its name. +// +// This function behavior is QUIRKY as documented in [NewHTTPTransport]. func newOOHTTPBaseTransport(dialer model.Dialer, tlsDialer model.TLSDialer) model.HTTPTransport { // Using oohttp to support any TLS library. txp := oohttp.DefaultTransport.(*oohttp.Transport).Clone() @@ -91,24 +92,12 @@ func newOOHTTPBaseTransport(dialer model.Dialer, tlsDialer model.TLSDialer) mode } } -// WrapHTTPTransport creates an HTTPTransport using the given logger -// and guarantees that returned errors are wrapped. -// -// This is a low level factory. Consider not using it directly. -func WrapHTTPTransport(logger model.DebugLogger, txp model.HTTPTransport) model.HTTPTransport { - return &httpTransportLogger{ - HTTPTransport: &httpTransportErrWrapper{txp}, - Logger: logger, - } -} - // NewHTTPTransportStdlib creates a new HTTPTransport using // the stdlib for DNS resolutions and TLS. // // This factory calls NewHTTPTransport with suitable dialers. // -// This factory and NewHTTPTransport are the recommended -// ways of creating a new HTTPTransport. +// This function behavior is QUIRKY as documented in [NewHTTPTransport]. func (netx *Netx) NewHTTPTransportStdlib(logger model.DebugLogger) model.HTTPTransport { dialer := netx.NewDialerWithResolver(logger, netx.NewStdlibResolver(logger)) tlsDialer := NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(logger)) @@ -117,6 +106,8 @@ func (netx *Netx) NewHTTPTransportStdlib(logger model.DebugLogger) model.HTTPTra // NewHTTPTransportStdlib is equivalent to creating an empty [*Netx] // and calling its NewHTTPTransportStdlib method. +// +// This function behavior is QUIRKY as documented in [NewHTTPTransport]. func NewHTTPTransportStdlib(logger model.DebugLogger) model.HTTPTransport { netx := &Netx{Underlying: nil} return netx.NewHTTPTransportStdlib(logger) @@ -124,6 +115,8 @@ func NewHTTPTransportStdlib(logger model.DebugLogger) model.HTTPTransport { // NewHTTPClientStdlib creates a new HTTPClient that uses the // standard library for TLS and DNS resolutions. +// +// This function behavior is QUIRKY as documented in [NewHTTPTransport]. func NewHTTPClientStdlib(logger model.DebugLogger) model.HTTPClient { txp := NewHTTPTransportStdlib(logger) return NewHTTPClient(txp) @@ -131,16 +124,8 @@ func NewHTTPClientStdlib(logger model.DebugLogger) model.HTTPClient { // NewHTTPClientWithResolver creates a new HTTPTransport using the // 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)) } - -// NewHTTPClient creates a new, wrapped HTTPClient using the given transport. -func NewHTTPClient(txp model.HTTPTransport) model.HTTPClient { - return WrapHTTPClient(&http.Client{Transport: txp}) -} - -// WrapHTTPClient wraps an HTTP client to add error wrapping capabilities. -func WrapHTTPClient(clnt model.HTTPClient) model.HTTPClient { - return &httpClientErrWrapper{clnt} -} diff --git a/internal/netxlite/http_test.go b/internal/netxlite/httpquirks_test.go similarity index 93% rename from internal/netxlite/http_test.go rename to internal/netxlite/httpquirks_test.go index e5d29a8c8d..3043293e4e 100644 --- a/internal/netxlite/http_test.go +++ b/internal/netxlite/httpquirks_test.go @@ -157,13 +157,3 @@ func TestNewHTTPClientWithResolver(t *testing.T) { t.Fatal("invalid resolver") } } - -func TestWrapHTTPClient(t *testing.T) { - origClient := &http.Client{} - wrapped := WrapHTTPClient(origClient) - errWrapper := wrapped.(*httpClientErrWrapper) - innerClient := errWrapper.HTTPClient.(*http.Client) - if innerClient != origClient { - t.Fatal("not the inner client we expected") - } -} diff --git a/internal/netxlite/httpwrap.go b/internal/netxlite/httpwrap.go new file mode 100644 index 0000000000..fade2f4b9a --- /dev/null +++ b/internal/netxlite/httpwrap.go @@ -0,0 +1,27 @@ +package netxlite + +// +// Wrappers for already constructed types. +// + +import ( + "github.com/ooni/probe-cli/v3/internal/model" +) + +// WrapHTTPTransport creates an HTTPTransport using the given logger +// and guarantees that returned errors are wrapped. +// +// This is a low level factory. Consider not using it directly. +func WrapHTTPTransport(logger model.DebugLogger, txp model.HTTPTransport) model.HTTPTransport { + return &httpTransportLogger{ + HTTPTransport: &httpTransportErrWrapper{txp}, + Logger: logger, + } +} + +// WrapHTTPClient wraps an HTTP client to add error wrapping capabilities. +// +// This is a low level factory. Consider not using it directly. +func WrapHTTPClient(clnt model.HTTPClient) model.HTTPClient { + return &httpClientErrWrapper{clnt} +} diff --git a/internal/netxlite/httpwrap_test.go b/internal/netxlite/httpwrap_test.go new file mode 100644 index 0000000000..eb200c9735 --- /dev/null +++ b/internal/netxlite/httpwrap_test.go @@ -0,0 +1,16 @@ +package netxlite + +import ( + "net/http" + "testing" +) + +func TestWrapHTTPClient(t *testing.T) { + origClient := &http.Client{} + wrapped := WrapHTTPClient(origClient) + errWrapper := wrapped.(*httpClientErrWrapper) + innerClient := errWrapper.HTTPClient.(*http.Client) + if innerClient != origClient { + t.Fatal("not the inner client we expected") + } +} diff --git a/internal/netxlite/integration_test.go b/internal/netxlite/integration_test.go index dd08256a64..81d8c2caff 100644 --- a/internal/netxlite/integration_test.go +++ b/internal/netxlite/integration_test.go @@ -9,6 +9,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "runtime" "testing" "time" @@ -78,6 +79,9 @@ func TestMeasureWithSystemResolver(t *testing.T) { // e.g. a domain containing a few random letters addrs, err := r.LookupHost(ctx, randx.Letters(7)+".ooni.nonexistent") if err == nil || err.Error() != netxlite.FailureGenericTimeoutError { + if runtime.GOOS == "windows" { + t.Skip("https://github.com/ooni/probe/issues/2535") + } t.Fatal("not the error we expected", err) } if addrs != nil { @@ -551,6 +555,8 @@ func TestHTTPTransport(t *testing.T) { conn.Close() })) defer srvr.Close() + // 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(model.DiscardLogger) req, err := http.NewRequest("GET", srvr.URL, nil) if err != nil { diff --git a/internal/netxlite/netx_test.go b/internal/netxlite/netx_test.go index c502efca70..803f015b54 100644 --- a/internal/netxlite/netx_test.go +++ b/internal/netxlite/netx_test.go @@ -75,6 +75,8 @@ func TestNetx(t *testing.T) { netx := &Netx{underlyingNetwork} t.Run("HTTPS fetch", func(t *testing.T) { + // TODO(https://github.com/ooni/probe/issues/2534): NewHTTPTransportStdlib is QUIRKY but we probably + // don't care about using a QUIRKY function here? txp := netx.NewHTTPTransportStdlib(log.Log) client := &http.Client{Transport: txp} resp, err := client.Get("https://www.example.com/") diff --git a/internal/netxlite/tproxy_test.go b/internal/netxlite/tproxy_test.go index 51916f2e05..60d593a53a 100644 --- a/internal/netxlite/tproxy_test.go +++ b/internal/netxlite/tproxy_test.go @@ -83,6 +83,7 @@ func TestWithCustomTProxy(t *testing.T) { } WithCustomTProxy(tproxy, func() { + // TODO(https://github.com/ooni/probe/issues/2534): NewHTTPClientStdlib has QUIRKS but they're not needed here clnt := NewHTTPClientStdlib(model.DiscardLogger) req, err := http.NewRequestWithContext(context.Background(), "GET", srvr.URL, nil) if err != nil { diff --git a/internal/oohelperd/handler.go b/internal/oohelperd/handler.go index 30ff5a87d0..ace6709b5a 100644 --- a/internal/oohelperd/handler.go +++ b/internal/oohelperd/handler.go @@ -72,6 +72,8 @@ func NewHandler() *Handler { Measure: measure, NewHTTPClient: func(logger model.Logger) model.HTTPClient { + // 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, netxlite.NewHTTPTransportWithResolver, diff --git a/internal/testingx/httptestx_test.go b/internal/testingx/httptestx_test.go index bd91b44a60..c34e6676a0 100644 --- a/internal/testingx/httptestx_test.go +++ b/internal/testingx/httptestx_test.go @@ -149,6 +149,9 @@ func TestHTTPTestxWithStdlib(t *testing.T) { tlsHandshaker := netxlite.NewTLSHandshakerStdlib(log.Log) tlsDialer := netxlite.NewTLSDialerWithConfig( tcpDialer, tlsHandshaker, &tls.Config{RootCAs: srvr.X509CertPool}) + // 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(log.Log, tcpDialer, tlsDialer) client := netxlite.NewHTTPClient(txp) @@ -421,6 +424,9 @@ func TestHTTPTestxWithNetem(t *testing.T) { tlsHandshaker := netxlite.NewTLSHandshakerStdlib(log.Log) tlsDialer := netxlite.NewTLSDialerWithConfig( tcpDialer, tlsHandshaker, &tls.Config{RootCAs: srvr.X509CertPool}) + // 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(log.Log, tcpDialer, tlsDialer) client := netxlite.NewHTTPClient(txp) diff --git a/internal/throttling/throttling_test.go b/internal/throttling/throttling_test.go index 37de4d9173..3f13d6ff8a 100644 --- a/internal/throttling/throttling_test.go +++ b/internal/throttling/throttling_test.go @@ -44,6 +44,9 @@ func TestSamplerWorkingAsIntended(t *testing.T) { dialer := tx.NewDialerWithoutResolver(model.DiscardLogger) // create an 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. txp := netxlite.NewHTTPTransport(model.DiscardLogger, dialer, netxlite.NewNullTLSDialer()) // create the HTTP request to issue diff --git a/internal/tutorial/netxlite/chapter07/README.md b/internal/tutorial/netxlite/chapter07/README.md index 92737b402e..ecd359814d 100644 --- a/internal/tutorial/netxlite/chapter07/README.md +++ b/internal/tutorial/netxlite/chapter07/README.md @@ -81,6 +81,9 @@ a single request using the given TLS conn. uses a cleartext TCP connection. In the next chapter we'll see how to do the same using QUIC.) +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. ```Go clnt := &http.Client{Transport: netxlite.NewHTTPTransport( log.Log, netxlite.NewNullDialer(), diff --git a/internal/tutorial/netxlite/chapter07/main.go b/internal/tutorial/netxlite/chapter07/main.go index 952c20fc04..60ac559eeb 100644 --- a/internal/tutorial/netxlite/chapter07/main.go +++ b/internal/tutorial/netxlite/chapter07/main.go @@ -82,6 +82,9 @@ func main() { // uses a cleartext TCP connection. In the next chapter we'll // see how to do the same using QUIC.) // + // 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. // ```Go clnt := &http.Client{Transport: netxlite.NewHTTPTransport( log.Log, netxlite.NewNullDialer(),