From 7711805e9e4681817faae23ae3fe0ef41ef2f3f7 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 10 Oct 2023 16:52:19 +0200 Subject: [PATCH 1/8] feat: mvp of conditionally enabling experiments We have a class of experiments that I call experimental experiments. As a reminder, experimental experiments are the ones we have just added in a development cycle and which may potentially have issues down the line, so that we may want to disable them while working on an emergency release to fix them. Currently, riseupvpn and echechk fall into this class. The idea of this diff is to implement the functional specification described by issue https://github.com/ooni/probe/issues/2553. --- internal/checkincache/checkincache.go | 9 ++++++++ internal/engine/session.go | 31 +++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/internal/checkincache/checkincache.go b/internal/checkincache/checkincache.go index 6d16e18a6a..d4bde38aa6 100644 --- a/internal/checkincache/checkincache.go +++ b/internal/checkincache/checkincache.go @@ -3,6 +3,7 @@ package checkincache import ( "encoding/json" + "fmt" "time" "github.com/ooni/probe-cli/v3/internal/model" @@ -52,3 +53,11 @@ func GetFeatureFlag(kvStore model.KeyValueStore, name string) bool { } return wrapper.Flags[name] // works even if map is nil } + +// ExperimentEnabled returns whether a given experiment has been enabled by a previous +// execution of check-in. We use this feature to disable experimental experiments that may +// start misbehaving thus requiring us to issue an emergency release. +func ExperimentEnabled(kvStore model.KeyValueStore, name string) bool { + key := fmt.Sprintf("%s_enabled", name) + return GetFeatureFlag(kvStore, key) +} diff --git a/internal/engine/session.go b/internal/engine/session.go index 6d6bae043f..76f149ce7b 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -402,10 +402,19 @@ var ErrAlreadyUsingProxy = errors.New( "session: cannot create a new tunnel of this kind: we are already using a proxy", ) +// ErrExperimentNotEnabled indicates that an experiment is not enabled by the check-in API, which +// typically happens when we know that a given experiment is known to have issues. +var ErrExperimentNotEnabled = errors.New("session: experiment not enabled by check-in API") + // 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 func (s *Session) NewExperimentBuilder(name string) (model.ExperimentBuilder, error) { + + // Handle the experiment where we dynamically chose LTE for some users. + // + // TODO(https://github.com/ooni/probe/issues/2555): perform the actual comparison + // and improve the LTE implementation so that we can always use it. name = registry.CanonicalizeExperimentName(name) switch { case name == "web_connectivity" && checkincache.GetFeatureFlag(s.kvStore, "webconnectivity_0.5"): @@ -413,9 +422,31 @@ func (s *Session) NewExperimentBuilder(name string) (model.ExperimentBuilder, er // feature flag has been set through the check-in API s.Logger().Infof("using webconnectivity LTE") name = "web_connectivity@v0.5" + default: // nothing } + + // Some experiments are disabled by default and we re-enable them or + // disable them again using the check-in API. + // + // TODO(https://github.com/ooni/probe/issues/2554): we need to restructure + // of we run experiments to make sure check-in flags are always fresh. + switch name { + case "riseupvpn", "echcheck": + if os.Getenv("OONI_FORCE_ENABLE_EXPERIMENT") != "1" { + if !checkincache.ExperimentEnabled(s.kvStore, name) { + s.logger.Warnf("experiment '%s' not enabled through the check-in API", name) + s.logger.Warnf("use `export OONI_FORCE_ENABLE_EXPERIMENT=1 to bypass this restriction") + return nil, ErrExperimentNotEnabled + } + } + // fallthrough + + default: + // nothing + } + eb, err := newExperimentBuilder(s, name) if err != nil { return nil, err From b1413edc6eeebf63a7d63022dad862bba447f168 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 11 Oct 2023 08:19:32 +0200 Subject: [PATCH 2/8] x --- internal/engine/session.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/engine/session.go b/internal/engine/session.go index 76f149ce7b..eede8c274c 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -411,10 +411,12 @@ var ErrExperimentNotEnabled = errors.New("session: experiment not enabled by che // there's no such experiment with the given name func (s *Session) NewExperimentBuilder(name string) (model.ExperimentBuilder, error) { - // Handle the experiment where we dynamically chose LTE for some users. + // 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. + // 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. name = registry.CanonicalizeExperimentName(name) switch { case name == "web_connectivity" && checkincache.GetFeatureFlag(s.kvStore, "webconnectivity_0.5"): @@ -451,6 +453,7 @@ func (s *Session) NewExperimentBuilder(name string) (model.ExperimentBuilder, er if err != nil { return nil, err } + return eb, nil } From 1c4eb6e17e91c96738161f791426da9c172f604f Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 11 Oct 2023 10:14:52 +0200 Subject: [PATCH 3/8] x --- internal/engine/experimentbuilder.go | 2 +- internal/engine/inputloader.go | 2 + internal/engine/session.go | 42 --- internal/registry/allexperiments.go | 3 + internal/registry/dash.go | 7 +- internal/registry/dnscheck.go | 5 +- internal/registry/dnsping.go | 5 +- internal/registry/example.go | 5 +- internal/registry/factory.go | 61 +++- internal/registry/factory_test.go | 391 ++++++++++++++++++++++++ internal/registry/fbmessenger.go | 5 +- internal/registry/hhfm.go | 5 +- internal/registry/hirl.go | 5 +- internal/registry/httphostheader.go | 5 +- internal/registry/ndt.go | 7 +- internal/registry/portfiltering.go | 7 +- internal/registry/psiphon.go | 5 +- internal/registry/quicping.go | 5 +- internal/registry/run.go | 5 +- internal/registry/signal.go | 5 +- internal/registry/simplequicping.go | 5 +- internal/registry/sniblocking.go | 5 +- internal/registry/stunreachability.go | 5 +- internal/registry/tcpping.go | 5 +- internal/registry/telegram.go | 7 +- internal/registry/tlsmiddlebox.go | 5 +- internal/registry/tlsping.go | 5 +- internal/registry/tlstool.go | 5 +- internal/registry/tor.go | 5 +- internal/registry/torsf.go | 5 +- internal/registry/urlgetter.go | 5 +- internal/registry/vanillator.go | 5 +- internal/registry/webconnectivity.go | 7 +- internal/registry/webconnectivityv05.go | 7 +- internal/registry/whatsapp.go | 5 +- 35 files changed, 549 insertions(+), 109 deletions(-) diff --git a/internal/engine/experimentbuilder.go b/internal/engine/experimentbuilder.go index 3cbd46f218..a60d0a50d5 100644 --- a/internal/engine/experimentbuilder.go +++ b/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 } diff --git a/internal/engine/inputloader.go b/internal/engine/inputloader.go index 5a50505fb9..34074ee628 100644 --- a/internal/engine/inputloader.go +++ b/internal/engine/inputloader.go @@ -219,6 +219,8 @@ 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(XXX) switch registry.CanonicalizeExperimentName(name) { case "dnscheck": return dnsCheckDefaultInput, nil diff --git a/internal/engine/session.go b/internal/engine/session.go index eede8c274c..c809022696 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -11,7 +11,6 @@ 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" @@ -19,7 +18,6 @@ import ( "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" @@ -410,50 +408,10 @@ var ErrExperimentNotEnabled = errors.New("session: experiment not enabled by che // 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) { - - // 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. - 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 - } - - // Some experiments are disabled by default and we re-enable them or - // disable them again using the check-in API. - // - // TODO(https://github.com/ooni/probe/issues/2554): we need to restructure - // of we run experiments to make sure check-in flags are always fresh. - switch name { - case "riseupvpn", "echcheck": - if os.Getenv("OONI_FORCE_ENABLE_EXPERIMENT") != "1" { - if !checkincache.ExperimentEnabled(s.kvStore, name) { - s.logger.Warnf("experiment '%s' not enabled through the check-in API", name) - s.logger.Warnf("use `export OONI_FORCE_ENABLE_EXPERIMENT=1 to bypass this restriction") - return nil, ErrExperimentNotEnabled - } - } - // fallthrough - - default: - // nothing - } - eb, err := newExperimentBuilder(s, name) if err != nil { return nil, err } - return eb, nil } diff --git a/internal/registry/allexperiments.go b/internal/registry/allexperiments.go index d191b6c515..fb53b737ec 100644 --- a/internal/registry/allexperiments.go +++ b/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) return } diff --git a/internal/registry/dash.go b/internal/registry/dash.go index 8551f6a20a..59175fca0c 100644 --- a/internal/registry/dash.go +++ b/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, } } diff --git a/internal/registry/dnscheck.go b/internal/registry/dnscheck.go index f949886938..3d474613a2 100644 --- a/internal/registry/dnscheck.go +++ b/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, } } diff --git a/internal/registry/dnsping.go b/internal/registry/dnsping.go index 362c9b7f01..1ccb3122c0 100644 --- a/internal/registry/dnsping.go +++ b/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, } } diff --git a/internal/registry/example.go b/internal/registry/example.go index e8af265d8e..e487b22f04 100644 --- a/internal/registry/example.go +++ b/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, } } diff --git a/internal/registry/factory.go b/internal/registry/factory.go index 9789bb3688..9ae8683ca3 100644 --- a/internal/registry/factory.go +++ b/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,64 @@ 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.` + +const experimentForceEnableEnvVar = "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. + name = CanonicalizeExperimentName(name) + 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 + // of we run experiments to make sure check-in flags are always fresh. + if factory.enabledByDefault { + return factory, nil // enabled by default + } + if os.Getenv(experimentForceEnableEnvVar) == "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) } diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index f881ffdd81..bba0c607fc 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -2,9 +2,15 @@ package registry import ( "errors" + "fmt" + "os" "testing" "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/checkincache" + "github.com/ooni/probe-cli/v3/internal/experiment/webconnectivitylte" + "github.com/ooni/probe-cli/v3/internal/kvstore" + "github.com/ooni/probe-cli/v3/internal/model" ) type fakeExperimentConfig struct { @@ -345,3 +351,388 @@ func TestExperimentBuilderSetOptionsAny(t *testing.T) { } }) } + +func TestNewFactory(t *testing.T) { + // experimentSpecificExpectations contains expectations for an experiment + type experimentSpecificExpectations struct { + // enabledByDefault contains the expected value for the enabledByDefault factory field. + enabledByDefault bool + + // inputPolicy contains the expected value for the input policy. + inputPolicy model.InputPolicy + + // interruptible contains the expected value for interrupted. + interruptible bool + } + + // expectationsMap contains expectations for each experiment that exists + expectationsMap := map[string]*experimentSpecificExpectations{ + "dash": { + enabledByDefault: true, + inputPolicy: model.InputNone, + interruptible: true, + }, + "dnscheck": { + enabledByDefault: true, + inputPolicy: model.InputOrStaticDefault, + }, + "dnsping": { + enabledByDefault: true, + inputPolicy: model.InputOrStaticDefault, + }, + "echcheck": { + // Note: echcheck is not enabled by default because we just introduced it + // into 3.19.0-alpha, which makes it a relatively new experiment. + //enabledByDefault: false, + inputPolicy: model.InputOptional, + }, + "example": { + enabledByDefault: true, + inputPolicy: model.InputNone, + interruptible: true, + }, + "facebook_messenger": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "http_header_field_manipulation": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "http_host_header": { + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, + }, + "http_invalid_request_line": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "ndt": { + enabledByDefault: true, + inputPolicy: model.InputNone, + interruptible: true, + }, + "portfiltering": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "psiphon": { + enabledByDefault: true, + inputPolicy: model.InputOptional, + }, + "quicping": { + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, + }, + "riseupvpn": { + // Note: riseupvpn is not enabled by default because it has been flaky + // in the past and we want to be defensive here. + //enabledByDefault: false, + inputPolicy: model.InputNone, + }, + "run": { + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, + }, + "signal": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "simple_sni": { + // Note: simple_sni is not enabled by default because it has only been + // introduced for writing tutorials and should not be used. + //enabledByDefault: false, + inputPolicy: model.InputOrQueryBackend, + }, + "simplequicping": { + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, + }, + "sni_blocking": { + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, + }, + "stunreachability": { + enabledByDefault: true, + inputPolicy: model.InputOrStaticDefault, + }, + "tcpping": { + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, + }, + "tlsmiddlebox": { + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, + }, + "telegram": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "tlsping": { + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, + }, + "tlstool": { + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, + }, + "tor": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "torsf": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "urlgetter": { + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, + }, + "vanilla_tor": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "web_connectivity": { + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, + }, + "web_connectivity@v0.5": { + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, + }, + "whatsapp": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + } + + // testCase is a test case checked by this func + type testCase struct { + // description describes the test case + description string + + // experimentName is the experiment experimentName + experimentName string + + // kvStore is the key-value store to use + kvStore model.KeyValueStore + + // setForceEnableExperiment sets the OONI_FORCE_ENABLE_EXPERIMENT=1 env variable + setForceEnableExperiment bool + + // expectErr is the error we expect when calling NewFactory + expectErr error + } + + // allCases contains all test cases + allCases := []*testCase{} + + // create test cases for canonical experiment names + for _, name := range ExperimentNames() { + allCases = append(allCases, &testCase{ + description: name, + experimentName: name, + kvStore: &kvstore.Memory{}, + expectErr: (func() error { + expectations := expectationsMap[name] + if expectations == nil { + t.Fatal("no expectations for", name) + } + if !expectations.enabledByDefault { + return ErrRequiresForceEnable + } + return nil + }()), + }) + } + + // add additional test for the ndt7 experiment name + allCases = append(allCases, &testCase{ + description: "the ndt7 name still works", + experimentName: "ndt7", + kvStore: &kvstore.Memory{}, + expectErr: nil, + }) + + // add additional test for the dns_check experiment name + allCases = append(allCases, &testCase{ + description: "the dns_check name still works", + experimentName: "dns_check", + kvStore: &kvstore.Memory{}, + expectErr: nil, + }) + + // add additional test for the stun_reachability experiment name + allCases = append(allCases, &testCase{ + description: "the stun_reachability name still works", + experimentName: "stun_reachability", + kvStore: &kvstore.Memory{}, + expectErr: nil, + }) + + // add additional test for the web_connectivity@v_0_5 experiment name + allCases = append(allCases, &testCase{ + description: "the web_connectivity@v_0_5 name still works", + experimentName: "web_connectivity@v_0_5", + kvStore: &kvstore.Memory{}, + expectErr: nil, + }) + + // make sure we can create default-not-enabled experiments if we + // configure the proper environment variable + for name, expectations := range expectationsMap { + if expectations.enabledByDefault { + continue + } + + allCases = append(allCases, &testCase{ + description: fmt.Sprintf("we can create %s with OONI_FORCE_ENABLE_EXPERIMENT=1", name), + experimentName: name, + kvStore: &kvstore.Memory{}, + setForceEnableExperiment: true, + expectErr: nil, + }) + } + + // make sure we can create default-not-enabled experiments if we + // configure the proper check-in flags + for name, expectations := range expectationsMap { + if expectations.enabledByDefault { + continue + } + + // create a check-in configuration with the experiment being enabled + store := &kvstore.Memory{} + checkincache.Store(store, &model.OOAPICheckInResult{ + Conf: model.OOAPICheckInResultConfig{ + Features: map[string]bool{ + fmt.Sprintf("%s_enabled", name): true, + }, + }, + }) + + allCases = append(allCases, &testCase{ + description: fmt.Sprintf("we can create %s with the proper check-in config", name), + experimentName: name, + kvStore: store, + setForceEnableExperiment: false, + expectErr: nil, + }) + } + + // perform checks for each name + for _, tc := range allCases { + t.Run(tc.description, func(t *testing.T) { + // make sure the bypass environment variable is not set + if os.Getenv(experimentForceEnableEnvVar) != "" { + t.Fatal("the OONI_FORCE_ENABLE_EXPERIMENT env variable shouldn't be set") + } + + // if needed, set the environment variable for the scope of the func + if tc.setForceEnableExperiment { + os.Setenv(experimentForceEnableEnvVar, "1") + defer os.Unsetenv(experimentForceEnableEnvVar) + } + + t.Log("experimentName:", tc.experimentName) + + // get experiment expectations -- note that here we must canonicalize the + // experiment name otherwise we won't find it into the map + expectations := expectationsMap[CanonicalizeExperimentName(tc.experimentName)] + if expectations == nil { + t.Fatal("no expectations for", tc.experimentName) + } + + t.Logf("expectations: %+v", expectations) + + // get the experiment factory + factory, err := NewFactory(tc.experimentName, tc.kvStore, model.DiscardLogger) + + t.Logf("NewFactory returned: %+v %+v", factory, err) + + // make sure the returned error makes sense + switch { + case tc.expectErr == nil && err != nil: + t.Fatal(tc.experimentName, ": expected", tc.expectErr, "got", err) + + case tc.expectErr != nil && err == nil: + t.Fatal(tc.experimentName, ": expected", tc.expectErr, "got", err) + + case tc.expectErr != nil && err != nil: + if !errors.Is(err, tc.expectErr) { + t.Fatal(tc.experimentName, ": expected", tc.expectErr, "got", err) + } + return + + case tc.expectErr == nil && err == nil: + // fallthrough + } + + // make sure the enabled by default field is consistent with expectations + if factory.enabledByDefault != expectations.enabledByDefault { + t.Fatal(tc.experimentName, ": expected", expectations.enabledByDefault, "got", factory.enabledByDefault) + } + + // make sure the input policy is the expected one + if factory.inputPolicy != expectations.inputPolicy { + t.Fatal(tc.experimentName, ": expected", expectations.inputPolicy, "got", factory.inputPolicy) + } + + // make sure the interrupted value is the expected one + if factory.interruptible != expectations.interruptible { + t.Fatal(tc.experimentName, ": expected", expectations.interruptible, "got", factory.interruptible) + } + + // make sure we can creare the measurer + measurer := factory.NewExperimentMeasurer() + if measurer == nil { + t.Fatal("expected non-nil measurer, got nil") + } + }) + } + + // make sure we create web_connectivity@v0.5 when the check-in says so + t.Run("we honor check-in flags for web_connectivity@v0.5", func(t *testing.T) { + // create a keyvalue store with the proper flags + store := &kvstore.Memory{} + checkincache.Store(store, &model.OOAPICheckInResult{ + Conf: model.OOAPICheckInResultConfig{ + Features: map[string]bool{ + "webconnectivity_0.5": true, + }, + }, + }) + + // get the experiment factory + factory, err := NewFactory("web_connectivity", store, model.DiscardLogger) + if err != nil { + t.Fatal(err) + } + + // make sure the enabled by default field is consistent with expectations + if !factory.enabledByDefault { + t.Fatal("expected enabledByDefault to be true") + } + + // make sure the input policy is the expected one + if factory.inputPolicy != model.InputOrQueryBackend { + t.Fatal("expected inputPolicy to be InputOrQueryBackend") + } + + // make sure the interrupted value is the expected one + if factory.interruptible { + t.Fatal("expected interruptible to be false") + } + + // make sure we can creare the measurer + measurer := factory.NewExperimentMeasurer() + if measurer == nil { + t.Fatal("expected non-nil measurer, got nil") + } + + // make sure the type we're creating is the correct one + if _, good := measurer.(*webconnectivitylte.Measurer); !good { + t.Fatalf("expected to see an instance of *webconnectivitylte.Measurer, got %T", measurer) + } + }) +} diff --git a/internal/registry/fbmessenger.go b/internal/registry/fbmessenger.go index 73bee4fe3c..0e3be67df0 100644 --- a/internal/registry/fbmessenger.go +++ b/internal/registry/fbmessenger.go @@ -16,7 +16,8 @@ func init() { *config.(*fbmessenger.Config), ) }, - config: &fbmessenger.Config{}, - inputPolicy: model.InputNone, + config: &fbmessenger.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/hhfm.go b/internal/registry/hhfm.go index 3b1038ad64..44eb426e92 100644 --- a/internal/registry/hhfm.go +++ b/internal/registry/hhfm.go @@ -16,7 +16,8 @@ func init() { *config.(*hhfm.Config), ) }, - config: &hhfm.Config{}, - inputPolicy: model.InputNone, + config: &hhfm.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/hirl.go b/internal/registry/hirl.go index 2c55b53b74..1209052fc1 100644 --- a/internal/registry/hirl.go +++ b/internal/registry/hirl.go @@ -16,7 +16,8 @@ func init() { *config.(*hirl.Config), ) }, - config: &hirl.Config{}, - inputPolicy: model.InputNone, + config: &hirl.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/httphostheader.go b/internal/registry/httphostheader.go index 7c7ff890fd..0eaa98c654 100644 --- a/internal/registry/httphostheader.go +++ b/internal/registry/httphostheader.go @@ -16,7 +16,8 @@ func init() { *config.(*httphostheader.Config), ) }, - config: &httphostheader.Config{}, - inputPolicy: model.InputOrQueryBackend, + config: &httphostheader.Config{}, + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, } } diff --git a/internal/registry/ndt.go b/internal/registry/ndt.go index ded014a4c0..c07d5ff7ff 100644 --- a/internal/registry/ndt.go +++ b/internal/registry/ndt.go @@ -16,8 +16,9 @@ func init() { *config.(*ndt7.Config), ) }, - config: &ndt7.Config{}, - interruptible: true, - inputPolicy: model.InputNone, + config: &ndt7.Config{}, + enabledByDefault: true, + interruptible: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/portfiltering.go b/internal/registry/portfiltering.go index 2c175638b4..36e152f38b 100644 --- a/internal/registry/portfiltering.go +++ b/internal/registry/portfiltering.go @@ -16,8 +16,9 @@ func init() { config.(portfiltering.Config), ) }, - config: portfiltering.Config{}, - interruptible: false, - inputPolicy: model.InputNone, + config: portfiltering.Config{}, + enabledByDefault: true, + interruptible: false, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/psiphon.go b/internal/registry/psiphon.go index 18aae3d784..46ff7e06ca 100644 --- a/internal/registry/psiphon.go +++ b/internal/registry/psiphon.go @@ -16,7 +16,8 @@ func init() { *config.(*psiphon.Config), ) }, - config: &psiphon.Config{}, - inputPolicy: model.InputOptional, + config: &psiphon.Config{}, + enabledByDefault: true, + inputPolicy: model.InputOptional, } } diff --git a/internal/registry/quicping.go b/internal/registry/quicping.go index 7ada9c3e0b..67a87870c9 100644 --- a/internal/registry/quicping.go +++ b/internal/registry/quicping.go @@ -16,7 +16,8 @@ func init() { *config.(*quicping.Config), ) }, - config: &quicping.Config{}, - inputPolicy: model.InputStrictlyRequired, + config: &quicping.Config{}, + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, } } diff --git a/internal/registry/run.go b/internal/registry/run.go index 2310670c15..371aac43bb 100644 --- a/internal/registry/run.go +++ b/internal/registry/run.go @@ -16,7 +16,8 @@ func init() { *config.(*run.Config), ) }, - config: &run.Config{}, - inputPolicy: model.InputStrictlyRequired, + config: &run.Config{}, + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, } } diff --git a/internal/registry/signal.go b/internal/registry/signal.go index 9c568ce077..927630ff00 100644 --- a/internal/registry/signal.go +++ b/internal/registry/signal.go @@ -16,7 +16,8 @@ func init() { *config.(*signal.Config), ) }, - config: &signal.Config{}, - inputPolicy: model.InputNone, + config: &signal.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/simplequicping.go b/internal/registry/simplequicping.go index 33bf29b65d..6b56ad4ed7 100644 --- a/internal/registry/simplequicping.go +++ b/internal/registry/simplequicping.go @@ -16,7 +16,8 @@ func init() { *config.(*simplequicping.Config), ) }, - config: &simplequicping.Config{}, - inputPolicy: model.InputStrictlyRequired, + config: &simplequicping.Config{}, + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, } } diff --git a/internal/registry/sniblocking.go b/internal/registry/sniblocking.go index bda66d1805..a7f5fb7897 100644 --- a/internal/registry/sniblocking.go +++ b/internal/registry/sniblocking.go @@ -16,7 +16,8 @@ func init() { *config.(*sniblocking.Config), ) }, - config: &sniblocking.Config{}, - inputPolicy: model.InputOrQueryBackend, + config: &sniblocking.Config{}, + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, } } diff --git a/internal/registry/stunreachability.go b/internal/registry/stunreachability.go index 38ab7cf16f..291f5bb6ac 100644 --- a/internal/registry/stunreachability.go +++ b/internal/registry/stunreachability.go @@ -16,7 +16,8 @@ func init() { *config.(*stunreachability.Config), ) }, - config: &stunreachability.Config{}, - inputPolicy: model.InputOrStaticDefault, + config: &stunreachability.Config{}, + enabledByDefault: true, + inputPolicy: model.InputOrStaticDefault, } } diff --git a/internal/registry/tcpping.go b/internal/registry/tcpping.go index 0be3933832..89d01189dc 100644 --- a/internal/registry/tcpping.go +++ b/internal/registry/tcpping.go @@ -16,7 +16,8 @@ func init() { *config.(*tcpping.Config), ) }, - config: &tcpping.Config{}, - inputPolicy: model.InputStrictlyRequired, + config: &tcpping.Config{}, + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, } } diff --git a/internal/registry/telegram.go b/internal/registry/telegram.go index 2218dfd248..d68ffea468 100644 --- a/internal/registry/telegram.go +++ b/internal/registry/telegram.go @@ -16,8 +16,9 @@ func init() { config.(telegram.Config), ) }, - config: telegram.Config{}, - interruptible: false, - inputPolicy: model.InputNone, + config: telegram.Config{}, + enabledByDefault: true, + interruptible: false, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/tlsmiddlebox.go b/internal/registry/tlsmiddlebox.go index 4ca5733b12..132df65027 100644 --- a/internal/registry/tlsmiddlebox.go +++ b/internal/registry/tlsmiddlebox.go @@ -16,7 +16,8 @@ func init() { *config.(*tlsmiddlebox.Config), ) }, - config: &tlsmiddlebox.Config{}, - inputPolicy: model.InputStrictlyRequired, + config: &tlsmiddlebox.Config{}, + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, } } diff --git a/internal/registry/tlsping.go b/internal/registry/tlsping.go index 862b3186d0..4d49fc59dc 100644 --- a/internal/registry/tlsping.go +++ b/internal/registry/tlsping.go @@ -16,7 +16,8 @@ func init() { *config.(*tlsping.Config), ) }, - config: &tlsping.Config{}, - inputPolicy: model.InputStrictlyRequired, + config: &tlsping.Config{}, + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, } } diff --git a/internal/registry/tlstool.go b/internal/registry/tlstool.go index ed2047ddbf..8327a4ad7e 100644 --- a/internal/registry/tlstool.go +++ b/internal/registry/tlstool.go @@ -16,7 +16,8 @@ func init() { *config.(*tlstool.Config), ) }, - config: &tlstool.Config{}, - inputPolicy: model.InputOrQueryBackend, + config: &tlstool.Config{}, + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, } } diff --git a/internal/registry/tor.go b/internal/registry/tor.go index e5dc81f212..02b6c9b2f9 100644 --- a/internal/registry/tor.go +++ b/internal/registry/tor.go @@ -16,7 +16,8 @@ func init() { *config.(*tor.Config), ) }, - config: &tor.Config{}, - inputPolicy: model.InputNone, + config: &tor.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/torsf.go b/internal/registry/torsf.go index 38ef135151..ad2d6051c3 100644 --- a/internal/registry/torsf.go +++ b/internal/registry/torsf.go @@ -16,7 +16,8 @@ func init() { *config.(*torsf.Config), ) }, - config: &torsf.Config{}, - inputPolicy: model.InputNone, + config: &torsf.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/urlgetter.go b/internal/registry/urlgetter.go index 8044af92ce..8f8c45985a 100644 --- a/internal/registry/urlgetter.go +++ b/internal/registry/urlgetter.go @@ -16,7 +16,8 @@ func init() { *config.(*urlgetter.Config), ) }, - config: &urlgetter.Config{}, - inputPolicy: model.InputStrictlyRequired, + config: &urlgetter.Config{}, + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, } } diff --git a/internal/registry/vanillator.go b/internal/registry/vanillator.go index 5f9d4fa36d..cd8fae6583 100644 --- a/internal/registry/vanillator.go +++ b/internal/registry/vanillator.go @@ -16,7 +16,8 @@ func init() { *config.(*vanillator.Config), ) }, - config: &vanillator.Config{}, - inputPolicy: model.InputNone, + config: &vanillator.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/webconnectivity.go b/internal/registry/webconnectivity.go index ada8767417..2e8b857861 100644 --- a/internal/registry/webconnectivity.go +++ b/internal/registry/webconnectivity.go @@ -16,8 +16,9 @@ func init() { config.(webconnectivity.Config), ) }, - config: webconnectivity.Config{}, - interruptible: false, - inputPolicy: model.InputOrQueryBackend, + config: webconnectivity.Config{}, + enabledByDefault: true, + interruptible: false, + inputPolicy: model.InputOrQueryBackend, } } diff --git a/internal/registry/webconnectivityv05.go b/internal/registry/webconnectivityv05.go index 7697113f61..28a5d3c8de 100644 --- a/internal/registry/webconnectivityv05.go +++ b/internal/registry/webconnectivityv05.go @@ -18,8 +18,9 @@ func init() { config.(*webconnectivitylte.Config), ) }, - config: &webconnectivitylte.Config{}, - interruptible: false, - inputPolicy: model.InputOrQueryBackend, + config: &webconnectivitylte.Config{}, + enabledByDefault: true, + interruptible: false, + inputPolicy: model.InputOrQueryBackend, } } diff --git a/internal/registry/whatsapp.go b/internal/registry/whatsapp.go index 9d82598260..cac5efde0e 100644 --- a/internal/registry/whatsapp.go +++ b/internal/registry/whatsapp.go @@ -16,7 +16,8 @@ func init() { *config.(*whatsapp.Config), ) }, - config: &whatsapp.Config{}, - inputPolicy: model.InputNone, + config: &whatsapp.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } From 994ae6f9f3b8d488e1efe84f6ee5e696f3c0f211 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 11 Oct 2023 10:25:32 +0200 Subject: [PATCH 4/8] x --- internal/checkincache/checkincache.go | 9 +++++++-- internal/engine/inputloader.go | 4 +++- internal/registry/factory_test.go | 26 +++++++++++++++++++------- 3 files changed, 29 insertions(+), 10 deletions(-) diff --git a/internal/checkincache/checkincache.go b/internal/checkincache/checkincache.go index d4bde38aa6..1610580285 100644 --- a/internal/checkincache/checkincache.go +++ b/internal/checkincache/checkincache.go @@ -54,10 +54,15 @@ 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. We use this feature to disable experimental experiments that may // start misbehaving thus requiring us to issue an emergency release. func ExperimentEnabled(kvStore model.KeyValueStore, name string) bool { - key := fmt.Sprintf("%s_enabled", name) - return GetFeatureFlag(kvStore, key) + return GetFeatureFlag(kvStore, ExperimentEnabledKey(name)) } diff --git a/internal/engine/inputloader.go b/internal/engine/inputloader.go index 34074ee628..788cbb5191 100644 --- a/internal/engine/inputloader.go +++ b/internal/engine/inputloader.go @@ -220,7 +220,9 @@ func StaticBareInputForExperiment(name string) ([]string, error) { // with a non-canonical experiment name, so we need to convert // the experiment name to be canonical before proceeding. // - // TODO(XXX) + // TODO(https://github.com/ooni/probe/issues/1390) + // + // TODO(https://github.com/ooni/probe/issues/2557) switch registry.CanonicalizeExperimentName(name) { case "dnscheck": return dnsCheckDefaultInput, nil diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index bba0c607fc..a24a142681 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -606,7 +606,7 @@ func TestNewFactory(t *testing.T) { checkincache.Store(store, &model.OOAPICheckInResult{ Conf: model.OOAPICheckInResultConfig{ Features: map[string]bool{ - fmt.Sprintf("%s_enabled", name): true, + checkincache.ExperimentEnabledKey(name): true, }, }, }) @@ -674,13 +674,13 @@ func TestNewFactory(t *testing.T) { } // make sure the input policy is the expected one - if factory.inputPolicy != expectations.inputPolicy { - t.Fatal(tc.experimentName, ": expected", expectations.inputPolicy, "got", factory.inputPolicy) + if v := factory.InputPolicy(); v != expectations.inputPolicy { + t.Fatal(tc.experimentName, ": expected", expectations.inputPolicy, "got", v) } // make sure the interrupted value is the expected one - if factory.interruptible != expectations.interruptible { - t.Fatal(tc.experimentName, ": expected", expectations.interruptible, "got", factory.interruptible) + if v := factory.Interruptible(); v != expectations.interruptible { + t.Fatal(tc.experimentName, ": expected", expectations.interruptible, "got", v) } // make sure we can creare the measurer @@ -715,12 +715,12 @@ func TestNewFactory(t *testing.T) { } // make sure the input policy is the expected one - if factory.inputPolicy != model.InputOrQueryBackend { + if factory.InputPolicy() != model.InputOrQueryBackend { t.Fatal("expected inputPolicy to be InputOrQueryBackend") } // make sure the interrupted value is the expected one - if factory.interruptible { + if factory.Interruptible() { t.Fatal("expected interruptible to be false") } @@ -735,4 +735,16 @@ func TestNewFactory(t *testing.T) { t.Fatalf("expected to see an instance of *webconnectivitylte.Measurer, got %T", measurer) } }) + + // add a test case for a nonexistent experiment + t.Run("we correctly return an error for a nonexistent experiment", func(t *testing.T) { + // the empty string is a nonexistent experiment + factory, err := NewFactory("", &kvstore.Memory{}, model.DiscardLogger) + if !errors.Is(err, ErrNoSuchExperiment) { + t.Fatal("unexpected err", err) + } + if factory != nil { + t.Fatal("expected nil factory here") + } + }) } From 7eb1579b05fe005c91bc704b623725f0cc5fc037 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 11 Oct 2023 10:31:35 +0200 Subject: [PATCH 5/8] x --- internal/checkincache/checkincache.go | 7 +++++-- internal/engine/inputloader.go | 6 ++++-- internal/engine/session.go | 4 ---- internal/registry/allexperiments.go | 2 +- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/internal/checkincache/checkincache.go b/internal/checkincache/checkincache.go index 1610580285..4b6e0442ab 100644 --- a/internal/checkincache/checkincache.go +++ b/internal/checkincache/checkincache.go @@ -26,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{ @@ -61,8 +64,8 @@ func ExperimentEnabledKey(name string) string { } // ExperimentEnabled returns whether a given experiment has been enabled by a previous -// execution of check-in. We use this feature to disable experimental experiments that may -// start misbehaving thus requiring us to issue an emergency release. +// 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)) } diff --git a/internal/engine/inputloader.go b/internal/engine/inputloader.go index 788cbb5191..9bc8b0a86e 100644 --- a/internal/engine/inputloader.go +++ b/internal/engine/inputloader.go @@ -220,9 +220,11 @@ func StaticBareInputForExperiment(name string) ([]string, error) { // 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) + // 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) + // 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 diff --git a/internal/engine/session.go b/internal/engine/session.go index c809022696..27e897d48c 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -400,10 +400,6 @@ var ErrAlreadyUsingProxy = errors.New( "session: cannot create a new tunnel of this kind: we are already using a proxy", ) -// ErrExperimentNotEnabled indicates that an experiment is not enabled by the check-in API, which -// typically happens when we know that a given experiment is known to have issues. -var ErrExperimentNotEnabled = errors.New("session: experiment not enabled by check-in API") - // 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/registry/allexperiments.go b/internal/registry/allexperiments.go index fb53b737ec..39ce61226b 100644 --- a/internal/registry/allexperiments.go +++ b/internal/registry/allexperiments.go @@ -10,6 +10,6 @@ func ExperimentNames() (names []string) { for key := range AllExperiments { names = append(names, key) } - sort.Strings(names) + sort.Strings(names) // sort by name to always provide predictable output return } From 5a592ce95e59e4833699c477afb109e147d9889e Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 11 Oct 2023 10:36:23 +0200 Subject: [PATCH 6/8] x --- internal/registry/factory.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/registry/factory.go b/internal/registry/factory.go index 9ae8683ca3..2fde819a7c 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -246,7 +246,6 @@ func NewFactory(name string, kvStore model.KeyValueStore, logger model.Logger) ( // 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. - name = CanonicalizeExperimentName(name) switch { case name == "web_connectivity" && checkincache.GetFeatureFlag(kvStore, "webconnectivity_0.5"): // use LTE rather than the normal webconnectivity when the From 186b4ba0dcaf008c3330475c1b476d255f6e8fb4 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 11 Oct 2023 10:43:45 +0200 Subject: [PATCH 7/8] Apply suggestions from code review --- internal/registry/factory.go | 2 +- internal/registry/factory_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/registry/factory.go b/internal/registry/factory.go index 2fde819a7c..fe2c328e23 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -269,7 +269,7 @@ func NewFactory(name string, kvStore model.KeyValueStore, logger model.Logger) ( // Note: check-in flags expire after 24h. // // TODO(https://github.com/ooni/probe/issues/2554): we need to restructure - // of we run experiments to make sure check-in flags are always fresh. + // how we run experiments to make sure check-in flags are always fresh. if factory.enabledByDefault { return factory, nil // enabled by default } diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index a24a142681..2523a1a8f1 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -637,7 +637,7 @@ func TestNewFactory(t *testing.T) { t.Log("experimentName:", tc.experimentName) // get experiment expectations -- note that here we must canonicalize the - // experiment name otherwise we won't find it into the map + // experiment name otherwise we won't find it into the map when testing non-canonical names expectations := expectationsMap[CanonicalizeExperimentName(tc.experimentName)] if expectations == nil { t.Fatal("no expectations for", tc.experimentName) @@ -678,12 +678,12 @@ func TestNewFactory(t *testing.T) { t.Fatal(tc.experimentName, ": expected", expectations.inputPolicy, "got", v) } - // make sure the interrupted value is the expected one + // make sure the interruptible value is the expected one if v := factory.Interruptible(); v != expectations.interruptible { t.Fatal(tc.experimentName, ": expected", expectations.interruptible, "got", v) } - // make sure we can creare the measurer + // make sure we can create the measurer measurer := factory.NewExperimentMeasurer() if measurer == nil { t.Fatal("expected non-nil measurer, got nil") @@ -724,7 +724,7 @@ func TestNewFactory(t *testing.T) { t.Fatal("expected interruptible to be false") } - // make sure we can creare the measurer + // make sure we can create the measurer measurer := factory.NewExperimentMeasurer() if measurer == nil { t.Fatal("expected non-nil measurer, got nil") From 931cceca7e1f06a59ae2150111fb9216b33aeb26 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 11 Oct 2023 10:57:44 +0200 Subject: [PATCH 8/8] x --- internal/engine/experiment_integration_test.go | 7 +++++++ internal/registry/factory.go | 6 ++++-- internal/registry/factory_test.go | 6 +++--- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/internal/engine/experiment_integration_test.go b/internal/engine/experiment_integration_test.go index 865a45203b..81aeb28e09 100644 --- a/internal/engine/experiment_integration_test.go +++ b/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 { diff --git a/internal/registry/factory.go b/internal/registry/factory.go index 2fde819a7c..b1025bea4c 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -232,7 +232,9 @@ most recent check-in API call did not enable this experiment as well. You can by 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.` -const experimentForceEnableEnvVar = "OONI_FORCE_ENABLE_EXPERIMENT" +// 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, kvStore model.KeyValueStore, logger model.Logger) (*Factory, error) { @@ -273,7 +275,7 @@ func NewFactory(name string, kvStore model.KeyValueStore, logger model.Logger) ( if factory.enabledByDefault { return factory, nil // enabled by default } - if os.Getenv(experimentForceEnableEnvVar) == "1" { + if os.Getenv(OONI_FORCE_ENABLE_EXPERIMENT) == "1" { return factory, nil // enabled by environment variable } if checkincache.ExperimentEnabled(kvStore, name) { diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index a24a142681..f336a2370a 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -624,14 +624,14 @@ func TestNewFactory(t *testing.T) { for _, tc := range allCases { t.Run(tc.description, func(t *testing.T) { // make sure the bypass environment variable is not set - if os.Getenv(experimentForceEnableEnvVar) != "" { + if os.Getenv(OONI_FORCE_ENABLE_EXPERIMENT) != "" { t.Fatal("the OONI_FORCE_ENABLE_EXPERIMENT env variable shouldn't be set") } // if needed, set the environment variable for the scope of the func if tc.setForceEnableExperiment { - os.Setenv(experimentForceEnableEnvVar, "1") - defer os.Unsetenv(experimentForceEnableEnvVar) + os.Setenv(OONI_FORCE_ENABLE_EXPERIMENT, "1") + defer os.Unsetenv(OONI_FORCE_ENABLE_EXPERIMENT) } t.Log("experimentName:", tc.experimentName)