From 0fae060dffd4d3fb1128180e195a12a115919ba0 Mon Sep 17 00:00:00 2001 From: Edouard Benauw Date: Fri, 20 Jun 2025 00:30:53 +0200 Subject: [PATCH] fix(cloudflare): improve handling of rate limiting errors (#5524) * fix cloudflare * factorize error conversion * only log if not a soft error * add comment for workaround * Remove debug print Co-authored-by: vflaux <38909103+vflaux@users.noreply.github.com> * Use assert Co-authored-by: vflaux <38909103+vflaux@users.noreply.github.com> * Improve error check Co-authored-by: vflaux <38909103+vflaux@users.noreply.github.com> * fix lint --------- Co-authored-by: vflaux <38909103+vflaux@users.noreply.github.com> --- provider/cloudflare/cloudflare.go | 47 +++++++++++++------------- provider/cloudflare/cloudflare_test.go | 13 +++++++ 2 files changed, 36 insertions(+), 24 deletions(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index af827daa1..d4e4f97ab 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -271,6 +271,23 @@ func getCreateDNSRecordParam(cfc cloudFlareChange) cloudflare.CreateDNSRecordPar } } +func convertCloudflareError(err error) error { + var apiErr *cloudflare.Error + if errors.As(err, &apiErr) { + if apiErr.ClientRateLimited() || apiErr.StatusCode >= http.StatusInternalServerError { + // Handle rate limit error as a soft error + return provider.NewSoftError(err) + } + } + // This is a workaround because Cloudflare library does not return a specific error type for rate limit exceeded. + // See https://github.com/cloudflare/cloudflare-go/issues/4155 and https://github.com/kubernetes-sigs/external-dns/pull/5524 + // This workaround can be removed once Cloudflare library returns a specific error type. + if strings.Contains(err.Error(), "exceeded available rate limit retries") { + return provider.NewSoftError(err) + } + return err +} + // NewCloudFlareProvider initializes a new CloudFlare DNS based Provider. func NewCloudFlareProvider(domainFilter endpoint.DomainFilter, zoneIDFilter provider.ZoneIDFilter, proxiedByDefault bool, dryRun bool, regionKey string, customHostnamesConfig CustomHostnamesConfig, dnsRecordsConfig DNSRecordsConfig) (*CloudFlareProvider, error) { // initialize via chosen auth method and returns new API object @@ -335,14 +352,7 @@ func (p *CloudFlareProvider) Zones(ctx context.Context) ([]cloudflare.Zone, erro zonesResponse, err := p.Client.ListZonesContext(ctx) if err != nil { - var apiErr *cloudflare.Error - if errors.As(err, &apiErr) { - if apiErr.ClientRateLimited() || apiErr.StatusCode >= http.StatusInternalServerError { - // Handle rate limit error as a soft error - return nil, provider.NewSoftError(err) - } - } - return nil, err + return nil, convertCloudflareError(err) } for _, zone := range zonesResponse.Result { @@ -753,14 +763,7 @@ func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Contex for { pageRecords, resultInfo, err := p.Client.ListDNSRecords(ctx, cloudflare.ZoneIdentifier(zoneID), params) if err != nil { - var apiErr *cloudflare.Error - if errors.As(err, &apiErr) { - if apiErr.ClientRateLimited() || apiErr.StatusCode >= http.StatusInternalServerError { - // Handle rate limit error as a soft error - return nil, provider.NewSoftError(err) - } - } - return nil, err + return nil, convertCloudflareError(err) } for _, r := range pageRecords { @@ -788,15 +791,11 @@ func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Conte for { pageCustomHostnameListResponse, result, err := p.Client.CustomHostnames(ctx, zoneID, resultInfo.Page, cloudflare.CustomHostname{}) if err != nil { - var apiErr *cloudflare.Error - if errors.As(err, &apiErr) { - if apiErr.ClientRateLimited() || apiErr.StatusCode >= http.StatusInternalServerError { - // Handle rate limit error as a soft error - return nil, provider.NewSoftError(err) - } + convertedError := convertCloudflareError(err) + if !errors.Is(convertedError, provider.SoftError) { + log.Errorf("zone %q failed to fetch custom hostnames. Please check if \"Cloudflare for SaaS\" is enabled and API key permissions, %v", zoneID, err) } - log.Errorf("zone %q failed to fetch custom hostnames. Please check if \"Cloudflare for SaaS\" is enabled and API key permissions, %v", zoneID, err) - return nil, err + return nil, convertedError } for _, ch := range pageCustomHostnameListResponse { chs[newCustomHostnameIndex(ch)] = ch diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index 97c20bb4d..dab08948e 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -955,6 +955,19 @@ func TestCloudflareListZonesRateLimited(t *testing.T) { } } +func TestCloudflareListZonesRateLimitedStringError(t *testing.T) { + // Create a mock client that returns a rate limit error + client := NewMockCloudFlareClient() + client.listZonesContextError = errors.New("exceeded available rate limit retries") + p := &CloudFlareProvider{Client: client} + + // Call the Zones function + _, err := p.Zones(context.Background()) + + // Assert that a soft error was returned + assert.ErrorIs(t, err, provider.SoftError, "expected a rate limit error") +} + func TestCloudflareListZoneInternalErrors(t *testing.T) { // Create a mock client that returns a internal server error client := NewMockCloudFlareClient()