Merge branch 'config-validation-revamp'

This introduces a new configuration validation API and middleware builtin.
The big change here is that providers and provisioners are once again
validated. The API change itself is pretty minor, but important: machines
are passed into the validation instead of just the environment. This allows
validation to do a lot more, since it has access to the machine it actually
cares about.

lib/vagrant/action.rb

@@ -12,6 +12,7 @@ module Vagrant
       autoload :BoxAdd,  "vagrant/action/builtin/box_add"
       autoload :Call,    "vagrant/action/builtin/call"
       autoload :Confirm, "vagrant/action/builtin/confirm"
+      autoload :ConfigValidate, "vagrant/action/builtin/config_validate"
       autoload :EnvSet,  "vagrant/action/builtin/env_set"
       autoload :Provision, "vagrant/action/builtin/provision"
       autoload :SSHExec, "vagrant/action/builtin/ssh_exec"
@@ -20,7 +21,6 @@ module Vagrant
 
     module General
       autoload :Package,  'vagrant/action/general/package'
-      autoload :Validate, 'vagrant/action/general/validate'
     end
 
     # This is the action that will add a box from a URL. This middleware

lib/vagrant/action/builtin/config_validate.rb

@@ -0,0 +1,30 @@
+require "vagrant/util/template_renderer"
+
+module Vagrant
+  module Action
+    module Builtin
+      # This class validates the configuration and raises an exception
+      # if there are any validation errors.
+      class ConfigValidate
+        def initialize(app, env)
+          @app = app
+        end
+
+        def call(env)
+          if !env.has_key?(:config_validate) || env[:config_validate]
+            errors = env[:machine].config.validate(env[:machine])
+
+            if errors && !errors.empty?
+              raise Errors::ConfigInvalid,
+                :errors => Util::TemplateRenderer.render(
+                  "config/validation_failed",
+                  :errors => errors)
+            end
+          end
+
+          @app.call(env)
+        end
+      end
+    end
+  end
+end

lib/vagrant/action/general/validate.rb

@@ -1,21 +0,0 @@
-module Vagrant
-  module Action
-    module General
-      # Simply validates the configuration of the current Vagrant
-      # environment.
-      class Validate
-        def initialize(app, env)
-          @app = app
-        end
-
-        def call(env)
-          if !env.has_key?(:validate) || env[:validate]
-            env[:machine].config.validate!(env[:machine].env)
-          end
-
-          @app.call(env)
-        end
-      end
-    end
-  end
-end

lib/vagrant/config.rb

@@ -4,7 +4,6 @@ module Vagrant
   module Config
     autoload :Base,          'vagrant/config/base'
     autoload :Container,     'vagrant/config/container'
-    autoload :ErrorRecorder, 'vagrant/config/error_recorder'
     autoload :Loader,        'vagrant/config/loader'
     autoload :VersionBase,   'vagrant/config/version_base'
 

lib/vagrant/config/error_recorder.rb

@@ -1,19 +0,0 @@
-module Vagrant
-  module Config
-    # A class which is passed into the various {Base#validate} methods and
-    # can be used as a helper to add error messages about a single config
-    # class.
-    class ErrorRecorder
-      attr_reader :errors
-
-      def initialize
-        @errors = []
-      end
-
-      # Adds an error to the list of errors.
-      def add(message)
-        @errors << message
-      end
-    end
-  end
-end

lib/vagrant/config/v1/root.rb

@@ -39,26 +39,6 @@ module Vagrant
           end
         end
 
-        # Validates the configuration classes of this instance and raises an
-        # exception if they are invalid. If you are implementing a custom configuration
-        # class, the method you want to implement is {Base#validate}. This is
-        # the method that checks all the validation, not one which defines
-        # validation rules.
-        def validate!(env)
-          # Validate each of the configured classes and store the results into
-          # a hash.
-          errors = @keys.inject({}) do |container, data|
-            key, instance = data
-            recorder = ErrorRecorder.new
-            instance.validate(env, recorder)
-            container[key.to_sym] = recorder if !recorder.errors.empty?
-            container
-          end
-
-          return if errors.empty?
-          raise Errors::ConfigValidationFailed, :messages => Util::TemplateRenderer.render("config/validation_failed", :errors => errors)
-        end
-
         # Returns the internal state of the root object. This is used
         # by outside classes when merging, and shouldn't be called directly.
         # Note the strange method name is to attempt to avoid any name

lib/vagrant/config/v2/root.rb

@@ -1,3 +1,5 @@
+require "vagrant/config/v2/util"
+
 module Vagrant
   module Config
     module V2
@@ -39,24 +41,32 @@ module Vagrant
           end
         end
 
-        # Validates the configuration classes of this instance and raises an
+        # This validates the configuration and returns a hash of error
-        # exception if they are invalid. If you are implementing a custom configuration
+        # messages by section. If there are no errors, an empty hash
-        # class, the method you want to implement is {Base#validate}. This is
+        # is returned.
-        # the method that checks all the validation, not one which defines
+        #
-        # validation rules.
+        # @param [Environment] env
-        def validate!(env)
+        # @return [Hash]
-          # Validate each of the configured classes and store the results into
+        def validate(machine)
-          # a hash.
+          # Go through each of the configuration keys and validate
-          errors = @keys.inject({}) do |container, data|
+          errors = {}
-            key, instance = data
+          @keys.each do |_key, instance|
-            recorder = ErrorRecorder.new
+            if instance.respond_to?(:validate)
-            instance.validate(env, recorder)
+              # Validate this single item, and if we have errors then
-            container[key.to_sym] = recorder if !recorder.errors.empty?
+              # we merge them into our total errors list.
-            container
+              result = instance.validate(machine)
+              if result && !result.empty?
+                errors = Util.merge_errors(errors, result)
+              end
+            end
           end
 
-          return if errors.empty?
+          # Go through and delete empty keys
-          raise Errors::ConfigValidationFailed, :messages => Util::TemplateRenderer.render("config/validation_failed", :errors => errors)
+          errors.keys.each do |key|
+            errors.delete(key) if errors[key].empty?
+          end
+
+          errors
         end
 
         # Returns the internal state of the root object. This is used

lib/vagrant/config/v2/util.rb

@@ -0,0 +1,21 @@
+module Vagrant
+  module Config
+    module V2
+      class Util
+        # This merges two error hashes from validate methods.
+        #
+        # @param [Hash] first
+        # @param [Hash] second
+        # @return [Hash] Merged result
+        def self.merge_errors(first, second)
+          first.dup.tap do |result|
+            second.each do |key, value|
+              result[key] ||= []
+              result[key] += value
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/vagrant/errors.rb

@@ -150,14 +150,8 @@ module Vagrant
       error_key(:cli_invalid_options)
     end
 
-    class ConfigValidationFailed < VagrantError
+    class ConfigInvalid < VagrantError
-      status_code(42)
+      error_key(:config_invalid)
-      error_key(:config_validation)
-    end
-
-    class DeprecationError < VagrantError
-      status_code(60)
-      error_key(:deprecation)
     end
 
     class DestroyRequiresForce < VagrantError

lib/vagrant/plugin/v2/config.rb

@@ -90,12 +90,10 @@ module Vagrant
         # Called after the configuration is finalized and loaded to validate
         # this object.
         #
-        # @param [Environment] env Vagrant::Environment object of the
+        # @param [Machine] machine Access to the machine that is being
-        #   environment that this configuration has been loaded into. This
+        #   validated.
-        #   gives you convenient access to things like the the root path
+        # @return [Hash]
-        #   and so on.
+        def validate(machine)
-        # @param [ErrorRecorder] errors
-        def validate(env, errors)
         end
       end
     end

plugins/kernel_v2/config/ssh.rb

@@ -15,15 +15,21 @@ module VagrantPlugins
       attr_accessor :forward_x11
       attr_accessor :shell
 
-      def validate(env, errors)
+      def validate(machine)
+        errors = []
+
         [:username, :max_tries, :timeout].each do |field|
           value = instance_variable_get("@#{field}".to_sym)
-          errors.add(I18n.t("vagrant.config.common.error_empty", :field => field)) if !value
+          errors << I18n.t("vagrant.config.common.error_empty", :field => field) if !value
         end
 
-        if private_key_path && !File.file?(File.expand_path(private_key_path, env.root_path))
+        if private_key_path && \
-          errors.add(I18n.t("vagrant.config.ssh.private_key_missing", :path => private_key_path))
+          !File.file?(File.expand_path(private_key_path, machine.env.root_path))
-        end
+          errors << I18n.t("vagrant.config.ssh.private_key_missing", :path => private_key_path)
+        end
+
+        # Return the errors
+        { "ssh" => errors }
       end
     end
   end

plugins/kernel_v2/config/vm.rb

@@ -1,6 +1,7 @@
 require "pathname"
 
 require "vagrant"
+require "vagrant/config/v2/util"
 
 require File.expand_path("../vm_provider", __FILE__)
 require File.expand_path("../vm_provisioner", __FILE__)
@@ -122,26 +123,50 @@ module VagrantPlugins
         define(DEFAULT_VM_NAME) if defined_vm_keys.empty?
       end
 
-      def validate(env, errors)
+      def validate(machine)
-        errors.add(I18n.t("vagrant.config.vm.box_missing")) if !box
+        errors = []
-        errors.add(I18n.t("vagrant.config.vm.box_not_found", :name => box)) if box && !box_url && !env.boxes.find(box, :virtualbox)
+        errors << I18n.t("vagrant.config.vm.box_missing") if !box
-        errors.add(I18n.t("vagrant.config.vm.base_mac_invalid")) if env.boxes.find(box, :virtualbox) && !base_mac
+        errors << I18n.t("vagrant.config.vm.box_not_found", :name => box) if \
+          box && !box_url && !machine.box
 
         shared_folders.each do |name, options|
-          hostpath = Pathname.new(options[:hostpath]).expand_path(env.root_path)
+          hostpath = Pathname.new(options[:hostpath]).expand_path(machine.env.root_path)
 
           if !hostpath.directory? && !options[:create]
-            errors.add(I18n.t("vagrant.config.vm.shared_folder_hostpath_missing",
+            errors << I18n.t("vagrant.config.vm.shared_folder_hostpath_missing",
                        :name => name,
-                       :path => options[:hostpath]))
+                       :path => options[:hostpath])
           end
 
           if options[:nfs] && (options[:owner] || options[:group])
             # Owner/group don't work with NFS
-            errors.add(I18n.t("vagrant.config.vm.shared_folder_nfs_owner_group",
+            errors << I18n.t("vagrant.config.vm.shared_folder_nfs_owner_group",
-                              :name => name))
+                              :name => name)
-          end
+          end
-        end
+        end
+
+        # We're done with VM level errors so prepare the section
+        errors = { "vm" => errors }
+
+        # Validate only the _active_ provider
+        if machine.provider_config
+          provider_errors = machine.provider_config.validate(machine)
+          if provider_errors
+            errors = Vagrant::Config::V2::Util.merge_errors(errors, provider_errors)
+          end
+        end
+
+        # Validate provisioners
+        @provisioners.each do |vm_provisioner|
+          if vm_provisioner.config
+            provisioner_errors = vm_provisioner.config.validate(machine)
+            if provisioner_errors
+              errors = Vagrant::Config::V2::Util.merge_errors(errors, provisioner_errors)
+            end
+          end
+        end
+
+        errors
       end
     end
   end

plugins/providers/virtualbox/action.rb

@@ -85,7 +85,7 @@ module VagrantPlugins
 
             b2.use Call, DestroyConfirm do |env2, b3|
               if env2[:result]
-                b3.use Vagrant::Action::General::Validate
+                b3.use ConfigValidate
                 b3.use CheckAccessible
                 b3.use EnvSet, :force => true
                 b3.use action_halt
@@ -144,7 +144,7 @@ module VagrantPlugins
       def self.action_provision
         Vagrant::Action::Builder.new.tap do |b|
           b.use CheckVirtualbox
-          b.use Vagrant::Action::General::Validate
+          b.use ConfigValidate
           b.use Call, Created do |env1, b2|
             if !env1[:result]
               b2.use MessageNotCreated
@@ -176,7 +176,7 @@ module VagrantPlugins
               next
             end
 
-            b2.use Vagrant::Action::General::Validate
+            b2.use ConfigValidate
             b2.use action_halt
             b2.use action_start
           end
@@ -228,7 +228,7 @@ module VagrantPlugins
       def self.action_start
         Vagrant::Action::Builder.new.tap do |b|
           b.use CheckVirtualbox
-          b.use Vagrant::Action::General::Validate
+          b.use ConfigValidate
           b.use Call, IsRunning do |env, b2|
             # If the VM is running, then our work here is done, exit
             next if env[:result]
@@ -268,7 +268,7 @@ module VagrantPlugins
       def self.action_up
         Vagrant::Action::Builder.new.tap do |b|
           b.use CheckVirtualbox
-          b.use Vagrant::Action::General::Validate
+          b.use ConfigValidate
           b.use Call, Created do |env, b2|
             # If the VM is NOT created yet, then do the setup steps
             if !env[:result]

plugins/providers/virtualbox/action/import.rb

@@ -40,7 +40,7 @@ module VagrantPlugins
             # validate the configuration here, and we don't want to confirm
             # we want to destroy.
             destroy_env = env.clone
-            destroy_env[:validate] = false
+            destroy_env[:config_validate] = false
             destroy_env[:force_confirm_destroy] = true
             env[:action_runner].run(Action.action_destroy, destroy_env)
           end

plugins/provisioners/chef/config/base.rb

@@ -52,12 +52,6 @@ module VagrantPlugins
             run_list << name
         end
 
-        def validate(env, errors)
-          super
-
-          errors.add(I18n.t("vagrant.config.chef.vagrant_as_json_key")) if json.has_key?(:vagrant)
-        end
-
         def instance_variables_hash
           # Overridden so that the 'json' key could be removed, since its just
           # merged into the config anyways

plugins/provisioners/chef/config/chef_client.rb

@@ -22,12 +22,16 @@ module VagrantPlugins
         def file_backup_path; @file_backup_path || "/srv/chef/cache"; end
         def encrypted_data_bag_secret; @encrypted_data_bag_secret || "/tmp/encrypted_data_bag_secret"; end
 
-        def validate(env, errors)
+        def validate(machine)
-          super
+          errors = []
+          errors << I18n.t("vagrant.config.chef.server_url_empty") if \
+            !chef_server_url || chef_server_url.strip == ""
+          errors << I18n.t("vagrant.config.chef.validation_key_path") if \
+            !validation_key_path
+          errors << I18n.t("vagrant.config.chef.run_list_empty") if \
+            @run_list && @run_list.empty?
 
-          errors.add(I18n.t("vagrant.config.chef.server_url_empty")) if !chef_server_url || chef_server_url.strip == ""
+          { "chef client provisioner" => errors }
-          errors.add(I18n.t("vagrant.config.chef.validation_key_path")) if !validation_key_path
-          errors.add(I18n.t("vagrant.config.chef.run_list_empty")) if @run_list && @run_list.empty?
         end
       end
     end

plugins/provisioners/chef/config/chef_solo.rb

@@ -37,11 +37,14 @@ module VagrantPlugins
           @nfs || false
         end
 
-        def validate(env, errors)
+        def validate(machine)
-          super
+          errors = []
+          errors << I18n.t("vagrant.config.chef.cookbooks_path_empty") if \
+            !cookbooks_path || [cookbooks_path].flatten.empty?
+          errors << I18n.t("vagrant.config.chef.run_list_empty") if \
+            !run_list || run_list.empty?
 
-          errors.add(I18n.t("vagrant.config.chef.cookbooks_path_empty")) if !cookbooks_path || [cookbooks_path].flatten.empty?
+          { "chef solo provisioner" => errors }
-          errors.add(I18n.t("vagrant.config.chef.run_list_empty")) if !run_list || run_list.empty?
         end
       end
     end

plugins/provisioners/puppet/config/puppet.rb

@@ -35,29 +35,34 @@ module VagrantPlugins
           end
         end
 
-        def validate(env, errors)
+        def validate(machine)
+          errors = []
+
           # Calculate the manifests and module paths based on env
-          this_expanded_manifests_path = expanded_manifests_path(env.root_path)
+          this_expanded_manifests_path = expanded_manifests_path(machine.env.root_path)
-          this_expanded_module_paths = expanded_module_paths(env.root_path)
+          this_expanded_module_paths = expanded_module_paths(machine.env.root_path)
 
           # Manifests path/file validation
           if !this_expanded_manifests_path.directory?
-            errors.add(I18n.t("vagrant.provisioners.puppet.manifests_path_missing",
+            errors << I18n.t("vagrant.provisioners.puppet.manifests_path_missing",
-                              :path => this_expanded_manifests_path))
+                             :path => this_expanded_manifests_path)
           else
             expanded_manifest_file = this_expanded_manifests_path.join(manifest_file)
             if !expanded_manifest_file.file?
-              errors.add(I18n.t("vagrant.provisioners.puppet.manifest_missing",
+              errors << I18n.t("vagrant.provisioners.puppet.manifest_missing",
-                                :manifest => expanded_manifest_file.to_s))
+                               :manifest => expanded_manifest_file.to_s)
             end
           end
 
           # Module paths validation
           this_expanded_module_paths.each do |path|
             if !path.directory?
-              errors.add(I18n.t("vagrant.provisioners.puppet.module_path_missing", :path => path))
+              errors << I18n.t("vagrant.provisioners.puppet.module_path_missing",
-            end
+                               :path => path)
-          end
+            end
+          end
+
+          { "puppet provisioner" => errors }
         end
       end
     end

plugins/provisioners/shell/config.rb

@@ -10,32 +10,36 @@ module VagrantPlugins
         @upload_path = "/tmp/vagrant-shell"
       end
 
-      def validate(env, errors)
+      def validate(machine)
+        errors = []
+
         # Validate that the parameters are properly set
         if path && inline
-          errors.add(I18n.t("vagrant.provisioners.shell.path_and_inline_set"))
+          errors << I18n.t("vagrant.provisioners.shell.path_and_inline_set")
         elsif !path && !inline
-          errors.add(I18n.t("vagrant.provisioners.shell.no_path_or_inline"))
+          errors << I18n.t("vagrant.provisioners.shell.no_path_or_inline")
         end
 
         # Validate the existence of a script to upload
         if path
-          expanded_path = Pathname.new(path).expand_path(env.root_path)
+          expanded_path = Pathname.new(path).expand_path(machine.env.root_path)
           if !expanded_path.file?
-            errors.add(I18n.t("vagrant.provisioners.shell.path_invalid",
+            errors << I18n.t("vagrant.provisioners.shell.path_invalid",
-                              :path => expanded_path))
+                              :path => expanded_path)
           end
         end
 
         # There needs to be a path to upload the script to
         if !upload_path
-          errors.add(I18n.t("vagrant.provisioners.shell.upload_path_not_set"))
+          errors << I18n.t("vagrant.provisioners.shell.upload_path_not_set")
         end
 
         # If there are args and its not a string, that is a problem
         if args && !args.is_a?(String)
-          errors.add(I18n.t("vagrant.provisioners.shell.args_not_string"))
+          errors << I18n.t("vagrant.provisioners.shell.args_not_string")
-        end
+        end
+
+        { "shell provisioner" => errors }
       end
     end
   end

templates/config/validation_failed.erb

@@ -1,6 +1,6 @@
-<% errors.each do |key, container| -%>
+<% errors.each do |section, list| -%>
-<%= key %>:
+<%= section %>:
-<% container.errors.each do |error| -%>
+<% list.each do |error| -%>
 * <%= error %>
 <% end -%>
 

templates/locales/en.yml

@@ -56,16 +56,11 @@ en:
         available below.
 
         %{help}
-      config_validation: |-
+      config_invalid: |-
-        There was a problem with the configuration of Vagrant. The error message(s)
+        There are errors in the configuration of this machine. Please fix
-        are printed below:
+        the following errors and try again:
 
-        %{messages}
+        %{errors}
-      deprecation: |-
-        You are using a feature that has been removed in this version. Explanation:
-
-        %{message}
-        Note that this error message will not appear in the next version of Vagrant.
       destroy_requires_force: |-
         Destroy doesn't have a TTY to ask for confirmation. Please pass the
         `--force` flag to force a destroy, otherwise attach a TTY so that
@@ -316,13 +311,11 @@ en:
 #-------------------------------------------------------------------------------
     config:
       common:
-        error_empty: "`%{field}` must be filled in."
+        error_empty: "`%{field}` must be not be empty."
       chef:
         cookbooks_path_empty: "Must specify a cookbooks path for chef solo."
         run_list_empty: "Run list must not be empty."
         server_url_empty: "Chef server URL must be populated."
-        vagrant_as_json_key: |-
-          `vagrant` cannot be a JSON key, because it is used by Vagrant already.
         validation_key_path: "Validation key path must be valid path to your chef server validation key."
       ssh:
         private_key_missing: "`private_key_path` file must exist: %{path}"

test/unit/vagrant/config/v2/root_test.rb

@@ -31,4 +31,58 @@ describe Vagrant::Config::V2::Root do
       "keys"       => {}
     }
   end
+
+  describe "validation" do
+    let(:instance) do
+      map = { :foo => Object, :bar => Object }
+      described_class.new(map)
+    end
+
+    it "should return nil if valid" do
+      instance.validate({}).should == {}
+    end
+
+    it "should return errors if invalid" do
+      errors = { "foo" => ["errors!"] }
+      env    = { "errors" => errors }
+      foo    = instance.foo
+      def foo.validate(env)
+        env["errors"]
+      end
+
+      instance.validate(env).should == errors
+    end
+
+    it "should merge errors via array concat if matching keys" do
+      errors = { "foo" => ["errors!"] }
+      env    = { "errors" => errors }
+      foo    = instance.foo
+      bar    = instance.bar
+      def foo.validate(env)
+        env["errors"]
+      end
+
+      def bar.validate(env)
+        env["errors"].merge({ "bar" => ["bar"] })
+      end
+
+      expected_errors = {
+        "foo" => ["errors!", "errors!"],
+        "bar" => ["bar"]
+      }
+
+      instance.validate(env).should == expected_errors
+    end
+
+    it "shouldn't count empty keys" do
+      errors = { "foo" => [] }
+      env    = { "errors" => errors }
+      foo    = instance.foo
+      def foo.validate(env)
+        env["errors"]
+      end
+
+      instance.validate(env).should == {}
+    end
+  end
 end

test/unit/vagrant/config/v2/util_test.rb

@@ -0,0 +1,21 @@
+require File.expand_path("../../../../base", __FILE__)
+
+require "vagrant/config/v2/util"
+
+describe Vagrant::Config::V2::Util do
+  describe "merging errors" do
+    it "should merge matching keys and leave the rest alone" do
+      first  = { "one" => ["foo"], "two" => ["two"] }
+      second = { "one" => ["bar"], "three" => ["three"] }
+
+      expected = {
+        "one" => ["foo", "bar"],
+        "two" => ["two"],
+        "three" => ["three"]
+      }
+
+      result = described_class.merge_errors(first, second)
+      result.should == expected
+    end
+  end
+end