diff --git a/internal/engine/netx/httptransport.go b/internal/engine/netx/httptransport.go index c794a36..4cc095e 100644 --- a/internal/engine/netx/httptransport.go +++ b/internal/engine/netx/httptransport.go @@ -10,29 +10,21 @@ import ( // httpTransportConfig contains the configuration required for constructing an HTTP transport type httpTransportConfig struct { Dialer model.Dialer + Logger model.Logger QUICDialer model.QUICDialer TLSDialer model.TLSDialer TLSConfig *tls.Config } // newHTTP3Transport creates a new HTTP3Transport instance. -// -// Deprecation warning -// -// New code should use netxlite.NewHTTP3Transport instead. func newHTTP3Transport(config httpTransportConfig) model.HTTPTransport { // Rationale for using NoLogger here: previously this code did // not use a logger as well, so it's fine to keep it as is. - return netxlite.NewHTTP3Transport(model.DiscardLogger, - config.QUICDialer, config.TLSConfig) + return netxlite.NewHTTP3Transport(config.Logger, config.QUICDialer, config.TLSConfig) } // newSystemTransport creates a new "system" HTTP transport. That is a transport // using the Go standard library with custom dialer and TLS dialer. -// -// Deprecation warning -// -// New code should use netxlite.NewHTTPTransport instead. func newSystemTransport(config httpTransportConfig) model.HTTPTransport { - return netxlite.NewOOHTTPBaseTransport(config.Dialer, config.TLSDialer) + return netxlite.NewHTTPTransport(config.Logger, config.Dialer, config.TLSDialer) } diff --git a/internal/engine/netx/netx.go b/internal/engine/netx/netx.go index 7395f53..aede297 100644 --- a/internal/engine/netx/netx.go +++ b/internal/engine/netx/netx.go @@ -145,15 +145,16 @@ func NewHTTPTransport(config Config) model.HTTPTransport { } tInfo := allTransportsInfo[config.HTTP3Enabled] txp := tInfo.Factory(httpTransportConfig{ - Dialer: config.Dialer, QUICDialer: config.QUICDialer, TLSDialer: config.TLSDialer, - TLSConfig: config.TLSConfig}) + Dialer: config.Dialer, + Logger: model.ValidLoggerOrDefault(config.Logger), + QUICDialer: config.QUICDialer, + TLSDialer: config.TLSDialer, + TLSConfig: config.TLSConfig, + }) if config.ByteCounter != nil { txp = &bytecounter.HTTPTransport{ Counter: config.ByteCounter, HTTPTransport: txp} } - if config.Logger != nil { - txp = &netxlite.HTTPTransportLogger{Logger: config.Logger, HTTPTransport: txp} - } if config.Saver != nil { txp = &tracex.HTTPTransportSaver{ HTTPTransport: txp, Saver: config.Saver} diff --git a/internal/engine/netx/netx_test.go b/internal/engine/netx/netx_test.go index 137b955..ef0a8b9 100644 --- a/internal/engine/netx/netx_test.go +++ b/internal/engine/netx/netx_test.go @@ -272,13 +272,6 @@ func TestNewTLSDialer(t *testing.T) { }) } -func TestNewVanilla(t *testing.T) { - txp := NewHTTPTransport(Config{}) - if _, ok := txp.(*netxlite.HTTPTransportWrapper); !ok { - t.Fatal("not the transport we expected") - } -} - func TestNewWithDialer(t *testing.T) { expected := errors.New("mocked error") dialer := &mocks.Dialer{ @@ -311,25 +304,7 @@ func TestNewWithByteCounter(t *testing.T) { if bctxp.Counter != counter { t.Fatal("not the byte counter we expected") } - if _, ok := bctxp.HTTPTransport.(*netxlite.HTTPTransportWrapper); !ok { - t.Fatal("not the transport we expected") - } -} - -func TestNewWithLogger(t *testing.T) { - txp := NewHTTPTransport(Config{ - Logger: log.Log, - }) - ltxp, ok := txp.(*netxlite.HTTPTransportLogger) - if !ok { - t.Fatal("not the transport we expected") - } - if ltxp.Logger != log.Log { - t.Fatal("not the logger we expected") - } - if _, ok := ltxp.HTTPTransport.(*netxlite.HTTPTransportWrapper); !ok { - t.Fatal("not the transport we expected") - } + // We are going to trust the underlying transport returned by netxlite } func TestNewWithSaver(t *testing.T) { @@ -347,9 +322,7 @@ func TestNewWithSaver(t *testing.T) { if stxptxp.Saver != saver { t.Fatal("not the logger we expected") } - if _, ok := stxptxp.HTTPTransport.(*netxlite.HTTPTransportWrapper); !ok { - t.Fatal("not the transport we expected") - } + // We are going to trust the underlying type returned by netxlite } func TestNewDNSClientInvalidURL(t *testing.T) { diff --git a/internal/netxlite/http.go b/internal/netxlite/http.go index 1599cb4..05464e3 100644 --- a/internal/netxlite/http.go +++ b/internal/netxlite/http.go @@ -114,15 +114,8 @@ func NewHTTPTransportWithResolver(logger model.DebugLogger, reso model.Resolver) return NewHTTPTransport(logger, dialer, tlsDialer) } -// NewHTTPTransport combines NewOOHTTPBaseTransport and WrapHTTPTransport. -// -// 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)) -} - -// NewOOHTTPBaseTransport creates an HTTPTransport using the given dialers. +// NewHTTPTransport returns a wrapped HTTP transport for HTTP2 and HTTP/1.1 +// using the given dialer and logger. // // The returned transport will gracefully handle TLS connections // created using gitlab.com/yawning/utls.git, if the TLS dialer @@ -131,7 +124,7 @@ func NewHTTPTransport(logger model.DebugLogger, dialer model.Dialer, tlsDialer m // The returned transport will not have a configured proxy, not // even the proxy configurable from the environment. // -// The returned transport will disable transparent decompression +// QUIRK: the returned transport will disable transparent decompression // of compressed response bodies (and will not automatically // ask for such compression, though you can always do that manually). // @@ -139,14 +132,23 @@ func NewHTTPTransport(logger model.DebugLogger, dialer model.Dialer, tlsDialer m // created using its dialer and TLS dialer to always have a // read watchdog timeout to address https://github.com/ooni/probe/issues/1609. // -// The returned transport will always enforce 1 connection per host +// QUIRK: the returned transport will always enforce 1 connection per host // and we cannot get rid of this QUIRK requirement because it is // 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 is a low level factory. Consider not using it directly. -func NewOOHTTPBaseTransport(dialer model.Dialer, tlsDialer model.TLSDialer) model.HTTPTransport { +// 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)) +} + +// newOOHTTPBaseTransport is the low-level factory used by NewHTTPTransport +// to create a new, suitable HTTPTransport for HTTP2 and HTTP/1.1. +// +// This factory uses github.com/ooni/oohttp, hence its name. +func newOOHTTPBaseTransport(dialer model.Dialer, tlsDialer model.TLSDialer) model.HTTPTransport { // Using oohttp to support any TLS library. txp := oohttp.DefaultTransport.(*oohttp.Transport).Clone() diff --git a/internal/netxlite/http3.go b/internal/netxlite/http3.go index 0077f3a..b5ab546 100644 --- a/internal/netxlite/http3.go +++ b/internal/netxlite/http3.go @@ -48,19 +48,16 @@ func (txp *http3Transport) CloseIdleConnections() { // then the code will use the default TLS configuration. func NewHTTP3Transport( logger model.DebugLogger, dialer model.QUICDialer, tlsConfig *tls.Config) model.HTTPTransport { - return &httpTransportLogger{ - HTTPTransport: &http3Transport{ - child: &http3.RoundTripper{ - Dial: dialer.DialContext, - // The following (1) reduces the number of headers that Go will - // automatically send for us and (2) ensures that we always receive - // back the true headers, such as Content-Length. This change is - // functional to OONI's goal of observing the network. - DisableCompression: true, - TLSClientConfig: tlsConfig, - }, - dialer: dialer, + return WrapHTTPTransport(logger, &http3Transport{ + child: &http3.RoundTripper{ + Dial: dialer.DialContext, + // The following (1) reduces the number of headers that Go will + // automatically send for us and (2) ensures that we always receive + // back the true headers, such as Content-Length. This change is + // functional to OONI's goal of observing the network. + DisableCompression: true, + TLSClientConfig: tlsConfig, }, - Logger: logger, - } + dialer: dialer, + }) } diff --git a/internal/netxlite/http3_test.go b/internal/netxlite/http3_test.go index 0291505..6f21295 100644 --- a/internal/netxlite/http3_test.go +++ b/internal/netxlite/http3_test.go @@ -72,7 +72,8 @@ func TestNewHTTP3Transport(t *testing.T) { if logger.Logger != log.Log { t.Fatal("invalid logger") } - h3txp := logger.HTTPTransport.(*http3Transport) + ew := logger.HTTPTransport.(*httpTransportErrWrapper) + h3txp := ew.HTTPTransport.(*http3Transport) if h3txp.dialer != qd { t.Fatal("invalid dialer") } diff --git a/internal/netxlite/legacy.go b/internal/netxlite/legacy.go index bd284e9..ffb8ae9 100644 --- a/internal/netxlite/legacy.go +++ b/internal/netxlite/legacy.go @@ -17,8 +17,6 @@ var ( // // Deprecated: do not use these names in new code. type ( - HTTPTransportWrapper = httpTransportConnectionsCloser - HTTPTransportLogger = httpTransportLogger ErrorWrapperResolver = resolverErrWrapper ResolverSystemDoNotInstantiate = resolverSystem // instantiate => crash w/ nil transport ResolverLogger = resolverLogger