From 79be164bea1c332203ea115eb23d0e0d99411d62 Mon Sep 17 00:00:00 2001 From: Valentin Flaux <38909103+vflaux@users.noreply.github.com> Date: Fri, 25 Apr 2025 13:27:24 +0200 Subject: [PATCH 1/2] cloudflare: move regional logic to dedicated file --- provider/cloudflare/cloudflare.go | 186 +-------- provider/cloudflare/cloudflare_regional.go | 210 ++++++++++ .../cloudflare/cloudflare_regional_test.go | 373 ++++++++++++++++++ provider/cloudflare/cloudflare_test.go | 334 +--------------- 4 files changed, 585 insertions(+), 518 deletions(-) create mode 100644 provider/cloudflare/cloudflare_regional.go create mode 100644 provider/cloudflare/cloudflare_regional_test.go diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index 29dd800097..dbbbbc0290 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -78,11 +78,6 @@ type CustomHostnameIndex struct { type CustomHostnamesMap map[CustomHostnameIndex]cloudflare.CustomHostname -type DataLocalizationRegionalHostnameChange struct { - Action string - cloudflare.RegionalHostname -} - var recordTypeProxyNotSupported = map[string]bool{ "LOC": true, "MX": true, @@ -103,12 +98,6 @@ var recordTypeCustomHostnameSupported = map[string]bool{ "CNAME": true, } -var recordTypeRegionalHostnameSupported = map[string]bool{ - "A": true, - "AAAA": true, - "CNAME": true, -} - // cloudFlareDNS is the subset of the CloudFlare API that we actually use. Add methods as required. Signatures must match exactly. type cloudFlareDNS interface { UserDetails(ctx context.Context) (cloudflare.User, error) @@ -157,20 +146,6 @@ func (z zoneService) UpdateDNSRecord(ctx context.Context, rc *cloudflare.Resourc return err } -func (z zoneService) CreateDataLocalizationRegionalHostname(ctx context.Context, rc *cloudflare.ResourceContainer, rp cloudflare.CreateDataLocalizationRegionalHostnameParams) error { - _, err := z.service.CreateDataLocalizationRegionalHostname(ctx, rc, rp) - return err -} - -func (z zoneService) UpdateDataLocalizationRegionalHostname(ctx context.Context, rc *cloudflare.ResourceContainer, rp cloudflare.UpdateDataLocalizationRegionalHostnameParams) error { - _, err := z.service.UpdateDataLocalizationRegionalHostname(ctx, rc, rp) - return err -} - -func (z zoneService) DeleteDataLocalizationRegionalHostname(ctx context.Context, rc *cloudflare.ResourceContainer, hostname string) error { - return z.service.DeleteDataLocalizationRegionalHostname(ctx, rc, hostname) -} - func (z zoneService) DeleteDNSRecord(ctx context.Context, rc *cloudflare.ResourceContainer, recordID string) error { return z.service.DeleteDNSRecord(ctx, rc, recordID) } @@ -273,22 +248,6 @@ func updateDNSRecordParam(cfc cloudFlareChange) cloudflare.UpdateDNSRecordParams } } -// createDataLocalizationRegionalHostnameParams is a function that returns the appropriate RegionalHostname Param based on the cloudFlareChange passed in -func createDataLocalizationRegionalHostnameParams(rhc DataLocalizationRegionalHostnameChange) cloudflare.CreateDataLocalizationRegionalHostnameParams { - return cloudflare.CreateDataLocalizationRegionalHostnameParams{ - Hostname: rhc.Hostname, - RegionKey: rhc.RegionKey, - } -} - -// updateDataLocalizationRegionalHostnameParams is a function that returns the appropriate RegionalHostname Param based on the cloudFlareChange passed in -func updateDataLocalizationRegionalHostnameParams(rhc DataLocalizationRegionalHostnameChange) cloudflare.UpdateDataLocalizationRegionalHostnameParams { - return cloudflare.UpdateDataLocalizationRegionalHostnameParams{ - Hostname: rhc.Hostname, - RegionKey: rhc.RegionKey, - } -} - // getCreateDNSRecordParam is a function that returns the appropriate Record Param based on the cloudFlareChange passed in func getCreateDNSRecordParam(cfc cloudFlareChange) cloudflare.CreateDNSRecordParams { return cloudflare.CreateDNSRecordParams{ @@ -538,129 +497,6 @@ func (p *CloudFlareProvider) submitCustomHostnameChanges(ctx context.Context, zo return !failedChange } -// submitDataLocalizationRegionalHostnameChanges applies a set of data localization regional hostname changes, returns false if it fails -func (p *CloudFlareProvider) submitDataLocalizationRegionalHostnameChanges(ctx context.Context, changes []DataLocalizationRegionalHostnameChange, resourceContainer *cloudflare.ResourceContainer) bool { - failedChange := false - - for _, change := range changes { - logFields := log.Fields{ - "hostname": change.Hostname, - "region_key": change.RegionKey, - "action": change.Action, - "zone": resourceContainer.Identifier, - } - log.WithFields(logFields).Info("Changing regional hostname") - switch change.Action { - case cloudFlareCreate: - log.WithFields(logFields).Debug("Creating regional hostname") - if p.DryRun { - continue - } - regionalHostnameParam := createDataLocalizationRegionalHostnameParams(change) - err := p.Client.CreateDataLocalizationRegionalHostname(ctx, resourceContainer, regionalHostnameParam) - if err != nil { - var apiErr *cloudflare.Error - if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusConflict { - log.WithFields(logFields).Debug("Regional hostname already exists, updating instead") - params := updateDataLocalizationRegionalHostnameParams(change) - err := p.Client.UpdateDataLocalizationRegionalHostname(ctx, resourceContainer, params) - if err != nil { - failedChange = true - log.WithFields(logFields).Errorf("failed to update regional hostname: %v", err) - } - continue - } - failedChange = true - log.WithFields(logFields).Errorf("failed to create regional hostname: %v", err) - } - case cloudFlareUpdate: - log.WithFields(logFields).Debug("Updating regional hostname") - if p.DryRun { - continue - } - regionalHostnameParam := updateDataLocalizationRegionalHostnameParams(change) - err := p.Client.UpdateDataLocalizationRegionalHostname(ctx, resourceContainer, regionalHostnameParam) - if err != nil { - var apiErr *cloudflare.Error - if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound { - log.WithFields(logFields).Debug("Regional hostname not does not exists, creating instead") - params := createDataLocalizationRegionalHostnameParams(change) - err := p.Client.CreateDataLocalizationRegionalHostname(ctx, resourceContainer, params) - if err != nil { - failedChange = true - log.WithFields(logFields).Errorf("failed to create regional hostname: %v", err) - } - continue - } - failedChange = true - log.WithFields(logFields).Errorf("failed to update regional hostname: %v", err) - } - case cloudFlareDelete: - log.WithFields(logFields).Debug("Deleting regional hostname") - if p.DryRun { - continue - } - err := p.Client.DeleteDataLocalizationRegionalHostname(ctx, resourceContainer, change.Hostname) - if err != nil { - var apiErr *cloudflare.Error - if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound { - log.WithFields(logFields).Debug("Regional hostname does not exists, nothing to do") - continue - } - failedChange = true - log.WithFields(logFields).Errorf("failed to delete regional hostname: %v", err) - } - } - } - - return !failedChange -} - -// dataLocalizationRegionalHostnamesChanges processes a slice of cloudFlare changes and consolidates them -// into a list of data localization regional hostname changes. -// returns nil if no changes are needed -func dataLocalizationRegionalHostnamesChanges(changes []*cloudFlareChange) ([]DataLocalizationRegionalHostnameChange, error) { - regionalHostnameChanges := make(map[string]DataLocalizationRegionalHostnameChange) - for _, change := range changes { - if change.RegionalHostname.Hostname == "" { - continue - } - if change.RegionalHostname.RegionKey == "" { - return nil, fmt.Errorf("region key is empty for regional hostname %q", change.RegionalHostname.Hostname) - } - regionalHostname, ok := regionalHostnameChanges[change.RegionalHostname.Hostname] - switch change.Action { - case cloudFlareCreate, cloudFlareUpdate: - if !ok { - regionalHostnameChanges[change.RegionalHostname.Hostname] = DataLocalizationRegionalHostnameChange{ - Action: change.Action, - RegionalHostname: change.RegionalHostname, - } - continue - } - if regionalHostname.RegionKey != change.RegionalHostname.RegionKey { - return nil, fmt.Errorf("conflicting region keys for regional hostname %q: %q and %q", change.RegionalHostname.Hostname, regionalHostname.RegionKey, change.RegionalHostname.RegionKey) - } - if (change.Action == cloudFlareUpdate && regionalHostname.Action != cloudFlareUpdate) || - regionalHostname.Action == cloudFlareDelete { - regionalHostnameChanges[change.RegionalHostname.Hostname] = DataLocalizationRegionalHostnameChange{ - Action: cloudFlareUpdate, - RegionalHostname: change.RegionalHostname, - } - } - case cloudFlareDelete: - if !ok { - regionalHostnameChanges[change.RegionalHostname.Hostname] = DataLocalizationRegionalHostnameChange{ - Action: cloudFlareDelete, - RegionalHostname: change.RegionalHostname, - } - continue - } - } - } - return slices.Collect(maps.Values(regionalHostnameChanges)), nil -} - // 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 @@ -862,13 +698,6 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, ep *endpoint.End newCustomHostnames[v] = p.newCustomHostname(v, ep.DNSName) } } - regionalHostname := cloudflare.RegionalHostname{} - if regionKey := getRegionKey(ep, p.RegionKey); regionKey != "" { - regionalHostname = cloudflare.RegionalHostname{ - Hostname: ep.DNSName, - RegionKey: regionKey, - } - } // Load comment from program flag comment := p.DNSRecordsConfig.Comment @@ -893,7 +722,7 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, ep *endpoint.End Content: target, Comment: comment, }, - RegionalHostname: regionalHostname, + RegionalHostname: p.regionalHostname(ep), CustomHostnamesPrev: prevCustomHostnames, CustomHostnames: newCustomHostnames, } @@ -1001,19 +830,6 @@ func shouldBeProxied(ep *endpoint.Endpoint, proxiedByDefault bool) bool { return proxied } -func getRegionKey(endpoint *endpoint.Endpoint, defaultRegionKey string) string { - if !recordTypeRegionalHostnameSupported[endpoint.RecordType] { - return "" - } - - for _, v := range endpoint.ProviderSpecific { - if v.Name == annotations.CloudflareRegionKey { - return v.Value - } - } - return defaultRegionKey -} - func getEndpointCustomHostnames(ep *endpoint.Endpoint) []string { for _, v := range ep.ProviderSpecific { if v.Name == annotations.CloudflareCustomHostnameKey { diff --git a/provider/cloudflare/cloudflare_regional.go b/provider/cloudflare/cloudflare_regional.go new file mode 100644 index 0000000000..091ea2e95e --- /dev/null +++ b/provider/cloudflare/cloudflare_regional.go @@ -0,0 +1,210 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package cloudflare + +import ( + "context" + "errors" + "fmt" + "maps" + "net/http" + "slices" + + "github.com/cloudflare/cloudflare-go" + log "github.com/sirupsen/logrus" + + "sigs.k8s.io/external-dns/endpoint" + "sigs.k8s.io/external-dns/source/annotations" +) + +var recordTypeRegionalHostnameSupported = map[string]bool{ + "A": true, + "AAAA": true, + "CNAME": true, +} + +type regionalHostnameChange struct { + action string + cloudflare.RegionalHostname +} + +func (z zoneService) CreateDataLocalizationRegionalHostname(ctx context.Context, rc *cloudflare.ResourceContainer, rp cloudflare.CreateDataLocalizationRegionalHostnameParams) error { + _, err := z.service.CreateDataLocalizationRegionalHostname(ctx, rc, rp) + return err +} + +func (z zoneService) UpdateDataLocalizationRegionalHostname(ctx context.Context, rc *cloudflare.ResourceContainer, rp cloudflare.UpdateDataLocalizationRegionalHostnameParams) error { + _, err := z.service.UpdateDataLocalizationRegionalHostname(ctx, rc, rp) + return err +} + +func (z zoneService) DeleteDataLocalizationRegionalHostname(ctx context.Context, rc *cloudflare.ResourceContainer, hostname string) error { + return z.service.DeleteDataLocalizationRegionalHostname(ctx, rc, hostname) +} + +// createDataLocalizationRegionalHostnameParams is a function that returns the appropriate RegionalHostname Param based on the cloudFlareChange passed in +func createDataLocalizationRegionalHostnameParams(rhc regionalHostnameChange) cloudflare.CreateDataLocalizationRegionalHostnameParams { + return cloudflare.CreateDataLocalizationRegionalHostnameParams{ + Hostname: rhc.Hostname, + RegionKey: rhc.RegionKey, + } +} + +// updateDataLocalizationRegionalHostnameParams is a function that returns the appropriate RegionalHostname Param based on the cloudFlareChange passed in +func updateDataLocalizationRegionalHostnameParams(rhc regionalHostnameChange) cloudflare.UpdateDataLocalizationRegionalHostnameParams { + return cloudflare.UpdateDataLocalizationRegionalHostnameParams{ + Hostname: rhc.Hostname, + RegionKey: rhc.RegionKey, + } +} + +// submitDataLocalizationRegionalHostnameChanges applies a set of data localization regional hostname changes, returns false if it fails +func (p *CloudFlareProvider) submitDataLocalizationRegionalHostnameChanges(ctx context.Context, rhChanges []regionalHostnameChange, resourceContainer *cloudflare.ResourceContainer) bool { + failedChange := false + + for _, rhChange := range rhChanges { + logFields := log.Fields{ + "hostname": rhChange.Hostname, + "region_key": rhChange.RegionKey, + "action": rhChange.action, + "zone": resourceContainer.Identifier, + } + log.WithFields(logFields).Info("Changing regional hostname") + switch rhChange.action { + case cloudFlareCreate: + log.WithFields(logFields).Debug("Creating regional hostname") + if p.DryRun { + continue + } + regionalHostnameParam := createDataLocalizationRegionalHostnameParams(rhChange) + err := p.Client.CreateDataLocalizationRegionalHostname(ctx, resourceContainer, regionalHostnameParam) + if err != nil { + var apiErr *cloudflare.Error + if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusConflict { + log.WithFields(logFields).Debug("Regional hostname already exists, updating instead") + params := updateDataLocalizationRegionalHostnameParams(rhChange) + err := p.Client.UpdateDataLocalizationRegionalHostname(ctx, resourceContainer, params) + if err != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to update regional hostname: %v", err) + } + continue + } + failedChange = true + log.WithFields(logFields).Errorf("failed to create regional hostname: %v", err) + } + case cloudFlareUpdate: + log.WithFields(logFields).Debug("Updating regional hostname") + if p.DryRun { + continue + } + regionalHostnameParam := updateDataLocalizationRegionalHostnameParams(rhChange) + err := p.Client.UpdateDataLocalizationRegionalHostname(ctx, resourceContainer, regionalHostnameParam) + if err != nil { + var apiErr *cloudflare.Error + if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound { + log.WithFields(logFields).Debug("Regional hostname not does not exists, creating instead") + params := createDataLocalizationRegionalHostnameParams(rhChange) + err := p.Client.CreateDataLocalizationRegionalHostname(ctx, resourceContainer, params) + if err != nil { + failedChange = true + log.WithFields(logFields).Errorf("failed to create regional hostname: %v", err) + } + continue + } + failedChange = true + log.WithFields(logFields).Errorf("failed to update regional hostname: %v", err) + } + case cloudFlareDelete: + log.WithFields(logFields).Debug("Deleting regional hostname") + if p.DryRun { + continue + } + err := p.Client.DeleteDataLocalizationRegionalHostname(ctx, resourceContainer, rhChange.Hostname) + if err != nil { + var apiErr *cloudflare.Error + if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound { + log.WithFields(logFields).Debug("Regional hostname does not exists, nothing to do") + continue + } + failedChange = true + log.WithFields(logFields).Errorf("failed to delete regional hostname: %v", err) + } + } + } + + return !failedChange +} + +func (p *CloudFlareProvider) regionalHostname(ep *endpoint.Endpoint) cloudflare.RegionalHostname { + if p.RegionKey == "" || !recordTypeRegionalHostnameSupported[ep.RecordType] { + return cloudflare.RegionalHostname{} + } + regionKey := p.RegionKey + if epRegionKey, exists := ep.GetProviderSpecificProperty(annotations.CloudflareRegionKey); exists { + regionKey = epRegionKey + } + return cloudflare.RegionalHostname{ + Hostname: ep.DNSName, + RegionKey: regionKey, + } +} + +// dataLocalizationRegionalHostnamesChanges processes a slice of cloudFlare changes and consolidates them +// into a list of data localization regional hostname changes. +// returns nil if no changes are needed +func dataLocalizationRegionalHostnamesChanges(changes []*cloudFlareChange) ([]regionalHostnameChange, error) { + regionalHostnameChanges := make(map[string]regionalHostnameChange) + for _, change := range changes { + if change.RegionalHostname.Hostname == "" { + continue + } + if change.RegionalHostname.RegionKey == "" { + return nil, fmt.Errorf("region key is empty for regional hostname %q", change.RegionalHostname.Hostname) + } + regionalHostname, ok := regionalHostnameChanges[change.RegionalHostname.Hostname] + switch change.Action { + case cloudFlareCreate, cloudFlareUpdate: + if !ok { + regionalHostnameChanges[change.RegionalHostname.Hostname] = regionalHostnameChange{ + action: change.Action, + RegionalHostname: change.RegionalHostname, + } + continue + } + if regionalHostname.RegionKey != change.RegionalHostname.RegionKey { + return nil, fmt.Errorf("conflicting region keys for regional hostname %q: %q and %q", change.RegionalHostname.Hostname, regionalHostname.RegionKey, change.RegionalHostname.RegionKey) + } + if (change.Action == cloudFlareUpdate && regionalHostname.action != cloudFlareUpdate) || + regionalHostname.action == cloudFlareDelete { + regionalHostnameChanges[change.RegionalHostname.Hostname] = regionalHostnameChange{ + action: cloudFlareUpdate, + RegionalHostname: change.RegionalHostname, + } + } + case cloudFlareDelete: + if !ok { + regionalHostnameChanges[change.RegionalHostname.Hostname] = regionalHostnameChange{ + action: cloudFlareDelete, + RegionalHostname: change.RegionalHostname, + } + continue + } + } + } + return slices.Collect(maps.Values(regionalHostnameChanges)), nil +} diff --git a/provider/cloudflare/cloudflare_regional_test.go b/provider/cloudflare/cloudflare_regional_test.go new file mode 100644 index 0000000000..f8273a601f --- /dev/null +++ b/provider/cloudflare/cloudflare_regional_test.go @@ -0,0 +1,373 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package cloudflare + +import ( + "reflect" + "slices" + "testing" + + "github.com/cloudflare/cloudflare-go" + + "github.com/stretchr/testify/assert" + "sigs.k8s.io/external-dns/endpoint" +) + +func Test_regionalHostname(t *testing.T) { + type args struct { + endpoint *endpoint.Endpoint + defaultRegionKey string + } + tests := []struct { + name string + args args + want cloudflare.RegionalHostname + }{ + { + name: "no region key", + args: args{ + endpoint: &endpoint.Endpoint{ + RecordType: "A", + DNSName: "example.com", + }, + defaultRegionKey: "", + }, + want: cloudflare.RegionalHostname{}, + }, + { + name: "default region key", + args: args{ + endpoint: &endpoint.Endpoint{ + RecordType: "A", + DNSName: "example.com", + }, + defaultRegionKey: "us", + }, + want: cloudflare.RegionalHostname{ + Hostname: "example.com", + RegionKey: "us", + }, + }, + { + name: "endpoint with region key", + args: args{ + endpoint: &endpoint.Endpoint{ + RecordType: "A", + DNSName: "example.com", + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-region-key", + Value: "eu", + }, + }, + }, + defaultRegionKey: "us", + }, + want: cloudflare.RegionalHostname{ + Hostname: "example.com", + RegionKey: "eu", + }, + }, + { + name: "endpoint with empty region key", + args: args{ + endpoint: &endpoint.Endpoint{ + RecordType: "A", + DNSName: "example.com", + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-region-key", + Value: "", + }, + }, + }, + defaultRegionKey: "us", + }, + want: cloudflare.RegionalHostname{ + Hostname: "example.com", + RegionKey: "", + }, + }, + { + name: "unsupported record type", + args: args{ + endpoint: &endpoint.Endpoint{ + RecordType: "TXT", + DNSName: "example.com", + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-region-key", + Value: "eu", + }, + }, + }, + defaultRegionKey: "us", + }, + want: cloudflare.RegionalHostname{}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := CloudFlareProvider{RegionKey: tt.args.defaultRegionKey} + got := p.regionalHostname(tt.args.endpoint) + assert.Equal(t, got, tt.want) + }) + } +} + +func Test_dataLocalizationRegionalHostnamesChanges(t *testing.T) { + cmpDataLocalizationRegionalHostnameChange := func(i, j regionalHostnameChange) int { + if i.action == j.action { + return 0 + } + if i.Hostname < j.Hostname { + return -1 + } + return 1 + } + type args struct { + changes []*cloudFlareChange + } + tests := []struct { + name string + args args + want []regionalHostnameChange + wantErr bool + }{ + { + name: "empty input", + args: args{ + changes: []*cloudFlareChange{}, + }, + want: nil, + wantErr: false, + }, + { + name: "changes without RegionalHostname", + args: args{ + changes: []*cloudFlareChange{ + { + Action: cloudFlareCreate, + ResourceRecord: cloudflare.DNSRecord{ + Name: "example.com", + }, + RegionalHostname: cloudflare.RegionalHostname{}, // Empty + }, + }, + }, + want: nil, + wantErr: false, + }, + { + name: "change with empty RegionKey", + args: args{ + changes: []*cloudFlareChange{ + { + Action: cloudFlareCreate, + ResourceRecord: cloudflare.DNSRecord{ + Name: "example.com", + }, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example.com", + RegionKey: "", // Empty region key + }, + }, + }, + }, + wantErr: true, + }, + { + name: "conflicting region keys", + args: args{ + changes: []*cloudFlareChange{ + { + Action: cloudFlareCreate, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example.com", + RegionKey: "eu", + }, + }, + { + Action: cloudFlareCreate, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example.com", + RegionKey: "us", // Different region key for same hostname + }, + }, + }, + }, + wantErr: true, + }, + { + name: "update takes precedence over create & delete", + args: args{ + changes: []*cloudFlareChange{ + { + Action: cloudFlareCreate, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example.com", + RegionKey: "eu", + }, + }, + { + Action: cloudFlareUpdate, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example.com", + RegionKey: "eu", + }, + }, + { + Action: cloudFlareDelete, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example.com", + RegionKey: "eu", + }, + }, + }, + }, + want: []regionalHostnameChange{ + { + action: cloudFlareUpdate, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example.com", + RegionKey: "eu", + }, + }, + }, + wantErr: false, + }, + { + name: "create after delete becomes update", + args: args{ + changes: []*cloudFlareChange{ + { + Action: cloudFlareDelete, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example.com", + RegionKey: "eu", + }, + }, + { + Action: cloudFlareCreate, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example.com", + RegionKey: "eu", + }, + }, + }, + }, + want: []regionalHostnameChange{ + { + action: cloudFlareUpdate, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example.com", + RegionKey: "eu", + }, + }, + }, + wantErr: false, + }, + { + name: "consolidate mixed actions for different hostnames", + args: args{ + changes: []*cloudFlareChange{ + { + Action: cloudFlareCreate, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example1.com", + RegionKey: "eu", + }, + }, + { + Action: cloudFlareUpdate, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example2.com", + RegionKey: "us", + }, + }, + { + Action: cloudFlareDelete, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example3.com", + RegionKey: "ap", + }, + }, + // duplicated actions + { + Action: cloudFlareCreate, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example1.com", + RegionKey: "eu", + }, + }, + { + Action: cloudFlareUpdate, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example2.com", + RegionKey: "us", + }, + }, + { + Action: cloudFlareDelete, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example3.com", + RegionKey: "ap", + }, + }, + }, + }, + want: []regionalHostnameChange{ + { + action: cloudFlareCreate, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example1.com", + RegionKey: "eu", + }, + }, + { + action: cloudFlareUpdate, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example2.com", + RegionKey: "us", + }, + }, + { + action: cloudFlareDelete, + RegionalHostname: cloudflare.RegionalHostname{ + Hostname: "example3.com", + RegionKey: "ap", + }, + }, + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := dataLocalizationRegionalHostnamesChanges(tt.args.changes) + if (err != nil) != tt.wantErr { + t.Errorf("dataLocalizationRegionalHostnamesChanges() error = %v, wantErr %v", err, tt.wantErr) + return + } + slices.SortFunc(got, cmpDataLocalizationRegionalHostnameChange) + slices.SortFunc(tt.want, cmpDataLocalizationRegionalHostnameChange) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("dataLocalizationRegionalHostnamesChanges() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index 6bd9341152..8cb449f5cd 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -21,13 +21,12 @@ import ( "errors" "fmt" "os" - "reflect" "slices" "sort" "strings" "testing" - cloudflare "github.com/cloudflare/cloudflare-go" + "github.com/cloudflare/cloudflare-go" "github.com/maxatome/go-testdeep/td" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -2986,337 +2985,6 @@ func TestCloudflareListCustomHostnamesWithPagionation(t *testing.T) { assert.Equal(t, len(chs), CustomHostnamesNumber) } -func Test_getRegionKey(t *testing.T) { - type args struct { - endpoint *endpoint.Endpoint - defaultRegionKey string - } - tests := []struct { - name string - args args - want string - }{ - { - name: "no region key", - args: args{ - endpoint: &endpoint.Endpoint{ - RecordType: "A", - }, - defaultRegionKey: "", - }, - want: "", - }, - { - name: "default region key", - args: args{ - endpoint: &endpoint.Endpoint{ - RecordType: "A", - }, - defaultRegionKey: "us", - }, - want: "us", - }, - { - name: "endpoint with region key", - args: args{ - endpoint: &endpoint.Endpoint{ - RecordType: "A", - ProviderSpecific: endpoint.ProviderSpecific{ - { - Name: "external-dns.alpha.kubernetes.io/cloudflare-region-key", - Value: "eu", - }, - }, - }, - defaultRegionKey: "us", - }, - want: "eu", - }, - { - name: "endpoint with empty region key", - args: args{ - endpoint: &endpoint.Endpoint{ - RecordType: "A", - ProviderSpecific: endpoint.ProviderSpecific{ - { - Name: "external-dns.alpha.kubernetes.io/cloudflare-region-key", - Value: "", - }, - }, - }, - defaultRegionKey: "us", - }, - want: "", - }, - { - name: "unsupported record type", - args: args{ - endpoint: &endpoint.Endpoint{ - RecordType: "TXT", - ProviderSpecific: endpoint.ProviderSpecific{ - { - Name: "external-dns.alpha.kubernetes.io/cloudflare-region-key", - Value: "eu", - }, - }, - }, - defaultRegionKey: "us", - }, - want: "", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := getRegionKey(tt.args.endpoint, tt.args.defaultRegionKey); got != tt.want { - t.Errorf("getRegionKey() = %v, want %v", got, tt.want) - } - }) - } -} - -func Test_dataLocalizationRegionalHostnamesChanges(t *testing.T) { - cmpDataLocalizationRegionalHostnameChange := func(i, j DataLocalizationRegionalHostnameChange) int { - if i.Action == j.Action { - return 0 - } - if i.Hostname < j.Hostname { - return -1 - } - return 1 - } - type args struct { - changes []*cloudFlareChange - } - tests := []struct { - name string - args args - want []DataLocalizationRegionalHostnameChange - wantErr bool - }{ - { - name: "empty input", - args: args{ - changes: []*cloudFlareChange{}, - }, - want: nil, - wantErr: false, - }, - { - name: "changes without RegionalHostname", - args: args{ - changes: []*cloudFlareChange{ - { - Action: cloudFlareCreate, - ResourceRecord: cloudflare.DNSRecord{ - Name: "example.com", - }, - RegionalHostname: cloudflare.RegionalHostname{}, // Empty - }, - }, - }, - want: nil, - wantErr: false, - }, - { - name: "change with empty RegionKey", - args: args{ - changes: []*cloudFlareChange{ - { - Action: cloudFlareCreate, - ResourceRecord: cloudflare.DNSRecord{ - Name: "example.com", - }, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example.com", - RegionKey: "", // Empty region key - }, - }, - }, - }, - wantErr: true, - }, - { - name: "conflicting region keys", - args: args{ - changes: []*cloudFlareChange{ - { - Action: cloudFlareCreate, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example.com", - RegionKey: "eu", - }, - }, - { - Action: cloudFlareCreate, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example.com", - RegionKey: "us", // Different region key for same hostname - }, - }, - }, - }, - wantErr: true, - }, - { - name: "update takes precedence over create & delete", - args: args{ - changes: []*cloudFlareChange{ - { - Action: cloudFlareCreate, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example.com", - RegionKey: "eu", - }, - }, - { - Action: cloudFlareUpdate, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example.com", - RegionKey: "eu", - }, - }, - { - Action: cloudFlareDelete, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example.com", - RegionKey: "eu", - }, - }, - }, - }, - want: []DataLocalizationRegionalHostnameChange{ - { - Action: cloudFlareUpdate, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example.com", - RegionKey: "eu", - }, - }, - }, - wantErr: false, - }, - { - name: "create after delete becomes update", - args: args{ - changes: []*cloudFlareChange{ - { - Action: cloudFlareDelete, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example.com", - RegionKey: "eu", - }, - }, - { - Action: cloudFlareCreate, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example.com", - RegionKey: "eu", - }, - }, - }, - }, - want: []DataLocalizationRegionalHostnameChange{ - { - Action: cloudFlareUpdate, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example.com", - RegionKey: "eu", - }, - }, - }, - wantErr: false, - }, - { - name: "consolidate mixed actions for different hostnames", - args: args{ - changes: []*cloudFlareChange{ - { - Action: cloudFlareCreate, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example1.com", - RegionKey: "eu", - }, - }, - { - Action: cloudFlareUpdate, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example2.com", - RegionKey: "us", - }, - }, - { - Action: cloudFlareDelete, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example3.com", - RegionKey: "ap", - }, - }, - // duplicated actions - { - Action: cloudFlareCreate, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example1.com", - RegionKey: "eu", - }, - }, - { - Action: cloudFlareUpdate, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example2.com", - RegionKey: "us", - }, - }, - { - Action: cloudFlareDelete, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example3.com", - RegionKey: "ap", - }, - }, - }, - }, - want: []DataLocalizationRegionalHostnameChange{ - { - Action: cloudFlareCreate, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example1.com", - RegionKey: "eu", - }, - }, - { - Action: cloudFlareUpdate, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example2.com", - RegionKey: "us", - }, - }, - { - Action: cloudFlareDelete, - RegionalHostname: cloudflare.RegionalHostname{ - Hostname: "example3.com", - RegionKey: "ap", - }, - }, - }, - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := dataLocalizationRegionalHostnamesChanges(tt.args.changes) - if (err != nil) != tt.wantErr { - t.Errorf("dataLocalizationRegionalHostnamesChanges() error = %v, wantErr %v", err, tt.wantErr) - return - } - slices.SortFunc(got, cmpDataLocalizationRegionalHostnameChange) - slices.SortFunc(tt.want, cmpDataLocalizationRegionalHostnameChange) - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("dataLocalizationRegionalHostnamesChanges() = %v, want %v", got, tt.want) - } - }) - } -} - func TestZoneHasPaidPlan(t *testing.T) { client := NewMockCloudFlareClient() cfprovider := &CloudFlareProvider{ From 547565154a88dd796c78e4a737a6b41902aa2f7f Mon Sep 17 00:00:00 2001 From: Valentin Flaux <38909103+vflaux@users.noreply.github.com> Date: Mon, 28 Apr 2025 03:46:45 +0200 Subject: [PATCH 2/2] cloudflare: actions as enum type --- provider/cloudflare/cloudflare.go | 22 +++++++++++++++++----- provider/cloudflare/cloudflare_regional.go | 2 +- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index dbbbbc0290..7b910c4d19 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -38,13 +38,15 @@ import ( "sigs.k8s.io/external-dns/source/annotations" ) +type changeAction int + const ( // cloudFlareCreate is a ChangeAction enum value - cloudFlareCreate = "CREATE" + cloudFlareCreate changeAction = iota // cloudFlareDelete is a ChangeAction enum value - cloudFlareDelete = "DELETE" + cloudFlareDelete // cloudFlareUpdate is a ChangeAction enum value - cloudFlareUpdate = "UPDATE" + cloudFlareUpdate // defaultTTL 1 = automatic defaultTTL = 1 @@ -53,6 +55,16 @@ const ( paidZoneMaxCommentLength = 500 ) +var changeActionNames = map[changeAction]string{ + cloudFlareCreate: "CREATE", + cloudFlareDelete: "DELETE", + cloudFlareUpdate: "UPDATE", +} + +func (action changeAction) String() string { + return changeActionNames[action] +} + // 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 @@ -225,7 +237,7 @@ type CloudFlareProvider struct { // cloudFlareChange differentiates between ChangActions type cloudFlareChange struct { - Action string + Action changeAction ResourceRecord cloudflare.DNSRecord RegionalHostname cloudflare.RegionalHostname CustomHostnames map[string]cloudflare.CustomHostname @@ -680,7 +692,7 @@ func (p *CloudFlareProvider) newCustomHostname(customHostname string, origin str } } -func (p *CloudFlareProvider) newCloudFlareChange(action string, ep *endpoint.Endpoint, target string, current *endpoint.Endpoint) *cloudFlareChange { +func (p *CloudFlareProvider) newCloudFlareChange(action changeAction, ep *endpoint.Endpoint, target string, current *endpoint.Endpoint) *cloudFlareChange { ttl := defaultTTL proxied := shouldBeProxied(ep, p.proxiedByDefault) diff --git a/provider/cloudflare/cloudflare_regional.go b/provider/cloudflare/cloudflare_regional.go index 091ea2e95e..f2b355952e 100644 --- a/provider/cloudflare/cloudflare_regional.go +++ b/provider/cloudflare/cloudflare_regional.go @@ -38,7 +38,7 @@ var recordTypeRegionalHostnameSupported = map[string]bool{ } type regionalHostnameChange struct { - action string + action changeAction cloudflare.RegionalHostname }