diff --git a/registry/txt.go b/registry/txt.go index 325996936..b428cfe9a 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -19,6 +19,7 @@ package registry import ( "context" "errors" + "strings" "time" @@ -58,6 +59,53 @@ type TXTRegistry struct { // encrypt text records txtEncryptEnabled bool txtEncryptAESKey []byte + + // 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 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[recordKey]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, + } + im.entries[key] = struct{}{} +} + +// 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, + } + _, ok := im.entries[key] + return !ok +} + +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 @@ -100,6 +148,7 @@ func NewTXTRegistry(provider provider.Provider, txtPrefix, txtSuffix, ownerID st excludeRecordTypes: excludeRecordTypes, txtEncryptEnabled: txtEncryptEnabled, txtEncryptAESKey: txtEncryptAESKey, + existingTXTs: newExistingTXTs(), }, nil } @@ -167,6 +216,7 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error } labelMap[key] = labels txtRecordsMap[record.DNSName] = struct{}{} + im.existingTXTs.add(record) } for _, ep := range endpoints { @@ -230,6 +280,10 @@ 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) // Always create new format record @@ -243,7 +297,9 @@ func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpo 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 } @@ -251,6 +307,8 @@ func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpo // 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), @@ -263,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.generateTXTRecord(r)...) + filteredChanges.Create = append(filteredChanges.Create, im.generateTXTRecordWithFilter(r, im.existingTXTs.isAbsent)...) if im.cacheInterval > 0 { im.addToCache(r) diff --git a/registry/txt_test.go b/registry/txt_test.go index d59ce90ef..05cdd030d 100644 --- a/registry/txt_test.go +++ b/registry/txt_test.go @@ -18,6 +18,7 @@ package registry import ( "context" + "fmt" "reflect" "strings" "testing" @@ -1807,3 +1808,223 @@ func TestTXTRegistryRecordsWithEmptyTargets(t *testing.T) { testutils.TestHelperLogContains("TXT record has no targets empty-targets.test-zone.example.org", hook, t) } + +// 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 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 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{ + 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), + }, + }, + { + 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), + }, + }, + { + 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"} { + 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) + + // 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.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.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 + } + + // 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) + 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, + ) + } + }) + } + } + } +} diff --git a/registry/txt_utils_test.go b/registry/txt_utils_test.go index 5ff663c54..e37c5495b 100644 --- a/registry/txt_utils_test.go +++ b/registry/txt_utils_test.go @@ -43,3 +43,51 @@ func newEndpointWithOwnerResource(dnsName, target, recordType, ownerID, resource e.Labels[endpoint.ResourceLabelKey] = resource return e } + +// This is primarily used to prevent data races when running tests in parallel (t.Parallel). +func cloneEndpointsWithOpts(list []*endpoint.Endpoint, opt ...func(*endpoint.Endpoint)) []*endpoint.Endpoint { + cloned := make([]*endpoint.Endpoint, len(list)) + for i, e := range list { + cloned[i] = cloneEndpointWithOpts(e, opt...) + } + return cloned +} + +func cloneEndpointWithOpts(e *endpoint.Endpoint, opt ...func(*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 + + ep := &endpoint.Endpoint{ + DNSName: e.DNSName, + Targets: targets, + RecordType: e.RecordType, + RecordTTL: ttl, + Labels: labels, + ProviderSpecific: providerSpecific, + SetIdentifier: e.SetIdentifier, + } + for _, o := range opt { + o(ep) + } + return ep +}