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

cloudflare_dns: fix crash when deleting a DNS record or when updating a record with solo=true #9649

Merged
merged 4 commits into from
Jan 31, 2025

Conversation

valievkarim
Copy link
Contributor

SUMMARY

On 2025-01-27, Cloudflare removed the zone_id field from the DNS record API responses. This caused a KeyError in the delete_dns_records method, which previously relied on rr['zone_id'].

This commit ensures the zone ID is retrieved via _get_zone_id() rather than using the no-longer-provided zone_id field in the record response.

Reference: https://developers.cloudflare.com/dns/changelog/#2025-01-27

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

cloudflare_dns

ADDITIONAL INFORMATION

before fix:

cfdnstest.yaml

- name: test cloudflare_dns
  hosts: localhost
  gather_facts: no
  vars:
    cloudflare_key_path: "cloudflare-token.txt"
    dns_name: cfrecordtest
    proxied_dns_zone: scopeful.io
    dest_1: "2a01:4f9:6b:4721::1"
    dest_2: "2a01:4f9:6b:4721::2"
  tasks:
    - name: "Create proxied AAAA record {{ dns_name }}.{{ proxied_dns_zone }} => {{ dest_1 }}"
      cloudflare_dns:
        zone: "{{ proxied_dns_zone }}"
        record: "{{ dns_name }}"
        type: AAAA
        solo: true
        proxied: true
        value: "{{ dest_1 }}"
        api_token: "{{ lookup('file', cloudflare_key_path) }}"

    - name: "Update proxied AAAA record {{ dns_name }}.{{ proxied_dns_zone }} => {{ dest_2 }}"
      cloudflare_dns:
        zone: "{{ proxied_dns_zone }}"
        record: "{{ dns_name }}"
        type: AAAA
        solo: true
        proxied: true
        value: "{{ dest_2 }}"
        api_token: "{{ lookup('file', cloudflare_key_path) }}"

    - name: "Delete AAAA record {{ dns_name }}.{{ proxied_dns_zone }}"
      cloudflare_dns:
        zone: "{{ proxied_dns_zone }}"
        record: "{{ dns_name }}"
        type: AAAA
        state: absent
        api_token: "{{ lookup('file', cloudflare_key_path) }}"
$ ansible-playbook cfdnstest.yaml
[WARNING]: No inventory was parsed, only implicit localhost is available
[WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match 'all'

PLAY [test cloudflare_dns] *******************************************************************************************************************************************************

TASK [Create proxied AAAA record cfrecordtest.scopeful.io => 2a01:4f9:6b:4721::1] ************************************************************************************************
changed: [localhost]

TASK [Update proxied AAAA record cfrecordtest.scopeful.io => 2a01:4f9:6b:4721::2] ************************************************************************************************
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: KeyError: 'zone_id'
fatal: [localhost]: FAILED! => changed=false
  module_stderr: |-
    Traceback (most recent call last):
      File "/Users/karim/.ansible/tmp/ansible-tmp-1738197314.785573-15327-100923117162027/AnsiballZ_cloudflare_dns.py", line 107, in <module>
        _ansiballz_main()
      File "/Users/karim/.ansible/tmp/ansible-tmp-1738197314.785573-15327-100923117162027/AnsiballZ_cloudflare_dns.py", line 99, in _ansiballz_main
        invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
      File "/Users/karim/.ansible/tmp/ansible-tmp-1738197314.785573-15327-100923117162027/AnsiballZ_cloudflare_dns.py", line 47, in invoke_module
        runpy.run_module(mod_name='ansible_collections.community.general.plugins.modules.cloudflare_dns', init_globals=dict(_module_fqn='ansible_collections.community.general.plugins.modules.cloudflare_dns', _modlib_path=modlib_path),
      File "<frozen runpy>", line 226, in run_module
      File "<frozen runpy>", line 98, in _run_module_code
      File "<frozen runpy>", line 88, in _run_code
      File "/var/folders/tr/2fynb8ld26g1_kjxwsyxxmzm0000gn/T/ansible_cloudflare_dns_payload_ywbxuqcw/ansible_cloudflare_dns_payload.zip/ansible_collections/community/general/plugins/modules/cloudflare_dns.py", line 1000, in <module>
      File "/var/folders/tr/2fynb8ld26g1_kjxwsyxxmzm0000gn/T/ansible_cloudflare_dns_payload_ywbxuqcw/ansible_cloudflare_dns_payload.zip/ansible_collections/community/general/plugins/modules/cloudflare_dns.py", line 987, in main
      File "/var/folders/tr/2fynb8ld26g1_kjxwsyxxmzm0000gn/T/ansible_cloudflare_dns_payload_ywbxuqcw/ansible_cloudflare_dns_payload.zip/ansible_collections/community/general/plugins/modules/cloudflare_dns.py", line 695, in delete_dns_records
    KeyError: 'zone_id'
  module_stdout: ''
  msg: |-
    MODULE FAILURE: No start of json char found
    See stdout/stderr for the exact error
  rc: 1

PLAY RECAP ***********************************************************************************************************************************************************************
localhost                  : ok=1    changed=1    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0

after fix:

$ ansible-playbook cfdnstest.yaml
[WARNING]: No inventory was parsed, only implicit localhost is available
[WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match 'all'

PLAY [test cloudflare_dns] *******************************************************************************************************************************************************

TASK [Create proxied AAAA record cfrecordtest.scopeful.io => 2a01:4f9:6b:4721::1] ************************************************************************************************
ok: [localhost]

TASK [Update proxied AAAA record cfrecordtest.scopeful.io => 2a01:4f9:6b:4721::2] ************************************************************************************************
changed: [localhost]

TASK [Delete AAAA record cfrecordtest.scopeful.io] *******************************************************************************************************************************
changed: [localhost]

PLAY RECAP ***********************************************************************************************************************************************************************
localhost                  : ok=3    changed=2    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

… a record with solo=true

On 2025-01-27, Cloudflare removed the 'zone_id' field from the DNS record API responses. This caused a KeyError in the delete_dns_records method, which previously relied on rr['zone_id'].

This commit ensures the zone ID is retrieved via _get_zone_id() rather than using the no-longer-provided 'zone_id' field in the record response.

Reference: https://developers.cloudflare.com/dns/changelog/#2025-01-27
@valievkarim valievkarim changed the title cloudflare_dns: fix crash when deleting a DNS record or when updating a record with solo=true [WIP] cloudflare_dns: fix crash when deleting a DNS record or when updating a record with solo=true Jan 30, 2025
@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added WIP Work in progress bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review traceback labels Jan 30, 2025
@ansibullbot ansibullbot removed the small_patch Hopefully easy to review label Jan 30, 2025
@valievkarim valievkarim changed the title [WIP] cloudflare_dns: fix crash when deleting a DNS record or when updating a record with solo=true cloudflare_dns: fix crash when deleting a DNS record or when updating a record with solo=true Jan 30, 2025
@ansibullbot ansibullbot removed the WIP Work in progress label Jan 30, 2025
@valievkarim
Copy link
Contributor Author

ready_for_review

@felixfontein felixfontein added backport-9 Automatically create a backport for the stable-9 branch backport-10 Automatically create a backport for the stable-10 branch labels Jan 30, 2025
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I've added a first comment below.

@valievkarim
Copy link
Contributor Author

@felixfontein i've accepted your change, thank you for review!

@valievkarim
Copy link
Contributor Author

this fixes issue #9652

@felixfontein
Copy link
Collaborator

@ibot3 would you mind testing this PR?

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

LGTM from the Ansible perspective, cannot vouch for how it behaves for realsies with Cloudflare

@ibot3
Copy link

ibot3 commented Jan 31, 2025

@ibot3 would you mind testing this PR?

Works for me 👍

@felixfontein felixfontein merged commit 19d0049 into ansible-collections:main Jan 31, 2025
138 checks passed
Copy link

patchback bot commented Jan 31, 2025

Backport to stable-9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-9/19d0049698822de628b536361e27b4bf0032b840/pr-9649

Backported as #9654

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jan 31, 2025
… a record with solo=true (#9649)

* cloudflare_dns: fix crash when deleting a DNS record or when updating a record with solo=true

On 2025-01-27, Cloudflare removed the 'zone_id' field from the DNS record API responses. This caused a KeyError in the delete_dns_records method, which previously relied on rr['zone_id'].

This commit ensures the zone ID is retrieved via _get_zone_id() rather than using the no-longer-provided 'zone_id' field in the record response.

Reference: https://developers.cloudflare.com/dns/changelog/#2025-01-27

* Add changelog fragment

* Update changelogs/fragments/9649-cloudflare_dns-fix-crash-when-deleting-record.yml

Co-authored-by: Felix Fontein <[email protected]>

* Update changelogs/fragments/9649-cloudflare_dns-fix-crash-when-deleting-record.yml

Co-authored-by: Felix Fontein <[email protected]>

---------

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 19d0049)
Copy link

patchback bot commented Jan 31, 2025

Backport to stable-10: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-10/19d0049698822de628b536361e27b4bf0032b840/pr-9649

Backported as #9655

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jan 31, 2025
… a record with solo=true (#9649)

* cloudflare_dns: fix crash when deleting a DNS record or when updating a record with solo=true

On 2025-01-27, Cloudflare removed the 'zone_id' field from the DNS record API responses. This caused a KeyError in the delete_dns_records method, which previously relied on rr['zone_id'].

This commit ensures the zone ID is retrieved via _get_zone_id() rather than using the no-longer-provided 'zone_id' field in the record response.

Reference: https://developers.cloudflare.com/dns/changelog/#2025-01-27

* Add changelog fragment

* Update changelogs/fragments/9649-cloudflare_dns-fix-crash-when-deleting-record.yml

Co-authored-by: Felix Fontein <[email protected]>

* Update changelogs/fragments/9649-cloudflare_dns-fix-crash-when-deleting-record.yml

Co-authored-by: Felix Fontein <[email protected]>

---------

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 19d0049)
@felixfontein
Copy link
Collaborator

@valievkarim thanks for fixing this!
@russoz thanks for reviewing!
@ibot3 thanks for testing!

felixfontein pushed a commit that referenced this pull request Jan 31, 2025
… deleting a DNS record or when updating a record with solo=true (#9654)

cloudflare_dns: fix crash when deleting a DNS record or when updating a record with solo=true (#9649)

* cloudflare_dns: fix crash when deleting a DNS record or when updating a record with solo=true

On 2025-01-27, Cloudflare removed the 'zone_id' field from the DNS record API responses. This caused a KeyError in the delete_dns_records method, which previously relied on rr['zone_id'].

This commit ensures the zone ID is retrieved via _get_zone_id() rather than using the no-longer-provided 'zone_id' field in the record response.

Reference: https://developers.cloudflare.com/dns/changelog/#2025-01-27

* Add changelog fragment

* Update changelogs/fragments/9649-cloudflare_dns-fix-crash-when-deleting-record.yml

Co-authored-by: Felix Fontein <[email protected]>

* Update changelogs/fragments/9649-cloudflare_dns-fix-crash-when-deleting-record.yml

Co-authored-by: Felix Fontein <[email protected]>

---------

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 19d0049)

Co-authored-by: valievkarim <[email protected]>
felixfontein pushed a commit that referenced this pull request Jan 31, 2025
…n deleting a DNS record or when updating a record with solo=true (#9655)

cloudflare_dns: fix crash when deleting a DNS record or when updating a record with solo=true (#9649)

* cloudflare_dns: fix crash when deleting a DNS record or when updating a record with solo=true

On 2025-01-27, Cloudflare removed the 'zone_id' field from the DNS record API responses. This caused a KeyError in the delete_dns_records method, which previously relied on rr['zone_id'].

This commit ensures the zone ID is retrieved via _get_zone_id() rather than using the no-longer-provided 'zone_id' field in the record response.

Reference: https://developers.cloudflare.com/dns/changelog/#2025-01-27

* Add changelog fragment

* Update changelogs/fragments/9649-cloudflare_dns-fix-crash-when-deleting-record.yml

Co-authored-by: Felix Fontein <[email protected]>

* Update changelogs/fragments/9649-cloudflare_dns-fix-crash-when-deleting-record.yml

Co-authored-by: Felix Fontein <[email protected]>

---------

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 19d0049)

Co-authored-by: valievkarim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-9 Automatically create a backport for the stable-9 branch backport-10 Automatically create a backport for the stable-10 branch bug This issue/PR relates to a bug has_issue module module new_contributor Help guide this first time contributor plugins plugin (any type) traceback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants