octodns / octodns-cloudflare

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

feat: add ignore_proxied config support #14

Closed mrbarletta closed 2 years ago

mrbarletta commented 2 years ago

Adding this change will stop the proxied setting to be forcefully set for any _PROXIABLE_RECORD_TYPES record.

Currently, if the source DNS doesn't support proxied setting, instead of ignoring the proxied flag, it will always be set to false overriding a change in the Cloudflare's DNS interface.

IMHO, if the proxied flag is not set for a given record, it should not be set to false but ignored.

mrbarletta commented 2 years ago

IMHO, if the proxied flag is not set for a given record, it should not be set to false but ignored.

I believe that would remove octoDNS's ability to manage proxied since it could turn it on, but not off. The most common use case is managing Cloudflare records with yaml config in a git repo in which case people likely want the proxied state to match what's in the config. Using another provider. (e.g. PowerDNS) as a source is supported, but as far as I know much less common.

Ross, I don't see why is that the case, maybe its too late for me but the proxied: false would turn off, proxied: true will turn on, and no proxied setting for the record should (this is what I suggest) be ignored instead of setting the record as its set to proxied: false.

mrbarletta commented 2 years ago

@ross anything I can do to have this merged? thanks

ross commented 2 years ago

This doesn't appear to work in my testing. The check of ignore_proxied only appears to happen during populate and it does correctly avoid setting cloudflare.proxied: true in _record_for, but extra_changes is still seeing the proxied difference and adding in a change for the record which then wants to update things:

I created a proxied record with:

  cloudflare:
    class: octodns_cloudflare.CloudflareProvider
    token: env/CLOUDFLARE_TOKEN
    strict_supports: false
'': 
  - octodns:
      cloudflare:
        proxied: true
    type: A
    value: 52.147.213.50
Screen Shot 2022-05-30 at 1 59 52 PM

And then added ignore_proxied: true to the provider config and removed that bit from the record so that it was unconfigured (as if it had come from powerdns)

  cloudflare:
    class: octodns_cloudflare.CloudflareProvider
    token: env/CLOUDFLARE_TOKEN
    strict_supports: false
    ignore_proxied: true
'': 
  - type: A
    value: 52.147.213.50

When I plan it still sees the proxied difference:

...
2022-05-30T14:03:31  [4649917952] INFO  CloudflareProvider[cloudflare] plan:   extra changes
  Update
    <ARecord A 300, xormedia.com., ['52.147.213.50']> ->
    <ARecord A 3600, xormedia.com., ['52.147.213.50']> (config)
...

********************************************************************************
* xormedia.com.
********************************************************************************
* cloudflare (CloudflareProvider)
*   Update
*     <ARecord A 300, xormedia.com., ['52.147.213.50']> ->
*     <ARecord A 3600, xormedia.com., ['52.147.213.50']> (config)
*   Summary: Creates=0, Updates=1, Deletes=0, Existing Records=2
********************************************************************************

The lines that add it are https://github.com/octodns/octodns-cloudflare/blob/0a6d3e82ab1904aca549196a5a477f8bd74482c7/octodns_cloudflare/__init__.py#L828-L830

Beyond that tests do pass, but there's no test coverage of the additions made in the PR which would be required to move forward (once the functionality is working as desired.)