Add toggle to control invalid character substitution in group names (#52748)

* make add_group return proper name
* ensure central transform/check
* added 'silent' option to avoid spamming current users
  those already using the plugins were used to the transformations, so no need to alert them
* centralized valid var names
* dont display dupes
* comment on regex
* added regex tests
  ini and script will now warn about deprecation
* more complete errormsg

14 files changed, 106 insertions, 40 deletions

diff --git a/changelogs/fragments/togggle_invalid_group_chars.yml b/changelogs/fragments/togggle_invalid_group_chars.yml
new file mode 100644
index 0000000000..d138948a02
--- /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 65e5802563..953cae8661 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 f0986553fb..2b92c3bb5c 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 6de1457521..c87d938564 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 52f69af63c..55f8aad74f 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 582a361de3..275c5bade8 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 e834a9078b..f16b0e6463 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 db9beef846..60d4ee1e57 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 e5e5d0dc2e..216bd81e8a 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 74da40760c..86e3c9a6ef 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 bc3105d467..9bd439abdf 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 2103119904..c759f49513 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 d8b9599365..39433989a7 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 0000000000..d47e68d3e5
--- /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)