Salt: [REGRESSION] file.managed: state.apply "'Changes' should be a dictionary" error and unuseful output if invalid 'source' file path is specified

[9numbernine9]

Description of Issue

Hello!

In Salt 2019.2.2 (and likely any 2019.2.x version), when using the file.managed state and specifying a source file that doesn't actually exist (e.g. source: salt://path/to/nonexistent/file.txt), state.apply will return a "'Changes' should be a dictionary" error instead of returning the _real_ error, i.e. that I'm trying to use a file that doesn't exist in SaltFS. This is a break from 2018.3.x which actually reports a useful Source file not found error.

Setup

[root@localhost salt]# pwd
/tmp/salt

[root@localhost salt]# ls
state.sls

[root@localhost salt]# cat state.sls
/tmp/salt/target_file.txt:
  file.managed:
    - source: salt://not_a_file.txt

Steps to Reproduce Issue

Output when using Salt 2019.2.2:

[root@localhost salt]# salt-call --local --file-root=/tmp/salt/ state.apply state test=True
[ERROR   ] (False, u"Source file salt://not_a_file.txt not found in saltenv 'base'")
local:
The State execution failed to record the order in which all states were executed. The state return missing data is:
{u'changes': {},
 u'comment': u"An exception occurred in this state: 'Changes' should be a dictionary.",
 u'name': u'later',
 u'result': False}
----------
          ID: /tmp/salt/target_file.txt
    Function: file.managed
      Result: False
     Comment: An exception occurred in this state: 'Changes' should be a dictionary.
     Changes:

Summary for local
------------
Succeeded: 0
Failed:    1
------------
Total states run:     1
Total run time:   0.000 ms

When using Salt 2018.3.4 the output is much more useful:

[root@localhost salt]# salt-call --local --file-root=/tmp/salt/ state.apply state test=True
[ERROR   ] Source file salt://not_a_file.txt not found
local:
----------
          ID: /tmp/salt/target_file.txt
    Function: file.managed
      Result: False
     Comment: Source file salt://not_a_file.txt not found
     Started: 11:00:08.781896
    Duration: 26.166 ms
     Changes:   Invalid Changes data: (False, u'Source file salt://not_a_file.txt not found')

Summary for local
------------
Succeeded: 0 (changed=1)
Failed:    1
------------
Total states run:     1
Total run time:  26.166 ms

Versions Report

Salt Version:
           Salt: 2019.2.2

Dependency Versions:
           cffi: 1.6.0
       cherrypy: Not Installed
       dateutil: 1.5
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: 2.14
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.5 (default, Aug  7 2019, 00:51:29)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.3.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4

System Versions:
           dist: centos 7.7.1908 Core
         locale: UTF-8
        machine: x86_64
        release: 3.10.0-1062.4.1.el7.x86_64
         system: Linux
        version: CentOS Linux 7.7.1908 Core

[xeacott]

Thanks for reporting this! I'm thinking that there may have been a code path that has been introduced which leads to reporting this error, or possibly the error message has changed. But I agree, the error message should have a better message.

[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.

[9numbernine9]

Please keep this issue open, stale-bot. :-)

[stale[bot]]

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

[sagetherage]

At this time we are unable to allocate resources to this issue, and are planning for the next release cycle to plan and commit.

[sagetherage]

same issue with a different module: https://github.com/saltstack/salt/issues/55562 - need to fix this broadly, may need input validation, looking to correct this in the Magnesium release

[zbukhari]

Just to stave off stale bot mainly.

We're running into this as well. We use salt-ssh for a state.apply test=True prior to making our changes live and it's a part of our validation which normally works pretty well.

Hope that helps. We're on 2019.2.4.

$ salt-ssh --versions-report
Salt Version:
           Salt: 2019.2.4

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.4.2
      docker-py: 1.9.0
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.8
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.5.2 (default, Apr 16 2020, 17:47:17)
   python-gnupg: 0.3.8
         PyYAML: 3.11
          PyZMQ: 15.2.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4

System Versions:
           dist: Ubuntu 16.04 xenial
         locale: UTF-8
        machine: x86_64
        release: 4.15.0-99-generic
         system: Linux
        version: Ubuntu 16.04 xenial

Earlier version of this message said the file was there, it wasn't but the error message changing would be useful :-)

[sagetherage]

@zbukhari stale bot was ~beaten into the ground and~ disabled earlier in the year :D and yes, we are looking into this and the salt-ssh state.apply test=True issue for the next major release Magnesium, target date in the fall 2020.

[waynew]

@zbukhari do you know if this issue is present in the most recent 3001 release? I haven't gone looking, but I have a feeling like I've fixed this (or wrote tests for it) somewhat recently :suspect:

[waynew]

Ahhhhhh. I did write some tests: https://github.com/waynew/salt/commit/772fe25beae573587efcc6c3d3d126e3616bad07

I'm guessing that a similar approach needs to happen here. @sagetherage we might be able to do some automagic smoke testing here, but I think fundamentally it's just that certain things are returning the wrong kind of data. There may be someone with a good idea about an approach here, but at least at the moment, I've got nothing.

FWIW contracts in pop/idem would solve this problem 😂

[sagetherage]

We have to de-scope this for Magnesium, you can see the pop projects in GitLab: https://gitlab.com/saltstack/pop and there are many idem projects, and I am unsure exactly but at least this link gives you a place to start.

[sastorsl]

Still an issue in salt-2019.2.7

[mariusvw]

While searching for a solution fo the same error I ran into this issue.
Came up with the solution to use onlyif to first check if the source file exists.

copy_somefile:
  file.managed:
    - name: /srv/www/somefile
    - source: /srv/tmp/somefile.dist
    - mode: 644
    - onlyif:
      - fun: file.file_exists
        path: /srv/tmp/somefile.dist