refactor(intra): use outline-sdk as the network stack #125
Conversation
go.mod
Outdated
| @@ -8,16 +8,16 @@ require ( | |||
| github.com/Jigsaw-Code/outline-sdk v0.0.2 | |||
There was a problem hiding this comment.
We should update this to v0.0.6 to make sure it's still compatible.
intra/tunnel.go
Outdated
| core.RegisterOutputFn(tunWriter.Write) | ||
| t := &intratunnel{ | ||
| Tunnel: tunnel.NewTunnel(tunWriter, core.NewLWIPStack()), | ||
| func NewTunnel(fakedns string, dohdns doh.Transport, tun io.Closer, protector protect.Protector, listener Listener) (t *Tunnel, err error) { |
There was a problem hiding this comment.
The IPDevice should be independent of the TUN device.
Remove the tun input.
Also, remove the Disconnect() method. You are already exposing Close().
In the relay code, you can close the tun device once the IPDevice closes.
There was a problem hiding this comment.
Yes, I was trying to keep all interface unchanged. Disconnect() is exposed to Java as well.
There was a problem hiding this comment.
It's quite confusing to have both, and it complicates our code. To figure out why we need to pass the tun device is mind-boggling.
It's not a requirement to preserve the old interface, especially for Intra. That requirement only gets in our way.
We need to prioritize not having to worry about the Go backend interface, as that's a major productivity hurdle. That was the point of moving the code to the client repo too (which I understand will happen separately). Perhaps it makes sense to move the code first before future changes. For the Outline integration, let's move the code first (without changes, but proper build), before changing it. I believe that will work better, as it allows use to change the backend and frontend at once.
We can leave the clean up to another PR, but if we want Intra to be a model to copy, we should simplify and clean up the legacy interfaces, so people don't spread bad practices.
There was a problem hiding this comment.
As we discussed offline, in this PR, let's focus on "refactoring" without changing the old interface. Then I'll copy the code to the Intra repository without any code modification (but with Gradle build script). After that, we can further redesign the interface (e.g. use IntraDevice, and only export one single package instead of five).
| return t.sni.Configure(f, suffix, strings.ToLower(country)) | ||
| } | ||
|
|
||
| func (t *Tunnel) Disconnect() { |
There was a problem hiding this comment.
See the refactor planning comment above.
| } | ||
|
|
||
| func isErrClosed(err error) bool { | ||
| return errors.Is(err, os.ErrClosed) || errors.Is(err, fs.ErrClosed) || errors.Is(err, network.ErrClosed) |
There was a problem hiding this comment.
Do we return network.ErrClosed on Reads? I thought we would just return ErrClosed on writes, and return EOF on reads.
There was a problem hiding this comment.
It's not us, it's the tun device (os.File) that doesn't conform with go's rules, it will return os.ErrClosed. But I expanded to more similar ErrClosed errors here.
Co-authored-by: Vinicius Fortuna <[email protected]>
|
@fortuna , this PR is ready for review. Thanks. |
This PR replaces the
go-tun2socks/corewithoutline-sdk/*in the Intra project, and removes the dependency ontunnel.Tunnel, making theintrapackage a standalone Go package that can be copied to other locations, such as the Intra repository.I kept the signatures of all exported interfaces the same. We can refactor them after we move the code to the Intra repository.
Related PR: Jigsaw-Code/outline-sdk#66