octodns / octodns-cloudflare

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

Cloudflare processor #78

Closed mhosker closed 7 months ago

mhosker commented 9 months ago

Related to octodns/octodns#1130

A processor to allow Cloudflare proxied records to be used on other providers without exposing the proxied record value.

An example of this when using Cloudflare & NS1 -

    "********************************************************************************",
    "* example.com.",
    "********************************************************************************",
    "* cloudflare (CloudflareProvider)",
    "*   Create <ARecord A 300, test.example.com., ['1.2.3.4'], {'cloudflare': {'proxied': True}}> (zones)",
    "*   Summary: Creates=1, Updates=0, Deletes=0, Existing Records=14",
    "* ns1 (Ns1Provider)",
    "*   Create <CnameRecord CNAME 300, test.example.com., test.example.com.cdn.cloudflare.net.> ()",
    "*   Summary: Creates=1, Updates=0, Deletes=0, Existing Records=12",
    "********************************************************************************"

By using the .cdn.cloudflare.net subdomain we are able to point proxied records back to Cloudflare on non Cloudflare providers.

This allows for all the benefits of active split authority on non Cloudflare proxied records while still being able to retain the Cloudflare CDN / WAF benefits for specific proxied records.

ross commented 9 months ago

As of https://github.com/octodns/octodns-cloudflare/pull/82 there's a seperate processors file/directory. This should get moved into there rather than be in the main init

mhosker commented 9 months ago

One possible problem I have noticed during recent testing is the ability to have a non Cloudflare proxyable record type ending up coexisting with a CNAME. e.g -

test:
- octodns:
    cloudflare:
      proxied: true
  ttl: 300
  type: A
  value: 1.2.3.4
- octodns:
    cloudflare:
      auto-ttl: true
  ttl: 300
  type: TXT
  value: this is a test

The above will output -

    "********************************************************************************",
    "* example.com.",
    "********************************************************************************",
    "* cloudflare (CloudflareProvider)",
    "*   Create <ARecord A 300, test.example.com., ['1.2.3.4'], {'cloudflare': {'proxied': True}}> (zones)",
    "*   Create <TxtRecord TXT 300, test.example.com., ['this is a test'], {'cloudflare': {'auto-ttl': True}}> (zones)",
    "*   Summary: Creates=2, Updates=0, Deletes=0, Existing Records=14",
    "* ns1 (Ns1Provider)",
    "*   Create <CnameRecord CNAME 300, test.example.com., test.example.com.cdn.cloudflare.net.> ()",
    "*   Create <TxtRecord TXT 300, test.example.com., ['this is a test'], {'cloudflare': {'auto-ttl': True}}> (zones)",
    "*   Summary: Creates=2, Updates=0, Deletes=0, Existing Records=12",
    "********************************************************************************"

Which is technically perfectly correct, however goes against the RFC.

In my case using Cloudflare and NS1 I am lucky and NS1 seems to be able to magically handle it, I assume by using CNAME flattening.

Other providers though will likely be less forgiving and produce an error about the other records coexisting with the CNAME.

The only real "fix" I can think of is for records with non Cloudflare proxyable types to look at all other records with the same name and check if any of them are CF proxied. If they are, then skip. I'm not sure if that is possible though, and if so, how.

If it were possible it would mean in the above example only the CNAME would be created and the TXT skipped, for both providers, to avoid asymmetric zones.

Either way I think its a bit of an edge case - I don't have any real world use cases in any zones I manage.

ross commented 7 months ago

Failures now are lack of test coverage.

mhosker commented 7 months ago

Written a first version of the tests. Problem I am running into is records destined to the Cloudflare provider. The very first thing my processor does is -

        # Check if zone is destined for Cloudflare
        if isinstance(target, CloudflareProvider):
            # If it is then dont bother with any processing just return now
            return desired, existing

Which should basically skip running any of the processor logic and throw back the original untouched records if destined for CF.

This works fine outside of the tests.

As part of the tests I am passing the CloudflareProvider, initially by importing from octodns_cloudflare import CloudflareProvider and then...

            processor.process_source_and_target_zones(
                zone, zone_empty, CloudflareProvider
            )

However, not working...

I suspect I am not passing the CloudflareProvider object correctly to the process_source_and_target_zones() function.

Need to do some further troubleshooting, but if anything stands out - let me know. I suspect its a simple thing...!

ross commented 7 months ago

processor.process_source_and_target_zones( zone, zone_empty, CloudflareProvider )

That looks like it's probably the class object and not a CloudflareProvider instance. You'll probably want something like:

cloudflare_provider = CloudflareProvider('test', 'email', 'token', retry_period=0)
...
processor.process_source_and_target_zones(
                 zone, zone_empty, cloudflare_provider
)

and then an instance of another provider, maybe YamlProvider, to test the other case.

mhosker commented 7 months ago

Thanks for that!

Indeed, of course... I was just passing the class and not actually a provider object with "config"!

Tests now all passing, welcome feedback.

:)