fix(oonimkall): use richer-input-aware target loader

docs/design/dd-008-richer-input.md

@@ -445,6 +445,106 @@ 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.
 
+## Implementation: oonimkall changes
+
+In [#1620](https://github.com/ooni/probe-cli/pull/1620), we started to
+modify the `./pkg/oonimkall` package to support richer input.
+
+Before this diff, the code was not using a loader for loading targets
+for experiments, and the code roughly looked like this:
+
+```Go
+switch builder.InputPolicy() {
+	case model.InputOrQueryBackend, model.InputStrictlyRequired:
+		if len(r.settings.Inputs) <= 0 {
+			r.emitter.EmitFailureStartup("no input provided")
+			return
+		}
+
+	case model.InputOrStaticDefault:
+		if len(r.settings.Inputs) <= 0 {
+			inputs, err := targetloading.StaticBareInputForExperiment(r.settings.Name)
+			if err != nil {
+				r.emitter.EmitFailureStartup("no default static input for this experiment")
+				return
+			}
+			r.settings.Inputs = inputs
+		}
+
+	case model.InputOptional:
+		if len(r.settings.Inputs) <= 0 {
+			r.settings.Inputs = append(r.settings.Inputs, "")
+		}
+
+	default: // treat this case as engine.InputNone.
+		if len(r.settings.Inputs) > 0 {
+			r.emitter.EmitFailureStartup("experiment does not accept input")
+			return
+		}
+		r.settings.Inputs = append(r.settings.Inputs, "")
+}
+```
+
+Basically, we were switching on the experiment builder's `InputPolicy` and
+checking whether input was present or absent according to policy. But, we were
+not *actually* loading input when needed.
+
+To support richer input for experiments such as `openvpn`, instead, we must
+use a loader and fetch such input, as follows:
+
+```Go
+loader := builder.NewTargetLoader(&model.ExperimentTargetLoaderConfig{
+	CheckInConfig: &model.OOAPICheckInConfig{ /* not needed for now */ },
+	Session:      sess,
+	StaticInputs: r.settings.Inputs,
+	SourceFiles:  []string{},
+})
+loadCtx, loadCancel := context.WithTimeout(rootCtx, 30*time.Second)
+defer loadCancel()
+targets, err := loader.Load(loadCtx)
+if err != nil {
+	r.emitter.EmitFailureStartup(err.Error())
+	return
+}
+```
+
+After this change, we still assume the mobile app is providing us with
+inputs for Web Connectivity. Because the loader honours user-provided inputs,
+there's no functional change with the previous behavior. However, if there
+is no input, we're going to load it using the proper mechanisms, including
+using the correct backend API for the `openvpn` experiment.
+
+Also, to pave the way for supporting loading for Web Connectivity as well, we
+need to supply the information required to populate the URLs table as part
+of the `status.measurement_start` event, as follows:
+
+```diff
+ type eventMeasurementGeneric struct {
++	CategoryCode string `json:"category_code,omitempty"`
++	CountryCode  string `json:"country_code,omitempty"`
+	Failure      string `json:"failure,omitempty"`
+	Idx          int64  `json:"idx"`
+	Input        string `json:"input"`
+	JSONStr      string `json:"json_str,omitempty"`
+ }
+
+
+ r.emitter.Emit(eventTypeStatusMeasurementStart, eventMeasurementGeneric{
++	CategoryCode: target.Category(),
++	CountryCode:  target.Country(),
+ 	Idx:          int64(idx),
+ 	Input:        target.Input(),
+ })
+```
+
+By providing the `CategoryCode` and the `CountryCode`, the mobile app is now
+able to correctly populate the URLs table ahead of measuring.
+
+Future work will address passing the correct check-in options to the
+experiment runner, so that we can actually remove the mobile app source
+code that invokes the check-in API, and simplify both the codebase of
+the mobile app and the one of `./pkg/oonimkall`.
+
 ## Next steps
 
 This is a rough sequence of next steps that we should expand as we implement
@@ -466,11 +566,12 @@ than using the check-in API (and maybe keep the caching support?).
 its own constructor for the proper target loader, and split the implementation
 inside of the `targetloader` package to have multiple target loaders.
 
-	*  rework `pkg/oonimkall` to invoke a target loader rather than relying
-	on the `InputPolicy`
-
 	*  make sure richer-input-enabled experiments can run with `oonimkall`
 	after we have performed the previous change
 
+	* make sure we're passing the correct check-in settings to `oonimkall`
+	such that it's possible to run Web Connectivity from mobile using
+	the loader and we can simplify the mobile app codebase
+
 *  devise long term strategy for delivering richer input to `oonimkall`
 from mobile apps, which we'll need as soon as we convert the IM experiments

internal/targetloading/targetloading.go

@@ -215,10 +215,10 @@ var dnsCheckDefaultInput = []string{
 
 var stunReachabilityDefaultInput = stuninput.AsnStunReachabilityInput()
 
-// StaticBareInputForExperiment returns the list of strings an
+// staticBareInputForExperiment returns the list of strings an
 // experiment should use as static input. In case there is no
 // static input for this experiment, we return an error.
-func StaticBareInputForExperiment(name string) ([]string, error) {
+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.
@@ -239,7 +239,7 @@ func StaticBareInputForExperiment(name string) ([]string, error) {
 // staticInputForExperiment returns the static input for the given experiment
 // or an error if there's no static input for the experiment.
 func staticInputForExperiment(name string) ([]model.ExperimentTarget, error) {
-	return stringListToModelExperimentTarget(StaticBareInputForExperiment(name))
+	return stringListToModelExperimentTarget(staticBareInputForExperiment(name))
 }
 
 // loadOrStaticDefault implements the InputOrStaticDefault policy.
@@ -364,15 +364,6 @@ func (il *Loader) checkIn(
 	return &reply.Tests, nil
 }
 
-// fetchOpenVPNConfig fetches vpn information for the configured providers
-func (il *Loader) fetchOpenVPNConfig(ctx context.Context, provider string) (*model.OOAPIVPNProviderConfig, error) {
-	reply, err := il.Session.FetchOpenVPNConfig(ctx, provider, "XX")
-	if err != nil {
-		return nil, err
-	}
-	return reply, nil
-}
-
 // preventMistakes makes the code more robust with respect to any possible
 // integration issue where the backend returns to us URLs that don't
 // belong to the category codes we requested.

pkg/oonimkall/taskmodel.go

@@ -106,10 +106,12 @@ type eventLog struct {
 }
 
 type eventMeasurementGeneric struct {
-	Failure string `json:"failure,omitempty"`
-	Idx     int64  `json:"idx"`
-	Input   string `json:"input"`
-	JSONStr string `json:"json_str,omitempty"`
+	CategoryCode string `json:"category_code,omitempty"`
+	CountryCode  string `json:"country_code,omitempty"`
+	Failure      string `json:"failure,omitempty"`
+	Idx          int64  `json:"idx"`
+	Input        string `json:"input"`
+	JSONStr      string `json:"json_str,omitempty"`
 }
 
 type eventStatusEnd struct {
@@ -314,4 +316,8 @@ type settingsOptions struct {
 	// SoftwareVersion is the software version. If this option is not
 	// present, then the library startup will fail.
 	SoftwareVersion string `json:"software_version,omitempty"`
+
+	// TODO(https://github.com/ooni/probe/issues/2767): to support OONI Run v2 descriptors with
+	// richer input from mobile, here we also need a string-serialization
+	// of the descriptor options to load.
 }