From efdb148f61879b838d3725644d78a1221d6ef5f4 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Fri, 27 May 2016 16:05:50 -0400 Subject: [PATCH] Move pre-flight package validation to middleware This commit separates the scratch and output directory creation from the main package middleware into its own PackageSetupFolders middleware. Additionally, the validation that ensures an output file does not exist is moved into a validation function that can be shared across multiple methods. This refactor permits a pre-flight check to ensure box packaging would be successful before actually stopping the VM. Fixes GH-7351 --- lib/vagrant/action/general/package.rb | 59 +++++++++++++++---- plugins/providers/virtualbox/action.rb | 9 ++- .../providers/virtualbox/action/package.rb | 30 +--------- .../virtualbox/action/package_setup_files.rb | 51 ++++++++++++++++ .../action/package_setup_folders.rb | 38 ++++++++++++ .../virtualbox/action/setup_package_files.rb | 52 +++------------- templates/locales/en.yml | 13 ++-- 7 files changed, 159 insertions(+), 93 deletions(-) create mode 100644 plugins/providers/virtualbox/action/package_setup_files.rb create mode 100644 plugins/providers/virtualbox/action/package_setup_folders.rb diff --git a/lib/vagrant/action/general/package.rb b/lib/vagrant/action/general/package.rb index beb6f05fc..ee9904e89 100644 --- a/lib/vagrant/action/general/package.rb +++ b/lib/vagrant/action/general/package.rb @@ -20,25 +20,63 @@ module Vagrant class Package include Util + # Perform sanity validations that the provided output filepath is sane. + # In particular, this function validates: + # + # - The output path is a regular file (not a directory or symlink) + # - No file currently exists at the given path + # - A directory of package files was actually provided (internal) + # + # @param [String] output path to the output file + # @param [String] directory path to a directory containing the files + def self.validate!(output, directory) + filename = File.basename(output.to_s) + output = fullpath(output) + + if File.directory?(output) + raise Vagrant::Errors::PackageOutputDirectory + end + + if File.exist?(output) + raise Vagrant::Errors::PackageOutputExists, filename: filename + end + + if !Vagrant::Util::Presence.present?(directory) || !File.directory?(directory) + raise Vagrant::Errors::PackageRequiresDirectory + end + end + + # Calculate the full path of the given path, relative to the current + # working directory (where the command was run). + # + # @param [String] output the relative path + def self.fullpath(output) + File.expand_path(output, Dir.pwd) + end + + # The path to the final output file. + # @return [String] + attr_reader :fullpath + def initialize(app, env) @app = app env["package.files"] ||= {} env["package.output"] ||= "package.box" + + @fullpath = self.class.fullpath(env["package.output"]) end def call(env) @env = env - file_name = File.basename(@env["package.output"].to_s) - raise Errors::PackageOutputDirectory if File.directory?(tar_path) - raise Errors::PackageOutputExists, file_name:file_name if File.exist?(tar_path) - raise Errors::PackageRequiresDirectory if !env["package.directory"] || - !File.directory?(env["package.directory"]) + self.class.validate!(env["package.output"], env["package.directory"]) + + raise Errors::PackageOutputDirectory if File.directory?(fullpath) @app.call(env) - @env[:ui].info I18n.t("vagrant.actions.general.package.compressing", tar_path: tar_path) + @env[:ui].info I18n.t("vagrant.actions.general.package.compressing", fullpath: fullpath) copy_include_files setup_private_key compress @@ -54,7 +92,7 @@ module Vagrant end # Cleanup any packaged files if the packaging failed at some point. - File.delete(tar_path) if File.exist?(tar_path) + File.delete(fullpath) if File.exist?(fullpath) end # This method copies the include files (passed in via command line) @@ -88,7 +126,7 @@ module Vagrant def compress # Get the output path. We have to do this up here so that the # pwd returns the proper thing. - output_path = tar_path.to_s + output_path = fullpath.to_s # Switch into that directory and package everything up Util::SafeChdir.safe_chdir(@env["package.directory"]) do @@ -147,11 +185,6 @@ module Vagrant f.puts %Q[end] end end - - # Path to the final box output file - def tar_path - File.expand_path(@env["package.output"], FileUtils.pwd) - end end end end diff --git a/plugins/providers/virtualbox/action.rb b/plugins/providers/virtualbox/action.rb index 7da0c06ae..f43846191 100644 --- a/plugins/providers/virtualbox/action.rb +++ b/plugins/providers/virtualbox/action.rb @@ -33,6 +33,8 @@ module VagrantPlugins autoload :Network, File.expand_path("../action/network", __FILE__) autoload :NetworkFixIPv6, File.expand_path("../action/network_fix_ipv6", __FILE__) autoload :Package, File.expand_path("../action/package", __FILE__) + autoload :PackageSetupFiles, File.expand_path("../action/package_setup_files", __FILE__) + autoload :PackageSetupFolders, File.expand_path("../action/package_setup_folders", __FILE__) autoload :PackageVagrantfile, File.expand_path("../action/package_vagrantfile", __FILE__) autoload :PrepareCloneSnapshot, File.expand_path("../action/prepare_clone_snapshot", __FILE__) autoload :PrepareNFSSettings, File.expand_path("../action/prepare_nfs_settings", __FILE__) @@ -41,12 +43,14 @@ module VagrantPlugins autoload :Resume, File.expand_path("../action/resume", __FILE__) autoload :SaneDefaults, File.expand_path("../action/sane_defaults", __FILE__) autoload :SetName, File.expand_path("../action/set_name", __FILE__) - autoload :SetupPackageFiles, File.expand_path("../action/setup_package_files", __FILE__) autoload :SnapshotDelete, File.expand_path("../action/snapshot_delete", __FILE__) autoload :SnapshotRestore, File.expand_path("../action/snapshot_restore", __FILE__) autoload :SnapshotSave, File.expand_path("../action/snapshot_save", __FILE__) autoload :Suspend, File.expand_path("../action/suspend", __FILE__) + # @deprecated use {PackageSetupFiles} instead + autoload :SetupPackageFiles, File.expand_path("../action/setup_package_files", __FILE__) + # Include the built-in modules so that we can use them as top-level # things. include Vagrant::Action::Builtin @@ -150,7 +154,8 @@ module VagrantPlugins next end - b2.use SetupPackageFiles + b2.use PackageSetupFolders + b2.use PackageSetupFiles b2.use CheckAccessible b2.use action_halt b2.use ClearForwardedPorts diff --git a/plugins/providers/virtualbox/action/package.rb b/plugins/providers/virtualbox/action/package.rb index 62a564ac9..fb5ebdabf 100644 --- a/plugins/providers/virtualbox/action/package.rb +++ b/plugins/providers/virtualbox/action/package.rb @@ -1,6 +1,4 @@ -require 'fileutils' - -require 'vagrant/action/general/package' +require_relative "../../../../lib/vagrant/action/general/package" module VagrantPlugins module ProviderVirtualBox @@ -10,33 +8,7 @@ module VagrantPlugins # called in the unit tests. alias_method :general_call, :call def call(env) - # Setup the temporary directory - @temp_dir = env[:tmp_path].join(Time.now.to_i.to_s) - env["export.temp_dir"] = @temp_dir - FileUtils.mkpath(env["export.temp_dir"]) - - # Just match up a couple environmental variables so that - # the superclass will do the right thing. Then, call the - # superclass - env["package.directory"] = env["export.temp_dir"] - general_call(env) - - # Always call recover to clean up the temp dir - clean_temp_dir - end - - def recover(env) - clean_temp_dir - super - end - - protected - - def clean_temp_dir - if @temp_dir && File.exist?(@temp_dir) - FileUtils.rm_rf(@temp_dir) - end end end end diff --git a/plugins/providers/virtualbox/action/package_setup_files.rb b/plugins/providers/virtualbox/action/package_setup_files.rb new file mode 100644 index 000000000..a70c7751a --- /dev/null +++ b/plugins/providers/virtualbox/action/package_setup_files.rb @@ -0,0 +1,51 @@ +module VagrantPlugins + module ProviderVirtualBox + module Action + class PackageSetupFiles + def initialize(app, env) + @app = app + + env["package.include"] ||= [] + env["package.vagrantfile"] ||= nil + end + + def call(env) + files = {} + env["package.include"].each do |file| + source = Pathname.new(file) + dest = nil + + # If the source is relative then we add the file as-is to the include + # directory. Otherwise, we copy only the file into the root of the + # include directory. Kind of strange, but seems to match what people + # expect based on history. + if source.relative? + dest = source + else + dest = source.basename + end + + # Assign the mapping + files[file] = dest + end + + if env["package.vagrantfile"] + # Vagrantfiles are treated special and mapped to a specific file + files[env["package.vagrantfile"]] = "_Vagrantfile" + end + + # Verify the mapping + files.each do |from, _| + raise Vagrant::Errors::PackageIncludeMissing, + file: from if !File.exist?(from) + end + + # Save the mapping + env["package.files"] = files + + @app.call(env) + end + end + end + end +end diff --git a/plugins/providers/virtualbox/action/package_setup_folders.rb b/plugins/providers/virtualbox/action/package_setup_folders.rb new file mode 100644 index 000000000..a045122b7 --- /dev/null +++ b/plugins/providers/virtualbox/action/package_setup_folders.rb @@ -0,0 +1,38 @@ +require "fileutils" + +require_relative "../../../../lib/vagrant/action/general/package" + +module VagrantPlugins + module ProviderVirtualBox + module Action + class PackageSetupFolders + include Vagrant::Util::Presence + + def initialize(app, env) + @app = app + end + + def call(env) + env["package.output"] ||= "package.box" + env["package.directory"] ||= Dir.mktmpdir("package-", env[:tmp_path]) + + # Match up a couple environmental variables so that the other parts of + # Vagrant will do the right thing. + env["export.temp_dir"] = env["package.directory"] + + Vagrant::Action::General::Package.validate!( + env["package.output"], env["package.directory"]) + + @app.call(env) + end + + def recover(env) + dir = env["package.directory"] + if File.exist?(dir) + FileUtils.rm_rf(dir) + end + end + end + end + end +end diff --git a/plugins/providers/virtualbox/action/setup_package_files.rb b/plugins/providers/virtualbox/action/setup_package_files.rb index 1654393c4..d7bc93da3 100644 --- a/plugins/providers/virtualbox/action/setup_package_files.rb +++ b/plugins/providers/virtualbox/action/setup_package_files.rb @@ -1,49 +1,15 @@ +require "log4r" + +require_relative "package_setup_files" + module VagrantPlugins module ProviderVirtualBox module Action - class SetupPackageFiles - def initialize(app, env) - @app = app - - env["package.include"] ||= [] - env["package.vagrantfile"] ||= nil - end - - def call(env) - files = {} - env["package.include"].each do |file| - source = Pathname.new(file) - dest = nil - - # If the source is relative then we add the file as-is to the include - # directory. Otherwise, we copy only the file into the root of the - # include directory. Kind of strange, but seems to match what people - # expect based on history. - if source.relative? - dest = source - else - dest = source.basename - end - - # Assign the mapping - files[file] = dest - end - - if env["package.vagrantfile"] - # Vagrantfiles are treated special and mapped to a specific file - files[env["package.vagrantfile"]] = "_Vagrantfile" - end - - # Verify the mapping - files.each do |from, _| - raise Vagrant::Errors::PackageIncludeMissing, - file: from if !File.exist?(from) - end - - # Save the mapping - env["package.files"] = files - - @app.call(env) + class SetupPackageFiles < PackageSetupFiles + def initialize(*) + @logger = Log4r::Logger.new("vagrant::plugins::virtualbox::setup_package_files") + @logger.warn { "SetupPackageFiles has been renamed to PackageSetupFiles" } + super end end end diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 481e9b754..7a887e133 100755 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -1734,10 +1734,10 @@ en: Sometimes, Vagrant will attempt to auto-correct this for you. In this case, Vagrant was unable to. This is usually because the guest machine is in a state which doesn't allow modifying port forwarding. You could - try 'vagrant reload' (equivalent of running a halt followed by an up) - so vagrant can attempt to auto-correct this upon booting. Be warned + try 'vagrant reload' (equivalent of running a halt followed by an up) + so vagrant can attempt to auto-correct this upon booting. Be warned that any unsaved work might be lost. - + fixed_collision: |- Fixed port collision for %{guest_port} => %{host_port}. Now on port %{new_port}. forwarding: Forwarding ports... @@ -1883,16 +1883,17 @@ en: general: package: packaging: "Packaging additional file: %{file}" - compressing: "Compressing package to: %{tar_path}" + compressing: "Compressing package to: %{fullpath}" output_exists: |- - The specified file '%{file_name}' to save the package as already exists. Please + The specified file '%{filename}' to save the package as already exists. Please remove this file or specify a different file name for outputting. output_is_directory: |- The specified output is a directory. Please specify a path including a filename. requires_directory: |- A directory was not specified to package. This should never happen - and is a result of an internal inconsistency. + and is a result of an internal inconsistency. Please report a bug + on the Vagrant bug tracker. include_file_missing: |- Package include file doesn't exist: %{file}