Repository: saltstack/salt
Pull request: Fix for #38318
This is my first foray in the salt codebase, so apologies in advance if I've got any of the process wrong. 

### What does this PR do?
Fixes #38318 
A file.recurse state with clean=True that "requires" another recurse state will correctly remove files that do not originate in from either state.

### What issues does this PR fix or reference?
#38318 

### Previous Behavior
A file.recurse state that requires another file.recurse state and has clean=True would not remove any files that were below the required recurse directory in the tree.

### New Behavior
_gen_keep_files now retrieves the full list of files managed by a required recurse state and adds them to the keep list, rather than simply keeping the entire recurse directory.

### Tests written?

No

Please review [Salt's Contributing Guide](https://docs.saltstack.com/en/latest/topics/development/contributing.html) for best practices.


Problem statement:
file.recurse clean: True and requires not working together
### Description of Issue/Question
trying to help someone earlyer today i setup a test to make sure what i was saying would be true. 
my test consisted of 2 file.recurse states. one requiring the other. the one with the require also had clean: True

when i ran the state it ran as expecting creating the directory and populating it with both file.recurse items. however when i deleted an item from one of the source directories. it was not being deleted with a rerun of the state. 

remove the recurse and clean ran properly. deleted everything not in the single file.recurse [including the other file.recurse files]

readd the require and files were created properly. 
touch a file in the target directory and clean did not remove it. 

### Setup
2 states

```
recurse1:
  file.recurse:
    - name: /tmp/testing
    - source: salt://testing/files/recurse1

recurse2:
  file.recurse:
    - name: /tmp/testing
    - source: salt://testing/files/recurse2
    - clean: True
    - require:
      - file: recurse1
```

contents of sources
```
cd recurse1
[root@salt01 recurse1]# ls
a  b  c
[root@salt01 recurse1]# cd ..
[root@salt01 files]# cd recurse2
[root@salt01 recurse2]# ls
d  e
```
after the first run /tmp/testing will have a,b,c,d,e
if you then touch a file in /tmp/testing or delete a file out of either of the sources
that file will not be removed in the target on following runs of the state.
remove the require and everything works as it should [the first recursive runs finds everything is ok. the second deletes everything that doesn't belong to it.]


```
[root@salt01 testing]# touch file
[root@salt01 testing]# ls
a  b  c  d  e  file
[root@salt01 testing]# salt-call state.apply testing.test
local:
----------
          ID: recurse1
    Function: file.recurse
        Name: /tmp/testing
      Result: True
     Comment: The directory /tmp/testing is in the correct state
     Started: 12:13:11.322959
    Duration: 89.382 ms
     Changes:
----------
          ID: recurse2
    Function: file.recurse
        Name: /tmp/testing
      Result: True
     Comment: The directory /tmp/testing is in the correct state
     Started: 12:13:11.412760
    Duration: 33.839 ms
     Changes:

Summary for local
------------
Succeeded: 2
Failed:    0
------------
Total states run:     2
Total run time: 123.221 ms
[root@salt01 testing]# ls
a  b  c  d  e  file
[root@salt01 testing]#
```

debug output
```
[DEBUG   ] Results of YAML rendering:
OrderedDict([('recurse1', OrderedDict([('file.recurse', [OrderedDict([('name', '/tmp/testing')]), OrderedDict([('source', 'salt://testing/files/recurse1')])])])), ('recurse2', OrderedDict([('file.recurse', [OrderedDict([('name', '/tmp/testing')]), OrderedDict([('source', 'salt://testing/files/recurse2')]), OrderedDict([('clean', True)]), OrderedDict([('require', [OrderedDict([('file', 'recurse1')])])])])]))])
[PROFILE ] Time (in seconds) to render '/var/cache/salt/minion/files/base/testing/test.sls' using 'yaml' renderer: 0.0402438640594
[DEBUG   ] LazyLoaded file.recurse
[INFO    ] Running state [/tmp/testing] at time 12:19:34.749257
[INFO    ] Executing state file.recurse for /tmp/testing
[DEBUG   ] LazyLoaded file.source_list
[DEBUG   ] LazyLoaded cp.list_master
[DEBUG   ] Initializing new AsyncZeroMQReqChannel for ('/etc/salt/pki/minion', 'salt01.wolfcloud.bad4.us', 'tcp://10.40.0.105:4506', 'aes')
[DEBUG   ] Initializing new AsyncAuth for ('/etc/salt/pki/minion', 'salt01.wolfcloud.bad4.us', 'tcp://10.40.0.105:4506')
[DEBUG   ] In saltenv 'base', looking at rel_path 'testing/files/recurse1/a' to resolve 'salt://testing/files/recurse1/a'
[DEBUG   ] In saltenv 'base', ** considering ** path '/var/cache/salt/minion/files/base/testing/files/recurse1/a' to resolve 'salt://testing/files/recurse1/a'
[INFO    ] Fetching file from saltenv 'base', ** skipped ** latest already in cache 'salt://testing/files/recurse1/a', mode up-to-date
[DEBUG   ] In saltenv 'base', looking at rel_path 'testing/files/recurse1/b' to resolve 'salt://testing/files/recurse1/b'
[DEBUG   ] In saltenv 'base', ** considering ** path '/var/cache/salt/minion/files/base/testing/files/recurse1/b' to resolve 'salt://testing/files/recurse1/b'
[INFO    ] Fetching file from saltenv 'base', ** skipped ** latest already in cache 'salt://testing/files/recurse1/b', mode up-to-date
[DEBUG   ] In saltenv 'base', looking at rel_path 'testing/files/recurse1/c' to resolve 'salt://testing/files/recurse1/c'
[DEBUG   ] In saltenv 'base', ** considering ** path '/var/cache/salt/minion/files/base/testing/files/recurse1/c' to resolve 'salt://testing/files/recurse1/c'
[INFO    ] Fetching file from saltenv 'base', ** skipped ** latest already in cache 'salt://testing/files/recurse1/c', mode up-to-date
[INFO    ] The directory /tmp/testing is in the correct state
[INFO    ] Completed state [/tmp/testing] at time 12:19:34.837132 duration_in_ms=87.875
[INFO    ] Running state [/tmp/testing] at time 12:19:34.837584
[INFO    ] Executing state file.recurse for /tmp/testing
[DEBUG   ] In saltenv 'base', looking at rel_path 'testing/files/recurse2/d' to resolve 'salt://testing/files/recurse2/d'
[DEBUG   ] In saltenv 'base', ** considering ** path '/var/cache/salt/minion/files/base/testing/files/recurse2/d' to resolve 'salt://testing/files/recurse2/d'
[INFO    ] Fetching file from saltenv 'base', ** skipped ** latest already in cache 'salt://testing/files/recurse2/d', mode up-to-date
[DEBUG   ] In saltenv 'base', looking at rel_path 'testing/files/recurse2/e' to resolve 'salt://testing/files/recurse2/e'
[DEBUG   ] In saltenv 'base', ** considering ** path '/var/cache/salt/minion/files/base/testing/files/recurse2/e' to resolve 'salt://testing/files/recurse2/e'
[INFO    ] Fetching file from saltenv 'base', ** skipped ** latest already in cache 'salt://testing/files/recurse2/e', mode up-to-date
[INFO    ] The directory /tmp/testing is in the correct state
[INFO    ] Completed state [/tmp/testing] at time 12:19:34.908733 duration_in_ms=71.149
[DEBUG   ] File /var/cache/salt/minion/accumulator/35284880 does not exist, no need to cleanup.
[DEBUG   ] LazyLoaded config.option
```
### Versions Report

```
salt-call --versions
Salt Version:
           Salt: 2016.11.1

Dependency Versions:
           cffi: 1.9.1
       cherrypy: 8.1.2
       dateutil: 2.6.0
          gitdb: 0.6.4
      gitpython: Not Installed
          ioflo: 1.6.5
         Jinja2: 2.8
        libgit2: Not Installed
        libnacl: 1.5.0
       M2Crypto: Not Installed
           Mako: 1.0.6
   msgpack-pure: 0.1.3
 msgpack-python: 0.4.8
   mysql-python: 1.2.5
      pycparser: 2.17
       pycrypto: 2.6.1
         pygit2: Not Installed
         Python: 2.7.5 (default, Nov  6 2016, 00:28:07)
   python-gnupg: 0.3.9
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: 0.9.0
        timelib: 0.2.4
        Tornado: 4.4.2
            ZMQ: 4.1.6

System Versions:
           dist: centos 7.3.1611 Core
        machine: x86_64
        release: 3.10.0-327.10.1.el7.x86_64
         system: Linux
        version: CentOS Linux 7.3.1611 Core```

Hints from issue discussion:
I'm the guy he was helping :)

FWIW, I did a similar test and got a similar result on 2016.3.1. I also tried with the first file state as a file.managed instead of file.recurse, and that behaved correctly, retaining the file managed by the "required" state and removing an extra one added by hand.
@whytewolf looks like i'm able to replicate this.

here is a docker container for anyone that wants to quickly replicate this:

1. `docker run -it -v /home/ch3ll/git/salt/:/testing/ ch3ll/issues:38318` (where /home/ch3ll/git/salt is a local cloned git repo)
2. `touch /tmp/testing/removethis`
3. `salt-call --local state.sls test` (and it doesn't remove the file)
4. ` ls /tmp/testing/` and you will see teh file `removethis` still exists
5. if you remove the require statement in /srv/salt/test.sls then it will remove the file.

Looks like we need to get this fixed up. Thanks

Commit message: Move generation of managed files/dirs/symlinks in a recurse state into a new function, that is also used by  _gen_keep_files to get this list of files to keep from required recurse states.

Formatting fixes

Patch to review:
diff --git a/salt/states/file.py b/salt/states/file.py
index 96915736f400..74aeac00305e 100644
--- a/salt/states/file.py
+++ b/salt/states/file.py
@@ -350,6 +350,139 @@ def _check_user(user, group):
     return err
 
 
+def _gen_recurse_managed_files(
+        name,
+        source,
+        keep_symlinks=False,
+        include_pat=None,
+        exclude_pat=None,
+        maxdepth=None,
+        include_empty=False,
+        **kwargs):
+    '''
+    Generate the list of files managed by a recurse state
+    '''
+
+    # Process symlinks and return the updated filenames list
+    def process_symlinks(filenames, symlinks):
+        for lname, ltarget in six.iteritems(symlinks):
+            if not salt.utils.check_include_exclude(
+                    os.path.relpath(lname, srcpath), include_pat, exclude_pat):
+                continue
+            srelpath = os.path.relpath(lname, srcpath)
+            # Check for max depth
+            if maxdepth is not None:
+                srelpieces = srelpath.split('/')
+                if not srelpieces[-1]:
+                    srelpieces = srelpieces[:-1]
+                if len(srelpieces) > maxdepth + 1:
+                    continue
+            # Check for all paths that begin with the symlink
+            # and axe it leaving only the dirs/files below it.
+            # This needs to use list() otherwise they reference
+            # the same list.
+            _filenames = list(filenames)
+            for filename in _filenames:
+                if filename.startswith(lname):
+                    log.debug('** skipping file ** {0}, it intersects a '
+                              'symlink'.format(filename))
+                    filenames.remove(filename)
+            # Create the symlink along with the necessary dirs.
+            # The dir perms/ownership will be adjusted later
+            # if needed
+            managed_symlinks.add((srelpath, ltarget))
+
+            # Add the path to the keep set in case clean is set to True
+            keep.add(os.path.join(name, srelpath))
+        vdir.update(keep)
+        return filenames
+
+    managed_files = set()
+    managed_directories = set()
+    managed_symlinks = set()
+    keep = set()
+    vdir = set()
+
+    srcpath, senv = salt.utils.url.parse(source)
+    if senv is None:
+        senv = __env__
+    if not srcpath.endswith('/'):
+        # we're searching for things that start with this *directory*.
+        # use '/' since #master only runs on POSIX
+        srcpath = srcpath + '/'
+    fns_ = __salt__['cp.list_master'](senv, srcpath)
+
+    # If we are instructed to keep symlinks, then process them.
+    if keep_symlinks:
+        # Make this global so that emptydirs can use it if needed.
+        symlinks = __salt__['cp.list_master_symlinks'](senv, srcpath)
+        fns_ = process_symlinks(fns_, symlinks)
+
+    for fn_ in fns_:
+        if not fn_.strip():
+            continue
+
+        # fn_ here is the absolute (from file_roots) source path of
+        # the file to copy from; it is either a normal file or an
+        # empty dir(if include_empty==true).
+
+        relname = sdecode(os.path.relpath(fn_, srcpath))
+        if relname.startswith('..'):
+            continue
+
+        # Check for maxdepth of the relative path
+        if maxdepth is not None:
+            # Since paths are all master, just use POSIX separator
+            relpieces = relname.split('/')
+            # Handle empty directories (include_empty==true) by removing the
+            # the last piece if it is an empty string
+            if not relpieces[-1]:
+                relpieces = relpieces[:-1]
+            if len(relpieces) > maxdepth + 1:
+                continue
+
+        # Check if it is to be excluded. Match only part of the path
+        # relative to the target directory
+        if not salt.utils.check_include_exclude(
+                relname, include_pat, exclude_pat):
+            continue
+        dest = os.path.join(name, relname)
+        dirname = os.path.dirname(dest)
+        keep.add(dest)
+
+        if dirname not in vdir:
+            # verify the directory perms if they are set
+            managed_directories.add(dirname)
+            vdir.add(dirname)
+
+        src = salt.utils.url.create(fn_, saltenv=senv)
+        managed_files.add((dest, src))
+
+    if include_empty:
+        mdirs = __salt__['cp.list_master_dirs'](senv, srcpath)
+        for mdir in mdirs:
+            if not salt.utils.check_include_exclude(
+                    os.path.relpath(mdir, srcpath), include_pat, exclude_pat):
+                continue
+            mdest = os.path.join(name, os.path.relpath(mdir, srcpath))
+            # Check for symlinks that happen to point to an empty dir.
+            if keep_symlinks:
+                islink = False
+                for link in symlinks:
+                    if mdir.startswith(link, 0):
+                        log.debug('** skipping empty dir ** {0}, it intersects'
+                                  ' a symlink'.format(mdir))
+                        islink = True
+                        break
+                if islink:
+                    continue
+
+            managed_directories.add(mdest)
+            keep.add(mdest)
+
+    return managed_files, managed_directories, managed_symlinks, keep
+
+
 def _gen_keep_files(name, require, walk_d=None):
     '''
     Generate the list of files that need to be kept when a dir based function
@@ -406,9 +539,14 @@ def _process(name):
                 # another state.
                 if low['name'] == comp['file'] or low['__id__'] == comp['file']:
                     fn = low['name']
+                    fun = low['fun']
                     if os.path.isdir(fn):
                         if _is_child(fn, name):
-                            if walk_d:
+                            if fun == 'recurse':
+                                fkeep = _gen_recurse_managed_files(**low)[3]
+                                log.debug('Keep from {0}: {1}'.format(fn, fkeep))
+                                keep.update(fkeep)
+                            elif walk_d:
                                 walk_ret = set()
                                 _process_by_walk_d(fn, walk_ret)
                                 keep.update(walk_ret)
@@ -416,6 +554,7 @@ def _process(name):
                                 keep.update(_process(fn))
                     else:
                         keep.add(fn)
+    log.debug('Files to keep from required states: %s' % list(keep))
     return list(keep)
 
 
@@ -3018,123 +3157,31 @@ def manage_directory(path):
             require=None)
         merge_ret(path, _ret)
 
-    # Process symlinks and return the updated filenames list
-    def process_symlinks(filenames, symlinks):
-        for lname, ltarget in six.iteritems(symlinks):
-            if not salt.utils.check_include_exclude(
-                    os.path.relpath(lname, srcpath), include_pat, exclude_pat):
-                continue
-            srelpath = os.path.relpath(lname, srcpath)
-            # Check for max depth
-            if maxdepth is not None:
-                srelpieces = srelpath.split('/')
-                if not srelpieces[-1]:
-                    srelpieces = srelpieces[:-1]
-                if len(srelpieces) > maxdepth + 1:
-                    continue
-            # Check for all paths that begin with the symlink
-            # and axe it leaving only the dirs/files below it.
-            # This needs to use list() otherwise they reference
-            # the same list.
-            _filenames = list(filenames)
-            for filename in _filenames:
-                if filename.startswith(lname):
-                    log.debug('** skipping file ** {0}, it intersects a '
-                              'symlink'.format(filename))
-                    filenames.remove(filename)
-            # Create the symlink along with the necessary dirs.
-            # The dir perms/ownership will be adjusted later
-            # if needed
-            _ret = symlink(os.path.join(name, srelpath),
-                           ltarget,
-                           makedirs=True,
-                           force=force_symlinks,
-                           user=user,
-                           group=group,
-                           mode=sym_mode)
-            if not _ret:
-                continue
-            merge_ret(os.path.join(name, srelpath), _ret)
-            # Add the path to the keep set in case clean is set to True
-            keep.add(os.path.join(name, srelpath))
-        vdir.update(keep)
-        return filenames
-
-    keep = set()
-    vdir = set()
-    if not srcpath.endswith('/'):
-        # we're searching for things that start with this *directory*.
-        # use '/' since #master only runs on POSIX
-        srcpath = srcpath + '/'
-    fns_ = __salt__['cp.list_master'](senv, srcpath)
-    # If we are instructed to keep symlinks, then process them.
-    if keep_symlinks:
-        # Make this global so that emptydirs can use it if needed.
-        symlinks = __salt__['cp.list_master_symlinks'](senv, srcpath)
-        fns_ = process_symlinks(fns_, symlinks)
-    for fn_ in fns_:
-        if not fn_.strip():
-            continue
-
-        # fn_ here is the absolute (from file_roots) source path of
-        # the file to copy from; it is either a normal file or an
-        # empty dir(if include_empty==true).
-
-        relname = sdecode(os.path.relpath(fn_, srcpath))
-        if relname.startswith('..'):
-            continue
-
-        # Check for maxdepth of the relative path
-        if maxdepth is not None:
-            # Since paths are all master, just use POSIX separator
-            relpieces = relname.split('/')
-            # Handle empty directories (include_empty==true) by removing the
-            # the last piece if it is an empty string
-            if not relpieces[-1]:
-                relpieces = relpieces[:-1]
-            if len(relpieces) > maxdepth + 1:
-                continue
-
-        # Check if it is to be excluded. Match only part of the path
-        # relative to the target directory
-        if not salt.utils.check_include_exclude(
-                relname, include_pat, exclude_pat):
+    mng_files, mng_dirs, mng_symlinks, keep = _gen_recurse_managed_files(
+        name,
+        source,
+        keep_symlinks,
+        include_pat,
+        exclude_pat,
+        maxdepth,
+        include_empty)
+
+    for srelpath, ltarget in mng_symlinks:
+        _ret = symlink(os.path.join(name, srelpath),
+                       ltarget,
+                       makedirs=True,
+                       force=force_symlinks,
+                       user=user,
+                       group=group,
+                       mode=sym_mode)
+        if not _ret:
             continue
-        dest = os.path.join(name, relname)
-        dirname = os.path.dirname(dest)
-        keep.add(dest)
-
-        if dirname not in vdir:
-            # verify the directory perms if they are set
-            manage_directory(dirname)
-            vdir.add(dirname)
-
-        src = salt.utils.url.create(fn_, saltenv=senv)
+        merge_ret(os.path.join(name, srelpath), _ret)
+    for dirname in mng_dirs:
+        manage_directory(dirname)
+    for dest, src in mng_files:
         manage_file(dest, src)
 
-    if include_empty:
-        mdirs = __salt__['cp.list_master_dirs'](senv, srcpath)
-        for mdir in mdirs:
-            if not salt.utils.check_include_exclude(
-                    os.path.relpath(mdir, srcpath), include_pat, exclude_pat):
-                continue
-            mdest = os.path.join(name, os.path.relpath(mdir, srcpath))
-            # Check for symlinks that happen to point to an empty dir.
-            if keep_symlinks:
-                islink = False
-                for link in symlinks:
-                    if mdir.startswith(link, 0):
-                        log.debug('** skipping empty dir ** {0}, it intersects'
-                                  ' a symlink'.format(mdir))
-                        islink = True
-                        break
-                if islink:
-                    continue
-
-            manage_directory(mdest)
-            keep.add(mdest)
-
-    keep = list(keep)
     if clean:
         # TODO: Use directory(clean=True) instead
         keep += _gen_keep_files(name, require)


Code review comments:
diff_hunk: @@ -406,16 +539,22 @@ def _process(name):
                 # another state.
                 if low['name'] == comp['file'] or low['__id__'] == comp['file']:
                     fn = low['name']
+                    fun = low['fun']
                     if os.path.isdir(fn):
                         if _is_child(fn, name):
-                            if walk_d:
+                            if fun == 'recurse':
+                                fkeep = _gen_recurse_managed_files(**low)[3]
+                                log.debug('Keep from {0}: {1}'.format(fn, fkeep))
+                                keep.update(fkeep)
+                            elif walk_d:
                                 walk_ret = set()
                                 _process_by_walk_d(fn, walk_ret)
                                 keep.update(walk_ret)
                             else:
                                 keep.update(_process(fn))
                     else:
                         keep.add(fn)
+    log.debug('Files to keep from required states: %s' % list(keep))
path: salt/states/file.py
text: @user1:
The logging functions already perform string replacement using the single-asterisk expansion. Moreover, I think our pylint currently flags usage of the ``%`` operator. This line should be restated as:

```python
log.debug('Files to keep from required states: %s', list(keep))
```

diff_hunk: @@ -350,6 +350,139 @@ def _check_user(user, group):
     return err
 
 
+def _gen_recurse_managed_files(
+        name,
+        source,
+        keep_symlinks=False,
+        include_pat=None,
+        exclude_pat=None,
+        maxdepth=None,
+        include_empty=False,
+        **kwargs):
+    '''
+    Generate the list of files managed by a recurse state
+    '''
+
+    # Process symlinks and return the updated filenames list
+    def process_symlinks(filenames, symlinks):
+        for lname, ltarget in six.iteritems(symlinks):
+            if not salt.utils.check_include_exclude(
+                    os.path.relpath(lname, srcpath), include_pat, exclude_pat):
+                continue
+            srelpath = os.path.relpath(lname, srcpath)
+            # Check for max depth
+            if maxdepth is not None:
+                srelpieces = srelpath.split('/')
+                if not srelpieces[-1]:
+                    srelpieces = srelpieces[:-1]
+                if len(srelpieces) > maxdepth + 1:
+                    continue
+            # Check for all paths that begin with the symlink
+            # and axe it leaving only the dirs/files below it.
+            # This needs to use list() otherwise they reference
+            # the same list.
+            _filenames = list(filenames)
+            for filename in _filenames:
+                if filename.startswith(lname):
+                    log.debug('** skipping file ** {0}, it intersects a '
+                              'symlink'.format(filename))
+                    filenames.remove(filename)
+            # Create the symlink along with the necessary dirs.
+            # The dir perms/ownership will be adjusted later
+            # if needed
+            managed_symlinks.add((srelpath, ltarget))
+
+            # Add the path to the keep set in case clean is set to True
+            keep.add(os.path.join(name, srelpath))
+        vdir.update(keep)
+        return filenames
+
+    managed_files = set()
+    managed_directories = set()
+    managed_symlinks = set()
+    keep = set()
+    vdir = set()
+
+    srcpath, senv = salt.utils.url.parse(source)
+    if senv is None:
+        senv = __env__
+    if not srcpath.endswith('/'):
+        # we're searching for things that start with this *directory*.
+        # use '/' since #master only runs on POSIX
+        srcpath = srcpath + '/'
path: salt/states/file.py
text: @user1:
I know you didn't originally write this code, but since we'd like to get the master running on Windows eventually, we should probably use ``os.sep`` here. Could you make this change?

```python
if not srcpath.endswith(os.sep):
    srcpath += os.sep
```