Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
8 changes: 6 additions & 2 deletions registry/txt.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,12 @@ func (im *TXTRegistry) OwnerID() string {
// If TXT records was created previously to indicate ownership its corresponding value
// will be added to the endpoints Labels map
func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error) {
// existingTXTs must always hold the latest TXT records, so it needs to be reset every time.
// Previously, it was reset with a defer after ApplyChanges, but ApplyChanges is not called
// when plan.HasChanges() is false (i.e., when there are no changes to apply).
// In that case, stale TXT record information could remain, so we reset it here instead.
im.existingTXTs.reset()

// If we have the zones cached AND we have refreshed the cache since the
// last given interval, then just use the cached results.
if im.recordsCache != nil && time.Since(im.recordsCacheRefreshTime) < im.cacheInterval {
Expand Down Expand Up @@ -321,8 +327,6 @@ func (im *TXTRegistry) generateTXTRecordWithFilter(r *endpoint.Endpoint, filter
// 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),
Expand Down
80 changes: 80 additions & 0 deletions registry/txt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ You may obtain a copy of the License at
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

See the License for the specific language governing permissions and
limitations under the License.
*/
Expand Down Expand Up @@ -2109,3 +2110,82 @@ func TestTXTRecordMigration(t *testing.T) {
assert.Equal(t, updatedTXTRecord[0].Targets, expectedFinalTXT[0].Targets)

}

// TestRecreateRecordAfterDeletion ensures that when A and TXT records are deleted,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that this test failed on previous versions with the following output:

kai@kainoMacBook-Pro external-dns % go test ./registry/...
--- FAIL: TestRecreateRecordAfterDeletion (0.00s)
    txt_test.go:2190:
                Error Trace:    /Users/kai/external-dns/registry/txt_test.go:2190
                Error:          Should be true
                Test:           TestRecreateRecordAfterDeletion
                Messages:       Expected records after reconciliation: [bar.test-zone.example.org 0 IN A  1.2.3.4 [] a-bar.test-zone.example.org 0 IN TXT  "heritage=external-dns,external-dns/owner=foo" []], but got: [bar.test-zone.example.org 0 IN A  1.2.3.4 []]
FAIL
FAIL    sigs.k8s.io/external-dns/registry       0.345s
FAIL

// both are correctly recreated in subsequent reconciliation loops.
// This prevents regression of the issue where stale TXT record state
// caused ExternalDNS to skip recreating TXT records after deletion.
func TestRecreateRecordAfterDeletion(t *testing.T) {
ownerID := "foo"
ctx := context.Background()
p := inmemory.NewInMemoryProvider()
p.CreateZone(testZone)

r, _ := NewTXTRegistry(p, "%{record_type}-", "", "foo", 0, "", []string{endpoint.RecordTypeA}, []string{}, false, nil, "")

createdRecords := newEndpointWithOwnerAndLabels("bar.test-zone.example.org", "1.2.3.4", endpoint.RecordTypeA, ownerID, nil)
txtRecord := r.generateTXTRecord(createdRecords)

// 1. Create initial A and TXT records.
creates := append([]*endpoint.Endpoint{createdRecords}, txtRecord...)
err := p.ApplyChanges(ctx, &plan.Changes{
Create: creates,
})
assert.NoError(t, err)

// 2. Simulate a "no change" reconciliation (ApplyChanges won't be called).
desired := []*endpoint.Endpoint{
{
DNSName: "bar.test-zone.example.org",
Targets: endpoint.Targets{
"1.2.3.4",
},
RecordType: endpoint.RecordTypeA,
},
}

records, err := r.Records(ctx)
assert.NoError(t, err)

calculated := &plan.Plan{
Policies: []plan.Policy{&plan.SyncPolicy{}},
ManagedRecords: []string{endpoint.RecordTypeA},
Current: records,
Desired: desired,
OwnerID: ownerID,
}
calculated = calculated.Calculate()
// ApplyChanges is not called to simulate no changes.
assert.False(t, calculated.Changes.HasChanges(), "There should be no changes")

// 3. Delete both A and TXT records (simulate manual deletion)
deletes := append([]*endpoint.Endpoint{createdRecords}, txtRecord...)
err = p.ApplyChanges(ctx, &plan.Changes{
Delete: deletes,
})
assert.NoError(t, err)

// 4. Run reconciliation again — both A and TXT should be recreated.
records, err = r.Records(ctx)
assert.NoError(t, err)

calculated = &plan.Plan{
Policies: []plan.Policy{&plan.SyncPolicy{}},
ManagedRecords: []string{endpoint.RecordTypeA},
Current: records,
Desired: desired,
OwnerID: ownerID,
}
calculated = calculated.Calculate()
if !calculated.Changes.HasChanges() {
assert.Fail(t, "There should be changes")
}

err = r.ApplyChanges(ctx, calculated.Changes)
assert.NoError(t, err)

// 5. Verify that both A and TXT records are recreated successfully.
records, err = p.Records(ctx)
assert.NoError(t, err)
assert.True(t, testutils.SameEndpoints(records, append(desired, txtRecord...)), "Expected records after reconciliation: %v, but got: %v", append(desired, txtRecord...), records)
}