Ensure that requested protocol is available and correct

There are a few related changes here:
* ensure that we always have the requested protocol, and
  use the default when we can't otherwise resolve it. This corrects
  the first part of CHEF-8432
* This also finishes the work started in a previous branch
  that moves the config handling out of TrainConnector::initialize
  and into a lazy loader for config.
* Ensure that the baseline configuration contains only things
  that will not be picked up when merging user-provided config. This
  meant moving :non_interactive out to the caller (bootstrap).
* Removed helpers that weren't helping much.

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

2 files changed, 104 insertions, 51 deletions

diff --git a/lib/chef/knife/bootstrap/train_connector.rb b/lib/chef/knife/bootstrap/train_connector.rb
index 429874dc07..92fecbac43 100644
--- a/lib/chef/knife/bootstrap/train_connector.rb
+++ b/lib/chef/knife/bootstrap/train_connector.rb
@@ -35,13 +35,27 @@ 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
+
+        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
 
         def connect!
@@ -51,11 +65,11 @@ class Chef
         end
 
         def hostname
-          @config[:host]
+          config[:host]
         end
 
         def password_auth?
-          @config.key? :password
+          config.key? :password
         end
 
         # True if we're connected to a linux host
@@ -76,14 +90,6 @@ class Chef
           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.
         #
@@ -98,7 +104,7 @@ 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
@@ -143,37 +149,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)
@@ -182,7 +179,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]
@@ -212,14 +209,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)
@@ -231,7 +228,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 c976a805ee..385a192648 100644
--- a/spec/unit/knife/bootstrap/train_connector_spec.rb
+++ b/spec/unit/knife/bootstrap/train_connector_spec.rb
@@ -20,28 +20,33 @@ require "ostruct"
 require "chef/knife/bootstrap/train_connector"
 
 describe Chef::Knife::Bootstrap::TrainConnector do
-  let(:transport) { "mock" }
+  let(:protocol) { "mock" }
   let(:family) { "unknown" }
   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
-    # Specifying sudo: false ensures that attempted operations
-    # don't fail because the mock platform doesn't support sudo.
-    # Example groups can still override by setting explicitly it in 'opts'
-    tc = Chef::Knife::Bootstrap::TrainConnector.new(host_url, transport, { sudo: false }.merge(opts))
-    tc.connect!
-    tc.connection.mock_os(
-      family: family,
-      name: name,
-      release: release,
-      arch: arch
-    )
+    # 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
 
+  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
     context "on linux" do
       let(:family) { "debian" }
@@ -79,8 +84,59 @@ describe Chef::Knife::Bootstrap::TrainConnector do
     end
   end
 
-  describe "::new" do
+  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