From f73945037055193de1e97286359e07cea39f7074 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sat, 3 Apr 2021 21:09:34 +0200 Subject: [PATCH] refactor(tunnel): simplify psiphon implementation (#289) Simplify interaction within the package by avoiding to have a psiphon specific config. Use a Config instead. Part of https://github.com/ooni/probe/issues/985. --- internal/engine/tunnel/config.go | 58 ++++++++++++++++++++++++++ internal/engine/tunnel/psiphon.go | 57 +++++-------------------- internal/engine/tunnel/psiphon_test.go | 54 +++++++++--------------- internal/engine/tunnel/tunnel.go | 18 +------- 4 files changed, 88 insertions(+), 99 deletions(-) create mode 100644 internal/engine/tunnel/config.go diff --git a/internal/engine/tunnel/config.go b/internal/engine/tunnel/config.go new file mode 100644 index 0000000..ff2f310 --- /dev/null +++ b/internal/engine/tunnel/config.go @@ -0,0 +1,58 @@ +package tunnel + +import ( + "context" + "os" + + "github.com/ooni/psiphon/oopsi/github.com/Psiphon-Labs/psiphon-tunnel-core/ClientLibrary/clientlib" +) + +// Config contains the configuration for creating a Tunnel instance. +type Config struct { + // Name is the mandatory name of the tunnel. We support + // "tor" and "psiphon" tunnels. + Name string + + // Session is the current measurement session. + Session Session + + // WorkDir is the directory in which the tunnel SHOULD + // store its state, if any. + WorkDir string + + // testMkdirAll allows us to mock os.MkdirAll in testing code. + testMkdirAll func(path string, perm os.FileMode) error + + // testRemoveAll allows us to mock os.RemoveAll in testing code. + testRemoveAll func(path string) error + + // testStartPsiphon allows us to mock psiphon's clientlib.StartTunnel. + testStartPsiphon func(ctx context.Context, config []byte, + workdir string) (*clientlib.PsiphonTunnel, error) +} + +// mkdirAll calls either testMkdirAll or os.MkdirAll. +func (c *Config) mkdirAll(path string, perm os.FileMode) error { + if c.testMkdirAll != nil { + return c.testMkdirAll(path, perm) + } + return os.MkdirAll(path, perm) +} + +// removeAll calls either testRemoveAll or os.RemoveAll. +func (c *Config) removeAll(path string) error { + if c.testRemoveAll != nil { + return c.testRemoveAll(path) + } + return os.RemoveAll(path) +} + +// startPsiphon calls either testStartPsiphon or psiphon's clientlib.StartTunnel. +func (c *Config) startPsiphon(ctx context.Context, config []byte, + workdir string) (*clientlib.PsiphonTunnel, error) { + if c.testStartPsiphon != nil { + return c.testStartPsiphon(ctx, config, workdir) + } + return clientlib.StartTunnel(ctx, config, "", clientlib.Parameters{ + DataRootDirectory: &workdir}, nil, nil) +} diff --git a/internal/engine/tunnel/psiphon.go b/internal/engine/tunnel/psiphon.go index 6b515e6..73676e8 100644 --- a/internal/engine/tunnel/psiphon.go +++ b/internal/engine/tunnel/psiphon.go @@ -5,81 +5,41 @@ import ( "fmt" "net" "net/url" - "os" "path/filepath" "time" "github.com/ooni/psiphon/oopsi/github.com/Psiphon-Labs/psiphon-tunnel-core/ClientLibrary/clientlib" ) -// psiphonDependencies contains dependencies for psiphonStart -type psiphonDependencies interface { - MkdirAll(path string, perm os.FileMode) error - RemoveAll(path string) error - Start(ctx context.Context, config []byte, - workdir string) (*clientlib.PsiphonTunnel, error) -} - -type defaultDependencies struct{} - -func (defaultDependencies) MkdirAll(path string, perm os.FileMode) error { - return os.MkdirAll(path, perm) -} - -func (defaultDependencies) RemoveAll(path string) error { - return os.RemoveAll(path) -} - -func (defaultDependencies) Start( - ctx context.Context, config []byte, workdir string) (*clientlib.PsiphonTunnel, error) { - return clientlib.StartTunnel(ctx, config, "", clientlib.Parameters{ - DataRootDirectory: &workdir}, nil, nil) -} - -// psiphonConfig contains the settings for psiphonStart. The empty config object implies -// that we will be using default settings for starting the tunnel. -type psiphonConfig struct { - // Dependencies contains dependencies for Start. - Dependencies psiphonDependencies - - // WorkDir is the directory where Psiphon should store - // its configuration database. - WorkDir string -} - // psiphonTunnel is a psiphon tunnel type psiphonTunnel struct { tunnel *clientlib.PsiphonTunnel duration time.Duration } -func makeworkingdir(config psiphonConfig) (string, error) { +func makeworkingdir(config *Config) (string, error) { const testdirname = "oonipsiphon" workdir := filepath.Join(config.WorkDir, testdirname) - if err := config.Dependencies.RemoveAll(workdir); err != nil { + if err := config.removeAll(workdir); err != nil { return "", err } - if err := config.Dependencies.MkdirAll(workdir, 0700); err != nil { + if err := config.mkdirAll(workdir, 0700); err != nil { return "", err } return workdir, nil } // psiphonStart starts the psiphon tunnel. -func psiphonStart( - ctx context.Context, sess Session, config psiphonConfig) (Tunnel, error) { +func psiphonStart(ctx context.Context, config *Config) (Tunnel, error) { select { case <-ctx.Done(): return nil, ctx.Err() // simplifies unit testing this code default: } - if config.Dependencies == nil { - config.Dependencies = defaultDependencies{} - } if config.WorkDir == "" { - config.WorkDir = sess.TempDir() + config.WorkDir = config.Session.TempDir() } - configJSON, err := sess.FetchPsiphonConfig(ctx) + configJSON, err := config.Session.FetchPsiphonConfig(ctx) if err != nil { return nil, err } @@ -88,7 +48,7 @@ func psiphonStart( return nil, err } start := time.Now() - tunnel, err := config.Dependencies.Start(ctx, configJSON, workdir) + tunnel, err := config.startPsiphon(ctx, configJSON, workdir) if err != nil { return nil, err } @@ -96,6 +56,9 @@ func psiphonStart( return &psiphonTunnel{tunnel: tunnel, duration: stop.Sub(start)}, nil } +// TODO(bassosimone): define the NullTunnel rather than relying on +// this magic that a nil psiphonTunnel works. + // Stop is an idempotent method that shuts down the tunnel func (t *psiphonTunnel) Stop() { if t != nil { diff --git a/internal/engine/tunnel/psiphon_test.go b/internal/engine/tunnel/psiphon_test.go index c4fdb36..40a1624 100644 --- a/internal/engine/tunnel/psiphon_test.go +++ b/internal/engine/tunnel/psiphon_test.go @@ -15,7 +15,9 @@ func TestPsiphonFetchPsiphonConfigFailure(t *testing.T) { sess := &mockable.Session{ MockableFetchPsiphonConfigErr: expected, } - tunnel, err := psiphonStart(context.Background(), sess, psiphonConfig{}) + tunnel, err := psiphonStart(context.Background(), &Config{ + Session: sess, + }) if !errors.Is(err, expected) { t.Fatal("not the error we expected") } @@ -26,14 +28,14 @@ func TestPsiphonFetchPsiphonConfigFailure(t *testing.T) { func TestPsiphonMakeMkdirAllFailure(t *testing.T) { expected := errors.New("mocked error") - dependencies := psiphonFakeDependencies{ - MkdirAllErr: expected, - } sess := &mockable.Session{ MockableFetchPsiphonConfigResult: []byte(`{}`), } - tunnel, err := psiphonStart(context.Background(), sess, psiphonConfig{ - Dependencies: dependencies, + tunnel, err := psiphonStart(context.Background(), &Config{ + Session: sess, + testMkdirAll: func(path string, perm os.FileMode) error { + return expected + }, }) if !errors.Is(err, expected) { t.Fatal("not the error we expected") @@ -45,14 +47,14 @@ func TestPsiphonMakeMkdirAllFailure(t *testing.T) { func TestPsiphonMakeRemoveAllFailure(t *testing.T) { expected := errors.New("mocked error") - dependencies := psiphonFakeDependencies{ - RemoveAllErr: expected, - } sess := &mockable.Session{ MockableFetchPsiphonConfigResult: []byte(`{}`), } - tunnel, err := psiphonStart(context.Background(), sess, psiphonConfig{ - Dependencies: dependencies, + tunnel, err := psiphonStart(context.Background(), &Config{ + Session: sess, + testRemoveAll: func(path string) error { + return expected + }, }) if !errors.Is(err, expected) { t.Fatal("not the error we expected") @@ -64,14 +66,15 @@ func TestPsiphonMakeRemoveAllFailure(t *testing.T) { func TestPsiphonMakeStartFailure(t *testing.T) { expected := errors.New("mocked error") - dependencies := psiphonFakeDependencies{ - StartErr: expected, - } sess := &mockable.Session{ MockableFetchPsiphonConfigResult: []byte(`{}`), } - tunnel, err := psiphonStart(context.Background(), sess, psiphonConfig{ - Dependencies: dependencies, + tunnel, err := psiphonStart(context.Background(), &Config{ + Session: sess, + testStartPsiphon: func(ctx context.Context, config []byte, + workdir string) (*clientlib.PsiphonTunnel, error) { + return nil, expected + }, }) if !errors.Is(err, expected) { t.Fatal("not the error we expected") @@ -91,22 +94,3 @@ func TestPsiphonNilTunnel(t *testing.T) { } tunnel.Stop() // must not crash } - -type psiphonFakeDependencies struct { - MkdirAllErr error - RemoveAllErr error - StartErr error -} - -func (fd psiphonFakeDependencies) MkdirAll(path string, perm os.FileMode) error { - return fd.MkdirAllErr -} - -func (fd psiphonFakeDependencies) RemoveAll(path string) error { - return fd.RemoveAllErr -} - -func (fd psiphonFakeDependencies) Start( - ctx context.Context, config []byte, workdir string) (*clientlib.PsiphonTunnel, error) { - return nil, fd.StartErr -} diff --git a/internal/engine/tunnel/tunnel.go b/internal/engine/tunnel/tunnel.go index a714e94..d11926e 100644 --- a/internal/engine/tunnel/tunnel.go +++ b/internal/engine/tunnel/tunnel.go @@ -27,20 +27,6 @@ type Tunnel interface { Stop() } -// Config contains the configuration for creating a Tunnel instance. -type Config struct { - // Name is the mandatory name of the tunnel. We support - // "tor" and "psiphon" tunnels. - Name string - - // Session is the current measurement session. - Session Session - - // WorkDir is the directory in which the tunnel SHOULD - // store its state, if any. - WorkDir string -} - // Start starts a new tunnel by name or returns an error. Note that if you // pass to this function the "" tunnel, you get back nil, nil. func Start(ctx context.Context, config *Config) (Tunnel, error) { @@ -51,9 +37,7 @@ func Start(ctx context.Context, config *Config) (Tunnel, error) { return enforceNilContract(nil, nil) case "psiphon": logger.Infof("starting %s tunnel; please be patient...", config.Name) - tun, err := psiphonStart(ctx, config.Session, psiphonConfig{ - WorkDir: config.WorkDir, - }) + tun, err := psiphonStart(ctx, config) return enforceNilContract(tun, err) case "tor": logger.Infof("starting %s tunnel; please be patient...", config.Name)