feat(map): use targeting like syntax for configuration #191
Conversation
|
Best reviewed: commit by commit
Optimal code review plan
|
|
There is one corner case for which I don't know what to do. Users can query any value from pillars (using 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:
Do you have any preference? Thanks. |
what is the value in this case? |
in the example |
|
and well... it would lead to have dangling keys in |
Ho no, the idea is to do the same as the So, with the {%- set tplroot = tpldir.split('/')[0] %}
{%- from tplroot ~ "/map.jinja" import mapdata with context %}
{%- if mapdata.saltversion.startswith('3001') %}
{#- […] #} |
i meant dangling keys towards the pillars that are actually loaded 👍 saltversion: 3000.1
sshd_config:
foo: barI don't know if it's a good idea... |
Well, the load of |
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.
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 |
|
what bothers me is saltversion: 3000.1
sshd_config:
foo: barIMO 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') %} |
Actually, the purpose of this PR is to have a single configuration for
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 There are no many formulas with multiple variables exported by |
|
Just some notes for the time being. https://docs.saltstack.com/en/latest/topics/targeting/compound.html
https://docs.saltstack.com/en/latest/faq.html#faq-grain-security
|
|
I found one issue with this scheme: I used to set a single pillar for 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 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 EDIT: I modified the patch to have better log and fix the global parameter path (no leading |
|
So, finally, I think the remove of The configuration of
If people agree, I'll rework that PR with the proposed patch. Regards. |
@baby-gnu My general instinct is that this is the right option. Do we have an example of what this looks like? |
|
Now, I'm wondering if using the file name
We could load the Global valuesThese files don't exists actually
Per formula values
|
I think I could update the PR #194 to show how to use the salt file server instead of pillars. |
@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 |
I made a test:
Note that the And here is the What do you think about this? |
|
@baby-gnu Yes, this seems like the right way forward.
How would the user overrride this file?
Since these are both relatively new features (v4/5 |
As an example, I will:
This way:
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. |
@baby-gnu All good at this end. Please go ahead so that we can start testing it out for proper feedback. |
baby-gnu
force-pushed
the
feature/map-config-targeting-like-syntax
branch
from
fe98819
to
bf9d76d
Compare
|
@myii Now that I finalized this branch, I now see that the 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:
So, if someone found that Regards. |
|
@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 |
Actually, with this jinja code (and it's equivalent in
You 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? |
Sure. But what about
or a prefix :
But on my side I ended up using this with stacker :
or
Edit : I find it cleaner than the way I was using before. I used prefix ( Now I do something like : |
OK, so let everything in |
|
There is no |
baby-gnu
force-pushed
the
feature/map-config-targeting-like-syntax
branch
from
ce9f1b5
to
9129a23
Compare
|
Now I'll refactor the commits to have a better implementation progression, for future history review ;-) |
baby-gnu
force-pushed
the
feature/map-config-targeting-like-syntax
branch
2 times, most recently
from
a035ab2
to
a85ecb2
Compare
|
So now, I think the commits are in good shape for review and view how things are changed to reach v5 I even validate manually each of them separately: and all runs were successful. |
baby-gnu
force-pushed
the
feature/map-config-targeting-like-syntax
branch
from
a85ecb2
to
9bc8c8f
Compare
There was a problem hiding this comment.
@baby-gnu We're all good with the implementation, this is the eyeball-review, nearly all just formatting. Just one main issue identified with the positioning of to_bool.
The `config_get_lookup` and `config_get` sources 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.get` without
being configurable by the user.
We define a mechanism to select `map.jinja` sources with similar
notation as the salt targeting system.
The `map.jinja` file uses several sources where to lookup parameter
values. The list of sources can be modified by two files:
1. a global salt://parameters/map_jinja.yaml
2. a per formula salt://{{ tplroot }}/parameters/map_jinja.yaml.
Each source definition has the form `<TYPE>:<OPTION>@<KEY>` where
`<TYPE>` can be one of:
- `Y` to load values from YAML files, this is the default when no type
is defined
- `C` to lookup values with `config.get`
- `G` to lookup values with `grains.get`
- `I` to lookup values with `pillar.get`
The YAML type option can define the query method to lookup the key
value to build the file name:
- `C` to query with `config.get`, this is the default when to query
method is defined
- `G` to query with `grains.get`
- `I` to query with `pillar.get`
The `C`, `G` or `I` types can define the `SUB` option to store values
in the sub key `mapdata.<key>` instead of directly in `mapdata`.
Finally, the `<KEY>` describe what to lookup to either build the YAML
filename or gather values using one of the query method.
BREAKING CHANGE: the configuration `map_jinja:sources` is only
configurable with `salt://parameters/map_jinja.yaml`
and `salt://{{ tplroot }}/parameters/map_jinja.yaml`
BREAKING CHANGE: the `map_jinja:config_get_roots` is replaced by
compound like `map_jinja:sources`
BREAKING CHANGE: the two `config_get_lookup` and `config_get` are
replaced by `C@<tplroot>:lookup` and `C@<tplroot>`
sources
No need to prefix some of them with underscore `_`.
baby-gnu
force-pushed
the
feature/map-config-targeting-like-syntax
branch
from
9bc8c8f
to
7de2d6f
Compare
|
🎉 This PR is included in version 3.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
@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