From ffe076ddd96a5d3719eed4cc8b71c06cdd1e3921 Mon Sep 17 00:00:00 2001 From: Olaf Klischat Date: Sat, 28 Oct 2017 16:22:22 +0200 Subject: [PATCH 1/8] support multiple targets by having one Endpoint per DNSName and Target rather than just per DNSName --- controller/controller_test.go | 14 ++++++----- plan/plan.go | 20 ++++------------ plan/plan_test.go | 44 +++++++++++++++++++++-------------- registry/txt.go | 21 +++++++++++++---- 4 files changed, 55 insertions(+), 44 deletions(-) diff --git a/controller/controller_test.go b/controller/controller_test.go index ac298daff7..94b18fb7b1 100644 --- a/controller/controller_test.go +++ b/controller/controller_test.go @@ -94,8 +94,9 @@ func TestRunOnce(t *testing.T) { Target: "1.2.3.4", }, { - DNSName: "update-record", - Target: "8.8.4.4", + DNSName: "update-record", + Target: "8.8.4.4", + RecordTTL: 100, }, }, nil) @@ -103,8 +104,9 @@ func TestRunOnce(t *testing.T) { provider := newMockProvider( []*endpoint.Endpoint{ { - DNSName: "update-record", - Target: "8.8.8.8", + DNSName: "update-record", + Target: "8.8.4.4", + RecordTTL: 50, }, { DNSName: "delete-record", @@ -116,10 +118,10 @@ func TestRunOnce(t *testing.T) { {DNSName: "create-record", Target: "1.2.3.4"}, }, UpdateNew: []*endpoint.Endpoint{ - {DNSName: "update-record", Target: "8.8.4.4"}, + {DNSName: "update-record", Target: "8.8.4.4", RecordTTL: 100}, }, UpdateOld: []*endpoint.Endpoint{ - {DNSName: "update-record", Target: "8.8.8.8"}, + {DNSName: "update-record", Target: "8.8.4.4", RecordTTL: 50}, }, Delete: []*endpoint.Endpoint{ {DNSName: "delete-record", Target: "4.3.2.1"}, diff --git a/plan/plan.go b/plan/plan.go index b8ba4765df..f7ad49b3da 100644 --- a/plan/plan.go +++ b/plan/plan.go @@ -65,24 +65,16 @@ func (p *Plan) Calculate() *Plan { continue } - targetChanged := targetChanged(desired, current) shouldUpdateTTL := shouldUpdateTTL(desired, current) - if !targetChanged && !shouldUpdateTTL { + if !shouldUpdateTTL { log.Debugf("Skipping endpoint %v because nothing has changed", desired) continue } changes.UpdateOld = append(changes.UpdateOld, current) - desired.MergeLabels(current.Labels) // inherit the labels from the dns provider, including Owner ID - - if targetChanged { - desired.RecordType = current.RecordType // inherit the type from the dns provider - } - - if !shouldUpdateTTL { - desired.RecordTTL = current.RecordTTL - } + desired.MergeLabels(current.Labels) // inherit the labels from the dns provider, including Owner ID + desired.RecordType = current.RecordType // inherit the type from the dns provider changes.UpdateNew = append(changes.UpdateNew, desired) } @@ -109,10 +101,6 @@ func (p *Plan) Calculate() *Plan { return plan } -func targetChanged(desired, current *endpoint.Endpoint) bool { - return desired.Target != current.Target -} - func shouldUpdateTTL(desired, current *endpoint.Endpoint) bool { if !desired.RecordTTL.IsConfigured() { return false @@ -123,7 +111,7 @@ func shouldUpdateTTL(desired, current *endpoint.Endpoint) bool { // recordExists checks whether a record can be found in a list of records. func recordExists(needle *endpoint.Endpoint, haystack []*endpoint.Endpoint) (*endpoint.Endpoint, bool) { for _, record := range haystack { - if record.DNSName == needle.DNSName { + if record.DNSName == needle.DNSName && record.Target == needle.Target { return record, true } } diff --git a/plan/plan_test.go b/plan/plan_test.go index a117b6077a..f6ffb34d48 100644 --- a/plan/plan_test.go +++ b/plan/plan_test.go @@ -27,34 +27,37 @@ import ( // TestCalculate tests that a plan can calculate actions to move a list of // current records to a list of desired records. func TestCalculate(t *testing.T) { + // we need different TTLs to create differing Endpoints with the same name and target + ttl := endpoint.TTL(300) + ttl2 := endpoint.TTL(50) + // empty list of records empty := []*endpoint.Endpoint{} // a simple entry fooV1 := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v1", endpoint.RecordTypeCNAME)} // the same entry but with different target fooV2 := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v2", endpoint.RecordTypeCNAME)} + // the same entry as before but with varying TTLs + fooV2ttl1 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v2", endpoint.RecordTypeCNAME, ttl)} + fooV2ttl2 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v2", endpoint.RecordTypeCNAME, ttl2)} // another simple entry bar := []*endpoint.Endpoint{endpoint.NewEndpoint("bar", "v1", endpoint.RecordTypeCNAME)} // test case with labels - noLabels := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v2", endpoint.RecordTypeCNAME)} - labeledV2 := []*endpoint.Endpoint{newEndpointWithOwner("foo", "v2", "123")} - labeledV1 := []*endpoint.Endpoint{newEndpointWithOwner("foo", "v1", "123")} + unlabeledTTL2 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v2", endpoint.RecordTypeCNAME, ttl2)} + labeledTTL1 := []*endpoint.Endpoint{newEndpointWithOwnerAndTTL("foo", "v2", "123", ttl)} + labeledTTL2 := []*endpoint.Endpoint{newEndpointWithOwnerAndTTL("foo", "v2", "123", ttl2)} // test case with type inheritance - noType := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v2", "")} - typedV2 := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v2", endpoint.RecordTypeA)} - typedV1 := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v1", endpoint.RecordTypeA)} + untypedTTL2 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v2", "", ttl2)} + typedTTL1 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v2", endpoint.RecordTypeA, ttl)} + typedTTL2 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v2", endpoint.RecordTypeA, ttl2)} - // test case with TTL - ttl := endpoint.TTL(300) - ttl2 := endpoint.TTL(50) + // explicit TTL test cases ttlV1 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v1", endpoint.RecordTypeCNAME, ttl)} ttlV2 := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v1", endpoint.RecordTypeCNAME)} ttlV3 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v1", endpoint.RecordTypeCNAME, ttl)} ttlV4 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v1", endpoint.RecordTypeCNAME, ttl2)} - ttlV5 := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v2", endpoint.RecordTypeCNAME)} - ttlV6 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v2", endpoint.RecordTypeCNAME, ttl)} for _, tc := range []struct { policies []Policy @@ -69,24 +72,24 @@ func TestCalculate(t *testing.T) { {[]Policy{&SyncPolicy{}}, fooV1, fooV1, empty, empty, empty, empty}, // Nothing is desired deletes the current. {[]Policy{&SyncPolicy{}}, fooV1, empty, empty, empty, empty, fooV1}, - // Current and desired match but Target is different triggers an update. - {[]Policy{&SyncPolicy{}}, fooV1, fooV2, empty, fooV1, fooV2, empty}, + // Current and desired match but TTL is different triggers an update. + {[]Policy{&SyncPolicy{}}, fooV2ttl1, fooV2ttl2, empty, fooV2ttl1, fooV2ttl2, empty}, // Both exist but are different creates desired and deletes current. {[]Policy{&SyncPolicy{}}, fooV1, bar, bar, empty, empty, fooV1}, + // Same thing with current and desired only having different targets + {[]Policy{&SyncPolicy{}}, fooV1, fooV2, fooV2, empty, empty, fooV1}, // Nothing is desired but policy doesn't allow deletions. {[]Policy{&UpsertOnlyPolicy{}}, fooV1, empty, empty, empty, empty, empty}, // Labels should be inherited - {[]Policy{&SyncPolicy{}}, labeledV1, noLabels, empty, labeledV1, labeledV2, empty}, + {[]Policy{&SyncPolicy{}}, labeledTTL1, unlabeledTTL2, empty, labeledTTL1, labeledTTL2, empty}, // RecordType should be inherited - {[]Policy{&SyncPolicy{}}, typedV1, noType, empty, typedV1, typedV2, empty}, + {[]Policy{&SyncPolicy{}}, typedTTL1, untypedTTL2, empty, typedTTL1, typedTTL2, empty}, // If desired TTL is not configured, do not update {[]Policy{&SyncPolicy{}}, ttlV1, ttlV2, empty, empty, empty, empty}, // If desired TTL is configured but is the same as current TTL, do not update {[]Policy{&SyncPolicy{}}, ttlV1, ttlV3, empty, empty, empty, empty}, // If desired TTL is configured and is not the same as current TTL, need to update {[]Policy{&SyncPolicy{}}, ttlV1, ttlV4, empty, ttlV1, ttlV4, empty}, - // If target changed and desired TTL is not configured, do not update TTL - {[]Policy{&SyncPolicy{}}, ttlV1, ttlV5, empty, ttlV1, ttlV6, empty}, } { // setup plan plan := &Plan{ @@ -187,3 +190,10 @@ func newEndpointWithOwner(dnsName, target, ownerID string) *endpoint.Endpoint { e.Labels[endpoint.OwnerLabelKey] = ownerID return e } + +func newEndpointWithOwnerAndTTL(dnsName, target, ownerID string, ttl endpoint.TTL) *endpoint.Endpoint { + e := endpoint.NewEndpoint(dnsName, target, endpoint.RecordTypeCNAME) + e.Labels[endpoint.OwnerLabelKey] = ownerID + e.RecordTTL = ttl + return e +} diff --git a/registry/txt.go b/registry/txt.go index b359f31711..5fb96188ae 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -101,15 +101,26 @@ func (im *TXTRegistry) ApplyChanges(changes *plan.Changes) error { Delete: filterOwnedRecords(im.ownerID, changes.Delete), } + // create one create / delete TXT record per created/deleted DNSName + + createdDomains := map[string]bool{} + for _, r := range filteredChanges.Create { - txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), im.getTXTLabel(), endpoint.RecordTypeTXT) - filteredChanges.Create = append(filteredChanges.Create, txt) + if !createdDomains[r.DNSName] { + txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), im.getTXTLabel(), endpoint.RecordTypeTXT) + filteredChanges.Create = append(filteredChanges.Create, txt) + createdDomains[r.DNSName] = true + } } - for _, r := range filteredChanges.Delete { - txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), im.getTXTLabel(), endpoint.RecordTypeTXT) + deletedDomains := map[string]bool{} - filteredChanges.Delete = append(filteredChanges.Delete, txt) + for _, r := range filteredChanges.Delete { + if !deletedDomains[r.DNSName] { + txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), im.getTXTLabel(), endpoint.RecordTypeTXT) + filteredChanges.Delete = append(filteredChanges.Delete, txt) + deletedDomains[r.DNSName] = true + } } return im.provider.ApplyChanges(filteredChanges) From 2d45bc2e19dd455f52925831c171c8506f423eb9 Mon Sep 17 00:00:00 2001 From: Olaf Klischat Date: Mon, 30 Oct 2017 15:51:21 +0100 Subject: [PATCH 2/8] cloudflare provider fixed for multi-target --- provider/cloudflare.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/provider/cloudflare.go b/provider/cloudflare.go index cef57b1643..8ad6eb8ac3 100644 --- a/provider/cloudflare.go +++ b/provider/cloudflare.go @@ -251,7 +251,7 @@ func (p *CloudFlareProvider) changesByZone(zones []cloudflare.Zone, changeSet [] func (p *CloudFlareProvider) getRecordID(records []cloudflare.DNSRecord, record cloudflare.DNSRecord) string { for _, zoneRecord := range records { - if zoneRecord.Name == record.Name && zoneRecord.Type == record.Type { + if zoneRecord.Name == record.Name && zoneRecord.Type == record.Type && zoneRecord.Content == record.Content { return zoneRecord.ID } } From e1dd79fd6d4b23a894bf3b7817f04542d3274eee Mon Sep 17 00:00:00 2001 From: Olaf Klischat Date: Tue, 31 Oct 2017 22:09:45 +0100 Subject: [PATCH 3/8] TXTRegistry: ownerID tracking (for multi-target records) via internal state rather than Endpoint.Labels --- plan/plan.go | 2 +- registry/registry.go | 14 -------- registry/txt.go | 77 ++++++++++++++++++++++++++++++++------------ 3 files changed, 57 insertions(+), 36 deletions(-) diff --git a/plan/plan.go b/plan/plan.go index f7ad49b3da..9301923aec 100644 --- a/plan/plan.go +++ b/plan/plan.go @@ -73,7 +73,7 @@ func (p *Plan) Calculate() *Plan { } changes.UpdateOld = append(changes.UpdateOld, current) - desired.MergeLabels(current.Labels) // inherit the labels from the dns provider, including Owner ID + desired.MergeLabels(current.Labels) // inherit the labels from the dns provider desired.RecordType = current.RecordType // inherit the type from the dns provider changes.UpdateNew = append(changes.UpdateNew, desired) diff --git a/registry/registry.go b/registry/registry.go index 6107299578..a108ddc046 100644 --- a/registry/registry.go +++ b/registry/registry.go @@ -19,7 +19,6 @@ package registry import ( "github.com/kubernetes-incubator/external-dns/endpoint" "github.com/kubernetes-incubator/external-dns/plan" - log "github.com/sirupsen/logrus" ) // Registry is an interface which should enables ownership concept in external-dns @@ -30,16 +29,3 @@ type Registry interface { Records() ([]*endpoint.Endpoint, error) ApplyChanges(changes *plan.Changes) error } - -//TODO(ideahitme): consider moving this to Plan -func filterOwnedRecords(ownerID string, eps []*endpoint.Endpoint) []*endpoint.Endpoint { - filtered := []*endpoint.Endpoint{} - for _, ep := range eps { - if endpointOwner, ok := ep.Labels[endpoint.OwnerLabelKey]; !ok || endpointOwner != ownerID { - log.Debugf(`Skipping endpoint %v because owner id does not match, found: "%s", required: "%s"`, ep, endpointOwner, ownerID) - continue - } - filtered = append(filtered, ep) - } - return filtered -} diff --git a/registry/txt.go b/registry/txt.go index 5fb96188ae..eef159ccef 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" + log "github.com/sirupsen/logrus" "regexp" "strings" @@ -38,6 +39,8 @@ type TXTRegistry struct { provider provider.Provider ownerID string //refers to the owner id of the current instance mapper nameMapper + ownerMap map[string]string // map DNSName => ownerID + recCount map[string]int // map DNSName => number of records } // NewTXTRegistry returns new TXTRegistry object @@ -66,13 +69,16 @@ func (im *TXTRegistry) Records() ([]*endpoint.Endpoint, error) { endpoints := make([]*endpoint.Endpoint, 0) - ownerMap := map[string]string{} + im.ownerMap = make(map[string]string) + im.recCount = make(map[string]int) for _, record := range records { if record.RecordType != endpoint.RecordTypeTXT { endpoints = append(endpoints, record) + im.recCount[record.DNSName]++ continue } + ownerID := im.extractOwnerID(record.Target) if ownerID == "" { //case when value of txt record cannot be identified @@ -80,13 +86,12 @@ func (im *TXTRegistry) Records() ([]*endpoint.Endpoint, error) { endpoints = append(endpoints, record) continue } + endpointDNSName := im.mapper.toEndpointName(record.DNSName) - ownerMap[endpointDNSName] = ownerID + im.ownerMap[endpointDNSName] = ownerID } - for _, ep := range endpoints { - ep.Labels[endpoint.OwnerLabelKey] = ownerMap[ep.DNSName] - } + log.Debugf("after scanning: ownerMap: %s, recCount: %s", im.ownerMap, im.recCount) return endpoints, err } @@ -94,36 +99,66 @@ func (im *TXTRegistry) Records() ([]*endpoint.Endpoint, error) { // ApplyChanges updates dns provider with the changes // for each created/deleted record it will also take into account TXT records for creation/deletion func (im *TXTRegistry) ApplyChanges(changes *plan.Changes) error { + if im.ownerMap == nil { + // Records() never called yet or error during last ApplyChanges() call => rescan + _, err := im.Records() + if err != nil { + return err + } + } + filteredChanges := &plan.Changes{ - Create: changes.Create, - UpdateNew: filterOwnedRecords(im.ownerID, changes.UpdateNew), - UpdateOld: filterOwnedRecords(im.ownerID, changes.UpdateOld), - Delete: filterOwnedRecords(im.ownerID, changes.Delete), + Create: []*endpoint.Endpoint{}, + UpdateNew: im.filterOwnedRecords(changes.UpdateNew), + UpdateOld: im.filterOwnedRecords(changes.UpdateOld), + Delete: im.filterOwnedRecords(changes.Delete), } - // create one create / delete TXT record per created/deleted DNSName + for _, r := range changes.Create { + rOwner := im.ownerMap[r.DNSName] - createdDomains := map[string]bool{} + if rOwner == "" || rOwner == im.ownerID { + filteredChanges.Create = append(filteredChanges.Create, r) - for _, r := range filteredChanges.Create { - if !createdDomains[r.DNSName] { - txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), im.getTXTLabel(), endpoint.RecordTypeTXT) - filteredChanges.Create = append(filteredChanges.Create, txt) - createdDomains[r.DNSName] = true + im.ownerMap[r.DNSName] = im.ownerID + im.recCount[r.DNSName]++ + + if 1 == im.recCount[r.DNSName] { + txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), im.getTXTLabel(), endpoint.RecordTypeTXT) + filteredChanges.Create = append(filteredChanges.Create, txt) + } } } - deletedDomains := map[string]bool{} - for _, r := range filteredChanges.Delete { - if !deletedDomains[r.DNSName] { + if im.recCount[r.DNSName]--; im.recCount[r.DNSName] <= 0 { + im.recCount[r.DNSName] = 0 + delete(im.ownerMap, r.DNSName) txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), im.getTXTLabel(), endpoint.RecordTypeTXT) filteredChanges.Delete = append(filteredChanges.Delete, txt) - deletedDomains[r.DNSName] = true } } - return im.provider.ApplyChanges(filteredChanges) + log.Debugf("before provider.ApplyChanges: ownerMap: %s, recCount: %s", im.ownerMap, im.recCount) + + err := im.provider.ApplyChanges(filteredChanges) + if err != nil { + // error occured in the provider => we don't know which records were stored => force re-scanning on the next call + im.ownerMap = nil + } + return err +} + +func (im *TXTRegistry) filterOwnedRecords(eps []*endpoint.Endpoint) []*endpoint.Endpoint { + filtered := []*endpoint.Endpoint{} + for _, ep := range eps { + if endpointOwner, ok := im.ownerMap[ep.DNSName]; !ok || endpointOwner != im.ownerID { + log.Debugf(`Skipping endpoint %v because owner id does not match, found: "%s", required: "%s"`, ep, endpointOwner, im.ownerID) + continue + } + filtered = append(filtered, ep) + } + return filtered } /** From 28fec84902a926b7b3e4ed2c78619df82891f6e1 Mon Sep 17 00:00:00 2001 From: Olaf Klischat Date: Wed, 1 Nov 2017 00:11:10 +0100 Subject: [PATCH 4/8] tests adapted --- registry/txt.go | 4 +- registry/txt_test.go | 183 +++++++++++++++++++------------------------ 2 files changed, 84 insertions(+), 103 deletions(-) diff --git a/registry/txt.go b/registry/txt.go index eef159ccef..33e930350e 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -88,7 +88,9 @@ func (im *TXTRegistry) Records() ([]*endpoint.Endpoint, error) { } endpointDNSName := im.mapper.toEndpointName(record.DNSName) - im.ownerMap[endpointDNSName] = ownerID + if endpointDNSName != "" { + im.ownerMap[endpointDNSName] = ownerID + } } log.Debugf("after scanning: ownerMap: %s, recCount: %s", im.ownerMap, im.recCount) diff --git a/registry/txt_test.go b/registry/txt_test.go index 9a8cc73490..8606e23412 100644 --- a/registry/txt_test.go +++ b/registry/txt_test.go @@ -68,15 +68,15 @@ func testTXTRegistryRecordsPrefixed(t *testing.T) { p.CreateZone(testZone) p.ApplyChanges(&plan.Changes{ Create: []*endpoint.Endpoint{ - newEndpointWithOwner("foo.test-zone.example.org", "foo.loadbalancer.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("bar.test-zone.example.org", "my-domain.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("txt.bar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("txt.bar.test-zone.example.org", "baz.test-zone.example.org", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("qux.test-zone.example.org", "random", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner-2\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + endpoint.NewEndpoint("foo.test-zone.example.org", "foo.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("bar.test-zone.example.org", "my-domain.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("txt.bar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("txt.bar.test-zone.example.org", "baz.test-zone.example.org", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("qux.test-zone.example.org", "random", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner-2\"", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), }, }) expectedRecords := []*endpoint.Endpoint{ @@ -84,55 +84,44 @@ func testTXTRegistryRecordsPrefixed(t *testing.T) { DNSName: "foo.test-zone.example.org", Target: "foo.loadbalancer.com", RecordType: endpoint.RecordTypeCNAME, - Labels: map[string]string{ - endpoint.OwnerLabelKey: "", - }, }, { DNSName: "bar.test-zone.example.org", Target: "my-domain.com", RecordType: endpoint.RecordTypeCNAME, - Labels: map[string]string{ - endpoint.OwnerLabelKey: "owner", - }, }, { DNSName: "txt.bar.test-zone.example.org", Target: "baz.test-zone.example.org", RecordType: endpoint.RecordTypeCNAME, - Labels: map[string]string{ - endpoint.OwnerLabelKey: "", - }, }, { DNSName: "qux.test-zone.example.org", Target: "random", RecordType: endpoint.RecordTypeTXT, - Labels: map[string]string{ - endpoint.OwnerLabelKey: "", - }, }, { DNSName: "tar.test-zone.example.org", Target: "tar.loadbalancer.com", RecordType: endpoint.RecordTypeCNAME, - Labels: map[string]string{ - endpoint.OwnerLabelKey: "owner-2", - }, }, { DNSName: "foobar.test-zone.example.org", Target: "foobar.loadbalancer.com", RecordType: endpoint.RecordTypeCNAME, - Labels: map[string]string{ - endpoint.OwnerLabelKey: "", - }, }, } + // white-box testing: check tracked ownerIDs + expectedOwnerMap := map[string]string{ + "bar.test-zone.example.org": "owner", + "tar.test-zone.example.org": "owner-2", + } + r, _ := NewTXTRegistry(p, "txt.", "owner") records, _ := r.Records() assert.True(t, testutils.SameEndpoints(records, expectedRecords)) + assert.Equal(t, expectedOwnerMap, r.ownerMap) } func testTXTRegistryRecordsNoPrefix(t *testing.T) { @@ -140,15 +129,15 @@ func testTXTRegistryRecordsNoPrefix(t *testing.T) { p.CreateZone(testZone) p.ApplyChanges(&plan.Changes{ Create: []*endpoint.Endpoint{ - newEndpointWithOwner("foo.test-zone.example.org", "foo.loadbalancer.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("bar.test-zone.example.org", "my-domain.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("txt.bar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("txt.bar.test-zone.example.org", "baz.test-zone.example.org", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("qux.test-zone.example.org", "random", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner-2\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + endpoint.NewEndpoint("foo.test-zone.example.org", "foo.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("bar.test-zone.example.org", "my-domain.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("txt.bar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("txt.bar.test-zone.example.org", "baz.test-zone.example.org", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("qux.test-zone.example.org", "random", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner-2\"", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), }, }) expectedRecords := []*endpoint.Endpoint{ @@ -156,56 +145,46 @@ func testTXTRegistryRecordsNoPrefix(t *testing.T) { DNSName: "foo.test-zone.example.org", Target: "foo.loadbalancer.com", RecordType: endpoint.RecordTypeCNAME, - Labels: map[string]string{ - endpoint.OwnerLabelKey: "", - }, }, { DNSName: "bar.test-zone.example.org", Target: "my-domain.com", RecordType: endpoint.RecordTypeCNAME, - Labels: map[string]string{ - endpoint.OwnerLabelKey: "", - }, }, { DNSName: "txt.bar.test-zone.example.org", Target: "baz.test-zone.example.org", RecordType: endpoint.RecordTypeCNAME, - Labels: map[string]string{ - endpoint.OwnerLabelKey: "owner", - }, }, { DNSName: "qux.test-zone.example.org", Target: "random", RecordType: endpoint.RecordTypeTXT, - Labels: map[string]string{ - endpoint.OwnerLabelKey: "", - }, }, { DNSName: "tar.test-zone.example.org", Target: "tar.loadbalancer.com", RecordType: endpoint.RecordTypeCNAME, - Labels: map[string]string{ - endpoint.OwnerLabelKey: "", - }, }, { DNSName: "foobar.test-zone.example.org", Target: "foobar.loadbalancer.com", RecordType: endpoint.RecordTypeCNAME, - Labels: map[string]string{ - endpoint.OwnerLabelKey: "owner", - }, }, } + // white-box testing: check tracked ownerIDs + expectedOwnerMap := map[string]string{ + "txt.bar.test-zone.example.org": "owner", + "txt.tar.test-zone.example.org": "owner-2", + "foobar.test-zone.example.org": "owner", + } + r, _ := NewTXTRegistry(p, "", "owner") records, _ := r.Records() assert.True(t, testutils.SameEndpoints(records, expectedRecords)) + assert.Equal(t, expectedOwnerMap, r.ownerMap) } func testTXTRegistryApplyChanges(t *testing.T) { @@ -218,49 +197,54 @@ func testTXTRegistryApplyChangesWithPrefix(t *testing.T) { p.CreateZone(testZone) p.ApplyChanges(&plan.Changes{ Create: []*endpoint.Endpoint{ - newEndpointWithOwner("foo.test-zone.example.org", "foo.loadbalancer.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("bar.test-zone.example.org", "my-domain.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("txt.bar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("txt.bar.test-zone.example.org", "baz.test-zone.example.org", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("qux.test-zone.example.org", "random", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("txt.foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + endpoint.NewEndpoint("foo.test-zone.example.org", "foo.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("bar.test-zone.example.org", "my-domain.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("txt.bar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("txt.bar.test-zone.example.org", "baz.test-zone.example.org", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("qux.test-zone.example.org", "random", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("txt.foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), }, }) r, _ := NewTXTRegistry(p, "txt.", "owner") changes := &plan.Changes{ Create: []*endpoint.Endpoint{ - newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", "", ""), + endpoint.NewEndpoint("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", ""), }, Delete: []*endpoint.Endpoint{ - newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), + endpoint.NewEndpoint("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME), }, UpdateNew: []*endpoint.Endpoint{ - newEndpointWithOwner("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), + endpoint.NewEndpoint("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME), }, UpdateOld: []*endpoint.Endpoint{ - newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), + endpoint.NewEndpoint("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME), }, } expected := &plan.Changes{ Create: []*endpoint.Endpoint{ - newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", "", ""), - newEndpointWithOwner("txt.new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + endpoint.NewEndpoint("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", ""), + endpoint.NewEndpoint("txt.new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), }, Delete: []*endpoint.Endpoint{ - newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), - newEndpointWithOwner("txt.foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + endpoint.NewEndpoint("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("txt.foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), }, UpdateNew: []*endpoint.Endpoint{ - newEndpointWithOwner("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), + endpoint.NewEndpoint("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME), }, UpdateOld: []*endpoint.Endpoint{ - newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), + endpoint.NewEndpoint("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME), }, } + expectedOwnerMap := map[string]string{ + "bar.test-zone.example.org": "owner", + "tar.test-zone.example.org": "owner", + "new-record-1.test-zone.example.org": "owner", + } p.OnApplyChanges = func(got *plan.Changes) { mExpected := map[string][]*endpoint.Endpoint{ "Create": expected.Create, @@ -278,6 +262,7 @@ func testTXTRegistryApplyChangesWithPrefix(t *testing.T) { } err := r.ApplyChanges(changes) require.NoError(t, err) + assert.Equal(t, expectedOwnerMap, r.ownerMap) } func testTXTRegistryApplyChangesNoPrefix(t *testing.T) { @@ -285,45 +270,50 @@ func testTXTRegistryApplyChangesNoPrefix(t *testing.T) { p.CreateZone(testZone) p.ApplyChanges(&plan.Changes{ Create: []*endpoint.Endpoint{ - newEndpointWithOwner("foo.test-zone.example.org", "foo.loadbalancer.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("bar.test-zone.example.org", "my-domain.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("txt.bar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("txt.bar.test-zone.example.org", "baz.test-zone.example.org", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("qux.test-zone.example.org", "random", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + endpoint.NewEndpoint("foo.test-zone.example.org", "foo.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("bar.test-zone.example.org", "my-domain.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("txt.bar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("txt.bar.test-zone.example.org", "baz.test-zone.example.org", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("qux.test-zone.example.org", "random", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), }, }) r, _ := NewTXTRegistry(p, "", "owner") changes := &plan.Changes{ Create: []*endpoint.Endpoint{ - newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", "", ""), + endpoint.NewEndpoint("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", ""), }, Delete: []*endpoint.Endpoint{ - newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), + endpoint.NewEndpoint("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME), }, UpdateNew: []*endpoint.Endpoint{ - newEndpointWithOwner("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner-2"), + endpoint.NewEndpoint("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME), }, UpdateOld: []*endpoint.Endpoint{ - newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner-2"), + endpoint.NewEndpoint("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME), }, } expected := &plan.Changes{ Create: []*endpoint.Endpoint{ - newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", "", ""), - newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + endpoint.NewEndpoint("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", ""), + endpoint.NewEndpoint("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), }, Delete: []*endpoint.Endpoint{ - newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), - newEndpointWithOwner("foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + endpoint.NewEndpoint("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), }, UpdateNew: []*endpoint.Endpoint{}, UpdateOld: []*endpoint.Endpoint{}, } + expectedOwnerMap := map[string]string{ + "txt.bar.test-zone.example.org": "owner", + "txt.tar.test-zone.example.org": "owner", + "new-record-1.test-zone.example.org": "owner", + } p.OnApplyChanges = func(got *plan.Changes) { mExpected := map[string][]*endpoint.Endpoint{ "Create": expected.Create, @@ -341,16 +331,5 @@ func testTXTRegistryApplyChangesNoPrefix(t *testing.T) { } err := r.ApplyChanges(changes) require.NoError(t, err) -} - -/** - -helper methods - -*/ - -func newEndpointWithOwner(dnsName, target, recordType, ownerID string) *endpoint.Endpoint { - e := endpoint.NewEndpoint(dnsName, target, recordType) - e.Labels[endpoint.OwnerLabelKey] = ownerID - return e + assert.Equal(t, expectedOwnerMap, r.ownerMap) } From 2531925b44e70d9e562ca19fa90d1c33d6bd841e Mon Sep 17 00:00:00 2001 From: Olaf Klischat Date: Wed, 8 Nov 2017 17:43:10 +0100 Subject: [PATCH 5/8] inmemory provider fixed for multi-target --- provider/inmemory.go | 68 +++++++++++++++++----------- provider/inmemory_test.go | 95 ++++++++++++++++++++++++++++----------- 2 files changed, 111 insertions(+), 52 deletions(-) diff --git a/provider/inmemory.go b/provider/inmemory.go index 7803dc2ee1..59c7381faa 100644 --- a/provider/inmemory.go +++ b/provider/inmemory.go @@ -130,7 +130,7 @@ func (im *InMemoryProvider) Records() ([]*endpoint.Endpoint, error) { } for _, record := range records { - endpoints = append(endpoints, endpoint.NewEndpoint(record.Name, record.Target, record.Type)) + endpoints = append(endpoints, endpoint.NewEndpointWithTTL(record.Name, record.Target, record.Type, record.RecordTTL)) } } @@ -201,9 +201,10 @@ func convertToInMemoryRecord(endpoints []*endpoint.Endpoint) []*inMemoryRecord { records := []*inMemoryRecord{} for _, ep := range endpoints { records = append(records, &inMemoryRecord{ - Type: ep.RecordType, - Name: ep.DNSName, - Target: ep.Target, + Type: ep.RecordType, + Name: ep.DNSName, + Target: ep.Target, + RecordTTL: ep.RecordTTL, }) } return records @@ -241,10 +242,12 @@ func (f *filter) EndpointZoneID(endpoint *endpoint.Endpoint, zones map[string]st // Type - type of record // Name - DNS name assigned to the record // Target - target of the record +// RecordTTL - TTL of the record type inMemoryRecord struct { - Type string - Name string - Target string + Type string + Name string + Target string + RecordTTL endpoint.TTL } type zone map[string][]*inMemoryRecord @@ -303,18 +306,17 @@ func (c *inMemoryClient) ApplyChanges(zoneID string, changes *inMemoryChange) er } c.zones[zoneID][newEndpoint.Name] = append(c.zones[zoneID][newEndpoint.Name], newEndpoint) } - for _, updateEndpoint := range changes.UpdateNew { - for _, rec := range c.zones[zoneID][updateEndpoint.Name] { - if rec.Type == updateEndpoint.Type { - rec.Target = updateEndpoint.Target - break - } + for i, updateOld := range changes.UpdateOld { + if rec := findRecord(updateOld, c.zones[zoneID][updateOld.Name]); rec != nil { + updateNew := changes.UpdateNew[i] + rec.Target = updateNew.Target + rec.RecordTTL = updateNew.RecordTTL } } for _, deleteEndpoint := range changes.Delete { newSet := make([]*inMemoryRecord, 0) for _, rec := range c.zones[zoneID][deleteEndpoint.Name] { - if rec.Type != deleteEndpoint.Type { + if rec.Type != deleteEndpoint.Type || rec.Target != deleteEndpoint.Target { newSet = append(newSet, rec) } } @@ -323,15 +325,29 @@ func (c *inMemoryClient) ApplyChanges(zoneID string, changes *inMemoryChange) er return nil } -func (c *inMemoryClient) updateMesh(mesh map[string]map[string]bool, record *inMemoryRecord) error { +// mesh: map DNSName => map Type => map Target => bool +func (c *inMemoryClient) updateMesh(mesh map[string]map[string]map[string]bool, record *inMemoryRecord) error { if _, exists := mesh[record.Name]; exists { - if mesh[record.Name][record.Type] { - return ErrDuplicateRecordFound + if _, exists := mesh[record.Name][record.Type]; exists { + if mesh[record.Name][record.Type][record.Target] { + return ErrDuplicateRecordFound + } + + mesh[record.Name][record.Type][record.Target] = true + return nil + } + + mesh[record.Name][record.Type] = map[string]bool{ + record.Target: true, } - mesh[record.Name][record.Type] = true return nil } - mesh[record.Name] = map[string]bool{record.Type: true} + + mesh[record.Name] = map[string]map[string]bool{ + record.Type: { + record.Target: true, + }, + } return nil } @@ -341,9 +357,9 @@ func (c *inMemoryClient) validateChangeBatch(zone string, changes *inMemoryChang if !ok { return ErrZoneNotFound } - mesh := map[string]map[string]bool{} + mesh := map[string]map[string]map[string]bool{} for _, newEndpoint := range changes.Create { - if c.findByType(newEndpoint.Type, curZone[newEndpoint.Name]) != nil { + if findRecord(newEndpoint, curZone[newEndpoint.Name]) != nil { return ErrRecordAlreadyExists } if err := c.updateMesh(mesh, newEndpoint); err != nil { @@ -351,7 +367,7 @@ func (c *inMemoryClient) validateChangeBatch(zone string, changes *inMemoryChang } } for _, updateEndpoint := range changes.UpdateNew { - if c.findByType(updateEndpoint.Type, curZone[updateEndpoint.Name]) == nil { + if findRecord(updateEndpoint, curZone[updateEndpoint.Name]) == nil { return ErrRecordNotFound } if err := c.updateMesh(mesh, updateEndpoint); err != nil { @@ -359,12 +375,12 @@ func (c *inMemoryClient) validateChangeBatch(zone string, changes *inMemoryChang } } for _, updateOldEndpoint := range changes.UpdateOld { - if rec := c.findByType(updateOldEndpoint.Type, curZone[updateOldEndpoint.Name]); rec == nil || rec.Target != updateOldEndpoint.Target { + if findRecord(updateOldEndpoint, curZone[updateOldEndpoint.Name]) == nil { return ErrRecordNotFound } } for _, deleteEndpoint := range changes.Delete { - if rec := c.findByType(deleteEndpoint.Type, curZone[deleteEndpoint.Name]); rec == nil || rec.Target != deleteEndpoint.Target { + if findRecord(deleteEndpoint, curZone[deleteEndpoint.Name]) == nil { return ErrRecordNotFound } if err := c.updateMesh(mesh, deleteEndpoint); err != nil { @@ -374,9 +390,9 @@ func (c *inMemoryClient) validateChangeBatch(zone string, changes *inMemoryChang return nil } -func (c *inMemoryClient) findByType(recordType string, records []*inMemoryRecord) *inMemoryRecord { +func findRecord(needle *inMemoryRecord, records []*inMemoryRecord) *inMemoryRecord { for _, record := range records { - if record.Type == recordType { + if record.Type == needle.Type && record.Name == needle.Name && record.Target == needle.Target { return record } } diff --git a/provider/inmemory_test.go b/provider/inmemory_test.go index 53a191258b..0be0e9bae6 100644 --- a/provider/inmemory_test.go +++ b/provider/inmemory_test.go @@ -32,7 +32,7 @@ var ( ) func TestInMemoryProvider(t *testing.T) { - t.Run("findByType", testInMemoryFindByType) + t.Run("findRecord", testInMemoryFindRecord) t.Run("Records", testInMemoryRecords) t.Run("validateChangeBatch", testInMemoryValidateChangeBatch) t.Run("ApplyChanges", testInMemoryApplyChanges) @@ -40,81 +40,119 @@ func TestInMemoryProvider(t *testing.T) { t.Run("CreateZone", testInMemoryCreateZone) } -func testInMemoryFindByType(t *testing.T) { +func testInMemoryFindRecord(t *testing.T) { for _, ti := range []struct { title string - findType string + needle *inMemoryRecord records []*inMemoryRecord expected *inMemoryRecord expectedEmpty bool }{ { title: "no records, empty type", - findType: "", + needle: nil, records: nil, expected: nil, expectedEmpty: true, }, { - title: "no records, non-empty type", - findType: endpoint.RecordTypeA, + title: "no records, non-empty needle", + needle: &inMemoryRecord{ + Type: endpoint.RecordTypeA, + }, records: nil, expected: nil, expectedEmpty: true, }, { - title: "one record, empty type", - findType: "", + title: "one record, empty type", + needle: &inMemoryRecord{ + Type: "", + Name: "foo.com", + Target: "1.1.1.1", + }, records: []*inMemoryRecord{ { - Type: endpoint.RecordTypeA, + Type: endpoint.RecordTypeA, + Name: "foo.com", + Target: "1.1.1.1", }, }, expected: nil, expectedEmpty: true, }, { - title: "one record, wrong type", - findType: endpoint.RecordTypeCNAME, + title: "one record, wrong type", + needle: &inMemoryRecord{ + Type: endpoint.RecordTypeA, + Name: "foo.com", + Target: "1.1.1.1", + }, records: []*inMemoryRecord{ { - Type: endpoint.RecordTypeA, + Type: endpoint.RecordTypeCNAME, + Name: "foo.com", + Target: "1.1.1.1", }, }, expected: nil, expectedEmpty: true, }, { - title: "one record, right type", - findType: endpoint.RecordTypeA, + title: "one record, matching", + needle: &inMemoryRecord{ + Type: endpoint.RecordTypeA, + Name: "foo.com", + Target: "1.1.1.1", + RecordTTL: 100, + }, records: []*inMemoryRecord{ { - Type: endpoint.RecordTypeA, + Type: endpoint.RecordTypeA, + Name: "foo.com", + Target: "1.1.1.1", + RecordTTL: 42, }, }, expected: &inMemoryRecord{ - Type: endpoint.RecordTypeA, + Type: endpoint.RecordTypeA, + Name: "foo.com", + Target: "1.1.1.1", + RecordTTL: 42, }, }, { - title: "multiple records, right type", - findType: endpoint.RecordTypeA, + title: "multiple records, pme matching", + needle: &inMemoryRecord{ + Type: endpoint.RecordTypeA, + Name: "foo.com", + Target: "1.1.1.1", + RecordTTL: 100, + }, records: []*inMemoryRecord{ { - Type: endpoint.RecordTypeA, + Type: endpoint.RecordTypeA, + Name: "foo.com", + Target: "2.2.2.2", + RecordTTL: 42, }, { - Type: endpoint.RecordTypeTXT, + Type: endpoint.RecordTypeA, + Name: "foo.com", + Target: "1.1.1.1", + RecordTTL: 42, }, }, expected: &inMemoryRecord{ - Type: endpoint.RecordTypeA, + Type: endpoint.RecordTypeA, + Name: "foo.com", + Target: "1.1.1.1", + RecordTTL: 42, }, }, } { t.Run(ti.title, func(t *testing.T) { - c := newInMemoryClient() - record := c.findByType(ti.findType, ti.records) + record := findRecord(ti.needle, ti.records) if ti.expectedEmpty { assert.Nil(t, record) } else { @@ -245,6 +283,7 @@ func testInMemoryValidateChangeBatch(t *testing.T) { Name: "foo.bar.org", Target: "5.5.5.5", Type: endpoint.RecordTypeA, + RecordTTL: 50, }, }, }, @@ -519,8 +558,9 @@ func testInMemoryValidateChangeBatch(t *testing.T) { UpdateNew: []*endpoint.Endpoint{ { DNSName: "foo.bar.org", - Target: "4.8.8.4", + Target: "5.5.5.5", RecordType: endpoint.RecordTypeA, + RecordTTL: 100, }, }, UpdateOld: []*endpoint.Endpoint{ @@ -579,6 +619,7 @@ func getInitData() map[string]zone { Name: "foo.bar.org", Target: "5.5.5.5", Type: endpoint.RecordTypeA, + RecordTTL: 50, }, }, }, @@ -704,8 +745,9 @@ func testInMemoryApplyChanges(t *testing.T) { UpdateNew: []*endpoint.Endpoint{ { DNSName: "foo.bar.org", - Target: "4.8.8.4", + Target: "5.5.5.5", RecordType: endpoint.RecordTypeA, + RecordTTL: 100, }, }, UpdateOld: []*endpoint.Endpoint{ @@ -741,8 +783,9 @@ func testInMemoryApplyChanges(t *testing.T) { "foo.bar.org": []*inMemoryRecord{ { Name: "foo.bar.org", - Target: "4.8.8.4", + Target: "5.5.5.5", Type: endpoint.RecordTypeA, + RecordTTL: 100, }, }, "foo.bar.new.org": []*inMemoryRecord{ From 837f376fd2412c7bd703b26127901f5323a6c153 Mon Sep 17 00:00:00 2001 From: Olaf Klischat Date: Wed, 8 Nov 2017 17:43:30 +0100 Subject: [PATCH 6/8] TXTRegistry multi-target tests updateOld/-New can only be used to update an existing record without changing its identifying information, i.e. name and target. so instead of updateOld: foo.com => 1.1.1.1, updateNew: foo.com => 2.2.2.2 one must: delete: foo.com => 1.1.1.1, create: foo.com => 2.2.2.2 --- provider/inmemory_test.go | 22 +-- registry/txt_test.go | 398 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 397 insertions(+), 23 deletions(-) diff --git a/provider/inmemory_test.go b/provider/inmemory_test.go index 0be0e9bae6..680698d1c8 100644 --- a/provider/inmemory_test.go +++ b/provider/inmemory_test.go @@ -280,10 +280,10 @@ func testInMemoryValidateChangeBatch(t *testing.T) { }, "foo.bar.org": []*inMemoryRecord{ { - Name: "foo.bar.org", - Target: "5.5.5.5", - Type: endpoint.RecordTypeA, - RecordTTL: 50, + Name: "foo.bar.org", + Target: "5.5.5.5", + Type: endpoint.RecordTypeA, + RecordTTL: 50, }, }, }, @@ -616,9 +616,9 @@ func getInitData() map[string]zone { }, "foo.bar.org": []*inMemoryRecord{ { - Name: "foo.bar.org", - Target: "5.5.5.5", - Type: endpoint.RecordTypeA, + Name: "foo.bar.org", + Target: "5.5.5.5", + Type: endpoint.RecordTypeA, RecordTTL: 50, }, }, @@ -782,10 +782,10 @@ func testInMemoryApplyChanges(t *testing.T) { }, "foo.bar.org": []*inMemoryRecord{ { - Name: "foo.bar.org", - Target: "5.5.5.5", - Type: endpoint.RecordTypeA, - RecordTTL: 100, + Name: "foo.bar.org", + Target: "5.5.5.5", + Type: endpoint.RecordTypeA, + RecordTTL: 100, }, }, "foo.bar.new.org": []*inMemoryRecord{ diff --git a/registry/txt_test.go b/registry/txt_test.go index 8606e23412..ce91a3d158 100644 --- a/registry/txt_test.go +++ b/registry/txt_test.go @@ -190,6 +190,13 @@ func testTXTRegistryRecordsNoPrefix(t *testing.T) { func testTXTRegistryApplyChanges(t *testing.T) { t.Run("With Prefix", testTXTRegistryApplyChangesWithPrefix) t.Run("No prefix", testTXTRegistryApplyChangesNoPrefix) + t.Run("Multitarget create", testTXTRegistryMultitargetCreate) + t.Run("Multitarget add", testTXTRegistryMultitargetAdd) + t.Run("Multitarget delete", testTXTRegistryMultitargetDelete) + t.Run("Multitarget delete all", testTXTRegistryMultitargetDeleteAll) + t.Run("Multitarget try to add to non-owned cluster", testTXTRegistryMultitargetAddForeign) + t.Run("Multitarget create, update and delete", testTXTRegistryMultitargetCreateDeleteUpdate) + t.Run("Multitarget change target via delete and create", testTXTRegistryMultitargetDeleteAndCreate) } func testTXTRegistryApplyChangesWithPrefix(t *testing.T) { @@ -213,37 +220,39 @@ func testTXTRegistryApplyChangesWithPrefix(t *testing.T) { changes := &plan.Changes{ Create: []*endpoint.Endpoint{ endpoint.NewEndpoint("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", ""), - }, - Delete: []*endpoint.Endpoint{ - endpoint.NewEndpoint("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME), - }, - UpdateNew: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multiadd.test-zone.example.org", "backend1.lb.com", ""), + endpoint.NewEndpoint("multiadd.test-zone.example.org", "backend2.lb.com", ""), endpoint.NewEndpoint("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME), }, - UpdateOld: []*endpoint.Endpoint{ + Delete: []*endpoint.Endpoint{ endpoint.NewEndpoint("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME), + endpoint.NewEndpoint("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME), }, + UpdateNew: []*endpoint.Endpoint{}, + UpdateOld: []*endpoint.Endpoint{}, } expected := &plan.Changes{ Create: []*endpoint.Endpoint{ endpoint.NewEndpoint("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", ""), endpoint.NewEndpoint("txt.new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("multiadd.test-zone.example.org", "backend1.lb.com", ""), + endpoint.NewEndpoint("multiadd.test-zone.example.org", "backend2.lb.com", ""), + endpoint.NewEndpoint("txt.multiadd.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME), }, Delete: []*endpoint.Endpoint{ + endpoint.NewEndpoint("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME), endpoint.NewEndpoint("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME), endpoint.NewEndpoint("txt.foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), }, - UpdateNew: []*endpoint.Endpoint{ - endpoint.NewEndpoint("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME), - }, - UpdateOld: []*endpoint.Endpoint{ - endpoint.NewEndpoint("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME), - }, + UpdateNew: []*endpoint.Endpoint{}, + UpdateOld: []*endpoint.Endpoint{}, } expectedOwnerMap := map[string]string{ "bar.test-zone.example.org": "owner", "tar.test-zone.example.org": "owner", "new-record-1.test-zone.example.org": "owner", + "multiadd.test-zone.example.org": "owner", } p.OnApplyChanges = func(got *plan.Changes) { mExpected := map[string][]*endpoint.Endpoint{ @@ -286,6 +295,8 @@ func testTXTRegistryApplyChangesNoPrefix(t *testing.T) { changes := &plan.Changes{ Create: []*endpoint.Endpoint{ endpoint.NewEndpoint("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", ""), + endpoint.NewEndpoint("multiadd.test-zone.example.org", "backend1.lb.com", ""), + endpoint.NewEndpoint("multiadd.test-zone.example.org", "backend2.lb.com", ""), }, Delete: []*endpoint.Endpoint{ endpoint.NewEndpoint("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME), @@ -301,6 +312,9 @@ func testTXTRegistryApplyChangesNoPrefix(t *testing.T) { Create: []*endpoint.Endpoint{ endpoint.NewEndpoint("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", ""), endpoint.NewEndpoint("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("multiadd.test-zone.example.org", "backend1.lb.com", ""), + endpoint.NewEndpoint("multiadd.test-zone.example.org", "backend2.lb.com", ""), + endpoint.NewEndpoint("multiadd.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), }, Delete: []*endpoint.Endpoint{ endpoint.NewEndpoint("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME), @@ -313,6 +327,7 @@ func testTXTRegistryApplyChangesNoPrefix(t *testing.T) { "txt.bar.test-zone.example.org": "owner", "txt.tar.test-zone.example.org": "owner", "new-record-1.test-zone.example.org": "owner", + "multiadd.test-zone.example.org": "owner", } p.OnApplyChanges = func(got *plan.Changes) { mExpected := map[string][]*endpoint.Endpoint{ @@ -333,3 +348,362 @@ func testTXTRegistryApplyChangesNoPrefix(t *testing.T) { require.NoError(t, err) assert.Equal(t, expectedOwnerMap, r.ownerMap) } + +func testTXTRegistryMultitargetCreate(t *testing.T) { + p := provider.NewInMemoryProvider() + p.CreateZone(testZone) + + r, _ := NewTXTRegistry(p, "", "owner") + + changes := &plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + }, + } + expected := &plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + }, + } + p.OnApplyChanges = func(got *plan.Changes) { + mExpected := map[string][]*endpoint.Endpoint{ + "Create": expected.Create, + "UpdateNew": expected.UpdateNew, + "UpdateOld": expected.UpdateOld, + "Delete": expected.Delete, + } + mGot := map[string][]*endpoint.Endpoint{ + "Create": got.Create, + "UpdateNew": got.UpdateNew, + "UpdateOld": got.UpdateOld, + "Delete": got.Delete, + } + assert.True(t, testutils.SamePlanChanges(mGot, mExpected)) + } + err := r.ApplyChanges(changes) + require.NoError(t, err) +} + +func testTXTRegistryMultitargetAdd(t *testing.T) { + p := provider.NewInMemoryProvider() + p.CreateZone(testZone) + p.ApplyChanges(&plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + }, + }) + r, _ := NewTXTRegistry(p, "", "owner") + + changes := &plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "3.3.3.3", endpoint.RecordTypeA), + }, + } + expected := &plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "3.3.3.3", endpoint.RecordTypeA), + }, + } + p.OnApplyChanges = func(got *plan.Changes) { + mExpected := map[string][]*endpoint.Endpoint{ + "Create": expected.Create, + "UpdateNew": expected.UpdateNew, + "UpdateOld": expected.UpdateOld, + "Delete": expected.Delete, + } + mGot := map[string][]*endpoint.Endpoint{ + "Create": got.Create, + "UpdateNew": got.UpdateNew, + "UpdateOld": got.UpdateOld, + "Delete": got.Delete, + } + assert.True(t, testutils.SamePlanChanges(mGot, mExpected)) + } + + err := r.ApplyChanges(changes) + require.NoError(t, err) + + finalRecords, err := r.Records() + require.NoError(t, err) + assert.True(t, testutils.SameEndpoints(finalRecords, []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "3.3.3.3", endpoint.RecordTypeA), + })) +} + +func testTXTRegistryMultitargetDelete(t *testing.T) { + p := provider.NewInMemoryProvider() + p.CreateZone(testZone) + p.ApplyChanges(&plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "3.3.3.3", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + }, + }) + r, _ := NewTXTRegistry(p, "", "owner") + + changes := &plan.Changes{ + Delete: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + }, + } + expected := &plan.Changes{ + Delete: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + }, + } + p.OnApplyChanges = func(got *plan.Changes) { + mExpected := map[string][]*endpoint.Endpoint{ + "Create": expected.Create, + "UpdateNew": expected.UpdateNew, + "UpdateOld": expected.UpdateOld, + "Delete": expected.Delete, + } + mGot := map[string][]*endpoint.Endpoint{ + "Create": got.Create, + "UpdateNew": got.UpdateNew, + "UpdateOld": got.UpdateOld, + "Delete": got.Delete, + } + assert.True(t, testutils.SamePlanChanges(mGot, mExpected)) + } + + err := r.ApplyChanges(changes) + require.NoError(t, err) + + finalRecords, err := r.Records() + require.NoError(t, err) + assert.True(t, testutils.SameEndpoints(finalRecords, []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "3.3.3.3", endpoint.RecordTypeA), + })) +} + +func testTXTRegistryMultitargetDeleteAll(t *testing.T) { + p := provider.NewInMemoryProvider() + p.CreateZone(testZone) + p.ApplyChanges(&plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + }, + }) + r, _ := NewTXTRegistry(p, "", "owner") + + changes := &plan.Changes{ + Delete: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + }, + } + expected := &plan.Changes{ + Delete: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + }, + } + p.OnApplyChanges = func(got *plan.Changes) { + mExpected := map[string][]*endpoint.Endpoint{ + "Create": expected.Create, + "UpdateNew": expected.UpdateNew, + "UpdateOld": expected.UpdateOld, + "Delete": expected.Delete, + } + mGot := map[string][]*endpoint.Endpoint{ + "Create": got.Create, + "UpdateNew": got.UpdateNew, + "UpdateOld": got.UpdateOld, + "Delete": got.Delete, + } + assert.True(t, testutils.SamePlanChanges(mGot, mExpected)) + } + + err := r.ApplyChanges(changes) + require.NoError(t, err) + + finalRecords, err := r.Records() + require.NoError(t, err) + assert.True(t, testutils.SameEndpoints(finalRecords, []*endpoint.Endpoint{})) +} + +func testTXTRegistryMultitargetAddForeign(t *testing.T) { + p := provider.NewInMemoryProvider() + p.CreateZone(testZone) + p.ApplyChanges(&plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=notme\"", endpoint.RecordTypeTXT), + }, + }) + r, _ := NewTXTRegistry(p, "", "owner") + + changes := &plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "3.3.3.3", endpoint.RecordTypeA), + }, + } + expected := &plan.Changes{ + Create: []*endpoint.Endpoint{}, + } + p.OnApplyChanges = func(got *plan.Changes) { + mExpected := map[string][]*endpoint.Endpoint{ + "Create": expected.Create, + "UpdateNew": expected.UpdateNew, + "UpdateOld": expected.UpdateOld, + "Delete": expected.Delete, + } + mGot := map[string][]*endpoint.Endpoint{ + "Create": got.Create, + "UpdateNew": got.UpdateNew, + "UpdateOld": got.UpdateOld, + "Delete": got.Delete, + } + assert.True(t, testutils.SamePlanChanges(mGot, mExpected)) + } + + err := r.ApplyChanges(changes) + require.NoError(t, err) + + finalRecords, err := r.Records() + require.NoError(t, err) + assert.True(t, testutils.SameEndpoints(finalRecords, []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + })) +} + +func testTXTRegistryMultitargetCreateDeleteUpdate(t *testing.T) { + p := provider.NewInMemoryProvider() + p.CreateZone(testZone) + p.ApplyChanges(&plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpointWithTTL("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA, 50), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + endpoint.NewEndpoint("target2.test-zone.example.org", "9.9.9.9", endpoint.RecordTypeA), + endpoint.NewEndpoint("target2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + }, + }) + r, _ := NewTXTRegistry(p, "", "owner") + + changes := &plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "3.3.3.3", endpoint.RecordTypeA), + }, + UpdateOld: []*endpoint.Endpoint{ + endpoint.NewEndpointWithTTL("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA, 50), + }, + UpdateNew: []*endpoint.Endpoint{ + endpoint.NewEndpointWithTTL("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA, 100), + }, + Delete: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("target2.test-zone.example.org", "9.9.9.9", endpoint.RecordTypeA), + }, + } + expected := &plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "3.3.3.3", endpoint.RecordTypeA), + }, + UpdateOld: []*endpoint.Endpoint{ + endpoint.NewEndpointWithTTL("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA, 50), + }, + UpdateNew: []*endpoint.Endpoint{ + endpoint.NewEndpointWithTTL("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA, 100), + }, + Delete: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("target2.test-zone.example.org", "9.9.9.9", endpoint.RecordTypeA), + endpoint.NewEndpoint("target2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + }, + } + p.OnApplyChanges = func(got *plan.Changes) { + mExpected := map[string][]*endpoint.Endpoint{ + "Create": expected.Create, + "UpdateNew": expected.UpdateNew, + "UpdateOld": expected.UpdateOld, + "Delete": expected.Delete, + } + mGot := map[string][]*endpoint.Endpoint{ + "Create": got.Create, + "UpdateNew": got.UpdateNew, + "UpdateOld": got.UpdateOld, + "Delete": got.Delete, + } + assert.True(t, testutils.SamePlanChanges(mGot, mExpected)) + } + + err := r.ApplyChanges(changes) + require.NoError(t, err) + + finalRecords, err := r.Records() + require.NoError(t, err) + assert.True(t, testutils.SameEndpoints(finalRecords, []*endpoint.Endpoint{ + endpoint.NewEndpointWithTTL("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA, 100), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "3.3.3.3", endpoint.RecordTypeA), + })) +} + +func testTXTRegistryMultitargetDeleteAndCreate(t *testing.T) { + p := provider.NewInMemoryProvider() + p.CreateZone(testZone) + p.ApplyChanges(&plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT), + }, + }) + r, _ := NewTXTRegistry(p, "", "owner") + + changes := &plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + }, + Delete: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + }, + } + expected := &plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + }, + Delete: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + }, + } + p.OnApplyChanges = func(got *plan.Changes) { + mExpected := map[string][]*endpoint.Endpoint{ + "Create": expected.Create, + "UpdateNew": expected.UpdateNew, + "UpdateOld": expected.UpdateOld, + "Delete": expected.Delete, + } + mGot := map[string][]*endpoint.Endpoint{ + "Create": got.Create, + "UpdateNew": got.UpdateNew, + "UpdateOld": got.UpdateOld, + "Delete": got.Delete, + } + assert.True(t, testutils.SamePlanChanges(mGot, mExpected)) + } + + err := r.ApplyChanges(changes) + require.NoError(t, err) + + finalRecords, err := r.Records() + require.NoError(t, err) + assert.True(t, testutils.SameEndpoints(finalRecords, []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + })) +} From 6ac1b1ab73e0729e7cfaa50424ac4b01a82fde9e Mon Sep 17 00:00:00 2001 From: Olaf Klischat Date: Tue, 7 Nov 2017 12:42:32 +0100 Subject: [PATCH 7/8] TestNoopRegistry fixed for multi-target --- registry/noop_test.go | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/registry/noop_test.go b/registry/noop_test.go index b4d0aa7d8b..db682eb3c4 100644 --- a/registry/noop_test.go +++ b/registry/noop_test.go @@ -71,20 +71,29 @@ func testNoopApplyChanges(t *testing.T) { providerRecords := []*endpoint.Endpoint{ { DNSName: "example.org", - Target: "old-lb.com", + Target: "example-lb.com", RecordType: endpoint.RecordTypeCNAME, + RecordTTL: 50, }, } expectedUpdate := []*endpoint.Endpoint{ { DNSName: "example.org", - Target: "new-example-lb.com", + Target: "example-lb.com", + RecordType: endpoint.RecordTypeCNAME, + RecordTTL: 150, + }, + { + DNSName: "example.org", + Target: "example-lb-2.com", RecordType: endpoint.RecordTypeCNAME, + RecordTTL: 50, }, { DNSName: "new-record.org", Target: "new-lb.org", RecordType: endpoint.RecordTypeCNAME, + RecordTTL: 42, }, } @@ -98,8 +107,9 @@ func testNoopApplyChanges(t *testing.T) { Create: []*endpoint.Endpoint{ { DNSName: "example.org", - Target: "lb.com", + Target: "example-lb.com", RecordType: endpoint.RecordTypeCNAME, + RecordTTL: 100, }, }, }) @@ -108,24 +118,33 @@ func testNoopApplyChanges(t *testing.T) { //correct changes require.NoError(t, r.ApplyChanges(&plan.Changes{ Create: []*endpoint.Endpoint{ + { + DNSName: "example.org", + Target: "example-lb-2.com", + RecordType: endpoint.RecordTypeCNAME, + RecordTTL: 50, + }, { DNSName: "new-record.org", Target: "new-lb.org", RecordType: endpoint.RecordTypeCNAME, + RecordTTL: 42, }, }, UpdateNew: []*endpoint.Endpoint{ { DNSName: "example.org", - Target: "new-example-lb.com", + Target: "example-lb.com", RecordType: endpoint.RecordTypeCNAME, + RecordTTL: 150, }, }, UpdateOld: []*endpoint.Endpoint{ { DNSName: "example.org", - Target: "old-lb.com", + Target: "example-lb.com", RecordType: endpoint.RecordTypeCNAME, + RecordTTL: 50, }, }, })) From 612236d28c070ec8b90c19550c6e3bedefdf797d Mon Sep 17 00:00:00 2001 From: Olaf Klischat Date: Wed, 8 Nov 2017 15:30:14 +0100 Subject: [PATCH 8/8] TXTRegistry: do not create records if the name existed already without an owner record --- registry/txt.go | 2 +- registry/txt_test.go | 47 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/registry/txt.go b/registry/txt.go index 33e930350e..a827c435bb 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -119,7 +119,7 @@ func (im *TXTRegistry) ApplyChanges(changes *plan.Changes) error { for _, r := range changes.Create { rOwner := im.ownerMap[r.DNSName] - if rOwner == "" || rOwner == im.ownerID { + if rOwner == im.ownerID || im.recCount[r.DNSName] == 0 { // only change anything if we own r.DNSName or it doesn't exist yet filteredChanges.Create = append(filteredChanges.Create, r) im.ownerMap[r.DNSName] = im.ownerID diff --git a/registry/txt_test.go b/registry/txt_test.go index ce91a3d158..3ebaa9f058 100644 --- a/registry/txt_test.go +++ b/registry/txt_test.go @@ -195,6 +195,7 @@ func testTXTRegistryApplyChanges(t *testing.T) { t.Run("Multitarget delete", testTXTRegistryMultitargetDelete) t.Run("Multitarget delete all", testTXTRegistryMultitargetDeleteAll) t.Run("Multitarget try to add to non-owned cluster", testTXTRegistryMultitargetAddForeign) + t.Run("Multitarget try to add to unowned cluster", testTXTRegistryMultitargetAddUnowned) t.Run("Multitarget create, update and delete", testTXTRegistryMultitargetCreateDeleteUpdate) t.Run("Multitarget change target via delete and create", testTXTRegistryMultitargetDeleteAndCreate) } @@ -583,6 +584,52 @@ func testTXTRegistryMultitargetAddForeign(t *testing.T) { })) } +func testTXTRegistryMultitargetAddUnowned(t *testing.T) { + p := provider.NewInMemoryProvider() + p.CreateZone(testZone) + p.ApplyChanges(&plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + }, + }) + r, _ := NewTXTRegistry(p, "", "owner") + + changes := &plan.Changes{ + Create: []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "3.3.3.3", endpoint.RecordTypeA), + }, + } + expected := &plan.Changes{ + Create: []*endpoint.Endpoint{}, + } + p.OnApplyChanges = func(got *plan.Changes) { + mExpected := map[string][]*endpoint.Endpoint{ + "Create": expected.Create, + "UpdateNew": expected.UpdateNew, + "UpdateOld": expected.UpdateOld, + "Delete": expected.Delete, + } + mGot := map[string][]*endpoint.Endpoint{ + "Create": got.Create, + "UpdateNew": got.UpdateNew, + "UpdateOld": got.UpdateOld, + "Delete": got.Delete, + } + assert.True(t, testutils.SamePlanChanges(mGot, mExpected)) + } + + err := r.ApplyChanges(changes) + require.NoError(t, err) + + finalRecords, err := r.Records() + require.NoError(t, err) + assert.True(t, testutils.SameEndpoints(finalRecords, []*endpoint.Endpoint{ + endpoint.NewEndpoint("multitarget.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA), + endpoint.NewEndpoint("multitarget.test-zone.example.org", "2.2.2.2", endpoint.RecordTypeA), + })) +} + func testTXTRegistryMultitargetCreateDeleteUpdate(t *testing.T) { p := provider.NewInMemoryProvider() p.CreateZone(testZone)