file.recurse file_mode: keep doesn't work with salt-ssh

[nergdron]

Description of Issue

When recursively copying a directory to a minion using the file.recurse state, when using the file_mode: keep options, permissions from the source host on files are not actually kept intact, despite documentation saying they will be:
https://docs.saltstack.com/en/latest/ref/states/all/salt.states.file.html#salt.states.file.recurse

Setup

/root/some_dir:
  file.recurse:
    - source: salt://{{ slspath }}/some_dir
    - user: root
    - group: root
    - file_mode: keep

Steps to Reproduce Issue

Use salt-ssh to apply the state to a remote system, watch as all files are created with 600 permissions instead of their source permissions.

Versions Report

Salt Version:
           Salt: 2019.2.0
 
Dependency Versions:
           cffi: 1.12.3
       cherrypy: Not Installed
       dateutil: 2.7.3
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.10.1
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.6.1
   mysql-python: Not Installed
      pycparser: 2.19
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.7.3 (default, Aug 20 2019, 17:04:43)
   python-gnupg: Not Installed
         PyYAML: 5.1
          PyZMQ: 18.0.2
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.3.1
 
System Versions:
           dist: Ubuntu 19.04 disco
         locale: UTF-8
        machine: x86_64
        release: 5.0.0-25-generic
         system: Linux
        version: Ubuntu 19.04 disco

[nergdron]

note that explicitly setting permissions with something like file_mode: 755 works, but then all files have the same permissions. so I'd have to create a ton of individual state entries for each file to get all the permissions right.

[xeacott]

I was just looking into this and the file.recurse state makes it was to calling file.manage in the execution module, which does says this:

    mode
        The permissions to set on this file, e.g. ``644``, ``0775``, or
        ``4664``.

        The default mode for new files and directories corresponds to the
        umask of the salt process. The mode of existing files and directories
        will only be changed if ``mode`` is specified.

        .. note::
            This option is **not** supported on Windows.

        .. versionchanged:: 2016.11.0
            This option can be set to ``keep``, and Salt will keep the mode
            from the Salt fileserver. This is only supported when the
            ``source`` URL begins with ``salt://``, or for files local to the
            minion. Because the ``source`` option cannot be used with any of
            the ``contents`` options, setting the ``mode`` to ``keep`` is also
            incompatible with the ``contents`` options.

        .. note:: keep does not work with salt-ssh.

            As a consequence of how the files are transferred to the minion, and
            the inability to connect back to the master with salt-ssh, salt is
            unable to stat the file as it exists on the fileserver and thus
            cannot mirror the mode on the salt-ssh minion

[nergdron]

ahhh so maybe it needs a documentation note on the file.recursive state page to match that. but also, I assume the files are just getting SCP'd over, so I'm not sure why salt-ssh couldn't stat the files locally and apply those changes remotely, or even just use rsync if available. but that's definitely more into feature request territory.

regardless, it'd be good to have a supported workaround for salt-ssh to do this without having to have like a thousand separate file entries to sync a whole dir.

[xeacott]

At minimum, that information should bleed into the state page, I agree. As far as the implementation moving forward, I'll add a feature request to this until it gets investigated as to exactly why it is this way.

[xeacott]

But there does need to be a separate PR to address the documentation. :)

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.

[petrows]

Have same issue on salt. Even more, on salt-master i have another issue, test=true does not show changes, but salt.apply does something:

1. Salt-ssh call on machine:

./bin/apply-ssh pve.pws state.apply                                                                                                            
pve.pws:
----------
...
----------
          ID: pve-backup-code
    Function: file.recurse
        Name: /opt/backup
      Result: True
     Comment: Recursively updated /opt/backup
     Started: 20:02:08.571623
    Duration: 46.607 ms
     Changes:   
              ----------
              /opt/backup/bin/backup:
                  ----------
                  mode:
                      0600
              /opt/backup/bin/backup-post:
                  ----------
                  mode:
                      0600
              /opt/backup/bin/backup-pre:
                  ----------
                  mode:
                      0600
              /opt/backup/bin/daily-post:
                  ----------
                  mode:
                      0600
              /opt/backup/bin/daily-pre:
                  ----------
                  mode:
                      0600
              /opt/backup/bin/git-update:
                  ----------
                  mode:
                      0600
              /opt/backup/bin/vz-dump:
                  ----------
                  mode:
                      0600
              /opt/backup/bin/weekly-pre:
                  ----------
                  mode:
                      0600
              /opt/backup/rsnapshot/daily.conf:
                  ----------
                  mode:
                      0600

Summary for pve.pws
--------------
Succeeded: 103 (changed=5)
Failed:      0
--------------
Total states run:     103
Total run time:     3.879 s

Same machine on salt-master:

# salt pve.pws state.apply test=true
pve.pws:

Summary for pve.pws
--------------
Succeeded: 103
Failed:      0
--------------
Total states run:     103
Total run time:     3.538 s

# salt pve.pws state.apply
pve.pws:
----------
          ID: pve-backup-code
    Function: file.recurse
        Name: /opt/backup
      Result: True
     Comment: Recursively updated /opt/backup
     Started: 20:08:24.438259
    Duration: 181.159 ms
     Changes:
              ----------
              /opt/backup/bin/backup:
                  ----------
                  mode:
                      0755
              /opt/backup/bin/backup-post:
                  ----------
                  mode:
                      0755
              /opt/backup/bin/backup-pre:
                  ----------
                  mode:
                      0755
              /opt/backup/bin/daily-post:
                  ----------
                  mode:
                      0755
              /opt/backup/bin/daily-pre:
                  ----------
                  mode:
                      0755
              /opt/backup/bin/git-update:
                  ----------
                  mode:
                      0755
              /opt/backup/bin/vz-dump:
                  ----------
                  mode:
                      0755
              /opt/backup/bin/weekly-pre:
                  ----------
                  mode:
                      0755
              /opt/backup/rsnapshot/daily.conf:
                  ----------
                  mode:
                      0644

Summary for pve.pws
--------------
Succeeded: 103 (changed=1)
Failed:      0
--------------
Total states run:     103
Total run time:     4.675 s

With test-true no changes detected, but call of state.apply actually reverts broken permissions from salt-ssh

[zaz]

As a consequence of how the files are transferred to the minion, and the inability to connect back to the master with salt-ssh, salt is unable to stat the file as it exists on the fileserver and thus cannot mirror the mode on the salt-ssh minion

Aren't the files transferred to the minion with SCP? In that case, would it not be as simple as adding the -p flag to SCP? If not, how are the files transferred to the minion?