Skip to content

Commit

Permalink
address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
ainghazal committed Jun 24, 2024
1 parent 9a649d6 commit 12752f3
Show file tree
Hide file tree
Showing 5 changed files with 19 additions and 29 deletions.
13 changes: 1 addition & 12 deletions internal/experiment/openvpn/endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package openvpn
import (
"errors"
"fmt"
"sort"
"testing"
"time"

Expand Down Expand Up @@ -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")
Expand All @@ -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",
Expand Down
19 changes: 10 additions & 9 deletions internal/experiment/openvpn/openvpn.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions internal/experiment/openvpn/openvpn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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) {
Expand Down
9 changes: 6 additions & 3 deletions internal/experiment/openvpn/richerinput.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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",
}
Expand All @@ -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,
Expand Down
3 changes: 0 additions & 3 deletions internal/experiment/openvpn/richerinput_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 12752f3

Please sign in to comment.