From d1a778dbfb136fe5603e02915aae06f33583ed61 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Sun, 30 Oct 2016 06:33:57 -0700 Subject: [PATCH] Include error handling when subprocess commands fail --- lib/vagrant/errors.rb | 4 ++ plugins/hosts/linux/cap/nfs.rb | 52 +++++++++++++++---- templates/locales/en.yml | 8 +++ test/unit/plugins/hosts/linux/cap/nfs_test.rb | 20 +++++-- 4 files changed, 70 insertions(+), 14 deletions(-) diff --git a/lib/vagrant/errors.rb b/lib/vagrant/errors.rb index fd289eef6..9bff60a47 100644 --- a/lib/vagrant/errors.rb +++ b/lib/vagrant/errors.rb @@ -456,6 +456,10 @@ module Vagrant error_key(:nfs_bad_exports) end + class NFSExportsFailed < VagrantError + error_key(:nfs_exports_failed) + end + class NFSCantReadExports < VagrantError error_key(:nfs_cant_read_exports) end diff --git a/plugins/hosts/linux/cap/nfs.rb b/plugins/hosts/linux/cap/nfs.rb index 7d23926a3..731399b8e 100644 --- a/plugins/hosts/linux/cap/nfs.rb +++ b/plugins/hosts/linux/cap/nfs.rb @@ -39,7 +39,7 @@ module VagrantPlugins sleep 0.5 nfs_cleanup("#{Process.uid} #{id}") - output = "#{File.read(NFS_EXPORTS_PATH)}\n#{output}" + output = "#{nfs_exports_content}\n#{output}" nfs_write_exports(output) if nfs_running?(nfs_check_command) @@ -65,7 +65,7 @@ module VagrantPlugins user = Process.uid # Create editor instance for removing invalid IDs - editor = Vagrant::Util::StringBlockEditor.new(File.read(NFS_EXPORTS_PATH)) + editor = Vagrant::Util::StringBlockEditor.new(nfs_exports_content) # Build composite IDs with UID information and discover invalid entries composite_ids = valid_ids.map do |v_id| @@ -87,7 +87,7 @@ module VagrantPlugins def self.nfs_cleanup(remove_ids) return if !File.exist?(NFS_EXPORTS_PATH) - editor = Vagrant::Util::StringBlockEditor.new(File.read(NFS_EXPORTS_PATH)) + editor = Vagrant::Util::StringBlockEditor.new(nfs_exports_content) remove_ids = Array(remove_ids) # Remove all invalid ID entries @@ -98,7 +98,7 @@ module VagrantPlugins end def self.nfs_write_exports(new_exports_content) - if(File.read(NFS_EXPORTS_PATH).strip != new_exports_content.strip) + if(nfs_exports_content != new_exports_content.strip) begin # Write contents out to temporary file new_exports_file = Tempfile.create('vagrant') @@ -106,7 +106,6 @@ module VagrantPlugins new_exports_file.close new_exports_path = new_exports_file.path - # puts File.read(new_exports_path).inspect # Only use "sudo" if we can't write to /etc/exports directly sudo_command = "" sudo_command = "sudo " if !File.writable?(NFS_EXPORTS_PATH) @@ -117,13 +116,25 @@ module VagrantPlugins if existing_stat.mode != new_stat.mode File.chmod(existing_stat.mode, new_exports_path) end - # TODO: Error check if existing_stat.uid != new_stat.uid || existing_stat.gid != new_stat.gid - system("#{sudo_command}chown #{existing_stat.uid}:#{existing_stat.gid} #{new_exports_path}") + chown_cmd = "#{sudo_command}chown #{existing_stat.uid}:#{existing_stat.gid} #{new_exports_path}" + result = Vagrant::Util::Subprocess.execute(*Shellwords.split(chown_cmd)) + if result.exit_code != 0 + raise Vagrant::Errors::NFSExportsFailed, + command: chown_cmd, + stderr: result.stderr, + stdout: result.stdout + end + end + # Always force move the file to prevent overwrite prompting + mv_cmd = "#{sudo_command}mv -f #{new_exports_path} #{NFS_EXPORTS_PATH}" + result = Vagrant::Util::Subprocess.execute(*Shellwords.split(mv_cmd)) + if result.exit_code != 0 + raise Vagrant::Errors::NFSExportsFailed, + command: mv_cmd, + stderr: result.stderr, + stdout: result.stdout end - - # Replace existing exports file - system("#{sudo_command}mv #{new_exports_path} #{NFS_EXPORTS_PATH}") ensure if File.exist?(new_exports_path) File.unlink(new_exports_path) @@ -132,6 +143,27 @@ module VagrantPlugins end end + def self.nfs_exports_content + if(File.exist?(NFS_EXPORTS_PATH)) + if(File.readable?(NFS_EXPORTS_PATH)) + File.read(NFS_EXPORTS_PATH) + else + cmd = "sudo cat #{NFS_EXPORTS_PATH}" + result = Vagrant::Util::Subprocess.execute(*Shellwords.split(cmd)) + if result.exit_code != 0 + raise Vagrant::Errors::NFSExportsFailed, + command: cmd, + stderr: result.stderr, + stdout: result.stdout + else + result.stdout + end + end + else + "" + end + end + def self.nfs_opts_setup(folders) folders.each do |k, opts| if !opts[:linux__nfs_options] diff --git a/templates/locales/en.yml b/templates/locales/en.yml index cfb067cc5..d3114d845 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -877,6 +877,14 @@ en: the issues below and execute "vagrant reload": %{output} + nfs_exports_failed: |- + Vagrant failed to install an updated NFS exports file. This may be + due to overly restrictive permissions on your NFS exports file. Please + validate them and try again. + + command: %{command} + stdout: %{stdout} + stderr: %{stderr} nfs_cant_read_exports: |- Vagrant can't read your current NFS exports! The exports file should be readable by any user. This is usually caused by invalid permissions diff --git a/test/unit/plugins/hosts/linux/cap/nfs_test.rb b/test/unit/plugins/hosts/linux/cap/nfs_test.rb index 74d673b64..144e9c8fd 100644 --- a/test/unit/plugins/hosts/linux/cap/nfs_test.rb +++ b/test/unit/plugins/hosts/linux/cap/nfs_test.rb @@ -31,20 +31,22 @@ describe VagrantPlugins::HostLinux::Cap::NFS do VagrantPlugins::HostLinux::Cap::NFS.const_set(:NFS_EXPORTS_PATH, @original_exports_path) File.unlink(tmp_exports_path.to_s) if File.exist?(tmp_exports_path.to_s) @tmp_exports = nil - @env = nil end describe ".nfs_export" do + + let(:cap){ caps.get(:nfs_export) } + before do allow(env).to receive(:host).and_return(host) allow(host).to receive(:capability).with(:nfs_apply_command).and_return("/bin/true") allow(host).to receive(:capability).with(:nfs_check_command).and_return("/bin/true") allow(host).to receive(:capability).with(:nfs_start_command).and_return("/bin/true") allow(ui).to receive(:info) + allow(cap).to receive(:system).with("sudo /bin/true").and_return(true) + allow(cap).to receive(:system).with("/bin/true").and_return(true) end - let(:cap){ caps.get(:nfs_export) } - it "should export new entries" do cap.nfs_export(env, ui, SecureRandom.uuid, ["127.0.0.1"], "tmp" => {:hostpath => "/tmp"}) exports_content = File.read(exports_path) @@ -106,7 +108,6 @@ EOH expect(exports_content).to include("/var") expect(exports_content).not_to include("/tmp") end - end describe ".nfs_write_exports" do @@ -137,8 +138,19 @@ EOH expect(original_stat.mode).to eq(updated_stat.mode) end + it "should raise exception when failing to move new exports file" do + expect(Vagrant::Util::Subprocess).to receive(:execute).and_return( + Vagrant::Util::Subprocess::Result.new(1, "Failed to move file", "") + ) + expect{ described_class.nfs_write_exports("new content") }.to raise_error(Vagrant::Errors::NFSExportsFailed) + end + it "should retain existing file owner and group IDs" do pending("investigate using a simulated FS to test") end + + it "should raise custom exception when chown fails" do + pending("investigate using a simulated FS to test") + end end end