netx: spring-summer 2021 refactoring #1591
Assignees
Comments
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Jun 8, 2021
I am currently working on https://github.com/ooni/probe-engine/issues/725 with an eye on ooni/probe#1507. While approaching these issues, it seems that the `netx` codebase could use some consolidation (_coagula_) and some splitting (_solve_). The general idea of these changes is that I want to arrive to the situation where we have (1) a `New` factory method for each package under `netx` for which it makes sense (e.g., `dialer.New`, `tlsdialer.New`, `httptransport.New`), (2) a separate `Config` structure per package (e.g., `dialer.Config`) rather than having all the possible config into the same structure (3) part of the `urlgetter` code (and namely the low-level part) moved into the `netx` package. (See ooni/probe#1591.) There is too much bureaucracy around writing a new experiment. Much of this bureaucracy will go (it seems) if we do what I have said above. After that, we will end up that you run tests by using the top-level `netx` package. (In any case, I am not of course 100% sure about all the changes that will come after, but this comment seems enough to set a direction.) These are the changes in this pull request: * refactor(netx): extract tlsdialer from dialer We want these two packages to be separate. Dialer was doing too much before. Separating TLS dialing code into a separate package allows us to have a `tlsdialer.New` factory that clearly conveys the meaning. Also, this would allow us to much more clearly separate configuration and simplify reasoning on what each factory does. * refactor(engine): move `tlsx` package inside `netx` and merge the `gocertifi` package inside it It would be tempting to merge everything inside `tlsdialer` but the reality is that also the `quicdialer` package needs to use the same code, therefore, it seems more tidy to actually have some tls extensions in `netx`.
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Jun 8, 2021
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 ooni/probe#1591.
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Jun 8, 2021
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 ooni/probe#1591.
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Jun 8, 2021
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Jun 8, 2021
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Jun 8, 2021
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Jun 8, 2021
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Jun 8, 2021
The socks5 factory always returns a DialContext capable dialer. We just need to cast to obtain such a dialer. Also, the code will use the DialContext if passed a dialer that implements DialContext. Write a test that proves my point. Part of ooni/probe#1591.
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Jun 8, 2021
The socks5 factory always returns a DialContext capable dialer. We just need to cast to obtain such a dialer. Also, the code will use the DialContext if passed a dialer that implements DialContext. Write a test that proves my point. Part of ooni/probe#1591.
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Jun 9, 2021
The socks5 factory always returns a DialContext capable dialer. We just need to cast to obtain such a dialer. Also, the code will use the DialContext if passed a dialer that implements DialContext. Write a test that proves my point. Part of ooni/probe#1591.
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Jun 9, 2021
This follows the blueprint of `module.Config` and `nodule.New` described at ooni/probe#1591.
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Jun 9, 2021
* refactor(netx/dialer): hide implementation complexity This follows the blueprint of `module.Config` and `nodule.New` described at ooni/probe#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're working on improving errors in #1505 and we documented what is the issue with Windows errors at #1571 (comment). |
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Sep 5, 2021
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Sep 5, 2021
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Sep 5, 2021
Regenerate sources and make sure the tests pass. See ooni/probe#1591.
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Sep 5, 2021
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Sep 5, 2021
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
No functional change, as it's clearly obvious from the output. While there, also rename the generator for certifi. We are planning on merging errorsx into netxlite. The first step is to give different names to the code generating programs. See ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
This is a preliminary change before merging errorsx into netxlite. See ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
We will move the sane part of this package to i/netxlite/errorsx and we will move the rest to i/e/legacy/errorsx. What is the sane part? The sane part is error classifiers plus the definition of ErrWrapper. The rest, including the rules on how to decide whether an operation is major, are tricky and we should consider them legacy and replace them with rules that are more easy to understand and reason on. Part of ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
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 ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
…i#478) For consistency and also because the SafeErrorWrapperBuilder seems to be the building pattern than the original code assumed. New code should not use it, but I'd rather keep legacy code consistent formally and with its own original assumptions. In particular, it matters that SafeErrorWrapperBuilder assigns the most relevant operation that failed. We were not doing that when we were manually creating a new ErrWrapper. Part of ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
We need still to add similar wrappers to internal/netxlite but we will adopt a saner approach to error wrapping this time. See ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
No real functional change. A few are needed and they will come next. With this diff I just wanted to do cosmetic changes and documentation changes, to ensure this package is okay. See ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
This change makes the code more tidy and easier to read. No functional change, though. See ooni/probe#1591.
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
While there, add one more test checking for whether the internal CA bundle we use can actually be loaded. Part of ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
* refactor(netxlite): improve tests for http and http3 See ooni/probe#1591 * Update internal/netxlite/http3.go
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
They are now more readable. I'll do another pass and start separating integration testing from unit testing. I think we need to have some always on integration testing for netxlite that runs on macOS, linux, and windows. See ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
This simplifies serializing errors to `*string`. It did not occur to me before. It seems quite a nice improvement. Part of ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
Adapt other places where it was not using a logger to either choose a reasonable logger or disable logging for backwards compat. See ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
* netxlite: improve docs, tests, and code quality * better documentation * more strict testing of dialer (especially make sure we document the quirk in ooni/probe#1779 and we have tests to guarantee we don't screw up here) * introduce NewErrWrapper factory for creating errors so we have confidence we are creating them correctly Part of ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
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 ooni/oohttp@cba9b1c. Part of ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
There are a bunch of packages where we don't really need to depend on netx but we can use local definitions that describe what we are expecting from data structures we receive in input. This diff addresses one of such cases. Part of ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
…ni#501) While there, also change to pointer receiver and use internal testing for what are clearly unit tests. Part of ooni/probe#1591.
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
) While there, generally convert more code to internal testing and to using pointer receivers as well. Part of ooni/probe#1591.
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
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 ooni/probe#1591
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this issue
Mar 8, 2022
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 ooni/probe#1591
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We are going to touch
netxa bit in the following weeks with @kelmenhorst.Improving error handling
We are trying to improve the way in which we handle errors (see #1505).
We also want to be sure that we correctly detect all windows errors (see #1526 and #1514) among others.
Increasing packages isolation
The general idea of these changes is that I want to arrive to the situation where we have (1) a
Newfactory method for each package undernetxfor which it makes sense (e.g.,dialer.New,tlsdialer.New,httptransport.New), (2) a separateConfigstructure per package (e.g.,dialer.Config) rather than having all the possible config into the same structure (3) part of theurlgettercode (and namely the low-level part) moved into thenetxpackage.Remove selfcensor pkg
It adds significant complexity and is basically unused because we use Jafar. So, let us get rid of it.
Removing the failed operation
It seems we can compute the failed operation from network events. The way in which we also compute it with error wrapping adds significant complexity and is becoming a barrier towards more easily adapting and changing the code, because the whole machinery around the error wrapping is complex.
The text was updated successfully, but these errors were encountered: