Merge pull request #7353 from mitchellh/sethvargo/package_preflight

Move pre-flight package validation to middleware

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)
+          self.class.validate!(env["package.output"], env["package.directory"])
-          raise Errors::PackageOutputExists, file_name:file_name if File.exist?(tar_path)
+
-          raise Errors::PackageRequiresDirectory if !env["package.directory"] ||
+          raise Errors::PackageOutputDirectory if File.directory?(fullpath)
-            !File.directory?(env["package.directory"])
 
           @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

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

plugins/providers/virtualbox/action/package.rb

@@ -1,6 +1,4 @@
-require 'fileutils'
+require_relative "../../../../lib/vagrant/action/general/package"
-
-require '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

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

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

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
+      class SetupPackageFiles < PackageSetupFiles
-        def initialize(app, env)
+        def initialize(*)
-          @app = app
+          @logger = Log4r::Logger.new("vagrant::plugins::virtualbox::setup_package_files")
-
+          @logger.warn { "SetupPackageFiles has been renamed to PackageSetupFiles" }
-          env["package.include"] ||= []
+          super
-          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

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) 
+            try 'vagrant reload' (equivalent of running a halt followed by an up)
-            so vagrant can attempt to auto-correct this upon booting. Be warned 
+            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}