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

aws - Added new action and a bugfix for existing action for route53 #9291

Merged
merged 53 commits into from Mar 4, 2024

Conversation

kk1532
Copy link
Contributor

@kk1532 kk1532 commented Feb 11, 2024

Added new action called remove-recordset for deleting route53 resource record sets. This action has been created for the resource rrset.

Sample Policy

              - name: route53-remove-filtered-records
                resource: aws.rrset
                filters:
                  - type: value
                    key: AliasTarget.DNSName
                    value: "email.gc.example.com."
                actions:
                  - type: remove-recordset

Also added a bug-fix for an existing action 'delete' for the resource hostedzone. Existing delete action code covers only CNAME and A records (Name to IP) of route 53. This bug-fix would cover Alias A records (Name to Service FQDN) as well, as Alias A records of route 53 will not have TTL and ResourceRecords attribute in payload.

This bug-fix would fix below errors.

"errorMessage": "'TTL'",
"errorType": "KeyError",

and

botocore.errorfactory.HostedZoneNotEmpty: An error occurred (HostedZoneNotEmpty) when calling the DeleteHostedZone operation: The specified hosted zone contains non-required resource record sets and so cannot be deleted.[ERROR]

we would get HostedZoneNotEmpty error for the hostedzone with Alias A record deletion.

kk1532 and others added 30 commits April 5, 2022 06:17
mu - changes on LambdaRetry max_attempts
Also rename/shorten the ignore parameter for readability.
@kk1532 kk1532 requested a review from kapilt as a code owner February 11, 2024 19:01
c7n/resources/route53.py Outdated Show resolved Hide resolved
@StevenGunn
Copy link
Contributor

@kapilt thanks for checking this out, we are hoping to get this functionality included in the next release. Could you take a look at @kk1532's recent updates and re-review when you get a chance?

c7n/resources/route53.py Outdated Show resolved Hide resolved
@@ -267,24 +330,27 @@ def delete_records(self, client, hz):
# Exempt the two zone associated mandatory records
if rrset['Name'] == hz['Name'] and rrset['Type'] in ('NS', 'SOA'):
continue
rrsetdata = self.generate_rrset(rrset)
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems like since we now have a delete record set action, we could remove that implementation from here and just call the delete action on the record set, composition style to avoid the duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kapilt
Can you check and confirm, are below the expected change?

on @HostedZone.action_registry.register('delete')
class Delete(BaseAction):

 def process(self, hosted_zones):
        client = local_session(self.manager.session_factory).client('route53')
        rrsetdelete = ResourceRecordSetRemove(self.data, self.manager, self.log_dir)
        error = None
        for hz in hosted_zones:
            if self.data.get('force'):
                rrsetdelete.process("", hz)

and remove the method def delete_records(self, client, hz):

add equivalent entry on record set delete action like below

  def process(self, recordsets, hz=None):
        client = local_session(self.manager.session_factory).client('route53')
        if hz is not None:
            paginator = client.get_paginator('list_resource_record_sets')
            paginator.PAGE_ITERATOR_CLS = RetryPageIterator
            recordsets = paginator.paginate(HostedZoneId=hz['Id']).build_full_result()

        try:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this composition style, we can avoid two delete implementation for record deletion.

@StevenGunn
Copy link
Contributor

hey @kapilt, circling back on this one, can you please approve/merge if all looks good?

Copy link
Collaborator

@kapilt kapilt left a comment

Choose a reason for hiding this comment

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

lgtm

@kapilt kapilt merged commit 944b183 into cloud-custodian:main Mar 4, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants