engine: adapt to use bridges #2531

bassosimone · 2023-09-11T10:31:58Z

Update: 2023-10-06 we're now naming the functionality bridge because it looks like a better name.

This issue is about adapting the ooniprobe engine (i.e., the ./internal directory in ooni/probe-cli) to use beacons. We define as "beacon" a host that relies traffic to the OONI backend and the Web Connectivity test helper. We want to switch to use beacons for the two following reasons. First, we want to reduce the number of interactions required to run Web Connectivity. Second, we want to increase the reliability of the probes.

While working on this issue, we should also probably consider the following two side quests. First, the engine itself consists of several packages that need to interoperate and we can probably simplify the structure of the code by merging several packages together and better documenting the interaction between all the moving pieces. Second, we are currently using the same code paths for communicating with the backend and for measuring. However, the requirements are quite different. For example, using an HTTP/HTTPS proxy is more complex than it should because of this design. Additionally, the policies that we need for implementing beacons are different than we one we use for measuring. For example, the HTTP transport we are currently using has specific constraints required by the ./internal/legacy/netx package that prevent us from having parallel outstanding HTTP requests, which in turns limits our flexibility and performance. Therefore, it may be smarter to have a separate HTTP transport using a different configuration, to better meet our design goals with a conceptually simpler codebase. These two side quests are the reasons why I am tagging this issue also as "techdebt": there may be techdebt and in such case we'll pay for it.

Working on this issue proper would first entail studying the existing codebase carefully to determine the proper places where to intervene and make ameliorating changes. Once we have clarity about this, we will modify the implementation to make it (a) not more complex than it currently is and (b) suitable for using beacons in production.

We will consider this issue "fixed" when we'll have merged support for beacons into the main development branch.

Update (2023-10-02) You can read a summary of the work done this issue at #2531 (comment).

The text was updated successfully, but these errors were encountered:

I am starting to look into how to support beacons into the codebase as explained by ooni/probe#2531. The first issue that I stumbled upon is that there is excessive coupling between packages at the lower-levels, which we can reduce with easy refactoring, and which would help reasoning on what to do to address the underlying issue. This diff is the first minor step towards cleaning up the code a bit: we are moving a functionality that pertains to logging from measurexlite to logx (which is where it should be). While there, I noticed I needed to run `go mod tidy` to update the `go.mod` and `go.sum` files, which were outdated.

## 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: ooni/probe#2531 - [x] if you changed anything related to how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request: N/A - [x] if you changed code inside an experiment, make sure you bump its version number: N/A ## Description I am starting to look into how to support beacons into the codebase as explained by ooni/probe#2531. The first issue that I stumbled upon is that there is excessive coupling between packages at the lower-levels, which we can reduce with easy refactoring, and which would help reasoning on what to do to address the underlying issue. This diff is the first minor step towards cleaning up the code a bit: we are moving a functionality that pertains to logging from measurexlite to logx (which is where it should be). While there, I noticed I needed to run `go mod tidy` to update the `go.mod` and `go.sum` files, which were outdated.

I am reasoning about ooni/probe#2531 and thus reading the codebase. I stumbled upon this inconsistency. While there, I noticed some documentation also needed updating.

This code modifies how we construct netxlite's system resolver such that public functions use the *Netx equivalents. The general idea for which I am pushing here is to have additional clarity about dependencies, to better analyze the requirements of non measuring code for ooni/probe#2531.

This diff modifies how we construct netxlite's system resolver such that public functions use the *Netx equivalents. While there, recognize that there wasn't enough testing for the optional wrappers provided by `model.DNSTransportWrapper` and that we are not using this functionality. So, rather than writing new tests for this functionality, we can actually just drop it and simplify the codebase. While there, recognize that `netxlite.WrapDNSTransport` could easily become private. While there, recognize that `./legacy/netx` needs lots of public function being exported by `netxlite` but we don't need to expose that many implementation details to new code that will be using `*Netx`. So, make sure all the new methods we create for `*Netx` are actually private methods. (Ideally, the API surface of `netxlite` should be smaller; we would not be able to get there for quite some time, but we can at least avoid increasing the API surface.) The general idea for which I am pushing here is to have additional clarity about dependencies, to better analyze the requirements of non measuring code for ooni/probe#2531.

This diff is like 7224984 except that we are converting the system dialer constructors to use *Netx rather than converting the system resolver. While there, realize that DialerSystem could become private. Part of ooni/probe#2531

This diff is similar to 07a048c but here we use *Netx to create UDP sockets. While there, recognize that UDP code needs its own files and should not live inside quic{,_test}.go. Part of ooni/probe#2531

This diff is similar to 07a048c but here we use *Netx to create UDP sockets. While there, recognize that UDP code needs its own files and should not live inside quic{,_test}.go. Part of ooni/probe#2531.

This diff is like 8a5edc2 but here we use *Netx to create QUIC dialers. The general idea of this patchset is to ensure we're not using duplicate code for constructing netxlite types, which is good to do now, because we're about to introduce new netxlite types for the network with which we communicate with the OONI backend. Reference issue: ooni/probe#2531

This diff is like 8a5edc2 but here we use *Netx to create QUIC dialers. While there, recognize that `WrapQUICDialer` could become a private function. The general idea of this patchset is to ensure we're not using duplicate code for constructing netxlite types, which is good to do now, because we're about to introduce new netxlite types for the network with which we communicate with the OONI backend. Reference issue: ooni/probe#2531

This diff is like 50279a7 but uses *Netx to create TLS handshakers. The general idea of this patchset is to ensure we're not using duplicate code for constructing netxlite types, which is good to do now, because we're about to introduce new netxlite types for the network with which we communicate with the OONI backend. Reference issue: ooni/probe#2531

This diff is like 50279a7 but uses *Netx to create HTTP transports. The general idea of this patchset is to ensure we're not using duplicate code for constructing netxlite types, which is good to do now, because we're about to introduce new netxlite types for the network with which we communicate with the OONI backend. Reference issue: ooni/probe#2531

This diff is like 50279a7 but uses *Netx to create HTTP transports. While there, annotate a test that is notoriously flaky on Windows. The general idea of this patchset is to ensure we're not using duplicate code for constructing netxlite types, which is good to do now, because we're about to introduce new netxlite types for the network with which we communicate with the OONI backend. Reference issue: ooni/probe#2531

Now that we've more or less reached the point where we wanted to be with ooni/probe#2531, let's spend some time to refactor the implementation, now that we know very well how it works, such that modifying it in the future would be easier. The first order of business here seems to hide implementation details and get rid of too many HTTPSDialer prefixes which only cause confusion when looking at the available structs. Here, in particular, we deal with the static-policy API. To this end, we also need to more some tests around because they need internals. Also, while there, add a top-level description to some files, so that newcomers have an idea of what each file is about.

The dns-policy API is the API where we use the DNS to generate tactics for dialing. Now we have made it private. Also, we have renamed files. Most of this package is related to HTTPS dialing anyway, so we don't need to be that obsessive and add the HTTPSDialer / httpsdialer prefix everywhere. While there, also hide and move the "null" stats tracker, which is clearly a private implementation detail. Part of ooni/probe#2531

What needs to be public is the network API, while all the rest can and should instead be private. Part of ooni/probe#2531

We need this functionality to ask the stats questions like "what are all the available tactics for www.example.com:443 along with their stats?". In turn, this information is needed to implement a new policy that uses existing stats to choose what to do. While there, rename files to make it clear which files contain policies and which contain state trackers. Useful because upcoming changes will add files containing state-based policy. While there, stop being paranoid about cloning a nil map to a nil map, acknowledge this is not important and making it round trip correctly would require more code we don't need, and simply adjust tests to accept nil and empty map as equal. Part of ooni/probe#2531

This policy first checks the stats and sorts the entries for which we have the highest nonzero success rate. Then, we use an underlying policy to produce additional suitable tactics for the HTTPS dialer. There is a check preventing the same tactic from being emitted twice. This means that, if we emit a tactic looking into the stats, we're not going to emit the same exact tactic again when using our beacons policy generator. Also, because we try using stable sorting, and assuming some tactics work, we would most likely continue using the tactics that work, until something bad happens and we need to try and see whether other tactics work. While there, rename statstracker{,_test}.go to statsmanager{,_test}.go because the type inside it is called "state manager". The state tracker is an abstract concept only used by the https dialer code. Part of ooni/probe#2531

This commit modifies the https-dialer policy we create to take into account stats to generate tactics. As of this commit, the overall policy is as follows: 1. if `$OONI_HOME/$engine_dir/httpsdialerstatic.conf` exists and contains entries for the endpoint's domain (e.g., "www.example.com:443"), then we unconditionally use it to generate tactics, otherwise; 2. we generate tactics using existing stats and filter only the tactics that are less than one week old (filtering done when loading from the kvstore) and have worked at least once, otherwise; 3. we generate tactics using known beacons and candidate SNIs with the extra caveat that we're not going to generate a tactic we have generate already in the previous step, otherwise; 4. we use whatever resolver is configured to lookup for the domain name and generate tactics doing the boring thing of using the resolved IP addrs along with the SNI being equal to the original domain. Note that this diff fixes a previously untested for bug caused by trying to obtain statistics for an unknow endpoint domain. Part of ooni/probe#2531

This diff attempts to improve the code quality of enginenetx by identifying cases where the code could be made to crash with specially written input and adds regress tests and checks to avoid these kind of crashes to happen. While there, perform a code review and improve comments and naming. Part of ooni/probe#2531

This diff modifies how we construct a `*Network` to use a very simple DNS-only policy when using a proxy. We don't need to try anything fancy when we're using a proxy because we assume the proxy knows how to do circumvention and we don't to get in the proxy way. While there, recognize that the static policy was named very similarly to the stats policy, so rename the former user policy, such that there is more lexicographical difference. Part of ooni/probe#2531

This diff adjusts code inside the enginenetx package such that we gracefully deal with nil values in more cases. Part of ooni/probe#2531

If we're running the enginenetx code inside an environment that sprays ~random IP addresses during DNS lookup, we're going to end up using quite a large amount of statistics. While this amount of statistics could potentially be great in general, we need to be mindful of memory and disk occupation as well as of the overhead of serializing and deserializing. To address these concerns, add the arbitrary rule that we don't want more than ten domain-endpoint entries and implement code to zap old and/or always-failing entries. The code will run when loading the entries, when serializing them, and periodically while the `*Network` is up and running. See ooni/probe#2531

Provided that we get correct IP addresses, which is a big IF, we can try to avoid using offending SNIs when using the THs. Part of ooni/probe#2531

This diff is yak shaving for a subsequent diff that attempts to mitigate potential IP addresses leaks caused by using happy eyeballs more aggressively in the codebase. The general idea is that we previously had a netxlite implementation that gave IPv4 preference over IPv6, meaning that we would end up using IPv4 in most cases and very rarely IPv6. As part of my work to make beacons possible, I have introduced happy eyeballs, which means we may sometimes use IPv4 instead of IPv6 if we adopt happy eyeballs more widely. This fact makes it more likely that we include the IPv6 address of a probe when we know its IPv4 address or the other way around into measurements. To mitigate this possible issue before it actually is possible (note that I have not yet changed how we discover the probe IP), I am going to proactively modify the serialization of HTTP bodies and headers to scrub IP endpoints unconditionally. However, to do this, I need to decouple the scrubber package from model such that internal/model/archival.go can use the scrubber package. Part of ooni/probe#2531

This set of changes is the continuation of what I started doing in ooni@4a86c54 to revamp out scrubbing capabilities. Reorganizing tests is pure refactoring. Exposing ScrubBytes is because I think we can rationalize a bit what we scrub on the fly in the `internal/model/archival.go` file and we need to indeed start scrubbing byte arrays. I'm doing this work as part of ooni/probe#2531

There are cases where we know we have binary data in input and we want the output to be binary data using the dictionary encoding like `{"format":"base64","data":"..."}`. Such cases are, for example, DNS messages bodies. There is no need for us to pass through the MaybeArchivalBinaryData in such cases. Additionally, this makes MaybeArchivalBinaryData a bit more complex than it probably needs to be. What's more, ArchivalBinaryData is for when you do not require to scrub the data. I want to introduce new data types that automatically perform scrubbing when they're used. But this puts even more pressure on MaybeArchivalBinaryData, and hence this commit, to start differentiating between: 1. always binary data vs maybe binary data 2. no scrubbing required vs scrubbing required The rationale for doing this set of changes has been explained in ooni#1319. The reference issue is ooni/probe#2531. For now, this commit just adds the new type and tests for it without using the type, which we'll do later once we have added better marshal/unmarshal testing for the interested types.

I am about to modify this structure to unconditionally use ArchivalBinaryData rather than ArchivalMaybeBinaryData, following the plan that I explained in ooni#1319. Before doing that, I want MORE test coverage for ArchivalTLSOrQUICHandshakeResult. Part of ooni/probe#2531

It did not originally occur to me, but now it's clear that we can avoid using a struct to wrap the data type. It just suffices to use the new type. I think this is better in terms of writing code because the only two things we need to do are: 1. make sure we have serialization and unserialization tests; 2. use the correct data type in the struct. For all intents and purposes the ArchivalBinaryData is just a special kind of []byte attached to custom marshal/unmarshal rules. Part of ooni/probe#2531

…ni#1324) This diff modifies model.ArchivalBinaryData to always use ArchivalBinaryData for representing certificates rather than passing through the more complex (and unneeded in this case!) model.ArchivalMaybeBinaryData. Closes ooni/probe#2165 Part of ooni/probe#2531

This type is ~equivalent to ArchivalMaybeBinaryData but designed to hold a string, do less type conversions, and be easier to use. The intent is to replace the ArchivalMaybeBinaryData type with this type. For now, let us introduce the new type and its tests. Part of ooni/probe#2531

My aim here is to have marshal/unmarshal tests for all the toplevel data structures inside the internal/model/archival.go file. Once I have done that, I can have additional confidence about changing the structure fields and simplifying the code. All of this is preparatory work to automatically scrub HTTP measurements which is something we should really do before continuing to improve the boostrap process in light of ooni/probe#2531. I have also added currently skipped tests for other structures as placeholder, but I did not want to submit a too large PR, hence for now we will just have tests for ArchivalDNSLookupResult. More PRs will follow and they will add additional unit tests.

My aim here is to have marshal/unmarshal tests for all the toplevel data structures inside the internal/model/archival.go file. Once I have done that, I can have additional confidence about changing the structure fields and simplifying the code. All of this is preparatory work to automatically scrub HTTP measurements which is something we should really do before continuing to improve the boostrap process in light of ooni/probe#2531.

ooni#1330) We want the tests w/ binary data to make sure we can safely replace model.ArchivalMaybeBinaryData. We want the tests w/ IP addresses because once we replace the above mentioned type, we'll implement aggressive scrubbing. Part of ooni/probe#2531

) This diff modifies the model for HTTP requests and responses to use the ArchivalBinaryString type to represent bodies. The tests I have introduced in previous commits are still passing, so we can have good confidence that this change is actually WAI. Part of ooni/probe#2531

I was trying to make sure the code was using a new definition of headers based on ArchivalMaybeBinaryString, but I stumbled upon a roadblock where there were too many code paths settings headers. I did not feel comfortable changing multiple code paths, so here is instead a diff that unifies setting headers. The slightly tricky part of this diff has been the requirement that we preserve headers' capitalization for hhfm. The catch seems to be that we should not use http.Header for setting the headers in hhfm, but rather pass through map[string][]string, which is not attached case normalization setters. I think we're fine in this department because we already had test cases and I added a bunch more test cases. After this diff is merged, I can resume with my plan to make headers use ArchivalMaybeBinaryString, which, in turn, is functional to automatically scrub headers, which is something we would like to happen in light of the introduction of more happy eyeballs code due to ooni/probe#2531.

This diff modifies the headers-as-a-map representation such that we use ArchivalMaybeBinaryString instead of ArchivalMaybeBinaryData. The overall idea is to migrate all users of the latter to the former, such that we can remove the latter and implement extra scrubbing for all ArchivalMaybeBinaryString users. Part of ooni/probe#2531

This commit changes the list representation of HTTP headers to use the ArchivalMaybeBinaryString type. Now that we have migrated most representations of HTTP headers to use this new type, we can finally implement scrubbing. As a reminder, we still need to migrate the ./legacy/measurex implementation, however that one is a bit annoying because there are no tests at all for measurex 🤦🤦🤦🤦🤦🤦🤦. That said, given that ./legacy/measurex is deprecated and it is only used by the tor experiment, I think leaving it untouched would probably be the right thing to do here. Part of ooni/probe#2531

) This commit modifies ArchivalMaybeBinaryData to unconditionally scrub IP addresses and endpoints. It also renames the struct ArchivalScrubbedMaybeBinaryData to underline that it's scrubbing now. We're using ArchivalScrubbedMaybeBinaryData to represent HTTP headers and bodies collected by OONI measurements. With this commit merged, I am now much less concerned about the potential unintended effects of aggressively using happy eyeballs, which we started introducing as part of ooni/probe#2531. In other words, the rest of the refactoring of the OONI bootstrap could proceed a bit faster.

This diff concludes most of the development work associated with ooni/probe#2531. We move the nearly unused ArchivalMaybeBinaryData to the internal/legacy/legacymodel package to clearly mark it deprecated. Finishing removing this struct is a task that is documented by ooni/probe#2543.

They're quack like bridges, OONI bridges. Also, name the config file from the PoV of the user. Part of ooni/probe#2531.

bassosimone added enhancement improving existing code or new feature testing priority/high ooni/probe-engine techdebt This issue describes technical debt labels Sep 11, 2023

bassosimone self-assigned this Sep 11, 2023

bassosimone mentioned this issue Sep 11, 2023

refactor(measurexlite): move OperationLogger to logx ooni/probe-cli#1246

Merged

4 tasks

bassosimone mentioned this issue Sep 11, 2023

cleanup(netxlite): QUICListener -> UDPListener ooni/probe-cli#1247

Merged

bassosimone mentioned this issue Sep 11, 2023

refactor(netxlite): use *Netx for the system resolver ooni/probe-cli#1248

Merged

bassosimone mentioned this issue Sep 12, 2023

refactor(netxlite): use *Netx for the system dialer ooni/probe-cli#1249

Merged

bassosimone mentioned this issue Sep 12, 2023

refactor(netxlite): use *Netx for creating UDP sockets ooni/probe-cli#1250

Merged

bassosimone mentioned this issue Sep 12, 2023

refactor(netxlite): use *Netx for creating QUIC dialers ooni/probe-cli#1251

Merged

bassosimone mentioned this issue Sep 12, 2023

refactor(netxlite): use *Netx for creating TLS handshakers ooni/probe-cli#1252

Merged

bassosimone mentioned this issue Sep 12, 2023

refactor(netxlite): use *Netx for creating HTTP transports ooni/probe-cli#1253

Merged

Murphy-OrangeMud pushed a commit to Murphy-OrangeMud/probe-cli that referenced this issue Feb 13, 2024

fix: rename beacons to bridges (ooni#1349)

aeecd5d

They're quack like bridges, OONI bridges. Also, name the config file from the PoV of the user. Part of ooni/probe#2531.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

engine: adapt to use bridges #2531

engine: adapt to use bridges #2531

bassosimone commented Sep 11, 2023 •

edited

Loading

engine: adapt to use bridges #2531

engine: adapt to use bridges #2531

Comments

bassosimone commented Sep 11, 2023 • edited Loading

bassosimone commented Sep 11, 2023 •

edited

Loading