diff --git a/changelogs/fragments/togggle_invalid_group_chars.yml b/changelogs/fragments/togggle_invalid_group_chars.yml new file mode 100644 index 00000000000..d138948a022 --- /dev/null +++ b/changelogs/fragments/togggle_invalid_group_chars.yml @@ -0,0 +1,2 @@ +minor_changes: + - add toggle to allow user to override invalid group character filter diff --git a/lib/ansible/config/base.yml b/lib/ansible/config/base.yml index 65e58025638..953cae8661d 100644 --- a/lib/ansible/config/base.yml +++ b/lib/ansible/config/base.yml @@ -1439,6 +1439,18 @@ INTERPRETER_PYTHON_FALLBACK: - python # FUTURE: add inventory override once we're sure it can't be abused by a rogue target version_added: "2.8" +TRANSFORM_INVALID_GROUP_CHARS: + name: Transform invalid characters in group names + default: False + description: + - Make ansible transform invalid characters in group names supplied by inventory sources. + - If 'false' it will allow for the group name but warn about the issue. + - When 'true' it will replace any invalid charachters with '_' (underscore). + env: [{name: ANSIBLE_TRANSFORM_INVALID_GROUP_CHARS}] + ini: + - {key: force_valid_group_names, section: defaults} + type: bool + version_added: '2.8' INVALID_TASK_ATTRIBUTE_FAILED: name: Controls whether invalid attributes for a task result in errors instead of warnings default: True diff --git a/lib/ansible/constants.py b/lib/ansible/constants.py index f0986553fb0..2b92c3bb5ca 100644 --- a/lib/ansible/constants.py +++ b/lib/ansible/constants.py @@ -6,6 +6,7 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type import os +import re from ast import literal_eval from jinja2 import Template @@ -117,6 +118,9 @@ TREE_DIR = None VAULT_VERSION_MIN = 1.0 VAULT_VERSION_MAX = 1.0 +# This matches a string that cannot be used as a valid python variable name i.e 'not-valid', 'not!valid@either' '1_nor_This' +INVALID_VARIABLE_NAMES = re.compile(r'^[^a-zA-Z_]|[^a-zA-Z0-9_]') + # FIXME: remove once play_context mangling is removed # the magic variable mapping dictionary below is used to translate # host/inventory variables to fields in the PlayContext diff --git a/lib/ansible/inventory/data.py b/lib/ansible/inventory/data.py index 6de14575218..c87d9385642 100644 --- a/lib/ansible/inventory/data.py +++ b/lib/ansible/inventory/data.py @@ -156,21 +156,25 @@ class InventoryData(object): return matching_host def add_group(self, group): - ''' adds a group to inventory if not there already ''' + ''' adds a group to inventory if not there already, returns named actually used ''' if group: if not isinstance(group, string_types): raise AnsibleError("Invalid group name supplied, expected a string but got %s for %s" % (type(group), group)) if group not in self.groups: g = Group(group) - self.groups[group] = g - self._groups_dict_cache = {} - display.debug("Added group %s to inventory" % group) + if g.name not in self.groups: + self.groups[g.name] = g + self._groups_dict_cache = {} + display.debug("Added group %s to inventory" % group) + group = g.name else: display.debug("group %s already in inventory" % group) else: raise AnsibleError("Invalid empty/false group name provided: %s" % group) + return group + def remove_group(self, group): if group in self.groups: @@ -188,6 +192,8 @@ class InventoryData(object): if host: if not isinstance(host, string_types): raise AnsibleError("Invalid host name supplied, expected a string but got %s for %s" % (type(host), host)) + + # TODO: add to_safe_host_name g = None if group: if group in self.groups: @@ -223,6 +229,8 @@ class InventoryData(object): else: raise AnsibleError("Invalid empty host name provided: %s" % host) + return host + def remove_host(self, host): if host.name in self.hosts: diff --git a/lib/ansible/inventory/group.py b/lib/ansible/inventory/group.py index 52f69af63c6..55f8aad74f1 100644 --- a/lib/ansible/inventory/group.py +++ b/lib/ansible/inventory/group.py @@ -17,9 +17,31 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type +from itertools import chain + +from ansible import constants as C from ansible.errors import AnsibleError +from ansible.module_utils._text import to_native, to_text -from itertools import chain +from ansible.utils.display import Display + +display = Display() + + +def to_safe_group_name(name, replacer="_", force=False, silent=False): + # Converts 'bad' characters in a string to underscores (or provided replacer) so they can be used as Ansible hosts or groups + + if name: # when deserializing we might not have name yet + invalid_chars = C.INVALID_VARIABLE_NAMES.findall(name) + if invalid_chars: + msg = 'invalid character(s) "%s" in group name (%s)' % (to_text(set(invalid_chars)), to_text(name)) + if C.TRANSFORM_INVALID_GROUP_CHARS or force: + name = C.INVALID_VARIABLE_NAMES.sub(replacer, name) + if not silent: + display.warning('Replacing ' + msg) + else: + display.deprecated('Ignoring ' + msg, version='2.12') + return name class Group: @@ -30,7 +52,7 @@ class Group: def __init__(self, name=None): self.depth = 0 - self.name = name + self.name = to_safe_group_name(name) self.hosts = [] self._hosts = None self.vars = {} @@ -148,9 +170,7 @@ class Group: start_ancestors = group.get_ancestors() new_ancestors = self.get_ancestors() if group in new_ancestors: - raise AnsibleError( - "Adding group '%s' as child to '%s' creates a recursive " - "dependency loop." % (group.name, self.name)) + raise AnsibleError("Adding group '%s' as child to '%s' creates a recursive dependency loop." % (to_native(group.name), to_native(self.name))) new_ancestors.add(self) new_ancestors.difference_update(start_ancestors) @@ -188,7 +208,7 @@ class Group: g.depth = depth unprocessed.update(g.child_groups) if depth - start_depth > len(seen): - raise AnsibleError("The group named '%s' has a recursive dependency loop." % self.name) + raise AnsibleError("The group named '%s' has a recursive dependency loop." % to_native(self.name)) def add_host(self, host): if host.name not in self.host_names: diff --git a/lib/ansible/plugins/inventory/__init__.py b/lib/ansible/plugins/inventory/__init__.py index 582a361de39..275c5bade88 100644 --- a/lib/ansible/plugins/inventory/__init__.py +++ b/lib/ansible/plugins/inventory/__init__.py @@ -21,10 +21,10 @@ __metaclass__ = type import hashlib import os -import re import string from ansible.errors import AnsibleError, AnsibleParserError +from ansible.inventory.group import to_safe_group_name as original_safe from ansible.parsing.utils.addresses import parse_address from ansible.plugins import AnsiblePlugin from ansible.plugins.cache import InventoryFileCacheModule @@ -37,13 +37,11 @@ from ansible.utils.display import Display display = Display() -_SAFE_GROUP = re.compile("[^A-Za-z0-9_]") - # Helper methods def to_safe_group_name(name): - ''' Converts 'bad' characters in a string to underscores so they can be used as Ansible hosts or groups ''' - return _SAFE_GROUP.sub("_", name) + # placeholder for backwards compat + return original_safe(name, force=True, silent=True) def detect_range(line=None): @@ -319,6 +317,7 @@ class Constructable(object): self.templar.set_available_variables(variables) for group_name in groups: conditional = "{%% if %s %%} True {%% else %%} False {%% endif %%}" % groups[group_name] + group_name = to_safe_group_name(group_name) try: result = boolean(self.templar.template(conditional)) except Exception as e: @@ -327,8 +326,8 @@ class Constructable(object): continue if result: - # ensure group exists - self.inventory.add_group(group_name) + # ensure group exists, use sanatized name + group_name = self.inventory.add_group(group_name) # add host to group self.inventory.add_child(group_name, host) diff --git a/lib/ansible/plugins/inventory/aws_ec2.py b/lib/ansible/plugins/inventory/aws_ec2.py index e834a9078bc..f16b0e64631 100644 --- a/lib/ansible/plugins/inventory/aws_ec2.py +++ b/lib/ansible/plugins/inventory/aws_ec2.py @@ -446,7 +446,7 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): def _populate(self, groups, hostnames): for group in groups: - self.inventory.add_group(group) + group = self.inventory.add_group(group) self._add_hosts(hosts=groups[group], group=group, hostnames=hostnames) self.inventory.add_child('all', group) diff --git a/lib/ansible/plugins/inventory/foreman.py b/lib/ansible/plugins/inventory/foreman.py index db9beef8463..60d4ee1e57a 100644 --- a/lib/ansible/plugins/inventory/foreman.py +++ b/lib/ansible/plugins/inventory/foreman.py @@ -60,14 +60,12 @@ password: secure validate_certs: False ''' -import re - from distutils.version import LooseVersion from ansible.errors import AnsibleError from ansible.module_utils._text import to_bytes, to_native from ansible.module_utils.common._collections_compat import MutableMapping -from ansible.plugins.inventory import BaseInventoryPlugin, Cacheable +from ansible.plugins.inventory import BaseInventoryPlugin, Cacheable, to_safe_group_name # 3rd party imports try: @@ -185,14 +183,6 @@ class InventoryModule(BaseInventoryPlugin, Cacheable): raise ValueError("More than one set of facts returned for '%s'" % host) return facts - def to_safe(self, word): - '''Converts 'bad' characters in a string to underscores so they can be used as Ansible groups - #> ForemanInventory.to_safe("foo-bar baz") - 'foo_barbaz' - ''' - regex = r"[^A-Za-z0-9\_]" - return re.sub(regex, "_", word.replace(" ", "")) - def _populate(self): for host in self._get_hosts(): @@ -203,8 +193,8 @@ class InventoryModule(BaseInventoryPlugin, Cacheable): # create directly mapped groups group_name = host.get('hostgroup_title', host.get('hostgroup_name')) if group_name: - group_name = self.to_safe('%s%s' % (self.get_option('group_prefix'), group_name.lower())) - self.inventory.add_group(group_name) + group_name = to_safe_group_name('%s%s' % (self.get_option('group_prefix'), group_name.lower().replace(" ", ""))) + group_name = self.inventory.add_group(group_name) self.inventory.add_child(group_name, host['name']) # set host vars from host info @@ -224,7 +214,8 @@ class InventoryModule(BaseInventoryPlugin, Cacheable): try: self.inventory.set_variable(host['name'], p['name'], p['value']) except ValueError as e: - self.display.warning("Could not set parameter hostvar for %s, skipping %s: %s" % (host, p['name'], to_native(p['value']))) + self.display.warning("Could not set hostvar %s to '%s' for the '%s' host, skipping: %s" % + (p['name'], to_native(p['value']), host, to_native(e))) # set host vars from facts if self.get_option('want_facts'): diff --git a/lib/ansible/plugins/inventory/ini.py b/lib/ansible/plugins/inventory/ini.py index e5e5d0dc2e1..216bd81e8a8 100644 --- a/lib/ansible/plugins/inventory/ini.py +++ b/lib/ansible/plugins/inventory/ini.py @@ -77,6 +77,7 @@ EXAMPLES = ''' import ast import re +from ansible.inventory.group import to_safe_group_name from ansible.plugins.inventory import BaseFileInventoryPlugin from ansible.errors import AnsibleError, AnsibleParserError @@ -171,6 +172,8 @@ class InventoryModule(BaseFileInventoryPlugin): if m: (groupname, state) = m.groups() + groupname = to_safe_group_name(groupname) + state = state or 'hosts' if state not in ['hosts', 'children', 'vars']: title = ":".join(m.groups()) diff --git a/lib/ansible/plugins/inventory/script.py b/lib/ansible/plugins/inventory/script.py index 74da40760c5..86e3c9a6ef5 100644 --- a/lib/ansible/plugins/inventory/script.py +++ b/lib/ansible/plugins/inventory/script.py @@ -153,7 +153,7 @@ class InventoryModule(BaseInventoryPlugin, Cacheable): try: got = data_from_meta.get(host, {}) except AttributeError as e: - raise AnsibleError("Improperly formatted host information for %s: %s" % (host, to_native(e))) + raise AnsibleError("Improperly formatted host information for %s: %s" % (host, to_native(e)), orig_exc=e) self._populate_host_vars([host], got) @@ -162,7 +162,7 @@ class InventoryModule(BaseInventoryPlugin, Cacheable): def _parse_group(self, group, data): - self.inventory.add_group(group) + group = self.inventory.add_group(group) if not isinstance(data, dict): data = {'hosts': data} @@ -187,7 +187,7 @@ class InventoryModule(BaseInventoryPlugin, Cacheable): if group != '_meta' and isinstance(data, dict) and 'children' in data: for child_name in data['children']: - self.inventory.add_group(child_name) + child_name = self.inventory.add_group(child_name) self.inventory.add_child(group, child_name) def get_host_variables(self, path, host): diff --git a/lib/ansible/plugins/inventory/toml.py b/lib/ansible/plugins/inventory/toml.py index bc3105d467a..9bd439abdfb 100644 --- a/lib/ansible/plugins/inventory/toml.py +++ b/lib/ansible/plugins/inventory/toml.py @@ -163,7 +163,7 @@ class InventoryModule(BaseFileInventoryPlugin): self.display.warning("Skipping '%s' as this is not a valid group definition" % group) return - self.inventory.add_group(group) + group = self.inventory.add_group(group) if group_data is None: return diff --git a/lib/ansible/plugins/inventory/virtualbox.py b/lib/ansible/plugins/inventory/virtualbox.py index 21031199044..c759f49513d 100644 --- a/lib/ansible/plugins/inventory/virtualbox.py +++ b/lib/ansible/plugins/inventory/virtualbox.py @@ -107,7 +107,7 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): if group == 'all': continue else: - self.inventory.add_group(group) + group = self.inventory.add_group(group) hosts = source_data[group].get('hosts', []) for host in hosts: self._populate_host_vars([host], hostvars.get(host, {}), group) @@ -162,10 +162,10 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): elif k == 'Groups': for group in v.split('/'): if group: + group = self.inventory.add_group(group) + self.inventory.add_child(group, current_host) if group not in cacheable_results: cacheable_results[group] = {'hosts': []} - self.inventory.add_group(group) - self.inventory.add_child(group, current_host) cacheable_results[group]['hosts'].append(current_host) continue diff --git a/lib/ansible/plugins/inventory/yaml.py b/lib/ansible/plugins/inventory/yaml.py index d8b95993653..39433989a7d 100644 --- a/lib/ansible/plugins/inventory/yaml.py +++ b/lib/ansible/plugins/inventory/yaml.py @@ -124,7 +124,7 @@ class InventoryModule(BaseFileInventoryPlugin): if isinstance(group_data, (MutableMapping, NoneType)): try: - self.inventory.add_group(group) + group = self.inventory.add_group(group) except AnsibleError as e: raise AnsibleParserError("Unable to add group %s: %s" % (group, to_text(e))) diff --git a/test/units/regex/test_invalid_var_names.py b/test/units/regex/test_invalid_var_names.py new file mode 100644 index 00000000000..d47e68d3e59 --- /dev/null +++ b/test/units/regex/test_invalid_var_names.py @@ -0,0 +1,27 @@ +# Make coding more python3-ish +from __future__ import (absolute_import, division, print_function) +__metaclass__ = type + +from units.compat import unittest + +from ansible import constants as C + + +test_cases = (('not-valid', ['-'], 'not_valid'), ('not!valid@either', ['!', '@'], 'not_valid_either'), ('1_nor_This', ['1'], '__nor_This')) + + +class TestInvalidVars(unittest.TestCase): + + def test_positive_matches(self): + + for name, invalid, sanitized in test_cases: + self.assertEqual(C.INVALID_VARIABLE_NAMES.findall(name), invalid) + + def test_negative_matches(self): + for name in ('this_is_valid', 'Also_1_valid', 'noproblem'): + self.assertEqual(C.INVALID_VARIABLE_NAMES.findall(name), []) + + def test_get_setting(self): + + for name, invalid, sanitized in test_cases: + self.assertEqual(C.INVALID_VARIABLE_NAMES.sub('_', name), sanitized)