diff --git a/docs/design/dd-008-richer-input.md b/docs/design/dd-008-richer-input.md index 7927d7974..730616747 100644 --- a/docs/design/dd-008-richer-input.md +++ b/docs/design/dd-008-richer-input.md @@ -199,6 +199,7 @@ type ExperimentTarget struct { Category() string // equivalent to OOAPIURLInfo.CategoryCode Country() string // equivalent to OOAPIURLInfo.CountryCode Input() string // equivalent to OOAPIURLInfo.URL + String() string // serializes to the input } type InputLoader = TargetLoader // we renamed to reflect the change of purpose @@ -212,6 +213,10 @@ type Experiment interface { } ``` +The `String` method is used to reduce the `ExperimentTarget` to the input string, which +allows for backwards compatibility. We can obtain a string representation of the target's +input and use it every time where previous we used the `input` string. + Note that we also renamed the `InputLoader` to `TargetLoader` to reflect the fact that we're not loading bare input anymore, rather we're loading richer input targets. @@ -361,14 +366,90 @@ wouldn't be able to cast such an interface to the `*target` type. Therefore, unconditionally casting could lead to crashes when integrating new code and generally makes for a less robust codebase. +## Implementation: add OpenVPN + +Pull request [#1625](https://github.com/ooni/probe-cli/pull/1625) added richer +input support for the `openvpn` experiment. Because this experiment already +supports richer input through the `api.dev.ooni.io` backend, we now have the +first experiment capable of using richer input. + +## Implementation: fix serializing options + +Pull request [#1630](https://github.com/ooni/probe-cli/pull/1630) adds +support for correctly serializing options. We extend the model of a richer +input target to include the following function: + +```Go +type ExperimentTarget struct { + // ... + Options() []string +} +``` + +Then we implement `Options` for every possible experiment target. There is +a default implementation in the `experimentconfig` package implementing the +default semantics that was also available before: + +1. skip fields whose name starts with `Safe`; + +2. only serialize scalar values; + +3. do not serializes any zero value. + +Additionally, we now serialize the options inside the `newMeasurement` +constructor typical of each experiment. + +## Implementation: improve passing options to experiments + +Pull request [#1629](https://github.com/ooni/probe-cli/pull/1629) modifies +the way in which the `./internal/oonirun` package loads data for experiments +such that, when using OONI Run v2, we load its `options` field as a +`json.RawMessage` rather than using a `map[string]any`. This fact is +significant because, previously, we could only unmarshal options provided +by command line, which were always scalar. With this change, instead, we +can keep backwards compatibility with respect to the command line but it's +now also possible for experiments options specified via OONI Run v2 to +provide non-scalar options. + +The key change to enable this is to modify a `*registry.Factory` type to add: + +```Go +type Factory struct { /* ... */ } + +func (fx *Factory) SetOptionsJSON(value json.RawMessage) error +``` + +In this way, we can directly assign the raw JSON to the experiment config +that is kept inside of the `*Factory` itself. + +Additionally, constructing an experiment using `*oonirun.Experiment` now +includes two options related field: + +```Go +type Experiment struct { + InitialOptions json.RawMessage // new addition + ExtraOptions map[string]any // also present before +} +``` + +Initialization of experiment options will work as follows: + +1. the per-experiment `*Factory` constructor initializes fields to their +default value, which, in most cases, SHOULD be the zero value; + +2. we update the config using `InitialOptions` unless it is empty; + +3. we update the config using `ExtraOptions` unless it is empty. + +In practice, the code would always use either `InitialOptions` or +`ExtraOptions`, but we also wanted to specify priority in case both +of them were available. + ## Next steps This is a rough sequence of next steps that we should expand as we implement additional bits of richer input and for which we need reference issues. -* setting `(*Measurement).Options` inside `(*engine.experiment).newMeasurement` -rather than inside the `oonirun` package. - * fully convert `dnscheck`'s static list to live inside `dnscheck` instead of `targetloading` and to use the proper richer input. @@ -378,15 +459,9 @@ rather than inside the `oonirun` package. * implement backend API for serving `stunreachability` richer input. - * implement backend API for serving `openvpn` richer input. - * deliver feature flags using experiment-specific richer input rather than using the check-in API (and maybe keep the caching support?). -* deliver OONI Run v2 options as a `json.RawMessage` rather than passing -through a `map[string]any` such that we can support non-scalar options when -using OONI Run v2. - * try to eliminate `InputPolicy` and instead have each experiment define its own constructor for the proper target loader, and split the implementation inside of the `targetloader` package to have multiple target loaders. diff --git a/internal/engine/experiment.go b/internal/engine/experiment.go index 5f6ac5d87..42a237357 100644 --- a/internal/engine/experiment.go +++ b/internal/engine/experiment.go @@ -46,6 +46,10 @@ func (emr *experimentMutableReport) Get() (report probeservices.ReportChannel) { return } +// TODO(bassosimone,DecFox): it would be nice if `*experiment` depended +// on an interface rather than depending on the concrete session, because +// that will allow us to write tests using mocks much more easily. + // experiment implements [model.Experiment]. type experiment struct { byteCounter *bytecounter.Counter @@ -111,13 +115,10 @@ func (e *experiment) SubmitAndUpdateMeasurementContext( // newMeasurement creates a new measurement for this experiment with the given input. func (e *experiment) newMeasurement(target model.ExperimentTarget) *model.Measurement { utctimenow := time.Now().UTC() - // TODO(bassosimone,DecFox): move here code that supports filling the options field - // when there is richer input, which currently is inside ./internal/oonirun. - // - // We MUST do this because the current solution only works for OONI Run and when - // there are command line options but does not work for API/static targets. + m := &model.Measurement{ DataFormatVersion: model.OOAPIReportDefaultDataFormatVersion, + Options: target.Options(), Input: model.MeasurementInput(target.Input()), MeasurementStartTime: utctimenow.Format(model.MeasurementDateFormat), MeasurementStartTimeSaved: utctimenow, @@ -135,6 +136,7 @@ func (e *experiment) newMeasurement(target model.ExperimentTarget) *model.Measur TestStartTime: e.testStartTime, TestVersion: e.testVersion, } + m.AddAnnotation("architecture", runtime.GOARCH) m.AddAnnotation("engine_name", "ooniprobe-engine") m.AddAnnotation("engine_version", version.Version) @@ -144,6 +146,7 @@ func (e *experiment) newMeasurement(target model.ExperimentTarget) *model.Measur m.AddAnnotation("vcs_revision", runtimex.BuildInfo.VcsRevision) m.AddAnnotation("vcs_time", runtimex.BuildInfo.VcsTime) m.AddAnnotation("vcs_tool", runtimex.BuildInfo.VcsTool) + return m } diff --git a/internal/engine/experiment_test.go b/internal/engine/experiment_test.go index ceab79d7b..c9be288df 100644 --- a/internal/engine/experiment_test.go +++ b/internal/engine/experiment_test.go @@ -1,9 +1,14 @@ package engine import ( + "sync" "testing" + "time" + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/bytecounter" "github.com/ooni/probe-cli/v3/internal/enginelocate" + "github.com/ooni/probe-cli/v3/internal/experiment/dnscheck" "github.com/ooni/probe-cli/v3/internal/experiment/example" "github.com/ooni/probe-cli/v3/internal/experiment/signal" "github.com/ooni/probe-cli/v3/internal/model" @@ -105,3 +110,84 @@ func TestExperimentMeasurementSummaryKeys(t *testing.T) { } }) } + +// This test ensures that (*experiment).newMeasurement is working as intended. +func TestExperimentNewMeasurement(t *testing.T) { + // create a session for testing that does not use the network at all + sess := newSessionForTestingNoLookups(t) + + // create a conventional time for starting the experiment + t0 := time.Date(2024, 6, 27, 10, 33, 0, 0, time.UTC) + + // create the experiment + exp := &experiment{ + byteCounter: bytecounter.New(), + callbacks: model.NewPrinterCallbacks(model.DiscardLogger), + measurer: &dnscheck.Measurer{}, + mrep: &experimentMutableReport{ + mu: sync.Mutex{}, + report: nil, + }, + session: sess, + testName: "dnscheck", + testStartTime: t0.Format(model.MeasurementDateFormat), + testVersion: "0.1.0", + } + + // create the richer input target + target := &dnscheck.Target{ + Config: &dnscheck.Config{ + DefaultAddrs: "8.8.8.8 2001:4860:4860::8888", + HTTP3Enabled: true, + }, + URL: "https://dns.google/dns-query", + } + + // create measurement + meas := exp.newMeasurement(target) + + // make sure the input is correctly serialized + t.Run("Input", func(t *testing.T) { + if meas.Input != "https://dns.google/dns-query" { + t.Fatal("unexpected meas.Input") + } + }) + + // make sure the options are correctly serialized + t.Run("Options", func(t *testing.T) { + expectOptions := []string{`DefaultAddrs=8.8.8.8 2001:4860:4860::8888`, `HTTP3Enabled=true`} + if diff := cmp.Diff(expectOptions, meas.Options); diff != "" { + t.Fatal(diff) + } + }) + + // make sure we've got the expected annotation keys + t.Run("Annotations", func(t *testing.T) { + const ( + expected = 1 << iota + got + ) + m := map[string]int{ + "architecture": expected, + "engine_name": expected, + "engine_version": expected, + "go_version": expected, + "platform": expected, + "vcs_modified": expected, + "vcs_revision": expected, + "vcs_time": expected, + "vcs_tool": expected, + } + for key := range meas.Annotations { + m[key] |= got + } + for key, value := range m { + if value != expected|got { + t.Fatal("expected", expected|got, "for", key, "got", value) + } + } + }) + + // TODO(bassosimone,DecFox): this is the correct place where to + // add more tests regarding how we create measurements. +} diff --git a/internal/experiment/dnscheck/dnscheck.go b/internal/experiment/dnscheck/dnscheck.go index b5c1583b2..815ab2511 100644 --- a/internal/experiment/dnscheck/dnscheck.go +++ b/internal/experiment/dnscheck/dnscheck.go @@ -134,7 +134,7 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { if !ok { return ErrInvalidInputType } - config, input := target.Options, target.URL + config, input := target.Config, target.URL sess.Logger().Infof("dnscheck: using richer input: %+v %+v", config, input) // 1. fill the measurement with test keys diff --git a/internal/experiment/dnscheck/dnscheck_test.go b/internal/experiment/dnscheck/dnscheck_test.go index bf8f89555..c7055930c 100644 --- a/internal/experiment/dnscheck/dnscheck_test.go +++ b/internal/experiment/dnscheck/dnscheck_test.go @@ -72,7 +72,7 @@ func TestDNSCheckFailsWithoutInput(t *testing.T) { Session: newsession(), Target: &Target{ URL: "", // explicitly empty - Options: &Config{ + Config: &Config{ Domain: "example.com", }, }, @@ -90,8 +90,8 @@ func TestDNSCheckFailsWithInvalidURL(t *testing.T) { Measurement: &model.Measurement{Input: "Not a valid URL \x7f"}, Session: newsession(), Target: &Target{ - URL: "Not a valid URL \x7f", - Options: &Config{}, + URL: "Not a valid URL \x7f", + Config: &Config{}, }, } err := measurer.Run(context.Background(), args) @@ -107,8 +107,8 @@ func TestDNSCheckFailsWithUnsupportedProtocol(t *testing.T) { Measurement: &model.Measurement{Input: "file://1.1.1.1"}, Session: newsession(), Target: &Target{ - URL: "file://1.1.1.1", - Options: &Config{}, + URL: "file://1.1.1.1", + Config: &Config{}, }, } err := measurer.Run(context.Background(), args) @@ -128,7 +128,7 @@ func TestWithCancelledContext(t *testing.T) { Session: newsession(), Target: &Target{ URL: "dot://one.one.one.one", - Options: &Config{ + Config: &Config{ DefaultAddrs: "1.1.1.1 1.0.0.1", }, }, @@ -191,7 +191,7 @@ func TestDNSCheckValid(t *testing.T) { Session: newsession(), Target: &Target{ URL: "dot://one.one.one.one:853", - Options: &Config{ + Config: &Config{ DefaultAddrs: "1.1.1.1 1.0.0.1", }, }, @@ -239,8 +239,8 @@ func TestDNSCheckWait(t *testing.T) { Measurement: &measurement, Session: newsession(), Target: &Target{ - URL: input, - Options: &Config{}, + URL: input, + Config: &Config{}, }, } err := measurer.Run(context.Background(), args) diff --git a/internal/experiment/dnscheck/richerinput.go b/internal/experiment/dnscheck/richerinput.go index 4d4a7ce52..f42d077e2 100644 --- a/internal/experiment/dnscheck/richerinput.go +++ b/internal/experiment/dnscheck/richerinput.go @@ -3,6 +3,7 @@ package dnscheck import ( "context" + "github.com/ooni/probe-cli/v3/internal/experimentconfig" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/reflectx" "github.com/ooni/probe-cli/v3/internal/targetloading" @@ -10,8 +11,8 @@ import ( // Target is a richer-input target that this experiment should measure. type Target struct { - // Options contains the configuration. - Options *Config + // Config contains the configuration. + Config *Config // URL is the input URL. URL string @@ -34,6 +35,11 @@ func (t *Target) Input() string { return t.URL } +// Options implements [model.ExperimentTarget]. +func (t *Target) Options() []string { + return experimentconfig.DefaultOptionsSerializer(t.Config) +} + // String implements [model.ExperimentTarget]. func (t *Target) String() string { return t.URL @@ -83,8 +89,8 @@ func (tl *targetLoader) Load(ctx context.Context) ([]model.ExperimentTarget, err var targets []model.ExperimentTarget for _, input := range inputs { targets = append(targets, &Target{ - Options: tl.options, - URL: input, + Config: tl.options, + URL: input, }) } return targets, nil @@ -100,14 +106,14 @@ var defaultInput = []model.ExperimentTarget{ // &Target{ URL: "https://dns.google/dns-query", - Options: &Config{ + Config: &Config{ HTTP3Enabled: true, DefaultAddrs: "8.8.8.8 8.8.4.4", }, }, &Target{ URL: "https://dns.google/dns-query", - Options: &Config{ + Config: &Config{ DefaultAddrs: "8.8.8.8 8.8.4.4", }, }, diff --git a/internal/experiment/dnscheck/richerinput_test.go b/internal/experiment/dnscheck/richerinput_test.go index 2d9e5dd53..46f361cf9 100644 --- a/internal/experiment/dnscheck/richerinput_test.go +++ b/internal/experiment/dnscheck/richerinput_test.go @@ -16,7 +16,7 @@ import ( func TestTarget(t *testing.T) { target := &Target{ URL: "https://dns.google/dns-query", - Options: &Config{ + Config: &Config{ DefaultAddrs: "8.8.8.8 8.8.4.4", Domain: "example.com", HTTP3Enabled: false, @@ -44,6 +44,19 @@ func TestTarget(t *testing.T) { } }) + t.Run("Options", func(t *testing.T) { + expect := []string{ + "DefaultAddrs=8.8.8.8 8.8.4.4", + "Domain=example.com", + "HTTPHost=dns.google", + "TLSServerName=dns.google.com", + "TLSVersion=TLSv1.3", + } + if diff := cmp.Diff(expect, target.Options()); diff != "" { + t.Fatal(diff) + } + }) + t.Run("String", func(t *testing.T) { if target.String() != "https://dns.google/dns-query" { t.Fatal("invalid String") @@ -79,14 +92,14 @@ func TestNewLoader(t *testing.T) { var testDefaultInput = []model.ExperimentTarget{ &Target{ URL: "https://dns.google/dns-query", - Options: &Config{ + Config: &Config{ HTTP3Enabled: true, DefaultAddrs: "8.8.8.8 8.8.4.4", }, }, &Target{ URL: "https://dns.google/dns-query", - Options: &Config{ + Config: &Config{ DefaultAddrs: "8.8.8.8 8.8.4.4", }, }, @@ -136,25 +149,25 @@ func TestTargetLoaderLoad(t *testing.T) { expectTargets: []model.ExperimentTarget{ &Target{ URL: "https://dns.cloudflare.com/dns-query", - Options: &Config{ + Config: &Config{ DefaultAddrs: "1.1.1.1 1.0.0.1", }, }, &Target{ URL: "https://one.one.one.one/dns-query", - Options: &Config{ + Config: &Config{ DefaultAddrs: "1.1.1.1 1.0.0.1", }, }, &Target{ URL: "https://1dot1dot1dot1dot.com/dns-query", - Options: &Config{ + Config: &Config{ DefaultAddrs: "1.1.1.1 1.0.0.1", }, }, &Target{ URL: "https://dns.cloudflare/dns-query", - Options: &Config{ + Config: &Config{ DefaultAddrs: "1.1.1.1 1.0.0.1", }, }, @@ -202,12 +215,12 @@ func TestTargetLoaderLoad(t *testing.T) { expectErr: nil, expectTargets: []model.ExperimentTarget{ &Target{ - URL: "https://dns.cloudflare.com/dns-query", - Options: &Config{}, + URL: "https://dns.cloudflare.com/dns-query", + Config: &Config{}, }, &Target{ - URL: "https://one.one.one.one/dns-query", - Options: &Config{}, + URL: "https://one.one.one.one/dns-query", + Config: &Config{}, }, }, }, @@ -229,12 +242,12 @@ func TestTargetLoaderLoad(t *testing.T) { expectErr: nil, expectTargets: []model.ExperimentTarget{ &Target{ - URL: "https://1dot1dot1dot1dot.com/dns-query", - Options: &Config{}, + URL: "https://1dot1dot1dot1dot.com/dns-query", + Config: &Config{}, }, &Target{ - URL: "https://dns.cloudflare/dns-query", - Options: &Config{}, + URL: "https://dns.cloudflare/dns-query", + Config: &Config{}, }, }, }, diff --git a/internal/experiment/openvpn/openvpn.go b/internal/experiment/openvpn/openvpn.go index 0cf8255f4..23e904a9e 100644 --- a/internal/experiment/openvpn/openvpn.go +++ b/internal/experiment/openvpn/openvpn.go @@ -227,7 +227,7 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { if !ok { return targetloading.ErrInvalidInputType } - config, input := target.Options, target.URL + config, input := target.Config, target.URL // 2. obtain the endpoint representation from the input URL endpoint, err := newEndpointFromInputString(input) diff --git a/internal/experiment/openvpn/openvpn_test.go b/internal/experiment/openvpn/openvpn_test.go index e45b52ae7..f050fab3a 100644 --- a/internal/experiment/openvpn/openvpn_test.go +++ b/internal/experiment/openvpn/openvpn_test.go @@ -202,8 +202,8 @@ func TestBadTargetURLFailure(t *testing.T) { Measurement: measurement, Session: sess, Target: &openvpn.Target{ - URL: "openvpn://badprovider/?address=aa", - Options: &openvpn.Config{}, + URL: "openvpn://badprovider/?address=aa", + Config: &openvpn.Config{}, }, } err := m.Run(ctx, args) @@ -260,8 +260,8 @@ func TestSuccess(t *testing.T) { Measurement: measurement, Session: sess, Target: &openvpn.Target{ - URL: "openvpn://riseupvpn.corp/?address=127.0.0.1:9989&transport=tcp", - Options: &openvpn.Config{}, + URL: "openvpn://riseupvpn.corp/?address=127.0.0.1:9989&transport=tcp", + Config: &openvpn.Config{}, }, } err := m.Run(ctx, args) diff --git a/internal/experiment/openvpn/richerinput.go b/internal/experiment/openvpn/richerinput.go index 050673b35..5743865e9 100644 --- a/internal/experiment/openvpn/richerinput.go +++ b/internal/experiment/openvpn/richerinput.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/ooni/probe-cli/v3/internal/experimentconfig" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/reflectx" "github.com/ooni/probe-cli/v3/internal/targetloading" @@ -23,8 +24,8 @@ var providerAuthentication = map[string]AuthMethod{ // Target is a richer-input target that this experiment should measure. type Target struct { - // Options contains the configuration. - Options *Config + // Config contains the configuration. + Config *Config // URL is the input URL. URL string @@ -47,6 +48,11 @@ func (t *Target) Input() string { return t.URL } +// Options implements [model.ExperimentTarget]. +func (t *Target) Options() (options []string) { + return experimentconfig.DefaultOptionsSerializer(t.Config) +} + // String implements [model.ExperimentTarget]. func (t *Target) String() string { return t.URL @@ -96,8 +102,8 @@ func (tl *targetLoader) Load(ctx context.Context) ([]model.ExperimentTarget, err var targets []model.ExperimentTarget for _, input := range inputs { targets = append(targets, &Target{ - Options: tl.options, - URL: input, + Config: tl.options, + URL: input, }) } return targets, nil @@ -140,8 +146,8 @@ func (tl *targetLoader) loadFromBackend(ctx context.Context) ([]model.Experiment // TODO(ainghazal): implement (surfshark, etc) } targets = append(targets, &Target{ - URL: input, - Options: config, + URL: input, + Config: config, }) } diff --git a/internal/experiment/openvpn/richerinput_test.go b/internal/experiment/openvpn/richerinput_test.go index 4c0469663..6c8097f1a 100644 --- a/internal/experiment/openvpn/richerinput_test.go +++ b/internal/experiment/openvpn/richerinput_test.go @@ -16,7 +16,7 @@ import ( func TestTarget(t *testing.T) { target := &Target{ URL: "openvpn://unknown.corp?address=1.1.1.1%3A443&transport=udp", - Options: &Config{ + Config: &Config{ Auth: "SHA512", Cipher: "AES-256-GCM", Provider: "unknown", @@ -44,6 +44,17 @@ func TestTarget(t *testing.T) { } }) + t.Run("Options", func(t *testing.T) { + expect := []string{ + "Auth=SHA512", + "Cipher=AES-256-GCM", + "Provider=unknown", + } + if diff := cmp.Diff(expect, target.Options()); diff != "" { + t.Fatal(diff) + } + }) + t.Run("String", func(t *testing.T) { if target.String() != "openvpn://unknown.corp?address=1.1.1.1%3A443&transport=udp" { t.Fatal("invalid String") @@ -112,7 +123,7 @@ func TestTargetLoaderLoad(t *testing.T) { expectTargets: []model.ExperimentTarget{ &Target{ URL: "openvpn://unknown.corp/1.1.1.1", - Options: &Config{ + Config: &Config{ Provider: "unknown", SafeCA: "aa", SafeCert: "bb", diff --git a/internal/experimentconfig/experimentconfig.go b/internal/experimentconfig/experimentconfig.go new file mode 100644 index 000000000..58932b0e9 --- /dev/null +++ b/internal/experimentconfig/experimentconfig.go @@ -0,0 +1,84 @@ +// Package experimentconfig contains code to manage experiments configuration. +package experimentconfig + +import ( + "fmt" + "reflect" + "strings" +) + +// TODO(bassosimone): we should probably move here all the code inside +// of registry used to serialize existing options and to set values from +// generic map[string]any types. + +// DefaultOptionsSerializer serializes options for [model.ExperimentTarget] +// honouring its Options method contract: +// +// 1. we do not serialize options whose name starts with "Safe"; +// +// 2. we only serialize scalar values; +// +// 3. we never serialize any zero values. +// +// This method MUST be passed a pointer to a struct. Otherwise, the return +// value will be a zero-length list (either nil or empty). +func DefaultOptionsSerializer(config any) (options []string) { + // as documented, this method MUST be passed a struct pointer + // + // Implementation note: the .Elem method converts a nil + // pointer to a zero-value pointee type. + stval := reflect.ValueOf(config) + if stval.Kind() != reflect.Pointer { + return + } + stval = stval.Elem() + if stval.Kind() != reflect.Struct { + return + } + + // obtain the structure type + stt := stval.Type() + + // cycle through the struct fields + for idx := 0; idx < stval.NumField(); idx++ { + // obtain the field type and value + fieldval, fieldtype := stval.Field(idx), stt.Field(idx) + + // make sure the field is public + if !fieldtype.IsExported() { + continue + } + + // make sure the field name does not start with "Safe" + if strings.HasPrefix(fieldtype.Name, "Safe") { + continue + } + + // add the field iff it's a nonzero scalar + switch fieldval.Kind() { + case reflect.Bool, + reflect.Int, + reflect.Int8, + reflect.Int16, + reflect.Int32, + reflect.Int64, + reflect.Uint, + reflect.Uint8, + reflect.Uint16, + reflect.Uint32, + reflect.Uint64, + reflect.Float32, + reflect.Float64, + reflect.String: + if fieldval.IsZero() { + continue + } + options = append(options, fmt.Sprintf("%s=%v", fieldtype.Name, fieldval.Interface())) + + default: + // nothing + } + } + + return +} diff --git a/internal/experimentconfig/experimentconfig_test.go b/internal/experimentconfig/experimentconfig_test.go new file mode 100644 index 000000000..ad5090bcb --- /dev/null +++ b/internal/experimentconfig/experimentconfig_test.go @@ -0,0 +1,175 @@ +package experimentconfig + +import ( + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestDefaultOptionsSerializer(t *testing.T) { + // configuration is the configuration we're testing the serialization of. + // + // Note that there's no `ooni:"..."` annotation here because we have changed + // our model in https://github.com/ooni/probe-cli/pull/1629, and now this kind + // of annotations are only command-line related. + type configuration struct { + // booleans + ValBool bool + + // integers + ValInt int + ValInt8 int8 + ValInt16 int16 + ValInt32 int32 + ValInt64 int64 + + // unsigned integers + ValUint uint + ValUint8 uint8 + ValUint16 uint16 + ValUint32 uint32 + ValUint64 uint64 + + // floats + ValFloat32 float32 + ValFloat64 float64 + + // strings + ValString string + + // unexported fields we should ignore + privateInt int + privateString string + privateList []int16 + + // safe fields we should ignore + SafeBool bool + SafeInt int + SafeString string + + // non-scalar fields we should ignore + NSList []int64 + NSMap map[string]string + } + + // testcase is a test case implemented by this function + type testcase struct { + // name is the test case name + name string + + // config is the config to transform into a list of options + config any + + // expectConfigType is an extra check to make sure we're actually + // passing the correct type for the config, which is here to ensure + // that, with a nil pointer to struct, we're not crashing. We need + // some extra case here because of how the Go type system work, + // and specifically we want to be sure we're passing an any containing + // a tuple like (type=*configuration,value=nil). + // + // See https://codefibershq.com/blog/golang-why-nil-is-not-always-nil + expectConfigType string + + // expect is the expected result + expect []string + } + + cases := []testcase{ + { + name: "we return a nil list for zero values", + expectConfigType: "*experimentconfig.configuration", + config: &configuration{}, + expect: nil, + }, + + { + name: "we return a nil list for non-pointers", + expectConfigType: "experimentconfig.configuration", + config: configuration{}, + expect: nil, + }, + + { + name: "we return a nil list for non-struct pointers", + expectConfigType: "*int64", + config: func() *int64 { + v := int64(12345) + return &v + }(), + expect: nil, + }, + + { + name: "we return a nil list for a nil struct pointer", + expectConfigType: "*experimentconfig.configuration", + config: func() *configuration { + return (*configuration)(nil) + }(), + expect: nil, + }, + + { + name: "we only serialize the fields that should be exported", + expectConfigType: "*experimentconfig.configuration", + config: &configuration{ + ValBool: true, + ValInt: 1, + ValInt8: 2, + ValInt16: 3, + ValInt32: 4, + ValInt64: 5, + ValUint: 6, + ValUint8: 7, + ValUint16: 8, + ValUint32: 9, + ValUint64: 10, + ValFloat32: 11, + ValFloat64: 12, + ValString: "tredici", + privateInt: 14, + privateString: "quindici", + privateList: []int16{16}, + SafeBool: true, + SafeInt: 18, + SafeString: "diciannove", + NSList: []int64{20}, + NSMap: map[string]string{"21": "22"}, + }, + expect: []string{ + "ValBool=true", + "ValInt=1", + "ValInt8=2", + "ValInt16=3", + "ValInt32=4", + "ValInt64=5", + "ValUint=6", + "ValUint8=7", + "ValUint16=8", + "ValUint32=9", + "ValUint64=10", + "ValFloat32=11", + "ValFloat64=12", + "ValString=tredici", + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // first make sure that tc.config has really the expected + // type for the reason explained in its docstring + if actual := fmt.Sprintf("%T", tc.config); actual != tc.expectConfigType { + t.Fatal("expected", tc.expectConfigType, "got", actual) + } + + // then serialize the content of the config to a list of strings + got := DefaultOptionsSerializer(tc.config) + + // finally, make sure that the result matches expectations + if diff := cmp.Diff(tc.expect, got); diff != "" { + t.Fatal(diff) + } + }) + } +} diff --git a/internal/model/experiment.go b/internal/model/experiment.go index b514df3de..31b80f01f 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -99,6 +99,17 @@ type ExperimentTarget interface { // Input returns the experiment input, which is typically a URL. Input() string + // Options transforms the options contained by this target + // into a []string containing options as they were provided + // using the command line `-O option=value` syntax. + // + // This method MUST NOT serialize all the options whose name + // starts with the "Safe" prefix. This method MUST skip serializing + // sensitive options, non-scalar options, and zero value options. + // + // Consider using the [experimentconfig] package to serialize. + Options() []string + // String MUST return the experiment input. // // Implementation note: previously existing code often times treated diff --git a/internal/model/ooapi.go b/internal/model/ooapi.go index 3750cece1..8a0449568 100644 --- a/internal/model/ooapi.go +++ b/internal/model/ooapi.go @@ -224,6 +224,21 @@ func (o *OOAPIURLInfo) Input() string { return o.URL } +// Options implements ExperimentTarget. +func (o *OOAPIURLInfo) Options() []string { + // Implementation note: we're not serializing any options for now. If/when + // we do that, remember the Options contract: + // + // 1. skip options whose name begins with "Safe"; + // + // 2. skip options that are not scalars; + // + // 3. avoid serializing zero values. + // + // Consider using the [experimentconfig] package to serialize. + return nil +} + // String implements [ExperimentTarget]. func (o *OOAPIURLInfo) String() string { return o.URL diff --git a/internal/model/ooapi_test.go b/internal/model/ooapi_test.go index aee509e85..a02958183 100644 --- a/internal/model/ooapi_test.go +++ b/internal/model/ooapi_test.go @@ -128,6 +128,10 @@ func TestOOAPIURLInfo(t *testing.T) { t.Fatal("invalid Input") } + if info.Options() != nil { + t.Fatal("invalid Options") + } + if info.String() != "https://www.facebook.com/" { t.Fatal("invalid String") } diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index 1fe1c7079..0435b21d1 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -7,9 +7,7 @@ package oonirun import ( "context" "encoding/json" - "fmt" "math/rand" - "strings" "sync/atomic" "time" @@ -92,13 +90,6 @@ func (ed *Experiment) Run(ctx context.Context) error { return err } - // TODO(bassosimone): we need another patch after the current one - // to correctly serialize the options as configured using InitialOptions - // and ExtraOptions otherwise the Measurement.Options field turns out - // to always be empty and this is highly suboptimal for us. - // - // The next patch is https://github.com/ooni/probe-cli/pull/1630. - // 2. configure experiment's options // // This MUST happen before loading targets because the options will @@ -116,7 +107,7 @@ func (ed *Experiment) Run(ctx context.Context) error { // 4. randomize input, if needed if ed.Random { - // Note: since go1.20 the default random generated is random seeded + // Note: since go1.20 the default random generator is randomly seeded // // See https://tip.golang.org/doc/go1.20 rand.Shuffle(len(targetList), func(i, j int) { @@ -182,7 +173,6 @@ func (ed *Experiment) newInputProcessor(experiment model.Experiment, }, Inputs: inputList, MaxRuntime: time.Duration(ed.MaxRuntime) * time.Second, - Options: experimentOptionsToStringList(ed.ExtraOptions), Saver: NewInputProcessorSaverWrapper(saver), Submitter: &experimentSubmitterWrapper{ child: NewInputProcessorSubmitterWrapper(submitter), @@ -243,22 +233,6 @@ func (ed *Experiment) newTargetLoader(builder model.ExperimentBuilder) targetLoa }) } -// experimentOptionsToStringList convers the options to []string, which is -// the format with which we include them into a OONI Measurement. The resulting -// []string will skip any option that is named with a `Safe` prefix (case -// sensitive). -func experimentOptionsToStringList(options map[string]any) (out []string) { - // the prefix to skip inclusion in the string list - safeOptionPrefix := "Safe" - for key, value := range options { - if strings.HasPrefix(key, safeOptionPrefix) { - continue - } - out = append(out, fmt.Sprintf("%s=%v", key, value)) - } - return -} - // experimentWrapper wraps an experiment and logs progress type experimentWrapper struct { // child is the child experiment wrapper diff --git a/internal/oonirun/experiment_test.go b/internal/oonirun/experiment_test.go index cbe24d803..c840d7a17 100644 --- a/internal/oonirun/experiment_test.go +++ b/internal/oonirun/experiment_test.go @@ -4,8 +4,6 @@ import ( "context" "encoding/json" "errors" - "reflect" - "sort" "testing" "time" @@ -189,48 +187,6 @@ func TestExperimentSetOptions(t *testing.T) { } } -func Test_experimentOptionsToStringList(t *testing.T) { - type args struct { - options map[string]any - } - tests := []struct { - name string - args args - wantOut []string - }{ - { - name: "happy path: a map with three entries returns three items", - args: args{ - map[string]any{ - "foo": 1, - "bar": 2, - "baaz": 3, - }, - }, - wantOut: []string{"baaz=3", "bar=2", "foo=1"}, - }, - { - name: "an option beginning with `Safe` is skipped from the output", - args: args{ - map[string]any{ - "foo": 1, - "Safefoo": 42, - }, - }, - wantOut: []string{"foo=1"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - gotOut := experimentOptionsToStringList(tt.args.options) - sort.Strings(gotOut) - if !reflect.DeepEqual(gotOut, tt.wantOut) { - t.Errorf("experimentOptionsToStringList() = %v, want %v", gotOut, tt.wantOut) - } - }) - } -} - func TestExperimentRun(t *testing.T) { errMocked := errors.New("mocked error") type fields struct { diff --git a/internal/oonirun/inputprocessor.go b/internal/oonirun/inputprocessor.go index d4e292fd6..57b4f4489 100644 --- a/internal/oonirun/inputprocessor.go +++ b/internal/oonirun/inputprocessor.go @@ -55,9 +55,6 @@ type InputProcessor struct { // there will be no MaxRuntime limit. MaxRuntime time.Duration - // Options contains command line options for this experiment. - Options []string - // Saver is the code that will save measurement results // on persistent storage (e.g. the file system). Saver InputProcessorSaverWrapper @@ -144,9 +141,13 @@ func (ip *InputProcessor) run(ctx context.Context) (int, error) { return 0, err } meas.AddAnnotations(ip.Annotations) - meas.Options = ip.Options err = ip.Submitter.Submit(ctx, idx, meas) if err != nil { + // TODO(bassosimone): when re-reading this code, I find it confusing that + // we return on error because I am always like "wait, this is not the right + // thing to do here". Then, I remember that the experimentSubmitterWrapper{} + // ignores this error and so it's like it does not exist. Maybe we should + // rewrite the code to do the right thing here 😬😬😬. return 0, err } // Note: must be after submission because submission modifies diff --git a/internal/oonirun/inputprocessor_test.go b/internal/oonirun/inputprocessor_test.go index a427b5126..1c8ee6ca6 100644 --- a/internal/oonirun/inputprocessor_test.go +++ b/internal/oonirun/inputprocessor_test.go @@ -71,7 +71,6 @@ func TestInputProcessorSubmissionFailed(t *testing.T) { Inputs: []model.ExperimentTarget{ model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.kernel.org/"), }, - Options: []string{"fake=true"}, Submitter: NewInputProcessorSubmitterWrapper( &FakeInputProcessorSubmitter{Err: expected}, ), @@ -96,9 +95,6 @@ func TestInputProcessorSubmissionFailed(t *testing.T) { if m.Annotations["antani"] != "antani" { t.Fatal("invalid annotation: antani") } - if len(m.Options) != 1 || m.Options[0] != "fake=true" { - t.Fatal("options not set") - } } type FakeInputProcessorSaver struct { @@ -120,7 +116,6 @@ func TestInputProcessorSaveOnDiskFailed(t *testing.T) { Inputs: []model.ExperimentTarget{ model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.kernel.org/"), }, - Options: []string{"fake=true"}, Saver: NewInputProcessorSaverWrapper( &FakeInputProcessorSaver{Err: expected}, ), @@ -144,7 +139,6 @@ func TestInputProcessorGood(t *testing.T) { model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.kernel.org/"), model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.slashdot.org/"), }, - Options: []string{"fake=true"}, Saver: NewInputProcessorSaverWrapper(saver), Submitter: NewInputProcessorSubmitterWrapper(submitter), } @@ -186,7 +180,6 @@ func TestInputProcessorMaxRuntime(t *testing.T) { model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.slashdot.org/"), }, MaxRuntime: 1 * time.Nanosecond, - Options: []string{"fake=true"}, Saver: NewInputProcessorSaverWrapper(saver), Submitter: NewInputProcessorSubmitterWrapper(submitter), }