Multiple issues with Saltclass variable expansion

[max-arnold]

Description of Issue/Question

I spent more than 2 days trying to migrate existing reclass tree to saltclass (introduced in #42349) and found multiple issues with saltclass variable expansion.

1. Variable expansion doesn't work at all under Python 2.7

The reason is that checking for str is not enough, strings can also be unicode with 2.7: https://github.com/saltstack/salt/blob/2018.3.1/salt/utils/saltclass.py#L137

This feature definitely needs more unit tests. Jenkins builds for 2.7 can catch issues like this.

2. Not all variables are expanded

Salt 2017.7, reclass:

% salt-call --id example.com pillar.data
local:
    ----------
    __reclass__:
        ----------
        applications:
            - consul
        classes:
            - consul
            - consul.agent
            - site.mydc
        environment:
            base
        nodename:
            example.com
    consul:
        ----------
        agent:
            ----------
            args:
                - datacenter=mydc
                - join=1.2.3.4
        cur_version:
            1.0.3
        dc:
            ----------
            name:
                mydc
            servers:
                1.2.3.4
        versions:
            ----------
            1.0.3:
                ----------
                source:
                    https://releases.hashicorp.com/consul/1.0.3/consul_1.0.3_linux_amd64.zip
                source_hash:
                    sha256=4782e4662de8effe49e97c50b1a1233c03c0026881f6c004144cc3b73f446ec5

Salt 2018.3.1, saltclass

local:
    ----------
    __saltclass__:
        ----------
        classes:
            - consul
            - consul.agent
            - site.mydc
        environment:
        nodename:
            example.com
        states:
            - consul
    consul:
        ----------
        agent:
            ----------
            args:
                - datacenter=${consul:dc:name}
                - join=${consul:dc:servers}
        cur_version:
            1.0.3
        versions:
            ----------
            1.0.3:
                ----------
                source:
                    https://releases.hashicorp.com/consul/1.0.3/consul_1.0.3_linux_amd64.zip
                source_hash:
                    sha256=4782e4662de8effe49e97c50b1a1233c03c0026881f6c004144cc3b73f446ec5

Minimal state tree (both for reclass and saltclass) to reproduce: example.zip

3. Multiple variables on a single line probably won't be expanded

Variable parsing code tells me that things like one below won't work:

rules:
  - '-A PREROUTING -s 1.2.3.4/32 -d ${ipv4:wan} -p tcp --dport 1234 ${_fw_:dnat} 5.6.7.8:1234'

It worked just fine with reclass.

4. Missing variables are rendered as is, which is hard to debug and potentially dangerous

I spent hours trying to understand why Jinja rendering fails with Jinja variable 'str object' has no attribute 'get'. We use variable expansion for pillar subtrees and due to issue (2) it returned unexpanded variable as str instead of dict.

It is essential to hard fail on missing variables (and classes). If necessary, I will create a separate issue for this.

Any typo in a variable or class name should lead to failure with an appropriate error message; otherwise, it will eventually lead to silent failures in production. Failing hard is also useful for simple automated linting/testing.

So far I'm not ready to put Saltclass to production. Instead I fixed reclass to make it work with Salt 2018.3: madduck/reclass#83

Versions Report

salt --versions-report

Salt Version: Salt: 2018.3.1

Dependency Versions:
cffi: Not Installed
cherrypy: Not Installed
dateutil: Not Installed
docker-py: Not Installed
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.5.6
mysql-python: Not Installed
pycparser: Not Installed
pycrypto: 2.6.1
pycryptodome: Not Installed
pygit2: Not Installed
Python: 2.7.15 (default, May 2 2018, 00:53:27)
python-gnupg: Not Installed
PyYAML: 3.11
PyZMQ: 17.0.0
RAET: Not Installed
smmap: Not Installed
timelib: Not Installed
Tornado: 4.5.3
ZMQ: 4.1.6

System Versions:
dist:
locale: UTF-8
machine: x86_64
release: 17.6.0
system: Darwin
version: 10.13.5 x86_64

[max-arnold]

Cc: @olivier-mauras

[olivier-mauras]

@max-arnold this has been reported in #47634

PR has been merged

For hard fail I disagree with that, but you can open an issue for it so we can discuss that point. Now having an option to hard fail on missing class/variable sounds fair to me

[max-arnold]

Unfortunately, I can't test this with the latest develop branch. It fails with the following exception:

  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 219, in expand_classes_in_order
    classes_to_expand)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 219, in expand_classes_in_order
    classes_to_expand)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 225, in expand_classes_in_order
    classes_to_expand)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 209, in expand_classes_in_order
    dict_merge(salt_data['__pillar__'], expanded_classes[klass].get('pillars', {}))
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 94, in dict_merge
    dict_merge(a[key], b[key], path + [six.text_type(key)])
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 88, in dict_merge
    if b[key][0] == '^':

[CRITICAL] Pillar render error: Failed to load ext_pillar saltclass: list index out of range

[max-arnold]

Also, is it normal that during salt-call --id example.com pillar.data the function saltclass.py", line 62, ext_pillar() is called twice?

First invocation

  /Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/minion.py(832)gen_modules()
    831             self.opts['saltenv'],
--> 832             pillarenv=self.opts.get('pillarenv'),
    833         ).compile_pillar()

  /Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/__init__.py(997)compile_pillar()
    996                 pillar, errors = self.render_pillar(matches)
--> 997                 pillar, errors = self.ext_pillar(pillar, errors=errors)
    998         else:

> /Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/__init__.py(901)ext_pillar()
    900         import ipdb; ipdb.set_trace()
--> 901         if errors is None:
    902             errors = []

Second invocation

  /Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/modules/pillar.py(278)items()
    277
--> 278     return pillar.compile_pillar()
    279

  /Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/__init__.py(997)compile_pillar()
    996                 pillar, errors = self.render_pillar(matches)
--> 997                 pillar, errors = self.ext_pillar(pillar, errors=errors)
    998         else:

> /Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/__init__.py(901)ext_pillar()
    900         import ipdb; ipdb.set_trace()
--> 901         if errors is None:
    902             errors = []

[max-arnold]

@olivier-mauras This if b[key][0] == '^': exception above is triggered on the following class:

pillars:
  users:
    unprivileged: []

key = 'unprivileged'

[max-arnold]

I asked a question on SaltStack Slack group regarding this double call. It seems to be ok:

https://docs.saltstack.com/en/latest/topics/pillar/#in-memory-pillar-data-vs-on-demand-pillar-data

`pillar.data` is an alias for `pillar.items`
which compiles fresh pillar data
the first compilation compiles the "in-memory" pillar data, but `pillar.items` is designed specifically to compile fresh data because you can feed it different pillar environment values that would affect the data compiled
Keep in mind that "in-memory" pillar data is mostly designed for the minion daemon
but salt-call is its own separate process, so it has to compile that "in-memory" pillar data each time
If you are not passing any kind of special pillar environments, you should use `pillar.get`, which returns from the in-memory pillar data
and would thus not compile pillar twice

[Ch3LL]

ping @olivier-mauras can you take a look here

[olivier-mauras]

Yes but sadly I can't reproduce it :(
@max-arnold Would you mind giving me a minimal dataset - class + node - that make this crash happen? Adding a empty list in my test dataset doesn't raise anything :(

[max-arnold]

@olivier-mauras Sorry, my previous example was incomplete.

crashes.zip - reclass folder is for Salt 2017.7 and reclass, saltclass is for the latest Salt develop branch.

Steps to reproduce

This is how it should be rendered (Salt 2017.7, reclass):

% salt-call --id example.com pillar.data
local:
    ----------
    __reclass__:
        ----------
        applications:
        classes:
            - crontab
            - global
            - application.crontab
        environment:
            base
        nodename:
            example.com
    _project_:
        ----------
        upstream:
            - 127.0.0.1:1234
    cron:
        ----------
        enabled:
            - blah
    upstreams:
        - 127.0.0.1:1234
    users:
        ----------
        unprivileged:
            - user

And this is what happens with Salt develop branch:

% salt-call --id example.com pillar.data
[ERROR   ] Execption caught loading ext_pillar 'saltclass':
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/__init__.py", line 954, in ext_pillar
    key)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/__init__.py", line 882, in _external_pillar_data
    *val)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/saltclass.py", line 62, in ext_pillar
    return sc.get_pillars(minion_id, salt_data)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 306, in get_pillars
    states_list) = expanded_dict_from_minion(minion_id, salt_data)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 286, in expanded_dict_from_minion
    salt_data, [], {}, [])
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 225, in expand_classes_in_order
    classes_to_expand)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 219, in expand_classes_in_order
    classes_to_expand)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 209, in expand_classes_in_order
    dict_merge(salt_data['__pillar__'], expanded_classes[klass].get('pillars', {}))
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 94, in dict_merge
    dict_merge(a[key], b[key], path + [six.text_type(key)])
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 88, in dict_merge
    if b[key][0] == '^':

[CRITICAL] Pillar render error: Failed to load ext_pillar saltclass: list index out of range
[ERROR   ] Execption caught loading ext_pillar 'saltclass':
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/__init__.py", line 954, in ext_pillar
    key)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/__init__.py", line 882, in _external_pillar_data
    *val)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/saltclass.py", line 62, in ext_pillar
    return sc.get_pillars(minion_id, salt_data)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 306, in get_pillars
    states_list) = expanded_dict_from_minion(minion_id, salt_data)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 286, in expanded_dict_from_minion
    salt_data, [], {}, [])
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 225, in expand_classes_in_order
    classes_to_expand)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 219, in expand_classes_in_order
    classes_to_expand)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 209, in expand_classes_in_order
    dict_merge(salt_data['__pillar__'], expanded_classes[klass].get('pillars', {}))
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 94, in dict_merge
    dict_merge(a[key], b[key], path + [six.text_type(key)])
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 88, in dict_merge
    if b[key][0] == '^':

[CRITICAL] Pillar render error: Failed to load ext_pillar saltclass: list index out of range
local:
    ----------
    _errors:
        - Failed to load ext_pillar saltclass: list index out of range
    cron:
        ----------
        enabled:
            - blah
    users:
        ----------
        unprivileged:
            - user

And while reducing my reclass tree to the minimal reproducible state, I found another crash:

% mv saltclass/classes/application saltclass/classes/application.disabled

% salt-call --id example.com pillar.data

[WARNING ] application.crontab: Class definition not found
[ERROR   ] Execption caught loading ext_pillar 'saltclass':
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/__init__.py", line 954, in ext_pillar
    key)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/__init__.py", line 882, in _external_pillar_data
    *val)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/saltclass.py", line 62, in ext_pillar
    return sc.get_pillars(minion_id, salt_data)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 314, in get_pillars
    pillars_dict_expanded = expand_variables(pillars_dict['pillars'], {}, [])
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 186, in expand_variables
    b = find_and_process_re(v, v, k, b, expanded)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 158, in find_and_process_re
    v_new = v.replace(re_str, v_expanded)

[CRITICAL] Pillar render error: Failed to load ext_pillar saltclass: coercing to Unicode: need string or buffer, list found
[WARNING ] application.crontab: Class definition not found
[ERROR   ] Execption caught loading ext_pillar 'saltclass':
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/__init__.py", line 954, in ext_pillar
    key)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/__init__.py", line 882, in _external_pillar_data
    *val)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/pillar/saltclass.py", line 62, in ext_pillar
    return sc.get_pillars(minion_id, salt_data)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 314, in get_pillars
    pillars_dict_expanded = expand_variables(pillars_dict['pillars'], {}, [])
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 186, in expand_variables
    b = find_and_process_re(v, v, k, b, expanded)
  File "/Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py", line 158, in find_and_process_re
    v_new = v.replace(re_str, v_expanded)

[CRITICAL] Pillar render error: Failed to load ext_pillar saltclass: coercing to Unicode: need string or buffer, list found
local:
    ----------
    _errors:
        - Failed to load ext_pillar saltclass: coercing to Unicode: need string or buffer, list found
    _project_:
        ----------
        upstream:
            - 127.0.0.1:1234
    cron:
        ----------
        enabled:
            - blah
    upstreams:
        ${_project_:upstream}
    users:
        ----------
        unprivileged:
            - user

[max-arnold]

@olivier-mauras Would you mind joining the Slack group? http://saltstackcommunity.herokuapp.com/

I'd like to discuss a few other issues I found, before filing them on Github.

[ketzacoatl]

@max-arnold have you been able to use saltclass in your deployment, or are you still using reclass?

[max-arnold]

@ketzacoatl Yes, I was able to use saltclass, but only for trivial purposes (new state tree from scratch with ~10 states and 2 nodes). Was bitten multiple times by silent failures on missing variable expansion.

I was unable to migrate the production install mentioned above (~300 classes, ~80 nodes, ~90 states). The issues so far:

1. This one Multiple issues with Saltclass variable expansion #48170
2. Performance regression (not a blocker, but quite annoying) Saltclass performance #48167
3. A crash Crash while applying state to new minion with saltclass enabled #48434

I still hope that eventually, we will have a robust and well maintained reclass implementation in the core and I'm willing to help with testing once the issues are resolved.

In the meantime I patched the original issue with reclass madduck/reclass#83 and @epcim ported it salt-formulas/reclass#49 to the maintained reclass fork which I recommend to use for now: https://github.com/salt-formulas/reclass/

[max-arnold]

Another one (Fluorine): #50262