From 40acd5b287932d6bb562748cbcd98520ac77d97d Mon Sep 17 00:00:00 2001 From: Mark Woolley Date: Tue, 30 Nov 2021 18:09:50 +0000 Subject: [PATCH] Fix route53_info max_items / type being ignored (#813) 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 Reviewed-by: Markus Bergholz Reviewed-by: Alina Buzachis Reviewed-by: None This commit was initially merged in https://github.com/ansible-collections/community.aws See: https://github.com/ansible-collections/community.aws/commit/e323f838086127acbd524eec55d5c967a65c8196 --- plugins/modules/route53_info.py | 49 +++++++++++----- .../targets/route53/tasks/main.yml | 56 +++++++++++++++++-- 2 files changed, 84 insertions(+), 21 deletions(-) diff --git a/plugins/modules/route53_info.py b/plugins/modules/route53_info.py index abdf7e44709..322ce7b0523 100644 --- a/plugins/modules/route53_info.py +++ b/plugins/modules/route53_info.py @@ -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 @@ -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: @@ -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') @@ -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') @@ -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) @@ -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 { @@ -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, @@ -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'), diff --git a/tests/integration/targets/route53/tasks/main.yml b/tests/integration/targets/route53/tasks/main.yml index f827387d83d..92f8601e64a 100644 --- a/tests/integration/targets/route53/tasks/main.yml +++ b/tests/integration/targets/route53/tasks/main.yml @@ -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 @@ -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' @@ -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: @@ -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 @@ -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 }}' @@ -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 }}' @@ -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 }}' @@ -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 @@ -617,6 +654,7 @@ 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 @@ -624,6 +662,7 @@ 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 @@ -631,6 +670,7 @@ when: - '"ResourceRecords" in item' - '"SetIdentifier" in item' + - name: 'Loop over A/AAAA/CNAME records and delete them' route53: state: absent @@ -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 @@ -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 @@ -676,6 +718,7 @@ when: - '"ResourceRecords" in item' - '"SetIdentifier" in item' + - name: 'Loop over A/AAAA/CNAME records and delete them' route53: state: absent @@ -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 @@ -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