aws_ec2 fix hostnames

changelogs/fragments/862-aws_ec2-hostnames.yml

@@ -0,0 +1,2 @@
+bugfixes:
+- aws_ec2 - ensure the correct number of hosts are returned when tags as hostnames are used (https://github.com/ansible-collections/amazon.aws/pull/862).

plugins/inventory/aws_ec2.py

@@ -603,17 +603,25 @@ def _get_tag_hostname(self, preference, instance):
             tag_hostnames = tag_hostnames.split(',')
         else:
             tag_hostnames = [tag_hostnames]
+
         tags = boto3_tag_list_to_ansible_dict(instance.get('Tags', []))
+        tag_values = []
         for v in tag_hostnames:
             if '=' in v:
                 tag_name, tag_value = v.split('=')
                 if tags.get(tag_name) == tag_value:
-                    return to_text(tag_name) + "_" + to_text(tag_value)
+                    tag_values.append(to_text(tag_name) + "_" + to_text(tag_value))
             else:
                 tag_value = tags.get(v)
                 if tag_value:
-                    return to_text(tag_value)
-        return None
+                    tag_values.append(to_text(tag_value))
+        return tag_values
+
+    def _sanitize_hostname(self, hostname):
+        if ':' in to_text(hostname):
+            return self._sanitize_group_name(to_text(hostname))
+        else:
+            return to_text(hostname)
 
     def _get_hostname(self, instance, hostnames):
         '''
@@ -625,6 +633,7 @@ def _get_hostname(self, instance, hostnames):
             hostnames = ['dns-name', 'private-dns-name']
 
         hostname = None
+        hostname_list = []
         for preference in hostnames:
             if isinstance(preference, dict):
                 if 'name' not in preference:
@@ -633,18 +642,20 @@ def _get_hostname(self, instance, hostnames):
                 hostname_from_prefix = self._get_hostname(instance, [preference["prefix"]])
                 separator = preference.get("separator", "_")
                 if hostname and hostname_from_prefix and 'prefix' in preference:
-                    hostname = hostname_from_prefix + separator + hostname
+                    hostname = hostname_from_prefix[0] + separator + hostname[0]
             elif preference.startswith('tag:'):
                 hostname = self._get_tag_hostname(preference, instance)
             else:
                 hostname = self._get_boto_attr_chain(preference, instance)
+
             if hostname:
-                break
-        if hostname:
-            if ':' in to_text(hostname):
-                return self._sanitize_group_name((to_text(hostname)))
-            else:
-                return to_text(hostname)
+                if isinstance(hostname, list):
+                    for host in hostname:
+                        hostname_list.append(self._sanitize_hostname(host))
+                elif isinstance(hostname, str):
+                    hostname_list.append(self._sanitize_hostname(hostname))
+
+        return hostname_list
 
     def _query(self, regions, include_filters, exclude_filters, strict_permissions):
         '''
@@ -688,7 +699,7 @@ def _add_hosts(self, hosts, group, hostnames):
             :param hostnames: a list of hostname destination variables in order of preference
         '''
         for host in hosts:
-            hostname = self._get_hostname(host, hostnames)
+            hostname_list = self._get_hostname(host, hostnames)
 
             host = camel_dict_to_snake_dict(host, ignore_list=['Tags'])
             host['tags'] = boto3_tag_list_to_ansible_dict(host.get('tags', []))
@@ -700,9 +711,10 @@ def _add_hosts(self, hosts, group, hostnames):
             # Allow easier grouping by region
             host['placement']['region'] = host['placement']['availability_zone'][:-1]
 
-            if not hostname:
+            if not hostname_list:
                 continue
-            self.inventory.add_host(hostname, group=group)
+            for hostname in hostname_list:
+                self.inventory.add_host(to_text(hostname), group=group)
             hostvars_prefix = self.get_option("hostvars_prefix")
             hostvars_suffix = self.get_option("hostvars_suffix")
             new_vars = dict()
@@ -712,21 +724,23 @@ def _add_hosts(self, hosts, group, hostnames):
                 if hostvars_suffix:
                     hostvar = hostvar + hostvars_suffix
                 new_vars[hostvar] = hostval
-                self.inventory.set_variable(hostname, hostvar, hostval)
+                for hostname in hostname_list:
+                    self.inventory.set_variable(to_text(hostname), hostvar, hostval)
             host.update(new_vars)
 
             # Use constructed if applicable
 
             strict = self.get_option('strict')
 
             # Composed variables
-            self._set_composite_vars(self.get_option('compose'), host, hostname, strict=strict)
+            for hostname in hostname_list:
+                self._set_composite_vars(self.get_option('compose'), host, to_text(hostname), strict=strict)
 
-            # Complex groups based on jinja2 conditionals, hosts that meet the conditional are added to group
-            self._add_host_to_composed_groups(self.get_option('groups'), host, hostname, strict=strict)
+                # Complex groups based on jinja2 conditionals, hosts that meet the conditional are added to group
+                self._add_host_to_composed_groups(self.get_option('groups'), host, to_text(hostname), strict=strict)
 
-            # Create groups based on variable values and add the corresponding hosts to it
-            self._add_host_to_keyed_groups(self.get_option('keyed_groups'), host, hostname, strict=strict)
+                # Create groups based on variable values and add the corresponding hosts to it
+                self._add_host_to_keyed_groups(self.get_option('keyed_groups'), host, to_text(hostname), strict=strict)
 
     def _set_credentials(self, loader):
         '''

tests/integration/targets/inventory_aws_ec2/playbooks/test_populating_inventory_with_hostnames.yml

@@ -0,0 +1,59 @@
+---
+- hosts: 127.0.0.1
+  connection: local
+  gather_facts: false
+  environment: "{{ ansible_test.environment }}"
+  tasks:
+
+    - module_defaults:
+        group/aws:
+          aws_access_key: '{{ aws_access_key }}'
+          aws_secret_key: '{{ aws_secret_key }}'
+          security_token: '{{ security_token | default(omit) }}'
+          region: '{{ aws_region }}'
+      block:
+
+        # Create VPC, subnet, security group, and find image_id to create instance
+        - include_tasks: setup.yml
+
+        # Create new host
+        - name: create a new host
+          ec2_instance:
+            image_id: '{{ image_id }}'
+            name: '{{ resource_prefix }}'
+            tags:
+              Tag1: Test1
+              Tag2: Test2
+            instance_type: t2.micro
+            security_groups: '{{ sg_id }}'
+            vpc_subnet_id: '{{ subnet_id }}'
+            wait: false
+          register: setup_instance
+
+        # refresh inventory
+        - meta: refresh_inventory
+
+        - name: assert groups and hostvars were populated with inventory
+          assert:
+            that:
+              - "'aws_ec2' in groups"
+              - groups['aws_ec2'] | length == 2
+              - "'Test1' in groups['aws_ec2']"
+              - "'Test2' in groups['aws_ec2']"
+              - "'Test1' in hostvars"
+              - "'Test2' in hostvars"
+
+      always:
+
+        - name: remove ec2 instance
+          ec2_instance:
+            instance_type: t2.micro
+            instance_ids: '{{ setup_instance.instance_ids }}'
+            state: absent
+            name: '{{ resource_prefix }}'
+            security_groups: "{{ sg_id }}"
+            vpc_subnet_id: "{{ subnet_id }}"
+          ignore_errors: true
+          when: setup_instance is defined
+
+        - include_tasks: tear_down.yml

tests/integration/targets/inventory_aws_ec2/runme.sh

@@ -40,6 +40,8 @@ ansible-playbook playbooks/create_inventory_config.yml -e "template='inventory_w
 ansible-playbook playbooks/test_populating_inventory_with_concatenation.yml "$@"
 ansible-playbook playbooks/create_inventory_config.yml -e "template='inventory_with_literal_string.yml.j2'" "$@"
 ansible-playbook playbooks/test_populating_inventory_with_literal_string.yml "$@"
+ansible-playbook playbooks/create_inventory_config.yml -e "template='inventory_with_hostnames.yml.j2'" "$@"
+ansible-playbook playbooks/test_populating_inventory_with_hostnames.yml "$@"
 
 # generate inventory config with includes_entries_matching and prepare the tests
 ansible-playbook playbooks/create_inventory_config.yml -e "template='inventory_with_include_or_exclude_filters.yml.j2'" "$@"

tests/integration/targets/inventory_aws_ec2/templates/inventory_with_hostnames.yml.j2

@@ -0,0 +1,21 @@
+plugin: amazon.aws.aws_ec2
+aws_access_key_id: '{{ aws_access_key }}'
+aws_secret_access_key: '{{ aws_secret_key }}'
+{% if security_token | default(false) %}
+aws_security_token: '{{ security_token }}'
+{% endif %}
+regions:
+- '{{ aws_region }}'
+keyed_groups:
+- prefix: tag
+  key: tags
+hostnames:
+# can also be specified using
+# - tag:Tag1,Tag2
+# or
+# - tag:Tag1=Test1,Tag2=Test2
+# or
+- tag:Tag1
+- tag:Tag2
+compose:
+   ansible_host: private_ip_address

tests/unit/plugins/inventory/test_aws_ec2.py

@@ -145,19 +145,19 @@ def test_boto3_conn(inventory):
 
 def test_get_hostname_default(inventory):
     instance = instances['Instances'][0]
-    assert inventory._get_hostname(instance, hostnames=None) == "ec2-12-345-67-890.compute-1.amazonaws.com"
+    assert inventory._get_hostname(instance, hostnames=None)[0] == "ec2-12-345-67-890.compute-1.amazonaws.com"
 
 
 def test_get_hostname(inventory):
     hostnames = ['ip-address', 'dns-name']
     instance = instances['Instances'][0]
-    assert inventory._get_hostname(instance, hostnames) == "12.345.67.890"
+    assert inventory._get_hostname(instance, hostnames)[0] == "12.345.67.890"
 
 
 def test_get_hostname_dict(inventory):
     hostnames = [{'name': 'private-ip-address', 'separator': '_', 'prefix': 'tag:Name'}]
     instance = instances['Instances'][0]
-    assert inventory._get_hostname(instance, hostnames) == "aws_ec2_098.76.54.321"
+    assert inventory._get_hostname(instance, hostnames)[0] == "aws_ec2_098.76.54.321"
 
 
 def test_set_credentials(inventory):