Skip to content

Commit 04f6a99

Browse files
mrozentsvaygmrozentsvayg
authored
fix(cloudflare): custom hostnames edge-cases causing duplicates (#5183)
* fix(cloudflare): custom hostnames edge-cases causing duplicates * syntax/style * Use %q log fmt for cloudflare provider code * move custom hostnames related submitChanges() implementation to a separate method submitCustomHostnameChanges(); extend truncated logging * use maps for DNS records getRecordID() and custom hostnames getCustomHostname() for faster lookups * types for records/custom hostnames maps * tidy up using underlying maps for dns records and custom hostnames * style/naming * fix private names * combine unnecessarily separated conditions
1 parent f6d49dd commit 04f6a99

File tree

2 files changed

+285
-125
lines changed

2 files changed

+285
-125
lines changed

‎provider/cloudflare/cloudflare.go

+130-79
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,22 @@ var proxyEnabled *bool = boolPtr(true)
5555
// proxyDisabled is a pointer to a bool false showing the record should not be proxied through cloudflare
5656
var proxyDisabled *bool = boolPtr(false)
5757

58+
// for faster getRecordID() lookup
59+
type DNSRecordIndex struct {
60+
Name string
61+
Type string
62+
Content string
63+
}
64+
65+
type DNSRecordsMap map[DNSRecordIndex]cloudflare.DNSRecord
66+
67+
// for faster getCustomHostname() lookup
68+
type CustomHostnameIndex struct {
69+
Hostname string
70+
}
71+
72+
type CustomHostnamesMap map[CustomHostnameIndex]cloudflare.CustomHostname
73+
5874
var recordTypeProxyNotSupported = map[string]bool{
5975
"LOC": true,
6076
"MX": true,
@@ -229,7 +245,7 @@ func NewCloudFlareProvider(domainFilter endpoint.DomainFilter, zoneIDFilter prov
229245
config, err = cloudflare.New(os.Getenv("CF_API_KEY"), os.Getenv("CF_API_EMAIL"))
230246
}
231247
if err != nil {
232-
return nil, fmt.Errorf("failed to initialize cloudflare provider: %v", err)
248+
return nil, fmt.Errorf("failed to initialize cloudflare provider: %w", err)
233249
}
234250
provider := &CloudFlareProvider{
235251
// Client: config,
@@ -254,10 +270,10 @@ func (p *CloudFlareProvider) Zones(ctx context.Context) ([]cloudflare.Zone, erro
254270
if len(p.zoneIDFilter.ZoneIDs) > 0 && p.zoneIDFilter.ZoneIDs[0] != "" {
255271
log.Debugln("zoneIDFilter configured. only looking up zone IDs defined")
256272
for _, zoneID := range p.zoneIDFilter.ZoneIDs {
257-
log.Debugf("looking up zone %s", zoneID)
273+
log.Debugf("looking up zone %q", zoneID)
258274
detailResponse, err := p.Client.ZoneDetails(ctx, zoneID)
259275
if err != nil {
260-
log.Errorf("zone %s lookup failed, %v", zoneID, err)
276+
log.Errorf("zone %q lookup failed, %v", zoneID, err)
261277
return result, err
262278
}
263279
log.WithFields(log.Fields{
@@ -285,7 +301,7 @@ func (p *CloudFlareProvider) Zones(ctx context.Context) ([]cloudflare.Zone, erro
285301

286302
for _, zone := range zonesResponse.Result {
287303
if !p.domainFilter.Match(zone.Name) {
288-
log.Debugf("zone %s not in domain filter", zone.Name)
304+
log.Debugf("zone %q not in domain filter", zone.Name)
289305
continue
290306
}
291307
result = append(result, zone)
@@ -359,6 +375,75 @@ func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Cha
359375
return p.submitChanges(ctx, cloudflareChanges)
360376
}
361377

378+
// submitCustomHostnameChanges implements Custom Hostname functionality for the Change, returns false if it fails
379+
func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zoneID string, change *cloudFlareChange, chs CustomHostnamesMap, logFields log.Fields) bool {
380+
failedChange := false
381+
// return early if disabled
382+
if !p.CustomHostnamesConfig.Enabled {
383+
return !failedChange
384+
}
385+
386+
switch change.Action {
387+
case cloudFlareUpdate:
388+
if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] {
389+
prevChName := change.CustomHostnamePrev
390+
newChName := change.CustomHostname.Hostname
391+
if prevCh, err := getCustomHostname(chs, prevChName); err == nil {
392+
prevChID := prevCh.ID
393+
if prevChID != "" && prevChName != newChName {
394+
log.WithFields(logFields).Infof("Removing previous custom hostname %q/%q", prevChID, prevChName)
395+
chErr := p.Client.DeleteCustomHostname(ctx, zoneID, prevChID)
396+
if chErr != nil {
397+
failedChange = true
398+
log.WithFields(logFields).Errorf("failed to remove previous custom hostname %q/%q: %v", prevChID, prevChName, chErr)
399+
}
400+
}
401+
}
402+
if newChName != "" && prevChName != newChName {
403+
log.WithFields(logFields).Infof("Adding custom hostname %q", newChName)
404+
_, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname)
405+
if chErr != nil {
406+
failedChange = true
407+
log.WithFields(logFields).Errorf("failed to add custom hostname %q: %v", newChName, chErr)
408+
}
409+
}
410+
}
411+
case cloudFlareDelete:
412+
if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" {
413+
log.WithFields(logFields).Infof("Deleting custom hostname %q", change.CustomHostname.Hostname)
414+
if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil {
415+
chID := ch.ID
416+
chErr := p.Client.DeleteCustomHostname(ctx, zoneID, chID)
417+
if chErr != nil {
418+
failedChange = true
419+
log.WithFields(logFields).Errorf("failed to delete custom hostname %q/%q: %v", chID, change.CustomHostname.Hostname, chErr)
420+
}
421+
} else {
422+
log.WithFields(logFields).Warnf("failed to delete custom hostname %q: %v", change.CustomHostname.Hostname, err)
423+
}
424+
}
425+
case cloudFlareCreate:
426+
if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] && change.CustomHostname.Hostname != "" {
427+
log.WithFields(logFields).Infof("Creating custom hostname %q", change.CustomHostname.Hostname)
428+
if ch, err := getCustomHostname(chs, change.CustomHostname.Hostname); err == nil {
429+
if change.CustomHostname.CustomOriginServer == ch.CustomOriginServer {
430+
log.WithFields(logFields).Warnf("custom hostname %q already exists with the same origin %q, continue", change.CustomHostname.Hostname, ch.CustomOriginServer)
431+
} else {
432+
failedChange = true
433+
log.WithFields(logFields).Errorf("failed to create custom hostname, %q already exists with origin %q", change.CustomHostname.Hostname, ch.CustomOriginServer)
434+
}
435+
} else {
436+
_, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname)
437+
if chErr != nil {
438+
failedChange = true
439+
log.WithFields(logFields).Errorf("failed to create custom hostname %q: %v", change.CustomHostname.Hostname, chErr)
440+
}
441+
}
442+
}
443+
}
444+
return !failedChange
445+
}
446+
362447
// submitChanges takes a zone and a collection of Changes and sends them as a single transaction.
363448
func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloudFlareChange) error {
364449
// return early if there is nothing to change
@@ -395,37 +480,15 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud
395480
resourceContainer := cloudflare.ZoneIdentifier(zoneID)
396481
records, err := p.listDNSRecordsWithAutoPagination(ctx, zoneID)
397482
if err != nil {
398-
return fmt.Errorf("could not fetch records from zone, %v", err)
483+
return fmt.Errorf("could not fetch records from zone, %w", err)
399484
}
400485
chs, chErr := p.listCustomHostnamesWithPagination(ctx, zoneID)
401486
if chErr != nil {
402487
return fmt.Errorf("could not fetch custom hostnames from zone, %v", chErr)
403488
}
404489
if change.Action == cloudFlareUpdate {
405-
if recordTypeCustomHostnameSupported[change.ResourceRecord.Type] {
406-
prevCh := change.CustomHostnamePrev
407-
newCh := change.CustomHostname.Hostname
408-
if prevCh != "" {
409-
prevChID, _ := p.getCustomHostnameOrigin(chs, prevCh)
410-
if prevChID != "" && prevCh != newCh {
411-
log.WithFields(logFields).Infof("Removing previous custom hostname %v/%v", prevChID, prevCh)
412-
chErr := p.Client.DeleteCustomHostname(ctx, zoneID, prevChID)
413-
if chErr != nil {
414-
failedChange = true
415-
log.WithFields(logFields).Errorf("failed to remove previous custom hostname %v/%v: %v", prevChID, prevCh, chErr)
416-
}
417-
}
418-
}
419-
if newCh != "" {
420-
if prevCh != newCh {
421-
log.WithFields(logFields).Infof("Adding custom hostname %v", newCh)
422-
_, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname)
423-
if chErr != nil {
424-
failedChange = true
425-
log.WithFields(logFields).Errorf("failed to add custom hostname %v: %v", newCh, chErr)
426-
}
427-
}
428-
}
490+
if !p.submitCustomHostnameChanges(ctx, zoneID, change, chs, logFields) {
491+
failedChange = true
429492
}
430493
recordID := p.getRecordID(records, change.ResourceRecord)
431494
if recordID == "" {
@@ -457,19 +520,8 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud
457520
failedChange = true
458521
log.WithFields(logFields).Errorf("failed to delete record: %v", err)
459522
}
460-
if change.CustomHostname.Hostname == "" {
461-
continue
462-
}
463-
log.WithFields(logFields).Infof("Deleting custom hostname %v", change.CustomHostname.Hostname)
464-
chID, _ := p.getCustomHostnameOrigin(chs, change.CustomHostname.Hostname)
465-
if chID == "" {
466-
log.WithFields(logFields).Infof("Custom hostname %v not found", change.CustomHostname.Hostname)
467-
continue
468-
}
469-
chErr := p.Client.DeleteCustomHostname(ctx, zoneID, chID)
470-
if chErr != nil {
523+
if !p.submitCustomHostnameChanges(ctx, zoneID, change, chs, logFields) {
471524
failedChange = true
472-
log.WithFields(logFields).Errorf("failed to delete custom hostname %v/%v: %v", chID, change.CustomHostname.Hostname, chErr)
473525
}
474526
} else if change.Action == cloudFlareCreate {
475527
recordParam := getCreateDNSRecordParam(*change)
@@ -478,20 +530,8 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud
478530
failedChange = true
479531
log.WithFields(logFields).Errorf("failed to create record: %v", err)
480532
}
481-
if change.CustomHostname.Hostname == "" {
482-
continue
483-
}
484-
log.WithFields(logFields).Infof("Creating custom hostname %v", change.CustomHostname.Hostname)
485-
chID, chOrigin := p.getCustomHostnameOrigin(chs, change.CustomHostname.Hostname)
486-
if chID != "" {
533+
if !p.submitCustomHostnameChanges(ctx, zoneID, change, chs, logFields) {
487534
failedChange = true
488-
log.WithFields(logFields).Errorf("failed to create custom hostname, %v already exists for origin %v", change.CustomHostname.Hostname, chOrigin)
489-
continue
490-
}
491-
_, chErr := p.Client.CreateCustomHostname(ctx, zoneID, change.CustomHostname)
492-
if chErr != nil {
493-
failedChange = true
494-
log.WithFields(logFields).Errorf("failed to create custom hostname %v: %v", change.CustomHostname.Hostname, chErr)
495535
}
496536
}
497537
}
@@ -501,7 +541,7 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud
501541
}
502542

503543
if len(failedZones) > 0 {
504-
return fmt.Errorf("failed to submit all changes for the following zones: %v", failedZones)
544+
return fmt.Errorf("failed to submit all changes for the following zones: %q", failedZones)
505545
}
506546

507547
return nil
@@ -535,7 +575,7 @@ func (p *CloudFlareProvider) changesByZone(zones []cloudflare.Zone, changeSet []
535575
for _, c := range changeSet {
536576
zoneID, _ := zoneNameIDMapper.FindZone(c.ResourceRecord.Name)
537577
if zoneID == "" {
538-
log.Debugf("Skipping record %s because no hosted zone matching record DNS Name was detected", c.ResourceRecord.Name)
578+
log.Debugf("Skipping record %q because no hosted zone matching record DNS Name was detected", c.ResourceRecord.Name)
539579
continue
540580
}
541581
changes[zoneID] = append(changes[zoneID], c)
@@ -544,22 +584,21 @@ func (p *CloudFlareProvider) changesByZone(zones []cloudflare.Zone, changeSet []
544584
return changes
545585
}
546586

547-
func (p *CloudFlareProvider) getRecordID(records []cloudflare.DNSRecord, record cloudflare.DNSRecord) string {
548-
for _, zoneRecord := range records {
549-
if zoneRecord.Name == record.Name && zoneRecord.Type == record.Type && zoneRecord.Content == record.Content {
550-
return zoneRecord.ID
551-
}
587+
func (p *CloudFlareProvider) getRecordID(records DNSRecordsMap, record cloudflare.DNSRecord) string {
588+
if zoneRecord, ok := records[DNSRecordIndex{Name: record.Name, Type: record.Type, Content: record.Content}]; ok {
589+
return zoneRecord.ID
552590
}
553591
return ""
554592
}
555593

556-
func (p *CloudFlareProvider) getCustomHostnameOrigin(chs []cloudflare.CustomHostname, hostname string) (string, string) {
557-
for _, zoneCh := range chs {
558-
if zoneCh.Hostname == hostname {
559-
return zoneCh.ID, zoneCh.CustomOriginServer
560-
}
594+
func getCustomHostname(chs CustomHostnamesMap, chName string) (cloudflare.CustomHostname, error) {
595+
if chName == "" {
596+
return cloudflare.CustomHostname{}, fmt.Errorf("failed to get custom hostname: %q is empty", chName)
597+
}
598+
if ch, ok := chs[CustomHostnameIndex{Hostname: chName}]; ok {
599+
return ch, nil
561600
}
562-
return "", ""
601+
return cloudflare.CustomHostname{}, fmt.Errorf("failed to get custom hostname: %q not found", chName)
563602
}
564603

565604
func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoint.Endpoint, target string, current *endpoint.Endpoint) *cloudFlareChange {
@@ -580,7 +619,7 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoi
580619
newCustomHostname = cloudflare.CustomHostname{
581620
Hostname: getEndpointCustomHostname(endpoint),
582621
CustomOriginServer: endpoint.DNSName,
583-
SSL: getCustomHostnamesSSLOptions(endpoint, p.CustomHostnamesConfig),
622+
SSL: getCustomHostnamesSSLOptions(p.CustomHostnamesConfig),
584623
}
585624
}
586625
return &cloudFlareChange{
@@ -607,9 +646,14 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoi
607646
}
608647
}
609648

649+
func newDNSRecordIndex(r cloudflare.DNSRecord) DNSRecordIndex {
650+
return DNSRecordIndex{Name: r.Name, Type: r.Type, Content: r.Content}
651+
}
652+
610653
// listDNSRecordsWithAutoPagination performs automatic pagination of results on requests to cloudflare.ListDNSRecords with custom per_page values
611-
func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Context, zoneID string) ([]cloudflare.DNSRecord, error) {
612-
var records []cloudflare.DNSRecord
654+
func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Context, zoneID string) (DNSRecordsMap, error) {
655+
// for faster getRecordID lookup
656+
records := make(DNSRecordsMap)
613657
resultInfo := cloudflare.ResultInfo{PerPage: p.DNSRecordsPerPage, Page: 1}
614658
params := cloudflare.ListDNSRecordsParams{ResultInfo: resultInfo}
615659
for {
@@ -625,7 +669,9 @@ func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Contex
625669
return nil, err
626670
}
627671

628-
records = append(records, pageRecords...)
672+
for _, r := range pageRecords {
673+
records[newDNSRecordIndex(r)] = r
674+
}
629675
params.ResultInfo = resultInfo.Next()
630676
if params.ResultInfo.Done() {
631677
break
@@ -634,12 +680,16 @@ func (p *CloudFlareProvider) listDNSRecordsWithAutoPagination(ctx context.Contex
634680
return records, nil
635681
}
636682

683+
func newCustomHostnameIndex(ch cloudflare.CustomHostname) CustomHostnameIndex {
684+
return CustomHostnameIndex{Hostname: ch.Hostname}
685+
}
686+
637687
// listCustomHostnamesWithPagination performs automatic pagination of results on requests to cloudflare.CustomHostnames
638-
func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Context, zoneID string) ([]cloudflare.CustomHostname, error) {
688+
func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Context, zoneID string) (CustomHostnamesMap, error) {
639689
if !p.CustomHostnamesConfig.Enabled {
640690
return nil, nil
641691
}
642-
var chs []cloudflare.CustomHostname
692+
chs := make(CustomHostnamesMap)
643693
resultInfo := cloudflare.ResultInfo{Page: 1}
644694
for {
645695
pageCustomHostnameListResponse, result, err := p.Client.CustomHostnames(ctx, zoneID, resultInfo.Page, cloudflare.CustomHostname{})
@@ -651,11 +701,12 @@ func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Conte
651701
return nil, provider.NewSoftError(err)
652702
}
653703
}
654-
log.Errorf("zone %s failed to fetch custom hostnames. Please check if \"Cloudflare for SaaS\" is enabled and API key permissions, %v", zoneID, err)
704+
log.Errorf("zone %q failed to fetch custom hostnames. Please check if \"Cloudflare for SaaS\" is enabled and API key permissions, %v", zoneID, err)
655705
return nil, err
656706
}
657-
658-
chs = append(chs, pageCustomHostnameListResponse...)
707+
for _, ch := range pageCustomHostnameListResponse {
708+
chs[newCustomHostnameIndex(ch)] = ch
709+
}
659710
resultInfo = result.Next()
660711
if resultInfo.Done() {
661712
break
@@ -664,7 +715,7 @@ func (p *CloudFlareProvider) listCustomHostnamesWithPagination(ctx context.Conte
664715
return chs, nil
665716
}
666717

667-
func getCustomHostnamesSSLOptions(endpoint *endpoint.Endpoint, customHostnamesConfig CustomHostnamesConfig) *cloudflare.CustomHostnameSSL {
718+
func getCustomHostnamesSSLOptions(customHostnamesConfig CustomHostnamesConfig) *cloudflare.CustomHostnameSSL {
668719
return &cloudflare.CustomHostnameSSL{
669720
Type: "dv",
670721
Method: "http",
@@ -683,7 +734,7 @@ func shouldBeProxied(endpoint *endpoint.Endpoint, proxiedByDefault bool) bool {
683734
if v.Name == source.CloudflareProxiedKey {
684735
b, err := strconv.ParseBool(v.Value)
685736
if err != nil {
686-
log.Errorf("Failed to parse annotation [%s]: %v", source.CloudflareProxiedKey, err)
737+
log.Errorf("Failed to parse annotation [%q]: %v", source.CloudflareProxiedKey, err)
687738
} else {
688739
proxied = b
689740
}
@@ -706,7 +757,7 @@ func getEndpointCustomHostname(endpoint *endpoint.Endpoint) string {
706757
return ""
707758
}
708759

709-
func groupByNameAndTypeWithCustomHostnames(records []cloudflare.DNSRecord, chs []cloudflare.CustomHostname) []*endpoint.Endpoint {
760+
func groupByNameAndTypeWithCustomHostnames(records DNSRecordsMap, chs CustomHostnamesMap) []*endpoint.Endpoint {
710761
endpoints := []*endpoint.Endpoint{}
711762

712763
// group supported records by name and type

0 commit comments

Comments
 (0)