Add provider cache

[tjamet]

Description

In the current implementation, DNS providers are called to list all records on every loop. This is expensive in terms of number of requests to the provider and may result in being rate limited, as reported in 1293 and 3397.

In our case, we have approximately 20,000 records in our AWS Hosted Zone. The ListResourceRecordSets API call allows a maximum of 300 items per call. That requires 67 API calls per external-dns deployment during every sync period

With this, we introduce an optional generic caching mechanism at the provider level, that re-uses the latest known list of records for a given time.

This prevents from expensive Provider calls to list all records for each object modification that does not change the actual record (annotations, statuses, ingress routing, ...)

This introduces 2 trade-offs:

1. Any changes or corruption directly on the provider side will be longer to detect and to resolve, up to the cache time

2. Any conflicting records in the DNS provider (such as a different external-dns instance) injected during the cache validity will cause the first iteration of the next reconcile loop to fail, and hence add a delay until the next retry

Checklist

• Unit tests updated
• End user documentation updated

reopen #3402

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: tjamet / name: Thibault Jamet (089744c, eb07eb9, 2955e5d, 82c6983, 29191e2, ef0dd29, b2ff161, b2d678f, 9b759f0, 8d2f1c0, 1736048, a6ab2ba, 309f34f, 6c5faaf)

[k8s-ci-robot]

Hi @tjamet. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[mloiseleur]

/ok-to-test

[mloiseleur]

This PR is really great, with test, documentation and metrics.
Many thanks for that.
For the documentation, Wdyt of adding a new page about this topic in advanced topic section ?
/lgtm
/assign @Raffo

[tjamet]

This PR is really great, with test, documentation and metrics. Many thanks for that. For the documentation, Wdyt of adding a new page about this topic in advanced topic section ? /lgtm /assign @Raffo

Happy to do so.

I will take a look if I can do it in the coming days

[tjamet]

This PR is really great, with test, documentation and metrics. Many thanks for that. For the documentation, Wdyt of adding a new page about this topic in advanced topic section ? /lgtm /assign @Raffo

Hi!

I have added a page dealing with those API rate limits concerns.
let me know what you think, if there is anything we need to do to improve it.

[mloiseleur]

@tjamet I have left some suggestion, mainly formatting. Many thanks, it's quite concise and clear.

@Raffo Do you want to review it before merge ?

[tjamet]

@tjamet We are near the merge for this one. Would you please rebase it ?

Sure, I have just rebased it!

[mloiseleur]

/lgtm

[tjamet]

/lgtm

Apologies, I didn't see the LGTM and applied a suggestion. I guess this will require a new lgtm

[Raffo]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Raffo

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

[kashalls]

Could someone explain why we changed the type here?

Edit: Meant to comment on the provider type here.

[tjamet]

At the time of implementing, there was a conflict with some providers.
It seems that this is not the case any longer and that this change was kept over rebases.
I tried reverting b2ff1619 and it seems to work after some minor fixes.

What problems do you encounter because of this change?

[kashalls]

What problems do you encounter because of this change?

It introduced a breaking change for some of the webhook providers as we used the DomainFilter type to simply Marshal the filter list for negotiation. We now just take the filter json and marshal it ourselves which is no big issue. It's just a loss of functionality. (writing this at 2am, might have a better way to explain it later)

Reference

[oferz-everc]

@tjamet @Raffo
Just checking if I'm missing something - shouldn't the deployment.yaml needs to be updated with the new flag of "ProviderCacheTime"? cause without it, how can the chart would be able to use it?
I'm trying to upgrade to latest version, but when adding the value for "ProviderCacheTime" there's no diff in the deployment.

[tjamet]

Adding a specific named Values entry would indeed be an option.
Alternatively, you can use extraArgs already supported in the chart

extraArgs:
- --min-event-sync-interval=5m
- --provider-cache-time=60m
- --aws-zones-cache-duration=10m

Those will be provided to the deployment