From caf25141dc184fde96c195b43743201e9e5501e7 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 11 Sep 2023 17:33:43 +0200 Subject: [PATCH 1/8] refactor(netxlite): use *Netx for the system resolver This code modifies how we construct netxlite's system resolver such that public functions use the *Netx equivalents. The general idea for which I am pushing here is to have additional clarity about dependencies, to better analyze the requirements of non measuring code for https://github.com/ooni/probe/issues/2531. --- internal/netxlite/dnsovergetaddrinfo.go | 7 ++++++- internal/netxlite/netx.go | 13 ++----------- internal/netxlite/resolvercore.go | 20 ++++++++++++++++---- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/internal/netxlite/dnsovergetaddrinfo.go b/internal/netxlite/dnsovergetaddrinfo.go index c41dc60c5e..5904b92964 100644 --- a/internal/netxlite/dnsovergetaddrinfo.go +++ b/internal/netxlite/dnsovergetaddrinfo.go @@ -27,9 +27,14 @@ type dnsOverGetaddrinfoTransport struct { provider *MaybeCustomUnderlyingNetwork } +func (netx *Netx) newDNSOverGetaddrinfoTransport() model.DNSTransport { + return &dnsOverGetaddrinfoTransport{provider: netx.tproxyNilSafeProvider()} +} + // NewDNSOverGetaddrinfoTransport creates a new dns-over-getaddrinfo transport. func NewDNSOverGetaddrinfoTransport() model.DNSTransport { - return &dnsOverGetaddrinfoTransport{} + netx := &Netx{Underlying: nil} + return netx.newDNSOverGetaddrinfoTransport() } var _ model.DNSTransport = &dnsOverGetaddrinfoTransport{} diff --git a/internal/netxlite/netx.go b/internal/netxlite/netx.go index 37a2961639..519b2d9848 100644 --- a/internal/netxlite/netx.go +++ b/internal/netxlite/netx.go @@ -18,17 +18,8 @@ type Netx struct { } // tproxyNilSafeProvider wraps the [model.UnderlyingNetwork] using a [tproxyNilSafeProvider]. -func (n *Netx) tproxyNilSafeProvider() *MaybeCustomUnderlyingNetwork { - return &MaybeCustomUnderlyingNetwork{n.Underlying} -} - -// NewStdlibResolver is like [netxlite.NewStdlibResolver] but the constructed [model.Resolver] -// uses the [model.UnderlyingNetwork] configured inside the [Netx] structure. -func (n *Netx) NewStdlibResolver(logger model.DebugLogger, wrappers ...model.DNSTransportWrapper) model.Resolver { - unwrapped := &resolverSystem{ - t: WrapDNSTransport(&dnsOverGetaddrinfoTransport{provider: n.tproxyNilSafeProvider()}, wrappers...), - } - return WrapResolver(logger, unwrapped) +func (netx *Netx) tproxyNilSafeProvider() *MaybeCustomUnderlyingNetwork { + return &MaybeCustomUnderlyingNetwork{netx.Underlying} } // NewDialerWithResolver is like [netxlite.NewDialerWithResolver] but the constructed [model.Dialer] diff --git a/internal/netxlite/resolvercore.go b/internal/netxlite/resolvercore.go index cb9940c6d8..ee5cffecc3 100644 --- a/internal/netxlite/resolvercore.go +++ b/internal/netxlite/resolvercore.go @@ -27,8 +27,15 @@ var ErrNoDNSTransport = errors.New("operation requires a DNS transport") // with an internal "stdlib" 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 (netx *Netx) NewStdlibResolver(logger model.DebugLogger, wrappers ...model.DNSTransportWrapper) model.Resolver { + return WrapResolver(logger, netx.newUnwrappedStdlibResolver(wrappers...)) +} + +// NewStdlibResolver is equivalent to creating an empty [*Netx] +// and callings its NewStdlibResolver method. func NewStdlibResolver(logger model.DebugLogger, wrappers ...model.DNSTransportWrapper) model.Resolver { - return WrapResolver(logger, NewUnwrappedStdlibResolver(wrappers...)) + netx := &Netx{Underlying: nil} + return netx.NewStdlibResolver(logger, wrappers...) } // NewParallelDNSOverHTTPSResolver creates a new DNS over HTTPS resolver @@ -40,13 +47,18 @@ func NewParallelDNSOverHTTPSResolver(logger model.DebugLogger, URL string) model return WrapResolver(logger, NewUnwrappedParallelResolver(txp)) } +func (netx *Netx) newUnwrappedStdlibResolver(wrappers ...model.DNSTransportWrapper) model.Resolver { + return &resolverSystem{ + t: WrapDNSTransport(netx.newDNSOverGetaddrinfoTransport(), wrappers...), + } +} + // 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(NewDNSOverGetaddrinfoTransport(), wrappers...), - } + netx := &Netx{Underlying: nil} + return netx.newUnwrappedStdlibResolver(wrappers...) } // NewSerialUDPResolver creates a new Resolver using DNS-over-UDP From b017ed84b3c9fbd0306ecd0a23f83ca99a991c78 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 11 Sep 2023 17:44:36 +0200 Subject: [PATCH 2/8] x --- internal/netxlite/dnsovergetaddrinfo.go | 2 +- internal/netxlite/netx.go | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/netxlite/dnsovergetaddrinfo.go b/internal/netxlite/dnsovergetaddrinfo.go index 5904b92964..e5a94c92aa 100644 --- a/internal/netxlite/dnsovergetaddrinfo.go +++ b/internal/netxlite/dnsovergetaddrinfo.go @@ -28,7 +28,7 @@ type dnsOverGetaddrinfoTransport struct { } func (netx *Netx) newDNSOverGetaddrinfoTransport() model.DNSTransport { - return &dnsOverGetaddrinfoTransport{provider: netx.tproxyNilSafeProvider()} + return &dnsOverGetaddrinfoTransport{provider: netx.maybeCustomUnderlyingNetwork()} } // NewDNSOverGetaddrinfoTransport creates a new dns-over-getaddrinfo transport. diff --git a/internal/netxlite/netx.go b/internal/netxlite/netx.go index 519b2d9848..b3d5687646 100644 --- a/internal/netxlite/netx.go +++ b/internal/netxlite/netx.go @@ -17,21 +17,21 @@ type Netx struct { Underlying model.UnderlyingNetwork } -// tproxyNilSafeProvider wraps the [model.UnderlyingNetwork] using a [tproxyNilSafeProvider]. -func (netx *Netx) tproxyNilSafeProvider() *MaybeCustomUnderlyingNetwork { +// maybeCustomUnderlyingNetwork wraps the [model.UnderlyingNetwork] using a [*MaybeCustomUnderlyingNetwork]. +func (netx *Netx) maybeCustomUnderlyingNetwork() *MaybeCustomUnderlyingNetwork { return &MaybeCustomUnderlyingNetwork{netx.Underlying} } // NewDialerWithResolver is like [netxlite.NewDialerWithResolver] but the constructed [model.Dialer] // uses the [model.UnderlyingNetwork] configured inside the [Netx] structure. func (n *Netx) NewDialerWithResolver(dl model.DebugLogger, r model.Resolver, w ...model.DialerWrapper) model.Dialer { - return WrapDialer(dl, r, &DialerSystem{provider: n.tproxyNilSafeProvider()}, w...) + return WrapDialer(dl, r, &DialerSystem{provider: n.maybeCustomUnderlyingNetwork()}, w...) } // NewUDPListener is like [netxlite.NewUDPListener] but the constructed [model.UDPListener] // uses the [model.UnderlyingNetwork] configured inside the [Netx] structure. func (n *Netx) NewUDPListener() model.UDPListener { - return &udpListenerErrWrapper{&udpListenerStdlib{provider: n.tproxyNilSafeProvider()}} + return &udpListenerErrWrapper{&udpListenerStdlib{provider: n.maybeCustomUnderlyingNetwork()}} } // NewQUICDialerWithResolver is like [netxlite.NewQUICDialerWithResolver] but the constructed @@ -40,7 +40,7 @@ func (n *Netx) NewQUICDialerWithResolver(listener model.UDPListener, logger mode resolver model.Resolver, wrappers ...model.QUICDialerWrapper) (outDialer model.QUICDialer) { baseDialer := &quicDialerQUICGo{ UDPListener: listener, - provider: n.tproxyNilSafeProvider(), + provider: n.maybeCustomUnderlyingNetwork(), } return WrapQUICDialer(logger, resolver, baseDialer, wrappers...) } @@ -48,7 +48,7 @@ func (n *Netx) NewQUICDialerWithResolver(listener model.UDPListener, logger mode // NewTLSHandshakerStdlib is like [netxlite.NewTLSHandshakerStdlib] but the constructed [model.TLSHandshaker] // uses the [model.UnderlyingNetwork] configured inside the [Netx] structure. func (n *Netx) NewTLSHandshakerStdlib(logger model.DebugLogger) model.TLSHandshaker { - return newTLSHandshakerLogger(&tlsHandshakerConfigurable{provider: n.tproxyNilSafeProvider()}, logger) + return newTLSHandshakerLogger(&tlsHandshakerConfigurable{provider: n.maybeCustomUnderlyingNetwork()}, logger) } // NewHTTPTransportStdlib is like [netxlite.NewHTTPTransportStdlib] but the constructed [model.HTTPTransport] From 8aeb5f8e79528dd3c90b5cb5ccc21b30b0b25030 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 11 Sep 2023 17:59:23 +0200 Subject: [PATCH 3/8] x --- internal/netxlite/dialer_test.go | 2 +- internal/netxlite/resolvercore_test.go | 55 ++++++++++++++++++++++---- 2 files changed, 48 insertions(+), 9 deletions(-) diff --git a/internal/netxlite/dialer_test.go b/internal/netxlite/dialer_test.go index 800db868c1..68e6379db9 100644 --- a/internal/netxlite/dialer_test.go +++ b/internal/netxlite/dialer_test.go @@ -24,7 +24,7 @@ func TestNewDialerWithStdlibResolver(t *testing.T) { } // typecheck the resolver reso := logger.Dialer.(*dialerResolverWithTracing) - typecheckForSystemResolver(t, reso.Resolver, model.DiscardLogger) + typecheckForSystemResolver(t, reso.Resolver, model.DiscardLogger) // FIXME // typecheck the dialer logger = reso.Dialer.(*dialerLogger) if logger.DebugLogger != model.DiscardLogger { diff --git a/internal/netxlite/resolvercore_test.go b/internal/netxlite/resolvercore_test.go index ac6d8652b5..eb6f385015 100644 --- a/internal/netxlite/resolvercore_test.go +++ b/internal/netxlite/resolvercore_test.go @@ -17,24 +17,63 @@ import ( "github.com/ooni/probe-cli/v3/internal/testingx" ) -func typecheckForSystemResolver(t *testing.T, resolver model.Resolver, logger model.DebugLogger) { +type dnsTransportWrapperForTesting struct { + txp model.DNSTransport +} + +var _ model.DNSTransport = &dnsTransportWrapperForTesting{} + +// Address implements model.DNSTransport. +func (t *dnsTransportWrapperForTesting) Address() string { + return t.txp.Address() +} + +// CloseIdleConnections implements model.DNSTransport. +func (t *dnsTransportWrapperForTesting) CloseIdleConnections() { + t.txp.CloseIdleConnections() +} + +// Network implements model.DNSTransport. +func (t *dnsTransportWrapperForTesting) Network() string { + return t.txp.Network() +} + +// RequiresPadding implements model.DNSTransport. +func (t *dnsTransportWrapperForTesting) RequiresPadding() bool { + return t.txp.RequiresPadding() +} + +// RoundTrip implements model.DNSTransport. +func (t *dnsTransportWrapperForTesting) RoundTrip(ctx context.Context, query model.DNSQuery) (model.DNSResponse, error) { + return t.txp.RoundTrip(ctx, query) +} + +type dnsTransportWrapperFactoryForTesting struct{} + +var _ model.DNSTransportWrapper = &dnsTransportWrapperFactoryForTesting{} + +// WrapDNSTransport implements model.DNSTransportWrapper. +func (*dnsTransportWrapperFactoryForTesting) WrapDNSTransport(txp model.DNSTransport) model.DNSTransport { + return &dnsTransportWrapperForTesting{txp} +} + +func TestNewResolverSystem(t *testing.T) { + // TODO(bassosimone): we need to extend this test to make sure we honour wrapping. + resolver := NewStdlibResolver(model.DiscardLogger, &dnsTransportWrapperFactoryForTesting{}) + idna := resolver.(*resolverIDNA) loggerReso := idna.Resolver.(*resolverLogger) - if loggerReso.Logger != logger { + if loggerReso.Logger != model.DiscardLogger { t.Fatal("invalid logger") } shortCircuit := loggerReso.Resolver.(*resolverShortCircuitIPAddr) errWrapper := shortCircuit.Resolver.(*resolverErrWrapper) reso := errWrapper.Resolver.(*resolverSystem) - txpErrWrapper := reso.t.(*dnsTransportErrWrapper) + txpExternal := reso.t.(*dnsTransportWrapperForTesting) + txpErrWrapper := txpExternal.txp.(*dnsTransportErrWrapper) _ = txpErrWrapper.DNSTransport.(*dnsOverGetaddrinfoTransport) } -func TestNewResolverSystem(t *testing.T) { - resolver := NewStdlibResolver(model.DiscardLogger) - typecheckForSystemResolver(t, resolver, model.DiscardLogger) -} - func TestNewSerialUDPResolver(t *testing.T) { d := NewDialerWithoutResolver(log.Log) resolver := NewSerialUDPResolver(log.Log, d, "1.1.1.1:53") From 3c9d305508d6c7cf58e260e8c109f031c9059c67 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 11 Sep 2023 19:19:03 +0200 Subject: [PATCH 4/8] x --- internal/model/netx.go | 6 --- internal/netxlite/dialer_test.go | 2 +- internal/netxlite/dnstransport.go | 13 +----- internal/netxlite/dnstransport_test.go | 7 +--- internal/netxlite/resolvercore.go | 35 ++++++---------- internal/netxlite/resolvercore_test.go | 55 ++++---------------------- internal/testingx/tlssniproxy.go | 2 +- 7 files changed, 26 insertions(+), 94 deletions(-) diff --git a/internal/model/netx.go b/internal/model/netx.go index 3148922511..b55cbd5001 100644 --- a/internal/model/netx.go +++ b/internal/model/netx.go @@ -105,12 +105,6 @@ type DNSEncoder interface { Encode(domain string, qtype uint16, padding bool) DNSQuery } -// DNSTransportWrapper is a type that takes in input a DNSTransport -// and returns in output a wrapped DNSTransport. -type DNSTransportWrapper interface { - WrapDNSTransport(txp DNSTransport) DNSTransport -} - // DNSTransport represents an abstract DNS transport. type DNSTransport interface { // RoundTrip sends a DNS query and receives the reply. diff --git a/internal/netxlite/dialer_test.go b/internal/netxlite/dialer_test.go index 68e6379db9..cf65783bcd 100644 --- a/internal/netxlite/dialer_test.go +++ b/internal/netxlite/dialer_test.go @@ -24,7 +24,7 @@ func TestNewDialerWithStdlibResolver(t *testing.T) { } // typecheck the resolver reso := logger.Dialer.(*dialerResolverWithTracing) - typecheckForSystemResolver(t, reso.Resolver, model.DiscardLogger) // FIXME + typeCheckForSystemResolver(t, reso.Resolver, model.DiscardLogger) // FIXME // typecheck the dialer logger = reso.Dialer.(*dialerLogger) if logger.DebugLogger != model.DiscardLogger { diff --git a/internal/netxlite/dnstransport.go b/internal/netxlite/dnstransport.go index 4a8ed39f3b..b49ad5c41b 100644 --- a/internal/netxlite/dnstransport.go +++ b/internal/netxlite/dnstransport.go @@ -10,20 +10,11 @@ import ( "github.com/ooni/probe-cli/v3/internal/model" ) -// WrapDNSTransport wraps a DNSTransport to provide error wrapping. This function will -// apply all the provided wrappers around the default transport wrapping. If any of the -// wrappers is nil, we just silently and gracefully ignore it. -func WrapDNSTransport(txp model.DNSTransport, - wrappers ...model.DNSTransportWrapper) (out model.DNSTransport) { +// WrapDNSTransport wraps a DNSTransport to provide error wrapping. +func WrapDNSTransport(txp model.DNSTransport) (out model.DNSTransport) { out = &dnsTransportErrWrapper{ DNSTransport: txp, } - for _, wrapper := range wrappers { - if wrapper == nil { - continue // skip as documented - } - out = wrapper.WrapDNSTransport(out) // compose with user-provided wrappers - } return } diff --git a/internal/netxlite/dnstransport_test.go b/internal/netxlite/dnstransport_test.go index 22b38fe4f8..eea16c5a50 100644 --- a/internal/netxlite/dnstransport_test.go +++ b/internal/netxlite/dnstransport_test.go @@ -32,12 +32,7 @@ func (*dnsTransportWrapperSecond) WrapDNSTransport(txp model.DNSTransport) model func TestWrapDNSTransport(t *testing.T) { orig := &mocks.DNSTransport{} - extensions := []model.DNSTransportWrapper{ - &dnsTransportWrapperFirst{}, - nil, // explicitly test for documented use case - &dnsTransportWrapperSecond{}, - } - txp := WrapDNSTransport(orig, extensions...) + txp := WrapDNSTransport(orig) ext2 := txp.(*dnsTransportExtensionSecond) ext1 := ext2.DNSTransport.(*dnsTransportExtensionFirst) errWrapper := ext1.DNSTransport.(*dnsTransportErrWrapper) diff --git a/internal/netxlite/resolvercore.go b/internal/netxlite/resolvercore.go index ee5cffecc3..de03666c0d 100644 --- a/internal/netxlite/resolvercore.go +++ b/internal/netxlite/resolvercore.go @@ -24,18 +24,16 @@ import ( var ErrNoDNSTransport = errors.New("operation requires a DNS transport") // NewStdlibResolver creates a new Resolver by combining WrapResolver -// with an internal "stdlib" 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 (netx *Netx) NewStdlibResolver(logger model.DebugLogger, wrappers ...model.DNSTransportWrapper) model.Resolver { - return WrapResolver(logger, netx.newUnwrappedStdlibResolver(wrappers...)) +// with an internal "stdlib" resolver type. +func (netx *Netx) NewStdlibResolver(logger model.DebugLogger) model.Resolver { + return WrapResolver(logger, netx.newUnwrappedStdlibResolver()) } // NewStdlibResolver is equivalent to creating an empty [*Netx] // and callings its NewStdlibResolver method. -func NewStdlibResolver(logger model.DebugLogger, wrappers ...model.DNSTransportWrapper) model.Resolver { +func NewStdlibResolver(logger model.DebugLogger) model.Resolver { netx := &Netx{Underlying: nil} - return netx.NewStdlibResolver(logger, wrappers...) + return netx.NewStdlibResolver(logger) } // NewParallelDNSOverHTTPSResolver creates a new DNS over HTTPS resolver @@ -47,18 +45,18 @@ func NewParallelDNSOverHTTPSResolver(logger model.DebugLogger, URL string) model return WrapResolver(logger, NewUnwrappedParallelResolver(txp)) } -func (netx *Netx) newUnwrappedStdlibResolver(wrappers ...model.DNSTransportWrapper) model.Resolver { +func (netx *Netx) newUnwrappedStdlibResolver() model.Resolver { return &resolverSystem{ - t: WrapDNSTransport(netx.newDNSOverGetaddrinfoTransport(), wrappers...), + t: WrapDNSTransport(netx.newDNSOverGetaddrinfoTransport()), } } // 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 { +func NewUnwrappedStdlibResolver() model.Resolver { netx := &Netx{Underlying: nil} - return netx.newUnwrappedStdlibResolver(wrappers...) + return netx.newUnwrappedStdlibResolver() } // NewSerialUDPResolver creates a new Resolver using DNS-over-UDP @@ -73,13 +71,9 @@ func NewUnwrappedStdlibResolver(wrappers ...model.DNSTransportWrapper) model.Res // - dialer is the dialer to create and connect UDP conns // // - address is the server address (e.g., 1.1.1.1:53) -// -// - wrappers is the optional list of wrappers to wrap the underlying -// transport. Any nil wrapper will be silently ignored. -func NewSerialUDPResolver(logger model.DebugLogger, dialer model.Dialer, - address string, wrappers ...model.DNSTransportWrapper) model.Resolver { +func NewSerialUDPResolver(logger model.DebugLogger, dialer model.Dialer, address string) model.Resolver { return WrapResolver(logger, NewUnwrappedSerialResolver( - WrapDNSTransport(NewUnwrappedDNSOverUDPTransport(dialer, address), wrappers...), + WrapDNSTransport(NewUnwrappedDNSOverUDPTransport(dialer, address)), )) } @@ -93,13 +87,10 @@ func NewSerialUDPResolver(logger model.DebugLogger, dialer model.Dialer, // - dialer is the dialer to create and connect UDP conns // // - address is the server address (e.g., 1.1.1.1:53) -// -// - wrappers is the optional list of wrappers to wrap the underlying -// transport. Any nil wrapper will be silently ignored. func NewParallelUDPResolver(logger model.DebugLogger, dialer model.Dialer, - address string, wrappers ...model.DNSTransportWrapper) model.Resolver { + address string) model.Resolver { return WrapResolver(logger, NewUnwrappedParallelResolver( - WrapDNSTransport(NewUnwrappedDNSOverUDPTransport(dialer, address), wrappers...), + WrapDNSTransport(NewUnwrappedDNSOverUDPTransport(dialer, address)), )) } diff --git a/internal/netxlite/resolvercore_test.go b/internal/netxlite/resolvercore_test.go index eb6f385015..55b6f291df 100644 --- a/internal/netxlite/resolvercore_test.go +++ b/internal/netxlite/resolvercore_test.go @@ -17,63 +17,24 @@ import ( "github.com/ooni/probe-cli/v3/internal/testingx" ) -type dnsTransportWrapperForTesting struct { - txp model.DNSTransport -} - -var _ model.DNSTransport = &dnsTransportWrapperForTesting{} - -// Address implements model.DNSTransport. -func (t *dnsTransportWrapperForTesting) Address() string { - return t.txp.Address() -} - -// CloseIdleConnections implements model.DNSTransport. -func (t *dnsTransportWrapperForTesting) CloseIdleConnections() { - t.txp.CloseIdleConnections() -} - -// Network implements model.DNSTransport. -func (t *dnsTransportWrapperForTesting) Network() string { - return t.txp.Network() -} - -// RequiresPadding implements model.DNSTransport. -func (t *dnsTransportWrapperForTesting) RequiresPadding() bool { - return t.txp.RequiresPadding() -} - -// RoundTrip implements model.DNSTransport. -func (t *dnsTransportWrapperForTesting) RoundTrip(ctx context.Context, query model.DNSQuery) (model.DNSResponse, error) { - return t.txp.RoundTrip(ctx, query) -} - -type dnsTransportWrapperFactoryForTesting struct{} - -var _ model.DNSTransportWrapper = &dnsTransportWrapperFactoryForTesting{} - -// WrapDNSTransport implements model.DNSTransportWrapper. -func (*dnsTransportWrapperFactoryForTesting) WrapDNSTransport(txp model.DNSTransport) model.DNSTransport { - return &dnsTransportWrapperForTesting{txp} -} - -func TestNewResolverSystem(t *testing.T) { - // TODO(bassosimone): we need to extend this test to make sure we honour wrapping. - resolver := NewStdlibResolver(model.DiscardLogger, &dnsTransportWrapperFactoryForTesting{}) - +func typeCheckForSystemResolver(t *testing.T, resolver model.Resolver, logger model.DebugLogger) { idna := resolver.(*resolverIDNA) loggerReso := idna.Resolver.(*resolverLogger) - if loggerReso.Logger != model.DiscardLogger { + if loggerReso.Logger != logger { t.Fatal("invalid logger") } shortCircuit := loggerReso.Resolver.(*resolverShortCircuitIPAddr) errWrapper := shortCircuit.Resolver.(*resolverErrWrapper) reso := errWrapper.Resolver.(*resolverSystem) - txpExternal := reso.t.(*dnsTransportWrapperForTesting) - txpErrWrapper := txpExternal.txp.(*dnsTransportErrWrapper) + txpErrWrapper := reso.t.(*dnsTransportErrWrapper) _ = txpErrWrapper.DNSTransport.(*dnsOverGetaddrinfoTransport) } +func TestNewResolverSystem(t *testing.T) { + resolver := NewStdlibResolver(model.DiscardLogger) + typeCheckForSystemResolver(t, resolver, model.DiscardLogger) +} + func TestNewSerialUDPResolver(t *testing.T) { d := NewDialerWithoutResolver(log.Log) resolver := NewSerialUDPResolver(log.Log, d, "1.1.1.1:53") diff --git a/internal/testingx/tlssniproxy.go b/internal/testingx/tlssniproxy.go index ffd8a1e602..e350b0e370 100644 --- a/internal/testingx/tlssniproxy.go +++ b/internal/testingx/tlssniproxy.go @@ -17,7 +17,7 @@ import ( // TLSSNIProxyNetx is how [TLSSNIProxy] views [*netxlite.Netx]. type TLSSNIProxyNetx interface { NewDialerWithResolver(dl model.DebugLogger, r model.Resolver, w ...model.DialerWrapper) model.Dialer - NewStdlibResolver(logger model.DebugLogger, wrappers ...model.DNSTransportWrapper) model.Resolver + NewStdlibResolver(logger model.DebugLogger) model.Resolver } // TLSSNIProxy is a proxy using the SNI to figure out where to connect to. From 8a844ec98d5b6f4c765f59b41e3e6c2bd16cbb5d Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 11 Sep 2023 19:20:33 +0200 Subject: [PATCH 5/8] x --- internal/netxlite/dialer_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/netxlite/dialer_test.go b/internal/netxlite/dialer_test.go index cf65783bcd..98e8b1077b 100644 --- a/internal/netxlite/dialer_test.go +++ b/internal/netxlite/dialer_test.go @@ -24,7 +24,7 @@ func TestNewDialerWithStdlibResolver(t *testing.T) { } // typecheck the resolver reso := logger.Dialer.(*dialerResolverWithTracing) - typeCheckForSystemResolver(t, reso.Resolver, model.DiscardLogger) // FIXME + typeCheckForSystemResolver(t, reso.Resolver, model.DiscardLogger) // typecheck the dialer logger = reso.Dialer.(*dialerLogger) if logger.DebugLogger != model.DiscardLogger { From 7410b1299a5cbcaa7b3e13574618775bc9d82dbb Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 11 Sep 2023 19:27:30 +0200 Subject: [PATCH 6/8] x --- .../gardener/internal/dnsreport/dnsreport.go | 12 ++++----- internal/netxlite/dnsoverhttps.go | 4 +-- internal/netxlite/dnstransport.go | 4 +-- internal/netxlite/dnstransport_test.go | 26 ++----------------- internal/netxlite/resolvercore.go | 8 +++--- 5 files changed, 15 insertions(+), 39 deletions(-) diff --git a/internal/cmd/gardener/internal/dnsreport/dnsreport.go b/internal/cmd/gardener/internal/dnsreport/dnsreport.go index 34a4dd7574..ab165b5430 100644 --- a/internal/cmd/gardener/internal/dnsreport/dnsreport.go +++ b/internal/cmd/gardener/internal/dnsreport/dnsreport.go @@ -8,7 +8,6 @@ import ( "encoding/json" "fmt" "net" - "net/http" "net/url" "os" "path/filepath" @@ -255,12 +254,11 @@ func (s *Subcommand) dnsLookupHost(domain string) ([]string, error) { defer cancel() // create DNS transport using HTTP default client - dnsTransport := netxlite.WrapDNSTransport(&netxlite.DNSOverHTTPSTransport{ - Client: http.DefaultClient, - Decoder: &netxlite.DNSDecoderMiekg{}, - URL: s.DNSOverHTTPSServerURL, - HostOverride: "", - }) + dnsTransport := netxlite.NewDNSOverHTTPSTransportWithHTTPTransport( + netxlite.NewHTTPTransportStdlib(log.Log), + s.DNSOverHTTPSServerURL, + ) + defer dnsTransport.CloseIdleConnections() // create DNS resolver dnsResolver := netxlite.WrapResolver( diff --git a/internal/netxlite/dnsoverhttps.go b/internal/netxlite/dnsoverhttps.go index aeb56f08e7..88f9e317f0 100644 --- a/internal/netxlite/dnsoverhttps.go +++ b/internal/netxlite/dnsoverhttps.go @@ -46,13 +46,13 @@ func NewUnwrappedDNSOverHTTPSTransport(client model.HTTPClient, URL string) *DNS // NewDNSOverHTTPSTransport is like NewUnwrappedDNSOverHTTPSTransport but // returns an already wrapped DNSTransport. func NewDNSOverHTTPSTransport(client model.HTTPClient, URL string) model.DNSTransport { - return WrapDNSTransport(NewUnwrappedDNSOverHTTPSTransport(client, URL)) + return wrapDNSTransport(NewUnwrappedDNSOverHTTPSTransport(client, URL)) } // NewDNSOverHTTPSTransportWithHTTPTransport is like NewDNSOverHTTPSTransport // but takes in input an HTTPTransport rather than an HTTPClient. func NewDNSOverHTTPSTransportWithHTTPTransport(txp model.HTTPTransport, URL string) model.DNSTransport { - return WrapDNSTransport(NewUnwrappedDNSOverHTTPSTransport(NewHTTPClient(txp), URL)) + return wrapDNSTransport(NewUnwrappedDNSOverHTTPSTransport(NewHTTPClient(txp), URL)) } // NewUnwrappedDNSOverHTTPSTransportWithHostOverride creates a new DNSOverHTTPSTransport diff --git a/internal/netxlite/dnstransport.go b/internal/netxlite/dnstransport.go index b49ad5c41b..e4cdf7e3a5 100644 --- a/internal/netxlite/dnstransport.go +++ b/internal/netxlite/dnstransport.go @@ -10,8 +10,8 @@ import ( "github.com/ooni/probe-cli/v3/internal/model" ) -// WrapDNSTransport wraps a DNSTransport to provide error wrapping. -func WrapDNSTransport(txp model.DNSTransport) (out model.DNSTransport) { +// wrapDNSTransport wraps a DNSTransport to provide error wrapping. +func wrapDNSTransport(txp model.DNSTransport) (out model.DNSTransport) { out = &dnsTransportErrWrapper{ DNSTransport: txp, } diff --git a/internal/netxlite/dnstransport_test.go b/internal/netxlite/dnstransport_test.go index eea16c5a50..ab13015bf9 100644 --- a/internal/netxlite/dnstransport_test.go +++ b/internal/netxlite/dnstransport_test.go @@ -10,32 +10,10 @@ import ( "github.com/ooni/probe-cli/v3/internal/model" ) -type dnsTransportExtensionFirst struct { - model.DNSTransport -} - -type dnsTransportWrapperFirst struct{} - -func (*dnsTransportWrapperFirst) WrapDNSTransport(txp model.DNSTransport) model.DNSTransport { - return &dnsTransportExtensionFirst{txp} -} - -type dnsTransportExtensionSecond struct { - model.DNSTransport -} - -type dnsTransportWrapperSecond struct{} - -func (*dnsTransportWrapperSecond) WrapDNSTransport(txp model.DNSTransport) model.DNSTransport { - return &dnsTransportExtensionSecond{txp} -} - func TestWrapDNSTransport(t *testing.T) { orig := &mocks.DNSTransport{} - txp := WrapDNSTransport(orig) - ext2 := txp.(*dnsTransportExtensionSecond) - ext1 := ext2.DNSTransport.(*dnsTransportExtensionFirst) - errWrapper := ext1.DNSTransport.(*dnsTransportErrWrapper) + txp := wrapDNSTransport(orig) + errWrapper := txp.(*dnsTransportErrWrapper) underlying := errWrapper.DNSTransport if orig != underlying { t.Fatal("unexpected underlying transport") diff --git a/internal/netxlite/resolvercore.go b/internal/netxlite/resolvercore.go index de03666c0d..b30cd2f5fe 100644 --- a/internal/netxlite/resolvercore.go +++ b/internal/netxlite/resolvercore.go @@ -41,13 +41,13 @@ func NewStdlibResolver(logger model.DebugLogger) model.Resolver { // all the building blocks and calls WrapResolver on the returned resolver. func NewParallelDNSOverHTTPSResolver(logger model.DebugLogger, URL string) model.Resolver { client := &http.Client{Transport: NewHTTPTransportStdlib(logger)} - txp := WrapDNSTransport(NewUnwrappedDNSOverHTTPSTransport(client, URL)) + txp := wrapDNSTransport(NewUnwrappedDNSOverHTTPSTransport(client, URL)) return WrapResolver(logger, NewUnwrappedParallelResolver(txp)) } func (netx *Netx) newUnwrappedStdlibResolver() model.Resolver { return &resolverSystem{ - t: WrapDNSTransport(netx.newDNSOverGetaddrinfoTransport()), + t: wrapDNSTransport(netx.newDNSOverGetaddrinfoTransport()), } } @@ -73,7 +73,7 @@ func NewUnwrappedStdlibResolver() model.Resolver { // - address is the server address (e.g., 1.1.1.1:53) func NewSerialUDPResolver(logger model.DebugLogger, dialer model.Dialer, address string) model.Resolver { return WrapResolver(logger, NewUnwrappedSerialResolver( - WrapDNSTransport(NewUnwrappedDNSOverUDPTransport(dialer, address)), + wrapDNSTransport(NewUnwrappedDNSOverUDPTransport(dialer, address)), )) } @@ -90,7 +90,7 @@ func NewSerialUDPResolver(logger model.DebugLogger, dialer model.Dialer, address func NewParallelUDPResolver(logger model.DebugLogger, dialer model.Dialer, address string) model.Resolver { return WrapResolver(logger, NewUnwrappedParallelResolver( - WrapDNSTransport(NewUnwrappedDNSOverUDPTransport(dialer, address)), + wrapDNSTransport(NewUnwrappedDNSOverUDPTransport(dialer, address)), )) } From 1ef3564636d143ab7f074b35c837faec8c257793 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 11 Sep 2023 19:29:23 +0200 Subject: [PATCH 7/8] x --- .../cmd/gardener/internal/dnsreport/dnsreport.go | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/internal/cmd/gardener/internal/dnsreport/dnsreport.go b/internal/cmd/gardener/internal/dnsreport/dnsreport.go index ab165b5430..4d47b02249 100644 --- a/internal/cmd/gardener/internal/dnsreport/dnsreport.go +++ b/internal/cmd/gardener/internal/dnsreport/dnsreport.go @@ -253,18 +253,8 @@ func (s *Subcommand) dnsLookupHost(domain string) ([]string, error) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - // create DNS transport using HTTP default client - dnsTransport := netxlite.NewDNSOverHTTPSTransportWithHTTPTransport( - netxlite.NewHTTPTransportStdlib(log.Log), - s.DNSOverHTTPSServerURL, - ) - defer dnsTransport.CloseIdleConnections() - - // create DNS resolver - dnsResolver := netxlite.WrapResolver( - log.Log, - netxlite.NewUnwrappedParallelResolver(dnsTransport), - ) + dnsResolver := netxlite.NewParallelDNSOverHTTPSResolver(log.Log, s.DNSOverHTTPSServerURL) + defer dnsResolver.CloseIdleConnections() // lookup for both A and AAAA entries return dnsResolver.LookupHost(ctx, domain) From 600b614a5e03356a43da901f664334ccd48f5460 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 11 Sep 2023 19:34:13 +0200 Subject: [PATCH 8/8] x --- internal/netxlite/resolvercore.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/netxlite/resolvercore.go b/internal/netxlite/resolvercore.go index b30cd2f5fe..b9500bb326 100644 --- a/internal/netxlite/resolvercore.go +++ b/internal/netxlite/resolvercore.go @@ -87,8 +87,7 @@ func NewSerialUDPResolver(logger model.DebugLogger, dialer model.Dialer, address // - dialer is the dialer to create and connect UDP conns // // - address is the server address (e.g., 1.1.1.1:53) -func NewParallelUDPResolver(logger model.DebugLogger, dialer model.Dialer, - address string) model.Resolver { +func NewParallelUDPResolver(logger model.DebugLogger, dialer model.Dialer, address string) model.Resolver { return WrapResolver(logger, NewUnwrappedParallelResolver( wrapDNSTransport(NewUnwrappedDNSOverUDPTransport(dialer, address)), ))