Registry record type #2157

k0da · 2021-07-06T14:56:56Z

Description
Add additional txt record with record type marker

In order to track multiple record types with the same name, lets migrate
to new format, were record name contains record type in it.

For migration purposes old and new format will be kept for a while.

Fixes #1923

Checklist

Unit tests updated
[] End user documentation updated

k0da · 2021-07-06T14:57:34Z

/assign njuettner

haslersn · 2021-07-06T15:30:31Z

registry/txt.go

+	if len(DNSName) < 2 {
+		return pr.prefix + recordT + DNSName[0] + pr.suffix
+	}
+	return pr.prefix + recordT + DNSName[0] + pr.suffix + "." + DNSName[1]


Hierarchically, I think it would make more sense to have the record type precede the prefix. I personally would like to use:

<recordType><prefix><endpointDNSName>

@haslersn amended

jgrumboe · 2021-07-06T20:29:14Z

@k0da
I think your current implementation to add the record type with a fixed "." as delimiter will break the --txt-suffix behaviour or at least the intention to have the registry records alphabetically next to the original record. With your current implementation they would open a subdomain and that decision should be up to the user I would say.

Would it be a good thing to make the separator configurable or just go for a "-" or "_"?

k0da · 2021-07-06T20:47:00Z

@haslersn I think @jgrumboe have a good point here, lets stick to -

haslersn · 2021-07-06T20:51:29Z

@jgrumboe When adding the record type before DNSName[0], then it's not alphabetically next to the original record anyway, right?

My preference would be to (have the option to) configure it similar to the naming scheme of today's SRV records and DKIM records.

jgrumboe · 2021-07-06T21:08:29Z

@haslersn You're right. (I'm currently looking from a mobile, so it was just a quick look.)
I think a good option would be to look if txt-prefix or txt-suffix is set (both are mutual exclusive) and add the record type directly to prefix or suffix before those are applied to the DNS name.

Sorry, i don't understand what you mean with SRV and DKIM records.

haslersn · 2021-07-06T21:15:46Z

@jgrumboe

Sorry, i don't understand what you mean with SRV and DKIM records.

SRV records have the naming scheme _<service>._<protocol>.<name>
DKIM records have the naming scheme <selector>._domainkey.<name>

Similarly, I'd like to use the naming scheme <type>._heritage.<name> for my TXT registry records.

jgrumboe · 2021-07-06T21:30:11Z

@haslersn

Similarly, I'd like to use the naming schema <type>._heritage.<name> for my TXT registry records.

Would be already possible if, for example, you set txt-prefix to "._heritage." and record type will be added without delimiter. You would need to take care yourself in txt-prefix to set a delimiter of your choice for the record type, as in my example it's the first "." in front of "_heritage.".

Not much adoption in code needed as getting rid of hard-coded delimiter. This would also need no separate configuration option.

What do you think?

haslersn · 2021-07-06T21:39:12Z

@jgrumboe That's pretty much what I proposed. You just added that the type is added to the suffix instead, if a suffix is set.

k0da · 2021-07-06T21:52:05Z

@haslersn @jgrumboe if prefix is not set, and delimeter is not there, record name will be quite ugly out of the box. That would require to set prefix as a delimeter.

I don't like an idea of dancing around suffix prefix. Intention is to change default TXT record name from to . Then if suffix or prefix are configured, entry will be changed accordingly. Having record type in the beginning gives a clear context which record it belongs to.

jgrumboe · 2021-07-07T07:47:18Z

@k0da I agree that you would need some kind of delimiter set without any prefix or suffix set. Otherwise, it will look not pleasant, as you said.

"dancing around suffix and prefix" is inevitable, I think because that's why the suffix was introduced: to keep the original name in front and suffix it with registry related information. The record type is just another registry related information, in my opinion.

k0da · 2021-07-07T07:56:40Z

@jgrumboe My point is record type should be part of the initial name. And them prefixed, suffixed as user wants. Since I'm not suffix/prefix user, I'd like to gather some feedback from users of those, how it is used and what is the primary usecase. Adding record type is a functional change required to open a posibility to manage multiple record types, IIUC prefix/suffix is more decorator thingie.

jgrumboe · 2021-07-07T08:08:11Z

@k0da I wouldn't say that prefix/suffix is decoration. In use-cases where you want external-dns to manage CNAMEs a prefix or suffix is a must as otherwise the registry records wouldn't get created because of conflicts.

I understand that making the record type part of the record name is mitigating this already, but my feedback is that I would like to see this configurable for the user as it would feel like an "automated set" prefix. My use case with suffix was to make life easier for DNS management people, which are managing through a UI (like Infoblox) and have related records listed next to each other in alphabetical order. Others might prefer to have the registry records "hidden" from first sight within some subdomain.
My feedback is: I still would like to have the user decide how registry records should be inserted into their DNS system.

k0da · 2021-07-07T08:14:58Z

@jgrumboe so your case would be <prefix><recordtype>-<name> ?

jgrumboe · 2021-07-07T08:25:52Z

@k0da No, I'm the suffix fan-boy :)

The suggested implementation in this PR would be <recordtype>.<prefix><name>.
I would change this to <recordtype>-<prefix><name> and for my use case I would like to see <name>-<recordtype><suffix>, probably?! (Naming is hard!)
Either way, I bet you'll need some switch-case to distinguish what to do if neither prefix/suffix is set or one of them.

Just a very different idea: What about introducing a %{record_type} placeholder/template-variable for prefix/suffix?
So you wouldn't need to care about where to place the record type. Instead, the user would place this in prefix/suffix.

k0da · 2021-07-07T08:57:59Z

@jgrumboe I see your point now.

Right now, resulting name is cname-txt.foobar.test-zone.example.org where txt is a prefix (will probably open a PR to change prefix/suffix to something more readable/obvious). Letting user to control where to put recordtype will make extreamly hard to find out original endpoint name from txt record. Right now I just drop record type and keep suffix/prefix record the same. Having this configurable will introduce more complexity and possible further breakages.

jgrumboe · 2021-07-07T09:05:25Z

Sorry, "smashing" your PR wasn't my intention.

And what about this kind of templating variable? Wouldn't that be fairly easy to implement and help your case of multiple registry records?

k0da · 2021-07-07T09:52:44Z

@jgrumboe we can't use template here: https://github.com/kubernetes-sigs/external-dns/blob/master/registry/txt.go#L246

haslersn · 2021-07-07T10:25:06Z

I'd love the template ({{ record_type }}), if possible.

@k0da

@jgrumboe we can't use template here: https://github.com/kubernetes-sigs/external-dns/blob/master/registry/txt.go#L246

Why not? The function uses pr.prefix, which would contain the (not yet substituted) template, right?

If that's not possible, then we need to stick to something like @jgrumboe and I proposed.

@jgrumboe

I would change this to <recordtype>-<prefix><name>

Why prescribe the delimiter -, if it can be part of the prefix? See one of my very first comments: #2157 (comment)

k0da · 2021-07-07T10:40:21Z

@haslersn introducing a template would neglate suffix prefix and it would be something more like unique identifier.

I don't see quick and easy way parse TXT record into endpoint name from given template. Mind to provide dummy snippet?

@jgrumboe @haslersn you guys trying to make recordtype as part of either prefix or suffix, while it should be part of name, and then prefixed or suffixed. this way we can drop prefix/suffix and then parse it back to endpoint name.

haslersn · 2021-07-07T10:53:58Z

@k0da Here's a snippet without suffix support:

func (pr affixNameMapper) toEndpointName(txtDNSName string) string {
    lowerDNSName := strings.ToLower(txtDNSName)
    for _, type := range supportedTypes {
        instantiatedPrefix = strings.ReplaceAll(pr.prefix, "%{record_type}", type)
        if strings.HasPrefix(lowerDNSName, instantiatedPrefix) {
            return strings.TrimPrefix(lowerDNSName, instantiatedPrefix)
        }
    }
    return ""
}

However, (as soon as IPv6 support is introduced) there would a potential problem if one puts %{record_type} to the end of the prefix: Both

A aaaa.example.com
AAAA a.example.com

would result in the same name in the TXT registry.

k0da · 2022-03-31T15:30:29Z

Done

k0da · 2022-04-04T12:49:51Z

@Raffo ^^

Raffo

@k0da I added a few more comments, thank you for you updates.

I have also another request: can you add additional comments in the code on how to remove all the parts that we will have to eventually remove once the migration is not done. I want to make sure that we do not end up with more code than needed on a long term.

docs/registry.md

registry/txt.go

Raffo · 2022-04-06T08:22:29Z

Something is off with the linter 😞

k0da · 2022-04-06T10:05:14Z

@Raffo review points has been addressed. Please take a look

k0da · 2022-04-08T09:46:54Z

@Raffo re linter issues. Looks like something pulls go 1.18 https://github.com/kubernetes-sigs/external-dns/runs/5849391653?check_suite_focus=true#step:2:11

Raffo · 2022-04-14T13:09:53Z

@k0da yeah I saw that something bumped to go 1.18 so that’s gonna be failures for all builds 😭 I will go and check what’s wrong 🕵️

Raffo · 2022-04-14T13:47:48Z

You can merge master in and it should fix the linting issue.

In order to track multiple record types with the same name, lets migrate to new format, were record name contains record type in it. Signed-off-by: Dinar Valeev <dinar.valeev@absa.africa>

k0da · 2022-04-14T14:57:12Z

@Raffo rebased, tests and linter are green

Raffo · 2022-04-19T08:01:42Z

/approve
/lgtm

k8s-ci-robot · 2022-04-19T08:02:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k0da, Raffo

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Raffo]

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

msvticket · 2022-06-03T14:49:47Z

When upgrading to 0.12.0 from 0.10.2 I think it is this change that causes errors like

time="2022-06-03T14:13:09Z" level=error msg="InvalidChangeBatch: [Tried to delete resource record set [name='cname-my-service-pr32.example.com.', type='TXT'] but it was not found]\n\tstatus code: 400, request id: 551d501f-32cb-4e8b-a952-00c5779f72f1"
time="2022-06-03T14:13:09Z" level=error msg="failed to submit all changes for the following zones: [/hostedzone/ABC123456789]"

This is on AWS using Route53. Is there a setting to avoid this error?

cep21 · 2023-03-30T15:45:11Z

Hi,

Is it possible to revert this breaking change to resolve #2157 ?

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 6, 2021

k8s-ci-robot requested review from njuettner and seanmalloy July 6, 2021 14:57

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 6, 2021

k8s-ci-robot assigned njuettner Jul 6, 2021

k0da force-pushed the registry_record_type branch from 240dcc9 to 829e8cb Compare July 6, 2021 15:08

k0da mentioned this pull request Jul 6, 2021

External-dns project scope #1923

Closed

haslersn reviewed Jul 6, 2021

View reviewed changes

k0da force-pushed the registry_record_type branch 2 times, most recently from 9426904 to d3caadd Compare July 6, 2021 17:33

k0da requested a review from Raffo March 31, 2022 15:30

Raffo reviewed Apr 6, 2022

View reviewed changes

docs/registry.md Outdated Show resolved Hide resolved

docs/registry.md Outdated Show resolved Hide resolved

docs/registry.md Outdated Show resolved Hide resolved

docs/registry.md Outdated Show resolved Hide resolved

registry/txt.go Outdated Show resolved Hide resolved

k0da force-pushed the registry_record_type branch from d1caadf to d3e96cf Compare April 6, 2022 10:04

k0da requested a review from Raffo April 6, 2022 10:05

k0da force-pushed the registry_record_type branch from d3e96cf to 32d748b Compare April 6, 2022 10:45

Add additional txt record with record type marker

25c7cb2

In order to track multiple record types with the same name, lets migrate to new format, were record name contains record type in it. Signed-off-by: Dinar Valeev <dinar.valeev@absa.africa>

k0da force-pushed the registry_record_type branch from 32d748b to 25c7cb2 Compare April 14, 2022 14:39

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2022

k8s-ci-robot merged commit bb635fb into kubernetes-sigs:master Apr 19, 2022

lou-lan mentioned this pull request Apr 29, 2022

Update proposal for downgrading #2730

Closed

2 tasks

mac-chaffee mentioned this pull request May 5, 2022

No way to safely change --txt-prefix; required for CNAMEs #2737

Closed

This was referenced May 26, 2022

Changed the Deployment strategy to 'Recreate' so multiple external-dns pods don't conflict with each other. #2772

Merged

v0.12.0: Improve migration process for txt-prefix/suffix users #2773

Open

rust84 mentioned this pull request Jun 9, 2022

external-dns is unable to sync AWS records #2793

Closed

mgruener mentioned this pull request Jun 22, 2022

New TXT record breaks downward compatibility by retroactively limiting record length #2839

Closed

johngmyers mentioned this pull request Jun 7, 2023

Add johngmyers to reviewers #3664

Merged

2 tasks

johngmyers mentioned this pull request Sep 13, 2023

Add johngmyers as approver #3931

Merged

2 tasks

Registry record type #2157

Registry record type #2157

Conversation

k0da commented Jul 6, 2021

k0da commented Jul 6, 2021

haslersn Jul 6, 2021

Choose a reason for hiding this comment

k0da Jul 6, 2021

Choose a reason for hiding this comment

jgrumboe commented Jul 6, 2021

k0da commented Jul 6, 2021

haslersn commented Jul 6, 2021 • edited Loading

jgrumboe commented Jul 6, 2021 • edited Loading

haslersn commented Jul 6, 2021 • edited Loading

jgrumboe commented Jul 6, 2021 • edited Loading

haslersn commented Jul 6, 2021

k0da commented Jul 6, 2021

jgrumboe commented Jul 7, 2021

k0da commented Jul 7, 2021

jgrumboe commented Jul 7, 2021

k0da commented Jul 7, 2021

jgrumboe commented Jul 7, 2021

k0da commented Jul 7, 2021 • edited Loading

jgrumboe commented Jul 7, 2021

k0da commented Jul 7, 2021

haslersn commented Jul 7, 2021

k0da commented Jul 7, 2021

haslersn commented Jul 7, 2021 • edited Loading

k0da commented Mar 31, 2022

k0da commented Apr 4, 2022

Raffo left a comment

Choose a reason for hiding this comment

Raffo commented Apr 6, 2022

k0da commented Apr 6, 2022

k0da commented Apr 8, 2022

Raffo commented Apr 14, 2022

Raffo commented Apr 14, 2022

k0da commented Apr 14, 2022

Raffo commented Apr 19, 2022

k8s-ci-robot commented Apr 19, 2022

msvticket commented Jun 3, 2022

cep21 commented Mar 30, 2023

haslersn commented Jul 6, 2021 •

edited

Loading

jgrumboe commented Jul 6, 2021 •

edited

Loading

haslersn commented Jul 6, 2021 •

edited

Loading

jgrumboe commented Jul 6, 2021 •

edited

Loading

k0da commented Jul 7, 2021 •

edited

Loading

haslersn commented Jul 7, 2021 •

edited

Loading