more tests

internal/engine/inputloader.go

@@ -346,7 +346,7 @@ func (il *InputLoader) loadRemoteOpenVPN(ctx context.Context) ([]model.OOAPIURLI
 	for _, provider := range openvpnDefaultProviders {
 		reply, err := il.vpnConfig(ctx, provider)
 		if err != nil {
-			return nil, err
+			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.
@@ -364,7 +364,6 @@ func (il *InputLoader) loadRemoteOpenVPN(ctx context.Context) ([]model.OOAPIURLI
 		for _, endpoint := range openvpn.DefaultEndpoints {
 			urls = append(urls, model.OOAPIURLInfo{URL: endpoint.AsInputURI()})
 		}
-		return nil, ErrNoURLsReturned
 	}
 	return urls, nil
 }

internal/experiment/openvpn/endpoint.go

@@ -11,6 +11,7 @@ import (
 
 	vpnconfig "github.com/ooni/minivpn/pkg/config"
 	vpntracex "github.com/ooni/minivpn/pkg/tracex"
+	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
 var (
@@ -184,7 +185,11 @@ func isValidProvider(provider string) bool {
 // To obtain that, we merge the endpoint specific configuration with base options.
 // Base options are hardcoded for the moment, for comparability among different providers.
 // We can add them to the OONI API and as extra cli options if ever needed.
-func getOpenVPNConfig(tracer *vpntracex.Tracer, endpoint *endpoint, creds *vpnconfig.OpenVPNOptions) (*vpnconfig.Config, error) {
+func getOpenVPNConfig(
+	tracer *vpntracex.Tracer,
+	logger model.Logger,
+	endpoint *endpoint,
+	creds *vpnconfig.OpenVPNOptions) (*vpnconfig.Config, error) {
 	// TODO(ainghazal): use merge ability in vpnconfig.OpenVPNOptions merge (pending PR)
 	provider := endpoint.Provider
 	if !isValidProvider(provider) {
@@ -193,6 +198,7 @@ func getOpenVPNConfig(tracer *vpntracex.Tracer, endpoint *endpoint, creds *vpnco
 	baseOptions := defaultOptionsByProvider[provider]
 
 	cfg := vpnconfig.NewConfig(
+		vpnconfig.WithLogger(logger),
 		vpnconfig.WithOpenVPNOptions(
 			&vpnconfig.OpenVPNOptions{
 				// endpoint-specific options.
@@ -210,9 +216,9 @@ func getOpenVPNConfig(tracer *vpntracex.Tracer, endpoint *endpoint, creds *vpnco
 				Key:  creds.Key,
 			},
 		),
-		vpnconfig.WithHandshakeTracer(tracer))
+		vpnconfig.WithHandshakeTracer(tracer),
+	)
 
-	// TODO: sanity check (Remote, Port, Proto etc + missing certs)
 	return cfg, nil
 }
 

internal/experiment/openvpn/endpoint_test.go

@@ -284,7 +284,7 @@ func Test_getVPNConfig(t *testing.T) {
 		Key:  []byte("key"),
 	}
 
-	cfg, err := getOpenVPNConfig(tracer, e, creds)
+	cfg, err := getOpenVPNConfig(tracer, nil, e, creds)
 	if err != nil {
 		t.Fatalf("did not expect error, got: %v", err)
 	}
@@ -333,7 +333,7 @@ func Test_getVPNConfig_with_unknown_provider(t *testing.T) {
 		Cert: []byte("cert"),
 		Key:  []byte("key"),
 	}
-	_, err := getOpenVPNConfig(tracer, e, creds)
+	_, err := getOpenVPNConfig(tracer, nil, e, creds)
 	if !errors.Is(err, ErrInvalidInput) {
 		t.Fatalf("expected invalid input error, got: %v", err)
 	}

internal/experiment/openvpn/openvpn.go

@@ -231,79 +231,54 @@ func (m *Measurer) GetCredentialsFromOptionsOrAPI(
 
 }
 
-// Run implements model.ExperimentMeasurer.Run.
-// A single run expects exactly ONE input (endpoint), but we can modify whether
-// to test different transports by settings options.
-func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
-	callbacks := args.Callbacks
-	measurement := args.Measurement
-	sess := args.Session
+// mergeOpenVPNConfig attempts to get credentials from Options or API, and then
+// constructs a [vpnconfig.Config] instance after merging the credentials passed by options or API response.
+// It also returns an error if the operation fails.
+func (m *Measurer) mergeOpenVPNConfig(
+	ctx context.Context,
+	sess model.ExperimentSession,
+	endpoint *endpoint,
+	tracer *vpntracex.Tracer) (*vpnconfig.Config, error) {
 
-	endpoint, err := parseEndpoint(measurement)
+	logger := sess.Logger()
+
+	credentials, err := m.GetCredentialsFromOptionsOrAPI(ctx, sess, endpoint.Provider)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
-	tk := NewTestKeys()
-	sess.Logger().Infof("Probing endpoint %s", endpoint.String())
-
-	// TODO:  separate pre-connection checks
-	connResult, err := m.connectAndHandshake(ctx, int64(1), time.Now(), sess, endpoint)
+	openvpnConfig, err := getOpenVPNConfig(tracer, logger, endpoint, credentials)
 	if err != nil {
-		sess.Logger().Warn("Fatal error while attempting to connect to endpoint, aborting!")
-		return err
-	}
-	if connResult != nil {
-		tk.AddConnectionTestKeys(connResult)
+		return nil, err
 	}
-	tk.Success = tk.AllConnectionsSuccessful()
-
-	callbacks.OnProgress(1.0, "All endpoints probed")
-	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
-	// are writing new experiments!
-	return nil
+	// TODO: sanity check (Remote, Port, Proto etc + missing certs)
+	return openvpnConfig, nil
 }
 
 // connectAndHandshake dials a connection and attempts an OpenVPN handshake using that dialer.
-func (m *Measurer) connectAndHandshake(ctx context.Context, index int64, zeroTime time.Time, sess model.ExperimentSession, endpoint *endpoint) (*SingleConnection, error) {
-
-	logger := sess.Logger()
+func (m *Measurer) connectAndHandshake(
+	ctx context.Context,
+	zeroTime time.Time,
+	index int64,
+	logger model.Logger,
+	endpoint *endpoint,
+	openvpnConfig *vpnconfig.Config,
+	handshakeTracer *vpntracex.Tracer) (*SingleConnection, error) {
 
 	// create a trace for the network dialer
 	trace := measurexlite.NewTrace(index, zeroTime)
 
 	dialer := trace.NewDialerWithoutResolver(logger)
 
+	var failure string
 	// create a vpn tun Device that attempts to dial and performs the handshake
-	handshakeTracer := vpntracex.NewTracerWithTransactionID(zeroTime, index)
-
-	// TODO -- move to outer function ------
-	credentials, err := m.GetCredentialsFromOptionsOrAPI(ctx, sess, endpoint.Provider)
-	if err != nil {
-		return nil, err
-	}
-
-	openvpnConfig, err := getOpenVPNConfig(handshakeTracer, endpoint, credentials)
-	if err != nil {
-		return nil, err
-	}
-	// TODO -- move to outer function ------
-
 	tun, err := tunnel.Start(ctx, dialer, openvpnConfig)
-
-	var failure string
 	if err != nil {
 		failure = err.Error()
 	}
-	defer tun.Close()
+	if tun != nil {
+		defer tun.Close()
+	}
 
 	handshakeEvents := handshakeTracer.Trace()
 	port, _ := strconv.Atoi(endpoint.Port)
@@ -347,15 +322,63 @@ func (m *Measurer) connectAndHandshake(ctx context.Context, index int64, zeroTim
 	}, nil
 }
 
-// TODO: get cached from session instead of fetching every time
 func (m *Measurer) FetchProviderCredentials(
 	ctx context.Context,
 	sess model.ExperimentSession,
 	provider string) (*model.OOAPIVPNProviderConfig, error) {
-	// TODO(ainghazal): do pass country code, can be useful to orchestrate campaigns specific to areas
+	// TODO(ainghazal): pass real country code, can be useful to orchestrate campaigns specific to areas.
 	config, err := sess.FetchOpenVPNConfig(ctx, provider, "XX")
 	if err != nil {
 		return &model.OOAPIVPNProviderConfig{}, err
 	}
 	return config, nil
 }
+
+// Run implements model.ExperimentMeasurer.Run.
+// A single run expects exactly ONE input (endpoint), but we can modify whether
+// to test different transports by settings options.
+func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
+	callbacks := args.Callbacks
+	measurement := args.Measurement
+	sess := args.Session
+
+	endpoint, err := parseEndpoint(measurement)
+	if err != nil {
+		return err
+	}
+
+	tk := NewTestKeys()
+
+	zeroTime := time.Now()
+	idx := int64(1)
+	handshakeTracer := vpntracex.NewTracerWithTransactionID(zeroTime, idx)
+
+	openvpnConfig, err := m.mergeOpenVPNConfig(ctx, sess, endpoint, handshakeTracer)
+	if err != nil {
+		return err
+	}
+	sess.Logger().Infof("Probing endpoint %s", endpoint.String())
+
+	connResult, err := m.connectAndHandshake(ctx, zeroTime, idx, sess.Logger(), endpoint, openvpnConfig, handshakeTracer)
+	if err != nil {
+		sess.Logger().Warn("Fatal error while attempting to connect to endpoint, aborting!")
+		return err
+	}
+	if connResult != nil {
+		tk.AddConnectionTestKeys(connResult)
+	}
+	tk.Success = tk.AllConnectionsSuccessful()
+
+	callbacks.OnProgress(1.0, "All endpoints probed")
+	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
+	// are writing new experiments!
+	return nil
+}

internal/experiment/openvpn/openvpn_test.go

@@ -254,12 +254,6 @@ func TestGetCredentialsFromOptionsOrAPI(t *testing.T) {
 			t.Fatalf("expected nil options, got %v", opts)
 		}
 	})
-	/*
-		sess.MockFetchOpenVPNConfig = func(context.Context, string, string) (*model.OOAPIVPNProviderConfig, error) {
-			return nil, someError
-		}
-	*/
-
 }
 
 func TestAddConnectionTestKeys(t *testing.T) {
@@ -369,25 +363,6 @@ func TestVPNInput(t *testing.T) {
 	// TODO -- do a real test, get credentials etc.
 }
 
-func TestSuccess(t *testing.T) {
-	m := openvpn.NewExperimentMeasurer(openvpn.Config{}, "openvpn")
-	ctx := context.Background()
-	sess := makeMockSession()
-	callbacks := model.NewPrinterCallbacks(sess.Logger())
-	measurement := new(model.Measurement)
-	args := &model.ExperimentArgs{
-		Callbacks:   callbacks,
-		Measurement: measurement,
-		Session:     sess,
-	}
-	fmt.Println(args, m, ctx)
-	// TODO: mock runner
-	// err := m.Run(ctx, args)
-	//if err != nil {
-	//	t.Fatal(err)
-	//}
-}
-
 func TestMeasurer_FetchProviderCredentials(t *testing.T) {
 	t.Run("Measurer.FetchProviderCredentials calls method in session", func(t *testing.T) {
 		m := openvpn.NewExperimentMeasurer(
@@ -422,3 +397,31 @@ func TestMeasurer_FetchProviderCredentials(t *testing.T) {
 	})
 
 }
+
+func TestSuccess(t *testing.T) {
+	m := openvpn.NewExperimentMeasurer(openvpn.Config{
+		Provider: "riseup",
+		SafeCA:   "base64:Zm9v",
+		SafeKey:  "base64:Zm9v",
+		SafeCert: "base64:Zm9v",
+	}, "openvpn")
+	ctx := context.Background()
+	sess := makeMockSession()
+	callbacks := model.NewPrinterCallbacks(sess.Logger())
+	measurement := new(model.Measurement)
+	measurement.Input = "openvpn://riseup.corp/?address=127.0.0.1:9989&transport=tcp"
+	args := &model.ExperimentArgs{
+		Callbacks:   callbacks,
+		Measurement: measurement,
+		Session:     sess,
+	}
+	if sess.Logger() == nil {
+		t.Fatal("logger should not be nil")
+	}
+	fmt.Println(ctx, args, m)
+
+	err := m.Run(ctx, args)
+	if err != nil {
+		t.Fatal(err)
+	}
+}