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

Avoid loop.first when working with users and vault_users data #729

Merged
merged 3 commits into from
Jan 13, 2017

Conversation

fullyint
Copy link
Contributor

@fullyint fullyint commented Jan 9, 2017

Fixes variable referenced before assignment in enclosing scope error reported in https://discourse.roots.io/t/8488/. Python 2.7.12 seems to handle the {% if loop.first %} jinja control structure differently than 2.7.11. Examples of loop.first in Trellis: first, second, third.

loop.first

This PR removes use of {% if loop.first %}, which was used in the context of retrieving user password from vault_users (a list) or groups from users (a list). To retrieve a specific user from the list, the code looped over all users, selecting the user with the correct name.

The purpose of loop.first in this looping was to only retrieve one value per user in case someone had naively listed a particular user multiple times in users or vault_users. However, this use of loop.first causes the error above with in python 2.7.12.

There is no failure if the instances of {% if loop.first %} are removed, but then there would be an error in the rare case that someone naively duplicates a user in these lists. So, this PR takes a different approach, avoiding {% ... %} blocks as much as possible.

salt optional

This PR also fixes a templating error that would have occurred if salt (from vault_users) were undefined. Password creation in the Setup users task will no longer fail if someone omits the salt.

first filter

The first filter throws an error if applied to an empty list. Some variables are defined in two steps to avoid such an error.

The revised definition of ansible_become_pass is split in two assignments. First the passwords temporary variable is created. Then passwords | ternary(passwords | first, None) only applies the first filter if passwords is not an empty list. (The temporary passwords list variable would be empty if no password were defined for admin_user in vault_users.)

The other example is that user_secrets_list gathers the list of passwords and salts for a user. This could be an empty list if the user has no secrets in vault_users, or there could be multiple secrets for users who are duplicated in vault_users. The user_secrets variable only applies the first filter if the user_secrets_list is not empty.

@milanvanschaik
Copy link

I've got the same problem. Thanks for the PR, would be great to see it merged soon. 👍

@swalkinshaw
Copy link
Member

Excellent debugging 👍

We might want to look into creating more custom filters/modules to deal with complex things like these. I'd have a little more confidence in them being pure python vs a mix of Jinja filters + Python functions.

@fullyint
Copy link
Contributor Author

After wrestling to simplify, trying many different options, I decided to revert my original commit, then add a commit to just remove the instances of loop.first. We can write a validation somewhere else that checks for duplicate users (e.g., in #562). The logic could look like this:

"{{ (users | map(attribute='name') | sort) != (users | map(attribute='name') | unique | sort) }}"

In addition to removing loop.first, the new commit adjusts the set_fact task that defines ansible_become_pass, adding {% raw %} around the value. Although the raw_vars feature enables a user.password to be templated in the task even if it contains chars like {{, once templated, if we don't re-add the {% raw %} like I've done here, the task's variable definition would read like this:

ansible_become_pass: examp{{le_password

That moment of definition doesn't fail, but the templating failure occurs later, when Ansible uses the ansible_become_pass during the setup task of the next play. So, as the jinja docs mention is possible, I've added the {% raw %} tags inside {{ }} in order to escape and include them in the definition:

"{{ '{% raw %}' }}{{ user.password | default('') }}{{ '{% endraw %}' }}"

@swalkinshaw mentioned this:

We might want to look into creating more custom filters/modules to deal with complex things like these. I'd have a little more confidence in them being pure python vs a mix of Jinja filters + Python functions.

As a viable alternative to the commit described above, I created a trellis-users-magic-var demo branch with a new trellis_users var built from the combination of users and vault_users. Example:

trellis_users:
  admin:
    groups: [...]
    keys: [...]
    password: '...'
    password_hash: '...'
  web:
    groups: [...]
    keys: [...]
    password: '...'
    password_hash: '...'

It's more code, thus more liability, but it simplifies some things. Example from the "Setup users" task:

- password: '{% for user in vault_users | default([]) if user.name == item.name and user.password is defined %}{{ user.password | password_hash("sha512", user.salt | default("") | truncate(16, true, "") | regex_replace("[^\.\/a-zA-Z0-9]", "x")) }}{% else %}{{ None }}{% endfor %}'
+ password: "{{ trellis_users[item.name].password_hash }}"

Let me know if you'd prefer this trellis_users alternative. It is tested and ready to go.

@swalkinshaw
Copy link
Member

Let's just go with what you have now. The trellis_users solution doesn't clean things up nearly as much as I'd hope.

👍

Fixes 'variable referenced before assignment in enclosing scope'
error that appeared with python 2.7.12 and its apparent change in
handling the {% if loop.first %} jinja control structure.
Fixes 'variable referenced before assignment in enclosing scope'
error that appeared with python 2.7.12 and its apparent change in
handling the {% if loop.first %} jinja control structure.
@fullyint fullyint merged commit 85a12d1 into master Jan 13, 2017
@fullyint fullyint deleted the jinja-loop-first branch January 13, 2017 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants