Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(dnscheck): gracefully cast to Target #1623

Merged
merged 2 commits into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions docs/design/dd-008-richer-input.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ the following command runs the `dnscheck` experiment measuring
./miniooni dnscheck -i https://8.8.8.8/dns-query -O HTTP3Enabled=true
```


Additionally, OONI Run v2 allows to run experiments with options. For example,
the following JSON document is equivalent to the previous `miniooni` command:

Expand Down Expand Up @@ -151,16 +150,17 @@ Solving this problem is crucial because most OONI measurements run automatically
in the background with input provided by the backend. Therefore, by enabling
richer input, we open up the possibility of answering specific research questions
requiring options at scale. For example, richer input would enable us to
study [DNS over HTTP/3 is blocking](https://github.com/ooni/probe/issues/2675))
study [DNS over HTTP/3 blocking](https://github.com/ooni/probe/issues/2675))
at scale.

## Design choice: deprecating the check-in API

We originally envisioned distributing richer input through the check-in API
but we later realized that this design would be problematic because:

1. it prevents us from having experiments implemented as scripts, a solution
that we heavily explored while researching richer input;
1. it prevents us from having experiments implemented as scripts, [a solution
that we heavily explored while researching richer input](
https://github.com/bassosimone/2023-12-09-ooni-javascript);

2. the check-in API serves URLs for Web Connectivity, which is the most
important experiment we run, which means that changing the component serving
Expand Down Expand Up @@ -343,12 +343,24 @@ the changes roughly look like this:
+ if args.Target == nil {
+ return ErrInputRequired
+ }
+ input := args.Target.(*target).input
+ input, ok := args.Target.(*target).input
+ if !ok {
+ return ErrInvalidInputType
+ }
+ options := args.Target.(*target).options
// [...]
}
```

Note how we MUST gracefully cast to `*target` (as we did in [probe-cli#1623](
https://github.com/ooni/probe-cli/pull/1623)) because richer input could
potentially come from ~any source, including the mobile app. While richer input
is anything that fullfills the `model.ExperimentTarget` interface, mobile apps
could, for example, construct a Java class implementing such an interface but we
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.

## Next steps

This is a rough sequence of next steps that we should expand as we implement
Expand Down Expand Up @@ -387,5 +399,3 @@ inside of the `targetloader` package to have multiple target loaders.

* 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


8 changes: 6 additions & 2 deletions internal/experiment/dnscheck/dnscheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ func (m *Measurer) ExperimentVersion() string {
// that are used by this experiment to implement its functionality.
var (
ErrInputRequired = targetloading.ErrInputRequired
ErrInvalidInputType = targetloading.ErrInvalidInputType
ErrInvalidURL = errors.New("the input URL is invalid")
ErrUnsupportedURLScheme = errors.New("unsupported URL scheme")
)
Expand All @@ -125,11 +126,14 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
measurement := args.Measurement
sess := args.Session

// 0. obtain the richer input target, config, and input or panic
// 0. obtain the richer input target, config, and input or fail
if args.Target == nil {
return ErrInputRequired
}
target := args.Target.(*Target)
target, ok := args.Target.(*Target)
if !ok {
return ErrInvalidInputType
}
config, input := target.Options, target.URL
sess.Logger().Infof("dnscheck: using richer input: %+v %+v", config, input)

Expand Down
14 changes: 14 additions & 0 deletions internal/experiment/dnscheck/dnscheck_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,20 @@ func TestExperimentNameAndVersion(t *testing.T) {
}
}

func TestDNSCheckFailsWithInvalidInputType(t *testing.T) {
measurer := NewExperimentMeasurer()
args := &model.ExperimentArgs{
Callbacks: model.NewPrinterCallbacks(log.Log),
Measurement: new(model.Measurement),
Session: newsession(),
Target: &model.OOAPIURLInfo{}, // not the expected input type
}
err := measurer.Run(context.Background(), args)
if !errors.Is(err, ErrInvalidInputType) {
t.Fatal("expected no input error")
}
}

func TestDNSCheckFailsWithoutInput(t *testing.T) {
measurer := NewExperimentMeasurer()
args := &model.ExperimentArgs{
Expand Down
1 change: 1 addition & 0 deletions internal/targetloading/targetloading.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var (
ErrInputRequired = errors.New("no input provided")
ErrNoInputExpected = errors.New("we did not expect any input")
ErrNoStaticInput = errors.New("no static input for this experiment")
ErrInvalidInputType = errors.New("invalid richer input type")
)

// Session is the session according to a [*Loader] instance.
Expand Down
Loading