We recently started moving core data structures inside of the
internal/model package as detailed in https://github.com/ooni/probe/issues/1885.
The chief reason to do that is to have a set of fundamental
shared data types to help us rationalize the codebase.
This specific diff moves internal/netx/archival's core data types
inside the internal/model package. While there, it also refactors the
existing tests to improve their quality. Additionally, we also added
an extra test to ensure `ArchivalHTTPBody` is an alias for
`ArchivalMaybeBinaryData`, which is required to ensure the
custom JSON serialization process works for it.
We're doing that because both internal/netx/archival and
internal/measurex define their own archival data structures.
We developed measurex using its own structures because it
allowed to iterate more quickly. Now that we have sketched
out measurex, the time has come to consolidate.
My overall aim is to spend a few more hours this week on
engineering measurex. This work is preliminary work before
we finish up both measurex and websteps.
We described this cleanup in https://github.com/ooni/probe/issues/1957.
This diff addresses two items of https://github.com/ooni/probe/issues/1956:
> - [ ] we can remove legacy names from `./internal/engine/netx/resolver/legacy.go`
>
> - [ ] we can remove `DialTLSContext` from `./internal/engine/netx/resolver/tls_test.go`
More cleanups may follow.
This is another cleanup point mentioned by https://github.com/ooni/probe/issues/1956.
While there, fix a bunch of comments in jafar that were incorrectly
referring to the netx package name.
This diff addresses another point of https://github.com/ooni/probe/issues/1956:
> - [ ] observe that we're still using a bunch of private interfaces for common interfaces such as the `Dialer`, so we can get rid of these private interfaces and always use the ones in `model`, which allows us to remove a bunch of legacy wrappers
Additional cleanups may still be possible. The more I cleanup, the more I see
there's extra legacy code we can dispose of (which seems good?).
This diff implements the first two cleanups defined at https://github.com/ooni/probe/issues/1956:
> - [ ] observe that `netxlite` and `netx` differ in error wrapping only in the way in which we set `ErrWrapper.Operation`. Observe that the code using `netxlite` does not care about such a field. Therefore, we can modify `netxlite` to set such a field using the code of `netx` and we can remove `netx` specific code for errors (which currently lives inside of the `./internal/engine/legacy/errorsx` package
>
> - [ ] after we've done the previous cleanup, we can make all the classifiers code private, since there's no code outside `netxlite` that needs them
A subsequent diff will address the remaining cleanup.
While there, notice that there are failing, unrelated obfs4 tests, so disable them in short mode. (I am confident these tests are unrelated because they fail for me when running test locally from the `master` branch.)
This commit completely removes the original netx implementation,
which was only used by `tor`, since this has changed in
https://github.com/ooni/probe-cli/pull/652.
The original netx implementation was my first attempt at performing
network measurements using Go. It started its life inside of the
https://github.com/ooni/netx repository. It was later merged into
the https://github.com/ooni/probe-engine repository. It finally
ended up into this repository when we merged probe-engine with it.
The main issue with the original implementation is that it was
a bit too complex and used channels where they were probably not
necessary. Because of that, later I introduced a second netx
implementation, which currently lives in ./internal/engine/netx.
The current netx implementation, the third one, lives in the
./internal/netxlite package. We are currently working to replace
the second implementation with the third one, but this is happening
at a slow pace. Also, the second implementation does not have big
maintenance concerns but it's just a bit too bureaucratic to use
since it involves creating lots of `Config` structures.
The reference issue is probably https://github.com/ooni/probe/issues/1688,
since this diff has been enabled by rewriting Tor to use `measurex`
(a library living on top of `netxlite`).
## 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
* [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.
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.
When preparing a tutorial for netxlite, I figured it is easier
to tell people "hey, this is the package you should use for all
low-level networking stuff" rather than introducing people to
a set of packages working together where some piece of functionality
is here and some other piece is there.
Part of https://github.com/ooni/probe/issues/1591
I have recently seen a data race related our way of
mutating the outgoing request to set the host header.
Unfortunately, I've lost track of the race output,
because I rebooted my Linux box before saving it.
Though, after inspecting why and and where we're mutating
outgoing requets, I've found that:
1. we add the host header when logging to have it logged,
which is not a big deal since we already emit the URL
rather than just the URL path when logging a request, and
so we can safely zap this piece of code;
2. as a result, in measurements we may omit the host header
but again this is pretty much obvious from the URL itself
and so it should not be very important (nonetheless,
avoid surprises and keep the existing behavior);
3. when the User-Agent header is not set, we default to
a `miniooni/0.1.0-dev` user agent, which is probably not
very useful anyway, so we can actually remove it.
Part of https://github.com/ooni/probe/issues/1733 (this diff
has been extracted from https://github.com/ooni/probe-cli/pull/506).
While there, modernize the way in which we run tests to avoid
depending on the fake files scattered around the tree and to
use some well defined mock structures instead.
Part of https://github.com/ooni/probe/issues/1591
I discovered which transport were used by apitool and made sure he gets the same transports now. While there, I discovered an issue with ooni/oohttp that has been fixed with cba9b1ce5e.
Part of https://github.com/ooni/probe/issues/1591
Adapt other places where it was not using a logger to either choose
a reasonable logger or disable logging for backwards compat.
See https://github.com/ooni/probe/issues/1591
We need still to add similar wrappers to internal/netxlite but we
will adopt a saner approach to error wrapping this time.
See https://github.com/ooni/probe/issues/1591
The legacy part for now is internal/errorsx. It will stay there until
I figure out whether it also needs some extra bug fixing.
The good part is now in internal/netxlite/errorsx and contains all the
logic for mapping errors. We need to further improve upon this logic
by writing more thorough integration tests for QUIC.
We also need to copy the various dialer, conn, etc adapters that set
errors. We will put them inside netxlite and we will generate errors in
a way that is less crazy with respect to the major operation. (The
idea is to always wrap, given that now we measure in an incremental way
and we don't measure every operation together.)
Part of https://github.com/ooni/probe/issues/1591
With this change, we are now able to change more dependent code to simplify
the way in which we create and manage resolvers.
See https://github.com/ooni/probe/issues/1591
Like before, do not touch the rest of the tree. Rather create
compatibility types declared as legacy.
We will soon be able to close idle connections for an HTTP3
transport using any kind of resolvers more easily.
See https://github.com/ooni/probe/issues/1591
This basically adapts already existing code inside websteps to
instead be into the netxlite package, where it belongs.
In the process, abstract the TLSDialer but keep a reference to the
previous name to avoid refactoring existing code (just for now).
While there, notice that the right name is CloseIdleConnections (i.e.,
plural not singular) and change the name.
While there, since we abstracted TLSDialer to be an interface, create
suitable factories for making a TLSDialer type from a Dialer and a
TLSHandshaker.
See https://github.com/ooni/probe/issues/1591
Like we did before for the resolver, a dialer should propagate the
request to close idle connections to underlying types.
See https://github.com/ooni/probe/issues/1591
We would like to refactor the code so that a DoH resolver owns the
connections of its underlying HTTP client.
To do that, we need first to incorporate CloseIdleConnections
into the Resolver model. Then, we need to add the same function
to all netxlite types that wrap a Resolver type.
At the same time, we want the rest of the code for now to continue
with the simpler definition of a Resolver, now called ResolverLegacy.
We will eventually propagate this change to the rest of the tree
and simplify the way in which we manage Resolvers.
To make this possible, we introduce a new factory function that
adapts a ResolverLegacy to become a Resolver.
See https://github.com/ooni/probe/issues/1591.
## Description
This PR continues the refactoring of `netx` under the following principles:
1. do not break the rest of the tree and do not engage in extensive tree-wide refactoring yet
2. move under `netxlite` clearly related subpackages (e.g., `iox`, `netxmocks`)
3. move into `internal/netxlite/internal` stuff that is clearly private of `netxlite`
4. hide implementation details in `netxlite` pending new factories
5. refactor `tls` code in `netxlite` to clearly separate `crypto/tls` code from `utls` code
After each commit, I run `go test -short -race ./...` locally. Each individual commit explains what it does. I will squash, but this operation will preserve the original commit titles, so this will give further insight on each step.
## Commits
* refactor: rename netxmocks -> netxlite/mocks
Part of https://github.com/ooni/probe/issues/1591
* refactor: rename quicx -> netxlite/quicx
See https://github.com/ooni/probe/issues/1591
* refactor: rename iox -> netxlite/iox
Regenerate sources and make sure the tests pass.
See https://github.com/ooni/probe/issues/1591.
* refactor(iox): move MockableReader to netxlite/mocks
See https://github.com/ooni/probe/issues/1591
* refactor(netxlite): generator is an implementation detail
See https://github.com/ooni/probe/issues/1591
* refactor(netxlite): separate tls and utls code
See https://github.com/ooni/probe/issues/1591
* refactor(netxlite): hide most types but keep old names as legacy
With this change we avoid breaking the rest of the tree, but we start
hiding some implementation details a bit. Factories will follow.
See https://github.com/ooni/probe/issues/1591
* refactor: cleaner way of passing a UDPConn around
Also part of https://github.com/ooni/probe/issues/1505
* Update internal/engine/netx/quicdialer/connectionstate.go
I needed to add some tests as integration tests due to circular
imports, but this is ~fine because we quite likely want many
integration tests in the errorsx package anyway.
Part of https://github.com/ooni/probe/issues/1505.
With this factory, we want to construct ourselves the TLS dialer
so that we can use a dialer wrapper that always sets timeouts when
reading, addressing https://github.com/ooni/probe/issues/1609.
As a result, we cannot immediately replace the i/e/netx factory
for creating a new HTTP transport, since the functions signatures
are not directly compatible.
Refactoring is part of https://github.com/ooni/probe/issues/1505.
This diff is part of https://github.com/ooni/probe/issues/1505.
You will notice that I have not adapted all the (great) tests we had
previously. They should live at another layer, and namely the one that
deals with performing measurements.
When I'm refactoring such a layer I'll ensure those tests that I have
not adapted here are reintroduced into the tree.
Auto-configure every relevant TLS field as close as possible to
where it's actually used.
As a side effect, add support for mocking the creation of a TLS
connection, which should possibly be useful for uTLS?
Work that is part of https://github.com/ooni/probe/issues/1505
The BogonResolver relied on its wrapper resolver to pass along the
list of addresses _and_ the error. But the idiomatic thing to do is
often to return `nil` when there is an error.
I broke this very fragile assumption in https://github.com/ooni/probe-cli/pull/399.
I could of course fix it, but this assumption is clearly wrong
and we should not allow such fragile code in the tree.
We are not using BogonIsError much in the tree. The only place in
which we're using it for measuring seems to be dnscheck.
It may be that this surprising behavior was what caused the issue at
https://github.com/ooni/probe/issues/1510 in the first place.
Regardless, let's remove fragile code and adjust the test that was
failing. Also that test is quick so it can run in `-short` mode.
Spotted while working on https://github.com/ooni/probe/issues/1505.
