-
Notifications
You must be signed in to change notification settings - Fork 54
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.
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
feat: add the openvpn experiment #1585
Merged
Merged
Changes from 43 commits
Commits
Show all changes
56 commits
Select commit
Hold shift + click to select a range
5c0a415
stub for openvpn experiment
ainghazal 06624b0
add hardcoded endpoint
ainghazal aaf6995
update archival format
ainghazal d4b65c3
wip
ainghazal c4bb706
hardcode credentials as step zero
ainghazal 87bbe21
flatten test keys
ainghazal 8c2e23e
wip
ainghazal 54c9510
wip
ainghazal 1bc0764
include openvpn options in handshake result
ainghazal 567ed14
add fields to archival struct for handshake result
ainghazal 5974d03
wip: input
ainghazal 0852d1a
api call to fetch credentials
ainghazal 8bb671b
use the newer endpoint, provider in url
ainghazal 1ea507f
wip: fetch config, process input as part of inputloader
ainghazal 46b40f1
parse input in new format
ainghazal 0c6a062
cache config from api
ainghazal 199f105
cache per-provider config
ainghazal 6ada20f
do not parse base64 in creds from api
ainghazal accaf97
tests
ainghazal bb25311
update docs
ainghazal dfd8c83
move archival structs to internal/model
ainghazal d6d7ed8
move list of enabled providers to expriment/openvpn
ainghazal e291c0e
do not use credentials
ainghazal 8934898
remove test file
ainghazal 25c98a9
go mod tidy
ainghazal 4ef2b80
add expectations for openvpn
ainghazal 779ab03
add vpn suffix, there seems to be a mismatch in the provider name
ainghazal 47aa87d
add tests for inputloader
ainghazal 4a68032
update after httpclientx api change
ainghazal c11fbce
fix after rebase
ainghazal f169fb6
test stub
ainghazal 337230c
adapt after merge/api change
ainghazal 0a24b30
add tests for probeservices/openvpn
ainghazal 2f64b08
minor style changes for clarity and correctness
ainghazal 4bc13c8
debug possible deadlock
ainghazal 8fcfaac
prefix annoyance
ainghazal 9d7a6be
debug
ainghazal 32f3f00
complete hacks to bypass vpn provider name in backend
ainghazal a55816c
fix tests for bad parsing
ainghazal 6ab511e
fix bad comparison
ainghazal 07daac4
revert removal of lock
ainghazal 410126a
pass output to test to avoid panic
ainghazal 2a6fc25
force single provider for now
ainghazal ec8396a
add changes after code review
ainghazal b909756
move the lock after instantiating the client
ainghazal b8fbd82
unflatten the failure
ainghazal 8c5fa11
abort experiment if no urls returned
ainghazal 330f1aa
Merge remote-tracking branch 'ooni/master' into feat/openvpn-2024-3
ainghazal 1ce6835
Merge branch 'master' into feat/openvpn-2024-3
bassosimone 6377ba8
add stub to receive options from oonirun descriptors
ainghazal 9b27cc1
use measurexlite.NewFailure
ainghazal 196d584
update tests
ainghazal 1a60013
update test after change in default endpoints if api fails
ainghazal 0fae492
Merge branch 'master' into feat/openvpn-2024-3
bassosimone 9fac70c
fix(registry): update openvpn registration
bassosimone 5afb8bd
Update internal/engine/session.go
bassosimone File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import ( | |
"net/url" | ||
|
||
"github.com/apex/log" | ||
"github.com/ooni/probe-cli/v3/internal/experiment/openvpn" | ||
"github.com/ooni/probe-cli/v3/internal/fsx" | ||
"github.com/ooni/probe-cli/v3/internal/model" | ||
"github.com/ooni/probe-cli/v3/internal/registry" | ||
|
@@ -28,6 +29,8 @@ var ( | |
// introduce this abstraction because it helps us with testing. | ||
type InputLoaderSession interface { | ||
CheckIn(ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) | ||
FetchOpenVPNConfig(ctx context.Context, | ||
provider, cc string) (*model.OOAPIVPNProviderConfig, error) | ||
} | ||
|
||
// InputLoaderLogger is the logger according to an InputLoader. | ||
|
@@ -299,6 +302,21 @@ func (il *InputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]mode | |
|
||
// loadRemote loads inputs from a remote source. | ||
func (il *InputLoader) loadRemote(ctx context.Context) ([]model.OOAPIURLInfo, error) { | ||
switch registry.CanonicalizeExperimentName(il.ExperimentName) { | ||
case "openvpn": | ||
// TODO(ainghazal): given the semantics of the current API call, in an ideal world we'd need to pass | ||
// the desired provider here, if known. We only have one provider for now, so I'm acting like YAGNI. Another | ||
// option is perhaps to coalesce all the known providers per proto into a single API call and let client | ||
// pick whatever they want. | ||
// This will likely improve after Richer Input is available. | ||
return il.loadRemoteOpenVPN(ctx) | ||
default: | ||
return il.loadRemoteWebConnectivity(ctx) | ||
} | ||
} | ||
|
||
// loadRemoteWebConnectivity loads webconnectivity inputs from a remote source. | ||
func (il *InputLoader) loadRemoteWebConnectivity(ctx context.Context) ([]model.OOAPIURLInfo, error) { | ||
config := il.CheckInConfig | ||
if config == nil { | ||
// Note: Session.CheckIn documentation says it will fill in | ||
|
@@ -317,6 +335,43 @@ func (il *InputLoader) loadRemote(ctx context.Context) ([]model.OOAPIURLInfo, er | |
return reply.WebConnectivity.URLs, nil | ||
} | ||
|
||
// loadRemoteOpenVPN loads openvpn inputs from a remote source. | ||
func (il *InputLoader) loadRemoteOpenVPN(ctx context.Context) ([]model.OOAPIURLInfo, error) { | ||
// VPN Inputs do not match exactly the semantics expected from [model.OOAPIURLInfo], | ||
// since OOAPIURLInfo is oriented towards webconnectivity, | ||
// but we force VPN targets in the URL and ignore all the other fields. | ||
// There's still some level of impedance mismatch here, since it's also possible that | ||
// the user of the library wants to use remotes by unknown providers passed via cli options, | ||
// oonirun etc; in that case we'll need to extract the provider annotation from the URLs. | ||
urls := make([]model.OOAPIURLInfo, 0) | ||
|
||
// The openvpn experiment contains an array of the providers that the API knows about. | ||
// We try to get all the remotes from the API for the list of enabled providers. | ||
for _, provider := range openvpn.APIEnabledProviders { | ||
reply, err := il.vpnConfig(ctx, provider) | ||
if err != nil { | ||
break | ||
} | ||
// here we're just collecting all the inputs. we also cache the configs so that | ||
// each experiment run can access the credentials for a given provider. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see where you're caching the configs in the following two lines There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you're right - cacheing happens in the session method. updated the comment. |
||
for _, input := range reply.Inputs { | ||
urls = append(urls, model.OOAPIURLInfo{URL: input}) | ||
} | ||
} | ||
|
||
if len(urls) <= 0 { | ||
// loadRemote returns ErrNoURLsReturned at this point for webconnectivity, | ||
// but for OpenVPN we want to return a sensible default to be | ||
// able to probe some endpoints even in very restrictive environments. | ||
// Do note this means that you have to provide valid credentials | ||
// by some other means. | ||
for _, endpoint := range openvpn.DefaultEndpoints { | ||
urls = append(urls, model.OOAPIURLInfo{URL: endpoint.AsInputURI()}) | ||
} | ||
} | ||
return urls, nil | ||
} | ||
|
||
// checkIn executes the check-in and filters the returned URLs to exclude | ||
// the URLs that are not part of the requested categories. This is done for | ||
// robustness, just in case we or the API do something wrong. | ||
|
@@ -335,6 +390,15 @@ func (il *InputLoader) checkIn( | |
return &reply.Tests, nil | ||
} | ||
|
||
// vpnConfig fetches vpn information for the configured providers | ||
func (il *InputLoader) vpnConfig(ctx context.Context, provider string) (*model.OOAPIVPNProviderConfig, error) { | ||
reply, err := il.Session.FetchOpenVPNConfig(ctx, provider, "XX") | ||
if err != nil { | ||
return nil, err | ||
} | ||
return reply, nil | ||
} | ||
|
||
// preventMistakes makes the code more robust with respect to any possible | ||
// integration issue where the backend returns to us URLs that don't | ||
// belong to the category codes we requested. | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to stop at the first error, you should return an error here. Looking at the context of the code, it seems to me the semantics to want is to ignore (and maybe warn?) on error. To this end, you should continue here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tx for pointing this out! this for loop is not really used atm, so I'll go with returning the error