From ab036ddd0bcb5d844756b48adcaef063a12b5d2f Mon Sep 17 00:00:00 2001 From: Gilles Cornu Date: Wed, 20 Apr 2016 18:05:28 +0200 Subject: [PATCH] provisioners/ansible: don't format raw_arguments With cb80286a4ac22b9c55ee3d213d980ea73b11cd14, the helper function stringify_ansible_playbook_command was also applied on the `raw_arguments` content, which is not wanted. Given that users have used the `raw_arguments` option as a workaround to avoid the bug GH-6726, this new change ensure that any `--extra-vars` option passed as a raw argument won't be additonally enquoted by the ansible_local provisioner. This change also improves the ansible remote provisioner verbose output, but has no impact on its behaviour, which was already correct. Note that this refactoring introduces some code duplications that are not very elegant (see ansible_playbook_command_for_shell_execution in host.rb and execute_ansible_playbook_from_host in base.rb). I hope we can find a better implementation later, but it is good enough for now since all these parts are covered by corresponding unit tests (the `ansible_local` stuff being tested via the verbose output of the ansible remote provisioner). --- plugins/provisioners/ansible/config/base.rb | 3 ++ plugins/provisioners/ansible/helpers.rb | 24 ------------ .../provisioners/ansible/provisioner/base.rb | 37 +++++++++++++++++-- .../provisioners/ansible/provisioner/guest.rb | 3 +- .../provisioners/ansible/provisioner/host.rb | 14 +++++-- .../provisioners/ansible/provisioner_test.rb | 6 +-- 6 files changed, 50 insertions(+), 37 deletions(-) diff --git a/plugins/provisioners/ansible/config/base.rb b/plugins/provisioners/ansible/config/base.rb index 14ae1a3e6..d6881c70c 100644 --- a/plugins/provisioners/ansible/config/base.rb +++ b/plugins/provisioners/ansible/config/base.rb @@ -112,6 +112,9 @@ module VagrantPlugins end end + if raw_arguments + @raw_arguments = Helpers::as_array(raw_arguments) + end end end end diff --git a/plugins/provisioners/ansible/helpers.rb b/plugins/provisioners/ansible/helpers.rb index ed6e1d24e..24534c260 100644 --- a/plugins/provisioners/ansible/helpers.rb +++ b/plugins/provisioners/ansible/helpers.rb @@ -3,30 +3,6 @@ require "vagrant" module VagrantPlugins module Ansible class Helpers - def self.stringify_ansible_playbook_command(env, command) - shell_command = '' - env.each_pair do |k, v| - if k == 'ANSIBLE_SSH_ARGS' - shell_command += "#{k}='#{v}' " - else - shell_command += "#{k}=#{v} " - end - end - - shell_arg = [] - command.each do |arg| - if arg =~ /(--start-at-task|--limit)=(.+)/ - shell_arg << %Q(#{$1}="#{$2}") - elsif arg =~ /(--extra-vars)=(.+)/ - shell_arg << %Q(%s="%s") % [$1, $2.gsub('\\', '\\\\\\').gsub('"', %Q(\\"))] - else - shell_arg << arg - end - end - - shell_command += shell_arg.join(' ') - end - def self.expand_path_in_unix_style(path, base_dir) # Remove the possible drive letter, which is added # by `File.expand_path` when running on a Windows host diff --git a/plugins/provisioners/ansible/provisioner/base.rb b/plugins/provisioners/ansible/provisioner/base.rb index 813f96a3e..6092ecfe3 100644 --- a/plugins/provisioners/ansible/provisioner/base.rb +++ b/plugins/provisioners/ansible/provisioner/base.rb @@ -25,6 +25,39 @@ module VagrantPlugins @inventory_path = nil end + def ansible_playbook_command_for_shell_execution + shell_command = [] + @environment_variables.each_pair do |k, v| + if k == 'ANSIBLE_SSH_ARGS' + shell_command << "#{k}='#{v}'" + else + shell_command << "#{k}=#{v}" + end + end + + shell_command << "ansible-playbook" + + shell_args = [] + @command_arguments.each do |arg| + if arg =~ /(--start-at-task|--limit)=(.+)/ + shell_args << %Q(#{$1}="#{$2}") + elsif arg =~ /(--extra-vars)=(.+)/ + shell_args << %Q(%s="%s") % [$1, $2.gsub('\\', '\\\\\\').gsub('"', %Q(\\"))] + else + shell_args << arg + end + end + + shell_command << shell_args + + # Add the raw arguments at the end, to give them the highest precedence + shell_command << config.raw_arguments if config.raw_arguments + + shell_command << config.playbook + + shell_command.flatten.join(' ') + end + def prepare_common_command_arguments # By default we limit by the current machine, # but this can be overridden by the `limit` option. @@ -43,10 +76,6 @@ module VagrantPlugins @command_arguments << "--tags=#{Helpers::as_list_argument(config.tags)}" if config.tags @command_arguments << "--skip-tags=#{Helpers::as_list_argument(config.skip_tags)}" if config.skip_tags @command_arguments << "--start-at-task=#{config.start_at_task}" if config.start_at_task - - # Finally, add the raw configuration options, which has the highest precedence - # and can therefore potentially override any other options of this provisioner. - @command_arguments.concat(Helpers::as_array(config.raw_arguments)) if config.raw_arguments end def prepare_common_environment_variables diff --git a/plugins/provisioners/ansible/provisioner/guest.rb b/plugins/provisioners/ansible/provisioner/guest.rb index 741b07301..a6dda8131 100644 --- a/plugins/provisioners/ansible/provisioner/guest.rb +++ b/plugins/provisioners/ansible/provisioner/guest.rb @@ -78,8 +78,7 @@ module VagrantPlugins prepare_common_command_arguments prepare_common_environment_variables - command = (%w(ansible-playbook) << @command_arguments << config.playbook).flatten - remote_command = "cd #{config.provisioning_path} && #{Helpers::stringify_ansible_playbook_command(@environment_variables, command)}" + remote_command = "cd #{config.provisioning_path} && #{ansible_playbook_command_for_shell_execution}" execute_ansible_command_on_guest "playbook", remote_command end diff --git a/plugins/provisioners/ansible/provisioner/host.rb b/plugins/provisioners/ansible/provisioner/host.rb index 245a00499..2ff957bc8 100644 --- a/plugins/provisioners/ansible/provisioner/host.rb +++ b/plugins/provisioners/ansible/provisioner/host.rb @@ -94,8 +94,6 @@ module VagrantPlugins command_template = config.galaxy_command.gsub(' ', VAGRANT_ARG_SEPARATOR) str_command = command_template % command_values - ui_running_ansible_command "galaxy", str_command.gsub(VAGRANT_ARG_SEPARATOR, ' ') - command = str_command.split(VAGRANT_ARG_SEPARATOR) command << { # Write stdout and stderr data, since it's the regular Ansible output @@ -103,6 +101,8 @@ module VagrantPlugins workdir: @machine.env.root_path.to_s } + ui_running_ansible_command "galaxy", str_command.gsub(VAGRANT_ARG_SEPARATOR, ' ') + execute_command_from_host command end @@ -111,9 +111,13 @@ module VagrantPlugins prepare_environment_variables # Assemble the full ansible-playbook command - command = (%w(ansible-playbook) << @command_arguments << config.playbook).flatten + command = %w(ansible-playbook) << @command_arguments - ui_running_ansible_command "playbook", Helpers::stringify_ansible_playbook_command(@environment_variables, command) + # Add the raw arguments at the end, to give them the highest precedence + command << config.raw_arguments if config.raw_arguments + + command << config.playbook + command = command.flatten command << { env: @environment_variables, @@ -122,6 +126,8 @@ module VagrantPlugins workdir: @machine.env.root_path.to_s } + ui_running_ansible_command "playbook", ansible_playbook_command_for_shell_execution + execute_command_from_host command end diff --git a/test/unit/plugins/provisioners/ansible/provisioner_test.rb b/test/unit/plugins/provisioners/ansible/provisioner_test.rb index 4cc0286f8..e662cd959 100644 --- a/test/unit/plugins/provisioners/ansible/provisioner_test.rb +++ b/test/unit/plugins/provisioners/ansible/provisioner_test.rb @@ -770,14 +770,14 @@ VF config.skip_tags = %w(foo bar) config.limit = 'machine*:&vagrant:!that_one' config.start_at_task = "joe's awesome task" - config.raw_arguments = ["--why-not", "--su-user=foot", "--ask-su-pass", "--limit=all", "--private-key=./myself.key"] + config.raw_arguments = ["--why-not", "--su-user=foot", "--ask-su-pass", "--limit=all", "--private-key=./myself.key", "--extra-vars='{\"var3\":\"foo\"}'"] # environment variables config.host_key_checking = true config.raw_ssh_args = ['-o ControlMaster=no'] end - it_should_set_arguments_and_environment_variables 20, 4, true + it_should_set_arguments_and_environment_variables 21, 4, true it_should_explicitly_enable_ansible_ssh_control_persist_defaults it_should_set_optional_arguments({ "extra_vars" => "--extra-vars={\"var1\":\"string with 'apostrophes', \\\\, \\\" and =\",\"var2\":{\"x\":42}}", "sudo" => "--sudo", @@ -804,7 +804,7 @@ VF it "shows the ansible-playbook command, with additional quotes when required" do expect(machine.env.ui).to receive(:detail).with { |full_command| - expect(full_command).to eq(%Q(PYTHONUNBUFFERED=1 ANSIBLE_FORCE_COLOR=true ANSIBLE_HOST_KEY_CHECKING=true ANSIBLE_SSH_ARGS='-o IdentitiesOnly=yes -i '/my/key1' -i '/my/key2' -o ForwardAgent=yes -o ControlMaster=no -o ControlMaster=auto -o ControlPersist=60s' ansible-playbook --connection=ssh --timeout=30 --ask-sudo-pass --ask-vault-pass --limit="machine*:&vagrant:!that_one" --inventory-file=#{generated_inventory_dir} --extra-vars="{\\"var1\\":\\"string with 'apostrophes', \\\\\\\\, \\\\\\" and =\\",\\"var2\\":{\\"x\\":42}}" --sudo --sudo-user=deployer -vvv --vault-password-file=#{File.expand_path(__FILE__)} --tags=db,www --skip-tags=foo,bar --start-at-task="joe's awesome task" --why-not --su-user=foot --ask-su-pass --limit="all" --private-key=./myself.key playbook.yml)) + expect(full_command).to eq(%Q(PYTHONUNBUFFERED=1 ANSIBLE_FORCE_COLOR=true ANSIBLE_HOST_KEY_CHECKING=true ANSIBLE_SSH_ARGS='-o IdentitiesOnly=yes -i '/my/key1' -i '/my/key2' -o ForwardAgent=yes -o ControlMaster=no -o ControlMaster=auto -o ControlPersist=60s' ansible-playbook --connection=ssh --timeout=30 --ask-sudo-pass --ask-vault-pass --limit="machine*:&vagrant:!that_one" --inventory-file=#{generated_inventory_dir} --extra-vars="{\\"var1\\":\\"string with 'apostrophes', \\\\\\\\, \\\\\\" and =\\",\\"var2\\":{\\"x\\":42}}" --sudo --sudo-user=deployer -vvv --vault-password-file=#{File.expand_path(__FILE__)} --tags=db,www --skip-tags=foo,bar --start-at-task="joe's awesome task" --why-not --su-user=foot --ask-su-pass --limit=all --private-key=./myself.key --extra-vars='{\"var3\":\"foo\"}' playbook.yml)) } end end