feat(txt-registry): deprecate legacy txt-format

[ivankatliarchuk]

Description

• follow-up for feat(txt-registry): add option to use only new format #4946
• relates feat(TXT Registry): metadata format #4624
• relates Multiple design failures in new-format TXT registry ownership records #3757

TODO:

• ✅ fix tests
• ✅ test on real cluster and account
• ✅ review docs
• ✅ added TXT legacy record cleanup script

Checklist

• Unit tests updated
• End user documentation updated

Executed on real cluster with arguments

go run main.go \
    --provider=aws \
    --registry=txt \
    --source=fake \
    --aws-zone-type=private \
    --zone-id-filter=/hostedzone/XXXXXX \
    --log-level=debug \
    --policy=sync \
    --interval=60s \
    --fqdn-template=a1.ex.com

Without change, records current and old format created

aws route53 list-resource-record-sets --hosted-zone-id ${ZONE_UNDER_TEST} --query "ResourceRecordSets[?Type=='TXT'].{Name:Name, Value:ResourceRecords[0].Value}" --output table
-----------------------------------------------------------------------------
|                          ListResourceRecordSets                           |
+--------------------+------------------------------------------------------+
|        Name        |                        Value                         |
+--------------------+------------------------------------------------------+
|  a-fhbr.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-gwbl.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-hgmc.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-hmqw.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-hoxg.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-ipfd.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-melr.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-mkde.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-uknh.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-xiqj.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  fhbr.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  gwbl.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  hgmc.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  hmqw.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  hoxg.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  ipfd.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  melr.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  mkde.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  uknh.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  xiqj.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
+--------------------+------------------------------------------------------+

With the change. New format records created, letagcy TXT records left untouched

❯❯ aws route53 list-resource-record-sets --hosted-zone-id ${ZONE_UNDER_TEST} --query "ResourceRecordSets[?Type=='TXT'].{Name:Name, Value:ResourceRecords[0].Value}" --output table
-----------------------------------------------------------------------------
|                          ListResourceRecordSets                           |
+--------------------+------------------------------------------------------+
|        Name        |                        Value                         |
+--------------------+------------------------------------------------------+
|  a-dkmw.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-ihkf.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-iztb.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-jlmh.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-kmam.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-lqpv.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-rizx.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-sjfa.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-uvlq.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  a-vgpc.a1.ex.com. |  "heritage=external-dns,external-dns/owner=default"  |
|  bwwo.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  ecvx.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  ilan.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  iyda.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  nhhy.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  pazy.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  sdeo.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  tosb.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  wrej.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
|  yymt.a1.ex.com.   |  "heritage=external-dns,external-dns/owner=default"  |
+--------------------+------------------------------------------------------+
``

[ivankatliarchuk]

/label tide/merge-method-squash

[ivankatliarchuk]

/kind cleanup

[mcharriere]

Legacy format has one owner ID, then for non legacy to have another owner ID, all records needs to be recreated

which case is that one? changing owner id is not even supported by external-dns

EDIT:
Ah I think I understand your comment, the first file in that commit says something about owner-id. Please ignore and check the rest of the files, in particular docs/registry.md. The ‎docs/proposal/registry.md not really relevant for the discussion.

[ivankatliarchuk]

I'm sorry, I'm not entirely clear on what's being asked. The issue is that I haven't discovered a reliable way to automate legacy TXT record deletions without downtimes. That's why it's not in this pull request. If someone knows how to automate this, nothing wrong to open a pull request with proposed functionality.

[mrozentsvayg]

The author of the new format saw the possibility of cleaning up the old registry records here: 25c7cb2

Although some of the code changed since then, this line still suggest the clean up can be done there. I'm playing a bit with it but there are some edge cases that need further research.

this is not entirely correct, the reservation here is that the Registry.ApplyChanges() code only gets called on plan.Changes.HasChanges()
it's a graceful approach of deleting the records(provider agnostic), but only eventually consistent

the effective solution in my opinion could be adding specific one-off mode for the external-dns or building a separate tool to aggressively scan the txt registry regardless of the changes and:

1. delete the old-style txt records with the respect to the existing prefix/suffix/owner configuration (can't do much more for all the combinations);
2. inform if TXT records are missing for the expected DNS records (now it's also only get handled in case of changes) and potentially try to fix it;
3. inform if there are extra TXT records in the zone unexpected for the current DNS records and prefix/suffix/owner configuration;

if we would like to discuss it further, it doesn't necessarily have to block this PR, which essentially is to stop adding more garbage TXT records, especially for the new users

[mcharriere]

this is not entirely correct, the reservation here is that the Registry.ApplyChanges() code only gets called on plan.Changes.HasChanges()

the txt registry has a way to force an update (see https://github.com/kubernetes-sigs/external-dns/blob/master/registry/txt.go#L217); making the solution possible.

This PR deletes a part that is needed to implement the cleanup of the old-format records.

[ivankatliarchuk]

Ok. I'll put this pr on hold. Let's get back to it in few month time.

/hold

[ivankatliarchuk]

Seems like noone suggested a solution to what was discussed here.

Rationale to remove legacy TXT formats

• We could not or it's extremely difficult to fix bugs related to formats, and list is growing
• We are stuck with both formats and no plan

Drawback

• This PR is not going to help cleanup environments. I personally see not safe/reliable way doing that, if it exists, the community had time to implement one.

Two options, merging this PR or closing. I'm fine with any of them

@Raffo or @szuecs could you make a decision. Go or No-go?

[frittentheke]

Seems like noone suggested a solution to what was discussed here.

Rationale to remove legacy TXT formats

* We could not or it's extremely difficult to fix bugs related to formats, and list is growing
* We are stuck with both formats and no plan

Regarding any bugs and potential fixes for issues like #2903 I also believe the exposure and complexity needs to be reduced.

Drawback

* This PR is not going to help cleanup environments. I personally see not safe/reliable way doing that, if it exists, the community had time to implement one.

It's a bit unfortunate that there never was an external-dns bridge release doing some sort of consistency check and then active migration to the new format. Including logging warnings about potential inconsistencies.

Two options, merging this PR or closing. I'm fine with any of them
@Raffo or @szuecs could you make a decision. Go or No-go?

👀

[Evesy]

Between the two I'm in favour of making a breaking change and dropping the legacy format since there's little appetite in providing an automated migration path.

We've performed a migration for all our records (Google & Cloudflare providers) and though it was a bit of toil it wasn't too much effort to do so.

I don't feel continuing to have both TXT formats, especially given they each have their own caveats, is sustainable to keep long term, and it's not great user experience with the current implementation

[mloiseleur]

/approve cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from mloiseleur. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mloiseleur]

/lgtm cancel

[mloiseleur]

Sorry, I thought I was on an other PR. I resumed this PR to its previous state.

[Yunir]

do we want to change cname-* to a-* for AWS txt record for aliases to make it similar to ipv6 AAAA-record&aaaa-* txt-record to just remove confusion? (confussion discussion is going here: #2903 (comment)) Or we can leave it just as a legacy of a product, because it does not brake anything.
If we want to change it, then I think it's easier first to do rename-PR for TXT new format of A alias record for AWS from prefix cname-* to a-*. If no, then we are good to go with this PR.
I am also eager to reduce the amount of txt records to one.

[frittentheke]

do we want to change cname-* to a-* for AWS txt record for aliases to make it similar to ipv6 AAAA-record&aaaa-* txt-record to just remove confusion? (confussion discussion is going here: #2903 (comment)) Or we can leave it just as a legacy of a product, because it does not brake anything. If we want to change it, then I think it's easier first to do rename-PR for TXT new format of A alias record for AWS from prefix cname-* to a-*. If no, then we are good to go with this PR. I am also eager to reduce the amount of txt records to one.

Yes. Remove old format first! No option, no switch. Simply drop it from external-dns.
The migration phase has been going on long enough as having two formats now actively causes issues with new changes and bugfixes.

If that is done, I strongly vote for fixing the prefix for A-records to a-. Them being an alias is simply the technicality of AWS (or other providers which support it). The record TYPE is still either A or AAAA.

Just my 2 cents.

[Yunir]

Yes. Remove old format first! ... If that is done, I strongly vote for fixing the prefix for A-records to a-.

I think you misread my message.
If we want to move from cname- to a-, then it is safer to do it prior the removal of old-txt format. Why:

• old-txt-format (txt w/o prefix) needs for external-dns to continue manage the records w/o any disruption while refactoring
• new-txt-format can be changed easily, because it's additional record for now (refactor from cname- to a-)

But if we first remove old-txt format, then we will be left without 'intermediate' working option (it will introduce the 'breaking change' with 'action needed') - external-dns will manage either only A record which has cname-<domain> or a-<domain>, not both. It will be harder to migrate. If we do it, then external-dns will lose rights to manage all records, because it would try to search for a-<domain> only, but old version were creating just cname-<domain>.

So, I am more asking if we want to remove that confusion with prefix, then propose to do it first, as it will be much easier to do the refactor and migrate. But if we don't want to remove confusion, then we might just leave cname- prefix for A-alias aws records as a legacy, it is also fine.

@frittentheke, I understand your willing to move this PR faster, but before the decision made, you may still fallback to the usage of --txt-new-format-only flag as a temporary solution.

[frittentheke]

Yes. Remove old format first! ... If that is done, I strongly vote for fixing the prefix for A-records to a-.

I think you misread my message. If we want to move from cname- to a-, then it is safer to do it prior the removal of old-txt format. Why:

* old-txt-format (txt w/o prefix) needs for external-dns to continue manage the records w/o any disruption while refactoring

* new-txt-format can be changed easily, because it's additional record for now (refactor from `cname-` to `a-`)

But if we first remove old-txt format, then we will be left without 'intermediate' working option (it will introduce the 'breaking change' with 'action needed') - external-dns will manage either only A record which has cname-<domain> or a-<domain>, not both. It will be harder to migrate. If we do it, then external-dns will lose rights to manage all records, because it would try to search for a-<domain> only, but old version were creating just cname-<domain>.

@frittentheke, I understand your willing to move this PR faster, but before the decision made, you may still fallback to the usage of --txt-new-format-only flag as a temporary solution.

No I did not misread it. The opportunity to clean up anything before the new format is LONG gone. It's out there, for quite a while and the last releases actively communicated the intend to remove the old format. We pushed ahead exactly as you said and applied --txt-new-format-only in our environment and removed all legacy TXT records in the process.
See e.g. https://github.com/kubernetes-sigs/external-dns/blob/cd624b6f550c75a5accab84b1354f851e232f384/docs/registry/txt.md#record-format-options which go us started early knowing this very PR here was already open and deprecation was coming.

In the field you will now likely encounter a wild mix of both TXT record formats.

There also was plenty discussion around any potential shortcomings in #3757.

So, I am more asking if we want to remove that confusion with prefix, then propose to do it first, as it will be much easier to do the refactor and migrate. But if we don't want to remove confusion, then we might just leave cname- prefix for A-alias aws records as a legacy, it is also fine.

To clarify what I meant:

Remove the old TXT record format for good (making any of those TXT records just meaningless garbage to external-dns). Then and only then(!) pick up on the renaming bugfix and please add an active conversion / rewrite of the prefix. This should be fairly easy then as it's not possible to have a CNAME and and A record at the same time. So a simple up-conversion is possible. But please don't give up on making things right in regard to the prefix. It's confusing and wrong naming one a cname- and the other an aaaa-. And it's only the non-functional record-keeping TXT records after all that need the cleanup. All of which are totally owned and maintained by external-dns and its responsibility to keep tidy.

[ivankatliarchuk]

This particular TXT registry format(old and new to an extend) has been a long-standing problem. New TXT formats have been available for a long time, and disabling the old format would not introduce a breaking change because the TXT records are just ownership records, while the new one has been standard for many versions. If we remove support for these legacy TXT records, the main concern will be dangling records from the old format, which is why we are waiting where someone would like to implement a solution to reliably do a cleanup for all combinations of the config and providers.

Once again - as these are primarily ownership records, their removal should not cause any functional breakage. But as usual, it could be, that there are could be some issues or edge cases. But is difficult to predict.

Users who don't require two TXT ownership records per DNS entry have viable options. They can either encrypt their TXT records, which ensures only the new format TXT record is created, or they can use the --txt-new-format-only flag to exclusively enable new records. It's important to note that, to date, not a single bug or issue related to this functionality has been identified. In both scenarios, any existing old TXT ownership records will need to be removed either manually or via proprietary scripts to prevent potential downtimes.

[ivankatliarchuk]

But if we first remove old-txt format, then we will be left without 'intermediate' working option (it will introduce the 'breaking change' with 'action needed') - external-dns will manage either only A record which has cname-<domain> or a-<domain>, not both. It will be harder to migrate. If we do it, then external-dns will lose rights to manage all records, because it would try to search for a-<domain> only, but old version were creating just cname-<domain>.

There is a intermediate option - which is current TXT format(new format), introduced years back. Is there is an open issue that describes this problem, I could have a look, just need to know how to reproduce it. More important, if the prefix should be cname-, but is an aaa- or whatever, it doesn't necessarily break functionality. These TXT records are primarily for external-dns to determine if it manages or owns a particular record.

[ivankatliarchuk]

If that is done, I strongly vote for fixing the prefix for A-records to a-. Them being an alias is simply the technicality of AWS (or other providers which support it). The record TYPE is still either A or AAAA.

I'll try to have a look at this one. I remember it was mentioned in one of the issues. Just need to find and see if there is an easy fix.