feat(tunnel): introduce persistent tunnel state dir (#294)

* feat(tunnel): introduce persistent tunnel state dir

This diff introduces a persistent state directory for tunnels, so that
we can bootstrap them more quickly after the first time.

Part of https://github.com/ooni/probe/issues/985

* fix: make tunnel dir optional

We have many tests where it does not make sense to explicitly
provide a tunnel dir because we're not using tunnels.

This should simplify setting up a session.

* fix(tunnel): repair tests

* final changes

* more cleanups

internal/engine/tunnel/config.go

@@ -9,35 +9,36 @@ import (
 	"github.com/ooni/psiphon/oopsi/github.com/Psiphon-Labs/psiphon-tunnel-core/ClientLibrary/clientlib"
 )
 
-// Config contains the configuration for creating a Tunnel instance.
+// Config contains the configuration for creating a Tunnel instance. You need
+// to fill the mandatory fields. You SHOULD NOT modify the content of this
+// structure while in use, because that may lead to data races.
 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 is the current measurement session. This
+	// field is mandatory.
 	Session Session
 
-	// TorArgs contains the arguments that you want us to pass
+	// TorArgs contains the optional arguments that you want us to pass
 	// to the tor binary when invoking it. By default we do not
 	// pass any extra argument. This flag might be useful to
 	// configure pluggable transports.
 	TorArgs []string
 
-	// TorBinary is the path of the TorBinary we SHOULD be
+	// TorBinary is the optional path of the TorBinary we SHOULD be
 	// executing. When not set, we execute `tor`.
 	TorBinary string
 
-	// WorkDir is the directory in which the tunnel SHOULD
-	// store its state, if any.
-	WorkDir string
+	// TunnelDir is the mandatory directory in which the tunnel SHOULD
+	// store its state, if any. If this field is empty, the
+	// Start function fails with ErrEmptyTunnelDir.
+	TunnelDir 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)
@@ -62,14 +63,6 @@ func (c *Config) mkdirAll(path string, perm os.FileMode) error {
 	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) {

internal/engine/tunnel/integration_test.go

@@ -11,19 +11,23 @@ import (
 )
 
 func TestPsiphonStartWithCancelledContext(t *testing.T) {
+	// TODO(bassosimone): this test can use a mockable session so we
+	// can move it inside of the internal tests.
 	ctx, cancel := context.WithCancel(context.Background())
-	cancel()
+	cancel() // fail immediately
 	sess, err := engine.NewSession(engine.SessionConfig{
 		Logger:          log.Log,
-		SoftwareName:    "ooniprobe-engine",
-		SoftwareVersion: "0.0.1",
+		SoftwareName:    "miniooni",
+		SoftwareVersion: "0.1.0-dev",
+		TunnelDir:       "testdata",
 	})
 	if err != nil {
 		t.Fatal(err)
 	}
 	tunnel, err := tunnel.Start(ctx, &tunnel.Config{
-		Name:    "psiphon",
-		Session: sess,
+		Name:      "psiphon",
+		Session:   sess,
+		TunnelDir: "testdata",
 	})
 	if !errors.Is(err, context.Canceled) {
 		t.Fatal("not the error we expected")
@@ -41,13 +45,15 @@ func TestPsiphonStartStop(t *testing.T) {
 		Logger:          log.Log,
 		SoftwareName:    "ooniprobe-engine",
 		SoftwareVersion: "0.0.1",
+		TunnelDir:       "testdata",
 	})
 	if err != nil {
 		t.Fatal(err)
 	}
 	tunnel, err := tunnel.Start(context.Background(), &tunnel.Config{
-		Name:    "psiphon",
-		Session: sess,
+		Name:      "psiphon",
+		Session:   sess,
+		TunnelDir: "testdata",
 	})
 	if err != nil {
 		t.Fatal(err)

internal/engine/tunnel/psiphon.go

@@ -13,29 +13,16 @@ import (
 
 // psiphonTunnel is a psiphon tunnel
 type psiphonTunnel struct {
+	// bootstrapTime is the bootstrapTime of the bootstrap
+	bootstrapTime time.Duration
+
 	// tunnel is the underlying psiphon tunnel
 	tunnel *clientlib.PsiphonTunnel
-
-	// duration is the duration of the bootstrap
-	duration time.Duration
 }
 
-// TODO(bassosimone): _always_ wiping the state directory
-// here is absolutely wrong. This prevents us from reusing
-// an existing psiphon cache existing on disk. We want to
-// delete the directory _only_ in the psiphon nettest.
-
 // psiphonMakeWorkingDir creates the working directory
 func psiphonMakeWorkingDir(config *Config) (string, error) {
-	const testdirname = "oonipsiphon"
-	baseWorkDir := config.WorkDir
-	if baseWorkDir == "" {
-		baseWorkDir = config.Session.TempDir()
-	}
-	workdir := filepath.Join(baseWorkDir, testdirname)
-	if err := config.removeAll(workdir); err != nil {
-		return "", err
-	}
+	workdir := filepath.Join(config.TunnelDir, config.Name)
 	if err := config.mkdirAll(workdir, 0700); err != nil {
 		return "", err
 	}
@@ -49,6 +36,9 @@ func psiphonStart(ctx context.Context, config *Config) (Tunnel, error) {
 		return nil, ctx.Err() // simplifies unit testing this code
 	default:
 	}
+	if config.TunnelDir == "" {
+		return nil, ErrEmptyTunnelDir
+	}
 	configJSON, err := config.Session.FetchPsiphonConfig(ctx)
 	if err != nil {
 		return nil, err
@@ -63,7 +53,7 @@ func psiphonStart(ctx context.Context, config *Config) (Tunnel, error) {
 		return nil, err
 	}
 	stop := time.Now()
-	return &psiphonTunnel{tunnel: tunnel, duration: stop.Sub(start)}, nil
+	return &psiphonTunnel{tunnel: tunnel, bootstrapTime: stop.Sub(start)}, nil
 }
 
 // TODO(bassosimone): define the NullTunnel rather than relying on
@@ -91,7 +81,7 @@ func (t *psiphonTunnel) SOCKS5ProxyURL() (proxyURL *url.URL) {
 // BootstrapTime returns the bootstrap time
 func (t *psiphonTunnel) BootstrapTime() (duration time.Duration) {
 	if t != nil {
-		duration = t.duration
+		duration = t.bootstrapTime
 	}
 	return
 }

internal/engine/tunnel/psiphon_test.go

@@ -16,7 +16,8 @@ func TestPsiphonFetchPsiphonConfigFailure(t *testing.T) {
 		MockableFetchPsiphonConfigErr: expected,
 	}
 	tunnel, err := psiphonStart(context.Background(), &Config{
-		Session: sess,
+		Session:   sess,
+		TunnelDir: "testdata",
 	})
 	if !errors.Is(err, expected) {
 		t.Fatal("not the error we expected")
@@ -32,7 +33,8 @@ func TestPsiphonMkdirAllFailure(t *testing.T) {
 		MockableFetchPsiphonConfigResult: []byte(`{}`),
 	}
 	tunnel, err := psiphonStart(context.Background(), &Config{
-		Session: sess,
+		Session:   sess,
+		TunnelDir: "testdata",
 		testMkdirAll: func(path string, perm os.FileMode) error {
 			return expected
 		},
@@ -45,32 +47,14 @@ func TestPsiphonMkdirAllFailure(t *testing.T) {
 	}
 }
 
-func TestPsiphonRemoveAllFailure(t *testing.T) {
-	expected := errors.New("mocked error")
-	sess := &mockable.Session{
-		MockableFetchPsiphonConfigResult: []byte(`{}`),
-	}
-	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")
-	}
-	if tunnel != nil {
-		t.Fatal("expected nil tunnel here")
-	}
-}
-
 func TestPsiphonStartFailure(t *testing.T) {
 	expected := errors.New("mocked error")
 	sess := &mockable.Session{
 		MockableFetchPsiphonConfigResult: []byte(`{}`),
 	}
 	tunnel, err := psiphonStart(context.Background(), &Config{
-		Session: sess,
+		Session:   sess,
+		TunnelDir: "testdata",
 		testStartPsiphon: func(ctx context.Context, config []byte,
 			workdir string) (*clientlib.PsiphonTunnel, error) {
 			return nil, expected

internal/engine/tunnel/testdata/.gitignore

@@ -0,0 +1 @@
+*

internal/engine/tunnel/tor.go

@@ -3,18 +3,18 @@ package tunnel
 import (
 	"context"
 	"fmt"
+	"io"
 	"net/url"
-	"path"
+	"path/filepath"
 	"strings"
 	"time"
 
 	"github.com/cretz/bine/tor"
 )
 
-// torProcess is a running tor process
+// torProcess is a running tor process.
 type torProcess interface {
-	// Close kills the running tor process
-	Close() error
+	io.Closer
 }
 
 // torTunnel is the Tor tunnel
@@ -52,6 +52,9 @@ func (tt *torTunnel) Stop() {
 	}
 }
 
+// TODO(bassosimone): the current design is such that we have a bunch of
+// torrc-$number and a growing tor.log file inside of stateDir.
+
 // torStart starts the tor tunnel.
 func torStart(ctx context.Context, config *Config) (Tunnel, error) {
 	select {
@@ -59,14 +62,18 @@ func torStart(ctx context.Context, config *Config) (Tunnel, error) {
 		return nil, ctx.Err() // allows to write unit tests using this code
 	default:
 	}
-	logfile := LogFile(config.Session)
+	if config.TunnelDir == "" {
+		return nil, ErrEmptyTunnelDir
+	}
+	stateDir := filepath.Join(config.TunnelDir, "tor")
+	logfile := filepath.Join(stateDir, "tor.log")
 	extraArgs := append([]string{}, config.TorArgs...)
 	extraArgs = append(extraArgs, "Log")
 	extraArgs = append(extraArgs, "notice stderr")
 	extraArgs = append(extraArgs, "Log")
 	extraArgs = append(extraArgs, fmt.Sprintf(`notice file %s`, logfile))
 	instance, err := config.torStart(ctx, &tor.StartConf{
-		DataDir:   path.Join(config.Session.TempDir(), "tor"),
+		DataDir:   stateDir,
 		ExtraArgs: extraArgs,
 		ExePath:   config.TorBinary,
 		NoHush:    true,
@@ -102,9 +109,3 @@ func torStart(ctx context.Context, config *Config) (Tunnel, error) {
 		proxy:         &url.URL{Scheme: "socks5", Host: proxyAddress},
 	}, nil
 }
-
-// LogFile returns the name of tor logs given a specific session. The file
-// is always located somewhere inside the sess.TempDir() directory.
-func LogFile(sess Session) string {
-	return path.Join(sess.TempDir(), "tor.log")
-}

internal/engine/tunnel/tor_test.go

@@ -11,17 +11,20 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/engine/internal/mockable"
 )
 
-type Closer struct {
+// torCloser is used to mock a running tor process, which
+// we abstract as a io.Closer in tor.go.
+type torCloser struct {
 	counter int
 }
 
-func (c *Closer) Close() error {
+// Close implements io.Closer.Close.
+func (c *torCloser) Close() error {
 	c.counter++
 	return errors.New("mocked mocked mocked")
 }
 
 func TestTorTunnelNonNil(t *testing.T) {
-	closer := new(Closer)
+	closer := new(torCloser)
 	proxy := &url.URL{Scheme: "x", Host: "10.0.0.1:443"}
 	tun := &torTunnel{
 		bootstrapTime: 128,
@@ -53,8 +56,11 @@ func TestTorTunnelNil(t *testing.T) {
 
 func TestTorStartWithCancelledContext(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
-	cancel()
-	tun, err := torStart(ctx, &Config{Session: &mockable.Session{}})
+	cancel() // fail immediately
+	tun, err := torStart(ctx, &Config{
+		Session:   &mockable.Session{},
+		TunnelDir: "testdata",
+	})
 	if !errors.Is(err, context.Canceled) {
 		t.Fatal("not the error we expected")
 	}
@@ -67,7 +73,8 @@ func TestTorStartStartFailure(t *testing.T) {
 	expected := errors.New("mocked error")
 	ctx := context.Background()
 	tun, err := torStart(ctx, &Config{
-		Session: &mockable.Session{},
+		Session:   &mockable.Session{},
+		TunnelDir: "testdata",
 		testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 			return nil, expected
 		},
@@ -84,7 +91,8 @@ func TestTorStartEnableNetworkFailure(t *testing.T) {
 	expected := errors.New("mocked error")
 	ctx := context.Background()
 	tun, err := torStart(ctx, &Config{
-		Session: &mockable.Session{},
+		Session:   &mockable.Session{},
+		TunnelDir: "testdata",
 		testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 			return &tor.Tor{}, nil
 		},
@@ -104,7 +112,8 @@ func TestTorStartGetInfoFailure(t *testing.T) {
 	expected := errors.New("mocked error")
 	ctx := context.Background()
 	tun, err := torStart(ctx, &Config{
-		Session: &mockable.Session{},
+		Session:   &mockable.Session{},
+		TunnelDir: "testdata",
 		testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 			return &tor.Tor{}, nil
 		},
@@ -126,7 +135,8 @@ func TestTorStartGetInfoFailure(t *testing.T) {
 func TestTorStartGetInfoInvalidNumberOfKeys(t *testing.T) {
 	ctx := context.Background()
 	tun, err := torStart(ctx, &Config{
-		Session: &mockable.Session{},
+		Session:   &mockable.Session{},
+		TunnelDir: "testdata",
 		testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 			return &tor.Tor{}, nil
 		},
@@ -148,7 +158,8 @@ func TestTorStartGetInfoInvalidNumberOfKeys(t *testing.T) {
 func TestTorStartGetInfoInvalidKey(t *testing.T) {
 	ctx := context.Background()
 	tun, err := torStart(ctx, &Config{
-		Session: &mockable.Session{},
+		Session:   &mockable.Session{},
+		TunnelDir: "testdata",
 		testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 			return &tor.Tor{}, nil
 		},
@@ -170,7 +181,8 @@ func TestTorStartGetInfoInvalidKey(t *testing.T) {
 func TestTorStartGetInfoInvalidProxyType(t *testing.T) {
 	ctx := context.Background()
 	tun, err := torStart(ctx, &Config{
-		Session: &mockable.Session{},
+		Session:   &mockable.Session{},
+		TunnelDir: "testdata",
 		testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 			return &tor.Tor{}, nil
 		},
@@ -192,7 +204,8 @@ func TestTorStartGetInfoInvalidProxyType(t *testing.T) {
 func TestTorStartUnsupportedProxy(t *testing.T) {
 	ctx := context.Background()
 	tun, err := torStart(ctx, &Config{
-		Session: &mockable.Session{},
+		Session:   &mockable.Session{},
+		TunnelDir: "testdata",
 		testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 			return &tor.Tor{}, nil
 		},

internal/engine/tunnel/tunnel.go

@@ -5,6 +5,7 @@ package tunnel
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net/url"
 	"time"
 )
@@ -12,16 +13,28 @@ import (
 // Session is the way in which this package sees a Session.
 type Session interface {
 	FetchPsiphonConfig(ctx context.Context) ([]byte, error)
-	TempDir() string
 }
 
 // Tunnel is a tunnel used by the session
 type Tunnel interface {
+	// BootstrapTime returns the time it required to
+	// create an instance of the tunnel
 	BootstrapTime() time.Duration
+
+	// SOCKS5ProxyURL returns the SOCSK5 proxy URL
 	SOCKS5ProxyURL() *url.URL
+
+	// Stop stops the tunnel. This method is idempotent.
 	Stop()
 }
 
+// ErrEmptyTunnelDir indicates that config.TunnelDir is empty.
+var ErrEmptyTunnelDir = errors.New("TunnelDir is empty")
+
+// ErrUnsupportedTunnelName indicates that the given tunnel name
+// is not supported by this package.
+var ErrUnsupportedTunnelName = errors.New("unsupported tunnel name")
+
 // 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) {
@@ -35,11 +48,15 @@ func Start(ctx context.Context, config *Config) (Tunnel, error) {
 		tun, err := torStart(ctx, config)
 		return enforceNilContract(tun, err)
 	default:
-		return nil, errors.New("unsupported tunnel")
+		return nil, fmt.Errorf("%w: %s", ErrUnsupportedTunnelName, config.Name)
 	}
 }
 
+// enforceNilContract ensures that either the tunnel is nil
+// or the error is nil.
 func enforceNilContract(tun Tunnel, err error) (Tunnel, error) {
+	// TODO(bassosimone): we currently allow returning nil, nil but
+	// we want to change this to return a fake NilTunnel.
 	if err != nil {
 		return nil, err
 	}

internal/engine/tunnel/tunnel_test.go

@@ -10,8 +10,7 @@ import (
 )
 
 func TestStartNoTunnel(t *testing.T) {
-	ctx, cancel := context.WithCancel(context.Background())
-	cancel()
+	ctx := context.Background()
 	tunnel, err := Start(ctx, &Config{
 		Name: "",
 		Session: &mockable.Session{
@@ -26,14 +25,15 @@ func TestStartNoTunnel(t *testing.T) {
 	}
 }
 
-func TestStartPsiphonTunnel(t *testing.T) {
+func TestStartPsiphonWithCancelledContext(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
-	cancel()
+	cancel() // fail immediately
 	tunnel, err := Start(ctx, &Config{
 		Name: "psiphon",
 		Session: &mockable.Session{
 			MockableLogger: log.Log,
 		},
+		TunnelDir: "testdata",
 	})
 	if !errors.Is(err, context.Canceled) {
 		t.Fatal("not the error we expected")
@@ -43,14 +43,15 @@ func TestStartPsiphonTunnel(t *testing.T) {
 	}
 }
 
-func TestStartTorTunnel(t *testing.T) {
+func TestStartTorWithCancelledContext(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
-	cancel()
+	cancel() // fail immediately
 	tunnel, err := Start(ctx, &Config{
 		Name: "tor",
 		Session: &mockable.Session{
 			MockableLogger: log.Log,
 		},
+		TunnelDir: "testdata",
 	})
 	if !errors.Is(err, context.Canceled) {
 		t.Fatal("not the error we expected")
@@ -61,18 +62,17 @@ func TestStartTorTunnel(t *testing.T) {
 }
 
 func TestStartInvalidTunnel(t *testing.T) {
-	ctx, cancel := context.WithCancel(context.Background())
-	cancel()
+	ctx := context.Background()
 	tunnel, err := Start(ctx, &Config{
 		Name: "antani",
 		Session: &mockable.Session{
 			MockableLogger: log.Log,
 		},
+		TunnelDir: "testdata",
 	})
-	if err == nil || err.Error() != "unsupported tunnel" {
+	if !errors.Is(err, ErrUnsupportedTunnelName) {
 		t.Fatal("not the error we expected")
 	}
-	t.Log(tunnel)
 	if tunnel != nil {
 		t.Fatal("expected nil tunnel here")
 	}