refactor: fail gracefully while parsing config

the Options object is now a function that also returns an error. The
NewConfig() constructor checks for errors and returns it, if any.

- Resolves: ooni#69

cmd/minivpn/main.go

@@ -94,7 +94,10 @@ func main() {
 	defer cancel()
 
 	// create config from the passed options
-	vpncfg := config.NewConfig(opts...)
+	vpncfg, err := config.NewConfig(opts...)
+	if err != nil {
+		log.WithError(err).Fatal("cannot parse config")
+	}
 
 	// create a vpn tun Device
 	tun, err := tunnel.Start(ctx, &net.Dialer{}, vpncfg)

internal/datachannel/common_test.go

@@ -14,7 +14,8 @@ import (
 )
 
 func makeTestingSession() *session.Manager {
-	manager, err := session.NewManager(config.NewConfig())
+	cfg, _ := config.NewConfig()
+	manager, err := session.NewManager(cfg)
 	runtimex.PanicOnError(err, "could not get session manager")
 	manager.SetRemoteSessionID(model.SessionID{0x01})
 	return manager

internal/datachannel/service_test.go

@@ -27,7 +27,8 @@ func TestService_StartWorkers(t *testing.T) {
 	session := makeTestingSession()
 
 	opts := makeTestingOptions(t, "AES-128-GCM", "sha512")
-	s.StartWorkers(config.NewConfig(config.WithOpenVPNOptions(opts)), workers, session)
+	cfg, _ := config.NewConfig(config.WithOpenVPNOptions(opts))
+	s.StartWorkers(cfg, workers, session)
 
 	keyReady <- makeTestingDataChannelKey()
 	<-session.Ready

internal/networkio/service_test.go

@@ -42,10 +42,10 @@ func TestService_StartStopWorkers(t *testing.T) {
 		NetworkToMuxer: &networkToMuxer,
 	}
 
-	s.StartWorkers(config.NewConfig(config.WithLogger(log.Log)), workersManager, framingConn)
+	cfg, _ := config.NewConfig(config.WithLogger(log.Log))
+	s.StartWorkers(cfg, workersManager, framingConn)
 	got := <-networkToMuxer
 
-	//time.Sleep(time.Millisecond * 10)
 	workersManager.StartShutdown()
 	workersManager.WaitWorkersShutdown()
 

internal/reliabletransport/common_test.go

@@ -18,7 +18,8 @@ import (
 // initManagers initializes a workers manager and a session manager.
 func initManagers() (*workers.Manager, *session.Manager) {
 	w := workers.NewManager(log.Log)
-	s, err := session.NewManager(config.NewConfig(config.WithLogger(log.Log)))
+	cfg, _ := config.NewConfig(config.WithLogger(log.Log))
+	s, err := session.NewManager(cfg)
 	runtimex.PanicOnError(err, "cannot create session manager")
 	return w, s
 }

internal/reliabletransport/reliable_ack_test.go

@@ -19,9 +19,8 @@ import (
    └─┬──┘   └──▲─┘
      │         │
      │         │
-     │         │
-     ▼       send
-    ack
+     ▼         |
+    ack      send
 */
 func TestReliable_ACK(t *testing.T) {
 	if testing.Verbose() {
@@ -129,7 +128,8 @@ func TestReliable_ACK(t *testing.T) {
 			t0 := time.Now()
 
 			// let the workers pump up the jam!
-			s.StartWorkers(config.NewConfig(config.WithLogger(log.Log)), workers, session)
+			cfg, _ := config.NewConfig(config.WithLogger(log.Log))
+			s.StartWorkers(cfg, workers, session)
 
 			writer := vpntest.NewPacketWriter(dataIn)
 

internal/reliabletransport/reliable_loss_test.go

@@ -251,7 +251,8 @@ func TestReliable_WithLoss(t *testing.T) {
 			t0 := time.Now()
 
 			// let the workers pump up the jam!
-			s.StartWorkers(config.NewConfig(config.WithLogger(log.Log)), workers, session)
+			cfg, _ := config.NewConfig(config.WithLogger(log.Log))
+			s.StartWorkers(cfg, workers, session)
 
 			writer := vpntest.NewPacketWriter(dataIn)
 			go writer.WriteSequenceWithFixedPayload(tt.args.inputSequence, tt.args.inputPayload, 3)

internal/reliabletransport/reliable_reorder_test.go

@@ -124,7 +124,8 @@ func TestReliable_Reordering_UP(t *testing.T) {
 			t0 := time.Now()
 
 			// let the workers pump up the jam!
-			s.StartWorkers(config.NewConfig(config.WithLogger(log.Log)), workers, session)
+			cfg, _ := config.NewConfig(config.WithLogger(log.Log))
+			s.StartWorkers(cfg, workers, session)
 
 			writer := vpntest.NewPacketWriter(dataIn)
 			initializeSessionIDForWriter(writer, session)

internal/reliabletransport/service_test.go

@@ -43,10 +43,14 @@ func TestService_StartWorkers(t *testing.T) {
 				}(),
 			},
 			args: args{
-				config:         config.NewConfig(config.WithLogger(log.Log)),
+				config: func() *config.Config {
+					cfg, _ := config.NewConfig(config.WithLogger(log.Log))
+					return cfg
+				}(),
 				workersManager: workers.NewManager(log.Log),
 				sessionManager: func() *session.Manager {
-					m, _ := session.NewManager(config.NewConfig(config.WithLogger(log.Log)))
+					cfg, _ := config.NewConfig(config.WithLogger(log.Log))
+					m, _ := session.NewManager(cfg)
 					return m
 				}(),
 			},

internal/tlssession/common_test.go

@@ -8,7 +8,8 @@ import (
 )
 
 func makeTestingSession() *session.Manager {
-	manager, err := session.NewManager(config.NewConfig())
+	cfg, _ := config.NewConfig()
+	manager, err := session.NewManager(cfg)
 	runtimex.PanicOnError(err, "could not get session manager")
 	manager.SetRemoteSessionID(model.SessionID{0x01})
 	return manager

pkg/config/config.go

@@ -1,11 +1,11 @@
 package config
 
 import (
+	"fmt"
 	"net"
 
 	"github.com/apex/log"
 	"github.com/ooni/minivpn/internal/model"
-	"github.com/ooni/minivpn/internal/runtimex"
 )
 
 // Config contains options to initialize the OpenVPN tunnel.
@@ -20,8 +20,9 @@ type Config struct {
 	tracer model.HandshakeTracer
 }
 
-// NewConfig returns a Config ready to intialize a vpn tunnel.
-func NewConfig(options ...Option) *Config {
+// NewConfig returns a Config ready to intialize a vpn tunnel. In case any error is raised during the
+// initialization, it will be returned too.
+func NewConfig(options ...Option) (*Config, error) {
 	cfg := &Config{
 		openvpnOptions: &OpenVPNOptions{},
 		logger:         log.Log,
@@ -30,16 +31,18 @@ func NewConfig(options ...Option) *Config {
 	for _, opt := range options {
 		opt(cfg)
 	}
-	return cfg
+	return cfg, nil
 }
 
-// Option is an option you can pass to initialize minivpn.
-type Option func(config *Config)
+// Option is an option you can pass to initialize minivpn. It will return any error
+// that should be propagated by the constructor.
+type Option func(config *Config) error
 
 // WithLogger configures the passed [Logger].
 func WithLogger(logger model.Logger) Option {
-	return func(config *Config) {
+	return func(config *Config) error {
 		config.logger = logger
+		return nil
 	}
 }
 
@@ -50,8 +53,9 @@ func (c *Config) Logger() model.Logger {
 
 // WithHandshakeTracer configures the passed [HandshakeTracer].
 func WithHandshakeTracer(tracer model.HandshakeTracer) Option {
-	return func(config *Config) {
+	return func(config *Config) error {
 		config.tracer = tracer
+		return nil
 	}
 }
 
@@ -62,18 +66,24 @@ func (c *Config) Tracer() model.HandshakeTracer {
 
 // WithConfigFile configures OpenVPNOptions parsed from the given file.
 func WithConfigFile(configPath string) Option {
-	return func(config *Config) {
+	return func(config *Config) error {
 		openvpnOpts, err := ReadConfigFile(configPath)
-		runtimex.PanicOnError(err, "cannot parse config file")
-		runtimex.PanicIfFalse(openvpnOpts.HasAuthInfo(), "missing auth info")
+		if err != nil {
+			return err
+		}
+		if !openvpnOpts.HasAuthInfo() {
+			return fmt.Errorf("%w: %s", ErrBadConfig, "missing auth info")
+		}
 		config.openvpnOptions = openvpnOpts
+		return nil
 	}
 }
 
 // WithOpenVPNOptions configures the passed OpenVPN options.
 func WithOpenVPNOptions(openvpnOptions *OpenVPNOptions) Option {
-	return func(config *Config) {
+	return func(config *Config) error {
 		config.openvpnOptions = openvpnOptions
+		return nil
 	}
 }
 

pkg/config/config_test.go

@@ -11,7 +11,7 @@ import (
 
 func TestNewConfig(t *testing.T) {
 	t.Run("default constructor does not fail", func(t *testing.T) {
-		c := NewConfig()
+		c, _ := NewConfig()
 		if c.logger == nil {
 			t.Errorf("logger should not be nil")
 		}
@@ -21,22 +21,22 @@ func TestNewConfig(t *testing.T) {
 	})
 	t.Run("WithLogger sets the logger", func(t *testing.T) {
 		testLogger := model.NewTestLogger()
-		c := NewConfig(WithLogger(testLogger))
+		c, _ := NewConfig(WithLogger(testLogger))
 		if c.Logger() != testLogger {
 			t.Errorf("expected logger to be set to the configured one")
 		}
 	})
 	t.Run("WithTracer sets the tracer", func(t *testing.T) {
 		testTracer := model.HandshakeTracer(model.DummyTracer{})
-		c := NewConfig(WithHandshakeTracer(testTracer))
+		c, _ := NewConfig(WithHandshakeTracer(testTracer))
 		if c.Tracer() != testTracer {
 			t.Errorf("expected tracer to be set to the configured one")
 		}
 	})
 
 	t.Run("WithConfigFile sets OpenVPNOptions after parsing the configured file", func(t *testing.T) {
 		configFile := writeValidConfigFile(t.TempDir())
-		c := NewConfig(WithConfigFile(configFile))
+		c, _ := NewConfig(WithConfigFile(configFile))
 		opts := c.OpenVPNOptions()
 		if opts.Proto.String() != "udp" {
 			t.Error("expected proto udp")

tests/integration/main.go

@@ -17,6 +17,7 @@ import (
 	"github.com/ooni/minivpn/extras/ping"
 	"github.com/ooni/minivpn/internal/networkio"
 	"github.com/ooni/minivpn/internal/tun"
+	"github.com/ooni/minivpn/pkg/config"
 )
 
 const (
@@ -117,7 +118,7 @@ func main() {
 	defer os.RemoveAll(tmp) // clean up
 
 	fmt.Println("launching docker")
-	config, pool, resource, err := launchDocker("AES-256-GCM", "SHA256")
+	configInfo, pool, resource, err := launchDocker("AES-256-GCM", "SHA256")
 	if err != nil {
 		log.WithError(err).Fatal("cannot start docker")
 	}
@@ -131,12 +132,12 @@ func main() {
 	defer cfgFile.Close()
 	fmt.Println("Config written to: " + cfgFile.Name())
 
-	if _, err = cfgFile.Write(config); err != nil {
+	if _, err = cfgFile.Write(configInfo); err != nil {
 		log.WithError(err).Fatal("Failed to write config to temporary file")
 	}
 
 	// actual test begins
-	vpnConfig := config.NewConfig(config.WithConfigFile(cfgFile.Name()))
+	vpnConfig, err := config.NewConfig(config.WithConfigFile(cfgFile.Name()))
 
 	dialer := networkio.NewDialer(log.Log, &net.Dialer{})
 	conn, err := dialer.DialContext(context.TODO(), vpnConfig.Remote().Protocol, vpnConfig.Remote().Endpoint)