diff options
author | Bryan McLellan <btm@loftninjas.org> | 2019-05-13 18:00:07 -0400 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-05-13 18:00:07 -0400 |
commit | 0924dfcd96ab9281f3b2eef73b48475a8ed190eb (patch) | |
tree | ed5b338f5b6b16912571a26280d804a6699857f9 | |
parent | 3287fb0805ee519bf5bf3d52e268518faa3401d6 (diff) | |
parent | 218bd7f031e8820d2e0a7a7bc6fcc3c16df99c60 (diff) | |
download | chef-0924dfcd96ab9281f3b2eef73b48475a8ed190eb.tar.gz |
Merge pull request #8521 from MsysTechnologiesllc/VSingh/bootstrap-session-timeout
Chef 15: Add --session-timeout bootstrap option for both ssh & winrm
-rw-r--r-- | lib/chef/knife/bootstrap.rb | 35 | ||||
-rw-r--r-- | spec/unit/knife/bootstrap_spec.rb | 63 |
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 |