Merge pull request #3857 from coderanger/gateway-key

Refactor knife ssh options stuff

3 files changed, 82 insertions, 36 deletions

diff --git a/lib/chef/knife/ssh.rb b/lib/chef/knife/ssh.rb
index 68e01cf94f..bb3d9d78bb 100644
--- a/lib/chef/knife/ssh.rb
+++ b/lib/chef/knife/ssh.rb
@@ -132,15 +132,19 @@ class Chef
         if config[:ssh_gateway]
           gw_host, gw_user = config[:ssh_gateway].split('@').reverse
           gw_host, gw_port = gw_host.split(':')
-          gw_opts = gw_port ? { :port => gw_port } : {}
+          gw_opts = session_options(gw_host, gw_port, gw_user)
+          user = gw_opts.delete(:user)
 
-          session.via(gw_host, gw_user || config[:ssh_user], gw_opts)
+          begin
+            # Try to connect with a key.
+            session.via(gw_host, user, gw_opts)
+          rescue Net::SSH::AuthenticationFailed
+            prompt = "Enter the password for #{user}@#{gw_host}: "
+            gw_opts[:password] = prompt_for_password(prompt)
+            # Try again with a password.
+            session.via(gw_host, user, gw_opts)
+          end
         end
-      rescue Net::SSH::AuthenticationFailed
-        user = gw_user || config[:ssh_user]
-        prompt = "Enter the password for #{user}@#{gw_host}: "
-        gw_opts.merge!(:password => prompt_for_password(prompt))
-        session.via(gw_host, user, gw_opts)
       end
 
       def configure_session
@@ -204,32 +208,48 @@ class Chef
         list
       end
 
-      def session_from_list(list)
-        list.each do |item|
-          host, ssh_port = item
-          Chef::Log.debug("Adding #{host}")
-          session_opts = {}
-
-          ssh_config = Net::SSH.configuration_for(host)
-
+      # Net::SSH session options hash for global options. These should be
+      # options that will apply to the gateway connection in addition to the
+      # main one.
+      #
+      # @since 12.5.0
+      # @param host [String] Hostname for this session.
+      # @param port [String] SSH port for this session.
+      # @param user [String] Optional username for this session.
+      # @return [Hash<Symbol, Object>]
+      def session_options(host, port, user=nil)
+        ssh_config = Net::SSH.configuration_for(host)
+        {}.tap do |opts|
           # Chef::Config[:knife][:ssh_user] is parsed in #configure_user and written to config[:ssh_user]
-          user = config[:ssh_user] || ssh_config[:user]
-          hostspec = user ? "#{user}@#{host}" : host
-          session_opts[:keys] = File.expand_path(config[:identity_file]) if config[:identity_file]
-          session_opts[:keys_only] = true if config[:identity_file]
-          session_opts[:password] = config[:ssh_password] if config[:ssh_password]
-          session_opts[:forward_agent] = config[:forward_agent]
-          session_opts[:port] = config[:ssh_port] ||
-                                ssh_port || # Use cloud port if available
-                                Chef::Config[:knife][:ssh_port] ||
-                                ssh_config[:port]
-          session_opts[:logger] = Chef::Log.logger if Chef::Log.level == :debug
-
+          opts[:user] = user || config[:ssh_user] || ssh_config[:user]
+          if config[:identity_file]
+            opts[:keys] = File.expand_path(config[:identity_file])
+            opts[:keys_only] = true
+          end
+          # Don't set the keys to nil if we don't have them.
+          forward_agent = config[:forward_agent] || ssh_config[:forward_agent]
+          opts[:forward_agent] = forward_agent unless forward_agent.nil?
+          port ||= ssh_config[:port]
+          opts[:port] = port unless port.nil?
+          opts[:logger] = Chef::Log.logger if Chef::Log.level == :debug
           if !config[:host_key_verify]
-            session_opts[:paranoid] = false
-            session_opts[:user_known_hosts_file] = "/dev/null"
+            opts[:paranoid] = false
+            opts[:user_known_hosts_file] = '/dev/null'
           end
+        end
+      end
 
+      def session_from_list(list)
+        list.each do |item|
+          host, ssh_port = item
+          Chef::Log.debug("Adding #{host}")
+          session_opts = session_options(host, ssh_port)
+          # Handle port overrides for the main connection.
+          session_opts[:port] = Chef::Config[:knife][:ssh_port] if Chef::Config[:knife][:ssh_port]
+          session_opts[:port] = config[:ssh_port] if config[:ssh_port]
+          # Create the hostspec.
+          hostspec = session_opts[:user] ? "#{session_opts.delete(:user)}@#{host}" : host
+          # Connect a new session on the multi.
           session.use(hostspec, session_opts)
 
           @longest = host.length if host.length > @longest
diff --git a/spec/functional/knife/cookbook_delete_spec.rb b/spec/functional/knife/cookbook_delete_spec.rb
index 15ac8f55ab..bffad8cbed 100644
--- a/spec/functional/knife/cookbook_delete_spec.rb
+++ b/spec/functional/knife/cookbook_delete_spec.rb
@@ -40,20 +40,30 @@ describe Chef::Knife::CookbookDelete do
   end
 
   context "when the cookbook doesn't exist" do
-    before do
-      @log_output = StringIO.new
-
-      Chef::Log.logger = Logger.new(@log_output)
-      Chef::Log.level = :debug
+    let(:log_output) { StringIO.new }
 
+    before do
       @knife.name_args = %w{no-such-cookbook}
       @api.get("/cookbooks/no-such-cookbook", 404, Chef::JSONCompat.to_json({'error'=>'dear Tim, no. -Sent from my iPad'}))
     end
 
+    around do |ex|
+      old_logger = Chef::Log.logger
+      old_level = Chef::Log.level
+      begin
+        Chef::Log.logger = Logger.new(log_output)
+        Chef::Log.level = :debug
+        ex.run
+      ensure
+        Chef::Log.logger = old_logger
+        Chef::Log.level = old_level
+      end
+    end
+
     it "logs an error and exits" do
-      allow(@knife.ui).to receive(:stderr).and_return(@log_output)
+      allow(@knife.ui).to receive(:stderr).and_return(log_output)
       expect {@knife.run}.to raise_error(SystemExit)
-      expect(@log_output.string).to match(/Cannot find a cookbook named no-such-cookbook to delete/)
+      expect(log_output.string).to match(/Cannot find a cookbook named no-such-cookbook to delete/)
     end
 
   end
diff --git a/spec/functional/knife/ssh_spec.rb b/spec/functional/knife/ssh_spec.rb
index 6608d05771..51524b7009 100644
--- a/spec/functional/knife/ssh_spec.rb
+++ b/spec/functional/knife/ssh_spec.rb
@@ -31,6 +31,22 @@ describe Chef::Knife::Ssh do
     @server.stop
   end
 
+  let(:ssh_config) { Hash.new }
+  before do
+    allow(Net::SSH).to receive(:configuration_for).and_return(ssh_config)
+  end
+
+  # Force log level to info.
+  around do |ex|
+    old_level = Chef::Log.level
+    begin
+      Chef::Log.level = :info
+      ex.run
+    ensure
+      Chef::Log.level = old_level
+    end
+  end
+
   describe "identity file" do
     context "when knife[:ssh_identity_file] is set" do
       before do