forked from ooni/probe-cli
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat(netxlite): add flexible HTTP transport factory (ooni#1270)
Now that we've clearly labeled and packaged technical debt, we can copy existing technical-debt-ridden code and modify it to obtain a flexible factory for creating HTTP transports, `NewHTTPTransportWithOptions`. 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 that we previously unconditionally configured. More in detail, this is a side-by-side comparison between new code's defaults and old code: | Setting Name | httpquirks.go (old code) | httpfactory.go (new code) | | ------------------- | ------------------------ | -------------------------- | | .Proxy | nil | nil | | .MaxConnsPerHost | 1 | ooni/oohttp's default | | .DisableCompression | true | true | | .ForceAttemptHTTP2 | true | true | So, basically, the biggest change is that we've removed the limitation of the max number of connections per host being set to 1. In any case, the new code allows to configure each of these fields. This factory will be the starting point for having custom network functions for the engine in line with ooni/probe#2531. While there, acknowledge that `NewHTTPClient` contains technical debt because it does unconditionally disable cookies, to document that and move it inside of the proper file (`httpquirks.go`).
- Loading branch information
1 parent
263b486
commit 65eb0fd
Showing
3 changed files
with
270 additions
and
4 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,96 @@ | ||
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 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: | ||
// | ||
// 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; | ||
// | ||
// 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. | ||
// | ||
// 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.DisableCompression = true | ||
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) { | ||
// "If Proxy is nil or returns a nil *URL, no proxy is used." | ||
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 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 | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,169 @@ | ||
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 | ||
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", | ||
"DisableCompression", | ||
"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") | ||
} | ||
if !underlying.DisableCompression { | ||
t.Fatal("expected true .DisableCompression") | ||
} | ||
}) | ||
|
||
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) | ||
}) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters