More work on ELBv2 module_utils (#940)

More work on ELBv2 module_utils

SUMMARY

Refactors LB creation and makes sure that  ip_address_type is set on creation (bug found when working on fixing NLB tests)
Fixes bug with DefaultAction comparisons
Extends tests for _prune_ForwardConfig
Extends tests for _prune_secrets
Fixes KeyError bug uncovered when extending tests for _prune_secrets

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
plugins/module_utils/elbv2.py
ADDITIONAL INFORMATION
Fixes: ansible-collections/community.aws#604
See also: ansible-collections/community.aws#1365

Reviewed-by: Alina Buzachis <None>

changelogs/fragments/604-elb_network_lb.yml

@@ -0,0 +1,6 @@
+bugfixes:
+- module_utils/elbv2 - improvements to idempotency when comparing listeners (https://github.com/ansible-collections/community.aws/issues/604).
+- module_utils/elbv2 - fixes ``KeyError`` when using ``UseExistingClientSecret`` rather than ``ClientSecret`` (https://github.com/ansible-collections/amazon.aws/pull/940).
+minor_changes:
+- module_utils/elbv2 - ensures that ``ip_address_type`` is set on creation rather than re-setting it after creation (https://github.com/ansible-collections/amazon.aws/pull/940).
+- module_utils/elbv2 - uses new waiters with retries for temporary failures (https://github.com/ansible-collections/amazon.aws/pull/940).

plugins/module_utils/elbv2.py

@@ -19,6 +19,54 @@
 from .elb_utils import convert_tg_name_to_arn
 from .elb_utils import get_elb
 from .elb_utils import get_elb_listener
+from .waiters import get_waiter
+
+
+def _simple_forward_config_arn(config, parent_arn):
+    config = deepcopy(config)
+
+    stickiness = config.pop('TargetGroupStickinessConfig', {'Enabled': False})
+    # Stickiness options set, non default value
+    if stickiness != {'Enabled': False}:
+        return False
+
+    target_groups = config.pop('TargetGroups', [])
+
+    # non-default config left over, probably invalid
+    if config:
+        return False
+    # Multiple TGS, not simple
+    if len(target_groups) > 1:
+        return False
+
+    if not target_groups:
+        # with no TGs defined, but an ARN set, this is one of the minimum possible configs
+        return parent_arn or False
+
+    target_group = target_groups[0]
+    # We don't care about the weight with a single TG
+    target_group.pop('Weight', None)
+
+    target_group_arn = target_group.pop('TargetGroupArn', None)
+
+    # non-default config left over
+    if target_group:
+        return False
+
+    # We didn't find an ARN
+    if not (target_group_arn or parent_arn):
+        return False
+
+    # Only one
+    if not parent_arn:
+        return target_group_arn
+    if not target_group_arn:
+        return parent_arn
+
+    if parent_arn != target_group_arn:
+        return False
+
+    return target_group_arn
 
 
 # ForwardConfig may be optional if we've got a single TargetGroupArn entry
@@ -31,30 +79,30 @@ def _prune_ForwardConfig(action):
         return action
     if "ForwardConfig" not in action:
         return action
-    # Where we have multiple TGs, action['TargetGroupArn'] shouldn't be set
-    if "TargetGroupArn" not in action:
-        return action
 
-    # This is the same as having TargetGroupArn set
-    equivalent_action = {
-        'TargetGroupStickinessConfig': {'Enabled': False},
-        'TargetGroups': [{"TargetGroupArn": action["TargetGroupArn"], "Weight": 1}],
-    }
-    if action["ForwardConfig"] != equivalent_action:
+    parent_arn = action.get('TargetGroupArn', None)
+    arn = _simple_forward_config_arn(action["ForwardConfig"], parent_arn)
+    if not arn:
         return action
 
     # Remove the redundant ForwardConfig
     newAction = action.copy()
     del(newAction["ForwardConfig"])
+    newAction["TargetGroupArn"] = arn
     return newAction
 
 
 # the AWS api won't return the client secret, so we'll have to remove it
 # or the module will always see the new and current actions as different
 # and try to apply the same config
 def _prune_secret(action):
-    if action['Type'] == 'authenticate-oidc':
-        action['AuthenticateOidcConfig'].pop('ClientSecret')
+    if action['Type'] != 'authenticate-oidc':
+        return action
+
+    action['AuthenticateOidcConfig'].pop('ClientSecret', None)
+    if action['AuthenticateOidcConfig'].get('UseExistingClientSecret', False):
+        action['AuthenticateOidcConfig'].pop('UseExistingClientSecret')
+
     return action
 
 
@@ -75,22 +123,47 @@ def __init__(self, connection, module):
         self.subnet_mappings = module.params.get("subnet_mappings")
         self.subnets = module.params.get("subnets")
         self.deletion_protection = module.params.get("deletion_protection")
+        self.elb_ip_addr_type = module.params.get("ip_address_type")
         self.wait = module.params.get("wait")
 
         if module.params.get("tags") is not None:
             self.tags = ansible_dict_to_boto3_tag_list(module.params.get("tags"))
         else:
             self.tags = None
+
         self.purge_tags = module.params.get("purge_tags")
 
         self.elb = get_elb(connection, module, self.name)
         if self.elb is not None:
             self.elb_attributes = self.get_elb_attributes()
-            self.elb['tags'] = self.get_elb_tags()
             self.elb_ip_addr_type = self.get_elb_ip_address_type()
+            self.elb['tags'] = self.get_elb_tags()
         else:
             self.elb_attributes = None
-            self.elb_ip_addr_type = None
+
+    def wait_for_ip_type(self, elb_arn, ip_type):
+        """
+        Wait for load balancer to reach 'active' status
+
+        :param elb_arn: The load balancer ARN
+        :return:
+        """
+
+        if not self.wait:
+            return
+
+        waiter_names = {
+            'ipv4': 'load_balancer_ip_address_type_ipv4',
+            'dualstack': 'load_balancer_ip_address_type_dualstack',
+        }
+        if ip_type not in waiter_names:
+            return
+
+        try:
+            waiter = get_waiter(self.connection, waiter_names.get(ip_type))
+            waiter.wait(LoadBalancerArns=[elb_arn])
+        except (BotoCoreError, ClientError) as e:
+            self.module.fail_json_aws(e)
 
     def wait_for_status(self, elb_arn):
         """
@@ -100,8 +173,28 @@ def wait_for_status(self, elb_arn):
         :return:
         """
 
+        if not self.wait:
+            return
+
         try:
-            waiter = self.connection.get_waiter('load_balancer_available')
+            waiter = get_waiter(self.connection, 'load_balancer_available')
+            waiter.wait(LoadBalancerArns=[elb_arn])
+        except (BotoCoreError, ClientError) as e:
+            self.module.fail_json_aws(e)
+
+    def wait_for_deletion(self, elb_arn):
+        """
+        Wait for load balancer to reach 'active' status
+
+        :param elb_arn: The load balancer ARN
+        :return:
+        """
+
+        if not self.wait:
+            return
+
+        try:
+            waiter = get_waiter(self.connection, 'load_balancers_deleted')
             waiter.wait(LoadBalancerArns=[elb_arn])
         except (BotoCoreError, ClientError) as e:
             self.module.fail_json_aws(e)
@@ -200,6 +293,8 @@ def delete(self):
         except (BotoCoreError, ClientError) as e:
             self.module.fail_json_aws(e)
 
+        self.wait_for_deletion(self.elb['LoadBalancerArn'])
+
         self.changed = True
 
     def compare_subnets(self):
@@ -264,15 +359,57 @@ def modify_ip_address_type(self, ip_addr_type):
         Modify ELB ip address type
         :return:
         """
-        if self.elb_ip_addr_type != ip_addr_type:
-            try:
-                AWSRetry.jittered_backoff()(
-                    self.connection.set_ip_address_type
-                )(LoadBalancerArn=self.elb['LoadBalancerArn'], IpAddressType=ip_addr_type)
-            except (BotoCoreError, ClientError) as e:
-                self.module.fail_json_aws(e)
+        if ip_addr_type is None:
+            return
+        if self.elb_ip_addr_type == ip_addr_type:
+            return
+
+        try:
+            AWSRetry.jittered_backoff()(
+                self.connection.set_ip_address_type
+            )(LoadBalancerArn=self.elb['LoadBalancerArn'], IpAddressType=ip_addr_type)
+        except (BotoCoreError, ClientError) as e:
+            self.module.fail_json_aws(e)
+
+        self.changed = True
+        self.wait_for_ip_type(self.elb['LoadBalancerArn'], ip_addr_type)
 
+    def _elb_create_params(self):
+        # Required parameters
+        params = dict()
+        params['Name'] = self.name
+        params['Type'] = self.type
+
+        # Other parameters
+        if self.elb_ip_addr_type is not None:
+            params['IpAddressType'] = self.elb_ip_addr_type
+        if self.subnets is not None:
+            params['Subnets'] = self.subnets
+        if self.subnet_mappings is not None:
+            params['SubnetMappings'] = self.subnet_mappings
+        if self.tags:
+            params['Tags'] = self.tags
+        # Scheme isn't supported for GatewayLBs, so we won't add it here, even though we don't
+        # support them yet.
+
+        return params
+
+    def create_elb(self):
+        """
+        Create a load balancer
+        :return:
+        """
+
+        params = self._elb_create_params()
+
+        try:
+            self.elb = AWSRetry.jittered_backoff()(self.connection.create_load_balancer)(**params)['LoadBalancers'][0]
             self.changed = True
+            self.new_load_balancer = True
+        except (BotoCoreError, ClientError) as e:
+            self.module.fail_json_aws(e)
+
+        self.wait_for_status(self.elb['LoadBalancerArn'])
 
 
 class ApplicationLoadBalancer(ElasticLoadBalancerV2):
@@ -314,37 +451,14 @@ def __init__(self, connection, connection_ec2, module):
         if self.elb is not None and self.elb['Type'] != 'application':
             self.module.fail_json(msg="The load balancer type you are trying to manage is not application. Try elb_network_lb module instead.")
 
-    def create_elb(self):
-        """
-        Create a load balancer
-        :return:
-        """
+    def _elb_create_params(self):
+        params = super()._elb_create_params()
 
-        # Required parameters
-        params = dict()
-        params['Name'] = self.name
-        params['Type'] = self.type
-
-        # Other parameters
-        if self.subnets is not None:
-            params['Subnets'] = self.subnets
-        if self.subnet_mappings is not None:
-            params['SubnetMappings'] = self.subnet_mappings
         if self.security_groups is not None:
             params['SecurityGroups'] = self.security_groups
         params['Scheme'] = self.scheme
-        if self.tags:
-            params['Tags'] = self.tags
-
-        try:
-            self.elb = AWSRetry.jittered_backoff()(self.connection.create_load_balancer)(**params)['LoadBalancers'][0]
-            self.changed = True
-            self.new_load_balancer = True
-        except (BotoCoreError, ClientError) as e:
-            self.module.fail_json_aws(e)
 
-        if self.wait:
-            self.wait_for_status(self.elb['LoadBalancerArn'])
+        return params
 
     def compare_elb_attributes(self):
         """
@@ -485,35 +599,12 @@ def __init__(self, connection, connection_ec2, module):
         if self.elb is not None and self.elb['Type'] != 'network':
             self.module.fail_json(msg="The load balancer type you are trying to manage is not network. Try elb_application_lb module instead.")
 
-    def create_elb(self):
-        """
-        Create a load balancer
-        :return:
-        """
+    def _elb_create_params(self):
+        params = super()._elb_create_params()
 
-        # Required parameters
-        params = dict()
-        params['Name'] = self.name
-        params['Type'] = self.type
-
-        # Other parameters
-        if self.subnets is not None:
-            params['Subnets'] = self.subnets
-        if self.subnet_mappings is not None:
-            params['SubnetMappings'] = self.subnet_mappings
         params['Scheme'] = self.scheme
-        if self.tags:
-            params['Tags'] = self.tags
-
-        try:
-            self.elb = AWSRetry.jittered_backoff()(self.connection.create_load_balancer)(**params)['LoadBalancers'][0]
-            self.changed = True
-            self.new_load_balancer = True
-        except (BotoCoreError, ClientError) as e:
-            self.module.fail_json_aws(e)
 
-        if self.wait:
-            self.wait_for_status(self.elb['LoadBalancerArn'])
+        return params
 
     def modify_elb_attributes(self):
         """