feat: correctly set options from richer input

ooni · Jun 26, 2024 · ca1cdc7 · ca1cdc7
1 parent 4f183bd
commit ca1cdc7
Show file tree

Hide file tree

Showing 15 changed files with 164 additions and 64 deletions.
diff --git 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
@@ -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
@@ -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
@@ -3,15 +3,16 @@ 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"
 )
 
 // 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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",