Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

riseupvpn: future improvements #1432

Closed
2 tasks
bassosimone opened this issue Apr 2, 2021 · 2 comments
Closed
2 tasks

riseupvpn: future improvements #1432

bassosimone opened this issue Apr 2, 2021 · 2 comments
Assignees
@bassosimone
Labels
priority/low

Comments

@bassosimone
Copy link
Contributor

bassosimone commented Apr 2, 2021 •
edited
Loading

We originally used this issue to specify the following improvements:

  • specific error for API failure (the endpoint is misconfigured?)
  • specific error if JSON does not parse (ditto)

In reality, in 2023-10 I used an issue more generally as the reference for improving riseupvpn.

See #1432 (comment) for a discussion of the original objectives and what we have chosen to do instead, including references to follow up issues.
@bassosimone bassosimone self-assigned this Apr 2, 2021
@bassosimone bassosimone mentioned this issue Apr 2, 2021
Merged
bassosimone added a commit to ooni/probe-cli that referenced this issue Oct 11, 2023
@bassosimone
refactor(riseupvpn): rename structs, change progress, bump version
0df2d7f 
This diff renames some structs, changes the code emitting the progress,
and bumps the experiment version. In my previous commit, I said I did
bump the version number but that was not actually the case.

This diff has been extracted from #1125.

This diff is part of ooni/probe#1432
@bassosimone bassosimone mentioned this issue Oct 11, 2023
Merged
bassosimone added a commit to ooni/probe-cli that referenced this issue Oct 11, 2023
@bassosimone @cyBerta
refactor(riseupvpn): rename structs, change progress, bump version (#…
4828c58 
…1361)

This diff renames some structs, changes the code emitting the progress,
and bumps the experiment version. In my previous commit, I said I did
bump the version number but that was not actually the case.

This diff has been extracted from
#1125.

This diff is part of ooni/probe#1432

---------

Co-authored-by: cyBerta <[email protected]>
bassosimone added a commit to ooni/probe-cli that referenced this issue Oct 11, 2023
@bassosimone
refactor(riseupvpn): handle failing API and simplify test keys
dda1339 
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.
@bassosimone bassosimone mentioned this issue Oct 11, 2023
Merged
bassosimone added a commit to ooni/probe-cli that referenced this issue Oct 11, 2023
@bassosimone @cyBerta
refactor(riseupvpn): handle failing API and simplify test keys (#1363)
db70616 
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]>
bassosimone added a commit to ooni/probe-cli that referenced this issue Oct 11, 2023
@bassosimone
feat(riseupvpn): include bootstrap in progress
3b5260e 
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.
@bassosimone bassosimone mentioned this issue Oct 11, 2023
Merged
@bassosimone
Copy link
Contributor Author

bassosimone commented Oct 11, 2023 •
edited
Loading

Of the two improvements mentioned above:

  • specific error for API failure (the endpoint is misconfigured?)

The former has not been implemented, because we think we want to submit anyway. The crux of the matter has been to stop classifying the measurement inside the probe (ooni/probe-cli#1360) and the next logical step is to introduce more lax measurements classification in the backend (ooni/backend#745).

  • specific error if JSON does not parse (ditto)

Because in ooni/probe-cli#1363 we said we wanted to use richer input (#2559) to serve targets to probes, this seems unnecessary, since this work would be done on the backend side.

@bassosimone
Copy link
Contributor Author

Since I used this issue as a container for "riseupvpn improvements" and we have improvements and we have a more clear set of follow-up issues, I feel okay closing this issue now.

bassosimone added a commit to ooni/probe-cli that referenced this issue Oct 11, 2023
@bassosimone
feat(riseupvpn): include bootstrap in progress (#1364)
2947ede 
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.
bassosimone added a commit to ooni/probe-cli that referenced this issue Oct 11, 2023
@bassosimone @cyBerta
[backport] refactor(riseupvpn): rename structs, change progress, bump…
cafc118 
… version

This diff backports #1361 to the release/3.19 branch.

This diff renames some structs, changes the code emitting the progress,
and bumps the experiment version. In my previous commit, I said I did
bump the version number but that was not actually the case.

This diff has been extracted from
#1125.

This diff is part of ooni/probe#1432

---------

Co-authored-by: cyBerta <[email protected]>
bassosimone added a commit to ooni/probe-cli that referenced this issue Oct 11, 2023
@bassosimone @cyBerta
[backport] refactor(riseupvpn): handle failing API and simplify test …
3989fd4 
…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]>
bassosimone added a commit to ooni/probe-cli that referenced this issue Oct 11, 2023
@bassosimone
[backport] feat(riseupvpn): include bootstrap in progress
a1b7f60 
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.
Murphy-OrangeMud pushed a commit to Murphy-OrangeMud/probe-cli that referenced this issue Feb 13, 2024
@bassosimone @cyBerta @Murphy-OrangeMud
refactor(riseupvpn): rename structs, change progress, bump version (o…
e95e8d8 
…oni#1361)

This diff renames some structs, changes the code emitting the progress,
and bumps the experiment version. In my previous commit, I said I did
bump the version number but that was not actually the case.

This diff has been extracted from
ooni#1125.

This diff is part of ooni/probe#1432

---------

Co-authored-by: cyBerta <[email protected]>
Murphy-OrangeMud pushed a commit to Murphy-OrangeMud/probe-cli that referenced this issue Feb 13, 2024
@bassosimone @cyBerta @Murphy-OrangeMud
refactor(riseupvpn): handle failing API and simplify test keys (ooni#…
3fec1d6 
…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]>
Murphy-OrangeMud pushed a commit to Murphy-OrangeMud/probe-cli that referenced this issue Feb 13, 2024
@bassosimone @Murphy-OrangeMud
feat(riseupvpn): include bootstrap in progress (ooni#1364)
3f145a4 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bassosimone bassosimone

Labels
priority/low
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@bassosimone