From da1c13e3126560ae87be99c46759489a0a872306 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 23 Aug 2022 11:43:44 +0200 Subject: [PATCH] cleanup: remove UnderlyingNetworkLibrary and TProxy (#874) * cleanup: remove UnderlyingNetworkLibrary and TProxy While there, replace mixture of mocking and real connections inside quicping with pure mocking of network connections. Closes https://github.com/ooni/probe/issues/2224 * cleanup: we don't need a SimpleResolver now This type was only used by UnderlyingNetworkLibrary and all the rest of the code uses Resolver. So, let's avoid complexity by zapping the SimpleResolver type and merging it inside Resolver. --- .../engine/experiment/quicping/quicping.go | 15 +- .../experiment/quicping/quicping_test.go | 134 ++++++++---------- internal/model/netx.go | 26 +--- internal/netxlite/dialer.go | 4 +- internal/netxlite/dnsovergetaddrinfo.go | 4 +- internal/netxlite/dnsovergetaddrinfo_test.go | 2 +- internal/netxlite/quic.go | 2 +- internal/netxlite/tproxy.go | 52 ------- 8 files changed, 75 insertions(+), 164 deletions(-) delete mode 100644 internal/netxlite/tproxy.go diff --git a/internal/engine/experiment/quicping/quicping.go b/internal/engine/experiment/quicping/quicping.go index f5c5c31..972f68e 100644 --- a/internal/engine/experiment/quicping/quicping.go +++ b/internal/engine/experiment/quicping/quicping.go @@ -18,7 +18,6 @@ import ( _ "crypto/sha256" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/netxlite" "github.com/ooni/probe-cli/v3/internal/tracex" ) @@ -44,8 +43,8 @@ type Config struct { // Port is the port to test. Port int64 `ooni:"port is the port to test"` - // networkLibrary is the underlying network library. Can be used for testing. - networkLib model.UnderlyingNetworkLibrary + // netListenUDP allows mocking the real net.ListenUDP call + netListenUDP func(network string, laddr *net.UDPAddr) (model.UDPLikeConn, error) } func (c *Config) repetitions() int64 { @@ -62,11 +61,11 @@ func (c *Config) port() string { return "443" } -func (c *Config) networkLibrary() model.UnderlyingNetworkLibrary { - if c.networkLib != nil { - return c.networkLib +func (c *Config) doListenUDP(network string, laddr *net.UDPAddr) (model.UDPLikeConn, error) { + if c.netListenUDP != nil { + return c.netListenUDP(network, laddr) } - return &netxlite.TProxyStdlib{} + return net.ListenUDP(network, laddr) } // TestKeys contains the experiment results. @@ -246,7 +245,7 @@ func (m *Measurer) Run( measurement.TestKeys = tk // create UDP socket - pconn, err := m.config.networkLibrary().ListenUDP("udp", &net.UDPAddr{}) + pconn, err := m.config.doListenUDP("udp", &net.UDPAddr{}) if err != nil { return err } diff --git a/internal/engine/experiment/quicping/quicping_test.go b/internal/engine/experiment/quicping/quicping_test.go index 725d1ea..79a214f 100644 --- a/internal/engine/experiment/quicping/quicping_test.go +++ b/internal/engine/experiment/quicping/quicping_test.go @@ -15,76 +15,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/model/mocks" ) -// FailStdLib is a failing model.UnderlyingNetworkLibrary. -type FailStdLib struct { - conn model.UDPLikeConn - err error - writeErr error - readErr error -} - -// ListenUDP implements model.UnderlyingNetworkLibrary.ListenUDP. -func (f *FailStdLib) ListenUDP(network string, laddr *net.UDPAddr) (model.UDPLikeConn, error) { - conn, _ := net.ListenUDP(network, laddr) - f.conn = model.UDPLikeConn(conn) - if f.err != nil { - return nil, f.err - } - if f.writeErr != nil { - return &mocks.UDPLikeConn{ - MockWriteTo: func(p []byte, addr net.Addr) (int, error) { - return 0, f.writeErr - }, - MockReadFrom: func(p []byte) (int, net.Addr, error) { - return f.conn.ReadFrom(p) - }, - MockSetDeadline: func(t time.Time) error { - return f.conn.SetDeadline(t) - }, - MockClose: func() error { - return f.conn.Close() - }, - }, nil - } - if f.readErr != nil { - return &mocks.UDPLikeConn{ - MockWriteTo: func(p []byte, addr net.Addr) (int, error) { - return f.conn.WriteTo(p, addr) - }, - MockReadFrom: func(p []byte) (int, net.Addr, error) { - return 0, nil, f.readErr - }, - MockSetDeadline: func(t time.Time) error { - return f.conn.SetDeadline(t) - }, - MockClose: func() error { - return f.conn.Close() - }, - }, nil - } - return &mocks.UDPLikeConn{}, nil -} - -// DefaultResolver implements model.UnderlyingNetworkLibrary.DefaultResolver. -func (f *FailStdLib) DefaultResolver() model.SimpleResolver { - return f -} - -// LookupHost implements model.SimpleResolver.LookupHost. -func (f *FailStdLib) LookupHost(ctx context.Context, domain string) ([]string, error) { - return nil, f.err -} - -// Network implements model.SimpleResolver.Network. -func (f *FailStdLib) Network() string { - return "fail_stdlib" -} - -// NewSimpleDialer implements UnderlyingNetworkLibrary.NewSimpleDialer. -func (f *FailStdLib) NewSimpleDialer(timeout time.Duration) model.SimpleDialer { - return nil -} - func TestNewExperimentMeasurer(t *testing.T) { measurer := NewExperimentMeasurer(Config{}) if measurer.ExperimentName() != "quicping" { @@ -201,7 +131,9 @@ func TestWithCancelledContext(t *testing.T) { func TestListenFails(t *testing.T) { expected := errors.New("expected") measurer := NewExperimentMeasurer(Config{ - networkLib: &FailStdLib{err: expected, readErr: nil, writeErr: nil}, + netListenUDP: func(network string, laddr *net.UDPAddr) (model.UDPLikeConn, error) { + return nil, expected + }, }) measurement := new(model.Measurement) measurement.Input = model.MeasurementTarget("google.com") @@ -221,8 +153,30 @@ func TestWriteFails(t *testing.T) { t.Skip("skip test in short mode") } expected := errors.New("expected") + setDeadlineCalled := false + closeCalled := false + pconn := &mocks.UDPLikeConn{ + MockReadFrom: func(p []byte) (int, net.Addr, error) { + source := make([]byte, len(p)) + copy(p, source) + return len(p), &mocks.Addr{}, nil + }, + MockSetDeadline: func(t time.Time) error { + setDeadlineCalled = true + return nil + }, + MockClose: func() error { + closeCalled = true + return nil + }, + MockWriteTo: func(p []byte, addr net.Addr) (int, error) { + return 0, expected + }, + } measurer := NewExperimentMeasurer(Config{ - networkLib: &FailStdLib{err: nil, readErr: nil, writeErr: expected}, + netListenUDP: func(network string, laddr *net.UDPAddr) (model.UDPLikeConn, error) { + return pconn, nil + }, Repetitions: 1, }) measurement := new(model.Measurement) @@ -245,15 +199,41 @@ func TestWriteFails(t *testing.T) { t.Fatal("ping: unexpected error type", i, *ping.Failure) } } + if !setDeadlineCalled { + t.Fatal("did not call set deadline") + } + if !closeCalled { + t.Fatal("did not call close") + } } func TestReadFails(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") } + setDeadlineCalled := false + closeCalled := false expected := errors.New("expected") + pconn := &mocks.UDPLikeConn{ + MockReadFrom: func(p []byte) (int, net.Addr, error) { + return 0, nil, expected + }, + MockSetDeadline: func(t time.Time) error { + setDeadlineCalled = true + return nil + }, + MockClose: func() error { + closeCalled = true + return nil + }, + MockWriteTo: func(p []byte, addr net.Addr) (int, error) { + return len(p), nil + }, + } measurer := NewExperimentMeasurer(Config{ - networkLib: &FailStdLib{err: nil, readErr: expected, writeErr: nil}, + netListenUDP: func(network string, laddr *net.UDPAddr) (model.UDPLikeConn, error) { + return pconn, nil + }, Repetitions: 1, }) measurement := new(model.Measurement) @@ -273,6 +253,12 @@ func TestReadFails(t *testing.T) { t.Fatal("expected an error here, ping", i) } } + if !setDeadlineCalled { + t.Fatal("did not call set deadline") + } + if !closeCalled { + t.Fatal("did not call close") + } } func TestNoResponse(t *testing.T) { @@ -303,7 +289,7 @@ func TestNoResponse(t *testing.T) { } func TestDissect(t *testing.T) { - // destID--srcID: 040b9649d3fd4c038ab6c073966f3921--44d064031288e97646451f + // destID--srcID: 040b9649d3fd4c038ab6c073966f3921--44d064031288e97646451f versionNegotiationResponse, _ := hex.DecodeString("eb0000000010040b9649d3fd4c038ab6c073966f39210b44d064031288e97646451f00000001ff00001dff00001cff00001b") measurer := NewExperimentMeasurer(Config{}) destID := "040b9649d3fd4c038ab6c073966f3921" diff --git a/internal/model/netx.go b/internal/model/netx.go index ecc9f5e..b38f89c 100644 --- a/internal/model/netx.go +++ b/internal/model/netx.go @@ -211,9 +211,8 @@ type QUICDialer interface { CloseIdleConnections() } -// SimpleResolver is a simplified resolver that only allows to perform -// an ordinary lookup operation and to know the resolver's name. -type SimpleResolver interface { +// Resolver performs domain name resolutions. +type Resolver interface { // LookupHost behaves like net.Resolver.LookupHost. LookupHost(ctx context.Context, hostname string) (addrs []string, err error) @@ -239,12 +238,6 @@ type SimpleResolver interface { // for an explanation of why it would not be proper to call "netgo" the // resolver we get by default from the standard library. Network() string -} - -// Resolver performs domain name resolutions. -type Resolver interface { - // A Resolver is also a SimpleResolver. - SimpleResolver // Address returns the resolver address (e.g., 8.8.8.8:53). Address() string @@ -484,18 +477,3 @@ type UDPLikeConn interface { // which is also instrumental to setting the read buffer. SyscallConn() (syscall.RawConn, error) } - -// UnderlyingNetworkLibrary defines the basic functionality from -// which the network extensions depend. By changing the default -// implementation of this interface, we can implement a wide array -// of tests, including self censorship tests. -type UnderlyingNetworkLibrary interface { - // ListenUDP creates a new model.UDPLikeConn conn. - ListenUDP(network string, laddr *net.UDPAddr) (UDPLikeConn, error) - - // DefaultResolver returns the default resolver. - DefaultResolver() SimpleResolver - - // NewSimpleDialer returns a new SimpleDialer. - NewSimpleDialer(timeout time.Duration) SimpleDialer -} diff --git a/internal/netxlite/dialer.go b/internal/netxlite/dialer.go index b9d0dcd..3ec0cf9 100644 --- a/internal/netxlite/dialer.go +++ b/internal/netxlite/dialer.go @@ -143,7 +143,7 @@ func NewDialerWithoutResolver(dl model.DebugLogger, w ...model.DialerWrapper) mo return NewDialerWithResolver(dl, &NullResolver{}, w...) } -// DialerSystem is a model.Dialer that uses TProxy.NewSimplerDialer +// DialerSystem is a model.Dialer that uses the stdlib's net.Dialer // to construct the new SimpleDialer used for dialing. This dialer has // a fixed timeout for each connect operation equal to 15 seconds. type DialerSystem struct { @@ -160,7 +160,7 @@ func (d *DialerSystem) newUnderlyingDialer() model.SimpleDialer { if t <= 0 { t = dialerDefaultTimeout } - return TProxy.NewSimpleDialer(t) + return &net.Dialer{Timeout: t} } func (d *DialerSystem) DialContext(ctx context.Context, network, address string) (net.Conn, error) { diff --git a/internal/netxlite/dnsovergetaddrinfo.go b/internal/netxlite/dnsovergetaddrinfo.go index 0aca6ed..58b66f2 100644 --- a/internal/netxlite/dnsovergetaddrinfo.go +++ b/internal/netxlite/dnsovergetaddrinfo.go @@ -81,7 +81,7 @@ func (txp *dnsOverGetaddrinfoTransport) lookupfn() func(ctx context.Context, dom if txp.testableLookupHost != nil { return txp.testableLookupHost } - return TProxy.DefaultResolver().LookupHost + return getaddrinfoLookupHost } func (txp *dnsOverGetaddrinfoTransport) RequiresPadding() bool { @@ -89,7 +89,7 @@ func (txp *dnsOverGetaddrinfoTransport) RequiresPadding() bool { } func (txp *dnsOverGetaddrinfoTransport) Network() string { - return TProxy.DefaultResolver().Network() + return getaddrinfoResolverNetwork() } func (txp *dnsOverGetaddrinfoTransport) Address() string { diff --git a/internal/netxlite/dnsovergetaddrinfo_test.go b/internal/netxlite/dnsovergetaddrinfo_test.go index 78ca5dc..37e5deb 100644 --- a/internal/netxlite/dnsovergetaddrinfo_test.go +++ b/internal/netxlite/dnsovergetaddrinfo_test.go @@ -21,7 +21,7 @@ func TestDNSOverGetaddrinfo(t *testing.T) { t.Run("Network", func(t *testing.T) { txp := &dnsOverGetaddrinfoTransport{} - if txp.Network() != TProxy.DefaultResolver().Network() { + if txp.Network() != getaddrinfoResolverNetwork() { t.Fatal("unexpected Network") } }) diff --git a/internal/netxlite/quic.go b/internal/netxlite/quic.go index 682c976..fef805e 100644 --- a/internal/netxlite/quic.go +++ b/internal/netxlite/quic.go @@ -29,7 +29,7 @@ var _ model.QUICListener = &quicListenerStdlib{} // Listen implements QUICListener.Listen. func (qls *quicListenerStdlib) Listen(addr *net.UDPAddr) (model.UDPLikeConn, error) { - return TProxy.ListenUDP("udp", addr) + return net.ListenUDP("udp", addr) } // NewQUICDialerWithResolver is the WrapDialer equivalent for QUIC where diff --git a/internal/netxlite/tproxy.go b/internal/netxlite/tproxy.go deleted file mode 100644 index 98af596..0000000 --- a/internal/netxlite/tproxy.go +++ /dev/null @@ -1,52 +0,0 @@ -package netxlite - -// -// Transparent proxy (for integration testing) -// - -import ( - "context" - "net" - "time" - - "github.com/ooni/probe-cli/v3/internal/model" -) - -// TProxy is the fundamental variable controlling how netxlite creates -// net.Conn and model.UDPLikeConn, as well as how it uses the stdlib -// resolver. By modifying this variable, you can effectively transparently -// proxy netxlite (and hence OONI) activities to other services. This is -// quite convenient when performing quality assurance tests. -var TProxy model.UnderlyingNetworkLibrary = &TProxyStdlib{} - -// TProxyStdlib is the default model.UnderlyingNetworkLibrary using -// the stdlib in the most obvious way for every functionality. -type TProxyStdlib struct{} - -// ListenUDP calls net.ListenUDP. -func (*TProxyStdlib) ListenUDP(network string, laddr *net.UDPAddr) (model.UDPLikeConn, error) { - return net.ListenUDP(network, laddr) -} - -// DefaultResolver returns the default resolver. -func (*TProxyStdlib) DefaultResolver() model.SimpleResolver { - return &tproxyDefaultResolver{} -} - -// NewSimpleDialer returns a &net.Dialer{Timeout: timeout} instance. -func (*TProxyStdlib) NewSimpleDialer(timeout time.Duration) model.SimpleDialer { - return &net.Dialer{Timeout: timeout} -} - -// tproxyDefaultResolver is the resolver we use by default. -type tproxyDefaultResolver struct{} - -// LookupHost implements model.SimpleResolver.LookupHost. -func (r *tproxyDefaultResolver) LookupHost(ctx context.Context, domain string) ([]string, error) { - return getaddrinfoLookupHost(ctx, domain) -} - -// Network implements model.SimpleResolver.Network. -func (r *tproxyDefaultResolver) Network() string { - return getaddrinfoResolverNetwork() -}