## Checklist
- [x] I have read the [contribution guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md)
- [x] reference issue for this pull request: https://github.com/ooni/probe/issues/1885
- [x] related ooni/spec pull request: N/A
Location of the issue tracker: https://github.com/ooni/probe
## Description
This PR contains a set of changes to move important interfaces and data types into the `./internal/model` package.
The criteria for including an interface or data type in here is roughly that the type should be important and used by several packages. We are especially interested to move more interfaces here to increase modularity.
An additional side effect is that, by reading this package, one should be able to understand more quickly how different parts of the codebase interact with each other.
This is what I want to move in `internal/model`:
- [x] most important interfaces from `internal/netxlite`
- [x] everything that was previously part of `internal/engine/model`
- [x] mocks from `internal/netxlite/mocks` should also be moved in here as a subpackage
This branch adds support for running:
1. `go-libtor` on mobile.
2. the tor provided by the desktop app via the `OONI_TOR_BINARY` environment variable.
See https://github.com/ooni/ooni.org/issues/761.
Co-authored-by: Simone Basso <bassosimone@gmail.com>
This diff is part of https://github.com/ooni/probe/issues/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 https://github.com/ooni/probe/issues/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 https://github.com/ooni/probe-cli/pull/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>
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
https://github.com/ooni/probe/issues/1814 and has been
extracted from https://github.com/ooni/probe-cli/pull/539.
What remains to be done, after this diff has landed is to ensure
things also work for ooniprobe and oonimkall.
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: https://github.com/ooni/probe/issues/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 https://github.com/ooni/probe-cli/pull/539.
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 https://github.com/ooni/spec/pull/227.
This diff has been extracted from https://github.com/ooni/probe-cli/pull/539.
Because the original diff was large, I'm splitting it in a set of
more easily manageable diffs.
The reference issue is https://github.com/ooni/probe/issues/1814, which
is complex enough to require us to proceed incrementally.
This diff WILL need to be backported to release/3.11.
* [forwardport] fix(oonimkall): make logger used by tasks unit testable (#623)
This diff forward ports e4b04642c51e7461728b25941624e1b97ef0ec83.
Reference issue: https://github.com/ooni/probe/issues/1903
* [forwardport] feat(oonimkall): improve taskEmitter testability (#624)
This diff forward ports 3e0f01a389c1f4cdd7878ec151aff91870a0bdff.
1. rename eventemitter{,_test}.go => taskemitter{,_test}.go because
the new name is more proper after we merged the internal/task package
inside of the oonimkall package;
2. rename runner.go's `run` function to `runTask`;
3. modify `runTask` to use the new `taskEmitterUsingChan` abstraction
on which we will spend more works in a later point of this list;
4. introduce `runTaskWithEmitter` factory that is called by `runTask`
and allows us to more easily write unit tests;
5. acknowledge that `runner` was not using its `out` field;
6. use the new `taskEmitterWrapper` in `newRunner`;
7. acknowledge that `runnerCallbacks` could use a generic
`taskEmitter` as field type rather than a specific type;
8. rewrite tests to use `runTaskWithEmitter` which leads to
simpler code that does not require a goroutine;
9. acknowledge that the code has been ignoring the `DisabledEvents`
settings for quite some time, so stop supporting it;
10. refactor the `taskEmitter` implementation to be like:
1. we still have the `taskEmitter` interface;
2. `taskEmitterUsingChan` wraps the channel and allows for
emitting events using the channel;
3. `taskEmitterUsingChan` owns an `eof` channel that is
closed by `Close` (which is idempotent) and signals we
should be stop emitting;
4. make sure `runTask` creates a `taskEmitterUsingChan`
and calls its `Close` method when done;
5. completely remove the code for disabling events
since the code was actually ignoring the stting;
6. add a `taskEmitterWrapper` that adds common functions
for emitting events to _any_ `taskWrapper`;
7. write unit tests for `taskEmitterUsingChan` and
for `taskEmitterWrapper`;
11. acknowledge that the abstraction we need for testing is
actually a thread-safe thing that collects events into a
vector containing events and refactor all tests accordingly.
See https://github.com/ooni/probe/issues/1903
* [forwardport] refactor(oonimkall): make the runner unit-testable (#625)
This diff forward ports 9423947faf6980d92d2fe67efe3829e8fef76586.
See https://github.com/ooni/probe/issues/1903
* [forwardport] feat(oonimkall): write unit tests for the runner component (#626)
This diff forward ports 35dd0e3788b8fa99c541452bbb5e0ae4871239e1.
Forward porting note: compared to 35dd0e3788b8fa99c541452bbb5e0ae4871239e1,
the diff I'm committing here is slightly different. In `master` we do not
have the case where a measurement fails and a measurement is returned, thus
I needed to adapt the test to become like this:
```diff
diff --git a/pkg/oonimkall/runner_internal_test.go b/pkg/oonimkall/runner_internal_test.go
index 334b574..84c7436 100644
--- a/pkg/oonimkall/runner_internal_test.go
+++ b/pkg/oonimkall/runner_internal_test.go
@@ -568,15 +568,6 @@ func TestTaskRunnerRun(t *testing.T) {
}, {
Key: failureMeasurement,
Count: 1,
- }, {
- Key: measurement,
- Count: 1,
- }, {
- Key: statusMeasurementSubmission,
- Count: 1,
- }, {
- Key: statusMeasurementDone,
- Count: 1,
}, {
Key: statusEnd,
Count: 1,
```
I still need to write more assertions for each emitted event
but the code we've here is already a great starting point.
See https://github.com/ooni/probe/issues/1903
* [forwardport] refactor(oonimkall): merge files, use proper names, zap unneeded integration tests (#627)
This diff forward ports f894427d24edc9a03fc78306d0093e7b51c46c25.
Forward porting note: this diff is slightly different from the original
mentioned above because it carries forward changes mentioned in the
previous diff caused by a different way of handling a failed measurement
in the master branch compared to the release/3.11 branch.
Move everything that looked like "task's model" inside of the
taskmodel.go file, for consistency.
Make sure it's clear some variables are event types.
Rename the concrete `runner` as `runnerForTask`.
Also, remove now-unnecessary (and flaky!) integration tests
for the `runnerForTask` type.
While there, notice there were wrong URLs that were generated
during the probe-engine => probe-cli move and fix them.
See https://github.com/ooni/probe/issues/1903
* [forwardport] refactor(oonimkall): we can simplify StartTask tests (#628)
This diff forward ports dcf2986c2032d8185d58d24130a7f2c2d61ef2fb.
* refactor(oonimkall): we can simplify StartTask tests
We have enough checks for runnerForTask. So we do not need to
duplicate them when checking for StartTask.
While there, refactor how we start tasks to remove the need for
extra runner functions.
This is the objective I wanted to achieve for oonimkall:
1. less duplicate tests, and
2. more unit tests (which are less flaky)
At this point, we're basically done (pending forwardporting to
master) with https://github.com/ooni/probe/issues/1903.
* fix(oonimkall): TestStartTaskGood shouldn't cancel the test
This creates a race condition where the test may fail if we cannot
complete the whole "Example" test in less than one second.
This should explain the build failures I've seen so far and why
I didn't see those failures when running locally.
1. add eof channel to event emitter and use this channel as signal
that we shouldn't be sending anymore instead of using a pattern where
we use a timer to decide sending has timed out (because we're using
a buffered channel, it is still possible for some evetns to end up
in the channel if there is space, but this is not a concern, because
the events will be deleted when the channel itself is gone);
2. refactor all tests where we assumed the output channel was closed
to actually use a parallel "eof" channel and use it as signal we
should not be sending anymore (not strictly required but still the
right thing to do in terms of using consistent patterns);
3. modify how we construct a runner so that it passes to the
event emitter an "eof" channel and closes this channel when the
main goroutine running the task is terminating;
4. modify the task to signal events such as "task goroutine started"
and "task goroutine stopped" using channels, which helps to write
much more correct tests;
5. take advantage of the previous change to improve the test that
ensures we're not blocking for a small number of events and also
improve the name of such a test to reflect what it's testing.
The related issue in term of fixing the channel usage pattern is
https://github.com/ooni/probe/issues/1438.
Regarding improving testability, instead, the correct reference
issue is https://github.com/ooni/probe/issues/1903.
There are possibly more changes to apply here to improve this package
and its testability, but let's land this diff first and then see
how much time is left for further improvements.
I've run unit and integration tests with `-race` locally.
This diff will need to be backported to `release/3.11`.
This diff forward ports 90bf0629b957c912a5a6f3bb6c98ad3abb5a2ff6 to `master`.
If we close the channel to signal the end of a task we may panic
when some background goroutine tries to post on the channel.
This bug is rare but may happen.
See for example https://github.com/ooni/probe/issues/1438.
How can we improve?
First, let us add a timeout when sending to the channel. Given that
the channel is buffered and we have a generous timeout (1/4 of a
second), it's unlikely we will really block. But, in the event in
which a late message appears, we'll eventually _unblock_ when
sending with a timeout. So, now we don't have to worry anymore
about leaking forever a goroutine.
Then, let us change the protocol with which we signal that a task
is done. We used to close the channel. Now, instead we just
synchronously post a nil on the channel when done.
In turn, we interpret this nil to mean that the task is done when
we receive messages.
The _main_ different with respect to before is that now we are
asking the consumer of our API to drain the channel. Because
before we had a blocking channel, it seems to me we were already
requiring the consumer of the API to do that. Which means, I think
in practical terms it did not change much.
Finally, acknowledge that we don't need a specific state variable
to tell us we're done and simplify a little bit the API by
just making isRunning private and using the "we're done" signal
to determine whether we've stopped running the task.
All these changes should be enough to close https://github.com/ooni/probe/issues/1438.
* [forwardport] fix(webconnectivity): send specific user agent (#615)
This forward ports b8c530388e66b2cc86abad26d077202782e4a823 to `master`.
See https://github.com/ooni/probe/issues/1902
* fix(websteps): send the correct user agent
Also related to https://github.com/ooni/probe/issues/1902: let's just
ensure that also websteps behaves in the correct way.
This diff forwardports 856e436e20d511a4f0d618546da7921fa9f8c5f6 to the master branch
Original commit message:
- - -
This pull request changes `mk` and github workflows to build and publish binaries on tag. We also update the documentation to explain this new branching model. Basically, we have release branches where we produce binary packages and we add extra code, on tag, to publish such packages inside a release.
We discussed removing most secrets from builds in this repository and having a different tool/repository that takes in input also secrets for doing follow-up actions after publishing. As a consequence, this pull request also removes all pieces of code that require secrets. The next step is to reinstate this code in this new repository/tool.
The existing code in `mk` also implemented caching. This feature was useful when doing local builds because it reduced the time required to obtain binary releases. With builds running as part of GitHub actions, we don't need caching because we spawn parallel machines to build binaries. Therefore, let us also remove caching, which makes the code simpler. (Caching in itself is hard and in https://github.com/ooni/probe/issues/1875 I noted that, for example, caching of the `ooni/go` repository was leading to some unwanted behaviour when changing the branch. Without caching, this behaviour is gone and we always generally use fresh information to produce builds.) Of course, this means that local builds are now slower, but I do not think this is a problem _because_ we want to use GitHub actions for building in the common case.
Reference issues: https://github.com/ooni/probe/issues/1879 and https://github.com/ooni/probe/issues/1875.
The final aspect to mention to conclude this description is an implementation one:
```
gh release create -p $tag --target $GITHUB_SHA || true
```
The code above uses `|| true` because there could already be a release. So, basically, it means that, if a release does not already exist, then we're going to create one. Otherwise, it does not matter because there's already a release.
This diff forward ports ea44e99451f345474738b9010ff791759a1f1367.
Original commit message:
- - -
This change allows for producing cloud builds using the psiphon
config files. We will add those files as build secrets. Only people
in the organization and collaborators with at least "write"
access could trigger builds containing such secrets.
Before this change, `./mk` unconditionally attempted to clone
github.com/ooni/probe-private. Now, it only checks whether
we need to clone _if_ files are not already there.
This allows us to use GitHub actions and secrets to copy the
files in there _without_ needing to clone a private repo.
Cloning a private repo would require us to include as repository
secret an access token with full `repo` scope, which is a very
broad scope. Instead, by using secrets to include psiphon config,
we are narrowing down the secrets required to make a release build.
See https://github.com/ooni/probe/issues/1878
This diff WILL require forward porting to the master branch.
The text I've written here documents the current process as of
today. The only major change I've added today is the `miniooni-staging`
branch, which previously wasn't published and only lived in the
private machine I was using for building `miniooni`.
This diff forward ports f47b0c6c16e0cd417e3591358eb85b45962f307d to master.
Original commit message:
- - -
1. we now need to name the framework `.xcframework` otherwise
gomobile refuses to build a new framework for us ¯\_(ツ)_/¯
2. remove duplicate errno definition for iOS (iOS and darwin
are considered the same, therefore we don't need iOS defs)
Reference issue for this PR: https://github.com/ooni/probe/issues/1876
This diff WILL need to be forwardported to master.
This commit forward ports dedd84fa7ecb09f718f6b1a9c83999cb37b34dfa.
Original commit message:
- - -
This diff changes code the release/3.11 branch to ensure we're not using dns.google and www.google.com over HTTP3. As documented in https://github.com/ooni/probe/issues/1873, since this morning (approx) these services do not support HTTP3 anymore. (I didn't bother with checking whether this issue affects _other_ Google services; I just limited my analysis to the services that we were using as part of testing.)
This patch WILL require forward porting to the master branch.
This diff forward ports abd212d014783cb96eaf821e9bb9a7b01039f837.
Original commit message:
- - -
This diff upgrades `./mk` to use go1.17.3 rather than go1.16. While there, also update other Android build tools.
This diff WILL need a forwardport to master.
Closes https://github.com/ooni/probe/issues/1863.
This diff forward ports adcb0f9ae3b9e074c301d4f7f0e8f2d0ef6466b9.
Original commit message:
- - -
- ensure we use go1.17.3 in workflows
- update to a version of ooni/oohttp that uses go1.17.3
This change WILL need to be forward ported to master.
Closes https://github.com/ooni/probe/issues/1861
This commit forward ports 74947dbbd12266c12a38fad51a70fc78a21720fd from
the `release/3.11` branch to `master`. Here's the original commit message:
- - -
Android is also Linux. The Android build fails because both
errno_linux.go and errno_android.go are compiled.
There's no difference between the files except into a comment
that mentions "linux" or "android".
Therefore, it's safe to remove the android-specific file
and just keep and use the linux-specific one.
Part of https://github.com/ooni/probe/issues/1863, where we're
forward porting ooni/go patches to go1.17.
I'm still trying to figure out whether I can build oonimkall
using the forward ported patches and this error prevents me
from building, because the build fails.
"やれやれだぜ"
Note that this patch WILL need to be forward ported to master.
This bug was previosuly reported to me by @hellais.
Because I did run `go generate ./internal/netxlite/...` we also
get for free updated certificates, which is OK.
1. introduce implementations of HTTPTransport and HTTPClient
that apply an error wrapping policy using the constructor
for a generic top-level error wrapper
2. make sure we use the implementations in point 1 when we
are constructing HTTPTransport and HTTPClient
3. make sure we apply error wrapping using the constructor for
a generic top-level error wrapper when reading bodies
4. acknowledge that error wrapping would be broken if we do
not return the same classification _and_ operation when we wrap
an already wrapped error, so fix the to code to do that
5. acknowledge that the classifiers already deal with preserving
the error string and explain why this is a quirk and why we
cannot remove it right now and what needs to happen to safely
remove this quirk from the codebase
Closes https://github.com/ooni/probe/issues/1860
This pull request introduces a set of Node.js scripts for performing A/B comparison of websteps and webconnectivity as described in https://github.com/ooni/probe/issues/1805. Rather than using Jafar, I ended up using `miniooni`'s `--censor` command line flag introduced in [v3.12.0-alpha.1](https://github.com/ooni/probe-cli/releases/tag/v3.12.0-alpha.1). The main reason for doing so is that it's simpler to run tests without requiring root access and Linux _and_ Docker (e.g., I did not develop part of this diff using Linux). Additionally, I choose to use Node.js rather than extending the existing Python framework for QA, because I found Node.js easier when working with JSON data.
Each tutorial provides you a piece of knowledge. Add some text in
the main readme file explaining the big picture to the reader.
Closes https://github.com/ooni/probe/issues/1819
This diff changes the algorithm used by webconnectivity's
httpanalysis.go to ignore any status code <= 0 rather
than just ignoring the == 0 case.
Make sure we add test cases for when the control's status
code is negative rather than being zero.
While there, simplify code where boolean checks could be
more compact according to staticcheck.
Closes https://github.com/ooni/probe/issues/1825
This change should simplify the pipeline's job.
Reference issue: https://github.com/ooni/probe/issues/1817.
I previously dismissed this possibility, but now it seems clear it
is simpler to have a very tabular data format internally and to
convert such a format to OONI's data format when serializing.
The OONI data format is what the pipeline expects, but processing
is easier with a more linear/tabular format.
This reverts commit 851b9913fa because
it seems it's not enough to allow us to see certificate errors with
quic, plus it's complex code. So, we'd rather develop a better approach,
and perhaps a simpler one, that works with QUIC as well.
This is the policy we need to provoke certificate errors. We'll divert
from, say, `8.8.8.8:443/udp` to, say, `1.1.1.1:443/udp`.
We'll do something similar for `443/tcp`.
This will cause certificate validation errors.
With this change, we have now implemented the simple design described
by https://github.com/ooni/probe/issues/1803#issuecomment-957323297.
When we're testing multiple endpoints, it's quite important to control
the order with which they are returned to the code.
This feature is especially relevant to Web Connectivity, which will
check the endpoints to connect to in order.
Therefore, we need to force deterministic results to ensure that we can
have deterministic tests when doing Web Connectivity QA.
This diff gives us the guarantee that we can have determinism.
Part of https://github.com/ooni/probe/issues/1803#issuecomment-957323297.
