(BKR-1423) Fix SSH settings in nodesets

[trevor-vaughan]

Allow SSH settings from nodesets to propagate properly into the host's
SSH configuration.

BKR-1423 #close

[trevor-vaughan]

@sbeaulie This is the correct fix instead of the PR in voxpupuli/beaker#1633

[trevor-vaughan]

@sbeaulie The issue is that if the :config item is populated, literally everything else is overridden which was the crux of the original problem. We may need an auxillary data structure for rsync.

[sbeaulie]

I've traced the code and the only place it seems to be used is with the rsynch method in beaker so I don't see where it would conflict - can you shed light on where it would replace everything? I can test my suggestion with the pupmod-simp-acpid project you suggested.

[trevor-vaughan]

The host['ssh'] hash is passed directly into the Net::SSH connection configuration. If host['ssh'][:config] is populated, Net::SSH will pick it up and run with it.

We almost need a host['ssh'][:cli_config] or something.

[trevor-vaughan]

Testing a solution now.

[sbeaulie]

This is the code where it is being used: https://github.com/puppetlabs/beaker/blob/master/lib/beaker/host.rb#L550-L552

We could change the name in both places. But I still don't see where NET::SSH would pick the host['ssh'][:config] up.

I tested with having both :config and the merge and it passed in pupmod-simp-acpid. The connection attempts look like this:

Attempting ssh connection to 10.X.X.X, user: root, opts: {:config=>"/var/folders/yb/y8xggbwd5l17w64zjz174h4c0000gn/T/el620200318-3752-y2mnkr", :verify_host_key=>false, :auth_methods=>["none", "publickey", "keyboard-interactive"], :port=>2222, :forward_agent=>true, :keys=>["XX/insecure_private_key"], :user_known_hosts_file=>"/dev/null", :keepalive=>true, :host_name=>"127.0.0.1", :user=>"root", :keys_only=>true, :logger=>...}

as you can see the config is there, and it succesfully merged the port from the configuration file.

[sbeaulie]

Can I ask you to test with my suggested change and give me the details on the failure you get?

Thanks!

[trevor-vaughan]

It will work, because it's ignoring everything else.

See the :config option in Net::SSH itself https://www.rubydoc.info/github/net-ssh/net-ssh/Net/SSH

This basically seems like it's fixing the original issue, but it's not because all of the other options in the hash are now ignored.

[trevor-vaughan]

If you try sticking something that Net::SSH can't handle into the Hash, you can see it fail.

[trevor-vaughan]

Ok, this commit, when combined with voxpupuli/beaker#1634 should do the right thing in all cases (I think)

[sbeaulie]

ok I'm following now 😄 From the Net SSH config class documents:

Note that you will never need to use this class directly–you can control whether the OpenSSH configuration files are read by passing the :config option to Net::SSH.start. (They are, by default.)

In beaker we run start like so Net::SSH.start(host, user, ssh_opts) where ssh_opts[:config] can be set as follows

:config => set to true to load the default OpenSSH config files (~/.ssh/config, /etc/ssh_config), or to false to not load them, or to a file-name (or array of file-names) to load those specific configuration files. Defaults to true.

If we look at the source code for Net:SSH version 5.0 used in beaker you are correct that the ssh_opts hash will return an ArgumentError for any non valid options:
github

The only thing in the start method that is different from the experience you describe is that it seems like it would load the file from :config (if its a file path) and then merge the other options on top, so the passed hash should take precedence, and not the :config:
https://github.com/net-ssh/net-ssh/blob/v5.0.0/lib/net/ssh.rb#L221

In conclusion, I'm happy with this fix, :vagrant_ssh_config should not be put in the 'ssh' hash, so putting it within the host is fine. Is there any documentation that would need updated for this new field? Since we do not make that variable avalable to users, and its only used for internal logic between beaker-vagrant and beaker I think it is fine without.

One thing left to fix is the Unit tests. See the travis results: https://travis-ci.com/github/puppetlabs/beaker-vagrant/jobs/299555431
I think the first failed test's expect can be changed to the new :vagrant_ssh_config and the second failed test's expect is probably not needed.