Merge pull request #8521 from MsysTechnologiesllc/VSingh/bootstrap-session-timeout

Chef 15: Add --session-timeout bootstrap option for both ssh & winrm

2 files changed, 85 insertions, 13 deletions

diff --git a/lib/chef/knife/bootstrap.rb b/lib/chef/knife/bootstrap.rb
index 04eabc3c83..477074fe49 100644
--- a/lib/chef/knife/bootstrap.rb
+++ b/lib/chef/knife/bootstrap.rb
@@ -58,6 +58,12 @@ class Chef
         long: "--max-wait SECONDS",
         description: "The maximum time to wait for the initial connection to be established."
 
+      option :session_timeout,
+        long: "--session-timeout SECONDS",
+        description: "The number of seconds to wait for each connection operation to be acknowledged while running bootstrap.",
+        proc: Proc.new { |protocol| Chef::Config[:knife][:session_timeout] = protocol },
+        default: 60
+
       # WinRM Authentication
       option :winrm_ssl_peer_fingerprint,
         long: "--winrm-ssl-peer-fingerprint FINGERPRINT",
@@ -109,11 +115,6 @@ class Chef
         description: "The Kerberos service used for authentication.",
         proc: Proc.new { |protocol| Chef::Config[:knife][:kerberos_service] = protocol }
 
-      option :winrm_session_timeout,
-        long: "--winrm-session-timeout SECONDS",
-        description: "The number of seconds to wait for each WinRM operation to be acknowledged while running bootstrap.",
-        proc: Proc.new { |protocol| Chef::Config[:knife][:winrm_session_timeout] = protocol }
-
       ## SSH Authentication
       option :ssh_gateway,
         short: "-G GATEWAY",
@@ -373,6 +374,8 @@ class Chef
           [:connection_port, "--winrm-port"],
         winrm_authentication_protocol:
           [:winrm_auth_method, "--winrm-authentication-protocol PROTOCOL"],
+        winrm_session_timeout:
+          [:session_timeout, "--winrm-session-timeout MINUTES"],
       }.freeze
 
       DEPRECATED_FLAGS.each do |deprecated_key, deprecation_entry|
@@ -538,6 +541,7 @@ class Chef
         validate_policy_options!
 
         winrm_warn_no_ssl_verification
+        warn_on_short_session_timeout
 
         $stdout.sync = true
         register_client
@@ -760,6 +764,24 @@ class Chef
         true
       end
 
+      # If session_timeout is too short, it is likely
+      # a holdover from "--winrm-session-timeout" which used
+      # minutes as its unit, instead of seconds.
+      # Warn the human so that they are not surprised.
+      #
+      # This will also erroneously warn if a string value is given,
+      # but argument type validation is something that needs addressing
+      # more broadly.
+      def warn_on_short_session_timeout
+        timeout = config_value(:session_timeout).to_i
+        if timeout <= 15
+          ui.warn <<~EOM
+            --session-timeout is set to #{config[:session_timeout]} minutes.
+            Did you mean "--session-timeout #{config[:session_timeout] * 60}" seconds?
+          EOM
+        end
+      end
+
       def winrm_warn_no_ssl_verification
         return unless winrm?
 
@@ -846,6 +868,7 @@ class Chef
         return opts if winrm?
         opts[:non_interactive] = true # Prevent password prompts from underlying net/ssh
         opts[:forward_agent] = (config_value(:ssh_forward_agent) === true)
+        opts[:connection_timeout] = config_value(:session_timeout).to_i
         opts
       end
 
@@ -944,7 +967,7 @@ class Chef
           opts[:ca_trust_file] = config_value(:ca_trust_file)
         end
 
-        opts[:operation_timeout] = config_value(:winrm_session_timeout) || 60
+        opts[:operation_timeout] = config_value(:session_timeout).to_i
 
         opts
       end
diff --git a/spec/unit/knife/bootstrap_spec.rb b/spec/unit/knife/bootstrap_spec.rb
index c45604b7d1..4bfd1011fa 100644
--- a/spec/unit/knife/bootstrap_spec.rb
+++ b/spec/unit/knife/bootstrap_spec.rb
@@ -831,7 +831,7 @@ describe Chef::Knife::Bootstrap do
             Chef::Config[:knife][:winrm_auth_method] = "kerberos" # default is negotiate
             Chef::Config[:knife][:winrm_basic_auth_only] = true
             Chef::Config[:knife][:winrm_no_verify_cert] = true
-            Chef::Config[:knife][:winrm_session_timeout] = 9999
+            Chef::Config[:knife][:session_timeout] = 9999
             Chef::Config[:knife][:winrm_ssl] = true
             Chef::Config[:knife][:winrm_ssl_peer_fingerprint] = "ABCDEF"
           end
@@ -880,7 +880,7 @@ describe Chef::Knife::Bootstrap do
                 logger: Chef::Log, # not configurable
                 ca_trust_file: "no trust",
                 max_wait_until_ready: 9999,
-                operation_timeout: 9999,
+                operation_timeout: 60,
                 ssl_peer_fingerprint: "ABCDEF",
                 winrm_transport: "kerberos",
                 winrm_basic_auth_only: true,
@@ -926,7 +926,7 @@ describe Chef::Knife::Bootstrap do
               knife.config[:winrm_auth_method] = "kerberos" # default is negotiate
               knife.config[:winrm_basic_auth_only] = false
               knife.config[:winrm_no_verify_cert] = false
-              knife.config[:winrm_session_timeout] = 1000
+              knife.config[:session_timeout] = 1000
               knife.config[:winrm_ssl] = false
               knife.config[:winrm_ssl_peer_fingerprint] = "FEDCBA"
             end
@@ -956,7 +956,9 @@ describe Chef::Knife::Bootstrap do
 
         context "and no values are provided from Chef::Config or CLI" do
           before do
-            knife.config = {}
+            # We will use knife's actual config since these tests
+            # have assumptions based on CLI default values
+            knife.merge_configs
           end
           let(:expected_result) do
             {
@@ -984,6 +986,7 @@ describe Chef::Knife::Bootstrap do
             # Set everything to easily identifiable and obviously fake values
             # to verify that Chef::Config is being sourced instead of knife.config
             Chef::Config[:knife][:max_wait] = 9999
+            Chef::Config[:knife][:session_timeout] = 9999
             Chef::Config[:knife][:ssh_user] = "sshbob"
             Chef::Config[:knife][:ssh_port] = 9999
             Chef::Config[:knife][:host_key_verify] = false
@@ -1001,6 +1004,7 @@ describe Chef::Knife::Bootstrap do
               {
                 logger: Chef::Log, # not configurable
                 max_wait_until_ready: 9999,
+                connection_timeout: 9999,
                 user: "sshbob",
                 bastion_host: "mygateway.local",
                 bastion_port: 1234,
@@ -1043,6 +1047,7 @@ describe Chef::Knife::Bootstrap do
               knife.config[:ssh_port] = "13" # canary to indirectly verify we're not looking for the wrong CLI flag
               knife.config[:connection_password] = "feta cheese"
               knife.config[:max_wait] = 150
+              knife.config[:session_timeout] = 120
               knife.config[:use_sudo] = true
               knife.config[:use_sudo_pasword] = true
               knife.config[:ssh_forward_agent] = true
@@ -1052,6 +1057,7 @@ describe Chef::Knife::Bootstrap do
               {
                 logger: Chef::Log, # not configurable
                 max_wait_until_ready: 150, # cli
+                connection_timeout: 120, # cli
                 user: "sshalice", # cli
                 password: "feta cheese", # cli
                 bastion_host: "mygateway.local", # Config
@@ -1075,6 +1081,7 @@ describe Chef::Knife::Bootstrap do
           context "and all CLI options have been given" do
             before do
               knife.config[:max_wait] = 150
+              knife.config[:session_timeout] = 120
               knife.config[:connection_user] = "sshroot"
               knife.config[:connection_port] = 1000
               knife.config[:connection_password] = "blah"
@@ -1099,6 +1106,7 @@ describe Chef::Knife::Bootstrap do
               {
                 logger: Chef::Log, # not configurable
                 max_wait_until_ready: 150,
+                connection_timeout: 120,
                 user: "sshroot",
                 password: "blah",
                 port: 1000,
@@ -1122,8 +1130,11 @@ describe Chef::Knife::Bootstrap do
         end
         context "and no values are provided from Chef::Config or CLI" do
           before do
-            knife.config = {}
+            # We will use knife's actual config since these tests
+            # have assumptions based on CLI default values
+            knife.merge_configs
           end
+
           let(:expected_result) do
             {
               forward_agent: false,
@@ -1133,6 +1144,7 @@ describe Chef::Knife::Bootstrap do
               sudo: false,
               verify_host_key: "always",
               non_interactive: true,
+              connection_timeout: 60,
             }
           end
           it "populates appropriate defaults" do
@@ -1485,12 +1497,26 @@ describe Chef::Knife::Bootstrap do
           knife.config[:ssh_forward_agent] = true
         end
         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 })
+          expect(knife.ssh_opts).to eq({ forward_agent: true, non_interactive: true, connection_timeout: 60 })
         end
       end
       context "when ssh_forward_agent is not set" do
         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 })
+          expect(knife.ssh_opts).to eq({ forward_agent: false, non_interactive: true, connection_timeout: 60 })
+        end
+      end
+      context "when session_timeout has a value" do
+        before do
+          knife.config[:session_timeout] = 120
+        end
+        it "returns a configuration hash with connection_timeout value." do
+          expect(knife.ssh_opts).to eq({ forward_agent: false, non_interactive: true, connection_timeout: 120 })
+        end
+      end
+
+      context "when session_timeout is not set" do
+        it "returns a configuration hash with connection_timeout default value." do
+          expect(knife.ssh_opts).to eq({ forward_agent: false, non_interactive: true, connection_timeout: 60 })
         end
       end
     end
@@ -1633,6 +1659,7 @@ describe Chef::Knife::Bootstrap do
       expect(knife).to receive(:validate_winrm_transport_opts!).ordered
       expect(knife).to receive(:validate_policy_options!).ordered
       expect(knife).to receive(:winrm_warn_no_ssl_verification).ordered
+      expect(knife).to receive(:warn_on_short_session_timeout).ordered
       expect(knife).to receive(:register_client).ordered
       expect(knife).to receive(:connect!).ordered
       expect(knife).to receive(:render_template).and_return "content"
@@ -2107,4 +2134,26 @@ describe Chef::Knife::Bootstrap do
       end
     end
   end
+
+  describe "#warn_on_short_session_timeout" do
+    let(:session_timeout) { 0 }
+    before do
+      allow(knife).to receive(:config).and_return(session_timeout: session_timeout)
+    end
+
+    context "timeout is more than 15" do
+      let(:session_timeout) { 16 }
+      it "does not issue a warning" do
+        expect(knife.ui).to_not receive(:warn)
+        knife.warn_on_short_session_timeout
+      end
+    end
+    context "timeout is 15 or less" do
+      let(:session_timeout) { 15 }
+      it "issues a warning" do
+        expect(knife.ui).to receive(:warn)
+        knife.warn_on_short_session_timeout
+      end
+    end
+  end
 end