copy: do not chmod if mode in not set

[user318]

ISSUE TYPE

• Bug Report

COMPONENT NAME

copy module

ANSIBLE VERSION

ansible 2.5.2
  config file = /home/user/.ansible.cfg
  configured module search path = ['/home/user/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/user/soft/test/lib/python3.6/site-packages/ansible
  executable location = /home/user/soft/test/bin/ansible
  python version = 3.6.1 (default, Jun  2 2017, 15:55:17) [GCC 4.8.4]

CONFIGURATION

N/A

OS / ENVIRONMENT

from Ubuntu Linux
manage Arista switch with Fedora-based OS
or
manage another Ubuntu Linux

SUMMARY

When using "copy" module with destination on a file system not supporting chmod(), I'm getting an exception.
vfat not always returns an error on chmod() call. If the filesystem is owned by the same user, it does not actually change permissions, but returns no error. If another user is able to write to this filesystem - for him chmod returns an error.
chmod() should not be called when mode is not specified or errors from it should be ignored.

STEPS TO REPRODUCE

ansible arista -u ansible -m copy -a "src=test.txt dest=/mnt/flash/test.txt" -vvv

Example of operations on vfat mount from user not owning the fs, but with write access:

user:/mnt/tmp$ ls -lha
total 25K
drwxrwx--- 2 nobody user 16K May 16 09:59 .
drwxr-xr-x 3 root   root   3 Apr 15  2017 ..
-rwxrwx--- 1 nobody user   0 May 16 09:53 1.txt
user:/mnt/tmp$ touch test.txt
user:/mnt/tmp$ ls -lha
total 25K
drwxrwx--- 2 nobody user 16K May 16 10:03 .
drwxr-xr-x 3 root   root   3 Apr 15  2017 ..
-rwxrwx--- 1 nobody user   0 May 16 09:53 1.txt
-rwxrwx--- 1 nobody user   0 May 16 10:03 test.txt
user:/mnt/tmp$ chmod 755 test.txt 
chmod: changing permissions of 'test.txt': Operation not permitted
user:/mnt/tmp$ 

User can create file, but chmod returns an error. The owner (nobody) have no error on chmod;

nobody:/mnt/tmp$ ls -lha
total 25K
drwxrwx--- 2 nobody user 16K May 16 10:03 .
drwxr-xr-x 3 root   root   3 Apr 15  2017 ..
-rwxrwx--- 1 nobody user   0 May 16 09:53 1.txt
-rwxrwx--- 1 nobody user   0 May 16 10:03 test.txt
nobody:/mnt/tmp$ chmod 755 test.txt
nobody:/mnt/tmp$ ls -lha
total 25K
drwxrwx--- 2 nobody user 16K May 16 10:03 .
drwxr-xr-x 3 root   root   3 Apr 15  2017 ..
-rwxrwx--- 1 nobody user   0 May 16 09:53 1.txt
-rwxrwx--- 1 nobody user   0 May 16 10:03 test.txt
nobody:/mnt/tmp$ 

EXPECTED RESULTS

No errors, file copied to managed host.

ACTUAL RESULTS

The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_WKlj_b/ansible_modlib.zip/ansible/module_utils/basic.py", line 2536, in atomic_move
    shutil.copy2(b_src, b_tmp_dest_name)
  File "/usr/lib/python2.7/shutil.py", line 128, in copy2
    copystat(src, dst)
  File "/usr/lib/python2.7/shutil.py", line 99, in copystat
    os.chmod(dst, mode)
OSError: [Errno 1] Operation not permitted: '/mnt/flash/.ansible_tmpAsS9sftest.txt'

aristalab | FAILED! => {
    "changed": false,
    "checksum": "da39a3ee5e6b4b0d3255bfef95601890afd80709",
    "diff": [],
    "invocation": {
        "module_args": {
            "attributes": null,
            "backup": false,
            "checksum": "da39a3ee5e6b4b0d3255bfef95601890afd80709",
            "content": null,
            "delimiter": null,
            "dest": "/mnt/flash/test.txt",
            "directory_mode": null,
            "follow": false,
            "force": true,
            "group": null,
            "local_follow": null,
            "mode": null,
            "original_basename": "test.txt",
            "owner": null,
            "regexp": null,
            "remote_src": null,
            "selevel": null,
            "serole": null,
            "setype": null,
            "seuser": null,
            "src": "/home/ansible/.ansible/tmp/ansible-tmp-1526452633.1935987-117985603557511/source",
            "unsafe_writes": null,
            "validate": null
        }
    },
    "msg": "Failed to replace file: /home/ansible/.ansible/tmp/ansible-tmp-1526452633.1935987-117985603557511/source to /mnt/flash/test.txt: [Errno 1] Operation not permitted: '/mnt/flash/.ansible_tmpAsS9sftest.txt'"
}

[ansibot]

Files identified in the description:

• lib/ansible/modules/files/copy.py

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

[cu]

Probable dupe of #7372 and #19731

[user318]

@cu In #19731 it was fixed only for remote_src=True and I was asked to file a separate bug.

[user318]

And #7372 is about chown(). This about chmod(). And patch there compares uid/gid to figure out wether to do chown() or not. But I want to be able to have an option to skip chown/chmod completely. So one can use filesystems which do not support those operations.

[ansibot]

cc @ptux
click here for bot help

[user318]

This changes works for me. Not sure though if I access mode parameter correctly.

ansible-modeignore.patch.txt

[ansibot]

Files identified in the description:

• lib/ansible/modules/copy.py

If these files are incorrect, please update the component name section of the description or use the !component bot command.

click here for bot help

[relrod]

This is expected behavior, copy expects to be copying to a posix-compatible filesystem.

[user318]

So what? Should we throw away any users with non-posix filesystems then? I suggest that ansible has some mode for "copy" when it can be usable for non-posix systems too. Isn't it a reasonable feature?

[bcoca]

It is reasonable to a point, for example, have win_copy for windows, for other systems a specific module might be needed per OS, due to the proprietary nature of most of these it is unlikely that the core team will develop them or accept them in core, as collections would be a more appropriate home for them.

[user318]

Separate module is not a good option, because it will be needed to have a separate module for template too and may be some others, if they depend on copy. Actually it there is not much to do in the copy module itself, but it is something done in supporting functions, that are not flexible enough for such cases. Even if it were a separate module, it would be good, I suppose, to use the same codebase for it.
And I do not actually think that there is need for os-specific modules. For example I do not want win_copy, I need it on Linux running on the Arista switch, which uses FAT filesystem on the flash. This is not my choice, but I have to live with it somehow. And I think that disabling chown/chmod attempts should perform quite well for Windows too. I do not propose to add some OS-specific extra features here, but for the copy module to skip some of its activities.

[bcoca]

we HAVE a win_copy module for these reasons, its not just chmod, you would have to disable stating, setfacl, chown, chgrp, selinux context ... and that is just off top of my head.

[user318]

I have doubt it works for me, because the documentation says:

• For non-Windows targets, use the copy module instead
• win_copy runs over WinRM

I have Linux and ssh, do not even know what that WinRM is. :) But I'll try it.

PS. I had problems with the module failing only because of chmod/chown/chgrp - they causes exception. Even if it tries to do something other - it still works without failing.

[bcoca]

i'm NOT telling you to use win_copy, i was using it as an example that for non posix systems we would need a dedicated module.

[user318]

OK, misunderstood it. :)
Anyway, the above error I receive is "Operation not permitted", i.e. permission error. But the module fails on it when I do not want to touch mode/owner. It may even happen on posix-compatible filesystems too. Also there are already several cases in atomic_move to support "non posix-compatible" filesystems. And there are also some checks for situations when access is not allowed (for unsafe_writes).
I do not understand what is bad with adding the option to copy/template/etc. so that they do not touch mode/owner? It is not about posix compatibility. It is not that easy just to make separate xxx_copy module here, because the changes needed are in atomic_move function in basic.py, so those parts need to be almost completely copied just for a couple of simple changes.