fix: append dot to recordName and 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])

[1] https://datatracker.ietf.org/doc/html/rfc2782 [2] https://github.com/openstack/designate/blob/698f7b605926ab26eb1577f8728f77c71e2fbda1/designate/tests/unit/objects/test_rrdata_srv.py

Motivation

Fixes https://github.com/kubernetes-sigs/external-dns/issues/5476, Fixes https://github.com/kubernetes-sigs/external-dns/issues/5389, Fixes https://github.com/kubernetes-sigs/external-dns/issues/5343, Fixes https://github.com/inovex/external-dns-openstack-webhook/issues/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.

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:

• https://github.com/kubernetes-sigs/external-dns/blob/1956c7e2df7807237c13de064750f003014a3bc0/endpoint/endpoint.go#L255
• https://github.com/kubernetes-sigs/external-dns/blob/1956c7e2df7807237c13de064750f003014a3bc0/endpoint/endpoint.go#L266

removing the trailing dots again.

Looking ahead ... there also is https://github.com/kubernetes-sigs/external-dns/blob/1956c7e2df7807237c13de064750f003014a3bc0/endpoint/endpoint.go#L459-L478 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 ...

• https://github.com/kubernetes-sigs/external-dns/blob/1956c7e2df7807237c13de064750f003014a3bc0/endpoint/endpoint.go#L255

• https://github.com/kubernetes-sigs/external-dns/blob/1956c7e2df7807237c13de064750f003014a3bc0/endpoint/endpoint.go#L266

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.

[ivankatliarchuk]

There is no need to touch DNS name, is not part of RFC, dot is optional there. If you really want to do that, you could do, if SRV then ... else ...

[latituder]

sorry, if the dot is optional, then how to use external-dns to create a SRV record with the dot in the openstack Designate ?

[frittentheke]

sorry, if the dot is optional, then how to use external-dns to create a SRV record with the dot in the openstack Designate ?

@latituder Could you please check if and where Designate enforces the dot at this position of the record? I also like to be liberal in what to accept but strict in what we send if we agree that always adding this dot follows this principle just fine ...

I shall l look into another revision of this PR later today ... sorry about the delay.

[ivankatliarchuk]

sorry, if the dot is optional, then how to use external-dns to create a SRV record with the dot in the openstack Designate ?

Please clarify which dot you have in mind. Dot in DNSName or in Target?

[frittentheke]

@latituder sorry for the delay. I just pushed another revision. Could you kindly test in your environment if Designate is happy now with the "." at the end of the SRV target?

Initially I was confused about a required final dot also for the record name, as Designate wants dots there as well: https://github.com/openstack/designate/blob/stable/2025.1/designate/objects/fields.py#L233-L234. But since this check is universal for all records (https://github.com/openstack/designate/blob/a891d81974a4c3aa56a143252b26df9dcbd5fba5/designate/objects/fields.py#L202-L218) this must be handled somewhere else (Webhook, Gophcloud, ...).

[ivankatliarchuk]

The method https://github.com/kubernetes-sigs/external-dns/blob/f7793950cc96aaab024f145b34600f782dccb7f0/endpoint/endpoint.go#L475 may need to be adjusted as well, aka add correct check for a dot

[frittentheke]

The method

https://github.com/kubernetes-sigs/external-dns/blob/f7793950cc96aaab024f145b34600f782dccb7f0/endpoint/endpoint.go#L475 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 https://github.com/kubernetes-sigs/external-dns/pull/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%
<!--	Total:	26

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.

[mloiseleur]

/approve

[frittentheke]

@ivankatliarchuk @mloiseleur apparently there is some test failing - see https://github.com/kubernetes-sigs/external-dns/pull/5534#issuecomment-3517797962

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 :see_no_evil: 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.

[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

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