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/docs/tutorials/cloudflare.md b/docs/tutorials/cloudflare.md index fb1eceb7c..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 -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 a163c4c7a..5a85c188f 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,9 +346,18 @@ func (p *CloudFlareProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) error { cloudflareChanges := []*cloudFlareChange{} - for _, endpoint := range changes.Create { - for _, target := range endpoint.Targets { - cloudflareChanges = append(cloudflareChanges, p.newCloudFlareChange(cloudFlareCreate, endpoint, target, nil)) + // if custom hostnames are enabled, deleting first allows to avoid conflicts with the new ones + if p.CustomHostnamesConfig.Enabled { + for _, e := range changes.Delete { + for _, target := range e.Targets { + cloudflareChanges = append(cloudflareChanges, p.newCloudFlareChange(cloudFlareDelete, e, target, nil)) + } + } + } + + for _, e := range changes.Create { + for _, target := range e.Targets { + cloudflareChanges = append(cloudflareChanges, p.newCloudFlareChange(cloudFlareCreate, e, 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 _, e := range changes.Delete { + for _, target := range e.Targets { + cloudflareChanges = append(cloudflareChanges, p.newCloudFlareChange(cloudFlareDelete, e, 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,48 +634,54 @@ 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) newCloudFlareChange(action string, endpoint *endpoint.Endpoint, target string, current *endpoint.Endpoint) *cloudFlareChange { - ttl := defaultCloudFlareRecordTTL - proxied := shouldBeProxied(endpoint, p.proxiedByDefault) +func (p *CloudFlareProvider) newCustomHostname(customHostname string, origin string) cloudflare.CustomHostname { + return cloudflare.CustomHostname{ + Hostname: customHostname, + CustomOriginServer: origin, + SSL: getCustomHostnamesSSLOptions(p.CustomHostnamesConfig), + } +} - if endpoint.RecordTTL.IsConfigured() { - ttl = int(endpoint.RecordTTL) +func (p *CloudFlareProvider) newCloudFlareChange(action string, ep *endpoint.Endpoint, target string, current *endpoint.Endpoint) *cloudFlareChange { + ttl := defaultCloudFlareRecordTTL + proxied := shouldBeProxied(ep, p.proxiedByDefault) + + if ep.RecordTTL.IsConfigured() { + ttl = int(ep.RecordTTL) } 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(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, }, - CustomHostnamePrev: customHostnamePrev, - CustomHostname: newCustomHostname, + CustomHostnamesPrev: prevCustomHostnames, + CustomHostnames: newCustomHostnames, } } @@ -727,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 { @@ -742,19 +781,20 @@ func shouldBeProxied(endpoint *endpoint.Endpoint, proxiedByDefault bool) bool { } } - if recordTypeProxyNotSupported[endpoint.RecordType] { + if recordTypeProxyNotSupported[ep.RecordType] { proxied = false } return proxied } -func getEndpointCustomHostname(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 { - return v.Value + customHostnames := strings.Split(v.Value, ",") + return customHostnames } } - return "" + return []string{} } func groupByNameAndTypeWithCustomHostnames(records DNSRecordsMap, chs CustomHostnamesMap) []*endpoint.Endpoint { @@ -777,11 +817,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 @@ -793,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), @@ -802,16 +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)) - // 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) + 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) + e = e.WithProviderSpecific(source.CloudflareCustomHostnameKey, strings.Join(customHostnames, ",")) } - endpoints = append(endpoints, ep) + endpoints = append(endpoints, e) } return endpoints diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index 1c02abd9b..4f3024bbb 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() @@ -1772,6 +1762,44 @@ 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 { + 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{ @@ -1801,7 +1829,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 +1839,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 +1893,25 @@ 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 err error + var records, endpoints []*endpoint.Endpoint + + records, err = provider.Records(ctx) + if errors.Is(err, nil) { + endpoints, err = provider.AdjustEndpoints(tc.Endpoints) } - - 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}, + 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) } - - 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 e := checkFailed(tc.Name, err, tc.shouldFail); !errors.Is(e, nil) { + t.Error(e) } } } @@ -1896,10 +1926,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 +1950,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 +1965,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 +2057,11 @@ func TestCloudflareCustomHostnameOperations(t *testing.T) { }, }, }, + shouldFail: false, + }, + { + Name: "causing to list failing to list custom hostname", + Endpoints: []*endpoint.Endpoint{}, shouldFail: true, }, { @@ -2148,9 +2176,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 +2209,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 +2230,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 +2311,31 @@ 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 err error + var records, endpoints []*endpoint.Endpoint + + records, err = provider.Records(ctx) + if errors.Is(err, nil) { + endpoints, err = provider.AdjustEndpoints(tc.Endpoints) } + 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) - 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 e := checkFailed(tc.Name, err, tc.shouldFail); !errors.Is(e, nil) { + t.Error(e) } } @@ -2271,18 +2358,139 @@ 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) } - 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 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") } } } @@ -2381,11 +2589,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) @@ -2398,7 +2606,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", @@ -2407,8 +2614,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) }