- Notifications
You must be signed in to change notification settings - Fork928
feat(cli): support header and header-command in config-ssh#10413
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
mafredri commentedNov 21, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hey@JoshVee, thanks for the PR and sorry for the long wait-time. I think this PR looks good, but there's a change I'd like to see before we merge it, and that's support for serializing the header command into the options. For instance, compare these two: ❯./coder config-ssh --header-command'echo foo=bar'>The following changes will be made to your SSH configuration: * Update the coder section in /home/coder/.ssh/coder Continue? (yes/no)yes Versus: ❯./coder config-ssh --header-command'echo foo=bar' -o ForwardAgent=yes>New options differ from previous options: New options: * ssh-option: ForwardAgent=yes Previous options: none (<- should list header command) Use new options? (yes/no)yes>The following changes will be made to your SSH configuration: * Use new options * Update the coder section in /home/coder/.ssh/coder Continue? (yes/no)yes Now let's say we forget about the header command next run, there's only going to be a cryptic change to the SSH config, and users may be left wondering why SSH isn't working afterwards (the update removes the header command, but doesn't say that the options/settings have changed): ❯./coder config-ssh -o ForwardAgent=yes>The following changes will be made to your SSH configuration: * Update the coder section in /home/coder/.ssh/coder Continue? (yes/no) The options are serialized under # ------------START-CODER-----------# This section is managed by coder. DO NOT EDIT.## You should not hand-edit this section unless you are removing it, all# changes will be lost when running "coder config-ssh".## Last config-ssh options:# :ssh-option=ForwardAgent=yes# I would suggest something like |
@JoshVee are you still interested in working on this PR? If not, we can see if someone on the team has the bandwidth to tackle it. |
| rootFlags += fmt.Sprintf(" --header %q", h) | ||
| } | ||
| if sshConfigOpts.headerCommand != "" { | ||
| rootFlags += fmt.Sprintf(" --header-command %q", sshConfigOpts.headerCommand) |
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.
Review: We shouldn't need to do special escaping here like we do for the coder binary, that's because we're simply passing a string argument to coder.
Updated the PR with changes to support serialization, while I was at it, I included support for ❯./coder config-ssh --header-command'echo "foo=bar"' -o ForwardAgent=yes>New options differ from previous options: New options: * ssh-option: ForwardAgent=yes * header-command: echo "foo=bar" Previous options: * ssh-option: ForwardAgent=yes * header: foo=bar Ready for review@Emyrk. |
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.
👍
| a = slices.Clone(a) | ||
| slices.Sort(a) | ||
| b = slices.Clone(b) | ||
| slices.Sort(b) |
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.
nit: curious, can we replaceslices.Clone with a simple copy to omit cloning and just operate on same objects?
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.
// Clone returns a copy of the slice.
// The elements are copied using assignment, so this is a shallow clone.
It's actually just a simple copy, so I think it's fine.
Thanks for your contribution@JoshVee, this finally went in! |
This PR modifies the
config-sshcommand so that it includes any provided--header-command\CODER_HEADER_COMMANDin the generated SSHProxyCommand.This ensures that any custom headers are also available when running
ssh coder.workspace.The avoids the need to always rely on having the
CODER_HEADER_COMMANDenvironment variable set globally.