octodns / octodns-cloudflare

Cloudflare DNS provider for octoDNS
MIT License
25 stars 18 forks source link

Cloudflare provider assumes a zone does not exist when it has no records #3

Closed LukasdeBoer closed 2 years ago

LukasdeBoer commented 3 years ago

We ran into an issue when using the Cloudflare provider for a new, existing zone with no DNS records. It will try to create the zone, even though it already exists:

********************************************************************************
* domain.com.
********************************************************************************
* cloudflare (CloudflareProvider)
*   Create Zone<domain.com.>
*   Create <CnameRecord CNAME 1, record.domain.com., record.domain2.com.> (config)
*   Summary: Creates=1, Updates=0, Deletes=0, Existing Records=0
********************************************************************************

After creating a single test record test.domain.com manually via the Cloudflare interface, this is the result:

********************************************************************************
* domain.com.
********************************************************************************
* cloudflare (CloudflareProvider)
*   Create <CnameRecord CNAME 1, record.domain.com., record.domain2.com.> (config)
*   Delete <ARecord A 1, test.domain.com., ['127.0.0.1']>
*   Summary: Creates=1, Updates=0, Deletes=1, Existing Records=1
********************************************************************************

As you can see it no longer tries to create the zone. I suspect it has something to do with the code around this line: https://github.com/octodns/octodns/blob/master/octodns/provider/cloudflare.py#L338.

ross commented 3 years ago

As you can see it no longer tries to create the zone. I suspect it has something to do with the code around this line: https://github.com/octodns/octodns/blob/master/octodns/provider/cloudflare.py#L338.

Yeah looks like it's short-cutting and using no records to indicate that the zone doesn't exist which as you point out isn't a safe assumption.

https://github.com/octodns/octodns/blob/81b68de097c34b2c6aa7dcdc59ec396a91e63a21/octodns/provider/cloudflare.py#L289-L290

maybe needs to return None or throw a (caught) exception when the zone doesn't yet exist. Either way https://github.com/octodns/octodns/blob/master/octodns/provider/cloudflare.py#L336-L337 will need a bit of a tweak.

It looks like the other calls to that property should be safe:

https://github.com/octodns/octodns/blob/81b68de097c34b2c6aa7dcdc59ec396a91e63a21/octodns/provider/cloudflare.py#L557 - in an update case so the zone has to exist to get there

https://github.com/octodns/octodns/blob/81b68de097c34b2c6aa7dcdc59ec396a91e63a21/octodns/provider/cloudflare.py#L663 - same situation, this time in a delete.

PR welcome :grin:

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.