From 4bc4abfe1bda7c1a43c75dc0411a98403c625559 Mon Sep 17 00:00:00 2001 From: Sloane Hertel Date: Wed, 13 Sep 2017 14:19:05 -0400 Subject: [PATCH] [cloud] ec2_group: Handle duplicate names between EC2 classic and VPC groups (#28931) * ec2_group: Handle name conflict with empty vpc_id. If several groups exist with the same name (and vpc_id is None) then treat the group outside the vpc as preferred (same as it would for a vpc group with vpc_id specified). Also don't run the egress rules code in that case. * Handle lack of `IpPermissionsEgress` attribute on EC2 classic groups In EC2 classic groups, the `while True` loop checking for egress permissions will continue infinitely. * Handle incompatible combinations of EC2 Classic + VPC groups * Fix integration tests in accounts lacking EC2 classic This change checks against the security group created, instead of the module parameters, for VPC ID. This means that new accounts with a default VPC will still wait properly for the first egress rule to populate. * Fix conditional for storing described groups with preference for matching VPC IDs * Revert `vpc_id is None` on conditional to allow for default VPCs --- lib/ansible/modules/cloud/amazon/ec2_group.py | 39 +++++++++++++------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/lib/ansible/modules/cloud/amazon/ec2_group.py b/lib/ansible/modules/cloud/amazon/ec2_group.py index 22041c59e3..0dc0095884 100644 --- a/lib/ansible/modules/cloud/amazon/ec2_group.py +++ b/lib/ansible/modules/cloud/amazon/ec2_group.py @@ -385,9 +385,17 @@ def get_target_from_rule(module, client, rule, name, group, groups, vpc_id): group_id = group['GroupId'] groups[group_id] = group groups[group_name] = group - elif group_name in groups: + elif group_name in groups and group.get('VpcId') and groups[group_name].get('VpcId'): + # both are VPC groups, this is ok + group_id = groups[group_name]['GroupId'] + elif group_name in groups and not (group.get('VpcId') or groups[group_name].get('VpcId')): + # both are EC2 classic, this is ok group_id = groups[group_name]['GroupId'] else: + # if we got here, either the target group does not exist, or there + # is a mix of EC2 classic + VPC groups. Mixing of EC2 classic + VPC + # is bad, so we have to create a new SG because no compatible group + # exists if not rule.get('group_desc', '').strip(): module.fail_json(msg="group %s will be automatically created by rule %s and " "no description was provided" % (group_name, rule)) @@ -633,17 +641,24 @@ def main(): groupName = sg['GroupName'] if groupName in groups: # Prioritise groups from the current VPC - if vpc_id is None or sg['VpcId'] == vpc_id: + # even if current VPC is EC2-Classic + if groups[groupName].get('VpcId') == vpc_id: + # Group saved already matches current VPC, change nothing + pass + elif vpc_id is None and groups[groupName].get('VpcId') is None: + # We're in EC2 classic, and the group already saved is as well + # No VPC groups can be used alongside EC2 classic groups + pass + else: + # the current SG stored has no direct match, so we can replace it groups[groupName] = sg else: groups[groupName] = sg - if group_id: - if sg['GroupId'] == group_id: - group = sg - else: - if groupName == name and (vpc_id is None or sg['VpcId'] == vpc_id): - group = sg + if group_id and sg['GroupId'] == group_id: + group = sg + elif groupName == name and (vpc_id is None or sg['VpcId'] == vpc_id): + group = sg # Ensure requested group is absent if state == 'absent': @@ -685,7 +700,7 @@ def main(): # amazon sometimes takes a couple seconds to update the security group so wait till it exists while True: group = get_security_groups_with_backoff(client, GroupIds=[group['GroupId']])['SecurityGroups'][0] - if not group['IpPermissionsEgress']: + if group.get('VpcId') and not group.get('IpPermissionsEgress'): pass else: break @@ -831,8 +846,8 @@ def main(): # If rule already exists, don't later delete it changed, ip_permission = authorize_ip("out", changed, client, group, groupRules, ipv6, ip_permission, module, rule, "ipv6") - else: - # when no egress rules are specified, + elif vpc_id is not None: + # when no egress rules are specified and we're in a VPC, # we add in a default allow all out rule, which was the # default behavior before egress rules were added default_egress_rule = 'out--1-None-None-' + group['GroupId'] + '-0.0.0.0/0' @@ -856,7 +871,7 @@ def main(): del groupRules[default_egress_rule] # Finally, remove anything left in the groupRules -- these will be defunct rules - if purge_rules_egress: + if purge_rules_egress and vpc_id is not None: for (rule, grant) in groupRules.values(): # we shouldn't be revoking 0.0.0.0 egress if grant != '0.0.0.0/0':