Switch bootstrap to use TrainConnector.

This replaces TargetHost, and the associated renames/minor updates.

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

2 files changed, 81 insertions, 97 deletions

diff --git a/lib/chef/knife/bootstrap.rb b/lib/chef/knife/bootstrap.rb
index daca0957d4..b2c4b72f5e 100644
--- a/lib/chef/knife/bootstrap.rb
+++ b/lib/chef/knife/bootstrap.rb
@@ -387,14 +387,10 @@ class Chef
 
       attr_accessor :client_builder
       attr_accessor :chef_vault_handler
-      attr_reader   :target_host
+      attr_reader   :connection
 
       deps do
         require "chef/json_compat"
-        require "tempfile"
-        require "chef_core/text" # i18n and standardized error structures
-        require "chef_core/target_host"
-        require "chef_core/target_resolver"
       end
 
       banner "knife bootstrap [PROTOCOL://][USER@]FQDN (options)"
@@ -417,7 +413,7 @@ class Chef
       #
       # @return [String] Default bootstrap template
       def default_bootstrap_template
-        if target_host.base_os == :windows
+        if connection.windows?
           "windows-#{Chef::Dist::CLIENT}-msi"
         else
           "chef-full"
@@ -482,11 +478,11 @@ class Chef
       end
 
       # Establish bootstrap context for template rendering.
-      # Requires target_host to be a live connection in order to determine
+      # Requires connection to be a live connection in order to determine
       # the correct platform.
       def bootstrap_context
         @bootstrap_context ||=
-          if target_host.base_os == :windows
+          if connection.windows?
             require "chef/knife/core/windows_bootstrap_context"
             Knife::Core::WindowsBootstrapContext.new(config, config[:run_list], Chef::Config, secret)
           else
@@ -556,17 +552,12 @@ class Chef
         bootstrap_path = upload_bootstrap(content)
         perform_bootstrap(bootstrap_path)
       ensure
-        target_host.del_file(bootstrap_path) if target_host && bootstrap_path
+        connection.del_file!(bootstrap_path) if connection && bootstrap_path
       end
 
       def register_client
         # chef-vault integration must use the new client-side hawtness, otherwise to use the
         # new client-side hawtness, just delete your validation key.
-        # 2019-04-01 TODO
-        # TODO -  should this raise if config says to use vault because json/file/item exists
-        #         but we still have a validation key?  That means we can't use the new client hawtness,
-        #         but we also don't tell the operator that their requested vault operations
-        #         won't be performed
         if chef_vault_handler.doing_chef_vault? ||
             (Chef::Config[:validation_key] &&
              !File.exist?(File.expand_path(Chef::Config[:validation_key])))
@@ -589,8 +580,8 @@ class Chef
       def perform_bootstrap(remote_bootstrap_script_path)
         ui.info("Bootstrapping #{ui.color(server_name, :bold)}")
         cmd = bootstrap_command(remote_bootstrap_script_path)
-        r = target_host.run_command(cmd) do |data|
-          ui.msg("#{ui.color(" [#{target_host.hostname}]", :cyan)} #{data}")
+        r = connection.run_command(cmd) do |data|
+          ui.msg("#{ui.color(" [#{connection.hostname}]", :cyan)} #{data}")
         end
         if r.exit_status != 0
           ui.error("The following error occurred on #{server_name}:")
@@ -603,24 +594,13 @@ class Chef
         ui.info("Connecting to #{ui.color(server_name, :bold)}")
         opts = connection_opts.dup
         do_connect(opts)
-      rescue => e
-        # Ugh. TODO: Train raises a Train::Transports::SSHFailed for a number of different errors. chef_core makes that
-        # a more general ConnectionFailed, with an error code based on the specific error text/reason provided from trainm.
-        # This means we have to look three layers into the exception to find out what actually happened instead of just
-        # looking at the exception type
-        #
-        # It doesn't help to provide our own error if it does't let the caller know what they need to identify the problem.
-        # Let's update chef_core to be a bit smarter about resolving the errors to an appropriate exception type
-        # (eg ChefCore::ConnectionFailed::AuthError or similar) that will work across protocols, instead of just a single
-        # ConnectionFailure type
-        #
-
-        if e.cause && e.cause.cause && e.cause.cause.class == Net::SSH::AuthenticationFailed
-          if opts[:password]
+      rescue Train::Error => e
+        if e.cause && e.cause.class == Net::SSH::AuthenticationFailed
+          if connection.password_auth?
             raise
           else
             ui.warn("Failed to authenticate #{opts[:user]} to #{server_name} - trying password auth")
-            password = ui.ask("Enter password for #{opts[:user]}@#{server_name} - trying password auth") do |q|
+            password = ui.ask("Enter password for #{opts[:user]}@#{server_name}.") do |q|
               q.echo = false
             end
           end
@@ -631,6 +611,9 @@ class Chef
         end
       end
 
+      # TODO - maybe remove the footgun detection this was built on.
+      # url values override CLI flags, if you provide both
+      # we'll use the one that you gave in the URL.
       def connection_protocol
         return @connection_protocol if @connection_protocol
         from_url = host_descriptor =~ /^(.*):\/\// ? $1 : nil
@@ -640,14 +623,10 @@ class Chef
       end
 
       def do_connect(conn_options)
-        # Resolve the given host name to a TargetHost instance. We will limit
-        # the number of hosts to 1 (effectivly eliminating wildcard support) since
-        # we only support running bootstrap against one host at a time.
-        resolver = ChefCore::TargetResolver.new(host_descriptor, connection_protocol,
-                                                conn_options, max_expanded_targets: 1)
-        @target_host = resolver.targets.first
-        target_host.connect!
-        target_host
+        require "chef/knife/bootstrap/train_connector"
+
+        @connection = TrainConnector.new(host_descriptor, connection_protocol, conn_options)
+        connection.connect!
       end
 
       # Fail if both first_boot_attributes and first_boot_attributes_from_file
@@ -664,7 +643,7 @@ class Chef
       # TODO test for this method
       # TODO check that the protoocol is valid.
       def validate_winrm_transport_opts!
-        return true if connection_protocol != "winrm"
+        return true unless winrm?
 
         if Chef::Config[:validation_key] && !File.exist?(File.expand_path(Chef::Config[:validation_key]))
           if config_value(:winrm_auth_method) == "plaintext" &&
@@ -739,7 +718,7 @@ class Chef
       end
 
       def winrm_warn_no_ssl_verification
-        return if connection_protocol != "winrm"
+        return unless winrm?
 
         # REVIEWER NOTE
         # The original check from knife plugin did not include winrm_ssl_peer_fingerprint
@@ -768,13 +747,8 @@ class Chef
         end
       end
 
-        #
-
-      # Create a configuration hash for TargetHost to connect
-      # to the remote host via Train.
-      #
       # @return a configuration hash suitable for connecting to the remote
-      # host via TargetHost.
+      # host via train
       def connection_opts
         return @connection_opts unless @connection_opts.nil?
         @connection_opts = {}
@@ -788,9 +762,16 @@ class Chef
         @connection_opts
       end
 
+      def winrm?
+        connection_protocol == "winrm"
+      end
+
+      def ssh?
+        connection_protocol == "ssh"
+      end
+
       # Common configuration for all protocols
       def base_opts
-        #
         port = config_value(:connection_port,
                             knife_key_for_protocol(connection_protocol, :port))
         user = config_value(:connection_user,
@@ -807,10 +788,9 @@ class Chef
       end
 
       def host_verify_opts
-        case connection_protocol
-        when "winrm"
+        if winrm?
           { self_signed: config_value(:winrm_no_verify_cert) === true }
-        when "ssh"
+        elsif ssh?
           # Fall back to the old knife config key name for back compat.
           { verify_host_key: config_value(:ssh_verify_host_key,
                                           :host_key_verify, true) === true }
@@ -959,16 +939,16 @@ class Chef
       end
 
       def upload_bootstrap(content)
-        script_name = target_host.base_os == :windows ? "bootstrap.bat" : "bootstrap.sh"
-        remote_path = target_host.normalize_path(File.join(target_host.temp_dir, script_name))
-        target_host.save_as_remote_file(content, remote_path)
+        script_name = connection.windows? ? "bootstrap.bat" : "bootstrap.sh"
+        remote_path = connection.normalize_path(File.join(connection.temp_dir, script_name))
+        connection.upload_file_content!(content, remote_path)
         remote_path
       end
 
       # build the command string for bootrapping
       # @return String
       def bootstrap_command(remote_path)
-        if target_host.base_os == :windows
+        if connection.windows?
           "cmd.exe /C #{remote_path}"
         else
           "sh #{remote_path}"
@@ -977,8 +957,9 @@ class Chef
 
       # To avoid cluttering the CLI options, some flags (such as port and user)
       # are shared between protocols.  However, there is still a need to allow the operator
-      # to specify defaults separately, since they may not be the same values for different protocols.
-      #
+      # to specify defaults separately, since they may not be the same values for different
+      # protocols.
+
       # These keys are available in Chef::Config, and are prefixed with the protocol name.
       # For example, :user CLI option will map to :winrm_user and :ssh_user Chef::Config keys,
       # based on the connection protocol in use.
diff --git a/spec/unit/knife/bootstrap_spec.rb b/spec/unit/knife/bootstrap_spec.rb
index f54c8ac1d6..ce590fc9ee 100644
--- a/spec/unit/knife/bootstrap_spec.rb
+++ b/spec/unit/knife/bootstrap_spec.rb
@@ -25,8 +25,17 @@ describe Chef::Knife::Bootstrap do
   let(:bootstrap_template) { nil }
   let(:stderr) { StringIO.new }
   let(:bootstrap_cli_options) { [ ] }
-  let(:base_os) { :linux }
-  let(:target_host) { double("TargetHost") }
+  let(:linux_test) { true }
+  let(:windows_test) { false }
+  let(:linux_test) { false }
+  let(:unix_test) { false }
+  let(:ssh_test) { false }
+
+  let(:connection) do
+    double("TrainConnector",
+                            windows?: windows_test,
+                            linux?: linux_test,
+                            unix?: unix_test) end
 
   let(:knife) do
     Chef::Log.logger = Logger.new(StringIO.new)
@@ -35,15 +44,11 @@ describe Chef::Knife::Bootstrap do
     k = Chef::Knife::Bootstrap.new(bootstrap_cli_options)
     allow(k.ui).to receive(:stderr).and_return(stderr)
     allow(k).to receive(:encryption_secret_provided_ignore_encrypt_flag?).and_return(false)
-    allow(k).to receive(:target_host).and_return target_host
+    allow(k).to receive(:connection).and_return connection
     k.merge_configs
     k
   end
 
-  before do
-    allow(target_host).to receive(:base_os).and_return base_os
-  end
-
   context "#bootstrap_template" do
     it "should default to chef-full" do
       expect(knife.bootstrap_template).to be_a_kind_of(String)
@@ -320,7 +325,7 @@ describe Chef::Knife::Bootstrap do
     subject(:knife) do
       k = described_class.new
       Chef::Config[:knife][:bootstrap_template] = template_file
-      allow(k).to receive(:target_host).and_return target_host
+      allow(k).to receive(:connection).and_return connection
       k.parse_options(options)
       k.merge_configs
       k
@@ -1578,7 +1583,7 @@ describe Chef::Knife::Bootstrap do
       expect(knife).to receive(:render_template).and_return "content"
       expect(knife).to receive(:upload_bootstrap).with("content").and_return "/remote/path.sh"
       expect(knife).to receive(:perform_bootstrap).with("/remote/path.sh")
-      expect(target_host).to receive(:del_file) # Make sure cleanup happens
+      expect(connection).to receive(:del_file!) # Make sure cleanup happens
 
       knife.run
 
@@ -1687,14 +1692,14 @@ describe Chef::Knife::Bootstrap do
     let(:result_mock) { double("result", exit_status: exit_status, stderr: "A message") }
 
     before do
-      allow(target_host).to receive(:hostname).and_return "testhost"
+      allow(connection).to receive(:hostname).and_return "testhost"
     end
     it "runs the remote script and logs the output" do
       expect(knife.ui).to receive(:info).with(/Bootstrapping.*/)
       expect(knife).to receive(:bootstrap_command)
         .with("/path.sh")
         .and_return("sh /path.sh")
-      expect(target_host)
+      expect(connection)
         .to receive(:run_command)
         .with("sh /path.sh")
         .and_yield("output here")
@@ -1710,7 +1715,7 @@ describe Chef::Knife::Bootstrap do
         expect(knife).to receive(:bootstrap_command)
           .with("/path.sh")
           .and_return("sh /path.sh")
-        expect(target_host).to receive(:run_command).with("sh /path.sh").and_return result_mock
+        expect(connection).to receive(:run_command).with("sh /path.sh").and_return result_mock
         expect { knife.perform_bootstrap("/path.sh") }.to raise_error(SystemExit)
       end
     end
@@ -1738,13 +1743,11 @@ describe Chef::Knife::Bootstrap do
 
     context "when an auth failure occurs" do
       let(:expected_error) do
-        # TODO This is awkward and ugly. Requires some refactor of chef_core/error
-        # to make it not so.  See comment in rescue block of connect! for details.
-        e = RuntimeError.new
-        interim = RuntimeError.new
+        e = Train::Error.new
         actual = Net::SSH::AuthenticationFailed.new
-        allow(interim).to receive(:cause).and_return(actual)
-        allow(e).to receive(:cause).and_return(interim)
+        # Simulate train's nested error - they wrap
+        # ssh/network errors in a TrainError.
+        allow(e).to receive(:cause).and_return(actual)
         e
       end
 
@@ -1754,7 +1757,7 @@ describe Chef::Knife::Bootstrap do
 
       context "and password auth was used" do
         before do
-          knife.config[:connection_password] = "tryme"
+          allow(connection).to receive(:password_auth?).and_return true
         end
 
         it "re-raises the error so as not to resubmit the same failing password" do
@@ -1765,8 +1768,8 @@ describe Chef::Knife::Bootstrap do
 
       context "and password auth was not used" do
         before do
-          knife.config.delete :connection_password
-          allow(target_host).to receive(:user).and_return "testuser"
+          allow(connection).to receive(:password_auth?).and_return false
+          allow(connection).to receive(:user).and_return "testuser"
         end
 
         it "warns, prompts for password, then reconnects with a password-enabled configuration using the new password" do
@@ -1793,7 +1796,7 @@ describe Chef::Knife::Bootstrap do
 
   describe "#bootstrap_context" do
     context "under Windows" do
-      let(:base_os) { :windows }
+      let(:windows_test) { true }
       it "creates a WindowsBootstrapContext" do
         require "chef/knife/core/windows_bootstrap_context"
         expect(knife.bootstrap_context.class).to eq Chef::Knife::Core::WindowsBootstrapContext
@@ -1801,7 +1804,7 @@ describe Chef::Knife::Bootstrap do
     end
 
     context "under linux" do
-      let(:base_os) { :linux }
+      let(:linux_test) { true }
       it "creates a BootstrapContext" do
         require "chef/knife/core/bootstrap_context"
         expect(knife.bootstrap_context.class).to eq Chef::Knife::Core::BootstrapContext
@@ -1841,25 +1844,25 @@ describe Chef::Knife::Bootstrap do
 
   describe "#upload_bootstrap" do
     before do
-      allow(target_host).to receive(:temp_dir).and_return(temp_dir)
-      allow(target_host).to receive(:normalize_path) { |a| a }
+      allow(connection).to receive(:temp_dir).and_return(temp_dir)
+      allow(connection).to receive(:normalize_path) { |a| a }
     end
 
     let(:content) { "bootstrap script content" }
     context "under Windows" do
-      let(:base_os) { :windows }
+      let(:windows_test) { true }
       let(:temp_dir) { "C:/Temp/bootstrap" }
-      it "creates a bat file in the temp dir provided by target_host, using given content" do
-        expect(target_host).to receive(:save_as_remote_file).with(content, "C:/Temp/bootstrap/bootstrap.bat")
+      it "creates a bat file in the temp dir provided by connection, using given content" do
+        expect(connection).to receive(:upload_file_content!).with(content, "C:/Temp/bootstrap/bootstrap.bat")
         expect(knife.upload_bootstrap(content)).to eq "C:/Temp/bootstrap/bootstrap.bat"
       end
     end
 
     context "under Linux" do
-      let(:base_os) { :linux }
+      let(:linux_test) { true }
       let(:temp_dir) { "/tmp/bootstrap" }
-      it "creates a 'sh file in the temp dir provided by target_host, using given content" do
-        expect(target_host).to receive(:save_as_remote_file).with(content, "/tmp/bootstrap/bootstrap.sh")
+      it "creates a 'sh file in the temp dir provided by connection, using given content" do
+        expect(connection).to receive(:upload_file_content!).with(content, "/tmp/bootstrap/bootstrap.sh")
         expect(knife.upload_bootstrap(content)).to eq "/tmp/bootstrap/bootstrap.sh"
       end
     end
@@ -1867,14 +1870,14 @@ describe Chef::Knife::Bootstrap do
 
   describe "#bootstrap_command" do
     context "under Windows" do
-      let(:base_os) { :windows }
+      let(:windows_test) { true }
       it "prefixes the command to run under cmd.exe" do
         expect(knife.bootstrap_command("autoexec.bat")).to eq "cmd.exe /C autoexec.bat"
       end
 
     end
     context "under Linux" do
-      let(:base_os) { :linux }
+      let(:linux_test) { true }
       it "prefixes the command to run under sh" do
         expect(knife.bootstrap_command("bootstrap")).to eq "sh bootstrap"
       end
@@ -1883,14 +1886,14 @@ describe Chef::Knife::Bootstrap do
 
   describe "#default_bootstrap_template" do
     context "under Windows" do
-      let(:base_os) { :windows }
+      let(:windows_test) { true }
       it "is windows-chef-client-msi" do
         expect(knife.default_bootstrap_template).to eq "windows-chef-client-msi"
       end
 
     end
     context "under Linux" do
-      let(:base_os) { :linux }
+      let(:linux_test) { true }
       it "is chef-full" do
         expect(knife.default_bootstrap_template).to eq "chef-full"
       end
@@ -1899,15 +1902,15 @@ describe Chef::Knife::Bootstrap do
 
   describe "#do_connect" do
     let(:host_descriptor) { "example.com" }
-    let(:target_host) { double("TargetHost") }
-    let(:resolver_mock) { double("TargetResolver", targets: [ target_host ]) }
+    let(:connection) { double("TrainConnector") }
+    let(:connector_mock) { double("TargetResolver", targets: [ connection ]) }
     before do
       allow(knife).to receive(:host_descriptor).and_return host_descriptor
     end
 
-    it "resolves the target and connects it" do
-      expect(ChefCore::TargetResolver).to receive(:new).and_return resolver_mock
-      expect(target_host).to receive(:connect!)
+    it "creates a TrainConnector and connects it" do
+      expect(Chef::Knife::Bootstrap::TrainConnector).to receive(:new).and_return connection
+      expect(connection).to receive(:connect!)
       knife.do_connect({})
     end
   end