From 290a8daa8b5bb2692e494286acd81889bbceaa26 Mon Sep 17 00:00:00 2001 From: "patchback[bot]" <45432694+patchback[bot]@users.noreply.github.com> Date: Thu, 13 Oct 2022 08:22:59 +0000 Subject: [PATCH] ec2_instance - update build_run_instance_spec to skip TagSpecification if None (#1151) (#1160) [PR #1151/a9ad1808 backport][stable-5] ec2_instance - update build_run_instance_spec to skip TagSpecification if None This is a backport of PR #1151 as merged into main (a9ad180). 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: Mark Chappell --- .../1148-build_run_instance_spec.yml | 7 + plugins/modules/ec2_instance.py | 44 +++--- .../targets/ec2_vpc_nat_gateway/aliases | 3 + tests/integration/targets/kms_key/aliases | 3 + tests/integration/targets/rds_cluster/aliases | 2 + .../targets/rds_cluster_snapshot/aliases | 2 + .../test_build_run_instance_spec.py | 126 ++++++++++++++++++ .../ec2_instance/test_determine_iam_role.py | 102 ++++++++++++++ 8 files changed, 269 insertions(+), 20 deletions(-) create mode 100644 changelogs/fragments/1148-build_run_instance_spec.yml create mode 100644 tests/unit/plugins/modules/ec2_instance/test_build_run_instance_spec.py create mode 100644 tests/unit/plugins/modules/ec2_instance/test_determine_iam_role.py diff --git a/changelogs/fragments/1148-build_run_instance_spec.yml b/changelogs/fragments/1148-build_run_instance_spec.yml new file mode 100644 index 00000000000..2e8a5f5db52 --- /dev/null +++ b/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). diff --git a/plugins/modules/ec2_instance.py b/plugins/modules/ec2_instance.py index 1e9687fec6b..65440a20885 100644 --- a/plugins/modules/ec2_instance.py +++ b/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), diff --git a/tests/integration/targets/ec2_vpc_nat_gateway/aliases b/tests/integration/targets/ec2_vpc_nat_gateway/aliases index f5291520b8b..5a9dd5bcdce 100644 --- a/tests/integration/targets/ec2_vpc_nat_gateway/aliases +++ b/tests/integration/targets/ec2_vpc_nat_gateway/aliases @@ -1,2 +1,5 @@ +time=10m + cloud/aws + ec2_vpc_nat_gateway_info diff --git a/tests/integration/targets/kms_key/aliases b/tests/integration/targets/kms_key/aliases index 967fd7fe094..36c332ab4ba 100644 --- a/tests/integration/targets/kms_key/aliases +++ b/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 diff --git a/tests/integration/targets/rds_cluster/aliases b/tests/integration/targets/rds_cluster/aliases index 0d778791cb1..6e9f239e073 100644 --- a/tests/integration/targets/rds_cluster/aliases +++ b/tests/integration/targets/rds_cluster/aliases @@ -1,3 +1,5 @@ +time=10m + cloud/aws rds_cluster_info diff --git a/tests/integration/targets/rds_cluster_snapshot/aliases b/tests/integration/targets/rds_cluster_snapshot/aliases index e9ea741b75a..7f2c75f2604 100644 --- a/tests/integration/targets/rds_cluster_snapshot/aliases +++ b/tests/integration/targets/rds_cluster_snapshot/aliases @@ -1,3 +1,5 @@ +time=10m + cloud/aws rds_snapshot_info diff --git a/tests/unit/plugins/modules/ec2_instance/test_build_run_instance_spec.py b/tests/unit/plugins/modules/ec2_instance/test_build_run_instance_spec.py new file mode 100644 index 00000000000..e889b676a76 --- /dev/null +++ b/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 diff --git a/tests/unit/plugins/modules/ec2_instance/test_determine_iam_role.py b/tests/unit/plugins/modules/ec2_instance/test_determine_iam_role.py new file mode 100644 index 00000000000..cdde74c97e2 --- /dev/null +++ b/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']