synchronize: quotes around arguments

[flybyray]

fix quoting for specific cmd arguments

Fixes:

• Synchronize module not recognizing remote ssh key #24
• Synchronize not using quotes around arguments like --out-format  #190

ISSUE TYPE:

• Bugfix Pull Request

COMPONENT NAME:
module: synchronize

[Akasurde]

cc @quidame @saito-hideki @aminvakil Could you please review this? Thanks in advance.

[Akasurde]

Would it be possible to add tests for this change?

[flybyray]

There is already a test for it.
https://github.com/ansible-collections/ansible.posix/blob/main/tests/integration/targets/synchronize/tasks/main.yml#L154-L166

As @quidame mentioned later the integration tests were disabled sometime ago. I now enabled them. localy with docker it worked

TASK [synchronize : synchronize files using rsync_path (issue#7182)] ***********
changed: [testhost] => {"changed": true, "cmd": "/usr/bin/rsync --delay-updates -F --compress --archive '--rsync-path=sudo rsync' '--out-format=<<CHANGED>>%i %n%L' /root/ansible_collections/ansible/posix/tests/output/.tmp/output_dir/foo.txt /root/ansible_collections/ansible/posix/tests/output/.tmp/output_dir/foo.rsync_path", "msg": ">f+++++++++ foo.txt\n", "rc": 0, "stdout_lines": [">f+++++++++ foo.txt"]}

TASK [synchronize : assert] ****************************************************
ok: [testhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [synchronize : Cleanup] ***************************************************
changed: [testhost] => (item=foo.rsync_path) => {"ansible_loop_var": "item", "changed": true, "item": "foo.rsync_path", "path": "/root/ansible_collections/ansible/posix/tests/output/.tmp/output_dir/foo.rsync_path", "state": "absent"}

https://dev.azure.com/ansible/ansible.posix/_build/results?buildId=19837&view=logs&j=649027d1-4c8e-54ec-15ed-99c8fd6e3c09&t=e29e47f7-55c5-531b-4273-3a98ea9afdae&l=404

[quidame]

hi @flybyray , thanks for fixing this !
Currently integration tests don't fail because they are disabled in tests/integration/targets/synchronize/aliases. Can you enable them and also add tests related to this PR ?

[saito-hideki]

Based on the code changes, I think we probably need to append the integration test like below:

- name: command-line options are quoted correctly (issue#24 and issue#190)
  synchronize:
    src: "{{output_dir}}/foo.txt"
    dest: "dummy:{{output_dir}}/foo.rsync_path_1"
  register: sync_result
  ignore_errors: true
- assert:
    that:
    - sync_result.cmd is search("'--rsh=.*'")
    - sync_result.cmd is search("'--out-format=.*'")

[flybyray]

Based on the code changes, I think we probably need to append the integration test like below:

- name: command-line options are quoted correctly (issue#24 and issue#190)
  synchronize:
    src: "{{output_dir}}/foo.txt"
    dest: "dummy:{{output_dir}}/foo.rsync_path_1"
  register: sync_result
  ignore_errors: true
- assert:
    that:
    - sync_result.cmd is search("'--rsh=.*'")
    - sync_result.cmd is search("'--out-format=.*'")

thanks. it should reference somehow the issues with their reported problems?

Can someone please have a look at those two changes:

https://github.com/ndgit/ansible.posix/blame/fix-24-and-190/plugins/modules/synchronize.py#L604

https://github.com/ndgit/ansible.posix/commit/0118bf0cb9256637684db3f18cbd012d3851065e#diff-3332cfa3b3869c49a3c44104c6662f6e08d303f01ca9491981a2ba38e333a560L604

https://github.com/ndgit/ansible.posix/blame/fix-24-and-190/plugins/modules/synchronize.py#L607

https://github.com/ndgit/ansible.posix/commit/0118bf0cb9256637684db3f18cbd012d3851065e#diff-3332cfa3b3869c49a3c44104c6662f6e08d303f01ca9491981a2ba38e333a560L607

I think they would not be required if the run_command from basic module would work as expected. It looks to me that there is most probably another issue, which should not be solved here.

with my change the command will be run differently:
https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/basic.py#L1856-L1858

The second would be the case with my change an not the first:

If args is a string and use_unsafe_shell=False it will split args to a list and run with shell=False

The implementation might beinefficient. Fist building a string from a list, than building a list from string.

[saito-hideki]

@flybyray Thank you for your comment. As you pointed out, my integration test example is not essential for this PR as it only tests the behavior of shlex_quote().
I also confirmed the integration test works fine now without '--allow-disabled' on my end.

Thanks again :)

[alisonatwork]

@gravesm @Akasurde @saito-hideki in case you didn't notice the bug report - this change has introduced a bug that breaks (formerly) correct and working synchronize tasks. Please can you take a look at the bug report #449 and suggest what to do from here.