Multiple issues with Saltclass variable expansion #48170

max-arnold · 2018-06-17T16:36:14Z

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.

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.

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

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.

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

The text was updated successfully, but these errors were encountered:

max-arnold · 2018-06-19T07:43:10Z

Cc: @olivier-mauras

olivier-mauras · 2018-06-19T08:03:28Z

@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 · 2018-06-19T10:05:11Z

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 · 2018-06-19T10:07:33Z

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 · 2018-06-19T10:26:17Z

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

pillars:
  users:
    unprivileged: []

key = 'unprivileged'

max-arnold · 2018-06-19T14:52:28Z

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 · 2018-06-19T19:13:03Z

ping @olivier-mauras can you take a look here

olivier-mauras · 2018-06-19T19:16:12Z

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 · 2018-06-20T06:29:07Z

@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 · 2018-06-20T06:38:02Z

@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 · 2018-07-26T19:18:08Z

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

max-arnold · 2018-07-27T14:55:17Z

@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:

This one Multiple issues with Saltclass variable expansion #48170
Performance regression (not a blocker, but quite annoying) Saltclass performance #48167
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 · 2018-10-27T13:58:52Z

Another one (Fluorine): #50262

stale · 2020-01-09T15:56:21Z

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.

Oloremo · 2020-01-09T16:04:18Z

not stale

stale · 2020-01-09T16:04:21Z

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

stale · 2020-02-08T16:37:14Z

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.

max-arnold · 2020-02-09T02:14:08Z

Bump

stale · 2020-02-09T02:14:11Z

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

twangboy · 2023-09-06T21:10:25Z

Closing this issue due to age and lack of activity. Please test this on version 3006.2 and create a new issue if the problem persists. The new issue template has more information and will allow us to track and reproduce the issue more effectively. Thanks!

Ch3LL added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jun 19, 2018

Ch3LL added this to the Blocked milestone Jun 19, 2018

a-a-abramov mentioned this issue Feb 4, 2019

Fix saltclass #51487

Closed

a-a-abramov mentioned this issue Apr 8, 2019

Large refactoring of SaltClass #52407

Closed

stale bot added the stale label Jan 9, 2020

stale bot removed the stale label Jan 9, 2020

stale bot added the stale label Feb 8, 2020

stale bot removed the stale label Feb 9, 2020

Ch3LL added Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around and removed Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged labels Feb 12, 2020

Ch3LL modified the milestones: Blocked, Approved Feb 12, 2020

Ch3LL added the P2 Priority 2 label Feb 12, 2020

sagetherage removed the P2 Priority 2 label Jun 3, 2020

twangboy closed this as completed Sep 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple issues with Saltclass variable expansion #48170

Multiple issues with Saltclass variable expansion #48170

max-arnold commented Jun 17, 2018

max-arnold commented Jun 19, 2018

olivier-mauras commented Jun 19, 2018 •

edited

Loading

max-arnold commented Jun 19, 2018

max-arnold commented Jun 19, 2018 •

edited

Loading

max-arnold commented Jun 19, 2018 •

edited

Loading

max-arnold commented Jun 19, 2018

Ch3LL commented Jun 19, 2018

olivier-mauras commented Jun 19, 2018

max-arnold commented Jun 20, 2018 •

edited

Loading

max-arnold commented Jun 20, 2018

ketzacoatl commented Jul 26, 2018

max-arnold commented Jul 27, 2018 •

edited

Loading

max-arnold commented Oct 27, 2018 •

edited

Loading

stale bot commented Jan 9, 2020

Oloremo commented Jan 9, 2020

stale bot commented Jan 9, 2020

stale bot commented Feb 8, 2020

max-arnold commented Feb 9, 2020

stale bot commented Feb 9, 2020

twangboy commented Sep 6, 2023

Multiple issues with Saltclass variable expansion #48170

Multiple issues with Saltclass variable expansion #48170

Comments

max-arnold commented Jun 17, 2018

Description of Issue/Question

Versions Report

max-arnold commented Jun 19, 2018

olivier-mauras commented Jun 19, 2018 • edited Loading

max-arnold commented Jun 19, 2018

max-arnold commented Jun 19, 2018 • edited Loading

max-arnold commented Jun 19, 2018 • edited Loading

max-arnold commented Jun 19, 2018

Ch3LL commented Jun 19, 2018

olivier-mauras commented Jun 19, 2018

max-arnold commented Jun 20, 2018 • edited Loading

Steps to reproduce

max-arnold commented Jun 20, 2018

ketzacoatl commented Jul 26, 2018

max-arnold commented Jul 27, 2018 • edited Loading

max-arnold commented Oct 27, 2018 • edited Loading

stale bot commented Jan 9, 2020

Oloremo commented Jan 9, 2020

stale bot commented Jan 9, 2020

stale bot commented Feb 8, 2020

max-arnold commented Feb 9, 2020

stale bot commented Feb 9, 2020

twangboy commented Sep 6, 2023

olivier-mauras commented Jun 19, 2018 •

edited

Loading

max-arnold commented Jun 19, 2018 •

edited

Loading

max-arnold commented Jun 19, 2018 •

edited

Loading

max-arnold commented Jun 20, 2018 •

edited

Loading

max-arnold commented Jul 27, 2018 •

edited

Loading

max-arnold commented Oct 27, 2018 •

edited

Loading