Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: coder connect integration #482
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
base: main
Are you sure you want to change the base?
feat: coder connect integration #482
Changes from 13 commits
95b5b4e
a65e550
2ecf1df
fb9a263
3a77138
2a3500e
9252fff
195151a
e7cad82
ea4b179
5e4e795
c3287eb
a2df5cc
feb1021
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add.
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>
).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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 underHost 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is quite nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like Toolbox & regular Jetbrains don't use wildcard SSH configs? I think that makes implementing this on those a lot easier:
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 usingcoder 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>
ifcoder connect exists
returned 0.I assume it would look something like:
In both
exists
andconvert
, we'd need to auth withcoderd
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 nowcoder 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>
, butcoder 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.
But yeah, it does show both unfortunately. WDYT about the alternatives?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Toolbox does use wildcard hostnames by default, with an option to opt out. see the sample config from my system
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 whatevercoder connect convert
is doing, right? It could even do theexists
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 addcoder connect exists
forconfig-ssh
.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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?? 🤔