Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check elb type in compare_subnets for elbv2 #50203

Merged
merged 4 commits into from
Jan 29, 2019

Conversation

ezmac
Copy link
Contributor

@ezmac ezmac commented Dec 20, 2018

SUMMARY

Fixes #49558
Add a check for LB type before accessing property that may not be there.

NLB and ALB have different structures for AvailabilityZone property

https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/elbv2.html#ElasticLoadBalancingv2.Client.create_load_balancer

For NLB:

'AvailabilityZones': [
  {
    'ZoneName': 'string',
    'SubnetId': 'string',
    'LoadBalancerAddresses': [
        {
          'IpAddress': 'string',
          'AllocationId': 'string'
        },
      ]
  },
],

For ALB

'AvailabilityZones': [
  {
    'SubnetId': 'subnet-8360a9e7',
    'ZoneName': 'us-west-2a',
  },
  {
    'SubnetId': 'subnet-b7d581c0',
    'ZoneName': 'us-west-2b',
  },
],
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

elbv2.py

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. labels Dec 20, 2018
@ezmac
Copy link
Contributor Author

ezmac commented Dec 24, 2018

Someone might want to add
+label aws

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 1, 2019
@ezmac ezmac closed this Jan 2, 2019
@ezmac ezmac reopened this Jan 2, 2019
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 2, 2019
@ezmac
Copy link
Contributor Author

ezmac commented Jan 7, 2019

ready_for_review

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 15, 2019
@briantist
Copy link
Contributor

briantist commented Jan 18, 2019

Hello @ezmac ! I wrote that part of the code in elbv2.py that does the comparison. Although I wrote it with NLB in mind, it was tested and has worked with ALB as well.

In our own environment, we've quite suddenly been having the missing key problem, even with NLBs that don't have a static IP. This previously, absolutely, worked, because the key was always present, with an array containing an empty dict.

Something has changed, either in boto or in the AWS API return, that's excluding the "empty" entry. I think it may be the API, based on the fact that this seems to have affected different people at different times, and though I may have missed it I don't see anything in the boto project that would have cleared this key out.

To that end, the code in your PR will "fix" ALBs but will still cause NLBs with no static IP to fail.

I recommend removing the conditional about ELB type and replacing it with this smaller fix instead. From the original file's line 172, which was this:

for address in subnet['LoadBalancerAddresses']:

Change it to this:

for address in subnet.get('LoadBalancerAddresses', []):

So far this is working well in my local environment. I could create a separate PR if you want but it may be easier for you to just make the change here.

@ansibot
Copy link
Contributor

ansibot commented Jan 18, 2019

@ezmac This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jan 18, 2019
@ezmac ezmac force-pushed the fix_app_elb_LoadBalancerAddress-49558 branch from 601fdf2 to cfc5289 Compare January 18, 2019 20:00
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 18, 2019
@ezmac
Copy link
Contributor Author

ezmac commented Jan 18, 2019

Thanks for the input; your solution is cleaner and looks like it still works for me.

@briantist
Copy link
Contributor

Thanks for the quick update @ezmac !

@flowerysong
Copy link
Contributor

shipit

@willthames willthames merged commit 71ef69d into ansible:devel Jan 29, 2019
@willthames
Copy link
Contributor

Thanks @ezmac for the fix, @briantist for the improvements and @flowerysong for the review.

@ezmac ezmac deleted the fix_app_elb_LoadBalancerAddress-49558 branch January 29, 2019 13:09
@sivel sivel removed the needs_triage Needs a first human triage before being processed. label Jan 29, 2019
@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

elb_aplication_lb module fails when updating an existing application load balancer
6 participants