From c2aa1e207f0126b47bad624fb943279f7acc4bc4 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 12 Nov 2018 15:31:27 -0800 Subject: [PATCH 1/7] Add a Communicator#reset! method --- lib/vagrant/plugin/v2/communicator.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/vagrant/plugin/v2/communicator.rb b/lib/vagrant/plugin/v2/communicator.rb index 41385f486..32b10a303 100644 --- a/lib/vagrant/plugin/v2/communicator.rb +++ b/lib/vagrant/plugin/v2/communicator.rb @@ -117,6 +117,13 @@ module Vagrant # @see #execute def test(command, opts=nil) end + + # Reset the communicator. For communicators which establish + # a persistent connection to the remote machine, this connection + # should be terminated and re-established. The communicator + # instance should be in a "fresh" state after calling this method. + def reset! + end end end end From cfc3e9e3981f0d37511641cfc5f107a20734c47a Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 12 Nov 2018 15:32:14 -0800 Subject: [PATCH 2/7] Reset communicator after updating user groups --- .../docker/cap/linux/docker_configure_vagrant_user.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/provisioners/docker/cap/linux/docker_configure_vagrant_user.rb b/plugins/provisioners/docker/cap/linux/docker_configure_vagrant_user.rb index 5283a9ef1..a85d8248f 100644 --- a/plugins/provisioners/docker/cap/linux/docker_configure_vagrant_user.rb +++ b/plugins/provisioners/docker/cap/linux/docker_configure_vagrant_user.rb @@ -9,6 +9,7 @@ module VagrantPlugins machine.communicate.tap do |comm| if comm.test("getent group docker") && !comm.test("id -Gn | grep docker") comm.sudo("usermod -a -G docker #{ssh_info[:username]}") + comm.reset! end end end From 747dd9301b2aaf89b411b1e398eada99344ad794 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 12 Nov 2018 15:33:19 -0800 Subject: [PATCH 3/7] Add reset! to ssh communicator. Reduce number of ssh info prints. --- plugins/communicators/ssh/communicator.rb | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/plugins/communicators/ssh/communicator.rb b/plugins/communicators/ssh/communicator.rb index deb241144..e9b0cce8f 100644 --- a/plugins/communicators/ssh/communicator.rb +++ b/plugins/communicators/ssh/communicator.rb @@ -69,11 +69,14 @@ module VagrantPlugins end # Got it! Let the user know what we're connecting to. - @machine.ui.detail("SSH address: #{ssh_info[:host]}:#{ssh_info[:port]}") - @machine.ui.detail("SSH username: #{ssh_info[:username]}") - ssh_auth_type = "private key" - ssh_auth_type = "password" if ssh_info[:password] - @machine.ui.detail("SSH auth method: #{ssh_auth_type}") + if !@ssh_info_notification + @machine.ui.detail("SSH address: #{ssh_info[:host]}:#{ssh_info[:port]}") + @machine.ui.detail("SSH username: #{ssh_info[:username]}") + ssh_auth_type = "private key" + ssh_auth_type = "password" if ssh_info[:password] + @machine.ui.detail("SSH auth method: #{ssh_auth_type}") + @ssh_info_notification = true + end previous_messages = {} while true @@ -309,6 +312,15 @@ module VagrantPlugins to: to.to_s end + def reset! + if @connection + @connection.close + @connection = nil + end + @ssh_info_notification = true # suppress ssh info output + wait_for_ready(5) + end + protected # Opens an SSH connection and yields it to a block. From afc138478d646a1cf1cf687bac66bb4f195e2065 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 12 Nov 2018 15:33:56 -0800 Subject: [PATCH 4/7] Add reset! method to winrm communicator --- plugins/communicators/winrm/communicator.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/plugins/communicators/winrm/communicator.rb b/plugins/communicators/winrm/communicator.rb index 14a742f5b..4c1fa8758 100644 --- a/plugins/communicators/winrm/communicator.rb +++ b/plugins/communicators/winrm/communicator.rb @@ -121,6 +121,10 @@ module VagrantPlugins return false end + def reset! + shell(true) + end + def shell(reload=false) @shell = nil if reload @shell ||= create_shell From 29880ccd1f45c1f3912a53ddb861e8175db8135d Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 12 Nov 2018 15:34:10 -0800 Subject: [PATCH 5/7] Add option to shell provisioner to reset communicator --- plugins/provisioners/shell/config.rb | 5 ++++- plugins/provisioners/shell/provisioner.rb | 6 ++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/plugins/provisioners/shell/config.rb b/plugins/provisioners/shell/config.rb index ae456f39e..b336b0c5c 100644 --- a/plugins/provisioners/shell/config.rb +++ b/plugins/provisioners/shell/config.rb @@ -17,6 +17,7 @@ module VagrantPlugins attr_accessor :sensitive attr_accessor :powershell_args attr_accessor :powershell_elevated_interactive + attr_accessor :reset def initialize @args = UNSET_VALUE @@ -31,6 +32,7 @@ module VagrantPlugins @keep_color = UNSET_VALUE @name = UNSET_VALUE @sensitive = UNSET_VALUE + @reset = UNSET_VALUE @powershell_args = UNSET_VALUE @powershell_elevated_interactive = UNSET_VALUE end @@ -48,6 +50,7 @@ module VagrantPlugins @keep_color = false if @keep_color == UNSET_VALUE @name = nil if @name == UNSET_VALUE @sensitive = false if @sensitive == UNSET_VALUE + @reset = false if @reset == UNSET_VALUE @powershell_args = "-ExecutionPolicy Bypass" if @powershell_args == UNSET_VALUE @powershell_elevated_interactive = false if @powershell_elevated_interactive == UNSET_VALUE @@ -68,7 +71,7 @@ module VagrantPlugins # Validate that the parameters are properly set if path && inline errors << I18n.t("vagrant.provisioners.shell.path_and_inline_set") - elsif !path && !inline + elsif !path && !inline && !reset errors << I18n.t("vagrant.provisioners.shell.no_path_or_inline") end diff --git a/plugins/provisioners/shell/provisioner.rb b/plugins/provisioners/shell/provisioner.rb index 3964c0a82..a77f9185e 100644 --- a/plugins/provisioners/shell/provisioner.rb +++ b/plugins/provisioners/shell/provisioner.rb @@ -18,6 +18,10 @@ module VagrantPlugins args = " #{args.join(" ")}" end + # In cases where the connection is just being reset + # bail out before attempting to do any actual provisioning + return if !config.path && !config.inline + case @machine.config.vm.communicator when :winrm provision_winrm(args) @@ -26,6 +30,8 @@ module VagrantPlugins else provision_ssh(args) end + ensure + @machine.communicate.reset! if config.reset end protected From 3ebe5b40e37267a137d6a81895a1ca2027ff5c2a Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 12 Nov 2018 15:34:26 -0800 Subject: [PATCH 6/7] Add test coverage on new functionality --- .../communicators/ssh/communicator_test.rb | 20 +++++++ .../communicators/winrm/communicator_test.rb | 7 +++ .../plugins/provisioners/shell/config_test.rb | 25 +++++++++ .../provisioners/shell/provisioner_test.rb | 54 ++++++++++++++++++- 4 files changed, 104 insertions(+), 2 deletions(-) diff --git a/test/unit/plugins/communicators/ssh/communicator_test.rb b/test/unit/plugins/communicators/ssh/communicator_test.rb index cfbcad89b..3b2cb9a32 100644 --- a/test/unit/plugins/communicators/ssh/communicator_test.rb +++ b/test/unit/plugins/communicators/ssh/communicator_test.rb @@ -162,6 +162,26 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do end end + describe "reset!" do + let(:connection) { double("connection") } + + before do + allow(communicator).to receive(:wait_for_ready) + allow(connection).to receive(:close) + communicator.send(:instance_variable_set, :@connection, connection) + end + + it "should close existing connection" do + expect(connection).to receive(:close) + communicator.reset! + end + + it "should call wait_for_ready to re-enable the connection" do + expect(communicator).to receive(:wait_for_ready) + communicator.reset! + end + end + describe ".ready?" do before(&connection_setup) it "returns true if shell test is successful" do diff --git a/test/unit/plugins/communicators/winrm/communicator_test.rb b/test/unit/plugins/communicators/winrm/communicator_test.rb index 268e7c48c..eaf3b3948 100644 --- a/test/unit/plugins/communicators/winrm/communicator_test.rb +++ b/test/unit/plugins/communicators/winrm/communicator_test.rb @@ -57,6 +57,13 @@ describe VagrantPlugins::CommunicatorWinRM::Communicator do end end + describe ".reset!" do + it "should create a new shell" do + expect(subject).to receive(:shell).with(true) + subject.reset! + end + end + describe ".ready?" do it "returns true if hostname command executes without error" do expect(shell).to receive(:cmd).with("hostname").and_return({ exitcode: 0 }) diff --git a/test/unit/plugins/provisioners/shell/config_test.rb b/test/unit/plugins/provisioners/shell/config_test.rb index f53af6c0e..e0d5a7c52 100644 --- a/test/unit/plugins/provisioners/shell/config_test.rb +++ b/test/unit/plugins/provisioners/shell/config_test.rb @@ -109,6 +109,31 @@ describe "VagrantPlugins::Shell::Config" do I18n.t("vagrant.provisioners.shell.env_must_be_a_hash") ) end + + it "returns an error if file and script are unset" do + subject.finalize! + result = subject.validate(machine) + expect(result["shell provisioner"]).to include( + I18n.t("vagrant.provisioners.shell.no_path_or_inline") + ) + end + + it "returns an error if inline and path are both set" do + subject.inline = "script" + subject.path = "script" + result = subject.validate(machine) + expect(result["shell provisioner"]).to include( + I18n.t("vagrant.provisioners.shell.path_and_inline_set") + ) + end + + it "returns no error when inline and path are unset but reset is true" do + subject.reset = true + subject.finalize! + + result = subject.validate(machine) + expect(result["shell provisioner"]).to be_empty + end end describe 'finalize!' do diff --git a/test/unit/plugins/provisioners/shell/provisioner_test.rb b/test/unit/plugins/provisioners/shell/provisioner_test.rb index 216abcde5..e71b3a3bf 100644 --- a/test/unit/plugins/provisioners/shell/provisioner_test.rb +++ b/test/unit/plugins/provisioners/shell/provisioner_test.rb @@ -16,6 +16,52 @@ describe "Vagrant::Shell::Provisioner" do allow(env).to receive(:tmp_path).and_return(Pathname.new("/dev/null")) end + context "when reset is enabled" do + let(:path) { nil } + let(:inline) { "" } + let(:communicator) { double("communicator") } + + let(:config) { + double( + :config, + :args => "doesn't matter", + :env => {}, + :upload_path => "arbitrary", + :remote? => false, + :path => path, + :inline => inline, + :binary => false, + :reset => true + ) + } + + let(:vsp) { + VagrantPlugins::Shell::Provisioner.new(machine, config) + } + + before { + allow(machine).to receive(:communicate).and_return(communicator) + allow(vsp).to receive(:provision_ssh) + } + + it "should provision and then reset the connection" do + expect(vsp).to receive(:provision_ssh) + expect(communicator).to receive(:reset!) + vsp.provision + end + + context "when path and inline are not set" do + let(:path) { nil } + let(:inline) { nil } + + it "should reset the connection and not provision" do + expect(vsp).not_to receive(:provision_ssh) + expect(communicator).to receive(:reset!) + vsp.provision + end + end + end + context "with a script that contains invalid us-ascii byte sequences" do let(:config) { double( @@ -27,6 +73,7 @@ describe "Vagrant::Shell::Provisioner" do :path => nil, :inline => script_that_is_incorrectly_us_ascii_encoded, :binary => false, + :reset => false ) } @@ -59,6 +106,7 @@ describe "Vagrant::Shell::Provisioner" do :path => nil, :inline => script, :binary => false, + :reset => false ) } @@ -87,7 +135,8 @@ describe "Vagrant::Shell::Provisioner" do :path => "http://example.com/script.sh", :binary => false, :md5 => nil, - :sha1 => 'EXPECTED_VALUE' + :sha1 => 'EXPECTED_VALUE', + :reset => false ) } @@ -117,7 +166,8 @@ describe "Vagrant::Shell::Provisioner" do :path => "http://example.com/script.sh", :binary => false, :md5 => 'EXPECTED_VALUE', - :sha1 => nil + :sha1 => nil, + :reset => false ) } From 6b02914956e1a50c57dbfab474f953eee2259c7b Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 12 Nov 2018 15:34:34 -0800 Subject: [PATCH 7/7] Update shell provisioner documentation Properly order options alphabetically and include new option for reset. --- .../source/docs/provisioning/shell.html.md | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/website/source/docs/provisioning/shell.html.md b/website/source/docs/provisioning/shell.html.md index c9dc5f2c9..bb5a23ea1 100644 --- a/website/source/docs/provisioning/shell.html.md +++ b/website/source/docs/provisioning/shell.html.md @@ -43,31 +43,22 @@ The remainder of the available options are optional: etc. as needed. You may also pass the arguments in using an array. In this case, Vagrant will handle quoting for you. -* `env` (hash) - List of key-value pairs to pass in as environment variables to - the script. Vagrant will handle quoting for environment variable values, but - the keys remain untouched. - * `binary` (boolean) - Vagrant automatically replaces Windows line endings with Unix line endings. If this is false, then Vagrant will not do this. By default this is "false". If the shell provisioner is communicating over WinRM, this defaults to "true". -* `privileged` (boolean) - Specifies whether to execute the shell script - as a privileged user or not (`sudo`). By default this is "true". Windows - guests use a scheduled task to run as a true administrator without the - WinRM limitations. - -* `upload_path` (string) - Is the remote path where the shell script will - be uploaded to. The script is uploaded as the SSH user over SCP, so this - location must be writable to that user. By default this is - "/tmp/vagrant-shell". On Windows, this will default to - "C:\tmp\vagrant-shell". +* `env` (hash) - List of key-value pairs to pass in as environment variables to + the script. Vagrant will handle quoting for environment variable values, but + the keys remain untouched. * `keep_color` (boolean) - Vagrant automatically colors output in green and red depending on whether the output is from stdout or stderr. If this is true, Vagrant will not do this, allowing the native colors from the script to be outputted. +* `md5` (string) - MD5 checksum used to validate remotely downloaded shell files. + * `name` (string) - This value will be displayed in the output so that identification by the user is easier when many shell provisioners are present. @@ -79,13 +70,25 @@ The remainder of the available options are optional: enable auto-login for Windows as the user must be logged in for interactive mode to work. -* `md5` (string) - MD5 checksum used to validate remotely downloaded shell files. +* `privileged` (boolean) - Specifies whether to execute the shell script + as a privileged user or not (`sudo`). By default this is "true". Windows + guests use a scheduled task to run as a true administrator without the + WinRM limitations. + +* `reset` (boolean) - Reset the communicator to the machine after completion. This + is useful when a shell may need to be reloaded. * `sha1` (string) - SHA1 checksum used to validate remotely downloaded shell files. * `sensitive` (boolean) - Marks the Hash values used in the `env` option as sensitive and hides them from output. By default this is "false". +* `upload_path` (string) - Is the remote path where the shell script will + be uploaded to. The script is uploaded as the SSH user over SCP, so this + location must be writable to that user. By default this is + "/tmp/vagrant-shell". On Windows, this will default to + "C:\tmp\vagrant-shell". + ## Inline Scripts