saltstack / salt

Software to automate the management and configuration of any infrastructure or application at scale. Install Salt from the Salt package repositories here:
https://docs.saltproject.io/salt/install-guide/en/latest/
Apache License 2.0
14.18k stars 5.48k forks source link

How to override nested parameters in map.jinja #28606

Closed andrejohansson closed 5 years ago

andrejohansson commented 9 years ago

If I have a defaults.yaml structure that looks like this (nested):

salt:
  minion:
    configfile: 
      name: 'C:\salt\conf\minion'
      source: 'salt://salt/files/minion.win'

How can I override this in map.jinja? I have tried both using colons and dots to separate the levels. I could maybe use the colon notation for the line merge=salt['pillar.get']('salt')) but then I'd have to merge each subsection individually?

{% set os_family_map = salt['grains.filter_by']({
        'Windows': {},
        'Ubuntu': {
            'minion:configfile:name' : '/etc/salt/minion',
            'minion:configfile:source' : 'salt://salt/files/minion'
        }
  }
  , grain="os_family"
  , merge=salt['pillar.get']('salt'))
%}
cmarzullo commented 9 years ago

I've run into this situation as well. Haven't worked out a solution yet.

lorengordon commented 9 years ago

What happens if you write the map in full nested dictionary notation? (In other words, does jinja understand the ':' notation?)

e.g.:

{% set os_family_map = salt['grains.filter_by']({
        'Windows': {},
        'Ubuntu': {
            'minion': {
                'configfile': {
                    'name' : '/etc/salt/minion',
                    'source' : 'salt://salt/files/minion'
                }
            }
        }
  }
  , grain="os_family"
  , merge=salt['pillar.get']('salt'))
%}
andrejohansson commented 9 years ago

@lorengordon I've tried that as well but I couldn't get it to work either. Seems like the merge function isn't recursive?

lorengordon commented 9 years ago

Browsing the code, it looks like it should perform a recursive update.

Re-reading the original post, you are defining a configuration in a 'defaults.yaml' file, but then in 'map.jinja' you use pillar.get to retrieve the default configuration. How are you getting the data from 'defaults.yaml' into pillar? I apologize if that is a normal salt option; I just am not familiar with that usage of a 'defaults.yaml' file with pillar. I would probably try something like:

{% import_yaml 'defaults.yaml' as defaults %}

{% set os_family_map = salt['grains.filter_by']({
        'Windows': {},
        'Ubuntu': {
            'salt': {
                'minion': {
                    'configfile': {
                        'name' : '/etc/salt/minion',
                        'source' : 'salt://salt/files/minion'
                    }
                }
            }
        }
  }
  , grain="os_family"
  , merge=defaults)
%}
andrejohansson commented 8 years ago

@lorengordon here is my full file, in essential this is merging defaults.yaml > map.jinja > pillars into one final configuration variable (in this case the name is salt because it´s for my salt formula):

# -*- coding: utf-8 -*-
# vim: ft=jinja

{## Start with  defaults from defaults.sls ##}
{% import_yaml 'salt/defaults.yaml' as default_settings %}

{## 
Setup variable using grains['os_family'] based logic, only add key:values here
that differ from whats in defaults.yaml
##}
{% set os_family_map = salt['grains.filter_by']({
        'Windows': {},
        'Ubuntu': {
            'salt': {
                'minion': {
                    'configfile': {
                        'name' : '/etc/salt/minion',
                        'source' : 'salt://salt/files/minion'
                    }
                }
            }
        }
  }
  , grain="os_family"
  , merge=salt['pillar.get']('salt'))
%}
{## Merge the flavor_map to the default settings ##}
{## do default_settings.salt.update(os_family_map) ##}

{## Merge in salt:lookup pillar ##}
{% set salt = salt['pillar.get'](
        'salt',
        default=default_settings.salt,
        merge=True
    )
%}

This data is then imported into states and config files using this line:

{% from "salt/map.jinja" import salt with context %}

And then variables are used in this way: - name: {{ salt.master.service.name }}

As for the merging, I have an unconfirmed feeling that merging takes place, but if a node exists its replaced completely. That is if I put a configfile node in pillar containing only name. Then the source that exists in defaults.yaml will be overwritten by blank values. Could this be correct @basepi?

lorengordon commented 8 years ago

I think there is a slight mismatch in your os map and the merge dictionary. I don't think the dictionary you're getting back in this line , merge=salt['pillar.get']('salt')) contains the key salt at the toplevel, so the keys in the dictionary you're merging with are not lining up quite right. Maybe try something like this, instead:

# -*- coding: utf-8 -*-
# vim: ft=jinja

{## Start with  defaults from defaults.sls ##}
{% import_yaml 'salt/defaults.yaml' as default_settings %}

{## 
Setup variable using grains['os_family'] based logic, only add key:values here
that differ from whats in defaults.yaml
##}
{% set os_family_map = salt['grains.filter_by']({
        'Windows': {},
        'Ubuntu': {
            'salt': {
                'minion': {
                    'configfile': {
                        'name' : '/etc/salt/minion',
                        'source' : 'salt://salt/files/minion'
                    }
                }
            }
        }
  }
  , grain="os_family"
  , merge= {
    'salt': salt['pillar.get']('salt'))
} %}

{## Merge the flavor_map to the default settings ##}
{% do default_settings.salt.update(os_family_map.salt) %}
b225ccc-zz commented 8 years ago

I'm having the same problem as @andrejohansson. I keep reading how pillar merging is recursive, but I think that overriding one key in a subtree causes the whole subtree to get overwritten. I'm not sure what the intended behavior is. In my case, a full recursive merge is desired. Can we get clarification on what the expected behavior is?

Here is another example:

$ cat defaults.yaml

a:
  b:
    c1: 'foo'
    c2: 'bar'

$ cat map.jinja

{% import_yaml 'test/defaults.yaml' as defaults %}

{% set os_family_map = salt['grains.filter_by']({
    'Debian': {
      'b': {
        'c2' : 'baz'
      }
    }
  },
  grain="os_family",
  merge=salt['pillar.get']('a'))
%}

{% do defaults.a.update(os_family_map) %}

{% set a = salt['pillar.get']('a',
                              default=defaults.a,
                              merge=True) %}

$ cat test.sls

{% from "test/map.jinja" import a with context %}

/tmp/map.txt:
  file.managed:
    - contents: |
        {{ a }}

/tmp/map.txt on target node (c1 is gone):

# cat /tmp/map.txt
{'b': {'c2': 'baz'}}

Edit: I'm suspecting that the jinja do is actually calling python's update, which does not merge recursively. So, after the do, a['b']['c1'] is effectively gone. If this is true, is there another way to get a recursive merge in this sort of workflow. I don't see how to use nested data/pillar structures in this sort of format, without making sure that you are specifying an "override" value at every level of the merge, which defeats the purpose of the defaults. Perhaps in this case it's better to just define the default values in os_family_map.

Sjd-Risca commented 8 years ago

Yes, I'm sorry but jinja dict.update does only the python dict.upate command (so there is no merging but just overwrite). Anyway I learnt that also the salt function grains.filter_by does not merge but only update (remember than to be careful when overwrite salt:lookups parameters).

@b225ccc : the pillar is merging only on the saltstack site, here we are dealing about jinja/python rules.

So you were asking how to merge?

You could do it two way,

Jinja merging

The following will merge dictionary B onto A following these rules:

{%- macro deep_merge(a, b) %}
{%-     for k,v in b.iteritems() %}
{%-         if v is string or v is number %}
{%-             do a.update({ k: v }) %}
{%-         elif v is not mapping %}
{%-             if a[k] is not defined %}
{%-                 do a.update({ k: v }) %}
{%-             elif a[k] is iterable and a[k] is not mapping and a[k] is not string %}
{%-                 do a.update({ k: v|list + a[k]|list}) %}
{%-             else %}
{%-                 do a.update({ k: v }) %}
{%-             endif %}
{%-         elif v is mapping %}
{%-             if a[k] is not defined %}
{%-                 do a.update({ k: v }) %}
{%-             elif a[k] is not mapping %}
{%-                 do a.update({ k: v }) %}
{%-             else %}
{%-                 do deep_merge(a[k], v) %}
{%-             endif %}
{%-         else %}
{%-            do a.update({ k: 'ERROR: case not contempled in merging!' }) %}
{%-         endif %}
{%-     endfor %}
{%- endmacro %}

Salt modules

The code is the same as before, but it is required to create a file inside salt/_modules/my_module.py as follow:

"""
This module adds some helper for jinja rendering. They should be call through:

{% set merged_dict = salt['my_helper.merge_recursive'](dicA, dictB) %}
"""

def merge_recursive(dict_a, dict_b):
    """Recursively merge the second dict into the first.

    If the key is not present, then it is added.
    If the key is present, then the values are merged.

    The merging of the values follows this rules, if the value is:
     * a scalar (str or digit): substitute
     * a list: join with the value of the first dict, but if the fist dict is
       not a list then substitute
    """

    for k, value in dict_b.iteritems():
        merge(k, value, dict_a, dict_b)

def merge(k, v, dict_a, dict_b):
    def is_scalar(value):
        if not isinstance(value, (dict, list)):
            return True
        else:
            return False
    def is_list(value):
        if isinstance(value, list):
            return True
        else:
            return False
    def is_dict(value):
        if isinstance(value, dict):
            return True
        else:
            return False

    if k not in dict_a.keys():
        dict_a.update({ k: v})
    else:
        if is_scalar(v):
            dict_a.update({ k: v})
        elif is_list(v):
            _l = dict_a[k]
            if is_list(dict_a[k]):
                dict_a[k] = v + _l
            else:
                dict_a[k] = v
        elif is_dict(v):
            if not is_dict(dict_a[k]):
                dict_a.update({k: v})
            else:
                merge_recursive(dict_a[k], v)

Comments

I think both way are equivalent, maybe the latest is better because gives a cleaner code, maybe the jinja is better because doesn't require extra modules? I wouldn't know what to answer.

Cheers

b225ccc-zz commented 8 years ago

@Sjd-Risca I didn't test them, but these look like great solutions to the problem! I ended up going a different route and nesting the grains.filter_by() functions to get the updating that I needed. (There is an example of this in the official apache formula.) As you point out, though, this method won't work for list merging. I can live with that for now.

Thanks

colegatron commented 8 years ago

Do anyone tested/validated/approved @Sjd-Risca solution? I don't care add extra modules (and learn how they work) if it is more useful than the grains.filter_by native solution.

AFAIU, the example should be salt['my_module.merge_recursive'], instead my_helper, right?

wwentland commented 8 years ago

https://docs.saltstack.com/en/develop/ref/modules/all/salt.modules.defaults.html in develop already defines a recursive merge function and can be used by putting the defaults.py in _modules.

It should also be noted that the .update(...) call does not perform merging and that it is normal to define overrides for the os_flavour_map in the foo:lookup bit of the pillar as exemplified in https://github.com/saltstack-formulas/template-formula/blob/master/template/map.jinja#L22

colegatron commented 8 years ago

tnx babilen, I prefer to do not use develop branches in production, so this solution is better for me because I see what exactly is going on, but I would like to know if someone tested it

Iván González Valiente

Systems Developer / AWS Certified Developer

2016-05-24 16:54 GMT+02:00 Wolodja Wentland notifications@github.com:

https://docs.saltstack.com/en/develop/ref/modules/all/salt.modules.defaults.html in develop already defines the "merge" function and can be used right away.

It should also be noted that the .update(...) call does not perform merging and that it is normal to define overrides for the os_flavour_map in the foo:lookup bit of the pillar as exemplified in https://github.com/saltstack-formulas/template-formula/blob/master/template/map.jinja#L22

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/saltstack/salt/issues/28606#issuecomment-221297029

wwentland commented 8 years ago

Sure, I didn't mean to run everything from develop, just that you can grab https://github.com/saltstack/salt/blob/develop/salt/modules/defaults.py from there and put it into _modules.

Sjd-Risca commented 8 years ago

I just checked that since saltstack 2015.5.3 you can already find part of the core code on /usr/lib/python2.7/dist-packages/salt/utils/dictupdate.py (it's almost the same thing as the one that I've written).

wwentland commented 8 years ago

Absolutely. It is just that you can't easily call that in a SLS that is being rendered with jinja, which is why defaults.merge was implemented as a temporary workaround before user defined jinja filters can be provided.

xiangyu123 commented 8 years ago

you can use pillar to merge or overwrite. Actually, that's what I do, it works

colegatron commented 8 years ago

Just to know, salt.merge does merge also a list?

wwentland commented 8 years ago

salt.utils.dictupdate.update merges lists if passed the merge_lists parameter, which is False by default.

salt.modules.defaults.merge does not implement the same interface and therefore appears to default to not merging lists. It would be trivial to add this to your copy of defaults.py though, @colegatron.

The following should achieve that:

def merge(dest, upd, **kwargs):
    '''
    defaults.merge
        Allows deep merging of dicts in formulas.

    If recursive_update=False is given, it will use the classic dict.update and
    will aggregate list object types instead of replacing them if merge_lists=True.

        CLI Example:
        .. code-block:: bash
        salt '*' default.merge a=b d=e
    It is more typical to use this in a templating language in formulas,
    instead of directly on the command-line.
    '''
    return dictupdate.update(dest, upd, **kwargs)

by calling it as defaults.update ... recursive_update=True

colegatron commented 8 years ago

Tnx @babilen, I'll test later

xlotlu commented 8 years ago

The secret lies with grains.filter_by's base kwarg:

A lookup_dict key to use for a base dictionary. The grain-selected lookup_dict is merged over this and then finally the merge dictionary is merged. This allows common values for each case to be collected in the base and overridden by the grain selection dictionary and the merge dictionary. Default is unset.

I find this nice and clean (I chose to keep the flavour-specific defaults in the yaml):

$ cat map.jinja

{% import_yaml 'test/defaults.yaml' as defs %}

{% set testvar = salt['grains'].filter_by(
    defs,
    base='default',
    merge=salt['pillar.get']('test:lookup')
) %}

The data: $ cat defaults.yaml

default:
  a: x
  b: y
  c: z
  d:
    is: recursing
    lots: indeed
    maybe: perhaps

RedHat:
  a: triple x
  d:
    is: still recursing
  m: m

Some pillar overrides: $ cat ../../pillar/test_data.sls

test:
  lookup:
    c: i can has overwrite
    d:
      maybe: definitely
    n: n

Output with: $ cat test.sls

{% from "test/map.jinja" import testvar with context %}

/tmp/debug.txt:
  file.managed:
    - contents: |
        {{ testvar|yaml(False)|indent(8) }}

And this is the result on a RedHat minion: $ cat /tmp/debug.txt

a: triple x
b: y
c: i can has overwrite
d:
  is: still recursing
  lots: indeed
  maybe: definitely
m: m
n: n
alexbleotu commented 8 years ago

@Sjd-Risca Jinja merge work great!

alexbleotu commented 8 years ago

Actually after further tests, the JINJA merge code has the side effect of setting references to sub-objects in the merge (where the code does an update: do a.update({ k: v })). That would have the side effect to change the b dict as well, or even worse, if one reuses it cause changes to the merge. This can be addressed by having a cloning function that creates a clone of a dict, list etc

AlexanderThaller commented 8 years ago

I have this testcase that shows that the merge doesnt work with nested dicts (the original dict gets modified as well):

template

{% set original_map = {"1": {"1.1":{"1.1.1":"abc"}}} %}
{% set copy_map = {} %}
{% do salt['defaults.merge'](copy_map, original_map) %}

{% for key, value in copy_map.items() %}
  {% for skey, svalue in value.items() %}
    {% do svalue.update({"1.1.2":"zzz"}) %}
  {% endfor %}
{% endfor %}

# original_map
{{ original_map | pprint }}

# copy_map
{{ copy_map | pprint }}

expected output

# original_map
{'1': {'1.1': {'1.1.1': 'abc'}}}

# copy_map
{'1': {'1.1': {'1.1.1': 'abc', '1.1.2': 'zzz'}}}

output

# original_map
{'1': {'1.1': {'1.1.1': 'abc', '1.1.2': 'zzz'}}}

# copy_map
{'1': {'1.1': {'1.1.1': 'abc', '1.1.2': 'zzz'}}}
alexbleotu commented 8 years ago

Yeah this is because of the references problem I discussed in my previous comment. I created a clone macro (I couldn't find one readily available in JINJA - if there is one, please share)

{%- macro clone_dict(src, dst) %}
{# 
    Clone src into dst. Dst should be an empty dict.
#}
{%-     for k,v in src.iteritems() %}
{%-         if v is mapping %}
{%-             set clone = {} %}
{%-             do clone_dict(v, clone) %}
{%-             do dst.update({k: clone}) %}
{%-         else %}
{%-              do dst.update({k:v}) %}
{%-         endif %}
{%-     endfor %}
{%- endmacro %}

The merge function becomes

{%- macro deep_merge(a, b) %}
{# 
    Merge dictionaries a, b into a.

    If values need to be overridden, the values in b take precedence.
#}
{%-     for k,v in b.iteritems() %}
{%-         if v is not mapping %}
{%-             do a.update({ k: v }) %}
{%-         elif v is mapping %}
{%-             if a[k] is not defined %}
{%-                 set clone = {} %}
{%-                 do clone_dict(v, clone) %}
{%-                 do a.update({ k: clone }) %}
{%-             elif a[k] is not mapping %}
{%-                 set clone = {} %}
{%-                 do clone_dict(v, clone) %}
{%-                 do a.update({ k: clone }) %}
{%-             else %}
{%-                 do deep_merge(a[k], v) %}
{%-             endif %}
{%-         else %}
{%-            do a.update({ k: 'ERROR: case not contempled in merging!' }) %}
{%-         endif %}
{%-     endfor %}
{%- endmacro %}

Note that I don't do concatenations of list in functions. The same problem would appear for lists and you would need to create a simple cloning macro for lists as well.

marco-m commented 6 years ago

Hello, any news ?

wwentland commented 6 years ago

I typically use something like the following for merging nested structures:


# -*- coding: utf-8 -*-
# vim: ft=jinja

{% import_yaml 'foo/defaults.yaml' as default_settings %}

{% set osarch = salt['grains.get']('osarch') %}

{% set os_family_map = salt['grains.filter_by']({
        'Debian': {
          'foo': {
            'server': {
              'pkg': 'foo-server-' ~ osarch,
            },
            'client': {
              'pkg': 'foo-client-' ~ osarch
            },
          },
        }}
        , grain="os_family")
%}

{% set oscodename_map = salt['grains.filter_by']({
  'stretch': {
    'foo': {
      'version': '427.3'
    },
  },
  'jessie': {
    'foo': {
      'version': '42'
    },
  },
  'wheezy'
    'foo': {
      'version': '23'
    },
  },
  grain="oscodename",
  merge=salt['pillar.get']('foo:lookup'))
%}

{% do salt['defaults.merge'](default_settings, os_family_map) %}
{% do salt['defaults.merge'](default_settings, oscodename_map) %}

{% set foo = salt['pillar.get'](
        'foo',
        default=default_settings.foo,
        merge=True
      )
%}

And move os_family_map, oscodename_map and similar maps into yaml files once they get too large.

stale[bot] commented 5 years ago

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.