From 6924d1ad81bfd248563af6e6dc02b07ede9d6c3b Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 24 May 2022 18:23:42 +0200 Subject: [PATCH] refactor: only use shaping dialer for ndt7 and dash (#754) See https://github.com/ooni/probe/issues/2112 for context. While there, run `go fix -fix buildtag ./...` --- internal/cmd/jafar/iptables/iptables_linux.go | 1 - .../jafar/iptables/iptables_unsupported.go | 1 - internal/engine/experiment/dash/dash.go | 10 ++- internal/engine/experiment/ndt7/dial.go | 3 + internal/engine/netx/dialer/dialer.go | 1 - internal/engine/netx/dialer/dialer_test.go | 6 +- .../engine/netx/dialer/shaping_disabled.go | 24 ------- internal/engine/netx/dialer/shaping_test.go | 22 ------- internal/engine/session_nopsiphon.go | 1 - internal/engine/session_nopsiphon_test.go | 1 - internal/engine/session_psiphon.go | 1 - internal/engine/session_psiphon_test.go | 1 - internal/netxlite/shaping.go | 15 +++++ internal/netxlite/shaping_otherwise.go | 11 ++++ internal/netxlite/shaping_otherwise_test.go | 17 +++++ .../shaping_shaping.go} | 10 +-- internal/netxlite/shaping_shaping_test.go | 66 +++++++++++++++++++ 17 files changed, 126 insertions(+), 65 deletions(-) delete mode 100644 internal/engine/netx/dialer/shaping_disabled.go delete mode 100644 internal/engine/netx/dialer/shaping_test.go create mode 100644 internal/netxlite/shaping.go create mode 100644 internal/netxlite/shaping_otherwise.go create mode 100644 internal/netxlite/shaping_otherwise_test.go rename internal/{engine/netx/dialer/shaping_enabled.go => netxlite/shaping_shaping.go} (78%) create mode 100644 internal/netxlite/shaping_shaping_test.go diff --git a/internal/cmd/jafar/iptables/iptables_linux.go b/internal/cmd/jafar/iptables/iptables_linux.go index b994dc2..09d4c4d 100644 --- a/internal/cmd/jafar/iptables/iptables_linux.go +++ b/internal/cmd/jafar/iptables/iptables_linux.go @@ -1,5 +1,4 @@ //go:build linux -// +build linux package iptables diff --git a/internal/cmd/jafar/iptables/iptables_unsupported.go b/internal/cmd/jafar/iptables/iptables_unsupported.go index 216f719..5c45412 100644 --- a/internal/cmd/jafar/iptables/iptables_unsupported.go +++ b/internal/cmd/jafar/iptables/iptables_unsupported.go @@ -1,5 +1,4 @@ //go:build !linux -// +build !linux package iptables diff --git a/internal/engine/experiment/dash/dash.go b/internal/engine/experiment/dash/dash.go index 83dd2f2..2b86e67 100644 --- a/internal/engine/experiment/dash/dash.go +++ b/internal/engine/experiment/dash/dash.go @@ -259,8 +259,14 @@ func (m Measurer) Run( httpClient := &http.Client{ Transport: netx.NewHTTPTransport(netx.Config{ ContextByteCounting: true, - DialSaver: saver, - Logger: sess.Logger(), + // Implements shaping if the user builds using `-tags shaping` + // See https://github.com/ooni/probe/issues/2112 + Dialer: netxlite.NewMaybeShapingDialer(netx.NewDialer(netx.Config{ + ContextByteCounting: true, + DialSaver: saver, + Logger: sess.Logger(), + })), + Logger: sess.Logger(), }), } defer httpClient.CloseIdleConnections() diff --git a/internal/engine/experiment/ndt7/dial.go b/internal/engine/experiment/ndt7/dial.go index 5c2a7c7..9966382 100644 --- a/internal/engine/experiment/ndt7/dial.go +++ b/internal/engine/experiment/ndt7/dial.go @@ -42,6 +42,9 @@ func (mgr dialManager) dialWithTestName(ctx context.Context, testName string) (* Logger: mgr.logger, ProxyURL: mgr.proxyURL, }, reso) + // Implements shaping if the user builds using `-tags shaping` + // See https://github.com/ooni/probe/issues/2112 + dlr = netxlite.NewMaybeShapingDialer(dlr) // We force using our bundled CA pool, which should fix // https://github.com/ooni/probe/issues/2031 tlsConfig := &tls.Config{ diff --git a/internal/engine/netx/dialer/dialer.go b/internal/engine/netx/dialer/dialer.go index e0df83e..a5f79eb 100644 --- a/internal/engine/netx/dialer/dialer.go +++ b/internal/engine/netx/dialer/dialer.go @@ -67,6 +67,5 @@ func New(config *Config, resolver model.Resolver) model.Dialer { if config.ContextByteCounting { d = &byteCounterDialer{Dialer: d} } - d = &shapingDialer{Dialer: d} return d } diff --git a/internal/engine/netx/dialer/dialer_test.go b/internal/engine/netx/dialer/dialer_test.go index d53e1cf..29af7c6 100644 --- a/internal/engine/netx/dialer/dialer_test.go +++ b/internal/engine/netx/dialer/dialer_test.go @@ -18,11 +18,7 @@ func TestNewCreatesTheExpectedChain(t *testing.T) { ProxyURL: &url.URL{}, ReadWriteSaver: saver, }, netxlite.DefaultResolver) - shd, ok := dlr.(*shapingDialer) - if !ok { - t.Fatal("not a shapingDialer") - } - bcd, ok := shd.Dialer.(*byteCounterDialer) + bcd, ok := dlr.(*byteCounterDialer) if !ok { t.Fatal("not a byteCounterDialer") } diff --git a/internal/engine/netx/dialer/shaping_disabled.go b/internal/engine/netx/dialer/shaping_disabled.go deleted file mode 100644 index 28f1238..0000000 --- a/internal/engine/netx/dialer/shaping_disabled.go +++ /dev/null @@ -1,24 +0,0 @@ -//go:build !shaping -// +build !shaping - -package dialer - -import ( - "context" - "net" - - "github.com/ooni/probe-cli/v3/internal/model" -) - -// shapingDialer ensures we don't use too much bandwidth -// when using integration tests at GitHub. To select -// the implementation with shaping use `-tags shaping`. -type shapingDialer struct { - model.Dialer -} - -// DialContext implements Dialer.DialContext -func (d *shapingDialer) DialContext( - ctx context.Context, network, address string) (net.Conn, error) { - return d.Dialer.DialContext(ctx, network, address) -} diff --git a/internal/engine/netx/dialer/shaping_test.go b/internal/engine/netx/dialer/shaping_test.go deleted file mode 100644 index 71e0401..0000000 --- a/internal/engine/netx/dialer/shaping_test.go +++ /dev/null @@ -1,22 +0,0 @@ -package dialer - -import ( - "net/http" - "testing" - - "github.com/ooni/probe-cli/v3/internal/netxlite" -) - -func TestShapingDialerGood(t *testing.T) { - d := &shapingDialer{Dialer: netxlite.DefaultDialer} - txp := &http.Transport{DialContext: d.DialContext} - client := &http.Client{Transport: txp} - resp, err := client.Get("https://www.google.com/") - if err != nil { - t.Fatal(err) - } - if resp == nil { - t.Fatal("expected nil response here") - } - resp.Body.Close() -} diff --git a/internal/engine/session_nopsiphon.go b/internal/engine/session_nopsiphon.go index f968f8b..cc03326 100644 --- a/internal/engine/session_nopsiphon.go +++ b/internal/engine/session_nopsiphon.go @@ -1,5 +1,4 @@ //go:build !ooni_psiphon_config -// +build !ooni_psiphon_config package engine diff --git a/internal/engine/session_nopsiphon_test.go b/internal/engine/session_nopsiphon_test.go index a523ad4..ecb6cc7 100644 --- a/internal/engine/session_nopsiphon_test.go +++ b/internal/engine/session_nopsiphon_test.go @@ -1,5 +1,4 @@ //go:build !ooni_psiphon_config -// +build !ooni_psiphon_config package engine diff --git a/internal/engine/session_psiphon.go b/internal/engine/session_psiphon.go index f5b8a69..48b5ab0 100644 --- a/internal/engine/session_psiphon.go +++ b/internal/engine/session_psiphon.go @@ -1,5 +1,4 @@ //go:build ooni_psiphon_config -// +build ooni_psiphon_config package engine diff --git a/internal/engine/session_psiphon_test.go b/internal/engine/session_psiphon_test.go index d65f445..4f453e5 100644 --- a/internal/engine/session_psiphon_test.go +++ b/internal/engine/session_psiphon_test.go @@ -1,5 +1,4 @@ //go:build ooni_psiphon_config -// +build ooni_psiphon_config package engine diff --git a/internal/netxlite/shaping.go b/internal/netxlite/shaping.go new file mode 100644 index 0000000..3ec0c4c --- /dev/null +++ b/internal/netxlite/shaping.go @@ -0,0 +1,15 @@ +package netxlite + +import "github.com/ooni/probe-cli/v3/internal/model" + +// NewMaybeShapingDialer takes in input a model.Dialer and returns in output another +// model.Dialer that MAY dial connections with I/O shaping, depending on whether +// the user builds with or without the `-tags shaping` CLI flag. +// +// We typically use `-tags shaping` when running integration tests for dash and ndt7 to +// avoiod hammering m-lab servers from the very-fast GitHub CI servers. +// +// See https://github.com/ooni/probe/issues/2112 for extra context. +func NewMaybeShapingDialer(dialer model.Dialer) model.Dialer { + return newMaybeShapingDialer(dialer) +} diff --git a/internal/netxlite/shaping_otherwise.go b/internal/netxlite/shaping_otherwise.go new file mode 100644 index 0000000..76a1273 --- /dev/null +++ b/internal/netxlite/shaping_otherwise.go @@ -0,0 +1,11 @@ +//go:build !shaping + +package netxlite + +import ( + "github.com/ooni/probe-cli/v3/internal/model" +) + +func newMaybeShapingDialer(dialer model.Dialer) model.Dialer { + return dialer +} diff --git a/internal/netxlite/shaping_otherwise_test.go b/internal/netxlite/shaping_otherwise_test.go new file mode 100644 index 0000000..8f7c581 --- /dev/null +++ b/internal/netxlite/shaping_otherwise_test.go @@ -0,0 +1,17 @@ +//go:build !shaping + +package netxlite + +import ( + "testing" + + "github.com/ooni/probe-cli/v3/internal/model/mocks" +) + +func TestNewShapingDialer(t *testing.T) { + in := &mocks.Dialer{} + out := NewMaybeShapingDialer(in) + if in != out { + t.Fatal("expected to see the same pointer") + } +} diff --git a/internal/engine/netx/dialer/shaping_enabled.go b/internal/netxlite/shaping_shaping.go similarity index 78% rename from internal/engine/netx/dialer/shaping_enabled.go rename to internal/netxlite/shaping_shaping.go index 5581ce7..7a2ec65 100644 --- a/internal/engine/netx/dialer/shaping_enabled.go +++ b/internal/netxlite/shaping_shaping.go @@ -1,7 +1,6 @@ //go:build shaping -// +build shaping -package dialer +package netxlite import ( "context" @@ -11,9 +10,10 @@ import ( "github.com/ooni/probe-cli/v3/internal/model" ) -// shapingDialer ensures we don't use too much bandwidth -// when using integration tests at GitHub. To select -// the implementation with shaping use `-tags shaping`. +func newMaybeShapingDialer(dialer model.Dialer) model.Dialer { + return &shapingDialer{dialer} +} + type shapingDialer struct { model.Dialer } diff --git a/internal/netxlite/shaping_shaping_test.go b/internal/netxlite/shaping_shaping_test.go new file mode 100644 index 0000000..b84751f --- /dev/null +++ b/internal/netxlite/shaping_shaping_test.go @@ -0,0 +1,66 @@ +//go:build shaping + +package netxlite + +import ( + "context" + "errors" + "net" + "testing" + + "github.com/ooni/probe-cli/v3/internal/model/mocks" +) + +func TestNewShapingDialerx(t *testing.T) { + t.Run("failure", func(t *testing.T) { + expected := errors.New("mocked error") + d := &mocks.Dialer{ + MockDialContext: func(ctx context.Context, network, address string) (net.Conn, error) { + return nil, expected + }, + } + shd := NewMaybeShapingDialer(d) + conn, err := shd.DialContext(context.Background(), "tcp", "8.8.8.8:443") + if !errors.Is(err, expected) { + t.Fatal("unexpected err", err) + } + if conn != nil { + t.Fatal("expected nil conn") + } + }) + + t.Run("success", func(t *testing.T) { + expected := errors.New("mocked error") + uc := &mocks.Conn{ + MockRead: func(b []byte) (int, error) { + return 0, expected + }, + MockWrite: func(b []byte) (int, error) { + return 0, expected + }, + } + d := &mocks.Dialer{ + MockDialContext: func(ctx context.Context, network, address string) (net.Conn, error) { + return uc, nil + }, + } + shd := NewMaybeShapingDialer(d) + conn, err := shd.DialContext(context.Background(), "tcp", "8.8.8.8:443") + if err != nil { + t.Fatal(err) + } + if _, ok := conn.(*shapingConn); !ok { + t.Fatal("not shapingConn") + } + validateCountAndErr := func(count int, err error) { + if !errors.Is(err, expected) { + t.Fatal("unexpected err", err) + } + if count != 0 { + t.Fatal("expected zero") + } + } + validateCountAndErr(conn.Read(make([]byte, 16))) + validateCountAndErr(conn.Write(make([]byte, 16))) + }) +}