Backport patches mentioned by ooni/probe#1912 (#635)

* [backport] refactor(stunreachability): input required and must be an URL (#630)

Here we're refactoring stunreachability to not provide internally a
default input and to take in input an URL rather than a string.

The related ooni/spec change is ooni/spec#227.

This diff has been extracted from #539.

Because the original diff was large, I'm splitting it in a set of
more easily manageable diffs.

The reference issue is ooni/probe#1814, which
is complex enough to require us to proceed incrementally.

This diff WILL need to be backported to release/3.11.

* [backport] refactor: create common package for holding STUN input (#631)

We want stunreachability to use the same STUN servers used by
snowflake, so let's start by making a common package holding the
servers. Let's also use this new package in Snowflake.

We're currently not using this package in stunreachability, but
I am going to apply this as a subsequent diff.

Reference issue: ooni/probe#1814. This
issue is a bit complex to address in a single PR, so we are going
to proceed incremntally.

This diff was extracted from #539.

* [backport] refactor: introduce and use InputOrStaticDefault (#632)

This commit introduces a new `InputLoader` policy by which, if no
input is provided, we use a static default input list.

We also modify the code to use this policy for dnscheck and
stunreachability, with proper input.

We also modify `miniooni` to pass the new `ExperimentName` field to
the `InputLoader` to indicate which default input list to use.

This diff is part of a set of diffs aiming at fixing
ooni/probe#1814 and has been
extracted from #539.

What remains to be done, after this diff has landed is to ensure
things also work for ooniprobe and oonimkall.

* [backport] fix(ooniprobe): dnscheck,stunreachability run w/ default input (#633)

This diff is part of ooni/probe#1814 and
teaches `ooniprobe` to run dnscheck and stunreachability by using the
default static input feature of the `InputLoader`.

I've manually tested that we can still run `websites` like
we did before (including category filtering).

I've also manually tested that now we can run `experimental` and
get parseable results for dnscheck and stunreachability.

With this diff in, we have fixed the original problem highlighted in
the ooni/probe#1814 issue.

Yet, because of the way in which I solved the problem, there is
more work to do. My changes have broken stunreachability for
mobile and now it's time I apply fixes to make it work again.

This diff was extracted from #539,
which at this point only basically contains the remaining fixes to
ensure we can run stunreachability on mobile.

Co-authored-by: Arturo Filastò <arturo@filasto.net>

Co-authored-by: Arturo Filastò <arturo@filasto.net>

* [backport] fix(oonimkall): run tests with InputOrStaticDefault policy (#634)

Previous work to make ooni/probe#1814
possible has broken running stunreachability on mobile.

This diff repairs the blunder and allows to run any experiment
using InputOrStaticDefault with oonimkall.

Diff extracted from #539.

Co-authored-by: Arturo Filastò <arturo@filasto.net>

cmd/ooniprobe/internal/nettests/dnscheck.go

@@ -1,60 +1,42 @@
 package nettests
 
 import (
-	"encoding/json"
+	"context"
 
-	"github.com/ooni/probe-cli/v3/internal/engine/experiment/dnscheck"
-	"github.com/ooni/probe-cli/v3/internal/engine/experiment/run"
-	"github.com/ooni/probe-cli/v3/internal/runtimex"
+	engine "github.com/ooni/probe-cli/v3/internal/engine"
+	"github.com/ooni/probe-cli/v3/internal/engine/model"
 )
 
 // DNSCheck nettest implementation.
 type DNSCheck struct{}
 
-var dnsCheckDefaultInput []string
-
-func dnsCheckMustMakeInput(input *run.StructuredInput) string {
-	data, err := json.Marshal(input)
-	runtimex.PanicOnError(err, "json.Marshal failed")
-	return string(data)
-}
-
-func init() {
-	// The following code just adds a minimal set of URLs to
-	// test using DNSCheck, so we start exposing it.
-	//
-	// TODO(bassosimone):
-	//
-	// 1. we should be getting input from the backend instead of
-	// having an hardcoded list of inputs here.
-	//
-	// 2. we should modify dnscheck to accept http3://... as a
-	// shortcut for https://... with h3. If we don't do that, we
-	// are stuck with the h3 results hiding h2 results in OONI
-	// Explorer because they use the same URL.
-	//
-	// 3. it seems we have the problem that dnscheck results
-	// appear as the `run` nettest in `ooniprobe list <ID>` because
-	// dnscheck is run using the `run` functionality.
-	dnsCheckDefaultInput = append(dnsCheckDefaultInput, dnsCheckMustMakeInput(
-		&run.StructuredInput{
-			DNSCheck: dnscheck.Config{},
-			Name:     "dnscheck",
-			Input:    "https://dns.google/dns-query",
-		}))
-	dnsCheckDefaultInput = append(dnsCheckDefaultInput, dnsCheckMustMakeInput(
-		&run.StructuredInput{
-			DNSCheck: dnscheck.Config{},
-			Name:     "dnscheck",
-			Input:    "https://cloudflare-dns.com/dns-query",
-		}))
+func (n DNSCheck) lookupURLs(ctl *Controller) ([]string, error) {
+	inputloader := &engine.InputLoader{
+		CheckInConfig: &model.CheckInConfig{
+			// not needed because we have default static input in the engine
+		},
+		ExperimentName: "dnscheck",
+		InputPolicy:    engine.InputOrStaticDefault,
+		Session:        ctl.Session,
+		SourceFiles:    ctl.InputFiles,
+		StaticInputs:   ctl.Inputs,
+	}
+	testlist, err := inputloader.Load(context.Background())
+	if err != nil {
+		return nil, err
+	}
+	return ctl.BuildAndSetInputIdxMap(ctl.Probe.DB(), testlist)
 }
 
 // Run starts the nettest.
 func (n DNSCheck) Run(ctl *Controller) error {
-	builder, err := ctl.Session.NewExperimentBuilder("run")
+	builder, err := ctl.Session.NewExperimentBuilder("dnscheck")
+	if err != nil {
+		return err
+	}
+	urls, err := n.lookupURLs(ctl)
 	if err != nil {
 		return err
 	}
-	return ctl.Run(builder, dnsCheckDefaultInput)
+	return ctl.Run(builder, urls)
 }

cmd/ooniprobe/internal/nettests/nettests.go

@@ -13,6 +13,7 @@ import (
 	engine "github.com/ooni/probe-cli/v3/internal/engine"
 	"github.com/ooni/probe-cli/v3/internal/engine/model"
 	"github.com/pkg/errors"
+	"upper.io/db.v3/lib/sqlbuilder"
 )
 
 // Nettest interface. Every Nettest should implement this.
@@ -64,12 +65,49 @@ type Controller struct {
 	curInputIdx int
 }
 
-// SetInputIdxMap is used to set the mapping of index into input. This mapping
-// is used to reference, for example, a particular URL based on the index inside
-// of the input list and the index of it in the database.
-func (c *Controller) SetInputIdxMap(inputIdxMap map[int64]int64) error {
-	c.inputIdxMap = inputIdxMap
-	return nil
+// BuildAndSetInputIdxMap takes in input a list of URLs in the format
+// returned by the check-in API (i.e., model.URLInfo) and performs
+// the following actions:
+//
+// 1. inserts each URL into the database;
+//
+// 2. builds a list of bare URLs to be tested;
+//
+// 3. registers a mapping between each URL and an index
+// and stores it into the controller.
+//
+// Arguments:
+//
+// - db is the database in which to register the URL;
+//
+// - testlist is the result from the check-in API (or possibly
+// a manually constructed list when applicable, e.g., for dnscheck
+// until we have an API for serving its input).
+//
+// Results:
+//
+// - on success, a list of strings containing URLs to test;
+//
+// - on failure, an error.
+func (c *Controller) BuildAndSetInputIdxMap(
+	db sqlbuilder.Database, testlist []model.URLInfo) ([]string, error) {
+	var urls []string
+	urlIDMap := make(map[int64]int64)
+	for idx, url := range testlist {
+		log.Debugf("Going over URL %d", idx)
+		urlID, err := database.CreateOrUpdateURL(
+			db, url.URL, url.CategoryCode, url.CountryCode,
+		)
+		if err != nil {
+			log.Error("failed to add to the URL table")
+			return nil, err
+		}
+		log.Debugf("Mapped URL %s to idx %d and urlID %d", url.URL, idx, urlID)
+		urlIDMap[int64(idx)] = urlID
+		urls = append(urls, url.URL)
+	}
+	c.inputIdxMap = urlIDMap
+	return urls, nil
 }
 
 // SetNettestIndex is used to set the current nettest index and total nettest

cmd/ooniprobe/internal/nettests/stunreachability.go

@@ -1,13 +1,42 @@
 package nettests
 
+import (
+	"context"
+
+	engine "github.com/ooni/probe-cli/v3/internal/engine"
+	"github.com/ooni/probe-cli/v3/internal/engine/model"
+)
+
 // STUNReachability nettest implementation.
 type STUNReachability struct{}
 
+func (n STUNReachability) lookupURLs(ctl *Controller) ([]string, error) {
+	inputloader := &engine.InputLoader{
+		CheckInConfig: &model.CheckInConfig{
+			// not needed because we have default static input in the engine
+		},
+		ExperimentName: "stunreachability",
+		InputPolicy:    engine.InputOrStaticDefault,
+		Session:        ctl.Session,
+		SourceFiles:    ctl.InputFiles,
+		StaticInputs:   ctl.Inputs,
+	}
+	testlist, err := inputloader.Load(context.Background())
+	if err != nil {
+		return nil, err
+	}
+	return ctl.BuildAndSetInputIdxMap(ctl.Probe.DB(), testlist)
+}
+
 // Run starts the nettest.
 func (n STUNReachability) Run(ctl *Controller) error {
 	builder, err := ctl.Session.NewExperimentBuilder("stunreachability")
 	if err != nil {
 		return err
 	}
-	return ctl.Run(builder, []string{""})
+	urls, err := n.lookupURLs(ctl)
+	if err != nil {
+		return err
+	}
+	return ctl.Run(builder, urls)
 }

cmd/ooniprobe/internal/nettests/web_connectivity.go

@@ -4,12 +4,11 @@ import (
 	"context"
 
 	"github.com/apex/log"
-	"github.com/ooni/probe-cli/v3/cmd/ooniprobe/internal/database"
 	engine "github.com/ooni/probe-cli/v3/internal/engine"
 	"github.com/ooni/probe-cli/v3/internal/engine/model"
 )
 
-func lookupURLs(ctl *Controller, categories []string) ([]string, map[int64]int64, error) {
+func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]string, error) {
 	inputloader := &engine.InputLoader{
 		CheckInConfig: &model.CheckInConfig{
 			// Setting Charging and OnWiFi to true causes the CheckIn
@@ -22,31 +21,17 @@ func lookupURLs(ctl *Controller, categories []string) ([]string, map[int64]int64
 				CategoryCodes: categories,
 			},
 		},
-		InputPolicy:  engine.InputOrQueryBackend,
-		Session:      ctl.Session,
-		SourceFiles:  ctl.InputFiles,
-		StaticInputs: ctl.Inputs,
+		ExperimentName: "web_connectivity",
+		InputPolicy:    engine.InputOrQueryBackend,
+		Session:        ctl.Session,
+		SourceFiles:    ctl.InputFiles,
+		StaticInputs:   ctl.Inputs,
 	}
 	testlist, err := inputloader.Load(context.Background())
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
-	var urls []string
-	urlIDMap := make(map[int64]int64)
-	for idx, url := range testlist {
-		log.Debugf("Going over URL %d", idx)
-		urlID, err := database.CreateOrUpdateURL(
-			ctl.Probe.DB(), url.URL, url.CategoryCode, url.CountryCode,
-		)
-		if err != nil {
-			log.Error("failed to add to the URL table")
-			return nil, nil, err
-		}
-		log.Debugf("Mapped URL %s to idx %d and urlID %d", url.URL, idx, urlID)
-		urlIDMap[int64(idx)] = urlID
-		urls = append(urls, url.URL)
-	}
-	return urls, urlIDMap, nil
+	return ctl.BuildAndSetInputIdxMap(ctl.Probe.DB(), testlist)
 }
 
 // WebConnectivity test implementation
@@ -55,14 +40,11 @@ type WebConnectivity struct{}
 // Run starts the test
 func (n WebConnectivity) Run(ctl *Controller) error {
 	log.Debugf("Enabled category codes are the following %v", ctl.Probe.Config().Nettests.WebsitesEnabledCategoryCodes)
-	urls, urlIDMap, err := lookupURLs(ctl, ctl.Probe.Config().Nettests.WebsitesEnabledCategoryCodes)
+	urls, err := n.lookupURLs(ctl, ctl.Probe.Config().Nettests.WebsitesEnabledCategoryCodes)
 	if err != nil {
 		return err
 	}
-	ctl.SetInputIdxMap(urlIDMap)
-	builder, err := ctl.Session.NewExperimentBuilder(
-		"web_connectivity",
-	)
+	builder, err := ctl.Session.NewExperimentBuilder("web_connectivity")
 	if err != nil {
 		return err
 	}

internal/cmd/miniooni/libminiooni.go

@@ -397,10 +397,11 @@ func MainWithConfiguration(experimentName string, currentOptions Options) {
 			OnWiFi:   true, // meaning: not on 4G
 			Charging: true,
 		},
-		InputPolicy:  builder.InputPolicy(),
-		StaticInputs: currentOptions.Inputs,
-		SourceFiles:  currentOptions.InputFilePaths,
-		Session:      sess,
+		ExperimentName: experimentName,
+		InputPolicy:    builder.InputPolicy(),
+		StaticInputs:   currentOptions.Inputs,
+		SourceFiles:    currentOptions.InputFilePaths,
+		Session:        sess,
 	}
 	inputs, err := inputLoader.Load(context.Background())
 	fatalOnError(err, "cannot load inputs")

internal/engine/allexperiments.go

@@ -49,7 +49,7 @@ var experimentsByName = map[string]func(*Session) *ExperimentBuilder{
 				))
 			},
 			config:      &dnscheck.Config{},
-			inputPolicy: InputStrictlyRequired,
+			inputPolicy: InputOrStaticDefault,
 		}
 	},
 
@@ -198,7 +198,7 @@ var experimentsByName = map[string]func(*Session) *ExperimentBuilder{
 				))
 			},
 			config:      &stunreachability.Config{},
-			inputPolicy: InputOptional,
+			inputPolicy: InputOrStaticDefault,
 		}
 	},
 

internal/engine/experiment/stunreachability/fake_test.go

@@ -6,6 +6,9 @@ import (
 	"time"
 )
 
+// TODO(bassosimone): we should use internal/netxlite/mocks rather
+// than rolling out a custom type private to this package.
+
 type FakeConn struct {
 	ReadError             error
 	ReadData              []byte

internal/engine/experiment/stunreachability/stunreachability.go

@@ -5,8 +5,10 @@ package stunreachability
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net"
+	"net/url"
 	"time"
 
 	"github.com/ooni/probe-cli/v3/internal/engine/legacy/errorsx"
@@ -20,7 +22,7 @@ import (
 
 const (
 	testName    = "stunreachability"
-	testVersion = "0.2.0"
+	testVersion = "0.3.0"
 )
 
 // Config contains the experiment config.
@@ -64,6 +66,15 @@ func wrap(err error) error {
 	}.MaybeBuild()
 }
 
+// errStunMissingInput means that the user did not provide any input
+var errStunMissingInput = errors.New("stun: missing input")
+
+// errStunMissingPortInURL means the URL is missing the port
+var errStunMissingPortInURL = errors.New("stun: missing port in URL")
+
+// errUnsupportedURLScheme means we don't support the URL scheme
+var errUnsupportedURLScheme = errors.New("stun: unsupported URL scheme")
+
 // Run implements ExperimentMeasurer.Run.
 func (m *Measurer) Run(
 	ctx context.Context, sess model.ExperimentSession,
@@ -72,7 +83,21 @@ func (m *Measurer) Run(
 	tk := new(TestKeys)
 	measurement.TestKeys = tk
 	registerExtensions(measurement)
-	if err := wrap(tk.run(ctx, m.config, sess, measurement, callbacks)); err != nil {
+	input := string(measurement.Input)
+	if input == "" {
+		return errStunMissingInput
+	}
+	URL, err := url.Parse(input)
+	if err != nil {
+		return err
+	}
+	if URL.Port() == "" {
+		return errStunMissingPortInURL
+	}
+	if URL.Scheme != "stun" {
+		return errUnsupportedURLScheme
+	}
+	if err := wrap(tk.run(ctx, m.config, sess, measurement, callbacks, URL.Host)); err != nil {
 		s := err.Error()
 		tk.Failure = &s
 		return err
@@ -83,12 +108,8 @@ func (m *Measurer) Run(
 func (tk *TestKeys) run(
 	ctx context.Context, config Config, sess model.ExperimentSession,
 	measurement *model.Measurement, callbacks model.ExperimentCallbacks,
+	endpoint string,
 ) error {
-	const defaultAddress = "stun.l.google.com:19302"
-	endpoint := string(measurement.Input)
-	if endpoint == "" {
-		endpoint = defaultAddress
-	}
 	callbacks.OnProgress(0, fmt.Sprintf("stunreachability: measuring: %s...", endpoint))
 	defer callbacks.OnProgress(
 		1, fmt.Sprintf("stunreachability: measuring: %s... done", endpoint))