fix(map.jinja): use pillar/config .get according to __cli option

[myii]

• Fix 5dc0b86 in refactor(map): use config.get instead of pillar.get #95
  • No option merge=True for config.get
• Use pillar.get for salt-call (i.e. salt-ssh)
• Use config.get via. defaults.merge otherwise
  • Reintroduce based on 775a930 in update formula with map.jinja and style guide references, improve REA… #20

---

Update: The whole method of fixing this changed completely by the end of this PR.

---

Apologies for seeking multiple reviews again but map.jinja is the core of our formulas and this is a significant change, merging the map based on the type of minion. In #95 (comment), I mentioned that it appeared to be either one or the other:

1. Use config.get via. defaults.merge -- get the benefit but leave salt-ssh behind.
2. Use pillar.get -- works for everyone but limited to pillar only.

But in #21 (comment), @alxwr mentioned that there was a way around this:

AFAIR we could work around the defaults.merge issue, because we can detect whether salt or salt-ssh is used. But this is not pretty. :-)

Asked on Slack:

Imran Iqbal
I've just been informed that there's a way of detecting that salt-ssh is being used. Anyone know how to do that?
Imran Iqbal
__cli?
Daniel Wallace
that might be correct, but i don’t know if it would have salt-call on the minion side
@​Gareth J. Greenaway didn’t you have to do something with this for the salt-ssh + vault?
Gareth J. Greenaway
@​gtmanfred yeah. I think there is something in __opts__ iirc, possibly client

Resulting in this PR, which constructs the map conditionally:

1. salt-call (i.e. salt-ssh): Use the recent map.jinja method of usingpillar.get.
2. Otherwise (i.e. salt-minion): Use config.get via. defaults.merge.

[alxwr]

AFAIK salt-ssh works with config.get. No need to distinguish between the two.
(defaults.merge is the one which does/did not work with salt-ssh, IIRC.)

My suggested change is to keep the code as DRY as possible and abstain from defaults.merge.

Edit:
Just understood from #95 (comment) that config.get lacks merge=True.
I'm thinking of solving this with grains.filter_by.

Edit 2:
Docs say that config.get hast merge=recurse and merge=override.
https://docs.saltstack.com/en/latest/ref/modules/all/salt.modules.config.html#salt.modules.config.get

[alxwr]

AFAIR the key in "salt vs. salt-ssh" was that defaults.merge does/did not work on salt-ssh. I suppose config.get works just fine, but I'd have to test that.

So if we could use config.get and stick to grains.filter_by (instead of defaults.merge) we'd just need one code path without distinguishing between salt and salt-ssh.

From my tests salt-ssh seems to be able to use config.get:

% salt-ssh host config.get apache:configdir
 host:
    /etc/apache2
 % salt-ssh host pillar.get apache:configdir
host:
    /etc/apache2

[myii]

config.get wasn't ever the issue itself (for salt-ssh) but the problem faced has been merging back the return from config.get into the rest of the map. defaults.merge is one way of reliably achieving this but if grains.filter_by can be coerced do it instead, that's even better.

[alxwr]

We may run into problems because salt-call and salt-ssh are two different ways of using Salt.
But maybe defaults.merge is broken for salt-call too, so this approach would be fine.

# salt-call grains.get saltpath
local:
    /usr/local/lib/python3.6/site-packages/salt

# salt-call config.get __cli   
local:
    salt-call

#% salt-ssh host grains.get saltpath
host:
    /var/tmp/.root_99b0b6_salt/pyall/salt

#% salt-ssh host config.get __cli   
host:
    salt-call

[myii]

This is a good point, which has helped establish that config.get __cli is not good enough by itself to identify salt-ssh. Just tried salt-call on a standard minion and it worked fine with defaults.merge, so it shouldn't lose out by being batched alongside salt-ssh (if we do have to use defaults.merge as the solution).

I've got some config.items diffs between all of the types, so I'm sure we can find a reliable solution (if required).

[daks]

(my 2 cents) salt-call can also be used to run salt commands from a minion and not a master, but with the same results I think.

[alxwr]

@myii did you try the following? (merge="recurse")

{#- Merge the template pillar/grains/etc. #}
{%- set template = salt['config.get']('template', default=defaults, merge='recurse') %}

From my test this should work: Screw that. Sry, it was late.

#salt|~ % salt-ssh host1 config.get empty merge=recurse "default={'zzzz': 'asdf'}"
#[ERROR   ] TypeError encountered executing config.get: get() got an unexpected keyword argument 'merge'
host1:
    TypeError encountered executing config.get: get() got an unexpected keyword argument 'merge'
#salt|~ % salt 'host2' config.get empty merge=recurse "default={'zzzz': 'asdf'}"
host2:
    ----------
    zzzz:
        asdf

Now that's a bummer!

[myii]

@alxwr I'm assuming you meant config.get in your example above, since pillar.get is what we're trying to move on from! Yes, I tried that. If the config.get results in an empty dict, the entire map ends up as an empty dict -- you can test this by using an empty dict in the pillar and not providing any of the other config settings. What should happen is that the default map remains intact.

[myii]

Assuming that __cli is reliable to differentiate between salt-minion and salt-ssh, here's a stripped down diff between salt-ssh and salt-call:

-salt-call --local config.items
+salt-ssh 'minion' config.items

     cachedir:
-        /var/cache/salt/minion
+        /var/tmp/.root_08c4d3_salt/running_data/var/cache/salt/minion
     caller_floscript:
-        /usr/lib/python2.7/dist-packages/salt/daemons/flo/caller.flo
+        /var/tmp/.root_08c4d3_salt/pyall/salt/daemons/flo/caller.flo
     conf_file:
-        /etc/salt/minion
+        /var/tmp/.root_08c4d3_salt/minion
     config_dir:
-        /etc/salt
+        /var/tmp/.root_08c4d3_salt
     extension_modules:
-        /var/cache/salt/minion/extmods
+        /var/tmp/.root_08c4d3_salt/running_data/var/cache/salt/minion/extmods
+    fileserver_list_cache_time:
+        3
     fun:
         pythonexecutable:
-            /usr/bin/python
+            /usr/bin/python2.7
         pythonpath:
-            - /usr/bin
+            - /var/tmp/.root_08c4d3_salt/py2
+            - /var/tmp/.root_08c4d3_salt/pyall
+            - /var/tmp/.root_08c4d3_salt
             - /usr/lib/python2.7
             - /usr/lib/python2.7/plat-x86_64-linux-gnu
             - /usr/lib/python2.7/lib-tk
             - 6
             - final
             - 0
         saltpath:
-            /usr/lib/python2.7/dist-packages/salt
+            /var/tmp/.root_08c4d3_salt/pyall/salt
     grains_cache:
-        False
+        True
     log_file:
-        /var/log/salt/minion
+        /var/tmp/.root_08c4d3_salt/running_data/salt-call.log
     log_level:
-        warning
+        quiet
     minion_floscript:
-        /usr/lib/python2.7/dist-packages/salt/daemons/flo/minion.flo
+        /var/tmp/.root_08c4d3_salt/pyall/salt/daemons/flo/minion.flo
+    output:
+        json
     pidfile:
-        /var/run/salt-minion.pid
+        /var/tmp/.root_08c4d3_salt/running_data/var/run/salt-minion.pid
     pki_dir:
-        /etc/salt/pki/minion
+        /var/tmp/.root_08c4d3_salt/running_data/etc/salt/pki/minion
     print_metadata:
-        False
+        True
     retcode_passthrough:
-        False
+        True
     root_dir:
-        /
+        /var/tmp/.root_08c4d3_salt/running_data
     sock_dir:
-        /var/run/salt/minion
+        /var/tmp/.root_08c4d3_salt/running_data

This seconds some suggestions made by @alxwr in #21 (comment), regarding paths such as pythonpath and saltpath. #21 (comment) by @alxwr also brings up config_dir (or even cachedir). So which one of these is the right one to use in conjunction with __cli?

[myii]

Looking at the above diff, root_dir appeared to be a simple distinguishing factor between salt-ssh and salt-call, so added that to this PR.

[alxwr]

@alxwr I'm assuming you meant config.get in your example above, since pillar.get is what we're trying to move on from!

Yes, indeed. Sry, it was late. And my test was bogus.

not providing any of the other config settings.

Now I understand. :-) Thanks!

Well, I guess the right solution would be to implement the merge argument in salt-ssh's config.get, but that won't bring us any further regarding existing setups.

I came up with a solution supporting both salt and salt-ssh:

{%- set _lookup = salt['config.get']('template:lookup', default={}) %}
{%- set _config = salt['config.get']('template', default={}) %}

{%- set template = salt['grains.filter_by'](default_settings, default='template',
    merge=salt['grains.filter_by'](osfamilymap, grain='os_family',
      merge=salt['grains.filter_by'](osmap, grain='os',
        merge=salt['grains.filter_by'](osfingermap, grain='osfinger',
          merge=salt['grains.filter_by']({'lookup': _lookup}, default='lookup',
            merge=salt['grains.filter_by']({'config': _config}, default='config')
          )
        )
      )
    )
) %}

Patch: alxwr@a127f91

[alxwr]

@myii I was able to merge mapped values and config.get in salt-ssh:

# cat /tmp/template-formula.conf
########################################################################
# File managed by Salt at <salt://template/files/default/example.tmpl.jinja>.
# Your changes will be overwritten.
########################################################################

This is another example file from SaltStack template-formula.

{'config': '/tmp/template-formula.conf', 'tofs': {'source_files': {'template-config-file-file-managed': ['example.tmpl.jinja']}}, 'pkg': 'zsh', 'service': {'name': 'template'}}

'service': {'name': 'template'} comes from defaults.yaml.

[myii]

@alxwr That looks like an excellent development! One thing I'd like to discuss further is adding merge='recurse' to both config.get statements, as I've done in the PR so far. Otherwise, I'm a bit busy right now but I'll test it and get back to you as soon as I get a chance.

[alxwr]

@myii Take your time, I'm busy too. I should be working on another project since two hours, but this issue here is much more exciting. :-)

So, question for later:
merge='recurse' does not work on salt-ssh. Is there a reason which necessitates its use?

[myii]

@alxwr Another salt-ssh bug! Can't use merge='recurse' with config.get!

- Rendering SLS 'base:template.test' failed: Jinja error: get() got an unexpected keyword argument 'merge'
  /var/tmp/.root_08c4d3_salt/running_data/var/cache/salt/minion/files/base/template/map.jinja(11):
  ---
  [...]
  {#- Start imports as #}
  {%- import_yaml tplroot ~ "/defaults.yaml" as default_settings %}
  {%- import_yaml tplroot ~ "/osfamilymap.yaml" as osfamilymap %}
  {%- import_yaml tplroot ~ "/osmap.yaml" as osmap %}
  {%- import_yaml tplroot ~ "/osfingermap.yaml" as osfingermap %}
  {%- set _lookup = salt['config.get']('template:lookup', default={}, merge='recurse') %}    <======================
  {%- set _config = salt['config.get']('template', default={}, merge='recurse') %}
  
  {%- set template = salt['grains.filter_by'](default_settings,
      default='template',
      merge=salt['grains.filter_by'](osfamilymap, grain='os_family',
  [...]
  ---
  Traceback (most recent call last):
    File "/usr/lib/python2.7/dist-packages/salt/utils/templates.py", line 393, in render_jinja_tmpl
      output = template.render(**decoded_context)
    File "/usr/lib/python2.7/dist-packages/jinja2/environment.py", line 969, in render
      return self.environment.handle_exception(exc_info, True)
    File "/usr/lib/python2.7/dist-packages/jinja2/environment.py", line 742, in handle_exception
      reraise(exc_type, exc_value, tb)
    File "<template>", line 7, in top-level template code
    File "/usr/lib/python2.7/dist-packages/jinja2/environment.py", line 1013, in make_module
      return TemplateModule(self, self.new_context(vars, shared, locals))
    File "/usr/lib/python2.7/dist-packages/jinja2/environment.py", line 1070, in __init__
      self._body_stream = list(template.root_render_func(context))
    File "/var/tmp/.root_08c4d3_salt/running_data/var/cache/salt/minion/files/base/template/map.jinja", line 11, in top-level template code
      {%- set _lookup = salt['config.get']('template:lookup', default={}, merge='recurse') %}
  TypeError: get() got an unexpected keyword argument 'merge'

[myii]

@alxwr The benefit of using merge='recurse' is that it merges values across all of the config levels:

• sdb (or similar) < minion config file < grains < pillar < master config (if configured) < default

The drawback is that it's surely slower than stopping at the first match found.

Note: the breakdown above may not be exact, perhaps someone else can correct any issues.

[alxwr]

@myii we could make the merge parameter configurable. (I actually first match.) Maybe this is even benefits the user more. Another option is to do the merge in map.jinja, but I'd rather not re-implement Salt functionality.

[myii]

@alxwr That's interesting and it's given me an idea of how to finally make map.jinja 100% portable. When I get a chance, I'm going to try some stuff at my end and report back. Thanks for all the feedback, this has already become a much nicer solution all round. I'll definitely re-author the commit (grains.filter_by) in your name!

[myii]

@alxwr Unfortunately, the new solution isn't working for me (according to the method shown above).

Using an empty pillar:

template: {}

But setting the following values in the minion's config file:

template:
  lookup:
    minion: template-minion

Both salt-minion and salt-call --local end up with the same map. However, the diff with salt-ssh is:

--- `salt-ssh`
+++ `salt-minion`
@@ -1,9 +1,5 @@
 {
   "config": "/etc/template.d/custom-ubuntu.conf",
-  "lookup": {
-    "minion": "template-minion"
-  },
-  "minion": "template-minion",
   "pkg": "template-ubuntu",
   "service": {
     "name": "template"

So the config.get values aren't being merged in properly.

[alxwr]

@alxwr Unfortunately, the new solution isn't working for me (according to the method shown above).

My point was that the formula's defaults (*.yaml) get merged with config.get's output. I didn't even mention minion config. The reason you are not seeing minion config is the (assumed) lack of merge='recurse'. I suggested making this parameter configurable.

[alxwr]

@myii: I sketched a configurable merge= here: alxwr/template-formula@07b534b

[myii]

@alxwr No, actually config.get will start the search at the minion's config file, it has nothing to do with merge='recurse'. The only benefit to using merge='recurse' is to continue merging after the first match. So something more needs to be done here, otherwise we're not getting the benefit of config.get. Its return needs to be merged on top of the basic map provided by the *.yaml files, the same way that was done with pillar.get.

[myii]

@myii: I sketched a configurable merge= here: alxwr/template-formula@07b534b

@alxwr The problem with salt-ssh still stands, unfortunately. You can't even supply merge=None without it crashing.

[alxwr]

@myii We should be collecting known salt-ssh incompatibilities by now. :-D

What if we implement the behaviour of config.get in map.jinja, but only if salt-ssh was detected? Something like:

{# determine is_salt_ssh here #}

{%- if is_salt_ssh %}
{%-   set _lookup = salt['pillar.get']('template:lookup', default={}) %}
{%-   set _config = salt['pillar.get']('template', default={}) %}
{#-   including the possibility of merging minion data etc. into the dict 'the salt-ssh way' #}
{%- else %}
{%-   set _merge = salt['config.get']('template:config_get_merge', default='recurse') %}
{%-   set _lookup = salt['config.get']('template:lookup', default={}, merge=_merge) %}
{%-   set _config = salt['config.get']('template', default={}, merge=_merge) %}
{%- endif %}

[myii]

@alxwr Yes, that's what's being done by this original PR but in a convoluted way! I'm won't be around for a while but we can keep working on finding a simplified solution. How do you merge back into the map with the method you've outlined in the comment above?

[alxwr]

@myii I use grains.filter_by.

[alxwr]

@myii Take your time. :-)
My goal is to keep the differing code (salt vs salt-ssh) as minimal as possible and make it very explicit which parts differ and why.

[noelmcloughlin]

• saltstack/salt#44693

This ticket is reopened now.

[myii]

Thanks for chasing that up @noelmcloughlin.

[myii]

@alxwr @vutny OK, we're almost there thanks to you both for your input. Please check over my last changes in 976fe11, in order to detect the type of call that has been made (i.e. to differentiate execution for salt-ssh). Other than that, there is the last matter of this comment above: do we want to make the config.get merge strategy configurable or not? It's default is None, so I'm inclined to agree with the comment that it should be done.

[alxwr]

@myii I've made some minor remarks, but overall: LGTM

[alxwr]

@myii: Maybe our testing was flawed! (Nevertheless I took the time to review your patch, because in salt-ssh config.get lacks merge=.)

I came across this because I need to set a minion setting for salt.modules.tls.

Based on your comment #102 (comment) I did the following:

/etc/salt/ does not even exist on this machine.
If I add it, salt-ssh host config.get template ignores it.

If I add the minion options to roster they are read:

# roster
host:
  minion_opts:
    template:
      lookup:
        minion: template-minion

% salt-ssh host config.get 'template'         
host:
    ----------
    lookup:
        ----------
        minion:
            template-minion

Grains work too and can be overwritten via minion_opts:

% salt-ssh host config.get 'kernel'
host:
    FreeBSD
% salt-ssh host config.get 'kernelrelease'
host:
    fancy

So in salt-ssh the minion is not configured in /etc/salt/minion, but in the roster file.

If we just picked the wrong place to put our config, then config.get is usable in salt-ssh and the only real difference is merge=.

So we could change the if in 976fe11#diff-53e917c5e7d82bdcb337fb8b29dee69bR30 to:

{%- if cli == 'min' or cli == 'cll' %}
{%-   set default_merge_strategy = "recurse" %}
{%- else %}
{%-   set default_merge_strategy = None %}
{%- endif %}
{%- set merge_strategy = salt['config.get']('template:config_get_merge_strategy', default=default_merge_strategy) %}
{# Don't set merge= if merge_strategy is None #}

Please correct me if I'm wrong. (It's been a few days since my last comments.)

[myii]

After a lot of work together with @alxwr in our Slack/IRC/Matrix room, we've finally concluded that the most reliable option is to simply remove the merge from config.get and leave the map as-is otherwise. So while this has been a very involved PR, the final changeset is the removal of 12 characters from a single line! Thanks to all contributors for their input, particularly @alxwr! Our formulas are now truly compatible with config.get and the mass-conversions can begin!

For reference, all of the discussions start here: https://freenode.logbot.info/saltstack-formulas/20190515#c2178767.

[alxwr]

@myii LGTM
Thanks for your patience!

[myii]

@alxwr Not at all, like yourself I wanted to reach the best available solution and thanks to your help, I believe we've got there. I feel confident that this change will open up many possibilities with the rest of our formulas, so it was well worth the effort and time.

[saltstack-formulas-travis]

🎉 This PR is included in version 2.1.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀