Closed baby-gnu closed 3 years ago
pull-assistant[bot]
commented
4 years ago 
Best reviewed: commit by commit
Powered by Pull Assistant. Last update f33774a ... 4878ead. Read the comment docs.
baby-gnu
commented
4 years ago There is one corner case for which I don't know what to do.
Users can query any value from pillars (using I@<key>), grains (using G@<key>), config (using C@<key>) but it can result in the error TypeError: Cannot update using non-dict types in dictupdate.update() if the returned value is not a dict, for example:
map_jinja:
sources:
# By default, when no `@` sign is present, it's `Y:C@` for YAML files with key lookup using `config.get`
- "osarch"
- "os_family"
- "os"
- "osfinger"
# This will cause the error `TypeError: Cannot update using non-dict types in dictupdate.update()`
- "G@saltversion"
# Use `:SUB` to merge values from `config.get` under `mapdata.<key>` to keep
# compatibility with user pillars.
# The `<key>` and `<key>:lookup` are merged together
- "C:SUB@openssh:lookup"
- "C:SUB@openssh"
- "C:SUB@sshd_config:lookup"
- "C:SUB@sshd_config"
- "C:SUB@ssh_config:lookup"
- "C:SUB@ssh_config"
# Lookup role based YAML files with role names from pillars only
- "Y:I@roles"
# Lookup DNS domain name based YAML files with DNS domain names from grains only
- "Y:G@dns:domain"
- "id"I see two possibilities:
mapdata[<key>] = <value>Do you have any preference?
Thanks.
n-rodriguez
commented
4 years ago Do you have any preference?
create
mapdata[<key>] = <value>
what is the value in this case?
baby-gnu
commented
4 years ago Do you have any preference?
create
mapdata[<key>] = <value>what is the value in this case?
in the example mapdata.saltversion == '3001.1'
n-rodriguez
commented
4 years ago and mapdata in mapdata[<key>] = <value> is https://github.com/saltstack-formulas/openssh-formula/blob/master/openssh/map.jinja#L207 ?
well... it would lead to have dangling keys in mapdata? no?
baby-gnu
commented
4 years ago and
mapdatainmapdata[<key>] = <value>is https://github.com/saltstack-formulas/openssh-formula/blob/master/openssh/map.jinja#L207 ?well... it would lead to have dangling keys in
mapdata? no?
Ho no, the idea is to do the same as the sub key.
So, with the map_jinja:sources from previous example, the formula authors/contributors could do something like:
{%- set tplroot = tpldir.split('/')[0] %}
{%- from tplroot ~ "/map.jinja" import mapdata with context %}
{%- if mapdata.saltversion.startswith('3001') %}
{#- […] #}well... it would lead to have dangling keys in
mapdata? no?
i meant dangling keys towards the pillars that are actually loaded :+1:
saltversion: 3000.1
sshd_config:
foo: barI don't know if it's a good idea...
baby-gnu
commented
4 years ago well... it would lead to have dangling keys in
mapdata? no?i meant dangling keys towards the pillars that are actually loaded +1
saltversion: 3000.1 sshd_config: foo: barI don't know if it's a good idea...
Well, the load of C:SUB@sshd_config looks the same to me as G@saltversion except that C:SUB@sshd_config is a dict ;-)
n-rodriguez
commented
4 years ago Well, the load of
C:SUB@sshd_configlooks the same to me asG@saltversionexcept thatC:SUB@sshd_configis adict;-)
Well, the load of C:SUB@sshd_config looks the same to me as G@saltversion except that G@saltversion loads something completely different. But it seems to be the purpose of this PR.
except that
C:SUB@sshd_configis adict
I don't make the distinction on the value. No matter what the value is (a dict, a string, a bool, a null) you will store it in mapdata with mapdata[<key>] = <value>
n-rodriguez
commented
4 years ago what bothers me is
saltversion: 3000.1
sshd_config:
foo: barIMO saltversion has nothing to do here... but as I said it seems to be the purpose of this PR so why not.
I understand you want to do this :
{%- if mapdata.saltversion.startswith('3001') %}but you can do it like this
{%- if salt['grains']['saltversion'].startswith('3001') %}or
{%- if salt['grains'].get('saltversion').startswith('3001') %}IMO
saltversionhas nothing to do here... but as I said it seems to be the purpose of this PR so why not.
Actually, the purpose of this PR is to have a single configuration for map.jinja by merging map_jinja:config_get_roots with map_jinja:sources. I remember that someone ask for something like the targeting notation to avoid having the special config_get_lookup and config_get in map_jinja:sources (but I don't remember the nickname).
I understand you want to do this :
{%- if mapdata.saltversion.startswith('3001') %}but you can do it like this
{%- if salt['grains']['saltversion'].startswith('3001') %}or
{%- if salt['grains'].get('saltversion').startswith('3001') %}
That's not really something I really want to do but a new possibility opened by this PR. It's just that I did some tests to see if C@, G@ and I@ were doing the proper thing and encounter the TypeError: Cannot update using non-dict types in dictupdate.update() when querying a random grain.
There are no many formulas with multiple variables exported by map.jinja, so this PR will be a little use in fact but I found amusing to do it and now I'm concerned about how to handle users requesting non dict grains.
myii
commented
4 years ago Just some notes for the time being.
https://docs.saltstack.com/en/latest/topics/targeting/compound.html
Letter Match Type Example Alt Delimiter? G Grains glob G@os:Ubuntu Yes E PCRE Minion ID E@web\d+.(dev|qa|prod).loc No P Grains PCRE P@os:(RedHat|Fedora|CentOS) Yes L List of minions L@minion1.example.com,minion3.domain.com or bl*.domain.com No I Pillar glob I@pdata:foobar Yes J Pillar PCRE J@pdata:^(foo|bar)$ Yes S Subnet/IP address S@192.168.1.0/24 or S@192.168.1.100 No R Range cluster R@%foo.bar No N Nodegroups N@group1 No
https://docs.saltstack.com/en/latest/faq.html#faq-grain-security
Is Targeting using Grain Data Secure?
Because grains can be set by users that have access to the minion configuration files on the local system, grains are considered less secure than other identifiers in Salt. Use caution when targeting sensitive operations or setting pillar values based on grain data.
The only grain which can be safely used is
grains['id']which contains the Minion ID.When possible, you should target sensitive operations and data using the Minion ID. If the Minion ID of a system changes, the Salt Minion's public key must be re-accepted by an administrator on the Salt Master, making it less vulnerable to impersonation attacks.
id only.baby-gnu
commented
4 years ago I found one issue with this scheme: I used to set a single pillar for map_jinja:sources to add roles and dns:domain and this can't be done with this PR since the name of the formula is used in the list.
This is a no GO for me. I need to think at a way to have both features.
baby-gnu
commented
3 years ago This is a no GO for me. I need to think at a way to have both features.
I manage to do it only by loading a global defaults.yaml which contains the following YAML:
values:
map_jinja:
sources:
- "osarch"
- "os_family"
- "os"
- "osfinger"
- "C@{{ tplroot ~ ':lookup' }}"
- "C@{{ tplroot }}"
- "dns:domain"
# Based on minion ID
- "domain"
- "id"This require the following diff:
diff --git a/openssh/map.jinja b/openssh/map.jinja
index 3ad3947..ce1dfa6 100644
--- a/openssh/map.jinja
+++ b/openssh/map.jinja
@@ -8,13 +8,25 @@
{#- Where to lookup parameters source files #}
{%- set map_sources_dir = tplroot ~ "/parameters" %}
-{#- Load defaults first to allow per formula default map.jinja configuration #}
+{#- Load global defaults first to allow centralised default map.jinja configuration #}
+{%- set _global_defaults_filename = "parameters/defaults.yaml" %}
+{%- do salt["log.debug"](
+ "map.jinja: load default parameters from "
+ ~ _global_defaults_filename
+ ) %}
+{%- load_yaml as global_default_settings %}
+{%- include _global_defaults_filename ignore missing %}
+{%- endload %}
+
+{#- Load formula defaults secondly to allow per formula default map.jinja configuration #}
{%- set _defaults_filename = map_sources_dir ~ "/defaults.yaml" %}
{%- do salt["log.debug"](
- "map.jinja: initialise parameters from "
+ "map.jinja: load default parameters from "
~ _defaults_filename
) %}
-{%- import_yaml _defaults_filename as default_settings %}
+{%- load_yaml as default_settings %}
+{%- include _defaults_filename ignore missing %}
+{%- endload %}
{#- List of sources to lookup for parameters #}
{%- do salt["log.debug"]("map.jinja: lookup 'map_jinja' configuration sources") %}
@@ -28,7 +40,22 @@
"C@" ~ tplroot,
"id",
] %}
-{#- Configure map.jinja from defaults.yaml #}
+{%- do salt["log.debug"](
+ "map.jinja: configure map.jinja from global defaults "
+ ~ _global_defaults_filename
+ ~ ": \n" ~ global_default_settings
+ | yaml(False)
+ ) %}
+{%- set map_sources = global_default_settings | traverse(
+ "values:map_jinja:sources",
+ map_sources,
+ ) %}
+
+{%- do salt["log.debug"](
+ "map.jinja: configure map.jinja from formula defaults "
+ ~ _defaults_filename
+ ~ ": \n" ~ default_settings | yaml(False)
+ ) %}
{%- set map_sources = default_settings | traverse(
"values:map_jinja:sources",
map_sources,This result in the following
[DEBUG ] Fetching file from saltenv 'base', ** attempting ** 'salt://foo/map.jinja'
[DEBUG ] No dest file found
[INFO ] Fetching file from saltenv 'base', ** done ** 'foo/map.jinja'
[DEBUG ] In saltenv 'base', looking at rel_path 'foo/libsaltcli.jinja' to resolve 'salt://foo/libsaltcli.jinja'
[DEBUG ] In saltenv 'base', ** considering ** path '/var/cache/salt/minion/files/base/foo/libsaltcli.jinja' to resolve 'salt://foo/libsaltcli.jinja'
[DEBUG ] LazyLoaded log.debug
[DEBUG ] [libsaltcli] the salt command type has been identified to be: minion
[DEBUG ] map.jinja: load default parameters from parameters/defaults.yaml
[DEBUG ] In saltenv 'base', looking at rel_path 'parameters/defaults.yaml' to resolve 'salt://parameters/defaults.yaml'
[DEBUG ] In saltenv 'base', ** considering ** path '/var/cache/salt/minion/files/base/parameters/defaults.yaml' to resolve 'salt://parameters/defaults.yaml'
[DEBUG ] map.jinja: load default parameters from foo/parameters/defaults.yaml
[DEBUG ] In saltenv 'base', looking at rel_path 'foo/parameters/defaults.yaml' to resolve 'salt://foo/parameters/defaults.yaml'
[DEBUG ] In saltenv 'base', ** considering ** path '/var/cache/salt/minion/files/base/foo/parameters/defaults.yaml' to resolve 'salt://foo/parameters/defaults.yaml'
[DEBUG ] map.jinja: lookup 'map_jinja' configuration sources
[DEBUG ] map.jinja: configure map.jinja from global defaults parameters/defaults.yaml:
values:
map_jinja:
sources:
- osarch
- os_family
- os
- osfinger
- C@foo:lookup
- C@foo
- dns:domain
- domain
- id
[DEBUG ] map.jinja: configure map.jinja from formula defaults foo/parameters/defaults.yaml:
values: {}
[DEBUG ] LazyLoaded config.get
[DEBUG ] map.jinja: load parameters with sources from ['osarch', 'os_family', 'os', 'osfinger', 'C@foo:lookup', 'C@foo', 'dns:domain', 'domain', 'id']
[DEBUG ] map.jinja: lookup 'osarch' with 'config.get'
[DEBUG ] map.jinja: load parameters from file foo/parameters/osarch/amd64.yaml
[DEBUG ] Could not find file 'salt://foo/parameters/osarch/amd64.yaml' in saltenv 'base'
[DEBUG ] map.jinja: lookup 'os_family' with 'config.get'
[DEBUG ] map.jinja: load parameters from file foo/parameters/os_family/Debian.yaml
[DEBUG ] Could not find file 'salt://foo/parameters/os_family/Debian.yaml' in saltenv 'base'
[DEBUG ] map.jinja: lookup 'os' with 'config.get'
[DEBUG ] map.jinja: load parameters from file foo/parameters/os/Debian.yaml
[DEBUG ] Could not find file 'salt://foo/parameters/os/Debian.yaml' in saltenv 'base'
[DEBUG ] map.jinja: lookup 'osfinger' with 'config.get'
[DEBUG ] map.jinja: load parameters from file foo/parameters/osfinger/Debian-10.yaml
[DEBUG ] Could not find file 'salt://foo/parameters/osfinger/Debian-10.yaml' in saltenv 'base'
[DEBUG ] map.jinja: retrieve 'foo:lookup' with 'config.get', merge: strategy='None'
[DEBUG ] map.jinja: merge 'foo:lookup' retrieved with 'config.get', merge: strategy='smart', lists='False'
[DEBUG ] LazyLoaded slsutil.merge
[DEBUG ] map.jinja: retrieve 'foo' with 'config.get', merge: strategy='None'
[DEBUG ] map.jinja: merge 'foo' retrieved with 'config.get', merge: strategy='smart', lists='False'
[DEBUG ] map.jinja: lookup 'dns:domain' with 'config.get'
[DEBUG ] map.jinja: load parameters from file foo/parameters/dns:domain/example.net.yaml
[DEBUG ] Could not find file 'salt://foo/parameters/dns:domain/example.net.yaml' in saltenv 'base'
[DEBUG ] map.jinja: lookup 'domain' with 'config.get'
[DEBUG ] map.jinja: load parameters from file foo/parameters/domain/example.net.yaml
[DEBUG ] Could not find file 'salt://foo/parameters/domain/example.net.yaml' in saltenv 'base'
[DEBUG ] map.jinja: lookup 'id' with 'config.get'
[DEBUG ] map.jinja: load parameters from file foo/parameters/id/testmachine2.example.net.yaml
[DEBUG ] Could not find file 'salt://foo/parameters/id/testmachine2.example.net.yaml' in saltenv 'base'
[DEBUG ] map.jinja: save parameters in variable 'mapdata'EDIT: I modified the patch to have better log and fix the global parameter path (no leading /)
baby-gnu
commented
3 years ago So, finally, I think the remove of config_get_lookup and config_get requires to not configure map.jinja by looking up map_jinja:sources with config.get.
The configuration of map.jinja became only possible with:
defaults.yaml (not sure to put it under /parameters or directly at top level)defaults.yaml (required for openssh which requires more than C@{{ tplroot }})If people agree, I'll rework that PR with the proposed patch.
Regards.
myii
commented
3 years ago
- a per formula
defaults.yaml(required foropensshwhich requires more thanC@{{ tplroot }})
@baby-gnu My general instinct is that this is the right option. Do we have an example of what this looks like?
baby-gnu
commented
3 years ago Now, I'm wondering if using the file name defaults.yaml to configure map.jinja is a good idea. It's like mixing 2 things:
map.jinja configurationWe could load the map_jinja:sources from several places, either global or formula specific.
These files don't exists actually
/parameters/defaults.yaml: default global values (for all formula), it could include map_jinja:sources but others too so I'm not very confident with it for now/parameters/map_jinja.yaml: unambiguous, dedicated file to configure map.jinja globally (for all formula), it's my preferred choice for global map.jinja configuration{{ tplroot }}/parameters/defaults.yaml: default values for a specific formula, it could include map_jinja:sources but is mostly used for the formula itself. Modifying this file for a user requires to copy all the content of the original one (since the salt file server serves the first file found){{ tplroot }}/parameters/map_jinja.yaml: unambiguous, dedicated file to configure map.jinja for a specific formula, it does not exists actually but could be introduced by this PR, it's my preferred choice for per formula map.jinja configurationbaby-gnu
commented
3 years ago @baby-gnu My general instinct is that this is the right option. Do we have an example of what this looks like?
I think I could update the PR #194 to show how to use the salt file server instead of pillars.
myii
commented
3 years ago Global values
...
/parameters/map_jinja.yaml: unambiguous, dedicated file to configuremap.jinjaglobally (for all formula), it's my preferred choice for globalmap.jinjaconfigurationPer formula values
...
{{ tplroot }}/parameters/map_jinja.yaml: unambiguous, dedicated file to configuremap.jinjafor a specific formula, it does not exists actually but could be introduced by this PR, it's my preferred choice for per formulamap.jinjaconfiguration
@baby-gnu Gut feeling is the above. Actually, I'd be really interested if we could have both -- use the global settings if not overridden by the user in {{ tplroot }}/parameters/map_jinja.yaml. Need a way to make it easy for users to provide these files without clashing with any files provided by the repo itself.
baby-gnu
commented
3 years ago Now, I'm wondering if using the file name
defaults.yamlto configuremap.jinjais a good idea. It's like mixing 2 things:* the `map.jinja` configuration * the formula configurationWe could load the
map_jinja:sourcesfrom several places, either global or formula specific.
I made a test:
map.jinja config from parameters/map_jinja.yaml if it exists (no error if it does not), this file can be provided by user.map.jinja config from {{ tplroot }}/parameters/map_jinja.yaml if it exists (no error if it does not), this file is provided by the formula and can be easily overrode by the userNote that the map.jinja values are removed from _mapdata as you can see in the failing tests, I can merge it in the default_settings if you prefer to follow the any changes to the map.jinja configuration.
And here is the DEBUG output I got:
[DEBUG ] In saltenv 'base', looking at rel_path 'openssh/map.jinja' to resolve 'salt://openssh/map.jinja'
[DEBUG ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openssh/map.jinja' to resolve 'salt://openssh/map.jinja'
[DEBUG ] In saltenv 'base', looking at rel_path 'openssh/libsaltcli.jinja' to resolve 'salt://openssh/libsaltcli.jinja'
[DEBUG ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openssh/libsaltcli.jinja' to resolve 'salt://openssh/libsaltcli.jinja'
[DEBUG ] [libsaltcli] the salt command type has been identified to be: local
[DEBUG ] map.jinja: load global map.jinja values from parameters/map_jinja.yaml
[DEBUG ] Could not find file 'salt://parameters/map_jinja.yaml' in saltenv 'base'
[DEBUG ] map.jinja: load per formula map.jinja values from openssh/parameters/map_jinja.yaml
[DEBUG ] In saltenv 'base', looking at rel_path 'openssh/parameters/map_jinja.yaml' to resolve 'salt://openssh/parameters/map_jinja.yaml'
[DEBUG ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openssh/parameters/map_jinja.yaml' to resolve 'salt://openssh/parameters/map_jinja.yaml'
[DEBUG ] map.jinja: built-in configuration sources:
values:
map_jinja:
sources:
- osarch
- os_family
- os
- osfinger
- C@openssh:lookup
- C@openssh
- id
[DEBUG ] map.jinja: configuration sources from global map.jinja configuration parameters/map_jinja.yaml:
values:
map_jinja:
sources: {}
[DEBUG ] map.jinja: configuration sources from formula map.jinja configuration openssh/parameters/map_jinja.yaml:
values:
map_jinja:
sources:
- osarch
- os_family
- os
- osfinger
- C:SUB@openssh:lookup
- C:SUB@openssh
- C:SUB@sshd_config:lookup
- C:SUB@sshd_config
- C:SUB@ssh_config:lookup
- C:SUB@ssh_config
- id
[DEBUG ] map.jinja: load parameters with sources from:
- osarch
- os_family
- os
- osfinger
- C:SUB@openssh:lookup
- C:SUB@openssh
- C:SUB@sshd_config:lookup
- C:SUB@sshd_config
- C:SUB@ssh_config:lookup
- C:SUB@ssh_config
- id
[DEBUG ] map.jinja: load per formula default values from openssh/parameters/defaults.yaml
[DEBUG ] In saltenv 'base', looking at rel_path 'openssh/parameters/defaults.yaml' to resolve 'salt://openssh/parameters/defaults.yaml'
[DEBUG ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openssh/parameters/defaults.yaml' to resolve 'salt://openssh/parameters/defaults.yaml'
[DEBUG ] map.jinja: lookup 'osarch' with 'config.get'
[DEBUG ] map.jinja: load parameters from file openssh/parameters/osarch/amd64.yaml
[DEBUG ] Could not find file 'salt://openssh/parameters/osarch/amd64.yaml' in saltenv 'base'
[DEBUG ] map.jinja: lookup 'os_family' with 'config.get'
[DEBUG ] map.jinja: load parameters from file openssh/parameters/os_family/Debian.yaml
[DEBUG ] In saltenv 'base', looking at rel_path 'openssh/parameters/os_family/Debian.yaml' to resolve 'salt://openssh/parameters/os_family/Debian.yaml'
[DEBUG ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openssh/parameters/os_family/Debian.yaml' to resolve 'salt://openssh/parameters/os_family/Debian.yaml'
[DEBUG ] map.jinja: merge parameters from openssh/parameters/os_family/Debian.yaml
[DEBUG ] map.jinja: lookup 'os' with 'config.get'
[DEBUG ] map.jinja: load parameters from file openssh/parameters/os/Debian.yaml
[DEBUG ] Could not find file 'salt://openssh/parameters/os/Debian.yaml' in saltenv 'base'
[DEBUG ] map.jinja: lookup 'osfinger' with 'config.get'
[DEBUG ] map.jinja: load parameters from file openssh/parameters/osfinger/Debian-10.yaml
[DEBUG ] Could not find file 'salt://openssh/parameters/osfinger/Debian-10.yaml' in saltenv 'base'
[DEBUG ] map.jinja: retrieve 'openssh:lookup' with 'config.get', merge: strategy='None'
[DEBUG ] map.jinja: merge sub key 'openssh:lookup' retrieved with 'config.get', merge: strategy='smart', lists='False'
[DEBUG ] map.jinja: retrieve 'openssh' with 'config.get', merge: strategy='None'
[DEBUG ] map.jinja: merge sub key 'openssh' retrieved with 'config.get', merge: strategy='smart', lists='False'
[DEBUG ] map.jinja: retrieve 'sshd_config:lookup' with 'config.get', merge: strategy='None'
[DEBUG ] map.jinja: merge sub key 'sshd_config:lookup' retrieved with 'config.get', merge: strategy='smart', lists='False'
[DEBUG ] map.jinja: retrieve 'sshd_config' with 'config.get', merge: strategy='None'
[DEBUG ] map.jinja: merge sub key 'sshd_config' retrieved with 'config.get', merge: strategy='smart', lists='False'
[DEBUG ] map.jinja: retrieve 'ssh_config:lookup' with 'config.get', merge: strategy='None'
[DEBUG ] map.jinja: merge sub key 'ssh_config:lookup' retrieved with 'config.get', merge: strategy='smart', lists='False'
[DEBUG ] map.jinja: retrieve 'ssh_config' with 'config.get', merge: strategy='None'
[DEBUG ] map.jinja: merge sub key 'ssh_config' retrieved with 'config.get', merge: strategy='smart', lists='False'
[DEBUG ] map.jinja: lookup 'id' with 'config.get'
[DEBUG ] map.jinja: load parameters from file openssh/parameters/id/example.net.yaml
[DEBUG ] Could not find file 'salt://openssh/parameters/id/example.net.yaml' in saltenv 'base'
[DEBUG ] map.jinja: save parameters in variable 'mapdata'What do you think about this?
myii
commented
3 years ago @baby-gnu Yes, this seems like the right way forward.
- load
map.jinjaconfig from{{ tplroot }}/parameters/map_jinja.yamlif it exists (no error if it does not), this file is provided by the formula and can be easily overrode by the user
How would the user overrride this file?
Note that the
map.jinjavalues are removed from_mapdataas you can see in the failing tests, I can merge it in thedefault_settingsif you prefer to follow the any changes to themap.jinjaconfiguration.
Since these are both relatively new features (v4/5 map.jinja and _mapdata dump comparison), I'm not attached to either option. Should we be verifying that map_jinja:sources are working as expected? I suppose we should, if it's relatively simple to implement.
baby-gnu
commented
3 years ago How would the user overrride this file?
As an example, I will:
roots is higher in priority than gitfs/srv/salt/parameters and /srv/salt/{{ tplroot }}/parameters/ directoriescreate the /srv/salt/parameters/map_jinja.yaml with the following content
values:
map_jinja:
sources:
# default values
- "osarch"
- "os_family"
- "os"
- "osfinger"
- "C@{{ tplroot ~ ':lookup' }}"
- "C@{{ tplroot }}"
# role based configuration
- "roles"
# DNS domain configured (DHCP or resolv.conf)
- "dns:domain"
# Based on minion ID
- "domain"
# default values
- "id"create the /srv/salt/openssh/parameters/map_jinja.yaml with the following content
values:
map_jinja:
sources:
# default values
- "osarch"
- "os_family"
- "os"
- "osfinger"
# Merge values from `config.get` under `mapdata.<key>` to keep
# compatibility with user pillars.
# The `<key>` and `<key>:lookup` are merged together
- "C:SUB@openssh:lookup"
- "C:SUB@openssh"
- "C:SUB@sshd_config:lookup"
- "C:SUB@sshd_config"
- "C:SUB@ssh_config:lookup"
- "C:SUB@ssh_config"
# role based configuration
- "roles"
# DNS domain configured (DHCP or resolv.conf)
- "dns:domain"
# Based on minion ID
- "domain"
# default values
- "id"This way:
map.jinja to include parameters values per roles and DNS domainmap.jinja for the openssh-formula to include parameters values per roles and DNS domain, this one is required since the global configuration would broke this formulaSince these are both relatively new features (v4/5 map.jinja and _mapdata dump comparison), I'm not attached to either option. Should we be verifying that map_jinja:sources are working as expected? I suppose we should, if it's relatively simple to implement.
Yes, it is. And it's working.
I'll merge my personal branch tomorrow and clean the PR if you are OK.
Thanks for your comments.
myii
commented
3 years ago I'll merge my personal branch tomorrow and clean the PR if you are OK.
@baby-gnu All good at this end. Please go ahead so that we can start testing it out for proper feedback.
baby-gnu
commented
3 years ago @myii Now that I finalized this branch, I now see that the map_jinja.yaml could contains quite complex things like:
values:
map_jinja:
sources:
# default values
- "parameters/osarch/{{ salt['grains.get']('osarch') }}.yaml"
- "parameters/os_family/{{ salt['grains.get']('os_family') }}.yaml"
- "parameters/os/{{ salt['grains.get']('os') }}.yaml"
- "parameters/osfinger/{{ salt['grains.get']('osfinger') }}.yaml"
- "C@{{ tplroot ~ ':lookup' }}"
- "C@{{ tplroot }}"
# role based configuration
{%- for role in salt['config.get']('roles') %}
{%- if role %}
- "parameters/roles/{{ role }}.yaml"
{%- endif %}
{%- endfor %}
# DNS domain configured (DHCP or resolv.conf)
- "parameters/dns:domain/{{ salt['grains.get']('dns:domain') }}.yaml"
# Based on minion ID
- "parameters/domain/{{ salt['grains.get']('domain') }}.yaml"
# default values
- "parameters/id/{{ salt['grains.get']('id') }}.yaml"Which:
config.get/pillar.get/grains.getSo, if someone found that map.jinja is too complex, we could simplify a lot map.jinja to the expense of forcing Jinja in map_jinja.yaml file. But I prefer the way it is now ;-)
Regards.
n-rodriguez
commented
3 years ago @baby-gnu just a question :
# role based configuration
{%- for role in salt['config.get']('roles') %}
{%- if role %}
- "parameters/roles/{{ role }}.yaml"
{%- endif %}
{%- endfor %}how do you make the distinction between client role and server role? one machine could have both roles, for instance salt-master and salt-minion role.
baby-gnu
commented
3 years ago how do you make the distinction between client role and server role? one machine could have both roles, for instance
salt-masterandsalt-minionrole.
Actually, with this jinja code (and it's equivalent in map.jinja which will handle the - roles item), map.jinja will load values from:
{{ tplroot }}/parameters/roles/salt-master.yaml{{ tplroot }}/parameters/roles/salt-minion.yamlYou can only have a problem if both roles define the same parameter with different values.
The writer of a formula should take care of that case and if her formula has several components, they must play well together on the same minion ;-)
But maybe you see any issue with this?
n-rodriguez
commented
3 years ago Actually, with this jinja code (and it's equivalent in
map.jinjawhich will handle the- rolesitem),map.jinjawill load values from:* `{{ tplroot }}/parameters/roles/salt-master.yaml` * `{{ tplroot }}/parameters/roles/salt-minion.yaml`
Sure. But what about syslog-ng? Of course you can do with a suffix :
{{ tplroot }}/parameters/roles/syslog-ng-client.yaml{{ tplroot }}/parameters/roles/syslog-ng-server.yamlor a prefix :
{{ tplroot }}/parameters/roles/client-syslog-ng.yaml{{ tplroot }}/parameters/roles/server-syslog-ng.yamlBut on my side I ended up using this with stacker :
{{ tplroot }}/parameters/roles/client/syslog-ng.yaml{{ tplroot }}/parameters/roles/server/syslog-ng.yamlor
{{ tplroot }}/parameters/roles/client/salt.yaml{{ tplroot }}/parameters/roles/server/salt.yamlEdit :
I find it cleaner than the way I was using before.
I used prefix (client- or server-) but had to play with strings to get the role "type" (client or server)
Example : server-syslog-ng | split('-')[0] to get server and go in the right directory.
Now I do something like :
{%- for role in salt['config.get']('roles:server', []) %}
{{ tplroot }}/parameters/roles/server/{{ role }}.yaml
{% endfor %}
{%- for role in salt['config.get']('roles:client', []) %}
{{ tplroot }}/parameters/roles/client/{{ role }}.yaml
{% endfor %}But on my side I ended up using this with stacker :
For stacker, you are defining a formula specific role mapping.
With map.jinja, you can define a global mapping and a local for few specific formulas.
But for your example, I will define this global map.jinja configuration in salt://parameters/map_jinja.yaml:
values:
map_jinja:
sources:
# default values
- "osarch"
- "os_family"
- "os"
- "osfinger"
- "C@{{ tplroot ~ ':lookup' }}"
- "C@{{ tplroot }}"
# role based configuration
- "roles"
# DNS domain configured (DHCP or resolv.conf)
- "dns:domain"
# Based on minion ID
- "domain"
# default values
- "id"And define 2 roles:
syslog-ng/clientsyslog-ng/serverI make a quick test (tplroot is test-syslog) and the debug output of map.jinja produce:
[…]
[DEBUG ] map.jinja: lookup 'roles' with 'config.get'
[DEBUG ] map.jinja: load parameters from file test-syslog/parameters/roles/syslog-ng/client.yaml
[DEBUG ] Could not find file 'salt://test-syslog/parameters/roles/syslog-ng/client.yaml' in saltenv 'base'
[DEBUG ] map.jinja: load parameters from file test-syslog/parameters/roles/syslog-ng/server.yaml
[DEBUG ] Could not find file 'salt://test-syslog/parameters/roles/syslog-ng/server.yaml' in saltenv 'base'
[…]The documentation of map.jinja is now in this PR to get it in sync with the code.
myii
commented
3 years ago @saltstack-formulas/wg As discussed in the working group meetings, if others can help review at least the documentation portion, that will be extremely helpful.
baby-gnu
commented
3 years ago I've got a couple of other things to add after this but I don't want to cram too much into one review!
But I think the alternate delimiter option is great and will give more flexibility for the future. Thanks!
Thanks.
baby-gnu
commented
3 years ago I think this PR will be reworked with my refactoring and I'll take the opportunity of having breaking changes to put all the libs under a lib directory (or maybe a libjinja?).
Then, when we will be OK with that, I'll make a PR for libtofs.
myii
commented
3 years ago ...and I'll take the opportunity of having breaking changes to put all the libs under a
libdirectory (or maybe alibjinja?).
@baby-gnu I've just been doing some work on the ssf-formula in order to manage the whole map.jinja verification setup. While doing this, I got reminded of how difficult it would be to move these files that are already managed. Many formulas already use libtofs.jinja -- this would require a bunch of steps both automated and manual:
libtofs.jinja.ssf-formula, which would include:
CODEOWNERS.And then all of that would be need to verified for every formula. Some formulas would invariably be using the new paths while others the old paths (this is really horrible to maintain). Not to mention the ones that manage libsaltcli.jinja, etc.
While I agreed with the idea, it's (again), far too much refactoring for little gain -- apologies.
baby-gnu
commented
3 years ago While I agreed with the idea, it's (again), far too much refactoring for little gain -- apologies.
OK, so let everything in tlproot. Thanks.
baby-gnu
commented
3 years ago There is no delimiter for config.get over salt-ssh:
root@scribe:~# salt-ssh test-minion config.get 'foo!bar' delimiter='!'
[ERROR ] TypeError encountered executing config.get: get() got an unexpected keyword argument 'delimiter'
test-minion:
TypeError encountered executing config.get: get() got an unexpected keyword argument 'delimiter'Now I'll refactor the commits to have a better implementation progression, for future history review ;-)
baby-gnu
commented
3 years ago So now, I think the commits are in good shape for review and view how things are changed to reach v5 map.jinja.
I even validate manually each of them separately:
for i in $(git log --pretty=format:"%h" master..HEAD)
do
git checkout ${i}
./bin/kitchen converge default-debian-10-master-py3 && ./bin/kitchen verify default-debian-10-master-py3
doneand all runs were successful.
saltstack-formulas-travis
commented
3 years ago :tada: This PR is included in version 3.0.0 :tada:
The release is available on GitHub release
Your semantic-release bot :package::rocket:
myii
commented
3 years ago @baby-gnu It took us only 6 months (nearly) but we got there in the end! Thanks for your patience and perseverance.
A big thanks also goes to @n-rodriguez @dafyddj and @vutny for their involvement with improving this implementation.
PR progress checklist (to be filled in by reviewers)
What type of PR is this?
Primary type
[build]Changes related to the build system[chore]Changes to the build process or auxiliary tools and libraries such as documentation generation[ci]Changes to the continuous integration configuration[feat]A new feature[fix]A bug fix[perf]A code change that improves performance[refactor]A code change that neither fixes a bug nor adds a feature[revert]A change used to revert a previous commit[style]Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)Secondary type
[docs]Documentation changes[test]Adding missing or correcting existing testsDoes this PR introduce a
BREAKING CHANGE?Yes.
BREAKING CHANGE: the configuration
map_jinja:sourcesis only configurable withsalt://parameters/map_jinja.yamlandsalt://{{ tplroot }}/parameters/map_jinja.yamlBREAKING CHANGE: the
map_jinja:config_get_rootsis replaced by compound likemap_jinja:sourcesBREAKING CHANGE: the two
map_jinja:sourcesconfig_get_lookupandconfig_getare replaced byC@<tplroot>:lookupandC@<tplroot>sourcesRelated issues and/or pull requests
186
Describe the changes you're proposing
The
config_get_lookupandconfig_getsources lack flexibility.It's not easy to query several pillars and/or grains keys with the actual system. And the query method is forced to
config.getwithout being configurable by the user.We define a mechanism to select
map.jinjasources with similar notation as the salt targeting system.Each source has a type:
Yto load values from YAML files, this is the default when no type is definedCto lookup values withconfig.getGto lookup values withgrains.getIto lookup values withpillar.getThe YAML type can define the query method to lookup the key value to build the file name:
Cto query withconfig.get, this is the default when to query method is definedGto query withgrains.getIto query withpillar.getThe
C,GorItypes can define theSUBattribute to merge values in the sub keymapdata.<key>instead of directly inmapdata.BREAKING CHANGE: the configuration
map_jinja:sourcesis only configurable withsalt://parameters/map_jinja.yamlandsalt://{{ tplroot }}/parameters/map_jinja.yamlBREAKING CHANGE: the
map_jinja:config_get_rootsis replaced by compound likemap_jinja:sourcesBREAKING CHANGE: the two
map_jinja:sourcesconfig_get_lookupandconfig_getare replaced byC@<tplroot>:lookupandC@<tplroot>sourcesUsers can now define
map.jinjasources like:The
map.jinjaconfiguration are looked-up from:salt://parameters/map_jinja.yamlto define it globally for all formulassalt://{{ tplroot }}/parameters/map_jinja.yamlto define it per formulaThe
map_jinja.yamlfile can contains Jinja constructs like the use of{{ tplroot }}:Pillar / config required to test the proposed changes
Included in the commit.
Debug log showing how the proposed changes work
Documentation checklist
README(e.g.Available states).pillar.example.Testing checklist
state_top).Additional context