Skip to content

Commit

Permalink
Fix route53_info max_items / type being ignored (#813)
Browse files Browse the repository at this point in the history
Fix route53_info max_items / type being ignored

SUMMARY
Currently if max_items is set on the route53_info module then it is ignored meaning all items are returned.
type is also ignored due to an incorrect if statement
It looks like it was a regression introduced here:
ansible/ansible@6075536#diff-23a0c9250633162d50c3f06442b7a552a5ae0659a24dd01a328c0e165e473616
The tests have been updated to reflect a check on max_items and type
Fixes: #529
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
route53_info
ADDITIONAL INFORMATION
The problem with max_items being ignored is resolved by adding PaginationConfig and adding MaxItems to that instead.
The problem with type being ignored is resolved by fixing an if statement.
Boto3 docs: https://boto3.amazonaws.com/v1/documentation/api/1.18.7/reference/services/route53.html#Route53.Paginator.ListResourceRecordSets

Reviewed-by: Mark Chappell <None>
Reviewed-by: Markus Bergholz <[email protected]>
Reviewed-by: Alina Buzachis <None>
Reviewed-by: None <None>
  • Loading branch information
marknet15 authored Nov 30, 2021
1 parent 2b5683c commit e323f83
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 21 deletions.
2 changes: 2 additions & 0 deletions changelogs/fragments/813-route53_info-fix-max_items.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
minor_changes:
- route53_info - `max_items` and `type` are no longer ignored fixing a regression (https://github.com/ansible-collections/community.aws/pull/813).
49 changes: 34 additions & 15 deletions plugins/modules/route53_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
description:
- Maximum number of items to return for various get/list requests.
required: false
type: str
type: int
next_marker:
description:
- "Some requests such as list_command: hosted_zones will return a maximum
Expand All @@ -72,7 +72,7 @@
description:
- The type of DNS record.
required: false
choices: [ 'A', 'CNAME', 'MX', 'AAAA', 'TXT', 'PTR', 'SRV', 'SPF', 'CAA', 'NS' ]
choices: [ 'A', 'CNAME', 'MX', 'AAAA', 'TXT', 'PTR', 'SRV', 'SPF', 'CAA', 'NS', 'NAPTR', 'SOA', 'DS' ]
type: str
dns_name:
description:
Expand Down Expand Up @@ -228,9 +228,13 @@ def get_hosted_zone(client, module):

def reusable_delegation_set_details(client, module):
params = dict()

if not module.params.get('delegation_set_id'):
# Set PaginationConfig with max_items
if module.params.get('max_items'):
params['MaxItems'] = module.params.get('max_items')
params['PaginationConfig'] = dict(
MaxItems=module.params.get('max_items')
)

if module.params.get('next_marker'):
params['Marker'] = module.params.get('next_marker')
Expand All @@ -246,8 +250,11 @@ def reusable_delegation_set_details(client, module):
def list_hosted_zones(client, module):
params = dict()

# Set PaginationConfig with max_items
if module.params.get('max_items'):
params['MaxItems'] = module.params.get('max_items')
params['PaginationConfig'] = dict(
MaxItems=module.params.get('max_items')
)

if module.params.get('next_marker'):
params['Marker'] = module.params.get('next_marker')
Expand All @@ -272,8 +279,11 @@ def list_hosted_zones_by_name(client, module):
if module.params.get('dns_name'):
params['DNSName'] = module.params.get('dns_name')

# Set PaginationConfig with max_items
if module.params.get('max_items'):
params['MaxItems'] = module.params.get('max_items')
params['PaginationConfig'] = dict(
MaxItems=module.params.get('max_items')
)

return client.list_hosted_zones_by_name(**params)

Expand Down Expand Up @@ -340,12 +350,15 @@ def get_resource_tags(client, module):
def list_health_checks(client, module):
params = dict()

if module.params.get('max_items'):
params['MaxItems'] = module.params.get('max_items')

if module.params.get('next_marker'):
params['Marker'] = module.params.get('next_marker')

# Set PaginationConfig with max_items
if module.params.get('max_items'):
params['PaginationConfig'] = dict(
MaxItems=module.params.get('max_items')
)

paginator = client.get_paginator('list_health_checks')
health_checks = paginator.paginate(**params).build_full_result()['HealthChecks']
return {
Expand All @@ -362,19 +375,25 @@ def record_sets_details(client, module):
else:
module.fail_json(msg="Hosted Zone Id is required")

if module.params.get('max_items'):
params['MaxItems'] = module.params.get('max_items')

if module.params.get('start_record_name'):
params['StartRecordName'] = module.params.get('start_record_name')

# Check that both params are set if type is applied
if module.params.get('type') and not module.params.get('start_record_name'):
module.fail_json(msg="start_record_name must be specified if type is set")
elif module.params.get('type'):

if module.params.get('type'):
params['StartRecordType'] = module.params.get('type')

# Set PaginationConfig with max_items
if module.params.get('max_items'):
params['PaginationConfig'] = dict(
MaxItems=module.params.get('max_items')
)

paginator = client.get_paginator('list_resource_record_sets')
record_sets = paginator.paginate(**params).build_full_result()['ResourceRecordSets']

return {
"ResourceRecordSets": record_sets,
"list": record_sets,
Expand Down Expand Up @@ -420,12 +439,12 @@ def main():
], required=True),
change_id=dict(),
hosted_zone_id=dict(),
max_items=dict(),
max_items=dict(type='int'),
next_marker=dict(),
delegation_set_id=dict(),
start_record_name=dict(),
type=dict(choices=[
'A', 'CNAME', 'MX', 'AAAA', 'TXT', 'PTR', 'SRV', 'SPF', 'CAA', 'NS'
type=dict(type='str', choices=[
'A', 'CNAME', 'MX', 'AAAA', 'TXT', 'PTR', 'SRV', 'SPF', 'CAA', 'NS', 'NAPTR', 'SOA', 'DS'
]),
dns_name=dict(),
resource_id=dict(type='list', aliases=['resource_ids'], elements='str'),
Expand Down
56 changes: 50 additions & 6 deletions tests/integration/targets/route53/tasks/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@
record_set: '{{ get_result.set }}'
qdn_record: 'qdn_test.{{ zone_one }}'

## test A recordset creation and order adjustments
- name: 'Create same A record using zone non-qualified domain'
route53:
state: present
Expand Down Expand Up @@ -220,17 +221,19 @@
- mv_a_record is not failed
- mv_a_record is not changed

# Get resulting A record and ensure max_items is applied
- name: 'get Route53 A record information'
route53_info:
type: A
query: record_sets
hosted_zone_id: '{{ z1.zone_id }}'
start_record_name: 'order_test.{{ zone_one }}'
max_items: 50
max_items: 1
register: records

- assert:
that:
- records.ResourceRecordSets|length == 3
- records.ResourceRecordSets|length == 1
- records.ResourceRecordSets[0].ResourceRecords|length == 2
- records.ResourceRecordSets[0].ResourceRecords[0].Value == '192.0.2.2'
- records.ResourceRecordSets[0].ResourceRecords[1].Value == '192.0.2.1'
Expand Down Expand Up @@ -261,6 +264,7 @@
- '192.0.2.2'
register: del_a_record
ignore_errors: true

- name: 'This should not fail, because `overwrite` is true'
assert:
that:
Expand All @@ -275,12 +279,44 @@
start_record_name: 'order_test.{{ zone_one }}'
max_items: 50
register: records

- assert:
that:
- records.ResourceRecordSets|length == 3
- records.ResourceRecordSets[0].ResourceRecords|length == 1
- records.ResourceRecordSets[0].ResourceRecords[0].Value == '192.0.2.2'

## Test CNAME record creation and retrive info
- name: "Create CNAME record"
route53:
state: present
zone: "{{ zone_one }}"
type: CNAME
record: "cname_test.{{ zone_one }}"
value: "order_test.{{ zone_one }}"
register: cname_record

- assert:
that:
- cname_record is not failed
- cname_record is changed

- name: "Get Route53 CNAME record information"
route53_info:
type: CNAME
query: record_sets
hosted_zone_id: "{{ z1.zone_id }}"
start_record_name: "cname_test.{{ zone_one }}"
max_items: 1
register: cname_records

- assert:
that:
- cname_records.ResourceRecordSets|length == 1
- cname_records.ResourceRecordSets[0].ResourceRecords|length == 1
- cname_records.ResourceRecordSets[0].ResourceRecords[0].Value == "order_test.{{ zone_one }}"

## Test CAA record creation
- name: 'Create a LetsEncrypt CAA record'
route53:
state: present
Expand Down Expand Up @@ -389,7 +425,7 @@
- wc_a_record.diff.after == {}

- name: create a record with different TTL
community.aws.route53:
route53:
state: present
zone: '{{ zone_one }}'
record: 'localhost.{{ zone_one }}'
Expand All @@ -404,7 +440,7 @@
- ttl30 is changed

- name: delete previous record without mention ttl and value
community.aws.route53:
route53:
state: absent
zone: '{{ zone_one }}'
record: 'localhost.{{ zone_one }}'
Expand All @@ -416,7 +452,7 @@
- ttl30 is changed

- name: immutable delete previous record without mention ttl and value
community.aws.route53:
route53:
state: absent
zone: '{{ zone_one }}'
record: 'localhost.{{ zone_one }}'
Expand Down Expand Up @@ -604,6 +640,7 @@
query: record_sets
hosted_zone_id: '{{ z1.zone_id }}'
register: z1_records

- name: 'Loop over A/AAAA/CNAME Alias records and delete them'
route53:
state: absent
Expand All @@ -617,20 +654,23 @@
loop: '{{ z1_records.ResourceRecordSets | selectattr("Type", "in", ["A", "AAAA", "CNAME", "CAA"]) | list }}'
when:
- '"AliasTarget" in item'

- name: 'Loop over A/AAAA/CNAME records and delete them'
route53:
state: absent
zone: '{{ zone_one }}'
record: '{{ item.Name }}'
type: '{{ item.Type }}'
value: '{{ item.ResourceRecords | map(attribute="Value") | join(",") }}'
weight: '{{ item.Weight | default(omit) }}'
identifier: '{{ item.SetIdentifier }}'
region: '{{ omit }}'
ignore_errors: True
loop: '{{ z1_records.ResourceRecordSets | selectattr("Type", "in", ["A", "AAAA", "CNAME", "CAA"]) | list }}'
when:
- '"ResourceRecords" in item'
- '"SetIdentifier" in item'

- name: 'Loop over A/AAAA/CNAME records and delete them'
route53:
state: absent
Expand All @@ -647,6 +687,7 @@
query: record_sets
hosted_zone_id: '{{ z2.zone_id }}'
register: z2_records

- name: 'Loop over A/AAAA/CNAME Alias records and delete them'
route53:
state: absent
Expand All @@ -661,6 +702,7 @@
loop: '{{ z2_records.ResourceRecordSets | selectattr("Type", "in", ["A", "AAAA", "CNAME", "CAA"]) | list }}'
when:
- '"AliasTarget" in item'

- name: 'Loop over A/AAAA/CNAME records and delete them'
route53:
state: absent
Expand All @@ -676,6 +718,7 @@
when:
- '"ResourceRecords" in item'
- '"SetIdentifier" in item'

- name: 'Loop over A/AAAA/CNAME records and delete them'
route53:
state: absent
Expand All @@ -697,6 +740,7 @@
ignore_errors: yes
retries: 10
until: delete_one is not failed

- name: 'Delete test zone two {{ zone_two }}'
route53_zone:
state: absent
Expand All @@ -705,7 +749,7 @@
ignore_errors: yes
retries: 10
until: delete_two is not failed
when: false

- name: destroy VPC
ec2_vpc_net:
cidr_block: 192.0.2.0/24
Expand Down

0 comments on commit e323f83

Please sign in to comment.