Skip to content
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

feat: enable forwarded ports using built-in proxy #5673

Merged
merged 11 commits into from
Oct 24, 2022
Merged

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Oct 19, 2022

Description

This PR enables the forwarded ports panel in Code and then adds a tunnelFactory to forward them using code-server's built-in proxy (/proxy/port).

One thing to note: it seems ports will not be "un-forwarded" due to a limitation upstream. However, reloading code-server, they are removed. See here:
image

TODOs

The following things remain:

  • test with code-server not on localhost
  • killing port should remove from ports panel this behavior exists upstream so leaving as is
  • don't show port running code-server
  • use correct link with browser icon
  • test "open in browser" notification link
  • test using environment variable like VSCODE_PROXY_URI

Screenshots

Example using VSCODE_PROXY_URI=https://{{port}}.kyle.dev
image

Example using Caddy to server code-server behind proxy:
image

@jsjoeio jsjoeio changed the title jsjoeio/tunnel feat: enable forwarded ports using built-in proxy Oct 19, 2022
@jsjoeio jsjoeio self-assigned this Oct 19, 2022
This makes the forwarded ports panel enabled by default.
This adds a `tunnelProvider` along with a `tunnelFactory` so that ports
are forwarded and use code-server's built-in proxy.
This adds a check in our `resolveExternalUri` patch to skip modifying if
the `authority` and the `location.host` match to prevent
`localhost:<port>/proxy/<port>` from being modified.
@jsjoeio jsjoeio marked this pull request as ready for review October 20, 2022 18:17
@jsjoeio jsjoeio requested a review from a team as a code owner October 20, 2022 18:17
@jsjoeio jsjoeio temporarily deployed to npm October 20, 2022 18:29 Inactive
@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Merging #5673 (b401cbd) into main (430b567) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5673   +/-   ##
=======================================
  Coverage   72.61%   72.61%           
=======================================
  Files          30       30           
  Lines        1680     1680           
  Branches      368      368           
=======================================
  Hits         1220     1220           
  Misses        397      397           
  Partials       63       63           

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 430b567...b401cbd. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Oct 20, 2022

✨ code-server dev build published to npm for PR #5673!

  • Last publish status: success
  • Commit: b401cbd

To install in a local project, run:

npm install @coder/code-server-pr@5673

To install globally, run:

npm install -g @coder/code-server-pr@5673

@jsjoeio jsjoeio temporarily deployed to npm October 20, 2022 18:50 Inactive
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Oct 20, 2022

npm install --unsafe-perm @coder/code-server-pr@4.7.1-5673-3c17798eb377432f160f3aef7cad61bb2a26a83f

patches/proxy-uri.diff Show resolved Hide resolved
patches/proxy-uri.diff Outdated Show resolved Hide resolved
patches/proxy-uri.diff Show resolved Hide resolved
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Very exciting 🎉

For some reason I am not seeing the ports panel (tried npm and the CI artifact). 🤔

$ code-server --auth none
[2022-10-21T15:48:23.934Z] info  code-server 4.7.1-5673-3c17798eb377432f160f3aef7cad61bb2a26a83f 3c17798eb377432f160f3aef7cad61bb2a26a83f

patches/proxy-uri.diff Outdated Show resolved Hide resolved
patches/proxy-uri.diff Outdated Show resolved Hide resolved
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Oct 21, 2022

For some reason I am not seeing the ports panel (tried npm and the CI artifact). 🤔

Hmm...do you have it hidden by chance? @bpmct and I did test it in a v2 workspace (Linux) and we saw it. I'm not sure what else might cause it to be missing 🤔

@jsjoeio jsjoeio requested a review from code-asher October 21, 2022 17:16
@bpmct
Copy link
Member

bpmct commented Oct 21, 2022

For some reason I am not seeing the ports panel (tried npm and the CI artifact). 🤔

Hmm...do you have it hidden by chance? @bpmct and I did test it in a v2 workspace (Linux) and we saw it. I'm not sure what else might cause it to be missing 🤔

here are the steps we did: coder/coder@main...dogfood-code-server-ports

@jsjoeio jsjoeio temporarily deployed to npm October 21, 2022 17:25 Inactive
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Apparently I was having a caching issue. Works for me now!

@code-asher
Copy link
Member

code-asher commented Oct 21, 2022

We might want to edit webClientServer.ts and add a trailing slash since most of the time that will be needed:

proxyEndpointTemplate: process.env.VSCODE_PROXY_URI ?? base + '/proxy/{{port}}/',

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Oct 21, 2022

Good point! I'll do that and then merge

@jsjoeio jsjoeio enabled auto-merge (squash) October 21, 2022 21:48
@jsjoeio jsjoeio temporarily deployed to npm October 21, 2022 23:36 Inactive
@jsjoeio jsjoeio temporarily deployed to npm October 24, 2022 17:34 Inactive
@jsjoeio jsjoeio temporarily deployed to npm October 24, 2022 17:47 Inactive
@jsjoeio jsjoeio temporarily deployed to npm October 24, 2022 18:00 Inactive
@jsjoeio jsjoeio merged commit 031e903 into main Oct 24, 2022
@jsjoeio jsjoeio deleted the jsjoeio/tunnel branch October 24, 2022 18:11
@codecae
Copy link

codecae commented Nov 29, 2022

FWIW:

I have been experiencing potential issues with this PR on systems in which the code-server is running behind another proxy. Websocket connections are constantly being disconnected and reconnected at a high rate making the UI inaccessible and rendering the application unusable.

I will open another issue for this, but wanted to share in this thread as well.

Having an optional kill switch for this feature might be beneficial for folks like myself who would prefer to opt out so we can stay current with code-server, otherwise!

==> code-server-stderr.log <==


==> code-server-stdout.log <==

[2022-11-29T06:28:43.409Z] info  code-server 4.8.0 005fa87699b4b3c791bd97693400dd016a7ee315
[2022-11-29T06:28:43.411Z] info  Using user-data-dir ~/.local/share/code-server
[2022-11-29T06:28:43.435Z] info  Using config file ~/.config/code-server/config.yaml
[2022-11-29T06:28:43.436Z] info  HTTP server listening on http://127.0.0.1:43773/ 
[2022-11-29T06:28:43.436Z] info    - Authentication is disabled 
[2022-11-29T06:28:43.436Z] info    - Not serving HTTPS 
[06:28:44] Extension host agent started.
[06:28:46] [127.0.0.1][a1361db6][ManagementConnection] New connection established.
[06:28:47] [127.0.0.1][b3b52f2c][ExtensionHostConnection] New connection established.
[06:28:47] [127.0.0.1][b3b52f2c][ExtensionHostConnection] <78> Launched Extension Host Process.
[06:28:57] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.

==> code-server-stderr.log <==
[06:28:58] Error: read ECONNRESET
    at TCP.onStreamRead (node:internal/stream_base_commons:217:20) {
  errno: -104,
  code: 'ECONNRESET',
  syscall: 'read'
}

==> code-server-stdout.log <==
[06:28:58] [127.0.0.1][a1361db6][ManagementConnection] The client has disconnected, will wait for reconnection 3h before disposing...
[06:28:58] [127.0.0.1][a1361db6][ManagementConnection] Another client has connected, will shorten the wait for reconnection 5m before disposing...
[06:28:58] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.

==> code-server-stderr.log <==
[06:28:58] Error: read ECONNRESET
    at TCP.onStreamRead (node:internal/stream_base_commons:217:20) {
  errno: -104,
  code: 'ECONNRESET',
  syscall: 'read'
}

==> code-server-stdout.log <==
[06:28:58] [127.0.0.1][a1361db6][ManagementConnection] The client has disconnected, will wait for reconnection 3h before disposing...
[06:28:59] [127.0.0.1][a1361db6][ManagementConnection] Another client has connected, will shorten the wait for reconnection 5m before disposing...
[06:28:59] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:28:59] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:00] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.

==> code-server-stderr.log <==
[06:29:01] Error: read ECONNRESET
    at TCP.onStreamRead (node:internal/stream_base_commons:217:20) {
  errno: -104,
  code: 'ECONNRESET',
  syscall: 'read'
}

==> code-server-stdout.log <==
[06:29:01] [127.0.0.1][a1361db6][ManagementConnection] The client has disconnected, will wait for reconnection 3h before disposing...
[06:29:01] [127.0.0.1][a1361db6][ManagementConnection] Another client has connected, will shorten the wait for reconnection 5m before disposing...
[06:29:01] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:02] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:03] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:04] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:05] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:06] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:07] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:08] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:09] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:10] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:11] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:12] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.

==> code-server-stderr.log <==
[06:29:13] Error: read ECONNRESET
    at TCP.onStreamRead (node:internal/stream_base_commons:217:20) {
  errno: -104,
  code: 'ECONNRESET',
  syscall: 'read'
}

==> code-server-stdout.log <==
[06:29:13] [127.0.0.1][a1361db6][ManagementConnection] The client has disconnected, will wait for reconnection 3h before disposing...
[06:29:14] [127.0.0.1][a1361db6][ManagementConnection] Another client has connected, will shorten the wait for reconnection 5m before disposing...
[06:29:14] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:15] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:16] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:17] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:18] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:19] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.
[06:29:20] [127.0.0.1][a1361db6][ManagementConnection] The client has reconnected.```

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Nov 30, 2022

Having an optional kill switch for this feature might be beneficial for folks like myself who would prefer to opt out so we can stay current with code-server, otherwise!

I didn't consider that when I merged this - great point! I'll keep an eye out for that issue.

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.

4 participants