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

route53_health_check: Add feature to create multiple health checks without updating existing health check #1143

Merged
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
minor_changes:
- route53_health_check - Added new parameter `health_check_id` with alias `id` to allow update and delete health check by ID. (https://github.com/ansible-collections/community.aws/pull/1143)
- route53_health_check - Added new parameter `use_unique_names` used with new parameter `health_check_name` with alias `name` to set health check name as unique identifier. (https://github.com/ansible-collections/community.aws/pull/1143)
mandar242 marked this conversation as resolved.
Show resolved Hide resolved
175 changes: 157 additions & 18 deletions plugins/modules/route53_health_check.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
description:
- The type of health check that you want to create, which indicates how
Amazon Route 53 determines whether an endpoint is healthy.
required: true
- Once health_check is created, type can not be changed.
choices: [ 'HTTP', 'HTTPS', 'HTTP_STR_MATCH', 'HTTPS_STR_MATCH', 'TCP' ]
type: str
resource_path:
Expand Down Expand Up @@ -86,6 +86,28 @@
- Will default to C(3) if not specified on creation.
choices: [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]
type: int
health_check_name:
description:
- Name of the Health Check.
- Used together with I(use_unique_names) to set/make use of I(health_check_name) as a unique identifier.
type: str
required: False
aliases: ['name']
version_added: 4.1.0
use_unique_names:
description:
- Used together with I(health_check_name) to set/make use of I(health_check_name) as a unique identifier.
type: bool
required: False
version_added: 4.1.0
health_check_id:
description:
- ID of the health check to be update or deleted.
- If provided, a health check can be updated or deleted based on the ID as unique identifier.
type: str
required: False
aliases: ['id']
version_added: 4.1.0
author:
- "zimbatm (@zimbatm)"
notes:
Expand Down Expand Up @@ -120,10 +142,35 @@
weight: 100
health_check: "{{ my_health_check.health_check.id }}"

- name: create a simple health check with health_check_name as unique identifier
route53_health_check:
mandar242 marked this conversation as resolved.
Show resolved Hide resolved
state: present
health_check_name: ansible
fqdn: ansible.com
port: 443
type: HTTPS
use_unique_names: true

- name: Delete health-check
community.aws.route53_health_check:
state: absent
fqdn: host1.example.com

markuman marked this conversation as resolved.
Show resolved Hide resolved
- name: Update Health check by ID - update ip_address
route53_health_check:
id: 12345678-abcd-abcd-abcd-0fxxxxxxxxxx
ip_address: 1.2.3.4

- name: Update Health check by ID - update port
route53_health_check:
id: 12345678-abcd-abcd-abcd-0fxxxxxxxxxx
ip_address: 8080

- name: Delete Health check by ID
route53_health_check:
state: absent
id: 12345678-abcd-abcd-abcd-0fxxxxxxxxxx

'''

RETURN = r'''
Expand Down Expand Up @@ -249,7 +296,6 @@ def find_health_check(ip_addr, fqdn, hc_type, request_interval, port):
# Additionally, we can't properly wrap the paginator, so retrying means
# starting from scratch with a paginator
results = _list_health_checks()

while True:
for check in results.get('HealthChecks'):
config = check.get('HealthCheckConfig')
Expand All @@ -268,6 +314,20 @@ def find_health_check(ip_addr, fqdn, hc_type, request_interval, port):
return None


def get_existing_checks_with_name():
results = _list_health_checks()
health_checks_with_name = {}
while True:
for check in results.get('HealthChecks'):
if 'Name' in describe_health_check(check['Id'])['tags']:
check_name = describe_health_check(check['Id'])['tags']['Name']
health_checks_with_name[check_name] = check
if results.get('IsTruncated', False):
results = _list_health_checks(Marker=results.get('NextMarker'))
else:
return health_checks_with_name
Comment on lines +320 to +328
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
while True:
for check in results.get('HealthChecks'):
if 'Name' in describe_health_check(check['Id'])['tags']:
check_name = describe_health_check(check['Id'])['tags']['Name']
health_checks_with_name[check_name] = check
if results.get('IsTruncated', False):
results = _list_health_checks(Marker=results.get('NextMarker'))
else:
return health_checks_with_name
while True:
for check in results.get('HealthChecks'):
if 'Name' in describe_health_check(check['Id'])['tags']:
check_name = describe_health_check(check['Id'])['tags']['Name']
health_checks_with_name[check_name] = check
if results.get('IsTruncated', False):
results = _list_health_checks(Marker=results.get('NextMarker'))
else:
return health_checks_with_name

hm we should try avoid while True:.
You can do something like that, where the function calls itself, based in the return value of NextMarker.
but on the other hand do we have multiple while True: constructs already in c.a



def delete_health_check(check_id):
if not check_id:
return False, None
Expand Down Expand Up @@ -348,10 +408,14 @@ def create_health_check(ip_addr_in, fqdn_in, type_in, request_interval_in, port_


def update_health_check(existing_check):
# In theory it's also possible to update the IPAddress, Port and
# FullyQualifiedDomainName, however, because we use these in lieu of a
# 'Name' to uniquely identify the health check this isn't currently
# supported. If we accepted an ID it would be possible to modify them.
# It's possible to update following parameters
# - ResourcePath
# - SearchString
# - FailureThreshold
# - Disabled
# - IPAddress
# - Port
# - FullyQualifiedDomainName

changes = dict()
existing_config = existing_check.get('HealthCheckConfig')
Expand All @@ -372,10 +436,23 @@ def update_health_check(existing_check):
if disabled is not None and disabled != existing_config.get('Disabled'):
changes['Disabled'] = module.params.get('disabled')

# If updating based on Health Check ID or health_check_name, we can update
if module.params.get('health_check_id') or module.params.get('use_unique_names'):
ip_address = module.params.get('ip_address', None)
if ip_address is not None and ip_address != existing_config.get('IPAddress'):
changes['IPAddress'] = module.params.get('ip_address')

port = module.params.get('port', None)
if port is not None and port != existing_config.get('Port'):
changes['Port'] = module.params.get('port')
markuman marked this conversation as resolved.
Show resolved Hide resolved

fqdn = module.params.get('fqdn', None)
if fqdn is not None and fqdn != existing_config.get('FullyQualifiedDomainName'):
changes['FullyQualifiedDomainName'] = module.params.get('fqdn')

# No changes...
if not changes:
return False, None

if module.check_mode:
return True, 'update'

Expand Down Expand Up @@ -419,31 +496,44 @@ def main():
disabled=dict(type='bool'),
ip_address=dict(),
port=dict(type='int'),
type=dict(required=True, choices=['HTTP', 'HTTPS', 'HTTP_STR_MATCH', 'HTTPS_STR_MATCH', 'TCP']),
type=dict(choices=['HTTP', 'HTTPS', 'HTTP_STR_MATCH', 'HTTPS_STR_MATCH', 'TCP']),
resource_path=dict(),
fqdn=dict(),
string_match=dict(),
request_interval=dict(type='int', choices=[10, 30], default=30),
failure_threshold=dict(type='int', choices=[1, 2, 3, 4, 5, 6, 7, 8, 9, 10]),
tags=dict(type='dict', aliases=['resource_tags']),
purge_tags=dict(type='bool'),
health_check_id=dict(type='str', aliases=['id'], required=False),
health_check_name=dict(type='str', aliases=['name'], required=False),
use_unique_names=dict(type='bool', required=False),
)

args_one_of = [
['ip_address', 'fqdn'],
['ip_address', 'fqdn', 'health_check_id'],
]

args_if = [
['type', 'TCP', ('port',)],
]

args_required_together = [
['use_unique_names', 'health_check_name'],
markuman marked this conversation as resolved.
Show resolved Hide resolved
]

args_mutually_exclusive = [
['health_check_id', 'health_check_name']
]

global module
global client

module = AnsibleAWSModule(
argument_spec=argument_spec,
required_one_of=args_one_of,
required_if=args_if,
required_together=args_required_together,
mutually_exclusive=args_mutually_exclusive,
supports_check_mode=True,
)

Expand All @@ -455,6 +545,9 @@ def main():
version='5.0.0', collection_name='community.aws')
module.params['purge_tags'] = False

if not module.params.get('health_check_id') and not module.params.get('type'):
module.fail_json(msg="parameter 'type' is required if not updating or deleting health check by ID.")

state_in = module.params.get('state')
ip_addr_in = module.params.get('ip_address')
port_in = module.params.get('port')
Expand All @@ -464,6 +557,8 @@ def main():
string_match_in = module.params.get('string_match')
request_interval_in = module.params.get('request_interval')
failure_threshold_in = module.params.get('failure_threshold')
health_check_name = module.params.get('health_check_name')
tags = module.params.get('tags')

# Default port
if port_in is None:
Expand All @@ -484,22 +579,66 @@ def main():
action = None
check_id = None

existing_check = find_health_check(ip_addr_in, fqdn_in, type_in, request_interval_in, port_in)

if existing_check:
check_id = existing_check.get('Id')

if module.params.get('use_unique_names') or module.params.get('health_check_id'):
module.deprecate(
'The health_check_name is currently non required parameter.'
' This behavior will change and health_check_name '
' will change to required=True and use_unique_names will change to default=True in release 6.0.0.',
version='6.0.0', collection_name='community.aws')

# If update or delete Health Check based on ID
update_delete_by_id = False
if module.params.get('health_check_id'):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than having two variables (id_to_update_delete and check_id) I think we should update the code at line 561:564 to be:

existing_check = find_health_check(ip_addr_in, fqdn_in, type_in, request_interval_in, port_in)

if module.params.get('health_check_id'):
    # Prefer the ID specified by the user
    check_id = module.params.get('health_check_id')
elif existing_check:
    check_id = existing_check.get('Id') 

It has the same effect of acting on an ID provided by the user regardless of what ID was returned by find_health_check(), but with fewer conditionals later in the code (like the block currently at lines 574:579 under `if state_in == 'absent').

We might want to add a check though to give a warning if the ID returned by find_health_check() is different from module.params.health_check_id, since having data in existing_check for the wrong ID could affect changed status. @markuman What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

We might want to add a check though to give a warning if the ID returned by find_health_check() is different from module.params.health_check_id, since having data in existing_check for the wrong ID could affect changed status. @markuman What do you think?

If module.params.health_check_id is provided, find_health_check() should/must not be invoked imo.

1. introduce a new id parameter that updates existing health checks
...
If the id is not provided, keep the behaviour as it is, just throw warnings that it will change in the future...

But it is invoked, and overwritten later in L571, if health_check_id is provided. FMPOV the logic looks correct. There might be room to improve the condition tree.

The existing integration test passes and also the appended that treated the tag:Name is unique key also.
What currently is missing are some tests that uses the health_check_id to update and delete health checks. But I guess @mandar242 is still in progress with it.

update_delete_by_id = True
id_to_update_delete = module.params.get('health_check_id')
try:
existing_check = client.get_health_check(HealthCheckId=id_to_update_delete)['HealthCheck']
except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e:
module.exit_json(changed=False, msg='The specified health check with ID: {0} does not exist'.format(id_to_update_delete))
else:
existing_check = find_health_check(ip_addr_in, fqdn_in, type_in, request_interval_in, port_in)
if existing_check:
check_id = existing_check.get('Id')

# Delete Health Check
if state_in == 'absent':
changed, action = delete_health_check(check_id)
if update_delete_by_id:
changed, action = delete_health_check(id_to_update_delete)
else:
changed, action = delete_health_check(check_id)
check_id = None

# Create Health Check
elif state_in == 'present':
if existing_check is None:
if existing_check is None and not module.params.get('use_unique_names') and not update_delete_by_id:
changed, action, check_id = create_health_check(ip_addr_in, fqdn_in, type_in, request_interval_in, port_in)

# Update Health Check
else:
changed, action = update_health_check(existing_check)
# If health_check_name is a unique identifier
if module.params.get('use_unique_names'):
existing_checks_with_name = get_existing_checks_with_name()
# update the health_check if another health check with same name exists
if health_check_name in existing_checks_with_name:
changed, action = update_health_check(existing_checks_with_name[health_check_name])
else:
# create a new health_check if another health check with same name does not exists
changed, action, check_id = create_health_check(ip_addr_in, fqdn_in, type_in, request_interval_in, port_in)
# Add tag to add name to health check
if check_id:
if not tags:
tags = {}
tags['Name'] = health_check_name

else:
if update_delete_by_id:
changed, action = update_health_check(existing_check)
else:
changed, action = update_health_check(existing_check)

if check_id:
changed |= manage_tags(module, client, 'healthcheck', check_id,
module.params.get('tags'), module.params.get('purge_tags'))
tags, module.params.get('purge_tags'))

health_check = describe_health_check(id=check_id)
health_check['action'] = action
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

#ip_address: We allocate an EIP due to route53 restrictions
fqdn: '{{ tiny_prefix }}.route53-health.ansible.test'
fqdn_1: '{{ tiny_prefix }}-1.route53-health.ansible.test'
port: 8080
type: 'TCP'
request_interval: 30
Expand All @@ -27,7 +28,9 @@ failure_threshold_updated: 1
# for string_match we need an _STR_MATCH type
type_https_match: 'HTTPS_STR_MATCH'
type_http_match: 'HTTP_STR_MATCH'
type_http: 'HTTP'
resource_path: '/health.php'
resource_path_1: '/new-health.php'
resource_path_updated: '/healthz'
string_match: 'Hello'
string_match_updated: 'Hello World'
Loading