TXT registry: metadata format

[mcharriere]

Description

This PR is the continuation of the work done in https://github.com/kubernetes-sigs/external-dns/pull/3774

It includes a new option to select between 2 different modes:

• transition: Old and new formats are created.
• only-metadata: only metadata format is created.

I mostly resolved conflicts with master. Docs are still missing, but I think that all of the comments in the previous PR have been addressed. Including the integration with the AWS Alias behavior change.

Ref https://app.slack.com/client/T09NY5SBT/C771MKDKQ

Fixes https://github.com/kubernetes-sigs/external-dns/issues/3757

Checklist

• [ ] Unit tests updated
• [ ] End user documentation updated

[k8s-ci-robot]

Hi @mcharriere. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[danielloader]

Would be good to keep this going, having duplicated TXT records following both formats is creating a lot of visual pollution in the zone.

[mloiseleur]

/ok-to-test

[mloiseleur]

/lgtm

[szuecs]

The reference to slack does not work for me in the browser. Can you post a link to the discussion for weblicents?

[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 Kubernetes 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

[mcharriere]

I just rebased to master to keep up to date.

@mloiseleur @szuecs when you have some time, could you please take a look into this PR? thank you!

[mloiseleur]

/lgtm /assign @Raffo

[mloiseleur]

/retitle feat(TXT Registry): metadata format

[ivankatliarchuk]

/assign

[ivankatliarchuk]

Ups, accidently delited my comment. Hi @mcharriere would you be able to share how/where you tested this change?

Number of rerocds, number of zones, type of records. Something that could help me to test as well.

[mcharriere]

Ups, accidently delited my comment. Hi @mcharriere would you be able to share how/where you tested this change? Number of rerocds, number of zones, type of records. Something that could help me to test as well.

I ran my tests some time ago, and included:

• A few hundred records
• Upgrade from external-dns 0.11 (pre introduction of "new" format) to transition (this makes little sense, but anyways)
• upgrade from external-dns 0.11 (pre introduction of "new" format) to only-metadata
• upgrade from external-dns 0.13 to transition
• upgrade from external-dns 0.13 to only-metadata
• type of records: A, CNAME and AWS Aliases
• 2 zones, 1 public and 1 private
• All against Route53 because it's what I have available. I could do Azure as well but I need to work a bit on my setup.

[k8s-ci-robot]

PR needs rebase.

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.

[ivankatliarchuk]

Hi. Could you rebase and also provide a set of manual test steps using manifests and kubectl commands? I'll try to allocate some time to validate the behavior.

[mcharriere]

I was checking the changes and to me having both flags txt-new-format-only and txt-format feels kind of awkward.

Should we instead do something like:

• txt-new-format-only: disables old format & metadata format
• txt-metadata-format-only: disables old format & new format

Now, what would be the default behavior when both options are in false?

• Should we create old & new but skip metadata? This way the arguments are inconsistent.
• Should we create the 3 records? That keeps the current behavior from this PR but I still find it unfortunate.

[mloiseleur]

I understand you refer to this PR:

• https://github.com/kubernetes-sigs/external-dns/pull/4946

What about introducing a versioning in this format ? Something like

--txt-format=v2.1                    # Only new format with metadata
--txt-format=v2.0                    # Only new format without metadata
--txt-format=v1.0                    # New and old format, without metadata 

With a default to v1.0 FTM, and when the time comes, we will default to v2.1.

How a user is supposed to make the transition between current behavior and the new one ?

[ivankatliarchuk]

Let's plan this out. We might want to put together a brief proposal, but let's keep it simple.

So what we have

1. Default behavior that creates 2 records, and the format is invalid
2. We have a flag in master, that disable creation of old records but keep new invalid format records
3. A solution is needed to transition away from options 1 and 2. This could require a series of steps, deprecations, format changes.

I do not know how to solve current problem with TXT formats. This pull request suggests a possible path forward for sure.

[ivankatliarchuk]

/label tide/merge-method-squash

[z0rc]

Context: end user perspective, external-dns creates old and new format records, goal is to have single TXT record per regular record.

With #4946 merged. Should I plan migration to --txt-new-format-only from old + new to new and later plan migration from new to metadata? Or should I wait and plan migration from old + new directly to metadata once this PR merged and released?

[ivankatliarchuk]

Most likely this should be documented. Current pull-request is actually fixing, or moving to a new version of the format. So we going to have 3 formats

• old
• recent
• current, which is this pull request

Just throwing out an idea, I would expect a PR owner to work out options: releasing this pull request, means we only going to support following formats;

• recent
• current (new, this pr)

If flags set

--txt-new-format-only
--txt-format=v1.0

This will only generate newest format for version v1.0

If flag is set

--txt-format=v1.0

This will generate current and recent formats as fallback.

--txt-new-format-only

Will default behaviour to --txt-format=v1.2 where v1.0 and v1.2 is latest supported

Intereting idea with format version, this will reduce number of flags, as we could document txt format versions, and probably have something like --txt-format=v1.0,v1.2 aka multiple for migrations in the future.

Plan to completely remove old format in this or follow-up pull requests.

[mcharriere]

IMO keeping both flags (--txt-format and --txt-new-format-only) breaks the UX, just because the naming is not coherent. In one flag we refer as new but in the other we name them with versions.

--txt-format=v2.1                    # Only new format with metadata
--txt-format=v2.0                    # Only new format without metadata
--txt-format=v1.0                    # New and old format, without metadata 

I don't think those are versions of the format but instead their are versions of how the TXT registry works. If we wanted to have versioned formats, I'd expect that each version represents one format:

• v1.0: format used before 0.12; what we used to refer as old
• v2.0: format introduced in 0.12; what we used to refer as new, which includes the record type in it.
• v3.0: the format introduced in this PR, the metadata format

With that in mind, we can set the flag --txt-format=v1.0,v2.0 by default; keeping the existing behavior. Then, users can choose to disable the old format by setting --txt-format=v2.0 or enable the metadata format in one shot by setting it as --txt-format=v3.0. In this proposal, the flag --txt-new-format-only should be removed for consistency.

Eventually, the project can make the transition to v3 in further releases. We could discuss different approaches for that, but what is important is that in this PR, I'd only introduce the --txt-format without altering the behavior (this is not actually happening right now in the PR).

So, if you think this is reasonable and makes for a good interface, I can commit to do the changes soon.

[ivankatliarchuk]

Sounds like a plan. We definitely need a deprecation plan for the old formats. I'd like to propose removing the 0.12 format immediately after the next release – I'm not convinced it's worth keeping.

It would be valuable to obtain the approvers' views on how to tackle this.

[mloiseleur]

@mcharriere That makes sense to me. It looks clear from a user perspective.

cc @Raffo @szuecs

[szuecs]

I agree with @ivankatliarchuk and would just delete the old and new flags and only keep one. Then we have a clear migration path. If we want to refactor the flags as in https://github.com/kubernetes-sigs/external-dns/pull/4624#issuecomment-2689035175 instead of deleting them I don't have a strong opinion. IMO: less code the better, so I would just delete the code that is not the "new" format and have it the default.

[Raffo]

I agree with @szuecs on this one.

[mcharriere]

I'm not sure if I understand @szuecs suggestion.

would just delete the old and new flags and only keep one.

Could you clarify what are the old and the new flags?

Then we have a clear migration path. If we want to refactor the flags as in https://github.com/kubernetes-sigs/external-dns/pull/4624#issuecomment-2689035175 instead of deleting them I don't have a strong opinion.

If we delete both flags (--txt-new-format-only AND --txt-format); what would be the migration path and the mechanism to do it?

IMO: less code the better, so I would just delete the code that is not the "new" format and have it the default.

I'm fine with that, but deleting the any of the formats is IMO out of the scope of this PR. For the my own and reviewers sake it would better to keep things separated.

[ivankatliarchuk]

I my simplify it or make it more step action based.

Consider context, this change very high likely is going to be in next release cut.

1. We will do a next release 0.15.2 or whatever with all the flags, so next release is going to be a migration version, but without this change
2. As soon as there is a release cut, this change is going into master
3. Seems like, we going not to have --txt-format versions for now, instead this change expected to

• remove all the related flags and code related to old formats
• do what you suggested here https://github.com/kubernetes-sigs/external-dns/pull/4624#issuecomment-2689035175

@mloiseleur am I understand it correctly?

[mloiseleur]

@ivankatliarchuk There has been no release with #4946, so I do not see any blocker to go straight to step 3 and release next version with this new flag.

I don't think those are versions of the format but instead their are versions of how the TXT registry works.

@mcharriere It's correct. In a search of trying to name things correctly, wdyt of naming the flag --txt-registry or --txt-registry-version ?

[ivankatliarchuk]

Got it, make sense. So potential action steps

So action 1

1. Add new format, new flag. No need to support legacy or current formats, no need to create 2 TXT records, just single record with new format
2. We most likely going to cut next release, with migration steps to new format. And we need an issue created with removing old formats. We will pin this issue. e need to make sure we have python or go code embedded into codebase to cleanup Route53 registries from all the mess created with legacy formats
3. We capture feedback and going to delete all the code and related flags for previous versions

This is just an example plan

I see no point point in creating 2-3 TXT records for each Route53 record, quite hard to support such code and for example, we have no capacity in our Route53 to support such case, and it creates friction and bugs in the project. It would be beneficial, if code could capture which TXT records needs updated, at least as run-once operation with flag --once

[ivankatliarchuk]

Do you mean we should do in this release

remove all the related flags and code related to old formats