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 }