ec2_instance - update build_run_instance_spec to skip TagSpecificatio…

…n if None (#1151)

ec2_instance - update build_run_instance_spec to skip TagSpecification if None

SUMMARY
fixes: #1148
When no tags are supplied, build_run_instance_spec currently includes 'TagSpecification': None.  This results in botocore throwing an exception.
Also renames instance_role to iam_instance_profile (keeping the original as an alias).  While this could be split off, I'll just perform a partial backport for the bugfix when backporting to 4.x, while working through some unit tests the inaccuracy of the parameter name was apparent.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
plugins/modules/ec2_instance.py
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Gonéri Le Bouder <goneri@lebouder.net>
(cherry picked from commit a9ad180)

changelogs/fragments/1148-build_run_instance_spec.yml

@@ -0,0 +1,7 @@
+minor_changes:
+- ec2_instance - the ``instance_role`` parameter has been renamed to ``iam_instance_profile`` to
+  better reflect what it is, ``instance_role`` remains as an alias
+  (https://github.com/ansible-collections/amazon.aws/pull/1151).
+bugfixes:
+- ec2_instance - fixes ``Invalid type for parameter TagSpecifications, value None`` error when
+  tags aren't specified (https://github.com/ansible-collections/amazon.aws/issues/1148).

plugins/modules/ec2_instance.py

@@ -293,13 +293,14 @@
       - By default, instances are filtered for counting by their "Name" tag, base AMI, state (running, by default), and
         subnet ID. Any queryable filter can be used. Good candidates are specific tags, SSH keys, or security groups.
     type: dict
-  instance_role:
+  iam_instance_profile:
     description:
-      - The ARN or name of an EC2-enabled instance role to be used.
+      - The ARN or name of an EC2-enabled IAM instance profile to be used.
       - If a name is not provided in ARN format then the ListInstanceProfiles permission must also be granted.
         U(https://docs.aws.amazon.com/IAM/latest/APIReference/API_ListInstanceProfiles.html)
       - If no full ARN is provided, the role with a matching name will be used from the active AWS account.
     type: str
+    aliases: ['instance_role']
   placement_group:
     description:
       - The placement group that needs to be assigned to the instance.
@@ -1354,28 +1355,31 @@ def build_run_instance_spec(params):
         MaxCount=1,
         MinCount=1,
     )
-    # network parameters
+    spec.update(**build_top_level_options(params))
+
     spec['NetworkInterfaces'] = build_network_spec(params)
     spec['BlockDeviceMappings'] = build_volume_spec(params)
-    spec.update(**build_top_level_options(params))
-    spec['TagSpecifications'] = build_instance_tags(params)
+
+    tag_spec = build_instance_tags(params)
+    if tag_spec is not None:
+        spec['TagSpecifications'] = tag_spec
 
     # IAM profile
-    if params.get('instance_role'):
-        spec['IamInstanceProfile'] = dict(Arn=determine_iam_role(params.get('instance_role')))
+    if params.get('iam_instance_profile'):
+        spec['IamInstanceProfile'] = dict(Arn=determine_iam_role(params.get('iam_instance_profile')))
 
-    if module.params.get('exact_count'):
-        spec['MaxCount'] = module.params.get('to_launch')
-        spec['MinCount'] = module.params.get('to_launch')
+    if params.get('exact_count'):
+        spec['MaxCount'] = params.get('to_launch')
+        spec['MinCount'] = params.get('to_launch')
 
-    if module.params.get('count'):
-        spec['MaxCount'] = module.params.get('count')
-        spec['MinCount'] = module.params.get('count')
+    if params.get('count'):
+        spec['MaxCount'] = params.get('count')
+        spec['MinCount'] = params.get('count')
 
-    if not module.params.get('launch_template'):
-        spec['InstanceType'] = params['instance_type'] if module.params.get('instance_type') else 't2.micro'
+    if not params.get('launch_template'):
+        spec['InstanceType'] = params['instance_type'] if params.get('instance_type') else 't2.micro'
 
-    if module.params.get('launch_template') and module.params.get('instance_type'):
+    if params.get('launch_template') and params.get('instance_type'):
         spec['InstanceType'] = params['instance_type']
 
     return spec
@@ -1794,9 +1798,9 @@ def determine_iam_role(name_or_arn):
         role = iam.get_instance_profile(InstanceProfileName=name_or_arn, aws_retry=True)
         return role['InstanceProfile']['Arn']
     except is_boto3_error_code('NoSuchEntity') as e:
-        module.fail_json_aws(e, msg="Could not find instance_role {0}".format(name_or_arn))
+        module.fail_json_aws(e, msg="Could not find iam_instance_profile {0}".format(name_or_arn))
     except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:  # pylint: disable=duplicate-except
-        module.fail_json_aws(e, msg="An error occurred while searching for instance_role {0}. Please try supplying the full ARN.".format(name_or_arn))
+        module.fail_json_aws(e, msg="An error occurred while searching for iam_instance_profile {0}. Please try supplying the full ARN.".format(name_or_arn))
 
 
 def handle_existing(existing_matches, state):
@@ -1827,7 +1831,7 @@ def handle_existing(existing_matches, state):
                     module.fail_json_aws(e, msg="Could not apply change {0} to existing instance.".format(str(c)))
         all_changes.extend(changes)
         changed |= bool(changes)
-        changed |= add_or_update_instance_profile(existing_matches[0], module.params.get('instance_role'))
+        changed |= add_or_update_instance_profile(existing_matches[0], module.params.get('iam_instance_profile'))
         changed |= change_network_attachments(existing_matches[0], module.params)
 
     altered = find_instances(ids=[i['InstanceId'] for i in existing_matches])
@@ -2022,7 +2026,7 @@ def main():
         availability_zone=dict(type='str'),
         security_groups=dict(default=[], type='list', elements='str'),
         security_group=dict(type='str'),
-        instance_role=dict(type='str'),
+        iam_instance_profile=dict(type='str', aliases=['instance_role']),
         name=dict(type='str'),
         tags=dict(type='dict', aliases=['resource_tags']),
         purge_tags=dict(type='bool', default=True),

tests/integration/targets/ec2_vpc_nat_gateway/aliases

@@ -1,2 +1,5 @@
+time=10m
+
 cloud/aws
+
 ec2_vpc_nat_gateway_info

tests/integration/targets/kms_key/aliases

@@ -3,6 +3,9 @@
 # No KMS supported waiters, and manual waiting for updates didn't fix the issue either.
 # Issue likely from AWS side - added waits on updates in integration tests to workaround this.
 
+# Some KMS operations are just slow
+time=10m
+
 cloud/aws
 
 kms_key_info

tests/integration/targets/rds_cluster/aliases

@@ -1,3 +1,5 @@
+time=10m
+
 cloud/aws
 
 rds_cluster_info

tests/integration/targets/rds_cluster_snapshot/aliases

@@ -1,3 +1,5 @@
+time=10m
+
 cloud/aws
 
 rds_snapshot_info

tests/unit/plugins/modules/ec2_instance/test_build_run_instance_spec.py

@@ -0,0 +1,126 @@
+# (c) 2022 Red Hat Inc.
+#
+# This file is part of Ansible
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+import pytest
+
+from ansible_collections.amazon.aws.tests.unit.compat.mock import sentinel
+import ansible_collections.amazon.aws.plugins.modules.ec2_instance as ec2_instance_module
+
+
+@pytest.fixture
+def params_object():
+    params = {
+        'iam_instance_profile': None,
+        'exact_count': None,
+        'count': None,
+        'launch_template': None,
+        'instance_type': None,
+    }
+    return params
+
+
+@pytest.fixture
+def ec2_instance(monkeypatch):
+    # monkey patches various ec2_instance module functions, we'll separately test the operation of
+    # these functions, we just care that it's passing the results into the right place in the
+    # instance spec.
+    monkeypatch.setattr(ec2_instance_module, 'build_top_level_options', lambda params: {'TOP_LEVEL_OPTIONS': sentinel.TOP_LEVEL})
+    monkeypatch.setattr(ec2_instance_module, 'build_network_spec', lambda params: sentinel.NETWORK_SPEC)
+    monkeypatch.setattr(ec2_instance_module, 'build_volume_spec', lambda params: sentinel.VOlUME_SPEC)
+    monkeypatch.setattr(ec2_instance_module, 'build_instance_tags', lambda params: sentinel.TAG_SPEC)
+    monkeypatch.setattr(ec2_instance_module, 'determine_iam_role', lambda params: sentinel.IAM_PROFILE_ARN)
+    return ec2_instance_module
+
+
+def _assert_defaults(instance_spec, to_skip=None):
+    if not to_skip:
+        to_skip = []
+
+    assert isinstance(instance_spec, dict)
+
+    if 'TagSpecifications' not in to_skip:
+        assert 'TagSpecifications' in instance_spec
+        assert instance_spec['TagSpecifications'] is sentinel.TAG_SPEC
+
+    if 'NetworkInterfaces' not in to_skip:
+        assert 'NetworkInterfaces' in instance_spec
+        assert instance_spec['NetworkInterfaces'] is sentinel.NETWORK_SPEC
+
+    if 'BlockDeviceMappings' not in to_skip:
+        assert 'BlockDeviceMappings' in instance_spec
+        assert instance_spec['BlockDeviceMappings'] is sentinel.VOlUME_SPEC
+
+    if 'IamInstanceProfile' not in to_skip:
+        # By default, this shouldn't be returned
+        assert 'IamInstanceProfile' not in instance_spec
+
+    if 'MinCount' not in to_skip:
+        assert 'MinCount' in instance_spec
+        instance_spec['MinCount'] == 1
+
+    if 'MaxCount' not in to_skip:
+        assert 'MaxCount' in instance_spec
+        instance_spec['MaxCount'] == 1
+
+    if 'TOP_LEVEL_OPTIONS' not in to_skip:
+        assert 'TOP_LEVEL_OPTIONS' in instance_spec
+        assert instance_spec['TOP_LEVEL_OPTIONS'] is sentinel.TOP_LEVEL
+
+
+def test_build_run_instance_spec_defaults(params_object, ec2_instance):
+    instance_spec = ec2_instance.build_run_instance_spec(params_object)
+    _assert_defaults(instance_spec)
+
+
+def test_build_run_instance_spec_tagging(params_object, ec2_instance, monkeypatch):
+    # build_instance_tags can return None, RunInstance doesn't like this
+    monkeypatch.setattr(ec2_instance_module, 'build_instance_tags', lambda params: None)
+    instance_spec = ec2_instance.build_run_instance_spec(params_object)
+    _assert_defaults(instance_spec, ['TagSpecifications'])
+    assert 'TagSpecifications' not in instance_spec
+
+    # if someone *explicitly* passes {} (rather than not setting it), then [] can be returned
+    monkeypatch.setattr(ec2_instance_module, 'build_instance_tags', lambda params: [])
+    instance_spec = ec2_instance.build_run_instance_spec(params_object)
+    _assert_defaults(instance_spec, ['TagSpecifications'])
+    assert 'TagSpecifications' in instance_spec
+    assert instance_spec['TagSpecifications'] == []
+
+
+def test_build_run_instance_spec_instance_profile(params_object, ec2_instance):
+    params_object['iam_instance_profile'] = sentinel.INSTANCE_PROFILE_NAME
+    instance_spec = ec2_instance.build_run_instance_spec(params_object)
+    _assert_defaults(instance_spec, ['IamInstanceProfile'])
+    assert 'IamInstanceProfile' in instance_spec
+    assert instance_spec['IamInstanceProfile'] == {'Arn': sentinel.IAM_PROFILE_ARN}
+
+
+def test_build_run_instance_spec_count(params_object, ec2_instance):
+    # When someone passes 'count', that number of instances will be *launched*
+    params_object['count'] = sentinel.COUNT
+    instance_spec = ec2_instance.build_run_instance_spec(params_object)
+    _assert_defaults(instance_spec, ['MaxCount', 'MinCount'])
+    assert 'MaxCount' in instance_spec
+    assert 'MinCount' in instance_spec
+    assert instance_spec['MaxCount'] == sentinel.COUNT
+    assert instance_spec['MinCount'] == sentinel.COUNT
+
+
+def test_build_run_instance_spec_exact_count(params_object, ec2_instance):
+    # The "exact_count" logic relies on enforce_count doing the math to figure out how many
+    # instances to start/stop.  The enforce_count call is responsible for ensuring that 'to_launch'
+    # is set and is a positive integer.
+    params_object['exact_count'] = sentinel.EXACT_COUNT
+    params_object['to_launch'] = sentinel.TO_LAUNCH
+    instance_spec = ec2_instance.build_run_instance_spec(params_object)
+
+    _assert_defaults(instance_spec, ['MaxCount', 'MinCount'])
+    assert 'MaxCount' in instance_spec
+    assert 'MinCount' in instance_spec
+    assert instance_spec['MaxCount'] == sentinel.TO_LAUNCH
+    assert instance_spec['MinCount'] == sentinel.TO_LAUNCH

tests/unit/plugins/modules/ec2_instance/test_determine_iam_role.py

@@ -0,0 +1,102 @@
+# (c) 2022 Red Hat Inc.
+#
+# This file is part of Ansible
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+import pytest
+import sys
+
+from ansible_collections.amazon.aws.tests.unit.compat.mock import MagicMock
+from ansible_collections.amazon.aws.tests.unit.compat.mock import sentinel
+import ansible_collections.amazon.aws.plugins.modules.ec2_instance as ec2_instance_module
+import ansible_collections.amazon.aws.plugins.module_utils.arn as utils_arn
+from ansible_collections.amazon.aws.plugins.module_utils.botocore import HAS_BOTO3
+
+try:
+    import botocore
+except ImportError:
+    pass
+
+pytest.mark.skipif(not HAS_BOTO3, reason="test_determine_iam_role.py requires the python modules 'boto3' and 'botocore'")
+
+
+def _client_error(code='GenericError'):
+    return botocore.exceptions.ClientError(
+        {'Error': {'Code': code, 'Message': 'Something went wrong'},
+         'ResponseMetadata': {'RequestId': '01234567-89ab-cdef-0123-456789abcdef'}},
+        'some_called_method')
+
+
+@pytest.fixture
+def params_object():
+    params = {
+        'instance_role': None,
+        'exact_count': None,
+        'count': None,
+        'launch_template': None,
+        'instance_type': None,
+    }
+    return params
+
+
+class FailJsonException(Exception):
+    def __init__(self):
+        pass
+
+
+@pytest.fixture
+def ec2_instance(monkeypatch):
+    monkeypatch.setattr(ec2_instance_module, 'parse_aws_arn', lambda arn: None)
+    monkeypatch.setattr(ec2_instance_module, 'module', MagicMock())
+    ec2_instance_module.module.fail_json.side_effect = FailJsonException()
+    ec2_instance_module.module.fail_json_aws.side_effect = FailJsonException()
+    return ec2_instance_module
+
+
+def test_determine_iam_role_arn(params_object, ec2_instance, monkeypatch):
+    # Revert the default monkey patch to make it simple to try passing a valid ARNs
+    monkeypatch.setattr(ec2_instance, 'parse_aws_arn', utils_arn.parse_aws_arn)
+
+    # Simplest example, someone passes a valid instance profile ARN
+    arn = ec2_instance.determine_iam_role('arn:aws:iam::123456789012:instance-profile/myprofile')
+    assert arn == 'arn:aws:iam::123456789012:instance-profile/myprofile'
+
+
+def test_determine_iam_role_name(params_object, ec2_instance):
+    profile_description = {'InstanceProfile': {'Arn': sentinel.IAM_PROFILE_ARN}}
+    iam_client = MagicMock(**{"get_instance_profile.return_value": profile_description})
+    ec2_instance_module.module.client.return_value = iam_client
+
+    arn = ec2_instance.determine_iam_role(sentinel.IAM_PROFILE_NAME)
+    assert arn == sentinel.IAM_PROFILE_ARN
+
+
+def test_determine_iam_role_missing(params_object, ec2_instance):
+    missing_exception = _client_error('NoSuchEntity')
+    iam_client = MagicMock(**{"get_instance_profile.side_effect": missing_exception})
+    ec2_instance_module.module.client.return_value = iam_client
+
+    with pytest.raises(FailJsonException) as exception:
+        arn = ec2_instance.determine_iam_role(sentinel.IAM_PROFILE_NAME)
+
+    assert ec2_instance_module.module.fail_json_aws.call_count == 1
+    assert ec2_instance_module.module.fail_json_aws.call_args.args[0] is missing_exception
+    assert 'Could not find' in ec2_instance_module.module.fail_json_aws.call_args.kwargs['msg']
+
+
+@pytest.mark.skipif(sys.version_info < (3, 8), reason='call_args behaviour changed in Python 3.8')
+def test_determine_iam_role_missing(params_object, ec2_instance):
+    missing_exception = _client_error()
+    iam_client = MagicMock(**{"get_instance_profile.side_effect": missing_exception})
+    ec2_instance_module.module.client.return_value = iam_client
+
+    with pytest.raises(FailJsonException) as exception:
+        arn = ec2_instance.determine_iam_role(sentinel.IAM_PROFILE_NAME)
+
+    assert ec2_instance_module.module.fail_json_aws.call_count == 1
+    assert ec2_instance_module.module.fail_json_aws.call_args.args[0] is missing_exception
+    assert 'An error occurred while searching' in ec2_instance_module.module.fail_json_aws.call_args.kwargs['msg']
+    assert 'Please try supplying the full ARN' in ec2_instance_module.module.fail_json_aws.call_args.kwargs['msg']