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

2 files changed, 38 insertions, 10 deletions

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: