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

New module = elb_application_lb #19491

Merged
merged 17 commits into from May 31, 2017

Conversation

@wimnat
Copy link
Contributor

commented Dec 18, 2016

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

elb_application_lb

ANSIBLE VERSION

2.3

SUMMARY

New module for AWS application ELBs. I have broken with naming convention compared to the classic elb module ec2_elb_lb because ELB is actually a separate part of boto3 and not part of ec2. I suggest we rename ec2_elb_lb to elb_classic_lb.

I have also not attempted to merge classic and application load balancers in to one module because they are again separate in boto3 API and operate different enough to warrant two modules.

The module is currently unfinished. I know there are bugs and modification of listeners doesn't work properly at the moment either but i have created the PR to get some feedback from the community as I imagine this module is probably quite sought after.

@wimnat

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2016

@gundalow
Copy link
Contributor

left a comment

needs_revision

lib/ansible/modules/cloud/amazon/elb_application_lb.py Outdated
choices: [ 'yes', 'no' ]
idle_timeout:
description:
- The number of seconds to wait before an idle connection is closed

This comment has been minimized.

Copy link
@gundalow

gundalow Dec 19, 2016

Contributor

Missing full stop. descriptions: must be sentences.

This comment has been minimized.

Copy link
@wimnat

wimnat Dec 19, 2016

Author Contributor

Done

@wimnat

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2016

For those asking about a name change here are my reasons:

  • The older "classic" elb module name of ec2_elb_lb i think is not particularly clear and I think it should be renamed to elb_classic_lb. I have already put this forward in the proposals repo.
  • Although the ELBs turn up in the EC2 section of the AWS console, they are not controlled by the ec2 API, they are controlled by a separate ELB API. Ansible dictates that we should follow boto3 naming convention where possible so i think starting the module name with elb_ rather than ec2_ would be proper
  • Someone suggested a name of ec2_alb - aside from the reason above, i think this name is ambiguous and also for the same reasons mentioned here - http://softwareengineering.stackexchange.com/questions/33349/scientific-evidence-that-supports-using-long-variable-names-instead-of-abbreviat
@ryansb
Copy link
Contributor

left a comment

We can definitely add an elb_classic_lb alias that will point at ec2_elb_lb, the renaming process takes multiple cycles because we must have one release (at least) for each phase (numbers assume starting now):

  1. Allow the new name (release the alias) in 2.3 and add a warning on the old name
  2. Release 2.4 and continue warning on the old name
  3. Release 2.5 and rename ec2_elb_lb to _ec2_elb_lb
  4. Release 2.6 without _ec2_elb_lb
lib/ansible/modules/cloud/amazon/elb_application_lb.py Outdated

DOCUMENTATION = '''
---
module: elb_lb_application

This comment has been minimized.

Copy link
@ryansb

ryansb Dec 30, 2016

Contributor

Fix this name to match file name

- A list of the names or IDs of the security groups to assign to the load balancer. Required if state=present.
required: false
default: []
scheme:

This comment has been minimized.

Copy link
@ryansb

ryansb Dec 30, 2016

Contributor

It may be more obvious to name this parameter something like public as a boolean, but I'd be worried about AWS adding some third scheme meaning we have to obsolete the boolean and use their new value. No change required here, but would like your thoughts.

This comment has been minimized.

Copy link
@wimnat

wimnat Feb 7, 2017

Author Contributor

I agree. Chances of AWS creating a third schema are not low.

Also, with this scheme, at least we're following AWS API convention.

lib/ansible/modules/cloud/amazon/elb_application_lb.py Outdated
ssl_policy: # The security policy that defines which ciphers and protocols are supported. The default is the current predefined security policy.
certificates: # The ARN of the certificate (only one certficate ARN should be provided)
default_actions:
- type: forward # Required. Only 'forward' is accepted at this time

This comment has been minimized.

Copy link
@ryansb

ryansb Dec 30, 2016

Contributor

Can this default to forward since that's the only accepted value?

This comment has been minimized.

Copy link
@wimnat

wimnat Feb 7, 2017

Author Contributor

Can't default this as it's not actually an Ansilble parameter. It's a child of listeners.

lib/ansible/modules/cloud/amazon/elb_application_lb.py Outdated
Rules:
- Conditions:
- Field: path-pattern
Values:

This comment has been minimized.

Copy link
@ryansb

ryansb Dec 30, 2016

Contributor

I think this needs to move out one indentation level to match Field

lib/ansible/modules/cloud/amazon/elb_application_lb.py Outdated
- '/test'
Priority: '1'
Actions:
- 'TargetGroupName': 'test-target-group'

This comment has been minimized.

Copy link
@ryansb

ryansb Dec 30, 2016

Contributor

Remove these single-quotes.

lib/ansible/modules/cloud/amazon/elb_application_lb.py Outdated
- Field: path-pattern
Values:
- '/test'
Priority: '1'

This comment has been minimized.

Copy link
@ryansb

ryansb Dec 30, 2016

Contributor

Can this work as an int as well, or must it be quoted?

lib/ansible/modules/cloud/amazon/elb_application_lb.py Outdated
try:
response = connection.describe_target_groups(Names=[tg_name])
except botocore.exceptions.ClientError as e:
module.fail_json(msg=e.message, **camel_dict_to_snake_dict(e.response))

This comment has been minimized.

Copy link
@ryansb

ryansb Dec 30, 2016

Contributor

Please add the kwarg exception=traceback.format_exc() to this failure.

lib/ansible/modules/cloud/amazon/elb_application_lb.py Outdated

try:
return connection.describe_load_balancers(Names=[module.params.get("name")])['LoadBalancers'][0]
except (ClientError, NoCredentialsError) as e:

This comment has been minimized.

Copy link
@ryansb

ryansb Dec 30, 2016

Contributor

Can you handle NoCreds separately to raise a less generic message?

This comment has been minimized.

Copy link
@wimnat

wimnat Mar 12, 2017

Author Contributor

The handling of the exception will be identical as it just uses the message passed from AWS

lib/ansible/modules/cloud/amazon/elb_application_lb.py Outdated
try:
connection.modify_load_balancer_attributes(LoadBalancerArn=elb['LoadBalancerArn'], Attributes=params['Attributes'])
except (ClientError, NoCredentialsError) as e:
module.fail_json(msg=e.message, **camel_dict_to_snake_dict(e.response))

This comment has been minimized.

Copy link
@ryansb

ryansb Dec 30, 2016

Contributor

Add exception=traceback.format_exc() here as well please.

lib/ansible/modules/cloud/amazon/elb_application_lb.py Outdated
try:
params['SecurityGroups'] = get_ec2_security_group_ids_from_names(module.params.get('security_groups'), connection_ec2, boto3=True)
except ValueError as e:
module.fail_json(msg=str(e))

This comment has been minimized.

Copy link
@ryansb

ryansb Dec 30, 2016

Contributor

Add traceback here as well (for line numbers).

@ansibot ansibot added the ci_verified label May 16, 2017

- Protocol: http # Required. The protocol for connections from clients to the load balancer (http or https).
Port: 80 # Required. The port on which the load balancer is listening.
SslPolicy: # The security policy that defines which ciphers and protocols are supported. The default is the current predefined security policy.
Certificates: # The ARN of the certificate (only one certficate ARN should be provided)

This comment has been minimized.

Copy link
@jbscare

jbscare May 16, 2017

Contributor

The later example of the Certificates parameter is much more clear about what's expected here (a list of dicts, and not a string of the ARN, which is what the comment here made it sound like) -- it might be nice if this example matched.

This comment has been minimized.

Copy link
@wimnat

wimnat May 18, 2017

Author Contributor

done

- Protocol: http # Required. The protocol for connections from clients to the load balancer (http or https).
Port: 80 # Required. The port on which the load balancer is listening.
SslPolicy: # The security policy that defines which ciphers and protocols are supported. The default is the current predefined security policy.
Certificates: # The ARN of the certificate (only one certficate ARN should be provided)

This comment has been minimized.

Copy link
@jbscare

jbscare May 16, 2017

Contributor

The later example of the Certificates parameter is much more clear about what's expected here (a list of dicts, and not a string of the ARN, which is what the comment here made it sound like) -- it might be nice if this example matched.

This comment has been minimized.

Copy link
@wimnat

wimnat May 18, 2017

Author Contributor

done

@ryansb ryansb dismissed their stale review May 17, 2017

Addressed

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 18, 2017

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/modules/cloud/amazon/elb_application_lb.py:117:161: E501 line too long (177 > 160 characters)
lib/ansible/modules/cloud/amazon/elb_application_lb.py:140:161: E501 line too long (177 > 160 characters)

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 18, 2017

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/modules/cloud/amazon/elb_application_lb.py:37:157: W291 trailing whitespace
lib/ansible/modules/cloud/amazon/elb_application_lb.py:62:157: W291 trailing whitespace
lib/ansible/modules/cloud/amazon/elb_application_lb.py:67:157: W291 trailing whitespace
lib/ansible/modules/cloud/amazon/elb_application_lb.py:74:158: W291 trailing whitespace

click here for bot help

@s-hertel

This comment has been minimized.

Copy link
Contributor

commented May 20, 2017

Looks great after pep8 is fixed. Edit: I guess there's a problem maybe with the bot allowing spaces in the docs. Hm. @gundalow Do you have thoughts on that since this is a docs issue?

shipit

@s-hertel

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

@wimnat Looking forward to getting this merged. LMK if you want me to make pep8 fixes.

@ansibot ansibot added the stale_ci label May 30, 2017

@wimnat

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2017

@s-hertel i think we're there. Once this and target_group is merged i'm going to have such a party :D

@s-hertel

This comment has been minimized.

Copy link
Contributor

commented May 31, 2017

shipit

@willthames

This comment has been minimized.

Copy link
Contributor

commented May 31, 2017

shipit

@ryansb

ryansb approved these changes May 31, 2017

@ryansb ryansb merged commit 76e56bf into ansible:devel May 31, 2017

1 check passed

Shippable Run 23781 status is SUCCESS.
Details
@ryansb

This comment has been minimized.

Copy link
Contributor

commented May 31, 2017

In a followup PR it would be nice to wrap these calls in AWSRetry.backoff, since there are lots of calls hitting the same API in this module, so it'd be easy to hit limits at scale.

@warrd

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2017

This PR is not python3 compatible so added a follow up #25300

@wimnat wimnat deleted the wimnat:ansible-modules-extras/pull/2946 branch Jun 15, 2017

@ansible ansible locked and limited conversation to collaborators Apr 26, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.