From 41bed21e2001b3925ec95472b5e21ff03f79fb68 Mon Sep 17 00:00:00 2001 From: Jill R <4121322+jillr@users.noreply.github.com> Date: Tue, 16 Jun 2020 20:43:06 -0700 Subject: [PATCH] ec2_group: Ensure group-based rule targets have consistent data formats (#69748) Ensure group-based rule targets have consistent data formats throughout the module. Backported from https://github.com/ansible-collections/amazon.aws/pull/33 --- lib/ansible/modules/cloud/amazon/ec2_group.py | 48 ++++++++++++++----- .../targets/ec2_group/tasks/main.yml | 2 + .../ec2_group/tasks/multi_nested_target.yml | 1 + .../ec2_group/tasks/rule_group_create.yml | 12 +++++ 4 files changed, 52 insertions(+), 11 deletions(-) diff --git a/lib/ansible/modules/cloud/amazon/ec2_group.py b/lib/ansible/modules/cloud/amazon/ec2_group.py index 4e05e795d97..1c590d9d0db 100644 --- a/lib/ansible/modules/cloud/amazon/ec2_group.py +++ b/lib/ansible/modules/cloud/amazon/ec2_group.py @@ -385,6 +385,16 @@ def to_permission(rule): def rule_from_group_permission(perm): + """ + Returns a rule dict from an existing security group. + + When using a security group as a target all 3 fields (OwnerId, GroupId, and + GroupName) need to exist in the target. This ensures consistency of the + values that will be compared to desired_ingress or desired_egress + in wait_for_rule_propagation(). + GroupId is preferred as it is more specific except when targeting 'amazon-' + prefixed security groups (such as EC2 Classic ELBs). + """ def ports_from_permission(p): if 'FromPort' not in p and 'ToPort' not in p: return (None, None) @@ -410,24 +420,28 @@ def rule_from_group_permission(perm): if 'UserIdGroupPairs' in perm and perm['UserIdGroupPairs']: for pair in perm['UserIdGroupPairs']: target = ( - pair.get('UserId', None), + pair.get('UserId', current_account_id), pair.get('GroupId', None), - pair.get('GroupName', None), + None, ) if pair.get('UserId', '').startswith('amazon-'): # amazon-elb and amazon-prefix rules don't need # group-id specified, so remove it when querying # from permission target = ( - target[0], + pair.get('UserId', None), None, - target[2], + pair.get('GroupName', None), ) - elif 'VpcPeeringConnectionId' in pair or pair['UserId'] != current_account_id: + elif 'VpcPeeringConnectionId' not in pair and pair['UserId'] != current_account_id: + # EC2-Classic cross-account + pass + elif 'VpcPeeringConnectionId' in pair: + # EC2-VPC cross-account VPC peering target = ( pair.get('UserId', None), pair.get('GroupId', None), - pair.get('GroupName', None), + None, ) yield Rule( @@ -439,7 +453,7 @@ def rule_from_group_permission(perm): ) -@AWSRetry.backoff(tries=5, delay=5, backoff=2.0) +@AWSRetry.backoff(tries=5, delay=5, backoff=2.0, catch_extra_error_codes=['InvalidGroup.NotFound']) def get_security_groups_with_backoff(connection, **kwargs): return connection.describe_security_groups(**kwargs) @@ -494,8 +508,14 @@ def get_target_from_rule(module, client, rule, name, group, groups, vpc_id): AWS accepts an ip range or a security group as target of a rule. This function validate the rule specification and return either a non-None group_id or a non-None ip range. + + When using a security group as a target all 3 fields (OwnerId, GroupId, and + GroupName) need to exist in the target. This ensures consistency of the + values that will be compared to current_rules (from current_ingress and + current_egress) in wait_for_rule_propagation(). """ FOREIGN_SECURITY_GROUP_REGEX = r'^([^/]+)/?(sg-\S+)?/(\S+)' + owner_id = current_account_id group_id = None group_name = None target_group_created = False @@ -503,16 +523,22 @@ def get_target_from_rule(module, client, rule, name, group, groups, vpc_id): validate_rule(module, rule) if rule.get('group_id') and re.match(FOREIGN_SECURITY_GROUP_REGEX, rule['group_id']): # this is a foreign Security Group. Since you can't fetch it you must create an instance of it + # Matches on groups like amazon-elb/sg-5a9c116a/amazon-elb-sg, amazon-elb/amazon-elb-sg, + # and peer-VPC groups like 0987654321/sg-1234567890/example owner_id, group_id, group_name = re.match(FOREIGN_SECURITY_GROUP_REGEX, rule['group_id']).groups() group_instance = dict(UserId=owner_id, GroupId=group_id, GroupName=group_name) groups[group_id] = group_instance groups[group_name] = group_instance - # group_id/group_name are mutually exclusive - give group_id more precedence as it is more specific if group_id and group_name: - group_name = None + if group_name.startswith('amazon-'): + # amazon-elb and amazon-prefix rules don't need group_id specified, + group_id = None + else: + # group_id/group_name are mutually exclusive - give group_id more precedence as it is more specific + group_name = None return 'group', (owner_id, group_id, group_name), False elif 'group_id' in rule: - return 'group', rule['group_id'], False + return 'group', (owner_id, rule['group_id'], None), False elif 'group_name' in rule: group_name = rule['group_name'] if group_name == name: @@ -571,7 +597,7 @@ def get_target_from_rule(module, client, rule, name, group, groups, vpc_id): groups[group_id] = auto_group groups[group_name] = auto_group target_group_created = True - return 'group', group_id, target_group_created + return 'group', (owner_id, group_id, None), target_group_created elif 'cidr_ip' in rule: return 'ipv4', validate_ip(module, rule['cidr_ip']), False elif 'cidr_ipv6' in rule: diff --git a/test/integration/targets/ec2_group/tasks/main.yml b/test/integration/targets/ec2_group/tasks/main.yml index 9b558656cd1..70a7a2ac869 100644 --- a/test/integration/targets/ec2_group/tasks/main.yml +++ b/test/integration/targets/ec2_group/tasks/main.yml @@ -519,6 +519,7 @@ assert: that: - result.changed + - result.warning is not defined - result.ip_permissions|length == 2 - result.ip_permissions[0].user_id_group_pairs or result.ip_permissions[1].user_id_group_pairs @@ -626,6 +627,7 @@ that: - 'result.changed' - 'result.group_id.startswith("sg-")' + - result.warning is not defined # ============================================================ - name: test adding a range of ports and ports given as strings (expected changed=true) (CHECK MODE) diff --git a/test/integration/targets/ec2_group/tasks/multi_nested_target.yml b/test/integration/targets/ec2_group/tasks/multi_nested_target.yml index 876f2a30a3c..a4dfde0ad8b 100644 --- a/test/integration/targets/ec2_group/tasks/multi_nested_target.yml +++ b/test/integration/targets/ec2_group/tasks/multi_nested_target.yml @@ -220,6 +220,7 @@ - assert: that: - result.changed + - result.warning is not defined - 'result.ip_permissions[0].ip_ranges | length == 3 or result.ip_permissions[1].ip_ranges | length == 3' - 'result.ip_permissions[0].ipv6_ranges | length == 3 or result.ip_permissions[1].ipv6_ranges | length == 3' diff --git a/test/integration/targets/ec2_group/tasks/rule_group_create.yml b/test/integration/targets/ec2_group/tasks/rule_group_create.yml index 465bdc569fb..e502920bb79 100644 --- a/test/integration/targets/ec2_group/tasks/rule_group_create.yml +++ b/test/integration/targets/ec2_group/tasks/rule_group_create.yml @@ -51,6 +51,10 @@ - 60 - 80 + - assert: + that: + - result.warning is not defined + - name: Create a group with only the default rule ec2_group: name: '{{ec2_group_name}}-auto-create-1' @@ -87,6 +91,10 @@ state: present register: result + - assert: + that: + - result.warning is not defined + - name: Create a 4th group ec2_group: name: '{{ec2_group_name}}-auto-create-4' @@ -113,6 +121,10 @@ <<: *aws_connection_info state: present + - assert: + that: + - result.warning is not defined + always: - name: tidy up egress rule test security group ec2_group: