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

@@ -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)
+}

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(
+func psiphonStart(ctx context.Context, config *Config) (Tunnel, error) {
-	ctx context.Context, sess Session, config psiphonConfig) (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 {

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{
+	tunnel, err := psiphonStart(context.Background(), &Config{
-		Dependencies: dependencies,
+		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{
+	tunnel, err := psiphonStart(context.Background(), &Config{
-		Dependencies: dependencies,
+		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{
+	tunnel, err := psiphonStart(context.Background(), &Config{
-		Dependencies: dependencies,
+		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
-}

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{
+		tun, err := psiphonStart(ctx, config)
-			WorkDir: config.WorkDir,
-		})
 		return enforceNilContract(tun, err)
 	case "tor":
 		logger.Infof("starting %s tunnel; please be patient...", config.Name)