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_eip: add support of updating reverse dns record for eip #2292

Merged
merged 12 commits into from
Sep 27, 2024
Merged

ec2_eip: add support of updating reverse dns record for eip #2292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3cfa776
add support of updating reverse dns record for eip
mandar242 Sep 10, 2024
7711f1f
add return block and changelog fragment
mandar242 Sep 10, 2024
6e565d2
minor fix
mandar242 Sep 10, 2024
9ed640d
modified based on feedback
mandar242 Sep 11, 2024
cdaa6a3
remove additional param
mandar242 Sep 17, 2024
ff496d6
minor fixes
mandar242 Sep 20, 2024
9422ccf
add check_mode and tests
mandar242 Sep 20, 2024
11cf4b0
update tests, add ability to reset reverse dns record
mandar242 Sep 26, 2024
3c0d6c0
minor fix
mandar242 Sep 26, 2024
934a227
add unit tests
mandar242 Sep 26, 2024
a262d83
linter fix, change version_added to 8.3.0
mandar242 Sep 27, 2024
af97b42
linter fix
mandar242 Sep 27, 2024
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
3 changes: 3 additions & 0 deletions changelogs/fragments/2292-ec2_eip-add-support-to-update-reverse-dns-record.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
minor_changes:
- ec2_eip - Add support to update reverse DNS record of an EIP (https://github.com/ansible-collections/amazon.aws/pull/2292).
113 changes: 113 additions & 0 deletions plugins/modules/ec2_eip.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@
- Allocates the new Elastic IP from the provided public IPv4 pool (BYOIP)
only applies to newly allocated Elastic IPs, isn't validated when O(reuse_existing_ip_allowed=true).
type: str
domain_name:
description: The domain name to attach to the IP address.
required: false
type: str
mandar242 marked this conversation as resolved.
Show resolved Hide resolved
version_added: 8.3.0
extends_documentation_fragment:
- amazon.aws.common.modules
- amazon.aws.region.modules
Expand Down Expand Up @@ -201,6 +206,23 @@
tag_name: reserved_for
tag_value: "{{ inventory_hostname }}"
public_ipv4_pool: ipv4pool-ec2-0588c9b75a25d1a02

- name: create new IP and modify it's reverse DNS record
amazon.aws.ec2_eip:
state: present
domain_name: test-domain.xyz

- name: Modify reverse DNS record of an existing EIP
amazon.aws.ec2_eip:
public_ip: 44.224.84.105
domain_name: test-domain.xyz
state: present

- name: Remove reverse DNS record of an existing EIP
amazon.aws.ec2_eip:
public_ip: 44.224.84.105
domain_name: ""
state: present
"""

RETURN = r"""
Expand All @@ -214,6 +236,46 @@
returned: on success
type: str
sample: 52.88.159.209
update_reverse_dns_record_result:
description: Information about result of update reverse dns record operation.
returned: When O(domain_name) is specified.
type: dict
contains:
address:
description: Information about the Elastic IP address.
returned: always
type: dict
contains:
allocation_id:
description: The allocation ID.
returned: always
type: str
sample: "eipalloc-00a11aa111aaa1a11"
ptr_record:
description: The pointer (PTR) record for the IP address.
returned: always
type: str
sample: "ec2-11-22-33-44.us-east-2.compute.amazonaws.com."
ptr_record_update:
description: The updated PTR record for the IP address.
returned: always
type: dict
contains:
status:
description: The status of the PTR record update.
returned: always
type: str
sample: "PENDING"
value:
description: The value for the PTR record update.
returned: always
type: str
sample: "example.com"
public_ip:
description: The public IP address.
returned: always
type: str
sample: "11.22.33.44"
"""

from typing import Any
Expand All @@ -223,6 +285,8 @@
from typing import Tuple
from typing import Union

from ansible.module_utils.common.dict_transformations import camel_dict_to_snake_dict

from ansible_collections.amazon.aws.plugins.module_utils.ec2 import AnsibleEC2Error
from ansible_collections.amazon.aws.plugins.module_utils.ec2 import allocate_address as allocate_ip_address
from ansible_collections.amazon.aws.plugins.module_utils.ec2 import associate_address
Expand Down Expand Up @@ -410,6 +474,7 @@ def ensure_present(
public_ipv4_pool = module.params.get("public_ipv4_pool")
tags = module.params.get("tags")
purge_tags = module.params.get("purge_tags")
domain_name = module.params.get("domain_name")

# Tags for *searching* for an EIP.
search_tags = generate_tag_dict(module)
Expand All @@ -422,6 +487,12 @@ def ensure_present(
client, module.check_mode, search_tags, domain, reuse_existing_ip_allowed, tags, public_ipv4_pool
)

if domain_name is not None:
changed, update_reverse_dns_record_result = update_reverse_dns_record_of_eip(
client, module, address, domain_name
)
result.update({"update_reverse_dns_record_result": update_reverse_dns_record_result})

# Associate address to instance
if device_id:
# Find instance
Expand Down Expand Up @@ -462,13 +533,55 @@ def ensure_present(
client, module, address["AllocationId"], resource_type="elastic-ip", tags=tags, purge_tags=purge_tags
)
result.update({"public_ip": address["PublicIp"], "allocation_id": address["AllocationId"]})

result["changed"] = changed
return result


def update_reverse_dns_record_of_eip(client, module: AnsibleAWSModule, address, domain_name):
if module.check_mode:
return True, {}

current_ptr_record_domain = client.describe_addresses_attribute(
AllocationIds=[address["AllocationId"]], Attribute="domain-name"
)

if (
current_ptr_record_domain["Addresses"]
and current_ptr_record_domain["Addresses"][0]["PtrRecord"] == domain_name + "."
):
return False, {"ptr_record": domain_name + "."}

if len(domain_name) == 0:
try:
update_reverse_dns_record_result = client.reset_address_attribute(
AllocationId=address["AllocationId"], Attribute="domain-name"
)
changed = True
except AnsibleEC2Error as e:
module.fail_json_aws_error(e)

if "ResponseMetadata" in update_reverse_dns_record_result:
del update_reverse_dns_record_result["ResponseMetadata"]
else:
try:
update_reverse_dns_record_result = client.modify_address_attribute(
AllocationId=address["AllocationId"], DomainName=domain_name
)
changed = True
except AnsibleEC2Error as e:
module.fail_json_aws_error(e)

if "ResponseMetadata" in update_reverse_dns_record_result:
del update_reverse_dns_record_result["ResponseMetadata"]

return changed, camel_dict_to_snake_dict(update_reverse_dns_record_result)


def main():
argument_spec = dict(
device_id=dict(required=False),
domain_name=dict(required=False, type="str"),
public_ip=dict(required=False, aliases=["ip"]),
state=dict(required=False, default="present", choices=["present", "absent"]),
in_vpc=dict(required=False, type="bool", default=False),
Expand Down
2 changes: 2 additions & 0 deletions tests/integration/targets/ec2_eip/defaults/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@ eip_test_tags:
AnsibleEIPTestPrefix: "{{ resource_prefix }}"
eip_info_filters:
tag:AnsibleEIPTestPrefix: "{{ resource_prefix }}"
test_domain: "{{ resource_prefix }}.example.xyz"
test_hosted_zone: "{{ resource_prefix }}.example.xyz"
3 changes: 3 additions & 0 deletions tests/integration/targets/ec2_eip/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,8 @@
- ansible.builtin.include_tasks: tasks/attach_detach_to_eni.yml
- ansible.builtin.include_tasks: tasks/attach_detach_to_instance.yml

# Disabled as it requires a registered domain, and corresponding hosted zone
# - ansible.builtin.include_tasks: tasks/update_reverse_dns_record.yml

always:
- ansible.builtin.include_tasks: tasks/teardown.yml
117 changes: 117 additions & 0 deletions tests/integration/targets/ec2_eip/tasks/update_reverse_dns_record.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
- name: Test EIP allocation and reverse DNS record operations
block:
# ------------------------------------------------------------------------------------------
# Allocate EIP with reverse DNS record - check mode
# ------------------------------------------------------------------------------------------
- name: Allocate a new EIP and modify it's reverse DNS record - check_mode
amazon.aws.ec2_eip:
state: present
domain_name: "{{ test_domain }}"
tags: "{{ eip_test_tags }}"
register: eip
check_mode: true

- name: Assert that task result was as expected
ansible.builtin.assert:
that:
- eip is changed

- name: Ensure no new EIP was created
ansible.builtin.include_tasks: tasks/common.yml
vars:
has_no_new_eip: true

# ------------------------------------------------------------------------------------------
# Allocate EIP with reverse DNS record
# ------------------------------------------------------------------------------------------
- name: Allocate a new EIP and modify it's reverse DNS record
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also please add an idempotency check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to slack channel for discussion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alinabuzachis I have updated the tested to include more testing. The tests pass locally

        "ec2:DescribeVpcs",
        "ec2:DeleteVpc"
    ],
    "vpc": {}
}

PLAY RECAP *********************************************************************
testhost                   : ok=45   changed=17   unreachable=0    failed=0    skipped=16   rescued=0    ignored=0   

AWS ACTIONS: ['ec2:AllocateAddress', 'ec2:AttachInternetGateway', 'ec2:AuthorizeSecurityGroupIngress', 'ec2:CreateInternetGateway', 'ec2:CreateNetworkInterface', 'ec2:CreateSecurityGroup', 'ec2:CreateSubnet', 'ec2:CreateVpc', 'ec2:DeleteInternetGateway', 'ec2:DeleteNetworkInterface', 'ec2:DeleteSecurityGroup', 'ec2:DeleteSubnet', 'ec2:DeleteVpc', 'ec2:DescribeAddresses', 'ec2:DescribeAddressesAttribute', 'ec2:DescribeAvailabilityZones', 'ec2:DescribeInternetGateways', 'ec2:DescribeNetworkInterfaces', 'ec2:DescribeSecurityGroups', 'ec2:DescribeSubnets', 'ec2:DescribeTags', 'ec2:DescribeVpcAttribute', 'ec2:DescribeVpcs', 'ec2:DetachInternetGateway', 'ec2:ModifyAddressAttribute', 'ec2:ModifyVpcAttribute', 'ec2:ReleaseAddress', 'ec2:ResetAddressAttribute', 'route53:ChangeResourceRecordSets', 'route53:GetChange', 'route53:ListHostedZones', 'route53:ListResourceRecordSets', 'sts:GetCallerIdentity']
Command exited with status 0 after 435.9944279193878 seconds.

But, for now, I have disabled the tests for reverse DNS record feature as it requires use of registered domain and a corresponding hosted zone to successfully add reverse DNS record to an EIP.
Slack thread started for discussion on how to handle this.

amazon.aws.ec2_eip:
state: present
domain_name: "{{ test_domain }}"
tags: "{{ eip_test_tags }}"
register: eip

- name: Add EIP IP address an A record
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need this step to test the newly added parameter? While it is beneficial to have an additional test for full functionality, I believe that if ec2_eip supports adding or modifying the EIP with a domain name and runs correctly in our CI account, we might not need to do the Route53 testing. What do you think? cc @alinabuzachis @abikouo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from what I understand if A record is not present in the hosted zone of domain being added to EIP reverse DNS recrod,

  1. The reverse DNS record does not get applied and fails
  2. There will be no way to test idempotency as there will be no successful EIP with domain applied
    that's why initially I had skipped adding idempotency test @GomathiselviS @alinabuzachis

amazon.aws.route53:
state: present
zone: "{{ test_hosted_zone }}"
record: "{{ test_domain }}"
type: A
ttl: 7200
value: "{{ eip.public_ip}}"
identifier: "{{ resource_prefix }}"
wait: true

- name: Wait for reverse DNS record update to complete
pause:
minutes: 3

- name: Assert that task result was as expected
ansible.builtin.assert:
that:
- eip is changed
- eip.public_ip is defined and ( eip.public_ip | ansible.utils.ipaddr )
- eip.allocation_id is defined and eip.allocation_id.startswith("eipalloc-")
- eip.update_reverse_dns_record_result is defined
- eip.update_reverse_dns_record_result.address.ptr_record_update is defined
- eip.update_reverse_dns_record_result.address.ptr_record_update.value == "{{ test_domain }}."

# ------------------------------------------------------------------------------------------
# Allocate EIP with reverse DNS record - idempotence
# ------------------------------------------------------------------------------------------
- name: Try modifying reverse DNS record of EIP to same domain as current - Idempotent
amazon.aws.ec2_eip:
state: present
public_ip: "{{ eip.public_ip }}"
domain_name: "{{ test_domain }}"
tags: "{{ eip_test_tags }}"
register: eip

- name: Assert that task result was as expected
ansible.builtin.assert:
that:
- eip is not changed
- eip.public_ip is defined and ( eip.public_ip | ansible.utils.ipaddr )
- eip.allocation_id is defined and eip.allocation_id.startswith("eipalloc-")
- eip.update_reverse_dns_record_result is defined
- eip.update_reverse_dns_record_result.ptr_record == "{{ test_domain }}."

# ------------------------------------------------------------------------------------------
# Update reverse DNS record of existing EIP - remove reverse DNS record
# ------------------------------------------------------------------------------------------
- name: Try modifying reverse DNS record of EIP to different domain than current
amazon.aws.ec2_eip:
state: present
public_ip: "{{ eip.public_ip }}"
domain_name: ""
tags: "{{ eip_test_tags }}"
register: eip

- name: Assert that changes were applied
ansible.builtin.assert:
that:
- eip is changed
- eip.public_ip is defined and ( eip.public_ip | ansible.utils.ipaddr )
- eip.allocation_id is defined and eip.allocation_id.startswith("eipalloc-")

- name: Wait for reverse DNS record update to complete
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, update takes more than a minute to complete, if not completed and we move to next task then the next task fails saying another request already pending on EIP

pause:
minutes: 3

always:

- name: Delete EIP IP address an A record
amazon.aws.route53:
state: present
zone: "{{ test_hosted_zone }}"
record: "{{ test_domain }}"
type: A
ttl: 7200
value: "{{ eip.public_ip}}"
identifier: "{{ resource_prefix }}"
wait: true

- name: Delete EIP
ansible.builtin.include_tasks: tasks/common.yml
vars:
delete_eips: true
Loading
Loading