Skip to content

Commit 6e14fe8

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

File tree

3 files changed

+49
-66
lines changed

3 files changed

+49
-66
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: 18 additions & 31 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()
@@ -1570,14 +1568,12 @@ func TestGenerateTXTWithMigration(t *testing.T) {
15701568

15711569
func TestGenerateTXTForAAAA(t *testing.T) {
15721570
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-
},
1571+
expectedTXT := &endpoint.Endpoint{
1572+
DNSName: "aaaa-foo.test-zone.example.org",
1573+
Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
1574+
RecordType: endpoint.RecordTypeTXT,
1575+
Labels: map[string]string{
1576+
endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org",
15811577
},
15821578
}
15831579
p := inmemory.NewInMemoryProvider()
@@ -1595,8 +1591,8 @@ func TestFailGenerateTXT(t *testing.T) {
15951591
RecordType: endpoint.RecordTypeCNAME,
15961592
Labels: map[string]string{},
15971593
}
1598-
// A bad DNS name returns empty expected TXT
1599-
expectedTXT := []*endpoint.Endpoint{}
1594+
// A bad DNS name returns nil
1595+
var expectedTXT *endpoint.Endpoint
16001596
p := inmemory.NewInMemoryProvider()
16011597
p.CreateZone(testZone)
16021598
r, _ := NewTXTRegistry(p, "", "", "owner", time.Hour, "", []string{}, []string{}, false, nil, "")
@@ -1749,23 +1745,14 @@ func TestGenerateTXTRecordWithNewFormatOnly(t *testing.T) {
17491745
for _, tc := range testCases {
17501746
t.Run(tc.name, func(t *testing.T) {
17511747
r, _ := NewTXTRegistry(p, "", "", "owner", time.Hour, "", []string{}, []string{}, false, nil, "")
1752-
records := r.generateTXTRecord(tc.endpoint)
1748+
txt := r.generateTXTRecord(tc.endpoint)
17531749

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

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

17601754
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,
1755+
assert.True(t, strings.HasPrefix(txt.DNSName, tc.expectedPrefix),
17691756
"Should have at least one record with prefix %s when using new format", tc.expectedPrefix)
17701757
}
17711758
})

0 commit comments

Comments
 (0)