Skip to content

feat: add port-forward subcommand #1350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
May 18, 2022
Merged

feat: add port-forward subcommand #1350

merged 12 commits into from
May 18, 2022

Conversation

deansheather
Copy link
Member

@deansheather deansheather commented May 9, 2022

Adds a new webrtc sub-protocol dial where the datachannel label is a URI to dial i.e. tcp://127.0.0.1:1234 or unix:///path/to/socket.sock. All protocols supported by net.Dial() on the agent's OS are supported, including tcp, udp, and unix (except Windows).

To transmit errors, the first packet on a dial datachannel is a JSON payload sent from the agent with the error from net.Dial if any. If there was an error, the datachannel will be closed immediately.

The coder port-forward <workspace> subcommand accepts one or more flags such as --tcp, --udp, or --unix. You can specify multiple flags of each type, and multiple types of flags in the same command. To aid Windows users debugging Unix sockets in their workspace (as Unix sockets are not supported on Windows), you can use --unix <port>:<path> instead which will listen locally on a TCP port.

Port 0 is unsupported at this time but could be easily added in the future. Avoided for now because it's difficult to test (as we would need to parse the output to detect what port was actually used).

Subtasks

  • Add dial WebRTC sub-protocol
  • Add dial sub-protocol tests for TCP, UDP and Unix
  • Add coder port-forward command with alias coder tunnel that uses it

Resolves #1306

@deansheather deansheather changed the title feat: add agent dial handler feat: add port-forward subcommand May 9, 2022
@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

Merging #1350 (1972629) into main (f5693df) will decrease coverage by 0.18%.
The diff coverage is 79.01%.

@@            Coverage Diff             @@
##             main    #1350      +/-   ##
==========================================
- Coverage   66.58%   66.39%   -0.19%     
==========================================
  Files         284      278       -6     
  Lines       18593    18450     -143     
  Branches      235      235              
==========================================
- Hits        12380    12250     -130     
+ Misses       4950     4934      -16     
- Partials     1263     1266       +3     
Flag Coverage Δ
unittest-go-macos-latest ?
unittest-go-postgres- 65.21% <79.01%> (+0.06%) ⬆️
unittest-go-ubuntu-latest 56.09% <79.01%> (-0.19%) ⬇️
unittest-go-windows-2022 ?
unittest-js 73.67% <ø> (ø)
Impacted Files Coverage Δ
agent/conn.go 70.00% <78.57%> (+5.29%) ⬆️
agent/agent.go 66.97% <78.84%> (+1.06%) ⬆️
peer/conn.go 76.16% <100.00%> (-6.15%) ⬇️
provisionersdk/serve.go 35.13% <0.00%> (-8.11%) ⬇️
cli/configssh.go 62.04% <0.00%> (-6.57%) ⬇️
peerbroker/dial.go 77.04% <0.00%> (-6.56%) ⬇️
pty/ptytest/ptytest.go 86.95% <0.00%> (-4.35%) ⬇️
cli/templateinit.go 58.62% <0.00%> (-3.45%) ⬇️
provisionerd/provisionerd.go 74.83% <0.00%> (-1.88%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5693df...1972629. Read the comment docs.

Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really clean! Just a few minor comments. 😎😎😎

Comment on lines +242 to +245
cases := []struct {
name string
setup func(t *testing.T) net.Listener
}{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are done beautifully! 😍

Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small suggestion and logic check, looks good otherwise 👍🏻.

agent/agent.go Outdated
ctx, cancel := context.WithCancel(ctx)

copyFunc := func(dst io.WriteCloser, src io.Reader) {
defer cancel()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it lead to problems if c1 finishes early, resulting in c2.Close() being called before c2 has finished copying?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched this to a WaitGroup to avoid this case... not sure if it could've happened in practice but better to be safe than sorry

@misskniss misskniss added this to the V2 Beta milestone May 15, 2022
@f0ssel
Copy link
Contributor

f0ssel commented May 16, 2022

Closes #828

@deansheather
Copy link
Member Author

Incidentally while testing this PR I found and fixed two bugs:

  • If you open two datachannels in quick succession they may get dropped because we used an unbuffered channel and we pushed the new datachannel into the channel without blocking (i.e. in a select)
  • We add usageHeader cobra template every time the CLI starts instead of once, which could cause concurrent map writes

@deansheather deansheather marked this pull request as ready for review May 17, 2022 17:01
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Great job on this, very awesome developer UX 🥳🥳🥳🥳🥳.

Comment on lines +733 to +761
// Bicopy copies all of the data between the two connections and will close them
// after one or both of them are done writing. If the context is canceled, both
// of the connections will be closed.
func Bicopy(ctx context.Context, c1, c2 io.ReadWriteCloser) {
defer c1.Close()
defer c2.Close()

var wg sync.WaitGroup
copyFunc := func(dst io.WriteCloser, src io.Reader) {
defer wg.Done()
_, _ = io.Copy(dst, src)
}

wg.Add(2)
go copyFunc(c1, c2)
go copyFunc(c2, c1)

// Convert waitgroup to a channel so we can also wait on the context.
done := make(chan struct{})
go func() {
defer close(done)
wg.Wait()
}()

select {
case <-ctx.Done():
case <-done:
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just close the other connection if one finishes? Because the dialer already has a context, all connections will be closed when it exists.

go func() {
  defer c1.Close()
  _, _ = io.Copy(c1, c2)
}()
defer c2.Close()
_, _ = io.Copy(c2, c1)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once connected the context passed to DialContext does not affect the lifetime of the connection, so if the context is cancelled the net.Conn won't be closed.

@deansheather deansheather merged commit 9141be3 into main May 18, 2022
@deansheather deansheather deleted the webrtc-dial branch May 18, 2022 14:10
@ammario ammario mentioned this pull request May 23, 2022
2 tasks
kylecarbs pushed a commit that referenced this pull request Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a port forwarding command (similar to v1)
5 participants