fix: provide good default input for dnscheck and stunreachability #539

hellais · 2021-10-12T15:55:28Z

Checklist

I have read the contribution guidelines
reference issue for this pull request: cli: dnscheck results accounted as the "run" experiment probe#1814
companion ooni/spec PR: fix(stun): document changes in input handling spec#227

Description

This diff aims to solve the problem that dnscheck and stunreachability are not easily runnable from ooniprobe. We started with a small diff, and then we needed to hammer other pieces of the codebase to make it WAI.

This diff will need to be backported to the release/3.11 branch. (Also, this diff will work better when using the release/3.11 branch because ooni/probe#1816 and ooni/probe#1808 make the master branch not always ideal, but we started on master and I'd say we merge on master and then backport.)

Closes: ooni/probe#1814

hellais · 2021-10-12T15:58:16Z

This doesn't actually work as it crashes with the following stacktrace:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0xf0 pc=0x4a69991]

bassosimone · 2021-10-12T20:06:33Z

@hellais ouch, so I guess we gotta figure out why :-(

bassosimone

We need to figure out why it's crashing, so I'm using the "request changes" feature to signal that we're not quite done here yet.

bassosimone · 2021-10-13T08:49:38Z

@hellais I have fixed the way in which the code works and it should be okay now. Could you please test it? I am going to figure out why it was crashing after your initial diff and apply fixes/write tests as a follow up step.

Dismissing my review since I introduced significant changes on the same branch :^)

hellais · 2021-10-13T14:34:03Z

I tested this branch and I can confirm that it's not crashing on the running of the test anymore.

However the show command for both the stunreachability and the dnscheck tests is not working as expected.

This is the output of a show command:

./ooniprobe -v show 30528
   • ooni version 3.11.0-alpha
   • Reading default config file
   • Connecting to database sqlite3:///Users/art/.ooniprobe/db/main.sqlite3
   • running migrations
   • performed 0 migrations
   • using https://api.ooni.io/api/v1/raw_measurement?report_id=20211013T143221Z_stunreachability_IT_30722_n1_C21JUQ865m4eaZbb
{
  "error": ""
}%

I believe the reason for it failing to do the lookup is that it's not passing the input value to the raw_measurement lookup query. I suspect this is due to the fact that we aren't telling probe-cli that this test is a multi-input test and it doesn't record the input value inside of the table for it.

We're about to make the stun experiment a first class citizen of ooniprobe as part of the `ooniprobe run experimental` command. All ooniprobe tests require a URL as their input. So, make stun no exception and add require the `stun://` scheme now.

1. the input MUST now be an URL 2. there is no default input anymore See ooni/probe-cli#539

I still think I _may_ have broken oonimkall for stunreachability.

Yare yare daze

bassosimone

Did a round of self code review

bassosimone · 2021-11-26T12:18:25Z

cmd/ooniprobe/internal/nettests/dnscheck.go

+	if err != nil {
+		return nil, err
+	}
+	return ctl.BuildAndSetInputIdxMap(ctl.Probe.DB(), testlist)


Explanation of the changes in this file: DNSCheck now has a lookupURLs method that uses the engine's InputLoader with the InputOrStaticDefault policy. The end result is that we use --input or --input-files if they are provided. Otherwise, we fallback to a static input list defined in the engine.

bassosimone · 2021-11-26T12:19:40Z

cmd/ooniprobe/internal/nettests/dnscheck.go

 }

 // Run starts the nettest.
 func (n DNSCheck) Run(ctl *Controller) error {
-	builder, err := ctl.Session.NewExperimentBuilder("run")
+	builder, err := ctl.Session.NewExperimentBuilder("dnscheck")


This two-lines change above is what we started from. Originally dnscheck here used the engine's run functionality to run tests with many options. Yet, we cannot do that easily. So, we fallback to running dnscheck directly with the afore-mentioned "static input" mechanism.

bassosimone · 2021-11-26T12:20:50Z

cmd/ooniprobe/internal/nettests/nettests.go

+		urls = append(urls, url.URL)
+	}
+	c.inputIdxMap = urlIDMap
+	return urls, nil


This function factors code that previously was Web-Connectivity specific. We now have a mechanism that all tests can use to configure their input so that it's stored into the inputs URL. Because the data model assumes an URL, this means we also changed stunreachability to require URLs as input for consistency (you'll see this later).

cmd/ooniprobe/internal/nettests/dnscheck.go

bassosimone · 2021-11-26T12:23:20Z

cmd/ooniprobe/internal/nettests/stunreachability.go

+	}
+	return ctl.BuildAndSetInputIdxMap(ctl.Probe.DB(), testlist)
+}
+


This diff is simular to the dnscheck one. Also here we use the default static input if no input is provided.

bassosimone · 2021-11-26T12:36:00Z

internal/engine/inputloader.go

+// 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) {


This function is public because pkg/oonimkall does not use InputLoader directly for loading inputs. There is need to refactor how we run experiments on mobile. (やれやれ)

internal/engine/inputloader_test.go

bassosimone · 2021-11-26T12:39:25Z

internal/ptx/snowflake.go

-		"stun:stun.voipgate.com:3478",
-		"stun:stun.voys.nl:3478",
-	}
+	return stuninput.AsSnowflakeInput()


Rather than having two static copies of STUN targets (one for snowflake and one for stunreachability), I have created a single package that keeps those copies.

pkg/oonimkall/internal/tasks/runner.go

bassosimone · 2021-11-26T12:42:27Z

pkg/oonimkall/internal/tasks/runner.go

+			}
+			r.settings.Inputs = inputs
+		default:
+			r.settings.Inputs = append(r.settings.Inputs, "")


Ouch, these two lines are wrong, we should make the switch exhaustive I think

bassosimone · 2021-11-26T13:08:23Z

The code in here is not ready yet. I think it starts to be clear that we need to improve testing a bit inside the pkg/oonimkall package, after I have changed the code handling input. I think we're currently using a lot this example experiment to test for things, but perhaps it would be better to just use the real experiments. This would give us greater confidence. Also, there is some annoying duplication of testing between pkg/oonimkall and pkg/oonimkall/tasks. As bad as it may be to further complicate this diff, I think there is some need to consolidate and improve how we test pkg/oonimkall. Maybe the right thing to do is to move some bits of this diff out to smaller diffs and apply more refactoring. After all, here we are touching code that is quite old, so it's reasonable we have to do some more working. I'm going to convert this PR to draft for the time being to reflect the fact that I think it's not completely ready yet.

bassosimone · 2021-12-03T08:46:43Z

The work mentioned in #539 (comment) has now been done in ooni/probe#1903. It's now time to resume work on this PR and land it at last!

Conflicts: internal/engine/allexperiments.go pkg/oonimkall/internal/tasks/runner_integration_test.go pkg/oonimkall/taskrunner.go

Here we're refactoring stunreachability to not provide internally a default input and to take in input an URL rather than a string. The related ooni/spec change is ooni/spec#227. This diff has been extracted from #539. Because the original diff was large, I'm splitting it in a set of more easily manageable diffs. The reference issue is ooni/probe#1814, which is complex enough to require us to proceed incrementally. This diff WILL need to be backported to release/3.11.

We want stunreachability to use the same STUN servers used by snowflake, so let's start by making a common package holding the servers. Let's also use this new package in Snowflake. We're currently not using this package in stunreachability, but I am going to apply this as a subsequent diff. Reference issue: ooni/probe#1814. This issue is a bit complex to address in a single PR, so we are going to proceed incremntally. This diff was extracted from #539.

This commit introduces a new `InputLoader` policy by which, if no input is provided, we use a static default input list. We also modify the code to use this policy for dnscheck and stunreachability, with proper input. We also modify `miniooni` to pass the new `ExperimentName` field to the `InputLoader` to indicate which default input list to use. This diff is part of a set of diffs aiming at fixing ooni/probe#1814 and has been extracted from #539. What remains to be done, after this diff has landed is to ensure things also work for ooniprobe and oonimkall.

Conflicts: internal/engine/inputloader.go internal/engine/inputloader_test.go

This diff is part of ooni/probe#1814 and teaches `ooniprobe` to run dnscheck and stunreachability by using the default static input feature of the `InputLoader`. I've manually tested that we can still run `websites` like we did before (including category filtering). I've also manually tested that now we can run `experimental` and get parseable results for dnscheck and stunreachability. With this diff in, we have fixed the original problem highlighted in the ooni/probe#1814 issue. Yet, because of the way in which I solved the problem, there is more work to do. My changes have broken stunreachability for mobile and now it's time I apply fixes to make it work again. This diff was extracted from #539, which at this point only basically contains the remaining fixes to ensure we can run stunreachability on mobile. Co-authored-by: Arturo Filastò <arturo@filasto.net>

This diff is part of ooni/probe#1814 and teaches `ooniprobe` to run dnscheck and stunreachability by using the default static input feature of the `InputLoader`. I've manually tested that we can still run `websites` like we did before (including category filtering). I've also manually tested that now we can run `experimental` and get parseable results for dnscheck and stunreachability. With this diff in, we have fixed the original problem highlighted in the ooni/probe#1814 issue. Yet, because of the way in which I solved the problem, there is more work to do. My changes have broken stunreachability for mobile and now it's time I apply fixes to make it work again. This diff was extracted from #539, which at this point only basically contains the remaining fixes to ensure we can run stunreachability on mobile. Co-authored-by: Arturo Filastò <arturo@filasto.net> Co-authored-by: Arturo Filastò <arturo@filasto.net>

Previous work to make ooni/probe#1814 possible has broken running stunreachability on mobile. This diff repairs the blunder and allows to run any experiment using InputOrStaticDefault with oonimkall. Diff extracted from #539.

Conflicts: pkg/oonimkall/taskrunner.go

bassosimone · 2021-12-03T16:44:51Z

All the work that was part of this PR has been split into smaller patches and applied separately. We can close now.

…URL (#630) Here we're refactoring stunreachability to not provide internally a default input and to take in input an URL rather than a string. The related ooni/spec change is ooni/spec#227. This diff has been extracted from #539. Because the original diff was large, I'm splitting it in a set of more easily manageable diffs. The reference issue is ooni/probe#1814, which is complex enough to require us to proceed incrementally. This diff WILL need to be backported to release/3.11.

We want stunreachability to use the same STUN servers used by snowflake, so let's start by making a common package holding the servers. Let's also use this new package in Snowflake. We're currently not using this package in stunreachability, but I am going to apply this as a subsequent diff. Reference issue: ooni/probe#1814. This issue is a bit complex to address in a single PR, so we are going to proceed incremntally. This diff was extracted from #539.

This commit introduces a new `InputLoader` policy by which, if no input is provided, we use a static default input list. We also modify the code to use this policy for dnscheck and stunreachability, with proper input. We also modify `miniooni` to pass the new `ExperimentName` field to the `InputLoader` to indicate which default input list to use. This diff is part of a set of diffs aiming at fixing ooni/probe#1814 and has been extracted from #539. What remains to be done, after this diff has landed is to ensure things also work for ooniprobe and oonimkall.

…nput (#633) This diff is part of ooni/probe#1814 and teaches `ooniprobe` to run dnscheck and stunreachability by using the default static input feature of the `InputLoader`. I've manually tested that we can still run `websites` like we did before (including category filtering). I've also manually tested that now we can run `experimental` and get parseable results for dnscheck and stunreachability. With this diff in, we have fixed the original problem highlighted in the ooni/probe#1814 issue. Yet, because of the way in which I solved the problem, there is more work to do. My changes have broken stunreachability for mobile and now it's time I apply fixes to make it work again. This diff was extracted from #539, which at this point only basically contains the remaining fixes to ensure we can run stunreachability on mobile. Co-authored-by: Arturo Filastò <arturo@filasto.net> Co-authored-by: Arturo Filastò <arturo@filasto.net>

…#634) Previous work to make ooni/probe#1814 possible has broken running stunreachability on mobile. This diff repairs the blunder and allows to run any experiment using InputOrStaticDefault with oonimkall. Diff extracted from #539.

* [backport] refactor(stunreachability): input required and must be an URL (#630) Here we're refactoring stunreachability to not provide internally a default input and to take in input an URL rather than a string. The related ooni/spec change is ooni/spec#227. This diff has been extracted from #539. Because the original diff was large, I'm splitting it in a set of more easily manageable diffs. The reference issue is ooni/probe#1814, which is complex enough to require us to proceed incrementally. This diff WILL need to be backported to release/3.11. * [backport] refactor: create common package for holding STUN input (#631) We want stunreachability to use the same STUN servers used by snowflake, so let's start by making a common package holding the servers. Let's also use this new package in Snowflake. We're currently not using this package in stunreachability, but I am going to apply this as a subsequent diff. Reference issue: ooni/probe#1814. This issue is a bit complex to address in a single PR, so we are going to proceed incremntally. This diff was extracted from #539. * [backport] refactor: introduce and use InputOrStaticDefault (#632) This commit introduces a new `InputLoader` policy by which, if no input is provided, we use a static default input list. We also modify the code to use this policy for dnscheck and stunreachability, with proper input. We also modify `miniooni` to pass the new `ExperimentName` field to the `InputLoader` to indicate which default input list to use. This diff is part of a set of diffs aiming at fixing ooni/probe#1814 and has been extracted from #539. What remains to be done, after this diff has landed is to ensure things also work for ooniprobe and oonimkall. * [backport] fix(ooniprobe): dnscheck,stunreachability run w/ default input (#633) This diff is part of ooni/probe#1814 and teaches `ooniprobe` to run dnscheck and stunreachability by using the default static input feature of the `InputLoader`. I've manually tested that we can still run `websites` like we did before (including category filtering). I've also manually tested that now we can run `experimental` and get parseable results for dnscheck and stunreachability. With this diff in, we have fixed the original problem highlighted in the ooni/probe#1814 issue. Yet, because of the way in which I solved the problem, there is more work to do. My changes have broken stunreachability for mobile and now it's time I apply fixes to make it work again. This diff was extracted from #539, which at this point only basically contains the remaining fixes to ensure we can run stunreachability on mobile. Co-authored-by: Arturo Filastò <arturo@filasto.net> Co-authored-by: Arturo Filastò <arturo@filasto.net> * [backport] fix(oonimkall): run tests with InputOrStaticDefault policy (#634) Previous work to make ooni/probe#1814 possible has broken running stunreachability on mobile. This diff repairs the blunder and allows to run any experiment using InputOrStaticDefault with oonimkall. Diff extracted from #539. Co-authored-by: Arturo Filastò <arturo@filasto.net>

Here we're refactoring stunreachability to not provide internally a default input and to take in input an URL rather than a string. The related ooni/spec change is ooni/spec#227. This diff has been extracted from ooni#539. Because the original diff was large, I'm splitting it in a set of more easily manageable diffs. The reference issue is ooni/probe#1814, which is complex enough to require us to proceed incrementally. This diff WILL need to be backported to release/3.11.

We want stunreachability to use the same STUN servers used by snowflake, so let's start by making a common package holding the servers. Let's also use this new package in Snowflake. We're currently not using this package in stunreachability, but I am going to apply this as a subsequent diff. Reference issue: ooni/probe#1814. This issue is a bit complex to address in a single PR, so we are going to proceed incremntally. This diff was extracted from ooni#539.

This commit introduces a new `InputLoader` policy by which, if no input is provided, we use a static default input list. We also modify the code to use this policy for dnscheck and stunreachability, with proper input. We also modify `miniooni` to pass the new `ExperimentName` field to the `InputLoader` to indicate which default input list to use. This diff is part of a set of diffs aiming at fixing ooni/probe#1814 and has been extracted from ooni#539. What remains to be done, after this diff has landed is to ensure things also work for ooniprobe and oonimkall.

) This diff is part of ooni/probe#1814 and teaches `ooniprobe` to run dnscheck and stunreachability by using the default static input feature of the `InputLoader`. I've manually tested that we can still run `websites` like we did before (including category filtering). I've also manually tested that now we can run `experimental` and get parseable results for dnscheck and stunreachability. With this diff in, we have fixed the original problem highlighted in the ooni/probe#1814 issue. Yet, because of the way in which I solved the problem, there is more work to do. My changes have broken stunreachability for mobile and now it's time I apply fixes to make it work again. This diff was extracted from ooni#539, which at this point only basically contains the remaining fixes to ensure we can run stunreachability on mobile. Co-authored-by: Arturo Filastò <arturo@filasto.net> Co-authored-by: Arturo Filastò <arturo@filasto.net>

Previous work to make ooni/probe#1814 possible has broken running stunreachability on mobile. This diff repairs the blunder and allows to run any experiment using InputOrStaticDefault with oonimkall. Diff extracted from ooni#539.

Xredmi · 2022-11-13T19:13:48Z

Checklist

I have read the contribution guidelines

reference issue for this pull request: cli: dnscheck results accounted as the "run" experiment probe#1814

companion ooni/spec PR: fix(stun): document changes in input handling spec#227

Description

This diff aims to solve the problem that dnscheck and stunreachability are not easily runnable from ooniprobe. We started with a small diff, and then we needed to hammer other pieces of the codebase to make it WAI.

This diff will need to be backported to the release/3.11 branch. (Also, this diff will work better when using the release/3.11 branch because ooni/probe#1816 and ooni/probe#1808 make the master branch not always ideal, but we started on master and I'd say we merge on master and then backport.)

Fix issue with the test_name of dnscheck being run

2918cb5

Closes: ooni/probe#1814

hellais requested a review from bassosimone as a code owner October 12, 2021 15:55

bassosimone previously requested changes Oct 12, 2021

View reviewed changes

fix: provide good default input for dnscheck and stunreachability

172838d

bassosimone changed the title ~~Fix issue with the test_name of dnscheck being run~~ fix: provide good default input for dnscheck and stunreachability Oct 13, 2021

bassosimone added 3 commits November 26, 2021 10:19

Merge branch 'master' into fix/1814

645fb85

refactor(stun): the input must be a URL

f38875c

We're about to make the stun experiment a first class citizen of ooniprobe as part of the `ooniprobe run experimental` command. All ooniprobe tests require a URL as their input. So, make stun no exception and add require the `stun://` scheme now.

fix: make sure experimental tests have input URLs

09ae4a5

bassosimone mentioned this pull request Nov 26, 2021

ci failure: stunreachability_test.go:39: generic_timeout_error ooni/probe#1556

Closed

bassosimone added a commit to ooni/spec that referenced this pull request Nov 26, 2021

fix(stun): document changes in input handling

f5693f2

1. the input MUST now be an URL 2. there is no default input anymore See ooni/probe-cli#539

bassosimone mentioned this pull request Nov 26, 2021

fix(stun): document changes in input handling ooni/spec#227

Merged

4 tasks

bassosimone added a commit to ooni/spec that referenced this pull request Nov 26, 2021

fix(stun): document changes in input handling (#227)

238e3c6

1. the input MUST now be an URL 2. there is no default input anymore See ooni/probe-cli#539

bassosimone added 4 commits November 26, 2021 11:35

fix: declare that stunreachability strictly requires input

e8512d0

fix: better solution where also miniooni works

f8eb3ad

I still think I _may_ have broken oonimkall for stunreachability.

fix: consolidate stun targets into a single place

fe0d444

fix: we need some default with mobile as well

8552153

Yare yare daze

bassosimone reviewed Nov 26, 2021

View reviewed changes

bassosimone added 2 commits November 26, 2021 13:46

Apply suggestions from code review

d43bff6

perhaps better code regarding input options

a93eab2

bassosimone marked this pull request as draft November 26, 2021 13:08

bassosimone mentioned this pull request Nov 26, 2021

refactor: make the pkg/oonimkall package more easily testable ooni/probe#1903

Closed

Merge branch 'master' into fix/1814

8b26591

Conflicts: internal/engine/allexperiments.go pkg/oonimkall/internal/tasks/runner_integration_test.go pkg/oonimkall/taskrunner.go

bassosimone mentioned this pull request Dec 3, 2021

refactor: create common package for holding STUN input #631

Merged

Merge branch 'master' into fix/1814

fdf378c

bassosimone mentioned this pull request Dec 3, 2021

refactor: introduce and use InputOrStaticDefault #632

Merged

Merge branch 'master' into fix/1814

38f50ae

Conflicts: internal/engine/inputloader.go internal/engine/inputloader_test.go

bassosimone mentioned this pull request Dec 3, 2021

fix(ooniprobe): dnscheck,stunreachability run w/ default input #633

Merged

Merge branch 'master' into fix/1814

c2c27fd

bassosimone mentioned this pull request Dec 3, 2021

fix(oonimkall): run tests with InputOrStaticDefault policy #634

Merged

Merge branch 'master' into fix/1814

fc1968b

Conflicts: pkg/oonimkall/taskrunner.go

bassosimone closed this Dec 3, 2021

bassosimone deleted the fix/1814 branch December 3, 2021 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: provide good default input for dnscheck and stunreachability #539

fix: provide good default input for dnscheck and stunreachability #539

hellais commented Oct 12, 2021 •

edited by bassosimone

Loading

hellais commented Oct 12, 2021 •

edited

Loading

bassosimone commented Oct 12, 2021

bassosimone left a comment

bassosimone commented Oct 13, 2021

hellais commented Oct 13, 2021 •

edited

Loading

bassosimone left a comment

bassosimone Nov 26, 2021

bassosimone Nov 26, 2021

bassosimone Nov 26, 2021

bassosimone Nov 26, 2021

bassosimone Nov 26, 2021

bassosimone Nov 26, 2021

bassosimone Nov 26, 2021

bassosimone commented Nov 26, 2021

bassosimone commented Dec 3, 2021

bassosimone commented Dec 3, 2021

Xredmi commented Nov 13, 2022

Checklist

Description

fix: provide good default input for dnscheck and stunreachability #539

fix: provide good default input for dnscheck and stunreachability #539

Conversation

hellais commented Oct 12, 2021 • edited by bassosimone Loading

Checklist

Description

hellais commented Oct 12, 2021 • edited Loading

bassosimone commented Oct 12, 2021

bassosimone left a comment

Choose a reason for hiding this comment

bassosimone commented Oct 13, 2021

hellais commented Oct 13, 2021 • edited Loading

bassosimone left a comment

Choose a reason for hiding this comment

bassosimone Nov 26, 2021

Choose a reason for hiding this comment

bassosimone Nov 26, 2021

Choose a reason for hiding this comment

bassosimone Nov 26, 2021

Choose a reason for hiding this comment

bassosimone Nov 26, 2021

Choose a reason for hiding this comment

bassosimone Nov 26, 2021

Choose a reason for hiding this comment

bassosimone Nov 26, 2021

Choose a reason for hiding this comment

bassosimone Nov 26, 2021

Choose a reason for hiding this comment

bassosimone commented Nov 26, 2021

bassosimone commented Dec 3, 2021

bassosimone commented Dec 3, 2021

Xredmi commented Nov 13, 2022

Checklist

Description

hellais commented Oct 12, 2021 •

edited by bassosimone

Loading

hellais commented Oct 12, 2021 •

edited

Loading

hellais commented Oct 13, 2021 •

edited

Loading