From 2d3d5d9cdc760e986b31a7dc87604581b8f9b596 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 5 Jun 2022 19:52:39 +0200 Subject: [PATCH] cleanup(netx): stop using most netxlite resolver internals (#797) This diff modifies netx to stop using most netxlite resolver internals but the internal function that creates a new, unwrapped system resolver, which will be dealt with in a subsequent pull request. See https://github.com/ooni/probe/issues/2121 --- internal/engine/netx/netx.go | 18 +-- internal/engine/netx/netx_test.go | 216 ------------------------------ internal/netxlite/bogon.go | 11 +- internal/netxlite/bogon_test.go | 4 + internal/netxlite/legacy.go | 12 -- 5 files changed, 18 insertions(+), 243 deletions(-) diff --git a/internal/engine/netx/netx.go b/internal/engine/netx/netx.go index aede297..c12f1bd 100644 --- a/internal/engine/netx/netx.go +++ b/internal/engine/netx/netx.go @@ -63,10 +63,10 @@ func NewResolver(config Config) model.Resolver { if config.BaseResolver == nil { config.BaseResolver = netxlite.NewResolverSystem() } - var r model.Resolver = config.BaseResolver - r = &netxlite.AddressResolver{ - Resolver: r, - } + r := netxlite.WrapResolver( + model.ValidLoggerOrDefault(config.Logger), + config.BaseResolver, + ) if config.CacheResolutions { r = &CacheResolver{Resolver: r} } @@ -80,15 +80,7 @@ func NewResolver(config Config) model.Resolver { if config.BogonIsError { r = &netxlite.BogonResolver{Resolver: r} } - r = &netxlite.ErrorWrapperResolver{Resolver: r} - if config.Logger != nil { - r = &netxlite.ResolverLogger{ - Logger: config.Logger, - Resolver: r, - } - } - r = config.Saver.WrapResolver(r) // WAI when config.Saver==nil - return &netxlite.ResolverIDNA{Resolver: r} + return config.Saver.WrapResolver(r) // WAI when config.Saver==nil } // NewDialer creates a new Dialer from the specified config diff --git a/internal/engine/netx/netx_test.go b/internal/engine/netx/netx_test.go index ef0a8b9..7a812a2 100644 --- a/internal/engine/netx/netx_test.go +++ b/internal/engine/netx/netx_test.go @@ -17,198 +17,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/tracex" ) -func TestNewResolverVanilla(t *testing.T) { - r := NewResolver(Config{}) - ir, ok := r.(*netxlite.ResolverIDNA) - if !ok { - t.Fatal("not the resolver we expected") - } - ewr, ok := ir.Resolver.(*netxlite.ErrorWrapperResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - ar, ok := ewr.Resolver.(*netxlite.AddressResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - _, ok = ar.Resolver.(*netxlite.ResolverSystemDoNotInstantiate) - if !ok { - t.Fatal("not the resolver we expected") - } -} - -func TestNewResolverSpecificResolver(t *testing.T) { - r := NewResolver(Config{ - BaseResolver: &netxlite.BogonResolver{ - // not initialized because it doesn't matter in this context - }, - }) - ir, ok := r.(*netxlite.ResolverIDNA) - if !ok { - t.Fatal("not the resolver we expected") - } - ewr, ok := ir.Resolver.(*netxlite.ErrorWrapperResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - ar, ok := ewr.Resolver.(*netxlite.AddressResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - _, ok = ar.Resolver.(*netxlite.BogonResolver) - if !ok { - t.Fatal("not the resolver we expected") - } -} - -func TestNewResolverWithBogonFilter(t *testing.T) { - r := NewResolver(Config{ - BogonIsError: true, - }) - ir, ok := r.(*netxlite.ResolverIDNA) - if !ok { - t.Fatal("not the resolver we expected") - } - ewr, ok := ir.Resolver.(*netxlite.ErrorWrapperResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - br, ok := ewr.Resolver.(*netxlite.BogonResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - ar, ok := br.Resolver.(*netxlite.AddressResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - _, ok = ar.Resolver.(*netxlite.ResolverSystemDoNotInstantiate) - if !ok { - t.Fatal("not the resolver we expected") - } -} - -func TestNewResolverWithLogging(t *testing.T) { - r := NewResolver(Config{ - Logger: log.Log, - }) - ir, ok := r.(*netxlite.ResolverIDNA) - if !ok { - t.Fatal("not the resolver we expected") - } - lr, ok := ir.Resolver.(*netxlite.ResolverLogger) - if !ok { - t.Fatal("not the resolver we expected") - } - if lr.Logger != log.Log { - t.Fatal("not the logger we expected") - } - ewr, ok := lr.Resolver.(*netxlite.ErrorWrapperResolver) - if !ok { - t.Fatalf("not the resolver we expected") - } - ar, ok := ewr.Resolver.(*netxlite.AddressResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - _, ok = ar.Resolver.(*netxlite.ResolverSystemDoNotInstantiate) - if !ok { - t.Fatal("not the resolver we expected") - } -} - -func TestNewResolverWithSaver(t *testing.T) { - saver := new(tracex.Saver) - r := NewResolver(Config{ - Saver: saver, - }) - ir, ok := r.(*netxlite.ResolverIDNA) - if !ok { - t.Fatal("not the resolver we expected") - } - sr, ok := ir.Resolver.(*tracex.ResolverSaver) - if !ok { - t.Fatal("not the resolver we expected") - } - if sr.Saver != saver { - t.Fatal("not the saver we expected") - } - ewr, ok := sr.Resolver.(*netxlite.ErrorWrapperResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - ar, ok := ewr.Resolver.(*netxlite.AddressResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - _, ok = ar.Resolver.(*netxlite.ResolverSystemDoNotInstantiate) - if !ok { - t.Fatal("not the resolver we expected") - } -} - -func TestNewResolverWithReadWriteCache(t *testing.T) { - r := NewResolver(Config{ - CacheResolutions: true, - }) - ir, ok := r.(*netxlite.ResolverIDNA) - if !ok { - t.Fatal("not the resolver we expected") - } - ewr, ok := ir.Resolver.(*netxlite.ErrorWrapperResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - cr, ok := ewr.Resolver.(*CacheResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - if cr.ReadOnly != false { - t.Fatal("expected readwrite cache here") - } - ar, ok := cr.Resolver.(*netxlite.AddressResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - _, ok = ar.Resolver.(*netxlite.ResolverSystemDoNotInstantiate) - if !ok { - t.Fatal("not the resolver we expected") - } -} - -func TestNewResolverWithPrefilledReadonlyCache(t *testing.T) { - r := NewResolver(Config{ - DNSCache: map[string][]string{ - "dns.google.com": {"8.8.8.8"}, - }, - }) - ir, ok := r.(*netxlite.ResolverIDNA) - if !ok { - t.Fatal("not the resolver we expected") - } - ewr, ok := ir.Resolver.(*netxlite.ErrorWrapperResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - cr, ok := ewr.Resolver.(*CacheResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - if cr.ReadOnly != true { - t.Fatal("expected readonly cache here") - } - if cr.Get("dns.google.com")[0] != "8.8.8.8" { - t.Fatal("cache not correctly prefilled") - } - ar, ok := cr.Resolver.(*netxlite.AddressResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - _, ok = ar.Resolver.(*netxlite.ResolverSystemDoNotInstantiate) - if !ok { - t.Fatal("not the resolver we expected") - } -} - func TestNewTLSDialer(t *testing.T) { t.Run("we always have error wrapping", func(t *testing.T) { server := filtering.NewTLSServer(filtering.TLSActionReset) @@ -345,30 +153,6 @@ func TestNewDNSClientUnsupportedScheme(t *testing.T) { } } -func TestNewDNSClientSystemResolver(t *testing.T) { - dnsclient, err := NewDNSClient( - Config{}, "system:///") - if err != nil { - t.Fatal(err) - } - if _, ok := dnsclient.(*netxlite.ResolverSystemDoNotInstantiate); !ok { - t.Fatal("not the resolver we expected") - } - dnsclient.CloseIdleConnections() -} - -func TestNewDNSClientEmpty(t *testing.T) { - dnsclient, err := NewDNSClient( - Config{}, "") - if err != nil { - t.Fatal(err) - } - if _, ok := dnsclient.(*netxlite.ResolverSystemDoNotInstantiate); !ok { - t.Fatal("not the resolver we expected") - } - dnsclient.CloseIdleConnections() -} - func TestNewDNSClientPowerdnsDoH(t *testing.T) { dnsclient, err := NewDNSClient( Config{}, "doh://powerdns") diff --git a/internal/netxlite/bogon.go b/internal/netxlite/bogon.go index 7a9b2d8..a905382 100644 --- a/internal/netxlite/bogon.go +++ b/internal/netxlite/bogon.go @@ -16,6 +16,12 @@ import ( // BogonResolver is a bogon aware resolver. When a bogon is encountered in // a reply, this resolver will return ErrDNSBogon. +// +// This resolver is not part of the default chain created by WrapResolver +// therefore it returns errors that have already been wrapped. +// +// BUG: This resolver currently only implements LookupHost. All the other +// lookup methods will always return ErrNoDNSTransport. type BogonResolver struct { Resolver model.Resolver } @@ -26,11 +32,12 @@ var _ model.Resolver = &BogonResolver{} func (r *BogonResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) { addrs, err := r.Resolver.LookupHost(ctx, hostname) if err != nil { - return nil, err + return nil, err // not our responsibility to wrap this error } for _, addr := range addrs { if IsBogon(addr) { - return nil, ErrDNSBogon + // wrap ErrDNSBogon as documented + return nil, newErrWrapper(classifyResolverError, ResolveOperation, ErrDNSBogon) } } return addrs, nil diff --git a/internal/netxlite/bogon_test.go b/internal/netxlite/bogon_test.go index cd5b1fc..139d8bc 100644 --- a/internal/netxlite/bogon_test.go +++ b/internal/netxlite/bogon_test.go @@ -62,6 +62,10 @@ func TestBogonResolver(t *testing.T) { if !errors.Is(err, ErrDNSBogon) { t.Fatal("unexpected err", err) } + var ew *ErrWrapper + if !errors.As(err, &ew) { + t.Fatal("error has not been wrapped") + } if len(addrs) > 0 { t.Fatal("expected no addrs") } diff --git a/internal/netxlite/legacy.go b/internal/netxlite/legacy.go index ea5ef80..a9c5b6d 100644 --- a/internal/netxlite/legacy.go +++ b/internal/netxlite/legacy.go @@ -9,16 +9,4 @@ package netxlite // Deprecated: do not use these names in new code. var ( NewResolverSystem = newResolverSystem - DefaultResolver = newResolverSystem() -) - -// These types export internal names to legacy ooni/probe-cli code. -// -// Deprecated: do not use these names in new code. -type ( - ErrorWrapperResolver = resolverErrWrapper - ResolverSystemDoNotInstantiate = resolverSystem // instantiate => crash w/ nil transport - ResolverLogger = resolverLogger - ResolverIDNA = resolverIDNA - AddressResolver = resolverShortCircuitIPAddr )