diff --git a/internal/experiment/openvpn/endpoint_test.go b/internal/experiment/openvpn/endpoint_test.go index 2cd7ba8af7..bfd3eb6027 100644 --- a/internal/experiment/openvpn/endpoint_test.go +++ b/internal/experiment/openvpn/endpoint_test.go @@ -3,7 +3,6 @@ package openvpn import ( "errors" "fmt" - "sort" "testing" "time" @@ -270,16 +269,6 @@ func Test_endpoint_String(t *testing.T) { } } -func Test_endpointList_Shuffle(t *testing.T) { - shuffled := DefaultEndpoints.Shuffle() - sort.Slice(shuffled, func(i, j int) bool { - return shuffled[i].IPAddr < shuffled[j].IPAddr - }) - if diff := cmp.Diff(shuffled, DefaultEndpoints); diff != "" { - t.Error(diff) - } -} - func Test_isValidProvider(t *testing.T) { if valid := isValidProvider("riseupvpn"); !valid { t.Fatal("riseup is the only valid provider now") @@ -289,7 +278,7 @@ func Test_isValidProvider(t *testing.T) { } } -func Test_mergeVPNConfig(t *testing.T) { +func Test_newVPNConfig(t *testing.T) { tracer := vpntracex.NewTracer(time.Now()) e := &endpoint{ Provider: "riseupvpn", diff --git a/internal/experiment/openvpn/openvpn.go b/internal/experiment/openvpn/openvpn.go index a51f8d9277..e2d8a9fd4c 100644 --- a/internal/experiment/openvpn/openvpn.go +++ b/internal/experiment/openvpn/openvpn.go @@ -95,21 +95,20 @@ func (tk *TestKeys) AllConnectionsSuccessful() bool { } // Measurer performs the measurement. -type Measurer struct { -} +type Measurer struct{} // NewExperimentMeasurer creates a new ExperimentMeasurer. func NewExperimentMeasurer() model.ExperimentMeasurer { - return Measurer{} + return &Measurer{} } // ExperimentName implements model.ExperimentMeasurer.ExperimentName. -func (m Measurer) ExperimentName() string { +func (m *Measurer) ExperimentName() string { return testName } // ExperimentVersion implements model.ExperimentMeasurer.ExperimentVersion. -func (m Measurer) ExperimentVersion() string { +func (m *Measurer) ExperimentVersion() string { return testVersion } @@ -174,6 +173,8 @@ func (m *Measurer) connectAndHandshake( } } +// TimestampsFromHandshake returns the t0, t and duration of the handshake events. +// If the passed events are a zero-len array, all of the results will be zero. func TimestampsFromHandshake(events []*vpntracex.Event) (float64, float64, float64) { var ( t0 float64 @@ -236,6 +237,10 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { return err } + // TODO(ainghazal): validate we have valid config for each endpoint. + // TODO(ainghazal): validate hostname is a valid IP (ipv4 or 6) + // TODO(ainghazal): decide what to do if we have expired certs (abort one measurement or abort the whole experiment?) + // 3. build openvpn config from endpoint and options openvpnConfig, err := newOpenVPNConfig(handshakeTracer, sess.Logger(), endpoint, config) if err != nil { @@ -253,10 +258,6 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { // 5. assign the testkeys measurement.TestKeys = tk - // TODO(ainghazal): validate we have valid config for each endpoint. - // TODO(ainghazal): validate hostname is a valid IP (ipv4 or 6) - // TODO(ainghazal): decide what to do if we have expired certs (abort one measurement or abort the whole experiment?) - // Note: if here we return an error, the parent code will assume // something fundamental was wrong and we don't have a measurement // to submit to the OONI collector. Keep this in mind when you diff --git a/internal/experiment/openvpn/openvpn_test.go b/internal/experiment/openvpn/openvpn_test.go index 77f71411df..6c25c90ae0 100644 --- a/internal/experiment/openvpn/openvpn_test.go +++ b/internal/experiment/openvpn/openvpn_test.go @@ -221,7 +221,7 @@ func TestVPNInput(t *testing.T) { func TestMeasurer_FetchProviderCredentials(t *testing.T) { t.Run("Measurer.FetchProviderCredentials calls method in session", func(t *testing.T) { - m := openvpn.NewExperimentMeasurer().(openvpn.Measurer) + m := openvpn.NewExperimentMeasurer().(*openvpn.Measurer) sess := makeMockSession() _, err := m.FetchProviderCredentials( @@ -234,7 +234,7 @@ func TestMeasurer_FetchProviderCredentials(t *testing.T) { t.Run("Measurer.FetchProviderCredentials raises error if API calls fail", func(t *testing.T) { someError := errors.New("unexpected") - m := openvpn.NewExperimentMeasurer().(openvpn.Measurer) + m := openvpn.NewExperimentMeasurer().(*openvpn.Measurer) sess := makeMockSession() sess.MockFetchOpenVPNConfig = func(context.Context, string, string) (*model.OOAPIVPNProviderConfig, error) { diff --git a/internal/experiment/openvpn/richerinput.go b/internal/experiment/openvpn/richerinput.go index 4e6e30834c..050673b359 100644 --- a/internal/experiment/openvpn/richerinput.go +++ b/internal/experiment/openvpn/richerinput.go @@ -103,8 +103,8 @@ func (tl *targetLoader) Load(ctx context.Context) ([]model.ExperimentTarget, err return targets, nil } -// TODO(ainghazal): we might want to get both the BaseURL and the HTTPClient from the session, -// and then deal with the openvpn-specific API calls ourselves within the boundaries of the experiment. +// TODO(https://github.com/ooni/probe/issues/2755): make the code that fetches experiment private +// and let the common code export just the bare minimum to make this possible. func (tl *targetLoader) loadFromBackend(ctx context.Context) ([]model.ExperimentTarget, error) { if tl.options.Provider == "" { tl.options.Provider = defaultProvider @@ -126,7 +126,8 @@ func (tl *targetLoader) loadFromBackend(ctx context.Context) ([]model.Experiment for _, input := range apiConfig.Inputs { config := &Config{ - // Auth and Cipher are hardcoded for now. + // TODO(ainghazal): Auth and Cipher are hardcoded for now. + // Backend should provide them as richer input; and if empty we can use these as defaults. Auth: "SHA512", Cipher: "AES-256-GCM", } @@ -135,6 +136,8 @@ func (tl *targetLoader) loadFromBackend(ctx context.Context) ([]model.Experiment config.SafeCA = apiConfig.Config.CA config.SafeCert = apiConfig.Config.Cert config.SafeKey = apiConfig.Config.Key + case AuthUserPass: + // TODO(ainghazal): implement (surfshark, etc) } targets = append(targets, &Target{ URL: input, diff --git a/internal/experiment/openvpn/richerinput_test.go b/internal/experiment/openvpn/richerinput_test.go index 83782d2bed..2fc0042469 100644 --- a/internal/experiment/openvpn/richerinput_test.go +++ b/internal/experiment/openvpn/richerinput_test.go @@ -142,9 +142,6 @@ func TestTargetLoaderLoad(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { // create a target loader using the given config - // - // note that we use a default test input for results predictability - // since the static list may change over time tl := &targetLoader{ loader: tc.loader, options: tc.options,