From c5ad5eedeb5dfe88ecb42ec7315a4d6e46978101 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 5 Apr 2021 15:28:13 +0200 Subject: [PATCH] feat: create tunnel inside NewSession (#286) * feat: create tunnel inside NewSession We want to create the tunnel when we create the session. This change allows us to nicely ignore the problem of creating a tunnel when we already have a proxy, as well as the problem of locking. Everything is happening, in fact, inside of the NewSession factory. Modify miniooni such that --tunnel is just syntactic sugar for --proxy, at least for now. We want, in the future, to teach the tunnel to possibly use a socks5 proxy. Because starting a tunnel is a slow operation, we need a context in NewSession. This causes a bunch of places to change. Not really a big deal except we need to propagate the changes. Make sure that the mobile code can create a new session using a proxy for all the APIs we support. Make sure all tests are still green and we don't loose coverage of the various ways in which this code could be used. This change is part of https://github.com/ooni/probe/issues/985. * changes after merge * fix: only keep tests that can hopefully work While there, identify other places where we should add more tests or fix integration tests. Part of https://github.com/ooni/probe/issues/985 --- cmd/ooniprobe/internal/cli/geoip/geoip.go | 4 +- .../internal/nettests/nettests_test.go | 3 +- cmd/ooniprobe/internal/nettests/run.go | 3 +- cmd/ooniprobe/internal/ooni/ooni.go | 11 +- cmd/ooniprobe/internal/oonitest/oonitest.go | 3 +- internal/cmd/miniooni/libminiooni.go | 23 ++- .../fbmessenger/fbmessenger_test.go | 2 +- internal/engine/experiment/hhfm/hhfm_test.go | 2 +- .../engine/experiment/urlgetter/getter.go | 4 + .../experiment/urlgetter/getter_test.go | 3 + .../webconnectivity/webconnectivity_test.go | 2 +- internal/engine/inputloader_network_test.go | 2 +- internal/engine/inputloader_test.go | 2 +- internal/engine/session.go | 128 ++++++++-------- internal/engine/session_integration_test.go | 137 ++---------------- internal/engine/tunnel/integration_test.go | 5 +- pkg/oonimkall/internal/tasks/runner.go | 6 +- pkg/oonimkall/session.go | 18 ++- 18 files changed, 135 insertions(+), 223 deletions(-) diff --git a/cmd/ooniprobe/internal/cli/geoip/geoip.go b/cmd/ooniprobe/internal/cli/geoip/geoip.go index d757b5c..4a506a3 100644 --- a/cmd/ooniprobe/internal/cli/geoip/geoip.go +++ b/cmd/ooniprobe/internal/cli/geoip/geoip.go @@ -1,6 +1,8 @@ package geoip import ( + "context" + "github.com/alecthomas/kingpin" "github.com/apex/log" "github.com/ooni/probe-cli/v3/cmd/ooniprobe/internal/cli/root" @@ -34,7 +36,7 @@ func dogeoip(config dogeoipconfig) error { return err } - engine, err := probeCLI.NewProbeEngine() + engine, err := probeCLI.NewProbeEngine(context.Background()) if err != nil { return err } diff --git a/cmd/ooniprobe/internal/nettests/nettests_test.go b/cmd/ooniprobe/internal/nettests/nettests_test.go index cfe240a..a555b11 100644 --- a/cmd/ooniprobe/internal/nettests/nettests_test.go +++ b/cmd/ooniprobe/internal/nettests/nettests_test.go @@ -1,6 +1,7 @@ package nettests import ( + "context" "io/ioutil" "path" "testing" @@ -34,7 +35,7 @@ func TestCreateContext(t *testing.T) { func TestRun(t *testing.T) { probe := newOONIProbe(t) - sess, err := probe.NewSession() + sess, err := probe.NewSession(context.Background()) if err != nil { t.Fatal(err) } diff --git a/cmd/ooniprobe/internal/nettests/run.go b/cmd/ooniprobe/internal/nettests/run.go index 6450cdf..137a6b6 100644 --- a/cmd/ooniprobe/internal/nettests/run.go +++ b/cmd/ooniprobe/internal/nettests/run.go @@ -1,6 +1,7 @@ package nettests import ( + "context" "sync" "time" @@ -57,7 +58,7 @@ func RunGroup(config RunGroupConfig) error { return nil } - sess, err := config.Probe.NewSession() + sess, err := config.Probe.NewSession(context.Background()) if err != nil { log.WithError(err).Error("Failed to create a measurement session") return err diff --git a/cmd/ooniprobe/internal/ooni/ooni.go b/cmd/ooniprobe/internal/ooni/ooni.go index 61f354d..41fd682 100644 --- a/cmd/ooniprobe/internal/ooni/ooni.go +++ b/cmd/ooniprobe/internal/ooni/ooni.go @@ -1,6 +1,7 @@ package ooni import ( + "context" _ "embed" // because we embed a file "io/ioutil" "os" @@ -26,7 +27,7 @@ type ProbeCLI interface { IsBatch() bool Home() string TempDir() string - NewProbeEngine() (ProbeEngine, error) + NewProbeEngine(ctx context.Context) (ProbeEngine, error) } // ProbeEngine is an instance of the OONI Probe engine. @@ -201,7 +202,7 @@ func (p *Probe) Init(softwareName, softwareVersion string) error { // NewSession creates a new ooni/probe-engine session using the // current configuration inside the context. The caller must close // the session when done using it, by calling sess.Close(). -func (p *Probe) NewSession() (*engine.Session, error) { +func (p *Probe) NewSession(ctx context.Context) (*engine.Session, error) { kvstore, err := engine.NewFileSystemKVStore( utils.EngineDir(p.home), ) @@ -211,7 +212,7 @@ func (p *Probe) NewSession() (*engine.Session, error) { if err := os.MkdirAll(utils.TunnelDir(p.home), 0700); err != nil { return nil, errors.Wrap(err, "creating tunnel dir") } - return engine.NewSession(engine.SessionConfig{ + return engine.NewSession(ctx, engine.SessionConfig{ KVStore: kvstore, Logger: enginex.Logger, SoftwareName: p.softwareName, @@ -222,8 +223,8 @@ func (p *Probe) NewSession() (*engine.Session, error) { } // NewProbeEngine creates a new ProbeEngine instance. -func (p *Probe) NewProbeEngine() (ProbeEngine, error) { - sess, err := p.NewSession() +func (p *Probe) NewProbeEngine(ctx context.Context) (ProbeEngine, error) { + sess, err := p.NewSession(ctx) if err != nil { return nil, err } diff --git a/cmd/ooniprobe/internal/oonitest/oonitest.go b/cmd/ooniprobe/internal/oonitest/oonitest.go index 5483171..555b620 100644 --- a/cmd/ooniprobe/internal/oonitest/oonitest.go +++ b/cmd/ooniprobe/internal/oonitest/oonitest.go @@ -2,6 +2,7 @@ package oonitest import ( + "context" "sync" "github.com/apex/log" @@ -60,7 +61,7 @@ func (cli *FakeProbeCLI) TempDir() string { } // NewProbeEngine implements ProbeCLI.NewProbeEngine -func (cli *FakeProbeCLI) NewProbeEngine() (ooni.ProbeEngine, error) { +func (cli *FakeProbeCLI) NewProbeEngine(ctx context.Context) (ooni.ProbeEngine, error) { return cli.FakeProbeEnginePtr, cli.FakeProbeEngineErr } diff --git a/internal/cmd/miniooni/libminiooni.go b/internal/cmd/miniooni/libminiooni.go index 70d734e..d023e40 100644 --- a/internal/cmd/miniooni/libminiooni.go +++ b/internal/cmd/miniooni/libminiooni.go @@ -140,6 +140,10 @@ func fatalIfFalse(cond bool, msg string) { } } +func fatalIfTrue(cond bool, msg string) { + fatalIfFalse(!cond, msg) +} + // Main is the main function of miniooni. This function parses the command line // options and uses a global state. Use MainWithConfiguration if you want to avoid // using any global state and relying on command line options. @@ -273,6 +277,15 @@ of seconds after which to stop running Web Connectivity. This error message will be removed after 2021-11-01. ` +// tunnelAndProxy is the text printed when the user specifies +// both the --tunnel and the --proxy options +const tunnelAndProxy = `USAGE ERROR: The --tunnel option and the --proxy +option cannot be specified at the same time. The --tunnel option is actually +just syntactic sugar for --proxy. Setting --tunnel=psiphon is currently the +equivalent of setting --proxy=psiphon:///. This MAY change in a future version +of miniooni, when we will allow a tunnel to use a proxy. +` + // MainWithConfiguration is the miniooni main with a specific configuration // represented by the experiment name and the current options. // @@ -280,6 +293,11 @@ This error message will be removed after 2021-11-01. // integrate this function to either handle the panic of ignore it. func MainWithConfiguration(experimentName string, currentOptions Options) { fatalIfFalse(currentOptions.Limit == 0, limitRemoved) + fatalIfTrue(currentOptions.Proxy != "" && currentOptions.Tunnel != "", + tunnelAndProxy) + if currentOptions.Tunnel != "" { + currentOptions.Proxy = fmt.Sprintf("%s:///", currentOptions.Tunnel) + } ctx := context.Background() @@ -354,7 +372,7 @@ func MainWithConfiguration(experimentName string, currentOptions Options) { }} } - sess, err := engine.NewSession(config) + sess, err := engine.NewSession(ctx, config) fatalOnError(err, "cannot create measurement session") defer func() { sess.Close() @@ -365,9 +383,6 @@ func MainWithConfiguration(experimentName string, currentOptions Options) { }() log.Debugf("miniooni temporary directory: %s", sess.TempDir()) - err = sess.MaybeStartTunnel(context.Background(), currentOptions.Tunnel) - fatalOnError(err, "cannot start session tunnel") - log.Info("Looking up OONI backends; please be patient...") err = sess.MaybeLookupBackends() fatalOnError(err, "cannot lookup OONI backends") diff --git a/internal/engine/experiment/fbmessenger/fbmessenger_test.go b/internal/engine/experiment/fbmessenger/fbmessenger_test.go index ab6438e..608b809 100644 --- a/internal/engine/experiment/fbmessenger/fbmessenger_test.go +++ b/internal/engine/experiment/fbmessenger/fbmessenger_test.go @@ -221,7 +221,7 @@ func TestComputeEndpointStatsDNSIsLying(t *testing.T) { } func newsession(t *testing.T) model.ExperimentSession { - sess, err := engine.NewSession(engine.SessionConfig{ + sess, err := engine.NewSession(context.Background(), engine.SessionConfig{ AvailableProbeServices: []model.Service{{ Address: "https://ams-pg-test.ooni.org", Type: "https", diff --git a/internal/engine/experiment/hhfm/hhfm_test.go b/internal/engine/experiment/hhfm/hhfm_test.go index 1cd1504..b3190fd 100644 --- a/internal/engine/experiment/hhfm/hhfm_test.go +++ b/internal/engine/experiment/hhfm/hhfm_test.go @@ -558,7 +558,7 @@ func TestTransactCannotReadBody(t *testing.T) { } func newsession(t *testing.T) model.ExperimentSession { - sess, err := engine.NewSession(engine.SessionConfig{ + sess, err := engine.NewSession(context.Background(), engine.SessionConfig{ AvailableProbeServices: []model.Service{{ Address: "https://ams-pg-test.ooni.org", Type: "https", diff --git a/internal/engine/experiment/urlgetter/getter.go b/internal/engine/experiment/urlgetter/getter.go index 12b5855..9dcd734 100644 --- a/internal/engine/experiment/urlgetter/getter.go +++ b/internal/engine/experiment/urlgetter/getter.go @@ -82,6 +82,10 @@ func (g Getter) Get(ctx context.Context) (TestKeys, error) { return tk, err } +// TODO(bassosimone): this mechanism where we count breaks tests +// because now tests are not idempotent anymore. Therefore, we +// SHOULD be creating a temporary directory instead. + var ( // tunnelDirCount counts the number of tunnels started by // the urlgetter package so far. diff --git a/internal/engine/experiment/urlgetter/getter_test.go b/internal/engine/experiment/urlgetter/getter_test.go index e08e5d8..6c02991 100644 --- a/internal/engine/experiment/urlgetter/getter_test.go +++ b/internal/engine/experiment/urlgetter/getter_test.go @@ -654,6 +654,9 @@ func TestGetterIntegrationHTTPSWithTunnel(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") } + // TODO(bassosimone): this test is broken. It now requires a + // real Session to work as intended. We didn't notice until now + // because integration tests do not run for every PR. ctx := context.Background() g := urlgetter.Getter{ Config: urlgetter.Config{ diff --git a/internal/engine/experiment/webconnectivity/webconnectivity_test.go b/internal/engine/experiment/webconnectivity/webconnectivity_test.go index 13a6bfa..dc4dc35 100644 --- a/internal/engine/experiment/webconnectivity/webconnectivity_test.go +++ b/internal/engine/experiment/webconnectivity/webconnectivity_test.go @@ -201,7 +201,7 @@ func TestMeasureWithNoAvailableTestHelpers(t *testing.T) { } func newsession(t *testing.T, lookupBackends bool) model.ExperimentSession { - sess, err := engine.NewSession(engine.SessionConfig{ + sess, err := engine.NewSession(context.Background(), engine.SessionConfig{ AvailableProbeServices: []model.Service{{ Address: "https://ams-pg-test.ooni.org", Type: "https", diff --git a/internal/engine/inputloader_network_test.go b/internal/engine/inputloader_network_test.go index 97c5e24..729a869 100644 --- a/internal/engine/inputloader_network_test.go +++ b/internal/engine/inputloader_network_test.go @@ -14,7 +14,7 @@ func TestInputLoaderInputOrQueryBackendWithNoInput(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") } - sess, err := engine.NewSession(engine.SessionConfig{ + sess, err := engine.NewSession(context.Background(), engine.SessionConfig{ AvailableProbeServices: []model.Service{{ Address: "https://ams-pg-test.ooni.org/", Type: "https", diff --git a/internal/engine/inputloader_test.go b/internal/engine/inputloader_test.go index 764796c..251b5dc 100644 --- a/internal/engine/inputloader_test.go +++ b/internal/engine/inputloader_test.go @@ -236,7 +236,7 @@ func TestInputLoaderInputOrQueryBackendWithInput(t *testing.T) { } func TestInputLoaderInputOrQueryBackendWithNoInputAndCancelledContext(t *testing.T) { - sess, err := NewSession(SessionConfig{ + sess, err := NewSession(context.Background(), SessionConfig{ KVStore: kvstore.NewMemoryKeyValueStore(), Logger: log.Log, SoftwareName: "miniooni", diff --git a/internal/engine/session.go b/internal/engine/session.go index c34f0df..4c1d31c 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -63,12 +63,6 @@ type Session struct { softwareName string softwareVersion string tempDir string - torArgs []string - torBinary string - tunnelDir string - tunnelMu sync.Mutex - tunnelName string - tunnel tunnel.Tunnel // closeOnce allows us to call Close just once. closeOnce sync.Once @@ -92,6 +86,18 @@ type Session struct { // allowing us to mock NewProbeServicesClient when calling CheckIn. testNewProbeServicesClientForCheckIn func(ctx context.Context) ( sessionProbeServicesClientForCheckIn, error) + + // torArgs contains the optional arguments for tor that we may need + // to pass to urlgetter when it uses a tor tunnel. + torArgs []string + + // torBinary contains the optional path to the tor binary that we + // may need to pass to urlgetter when it uses a tor tunnel. + torBinary string + + // tunnel is the optional tunnel that we may be using. It is created + // by NewSession and it is cleaned up by Close. + tunnel tunnel.Tunnel } // sessionProbeServicesClientForCheckIn returns the probe services @@ -100,8 +106,32 @@ type sessionProbeServicesClientForCheckIn interface { CheckIn(ctx context.Context, config model.CheckInConfig) (*model.CheckInInfo, error) } -// NewSession creates a new session or returns an error -func NewSession(config SessionConfig) (*Session, error) { +// NewSession creates a new session. This factory function will +// execute the following steps: +// +// 1. Make sure the config is sane, apply reasonable defaults +// where possible, otherwise return an error. +// +// 2. Create a temporary directory. +// +// 3. Create an instance of the session. +// +// 4. If the user requested for a proxy that entails a tunnel (at the +// moment of writing this note, either psiphon or tor), then start the +// requested tunnel and configure it as our proxy. +// +// 5. Create a compound resolver for the session that will attempt +// to use a bunch of DoT/DoH servers before falling back to the system +// resolver if nothing else works (see the sessionresolver pkg). This +// sessionresolver will be using the configured proxy, if any. +// +// 6. Create the default HTTP transport that we should be using when +// we communicate with the OONI backends. This transport will be +// using the configured proxy, if any. +// +// If any of these steps fails, then we cannot create a measurement +// session and we return an error. +func NewSession(ctx context.Context, config SessionConfig) (*Session, error) { if config.Logger == nil { return nil, errors.New("Logger is empty") } @@ -127,26 +157,43 @@ func NewSession(config SessionConfig) (*Session, error) { byteCounter: bytecounter.New(), kvStore: config.KVStore, logger: config.Logger, - proxyURL: config.ProxyURL, queryProbeServicesCount: atomicx.NewInt64(), softwareName: config.SoftwareName, softwareVersion: config.SoftwareVersion, tempDir: tempDir, torArgs: config.TorArgs, torBinary: config.TorBinary, - tunnelDir: config.TunnelDir, } + proxyURL := config.ProxyURL + if proxyURL != nil { + switch proxyURL.Scheme { + case "psiphon", "tor": + tunnel, err := tunnel.Start(ctx, &tunnel.Config{ + Name: proxyURL.Scheme, + Session: &sessionTunnelEarlySession{}, + TorArgs: config.TorArgs, + TorBinary: config.TorBinary, + TunnelDir: config.TunnelDir, + }) + if err != nil { + return nil, err + } + sess.tunnel = tunnel + proxyURL = tunnel.SOCKS5ProxyURL() + } + } + sess.proxyURL = proxyURL httpConfig := netx.Config{ ByteCounter: sess.byteCounter, BogonIsError: true, Logger: sess.logger, - ProxyURL: config.ProxyURL, + ProxyURL: proxyURL, } sess.resolver = &sessionresolver.Resolver{ ByteCounter: sess.byteCounter, KVStore: config.KVStore, Logger: sess.logger, - ProxyURL: config.ProxyURL, + ProxyURL: proxyURL, } httpConfig.FullResolver = sess.resolver sess.httpDefaultTransport = netx.NewHTTPTransport(httpConfig) @@ -330,63 +377,6 @@ var ErrAlreadyUsingProxy = errors.New( "session: cannot create a new tunnel of this kind: we are already using a proxy", ) -// MaybeStartTunnel starts the requested tunnel. -// -// This function silently succeeds if we're already using a tunnel with -// the same name or if the requested tunnel name is the empty string. This -// function fails, tho, when we already have a proxy or a tunnel with -// another name and we try to open a tunnel. This function of course also -// fails if we cannot start the requested tunnel. All in all, if you request -// for a tunnel name that is not the empty string and you get a nil error, -// you can be confident that session.ProxyURL() gives you the tunnel URL. -// -// The tunnel will be closed by session.Close(). -func (s *Session) MaybeStartTunnel(ctx context.Context, name string) error { - // TODO(bassosimone): see if we can unify tunnelMu and mu. - s.tunnelMu.Lock() - defer s.tunnelMu.Unlock() - if name == "" { - // There is no point in continuing if we know - // we don't need to do anything. - return nil - } - if s.tunnel != nil && s.tunnelName == name { - // We've been asked more than once to start the same tunnel. - return nil - } - if s.proxyURL != nil && name == "" { - // The user configured a proxy and here we're not actually trying - // to start any tunnel since `name` is empty. - // TODO(bassosimone): this if branch is probably useless now - // because we stop above when name is "". - return nil - } - if s.proxyURL != nil || s.tunnel != nil { - // We already have a proxy or we have a different tunnel. Because a tunnel - // sets a proxy, the second check for s.tunnel is for robustness. - return ErrAlreadyUsingProxy - } - s.logger.Infof("starting '%s' tunnel; please be patient...", name) - tunnel, err := tunnel.Start(ctx, &tunnel.Config{ - Name: name, - Session: &sessionTunnelEarlySession{}, - TorArgs: s.TorArgs(), - TorBinary: s.TorBinary(), - TunnelDir: s.tunnelDir, - }) - if err != nil { - return err - } - // Implementation note: tunnel _may_ be NIL here if name is "" - if tunnel == nil { - return nil - } - s.tunnelName = name - s.tunnel = tunnel - s.proxyURL = tunnel.SOCKS5ProxyURL() - return nil -} - // NewExperimentBuilder returns a new experiment builder // for the experiment with the given name, or an error if // there's no such experiment with the given name diff --git a/internal/engine/session_integration_test.go b/internal/engine/session_integration_test.go index e7954b4..f6a86c8 100644 --- a/internal/engine/session_integration_test.go +++ b/internal/engine/session_integration_test.go @@ -75,7 +75,7 @@ func TestNewSessionBuilderGood(t *testing.T) { } func newSessionMustFail(t *testing.T, config SessionConfig) { - sess, err := NewSession(config) + sess, err := NewSession(context.Background(), config) if err == nil { t.Fatal("expected an error here") } @@ -88,7 +88,7 @@ func TestSessionTorArgsTorBinary(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") } - sess, err := NewSession(SessionConfig{ + sess, err := NewSession(context.Background(), SessionConfig{ AvailableProbeServices: []model.Service{{ Address: "https://ams-pg-test.ooni.org", Type: "https", @@ -120,7 +120,7 @@ func TestSessionTorArgsTorBinary(t *testing.T) { } func newSessionForTestingNoLookupsWithProxyURL(t *testing.T, URL *url.URL) *Session { - sess, err := NewSession(SessionConfig{ + sess, err := NewSession(context.Background(), SessionConfig{ AvailableProbeServices: []model.Service{{ Address: "https://ams-pg-test.ooni.org", Type: "https", @@ -336,7 +336,7 @@ func TestGetAvailableProbeServices(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") } - sess, err := NewSession(SessionConfig{ + sess, err := NewSession(context.Background(), SessionConfig{ Logger: model.DiscardLogger, SoftwareName: "ooniprobe-engine", SoftwareVersion: "0.0.1", @@ -356,7 +356,7 @@ func TestMaybeLookupBackendsFailure(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") } - sess, err := NewSession(SessionConfig{ + sess, err := NewSession(context.Background(), SessionConfig{ Logger: model.DiscardLogger, SoftwareName: "ooniprobe-engine", SoftwareVersion: "0.0.1", @@ -377,7 +377,7 @@ func TestMaybeLookupTestHelpersIdempotent(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") } - sess, err := NewSession(SessionConfig{ + sess, err := NewSession(context.Background(), SessionConfig{ Logger: model.DiscardLogger, SoftwareName: "ooniprobe-engine", SoftwareVersion: "0.0.1", @@ -402,7 +402,7 @@ func TestAllProbeServicesUnsupported(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") } - sess, err := NewSession(SessionConfig{ + sess, err := NewSession(context.Background(), SessionConfig{ Logger: model.DiscardLogger, SoftwareName: "ooniprobe-engine", SoftwareVersion: "0.0.1", @@ -421,127 +421,8 @@ func TestAllProbeServicesUnsupported(t *testing.T) { } } -func TestStartTunnelGood(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - sess := newSessionForTestingNoLookups(t) - defer sess.Close() - ctx := context.Background() - if err := sess.MaybeStartTunnel(ctx, "psiphon"); err != nil { - t.Fatal(err) - } - if err := sess.MaybeStartTunnel(ctx, "psiphon"); err != nil { - t.Fatal(err) // check twice, must be idempotent - } - if sess.ProxyURL() == nil { - t.Fatal("expected non-nil ProxyURL") - } -} - -func TestStartTunnelNonexistent(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - sess := newSessionForTestingNoLookups(t) - defer sess.Close() - ctx := context.Background() - if err := sess.MaybeStartTunnel(ctx, "antani"); err.Error() != "unsupported tunnel" { - t.Fatal("not the error we expected") - } - if sess.ProxyURL() != nil { - t.Fatal("expected nil ProxyURL") - } -} - -func TestStartTunnelEmptyString(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - sess := newSessionForTestingNoLookups(t) - defer sess.Close() - ctx := context.Background() - if sess.MaybeStartTunnel(ctx, "") != nil { - t.Fatal("expected no error here") - } - if sess.ProxyURL() != nil { - t.Fatal("expected nil ProxyURL") - } -} - -func TestStartTunnelEmptyStringWithProxy(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - proxyURL := &url.URL{Scheme: "socks5", Host: "127.0.0.1:9050"} - sess := newSessionForTestingNoLookups(t) - sess.proxyURL = proxyURL - defer sess.Close() - ctx := context.Background() - if sess.MaybeStartTunnel(ctx, "") != nil { - t.Fatal("expected no error here") - } - diff := cmp.Diff(proxyURL, sess.ProxyURL()) - if diff != "" { - t.Fatal(diff) - } -} - -func TestStartTunnelWithAlreadyExistingTunnel(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - sess := newSessionForTestingNoLookups(t) - defer sess.Close() - ctx := context.Background() - if sess.MaybeStartTunnel(ctx, "psiphon") != nil { - t.Fatal("expected no error here") - } - prev := sess.ProxyURL() - err := sess.MaybeStartTunnel(ctx, "tor") - if !errors.Is(err, ErrAlreadyUsingProxy) { - t.Fatal("expected another error here") - } - cur := sess.ProxyURL() - diff := cmp.Diff(prev, cur) - if diff != "" { - t.Fatal(diff) - } -} - -func TestStartTunnelWithAlreadyExistingProxy(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - sess := newSessionForTestingNoLookups(t) - defer sess.Close() - ctx := context.Background() - orig := &url.URL{Scheme: "socks5", Host: "[::1]:9050"} - sess.proxyURL = orig - err := sess.MaybeStartTunnel(ctx, "psiphon") - if !errors.Is(err, ErrAlreadyUsingProxy) { - t.Fatal("expected another error here") - } - cur := sess.ProxyURL() - diff := cmp.Diff(orig, cur) - if diff != "" { - t.Fatal(diff) - } -} - -func TestStartTunnelCanceledContext(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - sess := newSessionForTestingNoLookups(t) - defer sess.Close() - ctx, cancel := context.WithCancel(context.Background()) - cancel() // immediately cancel - err := sess.MaybeStartTunnel(ctx, "psiphon") - if !errors.Is(err, context.Canceled) { - t.Fatal("not the error we expected") - } -} +// TODO(bassosimone): we should write unit/integration tests +// for the new way in which tunnels work. func TestUserAgentNoProxy(t *testing.T) { if testing.Short() { diff --git a/internal/engine/tunnel/integration_test.go b/internal/engine/tunnel/integration_test.go index b0de4ec..8f0da10 100644 --- a/internal/engine/tunnel/integration_test.go +++ b/internal/engine/tunnel/integration_test.go @@ -15,7 +15,7 @@ func TestPsiphonStartWithCancelledContext(t *testing.T) { // can move it inside of the internal tests. ctx, cancel := context.WithCancel(context.Background()) cancel() // fail immediately - sess, err := engine.NewSession(engine.SessionConfig{ + sess, err := engine.NewSession(ctx, engine.SessionConfig{ Logger: log.Log, SoftwareName: "miniooni", SoftwareVersion: "0.1.0-dev", @@ -41,7 +41,8 @@ func TestPsiphonStartStop(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") } - sess, err := engine.NewSession(engine.SessionConfig{ + ctx := context.Background() + sess, err := engine.NewSession(ctx, engine.SessionConfig{ Logger: log.Log, SoftwareName: "ooniprobe-engine", SoftwareVersion: "0.0.1", diff --git a/pkg/oonimkall/internal/tasks/runner.go b/pkg/oonimkall/internal/tasks/runner.go index afeacab..eb2a352 100644 --- a/pkg/oonimkall/internal/tasks/runner.go +++ b/pkg/oonimkall/internal/tasks/runner.go @@ -69,7 +69,7 @@ func (r *Runner) hasUnsupportedSettings(logger *ChanLogger) bool { return false } -func (r *Runner) newsession(logger *ChanLogger) (*engine.Session, error) { +func (r *Runner) newsession(ctx context.Context, logger *ChanLogger) (*engine.Session, error) { kvstore, err := engine.NewFileSystemKVStore(r.settings.StateDir) if err != nil { return nil, err @@ -88,7 +88,7 @@ func (r *Runner) newsession(logger *ChanLogger) (*engine.Session, error) { Address: r.settings.Options.ProbeServicesBaseURL, }} } - return engine.NewSession(config) + return engine.NewSession(ctx, config) } func (r *Runner) contextForExperiment( @@ -121,7 +121,7 @@ func (r *Runner) Run(ctx context.Context) { return } r.emitter.Emit(statusStarted, eventEmpty{}) - sess, err := r.newsession(logger) + sess, err := r.newsession(ctx, logger) if err != nil { r.emitter.EmitFailureStartup(err.Error()) return diff --git a/pkg/oonimkall/session.go b/pkg/oonimkall/session.go index 3b73b21..ea2c99b 100644 --- a/pkg/oonimkall/session.go +++ b/pkg/oonimkall/session.go @@ -116,11 +116,23 @@ type Session struct { sessp *engine.Session } -// NewSession creates a new session. You should use a session for running +// NewSession is like NewSessionWithContext but without context. This +// factory is deprecated and will be removed when we bump the major +// version number of ooni/probe-cli. +func NewSession(config *SessionConfig) (*Session, error) { + return newSessionWithContext(context.Background(), config) +} + +// NewSessionWithContext creates a new session. You should use a session for running // a set of operations in a relatively short time frame. You SHOULD NOT create // a single session and keep it all alive for the whole app lifecyle, since // the Session code is not specifically designed for this use case. -func NewSession(config *SessionConfig) (*Session, error) { +func NewSessionWithContext(ctx *Context, config *SessionConfig) (*Session, error) { + return newSessionWithContext(ctx.ctx, config) +} + +// newSessionWithContext implements NewSessionWithContext. +func newSessionWithContext(ctx context.Context, config *SessionConfig) (*Session, error) { kvstore, err := engine.NewFileSystemKVStore(config.StateDir) if err != nil { return nil, err @@ -150,7 +162,7 @@ func NewSession(config *SessionConfig) (*Session, error) { TempDir: config.TempDir, TunnelDir: config.TunnelDir, } - sessp, err := engine.NewSession(engineConfig) + sessp, err := engine.NewSession(ctx, engineConfig) if err != nil { return nil, err }