feat(templates): don't get openssh pillars in templates

baby-gnu commented 4 years ago

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
[x] [perf] A code change that improves performance
[x] [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`?

No.

Related issues and/or pull requests

Describe the changes you're proposing

The OpenSSH formula can't be used with salt-ssh because map.jinja is imported from templates.

We pass the pillars via the template engine context, this avoid the need to load map.jinja from the templates themselves and reduce the number of pillar.get calls.

openssh/config.sls (sshd_config): pass sshd_config in the context. (ssh_config): pass ssh_config in the context.
openssh/files/default/ssh_config: remove map.jinja import since it's now in the context.
openssh/files/default/sshd_config: ditoo.
openssh/known_hosts.sls: pass known_hosts in the context.
openssh/files/default/ssh_known_hosts: use known_hosts from the context instead of calling pillar.get several times.

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

pull-assistant[bot] commented 4 years ago

Best reviewed: commit by commit

Optimal code review plan

feat(templates): don't get openssh pillars in templates

Powered by Pull Assistant. Last update cb6e48f ... cb6e48f. Read the comment docs.

baby-gnu commented 4 years ago

I checked the perf type since I think that removing several pillar.get could improve a little. But I didn't do any benchmark.

javierbertoli commented 4 years ago

@baby-gnu I always disliked pillars data being searched/retrieved directly from the jinja templates, as I understand templates are there to be used by the state files as... well, templates and, therefore, data should be retrieved in the state file, where the file resource is rendered using that data as context interpolated into the template file.

I guess that people get used to doing the data retrieval directly on the template file because 'it's possible' but, as you mention, it leads to misbehaviour on some apps like salt-ssh that work ok when 'done right'.

So because of all the above, I think your PR is great. :smile:

One question: Won't all these traverses add extra load on the jinja parser? Why not replace

{%- set hostnames_target = known_hosts | traverse('hostnames:target', hostnames_target_default) -%}

with

{%- set hostnames_target = known_hosts.hostnames.target' | default(hostnames_target_default) -%}

or

    - context:
         known_hosts: {{ openssh | traverse("known_hosts", {}) }}

with

    - context:
         known_hosts: {{ openssh.known_hosts | defaut({}) }}

Is some extra benefit in using traverse that I'm not seing?

baby-gnu commented 4 years ago

Is some extra benefit in using traverse that I'm not seing?

One question: Won't all these traverses add extra load on the jinja parser? Why not replace
{%- set hostnames_target = known_hosts | traverse('hostnames:target', hostnames_target_default) -%}
with
{%- set hostnames_target = known_hosts.hostnames.target' | default(hostnames_target_default) -%}

If known_hosts.hostnames does not exists you will have the following error:

salt.exceptions.SaltRenderError: Jinja variable 'dict object' has no attribute 'hostnames'

    - context:
         known_hosts: {{ openssh | traverse("known_hosts", {}) }}

with

    - context:
         known_hosts: {{ openssh.known_hosts | defaut({}) }}

Here I could use your proposal.

Is some extra benefit in using traverse that I'm not seing?

The traverse is the only way to easily query deeper level keys and provide a fallback if there is a KeyError at any level.

I stick to using traverse each time for consistency and looking at the code of traverse I'm not sure it will be much more load for single subkey access than accessing a dict by the point notation and using the default or doing a var.get("key", "default").

baby-gnu commented 4 years ago

always disliked pillars data being searched/retrieved directly from the jinja templates, as I understand templates are there to be used by the state files as... well, templates and, therefore, data should be retrieved in the state file, where the file resource is rendered using that data as context interpolated into the template file.

I did not remove all the pillar.get calls to make changes as minimal as possible. And there are some pillar.get I don't understand ;-)

daks commented 4 years ago

As said by @javierbertoli I also dislike direct calls to pillar.get in templates and prefer to use context to pass data to them, so good idea for this PR!

About the traverse use, I don't really know it, so I tend to use foo.get('bar', {}) which is not a very nice syntax, especially when you want to get nested values... foo.get('bar', {}).get('baz', {})......get('leaf', {}) is really awful.

So if traverse() is the solution to that, great! I'm happy to learn it :) is it python or salt specific? Do you have any link to doc or code for this function?

baby-gnu commented 4 years ago

So if traverse() is the solution to that, great! I'm happy to learn it :) is it python or salt specific? Do you have any link to doc or code for this function?

It's a salt specific filter, you can find the documentation in salt jinja documentation and the source code in salt.utils.data.

It requires at least 2018.3.

myii commented 4 years ago

I did not remove all the pillar.get calls to make changes as minimal as possible. And there are some pillar.get I don't understand ;-)

This was actually informative to dig down into:

The preference can be set pillar[__formulas:print_template_url] for cross-formula purposes with pillar[openssh-formula:print_template_url] as a local override.

"cross-formula purposes" -- sounds very interesting...

baby-gnu commented 4 years ago

"cross-formula purposes" -- sounds very interesting...

It seems to not be used in any other formula.

myii commented 4 years ago

Lovely, thanks for all of the work, @baby-gnu.

@javierbertoli @daks Appreciate the reviews/comments.

saltstack-formulas-travis commented 4 years ago

:tada: This PR is included in version 1.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

javierbertoli commented 4 years ago

Great work, @baby-gnu! Thanks to you too, @myii for your thorough review :yum:

saltstack-formulas / openssh-formula