[backport] feat: mvp of conditionally enabling experiments

This diff backports #1355 to the release/3.19 branch.

- [x] I have read the [contribution
guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md)
- [x] reference issue for this pull request:
ooni/probe#2553
- [x] if you changed anything related to how experiments work and you
need to reflect these changes in the ooni/spec repository, please link
to the related ooni/spec pull request: N/A
- [x] if you changed code inside an experiment, make sure you bump its
version number

We have a class of experiments that I call experimental experiments.
These are experiments that we just added, so we're not 100% sure about
them, or experiments that are historically flaky, so we want to be
defensive.

Currently, riseupvpn and echechk fall into this class.

The idea of this diff is to implement the functional specification
described by issue ooni/probe#2553.

While there, make sure we have more robust testing for the
`./internal/registry` package.

While there, make sure package creation logic lives in
`./internal/registry` rather than in `./internal/engine`.

internal/checkincache/checkincache.go

@@ -3,6 +3,7 @@ package checkincache
 
 import (
 	"encoding/json"
+	"fmt"
 	"time"
 
 	"github.com/ooni/probe-cli/v3/internal/model"
@@ -25,6 +26,9 @@ type checkInFlagsWrapper struct {
 }
 
 // Store stores the result of the latest check-in in the given key-value store.
+//
+// We store check-in feature flags in a file called checkinflags.state. These flags
+// are valid for 24 hours, after which we consider them stale.
 func Store(kvStore model.KeyValueStore, resp *model.OOAPICheckInResult) error {
 	// store the check-in flags in the key-value store
 	wrapper := &checkInFlagsWrapper{
@@ -52,3 +56,16 @@ func GetFeatureFlag(kvStore model.KeyValueStore, name string) bool {
 	}
 	return wrapper.Flags[name] // works even if map is nil
 }
+
+// ExperimentEnabledKey returns the [model.KeyValueStore] key to use to
+// know whether a disabled experiment has been enabled via check-in.
+func ExperimentEnabledKey(name string) string {
+	return fmt.Sprintf("%s_enabled", name)
+}
+
+// ExperimentEnabled returns whether a given experiment has been enabled by a previous
+// execution of check-in. Some experiments are disabled by default for different reasons
+// and we use the check-in API to control whether and when they should be enabled.
+func ExperimentEnabled(kvStore model.KeyValueStore, name string) bool {
+	return GetFeatureFlag(kvStore, ExperimentEnabledKey(name))
+}

internal/engine/experiment_integration_test.go

@@ -13,6 +13,7 @@ import (
 	"testing"
 
 	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/registry"
 )
 
 func TestCreateAll(t *testing.T) {
@@ -21,6 +22,12 @@ func TestCreateAll(t *testing.T) {
 	}
 	sess := newSessionForTesting(t)
 	defer sess.Close()
+
+	// Since https://github.com/ooni/probe-cli/pull/1355, some experiments are disabled
+	// by default and we need an environment variable to instantiate them
+	os.Setenv(registry.OONI_FORCE_ENABLE_EXPERIMENT, "1")
+	defer os.Unsetenv(registry.OONI_FORCE_ENABLE_EXPERIMENT)
+
 	for _, name := range AllExperiments() {
 		builder, err := sess.NewExperimentBuilder(name)
 		if err != nil {

internal/engine/experimentbuilder.go

@@ -62,7 +62,7 @@ func (b *experimentBuilder) NewExperiment() model.Experiment {
 
 // newExperimentBuilder creates a new experimentBuilder instance.
 func newExperimentBuilder(session *Session, name string) (*experimentBuilder, error) {
-	factory, err := registry.NewFactory(name)
+	factory, err := registry.NewFactory(name, session.kvStore, session.logger)
 	if err != nil {
 		return nil, err
 	}

internal/engine/inputloader.go

@@ -219,6 +219,12 @@ 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.
+	//
+	// TODO(https://github.com/ooni/probe/issues/1390): serve DNSCheck
+	// inputs using richer input (aka check-in v2).
+	//
+	// TODO(https://github.com/ooni/probe/issues/2557): server STUNReachability
+	// inputs using richer input (aka check-in v2).
 	switch registry.CanonicalizeExperimentName(name) {
 	case "dnscheck":
 		return dnsCheckDefaultInput, nil

internal/engine/session.go

@@ -11,15 +11,13 @@ import (
 	"sync/atomic"
 
 	"github.com/ooni/probe-cli/v3/internal/bytecounter"
-	"github.com/ooni/probe-cli/v3/internal/checkincache"
 	"github.com/ooni/probe-cli/v3/internal/enginelocate"
 	"github.com/ooni/probe-cli/v3/internal/enginenetx"
 	"github.com/ooni/probe-cli/v3/internal/engineresolver"
 	"github.com/ooni/probe-cli/v3/internal/kvstore"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/platform"
 	"github.com/ooni/probe-cli/v3/internal/probeservices"
-	"github.com/ooni/probe-cli/v3/internal/registry"
 	"github.com/ooni/probe-cli/v3/internal/runtimex"
 	"github.com/ooni/probe-cli/v3/internal/tunnel"
 	"github.com/ooni/probe-cli/v3/internal/version"
@@ -406,16 +404,6 @@ var ErrAlreadyUsingProxy = errors.New(
 // for the experiment with the given name, or an error if
 // there's no such experiment with the given name
 func (s *Session) NewExperimentBuilder(name string) (model.ExperimentBuilder, error) {
-	name = registry.CanonicalizeExperimentName(name)
-	switch {
-	case name == "web_connectivity" && checkincache.GetFeatureFlag(s.kvStore, "webconnectivity_0.5"):
-		// use LTE rather than the normal webconnectivity when the
-		// feature flag has been set through the check-in API
-		s.Logger().Infof("using webconnectivity LTE")
-		name = "web_connectivity@v0.5"
-	default:
-		// nothing
-	}
 	eb, err := newExperimentBuilder(s, name)
 	if err != nil {
 		return nil, err

internal/registry/allexperiments.go

@@ -1,5 +1,7 @@
 package registry
 
+import "sort"
+
 // Where we register all the available experiments.
 var AllExperiments = map[string]*Factory{}
 
@@ -8,5 +10,6 @@ func ExperimentNames() (names []string) {
 	for key := range AllExperiments {
 		names = append(names, key)
 	}
+	sort.Strings(names) // sort by name to always provide predictable output
 	return
 }

internal/registry/dash.go

@@ -16,8 +16,9 @@ func init() {
 				*config.(*dash.Config),
 			)
 		},
-		config:        &dash.Config{},
-		interruptible: true,
-		inputPolicy:   model.InputNone,
+		config:           &dash.Config{},
+		enabledByDefault: true,
+		interruptible:    true,
+		inputPolicy:      model.InputNone,
 	}
 }

internal/registry/dnscheck.go

@@ -16,7 +16,8 @@ func init() {
 				*config.(*dnscheck.Config),
 			)
 		},
-		config:      &dnscheck.Config{},
-		inputPolicy: model.InputOrStaticDefault,
+		config:           &dnscheck.Config{},
+		enabledByDefault: true,
+		inputPolicy:      model.InputOrStaticDefault,
 	}
 }

internal/registry/dnsping.go

@@ -16,7 +16,8 @@ func init() {
 				*config.(*dnsping.Config),
 			)
 		},
-		config:      &dnsping.Config{},
-		inputPolicy: model.InputOrStaticDefault,
+		config:           &dnsping.Config{},
+		enabledByDefault: true,
+		inputPolicy:      model.InputOrStaticDefault,
 	}
 }

internal/registry/example.go

@@ -22,7 +22,8 @@ func init() {
 			Message:   "Good day from the example experiment!",
 			SleepTime: int64(time.Second),
 		},
-		interruptible: true,
-		inputPolicy:   model.InputNone,
+		enabledByDefault: true,
+		interruptible:    true,
+		inputPolicy:      model.InputNone,
 	}
 }

internal/registry/factory.go

@@ -7,9 +7,11 @@ package registry
 import (
 	"errors"
 	"fmt"
+	"os"
 	"reflect"
 	"strconv"
 
+	"github.com/ooni/probe-cli/v3/internal/checkincache"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/strcasex"
 )
@@ -22,6 +24,9 @@ type Factory struct {
 	// config contains the experiment's config.
 	config any
 
+	// enabledByDefault indicates whether this experiment is enabled by default.
+	enabledByDefault bool
+
 	// inputPolicy contains the experiment's InputPolicy.
 	inputPolicy model.InputPolicy
 
@@ -218,12 +223,65 @@ func CanonicalizeExperimentName(name string) string {
 // ErrNoSuchExperiment indicates a given experiment does not exist.
 var ErrNoSuchExperiment = errors.New("no such experiment")
 
+// ErrRequiresForceEnable is returned for experiments that are not enabled by default and are also
+// not enabled by the most recent check-in API call.
+var ErrRequiresForceEnable = errors.New("experiment not enabled by check-in API")
+
+const experimentDisabledByCheckInWarning = `experiment '%s' is not enabled by default and the
+most recent check-in API call did not enable this experiment as well. You can bypass this restriction
+by setting the OONI_FORCE_ENABLE_EXPERIMENT environment variable to the string "1". On Unix like
+systems, you can use 'export OONI_FORCE_ENABLE_EXPERIMENT=1' to set this environment variable.`
+
+// OONI_FORCE_ENABLE_EXPERIMENT is the name of the environment variable you should set to "1"
+// to bypass the algorithm preventing disabled by default experiments to be instantiated.
+const OONI_FORCE_ENABLE_EXPERIMENT = "OONI_FORCE_ENABLE_EXPERIMENT"
+
 // NewFactory creates a new Factory instance.
-func NewFactory(name string) (*Factory, error) {
+func NewFactory(name string, kvStore model.KeyValueStore, logger model.Logger) (*Factory, error) {
+	// Make sure we are deadling with the canonical experiment name. Historically MK used
+	// names such as WebConnectivity and we want to continue supporting this use case.
 	name = CanonicalizeExperimentName(name)
+
+	// Handle A/B testing where we dynamically choose LTE for some users. The current policy
+	// only relates to a few users to collect data.
+	//
+	// TODO(https://github.com/ooni/probe/issues/2555): perform the actual comparison
+	// and improve the LTE implementation so that we can always use it. See the actual
+	// issue test for additional details on this planned A/B test.
+	switch {
+	case name == "web_connectivity" && checkincache.GetFeatureFlag(kvStore, "webconnectivity_0.5"):
+		// use LTE rather than the normal webconnectivity when the
+		// feature flag has been set through the check-in API
+		logger.Infof("using webconnectivity LTE")
+		name = "web_connectivity@v0.5"
+
+	default:
+		// nothing
+	}
+
+	// Obtain the factory for the canonical name.
 	factory := AllExperiments[name]
 	if factory == nil {
 		return nil, fmt.Errorf("%w: %s", ErrNoSuchExperiment, name)
 	}
-	return factory, nil
+
+	// Some experiments are not enabled by default. To enable them we use
+	// the cached check-in response or an environment variable.
+	//
+	// Note: check-in flags expire after 24h.
+	//
+	// TODO(https://github.com/ooni/probe/issues/2554): we need to restructure
+	// how we run experiments to make sure check-in flags are always fresh.
+	if factory.enabledByDefault {
+		return factory, nil // enabled by default
+	}
+	if os.Getenv(OONI_FORCE_ENABLE_EXPERIMENT) == "1" {
+		return factory, nil // enabled by environment variable
+	}
+	if checkincache.ExperimentEnabled(kvStore, name) {
+		return factory, nil // enabled by check-in
+	}
+
+	logger.Warnf(experimentDisabledByCheckInWarning, name)
+	return nil, fmt.Errorf("%s: %w", name, ErrRequiresForceEnable)
 }