From e4f10eeac2638110e9a5e4317e5c7257d0d04842 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 31 May 2022 08:11:07 +0200 Subject: [PATCH] refactor: continue to simplify engine/netx (#769) The objective of this diff is to simplify the code inside engine/netx while moving more bits of code inside netxlite. See https://github.com/ooni/probe/issues/2121 --- internal/engine/experiment.go | 3 +- internal/engine/netx/dialer/bytecounter.go | 5 - internal/engine/netx/dialer/dialer.go | 8 +- internal/engine/netx/dialer/dialer_test.go | 21 ++- internal/engine/netx/dialer/doc.go | 4 - internal/engine/netx/dialer/example_test.go | 30 ---- .../engine/netx/dialer/integration_test.go | 25 ---- internal/engine/netx/dialer/proxy.go | 5 - .../engine/netx/httptransport/bytecounter.go | 5 - .../engine/netx/httptransport/fake_test.go | 62 -------- .../netx/httptransport/http3transport.go | 24 ---- .../netx/httptransport/http3transport_test.go | 40 ------ .../netx/httptransport/httptransport.go | 23 +++ .../engine/netx/httptransport/saver_test.go | 69 +++++++-- internal/engine/netx/httptransport/system.go | 38 ----- internal/engine/netx/netx.go | 4 +- internal/engine/netx/netx_test.go | 16 +-- .../engine/netx/quicdialer/connectionstate.go | 12 -- internal/engine/netx/quicdialer/saver.go | 5 + internal/engine/netx/resolver/bogon.go | 32 ----- internal/engine/netx/resolver/bogon_test.go | 37 ----- internal/netxlite/bogon.go | 51 +++++++ internal/netxlite/bogon_test.go | 136 +++++++++++++++++- internal/netxlite/legacy.go | 1 + 24 files changed, 312 insertions(+), 344 deletions(-) delete mode 100644 internal/engine/netx/dialer/bytecounter.go delete mode 100644 internal/engine/netx/dialer/doc.go delete mode 100644 internal/engine/netx/dialer/example_test.go delete mode 100644 internal/engine/netx/dialer/integration_test.go delete mode 100644 internal/engine/netx/dialer/proxy.go delete mode 100644 internal/engine/netx/httptransport/bytecounter.go delete mode 100644 internal/engine/netx/httptransport/fake_test.go delete mode 100644 internal/engine/netx/httptransport/http3transport.go delete mode 100644 internal/engine/netx/httptransport/http3transport_test.go delete mode 100644 internal/engine/netx/httptransport/system.go delete mode 100644 internal/engine/netx/quicdialer/connectionstate.go delete mode 100644 internal/engine/netx/resolver/bogon.go delete mode 100644 internal/engine/netx/resolver/bogon_test.go diff --git a/internal/engine/experiment.go b/internal/engine/experiment.go index 7f62c568ff..59562e79f5 100644 --- a/internal/engine/experiment.go +++ b/internal/engine/experiment.go @@ -11,7 +11,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/bytecounter" "github.com/ooni/probe-cli/v3/internal/engine/geolocate" - "github.com/ooni/probe-cli/v3/internal/engine/netx/httptransport" "github.com/ooni/probe-cli/v3/internal/engine/probeservices" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/version" @@ -286,7 +285,7 @@ func (e *Experiment) OpenReportContext(ctx context.Context) error { } // use custom client to have proper byte accounting httpClient := &http.Client{ - Transport: &httptransport.ByteCountingTransport{ + Transport: &bytecounter.HTTPTransport{ HTTPTransport: e.session.httpDefaultTransport, // proxy is OK Counter: e.byteCounter, }, diff --git a/internal/engine/netx/dialer/bytecounter.go b/internal/engine/netx/dialer/bytecounter.go deleted file mode 100644 index 08aae8b6d3..0000000000 --- a/internal/engine/netx/dialer/bytecounter.go +++ /dev/null @@ -1,5 +0,0 @@ -package dialer - -import "github.com/ooni/probe-cli/v3/internal/bytecounter" - -type byteCounterDialer = bytecounter.ContextAwareDialer diff --git a/internal/engine/netx/dialer/dialer.go b/internal/engine/netx/dialer/dialer.go index a5f79eb936..17e093ce97 100644 --- a/internal/engine/netx/dialer/dialer.go +++ b/internal/engine/netx/dialer/dialer.go @@ -1,8 +1,12 @@ +// Package dialer allows you to create a net.Dialer-compatible +// DialContext-enabled dialer with error wrapping, optional logging, +// optional network-events saving, and optional proxying. package dialer import ( "net/url" + "github.com/ooni/probe-cli/v3/internal/bytecounter" "github.com/ooni/probe-cli/v3/internal/engine/netx/trace" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" @@ -63,9 +67,9 @@ func New(config *Config, resolver model.Resolver) model.Dialer { Resolver: resolver, Dialer: d, } - d = &proxyDialer{ProxyURL: config.ProxyURL, Dialer: d} + d = &netxlite.MaybeProxyDialer{ProxyURL: config.ProxyURL, Dialer: d} if config.ContextByteCounting { - d = &byteCounterDialer{Dialer: d} + d = &bytecounter.ContextAwareDialer{Dialer: d} } return d } diff --git a/internal/engine/netx/dialer/dialer_test.go b/internal/engine/netx/dialer/dialer_test.go index 29af7c6c5d..41bd5b50a5 100644 --- a/internal/engine/netx/dialer/dialer_test.go +++ b/internal/engine/netx/dialer/dialer_test.go @@ -1,10 +1,12 @@ package dialer import ( + "net/http" "net/url" "testing" "github.com/apex/log" + "github.com/ooni/probe-cli/v3/internal/bytecounter" "github.com/ooni/probe-cli/v3/internal/engine/netx/trace" "github.com/ooni/probe-cli/v3/internal/netxlite" ) @@ -18,11 +20,11 @@ func TestNewCreatesTheExpectedChain(t *testing.T) { ProxyURL: &url.URL{}, ReadWriteSaver: saver, }, netxlite.DefaultResolver) - bcd, ok := dlr.(*byteCounterDialer) + bcd, ok := dlr.(*bytecounter.ContextAwareDialer) if !ok { t.Fatal("not a byteCounterDialer") } - pd, ok := bcd.Dialer.(*proxyDialer) + pd, ok := bcd.Dialer.(*netxlite.MaybeProxyDialer) if !ok { t.Fatal("not a proxyDialer") } @@ -51,3 +53,18 @@ func TestNewCreatesTheExpectedChain(t *testing.T) { t.Fatal("not a DialerSystem") } } + +func TestDialerNewSuccess(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + log.SetLevel(log.DebugLevel) + d := New(&Config{Logger: log.Log}, netxlite.DefaultResolver) + txp := &http.Transport{DialContext: d.DialContext} + client := &http.Client{Transport: txp} + resp, err := client.Get("http://www.google.com") + if err != nil { + t.Fatal(err) + } + resp.Body.Close() +} diff --git a/internal/engine/netx/dialer/doc.go b/internal/engine/netx/dialer/doc.go deleted file mode 100644 index 0ead6b093f..0000000000 --- a/internal/engine/netx/dialer/doc.go +++ /dev/null @@ -1,4 +0,0 @@ -// Package dialer allows you to create a net.Dialer-compatible -// DialContext-enabled dialer with error wrapping, optional logging, -// optional network-events saving, and optional proxying. -package dialer diff --git a/internal/engine/netx/dialer/example_test.go b/internal/engine/netx/dialer/example_test.go deleted file mode 100644 index f505ec006a..0000000000 --- a/internal/engine/netx/dialer/example_test.go +++ /dev/null @@ -1,30 +0,0 @@ -package dialer_test - -import ( - "context" - - "github.com/apex/log" - "github.com/ooni/probe-cli/v3/internal/engine/netx/dialer" - "github.com/ooni/probe-cli/v3/internal/engine/netx/trace" - "github.com/ooni/probe-cli/v3/internal/netxlite" -) - -func Example() { - saver := &trace.Saver{} - - dlr := dialer.New(&dialer.Config{ - DialSaver: saver, - Logger: log.Log, - ReadWriteSaver: saver, - }, netxlite.DefaultResolver) - - ctx := context.Background() - conn, err := dlr.DialContext(ctx, "tcp", "8.8.8.8:53") - if err != nil { - log.WithError(err).Fatal("DialContext failed") - } - - // ... use the connection ... - - conn.Close() -} diff --git a/internal/engine/netx/dialer/integration_test.go b/internal/engine/netx/dialer/integration_test.go deleted file mode 100644 index ed691ea12b..0000000000 --- a/internal/engine/netx/dialer/integration_test.go +++ /dev/null @@ -1,25 +0,0 @@ -package dialer_test - -import ( - "net/http" - "testing" - - "github.com/apex/log" - "github.com/ooni/probe-cli/v3/internal/engine/netx/dialer" - "github.com/ooni/probe-cli/v3/internal/netxlite" -) - -func TestDialerNewSuccess(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - log.SetLevel(log.DebugLevel) - d := dialer.New(&dialer.Config{Logger: log.Log}, netxlite.DefaultResolver) - txp := &http.Transport{DialContext: d.DialContext} - client := &http.Client{Transport: txp} - resp, err := client.Get("http://www.google.com") - if err != nil { - t.Fatal(err) - } - resp.Body.Close() -} diff --git a/internal/engine/netx/dialer/proxy.go b/internal/engine/netx/dialer/proxy.go deleted file mode 100644 index 54d29424f8..0000000000 --- a/internal/engine/netx/dialer/proxy.go +++ /dev/null @@ -1,5 +0,0 @@ -package dialer - -import "github.com/ooni/probe-cli/v3/internal/netxlite" - -type proxyDialer = netxlite.MaybeProxyDialer diff --git a/internal/engine/netx/httptransport/bytecounter.go b/internal/engine/netx/httptransport/bytecounter.go deleted file mode 100644 index 99b1f5e9f2..0000000000 --- a/internal/engine/netx/httptransport/bytecounter.go +++ /dev/null @@ -1,5 +0,0 @@ -package httptransport - -import "github.com/ooni/probe-cli/v3/internal/bytecounter" - -type ByteCountingTransport = bytecounter.HTTPTransport diff --git a/internal/engine/netx/httptransport/fake_test.go b/internal/engine/netx/httptransport/fake_test.go deleted file mode 100644 index 92acfb251b..0000000000 --- a/internal/engine/netx/httptransport/fake_test.go +++ /dev/null @@ -1,62 +0,0 @@ -package httptransport - -import ( - "context" - "net" - "net/http" - "time" - - "github.com/ooni/probe-cli/v3/internal/netxlite" -) - -type FakeDialer struct { - Conn net.Conn - Err error -} - -func (d FakeDialer) DialContext(ctx context.Context, network, address string) (net.Conn, error) { - time.Sleep(10 * time.Microsecond) - return d.Conn, d.Err -} - -type FakeTransport struct { - Name string - Err error - Func func(*http.Request) (*http.Response, error) - Resp *http.Response -} - -func (txp FakeTransport) Network() string { - return txp.Name -} - -func (txp FakeTransport) RoundTrip(req *http.Request) (*http.Response, error) { - time.Sleep(10 * time.Microsecond) - if txp.Func != nil { - return txp.Func(req) - } - if req.Body != nil { - netxlite.ReadAllContext(req.Context(), req.Body) - req.Body.Close() - } - if txp.Err != nil { - return nil, txp.Err - } - txp.Resp.Request = req // non thread safe but it doesn't matter - return txp.Resp, nil -} - -func (txp FakeTransport) CloseIdleConnections() {} - -type FakeBody struct { - Err error -} - -func (fb FakeBody) Read(p []byte) (int, error) { - time.Sleep(10 * time.Microsecond) - return 0, fb.Err -} - -func (fb FakeBody) Close() error { - return nil -} diff --git a/internal/engine/netx/httptransport/http3transport.go b/internal/engine/netx/httptransport/http3transport.go deleted file mode 100644 index 1aaf931a8d..0000000000 --- a/internal/engine/netx/httptransport/http3transport.go +++ /dev/null @@ -1,24 +0,0 @@ -package httptransport - -import ( - "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/netxlite" -) - -// NewHTTP3Transport creates a new HTTP3Transport instance. -// -// Deprecation warning -// -// New code should use netxlite.NewHTTP3Transport instead. -func NewHTTP3Transport(config Config) 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(&NoLogger{}, - config.QUICDialer, config.TLSConfig) -} - -type NoLogger struct{} - -func (*NoLogger) Debug(message string) {} - -func (*NoLogger) Debugf(format string, v ...interface{}) {} diff --git a/internal/engine/netx/httptransport/http3transport_test.go b/internal/engine/netx/httptransport/http3transport_test.go deleted file mode 100644 index 2e2b9e385a..0000000000 --- a/internal/engine/netx/httptransport/http3transport_test.go +++ /dev/null @@ -1,40 +0,0 @@ -package httptransport_test - -import ( - "context" - "crypto/tls" - "errors" - "net/http" - "testing" - - "github.com/lucas-clemente/quic-go" - "github.com/ooni/probe-cli/v3/internal/engine/netx/httptransport" - "github.com/ooni/probe-cli/v3/internal/model/mocks" -) - -func TestNewHTTP3Transport(t *testing.T) { - // make sure we can create a working transport using this factory. - expected := errors.New("mocked error") - txp := httptransport.NewHTTP3Transport(httptransport.Config{ - QUICDialer: &mocks.QUICDialer{ - MockDialContext: func(ctx context.Context, network, address string, - tlsConfig *tls.Config, quicConfig *quic.Config) (quic.EarlyConnection, error) { - return nil, expected - }, - MockCloseIdleConnections: func() { - // nothing - }, - }, - }) - req, err := http.NewRequest("GET", "https://google.com", nil) - if err != nil { - t.Fatal(err) - } - resp, err := txp.RoundTrip(req) - if !errors.Is(err, expected) { - t.Fatal("unexpected err", err) - } - if resp != nil { - t.Fatal("expected nil resp") - } -} diff --git a/internal/engine/netx/httptransport/httptransport.go b/internal/engine/netx/httptransport/httptransport.go index e1e71ba061..6fb8c07708 100644 --- a/internal/engine/netx/httptransport/httptransport.go +++ b/internal/engine/netx/httptransport/httptransport.go @@ -5,6 +5,7 @@ import ( "crypto/tls" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) // Config contains the configuration required for constructing an HTTP transport @@ -14,3 +15,25 @@ type Config struct { TLSDialer model.TLSDialer TLSConfig *tls.Config } + +// NewHTTP3Transport creates a new HTTP3Transport instance. +// +// Deprecation warning +// +// New code should use netxlite.NewHTTP3Transport instead. +func NewHTTP3Transport(config Config) 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) +} + +// 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 Config) model.HTTPTransport { + return netxlite.NewOOHTTPBaseTransport(config.Dialer, config.TLSDialer) +} diff --git a/internal/engine/netx/httptransport/saver_test.go b/internal/engine/netx/httptransport/saver_test.go index a5d45d8847..23868c48d2 100644 --- a/internal/engine/netx/httptransport/saver_test.go +++ b/internal/engine/netx/httptransport/saver_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "io" + "net" "net/http" "net/url" "strings" @@ -76,7 +77,7 @@ func TestSaverMetadataFailure(t *testing.T) { expected := errors.New("mocked error") saver := &trace.Saver{} txp := httptransport.SaverMetadataHTTPTransport{ - HTTPTransport: httptransport.FakeTransport{ + HTTPTransport: FakeTransport{ Err: expected, }, Saver: saver, @@ -161,7 +162,7 @@ func TestSaverTransactionFailure(t *testing.T) { expected := errors.New("mocked error") saver := &trace.Saver{} txp := httptransport.SaverTransactionHTTPTransport{ - HTTPTransport: httptransport.FakeTransport{ + HTTPTransport: FakeTransport{ Err: expected, }, Saver: saver, @@ -201,7 +202,7 @@ func TestSaverTransactionFailure(t *testing.T) { func TestSaverBodySuccess(t *testing.T) { saver := new(trace.Saver) txp := httptransport.SaverBodyHTTPTransport{ - HTTPTransport: httptransport.FakeTransport{ + HTTPTransport: FakeTransport{ Func: func(req *http.Request) (*http.Response, error) { data, err := netxlite.ReadAllContext(context.Background(), req.Body) if err != nil { @@ -272,7 +273,7 @@ func TestSaverBodySuccess(t *testing.T) { func TestSaverBodyRequestReadError(t *testing.T) { saver := new(trace.Saver) txp := httptransport.SaverBodyHTTPTransport{ - HTTPTransport: httptransport.FakeTransport{ + HTTPTransport: FakeTransport{ Func: func(req *http.Request) (*http.Response, error) { panic("should not be called") }, @@ -281,7 +282,7 @@ func TestSaverBodyRequestReadError(t *testing.T) { Saver: saver, } expected := errors.New("mocked error") - body := httptransport.FakeBody{Err: expected} + body := FakeBody{Err: expected} req, err := http.NewRequest("POST", "http://x.org/y", body) if err != nil { t.Fatal(err) @@ -303,7 +304,7 @@ func TestSaverBodyRoundTripError(t *testing.T) { saver := new(trace.Saver) expected := errors.New("mocked error") txp := httptransport.SaverBodyHTTPTransport{ - HTTPTransport: httptransport.FakeTransport{ + HTTPTransport: FakeTransport{ Err: expected, }, SnapshotSize: 4, @@ -343,11 +344,11 @@ func TestSaverBodyResponseReadError(t *testing.T) { saver := new(trace.Saver) expected := errors.New("mocked error") txp := httptransport.SaverBodyHTTPTransport{ - HTTPTransport: httptransport.FakeTransport{ + HTTPTransport: FakeTransport{ Func: func(req *http.Request) (*http.Response, error) { return &http.Response{ StatusCode: 200, - Body: httptransport.FakeBody{ + Body: FakeBody{ Err: expected, }, }, nil @@ -417,3 +418,55 @@ func TestCloneHeaders(t *testing.T) { } }) } + +type FakeDialer struct { + Conn net.Conn + Err error +} + +func (d FakeDialer) DialContext(ctx context.Context, network, address string) (net.Conn, error) { + time.Sleep(10 * time.Microsecond) + return d.Conn, d.Err +} + +type FakeTransport struct { + Name string + Err error + Func func(*http.Request) (*http.Response, error) + Resp *http.Response +} + +func (txp FakeTransport) Network() string { + return txp.Name +} + +func (txp FakeTransport) RoundTrip(req *http.Request) (*http.Response, error) { + time.Sleep(10 * time.Microsecond) + if txp.Func != nil { + return txp.Func(req) + } + if req.Body != nil { + netxlite.ReadAllContext(req.Context(), req.Body) + req.Body.Close() + } + if txp.Err != nil { + return nil, txp.Err + } + txp.Resp.Request = req // non thread safe but it doesn't matter + return txp.Resp, nil +} + +func (txp FakeTransport) CloseIdleConnections() {} + +type FakeBody struct { + Err error +} + +func (fb FakeBody) Read(p []byte) (int, error) { + time.Sleep(10 * time.Microsecond) + return 0, fb.Err +} + +func (fb FakeBody) Close() error { + return nil +} diff --git a/internal/engine/netx/httptransport/system.go b/internal/engine/netx/httptransport/system.go deleted file mode 100644 index f921ec7126..0000000000 --- a/internal/engine/netx/httptransport/system.go +++ /dev/null @@ -1,38 +0,0 @@ -package httptransport - -import ( - oohttp "github.com/ooni/oohttp" - "github.com/ooni/probe-cli/v3/internal/model" -) - -// 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 Config) model.HTTPTransport { - txp := oohttp.DefaultTransport.(*oohttp.Transport).Clone() - txp.DialContext = config.Dialer.DialContext - txp.DialTLSContext = config.TLSDialer.DialTLSContext - // Better for Cloudflare DNS and also better because we have less - // noisy events and we can better understand what happened. - txp.MaxConnsPerHost = 1 - // 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. - txp.DisableCompression = true - return &SystemTransportWrapper{&oohttp.StdlibTransport{Transport: txp}} -} - -// SystemTransportWrapper adapts *http.Transport to have the .Network method -type SystemTransportWrapper struct { - *oohttp.StdlibTransport -} - -func (txp *SystemTransportWrapper) Network() string { - return "tcp" -} - -var _ model.HTTPTransport = &SystemTransportWrapper{} diff --git a/internal/engine/netx/netx.go b/internal/engine/netx/netx.go index 5153b74573..d9e84c713e 100644 --- a/internal/engine/netx/netx.go +++ b/internal/engine/netx/netx.go @@ -97,7 +97,7 @@ func NewResolver(config Config) model.Resolver { r = cache } if config.BogonIsError { - r = resolver.BogonResolver{Resolver: r} + r = &netxlite.BogonResolver{Resolver: r} } r = &netxlite.ErrorWrapperResolver{Resolver: r} if config.Logger != nil { @@ -202,7 +202,7 @@ func NewHTTPTransport(config Config) model.HTTPTransport { TLSConfig: config.TLSConfig}) if config.ByteCounter != nil { - txp = &httptransport.ByteCountingTransport{ + txp = &bytecounter.HTTPTransport{ Counter: config.ByteCounter, HTTPTransport: txp} } if config.Logger != nil { diff --git a/internal/engine/netx/netx_test.go b/internal/engine/netx/netx_test.go index 9ff50d2c59..4f2cd78c45 100644 --- a/internal/engine/netx/netx_test.go +++ b/internal/engine/netx/netx_test.go @@ -42,7 +42,7 @@ func TestNewResolverVanilla(t *testing.T) { func TestNewResolverSpecificResolver(t *testing.T) { r := netx.NewResolver(netx.Config{ - BaseResolver: resolver.BogonResolver{ + BaseResolver: &netxlite.BogonResolver{ // not initialized because it doesn't matter in this context }, }) @@ -58,7 +58,7 @@ func TestNewResolverSpecificResolver(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - _, ok = ar.Resolver.(resolver.BogonResolver) + _, ok = ar.Resolver.(*netxlite.BogonResolver) if !ok { t.Fatal("not the resolver we expected") } @@ -76,7 +76,7 @@ func TestNewResolverWithBogonFilter(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - br, ok := ewr.Resolver.(resolver.BogonResolver) + br, ok := ewr.Resolver.(*netxlite.BogonResolver) if !ok { t.Fatal("not the resolver we expected") } @@ -420,7 +420,7 @@ func TestNewTLSDialerWithNoTLSVerifyAndNoConfig(t *testing.T) { func TestNewVanilla(t *testing.T) { txp := netx.NewHTTPTransport(netx.Config{}) - if _, ok := txp.(*httptransport.SystemTransportWrapper); !ok { + if _, ok := txp.(*netxlite.HTTPTransportWrapper); !ok { t.Fatal("not the transport we expected") } } @@ -473,14 +473,14 @@ func TestNewWithByteCounter(t *testing.T) { txp := netx.NewHTTPTransport(netx.Config{ ByteCounter: counter, }) - bctxp, ok := txp.(*httptransport.ByteCountingTransport) + bctxp, ok := txp.(*bytecounter.HTTPTransport) if !ok { t.Fatal("not the transport we expected") } if bctxp.Counter != counter { t.Fatal("not the byte counter we expected") } - if _, ok := bctxp.HTTPTransport.(*httptransport.SystemTransportWrapper); !ok { + if _, ok := bctxp.HTTPTransport.(*netxlite.HTTPTransportWrapper); !ok { t.Fatal("not the transport we expected") } } @@ -496,7 +496,7 @@ func TestNewWithLogger(t *testing.T) { if ltxp.Logger != log.Log { t.Fatal("not the logger we expected") } - if _, ok := ltxp.HTTPTransport.(*httptransport.SystemTransportWrapper); !ok { + if _, ok := ltxp.HTTPTransport.(*netxlite.HTTPTransportWrapper); !ok { t.Fatal("not the transport we expected") } } @@ -527,7 +527,7 @@ func TestNewWithSaver(t *testing.T) { if smtxp.Saver != saver { t.Fatal("not the logger we expected") } - if _, ok := smtxp.HTTPTransport.(*httptransport.SystemTransportWrapper); !ok { + if _, ok := smtxp.HTTPTransport.(*netxlite.HTTPTransportWrapper); !ok { t.Fatal("not the transport we expected") } } diff --git a/internal/engine/netx/quicdialer/connectionstate.go b/internal/engine/netx/quicdialer/connectionstate.go deleted file mode 100644 index 847bef4b9b..0000000000 --- a/internal/engine/netx/quicdialer/connectionstate.go +++ /dev/null @@ -1,12 +0,0 @@ -package quicdialer - -import ( - "crypto/tls" - - "github.com/lucas-clemente/quic-go" -) - -// connectionState returns the ConnectionState of a QUIC Session. -func connectionState(sess quic.EarlyConnection) tls.ConnectionState { - return sess.ConnectionState().TLS.ConnectionState -} diff --git a/internal/engine/netx/quicdialer/saver.go b/internal/engine/netx/quicdialer/saver.go index b7640d3a5a..470f8fe801 100644 --- a/internal/engine/netx/quicdialer/saver.go +++ b/internal/engine/netx/quicdialer/saver.go @@ -61,3 +61,8 @@ func (h HandshakeSaver) DialContext(ctx context.Context, network string, }) return sess, nil } + +// connectionState returns the ConnectionState of a QUIC Session. +func connectionState(sess quic.EarlyConnection) tls.ConnectionState { + return sess.ConnectionState().TLS.ConnectionState +} diff --git a/internal/engine/netx/resolver/bogon.go b/internal/engine/netx/resolver/bogon.go deleted file mode 100644 index e308688dd0..0000000000 --- a/internal/engine/netx/resolver/bogon.go +++ /dev/null @@ -1,32 +0,0 @@ -package resolver - -import ( - "context" - - "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/netxlite" -) - -// BogonResolver is a bogon aware resolver. When a bogon is encountered in -// a reply, this resolver will return an error. -// -// Deprecation warning -// -// This resolver is deprecated. The right thing to do would be to check -// for bogons right after a domain name resolution in the nettest. -type BogonResolver struct { - model.Resolver -} - -// LookupHost implements Resolver.LookupHost -func (r BogonResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) { - addrs, err := r.Resolver.LookupHost(ctx, hostname) - for _, addr := range addrs { - if netxlite.IsBogon(addr) { - return nil, netxlite.ErrDNSBogon - } - } - return addrs, err -} - -var _ model.Resolver = BogonResolver{} diff --git a/internal/engine/netx/resolver/bogon_test.go b/internal/engine/netx/resolver/bogon_test.go deleted file mode 100644 index ad98722641..0000000000 --- a/internal/engine/netx/resolver/bogon_test.go +++ /dev/null @@ -1,37 +0,0 @@ -package resolver_test - -import ( - "context" - "errors" - "testing" - - "github.com/ooni/probe-cli/v3/internal/engine/netx/resolver" - "github.com/ooni/probe-cli/v3/internal/netxlite" -) - -func TestBogonAwareResolverWithBogon(t *testing.T) { - r := resolver.BogonResolver{ - Resolver: resolver.NewFakeResolverWithResult([]string{"127.0.0.1"}), - } - addrs, err := r.LookupHost(context.Background(), "dns.google.com") - if !errors.Is(err, netxlite.ErrDNSBogon) { - t.Fatal("not the error we expected") - } - if len(addrs) > 0 { - t.Fatal("expected to see nil here") - } -} - -func TestBogonAwareResolverWithoutBogon(t *testing.T) { - orig := []string{"8.8.8.8"} - r := resolver.BogonResolver{ - Resolver: resolver.NewFakeResolverWithResult(orig), - } - addrs, err := r.LookupHost(context.Background(), "dns.google.com") - if err != nil { - t.Fatal(err) - } - if len(addrs) != len(orig) || addrs[0] != orig[0] { - t.Fatal("not the error we expected") - } -} diff --git a/internal/netxlite/bogon.go b/internal/netxlite/bogon.go index 7d954371bd..7a9b2d8097 100644 --- a/internal/netxlite/bogon.go +++ b/internal/netxlite/bogon.go @@ -7,11 +7,62 @@ package netxlite // import ( + "context" "net" + "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/runtimex" ) +// BogonResolver is a bogon aware resolver. When a bogon is encountered in +// a reply, this resolver will return ErrDNSBogon. +type BogonResolver struct { + Resolver model.Resolver +} + +var _ model.Resolver = &BogonResolver{} + +// LookupHost implements Resolver.LookupHost +func (r *BogonResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) { + addrs, err := r.Resolver.LookupHost(ctx, hostname) + if err != nil { + return nil, err + } + for _, addr := range addrs { + if IsBogon(addr) { + return nil, ErrDNSBogon + } + } + return addrs, nil +} + +// LookupHTTPS implements Resolver.LookupHTTPS +func (r *BogonResolver) LookupHTTPS(ctx context.Context, hostname string) (*model.HTTPSSvc, error) { + // TODO(bassosimone): decide whether we want to implement this method or not + return nil, ErrNoDNSTransport +} + +// LookupNS implements Resolver.LookupNS +func (r *BogonResolver) LookupNS(ctx context.Context, hostname string) ([]*net.NS, error) { + // TODO(bassosimone): decide whether we want to implement this method or not + return nil, ErrNoDNSTransport +} + +// Network implements Resolver.Network +func (r *BogonResolver) Network() string { + return r.Resolver.Network() +} + +// Address implements Resolver.Address +func (r *BogonResolver) Address() string { + return r.Resolver.Address() +} + +// CloseIdleConnections implements Resolver.CloseIdleConnections +func (r *BogonResolver) CloseIdleConnections() { + r.Resolver.CloseIdleConnections() +} + // IsBogon returns whether an IP address is bogon. Passing to this // function a non-IP address causes it to return true. func IsBogon(address string) bool { diff --git a/internal/netxlite/bogon_test.go b/internal/netxlite/bogon_test.go index 86666d1ca4..cd5b1fcd2a 100644 --- a/internal/netxlite/bogon_test.go +++ b/internal/netxlite/bogon_test.go @@ -1,6 +1,140 @@ package netxlite -import "testing" +import ( + "context" + "errors" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/model/mocks" +) + +func TestBogonResolver(t *testing.T) { + t.Run("LookupHost", func(t *testing.T) { + t.Run("with failure", func(t *testing.T) { + expected := errors.New("mocked") + reso := &BogonResolver{ + Resolver: &mocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + return nil, expected + }, + }, + } + ctx := context.Background() + addrs, err := reso.LookupHost(ctx, "dns.google") + if !errors.Is(err, expected) { + t.Fatal("unexpected err", err) + } + if len(addrs) > 0 { + t.Fatal("expected no addrs") + } + }) + + t.Run("with success and no bogon", func(t *testing.T) { + expected := []string{"8.8.8.8", "149.112.112.112"} + reso := &BogonResolver{ + Resolver: &mocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + return expected, nil + }, + }, + } + ctx := context.Background() + addrs, err := reso.LookupHost(ctx, "dns.google") + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(expected, addrs); diff != "" { + t.Fatal(diff) + } + }) + + t.Run("with success and bogon", func(t *testing.T) { + reso := &BogonResolver{ + Resolver: &mocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + return []string{"8.8.8.8", "10.34.34.35", "149.112.112.112"}, nil + }, + }, + } + ctx := context.Background() + addrs, err := reso.LookupHost(ctx, "dns.google") + if !errors.Is(err, ErrDNSBogon) { + t.Fatal("unexpected err", err) + } + if len(addrs) > 0 { + t.Fatal("expected no addrs") + } + }) + }) + + t.Run("LookupHTTPS", func(t *testing.T) { + ctx := context.Background() + reso := &BogonResolver{} + https, err := reso.LookupHTTPS(ctx, "dns.google") + if !errors.Is(err, ErrNoDNSTransport) { + t.Fatal("unexpected err", err) + } + if https != nil { + t.Fatal("expected nil https here") + } + }) + + t.Run("LookupNS", func(t *testing.T) { + ctx := context.Background() + reso := &BogonResolver{} + ns, err := reso.LookupNS(ctx, "dns.google") + if !errors.Is(err, ErrNoDNSTransport) { + t.Fatal("unexpected err", err) + } + if len(ns) > 0 { + t.Fatal("expected empty ns here") + } + }) + + t.Run("Network", func(t *testing.T) { + expected := "antani" + reso := &BogonResolver{ + Resolver: &mocks.Resolver{ + MockNetwork: func() string { + return expected + }, + }, + } + if reso.Network() != expected { + t.Fatal("unexpected network") + } + }) + + t.Run("Address", func(t *testing.T) { + expected := "antani" + reso := &BogonResolver{ + Resolver: &mocks.Resolver{ + MockAddress: func() string { + return expected + }, + }, + } + if reso.Address() != expected { + t.Fatal("unexpected address") + } + }) + + t.Run("CloseIdleConnections", func(t *testing.T) { + var called bool + reso := &BogonResolver{ + Resolver: &mocks.Resolver{ + MockCloseIdleConnections: func() { + called = true + }, + }, + } + reso.CloseIdleConnections() + if !called { + t.Fatal("not called") + } + }) +} func TestIsBogon(t *testing.T) { if IsBogon("antani") != true { diff --git a/internal/netxlite/legacy.go b/internal/netxlite/legacy.go index 8d72f9773d..6bbf6f1f12 100644 --- a/internal/netxlite/legacy.go +++ b/internal/netxlite/legacy.go @@ -20,6 +20,7 @@ var ( type ( DialerResolver = dialerResolver DialerLogger = dialerLogger + HTTPTransportWrapper = httpTransportConnectionsCloser HTTPTransportLogger = httpTransportLogger ErrorWrapperDialer = dialerErrWrapper ErrorWrapperQUICListener = quicListenerErrWrapper