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.)
## 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
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).
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
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
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.
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
What do I mean by pivoting? Netx is currently organized by row:
```
| dialer | quicdialer | resolver | ...
saving | | | | ...
errorwrapping | | | | ...
logging | | | | ...
mocking/sys | | | | ...
```
Every row needs to implement saving, errorwrapping, logging, mocking (or
adapting to the system or to some underlying library).
This causes cross package dependencies and, in turn, complexity. For
example, we need the `trace` package for supporting saving.
And `dialer`, `quickdialer`, et al. need to depend on such a package.
The same goes for errorwrapping.
This arrangement further complicates testing. For example, I am
currently working on https://github.com/ooni/probe/issues/1505 and
I realize it need to repeat integration tests in multiple places.
Let's say instead we pivot the above matrix as follows:
```
| saving | errorwrapping | logging | ...
dialer | | | | ...
quicdialer | | | | ...
logging | | | | ...
mocking/sys | | | | ...
...
```
In this way, now every row contains everything related to a specific
action to perform. We can now share code without relying on extra
support packages. What's more, we can write tests and, judding from
the way in which things are made, it seems we only need integration
testing in `errorwrapping` because it's where data quality matters
whereas, in all other cases, unit testing is fine.
I am going, therefore, to proceed with these changes and "pivot"
`netx`. Hopefully, it won't be too painful.
* refactor(netx/dialer): hide implementation complexity
This follows the blueprint of `module.Config` and `nodule.New`
described at https://github.com/ooni/probe/issues/1591.
* fix: ndt7 bug where we were not using the right resolver
* fix(legacy/netx): clarify irrelevant implementation change
* fix: improve comments
* fix(hhfm): do not use dialer.New b/c it breaks it
Unclear to me why this is happening. Still, improve upon the
previous situation by adding a timeout.
It does not seem a priority to look into this issue now.
We already configure a timeout in the underlying dialer, hence
there's no point in keeping the TimeoutDialer around.
Part of https://github.com/ooni/probe/issues/1507
We're currently use jafar for QA and jafar is a better mechanism,
even though it is not portable outside of Linux.
This self censorship mechanism was less cool and added a bunch
of (also cognitive) complexity to netx.
If we ever want to go down a self censorship like road, we probably
want to do as little work as possible in the problem and as much
work as possible inside a helper like jafar.
Part of https://github.com/ooni/probe/issues/1591.
* fix(webconnectivity): allow measuring https://1.1.1.1
There were two issues preventing us from doing so:
1. in netx, the address resolver was too later in the resolver
chain. Therefore, its result wasn't added to the events.
2. when building the DNSCache (in httpget.go), we didn't consider
the case where the input is an address. We need to treat this
case specially to make sure there is no DNSCache.
See https://github.com/ooni/probe/issues/1376.
* fix: add unit tests for code making the dnscache
* fix(netx): make sure all tests pass
* chore: bump webconnectivity version
This is how I did it:
1. `git clone https://github.com/ooni/probe-engine internal/engine`
2. ```
(cd internal/engine && git describe --tags)
v0.23.0
```
3. `nvim go.mod` (merging `go.mod` with `internal/engine/go.mod`
4. `rm -rf internal/.git internal/engine/go.{mod,sum}`
5. `git add internal/engine`
6. `find . -type f -name \*.go -exec sed -i 's@/ooni/probe-engine@/ooni/probe-cli/v3/internal/engine@g' {} \;`
7. `go build ./...` (passes)
8. `go test -race ./...` (temporary failure on RiseupVPN)
9. `go mod tidy`
10. this commit message
Once this piece of work is done, we can build a new version of `ooniprobe` that
is using `internal/engine` directly. We need to do more work to ensure all the
other functionality in `probe-engine` (e.g. making mobile packages) are still WAI.
Part of https://github.com/ooni/probe/issues/1335
