feat: ✨ add new OutlineDevice API that uses Outline SDK #118

jyyi1 · 2023-04-14T20:48:45Z

In this PR, I added a new OutlineDevice API that can be used by Outline Client to replace the old shadowsocks.CheckConnectivity, shadowsocks.newClient and OutlineTunnel. I integrated Outline SDK into the implementation as well.

I also refactored a bunch of things in this PR:

Export outline package in gomobile
Mark all outline/** sub packages as deprecated

Related PR: #122 , #124

outline/shadowsocks/client.go

…ecated

outline/shadowsocks/legacy_api.go

outline/internal/shadowsocks/shadowsocks.go

outline/client.go

outline/connectivity.go

outline/client.go

fortuna

Are you in the office tomorrow? perhaps we can brainstorm a bit on the api

outline/client.go

outline/electron/main.go

outline/client.go

outline/internal/shadowsocks/shadowsocks.go

outline/tun2socks/tunnel_android.go

outline/tun2socks/tunnel_darwin.go

outline/client.go

jyyi1 · 2023-04-24T22:49:04Z

@fortuna I will be in office on Wednesday, let me schedule a meeting.

jyyi1 · 2023-05-02T20:33:12Z

As discussed offline, @fortuna please take a look at the newly proposed tun2socks.ConnectTunnel, while still keeping all old function signature unchanged.

fortuna · 2023-05-03T19:26:25Z

outline/electron/main.go

+
+func startLegacyShadowsocksClient() {
+	// legacy raw flags
+	config := shadowsocks.Config{


Can you define a legacyConfigFromFlags(), and pass the config as a parameter?

Making the config explicit in the function call will make the call more understandable.

Let's use "start tunnel" to be analogous to the new code.
I'm imagining startLegacyShadowsocksTunnel(legacyConfigFromClient)

fortuna · 2023-05-03T19:38:54Z

outline/internal/shadowsocks/shadowsocks.go

+		return nil, nil, fmt.Errorf("failed to resolve proxy address: %w", err)
+	}
+
+	proxyTCPEndpoint := transport.TCPEndpoint{RemoteAddr: net.TCPAddr{IP: proxyIP.IP, Port: port}}


I suggest you pull the latest version of the SDK. This now takes a string. Example: https://github.com/Jigsaw-Code/outline-internal-sdk/blob/e79d369307d001859b3a4e2ecb0ecbf140a4f11f/examples/outline-connectivity/main.go#L132

FYI: #120

Let me know of you want to merge that or not. It shouldn't conflict much.

Sure, please merge that. BTW, I will work on the SDK change (to introduce IPDevice) before updating this PR as well.

fortuna · 2023-05-03T19:45:41Z

outline/electron/main.go

+			config.Prefix = p
+		}
+	}
+	client, err := shadowsocks.NewClient(&config)
 	if err != nil {
 		log.Errorf("Failed to create Shadowsocks client: %v", err)
 		os.Exit(neterrors.IllegalConfiguration.Number())
 	}

 	if *args.checkConnectivity {


Move this back out. The connectivity test and the tunnel flows are separate. We need to clearly run one or the other like we had before.

Also, don't we need a check connectivity with json config, for the udp update?

We don't need any connectivity test for the new json config tunnel. I'd rather expose a public OutlineTunnel.IsUDPSupported() method so the caller can retrieve that info.

fortuna · 2023-05-03T19:58:40Z

outline/tun2socks/tunnel_electron.go

+	if tunDev == nil {
+		return nil, errors.New("tun device is required")
+	}
+	tn, err := newTunnelFromJSON(configJSON, tunDev)


We need a clearer name. Perhaps this?

Suggested change

tn, err := newTunnelFromJSON(configJSON, tunDev)

proxyTun, err := newTunnelFromJSON(configJSON, tunDev)

I agree. Tunnel is a bad name, it's similar to tun device, which is very confusing.

fortuna · 2023-05-03T20:30:04Z

outline/tun2socks/tunnel.go

@@ -40,7 +44,7 @@ type Tunnel interface {
 }

 // Deprecated: use Tunnel directly.
-type OutlineTunnel = Tunnel
+type OutlineTunnel Tunnel

 type outlinetunnel struct {
 	tunnel.Tunnel


Let's remove this. I don't really think it's needed, and it's extremely confusing. Let's see what breaks and fix it. I believe the comments here will address most issues.

Suggested change

tunnel.Tunnel

fortuna · 2023-05-03T20:30:49Z

outline/tun2socks/tunnel_android.go

+//
+// This function does *not* take ownership of the TUN file descriptor `fd`; the
+// caller is responsible for closing after Tunnel disconnects.
+func ConnectTunnel(configJSON string, fd int) (Tunnel, error) {


The ConnectTunnel function is doing different things in different platforms. That is confusing. We need to fix that.

Can you make this take a ReadWriteCloser for the tun device and move this to the shared code?

fortuna · 2023-05-03T20:33:14Z

outline/tun2socks/tunnel_android.go

+	if err != nil {
+		return nil, err
+	}
+	tn, err := newTunnelFromJSON(configJSON, tunDev)


Remove tunDev, and give a better name for the proxy tunnel:

Suggested change

tn, err := newTunnelFromJSON(configJSON, tunDev)

proxyTun, err := newTunnelFromJSON(configJSON)

The initialization that needs tunDev should happen below.

fortuna · 2023-05-03T20:33:40Z

outline/tun2socks/tunnel_android.go

+		return nil, fmt.Errorf("unable to create tunnel from config: %w", err)
+	}
+
+	go tunnel.ProcessInputPackets(tn, tunDev)


Use CopyBuffer. See comment on Electron code, which should be shared.

fortuna · 2023-05-03T20:37:13Z

outline/tun2socks/tunnel_electron.go

+		return nil, fmt.Errorf("unable to create tunnel from config: %w", err)
+	}
+
+	go io.CopyBuffer(tn, tunDev, make([]byte, mtu))


We need to see the copy both ways in the same place. This code only shows one way, which is confusing. Where's the flow from the tun device to the proxy device? It takes a lot of digging to figure that out.

I think we could implement the bridge like this:

Suggested change

go io.CopyBuffer(tn, tunDev, make([]byte, mtu))

go func() {

defer proxyTun.Close()

io.CopyBuffer(proxyDevice, tunDev, make([]byte, mtu))

}

go proxyTun.WriteTo(tunDev)

The reason for WriteTo is to avoid a layer of buffers when possible.

proxyTun.WriteTo will effectively call core.RegisterOutputFn to write to the tun device, Then wait for proxyTun.Close(). ProxyTun.Close() should call lwipStack.Close(). This way you don't need tunnel.Tunnel anymore.

@sbruens

Outline Client is referencing the following APIs (Android JNI interface as an example) exported by `gomobile`. <img width="642" alt="image" src="https://user-images.githubusercontent.com/93548144/232151618-de7c91eb-2ee6-444e-bbe0-023262af2149.png"> But due to the recent refactoring, we introduced types that are not recognized by `gomobile`. This is the only function exported in `master` branch now: <img width="646" alt="image" src="https://user-images.githubusercontent.com/93548144/232152396-6cf278ab-f45b-448a-adb9-c03b13772eb7.png"> In this PR, I re-exported these functions (together with the new `NewClientFromJSON`) by using the built-in types. Here is the exported Android JNI interface: <img width="635" alt="image" src="https://user-images.githubusercontent.com/93548144/232151977-ad6bd330-4bc5-4383-b6ef-49dce4dc1853.png"> /cc @sbruens , although the required APIs are re-exported, some of the data types still need to be updated: * [`OutlineTunnel`](https://github.com/Jigsaw-Code/outline-client/blob/master/src/cordova/android/OutlineAndroidLib/outline/src/main/java/org/outline/vpn/VpnTunnel.java#L49) -> `Tunnel` * [`Tun2socksOutlineTunnel`](https://github.com/Jigsaw-Code/outline-client/blob/master/src/cordova/apple/OutlineAppleLib/Sources/PacketTunnelProviderSources/PacketTunnelProvider.m#L39) -> `Tun2socksTunnel` Related PR: #118

fix: 💊 make sure gomobile can export CheckConnectivity and NewClient

63aae04

jyyi1 requested a review from a team as a code owner April 14, 2023 20:48

jyyi1 requested a review from fortuna April 14, 2023 20:48

jyyi1 added 2 commits April 14, 2023 17:34

align connectShadowsocksTunnel with the old signature

be55019

revert the previous change, seems not working

82b552e

fortuna reviewed Apr 14, 2023

View reviewed changes

outline/shadowsocks/client.go Outdated Show resolved Hide resolved

refactor: export outline package to gomobile and mark subpackage depr…

804dc85

…ecated

jyyi1 requested a review from fortuna April 19, 2023 22:32

fortuna suggested changes Apr 20, 2023

View reviewed changes

jyyi1 added 2 commits April 21, 2023 18:38

define outline.Client as an interface instead of a struct

cc44584

rename NewShadowsocksClientFromJSON to NewClientFromJSON

dc5e4a6

jyyi1 requested a review from fortuna April 21, 2023 22:50

fortuna reviewed Apr 24, 2023

View reviewed changes

jyyi1 added 3 commits April 27, 2023 18:58

revert the interface change of outline.Client

3a9ff6a

add newTunnelFromJSON helper

393260e

export tun2socks.ConnectTunnel as the new API

0df72e5

jyyi1 requested a review from fortuna May 2, 2023 20:31

fortuna suggested changes May 3, 2023

View reviewed changes

jyyi1 changed the title ~~fix: 💊 make sure gomobile can export CheckConnectivity and NewClient~~ feat: ✨ add new OutlineDevice API that uses Outline SDK Jul 24, 2023

jyyi1 mentioned this pull request Jul 24, 2023

fix: 💊 re-export APIs used by Outline Client #122

Merged

jyyi1 added 2 commits August 14, 2023 17:06

Merge branch 'master' into junyi/fix-gomobile-exports

3c9a2c8

add internal/sdk package

c693adb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ✨ add new OutlineDevice API that uses Outline SDK #118

feat: ✨ add new OutlineDevice API that uses Outline SDK #118

jyyi1 commented Apr 14, 2023 •

edited

Loading

fortuna left a comment

jyyi1 commented Apr 24, 2023 •

edited

Loading

jyyi1 commented May 2, 2023

fortuna May 3, 2023

fortuna May 3, 2023

fortuna May 4, 2023

jyyi1 May 4, 2023

fortuna May 3, 2023

jyyi1 May 3, 2023

fortuna May 3, 2023

jyyi1 May 3, 2023

fortuna May 3, 2023

fortuna May 3, 2023

fortuna May 3, 2023

fortuna May 3, 2023

fortuna May 3, 2023

	tn, err := newTunnelFromJSON(configJSON, tunDev)
	proxyTun, err := newTunnelFromJSON(configJSON, tunDev)

-	go io.CopyBuffer(tn, tunDev, make([]byte, mtu))
+	go func() {
+	  defer proxyTun.Close()
+	  io.CopyBuffer(proxyDevice, tunDev, make([]byte, mtu))
+	}
+	go proxyTun.WriteTo(tunDev)

feat: ✨ add new OutlineDevice API that uses Outline SDK #118

Are you sure you want to change the base?

feat: ✨ add new OutlineDevice API that uses Outline SDK #118

Conversation

jyyi1 commented Apr 14, 2023 • edited Loading

fortuna left a comment

Choose a reason for hiding this comment

jyyi1 commented Apr 24, 2023 • edited Loading

jyyi1 commented May 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyyi1 commented Apr 14, 2023 •

edited

Loading

jyyi1 commented Apr 24, 2023 •

edited

Loading