From 8d37820a564cc7a1dea2eff893963d3a6185bbde Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Fri, 14 Mar 2025 22:52:03 -0700 Subject: [PATCH 01/16] fix(cloudflare): custom hostnames edge-cases causing duplicates --- provider/cloudflare/cloudflare.go | 70 +++++++------ provider/cloudflare/cloudflare_test.go | 138 +++++++++++++++++++++---- 2 files changed, 153 insertions(+), 55 deletions(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index 5df8277db..5b63caaba 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -406,7 +406,7 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud prevCh := change.CustomHostnamePrev newCh := change.CustomHostname.Hostname if prevCh != "" { - prevChID, _ := p.getCustomHostnameOrigin(chs, prevCh) + prevChID, _ := getCustomHostnameIdWithOrigin(chs, prevCh) if prevChID != "" && prevCh != newCh { log.WithFields(logFields).Infof("Removing previous custom hostname %v/%v", prevChID, prevCh) chErr := p.Client.DeleteCustomHostname(ctx, zoneID, prevChID) @@ -457,19 +457,18 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud failedChange = true log.WithFields(logFields).Errorf("failed to delete record: %v", err) } - if change.CustomHostname.Hostname == "" { - continue - } - log.WithFields(logFields).Infof("Deleting custom hostname %v", change.CustomHostname.Hostname) - chID, _ := p.getCustomHostnameOrigin(chs, change.CustomHostname.Hostname) - if chID == "" { - log.WithFields(logFields).Infof("Custom hostname %v not found", change.CustomHostname.Hostname) - continue - } - chErr := p.Client.DeleteCustomHostname(ctx, zoneID, chID) - if chErr != nil { - failedChange = true - log.WithFields(logFields).Errorf("failed to delete custom hostname %v/%v: %v", chID, change.CustomHostname.Hostname, chErr) + if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { + log.WithFields(logFields).Infof("Deleting custom hostname %v", change.CustomHostname.Hostname) + chID, _ := getCustomHostnameIdWithOrigin(chs, change.CustomHostname.Hostname) + if chID != "" { + chErr := p.Client.DeleteCustomHostname(ctx, zoneID, chID) + if chErr != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to delete custom hostname %v/%v: %v", chID, change.CustomHostname.Hostname, chErr) + } + } else { + log.WithFields(logFields).Infof("Custom hostname %v not found", change.CustomHostname.Hostname) + } } } else if change.Action == cloudFlareCreate { recordParam := getCreateDNSRecordParam(*change) @@ -478,20 +477,23 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud failedChange = true log.WithFields(logFields).Errorf("failed to create record: %v", err) } - if change.CustomHostname.Hostname == "" { - continue - } - log.WithFields(logFields).Infof("Creating custom hostname %v", change.CustomHostname.Hostname) - chID, chOrigin := p.getCustomHostnameOrigin(chs, change.CustomHostname.Hostname) - if chID != "" { - failedChange = true - log.WithFields(logFields).Errorf("failed to create custom hostname, %v already exists for origin %v", change.CustomHostname.Hostname, chOrigin) - continue - } - _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname) - if chErr != nil { - failedChange = true - log.WithFields(logFields).Errorf("failed to create custom hostname %v: %v", change.CustomHostname.Hostname, chErr) + if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { + log.WithFields(logFields).Infof("Creating custom hostname %v", change.CustomHostname.Hostname) + chID, chOrigin := getCustomHostnameIdWithOrigin(chs, change.CustomHostname.Hostname) + if chID == "" { + _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname) + if chErr != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to create custom hostname %v: %v", change.CustomHostname.Hostname, chErr) + } + } else { + if change.CustomHostname.CustomOriginServer == chOrigin { + log.WithFields(logFields).Infof("custom hostname %v already exists with the same origin %v, continue", change.CustomHostname.Hostname, chOrigin) + } else { + failedChange = true + log.WithFields(logFields).Errorf("failed to create custom hostname, %v already exists with origin %v", change.CustomHostname.Hostname, chOrigin) + } + } } } } @@ -553,10 +555,10 @@ func (p *CloudFlareProvider) getRecordID(records []cloudflare.DNSRecord, record return "" } -func (p *CloudFlareProvider) getCustomHostnameOrigin(chs []cloudflare.CustomHostname, hostname string) (string, string) { - for _, zoneCh := range chs { - if zoneCh.Hostname == hostname { - return zoneCh.ID, zoneCh.CustomOriginServer +func getCustomHostnameIdWithOrigin(chs []cloudflare.CustomHostname, hostname string) (string, string) { + for _, ch := range chs { + if ch.Hostname == hostname { + return ch.ID, ch.CustomOriginServer } } return "", "" @@ -580,7 +582,7 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoi newCustomHostname = cloudflare.CustomHostname{ Hostname: getEndpointCustomHostname(endpoint), CustomOriginServer: endpoint.DNSName, - SSL: getCustomHostnamesSSLOptions(endpoint, p.CustomHostnamesConfig), + SSL: getCustomHostnamesSSLOptions(p.CustomHostnamesConfig), } } return &cloudFlareChange{ @@ -664,7 +666,7 @@ func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Conte return chs, nil } -func getCustomHostnamesSSLOptions(endpoint *endpoint.Endpoint, customHostnamesConfig CustomHostnamesConfig) *cloudflare.CustomHostnameSSL { +func getCustomHostnamesSSLOptions(customHostnamesConfig CustomHostnamesConfig) *cloudflare.CustomHostnameSSL { return &cloudflare.CustomHostnameSSL{ Type: "dv", Method: "http", diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index a85ff4849..360f3816f 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -404,10 +404,10 @@ func getCustomHostnameIdxByID(chs []cloudflare.CustomHostname, customHostnameID return -1 } -func (p *CloudFlareProvider) getCustomHostnameIDbyCustomHostnameAndOrigin(chs []cloudflare.CustomHostname, customHostname string, origin string) (string, string) { - for _, zoneCh := range chs { - if zoneCh.Hostname == customHostname && zoneCh.CustomOriginServer == origin { - return zoneCh.ID, zoneCh.Hostname +func getCustomHostnameIDbyCustomHostnameAndOrigin(chs []cloudflare.CustomHostname, customHostname string, origin string) (string, string) { + for _, ch := range chs { + if ch.Hostname == customHostname && ch.CustomOriginServer == origin { + return ch.ID, ch.Hostname } } @@ -1908,10 +1908,10 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { shouldFail: true, }, { - Name: "add custom hostname to more than one endpoint", + Name: "custom hostname to the same origin", Endpoints: []*endpoint.Endpoint{ { - DNSName: "fail.foo.bar.com", + DNSName: "origin.foo.bar.com", Targets: endpoint.Targets{"1.2.3.4", "2.3.4.5"}, RecordType: endpoint.RecordTypeA, RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), @@ -1919,13 +1919,72 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { ProviderSpecific: endpoint.ProviderSpecific{ { Name: "external-dns.alpha.kubernetes.io/cloudflare-custom-hostname", - Value: "fail.foo.fancybar.com", + Value: "custom.foo.fancybar.com", + }, + }, + }, + }, + shouldFail: false, + }, + { + Name: "same custom hostname to the another origin", + Endpoints: []*endpoint.Endpoint{ + { + DNSName: "another-origin.foo.bar.com", + Targets: endpoint.Targets{"3.4.5.6"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-custom-hostname", + Value: "custom.foo.fancybar.com", }, }, }, }, shouldFail: true, }, + { + Name: "create CNAME records with custom hostname", + Endpoints: []*endpoint.Endpoint{ + { + DNSName: "c.foo.bar.com", + Targets: endpoint.Targets{"c.cname.foo.bar.com"}, + RecordType: endpoint.RecordTypeCNAME, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-custom-hostname", + Value: "c.foo.fancybar.com", + }, + }, + }, + }, + shouldFail: false, + }, + { + Name: "TXT registry record should not attempt to create custom hostname", + Endpoints: []*endpoint.Endpoint{ + { + DNSName: "cname-c.foo.bar.com", + Targets: endpoint.Targets{ + "heritage=external-dns,external-dns/owner=default,external-dns/resource=service/external-dns/my-domain-here-app", + }, + RecordType: endpoint.RecordTypeTXT, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-custom-hostname", + Value: "c.foo.fancybar.com", + }, + }, + }, + }, + shouldFail: false, + }, { Name: "failing to update custom hostname", Endpoints: []*endpoint.Endpoint{ @@ -2167,7 +2226,7 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { Current: records, Desired: endpoints, DomainFilter: endpoint.MatchAllDomainFilters{&domainFilter}, - ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, + ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME, endpoint.RecordTypeTXT}, } planned := plan.Calculate() @@ -2209,7 +2268,7 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { } for expectedOrigin, expectedCustomHostname := range tc.ExpectedCustomHostnames { - _, ch := provider.getCustomHostnameIDbyCustomHostnameAndOrigin(chs, expectedCustomHostname, expectedOrigin) + _, ch := getCustomHostnameIDbyCustomHostnameAndOrigin(chs, expectedCustomHostname, expectedOrigin) assert.Equal(t, expectedCustomHostname, ch) } } @@ -2229,7 +2288,8 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { Name string Endpoints []*endpoint.Endpoint ExpectedCustomHostnames map[string]string - preApplyHook bool + preApplyHook string + logOutput string }{ { Name: "create DNS record with custom hostname", @@ -2248,17 +2308,46 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { }, }, }, - preApplyHook: false, + preApplyHook: "", + logOutput: "", }, { Name: "remove DNS record with unexpectedly missing custom hostname", Endpoints: []*endpoint.Endpoint{}, - preApplyHook: true, + preApplyHook: "corrupt", + logOutput: "level=info msg=\"Custom hostname newerror-getCustomHostnameOrigin.foo.fancybar.com not found\" action=DELETE record=create.foo.bar.com", + }, + { + Name: "duplicate custom hostname", + Endpoints: []*endpoint.Endpoint{}, + preApplyHook: "duplicate", + logOutput: "", + }, + { + Name: "create DNS record with custom hostname", + Endpoints: []*endpoint.Endpoint{ + { + DNSName: "a.foo.bar.com", + Targets: endpoint.Targets{"1.2.3.4"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-custom-hostname", + Value: "a.foo.fancybar.com", + }, + }, + }, + }, + preApplyHook: "", + logOutput: "custom hostname a.foo.fancybar.com already exists with the same origin a.foo.bar.com, continue", }, } - b := testutils.LogsToBuffer(log.InfoLevel, t) for _, tc := range testCases { + b := testutils.LogsToBuffer(log.InfoLevel, t) + records, err := provider.Records(ctx) if err != nil { t.Errorf("should not fail, %s", err) @@ -2278,12 +2367,12 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { // manually corrupt custom hostname before the deletion step // the purpose is to cause getCustomHostnameOrigin() to fail on change.Action == cloudFlareDelete - if tc.preApplyHook { - chs, chErr := provider.listCustomHostnamesWithPagination(ctx, zoneID) - if chErr != nil { - t.Errorf("should not fail - %s, %s", tc.Name, chErr) - } - chID, _ := provider.getCustomHostnameOrigin(chs, "newerror-getCustomHostnameOrigin.foo.fancybar.com") + chs, chErr := provider.listCustomHostnamesWithPagination(ctx, zoneID) + if chErr != nil { + t.Errorf("should not fail - %s, %s", tc.Name, chErr) + } + if tc.preApplyHook == "corrupt" { + chID, _ := getCustomHostnameIdWithOrigin(chs, "newerror-getCustomHostnameOrigin.foo.fancybar.com") if chID != "" { t.Logf("corrupting custom hostname %v", chID) oldIdx := getCustomHostnameIdxByID(client.customHostnames[zoneID], chID) @@ -2295,14 +2384,21 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { } client.customHostnames[zoneID][oldIdx] = ch } - } + } else if tc.preApplyHook == "duplicate" { // manually inject duplicating custom hostname with the same name and origin + ch := cloudflare.CustomHostname{ + ID: "ID-random-123", + Hostname: "a.foo.fancybar.com", + CustomOriginServer: "a.foo.bar.com", + } + client.customHostnames[zoneID] = append(client.customHostnames[zoneID], ch) + } err = provider.ApplyChanges(context.Background(), planned.Changes) if err != nil { t.Errorf("should not fail - %s, %s", tc.Name, err) } + assert.Contains(t, b.String(), tc.logOutput) } - assert.Contains(t, b.String(), "level=info msg=\"Custom hostname newerror-getCustomHostnameOrigin.foo.fancybar.com not found\" action=DELETE record=create.foo.bar.com") } func TestCloudflareListCustomHostnamesWithPagionation(t *testing.T) { From 2a15aba5648e56c60695b0491f880b44c5f164e1 Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Sat, 15 Mar 2025 09:50:35 -0700 Subject: [PATCH 02/16] syntax/style --- provider/cloudflare/cloudflare.go | 56 +++++++++++++------------- provider/cloudflare/cloudflare_test.go | 6 +-- 2 files changed, 32 insertions(+), 30 deletions(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index 5b63caaba..9880b1bce 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -403,26 +403,26 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud } if change.Action == cloudFlareUpdate { if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] { - prevCh := change.CustomHostnamePrev - newCh := change.CustomHostname.Hostname - if prevCh != "" { - prevChID, _ := getCustomHostnameIdWithOrigin(chs, prevCh) - if prevChID != "" && prevCh != newCh { - log.WithFields(logFields).Infof("Removing previous custom hostname %v/%v", prevChID, prevCh) + prevChName := change.CustomHostnamePrev + newChName := change.CustomHostname.Hostname + if prevCh, err := getCustomHostname(chs, prevChName); err == nil { + prevChID := prevCh.ID + if prevChID != "" && prevChName != newChName { + log.WithFields(logFields).Infof("Removing previous custom hostname %v/%v", prevChID, prevChName) chErr := p.Client.DeleteCustomHostname(ctx, zoneID, prevChID) if chErr != nil { failedChange = true - log.WithFields(logFields).Errorf("failed to remove previous custom hostname %v/%v: %v", prevChID, prevCh, chErr) + log.WithFields(logFields).Errorf("failed to remove previous custom hostname %v/%v: %v", prevChID, prevChName, chErr) } } } - if newCh != "" { - if prevCh != newCh { - log.WithFields(logFields).Infof("Adding custom hostname %v", newCh) + if newChName != "" { + if prevChName != newChName { + log.WithFields(logFields).Infof("Adding custom hostname %v", newChName) _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname) if chErr != nil { failedChange = true - log.WithFields(logFields).Errorf("failed to add custom hostname %v: %v", newCh, chErr) + log.WithFields(logFields).Errorf("failed to add custom hostname %v: %v", newChName, chErr) } } } @@ -459,15 +459,15 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud } if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { log.WithFields(logFields).Infof("Deleting custom hostname %v", change.CustomHostname.Hostname) - chID, _ := getCustomHostnameIdWithOrigin(chs, change.CustomHostname.Hostname) - if chID != "" { + if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil { + chID := ch.ID chErr := p.Client.DeleteCustomHostname(ctx, zoneID, chID) if chErr != nil { failedChange = true log.WithFields(logFields).Errorf("failed to delete custom hostname %v/%v: %v", chID, change.CustomHostname.Hostname, chErr) } } else { - log.WithFields(logFields).Infof("Custom hostname %v not found", change.CustomHostname.Hostname) + log.WithFields(logFields).Warnf("failed to delete custom hostname %v: %v", change.CustomHostname.Hostname, err) } } } else if change.Action == cloudFlareCreate { @@ -479,20 +479,19 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud } if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { log.WithFields(logFields).Infof("Creating custom hostname %v", change.CustomHostname.Hostname) - chID, chOrigin := getCustomHostnameIdWithOrigin(chs, change.CustomHostname.Hostname) - if chID == "" { + if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil { + if change.CustomHostname.CustomOriginServer == ch.CustomOriginServer { + log.WithFields(logFields).Warnf("custom hostname %v already exists with the same origin %v, continue", change.CustomHostname.Hostname, ch.CustomOriginServer) + } else { + failedChange = true + log.WithFields(logFields).Errorf("failed to create custom hostname, %v already exists with origin %v", change.CustomHostname.Hostname, ch.CustomOriginServer) + } + } else { _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname) if chErr != nil { failedChange = true log.WithFields(logFields).Errorf("failed to create custom hostname %v: %v", change.CustomHostname.Hostname, chErr) } - } else { - if change.CustomHostname.CustomOriginServer == chOrigin { - log.WithFields(logFields).Infof("custom hostname %v already exists with the same origin %v, continue", change.CustomHostname.Hostname, chOrigin) - } else { - failedChange = true - log.WithFields(logFields).Errorf("failed to create custom hostname, %v already exists with origin %v", change.CustomHostname.Hostname, chOrigin) - } } } } @@ -555,13 +554,16 @@ func (p *CloudFlareProvider) getRecordID(records []cloudflare.DNSRecord, record return "" } -func getCustomHostnameIdWithOrigin(chs []cloudflare.CustomHostname, hostname string) (string, string) { +func getCustomHostname(chs []cloudflare.CustomHostname, chName string) (cloudflare.CustomHostname, error) { + if chName == "" { + return cloudflare.CustomHostname{}, fmt.Errorf("empty") + } for _, ch := range chs { - if ch.Hostname == hostname { - return ch.ID, ch.CustomOriginServer + if ch.Hostname == chName { + return ch, nil } } - return "", "" + return cloudflare.CustomHostname{}, fmt.Errorf("not found") } func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoint.Endpoint, target string, current *endpoint.Endpoint) *cloudFlareChange { diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index 360f3816f..746115c82 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -2315,7 +2315,7 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { Name: "remove DNS record with unexpectedly missing custom hostname", Endpoints: []*endpoint.Endpoint{}, preApplyHook: "corrupt", - logOutput: "level=info msg=\"Custom hostname newerror-getCustomHostnameOrigin.foo.fancybar.com not found\" action=DELETE record=create.foo.bar.com", + logOutput: "level=warning msg=\"failed to delete custom hostname newerror-getCustomHostnameOrigin.foo.fancybar.com: not found\" action=DELETE record=create.foo.bar.com", }, { Name: "duplicate custom hostname", @@ -2372,8 +2372,8 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { t.Errorf("should not fail - %s, %s", tc.Name, chErr) } if tc.preApplyHook == "corrupt" { - chID, _ := getCustomHostnameIdWithOrigin(chs, "newerror-getCustomHostnameOrigin.foo.fancybar.com") - if chID != "" { + if ch, err := getCustomHostname(chs, "newerror-getCustomHostnameOrigin.foo.fancybar.com"); err == nil { + chID := ch.ID t.Logf("corrupting custom hostname %v", chID) oldIdx := getCustomHostnameIdxByID(client.customHostnames[zoneID], chID) oldCh := client.customHostnames[zoneID][oldIdx] From 12cce6fa7febd77ad8f627318d36fc8a5fb14e61 Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Sun, 16 Mar 2025 10:47:59 -0700 Subject: [PATCH 03/16] Use %q log fmt for cloudflare provider code --- provider/cloudflare/cloudflare.go | 40 +++++++++++++------------- provider/cloudflare/cloudflare_test.go | 34 +++++++++++----------- 2 files changed, 37 insertions(+), 37 deletions(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index 9880b1bce..378474918 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -229,7 +229,7 @@ func NewCloudFlareProvider(domainFilter endpoint.DomainFilter, zoneIDFilter prov config, err = cloudflare.New(os.Getenv("CF_API_KEY"), os.Getenv("CF_API_EMAIL")) } if err != nil { - return nil, fmt.Errorf("failed to initialize cloudflare provider: %v", err) + return nil, fmt.Errorf("failed to initialize cloudflare provider: %w", err) } provider := &CloudFlareProvider{ // Client: config, @@ -254,10 +254,10 @@ func (p *CloudFlareProvider) Zones(ctx context.Context) ([]cloudflare.Zone, erro if len(p.zoneIDFilter.ZoneIDs) > 0 && p.zoneIDFilter.ZoneIDs[0] != "" { log.Debugln("zoneIDFilter configured. only looking up zone IDs defined") for _, zoneID := range p.zoneIDFilter.ZoneIDs { - log.Debugf("looking up zone %s", zoneID) + log.Debugf("looking up zone %q", zoneID) detailResponse, err := p.Client.ZoneDetails(ctx, zoneID) if err != nil { - log.Errorf("zone %s lookup failed, %v", zoneID, err) + log.Errorf("zone %q lookup failed, %v", zoneID, err) return result, err } log.WithFields(log.Fields{ @@ -285,7 +285,7 @@ func (p *CloudFlareProvider) Zones(ctx context.Context) ([]cloudflare.Zone, erro for _, zone := range zonesResponse.Result { if !p.domainFilter.Match(zone.Name) { - log.Debugf("zone %s not in domain filter", zone.Name) + log.Debugf("zone %q not in domain filter", zone.Name) continue } result = append(result, zone) @@ -395,7 +395,7 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud resourceContainer := cloudflare.ZoneIdentifier(zoneID) records, err := p.listDNSRecordsWithAutoPagination(ctx, zoneID) if err != nil { - return fmt.Errorf("could not fetch records from zone, %v", err) + return fmt.Errorf("could not fetch records from zone, %w", err) } chs, chErr := p.listCustomHostnamesWithPagination(ctx, zoneID) if chErr != nil { @@ -408,21 +408,21 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud if prevCh, err := getCustomHostname(chs, prevChName); err == nil { prevChID := prevCh.ID if prevChID != "" && prevChName != newChName { - log.WithFields(logFields).Infof("Removing previous custom hostname %v/%v", prevChID, prevChName) + log.WithFields(logFields).Infof("Removing previous custom hostname %q/%q", prevChID, prevChName) chErr := p.Client.DeleteCustomHostname(ctx, zoneID, prevChID) if chErr != nil { failedChange = true - log.WithFields(logFields).Errorf("failed to remove previous custom hostname %v/%v: %v", prevChID, prevChName, chErr) + log.WithFields(logFields).Errorf("failed to remove previous custom hostname %q/%q: %v", prevChID, prevChName, chErr) } } } if newChName != "" { if prevChName != newChName { - log.WithFields(logFields).Infof("Adding custom hostname %v", newChName) + log.WithFields(logFields).Infof("Adding custom hostname %q", newChName) _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname) if chErr != nil { failedChange = true - log.WithFields(logFields).Errorf("failed to add custom hostname %v: %v", newChName, chErr) + log.WithFields(logFields).Errorf("failed to add custom hostname %q: %v", newChName, chErr) } } } @@ -458,16 +458,16 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud log.WithFields(logFields).Errorf("failed to delete record: %v", err) } if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { - log.WithFields(logFields).Infof("Deleting custom hostname %v", change.CustomHostname.Hostname) + log.WithFields(logFields).Infof("Deleting custom hostname %q", change.CustomHostname.Hostname) if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil { chID := ch.ID chErr := p.Client.DeleteCustomHostname(ctx, zoneID, chID) if chErr != nil { failedChange = true - log.WithFields(logFields).Errorf("failed to delete custom hostname %v/%v: %v", chID, change.CustomHostname.Hostname, chErr) + log.WithFields(logFields).Errorf("failed to delete custom hostname %q/%q: %v", chID, change.CustomHostname.Hostname, chErr) } } else { - log.WithFields(logFields).Warnf("failed to delete custom hostname %v: %v", change.CustomHostname.Hostname, err) + log.WithFields(logFields).Warnf("failed to delete custom hostname %q: %v", change.CustomHostname.Hostname, err) } } } else if change.Action == cloudFlareCreate { @@ -478,19 +478,19 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud log.WithFields(logFields).Errorf("failed to create record: %v", err) } if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { - log.WithFields(logFields).Infof("Creating custom hostname %v", change.CustomHostname.Hostname) + log.WithFields(logFields).Infof("Creating custom hostname %q", change.CustomHostname.Hostname) if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil { if change.CustomHostname.CustomOriginServer == ch.CustomOriginServer { - log.WithFields(logFields).Warnf("custom hostname %v already exists with the same origin %v, continue", change.CustomHostname.Hostname, ch.CustomOriginServer) + log.WithFields(logFields).Warnf("custom hostname %q already exists with the same origin %q, continue", change.CustomHostname.Hostname, ch.CustomOriginServer) } else { failedChange = true - log.WithFields(logFields).Errorf("failed to create custom hostname, %v already exists with origin %v", change.CustomHostname.Hostname, ch.CustomOriginServer) + log.WithFields(logFields).Errorf("failed to create custom hostname, %q already exists with origin %q", change.CustomHostname.Hostname, ch.CustomOriginServer) } } else { _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname) if chErr != nil { failedChange = true - log.WithFields(logFields).Errorf("failed to create custom hostname %v: %v", change.CustomHostname.Hostname, chErr) + log.WithFields(logFields).Errorf("failed to create custom hostname %q: %v", change.CustomHostname.Hostname, chErr) } } } @@ -502,7 +502,7 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud } if len(failedZones) > 0 { - return fmt.Errorf("failed to submit all changes for the following zones: %v", failedZones) + return fmt.Errorf("failed to submit all changes for the following zones: %q", failedZones) } return nil @@ -536,7 +536,7 @@ func (p *CloudFlareProvider) changesByZone(zones []cloudflare.Zone, changeSet [] for _, c := range changeSet { zoneID, _ := zoneNameIDMapper.FindZone(c.ResourceRecord.Name) if zoneID == "" { - log.Debugf("Skipping record %s because no hosted zone matching record DNS Name was detected", c.ResourceRecord.Name) + log.Debugf("Skipping record %q because no hosted zone matching record DNS Name was detected", c.ResourceRecord.Name) continue } changes[zoneID] = append(changes[zoneID], c) @@ -655,7 +655,7 @@ func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Conte return nil, provider.NewSoftError(err) } } - log.Errorf("zone %s 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 } @@ -687,7 +687,7 @@ func shouldBeProxied(endpoint *endpoint.Endpoint, proxiedByDefault bool) bool { if v.Name == source.CloudflareProxiedKey { b, err := strconv.ParseBool(v.Value) if err != nil { - log.Errorf("Failed to parse annotation [%s]: %v", source.CloudflareProxiedKey, err) + log.Errorf("Failed to parse annotation [%q]: %v", source.CloudflareProxiedKey, err) } else { proxied = b } diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index 746115c82..dd92ed366 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -1866,9 +1866,9 @@ func TestCloudflareDNSRecordsOperationsFail(t *testing.T) { err = provider.ApplyChanges(context.Background(), planned.Changes) if err == nil && tc.shouldFail { - t.Errorf("should fail - %s, %s", tc.Name, err) + t.Errorf("should fail - %q, %v", tc.Name, err) } else if err != nil && !tc.shouldFail { - t.Errorf("should not fail - %s, %s", tc.Name, err) + t.Errorf("should not fail - %q, %v", tc.Name, err) } } } @@ -2216,7 +2216,7 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { for _, tc := range testFailCases { records, err := provider.Records(ctx) if err != nil { - t.Errorf("should not fail, %s", err) + t.Errorf("should not fail, %v", err) } endpoints, err := provider.AdjustEndpoints(tc.Endpoints) @@ -2233,16 +2233,16 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { err = provider.ApplyChanges(context.Background(), planned.Changes) if err == nil && tc.shouldFail { - t.Errorf("should fail - %s, %s", tc.Name, err) + t.Errorf("should fail - %q, %v", tc.Name, err) } else if err != nil && !tc.shouldFail { - t.Errorf("should not fail - %s, %s", tc.Name, err) + t.Errorf("should not fail - %q, %v", tc.Name, err) } } for _, tc := range testCases { records, err := provider.Records(ctx) if err != nil { - t.Errorf("should not fail, %s", err) + t.Errorf("should not fail, %v", err) } endpoints, err := provider.AdjustEndpoints(tc.Endpoints) @@ -2259,12 +2259,12 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { err = provider.ApplyChanges(context.Background(), planned.Changes) if err != nil { - t.Errorf("should not fail - %s, %s", tc.Name, err) + t.Errorf("should not fail - %q, %v", tc.Name, err) } chs, chErr := provider.listCustomHostnamesWithPagination(ctx, "001") if chErr != nil { - t.Errorf("should not fail - %s, %s", tc.Name, chErr) + t.Errorf("should not fail - %q, %v", tc.Name, chErr) } for expectedOrigin, expectedCustomHostname := range tc.ExpectedCustomHostnames { @@ -2315,7 +2315,7 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { Name: "remove DNS record with unexpectedly missing custom hostname", Endpoints: []*endpoint.Endpoint{}, preApplyHook: "corrupt", - logOutput: "level=warning msg=\"failed to delete custom hostname newerror-getCustomHostnameOrigin.foo.fancybar.com: not found\" action=DELETE record=create.foo.bar.com", + logOutput: "level=warning msg=\"failed to delete custom hostname \\\"newerror-getCustomHostnameOrigin.foo.fancybar.com\\\": not found\" action=DELETE record=create.foo.bar.com", }, { Name: "duplicate custom hostname", @@ -2341,7 +2341,7 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { }, }, preApplyHook: "", - logOutput: "custom hostname a.foo.fancybar.com already exists with the same origin a.foo.bar.com, continue", + logOutput: "custom hostname \\\"a.foo.fancybar.com\\\" already exists with the same origin \\\"a.foo.bar.com\\\", continue", }, } @@ -2350,7 +2350,7 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { records, err := provider.Records(ctx) if err != nil { - t.Errorf("should not fail, %s", err) + t.Errorf("should not fail, %v", err) } endpoints, err := provider.AdjustEndpoints(tc.Endpoints) @@ -2369,12 +2369,12 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { // the purpose is to cause getCustomHostnameOrigin() to fail on change.Action == cloudFlareDelete chs, chErr := provider.listCustomHostnamesWithPagination(ctx, zoneID) if chErr != nil { - t.Errorf("should not fail - %s, %s", tc.Name, chErr) + t.Errorf("should not fail - %q, %v", tc.Name, chErr) } if tc.preApplyHook == "corrupt" { if ch, err := getCustomHostname(chs, "newerror-getCustomHostnameOrigin.foo.fancybar.com"); err == nil { chID := ch.ID - t.Logf("corrupting custom hostname %v", chID) + t.Logf("corrupting custom hostname %q", chID) oldIdx := getCustomHostnameIdxByID(client.customHostnames[zoneID], chID) oldCh := client.customHostnames[zoneID][oldIdx] ch := cloudflare.CustomHostname{ @@ -2395,7 +2395,7 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { } err = provider.ApplyChanges(context.Background(), planned.Changes) if err != nil { - t.Errorf("should not fail - %s, %s", tc.Name, err) + t.Errorf("should not fail - %q, %v", tc.Name, err) } assert.Contains(t, b.String(), tc.logOutput) } @@ -2433,7 +2433,7 @@ func TestCloudflareListCustomHostnamesWithPagionation(t *testing.T) { records, err := provider.Records(ctx) if err != nil { - t.Errorf("should not fail, %s", err) + t.Errorf("should not fail, %v", err) } endpoints, err := provider.AdjustEndpoints(generatedEndpoints) @@ -2450,12 +2450,12 @@ func TestCloudflareListCustomHostnamesWithPagionation(t *testing.T) { err = provider.ApplyChanges(context.Background(), planned.Changes) if err != nil { - t.Errorf("should not fail - %s", err) + t.Errorf("should not fail - %v", err) } chs, chErr := provider.listCustomHostnamesWithPagination(ctx, "001") if chErr != nil { - t.Errorf("should not fail - %s", chErr) + t.Errorf("should not fail - %v", chErr) } assert.Equal(t, len(chs), CustomHostnamesNumber) } From b1f20511c5f4e46ac82172e4d8adbf41d1fe9c87 Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Tue, 18 Mar 2025 10:07:13 -0700 Subject: [PATCH 04/16] move custom hostnames related submitChanges() implementation to a separate method submitCustomHostnameChanges(); extend truncated logging --- provider/cloudflare/cloudflare.go | 133 +++++++++++++++---------- provider/cloudflare/cloudflare_test.go | 2 +- 2 files changed, 80 insertions(+), 55 deletions(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index 378474918..7b8493217 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -359,6 +359,77 @@ func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Cha return p.submitChanges(ctx, cloudflareChanges) } +// submitCustomHostnameChanges implements Custom Hostname functionality for the Change, returns false if it fails +func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zoneID string, change *cloudFlareChange, chs []cloudflare.CustomHostname, logFields log.Fields) bool { + failedChange := false + // return early if disabled + if !p.CustomHostnamesConfig.Enabled { + return !failedChange + } + + switch change.Action { + case cloudFlareUpdate: + if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] { + prevChName := change.CustomHostnamePrev + newChName := change.CustomHostname.Hostname + if prevCh, err := getCustomHostname(chs, prevChName); err == nil { + prevChID := prevCh.ID + if prevChID != "" && prevChName != newChName { + log.WithFields(logFields).Infof("Removing previous custom hostname %q/%q", prevChID, prevChName) + chErr := p.Client.DeleteCustomHostname(ctx, zoneID, prevChID) + if chErr != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to remove previous custom hostname %q/%q: %v", prevChID, prevChName, chErr) + } + } + } + if newChName != "" { + if prevChName != newChName { + log.WithFields(logFields).Infof("Adding custom hostname %q", newChName) + _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname) + if chErr != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to add custom hostname %q: %v", newChName, chErr) + } + } + } + } + case cloudFlareDelete: + if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { + log.WithFields(logFields).Infof("Deleting custom hostname %q", change.CustomHostname.Hostname) + if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil { + chID := ch.ID + chErr := p.Client.DeleteCustomHostname(ctx, zoneID, chID) + if chErr != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to delete custom hostname %q/%q: %v", chID, change.CustomHostname.Hostname, chErr) + } + } else { + log.WithFields(logFields).Warnf("failed to delete custom hostname %q: %v", change.CustomHostname.Hostname, err) + } + } + case cloudFlareCreate: + if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { + log.WithFields(logFields).Infof("Creating custom hostname %q", change.CustomHostname.Hostname) + if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil { + if change.CustomHostname.CustomOriginServer == ch.CustomOriginServer { + log.WithFields(logFields).Warnf("custom hostname %q already exists with the same origin %q, continue", change.CustomHostname.Hostname, ch.CustomOriginServer) + } else { + failedChange = true + log.WithFields(logFields).Errorf("failed to create custom hostname, %q already exists with origin %q", change.CustomHostname.Hostname, ch.CustomOriginServer) + } + } else { + _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname) + if chErr != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to create custom hostname %q: %v", change.CustomHostname.Hostname, chErr) + } + } + } + } + return !failedChange +} + // submitChanges takes a zone and a collection of Changes and sends them as a single transaction. func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloudFlareChange) error { // return early if there is nothing to change @@ -402,30 +473,8 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud return fmt.Errorf("could not fetch custom hostnames from zone, %v", chErr) } if change.Action == cloudFlareUpdate { - if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] { - prevChName := change.CustomHostnamePrev - newChName := change.CustomHostname.Hostname - if prevCh, err := getCustomHostname(chs, prevChName); err == nil { - prevChID := prevCh.ID - if prevChID != "" && prevChName != newChName { - log.WithFields(logFields).Infof("Removing previous custom hostname %q/%q", prevChID, prevChName) - chErr := p.Client.DeleteCustomHostname(ctx, zoneID, prevChID) - if chErr != nil { - failedChange = true - log.WithFields(logFields).Errorf("failed to remove previous custom hostname %q/%q: %v", prevChID, prevChName, chErr) - } - } - } - if newChName != "" { - if prevChName != newChName { - log.WithFields(logFields).Infof("Adding custom hostname %q", newChName) - _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname) - if chErr != nil { - failedChange = true - log.WithFields(logFields).Errorf("failed to add custom hostname %q: %v", newChName, chErr) - } - } - } + if !p.submitCustomHostnameChanges(ctx, zoneID, change, chs, logFields) { + failedChange = true } recordID := p.getRecordID(records, change.ResourceRecord) if recordID == "" { @@ -457,18 +506,8 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud failedChange = true log.WithFields(logFields).Errorf("failed to delete record: %v", err) } - if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { - log.WithFields(logFields).Infof("Deleting custom hostname %q", change.CustomHostname.Hostname) - if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil { - chID := ch.ID - chErr := p.Client.DeleteCustomHostname(ctx, zoneID, chID) - if chErr != nil { - failedChange = true - log.WithFields(logFields).Errorf("failed to delete custom hostname %q/%q: %v", chID, change.CustomHostname.Hostname, chErr) - } - } else { - log.WithFields(logFields).Warnf("failed to delete custom hostname %q: %v", change.CustomHostname.Hostname, err) - } + if !p.submitCustomHostnameChanges(ctx, zoneID, change, chs, logFields) { + failedChange = true } } else if change.Action == cloudFlareCreate { recordParam := getCreateDNSRecordParam(*change) @@ -477,22 +516,8 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud failedChange = true log.WithFields(logFields).Errorf("failed to create record: %v", err) } - if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { - log.WithFields(logFields).Infof("Creating custom hostname %q", change.CustomHostname.Hostname) - if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil { - if change.CustomHostname.CustomOriginServer == ch.CustomOriginServer { - log.WithFields(logFields).Warnf("custom hostname %q already exists with the same origin %q, continue", change.CustomHostname.Hostname, ch.CustomOriginServer) - } else { - failedChange = true - log.WithFields(logFields).Errorf("failed to create custom hostname, %q already exists with origin %q", change.CustomHostname.Hostname, ch.CustomOriginServer) - } - } else { - _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname) - if chErr != nil { - failedChange = true - log.WithFields(logFields).Errorf("failed to create custom hostname %q: %v", change.CustomHostname.Hostname, chErr) - } - } + if !p.submitCustomHostnameChanges(ctx, zoneID, change, chs, logFields) { + failedChange = true } } } @@ -556,14 +581,14 @@ func (p *CloudFlareProvider) getRecordID(records []cloudflare.DNSRecord, record func getCustomHostname(chs []cloudflare.CustomHostname, chName string) (cloudflare.CustomHostname, error) { if chName == "" { - return cloudflare.CustomHostname{}, fmt.Errorf("empty") + return cloudflare.CustomHostname{}, fmt.Errorf("failed to get custom hostname: %q is empty", chName) } for _, ch := range chs { if ch.Hostname == chName { return ch, nil } } - return cloudflare.CustomHostname{}, fmt.Errorf("not found") + return cloudflare.CustomHostname{}, fmt.Errorf("failed to get custom hostname: %q not found", chName) } func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoint.Endpoint, target string, current *endpoint.Endpoint) *cloudFlareChange { diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index dd92ed366..6758f740c 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -2315,7 +2315,7 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { Name: "remove DNS record with unexpectedly missing custom hostname", Endpoints: []*endpoint.Endpoint{}, preApplyHook: "corrupt", - logOutput: "level=warning msg=\"failed to delete custom hostname \\\"newerror-getCustomHostnameOrigin.foo.fancybar.com\\\": not found\" action=DELETE record=create.foo.bar.com", + logOutput: "level=warning msg=\"failed to delete custom hostname \\\"newerror-getCustomHostnameOrigin.foo.fancybar.com\\\": failed to get custom hostname: \\\"newerror-getCustomHostnameOrigin.foo.fancybar.com\\\" not found\" action=DELETE record=create.foo.bar.com", }, { Name: "duplicate custom hostname", From 037a10174f196af532e67d69051930a3d673203a Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Tue, 18 Mar 2025 12:12:57 -0700 Subject: [PATCH 05/16] use maps for DNS records getRecordID() and custom hostnames getCustomHostname() for faster lookups --- go.mod | 1 + go.sum | 2 + provider/cloudflare/cloudflare.go | 66 ++++++++++++++++---------- provider/cloudflare/cloudflare_test.go | 22 +++++---- 4 files changed, 55 insertions(+), 36 deletions(-) diff --git a/go.mod b/go.mod index a5b3a5ca7..9091fdb11 100644 --- a/go.mod +++ b/go.mod @@ -64,6 +64,7 @@ require ( github.com/ultradns/ultradns-sdk-go v1.3.7 go.etcd.io/etcd/client/v3 v3.5.19 go.uber.org/ratelimit v0.3.1 + golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 golang.org/x/net v0.36.0 golang.org/x/oauth2 v0.28.0 golang.org/x/sync v0.12.0 diff --git a/go.sum b/go.sum index 3ef64734c..1a79a5705 100644 --- a/go.sum +++ b/go.sum @@ -1157,6 +1157,8 @@ golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL golang.org/x/exp v0.0.0-20190125153040-c74c464bbbf2/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190306152737-a1d7652674e8/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20191030013958-a1ab85dbe136/go.mod h1:JXzH8nQsPlswgeRAPE3MuO9GYsAcnJvJ4vnMwN/5qkY= +golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 h1:2dVuKD2vS7b0QIHQbpyTISPd0LeHDbnYEryqj5Q1ug8= +golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56/go.mod h1:M4RDyNAINzryxdtnbRXRL/OHtkFuWGRjvuhBJpk2IlY= golang.org/x/image v0.0.0-20180708004352-c73c2afc3b81/go.mod h1:ux5Hcp/YLpHSI86hEcLt0YII63i6oz57MZXIpbrjZUs= golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js= golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0= diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index 7b8493217..39563ecde 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -26,6 +26,8 @@ import ( "strings" "time" + "golang.org/x/exp/maps" + cloudflare "github.com/cloudflare/cloudflare-go" log "github.com/sirupsen/logrus" @@ -55,6 +57,18 @@ var proxyEnabled *bool = boolPtr(true) // proxyDisabled is a pointer to a bool false showing the record should not be proxied through cloudflare var proxyDisabled *bool = boolPtr(false) +// for faster getRecordID() lookup +type DNSRecordIndex struct { + Name string + Type string + Content string +} + +// for faster getCustomHostname() lookup +type CustomHostnameIndex struct { + Hostname string +} + var recordTypeProxyNotSupported = map[string]bool{ "LOC": true, "MX": true, @@ -303,12 +317,12 @@ func (p *CloudFlareProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, endpoints := []*endpoint.Endpoint{} for _, zone := range zones { - records, err := p.listDNSRecordsWithAutoPagination(ctx, zone.ID) + recordsMap, err := p.listDNSRecordsWithAutoPagination(ctx, zone.ID) if err != nil { return nil, err } - chs, chErr := p.listCustomHostnamesWithPagination(ctx, zone.ID) + chsMap, chErr := p.listCustomHostnamesWithPagination(ctx, zone.ID) if chErr != nil { return nil, chErr } @@ -316,7 +330,7 @@ func (p *CloudFlareProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, // As CloudFlare does not support "sets" of targets, but instead returns // a single entry for each name/type/target, we have to group by name // and record to allow the planner to calculate the correct plan. See #992. - endpoints = append(endpoints, groupByNameAndTypeWithCustomHostnames(records, chs)...) + endpoints = append(endpoints, groupByNameAndTypeWithCustomHostnames(maps.Values(recordsMap), maps.Values(chsMap))...) } return endpoints, nil @@ -360,7 +374,7 @@ func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Cha } // submitCustomHostnameChanges implements Custom Hostname functionality for the Change, returns false if it fails -func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zoneID string, change *cloudFlareChange, chs []cloudflare.CustomHostname, logFields log.Fields) bool { +func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zoneID string, change *cloudFlareChange, chsMap map[CustomHostnameIndex]cloudflare.CustomHostname, logFields log.Fields) bool { failedChange := false // return early if disabled if !p.CustomHostnamesConfig.Enabled { @@ -372,7 +386,7 @@ func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zo if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] { prevChName := change.CustomHostnamePrev newChName := change.CustomHostname.Hostname - if prevCh, err := getCustomHostname(chs, prevChName); err == nil { + if prevCh, err := getCustomHostname(chsMap, prevChName); err == nil { prevChID := prevCh.ID if prevChID != "" && prevChName != newChName { log.WithFields(logFields).Infof("Removing previous custom hostname %q/%q", prevChID, prevChName) @@ -397,7 +411,7 @@ func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zo case cloudFlareDelete: if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { log.WithFields(logFields).Infof("Deleting custom hostname %q", change.CustomHostname.Hostname) - if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil { + if ch, err := getCustomHostname(chsMap, change.CustomHostname.Hostname); err == nil { chID := ch.ID chErr := p.Client.DeleteCustomHostname(ctx, zoneID, chID) if chErr != nil { @@ -411,7 +425,7 @@ func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zo case cloudFlareCreate: if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { log.WithFields(logFields).Infof("Creating custom hostname %q", change.CustomHostname.Hostname) - if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil { + if ch, err := getCustomHostname(chsMap, change.CustomHostname.Hostname); err == nil { if change.CustomHostname.CustomOriginServer == ch.CustomOriginServer { log.WithFields(logFields).Warnf("custom hostname %q already exists with the same origin %q, continue", change.CustomHostname.Hostname, ch.CustomOriginServer) } else { @@ -570,23 +584,19 @@ func (p *CloudFlareProvider) changesByZone(zones []cloudflare.Zone, changeSet [] return changes } -func (p *CloudFlareProvider) getRecordID(records []cloudflare.DNSRecord, record cloudflare.DNSRecord) string { - for _, zoneRecord := range records { - if zoneRecord.Name == record.Name && zoneRecord.Type == record.Type && zoneRecord.Content == record.Content { - return zoneRecord.ID - } +func (p *CloudFlareProvider) getRecordID(recordsMap map[DNSRecordIndex]cloudflare.DNSRecord, record cloudflare.DNSRecord) string { + if zoneRecord, ok := recordsMap[DNSRecordIndex{Name: record.Name, Type: record.Type, Content: record.Content}]; ok { + return zoneRecord.ID } return "" } -func getCustomHostname(chs []cloudflare.CustomHostname, chName string) (cloudflare.CustomHostname, error) { +func getCustomHostname(chsMap map[CustomHostnameIndex]cloudflare.CustomHostname, chName string) (cloudflare.CustomHostname, error) { if chName == "" { return cloudflare.CustomHostname{}, fmt.Errorf("failed to get custom hostname: %q is empty", chName) } - for _, ch := range chs { - if ch.Hostname == chName { - return ch, nil - } + if ch, ok := chsMap[CustomHostnameIndex{Hostname: chName}]; ok { + return ch, nil } return cloudflare.CustomHostname{}, fmt.Errorf("failed to get custom hostname: %q not found", chName) } @@ -637,8 +647,9 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoi } // listDNSRecordsWithAutoPagination performs automatic pagination of results on requests to cloudflare.ListDNSRecords with custom per_page values -func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Context, zoneID string) ([]cloudflare.DNSRecord, error) { - var records []cloudflare.DNSRecord +func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Context, zoneID string) (map[DNSRecordIndex]cloudflare.DNSRecord, error) { + // for faster getRecordID lookup + recordsMap := make(map[DNSRecordIndex]cloudflare.DNSRecord) resultInfo := cloudflare.ResultInfo{PerPage: p.DNSRecordsPerPage, Page: 1} params := cloudflare.ListDNSRecordsParams{ResultInfo: resultInfo} for { @@ -654,21 +665,23 @@ func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Contex return nil, err } - records = append(records, pageRecords...) + for _, r := range pageRecords { + recordsMap[DNSRecordIndex{Name: r.Name, Type: r.Type, Content: r.Content}] = r + } params.ResultInfo = resultInfo.Next() if params.ResultInfo.Done() { break } } - return records, nil + return recordsMap, nil } // listCustomHostnamesWithPagination performs automatic pagination of results on requests to cloudflare.CustomHostnames -func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Context, zoneID string) ([]cloudflare.CustomHostname, error) { +func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Context, zoneID string) (map[CustomHostnameIndex]cloudflare.CustomHostname, error) { if !p.CustomHostnamesConfig.Enabled { return nil, nil } - var chs []cloudflare.CustomHostname + chsMap := make(map[CustomHostnameIndex]cloudflare.CustomHostname) resultInfo := cloudflare.ResultInfo{Page: 1} for { pageCustomHostnameListResponse, result, err := p.Client.CustomHostnames(ctx, zoneID, resultInfo.Page, cloudflare.CustomHostname{}) @@ -683,14 +696,15 @@ func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Conte 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 } - - chs = append(chs, pageCustomHostnameListResponse...) + for _, ch := range pageCustomHostnameListResponse { + chsMap[CustomHostnameIndex{Hostname: ch.Hostname}] = ch + } resultInfo = result.Next() if resultInfo.Done() { break } } - return chs, nil + return chsMap, nil } func getCustomHostnamesSSLOptions(customHostnamesConfig CustomHostnamesConfig) *cloudflare.CustomHostnameSSL { diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index 6758f740c..b5a18999f 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -25,6 +25,8 @@ import ( "strings" "testing" + "golang.org/x/exp/maps" + cloudflare "github.com/cloudflare/cloudflare-go" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -1029,19 +1031,19 @@ func TestCloudflareApplyChangesError(t *testing.T) { func TestCloudflareGetRecordID(t *testing.T) { p := &CloudFlareProvider{} - records := []cloudflare.DNSRecord{ - { + recordsMap := map[DNSRecordIndex]cloudflare.DNSRecord{ + DNSRecordIndex{Name: "foo.com", Type: endpoint.RecordTypeCNAME, Content: "foobar"}: { Name: "foo.com", Type: endpoint.RecordTypeCNAME, Content: "foobar", ID: "1", }, - { + DNSRecordIndex{Name: "bar.de", Type: endpoint.RecordTypeA}: { Name: "bar.de", Type: endpoint.RecordTypeA, ID: "2", }, - { + DNSRecordIndex{Name: "bar.de", Type: endpoint.RecordTypeA, Content: "1.2.3.4"}: { Name: "bar.de", Type: endpoint.RecordTypeA, Content: "1.2.3.4", @@ -1049,29 +1051,29 @@ func TestCloudflareGetRecordID(t *testing.T) { }, } - assert.Equal(t, "", p.getRecordID(records, cloudflare.DNSRecord{ + assert.Equal(t, "", p.getRecordID(recordsMap, cloudflare.DNSRecord{ Name: "foo.com", Type: endpoint.RecordTypeA, Content: "foobar", })) - assert.Equal(t, "", p.getRecordID(records, cloudflare.DNSRecord{ + assert.Equal(t, "", p.getRecordID(recordsMap, cloudflare.DNSRecord{ Name: "foo.com", Type: endpoint.RecordTypeCNAME, Content: "fizfuz", })) - assert.Equal(t, "1", p.getRecordID(records, cloudflare.DNSRecord{ + assert.Equal(t, "1", p.getRecordID(recordsMap, cloudflare.DNSRecord{ Name: "foo.com", Type: endpoint.RecordTypeCNAME, Content: "foobar", })) - assert.Equal(t, "", p.getRecordID(records, cloudflare.DNSRecord{ + assert.Equal(t, "", p.getRecordID(recordsMap, cloudflare.DNSRecord{ Name: "bar.de", Type: endpoint.RecordTypeA, Content: "2.3.4.5", })) - assert.Equal(t, "2", p.getRecordID(records, cloudflare.DNSRecord{ + assert.Equal(t, "2", p.getRecordID(recordsMap, cloudflare.DNSRecord{ Name: "bar.de", Type: endpoint.RecordTypeA, Content: "1.2.3.4", @@ -2268,7 +2270,7 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { } for expectedOrigin, expectedCustomHostname := range tc.ExpectedCustomHostnames { - _, ch := getCustomHostnameIDbyCustomHostnameAndOrigin(chs, expectedCustomHostname, expectedOrigin) + _, ch := getCustomHostnameIDbyCustomHostnameAndOrigin(maps.Values(chs), expectedCustomHostname, expectedOrigin) assert.Equal(t, expectedCustomHostname, ch) } } From e47d83b22409fabb912065f8b6abd7f3a2f7200e Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Tue, 18 Mar 2025 12:24:54 -0700 Subject: [PATCH 06/16] types for records/custom hostnames maps --- provider/cloudflare/cloudflare.go | 18 +++++++++++------- provider/cloudflare/cloudflare_test.go | 8 ++++---- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index 39563ecde..554bfe2d7 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -64,11 +64,15 @@ type DNSRecordIndex struct { Content string } +type DNSRecordMap map[DNSRecordIndex]cloudflare.DNSRecord + // for faster getCustomHostname() lookup type CustomHostnameIndex struct { Hostname string } +type CustomHostnameMap map[CustomHostnameIndex]cloudflare.CustomHostname + var recordTypeProxyNotSupported = map[string]bool{ "LOC": true, "MX": true, @@ -374,7 +378,7 @@ func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Cha } // submitCustomHostnameChanges implements Custom Hostname functionality for the Change, returns false if it fails -func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zoneID string, change *cloudFlareChange, chsMap map[CustomHostnameIndex]cloudflare.CustomHostname, logFields log.Fields) bool { +func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zoneID string, change *cloudFlareChange, chsMap CustomHostnameMap, logFields log.Fields) bool { failedChange := false // return early if disabled if !p.CustomHostnamesConfig.Enabled { @@ -584,14 +588,14 @@ func (p *CloudFlareProvider) changesByZone(zones []cloudflare.Zone, changeSet [] return changes } -func (p *CloudFlareProvider) getRecordID(recordsMap map[DNSRecordIndex]cloudflare.DNSRecord, record cloudflare.DNSRecord) string { +func (p *CloudFlareProvider) getRecordID(recordsMap DNSRecordMap, record cloudflare.DNSRecord) string { if zoneRecord, ok := recordsMap[DNSRecordIndex{Name: record.Name, Type: record.Type, Content: record.Content}]; ok { return zoneRecord.ID } return "" } -func getCustomHostname(chsMap map[CustomHostnameIndex]cloudflare.CustomHostname, chName string) (cloudflare.CustomHostname, error) { +func getCustomHostname(chsMap CustomHostnameMap, chName string) (cloudflare.CustomHostname, error) { if chName == "" { return cloudflare.CustomHostname{}, fmt.Errorf("failed to get custom hostname: %q is empty", chName) } @@ -647,9 +651,9 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoi } // listDNSRecordsWithAutoPagination performs automatic pagination of results on requests to cloudflare.ListDNSRecords with custom per_page values -func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Context, zoneID string) (map[DNSRecordIndex]cloudflare.DNSRecord, error) { +func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Context, zoneID string) (DNSRecordMap, error) { // for faster getRecordID lookup - recordsMap := make(map[DNSRecordIndex]cloudflare.DNSRecord) + recordsMap := make(DNSRecordMap) resultInfo := cloudflare.ResultInfo{PerPage: p.DNSRecordsPerPage, Page: 1} params := cloudflare.ListDNSRecordsParams{ResultInfo: resultInfo} for { @@ -677,11 +681,11 @@ func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Contex } // listCustomHostnamesWithPagination performs automatic pagination of results on requests to cloudflare.CustomHostnames -func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Context, zoneID string) (map[CustomHostnameIndex]cloudflare.CustomHostname, error) { +func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Context, zoneID string) (CustomHostnameMap, error) { if !p.CustomHostnamesConfig.Enabled { return nil, nil } - chsMap := make(map[CustomHostnameIndex]cloudflare.CustomHostname) + chsMap := make(CustomHostnameMap) resultInfo := cloudflare.ResultInfo{Page: 1} for { pageCustomHostnameListResponse, result, err := p.Client.CustomHostnames(ctx, zoneID, resultInfo.Page, cloudflare.CustomHostname{}) diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index b5a18999f..cb5757a49 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -1031,19 +1031,19 @@ func TestCloudflareApplyChangesError(t *testing.T) { func TestCloudflareGetRecordID(t *testing.T) { p := &CloudFlareProvider{} - recordsMap := map[DNSRecordIndex]cloudflare.DNSRecord{ - DNSRecordIndex{Name: "foo.com", Type: endpoint.RecordTypeCNAME, Content: "foobar"}: { + recordsMap := DNSRecordMap{ + {Name: "foo.com", Type: endpoint.RecordTypeCNAME, Content: "foobar"}: { Name: "foo.com", Type: endpoint.RecordTypeCNAME, Content: "foobar", ID: "1", }, - DNSRecordIndex{Name: "bar.de", Type: endpoint.RecordTypeA}: { + {Name: "bar.de", Type: endpoint.RecordTypeA}: { Name: "bar.de", Type: endpoint.RecordTypeA, ID: "2", }, - DNSRecordIndex{Name: "bar.de", Type: endpoint.RecordTypeA, Content: "1.2.3.4"}: { + {Name: "bar.de", Type: endpoint.RecordTypeA, Content: "1.2.3.4"}: { Name: "bar.de", Type: endpoint.RecordTypeA, Content: "1.2.3.4", From 2e29a19ab70bc3148c76d67baabd016e401fb271 Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Wed, 19 Mar 2025 10:09:37 -0700 Subject: [PATCH 07/16] tidy up using underlying maps for dns records and custom hostnames --- go.mod | 1 - go.sum | 2 - provider/cloudflare/cloudflare.go | 54 ++++++++++++++------------ provider/cloudflare/cloudflare_test.go | 23 ++++++++--- 4 files changed, 47 insertions(+), 33 deletions(-) diff --git a/go.mod b/go.mod index 9091fdb11..a5b3a5ca7 100644 --- a/go.mod +++ b/go.mod @@ -64,7 +64,6 @@ require ( github.com/ultradns/ultradns-sdk-go v1.3.7 go.etcd.io/etcd/client/v3 v3.5.19 go.uber.org/ratelimit v0.3.1 - golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 golang.org/x/net v0.36.0 golang.org/x/oauth2 v0.28.0 golang.org/x/sync v0.12.0 diff --git a/go.sum b/go.sum index 1a79a5705..3ef64734c 100644 --- a/go.sum +++ b/go.sum @@ -1157,8 +1157,6 @@ golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL golang.org/x/exp v0.0.0-20190125153040-c74c464bbbf2/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190306152737-a1d7652674e8/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20191030013958-a1ab85dbe136/go.mod h1:JXzH8nQsPlswgeRAPE3MuO9GYsAcnJvJ4vnMwN/5qkY= -golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 h1:2dVuKD2vS7b0QIHQbpyTISPd0LeHDbnYEryqj5Q1ug8= -golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56/go.mod h1:M4RDyNAINzryxdtnbRXRL/OHtkFuWGRjvuhBJpk2IlY= golang.org/x/image v0.0.0-20180708004352-c73c2afc3b81/go.mod h1:ux5Hcp/YLpHSI86hEcLt0YII63i6oz57MZXIpbrjZUs= golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js= golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0= diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index 554bfe2d7..a0ab2f9ed 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -26,8 +26,6 @@ import ( "strings" "time" - "golang.org/x/exp/maps" - cloudflare "github.com/cloudflare/cloudflare-go" log "github.com/sirupsen/logrus" @@ -64,14 +62,14 @@ type DNSRecordIndex struct { Content string } -type DNSRecordMap map[DNSRecordIndex]cloudflare.DNSRecord +type DNSRecordsMap map[DNSRecordIndex]cloudflare.DNSRecord // for faster getCustomHostname() lookup type CustomHostnameIndex struct { Hostname string } -type CustomHostnameMap map[CustomHostnameIndex]cloudflare.CustomHostname +type CustomHostnamesMap map[CustomHostnameIndex]cloudflare.CustomHostname var recordTypeProxyNotSupported = map[string]bool{ "LOC": true, @@ -321,12 +319,12 @@ func (p *CloudFlareProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, endpoints := []*endpoint.Endpoint{} for _, zone := range zones { - recordsMap, err := p.listDNSRecordsWithAutoPagination(ctx, zone.ID) + records, err := p.listDNSRecordsWithAutoPagination(ctx, zone.ID) if err != nil { return nil, err } - chsMap, chErr := p.listCustomHostnamesWithPagination(ctx, zone.ID) + chs, chErr := p.listCustomHostnamesWithPagination(ctx, zone.ID) if chErr != nil { return nil, chErr } @@ -334,7 +332,7 @@ func (p *CloudFlareProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, // As CloudFlare does not support "sets" of targets, but instead returns // a single entry for each name/type/target, we have to group by name // and record to allow the planner to calculate the correct plan. See #992. - endpoints = append(endpoints, groupByNameAndTypeWithCustomHostnames(maps.Values(recordsMap), maps.Values(chsMap))...) + endpoints = append(endpoints, groupByNameAndTypeWithCustomHostnames(records, chs)...) } return endpoints, nil @@ -378,7 +376,7 @@ func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Cha } // submitCustomHostnameChanges implements Custom Hostname functionality for the Change, returns false if it fails -func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zoneID string, change *cloudFlareChange, chsMap CustomHostnameMap, logFields log.Fields) bool { +func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zoneID string, change *cloudFlareChange, chs CustomHostnamesMap, logFields log.Fields) bool { failedChange := false // return early if disabled if !p.CustomHostnamesConfig.Enabled { @@ -390,7 +388,7 @@ func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zo if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] { prevChName := change.CustomHostnamePrev newChName := change.CustomHostname.Hostname - if prevCh, err := getCustomHostname(chsMap, prevChName); err == nil { + if prevCh, err := getCustomHostname(chs, prevChName); err == nil { prevChID := prevCh.ID if prevChID != "" && prevChName != newChName { log.WithFields(logFields).Infof("Removing previous custom hostname %q/%q", prevChID, prevChName) @@ -415,7 +413,7 @@ func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zo case cloudFlareDelete: if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { log.WithFields(logFields).Infof("Deleting custom hostname %q", change.CustomHostname.Hostname) - if ch, err := getCustomHostname(chsMap, change.CustomHostname.Hostname); err == nil { + if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil { chID := ch.ID chErr := p.Client.DeleteCustomHostname(ctx, zoneID, chID) if chErr != nil { @@ -429,7 +427,7 @@ func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zo case cloudFlareCreate: if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { log.WithFields(logFields).Infof("Creating custom hostname %q", change.CustomHostname.Hostname) - if ch, err := getCustomHostname(chsMap, change.CustomHostname.Hostname); err == nil { + if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil { if change.CustomHostname.CustomOriginServer == ch.CustomOriginServer { log.WithFields(logFields).Warnf("custom hostname %q already exists with the same origin %q, continue", change.CustomHostname.Hostname, ch.CustomOriginServer) } else { @@ -588,18 +586,18 @@ func (p *CloudFlareProvider) changesByZone(zones []cloudflare.Zone, changeSet [] return changes } -func (p *CloudFlareProvider) getRecordID(recordsMap DNSRecordMap, record cloudflare.DNSRecord) string { - if zoneRecord, ok := recordsMap[DNSRecordIndex{Name: record.Name, Type: record.Type, Content: record.Content}]; ok { +func (p *CloudFlareProvider) getRecordID(records DNSRecordsMap, record cloudflare.DNSRecord) string { + if zoneRecord, ok := records[DNSRecordIndex{Name: record.Name, Type: record.Type, Content: record.Content}]; ok { return zoneRecord.ID } return "" } -func getCustomHostname(chsMap CustomHostnameMap, chName string) (cloudflare.CustomHostname, error) { +func getCustomHostname(chs CustomHostnamesMap, chName string) (cloudflare.CustomHostname, error) { if chName == "" { return cloudflare.CustomHostname{}, fmt.Errorf("failed to get custom hostname: %q is empty", chName) } - if ch, ok := chsMap[CustomHostnameIndex{Hostname: chName}]; ok { + if ch, ok := chs[CustomHostnameIndex{Hostname: chName}]; ok { return ch, nil } return cloudflare.CustomHostname{}, fmt.Errorf("failed to get custom hostname: %q not found", chName) @@ -650,10 +648,14 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoi } } +func getDNSRecordIndex(r cloudflare.DNSRecord) DNSRecordIndex { + return DNSRecordIndex{Name: r.Name, Type: r.Type, Content: r.Content} +} + // listDNSRecordsWithAutoPagination performs automatic pagination of results on requests to cloudflare.ListDNSRecords with custom per_page values -func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Context, zoneID string) (DNSRecordMap, error) { +func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Context, zoneID string) (DNSRecordsMap, error) { // for faster getRecordID lookup - recordsMap := make(DNSRecordMap) + records := make(DNSRecordsMap) resultInfo := cloudflare.ResultInfo{PerPage: p.DNSRecordsPerPage, Page: 1} params := cloudflare.ListDNSRecordsParams{ResultInfo: resultInfo} for { @@ -670,22 +672,26 @@ func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Contex } for _, r := range pageRecords { - recordsMap[DNSRecordIndex{Name: r.Name, Type: r.Type, Content: r.Content}] = r + records[getDNSRecordIndex(r)] = r } params.ResultInfo = resultInfo.Next() if params.ResultInfo.Done() { break } } - return recordsMap, nil + return records, nil +} + +func getCustomHostnameIndex(ch cloudflare.CustomHostname) CustomHostnameIndex { + return CustomHostnameIndex{Hostname: ch.Hostname} } // listCustomHostnamesWithPagination performs automatic pagination of results on requests to cloudflare.CustomHostnames -func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Context, zoneID string) (CustomHostnameMap, error) { +func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Context, zoneID string) (CustomHostnamesMap, error) { if !p.CustomHostnamesConfig.Enabled { return nil, nil } - chsMap := make(CustomHostnameMap) + chs := make(CustomHostnamesMap) resultInfo := cloudflare.ResultInfo{Page: 1} for { pageCustomHostnameListResponse, result, err := p.Client.CustomHostnames(ctx, zoneID, resultInfo.Page, cloudflare.CustomHostname{}) @@ -701,14 +707,14 @@ func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Conte return nil, err } for _, ch := range pageCustomHostnameListResponse { - chsMap[CustomHostnameIndex{Hostname: ch.Hostname}] = ch + chs[getCustomHostnameIndex(ch)] = ch } resultInfo = result.Next() if resultInfo.Done() { break } } - return chsMap, nil + return chs, nil } func getCustomHostnamesSSLOptions(customHostnamesConfig CustomHostnamesConfig) *cloudflare.CustomHostnameSSL { @@ -753,7 +759,7 @@ func getEndpointCustomHostname(endpoint *endpoint.Endpoint) string { return "" } -func groupByNameAndTypeWithCustomHostnames(records []cloudflare.DNSRecord, chs []cloudflare.CustomHostname) []*endpoint.Endpoint { +func groupByNameAndTypeWithCustomHostnames(records DNSRecordsMap, chs CustomHostnamesMap) []*endpoint.Endpoint { endpoints := []*endpoint.Endpoint{} // group supported records by name and type diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index cb5757a49..f3861f945 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -21,12 +21,11 @@ import ( "errors" "fmt" "os" + "slices" "sort" "strings" "testing" - "golang.org/x/exp/maps" - cloudflare "github.com/cloudflare/cloudflare-go" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -406,7 +405,7 @@ func getCustomHostnameIdxByID(chs []cloudflare.CustomHostname, customHostnameID return -1 } -func getCustomHostnameIDbyCustomHostnameAndOrigin(chs []cloudflare.CustomHostname, customHostname string, origin string) (string, string) { +func getCustomHostnameIDbyCustomHostnameAndOrigin(chs CustomHostnamesMap, customHostname string, origin string) (string, string) { for _, ch := range chs { if ch.Hostname == customHostname && ch.CustomOriginServer == origin { return ch.ID, ch.Hostname @@ -1031,7 +1030,7 @@ func TestCloudflareApplyChangesError(t *testing.T) { func TestCloudflareGetRecordID(t *testing.T) { p := &CloudFlareProvider{} - recordsMap := DNSRecordMap{ + recordsMap := DNSRecordsMap{ {Name: "foo.com", Type: endpoint.RecordTypeCNAME, Content: "foobar"}: { Name: "foo.com", Type: endpoint.RecordTypeCNAME, @@ -1311,7 +1310,19 @@ func TestCloudflareGroupByNameAndType(t *testing.T) { } for _, tc := range testCases { - assert.ElementsMatch(t, groupByNameAndTypeWithCustomHostnames(tc.Records, []cloudflare.CustomHostname{}), tc.ExpectedEndpoints) + records := make(DNSRecordsMap) + for _, r := range tc.Records { + records[getDNSRecordIndex(r)] = r + } + endpoints := groupByNameAndTypeWithCustomHostnames(records, CustomHostnamesMap{}) + // Targets order could be random with underlying map + for _, ep := range endpoints { + slices.Sort(ep.Targets) + } + for _, ep := range tc.ExpectedEndpoints { + slices.Sort(ep.Targets) + } + assert.ElementsMatch(t, endpoints, tc.ExpectedEndpoints) } } @@ -2270,7 +2281,7 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { } for expectedOrigin, expectedCustomHostname := range tc.ExpectedCustomHostnames { - _, ch := getCustomHostnameIDbyCustomHostnameAndOrigin(maps.Values(chs), expectedCustomHostname, expectedOrigin) + _, ch := getCustomHostnameIDbyCustomHostnameAndOrigin(chs, expectedCustomHostname, expectedOrigin) assert.Equal(t, expectedCustomHostname, ch) } } From 47236fd8bfeb456554aabfb9c5198ead83bc086c Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Wed, 19 Mar 2025 12:01:43 -0700 Subject: [PATCH 08/16] style/naming --- provider/cloudflare/cloudflare.go | 8 ++++---- provider/cloudflare/cloudflare_test.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index a0ab2f9ed..d0e9ba214 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -648,7 +648,7 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoi } } -func getDNSRecordIndex(r cloudflare.DNSRecord) DNSRecordIndex { +func NewDNSRecordIndex(r cloudflare.DNSRecord) DNSRecordIndex { return DNSRecordIndex{Name: r.Name, Type: r.Type, Content: r.Content} } @@ -672,7 +672,7 @@ func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Contex } for _, r := range pageRecords { - records[getDNSRecordIndex(r)] = r + records[NewDNSRecordIndex(r)] = r } params.ResultInfo = resultInfo.Next() if params.ResultInfo.Done() { @@ -682,7 +682,7 @@ func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Contex return records, nil } -func getCustomHostnameIndex(ch cloudflare.CustomHostname) CustomHostnameIndex { +func NewCustomHostnameIndex(ch cloudflare.CustomHostname) CustomHostnameIndex { return CustomHostnameIndex{Hostname: ch.Hostname} } @@ -707,7 +707,7 @@ func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Conte return nil, err } for _, ch := range pageCustomHostnameListResponse { - chs[getCustomHostnameIndex(ch)] = ch + chs[NewCustomHostnameIndex(ch)] = ch } resultInfo = result.Next() if resultInfo.Done() { diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index f3861f945..b42079d62 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -1312,7 +1312,7 @@ func TestCloudflareGroupByNameAndType(t *testing.T) { for _, tc := range testCases { records := make(DNSRecordsMap) for _, r := range tc.Records { - records[getDNSRecordIndex(r)] = r + records[NewDNSRecordIndex(r)] = r } endpoints := groupByNameAndTypeWithCustomHostnames(records, CustomHostnamesMap{}) // Targets order could be random with underlying map From 43b218880ec726f2db06ebb33dfc4c7f6fcee839 Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Wed, 19 Mar 2025 12:08:50 -0700 Subject: [PATCH 09/16] fix private names --- provider/cloudflare/cloudflare.go | 8 ++++---- provider/cloudflare/cloudflare_test.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index d0e9ba214..f43c467cf 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -648,7 +648,7 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoi } } -func NewDNSRecordIndex(r cloudflare.DNSRecord) DNSRecordIndex { +func newDNSRecordIndex(r cloudflare.DNSRecord) DNSRecordIndex { return DNSRecordIndex{Name: r.Name, Type: r.Type, Content: r.Content} } @@ -672,7 +672,7 @@ func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Contex } for _, r := range pageRecords { - records[NewDNSRecordIndex(r)] = r + records[newDNSRecordIndex(r)] = r } params.ResultInfo = resultInfo.Next() if params.ResultInfo.Done() { @@ -682,7 +682,7 @@ func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Contex return records, nil } -func NewCustomHostnameIndex(ch cloudflare.CustomHostname) CustomHostnameIndex { +func newCustomHostnameIndex(ch cloudflare.CustomHostname) CustomHostnameIndex { return CustomHostnameIndex{Hostname: ch.Hostname} } @@ -707,7 +707,7 @@ func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Conte return nil, err } for _, ch := range pageCustomHostnameListResponse { - chs[NewCustomHostnameIndex(ch)] = ch + chs[newCustomHostnameIndex(ch)] = ch } resultInfo = result.Next() if resultInfo.Done() { diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index b42079d62..1c02abd9b 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -1312,7 +1312,7 @@ func TestCloudflareGroupByNameAndType(t *testing.T) { for _, tc := range testCases { records := make(DNSRecordsMap) for _, r := range tc.Records { - records[NewDNSRecordIndex(r)] = r + records[newDNSRecordIndex(r)] = r } endpoints := groupByNameAndTypeWithCustomHostnames(records, CustomHostnamesMap{}) // Targets order could be random with underlying map From d8b31bc2095a257adf66f1242bd61d1e8360e427 Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Fri, 21 Mar 2025 08:17:33 -0700 Subject: [PATCH 10/16] combine unnecessarily separated conditions --- provider/cloudflare/cloudflare.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index f43c467cf..db8a3c132 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -399,14 +399,12 @@ func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zo } } } - if newChName != "" { - if prevChName != newChName { - log.WithFields(logFields).Infof("Adding custom hostname %q", newChName) - _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname) - if chErr != nil { - failedChange = true - log.WithFields(logFields).Errorf("failed to add custom hostname %q: %v", newChName, chErr) - } + if newChName != "" && prevChName != newChName { + log.WithFields(logFields).Infof("Adding custom hostname %q", newChName) + _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname) + if chErr != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to add custom hostname %q: %v", newChName, chErr) } } } From 6da38ba7462112dc41e86069a18cf68950c9c373 Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Sun, 30 Mar 2025 22:21:03 -0700 Subject: [PATCH 11/16] feat(cloudflare): multiple custom hostnames support --- docs/tutorials/cloudflare.md | 2 +- provider/cloudflare/cloudflare.go | 171 ++++++++----- provider/cloudflare/cloudflare_test.go | 333 +++++++++++++++++++------ 3 files changed, 362 insertions(+), 144 deletions(-) diff --git a/docs/tutorials/cloudflare.md b/docs/tutorials/cloudflare.md index fb1eceb7c..79270ad92 100644 --- a/docs/tutorials/cloudflare.md +++ b/docs/tutorials/cloudflare.md @@ -310,7 +310,7 @@ If not set the value will default to `global`. ## Setting cloudflare-custom-hostname -You can automatically configure custom hostnames for A/CNAME DNS records (as custom origins) using the `--cloudflare-custom-hostnames` flag and the `external-dns.alpha.kubernetes.io/cloudflare-custom-hostname: ""` annotation. +Automatic configuration of Cloudflare custom hostnames (using A/CNAME DNS records as custom origin servers) is enabled by the --cloudflare-custom-hostnames flag and the `external-dns.alpha.kubernetes.io/cloudflare-custom-hostname: ` annotation. Multiple hostnames are supported via a comma-separated list: `external-dns.alpha.kubernetes.io/cloudflare-custom-hostname: ,`. See [Cloudflare for Platforms](https://developers.cloudflare.com/cloudflare-for-platforms/cloudflare-for-saas/domain-support/) for more information on custom hostnames. diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index db8a3c132..f0fc39334 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -20,8 +20,11 @@ import ( "context" "errors" "fmt" + "maps" "net/http" "os" + "slices" + "sort" "strconv" "strings" "time" @@ -182,11 +185,11 @@ type CloudFlareProvider struct { // cloudFlareChange differentiates between ChangActions type cloudFlareChange struct { - Action string - ResourceRecord cloudflare.DNSRecord - RegionalHostname cloudflare.RegionalHostname - CustomHostname cloudflare.CustomHostname - CustomHostnamePrev string + Action string + ResourceRecord cloudflare.DNSRecord + RegionalHostname cloudflare.RegionalHostname + CustomHostnames map[string]cloudflare.CustomHostname + CustomHostnamesPrev []string } // RecordParamsTypes is a typeset of the possible Record Params that can be passed to cloudflare-go library @@ -324,6 +327,7 @@ func (p *CloudFlareProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, return nil, err } + // nil if custom hostnames are not enabled chs, chErr := p.listCustomHostnamesWithPagination(ctx, zone.ID) if chErr != nil { return nil, chErr @@ -342,6 +346,15 @@ func (p *CloudFlareProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) error { cloudflareChanges := []*cloudFlareChange{} + // if custom hostnames are enabled, deleting first allows to avoid conflicts with the new ones + if p.CustomHostnamesConfig.Enabled { + for _, endpoint := range changes.Delete { + for _, target := range endpoint.Targets { + cloudflareChanges = append(cloudflareChanges, p.newCloudFlareChange(cloudFlareDelete, endpoint, target, nil)) + } + } + } + for _, endpoint := range changes.Create { for _, target := range endpoint.Targets { cloudflareChanges = append(cloudflareChanges, p.newCloudFlareChange(cloudFlareCreate, endpoint, target, nil)) @@ -366,9 +379,12 @@ func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Cha } } - for _, endpoint := range changes.Delete { - for _, target := range endpoint.Targets { - cloudflareChanges = append(cloudflareChanges, p.newCloudFlareChange(cloudFlareDelete, endpoint, target, nil)) + // TODO: consider deleting before creating even if custom hostnames are not in use + if !p.CustomHostnamesConfig.Enabled { + for _, endpoint := range changes.Delete { + for _, target := range endpoint.Targets { + cloudflareChanges = append(cloudflareChanges, p.newCloudFlareChange(cloudFlareDelete, endpoint, target, nil)) + } } } @@ -386,57 +402,63 @@ func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zo switch change.Action { case cloudFlareUpdate: if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] { - prevChName := change.CustomHostnamePrev - newChName := change.CustomHostname.Hostname - if prevCh, err := getCustomHostname(chs, prevChName); err == nil { - prevChID := prevCh.ID - if prevChID != "" && prevChName != newChName { - log.WithFields(logFields).Infof("Removing previous custom hostname %q/%q", prevChID, prevChName) - chErr := p.Client.DeleteCustomHostname(ctx, zoneID, prevChID) - if chErr != nil { - failedChange = true - log.WithFields(logFields).Errorf("failed to remove previous custom hostname %q/%q: %v", prevChID, prevChName, chErr) + add, remove, _ := provider.Difference(change.CustomHostnamesPrev, slices.Collect(maps.Keys(change.CustomHostnames))) + + for _, changeCH := range remove { + if prevCh, err := getCustomHostname(chs, changeCH); err == nil { + prevChID := prevCh.ID + if prevChID != "" { + log.WithFields(logFields).Infof("Removing previous custom hostname %q/%q", prevChID, changeCH) + chErr := p.Client.DeleteCustomHostname(ctx, zoneID, prevChID) + if chErr != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to remove previous custom hostname %q/%q: %v", prevChID, changeCH, chErr) + } } } } - if newChName != "" && prevChName != newChName { - log.WithFields(logFields).Infof("Adding custom hostname %q", newChName) - _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname) + for _, changeCH := range add { + log.WithFields(logFields).Infof("Adding custom hostname %q", changeCH) + _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostnames[changeCH]) if chErr != nil { failedChange = true - log.WithFields(logFields).Errorf("failed to add custom hostname %q: %v", newChName, chErr) + log.WithFields(logFields).Errorf("failed to add custom hostname %q: %v", changeCH, chErr) } } } case cloudFlareDelete: - if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { - log.WithFields(logFields).Infof("Deleting custom hostname %q", change.CustomHostname.Hostname) - if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil { - chID := ch.ID - chErr := p.Client.DeleteCustomHostname(ctx, zoneID, chID) - if chErr != nil { - failedChange = true - log.WithFields(logFields).Errorf("failed to delete custom hostname %q/%q: %v", chID, change.CustomHostname.Hostname, chErr) + for _, changeCH := range change.CustomHostnames { + if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && changeCH.Hostname != "" { + log.WithFields(logFields).Infof("Deleting custom hostname %q", changeCH.Hostname) + if ch, err := getCustomHostname(chs, changeCH.Hostname); err == nil { + chID := ch.ID + chErr := p.Client.DeleteCustomHostname(ctx, zoneID, chID) + if chErr != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to delete custom hostname %q/%q: %v", chID, changeCH.Hostname, chErr) + } + } else { + log.WithFields(logFields).Warnf("failed to delete custom hostname %q: %v", changeCH.Hostname, err) } - } else { - log.WithFields(logFields).Warnf("failed to delete custom hostname %q: %v", change.CustomHostname.Hostname, err) } } case cloudFlareCreate: - if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" { - log.WithFields(logFields).Infof("Creating custom hostname %q", change.CustomHostname.Hostname) - if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil { - if change.CustomHostname.CustomOriginServer == ch.CustomOriginServer { - log.WithFields(logFields).Warnf("custom hostname %q already exists with the same origin %q, continue", change.CustomHostname.Hostname, ch.CustomOriginServer) + for _, changeCH := range change.CustomHostnames { + if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && changeCH.Hostname != "" { + log.WithFields(logFields).Infof("Creating custom hostname %q", changeCH.Hostname) + if ch, err := getCustomHostname(chs, changeCH.Hostname); err == nil { + if changeCH.CustomOriginServer == ch.CustomOriginServer { + log.WithFields(logFields).Warnf("custom hostname %q already exists with the same origin %q, continue", changeCH.Hostname, ch.CustomOriginServer) + } else { + failedChange = true + log.WithFields(logFields).Errorf("failed to create custom hostname, %q already exists with origin %q", changeCH.Hostname, ch.CustomOriginServer) + } } else { - failedChange = true - log.WithFields(logFields).Errorf("failed to create custom hostname, %q already exists with origin %q", change.CustomHostname.Hostname, ch.CustomOriginServer) - } - } else { - _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname) - if chErr != nil { - failedChange = true - log.WithFields(logFields).Errorf("failed to create custom hostname %q: %v", change.CustomHostname.Hostname, chErr) + _, chErr := p.Client.CreateCustomHostname(ctx, zoneID, changeCH) + if chErr != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to create custom hostname %q: %v", changeCH.Hostname, chErr) + } } } } @@ -460,9 +482,9 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud changesByZone := p.changesByZone(zones, changes) var failedZones []string - for zoneID, changes := range changesByZone { + for zoneID, zoneChanges := range changesByZone { var failedChange bool - for _, change := range changes { + for _, change := range zoneChanges { logFields := log.Fields{ "record": change.ResourceRecord.Name, "type": change.ResourceRecord.Type, @@ -557,6 +579,17 @@ func (p *CloudFlareProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([] } e.SetProviderSpecificProperty(source.CloudflareProxiedKey, strconv.FormatBool(proxied)) + if p.CustomHostnamesConfig.Enabled { + // sort custom hostnames in annotation to properly detect changes + if customHostnames := getEndpointCustomHostnames(e); len(customHostnames) > 1 { + sort.Strings(customHostnames) + e.SetProviderSpecificProperty(source.CloudflareCustomHostnameKey, strings.Join(customHostnames, ",")) + } + } else { + // ignore custom hostnames annotations if not enabled + e.DeleteProviderSpecificProperty(source.CloudflareCustomHostnameKey) + } + adjustedEndpoints = append(adjustedEndpoints, e) } return adjustedEndpoints, nil @@ -601,6 +634,14 @@ func getCustomHostname(chs CustomHostnamesMap, chName string) (cloudflare.Custom return cloudflare.CustomHostname{}, fmt.Errorf("failed to get custom hostname: %q not found", chName) } +func (p *CloudFlareProvider) newCustomHostname(customHostname string, origin string) cloudflare.CustomHostname { + return cloudflare.CustomHostname{ + Hostname: customHostname, + CustomOriginServer: origin, + SSL: getCustomHostnamesSSLOptions(p.CustomHostnamesConfig), + } +} + func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoint.Endpoint, target string, current *endpoint.Endpoint) *cloudFlareChange { ttl := defaultCloudFlareRecordTTL proxied := shouldBeProxied(endpoint, p.proxiedByDefault) @@ -610,16 +651,14 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoi } dt := time.Now() - customHostnamePrev := "" - newCustomHostname := cloudflare.CustomHostname{} + prevCustomHostnames := []string{} + newCustomHostnames := map[string]cloudflare.CustomHostname{} if p.CustomHostnamesConfig.Enabled { if current != nil { - customHostnamePrev = getEndpointCustomHostname(current) + prevCustomHostnames = getEndpointCustomHostnames(current) } - newCustomHostname = cloudflare.CustomHostname{ - Hostname: getEndpointCustomHostname(endpoint), - CustomOriginServer: endpoint.DNSName, - SSL: getCustomHostnamesSSLOptions(p.CustomHostnamesConfig), + for _, v := range getEndpointCustomHostnames(endpoint) { + newCustomHostnames[v] = p.newCustomHostname(v, endpoint.DNSName) } } return &cloudFlareChange{ @@ -641,8 +680,8 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoi RegionKey: p.RegionKey, CreatedOn: &dt, }, - CustomHostnamePrev: customHostnamePrev, - CustomHostname: newCustomHostname, + CustomHostnamesPrev: prevCustomHostnames, + CustomHostnames: newCustomHostnames, } } @@ -748,13 +787,15 @@ func shouldBeProxied(endpoint *endpoint.Endpoint, proxiedByDefault bool) bool { return proxied } -func getEndpointCustomHostname(endpoint *endpoint.Endpoint) string { +func getEndpointCustomHostnames(endpoint *endpoint.Endpoint) []string { for _, v := range endpoint.ProviderSpecific { if v.Name == source.CloudflareCustomHostnameKey { - return v.Value + customHostnames := strings.Split(v.Value, ",") + sort.Strings(customHostnames) + return customHostnames } } - return "" + return []string{} } func groupByNameAndTypeWithCustomHostnames(records DNSRecordsMap, chs CustomHostnamesMap) []*endpoint.Endpoint { @@ -777,11 +818,10 @@ func groupByNameAndTypeWithCustomHostnames(records DNSRecordsMap, chs CustomHost } // map custom origin to custom hostname, custom origin should match to a dns record - customOriginServers := map[string]string{} + customHostnames := map[string][]string{} - // only one latest custom hostname for a dns record would work; noop (chs is empty) if custom hostnames feature is not in use for _, c := range chs { - customOriginServers[c.CustomOriginServer] = c.Hostname + customHostnames[c.CustomOriginServer] = append(customHostnames[c.CustomOriginServer], c.Hostname) } // create single endpoint with all the targets for each name/type @@ -806,9 +846,10 @@ func groupByNameAndTypeWithCustomHostnames(records DNSRecordsMap, chs CustomHost continue } ep = ep.WithProviderSpecific(source.CloudflareProxiedKey, strconv.FormatBool(proxied)) - // noop (customOriginServers is empty) if custom hostnames feature is not in use - if customHostname, ok := customOriginServers[records[0].Name]; ok { - ep = ep.WithProviderSpecific(source.CloudflareCustomHostnameKey, customHostname) + // noop (customHostnames is empty) if custom hostnames feature is not in use + if customHostnames, ok := customHostnames[records[0].Name]; ok { + sort.Strings(customHostnames) + ep = ep.WithProviderSpecific(source.CloudflareCustomHostnameKey, strings.Join(customHostnames, ",")) } endpoints = append(endpoints, ep) diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index 1c02abd9b..5a4884c78 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -405,16 +405,6 @@ func getCustomHostnameIdxByID(chs []cloudflare.CustomHostname, customHostnameID return -1 } -func getCustomHostnameIDbyCustomHostnameAndOrigin(chs CustomHostnamesMap, customHostname string, origin string) (string, string) { - for _, ch := range chs { - if ch.Hostname == customHostname && ch.CustomOriginServer == origin { - return ch.ID, ch.Hostname - - } - } - return "", "" -} - func AssertActions(t *testing.T, provider *CloudFlareProvider, endpoints []*endpoint.Endpoint, actions []MockAction, managedRecords []string, args ...interface{}) { t.Helper() @@ -1801,7 +1791,7 @@ func TestCloudflareDNSRecordsOperationsFail(t *testing.T) { shouldFail: true, }, { - Name: "failing to list DNS record", + Name: "adding failing to list DNS record", Endpoints: []*endpoint.Endpoint{ { DNSName: "newerror-list-1.foo.bar.com", @@ -1811,6 +1801,11 @@ func TestCloudflareDNSRecordsOperationsFail(t *testing.T) { Labels: endpoint.Labels{}, }, }, + shouldFail: false, + }, + { + Name: "causing to list failing to list DNS record", + Endpoints: []*endpoint.Endpoint{}, shouldFail: true, }, { @@ -1860,28 +1855,29 @@ func TestCloudflareDNSRecordsOperationsFail(t *testing.T) { } for _, tc := range testFailCases { - records, err := provider.Records(ctx) - if err != nil { - t.Errorf("should not fail, %s", err) + var tcErr error + + if records, err := provider.Records(ctx); err != nil { + tcErr = err + } else { + if endpoints, err := provider.AdjustEndpoints(tc.Endpoints); err != nil { + tcErr = err + } else { + plan := &plan.Plan{ + Current: records, + Desired: endpoints, + DomainFilter: endpoint.MatchAllDomainFilters{&domainFilter}, + ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, + } + + planned := plan.Calculate() + tcErr = provider.ApplyChanges(context.Background(), planned.Changes) + } } - - endpoints, err := provider.AdjustEndpoints(tc.Endpoints) - - assert.NoError(t, err) - plan := &plan.Plan{ - Current: records, - Desired: endpoints, - DomainFilter: endpoint.MatchAllDomainFilters{&domainFilter}, - ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, - } - - planned := plan.Calculate() - - err = provider.ApplyChanges(context.Background(), planned.Changes) - if err == nil && tc.shouldFail { - t.Errorf("should fail - %q, %v", tc.Name, err) - } else if err != nil && !tc.shouldFail { - t.Errorf("should not fail - %q, %v", tc.Name, err) + if tcErr == nil && tc.shouldFail { + t.Errorf("should fail - %q, %v", tc.Name, tcErr) + } else if tcErr != nil && !tc.shouldFail { + t.Errorf("should not fail - %q, %v", tc.Name, tcErr) } } } @@ -1896,10 +1892,9 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { domainFilter := endpoint.NewDomainFilter([]string{"bar.com"}) testFailCases := []struct { - Name string - Endpoints []*endpoint.Endpoint - ExpectedCustomHostnames map[string]string - shouldFail bool + Name string + Endpoints []*endpoint.Endpoint + shouldFail bool }{ { Name: "failing to create custom hostname on record creation", @@ -1921,7 +1916,7 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { shouldFail: true, }, { - Name: "custom hostname to the same origin", + Name: "same custom hostname to the same origin", Endpoints: []*endpoint.Endpoint{ { DNSName: "origin.foo.bar.com", @@ -1936,12 +1931,6 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { }, }, }, - }, - shouldFail: false, - }, - { - Name: "same custom hostname to the another origin", - Endpoints: []*endpoint.Endpoint{ { DNSName: "another-origin.foo.bar.com", Targets: endpoint.Targets{"3.4.5.6"}, @@ -2034,6 +2023,11 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { }, }, }, + shouldFail: false, + }, + { + Name: "causing to list failing to list custom hostname", + Endpoints: []*endpoint.Endpoint{}, shouldFail: true, }, { @@ -2148,9 +2142,7 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { Labels: endpoint.Labels{}, }, }, - ExpectedCustomHostnames: map[string]string{ - "nocustomhostname.foo.bar.com": "", - }, + ExpectedCustomHostnames: map[string]string{}, }, { Name: "add custom hostname", @@ -2183,8 +2175,7 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { }, }, ExpectedCustomHostnames: map[string]string{ - "a.foo.bar.com": "a.foo.fancybar.com", - "txt.foo.bar.com": "", + "a.foo.fancybar.com": "a.foo.bar.com", }, }, { @@ -2205,13 +2196,79 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { }, }, ExpectedCustomHostnames: map[string]string{ - "a.foo.bar.com": "a2.foo.fancybar.com", + "a2.foo.fancybar.com": "a.foo.bar.com", }, }, { - Name: "delete custom hostname", + Name: "add another unsorted custom hostnames", + Endpoints: []*endpoint.Endpoint{ + { + DNSName: "a.foo.bar.com", + Targets: endpoint.Targets{"1.2.3.4"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-custom-hostname", + Value: "a3.foo.fancybar.com,a4.foo.fancybar.com,a2.foo.fancybar.com", + }, + }, + }, + }, + ExpectedCustomHostnames: map[string]string{ + "a2.foo.fancybar.com": "a.foo.bar.com", + "a3.foo.fancybar.com": "a.foo.bar.com", + "a4.foo.fancybar.com": "a.foo.bar.com", + }, + }, + { + Name: "rename custom hostnames", + Endpoints: []*endpoint.Endpoint{ + { + DNSName: "a.foo.bar.com", + Targets: endpoint.Targets{"1.2.3.4"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-custom-hostname", + Value: "a3.foo.fancybar.com,a44.foo.fancybar.com,a22.foo.fancybar.com", + }, + }, + }, + }, + ExpectedCustomHostnames: map[string]string{ + "a22.foo.fancybar.com": "a.foo.bar.com", + "a3.foo.fancybar.com": "a.foo.bar.com", + "a44.foo.fancybar.com": "a.foo.bar.com", + }, + }, + { + Name: "remove some custom hostnames", + Endpoints: []*endpoint.Endpoint{ + { + DNSName: "a.foo.bar.com", + Targets: endpoint.Targets{"1.2.3.4"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-custom-hostname", + Value: "a3.foo.fancybar.com", + }, + }, + }, + }, + ExpectedCustomHostnames: map[string]string{ + "a3.foo.fancybar.com": "a.foo.bar.com", + }, + }, + { + Name: "delete custom hostnames", Endpoints: []*endpoint.Endpoint{ - { DNSName: "a.foo.bar.com", Targets: endpoint.Targets{"1.2.3.4"}, @@ -2220,35 +2277,33 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { Labels: endpoint.Labels{}, }, }, - ExpectedCustomHostnames: map[string]string{ - "a.foo.bar.com": "", - }, + ExpectedCustomHostnames: map[string]string{}, }, } for _, tc := range testFailCases { - records, err := provider.Records(ctx) - if err != nil { - t.Errorf("should not fail, %v", err) + var tcErr error + + if records, err := provider.Records(ctx); err != nil { + tcErr = err + } else { + if endpoints, err := provider.AdjustEndpoints(tc.Endpoints); err != nil { + tcErr = err + } else { + plan := &plan.Plan{ + Current: records, + Desired: endpoints, + DomainFilter: endpoint.MatchAllDomainFilters{&domainFilter}, + ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME, endpoint.RecordTypeTXT}, + } + planned := plan.Calculate() + tcErr = provider.ApplyChanges(context.Background(), planned.Changes) + } } - - endpoints, err := provider.AdjustEndpoints(tc.Endpoints) - - assert.NoError(t, err) - plan := &plan.Plan{ - Current: records, - Desired: endpoints, - DomainFilter: endpoint.MatchAllDomainFilters{&domainFilter}, - ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME, endpoint.RecordTypeTXT}, - } - - planned := plan.Calculate() - - err = provider.ApplyChanges(context.Background(), planned.Changes) - if err == nil && tc.shouldFail { - t.Errorf("should fail - %q, %v", tc.Name, err) - } else if err != nil && !tc.shouldFail { - t.Errorf("should not fail - %q, %v", tc.Name, err) + if tcErr == nil && tc.shouldFail { + t.Errorf("should fail - %q, %v", tc.Name, tcErr) + } else if tcErr != nil && !tc.shouldFail { + t.Errorf("should not fail - %q, %v", tc.Name, tcErr) } } @@ -2280,9 +2335,131 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { t.Errorf("should not fail - %q, %v", tc.Name, chErr) } - for expectedOrigin, expectedCustomHostname := range tc.ExpectedCustomHostnames { - _, ch := getCustomHostnameIDbyCustomHostnameAndOrigin(chs, expectedCustomHostname, expectedOrigin) - assert.Equal(t, expectedCustomHostname, ch) + actualCustomHostnames := map[string]string{} + for _, ch := range chs { + actualCustomHostnames[ch.Hostname] = ch.CustomOriginServer + } + assert.Equal(t, tc.ExpectedCustomHostnames, actualCustomHostnames, "custom hostnames should be the same") + } +} + +func TestCloudflareDisabledCustomHostnameOperations(t *testing.T) { + client := NewMockCloudFlareClient() + provider := &CloudFlareProvider{ + Client: client, + CustomHostnamesConfig: CustomHostnamesConfig{Enabled: false}, + } + ctx := context.Background() + domainFilter := endpoint.NewDomainFilter([]string{"bar.com"}) + + testCases := []struct { + Name string + Endpoints []*endpoint.Endpoint + testChanges bool + }{ + { + Name: "add custom hostname", + Endpoints: []*endpoint.Endpoint{ + { + DNSName: "a.foo.bar.com", + Targets: endpoint.Targets{"1.2.3.11"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-custom-hostname", + Value: "a.foo.fancybar.com", + }, + }, + }, + { + DNSName: "b.foo.bar.com", + Targets: endpoint.Targets{"1.2.3.12"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + }, + { + DNSName: "c.foo.bar.com", + Targets: endpoint.Targets{"1.2.3.13"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-custom-hostname", + Value: "c1.foo.fancybar.com", + }, + }, + }, + }, + testChanges: false, + }, + { + Name: "add custom hostname", + Endpoints: []*endpoint.Endpoint{ + { + DNSName: "a.foo.bar.com", + Targets: endpoint.Targets{"1.2.3.11"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + }, + { + DNSName: "b.foo.bar.com", + Targets: endpoint.Targets{"1.2.3.12"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-custom-hostname", + Value: "b.foo.fancybar.com", + }, + }, + }, + { + DNSName: "c.foo.bar.com", + Targets: endpoint.Targets{"1.2.3.13"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-custom-hostname", + Value: "c2.foo.fancybar.com", + }, + }, + }, + }, + testChanges: true, + }, + } + + for _, tc := range testCases { + records, err := provider.Records(ctx) + if err != nil { + t.Errorf("should not fail, %v", err) + } + + endpoints, err := provider.AdjustEndpoints(tc.Endpoints) + + assert.NoError(t, err) + plan := &plan.Plan{ + Current: records, + Desired: endpoints, + DomainFilter: endpoint.MatchAllDomainFilters{&domainFilter}, + ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, + } + + planned := plan.Calculate() + err = provider.ApplyChanges(context.Background(), planned.Changes) + if err != nil { + t.Errorf("should not fail - %q, %v", tc.Name, err) + } + if tc.testChanges { + assert.Equal(t, planned.Changes.HasChanges(), false, "no new changes should be here") } } } From 647d850729aed748b27b3333d1dab47fec4cfc87 Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Mon, 31 Mar 2025 09:30:05 -0700 Subject: [PATCH 12/16] remove redunaant custom hostnames sort --- provider/cloudflare/cloudflare.go | 1 - 1 file changed, 1 deletion(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index f0fc39334..2ba2723ff 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -791,7 +791,6 @@ func getEndpointCustomHostnames(endpoint *endpoint.Endpoint) []string { for _, v := range endpoint.ProviderSpecific { if v.Name == source.CloudflareCustomHostnameKey { customHostnames := strings.Split(v.Value, ",") - sort.Strings(customHostnames) return customHostnames } } From 3ef8c7336f57ce8460d505683c37321d9175f443 Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Mon, 31 Mar 2025 12:02:41 -0700 Subject: [PATCH 13/16] rename variables with ambiguous "endpoint" names --- provider/cloudflare/cloudflare.go | 56 +++++++++++++++---------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index 2ba2723ff..f4780adda 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -348,16 +348,16 @@ func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Cha // if custom hostnames are enabled, deleting first allows to avoid conflicts with the new ones if p.CustomHostnamesConfig.Enabled { - for _, endpoint := range changes.Delete { - for _, target := range endpoint.Targets { - cloudflareChanges = append(cloudflareChanges, p.newCloudFlareChange(cloudFlareDelete, endpoint, target, nil)) + for _, e := range changes.Delete { + for _, target := range e.Targets { + cloudflareChanges = append(cloudflareChanges, p.newCloudFlareChange(cloudFlareDelete, e, target, nil)) } } } - for _, endpoint := range changes.Create { - for _, target := range endpoint.Targets { - cloudflareChanges = append(cloudflareChanges, p.newCloudFlareChange(cloudFlareCreate, endpoint, target, nil)) + for _, e := range changes.Create { + for _, target := range e.Targets { + cloudflareChanges = append(cloudflareChanges, p.newCloudFlareChange(cloudFlareCreate, e, target, nil)) } } @@ -381,9 +381,9 @@ func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Cha // TODO: consider deleting before creating even if custom hostnames are not in use if !p.CustomHostnamesConfig.Enabled { - for _, endpoint := range changes.Delete { - for _, target := range endpoint.Targets { - cloudflareChanges = append(cloudflareChanges, p.newCloudFlareChange(cloudFlareDelete, endpoint, target, nil)) + for _, e := range changes.Delete { + for _, target := range e.Targets { + cloudflareChanges = append(cloudflareChanges, p.newCloudFlareChange(cloudFlareDelete, e, target, nil)) } } } @@ -642,12 +642,12 @@ func (p *CloudFlareProvider) newCustomHostname(customHostname string, origin str } } -func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoint.Endpoint, target string, current *endpoint.Endpoint) *cloudFlareChange { +func (p *CloudFlareProvider) newCloudFlareChange(action string, ep *endpoint.Endpoint, target string, current *endpoint.Endpoint) *cloudFlareChange { ttl := defaultCloudFlareRecordTTL - proxied := shouldBeProxied(endpoint, p.proxiedByDefault) + proxied := shouldBeProxied(ep, p.proxiedByDefault) - if endpoint.RecordTTL.IsConfigured() { - ttl = int(endpoint.RecordTTL) + if ep.RecordTTL.IsConfigured() { + ttl = int(ep.RecordTTL) } dt := time.Now() @@ -657,26 +657,26 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoi if current != nil { prevCustomHostnames = getEndpointCustomHostnames(current) } - for _, v := range getEndpointCustomHostnames(endpoint) { - newCustomHostnames[v] = p.newCustomHostname(v, endpoint.DNSName) + for _, v := range getEndpointCustomHostnames(ep) { + newCustomHostnames[v] = p.newCustomHostname(v, ep.DNSName) } } return &cloudFlareChange{ Action: action, ResourceRecord: cloudflare.DNSRecord{ - Name: endpoint.DNSName, + Name: ep.DNSName, TTL: ttl, // We have to use pointers to bools now, as the upstream cloudflare-go library requires them // see: https://github.com/cloudflare/cloudflare-go/pull/595 Proxied: &proxied, - Type: endpoint.RecordType, + Type: ep.RecordType, Content: target, Meta: map[string]interface{}{ "region": p.RegionKey, }, }, RegionalHostname: cloudflare.RegionalHostname{ - Hostname: endpoint.DNSName, + Hostname: ep.DNSName, RegionKey: p.RegionKey, CreatedOn: &dt, }, @@ -766,10 +766,10 @@ func getCustomHostnamesSSLOptions(customHostnamesConfig CustomHostnamesConfig) * } } -func shouldBeProxied(endpoint *endpoint.Endpoint, proxiedByDefault bool) bool { +func shouldBeProxied(ep *endpoint.Endpoint, proxiedByDefault bool) bool { proxied := proxiedByDefault - for _, v := range endpoint.ProviderSpecific { + for _, v := range ep.ProviderSpecific { if v.Name == source.CloudflareProxiedKey { b, err := strconv.ParseBool(v.Value) if err != nil { @@ -781,14 +781,14 @@ func shouldBeProxied(endpoint *endpoint.Endpoint, proxiedByDefault bool) bool { } } - if recordTypeProxyNotSupported[endpoint.RecordType] { + if recordTypeProxyNotSupported[ep.RecordType] { proxied = false } return proxied } -func getEndpointCustomHostnames(endpoint *endpoint.Endpoint) []string { - for _, v := range endpoint.ProviderSpecific { +func getEndpointCustomHostnames(ep *endpoint.Endpoint) []string { + for _, v := range ep.ProviderSpecific { if v.Name == source.CloudflareCustomHostnameKey { customHostnames := strings.Split(v.Value, ",") return customHostnames @@ -832,7 +832,7 @@ func groupByNameAndTypeWithCustomHostnames(records DNSRecordsMap, chs CustomHost for i, record := range records { targets[i] = record.Content } - ep := endpoint.NewEndpointWithTTL( + e := endpoint.NewEndpointWithTTL( records[0].Name, records[0].Type, endpoint.TTL(records[0].TTL), @@ -841,17 +841,17 @@ func groupByNameAndTypeWithCustomHostnames(records DNSRecordsMap, chs CustomHost if records[0].Proxied != nil { proxied = *records[0].Proxied } - if ep == nil { + if e == nil { continue } - ep = ep.WithProviderSpecific(source.CloudflareProxiedKey, strconv.FormatBool(proxied)) + e = e.WithProviderSpecific(source.CloudflareProxiedKey, strconv.FormatBool(proxied)) // noop (customHostnames is empty) if custom hostnames feature is not in use if customHostnames, ok := customHostnames[records[0].Name]; ok { sort.Strings(customHostnames) - ep = ep.WithProviderSpecific(source.CloudflareCustomHostnameKey, strings.Join(customHostnames, ",")) + e = e.WithProviderSpecific(source.CloudflareCustomHostnameKey, strings.Join(customHostnames, ",")) } - endpoints = append(endpoints, ep) + endpoints = append(endpoints, e) } return endpoints From 165506c275e22ef0ca4598043b962d31b8ed3456 Mon Sep 17 00:00:00 2001 From: mrozentsvayg Date: Mon, 31 Mar 2025 15:27:42 -0700 Subject: [PATCH 14/16] Update cloudflare.md split long lines --- docs/tutorials/cloudflare.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/tutorials/cloudflare.md b/docs/tutorials/cloudflare.md index 79270ad92..555096098 100644 --- a/docs/tutorials/cloudflare.md +++ b/docs/tutorials/cloudflare.md @@ -310,7 +310,9 @@ If not set the value will default to `global`. ## Setting cloudflare-custom-hostname -Automatic configuration of Cloudflare custom hostnames (using A/CNAME DNS records as custom origin servers) is enabled by the --cloudflare-custom-hostnames flag and the `external-dns.alpha.kubernetes.io/cloudflare-custom-hostname: ` annotation. Multiple hostnames are supported via a comma-separated list: `external-dns.alpha.kubernetes.io/cloudflare-custom-hostname: ,`. +Automatic configuration of Cloudflare custom hostnames (using A/CNAME DNS records as custom origin servers) is enabled by the --cloudflare-custom-hostnames flag and the `external-dns.alpha.kubernetes.io/cloudflare-custom-hostname: ` annotation. + +Multiple hostnames are supported via a comma-separated list: `external-dns.alpha.kubernetes.io/cloudflare-custom-hostname: ,`. See [Cloudflare for Platforms](https://developers.cloudflare.com/cloudflare-for-platforms/cloudflare-for-saas/domain-support/) for more information on custom hostnames. From f3f58dd676af95097c5698954bdc4d33a7739a05 Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Tue, 1 Apr 2025 12:01:50 -0700 Subject: [PATCH 15/16] [attempt to] improve nested conditions readability for cloudflare tests failures scenarios --- provider/cloudflare/cloudflare_test.go | 111 +++++++++++++------------ 1 file changed, 57 insertions(+), 54 deletions(-) diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index 5a4884c78..5fb6699d0 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -1762,6 +1762,17 @@ func TestCloudflareZoneRecordsFail(t *testing.T) { } } +// check if the error is expected +func checkFailed(name string, err error, shouldFail bool) error { + if errors.Is(err, nil) && shouldFail { + return fmt.Errorf("should fail - %q", name) + } + if !errors.Is(err, nil) && !shouldFail { + return fmt.Errorf("should not fail - %q, %v", name, err) + } + return nil +} + func TestCloudflareDNSRecordsOperationsFail(t *testing.T) { client := NewMockCloudFlareClient() provider := &CloudFlareProvider{ @@ -1855,29 +1866,25 @@ func TestCloudflareDNSRecordsOperationsFail(t *testing.T) { } for _, tc := range testFailCases { - var tcErr error + var err error + var records, endpoints []*endpoint.Endpoint - if records, err := provider.Records(ctx); err != nil { - tcErr = err - } else { - if endpoints, err := provider.AdjustEndpoints(tc.Endpoints); err != nil { - tcErr = err - } else { - plan := &plan.Plan{ - Current: records, - Desired: endpoints, - DomainFilter: endpoint.MatchAllDomainFilters{&domainFilter}, - ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, - } - - planned := plan.Calculate() - tcErr = provider.ApplyChanges(context.Background(), planned.Changes) - } + records, err = provider.Records(ctx) + if errors.Is(err, nil) { + endpoints, err = provider.AdjustEndpoints(tc.Endpoints) } - if tcErr == nil && tc.shouldFail { - t.Errorf("should fail - %q, %v", tc.Name, tcErr) - } else if tcErr != nil && !tc.shouldFail { - t.Errorf("should not fail - %q, %v", tc.Name, tcErr) + if errors.Is(err, nil) { + plan := &plan.Plan{ + Current: records, + Desired: endpoints, + DomainFilter: endpoint.MatchAllDomainFilters{&domainFilter}, + ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, + } + planned := plan.Calculate() + err = provider.ApplyChanges(context.Background(), planned.Changes) + } + if e := checkFailed(tc.Name, err, tc.shouldFail); !errors.Is(e, nil) { + t.Error(e) } } } @@ -2282,28 +2289,26 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { } for _, tc := range testFailCases { - var tcErr error + var err error + var records, endpoints []*endpoint.Endpoint - if records, err := provider.Records(ctx); err != nil { - tcErr = err - } else { - if endpoints, err := provider.AdjustEndpoints(tc.Endpoints); err != nil { - tcErr = err - } else { - plan := &plan.Plan{ - Current: records, - Desired: endpoints, - DomainFilter: endpoint.MatchAllDomainFilters{&domainFilter}, - ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME, endpoint.RecordTypeTXT}, - } - planned := plan.Calculate() - tcErr = provider.ApplyChanges(context.Background(), planned.Changes) - } + records, err = provider.Records(ctx) + if errors.Is(err, nil) { + endpoints, err = provider.AdjustEndpoints(tc.Endpoints) } - if tcErr == nil && tc.shouldFail { - t.Errorf("should fail - %q, %v", tc.Name, tcErr) - } else if tcErr != nil && !tc.shouldFail { - t.Errorf("should not fail - %q, %v", tc.Name, tcErr) + if errors.Is(err, nil) { + plan := &plan.Plan{ + Current: records, + Desired: endpoints, + DomainFilter: endpoint.MatchAllDomainFilters{&domainFilter}, + ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME, endpoint.RecordTypeTXT}, + } + planned := plan.Calculate() + err = provider.ApplyChanges(context.Background(), planned.Changes) + + } + if e := checkFailed(tc.Name, err, tc.shouldFail); !errors.Is(e, nil) { + t.Error(e) } } @@ -2326,13 +2331,13 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { planned := plan.Calculate() err = provider.ApplyChanges(context.Background(), planned.Changes) - if err != nil { - t.Errorf("should not fail - %q, %v", tc.Name, err) + if e := checkFailed(tc.Name, err, false); !errors.Is(e, nil) { + t.Error(e) } chs, chErr := provider.listCustomHostnamesWithPagination(ctx, "001") - if chErr != nil { - t.Errorf("should not fail - %q, %v", tc.Name, chErr) + if e := checkFailed(tc.Name, chErr, false); !errors.Is(e, nil) { + t.Error(e) } actualCustomHostnames := map[string]string{} @@ -2452,11 +2457,10 @@ func TestCloudflareDisabledCustomHostnameOperations(t *testing.T) { DomainFilter: endpoint.MatchAllDomainFilters{&domainFilter}, ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, } - planned := plan.Calculate() err = provider.ApplyChanges(context.Background(), planned.Changes) - if err != nil { - t.Errorf("should not fail - %q, %v", tc.Name, err) + if e := checkFailed(tc.Name, err, false); !errors.Is(e, nil) { + t.Error(e) } if tc.testChanges { assert.Equal(t, planned.Changes.HasChanges(), false, "no new changes should be here") @@ -2558,11 +2562,11 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { // manually corrupt custom hostname before the deletion step // the purpose is to cause getCustomHostnameOrigin() to fail on change.Action == cloudFlareDelete chs, chErr := provider.listCustomHostnamesWithPagination(ctx, zoneID) - if chErr != nil { - t.Errorf("should not fail - %q, %v", tc.Name, chErr) + if e := checkFailed(tc.Name, chErr, false); !errors.Is(e, nil) { + t.Error(e) } if tc.preApplyHook == "corrupt" { - if ch, err := getCustomHostname(chs, "newerror-getCustomHostnameOrigin.foo.fancybar.com"); err == nil { + if ch, err := getCustomHostname(chs, "newerror-getCustomHostnameOrigin.foo.fancybar.com"); errors.Is(err, nil) { chID := ch.ID t.Logf("corrupting custom hostname %q", chID) oldIdx := getCustomHostnameIdxByID(client.customHostnames[zoneID], chID) @@ -2575,7 +2579,6 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { client.customHostnames[zoneID][oldIdx] = ch } } else if tc.preApplyHook == "duplicate" { // manually inject duplicating custom hostname with the same name and origin - ch := cloudflare.CustomHostname{ ID: "ID-random-123", Hostname: "a.foo.fancybar.com", @@ -2584,8 +2587,8 @@ func TestCloudflareCustomHostnameNotFoundOnRecordDeletion(t *testing.T) { client.customHostnames[zoneID] = append(client.customHostnames[zoneID], ch) } err = provider.ApplyChanges(context.Background(), planned.Changes) - if err != nil { - t.Errorf("should not fail - %q, %v", tc.Name, err) + if e := checkFailed(tc.Name, err, false); !errors.Is(e, nil) { + t.Error(e) } assert.Contains(t, b.String(), tc.logOutput) } From 6808aa319848f51a800a0c0c6f42d36207501940 Mon Sep 17 00:00:00 2001 From: Mikhail Rozentsvayg Date: Wed, 2 Apr 2025 10:09:00 -0700 Subject: [PATCH 16/16] add test for logging error when creating endpoint and ignoring too long record name (shouldn't happen) --- .gitignore | 1 + provider/cloudflare/cloudflare_test.go | 27 ++++++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/.gitignore b/.gitignore index 3907ba757..29561b906 100644 --- a/.gitignore +++ b/.gitignore @@ -15,6 +15,7 @@ # Vscode files .vscode +__debug_* # This is where the result of the go build goes /output*/ diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index 5fb6699d0..4f3024bbb 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -1762,6 +1762,33 @@ func TestCloudflareZoneRecordsFail(t *testing.T) { } } +// TestCloudflareLongRecordsErrorLog checks if the error is logged when a record name exceeds 63 characters +// it's not likely to happen in practice, as the Cloudflare API should reject having it +func TestCloudflareLongRecordsErrorLog(t *testing.T) { + client := NewMockCloudFlareClientWithRecords(map[string][]cloudflare.DNSRecord{ + "001": { + { + ID: "1234567890", + Name: "very-very-very-very-very-very-very-long-name-more-than-63-bytes-long.bar.com", + Type: endpoint.RecordTypeTXT, + TTL: 120, + Content: "some-content", + }, + }, + }) + b := testutils.LogsToBuffer(log.InfoLevel, t) + p := &CloudFlareProvider{ + Client: client, + CustomHostnamesConfig: CustomHostnamesConfig{Enabled: true}, + } + ctx := context.Background() + _, err := p.Records(ctx) + if err != nil { + t.Errorf("should not fail - too long record, %s", err) + } + assert.Contains(t, b.String(), "is longer than 63 characters. Cannot create endpoint") +} + // check if the error is expected func checkFailed(name string, err error, shouldFail bool) error { if errors.Is(err, nil) && shouldFail {