From 3a27c2957712788120122209f19787e031bf55e3 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Sat, 28 May 2016 23:00:24 -0400 Subject: [PATCH 01/11] Add a new util for generating tempfiles --- lib/vagrant/util.rb | 1 + lib/vagrant/util/tempfile.rb | 58 ++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 lib/vagrant/util/tempfile.rb diff --git a/lib/vagrant/util.rb b/lib/vagrant/util.rb index 896b5d7e0..f3017dea4 100644 --- a/lib/vagrant/util.rb +++ b/lib/vagrant/util.rb @@ -9,6 +9,7 @@ module Vagrant autoload :SafeExec, 'vagrant/util/safe_exec' autoload :StackedProcRunner, 'vagrant/util/stacked_proc_runner' autoload :TemplateRenderer, 'vagrant/util/template_renderer' + autoload :Tempfile, 'vagrant/util/tempfile' autoload :Subprocess, 'vagrant/util/subprocess' end end diff --git a/lib/vagrant/util/tempfile.rb b/lib/vagrant/util/tempfile.rb new file mode 100644 index 000000000..66006b788 --- /dev/null +++ b/lib/vagrant/util/tempfile.rb @@ -0,0 +1,58 @@ +require "tempfile" + +module Vagrant + module Util + class Tempfile + # Utility function for creating a temporary file that will persist for + # the duration of the block and then be removed after the block finishes. + # + # @example + # + # Tempfile.create("arch-configure-networks") do |f| + # f.write("network-1") + # f.fsync + # f.close + # do_something_with_file(f.path) + # end + # + # @example + # + # Tempfile.create(["runner", "ps1"]) do |f| + # # f will have a suffix of ".ps1" + # # ... + # end + # + # @param [String, Array] name the prefix of the tempfile to create + # @param [Hash] options a list of options + # @param [Proc] block the block to yield the file object to + # + # @yield [File] + def self.create(name, options = {}) + raise "No block given!" if !block_given? + + options = { + binmode: true + }.merge(options) + + # The user can specify an array where the first parameter is the prefix + # and the last parameter is the file suffix. We want to prefix the + # "prefix" with `vagrant-`, and this does that + if name.is_a?(Array) + basename = ["vagrant-#{name[0]}", name[1]] + else + basename = "vagrant-#{name}" + end + + Dir::Tmpname.create(basename) do |path| + begin + f = File.open(path, "w+") + f.binmode if options[:binmode] + yield f + ensure + File.unlink(f.path) if File.file?(f.path) + end + end + end + end + end +end From fb60d3423666371a3486d2b0eead25fcbb92d972 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Sat, 28 May 2016 23:04:38 -0400 Subject: [PATCH 02/11] Add unique names to all tmpdir and tempfile calls in tests + cleanup This commit attempts to uniquely identify the temporary files and directories that are created during test runs. Where it was a quick fix, this commit also removes the temporary files and directories. There are still a ton of temporary files due to calls to .isolated_environment in the tests without an easy API an easy way to provide a closer to that function. --- test/support/isolated_environment.rb | 2 +- test/unit/base.rb | 10 ++- .../commands/box/command/update_test.rb | 8 ++- test/unit/plugins/pushes/ftp/adapter_test.rb | 2 +- test/unit/plugins/pushes/ftp/push_test.rb | 6 +- test/unit/support/isolated_environment.rb | 8 +-- test/unit/support/shared/base_context.rb | 14 +++- .../vagrant/action/builtin/box_add_test.rb | 64 ++++++++++--------- test/unit/vagrant/action/builtin/lock_test.rb | 7 +- .../builtin/mixin_synced_folders_test.rb | 8 ++- .../builtin/synced_folder_cleanup_test.rb | 6 +- .../action/builtin/synced_folders_test.rb | 6 +- test/unit/vagrant/box_collection_test.rb | 9 ++- test/unit/vagrant/box_test.rb | 6 +- test/unit/vagrant/environment_test.rb | 28 ++++---- test/unit/vagrant/machine_index_test.rb | 6 +- test/unit/vagrant/machine_test.rb | 6 +- test/unit/vagrant/plugin/manager_test.rb | 6 +- test/unit/vagrant/plugin/state_file_test.rb | 6 +- test/unit/vagrant/util/safe_chdir_test.rb | 16 +++-- test/unit/vagrant/vagrantfile_test.rb | 6 +- 21 files changed, 146 insertions(+), 84 deletions(-) diff --git a/test/support/isolated_environment.rb b/test/support/isolated_environment.rb index 719966bb2..013017df2 100644 --- a/test/support/isolated_environment.rb +++ b/test/support/isolated_environment.rb @@ -28,7 +28,7 @@ class IsolatedEnvironment @logger = Log4r::Logger.new("test::isolated_environment") # Create a temporary directory for our work - @tempdir = Vagrant::Util::Platform.fs_real_path(Dir.mktmpdir("vagrant")) + @tempdir = Vagrant::Util::Platform.fs_real_path(Dir.mktmpdir("vagrant-iso-env")) @logger.info("Initialize isolated environment: #{@tempdir}") # Setup the home and working directories diff --git a/test/unit/base.rb b/test/unit/base.rb index e3c68c099..0ec219ae3 100644 --- a/test/unit/base.rb +++ b/test/unit/base.rb @@ -27,6 +27,10 @@ require "unit/support/shared/virtualbox_context" $stdout.sync = true $stderr.sync = true +# Create a temporary directory where test vagrant will run. The reason we save +# this to a constant is so we can clean it up later. +VAGRANT_TEST_CWD = Dir.mktmpdir("vagrant-test-cwd") + # Configure RSpec RSpec.configure do |c| c.treat_symbols_as_metadata_keys_with_true_values = true @@ -36,11 +40,15 @@ RSpec.configure do |c| else c.filter_run_excluding :windows end + + c.after(:suite) do + FileUtils.rm_rf(VAGRANT_TEST_CWD) + end end # Configure VAGRANT_CWD so that the tests never find an actual # Vagrantfile anywhere, or at least this minimizes those chances. -ENV["VAGRANT_CWD"] = Dir.mktmpdir("vagrant") +ENV["VAGRANT_CWD"] = VAGRANT_TEST_CWD # Set the dummy provider to the default for tests ENV["VAGRANT_DEFAULT_PROVIDER"] = "dummy" diff --git a/test/unit/plugins/commands/box/command/update_test.rb b/test/unit/plugins/commands/box/command/update_test.rb index 067d0dd35..35d84c791 100644 --- a/test/unit/plugins/commands/box/command/update_test.rb +++ b/test/unit/plugins/commands/box/command/update_test.rb @@ -35,7 +35,9 @@ describe VagrantPlugins::CommandBox::Command::Update do context "updating specific box" do let(:argv) { ["--box", "foo"] } - let(:metadata_url) { Pathname.new(Dir.mktmpdir).join("metadata.json") } + let(:scratch) { Dir.mktmpdir("vagrant-test-command-box-update-execute") } + + let(:metadata_url) { Pathname.new(scratch).join("metadata.json") } before do metadata_url.open("w") do |f| @@ -46,6 +48,10 @@ describe VagrantPlugins::CommandBox::Command::Update do "foo", "1.0", :virtualbox, metadata_url: metadata_url.to_s) end + after do + FileUtils.rm_rf(scratch) + end + it "doesn't update if they're up to date" do called = false allow(action_runner).to receive(:run) do |callable, opts| diff --git a/test/unit/plugins/pushes/ftp/adapter_test.rb b/test/unit/plugins/pushes/ftp/adapter_test.rb index 6ef07196a..b577ee49d 100644 --- a/test/unit/plugins/pushes/ftp/adapter_test.rb +++ b/test/unit/plugins/pushes/ftp/adapter_test.rb @@ -64,7 +64,7 @@ describe VagrantPlugins::FTPPush::FTPAdapter do describe "#upload" do before do - @dir = Dir.mktmpdir + @dir = Dir.mktmpdir("vagrant-ftp-push-adapter-upload") FileUtils.touch("#{@dir}/file") end diff --git a/test/unit/plugins/pushes/ftp/push_test.rb b/test/unit/plugins/pushes/ftp/push_test.rb index 00ed3ae11..96537cd42 100644 --- a/test/unit/plugins/pushes/ftp/push_test.rb +++ b/test/unit/plugins/pushes/ftp/push_test.rb @@ -109,7 +109,7 @@ describe VagrantPlugins::FTPPush::Push do describe "#all_files" do before(:all) do - @dir = Dir.mktmpdir + @dir = Dir.mktmpdir("vagrant-ftp-push-push-all-files") FileUtils.touch("#{@dir}/.hidden.rb") FileUtils.touch("#{@dir}/application.rb") @@ -151,9 +151,9 @@ describe VagrantPlugins::FTPPush::Push do end end - describe "includes_files" do + describe "#includes_files" do before(:all) do - @dir = Dir.mktmpdir + @dir = Dir.mktmpdir("vagrant-ftp-push-includes-files") FileUtils.touch("#{@dir}/.hidden.rb") FileUtils.touch("#{@dir}/application.rb") diff --git a/test/unit/support/isolated_environment.rb b/test/unit/support/isolated_environment.rb index d8393f72b..de2848927 100644 --- a/test/unit/support/isolated_environment.rb +++ b/test/unit/support/isolated_environment.rb @@ -132,8 +132,8 @@ module Unit # @return [Pathname] Path to the newly created box. def box1_file # Create a temporary directory to store our data we will tar up - td_source = Dir.mktmpdir - td_dest = Dir.mktmpdir + td_source = Dir.mktmpdir("vagrant-box1-source") + td_dest = Dir.mktmpdir("vagrant-box-1-dest") # Store the temporary directory so it is not deleted until # this instance is garbage collected. @@ -177,8 +177,8 @@ module Unit }.merge(options[:metadata] || {}) # Create a temporary directory to store our data we will tar up - td_source = Dir.mktmpdir - td_dest = Dir.mktmpdir + td_source = Dir.mktmpdir("vagrant-box-2-source") + td_dest = Dir.mktmpdir("vagrant-box-2-dest") # Store the temporary directory so it is not deleted until # this instance is garbage collected. diff --git a/test/unit/support/shared/base_context.rb b/test/unit/support/shared/base_context.rb index c84b6ae1d..23a29294e 100644 --- a/test/unit/support/shared/base_context.rb +++ b/test/unit/support/shared/base_context.rb @@ -73,19 +73,27 @@ shared_context "unit" do end # This creates a temporary directory and returns a {Pathname} - # pointing to it. + # pointing to it. If a block is given, the pathname is yielded and the + # temporary directory is removed at the end of the block. # # @return [Pathname] def temporary_dir # Create a temporary directory and append it to the instance # variabe so that it isn't garbage collected and deleted - d = Dir.mktmpdir("vagrant") + d = Dir.mktmpdir("vagrant-temporary-dir") @_temp_files ||= [] @_temp_files << d # Return the pathname result = Pathname.new(Vagrant::Util::Platform.fs_real_path(d)) - yield result if block_given? + if block_given? + begin + yield result + ensure + FileUtils.rm_rf(result) + end + end + return result end diff --git a/test/unit/vagrant/action/builtin/box_add_test.rb b/test/unit/vagrant/action/builtin/box_add_test.rb index db3c7044f..c1409a558 100644 --- a/test/unit/vagrant/action/builtin/box_add_test.rb +++ b/test/unit/vagrant/action/builtin/box_add_test.rb @@ -17,7 +17,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do let(:env) { { box_collection: box_collection, hook: Proc.new { |name, env| env }, - tmp_path: Pathname.new(Dir.mktmpdir), + tmp_path: Pathname.new(Dir.mktmpdir("vagrant-test-builtin-box-add")), ui: Vagrant::UI::Silent.new, } } @@ -31,6 +31,10 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do Vagrant::Box.new("foo", :virtualbox, "1.0", box_dir) end + after do + FileUtils.rm_rf(env[:tmp_path]) + end + # Helper to quickly SHA1 checksum a path def checksum(path) FileChecksum.new(path, Digest::SHA1).checksum @@ -39,9 +43,6 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do def with_ftp_server(path, **opts) path = Pathname.new(path) - tf = Tempfile.new("vagrant") - tf.close - port = nil server = nil with_random_port do |port1, port2| @@ -56,7 +57,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do end def with_web_server(path, **opts) - tf = Tempfile.new("vagrant") + tf = Tempfile.new("vagrant-web-server") tf.close opts[:json_type] ||= "application/json" @@ -74,6 +75,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do thr = Thread.new { server.start } yield port ensure + tf.unlink server.shutdown rescue nil thr.join rescue nil end @@ -282,7 +284,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do context "with box metadata" do it "adds from HTTP URL" do box_path = iso_env.box2_file(:virtualbox) - tf = Tempfile.new(["vagrant", ".json"]).tap do |f| + tf = Tempfile.new(["vagrant-test-box-http-url", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -325,7 +327,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "adds from HTTP URL with complex JSON mime type" do box_path = iso_env.box2_file(:virtualbox) - tf = Tempfile.new(["vagrant", ".json"]).tap do |f| + tf = Tempfile.new(["vagrant-test-http-json", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -370,7 +372,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "adds from shorthand path" do box_path = iso_env.box2_file(:virtualbox) - td = Pathname.new(Dir.mktmpdir) + td = Pathname.new(Dir.mktmpdir("vagrant-test-box-add-shorthand-path")) tf = td.join("mitchellh", "precise64.json") tf.dirname.mkpath tf.open("w") do |f| @@ -414,11 +416,13 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do subject.call(env) end end + + FileUtils.rm_rf(td) end it "add from shorthand path with configured server url" do box_path = iso_env.box2_file(:virtualbox) - td = Pathname.new(Dir.mktmpdir) + td = Pathname.new(Dir.mktmpdir("vagrant-test-box-add-server-url")) tf = td.join("mitchellh", "precise64.json") tf.dirname.mkpath tf.open("w") do |f| @@ -461,11 +465,13 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do subject.call(env) end + + FileUtils.rm_rf(td) end it "authenticates HTTP URLs and adds them" do box_path = iso_env.box2_file(:virtualbox) - tf = Tempfile.new(["vagrant", ".json"]).tap do |f| + tf = Tempfile.new(["vagrant-test-http", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -523,7 +529,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "adds from HTTP URL with a checksum" do box_path = iso_env.box2_file(:virtualbox) - tf = Tempfile.new(["vagrant", ".json"]).tap do |f| + tf = Tempfile.new(["vagrant-test-http-checksum", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -568,7 +574,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "raises an exception if checksum given but not correct" do box_path = iso_env.box2_file(:virtualbox) - tf = Tempfile.new(["vagrant", ".json"]).tap do |f| + tf = Tempfile.new(["vagrant-test-bad-checksum", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -606,9 +612,6 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do end it "raises an error if no Vagrant server is set" do - tf = Tempfile.new("foo") - tf.close - env[:box_url] = "mitchellh/precise64.json" expect(box_collection).to receive(:add).never @@ -621,10 +624,9 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do end it "raises an error if shorthand is invalid" do - tf = Tempfile.new("foo") - tf.close + path = Dir::Tmpname.create("vagrant-shorthand-invalid") {} - with_web_server(Pathname.new(tf.path)) do |port| + with_web_server(Pathname.new(path)) do |port| env[:box_url] = "mitchellh/precise64.json" expect(box_collection).to receive(:add).never @@ -640,7 +642,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "raises an error if multiple metadata URLs are given" do box_path = iso_env.box2_file(:virtualbox) - tf = Tempfile.new("vagrant").tap do |f| + tf = Tempfile.new(["vagrant-box-multi-metadata", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -676,7 +678,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "adds the latest version of a box with only one provider" do box_path = iso_env.box2_file(:virtualbox) - tf = Tempfile.new("vagrant").tap do |f| + tf = Tempfile.new(["vagrant-box-latest-version", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -715,7 +717,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "adds the latest version of a box with the specified provider" do box_path = iso_env.box2_file(:vmware) - tf = Tempfile.new("vagrant").tap do |f| + tf = Tempfile.new(["vagrant-box-specific-provider", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -761,7 +763,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "adds the latest version of a box with the specified provider, even if not latest" do box_path = iso_env.box2_file(:vmware) - tf = Tempfile.new("vagrant").tap do |f| + tf = Tempfile.new(["vagrant-box-specified-provider", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -810,7 +812,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "adds the constrained version of a box with the only provider" do box_path = iso_env.box2_file(:vmware) - tf = Tempfile.new("vagrant").tap do |f| + tf = Tempfile.new(["vagrant-box-constrained", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -850,7 +852,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "adds the constrained version of a box with the specified provider" do box_path = iso_env.box2_file(:vmware) - tf = Tempfile.new("vagrant").tap do |f| + tf = Tempfile.new(["vagrant-box-constrained-provider", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -895,7 +897,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "adds the latest version of a box with any specified provider" do box_path = iso_env.box2_file(:vmware) - tf = Tempfile.new("vagrant").tap do |f| + tf = Tempfile.new(["vagrant-box-latest-version", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -943,7 +945,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "asks the user what provider if multiple options" do box_path = iso_env.box2_file(:virtualbox) - tf = Tempfile.new("vagrant").tap do |f| + tf = Tempfile.new(["vagrant-box-provider-asks", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -989,7 +991,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "raises an exception if the name doesn't match a requested name" do box_path = iso_env.box2_file(:virtualbox) - tf = Tempfile.new("vagrant").tap do |f| + tf = Tempfile.new(["vagrant-box-name-mismatch", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -1024,7 +1026,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "raises an exception if no matching version" do box_path = iso_env.box2_file(:vmware) - tf = Tempfile.new("vagrant").tap do |f| + tf = Tempfile.new(["vagrant-box-no-matching-version", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -1055,7 +1057,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do end it "raises an error if there is no matching provider" do - tf = Tempfile.new("vagrant").tap do |f| + tf = Tempfile.new(["vagrant-box-no-matching-provider", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -1089,7 +1091,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "raises an error if a box already exists" do box_path = iso_env.box2_file(:virtualbox) - tf = Tempfile.new("vagrant").tap do |f| + tf = Tempfile.new(["vagrant-box-already-exists", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", @@ -1124,7 +1126,7 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows do it "force adds a box if specified" do box_path = iso_env.box2_file(:virtualbox) - tf = Tempfile.new("vagrant").tap do |f| + tf = Tempfile.new(["vagrant-box-force-add", ".json"]).tap do |f| f.write(<<-RAW) { "name": "foo/bar", diff --git a/test/unit/vagrant/action/builtin/lock_test.rb b/test/unit/vagrant/action/builtin/lock_test.rb index f2cdf793a..a04acf1dd 100644 --- a/test/unit/vagrant/action/builtin/lock_test.rb +++ b/test/unit/vagrant/action/builtin/lock_test.rb @@ -4,8 +4,7 @@ describe Vagrant::Action::Builtin::Lock do let(:app) { lambda { |env| } } let(:env) { {} } let(:lock_path) do - @__lock_path = Tempfile.new("vagrant-test-lock") - @__lock_path.path.to_s + Dir::Tmpname.create("vagrant-test-lock") {} end let(:options) do @@ -15,6 +14,10 @@ describe Vagrant::Action::Builtin::Lock do } end + after do + File.unlink(lock_path) if File.file?(lock_path) + end + it "should require a path" do expect { described_class.new(app, env) }. to raise_error(ArgumentError) diff --git a/test/unit/vagrant/action/builtin/mixin_synced_folders_test.rb b/test/unit/vagrant/action/builtin/mixin_synced_folders_test.rb index 67a5eca27..ab4934dff 100644 --- a/test/unit/vagrant/action/builtin/mixin_synced_folders_test.rb +++ b/test/unit/vagrant/action/builtin/mixin_synced_folders_test.rb @@ -13,9 +13,9 @@ describe Vagrant::Action::Builtin::MixinSyncedFolders do end end - let(:machine) do - data_dir = Pathname.new(Dir.mktmpdir) + let(:data_dir) { Pathname.new(Dir.mktmpdir("vagrant-test-mixin-synced-folders")) } + let(:machine) do double("machine").tap do |machine| allow(machine).to receive(:config).and_return(machine_config) allow(machine).to receive(:data_dir).and_return(data_dir) @@ -30,6 +30,10 @@ describe Vagrant::Action::Builtin::MixinSyncedFolders do let(:vm_config) { double("machine_vm_config", :allowed_synced_folder_types => nil) } + after do + FileUtils.rm_rf(data_dir) + end + describe "default_synced_folder_type" do it "returns the usable implementation" do plugins = { diff --git a/test/unit/vagrant/action/builtin/synced_folder_cleanup_test.rb b/test/unit/vagrant/action/builtin/synced_folder_cleanup_test.rb index 52973f33d..c50d28628 100644 --- a/test/unit/vagrant/action/builtin/synced_folder_cleanup_test.rb +++ b/test/unit/vagrant/action/builtin/synced_folder_cleanup_test.rb @@ -53,12 +53,16 @@ describe Vagrant::Action::Builtin::SyncedFolderCleanup do plugins[:nfs] = [impl(true, "nfs"), 5] env[:machine] = Object.new - env[:root_path] = Pathname.new(Dir.mktmpdir) + env[:root_path] = Pathname.new(Dir.mktmpdir("vagrant-test-synced-folder-cleanup-call")) subject.stub(plugins: plugins) subject.stub(synced_folders: synced_folders) end + after do + FileUtils.rm_rf(env[:root_path]) + end + it "should invoke cleanup" do tracker = create_cleanup_tracker plugins[:tracker] = [tracker, 15] diff --git a/test/unit/vagrant/action/builtin/synced_folders_test.rb b/test/unit/vagrant/action/builtin/synced_folders_test.rb index 5e606b005..f602abd58 100644 --- a/test/unit/vagrant/action/builtin/synced_folders_test.rb +++ b/test/unit/vagrant/action/builtin/synced_folders_test.rb @@ -41,12 +41,16 @@ describe Vagrant::Action::Builtin::SyncedFolders do plugins[:default] = [impl(true, "default"), 10] plugins[:nfs] = [impl(true, "nfs"), 5] - env[:root_path] = Pathname.new(Dir.mktmpdir) + env[:root_path] = Pathname.new(Dir.mktmpdir("vagrant-test-synced-folders-call")) subject.stub(plugins: plugins) subject.stub(synced_folders: synced_folders) allow(subject).to receive(:save_synced_folders) end + after do + FileUtils.rm_rf(env[:root_path]) + end + it "should create on the host if specified" do synced_folders["default"] = { "root" => { diff --git a/test/unit/vagrant/box_collection_test.rb b/test/unit/vagrant/box_collection_test.rb index 21a39ae70..7f121f17d 100644 --- a/test/unit/vagrant/box_collection_test.rb +++ b/test/unit/vagrant/box_collection_test.rb @@ -39,8 +39,13 @@ describe Vagrant::BoxCollection, :skip_windows do end it 'does not raise an exception when a file appears in the boxes dir' do - Tempfile.new('a_file', environment.boxes_dir) - expect { subject.all }.to_not raise_error + t = Tempfile.new('a_file', environment.boxes_dir) + t.close + begin + expect { subject.all }.to_not raise_error + ensure + t.unlink + end end end diff --git a/test/unit/vagrant/box_test.rb b/test/unit/vagrant/box_test.rb index f5fa7dac3..8843e64b4 100644 --- a/test/unit/vagrant/box_test.rb +++ b/test/unit/vagrant/box_test.rb @@ -227,7 +227,7 @@ describe Vagrant::Box, :skip_windows do context "#load_metadata" do let(:metadata_url) do - Tempfile.new("vagrant").tap do |f| + Tempfile.new("vagrant-test-box-test").tap do |f| f.write(<<-RAW) { "name": "foo", @@ -244,6 +244,10 @@ describe Vagrant::Box, :skip_windows do metadata_url: metadata_url.path) end + after do + metadata_url.unlink if metadata_url.file? + end + it "loads the url and returns the data" do result = subject.load_metadata expect(result.name).to eq("foo") diff --git a/test/unit/vagrant/environment_test.rb b/test/unit/vagrant/environment_test.rb index 37a2d0caf..b47ab7c66 100644 --- a/test/unit/vagrant/environment_test.rb +++ b/test/unit/vagrant/environment_test.rb @@ -69,14 +69,14 @@ describe Vagrant::Environment do describe "#home_path" do it "is set to the home path given" do - temporary_dir do |dir| + Dir.mktmpdir("vagrant-test-env-home-path-given") do |dir| instance = described_class.new(home_path: dir) expect(instance.home_path).to eq(Pathname.new(dir)) end end it "is set to the environmental variable VAGRANT_HOME" do - temporary_dir do |dir| + Dir.mktmpdir("vagrant-test-env-home-env-var") do |dir| instance = with_temp_env("VAGRANT_HOME" => dir.to_s) do described_class.new end @@ -99,7 +99,7 @@ describe Vagrant::Environment do end it "is okay if it has the current version" do - Dir.mktmpdir do |dir| + Dir.mktmpdir("vagrant-test-env-current-version") do |dir| Pathname.new(dir).join("setup_version").open("w") do |f| f.write(Vagrant::Environment::CURRENT_SETUP_VERSION) end @@ -112,7 +112,7 @@ describe Vagrant::Environment do end it "raises an exception if the version is newer than ours" do - Dir.mktmpdir do |dir| + Dir.mktmpdir("vagrant-test-env-newer-version") do |dir| Pathname.new(dir).join("setup_version").open("w") do |f| f.write("100.5") end @@ -123,7 +123,7 @@ describe Vagrant::Environment do end it "raises an exception if there is an unknown home directory version" do - Dir.mktmpdir do |dir| + Dir.mktmpdir("vagrant-test-env-unknown-home") do |dir| Pathname.new(dir).join("setup_version").open("w") do |f| f.write("0.7") end @@ -645,7 +645,7 @@ VF describe "active machines" do it "should be empty if there is no root path" do - Dir.mktmpdir do |temp_dir| + Dir.mktmpdir("vagrant-test-env-no-root-path") do |temp_dir| instance = described_class.new(cwd: temp_dir) expect(instance.active_machines).to be_empty end @@ -715,7 +715,7 @@ VF describe "current working directory" do it "is the cwd by default" do - Dir.mktmpdir do |temp_dir| + Dir.mktmpdir("vagrant-test-env-cwd-default") do |temp_dir| Dir.chdir(temp_dir) do with_temp_env("VAGRANT_CWD" => nil) do expect(described_class.new.cwd).to eq(Pathname.new(Dir.pwd)) @@ -725,14 +725,14 @@ VF end it "is set to the cwd given" do - Dir.mktmpdir do |directory| + Dir.mktmpdir("vagrant-test-env-set-cwd") do |directory| instance = described_class.new(cwd: directory) expect(instance.cwd).to eq(Pathname.new(directory)) end end it "is set to the environmental variable VAGRANT_CWD" do - Dir.mktmpdir do |directory| + Dir.mktmpdir("vagrant-test-env-set-vagrant-cwd") do |directory| instance = with_temp_env("VAGRANT_CWD" => directory) do described_class.new end @@ -905,7 +905,7 @@ VF end it "is expanded relative to the cwd" do - Dir.mktmpdir do |temp_dir| + Dir.mktmpdir("vagrant-test-env-relative-cwd") do |temp_dir| Dir.chdir(temp_dir) do instance = described_class.new(local_data_path: "foo") expect(instance.local_data_path).to eq(instance.cwd.join("foo")) @@ -915,7 +915,7 @@ VF end it "is set to the given value" do - Dir.mktmpdir do |dir| + Dir.mktmpdir("vagrant-test-env-set-given") do |dir| instance = described_class.new(local_data_path: dir) expect(instance.local_data_path.to_s).to eq(dir) end @@ -923,7 +923,7 @@ VF describe "upgrading V1 dotfiles" do let(:v1_dotfile_tempfile) do - Tempfile.new("vagrant").tap do |f| + Tempfile.new("vagrant-upgrade-dotfile").tap do |f| f.close end end @@ -932,6 +932,10 @@ VF let(:local_data_path) { v1_dotfile_tempfile.path } let(:instance) { described_class.new(local_data_path: local_data_path) } + after do + File.unlink(v1_dotfile_tempfile) if File.file?(v1_dotfile_template) + end + it "should be fine if dotfile is empty" do v1_dotfile.open("w+") do |f| f.write("") diff --git a/test/unit/vagrant/machine_index_test.rb b/test/unit/vagrant/machine_index_test.rb index b045af086..57ad2c951 100644 --- a/test/unit/vagrant/machine_index_test.rb +++ b/test/unit/vagrant/machine_index_test.rb @@ -9,7 +9,7 @@ require "vagrant/machine_index" describe Vagrant::MachineIndex do include_context "unit" - let(:data_dir) { temporary_dir } + let(:data_dir) { Dir.mktmpdir } let(:entry_klass) { Vagrant::MachineIndex::Entry } let(:new_entry) do @@ -21,6 +21,10 @@ describe Vagrant::MachineIndex do subject { described_class.new(data_dir) } + after do + FileUtils.rm_rf(data_dir) + end + it "raises an exception if the data file is corrupt" do data_dir.join("index").open("w") do |f| f.write(JSON.dump({})) diff --git a/test/unit/vagrant/machine_test.rb b/test/unit/vagrant/machine_test.rb index f3ec97d4a..6cf35d6f7 100644 --- a/test/unit/vagrant/machine_test.rb +++ b/test/unit/vagrant/machine_test.rb @@ -28,7 +28,7 @@ describe Vagrant::Machine do end let(:config) { env.vagrantfile.config } - let(:data_dir) { Pathname.new(Dir.mktmpdir("vagrant")) } + let(:data_dir) { Pathname.new(Dir.mktmpdir("vagrant-machine-data-dir")) } let(:env) do # We need to create a Vagrantfile so that this test environment # has a proper root path @@ -42,6 +42,10 @@ describe Vagrant::Machine do let(:instance) { new_instance } + after do + FileUtils.rm_rf(data_dir) + end + subject { instance } def new_provider_mock diff --git a/test/unit/vagrant/plugin/manager_test.rb b/test/unit/vagrant/plugin/manager_test.rb index 34ccf463e..ea8764b06 100644 --- a/test/unit/vagrant/plugin/manager_test.rb +++ b/test/unit/vagrant/plugin/manager_test.rb @@ -11,11 +11,7 @@ describe Vagrant::Plugin::Manager do include_context "unit" let(:path) do - f = Tempfile.new("vagrant") - p = f.path - f.close - f.unlink - Pathname.new(p) + Pathname.new(Dir::Tmpname.create("vagrant-test-plugin-manager") {}) end let(:bundler) { double("bundler") } diff --git a/test/unit/vagrant/plugin/state_file_test.rb b/test/unit/vagrant/plugin/state_file_test.rb index 0b0418d70..a0fbccf66 100644 --- a/test/unit/vagrant/plugin/state_file_test.rb +++ b/test/unit/vagrant/plugin/state_file_test.rb @@ -5,11 +5,7 @@ require File.expand_path("../../../base", __FILE__) describe Vagrant::Plugin::StateFile do let(:path) do - f = Tempfile.new("vagrant") - p = f.path - f.close - f.unlink - Pathname.new(p) + Pathname.new(Dir::Tmpname.create("vagrant-test-statefile") {}) end after do diff --git a/test/unit/vagrant/util/safe_chdir_test.rb b/test/unit/vagrant/util/safe_chdir_test.rb index d1025a905..95b88ebc7 100644 --- a/test/unit/vagrant/util/safe_chdir_test.rb +++ b/test/unit/vagrant/util/safe_chdir_test.rb @@ -5,10 +5,17 @@ require File.expand_path("../../../base", __FILE__) require 'vagrant/util/safe_chdir' describe Vagrant::Util::SafeChdir do + let(:temp_dir) { Dir.mktmpdir("vagrant-test-util-safe-chdir") } + let(:temp_dir2) { Dir.mktmpdir("vagrant-test-util-safe-chdir-2") } + + after do + FileUtils.rm_rf(temp_dir) + FileUtils.rm_rf(temp_dir2) + end + it "should change directories" do expected = nil result = nil - temp_dir = Dir.mktmpdir Dir.chdir(temp_dir) do expected = Dir.pwd @@ -24,15 +31,14 @@ describe Vagrant::Util::SafeChdir do it "should allow recursive chdir" do expected = nil result = nil - temp_path = Dir.mktmpdir - Dir.chdir(temp_path) do + Dir.chdir(temp_dir) do expected = Dir.pwd end expect do - described_class.safe_chdir(Dir.mktmpdir) do - described_class.safe_chdir(temp_path) do + described_class.safe_chdir(temp_dir2) do + described_class.safe_chdir(temp_dir) do result = Dir.pwd end end diff --git a/test/unit/vagrant/vagrantfile_test.rb b/test/unit/vagrant/vagrantfile_test.rb index 1354587a2..cb3480c7b 100644 --- a/test/unit/vagrant/vagrantfile_test.rb +++ b/test/unit/vagrant/vagrantfile_test.rb @@ -58,7 +58,7 @@ describe Vagrant::Vagrantfile do describe "#machine" do let(:boxes) { Vagrant::BoxCollection.new(iso_env.boxes_dir) } - let(:data_path) { Pathname.new(Dir.mktmpdir) } + let(:data_path) { Pathname.new(Dir.mktmpdir("vagrant-machine-data-path")) } let(:env) { iso_env.create_vagrant_env } let(:iso_env) { isolated_environment } let(:vagrantfile) { described_class.new(loader, keys) } @@ -86,6 +86,10 @@ describe Vagrant::Vagrantfile do VF end + after do + FileUtils.rm_rf(data_path.to_s) + end + describe '#data_dir' do subject { super().data_dir } it { should eq(data_path) } From fb7c4033a917d87be8a432ab972a29d65a19d410 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Sat, 28 May 2016 23:05:37 -0400 Subject: [PATCH 03/11] Do not create a tempfile when downloading box metadata The only reason we were using Tempfile was to generate the path. This commit switches to using `Dir::Tmpname.create`, which accomplishes the same thing without the overhead of creating and removing a tempfile. --- lib/vagrant/box.rb | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/vagrant/box.rb b/lib/vagrant/box.rb index 7b0c7da3a..a6d6aa737 100644 --- a/lib/vagrant/box.rb +++ b/lib/vagrant/box.rb @@ -118,8 +118,7 @@ module Vagrant # @param [Hash] download_options Options to pass to the downloader. # @return [BoxMetadata] def load_metadata(**download_options) - tf = Tempfile.new("vagrant") - tf.close + path = Dir::Tmpname.create("vagrant-load-metadata") {} url = @metadata_url if File.file?(url) || url !~ /^[a-z0-9]+:.*$/i @@ -129,13 +128,13 @@ module Vagrant end opts = { headers: ["Accept: application/json"] }.merge(download_options) - Util::Downloader.new(url, tf.path, **opts).download! - BoxMetadata.new(File.open(tf.path, "r")) + Util::Downloader.new(url, path, **opts).download! + BoxMetadata.new(File.open(path, "r")) rescue Errors::DownloaderError => e raise Errors::BoxMetadataDownloadError, message: e.extra_data[:message] ensure - tf.unlink if tf + File.unlink(path) if File.file?(path) end # Checks if the box has an update and returns the metadata, version, From 3d2390fc947fdc7987ebf14723a33c35fe98d1e9 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Sat, 28 May 2016 23:11:14 -0400 Subject: [PATCH 04/11] Give a unique, prefixed name to all tempfiles This commit basically grepped the code base for all uses of Dir.mktmpdir and Tempfile.new/open and ensures the value is unique within the code base and also prefixed with `vagrant-`. Previously, most invocations of these commands simply used "vagrant", thus making them indistinguishable when trying to identify leaks. --- lib/vagrant/bundler.rb | 9 ++++----- lib/vagrant/util/platform.rb | 2 +- plugins/hosts/darwin/cap/provider_install_virtualbox.rb | 9 ++++----- plugins/hosts/windows/cap/provider_install_virtualbox.rb | 9 ++++----- .../providers/virtualbox/action/package_setup_folders.rb | 2 +- 5 files changed, 14 insertions(+), 17 deletions(-) diff --git a/lib/vagrant/bundler.rb b/lib/vagrant/bundler.rb index 853807ebe..13fe174d1 100644 --- a/lib/vagrant/bundler.rb +++ b/lib/vagrant/bundler.rb @@ -56,7 +56,7 @@ module Vagrant # Setup the "local" Bundler configuration. We need to set BUNDLE_PATH # because the existence of this actually suppresses `sudo`. - @appconfigpath = Dir.mktmpdir + @appconfigpath = Dir.mktmpdir("vagrant-bundle-app-config") File.open(File.join(@appconfigpath, "config"), "w+") do |f| f.write("BUNDLE_PATH: \"#{bundle_path}\"") end @@ -254,14 +254,13 @@ module Vagrant def with_isolated_gem raise Errors::BundlerDisabled if !@enabled - tmp_gemfile = tempfile("vagrant-gemfile") - tmp_gemfile.close + tmp_gemfile = Dir::Tmpname.create("vagrant-gemfile") {} # Remove bundler settings so that Bundler isn't loaded when building # native extensions because it causes all sorts of problems. old_rubyopt = ENV["RUBYOPT"] old_gemfile = ENV["BUNDLE_GEMFILE"] - ENV["BUNDLE_GEMFILE"] = tmp_gemfile.path + ENV["BUNDLE_GEMFILE"] = tmp_gemfile ENV["RUBYOPT"] = (ENV["RUBYOPT"] || "").gsub(/-rbundler\/setup\s*/, "") # Set the GEM_HOME so gems are installed only to our local gem dir @@ -291,7 +290,7 @@ module Vagrant return yield end ensure - tmp_gemfile.unlink rescue nil + File.unlink(tmp_gemfile) if File.file?(tmp_gemfile) ENV["BUNDLE_GEMFILE"] = old_gemfile ENV["GEM_HOME"] = @gem_home diff --git a/lib/vagrant/util/platform.rb b/lib/vagrant/util/platform.rb index 0aba82150..e210e0f3b 100644 --- a/lib/vagrant/util/platform.rb +++ b/lib/vagrant/util/platform.rb @@ -125,7 +125,7 @@ module Vagrant # directory runs a different filesystem than the root directory. # However, this works in many cases. def fs_case_sensitive? - Dir.mktmpdir("vagrant") do |tmp_dir| + Dir.mktmpdir("vagrant-fs-case-sensitive") do |tmp_dir| tmp_file = File.join(tmp_dir, "FILE") File.open(tmp_file, "w") do |f| f.write("foo") diff --git a/plugins/hosts/darwin/cap/provider_install_virtualbox.rb b/plugins/hosts/darwin/cap/provider_install_virtualbox.rb index 3eba42ac1..88a7eefa5 100644 --- a/plugins/hosts/darwin/cap/provider_install_virtualbox.rb +++ b/plugins/hosts/darwin/cap/provider_install_virtualbox.rb @@ -16,8 +16,7 @@ module VagrantPlugins SHA256SUM = "62f933115498e51ddf5f2dab47dc1eebb42eb78ea1a7665cb91c53edacc847c6".freeze def self.provider_install_virtualbox(env) - tf = Tempfile.new("vagrant") - tf.close + path = Dir::Tmpname.create("vagrant-provider-install-virtualbox") {} # Prefixed UI for prettiness ui = Vagrant::UI::Prefixed.new(env.ui, "") @@ -28,11 +27,11 @@ module VagrantPlugins version: VERSION)) ui.detail(I18n.t( "vagrant.hosts.darwin.virtualbox_install_detail")) - dl = Vagrant::Util::Downloader.new(URL, tf.path, ui: ui) + dl = Vagrant::Util::Downloader.new(URL, path, ui: ui) dl.download! # Validate that the file checksum matches - actual = FileChecksum.new(tf.path, Digest::SHA2).checksum + actual = FileChecksum.new(path, Digest::SHA2).checksum if actual != SHA256SUM raise Vagrant::Errors::ProviderChecksumMismatch, provider: "virtualbox", @@ -46,7 +45,7 @@ module VagrantPlugins ui.detail(I18n.t( "vagrant.hosts.darwin.virtualbox_install_install_detail")) script = File.expand_path("../../scripts/install_virtualbox.sh", __FILE__) - result = Vagrant::Util::Subprocess.execute("bash", script, tf.path) + result = Vagrant::Util::Subprocess.execute("bash", script, path) if result.exit_code != 0 raise Vagrant::Errors::ProviderInstallFailed, provider: "virtualbox", diff --git a/plugins/hosts/windows/cap/provider_install_virtualbox.rb b/plugins/hosts/windows/cap/provider_install_virtualbox.rb index 4611990e4..0aaf649e9 100644 --- a/plugins/hosts/windows/cap/provider_install_virtualbox.rb +++ b/plugins/hosts/windows/cap/provider_install_virtualbox.rb @@ -17,8 +17,7 @@ module VagrantPlugins SHA256SUM = "3e5ed8fe4ada6eef8dfb4fe6fd79fcab4b242acf799f7d3ab4a17b43838b1e04".freeze def self.provider_install_virtualbox(env) - tf = Tempfile.new("vagrant") - tf.close + path = Dir::Tmpname.create("vagrant-provider-install-virtualbox") {} # Prefixed UI for prettiness ui = Vagrant::UI::Prefixed.new(env.ui, "") @@ -29,11 +28,11 @@ module VagrantPlugins version: VERSION)) ui.detail(I18n.t( "vagrant.hosts.windows.virtualbox_install_detail")) - dl = Vagrant::Util::Downloader.new(URL, tf.path, ui: ui) + dl = Vagrant::Util::Downloader.new(URL, path, ui: ui) dl.download! # Validate that the file checksum matches - actual = FileChecksum.new(tf.path, Digest::SHA2).checksum + actual = FileChecksum.new(path, Digest::SHA2).checksum if actual != SHA256SUM raise Vagrant::Errors::ProviderChecksumMismatch, provider: "virtualbox", @@ -47,7 +46,7 @@ module VagrantPlugins ui.detail(I18n.t( "vagrant.hosts.windows.virtualbox_install_install_detail")) script = File.expand_path("../../scripts/install_virtualbox.ps1", __FILE__) - result = Vagrant::Util::PowerShell.execute(script, tf.path) + result = Vagrant::Util::PowerShell.execute(script, path) if result.exit_code != 0 raise Vagrant::Errors::ProviderInstallFailed, provider: "virtualbox", diff --git a/plugins/providers/virtualbox/action/package_setup_folders.rb b/plugins/providers/virtualbox/action/package_setup_folders.rb index a045122b7..26a6503a5 100644 --- a/plugins/providers/virtualbox/action/package_setup_folders.rb +++ b/plugins/providers/virtualbox/action/package_setup_folders.rb @@ -14,7 +14,7 @@ module VagrantPlugins def call(env) env["package.output"] ||= "package.box" - env["package.directory"] ||= Dir.mktmpdir("package-", env[:tmp_path]) + env["package.directory"] ||= Dir.mktmpdir("vagrant-package-", env[:tmp_path]) # Match up a couple environmental variables so that the other parts of # Vagrant will do the right thing. From 5a4f3453631621f982cab78ce0a47ebfe70ef7fa Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Sat, 28 May 2016 23:17:40 -0400 Subject: [PATCH 05/11] Use Util::Tempfile when configuring networks This fixes a fairly large tempfile leak. Vagrant uses a template renderer to write network configuration files locally to disk. Then, that temporarily file is uploaded to the remote host and moved into place. Since Vagrant is such a short-lived process, GC never came along and cleaned up those tempfiles, resulting in many temporary files being created through regular Vagrant usage. The Util::Tempfile class uses a block to ensure the temporary file is deleted when the block finishes. This API required small tweaks to the usage, but provides more safety to ensure the files are deleted. --- plugins/communicators/winrm/communicator.rb | 17 +++---- plugins/guests/arch/cap/configure_networks.rb | 29 ++++++----- .../guests/coreos/cap/configure_networks.rb | 15 +++--- .../guests/debian/cap/configure_networks.rb | 22 ++++---- .../guests/fedora/cap/configure_networks.rb | 18 +++---- .../guests/freebsd/cap/configure_networks.rb | 17 +++---- .../guests/funtoo/cap/configure_networks.rb | 18 ++++--- .../guests/gentoo/cap/configure_networks.rb | 17 +++---- .../guests/netbsd/cap/configure_networks.rb | 18 +++---- plugins/guests/nixos/cap/change_host_name.rb | 19 +++---- .../guests/nixos/cap/configure_networks.rb | 20 ++++---- .../guests/openbsd/cap/configure_networks.rb | 16 +++--- .../guests/redhat/cap/configure_networks.rb | 18 +++---- .../slackware/cap/configure_networks.rb | 18 +++---- plugins/guests/suse/cap/configure_networks.rb | 18 +++---- .../provisioners/ansible/provisioner/guest.rb | 18 +++---- .../chef/provisioner/chef_apply.rb | 20 ++++---- .../guests/arch/cap/change_host_name_test.rb | 37 ++++++++++++++ .../arch/cap/configure_networks_test.rb | 48 ++++++++++++++++++ .../coreos/cap/change_host_name_test.rb | 37 ++++++++++++++ .../debian/cap/change_host_name_test.rb | 10 ++-- .../debian/cap/configure_networks_test.rb | 50 +++++++++++++++++++ 22 files changed, 339 insertions(+), 161 deletions(-) create mode 100644 test/unit/plugins/guests/arch/cap/change_host_name_test.rb create mode 100644 test/unit/plugins/guests/arch/cap/configure_networks_test.rb create mode 100644 test/unit/plugins/guests/coreos/cap/change_host_name_test.rb create mode 100644 test/unit/plugins/guests/debian/cap/configure_networks_test.rb diff --git a/plugins/communicators/winrm/communicator.rb b/plugins/communicators/winrm/communicator.rb index fe884d272..bb04ee277 100644 --- a/plugins/communicators/winrm/communicator.rb +++ b/plugins/communicators/winrm/communicator.rb @@ -5,11 +5,14 @@ require "log4r" require_relative "helper" require_relative "shell" require_relative "command_filter" +require_relative "../../../lib/vagrant/util/tempfile" module VagrantPlugins module CommunicatorWinRM # Provides communication channel for Vagrant commands via WinRM. class Communicator < Vagrant.plugin("2", :communicator) + include Vagrant::Util + def self.match?(machine) # This is useless, and will likely be removed in the future (this # whole method). @@ -202,15 +205,11 @@ module VagrantPlugins interactive: interactive, }) guest_script_path = "c:/tmp/vagrant-elevated-shell.ps1" - file = Tempfile.new(["vagrant-elevated-shell", "ps1"]) - begin - file.write(script) - file.fsync - file.close - upload(file.path, guest_script_path) - ensure - file.close - file.unlink + Tempfile.create(["vagrant-elevated-shell", "ps1"]) do |f| + f.write(script) + f.fsync + f.close + upload(f.path, guest_script_path) end # Convert to double byte unicode string then base64 encode diff --git a/plugins/guests/arch/cap/configure_networks.rb b/plugins/guests/arch/cap/configure_networks.rb index ad5a356d3..76de628dd 100644 --- a/plugins/guests/arch/cap/configure_networks.rb +++ b/plugins/guests/arch/cap/configure_networks.rb @@ -1,7 +1,6 @@ # -*- coding: utf-8 -*- -require "tempfile" - -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module GuestArch @@ -10,23 +9,29 @@ module VagrantPlugins include Vagrant::Util def self.configure_networks(machine, networks) - interfaces = Array.new + tempfiles = [] + interfaces = [] + machine.communicate.sudo("ip -o -0 addr | grep -v LOOPBACK | awk '{print $2}' | sed 's/://'") do |_, result| interfaces = result.split("\n") end - networks.each do |network| + networks.each.with_index do |network, i| network[:device] = interfaces[network[:interface]] - entry = TemplateRenderer.render("guests/arch/network_#{network[:type]}", options: network) + entry = TemplateRenderer.render("guests/arch/network_#{network[:type]}", + options: network) - temp = Tempfile.new("vagrant") - temp.binmode - temp.write(entry) - temp.close + remote_path = "/tmp/vagrant-network-#{Time.now.to_i}-#{i}" - machine.communicate.upload(temp.path, "/tmp/vagrant_network") - machine.communicate.sudo("mv /tmp/vagrant_network /etc/netctl/#{network[:device]}") + Tempfile.create("arch-configure-networks") do |f| + f.write(entry) + f.fsync + f.close + machine.communicate.upload(f.path, remote_path) + end + + machine.communicate.sudo("mv #{remote_path} /etc/netctl/#{network[:device]}") machine.communicate.sudo("ip link set #{network[:device]} down && netctl restart #{network[:device]} && netctl enable #{network[:device]}") end end diff --git a/plugins/guests/coreos/cap/configure_networks.rb b/plugins/guests/coreos/cap/configure_networks.rb index 819f6f818..eeca1302c 100644 --- a/plugins/guests/coreos/cap/configure_networks.rb +++ b/plugins/guests/coreos/cap/configure_networks.rb @@ -1,6 +1,5 @@ -require "tempfile" - -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module GuestCoreOS @@ -54,11 +53,11 @@ module VagrantPlugins }) end - Tempfile.open("vagrant") do |temp| - temp.binmode - temp.write(entry) - temp.close - comm.upload(temp.path, "/tmp/etcd-cluster.service") + Tempfile.create("coreos-configure-networks") do |f| + f.write(entry) + f.fsync + f.close + comm.upload(f.path, "/tmp/etcd-cluster.service") end comm.sudo("mv /tmp/etcd-cluster.service /media/state/units/") diff --git a/plugins/guests/debian/cap/configure_networks.rb b/plugins/guests/debian/cap/configure_networks.rb index 0580bc293..15095968e 100644 --- a/plugins/guests/debian/cap/configure_networks.rb +++ b/plugins/guests/debian/cap/configure_networks.rb @@ -1,7 +1,7 @@ -require 'set' -require 'tempfile' +require "set" -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module GuestDebian @@ -29,14 +29,14 @@ module VagrantPlugins entries << entry end - # Perform the careful dance necessary to reconfigure - # the network interfaces - temp = Tempfile.new("vagrant") - temp.binmode - temp.write(entries.join("\n")) - temp.close - - comm.upload(temp.path, "/tmp/vagrant-network-entry") + # Perform the careful dance necessary to reconfigure the network + # interfaces. + Tempfile.create("debian-configure-networks") do |f| + f.write(entries.join("\n")) + f.fsync + f.close + comm.upload(f.path, "/tmp/vagrant-network-entry") + end # Bring down all the interfaces we're reconfiguring. By bringing down # each specifically, we avoid reconfiguring eth0 (the NAT interface) so diff --git a/plugins/guests/fedora/cap/configure_networks.rb b/plugins/guests/fedora/cap/configure_networks.rb index d2aa8beca..425d4c5db 100644 --- a/plugins/guests/fedora/cap/configure_networks.rb +++ b/plugins/guests/fedora/cap/configure_networks.rb @@ -1,8 +1,8 @@ require "set" -require "tempfile" -require "vagrant/util/retryable" -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/retryable" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module GuestFedora @@ -96,12 +96,12 @@ module VagrantPlugins entry = TemplateRenderer.render("guests/fedora/network_#{network[:type]}", options: network) - temp = Tempfile.new("vagrant") - temp.binmode - temp.write(entry) - temp.close - - machine.communicate.upload(temp.path, "/tmp/vagrant-network-entry_#{interface}") + Tempfile.create("fedora-configure-networks") do |f| + f.write(entry) + f.fsync + f.close + machine.communicate.upload(f.path, "/tmp/vagrant-network-entry_#{interface}") + end end # Bring down all the interfaces we're reconfiguring. By bringing down diff --git a/plugins/guests/freebsd/cap/configure_networks.rb b/plugins/guests/freebsd/cap/configure_networks.rb index 27dfc850d..4744c641d 100644 --- a/plugins/guests/freebsd/cap/configure_networks.rb +++ b/plugins/guests/freebsd/cap/configure_networks.rb @@ -1,6 +1,5 @@ -require "tempfile" - -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module GuestFreeBSD @@ -29,13 +28,13 @@ module VagrantPlugins entry = TemplateRenderer.render("guests/freebsd/network_#{network[:type]}", options: network, ifname: ifname) - # Write the entry to a temporary location - temp = Tempfile.new("vagrant") - temp.binmode - temp.write(entry) - temp.close + Tempfile.create("freebsd-configure-networks") do |f| + f.write(entry) + f.fsync + f.close + machine.communicate.upload(f.path, "/tmp/vagrant-network-entry") + end - machine.communicate.upload(temp.path, "/tmp/vagrant-network-entry") machine.communicate.sudo("su -m root -c 'cat /tmp/vagrant-network-entry >> /etc/rc.conf'", {shell: "sh"}) machine.communicate.sudo("rm -f /tmp/vagrant-network-entry", {shell: "sh"}) diff --git a/plugins/guests/funtoo/cap/configure_networks.rb b/plugins/guests/funtoo/cap/configure_networks.rb index c52182b6f..d5a1a653a 100644 --- a/plugins/guests/funtoo/cap/configure_networks.rb +++ b/plugins/guests/funtoo/cap/configure_networks.rb @@ -1,6 +1,5 @@ -require "tempfile" - -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module GuestFuntoo @@ -23,12 +22,15 @@ module VagrantPlugins ifFile = "netif.eth#{network[:interface]}" entry = TemplateRenderer.render("guests/funtoo/network_#{network[:type]}", options: network) + # Upload the entry to a temporary location - temp = Tempfile.new("vagrant") - temp.binmode - temp.write(entry) - temp.close - comm.upload(temp.path, "/tmp/vagrant-#{ifFile}") + Tempfile.create("funtoo-configure-networks") do |f| + f.write(entry) + f.fsync + f.close + comm.upload(f.path, "/tmp/vagrant-#{ifFile}") + end + comm.sudo("cp /tmp/vagrant-#{ifFile} /etc/conf.d/#{ifFile}") comm.sudo("chmod 0644 /etc/conf.d/#{ifFile}") comm.sudo("ln -fs /etc/init.d/netif.tmpl /etc/init.d/#{ifFile}") diff --git a/plugins/guests/gentoo/cap/configure_networks.rb b/plugins/guests/gentoo/cap/configure_networks.rb index 549977b02..5d7796ef0 100644 --- a/plugins/guests/gentoo/cap/configure_networks.rb +++ b/plugins/guests/gentoo/cap/configure_networks.rb @@ -1,6 +1,5 @@ -require "tempfile" - -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module GuestGentoo @@ -21,12 +20,12 @@ module VagrantPlugins options: network) # Upload the entry to a temporary location - temp = Tempfile.new("vagrant") - temp.binmode - temp.write(entry) - temp.close - - comm.upload(temp.path, "/tmp/vagrant-network-entry") + Tempfile.create("gentoo-configure-networks") do |f| + f.write(entry) + f.fsync + f.close + comm.upload(f.path, "/tmp/vagrant-network-entry") + end # Configure the interface comm.sudo("ln -fs /etc/init.d/net.lo /etc/init.d/net.eth#{network[:interface]}") diff --git a/plugins/guests/netbsd/cap/configure_networks.rb b/plugins/guests/netbsd/cap/configure_networks.rb index b5c157c4c..d52f48267 100644 --- a/plugins/guests/netbsd/cap/configure_networks.rb +++ b/plugins/guests/netbsd/cap/configure_networks.rb @@ -1,6 +1,5 @@ -require "tempfile" - -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module GuestNetBSD @@ -20,13 +19,13 @@ module VagrantPlugins entry = TemplateRenderer.render("guests/netbsd/network_#{network[:type]}", options: network) - temp = Tempfile.new("vagrant") - temp.binmode - temp.write(entry) - temp.close + Tempfile.create("netbsd-configure-networks") do |f| + f.write(entry) + f.fsync + f.close + machine.communicate.upload(f.path, "/tmp/vagrant-network-entry") + end - # upload it and append it to the new rc.conf file - machine.communicate.upload(temp.path, "/tmp/vagrant-network-entry") machine.communicate.sudo("cat /tmp/vagrant-network-entry >> #{newrcconf}") machine.communicate.sudo("rm -f /tmp/vagrant-network-entry") @@ -45,7 +44,6 @@ module VagrantPlugins # install new rc.conf machine.communicate.sudo("install -c -o 0 -g 0 -m 644 #{newrcconf} /etc/rc.conf") - end end end diff --git a/plugins/guests/nixos/cap/change_host_name.rb b/plugins/guests/nixos/cap/change_host_name.rb index 6b951113b..5fbd5c38a 100644 --- a/plugins/guests/nixos/cap/change_host_name.rb +++ b/plugins/guests/nixos/cap/change_host_name.rb @@ -1,6 +1,5 @@ -require 'tempfile' - -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module GuestNixos @@ -16,13 +15,15 @@ module VagrantPlugins # Upload a file. def self.upload(machine, content, remote_path) - local_temp = Tempfile.new("vagrant-upload") - local_temp.binmode - local_temp.write(content) - local_temp.close remote_temp = mktemp(machine) - machine.communicate.upload(local_temp.path, "#{remote_temp}") - local_temp.delete + + Tempfile.create("nixos-change-host-name") do |f| + f.write(content) + f.fsync + f.close + machine.communicate.upload(f.path, "#{remote_temp}") + end + machine.communicate.sudo("mv #{remote_temp} #{remote_path}") end diff --git a/plugins/guests/nixos/cap/configure_networks.rb b/plugins/guests/nixos/cap/configure_networks.rb index b2174c81c..4161610b4 100644 --- a/plugins/guests/nixos/cap/configure_networks.rb +++ b/plugins/guests/nixos/cap/configure_networks.rb @@ -1,7 +1,7 @@ -require 'tempfile' -require 'ipaddr' +require "ipaddr" -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module GuestNixos @@ -65,13 +65,15 @@ module VagrantPlugins # Upload a file. def self.upload(machine, content, remote_path) - local_temp = Tempfile.new("vagrant-upload") - local_temp.binmode - local_temp.write(content) - local_temp.close remote_temp = mktemp(machine) - machine.communicate.upload(local_temp.path, "#{remote_temp}") - local_temp.delete + + Tempfile.create("nixos-configure-networks") do |f| + f.write(content) + f.fsync + f.close + machine.communicate.upload(f.path, "#{remote_temp}") + end + machine.communicate.sudo("mv #{remote_temp} #{remote_path}") end diff --git a/plugins/guests/openbsd/cap/configure_networks.rb b/plugins/guests/openbsd/cap/configure_networks.rb index 5ca9fed07..774fe2c79 100644 --- a/plugins/guests/openbsd/cap/configure_networks.rb +++ b/plugins/guests/openbsd/cap/configure_networks.rb @@ -1,6 +1,5 @@ -require "tempfile" - -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module GuestOpenBSD @@ -13,10 +12,12 @@ module VagrantPlugins entry = TemplateRenderer.render("guests/openbsd/network_#{network[:type]}", options: network) - temp = Tempfile.new("vagrant") - temp.binmode - temp.write(entry) - temp.close + Tempfile.create("openbsd-configure-networks") do |f| + f.write(entry) + f.fsync + f.close + machine.communicate.upload(f.path, "/tmp/vagrant-network-entry") + end # Determine the interface prefix... command = "ifconfig -a | grep -o ^[0-9a-z]*" @@ -31,7 +32,6 @@ module VagrantPlugins end end - machine.communicate.upload(temp.path, "/tmp/vagrant-network-entry") machine.communicate.sudo("mv /tmp/vagrant-network-entry /etc/hostname.#{ifname}") # remove old configurations diff --git a/plugins/guests/redhat/cap/configure_networks.rb b/plugins/guests/redhat/cap/configure_networks.rb index f176cca3d..7001bacc8 100644 --- a/plugins/guests/redhat/cap/configure_networks.rb +++ b/plugins/guests/redhat/cap/configure_networks.rb @@ -1,8 +1,8 @@ require "set" -require "tempfile" -require "vagrant/util/retryable" -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/retryable" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module GuestRedHat @@ -55,12 +55,12 @@ module VagrantPlugins entry = TemplateRenderer.render("guests/redhat/network_#{network[:type]}", options: network) - temp = Tempfile.new("vagrant") - temp.binmode - temp.write(entry) - temp.close - - machine.communicate.upload(temp.path, "/tmp/vagrant-network-entry_#{network[:interface]}") + Tempfile.create("red-hat-configure-networks") do |f| + f.write(entry) + f.fsync + f.close + machine.communicate.upload(f.path, "/tmp/vagrant-network-entry_#{network[:interface]}") + end end # Bring down all the interfaces we're reconfiguring. By bringing down diff --git a/plugins/guests/slackware/cap/configure_networks.rb b/plugins/guests/slackware/cap/configure_networks.rb index dc84080e0..d3b8e23fc 100644 --- a/plugins/guests/slackware/cap/configure_networks.rb +++ b/plugins/guests/slackware/cap/configure_networks.rb @@ -1,7 +1,6 @@ # -*- coding: utf-8 -*- -require "tempfile" - -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module GuestSlackware @@ -20,15 +19,16 @@ module VagrantPlugins entry = TemplateRenderer.render("guests/slackware/network_#{network[:type]}", options: network) - temp = Tempfile.new("vagrant") - temp.binmode - temp.write(entry) - temp.close + Tempfile.create("slackware-configure-networks") do |f| + f.write(entry) + f.fsync + f.close + machine.communicate.upload(f.path, "/tmp/vagrant_network") + end - machine.communicate.upload(temp.path, "/tmp/vagrant_network") machine.communicate.sudo("mv /tmp/vagrant_network /etc/rc.d/rc.inet1.conf") machine.communicate.sudo("/etc/rc.d/rc.inet1") - end + end end end end diff --git a/plugins/guests/suse/cap/configure_networks.rb b/plugins/guests/suse/cap/configure_networks.rb index 00902dbd0..204ff5f69 100644 --- a/plugins/guests/suse/cap/configure_networks.rb +++ b/plugins/guests/suse/cap/configure_networks.rb @@ -1,8 +1,8 @@ require "set" -require "tempfile" -require "vagrant/util/retryable" -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/retryable" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module GuestSUSE @@ -33,12 +33,12 @@ module VagrantPlugins entry = TemplateRenderer.render("guests/suse/network_#{network[:type]}", options: network) - temp = Tempfile.new("vagrant") - temp.binmode - temp.write(entry) - temp.close - - machine.communicate.upload(temp.path, "/tmp/vagrant-network-entry_#{network[:interface]}") + Tempfile.create("suse-configure-networks") do |f| + f.write(entry) + f.fsync + f.close + machine.communicate.upload(f.path, "/tmp/vagrant-network-entry_#{network[:interface]}") + end end # Bring down all the interfaces we're reconfiguring. By bringing down diff --git a/plugins/provisioners/ansible/provisioner/guest.rb b/plugins/provisioners/ansible/provisioner/guest.rb index ade03151e..3f9fd7163 100644 --- a/plugins/provisioners/ansible/provisioner/guest.rb +++ b/plugins/provisioners/ansible/provisioner/guest.rb @@ -1,11 +1,11 @@ -require 'tempfile' - require_relative "base" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module Ansible module Provisioner class Guest < Base + include Vagrant::Util def initialize(machine, config) super @@ -103,14 +103,14 @@ module VagrantPlugins inventory_basedir = File.join(config.tmp_path, "inventory") inventory_path = File.join(inventory_basedir, "vagrant_ansible_local_inventory") - temp_inventory = Tempfile.new("vagrant_ansible_local_inventory_#{@machine.name}") - temp_inventory.write(inventory_content) - temp_inventory.close - create_and_chown_remote_folder(inventory_basedir) - @machine.communicate.tap do |comm| - comm.sudo("rm -f #{inventory_path}", error_check: false) - comm.upload(temp_inventory.path, inventory_path) + @machine.communicate.sudo("rm -f #{inventory_path}", error_check: false) + + Tempfile.create("ansible-local-inventory-#{@machine.name}") do |f| + f.write(inventory_content) + f.fsync + f.close + @machine.communicate.upload(f.path, inventory_path) end return inventory_basedir diff --git a/plugins/provisioners/chef/provisioner/chef_apply.rb b/plugins/provisioners/chef/provisioner/chef_apply.rb index 602c81f3d..a5aab3d8a 100644 --- a/plugins/provisioners/chef/provisioner/chef_apply.rb +++ b/plugins/provisioners/chef/provisioner/chef_apply.rb @@ -1,7 +1,7 @@ require "digest/md5" -require "tempfile" require_relative "base" +require_relative "../../../../lib/vagrant/util/tempfile" module VagrantPlugins module Chef @@ -54,17 +54,15 @@ module VagrantPlugins # Write the raw recipe contents to a tempfile and upload that to the # machine. def upload_recipe - # Write the raw recipe contents to a tempfile - file = Tempfile.new(["vagrant-chef-apply", ".rb"]) - file.write(config.recipe) - file.rewind + # Write the raw recipe contents to a tempfile and upload + Tempfile.create(["chef-apply", ".rb"]) do |f| + f.write(config.recipe) + f.fsync + f.close - # Upload the tempfile to the guest - @machine.communicate.upload(file.path, target_recipe_path) - ensure - # Delete our template - file.close - file.unlink + # Upload the tempfile to the guest + @machine.communicate.upload(f.path, target_recipe_path) + end end end end diff --git a/test/unit/plugins/guests/arch/cap/change_host_name_test.rb b/test/unit/plugins/guests/arch/cap/change_host_name_test.rb new file mode 100644 index 000000000..c8cf9c1ce --- /dev/null +++ b/test/unit/plugins/guests/arch/cap/change_host_name_test.rb @@ -0,0 +1,37 @@ +require_relative "../../../../base" + +describe "VagrantPlugins::GuestArch::Cap::ChangeHostName" do + let(:described_class) do + VagrantPlugins::GuestArch::Plugin + .components + .guest_capabilities[:arch] + .get(:change_host_name) + end + + let(:machine) { double("machine") } + let(:communicator) { VagrantTests::DummyCommunicator::Communicator.new(machine) } + + before do + allow(machine).to receive(:communicate).and_return(communicator) + end + + after do + communicator.verify_expectations! + end + + describe ".change_host_name" do + let(:hostname) { "example.com" } + + it "sets the hostname" do + communicator.stub_command("sudo hostname | grep '#{hostname}'", exit_code: 1) + communicator.expect_command("hostnamectl set-hostname #{hostname}") + described_class.change_host_name(machine, hostname) + end + + it "does not change the hostname if already set" do + communicator.stub_command("sudo hostname | grep '#{hostname}'", exit_code: 0) + described_class.change_host_name(machine, hostname) + expect(communicator.received_commands.size).to eq(1) + end + end +end diff --git a/test/unit/plugins/guests/arch/cap/configure_networks_test.rb b/test/unit/plugins/guests/arch/cap/configure_networks_test.rb new file mode 100644 index 000000000..2f6de689e --- /dev/null +++ b/test/unit/plugins/guests/arch/cap/configure_networks_test.rb @@ -0,0 +1,48 @@ +require_relative "../../../../base" + +describe "VagrantPlugins::GuestArch::Cap::ConfigureNetworks" do + let(:described_class) do + VagrantPlugins::GuestArch::Plugin + .components + .guest_capabilities[:arch] + .get(:configure_networks) + end + + let(:machine) { double("machine") } + let(:communicator) { VagrantTests::DummyCommunicator::Communicator.new(machine) } + + before do + allow(machine).to receive(:communicate).and_return(communicator) + communicator.stub_command("ip -o -0 addr | grep -v LOOPBACK | awk '{print $2}' | sed 's/://'", + stdout: "eth1\neth2") + end + + after do + communicator.verify_expectations! + end + + describe ".configure_networks" do + let(:network_1) do + { + interface: 0, + type: "dhcp", + } + end + + let(:network_2) do + { + interface: 1, + type: "static", + ip: "33.33.33.10", + netmask: "255.255.0.0", + gateway: "33.33.0.1", + } + end + + it "creates and starts the networks" do + communicator.expect_command("ip link set eth1 down && netctl restart eth1 && netctl enable eth1") + communicator.expect_command("ip link set eth2 down && netctl restart eth2 && netctl enable eth2") + described_class.configure_networks(machine, [network_1, network_2]) + end + end +end diff --git a/test/unit/plugins/guests/coreos/cap/change_host_name_test.rb b/test/unit/plugins/guests/coreos/cap/change_host_name_test.rb new file mode 100644 index 000000000..d893384ab --- /dev/null +++ b/test/unit/plugins/guests/coreos/cap/change_host_name_test.rb @@ -0,0 +1,37 @@ +require_relative "../../../../base" + +describe "VagrantPlugins::GuestCoreOS::Cap::ChangeHostName" do + let(:described_class) do + VagrantPlugins::GuestCoreOS::Plugin + .components + .guest_capabilities[:coreos] + .get(:change_host_name) + end + + let(:machine) { double("machine") } + let(:communicator) { VagrantTests::DummyCommunicator::Communicator.new(machine) } + + before do + allow(machine).to receive(:communicate).and_return(communicator) + end + + after do + communicator.verify_expectations! + end + + describe ".change_host_name" do + let(:hostname) { "example.com" } + + it "sets the hostname" do + communicator.stub_command("sudo hostname --fqdn | grep '#{hostname}'", exit_code: 1) + communicator.expect_command("hostname example") + described_class.change_host_name(machine, hostname) + end + + it "does not change the hostname if already set" do + communicator.stub_command("sudo hostname --fqdn | grep '#{hostname}'", exit_code: 0) + described_class.change_host_name(machine, hostname) + expect(communicator.received_commands.size).to eq(1) + end + end +end diff --git a/test/unit/plugins/guests/debian/cap/change_host_name_test.rb b/test/unit/plugins/guests/debian/cap/change_host_name_test.rb index 76a1a48ba..ce18667ef 100644 --- a/test/unit/plugins/guests/debian/cap/change_host_name_test.rb +++ b/test/unit/plugins/guests/debian/cap/change_host_name_test.rb @@ -1,10 +1,14 @@ -require File.expand_path("../../../../../base", __FILE__) -require File.expand_path("../../../support/shared/debian_like_host_name_examples", __FILE__) +require_relative "../../../../base" +require_relative "../../support/shared/debian_like_host_name_examples" describe "VagrantPlugins::GuestDebian::Cap::ChangeHostName" do let(:described_class) do - VagrantPlugins::GuestDebian::Plugin.components.guest_capabilities[:debian].get(:change_host_name) + VagrantPlugins::GuestDebian::Plugin + .components + .guest_capabilities[:debian] + .get(:change_host_name) end + let(:machine) { double("machine") } let(:communicator) { VagrantTests::DummyCommunicator::Communicator.new(machine) } let(:old_hostname) { 'oldhostname.olddomain.tld' } diff --git a/test/unit/plugins/guests/debian/cap/configure_networks_test.rb b/test/unit/plugins/guests/debian/cap/configure_networks_test.rb new file mode 100644 index 000000000..51d717445 --- /dev/null +++ b/test/unit/plugins/guests/debian/cap/configure_networks_test.rb @@ -0,0 +1,50 @@ +require_relative "../../../../base" + +describe "VagrantPlugins::GuestDebian::Cap::ConfigureNetworks" do + let(:described_class) do + VagrantPlugins::GuestDebian::Plugin + .components + .guest_capabilities[:debian] + .get(:configure_networks) + end + + let(:machine) { double("machine") } + let(:communicator) { VagrantTests::DummyCommunicator::Communicator.new(machine) } + + before do + allow(machine).to receive(:communicate).and_return(communicator) + end + + after do + communicator.verify_expectations! + end + + describe ".configure_networks" do + let(:network_0) do + { + interface: 0, + type: "dhcp", + } + end + + let(:network_1) do + { + interface: 1, + type: "static", + ip: "33.33.33.10", + netmask: "255.255.0.0", + gateway: "33.33.0.1", + } + end + + it "creates and starts the networks" do + communicator.expect_command("/sbin/ifdown eth0 2> /dev/null || true") + communicator.expect_command("/sbin/ip addr flush dev eth0 2> /dev/null") + communicator.expect_command("/sbin/ifdown eth1 2> /dev/null || true") + communicator.expect_command("/sbin/ip addr flush dev eth1 2> /dev/null") + communicator.expect_command("/sbin/ifup eth0") + communicator.expect_command("/sbin/ifup eth1") + described_class.configure_networks(machine, [network_0, network_1]) + end + end +end From f95eb124d531259ce4e9fc2337fff3135064060b Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Sat, 28 May 2016 23:20:36 -0400 Subject: [PATCH 06/11] Use Util::Tempfile in Chef provisioner This also fixes some Windowsisms --- plugins/provisioners/chef/provisioner/base.rb | 78 ++++++++----------- 1 file changed, 31 insertions(+), 47 deletions(-) diff --git a/plugins/provisioners/chef/provisioner/base.rb b/plugins/provisioners/chef/provisioner/base.rb index 3f5ec7106..936e4f455 100644 --- a/plugins/provisioners/chef/provisioner/base.rb +++ b/plugins/provisioners/chef/provisioner/base.rb @@ -1,7 +1,6 @@ -require 'tempfile' - -require "vagrant/util/presence" -require "vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/presence" +require_relative "../../../../lib/vagrant/util/template_renderer" +require_relative "../../../../lib/vagrant/util/tempfile" require_relative "../installer" @@ -12,6 +11,7 @@ module VagrantPlugins # chef-solo and chef-client provisioning are stored. This is **not an actual # provisioner**. Instead, {ChefSolo} or {ChefServer} should be used. class Base < Vagrant.plugin("2", :provisioner) + include Vagrant::Util include Vagrant::Util::Presence class ChefError < Vagrant::Errors::VagrantError @@ -118,7 +118,7 @@ module VagrantPlugins @machine.communicate.upload(expanded, remote_custom_config_path) end - config_file = Vagrant::Util::TemplateRenderer.render(template, { + config_file = TemplateRenderer.render(template, { custom_configuration: remote_custom_config_path, encrypted_data_bag_secret: guest_encrypted_data_bag_secret_key_path, environment: @config.environment, @@ -138,16 +138,14 @@ module VagrantPlugins formatter: @config.formatter }.merge(template_vars)) - # Create a temporary file to store the data so we - # can upload it - temp = Tempfile.new("vagrant") - temp.write(config_file) - temp.close - + # Create a temporary file to store the data so we can upload it. remote_file = File.join(guest_provisioning_path, filename) - @machine.communicate.tap do |comm| - comm.sudo("rm -f #{remote_file}", error_check: false) - comm.upload(temp.path, remote_file) + @machine.communicate.sudo(remove_command(remote_file), error_check: false) + Tempfile.create("chef-provisioner-config") do |f| + f.write(config_file) + f.fsync + f.close + @machine.communicate.upload(f.path, remote_file) end end @@ -160,22 +158,14 @@ module VagrantPlugins !@config.run_list.empty? json = JSON.pretty_generate(json) - # Create a temporary file to store the data so we - # can upload it - temp = Tempfile.new("vagrant") - temp.write(json) - temp.close - + # Create a temporary file to store the data so we can upload it. remote_file = File.join(guest_provisioning_path, "dna.json") - @machine.communicate.tap do |comm| - if windows? - command = "if (test-path '#{remote_file}') {rm '#{remote_file}' -force -recurse}" - else - command = "rm -f #{remote_file}" - end - - comm.sudo(command, error_check: false) - comm.upload(temp.path, remote_file) + @machine.communicate.sudo(remove_command(remote_file), error_check: false) + Tempfile.create("chef-provisioner-config") do |f| + f.write(json) + f.fsync + f.close + @machine.communicate.upload(f.path, remote_file) end end @@ -186,29 +176,15 @@ module VagrantPlugins @machine.ui.info I18n.t( "vagrant.provisioners.chef.upload_encrypted_data_bag_secret_key") - @machine.communicate.tap do |comm| - if windows? - command = "if (test-path ""#{remote_file}"") {rm ""#{remote_file}"" -force -recurse}" - else - command = "rm -f #{remote_file}" - end - - comm.sudo(command, error_check: false) - comm.upload(encrypted_data_bag_secret_key_path, remote_file) - end + @machine.communicate.sudo(remove_command(remote_file), error_check: false) + @machine.communicate.upload(encrypted_data_bag_secret_key_path, remote_file) end def delete_encrypted_data_bag_secret remote_file = guest_encrypted_data_bag_secret_key_path - if remote_file - if windows? - command = "if (test-path ""#{remote_file}"") {rm ""#{remote_file}"" -force -recurse}" - else - command = "rm -f #{remote_file}" - end + return if remote_file.nil? - @machine.communicate.sudo(command, error_check: false) - end + @machine.communicate.sudo(remove_command(remote_file), error_check: false) end def encrypted_data_bag_secret_key_path @@ -258,6 +234,14 @@ module VagrantPlugins end end + def remove_command(path) + if windows? + "if (test-path ""#{path}"") {rm ""#{path}"" -force -recurse}" + else + "rm -f #{path}" + end + end + def windows? @machine.config.vm.communicator == :winrm end From ca337122dc4d9baac522e736a3f2c97248d6b6cf Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Sat, 28 May 2016 23:44:11 -0400 Subject: [PATCH 07/11] Fix test issues --- test/unit/vagrant/box_test.rb | 2 +- test/unit/vagrant/environment_test.rb | 2 +- test/unit/vagrant/machine_index_test.rb | 2 +- test/unit/vagrant/machine_test.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/unit/vagrant/box_test.rb b/test/unit/vagrant/box_test.rb index 8843e64b4..ab6c6c548 100644 --- a/test/unit/vagrant/box_test.rb +++ b/test/unit/vagrant/box_test.rb @@ -245,7 +245,7 @@ describe Vagrant::Box, :skip_windows do end after do - metadata_url.unlink if metadata_url.file? + metadata_url.unlink end it "loads the url and returns the data" do diff --git a/test/unit/vagrant/environment_test.rb b/test/unit/vagrant/environment_test.rb index b47ab7c66..8c1e3b06d 100644 --- a/test/unit/vagrant/environment_test.rb +++ b/test/unit/vagrant/environment_test.rb @@ -933,7 +933,7 @@ VF let(:instance) { described_class.new(local_data_path: local_data_path) } after do - File.unlink(v1_dotfile_tempfile) if File.file?(v1_dotfile_template) + File.unlink(v1_dotfile) if File.file?(v1_dotfile) end it "should be fine if dotfile is empty" do diff --git a/test/unit/vagrant/machine_index_test.rb b/test/unit/vagrant/machine_index_test.rb index 57ad2c951..1ea85110c 100644 --- a/test/unit/vagrant/machine_index_test.rb +++ b/test/unit/vagrant/machine_index_test.rb @@ -9,7 +9,7 @@ require "vagrant/machine_index" describe Vagrant::MachineIndex do include_context "unit" - let(:data_dir) { Dir.mktmpdir } + let(:data_dir) { temporary_dir } let(:entry_klass) { Vagrant::MachineIndex::Entry } let(:new_entry) do diff --git a/test/unit/vagrant/machine_test.rb b/test/unit/vagrant/machine_test.rb index 6cf35d6f7..3c15f7d87 100644 --- a/test/unit/vagrant/machine_test.rb +++ b/test/unit/vagrant/machine_test.rb @@ -43,7 +43,7 @@ describe Vagrant::Machine do let(:instance) { new_instance } after do - FileUtils.rm_rf(data_dir) + FileUtils.rm_rf(data_dir) if data_dir end subject { instance } From 982af05178b2b44dd3cdf38e2aeef4808cf46207 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Sat, 28 May 2016 23:53:20 -0400 Subject: [PATCH 08/11] Add a note about why we will always leak RDP tmpfiles --- plugins/hosts/darwin/cap/rdp.rb | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/plugins/hosts/darwin/cap/rdp.rb b/plugins/hosts/darwin/cap/rdp.rb index d3df31d05..3a02589d6 100644 --- a/plugins/hosts/darwin/cap/rdp.rb +++ b/plugins/hosts/darwin/cap/rdp.rb @@ -9,7 +9,18 @@ module VagrantPlugins class RDP def self.rdp_client(env, rdp_info) config_path = self.generate_config_file(rdp_info) - Vagrant::Util::Subprocess.execute("open", config_path.to_s) + begin + Vagrant::Util::Subprocess.execute("open", config_path.to_s) + ensure + # Note: this technically will never get run; neither would an + # at_exit call. The reason is that `exec` replaces this process, + # effectively the same as `kill -9`. This is solely here to prove + # that and so that future developers do not waste a ton of time + # try to identify why Vagrant is leaking RDP connection files. + # There is a catch-22 here in that we can't delete the file before + # we exec, and we can't delete the file after we exec :(. + File.unlink(config_path) if File.file?(config_path) + end end protected @@ -25,21 +36,24 @@ module VagrantPlugins } # Create the ".rdp" file - config_path = Pathname.new(Dir.tmpdir).join( - "vagrant-rdp-#{Time.now.to_i}-#{rand(10000)}.rdp") - config_path.open("w+") do |f| + t = ::Tempfile.new(["vagrant-rdp", ".rdp"]).tap do |f| + f.binmode + opts.each do |k, v| - f.puts("#{k}:#{v}") + f.write("#{k}:#{v}") end if rdp_info[:extra_args] rdp_info[:extra_args].each do |arg| - f.puts("#{arg}") + f.write("#{arg}") end end + + f.fsync + f.close end - return config_path + return t.path end end end From 1b414d3d9e2a4fdbb8a06fddf4bdeae6a7744ed2 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Sun, 29 May 2016 00:18:33 -0400 Subject: [PATCH 09/11] That file has to exist --- lib/vagrant/box.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/vagrant/box.rb b/lib/vagrant/box.rb index a6d6aa737..cd839ee91 100644 --- a/lib/vagrant/box.rb +++ b/lib/vagrant/box.rb @@ -118,7 +118,8 @@ module Vagrant # @param [Hash] download_options Options to pass to the downloader. # @return [BoxMetadata] def load_metadata(**download_options) - path = Dir::Tmpname.create("vagrant-load-metadata") {} + tf = Tempfile.new("vagrant-load-metadata") + tf.close url = @metadata_url if File.file?(url) || url !~ /^[a-z0-9]+:.*$/i @@ -128,13 +129,13 @@ module Vagrant end opts = { headers: ["Accept: application/json"] }.merge(download_options) - Util::Downloader.new(url, path, **opts).download! - BoxMetadata.new(File.open(path, "r")) + Util::Downloader.new(url, tf.path, **opts).download! + BoxMetadata.new(File.open(tf.path, "r")) rescue Errors::DownloaderError => e raise Errors::BoxMetadataDownloadError, message: e.extra_data[:message] ensure - File.unlink(path) if File.file?(path) + tf.unlink if tf end # Checks if the box has an update and returns the metadata, version, From b993699af6a028c170a5faf598da4eef28bcb03f Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Sun, 29 May 2016 00:34:00 -0400 Subject: [PATCH 10/11] Cleanup more files in tests --- .../plugins/providers/docker/config_test.rb | 8 ++++++-- test/unit/plugins/pushes/atlas/push_test.rb | 18 +++++++++++------- test/unit/plugins/pushes/ftp/push_test.rb | 3 ++- test/unit/vagrant/box_test.rb | 9 ++++++++- test/unit/vagrant/environment_test.rb | 2 +- test/unit/vagrant/machine_index_test.rb | 2 +- 6 files changed, 29 insertions(+), 13 deletions(-) diff --git a/test/unit/plugins/providers/docker/config_test.rb b/test/unit/plugins/providers/docker/config_test.rb index a938fcf45..5dc59bcc4 100644 --- a/test/unit/plugins/providers/docker/config_test.rb +++ b/test/unit/plugins/providers/docker/config_test.rb @@ -10,13 +10,17 @@ describe VagrantPlugins::DockerProvider::Config do let(:machine) { double("machine") } let(:build_dir) do - temporary_dir.tap do |dir| - dir.join("Dockerfile").open("w") do |f| + Dir.mktmpdir("vagrant-test-docker-provider-build-dir").tap do |dir| + File.open(File.join(dir, "Dockerfile"), "wb+") do |f| f.write("Hello") end end end + after do + FileUtils.rm_rf(build_dir) + end + def assert_invalid errors = subject.validate(machine) if !errors.values.any? { |v| !v.empty? } diff --git a/test/unit/plugins/pushes/atlas/push_test.rb b/test/unit/plugins/pushes/atlas/push_test.rb index 974df36a8..9c2465177 100644 --- a/test/unit/plugins/pushes/atlas/push_test.rb +++ b/test/unit/plugins/pushes/atlas/push_test.rb @@ -125,6 +125,14 @@ describe VagrantPlugins::AtlasPush::Push do end describe "#uploader_path" do + let(:scratch) do + Pathname.new(Dir.mktmpdir("vagrant-test-atlas-push-upload-path")) + end + + after do + FileUtils.rm_rf(scratch) + end + it "should return the configured path if set" do config.uploader_path = "foo" expect(subject.uploader_path).to eq("foo") @@ -141,12 +149,10 @@ describe VagrantPlugins::AtlasPush::Push do end it "should look up the uploader in the embedded dir if installer" do - dir = temporary_dir - allow(Vagrant).to receive(:in_installer?).and_return(true) - allow(Vagrant).to receive(:installer_embedded_dir).and_return(dir.to_s) + allow(Vagrant).to receive(:installer_embedded_dir).and_return(scratch.to_s) - bin_path = dir.join("bin", bin) + bin_path = scratch.join("bin", bin) bin_path.dirname.mkpath bin_path.open("w+") { |f| f.write("hi") } @@ -154,10 +160,8 @@ describe VagrantPlugins::AtlasPush::Push do end it "should look up the uploader in the PATH if not in the installer" do - dir = temporary_dir - allow(Vagrant).to receive(:in_installer?).and_return(true) - allow(Vagrant).to receive(:installer_embedded_dir).and_return(dir.to_s) + allow(Vagrant).to receive(:installer_embedded_dir).and_return(scratch.to_s) expect(Vagrant::Util::Which).to receive(:which). with(described_class.const_get(:UPLOADER_BIN)). diff --git a/test/unit/plugins/pushes/ftp/push_test.rb b/test/unit/plugins/pushes/ftp/push_test.rb index 96537cd42..ba69e0078 100644 --- a/test/unit/plugins/pushes/ftp/push_test.rb +++ b/test/unit/plugins/pushes/ftp/push_test.rb @@ -41,7 +41,7 @@ describe VagrantPlugins::FTPPush::Push do end @server.start - @dir = temporary_dir + @dir = Dir.mktmpdir("vagrant-ftp-push") FileUtils.touch("#{@dir}/.hidden.rb") FileUtils.touch("#{@dir}/application.rb") FileUtils.touch("#{@dir}/config.rb") @@ -51,6 +51,7 @@ describe VagrantPlugins::FTPPush::Push do end after(:all) do + FileUtils.rm_rf(@dir) @server.stop end diff --git a/test/unit/vagrant/box_test.rb b/test/unit/vagrant/box_test.rb index ab6c6c548..dfba490aa 100644 --- a/test/unit/vagrant/box_test.rb +++ b/test/unit/vagrant/box_test.rb @@ -290,6 +290,14 @@ describe Vagrant::Box, :skip_windows do end describe "repackaging" do + let(:scratch) { Dir.mktmpdir("vagrant-test-box-repackaging") } + + let(:box_output_path) { File.join(scratch, "package.box") } + + after do + FileUtils.rm_rf(scratch) + end + it "should repackage the box" do test_file_contents = "hello, world!" @@ -300,7 +308,6 @@ describe Vagrant::Box, :skip_windows do end # Repackage our box to some temporary directory - box_output_path = temporary_dir.join("package.box") expect(subject.repackage(box_output_path)).to be_true # Let's now add this box again under a different name, and then diff --git a/test/unit/vagrant/environment_test.rb b/test/unit/vagrant/environment_test.rb index 8c1e3b06d..ba3466c9b 100644 --- a/test/unit/vagrant/environment_test.rb +++ b/test/unit/vagrant/environment_test.rb @@ -933,7 +933,7 @@ VF let(:instance) { described_class.new(local_data_path: local_data_path) } after do - File.unlink(v1_dotfile) if File.file?(v1_dotfile) + FileUtils.rm_rf(local_data_path) end it "should be fine if dotfile is empty" do diff --git a/test/unit/vagrant/machine_index_test.rb b/test/unit/vagrant/machine_index_test.rb index 1ea85110c..ed52e9eaa 100644 --- a/test/unit/vagrant/machine_index_test.rb +++ b/test/unit/vagrant/machine_index_test.rb @@ -9,7 +9,7 @@ require "vagrant/machine_index" describe Vagrant::MachineIndex do include_context "unit" - let(:data_dir) { temporary_dir } + let(:data_dir) { Pathname.new(Dir.mktmpdir("vagrant-test-machine-index-data-dir")) } let(:entry_klass) { Vagrant::MachineIndex::Entry } let(:new_entry) do From 159e1ec1f12c821b73a8d45500a4545415e310f4 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Sun, 29 May 2016 00:41:59 -0400 Subject: [PATCH 11/11] Use a real file for bundler --- lib/vagrant/bundler.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/vagrant/bundler.rb b/lib/vagrant/bundler.rb index 13fe174d1..5ed499abf 100644 --- a/lib/vagrant/bundler.rb +++ b/lib/vagrant/bundler.rb @@ -254,13 +254,14 @@ module Vagrant def with_isolated_gem raise Errors::BundlerDisabled if !@enabled - tmp_gemfile = Dir::Tmpname.create("vagrant-gemfile") {} + tmp_gemfile = tempfile("vagrant-gemfile") + tmp_gemfile.close # Remove bundler settings so that Bundler isn't loaded when building # native extensions because it causes all sorts of problems. old_rubyopt = ENV["RUBYOPT"] old_gemfile = ENV["BUNDLE_GEMFILE"] - ENV["BUNDLE_GEMFILE"] = tmp_gemfile + ENV["BUNDLE_GEMFILE"] = tmp_gemfile.path ENV["RUBYOPT"] = (ENV["RUBYOPT"] || "").gsub(/-rbundler\/setup\s*/, "") # Set the GEM_HOME so gems are installed only to our local gem dir @@ -290,7 +291,7 @@ module Vagrant return yield end ensure - File.unlink(tmp_gemfile) if File.file?(tmp_gemfile) + tmp_gemfile.unlink rescue nil ENV["BUNDLE_GEMFILE"] = old_gemfile ENV["GEM_HOME"] = @gem_home