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
https://github.com/ooni/probe/issues/1814 and has been
extracted from https://github.com/ooni/probe-cli/pull/539.

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

internal/cmd/miniooni/libminiooni.go

@@ -416,10 +416,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: InputStrictlyRequired,
+			inputPolicy: InputOrStaticDefault,
 		}
 	},
 

internal/engine/experimentbuilder.go

@@ -36,6 +36,11 @@ const (
 	// InputNone indicates that the experiment does not want any
 	// input and ignores the input if provided with it.
 	InputNone = InputPolicy("none")
+
+	// We gather input from StaticInput and SourceFiles. If there is
+	// input, we return it. Otherwise, we return an internal static
+	// list of inputs to be used with this experiment.
+	InputOrStaticDefault = InputPolicy("or_static_default")
 )
 
 // ExperimentBuilder is an experiment builder.
@@ -183,10 +188,18 @@ func (b *ExperimentBuilder) NewExperiment() *Experiment {
 
 // canonicalizeExperimentName allows code to provide experiment names
 // in a more flexible way, where we have aliases.
+//
+// Because we allow for uppercase experiment names for backwards
+// compatibility with MK, we need to add some exceptions here when
+// mapping (e.g., DNSCheck => dnscheck).
 func canonicalizeExperimentName(name string) string {
 	switch name = strcase.ToSnake(name); name {
 	case "ndt_7":
 		name = "ndt" // since 2020-03-18, we use ndt7 to implement ndt by default
+	case "dns_check":
+		name = "dnscheck"
+	case "stun_reachability":
+		name = "stunreachability"
 	default:
 	}
 	return name

internal/engine/inputloader.go

@@ -6,10 +6,12 @@ import (
 	"errors"
 	"fmt"
 	"io/fs"
+	"net/url"
 
 	"github.com/apex/log"
 	"github.com/ooni/probe-cli/v3/internal/engine/model"
 	"github.com/ooni/probe-cli/v3/internal/fsx"
+	"github.com/ooni/probe-cli/v3/internal/stuninput"
 )
 
 // These errors are returned by the InputLoader.
@@ -18,6 +20,7 @@ var (
 	ErrDetectedEmptyFile = errors.New("file did not contain any input")
 	ErrInputRequired     = errors.New("no input provided")
 	ErrNoInputExpected   = errors.New("we did not expect any input")
+	ErrNoStaticInput     = errors.New("no static input for this experiment")
 )
 
 // InputLoaderSession is the session according to an InputLoader. We
@@ -58,6 +61,12 @@ type InputLoaderLogger interface {
 // input, we return it. Otherwise, we use OONI's probe services
 // to gather input using the best API for the task.
 //
+// InputOrStaticDefault
+//
+// We gather input from StaticInput and SourceFiles. If there is
+// input, we return it. Otherwise, we return an internal static
+// list of inputs to be used with this experiment.
+//
 // InputStrictlyRequired
 //
 // We gather input from StaticInput and SourceFiles. If there is
@@ -69,6 +78,10 @@ type InputLoader struct {
 	// will set them to a default value.
 	CheckInConfig *model.CheckInConfig
 
+	// ExperimentName is the name of the experiment. This field
+	// is only used together with the InputOrStaticDefault policy.
+	ExperimentName string
+
 	// InputPolicy specifies the input policy for the
 	// current experiment. We will not load any input if
 	// the policy says we should not. You MUST fill in
@@ -105,6 +118,8 @@ func (il *InputLoader) Load(ctx context.Context) ([]model.URLInfo, error) {
 		return il.loadOrQueryBackend(ctx)
 	case InputStrictlyRequired:
 		return il.loadStrictlyRequired(ctx)
+	case InputOrStaticDefault:
+		return il.loadOrStaticDefault(ctx)
 	default:
 		return il.loadNone()
 	}
@@ -147,6 +162,59 @@ func (il *InputLoader) loadOrQueryBackend(ctx context.Context) ([]model.URLInfo,
 	return il.loadRemote(ctx)
 }
 
+// TODO(https://github.com/ooni/probe/issues/1390): we need to
+// implement serving DNSCheck targets from the API
+var dnsCheckDefaultInput = []string{
+	"https://dns.google/dns-query",
+	"https://8.8.8.8/dns-query",
+	"dot://8.8.8.8:853/",
+	"dot://8.8.4.4:853/",
+	"https://8.8.4.4/dns-query",
+	"https://cloudflare-dns.com/dns-query",
+	"https://1.1.1.1/dns-query",
+	"https://1.0.0.1/dns-query",
+	"dot://1.1.1.1:853/",
+	"dot://1.0.0.1:853/",
+	"https://dns.quad9.net/dns-query",
+	"https://9.9.9.9/dns-query",
+	"dot://9.9.9.9:853/",
+	"dot://dns.quad9.net/",
+}
+
+var stunReachabilityDefaultInput = stuninput.AsnStunReachabilityInput()
+
+// staticBareInputForExperiment returns the list of strings an
+// experiment should use as static input. In case there is no
+// static input for this experiment, we return an error.
+func staticBareInputForExperiment(name string) ([]string, error) {
+	// Implementation note: we may be called from pkg/oonimkall
+	// with a non-canonical experiment name, so we need to convert
+	// the experiment name to be canonical before proceeding.
+	switch canonicalizeExperimentName(name) {
+	case "dnscheck":
+		return dnsCheckDefaultInput, nil
+	case "stunreachability":
+		return stunReachabilityDefaultInput, nil
+	default:
+		return nil, ErrNoStaticInput
+	}
+}
+
+// staticInputForExperiment returns the static input for the given experiment
+// or an error if there's no static input for the experiment.
+func staticInputForExperiment(name string) ([]model.URLInfo, error) {
+	return stringListToModelURLInfo(staticBareInputForExperiment(name))
+}
+
+// loadOrStaticDefault implements the InputOrStaticDefault policy.
+func (il *InputLoader) loadOrStaticDefault(ctx context.Context) ([]model.URLInfo, error) {
+	inputs, err := il.loadLocal()
+	if err != nil || len(inputs) > 0 {
+		return inputs, err
+	}
+	return staticInputForExperiment(il.ExperimentName)
+}
+
 // loadLocal loads inputs from StaticInputs and SourceFiles.
 func (il *InputLoader) loadLocal() ([]model.URLInfo, error) {
 	inputs := []model.URLInfo{}
@@ -264,3 +332,26 @@ func (il *InputLoader) logger() InputLoaderLogger {
 	}
 	return log.Log
 }
+
+// stringListToModelURLInfo is an utility function to convert
+// a list of strings containing URLs into a list of model.URLInfo
+// which would have been returned by an hypothetical backend
+// API serving input for a test for which we don't have an API
+// yet (e.g., stunreachability and dnscheck).
+func stringListToModelURLInfo(input []string, err error) ([]model.URLInfo, error) {
+	if err != nil {
+		return nil, err
+	}
+	var output []model.URLInfo
+	for _, URL := range input {
+		if _, err := url.Parse(URL); err != nil {
+			return nil, err
+		}
+		output = append(output, model.URLInfo{
+			CategoryCode: "MISC", // hard to find a category
+			CountryCode:  "XX",   // representing no country
+			URL:          URL,
+		})
+	}
+	return output, nil
+}

internal/engine/inputloader_test.go

@@ -6,6 +6,7 @@ import (
 	"io"
 	"io/fs"
 	"os"
+	"strings"
 	"syscall"
 	"testing"
 
@@ -206,6 +207,134 @@ func TestInputLoaderInputStrictlyRequiredWithEmptyFile(t *testing.T) {
 	}
 }
 
+func TestInputLoaderInputOrStaticDefaultWithInput(t *testing.T) {
+	il := &InputLoader{
+		ExperimentName: "dnscheck",
+		StaticInputs:   []string{"https://www.google.com/"},
+		SourceFiles: []string{
+			"testdata/inputloader1.txt",
+			"testdata/inputloader2.txt",
+		},
+		InputPolicy: InputOrStaticDefault,
+	}
+	ctx := context.Background()
+	out, err := il.Load(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(out) != 5 {
+		t.Fatal("not the output length we expected")
+	}
+	expect := []model.URLInfo{
+		{URL: "https://www.google.com/"},
+		{URL: "https://www.x.org/"},
+		{URL: "https://www.slashdot.org/"},
+		{URL: "https://abc.xyz/"},
+		{URL: "https://run.ooni.io/"},
+	}
+	if diff := cmp.Diff(out, expect); diff != "" {
+		t.Fatal(diff)
+	}
+}
+
+func TestInputLoaderInputOrStaticDefaultWithEmptyFile(t *testing.T) {
+	il := &InputLoader{
+		ExperimentName: "dnscheck",
+		InputPolicy:    InputOrStaticDefault,
+		SourceFiles: []string{
+			"testdata/inputloader1.txt",
+			"testdata/inputloader3.txt", // we want it before inputloader2.txt
+			"testdata/inputloader2.txt",
+		},
+	}
+	ctx := context.Background()
+	out, err := il.Load(ctx)
+	if !errors.Is(err, ErrDetectedEmptyFile) {
+		t.Fatalf("not the error we expected: %+v", err)
+	}
+	if out != nil {
+		t.Fatal("not the output we expected")
+	}
+}
+
+func TestInputLoaderInputOrStaticDefaultWithoutInputDNSCheck(t *testing.T) {
+	il := &InputLoader{
+		ExperimentName: "dnscheck",
+		InputPolicy:    InputOrStaticDefault,
+	}
+	ctx := context.Background()
+	out, err := il.Load(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(out) != len(dnsCheckDefaultInput) {
+		t.Fatal("invalid output length")
+	}
+	for idx := 0; idx < len(dnsCheckDefaultInput); idx++ {
+		e := out[idx]
+		if e.CategoryCode != "MISC" {
+			t.Fatal("invalid category code")
+		}
+		if e.CountryCode != "XX" {
+			t.Fatal("invalid country code")
+		}
+		if e.URL != dnsCheckDefaultInput[idx] {
+			t.Fatal("invalid URL")
+		}
+	}
+}
+
+func TestInputLoaderInputOrStaticDefaultWithoutInputStunReachability(t *testing.T) {
+	il := &InputLoader{
+		ExperimentName: "stunreachability",
+		InputPolicy:    InputOrStaticDefault,
+	}
+	ctx := context.Background()
+	out, err := il.Load(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(out) != len(stunReachabilityDefaultInput) {
+		t.Fatal("invalid output length")
+	}
+	for idx := 0; idx < len(stunReachabilityDefaultInput); idx++ {
+		e := out[idx]
+		if e.CategoryCode != "MISC" {
+			t.Fatal("invalid category code")
+		}
+		if e.CountryCode != "XX" {
+			t.Fatal("invalid country code")
+		}
+		if e.URL != stunReachabilityDefaultInput[idx] {
+			t.Fatal("invalid URL")
+		}
+	}
+}
+
+func TestStaticBareInputForExperimentWorksWithNonCanonicalNames(t *testing.T) {
+	names := []string{"DNSCheck", "STUNReachability"}
+	for _, name := range names {
+		if _, err := staticInputForExperiment(name); err != nil {
+			t.Fatal("failure for", name, ":", err)
+		}
+	}
+}
+
+func TestInputLoaderInputOrStaticDefaultWithoutInputOtherName(t *testing.T) {
+	il := &InputLoader{
+		ExperimentName: "xx",
+		InputPolicy:    InputOrStaticDefault,
+	}
+	ctx := context.Background()
+	out, err := il.Load(ctx)
+	if !errors.Is(err, ErrNoStaticInput) {
+		t.Fatal("not the error we expected", err)
+	}
+	if out != nil {
+		t.Fatal("expected nil result here")
+	}
+}
+
 func TestInputLoaderInputOrQueryBackendWithInput(t *testing.T) {
 	il := &InputLoader{
 		StaticInputs: []string{"https://www.google.com/"},
@@ -494,3 +623,59 @@ func TestInputLoaderLoggerWorksAsIntended(t *testing.T) {
 		t.Fatal("logger not working as intended")
 	}
 }
+
+func TestStringListToModelURLInfoWithValidInput(t *testing.T) {
+	input := []string{
+		"stun://stun.voip.blackberry.com:3478",
+		"stun://stun.altar.com.pl:3478",
+	}
+	output, err := stringListToModelURLInfo(input, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(input) != len(output) {
+		t.Fatal("unexpected output length")
+	}
+	for idx := 0; idx < len(input); idx++ {
+		if input[idx] != output[idx].URL {
+			t.Fatal("unexpected entry")
+		}
+		if output[idx].CategoryCode != "MISC" {
+			t.Fatal("unexpected category")
+		}
+		if output[idx].CountryCode != "XX" {
+			t.Fatal("unexpected country")
+		}
+	}
+}
+
+func TestStringListToModelURLInfoWithInvalidInput(t *testing.T) {
+	input := []string{
+		"stun://stun.voip.blackberry.com:3478",
+		"\t", // <- not a valid URL
+		"stun://stun.altar.com.pl:3478",
+	}
+	output, err := stringListToModelURLInfo(input, nil)
+	if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") {
+		t.Fatal("no the error we expected", err)
+	}
+	if output != nil {
+		t.Fatal("unexpected nil output")
+	}
+}
+
+func TestStringListToModelURLInfoWithError(t *testing.T) {
+	input := []string{
+		"stun://stun.voip.blackberry.com:3478",
+		"\t",
+		"stun://stun.altar.com.pl:3478",
+	}
+	expected := errors.New("mocked error")
+	output, err := stringListToModelURLInfo(input, expected)
+	if !errors.Is(err, expected) {
+		t.Fatal("no the error we expected", err)
+	}
+	if output != nil {
+		t.Fatal("unexpected nil output")
+	}
+}