Only update PiHole entries when they have actually changed

[sfleener]

Description

While testing the PiHole provider, I noticed some behavior that caused issues with internal hostnames being resolved.

Even when the DNS entries have not changed, the provider first deletes all entries it manages before re-adding them one by one with the same endpoint they originally had. With a moderate amount of entries managed by external-dns (~10-15 or so in my testing), this process takes about 10 seconds total. Using the default update interval of 60 seconds, this leaves a 5-10 second window every minute where these hostnames don't exist on PiHole and fail to resolve.

This PR adds a simple dedupe check that makes sure the entries have actually changed before deleting and re-adding them. This removes any time period where unchanged hostnames do not resolve, and only leaves an unresolvable window in cases where the target has actually changed (which is probably what you'd want anyway).

Considered Alternatives

• Adding a proper upsert API to PiHole itself. While doable, it would be a bit more work and would also only cover new versions of PiHole.
• Increase the default update interval for PiHole providers. Kicks the can down the road, not a real solution.
• Make delete and create requests concurrently instead of running them one-by-one. This would shorten the window where hostnames are not available, but the unresolvable window does still exist and we would be bursting a bunch of requests to pihole simultaneously. This could be combated with limiting the number of concurrent requests, but it seemed like too much effort and complexity to be really worth it especially considering it doesn't fully solve the original problem.

Checklist

• Unit tests updated
• End user documentation updated: N/A

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: sfleener / name: Scott Fleener (83d8804, 27eb7c9)

[k8s-ci-robot]

Welcome @sfleener!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[johngmyers]

What if changes.UpdateNew has multiple endpoints with the same DNSName? Shouldn't you put the RecordType in the map key?

[sfleener]

Good catch! PiHole doesn't allow two records of the same RecordType with the same DNS name, but it does allow two records with the same DNS name if the record types are different. Fixed

[johngmyers]

Better unit tests would explicitly specify the expected requests, verifying the dnsname, recordtype, and target.

[sfleener]

Updated the tests to compare the actual and expected requests instead of just the counts

[johngmyers]

The new version of the tests is asserting that the three new records are created in order, which isn't actually a requirement.

I would have written this to put the expected calls in the tracker. The client would then find the request in the tracker and remove it, asserting that the request was found in the tracker. At the end, the test would assert that there were no remaining expected calls in the tracker.

[johngmyers]

/lgtm
/assign @Raffo

[Raffo]

@sfleener can you fix the linter issues? I'm happy to approve after those are fixed

[sfleener]

@Raffo The lints appear to be gofmt errors in files I didn't touch, I sync'd with master which hopefully should resolve them

[Raffo]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Raffo, sfleener

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Raffo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Raffo]

/lgtm