refactor(tunnel): remove nil tunnels hack (#296)

* refactor(tunnel): remove nil tunnels hack

This code was originally introduced because a tunnel could be
nil in session.go. I have verified that every invocation of
tunnel.Start is careful to ensure that we have a tunnel name
and that we don't manipulate a nil tunnel.

For this reason, I'd rather remove this tricky bit of code and
further simplify the tunnel code.

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

* even better docs

internal/engine/tunnel/config.go

@@ -10,14 +10,16 @@ import (
 )
 
 // Config contains the configuration for creating a Tunnel instance. You need
-// to fill the mandatory fields. You SHOULD NOT modify the content of this
+// to fill all 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 mandatory measurement session.
+	// Session is the mandatory measurement session, or a suitable
+	// mock of the required functionality. That is, the possibility
+	// of obtaining a valid psiphon configuration.
 	Session Session
 
 	// TorArgs contains the optional arguments that you want us to pass

internal/engine/tunnel/psiphon.go

@@ -56,32 +56,21 @@ func psiphonStart(ctx context.Context, config *Config) (Tunnel, error) {
 	return &psiphonTunnel{tunnel: tunnel, bootstrapTime: 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 {
-		t.tunnel.Stop()
-	}
+	t.tunnel.Stop()
 }
 
 // SOCKS5ProxyURL returns the SOCKS5 proxy URL.
-func (t *psiphonTunnel) SOCKS5ProxyURL() (proxyURL *url.URL) {
-	if t != nil {
-		proxyURL = &url.URL{
-			Scheme: "socks5",
-			Host: net.JoinHostPort(
-				"127.0.0.1", fmt.Sprintf("%d", t.tunnel.SOCKSProxyPort)),
-		}
+func (t *psiphonTunnel) SOCKS5ProxyURL() *url.URL {
+	return &url.URL{
+		Scheme: "socks5",
+		Host: net.JoinHostPort(
+			"127.0.0.1", fmt.Sprintf("%d", t.tunnel.SOCKSProxyPort)),
 	}
-	return
 }
 
 // BootstrapTime returns the bootstrap time
-func (t *psiphonTunnel) BootstrapTime() (duration time.Duration) {
-	if t != nil {
-		duration = t.bootstrapTime
-	}
-	return
+func (t *psiphonTunnel) BootstrapTime() time.Duration {
+	return t.bootstrapTime
 }

internal/engine/tunnel/psiphon_test.go

@@ -67,14 +67,3 @@ func TestPsiphonStartFailure(t *testing.T) {
 		t.Fatal("expected nil tunnel here")
 	}
 }
-
-func TestPsiphonNilTunnel(t *testing.T) {
-	var tunnel *psiphonTunnel
-	if tunnel.BootstrapTime() != 0 {
-		t.Fatal("expected zero bootstrap time")
-	}
-	if tunnel.SOCKS5ProxyURL() != nil {
-		t.Fatal("expected nil SOCKS Proxy URL")
-	}
-	tunnel.Stop() // must not crash
-}

internal/engine/tunnel/tor.go

@@ -30,26 +30,18 @@ type torTunnel struct {
 }
 
 // BootstrapTime returns the bootstrap time
-func (tt *torTunnel) BootstrapTime() (duration time.Duration) {
-	if tt != nil {
-		duration = tt.bootstrapTime
-	}
-	return
+func (tt *torTunnel) BootstrapTime() time.Duration {
+	return tt.bootstrapTime
 }
 
 // SOCKS5ProxyURL returns the URL of the SOCKS5 proxy
-func (tt *torTunnel) SOCKS5ProxyURL() (url *url.URL) {
-	if tt != nil {
-		url = tt.proxy
-	}
-	return
+func (tt *torTunnel) SOCKS5ProxyURL() *url.URL {
+	return tt.proxy
 }
 
 // Stop stops the Tor tunnel
 func (tt *torTunnel) Stop() {
-	if tt != nil {
-		tt.instance.Close()
-	}
+	tt.instance.Close()
 }
 
 // TODO(bassosimone): the current design is such that we have a bunch of

internal/engine/tunnel/tor_test.go

@@ -43,17 +43,6 @@ func TestTorTunnelNonNil(t *testing.T) {
 	}
 }
 
-func TestTorTunnelNil(t *testing.T) {
-	var tun *torTunnel
-	if tun.BootstrapTime() != 0 {
-		t.Fatal("not the bootstrap time we expected")
-	}
-	if tun.SOCKS5ProxyURL() != nil {
-		t.Fatal("not the url we expected")
-	}
-	tun.Stop() // ensure we don't crash
-}
-
 func TestTorStartWithCancelledContext(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	cancel() // fail immediately

internal/engine/tunnel/tunnel.go

@@ -1,5 +1,26 @@
 // Package tunnel allows to create tunnels to speak
 // with OONI backends and other services.
+//
+// You need to fill a Config object and call Start to
+// obtain an instance of Tunnel. The tunnel will expose
+// a SOCKS5 proxy. You need to configure your HTTP
+// code to use such a proxy. Remember to call the Stop
+// method of a tunnel when you are done.
+//
+// There are two use cases for this package. The first
+// use case is to enable urlgetter to perform measurements
+// over tunnels (mainly psiphon).
+//
+// The second use case is to use tunnels to reach to the
+// OONI backend when it's blocked. For the latter case
+// we currently mainly use psiphon. In such a case, we'll
+// use a psiphon configuration embedded into the OONI
+// binary itself. When you are running a version of OONI
+// that does not embed such a configuration, it won't
+// be possible to address this use case.
+//
+// See session.go in the engine package for more details
+// concerning this second use case.
 package tunnel
 
 import (
@@ -10,21 +31,27 @@ import (
 	"time"
 )
 
-// Session is the way in which this package sees a Session.
+// Session is a measurement session. We filter for the only
+// functionality we're interested to use. That is, fetching the
+// psiphon configuration from the OONI backend (if possible).
 type Session interface {
+	// FetchPsiphonConfig should fetch and return the psiphon config
+	// as a serialized JSON, or fail with an error.
 	FetchPsiphonConfig(ctx context.Context) ([]byte, error)
 }
 
-// Tunnel is a tunnel used by the session
+// Tunnel is a tunnel for communicating with OONI backends
+// (and other services) to circumvent blocking.
 type Tunnel interface {
 	// BootstrapTime returns the time it required to
-	// create an instance of the tunnel
+	// create a new tunnel instance.
 	BootstrapTime() time.Duration
 
-	// SOCKS5ProxyURL returns the SOCSK5 proxy URL
+	// SOCKS5ProxyURL returns the SOCSK5 proxy URL.
 	SOCKS5ProxyURL() *url.URL
 
-	// Stop stops the tunnel. This method is idempotent.
+	// Stop stops the tunnel. You should not attempt to
+	// use any other tunnel method after Stop.
 	Stop()
 }
 
@@ -35,30 +62,29 @@ var ErrEmptyTunnelDir = errors.New("TunnelDir is empty")
 // 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.
+// Start starts a new tunnel by name or returns an error. We currently
+// support the following tunnels:
+//
+// The "tor" tunnel requires the "tor" binary to be installed on
+// your system. You can use config.TorArgs and config.TorBinary to
+// select what binary to execute and with which arguments.
+//
+// The "psiphon" tunnel requires a configuration. Some builds of
+// ooniprobe embed a configuration into the binary. When this
+// is the case, the config.Session is a mocked object that just
+// retuns such configuration.
+//
+// Otherwise, If there is no embedded psiphon configuration, the
+// config.Session will must be an ordinary session. In such a
+// case, fetching the Psiphon configuration from the backend may
+// fail when the backend is not reachable.
 func Start(ctx context.Context, config *Config) (Tunnel, error) {
 	switch config.Name {
-	case "":
-		return enforceNilContract(nil, nil)
 	case "psiphon":
-		tun, err := psiphonStart(ctx, config)
-		return enforceNilContract(tun, err)
+		return psiphonStart(ctx, config)
 	case "tor":
-		tun, err := torStart(ctx, config)
-		return enforceNilContract(tun, err)
+		return torStart(ctx, config)
 	default:
 		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
-	}
-	return tun, nil
-}

internal/engine/tunnel/tunnel_test.go

@@ -17,8 +17,8 @@ func TestStartNoTunnel(t *testing.T) {
 			MockableLogger: log.Log,
 		},
 	})
-	if err != nil {
-		t.Fatal(err)
+	if !errors.Is(err, ErrUnsupportedTunnelName) {
+		t.Fatal("not the error we expected", err)
 	}
 	if tunnel != nil {
 		t.Fatal("expected nil tunnel here")