refactor(riseupvpn): handle failing API and simplify test keys #1363

bassosimone · 2023-10-11T15:33:39Z

This diff incorporates part of what has been implemented by @cyBerta in #1125 in response to my review as well as additional changes based on my own feelings about what is correct to do here.

Compared to the original diff, these are the changes that I implemented:

I have omitted the work to fetch from riseup geo service and figure out the correct gateways to test. The main reason for not including this body of work has been to reduce the size of the diff and the amount of code to deal with.
I modified the logic related to failures in fetching the CA and communicating with riseup services. The test fails immediately if we cannot fetch the proper CA or we cannot contact riseup services. I did not feel comfortable disabling the CA to access riseup services and connecting to the TCP endpoints discovered w/o CA verification.
In the test keys, I renamed api_failure to api_failures because I do not think it's optimal to keep the same name while the type has changed from *string to []string.

The spirit of the changes is not directly compatible with what we discussed with @cyBerta. The main difference is in my decision to fail early in case we miss the preconditions. As I wrote in #1125 (review), I think we should be using richer input (and start with its simplest form) to provision to probes the data they need to perform this experiment. By provisioning the data ourselves, we remove the coupling between getting the CA, accessing riseup services to get information on what gateways we should measure, and measure the gateways, which makes the experiment several orders of magnitude more robust.

Unfortunately, I do not have time, in this cycle, to perform all this richer input work. We'll try again for 3.20.

This work is part of ooni/probe#1432.

While there, I forced null callbacks when performing the CA fetch and contacting riseup services, otherwise we end up printing a non-monotonic progress status. Admittedly, also omitting to provide progress about these two operations is bad, but I think we won't be able to provide monotonic progress until we know what we should fetch in advance.

Co-authored-by: cyBerta [email protected]

@cyBerta

This diff incorporates part of what has been implemented by @cyBerta in #1125 in response to my review. I have omitted the work to use the geo service to figure out which are the correct gateways to test for now, which allows me to simplify the diff that I am committing to the master branch. The spirit of the changes is the following: 1. reduce the test keys to the minimum required by riseup to process the experiment results; 2. skip TLS verification if we cannot fetch the CA certificate and unconditionally fetch information about gateways. The major difference between this diff and the above-mentioned pull request is that I did not feel good to keep the `api_failure` name with a different type, which seems like a very breaking change, and instead I introduced a new type called `api_failures`. This work is part of ooni/probe#1432.

This diff modifies riseupvpn to include bootstrap in progress. We define as riseupvpn's bootstrap the process of fetching the CA and the various JSON files needed to find out the gateways to measure. I previously stopped including bootstrap in progress in #1363, which I did because the progress was not monotonic. This diff introduces helpers that make it very obvious to emit monotonic bootstrap through the ./internal/progress package. This work is part of ooni/probe#1432.

@cyBerta

…keys This diff backports #1363 to the release/3.19 branch. This diff incorporates part of what has been implemented by @cyBerta in #1125 in response to my review as well as additional changes based on my own feelings about what is correct to do here. Compared to the original diff, these are the changes that I implemented: 1. I have omitted the work to fetch from riseup geo service and figure out the correct gateways to test. The main reason for not including this body of work has been to reduce the size of the diff and the amount of code to deal with. 2. I modified the logic related to failures in fetching the CA and communicating with riseup services. The test fails immediately if we cannot fetch the proper CA or we cannot contact riseup services. I did not feel comfortable disabling the CA to access riseup services and connecting to the TCP endpoints discovered w/o CA verification. 3. In the test keys, I renamed `api_failure` to `api_failures` because I do not think it's optimal to keep the same name while the type has changed from `*string` to `[]string`. The spirit of the changes is not directly compatible with what we discussed with @cyBerta. The main difference is in my decision to fail early in case we miss the preconditions. As I wrote in #1125 (review), I think we should be using richer input (and start with its simplest form) to provision to probes the data they need to perform this experiment. By provisioning the data ourselves, we remove the coupling between getting the CA, accessing riseup services to get information on what gateways we should measure, and measure the gateways, which makes the experiment several orders of magnitude more robust. Unfortunately, I do not have time, in this cycle, to perform all this richer input work. We'll try again for 3.20. This work is part of ooni/probe#1432. While there, I forced null callbacks when performing the CA fetch and contacting riseup services, otherwise we end up printing a non-monotonic progress status. Admittedly, also omitting to provide progress about these two operations is bad, but I think we won't be able to provide monotonic progress until we know what we should fetch in advance. --------- Co-authored-by: cyBerta <[email protected]>

This diff backports #1364 to the release/3.19 branch. This diff modifies riseupvpn to include bootstrap in progress. We define as riseupvpn's bootstrap the process of fetching the CA and the various JSON files needed to find out the gateways to measure. I previously stopped including bootstrap in progress in #1363, which I did because the progress was not monotonic. This diff introduces helpers that make it very obvious to emit monotonic bootstrap through the ./internal/progress package. This work is part of ooni/probe#1432.

@cyBerta

…1363) This diff incorporates part of what has been implemented by @cyBerta in ooni#1125 in response to my review as well as additional changes based on my own feelings about what is correct to do here. Compared to the original diff, these are the changes that I implemented: 1. I have omitted the work to fetch from riseup geo service and figure out the correct gateways to test. The main reason for not including this body of work has been to reduce the size of the diff and the amount of code to deal with. 2. I modified the logic related to failures in fetching the CA and communicating with riseup services. The test fails immediately if we cannot fetch the proper CA or we cannot contact riseup services. I did not feel comfortable disabling the CA to access riseup services and connecting to the TCP endpoints discovered w/o CA verification. 3. In the test keys, I renamed `api_failure` to `api_failures` because I do not think it's optimal to keep the same name while the type has changed from `*string` to `[]string`. The spirit of the changes is not directly compatible with what we discussed with @cyBerta. The main difference is in my decision to fail early in case we miss the preconditions. As I wrote in ooni#1125 (review), I think we should be using richer input (and start with its simplest form) to provision to probes the data they need to perform this experiment. By provisioning the data ourselves, we remove the coupling between getting the CA, accessing riseup services to get information on what gateways we should measure, and measure the gateways, which makes the experiment several orders of magnitude more robust. Unfortunately, I do not have time, in this cycle, to perform all this richer input work. We'll try again for 3.20. This work is part of ooni/probe#1432. While there, I forced null callbacks when performing the CA fetch and contacting riseup services, otherwise we end up printing a non-monotonic progress status. Admittedly, also omitting to provide progress about these two operations is bad, but I think we won't be able to provide monotonic progress until we know what we should fetch in advance. --------- Co-authored-by: cyBerta <[email protected]>

This diff modifies riseupvpn to include bootstrap in progress. We define as riseupvpn's bootstrap the process of fetching the CA and the various JSON files needed to find out the gateways to measure. I previously stopped including bootstrap in progress in ooni#1363, which I did because the progress was not monotonic. This diff introduces helpers that make it very obvious to emit monotonic bootstrap through the ./internal/progress package. This work is part of ooni/probe#1432.

bassosimone requested a review from hellais as a code owner October 11, 2023 15:33

bassosimone added 4 commits October 11, 2023 19:09

further modify the experiment

f7114c0

x

8230542

x

9d67289

trigger github, maybe?

cb257ef

bassosimone mentioned this pull request Oct 11, 2023

riseupvpn: relax scoring logic ooni/backend#745

Closed

bassosimone merged commit db70616 into master Oct 11, 2023
7 of 9 checks passed

bassosimone deleted the issue/1432 branch October 11, 2023 18:05

bassosimone mentioned this pull request Oct 11, 2023

cli: release 3.19 ooni/probe#2524

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(riseupvpn): handle failing API and simplify test keys #1363

refactor(riseupvpn): handle failing API and simplify test keys #1363

bassosimone commented Oct 11, 2023 •

edited

Loading

refactor(riseupvpn): handle failing API and simplify test keys #1363

refactor(riseupvpn): handle failing API and simplify test keys #1363

Conversation

bassosimone commented Oct 11, 2023 • edited Loading

bassosimone commented Oct 11, 2023 •

edited

Loading