Skip to content

Commit c6ed951

Browse files
u-kaiu-kai
authored
fix(txt-registry): skip creation of already-existing TXT records (#4914) (#5459)
* fix(txt-registry): skip creation of already-existing TXT records (#4914) * refactor(registry): store existing TXT records temporarily between Records and ApplyChanges * refactor: apply review suggestion for code cleanup * test: add coverage for recreating A/CNAME records when TXT exists * refactor: extract generateTXTRecordWithFilter to avoid redundant filtering * test: add comprehensive test cases and isolate test data for t.Parallel * docs: clarify comment for isNotManaged to reflect actual behavior * fix: apply SetIdentifier before filtering TXT records, add regression test * chore: fix lint warnings * refactor: use key struct instead of concatenated string for map keys to reduce memory usage * fix txt_test.go * fix txt_test.go dependency * refactor(registry/txt): rename to isAbsent
1 parent 8cf6e42 commit c6ed951

File tree

3 files changed

+329
-2
lines changed

3 files changed

+329
-2
lines changed

‎registry/txt.go‎

Lines changed: 60 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package registry
1919
import (
2020
"context"
2121
"errors"
22+
2223
"strings"
2324
"time"
2425

@@ -58,6 +59,53 @@ type TXTRegistry struct {
5859
// encrypt text records
5960
txtEncryptEnabled bool
6061
txtEncryptAESKey []byte
62+
63+
// existingTXTs is the TXT records that already exist in the zone so that
64+
// ApplyChanges() can skip re-creating them. See the struct below for details.
65+
existingTXTs *existingTXTs
66+
}
67+
68+
// existingTXTs stores pre‑existing TXT records to avoid duplicate creation.
69+
// It relies on the fact that Records() is always called **before** ApplyChanges()
70+
// within a single reconciliation cycle.
71+
type existingTXTs struct {
72+
entries map[recordKey]struct{}
73+
}
74+
75+
type recordKey struct {
76+
dnsName string
77+
setIdentifier string
78+
}
79+
80+
func newExistingTXTs() *existingTXTs {
81+
return &existingTXTs{
82+
entries: make(map[recordKey]struct{}),
83+
}
84+
}
85+
86+
func (im *existingTXTs) add(r *endpoint.Endpoint) {
87+
key := recordKey{
88+
dnsName: r.DNSName,
89+
setIdentifier: r.SetIdentifier,
90+
}
91+
im.entries[key] = struct{}{}
92+
}
93+
94+
// isAbsent returns true when there is no entry for the given name in the store.
95+
// This is intended for the "if absent -> create" pattern.
96+
func (im *existingTXTs) isAbsent(ep *endpoint.Endpoint) bool {
97+
key := recordKey{
98+
dnsName: ep.DNSName,
99+
setIdentifier: ep.SetIdentifier,
100+
}
101+
_, ok := im.entries[key]
102+
return !ok
103+
}
104+
105+
func (im *existingTXTs) reset() {
106+
// Reset the existing TXT records for the next reconciliation loop.
107+
// This is necessary because the existing TXT records are only relevant for the current reconciliation cycle.
108+
im.entries = make(map[recordKey]struct{})
61109
}
62110

63111
// 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
100148
excludeRecordTypes: excludeRecordTypes,
101149
txtEncryptEnabled: txtEncryptEnabled,
102150
txtEncryptAESKey: txtEncryptAESKey,
151+
existingTXTs: newExistingTXTs(),
103152
}, nil
104153
}
105154

@@ -167,6 +216,7 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error
167216
}
168217
labelMap[key] = labels
169218
txtRecordsMap[record.DNSName] = struct{}{}
219+
im.existingTXTs.add(record)
170220
}
171221

172222
for _, ep := range endpoints {
@@ -230,6 +280,10 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error
230280
// depending on the newFormatOnly configuration. The old format is maintained for backwards
231281
// compatibility but can be disabled to reduce the number of DNS records.
232282
func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpoint {
283+
return im.generateTXTRecordWithFilter(r, func(ep *endpoint.Endpoint) bool { return true })
284+
}
285+
286+
func (im *TXTRegistry) generateTXTRecordWithFilter(r *endpoint.Endpoint, filter func(*endpoint.Endpoint) bool) []*endpoint.Endpoint {
233287
endpoints := make([]*endpoint.Endpoint, 0)
234288

235289
// Always create new format record
@@ -243,14 +297,18 @@ func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpo
243297
txtNew.WithSetIdentifier(r.SetIdentifier)
244298
txtNew.Labels[endpoint.OwnedRecordLabelKey] = r.DNSName
245299
txtNew.ProviderSpecific = r.ProviderSpecific
246-
endpoints = append(endpoints, txtNew)
300+
if filter(txtNew) {
301+
endpoints = append(endpoints, txtNew)
302+
}
247303
}
248304
return endpoints
249305
}
250306

251307
// ApplyChanges updates dns provider with the changes
252308
// for each created/deleted record it will also take into account TXT records for creation/deletion
253309
func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) error {
310+
defer im.existingTXTs.reset() // reset existing TXTs for the next reconciliation loop
311+
254312
filteredChanges := &plan.Changes{
255313
Create: changes.Create,
256314
UpdateNew: endpoint.FilterEndpointsByOwnerID(im.ownerID, changes.UpdateNew),
@@ -263,7 +321,7 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes)
263321
}
264322
r.Labels[endpoint.OwnerLabelKey] = im.ownerID
265323

266-
filteredChanges.Create = append(filteredChanges.Create, im.generateTXTRecord(r)...)
324+
filteredChanges.Create = append(filteredChanges.Create, im.generateTXTRecordWithFilter(r, im.existingTXTs.isAbsent)...)
267325

268326
if im.cacheInterval > 0 {
269327
im.addToCache(r)

‎registry/txt_test.go‎

Lines changed: 221 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package registry
1818

1919
import (
2020
"context"
21+
"fmt"
2122
"reflect"
2223
"strings"
2324
"testing"
@@ -1807,3 +1808,223 @@ func TestTXTRegistryRecordsWithEmptyTargets(t *testing.T) {
18071808

18081809
testutils.TestHelperLogContains("TXT record has no targets empty-targets.test-zone.example.org", hook, t)
18091810
}
1811+
1812+
// TestTXTRegistryRecreatesMissingRecords reproduces issue #4914.
1813+
// It verifies that External‑DNS recreates A/CNAME records that were accidentally deleted while their corresponding TXT records remain.
1814+
// An InMemoryProvider is used because, like Route53, it throws an error when attempting to create a duplicate record.
1815+
func TestTXTRegistryRecreatesMissingRecords(t *testing.T) {
1816+
ownerId := "owner"
1817+
tests := []struct {
1818+
name string
1819+
desired []*endpoint.Endpoint
1820+
existing []*endpoint.Endpoint
1821+
expectedCreate []*endpoint.Endpoint
1822+
}{
1823+
{
1824+
name: "Recreate missing A record when TXT exists",
1825+
desired: []*endpoint.Endpoint{
1826+
newEndpointWithOwner("new-record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ""),
1827+
},
1828+
existing: []*endpoint.Endpoint{
1829+
newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1830+
newEndpointWithOwner("a-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1831+
},
1832+
expectedCreate: []*endpoint.Endpoint{
1833+
newEndpointWithOwner("new-record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ownerId),
1834+
},
1835+
},
1836+
{
1837+
name: "Recreate missing AAAA record when TXT exists",
1838+
desired: []*endpoint.Endpoint{
1839+
newEndpointWithOwner("new-record-1.test-zone.example.org", "2001:db8::1", endpoint.RecordTypeAAAA, ""),
1840+
},
1841+
existing: []*endpoint.Endpoint{
1842+
newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1843+
newEndpointWithOwner("aaaa-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1844+
},
1845+
expectedCreate: []*endpoint.Endpoint{
1846+
newEndpointWithOwner("new-record-1.test-zone.example.org", "2001:db8::1", endpoint.RecordTypeAAAA, ownerId),
1847+
},
1848+
},
1849+
{
1850+
name: "Recreate missing CNAME record when TXT exists",
1851+
desired: []*endpoint.Endpoint{
1852+
newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ""),
1853+
},
1854+
existing: []*endpoint.Endpoint{
1855+
newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1856+
newEndpointWithOwner("cname-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1857+
},
1858+
expectedCreate: []*endpoint.Endpoint{
1859+
newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ownerId)},
1860+
},
1861+
{
1862+
name: "Recreate missing A and CNAME records when TXT exists",
1863+
desired: []*endpoint.Endpoint{
1864+
newEndpointWithOwner("new-record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ""),
1865+
newEndpointWithOwner("new-record-2.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ""),
1866+
},
1867+
existing: []*endpoint.Endpoint{
1868+
newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1869+
newEndpointWithOwner("new-record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1870+
newEndpointWithOwner("a-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1871+
newEndpointWithOwner("cname-new-record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1872+
},
1873+
expectedCreate: []*endpoint.Endpoint{
1874+
newEndpointWithOwner("new-record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ownerId),
1875+
newEndpointWithOwner("new-record-2.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ownerId),
1876+
},
1877+
},
1878+
{
1879+
name: "Recreate missing A records when TXT and CNAME exists",
1880+
desired: []*endpoint.Endpoint{
1881+
newEndpointWithOwner("new-record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ""),
1882+
newEndpointWithOwner("new-record-2.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ""),
1883+
},
1884+
existing: []*endpoint.Endpoint{
1885+
newEndpointWithOwner("new-record-2.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ownerId),
1886+
newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1887+
newEndpointWithOwner("new-record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1888+
newEndpointWithOwner("a-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1889+
newEndpointWithOwner("cname-new-record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1890+
},
1891+
expectedCreate: []*endpoint.Endpoint{
1892+
newEndpointWithOwner("new-record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ownerId),
1893+
},
1894+
},
1895+
{
1896+
name: "Only one A record is missing among several existing records",
1897+
desired: []*endpoint.Endpoint{
1898+
newEndpointWithOwner("record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ""),
1899+
newEndpointWithOwner("record-2.test-zone.example.org", "1.1.1.2", endpoint.RecordTypeA, ""),
1900+
newEndpointWithOwner("record-3.test-zone.example.org", "1.1.1.3", endpoint.RecordTypeA, ""),
1901+
newEndpointWithOwner("record-4.test-zone.example.org", "2001:db8::4", endpoint.RecordTypeAAAA, ""),
1902+
newEndpointWithOwner("record-5.test-zone.example.org", "cluster-b", endpoint.RecordTypeCNAME, ""),
1903+
},
1904+
existing: []*endpoint.Endpoint{
1905+
newEndpointWithOwner("record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1906+
newEndpointWithOwner("a-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1907+
1908+
newEndpointWithOwner("record-2.test-zone.example.org", "1.1.1.2", endpoint.RecordTypeA, ownerId),
1909+
newEndpointWithOwner("record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1910+
newEndpointWithOwner("a-record-2.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1911+
1912+
newEndpointWithOwner("record-3.test-zone.example.org", "1.1.1.3", endpoint.RecordTypeA, ownerId),
1913+
newEndpointWithOwner("record-3.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1914+
newEndpointWithOwner("a-record-3.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1915+
1916+
newEndpointWithOwner("record-4.test-zone.example.org", "2001:db8::4", endpoint.RecordTypeAAAA, ownerId),
1917+
newEndpointWithOwner("record-4.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1918+
newEndpointWithOwner("aaaa-record-4.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1919+
1920+
newEndpointWithOwner("record-5.test-zone.example.org", "cluster-b", endpoint.RecordTypeCNAME, ownerId),
1921+
newEndpointWithOwner("record-5.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1922+
newEndpointWithOwner("cname-record-5.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+ownerId+"\"", endpoint.RecordTypeTXT, ownerId),
1923+
},
1924+
expectedCreate: []*endpoint.Endpoint{
1925+
newEndpointWithOwner("record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ownerId),
1926+
},
1927+
},
1928+
{
1929+
name: "Should not recreate TXT records for existing A records without owner",
1930+
desired: []*endpoint.Endpoint{
1931+
newEndpointWithOwner("record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ""),
1932+
},
1933+
existing: []*endpoint.Endpoint{
1934+
newEndpointWithOwner("record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ownerId),
1935+
// Missing TXT record for the existing A record
1936+
},
1937+
expectedCreate: []*endpoint.Endpoint{},
1938+
},
1939+
{
1940+
name: "Should not recreate TXT records for existing A records with another owner",
1941+
desired: []*endpoint.Endpoint{
1942+
newEndpointWithOwner("record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, ""),
1943+
},
1944+
existing: []*endpoint.Endpoint{
1945+
// This test uses the `ownerId` variable, and "another-owner" simulates a different owner.
1946+
// In this case, TXT records should not be recreated.
1947+
newEndpointWithOwner("record-1.test-zone.example.org", "1.1.1.1", endpoint.RecordTypeA, "another-owner"),
1948+
newEndpointWithOwner("a-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner="+"another-owner"+"\"", endpoint.RecordTypeTXT, "another-owner"),
1949+
},
1950+
expectedCreate: []*endpoint.Endpoint{},
1951+
},
1952+
}
1953+
for _, tt := range tests {
1954+
for _, setIdentifier := range []string{"", "set-identifier"} {
1955+
for pName, policy := range plan.Policies {
1956+
// Clone inputs per policy to avoid data races when using t.Parallel.
1957+
desired := cloneEndpointsWithOpts(tt.desired, func(e *endpoint.Endpoint) {
1958+
e.WithSetIdentifier(setIdentifier)
1959+
})
1960+
existing := cloneEndpointsWithOpts(tt.existing, func(e *endpoint.Endpoint) {
1961+
e.WithSetIdentifier(setIdentifier)
1962+
})
1963+
expectedCreate := cloneEndpointsWithOpts(tt.expectedCreate, func(e *endpoint.Endpoint) {
1964+
e.WithSetIdentifier(setIdentifier)
1965+
})
1966+
1967+
t.Run(fmt.Sprintf("%s with %s policy and setIdentifier=%s", tt.name, pName, setIdentifier), func(t *testing.T) {
1968+
t.Parallel()
1969+
ctx := context.Background()
1970+
p := inmemory.NewInMemoryProvider()
1971+
1972+
// Given: Register existing records
1973+
p.CreateZone(testZone)
1974+
err := p.ApplyChanges(ctx, &plan.Changes{Create: existing})
1975+
assert.NoError(t, err)
1976+
1977+
// The first ApplyChanges call should create the expected records.
1978+
// Subsequent calls are expected to be no-ops (i.e., no additional creates).
1979+
isCalled := false
1980+
p.OnApplyChanges = func(ctx context.Context, changes *plan.Changes) {
1981+
if isCalled {
1982+
assert.Empty(t, changes.Create, "ApplyChanges should not be called multiple times with new changes")
1983+
} else {
1984+
assert.True(t,
1985+
testutils.SameEndpoints(changes.Create, expectedCreate),
1986+
"Expected create changes: %v, but got: %v", expectedCreate, changes.Create,
1987+
)
1988+
}
1989+
assert.Empty(t, changes.UpdateNew, "UpdateNew should be empty")
1990+
assert.Empty(t, changes.UpdateOld, "UpdateOld should be empty")
1991+
assert.Empty(t, changes.Delete, "Delete should be empty")
1992+
isCalled = true
1993+
}
1994+
1995+
// When: Apply changes to recreate missing A records
1996+
managedRecords := []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME, endpoint.RecordTypeAAAA, endpoint.RecordTypeTXT}
1997+
registry, err := NewTXTRegistry(p, "", "", ownerId, time.Hour, "", managedRecords, nil, false, nil)
1998+
assert.NoError(t, err)
1999+
2000+
expectedRecords := append(existing, expectedCreate...)
2001+
2002+
// Simulate the reconciliation loop by executing multiple times
2003+
reconciliationLoops := 3
2004+
for i := range reconciliationLoops {
2005+
records, err := registry.Records(ctx)
2006+
assert.NoError(t, err)
2007+
plan := &plan.Plan{
2008+
Policies: []plan.Policy{policy},
2009+
Current: records,
2010+
Desired: desired,
2011+
ManagedRecords: managedRecords,
2012+
OwnerID: ownerId,
2013+
}
2014+
plan = plan.Calculate()
2015+
err = registry.ApplyChanges(ctx, plan.Changes)
2016+
assert.NoError(t, err)
2017+
2018+
// Then: Verify that the missing records are recreated or the existing records are not modified
2019+
records, err = p.Records(ctx)
2020+
assert.NoError(t, err)
2021+
assert.True(t, testutils.SameEndpoints(records, expectedRecords),
2022+
"Expected records after reconciliation loop #%d: %v, but got: %v",
2023+
i, expectedRecords, records,
2024+
)
2025+
}
2026+
})
2027+
}
2028+
}
2029+
}
2030+
}

0 commit comments

Comments
 (0)