-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(txt-register): reset existingTXTs even when ApplyChanges is skipped to avoid stale TXT records #5897
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.
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
base: master
Are you sure you want to change the base?
Conversation
…ed to avoid stale TXT records
Hi @u-kai. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
…reated after deletion
|
||
} | ||
|
||
// TestRecreateRecordAfterDeletion ensures that when A and TXT records are deleted, |
There was a problem hiding this comment.
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
/ok-to-test |
/lgtm |
@szuecs @mloiseleur |
@u-kai You need to rebase on master branch, then the CI will pass green. |
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I’ve merged the latest changes from the master branch. |
What does it do ?
This PR fixes an issue where stale TXT record information could remain when ApplyChanges was not called.
Previously,
existingTXTs
were only reset via a defer insideApplyChanges
, but whenplan.Changes.HasChanges()
was false (i.e., no DNS changes to apply), that method was skipped, and the cached TXT records were not cleared.This change ensures that
existingTXTs
are always reset, so the latest TXT record state is correctly reflected in subsequent reconciliation loops.Motivation
When ExternalDNS detected no DNS changes (
plan.Changes.HasChanges() == false
),ApplyChanges
was skipped and the existingTXTs reset logic inside it was not executed.As a result, outdated TXT record information could persist and cause ExternalDNS to skip creating new TXT records for newly created A/AAAA records.
By resetting
existingTXTs
every time (even when there are no changes), we ensure that TXT ownership information always stays in sync with the current state in Route53.Fixes: #5894
More