Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ansible 2.0 breaks "here" documents in shell scripts #12856

Closed
larsks opened this issue Oct 21, 2015 · 10 comments
Closed

Ansible 2.0 breaks "here" documents in shell scripts #12856

larsks opened this issue Oct 21, 2015 · 10 comments
Labels
bug This issue/PR relates to a bug.
Milestone

Comments

@larsks
Copy link
Contributor

larsks commented Oct 21, 2015

I wanted to embed a "here" document (e.g. cat <<EOF ...) in a shell: script in my playbook, like this:

shell: |
  . credentials/keystonerc_admin

  cat <<-EOF
  auth_url: "$OS_AUTH_URL"
  username: "$OS_USERNAME"
  password: "$OS_PASSWORD"
  tenant_name: "$OS_TENANT_NAME"
  EOF

In 1.9.4, this breaks because the magic processing that looks for
module arguments in the command string resulted in the following
script:

. credentials/keystonerc_admin

 cat <<-EOF
 auth_url: \"$OS_AUTH_URL\"
 username: \"$OS_USERNAME\"
 password: \"$OS_PASSWORD\"
 tenant_name: \"$OS_TENANT_NAME\"
 EOF

Note the extra space ( ) at the beginning of every line, which
breaks shell parsing. It was possible to fix this by passing the
script directly to thecmd parameter, which would bypass the magic
"look for arguments" processing:

shell:
  cmd: |
    . credentials/keystonerc_admin

    cat <<-EOF
    auth_url: "$OS_AUTH_URL"
    username: "$OS_USERNAME"
    password: "$OS_PASSWORD"
    tenant_name: "$OS_TENANT_NAME"
    EOF

This worked great. But then Ansible 2.0 came along, and the above
fails with:

unsupported parameter for module: cmd

And without the cmd: parameter, it fails the same was as in 1.9.4.

@jimi-c jimi-c added this to the v2 milestone Oct 21, 2015
@larsks
Copy link
Contributor Author

larsks commented Oct 21, 2015

So, looking through the code, it looks a possible source of the problem is the parsing code in lib/ansible/parsing/splitter.py, which uses split_args to split up the arguments and then re-assembles them with ' '.join(...). This seems like the wrong thing to do, especially when receiving a YAML literal block. There are a few ways of fixing this:

  • Add back the cmd parameter, for providing an argument that doesn't get processed by splitter.py.
  • If no keyword arguments are found, return the argument string verbatim rather than splitting/re-assembling it
  • I had a third idea, but it just slipped my mind.

Any preferences?

@jimi-c
Copy link
Member

jimi-c commented Oct 21, 2015

Hi @larsks, in 2.0 you can actually use _raw_params for the cmd parameter, which works fine in my testing. I can add some code to the preprocess_data() method for Task to convert this automatically internally.

@larsks
Copy link
Contributor Author

larsks commented Oct 21, 2015

You mean, put code in that would remap cmd to _raw_params? I guess that works for me, but then suddenly everything supports a cmd parameter. Would it make more sense to do this in the command module itself? I don't have a strong feeling on the matter, I guess, since both solutions will result in properly handled shell scripts...

larsks added a commit to larsks/ansible that referenced this issue Oct 21, 2015
This commit addresses ansible#12856, in which shell scripts passed to the
`shell` module are corrupted to the split-and-rejoin approach taken in
`lib/anslibe/parsing/splitter.py`.

This commit modifies the behavior of `parse_kv` such that if there are
no keyword arguments the raw arguments are returned verbatim, rather
than going through the split-and-rejoin process.
@larsks
Copy link
Contributor Author

larsks commented Oct 21, 2015

@jimi-c, take a look at the PR I just submitted. This is a two-line change that implements the second option from my earlier comment: if there are no keyword arguments discovered, just return the raw string. With this change in place, this works just fine:

- hosts: localhost
  tasks:
    - shell: |
        cat <<EOF
        This is
        a test.
        EOF
      register: output

    - debug:
        var: output

@jimi-c
Copy link
Member

jimi-c commented Oct 21, 2015

@larsks no, only command/shell/script would support it. Here's the working patch I have:

diff --git a/lib/ansible/playbook/task.py b/lib/ansible/playbook/task.py
index b66c331..3527cee 100644
--- a/lib/ansible/playbook/task.py
+++ b/lib/ansible/playbook/task.py
@@ -171,6 +171,15 @@ class Task(Base, Conditional, Taggable, Become):
         args_parser = ModuleArgsParser(task_ds=ds)
         (action, args, delegate_to) = args_parser.parse()
 
+        # the command/shell/script modules used to support the `cmd` arg,
+        # which corresponds to what we now call _raw_params, so move that
+        # value over to _raw_params (assuming it is empty)
+        if action in ('command', 'shell', 'script'):
+            if 'cmd' in args:
+                if args.get('_raw_params', '') != '':
+                    raise AnsibleError("The 'cmd' argument cannot be used when other raw parameters are specified. Please put everything in one or the other place.", obj=ds)
+                args['_raw_params'] = args.pop('cmd')
+
         new_ds['action']      = action
         new_ds['args']        = args
         new_ds['delegate_to'] = delegate_to

@larsks
Copy link
Contributor Author

larsks commented Oct 21, 2015

I am a little more comfortable with my solution, because it doesn't require special-casing specific modules in task.py. I mean, what if I want to be able to pass verbatim scripts to something else that is not command or shell or script?

@jimi-c
Copy link
Member

jimi-c commented Oct 21, 2015

We already filter out things which may try to populate _raw_params to a subset of modules.

@larsks
Copy link
Contributor Author

larsks commented Oct 21, 2015

Huh. I didn't know that.

I think my solution is still more obvious to someone using Ansible (you want a shell scripts? Here's your shell script.), but I guess either solution solves my immediate problem.

@jimi-c
Copy link
Member

jimi-c commented Oct 21, 2015

Yep, I'm going to merge in my fix, as it follows the pattern in the code we already have.

@jimi-c jimi-c closed this as completed in 2b3c5aa Oct 21, 2015
@jimi-c
Copy link
Member

jimi-c commented Oct 21, 2015

Closing This Ticket

Hi!

We believe the above commit should resolve this problem for you. This will also be included in the next major release.

If you continue seeing any problems related to this issue, or if you have any further questions, please let us know by stopping by one of the two mailing lists, as appropriate:

Because this project is very active, we're unlikely to see comments made on closed tickets, but the mailing list is a great way to ask questions, or post if you don't think this particular issue is resolved.

Thank you!

omgjlk added a commit to omgjlk/ansible-lint that referenced this issue Sep 28, 2017
At some point, ansible grew a hidden argument to shell/command tasks,
the 'cmd' argument. History found at
ansible/ansible#12856

Our project is making use of the cmd argument so that our shell bits do
not get jinjad, so that we can use a here doc in the command.
Unfortunately this was tripping up various tests in ansible-lint, and we
had to ignore the files, which let other errors creep in. Thus this
change adds the type of syntax we use to the test yaml files and updates
the tests to propery handle them.

Signed-off-by: Jesse Keating <jkeating@j2solutions.net>
willthames pushed a commit to ansible/ansible-lint that referenced this issue Sep 28, 2017
At some point, ansible grew a hidden argument to shell/command tasks,
the 'cmd' argument. History found at
ansible/ansible#12856

Our project is making use of the cmd argument so that our shell bits do
not get jinjad, so that we can use a here doc in the command.
Unfortunately this was tripping up various tests in ansible-lint, and we
had to ignore the files, which let other errors creep in. Thus this
change adds the type of syntax we use to the test yaml files and updates
the tests to propery handle them.

Signed-off-by: Jesse Keating <jkeating@j2solutions.net>
rsumner31 added a commit to rsumner31/ansible-lint that referenced this issue Mar 3, 2018
At some point, ansible grew a hidden argument to shell/command tasks,
the 'cmd' argument. History found at
ansible/ansible#12856

Our project is making use of the cmd argument so that our shell bits
do
not get jinjad, so that we can use a here doc in the command.
Unfortunately this was tripping up various tests in ansible-lint, and
we
had to ignore the files, which let other errors creep in. Thus this
change adds the type of syntax we use to the test yaml files and
updates
the tests to propery handle them.

Signed-off-by: Jesse Keating <jkeating@j2solutions.net>
Conflicts:
	lib/ansiblelint/rules/CommandsInsteadOfArgumentsRule.py
	lib/ansiblelint/rules/CommandsInsteadOfModulesRule.py
	lib/ansiblelint/rules/EnvVarsInCommandRule.py
	lib/ansiblelint/rules/UseCommandInsteadOfShellRule.py
	test/command-check-success.yml
	test/command-instead-of-shell-success.yml
	test/env-vars-in-command-success.yml
rsumner31 added a commit to rsumner31/ansible-lint that referenced this issue Mar 3, 2018
At some point, ansible grew a hidden argument to shell/command tasks,
the 'cmd' argument. History found at
ansible/ansible#12856

Our project is making use of the cmd argument so that our shell bits
do
not get jinjad, so that we can use a here doc in the command.
Unfortunately this was tripping up various tests in ansible-lint, and
we
had to ignore the files, which let other errors creep in. Thus this
change adds the type of syntax we use to the test yaml files and
updates
the tests to propery handle them.

Signed-off-by: Jesse Keating <jkeating@j2solutions.net>
rsumner31 added a commit to rsumner31/ansible-lint that referenced this issue Mar 5, 2018
At some point, ansible grew a hidden argument to shell/command tasks,
the 'cmd' argument. History found at
ansible/ansible#12856

Our project is making use of the cmd argument so that our shell bits
do
not get jinjad, so that we can use a here doc in the command.
Unfortunately this was tripping up various tests in ansible-lint, and
we
had to ignore the files, which let other errors creep in. Thus this
change adds the type of syntax we use to the test yaml files and
updates
the tests to propery handle them.

Signed-off-by: Jesse Keating <jkeating@j2solutions.net>
Conflicts:
	lib/ansiblelint/rules/CommandsInsteadOfArgumentsRule.py
	lib/ansiblelint/rules/CommandsInsteadOfModulesRule.py
	lib/ansiblelint/rules/EnvVarsInCommandRule.py
	lib/ansiblelint/rules/UseCommandInsteadOfShellRule.py
	test/command-check-success.yml
	test/command-instead-of-shell-success.yml
	test/env-vars-in-command-success.yml
Conflicts:
	lib/ansiblelint/rules/CommandsInsteadOfModulesRule.py
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bug_report labels Mar 7, 2018
@ansible ansible locked and limited conversation to collaborators Apr 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue/PR relates to a bug.
Projects
None yet
Development

No branches or pull requests

3 participants