From 095c0dd650e4e0eaa1be72c9858cdc0d0755e769 Mon Sep 17 00:00:00 2001 From: Lim Yue Chuan Date: Sat, 18 May 2019 01:23:34 +0800 Subject: [PATCH] Fix CloudFlare provider to return a single endpoint for each name/type. --- provider/cloudflare.go | 46 +++++++- provider/cloudflare_test.go | 222 ++++++++++++++++++++++++++++++++++++ 2 files changed, 263 insertions(+), 5 deletions(-) diff --git a/provider/cloudflare.go b/provider/cloudflare.go index 85ff411c6c..cecb810c39 100644 --- a/provider/cloudflare.go +++ b/provider/cloudflare.go @@ -181,11 +181,10 @@ func (p *CloudFlareProvider) Records() ([]*endpoint.Endpoint, error) { return nil, err } - for _, r := range records { - if supportedRecordType(r.Type) { - endpoints = append(endpoints, endpoint.NewEndpointWithTTL(r.Name, r.Type, endpoint.TTL(r.TTL), r.Content).WithProviderSpecific(source.CloudflareProxiedKey, strconv.FormatBool(r.Proxied))) - } - } + // 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, groupByNameAndType(records)...) } return endpoints, nil @@ -354,3 +353,40 @@ func shouldBeProxied(endpoint *endpoint.Endpoint, proxiedByDefault bool) bool { } return proxied } + +func groupByNameAndType(records []cloudflare.DNSRecord) []*endpoint.Endpoint { + endpoints := []*endpoint.Endpoint{} + + // group supported records by name and type + groups := map[string][]cloudflare.DNSRecord{} + + for _, r := range records { + if !supportedRecordType(r.Type) { + continue + } + + groupBy := r.Name + r.Type + if _, ok := groups[groupBy]; !ok { + groups[groupBy] = []cloudflare.DNSRecord{} + } + + groups[groupBy] = append(groups[groupBy], r) + } + + // create single endpoint with all the targets for each name/type + for _, records := range groups { + targets := make([]string, len(records)) + for i, record := range records { + targets[i] = record.Content + } + endpoints = append(endpoints, + endpoint.NewEndpointWithTTL( + records[0].Name, + records[0].Type, + endpoint.TTL(records[0].TTL), + targets...). + WithProviderSpecific(source.CloudflareProxiedKey, strconv.FormatBool(records[0].Proxied))) + } + + return endpoints +} diff --git a/provider/cloudflare_test.go b/provider/cloudflare_test.go index 73e32d59a7..1c3b688546 100644 --- a/provider/cloudflare_test.go +++ b/provider/cloudflare_test.go @@ -595,3 +595,225 @@ func validateCloudFlareZones(t *testing.T, zones []cloudflare.Zone, expected []c assert.Equal(t, expected[i].Name, zone.Name) } } + +func TestGroupByNameAndType(t *testing.T) { + testCases := []struct { + Name string + Records []cloudflare.DNSRecord + ExpectedEndpoints []*endpoint.Endpoint + }{ + { + Name: "empty", + Records: []cloudflare.DNSRecord{}, + ExpectedEndpoints: []*endpoint.Endpoint{}, + }, + { + Name: "single record - single target", + Records: []cloudflare.DNSRecord{ + { + Name: "foo.com", + Type: endpoint.RecordTypeA, + Content: "10.10.10.1", + TTL: defaultCloudFlareRecordTTL, + }, + }, + ExpectedEndpoints: []*endpoint.Endpoint{ + { + DNSName: "foo.com", + Targets: endpoint.Targets{"10.10.10.1"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-proxied", + Value: "false", + }, + }, + }, + }, + }, + { + Name: "single record - multiple targets", + Records: []cloudflare.DNSRecord{ + { + Name: "foo.com", + Type: endpoint.RecordTypeA, + Content: "10.10.10.1", + TTL: defaultCloudFlareRecordTTL, + }, + { + Name: "foo.com", + Type: endpoint.RecordTypeA, + Content: "10.10.10.2", + TTL: defaultCloudFlareRecordTTL, + }, + }, + ExpectedEndpoints: []*endpoint.Endpoint{ + { + DNSName: "foo.com", + Targets: endpoint.Targets{"10.10.10.1", "10.10.10.2"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-proxied", + Value: "false", + }, + }, + }, + }, + }, + { + Name: "multiple record - multiple targets", + Records: []cloudflare.DNSRecord{ + { + Name: "foo.com", + Type: endpoint.RecordTypeA, + Content: "10.10.10.1", + TTL: defaultCloudFlareRecordTTL, + }, + { + Name: "foo.com", + Type: endpoint.RecordTypeA, + Content: "10.10.10.2", + TTL: defaultCloudFlareRecordTTL, + }, + { + Name: "bar.de", + Type: endpoint.RecordTypeA, + Content: "10.10.10.1", + TTL: defaultCloudFlareRecordTTL, + }, + { + Name: "bar.de", + Type: endpoint.RecordTypeA, + Content: "10.10.10.2", + TTL: defaultCloudFlareRecordTTL, + }, + }, + ExpectedEndpoints: []*endpoint.Endpoint{ + { + DNSName: "foo.com", + Targets: endpoint.Targets{"10.10.10.1", "10.10.10.2"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-proxied", + Value: "false", + }, + }, + }, + { + DNSName: "bar.de", + Targets: endpoint.Targets{"10.10.10.1", "10.10.10.2"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-proxied", + Value: "false", + }, + }, + }, + }, + }, + { + Name: "multiple record - mixed single/multiple targets", + Records: []cloudflare.DNSRecord{ + { + Name: "foo.com", + Type: endpoint.RecordTypeA, + Content: "10.10.10.1", + TTL: defaultCloudFlareRecordTTL, + }, + { + Name: "foo.com", + Type: endpoint.RecordTypeA, + Content: "10.10.10.2", + TTL: defaultCloudFlareRecordTTL, + }, + { + Name: "bar.de", + Type: endpoint.RecordTypeA, + Content: "10.10.10.1", + TTL: defaultCloudFlareRecordTTL, + }, + }, + ExpectedEndpoints: []*endpoint.Endpoint{ + { + DNSName: "foo.com", + Targets: endpoint.Targets{"10.10.10.1", "10.10.10.2"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-proxied", + Value: "false", + }, + }, + }, + { + DNSName: "bar.de", + Targets: endpoint.Targets{"10.10.10.1"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-proxied", + Value: "false", + }, + }, + }, + }, + }, + { + Name: "unsupported record type", + Records: []cloudflare.DNSRecord{ + { + Name: "foo.com", + Type: endpoint.RecordTypeA, + Content: "10.10.10.1", + TTL: defaultCloudFlareRecordTTL, + }, + { + Name: "foo.com", + Type: endpoint.RecordTypeA, + Content: "10.10.10.2", + TTL: defaultCloudFlareRecordTTL, + }, + { + Name: "bar.de", + Type: "NOT SUPPORTED", + Content: "10.10.10.1", + TTL: defaultCloudFlareRecordTTL, + }, + }, + ExpectedEndpoints: []*endpoint.Endpoint{ + { + DNSName: "foo.com", + Targets: endpoint.Targets{"10.10.10.1", "10.10.10.2"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: endpoint.TTL(defaultCloudFlareRecordTTL), + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-proxied", + Value: "false", + }, + }, + }, + }, + }, + } + + for _, tc := range testCases { + assert.Equal(t, groupByNameAndType(tc.Records), tc.ExpectedEndpoints) + } +}