From e9a022bd21b323c3e46868aff10dbe15b888ca0e Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Sat, 24 May 2025 18:33:49 +0900 Subject: [PATCH 01/13] fix(txt-registry): skip creation of already-existing TXT records (#4914) --- registry/txt.go | 49 +++++++++++++++++++++++++++++++++++++++++++- registry/txt_test.go | 40 ++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/registry/txt.go b/registry/txt.go index 06a8314a78..acf94adc31 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -19,6 +19,7 @@ package registry import ( "context" "errors" + "fmt" "strings" "time" @@ -60,6 +61,46 @@ type TXTRegistry struct { txtEncryptAESKey []byte newFormatOnly bool + + // existingTXT caches the TXT records that already exist in the zone so that + // ApplyChanges() can skip re-creating them. See the struct below for details. + existingTXT txtCache +} + +// txtCache stores pre‑existing TXT records to avoid duplicate creation. +// It relies on the fact that Records() is always called **before** ApplyChanges() +// within a single reconciliation cycle. The "synced" flag prevents accidental +// use before the cache is filled. +type txtCache struct { + entries []*endpoint.Endpoint + synced bool +} + +// filterExistingTXT removes endpoints whose TXT companions are already present +// in the provider. Caller must guarantee that c.synced == true. +func (r *TXTRegistry) filterExistingTXT(eps []*endpoint.Endpoint) []*endpoint.Endpoint { + if !r.existingTXT.synced { + // If we haven't synced the cache yet, we need to call Records() to populate the cache. + // This is generally not executed because Records() is always called before ApplyChanges() + // in the same reconciliation cycle. + _, _ = r.Records(context.Background()) + } + + // Build a lookup table of existing TXT keys. + table := make(map[string]struct{}, len(r.existingTXT.entries)) + for _, rec := range r.existingTXT.entries { + k := fmt.Sprintf("%s|%s|%s", rec.DNSName, rec.RecordType, rec.SetIdentifier) + table[k] = struct{}{} + } + + var out []*endpoint.Endpoint + for _, ep := range eps { + k := fmt.Sprintf("%s|%s|%s", ep.DNSName, ep.RecordType, ep.SetIdentifier) + if _, found := table[k]; !found { + out = append(out, ep) + } + } + return out } // NewTXTRegistry returns a new TXTRegistry object. When newFormatOnly is true, it will only @@ -104,6 +145,7 @@ func NewTXTRegistry(provider provider.Provider, txtPrefix, txtSuffix, ownerID st txtEncryptEnabled: txtEncryptEnabled, txtEncryptAESKey: txtEncryptAESKey, newFormatOnly: newFormatOnly, + existingTXT: txtCache{}, }, nil } @@ -123,6 +165,7 @@ func (im *TXTRegistry) OwnerID() string { // If TXT records was created previously to indicate ownership its corresponding value // will be added to the endpoints Labels map func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error) { + im.existingTXT.synced = true // If we have the zones cached AND we have refreshed the cache since the // last given interval, then just use the cached results. if im.recordsCache != nil && time.Since(im.recordsCacheRefreshTime) < im.cacheInterval { @@ -171,6 +214,8 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error } labelMap[key] = labels txtRecordsMap[record.DNSName] = struct{}{} + + im.existingTXT.entries = append(im.existingTXT.entries, record) } for _, ep := range endpoints { @@ -279,7 +324,9 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) } r.Labels[endpoint.OwnerLabelKey] = im.ownerID - filteredChanges.Create = append(filteredChanges.Create, im.generateTXTRecord(r)...) + generatedTXTRecords := im.generateTXTRecord(r) + + filteredChanges.Create = append(filteredChanges.Create, im.filterExistingTXT(generatedTXTRecords)...) if im.cacheInterval > 0 { im.addToCache(r) diff --git a/registry/txt_test.go b/registry/txt_test.go index bafacce542..197923398d 100644 --- a/registry/txt_test.go +++ b/registry/txt_test.go @@ -1838,3 +1838,43 @@ func TestTXTRegistryRecordsWithEmptyTargets(t *testing.T) { testutils.TestHelperLogContains("TXT record has no targets empty-targets.test-zone.example.org", hook, t) } + +// TestTXTRegistryRecreatesMissingDataRecords reproduces issue #4914. +// It verifies that External‑DNS recreates A/CNAME records that were accidentally deleted while their companion TXT records remain. +// An InMemoryProvider is used because, like Route53, it throws an error when attempting to create a duplicate record. +func TestTXTRegistryRecreatesMissingDataRecords(t *testing.T) { + ctx := context.Background() + p := inmemory.NewInMemoryProvider() + + // Data records that disappeared from the zone (to be recreated). + missingDataRecords := []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ""), + newEndpointWithOwner("new-record-2.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ""), + } + + // TXT records that survived the accidental deletion of data records. + existingTXTRecords := []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwner("cname-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwner("new-record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwner("a-new-record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + } + + p.CreateZone(testZone) + + err := p.ApplyChanges(ctx, &plan.Changes{Create: existingTXTRecords}) + assert.NoError(t, err) + + registry, err := NewTXTRegistry(p, "", "", "owner", time.Hour, "", nil, nil, false, nil, false) + assert.NoError(t, err) + + // Reconciliation attempts to recreate the missing data records. + err = registry.ApplyChanges(ctx, &plan.Changes{Create: missingDataRecords}) + assert.NoError(t, err) + + records, err := p.Records(ctx) + assert.NoError(t, err) + + expectedRecords := append(missingDataRecords, existingTXTRecords...) + assert.True(t, testutils.SameEndpoints(records, expectedRecords)) +} From 61e4d499456df90a16139f50f5fb0d16cc68249b Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Sun, 25 May 2025 19:32:51 +0900 Subject: [PATCH 02/13] refactor(registry): store existing TXT records temporarily between Records and ApplyChanges --- registry/txt.go | 57 ++++++++++++++++++++++++-------------------- registry/txt_test.go | 1 + 2 files changed, 32 insertions(+), 26 deletions(-) diff --git a/registry/txt.go b/registry/txt.go index acf94adc31..7ba777cad6 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -20,6 +20,8 @@ import ( "context" "errors" "fmt" + + //"fmt" "strings" "time" @@ -62,40 +64,45 @@ type TXTRegistry struct { newFormatOnly bool - // existingTXT caches the TXT records that already exist in the zone so that + // existingTXTs is the TXT records that already exist in the zone so that // ApplyChanges() can skip re-creating them. See the struct below for details. - existingTXT txtCache + existingTXTs existingTXTs } -// txtCache stores pre‑existing TXT records to avoid duplicate creation. +// existingTXTs stores pre‑existing TXT records to avoid duplicate creation. // It relies on the fact that Records() is always called **before** ApplyChanges() -// within a single reconciliation cycle. The "synced" flag prevents accidental -// use before the cache is filled. -type txtCache struct { +// within a single reconciliation cycle. +type existingTXTs struct { entries []*endpoint.Endpoint - synced bool } -// filterExistingTXT removes endpoints whose TXT companions are already present -// in the provider. Caller must guarantee that c.synced == true. -func (r *TXTRegistry) filterExistingTXT(eps []*endpoint.Endpoint) []*endpoint.Endpoint { - if !r.existingTXT.synced { - // If we haven't synced the cache yet, we need to call Records() to populate the cache. - // This is generally not executed because Records() is always called before ApplyChanges() - // in the same reconciliation cycle. - _, _ = r.Records(context.Background()) +func newExistingTXTs() existingTXTs { + return existingTXTs{ + entries: make([]*endpoint.Endpoint, 0), } +} +func (im *existingTXTs) add(r *endpoint.Endpoint) { + im.entries = append(im.entries, r) +} + +// filterOutExistingTXTRecords removes endpoints whose TXT companions are already present +func (im *existingTXTs) filterOutExistingTXTRecords(eps []*endpoint.Endpoint) []*endpoint.Endpoint { // Build a lookup table of existing TXT keys. - table := make(map[string]struct{}, len(r.existingTXT.entries)) - for _, rec := range r.existingTXT.entries { - k := fmt.Sprintf("%s|%s|%s", rec.DNSName, rec.RecordType, rec.SetIdentifier) + table := make(map[string]struct{}, len(im.entries)) + for _, rec := range im.entries { + k := fmt.Sprintf("%s|%s", rec.DNSName, rec.SetIdentifier) table[k] = struct{}{} } var out []*endpoint.Endpoint for _, ep := range eps { - k := fmt.Sprintf("%s|%s|%s", ep.DNSName, ep.RecordType, ep.SetIdentifier) + if ep.RecordType != endpoint.RecordTypeTXT { + // Only filter TXT records. + out = append(out, ep) + continue + } + k := fmt.Sprintf("%s|%s", ep.DNSName, ep.SetIdentifier) if _, found := table[k]; !found { out = append(out, ep) } @@ -145,7 +152,7 @@ func NewTXTRegistry(provider provider.Provider, txtPrefix, txtSuffix, ownerID st txtEncryptEnabled: txtEncryptEnabled, txtEncryptAESKey: txtEncryptAESKey, newFormatOnly: newFormatOnly, - existingTXT: txtCache{}, + existingTXTs: existingTXTs{}, }, nil } @@ -165,7 +172,6 @@ func (im *TXTRegistry) OwnerID() string { // If TXT records was created previously to indicate ownership its corresponding value // will be added to the endpoints Labels map func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error) { - im.existingTXT.synced = true // If we have the zones cached AND we have refreshed the cache since the // last given interval, then just use the cached results. if im.recordsCache != nil && time.Since(im.recordsCacheRefreshTime) < im.cacheInterval { @@ -214,8 +220,7 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error } labelMap[key] = labels txtRecordsMap[record.DNSName] = struct{}{} - - im.existingTXT.entries = append(im.existingTXT.entries, record) + im.existingTXTs.add(record) } for _, ep := range endpoints { @@ -324,14 +329,14 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) } r.Labels[endpoint.OwnerLabelKey] = im.ownerID - generatedTXTRecords := im.generateTXTRecord(r) - - filteredChanges.Create = append(filteredChanges.Create, im.filterExistingTXT(generatedTXTRecords)...) + filteredChanges.Create = append(filteredChanges.Create, im.generateTXTRecord(r)...) if im.cacheInterval > 0 { im.addToCache(r) } } + filteredChanges.Create = im.existingTXTs.filterOutExistingTXTRecords(filteredChanges.Create) + im.existingTXTs = newExistingTXTs() // reset existing TXTs for the next reconciliation cycle for _, r := range filteredChanges.Delete { // when we delete TXT records for which value has changed (due to new label) this would still work because diff --git a/registry/txt_test.go b/registry/txt_test.go index 197923398d..9ad90ecc80 100644 --- a/registry/txt_test.go +++ b/registry/txt_test.go @@ -1867,6 +1867,7 @@ func TestTXTRegistryRecreatesMissingDataRecords(t *testing.T) { registry, err := NewTXTRegistry(p, "", "", "owner", time.Hour, "", nil, nil, false, nil, false) assert.NoError(t, err) + registry.Records(ctx) // Reconciliation attempts to recreate the missing data records. err = registry.ApplyChanges(ctx, &plan.Changes{Create: missingDataRecords}) From b660bc61755f0642ca5c8c126c5e57ff85beedb9 Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Mon, 26 May 2025 07:15:27 +0900 Subject: [PATCH 03/13] refactor: apply review suggestion for code cleanup --- registry/txt.go | 53 ++++++++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 29 deletions(-) diff --git a/registry/txt.go b/registry/txt.go index 7ba777cad6..9a56e004cd 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -21,7 +21,6 @@ import ( "errors" "fmt" - //"fmt" "strings" "time" @@ -73,41 +72,31 @@ type TXTRegistry struct { // It relies on the fact that Records() is always called **before** ApplyChanges() // within a single reconciliation cycle. type existingTXTs struct { - entries []*endpoint.Endpoint + entries map[string]struct{} } func newExistingTXTs() existingTXTs { return existingTXTs{ - entries: make([]*endpoint.Endpoint, 0), + entries: make(map[string]struct{}), } } func (im *existingTXTs) add(r *endpoint.Endpoint) { - im.entries = append(im.entries, r) + if im.entries == nil { + im.entries = make(map[string]struct{}) + } + im.entries[fmt.Sprintf("%s|%s", r.DNSName, r.SetIdentifier)] = struct{}{} } // filterOutExistingTXTRecords removes endpoints whose TXT companions are already present -func (im *existingTXTs) filterOutExistingTXTRecords(eps []*endpoint.Endpoint) []*endpoint.Endpoint { - // Build a lookup table of existing TXT keys. - table := make(map[string]struct{}, len(im.entries)) - for _, rec := range im.entries { - k := fmt.Sprintf("%s|%s", rec.DNSName, rec.SetIdentifier) - table[k] = struct{}{} - } - - var out []*endpoint.Endpoint - for _, ep := range eps { - if ep.RecordType != endpoint.RecordTypeTXT { - // Only filter TXT records. - out = append(out, ep) - continue - } - k := fmt.Sprintf("%s|%s", ep.DNSName, ep.SetIdentifier) - if _, found := table[k]; !found { - out = append(out, ep) - } - } - return out +func (im *existingTXTs) isManaged(ep *endpoint.Endpoint) bool { + k := fmt.Sprintf("%s|%s", ep.DNSName, ep.SetIdentifier) + _, ok := im.entries[k] + return ok +} + +func (im *existingTXTs) reset() existingTXTs { + return newExistingTXTs() } // NewTXTRegistry returns a new TXTRegistry object. When newFormatOnly is true, it will only @@ -152,7 +141,7 @@ func NewTXTRegistry(provider provider.Provider, txtPrefix, txtSuffix, ownerID st txtEncryptEnabled: txtEncryptEnabled, txtEncryptAESKey: txtEncryptAESKey, newFormatOnly: newFormatOnly, - existingTXTs: existingTXTs{}, + existingTXTs: newExistingTXTs(), }, nil } @@ -329,14 +318,20 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) } r.Labels[endpoint.OwnerLabelKey] = im.ownerID - filteredChanges.Create = append(filteredChanges.Create, im.generateTXTRecord(r)...) + generatedTXTs := im.generateTXTRecord(r) + for _, txt := range generatedTXTs { + // If the TXT record is already managed by this instance, skip it + if im.existingTXTs.isManaged(txt) { + continue + } + filteredChanges.Create = append(filteredChanges.Create, txt) + } if im.cacheInterval > 0 { im.addToCache(r) } } - filteredChanges.Create = im.existingTXTs.filterOutExistingTXTRecords(filteredChanges.Create) - im.existingTXTs = newExistingTXTs() // reset existing TXTs for the next reconciliation cycle + im.existingTXTs = im.existingTXTs.reset() // reset existing TXTs for the next reconciliation loop for _, r := range filteredChanges.Delete { // when we delete TXT records for which value has changed (due to new label) this would still work because From 596bb2d19d50f759a339b24f2165c588635b8bd6 Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Mon, 26 May 2025 20:14:25 +0900 Subject: [PATCH 04/13] test: add coverage for recreating A/CNAME records when TXT exists --- registry/txt_test.go | 128 +++++++++++++++++++++++++++++++------------ 1 file changed, 92 insertions(+), 36 deletions(-) diff --git a/registry/txt_test.go b/registry/txt_test.go index 9ad90ecc80..fd53eedd50 100644 --- a/registry/txt_test.go +++ b/registry/txt_test.go @@ -1839,43 +1839,99 @@ func TestTXTRegistryRecordsWithEmptyTargets(t *testing.T) { testutils.TestHelperLogContains("TXT record has no targets empty-targets.test-zone.example.org", hook, t) } -// TestTXTRegistryRecreatesMissingDataRecords reproduces issue #4914. -// It verifies that External‑DNS recreates A/CNAME records that were accidentally deleted while their companion TXT records remain. +// TestTXTRegistryRecreatesMissingRecords reproduces issue #4914. +// It verifies that External‑DNS recreates A/CNAME records that were accidentally deleted while their corresponding TXT records remain. // An InMemoryProvider is used because, like Route53, it throws an error when attempting to create a duplicate record. -func TestTXTRegistryRecreatesMissingDataRecords(t *testing.T) { - ctx := context.Background() - p := inmemory.NewInMemoryProvider() - - // Data records that disappeared from the zone (to be recreated). - missingDataRecords := []*endpoint.Endpoint{ - newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("new-record-2.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ""), - } +func TestTXTRegistryRecreatesMissingRecords(t *testing.T) { + ownerId := "owner" + tests := []struct { + name string + desired []*endpoint.Endpoint + existing []*endpoint.Endpoint + expectedCreate []*endpoint.Endpoint + }{ + { + name: "Recreate missing A record when TXT exists", + desired: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ""), + }, + existing: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + newEndpointWithOwner("a-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + }, + expectedCreate: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ownerId), + }, + }, + { + name: "Recreate missing CNAME record when TXT exists", + desired: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ""), + }, + existing: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + newEndpointWithOwner("cname-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + }, + expectedCreate: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ownerId)}, + }, + { + name: "Recreate missing A and CNAME records when TXT exists", + desired: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ""), + newEndpointWithOwner("new-record-2.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ""), + }, + existing: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + newEndpointWithOwner("new-record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + newEndpointWithOwner("a-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + newEndpointWithOwner("cname-new-record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + }, + expectedCreate: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ownerId), + newEndpointWithOwner("new-record-2.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ownerId), + }, + }, + // TODO: Test TXT record regeneration when only A/CNAME records exist. + // The regeneration logic will be introduced in a separate PR. + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Given: Register existing TXT records + ctx := context.Background() + p := inmemory.NewInMemoryProvider() + p.CreateZone(testZone) + err := p.ApplyChanges(ctx, &plan.Changes{Create: tt.existing}) + assert.NoError(t, err) + p.OnApplyChanges = func(ctx context.Context, changes *plan.Changes) { + assert.True(t, + testutils.SameEndpoints(changes.Create, tt.expectedCreate), + "Expected create changes: %v, but got: %v", tt.expectedCreate, changes.Create, + ) + } - // TXT records that survived the accidental deletion of data records. - existingTXTRecords := []*endpoint.Endpoint{ - newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("cname-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("new-record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("a-new-record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + // When: Apply changes to recreate missing A records + managedRecords := []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME, endpoint.RecordTypeTXT} + registry, err := NewTXTRegistry(p, "", "", ownerId, time.Hour, "", managedRecords, nil, false, nil, false) + assert.NoError(t, err) + records, err := registry.Records(ctx) + assert.NoError(t, err) + plan := &plan.Plan{ + Policies: []plan.Policy{&plan.SyncPolicy{}}, + Current: records, + Desired: tt.desired, + ManagedRecords: managedRecords, + OwnerID: ownerId, + } + plan = plan.Calculate() + err = registry.ApplyChanges(ctx, plan.Changes) + assert.NoError(t, err) + + // Then: Verify that the missing records are recreated + records, err = p.Records(ctx) + assert.NoError(t, err) + expectedRecords := append(tt.existing, tt.expectedCreate...) + assert.True(t, testutils.SameEndpoints(records, expectedRecords)) + }) } - - p.CreateZone(testZone) - - err := p.ApplyChanges(ctx, &plan.Changes{Create: existingTXTRecords}) - assert.NoError(t, err) - - registry, err := NewTXTRegistry(p, "", "", "owner", time.Hour, "", nil, nil, false, nil, false) - assert.NoError(t, err) - registry.Records(ctx) - - // Reconciliation attempts to recreate the missing data records. - err = registry.ApplyChanges(ctx, &plan.Changes{Create: missingDataRecords}) - assert.NoError(t, err) - - records, err := p.Records(ctx) - assert.NoError(t, err) - - expectedRecords := append(missingDataRecords, existingTXTRecords...) - assert.True(t, testutils.SameEndpoints(records, expectedRecords)) } From 4444a1d5a2bf886dcdb457b9ab1ea9030496fb8b Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Mon, 26 May 2025 20:29:57 +0900 Subject: [PATCH 05/13] refactor: extract generateTXTRecordWithFilter to avoid redundant filtering --- registry/txt.go | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/registry/txt.go b/registry/txt.go index 9a56e004cd..07eaed2e71 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -89,10 +89,10 @@ func (im *existingTXTs) add(r *endpoint.Endpoint) { } // filterOutExistingTXTRecords removes endpoints whose TXT companions are already present -func (im *existingTXTs) isManaged(ep *endpoint.Endpoint) bool { +func (im *existingTXTs) isNotManaged(ep *endpoint.Endpoint) bool { k := fmt.Sprintf("%s|%s", ep.DNSName, ep.SetIdentifier) _, ok := im.entries[k] - return ok + return !ok } func (im *existingTXTs) reset() existingTXTs { @@ -273,13 +273,17 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error // depending on the newFormatOnly configuration. The old format is maintained for backwards // compatibility but can be disabled to reduce the number of DNS records. func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpoint { + return im.generateTXTRecordWithFilter(r, func(ep *endpoint.Endpoint) bool { return true }) +} + +func (im *TXTRegistry) generateTXTRecordWithFilter(r *endpoint.Endpoint, filter func(*endpoint.Endpoint) bool) []*endpoint.Endpoint { endpoints := make([]*endpoint.Endpoint, 0) // Create legacy format record by default unless newFormatOnly is true if !im.newFormatOnly && !im.txtEncryptEnabled && !im.mapper.recordTypeInAffix() && r.RecordType != endpoint.RecordTypeAAAA { // old TXT record format txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), endpoint.RecordTypeTXT, r.Labels.Serialize(true, im.txtEncryptEnabled, im.txtEncryptAESKey)) - if txt != nil { + if txt != nil && filter(txt) { txt.WithSetIdentifier(r.SetIdentifier) txt.Labels[endpoint.OwnedRecordLabelKey] = r.DNSName txt.ProviderSpecific = r.ProviderSpecific @@ -294,7 +298,7 @@ func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpo recordType = endpoint.RecordTypeCNAME } txtNew := endpoint.NewEndpoint(im.mapper.toNewTXTName(r.DNSName, recordType), endpoint.RecordTypeTXT, r.Labels.Serialize(true, im.txtEncryptEnabled, im.txtEncryptAESKey)) - if txtNew != nil { + if txtNew != nil && filter(txtNew) { txtNew.WithSetIdentifier(r.SetIdentifier) txtNew.Labels[endpoint.OwnedRecordLabelKey] = r.DNSName txtNew.ProviderSpecific = r.ProviderSpecific @@ -318,14 +322,7 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) } r.Labels[endpoint.OwnerLabelKey] = im.ownerID - generatedTXTs := im.generateTXTRecord(r) - for _, txt := range generatedTXTs { - // If the TXT record is already managed by this instance, skip it - if im.existingTXTs.isManaged(txt) { - continue - } - filteredChanges.Create = append(filteredChanges.Create, txt) - } + filteredChanges.Create = append(filteredChanges.Create, im.generateTXTRecordWithFilter(r, im.existingTXTs.isNotManaged)...) if im.cacheInterval > 0 { im.addToCache(r) From 05e4d4eaa610da68330a9ae38efbf42d9d6d5175 Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Tue, 27 May 2025 09:24:52 +0900 Subject: [PATCH 06/13] test: add comprehensive test cases and isolate test data for t.Parallel --- registry/txt_test.go | 210 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 174 insertions(+), 36 deletions(-) diff --git a/registry/txt_test.go b/registry/txt_test.go index fd53eedd50..f11997178f 100644 --- a/registry/txt_test.go +++ b/registry/txt_test.go @@ -18,6 +18,7 @@ package registry import ( "context" + "fmt" "reflect" "strings" "testing" @@ -1685,6 +1686,50 @@ func newEndpointWithOwnerResource(dnsName, target, recordType, ownerID, resource return e } +// This is primarily used to prevent data races when running tests in parallel (t.Parallel). +func cloneEndpoints(list []*endpoint.Endpoint) []*endpoint.Endpoint { + cloned := make([]*endpoint.Endpoint, len(list)) + for i, e := range list { + cloned[i] = cloneEndpoint(e) + } + return cloned +} + +func cloneEndpoint(e *endpoint.Endpoint) *endpoint.Endpoint { + targets := make(endpoint.Targets, len(e.Targets)) + copy(targets, e.Targets) + + // SameEndpoints treats nil and empty maps/slices as different. + // To avoid introducing unintended differences, we retain nil when original is nil. + var labels endpoint.Labels + if e.Labels != nil { + labels = make(endpoint.Labels, len(e.Labels)) + for k, v := range e.Labels { + labels[k] = v + } + } + + var providerSpecific endpoint.ProviderSpecific + if e.ProviderSpecific != nil { + providerSpecific = make(endpoint.ProviderSpecific, len(e.ProviderSpecific)) + for i, p := range e.ProviderSpecific { + providerSpecific[i] = p + } + } + + ttl := e.RecordTTL + + return &endpoint.Endpoint{ + DNSName: e.DNSName, + Targets: targets, + RecordType: e.RecordType, + RecordTTL: ttl, + Labels: labels, + ProviderSpecific: providerSpecific, + SetIdentifier: e.SetIdentifier, + } +} + func TestNewTXTRegistryWithNewFormatOnly(t *testing.T) { p := inmemory.NewInMemoryProvider() @@ -1863,6 +1908,19 @@ func TestTXTRegistryRecreatesMissingRecords(t *testing.T) { newEndpointWithOwner("new-record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ownerId), }, }, + { + name: "Recreate missing AAAA record when TXT exists", + desired: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "2001:db8::1", endpoint.RecordTypeAAAA, ""), + }, + existing: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + newEndpointWithOwner("aaaa-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + }, + expectedCreate: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "2001:db8::1", endpoint.RecordTypeAAAA, ownerId), + }, + }, { name: "Recreate missing CNAME record when TXT exists", desired: []*endpoint.Endpoint{ @@ -1892,46 +1950,126 @@ func TestTXTRegistryRecreatesMissingRecords(t *testing.T) { newEndpointWithOwner("new-record-2.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ownerId), }, }, + { + name: "Recreate missing A records when TXT and CNAME exists", + desired: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ""), + newEndpointWithOwner("new-record-2.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ""), + }, + existing: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-2.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ownerId), + newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + newEndpointWithOwner("new-record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + newEndpointWithOwner("a-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + newEndpointWithOwner("cname-new-record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + }, + expectedCreate: []*endpoint.Endpoint{ + newEndpointWithOwner("new-record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ownerId), + }, + }, + { + name: "Only one A record is missing among several existing records", + desired: []*endpoint.Endpoint{ + newEndpointWithOwner("record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ""), + newEndpointWithOwner("record-2.test-zone.example.org", "1.1.1.2", endpoint.RecordTypeA, ""), + newEndpointWithOwner("record-3.test-zone.example.org", "1.1.1.3", endpoint.RecordTypeA, ""), + newEndpointWithOwner("record-4.test-zone.example.org", "2001:db8::4", endpoint.RecordTypeAAAA, ""), + newEndpointWithOwner("record-5.test-zone.example.org", "cluster-b", endpoint.RecordTypeCNAME, ""), + }, + existing: []*endpoint.Endpoint{ + newEndpointWithOwner("record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + newEndpointWithOwner("a-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + + newEndpointWithOwner("record-2.test-zone.example.org", "1.1.1.2", endpoint.RecordTypeA, ownerId), + newEndpointWithOwner("record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + newEndpointWithOwner("a-record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + + newEndpointWithOwner("record-3.test-zone.example.org", "1.1.1.3", endpoint.RecordTypeA, ownerId), + newEndpointWithOwner("record-3.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + newEndpointWithOwner("a-record-3.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + + newEndpointWithOwner("record-4.test-zone.example.org", "2001:db8::4", endpoint.RecordTypeAAAA, ownerId), + newEndpointWithOwner("record-4.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + newEndpointWithOwner("aaaa-record-4.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + + newEndpointWithOwner("record-5.test-zone.example.org", "cluster-b", endpoint.RecordTypeCNAME, ownerId), + newEndpointWithOwner("record-5.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + newEndpointWithOwner("cname-record-5.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId), + }, + expectedCreate: []*endpoint.Endpoint{ + newEndpointWithOwner("record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ownerId), + }, + }, // TODO: Test TXT record regeneration when only A/CNAME records exist. // The regeneration logic will be introduced in a separate PR. } for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Given: Register existing TXT records - ctx := context.Background() - p := inmemory.NewInMemoryProvider() - p.CreateZone(testZone) - err := p.ApplyChanges(ctx, &plan.Changes{Create: tt.existing}) - assert.NoError(t, err) - p.OnApplyChanges = func(ctx context.Context, changes *plan.Changes) { - assert.True(t, - testutils.SameEndpoints(changes.Create, tt.expectedCreate), - "Expected create changes: %v, but got: %v", tt.expectedCreate, changes.Create, - ) - } + for pName, policy := range plan.Policies { + // Clone inputs per policy to avoid data races when using t.Parallel. + desired := cloneEndpoints(tt.desired) + existing := cloneEndpoints(tt.existing) + expectedCreate := cloneEndpoints(tt.expectedCreate) + + t.Run(fmt.Sprintf("%s with %s policy", tt.name, pName), func(t *testing.T) { + t.Parallel() + ctx := context.Background() + p := inmemory.NewInMemoryProvider() + + // Given: Register existing records + p.CreateZone(testZone) + err := p.ApplyChanges(ctx, &plan.Changes{Create: existing}) + assert.NoError(t, err) + + // The first ApplyChanges call should create the expected records. + // Subsequent calls are expected to be no-ops (i.e., no additional creates). + isCalled := false + p.OnApplyChanges = func(ctx context.Context, changes *plan.Changes) { + if isCalled { + assert.Len(t, changes.Create, 0, "ApplyChanges should not be called multiple times with new changes") + } else { + assert.True(t, + testutils.SameEndpoints(changes.Create, expectedCreate), + "Expected create changes: %v, but got: %v", expectedCreate, changes.Create, + ) + } + assert.Len(t, changes.UpdateNew, 0, "UpdateNew should be empty") + assert.Len(t, changes.UpdateOld, 0, "UpdateOld should be empty") + assert.Len(t, changes.Delete, 0, "Delete should be empty") + isCalled = true + } - // When: Apply changes to recreate missing A records - managedRecords := []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME, endpoint.RecordTypeTXT} - registry, err := NewTXTRegistry(p, "", "", ownerId, time.Hour, "", managedRecords, nil, false, nil, false) - assert.NoError(t, err) - records, err := registry.Records(ctx) - assert.NoError(t, err) - plan := &plan.Plan{ - Policies: []plan.Policy{&plan.SyncPolicy{}}, - Current: records, - Desired: tt.desired, - ManagedRecords: managedRecords, - OwnerID: ownerId, - } - plan = plan.Calculate() - err = registry.ApplyChanges(ctx, plan.Changes) - assert.NoError(t, err) - - // Then: Verify that the missing records are recreated - records, err = p.Records(ctx) - assert.NoError(t, err) - expectedRecords := append(tt.existing, tt.expectedCreate...) - assert.True(t, testutils.SameEndpoints(records, expectedRecords)) - }) + // When: Apply changes to recreate missing A records + managedRecords := []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME, endpoint.RecordTypeAAAA, endpoint.RecordTypeTXT} + registry, err := NewTXTRegistry(p, "", "", ownerId, time.Hour, "", managedRecords, nil, false, nil, false) + assert.NoError(t, err) + + expectedRecords := append(existing, expectedCreate...) + + // Simulate the reconciliation loop by executing multiple times + reconciliationLoops := 3 + for i := range reconciliationLoops { + records, err := registry.Records(ctx) + assert.NoError(t, err) + plan := &plan.Plan{ + Policies: []plan.Policy{policy}, + Current: records, + Desired: desired, + ManagedRecords: managedRecords, + OwnerID: ownerId, + } + plan = plan.Calculate() + err = registry.ApplyChanges(ctx, plan.Changes) + assert.NoError(t, err) + + // Then: Verify that the missing records are recreated or the existing records are not modified + records, err = p.Records(ctx) + assert.NoError(t, err) + assert.True(t, testutils.SameEndpoints(records, expectedRecords), + "Expected records after reconciliation loop #%d: %v, but got: %v", + i, expectedRecords, records, + ) + } + }) + } } } From 6b8fe6a1ec7d03a957f0835824c8ac12880b7750 Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Tue, 27 May 2025 09:28:42 +0900 Subject: [PATCH 07/13] docs: clarify comment for isNotManaged to reflect actual behavior --- registry/txt.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/registry/txt.go b/registry/txt.go index 07eaed2e71..fc6d99d724 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -88,7 +88,8 @@ func (im *existingTXTs) add(r *endpoint.Endpoint) { im.entries[fmt.Sprintf("%s|%s", r.DNSName, r.SetIdentifier)] = struct{}{} } -// filterOutExistingTXTRecords removes endpoints whose TXT companions are already present +// isNotManaged reports whether the given endpoint's TXT record is absent from the existing set. +// Used to determine whether a new TXT record needs to be created. func (im *existingTXTs) isNotManaged(ep *endpoint.Endpoint) bool { k := fmt.Sprintf("%s|%s", ep.DNSName, ep.SetIdentifier) _, ok := im.entries[k] From d347e2991908a09846567e2e12807a002b27ac03 Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Tue, 27 May 2025 10:09:11 +0900 Subject: [PATCH 08/13] fix: apply SetIdentifier before filtering TXT records, add regression test --- registry/txt.go | 12 ++-- registry/txt_test.go | 144 +++++++++++++++++++++++-------------------- 2 files changed, 86 insertions(+), 70 deletions(-) diff --git a/registry/txt.go b/registry/txt.go index fc6d99d724..b83594e3db 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -284,11 +284,13 @@ func (im *TXTRegistry) generateTXTRecordWithFilter(r *endpoint.Endpoint, filter if !im.newFormatOnly && !im.txtEncryptEnabled && !im.mapper.recordTypeInAffix() && r.RecordType != endpoint.RecordTypeAAAA { // old TXT record format txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), endpoint.RecordTypeTXT, r.Labels.Serialize(true, im.txtEncryptEnabled, im.txtEncryptAESKey)) - if txt != nil && filter(txt) { + if txt != nil { txt.WithSetIdentifier(r.SetIdentifier) txt.Labels[endpoint.OwnedRecordLabelKey] = r.DNSName txt.ProviderSpecific = r.ProviderSpecific - endpoints = append(endpoints, txt) + if filter(txt) { + endpoints = append(endpoints, txt) + } } } @@ -299,11 +301,13 @@ func (im *TXTRegistry) generateTXTRecordWithFilter(r *endpoint.Endpoint, filter recordType = endpoint.RecordTypeCNAME } txtNew := endpoint.NewEndpoint(im.mapper.toNewTXTName(r.DNSName, recordType), endpoint.RecordTypeTXT, r.Labels.Serialize(true, im.txtEncryptEnabled, im.txtEncryptAESKey)) - if txtNew != nil && filter(txtNew) { + if txtNew != nil { txtNew.WithSetIdentifier(r.SetIdentifier) txtNew.Labels[endpoint.OwnedRecordLabelKey] = r.DNSName txtNew.ProviderSpecific = r.ProviderSpecific - endpoints = append(endpoints, txtNew) + if filter(txtNew) { + endpoints = append(endpoints, txtNew) + } } return endpoints } diff --git a/registry/txt_test.go b/registry/txt_test.go index f11997178f..af9d24309e 100644 --- a/registry/txt_test.go +++ b/registry/txt_test.go @@ -1687,15 +1687,15 @@ func newEndpointWithOwnerResource(dnsName, target, recordType, ownerID, resource } // This is primarily used to prevent data races when running tests in parallel (t.Parallel). -func cloneEndpoints(list []*endpoint.Endpoint) []*endpoint.Endpoint { +func cloneEndpointsWithOpts(list []*endpoint.Endpoint, opt ...func(*endpoint.Endpoint)) []*endpoint.Endpoint { cloned := make([]*endpoint.Endpoint, len(list)) for i, e := range list { - cloned[i] = cloneEndpoint(e) + cloned[i] = cloneEndpointWithOpts(e, opt...) } return cloned } -func cloneEndpoint(e *endpoint.Endpoint) *endpoint.Endpoint { +func cloneEndpointWithOpts(e *endpoint.Endpoint, opt ...func(*endpoint.Endpoint)) *endpoint.Endpoint { targets := make(endpoint.Targets, len(e.Targets)) copy(targets, e.Targets) @@ -1719,7 +1719,7 @@ func cloneEndpoint(e *endpoint.Endpoint) *endpoint.Endpoint { ttl := e.RecordTTL - return &endpoint.Endpoint{ + ep := &endpoint.Endpoint{ DNSName: e.DNSName, Targets: targets, RecordType: e.RecordType, @@ -1728,6 +1728,10 @@ func cloneEndpoint(e *endpoint.Endpoint) *endpoint.Endpoint { ProviderSpecific: providerSpecific, SetIdentifier: e.SetIdentifier, } + for _, o := range opt { + o(ep) + } + return ep } func TestNewTXTRegistryWithNewFormatOnly(t *testing.T) { @@ -2004,72 +2008,80 @@ func TestTXTRegistryRecreatesMissingRecords(t *testing.T) { // The regeneration logic will be introduced in a separate PR. } for _, tt := range tests { - for pName, policy := range plan.Policies { - // Clone inputs per policy to avoid data races when using t.Parallel. - desired := cloneEndpoints(tt.desired) - existing := cloneEndpoints(tt.existing) - expectedCreate := cloneEndpoints(tt.expectedCreate) - - t.Run(fmt.Sprintf("%s with %s policy", tt.name, pName), func(t *testing.T) { - t.Parallel() - ctx := context.Background() - p := inmemory.NewInMemoryProvider() - - // Given: Register existing records - p.CreateZone(testZone) - err := p.ApplyChanges(ctx, &plan.Changes{Create: existing}) - assert.NoError(t, err) - - // The first ApplyChanges call should create the expected records. - // Subsequent calls are expected to be no-ops (i.e., no additional creates). - isCalled := false - p.OnApplyChanges = func(ctx context.Context, changes *plan.Changes) { - if isCalled { - assert.Len(t, changes.Create, 0, "ApplyChanges should not be called multiple times with new changes") - } else { - assert.True(t, - testutils.SameEndpoints(changes.Create, expectedCreate), - "Expected create changes: %v, but got: %v", expectedCreate, changes.Create, - ) - } - assert.Len(t, changes.UpdateNew, 0, "UpdateNew should be empty") - assert.Len(t, changes.UpdateOld, 0, "UpdateOld should be empty") - assert.Len(t, changes.Delete, 0, "Delete should be empty") - isCalled = true - } - - // When: Apply changes to recreate missing A records - managedRecords := []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME, endpoint.RecordTypeAAAA, endpoint.RecordTypeTXT} - registry, err := NewTXTRegistry(p, "", "", ownerId, time.Hour, "", managedRecords, nil, false, nil, false) - assert.NoError(t, err) - - expectedRecords := append(existing, expectedCreate...) - - // Simulate the reconciliation loop by executing multiple times - reconciliationLoops := 3 - for i := range reconciliationLoops { - records, err := registry.Records(ctx) + for _, setIdentifier := range []string{"", "set-identifier"} { + for pName, policy := range plan.Policies { + // Clone inputs per policy to avoid data races when using t.Parallel. + desired := cloneEndpointsWithOpts(tt.desired, func(e *endpoint.Endpoint) { + e.WithSetIdentifier(setIdentifier) + }) + existing := cloneEndpointsWithOpts(tt.existing, func(e *endpoint.Endpoint) { + e.WithSetIdentifier(setIdentifier) + }) + expectedCreate := cloneEndpointsWithOpts(tt.expectedCreate, func(e *endpoint.Endpoint) { + e.WithSetIdentifier(setIdentifier) + }) + + t.Run(fmt.Sprintf("%s with %s policy and setIdentifier=%s", tt.name, pName, setIdentifier), func(t *testing.T) { + t.Parallel() + ctx := context.Background() + p := inmemory.NewInMemoryProvider() + + // Given: Register existing records + p.CreateZone(testZone) + err := p.ApplyChanges(ctx, &plan.Changes{Create: existing}) assert.NoError(t, err) - plan := &plan.Plan{ - Policies: []plan.Policy{policy}, - Current: records, - Desired: desired, - ManagedRecords: managedRecords, - OwnerID: ownerId, + + // The first ApplyChanges call should create the expected records. + // Subsequent calls are expected to be no-ops (i.e., no additional creates). + isCalled := false + p.OnApplyChanges = func(ctx context.Context, changes *plan.Changes) { + if isCalled { + assert.Len(t, changes.Create, 0, "ApplyChanges should not be called multiple times with new changes") + } else { + assert.True(t, + testutils.SameEndpoints(changes.Create, expectedCreate), + "Expected create changes: %v, but got: %v", expectedCreate, changes.Create, + ) + } + assert.Len(t, changes.UpdateNew, 0, "UpdateNew should be empty") + assert.Len(t, changes.UpdateOld, 0, "UpdateOld should be empty") + assert.Len(t, changes.Delete, 0, "Delete should be empty") + isCalled = true } - plan = plan.Calculate() - err = registry.ApplyChanges(ctx, plan.Changes) - assert.NoError(t, err) - // Then: Verify that the missing records are recreated or the existing records are not modified - records, err = p.Records(ctx) + // When: Apply changes to recreate missing A records + managedRecords := []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME, endpoint.RecordTypeAAAA, endpoint.RecordTypeTXT} + registry, err := NewTXTRegistry(p, "", "", ownerId, time.Hour, "", managedRecords, nil, false, nil, false) assert.NoError(t, err) - assert.True(t, testutils.SameEndpoints(records, expectedRecords), - "Expected records after reconciliation loop #%d: %v, but got: %v", - i, expectedRecords, records, - ) - } - }) + + expectedRecords := append(existing, expectedCreate...) + + // Simulate the reconciliation loop by executing multiple times + reconciliationLoops := 3 + for i := range reconciliationLoops { + records, err := registry.Records(ctx) + assert.NoError(t, err) + plan := &plan.Plan{ + Policies: []plan.Policy{policy}, + Current: records, + Desired: desired, + ManagedRecords: managedRecords, + OwnerID: ownerId, + } + plan = plan.Calculate() + err = registry.ApplyChanges(ctx, plan.Changes) + assert.NoError(t, err) + + // Then: Verify that the missing records are recreated or the existing records are not modified + records, err = p.Records(ctx) + assert.NoError(t, err) + assert.True(t, testutils.SameEndpoints(records, expectedRecords), + "Expected records after reconciliation loop #%d: %v, but got: %v", + i, expectedRecords, records, + ) + } + }) + } } } } From cc6d91e1ead634067c22f49cf59a1b0cf7284ba5 Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Tue, 27 May 2025 10:26:05 +0900 Subject: [PATCH 09/13] chore: fix lint warnings --- registry/txt_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/registry/txt_test.go b/registry/txt_test.go index af9d24309e..710eda3f8e 100644 --- a/registry/txt_test.go +++ b/registry/txt_test.go @@ -2036,16 +2036,16 @@ func TestTXTRegistryRecreatesMissingRecords(t *testing.T) { isCalled := false p.OnApplyChanges = func(ctx context.Context, changes *plan.Changes) { if isCalled { - assert.Len(t, changes.Create, 0, "ApplyChanges should not be called multiple times with new changes") + assert.Empty(t, changes.Create, "ApplyChanges should not be called multiple times with new changes") } else { assert.True(t, testutils.SameEndpoints(changes.Create, expectedCreate), "Expected create changes: %v, but got: %v", expectedCreate, changes.Create, ) } - assert.Len(t, changes.UpdateNew, 0, "UpdateNew should be empty") - assert.Len(t, changes.UpdateOld, 0, "UpdateOld should be empty") - assert.Len(t, changes.Delete, 0, "Delete should be empty") + assert.Empty(t, changes.UpdateNew, "UpdateNew should be empty") + assert.Empty(t, changes.UpdateOld, "UpdateOld should be empty") + assert.Empty(t, changes.Delete, "Delete should be empty") isCalled = true } From b10016665bb5463298cd893fda58696dbfd1e0c3 Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Thu, 5 Jun 2025 21:54:46 +0900 Subject: [PATCH 10/13] refactor: use key struct instead of concatenated string for map keys to reduce memory usage --- registry/txt.go | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/registry/txt.go b/registry/txt.go index b83594e3db..e72ed4c752 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -19,7 +19,6 @@ package registry import ( "context" "errors" - "fmt" "strings" "time" @@ -65,39 +64,53 @@ type TXTRegistry struct { // existingTXTs is the TXT records that already exist in the zone so that // ApplyChanges() can skip re-creating them. See the struct below for details. - existingTXTs existingTXTs + existingTXTs *existingTXTs } // existingTXTs stores pre‑existing TXT records to avoid duplicate creation. // It relies on the fact that Records() is always called **before** ApplyChanges() // within a single reconciliation cycle. type existingTXTs struct { - entries map[string]struct{} + entries map[recordKey]struct{} } -func newExistingTXTs() existingTXTs { - return existingTXTs{ - entries: make(map[string]struct{}), +type recordKey struct { + dnsName string + setIdentifier string +} + +func newExistingTXTs() *existingTXTs { + return &existingTXTs{ + entries: make(map[recordKey]struct{}), } } func (im *existingTXTs) add(r *endpoint.Endpoint) { + key := recordKey{ + dnsName: r.DNSName, + setIdentifier: r.SetIdentifier, + } if im.entries == nil { - im.entries = make(map[string]struct{}) + im.entries = make(map[recordKey]struct{}) } - im.entries[fmt.Sprintf("%s|%s", r.DNSName, r.SetIdentifier)] = struct{}{} + im.entries[key] = struct{}{} } // isNotManaged reports whether the given endpoint's TXT record is absent from the existing set. // Used to determine whether a new TXT record needs to be created. func (im *existingTXTs) isNotManaged(ep *endpoint.Endpoint) bool { - k := fmt.Sprintf("%s|%s", ep.DNSName, ep.SetIdentifier) - _, ok := im.entries[k] + key := recordKey{ + dnsName: ep.DNSName, + setIdentifier: ep.SetIdentifier, + } + _, ok := im.entries[key] return !ok } -func (im *existingTXTs) reset() existingTXTs { - return newExistingTXTs() +func (im *existingTXTs) reset() { + // Reset the existing TXT records for the next reconciliation loop. + // This is necessary because the existing TXT records are only relevant for the current reconciliation cycle. + im.entries = make(map[recordKey]struct{}) } // NewTXTRegistry returns a new TXTRegistry object. When newFormatOnly is true, it will only @@ -315,6 +328,8 @@ func (im *TXTRegistry) generateTXTRecordWithFilter(r *endpoint.Endpoint, filter // 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(ctx context.Context, changes *plan.Changes) error { + defer im.existingTXTs.reset() // reset existing TXTs for the next reconciliation loop + filteredChanges := &plan.Changes{ Create: changes.Create, UpdateNew: endpoint.FilterEndpointsByOwnerID(im.ownerID, changes.UpdateNew), @@ -333,7 +348,6 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) im.addToCache(r) } } - im.existingTXTs = im.existingTXTs.reset() // reset existing TXTs for the next reconciliation loop for _, r := range filteredChanges.Delete { // when we delete TXT records for which value has changed (due to new label) this would still work because From a5efc4094d950d7d4d361cabf571d85c5bc2a2b7 Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Thu, 31 Jul 2025 07:32:00 +0900 Subject: [PATCH 11/13] fix txt_test.go --- registry/txt_test.go | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/registry/txt_test.go b/registry/txt_test.go index 812cf2a952..7efa534c33 100644 --- a/registry/txt_test.go +++ b/registry/txt_test.go @@ -1926,8 +1926,30 @@ func TestTXTRegistryRecreatesMissingRecords(t *testing.T) { newEndpointWithOwner("record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ownerId), }, }, - // TODO: Test TXT record regeneration when only A/CNAME records exist. - // The regeneration logic will be introduced in a separate PR. + { + name: "Should not recreate TXT records for existing A records without owner", + desired: []*endpoint.Endpoint{ + newEndpointWithOwner("record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ""), + }, + existing: []*endpoint.Endpoint{ + newEndpointWithOwner("record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ownerId), + // Missing TXT record for the existing A record + }, + expectedCreate: []*endpoint.Endpoint{}, + }, + { + name: "Should not recreate TXT records for existing A records with another owner", + desired: []*endpoint.Endpoint{ + newEndpointWithOwner("record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ""), + }, + existing: []*endpoint.Endpoint{ + // This test uses the `ownerId` variable, and "another-owner" simulates a different owner. + // In this case, TXT records should not be recreated. + newEndpointWithOwner("record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, "another-owner"), + newEndpointWithOwner("a-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+"another-owner"+"\"", endpoint.RecordTypeTXT, "another-owner"), + }, + expectedCreate: []*endpoint.Endpoint{}, + }, } for _, tt := range tests { for _, setIdentifier := range []string{"", "set-identifier"} { From 0c31b1b7e00f226a91d85d0aa3c10effc71658fb Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Thu, 31 Jul 2025 07:54:26 +0900 Subject: [PATCH 12/13] fix txt_test.go dependency --- registry/txt_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/registry/txt_test.go b/registry/txt_test.go index 83356ad745..05cdd030d2 100644 --- a/registry/txt_test.go +++ b/registry/txt_test.go @@ -18,6 +18,7 @@ package registry import ( "context" + "fmt" "reflect" "strings" "testing" From 5d1f43e51277a5b5ba3bd1823ce789b96a4948fc Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Fri, 29 Aug 2025 21:13:49 +0900 Subject: [PATCH 13/13] refactor(registry/txt): rename to isAbsent --- registry/txt.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/registry/txt.go b/registry/txt.go index 74152636ab..b428cfe9ad 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -88,15 +88,12 @@ func (im *existingTXTs) add(r *endpoint.Endpoint) { dnsName: r.DNSName, setIdentifier: r.SetIdentifier, } - if im.entries == nil { - im.entries = make(map[recordKey]struct{}) - } im.entries[key] = struct{}{} } -// isNotManaged reports whether the given endpoint's TXT record is absent from the existing set. -// Used to determine whether a new TXT record needs to be created. -func (im *existingTXTs) isNotManaged(ep *endpoint.Endpoint) bool { +// isAbsent returns true when there is no entry for the given name in the store. +// This is intended for the "if absent -> create" pattern. +func (im *existingTXTs) isAbsent(ep *endpoint.Endpoint) bool { key := recordKey{ dnsName: ep.DNSName, setIdentifier: ep.SetIdentifier, @@ -324,7 +321,7 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) } r.Labels[endpoint.OwnerLabelKey] = im.ownerID - filteredChanges.Create = append(filteredChanges.Create, im.generateTXTRecordWithFilter(r, im.existingTXTs.isNotManaged)...) + filteredChanges.Create = append(filteredChanges.Create, im.generateTXTRecordWithFilter(r, im.existingTXTs.isAbsent)...) if im.cacheInterval > 0 { im.addToCache(r)