Provide an alternative implementation of Net::SSH::KnownHost in ssh_options #330

byroot · 2016-02-11T21:06:17Z

As discussed in #326 (comment)
Net::SSH re-parse the known_hosts files every time it needs to lookup for a known key.
This alternative implementation parse it once and for all, and cache the result.

NB: This is not complete yet, I need to also make test_known_hosts_for_when_an_host_hash_is_recognized pass.

cc @mattbrictson @TiagoCardoso1983

mattbrictson · 2016-02-11T22:49:29Z

@byroot I haven't had a chance to review yet, but in the meantime can you look into this test failure reported by Travis? Thanks.

SSHKit::Backend::TestNetssh
[]
  test_known_hosts_for_when_an_host_hash_is_recognized            FAIL (0.00s)
Minitest::Assertion:         Expected: 1
          Actual: 0
        /home/travis/build/capistrano/sshkit/test/unit/backends/test_netssh.rb:72:in `perform_known_hosts_test'
        /home/travis/build/capistrano/sshkit/test/unit/backends/test_netssh.rb:62:in `test_known_hosts_for_when_an_host_hash_is_recognized'

byroot · 2016-02-11T22:53:07Z

That's what I've put in my NB. I haven't finished yet :)

mattbrictson · 2016-02-11T23:24:28Z

Oops, sorry! 🤦

byroot · 2016-02-17T21:21:20Z

@mattbrictson I finally got around to finish the feature.

mattbrictson · 2016-02-18T17:12:14Z

Not sure if you saw this already, but RuboCop has flagged something that is breaking the build:

lib/sshkit/backends/netssh.rb:38:25: W: Assignment in condition - you probably meant to use ==.
            if key_list = keys[host]

https://travis-ci.org/capistrano/sshkit/jobs/109975382

byroot · 2016-02-18T17:47:20Z

Nope. But i'll fix it

byroot · 2016-02-18T18:07:37Z

Done 👌

mattbrictson · 2016-02-21T01:45:22Z

@byroot I think perhaps some documentation is in order to explain how this is supposed to be used?

I did my best to test this as follows:

Checked out net-ssh@master
Used your branch of sshkit
In my deploy.rb, added to my ssh_options: :known_hosts => SSHKit::Backend::Netssh::KnownHosts.new
Tried a cap production deploy:check

But:

NoMethodError: undefined method `each' for nil:NilClass
sshkit/backends/netssh.rb:41:in `block in keys_for'
sshkit/backends/netssh.rb:37:in `each'
sshkit/backends/netssh.rb:37:in `keys_for'
sshkit/backends/netssh.rb:132:in `block in search_for'
sshkit/backends/netssh.rb:131:in `map'
sshkit/backends/netssh.rb:131:in `search_for'
net/ssh/transport/algorithms.rb:246:in `prepare_preferred_algorithms!'
net/ssh/transport/algorithms.rb:124:in `initialize'
net/ssh/transport/session.rb:86:in `new'
net/ssh/transport/session.rb:86:in `initialize'
net/ssh.rb:227:in `new'
net/ssh.rb:227:in `start'

Is there more I have to do to use this correctly?

byroot · 2016-03-09T18:31:22Z

@mattbrictson apologies, I forgot to test in realistic conditions :/ and then got little time to fix it.

I just fixed the issue you had, and tested that branch against our capistrano recipe.

I think perhaps some documentation is in order to explain how this is supposed to be used?

It's exactly what you figured out:

In my deploy.rb, added to my ssh_options: :known_hosts => SSHKit::Backend::Netssh::KnownHosts.new

What kind of documentation are you thinking about? A README section?

I was seeing this as a beta feature for now that you have to explicitly and then maybe it could become the default in a future release?

Let me know.

mattbrictson · 2016-03-11T13:34:55Z

What kind of documentation are you thinking about? A README section?

Yes, I think a small mention in the README near where we talk about connection pooling would be good.

I was seeing this as a beta feature for now that you have to explicitly and then maybe it could become the default in a future release?

Sounds good. Please open a separate issue for making it the default, so that we don't lose track of that task.

👍

byroot · 2016-03-11T19:16:38Z

@mattbrictson I had to change the PR a bit with 25510e4 to react to net-ssh/net-ssh#320

Basically there is 2 possibilities:

net-ssh < 3.1.0: Even if you pass known_hosts: SSHKit::Backends::Netssh::KnownHosts.new it won't be used by net-ssh
net-ssh >- 3.1.0: Then it will be used.

I can setup a Travis build matrix with 2 versions of net-ssh if you want.

I also updated the README, let me know.

byroot · 2016-03-11T20:16:40Z

Oh and the profiles:

Without known_hosts caching:

==================================
  Mode: wall(1000)
  Samples: 3397 (6.44% miss rate)
  GC: 264 (7.77%)
==================================
     TOTAL    (pct)     SAMPLES    (pct)     FRAME
      1325  (39.0%)        1321  (38.9%)     SSHKit::Runner::Parallel#execute
       234   (6.9%)         234   (6.9%)     block (3 levels) in Net::SSH::KnownHosts#keys_for
       418  (12.3%)         164   (4.8%)     block (2 levels) in Net::SSH::KnownHosts#keys_for
       145   (4.3%)         107   (3.1%)     Net::SSH::Transport::Kex::DiffieHellmanGroup1SHA1#send_kexinit
       175   (5.2%)          99   (2.9%)     OpenSSL::PKey::DH#valid?
        92   (2.7%)          92   (2.7%)     Net::SSH::Compat.io_select
        76   (2.2%)          76   (2.2%)     block in OpenSSL::PKey::DH#valid?
        63   (1.9%)          63   (1.9%)     block (2 levels) in Net::SSH::Transport::ServerVersion#negotiate!
        49   (1.4%)          49   (1.4%)     block (2 levels) in Net::SSH::Connection::Channel#forward_local_env
        73   (2.1%)          44   (1.3%)     block in Net::SSH::Config.load
        46   (1.4%)          40   (1.2%)     Net::SSH::Buffer#read
       105   (3.1%)          32   (0.9%)     Net::SSH::Config.load
        37   (1.1%)          29   (0.9%)     Net::SSH::Transport::State#update_next_iv
        54   (1.6%)          27   (0.8%)     Net::SSH::BufferedIo#fill
        27   (0.8%)          27   (0.8%)     Net::SSH::Buffer#write_long
        23   (0.7%)          23   (0.7%)     Net::SSH::Config.pattern2regex
        23   (0.7%)          23   (0.7%)     Logger#debug?
        23   (0.7%)          23   (0.7%)     Net::SSH::Transport::HMAC::Abstract.mac_length
        22   (0.6%)          22   (0.6%)     Net::SSH::Transport::HMAC::Abstract.digest_class
        21   (0.6%)          21   (0.6%)     Net::SSH::BufferedIo#input
       448  (13.2%)          20   (0.6%)     Net::SSH::KnownHosts#keys_for
        46   (1.4%)          19   (0.6%)     Net::SSH::Buffer#read_long
       775  (22.8%)          18   (0.5%)     block in Net::SSH::Transport::Session#poll_message
        18   (0.5%)          18   (0.5%)     Net::SSH::Buffer#initialize
        65   (1.9%)          17   (0.5%)     block in Net::SSH::Buffer.from
        16   (0.5%)          16   (0.5%)     Net::SSH::KnownHosts#known_host_hash?
        82   (2.4%)          15   (0.4%)     Net::SSH::Buffer.from
       175   (5.2%)          14   (0.4%)     Net::SSH::Transport::PacketStream#poll_next_packet
        14   (0.4%)          14   (0.4%)     Net::SSH::Buffer#to_s
        14   (0.4%)          14   (0.4%)     block (3 levels) in <class:Digest>

With known_hosts caching:

==================================
  Mode: wall(1000)
  Samples: 2845 (9.39% miss rate)
  GC: 198 (6.96%)
==================================
     TOTAL    (pct)     SAMPLES    (pct)     FRAME
      1004  (35.3%)        1001  (35.2%)     SSHKit::Runner::Parallel#execute
       259   (9.1%)         241   (8.5%)     SSHKit::Backend::ConnectionPool#find_cache
       153   (5.4%)         153   (5.4%)     Net::SSH::Compat.io_select
       151   (5.3%)         116   (4.1%)     Net::SSH::Transport::Kex::DiffieHellmanGroup1SHA1#send_kexinit
       173   (6.1%)         114   (4.0%)     OpenSSL::PKey::DH#valid?
        60   (2.1%)          60   (2.1%)     block (2 levels) in Net::SSH::Transport::ServerVersion#negotiate!
        59   (2.1%)          59   (2.1%)     block in OpenSSL::PKey::DH#valid?
        61   (2.1%)          41   (1.4%)     block in Net::SSH::Config.load
        95   (3.3%)          34   (1.2%)     Net::SSH::Config.load
        34   (1.2%)          34   (1.2%)     block (2 levels) in Net::SSH::Connection::Channel#forward_local_env
        64   (2.2%)          31   (1.1%)     Net::SSH::BufferedIo#fill
        37   (1.3%)          30   (1.1%)     Net::SSH::Buffer#read
       157   (5.5%)          27   (0.9%)     Net::SSH::Transport::PacketStream#poll_next_packet
        27   (0.9%)          27   (0.9%)     Net::SSH::Buffer#initialize
        26   (0.9%)          26   (0.9%)     Logger#debug?
        24   (0.8%)          24   (0.8%)     Net::SSH::Buffer#length
        21   (0.7%)          21   (0.7%)     Net::SSH::Buffer#write_long
        19   (0.7%)          19   (0.7%)     Net::SSH::Transport::HMAC::Abstract.mac_length
        43   (1.5%)          19   (0.7%)     Net::SSH::BufferedIo#send_pending
        19   (0.7%)          19   (0.7%)     block in Net::SSH::Transport::PacketStream#enqueue_packet
        22   (0.8%)          18   (0.6%)     Net::SSH::Transport::State#update_next_iv
        18   (0.6%)          18   (0.6%)     Net::SSH::BufferedIo#input
        25   (0.9%)          17   (0.6%)     Net::SSH::Authentication::Agent#read_packet
        16   (0.6%)          16   (0.6%)     Net::SSH::Buffer#to_s
        15   (0.5%)          14   (0.5%)     OpenSSL::PKey::RSA#ssh_do_verify
        14   (0.5%)          14   (0.5%)     Net::SSH::Config.pattern2regex
        14   (0.5%)          14   (0.5%)     Net::SSH::Buffer#append
        13   (0.5%)          13   (0.5%)     block (3 levels) in <class:Digest>
        23   (0.8%)          13   (0.5%)     Net::SSH::KeyFactory.load_public_key
        11   (0.4%)          11   (0.4%)     Net::SSH::Transport::HMAC::Abstract.digest_class

mattbrictson · 2016-04-22T16:33:00Z

@byroot I am just now coming back to this PR. Let's try to get this merged in.

Can you fix the merge conflicts?
netssh.rb is getting pretty huge. What do you think about moving the new KnownHosts and KnownHostsKeys classes into separate files?
Otherwise, in your estimation, is this PR ready to go?

Thanks!

byroot · 2016-04-22T16:53:33Z

Make dense, i'll do all that in a few minutes

mattbrictson · 2016-04-22T17:05:23Z

Great. I'm planning on releasing SSHKit 1.10 today, and it would be nice to get this in.

…ptions As discussed in capistrano#326 (comment) Net::SSH re-parse the known_hosts files every time it needs to lookup for a known key. This alternative implementation parse it once and for all, and cache the result.

byroot · 2016-04-22T17:23:34Z

Can you fix the merge conflicts?

Done

moving the new KnownHosts and KnownHostsKeys classes into separate files?

Done as well. NB: I moved them both in a single file. Since they interact a lot together I think it makes sense.

Otherwise, in your estimation, is this PR ready to go?

It's been more than a month we are running it in production now. I just checked, we deployed a bit more than a thousand time with it, so 400k SSH connections over a month. IMO we're good :)

leehambley · 2016-04-22T17:28:48Z

It's been more than a month we are running it in production now. I just checked, we deployed a bit more than a thousand time with it, so 400k SSH connections over a month. IMO we're good :)

😆 🚀

mattbrictson · 2016-04-22T17:50:53Z

Merged. Thanks! 🙌

@byroot

## [1.11.3][] (2016-09-16) * Fix known_hosts caching to match on the entire hostlist [PR #364](capistrano/sshkit#364) @byroot ## [1.11.2][] (2016-07-29) ### Bug fixes * Fixed a crash occurring when `Host@keys` was set to a non-Enumerable. @xavierholt [PR #360](capistrano/sshkit#360) ## [1.11.1][] (2016-06-17) ### Bug fixes * Fixed a regression in 1.11.0 that would cause `ArgumentError: invalid option(s): known_hosts` in some older versions of net-ssh. @byroot [#357](capistrano/sshkit#357) ## [1.11.0][] (2016-06-14) ### Bug fixes * Fixed colorized output alignment in Logger::Pretty. @xavierholt [PR #349](capistrano/sshkit#349) * Fixed a bug that prevented nested `with` calls [#43](capistrano/sshkit#43) ### Other changes * Known hosts lookup optimization is now enabled by default. @byroot ## 1.10.0 (2016-04-22) * You can now opt-in to caching of SSH's known_hosts file for a speed boost when deploying to a large fleet of servers. Refer to the [README](https://github.com/capistrano/sshkit/tree/v1.10.0#known-hosts-caching) for details. We plan to turn this on by default in a future version of SSHKit. [PR #330](capistrano/sshkit#330) @byroot * SSHKit now explicitly closes its pooled SSH connections when Ruby exits; this fixes `zlib(finalizer): the stream was freed prematurely` warnings [PR #343](capistrano/sshkit#343) @mattbrictson * Allow command map entries (`SSHKit::CommandMap#[]`) to be Procs [PR #310](capistrano/sshkit#310) @mikz ## 1.9.0 **Refer to the 1.9.0.rc1 release notes for a full list of new features, fixes, and potentially breaking changes since SSHKit 1.8.1.** There are no changes since 1.9.0.rc1. ## 1.9.0.rc1 ### Potentially breaking changes * The SSHKit DSL is no longer automatically included when you `require` it. **This means you must now explicitly `include SSHKit::DSL`.** See [PR #219](capistrano/sshkit#219) for details. @beatrichartz * `SSHKit::Backend::Printer#test` now always returns true [PR #312](capistrano/sshkit#312) @mikz ### New features * `SSHKit::Formatter::Abstract` now accepts an optional Hash of options [PR #308](capistrano/sshkit#308) @mattbrictson * Add `SSHKit::Backend.current` so that Capistrano plugin authors can refactor helper methods and still have easy access to the currently-executing Backend without having to use global variables. * Add `SSHKit.config.default_runner` options that allows to override default command runner. This option also accepts a name of the custom runner class. * The ConnectionPool has been rewritten in this release to be more efficient and have a cleaner internal API. You can still completely disable the pool by setting `SSHKit::Backend::Netssh.pool.idle_timeout = 0`. @mattbrictson @byroot [PR #328](capistrano/sshkit#328) ### Bug fixes * make sure working directory for commands is properly cleared after `within` blocks [PR #307](capistrano/sshkit#307) @steved * display more accurate string for commands with spaces being output in `Formatter::Pretty` [PR #304](capistrano/sshkit#304) @steved [PR #319](capistrano/sshkit#319) @mattbrictson * Fix a race condition experienced in JRuby that could cause multi-server deploys to fail. [PR #322](capistrano/sshkit#322) @mattbrictson

mattbrictson added the new feature label Feb 11, 2016

byroot force-pushed the known-hosts branch from d2c1983 to 799920f Compare February 17, 2016 21:20

byroot force-pushed the known-hosts branch from 799920f to 8d507be Compare February 17, 2016 21:21

byroot force-pushed the known-hosts branch from 8d507be to 8e90737 Compare February 18, 2016 18:07

mattbrictson added the need more info label Feb 24, 2016

byroot force-pushed the known-hosts branch from 8e90737 to 3f660a4 Compare March 9, 2016 18:27

byroot force-pushed the known-hosts branch 2 times, most recently from 26a97e6 to 25510e4 Compare March 11, 2016 19:12

byroot force-pushed the known-hosts branch from bd22a33 to 9714a2b Compare March 11, 2016 19:17

byroot mentioned this pull request Mar 11, 2016

Cache known hosts lookup by default #339

Merged

byroot mentioned this pull request Mar 16, 2016

Issue: Parallel executor performance is mostly linear #326

Open

mattbrictson modified the milestone: 1.11.0 Apr 22, 2016

byroot force-pushed the known-hosts branch from 9714a2b to 8aaa391 Compare April 22, 2016 17:15

mattbrictson modified the milestones: 1.10.0, 1.11.0 Apr 22, 2016

mattbrictson merged commit 8aaa391 into capistrano:master Apr 22, 2016

mattbrictson added a commit that referenced this pull request Apr 22, 2016

Merge pull request #330 from byroot/known-hosts

fcfb74e

xavierholt mentioned this pull request Jul 21, 2016

Upgrading 1.10.0 to 1.11.1 breaks connecting to new hosts. #362

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide an alternative implementation of Net::SSH::KnownHost in ssh_options #330

Provide an alternative implementation of Net::SSH::KnownHost in ssh_options #330

byroot commented Feb 11, 2016

mattbrictson commented Feb 11, 2016

byroot commented Feb 11, 2016

mattbrictson commented Feb 11, 2016

byroot commented Feb 17, 2016

mattbrictson commented Feb 18, 2016

byroot commented Feb 18, 2016

byroot commented Feb 18, 2016

mattbrictson commented Feb 21, 2016

byroot commented Mar 9, 2016

mattbrictson commented Mar 11, 2016

byroot commented Mar 11, 2016

byroot commented Mar 11, 2016

mattbrictson commented Apr 22, 2016

byroot commented Apr 22, 2016

mattbrictson commented Apr 22, 2016

byroot commented Apr 22, 2016

leehambley commented Apr 22, 2016

mattbrictson commented Apr 22, 2016

Provide an alternative implementation of Net::SSH::KnownHost in ssh_options #330

Provide an alternative implementation of Net::SSH::KnownHost in ssh_options #330

Conversation

byroot commented Feb 11, 2016

mattbrictson commented Feb 11, 2016

byroot commented Feb 11, 2016

mattbrictson commented Feb 11, 2016

byroot commented Feb 17, 2016

mattbrictson commented Feb 18, 2016

byroot commented Feb 18, 2016

byroot commented Feb 18, 2016

mattbrictson commented Feb 21, 2016

byroot commented Mar 9, 2016

mattbrictson commented Mar 11, 2016

byroot commented Mar 11, 2016

byroot commented Mar 11, 2016

mattbrictson commented Apr 22, 2016

byroot commented Apr 22, 2016

mattbrictson commented Apr 22, 2016

byroot commented Apr 22, 2016

leehambley commented Apr 22, 2016

mattbrictson commented Apr 22, 2016