Skip to content

Commit 015748a

Browse files
bachorpbachorp
committed
refactor: explicitly return at most one endpoint in generateTXTRecord 
1 parent 5b6c92c commit 015748a

File tree

3 files changed

+71
-96
lines changed

3 files changed

+71
-96
lines changed

‎registry/txt.go‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -265,9 +265,8 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error
265265
// The migration is done for the TXT records owned by this instance only.
266266
if len(txtRecordsMap) > 0 && ep.Labels[endpoint.OwnerLabelKey] == im.ownerID {
267267
if plan.IsManagedRecord(ep.RecordType, im.managedRecordTypes, im.excludeRecordTypes) {
268-
// Get desired TXT records and detect the missing ones
269-
desiredTXTs := im.generateTXTRecord(ep)
270-
for _, desiredTXT := range desiredTXTs {
268+
// Get desired TXT record and check whether it is missing
269+
if desiredTXT := im.generateTXTRecord(ep); desiredTXT != nil {
271270
if _, exists := txtRecordsMap[desiredTXT.DNSName]; !exists {
272271
ep.WithProviderSpecific(providerSpecificForceUpdate, "true")
273272
}
@@ -285,13 +284,7 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error
285284
return endpoints, nil
286285
}
287286

288-
func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpoint {
289-
return im.generateTXTRecordWithFilter(r, func(ep *endpoint.Endpoint) bool { return true })
290-
}
291-
292-
func (im *TXTRegistry) generateTXTRecordWithFilter(r *endpoint.Endpoint, filter func(*endpoint.Endpoint) bool) []*endpoint.Endpoint {
293-
endpoints := make([]*endpoint.Endpoint, 0)
294-
287+
func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) *endpoint.Endpoint {
295288
recordType := r.RecordType
296289
// AWS Alias records are encoded as type "cname"
297290
if isAlias, found := r.GetProviderSpecificProperty("alias"); found && isAlias == "true" && recordType == endpoint.RecordTypeA {
@@ -307,11 +300,8 @@ func (im *TXTRegistry) generateTXTRecordWithFilter(r *endpoint.Endpoint, filter
307300
txtNew.WithSetIdentifier(r.SetIdentifier)
308301
txtNew.Labels[endpoint.OwnedRecordLabelKey] = r.DNSName
309302
txtNew.ProviderSpecific = r.ProviderSpecific
310-
if filter(txtNew) {
311-
endpoints = append(endpoints, txtNew)
312-
}
313303
}
314-
return endpoints
304+
return txtNew
315305
}
316306

317307
// ApplyChanges updates dns provider with the changes
@@ -332,7 +322,9 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes)
332322
}
333323
r.Labels[endpoint.OwnerLabelKey] = im.ownerID
334324

335-
filteredChanges.Create = append(filteredChanges.Create, im.generateTXTRecordWithFilter(r, im.existingTXTs.isAbsent)...)
325+
if txt := im.generateTXTRecord(r); txt != nil && im.existingTXTs.isAbsent(txt) {
326+
filteredChanges.Create = append(filteredChanges.Create, txt)
327+
}
336328

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

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

364360
// make sure TXT records are consistently updated as well
365361
for _, r := range filteredChanges.UpdateNew {
366-
filteredChanges.UpdateNew = append(filteredChanges.UpdateNew, im.generateTXTRecord(r)...)
362+
if txt := im.generateTXTRecord(r); txt != nil {
363+
filteredChanges.UpdateNew = append(filteredChanges.UpdateNew, txt)
364+
}
367365
// add new version of record to cache
368366
if im.cacheInterval > 0 {
369367
im.addToCache(r)

‎registry/txt_encryption_test.go‎

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -109,28 +109,26 @@ func TestGenerateTXTGenerateTextRecordEncryptionWihDecryption(t *testing.T) {
109109
key := []byte(k)
110110
r, err := NewTXTRegistry(p, "", "", "owner", time.Minute, "", []string{}, []string{}, true, key, "")
111111
assert.NoError(t, err, "Error creating TXT registry")
112-
txtRecords := r.generateTXTRecord(test.record)
113-
assert.Len(t, txtRecords, len(test.record.Targets))
112+
txt := r.generateTXTRecord(test.record)
113+
assert.NotNil(t, txt)
114114

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

119-
assert.Len(t, txt.Targets, 1)
120-
assert.LessOrEqual(t, len(txt.Targets), 1)
118+
assert.Len(t, txt.Targets, 1)
119+
assert.LessOrEqual(t, len(txt.Targets), 1)
121120

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

127-
actual, nonce, errDecrypt := endpoint.DecryptText(encryptedText, r.txtEncryptAESKey)
128-
assert.NoError(t, errDecrypt, "Error decrypting the encrypted text")
126+
actual, nonce, errDecrypt := endpoint.DecryptText(encryptedText, r.txtEncryptAESKey)
127+
assert.NoError(t, errDecrypt, "Error decrypting the encrypted text")
129128

130-
assert.True(t, strings.HasPrefix(encryptedText, nonce),
131-
"Nonce '%s' should be a prefix of the encrypted text: '%s'", nonce, encryptedText)
132-
assert.Equal(t, test.decrypted, actual)
133-
}
129+
assert.True(t, strings.HasPrefix(encryptedText, nonce),
130+
"Nonce '%s' should be a prefix of the encrypted text: '%s'", nonce, encryptedText)
131+
assert.Equal(t, test.decrypted, actual)
134132
}
135133
})
136134
}

‎registry/txt_test.go‎

Lines changed: 40 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -1516,14 +1516,12 @@ func TestNewTXTScheme(t *testing.T) {
15161516

15171517
func TestGenerateTXT(t *testing.T) {
15181518
record := newEndpointWithOwner("foo.test-zone.example.org", "new-foo.loadbalancer.com", endpoint.RecordTypeCNAME, "owner")
1519-
expectedTXT := []*endpoint.Endpoint{
1520-
{
1521-
DNSName: "cname-foo.test-zone.example.org",
1522-
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
1523-
RecordType: endpoint.RecordTypeTXT,
1524-
Labels: map[string]string{
1525-
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
1526-
},
1519+
expectedTXT := &endpoint.Endpoint{
1520+
DNSName: "cname-foo.test-zone.example.org",
1521+
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
1522+
RecordType: endpoint.RecordTypeTXT,
1523+
Labels: map[string]string{
1524+
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
15271525
},
15281526
}
15291527
p := inmemory.NewInMemoryProvider()
@@ -1535,14 +1533,12 @@ func TestGenerateTXT(t *testing.T) {
15351533

15361534
func TestGenerateTXTWithMigration(t *testing.T) {
15371535
record := newEndpointWithOwner("foo.test-zone.example.org", "1.2.3.4", endpoint.RecordTypeA, "owner")
1538-
expectedTXTBeforeMigration := []*endpoint.Endpoint{
1539-
{
1540-
DNSName: "a-foo.test-zone.example.org",
1541-
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
1542-
RecordType: endpoint.RecordTypeTXT,
1543-
Labels: map[string]string{
1544-
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
1545-
},
1536+
expectedTXTBeforeMigration := &endpoint.Endpoint{
1537+
DNSName: "a-foo.test-zone.example.org",
1538+
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
1539+
RecordType: endpoint.RecordTypeTXT,
1540+
Labels: map[string]string{
1541+
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
15461542
},
15471543
}
15481544
p := inmemory.NewInMemoryProvider()
@@ -1551,14 +1547,12 @@ func TestGenerateTXTWithMigration(t *testing.T) {
15511547
gotTXTBeforeMigration := r.generateTXTRecord(record)
15521548
assert.Equal(t, expectedTXTBeforeMigration, gotTXTBeforeMigration)
15531549

1554-
expectedTXTAfterMigration := []*endpoint.Endpoint{
1555-
{
1556-
DNSName: "a-foo.test-zone.example.org",
1557-
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=foobar\""},
1558-
RecordType: endpoint.RecordTypeTXT,
1559-
Labels: map[string]string{
1560-
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
1561-
},
1550+
expectedTXTAfterMigration := &endpoint.Endpoint{
1551+
DNSName: "a-foo.test-zone.example.org",
1552+
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=foobar\""},
1553+
RecordType: endpoint.RecordTypeTXT,
1554+
Labels: map[string]string{
1555+
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
15621556
},
15631557
}
15641558

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

15711565
func TestGenerateTXTForAAAA(t *testing.T) {
15721566
record := newEndpointWithOwner("foo.test-zone.example.org", "2001:DB8::1", endpoint.RecordTypeAAAA, "owner")
1573-
expectedTXT := []*endpoint.Endpoint{
1574-
{
1575-
DNSName: "aaaa-foo.test-zone.example.org",
1576-
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
1577-
RecordType: endpoint.RecordTypeTXT,
1578-
Labels: map[string]string{
1579-
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
1580-
},
1567+
expectedTXT := &endpoint.Endpoint{
1568+
DNSName: "aaaa-foo.test-zone.example.org",
1569+
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
1570+
RecordType: endpoint.RecordTypeTXT,
1571+
Labels: map[string]string{
1572+
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
15811573
},
15821574
}
15831575
p := inmemory.NewInMemoryProvider()
@@ -1595,8 +1587,8 @@ func TestFailGenerateTXT(t *testing.T) {
15951587
RecordType: endpoint.RecordTypeCNAME,
15961588
Labels: map[string]string{},
15971589
}
1598-
// A bad DNS name returns empty expected TXT
1599-
expectedTXT := []*endpoint.Endpoint{}
1590+
// A bad DNS name returns nil
1591+
var expectedTXT *endpoint.Endpoint
16001592
p := inmemory.NewInMemoryProvider()
16011593
p.CreateZone(testZone)
16021594
r, _ := NewTXTRegistry(p, "", "", "owner", time.Hour, "", []string{}, []string{}, false, nil, "")
@@ -1749,23 +1741,14 @@ func TestGenerateTXTRecordWithNewFormatOnly(t *testing.T) {
17491741
for _, tc := range testCases {
17501742
t.Run(tc.name, func(t *testing.T) {
17511743
r, _ := NewTXTRegistry(p, "", "", "owner", time.Hour, "", []string{}, []string{}, false, nil, "")
1752-
records := r.generateTXTRecord(tc.endpoint)
1744+
txt := r.generateTXTRecord(tc.endpoint)
17531745

1754-
assert.Len(t, records, tc.expectedRecords, tc.description)
1746+
assert.NotNil(t, txt, tc.description)
17551747

1756-
for _, record := range records {
1757-
assert.Equal(t, endpoint.RecordTypeTXT, record.RecordType)
1758-
}
1748+
assert.Equal(t, endpoint.RecordTypeTXT, txt.RecordType)
17591749

17601750
if tc.endpoint.RecordType == endpoint.RecordTypeAAAA {
1761-
hasNewFormat := false
1762-
for _, record := range records {
1763-
if strings.HasPrefix(record.DNSName, tc.expectedPrefix) {
1764-
hasNewFormat = true
1765-
break
1766-
}
1767-
}
1768-
assert.True(t, hasNewFormat,
1751+
assert.True(t, strings.HasPrefix(txt.DNSName, tc.expectedPrefix),
17691752
"Should have at least one record with prefix %s when using new format", tc.expectedPrefix)
17701753
}
17711754
})
@@ -2082,30 +2065,26 @@ func TestTXTRecordMigration(t *testing.T) {
20822065

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

2085-
expectedTXTRecords := []*endpoint.Endpoint{
2086-
{
2087-
DNSName: "a-bar.test-zone.example.org",
2088-
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=foo\""},
2089-
RecordType: endpoint.RecordTypeTXT,
2090-
},
2068+
expectedTXTRecord := endpoint.Endpoint{
2069+
DNSName: "a-bar.test-zone.example.org",
2070+
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=foo\""},
2071+
RecordType: endpoint.RecordTypeTXT,
20912072
}
20922073

2093-
assert.Equal(t, expectedTXTRecords[0].Targets, newTXTRecord[0].Targets)
2074+
assert.Equal(t, expectedTXTRecord.Targets, newTXTRecord.Targets)
20942075

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

20972078
updatedRecords, _ := r.Records(ctx)
20982079

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

2101-
expectedFinalTXT := []*endpoint.Endpoint{
2102-
{
2103-
DNSName: "a-bar.test-zone.example.org",
2104-
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=foobar\""},
2105-
RecordType: endpoint.RecordTypeTXT,
2106-
},
2082+
expectedFinalTXT := endpoint.Endpoint{
2083+
DNSName: "a-bar.test-zone.example.org",
2084+
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=foobar\""},
2085+
RecordType: endpoint.RecordTypeTXT,
21072086
}
21082087

2109-
assert.Equal(t, updatedTXTRecord[0].Targets, expectedFinalTXT[0].Targets)
2088+
assert.Equal(t, updatedTXTRecord.Targets, expectedFinalTXT.Targets)
21102089

21112090
}

0 commit comments

Comments
 (0)