diff --git a/internal/engine/experiment.go b/internal/engine/experiment.go index 5f6ac5d87..5061ec966 100644 --- a/internal/engine/experiment.go +++ b/internal/engine/experiment.go @@ -111,13 +111,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 +132,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 +142,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/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..7d5b70ed5 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, @@ -79,14 +79,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 +136,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 +202,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 +229,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..3d4d74ac3 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", @@ -112,7 +112,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..503378142 --- /dev/null +++ b/internal/experimentconfig/experimentconfig.go @@ -0,0 +1,76 @@ +// 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 do not serialize scalar 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 + 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 file name does not start with Safe + if strings.HasPrefix(fieldtype.Name, "Safe") { + continue + } + + // add the field iff it's a 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: + options = append(options, fmt.Sprintf("%s=%s", fieldtype.Name, fieldval.Interface())) + + default: + // nothing + } + } + + return +} diff --git a/internal/model/experiment.go b/internal/model/experiment.go index ad4e0dfcf..666718c43 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -99,6 +99,18 @@ 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 MAY skip serializing + // sensitive options and options we cannot serialize into a list + // of strings (e.g., objects and lists). + // + // 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..51cc49af5 100644 --- a/internal/model/ooapi.go +++ b/internal/model/ooapi.go @@ -224,6 +224,19 @@ 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. + // + // Consider using the [experimentconfig] package to serialize. + return nil +} + // String implements [ExperimentTarget]. func (o *OOAPIURLInfo) String() string { return o.URL diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index a66093524..505c943fc 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -92,11 +92,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. - // 2. configure experiment's options // // We first unmarshal the InitialOptions into the experiment @@ -121,7 +116,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) { @@ -177,7 +172,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), diff --git a/internal/oonirun/inputprocessor.go b/internal/oonirun/inputprocessor.go index d4e292fd6..79ef4380e 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,10 @@ 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): the default submitter used by + // miniooni ignores errors. Should we make it the default? 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..b6f97f453 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}, ), @@ -120,7 +119,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 +142,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 +183,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), }