ec2_instance - update build_run_instance_spec to skip TagSpecificatio…

…n if None (#1151) (#1162)

[stable-4] ec2_instance - update build_run_instance_spec to skip TagSpecification if None

Partial backport of #1151
SUMMARY
When no tags are supplied, build_run_instance_spec currently includes 'TagSpecification': None.  This results in botocore throwing an exception.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
plugins/modules/ec2_instance.py
ADDITIONAL INFORMATION

Reviewed-by: Gonéri Le Bouder <[email protected]>

changelogs/fragments/1148-build_run_instance_spec.yml

@@ -0,0 +1,3 @@
+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

@@ -283,10 +283,10 @@
     type: dict
   instance_role:
     description:
-      - The ARN or name of an EC2-enabled instance role 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.
+      - 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
   placement_group:
     description:
@@ -1333,28 +1333,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 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

tests/integration/targets/ec2_vpc_nat_gateway/aliases

@@ -1,2 +1,5 @@
+time=10m
+
 cloud/aws
+
 ec2_vpc_nat_gateway_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 = {
+        'instance_role': 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['instance_role'] = 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']