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.

Sign up for GitHub

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

Jump to bottom

Allow ssh/config to accept string, or array of strings #1129

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Allow ssh/config to accept string, or array of strings #1129

wants to merge 3 commits into from
58 −4

Conversation

Burgestrand
Copy link

@Burgestrand Burgestrand commented Oct 17, 2024 •
edited
Loading

Note

I made this PR rogue, in the sense that I forked and made a change that seems to have worked without discussing this change with anybody else first. I don't necessarily expect this to be merged. 🙂

On the topic of passing through config I would like to use this option, so that I can embed a config/ssh_config with my project and have Kamal use it, instead of having to modify my global ~/.ssh/config.

There are the following changes in this PR:

  • Actually pass ssh/config through to the SSH options, so that the documentation is accurate.
  • Modify run_over_ssh (shelling out to SSH) to actually use the config file if one is passed, or none if false is passed
  • Modify the validation to allow a path to a config file, in addition to the boolean
  • Modify the documentation because, we don't actually support an array of files

Supporting a string path allows support for using an ssh_config-file in Kamal, similar to this:

Host web
  HostName 8.8.8.8
  User kamal

Host accessories
  HostName 10.0.0.3
  User kamal
  ProxyJump web

... which then allows you to specify the hosts using the names, rather than IPs. The same file can also be used when SSHing in manually, using ssh -F ssh_config web.

This is reviewable commit-by-commit.

To do

  • Main question: is this change wanted?
  • Add tests to ensure this keeps working.

@Burgestrand Burgestrand mentioned this pull request Oct 17, 2024
Merged
@Burgestrand Burgestrand force-pushed the kbs/allow-ssh_options-in-ssh-config branch 2 times, most recently from 14cde2c to 7abd339 Compare October 17, 2024 18:08
@Burgestrand
Copy link
Author

FYI I force-pushed, because I added tests to each commit.

@Burgestrand

This comment was marked as outdated.

Sign in to view
This was referenced Oct 25, 2024
Closed
Closed
@GideonStowell
Copy link

@Burgestrand any updates on this?
I ran into this as well and very interested in getting this functionality added.

@Burgestrand
Copy link
Author

Burgestrand commented Oct 28, 2024 •
edited
Loading

For now this got me far enough to notice there's more to it to get it to work for a few specific commands.

I don't know which parts need to change to get e.g. log following to respect this flag. I won't be working on this for at least a few weeks, but if you want to pick up where I left off absolutely feel free.

On top of that I would say this is still missing the blessing of the maintainers of Kamal, i.e. is this something they'd like to merge and ultimately support.

@GideonStowell
Copy link

I would hope it is blessed by the maintainers because @jeremy linked this PR when I raised an issue abt the incorrect documentation related to ssh config.

Is the issue time or other resources? I don't have my ruby merit badge but I'd be willing to sponsor some hours if that's a motivator.

@Burgestrand Burgestrand force-pushed the kbs/allow-ssh_options-in-ssh-config branch from 7abd339 to d1749ca Compare October 28, 2024 21:32
@Burgestrand
Actually pass ssh/config through to SSHKit options
e8a41af 
This option is documented as available since basecamp#908 in:
https://github.com/basecamp/kamal/blob/74a06b0ccda616c86ebe1729d0795f39bcac9f00/lib/kamal/configuration/docs/ssh.yml#L65-L70

However, before this the options don't seem to pass through to SSHKit:
https://github.com/basecamp/kamal/blob/74a06b0ccda616c86ebe1729d0795f39bcac9f00/lib/kamal/commander.rb#L167

Since `config` isn't actually returned in `#options`.
@Burgestrand
Allow ssh/config to additionally accept a string
3a634ae 
This is accepted by Net::SSH, research done by @jeremy in basecamp#908 (comment)

This is already documented as working correctly in https://github.com/basecamp/kamal/blob/74a06b0ccda616c86ebe1729d0795f39bcac9f00/lib/kamal/configuration/docs/ssh.yml#L65-L70

However, before this change only booleans were allowed because of the example configuration file.
@Burgestrand
ssh/config does not accept an array of paths
25a3f5d 
`ssh -F` only accepts a single config file, and it's what we use for interactive commands, e.g. `kamal app logs --follow`.
@Burgestrand Burgestrand force-pushed the kbs/allow-ssh_options-in-ssh-config branch from d1749ca to 25a3f5d Compare October 28, 2024 21:41
@Burgestrand
Copy link
Author

Is the issue time or other resources? [...]

Motivation and time expenditure! I've been experimenting with Kamal for future projects, but paid client work got in between and I'm not using Kamal for them in the near future 😊

Regardless, I had some time to think about the "interactive commands" problem in the car today and figured "how hard can it be?", so now this PR also has some adjustments so that e.g. kamal app logs --follow work properly.

In doing that work I noticed the ssh CLI doesn't accept more than one ssh_config through -F, so I opted to remove the array option as a minimum common supported option between SSH and SSHKit.

@GideonStowell
Copy link

@djmb @Sija @jeremy
Any of you able to review these changes?

@sammoore
Copy link

sammoore commented Nov 20, 2024 •
edited
Loading

1. Ended up here because the Kamal documentation says you can set ssh/config to a file path (or array of paths) to load specific configuration, but per the above comment this appears to be incorrect, and I'm getting the error:

ERROR (Kamal::ConfigurationError): ssh/config: should be a boolean

Initially, I tried overriding the ssh configuration on a per-role basis (as the documentation says "overwrite other settings from the root configuration") just to get something working, but that too seems unsupported. So into this PR I went...

Use Case

I need multiple servers/VMs that have the same IP address to be treated as separate servers (they have different TCP ports for their SSH daemon, behind some proxy -- I'm deploying to TensorDock and this can happen). This PR was close to making my specific use case possible, but:

  • Kamal::Configuration::Ssh provides sane defaults for user and port, so they are always set within the #options hash
  • Kamal::Commands::Base#run_over_ssh specifies (or in my case case, overrides) the user and port CLI arguments based on the aforementioned options hash
  • Kamal::Commander (which appears to be a singleton) sets the SSHKit's ssh_options with these also present, which overrides the settings for the Host if present in the config.

Solution

I've worked around this with additional changes by essentially ignoring/disabling the user and port settings from the config if the SSH config parameter is a string (i.e. it should be a path to an SSH configuration):

  • do not prefix the host with the username@ or specify the port with the -p flag when running SSH commands
  • do not include user or port in the options hash, and thus the ssh_config passed to SSHKit

Another solution I thought of was supporting an explicit nil for user and port within the Kamal config, but I didn't want to update the validation logic just to get a working proof of concept.

I have not written tests (or even ran the existing ones yet) for that change, but so far so good. I may revisit the tests before I move to production, depending on the response here; if these semantics are something Kamal would be interested in adopting, let me know if I can help.

P.S. if there's another solution to my use-case that I'm missing and uses project-specific configuration, please let me know 🧙

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Burgestrand @GideonStowell @sammoore