module synchronize: quoting

[erwilan]

SUMMARY

a few years ago i contributed https://github.com/ansible/ansible/commit/f7c9f44aabbabe371358672be2381f9a11073f96

and now i have found https://github.com/ansible-collections/ansible.posix/commit/0118bf0cb9256637684db3f18cbd012d3851065e this commit has dramatic effects.

the commit should fix https://github.com/ansible-collections/ansible.posix/issues/190 and https://github.com/ansible-collections/ansible.posix/issues/24

but these issues are simply user errors:

1. you have to pass each rsync argument as separate list item to rsync_opts and
2. before commit https://github.com/ansible-collections/ansible.posix/commit/0118bf0cb9256637684db3f18cbd012d3851065e was applied each item in rsync_opts got passed as is and was not shell-like-splitted (shlex.split()) - so no quotes were removed

but instead of undoing this fatal commit, these additional changes were made to mitigate the newly introduced difficulties: https://github.com/ansible-collections/ansible.posix/commit/db12a40a4c9d7f32e3cee23cef2abcce272a188d https://github.com/ansible-collections/ansible.posix/commit/d1be5519e60e0d111715ba4cceb9b0f6d428c171

ansible runs the rsync command through the run_command method (https://docs.ansible.com/ansible/latest/reference_appendices/module_utils.html) there is a major difference whether you pass as string or a list to run_command

https://github.com/ansible-collections/ansible.posix/commit/0118bf0cb9256637684db3f18cbd012d3851065e does the following: insted of passing the list cmd to run_command it passes the string cmdstr to run_command

cmdstr is constructed that way: cmdstr = ' '.join(cmd)

cmdstr was never intendet to get passed to run_command - it was simply there to have a string for the return-dict.

if you wanted to pass a string instead of a list to run_command then you would have to do the following: cmdstr = ' '.join(shlex_quote(a) for a in cmd)

but what would be the benefit - handing over a list is the better and simpler solution.

COMPONENT NAME

synchronize

[alisonatwork]

I think we have also been bitten by this commit of 0118bf0 which broke the expected behavior of Ansible POSIX module. In our case we have a task that looks something like this:

synchronize:
  src: /x
  dest: /y
  rsync_opts:
  - "--filter=merge z.rsync-filter"

In previous versions of Ansible this was correctly passing the filter argument "merge z.rsync-filter", which reads filters from the specified filter file. There was no need to put additional quoting here because rsync_opts is already a list where each list element is parsed as an individual argument. But with the change of 0118bf0, it now parses the space inside the list element as a field separator and rsync returns with error: unexpected end of filter rule: merge.

The "fix" for this is to manually add more quotes into the rsync_opts, like this:

  rsync_opts:
  - "--filter=\"merge z.rsync-filter\""

But this seems like a bit of a roundabout solution since it defeats the point of using a list in the first place. (If we have to pass our own shell-quoted string anyway, then rsync_opts should just be a string.)

I think @erwilan is correct in saying that the new behavior is broken. I think if we already have a list, we should just treat it like a list.

I really hope someone can address this soon, because this behavior means that we need to have two different playbooks for two different versions of Ansible.

[flybyray]

SUMMARY

a few years ago i contributed ansible/ansible@f7c9f44

and now i have found 0118bf0 this commit has dramatic effects.

the commit should fix #190 and #24

but these issues are simply user errors:

1. you have to pass each rsync argument as separate list item to rsync_opts and
2. before commit 0118bf0 was applied each item in rsync_opts got passed as is and was not shell-like-splitted (shlex.split()) - so no quotes were removed

but instead of undoing this fatal commit, these additional changes were made to mitigate the newly introduced difficulties: db12a40 d1be551

ansible runs the rsync command through the run_command method (https://docs.ansible.com/ansible/latest/reference_appendices/module_utils.html) there is a major difference whether you pass as string or a list to run_command

0118bf0 does the following: insted of passing the list cmd to run_command it passes the string cmdstr to run_command

cmdstr is constructed that way: cmdstr = ' '.join(cmd)

cmdstr was never intendet to get passed to run_command - it was simply there to have a string for the return-dict.

if you wanted to pass a string instead of a list to run_command then you would have to do the following: cmdstr = ' '.join(shlex_quote(a) for a in cmd)

but what would be the benefit - handing over a list is the better and simpler solution.

COMPONENT NAME

synchronize

Please add a test for rsync_opts to your old commit and there would have been no dramatic effects and maybe even no user errors when a descriptive test shows usage. Additionally documentation could be improved too.