Skip to content
This repository has been archived by the owner on Mar 19, 2022. It is now read-only.

Shorten ssh control path #470

Merged
merged 1 commit into from
Apr 7, 2016

Conversation

mmrwoods
Copy link
Contributor

My previous commit to "fix" the ssh control path by including the remote
user and port also made it more likely that the resulting path would
exceed the maximum allowed length of the path to a unix domain socket.

I've replaced the use of '%r@%h:%p' in the ssh control path with '%C',
which is substituted by a hash of the concatenation of '%l%h%p%r', so it
still includes the remote user, host and port, but is also a predictable
length and less likely to exceed the maximum allowed length of a path to
a unix domain socket (104 "chars" on OSX, 108 I think on Linux).

The change was made after I ran into a "too long for Unix domain socket"
error myself when cooking a node, and has been tested in production.

@mmrwoods
Copy link
Contributor Author

Sorry, I brokes the ssh control path with my earlier commit to fix the ssh control path :-(

I think I've fixed it properly now :-)

@matschaffer
Copy link
Owner

Thanks! Do you happen to know how well supported that option is on Linux? I know OS X just got it recently so I'm a little hesitant to support it.

Tbh, I think I may release a new 0.5 that defaults to off. It's causing more bug reports than pretty much any other feature to date.

@mmrwoods
Copy link
Contributor Author

Ah, good point, I didn't think to check that the option was widely supported, and it's not, yet...

Support for %C in the control path was introduced in openssh 6.7 (http://www.openssh.com/txt/release-6.7), but the latest openssh for CentOS 7.2 is version 6.6.1. Supported LTS versions of Ubuntu are also stuck on earlier openssh versions.

Could just take a string of the user, host and port and hash it in ruby. This seems quite a fudge, but also seems necessary if the feature is to support older ssh clients.

TBH, I think you're probably right to consider changing the default for this option to off. It's probably more important that a client tool like knife-solo just works rather than works fast, and experienced sysadmins will know how to enable ssh multiplexing in their own local ssh config anyway. If the option was off by default, it could still use %C in the control path when enabled, which would mean it would also mostly just work once enabled on a typical developer workstation (highly unlikely to be running something like RHEL or Ubuntu LTS).

Thank you btw for knife-solo, it makes my life easier :-)

@matschaffer
Copy link
Owner

Yeah, that makes sense. That made me consider that a whole other side to this is that you can probably get similar speedup by configuring ssh completely externally of knife-solo.

Thanks for using it and glad it's helping!

My previous commit to "fix" the ssh control path by including the remote
user and port also made it more likely that the resulting path would
exceed the maximum allowed length of the path to a unix domain socket.

I've replaced the use of '%r@%h:%p' in the ssh control path with '%C',
which is substituted by a hash of the concatenation of '%l%h%p%r', so it
still includes the remote user, host and port, but is also a predictable
length and less likely to exceed the maximum allowed length of a path to
a unix domain socket (104 "chars" on OSX, 108 I think on Linux).

The change was made after I ran into a "too long for Unix domain socket"
error myself when cooking a node, and has been tested in production.
@mmrwoods
Copy link
Contributor Author

mmrwoods commented Apr 7, 2016

Rebased on master and force pushed

@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.661% when pulling 5c31058 on thickpaddy:shorten_ssh_control_path into 3b3fa54 on matschaffer:master.

@matschaffer matschaffer merged commit 065d3b7 into matschaffer:master Apr 7, 2016
@matschaffer matschaffer removed the ready label Apr 7, 2016
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