[TXT Registry] Fix handling of records produced by `toNewTXTName` in `toEndpoint` #3724

Sewci0 · 2023-06-23T11:14:35Z

The current implementation of the toEndpoint function is unable to handle records generated by the toNewTXTName function. As a result, domains created with a prefix such as %{record_type}foo. are not deleted after the associated CRD is removed.

This PR addresses this issue by improving the handling of records produced by the toNewTXTName function in the toEndpoint function. Additionally, it covers edge cases related to dashes or dots in the affix. A new test, TestToEndpointNameNewTXT, has been added to ensure the proper conversion between the output of toNewTXTName and its input using the toEndpoint function.

Additionally, this PR fixes the testTXTRegistryRecordsPrefixed test, which previously operated under the incorrect assumption that the output of toNewTXTName with the prefix txt. for dualstack.test-zone.example.org would be aaaa-txt.dualstack.test-zone.example.org. In reality when not using a templated record type, the type will be prepended to the domain rather than the affix.

DNSName := strings.SplitN(endpointDNSName, ".", 2)

if !pr.recordTypeInAffix() {
	DNSName[0] = recordT + DNSName[0]
}

Here are the results of the new test before the change:

Potentially fixes: #3007 #3186

Checklist

Unit tests updated

szuecs · 2023-06-26T09:58:34Z

/ok-to-test

szuecs · 2023-06-26T10:30:41Z

@mloiseleur can you review this one? ty

mloiseleur · 2023-06-26T15:16:34Z

@szuecs I'll check it this week.

Sewci0 · 2023-06-26T23:04:26Z

Thank you both for looking at this! As soon as this is merged it should also enable: #3735

registry/txt.go

registry/txt_test.go

registry/txt.go

johngmyers · 2023-06-27T05:34:59Z

It is unfortunate that this is as overengineered as it is. It is far more configurable than it needs to be and this leads to bugs such as the handling of %{record_type}. Why is %{record_type} even necessary in the first place?

Co-authored-by: John Gardiner Myers <jgmyers@proofpoint.com>

… into fix/to-endpoint-name

Sewci0 · 2023-06-27T08:16:03Z

It is unfortunate that this is as overengineered as it is. It is far more configurable than it needs to be and this leads to bugs such as the handling of %{record_type}. Why is %{record_type} even necessary in the first place?

It is currently the only option when working with APEX cname records, as mentioned here: #449

johngmyers · 2023-06-27T15:47:07Z

It is currently the only option when working with APEX cname records, as mentioned here: #449

That would be solved with --txt-prefix without arbitrary templating of %{record_type}, would it not?

Sewci0 · 2023-06-27T15:50:08Z

It is currently the only option when working with APEX cname records, as mentioned here: #449

That would be solved with --txt-prefix without arbitrary templating of %{record_type}, would it not?

It wouldn't as the record type gets prepended to the hostname not the affix.

external-dns/registry/txt.go

Lines 432 to 434 in bd8658e

    
           if !pr.recordTypeInAffix() { 
        
           	DNSName[0] = recordT + DNSName[0] 
        
           }

johngmyers · 2023-06-27T16:21:06Z

It wouldn't as the record type gets prepended to the hostname not the affix.

The record type demonstrably gets prepended to the prefix.

Sewci0 · 2023-06-27T16:38:42Z

It wouldn't as the record type gets prepended to the hostname not the affix.

The record type demonstrably gets prepended to the prefix.

@johngmyers I would love to see that demonstration. Running just now against the master branch:

func TestToNewTXTName(t *testing.T) {
	tests := []struct {
		name                 string
		pr                   affixNameMapper
		endpointDNSName      string
		recordType           string
		expectedNewTXTName   string
	}{
		{
			name: "Test 1",
			pr: affixNameMapper{
				prefix:              "prefix",
				suffix:              "",
			},
			endpointDNSName:    "example.com",
			recordType:         "A",
			expectedNewTXTName: "prefixa-example.com",
		},
		{
			name: "Test 2",
			pr: affixNameMapper{
				prefix:              "prefix.",
				suffix:              "",
			},
			endpointDNSName:    "test.example.com",
			recordType:         "A",
			expectedNewTXTName: "prefix.a-test.example.com",
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			result := tt.pr.toNewTXTName(tt.endpointDNSName, tt.recordType)
			assert.Equal(t, tt.expectedNewTXTName, result)
		})
	}
}

=== RUN   TestToNewTXTName
=== RUN   TestToNewTXTName/Test_1
=== RUN   TestToNewTXTName/Test_2
--- PASS: TestToNewTXTName (0.00s)
    --- PASS: TestToNewTXTName/Test_1 (0.00s)
    --- PASS: TestToNewTXTName/Test_2 (0.00s)
PASS

johngmyers · 2023-07-11T18:24:56Z

@Sewci0 would you be able to address my review comments?

Sewci0 · 2023-07-14T16:35:33Z

Hi @johngmyers, sorry it took me so long. I've been pretty busy. Thank you for your comments, I implemented all of your suggestions. Let me know your thoughts!

johngmyers

Thanks for fixing this!
/lgtm
/assign @szuecs

johngmyers · 2023-07-18T07:16:48Z

registry/txt.go

@@ -35,6 +34,12 @@ const (
 	providerSpecificForceUpdate = "txt/force-update"
 )

+type endpointKey struct {


Why not use endpoint.EndpointKey instead of redefining?

There's no such thing. Did you mean endpoint.Endpoint?

@Sewci0 it was moved recently 4417ad4.

szuecs · 2023-08-10T17:16:07Z

registry/txt.go

-func (pr affixNameMapper) dropAffix(name string) string {
+// dropAffixExtractType strips TXT record to find an endpoint name it manages
+// it also returns the record type
+func (pr affixNameMapper) dropAffixExtractType(name string) (baseName, recordType string) {


There is no use of named return values, so we can also just do:
(baseName, recordType string) -> (string, string)

If there is a blank return then we would return "","" in the current version

The problem with (string, string) is it makes it difficult to know what the strings contain. The names convey the semantics.

This could be also a comment instead of named return values, which actually define variables in scope of the func.

Named return values work better with IDEs than comments do.

szuecs · 2023-08-10T17:18:01Z

registry/txt.go

@@ -371,20 +396,24 @@ func (pr affixNameMapper) isSuffix() bool {
 	return len(pr.prefix) == 0 && len(pr.suffix) > 0
 }

-func (pr affixNameMapper) toEndpointName(txtDNSName string) (endpointName string, isAAAA bool) {
-	lowerDNSName, recordType := extractRecordType(strings.ToLower(txtDNSName))
+func (pr affixNameMapper) toEndpointName(txtDNSName string) (endpointName string, recordType string) {


same as in https://github.com/kubernetes-sigs/external-dns/pull/3724/files#r1290447277 , I would be in favor of dropping the "named return" in case we do not use it.

szuecs · 2023-08-10T17:29:58Z

@johngmyers from my side we can merge as I only have some nitpicks that we could address ourselves. As you added lgtm, I guess we are good to go.

/approve

k8s-ci-robot · 2023-08-10T17:33:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers, Sewci0, szuecs

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~~ [szuecs]

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

szuecs · 2023-08-10T19:26:24Z

hmm 2 jobs pull-external-dns-* are hanging. Any idea how to retrigger, @johngmyers ?

mloiseleur · 2023-08-10T19:34:10Z

/retest

k8s-ci-robot · 2023-08-10T20:02:34Z

@mloiseleur: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test pull-external-dns-lint
/test pull-external-dns-unit-test

Use /test all to run all jobs.

In response to this:

/retest pull-external-dns-lint

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/test-infra repository.

johngmyers · 2023-08-10T20:46:24Z

/retest

haslersn · 2024-03-27T16:31:49Z

There was an issue, which got fixed by this PR: #3031

Fix TXT registry handling of NewTXT domains

013f8cf

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 23, 2023

k8s-ci-robot requested review from johngmyers and njuettner June 23, 2023 11:14

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 23, 2023

Sewci0 changed the title ~~[TXT Registry] Fix handling of records produced by toNewTXTName function in toEndpoint~~ [TXT Registry] Fix handling of records produced by toNewTXTName function in toEndpoint Jun 23, 2023

Sewci0 changed the title ~~[TXT Registry] Fix handling of records produced by toNewTXTName function in toEndpoint~~ [TXT Registry] Fix handling of records produced by toNewTXTName in toEndpoint Jun 23, 2023

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 26, 2023

Handle old format

0f14da0

Sewci0 force-pushed the fix/to-endpoint-name branch from fe02767 to 0f14da0 Compare June 26, 2023 14:20

Sewci0 mentioned this pull request Jun 26, 2023

[TXT Registry] Disable generation of old style txt records when encryption is enabled #3735

Closed

1 task

Simplify record type handling

74be95a

Sewci0 force-pushed the fix/to-endpoint-name branch from ac75b88 to 74be95a Compare June 26, 2023 23:01

johngmyers suggested changes Jun 27, 2023

View reviewed changes

registry/txt.go Outdated Show resolved Hide resolved

registry/txt_test.go Show resolved Hide resolved

registry/txt_test.go Outdated Show resolved Hide resolved

registry/txt.go Outdated Show resolved Hide resolved

registry/txt.go Show resolved Hide resolved

k8s-ci-robot assigned johngmyers Jun 27, 2023

Sewci0 and others added 3 commits June 27, 2023 08:57

Add guardrails for AAAA records

f2c08ac

Update registry/txt.go

480979b

Co-authored-by: John Gardiner Myers <jgmyers@proofpoint.com>

Merge branch 'fix/to-endpoint-name' of github.com:Sewci0/external-dns…

e373685

… into fix/to-endpoint-name

Sewci0 closed this Jun 27, 2023

Sewci0 reopened this Jun 27, 2023

johngmyers mentioned this pull request Jul 7, 2023

WIP Always create AAAA alias records in route53 #3605

Open

2 tasks

Sewci0 added 5 commits July 14, 2023 17:05

Extract prefix and suffix to separate variables

fa38920

Disable generation of old style txt records when using recordTypeInAffix

f04457d

Add multiple dots test

1fad07b

Revert elements match

e8ee831

Implement endpoint key

8cd7c95

Make the tests more verbose

1ffcc5d

Sewci0 force-pushed the fix/to-endpoint-name branch from e76c681 to 1ffcc5d Compare July 14, 2023 16:49

Sewci0 requested a review from johngmyers July 17, 2023 12:12

johngmyers approved these changes Jul 18, 2023

View reviewed changes

k8s-ci-robot assigned szuecs Jul 18, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2023

johngmyers mentioned this pull request Jul 19, 2023

feat: add 'txt-generate-legacy-entry' to disable only txt format generation #3796

Closed

2 tasks

szuecs reviewed Aug 10, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2023

k8s-ci-robot merged commit 380e8b6 into kubernetes-sigs:master Aug 10, 2023
1 check passed

Sewci0 mentioned this pull request Sep 4, 2023

[TXT Registry] Fix Records() case handling #3912

Merged

1 task

johngmyers mentioned this pull request Sep 13, 2023

Add johngmyers as approver #3931

Merged

2 tasks

This was referenced Mar 27, 2024

Endpoints with templated prefix are not deleted #3031

Closed

Fix templated prefix for updates and deletes #3032

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TXT Registry] Fix handling of records produced by `toNewTXTName` in `toEndpoint` #3724

[TXT Registry] Fix handling of records produced by `toNewTXTName` in `toEndpoint` #3724

Sewci0 commented Jun 23, 2023 •

edited

Loading

szuecs commented Jun 26, 2023

szuecs commented Jun 26, 2023

mloiseleur commented Jun 26, 2023

Sewci0 commented Jun 26, 2023 •

edited

Loading

johngmyers commented Jun 27, 2023

Sewci0 commented Jun 27, 2023

johngmyers commented Jun 27, 2023

Sewci0 commented Jun 27, 2023

johngmyers commented Jun 27, 2023

Sewci0 commented Jun 27, 2023

johngmyers commented Jul 11, 2023

Sewci0 commented Jul 14, 2023

johngmyers left a comment

johngmyers Jul 18, 2023

Sewci0 Jul 18, 2023

sathieu Jul 26, 2023

szuecs Aug 10, 2023

johngmyers Aug 10, 2023

szuecs Aug 10, 2023

johngmyers Aug 11, 2023

szuecs Aug 10, 2023

szuecs commented Aug 10, 2023

k8s-ci-robot commented Aug 10, 2023

szuecs commented Aug 10, 2023

mloiseleur commented Aug 10, 2023 •

edited

Loading

k8s-ci-robot commented Aug 10, 2023

johngmyers commented Aug 10, 2023

haslersn commented Mar 27, 2024

[TXT Registry] Fix handling of records produced by toNewTXTName in toEndpoint #3724

[TXT Registry] Fix handling of records produced by toNewTXTName in toEndpoint #3724

Conversation

Sewci0 commented Jun 23, 2023 • edited Loading

szuecs commented Jun 26, 2023

szuecs commented Jun 26, 2023

mloiseleur commented Jun 26, 2023

Sewci0 commented Jun 26, 2023 • edited Loading

johngmyers commented Jun 27, 2023

Sewci0 commented Jun 27, 2023

johngmyers commented Jun 27, 2023

Sewci0 commented Jun 27, 2023

johngmyers commented Jun 27, 2023

Sewci0 commented Jun 27, 2023

johngmyers commented Jul 11, 2023

Sewci0 commented Jul 14, 2023

johngmyers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szuecs commented Aug 10, 2023

k8s-ci-robot commented Aug 10, 2023

szuecs commented Aug 10, 2023

mloiseleur commented Aug 10, 2023 • edited Loading

k8s-ci-robot commented Aug 10, 2023

johngmyers commented Aug 10, 2023

haslersn commented Mar 27, 2024

[TXT Registry] Fix handling of records produced by `toNewTXTName` in `toEndpoint` #3724

[TXT Registry] Fix handling of records produced by `toNewTXTName` in `toEndpoint` #3724

Sewci0 commented Jun 23, 2023 •

edited

Loading

Sewci0 commented Jun 26, 2023 •

edited

Loading

mloiseleur commented Aug 10, 2023 •

edited

Loading