From a7ee56459b2c990550c4fc33883c55cf4145f463 Mon Sep 17 00:00:00 2001 From: Gilles Cornu Date: Wed, 1 Jun 2016 00:30:07 +0200 Subject: [PATCH] provisioners/ansible(both): fix ansible config files presence checks With this change, the presence of Ansible configuration files (like playbook file, inventory path, galaxy role file, etc.) is no longer performed by the `config` classes, but by the `provisioner` classes (at the beginning of the provision command). This change fixes several issues: - Resolve #6984 as `provision` method are only executed when remote (ssh) communication with the guest machine is possible. - Resolve #6763 in a better way than 4e451c6 initially did. - Improve the general provisioner speed since the `config` checks are actually triggered by many vagrant actions (e.g. `destroy`,...), and can also be triggered multiple times during a vagrant run (e.g. on callback request made by the machine provider). Unlike the former `config`-based checks, the provision action won't collect all the invalid options, but only report the first invalid option found and abort the execution. Some unit tests were not implemented yet to save my scarce "open source contribution time" for other important issues, but they should be done at last via GH-6633. --- CHANGELOG.md | 6 +- plugins/provisioners/ansible/config/base.rb | 27 +---- plugins/provisioners/ansible/config/guest.rb | 26 ----- plugins/provisioners/ansible/config/host.rb | 21 ---- .../provisioners/ansible/provisioner/base.rb | 15 ++- .../provisioners/ansible/provisioner/guest.rb | 26 +++++ .../provisioners/ansible/provisioner/host.rb | 23 ++++ templates/locales/en.yml | 14 +-- .../provisioners/ansible/config/guest_test.rb | 102 +----------------- .../provisioners/ansible/config/host_test.rb | 64 ----------- .../provisioners/ansible/config/shared.rb | 55 ++++------ .../provisioners/ansible/provisioner_test.rb | 39 +++++++ 12 files changed, 133 insertions(+), 285 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bf0deb2e..a28e820a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -109,10 +109,8 @@ BUG FIXES: galaxy resources when running on a Windows host [GH-6740, GH-6757] - provisioners/ansible_local: Change the way to verify `ansible-galaxy` presence, to avoid a non-zero status code with Ansible 2.0 [GH-6793] - - provisioners/ansible_local: The configuration sanity checks now only warn - on missing files or directories, so that the requested vagrant command is - always executed (e.g. `vagrant destroy` is not aborted when the configured - playbook is not present on the guest) [GH-6763] + - provisioners/ansible(both provisioners): The Ansible configuration files + detection is only executed by the `provision` action [GH-6763, GH-6984] - provisioners/chef: Do not use double sudo when installing [GGH-6805, GH-6804] - provisioners/chef: Change the default channel to "stable" (previously it diff --git a/plugins/provisioners/ansible/config/base.rb b/plugins/provisioners/ansible/config/base.rb index 00b3f5072..67642467e 100644 --- a/plugins/provisioners/ansible/config/base.rb +++ b/plugins/provisioners/ansible/config/base.rb @@ -74,34 +74,15 @@ module VagrantPlugins @errors << I18n.t("vagrant.provisioners.ansible.errors.no_playbook") end - if playbook - check_path_is_a_file(machine, playbook, "vagrant.provisioners.ansible.errors.playbook_path_invalid") - end - - if inventory_path - check_path_exists(machine, inventory_path, "vagrant.provisioners.ansible.errors.inventory_path_invalid") - end - - if galaxy_role_file - check_path_is_a_file(machine, galaxy_role_file, "vagrant.provisioners.ansible.errors.galaxy_role_file_invalid") - end - - if vault_password_file - check_path_is_a_file(machine, vault_password_file, "vagrant.provisioners.ansible.errors.vault_password_file_invalid") - end - - # Validate that extra_vars is either a hash, or a path to an existing file + # Validate that extra_vars is either a Hash or a String (for a file path) if extra_vars extra_vars_is_valid = extra_vars.kind_of?(Hash) || extra_vars.kind_of?(String) if extra_vars.kind_of?(String) - # Accept the usage of '@' prefix in Vagrantfile (e.g. '@vars.yml' - # and 'vars.yml' are both supported) + # Accept the usage of '@' prefix in Vagrantfile + # (e.g. '@vars.yml' and 'vars.yml' are both supported) match_data = /^@?(.+)$/.match(extra_vars) extra_vars_path = match_data[1].to_s - extra_vars_is_valid = check_path_is_a_file(machine, extra_vars_path) - if extra_vars_is_valid - @extra_vars = '@' + extra_vars_path - end + @extra_vars = '@' + extra_vars_path end if !extra_vars_is_valid diff --git a/plugins/provisioners/ansible/config/guest.rb b/plugins/provisioners/ansible/config/guest.rb index 31507ad04..5f0086c4a 100644 --- a/plugins/provisioners/ansible/config/guest.rb +++ b/plugins/provisioners/ansible/config/guest.rb @@ -35,32 +35,6 @@ module VagrantPlugins { "ansible local provisioner" => @errors } end - protected - - def check_path(machine, path, test_args, error_message_key = nil) - remote_path = Helpers::expand_path_in_unix_style(path, @provisioning_path) - - if machine.communicate.ready? && !machine.communicate.test("test #{test_args} #{remote_path}") - if error_message_key - # only show warnings, as raising an error would abort the request - # vagrant action (e.g. prevent `destroy` to be executed) - machine.ui.warn(I18n.t(error_message_key, path: remote_path, system: "guest")) - end - return false - end - # when the machine is not ready for SSH communication, - # the check is "optimistically" bypassed. - true - end - - def check_path_is_a_file(machine, path, error_message_key = nil) - check_path(machine, path, "-f", error_message_key) - end - - def check_path_exists(machine, path, error_message_key = nil) - check_path(machine, path, "-e", error_message_key) - end - end end end diff --git a/plugins/provisioners/ansible/config/host.rb b/plugins/provisioners/ansible/config/host.rb index 04d2fac10..4e075ae64 100644 --- a/plugins/provisioners/ansible/config/host.rb +++ b/plugins/provisioners/ansible/config/host.rb @@ -48,27 +48,6 @@ module VagrantPlugins { "ansible remote provisioner" => @errors } end - protected - - def check_path(machine, path, path_test_method, error_message_key = nil) - expanded_path = Pathname.new(path).expand_path(machine.env.root_path) - if !expanded_path.public_send(path_test_method) - if error_message_key - @errors << I18n.t(error_message_key, path: expanded_path, system: "host") - end - return false - end - true - end - - def check_path_is_a_file(machine, path, error_message_key = nil) - check_path(machine, path, "file?", error_message_key) - end - - def check_path_exists(machine, path, error_message_key = nil) - check_path(machine, path, "exist?", error_message_key) - end - end end end diff --git a/plugins/provisioners/ansible/provisioner/base.rb b/plugins/provisioners/ansible/provisioner/base.rb index 6092ecfe3..786210474 100644 --- a/plugins/provisioners/ansible/provisioner/base.rb +++ b/plugins/provisioners/ansible/provisioner/base.rb @@ -25,6 +25,15 @@ module VagrantPlugins @inventory_path = nil end + def check_files_existence + check_path_is_a_file config.playbook, :playbook + + check_path_exists config.inventory_path, :inventory_path if config.inventory_path + check_path_is_a_file config.extra_vars[1..-1], :extra_vars if has_an_extra_vars_file_argument + check_path_is_a_file config.galaxy_role_file, :galaxy_role_file if config.galaxy_role_file + check_path_is_a_file config.vault_password_file, :vault_password if config.vault_password_file + end + def ansible_playbook_command_for_shell_execution shell_command = [] @environment_variables.each_pair do |k, v| @@ -198,8 +207,12 @@ module VagrantPlugins return inventory_groups end + def has_an_extra_vars_file_argument + config.extra_vars && config.extra_vars.kind_of?(String) && config.extra_vars =~ /^@.+$/ + end + def extra_vars_argument - if config.extra_vars.kind_of?(String) and config.extra_vars =~ /^@.+$/ + if has_an_extra_vars_file_argument # A JSON or YAML file is referenced. config.extra_vars else diff --git a/plugins/provisioners/ansible/provisioner/guest.rb b/plugins/provisioners/ansible/provisioner/guest.rb index 046806109..298f9cef3 100644 --- a/plugins/provisioners/ansible/provisioner/guest.rb +++ b/plugins/provisioners/ansible/provisioner/guest.rb @@ -14,6 +14,7 @@ module VagrantPlugins end def provision + check_files_existence check_and_install_ansible execute_ansible_galaxy_on_guest if config.galaxy_role_file execute_ansible_playbook_on_guest @@ -145,6 +146,31 @@ module VagrantPlugins end end + def check_path(path, test_args, option_name) + # Checks for the existence of given file (or directory) on the guest system, + # and error if it doesn't exist. + + remote_path = Helpers::expand_path_in_unix_style(path, config.provisioning_path) + command = "test #{test_args} #{remote_path}" + + @machine.communicate.execute( + command, + error_class: Ansible::Errors::AnsibleError, + error_key: :config_file_not_found, + config_option: option_name, + path: remote_path, + system: "guest" + ) + end + + def check_path_is_a_file(path, error_message_key) + check_path(path, "-f", error_message_key) + end + + def check_path_exists(path, error_message_key) + check_path(path, "-e", error_message_key) + end + end end end diff --git a/plugins/provisioners/ansible/provisioner/host.rb b/plugins/provisioners/ansible/provisioner/host.rb index 1eca11d45..f61024722 100644 --- a/plugins/provisioners/ansible/provisioner/host.rb +++ b/plugins/provisioners/ansible/provisioner/host.rb @@ -18,6 +18,7 @@ module VagrantPlugins # At this stage, the SSH access is guaranteed to be ready @ssh_info = @machine.ssh_info + check_files_existence warn_for_unsupported_platform execute_ansible_galaxy_from_host if config.galaxy_role_file execute_ansible_playbook_from_host @@ -257,6 +258,28 @@ module VagrantPlugins ssh_options.join(' ') end + def check_path(path, path_test_method, option_name) + # Checks for the existence of given file (or directory) on the host system, + # and error if it doesn't exist. + + expanded_path = Pathname.new(path).expand_path(@machine.env.root_path) + if !expanded_path.public_send(path_test_method) + raise Ansible::Errors::AnsibleError, + _key: :config_file_not_found, + config_option: option_name, + path: expanded_path, + system: "host" + end + end + + def check_path_is_a_file(path, option_name) + check_path(path, "file?", option_name) + end + + def check_path_exists(path, option_name) + check_path(path, "exist?", option_name) + end + end end end diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 005c88fc5..1514c7a1f 100755 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -2128,22 +2128,16 @@ en: or adapt the `version` option of this provisioner in your Vagrantfile. See https://docs.vagrantup.com/v2/provisioning/ansible_local.html for more information. + config_file_not_found: |- + `%{config_option}` does not exist on the %{system}: %{path} extra_vars_invalid: |- `extra_vars` must be a hash or a path to an existing file. Received: %{value} (as %{type}) + no_playbook: |- + `playbook` file path must be set. raw_arguments_invalid: |- `raw_arguments` must be an array of strings. Received: %{value} (as %{type}) raw_ssh_args_invalid: |- `raw_ssh_args` must be an array of strings. Received: %{value} (as %{type}) - galaxy_role_file_invalid: |- - `galaxy_role_file` does not exist on the %{system}: %{path} - inventory_path_invalid: |- - `inventory_path` does not exist on the %{system}: %{path} - no_playbook: |- - `playbook` file path must be set. - playbook_path_invalid: |- - `playbook` does not exist on the %{system}: %{path} - vault_password_file_invalid: |- - `vault_password_file` does not exist on the %{system}: %{path} installing: "Installing Ansible..." running_galaxy: "Running ansible-galaxy..." running_playbook: "Running ansible-playbook..." diff --git a/test/unit/plugins/provisioners/ansible/config/guest_test.rb b/test/unit/plugins/provisioners/ansible/config/guest_test.rb index 56f1366c6..4368c7032 100644 --- a/test/unit/plugins/provisioners/ansible/config/guest_test.rb +++ b/test/unit/plugins/provisioners/ansible/config/guest_test.rb @@ -14,7 +14,6 @@ describe VagrantPlugins::Ansible::Config::Guest do let(:communicator) { double("communicator") } let(:existing_file) { "this/path/is/a/stub" } - let(:non_existing_file) { "this/path/does/not/exist" } it "supports a list of options" do supported_options = %w( extra_vars @@ -57,107 +56,10 @@ describe VagrantPlugins::Ansible::Config::Guest do describe "#validate" do before do - machine.stub(communicate: communicator) - end - - context "when the machine is not ready to communicate" do - before do - allow(communicator).to receive(:ready?).and_return(false) - end - - it "cannot check the existence of remote file" do - subject.playbook = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible local provisioner"]).to eql([]) - # FIXME: commented out because this `communicator.ready?` stub is not working as expected - # expect(communicator).to receive(:ready?).ordered - # Note that communicator mock will fail if it receives an unexpected message, - # which is part of this spec. - end - end - - context "when the machine is ready to communicate" do - before do - allow(communicator).to receive(:ready?).and_return(true) - allow(communicator).to receive(:test).and_return(false) - - allow(communicator).to receive(:test) do |arg1| - arg1.include?("#{existing_file}") - end - - stubbed_ui = Vagrant::UI::Colored.new - machine.stub(ui: stubbed_ui) - allow(machine.ui).to receive(:warn) - - subject.playbook = existing_file - end - - it_behaves_like "an Ansible provisioner", "/vagrant", "local" - - it "only shows a warning if the playbook file does not exist" do - subject.playbook = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to be_nil - - # FIXME - # expect(machine).to receive(:ui).with { |warning_text| - # expect(warning_text).to eq("`playbook` does not exist on the guest: /vagrant/this/path/does/not/exist") - # } - end - - it "only shows a warning if inventory_path is specified, but does not exist" do - subject.inventory_path = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to be_nil - - # FIXME - # expect(machine.ui).to receive(:warn).with { |warning_text| - # expect(warning_text).to eq("`inventory_path` does not exist on the guest: this/path/does/not/exist") - # } - end - - it "only shows a warning if vault_password_file is specified, but does not exist" do - subject.vault_password_file = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to be_nil - - # FIXME - # expect(machine.ui).to receive(:warn).with { |warning_text| - # expect(warning_text).to eq("`inventory_path` does not exist on the guest: this/path/does/not/exist") - # } - end - - it "it doesn't consider missing files on the remote system as errors" do - subject.playbook = non_existing_file - subject.inventory_path = non_existing_file - subject.extra_vars = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible local provisioner"]).to include( - I18n.t("vagrant.provisioners.ansible.errors.extra_vars_invalid", - type: subject.extra_vars.class.to_s, - value: subject.extra_vars.to_s)) - - expect(result["ansible local provisioner"]).to_not include( - I18n.t("vagrant.provisioners.ansible.errors.playbook_path_invalid", - path: File.join("/vagrant", non_existing_file), system: "guest")) - - expect(result["ansible local provisioner"]).to_not include( - I18n.t("vagrant.provisioners.ansible.errors.inventory_path_invalid", - path: File.join("/vagrant", non_existing_file), system: "guest")) - end - + subject.playbook = existing_file end + it_behaves_like "an Ansible provisioner", "/vagrant", "local" end end diff --git a/test/unit/plugins/provisioners/ansible/config/host_test.rb b/test/unit/plugins/provisioners/ansible/config/host_test.rb index 8ebe6e35a..f6871c209 100644 --- a/test/unit/plugins/provisioners/ansible/config/host_test.rb +++ b/test/unit/plugins/provisioners/ansible/config/host_test.rb @@ -11,7 +11,6 @@ describe VagrantPlugins::Ansible::Config::Host, :skip_windows => true do let(:machine) { double("machine", env: Vagrant::Environment.new) } let(:existing_file) { File.expand_path(__FILE__) } - let(:non_existing_file) { "/this/does/not/exist" } it "supports a list of options" do supported_options = %w( ask_sudo_pass @@ -74,50 +73,6 @@ describe VagrantPlugins::Ansible::Config::Host, :skip_windows => true do it_behaves_like "an Ansible provisioner", "", "remote" - it "returns an error if the playbook file does not exist" do - subject.playbook = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to eql([ - I18n.t("vagrant.provisioners.ansible.errors.playbook_path_invalid", - path: non_existing_file, system: "host") - ]) - end - - it "returns an error if galaxy_role_file is specified, but does not exist" do - subject.galaxy_role_file = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to eql([ - I18n.t("vagrant.provisioners.ansible.errors.galaxy_role_file_invalid", - path: non_existing_file, system: "host") - ]) - end - - it "returns an error if inventory_path is specified, but does not exist" do - subject.inventory_path = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to eql([ - I18n.t("vagrant.provisioners.ansible.errors.inventory_path_invalid", - path: non_existing_file, system: "host") - ]) - end - - it "returns an error if vault_password_file is specified, but does not exist" do - subject.vault_password_file = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to eql([ - I18n.t("vagrant.provisioners.ansible.errors.vault_password_file_invalid", - path: non_existing_file, system: "host") - ]) - end - it "returns an error if the raw_ssh_args is of the wrong data type" do subject.raw_ssh_args = { arg1: 1, arg2: "foo" } subject.finalize! @@ -138,25 +93,6 @@ describe VagrantPlugins::Ansible::Config::Host, :skip_windows => true do expect(subject.raw_arguments).to eql(["-o ControlMaster=no"]) end - it "it collects and returns all detected errors" do - subject.playbook = non_existing_file - subject.inventory_path = non_existing_file - subject.extra_vars = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to include( - I18n.t("vagrant.provisioners.ansible.errors.playbook_path_invalid", - path: non_existing_file, system: "host")) - expect(result["ansible remote provisioner"]).to include( - I18n.t("vagrant.provisioners.ansible.errors.extra_vars_invalid", - type: subject.extra_vars.class.to_s, - value: subject.extra_vars.to_s)) - expect(result["ansible remote provisioner"]).to include( - I18n.t("vagrant.provisioners.ansible.errors.inventory_path_invalid", - path: non_existing_file, system: "host")) - end - end end diff --git a/test/unit/plugins/provisioners/ansible/config/shared.rb b/test/unit/plugins/provisioners/ansible/config/shared.rb index b3364e855..7ff56ee91 100644 --- a/test/unit/plugins/provisioners/ansible/config/shared.rb +++ b/test/unit/plugins/provisioners/ansible/config/shared.rb @@ -29,13 +29,6 @@ shared_examples_for 'an Ansible provisioner' do | path_prefix, ansible_setup | provisioner_label = "ansible #{ansible_setup} provisioner" provisioner_system = ansible_setup == "local" ? "guest" : "host" - it "passes if the playbook option refers to an existing file" do - subject.finalize! - - result = subject.validate(machine) - expect(result[provisioner_label]).to eql([]) - end - it "returns an error if the playbook option is undefined" do subject.playbook = nil subject.finalize! @@ -46,14 +39,6 @@ shared_examples_for 'an Ansible provisioner' do | path_prefix, ansible_setup | ]) end - it "passes if the extra_vars option refers to an existing file" do - subject.extra_vars = existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result[provisioner_label]).to eql([]) - end - it "passes if the extra_vars option is a hash" do subject.extra_vars = { var1: 1, var2: "foo" } subject.finalize! @@ -62,21 +47,6 @@ shared_examples_for 'an Ansible provisioner' do | path_prefix, ansible_setup | expect(result[provisioner_label]).to eql([]) end - # FIXME: - # The guest-based config should actually NOT return this error, - # but only display a warning (similarly to GH-6764 and 4e451c6) - it "returns an error if the extra_vars option refers to a file that does not exist" do - subject.extra_vars = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result[provisioner_label]).to eql([ - I18n.t("vagrant.provisioners.ansible.errors.extra_vars_invalid", - type: subject.extra_vars.class.to_s, - value: subject.extra_vars.to_s) - ]) - end - it "returns an error if the extra_vars option is of wrong data type" do subject.extra_vars = ["var1", 3, "var2", 5] subject.finalize! @@ -89,12 +59,12 @@ shared_examples_for 'an Ansible provisioner' do | path_prefix, ansible_setup | ]) end - it "passes if inventory_path refers to an existing location" do - subject.inventory_path = existing_file + it "converts a raw_arguments option defined as a String into an Array" do + subject.raw_arguments = "--foo=bar" subject.finalize! result = subject.validate(machine) - expect(result[provisioner_label]).to eql([]) + expect(subject.raw_arguments).to eql(%w(--foo=bar)) end it "returns an error if the raw_arguments is of the wrong data type" do @@ -109,12 +79,25 @@ shared_examples_for 'an Ansible provisioner' do | path_prefix, ansible_setup | ]) end - it "converts a raw_arguments option defined as a String into an Array" do - subject.raw_arguments = "--foo=bar" + it "it collects and returns all detected errors" do + subject.playbook = nil + subject.extra_vars = ["var1", 3, "var2", 5] + subject.raw_arguments = { arg1: 1, arg2: "foo" } subject.finalize! result = subject.validate(machine) - expect(subject.raw_arguments).to eql(%w(--foo=bar)) + + expect(result[provisioner_label].size).to eql(3) + expect(result[provisioner_label]).to include( + I18n.t("vagrant.provisioners.ansible.errors.no_playbook")) + expect(result[provisioner_label]).to include( + I18n.t("vagrant.provisioners.ansible.errors.extra_vars_invalid", + type: subject.extra_vars.class.to_s, + value: subject.extra_vars.to_s)) + expect(result[provisioner_label]).to include( + I18n.t("vagrant.provisioners.ansible.errors.raw_arguments_invalid", + type: subject.raw_arguments.class.to_s, + value: subject.raw_arguments.to_s)) end describe "sudo option" do diff --git a/test/unit/plugins/provisioners/ansible/provisioner_test.rb b/test/unit/plugins/provisioners/ansible/provisioner_test.rb index e662cd959..4a916cae2 100644 --- a/test/unit/plugins/provisioners/ansible/provisioner_test.rb +++ b/test/unit/plugins/provisioners/ansible/provisioner_test.rb @@ -59,6 +59,8 @@ VF stubbed_ui.stub(detail: "") machine.env.stub(ui: stubbed_ui) + subject.stub(:check_path) + config.playbook = 'playbook.yml' end @@ -203,6 +205,43 @@ VF end end + describe 'checking existence of Ansible configuration files' do + + describe 'when the playbook file does not exist' do + it "raises an error", skip_before: true, skip_after: true do + + subject.stub(:check_path).and_raise(VagrantPlugins::Ansible::Errors::AnsibleError, + _key: :config_file_not_found, + config_option: "playbook", + path: "/home/wip/test/invalid_path.yml", + system: "host") + + config.playbook = "/home/wip/test/invalid_path.yml" + config.finalize! + + expect {subject.provision}.to raise_error(VagrantPlugins::Ansible::Errors::AnsibleError, + "`playbook` does not exist on the host: /home/wip/test/invalid_path.yml") + end + end + + describe 'when the inventory path does not exist' do + it "raises an error" + end + + describe 'when the extra_vars file does not exist' do + it "raises an error" + end + + describe 'when the galaxy_role_file does not exist' do + it "raises an error" + end + + describe 'when the vault_password_file does not exist' do + it "raises an error" + end + + end + describe 'when ansible-playbook fails' do it "raises an error", skip_before: true, skip_after: true do config.finalize!