Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cloudflare - customizable pagination when listing DNS records #3364

Conversation

arturhoo
Copy link
Contributor

@arturhoo arturhoo commented Feb 3, 2023

Description

When listing DNS records in a given zone, by default, the cloudflare-go library fetches the results from Cloudflare's APIs in batches of size 50 - traversing the pages is done automatically when calling cloudflare.ListDNSRecordsParams with no pagination options.

However, users that have a large number of records in a zone, and with several deployments of external-dns fetching the records for the same zone, risk reaching Cloudflare's API limits.

Since version 0.58.0, the cloudflare-go library has a PerPage attribute on cloudflare.ListDNSRecordsParams which can help reduce the number of API calls made to Cloudflare and prevent rate limit events: https://api.cloudflare.com/#dns-records-for-a-zone-list-dns-records

This allows external-dns to fetch records in batch sizes of up to 5,000 (from the default 100), potentially reducing the number of API requests by a factor of 50.

In this PR, we allow users to tune the PerPage parameter. Since auto-pagination is not available when a custom PerPage value is passed to cloudflare.ListDNSRecordsParams (and the library is unlikely to do so in the future), we perform it ourselves.

An old, unused config option around pagination of zones, CloudflareZonesPerPage, is also removed - I believe it was supposed to be removed on #2298.

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 3, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 3, 2023
@arturhoo arturhoo marked this pull request as ready for review February 3, 2023 14:26
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 3, 2023
@arturhoo arturhoo force-pushed the cloudflare-paginated-list-requests branch from 588c66f to f1a966d Compare February 8, 2023 16:22
@arturhoo
Copy link
Contributor Author

@szuecs gentle ping :) - this is a follow-up to #3288 which we worked together.

Artur Rodrigues added 4 commits February 17, 2023 13:21
Signed-off-by: Artur Rodrigues <artur.rodrigues@lacework.net>
Signed-off-by: Artur Rodrigues <artur.rodrigues@lacework.net>
Signed-off-by: Artur Rodrigues <artur.rodrigues@lacework.net>
Signed-off-by: Artur Rodrigues <artur.rodrigues@lacework.net>
@arturhoo arturhoo force-pushed the cloudflare-paginated-list-requests branch from f1a966d to d8bf246 Compare February 17, 2023 13:22
Copy link
Contributor

@szuecs szuecs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to do a breaking change?
I think we can do better and enable people without breaking their deployments.

pkg/apis/externaldns/types.go Show resolved Hide resolved
@szuecs
Copy link
Contributor

szuecs commented Mar 2, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Mar 2, 2023
@szuecs
Copy link
Contributor

szuecs commented Mar 2, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arturhoo, szuecs

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 2, 2023
@szuecs
Copy link
Contributor

szuecs commented Mar 2, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2023
@k8s-ci-robot k8s-ci-robot merged commit b3a7698 into kubernetes-sigs:master Mar 2, 2023
@arturhoo arturhoo deleted the cloudflare-paginated-list-requests branch March 2, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants