Skip to content

refactor: explicitly return at most one endpoint in generateTXTRecord #5790

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

refactor: explicitly return at most one endpoint in generateTXTRecord #5790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b6c92c
fix: remove outdated comment
bachorp Sep 1, 2025
015748a
refactor: explicitly return at most one endpoint in `generateTXTRecord`
bachorp Sep 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 16 additions & 22 deletions registry/txt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,9 +265,8 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error
// The migration is done for the TXT records owned by this instance only.
if len(txtRecordsMap) > 0 && ep.Labels[endpoint.OwnerLabelKey] == im.ownerID {
if plan.IsManagedRecord(ep.RecordType, im.managedRecordTypes, im.excludeRecordTypes) {
// Get desired TXT records and detect the missing ones
desiredTXTs := im.generateTXTRecord(ep)
for _, desiredTXT := range desiredTXTs {
// Get desired TXT record and check whether it is missing
if desiredTXT := im.generateTXTRecord(ep); desiredTXT != nil {
if _, exists := txtRecordsMap[desiredTXT.DNSName]; !exists {
ep.WithProviderSpecific(providerSpecificForceUpdate, "true")
}
Expand All @@ -285,17 +284,7 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error
return endpoints, nil
}

// generateTXTRecord generates TXT records in either both formats (old and new) or new format only,
// 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
func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) *endpoint.Endpoint {
recordType := r.RecordType
// AWS Alias records are encoded as type "cname"
if isAlias, found := r.GetProviderSpecificProperty("alias"); found && isAlias == "true" && recordType == endpoint.RecordTypeA {
Expand All @@ -311,11 +300,8 @@ func (im *TXTRegistry) generateTXTRecordWithFilter(r *endpoint.Endpoint, filter
txtNew.WithSetIdentifier(r.SetIdentifier)
txtNew.Labels[endpoint.OwnedRecordLabelKey] = r.DNSName
txtNew.ProviderSpecific = r.ProviderSpecific
if filter(txtNew) {
endpoints = append(endpoints, txtNew)
}
}
return endpoints
return txtNew
}

// ApplyChanges updates dns provider with the changes
Expand All @@ -336,7 +322,9 @@ 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.isAbsent)...)
if txt := im.generateTXTRecord(r); txt != nil && im.existingTXTs.isAbsent(txt) {
filteredChanges.Create = append(filteredChanges.Create, txt)
}

if im.cacheInterval > 0 {
im.addToCache(r)
Expand All @@ -347,7 +335,9 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes)
// when we delete TXT records for which value has changed (due to new label) this would still work because
// !!! TXT record value is uniquely generated from the Labels of the endpoint. Hence old TXT record can be uniquely reconstructed
// !!! After migration to the new TXT registry format we can drop records in old format here!!!
filteredChanges.Delete = append(filteredChanges.Delete, im.generateTXTRecord(r)...)
if txt := im.generateTXTRecord(r); txt != nil {
filteredChanges.Delete = append(filteredChanges.Delete, txt)
}

if im.cacheInterval > 0 {
im.removeFromCache(r)
Expand All @@ -358,7 +348,9 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes)
for _, r := range filteredChanges.UpdateOld {
// when we updateOld TXT records for which value has changed (due to new label) this would still work because
// !!! TXT record value is uniquely generated from the Labels of the endpoint. Hence old TXT record can be uniquely reconstructed
filteredChanges.UpdateOld = append(filteredChanges.UpdateOld, im.generateTXTRecord(r)...)
if txt := im.generateTXTRecord(r); txt != nil {
filteredChanges.UpdateOld = append(filteredChanges.UpdateOld, txt)
}
// remove old version of record from cache
if im.cacheInterval > 0 {
im.removeFromCache(r)
Expand All @@ -367,7 +359,9 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes)

// make sure TXT records are consistently updated as well
for _, r := range filteredChanges.UpdateNew {
filteredChanges.UpdateNew = append(filteredChanges.UpdateNew, im.generateTXTRecord(r)...)
if txt := im.generateTXTRecord(r); txt != nil {
filteredChanges.UpdateNew = append(filteredChanges.UpdateNew, txt)
}
// add new version of record to cache
if im.cacheInterval > 0 {
im.addToCache(r)
Expand Down
32 changes: 15 additions & 17 deletions registry/txt_encryption_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,28 +109,26 @@ func TestGenerateTXTGenerateTextRecordEncryptionWihDecryption(t *testing.T) {
key := []byte(k)
r, err := NewTXTRegistry(p, "", "", "owner", time.Minute, "", []string{}, []string{}, true, key, "")
assert.NoError(t, err, "Error creating TXT registry")
txtRecords := r.generateTXTRecord(test.record)
assert.Len(t, txtRecords, len(test.record.Targets))
txt := r.generateTXTRecord(test.record)
assert.NotNil(t, txt)

for _, txt := range txtRecords {
// should return a TXT record with the encryption nonce label. At the moment nonce is not set as label.
assert.NotContains(t, txt.Labels, "txt-encryption-nonce")
// should return a TXT record with the encryption nonce label. At the moment nonce is not set as label.
assert.NotContains(t, txt.Labels, "txt-encryption-nonce")

assert.Len(t, txt.Targets, 1)
assert.LessOrEqual(t, len(txt.Targets), 1)
assert.Len(t, txt.Targets, 1)
assert.LessOrEqual(t, len(txt.Targets), 1)

// decrypt targets
for _, target := range txtRecords[0].Targets {
encryptedText, errUnquote := strconv.Unquote(target)
assert.NoError(t, errUnquote, "Error unquoting the encrypted text")
// decrypt targets
for _, target := range txt.Targets {
encryptedText, errUnquote := strconv.Unquote(target)
assert.NoError(t, errUnquote, "Error unquoting the encrypted text")

actual, nonce, errDecrypt := endpoint.DecryptText(encryptedText, r.txtEncryptAESKey)
assert.NoError(t, errDecrypt, "Error decrypting the encrypted text")
actual, nonce, errDecrypt := endpoint.DecryptText(encryptedText, r.txtEncryptAESKey)
assert.NoError(t, errDecrypt, "Error decrypting the encrypted text")

assert.True(t, strings.HasPrefix(encryptedText, nonce),
"Nonce '%s' should be a prefix of the encrypted text: '%s'", nonce, encryptedText)
assert.Equal(t, test.decrypted, actual)
}
assert.True(t, strings.HasPrefix(encryptedText, nonce),
"Nonce '%s' should be a prefix of the encrypted text: '%s'", nonce, encryptedText)
assert.Equal(t, test.decrypted, actual)
}
})
}
Expand Down
101 changes: 40 additions & 61 deletions registry/txt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1516,14 +1516,12 @@ func TestNewTXTScheme(t *testing.T) {

func TestGenerateTXT(t *testing.T) {
record := newEndpointWithOwner("foo.test-zone.example.org", "new-foo.loadbalancer.com", endpoint.RecordTypeCNAME, "owner")
expectedTXT := []*endpoint.Endpoint{
{
DNSName: "cname-foo.test-zone.example.org",
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
RecordType: endpoint.RecordTypeTXT,
Labels: map[string]string{
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
},
expectedTXT := &endpoint.Endpoint{
DNSName: "cname-foo.test-zone.example.org",
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
RecordType: endpoint.RecordTypeTXT,
Labels: map[string]string{
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
},
}
p := inmemory.NewInMemoryProvider()
Expand All @@ -1535,14 +1533,12 @@ func TestGenerateTXT(t *testing.T) {

func TestGenerateTXTWithMigration(t *testing.T) {
record := newEndpointWithOwner("foo.test-zone.example.org", "1.2.3.4", endpoint.RecordTypeA, "owner")
expectedTXTBeforeMigration := []*endpoint.Endpoint{
{
DNSName: "a-foo.test-zone.example.org",
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
RecordType: endpoint.RecordTypeTXT,
Labels: map[string]string{
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
},
expectedTXTBeforeMigration := &endpoint.Endpoint{
DNSName: "a-foo.test-zone.example.org",
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
RecordType: endpoint.RecordTypeTXT,
Labels: map[string]string{
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
},
}
p := inmemory.NewInMemoryProvider()
Expand All @@ -1551,14 +1547,12 @@ func TestGenerateTXTWithMigration(t *testing.T) {
gotTXTBeforeMigration := r.generateTXTRecord(record)
assert.Equal(t, expectedTXTBeforeMigration, gotTXTBeforeMigration)

expectedTXTAfterMigration := []*endpoint.Endpoint{
{
DNSName: "a-foo.test-zone.example.org",
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=foobar\""},
RecordType: endpoint.RecordTypeTXT,
Labels: map[string]string{
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
},
expectedTXTAfterMigration := &endpoint.Endpoint{
DNSName: "a-foo.test-zone.example.org",
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=foobar\""},
RecordType: endpoint.RecordTypeTXT,
Labels: map[string]string{
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
},
}

Expand All @@ -1570,14 +1564,12 @@ func TestGenerateTXTWithMigration(t *testing.T) {

func TestGenerateTXTForAAAA(t *testing.T) {
record := newEndpointWithOwner("foo.test-zone.example.org", "2001:DB8::1", endpoint.RecordTypeAAAA, "owner")
expectedTXT := []*endpoint.Endpoint{
{
DNSName: "aaaa-foo.test-zone.example.org",
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
RecordType: endpoint.RecordTypeTXT,
Labels: map[string]string{
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
},
expectedTXT := &endpoint.Endpoint{
DNSName: "aaaa-foo.test-zone.example.org",
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
RecordType: endpoint.RecordTypeTXT,
Labels: map[string]string{
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
},
}
p := inmemory.NewInMemoryProvider()
Expand All @@ -1595,8 +1587,8 @@ func TestFailGenerateTXT(t *testing.T) {
RecordType: endpoint.RecordTypeCNAME,
Labels: map[string]string{},
}
// A bad DNS name returns empty expected TXT
expectedTXT := []*endpoint.Endpoint{}
// A bad DNS name returns nil
var expectedTXT *endpoint.Endpoint
p := inmemory.NewInMemoryProvider()
p.CreateZone(testZone)
r, _ := NewTXTRegistry(p, "", "", "owner", time.Hour, "", []string{}, []string{}, false, nil, "")
Expand Down Expand Up @@ -1749,23 +1741,14 @@ func TestGenerateTXTRecordWithNewFormatOnly(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
r, _ := NewTXTRegistry(p, "", "", "owner", time.Hour, "", []string{}, []string{}, false, nil, "")
records := r.generateTXTRecord(tc.endpoint)
txt := r.generateTXTRecord(tc.endpoint)

assert.Len(t, records, tc.expectedRecords, tc.description)
assert.NotNil(t, txt, tc.description)

for _, record := range records {
assert.Equal(t, endpoint.RecordTypeTXT, record.RecordType)
}
assert.Equal(t, endpoint.RecordTypeTXT, txt.RecordType)

if tc.endpoint.RecordType == endpoint.RecordTypeAAAA {
hasNewFormat := false
for _, record := range records {
if strings.HasPrefix(record.DNSName, tc.expectedPrefix) {
hasNewFormat = true
break
}
}
assert.True(t, hasNewFormat,
assert.True(t, strings.HasPrefix(txt.DNSName, tc.expectedPrefix),
"Should have at least one record with prefix %s when using new format", tc.expectedPrefix)
}
})
Expand Down Expand Up @@ -2082,30 +2065,26 @@ func TestTXTRecordMigration(t *testing.T) {

newTXTRecord := r.generateTXTRecord(createdRecords[0])

expectedTXTRecords := []*endpoint.Endpoint{
{
DNSName: "a-bar.test-zone.example.org",
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=foo\""},
RecordType: endpoint.RecordTypeTXT,
},
expectedTXTRecord := endpoint.Endpoint{
DNSName: "a-bar.test-zone.example.org",
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=foo\""},
RecordType: endpoint.RecordTypeTXT,
}

assert.Equal(t, expectedTXTRecords[0].Targets, newTXTRecord[0].Targets)
assert.Equal(t, expectedTXTRecord.Targets, newTXTRecord.Targets)

r, _ = NewTXTRegistry(p, "%{record_type}-", "", "foobar", time.Hour, "", []string{}, []string{}, false, nil, "foo")

updatedRecords, _ := r.Records(ctx)

updatedTXTRecord := r.generateTXTRecord(updatedRecords[0])

expectedFinalTXT := []*endpoint.Endpoint{
{
DNSName: "a-bar.test-zone.example.org",
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=foobar\""},
RecordType: endpoint.RecordTypeTXT,
},
expectedFinalTXT := endpoint.Endpoint{
DNSName: "a-bar.test-zone.example.org",
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=foobar\""},
RecordType: endpoint.RecordTypeTXT,
}

assert.Equal(t, updatedTXTRecord[0].Targets, expectedFinalTXT[0].Targets)
assert.Equal(t, updatedTXTRecord.Targets, expectedFinalTXT.Targets)

}