c/ooni-probe-cli
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(netx): ensure we create ~same HTTP3 and HTTP2 transports (#795)

Browse Source 
1. Use the netxlite.NewHTTPTransport factory for creating a new
HTTP2 (and HTTP1) transport;

2. Recognize the netxlite.NewOOHTTPTransport has now become
an implementation detail so make it private;

3. Recognize that netxlite.NewHTTP3Transport should call
netxlite.WrapTransport so it returns the same typechain
returned by netxlite.NewHTTPTransport (modulo, of course,
the real underlying transport), so ensure that we are
calling netxlite.WrapTransport in NewHTTP3Transport;

4. Recognize that the table based constructor inside of
netx needs a logger to create HTTPTransport instances using
either netxlite.NewHTTP{,3}Transport so pass this argument
along and ensure it's not nil using a constructor inside
model that guarantees that;

5. Cleanup netx's tests to avoid type asserting on the
typechains returned by netxlite since we already test
that inside netxlite;

6. Recognize that now we can make more legacy names inside
of netxlite private because we don't need to use them
inside tests anymore (because of previous point).

Reference issue: https://github.com/ooni/probe/issues/2121
This commit is contained in:
Simone Basso 2022-06-05 17:41:06 +02:00 committed by GitHub
parent d5249a6cf7
commit c6b3889a33
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 39 additions and 75 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
internal/engine/netx/httptransport.go
View File

@ -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)
}

11
internal/engine/netx/netx.go
View File

@ -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}

31
internal/engine/netx/netx_test.go
View File

@ -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) {

28
internal/netxlite/http.go
View File

@ -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()

25
internal/netxlite/http3.go
View File

@ -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,
})
}

3
internal/netxlite/http3_test.go
View File

@ -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")
}

2
internal/netxlite/legacy.go
View File

@ -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