feat: coder connect integration #482
Conversation
src/commands.ts
Outdated
| @@ -491,12 +507,30 @@ export class Commands { | |||
| } else { | |||
| workspaceOwner = args[0] as string | |||
| workspaceName = args[1] as string | |||
| // workspaceAgent is reserved for args[2], but multiple agents aren't supported yet. | |||
| workspaceAgent = args[2] as string | |||
There was a problem hiding this comment.
The URI handler does indeed populate the agent name in args[2], but it may be undefined.
vscode.commands.executeCommand("coder.open", owner, workspace, agent, folder, openRecent)
The original comment is years out of date.
| @@ -353,7 +353,7 @@ export class WorkspaceTreeItem extends OpenableTreeItem { | |||
| showOwner ? vscode.TreeItemCollapsibleState.Collapsed : vscode.TreeItemCollapsibleState.Expanded, | |||
| workspace.owner_name, | |||
| workspace.name, | |||
| undefined, | |||
| agents[0]?.name, | |||
There was a problem hiding this comment.
This saves us needing to fetch the agent in openFromSidebar for a single-agent workspace.
openFromSidebar isn't callable on workspaces with multiple agents, so the undefined there is fine.
if the workspace is stopped, this will be undefined, prompting openWorkspace to always use the CLI instead of Connect.
This may be possible after we have coder/coder-desktop-macos#64
Does this mean we only use the CLI to start the workspace and later again connect via Coder Connect? |
Unfortunately, getting connection stats from the network extension to the desktop app is just one part. We also need to expose them programmatically in order for the VSCode extension to read them.
As of my last commit, yep! You can start a workspace, watch it build in VSCode, and then once the build is finished VSCode will switch over to using the Connect tunnel. |
|
Connecting to a dev container inside a Coder workspace also makes use of SSH. Should that also be updated? |
|
What will happen if the Coder Connect tunnel breaks due to an upgrade, or is turned off, or whatever? DO we gracefully fall back to CLI? |
src/commands.ts
Outdated
| let sshConfig | ||
| try { | ||
| // Fetch (or get defaults) for the SSH config. | ||
| sshConfig = await fetchSSHConfig(this.restClient, this.vscodeProposed) |
There was a problem hiding this comment.
The extension has an SSH config section which I assume is not going apply to Coder Connect, is that right? As well as anything the user has added to their SSH config manually.
Wondering if we should update the setting description to say the variables will not apply if there is a Coder Connect session we can use.
Speaking of settings, should using Coder Connect be behind a setting as well? Or a prompt? "We see Coder Connect, do you want to use that?" or similar.
There was a problem hiding this comment.
Wondering if we should update the setting description to say the variables will not apply if there is a Coder Connect session we can use.
Will add.
As well as anything the user has added to their SSH config manually.
These will apply, as long as they're on the Coder Connect hostname (so, if they just specify those options when running coder config-ssh it'll get added to *.<hostnamesuffix>).
Speaking of settings, should using Coder Connect be behind a setting as well? Or a prompt? "We see Coder Connect, do you want to use that?" or similar.
I don't think this is necessary, I can't imagine a scenario where the user would have Coder Connect on and not want to use it?
There was a problem hiding this comment.
I don't think this is necessary, I can't imagine a scenario where the user would have Coder Connect on and not want to use it?
I agree.
There was a problem hiding this comment.
I think the tricky part is that the user might not realize it will use Connect. So, they have X11Forwarding yes in their VS Code config or in their SSH config under Host coder-vscode--*, they are trying out Connect, one day they update the plugin, and suddenly no more forwarding. Some days it does forward, some days does not, depending on whether Connect is up (idk if this is realistic usage/behavior though).
So there should be clear messaging what the plugin is going to do IMO, somewhere. Silently changing behavior feels bad to me. But, I am neither a VS Code nor Connect user, so take what I say with a grain of salt. 😆 Maybe the behavior change will be obvious to users.
If we could make the same SSH settings apply either way, then that would be a different story.
There was a problem hiding this comment.
If we could make the same SSH settings apply either way, then that would be a different story.
With the updated coder ssh-config, any options applied to that will also apply to Coder Connect via VSCode, at least.
Though if we change the approach to what I discussed below that solves this issue too ig
There was a problem hiding this comment.
With the updated coder ssh-config, any options applied to that will also apply to Coder Connect via VSCode, at least.
That is quite nice!
| sshConfig.hostname_suffix, | ||
| ) | ||
| if (coderConnectAddr) { | ||
| remoteAuthority = `ssh-remote+${coderConnectAddr}` |
There was a problem hiding this comment.
Probably a dumb question, but this can result in multiple host names for the same workspace/agent? One for Connect, one for the fallback. Will that be potentially confusing, for example in the recents list I imagine it could show both? I know for the fallback we reopen with Connect if we can, but it does seem unfortunate to have both.
Is there any world where we use Connect if possible as part of the proxy command or does it have to be in the extension itself? That would make it trivial to implement for other IDEs and would let us reuse the same host name (and I suppose it would make it work for ssh <vscode host> on the command line too, although idk why someone would do that except for debugging). I would dread having to do this for Toolbox, if we even can. 😨
There was a problem hiding this comment.
I would dread having to do this for Toolbox, if we even can.
It looks like Toolbox & regular Jetbrains don't use wildcard SSH configs? I think that makes implementing this on those a lot easier:
Match host coder-jetbrains-toolbox-foo--test.coder.bar exec "/path/to/coder connect exists agent1.foo.owner.coder"
HostName agent1.foo.owner.coder
Host coder-jetbrains-toolbox-foo--test.coder.bar
ProxyCommand /path/to/coder ssh owner/foo.agent1
ConnectTimeout 0
StrictHostKeyChecking no
UserKnownHostsFile /dev/null
LogLevel ERROR
SetEnv CODER_SSH_SESSION_TYPE=JetBrains
This is similar to how the new coder config-ssh works, we only use the ProxyCommand if the DNS record doesn't exist, which we check using coder connect exists.
Since we use wildcard configs for the VSCode extension, using just the SSH config is pretty complicated. We'd need coder connect exists to be able to handle an argument of the form <owner>--<workspace>.<agent>. Then, we'd need another command to translate <owner>--<workspace>.<agent> to <agent>.<workspace>.<owner>.<hostnamesuffix> if coder connect exists returned 0.
I assume it would look something like:
Host coder-vscode.dev.coder.com--* exec "/path/to/coder connect exists %h"
ProxyCommand ssh "$(coder connect convert %h)
In both exists and convert, we'd need to auth with coderd to retrieve the hostname suffix, and then if the agent is missing from the authority string, we'd also need to resolve that somehow.
I think this approach is just too slow; two CLI invocations that need to init a codersdk.Client after reading from disk. Right now coder connect exists doesn't need to do that.
Alternatively, we could change the portion after coder-vscode.dev.coder.com--, to <agent>.<workspace>.<owner>.<hostnamesuffix>, but coder ssh doesn't support that right now, and we'd still need to fetch the hostnamesuffix in the extension.
This might be worth doing? We just only set the new hostname in the remote authority if the Coder CLI would support it.
However, it might be a big lift to ensure each plugin knows what agent it's connecting to before calling SSH. Right now the Coder CLI handles that if it's not supplied in the remote authority slug.
Will that be potentially confusing, for example in the recents list I imagine it could show both?
But yeah, it does show both unfortunately. WDYT about the alternatives?
There was a problem hiding this comment.
Toolbox does use wildcard hostnames by default, with an option to opt out. see the sample config from my system
# --- START CODER JETBRAINS TOOLBOX dev.coder.com
Host coder-jetbrains-toolbox-dev.coder.com--*
ProxyCommand "/Users/matifali/Library/Application Support/coder-toolbox/dev.coder.com/coder-darwin-arm64" --global-config "/Users/matifali/Library/Application Support/coder-toolbox/dev.coder.com/config" --url https://dev.coder.com ssh --stdio --disable-autostart --usage-app=jetbrains --ssh-host-prefix coder-jetbrains-toolbox-dev.coder.com-- %h
ConnectTimeout 0
StrictHostKeyChecking no
UserKnownHostsFile /dev/null
LogLevel ERROR
SetEnv CODER_SSH_SESSION_TYPE=JetBrains
Host coder-jetbrains-toolbox-dev.coder.com-bg--*
ProxyCommand "/Users/matifali/Library/Application Support/coder-toolbox/dev.coder.com/coder-darwin-arm64" --global-config "/Users/matifali/Library/Application Support/coder-toolbox/dev.coder.com/config" --url https://dev.coder.com ssh --stdio --disable-autostart --usage-app=disable --ssh-host-prefix coder-jetbrains-toolbox-dev.coder.com-bg-- %h
ConnectTimeout 0
StrictHostKeyChecking no
UserKnownHostsFile /dev/null
LogLevel ERROR
SetEnv CODER_SSH_SESSION_TYPE=JetBrains
# --- END CODER JETBRAINS TOOLBOX dev.coder.com
There was a problem hiding this comment.
Yeah, we do use wildcards in Gateway and Toolbox now. I do not think we will be able to switch the hostname from out of under Toolbox like we can here in VS Code unfortunately (at least not the way it is currently implemented, maybe it can if we implement our own connector instead of using their SSH stuff but idk if that is possible). 😢 If we can implement purely in the SSH config and CLI, that would be ideal.
IDK the best way to do it, but my initial thought would be to make coder ssh handle it. Maybe it should be behind a --use-connect flag or something, but basically it would just do whatever coder connect convert is doing, right? It could even do the exists part as well, if we want to avoid writing new blocks.
IMO remote editing from the IDE's point of view should be more or less just ssh <hostname> and we can hide away and reuse all this logic in the CLI.
There was a problem hiding this comment.
But, I do like the example you have as well. How slow are we talking? Some extra seconds to connect may not be the worst thing, if it reduces complexity on our end and makes it possible to support Connect with Toolbox.
There was a problem hiding this comment.
Not directly related but with toolbox JB uses the system ssh and support all SSH options. In gateway they had a built-in SSH implementation that didn't support all options.
There was a problem hiding this comment.
We decided against making it part of coder ssh because of the performance cost of a bicopy on the SSH connection. That's what led us to add coder connect exists for config-ssh.
How slow are we talking?
Honestly, it might be negligible on macOS and Linux. On the other hand, Spike discovered that coder connect exists takes almost 1.5s on Windows, and that's literally just to execute the binary and do a DNS lookup, so I'd expect this approach to be significantly slower there.
There was a problem hiding this comment.
Ahhh, fair. Was the performance cost of the bicopy measured? It does sound unfortunate.
1.5s for running exists is wild lol, I guess that is Windows for ya.
IMO we have a few paths forward, ideally we figure out which one before merging since this may inform how we should do it here as well:
- Do it somehow in the SSH config to support Toolbox
- Decide not to support Toolbox
- Figure out if there is a different way we can support Toolbox
But also do not let me just be a blocker if we need to move forward for now haha. I do think the doubled-up host names and the SSH settings have the potential to cause some discomfort though.
It occurs to me that if we had our own connection code instead of delegating to Microsoft's plugin, we could transparently use a different host name under the connection without doubling up on them. We could name the recent connections anything we wanted at that point. We could even add settings at that point too, I think?? 🤔
We don't currently. We'd need to watch for disconnect events from VSCode (assuming it exposes those via it's extension API), and react accordingly. |
I think this should be next in our priority, given how important it is to provide a seamless connection. |
Closes #447.
On both macOS and Windows, with Coder Connect running, I've tested that:
vscode://URI uses Coder ConnectIncluding the connection status indicator when using Connect is out of scope of the linked issue and this PR, as Coder Desktop does not currently expose those values in any way.
I recommend reviewing the PR with whitespace diffs disabled.