The integration test that was broken was:
```
--- FAIL: TestCreateInvalidExperiment (0.35s)
experiment_integration_test.go:192: expected a nil builder here
```
While there improve the documentation of the ExperimentSession
and see there's a method that we are not using.
This diff is a cleanup that I come up with while working
on https://github.com/ooni/probe/issues/2184.
This diff modifies the engine package to make Experiment and
ExperimentBuilder interfaces rather than structs.
The previosuly existing structs are now named experiment{,Builder}.
This diff helps https://github.com/ooni/probe/issues/2184
because it allows us to write unit tests more easily.
There should be no functional change.
While there, I removed a bunch of deprecated functions, which were
unnecessarily complicate the implementation and could be easily
replaced by passing them a context.Context or context.Background().
This diff refactors how we set options for experiments to accept
in input an any value or a map[string]any, depending on which method
we choose to actually set options.
There should be no functional change, except that now we're not
guessing the type and then attempting to set the value of the selected
field: now, instead, we match the provided type and the field's type
as part of the same function (i.e., SetOptionAny).
This diff is functional to https://github.com/ooni/probe/issues/2184,
because it will allow us to load options from a map[string]any,
which will be part of the OONI Run v2 JSON descriptor.
If we didn't apply this change, we would only have been to set options
from a map[string]string, which is good enough as a solution for the
CLI but is definitely clumsy when you have to write stuff like:
```JSON
{
"options": {
"HTTP3Enabled": "true"
}
}
```
when you could instead more naturally write:
```JSON
{
"options": {
"HTTP3Enabled": true
}
}
```
This diff makes the implementation of the engine package more
abstract by changing HTTPClient() to return a model.HTTPClient
as opposed to returning an *http.Client.
Part of https://github.com/ooni/probe/issues/2184
## 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/2158
- [x] if you changed anything related how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request: https://github.com/ooni/spec/pull/250
## Description
This diff refactors the codebase to reimplement tlsping and tcpping
to use the step-by-step measurements style.
See docs/design/dd-003-step-by-step.md for more information on the
step-by-step measurement style.
This pull request publishes the step-by-step design document that I have been discussing with @hellais and @DecFox recently. Compared to the document that was approved, this one has been edited for readability.
While there, I figured it was also be beneficial to publish the few ooni/probe-cli related design documents we produced in the past, because they probably help someone to get acquainted with the codebase.
Reference issue for this pull request: https://github.com/ooni/probe/issues/2148
This diff addresses the following points of https://github.com/ooni/probe/issues/2135:
- [x] the `childResolver` type is useless and we can use `model.Resolver` directly;
- [x] we should use `model/mocks` instead of custom fakes;
- [x] we should not use `log.Log` rather we should use `model.DiscardLogger`;
- [x] make `timeLimitedLookup` easier to test with a `-short` tests;
- [x] ensure `timeLimitedLookup` returns as soon as its context expires regardless of the child resolver;
Subsequent diffs will address more points mentioned in there.
This diff forward ports 261d1a4cdc88522f6a8f63d6c540f51054566b28 to master
whose original commit message follows:
- - -
It's not working for me from a couple of places and also it does not
seem to be documented upstream, see:
https://docs.namebase.io/guides-1/resolving-handshake-1/hdns.io
This diff WILL need to be forwardported to master.
This diff refactors netx and netxlite to ensure we're not using
netxlite legacy names inside of netx.
To this end, we're cheating a bit. We're exposing a new factory to
get an unwrapped stdlib resolver rather than defining a legacy name
to export the private name of the same factory.
This is actually a fine place to stop, for now, the next and
netxlite refactoring at https://github.com/ooni/probe/issues/2121.
Before finishing the ongoing refactoring and leaving whatever
is left of netx in tree, I would like to restructure it so that
we'll have an easy time next time we need to modify it.
Currently, every functionality lives into the `netx.go` file and
we have a support file called `httptransport.go`.
I would like to reorganize by topic, instead. This would allow
future me to more easily perform topic-specific changes.
While there, improve `netx`'s documentation and duplicate some of
this documentation inside `internal/README.md` to provide pointers
to previous documentation, historical context, and some help to
understand the logic architecture of network extensions (aka `netx`).
Part of https://github.com/ooni/probe-cli/pull/396
Now that we have properly refactored the caching resolvers we can
move them into netxlite as optional resolvers created using the
proper abstract factories we just added.
This diff reduces the complexity and the code size of netx.
See https://github.com/ooni/probe/issues/2121.
For testability, replace most if-based construction logic with
calls to well-tested factories living in other packages.
While there, acknowledge that a bunch of types could now be private
and make them private, modifying the code to call the public
factories allowing to construct said types instead.
Part of https://github.com/ooni/probe/issues/2121
This diff modifies netx to stop using most netxlite resolver internals
but the internal function that creates a new, unwrapped system resolver,
which will be dealt with in a subsequent pull request.
See https://github.com/ooni/probe/issues/2121
1. Use the netxlite.NewHTTPTransport factory for creating a new
HTTP2 (and HTTP1) transport;
2. Recognize the netxlite.NewOOHTTPTransport has now become
an implementation detail so make it private;
3. Recognize that netxlite.NewHTTP3Transport should call
netxlite.WrapTransport so it returns the same typechain
returned by netxlite.NewHTTPTransport (modulo, of course,
the real underlying transport), so ensure that we are
calling netxlite.WrapTransport in NewHTTP3Transport;
4. Recognize that the table based constructor inside of
netx needs a logger to create HTTPTransport instances using
either netxlite.NewHTTP{,3}Transport so pass this argument
along and ensure it's not nil using a constructor inside
model that guarantees that;
5. Cleanup netx's tests to avoid type asserting on the
typechains returned by netxlite since we already test
that inside netxlite;
6. Recognize that now we can make more legacy names inside
of netxlite private because we don't need to use them
inside tests anymore (because of previous point).
Reference issue: https://github.com/ooni/probe/issues/2121
This diff modifies netx to use netxlite to build the TLSDialer.
Building the TLSDialer entails building a TLSHandshaker.
While there, hide netxlite names we don't want to be public
and change netx tests to test for functionality.
To this end, refactor filtering to provide an easier to
use TLS server. We don't need the complexity of proxying
rather we need to provoke specific errors.
Part of https://github.com/ooni/probe/issues/2121
This diff forward ports b606494db8a9293384efaf5c33a88601f6e1e2a6
to the main development branch.
Dnscheck is emitting progress and the experiment controller is
also emitting progress. This messes up the progress bar.
See https://github.com/ooni/probe/issues/2058#issuecomment-1141638067
* refactor: move tracex outside of engine/netx
Consistently with https://github.com/ooni/probe/issues/2121 and
https://github.com/ooni/probe/issues/2115, we can now move tracex
outside of engine/netx. The main reason why this makes sense now
is that the package is now changed significantly from the one
that we imported from ooni/probe-engine.
We have improved its implementation, which had not been touched
significantly for quite some time, and converted it to unit
testing. I will document tomorrow some extra work I'd like to
do with this package but likely could not do $soon.
* go fmt
* regen tutorials
The exercise already allowed me to notice issues such as fields not
being properly initialized by savers.
This is one of the last steps before moving tracex away from the
internal/netx package and into the internal package.
See https://github.com/ooni/probe/issues/2121
Tracex contained some fragile code that assembled HTTP measurements
from scattered events, which worked because we were sure we were
performing a single measurement at any given time.
This diff restructures the code to emit a transaction-start and a
transaction-done events only. We have basically removed all the other
events (which we were not using). We kept the transaction-start
though, because it may be useful to see it when reading events. In
any case, what matters here is that we're now using the transaction-done
event aline to generate the archival HTTP measurement.
Hence, the original issue has been addressed. We will possibly
do more refactoring in the future, but for now this seems sufficient.
Part of https://github.com/ooni/probe/issues/2121
The main issue I see inside tracex at the moment is that we
construct the HTTP measurement from separate events.
This is fragile because we cannot be sure that these events
belong to the same round trip. (Currently, they _are_ part
of the same round trip, but this is a fragile assumption and
it would be much more robust to dispose of it.)
To prepare for emitting a single event, it's imperative to
have two distinct fields for HTTP request and response headers,
which is the main contribution in this commit.
Then, we have a bunch of smaller changes including:
1. correctly naming 'response' the DNS response (instead of 'reply')
2. ensure we always use pointer receivers
Reference issue: https://github.com/ooni/probe/issues/2121
Rather than matching a string, match a type.
This is more robust considering future refactorings.
We're confident the names did not change in _this_ refactoring
because we're still testing the same strings in the tests.
Part of https://github.com/ooni/probe/issues/2121
Acknowledge that transports MAY be used in isolation (i.e., outside
of a Resolver) and add support for wrapping.
Ensure that every factory that creates an unwrapped type is named
accordingly to hopefully ensure there are no surprises.
Implement DNSTransport wrapping and use a technique similar to the
one used by Dialer to customize the DNSTransport while constructing
more complex data types (e.g., a specific resolver).
Ensure that the stdlib resolver's own "getaddrinfo" transport (1)
is wrapped and (2) could be extended during construction.
This work is part of my ongoing effort to bring to this repository
websteps-illustrated changes relative to netxlite.
Ref issue: https://github.com/ooni/probe/issues/2096
This diff modifies the system resolver to use a getaddrinf transport.
Obviously the transport is a fake, but its existence will allow us
to observe DNS events more naturally.
A lookup using the system resolver would be a ANY lookup that will
contain all the resolved IP addresses into the same response.
This change was also part of websteps-illustrated, albeit the way in
which I did it there was less clean than what we have here.
Ref issue: https://github.com/ooni/probe/issues/2096
Rather than passing functions to construct complex objects such
as Dialer and QUICDialer, pass interface implementations.
Ensure that a nil implementation does not cause harm.
Make Saver implement the correct interface either directly or
indirectly. We need to implement the correct interface indirectly
for TCP conns (or connected UDP sockets) because we have two
distinct use cases inside netx: observing just the connect event
and observing just the I/O events.
With this change, the construction of composed Dialers and
QUICDialers is greatly simplified and more obvious.
Part of https://github.com/ooni/probe/issues/2121
The code that is now into the tracex package was written a long
time ago, so let's start to make it more in line with the coding
style of packages that were written more recently.
I didn't apply all the changes I'd like to apply in a single diff
and for now I am committing just this diff.
Broadly, what we need to do is:
1. improve documentation
2. ~always use pointer receivers (object receives have the issue
that they are not mutable by accident meaning that you can mutate
them but their state do not change after the call returns, which
is potentially a source of bugs in case you later refactor to use
a pointer receiver, so always use pointer receivers)
3. ~always avoid embedding (let's say we want to avoid embedding
for types we define and it's instead fine to embed types that are
defined in the stdlib: if later we add a new method, we will not
see a broken build and we'll probably forget to add the new method
to all wrappers -- conversely, if we're wrapping rather than
embedding, we'll see a broken build and act accordingly)
4. prefer unit tests and group tests by type being tested rather
than using a flat structure for tests
There's a coverage slippage that I'll compensate in a follow-up diff where I'll focus on unit testing.
Reference issue: https://github.com/ooni/probe/issues/2121
This diff creates a new package under netx called tracex that
contains everything we need to perform measurements using events
tracing and postprocessing (which is the technique with which
we implement most network experiments).
The general idea here is to (1) create a unique package out of
all of these packages; (2) clean up the code a bit (improve tests,
docs, apply more recent code patterns); (3) move the resulting
code as a toplevel package inside of internal.
Once this is done, netx can be further refactored to avoid
subpackages and we can search for more code to salvage/refactor.
See https://github.com/ooni/probe/issues/2121
This diff modifies the construction of a dialer to allow one
to insert custom dialer wrappers into the dialers chain.
The point of the chain in which we allow custom wrappers is the
optimal one for connect, read, and write measurements.
This new design is better than the previous netx design since
we don't need to construct the whole chain manually now.
The work in this diff is part of the effort to make engine/netx
just a tiny wrapper around netxlite.
See https://github.com/ooni/probe/issues/2121.
This diff required us to move some code around, but no major
change actually happened, except better tests.
While there, I also slightly refactored ndt7's implementation and
removed the ProxyURL setting, which was actually unused.
See https://github.com/ooni/probe/issues/2121
This diff replaces engine/netx code with netxlite code in
the engine/session.go file. To this end, we needed to move
some code from engine/netx to netxlite. While there, we
did review and improve the unit tests.
A notable change in this diff is (or seems to be) that in
engine/session.go we're not filtering for bogons anymore so
that, in principle, we could believe a resolver returning
to us bogon IP addresses for OONI services. However, I did
not bother with changing bogons filtering because the
sessionresolver package is already filtering for bogons,
so it is actually okay to avoid doing that again the
session.go code. See:
https://github.com/ooni/probe-cli/blob/v3.15.0-alpha.1/internal/engine/internal/sessionresolver/resolvermaker.go#L88
There are two reference issues for this cleanup:
1. https://github.com/ooni/probe/issues/2115
2. https://github.com/ooni/probe/issues/2121
After https://github.com/ooni/probe-cli/pull/764, the build for
CGO_ENABLED=0 has been broken for miniooni:
https://github.com/ooni/probe-cli/runs/6636995859?check_suite_focus=true
Likewise, it's not possible to run tests with CGO_ENABLED=0.
To make tests work with `CGO_ENABLED=0`, I needed to sacrifice some
unit tests run for the CGO case. It is not fully clear to me what was happening
here, but basically `getaddrinfo_cgo_test.go` was compiled with CGO
being disabled, even though the ``//go:build cgo` flag was specified.
Additionally, @hellais previously raised a valid point in the review
of https://github.com/ooni/probe-cli/pull/698:
> Another issue we should consider is that, if I understand how
> this works correctly, depending on whether or not we have built
> with CGO_ENABLED=0 on or not, we are going to be measuring
> things in a different way (using our cgo inspired getaddrinfo
> implementation or using netgo). This might present issues when
> analyzing or interpreting the data.
>
> Do we perhaps want to add some field to the output data format that
> gives us an indication of which DNS resolution code was used to
> generate the the metric?
This comment is relevant to the current commit because
https://github.com/ooni/probe-cli/pull/698 is the previous
iteration of https://github.com/ooni/probe-cli/pull/764.
So, while fixing the build and test issues, let us also distinguish
between the CGO_ENABLED=1 and CGO_ENABLED=0 cases.
Before this commit, OONI used "system" to indicate the case where
we were using net.DefaultResolver. This behavior dates back to the
Measurement Kit days. While it is true that ooni/probe-engine and
ooni/probe-cli could have been using netgo in the past when we
said "system" as the resolver, it also seems reasonable to continue
to use "system" top indicate getaddrinfo.
So, the choice here is basically to use "netgo" from now on to
indicate the cases in which we were built with CGO_ENABLED=0.
This change will need to be documented into ooni/spec along with
the introduction of the `android_dns_cache_no_data` error.
## 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/2029
- [x] if you changed anything related how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request: https://github.com/ooni/spec/pull/242
Simone Basso
DecFox
Ain Ghazal