cloudflare_dns: Fix setting SRV records with a root level entry #5972

rlenferink · 2023-02-10T15:44:08Z

SUMMARY

This fixes a bug in the cloudflare_dns module where setting a root-level SRV record resulted in a 400 Bad Request. After some debugging apparently the following line removes the zone suffix from the record:

community.general/plugins/modules/cloudflare_dns.py

Line 690 in de193ac

"name": params['record'][:-len('.' + params['zone'])],

Say, input data params['record'] = example.com and params['zone'] = example.com then name will be empty, which is not accepted by the Cloudflare API. Since the intention was to set a root-level entry, this PR ensures that in that specific case name will be equals to the zone.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

cloudflare_dns

ADDITIONAL INFORMATION

ansibullbot · 2023-02-10T15:49:12Z

cc @mgruener
click here for bot help

rlenferink · 2023-02-10T16:51:32Z

I just noticed the following related issue: #5167

/cc @felixfontein

The suggested solution in that issue is incorrect because that breaks the ability to set SRV records for subdomains. This PR works for me in a idempotent way.

felixfontein

Thanks for the contribution. Could you please also update the description for the name option to mention the special case for SRV records?

plugins/modules/cloudflare_dns.py

rlenferink · 2023-02-22T10:03:31Z

Thanks for the contribution. Could you please also update the description for the name option to mention the special case for SRV records?

I started adding a note to the record field, but after a bit more of digging through the code I started wondering if that is even needed/wanted. I think this is just a technical detail in the module itself.

In the start of the module a check is done for @ and if that is the case, it will already replace the value with zone:

community.general/plugins/modules/cloudflare_dns.py

Lines 411 to 412 in 867aee6

    
           if self.record == '@': 
        
               self.record = self.zone

I am willing to add this (if wanted), but I am wondeirng if this 'technical detail' is interesting, since apparently @ is already substituted with zone internally (only incorrectly for SRV records)

felixfontein · 2023-02-22T22:05:21Z

If nobody objects, I'll merge this this weekend.

…ecord name The cloudflare API accepts the record name + zone name to be sent. Removing that, will guarantee the module to be idempotent even though that line was added ~7 years ago for that specific reason: ansible/ansible-modules-extras@7477fe5 It seems the most logical explanition is that Cloudflare changed their API response somewhere over the last 7 years.

felixfontein · 2023-02-24T10:29:37Z

If nobody objects, I'll merge this by the end of this weekend.

rlenferink · 2023-02-24T10:42:57Z

If nobody objects, I'll merge this by the end of this weekend.

Sounds good to me.

patchback · 2023-02-26T13:03:34Z

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/094dc6b69c6ca4f54a261581962cdbf2956c0233/pr-5972

Backported as #6096

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

* cloudflare_dns: Fix setting SRV records with a root level entry * cloudflare_dns: Remove the part which deletes the zone from the SRV record name The cloudflare API accepts the record name + zone name to be sent. Removing that, will guarantee the module to be idempotent even though that line was added ~7 years ago for that specific reason: ansible/ansible-modules-extras@7477fe5 It seems the most logical explanition is that Cloudflare changed their API response somewhere over the last 7 years. * cloudflare_dns: Update the changelog fragment (cherry picked from commit 094dc6b)

patchback · 2023-02-26T13:03:44Z

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/094dc6b69c6ca4f54a261581962cdbf2956c0233/pr-5972

Backported as #6097

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

felixfontein · 2023-02-26T13:03:45Z

@rlenferink thanks a lot for fixing this!

* cloudflare_dns: Fix setting SRV records with a root level entry * cloudflare_dns: Remove the part which deletes the zone from the SRV record name The cloudflare API accepts the record name + zone name to be sent. Removing that, will guarantee the module to be idempotent even though that line was added ~7 years ago for that specific reason: ansible/ansible-modules-extras@7477fe5 It seems the most logical explanition is that Cloudflare changed their API response somewhere over the last 7 years. * cloudflare_dns: Update the changelog fragment (cherry picked from commit 094dc6b)

…V records with a root level entry (#6096) cloudflare_dns: Fix setting SRV records with a root level entry (#5972) * cloudflare_dns: Fix setting SRV records with a root level entry * cloudflare_dns: Remove the part which deletes the zone from the SRV record name The cloudflare API accepts the record name + zone name to be sent. Removing that, will guarantee the module to be idempotent even though that line was added ~7 years ago for that specific reason: ansible/ansible-modules-extras@7477fe5 It seems the most logical explanition is that Cloudflare changed their API response somewhere over the last 7 years. * cloudflare_dns: Update the changelog fragment (cherry picked from commit 094dc6b) Co-authored-by: Roy Lenferink <lenferinkroy@gmail.com>

…V records with a root level entry (#6097) cloudflare_dns: Fix setting SRV records with a root level entry (#5972) * cloudflare_dns: Fix setting SRV records with a root level entry * cloudflare_dns: Remove the part which deletes the zone from the SRV record name The cloudflare API accepts the record name + zone name to be sent. Removing that, will guarantee the module to be idempotent even though that line was added ~7 years ago for that specific reason: ansible/ansible-modules-extras@7477fe5 It seems the most logical explanition is that Cloudflare changed their API response somewhere over the last 7 years. * cloudflare_dns: Update the changelog fragment (cherry picked from commit 094dc6b) Co-authored-by: Roy Lenferink <lenferinkroy@gmail.com>

rlenferink force-pushed the fix-cloudflare-srv-record branch from 234aa56 to cf9e24d Compare February 10, 2023 15:46

ansibullbot added bug This issue/PR relates to a bug module module net_tools plugins plugin (any type) labels Feb 10, 2023

felixfontein reviewed Feb 11, 2023

View reviewed changes

plugins/modules/cloudflare_dns.py Outdated Show resolved Hide resolved

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 labels Feb 11, 2023

ansibullbot added stale_ci CI is older than 7 days, rerun before merging and removed stale_ci CI is older than 7 days, rerun before merging labels Feb 19, 2023

cloudflare_dns: Fix setting SRV records with a root level entry

d7742ea

rlenferink force-pushed the fix-cloudflare-srv-record branch from 2b0a35b to d7742ea Compare February 22, 2023 10:18

rlenferink added 2 commits February 24, 2023 11:15

cloudflare_dns: Update the changelog fragment

5502762

This comment was marked as outdated.

Sign in to view

felixfontein merged commit 094dc6b into ansible-collections:main Feb 26, 2023

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Feb 26, 2023

patchback bot mentioned this pull request Feb 26, 2023

[PR #5972/094dc6b6 backport][stable-5] cloudflare_dns: Fix setting SRV records with a root level entry #6096

Merged

patchback bot mentioned this pull request Feb 26, 2023

[PR #5972/094dc6b6 backport][stable-6] cloudflare_dns: Fix setting SRV records with a root level entry #6097

Merged

felixfontein mentioned this pull request Feb 26, 2023

cloudflare_dns: unable to add SRV records using example from docs #5167

Closed

1 task

rlenferink deleted the fix-cloudflare-srv-record branch February 26, 2023 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cloudflare_dns: Fix setting SRV records with a root level entry #5972

cloudflare_dns: Fix setting SRV records with a root level entry #5972

rlenferink commented Feb 10, 2023

ansibullbot commented Feb 10, 2023

rlenferink commented Feb 10, 2023

felixfontein left a comment

rlenferink commented Feb 22, 2023 •

edited

Loading

felixfontein commented Feb 22, 2023

This comment was marked as outdated.

felixfontein commented Feb 24, 2023

rlenferink commented Feb 24, 2023

patchback bot commented Feb 26, 2023 •

edited

Loading

patchback bot commented Feb 26, 2023 •

edited

Loading

felixfontein commented Feb 26, 2023

cloudflare_dns: Fix setting SRV records with a root level entry #5972

cloudflare_dns: Fix setting SRV records with a root level entry #5972

Conversation

rlenferink commented Feb 10, 2023

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Feb 10, 2023

rlenferink commented Feb 10, 2023

felixfontein left a comment

Choose a reason for hiding this comment

rlenferink commented Feb 22, 2023 • edited Loading

felixfontein commented Feb 22, 2023

This comment was marked as outdated.

felixfontein commented Feb 24, 2023

rlenferink commented Feb 24, 2023

patchback bot commented Feb 26, 2023 • edited Loading

Backport to stable-5: 💚 backport PR created

patchback bot commented Feb 26, 2023 • edited Loading

Backport to stable-6: 💚 backport PR created

felixfontein commented Feb 26, 2023

rlenferink commented Feb 22, 2023 •

edited

Loading

patchback bot commented Feb 26, 2023 •

edited

Loading

patchback bot commented Feb 26, 2023 •

edited

Loading