From 968768d753e0d29c5a1fd2d553bc9d9cb673153c Mon Sep 17 00:00:00 2001 From: Andrew Hay <39sumer3939@gmail.com> Date: Wed, 2 Jul 2025 19:22:38 +0000 Subject: [PATCH 01/10] fix(cloudflare): remove unneeded deletion and recreation --- provider/cloudflare/cloudflare.go | 40 +++++++++------ provider/cloudflare/cloudflare_test.go | 69 +++++++++++++++++++++++++- 2 files changed, 92 insertions(+), 17 deletions(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index 574212da4..f412a50bf 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -603,26 +603,34 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud } recordID := p.getRecordID(records, change.ResourceRecord) if recordID == "" { - log.WithFields(logFields).Errorf("failed to find previous record: %v", change.ResourceRecord) - continue - } - recordParam := updateDNSRecordParam(*change) - recordParam.ID = recordID - err := p.Client.UpdateDNSRecord(ctx, resourceContainer, recordParam) - if err != nil { - failedChange = true - log.WithFields(logFields).Errorf("failed to update record: %v", err) + log.WithFields(logFields).Warnf("failed to find previous record for update, attempting to create instead: %v", change.ResourceRecord) + // Convert UPDATE to CREATE when record is not found + recordParam := getCreateDNSRecordParam(*change) + _, err := p.Client.CreateDNSRecord(ctx, resourceContainer, recordParam) + if err != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to create record after update failure: %v", err) + } + } else { + recordParam := updateDNSRecordParam(*change) + recordParam.ID = recordID + err := p.Client.UpdateDNSRecord(ctx, resourceContainer, recordParam) + if err != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to update record: %v", err) + } } } else if change.Action == cloudFlareDelete { recordID := p.getRecordID(records, change.ResourceRecord) if recordID == "" { - log.WithFields(logFields).Errorf("failed to find previous record: %v", change.ResourceRecord) - continue - } - err := p.Client.DeleteDNSRecord(ctx, resourceContainer, recordID) - if err != nil { - failedChange = true - log.WithFields(logFields).Errorf("failed to delete record: %v", err) + log.WithFields(logFields).Warnf("failed to find record for deletion, record may already be deleted: %v", change.ResourceRecord) + // Don't treat this as a failure - the record might already be deleted + } else { + err := p.Client.DeleteDNSRecord(ctx, resourceContainer, recordID) + if err != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to delete record: %v", err) + } } if !p.submitCustomHostnameChanges(ctx, zoneID, change, chs, logFields) { failedChange = true diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index b304d2116..331e4a7a6 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -1095,7 +1095,7 @@ func TestCloudflareApplyChanges(t *testing.T) { t.Errorf("should not fail, %s", err) } - td.Cmp(t, client.Actions, []MockAction{ + td.CmpDeeply(t, client.Actions, []MockAction{ { Name: "Create", ZoneId: "001", @@ -2760,3 +2760,70 @@ func TestCloudFlareProvider_SupportedAdditionalRecordTypes(t *testing.T) { }) } } + +func TestCloudflareApplyChanges_UpdateFallbackToCreate(t *testing.T) { + client := NewMockCloudFlareClientWithRecords(map[string][]cloudflare.DNSRecord{}) + provider := &CloudFlareProvider{ + Client: client, + domainFilter: endpoint.NewDomainFilter([]string{"bar.com"}), + DryRun: false, + } + + current := []*endpoint.Endpoint{ + endpoint.NewEndpointWithTTL("test.bar.com", endpoint.RecordTypeCNAME, endpoint.TTL(120), "example.com"), + } + desired := []*endpoint.Endpoint{ + endpoint.NewEndpointWithTTL("test.bar.com", endpoint.RecordTypeCNAME, endpoint.TTL(300), "example.com"), + } + + changes := &plan.Changes{ + UpdateOld: current, + UpdateNew: desired, + } + + // Apply the changes - this should fallback to CREATE since no records exist + err := provider.ApplyChanges(context.Background(), changes) + assert.NoError(t, err) + + // Verify that a CREATE operation was performed (not UPDATE) + // The mock client should have received a CreateDNSRecord call + records := client.Records["001"] + assert.Len(t, records, 1, "Should have created one record") + + // Get the first (and only) record from the map + var createdRecord cloudflare.DNSRecord + for _, record := range records { + createdRecord = record + break + } + + assert.Equal(t, "test.bar.com", createdRecord.Name) + assert.Equal(t, endpoint.RecordTypeCNAME, createdRecord.Type) + assert.Equal(t, "example.com", createdRecord.Content) + assert.Equal(t, 300, createdRecord.TTL) +} + +func TestCloudflareApplyChanges_DeleteNonExistentRecord(t *testing.T) { + client := NewMockCloudFlareClientWithRecords(map[string][]cloudflare.DNSRecord{}) + provider := &CloudFlareProvider{ + Client: client, + domainFilter: endpoint.NewDomainFilter([]string{"bar.com"}), + DryRun: false, + } + + toDelete := []*endpoint.Endpoint{ + endpoint.NewEndpointWithTTL("test.bar.com", endpoint.RecordTypeCNAME, endpoint.TTL(120), "example.com"), + } + + changes := &plan.Changes{ + Delete: toDelete, + } + + // Apply the changes - this should gracefully handle the missing record + err := provider.ApplyChanges(context.Background(), changes) + assert.NoError(t, err, "DELETE operation should not fail when record doesn't exist") + + // Verify no records exist after the operation + records := client.Records["001"] + assert.Len(t, records, 0, "Should have no records") +} From 80c2ed9801e4759c8c85672190ee392bd9a0c2a0 Mon Sep 17 00:00:00 2001 From: Andrew Hay <39sumer3939@gmail.com> Date: Wed, 2 Jul 2025 19:52:05 +0000 Subject: [PATCH 02/10] style(cloudflare): update for linter --- provider/cloudflare/cloudflare.go | 7 ++++--- provider/cloudflare/cloudflare_test.go | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index f412a50bf..ab40082ee 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -597,7 +597,8 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud if chErr != nil { return fmt.Errorf("could not fetch custom hostnames from zone, %w", chErr) } - if change.Action == cloudFlareUpdate { + switch change.Action { + case cloudFlareUpdate: if !p.submitCustomHostnameChanges(ctx, zoneID, change, chs, logFields) { failedChange = true } @@ -620,7 +621,7 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud log.WithFields(logFields).Errorf("failed to update record: %v", err) } } - } else if change.Action == cloudFlareDelete { + case cloudFlareDelete: recordID := p.getRecordID(records, change.ResourceRecord) if recordID == "" { log.WithFields(logFields).Warnf("failed to find record for deletion, record may already be deleted: %v", change.ResourceRecord) @@ -635,7 +636,7 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud if !p.submitCustomHostnameChanges(ctx, zoneID, change, chs, logFields) { failedChange = true } - } else if change.Action == cloudFlareCreate { + case cloudFlareCreate: recordParam := getCreateDNSRecordParam(*change) _, err := p.Client.CreateDNSRecord(ctx, resourceContainer, recordParam) if err != nil { diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index 331e4a7a6..815886847 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -2825,5 +2825,5 @@ func TestCloudflareApplyChanges_DeleteNonExistentRecord(t *testing.T) { // Verify no records exist after the operation records := client.Records["001"] - assert.Len(t, records, 0, "Should have no records") + assert.Empty(t, records, "Should have no records") } From 8a310248054ecb189101cea1202e05e94dea04a7 Mon Sep 17 00:00:00 2001 From: Andrew Hay <39sumer3939@gmail.com> Date: Wed, 2 Jul 2025 19:56:55 +0000 Subject: [PATCH 03/10] style: format tests --- provider/cloudflare/cloudflare_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index 815886847..430575570 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -2789,14 +2789,14 @@ func TestCloudflareApplyChanges_UpdateFallbackToCreate(t *testing.T) { // The mock client should have received a CreateDNSRecord call records := client.Records["001"] assert.Len(t, records, 1, "Should have created one record") - + // Get the first (and only) record from the map var createdRecord cloudflare.DNSRecord for _, record := range records { createdRecord = record break } - + assert.Equal(t, "test.bar.com", createdRecord.Name) assert.Equal(t, endpoint.RecordTypeCNAME, createdRecord.Type) assert.Equal(t, "example.com", createdRecord.Content) From 1cd946cec3672eee61a5fd8e294c82df462e30e7 Mon Sep 17 00:00:00 2001 From: Andrew Hay <39sumer3939@gmail.com> Date: Thu, 3 Jul 2025 17:29:00 +0000 Subject: [PATCH 04/10] docs(cloudflare): error handling --- charts/external-dns/CHANGELOG.md | 1 + docs/tutorials/cloudflare.md | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/charts/external-dns/CHANGELOG.md b/charts/external-dns/CHANGELOG.md index 16a129a9a..4d9cff786 100644 --- a/charts/external-dns/CHANGELOG.md +++ b/charts/external-dns/CHANGELOG.md @@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fixed the type of `.extraContainers` from `object` to `list` (array). ([#5564](https://github.com/kubernetes-sigs/external-dns/pull/5564)) _@svengreb_ +- Improved Cloudflare provider error handling with graceful fallback for update/delete operations. ([#5604](https://github.com/kubernetes-sigs/external-dns/pull/5604)) _@andrewcharleshay_ ## [v1.17.0] - 2025-06-04 diff --git a/docs/tutorials/cloudflare.md b/docs/tutorials/cloudflare.md index 1147e0e26..ffcf91332 100644 --- a/docs/tutorials/cloudflare.md +++ b/docs/tutorials/cloudflare.md @@ -355,6 +355,29 @@ Requires [Cloudflare for SaaS](https://developers.cloudflare.com/cloudflare-for- Due to a limitation within the cloudflare-go v0 API, the custom hostname page size is fixed at 50. +## Error Handling and Reliability + +The Cloudflare provider includes enhanced error handling to improve reliability in production environments: + +### Graceful Update Operations +When ExternalDNS attempts to update a DNS record that no longer exists (e.g., manually deleted), the provider will automatically fallback to creating the record instead of failing. This ensures your desired DNS state is maintained even when records get out of sync. + +``` +WARN: failed to find previous record for update, attempting to create instead: example.com +``` + +### Idempotent Delete Operations +Delete operations are idempotent - attempting to delete a record that doesn't exist will not result in an error. This prevents false-positive failures when records are deleted outside of ExternalDNS. + +``` +WARN: failed to find record for deletion, record may already be deleted: example.com +``` + +### Benefits +- **Reduced operational burden**: Fewer false-positive errors requiring manual intervention +- **Improved automation reliability**: Graceful handling of edge cases +- **Better observability**: Clear distinction between warnings (handled gracefully) and errors (requiring attention) + ## Using CRD source to manage DNS records in Cloudflare Please refer to the [CRD source documentation](../sources/crd.md#example) for more information. From 2cc36f13f818c2b310ae1e81cac5c54734b30e7c Mon Sep 17 00:00:00 2001 From: Andrew Hay <39sumer3939@gmail.com> Date: Fri, 4 Jul 2025 18:34:19 +0000 Subject: [PATCH 05/10] test(cloudflare): increase coverage on submit changes --- .gitignore | 1 + provider/cloudflare/cloudflare_test.go | 314 ++++++++++++++++++++++--- 2 files changed, 279 insertions(+), 36 deletions(-) diff --git a/.gitignore b/.gitignore index 29561b906..fcaaf2580 100644 --- a/.gitignore +++ b/.gitignore @@ -64,3 +64,4 @@ docs/redirect site _scratch Pipfile +*coverage.out diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index 430575570..e7bb9a799 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -52,15 +52,20 @@ type MockAction struct { } type mockCloudFlareClient struct { - Zones map[string]string - Records map[string]map[string]cloudflare.DNSRecord - Actions []MockAction - listZonesError error - zoneDetailsError error - listZonesContextError error - dnsRecordsError error - customHostnames map[string][]cloudflare.CustomHostname - regionalHostnames map[string][]cloudflare.RegionalHostname + Zones map[string]string + Records map[string]map[string]cloudflare.DNSRecord + Actions []MockAction + listZonesError error + zoneDetailsError error + listZonesContextError error + dnsRecordsError error + createError error + updateError error + deleteError error + customHostnamesError error + regionalHostnamesError error + customHostnames map[string][]cloudflare.CustomHostname + regionalHostnames map[string][]cloudflare.RegionalHostname } var ExampleDomain = []cloudflare.DNSRecord{ @@ -158,6 +163,10 @@ func generateDNSRecordID(rrtype string, name string, content string) string { } func (m *mockCloudFlareClient) CreateDNSRecord(ctx context.Context, rc *cloudflare.ResourceContainer, rp cloudflare.CreateDNSRecordParams) (cloudflare.DNSRecord, error) { + if m.createError != nil { + return cloudflare.DNSRecord{}, m.createError + } + recordData := getDNSRecordFromRecordParams(rp) if recordData.ID == "" { recordData.ID = generateDNSRecordID(recordData.Type, recordData.Name, recordData.Content) @@ -186,7 +195,7 @@ func (m *mockCloudFlareClient) ListDNSRecords(ctx context.Context, rc *cloudflar if zone, ok := m.Records[rc.Identifier]; ok { for _, record := range zone { if strings.HasPrefix(record.Name, "newerror-list-") { - m.DeleteDNSRecord(ctx, rc, record.ID) + _ = m.DeleteDNSRecord(ctx, rc, record.ID) return nil, &cloudflare.ResultInfo{}, errors.New("failed to list erroring DNS record") } result = append(result, record) @@ -224,6 +233,10 @@ func (m *mockCloudFlareClient) ListDNSRecords(ctx context.Context, rc *cloudflar } func (m *mockCloudFlareClient) UpdateDNSRecord(ctx context.Context, rc *cloudflare.ResourceContainer, rp cloudflare.UpdateDNSRecordParams) error { + if m.updateError != nil { + return m.updateError + } + recordData := getDNSRecordFromRecordParams(rp) m.Actions = append(m.Actions, MockAction{ Name: "Update", @@ -243,6 +256,10 @@ func (m *mockCloudFlareClient) UpdateDNSRecord(ctx context.Context, rc *cloudfla } func (m *mockCloudFlareClient) DeleteDNSRecord(ctx context.Context, rc *cloudflare.ResourceContainer, recordID string) error { + if m.deleteError != nil { + return m.deleteError + } + m.Actions = append(m.Actions, MockAction{ Name: "Delete", ZoneId: rc.Identifier, @@ -282,7 +299,7 @@ func (m *mockCloudFlareClient) CustomHostnames(ctx context.Context, zoneID strin for idx := (page - 1) * perPage; idx < min(len(chs), page*perPage); idx++ { ch := m.customHostnames[zoneID][idx] if strings.HasPrefix(ch.Hostname, "newerror-list-") { - m.DeleteCustomHostname(ctx, zoneID, ch.ID) + _ = m.DeleteCustomHostname(ctx, zoneID, ch.ID) return nil, cloudflare.ResultInfo{}, errors.New("failed to list erroring custom hostname") } result = append(result, ch) @@ -1736,7 +1753,7 @@ func TestCustomTTLWithEnabledProxyNotChanged(t *testing.T) { }, } - provider.AdjustEndpoints(endpoints) + endpoints, _ = provider.AdjustEndpoints(endpoints) domainFilter := endpoint.NewDomainFilter([]string{"bar.com"}) plan := &plan.Plan{ @@ -2592,6 +2609,7 @@ func TestZoneHasPaidPlan(t *testing.T) { } assert.False(t, cfproviderWithZoneError.ZoneHasPaidPlan("subdomain.foo.com")) } + func TestCloudflareApplyChanges_AllErrorLogPaths(t *testing.T) { hook := testutils.LogsUnderTestWithLogLevel(log.ErrorLevel, t) @@ -2716,22 +2734,24 @@ func TestCloudflareApplyChanges_AllErrorLogPaths(t *testing.T) { // Test with custom hostnames enabled and disabled for _, tc := range cases { - if tc.customHostnamesEnabled { - provider.CustomHostnamesConfig = CustomHostnamesConfig{Enabled: true} - } else { - provider.CustomHostnamesConfig = CustomHostnamesConfig{Enabled: false} - } - hook.Reset() - err := provider.ApplyChanges(context.Background(), tc.changes) - assert.NoError(t, err, "ApplyChanges should not return error for newCloudFlareChange error (it should log and continue)") - errorLogCount := 0 - for _, entry := range hook.Entries { - if entry.Level == log.ErrorLevel && - strings.Contains(entry.Message, "failed to create cloudflare change") { - errorLogCount++ + t.Run(tc.name, func(t *testing.T) { + if tc.customHostnamesEnabled { + provider.CustomHostnamesConfig = CustomHostnamesConfig{Enabled: true} + } else { + provider.CustomHostnamesConfig = CustomHostnamesConfig{Enabled: false} } - } - assert.Equal(t, tc.errorLogCount, errorLogCount, "expected error log count for %s", tc.name) + hook.Reset() + err := provider.ApplyChanges(context.Background(), tc.changes) + assert.NoError(t, err, "ApplyChanges should not return error for newCloudFlareChange error (it should log and continue)") + errorLogCount := 0 + for _, entry := range hook.Entries { + if entry.Level == log.ErrorLevel && + strings.Contains(entry.Message, "failed to create cloudflare change") { + errorLogCount++ + } + } + assert.Equal(t, tc.errorLogCount, errorLogCount, "expected error log count for %s", tc.name) + }) } } @@ -2769,16 +2789,13 @@ func TestCloudflareApplyChanges_UpdateFallbackToCreate(t *testing.T) { DryRun: false, } - current := []*endpoint.Endpoint{ - endpoint.NewEndpointWithTTL("test.bar.com", endpoint.RecordTypeCNAME, endpoint.TTL(120), "example.com"), - } - desired := []*endpoint.Endpoint{ - endpoint.NewEndpointWithTTL("test.bar.com", endpoint.RecordTypeCNAME, endpoint.TTL(300), "example.com"), - } - changes := &plan.Changes{ - UpdateOld: current, - UpdateNew: desired, + UpdateOld: []*endpoint.Endpoint{ + endpoint.NewEndpointWithTTL("test.bar.com", endpoint.RecordTypeCNAME, endpoint.TTL(120), "example.com"), + }, + UpdateNew: []*endpoint.Endpoint{ + endpoint.NewEndpointWithTTL("test.bar.com", endpoint.RecordTypeCNAME, endpoint.TTL(300), "example.com"), + }, } // Apply the changes - this should fallback to CREATE since no records exist @@ -2827,3 +2844,228 @@ func TestCloudflareApplyChanges_DeleteNonExistentRecord(t *testing.T) { records := client.Records["001"] assert.Empty(t, records, "Should have no records") } + +func TestCloudflareSubmitChanges_ErrorHandling(t *testing.T) { + tests := []struct { + name string + setupClient func() *mockCloudFlareClient + changes []*cloudFlareChange + expectError bool + errorMsg string + }{{ + name: "handles zones error", + setupClient: func() *mockCloudFlareClient { + client := NewMockCloudFlareClient() + client.listZonesContextError = errors.New("zones API error") + return client + }, + changes: []*cloudFlareChange{ + { + Action: cloudFlareCreate, + ResourceRecord: cloudflare.DNSRecord{ + Name: "test.bar.com", + Type: "A", + Content: "1.2.3.4", + TTL: 300, + }, + }, + }, + expectError: true, + errorMsg: "zones API error", + }, + { + name: "handles list DNS records error", + setupClient: func() *mockCloudFlareClient { + client := NewMockCloudFlareClient() + client.dnsRecordsError = errors.New("DNS records API error") + return client + }, + changes: []*cloudFlareChange{ + { + Action: cloudFlareCreate, + ResourceRecord: cloudflare.DNSRecord{ + Name: "test.bar.com", + Type: "A", + Content: "1.2.3.4", + TTL: 300, + }, + }, + }, + expectError: true, + errorMsg: "could not fetch records from zone", + }, + { + name: "handles create record error", + setupClient: func() *mockCloudFlareClient { + client := NewMockCloudFlareClient() + client.createError = errors.New("create record failed") + return client + }, + changes: []*cloudFlareChange{ + { + Action: cloudFlareCreate, + ResourceRecord: cloudflare.DNSRecord{ + Name: "test.bar.com", + Type: "A", + Content: "1.2.3.4", + TTL: 300, + }, + }, + }, + expectError: true, + errorMsg: "failed to submit all changes", + }, { + name: "handles update record error", + setupClient: func() *mockCloudFlareClient { + client := NewMockCloudFlareClient() + // Add existing record to zone 001 + client.Records["001"]["existing-record"] = cloudflare.DNSRecord{ + ID: "existing-record", + Name: "test.bar.com", + Type: "A", + Content: "1.2.3.4", + TTL: 300, + } + client.updateError = errors.New("update record failed") + return client + }, + changes: []*cloudFlareChange{ + { + Action: cloudFlareUpdate, + ResourceRecord: cloudflare.DNSRecord{ + Name: "test.bar.com", + Type: "A", + Content: "1.2.3.4", // Same content so getRecordID can find it + TTL: 600, // Different TTL to make it a meaningful update + }, + }, + }, + expectError: true, + errorMsg: "failed to submit all changes", + }, + { + name: "handles delete record error", + setupClient: func() *mockCloudFlareClient { + client := NewMockCloudFlareClient() + // Add existing record to zone 001 + client.Records["001"]["existing-record"] = cloudflare.DNSRecord{ + ID: "existing-record", + Name: "test.bar.com", + Type: "A", + Content: "1.2.3.4", + TTL: 300, + } + client.deleteError = errors.New("delete record failed") + return client + }, + changes: []*cloudFlareChange{ + { + Action: cloudFlareDelete, + ResourceRecord: cloudflare.DNSRecord{ + Name: "test.bar.com", + Type: "A", + Content: "1.2.3.4", // Same content so getRecordID can find it + TTL: 300, + }, + }, + }, + expectError: true, + errorMsg: "failed to submit all changes", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := tt.setupClient() + provider := &CloudFlareProvider{ + Client: client, + domainFilter: endpoint.NewDomainFilter([]string{"bar.com", "foo.com"}), + DryRun: false, + } + + err := provider.submitChanges(context.Background(), tt.changes) + + if tt.expectError { + assert.Error(t, err) + assert.Contains(t, err.Error(), tt.errorMsg) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestCloudflareSubmitChanges_EmptyChanges(t *testing.T) { + client := NewMockCloudFlareClient() + provider := &CloudFlareProvider{ + Client: client, + domainFilter: endpoint.NewDomainFilter([]string{"bar.com"}), + DryRun: false, + } + + err := provider.submitChanges(context.Background(), []*cloudFlareChange{}) + assert.NoError(t, err) +} + +func TestCloudflareSubmitChanges_DryRun(t *testing.T) { + client := NewMockCloudFlareClient() + provider := &CloudFlareProvider{ + Client: client, + domainFilter: endpoint.NewDomainFilter([]string{"bar.com"}), + DryRun: true, + } + + changes := []*cloudFlareChange{ + { + Action: cloudFlareCreate, + ResourceRecord: cloudflare.DNSRecord{ + Name: "test.bar.com", + Type: "A", + Content: "1.2.3.4", + TTL: 300, + }, + }, + } + + err := provider.submitChanges(context.Background(), changes) + assert.NoError(t, err) + + // Should not have made any API calls + assert.Empty(t, client.Actions) +} + +func TestCloudflareSubmitChanges_UpdateFallbackCreateFailure(t *testing.T) { + // This test specifically covers the "failed to create record after update failure" log line + client := NewMockCloudFlareClient() // No existing records to ensure getRecordID returns "" + client.createError = errors.New("create operation failed") + + provider := &CloudFlareProvider{ + Client: client, + domainFilter: endpoint.NewDomainFilter([]string{"bar.com"}), + DryRun: false, + } + + changes := []*cloudFlareChange{ + { + Action: cloudFlareUpdate, + ResourceRecord: cloudflare.DNSRecord{ + Name: "nonexistent.bar.com", + Type: "A", + Content: "1.2.3.4", + TTL: 300, + }, + }, + } + + // This should: + // 1. Try to update a record + // 2. Fail to find the record (getRecordID returns "") + // 3. Fall back to create + // 4. Fail the create operation + // 5. Log "failed to create record after update failure" + err := provider.submitChanges(context.Background(), changes) + + // Should return an error since the fallback create failed + assert.Error(t, err) + assert.Contains(t, err.Error(), "failed to submit all changes") +} From 7d1c0cc5159c895e179dea838b25e3a06f2d8c7f Mon Sep 17 00:00:00 2001 From: Andrew Hay <39sumer3939@gmail.com> Date: Fri, 4 Jul 2025 18:54:27 +0000 Subject: [PATCH 06/10] docs(cloudflare): MD022 --- docs/tutorials/cloudflare.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/tutorials/cloudflare.md b/docs/tutorials/cloudflare.md index ffcf91332..d5ebcd9ef 100644 --- a/docs/tutorials/cloudflare.md +++ b/docs/tutorials/cloudflare.md @@ -360,20 +360,23 @@ Due to a limitation within the cloudflare-go v0 API, the custom hostname page si The Cloudflare provider includes enhanced error handling to improve reliability in production environments: ### Graceful Update Operations + When ExternalDNS attempts to update a DNS record that no longer exists (e.g., manually deleted), the provider will automatically fallback to creating the record instead of failing. This ensures your desired DNS state is maintained even when records get out of sync. -``` +```text WARN: failed to find previous record for update, attempting to create instead: example.com ``` -### Idempotent Delete Operations +### Idempotent Delete Operations + Delete operations are idempotent - attempting to delete a record that doesn't exist will not result in an error. This prevents false-positive failures when records are deleted outside of ExternalDNS. -``` +```text WARN: failed to find record for deletion, record may already be deleted: example.com ``` ### Benefits + - **Reduced operational burden**: Fewer false-positive errors requiring manual intervention - **Improved automation reliability**: Graceful handling of edge cases - **Better observability**: Clear distinction between warnings (handled gracefully) and errors (requiring attention) From 25a071c6b0de1cb3e2db98fe5bf0ba3fc38dead5 Mon Sep 17 00:00:00 2001 From: Andrew Hay <39sumer3939@gmail.com> Date: Fri, 4 Jul 2025 19:24:47 +0000 Subject: [PATCH 07/10] style(cloudflare): remove trailing whitespace --- provider/cloudflare/cloudflare_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index e7bb9a799..177f4baf9 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -3038,7 +3038,7 @@ func TestCloudflareSubmitChanges_UpdateFallbackCreateFailure(t *testing.T) { // This test specifically covers the "failed to create record after update failure" log line client := NewMockCloudFlareClient() // No existing records to ensure getRecordID returns "" client.createError = errors.New("create operation failed") - + provider := &CloudFlareProvider{ Client: client, domainFilter: endpoint.NewDomainFilter([]string{"bar.com"}), @@ -3064,7 +3064,7 @@ func TestCloudflareSubmitChanges_UpdateFallbackCreateFailure(t *testing.T) { // 4. Fail the create operation // 5. Log "failed to create record after update failure" err := provider.submitChanges(context.Background(), changes) - + // Should return an error since the fallback create failed assert.Error(t, err) assert.Contains(t, err.Error(), "failed to submit all changes") From d9cabcf73d192dc0c3ca827ac05238b48f2631f2 Mon Sep 17 00:00:00 2001 From: Andrew Hay <39sumer3939@gmail.com> Date: Tue, 8 Jul 2025 02:09:48 +0000 Subject: [PATCH 08/10] docs: clarify txt records --- charts/external-dns/CHANGELOG.md | 2 -- docs/tutorials/cloudflare.md | 18 +++++++++--------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/charts/external-dns/CHANGELOG.md b/charts/external-dns/CHANGELOG.md index 4d9cff786..b5322b664 100644 --- a/charts/external-dns/CHANGELOG.md +++ b/charts/external-dns/CHANGELOG.md @@ -25,8 +25,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fixed the type of `.extraContainers` from `object` to `list` (array). ([#5564](https://github.com/kubernetes-sigs/external-dns/pull/5564)) _@svengreb_ -- Improved Cloudflare provider error handling with graceful fallback for update/delete operations. ([#5604](https://github.com/kubernetes-sigs/external-dns/pull/5604)) _@andrewcharleshay_ - ## [v1.17.0] - 2025-06-04 ### Changed diff --git a/docs/tutorials/cloudflare.md b/docs/tutorials/cloudflare.md index d5ebcd9ef..12dbeed24 100644 --- a/docs/tutorials/cloudflare.md +++ b/docs/tutorials/cloudflare.md @@ -357,29 +357,29 @@ Due to a limitation within the cloudflare-go v0 API, the custom hostname page si ## Error Handling and Reliability -The Cloudflare provider includes enhanced error handling to improve reliability in production environments: +The Cloudflare provider includes some error handling improvements to help with common operational scenarios: -### Graceful Update Operations +### Update Fallback Behavior -When ExternalDNS attempts to update a DNS record that no longer exists (e.g., manually deleted), the provider will automatically fallback to creating the record instead of failing. This ensures your desired DNS state is maintained even when records get out of sync. +When ExternalDNS attempts to update a DNS record that no longer exists (e.g., manually deleted), the provider may attempt to fallback to creating the record instead of failing. This behavior depends on both the DNS record and its corresponding TXT registry record state - if only the DNS record is deleted but the TXT record remains, ExternalDNS will recreate the record. Note that this behavior may change in future versions. ```text WARN: failed to find previous record for update, attempting to create instead: example.com ``` -### Idempotent Delete Operations +### Delete Operation Behavior -Delete operations are idempotent - attempting to delete a record that doesn't exist will not result in an error. This prevents false-positive failures when records are deleted outside of ExternalDNS. +Delete operations generally don't fail when attempting to delete a record that doesn't exist. This can help reduce false-positive failures when records are deleted outside of ExternalDNS, though the exact behavior may vary. ```text WARN: failed to find record for deletion, record may already be deleted: example.com ``` -### Benefits +### Considerations -- **Reduced operational burden**: Fewer false-positive errors requiring manual intervention -- **Improved automation reliability**: Graceful handling of edge cases -- **Better observability**: Clear distinction between warnings (handled gracefully) and errors (requiring attention) +- **Error handling behavior may change**: The specific error handling described above is current implementation behavior and may be modified in future versions +- **Best practices recommended**: While ExternalDNS handles some edge cases, following DNS management best practices and avoiding manual DNS changes is still recommended +- **Monitor logs for warnings**: Pay attention to warning messages which may indicate configuration or operational issues ## Using CRD source to manage DNS records in Cloudflare From ff4494e4b2ed10f144702abc1a0d116253c1e3b1 Mon Sep 17 00:00:00 2001 From: Andrew Hay <39sumer3939@gmail.com> Date: Tue, 8 Jul 2025 02:22:08 +0000 Subject: [PATCH 09/10] docs: fix markdown linting issues - Add blank line before heading in CHANGELOG.md (MD022) - Break long line in cloudflare.md (MD013) --- charts/external-dns/CHANGELOG.md | 1 + docs/tutorials/cloudflare.md | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/charts/external-dns/CHANGELOG.md b/charts/external-dns/CHANGELOG.md index b5322b664..16a129a9a 100644 --- a/charts/external-dns/CHANGELOG.md +++ b/charts/external-dns/CHANGELOG.md @@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fixed the type of `.extraContainers` from `object` to `list` (array). ([#5564](https://github.com/kubernetes-sigs/external-dns/pull/5564)) _@svengreb_ + ## [v1.17.0] - 2025-06-04 ### Changed diff --git a/docs/tutorials/cloudflare.md b/docs/tutorials/cloudflare.md index 12dbeed24..0dcb132d4 100644 --- a/docs/tutorials/cloudflare.md +++ b/docs/tutorials/cloudflare.md @@ -361,7 +361,8 @@ The Cloudflare provider includes some error handling improvements to help with c ### Update Fallback Behavior -When ExternalDNS attempts to update a DNS record that no longer exists (e.g., manually deleted), the provider may attempt to fallback to creating the record instead of failing. This behavior depends on both the DNS record and its corresponding TXT registry record state - if only the DNS record is deleted but the TXT record remains, ExternalDNS will recreate the record. Note that this behavior may change in future versions. +When ExternalDNS attempts to update a DNS record that no longer exists (e.g., manually deleted), the provider may attempt to fallback to creating the record instead of failing. This behavior depends on both the DNS record and its corresponding +TXT registry record state - if only the DNS record is deleted but the TXT record remains, ExternalDNS will recreate the record. Note that this behavior may change in future versions. ```text WARN: failed to find previous record for update, attempting to create instead: example.com From fa502c2071b9cc9fc7da4009c5a154f667e67892 Mon Sep 17 00:00:00 2001 From: Andrew Hay <39sumer3939@gmail.com> Date: Tue, 8 Jul 2025 02:33:41 +0000 Subject: [PATCH 10/10] style: md009 --- docs/tutorials/cloudflare.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/tutorials/cloudflare.md b/docs/tutorials/cloudflare.md index 0dcb132d4..6646ee7ec 100644 --- a/docs/tutorials/cloudflare.md +++ b/docs/tutorials/cloudflare.md @@ -361,7 +361,7 @@ The Cloudflare provider includes some error handling improvements to help with c ### Update Fallback Behavior -When ExternalDNS attempts to update a DNS record that no longer exists (e.g., manually deleted), the provider may attempt to fallback to creating the record instead of failing. This behavior depends on both the DNS record and its corresponding +When ExternalDNS attempts to update a DNS record that no longer exists (e.g., manually deleted), the provider may attempt to fallback to creating the record instead of failing. This behavior depends on both the DNS record and its corresponding TXT registry record state - if only the DNS record is deleted but the TXT record remains, ExternalDNS will recreate the record. Note that this behavior may change in future versions. ```text