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

[WIP] Validate variable names on definition #58062

Draft
wants to merge 22 commits into
base: devel
Choose a base branch
from

Conversation

@mkrizek
Copy link
Contributor

@mkrizek mkrizek commented Jun 19, 2019

SUMMARY

Check valid identifiers and reserved names conflicts in time of variables 'arrival'.

TODO

  • integration tests (ci complete)

So far with this PR we validate:

  • set_facts
  • registered vars
  • play vars_prompt
  • play vars
  • play vars_files
  • block vars
  • task vars
  • include_vars
  • extra vars
  • include/import params
  • role (and include_role) params
  • role vars (vars/main.yml)
  • role defaults (defaults/main.yml)
  • playbook host/group vars
  • inventory host/group vars
  • inventory vars
  • facts
  • loop_control.loop_var/loop_control.index_var

Fixes #56042
Fixes #42561

Supersedes #56509 and #57812

Co-authored-by: Brian Coca bcoca@users.noreply.github.com

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

variables

@mkrizek
Copy link
Contributor Author

@mkrizek mkrizek commented Jun 19, 2019

cc @bcoca

Loading

lib/ansible/plugins/action/include_vars.py Outdated Show resolved Hide resolved
Loading
lib/ansible/plugins/vars/host_group_vars.py Outdated Show resolved Hide resolved
Loading
lib/ansible/utils/vars.py Outdated Show resolved Hide resolved
Loading
Copy link
Member

@bcoca bcoca left a comment

mostly looks good, some minor issues about where to do the filtering (core vs plugins)

Loading

@mkrizek mkrizek force-pushed the validate-variables branch from 2229f92 to 877943a Jun 21, 2019
lib/ansible/vars/validation.py Outdated Show resolved Hide resolved
Loading
lib/ansible/plugins/action/set_fact.py Outdated Show resolved Hide resolved
Loading
@s-hertel
Copy link
Contributor

@s-hertel s-hertel commented Jul 11, 2019

This could be done for adhoc too. That's how I came across it - ansible-playbook was giving me warnings but ansible was not.

Loading

@mkrizek
Copy link
Contributor Author

@mkrizek mkrizek commented Jul 12, 2019

This could be done for adhoc too. That's how I came across it - ansible-playbook was giving me warnings but ansible was not.

@s-hertel It should be independent on the cli used, trying the following simple tests, works as expected:

h1 ansible_connection=local loop_control=1
$ ansible -m set_fact -a 'register=1' h1
 [WARNING]: Found variable using reserved name: loop_control

 [WARNING]: Found variable using reserved name: register

h1 | SUCCESS => {
    "ansible_facts": {
        "register": "1"
    },
    "changed": false
}
$ ansible -m set_fact -a 'True=1' h1
 [WARNING]: Found variable using reserved name: loop_control

ERROR! Invalid variable name specified in facts: 'The variable name 'True' is not valid. Variables must start with a letter or underscore character, and contain only letters, numbers and underscores.'

Was there a particular use case you tried that this PR does not cover?

Loading

@mkrizek
Copy link
Contributor Author

@mkrizek mkrizek commented Jul 12, 2019

As a TODO note to myself: loop_control.loop_var/loop_control.index_var is not covered.

Loading

lib/ansible/vars/reserved.py Outdated Show resolved Hide resolved
Loading
@ansibot ansibot removed the stale_ci label Jul 12, 2019
@ansibot
Copy link
Contributor

@ansibot ansibot commented Jul 16, 2019

@mkrizek This PR was evaluated as a potentially problematic PR for the following reasons:

  • More than 50 changed files.

Such PR can only be merged by human. Contact a Core team member to review this PR on IRC: #ansible-devel on irc.freenode.net

click here for bot help

Loading

@s-hertel
Copy link
Contributor

@s-hertel s-hertel commented Jul 25, 2019

@mkrizek sorry for the belated reply. I think I conflated the things our PRs were fixing incorrectly (or maybe you just haven't yet added it here). I agree it would be nice if the fix was independent of the CLI used. Let me know if I should reopen my PR or if you think it's in the scope of this one.

Here's a reproducer with the two warning I added in (#58984) that I don't get on this branch (the reason I considered it a bug is that ansible-playbook does give this warning):

$ cat test_hosts
---
dev:
  vars:
    environment: "dev"  # overwriting environment does unwanted things
  hosts:
    host0:
    host1:
$ ansible-inventory -i test_hosts --list
 [WARNING]: Found variable using reserved name: environment

{
    "_meta": {
        "hostvars": {
            "host0": {
                "environment": "dev"
            },
            "host1": {
                "environment": "dev"
            }
        }
    },
    "all": {
        "children": [
            "dev",
            "ungrouped"
        ]
    },
    "dev": {
        "hosts": [
            "host0",
            "host1"
        ]
    }
}
$ ansible --inventory test_hosts host0 -m debug -a var=environment
 [WARNING]: Found variable using reserved name: environment

host0 | SUCCESS => {
    "environment": []
}

Loading

@mkrizek
Copy link
Contributor Author

@mkrizek mkrizek commented Jul 31, 2019

@s-hertel using register (other reserved keyword) instead of environment results in a warning:

$ cat test_hosts
dev:
  vars:
    register: random_value
  hosts:
    host0:
    host1:
$ ansible-inventory -i test_hosts --list
 [WARNING]: Found variable using reserved name: register

{
    "_meta": {
        "hostvars": {
            "host0": {
                "register": "random_value"
            },
            "host1": {
                "register": "random_value"
            }
        }
    },
    "all": {
        "children": [
            "dev",
            "ungrouped"
        ]
    },
    "dev": {
        "hosts": [
            "host0",
            "host1"
        ]
    }
}
$ ansible --inventory test_hosts host0 -m debug -a var=register
 [WARNING]: Found variable using reserved name: register

host0 | SUCCESS => {
    "register": "random_value"
}

This is because this PR adds _RESERVE_EXCEPTIONS = frozenset(('environment', 'gather_subset', 'vars')). Honestly I am not sure why as I took that code from @bcoca's #57812.

Loading

@bcoca
Copy link
Member

@bcoca bcoca commented Jul 31, 2019

mostly those are common variables i've seen in many plays, but fine if you remove them or create a config that allows 'whitelisting'

Loading

@mkrizek
Copy link
Contributor Author

@mkrizek mkrizek commented Apr 14, 2020

TODO: check #68950 to see why this PR does not handle that.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment