resolve code review comments 3

client/go/outline/connectivity.go

@@ -15,18 +15,10 @@
 package outline
 
 import (
-	"net"
-
 	"github.com/Jigsaw-Code/outline-apps/client/go/outline/connectivity"
 	"github.com/Jigsaw-Code/outline-apps/client/go/outline/platerrors"
 )
 
-const (
-	tcpTestWebsite = "http://example.com"
-	dnsServerIP    = "1.1.1.1"
-	dnsServerPort  = 53
-)
-
 // TCPAndUDPConnectivityResult represents the result of TCP and UDP connectivity checks.
 //
 // We use a struct instead of a tuple to preserve a strongly typed error that gobind recognizes.
@@ -40,16 +32,7 @@ type TCPAndUDPConnectivityResult struct {
 // containing a TCP error and a UDP error.
 // If the connectivity check was successful, the corresponding error field will be nil.
 func CheckTCPAndUDPConnectivity(client *Client) *TCPAndUDPConnectivityResult {
-	// Start asynchronous UDP support check.
-	udpErrChan := make(chan error)
-	go func() {
-		resolverAddr := &net.UDPAddr{IP: net.ParseIP(dnsServerIP), Port: dnsServerPort}
-		udpErrChan <- connectivity.CheckUDPConnectivityWithDNS(client, resolverAddr)
-	}()
-
-	tcpErr := connectivity.CheckTCPConnectivityWithHTTP(client, tcpTestWebsite)
-	udpErr := <-udpErrChan
-
+	tcpErr, udpErr := connectivity.CheckTCPAndUDPConnectivity(client, client)
 	return &TCPAndUDPConnectivityResult{
 		TCPError: platerrors.ToPlatformError(tcpErr),
 		UDPError: platerrors.ToPlatformError(udpErr),

client/go/outline/connectivity/connectivity.go

@@ -32,6 +32,31 @@ const (
 	bufferLength        = 512
 )
 
+const (
+	testTCPWebsite    = "http://example.com"
+	testDNSServerIP   = "1.1.1.1"
+	testDNSServerPort = 53
+)
+
+// CheckTCPAndUDPConnectivity checks whether the given `tcp` and `udp` clients can relay traffic.
+//
+// It parallelizes the execution of TCP and UDP checks, and returns a TCP error and a UDP error.
+// A nil error indicates successful connectivity for the corresponding protocol.
+func CheckTCPAndUDPConnectivity(
+	tcp transport.StreamDialer, udp transport.PacketListener,
+) (tcpErr error, udpErr error) {
+	// Start asynchronous UDP support check.
+	udpErrChan := make(chan error)
+	go func() {
+		resolverAddr := &net.UDPAddr{IP: net.ParseIP(testDNSServerIP), Port: testDNSServerPort}
+		udpErrChan <- CheckUDPConnectivityWithDNS(udp, resolverAddr)
+	}()
+
+	tcpErr = CheckTCPConnectivityWithHTTP(tcp, testTCPWebsite)
+	udpErr = <-udpErrChan
+	return
+}
+
 // CheckUDPConnectivityWithDNS determines whether the Outline proxy represented by `client` and
 // the network support UDP traffic by issuing a DNS query though a resolver at `resolverAddr`.
 // Returns nil on success or an error on failure.

client/go/outline/dialer_linux.go

@@ -21,25 +21,25 @@ import (
 
 // newFWMarkProtectedTCPDialer creates a base TCP dialer for [Client]
 // protected by the specified firewall mark.
-func newFWMarkProtectedTCPDialer(fwmark uint32) (net.Dialer, error) {
+func newFWMarkProtectedTCPDialer(fwmark uint32) net.Dialer {
 	return net.Dialer{
 		KeepAlive: -1,
 		Control: func(network, address string, c syscall.RawConn) error {
 			return c.Control(func(fd uintptr) {
 				syscall.SetsockoptInt(int(fd), syscall.SOL_SOCKET, syscall.SO_MARK, int(fwmark))
 			})
 		},
-	}, nil
+	}
 }
 
 // newFWMarkProtectedUDPDialer creates a new UDP dialer for [Client]
 // protected by the specified firewall mark.
-func newFWMarkProtectedUDPDialer(fwmark uint32) (net.Dialer, error) {
+func newFWMarkProtectedUDPDialer(fwmark uint32) net.Dialer {
 	return net.Dialer{
 		Control: func(network, address string, c syscall.RawConn) error {
 			return c.Control(func(fd uintptr) {
 				syscall.SetsockoptInt(int(fd), syscall.SOL_SOCKET, syscall.SO_MARK, int(fwmark))
 			})
 		},
-	}, nil
+	}
 }

client/go/outline/dialer_others.go

@@ -16,17 +16,12 @@
 
 package outline
 
-import (
-	"errors"
-	"net"
-)
+import "net"
 
-// newFWMarkProtectedTCPDialer is not supported on non-Linux platforms.
-func newFWMarkProtectedTCPDialer(fwmark uint32) (net.Dialer, error) {
-	return net.Dialer{}, errors.ErrUnsupported
+func newFWMarkProtectedTCPDialer(fwmark uint32) net.Dialer {
+	panic("SO_MARK socket option is only supported on Linux")
 }
 
-// newFWMarkProtectedUDPDialer is not supported on non-Linux platforms.
-func newFWMarkProtectedUDPDialer(fwmark uint32) (net.Dialer, error) {
-	return net.Dialer{}, errors.ErrUnsupported
+func newFWMarkProtectedUDPDialer(fwmark uint32) net.Dialer {
+	panic("SO_MARK socket option is only supported on Linux")
 }

client/go/outline/method_channel.go

@@ -61,9 +61,8 @@ func InvokeMethod(method string, input string) *InvokeMethodResult {
 		}
 
 	case MethodEstablishVPN:
-		conn, err := establishVPN(input)
+		err := establishVPN(input)
 		return &InvokeMethodResult{
-			Value: conn,
 			Error: platerrors.ToPlatformError(err),
 		}
 

client/go/outline/vpn.go

@@ -15,6 +15,7 @@
 package outline
 
 import (
+	"context"
 	"encoding/json"
 
 	perrs "github.com/Jigsaw-Code/outline-apps/client/go/outline/platerrors"
@@ -30,51 +31,26 @@ type vpnConfigJSON struct {
 // The configuration string should be a JSON object containing the VPN configuration
 // and the transport configuration.
 //
-// The function returns a JSON string representing the established VPN connection,
-// or an error if the connection fails.
-func establishVPN(configStr string) (string, error) {
+// The function returns a non-nil error if the connection fails.
+func establishVPN(configStr string) error {
 	var conf vpnConfigJSON
 	if err := json.Unmarshal([]byte(configStr), &conf); err != nil {
-		return "", perrs.PlatformError{
+		return perrs.PlatformError{
 			Code:    perrs.IllegalConfig,
 			Message: "invalid VPN config format",
 			Cause:   perrs.ToPlatformError(err),
 		}
 	}
 
-	// Create Outline Client and Device
-	tcp, err := newFWMarkProtectedTCPDialer(conf.VPNConfig.ProtectionMark)
-	if err != nil {
-		return "", err
-	}
-	udp, err := newFWMarkProtectedUDPDialer(conf.VPNConfig.ProtectionMark)
-	if err != nil {
-		return "", err
-	}
+	tcp := newFWMarkProtectedTCPDialer(conf.VPNConfig.ProtectionMark)
+	udp := newFWMarkProtectedUDPDialer(conf.VPNConfig.ProtectionMark)
 	c, err := newClientWithBaseDialers(conf.TransportConfig, tcp, udp)
 	if err != nil {
-		return "", err
-	}
-	proxy, err := NewDevice(c)
-	if err != nil {
-		return "", err
+		return err
 	}
 
-	// Establish system VPN to the proxy
-	conn, err := vpn.EstablishVPN(&conf.VPNConfig, proxy)
-	if err != nil {
-		return "", err
-	}
-
-	connJson, err := json.Marshal(conn)
-	if err != nil {
-		return "", perrs.PlatformError{
-			Code:    perrs.InternalError,
-			Message: "failed to return VPN connection as JSON",
-			Cause:   perrs.ToPlatformError(err),
-		}
-	}
-	return string(connJson), nil
+	_, err = vpn.EstablishVPN(context.Background(), &conf.VPNConfig, c, c)
+	return err
 }
 
 // closeVPN closes the currently active VPN connection.