Add a warning when session-timeout is too short

We converted --winrm-session-timeout MINUTES
to --session-timeout SECONDS

If someone is providing a short session timeout, it's possible
that they're doing so based on the flag accepting minutes.  We'll warn
them and suggest an alternative so that if things go wrong,
they'll know where to start looking.

We can remove this once we remove the deprecation for
winrm-session-timeout

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

2 files changed, 44 insertions, 2 deletions

diff --git a/lib/chef/knife/bootstrap.rb b/lib/chef/knife/bootstrap.rb
index 3f8f3642b5..b0f793e5c6 100644
--- a/lib/chef/knife/bootstrap.rb
+++ b/lib/chef/knife/bootstrap.rb
@@ -549,6 +549,7 @@ class Chef
         validate_policy_options!
 
         winrm_warn_no_ssl_verification
+        warn_on_short_session_timeout
 
         $stdout.sync = true
         register_client
@@ -752,6 +753,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?
 
diff --git a/spec/unit/knife/bootstrap_spec.rb b/spec/unit/knife/bootstrap_spec.rb
index 99fa861a07..5b5d6bd1a9 100644
--- a/spec/unit/knife/bootstrap_spec.rb
+++ b/spec/unit/knife/bootstrap_spec.rb
@@ -958,7 +958,7 @@ describe Chef::Knife::Bootstrap do
           before do
             # We will use knife's actual config since these tests
             # have assumptions based on CLI default values
-	    knife.merge_configs 
+            knife.merge_configs
           end
           let(:expected_result) do
             {
@@ -1132,7 +1132,7 @@ describe Chef::Knife::Bootstrap do
           before do
             # We will use knife's actual config since these tests
             # have assumptions based on CLI default values
-	    knife.merge_configs 
+            knife.merge_configs
           end
 
           let(:expected_result) do
@@ -1659,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"
@@ -2133,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