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.

Sign up for GitHub

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

ec2_asg: Add purge_tags to AutoScalingGroups. #960

Merged
merged 11 commits into from
Mar 11, 2022
Merged

ec2_asg: Add purge_tags to AutoScalingGroups. #960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2f972ba
WIP: describe asg tags
mandar242 Feb 25, 2022
d718135
WIP: handle basic add, delete, list tags cases
mandar242 Feb 26, 2022
4de12e4
Fix redundant code for add/remove tags
mandar242 Feb 28, 2022
2da9096
Add integration tests to test tag operations
mandar242 Mar 1, 2022
3cbda28
Sanity fixes, changelogs fragment
mandar242 Mar 2, 2022
046c440
Handle non-existing tags removal
mandar242 Mar 2, 2022
8452d4a
Remove list_tags, remove_tags
mandar242 Mar 5, 2022
6d606d0
Fix changelogs fragment
mandar242 Mar 5, 2022
3d01096
Modified tests based on feedback
mandar242 Mar 7, 2022
43d67eb
Add more tests
mandar242 Mar 8, 2022
4c32a8f
Handle remove all tags, move tag related tests to single file
mandar242 Mar 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/960-ec2_asg-purge-tags.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
minor_changes:
- ec2_asg - add support for ``purge_tags`` to ec2_asg (https://github.com/ansible-collections/community.aws/pull/960).
23 changes: 19 additions & 4 deletions plugins/modules/ec2_asg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,13 @@
- When I(propagate_at_launch) is true the tags will be propagated to the Instances created.
type: list
elements: dict
purge_tags:
description:
- If C(true), existing tags will be purged from the resource to match exactly what is defined by I(tags) parameter.
- If the I(tags) parameter is not set then tags will not be modified.
default: true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgive me for commenting on an already merged PR, but I just found out about this from a complete breakage of all ansible AWS services that use tags.

Why was default=True chosen, for a new field that no one was using previously, deemed a good idea?

Now, everything that touches any asset that uses tags MUST now add a purge_tags: False just to leave things as they are??

Otherwise all tags are deleted???

Existing use case to scale in/out an ASG, and touch nothing else:

- community.aws.ec2_asg:
    name: "{{ item.auto_scaling_group_name }}"
    min_size: "{{ count }}"
    max_size: "{{ count }}"
    desired_capacity: "{{ count }}"
    region: eu-central-1

Now, the above and strips all tags as well.

Given that most ASG tags are used to propagate context information to the ec2 instances, this now destroys all context information for the ASG and the instances it creates have no information about the runtime context.

How is that the "principle of least surprising behavior"?

This now requires adding the purge_tags: False everywhere where tags could be impacted.

- community.aws.ec2_asg:
    name: "{{ item.auto_scaling_group_name }}"
    min_size: "{{ count }}"
    max_size: "{{ count }}"
    desired_capacity: "{{ count }}"
    purge_tags: False       <----- This is now needed everywhere tags are used to say "leave alone" ?
    region: eu-central-1

This is the ONLY case where one has to specify a non-default option just to say: "leave alone".

Note also that

    purge_tags: False

is NOT backwards compatible, because that field didn't exist in previous versions, so you need 2 different cases depending on which side of this version you fall on.

Given that the purge_tags things seems to have propagated to many AWS assets, all in slightly different releases, one needs to account for this on a case by case basis depending on when the purge_tags was introduced into each module.

Now, if the default had been purge_tags=False, none of this would be an issue.
There would have been no changes required as the new behavior would match the old.

Am I the only one complaining about this? I can't believe this didn't hit more people.
Does no use use ansible to alter "only what is specified" and expect the rest to remain as-is?

I can't decide now whether to walk away from ansible as an AWS "minor edit" tool or, try deal with this "maybe we'll change something that destroys something else later, that you can't know about until it happens.

type: bool
version_added: 3.2.0
health_check_period:
description:
- Length of time in seconds after a new EC2 instance comes into service that Auto Scaling starts checking its health.
Expand Down Expand Up @@ -645,6 +652,7 @@
from ansible_collections.amazon.aws.plugins.module_utils.ec2 import AWSRetry
from ansible_collections.amazon.aws.plugins.module_utils.ec2 import snake_dict_to_camel_dict
from ansible_collections.amazon.aws.plugins.module_utils.ec2 import camel_dict_to_snake_dict
from ansible_collections.amazon.aws.plugins.module_utils.ec2 import ansible_dict_to_boto3_filter_list

ASG_ATTRIBUTES = ('AvailabilityZones', 'DefaultCooldown', 'DesiredCapacity',
'HealthCheckGracePeriod', 'HealthCheckType', 'LaunchConfigurationName',
Expand Down Expand Up @@ -1097,6 +1105,7 @@ def create_autoscaling_group(connection):
desired_capacity = module.params.get('desired_capacity')
vpc_zone_identifier = module.params.get('vpc_zone_identifier')
set_tags = module.params.get('tags')
purge_tags = module.params.get('purge_tags')
health_check_period = module.params.get('health_check_period')
health_check_type = module.params.get('health_check_type')
default_cooldown = module.params.get('default_cooldown')
Expand Down Expand Up @@ -1205,9 +1214,12 @@ def create_autoscaling_group(connection):
changed = True

# process tag changes
have_tags = as_group.get('Tags')
want_tags = asg_tags
if purge_tags and not want_tags and have_tags:
connection.delete_tags(Tags=list(have_tags))

if len(set_tags) > 0:
have_tags = as_group.get('Tags')
want_tags = asg_tags
if have_tags:
have_tags.sort(key=lambda x: x["Key"])
if want_tags:
Expand All @@ -1218,9 +1230,11 @@ def create_autoscaling_group(connection):

for dead_tag in set(have_tag_keyvals).difference(want_tag_keyvals):
changed = True
dead_tags.append(dict(ResourceId=as_group['AutoScalingGroupName'],
ResourceType='auto-scaling-group', Key=dead_tag))
if purge_tags:
dead_tags.append(dict(
ResourceId=as_group['AutoScalingGroupName'], ResourceType='auto-scaling-group', Key=dead_tag))
have_tags = [have_tag for have_tag in have_tags if have_tag['Key'] != dead_tag]

if dead_tags:
connection.delete_tags(Tags=dead_tags)

Expand Down Expand Up @@ -1838,6 +1852,7 @@ def main():
wait_timeout=dict(type='int', default=300),
state=dict(default='present', choices=['present', 'absent']),
tags=dict(type='list', default=[], elements='dict'),
purge_tags=dict(type='bool', default=True),
health_check_period=dict(type='int', default=300),
health_check_type=dict(default='EC2', choices=['EC2', 'ELB']),
default_cooldown=dict(type='int', default=300),
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/targets/ec2_asg/tasks/instance_detach.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@
- '{{ init_instance_2 }}'

# pause to allow completion of instance replacement
- name: Pause for 1 minute
- name: Pause for 30 seconds
pause:
seconds: 30

Expand Down Expand Up @@ -137,7 +137,7 @@
- '{{ instance_replace_1 }}'
- '{{ instance_replace_2 }}'

- name: Pause for 1 minute to allow completion of above task
- name: Pause for 30 seconds to allow completion of above task
pause:
seconds: 30

Expand Down
58 changes: 2 additions & 56 deletions tests/integration/targets/ec2_asg/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@
cidr_ip: 0.0.0.0/0
register: sg

- include_tasks: tag_operations.yml

- include_tasks: instance_detach.yml

- name: ensure launch configs exist
Expand Down Expand Up @@ -136,62 +138,6 @@
that:
- "output.viable_instances == 1"

- name: Tag asg
ec2_asg:
name: "{{ resource_prefix }}-asg"
tags:
- tag_a: 'value 1'
propagate_at_launch: no
- tag_b: 'value 2'
propagate_at_launch: yes
register: output

- assert:
that:
- "output.tags | length == 2"
- output is changed

- name: Re-Tag asg (different order)
ec2_asg:
name: "{{ resource_prefix }}-asg"
tags:
- tag_b: 'value 2'
propagate_at_launch: yes
- tag_a: 'value 1'
propagate_at_launch: no
register: output

- assert:
that:
- "output.tags | length == 2"
- output is not changed

- name: Re-Tag asg new tags
ec2_asg:
name: "{{ resource_prefix }}-asg"
tags:
- tag_c: 'value 3'
propagate_at_launch: no
register: output

- assert:
that:
- "output.tags | length == 1"
- output is changed

- name: Re-Tag asg update propagate_at_launch
ec2_asg:
name: "{{ resource_prefix }}-asg"
tags:
- tag_c: 'value 3'
propagate_at_launch: yes
register: output

- assert:
that:
- "output.tags | length == 1"
- output is changed

- name: Enable metrics collection
ec2_asg:
name: "{{ resource_prefix }}-asg"
Expand Down
Loading