deprecate invalid variable names in inventory (#85377)

* deprecate invalid variable names in inventory

* was previously a hard error for `Host`
* added missing check to `Group`
* swapped blanket Python keyword prohibition with Jinja singleton and `not` check

Co-authored-by: Matt Clay <matt@mystile.com>

* fix invalid variable name test

---------

Co-authored-by: Matt Clay <matt@mystile.com>
(cherry picked from commit 3c52b14c9e)

changelogs/fragments/templates_types_datatagging.yml

@@ -92,7 +92,6 @@ breaking_changes:
   - templating - Access to ``_`` prefixed attributes and methods, and methods with known side effects, is no longer permitted.
     In cases where a matching mapping key is present, the associated value will be returned instead of an error. 
     This increases template environment isolation and ensures more consistent behavior between the ``.`` and ``[]`` operators.
-  - inventory - Invalid variable names provided by inventories result in an inventory parse failure. This behavior is now consistent with other variable name usages throughout Ansible.
   - internals - The ``ansible.utils.native_jinja`` Python module has been removed.
   - internals - The ``AnsibleLoader`` and ``AnsibleDumper`` classes for working with YAML are now factory functions and cannot be extended.
   - public API - The ``ansible.vars.fact_cache.FactCache`` wrapper has been removed.

changelogs/fragments/variable_names.yml

@@ -0,0 +1,7 @@
+bugfixes:
+  - variables - Added Jinja scalar singletons (``true``, ``false``, ``none``) to invalid Ansible variable name detection.
+    Previously, variables with these names could be assigned without error, but could not be resolved.
+minor_changes:
+  - variables - Removed restriction on usage of most Python keywords as Ansible variable names.
+deprecated_features:
+  - inventory plugins - Setting invalid Ansible variable names in inventory plugins is deprecated.

lib/ansible/_internal/_templating/_jinja_bits.py

@@ -78,6 +78,21 @@ The values following this prefix up to the first newline are parsed as Jinja2 te
 To include this literal value at the start of a string, a space or other character must precede it.
 """
 
+JINJA_KEYWORDS = frozenset(
+    {
+        # scalar singletons (see jinja2.nodes.Name.can_assign)
+        'true',
+        'false',
+        'none',
+        'True',
+        'False',
+        'None',
+        # other
+        'not',  # unary operator always applicable to names
+    }
+)
+"""Names which have special meaning to Jinja and cannot be resolved as variable names."""
+
 display = Display()
 
 

lib/ansible/inventory/group.py

@@ -26,7 +26,7 @@ from ansible import constants as C
 from ansible.errors import AnsibleError
 from ansible.module_utils.common.text.converters import to_native, to_text
 from ansible.utils.display import Display
-from ansible.utils.vars import combine_vars
+from ansible.utils.vars import combine_vars, validate_variable_name
 
 from . import helpers  # this is left as a module import to facilitate easier unit test patching
 
@@ -221,6 +221,11 @@ class Group:
     def set_variable(self, key: str, value: t.Any) -> None:
         key = helpers.remove_trust(key)
 
+        try:
+            validate_variable_name(key)
+        except AnsibleError as ex:
+            Display().deprecated(msg=f'Accepting inventory variable with invalid name {key!r}.', version='2.23', help_text=ex._help_text, obj=ex.obj)
+
         if key == 'ansible_group_priority':
             self.set_priority(int(value))
         else:

lib/ansible/inventory/host.py

@@ -22,8 +22,10 @@ import typing as t
 
 from collections.abc import Mapping, MutableMapping
 
+from ansible.errors import AnsibleError
 from ansible.inventory.group import Group, InventoryObjectType
 from ansible.parsing.utils.addresses import patterns
+from ansible.utils.display import Display
 from ansible.utils.vars import combine_vars, get_unique_id, validate_variable_name
 
 from . import helpers  # this is left as a module import to facilitate easier unit test patching
@@ -117,7 +119,10 @@ class Host:
     def set_variable(self, key: str, value: t.Any) -> None:
         key = helpers.remove_trust(key)
 
-        validate_variable_name(key)
+        try:
+            validate_variable_name(key)
+        except AnsibleError as ex:
+            Display().deprecated(msg=f'Accepting inventory variable with invalid name {key!r}.', version='2.23', help_text=ex._help_text, obj=ex.obj)
 
         if key in self.vars and isinstance(self.vars[key], MutableMapping) and isinstance(value, Mapping):
             self.vars = combine_vars(self.vars, {key: value})

lib/ansible/utils/vars.py

@@ -28,6 +28,7 @@ from json import dumps
 from ansible import constants as C
 from ansible import context
 from ansible._internal import _json
+from ansible._internal._templating import _jinja_bits
 from ansible.errors import AnsibleError, AnsibleOptionsError
 from ansible.module_utils.datatag import native_type_name
 from ansible.module_utils.common.text.converters import to_native, to_text
@@ -252,6 +253,8 @@ def isidentifier(ident):
 
     Originally posted at https://stackoverflow.com/a/29586366
     """
+    # deprecated: description='Use validate_variable_name instead.' core_version='2.23'
+
     if not isinstance(ident, str):
         return False
 
@@ -269,7 +272,7 @@ def isidentifier(ident):
 
 def validate_variable_name(name: object) -> None:
     """Validate the given variable name is valid, raising an AnsibleError if it is not."""
-    if isinstance(name, str) and isidentifier(name):
+    if isinstance(name, str) and name.isidentifier() and name.isascii() and name not in _jinja_bits.JINJA_KEYWORDS:
         return
 
     if isinstance(name, (str, int, float, bool, type(None))):

test/integration/targets/set_fact/set_fact.yml

@@ -17,7 +17,7 @@
 
     - name: Attempt to use a template to set an invalid variable name with set_fact
       set_fact:
-        "{{ 'continue' }}": value
+        "{{ 'true' }}": value
       register: result
       ignore_errors: yes
 
@@ -25,7 +25,7 @@
       assert:
         that:
           - result is failed
-          - result.msg is contains "Invalid variable name 'continue'"
+          - result.msg is contains "Invalid variable name 'true'"
 
     - name: Attempt to use a template to set an invalid variable name type with set_fact
       set_fact:

test/units/template/test_template.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import io
+import keyword
 import pathlib
 
 import typing as t
@@ -381,3 +382,23 @@ def test_generate_ansible_template_vars(mocker: pytest_mock.MockerFixture, tmp_p
     tvars = _template.generate_ansible_template_vars(path="path", fullpath=str(tmp_path), dest_path=str(tmp_path))
 
     assert tvars['ansible_managed'] == 'value from config'
+
+
+_ALLOWED_PYTHON_KEYWORDS = sorted(set(keyword.kwlist) - _jinja_bits.JINJA_KEYWORDS)
+"""Python keywords which Jinja allows as variable names."""
+
+
+@pytest.mark.parametrize("keyword_name", _ALLOWED_PYTHON_KEYWORDS)
+def test_set_get_keyword(keyword_name: str) -> None:
+    """Verify Python keywords that are not Jinja keywords can be freely used to set and get variables in templates."""
+    template_set_get = TRUST.tag(f"{{% set {keyword_name} = 42 %}}{{{{ {keyword_name} }}}}")
+
+    assert Templar().template(template_set_get) == 42
+
+
+@pytest.mark.parametrize("keyword_name", _ALLOWED_PYTHON_KEYWORDS)
+def test_if_get_keyword(keyword_name: str) -> None:
+    """Verify Python keywords that are not Jinja keywords can be freely used as variables in templates and template conditionals."""
+    template_set_get = TRUST.tag(f"{{% if {keyword_name} == 42 %}}{{{{ {keyword_name} }}}}{{% endif %}}")
+
+    assert Templar(variables={keyword_name: 42}).template(template_set_get) == 42