mholt / caddy-dynamicdns

Caddy app that keeps your DNS records (A/AAAA) pointed at itself.
Apache License 2.0
250 stars 25 forks source link

Correctly detect current Cloudflare IPs #50

Closed pkoenig10 closed 1 year ago

pkoenig10 commented 1 year ago

When using the Cloudflare DNS provider, the current IPs are not detected correctly.

found DNS record    {"type": "A", "name": "", "zone": "example.com", "value": "12.34.56.78"}
found DNS record    {"type": "AAAA", "name": "", "zone": "example.com", "value": "2001:db8:3333:4444:5555:6666:7777:8888"}
domain not found in DNS {"domain": "example.com"}
domain not found in DNS {"domain": "example.com"}
looked up current IPs from DNS  {"lastIPs": {"example.com":{"A":[""],"AAAA":[""]}}}

The name in the Caddy config is @, but the name in the returned Record is the empty string. So we insert into recMap using .example.com, but lookup using example.com.

The libdns documentation isn't clear on the expected behavior of Record.Name for A records. If the empty string is a valid value for A records, then this PR is the correct fix. But if the value for A records should always be @, then this should be fixed in libdns/cloudflare.

I assume this is the correct fix since it matches similar code in libdns.

mholt commented 1 year ago

Ah, thanks for this. I haven't given the empty string / self case much consideration yet. Some DNS providers expect/require a @ whereas others use empty string. We should probably figure out which convention is best for libdns to use and go with that.

Do you have a preference/opinion/thoughts on whether @ or "" should be used?

francislavoie commented 1 year ago

Maybe we should call libdns.AbsoluteName() in here and then TrimSuffix the ., I dunno.

pkoenig10 commented 1 year ago

I don't have a strong opinion. If it was up to me, I'd probably just use the empty string, for couple reasons:

Whatever we decide, the value in the Caddy config should probably match the behavior of libdns. So if we change to use the empty string, this has the potential to break existing configurations which use @.

It also feels reasonable to me to just accept both values, like libdns.AbsoluteName.

pkoenig10 commented 1 year ago

Maybe we should call libdns.AbsoluteName() in here and then TrimSuffix the .

This sounds reasonable to me.

mholt commented 1 year ago

Awesome. Thank you both!!