fix(endpoint): ensure trailing dot to target as required by RFC 2782

[frittentheke]

What does it do ?

According to RFC2782 [1], SRV records are to be fully-qualified, ending with a final dot. Most providers are liberal, but e.g. OpenStack Designate expects the record name and targets to end with a dot ([2], [3])

[1] https://datatracker.ietf.org/doc/html/rfc2782
[2] https://github.com/openstack/designate/blob/698f7b605926ab26eb1577f8728f77c71e2fbda1/designate/tests/unit/objects/test_rrdata_srv.py
[3] https://github.com/openstack/designate/blob/18a5d4f4d9e6400fd1be97695b253d1acf920555/designate/objects/fields.py#L231-L243

Motivation

Fixes #5476, Fixes #5389, Fixes #5343, Fixes inovex/external-dns-openstack-webhook#50

More

• Yes, this PR title follows Conventional Commits
• Yes, I added unit tests
• Yes, I updated end user documentation accordingly

[k8s-ci-robot]

Hi @frittentheke. 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.

Details

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]

/ok-to-test

[ivankatliarchuk]

This change requrie a unit test. Otherwise nothing stopping to change this lines

[ivankatliarchuk]

Need to fix commit message(s) as well

[latituder]

@ivankatliarchuk
it seems the PR is stuck in step EasyCLA. how to getting the authorize of CLA of this bugfix?

[ivankatliarchuk]

@ivankatliarchuk it seems the PR is stuck in step EasyCLA. how to getting the authorize of CLA of this bugfix?

No idea. This is set on Kubernetes org level, I have no visibility to it

[ivankatliarchuk]

/retest

[ivankatliarchuk]

/easycla

[frittentheke]

Sorry for the delay on this PR @ivankatliarchuk ... I updated this PR with your suggestions and updated the tests to expect trailing dots now.

But ... this is still not working due to:

• external-dns/endpoint/endpoint.go

  Line 255 in 1956c7e

  cleanTargets[idx] = strings.TrimSuffix(target, ".")

• external-dns/endpoint/endpoint.go

  Line 266 in 1956c7e

  DNSName: strings.TrimSuffix(dnsName, "."),

removing the trailing dots again.

Looking ahead ... there also is

external-dns/endpoint/endpoint.go

Lines 459 to 478 in 1956c7e

	func (t Targets) ValidateSRVRecord() bool {
	for _, target := range t {
	// SRV records must have a priority, weight, and port value, e.g. "10 5 5060 example.com"
	// as per https://www.rfc-editor.org/rfc/rfc2782.txt
	targetParts := strings.Fields(strings.TrimSpace(target))
	if len(targetParts) != 4 {
	log.Debugf("Invalid SRV record target: %s. SRV records must have a priority, weight, and port value, e.g. '10 5 5060 example.com'", target)
	return false
	}
	for _, part := range targetParts[:3] {
	_, err := strconv.ParseUint(part, 10, 16)
	if err != nil {
	log.Debugf("Invalid SRV record target: %s. Invalid integer value in target.", target)
	return false
	}
	}
	}
	return true
	}

which validates existing records.
This function also might need changes to (also?) accept SRV records with (actually required) trailing dots.

Please kindly advise @ivankatliarchuk

[ivankatliarchuk]

According to RFC 2782, the target field of an SRV record must be a fully-qualified domain name (FQDN) — and FQDNs in DNS zone files are conventionally written with a trailing dot (.) to signify that the name is absolute, not relative.

So changing record most likely could be a breaking change. I personally usually using it with trailing dots as DNS resolution is way faster. But still

[ivankatliarchuk]

You could refine or tighten the func (t Targets) ValidateSRVRecord() bool implementation in a follow-up PR. It will likely require updates in other sources as well. You could also consider introducing something similar to MXType (see this example) to support that in subsequent PRs if have any apetite to improve SRV record lifecycle.

[frittentheke]

According to RFC 2782, the target field of an SRV record must be a fully-qualified domain name (FQDN) — and FQDNs in DNS zone files are conventionally written with a trailing dot (.) to signify that the name is absolute, not relative.

So changing record most likely could be a breaking change.

Does this then need some sort of release note?

What is your suggestion on changes to the code then to first have the trailing dots end up in the records?
I suppose the mentioned sections ...

• external-dns/endpoint/endpoint.go

  Line 255 in 1956c7e

  cleanTargets[idx] = strings.TrimSuffix(target, ".")

• external-dns/endpoint/endpoint.go

  Line 266 in 1956c7e

  DNSName: strings.TrimSuffix(dnsName, "."),

are where changes need to happen.

But I am really puzzled whether I should add some conditional check in this very function and

a) if the record type is SRV skip the cleanup
b) add an option (defaulting to false) in the method signature to allow the caller to skip the cleanup
c) Something else I did not think of?

I just don't want to beat the string until it suits me (or the SRV RFC), thus I am looking for you guidance @ivankatliarchuk on keeping things tidy.

[frittentheke]

The method

external-dns/endpoint/endpoint.go

Line 475 in f779395

func (t Targets) ValidateSRVRecord() bool {

may need to be adjusted as well, aka add correct check for a dot

You are right @ivankatliarchuk. I just extended the check added by #4871 to ensure there is a dot.

[coveralls]

Pull Request Test Coverage Report for Build 19329339527

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 26 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.006%) to 78.708%

---

Files with Coverage Reduction	New Missed Lines	%
endpoint.go	6	90.6%
service.go	20	93.33%

Totals
Change from base Build 19325329769:	0.006%
Covered Lines:	15906
Relevant Lines:	20209

---

💛 - Coveralls

[frittentheke]

@ivankatliarchuk sorry for the long delay in addressing the review remarks.
I believe (!) I have tackled them all, PTAL.

[ivankatliarchuk]

/lgtm

[mloiseleur]

/approve

[frittentheke]

@ivankatliarchuk @mloiseleur apparently there is some test failing - see #5534 (comment)

I just pushed a change in believing this is the right place. If not, I'd love some guidance on where this test data actually lives.

[mloiseleur]

@frittentheke Since this PR has been merged, you'll need to update this PR accordingly.

[frittentheke]

@frittentheke Since this PR has been merged, you'll need to update this PR accordingly.

ahhh sorry @mloiseleur, it was obviously too late yesterday. I missed that there might have been changes to master since my PR was branched off 🙈 I shall get to work and fix this then.

[frittentheke]

Hope this look good now @mloiseleur @ivankatliarchuk ... PTAL.

[frittentheke]

Could I kindly ask to take another peek at this PR @mloiseleur @ivankatliarchuk @szuecs ?

[ivankatliarchuk]

I only have a mobile phone. My laptop needs fixing. Maybe in a few weeks I will get it back.

[mloiseleur]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mloiseleur

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [mloiseleur]

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