Salt: file.recurse breaks directory permissions

Created on 26 Jul 2016  Â·  18Comments  Â·  Source: saltstack/salt

Description of Issue/Question

A normal file.recurse state results in broken permissions of the managed directory.

Steps to Reproduce Issue

Use a state such as:

/home/foo/.hooks:
  file.recurse:
    - source: salt://users/files/hooks
    - user: foo
    - group: foo
    - dir_mode: 2755
    - file_mode: 755

which creates the following directory:

/home/foo# ls -lah
total 32K
drwxr-x--- 5 foo foo 4.0K Jul 26 09:00 .
drwxr-xr-x 4 root  root  4.0K Jul 26 08:59 ..
...
dr-x--xrwt 3 foo foo 4.0K Jul 26 09:00 .hooks
...

I had to add an explicit file.directory state for /home/foo/.hooks to get correct (and sensible) permissions.

Versions Report

# salt-minion --versions-report
Salt Version:
           Salt: 2016.3.1

Dependency Versions:
           cffi: 0.8.6
       cherrypy: Not Installed
       dateutil: 2.2
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.3
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.2
   mysql-python: 1.2.3
      pycparser: 2.10
       pycrypto: 2.6.1
         pygit2: Not Installed
         Python: 2.7.9 (default, Mar  1 2015, 12:57:24)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 14.4.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.5

System Versions:
           dist: debian 8.5 
        machine: x86_64
        release: 3.16.0-4-amd64
         system: Linux
        version: debian 8.5

# salt --versions-report
Salt Version:
           Salt: 2016.3.1

Dependency Versions:
           cffi: 0.8.6
       cherrypy: Not Installed
       dateutil: 2.2
          gitdb: 0.5.4
      gitpython: 0.3.2 RC1
          ioflo: Not Installed
         Jinja2: 2.7.3
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.2
   mysql-python: 1.2.3
      pycparser: 2.10
       pycrypto: 2.6.1
         pygit2: Not Installed
         Python: 2.7.9 (default, Mar  1 2015, 12:57:24)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 14.4.0
           RAET: Not Installed
          smmap: 0.8.2
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.5

System Versions:
           dist: debian 8.5 
        machine: x86_64
        release: 3.16.0-4-amd64
         system: Linux
        version: debian 8.5
Bug State Module ZRELEASED - 2015.8.12 ZRELEASED - 2016.3.3
Source
picture babilen5

All 18 comments

@babilen i'm not able to replicate this on centos7 salt 2016.3.1:

 ch3ll@ch3ll-master î‚° /srv/salt î‚° ls -lah /home/foo/
total 12K
drwxr-sr-x.  3 ch3ll ch3ll 4.0K Jul 27 11:27 .
drwxr-xr-x. 11 root  root  4.0K Jul 27 11:36 ..
drwxr-sr-x.  2 ch3ll ch3ll 4.0K Jul 27 11:27 .hooks

Using the state:

/home/.hooks:
  file.recurse:
    - source: salt://test1/hooks
    - user: ch3ll
    - group: ch3ll
    - dir_mode: 2755
    - file_mode: 755

Anything i'm missing? Are you running this with a pre-existing directory maybe? Anything that would help me to replicate this. thanks

Ch3LL picture Ch3LL on 27 Jul 2016

Thank you for testing this, @Ch3LL !

The only striking difference is that I run it on Debian and not CentOS, but that really shouldn'tâ„¢ make a difference. I'll look into this again, but I've seen this both on vagrant boxes and actual VMs, so I doubt that it is specific to my testing setup.

babilen5 picture babilen5 on 27 Jul 2016

So, I managed to reproduce it. This _only_ happens if the directory doesn't exist yet and multiple directories are being created (i.e. files being synced are found in nested directories).

The exact configuration I used was:

# cat /srv/salt/salttest/top.sls 
base:
  '*':
    - recurse_bug

# cat /srv/salt/salttest/recurse_bug.sls 
/home/vagrant/.hooks:
  file.recurse:
    - source: salt://users/files/hooks
    - user: vagrant
    - group: vagrant
    - dir_mode: 2755
    - file_mode: 755

# find /srv/salt/salttest/users/      
/srv/salt/salttest/users/
/srv/salt/salttest/users/files
/srv/salt/salttest/users/files/hooks
/srv/salt/salttest/users/files/hooks/foo
/srv/salt/salttest/users/files/hooks/foo/bar
/srv/salt/salttest/users/files/hooks/foo/bar/baz
/srv/salt/salttest/users/files/hooks/foo/bar/baz/some_file

# ls -lah ~vagrant/.hooks/
total 12K
dr-x--xrwt 3 vagrant vagrant 4.0K Jul 28 08:27 .
drwxr-xr-x 4 vagrant vagrant 4.0K Jul 28 08:31 ..
drwxr-sr-x 3 vagrant vagrant 4.0K Jul 28 08:27 foo

I find it surprising that only the managed directory (i.e. the name argument) shows the broken permissions and not the other.

babilen5 picture babilen5 on 28 Jul 2016

@terminalmage Didn't you work on file.recurse recently?

babilen5 picture babilen5 on 2 Aug 2016

@babilen In develop only.

terminalmage picture terminalmage on 2 Aug 2016

@terminalmage The behaviour is the same in develop. I've just tested this against git HEAD (i.e. 2ad9efe47a806a671749fed71bcc0d7a92c98138) and was able to reproduce it there also.

Do you see something different?

babilen5 picture babilen5 on 3 Aug 2016

Well, that would be expected, since we branched 2016.3 from develop originally, and any changes in the 2016.3 branch are merged forward into the develop branch.

I didn't try to replicate this, I was only answering your question.

terminalmage picture terminalmage on 3 Aug 2016

Ah, I was just under the impression that you would be the best person to ask about this. Sorry if that wasn't the case.

babilen5 picture babilen5 on 3 Aug 2016

I am familiar enough with the code to address it, but I probably won't be able to get to it for a couple days. I'm juggling a few other issues at the moment.

terminalmage picture terminalmage on 3 Aug 2016

I wasn't able to reproduce this on any of the following:

  • 2016.3.1
  • Head of 2016.3 branch
  • Head of develop branch

Do you have an ACL set on /home/vagrant? What is the output of getfacl /home/vagrant? It's possible that a bad ACL is causing the directory to be created with those permissions, and Salt is only checking/fixing the mode for directories under name.

terminalmage picture terminalmage on 5 Aug 2016

@terminalmage Thank you for looking into this. I am not entirely sure why you can't reproduce it, which is why I am providing a complete example. It is using libvirt, but I'm sure that you can use vagrantbox too.

  1. Create the following Vagrantfile and salt/minion files:
$ cat Vagrantfile
#  -*- mode: ruby -*-
# vi: set ft=ruby :

VAGRANTFILE_API_VERSION = "2"

Vagrant.configure(VAGRANTFILE_API_VERSION) do |config|

  config.vm.box = "debian/jessie64"

  config.vm.provider :libvirt do |libvirt|
    libvirt.memory = "1024"
  end

  config.vm.define :salttest do |salttest|
    salttest.vm.hostname = "recurse-bug.test"
    salttest.vm.provision "shell", inline: "apt-get -qq install ca-certificates curl"

    salttest.vm.provision :salt do |salt|
      salt.install_type = "stable"
      salt.install_master = true
      salt.minion_config = "salt/minion"
    end
  end
end

$ cat salt/minion 
master: recurse-bug.test
  1. Run vagrant up, vagrant ssh and sudo -i
  2. Accept the minion key with salt-key -A → "Y"
  3. Create directories and a testfile with:
root@recurse-bug:~# mkdir -p /srv/salt/users/files/hooks/foo/bar/baz
root@recurse-bug:~# touch /srv/salt/users/files/hooks/foo/bar/baz/some_file
  1. Create suitable SLS and topfile
$ cat /srv/salt/top.sls 
base:
  '*':
    - recurse_bug

$ cat /srv/salt/recurse_bug.sls 
/home/vagrant/hooks:
  file.recurse:
    - source: salt://users/files/hooks
    - user: vagrant
    - group: vagrant
    - dir_mode: 2755
    - file_mode: 755
  1. Verfify that the directory structure has not been copied over:
root@recurse-bug:~# ls -lah /home/vagrant/
total 12K
drwxr-xr-x 3 vagrant vagrant 4.0K Jun  6 18:17 .
drwxr-xr-x 3 root    root    4.0K Jun  6 18:17 ..
drwx------ 2 vagrant vagrant 4.0K Aug  9 11:51 .ssh
  1. Run the highstate
root@recurse-bug:~# salt '*' state.highstate
recurse-bug.test:
----------
          ID: /home/vagrant/hooks
    Function: file.recurse
      Result: True
     Comment: Recursively updated /home/vagrant/hooks
     Started: 12:03:39.450219
    Duration: 39.3 ms
     Changes:   
              ----------
              /home/vagrant/hooks/foo/bar/baz:
                  ----------
                  /home/vagrant/hooks/foo/bar/baz:
                      New Dir
              /home/vagrant/hooks/foo/bar/baz/some_file:
                  ----------
                  diff:
                      New file
                  mode:
                      0755
                  user:
                      vagrant

Summary for recurse-bug.test
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
  1. Verify that the behaviour as reported in this bug has been reproduced:
root@recurse-bug:~# ls -lah /home/vagrant/
total 16K
drwxr-xr-x 4 vagrant vagrant 4.0K Aug  9 12:03 .
drwxr-xr-x 3 root    root    4.0K Jun  6 18:17 ..
drwx------ 2 vagrant vagrant 4.0K Aug  9 11:51 .ssh
dr-x--xrwt 3 vagrant vagrant 4.0K Aug  9 12:03 hooks

I would be quite surprised if you are seeing different behaviour. I also tested this against different salt versions and git HEADs. It would be interesting to know what you did exactly when you tried to reproduce it.

babilen5 picture babilen5 on 9 Aug 2016

Oh and you wanted to see the following:

root@recurse-bug:~# getfacl /home/vagrant/
getfacl: Removing leading '/' from absolute path names
# file: home/vagrant/
# owner: vagrant
# group: vagrant
user::rwx
group::r-x
other::r-x

which is not unusual in any way. The directory that exhibits the buggy behaviour looks like:

root@recurse-bug:~# getfacl /home/vagrant/hooks/
getfacl: Removing leading '/' from absolute path names
# file: home/vagrant/hooks/
# owner: vagrant
# group: vagrant
# flags: --t
user::r-x
group::--x
other::rwx
babilen5 picture babilen5 on 9 Aug 2016

Yeah, I was just guessing on the facl, there are no default entries in the output so that rules that out.

Just built your vagrant box and working on testing now. The only difference in my SLS was that I did not use a user or group parameter, I'll step through with a debugger and see if that mattered at all (I can't see why it would).

terminalmage picture terminalmage on 9 Aug 2016

Was able to reproduce in vagrant box, commencing step-through with debugger.

terminalmage picture terminalmage on 9 Aug 2016

OK, I was finally able to reproduce on my laptop, outside of your vagrant example... It turns out that this only occurs when the source dir does not have any files in it (only subdirectories). My setup contained files in the source dir as well as subdirectories. What is happening here is that we end up calling file.check_perms with a malformed mode the first time around (one converted to octal when it was already octal in the first place). When there are files in the source dir, however, we end up executing the file.directory state within file.recurse, which corrects the mode which was initially set incorrectly.

This bug exists back in the 2015.8 branch and likely going back much further, it's just that most use cases for file.recurse don't involve the source dir being devoid of actual files, so not enough people hit this edge case to notice it until now.

I'm working on a fix and an integration test now, should have it done by the end of the day. Thanks for your help in troubleshooting. In retrospect I would have been able to reproduce this much sooner if I had duplicated your example from several days ago, but I have a file.recurse test directory in my file_roots that I typically use for troubleshooting bug reports like this, and I did have several levels of directories being created, so it seemed to match your case well enough. It was only when I started stepping through with a debugger on both my laptop and your vagrant box to see how the cases differed that I noticed the discrepancy.

terminalmage picture terminalmage on 9 Aug 2016

Fixed in https://github.com/saltstack/salt/pull/35309, integration test included.

terminalmage picture terminalmage on 9 Aug 2016

Thank you very much!

babilen5 picture babilen5 on 9 Aug 2016
👍1

No prob! Thank you for helping me troubleshoot, and for helping uncover a corner case that looks to have been introduced nearly _4 years ago_. (!!!)

:+1: to @babilen

terminalmage picture terminalmage on 9 Aug 2016
👍1
Was this page helpful?
0 / 5 - 0 ratings

Related issues

golmaal picture golmaal  Â·  3Comments
qiushics picture qiushics  Â·  3Comments
zieba88 picture zieba88  Â·  3Comments
icycle77 picture icycle77  Â·  3Comments
sagetherage picture sagetherage  Â·  3Comments