Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(map): update to v5 map.jinja #134

Merged
merged 2 commits into from
Feb 14, 2021

Conversation

baby-gnu
Copy link
Contributor

@baby-gnu baby-gnu commented Nov 12, 2020

PR progress checklist (to be filled in by reviewers)

  • Changes to documentation are appropriate (or tick if not required)
  • Changes to tests are appropriate (or tick if not required)
  • Reviews completed

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 tests

Does this PR introduce a BREAKING CHANGE?

Yes.

BREAKING CHANGE: map.jinja now export a generic mapdata variable

BREAKING CHANGE: The parameters per grains are now under openvpn/parameters/

Related issues and/or pull requests

Describe the changes you're proposing

Use the under reviewing v5 map.jinja:

  • use compound matcher like syntax to select parameter values sources (managed by the new libmatchers.jinja)
  • make map.jinja configurable by loading the global salt://parameters/map_jinja.yaml and the per formula salt://{{ tplroot }}/parameters/map_jinja.yaml (managed by the new libmapstack.jinja)
  • load the formula parameter values using the new libmapstack.jinja
  • provide a formula map.jinja configuration to avoid the merge of values from salt["config.get"]("openvpn") like it was done previously

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

  • Updated the README (e.g. Available states).
  • Updated pillar.example.

Testing checklist

  • Included in Kitchen (i.e. under state_top).
  • Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
  • Updated the relevant test pillar.

Additional context

@baby-gnu baby-gnu requested review from dafyddj and a team as code owners November 12, 2020 12:24
@myii
Copy link
Member

myii commented Nov 19, 2020

@baby-gnu I'm really glad that you've provided this PR. It's unfortunate that I'm in a busier patch than usual, so that I'm unable to review it so far. Hopefully, I'll get time over the coming days. Thanks again!

@myii
Copy link
Member

myii commented Dec 21, 2020

@baby-gnu I finally got a chance to test this on top of the latest changes I've made to the map.jinja verifier via. the ssf-formula (only in my fork so far). The results are good; this PR merges on top cleanly. I've run it across all of the unique platforms and I've found there's only one problematic platform: Ubuntu 16.04.

local:
    Data failed to compile:
----------
    Rendering SLS 'base:openvpn._mapdata' failed: Jinja error: do_indent() got an unexpected keyword argument 'first'
/tmp/kitchen/var/cache/salt/minion/files/base/openvpn/libmatchers.jinja(61):
---
[...]
       "query_method": query_map[_defaults["query_type"]],
       "query_delimiter": _defaults["query_delimiter"],
       "query": matcher
     }
   ) %}
{%-       do salt["log.debug"](    <======================
     log_prefix
     ~ "use built-in defaults for matcher:\n"
     ~ parsed | yaml(False) | indent(4, first=True)
   ) %}
    .. versionchanged:: 2.10
        Blank lines are not indented by default.

        Rename the ``indentfirst`` argument to ``first``.

Unfortunately, since 16.04 and the versions of Salt being run here will still be maintained for some time, we're going to need a solution for this. Especially since we want to roll out this map.jinja solution to all formulas (over time).

@baby-gnu
Copy link
Contributor Author

Thanks a lot for this review, I'll look how to make the code compatible with all versions.

@baby-gnu
Copy link
Contributor Author

baby-gnu commented Jan 6, 2021

Strange, I thought that the new _madata control was proof against ordering and spacing?

Any idea @myii?

@baby-gnu
Copy link
Contributor Author

baby-gnu commented Jan 6, 2021

Strange, I thought that the new _madata control was proof against ordering and spacing?

I sorted the _mapdata file since the ruby YAML safe_load and dump do not have option to sort keys.

@baby-gnu
Copy link
Contributor Author

I rebased the branch with final version of map.jinja, libmapstack.jinja and libmatchers.jinja.

@myii
Copy link
Member

myii commented Jan 12, 2021

I rebased the branch with final version of map.jinja, libmapstack.jinja and libmatchers.jinja.

@baby-gnu The changes haven't been pushed here yet, though?

@baby-gnu
Copy link
Contributor Author

The changes haven't been pushed here yet, though?

Yes, I pushed my wip branch, now I push this PR.

Copy link
Member

@myii myii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some initial queries about if our new map.jinja solution can allow formula-specific "extra" Jinja to be provided, instead of moving the Jinja into our YAML files.

openvpn/parameters/defaults.yaml Outdated Show resolved Hide resolved
openvpn/parameters/defaults.yaml Outdated Show resolved Hide resolved
@baby-gnu baby-gnu force-pushed the feature/v5-map.jinja branch 2 times, most recently from 3759287 to e74a757 Compare January 15, 2021 16:36
@baby-gnu
Copy link
Contributor Author

Hello @myii.

I added:

  1. the load of a defaults.yaml.jinja just after defaults.yaml
  2. the load of a post_map.yaml.jinja as last file

each file must return a valid YAML content to be merged by map.jinja, like any other files, the use of .jinja extension permit to circumvent the yamllint errors with Jinja syntax in YAML files.

I think we could event generalise the process: for each <YAML> file to load, load a <YAML>.jinja file juste after.

In this formula, this will result in:

  • map.jinja configuration load from:
    • salt://parameters/map_jinja.yaml
    • salt://parameters/map_jinja.yaml.jinja
    • salt://openvpn/parameters/map_jinja.yaml
    • salt://openvpn/parameters/map_jinja.yaml.jinja
  • formula configuration load from:
    • salt://openvpn/parameters/defaults.yaml
    • salt://openvpn/parameters/defaults_jinja.yaml.jinja
    • salt://openvpn/parameters/osarch/amd64.yaml
    • salt://openvpn/parameters/osarch/amd64.yaml.jinja
    • salt://openvpn/parameters/os_family/Debian.yaml
    • salt://openvpn/parameters/os_family/Debian.yaml.jinja
    • salt://openvpn/parameters/os/Ubuntu.yaml
    • salt://openvpn/parameters/os/Ubuntu.yaml.jinja
    • salt://openvpn/parameters/osfinger/Ubuntu-20.04.yaml
    • salt://openvpn/parameters/osfinger/Ubuntu-20.04.yaml.jinja
    • salt["config.get"]("openvpn:lookup"), nothing new here
    • salt["config.get"]("openvpn"), nothing new here
    • salt://openvpn/parameters/id/d947a6d5d3d8.yaml
    • salt://openvpn/parameters/id/d947a6d5d3d8.yaml.jinja

If you agree, I can make a new commit to show how this will behave.

@baby-gnu baby-gnu force-pushed the feature/v5-map.jinja branch 4 times, most recently from 0b078a7 to a2a6548 Compare January 15, 2021 21:13
@baby-gnu
Copy link
Contributor Author

I made commits to explore the automatic load of .jinja file for each .yaml file.

sample of debug log
[DEBUG   ] In saltenv 'base', looking at rel_path 'openvpn/map.jinja' to resolve 'salt://openvpn/map.jinja'
[DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openvpn/map.jinja' to resolve 'salt://openvpn/map.jinja'
[DEBUG   ] In saltenv 'base', looking at rel_path 'openvpn/libmapstack.jinja' to resolve 'salt://openvpn/libmapstack.jinja'
[DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openvpn/libmapstack.jinja' to resolve 'salt://openvpn/libmapstack.jinja'
[DEBUG   ] Fetching file from saltenv 'base', ** attempting ** 'salt://openvpn/libmapstack.jinja'
[DEBUG   ] No dest file found
[INFO    ] Fetching file from saltenv 'base', ** done ** 'openvpn/libmapstack.jinja'
[DEBUG   ] In saltenv 'base', looking at rel_path 'openvpn/libmatchers.jinja' to resolve 'salt://openvpn/libmatchers.jinja'
[DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openvpn/libmatchers.jinja' to resolve 'salt://openvpn/libmatchers.jinja'
[DEBUG   ] In saltenv 'base', looking at rel_path 'openvpn/libsaltcli.jinja' to resolve 'salt://openvpn/libsaltcli.jinja'
[DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openvpn/libsaltcli.jinja' to resolve 'salt://openvpn/libsaltcli.jinja'
[DEBUG   ] LazyLoaded log.debug
[DEBUG   ] [libsaltcli] the salt command type has been identified to be: local
[DEBUG   ] map.jinja configuration: process matcher: 'map_jinja.yaml'
[…]
[DEBUG   ] map.jinja configuration: load configuration values from parameters/map_jinja.yaml
[DEBUG   ] Could not find file 'salt://parameters/map_jinja.yaml' in saltenv 'base'
[DEBUG   ] map.jinja configuration: load configuration values from parameters/map_jinja.yaml.jinja
[DEBUG   ] Could not find file 'salt://parameters/map_jinja.yaml.jinja' in saltenv 'base'
[DEBUG   ] map.jinja configuration: load configuration values from openvpn/parameters/map_jinja.yaml
[DEBUG   ] In saltenv 'base', looking at rel_path 'openvpn/parameters/map_jinja.yaml' to resolve 'salt://openvpn/parameters/map_jinja.yaml'
[DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openvpn/parameters/map_jinja.yaml' to resolve 'salt://openvpn/parameters/map_jinja.yaml'
[DEBUG   ] map.jinja configuration: loaded configuration values from openvpn/parameters/map_jinja.yaml:
[…]
[DEBUG   ] map.jinja configuration: merged configuration values from openvpn/parameters/map_jinja.yaml, merge: strategy='smart', merge_lists='False':
[…]
[DEBUG   ] map.jinja configuration: load configuration values from openvpn/parameters/map_jinja.yaml.jinja
[DEBUG   ] Could not find file 'salt://openvpn/parameters/map_jinja.yaml.jinja' in saltenv 'base'
[DEBUG   ] map.jinja configuration: final configuration values:
[…]
[DEBUG   ] map.jinja: process matcher: 'defaults.yaml'
[…]
[DEBUG   ] map.jinja: process matcher: 'Y:G@osarch'
[…]
[DEBUG   ] map.jinja: process matcher: 'Y:G@os_family'
[…]
[DEBUG   ] map.jinja: process matcher: 'Y:G@os'
[…]
[DEBUG   ] map.jinja: process matcher: 'Y:G@osfinger'
[…]
[DEBUG   ] map.jinja: process matcher: 'C@openvpn:lookup'
[…]
[DEBUG   ] map.jinja: process matcher: 'Y:G@id'
[…]
[DEBUG   ] map.jinja: process matcher: 'post_map.yaml.jinja'
[…]
[DEBUG   ] map.jinja: built-in configuration:
values: {}
[DEBUG   ] map.jinja: load configuration values from openvpn/parameters/defaults.yaml
[DEBUG   ] In saltenv 'base', looking at rel_path 'openvpn/parameters/defaults.yaml' to resolve 'salt://openvpn/parameters/defaults.yaml'
[DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openvpn/parameters/defaults.yaml' to resolve 'salt://openvpn/parameters/defaults.yaml'
[DEBUG   ] map.jinja: loaded configuration values from openvpn/parameters/defaults.yaml:
[…]
[DEBUG   ] map.jinja: merged configuration values from openvpn/parameters/defaults.yaml, merge: strategy='smart', merge_lists='False':
[…]
[DEBUG   ] map.jinja: load configuration values from openvpn/parameters/defaults.yaml.jinja
[DEBUG   ] In saltenv 'base', looking at rel_path 'openvpn/parameters/defaults.yaml.jinja' to resolve 'salt://openvpn/parameters/defaults.yaml.jinja'
[DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openvpn/parameters/defaults.yaml.jinja' to resolve 'salt://openvpn/parameters/defaults.yaml.jinja'
[DEBUG   ] map.jinja: loaded configuration values from openvpn/parameters/defaults.yaml.jinja:
values:
  multi_services: true
[DEBUG   ] map.jinja: merged configuration values from openvpn/parameters/defaults.yaml.jinja, merge: strategy='smart', merge_lists='False':
[…]
[DEBUG   ] map.jinja: load configuration values from openvpn/parameters/osarch/amd64.yaml
[DEBUG   ] Could not find file 'salt://openvpn/parameters/osarch/amd64.yaml' in saltenv 'base'
[DEBUG   ] map.jinja: load configuration values from openvpn/parameters/osarch/amd64.yaml.jinja
[DEBUG   ] Could not find file 'salt://openvpn/parameters/osarch/amd64.yaml.jinja' in saltenv 'base'
[DEBUG   ] map.jinja: load configuration values from openvpn/parameters/os_family/Debian.yaml
[DEBUG   ] In saltenv 'base', looking at rel_path 'openvpn/parameters/os_family/Debian.yaml' to resolve 'salt://openvpn/parameters/os_family/Debian.yaml'
[DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openvpn/parameters/os_family/Debian.yaml' to resolve 'salt://openvpn/parameters/os_family/Debian.yaml'
[DEBUG   ] map.jinja: loaded configuration values from openvpn/parameters/os_family/Debian.yaml:
[…]
[DEBUG   ] map.jinja: merged configuration values from openvpn/parameters/os_family/Debian.yaml, merge: strategy='smart', merge_lists='False':
[…]
[DEBUG   ] map.jinja: load configuration values from openvpn/parameters/os_family/Debian.yaml.jinja
[DEBUG   ] Could not find file 'salt://openvpn/parameters/os_family/Debian.yaml.jinja' in saltenv 'base'
[DEBUG   ] map.jinja: load configuration values from openvpn/parameters/os/Debian.yaml
[DEBUG   ] In saltenv 'base', looking at rel_path 'openvpn/parameters/os/Debian.yaml' to resolve 'salt://openvpn/parameters/os/Debian.yaml'
[DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openvpn/parameters/os/Debian.yaml' to resolve 'salt://openvpn/parameters/os/Debian.yaml'
[DEBUG   ] map.jinja: loaded configuration values from openvpn/parameters/os/Debian.yaml:
[…]
[DEBUG   ] map.jinja: merged configuration values from openvpn/parameters/os/Debian.yaml, merge: strategy='smart', merge_lists='False':
[…]
[DEBUG   ] map.jinja: load configuration values from openvpn/parameters/os/Debian.yaml.jinja
[DEBUG   ] Could not find file 'salt://openvpn/parameters/os/Debian.yaml.jinja' in saltenv 'base'
[DEBUG   ] map.jinja: load configuration values from openvpn/parameters/osfinger/Debian-10.yaml
[DEBUG   ] Could not find file 'salt://openvpn/parameters/osfinger/Debian-10.yaml' in saltenv 'base'
[DEBUG   ] map.jinja: load configuration values from openvpn/parameters/osfinger/Debian-10.yaml.jinja
[DEBUG   ] Could not find file 'salt://openvpn/parameters/osfinger/Debian-10.yaml.jinja' in saltenv 'base'
[DEBUG   ] map.jinja: merge 'openvpn:lookup' retrieved with 'config.get', merge: strategy='smart', lists='False':
[…]
[DEBUG   ] map.jinja: load configuration values from openvpn/parameters/id/06221140e4dc.yaml
[DEBUG   ] Could not find file 'salt://openvpn/parameters/id/06221140e4dc.yaml' in saltenv 'base'
[DEBUG   ] map.jinja: load configuration values from openvpn/parameters/id/06221140e4dc.yaml.jinja
[DEBUG   ] Could not find file 'salt://openvpn/parameters/id/06221140e4dc.yaml.jinja' in saltenv 'base'
[DEBUG   ] map.jinja: load configuration values from openvpn/parameters/post_map.yaml.jinja
[DEBUG   ] Could not find file 'salt://openvpn/parameters/post_map.yaml.jinja' in saltenv 'base'
[DEBUG   ] map.jinja: final configuration values:
values:
  bin_dir: null
  client:
    conf_dir: /etc/openvpn/client
    service: openvpn-client
  conf_dir: /etc/openvpn
  conf_ext: conf
  dh_files:
  - '512'
  dsaparam: false
  external_repo_enabled: true
  external_repo_supported:
  - wheezy
  - jessie
  - stretch
  external_repo_version: stable
  group: openvpn
  log_user: root
  manage_group: true
  manage_user: true
  multi_services: true
  network_manager_pkgs:
  - network-manager-openvpn
  - network-manager-openvpn-gnome
  pkgs:
  - openvpn
  server:
    conf_dir: /etc/openvpn/server
    service: openvpn-server
  service: openvpn
  service_function: running
  user: openvpn
[DEBUG   ] map.jinja: save parameters in variable 'mapdata'

@baby-gnu
Copy link
Contributor Author

Now, the post-map.jinja is included without being parsed as a YAML.

It can manipulate the mapdata variable as wanted, for example, I created a user custom post-map.jinja just to have some logs:

{%- do salt["log.debug"]("User defined post map.jinja manipulation") %}

Which produce the following log:

[DEBUG   ] map.jinja: save parameters in variable 'mapdata'
[DEBUG   ] map.jinja: post-processing of 'mapdata'
[DEBUG   ] In saltenv 'base', looking at rel_path 'openvpn/post-map.jinja' to resolve 'salt://openvpn/post-map.jinja'
[DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/openvpn/post-map.jinja' to resolve 'salt://openvpn/post-map.jinja'
[DEBUG   ] Fetching file from saltenv 'base', ** attempting ** 'salt://openvpn/post-map.jinja'
[DEBUG   ] No dest file found
[INFO    ] Fetching file from saltenv 'base', ** done ** 'openvpn/post-map.jinja'
[DEBUG   ] User defined post map.jinja manipulation

When no post-map.jinja exists, it logs:

[DEBUG   ] map.jinja: save parameters in variable 'mapdata'
[DEBUG   ] map.jinja: post-processing of 'mapdata'
[DEBUG   ] Could not find file 'salt://openvpn/post-map.jinja' in saltenv 'base'

@baby-gnu
Copy link
Contributor Author

baby-gnu commented Feb 1, 2021

I need to update the map.jinja documentation for the new functionalities.

@baby-gnu
Copy link
Contributor Author

baby-gnu commented Feb 8, 2021

I updated the documentation, I'll rebase the commits when it's OK to merge.

Thanks.

BREAKING CHANGE: `map.jinja` now export a generic `mapdata` variable

BREAKING CHANGE: The parameters per grains are now under `openvpn/parameters/`
For each `.yaml` file that `map.jinja` try to load, a `.yaml.jinja`
Jinja template is tried right after to permit jinja manipulation of
values and circumvent yamllint errors when using Jinja in YAML files.

At the end of the load of YAML files and Jinja YAML templates,
`map.jinja` include an optional `post-map.jinja` for post-processing
the `mapdata` variable.
@baby-gnu
Copy link
Contributor Author

I think I finished the rebase of that PR.

Copy link
Member

@myii myii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a significant milestone, thanks for your perseverance, @baby-gnu!

@myii myii merged commit c6dcf34 into saltstack-formulas:master Feb 14, 2021
@saltstack-formulas-travis

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants