Merge pull request #8440 from chef/CHEF-8432/fix-default-protocol

[CHEF-8432] Ensure default protocol is used properly. Use correct 'require' before accessing Net::SSH constants.

4 files changed, 186 insertions, 80 deletions

diff --git a/lib/chef/knife/bootstrap.rb b/lib/chef/knife/bootstrap.rb
index 43f6e9a023..1d1946044f 100644
--- a/lib/chef/knife/bootstrap.rb
+++ b/lib/chef/knife/bootstrap.rb
@@ -563,6 +563,7 @@ class Chef
         opts = connection_opts.dup
         do_connect(opts)
       rescue Train::Error => e
+        require "net/ssh"
         if e.cause && e.cause.class == Net::SSH::AuthenticationFailed
           if connection.password_auth?
             raise
@@ -579,7 +580,6 @@ 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
@@ -796,6 +796,7 @@ class Chef
       def ssh_opts
         opts = {}
         return opts if connection_protocol == "winrm"
+        opts[:non_interactive] = true # Prevent password prompts from underlying net/ssh
         opts[:forward_agent] = (config_value(:ssh_forward_agent) === true)
         opts
       end
diff --git a/lib/chef/knife/bootstrap/train_connector.rb b/lib/chef/knife/bootstrap/train_connector.rb
index 5230d6638c..df94290dbf 100644
--- a/lib/chef/knife/bootstrap/train_connector.rb
+++ b/lib/chef/knife/bootstrap/train_connector.rb
@@ -35,80 +35,85 @@ class Chef
 
         MKTEMP_NIX_COMMAND = "bash -c 'd=$(mktemp -d ${TMPDIR:-/tmp}/chef_XXXXXX); echo $d'".freeze
 
-        def initialize(host_url, default_transport, opts)
-          uri_opts = opts_from_uri(host_url)
-          uri_opts[:backend] ||= @default_transport
-          @transport_type = uri_opts[:backend]
+        def initialize(host_url, default_protocol, opts)
+          @host_url = host_url
+          @default_protocol = default_protocol
+          @opts_in = opts
+        end
 
-          # opts in the URI will override user-provided options
-          @config = transport_config(host_url, opts.merge(uri_opts))
+        def config
+          @config ||= begin
+                        uri_opts = opts_from_uri(@host_url, @default_protocol)
+                        transport_config(@host_url, @opts_in.merge(uri_opts))
+                      end
         end
 
-        # Because creating a valid train connection for testing is a two-step process in which
-        # we need to connect before mocking config,
-        # we expose test_instance as a way for tests to create actual instances
-        # but ensure that they don't connect to any back end.
-        def self.test_instance(url, protocol: "ssh",
-                               family: "unknown", name: "unknown",
-                               release: "unknown", arch: "x86_64",
-                               opts: {})
-          # Specifying sudo: false ensures that attempted operations
-          # don't fail because the mock platform doesn't support sudo
-          tc = TrainConnector.new(url, protocol, { sudo: false }.merge(opts))
-          tc.connect!
-          tc.connection.mock_os(
-            family: family,
-            name: name,
-            release: release,
-            arch: arch
-          )
-          tc
+        def connection
+          @connection ||= begin
+                       Train.validate_backend(config)
+                       train = Train.create(config[:backend], config)
+                       # Note that the train connection is not currently connected
+                       # to the remote host, but it's ready to go.
+                       train.connection
+                     end
         end
 
+        #
+        # Establish a connection to the configured host.
+        #
+        # @raise [TrainError]
+        # @raise [TrainUserError]
+        #
+        # @return [TrueClass] true if the connection could be established.
         def connect!
           # Force connection to establish
           connection.wait_until_ready
           true
         end
 
+        #
+        # @return [String] the configured hostname
         def hostname
-          @config[:host]
+          config[:host]
         end
 
+        # Answers the question, "is this connection configured for password auth?"
+        # @return [Boolean] true if the connection is configured with password auth
         def password_auth?
-          @config.key? :password
+          config.key? :password
         end
 
-        # True if we're connected to a linux host
+        # Answers the question, "Am I connected to a linux host?"
+        #
+        # @return [Boolean] true if the connected host is linux.
         def linux?
           connection.platform.linux?
         end
 
-        # True if we're connected to a unix host.
-        # NOTE: this is always true
-        # for a linux host because train classifies
-        # linux as a unix
+        # Answers the question, "Am I connected to a unix host?"
+        #
+        # @note this will alwys return true for a linux host
+        # because train classifies linux as a unix
+        #
+        # @return [Boolean] true if the connected host is unix or linux
         def unix?
           connection.platform.unix?
         end
 
-        # True if we're connected to a windows host
+        #
+        # Answers the question, "Am I connected to a Windows host?"
+        #
+        # @return [Boolean] true if the connected host is Windows
         def windows?
           connection.platform.windows?
         end
 
-        def winrm?
-          @transport_type == "winrm"
-        end
-
-        def ssh?
-          @transport_type == "ssh"
-        end
-
+        #
         # Creates a temporary directory on the remote host if it
-        # hasn't already. Caches directory location.
+        # hasn't already. Caches directory location. For *nix,
+        # it will ensure that the directory is owned by the logged-in user
         #
-        # Returns the path on the remote host.
+        # @return [String] the temporary path created on the remote host.
         def temp_dir
           cmd = windows? ? MKTEMP_WIN_COMMAND : MKTEMP_NIX_COMMAND
           @tmpdir ||= begin
@@ -119,43 +124,87 @@ class Chef
                           # running with sudo right now - so this directory would be owned by root.
                           # File upload is performed over SCP as the current logged-in user,
                           # so we'll set ownership to ensure that works.
-                          run_command!("chown #{@config[:user]} '#{dir}'")
+                          run_command!("chown #{config[:user]} '#{dir}'")
                         end
                         dir
                       end
         end
 
+        #
+        # Uploads a file from "local_path" to "remote_path"
+        #
+        # @param local_path [String] The path to a file on the local file system
+        # @param remote_path [String] The destination path on the remote file system.
+        # @return NilClass
         def upload_file!(local_path, remote_path)
           connection.upload(local_path, remote_path)
+          nil
         end
 
+        #
+        # Uploads the provided content into the file "remote_path" on the remote host.
+        #
+        # @param content [String] The content to upload into remote_path
+        # @param remote_path [String] The destination path on the remote file system.
+        # @return NilClass
         def upload_file_content!(content, remote_path)
           t = Tempfile.new("chef-content")
           t << content
           t.close
           upload_file!(t.path, remote_path)
+          nil
         ensure
           t.close
           t.unlink
         end
 
+        #
+        # Force-deletes the file at "path" from the remote host.
+        #
+        # @param path [String] The path of the file on the remote host
         def del_file!(path)
           if windows?
             run_command!("If (Test-Path \"#{path}\") { Remove-Item -Force -Path \"#{path}\" }")
           else
             run_command!("rm -f \"#{path}\"")
           end
+          nil
         end
 
-        # normalizes path across OS's
+        #
+        # normalizes path across OS's - always use forward slashes, which
+        # Windows and *nix understand.
+        #
+        # @param path [String] The path to normalize
+        #
+        # @return [String] the normalized path
         def normalize_path(path)
           path.tr("\\", "/")
         end
 
+        #
+        # Runs a command on the remote host.
+        #
+        # @param command [String] The command to run.
+        # @param data_handler [Proc] An optional block. When provided, inbound data will be
+        # published via `data_handler.call(data)`. This can allow
+        # callers to receive and render updates from remote command execution.
+        #
+        # @return [Train::Extras::CommandResult] an object containing stdout, stderr, and exit_status
         def run_command(command, &data_handler)
           connection.run_command(command, &data_handler)
         end
 
+        #
+        # Runs a command the remote host
+        #
+        # @param command [String] The command to run.
+        # @param data_handler [Proc] An optional block. When provided, inbound data will be
+        # published via `data_handler.call(data)`. This can allow
+        # callers to receive and render updates from remote command execution.
+        #
+        # @raise Chef::Knife::Bootstrap::RemoteExecutionFailed if an error occurs (non-zero exit status)
+        # @return [Train::Extras::CommandResult] an object containing stdout, stderr, and exit_status
         def run_command!(command, &data_handler)
           result = run_command(command, &data_handler)
           if result.exit_status != 0
@@ -164,37 +213,28 @@ class Chef
           result
         end
 
-        def connection
-          @connection ||= begin
-                       Train.validate_backend(@config)
-                       train = Train.create(@transport_type, @config)
-                       train.connection
-                     end
-        end
-
         private
 
         # For a given url and set of options, create a config
         # hash suitable for passing into train.
         def transport_config(host_url, opts_in)
+          # These baseline opts are not protocol-specific
           opts = { target: host_url,
-                   sudo: opts_in[:sudo] === false ? false : true,
                    www_form_encoded_password: true,
-                   key_files: opts_in[:key_files],
-                   non_interactive: true, # Prevent password prompts
                    transport_retries: 2,
                    transport_retry_sleep: 1,
-                   logger: opts_in[:logger],
-                   backend: @transport_type }
+                   backend: opts_in[:backend],
+                   logger: opts_in[:logger] }
 
-          # Base opts are those provided by the caller directly
+          # Accepts options provided by caller if they're not already configured,
+          # but note that they will be constrained to valid options for the backend protocol
           opts.merge!(opts_from_caller(opts, opts_in))
 
           # WinRM has some additional computed options
           opts.merge!(opts_inferred_from_winrm(opts, opts_in))
 
-          # Now that everything is populated, fill in anything left
-          # from user ssh config that may be present
+          # Now that everything is populated, fill in anything missing
+          # that may be found in user ssh config
           opts.merge!(missing_opts_from_ssh_config(opts, opts_in))
 
           Train.target_config(opts)
@@ -203,7 +243,7 @@ class Chef
         # Some winrm options are inferred based on other options.
         # Return a hash of winrm options based on configuration already built.
         def opts_inferred_from_winrm(config, opts_in)
-          return {} unless winrm?
+          return {} unless config[:backend] == "winrm"
           opts_out = {}
 
           if opts_in[:ssl]
@@ -233,14 +273,14 @@ class Chef
 
         # Extract any of username/password/host/port/transport
         # that are in the URI and return them as a config has
-        def opts_from_uri(uri)
+        def opts_from_uri(uri, default_protocol)
           # Train.unpack_target_from_uri only works for complete URIs in
           # form of proto://[user[:pass]@]host[:port]/
           # So we'll add the protocol prefix if it's not supplied.
           uri_to_check = if URI.regexp.match(uri)
                            uri
                          else
-                           "#{@transport_type}://#{uri}"
+                           "#{default_protocol}://#{uri}"
                          end
 
           Train.unpack_target_from_uri(uri_to_check)
@@ -252,7 +292,7 @@ class Chef
         # This is necessary because train will default these values
         # itself - causing SSH config data to be ignored
         def missing_opts_from_ssh_config(config, opts_in)
-          return {} unless ssh?
+          return {} unless config[:backend] == "ssh"
           host_cfg = ssh_config_for_host(config[:host])
           opts_out = {}
           opts_in.each do |key, _value|
diff --git a/spec/unit/knife/bootstrap/train_connector_spec.rb b/spec/unit/knife/bootstrap/train_connector_spec.rb
index 08bf21dd42..385a192648 100644
--- a/spec/unit/knife/bootstrap/train_connector_spec.rb
+++ b/spec/unit/knife/bootstrap/train_connector_spec.rb
@@ -25,20 +25,26 @@ describe Chef::Knife::Bootstrap::TrainConnector do
   let(:release) { "unknown" } # version
   let(:name) { "unknown" }
   let(:arch) { "x86_64" }
+  let(:connection_opts) { {} } # connection opts
   let(:host_url) { "mock://user1@example.com" }
-  let(:opts) { {} }
+  let(:mock_connection) { true }
+
   subject do
-    # Create a valid TargetHost with the backend stubbed out.
-    Chef::Knife::Bootstrap::TrainConnector.test_instance(host_url,
-                                                         protocol: protocol,
-                                                         family: family,
-                                                         name: name,
-                                                         release: release,
-                                                         arch: arch,
-                                                         opts: opts)
+    # Example groups can still override by setting explicitly it in 'connection_opts'
+    tc = Chef::Knife::Bootstrap::TrainConnector.new(host_url, protocol, connection_opts)
+    tc
   end
 
-  context "connect!" do
+  before(:each) do
+    if mock_connection
+      subject.connect!
+      subject.connection.mock_os(
+        family: family,
+        name: name,
+        release: release,
+        arch: arch
+      )
+    end
   end
 
   describe "platform helpers" do
@@ -78,6 +84,61 @@ describe Chef::Knife::Bootstrap::TrainConnector do
     end
   end
 
+  describe "#initialize" do
+    let(:mock_connection) { false }
+
+    context "when provided target is a proper URL" do
+      let(:protocol) { "ssh" }
+      let(:host_url) { "mock://user1@localhost:2200" }
+      it "correctly configures the instance from the URL" do
+        expect(subject.config[:backend]).to eq "mock"
+        expect(subject.config[:port]).to eq 2200
+        expect(subject.config[:host]).to eq "localhost"
+        expect(subject.config[:user]).to eq "user1"
+      end
+
+      context "and conflicting options are given" do
+        let(:connection_opts) { { user: "user2", host: "example.com", port: 15 } }
+        it "resolves them from the URI" do
+          expect(subject.config[:backend]).to eq "mock"
+          expect(subject.config[:port]).to eq 2200
+          expect(subject.config[:host]).to eq "localhost"
+          expect(subject.config[:user]).to eq "user1"
+        end
+      end
+    end
+
+    context "when provided target is just a hostname" do
+      let(:host_url) { "localhost" }
+      let(:protocol) { "mock" }
+      it "correctly sets backend protocol from the default" do
+        expect(subject.config[:backend]).to eq "mock"
+      end
+
+      context "and options have been provided that are supported by the transport" do
+        let(:protocol) { "ssh" }
+        let(:connection_opts) { { port: 15, user: "user2" } }
+
+        it "sets hostname and transport from arguments and provided fields from options" do
+          expect(subject.config[:backend]).to eq "ssh"
+          expect(subject.config[:host]).to eq "localhost"
+          expect(subject.config[:user]).to eq "user2"
+          expect(subject.config[:port]).to eq 15
+        end
+
+      end
+
+    end
+
+    context "when provided target is just a an IP address" do
+      let(:host_url) { "127.0.0.1" }
+      let(:protocol) { "mock" }
+      it "correctly sets backend protocol from the default" do
+        expect(subject.config[:backend]).to eq "mock"
+      end
+    end
+  end
+
   describe "#temp_dir" do
     context "under windows" do
       let(:family) { "windows" }
diff --git a/spec/unit/knife/bootstrap_spec.rb b/spec/unit/knife/bootstrap_spec.rb
index 5bef9c5659..995a2ef4c9 100644
--- a/spec/unit/knife/bootstrap_spec.rb
+++ b/spec/unit/knife/bootstrap_spec.rb
@@ -961,6 +961,7 @@ describe Chef::Knife::Bootstrap do
                 sudo: false,
                 verify_host_key: false,
                 port: 9999,
+                non_interactive: true,
               }
             end
 
@@ -1012,6 +1013,7 @@ describe Chef::Knife::Bootstrap do
                 sudo: true, # ccli
                 verify_host_key: false, # Config
                 port: 12, # cli
+                non_interactive: true,
               }
             end
 
@@ -1060,6 +1062,7 @@ describe Chef::Knife::Bootstrap do
                 sudo_options: "-H",
                 sudo_password: "blah",
                 verify_host_key: true,
+                non_interactive: true,
               }
             end
             it "generates a config hash using the CLI options and pulling nothing from Chef::Config" do
@@ -1079,6 +1082,7 @@ describe Chef::Knife::Bootstrap do
               keys_only: false,
               sudo: false,
               verify_host_key: true,
+              non_interactive: true,
             }
           end
           it "populates appropriate defaults" do
@@ -1430,13 +1434,13 @@ describe Chef::Knife::Bootstrap do
         before do
           knife.config[:ssh_forward_agent] = true
         end
-        it "returns a configuration hash with forward_agent set to true" do
-          expect(knife.ssh_opts).to eq({ forward_agent: true })
+        it "returns a configuration hash with forward_agent set to true. non-interactive is always true" do
+          expect(knife.ssh_opts).to eq({ forward_agent: true, non_interactive: true })
         end
       end
       context "when ssh_forward_agent is not set" do
-        it "returns a configuration hash with forward_agent set to false" do
-          expect(knife.ssh_opts).to eq({ forward_agent: false })
+        it "returns a configuration hash with forward_agent set to false. non-interactive is always true" do
+          expect(knife.ssh_opts).to eq({ forward_agent: false, non_interactive: true })
         end
       end
     end