Skip to content

feat: modify config-ssh to check for Coder Connect #17419

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 1 commit into from
Apr 17, 2025

Conversation

spikecurtis
Copy link
Contributor

@spikecurtis spikecurtis commented Apr 16, 2025

relates to #16828

Changes SSH config so that suffixes only match if Coder Connect is not running / available. This means that we will use the existing Coder Connect tunnel if it is available, rather than creating a new tunnel via coder ssh --stdio.

Copy link
Contributor Author

spikecurtis commented Apr 16, 2025

@spikecurtis spikecurtis marked this pull request as ready for review April 16, 2025 13:27
@spikecurtis spikecurtis force-pushed the spike/config-ssh-match-suffix branch from efdb86b to 3073db4 Compare April 16, 2025 13:30
@spikecurtis spikecurtis force-pushed the spike/connect-exists-cmd branch from ecef684 to 169ab02 Compare April 16, 2025 13:30
@spikecurtis spikecurtis changed the title feat: modifies config-ssh to check for Coder Connect feat: modify config-ssh to check for Coder Connect Apr 16, 2025
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.

Awesome! Tried it out and seems to work well. This also (perhaps inadvertently) fixes most issues when trying to connect to stopped workspaces as it will automatically proxy the first time and trigger autostart. 😄

@mafredri
Copy link
Member

Edit: Just realized that setting -o options are only effective for proxied commands. It'd be nice to have two entries in the SSH config, i.e:

Host *.coder
        Foo=Bar
Match host *.coder !exec "/Users/maf/Code/coder/coder connect exists %h"
        ConnectTimeout=0
        StrictHostKeyChecking=no
        UserKnownHostsFile=/dev/null
        LogLevel ERROR
        ProxyCommand /Users/maf/Code/coder/coder --global-config "/Users/maf/Library/Application Support/coderv2" ssh --stdio --hostname-suffix coder %h

Copy link
Member

@ethanndickson ethanndickson left a comment

Choose a reason for hiding this comment

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

LGTM after the -o settings are added to the non-proxycommand.

It'd be nice to have two entries in the SSH config, i.e:

Host *.coder
       Foo=Bar
Match host *.coder !exec "/Users/maf/Code/coder/coder connect exists %h"
...

It's confusing that this works, I thought the rule was whatever Host appeared first in the config was used, so that this would never hit the match block. Apparently not.

This also (perhaps inadvertently) fixes most issues when trying to connect to stopped workspaces as it will automatically proxy the first time and trigger autostart.

This is sick

@spikecurtis spikecurtis force-pushed the spike/connect-exists-cmd branch from 169ab02 to a89e1c3 Compare April 17, 2025 06:57
@spikecurtis spikecurtis force-pushed the spike/config-ssh-match-suffix branch from 3073db4 to 2bb6713 Compare April 17, 2025 06:57
@spikecurtis spikecurtis force-pushed the spike/connect-exists-cmd branch from a89e1c3 to dc5fab3 Compare April 17, 2025 07:12
@spikecurtis spikecurtis force-pushed the spike/config-ssh-match-suffix branch from 2bb6713 to afbcc03 Compare April 17, 2025 07:12
@spikecurtis spikecurtis changed the base branch from spike/connect-exists-cmd to graphite-base/17419 April 17, 2025 07:23
@spikecurtis spikecurtis force-pushed the spike/config-ssh-match-suffix branch from afbcc03 to a8cf4c3 Compare April 17, 2025 07:23
@spikecurtis spikecurtis force-pushed the graphite-base/17419 branch from dc5fab3 to 3b54254 Compare April 17, 2025 07:23
@graphite-app graphite-app bot changed the base branch from graphite-base/17419 to main April 17, 2025 07:24
@spikecurtis spikecurtis force-pushed the spike/config-ssh-match-suffix branch from a8cf4c3 to c4e9eeb Compare April 17, 2025 07:24
Copy link
Contributor Author

Ok, now looks like

Host *.coder
        ConnectTimeout=0
        StrictHostKeyChecking=no
        UserKnownHostsFile=/dev/null
        LogLevel ERROR

Match host *.coder !exec "C:\Users\micha\repos\coder\coder.exe connect exists %h"
        ProxyCommand C:\Users\micha\repos\coder\coder.exe --global-config C:\Users\micha\AppData\Roaming\coderv2 ssh --stdio --hostname-suffix coder %h

@spikecurtis spikecurtis force-pushed the spike/config-ssh-match-suffix branch from c4e9eeb to 77364ba Compare April 17, 2025 07:47
@spikecurtis spikecurtis merged commit b0854aa into main Apr 17, 2025
39 checks passed
Copy link
Contributor Author

Merge activity

  • Apr 17, 4:04 AM EDT: A user merged this pull request with Graphite.

@spikecurtis spikecurtis deleted the spike/config-ssh-match-suffix branch April 17, 2025 08:04
@github-actions github-actions bot locked and limited conversation to collaborators Apr 17, 2025
@mafredri
Copy link
Member

Ok, now looks like

Host *.coder
        ConnectTimeout=0
        StrictHostKeyChecking=no
        UserKnownHostsFile=/dev/null
        LogLevel ERROR

Match host *.coder !exec "C:\Users\micha\repos\coder\coder.exe connect exists %h"
        ProxyCommand C:\Users\micha\repos\coder\coder.exe --global-config C:\Users\micha\AppData\Roaming\coderv2 ssh --stdio --hostname-suffix coder %h

@spikecurtis works as expected now, thanks! It'd be nice to move those default ones (at least ConnectTimeout, StrictHostKeyChecking, and UserKnownHostsFile, but probably LogLevel too) to the match block though, since host key verification works well over Coder Connect (seems a shame to disable it) and the others are most likely related to proxying as well.

Copy link
Contributor Author

Honestly, we can probably retire StrictHostKeyChecking and UserKnownHostsFile settings entirely at some point soon, since we've implemented steps to ensure Agents use the same key on rebuilds.

@mafredri
Copy link
Member

Oh yeah, I thought it was still a requirement for the proxy command but it seems you are right, we can totally do that! I think the ConnectTimeout and LogLevel ERROR are still required to be able to wait for a workspace to start and perhaps show output as well, but have not verified.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants