From 80bac30cc1f4ad6cedcbeb09309da37af2eaf997 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 14 Sep 2023 17:55:33 +0200 Subject: [PATCH 1/6] feat(netxlite): add flexible HTTP transport factory Now that we've clearly labeled and package technical debt, we can copy existing technical-debt-ridden code and modify it to obtain a flexible factory for creating HTTP transports. In particular, this factory uses sensible defaults for measuring and there are options that you can pass it to customize details such as the backend proxy as well as other underlying behavior that we previously unconditionally configured. This factory will be the starting point for having custom network functions for the engine in line with https://github.com/ooni/probe/issues/2531. --- internal/netxlite/httpfactory.go | 85 ++++++++++++- internal/netxlite/httpfactory_test.go | 166 ++++++++++++++++++++++++++ internal/netxlite/httpquirks.go | 13 ++ 3 files changed, 260 insertions(+), 4 deletions(-) create mode 100644 internal/netxlite/httpfactory_test.go diff --git a/internal/netxlite/httpfactory.go b/internal/netxlite/httpfactory.go index 3e69de9808..8339734e63 100644 --- a/internal/netxlite/httpfactory.go +++ b/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 + } } diff --git a/internal/netxlite/httpfactory_test.go b/internal/netxlite/httpfactory_test.go new file mode 100644 index 0000000000..63ee337164 --- /dev/null +++ b/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) + }) +} diff --git a/internal/netxlite/httpquirks.go b/internal/netxlite/httpquirks.go index 7ab3af1f62..d4c26945e0 100644 --- a/internal/netxlite/httpquirks.go +++ b/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}) +} From 81003c191e45f1a749fdc1a2a934d97567861088 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 14 Sep 2023 18:01:38 +0200 Subject: [PATCH 2/6] Update internal/netxlite/httpfactory.go --- internal/netxlite/httpfactory.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/netxlite/httpfactory.go b/internal/netxlite/httpfactory.go index 8339734e63..d34eaf8cd5 100644 --- a/internal/netxlite/httpfactory.go +++ b/internal/netxlite/httpfactory.go @@ -18,7 +18,7 @@ type HTTPTransportOption func(txp *oohttp.Transport) // // 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 +// and .DialTLSContext fields of the transport. We also 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: From 97c9edf01686182b038167c15e10393c25f44194 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 14 Sep 2023 18:03:14 +0200 Subject: [PATCH 3/6] Update internal/netxlite/httpfactory.go --- internal/netxlite/httpfactory.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/netxlite/httpfactory.go b/internal/netxlite/httpfactory.go index d34eaf8cd5..3d0fc78d8b 100644 --- a/internal/netxlite/httpfactory.go +++ b/internal/netxlite/httpfactory.go @@ -67,6 +67,7 @@ func NewHTTPTransportWithOptions(logger model.Logger, func HTTPTransportOptionProxyURL(proxyURL *url.URL) HTTPTransportOption { return func(txp *oohttp.Transport) { txp.Proxy = func(r *oohttp.Request) (*url.URL, error) { + // "If Proxy is nil or returns a nil *URL, no proxy is used." return proxyURL, nil } } From b224287992f38fe4e0189d4abafd6943444ad688 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 14 Sep 2023 18:06:01 +0200 Subject: [PATCH 4/6] x --- internal/netxlite/httpfactory.go | 8 +++++++- internal/netxlite/httpfactory_test.go | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/internal/netxlite/httpfactory.go b/internal/netxlite/httpfactory.go index 8339734e63..f1c46a631e 100644 --- a/internal/netxlite/httpfactory.go +++ b/internal/netxlite/httpfactory.go @@ -27,7 +27,12 @@ type HTTPTransportOption func(txp *oohttp.Transport) // 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. +// 2. the .ForceAttemptHTTP2 field is set to true; +// +// 3. the .DisableCompression field is set to true, again required if we +// want to use this code for measuring, and we should make sure the defaults +// we're using are suitable for measuring, since the impact of making a +// mistake in measuring code is a data quality issue 😅. // // The returned transport supports logging and error wrapping because // internally this function calls [WrapHTTPTransport] before we return. @@ -46,6 +51,7 @@ func NewHTTPTransportWithOptions(logger model.Logger, txp.DialTLSContext = tlsDialer.DialTLSContext // As documented, disable proxies and force HTTP/2 + txp.DisableCompression = true txp.Proxy = nil txp.ForceAttemptHTTP2 = true diff --git a/internal/netxlite/httpfactory_test.go b/internal/netxlite/httpfactory_test.go index 63ee337164..e41e7b2a40 100644 --- a/internal/netxlite/httpfactory_test.go +++ b/internal/netxlite/httpfactory_test.go @@ -57,6 +57,7 @@ func TestNewHTTPTransportWithOptions(t *testing.T) { oohttp.Transport{}, "DialContext", "DialTLSContext", + "DisableCompression", "Proxy", "ForceAttemptHTTP2", ), @@ -78,6 +79,9 @@ func TestNewHTTPTransportWithOptions(t *testing.T) { if !underlying.ForceAttemptHTTP2 { t.Fatal("expected true .ForceAttemptHTTP2") } + if !underlying.DisableCompression { + t.Fatal("expected true .DisableCompression") + } }) unwrap := func(txp model.HTTPTransport) *oohttp.Transport { From a94d7473b0a7ba6826f0df725facd1da27faf4cb Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 14 Sep 2023 18:14:24 +0200 Subject: [PATCH 5/6] Update internal/netxlite/httpfactory.go --- internal/netxlite/httpfactory.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/netxlite/httpfactory.go b/internal/netxlite/httpfactory.go index 84e5544ba6..b39500271f 100644 --- a/internal/netxlite/httpfactory.go +++ b/internal/netxlite/httpfactory.go @@ -88,7 +88,7 @@ func HTTPTransportOptionMaxConnsPerHost(value int) HTTPTransportOption { } // HTTPTransportOptionDisableCompression configures the .DisableCompression field, which -// otherwise uses the default set in github.com/ooni/oohttp, i.e., false. +// otherwise is set to true, so that this code is ready for measuring out of the box. func HTTPTransportOptionDisableCompression(value bool) HTTPTransportOption { return func(txp *oohttp.Transport) { txp.DisableCompression = value From d58e1976740c00b0e977103f95eaf3bbb2671980 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 14 Sep 2023 18:15:05 +0200 Subject: [PATCH 6/6] Update internal/netxlite/httpfactory_test.go --- internal/netxlite/httpfactory_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/netxlite/httpfactory_test.go b/internal/netxlite/httpfactory_test.go index e41e7b2a40..c02955c11c 100644 --- a/internal/netxlite/httpfactory_test.go +++ b/internal/netxlite/httpfactory_test.go @@ -46,7 +46,6 @@ func TestNewHTTPTransportWithOptions(t *testing.T) { 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,