From 2502a237fb5e2dd3dc4e0db23dd19eabb292f6a1 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 6 Jun 2022 14:46:44 +0200 Subject: [PATCH] cleanup: netx does not use netxlite legacy names (#801) This diff refactors netx and netxlite to ensure we're not using netxlite legacy names inside of netx. To this end, we're cheating a bit. We're exposing a new factory to get an unwrapped stdlib resolver rather than defining a legacy name to export the private name of the same factory. This is actually a fine place to stop, for now, the next and netxlite refactoring at https://github.com/ooni/probe/issues/2121. --- .../internal/webconnectivity/webconnectivity_test.go | 2 +- internal/engine/experiment/ndt7/dial.go | 2 +- internal/engine/geolocate/geolocate.go | 2 +- internal/engine/geolocate/iplookup_test.go | 6 +++--- internal/engine/netx/dnstransport.go | 2 +- internal/engine/netx/resolver.go | 2 +- internal/measurex/resolver.go | 2 +- internal/measurex/tracing.go | 2 +- internal/netxlite/dialer.go | 4 ++-- internal/netxlite/dialer_test.go | 2 +- internal/netxlite/http.go | 2 +- internal/netxlite/http_test.go | 2 +- internal/netxlite/integration_test.go | 10 +++++----- internal/netxlite/legacy.go | 12 ------------ internal/netxlite/quic_test.go | 8 ++++---- internal/netxlite/resolvercore.go | 11 +++++++---- internal/netxlite/resolvercore_test.go | 2 +- internal/tutorial/netxlite/chapter05/README.md | 2 +- internal/tutorial/netxlite/chapter05/main.go | 2 +- 19 files changed, 34 insertions(+), 43 deletions(-) delete mode 100644 internal/netxlite/legacy.go diff --git a/internal/cmd/oohelperd/internal/webconnectivity/webconnectivity_test.go b/internal/cmd/oohelperd/internal/webconnectivity/webconnectivity_test.go index 6d1d49e..b70d10f 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/webconnectivity_test.go +++ b/internal/cmd/oohelperd/internal/webconnectivity/webconnectivity_test.go @@ -54,7 +54,7 @@ func TestWorkingAsIntended(t *testing.T) { Client: http.DefaultClient, Dialer: netxlite.NewDialerWithStdlibResolver(model.DiscardLogger), MaxAcceptableBody: 1 << 24, - Resolver: netxlite.NewResolverSystem(), + Resolver: netxlite.NewUnwrappedStdlibResolver(), } srv := httptest.NewServer(handler) defer srv.Close() diff --git a/internal/engine/experiment/ndt7/dial.go b/internal/engine/experiment/ndt7/dial.go index e81e941..7569420 100644 --- a/internal/engine/experiment/ndt7/dial.go +++ b/internal/engine/experiment/ndt7/dial.go @@ -30,7 +30,7 @@ func newDialManager(ndt7URL string, logger model.Logger, userAgent string) dialM } func (mgr dialManager) dialWithTestName(ctx context.Context, testName string) (*websocket.Conn, error) { - reso := netxlite.NewResolverStdlib(mgr.logger) + reso := netxlite.NewStdlibResolver(mgr.logger) dlr := netxlite.NewDialerWithResolver(mgr.logger, reso) dlr = bytecounter.WrapWithContextAwareDialer(dlr) // Implements shaping if the user builds using `-tags shaping` diff --git a/internal/engine/geolocate/geolocate.go b/internal/engine/geolocate/geolocate.go index 537023b..5776b14 100644 --- a/internal/engine/geolocate/geolocate.go +++ b/internal/engine/geolocate/geolocate.go @@ -114,7 +114,7 @@ func NewTask(config Config) *Task { config.UserAgent = fmt.Sprintf("ooniprobe-engine/%s", version.Version) } if config.Resolver == nil { - config.Resolver = netxlite.NewResolverStdlib(config.Logger) + config.Resolver = netxlite.NewStdlibResolver(config.Logger) } return &Task{ countryLookupper: mmdbLookupper{}, diff --git a/internal/engine/geolocate/iplookup_test.go b/internal/engine/geolocate/iplookup_test.go index 0cba122..15fb768 100644 --- a/internal/engine/geolocate/iplookup_test.go +++ b/internal/engine/geolocate/iplookup_test.go @@ -14,7 +14,7 @@ import ( func TestIPLookupGood(t *testing.T) { ip, err := (ipLookupClient{ Logger: log.Log, - Resolver: netxlite.NewResolverStdlib(model.DiscardLogger), + Resolver: netxlite.NewStdlibResolver(model.DiscardLogger), UserAgent: "ooniprobe-engine/0.1.0", }).LookupProbeIP(context.Background()) if err != nil { @@ -30,7 +30,7 @@ func TestIPLookupAllFailed(t *testing.T) { cancel() // immediately cancel to cause Do() to fail ip, err := (ipLookupClient{ Logger: log.Log, - Resolver: netxlite.NewResolverStdlib(model.DiscardLogger), + Resolver: netxlite.NewStdlibResolver(model.DiscardLogger), UserAgent: "ooniprobe-engine/0.1.0", }).LookupProbeIP(ctx) if !errors.Is(err, context.Canceled) { @@ -45,7 +45,7 @@ func TestIPLookupInvalidIP(t *testing.T) { ctx := context.Background() ip, err := (ipLookupClient{ Logger: log.Log, - Resolver: netxlite.NewResolverStdlib(model.DiscardLogger), + Resolver: netxlite.NewStdlibResolver(model.DiscardLogger), UserAgent: "ooniprobe-engine/0.1.0", }).doWithCustomFunc(ctx, invalidIPLookup) if !errors.Is(err, ErrInvalidIPAddress) { diff --git a/internal/engine/netx/dnstransport.go b/internal/engine/netx/dnstransport.go index b473dc9..3812e34 100644 --- a/internal/engine/netx/dnstransport.go +++ b/internal/engine/netx/dnstransport.go @@ -65,7 +65,7 @@ func NewDNSClientWithOverrides(config Config, URL, hostOverride, SNIOverride, } switch resolverURL.Scheme { case "system": - return netxlite.NewResolverSystem(), nil + return netxlite.NewUnwrappedStdlibResolver(), nil case "https": config.TLSConfig.NextProtos = []string{"h2", "http/1.1"} httpClient := &http.Client{Transport: NewHTTPTransport(config)} diff --git a/internal/engine/netx/resolver.go b/internal/engine/netx/resolver.go index d26329d..24e587d 100644 --- a/internal/engine/netx/resolver.go +++ b/internal/engine/netx/resolver.go @@ -12,7 +12,7 @@ import ( // NewResolver creates a new resolver from the specified config. func NewResolver(config Config) model.Resolver { if config.BaseResolver == nil { - config.BaseResolver = netxlite.NewResolverSystem() + config.BaseResolver = netxlite.NewUnwrappedStdlibResolver() } r := netxlite.WrapResolver( model.ValidLoggerOrDefault(config.Logger), diff --git a/internal/measurex/resolver.go b/internal/measurex/resolver.go index 9cfe3f9..bb508fa 100644 --- a/internal/measurex/resolver.go +++ b/internal/measurex/resolver.go @@ -28,7 +28,7 @@ func WrapResolver(begin time.Time, db WritableDB, r model.Resolver) model.Resolv // NewResolverSystem creates a system resolver and then wraps // it using the WrapResolver function. func (mx *Measurer) NewResolverSystem(db WritableDB, logger model.Logger) model.Resolver { - return mx.WrapResolver(db, netxlite.NewResolverStdlib(logger)) + return mx.WrapResolver(db, netxlite.NewStdlibResolver(logger)) } // NewResolverUDP is a convenience factory for creating a Resolver diff --git a/internal/measurex/tracing.go b/internal/measurex/tracing.go index f78de63..12741b9 100644 --- a/internal/measurex/tracing.go +++ b/internal/measurex/tracing.go @@ -54,7 +54,7 @@ func NewTracingHTTPTransport(logger model.Logger, begin time.Time, db WritableDB func NewTracingHTTPTransportWithDefaultSettings( begin time.Time, logger model.Logger, db WritableDB) *HTTPTransportDB { return NewTracingHTTPTransport(logger, begin, db, - netxlite.NewResolverStdlib(logger), + netxlite.NewStdlibResolver(logger), netxlite.NewDialerWithoutResolver(logger), netxlite.NewTLSHandshakerStdlib(logger), DefaultHTTPMaxBodySnapshotSize) diff --git a/internal/netxlite/dialer.go b/internal/netxlite/dialer.go index ecb3303..91ac454 100644 --- a/internal/netxlite/dialer.go +++ b/internal/netxlite/dialer.go @@ -15,10 +15,10 @@ import ( ) // NewDialerWithStdlibResolver is equivalent to creating a system resolver -// using NewResolverStdlib and then a dialer using NewDialerWithResolver where +// using NewStdlibResolver and then a dialer using NewDialerWithResolver where // the resolver argument is the previously created resolver. func NewDialerWithStdlibResolver(dl model.DebugLogger) model.Dialer { - reso := NewResolverStdlib(dl) + reso := NewStdlibResolver(dl) return NewDialerWithResolver(dl, reso) } diff --git a/internal/netxlite/dialer_test.go b/internal/netxlite/dialer_test.go index d4c308d..701bb29 100644 --- a/internal/netxlite/dialer_test.go +++ b/internal/netxlite/dialer_test.go @@ -141,7 +141,7 @@ func TestDialerResolver(t *testing.T) { t.Run("fails without a port", func(t *testing.T) { d := &dialerResolver{ Dialer: &DialerSystem{}, - Resolver: newResolverSystem(), + Resolver: NewUnwrappedStdlibResolver(), } const missingPort = "ooni.nu" conn, err := d.DialContext(context.Background(), "tcp", missingPort) diff --git a/internal/netxlite/http.go b/internal/netxlite/http.go index 05464e3..6d583cb 100644 --- a/internal/netxlite/http.go +++ b/internal/netxlite/http.go @@ -323,7 +323,7 @@ func (c *httpTLSConnWithReadTimeout) Read(b []byte) (int, error) { // This factory and NewHTTPTransport are the recommended // ways of creating a new HTTPTransport. func NewHTTPTransportStdlib(logger model.DebugLogger) model.HTTPTransport { - dialer := NewDialerWithResolver(logger, NewResolverStdlib(logger)) + dialer := NewDialerWithResolver(logger, NewStdlibResolver(logger)) tlsDialer := NewTLSDialer(dialer, NewTLSHandshakerStdlib(logger)) return NewHTTPTransport(logger, dialer, tlsDialer) } diff --git a/internal/netxlite/http_test.go b/internal/netxlite/http_test.go index d4946cf..6c19f53 100644 --- a/internal/netxlite/http_test.go +++ b/internal/netxlite/http_test.go @@ -236,7 +236,7 @@ func TestNewHTTPTransport(t *testing.T) { called.Add(1) }, }, - Resolver: NewResolverStdlib(log.Log), + Resolver: NewStdlibResolver(log.Log), } td := NewTLSDialer(d, NewTLSHandshakerStdlib(log.Log)) txp := NewHTTPTransport(log.Log, d, td) diff --git a/internal/netxlite/integration_test.go b/internal/netxlite/integration_test.go index 5ff1915..4b4d2f3 100644 --- a/internal/netxlite/integration_test.go +++ b/internal/netxlite/integration_test.go @@ -41,7 +41,7 @@ func TestMeasureWithSystemResolver(t *testing.T) { // t.Run("on success", func(t *testing.T) { - r := netxlite.NewResolverStdlib(log.Log) + r := netxlite.NewStdlibResolver(log.Log) defer r.CloseIdleConnections() ctx := context.Background() addrs, err := r.LookupHost(ctx, "dns.google.com") @@ -54,7 +54,7 @@ func TestMeasureWithSystemResolver(t *testing.T) { }) t.Run("for nxdomain", func(t *testing.T) { - r := netxlite.NewResolverStdlib(log.Log) + r := netxlite.NewStdlibResolver(log.Log) defer r.CloseIdleConnections() ctx := context.Background() addrs, err := r.LookupHost(ctx, "antani.ooni.org") @@ -67,7 +67,7 @@ func TestMeasureWithSystemResolver(t *testing.T) { }) t.Run("for timeout", func(t *testing.T) { - r := netxlite.NewResolverStdlib(log.Log) + r := netxlite.NewStdlibResolver(log.Log) defer r.CloseIdleConnections() const timeout = time.Nanosecond ctx, cancel := context.WithTimeout(context.Background(), timeout) @@ -472,7 +472,7 @@ func TestHTTPTransport(t *testing.T) { } t.Run("works as intended", func(t *testing.T) { - d := netxlite.NewDialerWithResolver(log.Log, netxlite.NewResolverStdlib(log.Log)) + d := netxlite.NewDialerWithResolver(log.Log, netxlite.NewStdlibResolver(log.Log)) td := netxlite.NewTLSDialer(d, netxlite.NewTLSHandshakerStdlib(log.Log)) txp := netxlite.NewHTTPTransport(log.Log, d, td) client := &http.Client{Transport: txp} @@ -523,7 +523,7 @@ func TestHTTP3Transport(t *testing.T) { d := netxlite.NewQUICDialerWithResolver( netxlite.NewQUICListener(), log.Log, - netxlite.NewResolverStdlib(log.Log), + netxlite.NewStdlibResolver(log.Log), ) txp := netxlite.NewHTTP3Transport(log.Log, d, &tls.Config{}) client := &http.Client{Transport: txp} diff --git a/internal/netxlite/legacy.go b/internal/netxlite/legacy.go deleted file mode 100644 index a9c5b6d..0000000 --- a/internal/netxlite/legacy.go +++ /dev/null @@ -1,12 +0,0 @@ -package netxlite - -// -// Legacy code -// - -// These vars export internal names to legacy ooni/probe-cli code. -// -// Deprecated: do not use these names in new code. -var ( - NewResolverSystem = newResolverSystem -) diff --git a/internal/netxlite/quic_test.go b/internal/netxlite/quic_test.go index 60c3a77..701322d 100644 --- a/internal/netxlite/quic_test.go +++ b/internal/netxlite/quic_test.go @@ -486,7 +486,7 @@ func TestQUICDialerResolver(t *testing.T) { t.Run("with missing port", func(t *testing.T) { tlsConfig := &tls.Config{} dialer := &quicDialerResolver{ - Resolver: NewResolverStdlib(log.Log), + Resolver: NewStdlibResolver(log.Log), Dialer: &quicDialerQUICGo{}} qconn, err := dialer.DialContext( context.Background(), "udp", "www.google.com", @@ -523,7 +523,7 @@ func TestQUICDialerResolver(t *testing.T) { // to establish a connection leads to a failure tlsConf := &tls.Config{} dialer := &quicDialerResolver{ - Resolver: NewResolverStdlib(log.Log), + Resolver: NewStdlibResolver(log.Log), Dialer: &quicDialerQUICGo{ QUICListener: &quicListenerStdlib{}, }} @@ -546,7 +546,7 @@ func TestQUICDialerResolver(t *testing.T) { var gotTLSConfig *tls.Config tlsConfig := &tls.Config{} dialer := &quicDialerResolver{ - Resolver: NewResolverStdlib(log.Log), + Resolver: NewStdlibResolver(log.Log), Dialer: &mocks.QUICDialer{ MockDialContext: func(ctx context.Context, network, address string, tlsConfig *tls.Config, quicConfig *quic.Config) (quic.EarlyConnection, error) { @@ -574,7 +574,7 @@ func TestQUICDialerResolver(t *testing.T) { t.Run("on success", func(t *testing.T) { expectedQConn := &mocks.QUICEarlyConnection{} dialer := &quicDialerResolver{ - Resolver: NewResolverStdlib(log.Log), + Resolver: NewStdlibResolver(log.Log), Dialer: &mocks.QUICDialer{ MockDialContext: func(ctx context.Context, network, address string, tlsConfig *tls.Config, quicConfig *quic.Config) (quic.EarlyConnection, error) { diff --git a/internal/netxlite/resolvercore.go b/internal/netxlite/resolvercore.go index b6149df..08c81f4 100644 --- a/internal/netxlite/resolvercore.go +++ b/internal/netxlite/resolvercore.go @@ -23,12 +23,12 @@ import ( // but you are using the "system" resolver instead. var ErrNoDNSTransport = errors.New("operation requires a DNS transport") -// NewResolverStdlib creates a new Resolver by combining WrapResolver +// NewStdlibResolver creates a new Resolver by combining WrapResolver // with an internal "system" resolver type. The list of optional wrappers // allow to wrap the underlying getaddrinfo transport. Any nil wrapper // will be silently ignored by the code that performs the wrapping. -func NewResolverStdlib(logger model.DebugLogger, wrappers ...model.DNSTransportWrapper) model.Resolver { - return WrapResolver(logger, newResolverSystem(wrappers...)) +func NewStdlibResolver(logger model.DebugLogger, wrappers ...model.DNSTransportWrapper) model.Resolver { + return WrapResolver(logger, NewUnwrappedStdlibResolver(wrappers...)) } // NewParallelDNSOverHTTPSResolver creates a new DNS over HTTPS resolver @@ -40,7 +40,10 @@ func NewParallelDNSOverHTTPSResolver(logger model.DebugLogger, URL string) model return WrapResolver(logger, NewUnwrappedParallelResolver(txp)) } -func newResolverSystem(wrappers ...model.DNSTransportWrapper) *resolverSystem { +// NewUnwrappedStdlibResolver returns a new, unwrapped resolver using the standard +// library (i.e., getaddrinfo if possible and &net.Resolver{} otherwise). As the name +// implies, this function returns an unwrapped resolver. +func NewUnwrappedStdlibResolver(wrappers ...model.DNSTransportWrapper) model.Resolver { return &resolverSystem{ t: WrapDNSTransport(&dnsOverGetaddrinfoTransport{}, wrappers...), } diff --git a/internal/netxlite/resolvercore_test.go b/internal/netxlite/resolvercore_test.go index 55bbf5e..6ea5c5a 100644 --- a/internal/netxlite/resolvercore_test.go +++ b/internal/netxlite/resolvercore_test.go @@ -29,7 +29,7 @@ func typecheckForSystemResolver(t *testing.T, resolver model.Resolver, logger mo } func TestNewResolverSystem(t *testing.T) { - resolver := NewResolverStdlib(model.DiscardLogger) + resolver := NewStdlibResolver(model.DiscardLogger) typecheckForSystemResolver(t, resolver, model.DiscardLogger) } diff --git a/internal/tutorial/netxlite/chapter05/README.md b/internal/tutorial/netxlite/chapter05/README.md index ef0a106..6660e94 100644 --- a/internal/tutorial/netxlite/chapter05/README.md +++ b/internal/tutorial/netxlite/chapter05/README.md @@ -51,7 +51,7 @@ The returned resolver implements an interface that is very close to the API of the `net.Resolver` struct. ```Go - reso := netxlite.NewResolverStdlib(log.Log) + reso := netxlite.NewStdlibResolver(log.Log) ``` We call `LookupHost` to map the hostname to IP addrs. The returned diff --git a/internal/tutorial/netxlite/chapter05/main.go b/internal/tutorial/netxlite/chapter05/main.go index b4ff34c..cf0c0ea 100644 --- a/internal/tutorial/netxlite/chapter05/main.go +++ b/internal/tutorial/netxlite/chapter05/main.go @@ -52,7 +52,7 @@ func main() { // close to the API of the `net.Resolver` struct. // // ```Go - reso := netxlite.NewResolverStdlib(log.Log) + reso := netxlite.NewStdlibResolver(log.Log) // ``` // // We call `LookupHost` to map the hostname to IP addrs. The returned