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

var-naming[no-role-prefix] should not trigger on variables starting with ansible_ (builtins) #3456

Closed
fanderchan opened this issue May 17, 2023 · 16 comments
Assignees
Labels

Comments

@fanderchan
Copy link

Summary

ansible-lint reports an error requiring my role variable name to add the prefix "role_name_", but modifying the built-in variable ansible_python_interpreter causes the program to crash.

Issue Type
  • Bug Report
OS / ENVIRONMENT
ansible-lint --version
ansible-lint 6.16.1
  • ansible installation method: pip
  • ansible-lint installation method: pip
STEPS TO REPRODUCE
- name: Set Python interpreter
  set_fact: 
    ansible_python_interpreter: /usr/bin/python3

When running ansible-lint check, the error reported:

WARNING  Listing 1 violation(s) that are fatal  
var-naming[no-role-prefix]: Variables names from within roles should use role_name_ as a prefix. (set_fact: ansible_python_interpreter)
roles/pre_check_and_set/tasks/main.yml:15 Task/Handler: Set Python interpreter
Desired Behavior

ansible-lint should skip checking ansible built-in variables to avoid requiring prefixes and causing program errors.

Actual Behavior

ansible-lint requires role variable names to be prefixed with "role_name_", but modifying the built-in variable ansible_python_interpreter causes the program to crash.

"role_name_ansible_python_interpreter": /usr/bin/python3

Please let ansible-lint skip checking ansible built-in variable names to avoid similar issues.

@fanderchan fanderchan added bug new Triage required labels May 17, 2023
@audgirka
Copy link
Contributor

@fanderchan When I modifying the built-in variable, and run ansible-lint on it the program doesn't crash for me.
And I think it would not be possible to skip the built-in variable check.
@ssbarnea thoughts?

@ssbarnea
Copy link
Member

That is indeed a bug, we should just ignore any name starting with ansible_ as maintaining a list would be probably too much.

@ssbarnea ssbarnea removed the new Triage required label May 17, 2023
@ssbarnea ssbarnea changed the title Please let ansible-lint skip checking ansible built-in variable names to avoid similar issues. var-naming[no-role-prefix] should not trigger on variables starting with ansible_ (builtins) May 17, 2023
@MarkusTeufelberger
Copy link
Contributor

There is also https://docs.ansible.com/ansible/latest/reference_appendices/config.html#inject-facts-as-vars (see discussion at https://old.reddit.com/r/ansible/comments/13giahm/is_it_preferable_to_utilize_ansible/) - probably there should even be a rule that actually only allows ansible_facts and disallows anything that depends on INJECT_FACTS_AS_VARS=True?

Especially considering the comment by @sivel there:

Short answer, use ansible_facts. We are planning, hopefully in 2.16 to begin emitting a deprecation warning on use for all ansible_ prefixed (injected) facts, informing users that the default value for INJECT_FACTS_AS_VARS will be changing from True to False in a few more releases. Hopefully moving everyone to hopefully use ansible_facts.

Aside from this, there is a performance benefit from setting INJECT_FACTS_AS_VARS=False now, and using ansible_facts.

@MarkusTeufelberger
Copy link
Contributor

This is also complicated by the fact (heh!) that ansible_python_interpreter is not a part of ansible_facts, so some should be OK to be prefixed with ansible_ as they are kinda built-in, others not. There is a list at https://docs.ansible.com/ansible/latest/reference_appendices/special_variables.html#special-variables - I hope it is complete, then there wouldn't be much to maintain.

@MarkusTeufelberger
Copy link
Contributor

MarkusTeufelberger commented May 17, 2023

hostvars, group_names or groups are examples for special variables that are probably used from time to time and not prefixed with ansible_ or role names.

@fanderchan
Copy link
Author

@fanderchan When I modifying the built-in variable, and run ansible-lint on it the program doesn't crash for me. And I think it would not be possible to skip the built-in variable check. @ssbarnea thoughts?

I don't mainly want to express the perspective of crash, this is related to my code. I hope to fix ansible-lint to skip checking built-in variables.

@ssbarnea
Copy link
Member

We are talking about setting variables here, not reading them as that is the only place where we check their names. Based on that, I would really be worried if someone tries to write role_name anywhere. In fact I would want to prevent any attempts to set a variable that overlaps with a builtin.

If I am correct, this means that we have two types of behaviors: some sets for builtins should be ignores but others should trigger a new type of violation, like attempting to set role_name.

@MarkusTeufelberger
Copy link
Contributor

The original issue by the way should be solved by setting that variable in the inventory (through host_vars or group_vars). Dynamically changing the Python interpreter after already launching Ansible is not something that I would recommend and unless there is a good reason for this that I am unaware of, I would agree with the linter in this case and not call this a bug/false positive. There might be other builtins that are maybe desirable(?) to be overwritten dynamically, but in general overwriting builtins sounds like something that should rarely/never be done.

I agree that trying to set a builtin variable should be a new/special kind of error (maybe in a different set of rules compared to no-role-prefix), I'm unsure if there are any builtins that should be ignored in such a rule.

@ssbarnea
Copy link
Member

@MarkusTeufelberger You are right, but I do remember good use-cases from few years back where we had to dynamically set python path for some platforms because ansible-core was not able to guess it correctly. That is why I am not in a hurry to declare its use bad.

@MarkusTeufelberger
Copy link
Contributor

dynamically

Really dynamically instead of manually? Interesting.

@ssbarnea
Copy link
Member

ssbarnea commented May 17, 2023

Yeah, involved using using raw command to get distro info and tuning it after. Even today I know distros that need a lot of tunnning to allow ansible to touch them, for example QNAP QuTS as it does not have python pre-installed and even after is installed,... it happens to be in an unpredictable location.

@fanderchan
Copy link
Author

@MarkusTeufelberger You are right, but I do remember good use-cases from few years back where we had to dynamically set python path for some platforms because ansible-core was not able to guess it correctly. That is why I am not in a hurry to declare its use bad.

You are right! My playbook needs to be compatible with many distributions. I've tested and found that some distributions automatically discover the Python interpreter as Python2, but these distributions must use Python3 to execute the yum command. Therefore, I manually set the ansible_python_interpreter after determining the system version in the task. Regardless, I believe there are scenarios where the built-in variables of Ansible need to be manually set, I just haven't encountered many of these situations.

@bcoca
Copy link
Member

bcoca commented May 17, 2023

Clarifying a few things:

  • ansible_facts contains data gathered from the remote, not all ansible_ variables, though many do get converted to such via the INJECT* setting discussed above.
  • While all new variables provided by core will be prefixed ansible_ there are quite a few that predate that norm
  • Not all ansible_ variables are core provided, most do come from ansible_facts, configuration and plugins.
  • There are 3 kind of variables provided by the core engine aside from user defined ones:
    • read only (you can try to assign to them, but this will be ignored) , like ansible_check_mode, playbook_dir, role_path, ansible_skip_tags`, etc. These provide basic information from Ansible core
    • settable by user for configuration purposes, these mostly are defined via a plugin or the base configuration system, i.e ansible_python_interpreter, ansible_remote_tmp, ansible_become_user, etc
    • mixed, this is a subset of the above, i.e ansilbe_user, ansible_connection, etc ... these are both settable by user and core can set them in some cases to reflect the resolution of configuration. Theytend to be very confusing and lead to weird behavior so we are planning on removing the 'resolution' part and would point at the config lookup plugin as an alternative.
  • since 2.15 roles variable handling has changed to what most people will expect (vars/ and defaults/ are still 'exported') but other variables defined in the role or at the role import/include will now be 'private' to the role. This does not include register/set_facts/include vars since these are set to the host and are independent of play objects.
  • while core has always suggested using role prefix in role vars to avoid conflicts, this is not a limitation, sometimes you DO want roles to update/modify common variables. i.e firewall_rules that each role appends to letting the final role consume and set the firewall itself.

@ssbarnea
Copy link
Member

6.12.6 fixed this

@BendingBender
Copy link

I don't get it. What if I want to use a temporary private key to initially connect to a new VM? I need to use ansible_ssh_private_key_file for ansible to use this file. How do I update it in a role without triggering the var-naming rule?

@ssbarnea
Copy link
Member

We have a very short whitelist of vars that are safe to set. Maybe we need to add this one to that list. @sivel wdyt?

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

No branches or pull requests

6 participants